[libcamera-devel] [PATCH v2] Add enable_auto_focus option to the GStreamer plugin
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jun 5 18:04:55 CEST 2023
Hi Nicolas,
On Mon, Jun 05, 2023 at 11:56:56AM -0400, Nicolas Dufresne wrote:
> Le lundi 05 juin 2023 à 18:33 +0300, Laurent Pinchart a écrit :
> > Hi Cedric,
> >
> > (CC'ing Nicolas)
> >
> > Thank you for the patch.
> >
> > On Mon, Jun 05, 2023 at 04:44:05PM +0200, Cedric Nugteren via libcamera-devel wrote:
> > > 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
> >
> > I think you meant "its auto-focus" instead of "it auto-focus" ?
> >
> > > 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.
> >
> > This should be written
> >
> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=188.
> >
> > which I realize isn't documented anywhere :-S I wonder where to best
> > mention this. Is there any particular place in the documentation that
> > would have caught your eyes ?
> >
> > > 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.
> >
> > Perfect :-)
> >
> > > ---
> > > 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);
> >
> > Nicolas, is there any particular rule when it comes to naming properties
> > for GStreamer elements ?
>
> The GLib part of it is:
>
> A property name consists of one or more segments consisting of ASCII letters and
> digits, separated by either the - or _ character. The first character of a
> property name must be a letter. These are the same rules as for signal naming
> (see g_signal_new()).
>
> I strongly prefer - instead of _. Generally speaking, I would like this
> discussion to happen before we start adding controls. One of the thing I was
> considering was to auto-generate the propertly from the yaml,
Seems like we have the same goal :-) I've just replied to your other
e-mail suggesting this.
Another option would be to create the mappings dynamically, based on the
information reported by the camera at runtime.
> and clearly this
> one is not a 1:1 mapping to the libcamera controls. But at the same time, there
> is bunch of needed exception for semantic reasons. The yaml is not descriptive
> enough to configure fully the properties unfortunately.
We can extend the yaml file with additional information if needed,
and/or extend the information reported by the camera at runtime
(assuming GStreamer wouldn't have unreasonable requirements of course
:-)).
> As its easier to express what I'm hoping for with code, here's my own WIP, which
> is much more C++ like then this one. I'm missing the AutoFocusMode.
>
> https://gitlab.collabora.com/nicolas/libcamera/-/commit/83d67744ccbd60ad67d2afc70be7f2d95032e4d7
Nice ! Thank you for working on that. I'll try to give it a look ASAP.
> > >
> > > 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);
> >
> > Incorrect indentation (you should use tabs instead of spaces, and the
> > last line is indented by one space too much.
> >
> > We have a tool, in utils/checkstyle.py, that checks for common coding
> > style violations and can in many cases propose fixes (see
> > https://libcamera.org/coding-style.html#tools). I recommend installing
> > it as a git post-commit hook, how to do so is also documented in the
> > previous URL.
> >
> > Please take the output of the tool with a pinch of salt though. For code
> > formatting, the tool is based on clang-format, and while it does a
> > fairly good job, not all suggestions are perfect. You can ignore some of
> > the warnings if you think that's appropriate (mostly based on the coding
> > style of the existing code in the file).
> >
> > > + 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);
> > > + } 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'"));
> > > + }
> > > + }
> >
> > We should be able to enable and disable auto-focus at runtime as well. I
> > don't want to ask for too much yak-shaving, so I'm fine with this patch
> > as an initial solution. Please note, however, that once we enable
> > support for controls more generically in libcamerasrc, the name of the
> > property may change.
> >
> > > +
> > > 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