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

[Laurent Pinchart]

Hello,

(CC'ing Naush and David)

On Mon, May 29, 2023 at 02:09:39PM -0400, Nicolas Dufresne via libcamera-devel wrote:
> 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.

I think enabling it by default makes sense, but that leads me to another
question: why is auto-focus not enabled by defaut (for cameras that
support it) in the Raspberry Pi IPA module ?

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

I strongly recommend using git-send-email to send patches as well.
Instructions are available at
https://libcamera.org/contributing.html#submitting-patches, which links
to the very nice https://git-send-email.io/ helper to setup
git-send-email.

> >   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);
> > +
> >  }

-- 
Regards,

Laurent Pinchart