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

[Kieran Bingham]

Hi Umang,

Quoting Umang Jain via libcamera-devel (2023-06-22 12:17:17)
> 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, abort early and report what is

I would change 'abort' to 'fail negotiation' as I thought from reading
this there was going to be a call to abort().

> 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>
> ---
> Cedric, Can you please help test this?
> 
> (I'm travelling so have tested this on my RPi and inferring from
> libcamera/gstreamer logs, it seems to have not broken anything)
> 
> An additional test from your side would be appreciated.
> ---
>  src/gstreamer/gstlibcamerasrc.cpp | 45 ++++++++++++++++++++++---------
>  src/gstreamer/gstlibcamerasrc.h   |  6 ++---
>  2 files changed, 36 insertions(+), 15 deletions(-)
> 
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index 47d8ff43..a2266310 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -146,7 +146,7 @@ struct _GstLibcameraSrc {
>         GstTask *task;
>  
>         gchar *camera_name;
> -       controls::AfModeEnum auto_focus_mode = controls::AfModeManual;
> +       ControlList *controls;
>  
>         GstLibcameraSrcState *state;
>         GstLibcameraAllocator *allocator;
> @@ -462,7 +462,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");
> @@ -579,18 +581,31 @@ 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.
> +        */
> +       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()));

Isn't this printing the name twice? Maybe this could be a single output
line.

> +                       supported_ctrl = false;
> +                       break;

I would probably not break here - as if there are multiple controls that
fail to validate - it might make sense to report all of them at once.


>                 }
>         }
>  
> +       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,
> @@ -674,7 +689,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);
> @@ -693,9 +709,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;
> @@ -760,6 +778,7 @@ gst_libcamera_src_finalize(GObject *object)
>         g_rec_mutex_clear(&self->stream_lock);
>         g_clear_object(&self->task);
>         g_free(self->camera_name);
> +       delete self->controls;

I would have said can we use a unique_ptr, but it doesn't look like that
makes sense without reworking the rest of the structure...

>         delete self->state;
>  
>         return klass->finalize(object);
> @@ -783,6 +802,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,

Nice to see the cast is no longer needed. Is that a result of a change
here, or was it already possible. ?

>                         "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
>