[libcamera-devel] [PATCH v2] Add enable_auto_focus option to the GStreamer plugin
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jun 5 20:26:29 CEST 2023
Hi Nicolas,
On Mon, Jun 05, 2023 at 01:54:28PM -0400, Nicolas Dufresne wrote:
> Le lundi 05 juin 2023 à 19:01 +0300, Laurent Pinchart a écrit :
> > On Mon, Jun 05, 2023 at 11:47:51AM -0400, Nicolas Dufresne via libcamera-devel wrote:
> > > Le lundi 05 juin 2023 à 16:44 +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.
> > > >
> > > > 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.
> > > >
> > > > This should close https://bugs.libcamera.org/show_bug.cgi?id=188.
> > > >
> > > > Signed-off-by: Cedric Nugteren <web at cedricnugteren.nl>
> > > > ---
> > > > Changes since v1:
> > > > - Now re-using the GParamSpec variables instead of re-creating.
> > > > - Use bool instead of gboolean for enable_auto_focus.
> > > > - Remove changes from the reconfigure_element functions.
> > > > ---
> > > > src/gstreamer/gstlibcameraprovider.cpp | 12 +++++++++++
> > > > src/gstreamer/gstlibcamerasrc.cpp | 29 +++++++++++++++++++++++++-
> > > > 2 files changed, 40 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/gstreamer/gstlibcameraprovider.cpp b/src/gstreamer/gstlibcameraprovider.cpp
> > > > index 6eb0a0eb..579cb8c0 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;
> > > > + bool 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;
> > > > }
> > > > @@ -82,6 +85,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 +127,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);
> > > > + pspec = 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, pspec);
> > > > }
> > > >
> > > > static GstDevice *
> > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> > > > index a10cbd4f..b2d8456b 100644
> > > > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > > > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > > > @@ -146,6 +146,7 @@ struct _GstLibcameraSrc {
> > > > GstTask *task;
> > > >
> > > > gchar *camera_name;
> > > > + 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);
> > >
> > > I don't really like the idea of mapping modes to a boolean here. I think this
> > > should be an GEnum type of property. Some mode requires some actions to actually
> > > work, best to just skip these for now.
> >
> > In the long run, we need to map all libcamera controls to GStreamer
> > properties. Given the amount of controls, I'm hoping we will be able to
> > either do so in a dynamic way (similar to what v4l2src does when mapping
> > V4L2 controls), or with code generation. That's why I'm not sure if we
> > need to spend more time now on mapping better this particular control
> > manually (as long as we don't need to preserve backward compatibility).
> > I trust your judgement on this topic.
>
> Properties are part of the API, so renaming them changing their type will break
> applications using it. I'd strongly suggest to make this an enum (this is easy)
> with the two supporting values. Adding more enums values later is not a break.
I'm fine with that, but note that we will have API breakages, as the
libcamera controls are not stable yet and will be changed.
> > > > + } 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'"));
> > > > + }
> > > > + }
> > > > +
> > > > 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);
> > > > + spec = 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, spec);
> > > > +
> > > > }
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list