[libcamera-devel] [PATCH v3] gstreamer: Maintain a control list to track libcamerasrc control properties

[Umang Jain]

On 6/29/23 12:59 AM, Kieran Bingham wrote:
> Quoting Umang Jain via libcamera-devel (2023-06-28 22:34:52)
>> As more and more libcamera controls get plumbed, more member variables
>> get introduced in struct GstLibcameraSrc. Instead of doing that, now
>> maintain a single ControlList which is more appropriate to keep track
>> of controls that get sets on libcamerasrc.
>>
>> This also brings easy validation of controls set on libcamerasrc. If
>> the controls are not supported by camera, fail negotiation and report
>> what is not supported.
>>
>> The current patch migrates previously introduced control,
>> auto-focus-mode, to be set directly in control list.
>>
>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>> ---
>> Changes in v3:
>> - Rebase over "[PATCH] gstreamer: Drop libcamera_private dependency"
>>
>> Changes in v2:
>> - Edit commit message
>> - Drop a break; so that all unsupported user-provided controls are
>>    reported at once.
>> ---
>>   src/gstreamer/gstlibcamerasrc.cpp | 44 ++++++++++++++++++++++---------
>>   src/gstreamer/gstlibcamerasrc.h   |  6 ++---
>>   2 files changed, 35 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
>> index f764a87a..b5c6e9ab 100644
>> --- a/src/gstreamer/gstlibcamerasrc.cpp
>> +++ b/src/gstreamer/gstlibcamerasrc.cpp
>> @@ -142,7 +142,7 @@ struct _GstLibcameraSrc {
>>          GstTask *task;
>>   
>>          gchar *camera_name;
>> -       controls::AfModeEnum auto_focus_mode = controls::AfModeManual;
>> +       ControlList *controls;
>>   
>>          GstLibcameraSrcState *state;
>>          GstLibcameraAllocator *allocator;
>> @@ -458,7 +458,9 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
>>          GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);
>>          GLibRecLocker lock(&self->stream_lock);
>>          GstLibcameraSrcState *state = self->state;
>> +       const ControlInfoMap &info_map = state->cam_->controls();
>>          GstFlowReturn flow_ret = GST_FLOW_OK;
>> +       gboolean supported_ctrl = true;
>>          gint ret;
>>   
>>          g_autoptr(GstStructure) element_caps = gst_structure_new_empty("caps");
>> @@ -575,18 +577,30 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
>>                  gst_flow_combiner_add_pad(self->flow_combiner, srcpad);
>>          }
>>   
>> -       if (self->auto_focus_mode != controls::AfModeManual) {
>> -               const ControlInfoMap &infoMap = state->cam_->controls();
>> -               if (infoMap.find(&controls::AfMode) != infoMap.end()) {
>> -                       state->initControls_.set(controls::AfMode, self->auto_focus_mode);
>> -               } else {
>> +       /*
>> +        * Check the controls set on libcamerasrc is supported by the camera.
>> +        * This can be easily done by CameraControlValidator but it is an internal
>> +        * libcamera class. Hence avoid adding a internal header even though
>> +        * gstreamer links with libcamera-private.
> I think we can remove this comment now or simplify it.

v3 was specifically sent to remove this comment, but apparently I send 
the old .patch file :-/
>
> No point mentioning anything to do with libcamera-private.
>
> Other than that, this seems like a good step forwards for getting more
> controls in.
>
> --
> Kieran
>
>
>
>> +        */
>> +       for (auto iter = self->controls->begin(); iter != self->controls->end(); iter++) {
>> +               if (info_map.find(iter->first) == info_map.end()) {
>> +                       const ControlIdMap *id_map = self->controls->idMap();
>>                          GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
>> -                                         ("Failed to enable auto focus"),
>> -                                         ("AfMode not supported by this camera, "
>> -                                          "please retry with 'auto-focus-mode=AfModeManual'"));
>> +                                         ("Failed to set %s", id_map->at(iter->first)->name().c_str()),
>> +                                         ("%s not supported by this camera, ", id_map->at(iter->first)->name().c_str()));
>> +                       supported_ctrl = false;
>>                  }
>>          }
>>   
>> +       if (!supported_ctrl) {
>> +               gst_task_stop(task);
>> +               flow_ret = GST_FLOW_NOT_NEGOTIATED;
>> +               goto done;
>> +       }
>> +
>> +       state->initControls_.merge(*self->controls);
>> +
>>          ret = state->cam_->start(&state->initControls_);
>>          if (ret) {
>>                  GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
>> @@ -670,7 +684,8 @@ gst_libcamera_src_set_property(GObject *object, guint prop_id,
>>                  self->camera_name = g_value_dup_string(value);
>>                  break;
>>          case PROP_AUTO_FOCUS_MODE:
>> -               self->auto_focus_mode = static_cast<controls::AfModeEnum>(g_value_get_enum(value));
>> +               self->controls->set(controls::AfMode,
>> +                                   static_cast<controls::AfModeEnum>(g_value_get_enum(value)));
>>                  break;
>>          default:
>>                  G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
>> @@ -689,9 +704,11 @@ gst_libcamera_src_get_property(GObject *object, guint prop_id, GValue *value,
>>          case PROP_CAMERA_NAME:
>>                  g_value_set_string(value, self->camera_name);
>>                  break;
>> -       case PROP_AUTO_FOCUS_MODE:
>> -               g_value_set_enum(value, static_cast<gint>(self->auto_focus_mode));
>> +       case PROP_AUTO_FOCUS_MODE: {
>> +               auto auto_focus_mode = self->controls->get(controls::AfMode).value_or(controls::AfModeManual);
>> +               g_value_set_enum(value, auto_focus_mode);
>>                  break;
>> +       }
>>          default:
>>                  G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
>>                  break;
>> @@ -757,6 +774,7 @@ gst_libcamera_src_finalize(GObject *object)
>>          g_clear_object(&self->task);
>>          g_mutex_clear(&self->state->lock_);
>>          g_free(self->camera_name);
>> +       delete self->controls;
>>          delete self->state;
>>   
>>          return klass->finalize(object);
>> @@ -782,6 +800,8 @@ gst_libcamera_src_init(GstLibcameraSrc *self)
>>          /* C-style friend. */
>>          state->src_ = self;
>>          self->state = state;
>> +
>> +       self->controls = new ControlList(controls::controls, nullptr);
>>   }
>>   
>>   static GstPad *
>> diff --git a/src/gstreamer/gstlibcamerasrc.h b/src/gstreamer/gstlibcamerasrc.h
>> index 0a88ba02..2e6d74cb 100644
>> --- a/src/gstreamer/gstlibcamerasrc.h
>> +++ b/src/gstreamer/gstlibcamerasrc.h
>> @@ -26,17 +26,17 @@ gst_libcamera_auto_focus_get_type()
>>          static GType type = 0;
>>          static const GEnumValue values[] = {
>>                  {
>> -                       static_cast<gint>(libcamera::controls::AfModeManual),
>> +                       libcamera::controls::AfModeManual,
>>                          "AfModeManual",
>>                          "manual-focus",
>>                  },
>>                  {
>> -                       static_cast<gint>(libcamera::controls::AfModeAuto),
>> +                       libcamera::controls::AfModeAuto,
>>                          "AfModeAuto",
>>                          "automatic-auto-focus",
>>                  },
>>                  {
>> -                       static_cast<gint>(libcamera::controls::AfModeContinuous),
>> +                       libcamera::controls::AfModeContinuous,
>>                          "AfModeContinuous",
>>                          "continuous-auto-focus",
>>                  },
>> -- 
>> 2.39.1
>>