| Message ID | CAM6JzGDQMvPaOJfvUGy286PhM5oUvyxSkWWq8YFvmKoW1T0Rfw@mail.gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
Hi, 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. > > 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@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. regards, Nicolas > 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); > + > }
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 ? > > 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@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); > > + > > }
Hi all, On Wed, 31 May 2023 at 16:06, Laurent Pinchart <laurent.pinchart@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@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
Hi Naush, 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. 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@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); > > > > + > > > > }
Hi Laurent, On Wed, 31 May 2023 at 16:52, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Naush, > > 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? 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@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
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@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); > > > > > > + > > > > > > }
Hi Laurent, On Thu, 1 Jun 2023 at 08:29, Laurent Pinchart <laurent.pinchart@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@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
Le jeudi 01 juin 2023 à 08:39 +0100, Naushir Patuck a écrit : > > 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. To reassure you, it can't be worse then what most UVC camera do today. Having a default matching this would help consistency though (or being similarly bad from some point of view). Later, on top, there should be ways to move to a more manual control in focus aware Gst applications. Nicolas
On Thu, Jun 01, 2023 at 12:24:22PM -0400, Nicolas Dufresne wrote: > Le jeudi 01 juin 2023 à 08:39 +0100, Naushir Patuck a écrit : > > > 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. > > To reassure you, it can't be worse then what most UVC camera do today. Having a > default matching this would help consistency though (or being similarly bad from > some point of view). Later, on top, there should be ways to move to a more > manual control in focus aware Gst applications. Hmmmm... I'm really in two minds here. First of all, we can't expect the same default behaviour for all cameras when it comes to auto-focus, simply because many cameras don't have a controllable focus lens, and some (I'm thinking about UVC devices here) may not have the ability to turn auto-focus on. Still, implementing consistent defaults where possible is a goal I highly value, in order to maximize application portability. When it comes to the libcamera core, setting the focus lens to the hyperfocal distance and disabling auto-focus seems like a good default to me. Different use cases would favour different defaults, and I don't think there's one particular use case that could be considered so prevalent that it would call for enabling continuous auto-focus by default. Different defaults could however be implemented in different layers. For instance, I could imagine PipeWire deciding to enabling continuous auto-focus by default on desktop or mobile systems. For libcamerasrc, I don't know if the same reasoning should lead to the same result, or if we should consider that the vast majority of use cases for the GStreamer element call for enabling auto-focus by default. I currently side towards doing the same as in the libcamera core, as I believe it would be confusing for users to have different default behaviours when using libcamera through different adaptation layers. However, I could also imagine enabling auto-focus by default in the V4L2 adaptation layer for instance, as that layer is mostly meant to expose libcamera-based cameras as webcams.
I have fixed the review comments and have installed and configured git send-email. However, apparently it started a new thread, I don't think that is what it should have done: https://lists.libcamera.org/pipermail/libcamera-devel/2023-May/037925.html (I used "git send-email --in-reply-to=037947 --to= libcamera-devel@lists.libcamera.org patch_file.patch") Also, you mentioned that would like to see the original patch better formatted. Should I just create a whole new patch altogether? Finally, about the default/non-default discussion, I was trying to be conservative here, because when I opened the original bug (#188), the reply I got sounded like you thought it was a missing feature rather than a bug. For me this is a blocking bug: I can't use the PiCamV3 at all due to random focus behaviour. So therefore I opted for keeping the default behaviour as it was before to make it more likely that this patch will be accepted. On Fri, Jun 2, 2023 at 7:19 AM Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > On Thu, Jun 01, 2023 at 12:24:22PM -0400, Nicolas Dufresne wrote: > > Le jeudi 01 juin 2023 à 08:39 +0100, Naushir Patuck a écrit : > > > > 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. > > > > To reassure you, it can't be worse then what most UVC camera do today. > Having a > > default matching this would help consistency though (or being similarly > bad from > > some point of view). Later, on top, there should be ways to move to a > more > > manual control in focus aware Gst applications. > > Hmmmm... I'm really in two minds here. > > First of all, we can't expect the same default behaviour for all cameras > when it comes to auto-focus, simply because many cameras don't have a > controllable focus lens, and some (I'm thinking about UVC devices here) > may not have the ability to turn auto-focus on. Still, implementing > consistent defaults where possible is a goal I highly value, in order to > maximize application portability. > > When it comes to the libcamera core, setting the focus lens to the > hyperfocal distance and disabling auto-focus seems like a good default > to me. Different use cases would favour different defaults, and I don't > think there's one particular use case that could be considered so > prevalent that it would call for enabling continuous auto-focus by > default. Different defaults could however be implemented in different > layers. For instance, I could imagine PipeWire deciding to enabling > continuous auto-focus by default on desktop or mobile systems. > > For libcamerasrc, I don't know if the same reasoning should lead to the > same result, or if we should consider that the vast majority of use > cases for the GStreamer element call for enabling auto-focus by default. > I currently side towards doing the same as in the libcamera core, as I > believe it would be confusing for users to have different default > behaviours when using libcamera through different adaptation layers. > However, I could also imagine enabling auto-focus by default in the V4L2 > adaptation layer for instance, as that layer is mostly meant to expose > libcamera-based cameras as webcams. > > -- > Regards, > > Laurent Pinchart >
Hi Laurent, Le vendredi 02 juin 2023 à 08:19 +0300, Laurent Pinchart a écrit : > For libcamerasrc, I don't know if the same reasoning should lead to the > same result, or if we should consider that the vast majority of use > cases for the GStreamer element call for enabling auto-focus by default. > I currently side towards doing the same as in the libcamera core, as I > believe it would be confusing for users to have different default > behaviours when using libcamera through different adaptation layers. > However, I could also imagine enabling auto-focus by default in the V4L2 > adaptation layer for instance, as that layer is mostly meant to expose > libcamera-based cameras as webcams. > I like to decide based on what the users get by default. A mimic of what a webcam usually do seems logical. What do you get in the simplest form of usage of the software (in GStreamer this means if you just plug and start the element: libcamerasrc ! ...). Not enabling it (or not setting the lense to a decent default position) on HW that needs to be focused means most of the time you'll just get a bad output. This impose more complexity on the application. Being a low level library does not imply it have to be difficult across multiple type of cameras. Its no more effort to disable then to enable it either. Of course, this can either happen in libcamera or in libcamerasrc element, that I don't care so much. I don't have the full portrait on auto-focus quality atm to judge if now is the time either, and which type of auto focus we should enable vs which one we should definitely avoid as it would make things worst. I would certainly not rush into "getting something" for that reason. But for sure, the design principle for the gstreamer element (and same will apply for the pipewire one) is that it goes from really simple to full control, and nothing special should be done to get a good image. This is mostly the reason why the element assume you can always use the cameras with a single stream (hopefully this will stay true ;-P). So assuming auto-focus feature actually works (I have no mean to verify), enabling it would mean it behaves similar to what webcam do. Nicolas
Hi Cedric, On Fri, Jun 02, 2023 at 03:53:03PM +0200, Cedric Nugteren wrote: > I have fixed the review comments and have installed and configured git > send-email. Thank you. That appears to have gone well as your new version hasn't been mangled by mail clients and/or servers :-) > However, apparently it started a new thread, I don't think that > is what it should have done: > https://lists.libcamera.org/pipermail/libcamera-devel/2023-May/037925.html > (I used "git send-email --in-reply-to=037947 --to= > libcamera-devel@lists.libcamera.org patch_file.patch") The --in-reply-to argument takes a Message-ID. The 037947 number you've used is the index from the mailing list archives. The Message-ID of your original e-mail was CAM6JzGDQMvPaOJfvUGy286PhM5oUvyxSkWWq8YFvmKoW1T0Rfw@mail.gmail.com (you can find it by checking the mail headers in your sent folder, or in patchwork at https://patchwork.libcamera.org/patch/18651/). This being said, new versions of a patch or patch series are not supposed to be sent as a replies to the previous version, so all is fine :-) > Also, you mentioned that would like to see the original patch better > formatted. Should I just create a whole new patch altogether? When submitting a new version of a patch, you should, well, submit a new version of the patch :-) The patch you have sent, titled "[PATCH] Apply review suggestions" is not a new version of the original patch, but a set of changes that apply on top. As the original patch won't be merged, you should instead send a v2 stemming from v1 and incorporating the changes requested during review. In git's terms, that means that the changes you've made to v1 should be squashed into the original commit (`git commit --amend`), not committed as a separate commit. When generating the patch with git-format-patch, you can pass the `-v2` argument to record the version number in the subject line (you can also do so manually by editing the patch file, but that's more error-prone). A nice thing to do when sending new versions is to include a changelog. See https://patchwork.libcamera.org/patch/18699/ for instance, and note the 'Changes since v1:' section under the '---'. This is something you can include directly in the commit message in your git tree, by adding a '---' marker below the tags (Signed-off-by & co.), and recording the changelog there. This helps reviewers understanding what has changed between patch versions. I've marked "[PATCH] Apply review suggestions" as "changes requested" in patchwork. Could you submit a real v2 of this patch ? > Finally, about the default/non-default discussion, I was trying to be > conservative here, because when I opened the original bug (#188), the reply > I got sounded like you thought it was a missing feature rather than a bug. > For me this is a blocking bug: I can't use the PiCamV3 at all due to random > focus behaviour. So therefore I opted for keeping the default behaviour as > it was before to make it more likely that this patch will be accepted. I'm tempted to default to disabling auto-focus. The discussion is still ongoing though, but that shouldn't block you. You can send a v2 with auto-focus disabled by default, and we can easily switch the default on top if desired. > On Fri, Jun 2, 2023 at 7:19 AM Laurent Pinchart wrote: > > On Thu, Jun 01, 2023 at 12:24:22PM -0400, Nicolas Dufresne wrote: > > > Le jeudi 01 juin 2023 à 08:39 +0100, Naushir Patuck a écrit : > > > > > 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. > > > > > > To reassure you, it can't be worse then what most UVC camera do today. Having a > > > default matching this would help consistency though (or being similarly bad from > > > some point of view). Later, on top, there should be ways to move to a more > > > manual control in focus aware Gst applications. > > > > Hmmmm... I'm really in two minds here. > > > > First of all, we can't expect the same default behaviour for all cameras > > when it comes to auto-focus, simply because many cameras don't have a > > controllable focus lens, and some (I'm thinking about UVC devices here) > > may not have the ability to turn auto-focus on. Still, implementing > > consistent defaults where possible is a goal I highly value, in order to > > maximize application portability. > > > > When it comes to the libcamera core, setting the focus lens to the > > hyperfocal distance and disabling auto-focus seems like a good default > > to me. Different use cases would favour different defaults, and I don't > > think there's one particular use case that could be considered so > > prevalent that it would call for enabling continuous auto-focus by > > default. Different defaults could however be implemented in different > > layers. For instance, I could imagine PipeWire deciding to enabling > > continuous auto-focus by default on desktop or mobile systems. > > > > For libcamerasrc, I don't know if the same reasoning should lead to the > > same result, or if we should consider that the vast majority of use > > cases for the GStreamer element call for enabling auto-focus by default. > > I currently side towards doing the same as in the libcamera core, as I > > believe it would be confusing for users to have different default > > behaviours when using libcamera through different adaptation layers. > > However, I could also imagine enabling auto-focus by default in the V4L2 > > adaptation layer for instance, as that layer is mostly meant to expose > > libcamera-based cameras as webcams.
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); 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", + "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); } 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; 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,
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. Signed-off-by: Cedric Nugteren <web@cedricnugteren.nl> --- src/gstreamer/gstlibcameraprovider.cpp | 13 ++++++++++++ src/gstreamer/gstlibcamerasrc.cpp | 29 +++++++++++++++++++++++++- 2 files changed, 41 insertions(+), 1 deletion(-) 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'")); + } + } + 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); + }