[libcamera-devel] [PATCH] Add enable_auto_focus option to the GStreamer plugin

[Nicolas Dufresne]

Hi,

Le vendredi 26 mai 2023 à 14:34 +0200, Cedric Nugteren via libcamera-devel a
écrit :
> Cameras such as the PiCam 3 support auto-focus, but the GStreamer plugin
> for libcamera does not enable auto-focus. With this patch auto-focus can
> be enabled for cameras that support it. By default it is disabled, which
> means default behaviour remains unchanged. For cameras that do not
> support auto-focus, an error message shows up if it auto-focus is
> enabled.

I'm not sure this default behaviour make sense. I would rather have it enabled
by default, since there is not other offered mean at the moment to obtain this
focus.

That being said, I haven't looked at the auto-focus feature, I was expecting
there would be different type of it, which would endup with different default
behaviour.

> 
> This was tested on cameras that do not support auto-focus (e.g. PiCam2)
> and was tested on a camera that does support auto-focus (PiCam3) by
> enabling it, observing auto-focus, and by disabling it or by not setting
> the option, both resulting in auto-focus being disabled.
> 
> Signed-off-by: Cedric Nugteren <web at cedricnugteren.nl>
> ---
>  src/gstreamer/gstlibcameraprovider.cpp | 13 ++++++++++++
>  src/gstreamer/gstlibcamerasrc.cpp      | 29 +++++++++++++++++++++++++-
>  2 files changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/src/gstreamer/gstlibcameraprovider.cpp
> b/src/gstreamer/gstlibcameraprovider.cpp
> index 6eb0a0eb..86fa2542 100644
> --- a/src/gstreamer/gstlibcameraprovider.cpp
> +++ b/src/gstreamer/gstlibcameraprovider.cpp
> @@ -31,6 +31,7 @@ GST_DEBUG_CATEGORY_STATIC(provider_debug);
>  
>  enum {
>   PROP_DEVICE_NAME = 1,
> + PROP_ENABLE_AUTO_FOCUS = 2,
>  };
>  
>  #define GST_TYPE_LIBCAMERA_DEVICE gst_libcamera_device_get_type()
> @@ -40,6 +41,7 @@ G_DECLARE_FINAL_TYPE(GstLibcameraDevice,
> gst_libcamera_device,
>  struct _GstLibcameraDevice {
>   GstDevice parent;
>   gchar *name;
> + gboolean enable_auto_focus = false;
>  };
>  
>  G_DEFINE_TYPE(GstLibcameraDevice, gst_libcamera_device, GST_TYPE_DEVICE)
> @@ -56,6 +58,7 @@ gst_libcamera_device_create_element(GstDevice *device, const
> gchar *name)
>   g_assert(source);
>  
>   g_object_set(source, "camera-name", GST_LIBCAMERA_DEVICE(device)->name,
> nullptr);
> + g_object_set(source, "enable-auto-focus", GST_LIBCAMERA_DEVICE(device)-
> >enable_auto_focus, nullptr);
>  
>   return source;
>  }
> @@ -68,6 +71,7 @@ gst_libcamera_device_reconfigure_element(GstDevice *device,
>   return FALSE;
>  
>   g_object_set(element, "camera-name", GST_LIBCAMERA_DEVICE(device)->name,
> nullptr);
> + g_object_set(element, "enable-auto-focus", GST_LIBCAMERA_DEVICE(device)-
> >enable_auto_focus, nullptr);

That is strange, unlike camera-name, this information is not discovered at run-
time, hence does not require a notify callback. I'd simply set the default at
construction time, and avoid this here.

>  
>   return TRUE;
>  }
> @@ -82,6 +86,9 @@ gst_libcamera_device_set_property(GObject *object, guint
> prop_id,
>   case PROP_DEVICE_NAME:
>   device->name = g_value_dup_string(value);
>   break;
> + case PROP_ENABLE_AUTO_FOCUS:
> + device->enable_auto_focus = g_value_get_boolean(value);
> + break;
>   default:
>   G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
>   break;
> @@ -121,6 +128,12 @@ gst_libcamera_device_class_init(GstLibcameraDeviceClass
> *klass)
>   (GParamFlags)(G_PARAM_STATIC_STRINGS | G_PARAM_WRITABLE |
>        G_PARAM_CONSTRUCT_ONLY));
>   g_object_class_install_property(object_class, PROP_DEVICE_NAME, pspec);
> + GParamSpec *spec2 = g_param_spec_boolean("enable-auto-focus",

Why a new one ? just reuse spec here, its just temporary pointer with no
ownership in it. It was only added to be able to fit into the maximum line
length.

> +                        "Enable auto-focus",
> +                        "Enable auto-focus if set to true, "
> +                        "disable it if set to false",
> +                         FALSE, G_PARAM_WRITABLE);
> + g_object_class_install_property(object_class, PROP_ENABLE_AUTO_FOCUS,
> spec2);

