[{"id":27415,"web_url":"https://patchwork.libcamera.org/comment/27415/","msgid":"","date":"2023-06-22T15:25:00","subject":"Re: [libcamera-devel] [PATCH] gstreamer: Maintain a control list to\n\ttrack libcamerasrc control properties","submitter":{"id":164,"url":"https://patchwork.libcamera.org/api/people/164/","name":"Cedric Nugteren","email":"cedric@plumerai.com"},"content":"I tested this on the PiCamV3 and the auto-focus option still seems to work\nindeed.\n\nCedric\n\nOn Thu, Jun 22, 2023 at 1:17 PM Umang Jain \nwrote:\n\n> As more and more libcamera controls get plumbed, more member variables\n> get introduced in struct GstLibcameraSrc. Instead of doing that, now\n> maintain a single ControlList which is more appropriate to keep track\n> of controls that get sets on libcamerasrc.\n>\n> This also brings easy validation of controls set on libcamerasrc. If\n> the controls are not supported by camera, abort early and report what is\n> not supported.\n>\n> The current patch migrates previously introduced control,\n> auto-focus-mode, to be set directly in control list.\n>\n> Signed-off-by: Umang Jain \n> ---\n> Cedric, Can you please help test this?\n>\n> (I'm travelling so have tested this on my RPi and inferring from\n> libcamera/gstreamer logs, it seems to have not broken anything)\n>\n> An additional test from your side would be appreciated.\n> ---\n> src/gstreamer/gstlibcamerasrc.cpp | 45 ++++++++++++++++++++++---------\n> src/gstreamer/gstlibcamerasrc.h | 6 ++---\n> 2 files changed, 36 insertions(+), 15 deletions(-)\n>\n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp\n> b/src/gstreamer/gstlibcamerasrc.cpp\n> index 47d8ff43..a2266310 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -146,7 +146,7 @@ struct _GstLibcameraSrc {\n> GstTask *task;\n>\n> gchar *camera_name;\n> - controls::AfModeEnum auto_focus_mode = controls::AfModeManual;\n> + ControlList *controls;\n>\n> GstLibcameraSrcState *state;\n> GstLibcameraAllocator *allocator;\n> @@ -462,7 +462,9 @@ gst_libcamera_src_task_enter(GstTask *task,\n> [[maybe_unused]] GThread *thread,\n> GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n> GLibRecLocker lock(&self->stream_lock);\n> GstLibcameraSrcState *state = self->state;\n> + const ControlInfoMap &info_map = state->cam_->controls();\n> GstFlowReturn flow_ret = GST_FLOW_OK;\n> + gboolean supported_ctrl = true;\n> gint ret;\n>\n> g_autoptr(GstStructure) element_caps =\n> gst_structure_new_empty(\"caps\");\n> @@ -579,18 +581,31 @@ 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->auto_focus_mode != controls::AfModeManual) {\n> - const ControlInfoMap &infoMap = state->cam_->controls();\n> - if (infoMap.find(&controls::AfMode) != infoMap.end()) {\n> - state->initControls_.set(controls::AfMode,\n> self->auto_focus_mode);\n> - } else {\n> + /*\n> + * Check the controls set on libcamerasrc is supported by the\n> camera.\n> + * This can be easily done by CameraControlValidator but it is an\n> internal\n> + * libcamera class. Hence avoid adding a internal header even\n> though\n> + * gstreamer links with libcamera-private.\n> + */\n> + for (auto iter = self->controls->begin(); iter !=\n> self->controls->end(); iter++) {\n> + if (info_map.find(iter->first) == info_map.end()) {\n> + const ControlIdMap *id_map =\n> self->controls->idMap();\n> GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> - (\"Failed to enable auto focus\"),\n> - (\"AfMode not supported by this\n> camera, \"\n> - \"please retry with\n> 'auto-focus-mode=AfModeManual'\"));\n> + (\"Failed to set %s\",\n> id_map->at(iter->first)->name().c_str()),\n> + (\"%s not supported by this\n> camera, \", id_map->at(iter->first)->name().c_str()));\n> + supported_ctrl = false;\n> + break;\n> }\n> }\n>\n> + if (!supported_ctrl) {\n> + gst_task_stop(task);\n> + flow_ret = GST_FLOW_NOT_NEGOTIATED;\n> + goto done;\n> + }\n> +\n> + state->initControls_.merge(*self->controls);\n> +\n> ret = state->cam_->start(&state->initControls_);\n> if (ret) {\n> GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> @@ -674,7 +689,8 @@ gst_libcamera_src_set_property(GObject *object, guint\n> prop_id,\n> self->camera_name = g_value_dup_string(value);\n> break;\n> case PROP_AUTO_FOCUS_MODE:\n> - self->auto_focus_mode =\n> static_cast(g_value_get_enum(value));\n> + self->controls->set(controls::AfMode,\n> +\n> static_cast(g_value_get_enum(value)));\n> break;\n> default:\n> G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> @@ -693,9 +709,11 @@ 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_AUTO_FOCUS_MODE:\n> - g_value_set_enum(value,\n> static_cast(self->auto_focus_mode));\n> + case PROP_AUTO_FOCUS_MODE: {\n> + auto auto_focus_mode =\n> self->controls->get(controls::AfMode).value_or(controls::AfModeManual);\n> + g_value_set_enum(value, auto_focus_mode);\n> break;\n> + }\n> default:\n> G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> break;\n> @@ -760,6 +778,7 @@ gst_libcamera_src_finalize(GObject *object)\n> g_rec_mutex_clear(&self->stream_lock);\n> g_clear_object(&self->task);\n> g_free(self->camera_name);\n> + delete self->controls;\n> delete self->state;\n>\n> return klass->finalize(object);\n> @@ -783,6 +802,8 @@ gst_libcamera_src_init(GstLibcameraSrc *self)\n> /* C-style friend. */\n> state->src_ = self;\n> self->state = state;\n> +\n> + self->controls = new ControlList(controls::controls, nullptr);\n> }\n>\n> static GstPad *\n> diff --git a/src/gstreamer/gstlibcamerasrc.h\n> b/src/gstreamer/gstlibcamerasrc.h\n> index 0a88ba02..2e6d74cb 100644\n> --- a/src/gstreamer/gstlibcamerasrc.h\n> +++ b/src/gstreamer/gstlibcamerasrc.h\n> @@ -26,17 +26,17 @@ gst_libcamera_auto_focus_get_type()\n> static GType type = 0;\n> static const GEnumValue values[] = {\n> {\n> -\n> static_cast(libcamera::controls::AfModeManual),\n> + libcamera::controls::AfModeManual,\n> \"AfModeManual\",\n> \"manual-focus\",\n> },\n> {\n> - static_cast(libcamera::controls::AfModeAuto),\n> + libcamera::controls::AfModeAuto,\n> \"AfModeAuto\",\n> \"automatic-auto-focus\",\n> },\n> {\n> -\n> static_cast(libcamera::controls::AfModeContinuous),\n> + libcamera::controls::AfModeContinuous,\n> \"AfModeContinuous\",\n> \"continuous-auto-focus\",\n> },\n> --\n> 2.39.1\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 565B8C3237\n\tfor ;\n\tThu, 22 Jun 2023 15:25:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 913D76002B;\n\tThu, 22 Jun 2023 17:25:14 +0200 (CEST)","from mail-vk1-xa2f.google.com (mail-vk1-xa2f.google.com\n\t[IPv6:2607:f8b0:4864:20::a2f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BDAA56002B\n\tfor ;\n\tThu, 22 Jun 2023 17:25:12 +0200 (CEST)","by mail-vk1-xa2f.google.com with SMTP id\n\t71dfb90a1353d-460eb67244eso2683880e0c.1\n\tfor ;\n\tThu, 22 Jun 2023 08:25:12 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1687447514;\n\tbh=HqLun0bDoVyx2s0LyfEayP+uFV4TT1u5mKr49bhG0LA=;\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=g47Q8mF30LEZNABcBDyekAwzohwSQC/O3kU1UUoG7FGL9JnTFc665tsji4PuPSpRe\n\tRjrSLyvs//Z7T6ogsqHUzsJfJ6Y1ilsGZgYo6Ri7AijMs3nhjRdYtNa8KPYJjLQUUn\n\tJsije7Ho8/1GK8zOrnz5swOJCMcahnJiokaBydMw+lbO3zEUt7ykMxjR9q1yKFUYsN\n\tB5uGBuVa9G89j51ckEYApxVBewfo1UP6UMQtXN4CJqdyW6EmhcNNUHyStvznKh6GqU\n\tbtW7TosP1hdvd2Qe152KdsR4ckwsiGlWvhqhw3at5VunVT5VM4X13AJf7D0CzxXWBp\n\t8T49oOja9BK4w==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=plumerai.com; s=google; t=1687447511; x=1690039511;\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=uSj4vxU96jbnKgv3M854dCselHebJ4DvHALdox8OdVM=;\n\tb=urE/p3Pl1DKEfYLmtJBogXgtIdyj7CnVqD3Fz9o2oSMwvb3rPtRmKCrAexVcG7wRMF\n\tex6UDypkmFCRRCPehd/AJwQn17PKKIFB70JqiqN1bUvHNmAjWEb5DWUc2Ati+KwgMKvx\n\tozZ0PF4nAgXwSp9+NDC/zuUtG3s8cEH2cWUnY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=plumerai.com\n\theader.i=@plumerai.com header.b=\"urE/p3Pl\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1687447511; x=1690039511;\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=uSj4vxU96jbnKgv3M854dCselHebJ4DvHALdox8OdVM=;\n\tb=cA2dbIEZOp8NTbmaO0lqXXAQDklZ3MDEj91RaBzXUS81vo6e7OZlTOBx38EMqOoBJO\n\tdadlcwX94TGGmKerpVVQWmTtWF6WsO52Z8uqpLBP8NNPmyzWCJd/lB7DbSzumX7EzG5F\n\tDQhDu0VCaoa3wa5Irkvca/2XaQf7GT2rCFn/MUznfWDdRL7rwhfLInCz1GJ2iTGjbkzm\n\tyWrGzlThYZRUvhvEDXSbnhCCw3kFt+GatbIaL+zd+6abSQAi//XG5Lykc10+lkzZaOFE\n\thpSr+AFNSf4SToWLwcou9gLTuifFEAQntYZzMG9erUT+w3AbHEvxXNanaTz4I5e6YVnI\n\t4ZpA==","X-Gm-Message-State":"AC+VfDw/aXDAUFlsWXrp/2G3jYfaxKFyYeMEewUUFLBRSkuejgti0aac\n\t+3ywDbTFVzPG2wk12BJBpTHj81rKGDp5mS6bDSq37RBCCjdL9m40","X-Google-Smtp-Source":"ACHHUZ5fvENYkml9UCdkvHLOyGxXDaC0wGmUC9dbNySyLxxzsJxGOKx7y3Gq8CL+BxNanC1kxfj3pfjoc22Xu61kD0Q=","X-Received":"by 2002:a67:ee58:0:b0:440:d201:f06 with SMTP id\n\tg24-20020a67ee58000000b00440d2010f06mr3875234vsp.20.1687447511553;\n\tThu, 22 Jun 2023 08:25:11 -0700 (PDT)","MIME-Version":"1.0","References":"<20230622111717.32236-1-umang.jain@ideasonboard.com>","In-Reply-To":"<20230622111717.32236-1-umang.jain@ideasonboard.com>","Date":"Thu, 22 Jun 2023 17:25:00 +0200","Message-ID":"","To":"Umang Jain ","Content-Type":"multipart/alternative; boundary=\"00000000000080ece505feb97d32\"","Subject":"Re: [libcamera-devel] [PATCH] gstreamer: Maintain a control list to\n\ttrack libcamerasrc control properties","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,\n\tCedric Nugteren ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27423,"web_url":"https://patchwork.libcamera.org/comment/27423/","msgid":"<168779489597.3585053.9706051913623380578@Monstersaurus>","date":"2023-06-26T15:54:55","subject":"Re: [libcamera-devel] [PATCH] gstreamer: Maintain a control list to\n\ttrack libcamerasrc control properties","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Umang,\n\nQuoting Umang Jain via libcamera-devel (2023-06-22 12:17:17)\n> As more and more libcamera controls get plumbed, more member variables\n> get introduced in struct GstLibcameraSrc. Instead of doing that, now\n> maintain a single ControlList which is more appropriate to keep track\n> of controls that get sets on libcamerasrc.\n> \n> This also brings easy validation of controls set on libcamerasrc. If\n> the controls are not supported by camera, abort early and report what is\n\nI would change 'abort' to 'fail negotiation' as I thought from reading\nthis there was going to be a call to abort().\n\n> not supported.\n> \n> The current patch migrates previously introduced control,\n> auto-focus-mode, to be set directly in control list.\n> \n> Signed-off-by: Umang Jain \n> ---\n> Cedric, Can you please help test this?\n> \n> (I'm travelling so have tested this on my RPi and inferring from\n> libcamera/gstreamer logs, it seems to have not broken anything)\n> \n> An additional test from your side would be appreciated.\n> ---\n> src/gstreamer/gstlibcamerasrc.cpp | 45 ++++++++++++++++++++++---------\n> src/gstreamer/gstlibcamerasrc.h | 6 ++---\n> 2 files changed, 36 insertions(+), 15 deletions(-)\n> \n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index 47d8ff43..a2266310 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -146,7 +146,7 @@ struct _GstLibcameraSrc {\n> GstTask *task;\n> \n> gchar *camera_name;\n> - controls::AfModeEnum auto_focus_mode = controls::AfModeManual;\n> + ControlList *controls;\n> \n> GstLibcameraSrcState *state;\n> GstLibcameraAllocator *allocator;\n> @@ -462,7 +462,9 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n> GLibRecLocker lock(&self->stream_lock);\n> GstLibcameraSrcState *state = self->state;\n> + const ControlInfoMap &info_map = state->cam_->controls();\n> GstFlowReturn flow_ret = GST_FLOW_OK;\n> + gboolean supported_ctrl = true;\n> gint ret;\n> \n> g_autoptr(GstStructure) element_caps = gst_structure_new_empty(\"caps\");\n> @@ -579,18 +581,31 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> gst_flow_combiner_add_pad(self->flow_combiner, srcpad);\n> }\n> \n> - if (self->auto_focus_mode != controls::AfModeManual) {\n> - const ControlInfoMap &infoMap = state->cam_->controls();\n> - if (infoMap.find(&controls::AfMode) != infoMap.end()) {\n> - state->initControls_.set(controls::AfMode, self->auto_focus_mode);\n> - } else {\n> + /*\n> + * Check the controls set on libcamerasrc is supported by the camera.\n> + * This can be easily done by CameraControlValidator but it is an internal\n> + * libcamera class. Hence avoid adding a internal header even though\n> + * gstreamer links with libcamera-private.\n> + */\n> + for (auto iter = self->controls->begin(); iter != self->controls->end(); iter++) {\n> + if (info_map.find(iter->first) == info_map.end()) {\n> + const ControlIdMap *id_map = self->controls->idMap();\n> GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> - (\"Failed to enable auto focus\"),\n> - (\"AfMode not supported by this camera, \"\n> - \"please retry with 'auto-focus-mode=AfModeManual'\"));\n> + (\"Failed to set %s\", id_map->at(iter->first)->name().c_str()),\n> + (\"%s not supported by this camera, \", id_map->at(iter->first)->name().c_str()));\n\nIsn't this printing the name twice? Maybe this could be a single output\nline.\n\n> + supported_ctrl = false;\n> + break;\n\nI would probably not break here - as if there are multiple controls that\nfail to validate - it might make sense to report all of them at once.\n\n\n> }\n> }\n> \n> + if (!supported_ctrl) {\n> + gst_task_stop(task);\n> + flow_ret = GST_FLOW_NOT_NEGOTIATED;\n> + goto done;\n> + }\n> +\n> + state->initControls_.merge(*self->controls);\n> +\n> ret = state->cam_->start(&state->initControls_);\n> if (ret) {\n> GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> @@ -674,7 +689,8 @@ gst_libcamera_src_set_property(GObject *object, guint prop_id,\n> self->camera_name = g_value_dup_string(value);\n> break;\n> case PROP_AUTO_FOCUS_MODE:\n> - self->auto_focus_mode = static_cast(g_value_get_enum(value));\n> + self->controls->set(controls::AfMode,\n> + static_cast(g_value_get_enum(value)));\n> break;\n> default:\n> G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> @@ -693,9 +709,11 @@ gst_libcamera_src_get_property(GObject *object, guint prop_id, GValue *value,\n> case PROP_CAMERA_NAME:\n> g_value_set_string(value, self->camera_name);\n> break;\n> - case PROP_AUTO_FOCUS_MODE:\n> - g_value_set_enum(value, static_cast(self->auto_focus_mode));\n> + case PROP_AUTO_FOCUS_MODE: {\n> + auto auto_focus_mode = self->controls->get(controls::AfMode).value_or(controls::AfModeManual);\n> + g_value_set_enum(value, auto_focus_mode);\n> break;\n> + }\n> default:\n> G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> break;\n> @@ -760,6 +778,7 @@ gst_libcamera_src_finalize(GObject *object)\n> g_rec_mutex_clear(&self->stream_lock);\n> g_clear_object(&self->task);\n> g_free(self->camera_name);\n> + delete self->controls;\n\nI would have said can we use a unique_ptr, but it doesn't look like that\nmakes sense without reworking the rest of the structure...\n\n> delete self->state;\n> \n> return klass->finalize(object);\n> @@ -783,6 +802,8 @@ gst_libcamera_src_init(GstLibcameraSrc *self)\n> /* C-style friend. */\n> state->src_ = self;\n> self->state = state;\n> +\n> + self->controls = new ControlList(controls::controls, nullptr);\n> }\n> \n> static GstPad *\n> diff --git a/src/gstreamer/gstlibcamerasrc.h b/src/gstreamer/gstlibcamerasrc.h\n> index 0a88ba02..2e6d74cb 100644\n> --- a/src/gstreamer/gstlibcamerasrc.h\n> +++ b/src/gstreamer/gstlibcamerasrc.h\n> @@ -26,17 +26,17 @@ gst_libcamera_auto_focus_get_type()\n> static GType type = 0;\n> static const GEnumValue values[] = {\n> {\n> - static_cast(libcamera::controls::AfModeManual),\n> + libcamera::controls::AfModeManual,\n\nNice to see the cast is no longer needed. Is that a result of a change\nhere, or was it already possible. ?\n\n> \"AfModeManual\",\n> \"manual-focus\",\n> },\n> {\n> - static_cast(libcamera::controls::AfModeAuto),\n> + libcamera::controls::AfModeAuto,\n> \"AfModeAuto\",\n> \"automatic-auto-focus\",\n> },\n> {\n> - static_cast(libcamera::controls::AfModeContinuous),\n> + libcamera::controls::AfModeContinuous,\n> \"AfModeContinuous\",\n> \"continuous-auto-focus\",\n> },\n> -- \n> 2.39.1\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 97731C3237\n\tfor ;\n\tMon, 26 Jun 2023 15:55:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B42CB628C0;\n\tMon, 26 Jun 2023 17:55:00 +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 91E54628BB\n\tfor ;\n\tMon, 26 Jun 2023 17:54:59 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 654472D8;\n\tMon, 26 Jun 2023 17:54:20 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1687794900;\n\tbh=FFwwW68K4C0HQHJXuloxCaTJf8aQMY7/8KBYXR+pFCA=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=bTvF3VYCT1rLTqyuNnzk39kEOwYWzyALlEhqpdnVfKQ+bC7A8KiERCIwDG3EjQmrq\n\t4owj0in7MK2GntkkE2cNIYluo4Qhm/6fukJTpXqXLcYrhkZmO7KdjNa7e6wADiQ4wE\n\t1M5TF/NbnzsXObiUUcLaIUWpLP6wfBL1KZLdXgCqEKJHtvVm1Ll7Mxm7qgvyDMXFKK\n\tMJJTc6VSq3iNz1WWTOEzyY5wvZSM16GorGpP87TqRmDcQNiM+jpsTLMXBpYpBQrlua\n\tRGYez2mf+m43YGeobYiaTQ9u4JwSg1V4AIEhcNDGG5N/7ylSU3QY7EhJmwbwFizt63\n\tQWWXMF7rtEAUQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1687794860;\n\tbh=FFwwW68K4C0HQHJXuloxCaTJf8aQMY7/8KBYXR+pFCA=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=Tn3Tv5kYkIfQKaA7bVQ3yWg6RKYhrwULn8tHvGiBuAyclYjaIpJHZIOSQlcfNcw5V\n\tm/8ZiYtYRxcsZ6H0B4TGfUV36620gsGxUUMez6S+W3H384cf/sMJnkE9R7LZ0bsUDB\n\thbqpMVF4mTcNSLHWoh0rsFR4BvsBjf6WVL0Flpjk="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Tn3Tv5kY\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20230622111717.32236-1-umang.jain@ideasonboard.com>","References":"<20230622111717.32236-1-umang.jain@ideasonboard.com>","To":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 26 Jun 2023 16:54:55 +0100","Message-ID":"<168779489597.3585053.9706051913623380578@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH] gstreamer: Maintain a control list to\n\ttrack libcamerasrc control properties","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":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"Cedric Nugteren ,\n\tCedric Nugteren ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27424,"web_url":"https://patchwork.libcamera.org/comment/27424/","msgid":"","date":"2023-06-26T16:15:37","subject":"Re: [libcamera-devel] [PATCH] gstreamer: Maintain a control list to\n\ttrack libcamerasrc control properties","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"HI Kieran,\n\nOn 6/26/23 9:24 PM, Kieran Bingham wrote:\n> Hi Umang,\n>\n> Quoting Umang Jain via libcamera-devel (2023-06-22 12:17:17)\n>> As more and more libcamera controls get plumbed, more member variables\n>> get introduced in struct GstLibcameraSrc. Instead of doing that, now\n>> maintain a single ControlList which is more appropriate to keep track\n>> of controls that get sets on libcamerasrc.\n>>\n>> This also brings easy validation of controls set on libcamerasrc. If\n>> the controls are not supported by camera, abort early and report what is\n> I would change 'abort' to 'fail negotiation' as I thought from reading\n> this there was going to be a call to abort().\n\nack\n>\n>> not supported.\n>>\n>> The current patch migrates previously introduced control,\n>> auto-focus-mode, to be set directly in control list.\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> Cedric, Can you please help test this?\n>>\n>> (I'm travelling so have tested this on my RPi and inferring from\n>> libcamera/gstreamer logs, it seems to have not broken anything)\n>>\n>> An additional test from your side would be appreciated.\n>> ---\n>> src/gstreamer/gstlibcamerasrc.cpp | 45 ++++++++++++++++++++++---------\n>> src/gstreamer/gstlibcamerasrc.h | 6 ++---\n>> 2 files changed, 36 insertions(+), 15 deletions(-)\n>>\n>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n>> index 47d8ff43..a2266310 100644\n>> --- a/src/gstreamer/gstlibcamerasrc.cpp\n>> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n>> @@ -146,7 +146,7 @@ struct _GstLibcameraSrc {\n>> GstTask *task;\n>> \n>> gchar *camera_name;\n>> - controls::AfModeEnum auto_focus_mode = controls::AfModeManual;\n>> + ControlList *controls;\n>> \n>> GstLibcameraSrcState *state;\n>> GstLibcameraAllocator *allocator;\n>> @@ -462,7 +462,9 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n>> GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n>> GLibRecLocker lock(&self->stream_lock);\n>> GstLibcameraSrcState *state = self->state;\n>> + const ControlInfoMap &info_map = state->cam_->controls();\n>> GstFlowReturn flow_ret = GST_FLOW_OK;\n>> + gboolean supported_ctrl = true;\n>> gint ret;\n>> \n>> g_autoptr(GstStructure) element_caps = gst_structure_new_empty(\"caps\");\n>> @@ -579,18 +581,31 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n>> gst_flow_combiner_add_pad(self->flow_combiner, srcpad);\n>> }\n>> \n>> - if (self->auto_focus_mode != controls::AfModeManual) {\n>> - const ControlInfoMap &infoMap = state->cam_->controls();\n>> - if (infoMap.find(&controls::AfMode) != infoMap.end()) {\n>> - state->initControls_.set(controls::AfMode, self->auto_focus_mode);\n>> - } else {\n>> + /*\n>> + * Check the controls set on libcamerasrc is supported by the camera.\n>> + * This can be easily done by CameraControlValidator but it is an internal\n>> + * libcamera class. Hence avoid adding a internal header even though\n>> + * gstreamer links with libcamera-private.\n>> + */\n>> + for (auto iter = self->controls->begin(); iter != self->controls->end(); iter++) {\n>> + if (info_map.find(iter->first) == info_map.end()) {\n>> + const ControlIdMap *id_map = self->controls->idMap();\n>> GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n>> - (\"Failed to enable auto focus\"),\n>> - (\"AfMode not supported by this camera, \"\n>> - \"please retry with 'auto-focus-mode=AfModeManual'\"));\n>> + (\"Failed to set %s\", id_map->at(iter->first)->name().c_str()),\n>> + (\"%s not supported by this camera, \", id_map->at(iter->first)->name().c_str()));\n> Isn't this printing the name twice? Maybe this could be a single output\n> line.\n\nAh probably,  I 'll try to take a look.\n>\n>> + supported_ctrl = false;\n>> + break;\n> I would probably not break here - as if there are multiple controls that\n> fail to validate - it might make sense to report all of them at once.\n\nGood idea!\n>\n>\n>> }\n>> }\n>> \n>> + if (!supported_ctrl) {\n>> + gst_task_stop(task);\n>> + flow_ret = GST_FLOW_NOT_NEGOTIATED;\n>> + goto done;\n>> + }\n>> +\n>> + state->initControls_.merge(*self->controls);\n>> +\n>> ret = state->cam_->start(&state->initControls_);\n>> if (ret) {\n>> GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n>> @@ -674,7 +689,8 @@ gst_libcamera_src_set_property(GObject *object, guint prop_id,\n>> self->camera_name = g_value_dup_string(value);\n>> break;\n>> case PROP_AUTO_FOCUS_MODE:\n>> - self->auto_focus_mode = static_cast(g_value_get_enum(value));\n>> + self->controls->set(controls::AfMode,\n>> + static_cast(g_value_get_enum(value)));\n>> break;\n>> default:\n>> G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n>> @@ -693,9 +709,11 @@ gst_libcamera_src_get_property(GObject *object, guint prop_id, GValue *value,\n>> case PROP_CAMERA_NAME:\n>> g_value_set_string(value, self->camera_name);\n>> break;\n>> - case PROP_AUTO_FOCUS_MODE:\n>> - g_value_set_enum(value, static_cast(self->auto_focus_mode));\n>> + case PROP_AUTO_FOCUS_MODE: {\n>> + auto auto_focus_mode = self->controls->get(controls::AfMode).value_or(controls::AfModeManual);\n>> + g_value_set_enum(value, auto_focus_mode);\n>> break;\n>> + }\n>> default:\n>> G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n>> break;\n>> @@ -760,6 +778,7 @@ gst_libcamera_src_finalize(GObject *object)\n>> g_rec_mutex_clear(&self->stream_lock);\n>> g_clear_object(&self->task);\n>> g_free(self->camera_name);\n>> + delete self->controls;\n> I would have said can we use a unique_ptr, but it doesn't look like that\n> makes sense without reworking the rest of the structure...\n\nI think you can't do unique_ptr of ControlList right now.\n>\n>> delete self->state;\n>> \n>> return klass->finalize(object);\n>> @@ -783,6 +802,8 @@ gst_libcamera_src_init(GstLibcameraSrc *self)\n>> /* C-style friend. */\n>> state->src_ = self;\n>> self->state = state;\n>> +\n>> + self->controls = new ControlList(controls::controls, nullptr);\n>> }\n>> \n>> static GstPad *\n>> diff --git a/src/gstreamer/gstlibcamerasrc.h b/src/gstreamer/gstlibcamerasrc.h\n>> index 0a88ba02..2e6d74cb 100644\n>> --- a/src/gstreamer/gstlibcamerasrc.h\n>> +++ b/src/gstreamer/gstlibcamerasrc.h\n>> @@ -26,17 +26,17 @@ gst_libcamera_auto_focus_get_type()\n>> static GType type = 0;\n>> static const GEnumValue values[] = {\n>> {\n>> - static_cast(libcamera::controls::AfModeManual),\n>> + libcamera::controls::AfModeManual,\n> Nice to see the cast is no longer needed. Is that a result of a change\n> here, or was it already possible. ?\n\nNot sure, I didn't test this isolated change with the earlier AF mode \npatch, but AFAIR i started from here developing this entire patch.\n>\n>> \"AfModeManual\",\n>> \"manual-focus\",\n>> },\n>> {\n>> - static_cast(libcamera::controls::AfModeAuto),\n>> + libcamera::controls::AfModeAuto,\n>> \"AfModeAuto\",\n>> \"automatic-auto-focus\",\n>> },\n>> {\n>> - static_cast(libcamera::controls::AfModeContinuous),\n>> + libcamera::controls::AfModeContinuous,\n>> \"AfModeContinuous\",\n>> \"continuous-auto-focus\",\n>> },\n>> -- \n>> 2.39.1\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 D0ED3BE175\n\tfor ;\n\tMon, 26 Jun 2023 16:15:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 16A77628C0;\n\tMon, 26 Jun 2023 18:15:43 +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 6191A628BB\n\tfor ;\n\tMon, 26 Jun 2023 18:15:41 +0200 (CEST)","from [192.168.0.136] (85-160-54-103.reb.o2.cz [85.160.54.103])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8314811A9;\n\tMon, 26 Jun 2023 18:15:01 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1687796143;\n\tbh=e3F8cPvGzH5uVhpJ9IRe87gRO/c4rUp3nhx5h8UPY4o=;\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=ULf9F7nQAee5CwoRsXmT1nSUWo41MPq/rfybBB511HCSIxyUTnjfdbuQzrGzRzwfG\n\tpKOO85IaxON97dHqsPsrqmIIAV473woP0zDBd4S268wuDn4GrKDZz+FNmmNjCiTQXA\n\thW0xYhfPcDgI51cEf76ODL0IEZC3QlRTXA28ygoBg27L9J6o0pCpDW4IwVu8jRp+3F\n\tyiOywTLu2aEiR43OTwGKJ6Ls0UseUxppCChir7zo7yzLQB4KKrK7VExA2mLWB1hAzB\n\tZi5YD7lbmEgh7tyT6JlnRiPx1PgeTUdZuUyLXGuWpIn898giCeXnUt5tMvEn4Sb6ys\n\tVQB7Q9zKJTWdg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1687796102;\n\tbh=e3F8cPvGzH5uVhpJ9IRe87gRO/c4rUp3nhx5h8UPY4o=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=EgVqLoiXOHHIOCOvyJ9zOwPs8gPZpIfay3t3o2ThSjML7fO4duAH2GZaThq9pln9l\n\t0Jx0TRYAqI3LvomgG2UZWD3vR2MwHXJk0xrSNpLGUTdMe6LRYo3CrzXzfAXYHJQkP6\n\tYy6MZDsTOnszlIbH4LXbaM2LoQfFlrtQIlqJfCVI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"EgVqLoiX\"; dkim-atps=neutral","Message-ID":"","Date":"Mon, 26 Jun 2023 18:15:37 +0200","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.7.1","Content-Language":"en-US","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20230622111717.32236-1-umang.jain@ideasonboard.com>\n\t<168779489597.3585053.9706051913623380578@Monstersaurus>","In-Reply-To":"<168779489597.3585053.9706051913623380578@Monstersaurus>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] gstreamer: Maintain a control list to\n\ttrack libcamerasrc control properties","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":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"Cedric Nugteren ,\n\tCedric Nugteren ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27425,"web_url":"https://patchwork.libcamera.org/comment/27425/","msgid":"<0470483e-dc21-c9bd-421b-d892c40ab36b@ideasonboard.com>","date":"2023-06-26T17:36:09","subject":"Re: [libcamera-devel] [PATCH] gstreamer: Maintain a control list to\n\ttrack libcamerasrc control properties","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi again,\n\nOn 6/26/23 9:45 PM, Umang Jain via libcamera-devel wrote:\n> HI Kieran,\n>\n> On 6/26/23 9:24 PM, Kieran Bingham wrote:\n>> Hi Umang,\n>>\n>> Quoting Umang Jain via libcamera-devel (2023-06-22 12:17:17)\n>>> As more and more libcamera controls get plumbed, more member variables\n>>> get introduced in struct GstLibcameraSrc. Instead of doing that, now\n>>> maintain a single ControlList which is more appropriate to keep track\n>>> of controls that get sets on libcamerasrc.\n>>>\n>>> This also brings easy validation of controls set on libcamerasrc. If\n>>> the controls are not supported by camera, abort early and report \n>>> what is\n>> I would change 'abort' to 'fail negotiation' as I thought from reading\n>> this there was going to be a call to abort().\n>\n> ack\n>>\n>>> not supported.\n>>>\n>>> The current patch migrates previously introduced control,\n>>> auto-focus-mode, to be set directly in control list.\n>>>\n>>> Signed-off-by: Umang Jain \n>>> ---\n>>> Cedric, Can you please help test this?\n>>>\n>>> (I'm travelling so have tested this on my RPi and inferring from\n>>> libcamera/gstreamer logs, it seems to have not broken anything)\n>>>\n>>> An additional test from your side would be appreciated.\n>>> ---\n>>>   src/gstreamer/gstlibcamerasrc.cpp | 45 \n>>> ++++++++++++++++++++++---------\n>>>   src/gstreamer/gstlibcamerasrc.h   |  6 ++---\n>>>   2 files changed, 36 insertions(+), 15 deletions(-)\n>>>\n>>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp \n>>> b/src/gstreamer/gstlibcamerasrc.cpp\n>>> index 47d8ff43..a2266310 100644\n>>> --- a/src/gstreamer/gstlibcamerasrc.cpp\n>>> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n>>> @@ -146,7 +146,7 @@ struct _GstLibcameraSrc {\n>>>          GstTask *task;\n>>>            gchar *camera_name;\n>>> -       controls::AfModeEnum auto_focus_mode = controls::AfModeManual;\n>>> +       ControlList *controls;\n>>>            GstLibcameraSrcState *state;\n>>>          GstLibcameraAllocator *allocator;\n>>> @@ -462,7 +462,9 @@ gst_libcamera_src_task_enter(GstTask *task, \n>>> [[maybe_unused]] GThread *thread,\n>>>          GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n>>>          GLibRecLocker lock(&self->stream_lock);\n>>>          GstLibcameraSrcState *state = self->state;\n>>> +       const ControlInfoMap &info_map = state->cam_->controls();\n>>>          GstFlowReturn flow_ret = GST_FLOW_OK;\n>>> +       gboolean supported_ctrl = true;\n>>>          gint ret;\n>>>            g_autoptr(GstStructure) element_caps = \n>>> gst_structure_new_empty(\"caps\");\n>>> @@ -579,18 +581,31 @@ gst_libcamera_src_task_enter(GstTask *task, \n>>> [[maybe_unused]] GThread *thread,\n>>> gst_flow_combiner_add_pad(self->flow_combiner, srcpad);\n>>>          }\n>>>   -       if (self->auto_focus_mode != controls::AfModeManual) {\n>>> -               const ControlInfoMap &infoMap = \n>>> state->cam_->controls();\n>>> -               if (infoMap.find(&controls::AfMode) != infoMap.end()) {\n>>> - state->initControls_.set(controls::AfMode, self->auto_focus_mode);\n>>> -               } else {\n>>> +       /*\n>>> +        * Check the controls set on libcamerasrc is supported by \n>>> the camera.\n>>> +        * This can be easily done by CameraControlValidator but it \n>>> is an internal\n>>> +        * libcamera class. Hence avoid adding a internal header \n>>> even though\n>>> +        * gstreamer links with libcamera-private.\n>>> +        */\n>>> +       for (auto iter = self->controls->begin(); iter != \n>>> self->controls->end(); iter++) {\n>>> +               if (info_map.find(iter->first) == info_map.end()) {\n>>> +                       const ControlIdMap *id_map = \n>>> self->controls->idMap();\n>>>                          GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n>>> -                                         (\"Failed to enable auto \n>>> focus\"),\n>>> -                                         (\"AfMode not supported by \n>>> this camera, \"\n>>> -                                          \"please retry with \n>>> 'auto-focus-mode=AfModeManual'\"));\n>>> +                                         (\"Failed to set %s\", \n>>> id_map->at(iter->first)->name().c_str()),\n>>> +                                         (\"%s not supported by this \n>>> camera, \", id_map->at(iter->first)->name().c_str()));\n>> Isn't this printing the name twice? Maybe this could be a single output\n>> line.\n>\n> Ah probably,  I 'll try to take a look.\n\nSo the second part, is really the DEBUG log not the main log.\n\nhttps://gstreamer.freedesktop.org/documentation/gstreamer/gstelement.html?gi-language=c#GST_ELEMENT_ERROR\n\nSo this is fine as is.\n>>\n>>> +                       supported_ctrl = false;\n>>> +                       break;\n>> I would probably not break here - as if there are multiple controls that\n>> fail to validate - it might make sense to report all of them at once.\n>\n> Good idea!\n>>\n>>\n>>>                  }\n>>>          }\n>>>   +       if (!supported_ctrl) {\n>>> +               gst_task_stop(task);\n>>> +               flow_ret = GST_FLOW_NOT_NEGOTIATED;\n>>> +               goto done;\n>>> +       }\n>>> +\n>>> +       state->initControls_.merge(*self->controls);\n>>> +\n>>>          ret = state->cam_->start(&state->initControls_);\n>>>          if (ret) {\n>>>                  GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n>>> @@ -674,7 +689,8 @@ gst_libcamera_src_set_property(GObject *object, \n>>> guint prop_id,\n>>>                  self->camera_name = g_value_dup_string(value);\n>>>                  break;\n>>>          case PROP_AUTO_FOCUS_MODE:\n>>> -               self->auto_focus_mode = \n>>> static_cast(g_value_get_enum(value));\n>>> +               self->controls->set(controls::AfMode,\n>>> + static_cast(g_value_get_enum(value)));\n>>>                  break;\n>>>          default:\n>>>                  G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, \n>>> pspec);\n>>> @@ -693,9 +709,11 @@ gst_libcamera_src_get_property(GObject *object, \n>>> guint prop_id, GValue *value,\n>>>          case PROP_CAMERA_NAME:\n>>>                  g_value_set_string(value, self->camera_name);\n>>>                  break;\n>>> -       case PROP_AUTO_FOCUS_MODE:\n>>> -               g_value_set_enum(value, \n>>> static_cast(self->auto_focus_mode));\n>>> +       case PROP_AUTO_FOCUS_MODE: {\n>>> +               auto auto_focus_mode = \n>>> self->controls->get(controls::AfMode).value_or(controls::AfModeManual);\n>>> +               g_value_set_enum(value, auto_focus_mode);\n>>>                  break;\n>>> +       }\n>>>          default:\n>>>                  G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, \n>>> pspec);\n>>>                  break;\n>>> @@ -760,6 +778,7 @@ gst_libcamera_src_finalize(GObject *object)\n>>>          g_rec_mutex_clear(&self->stream_lock);\n>>>          g_clear_object(&self->task);\n>>>          g_free(self->camera_name);\n>>> +       delete self->controls;\n>> I would have said can we use a unique_ptr, but it doesn't look like that\n>> makes sense without reworking the rest of the structure...\n>\n> I think you can't do unique_ptr of ControlList right now.\n>>\n>>>          delete self->state;\n>>>            return klass->finalize(object);\n>>> @@ -783,6 +802,8 @@ gst_libcamera_src_init(GstLibcameraSrc *self)\n>>>          /* C-style friend. */\n>>>          state->src_ = self;\n>>>          self->state = state;\n>>> +\n>>> +       self->controls = new ControlList(controls::controls, nullptr);\n>>>   }\n>>>     static GstPad *\n>>> diff --git a/src/gstreamer/gstlibcamerasrc.h \n>>> b/src/gstreamer/gstlibcamerasrc.h\n>>> index 0a88ba02..2e6d74cb 100644\n>>> --- a/src/gstreamer/gstlibcamerasrc.h\n>>> +++ b/src/gstreamer/gstlibcamerasrc.h\n>>> @@ -26,17 +26,17 @@ gst_libcamera_auto_focus_get_type()\n>>>          static GType type = 0;\n>>>          static const GEnumValue values[] = {\n>>>                  {\n>>> - static_cast(libcamera::controls::AfModeManual),\n>>> +                       libcamera::controls::AfModeManual,\n>> Nice to see the cast is no longer needed. Is that a result of a change\n>> here, or was it already possible. ?\n>\n> Not sure, I didn't test this isolated change with the earlier AF mode \n> patch, but AFAIR i started from here developing this entire patch.\n>>\n>>>                          \"AfModeManual\",\n>>>                          \"manual-focus\",\n>>>                  },\n>>>                  {\n>>> - static_cast(libcamera::controls::AfModeAuto),\n>>> +                       libcamera::controls::AfModeAuto,\n>>>                          \"AfModeAuto\",\n>>>                          \"automatic-auto-focus\",\n>>>                  },\n>>>                  {\n>>> - static_cast(libcamera::controls::AfModeContinuous),\n>>> +                       libcamera::controls::AfModeContinuous,\n>>>                          \"AfModeContinuous\",\n>>>                          \"continuous-auto-focus\",\n>>>                  },\n>>> -- \n>>> 2.39.1\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 D0715C3237\n\tfor ;\n\tMon, 26 Jun 2023 17:36:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2E3F8628BC;\n\tMon, 26 Jun 2023 19:36:14 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 02B1C628BB\n\tfor ;\n\tMon, 26 Jun 2023 19:36:12 +0200 (CEST)","from [192.168.0.136] (85-160-54-103.reb.o2.cz [85.160.54.103])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A95C54AD;\n\tMon, 26 Jun 2023 19:35:33 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1687800974;\n\tbh=qgOv9M5/zfopUud1t1msPNl93PrNpve1N9EtQ20qNIw=;\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=aKx1Xyb3AC2mo21I9xFocreFOzW8gtNW8H45eekFdLATqFSGvCXhG8rvSUjhzDwm0\n\t8S2KCYAfo5sVhORIjrsXZ6D7Ke03Y9/AbpQOKBEvKK58RNCHSp4RQQFrjY9g8EfZVC\n\tn6l2teMkNtVHVIZ5iQRRNgo9nG/O1GHJQDNSAZStzAdGb8pREpN0ZOItAZgkjFeSuG\n\tmmhcML1fkcRrLb6x7lMlv3eBOJ5vQhYzKOsnjI5/9Xvo7cfHUrQ/7tyA7TE14EMHUO\n\tRJaYx3JSK2nw2G7xtz73igoR6SzEseTmw0QhG5lAs9dVYE5fE1+cweA8ATuF7biWlv\n\tg/MaeEW5/Z78A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1687800934;\n\tbh=qgOv9M5/zfopUud1t1msPNl93PrNpve1N9EtQ20qNIw=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=QLhxxkiDhHp1FQsK2ISs9bBum8mdMeaVc4/l8oqK1aWXxVFg8lbU806GpjRJCqaup\n\tNXSueufal0A2sHAnyKoiqfps9WxLXLs77Bq1ETKp0BmeYYiDMMrxYUHMwfazLX07Yi\n\tFTcbsijtkIvAAL7BFlRH2wJjg3e5LYXUeSIiYEIo="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"QLhxxkiD\"; dkim-atps=neutral","Message-ID":"<0470483e-dc21-c9bd-421b-d892c40ab36b@ideasonboard.com>","Date":"Mon, 26 Jun 2023 19:36:09 +0200","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.7.1","Content-Language":"en-US","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20230622111717.32236-1-umang.jain@ideasonboard.com>\n\t<168779489597.3585053.9706051913623380578@Monstersaurus>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] gstreamer: Maintain a control list to\n\ttrack libcamerasrc control properties","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":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"Cedric Nugteren ,\n\tCedric Nugteren ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]