[{"id":27165,"web_url":"https://patchwork.libcamera.org/comment/27165/","msgid":"","date":"2023-05-29T18:09:39","subject":"Re: [libcamera-devel] [PATCH] Add enable_auto_focus option to the\n\tGStreamer plugin","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Hi,\n\nLe vendredi 26 mai 2023 à 14:34 +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\nI'm not sure this default behaviour make sense. I would rather have it enabled\nby default, since there is not other offered mean at the moment to obtain this\nfocus.\n\nThat being said, I haven't looked at the auto-focus feature, I was expecting\nthere would be different type of it, which would endup with different default\nbehaviour.\n\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> Signed-off-by: Cedric Nugteren \n> ---\n>  src/gstreamer/gstlibcameraprovider.cpp | 13 ++++++++++++\n>  src/gstreamer/gstlibcamerasrc.cpp      | 29 +++++++++++++++++++++++++-\n>  2 files changed, 41 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/gstreamer/gstlibcameraprovider.cpp\n> b/src/gstreamer/gstlibcameraprovider.cpp\n> index 6eb0a0eb..86fa2542 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>   PROP_DEVICE_NAME = 1,\n> + PROP_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,\n> gst_libcamera_device,\n>  struct _GstLibcameraDevice {\n>   GstDevice parent;\n>   gchar *name;\n> + gboolean 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\n> gchar *name)\n>   g_assert(source);\n>  \n>   g_object_set(source, \"camera-name\", GST_LIBCAMERA_DEVICE(device)->name,\n> nullptr);\n> + g_object_set(source, \"enable-auto-focus\", GST_LIBCAMERA_DEVICE(device)-\n> >enable_auto_focus, nullptr);\n>  \n>   return source;\n>  }\n> @@ -68,6 +71,7 @@ gst_libcamera_device_reconfigure_element(GstDevice *device,\n>   return FALSE;\n>  \n>   g_object_set(element, \"camera-name\", GST_LIBCAMERA_DEVICE(device)->name,\n> nullptr);\n> + g_object_set(element, \"enable-auto-focus\", GST_LIBCAMERA_DEVICE(device)-\n> >enable_auto_focus, nullptr);\n\nThat is strange, unlike camera-name, this information is not discovered at run-\ntime, hence does not require a notify callback. I'd simply set the default at\nconstruction time, and avoid this here.\n\n>  \n>   return TRUE;\n>  }\n> @@ -82,6 +86,9 @@ gst_libcamera_device_set_property(GObject *object, guint\n> prop_id,\n>   case PROP_DEVICE_NAME:\n>   device->name = g_value_dup_string(value);\n>   break;\n> + case PROP_ENABLE_AUTO_FOCUS:\n> + device->enable_auto_focus = g_value_get_boolean(value);\n> + break;\n>   default:\n>   G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n>   break;\n> @@ -121,6 +128,12 @@ gst_libcamera_device_class_init(GstLibcameraDeviceClass\n> *klass)\n>   (GParamFlags)(G_PARAM_STATIC_STRINGS | G_PARAM_WRITABLE |\n>        G_PARAM_CONSTRUCT_ONLY));\n>   g_object_class_install_property(object_class, PROP_DEVICE_NAME, pspec);\n> + GParamSpec *spec2 = g_param_spec_boolean(\"enable-auto-focus\",\n\nWhy a new one ? just reuse spec here, its just temporary pointer with no\nownership in it. It was only added to be able to fit into the maximum line\nlength.\n\n> +                        \"Enable auto-focus\",\n> +                        \"Enable auto-focus if set to true, \"\n> +                        \"disable it if set to false\",\n> +                         FALSE, G_PARAM_WRITABLE);\n> + g_object_class_install_property(object_class, PROP_ENABLE_AUTO_FOCUS,\n> spec2);\n\nNote, if you feel like it, you could make it more obvious by using\ng_steal_pointer() here, which is similar to std::move (but works for non C++\npointers).\n\n>  }\n>  \n>  static GstDevice *\n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp\n> b/src/gstreamer/gstlibcamerasrc.cpp\n> index a10cbd4f..672ea38a 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -146,6 +146,7 @@ struct _GstLibcameraSrc {\n>   GstTask *task;\n>  \n>   gchar *camera_name;\n> + gboolean enable_auto_focus = false;\n\nI guess I've been thinking C a lot so far, thanks for making me noticed we can\ndo that. Note a little inconstancy, gboolean is an in. So I'd suggest:\n\n+ bool enable_auto_focus = false;\n\n>  \n>   GstLibcameraSrcState *state;\n>   GstLibcameraAllocator *allocator;\n> @@ -154,7 +155,8 @@ struct _GstLibcameraSrc {\n>  \n>  enum {\n>   PROP_0,\n> - PROP_CAMERA_NAME\n> + PROP_CAMERA_NAME,\n> + PROP_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,\n> [[maybe_unused]] GThread *thread,\n>   gst_flow_combiner_add_pad(self->flow_combiner, srcpad);\n>   }\n>  \n> + if (self->enable_auto_focus) {\n> + const ControlInfoMap &infoMap = state->cam_->controls();\n> + if (infoMap.find(&controls::AfMode) != infoMap.end()) {\n> + state->initControls_.set(controls::AfMode, controls::AfModeContinuous);\n> + } else {\n> + GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> + (\"Failed to enable auto focus\"),\n> + (\"AfMode not found in camera controls, \"\n> + \"please retry with 'enable-auto-focus=false'\"));\n> + }\n> + }\n> +\n\nPlease resend in text form as you are suppose to (I recommend using git send-\nemail). This will ensure we don't smash the indentation completely. I'll stop my\nreview here, as its too hard to read without indentation.\n\nregards,\nNicolas\n\n>   ret = state->cam_->start(&state->initControls_);\n>   if (ret) {\n>   GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> @@ -659,6 +673,9 @@ gst_libcamera_src_set_property(GObject *object, guint\n> prop_id,\n>   g_free(self->camera_name);\n>   self->camera_name = g_value_dup_string(value);\n>   break;\n> + case PROP_ENABLE_AUTO_FOCUS:\n> + self->enable_auto_focus = g_value_get_boolean(value);\n> + break;\n>   default:\n>   G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n>   break;\n> @@ -676,6 +693,9 @@ gst_libcamera_src_get_property(GObject *object, guint\n> prop_id, GValue *value,\n>   case PROP_CAMERA_NAME:\n>   g_value_set_string(value, self->camera_name);\n>   break;\n> + case PROP_ENABLE_AUTO_FOCUS:\n> + g_value_set_boolean(value, self->enable_auto_focus);\n> + break;\n>   default:\n>   G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n>   break;\n> @@ -844,4 +864,11 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n>       | G_PARAM_READWRITE\n>       | G_PARAM_STATIC_STRINGS));\n>   g_object_class_install_property(object_class, PROP_CAMERA_NAME, spec);\n> + GParamSpec *spec2 = g_param_spec_boolean(\"enable-auto-focus\",\n> +                        \"Enable auto-focus\",\n> +                        \"Enable auto-focus if set to true, \"\n> +                        \"disable it if set to false\",\n> +                         FALSE, G_PARAM_WRITABLE);\n> + g_object_class_install_property(object_class, PROP_ENABLE_AUTO_FOCUS,\n> spec2);\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 B839BC3213\n\tfor ;\n\tMon, 29 May 2023 18:09:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 20464626F9;\n\tMon, 29 May 2023 20:09:44 +0200 (CEST)","from mail-qt1-x82c.google.com (mail-qt1-x82c.google.com\n\t[IPv6:2607:f8b0:4864:20::82c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2E87D6038E\n\tfor ;\n\tMon, 29 May 2023 20:09:42 +0200 (CEST)","by mail-qt1-x82c.google.com with SMTP id\n\td75a77b69052e-3f6b24467deso17763991cf.1\n\tfor ;\n\tMon, 29 May 2023 11:09:42 -0700 (PDT)","from nicolas-tpx395.localdomain ([2606:6d00:17:6c0::7a9])\n\tby smtp.gmail.com with ESMTPSA id\n\tq14-20020ac8450e000000b003e4f1b3ce43sm4018377qtn.50.2023.05.29.11.09.40\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 29 May 2023 11:09:40 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685383784;\n\tbh=mt4gcSklPbhD7LJgaWR/6jfJp3toROdLleHo2IdkhPk=;\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:\n\tFrom;\n\tb=M6+YNZVdPCjjW7tSIh7QD87BXHzWTs0exp0vjAwUxCpw0yAN1o/H+eWyDLxRIQE5j\n\txqOK4LYEel4hzhXVykUsbxRWooeelmg0pThoKG3bQaqYDkRG9MEUokxr5e1CUUqhO3\n\tVYD2XJ8YarXt3Jc0YfpCDG1i6DMM/9uDjFmJ4A+FWsbouKwhlrlf6ybhTweaIZq16E\n\tPZqKdED+4coJg+J7Dy3diVmGuT34d5tsECSAIjvcpKc+JeLpgA9gEBinVS51JfQNZk\n\tIUDTQ34rX/d82C5nl0XXDezCqQ3Gowbf8TA5IAScpV5b5h4c9c433gUZGh1Wh/vBmj\n\tAQDlC++SElP/w==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20221208.gappssmtp.com; s=20221208; t=1685383781;\n\tx=1687975781; \n\th=mime-version:user-agent:content-transfer-encoding:references\n\t:in-reply-to:date:to:from:subject:message-id:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=mt4gcSklPbhD7LJgaWR/6jfJp3toROdLleHo2IdkhPk=;\n\tb=Gv29jD8DnxTeubPFnEO3eR9wSyGg6F4iTJub6c9V/bXVvYfCKBy1mSDaqWx/oV5qkv\n\tXanpc9DfZkwiudsVePqpX3Og3JhA4uUMinz9n7F+GKd5+5HUmsOPJVK9Xxc8YajR4qUl\n\thB0HPsLEi3IBGJ6IqK9EMxE6YH2oidBLzdOLO3dQI9E7leWUcYch//kVaSeh96Pfkis7\n\tLXqai8HeX9YDEM1lpp96HLHA71f120oAgQ8St4e2ZDKybliOBXz+s0iVTAqLYq5k5bDN\n\toJWJZZPna/tCgb/awQStBx/5VP0g2fw0rT1Vc+Zh5wMMBvCRX5Yc3mNdU2wyWcmLsiyk\n\tg7eQ=="],"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=\"Gv29jD8D\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1685383781; x=1687975781;\n\th=mime-version:user-agent:content-transfer-encoding:references\n\t:in-reply-to:date:to:from:subject:message-id:x-gm-message-state:from\n\t:to:cc:subject:date:message-id:reply-to;\n\tbh=mt4gcSklPbhD7LJgaWR/6jfJp3toROdLleHo2IdkhPk=;\n\tb=Bn2q31LOqsqZKO/+Ca9R9E/KHT1QZkTJ0UDxZxd6J0lYr4WQEP3A044WBCr8UwV5ss\n\t0bCi7yJBcXaB30lWgXLN3cDSt7CIAJXZ3lav4DR9Lep4e9lYbRltPQQTjB2ZLuA0cKPX\n\t8EY7yZfyyKiEWC3I+ycSKf0UKccfMq/HfNHxIBZiKywUGWIyq+jf8BqQV1LePGy3Okoj\n\tN/abztSVLgWtkogtBwYR1lWYUMzYHIBPnC5Xc62zZLYHSWEM56zN1IbDpRxniVKXzYz1\n\tNotQRO+EI4rVxU9/ZvISE8AACbjfpAcqftqWXOBhfssoa1hRkilrSKfAKgRKKGohyCU0\n\t5qYQ==","X-Gm-Message-State":"AC+VfDxTk7ZdecDYsuZPPEhRqgupUJglz8i2hOxfzP0OpH8NtoB+Z/OC\n\tGjyCPPvuIHyQECk3JxyeJGWAgFtRr5msaXqIwU4=","X-Google-Smtp-Source":"ACHHUZ6Ffg76ImoRjb4AVujPlkWwBGXvCTsl6Apon1IFyYtvDaiNo8tSTwagfvx+Q4rdbfFmVvRosg==","X-Received":"by 2002:a05:622a:18d:b0:3f6:c146:a1a8 with SMTP id\n\ts13-20020a05622a018d00b003f6c146a1a8mr9757801qtw.43.1685383780853; \n\tMon, 29 May 2023 11:09:40 -0700 (PDT)","Message-ID":"","To":"Cedric Nugteren ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 29 May 2023 14:09:39 -0400","In-Reply-To":"","References":"","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] Add enable_auto_focus option to the\n\tGStreamer 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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27191,"web_url":"https://patchwork.libcamera.org/comment/27191/","msgid":"<20230531150629.GC24749@pendragon.ideasonboard.com>","date":"2023-05-31T15:06:29","subject":"Re: [libcamera-devel] [PATCH] Add enable_auto_focus option to the\n\tGStreamer plugin","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\n(CC'ing Naush and David)\n\nOn Mon, May 29, 2023 at 02:09:39PM -0400, Nicolas Dufresne via libcamera-devel wrote:\n> Le vendredi 26 mai 2023 à 14:34 +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> I'm not sure this default behaviour make sense. I would rather have it enabled\n> by default, since there is not other offered mean at the moment to obtain this\n> focus.\n> \n> That being said, I haven't looked at the auto-focus feature, I was expecting\n> there would be different type of it, which would endup with different default\n> behaviour.\n\nI think enabling it by default makes sense, but that leads me to another\nquestion: why is auto-focus not enabled by defaut (for cameras that\nsupport it) in the Raspberry Pi IPA module ?\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> > Signed-off-by: Cedric Nugteren \n> > ---\n> >  src/gstreamer/gstlibcameraprovider.cpp | 13 ++++++++++++\n> >  src/gstreamer/gstlibcamerasrc.cpp      | 29 +++++++++++++++++++++++++-\n> >  2 files changed, 41 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/src/gstreamer/gstlibcameraprovider.cpp\n> > b/src/gstreamer/gstlibcameraprovider.cpp\n> > index 6eb0a0eb..86fa2542 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> >   PROP_DEVICE_NAME = 1,\n> > + PROP_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,\n> > gst_libcamera_device,\n> >  struct _GstLibcameraDevice {\n> >   GstDevice parent;\n> >   gchar *name;\n> > + gboolean 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\n> > gchar *name)\n> >   g_assert(source);\n> >  \n> >   g_object_set(source, \"camera-name\", GST_LIBCAMERA_DEVICE(device)->name,\n> > nullptr);\n> > + g_object_set(source, \"enable-auto-focus\", GST_LIBCAMERA_DEVICE(device)-\n> > >enable_auto_focus, nullptr);\n> >  \n> >   return source;\n> >  }\n> > @@ -68,6 +71,7 @@ gst_libcamera_device_reconfigure_element(GstDevice *device,\n> >   return FALSE;\n> >  \n> >   g_object_set(element, \"camera-name\", GST_LIBCAMERA_DEVICE(device)->name,\n> > nullptr);\n> > + g_object_set(element, \"enable-auto-focus\", GST_LIBCAMERA_DEVICE(device)-\n> > >enable_auto_focus, nullptr);\n> \n> That is strange, unlike camera-name, this information is not discovered at run-\n> time, hence does not require a notify callback. I'd simply set the default at\n> construction time, and avoid this here.\n> \n> >   return TRUE;\n> >  }\n> > @@ -82,6 +86,9 @@ gst_libcamera_device_set_property(GObject *object, guint\n> > prop_id,\n> >   case PROP_DEVICE_NAME:\n> >   device->name = g_value_dup_string(value);\n> >   break;\n> > + case PROP_ENABLE_AUTO_FOCUS:\n> > + device->enable_auto_focus = g_value_get_boolean(value);\n> > + break;\n> >   default:\n> >   G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> >   break;\n> > @@ -121,6 +128,12 @@ gst_libcamera_device_class_init(GstLibcameraDeviceClass\n> > *klass)\n> >   (GParamFlags)(G_PARAM_STATIC_STRINGS | G_PARAM_WRITABLE |\n> >        G_PARAM_CONSTRUCT_ONLY));\n> >   g_object_class_install_property(object_class, PROP_DEVICE_NAME, pspec);\n> > + GParamSpec *spec2 = g_param_spec_boolean(\"enable-auto-focus\",\n> \n> Why a new one ? just reuse spec here, its just temporary pointer with no\n> ownership in it. It was only added to be able to fit into the maximum line\n> length.\n> \n> > +                        \"Enable auto-focus\",\n> > +                        \"Enable auto-focus if set to true, \"\n> > +                        \"disable it if set to false\",\n> > +                         FALSE, G_PARAM_WRITABLE);\n> > + g_object_class_install_property(object_class, PROP_ENABLE_AUTO_FOCUS, spec2);\n> \n> Note, if you feel like it, you could make it more obvious by using\n> g_steal_pointer() here, which is similar to std::move (but works for non C++\n> pointers).\n> \n> >  }\n> >  \n> >  static GstDevice *\n> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp\n> > b/src/gstreamer/gstlibcamerasrc.cpp\n> > index a10cbd4f..672ea38a 100644\n> > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > @@ -146,6 +146,7 @@ struct _GstLibcameraSrc {\n> >   GstTask *task;\n> >  \n> >   gchar *camera_name;\n> > + gboolean enable_auto_focus = false;\n> \n> I guess I've been thinking C a lot so far, thanks for making me noticed we can\n> do that. Note a little inconstancy, gboolean is an in. So I'd suggest:\n> \n> + bool enable_auto_focus = false;\n> \n> >   GstLibcameraSrcState *state;\n> >   GstLibcameraAllocator *allocator;\n> > @@ -154,7 +155,8 @@ struct _GstLibcameraSrc {\n> >  \n> >  enum {\n> >   PROP_0,\n> > - PROP_CAMERA_NAME\n> > + PROP_CAMERA_NAME,\n> > + PROP_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,\n> > [[maybe_unused]] GThread *thread,\n> >   gst_flow_combiner_add_pad(self->flow_combiner, srcpad);\n> >   }\n> >  \n> > + if (self->enable_auto_focus) {\n> > + const ControlInfoMap &infoMap = state->cam_->controls();\n> > + if (infoMap.find(&controls::AfMode) != infoMap.end()) {\n> > + state->initControls_.set(controls::AfMode, controls::AfModeContinuous);\n> > + } else {\n> > + GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > + (\"Failed to enable auto focus\"),\n> > + (\"AfMode not found in camera controls, \"\n> > + \"please retry with 'enable-auto-focus=false'\"));\n> > + }\n> > + }\n> > +\n> \n> Please resend in text form as you are suppose to (I recommend using git send-\n> email). This will ensure we don't smash the indentation completely. I'll stop my\n> review here, as its too hard to read without indentation.\n\nI strongly recommend using git-send-email to send patches as well.\nInstructions are available at\nhttps://libcamera.org/contributing.html#submitting-patches, which links\nto the very nice https://git-send-email.io/ helper to setup\ngit-send-email.\n\n> >   ret = state->cam_->start(&state->initControls_);\n> >   if (ret) {\n> >   GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > @@ -659,6 +673,9 @@ gst_libcamera_src_set_property(GObject *object, guint\n> > prop_id,\n> >   g_free(self->camera_name);\n> >   self->camera_name = g_value_dup_string(value);\n> >   break;\n> > + case PROP_ENABLE_AUTO_FOCUS:\n> > + self->enable_auto_focus = g_value_get_boolean(value);\n> > + break;\n> >   default:\n> >   G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> >   break;\n> > @@ -676,6 +693,9 @@ gst_libcamera_src_get_property(GObject *object, guint\n> > prop_id, GValue *value,\n> >   case PROP_CAMERA_NAME:\n> >   g_value_set_string(value, self->camera_name);\n> >   break;\n> > + case PROP_ENABLE_AUTO_FOCUS:\n> > + g_value_set_boolean(value, self->enable_auto_focus);\n> > + break;\n> >   default:\n> >   G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> >   break;\n> > @@ -844,4 +864,11 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n> >       | G_PARAM_READWRITE\n> >       | G_PARAM_STATIC_STRINGS));\n> >   g_object_class_install_property(object_class, PROP_CAMERA_NAME, spec);\n> > + GParamSpec *spec2 = g_param_spec_boolean(\"enable-auto-focus\",\n> > +                        \"Enable auto-focus\",\n> > +                        \"Enable auto-focus if set to true, \"\n> > +                        \"disable it if set to false\",\n> > +                         FALSE, G_PARAM_WRITABLE);\n> > + g_object_class_install_property(object_class, PROP_ENABLE_AUTO_FOCUS, spec2);\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 B5A62C31E9\n\tfor ;\n\tWed, 31 May 2023 15:06:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 200A5626F8;\n\tWed, 31 May 2023 17:06:32 +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 E4E5F626F8\n\tfor ;\n\tWed, 31 May 2023 17:06:30 +0200 (CEST)","from pendragon.ideasonboard.com (om126205251136.34.openmobile.ne.jp\n\t[126.205.251.136])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 707F6844;\n\tWed, 31 May 2023 17:06:08 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685545592;\n\tbh=etRGTP2VQ8ymTPOYJjV9OWURG0ABfiBnWy8TnTy8ZpI=;\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=yKcX09g2AHu7hKP2eHEID68mTttBVnkEALq8Pjo9lIE0DbwuaoFGiiRkaigYDMquQ\n\tIkCS1FErcQlGR7l4bqZBhz8b9tqoTJ67Ye0UEjT7X3VhLaSqx29Lyml+0DgY4rv8Rc\n\tdrfI9lnTJ/rHfrdcubTP1Z/3UBBDycvb/DTZr4Ub0cOa00zYlLNHEA6rckdszUGlVA\n\t1+8+jI7uWei0qlTsACXYWIiH+9I0g4vatUFwcetpzNAA/dKyaqq6HNBfoVSkvCpxWM\n\tzfoV2oZ9iL2GAOGIJVHKytVIk44TNExoc+Zj4/gNAGboomW3jjponXwb1pgD1rxVS9\n\tDP750SxIukQDA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685545569;\n\tbh=etRGTP2VQ8ymTPOYJjV9OWURG0ABfiBnWy8TnTy8ZpI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=v0o7J99nEJcyFAmOpw9gRtqP5QPdJ03thL8PNnefDqlp9QG8XEeKT0loMyocorQW1\n\tF43JMHH9+s4u3uSBu9qsEE5mt4FWYJUtbCYe57qqCt4YN2J6gO3MjRkKfBlPkEoOvF\n\trpDA7nQOZuECLB+zWwSMcO2NJl3Aijzwb9/r0o/Y="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"v0o7J99n\"; dkim-atps=neutral","Date":"Wed, 31 May 2023 18:06:29 +0300","To":"Nicolas Dufresne ","Message-ID":"<20230531150629.GC24749@pendragon.ideasonboard.com>","References":"\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] Add enable_auto_focus option to the\n\tGStreamer 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27193,"web_url":"https://patchwork.libcamera.org/comment/27193/","msgid":"","date":"2023-05-31T15:16:28","subject":"Re: [libcamera-devel] [PATCH] Add enable_auto_focus option to the\n\tGStreamer plugin","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi all,\n\nOn Wed, 31 May 2023 at 16:06, Laurent Pinchart\n wrote:\n>\n> Hello,\n>\n> (CC'ing Naush and David)\n>\n> On Mon, May 29, 2023 at 02:09:39PM -0400, Nicolas Dufresne via libcamera-devel wrote:\n> > Le vendredi 26 mai 2023 à 14:34 +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> > I'm not sure this default behaviour make sense. I would rather have it enabled\n> > by default, since there is not other offered mean at the moment to obtain this\n> > focus.\n> >\n> > That being said, I haven't looked at the auto-focus feature, I was expecting\n> > there would be different type of it, which would endup with different default\n> > behaviour.\n>\n> I think enabling it by default makes sense, but that leads me to another\n> question: why is auto-focus not enabled by defaut (for cameras that\n> support it) in the Raspberry Pi IPA module ?\n\nBelieve it or not, this has been asked a few times now :-)\n\nThe RPi IPA does not want to assume or trigger any particular behaviour based on\nthe specific sensor used. We feel it's up to applications to decide this\nunconditionally. In the case of a focus lens available, the application ought to\nchoose what it wants to do, e.g. hyperfocal fixed position, AF cycle then fixed\nposition, CAF, etc. We don't really want to choose a default that may not be\nwhat is needed as this is highly use case dependent.\n\nSo we essentially do nothing and leave the lens where it last was in the absence\nof any AF control.\n\nRegards,\nNaush\n\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> > > Signed-off-by: Cedric Nugteren \n> > > ---\n> > > src/gstreamer/gstlibcameraprovider.cpp | 13 ++++++++++++\n> > > src/gstreamer/gstlibcamerasrc.cpp | 29 +++++++++++++++++++++++++-\n> > > 2 files changed, 41 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/src/gstreamer/gstlibcameraprovider.cpp\n> > > b/src/gstreamer/gstlibcameraprovider.cpp\n> > > index 6eb0a0eb..86fa2542 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> > > PROP_DEVICE_NAME = 1,\n> > > + PROP_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,\n> > > gst_libcamera_device,\n> > > struct _GstLibcameraDevice {\n> > > GstDevice parent;\n> > > gchar *name;\n> > > + gboolean 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\n> > > gchar *name)\n> > > g_assert(source);\n> > >\n> > > g_object_set(source, \"camera-name\", GST_LIBCAMERA_DEVICE(device)->name,\n> > > nullptr);\n> > > + g_object_set(source, \"enable-auto-focus\", GST_LIBCAMERA_DEVICE(device)-\n> > > >enable_auto_focus, nullptr);\n> > >\n> > > return source;\n> > > }\n> > > @@ -68,6 +71,7 @@ gst_libcamera_device_reconfigure_element(GstDevice *device,\n> > > return FALSE;\n> > >\n> > > g_object_set(element, \"camera-name\", GST_LIBCAMERA_DEVICE(device)->name,\n> > > nullptr);\n> > > + g_object_set(element, \"enable-auto-focus\", GST_LIBCAMERA_DEVICE(device)-\n> > > >enable_auto_focus, nullptr);\n> >\n> > That is strange, unlike camera-name, this information is not discovered at run-\n> > time, hence does not require a notify callback. I'd simply set the default at\n> > construction time, and avoid this here.\n> >\n> > > return TRUE;\n> > > }\n> > > @@ -82,6 +86,9 @@ gst_libcamera_device_set_property(GObject *object, guint\n> > > prop_id,\n> > > case PROP_DEVICE_NAME:\n> > > device->name = g_value_dup_string(value);\n> > > break;\n> > > + case PROP_ENABLE_AUTO_FOCUS:\n> > > + device->enable_auto_focus = g_value_get_boolean(value);\n> > > + break;\n> > > default:\n> > > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > > break;\n> > > @@ -121,6 +128,12 @@ gst_libcamera_device_class_init(GstLibcameraDeviceClass\n> > > *klass)\n> > > (GParamFlags)(G_PARAM_STATIC_STRINGS | G_PARAM_WRITABLE |\n> > > G_PARAM_CONSTRUCT_ONLY));\n> > > g_object_class_install_property(object_class, PROP_DEVICE_NAME, pspec);\n> > > + GParamSpec *spec2 = g_param_spec_boolean(\"enable-auto-focus\",\n> >\n> > Why a new one ? just reuse spec here, its just temporary pointer with no\n> > ownership in it. It was only added to be able to fit into the maximum line\n> > length.\n> >\n> > > + \"Enable auto-focus\",\n> > > + \"Enable auto-focus if set to true, \"\n> > > + \"disable it if set to false\",\n> > > + FALSE, G_PARAM_WRITABLE);\n> > > + g_object_class_install_property(object_class, PROP_ENABLE_AUTO_FOCUS, spec2);\n> >\n> > Note, if you feel like it, you could make it more obvious by using\n> > g_steal_pointer() here, which is similar to std::move (but works for non C++\n> > pointers).\n> >\n> > > }\n> > >\n> > > static GstDevice *\n> > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp\n> > > b/src/gstreamer/gstlibcamerasrc.cpp\n> > > index a10cbd4f..672ea38a 100644\n> > > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > > @@ -146,6 +146,7 @@ struct _GstLibcameraSrc {\n> > > GstTask *task;\n> > >\n> > > gchar *camera_name;\n> > > + gboolean enable_auto_focus = false;\n> >\n> > I guess I've been thinking C a lot so far, thanks for making me noticed we can\n> > do that. Note a little inconstancy, gboolean is an in. So I'd suggest:\n> >\n> > + bool enable_auto_focus = false;\n> >\n> > > GstLibcameraSrcState *state;\n> > > GstLibcameraAllocator *allocator;\n> > > @@ -154,7 +155,8 @@ struct _GstLibcameraSrc {\n> > >\n> > > enum {\n> > > PROP_0,\n> > > - PROP_CAMERA_NAME\n> > > + PROP_CAMERA_NAME,\n> > > + PROP_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,\n> > > [[maybe_unused]] GThread *thread,\n> > > gst_flow_combiner_add_pad(self->flow_combiner, srcpad);\n> > > }\n> > >\n> > > + if (self->enable_auto_focus) {\n> > > + const ControlInfoMap &infoMap = state->cam_->controls();\n> > > + if (infoMap.find(&controls::AfMode) != infoMap.end()) {\n> > > + state->initControls_.set(controls::AfMode, controls::AfModeContinuous);\n> > > + } else {\n> > > + GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > > + (\"Failed to enable auto focus\"),\n> > > + (\"AfMode not found in camera controls, \"\n> > > + \"please retry with 'enable-auto-focus=false'\"));\n> > > + }\n> > > + }\n> > > +\n> >\n> > Please resend in text form as you are suppose to (I recommend using git send-\n> > email). This will ensure we don't smash the indentation completely. I'll stop my\n> > review here, as its too hard to read without indentation.\n>\n> I strongly recommend using git-send-email to send patches as well.\n> Instructions are available at\n> https://libcamera.org/contributing.html#submitting-patches, which links\n> to the very nice https://git-send-email.io/ helper to setup\n> git-send-email.\n>\n> > > ret = state->cam_->start(&state->initControls_);\n> > > if (ret) {\n> > > GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > > @@ -659,6 +673,9 @@ gst_libcamera_src_set_property(GObject *object, guint\n> > > prop_id,\n> > > g_free(self->camera_name);\n> > > self->camera_name = g_value_dup_string(value);\n> > > break;\n> > > + case PROP_ENABLE_AUTO_FOCUS:\n> > > + self->enable_auto_focus = g_value_get_boolean(value);\n> > > + break;\n> > > default:\n> > > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > > break;\n> > > @@ -676,6 +693,9 @@ gst_libcamera_src_get_property(GObject *object, guint\n> > > prop_id, GValue *value,\n> > > case PROP_CAMERA_NAME:\n> > > g_value_set_string(value, self->camera_name);\n> > > break;\n> > > + case PROP_ENABLE_AUTO_FOCUS:\n> > > + g_value_set_boolean(value, self->enable_auto_focus);\n> > > + break;\n> > > default:\n> > > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > > break;\n> > > @@ -844,4 +864,11 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n> > > | G_PARAM_READWRITE\n> > > | G_PARAM_STATIC_STRINGS));\n> > > g_object_class_install_property(object_class, PROP_CAMERA_NAME, spec);\n> > > + GParamSpec *spec2 = g_param_spec_boolean(\"enable-auto-focus\",\n> > > + \"Enable auto-focus\",\n> > > + \"Enable auto-focus if set to true, \"\n> > > + \"disable it if set to false\",\n> > > + FALSE, G_PARAM_WRITABLE);\n> > > + g_object_class_install_property(object_class, PROP_ENABLE_AUTO_FOCUS, spec2);\n> > > +\n> > > }\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 6BF0AC31E9\n\tfor ;\n\tWed, 31 May 2023 15:16:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C6997626FE;\n\tWed, 31 May 2023 17:16:43 +0200 (CEST)","from mail-yw1-x1135.google.com (mail-yw1-x1135.google.com\n\t[IPv6:2607:f8b0:4864:20::1135])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A73FE626FA\n\tfor ;\n\tWed, 31 May 2023 17:16:42 +0200 (CEST)","by mail-yw1-x1135.google.com with SMTP id\n\t00721157ae682-565ee3d14c2so49346837b3.2\n\tfor ;\n\tWed, 31 May 2023 08:16:42 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685546203;\n\tbh=ivMsBAG9eef1rygVjK5Pexlhk2Zo8rqiD8Ume+r7vIQ=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=v7281QTWjqCLD8mB9GAXvK735dWnAqjxnymwICqT5MZhvqyuxg8CCaPWyj8B/5w2U\n\t/Th2xYRAUzN7InXaIExxTDnqZ0oFwGewMihK6mfspdNdVwnYuBlsmNOMvV7Tto+qEA\n\t5bkpAzSrONTmSZyvKfj8z6Gg+OMuum/h0XBpJTPHbsQ5kgi22gZDyFsiAAB1L6cVjM\n\thjDMG0MY2jvXDMpDEC+/YHrk5zwZCwlIb6uC7hc5EVk+6v/eNYKnHxnDIc8LSAcOWn\n\tEkgrUmxL+LF3zMByGyGjduFVWhp81WYRuu3qdyb2LbydYjkpK1KhEl9p6B3MJak+w5\n\tTm2jkJKp7Zd0A==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1685546201; x=1688138201;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=se2skGbVQB4ifJcJJD0ySTbaugjejMmBtGqUYacvYbI=;\n\tb=E5OprROKxDj9/YXOYNjYb0JkC3fIez+bxF64PXnZILbFU6vIGeFJjsXEXnYgtbHGV7\n\tABkhbFWM89m9l+a1HkdiYamDpIT6trNwnyOqNgyi5maY1ETFZqQZuPvFpy/9vWZuJmZu\n\t9PgSZexXpG79Ylo1HeyDwW/B9bq7K1XomBX0nkCzdHdklXcIuSkOinpZAzikns2LU9xv\n\t/KZnHGrvPjIGvOLVLEaN4kiJm+5AyCKJ2bZZ+Jo+DVxZxuiqgiwF09GAemTrpgVsGoPn\n\tw9BjDgxpGveSTjM7kPh4RmIwPhHuOWeq6PAIbumTNrqolmsjTBunAzolE2o/QsVbYFKM\n\tti/A=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"E5OprROK\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1685546201; x=1688138201;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=se2skGbVQB4ifJcJJD0ySTbaugjejMmBtGqUYacvYbI=;\n\tb=IbZMhklnAUd1vErA6qX4MQWFoeNxaA5iP8s1RCu4KW4VgtKEHdbjGSuhJC/uW/AVAR\n\tGynGrFJDwPOg1n1CndWy3UpTM6TVsXpkg5UoPLTniS/nszRKxioBKEJsaxTLiy1udEp6\n\tsfPUEVZb7z/73UfWf32+OQR7mdISTuSxwqyKEV6apS1RlVQDtJTpCD7qaYdUnCq+vSZQ\n\t1MZlmaBF9BQH8nQ9ppbpN9pvgsW/pSckpCcnolBhinicpZRR09CzXygQBOYdPyyJOyho\n\tuAOMIQn2N/ZpYpC6Pc+ZDpuC79lY/43RcQtoYBF4HC26mev5Tl98nbmldMRElynvpmBm\n\tq6bw==","X-Gm-Message-State":"AC+VfDxhYkK3tE3WJg0IBIvP4V8HfBw0VisTG7e+07fn+OCWfJ6ZOI7P\n\tXhN4LFKxdoa/UASAzeWrxCiQbsVA6kpUt7Lg58RyhfAAYo2/jr8i+NWnHg==","X-Google-Smtp-Source":"ACHHUZ4cnlC0toOlKQ9eP7YFpcfhF0ot7hmoTu55tpjeVIEvhuJTo/KHbWPx9ZUUEs3o/Ezqq8aL68525oxIoLfwNpw=","X-Received":"by 2002:a0d:db54:0:b0:556:c2e1:6258 with SMTP id\n\td81-20020a0ddb54000000b00556c2e16258mr5375489ywe.11.1685546201447;\n\tWed, 31 May 2023 08:16:41 -0700 (PDT)","MIME-Version":"1.0","References":"\n\t\n\t<20230531150629.GC24749@pendragon.ideasonboard.com>","In-Reply-To":"<20230531150629.GC24749@pendragon.ideasonboard.com>","Date":"Wed, 31 May 2023 16:16:28 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH] Add enable_auto_focus option to the\n\tGStreamer 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":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"Cedric Nugteren ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27196,"web_url":"https://patchwork.libcamera.org/comment/27196/","msgid":"<20230531155248.GF24749@pendragon.ideasonboard.com>","date":"2023-05-31T15:52:48","subject":"Re: [libcamera-devel] [PATCH] Add enable_auto_focus option to the\n\tGStreamer plugin","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Wed, May 31, 2023 at 04:16:28PM +0100, Naushir Patuck wrote:\n> On Wed, 31 May 2023 at 16:06, Laurent Pinchart wrote:\n> > On Mon, May 29, 2023 at 02:09:39PM -0400, Nicolas Dufresne via libcamera-devel wrote:\n> > > Le vendredi 26 mai 2023 à 14:34 +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> > > I'm not sure this default behaviour make sense. I would rather have it enabled\n> > > by default, since there is not other offered mean at the moment to obtain this\n> > > focus.\n> > >\n> > > That being said, I haven't looked at the auto-focus feature, I was expecting\n> > > there would be different type of it, which would endup with different default\n> > > behaviour.\n> >\n> > I think enabling it by default makes sense, but that leads me to another\n> > question: why is auto-focus not enabled by defaut (for cameras that\n> > support it) in the Raspberry Pi IPA module ?\n> \n> Believe it or not, this has been asked a few times now :-)\n> \n> The RPi IPA does not want to assume or trigger any particular behaviour based on\n> the specific sensor used. We feel it's up to applications to decide this\n> unconditionally. In the case of a focus lens available, the application ought to\n> choose what it wants to do, e.g. hyperfocal fixed position, AF cycle then fixed\n> position, CAF, etc. We don't really want to choose a default that may not be\n> what is needed as this is highly use case dependent.\n> \n> So we essentially do nothing and leave the lens where it last was in the absence\n> of any AF control.\n\nThank you for the explanation.\n\nI'm not totally sure I would have reached the same conclusion when it\ncomes to the default behaviour, but that's fine, your rationale\nregarding AF mode selection makes sense. What I dislike, however, is\nleaving the lens where it last was. Generally speaking, libcamera should\nensure a consistent and fully-defined camera state when starting, and\nshouldn't depend on previous capture sessions.\n\nI would also favour consistent behaviour across different cameras\nregarding the default value of controls, and the AF mode in this case.\nThis means I would be fine disabling AF by default for IPU3 (and rkisp1,\none we merge AF support for that platform).\n\nThis leaves open the question of what to do in the GStreamer element by\ndefault. Does it make sense for libcamera to default to disabling\nautofocus, and enabling it by default in libcamerasrc ?\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> > > > Signed-off-by: Cedric Nugteren \n> > > > ---\n> > > > src/gstreamer/gstlibcameraprovider.cpp | 13 ++++++++++++\n> > > > src/gstreamer/gstlibcamerasrc.cpp | 29 +++++++++++++++++++++++++-\n> > > > 2 files changed, 41 insertions(+), 1 deletion(-)\n> > > >\n> > > > diff --git a/src/gstreamer/gstlibcameraprovider.cpp\n> > > > b/src/gstreamer/gstlibcameraprovider.cpp\n> > > > index 6eb0a0eb..86fa2542 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> > > > PROP_DEVICE_NAME = 1,\n> > > > + PROP_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,\n> > > > gst_libcamera_device,\n> > > > struct _GstLibcameraDevice {\n> > > > GstDevice parent;\n> > > > gchar *name;\n> > > > + gboolean 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\n> > > > gchar *name)\n> > > > g_assert(source);\n> > > >\n> > > > g_object_set(source, \"camera-name\", GST_LIBCAMERA_DEVICE(device)->name,\n> > > > nullptr);\n> > > > + g_object_set(source, \"enable-auto-focus\", GST_LIBCAMERA_DEVICE(device)-\n> > > > >enable_auto_focus, nullptr);\n> > > >\n> > > > return source;\n> > > > }\n> > > > @@ -68,6 +71,7 @@ gst_libcamera_device_reconfigure_element(GstDevice *device,\n> > > > return FALSE;\n> > > >\n> > > > g_object_set(element, \"camera-name\", GST_LIBCAMERA_DEVICE(device)->name,\n> > > > nullptr);\n> > > > + g_object_set(element, \"enable-auto-focus\", GST_LIBCAMERA_DEVICE(device)-\n> > > > >enable_auto_focus, nullptr);\n> > >\n> > > That is strange, unlike camera-name, this information is not discovered at run-\n> > > time, hence does not require a notify callback. I'd simply set the default at\n> > > construction time, and avoid this here.\n> > >\n> > > > return TRUE;\n> > > > }\n> > > > @@ -82,6 +86,9 @@ gst_libcamera_device_set_property(GObject *object, guint\n> > > > prop_id,\n> > > > case PROP_DEVICE_NAME:\n> > > > device->name = g_value_dup_string(value);\n> > > > break;\n> > > > + case PROP_ENABLE_AUTO_FOCUS:\n> > > > + device->enable_auto_focus = g_value_get_boolean(value);\n> > > > + break;\n> > > > default:\n> > > > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > > > break;\n> > > > @@ -121,6 +128,12 @@ gst_libcamera_device_class_init(GstLibcameraDeviceClass\n> > > > *klass)\n> > > > (GParamFlags)(G_PARAM_STATIC_STRINGS | G_PARAM_WRITABLE |\n> > > > G_PARAM_CONSTRUCT_ONLY));\n> > > > g_object_class_install_property(object_class, PROP_DEVICE_NAME, pspec);\n> > > > + GParamSpec *spec2 = g_param_spec_boolean(\"enable-auto-focus\",\n> > >\n> > > Why a new one ? just reuse spec here, its just temporary pointer with no\n> > > ownership in it. It was only added to be able to fit into the maximum line\n> > > length.\n> > >\n> > > > + \"Enable auto-focus\",\n> > > > + \"Enable auto-focus if set to true, \"\n> > > > + \"disable it if set to false\",\n> > > > + FALSE, G_PARAM_WRITABLE);\n> > > > + g_object_class_install_property(object_class, PROP_ENABLE_AUTO_FOCUS, spec2);\n> > >\n> > > Note, if you feel like it, you could make it more obvious by using\n> > > g_steal_pointer() here, which is similar to std::move (but works for non C++\n> > > pointers).\n> > >\n> > > > }\n> > > >\n> > > > static GstDevice *\n> > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp\n> > > > b/src/gstreamer/gstlibcamerasrc.cpp\n> > > > index a10cbd4f..672ea38a 100644\n> > > > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > > > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > > > @@ -146,6 +146,7 @@ struct _GstLibcameraSrc {\n> > > > GstTask *task;\n> > > >\n> > > > gchar *camera_name;\n> > > > + gboolean enable_auto_focus = false;\n> > >\n> > > I guess I've been thinking C a lot so far, thanks for making me noticed we can\n> > > do that. Note a little inconstancy, gboolean is an in. So I'd suggest:\n> > >\n> > > + bool enable_auto_focus = false;\n> > >\n> > > > GstLibcameraSrcState *state;\n> > > > GstLibcameraAllocator *allocator;\n> > > > @@ -154,7 +155,8 @@ struct _GstLibcameraSrc {\n> > > >\n> > > > enum {\n> > > > PROP_0,\n> > > > - PROP_CAMERA_NAME\n> > > > + PROP_CAMERA_NAME,\n> > > > + PROP_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,\n> > > > [[maybe_unused]] GThread *thread,\n> > > > gst_flow_combiner_add_pad(self->flow_combiner, srcpad);\n> > > > }\n> > > >\n> > > > + if (self->enable_auto_focus) {\n> > > > + const ControlInfoMap &infoMap = state->cam_->controls();\n> > > > + if (infoMap.find(&controls::AfMode) != infoMap.end()) {\n> > > > + state->initControls_.set(controls::AfMode, controls::AfModeContinuous);\n> > > > + } else {\n> > > > + GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > > > + (\"Failed to enable auto focus\"),\n> > > > + (\"AfMode not found in camera controls, \"\n> > > > + \"please retry with 'enable-auto-focus=false'\"));\n> > > > + }\n> > > > + }\n> > > > +\n> > >\n> > > Please resend in text form as you are suppose to (I recommend using git send-\n> > > email). This will ensure we don't smash the indentation completely. I'll stop my\n> > > review here, as its too hard to read without indentation.\n> >\n> > I strongly recommend using git-send-email to send patches as well.\n> > Instructions are available at\n> > https://libcamera.org/contributing.html#submitting-patches, which links\n> > to the very nice https://git-send-email.io/ helper to setup\n> > git-send-email.\n> >\n> > > > ret = state->cam_->start(&state->initControls_);\n> > > > if (ret) {\n> > > > GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > > > @@ -659,6 +673,9 @@ gst_libcamera_src_set_property(GObject *object, guint\n> > > > prop_id,\n> > > > g_free(self->camera_name);\n> > > > self->camera_name = g_value_dup_string(value);\n> > > > break;\n> > > > + case PROP_ENABLE_AUTO_FOCUS:\n> > > > + self->enable_auto_focus = g_value_get_boolean(value);\n> > > > + break;\n> > > > default:\n> > > > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > > > break;\n> > > > @@ -676,6 +693,9 @@ gst_libcamera_src_get_property(GObject *object, guint\n> > > > prop_id, GValue *value,\n> > > > case PROP_CAMERA_NAME:\n> > > > g_value_set_string(value, self->camera_name);\n> > > > break;\n> > > > + case PROP_ENABLE_AUTO_FOCUS:\n> > > > + g_value_set_boolean(value, self->enable_auto_focus);\n> > > > + break;\n> > > > default:\n> > > > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > > > break;\n> > > > @@ -844,4 +864,11 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n> > > > | G_PARAM_READWRITE\n> > > > | G_PARAM_STATIC_STRINGS));\n> > > > g_object_class_install_property(object_class, PROP_CAMERA_NAME, spec);\n> > > > + GParamSpec *spec2 = g_param_spec_boolean(\"enable-auto-focus\",\n> > > > + \"Enable auto-focus\",\n> > > > + \"Enable auto-focus if set to true, \"\n> > > > + \"disable it if set to false\",\n> > > > + FALSE, G_PARAM_WRITABLE);\n> > > > + g_object_class_install_property(object_class, PROP_ENABLE_AUTO_FOCUS, spec2);\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 759E8C3213\n\tfor ;\n\tWed, 31 May 2023 15:52:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B1A36626FA;\n\tWed, 31 May 2023 17:52:51 +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 3E3CA626F8\n\tfor ;\n\tWed, 31 May 2023 17:52:50 +0200 (CEST)","from pendragon.ideasonboard.com (om126205251136.34.openmobile.ne.jp\n\t[126.205.251.136])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D3CD1844;\n\tWed, 31 May 2023 17:52:27 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685548371;\n\tbh=CK4xbq33mRHjBc/y861YqpMQLu10Yv1VvF9NOibEjAg=;\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=kJgZxQ9iVgZANdPrQxN3MGA8EkmS64vRkb7uV1enSHper/7ZlHBaINYkBMiiGnVS4\n\ttj4xkrVSj3NShmRCJ/i6QKBdQI3Kf68QjAbywa8L3JbDQS1Ban3RG+BuInzDnArrn6\n\tYS8tvU5uIqgzQGzr1FMehd93L2DymE0bikV4arG31+IdjKyCQVMsa4XEFtc0nP8y7t\n\tXrPTHNRQHXMAAcJYV69rxU82BkQ6Yjbc8rZCrRevd0GtE7Od7u8Uc9b73h3YBg+Hti\n\t2BaWbUiOdBeMWEzXab6Sby1XkpCxKi6lqtxuj0pjBiQgCHgN4ZTSD8w62T5imkDJOA\n\t1NMXZT5FKCz7g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685548348;\n\tbh=CK4xbq33mRHjBc/y861YqpMQLu10Yv1VvF9NOibEjAg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=v0Rlsp9m6AdtyoYaiebduPa/PiOR06fYJw4KLuTDWs54fwNPuDt3Ok7wuA/O1eQla\n\tNevLmrHXZs9j1PYX/QHBZDsKa8I0IKuJXYYa7xnpTHf7lbP3e59DUGf9Er7BYnzZqK\n\tzy974maV3ECGhII0zkn9PR75s1fy70Xky3lh71eY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"v0Rlsp9m\"; dkim-atps=neutral","Date":"Wed, 31 May 2023 18:52:48 +0300","To":"Naushir Patuck ","Message-ID":"<20230531155248.GF24749@pendragon.ideasonboard.com>","References":"\n\t\n\t<20230531150629.GC24749@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] Add enable_auto_focus option to the\n\tGStreamer 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27204,"web_url":"https://patchwork.libcamera.org/comment/27204/","msgid":"","date":"2023-06-01T07:21:22","subject":"Re: [libcamera-devel] [PATCH] Add enable_auto_focus option to the\n\tGStreamer plugin","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Wed, 31 May 2023 at 16:52, Laurent Pinchart\n wrote:\n>\n> Hi Naush,\n>\n> On Wed, May 31, 2023 at 04:16:28PM +0100, Naushir Patuck wrote:\n> > On Wed, 31 May 2023 at 16:06, Laurent Pinchart wrote:\n> > > On Mon, May 29, 2023 at 02:09:39PM -0400, Nicolas Dufresne via libcamera-devel wrote:\n> > > > Le vendredi 26 mai 2023 à 14:34 +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> > > > I'm not sure this default behaviour make sense. I would rather have it enabled\n> > > > by default, since there is not other offered mean at the moment to obtain this\n> > > > focus.\n> > > >\n> > > > That being said, I haven't looked at the auto-focus feature, I was expecting\n> > > > there would be different type of it, which would endup with different default\n> > > > behaviour.\n> > >\n> > > I think enabling it by default makes sense, but that leads me to another\n> > > question: why is auto-focus not enabled by defaut (for cameras that\n> > > support it) in the Raspberry Pi IPA module ?\n> >\n> > Believe it or not, this has been asked a few times now :-)\n> >\n> > The RPi IPA does not want to assume or trigger any particular behaviour based on\n> > the specific sensor used. We feel it's up to applications to decide this\n> > unconditionally. In the case of a focus lens available, the application ought to\n> > choose what it wants to do, e.g. hyperfocal fixed position, AF cycle then fixed\n> > position, CAF, etc. We don't really want to choose a default that may not be\n> > what is needed as this is highly use case dependent.\n> >\n> > So we essentially do nothing and leave the lens where it last was in the absence\n> > of any AF control.\n>\n> Thank you for the explanation.\n>\n> I'm not totally sure I would have reached the same conclusion when it\n> comes to the default behaviour, but that's fine, your rationale\n> regarding AF mode selection makes sense. What I dislike, however, is\n> leaving the lens where it last was. Generally speaking, libcamera should\n> ensure a consistent and fully-defined camera state when starting, and\n> shouldn't depend on previous capture sessions.\n\nYes, I do agree with this.\n\nThe problem is choosing a default behaviour - CAF would not really be a nice\ndefault experience on a non-PDAF sensor. Perhaps doing a move to hyperfocal is\na good compromise and will provide a consistent experience across platforms and\nsensor modules?\n\nRegards,\nNaush\n\n>\n> I would also favour consistent behaviour across different cameras\n> regarding the default value of controls, and the AF mode in this case.\n> This means I would be fine disabling AF by default for IPU3 (and rkisp1,\n> one we merge AF support for that platform).\n>\n> This leaves open the question of what to do in the GStreamer element by\n> default. Does it make sense for libcamera to default to disabling\n> autofocus, and enabling it by default in libcamerasrc ?\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> > > > > Signed-off-by: Cedric Nugteren \n> > > > > ---\n> > > > > src/gstreamer/gstlibcameraprovider.cpp | 13 ++++++++++++\n> > > > > src/gstreamer/gstlibcamerasrc.cpp | 29 +++++++++++++++++++++++++-\n> > > > > 2 files changed, 41 insertions(+), 1 deletion(-)\n> > > > >\n> > > > > diff --git a/src/gstreamer/gstlibcameraprovider.cpp\n> > > > > b/src/gstreamer/gstlibcameraprovider.cpp\n> > > > > index 6eb0a0eb..86fa2542 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> > > > > PROP_DEVICE_NAME = 1,\n> > > > > + PROP_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,\n> > > > > gst_libcamera_device,\n> > > > > struct _GstLibcameraDevice {\n> > > > > GstDevice parent;\n> > > > > gchar *name;\n> > > > > + gboolean 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\n> > > > > gchar *name)\n> > > > > g_assert(source);\n> > > > >\n> > > > > g_object_set(source, \"camera-name\", GST_LIBCAMERA_DEVICE(device)->name,\n> > > > > nullptr);\n> > > > > + g_object_set(source, \"enable-auto-focus\", GST_LIBCAMERA_DEVICE(device)-\n> > > > > >enable_auto_focus, nullptr);\n> > > > >\n> > > > > return source;\n> > > > > }\n> > > > > @@ -68,6 +71,7 @@ gst_libcamera_device_reconfigure_element(GstDevice *device,\n> > > > > return FALSE;\n> > > > >\n> > > > > g_object_set(element, \"camera-name\", GST_LIBCAMERA_DEVICE(device)->name,\n> > > > > nullptr);\n> > > > > + g_object_set(element, \"enable-auto-focus\", GST_LIBCAMERA_DEVICE(device)-\n> > > > > >enable_auto_focus, nullptr);\n> > > >\n> > > > That is strange, unlike camera-name, this information is not discovered at run-\n> > > > time, hence does not require a notify callback. I'd simply set the default at\n> > > > construction time, and avoid this here.\n> > > >\n> > > > > return TRUE;\n> > > > > }\n> > > > > @@ -82,6 +86,9 @@ gst_libcamera_device_set_property(GObject *object, guint\n> > > > > prop_id,\n> > > > > case PROP_DEVICE_NAME:\n> > > > > device->name = g_value_dup_string(value);\n> > > > > break;\n> > > > > + case PROP_ENABLE_AUTO_FOCUS:\n> > > > > + device->enable_auto_focus = g_value_get_boolean(value);\n> > > > > + break;\n> > > > > default:\n> > > > > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > > > > break;\n> > > > > @@ -121,6 +128,12 @@ gst_libcamera_device_class_init(GstLibcameraDeviceClass\n> > > > > *klass)\n> > > > > (GParamFlags)(G_PARAM_STATIC_STRINGS | G_PARAM_WRITABLE |\n> > > > > G_PARAM_CONSTRUCT_ONLY));\n> > > > > g_object_class_install_property(object_class, PROP_DEVICE_NAME, pspec);\n> > > > > + GParamSpec *spec2 = g_param_spec_boolean(\"enable-auto-focus\",\n> > > >\n> > > > Why a new one ? just reuse spec here, its just temporary pointer with no\n> > > > ownership in it. It was only added to be able to fit into the maximum line\n> > > > length.\n> > > >\n> > > > > + \"Enable auto-focus\",\n> > > > > + \"Enable auto-focus if set to true, \"\n> > > > > + \"disable it if set to false\",\n> > > > > + FALSE, G_PARAM_WRITABLE);\n> > > > > + g_object_class_install_property(object_class, PROP_ENABLE_AUTO_FOCUS, spec2);\n> > > >\n> > > > Note, if you feel like it, you could make it more obvious by using\n> > > > g_steal_pointer() here, which is similar to std::move (but works for non C++\n> > > > pointers).\n> > > >\n> > > > > }\n> > > > >\n> > > > > static GstDevice *\n> > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp\n> > > > > b/src/gstreamer/gstlibcamerasrc.cpp\n> > > > > index a10cbd4f..672ea38a 100644\n> > > > > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > > > > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > > > > @@ -146,6 +146,7 @@ struct _GstLibcameraSrc {\n> > > > > GstTask *task;\n> > > > >\n> > > > > gchar *camera_name;\n> > > > > + gboolean enable_auto_focus = false;\n> > > >\n> > > > I guess I've been thinking C a lot so far, thanks for making me noticed we can\n> > > > do that. Note a little inconstancy, gboolean is an in. So I'd suggest:\n> > > >\n> > > > + bool enable_auto_focus = false;\n> > > >\n> > > > > GstLibcameraSrcState *state;\n> > > > > GstLibcameraAllocator *allocator;\n> > > > > @@ -154,7 +155,8 @@ struct _GstLibcameraSrc {\n> > > > >\n> > > > > enum {\n> > > > > PROP_0,\n> > > > > - PROP_CAMERA_NAME\n> > > > > + PROP_CAMERA_NAME,\n> > > > > + PROP_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,\n> > > > > [[maybe_unused]] GThread *thread,\n> > > > > gst_flow_combiner_add_pad(self->flow_combiner, srcpad);\n> > > > > }\n> > > > >\n> > > > > + if (self->enable_auto_focus) {\n> > > > > + const ControlInfoMap &infoMap = state->cam_->controls();\n> > > > > + if (infoMap.find(&controls::AfMode) != infoMap.end()) {\n> > > > > + state->initControls_.set(controls::AfMode, controls::AfModeContinuous);\n> > > > > + } else {\n> > > > > + GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > > > > + (\"Failed to enable auto focus\"),\n> > > > > + (\"AfMode not found in camera controls, \"\n> > > > > + \"please retry with 'enable-auto-focus=false'\"));\n> > > > > + }\n> > > > > + }\n> > > > > +\n> > > >\n> > > > Please resend in text form as you are suppose to (I recommend using git send-\n> > > > email). This will ensure we don't smash the indentation completely. I'll stop my\n> > > > review here, as its too hard to read without indentation.\n> > >\n> > > I strongly recommend using git-send-email to send patches as well.\n> > > Instructions are available at\n> > > https://libcamera.org/contributing.html#submitting-patches, which links\n> > > to the very nice https://git-send-email.io/ helper to setup\n> > > git-send-email.\n> > >\n> > > > > ret = state->cam_->start(&state->initControls_);\n> > > > > if (ret) {\n> > > > > GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > > > > @@ -659,6 +673,9 @@ gst_libcamera_src_set_property(GObject *object, guint\n> > > > > prop_id,\n> > > > > g_free(self->camera_name);\n> > > > > self->camera_name = g_value_dup_string(value);\n> > > > > break;\n> > > > > + case PROP_ENABLE_AUTO_FOCUS:\n> > > > > + self->enable_auto_focus = g_value_get_boolean(value);\n> > > > > + break;\n> > > > > default:\n> > > > > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > > > > break;\n> > > > > @@ -676,6 +693,9 @@ gst_libcamera_src_get_property(GObject *object, guint\n> > > > > prop_id, GValue *value,\n> > > > > case PROP_CAMERA_NAME:\n> > > > > g_value_set_string(value, self->camera_name);\n> > > > > break;\n> > > > > + case PROP_ENABLE_AUTO_FOCUS:\n> > > > > + g_value_set_boolean(value, self->enable_auto_focus);\n> > > > > + break;\n> > > > > default:\n> > > > > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > > > > break;\n> > > > > @@ -844,4 +864,11 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n> > > > > | G_PARAM_READWRITE\n> > > > > | G_PARAM_STATIC_STRINGS));\n> > > > > g_object_class_install_property(object_class, PROP_CAMERA_NAME, spec);\n> > > > > + GParamSpec *spec2 = g_param_spec_boolean(\"enable-auto-focus\",\n> > > > > + \"Enable auto-focus\",\n> > > > > + \"Enable auto-focus if set to true, \"\n> > > > > + \"disable it if set to false\",\n> > > > > + FALSE, G_PARAM_WRITABLE);\n> > > > > + g_object_class_install_property(object_class, PROP_ENABLE_AUTO_FOCUS, spec2);\n> > > > > +\n> > > > > }\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 6791DC31E9\n\tfor ;\n\tThu, 1 Jun 2023 07:21:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C6942626FA;\n\tThu, 1 Jun 2023 09:21:38 +0200 (CEST)","from mail-yb1-xb2d.google.com (mail-yb1-xb2d.google.com\n\t[IPv6:2607:f8b0:4864:20::b2d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B0EF361EA6\n\tfor ;\n\tThu, 1 Jun 2023 09:21:37 +0200 (CEST)","by mail-yb1-xb2d.google.com with SMTP id\n\t3f1490d57ef6-bacfc573647so533349276.1\n\tfor ;\n\tThu, 01 Jun 2023 00:21:37 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685604098;\n\tbh=KQph5m1RcJQ/jmi1J15JV2w+pVGj/YR8fCPrJOCF7GE=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=12SWapaG7hOlhmqSZt4czdCaXPUZaYhORiQZ/1bx1xkhoyG0pWzrQTHmGqts3bJcR\n\tvfL6lmVGSIspjUmVtLwCTTCSReq2ZyLt+xPAhpIAMu2dKTtOZ2bVrjoMV557kRmeR8\n\t3wV5rYunKMF9vKFxzP4LUxeKhYokg22n7n+/xYFwuvZJxQApAzvEz+r8Fw0ODJ4GSH\n\tqYiLTz2hqEu63t3QODbRMwP+VQFP3pPHPsd1R8+sNBXqh/sAW9ConZlg8pB/mUtVG3\n\tMq09PN3uzfl/twKn2gWOO/F8QNfFKuNkX55AlatVcuwLTA8fpZco0PqntkPUZiBinN\n\tWJCwt23vecCcQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1685604096; x=1688196096;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=e2jOtSEa426tPJn0odOj6Z39E3YzZ6c73nYomnytZIE=;\n\tb=V726E/nT466ZPphT/+rymlzwN7VVoRy6Pq1HNB/q0pa+ovo9kotv5/AFgUKbN+PPim\n\tHadmrvikdaXsu4Pwjg7Av+a+n0pyvpT9Bza/QFgjPUtBrRZwD6MHZeAomexVZ2HK6JC0\n\tFDjXIpCS+ujpUl75u4RvoqaDstIXWDUIah4vvVBqKHDAN1MzHx6vc6LAHA+Hg4k6UZ3i\n\tW6ALcv55A8DA5kkoLGxPlrpS2oowmXFT/lSlYyo2Y7BwMbrmkdHSsFga89CDyARW5+hp\n\tcl4F4O/jip4dTbchUQ5jTxq4LfBEBCgG/j3xeBFs0f7eNwdZEgrQx6tMeYZZ3NmgRw7o\n\tG79Q=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"V726E/nT\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1685604096; x=1688196096;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=e2jOtSEa426tPJn0odOj6Z39E3YzZ6c73nYomnytZIE=;\n\tb=S0LndhlaLtmHRlRYAwVI/rQwnaxCKj7bQBR+1svR+6JbaRv4fKTFNgfNOJXz4ah676\n\tdDUHDSQLmZ0gX7bTLBJvUdwU4ZKnp+fL4iXroyHRW9X2wEIVR3Qtz+Md0ch7JXymKQdz\n\tCJpuGRyOQ88/o4UeJFDHyp67QSbcDCdHVAYiALLez2GIvA98ZDmLfyuv8/6z7+kcu635\n\t11OArBrUU/MPEYDRqFIyT0q8PfN8lGZyl1k6GamHaDjvKJeIBGc9hAmbtFMZ0lIMUy/k\n\tjpfE/XNhOA96iEK8YNpA4TUYjD+sI8pGVx0Lco4NWUnfnEAFbdDzmTctmg8sq0I7hDtS\n\ttQDg==","X-Gm-Message-State":"AC+VfDx0UhURoGUjve9v6OUfIM05hvtp19kDT+XFOlUdJrJq2a3dQlJp\n\t1M6bC5P1gb+jq2gSKtErDySHhCAsPv7Isz5IWmvcXg==","X-Google-Smtp-Source":"ACHHUZ5ctnxRFupZQ1KHTn+NRdK41u/m6mlPRA8j6Qdoekh83gZNsQ1pNzErAfWVS/jAHZZXmG9jtwUsjoLu2j0im2U=","X-Received":"by 2002:a0d:cb43:0:b0:561:a7fd:d6b3 with SMTP id\n\tn64-20020a0dcb43000000b00561a7fdd6b3mr8370696ywd.33.1685604096344;\n\tThu, 01 Jun 2023 00:21:36 -0700 (PDT)","MIME-Version":"1.0","References":"\n\t\n\t<20230531150629.GC24749@pendragon.ideasonboard.com>\n\t\n\t<20230531155248.GF24749@pendragon.ideasonboard.com>","In-Reply-To":"<20230531155248.GF24749@pendragon.ideasonboard.com>","Date":"Thu, 1 Jun 2023 08:21:22 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH] Add enable_auto_focus option to the\n\tGStreamer 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":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"Cedric Nugteren ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27206,"web_url":"https://patchwork.libcamera.org/comment/27206/","msgid":"<20230601072857.GC22609@pendragon.ideasonboard.com>","date":"2023-06-01T07:28:57","subject":"Re: [libcamera-devel] [PATCH] Add enable_auto_focus option to the\n\tGStreamer plugin","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Thu, Jun 01, 2023 at 08:21:22AM +0100, Naushir Patuck wrote:\n> On Wed, 31 May 2023 at 16:52, Laurent Pinchart wrote:\n> > On Wed, May 31, 2023 at 04:16:28PM +0100, Naushir Patuck wrote:\n> > > On Wed, 31 May 2023 at 16:06, Laurent Pinchart wrote:\n> > > > On Mon, May 29, 2023 at 02:09:39PM -0400, Nicolas Dufresne via libcamera-devel wrote:\n> > > > > Le vendredi 26 mai 2023 à 14:34 +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> > > > > I'm not sure this default behaviour make sense. I would rather have it enabled\n> > > > > by default, since there is not other offered mean at the moment to obtain this\n> > > > > focus.\n> > > > >\n> > > > > That being said, I haven't looked at the auto-focus feature, I was expecting\n> > > > > there would be different type of it, which would endup with different default\n> > > > > behaviour.\n> > > >\n> > > > I think enabling it by default makes sense, but that leads me to another\n> > > > question: why is auto-focus not enabled by defaut (for cameras that\n> > > > support it) in the Raspberry Pi IPA module ?\n> > >\n> > > Believe it or not, this has been asked a few times now :-)\n> > >\n> > > The RPi IPA does not want to assume or trigger any particular behaviour based on\n> > > the specific sensor used. We feel it's up to applications to decide this\n> > > unconditionally. In the case of a focus lens available, the application ought to\n> > > choose what it wants to do, e.g. hyperfocal fixed position, AF cycle then fixed\n> > > position, CAF, etc. We don't really want to choose a default that may not be\n> > > what is needed as this is highly use case dependent.\n> > >\n> > > So we essentially do nothing and leave the lens where it last was in the absence\n> > > of any AF control.\n> >\n> > Thank you for the explanation.\n> >\n> > I'm not totally sure I would have reached the same conclusion when it\n> > comes to the default behaviour, but that's fine, your rationale\n> > regarding AF mode selection makes sense. What I dislike, however, is\n> > leaving the lens where it last was. Generally speaking, libcamera should\n> > ensure a consistent and fully-defined camera state when starting, and\n> > shouldn't depend on previous capture sessions.\n> \n> Yes, I do agree with this.\n> \n> The problem is choosing a default behaviour - CAF would not really be a nice\n> default experience on a non-PDAF sensor. Perhaps doing a move to hyperfocal is\n> a good compromise and will provide a consistent experience across platforms and\n> sensor modules?\n\nThis works for me. Would you send a patch to do so in the Raspberry Pi\nIPA module ?\n\nIf you have any idea what the default should be in the GStreamer\nelement, please feel free to chime in :-)\n\n> > I would also favour consistent behaviour across different cameras\n> > regarding the default value of controls, and the AF mode in this case.\n> > This means I would be fine disabling AF by default for IPU3 (and rkisp1,\n> > one we merge AF support for that platform).\n> >\n> > This leaves open the question of what to do in the GStreamer element by\n> > default. Does it make sense for libcamera to default to disabling\n> > autofocus, and enabling it by default in libcamerasrc ?\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> > > > > > Signed-off-by: Cedric Nugteren \n> > > > > > ---\n> > > > > > src/gstreamer/gstlibcameraprovider.cpp | 13 ++++++++++++\n> > > > > > src/gstreamer/gstlibcamerasrc.cpp | 29 +++++++++++++++++++++++++-\n> > > > > > 2 files changed, 41 insertions(+), 1 deletion(-)\n> > > > > >\n> > > > > > diff --git a/src/gstreamer/gstlibcameraprovider.cpp\n> > > > > > b/src/gstreamer/gstlibcameraprovider.cpp\n> > > > > > index 6eb0a0eb..86fa2542 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> > > > > > PROP_DEVICE_NAME = 1,\n> > > > > > + PROP_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,\n> > > > > > gst_libcamera_device,\n> > > > > > struct _GstLibcameraDevice {\n> > > > > > GstDevice parent;\n> > > > > > gchar *name;\n> > > > > > + gboolean 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\n> > > > > > gchar *name)\n> > > > > > g_assert(source);\n> > > > > >\n> > > > > > g_object_set(source, \"camera-name\", GST_LIBCAMERA_DEVICE(device)->name,\n> > > > > > nullptr);\n> > > > > > + g_object_set(source, \"enable-auto-focus\", GST_LIBCAMERA_DEVICE(device)-\n> > > > > > >enable_auto_focus, nullptr);\n> > > > > >\n> > > > > > return source;\n> > > > > > }\n> > > > > > @@ -68,6 +71,7 @@ gst_libcamera_device_reconfigure_element(GstDevice *device,\n> > > > > > return FALSE;\n> > > > > >\n> > > > > > g_object_set(element, \"camera-name\", GST_LIBCAMERA_DEVICE(device)->name,\n> > > > > > nullptr);\n> > > > > > + g_object_set(element, \"enable-auto-focus\", GST_LIBCAMERA_DEVICE(device)-\n> > > > > > >enable_auto_focus, nullptr);\n> > > > >\n> > > > > That is strange, unlike camera-name, this information is not discovered at run-\n> > > > > time, hence does not require a notify callback. I'd simply set the default at\n> > > > > construction time, and avoid this here.\n> > > > >\n> > > > > > return TRUE;\n> > > > > > }\n> > > > > > @@ -82,6 +86,9 @@ gst_libcamera_device_set_property(GObject *object, guint\n> > > > > > prop_id,\n> > > > > > case PROP_DEVICE_NAME:\n> > > > > > device->name = g_value_dup_string(value);\n> > > > > > break;\n> > > > > > + case PROP_ENABLE_AUTO_FOCUS:\n> > > > > > + device->enable_auto_focus = g_value_get_boolean(value);\n> > > > > > + break;\n> > > > > > default:\n> > > > > > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > > > > > break;\n> > > > > > @@ -121,6 +128,12 @@ gst_libcamera_device_class_init(GstLibcameraDeviceClass\n> > > > > > *klass)\n> > > > > > (GParamFlags)(G_PARAM_STATIC_STRINGS | G_PARAM_WRITABLE |\n> > > > > > G_PARAM_CONSTRUCT_ONLY));\n> > > > > > g_object_class_install_property(object_class, PROP_DEVICE_NAME, pspec);\n> > > > > > + GParamSpec *spec2 = g_param_spec_boolean(\"enable-auto-focus\",\n> > > > >\n> > > > > Why a new one ? just reuse spec here, its just temporary pointer with no\n> > > > > ownership in it. It was only added to be able to fit into the maximum line\n> > > > > length.\n> > > > >\n> > > > > > + \"Enable auto-focus\",\n> > > > > > + \"Enable auto-focus if set to true, \"\n> > > > > > + \"disable it if set to false\",\n> > > > > > + FALSE, G_PARAM_WRITABLE);\n> > > > > > + g_object_class_install_property(object_class, PROP_ENABLE_AUTO_FOCUS, spec2);\n> > > > >\n> > > > > Note, if you feel like it, you could make it more obvious by using\n> > > > > g_steal_pointer() here, which is similar to std::move (but works for non C++\n> > > > > pointers).\n> > > > >\n> > > > > > }\n> > > > > >\n> > > > > > static GstDevice *\n> > > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp\n> > > > > > b/src/gstreamer/gstlibcamerasrc.cpp\n> > > > > > index a10cbd4f..672ea38a 100644\n> > > > > > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > > > > > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > > > > > @@ -146,6 +146,7 @@ struct _GstLibcameraSrc {\n> > > > > > GstTask *task;\n> > > > > >\n> > > > > > gchar *camera_name;\n> > > > > > + gboolean enable_auto_focus = false;\n> > > > >\n> > > > > I guess I've been thinking C a lot so far, thanks for making me noticed we can\n> > > > > do that. Note a little inconstancy, gboolean is an in. So I'd suggest:\n> > > > >\n> > > > > + bool enable_auto_focus = false;\n> > > > >\n> > > > > > GstLibcameraSrcState *state;\n> > > > > > GstLibcameraAllocator *allocator;\n> > > > > > @@ -154,7 +155,8 @@ struct _GstLibcameraSrc {\n> > > > > >\n> > > > > > enum {\n> > > > > > PROP_0,\n> > > > > > - PROP_CAMERA_NAME\n> > > > > > + PROP_CAMERA_NAME,\n> > > > > > + PROP_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,\n> > > > > > [[maybe_unused]] GThread *thread,\n> > > > > > gst_flow_combiner_add_pad(self->flow_combiner, srcpad);\n> > > > > > }\n> > > > > >\n> > > > > > + if (self->enable_auto_focus) {\n> > > > > > + const ControlInfoMap &infoMap = state->cam_->controls();\n> > > > > > + if (infoMap.find(&controls::AfMode) != infoMap.end()) {\n> > > > > > + state->initControls_.set(controls::AfMode, controls::AfModeContinuous);\n> > > > > > + } else {\n> > > > > > + GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > > > > > + (\"Failed to enable auto focus\"),\n> > > > > > + (\"AfMode not found in camera controls, \"\n> > > > > > + \"please retry with 'enable-auto-focus=false'\"));\n> > > > > > + }\n> > > > > > + }\n> > > > > > +\n> > > > >\n> > > > > Please resend in text form as you are suppose to (I recommend using git send-\n> > > > > email). This will ensure we don't smash the indentation completely. I'll stop my\n> > > > > review here, as its too hard to read without indentation.\n> > > >\n> > > > I strongly recommend using git-send-email to send patches as well.\n> > > > Instructions are available at\n> > > > https://libcamera.org/contributing.html#submitting-patches, which links\n> > > > to the very nice https://git-send-email.io/ helper to setup\n> > > > git-send-email.\n> > > >\n> > > > > > ret = state->cam_->start(&state->initControls_);\n> > > > > > if (ret) {\n> > > > > > GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > > > > > @@ -659,6 +673,9 @@ gst_libcamera_src_set_property(GObject *object, guint\n> > > > > > prop_id,\n> > > > > > g_free(self->camera_name);\n> > > > > > self->camera_name = g_value_dup_string(value);\n> > > > > > break;\n> > > > > > + case PROP_ENABLE_AUTO_FOCUS:\n> > > > > > + self->enable_auto_focus = g_value_get_boolean(value);\n> > > > > > + break;\n> > > > > > default:\n> > > > > > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > > > > > break;\n> > > > > > @@ -676,6 +693,9 @@ gst_libcamera_src_get_property(GObject *object, guint\n> > > > > > prop_id, GValue *value,\n> > > > > > case PROP_CAMERA_NAME:\n> > > > > > g_value_set_string(value, self->camera_name);\n> > > > > > break;\n> > > > > > + case PROP_ENABLE_AUTO_FOCUS:\n> > > > > > + g_value_set_boolean(value, self->enable_auto_focus);\n> > > > > > + break;\n> > > > > > default:\n> > > > > > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > > > > > break;\n> > > > > > @@ -844,4 +864,11 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n> > > > > > | G_PARAM_READWRITE\n> > > > > > | G_PARAM_STATIC_STRINGS));\n> > > > > > g_object_class_install_property(object_class, PROP_CAMERA_NAME, spec);\n> > > > > > + GParamSpec *spec2 = g_param_spec_boolean(\"enable-auto-focus\",\n> > > > > > + \"Enable auto-focus\",\n> > > > > > + \"Enable auto-focus if set to true, \"\n> > > > > > + \"disable it if set to false\",\n> > > > > > + FALSE, G_PARAM_WRITABLE);\n> > > > > > + g_object_class_install_property(object_class, PROP_ENABLE_AUTO_FOCUS, spec2);\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 CED08C31E9\n\tfor ;\n\tThu, 1 Jun 2023 07:29:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 32929626FE;\n\tThu, 1 Jun 2023 09:29:01 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CDE3E626FA\n\tfor ;\n\tThu, 1 Jun 2023 09:28:59 +0200 (CEST)","from pendragon.ideasonboard.com (om126205251136.34.openmobile.ne.jp\n\t[126.205.251.136])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EC5BD289;\n\tThu, 1 Jun 2023 09:28:36 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685604541;\n\tbh=Yth+R+380dIKzGGIzUcTcV1PER/XNfmT6oz+bFe3GuY=;\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=jTuhY4rBI4ejnK91UCfc1jBu5xqWs2oTIz7m13XV44cVNT8vdnB4fnhAZJShu5SAj\n\tv50H0stTb0QHNOFoMKdDWJiMiBNOwzQ9RNXxwIey8jXJbKGywmCIjJdf4hCgicWUKs\n\tWbqICMD1y410fqkcM5EOfj3u/lNJbnOmxjvs+OFw/+abtS/UyEYYdDDrljqImhRaJs\n\tPPg4BJDBAMOB3zRr3vinfbFwUeRVRUlmtppz3N6I3u5BGkEzB+NtWMdMU3STvWaeKx\n\tW5QBqvW+BMOvVOTSUGGkgcxh73GghJGo5CmWw+gNYztKM8XNfA6jacz7gIt2wxmDL5\n\tV+gU/bdIn/iug==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685604518;\n\tbh=Yth+R+380dIKzGGIzUcTcV1PER/XNfmT6oz+bFe3GuY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=iJTUoWyt4lskJe2c/LXhZKLqDOcrtc2vHYk2VVP9DiMMNJJtJpWoHWr5ZkK90PwTS\n\t/7cnR5fHg5wJavrG2x0nX2soCTUv+e2TxhVZNK7Qcz/roTUmNq+8GIX1rySxMyFH1N\n\tBxQxqsQFeWdoIl3y7pufLswo3OM8uCrNYtqKYuI0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"iJTUoWyt\"; dkim-atps=neutral","Date":"Thu, 1 Jun 2023 10:28:57 +0300","To":"Naushir Patuck ","Message-ID":"<20230601072857.GC22609@pendragon.ideasonboard.com>","References":"\n\t\n\t<20230531150629.GC24749@pendragon.ideasonboard.com>\n\t\n\t<20230531155248.GF24749@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] Add enable_auto_focus option to the\n\tGStreamer 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27208,"web_url":"https://patchwork.libcamera.org/comment/27208/","msgid":"","date":"2023-06-01T07:39:07","subject":"Re: [libcamera-devel] [PATCH] Add enable_auto_focus option to the\n\tGStreamer plugin","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Thu, 1 Jun 2023 at 08:29, Laurent Pinchart\n wrote:\n>\n> Hi Naush,\n>\n> On Thu, Jun 01, 2023 at 08:21:22AM +0100, Naushir Patuck wrote:\n> > On Wed, 31 May 2023 at 16:52, Laurent Pinchart wrote:\n> > > On Wed, May 31, 2023 at 04:16:28PM +0100, Naushir Patuck wrote:\n> > > > On Wed, 31 May 2023 at 16:06, Laurent Pinchart wrote:\n> > > > > On Mon, May 29, 2023 at 02:09:39PM -0400, Nicolas Dufresne via libcamera-devel wrote:\n> > > > > > Le vendredi 26 mai 2023 à 14:34 +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> > > > > > I'm not sure this default behaviour make sense. I would rather have it enabled\n> > > > > > by default, since there is not other offered mean at the moment to obtain this\n> > > > > > focus.\n> > > > > >\n> > > > > > That being said, I haven't looked at the auto-focus feature, I was expecting\n> > > > > > there would be different type of it, which would endup with different default\n> > > > > > behaviour.\n> > > > >\n> > > > > I think enabling it by default makes sense, but that leads me to another\n> > > > > question: why is auto-focus not enabled by defaut (for cameras that\n> > > > > support it) in the Raspberry Pi IPA module ?\n> > > >\n> > > > Believe it or not, this has been asked a few times now :-)\n> > > >\n> > > > The RPi IPA does not want to assume or trigger any particular behaviour based on\n> > > > the specific sensor used. We feel it's up to applications to decide this\n> > > > unconditionally. In the case of a focus lens available, the application ought to\n> > > > choose what it wants to do, e.g. hyperfocal fixed position, AF cycle then fixed\n> > > > position, CAF, etc. We don't really want to choose a default that may not be\n> > > > what is needed as this is highly use case dependent.\n> > > >\n> > > > So we essentially do nothing and leave the lens where it last was in the absence\n> > > > of any AF control.\n> > >\n> > > Thank you for the explanation.\n> > >\n> > > I'm not totally sure I would have reached the same conclusion when it\n> > > comes to the default behaviour, but that's fine, your rationale\n> > > regarding AF mode selection makes sense. What I dislike, however, is\n> > > leaving the lens where it last was. Generally speaking, libcamera should\n> > > ensure a consistent and fully-defined camera state when starting, and\n> > > shouldn't depend on previous capture sessions.\n> >\n> > Yes, I do agree with this.\n> >\n> > The problem is choosing a default behaviour - CAF would not really be a nice\n> > default experience on a non-PDAF sensor. Perhaps doing a move to hyperfocal is\n> > a good compromise and will provide a consistent experience across platforms and\n> > sensor modules?\n>\n> This works for me. Would you send a patch to do so in the Raspberry Pi\n> IPA module ?\n\nYes I can do that. We should also possibly document this as the expected\ndefault somewhere, maybe in the LensPosition control description?\n\n>\n> If you have any idea what the default should be in the GStreamer\n> element, please feel free to chime in :-)\n\nPerhaps GStreamer should do nothing with focus by default and rely on the IPA\ndefault behaviour (i.e. go to hyperfocal)? This then relies on the application\nto choose what it wants to do on startup, just like any other application that\ndirectly uses libcamera.\n\nRegards,\nNaush\n\n\n>\n> > > I would also favour consistent behaviour across different cameras\n> > > regarding the default value of controls, and the AF mode in this case.\n> > > This means I would be fine disabling AF by default for IPU3 (and rkisp1,\n> > > one we merge AF support for that platform).\n> > >\n> > > This leaves open the question of what to do in the GStreamer element by\n> > > default. Does it make sense for libcamera to default to disabling\n> > > autofocus, and enabling it by default in libcamerasrc ?\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> > > > > > > Signed-off-by: Cedric Nugteren \n> > > > > > > ---\n> > > > > > > src/gstreamer/gstlibcameraprovider.cpp | 13 ++++++++++++\n> > > > > > > src/gstreamer/gstlibcamerasrc.cpp | 29 +++++++++++++++++++++++++-\n> > > > > > > 2 files changed, 41 insertions(+), 1 deletion(-)\n> > > > > > >\n> > > > > > > diff --git a/src/gstreamer/gstlibcameraprovider.cpp\n> > > > > > > b/src/gstreamer/gstlibcameraprovider.cpp\n> > > > > > > index 6eb0a0eb..86fa2542 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> > > > > > > PROP_DEVICE_NAME = 1,\n> > > > > > > + PROP_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,\n> > > > > > > gst_libcamera_device,\n> > > > > > > struct _GstLibcameraDevice {\n> > > > > > > GstDevice parent;\n> > > > > > > gchar *name;\n> > > > > > > + gboolean 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\n> > > > > > > gchar *name)\n> > > > > > > g_assert(source);\n> > > > > > >\n> > > > > > > g_object_set(source, \"camera-name\", GST_LIBCAMERA_DEVICE(device)->name,\n> > > > > > > nullptr);\n> > > > > > > + g_object_set(source, \"enable-auto-focus\", GST_LIBCAMERA_DEVICE(device)-\n> > > > > > > >enable_auto_focus, nullptr);\n> > > > > > >\n> > > > > > > return source;\n> > > > > > > }\n> > > > > > > @@ -68,6 +71,7 @@ gst_libcamera_device_reconfigure_element(GstDevice *device,\n> > > > > > > return FALSE;\n> > > > > > >\n> > > > > > > g_object_set(element, \"camera-name\", GST_LIBCAMERA_DEVICE(device)->name,\n> > > > > > > nullptr);\n> > > > > > > + g_object_set(element, \"enable-auto-focus\", GST_LIBCAMERA_DEVICE(device)-\n> > > > > > > >enable_auto_focus, nullptr);\n> > > > > >\n> > > > > > That is strange, unlike camera-name, this information is not discovered at run-\n> > > > > > time, hence does not require a notify callback. I'd simply set the default at\n> > > > > > construction time, and avoid this here.\n> > > > > >\n> > > > > > > return TRUE;\n> > > > > > > }\n> > > > > > > @@ -82,6 +86,9 @@ gst_libcamera_device_set_property(GObject *object, guint\n> > > > > > > prop_id,\n> > > > > > > case PROP_DEVICE_NAME:\n> > > > > > > device->name = g_value_dup_string(value);\n> > > > > > > break;\n> > > > > > > + case PROP_ENABLE_AUTO_FOCUS:\n> > > > > > > + device->enable_auto_focus = g_value_get_boolean(value);\n> > > > > > > + break;\n> > > > > > > default:\n> > > > > > > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > > > > > > break;\n> > > > > > > @@ -121,6 +128,12 @@ gst_libcamera_device_class_init(GstLibcameraDeviceClass\n> > > > > > > *klass)\n> > > > > > > (GParamFlags)(G_PARAM_STATIC_STRINGS | G_PARAM_WRITABLE |\n> > > > > > > G_PARAM_CONSTRUCT_ONLY));\n> > > > > > > g_object_class_install_property(object_class, PROP_DEVICE_NAME, pspec);\n> > > > > > > + GParamSpec *spec2 = g_param_spec_boolean(\"enable-auto-focus\",\n> > > > > >\n> > > > > > Why a new one ? just reuse spec here, its just temporary pointer with no\n> > > > > > ownership in it. It was only added to be able to fit into the maximum line\n> > > > > > length.\n> > > > > >\n> > > > > > > + \"Enable auto-focus\",\n> > > > > > > + \"Enable auto-focus if set to true, \"\n> > > > > > > + \"disable it if set to false\",\n> > > > > > > + FALSE, G_PARAM_WRITABLE);\n> > > > > > > + g_object_class_install_property(object_class, PROP_ENABLE_AUTO_FOCUS, spec2);\n> > > > > >\n> > > > > > Note, if you feel like it, you could make it more obvious by using\n> > > > > > g_steal_pointer() here, which is similar to std::move (but works for non C++\n> > > > > > pointers).\n> > > > > >\n> > > > > > > }\n> > > > > > >\n> > > > > > > static GstDevice *\n> > > > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp\n> > > > > > > b/src/gstreamer/gstlibcamerasrc.cpp\n> > > > > > > index a10cbd4f..672ea38a 100644\n> > > > > > > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > > > > > > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > > > > > > @@ -146,6 +146,7 @@ struct _GstLibcameraSrc {\n> > > > > > > GstTask *task;\n> > > > > > >\n> > > > > > > gchar *camera_name;\n> > > > > > > + gboolean enable_auto_focus = false;\n> > > > > >\n> > > > > > I guess I've been thinking C a lot so far, thanks for making me noticed we can\n> > > > > > do that. Note a little inconstancy, gboolean is an in. So I'd suggest:\n> > > > > >\n> > > > > > + bool enable_auto_focus = false;\n> > > > > >\n> > > > > > > GstLibcameraSrcState *state;\n> > > > > > > GstLibcameraAllocator *allocator;\n> > > > > > > @@ -154,7 +155,8 @@ struct _GstLibcameraSrc {\n> > > > > > >\n> > > > > > > enum {\n> > > > > > > PROP_0,\n> > > > > > > - PROP_CAMERA_NAME\n> > > > > > > + PROP_CAMERA_NAME,\n> > > > > > > + PROP_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,\n> > > > > > > [[maybe_unused]] GThread *thread,\n> > > > > > > gst_flow_combiner_add_pad(self->flow_combiner, srcpad);\n> > > > > > > }\n> > > > > > >\n> > > > > > > + if (self->enable_auto_focus) {\n> > > > > > > + const ControlInfoMap &infoMap = state->cam_->controls();\n> > > > > > > + if (infoMap.find(&controls::AfMode) != infoMap.end()) {\n> > > > > > > + state->initControls_.set(controls::AfMode, controls::AfModeContinuous);\n> > > > > > > + } else {\n> > > > > > > + GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > > > > > > + (\"Failed to enable auto focus\"),\n> > > > > > > + (\"AfMode not found in camera controls, \"\n> > > > > > > + \"please retry with 'enable-auto-focus=false'\"));\n> > > > > > > + }\n> > > > > > > + }\n> > > > > > > +\n> > > > > >\n> > > > > > Please resend in text form as you are suppose to (I recommend using git send-\n> > > > > > email). This will ensure we don't smash the indentation completely. I'll stop my\n> > > > > > review here, as its too hard to read without indentation.\n> > > > >\n> > > > > I strongly recommend using git-send-email to send patches as well.\n> > > > > Instructions are available at\n> > > > > https://libcamera.org/contributing.html#submitting-patches, which links\n> > > > > to the very nice https://git-send-email.io/ helper to setup\n> > > > > git-send-email.\n> > > > >\n> > > > > > > ret = state->cam_->start(&state->initControls_);\n> > > > > > > if (ret) {\n> > > > > > > GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > > > > > > @@ -659,6 +673,9 @@ gst_libcamera_src_set_property(GObject *object, guint\n> > > > > > > prop_id,\n> > > > > > > g_free(self->camera_name);\n> > > > > > > self->camera_name = g_value_dup_string(value);\n> > > > > > > break;\n> > > > > > > + case PROP_ENABLE_AUTO_FOCUS:\n> > > > > > > + self->enable_auto_focus = g_value_get_boolean(value);\n> > > > > > > + break;\n> > > > > > > default:\n> > > > > > > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > > > > > > break;\n> > > > > > > @@ -676,6 +693,9 @@ gst_libcamera_src_get_property(GObject *object, guint\n> > > > > > > prop_id, GValue *value,\n> > > > > > > case PROP_CAMERA_NAME:\n> > > > > > > g_value_set_string(value, self->camera_name);\n> > > > > > > break;\n> > > > > > > + case PROP_ENABLE_AUTO_FOCUS:\n> > > > > > > + g_value_set_boolean(value, self->enable_auto_focus);\n> > > > > > > + break;\n> > > > > > > default:\n> > > > > > > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > > > > > > break;\n> > > > > > > @@ -844,4 +864,11 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n> > > > > > > | G_PARAM_READWRITE\n> > > > > > > | G_PARAM_STATIC_STRINGS));\n> > > > > > > g_object_class_install_property(object_class, PROP_CAMERA_NAME, spec);\n> > > > > > > + GParamSpec *spec2 = g_param_spec_boolean(\"enable-auto-focus\",\n> > > > > > > + \"Enable auto-focus\",\n> > > > > > > + \"Enable auto-focus if set to true, \"\n> > > > > > > + \"disable it if set to false\",\n> > > > > > > + FALSE, G_PARAM_WRITABLE);\n> > > > > > > + g_object_class_install_property(object_class, PROP_ENABLE_AUTO_FOCUS, spec2);\n> > > > > > > +\n> > > > > > > }\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 886CAC31E9\n\tfor ;\n\tThu, 1 Jun 2023 07:39:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F1EE3626FA;\n\tThu, 1 Jun 2023 09:39:23 +0200 (CEST)","from mail-yb1-xb2e.google.com (mail-yb1-xb2e.google.com\n\t[IPv6:2607:f8b0:4864:20::b2e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F203D61EA6\n\tfor ;\n\tThu, 1 Jun 2023 09:39:22 +0200 (CEST)","by mail-yb1-xb2e.google.com with SMTP id\n\t3f1490d57ef6-bad05c6b389so555132276.2\n\tfor ;\n\tThu, 01 Jun 2023 00:39:22 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685605164;\n\tbh=XObc71niI++qot1SsFWsf/In4u7c/eJ2Hdgx9N65e+U=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=gpYmUaZdrNT5L8n7CqrSkxiaBeUDKPygVEi+gkwWh04JYeNs1UZ0FhavebEL0qwFH\n\tTHc2vAd1QgAuJSZH29PXcmRPJtUDT3vQN6EgjPelUf+Wlhce3nA6IDQBC7884ofuw1\n\tDugE3n+b84RuRclPdqwoJFtZOaaVw5/uaqJGsSzHb5xIjQnNzs7JMg8MtVahQa+WVG\n\tYe+9JhnS3JGjLvFHgXO9gzfBEOFfc7wiYeIG+EwQ+PPPYUajs6ZPCGsSmaApHMMSxI\n\tPFszJNp0Ygg8t9g/gY2T8de4EN/IothqyIdE6rW+KX1zaMZdIy/xhIb0RWd86/Myso\n\tHA1MPoADfVKOw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1685605162; x=1688197162;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=foXu9GdbvkIWdgGkiTeZV7rwatAh8hn//5kKnRWv7Lk=;\n\tb=qG8KBzHMSaOWZJUCVCeNKw4uXXywXHMemfTcETZanxO5kLmGibifKfpJLoxHjbz2gU\n\tKYxVYSOfuziL9U7lv1mkxItOX3oFLo9+drQWEyc9kF0LrTBOAacgcVcoYo/fOBu0uSkS\n\tLpqB0VFKk7Fby8RO9Oeo/b/QR4UoxM/clInMPDRUjvyf8RA1WrTeivDqWtyBK3qfl6Nz\n\t8+At/2EktMJ5jnzUzHjASMN+dgXpj3D8tWsozbblLyneS33lFbhNWBLh631VHmzb+XtR\n\tKLtrCyqy7FxmzcJAfi4XY8rO9ooHZ5yjQS0zhJ6HT/vBPINWFWpX4cFUZkfICe01mP0v\n\tFlSw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"qG8KBzHM\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1685605162; x=1688197162;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=foXu9GdbvkIWdgGkiTeZV7rwatAh8hn//5kKnRWv7Lk=;\n\tb=TbtI8sZrSelMnOecUlFjOQF44bozNBy+QkkPdoloOc1zIXxyDbRUHq6CiTEmqlsTzJ\n\tIxooI3fMyuhDvkjEkjeBwmUPlPfZDgvecuU+dkkGtcQ/r0Fo28fKpGdwxBEpo6EEdoL4\n\tZ8cAU6RS3SPnSmhSrM92goxkBBKkVdi8k/uAIbFbSlzXQkxjC9IO1fnrMObSCe+vhhHK\n\tmm5DNe+4BrXTfRwLof5aVN0+Oy2cz9Bgr7sVGSmL/FuZtu+ECMLgidi8a6k0UOeu261/\n\tytV6LpuAtPEEn2O/88zUNWm7hc7WVTpuQJUnMlIaCKi2ujPeljratuq1KEz4xi2NeyxC\n\toV4g==","X-Gm-Message-State":"AC+VfDz8p7xpIgt26x1CGEWahYvGe6PdS9Mw5+T7EmZaZ1SrfIBrIXGJ\n\tEJKyLBkebOMnxYlFODOXi8ZY4EQPOL7FaaXSQS3VJLtam/QJvKHu5l8=","X-Google-Smtp-Source":"ACHHUZ5G2od0jn8Bg3gs2l52t0NqcZKur2whg3ti/z1Tklbk/lGwsjSoyYLNMhCxKSpIvAcSmw5fD8o+Y/6LpgvWtNE=","X-Received":"by 2002:a81:468a:0:b0:569:2868:731a with SMTP id\n\tt132-20020a81468a000000b005692868731amr614101ywa.36.1685605161759;\n\tThu, 01 Jun 2023 00:39:21 -0700 (PDT)","MIME-Version":"1.0","References":"\n\t\n\t<20230531150629.GC24749@pendragon.ideasonboard.com>\n\t\n\t<20230531155248.GF24749@pendragon.ideasonboard.com>\n\t\n\t<20230601072857.GC22609@pendragon.ideasonboard.com>","In-Reply-To":"<20230601072857.GC22609@pendragon.ideasonboard.com>","Date":"Thu, 1 Jun 2023 08:39:07 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH] Add enable_auto_focus option to the\n\tGStreamer 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":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"Cedric Nugteren ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27211,"web_url":"https://patchwork.libcamera.org/comment/27211/","msgid":"<27ec35f78ffc653c5273d90cbca69c2f8088f713.camel@ndufresne.ca>","date":"2023-06-01T16:24:22","subject":"Re: [libcamera-devel] [PATCH] Add enable_auto_focus option to the\n\tGStreamer plugin","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Le jeudi 01 juin 2023 à 08:39 +0100, Naushir Patuck a écrit :\n> > If you have any idea what the default should be in the GStreamer\n> > element, please feel free to chime in :-)\n> \n> Perhaps GStreamer should do nothing with focus by default and rely on the IPA\n> default behaviour (i.e. go to hyperfocal)?  This then relies on the\n> application\n> to choose what it wants to do on startup, just like any other application that\n> directly uses libcamera.\n\nTo reassure you, it can't be worse then what most UVC camera do today. Having a\ndefault matching this would help consistency though (or being similarly bad from\nsome point of view). Later, on top, there should be ways to move to a more\nmanual control in focus aware Gst applications.\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 6D510C3200\n\tfor ;\n\tThu, 1 Jun 2023 16:24:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C874162728;\n\tThu, 1 Jun 2023 18:24:26 +0200 (CEST)","from mail-qk1-x730.google.com (mail-qk1-x730.google.com\n\t[IPv6:2607:f8b0:4864:20::730])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8DA866029F\n\tfor ;\n\tThu, 1 Jun 2023 18:24:25 +0200 (CEST)","by mail-qk1-x730.google.com with SMTP id\n\taf79cd13be357-75b0df7b225so93984785a.1\n\tfor ;\n\tThu, 01 Jun 2023 09:24:25 -0700 (PDT)","from nicolas-tpx395.localdomain ([2606:6d00:11:5f2f::7a9])\n\tby smtp.gmail.com with ESMTPSA id\n\tbd15-20020ad4568f000000b00628563d4441sm861848qvb.66.2023.06.01.09.24.23\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 01 Jun 2023 09:24:23 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685636666;\n\tbh=J0hqn2XD3I4syegNNfIr/DvWwFnDThQNhyUx4FTfSyw=;\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=AfXIM4sYnlDm2n/955x/q/jF7Dr1eFYDsONYlDige6/sGH8PQsa8SA5qGkecGehtx\n\tt7Uw03R85QtSOdBxwO3iDOphFzz2HQWTXK6+SpQN08/0cibnRKYMTu+/3ozqg3zz2w\n\t7t3oXZU1CzCiC0JzZg7uLLCx9td7GHFDGTdZNCtm7MLQi5RV9RCgVkCxv4wE5QgxjE\n\tKE9k69vNc6OdaY85M1Y4FnhRfGW0dJLQ0Z6HPf3iszDQOBQlJnAB2VETFJHctn39Ck\n\tZQ7qtDSXP9S7L4uVie+xji19tcuqzTBGDlI4Nr+H5cfYO+a2mrLoHv6UgR72zWtTOh\n\tQVUpEyEWgVwLA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20221208.gappssmtp.com; s=20221208; t=1685636664;\n\tx=1688228664; \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=J0hqn2XD3I4syegNNfIr/DvWwFnDThQNhyUx4FTfSyw=;\n\tb=F3QhJPBi8esC7X7j+Sc0DLQD8bud1Hz05Xgyb2IQCOk6yZwNIxe8k749jJas/juVWS\n\tTvzTjOA2WXjP1uYVMyUc2RC9i5Ba5srx1Jxun8A6Bi9u8xCxZ5t1DvQx1MplBJWT00ot\n\t7fDEm+LNZdK7hc8hc3xErSNKszTmM4obnJ1SjPDombp25bYDlzqd3werk0Yw91STFv16\n\tKe8tD1gNZQhSv+QufqYb6uc2f3Kl9OB9p7B8ppLsVPjpsmqgeoPBlnNRPbIvB7dhNNhI\n\tndT51VNc4H+plnbCnOIuVz/xm9Os7nJqz1O470U9D/5spmX/FO/um1GbORf1re5Jo89E\n\twGSA=="],"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=\"F3QhJPBi\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1685636664; x=1688228664;\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=J0hqn2XD3I4syegNNfIr/DvWwFnDThQNhyUx4FTfSyw=;\n\tb=PxRc7q9QSILliN7crXYaHgvdohNT7U81WJc5F+WrFxZGVHkvEtzOwECncir0SmP5vB\n\tkmHIj3gw947BO97Ek5LX7XaGiZ06SQ1VqAwUovOtrxg3W3NT4sehCDWTfN5XkFxZbRK6\n\tuptiKnzmrVCOTTuWi0y9b2Ip6eGxHu7851gSKfI0H1b6+uWnTJGM9UNCeVW+wpfPctLK\n\tbxcQ+MKn3vfzIDSRHtXx6jptSQP4DkzGoUZZbBC08XIMN4Twnss7hFe4+FinitfmW0CA\n\t44TNACZBfl46qo+sycL51VF5sYwLZ2UUNKxVKy4DJ6wzn3MzpGDknGT6Qji/SxR2aclV\n\tGYhQ==","X-Gm-Message-State":"AC+VfDzEBq6c3c76+6omQEOkDNi1+T5osHmt9m9nU7Lj4CcPap9oWG26\n\tp/VyHqgcUWSTUaAblqQeONZVJQ==","X-Google-Smtp-Source":"ACHHUZ6QbabdMEdpjHVIcMYniDx/KDTEbmvNxLNLTb6uaYGrtoQWF8i9wO+e05mEK6egDJGz+W0hkQ==","X-Received":"by 2002:ad4:5ce1:0:b0:621:7d4:e068 with SMTP id\n\tiv1-20020ad45ce1000000b0062107d4e068mr14028539qvb.52.1685636664321; \n\tThu, 01 Jun 2023 09:24:24 -0700 (PDT)","Message-ID":"<27ec35f78ffc653c5273d90cbca69c2f8088f713.camel@ndufresne.ca>","To":"Naushir Patuck , Laurent Pinchart\n\t","Date":"Thu, 01 Jun 2023 12:24:22 -0400","In-Reply-To":"","References":"\n\t\n\t<20230531150629.GC24749@pendragon.ideasonboard.com>\n\t\n\t<20230531155248.GF24749@pendragon.ideasonboard.com>\n\t\n\t<20230601072857.GC22609@pendragon.ideasonboard.com>\n\t","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] Add enable_auto_focus option to the\n\tGStreamer 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27213,"web_url":"https://patchwork.libcamera.org/comment/27213/","msgid":"<20230602051938.GO22609@pendragon.ideasonboard.com>","date":"2023-06-02T05:19:38","subject":"Re: [libcamera-devel] [PATCH] Add enable_auto_focus option to the\n\tGStreamer plugin","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Jun 01, 2023 at 12:24:22PM -0400, Nicolas Dufresne wrote:\n> Le jeudi 01 juin 2023 à 08:39 +0100, Naushir Patuck a écrit :\n> > > If you have any idea what the default should be in the GStreamer\n> > > element, please feel free to chime in :-)\n> > \n> > Perhaps GStreamer should do nothing with focus by default and rely on the IPA\n> > default behaviour (i.e. go to hyperfocal)?  This then relies on the application\n> > to choose what it wants to do on startup, just like any other application that\n> > directly uses libcamera.\n> \n> To reassure you, it can't be worse then what most UVC camera do today. Having a\n> default matching this would help consistency though (or being similarly bad from\n> some point of view). Later, on top, there should be ways to move to a more\n> manual control in focus aware Gst applications.\n\nHmmmm... I'm really in two minds here.\n\nFirst of all, we can't expect the same default behaviour for all cameras\nwhen it comes to auto-focus, simply because many cameras don't have a\ncontrollable focus lens, and some (I'm thinking about UVC devices here)\nmay not have the ability to turn auto-focus on. Still, implementing\nconsistent defaults where possible is a goal I highly value, in order to\nmaximize application portability.\n\nWhen it comes to the libcamera core, setting the focus lens to the\nhyperfocal distance and disabling auto-focus seems like a good default\nto me. Different use cases would favour different defaults, and I don't\nthink there's one particular use case that could be considered so\nprevalent that it would call for enabling continuous auto-focus by\ndefault. Different defaults could however be implemented in different\nlayers. For instance, I could imagine PipeWire deciding to enabling\ncontinuous auto-focus by default on desktop or mobile systems.\n\nFor libcamerasrc, I don't know if the same reasoning should lead to the\nsame result, or if we should consider that the vast majority of use\ncases for the GStreamer element call for enabling auto-focus by default.\nI currently side towards doing the same as in the libcamera core, as I\nbelieve it would be confusing for users to have different default\nbehaviours when using libcamera through different adaptation layers.\nHowever, I could also imagine enabling auto-focus by default in the V4L2\nadaptation layer for instance, as that layer is mostly meant to expose\nlibcamera-based cameras as webcams.","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 8E48AC3200\n\tfor ;\n\tFri, 2 Jun 2023 05:19:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D9F8861EA5;\n\tFri, 2 Jun 2023 07:19:41 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7BF4261EA5\n\tfor ;\n\tFri, 2 Jun 2023 07:19:40 +0200 (CEST)","from pendragon.ideasonboard.com (om126156168104.26.openmobile.ne.jp\n\t[126.156.168.104])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0BF5D83F;\n\tFri, 2 Jun 2023 07:19:16 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685683181;\n\tbh=/LuAm0jPysuUF46cg028vIOwA3tHlw1WUzLoc7wFXPk=;\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=jA5vRfb/pBhmz5QlxwO45aqX6sfFdy/GmbQMZV4NEKAmrqtiQDg6nuQr/PlqdRulV\n\t+t9cdWmjvG0UFO6SOCpI5bSU9zxAhgIYJye3iyvaKDmjZpSNVZisf70cOaYasaYPzd\n\t95pi8YB5CRidUDHA74h5YAjbw4pPwGQ2creofFbxdqWcG1FwwwaacqUkv7XIUBe9tH\n\tKeh++a9EDU3zb1//p0s1XKOZvtjYT3cwBdIqMO2kMqk10DcaE/5pbq7DR+DVTLKc71\n\tapzabjwcdmbXRBN6Hl09OhYZfP7xiwldjhIcuKBwI/4tRhczeDOQFpthIlaQEkRk5Y\n\t5lIv8yWtMKEjg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685683158;\n\tbh=/LuAm0jPysuUF46cg028vIOwA3tHlw1WUzLoc7wFXPk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=c7YuGQPMQenWN1TFKY7BU2yvxulF8s/TA7M2A1maNXUX+//VEPDsOucolWojUXU5U\n\tImgN6i2c0Fy/Cba/xeHP1u3wkTD+i5ijuceD0IqGx3Wwci/ce4sSJYVZZneoDziO/5\n\t0ch+jSjmPfIW8foSzNlGZzEbmV7Ah0ao4dy9zE20="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"c7YuGQPM\"; dkim-atps=neutral","Date":"Fri, 2 Jun 2023 08:19:38 +0300","To":"Nicolas Dufresne ","Message-ID":"<20230602051938.GO22609@pendragon.ideasonboard.com>","References":"\n\t\n\t<20230531150629.GC24749@pendragon.ideasonboard.com>\n\t\n\t<20230531155248.GF24749@pendragon.ideasonboard.com>\n\t\n\t<20230601072857.GC22609@pendragon.ideasonboard.com>\n\t\n\t<27ec35f78ffc653c5273d90cbca69c2f8088f713.camel@ndufresne.ca>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<27ec35f78ffc653c5273d90cbca69c2f8088f713.camel@ndufresne.ca>","Subject":"Re: [libcamera-devel] [PATCH] Add enable_auto_focus option to the\n\tGStreamer 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27226,"web_url":"https://patchwork.libcamera.org/comment/27226/","msgid":"","date":"2023-06-02T13:53:03","subject":"Re: [libcamera-devel] [PATCH] Add enable_auto_focus option to the\n\tGStreamer plugin","submitter":{"id":164,"url":"https://patchwork.libcamera.org/api/people/164/","name":"Cedric Nugteren","email":"cedric@plumerai.com"},"content":"I have fixed the review comments and have installed and configured git\nsend-email. However, apparently it started a new thread, I don't think that\nis what it should have done:\nhttps://lists.libcamera.org/pipermail/libcamera-devel/2023-May/037925.html\n(I used \"git send-email --in-reply-to=037947 --to=\nlibcamera-devel@lists.libcamera.org patch_file.patch\")\n\nAlso, you mentioned that would like to see the original patch better\nformatted. Should I just create a whole new patch altogether?\n\nFinally, about the default/non-default discussion, I was trying to be\nconservative here, because when I opened the original bug (#188), the reply\nI got sounded like you thought it was a missing feature rather than a bug.\nFor me this is a blocking bug: I can't use the PiCamV3 at all due to random\nfocus behaviour. So therefore I opted for keeping the default behaviour as\nit was before to make it more likely that this patch will be accepted.\n\nOn Fri, Jun 2, 2023 at 7:19 AM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> On Thu, Jun 01, 2023 at 12:24:22PM -0400, Nicolas Dufresne wrote:\n> > Le jeudi 01 juin 2023 à 08:39 +0100, Naushir Patuck a écrit :\n> > > > If you have any idea what the default should be in the GStreamer\n> > > > element, please feel free to chime in :-)\n> > >\n> > > Perhaps GStreamer should do nothing with focus by default and rely on\n> the IPA\n> > > default behaviour (i.e. go to hyperfocal)? This then relies on the\n> application\n> > > to choose what it wants to do on startup, just like any other\n> application that\n> > > directly uses libcamera.\n> >\n> > To reassure you, it can't be worse then what most UVC camera do today.\n> Having a\n> > default matching this would help consistency though (or being similarly\n> bad from\n> > some point of view). Later, on top, there should be ways to move to a\n> more\n> > manual control in focus aware Gst applications.\n>\n> Hmmmm... I'm really in two minds here.\n>\n> First of all, we can't expect the same default behaviour for all cameras\n> when it comes to auto-focus, simply because many cameras don't have a\n> controllable focus lens, and some (I'm thinking about UVC devices here)\n> may not have the ability to turn auto-focus on. Still, implementing\n> consistent defaults where possible is a goal I highly value, in order to\n> maximize application portability.\n>\n> When it comes to the libcamera core, setting the focus lens to the\n> hyperfocal distance and disabling auto-focus seems like a good default\n> to me. Different use cases would favour different defaults, and I don't\n> think there's one particular use case that could be considered so\n> prevalent that it would call for enabling continuous auto-focus by\n> default. Different defaults could however be implemented in different\n> layers. For instance, I could imagine PipeWire deciding to enabling\n> continuous auto-focus by default on desktop or mobile systems.\n>\n> For libcamerasrc, I don't know if the same reasoning should lead to the\n> same result, or if we should consider that the vast majority of use\n> cases for the GStreamer element call for enabling auto-focus by default.\n> I currently side towards doing the same as in the libcamera core, as I\n> believe it would be confusing for users to have different default\n> behaviours when using libcamera through different adaptation layers.\n> However, I could also imagine enabling auto-focus by default in the V4L2\n> adaptation layer for instance, as that layer is mostly meant to expose\n> libcamera-based cameras as webcams.\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 374A4C31E9\n\tfor ;\n\tFri, 2 Jun 2023 13:53:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 97E1861EA8;\n\tFri, 2 Jun 2023 15:53:17 +0200 (CEST)","from mail-vs1-xe2b.google.com (mail-vs1-xe2b.google.com\n\t[IPv6:2607:f8b0:4864:20::e2b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0769161EA8\n\tfor ;\n\tFri, 2 Jun 2023 15:53:16 +0200 (CEST)","by mail-vs1-xe2b.google.com with SMTP id\n\tada2fe7eead31-438a5069d58so523034137.0\n\tfor ;\n\tFri, 02 Jun 2023 06:53:15 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685713997;\n\tbh=so4iFpECu38iM2S9e9Y3Ckzwuk8Kf6oRDX8HYDm0vEg=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=CnghAIWE//xJAR+S8RWLUkt+V3AlE1edtnHjNchQRzi7pRp4xOUlsu6NDmUJVhDZ3\n\tuNTFS7p1o9/5OIw4jMLM9rbJBfcmuq8xD/lljZ9qA6L7nEt0C5DRQmX+h4LvLN9nk8\n\twaGZXLD4bi8DGROhN9Wj3CV7yI1X+Je6KIEFw3HcfB7RbZlo6dUHq5vqoTbHcbVmd4\n\t1yWxql+uXQ5Lh00GqMuammOgkr41fPbNAXT+p/ieY2AOD0UbPjMgWZcvK/VnhHu9tT\n\tQ6Pfv25j1DKpjbvGPEDAb3S1mpLTDD6DxPgjweldTywCrEDMVc3x9i9f0QvYpxqJkx\n\tkHe9pwU4tj1Qg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=plumerai.com; s=google; t=1685713995; x=1688305995;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=Kd/IduVQDc8hpg9N4+yV2+U5k/xUltZzcvHlqD58ykU=;\n\tb=SLT9O0inIgmEowzwqgT17eQdzyfqq1VXyApCTPL0fJOOxVMS6dtUMYCs3dVXv71N+d\n\t9JtgcGPh2ACiObCOqtMLCCyvnOzmDLDRqvduFbU/I2Qj4rxABeh+OipqiHWjfnuTbaRX\n\te1buk9wfI5G54C6zVrbw07ZIcXJHfQkjSDF0A="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=plumerai.com\n\theader.i=@plumerai.com header.b=\"SLT9O0in\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1685713995; x=1688305995;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=Kd/IduVQDc8hpg9N4+yV2+U5k/xUltZzcvHlqD58ykU=;\n\tb=OmjIefrLF/Db3mE8pC2kRPbCsdFrYkf54oTcWdW5qyw2rYtxT3Dmqw8Audv8HDdfLg\n\t5cQ2IjKAjHyt9m4Bnwo/K075xWnJOAfqA/RQZaV/eiF4pFuRdJ2bvuGqSFERELqZExMu\n\tV+vinxcZuSuf2ziCxuMXnwvrU7rtfFgUntY8TQDBu9bl+5YrYYvOhSEwMlL2O3a/0Joz\n\tJC9S41E6iShecfzHlx4QUOLpHc2q1uR7T/APELdlAXLoRE4NK2/kOLsK38V6RPMxKK+B\n\tufUxvag+a547UFwnHL8J+vTOGXAw2C1KYCYRUSgQh39olbsZZlrDPXveih4ifcMRBvR/\n\taiIQ==","X-Gm-Message-State":"AC+VfDxIKSlY8WBcui9zcAjf0z8aHbyxnm/FkVtJjkIHDbMUugJKx3TV\n\tieg1TAoK1ehzVVpWhO/K4FmxGi91aLDf6X/6sG3HMg==","X-Google-Smtp-Source":"ACHHUZ4wWGngEF4ge49grmERX2tkmEuSpyw9ujJI8VaVyC/Un6rB8ZEijSgxPznaloNDYF1yR8tkZXyW93BIWf2cOR8=","X-Received":"by 2002:a67:f1d2:0:b0:43b:1893:e41d with SMTP id\n\tv18-20020a67f1d2000000b0043b1893e41dmr1882297vsm.25.1685713994795;\n\tFri, 02 Jun 2023 06:53:14 -0700 (PDT)","MIME-Version":"1.0","References":"\n\t\n\t<20230531150629.GC24749@pendragon.ideasonboard.com>\n\t\n\t<20230531155248.GF24749@pendragon.ideasonboard.com>\n\t\n\t<20230601072857.GC22609@pendragon.ideasonboard.com>\n\t\n\t<27ec35f78ffc653c5273d90cbca69c2f8088f713.camel@ndufresne.ca>\n\t<20230602051938.GO22609@pendragon.ideasonboard.com>","In-Reply-To":"<20230602051938.GO22609@pendragon.ideasonboard.com>","Date":"Fri, 2 Jun 2023 15:53:03 +0200","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000da62da05fd25df12\"","Subject":"Re: [libcamera-devel] [PATCH] Add enable_auto_focus option to the\n\tGStreamer 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":"Cedric Nugteren via libcamera-devel\n\t","Reply-To":"Cedric Nugteren ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27231,"web_url":"https://patchwork.libcamera.org/comment/27231/","msgid":"<69251b734be018373b9c73ee15f2998900d12f6d.camel@ndufresne.ca>","date":"2023-06-02T16:10:44","subject":"Re: [libcamera-devel] [PATCH] Add enable_auto_focus option to the\n\tGStreamer plugin","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Hi Laurent,\n\nLe vendredi 02 juin 2023 à 08:19 +0300, Laurent Pinchart a écrit :\n> For libcamerasrc, I don't know if the same reasoning should lead to the\n> same result, or if we should consider that the vast majority of use\n> cases for the GStreamer element call for enabling auto-focus by default.\n> I currently side towards doing the same as in the libcamera core, as I\n> believe it would be confusing for users to have different default\n> behaviours when using libcamera through different adaptation layers.\n> However, I could also imagine enabling auto-focus by default in the V4L2\n> adaptation layer for instance, as that layer is mostly meant to expose\n> libcamera-based cameras as webcams.\n> \n\nI like to decide based on what the users get by default. A mimic of what a\nwebcam usually do seems logical. What do you get in the simplest form of usage\nof the software (in GStreamer this means if you just plug and start the element:\nlibcamerasrc ! ...).\n\nNot enabling it (or not setting the lense to a decent default position) on HW\nthat needs to be focused means most of the time you'll just get a bad output.\nThis impose more complexity on the application. Being a low level library does\nnot imply it have to be difficult across multiple type of cameras. Its no more\neffort to disable then to enable it either.\n\nOf course, this can either happen in libcamera or in libcamerasrc element, that\nI don't care so much. I don't have the full portrait on auto-focus quality atm\nto judge if now is the time either, and which type of auto focus we should\nenable vs which one we should definitely avoid as it would make things worst. I\nwould certainly not rush into \"getting something\" for that reason. But for sure,\nthe design principle for the gstreamer element (and same will apply for the\npipewire one) is that it goes from really simple to full control, and nothing\nspecial should be done to get a good image. This is mostly the reason why the\nelement assume you can always use the cameras with a single stream (hopefully\nthis will stay true ;-P). So assuming auto-focus feature actually works (I have\nno mean to verify), enabling it would mean it behaves similar to what webcam do.\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 70F7BC31E9\n\tfor ;\n\tFri, 2 Jun 2023 16:10:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EF7F262728;\n\tFri, 2 Jun 2023 18:10:47 +0200 (CEST)","from mail-qv1-xf2a.google.com (mail-qv1-xf2a.google.com\n\t[IPv6:2607:f8b0:4864:20::f2a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8C2DF61EA8\n\tfor ;\n\tFri, 2 Jun 2023 18:10:46 +0200 (CEST)","by mail-qv1-xf2a.google.com with SMTP id\n\t6a1803df08f44-628f267aa5aso5146876d6.1\n\tfor ;\n\tFri, 02 Jun 2023 09:10:46 -0700 (PDT)","from nicolas-tpx395.localdomain ([2606:6d00:11:5f2f::7a9])\n\tby smtp.gmail.com with ESMTPSA id\n\ti3-20020a0cd843000000b00621268e14efsm980076qvj.55.2023.06.02.09.10.44\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 02 Jun 2023 09:10:45 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685722248;\n\tbh=5wukUWp7+Qrss7T+MIRW+2oVaLkTk3KHzhYPrBZ56B0=;\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=jIKBl/drZLkqqtpdxJYx1uHEcREBUt1Btvqqfa5TpiMSR8ZylRMRWl6LP4SkkgiQd\n\tRSlUYt0Zt6cdkwtWs3LMOW4c8O4iK+G47mBrkHIZ2CM/RTkBnCiQnbjLKyvRu5Ihbf\n\t2VWcUyZIKCp7KHxeqCq87mV7b2Z0RZLavlzdU05ajiaVup6xS5Y5W8bO9IWT9FCIeE\n\tQBXC9a6USzLDk4p6Q3C1gtbjAfLetoRp+j95VJ2FMp7dlx9LvmDSBPd2tVf2d2k6SO\n\toIPVKkPBYK7aYit1gXkIYyX4xk3ZvLh4jNyzbYI7tHgQAWK2XBPjQTHCQS0pArljJ6\n\tHCXQBLmQsl0LA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20221208.gappssmtp.com; s=20221208; t=1685722245;\n\tx=1688314245; \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=5wukUWp7+Qrss7T+MIRW+2oVaLkTk3KHzhYPrBZ56B0=;\n\tb=z02J3gWcuaY7cUQM8Ua2JB6gM31xYklKKbWIufEWh1+pAJRPNwSz/B4H7N6IyLUOQ5\n\tYWqJBYsUebbUDwSjSBbGL8OEgt2lpKuCqbn95ejEvUw0DaDLqGOOkzQIt9ittFbkeq1h\n\tIM3twF6qYPwf3v3pAJuqxG2OyZPMPFTLaHtX73AJiHAfVHUseuoRhpesE493clsvo6+W\n\tODMyV7kHHVPjdtREL+Q4YgFUJNDDwOVXxKcZOT5ZqImbtbazuNKm0Pfbd0ioVX4kWQaG\n\tpm0Y38QNXgf0mFuN59zRalqddYVAfsgHInTXXe1UHRZ3A0JEXn9ZbdDrU17wCyKkOS7A\n\tLD5w=="],"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=\"z02J3gWc\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1685722245; x=1688314245;\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=5wukUWp7+Qrss7T+MIRW+2oVaLkTk3KHzhYPrBZ56B0=;\n\tb=DoG++damDXdl+mLj17PvFxBm9vUU+RKQB6C7JpaASYKO8fy0pjOXrbBiYLsxNyO26p\n\t92erXZNxG7BKAthQv+6FiD8wjaWpmj6syYetSoaQLdSUqDieR6jT3MkNwmN9Vypf3XKw\n\t8Eo2XwkRIv+gJAoFILSVGxT4iiiF+v5JALRCscNO0WbL3RWhS/r6P34kEhZwa7psH8TV\n\tA1dbaHT2KqT63OnODoO4EauFhvLWp/hChqcZ5wiIIxpanQjZntVrPO3/WUiXBfj324XW\n\tl//Jy7iUM1W8C5jcvLm2cQX9OAMspuSP5GDNKXm7Ff7FAZVicbC84VyDLsuILxqKICQd\n\tHZcA==","X-Gm-Message-State":"AC+VfDxjZQ3eyJw1fmmBXWaG6zbtvKgkTEypQJldft8Fd9rVPmYD4c4k\n\tvstw9vzAko74QbTIPy4GZ7MlUA==","X-Google-Smtp-Source":"ACHHUZ4dCI3GkLH2pS7NvFmEfPW3xZJ0TQcx63fo9Bcqw00yIFOLNcFpBVqvABaV+oULVLJShJFTeQ==","X-Received":"by 2002:a05:6214:e62:b0:626:199e:1b7d with SMTP id\n\tjz2-20020a0562140e6200b00626199e1b7dmr13051980qvb.61.1685722245402; \n\tFri, 02 Jun 2023 09:10:45 -0700 (PDT)","Message-ID":"<69251b734be018373b9c73ee15f2998900d12f6d.camel@ndufresne.ca>","To":"Laurent Pinchart ","Date":"Fri, 02 Jun 2023 12:10:44 -0400","In-Reply-To":"<20230602051938.GO22609@pendragon.ideasonboard.com>","References":"\n\t\n\t<20230531150629.GC24749@pendragon.ideasonboard.com>\n\t\n\t<20230531155248.GF24749@pendragon.ideasonboard.com>\n\t\n\t<20230601072857.GC22609@pendragon.ideasonboard.com>\n\t\n\t<27ec35f78ffc653c5273d90cbca69c2f8088f713.camel@ndufresne.ca>\n\t<20230602051938.GO22609@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] Add enable_auto_focus option to the\n\tGStreamer 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27244,"web_url":"https://patchwork.libcamera.org/comment/27244/","msgid":"<20230605073310.GG22604@pendragon.ideasonboard.com>","date":"2023-06-05T07:33:10","subject":"Re: [libcamera-devel] [PATCH] Add enable_auto_focus option to the\n\tGStreamer plugin","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Cedric,\n\nOn Fri, Jun 02, 2023 at 03:53:03PM +0200, Cedric Nugteren wrote:\n> I have fixed the review comments and have installed and configured git\n> send-email.\n\nThank you. That appears to have gone well as your new version hasn't\nbeen mangled by mail clients and/or servers :-)\n\n> However, apparently it started a new thread, I don't think that\n> is what it should have done:\n> https://lists.libcamera.org/pipermail/libcamera-devel/2023-May/037925.html\n> (I used \"git send-email --in-reply-to=037947 --to=\n> libcamera-devel@lists.libcamera.org patch_file.patch\")\n\nThe --in-reply-to argument takes a Message-ID. The 037947 number you've\nused is the index from the mailing list archives. The Message-ID of your\noriginal e-mail was\nCAM6JzGDQMvPaOJfvUGy286PhM5oUvyxSkWWq8YFvmKoW1T0Rfw@mail.gmail.com (you\ncan find it by checking the mail headers in your sent folder, or in\npatchwork at https://patchwork.libcamera.org/patch/18651/).\n\nThis being said, new versions of a patch or patch series are not\nsupposed to be sent as a replies to the previous version, so all is fine\n:-)\n\n> Also, you mentioned that would like to see the original patch better\n> formatted. Should I just create a whole new patch altogether?\n\nWhen submitting a new version of a patch, you should, well, submit a new\nversion of the patch :-) The patch you have sent, titled \"[PATCH] Apply\nreview suggestions\" is not a new version of the original patch, but a\nset of changes that apply on top. As the original patch won't be merged,\nyou should instead send a v2 stemming from v1 and incorporating the\nchanges requested during review.\n\nIn git's terms, that means that the changes you've made to v1 should be\nsquashed into the original commit (`git commit --amend`), not committed\nas a separate commit. When generating the patch with git-format-patch,\nyou can pass the `-v2` argument to record the version number in the\nsubject line (you can also do so manually by editing the patch file, but\nthat's more error-prone).\n\nA nice thing to do when sending new versions is to include a changelog.\nSee https://patchwork.libcamera.org/patch/18699/ for instance, and note\nthe 'Changes since v1:' section under the '---'. This is something you\ncan include directly in the commit message in your git tree, by adding a\n'---' marker below the tags (Signed-off-by & co.), and recording the\nchangelog there. This helps reviewers understanding what has changed\nbetween patch versions.\n\nI've marked \"[PATCH] Apply review suggestions\" as \"changes requested\" in\npatchwork. Could you submit a real v2 of this patch ?\n\n> Finally, about the default/non-default discussion, I was trying to be\n> conservative here, because when I opened the original bug (#188), the reply\n> I got sounded like you thought it was a missing feature rather than a bug.\n> For me this is a blocking bug: I can't use the PiCamV3 at all due to random\n> focus behaviour. So therefore I opted for keeping the default behaviour as\n> it was before to make it more likely that this patch will be accepted.\n\nI'm tempted to default to disabling auto-focus. The discussion is still\nongoing though, but that shouldn't block you. You can send a v2 with\nauto-focus disabled by default, and we can easily switch the default on\ntop if desired.\n\n> On Fri, Jun 2, 2023 at 7:19 AM Laurent Pinchart wrote:\n> > On Thu, Jun 01, 2023 at 12:24:22PM -0400, Nicolas Dufresne wrote:\n> > > Le jeudi 01 juin 2023 à 08:39 +0100, Naushir Patuck a écrit :\n> > > > > If you have any idea what the default should be in the GStreamer\n> > > > > element, please feel free to chime in :-)\n> > > >\n> > > > Perhaps GStreamer should do nothing with focus by default and rely on the IPA\n> > > > default behaviour (i.e. go to hyperfocal)? This then relies on the application\n> > > > to choose what it wants to do on startup, just like any other application that\n> > > > directly uses libcamera.\n> > >\n> > > To reassure you, it can't be worse then what most UVC camera do today. Having a\n> > > default matching this would help consistency though (or being similarly bad from\n> > > some point of view). Later, on top, there should be ways to move to a more\n> > > manual control in focus aware Gst applications.\n> >\n> > Hmmmm... I'm really in two minds here.\n> >\n> > First of all, we can't expect the same default behaviour for all cameras\n> > when it comes to auto-focus, simply because many cameras don't have a\n> > controllable focus lens, and some (I'm thinking about UVC devices here)\n> > may not have the ability to turn auto-focus on. Still, implementing\n> > consistent defaults where possible is a goal I highly value, in order to\n> > maximize application portability.\n> >\n> > When it comes to the libcamera core, setting the focus lens to the\n> > hyperfocal distance and disabling auto-focus seems like a good default\n> > to me. Different use cases would favour different defaults, and I don't\n> > think there's one particular use case that could be considered so\n> > prevalent that it would call for enabling continuous auto-focus by\n> > default. Different defaults could however be implemented in different\n> > layers. For instance, I could imagine PipeWire deciding to enabling\n> > continuous auto-focus by default on desktop or mobile systems.\n> >\n> > For libcamerasrc, I don't know if the same reasoning should lead to the\n> > same result, or if we should consider that the vast majority of use\n> > cases for the GStreamer element call for enabling auto-focus by default.\n> > I currently side towards doing the same as in the libcamera core, as I\n> > believe it would be confusing for users to have different default\n> > behaviours when using libcamera through different adaptation layers.\n> > However, I could also imagine enabling auto-focus by default in the V4L2\n> > adaptation layer for instance, as that layer is mostly meant to expose\n> > libcamera-based cameras as webcams.","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 9AFB3C31E9\n\tfor ;\n\tMon, 5 Jun 2023 07:33:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1B1B161EA2;\n\tMon, 5 Jun 2023 09:33:13 +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 8C12B61EA2\n\tfor ;\n\tMon, 5 Jun 2023 09:33:11 +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 16EB32BC;\n\tMon, 5 Jun 2023 09:32:45 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685950393;\n\tbh=/mJeyAiVQXRruZ7PQIQU4kcdSlKS8cljwruSqztk5O0=;\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=KpqLIMYDJeiMVHunUEf8wh6BIzEx84yzrlqh2PnZfFISFWVtSHYDpYRBxV1fjDKR8\n\tqR/Rx54vgEz7CQuVH3dxQyoySYRa2FPtg4cKkKVWXwo0A1g06BLsRTLjy6n2TdSHYI\n\tTsIXROgqTX5I8tKSHkhpi+1f4GcFyRZx6/wA9Rx+/SR/ItVrhDje+ACifSm7hrVzYl\n\tjkO8O8VhW5sf6kNfSVBNKfDAGwnlRD4FZ+EtZxMnXUoKOTZtzw6dedlY9FecpSx65/\n\teyu9XSU9vrG9uV9TLITkFJqbGaDfib5mPku5whREHn1P7D1Nc5xaz1LIpCwqQUwxlE\n\tUk7JaKGkW107Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685950367;\n\tbh=/mJeyAiVQXRruZ7PQIQU4kcdSlKS8cljwruSqztk5O0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=A4BCuHcVM7relgvzWwjzEm7v+76DA6ccCOiN0oP2ZRdT1zV2S+sOuoVw1mdIBKHJZ\n\tC1pzhXl/n0JdLVOpNYCNyHSQ0wmCDuoahgWsUgsUIKngz9oF6d5KxhD571YaHeEAu2\n\tfdXGLBsk/N8kSu5V3kW9c/QRmqH8VZZENtTKsO0o="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"A4BCuHcV\"; dkim-atps=neutral","Date":"Mon, 5 Jun 2023 10:33:10 +0300","To":"Cedric Nugteren ","Message-ID":"<20230605073310.GG22604@pendragon.ideasonboard.com>","References":"\n\t<20230531150629.GC24749@pendragon.ideasonboard.com>\n\t\n\t<20230531155248.GF24749@pendragon.ideasonboard.com>\n\t\n\t<20230601072857.GC22609@pendragon.ideasonboard.com>\n\t\n\t<27ec35f78ffc653c5273d90cbca69c2f8088f713.camel@ndufresne.ca>\n\t<20230602051938.GO22609@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] Add enable_auto_focus option to the\n\tGStreamer 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]