Note, if you feel like it, you could make it more obvious by using
g_steal_pointer() here, which is similar to std::move (but works for non C++
pointers).

>  }
>  
>  static GstDevice *
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp
> b/src/gstreamer/gstlibcamerasrc.cpp
> index a10cbd4f..672ea38a 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -146,6 +146,7 @@ struct _GstLibcameraSrc {
>   GstTask *task;
>  
>   gchar *camera_name;
> + gboolean enable_auto_focus = false;

I guess I've been thinking C a lot so far, thanks for making me noticed we can
do that. Note a little inconstancy, gboolean is an in. So I'd suggest:

+ bool enable_auto_focus = false;

>  
>   GstLibcameraSrcState *state;
>   GstLibcameraAllocator *allocator;
> @@ -154,7 +155,8 @@ struct _GstLibcameraSrc {
>  
>  enum {
>   PROP_0,
> - PROP_CAMERA_NAME
> + PROP_CAMERA_NAME,
> + PROP_ENABLE_AUTO_FOCUS,
>  };
>  
>  G_DEFINE_TYPE_WITH_CODE(GstLibcameraSrc, gst_libcamera_src, GST_TYPE_ELEMENT,
> @@ -577,6 +579,18 @@ gst_libcamera_src_task_enter(GstTask *task,
> [[maybe_unused]] GThread *thread,
>   gst_flow_combiner_add_pad(self->flow_combiner, srcpad);
>   }
>  
> + if (self->enable_auto_focus) {
> + const ControlInfoMap &infoMap = state->cam_->controls();
> + if (infoMap.find(&controls::AfMode) != infoMap.end()) {
> + state->initControls_.set(controls::AfMode, controls::AfModeContinuous);
> + } else {
> + GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
> + ("Failed to enable auto focus"),
> + ("AfMode not found in camera controls, "
> + "please retry with 'enable-auto-focus=false'"));
> + }
> + }
> +

Please resend in text form as you are suppose to (I recommend using git send-
email). This will ensure we don't smash the indentation completely. I'll stop my
review here, as its too hard to read without indentation.

regards,
Nicolas

>   ret = state->cam_->start(&state->initControls_);
>   if (ret) {
>   GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
> @@ -659,6 +673,9 @@ gst_libcamera_src_set_property(GObject *object, guint
> prop_id,
>   g_free(self->camera_name);
>   self->camera_name = g_value_dup_string(value);
>   break;
> + case PROP_ENABLE_AUTO_FOCUS:
> + self->enable_auto_focus = g_value_get_boolean(value);
> + break;
>   default:
>   G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
>   break;
> @@ -676,6 +693,9 @@ 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_ENABLE_AUTO_FOCUS:
> + g_value_set_boolean(value, self->enable_auto_focus);
> + break;
>   default:
>   G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
>   break;
> @@ -844,4 +864,11 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)
>       | G_PARAM_READWRITE
>       | G_PARAM_STATIC_STRINGS));
>   g_object_class_install_property(object_class, PROP_CAMERA_NAME, spec);
> + GParamSpec *spec2 = g_param_spec_boolean("enable-auto-focus",
> +                        "Enable auto-focus",
> +                        "Enable auto-focus if set to true, "
> +                        "disable it if set to false",
> +                         FALSE, G_PARAM_WRITABLE);
> + g_object_class_install_property(object_class, PROP_ENABLE_AUTO_FOCUS,
> spec2);
> +
>  }