[libcamera-devel] [PATCH] Add enable_auto_focus option to the GStreamer plugin
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Jun 1 09:28:57 CEST 2023
Hi Naush,
On Thu, Jun 01, 2023 at 08:21:22AM +0100, Naushir Patuck wrote:
> On Wed, 31 May 2023 at 16:52, Laurent Pinchart wrote:
> > On Wed, May 31, 2023 at 04:16:28PM +0100, Naushir Patuck wrote:
> > > On Wed, 31 May 2023 at 16:06, Laurent Pinchart wrote:
> > > > 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.
> >
> > Thank you for the explanation.
> >
> > I'm not totally sure I would have reached the same conclusion when it
> > comes to the default behaviour, but that's fine, your rationale
> > regarding AF mode selection makes sense. What I dislike, however, is
> > leaving the lens where it last was. Generally speaking, libcamera should
> > ensure a consistent and fully-defined camera state when starting, and
> > shouldn't depend on previous capture sessions.
>
> Yes, I do agree with this.
>
> The problem is choosing a default behaviour - CAF would not really be a nice
> default experience on a non-PDAF sensor. Perhaps doing a move to hyperfocal is
> a good compromise and will provide a consistent experience across platforms and
> sensor modules?
This works for me. Would you send a patch to do so in the Raspberry Pi
IPA module ?
If you have any idea what the default should be in the GStreamer
element, please feel free to chime in :-)
> > I would also favour consistent behaviour across different cameras
> > regarding the default value of controls, and the AF mode in this case.
> > This means I would be fine disabling AF by default for IPU3 (and rkisp1,
> > one we merge AF support for that platform).
> >
> > This leaves open the question of what to do in the GStreamer element by
> > default. Does it make sense for libcamera to default to disabling
> > autofocus, and enabling it by default in libcamerasrc ?
> >
> > > > > > 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