[{"id":27261,"web_url":"https://patchwork.libcamera.org/comment/27261/","msgid":"<20230605153331.GA7234@pendragon.ideasonboard.com>","date":"2023-06-05T15:33:31","subject":"Re: [libcamera-devel] [PATCH v2] Add enable_auto_focus option to\n\tthe GStreamer plugin","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Cedric,\n\n(CC'ing Nicolas)\n\nThank you for the patch.\n\nOn Mon, Jun 05, 2023 at 04:44:05PM +0200, Cedric Nugteren via libcamera-devel wrote:\n> Cameras such as the PiCam 3 support auto-focus, but the GStreamer plugin\n> for libcamera does not enable auto-focus. With this patch auto-focus can\n> be enabled for cameras that support it. By default it is disabled, which\n> means default behaviour remains unchanged. For cameras that do not\n> support auto-focus, an error message shows up if it auto-focus is\n\nI think you meant \"its auto-focus\" instead of \"it auto-focus\" ?\n\n> enabled.\n> \n> This was tested on cameras that do not support auto-focus (e.g. PiCam2)\n> and was tested on a camera that does support auto-focus (PiCam3) by\n> enabling it, observing auto-focus, and by disabling it or by not setting\n> the option, both resulting in auto-focus being disabled.\n> \n> This should close https://bugs.libcamera.org/show_bug.cgi?id=188.\n\nThis should be written\n\nBug: https://bugs.libcamera.org/show_bug.cgi?id=188.\n\nwhich I realize isn't documented anywhere :-S I wonder where to best\nmention this. Is there any particular place in the documentation that\nwould have caught your eyes ?\n\n> Signed-off-by: Cedric Nugteren \n> ---\n> Changes since v1:\n> - Now re-using the GParamSpec variables instead of re-creating.\n> - Use bool instead of gboolean for enable_auto_focus.\n> - Remove changes from the reconfigure_element functions.\n\nPerfect :-)\n\n> ---\n> src/gstreamer/gstlibcameraprovider.cpp | 12 +++++++++++\n> src/gstreamer/gstlibcamerasrc.cpp | 29 +++++++++++++++++++++++++-\n> 2 files changed, 40 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/gstreamer/gstlibcameraprovider.cpp b/src/gstreamer/gstlibcameraprovider.cpp\n> index 6eb0a0eb..579cb8c0 100644\n> --- a/src/gstreamer/gstlibcameraprovider.cpp\n> +++ b/src/gstreamer/gstlibcameraprovider.cpp\n> @@ -31,6 +31,7 @@ GST_DEBUG_CATEGORY_STATIC(provider_debug);\n> \n> enum {\n> \tPROP_DEVICE_NAME = 1,\n> +\tPROP_ENABLE_AUTO_FOCUS = 2,\n> };\n> \n> #define GST_TYPE_LIBCAMERA_DEVICE gst_libcamera_device_get_type()\n> @@ -40,6 +41,7 @@ G_DECLARE_FINAL_TYPE(GstLibcameraDevice, gst_libcamera_device,\n> struct _GstLibcameraDevice {\n> \tGstDevice parent;\n> \tgchar *name;\n> +\tbool enable_auto_focus = false;\n> };\n> \n> G_DEFINE_TYPE(GstLibcameraDevice, gst_libcamera_device, GST_TYPE_DEVICE)\n> @@ -56,6 +58,7 @@ gst_libcamera_device_create_element(GstDevice *device, const gchar *name)\n> \tg_assert(source);\n> \n> \tg_object_set(source, \"camera-name\", GST_LIBCAMERA_DEVICE(device)->name, nullptr);\n> +\tg_object_set(source, \"enable-auto-focus\", GST_LIBCAMERA_DEVICE(device)->enable_auto_focus, nullptr);\n\nNicolas, is there any particular rule when it comes to naming properties\nfor GStreamer elements ?\n\n> \n> \treturn source;\n> }\n> @@ -82,6 +85,9 @@ gst_libcamera_device_set_property(GObject *object, guint prop_id,\n> \tcase PROP_DEVICE_NAME:\n> \t\tdevice->name = g_value_dup_string(value);\n> \t\tbreak;\n> +\tcase PROP_ENABLE_AUTO_FOCUS:\n> +\t\tdevice->enable_auto_focus = g_value_get_boolean(value);\n> +\t\tbreak;\n> \tdefault:\n> \t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> \t\tbreak;\n> @@ -121,6 +127,12 @@ gst_libcamera_device_class_init(GstLibcameraDeviceClass *klass)\n> \t\t\t\t\t\t(GParamFlags)(G_PARAM_STATIC_STRINGS | G_PARAM_WRITABLE |\n> \t\t\t\t\t\t\t G_PARAM_CONSTRUCT_ONLY));\n> \tg_object_class_install_property(object_class, PROP_DEVICE_NAME, pspec);\n> +\tpspec = g_param_spec_boolean(\"enable-auto-focus\",\n> +\t \"Enable auto-focus\",\n> +\t \"Enable auto-focus if set to true, \"\n> +\t \"disable it if set to false\",\n> +\t FALSE, G_PARAM_WRITABLE);\n\nIncorrect indentation (you should use tabs instead of spaces, and the\nlast line is indented by one space too much.\n\nWe have a tool, in utils/checkstyle.py, that checks for common coding\nstyle violations and can in many cases propose fixes (see\nhttps://libcamera.org/coding-style.html#tools). I recommend installing\nit as a git post-commit hook, how to do so is also documented in the\nprevious URL.\n\nPlease take the output of the tool with a pinch of salt though. For code\nformatting, the tool is based on clang-format, and while it does a\nfairly good job, not all suggestions are perfect. You can ignore some of\nthe warnings if you think that's appropriate (mostly based on the coding\nstyle of the existing code in the file).\n\n> +\tg_object_class_install_property(object_class, PROP_ENABLE_AUTO_FOCUS, pspec);\n> }\n> \n> static GstDevice *\n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index a10cbd4f..b2d8456b 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -146,6 +146,7 @@ struct _GstLibcameraSrc {\n> \tGstTask *task;\n> \n> \tgchar *camera_name;\n> +\tbool enable_auto_focus = false;\n> \n> \tGstLibcameraSrcState *state;\n> \tGstLibcameraAllocator *allocator;\n> @@ -154,7 +155,8 @@ struct _GstLibcameraSrc {\n> \n> enum {\n> \tPROP_0,\n> -\tPROP_CAMERA_NAME\n> +\tPROP_CAMERA_NAME,\n> +\tPROP_ENABLE_AUTO_FOCUS,\n> };\n> \n> G_DEFINE_TYPE_WITH_CODE(GstLibcameraSrc, gst_libcamera_src, GST_TYPE_ELEMENT,\n> @@ -577,6 +579,18 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> \t\tgst_flow_combiner_add_pad(self->flow_combiner, srcpad);\n> \t}\n> \n> +\tif (self->enable_auto_focus) {\n> +\t\tconst ControlInfoMap &infoMap = state->cam_->controls();\n> +\t\tif (infoMap.find(&controls::AfMode) != infoMap.end()) {\n> +\t\t\tstate->initControls_.set(controls::AfMode, controls::AfModeContinuous);\n> +\t\t} else {\n> +\t\t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> +\t\t\t\t(\"Failed to enable auto focus\"),\n> +\t\t\t\t(\"AfMode not found in camera controls, \"\n> +\t\t\t\t\"please retry with 'enable-auto-focus=false'\"));\n> +\t\t}\n> +\t}\n\nWe should be able to enable and disable auto-focus at runtime as well. I\ndon't want to ask for too much yak-shaving, so I'm fine with this patch\nas an initial solution. Please note, however, that once we enable\nsupport for controls more generically in libcamerasrc, the name of the\nproperty may change. \n\n> +\n> \tret = state->cam_->start(&state->initControls_);\n> \tif (ret) {\n> \t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> @@ -659,6 +673,9 @@ gst_libcamera_src_set_property(GObject *object, guint prop_id,\n> \t\tg_free(self->camera_name);\n> \t\tself->camera_name = g_value_dup_string(value);\n> \t\tbreak;\n> +\tcase PROP_ENABLE_AUTO_FOCUS:\n> +\t\tself->enable_auto_focus = g_value_get_boolean(value);\n> +\t\tbreak;\n> \tdefault:\n> \t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> \t\tbreak;\n> @@ -676,6 +693,9 @@ gst_libcamera_src_get_property(GObject *object, guint prop_id, GValue *value,\n> \tcase PROP_CAMERA_NAME:\n> \t\tg_value_set_string(value, self->camera_name);\n> \t\tbreak;\n> +\tcase PROP_ENABLE_AUTO_FOCUS:\n> +\t\tg_value_set_boolean(value, self->enable_auto_focus);\n> +\t\tbreak;\n> \tdefault:\n> \t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> \t\tbreak;\n> @@ -844,4 +864,11 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n> \t\t\t\t\t\t\t | G_PARAM_READWRITE\n> \t\t\t\t\t\t\t | G_PARAM_STATIC_STRINGS));\n> \tg_object_class_install_property(object_class, PROP_CAMERA_NAME, spec);\n> +\tspec = g_param_spec_boolean(\"enable-auto-focus\",\n> +\t \"Enable auto-focus\",\n> +\t \"Enable auto-focus if set to true, \"\n> +\t \"disable it if set to false\",\n> +\t FALSE, G_PARAM_WRITABLE);\n> +\tg_object_class_install_property(object_class, PROP_ENABLE_AUTO_FOCUS, spec);\n> +\n> }","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CFDECC3200\n\tfor ;\n\tMon, 5 Jun 2023 15:33:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 407526287D;\n\tMon, 5 Jun 2023 17:33:34 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5E92162709\n\tfor ;\n\tMon, 5 Jun 2023 17:33:33 +0200 (CEST)","from pendragon.ideasonboard.com (om126156242094.26.openmobile.ne.jp\n\t[126.156.242.94])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 912857EC;\n\tMon, 5 Jun 2023 17:33:07 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685979214;\n\tbh=j4+E+7ElwUymXnKf20y+bOEu9zGfIDmxkHR+vAbaZSE=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=QeQvIoDXknuhp64QNggOy53wEn4X72zfUrHvXAw6TfY636INsjpw1o45ChtXlC+A3\n\twsnNGWsSBdBcF7JQeP7IZmz4neanfQhP2nP14NK4Gm+6NLFFVufBnosJtJI6yInZuC\n\tgL+rqsqqo0NkdSBOUmYdkYVQKe/kzyXmARJFrunbGnyMOAbU/StO42e/hq3HTynPcp\n\ts4nVTfo0C/c0N/JK5tGxvMai8BuvYYJHYPiWsXlFI6GKH+koBQWC8pSH01Mk66ZBXK\n\t+SIzYtNtD1aqIxuDxQ1CxHJgRxnPpy3IovhisR3k9kS+DDu1u0WURVNO4C5k1xBbmM\n\tXFUMCypfSF21Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685979188;\n\tbh=j4+E+7ElwUymXnKf20y+bOEu9zGfIDmxkHR+vAbaZSE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ax33QjfpuV9ueSqOaXCA6GMpLtVz1P0kwk17tgzm3BkX/qwLatc4xOfzYXIXS8/T1\n\tWYhcpteo/dnN6SEgVTt6lM1YdylpwMhZec8uJ/2D9oc9015Td2GhWUHsy1V8tl4ge1\n\tTpXCfwHV/U79/T+PSYSeptS1K9QO1DsfO+L7ABiI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ax33Qjfp\"; dkim-atps=neutral","Date":"Mon, 5 Jun 2023 18:33:31 +0300","To":"Cedric Nugteren ","Message-ID":"<20230605153331.GA7234@pendragon.ideasonboard.com>","References":"<20230605144405.118153-1-web@cedricnugteren.nl>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230605144405.118153-1-web@cedricnugteren.nl>","Subject":"Re: [libcamera-devel] [PATCH v2] Add enable_auto_focus option to\n\tthe GStreamer plugin","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org,\n\tCedric Nugteren ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27262,"web_url":"https://patchwork.libcamera.org/comment/27262/","msgid":"<236fda3525cfc091bd6874fc2cfabc1d888afc7b.camel@ndufresne.ca>","date":"2023-06-05T15:47:51","subject":"Re: [libcamera-devel] [PATCH v2] Add enable_auto_focus option to\n\tthe GStreamer plugin","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Hi,\n\nLe lundi 05 juin 2023 à 16:44 +0200, Cedric Nugteren via libcamera-devel a\nécrit :\n> Cameras such as the PiCam 3 support auto-focus, but the GStreamer plugin\n> for libcamera does not enable auto-focus. With this patch auto-focus can\n> be enabled for cameras that support it. By default it is disabled, which\n> means default behaviour remains unchanged. For cameras that do not\n> support auto-focus, an error message shows up if it auto-focus is\n> enabled.\n> \n> This was tested on cameras that do not support auto-focus (e.g. PiCam2)\n> and was tested on a camera that does support auto-focus (PiCam3) by\n> enabling it, observing auto-focus, and by disabling it or by not setting\n> the option, both resulting in auto-focus being disabled.\n> \n> This should close https://bugs.libcamera.org/show_bug.cgi?id=188.\n> \n> Signed-off-by: Cedric Nugteren \n> ---\n> Changes since v1:\n> - Now re-using the GParamSpec variables instead of re-creating.\n> - Use bool instead of gboolean for enable_auto_focus.\n> - Remove changes from the reconfigure_element functions.\n> ---\n> src/gstreamer/gstlibcameraprovider.cpp | 12 +++++++++++\n> src/gstreamer/gstlibcamerasrc.cpp | 29 +++++++++++++++++++++++++-\n> 2 files changed, 40 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/gstreamer/gstlibcameraprovider.cpp b/src/gstreamer/gstlibcameraprovider.cpp\n> index 6eb0a0eb..579cb8c0 100644\n> --- a/src/gstreamer/gstlibcameraprovider.cpp\n> +++ b/src/gstreamer/gstlibcameraprovider.cpp\n> @@ -31,6 +31,7 @@ GST_DEBUG_CATEGORY_STATIC(provider_debug);\n> \n> enum {\n> \tPROP_DEVICE_NAME = 1,\n> +\tPROP_ENABLE_AUTO_FOCUS = 2,\n> };\n> \n> #define GST_TYPE_LIBCAMERA_DEVICE gst_libcamera_device_get_type()\n> @@ -40,6 +41,7 @@ G_DECLARE_FINAL_TYPE(GstLibcameraDevice, gst_libcamera_device,\n> struct _GstLibcameraDevice {\n> \tGstDevice parent;\n> \tgchar *name;\n> +\tbool enable_auto_focus = false;\n> };\n> \n> G_DEFINE_TYPE(GstLibcameraDevice, gst_libcamera_device, GST_TYPE_DEVICE)\n> @@ -56,6 +58,7 @@ gst_libcamera_device_create_element(GstDevice *device, const gchar *name)\n> \tg_assert(source);\n> \n> \tg_object_set(source, \"camera-name\", GST_LIBCAMERA_DEVICE(device)->name, nullptr);\n> +\tg_object_set(source, \"enable-auto-focus\", GST_LIBCAMERA_DEVICE(device)->enable_auto_focus, nullptr);\n> \n> \treturn source;\n> }\n> @@ -82,6 +85,9 @@ gst_libcamera_device_set_property(GObject *object, guint prop_id,\n> \tcase PROP_DEVICE_NAME:\n> \t\tdevice->name = g_value_dup_string(value);\n> \t\tbreak;\n> +\tcase PROP_ENABLE_AUTO_FOCUS:\n> +\t\tdevice->enable_auto_focus = g_value_get_boolean(value);\n> +\t\tbreak;\n> \tdefault:\n> \t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> \t\tbreak;\n> @@ -121,6 +127,12 @@ gst_libcamera_device_class_init(GstLibcameraDeviceClass *klass)\n> \t\t\t\t\t\t(GParamFlags)(G_PARAM_STATIC_STRINGS | G_PARAM_WRITABLE |\n> \t\t\t\t\t\t\t G_PARAM_CONSTRUCT_ONLY));\n> \tg_object_class_install_property(object_class, PROP_DEVICE_NAME, pspec);\n> +\tpspec = g_param_spec_boolean(\"enable-auto-focus\",\n> +\t \"Enable auto-focus\",\n> +\t \"Enable auto-focus if set to true, \"\n> +\t \"disable it if set to false\",\n> +\t FALSE, G_PARAM_WRITABLE);\n> +\tg_object_class_install_property(object_class, PROP_ENABLE_AUTO_FOCUS, pspec);\n> }\n> \n> static GstDevice *\n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index a10cbd4f..b2d8456b 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -146,6 +146,7 @@ struct _GstLibcameraSrc {\n> \tGstTask *task;\n> \n> \tgchar *camera_name;\n> +\tbool enable_auto_focus = false;\n> \n> \tGstLibcameraSrcState *state;\n> \tGstLibcameraAllocator *allocator;\n> @@ -154,7 +155,8 @@ struct _GstLibcameraSrc {\n> \n> enum {\n> \tPROP_0,\n> -\tPROP_CAMERA_NAME\n> +\tPROP_CAMERA_NAME,\n> +\tPROP_ENABLE_AUTO_FOCUS,\n> };\n> \n> G_DEFINE_TYPE_WITH_CODE(GstLibcameraSrc, gst_libcamera_src, GST_TYPE_ELEMENT,\n> @@ -577,6 +579,18 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> \t\tgst_flow_combiner_add_pad(self->flow_combiner, srcpad);\n> \t}\n> \n> +\tif (self->enable_auto_focus) {\n> +\t\tconst ControlInfoMap &infoMap = state->cam_->controls();\n> +\t\tif (infoMap.find(&controls::AfMode) != infoMap.end()) {\n> +\t\t\tstate->initControls_.set(controls::AfMode, controls::AfModeContinuous);\n\nI don't really like the idea of mapping modes to a boolean here. I think this\nshould be an GEnum type of property. Some mode requires some actions to actually\nwork, best to just skip these for now.\n\n> +\t\t} else {\n> +\t\t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> +\t\t\t\t(\"Failed to enable auto focus\"),\n> +\t\t\t\t(\"AfMode not found in camera controls, \"\n> +\t\t\t\t\"please retry with 'enable-auto-focus=false'\"));\n> +\t\t}\n> +\t}\n> +\n> \tret = state->cam_->start(&state->initControls_);\n> \tif (ret) {\n> \t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> @@ -659,6 +673,9 @@ gst_libcamera_src_set_property(GObject *object, guint prop_id,\n> \t\tg_free(self->camera_name);\n> \t\tself->camera_name = g_value_dup_string(value);\n> \t\tbreak;\n> +\tcase PROP_ENABLE_AUTO_FOCUS:\n> +\t\tself->enable_auto_focus = g_value_get_boolean(value);\n> +\t\tbreak;\n> \tdefault:\n> \t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> \t\tbreak;\n> @@ -676,6 +693,9 @@ gst_libcamera_src_get_property(GObject *object, guint prop_id, GValue *value,\n> \tcase PROP_CAMERA_NAME:\n> \t\tg_value_set_string(value, self->camera_name);\n> \t\tbreak;\n> +\tcase PROP_ENABLE_AUTO_FOCUS:\n> +\t\tg_value_set_boolean(value, self->enable_auto_focus);\n> +\t\tbreak;\n> \tdefault:\n> \t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> \t\tbreak;\n> @@ -844,4 +864,11 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n> \t\t\t\t\t\t\t | G_PARAM_READWRITE\n> \t\t\t\t\t\t\t | G_PARAM_STATIC_STRINGS));\n> \tg_object_class_install_property(object_class, PROP_CAMERA_NAME, spec);\n> +\tspec = g_param_spec_boolean(\"enable-auto-focus\",\n> +\t \"Enable auto-focus\",\n> +\t \"Enable auto-focus if set to true, \"\n> +\t \"disable it if set to false\",\n> +\t FALSE, G_PARAM_WRITABLE);\n> +\tg_object_class_install_property(object_class, PROP_ENABLE_AUTO_FOCUS, spec);\n> +\n> }","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 684C9C31E9\n\tfor ;\n\tMon, 5 Jun 2023 15:47:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E3A7E62886;\n\tMon, 5 Jun 2023 17:47:55 +0200 (CEST)","from mail-qk1-x72d.google.com (mail-qk1-x72d.google.com\n\t[IPv6:2607:f8b0:4864:20::72d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 12E9C62880\n\tfor ;\n\tMon, 5 Jun 2023 17:47:54 +0200 (CEST)","by mail-qk1-x72d.google.com with SMTP id\n\taf79cd13be357-75ea05150b3so97488585a.0\n\tfor ;\n\tMon, 05 Jun 2023 08:47:53 -0700 (PDT)","from nicolas-tpx395.localdomain ([2606:6d00:11:5f2f::7a9])\n\tby smtp.gmail.com with ESMTPSA id\n\tbz5-20020ad44c05000000b005f227de6b1bsm4650381qvb.116.2023.06.05.08.47.52\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 05 Jun 2023 08:47:52 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685980075;\n\tbh=SMiLCd4C2OJZvxq6rDIo28k15ckiS6MnhZIgcKDfqeo=;\n\th=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Hs2oB1a1Cqe8cDPOkUJsDnRuIOHJQ1/rRZXDl8jEG62roll4gHC+v6gEEfn+Zl/mO\n\tDjrFc4ZgdXRzIw1HR0IKTS+8zTUXISK5nrd2RVupW+nGt55QxhHAfdcFv++rF5npN6\n\tdioJjj152Suegg9cK4yywPkdN+7Jtnun3In329TbMuD4DuS5ro/avzbDYatRCSdBkj\n\tyK0h1gKNZRFyVoNMwsscBbxWqeTKrsPXGrjOMidaMPziqzmtZsg88ma1IObWLxROrC\n\tZPnY5XEYNmlENoa8S5Hfyn+GF71+iNbLL2NkyFzUHHfcWFHPmGdNUZ0flz3/1CdMlQ\n\tkJSnGarFY/T7A==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20221208.gappssmtp.com; s=20221208; t=1685980073;\n\tx=1688572073; \n\th=mime-version:user-agent:content-transfer-encoding:references\n\t:in-reply-to:date:cc:to:from:subject:message-id:from:to:cc:subject\n\t:date:message-id:reply-to;\n\tbh=tjPgv6ybGQ5CW/4ojmfaVWBS8wYBRS5CTXVzHW1mNTM=;\n\tb=ZBqXuMxzH2erpEkjruQpqFb0Lo+CFQoxUKcph6BBhie3YbI+fHvxiR3gBGKUzKupro\n\tkjLz50hfk1YzmJyzMi9FdKZywTV03AJOCvmpTOcV+A1VFGiHa9umrO3qQ5K0hkgo+/Ro\n\tfv/P6odBe1V0SCqFhaGjuTQeywCjOSsuiYWsuHiIEPfBt/29WBFrI90+e0LMMCacMNB7\n\tqw8yAy3VVwL3xBWMNPgt2bFvaP+F1MLCzzc3fPBNyM3s/Hr+Aicqih9Kd9kBYr5U3AIZ\n\t7vd7Rpn98/K6u/hr6GDzFrF4Lsx0nYMm1okawlLgnRcEAbjrOGHoN2aP0E/NmdxKC7kb\n\tG7nA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=ndufresne-ca.20221208.gappssmtp.com\n\theader.i=@ndufresne-ca.20221208.gappssmtp.com header.b=\"ZBqXuMxz\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1685980073; x=1688572073;\n\th=mime-version:user-agent:content-transfer-encoding:references\n\t:in-reply-to:date:cc:to:from:subject:message-id:x-gm-message-state\n\t:from:to:cc:subject:date:message-id:reply-to;\n\tbh=tjPgv6ybGQ5CW/4ojmfaVWBS8wYBRS5CTXVzHW1mNTM=;\n\tb=KBHWDNbZIyodyIT2jDPlfEBc0IJ1at+mYT8c34f0QgsbwkK1aoVcoYpYIXR5ZV17Hh\n\tV5/ZamNFNtQlH56IIXy1Sy5mj/R6DsHCNsvNieMgowg/yRO62RluJElcvNxCJquD71Mk\n\tBLa9uU7AeN3HT2hJrfWxZJMUcS57a2wvz2NaQD3FNCaRybjbOsIM1NRK9FT6PoVNkXTT\n\t4dHmAW6wjtGZ8lZ/WjtxtCcK6ABO4o6nE1yXFs4nGnxYbDCddNFVMGtiEcq3vJDVDbgU\n\tV1O4FrrR6eoDwJa7vGneEQPZNJqQuYT7epXDKD8xgaMMIgTaGCOzr4ATs40veQF+I9Fu\n\tNmFg==","X-Gm-Message-State":"AC+VfDwNczFPZKcO8vHGssX6z/7+9kVNdy3Z7kU8m4WyVsE6K2EXXf6l\n\ti8WwcXz81cjEL0rz0YwudSJjF6Gs2Lbx12vMClg=","X-Google-Smtp-Source":"ACHHUZ6KO4Xwr7D6bOlLono/V2INrS16B7uiy1bTRraNh7ooYh5NbLp48Xpm2MDrJHXSlBchhB9VrQ==","X-Received":"by 2002:a05:6214:238f:b0:628:731d:7abf with SMTP id\n\tfw15-20020a056214238f00b00628731d7abfmr6860018qvb.42.1685980072807; \n\tMon, 05 Jun 2023 08:47:52 -0700 (PDT)","Message-ID":"<236fda3525cfc091bd6874fc2cfabc1d888afc7b.camel@ndufresne.ca>","To":"Cedric Nugteren ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 05 Jun 2023 11:47:51 -0400","In-Reply-To":"<20230605144405.118153-1-web@cedricnugteren.nl>","References":"<20230605144405.118153-1-web@cedricnugteren.nl>","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","User-Agent":"Evolution 3.48.1 (3.48.1-1.fc38) ","MIME-Version":"1.0","Subject":"Re: [libcamera-devel] [PATCH v2] Add enable_auto_focus option to\n\tthe GStreamer plugin","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Nicolas Dufresne via libcamera-devel\n\t","Reply-To":"Nicolas Dufresne ","Cc":"Cedric Nugteren ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27263,"web_url":"https://patchwork.libcamera.org/comment/27263/","msgid":"","date":"2023-06-05T15:56:56","subject":"Re: [libcamera-devel] [PATCH v2] Add enable_auto_focus option to\n\tthe GStreamer plugin","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Le lundi 05 juin 2023 à 18:33 +0300, Laurent Pinchart a écrit :\n> Hi Cedric,\n> \n> (CC'ing Nicolas)\n> \n> Thank you for the patch.\n> \n> On Mon, Jun 05, 2023 at 04:44:05PM +0200, Cedric Nugteren via libcamera-devel wrote:\n> > Cameras such as the PiCam 3 support auto-focus, but the GStreamer plugin\n> > for libcamera does not enable auto-focus. With this patch auto-focus can\n> > be enabled for cameras that support it. By default it is disabled, which\n> > means default behaviour remains unchanged. For cameras that do not\n> > support auto-focus, an error message shows up if it auto-focus is\n> \n> I think you meant \"its auto-focus\" instead of \"it auto-focus\" ?\n> \n> > enabled.\n> > \n> > This was tested on cameras that do not support auto-focus (e.g. PiCam2)\n> > and was tested on a camera that does support auto-focus (PiCam3) by\n> > enabling it, observing auto-focus, and by disabling it or by not setting\n> > the option, both resulting in auto-focus being disabled.\n> > \n> > This should close https://bugs.libcamera.org/show_bug.cgi?id=188.\n> \n> This should be written\n> \n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=188.\n> \n> which I realize isn't documented anywhere :-S I wonder where to best\n> mention this. Is there any particular place in the documentation that\n> would have caught your eyes ?\n> \n> > Signed-off-by: Cedric Nugteren \n> > ---\n> > Changes since v1:\n> > - Now re-using the GParamSpec variables instead of re-creating.\n> > - Use bool instead of gboolean for enable_auto_focus.\n> > - Remove changes from the reconfigure_element functions.\n> \n> Perfect :-)\n> \n> > ---\n> > src/gstreamer/gstlibcameraprovider.cpp | 12 +++++++++++\n> > src/gstreamer/gstlibcamerasrc.cpp | 29 +++++++++++++++++++++++++-\n> > 2 files changed, 40 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/src/gstreamer/gstlibcameraprovider.cpp b/src/gstreamer/gstlibcameraprovider.cpp\n> > index 6eb0a0eb..579cb8c0 100644\n> > --- a/src/gstreamer/gstlibcameraprovider.cpp\n> > +++ b/src/gstreamer/gstlibcameraprovider.cpp\n> > @@ -31,6 +31,7 @@ GST_DEBUG_CATEGORY_STATIC(provider_debug);\n> > \n> > enum {\n> > \tPROP_DEVICE_NAME = 1,\n> > +\tPROP_ENABLE_AUTO_FOCUS = 2,\n> > };\n> > \n> > #define GST_TYPE_LIBCAMERA_DEVICE gst_libcamera_device_get_type()\n> > @@ -40,6 +41,7 @@ G_DECLARE_FINAL_TYPE(GstLibcameraDevice, gst_libcamera_device,\n> > struct _GstLibcameraDevice {\n> > \tGstDevice parent;\n> > \tgchar *name;\n> > +\tbool enable_auto_focus = false;\n> > };\n> > \n> > G_DEFINE_TYPE(GstLibcameraDevice, gst_libcamera_device, GST_TYPE_DEVICE)\n> > @@ -56,6 +58,7 @@ gst_libcamera_device_create_element(GstDevice *device, const gchar *name)\n> > \tg_assert(source);\n> > \n> > \tg_object_set(source, \"camera-name\", GST_LIBCAMERA_DEVICE(device)->name, nullptr);\n> > +\tg_object_set(source, \"enable-auto-focus\", GST_LIBCAMERA_DEVICE(device)->enable_auto_focus, nullptr);\n> \n> Nicolas, is there any particular rule when it comes to naming properties\n> for GStreamer elements ?\n\nThe GLib part of it is:\n\n A property name consists of one or more segments consisting of ASCII letters and\n digits, separated by either the - or _ character. The first character of a\n property name must be a letter. These are the same rules as for signal naming\n (see g_signal_new()).\n\nI strongly prefer - instead of _. Generally speaking, I would like this\ndiscussion to happen before we start adding controls. One of the thing I was\nconsidering was to auto-generate the propertly from the yaml, and clearly this\none is not a 1:1 mapping to the libcamera controls. But at the same time, there\nis bunch of needed exception for semantic reasons. The yaml is not descriptive\nenough to configure fully the properties unfortunately.\n\nAs its easier to express what I'm hoping for with code, here's my own WIP, which\nis much more C++ like then this one. I'm missing the AutoFocusMode.\n\nhttps://gitlab.collabora.com/nicolas/libcamera/-/commit/83d67744ccbd60ad67d2afc70be7f2d95032e4d7\n\nNicolas\n\n> \n> > \n> > \treturn source;\n> > }\n> > @@ -82,6 +85,9 @@ gst_libcamera_device_set_property(GObject *object, guint prop_id,\n> > \tcase PROP_DEVICE_NAME:\n> > \t\tdevice->name = g_value_dup_string(value);\n> > \t\tbreak;\n> > +\tcase PROP_ENABLE_AUTO_FOCUS:\n> > +\t\tdevice->enable_auto_focus = g_value_get_boolean(value);\n> > +\t\tbreak;\n> > \tdefault:\n> > \t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > \t\tbreak;\n> > @@ -121,6 +127,12 @@ gst_libcamera_device_class_init(GstLibcameraDeviceClass *klass)\n> > \t\t\t\t\t\t(GParamFlags)(G_PARAM_STATIC_STRINGS | G_PARAM_WRITABLE |\n> > \t\t\t\t\t\t\t G_PARAM_CONSTRUCT_ONLY));\n> > \tg_object_class_install_property(object_class, PROP_DEVICE_NAME, pspec);\n> > +\tpspec = g_param_spec_boolean(\"enable-auto-focus\",\n> > +\t \"Enable auto-focus\",\n> > +\t \"Enable auto-focus if set to true, \"\n> > +\t \"disable it if set to false\",\n> > +\t FALSE, G_PARAM_WRITABLE);\n> \n> Incorrect indentation (you should use tabs instead of spaces, and the\n> last line is indented by one space too much.\n> \n> We have a tool, in utils/checkstyle.py, that checks for common coding\n> style violations and can in many cases propose fixes (see\n> https://libcamera.org/coding-style.html#tools). I recommend installing\n> it as a git post-commit hook, how to do so is also documented in the\n> previous URL.\n> \n> Please take the output of the tool with a pinch of salt though. For code\n> formatting, the tool is based on clang-format, and while it does a\n> fairly good job, not all suggestions are perfect. You can ignore some of\n> the warnings if you think that's appropriate (mostly based on the coding\n> style of the existing code in the file).\n> \n> > +\tg_object_class_install_property(object_class, PROP_ENABLE_AUTO_FOCUS, pspec);\n> > }\n> > \n> > static GstDevice *\n> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > index a10cbd4f..b2d8456b 100644\n> > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > @@ -146,6 +146,7 @@ struct _GstLibcameraSrc {\n> > \tGstTask *task;\n> > \n> > \tgchar *camera_name;\n> > +\tbool enable_auto_focus = false;\n> > \n> > \tGstLibcameraSrcState *state;\n> > \tGstLibcameraAllocator *allocator;\n> > @@ -154,7 +155,8 @@ struct _GstLibcameraSrc {\n> > \n> > enum {\n> > \tPROP_0,\n> > -\tPROP_CAMERA_NAME\n> > +\tPROP_CAMERA_NAME,\n> > +\tPROP_ENABLE_AUTO_FOCUS,\n> > };\n> > \n> > G_DEFINE_TYPE_WITH_CODE(GstLibcameraSrc, gst_libcamera_src, GST_TYPE_ELEMENT,\n> > @@ -577,6 +579,18 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> > \t\tgst_flow_combiner_add_pad(self->flow_combiner, srcpad);\n> > \t}\n> > \n> > +\tif (self->enable_auto_focus) {\n> > +\t\tconst ControlInfoMap &infoMap = state->cam_->controls();\n> > +\t\tif (infoMap.find(&controls::AfMode) != infoMap.end()) {\n> > +\t\t\tstate->initControls_.set(controls::AfMode, controls::AfModeContinuous);\n> > +\t\t} else {\n> > +\t\t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > +\t\t\t\t(\"Failed to enable auto focus\"),\n> > +\t\t\t\t(\"AfMode not found in camera controls, \"\n> > +\t\t\t\t\"please retry with 'enable-auto-focus=false'\"));\n> > +\t\t}\n> > +\t}\n> \n> We should be able to enable and disable auto-focus at runtime as well. I\n> don't want to ask for too much yak-shaving, so I'm fine with this patch\n> as an initial solution. Please note, however, that once we enable\n> support for controls more generically in libcamerasrc, the name of the\n> property may change. \n> \n> > +\n> > \tret = state->cam_->start(&state->initControls_);\n> > \tif (ret) {\n> > \t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > @@ -659,6 +673,9 @@ gst_libcamera_src_set_property(GObject *object, guint prop_id,\n> > \t\tg_free(self->camera_name);\n> > \t\tself->camera_name = g_value_dup_string(value);\n> > \t\tbreak;\n> > +\tcase PROP_ENABLE_AUTO_FOCUS:\n> > +\t\tself->enable_auto_focus = g_value_get_boolean(value);\n> > +\t\tbreak;\n> > \tdefault:\n> > \t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > \t\tbreak;\n> > @@ -676,6 +693,9 @@ gst_libcamera_src_get_property(GObject *object, guint prop_id, GValue *value,\n> > \tcase PROP_CAMERA_NAME:\n> > \t\tg_value_set_string(value, self->camera_name);\n> > \t\tbreak;\n> > +\tcase PROP_ENABLE_AUTO_FOCUS:\n> > +\t\tg_value_set_boolean(value, self->enable_auto_focus);\n> > +\t\tbreak;\n> > \tdefault:\n> > \t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > \t\tbreak;\n> > @@ -844,4 +864,11 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n> > \t\t\t\t\t\t\t | G_PARAM_READWRITE\n> > \t\t\t\t\t\t\t | G_PARAM_STATIC_STRINGS));\n> > \tg_object_class_install_property(object_class, PROP_CAMERA_NAME, spec);\n> > +\tspec = g_param_spec_boolean(\"enable-auto-focus\",\n> > +\t \"Enable auto-focus\",\n> > +\t \"Enable auto-focus if set to true, \"\n> > +\t \"disable it if set to false\",\n> > +\t FALSE, G_PARAM_WRITABLE);\n> > +\tg_object_class_install_property(object_class, PROP_ENABLE_AUTO_FOCUS, spec);\n> > +\n> > }\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BE641C3200\n\tfor ;\n\tMon, 5 Jun 2023 15:57:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 13DCA62888;\n\tMon, 5 Jun 2023 17:57:00 +0200 (CEST)","from mail-qk1-x72f.google.com (mail-qk1-x72f.google.com\n\t[IPv6:2607:f8b0:4864:20::72f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DD3786287D\n\tfor ;\n\tMon, 5 Jun 2023 17:56:58 +0200 (CEST)","by mail-qk1-x72f.google.com with SMTP id\n\taf79cd13be357-75d4b85b3ccso255447285a.2\n\tfor ;\n\tMon, 05 Jun 2023 08:56:58 -0700 (PDT)","from nicolas-tpx395.localdomain ([2606:6d00:11:5f2f::7a9])\n\tby smtp.gmail.com with ESMTPSA id\n\tpj15-20020a05620a1d8f00b0074df8eefe2dsm2286027qkn.98.2023.06.05.08.56.56\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 05 Jun 2023 08:56:57 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685980620;\n\tbh=nnkm3f+Mq0Edfmp3cHJhKOza0jBXLQAHVv8uXo6pdEw=;\n\th=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=1i5s7lklDF0w9oY70LkYPaXqPlTmVzqzuzg0h2lIVYzOoIIrLtpIosidNdj44iOcq\n\t6WUCWumMsb2fwFkol1Qaog38iJK7kkjXSxLt2yDjBCyFjLNF4ALBKZ/GcVpsgIdEFK\n\tL3cwaX1rHLkPS72kDL7WaM+ppNP2ghrCt5ggeHCzeyEh3+Tq3GWC8OapY/szNSjhyD\n\tzvU9Gqm4EOktq99upBrTYbJ3FLpldBCDpgihIJE2FT9u2eAK1nxkmQrSNvMXimKq8m\n\tJjXNVxMs9W47tyPo9MKfGuEYDObAGdR1D39R7us4FUOuH4EJYf8lDE9pqkKqt6NaKc\n\tWXrMEnXA15R1Q==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20221208.gappssmtp.com; s=20221208; t=1685980618;\n\tx=1688572618; \n\th=mime-version:user-agent:content-transfer-encoding:references\n\t:in-reply-to:date:cc:to:from:subject:message-id:from:to:cc:subject\n\t:date:message-id:reply-to;\n\tbh=VdDU12IjciWBCreefLGA/0T1WR+mnrbNgd47cad2kCk=;\n\tb=354A//cZwcYturUz2PYES96/JNVbXM82VPw+A0jH4x6t3lSux8mSf+Fh+71wdSlT/H\n\tQqQCT4NaKxcr4vmwD08UFZNP2lbfNKVhUzTCbEg76fuz4O3jrMR7kyGSqU8TsPWk3JvN\n\tT8a3vQu1e7mihgi2NPYBZFO2lqsnb3usj0B2hQHlgnPZyB8jwITZmgo2AJuqz23nycKV\n\tywZnXEo1gPEv2Cycb3NzWxR1gqnaAsn87zd47xLvPy7MwpYdnjH3cVswRXF/GM6VO0tB\n\t42LmmJPoJjza7yQREFCJxzSu8BvNQjFhqcrv2lpZvIVOopfnfRaRxoIhe5wwjRPm7aC5\n\tOe7Q=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=ndufresne-ca.20221208.gappssmtp.com\n\theader.i=@ndufresne-ca.20221208.gappssmtp.com header.b=\"354A//cZ\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1685980618; x=1688572618;\n\th=mime-version:user-agent:content-transfer-encoding:references\n\t:in-reply-to:date:cc:to:from:subject:message-id:x-gm-message-state\n\t:from:to:cc:subject:date:message-id:reply-to;\n\tbh=VdDU12IjciWBCreefLGA/0T1WR+mnrbNgd47cad2kCk=;\n\tb=DqJ+zBTC3hoDVADDhTdF97cByPKi0pSnvHCC9ZjNyf9tZxUHSH989BrWlNYUuEcPog\n\tBapeFYPRoOGhdXNMPWElA6oo18gxu4uEw4BwMQSxMJ5hXtai/6PfAZjonw/xWwO2Tylj\n\txAIj8byaoBk5/OW7w2p0nhSOLe/MLU6OosLewyAVWVSUXCUKinFdKgiOUWwvsAX9KAq6\n\tatLC+dAI2lhv4ETUaooQZuV6TOJXJ8rNOEY3dj2MBRL44eNEDmoJt5cexH6WGopeEmO1\n\trC0EKemtgeR51sGJUzWKaLIC1uedlrKfOrhfIfvl2u4YEAkpUebgKKXz0D1/OgMyaI/v\n\t2/cg==","X-Gm-Message-State":"AC+VfDyB2BskUxnGn+BNGfjvytLfqbpDQQaw6NAOZJwBh6adX4y6Q7cb\n\t4cVR0plv48KPCtM8kXtFSThVvUhJlvu54W2htrc=","X-Google-Smtp-Source":"ACHHUZ44psHSKvWOVX5gH3mDU1TJHKtcUZ+qGX6eL1TIFxnmgHfsVL5JXHZXKDyIyQXxSCs5mmNnMA==","X-Received":"by 2002:a37:6303:0:b0:75b:23a1:361c with SMTP id\n\tx3-20020a376303000000b0075b23a1361cmr115967qkb.45.1685980617668; \n\tMon, 05 Jun 2023 08:56:57 -0700 (PDT)","Message-ID":"","To":"Laurent Pinchart , Cedric Nugteren\n\t","Date":"Mon, 05 Jun 2023 11:56:56 -0400","In-Reply-To":"<20230605153331.GA7234@pendragon.ideasonboard.com>","References":"<20230605144405.118153-1-web@cedricnugteren.nl>\n\t<20230605153331.GA7234@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","User-Agent":"Evolution 3.48.1 (3.48.1-1.fc38) ","MIME-Version":"1.0","Subject":"Re: [libcamera-devel] [PATCH v2] Add enable_auto_focus option to\n\tthe GStreamer plugin","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Nicolas Dufresne via libcamera-devel\n\t","Reply-To":"Nicolas Dufresne ","Cc":"libcamera-devel@lists.libcamera.org,\n\tCedric Nugteren ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27264,"web_url":"https://patchwork.libcamera.org/comment/27264/","msgid":"<20230605160146.GB7234@pendragon.ideasonboard.com>","date":"2023-06-05T16:01:46","subject":"Re: [libcamera-devel] [PATCH v2] Add enable_auto_focus option to\n\tthe GStreamer plugin","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nicolas,\n\nOn Mon, Jun 05, 2023 at 11:47:51AM -0400, Nicolas Dufresne via libcamera-devel wrote:\n> Le lundi 05 juin 2023 à 16:44 +0200, Cedric Nugteren via libcamera-devel a écrit :\n> > Cameras such as the PiCam 3 support auto-focus, but the GStreamer plugin\n> > for libcamera does not enable auto-focus. With this patch auto-focus can\n> > be enabled for cameras that support it. By default it is disabled, which\n> > means default behaviour remains unchanged. For cameras that do not\n> > support auto-focus, an error message shows up if it auto-focus is\n> > enabled.\n> > \n> > This was tested on cameras that do not support auto-focus (e.g. PiCam2)\n> > and was tested on a camera that does support auto-focus (PiCam3) by\n> > enabling it, observing auto-focus, and by disabling it or by not setting\n> > the option, both resulting in auto-focus being disabled.\n> > \n> > This should close https://bugs.libcamera.org/show_bug.cgi?id=188.\n> > \n> > Signed-off-by: Cedric Nugteren \n> > ---\n> > Changes since v1:\n> > - Now re-using the GParamSpec variables instead of re-creating.\n> > - Use bool instead of gboolean for enable_auto_focus.\n> > - Remove changes from the reconfigure_element functions.\n> > ---\n> > src/gstreamer/gstlibcameraprovider.cpp | 12 +++++++++++\n> > src/gstreamer/gstlibcamerasrc.cpp | 29 +++++++++++++++++++++++++-\n> > 2 files changed, 40 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/src/gstreamer/gstlibcameraprovider.cpp b/src/gstreamer/gstlibcameraprovider.cpp\n> > index 6eb0a0eb..579cb8c0 100644\n> > --- a/src/gstreamer/gstlibcameraprovider.cpp\n> > +++ b/src/gstreamer/gstlibcameraprovider.cpp\n> > @@ -31,6 +31,7 @@ GST_DEBUG_CATEGORY_STATIC(provider_debug);\n> > \n> > enum {\n> > \tPROP_DEVICE_NAME = 1,\n> > +\tPROP_ENABLE_AUTO_FOCUS = 2,\n> > };\n> > \n> > #define GST_TYPE_LIBCAMERA_DEVICE gst_libcamera_device_get_type()\n> > @@ -40,6 +41,7 @@ G_DECLARE_FINAL_TYPE(GstLibcameraDevice, gst_libcamera_device,\n> > struct _GstLibcameraDevice {\n> > \tGstDevice parent;\n> > \tgchar *name;\n> > +\tbool enable_auto_focus = false;\n> > };\n> > \n> > G_DEFINE_TYPE(GstLibcameraDevice, gst_libcamera_device, GST_TYPE_DEVICE)\n> > @@ -56,6 +58,7 @@ gst_libcamera_device_create_element(GstDevice *device, const gchar *name)\n> > \tg_assert(source);\n> > \n> > \tg_object_set(source, \"camera-name\", GST_LIBCAMERA_DEVICE(device)->name, nullptr);\n> > +\tg_object_set(source, \"enable-auto-focus\", GST_LIBCAMERA_DEVICE(device)->enable_auto_focus, nullptr);\n> > \n> > \treturn source;\n> > }\n> > @@ -82,6 +85,9 @@ gst_libcamera_device_set_property(GObject *object, guint prop_id,\n> > \tcase PROP_DEVICE_NAME:\n> > \t\tdevice->name = g_value_dup_string(value);\n> > \t\tbreak;\n> > +\tcase PROP_ENABLE_AUTO_FOCUS:\n> > +\t\tdevice->enable_auto_focus = g_value_get_boolean(value);\n> > +\t\tbreak;\n> > \tdefault:\n> > \t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > \t\tbreak;\n> > @@ -121,6 +127,12 @@ gst_libcamera_device_class_init(GstLibcameraDeviceClass *klass)\n> > \t\t\t\t\t\t(GParamFlags)(G_PARAM_STATIC_STRINGS | G_PARAM_WRITABLE |\n> > \t\t\t\t\t\t\t G_PARAM_CONSTRUCT_ONLY));\n> > \tg_object_class_install_property(object_class, PROP_DEVICE_NAME, pspec);\n> > +\tpspec = g_param_spec_boolean(\"enable-auto-focus\",\n> > +\t \"Enable auto-focus\",\n> > +\t \"Enable auto-focus if set to true, \"\n> > +\t \"disable it if set to false\",\n> > +\t FALSE, G_PARAM_WRITABLE);\n> > +\tg_object_class_install_property(object_class, PROP_ENABLE_AUTO_FOCUS, pspec);\n> > }\n> > \n> > static GstDevice *\n> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > index a10cbd4f..b2d8456b 100644\n> > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > @@ -146,6 +146,7 @@ struct _GstLibcameraSrc {\n> > \tGstTask *task;\n> > \n> > \tgchar *camera_name;\n> > +\tbool enable_auto_focus = false;\n> > \n> > \tGstLibcameraSrcState *state;\n> > \tGstLibcameraAllocator *allocator;\n> > @@ -154,7 +155,8 @@ struct _GstLibcameraSrc {\n> > \n> > enum {\n> > \tPROP_0,\n> > -\tPROP_CAMERA_NAME\n> > +\tPROP_CAMERA_NAME,\n> > +\tPROP_ENABLE_AUTO_FOCUS,\n> > };\n> > \n> > G_DEFINE_TYPE_WITH_CODE(GstLibcameraSrc, gst_libcamera_src, GST_TYPE_ELEMENT,\n> > @@ -577,6 +579,18 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> > \t\tgst_flow_combiner_add_pad(self->flow_combiner, srcpad);\n> > \t}\n> > \n> > +\tif (self->enable_auto_focus) {\n> > +\t\tconst ControlInfoMap &infoMap = state->cam_->controls();\n> > +\t\tif (infoMap.find(&controls::AfMode) != infoMap.end()) {\n> > +\t\t\tstate->initControls_.set(controls::AfMode, controls::AfModeContinuous);\n> \n> I don't really like the idea of mapping modes to a boolean here. I think this\n> should be an GEnum type of property. Some mode requires some actions to actually\n> work, best to just skip these for now.\n\nIn the long run, we need to map all libcamera controls to GStreamer\nproperties. Given the amount of controls, I'm hoping we will be able to\neither do so in a dynamic way (similar to what v4l2src does when mapping\nV4L2 controls), or with code generation. That's why I'm not sure if we\nneed to spend more time now on mapping better this particular control\nmanually (as long as we don't need to preserve backward compatibility).\nI trust your judgement on this topic.\n\n> > +\t\t} else {\n> > +\t\t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > +\t\t\t\t(\"Failed to enable auto focus\"),\n> > +\t\t\t\t(\"AfMode not found in camera controls, \"\n> > +\t\t\t\t\"please retry with 'enable-auto-focus=false'\"));\n> > +\t\t}\n> > +\t}\n> > +\n> > \tret = state->cam_->start(&state->initControls_);\n> > \tif (ret) {\n> > \t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > @@ -659,6 +673,9 @@ gst_libcamera_src_set_property(GObject *object, guint prop_id,\n> > \t\tg_free(self->camera_name);\n> > \t\tself->camera_name = g_value_dup_string(value);\n> > \t\tbreak;\n> > +\tcase PROP_ENABLE_AUTO_FOCUS:\n> > +\t\tself->enable_auto_focus = g_value_get_boolean(value);\n> > +\t\tbreak;\n> > \tdefault:\n> > \t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > \t\tbreak;\n> > @@ -676,6 +693,9 @@ gst_libcamera_src_get_property(GObject *object, guint prop_id, GValue *value,\n> > \tcase PROP_CAMERA_NAME:\n> > \t\tg_value_set_string(value, self->camera_name);\n> > \t\tbreak;\n> > +\tcase PROP_ENABLE_AUTO_FOCUS:\n> > +\t\tg_value_set_boolean(value, self->enable_auto_focus);\n> > +\t\tbreak;\n> > \tdefault:\n> > \t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > \t\tbreak;\n> > @@ -844,4 +864,11 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n> > \t\t\t\t\t\t\t | G_PARAM_READWRITE\n> > \t\t\t\t\t\t\t | G_PARAM_STATIC_STRINGS));\n> > \tg_object_class_install_property(object_class, PROP_CAMERA_NAME, spec);\n> > +\tspec = g_param_spec_boolean(\"enable-auto-focus\",\n> > +\t \"Enable auto-focus\",\n> > +\t \"Enable auto-focus if set to true, \"\n> > +\t \"disable it if set to false\",\n> > +\t FALSE, G_PARAM_WRITABLE);\n> > +\tg_object_class_install_property(object_class, PROP_ENABLE_AUTO_FOCUS, spec);\n> > +\n> > }","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AAF05C31E9\n\tfor ;\n\tMon, 5 Jun 2023 16:01:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 214D46287D;\n\tMon, 5 Jun 2023 18:01:50 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D3E8B6287D\n\tfor ;\n\tMon, 5 Jun 2023 18:01:47 +0200 (CEST)","from pendragon.ideasonboard.com (om126156242094.26.openmobile.ne.jp\n\t[126.156.242.94])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 289D9BC;\n\tMon, 5 Jun 2023 18:01:21 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685980910;\n\tbh=XNFxbiKa+OIAxTZqQZ9+XpI3laMNDhOYKIjeYxmGze8=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=c3KbmnoXR/iSibr1/PvFZq0pMf1XbgTK+DxleluvUMJ6eV3pc2V6ynG2GPbToKUYw\n\tLKaPIpfXlWSMINOdsiracU5GkVJE8IhRonuo3gGSX8N7bzvYQrYERJtmf2AEzjPqoT\n\tdIwjdgNh7dUMNWENNOPDnjsrjxV4xfF6xOpaZZ4I27D3CFiR4kDP58ULuu4JWFlAhW\n\tS0eUWZY68jDUSJPl6w4Tzv4LkLkWScNOWe5uIkLfdVVdDm/SHtpE3Fi96ljC1kAozg\n\tdTGgLijutrDmt05hNymQaMO6EuSiRK8grLR2NSbBuJHdw+XkWwyL1kZ0BeA4rAH25M\n\tq8Jsf6xDfZgJA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685980883;\n\tbh=XNFxbiKa+OIAxTZqQZ9+XpI3laMNDhOYKIjeYxmGze8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VRfK03VMhLsADX+YNExuSfOuaTVRUE16Zzh8oc/Lb1pxED1e5B2NnpUsV819x7OpX\n\tLwT0BNe2kpXDE8j8bVDaMjRzHqcLp2CKjnVtwNori5QH4Bx2P0iac6VpWw9ZCAkMTF\n\t/Lsa+syfstBC9bSFcxGbnvl+z/BzGbRMlUPxP3YE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"VRfK03VM\"; dkim-atps=neutral","Date":"Mon, 5 Jun 2023 19:01:46 +0300","To":"Nicolas Dufresne ","Message-ID":"<20230605160146.GB7234@pendragon.ideasonboard.com>","References":"<20230605144405.118153-1-web@cedricnugteren.nl>\n\t<236fda3525cfc091bd6874fc2cfabc1d888afc7b.camel@ndufresne.ca>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<236fda3525cfc091bd6874fc2cfabc1d888afc7b.camel@ndufresne.ca>","Subject":"Re: [libcamera-devel] [PATCH v2] Add enable_auto_focus option to\n\tthe GStreamer plugin","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"Cedric Nugteren ,\n\tlibcamera-devel@lists.libcamera.org, \n\tCedric Nugteren ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27265,"web_url":"https://patchwork.libcamera.org/comment/27265/","msgid":"<20230605160455.GC7234@pendragon.ideasonboard.com>","date":"2023-06-05T16:04:55","subject":"Re: [libcamera-devel] [PATCH v2] Add enable_auto_focus option to\n\tthe GStreamer plugin","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nicolas,\n\nOn Mon, Jun 05, 2023 at 11:56:56AM -0400, Nicolas Dufresne wrote:\n> Le lundi 05 juin 2023 à 18:33 +0300, Laurent Pinchart a écrit :\n> > Hi Cedric,\n> > \n> > (CC'ing Nicolas)\n> > \n> > Thank you for the patch.\n> > \n> > On Mon, Jun 05, 2023 at 04:44:05PM +0200, Cedric Nugteren via libcamera-devel wrote:\n> > > Cameras such as the PiCam 3 support auto-focus, but the GStreamer plugin\n> > > for libcamera does not enable auto-focus. With this patch auto-focus can\n> > > be enabled for cameras that support it. By default it is disabled, which\n> > > means default behaviour remains unchanged. For cameras that do not\n> > > support auto-focus, an error message shows up if it auto-focus is\n> > \n> > I think you meant \"its auto-focus\" instead of \"it auto-focus\" ?\n> > \n> > > enabled.\n> > > \n> > > This was tested on cameras that do not support auto-focus (e.g. PiCam2)\n> > > and was tested on a camera that does support auto-focus (PiCam3) by\n> > > enabling it, observing auto-focus, and by disabling it or by not setting\n> > > the option, both resulting in auto-focus being disabled.\n> > > \n> > > This should close https://bugs.libcamera.org/show_bug.cgi?id=188.\n> > \n> > This should be written\n> > \n> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=188.\n> > \n> > which I realize isn't documented anywhere :-S I wonder where to best\n> > mention this. Is there any particular place in the documentation that\n> > would have caught your eyes ?\n> > \n> > > Signed-off-by: Cedric Nugteren \n> > > ---\n> > > Changes since v1:\n> > > - Now re-using the GParamSpec variables instead of re-creating.\n> > > - Use bool instead of gboolean for enable_auto_focus.\n> > > - Remove changes from the reconfigure_element functions.\n> > \n> > Perfect :-)\n> > \n> > > ---\n> > > src/gstreamer/gstlibcameraprovider.cpp | 12 +++++++++++\n> > > src/gstreamer/gstlibcamerasrc.cpp | 29 +++++++++++++++++++++++++-\n> > > 2 files changed, 40 insertions(+), 1 deletion(-)\n> > > \n> > > diff --git a/src/gstreamer/gstlibcameraprovider.cpp b/src/gstreamer/gstlibcameraprovider.cpp\n> > > index 6eb0a0eb..579cb8c0 100644\n> > > --- a/src/gstreamer/gstlibcameraprovider.cpp\n> > > +++ b/src/gstreamer/gstlibcameraprovider.cpp\n> > > @@ -31,6 +31,7 @@ GST_DEBUG_CATEGORY_STATIC(provider_debug);\n> > > \n> > > enum {\n> > > \tPROP_DEVICE_NAME = 1,\n> > > +\tPROP_ENABLE_AUTO_FOCUS = 2,\n> > > };\n> > > \n> > > #define GST_TYPE_LIBCAMERA_DEVICE gst_libcamera_device_get_type()\n> > > @@ -40,6 +41,7 @@ G_DECLARE_FINAL_TYPE(GstLibcameraDevice, gst_libcamera_device,\n> > > struct _GstLibcameraDevice {\n> > > \tGstDevice parent;\n> > > \tgchar *name;\n> > > +\tbool enable_auto_focus = false;\n> > > };\n> > > \n> > > G_DEFINE_TYPE(GstLibcameraDevice, gst_libcamera_device, GST_TYPE_DEVICE)\n> > > @@ -56,6 +58,7 @@ gst_libcamera_device_create_element(GstDevice *device, const gchar *name)\n> > > \tg_assert(source);\n> > > \n> > > \tg_object_set(source, \"camera-name\", GST_LIBCAMERA_DEVICE(device)->name, nullptr);\n> > > +\tg_object_set(source, \"enable-auto-focus\", GST_LIBCAMERA_DEVICE(device)->enable_auto_focus, nullptr);\n> > \n> > Nicolas, is there any particular rule when it comes to naming properties\n> > for GStreamer elements ?\n> \n> The GLib part of it is:\n> \n> A property name consists of one or more segments consisting of ASCII letters and\n> digits, separated by either the - or _ character. The first character of a\n> property name must be a letter. These are the same rules as for signal naming\n> (see g_signal_new()).\n> \n> I strongly prefer - instead of _. Generally speaking, I would like this\n> discussion to happen before we start adding controls. One of the thing I was\n> considering was to auto-generate the propertly from the yaml,\n\nSeems like we have the same goal :-) I've just replied to your other\ne-mail suggesting this.\n\nAnother option would be to create the mappings dynamically, based on the\ninformation reported by the camera at runtime.\n\n> and clearly this\n> one is not a 1:1 mapping to the libcamera controls. But at the same time, there\n> is bunch of needed exception for semantic reasons. The yaml is not descriptive\n> enough to configure fully the properties unfortunately.\n\nWe can extend the yaml file with additional information if needed,\nand/or extend the information reported by the camera at runtime\n(assuming GStreamer wouldn't have unreasonable requirements of course\n:-)).\n\n> As its easier to express what I'm hoping for with code, here's my own WIP, which\n> is much more C++ like then this one. I'm missing the AutoFocusMode.\n> \n> https://gitlab.collabora.com/nicolas/libcamera/-/commit/83d67744ccbd60ad67d2afc70be7f2d95032e4d7\n\nNice ! Thank you for working on that. I'll try to give it a look ASAP.\n\n> > > \n> > > \treturn source;\n> > > }\n> > > @@ -82,6 +85,9 @@ gst_libcamera_device_set_property(GObject *object, guint prop_id,\n> > > \tcase PROP_DEVICE_NAME:\n> > > \t\tdevice->name = g_value_dup_string(value);\n> > > \t\tbreak;\n> > > +\tcase PROP_ENABLE_AUTO_FOCUS:\n> > > +\t\tdevice->enable_auto_focus = g_value_get_boolean(value);\n> > > +\t\tbreak;\n> > > \tdefault:\n> > > \t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > > \t\tbreak;\n> > > @@ -121,6 +127,12 @@ gst_libcamera_device_class_init(GstLibcameraDeviceClass *klass)\n> > > \t\t\t\t\t\t(GParamFlags)(G_PARAM_STATIC_STRINGS | G_PARAM_WRITABLE |\n> > > \t\t\t\t\t\t\t G_PARAM_CONSTRUCT_ONLY));\n> > > \tg_object_class_install_property(object_class, PROP_DEVICE_NAME, pspec);\n> > > +\tpspec = g_param_spec_boolean(\"enable-auto-focus\",\n> > > +\t \"Enable auto-focus\",\n> > > +\t \"Enable auto-focus if set to true, \"\n> > > +\t \"disable it if set to false\",\n> > > +\t FALSE, G_PARAM_WRITABLE);\n> > \n> > Incorrect indentation (you should use tabs instead of spaces, and the\n> > last line is indented by one space too much.\n> > \n> > We have a tool, in utils/checkstyle.py, that checks for common coding\n> > style violations and can in many cases propose fixes (see\n> > https://libcamera.org/coding-style.html#tools). I recommend installing\n> > it as a git post-commit hook, how to do so is also documented in the\n> > previous URL.\n> > \n> > Please take the output of the tool with a pinch of salt though. For code\n> > formatting, the tool is based on clang-format, and while it does a\n> > fairly good job, not all suggestions are perfect. You can ignore some of\n> > the warnings if you think that's appropriate (mostly based on the coding\n> > style of the existing code in the file).\n> > \n> > > +\tg_object_class_install_property(object_class, PROP_ENABLE_AUTO_FOCUS, pspec);\n> > > }\n> > > \n> > > static GstDevice *\n> > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > > index a10cbd4f..b2d8456b 100644\n> > > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > > @@ -146,6 +146,7 @@ struct _GstLibcameraSrc {\n> > > \tGstTask *task;\n> > > \n> > > \tgchar *camera_name;\n> > > +\tbool enable_auto_focus = false;\n> > > \n> > > \tGstLibcameraSrcState *state;\n> > > \tGstLibcameraAllocator *allocator;\n> > > @@ -154,7 +155,8 @@ struct _GstLibcameraSrc {\n> > > \n> > > enum {\n> > > \tPROP_0,\n> > > -\tPROP_CAMERA_NAME\n> > > +\tPROP_CAMERA_NAME,\n> > > +\tPROP_ENABLE_AUTO_FOCUS,\n> > > };\n> > > \n> > > G_DEFINE_TYPE_WITH_CODE(GstLibcameraSrc, gst_libcamera_src, GST_TYPE_ELEMENT,\n> > > @@ -577,6 +579,18 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> > > \t\tgst_flow_combiner_add_pad(self->flow_combiner, srcpad);\n> > > \t}\n> > > \n> > > +\tif (self->enable_auto_focus) {\n> > > +\t\tconst ControlInfoMap &infoMap = state->cam_->controls();\n> > > +\t\tif (infoMap.find(&controls::AfMode) != infoMap.end()) {\n> > > +\t\t\tstate->initControls_.set(controls::AfMode, controls::AfModeContinuous);\n> > > +\t\t} else {\n> > > +\t\t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > > +\t\t\t\t(\"Failed to enable auto focus\"),\n> > > +\t\t\t\t(\"AfMode not found in camera controls, \"\n> > > +\t\t\t\t\"please retry with 'enable-auto-focus=false'\"));\n> > > +\t\t}\n> > > +\t}\n> > \n> > We should be able to enable and disable auto-focus at runtime as well. I\n> > don't want to ask for too much yak-shaving, so I'm fine with this patch\n> > as an initial solution. Please note, however, that once we enable\n> > support for controls more generically in libcamerasrc, the name of the\n> > property may change. \n> > \n> > > +\n> > > \tret = state->cam_->start(&state->initControls_);\n> > > \tif (ret) {\n> > > \t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > > @@ -659,6 +673,9 @@ gst_libcamera_src_set_property(GObject *object, guint prop_id,\n> > > \t\tg_free(self->camera_name);\n> > > \t\tself->camera_name = g_value_dup_string(value);\n> > > \t\tbreak;\n> > > +\tcase PROP_ENABLE_AUTO_FOCUS:\n> > > +\t\tself->enable_auto_focus = g_value_get_boolean(value);\n> > > +\t\tbreak;\n> > > \tdefault:\n> > > \t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > > \t\tbreak;\n> > > @@ -676,6 +693,9 @@ gst_libcamera_src_get_property(GObject *object, guint prop_id, GValue *value,\n> > > \tcase PROP_CAMERA_NAME:\n> > > \t\tg_value_set_string(value, self->camera_name);\n> > > \t\tbreak;\n> > > +\tcase PROP_ENABLE_AUTO_FOCUS:\n> > > +\t\tg_value_set_boolean(value, self->enable_auto_focus);\n> > > +\t\tbreak;\n> > > \tdefault:\n> > > \t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > > \t\tbreak;\n> > > @@ -844,4 +864,11 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n> > > \t\t\t\t\t\t\t | G_PARAM_READWRITE\n> > > \t\t\t\t\t\t\t | G_PARAM_STATIC_STRINGS));\n> > > \tg_object_class_install_property(object_class, PROP_CAMERA_NAME, spec);\n> > > +\tspec = g_param_spec_boolean(\"enable-auto-focus\",\n> > > +\t \"Enable auto-focus\",\n> > > +\t \"Enable auto-focus if set to true, \"\n> > > +\t \"disable it if set to false\",\n> > > +\t FALSE, G_PARAM_WRITABLE);\n> > > +\tg_object_class_install_property(object_class, PROP_ENABLE_AUTO_FOCUS, spec);\n> > > +\n> > > }","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 49221C3200\n\tfor ;\n\tMon, 5 Jun 2023 16:04:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BBAD562880;\n\tMon, 5 Jun 2023 18:04:58 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 39FA36287D\n\tfor ;\n\tMon, 5 Jun 2023 18:04:57 +0200 (CEST)","from pendragon.ideasonboard.com (om126156242094.26.openmobile.ne.jp\n\t[126.156.242.94])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6A2F6BC;\n\tMon, 5 Jun 2023 18:04:31 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685981098;\n\tbh=dUd5zTUpibPthU17PiUP3w7leKeS0gpzlpXXLGAYb4w=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=dckww5kNBzj+EMguPJPWjTcIBWMfg0OrGN9Gs6pfBf65uNejrYfEWpsuqvStKQR1X\n\tZumbGOw6XUb+YcSK2IH/oAyHvpBwr8cZxQIQmqOeMgcsGdme2ysWSfkVh9AjUabZ+Y\n\tRxYgA+nyqxJnrm6NfQYj+Fw7q6MW4yRakc3aIJ6hRRlx29zU9aAtBC+ww4lIu7oXrT\n\tCdcoY2hLJLJEOUL+PTjRYniAuPQnrUGiF6l6yMhiHLKP/D1b0Ggh8hTXfGXP42qt3m\n\tAJVKz7bTeZ6i3//iz7HqmC9rbCzURZr77jPHgPk9JablAR48mZ8w6SXjAkkxwHhXit\n\t46XN3iitUx0gQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685981072;\n\tbh=dUd5zTUpibPthU17PiUP3w7leKeS0gpzlpXXLGAYb4w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=H6znL3NXcufzTFlm8dY5PfR+Pb7LBfH/iejUAFP02AhziB6lKYYDLodsp0QzYofAq\n\tB2VlxKNP7u3mQl79c1rXC/3xa6eNKYyM709Ofde3JlWxA61L7+PSJokXjHDI6g+qaS\n\tUL+qD91gz9SmMtACU0nODZUryVPQ/7Yl3f9qY8uA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"H6znL3NX\"; dkim-atps=neutral","Date":"Mon, 5 Jun 2023 19:04:55 +0300","To":"Nicolas Dufresne ","Message-ID":"<20230605160455.GC7234@pendragon.ideasonboard.com>","References":"<20230605144405.118153-1-web@cedricnugteren.nl>\n\t<20230605153331.GA7234@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2] Add enable_auto_focus option to\n\tthe GStreamer plugin","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"Cedric Nugteren ,\n\tlibcamera-devel@lists.libcamera.org, \n\tCedric Nugteren ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27267,"web_url":"https://patchwork.libcamera.org/comment/27267/","msgid":"","date":"2023-06-05T17:52:41","subject":"Re: [libcamera-devel] [PATCH v2] Add enable_auto_focus option to\n\tthe GStreamer plugin","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Le lundi 05 juin 2023 à 19:04 +0300, Laurent Pinchart a écrit\n> > \n[...]\n\n> > > Nicolas, is there any particular rule when it comes to naming properties\n> > > for GStreamer elements ?\n> > \n> > The GLib part of it is:\n> > \n> > A property name consists of one or more segments consisting of ASCII letters and\n> > digits, separated by either the - or _ character. The first character of a\n> > property name must be a letter. These are the same rules as for signal naming\n> > (see g_signal_new()).\n> > \n> > I strongly prefer - instead of _. Generally speaking, I would like this\n> > discussion to happen before we start adding controls. One of the thing I was\n> > considering was to auto-generate the propertly from the yaml,\n> \n> Seems like we have the same goal :-) I've just replied to your other\n> e-mail suggesting this.\n> \n> Another option would be to create the mappings dynamically, based on the\n> information reported by the camera at runtime.\n\nYou can't add/remove/modify GObject properties at run-time. The properties are\nconsidered part of the API and should be stable.\n\nThe alternative would be to generate per camera elements, as used for CODECs,\nbut this currently only works for hardwired hardware and would not work well for\nhot-plug capable cameras. This would need lots more infrastructure into\nGStreamer, and for this reason, I would refrain from going that direction.\n\n> \n> > and clearly this\n> > one is not a 1:1 mapping to the libcamera controls. But at the same time, there\n> > is bunch of needed exception for semantic reasons. The yaml is not descriptive\n> > enough to configure fully the properties unfortunately.\n> \n> We can extend the yaml file with additional information if needed,\n> and/or extend the information reported by the camera at runtime\n> (assuming GStreamer wouldn't have unreasonable requirements of course\n> :-)).\n\nCurrently known issues are arrays, which can possibly be exposed as lists\nproperties, but aren't as nice to use. Based on current usage of arrays, a\nsimple mapping like -red/-blue/-red would look nicer. To be discussed.\n\nRanges are not all well exposed. I think the idea is that range are per camera,\nbut this isn't valid for GObject properties.\n\nThere is probably more, but I cannot remember at the moment.\n\n> \n> > As its easier to express what I'm hoping for with code, here's my own WIP, which\n> > is much more C++ like then this one. I'm missing the AutoFocusMode.\n> > \n> > https://gitlab.collabora.com/nicolas/libcamera/-/commit/83d67744ccbd60ad67d2afc70be7f2d95032e4d7\n> \n> Nice ! Thank you for working on that. I'll try to give it a look ASAP.\n> \n> > > \n\nIts a pleasure. Cedric, for your interest, this does not means in anyway we'll\nreject your changes. I think the only changes I'd suggest is to use a enum, and\nexpose auto-focus-mode with the 2 modes you currently support. This way we won't\nbreak our users application as soon as we have these generalized.\n\nNicolas","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 21587C3200\n\tfor ;\n\tMon, 5 Jun 2023 17:52:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7FF3B6288D;\n\tMon, 5 Jun 2023 19:52:45 +0200 (CEST)","from mail-oi1-x22b.google.com (mail-oi1-x22b.google.com\n\t[IPv6:2607:f8b0:4864:20::22b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 265736287D\n\tfor ;\n\tMon, 5 Jun 2023 19:52:44 +0200 (CEST)","by mail-oi1-x22b.google.com with SMTP id\n\t5614622812f47-38e04d1b2b4so4298556b6e.3\n\tfor ;\n\tMon, 05 Jun 2023 10:52:44 -0700 (PDT)","from nicolas-tpx395.localdomain ([2606:6d00:11:5f2f::7a9])\n\tby smtp.gmail.com with ESMTPSA id\n\tg13-20020ad4514d000000b006260e7361ebsm4787893qvq.1.2023.06.05.10.52.42\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 05 Jun 2023 10:52:42 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685987565;\n\tbh=XRgGDMspTbGWlF0tCLQGljUq0jTOuberopQMFqJJ0dc=;\n\th=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=oFfCoHmLuDNhXP5QDh3iSfQNAFMuNGars+lCZZ5qtDnkcDLXNIsQ3qMYz5E24Vb2l\n\t28OoB157EBeAk+BO4bVxGM9TlqOkDTFfxQBXwBiOSESSCqH78wBxCazTIhiQRLj778\n\tU4OuygzsEvapW5bW8EL73P2hqaygsYYJLtIw44lEnxgG9TsO2HT+xAcsWppsWUlCE1\n\t46Tl2GGpW/+GppXusiOkQu3qXnNcnqWqxLsLTUgia8hsuv0dVGULYsxYK5lKorDd7C\n\tWNHCyigweJnvB4EumXnSPIVx0o82AR5JEhVTooIXfm9FgKxjTvwknMHYpS1gekx4Op\n\t3lMsmU/NTzRxg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20221208.gappssmtp.com; s=20221208; t=1685987563;\n\tx=1688579563; \n\th=mime-version:user-agent:content-transfer-encoding:references\n\t:in-reply-to:date:cc:to:from:subject:message-id:from:to:cc:subject\n\t:date:message-id:reply-to;\n\tbh=s9FLN5rp7rFZJE5CDTtWXbh8+v7LJfiX95/Y984NUfU=;\n\tb=WYV5GcJCTHxGuw1ry7ShPaSm7sBwIxk+/LbUttT+i8sPtOfYQwSPQKzuVTP9WiIBYs\n\tLXd73vi48Y4SetSEobCZnZiEJ4eHDr0dAf+cX9ycfhCBAEPXWNXkJkZfuPdbDWrePfBs\n\tqmuw7k3wZWbIhLMUWHgI/jo9hZsB/2tXGP7nAq7ehmG5tDCJno8BTXBn7aXNK0RP5ucf\n\tRHAIDXkWfxja9vXSNzji+uB8QBBMQzc76VDLetaqhqcJe5CeXhDiPRMMJ42MC5uc/S60\n\tyFsbzdtoCSI1CLKtS1t9Kgb8bsJesS/xgqm67pvuiTFz0ID5+dJjDs+jKwbKHeVBFO+D\n\tYlkw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=ndufresne-ca.20221208.gappssmtp.com\n\theader.i=@ndufresne-ca.20221208.gappssmtp.com header.b=\"WYV5GcJC\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1685987563; x=1688579563;\n\th=mime-version:user-agent:content-transfer-encoding:references\n\t:in-reply-to:date:cc:to:from:subject:message-id:x-gm-message-state\n\t:from:to:cc:subject:date:message-id:reply-to;\n\tbh=s9FLN5rp7rFZJE5CDTtWXbh8+v7LJfiX95/Y984NUfU=;\n\tb=XjX9Y5Rm0lwSHA5MHOtH8BWjE8055WDGD9cTAVvl5dWzaKqbx3bom1dN9vzqNsgU6v\n\t8hfN4k9Bo+Gw6znCV86rbSzzjM5uTh7RKOBv5ZyVa1Phmz0qlmeYPpPUOGh7kZC1G8gx\n\tRqCEiz4OZIKjoiVvG4vpKRXT/a5Ndmt4yyNXw/uuFnEW6pi4ktSNgbbnsLM5Qyzri2eg\n\tfnFUe+BPv/TzGsnyvfTTL3NvH0EJQZzQTy+fVwkw4RDvSSsHfI1EFmuTa6YS4lU1rfOc\n\tAWRkFqMYnWaSCw8mu7ETV4YfIXR8X8zddXPRp4jVBqkoSkC/i0FF9A/ErPKizGOETlK2\n\tZC9Q==","X-Gm-Message-State":"AC+VfDyDn2uLEKRAD/3wz2UtuTBvZmrx6rUt0hCKk7RhW8junN3+RRzX\n\tw7N+FdXAK90T9VDkdRlkv12jdbHZ6iBTrLvLbYw=","X-Google-Smtp-Source":"ACHHUZ7rjW3mmkjYlmgcgObtZE03i09IxSIJly6NKq2LWkou2A5TMkkWVJNz7zfVMZnQNA++uWZr4Q==","X-Received":"by 2002:a05:6808:dc7:b0:39a:a99a:2195 with SMTP id\n\tg7-20020a0568080dc700b0039aa99a2195mr5003012oic.31.1685987562705; \n\tMon, 05 Jun 2023 10:52:42 -0700 (PDT)","Message-ID":"","To":"Laurent Pinchart ","Date":"Mon, 05 Jun 2023 13:52:41 -0400","In-Reply-To":"<20230605160455.GC7234@pendragon.ideasonboard.com>","References":"<20230605144405.118153-1-web@cedricnugteren.nl>\n\t<20230605153331.GA7234@pendragon.ideasonboard.com>\n\t\n\t<20230605160455.GC7234@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","User-Agent":"Evolution 3.48.1 (3.48.1-1.fc38) ","MIME-Version":"1.0","Subject":"Re: [libcamera-devel] [PATCH v2] Add enable_auto_focus option to\n\tthe GStreamer plugin","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Nicolas Dufresne via libcamera-devel\n\t","Reply-To":"Nicolas Dufresne ","Cc":"Cedric Nugteren ,\n\tlibcamera-devel@lists.libcamera.org, \n\tCedric Nugteren ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27268,"web_url":"https://patchwork.libcamera.org/comment/27268/","msgid":"","date":"2023-06-05T17:54:28","subject":"Re: [libcamera-devel] [PATCH v2] Add enable_auto_focus option to\n\tthe GStreamer plugin","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Hi,\n\nLe lundi 05 juin 2023 à 19:01 +0300, Laurent Pinchart a écrit :\n> Hi Nicolas,\n> \n> On Mon, Jun 05, 2023 at 11:47:51AM -0400, Nicolas Dufresne via libcamera-devel wrote:\n> > Le lundi 05 juin 2023 à 16:44 +0200, Cedric Nugteren via libcamera-devel a écrit :\n> > > Cameras such as the PiCam 3 support auto-focus, but the GStreamer plugin\n> > > for libcamera does not enable auto-focus. With this patch auto-focus can\n> > > be enabled for cameras that support it. By default it is disabled, which\n> > > means default behaviour remains unchanged. For cameras that do not\n> > > support auto-focus, an error message shows up if it auto-focus is\n> > > enabled.\n> > > \n> > > This was tested on cameras that do not support auto-focus (e.g. PiCam2)\n> > > and was tested on a camera that does support auto-focus (PiCam3) by\n> > > enabling it, observing auto-focus, and by disabling it or by not setting\n> > > the option, both resulting in auto-focus being disabled.\n> > > \n> > > This should close https://bugs.libcamera.org/show_bug.cgi?id=188.\n> > > \n> > > Signed-off-by: Cedric Nugteren \n> > > ---\n> > > Changes since v1:\n> > > - Now re-using the GParamSpec variables instead of re-creating.\n> > > - Use bool instead of gboolean for enable_auto_focus.\n> > > - Remove changes from the reconfigure_element functions.\n> > > ---\n> > > src/gstreamer/gstlibcameraprovider.cpp | 12 +++++++++++\n> > > src/gstreamer/gstlibcamerasrc.cpp | 29 +++++++++++++++++++++++++-\n> > > 2 files changed, 40 insertions(+), 1 deletion(-)\n> > > \n> > > diff --git a/src/gstreamer/gstlibcameraprovider.cpp b/src/gstreamer/gstlibcameraprovider.cpp\n> > > index 6eb0a0eb..579cb8c0 100644\n> > > --- a/src/gstreamer/gstlibcameraprovider.cpp\n> > > +++ b/src/gstreamer/gstlibcameraprovider.cpp\n> > > @@ -31,6 +31,7 @@ GST_DEBUG_CATEGORY_STATIC(provider_debug);\n> > > \n> > > enum {\n> > > \tPROP_DEVICE_NAME = 1,\n> > > +\tPROP_ENABLE_AUTO_FOCUS = 2,\n> > > };\n> > > \n> > > #define GST_TYPE_LIBCAMERA_DEVICE gst_libcamera_device_get_type()\n> > > @@ -40,6 +41,7 @@ G_DECLARE_FINAL_TYPE(GstLibcameraDevice, gst_libcamera_device,\n> > > struct _GstLibcameraDevice {\n> > > \tGstDevice parent;\n> > > \tgchar *name;\n> > > +\tbool enable_auto_focus = false;\n> > > };\n> > > \n> > > G_DEFINE_TYPE(GstLibcameraDevice, gst_libcamera_device, GST_TYPE_DEVICE)\n> > > @@ -56,6 +58,7 @@ gst_libcamera_device_create_element(GstDevice *device, const gchar *name)\n> > > \tg_assert(source);\n> > > \n> > > \tg_object_set(source, \"camera-name\", GST_LIBCAMERA_DEVICE(device)->name, nullptr);\n> > > +\tg_object_set(source, \"enable-auto-focus\", GST_LIBCAMERA_DEVICE(device)->enable_auto_focus, nullptr);\n> > > \n> > > \treturn source;\n> > > }\n> > > @@ -82,6 +85,9 @@ gst_libcamera_device_set_property(GObject *object, guint prop_id,\n> > > \tcase PROP_DEVICE_NAME:\n> > > \t\tdevice->name = g_value_dup_string(value);\n> > > \t\tbreak;\n> > > +\tcase PROP_ENABLE_AUTO_FOCUS:\n> > > +\t\tdevice->enable_auto_focus = g_value_get_boolean(value);\n> > > +\t\tbreak;\n> > > \tdefault:\n> > > \t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > > \t\tbreak;\n> > > @@ -121,6 +127,12 @@ gst_libcamera_device_class_init(GstLibcameraDeviceClass *klass)\n> > > \t\t\t\t\t\t(GParamFlags)(G_PARAM_STATIC_STRINGS | G_PARAM_WRITABLE |\n> > > \t\t\t\t\t\t\t G_PARAM_CONSTRUCT_ONLY));\n> > > \tg_object_class_install_property(object_class, PROP_DEVICE_NAME, pspec);\n> > > +\tpspec = g_param_spec_boolean(\"enable-auto-focus\",\n> > > +\t \"Enable auto-focus\",\n> > > +\t \"Enable auto-focus if set to true, \"\n> > > +\t \"disable it if set to false\",\n> > > +\t FALSE, G_PARAM_WRITABLE);\n> > > +\tg_object_class_install_property(object_class, PROP_ENABLE_AUTO_FOCUS, pspec);\n> > > }\n> > > \n> > > static GstDevice *\n> > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > > index a10cbd4f..b2d8456b 100644\n> > > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > > @@ -146,6 +146,7 @@ struct _GstLibcameraSrc {\n> > > \tGstTask *task;\n> > > \n> > > \tgchar *camera_name;\n> > > +\tbool enable_auto_focus = false;\n> > > \n> > > \tGstLibcameraSrcState *state;\n> > > \tGstLibcameraAllocator *allocator;\n> > > @@ -154,7 +155,8 @@ struct _GstLibcameraSrc {\n> > > \n> > > enum {\n> > > \tPROP_0,\n> > > -\tPROP_CAMERA_NAME\n> > > +\tPROP_CAMERA_NAME,\n> > > +\tPROP_ENABLE_AUTO_FOCUS,\n> > > };\n> > > \n> > > G_DEFINE_TYPE_WITH_CODE(GstLibcameraSrc, gst_libcamera_src, GST_TYPE_ELEMENT,\n> > > @@ -577,6 +579,18 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> > > \t\tgst_flow_combiner_add_pad(self->flow_combiner, srcpad);\n> > > \t}\n> > > \n> > > +\tif (self->enable_auto_focus) {\n> > > +\t\tconst ControlInfoMap &infoMap = state->cam_->controls();\n> > > +\t\tif (infoMap.find(&controls::AfMode) != infoMap.end()) {\n> > > +\t\t\tstate->initControls_.set(controls::AfMode, controls::AfModeContinuous);\n> > \n> > I don't really like the idea of mapping modes to a boolean here. I think this\n> > should be an GEnum type of property. Some mode requires some actions to actually\n> > work, best to just skip these for now.\n> \n> In the long run, we need to map all libcamera controls to GStreamer\n> properties. Given the amount of controls, I'm hoping we will be able to\n> either do so in a dynamic way (similar to what v4l2src does when mapping\n> V4L2 controls), or with code generation. That's why I'm not sure if we\n> need to spend more time now on mapping better this particular control\n> manually (as long as we don't need to preserve backward compatibility).\n> I trust your judgement on this topic.\n\nProperties are part of the API, so renaming them changing their type will break\napplications using it. I'd strongly suggest to make this an enum (this is easy)\nwith the two supporting values. Adding more enums values later is not a break.\n\n> \n> > > +\t\t} else {\n> > > +\t\t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > > +\t\t\t\t(\"Failed to enable auto focus\"),\n> > > +\t\t\t\t(\"AfMode not found in camera controls, \"\n> > > +\t\t\t\t\"please retry with 'enable-auto-focus=false'\"));\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > \tret = state->cam_->start(&state->initControls_);\n> > > \tif (ret) {\n> > > \t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > > @@ -659,6 +673,9 @@ gst_libcamera_src_set_property(GObject *object, guint prop_id,\n> > > \t\tg_free(self->camera_name);\n> > > \t\tself->camera_name = g_value_dup_string(value);\n> > > \t\tbreak;\n> > > +\tcase PROP_ENABLE_AUTO_FOCUS:\n> > > +\t\tself->enable_auto_focus = g_value_get_boolean(value);\n> > > +\t\tbreak;\n> > > \tdefault:\n> > > \t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > > \t\tbreak;\n> > > @@ -676,6 +693,9 @@ gst_libcamera_src_get_property(GObject *object, guint prop_id, GValue *value,\n> > > \tcase PROP_CAMERA_NAME:\n> > > \t\tg_value_set_string(value, self->camera_name);\n> > > \t\tbreak;\n> > > +\tcase PROP_ENABLE_AUTO_FOCUS:\n> > > +\t\tg_value_set_boolean(value, self->enable_auto_focus);\n> > > +\t\tbreak;\n> > > \tdefault:\n> > > \t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > > \t\tbreak;\n> > > @@ -844,4 +864,11 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n> > > \t\t\t\t\t\t\t | G_PARAM_READWRITE\n> > > \t\t\t\t\t\t\t | G_PARAM_STATIC_STRINGS));\n> > > \tg_object_class_install_property(object_class, PROP_CAMERA_NAME, spec);\n> > > +\tspec = g_param_spec_boolean(\"enable-auto-focus\",\n> > > +\t \"Enable auto-focus\",\n> > > +\t \"Enable auto-focus if set to true, \"\n> > > +\t \"disable it if set to false\",\n> > > +\t FALSE, G_PARAM_WRITABLE);\n> > > +\tg_object_class_install_property(object_class, PROP_ENABLE_AUTO_FOCUS, spec);\n> > > +\n> > > }\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AAF89C31E9\n\tfor ;\n\tMon, 5 Jun 2023 17:54:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1E7E362886;\n\tMon, 5 Jun 2023 19:54:33 +0200 (CEST)","from mail-qv1-xf2c.google.com (mail-qv1-xf2c.google.com\n\t[IPv6:2607:f8b0:4864:20::f2c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3BFF762880\n\tfor ;\n\tMon, 5 Jun 2023 19:54:31 +0200 (CEST)","by mail-qv1-xf2c.google.com with SMTP id\n\t6a1803df08f44-6237faa8677so37583466d6.1\n\tfor ;\n\tMon, 05 Jun 2023 10:54:31 -0700 (PDT)","from nicolas-tpx395.localdomain ([2606:6d00:11:5f2f::7a9])\n\tby smtp.gmail.com with ESMTPSA id\n\ts9-20020a0cf789000000b005dd8b934579sm4779844qvn.17.2023.06.05.10.54.29\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 05 Jun 2023 10:54:29 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685987673;\n\tbh=x00n3pfbHtMgbM3v2AEOVHDRSiE2hQWy/XUs6E+/Q9E=;\n\th=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=SLfNnPqhbfqfJ+ake9Y2tvqyXbO9MsvSnwdEttKYJ8DpUUj3OweFBjVPfs119fxBz\n\t844nMPnOyavaOu3A2k1rtxET7PEDtSXmiKS0fM+4AOvAmw0lZ9ou+GGh+ha8K3I4Kk\n\tHQAO722x7l6nZL1MTUt5Keqa0atcrrsuMrUomSH10LTv93PKluVHtzUa3lzoIxDbDv\n\t8rnLSRTMQU3ZaTq7e6y6o2BrZQkJqQix72eaJ/eEa3NDMHLmmn2SgwQrr+2hH6R57c\n\tk39+JVENghlX3+33AiKjFyISiZ/ho1ldnAn9QmMELon/cwGealWAd/XxGwYW0DhsPi\n\t5746RWFOTfFFg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20221208.gappssmtp.com; s=20221208; t=1685987670;\n\tx=1688579670; \n\th=mime-version:user-agent:content-transfer-encoding:references\n\t:in-reply-to:date:cc:to:from:subject:message-id:from:to:cc:subject\n\t:date:message-id:reply-to;\n\tbh=FqbaM6kqKJL4jz/lFUyjBe3RPCDYYJFwKuohVTnagao=;\n\tb=x/RLhgDyGUOoyWGx/Amvk7TsG5rOkRQu1V2j46777DsbMYNmOeI16aOnwNeYvT2l2w\n\tztDuAILn18FcdMys5wUibbHxuvVQOl1meDITmnxxp65NVmk0A13bZaA1IHeRfI12mhMp\n\tpFXTb3oscYKzcdi3PdoneIQ8GnwVe2UQW/WGyNOuax5MnmeuZUPay2UloOs1FtLhMw9g\n\ttgw7kGAbWHz0R0/jLc8rzb4LnEgioclqfG01+55OmdvzWvnnbxwsED1leuOk6TlT68eU\n\tVnKJOqD+cHy1MqRYxbnWSqAPETOFq6bIZCgIDV+mcs4mT+QtW0kPdTyQPqfrcODtecD6\n\tjaAA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=ndufresne-ca.20221208.gappssmtp.com\n\theader.i=@ndufresne-ca.20221208.gappssmtp.com header.b=\"x/RLhgDy\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1685987670; x=1688579670;\n\th=mime-version:user-agent:content-transfer-encoding:references\n\t:in-reply-to:date:cc:to:from:subject:message-id:x-gm-message-state\n\t:from:to:cc:subject:date:message-id:reply-to;\n\tbh=FqbaM6kqKJL4jz/lFUyjBe3RPCDYYJFwKuohVTnagao=;\n\tb=akOH99uOcCEXI0TkxQ1juu/sM7UcZ//z4V3H/9poda3mTT0lkUjcf2RLYW9mfD9Zkr\n\tDsKYx0YupBhhpCMeFcNK4OXWjXFnOSKJFEPuMlTi6U+ibvgmAyzUEAaOLwV7bQ1lIXy6\n\tQiD3ED8xL5YqvLH6k6SwC8gR2pX1hNAikfKf+6Q1kVueHAn7V9B6jUv+fR/EcN1gO8X0\n\tFDz6zgWMm6GQhGCjceBInDjm/e0alfGNU5IoqCvIFcir+3/doKrOLXB0IQtlZtTlBQvT\n\tqPhH8roDZoVLWTu0I6v/2j3DPIx8j3WFZf8zQrLgNuebyS6A5hP8N04gAp4HnnaNeY+a\n\teWKA==","X-Gm-Message-State":"AC+VfDxltH1Apzdm1F/hCNuLSyDdhvA9Lx8KIVddYWrfOEgLCzsY+KK/\n\tBc0doGmbq9QH/uFUGEazQlW0tS80zFLzPpg7YSM=","X-Google-Smtp-Source":"ACHHUZ7qZU7obeYHkAW9I5eZfPtthfinM+cDLojWzIa5WJX8AA4Wc4ct1WJuqjH32xz3q+25PtQAbQ==","X-Received":"by 2002:a05:6214:5087:b0:626:2f1b:b427 with SMTP id\n\tkk7-20020a056214508700b006262f1bb427mr8386657qvb.10.1685987669996; \n\tMon, 05 Jun 2023 10:54:29 -0700 (PDT)","Message-ID":"","To":"Laurent Pinchart ","Date":"Mon, 05 Jun 2023 13:54:28 -0400","In-Reply-To":"<20230605160146.GB7234@pendragon.ideasonboard.com>","References":"<20230605144405.118153-1-web@cedricnugteren.nl>\n\t<236fda3525cfc091bd6874fc2cfabc1d888afc7b.camel@ndufresne.ca>\n\t<20230605160146.GB7234@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","User-Agent":"Evolution 3.48.1 (3.48.1-1.fc38) ","MIME-Version":"1.0","Subject":"Re: [libcamera-devel] [PATCH v2] Add enable_auto_focus option to\n\tthe GStreamer plugin","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Nicolas Dufresne via libcamera-devel\n\t","Reply-To":"Nicolas Dufresne ","Cc":"Cedric Nugteren ,\n\tlibcamera-devel@lists.libcamera.org, \n\tCedric Nugteren ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27269,"web_url":"https://patchwork.libcamera.org/comment/27269/","msgid":"<20230605182405.GB14242@pendragon.ideasonboard.com>","date":"2023-06-05T18:24:05","subject":"Re: [libcamera-devel] [PATCH v2] Add enable_auto_focus option to\n\tthe GStreamer plugin","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nicolas,\n\nOn Mon, Jun 05, 2023 at 01:52:41PM -0400, Nicolas Dufresne wrote:\n> Le lundi 05 juin 2023 à 19:04 +0300, Laurent Pinchart a écrit\n> > > \n> [...]\n> \n> > > > Nicolas, is there any particular rule when it comes to naming properties\n> > > > for GStreamer elements ?\n> > > \n> > > The GLib part of it is:\n> > > \n> > > A property name consists of one or more segments consisting of ASCII letters and\n> > > digits, separated by either the - or _ character. The first character of a\n> > > property name must be a letter. These are the same rules as for signal naming\n> > > (see g_signal_new()).\n> > > \n> > > I strongly prefer - instead of _. Generally speaking, I would like this\n> > > discussion to happen before we start adding controls. One of the thing I was\n> > > considering was to auto-generate the propertly from the yaml,\n> > \n> > Seems like we have the same goal :-) I've just replied to your other\n> > e-mail suggesting this.\n> > \n> > Another option would be to create the mappings dynamically, based on the\n> > information reported by the camera at runtime.\n> \n> You can't add/remove/modify GObject properties at run-time. The properties are\n> considered part of the API and should be stable.\n> \n> The alternative would be to generate per camera elements, as used for CODECs,\n> but this currently only works for hardwired hardware and would not work well for\n> hot-plug capable cameras. This would need lots more infrastructure into\n> GStreamer, and for this reason, I would refrain from going that direction.\n\nOK. Then the code generation option is the best option. How should a\ncontrol that is not supported by a particular camera be handled ?\n\nI've looked at v4l2src, and as far as I can understand, there's a single\n\"extra-controls\" property that stores a key,value list. Is this a\nmechanism you would recommend ?\n\n> > > and clearly this\n> > > one is not a 1:1 mapping to the libcamera controls. But at the same time, there\n> > > is bunch of needed exception for semantic reasons. The yaml is not descriptive\n> > > enough to configure fully the properties unfortunately.\n> > \n> > We can extend the yaml file with additional information if needed,\n> > and/or extend the information reported by the camera at runtime\n> > (assuming GStreamer wouldn't have unreasonable requirements of course\n> > :-)).\n> \n> Currently known issues are arrays, which can possibly be exposed as lists\n> properties, but aren't as nice to use. Based on current usage of arrays, a\n> simple mapping like -red/-blue/-red would look nicer. To be discussed.\n> \n> Ranges are not all well exposed. I think the idea is that range are per camera,\n> but this isn't valid for GObject properties.\n\nYes, ranges are per camera, and that's not something we can change. The\nranges can even change at runtime depending on the camera configuration,\nalthough I'd like to find a better API than that.\n\nIf this doesn't fit the needs of GStreamer then we'll need to find a\ndifferent mechanism we can use on the GStreamer side.\n\n> There is probably more, but I cannot remember at the moment.\n> \n> > > As its easier to express what I'm hoping for with code, here's my own WIP, which\n> > > is much more C++ like then this one. I'm missing the AutoFocusMode.\n> > > \n> > > https://gitlab.collabora.com/nicolas/libcamera/-/commit/83d67744ccbd60ad67d2afc70be7f2d95032e4d7\n> > \n> > Nice ! Thank you for working on that. I'll try to give it a look ASAP.\n> \n> Its a pleasure. Cedric, for your interest, this does not means in anyway we'll\n> reject your changes. I think the only changes I'd suggest is to use a enum, and\n> expose auto-focus-mode with the 2 modes you currently support. This way we won't\n> break our users application as soon as we have these generalized.","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3EF41C3200\n\tfor ;\n\tMon, 5 Jun 2023 18:24:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8ED8362880;\n\tMon, 5 Jun 2023 20:24:08 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 809496287D\n\tfor ;\n\tMon, 5 Jun 2023 20:24:07 +0200 (CEST)","from pendragon.ideasonboard.com (om126156242094.26.openmobile.ne.jp\n\t[126.156.242.94])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 962B3E4;\n\tMon, 5 Jun 2023 20:23:41 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685989448;\n\tbh=rFg+mLkPHCS/UI/R51dP4pkeRtXD+j+NVTzKTNI1h6E=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=jgI1szTVhMpnABp4IYM5IW6OeMlv/n4yb2/8u8v6SXrkL0KMp/v6hPdDmRl6s7EV3\n\tTtUY/8GfCYwG05ne8N7Fi4LVF5d8/FTBBBWW8vJBahYxGIffGzlVUtmhqVatj/PuJm\n\tgdnPdvOXPUytqCLNneQmEbH+TilW8fBzGxPCH7IfzGiW596x7TWG8OihPr+6SjIoCU\n\tYmw9M0PFt7wQAfG2tO9sn6TscPdnhVoVIKAZdrsD4rSNGaWjIAm43UbRDfspMKpfx+\n\t/8ZiGBg3qFgy8r7r20oQTCD+O6rGzk+EcbPMLoO0ctS+wBTtWIvCa8YjD4Y4nrbtgp\n\t92buqeXi3rJsw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685989422;\n\tbh=rFg+mLkPHCS/UI/R51dP4pkeRtXD+j+NVTzKTNI1h6E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bZDOv8FETDk8RDlCcqUznYuWtjHaQJtFiT1O3pQ853NbceUjwnVYuO6RYDaMATVc8\n\tlwHVbXwbXaotXpOhjPfqD+ZDqse+cOpL2boS37IT0NbNf9FHoGCwATCJWiTV63slbp\n\tz/HrOgj5aJHCU0faafUGLvFTrMWFaATwWxhG5RQs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"bZDOv8FE\"; dkim-atps=neutral","Date":"Mon, 5 Jun 2023 21:24:05 +0300","To":"Nicolas Dufresne ","Message-ID":"<20230605182405.GB14242@pendragon.ideasonboard.com>","References":"<20230605144405.118153-1-web@cedricnugteren.nl>\n\t<20230605153331.GA7234@pendragon.ideasonboard.com>\n\t\n\t<20230605160455.GC7234@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2] Add enable_auto_focus option to\n\tthe GStreamer plugin","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"Cedric Nugteren ,\n\tlibcamera-devel@lists.libcamera.org, \n\tCedric Nugteren ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27270,"web_url":"https://patchwork.libcamera.org/comment/27270/","msgid":"<20230605182629.GC14242@pendragon.ideasonboard.com>","date":"2023-06-05T18:26:29","subject":"Re: [libcamera-devel] [PATCH v2] Add enable_auto_focus option to\n\tthe GStreamer plugin","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nicolas,\n\nOn Mon, Jun 05, 2023 at 01:54:28PM -0400, Nicolas Dufresne wrote:\n> Le lundi 05 juin 2023 à 19:01 +0300, Laurent Pinchart a écrit :\n> > On Mon, Jun 05, 2023 at 11:47:51AM -0400, Nicolas Dufresne via libcamera-devel wrote:\n> > > Le lundi 05 juin 2023 à 16:44 +0200, Cedric Nugteren via libcamera-devel a écrit :\n> > > > Cameras such as the PiCam 3 support auto-focus, but the GStreamer plugin\n> > > > for libcamera does not enable auto-focus. With this patch auto-focus can\n> > > > be enabled for cameras that support it. By default it is disabled, which\n> > > > means default behaviour remains unchanged. For cameras that do not\n> > > > support auto-focus, an error message shows up if it auto-focus is\n> > > > enabled.\n> > > > \n> > > > This was tested on cameras that do not support auto-focus (e.g. PiCam2)\n> > > > and was tested on a camera that does support auto-focus (PiCam3) by\n> > > > enabling it, observing auto-focus, and by disabling it or by not setting\n> > > > the option, both resulting in auto-focus being disabled.\n> > > > \n> > > > This should close https://bugs.libcamera.org/show_bug.cgi?id=188.\n> > > > \n> > > > Signed-off-by: Cedric Nugteren \n> > > > ---\n> > > > Changes since v1:\n> > > > - Now re-using the GParamSpec variables instead of re-creating.\n> > > > - Use bool instead of gboolean for enable_auto_focus.\n> > > > - Remove changes from the reconfigure_element functions.\n> > > > ---\n> > > > src/gstreamer/gstlibcameraprovider.cpp | 12 +++++++++++\n> > > > src/gstreamer/gstlibcamerasrc.cpp | 29 +++++++++++++++++++++++++-\n> > > > 2 files changed, 40 insertions(+), 1 deletion(-)\n> > > > \n> > > > diff --git a/src/gstreamer/gstlibcameraprovider.cpp b/src/gstreamer/gstlibcameraprovider.cpp\n> > > > index 6eb0a0eb..579cb8c0 100644\n> > > > --- a/src/gstreamer/gstlibcameraprovider.cpp\n> > > > +++ b/src/gstreamer/gstlibcameraprovider.cpp\n> > > > @@ -31,6 +31,7 @@ GST_DEBUG_CATEGORY_STATIC(provider_debug);\n> > > > \n> > > > enum {\n> > > > \tPROP_DEVICE_NAME = 1,\n> > > > +\tPROP_ENABLE_AUTO_FOCUS = 2,\n> > > > };\n> > > > \n> > > > #define GST_TYPE_LIBCAMERA_DEVICE gst_libcamera_device_get_type()\n> > > > @@ -40,6 +41,7 @@ G_DECLARE_FINAL_TYPE(GstLibcameraDevice, gst_libcamera_device,\n> > > > struct _GstLibcameraDevice {\n> > > > \tGstDevice parent;\n> > > > \tgchar *name;\n> > > > +\tbool enable_auto_focus = false;\n> > > > };\n> > > > \n> > > > G_DEFINE_TYPE(GstLibcameraDevice, gst_libcamera_device, GST_TYPE_DEVICE)\n> > > > @@ -56,6 +58,7 @@ gst_libcamera_device_create_element(GstDevice *device, const gchar *name)\n> > > > \tg_assert(source);\n> > > > \n> > > > \tg_object_set(source, \"camera-name\", GST_LIBCAMERA_DEVICE(device)->name, nullptr);\n> > > > +\tg_object_set(source, \"enable-auto-focus\", GST_LIBCAMERA_DEVICE(device)->enable_auto_focus, nullptr);\n> > > > \n> > > > \treturn source;\n> > > > }\n> > > > @@ -82,6 +85,9 @@ gst_libcamera_device_set_property(GObject *object, guint prop_id,\n> > > > \tcase PROP_DEVICE_NAME:\n> > > > \t\tdevice->name = g_value_dup_string(value);\n> > > > \t\tbreak;\n> > > > +\tcase PROP_ENABLE_AUTO_FOCUS:\n> > > > +\t\tdevice->enable_auto_focus = g_value_get_boolean(value);\n> > > > +\t\tbreak;\n> > > > \tdefault:\n> > > > \t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > > > \t\tbreak;\n> > > > @@ -121,6 +127,12 @@ gst_libcamera_device_class_init(GstLibcameraDeviceClass *klass)\n> > > > \t\t\t\t\t\t(GParamFlags)(G_PARAM_STATIC_STRINGS | G_PARAM_WRITABLE |\n> > > > \t\t\t\t\t\t\t G_PARAM_CONSTRUCT_ONLY));\n> > > > \tg_object_class_install_property(object_class, PROP_DEVICE_NAME, pspec);\n> > > > +\tpspec = g_param_spec_boolean(\"enable-auto-focus\",\n> > > > +\t \"Enable auto-focus\",\n> > > > +\t \"Enable auto-focus if set to true, \"\n> > > > +\t \"disable it if set to false\",\n> > > > +\t FALSE, G_PARAM_WRITABLE);\n> > > > +\tg_object_class_install_property(object_class, PROP_ENABLE_AUTO_FOCUS, pspec);\n> > > > }\n> > > > \n> > > > static GstDevice *\n> > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > > > index a10cbd4f..b2d8456b 100644\n> > > > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > > > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > > > @@ -146,6 +146,7 @@ struct _GstLibcameraSrc {\n> > > > \tGstTask *task;\n> > > > \n> > > > \tgchar *camera_name;\n> > > > +\tbool enable_auto_focus = false;\n> > > > \n> > > > \tGstLibcameraSrcState *state;\n> > > > \tGstLibcameraAllocator *allocator;\n> > > > @@ -154,7 +155,8 @@ struct _GstLibcameraSrc {\n> > > > \n> > > > enum {\n> > > > \tPROP_0,\n> > > > -\tPROP_CAMERA_NAME\n> > > > +\tPROP_CAMERA_NAME,\n> > > > +\tPROP_ENABLE_AUTO_FOCUS,\n> > > > };\n> > > > \n> > > > G_DEFINE_TYPE_WITH_CODE(GstLibcameraSrc, gst_libcamera_src, GST_TYPE_ELEMENT,\n> > > > @@ -577,6 +579,18 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> > > > \t\tgst_flow_combiner_add_pad(self->flow_combiner, srcpad);\n> > > > \t}\n> > > > \n> > > > +\tif (self->enable_auto_focus) {\n> > > > +\t\tconst ControlInfoMap &infoMap = state->cam_->controls();\n> > > > +\t\tif (infoMap.find(&controls::AfMode) != infoMap.end()) {\n> > > > +\t\t\tstate->initControls_.set(controls::AfMode, controls::AfModeContinuous);\n> > > \n> > > I don't really like the idea of mapping modes to a boolean here. I think this\n> > > should be an GEnum type of property. Some mode requires some actions to actually\n> > > work, best to just skip these for now.\n> > \n> > In the long run, we need to map all libcamera controls to GStreamer\n> > properties. Given the amount of controls, I'm hoping we will be able to\n> > either do so in a dynamic way (similar to what v4l2src does when mapping\n> > V4L2 controls), or with code generation. That's why I'm not sure if we\n> > need to spend more time now on mapping better this particular control\n> > manually (as long as we don't need to preserve backward compatibility).\n> > I trust your judgement on this topic.\n> \n> Properties are part of the API, so renaming them changing their type will break\n> applications using it. I'd strongly suggest to make this an enum (this is easy)\n> with the two supporting values. Adding more enums values later is not a break.\n\nI'm fine with that, but note that we will have API breakages, as the\nlibcamera controls are not stable yet and will be changed.\n\n> > > > +\t\t} else {\n> > > > +\t\t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > > > +\t\t\t\t(\"Failed to enable auto focus\"),\n> > > > +\t\t\t\t(\"AfMode not found in camera controls, \"\n> > > > +\t\t\t\t\"please retry with 'enable-auto-focus=false'\"));\n> > > > +\t\t}\n> > > > +\t}\n> > > > +\n> > > > \tret = state->cam_->start(&state->initControls_);\n> > > > \tif (ret) {\n> > > > \t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > > > @@ -659,6 +673,9 @@ gst_libcamera_src_set_property(GObject *object, guint prop_id,\n> > > > \t\tg_free(self->camera_name);\n> > > > \t\tself->camera_name = g_value_dup_string(value);\n> > > > \t\tbreak;\n> > > > +\tcase PROP_ENABLE_AUTO_FOCUS:\n> > > > +\t\tself->enable_auto_focus = g_value_get_boolean(value);\n> > > > +\t\tbreak;\n> > > > \tdefault:\n> > > > \t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > > > \t\tbreak;\n> > > > @@ -676,6 +693,9 @@ gst_libcamera_src_get_property(GObject *object, guint prop_id, GValue *value,\n> > > > \tcase PROP_CAMERA_NAME:\n> > > > \t\tg_value_set_string(value, self->camera_name);\n> > > > \t\tbreak;\n> > > > +\tcase PROP_ENABLE_AUTO_FOCUS:\n> > > > +\t\tg_value_set_boolean(value, self->enable_auto_focus);\n> > > > +\t\tbreak;\n> > > > \tdefault:\n> > > > \t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > > > \t\tbreak;\n> > > > @@ -844,4 +864,11 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n> > > > \t\t\t\t\t\t\t | G_PARAM_READWRITE\n> > > > \t\t\t\t\t\t\t | G_PARAM_STATIC_STRINGS));\n> > > > \tg_object_class_install_property(object_class, PROP_CAMERA_NAME, spec);\n> > > > +\tspec = g_param_spec_boolean(\"enable-auto-focus\",\n> > > > +\t \"Enable auto-focus\",\n> > > > +\t \"Enable auto-focus if set to true, \"\n> > > > +\t \"disable it if set to false\",\n> > > > +\t FALSE, G_PARAM_WRITABLE);\n> > > > +\tg_object_class_install_property(object_class, PROP_ENABLE_AUTO_FOCUS, spec);\n> > > > +\n> > > > }","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BDCE9C3200\n\tfor ;\n\tMon, 5 Jun 2023 18:26:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2A2D362886;\n\tMon, 5 Jun 2023 20:26:32 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 29E1362880\n\tfor ;\n\tMon, 5 Jun 2023 20:26:31 +0200 (CEST)","from pendragon.ideasonboard.com (om126156242094.26.openmobile.ne.jp\n\t[126.156.242.94])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4376BE4;\n\tMon, 5 Jun 2023 20:26:04 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685989592;\n\tbh=8tE66l08RPtuHRk1r9uHL9gmDkp8neN03jLjfbGqcg8=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=qLWog6hpooEVsxdYnDXKeawuNYLL+DSUHLmtu4z/EL07d+qD/P0oYe8PD0DaFCOBC\n\tw1/00ekTO4I+eSQ7zsyTo6pYZhVBSocmfAQ+7s5cRbzWJd2RLL0DOyNwsS5J/laaE8\n\tqxrf/H2wNu/d7JgadYuEScee2S9Z7+XDAoW7A/qKDn0antw55KeYOUtKtXHA1+EQNZ\n\tjKJYt8lYcGxJ6pIQI4loX2neHNxwaCXU3ZFhuD2A8ou70J3IsBE3QVl+diHfv7VvG5\n\thlo66iSkSU7RaqLNYvVae2/pxnj5SeMikC4QMLxyMlo17xyMbkVlgqot4ne+p5F1QL\n\tRZEABKdQegD3Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685989566;\n\tbh=8tE66l08RPtuHRk1r9uHL9gmDkp8neN03jLjfbGqcg8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=B6DGnKgmMaZi7rGou1WGZ3hXFMJCv838qtcedOUW5Pvwx/hfDh4MrKbgD5hdeMO0E\n\t7uW7idTB+l4NT5IhQgxV8sJmJOVlSqHuIMe6crHqVi4JYrOK+5eCXwRMhNWIKueS03\n\trLl8y4kYljLbxpYQlLwSY15Taf6Zyc7WBGfQt0R4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"B6DGnKgm\"; dkim-atps=neutral","Date":"Mon, 5 Jun 2023 21:26:29 +0300","To":"Nicolas Dufresne ","Message-ID":"<20230605182629.GC14242@pendragon.ideasonboard.com>","References":"<20230605144405.118153-1-web@cedricnugteren.nl>\n\t<236fda3525cfc091bd6874fc2cfabc1d888afc7b.camel@ndufresne.ca>\n\t<20230605160146.GB7234@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2] Add enable_auto_focus option to\n\tthe GStreamer plugin","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"Cedric Nugteren ,\n\tlibcamera-devel@lists.libcamera.org, \n\tCedric Nugteren ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]