[{"id":27426,"web_url":"https://patchwork.libcamera.org/comment/27426/","msgid":"<9599c74149b0cd025c5890741cdfff779547f3fa.camel@ndufresne.ca>","date":"2023-06-26T19:20:42","subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Maintain a control list\n\tto track libcamerasrc control properties","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Le lundi 26 juin 2023 à 19:47 +0200, Umang Jain a écrit :\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, fail negotiation and report\n> what is 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> Changes in v2:\n> - Edit commit message \n> - Drop a break; so that all unsupported user-provided controls are\n> reported at once.\n> ---\n> src/gstreamer/gstlibcamerasrc.cpp | 44 ++++++++++++++++++++++---------\n> src/gstreamer/gstlibcamerasrc.h | 6 ++---\n> 2 files changed, 35 insertions(+), 15 deletions(-)\n> \n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index 47d8ff43..11f15068 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -146,7 +146,7 @@ struct _GstLibcameraSrc {\n> \tGstTask *task;\n> \n> \tgchar *camera_name;\n> -\tcontrols::AfModeEnum auto_focus_mode = controls::AfModeManual;\n> +\tControlList *controls;\n> \n> \tGstLibcameraSrcState *state;\n> \tGstLibcameraAllocator *allocator;\n> @@ -462,7 +462,9 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> \tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n> \tGLibRecLocker lock(&self->stream_lock);\n> \tGstLibcameraSrcState *state = self->state;\n> +\tconst ControlInfoMap &info_map = state->cam_->controls();\n> \tGstFlowReturn flow_ret = GST_FLOW_OK;\n> +\tgboolean supported_ctrl = true;\n> \tgint ret;\n> \n> \tg_autoptr(GstStructure) element_caps = gst_structure_new_empty(\"caps\");\n> @@ -579,18 +581,30 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> \t\tgst_flow_combiner_add_pad(self->flow_combiner, srcpad);\n> \t}\n> \n> -\tif (self->auto_focus_mode != controls::AfModeManual) {\n> -\t\tconst ControlInfoMap &infoMap = state->cam_->controls();\n> -\t\tif (infoMap.find(&controls::AfMode) != infoMap.end()) {\n> -\t\t\tstate->initControls_.set(controls::AfMode, self->auto_focus_mode);\n> -\t\t} else {\n> +\t/*\n> +\t * Check the controls set on libcamerasrc is supported by the camera.\n> +\t * This can be easily done by CameraControlValidator but it is an internal\n> +\t * libcamera class. Hence avoid adding a internal header even though\n> +\t * gstreamer links with libcamera-private.\n\nI don't remember why GStreamer links to that, and why we should accept this as a\nfact. The longer term plan, when libcamera is stable, was to migrate this\nelement into GStreamer project, but such thing will prevent it.\n\n> +\t */\n> +\tfor (auto iter = self->controls->begin(); iter != self->controls->end(); iter++) {\n> +\t\tif (info_map.find(iter->first) == info_map.end()) {\n> +\t\t\tconst ControlIdMap *id_map = self->controls->idMap();\n> \t\t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> -\t\t\t\t\t (\"Failed to enable auto focus\"),\n> -\t\t\t\t\t (\"AfMode not supported by this camera, \"\n> -\t\t\t\t\t \"please retry with 'auto-focus-mode=AfModeManual'\"));\n> +\t\t\t\t\t (\"Failed to set %s\", id_map->at(iter->first)->name().c_str()),\n> +\t\t\t\t\t (\"%s not supported by this camera, \", id_map->at(iter->first)->name().c_str()));\n> +\t\t\tsupported_ctrl = false;\n> \t\t}\n> \t}\n> \n> +\tif (!supported_ctrl) {\n> +\t\tgst_task_stop(task);\n> +\t\tflow_ret = GST_FLOW_NOT_NEGOTIATED;\n> +\t\tgoto done;\n> +\t}\n> +\n> +\tstate->initControls_.merge(*self->controls);\n> +\n> \tret = state->cam_->start(&state->initControls_);\n> \tif (ret) {\n> \t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> @@ -674,7 +688,8 @@ gst_libcamera_src_set_property(GObject *object, guint prop_id,\n> \t\tself->camera_name = g_value_dup_string(value);\n> \t\tbreak;\n> \tcase PROP_AUTO_FOCUS_MODE:\n> -\t\tself->auto_focus_mode = static_cast(g_value_get_enum(value));\n> +\t\tself->controls->set(controls::AfMode,\n> +\t\t\t\t static_cast(g_value_get_enum(value)));\n> \t\tbreak;\n> \tdefault:\n> \t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> @@ -693,9 +708,11 @@ gst_libcamera_src_get_property(GObject *object, guint prop_id, GValue *value,\n> \tcase PROP_CAMERA_NAME:\n> \t\tg_value_set_string(value, self->camera_name);\n> \t\tbreak;\n> -\tcase PROP_AUTO_FOCUS_MODE:\n> -\t\tg_value_set_enum(value, static_cast(self->auto_focus_mode));\n> +\tcase PROP_AUTO_FOCUS_MODE: {\n> +\t\tauto auto_focus_mode = self->controls->get(controls::AfMode).value_or(controls::AfModeManual);\n> +\t\tg_value_set_enum(value, auto_focus_mode);\n> \t\tbreak;\n> +\t}\n> \tdefault:\n> \t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> \t\tbreak;\n> @@ -760,6 +777,7 @@ gst_libcamera_src_finalize(GObject *object)\n> \tg_rec_mutex_clear(&self->stream_lock);\n> \tg_clear_object(&self->task);\n> \tg_free(self->camera_name);\n> +\tdelete self->controls;\n> \tdelete self->state;\n> \n> \treturn klass->finalize(object);\n> @@ -783,6 +801,8 @@ gst_libcamera_src_init(GstLibcameraSrc *self)\n> \t/* C-style friend. */\n> \tstate->src_ = self;\n> \tself->state = state;\n> +\n> +\tself->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> \tstatic GType type = 0;\n> \tstatic const GEnumValue values[] = {\n> \t\t{\n> -\t\t\tstatic_cast(libcamera::controls::AfModeManual),\n> +\t\t\tlibcamera::controls::AfModeManual,\n> \t\t\t\"AfModeManual\",\n> \t\t\t\"manual-focus\",\n> \t\t},\n> \t\t{\n> -\t\t\tstatic_cast(libcamera::controls::AfModeAuto),\n> +\t\t\tlibcamera::controls::AfModeAuto,\n> \t\t\t\"AfModeAuto\",\n> \t\t\t\"automatic-auto-focus\",\n> \t\t},\n> \t\t{\n> -\t\t\tstatic_cast(libcamera::controls::AfModeContinuous),\n> +\t\t\tlibcamera::controls::AfModeContinuous,\n> \t\t\t\"AfModeContinuous\",\n> \t\t\t\"continuous-auto-focus\",\n> \t\t},","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 67192C3237\n\tfor ;\n\tMon, 26 Jun 2023 19:20:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B08E7628C0;\n\tMon, 26 Jun 2023 21:20:47 +0200 (CEST)","from mail-vs1-xe33.google.com (mail-vs1-xe33.google.com\n\t[IPv6:2607:f8b0:4864:20::e33])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6343A628BB\n\tfor ;\n\tMon, 26 Jun 2023 21:20:46 +0200 (CEST)","by mail-vs1-xe33.google.com with SMTP id\n\tada2fe7eead31-440d1ba5662so940579137.2\n\tfor ;\n\tMon, 26 Jun 2023 12:20:46 -0700 (PDT)","from nicolas-tpx395.localdomain ([2606:6d00:15:c623::7a9])\n\tby smtp.gmail.com with ESMTPSA id\n\ts1-20020a0cf781000000b006261e6a88c7sm3463566qvn.36.2023.06.26.12.20.42\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 26 Jun 2023 12:20:43 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1687807247;\n\tbh=C9QcAPDbrWm/6ADTwSX5yoJio9fotpxK2j+NrKY6evk=;\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=Vb071//fU/ORqvFA+zDPGRI9iWRuGolcrpv+J2tgkSeHLIXJgWVdWSEBm7DjhMsMJ\n\tbWPiuH8S0nsMflTh4DpK+2WEFSVs1JAso++lNrXT7E4sJlB/HRGjne03urqrfyToK5\n\tR2DvC7T/t2VmM14LS3RMQos/JneezORVqCRKyPcaHWi12Cg23tfkIW/kJknX3rka22\n\ty7TiCL2Vd3Y5Jsmiw75uhc1OfQVpJTkSKE1B4hxzVrLar8nPavPKuaL0GcQ77bue9W\n\tH4MIkhwf/ILI7mgT/u1eWr2sn9+wGPpfzRrtsGW0bciB/ov49pM2LA/vXxS8NsyyBk\n\teJpAyDNaQR8+Q==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20221208.gappssmtp.com; s=20221208; t=1687807245;\n\tx=1690399245; \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=T4hN3GMoHa7vlldxLbBB4vkFPlLXCB9UmVQRKlo5vkU=;\n\tb=VEZYf/Qc3V8gdZmTEsABi5AuAJT621++l8u+jMSAXX//xkAOJ1aoXvBRTpRpp7hp8D\n\tOfvSecnmuON9psanvd2KVG4RiIF0stxjogDW821d7++y/koEg4N5f9fRY2HimjYCQh9u\n\tJcUhBTRe7V2Hcia+q0ovMEgQKnc4qTwwAcX46k2+IOKdd3e4vnO0AG0rnhFXdgsOBomM\n\tvztAvJBTmiAj0GR/SEaMqzUJqBAR7QcolheHxodPUKqNbEk4E+cRqZaeMu/P7+P/gLxh\n\tz6ivLcrWSD5mUH1WfncrSsqeSPuBBc4AeofZwSIXiCcjFTKlMNxyYf2hhy5a0xg6GDVA\n\tH6fg=="],"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=\"VEZYf/Qc\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1687807245; x=1690399245;\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=T4hN3GMoHa7vlldxLbBB4vkFPlLXCB9UmVQRKlo5vkU=;\n\tb=iQKOk3sqG64vjtaddNiqZpmZQkIX+2Bk4n2J04AEPPo/uep5GcVGr4+AJWLmz1fe+T\n\t7g5nT2+zm4XtBboOlvc20Dt6+vPV/z4h4T6QwFIEhEUD5EdQzwTgqGVMNBp/xI0SjcwD\n\tnnuzXqPhqGoj6h+is13ei3Op08hQz2WlgSQ5Fp5c4SWZYyOqSq/SneIWydwdI2ku65cV\n\tsdsn9v0GnnoV4i1XZLMlx4it1F1f9QbVxD09Gi7CFPbkWVrZ/+v34RXBYS1u1nGc7VB4\n\tt9sF14w1Rwa84hiYLO9+3LpdOP3owfG0TZMI6FYt4Wgz7SmzlE+v/RNOnoROrZLqzd8i\n\tYosQ==","X-Gm-Message-State":"AC+VfDzWE6zhuucDVfWZYqhMxolLNZ2dqNJ8oFnWat5y9iWBJcdLeghb\n\tkxZcqSKJ8CISaVptgHj4wIcNug==","X-Google-Smtp-Source":"ACHHUZ5RacS9G43kZM2+KWTELD1LbvNLTu1l3YD0wCAdgjnhhpw6OkaWINxJPRdTGUG37cjLCLyfDg==","X-Received":"by 2002:a67:ee48:0:b0:443:6549:3e18 with SMTP id\n\tg8-20020a67ee48000000b0044365493e18mr1353112vsp.18.1687807243581; \n\tMon, 26 Jun 2023 12:20:43 -0700 (PDT)","Message-ID":"<9599c74149b0cd025c5890741cdfff779547f3fa.camel@ndufresne.ca>","To":"Umang Jain , \n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 26 Jun 2023 15:20:42 -0400","In-Reply-To":"<20230626174752.71344-1-umang.jain@ideasonboard.com>","References":"<20230626174752.71344-1-umang.jain@ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","User-Agent":"Evolution 3.48.3 (3.48.3-1.fc38) ","MIME-Version":"1.0","Subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Maintain a control list\n\tto track 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":"Nicolas Dufresne via libcamera-devel\n\t","Reply-To":"Nicolas Dufresne ","Cc":"Cedric Nugteren ,\n\tCedric Nugteren ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27427,"web_url":"https://patchwork.libcamera.org/comment/27427/","msgid":"<148ed62d-fae9-e531-8799-bfbdc2899c9c@ideasonboard.com>","date":"2023-06-26T20:52:56","subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Maintain a control list\n\tto track libcamerasrc control properties","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Nicolas,\n\nOn 6/26/23 12:50 AM, Nicolas Dufresne wrote:\n> Le lundi 26 juin 2023 à 19:47 +0200, Umang Jain a écrit :\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, fail negotiation and report\n>> what is 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>> Changes in v2:\n>> - Edit commit message\n>> - Drop a break; so that all unsupported user-provided controls are\n>> reported at once.\n>> ---\n>> src/gstreamer/gstlibcamerasrc.cpp | 44 ++++++++++++++++++++++---------\n>> src/gstreamer/gstlibcamerasrc.h | 6 ++---\n>> 2 files changed, 35 insertions(+), 15 deletions(-)\n>>\n>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n>> index 47d8ff43..11f15068 100644\n>> --- a/src/gstreamer/gstlibcamerasrc.cpp\n>> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n>> @@ -146,7 +146,7 @@ struct _GstLibcameraSrc {\n>> \tGstTask *task;\n>> \n>> \tgchar *camera_name;\n>> -\tcontrols::AfModeEnum auto_focus_mode = controls::AfModeManual;\n>> +\tControlList *controls;\n>> \n>> \tGstLibcameraSrcState *state;\n>> \tGstLibcameraAllocator *allocator;\n>> @@ -462,7 +462,9 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n>> \tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n>> \tGLibRecLocker lock(&self->stream_lock);\n>> \tGstLibcameraSrcState *state = self->state;\n>> +\tconst ControlInfoMap &info_map = state->cam_->controls();\n>> \tGstFlowReturn flow_ret = GST_FLOW_OK;\n>> +\tgboolean supported_ctrl = true;\n>> \tgint ret;\n>> \n>> \tg_autoptr(GstStructure) element_caps = gst_structure_new_empty(\"caps\");\n>> @@ -579,18 +581,30 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n>> \t\tgst_flow_combiner_add_pad(self->flow_combiner, srcpad);\n>> \t}\n>> \n>> -\tif (self->auto_focus_mode != controls::AfModeManual) {\n>> -\t\tconst ControlInfoMap &infoMap = state->cam_->controls();\n>> -\t\tif (infoMap.find(&controls::AfMode) != infoMap.end()) {\n>> -\t\t\tstate->initControls_.set(controls::AfMode, self->auto_focus_mode);\n>> -\t\t} else {\n>> +\t/*\n>> +\t * Check the controls set on libcamerasrc is supported by the camera.\n>> +\t * This can be easily done by CameraControlValidator but it is an internal\n>> +\t * libcamera class. Hence avoid adding a internal header even though\n>> +\t * gstreamer links with libcamera-private.\n> I don't remember why GStreamer links to that, and why we should accept this as a\n> fact. The longer term plan, when libcamera is stable, was to migrate this\n> element into GStreamer project, but such thing will prevent it.\n\nIt's one of the mutex lock used in struct GstLibcameraSrcState because \n is part of private [1]\n\nI can  replace it with GMutex and GLibLocker right away but we will \nprobably lose clang's thread annotation there itself.\n\n[1] \nhttps://git.libcamera.org/libcamera/libcamera.git/commit/?id=777b0e0a655cce258a2b11e98546c3fc5a5be031\n>\n>> +\t */\n>> +\tfor (auto iter = self->controls->begin(); iter != self->controls->end(); iter++) {\n>> +\t\tif (info_map.find(iter->first) == info_map.end()) {\n>> +\t\t\tconst ControlIdMap *id_map = self->controls->idMap();\n>> \t\t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n>> -\t\t\t\t\t (\"Failed to enable auto focus\"),\n>> -\t\t\t\t\t (\"AfMode not supported by this camera, \"\n>> -\t\t\t\t\t \"please retry with 'auto-focus-mode=AfModeManual'\"));\n>> +\t\t\t\t\t (\"Failed to set %s\", id_map->at(iter->first)->name().c_str()),\n>> +\t\t\t\t\t (\"%s not supported by this camera, \", id_map->at(iter->first)->name().c_str()));\n>> +\t\t\tsupported_ctrl = false;\n>> \t\t}\n>> \t}\n>> \n>> +\tif (!supported_ctrl) {\n>> +\t\tgst_task_stop(task);\n>> +\t\tflow_ret = GST_FLOW_NOT_NEGOTIATED;\n>> +\t\tgoto done;\n>> +\t}\n>> +\n>> +\tstate->initControls_.merge(*self->controls);\n>> +\n>> \tret = state->cam_->start(&state->initControls_);\n>> \tif (ret) {\n>> \t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n>> @@ -674,7 +688,8 @@ gst_libcamera_src_set_property(GObject *object, guint prop_id,\n>> \t\tself->camera_name = g_value_dup_string(value);\n>> \t\tbreak;\n>> \tcase PROP_AUTO_FOCUS_MODE:\n>> -\t\tself->auto_focus_mode = static_cast(g_value_get_enum(value));\n>> +\t\tself->controls->set(controls::AfMode,\n>> +\t\t\t\t static_cast(g_value_get_enum(value)));\n>> \t\tbreak;\n>> \tdefault:\n>> \t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n>> @@ -693,9 +708,11 @@ gst_libcamera_src_get_property(GObject *object, guint prop_id, GValue *value,\n>> \tcase PROP_CAMERA_NAME:\n>> \t\tg_value_set_string(value, self->camera_name);\n>> \t\tbreak;\n>> -\tcase PROP_AUTO_FOCUS_MODE:\n>> -\t\tg_value_set_enum(value, static_cast(self->auto_focus_mode));\n>> +\tcase PROP_AUTO_FOCUS_MODE: {\n>> +\t\tauto auto_focus_mode = self->controls->get(controls::AfMode).value_or(controls::AfModeManual);\n>> +\t\tg_value_set_enum(value, auto_focus_mode);\n>> \t\tbreak;\n>> +\t}\n>> \tdefault:\n>> \t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n>> \t\tbreak;\n>> @@ -760,6 +777,7 @@ gst_libcamera_src_finalize(GObject *object)\n>> \tg_rec_mutex_clear(&self->stream_lock);\n>> \tg_clear_object(&self->task);\n>> \tg_free(self->camera_name);\n>> +\tdelete self->controls;\n>> \tdelete self->state;\n>> \n>> \treturn klass->finalize(object);\n>> @@ -783,6 +801,8 @@ gst_libcamera_src_init(GstLibcameraSrc *self)\n>> \t/* C-style friend. */\n>> \tstate->src_ = self;\n>> \tself->state = state;\n>> +\n>> +\tself->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>> \tstatic GType type = 0;\n>> \tstatic const GEnumValue values[] = {\n>> \t\t{\n>> -\t\t\tstatic_cast(libcamera::controls::AfModeManual),\n>> +\t\t\tlibcamera::controls::AfModeManual,\n>> \t\t\t\"AfModeManual\",\n>> \t\t\t\"manual-focus\",\n>> \t\t},\n>> \t\t{\n>> -\t\t\tstatic_cast(libcamera::controls::AfModeAuto),\n>> +\t\t\tlibcamera::controls::AfModeAuto,\n>> \t\t\t\"AfModeAuto\",\n>> \t\t\t\"automatic-auto-focus\",\n>> \t\t},\n>> \t\t{\n>> -\t\t\tstatic_cast(libcamera::controls::AfModeContinuous),\n>> +\t\t\tlibcamera::controls::AfModeContinuous,\n>> \t\t\t\"AfModeContinuous\",\n>> \t\t\t\"continuous-auto-focus\",\n>> \t\t},","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 8AF4BBE175\n\tfor ;\n\tMon, 26 Jun 2023 20:53:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C0099628BC;\n\tMon, 26 Jun 2023 22:53:00 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5F11B628BB\n\tfor ;\n\tMon, 26 Jun 2023 22:52:59 +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 3045F2D8;\n\tMon, 26 Jun 2023 22:52:20 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1687812780;\n\tbh=wm9l9XD2fADMG4H8lVI2odTbH1mvLQA3GUPamz0pkcA=;\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=rfXwXc5fcR6PBM86Y+rsKJcyjgNjCXyw8QBjk7w45RPj+FseDgEj2ilzcdZkBy+Jo\n\tTc3TgkzZHoQR/tdCQJv4Xka+dRW8/qX1wJaeHXTkuK3pe6Vq0sFXnaURLqZrDFxWgr\n\tnDTiCV2eUGfxriEcmFy7sz/lDiit8a/73rsI/BWZdobyvAurtRvn4BF1SduigIv7tk\n\tF7J4AuxA91aAiX12CYdm5Y36Wty8ZvtOUTLQ1SYttb5t1bhhrQQckK+iBGz1EVvxZH\n\t5Wh0R6cgMC+fBaC7GniPI4w9/+8vN6A1jgtrIe8AprvRSnpzlpSst/GVMHI+kFqqrq\n\ts7Nl+llFYSv4A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1687812740;\n\tbh=wm9l9XD2fADMG4H8lVI2odTbH1mvLQA3GUPamz0pkcA=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=Cd0gAFJIvS3ygILfs6B8otgVlwS5TCJA0u4nZTn0WXC+vUTGqPSlADOd5cqhPAYza\n\tVRnHl2pLNluIiX3Rgt6dFe6EsYY+LLH6IciEL2c6GKhzOB0Gfwofm/3YXvobdjkPRX\n\t8k2P0A0kOe5YNF4vGSRVmlyrnXRph20HYnKlO7UE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Cd0gAFJI\"; dkim-atps=neutral","Message-ID":"<148ed62d-fae9-e531-8799-bfbdc2899c9c@ideasonboard.com>","Date":"Mon, 26 Jun 2023 22:52:56 +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":"Nicolas Dufresne ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20230626174752.71344-1-umang.jain@ideasonboard.com>\n\t<9599c74149b0cd025c5890741cdfff779547f3fa.camel@ndufresne.ca>","In-Reply-To":"<9599c74149b0cd025c5890741cdfff779547f3fa.camel@ndufresne.ca>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Maintain a control list\n\tto track 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":27430,"web_url":"https://patchwork.libcamera.org/comment/27430/","msgid":"<14ccd3d9ac119a375729aa81b0f11fd856bd6dd6.camel@ndufresne.ca>","date":"2023-06-28T14:21:27","subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Maintain a control list\n\tto track libcamerasrc control properties","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Le lundi 26 juin 2023 à 22:52 +0200, Umang Jain a écrit :\n> Hi Nicolas,\n> \n> On 6/26/23 12:50 AM, Nicolas Dufresne wrote:\n> > Le lundi 26 juin 2023 à 19:47 +0200, Umang Jain a écrit :\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, fail negotiation and report\n> > > what is 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> > > Changes in v2:\n> > > - Edit commit message\n> > > - Drop a break; so that all unsupported user-provided controls are\n> > > reported at once.\n> > > ---\n> > > src/gstreamer/gstlibcamerasrc.cpp | 44 ++++++++++++++++++++++---------\n> > > src/gstreamer/gstlibcamerasrc.h | 6 ++---\n> > > 2 files changed, 35 insertions(+), 15 deletions(-)\n> > > \n> > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > > index 47d8ff43..11f15068 100644\n> > > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > > @@ -146,7 +146,7 @@ struct _GstLibcameraSrc {\n> > > \tGstTask *task;\n> > > \n> > > \tgchar *camera_name;\n> > > -\tcontrols::AfModeEnum auto_focus_mode = controls::AfModeManual;\n> > > +\tControlList *controls;\n> > > \n> > > \tGstLibcameraSrcState *state;\n> > > \tGstLibcameraAllocator *allocator;\n> > > @@ -462,7 +462,9 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> > > \tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n> > > \tGLibRecLocker lock(&self->stream_lock);\n> > > \tGstLibcameraSrcState *state = self->state;\n> > > +\tconst ControlInfoMap &info_map = state->cam_->controls();\n> > > \tGstFlowReturn flow_ret = GST_FLOW_OK;\n> > > +\tgboolean supported_ctrl = true;\n> > > \tgint ret;\n> > > \n> > > \tg_autoptr(GstStructure) element_caps = gst_structure_new_empty(\"caps\");\n> > > @@ -579,18 +581,30 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> > > \t\tgst_flow_combiner_add_pad(self->flow_combiner, srcpad);\n> > > \t}\n> > > \n> > > -\tif (self->auto_focus_mode != controls::AfModeManual) {\n> > > -\t\tconst ControlInfoMap &infoMap = state->cam_->controls();\n> > > -\t\tif (infoMap.find(&controls::AfMode) != infoMap.end()) {\n> > > -\t\t\tstate->initControls_.set(controls::AfMode, self->auto_focus_mode);\n> > > -\t\t} else {\n> > > +\t/*\n> > > +\t * Check the controls set on libcamerasrc is supported by the camera.\n> > > +\t * This can be easily done by CameraControlValidator but it is an internal\n> > > +\t * libcamera class. Hence avoid adding a internal header even though\n> > > +\t * gstreamer links with libcamera-private.\n> > I don't remember why GStreamer links to that, and why we should accept this as a\n> > fact. The longer term plan, when libcamera is stable, was to migrate this\n> > element into GStreamer project, but such thing will prevent it.\n> \n> It's one of the mutex lock used in struct GstLibcameraSrcState because \n> is part of private [1]\n> \n> I can  replace it with GMutex and GLibLocker right away but we will \n> probably lose clang's thread annotation there itself.\n> \n> [1] \n> https://git.libcamera.org/libcamera/libcamera.git/commit/?id=777b0e0a655cce258a2b11e98546c3fc5a5be031\n\nLet's move away from it and live by our choice to keep that private. We can\nannotate GLibLocker and make it a requirement to only use that, or annotated\nwrappers, but to be fair, I had no idea we had some kind of annotation and I've\nbeen building with GCC anyway, so it probably have never been used.\n\n> > \n> > > +\t */\n> > > +\tfor (auto iter = self->controls->begin(); iter != self->controls->end(); iter++) {\n> > > +\t\tif (info_map.find(iter->first) == info_map.end()) {\n> > > +\t\t\tconst ControlIdMap *id_map = self->controls->idMap();\n> > > \t\t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > > -\t\t\t\t\t (\"Failed to enable auto focus\"),\n> > > -\t\t\t\t\t (\"AfMode not supported by this camera, \"\n> > > -\t\t\t\t\t \"please retry with 'auto-focus-mode=AfModeManual'\"));\n> > > +\t\t\t\t\t (\"Failed to set %s\", id_map->at(iter->first)->name().c_str()),\n> > > +\t\t\t\t\t (\"%s not supported by this camera, \", id_map->at(iter->first)->name().c_str()));\n> > > +\t\t\tsupported_ctrl = false;\n> > > \t\t}\n> > > \t}\n> > > \n> > > +\tif (!supported_ctrl) {\n> > > +\t\tgst_task_stop(task);\n> > > +\t\tflow_ret = GST_FLOW_NOT_NEGOTIATED;\n> > > +\t\tgoto done;\n> > > +\t}\n> > > +\n> > > +\tstate->initControls_.merge(*self->controls);\n> > > +\n> > > \tret = state->cam_->start(&state->initControls_);\n> > > \tif (ret) {\n> > > \t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > > @@ -674,7 +688,8 @@ gst_libcamera_src_set_property(GObject *object, guint prop_id,\n> > > \t\tself->camera_name = g_value_dup_string(value);\n> > > \t\tbreak;\n> > > \tcase PROP_AUTO_FOCUS_MODE:\n> > > -\t\tself->auto_focus_mode = static_cast(g_value_get_enum(value));\n> > > +\t\tself->controls->set(controls::AfMode,\n> > > +\t\t\t\t static_cast(g_value_get_enum(value)));\n> > > \t\tbreak;\n> > > \tdefault:\n> > > \t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > > @@ -693,9 +708,11 @@ gst_libcamera_src_get_property(GObject *object, guint prop_id, GValue *value,\n> > > \tcase PROP_CAMERA_NAME:\n> > > \t\tg_value_set_string(value, self->camera_name);\n> > > \t\tbreak;\n> > > -\tcase PROP_AUTO_FOCUS_MODE:\n> > > -\t\tg_value_set_enum(value, static_cast(self->auto_focus_mode));\n> > > +\tcase PROP_AUTO_FOCUS_MODE: {\n> > > +\t\tauto auto_focus_mode = self->controls->get(controls::AfMode).value_or(controls::AfModeManual);\n> > > +\t\tg_value_set_enum(value, auto_focus_mode);\n> > > \t\tbreak;\n> > > +\t}\n> > > \tdefault:\n> > > \t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > > \t\tbreak;\n> > > @@ -760,6 +777,7 @@ gst_libcamera_src_finalize(GObject *object)\n> > > \tg_rec_mutex_clear(&self->stream_lock);\n> > > \tg_clear_object(&self->task);\n> > > \tg_free(self->camera_name);\n> > > +\tdelete self->controls;\n> > > \tdelete self->state;\n> > > \n> > > \treturn klass->finalize(object);\n> > > @@ -783,6 +801,8 @@ gst_libcamera_src_init(GstLibcameraSrc *self)\n> > > \t/* C-style friend. */\n> > > \tstate->src_ = self;\n> > > \tself->state = state;\n> > > +\n> > > +\tself->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> > > \tstatic GType type = 0;\n> > > \tstatic const GEnumValue values[] = {\n> > > \t\t{\n> > > -\t\t\tstatic_cast(libcamera::controls::AfModeManual),\n> > > +\t\t\tlibcamera::controls::AfModeManual,\n> > > \t\t\t\"AfModeManual\",\n> > > \t\t\t\"manual-focus\",\n> > > \t\t},\n> > > \t\t{\n> > > -\t\t\tstatic_cast(libcamera::controls::AfModeAuto),\n> > > +\t\t\tlibcamera::controls::AfModeAuto,\n> > > \t\t\t\"AfModeAuto\",\n> > > \t\t\t\"automatic-auto-focus\",\n> > > \t\t},\n> > > \t\t{\n> > > -\t\t\tstatic_cast(libcamera::controls::AfModeContinuous),\n> > > +\t\t\tlibcamera::controls::AfModeContinuous,\n> > > \t\t\t\"AfModeContinuous\",\n> > > \t\t\t\"continuous-auto-focus\",\n> > > \t\t},\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 A8A7BBE175\n\tfor ;\n\tWed, 28 Jun 2023 14:21:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0AE05628C0;\n\tWed, 28 Jun 2023 16:21:32 +0200 (CEST)","from mail-qk1-x72e.google.com (mail-qk1-x72e.google.com\n\t[IPv6:2607:f8b0:4864:20::72e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7433A61E3B\n\tfor ;\n\tWed, 28 Jun 2023 16:21:30 +0200 (CEST)","by mail-qk1-x72e.google.com with SMTP id\n\taf79cd13be357-763a2e39b88so568082885a.1\n\tfor ;\n\tWed, 28 Jun 2023 07:21:30 -0700 (PDT)","from nicolas-tpx395.localdomain ([2606:6d00:15:c623::7a9])\n\tby smtp.gmail.com with ESMTPSA id\n\tm18-20020ae9e012000000b0075d49ce31c3sm3485385qkk.91.2023.06.28.07.21.28\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 28 Jun 2023 07:21:28 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1687962092;\n\tbh=pXKH/i5kIAXI0mcJNC1PBKOyeACR6PBTbJl1BGLjxBA=;\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=p84seMlzVR55VLpin7NSr/VcHEi3wQlfj71lKSbG0YeCEQ9+GSLatHF2TNfmlNWfE\n\t4aHcJohMNBhriDINAus5cpi23tgfjOxu+0xpoMoZ5tTTdemdE0inVYHxc22cmmfaro\n\tPHT9ClHqf+QgHgBB6sJa4fvntQ9QAiI5xGVXZY3hZX1oCCQpvxx3hBpNGSzEVCobjI\n\t+s5sYrBXVmYRd+RRifPbBbjtZDlvO2l9g/vuUrRc0N5wwFm/WiIoke+ypGNugGjAWU\n\tgjctFbiDFox/6lBKalkhHqLfjcbsNvlyd+d31jw++KyIBvWc5uv4TzuuLRO8O6G4+k\n\t5TmeeOJTIfNww==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20221208.gappssmtp.com; s=20221208; t=1687962089;\n\tx=1690554089; \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=+fbhM9g76ejC4R+BgsFirC1VBaMfGQR8kRIp4QPKyL4=;\n\tb=5CaTwKB/N8hLoE454g1szlgXHbwrrphph9YrJwD2O9OT5LLzCe6rozBp+D6DOSS7yy\n\taSBFohULYk+w16HfQTOrkWQ7YfsbcZmDnHMS8bVlJkB9Wws0ixtw0+OG43Sz74Zuw44w\n\tKB1Hg/RzrqgH3vnbp1cxJ1m/XQVy1XYOGrdMmS6iwe5JxrCUDuQND6q9BRLgaThfnIsm\n\t0vLDnsjiHQ7zsSRX3XlLpSuXvrm8veGcIolCx548yCOnVLDSXOi14HMCj1SfdMfXRJNN\n\trr9JIzbZKO/GUbs3gOpMVkY+OSyV3e0T5qonATHq/HXKVmZx8EFMV+dKA5jPl+r+faFq\n\tledQ=="],"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=\"5CaTwKB/\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1687962089; x=1690554089;\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=+fbhM9g76ejC4R+BgsFirC1VBaMfGQR8kRIp4QPKyL4=;\n\tb=ZT5agfoDtAixNzUef0jBNAE21ReNwC4ZhD39ZgaWxWKpoQ9gl3xTAFyUOQhkffJTMp\n\tEjOh038zBb0VoD67FjrxX+YitTJaq2dttBKRXR6Q/ey6DnDvxNYACLNLTPEz2g6f3Bi9\n\t3BkQcvx3o//kO3btWX8STRnlcJN8YfWXF8J764hf5oWjDK0AENUGHSrpCs4qMHP5dGd0\n\tnuW90JZVkPfzMYjH3ytUY6zBr0lGk1IRL7I6NZbQ06MpQ5uQfokSFxoPLsmEelUhpKB9\n\tTQiLSRxVxkwGc0+K6CkVOX3x1GxPZP3eSobm2n0cz75ByLlA0ia0l5n+eqpyVMR4xqS+\n\tRjtA==","X-Gm-Message-State":"AC+VfDy9ZzoGic/++abp0MzW7nPDhLzjgXastNgkTH8TnjdfAshhXri0\n\t2DOfwEu3y+bvw9Lqzzq42NQ6Dw==","X-Google-Smtp-Source":"ACHHUZ5s/WLbVka3qZD25bYzbFKWpRnQUBipKt9CZPtHWi+TDifjNQ9kekX4rAA1aIZkehBlHQbPgw==","X-Received":"by 2002:a05:620a:192a:b0:765:50f5:2e2a with SMTP id\n\tbj42-20020a05620a192a00b0076550f52e2amr18931372qkb.36.1687962089182; \n\tWed, 28 Jun 2023 07:21:29 -0700 (PDT)","Message-ID":"<14ccd3d9ac119a375729aa81b0f11fd856bd6dd6.camel@ndufresne.ca>","To":"Umang Jain , \n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 28 Jun 2023 10:21:27 -0400","In-Reply-To":"<148ed62d-fae9-e531-8799-bfbdc2899c9c@ideasonboard.com>","References":"<20230626174752.71344-1-umang.jain@ideasonboard.com>\n\t<9599c74149b0cd025c5890741cdfff779547f3fa.camel@ndufresne.ca>\n\t<148ed62d-fae9-e531-8799-bfbdc2899c9c@ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","User-Agent":"Evolution 3.48.3 (3.48.3-1.fc38) ","MIME-Version":"1.0","Subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Maintain a control list\n\tto track 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":"Nicolas Dufresne via libcamera-devel\n\t","Reply-To":"Nicolas Dufresne ","Cc":"Cedric Nugteren ,\n\tCedric Nugteren ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27435,"web_url":"https://patchwork.libcamera.org/comment/27435/","msgid":"<168799349143.3298351.185837055942079632@Monstersaurus>","date":"2023-06-28T23:04:51","subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Maintain a control list\n\tto track libcamerasrc control properties","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Nicolas Dufresne via libcamera-devel (2023-06-28 15:21:27)\n> Le lundi 26 juin 2023 à 22:52 +0200, Umang Jain a écrit :\n> > Hi Nicolas,\n> > \n> > On 6/26/23 12:50 AM, Nicolas Dufresne wrote:\n> > > Le lundi 26 juin 2023 à 19:47 +0200, Umang Jain a écrit :\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, fail negotiation and report\n> > > > what is 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> > > > Changes in v2:\n> > > > - Edit commit message\n> > > > - Drop a break; so that all unsupported user-provided controls are\n> > > > reported at once.\n> > > > ---\n> > > > src/gstreamer/gstlibcamerasrc.cpp | 44 ++++++++++++++++++++++---------\n> > > > src/gstreamer/gstlibcamerasrc.h | 6 ++---\n> > > > 2 files changed, 35 insertions(+), 15 deletions(-)\n> > > > \n> > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > > > index 47d8ff43..11f15068 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,30 @@ 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> > > I don't remember why GStreamer links to that, and why we should accept this as a\n> > > fact. The longer term plan, when libcamera is stable, was to migrate this\n> > > element into GStreamer project, but such thing will prevent it.\n> > \n> > It's one of the mutex lock used in struct GstLibcameraSrcState because \n> > is part of private [1]\n> > \n> > I can  replace it with GMutex and GLibLocker right away but we will \n> > probably lose clang's thread annotation there itself.\n> > \n> > [1] \n> > https://git.libcamera.org/libcamera/libcamera.git/commit/?id=777b0e0a655cce258a2b11e98546c3fc5a5be031\n> \n> Let's move away from it and live by our choice to keep that private. We can\n> annotate GLibLocker and make it a requirement to only use that, or annotated\n> wrappers, but to be fair, I had no idea we had some kind of annotation and I've\n> been building with GCC anyway, so it probably have never been used.\n\nThe annotations are checked at compile time I believe and we include\nclang in our integration compiler matrix, so the macros have been 'used'\nbut it's fine to drop them here I believe.\n\n--\nKieran\n\n\n> \n> > > \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> > > > + supported_ctrl = false;\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 +688,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 +708,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 +777,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 +801,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> > > > \"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>","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 CB2E7BDB1D\n\tfor ;\n\tWed, 28 Jun 2023 23:04:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 31C65628BC;\n\tThu, 29 Jun 2023 01:04:56 +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 00FDB61E3F\n\tfor ;\n\tThu, 29 Jun 2023 01:04:54 +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 4EFB5289;\n\tThu, 29 Jun 2023 01:04:14 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1687993496;\n\tbh=2yo4YYKq+/L9xBbw2WFALAPoyNjGmuYoOgHiHJcAlBA=;\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=G56BumEwHyBdXPodTuYW7B2hVwZ3SswlTeGMNvQIznkIeTYIYNbpQI5KKKQ9EVC5B\n\t39ttUpya5ENUX3BmQQpvomErsC7b4wdz+7RBV52W7aKFWCrA6Dgechs0ve6zzB4nUN\n\tJygrcqFF+qprzY2hJ87BYL8+2m33GjxlEA+bbxRjsCUAgkEFBL6cAcDn+1orA3oQFK\n\tmXGTELLSilAWMCSbn3gI17EC+e0jGb/4XtLPwPS8CAAdy6LOn3uvLKx3X9MvQlwD8o\n\tCByletGqhXQVKEkxhljhQxRQYtfsPn6o+LA9qKwv3cUlx7KeJIJa2p7Z3ph8j6Dcem\n\tiRyJqLUJmnk6Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1687993454;\n\tbh=2yo4YYKq+/L9xBbw2WFALAPoyNjGmuYoOgHiHJcAlBA=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=VL7NUXQCaBzdAStGfchqL3JAC696EHMXOxGPyXmALx3WIz0a5MBeRekq/yFxTdZDE\n\tlnO/sgTSID7yGoXzLp5pT1v0M0Guesbs/r38kMft5PeH+6UllQ5c8FGACqDek0SD6q\n\tNJNMQHlya4/dCQHLr2JMibvem6034AJ+K37eJJpA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"VL7NUXQC\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<14ccd3d9ac119a375729aa81b0f11fd856bd6dd6.camel@ndufresne.ca>","References":"<20230626174752.71344-1-umang.jain@ideasonboard.com>\n\t<9599c74149b0cd025c5890741cdfff779547f3fa.camel@ndufresne.ca>\n\t<148ed62d-fae9-e531-8799-bfbdc2899c9c@ideasonboard.com>\n\t<14ccd3d9ac119a375729aa81b0f11fd856bd6dd6.camel@ndufresne.ca>","To":"Nicolas Dufresne ,\n\tUmang Jain ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 29 Jun 2023 00:04:51 +0100","Message-ID":"<168799349143.3298351.185837055942079632@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Maintain a control list\n\tto track 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\" "}}]