[libcamera-devel] [PATCH] Add enable_auto_focus option to the GStreamer plugin
Naushir Patuck
naush at raspberrypi.com
Wed May 31 17:16:28 CEST 2023
Hi all,
On Wed, 31 May 2023 at 16:06, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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 ?
Believe it or not, this has been asked a few times now :-)
The RPi IPA does not want to assume or trigger any particular behaviour based on
the specific sensor used. We feel it's up to applications to decide this
unconditionally. In the case of a focus lens available, the application ought to
choose what it wants to do, e.g. hyperfocal fixed position, AF cycle then fixed
position, CAF, etc. We don't really want to choose a default that may not be
what is needed as this is highly use case dependent.
So we essentially do nothing and leave the lens where it last was in the absence
of any AF control.
Regards,
Naush
>
> > > 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
More information about the libcamera-devel
mailing list