[libcamera-devel] [PATCH] Add enable_auto_focus option to the GStreamer plugin
Naushir Patuck
naush at raspberrypi.com
Thu Jun 1 09:39:07 CEST 2023
Hi Laurent,
On Thu, 1 Jun 2023 at 08:29, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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 ?
Yes I can do that. We should also possibly document this as the expected
default somewhere, maybe in the LensPosition control description?
>
> If you have any idea what the default should be in the GStreamer
> element, please feel free to chime in :-)
Perhaps GStreamer should do nothing with focus by default and rely on the IPA
default behaviour (i.e. go to hyperfocal)? This then relies on the application
to choose what it wants to do on startup, just like any other application that
directly uses libcamera.
Regards,
Naush
>
> > > 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