[{"id":24623,"web_url":"https://patchwork.libcamera.org/comment/24623/","msgid":"<4283cb4a5fb915870ed3775a35f78e69470026b1.camel@collabora.com>","date":"2022-08-17T18:02:04","subject":"Re: [libcamera-devel] [PATCH v4 1/1] gstreamer: Provide mulitple\n\tcolorimetry support","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Hi Rishi,\n\nI like this version, I've provided some very minor feedback. With these\nimproved:\n\nReviewed-by: Nicolas Dufresne \n\nLe mercredi 17 août 2022 à 22:55 +0530, Rishikesh Donadkar a écrit :\n> Iterate over each GValue in the colorimetry list,\n> retrieve the colorimetry string, convert to colorspace using the\n> function colorspace_from_colorimetry() and validate the camera configuration.\n> Retrieve the colorspace after validation, convert to colorimetry and\n> check if the it is same as the colorimetry requested.\n> \n> If none of the colorimetry requested is supported by the camera (i.e. not\n> the same after validation) then set the stream_cfg to the previous\n> configuration that was present before trying new colorimetry.\n> \n> Signed-off-by: Rishikesh Donadkar \n> Signed-off-by: Umang Jain \n> ---\n> src/gstreamer/gstlibcamera-utils.cpp | 75 +++++++++++++++++++++-------\n> src/gstreamer/gstlibcamera-utils.h | 4 +-\n> src/gstreamer/gstlibcamerasrc.cpp | 2 +-\n> 3 files changed, 62 insertions(+), 19 deletions(-)\n> \n> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> index d895a67d..2a88444c 100644\n> --- a/src/gstreamer/gstlibcamera-utils.cpp\n> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> @@ -302,13 +302,39 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg\n> \treturn caps;\n> }\n> \n> +static int\n\nLooking at the code, it seems like this is a boolean, not sure if hiding that\nfact behind a int is usual in libcamera, but wanted to mention this looks a\nlittle strange.\n\n> +configure_colorspace_from_caps(StreamConfiguration &stream_cfg,\n> +\t\t\t const gchar *colorimetry_str)\n> +{\n> +\tgint colorimetry_valid = 1, colorimetry_invalid = 0;\n> +\tif (colorimetry_str) {\n> +\t\tGstVideoColorimetry colorimetry;\n> +\n> +\t\tif (!gst_video_colorimetry_from_string(&colorimetry, colorimetry_str)) {\n> +\t\t\tg_critical(\"Invalid colorimetry %s\", colorimetry_str);\n> +\t\t\treturn colorimetry_invalid;\n> +\t\t}\n> +\n> +\t\tstream_cfg.colorSpace = colorspace_from_colorimetry(colorimetry);\n> +\t\t/* Check if colorimetry had any identifiers which did not map */\n> +\t\tif (colorimetry.primaries != GST_VIDEO_COLOR_PRIMARIES_UNKNOWN &&\n> +\t\t stream_cfg.colorSpace == ColorSpace::Raw) {\n> +\t\t\tGST_ERROR(\"One or more identifiers could not be mapped for %s colorimetry\",\n> +\t\t\t\t colorimetry_str);\n> +\t\t\tstream_cfg.colorSpace = std::nullopt;\n> +\t\t}\n> +\t}\n> +\treturn colorimetry_valid;\n\nThe variable does not seem to be written too, compiler will optimize it out, but\nstill, I'd return true/false, or use constant or defines if readability was your\ngoal.\n\n> +}\n> +\n> void\n> -gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,\n> +gst_libcamera_configure_stream_from_caps(std::unique_ptr &cam_cfg,\n> +\t\t\t\t\t StreamConfiguration &stream_cfg,\n> \t\t\t\t\t GstCaps *caps)\n> {\n> \tGstVideoFormat gst_format = pixel_format_to_gst_format(stream_cfg.pixelFormat);\n> \tguint i;\n> -\tgint best_fixed = -1, best_in_range = -1;\n> +\tgint best_fixed = -1, best_in_range = -1, ret;\n> \tGstStructure *s;\n> \n> \t/*\n> @@ -381,23 +407,38 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,\n> \tstream_cfg.size.width = width;\n> \tstream_cfg.size.height = height;\n> \n> -\t/* Configure colorimetry */\n> -\tif (gst_structure_has_field(s, \"colorimetry\")) {\n> -\t\tconst gchar *colorimetry_str = gst_structure_get_string(s, \"colorimetry\");\n> -\t\tGstVideoColorimetry colorimetry;\n> -\n> -\t\tif (!gst_video_colorimetry_from_string(&colorimetry, colorimetry_str))\n> -\t\t\tg_critical(\"Invalid colorimetry %s\", colorimetry_str);\n> -\n> -\t\tstream_cfg.colorSpace = colorspace_from_colorimetry(colorimetry);\n> -\t\t/* Check if colorimetry had any identifiers which did not map */\n> -\t\tif (colorimetry.primaries != GST_VIDEO_COLOR_PRIMARIES_UNKNOWN &&\n> -\t\t stream_cfg.colorSpace == ColorSpace::Raw) {\n> -\t\t\tGST_ERROR(\"One or more identifiers could not be mapped for %s colorimetry\",\n> -\t\t\t\t colorimetry_str);\n> -\t\t\tstream_cfg.colorSpace = std::nullopt;\n> +\tconst GValue *colorimetry = gst_structure_get_value(s, \"colorimetry\");\n> +\tif (!colorimetry)\n> +\t\treturn;\n> +\n> +\t/* Configure Colorimetry */\n> +\tif (GST_VALUE_HOLDS_LIST(colorimetry)) {\n> +\t\tStreamConfiguration pristine_stream_cfg = stream_cfg;\n> +\t\tfor (i = 0; i < gst_value_list_get_size(colorimetry); i++) {\n> +\t\t\tconst GValue *val = gst_value_list_get_value(colorimetry, i);\n> +\t\t\tGstVideoColorimetry colorimetry_old, colorimetry_new;\n> +\n> +\t\t\tret = configure_colorspace_from_caps(stream_cfg, g_value_get_string(val));\n> +\t\t\tif (!ret)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tcolorimetry_old = colorimetry_from_colorspace(stream_cfg.colorSpace.value());\n> +\n> +\t\t\t/* Validate the configuration and check if the requested\n> +\t\t\t * colorimetry can be applied to the sensor.\n> +\t\t\t */\n> +\t\t\tif (cam_cfg->validate() != CameraConfiguration::Invalid) {\n> +\t\t\t\tcolorimetry_new = colorimetry_from_colorspace(stream_cfg.colorSpace.value());\n> +\t\t\t\tif (gst_video_colorimetry_is_equal(&colorimetry_old, &colorimetry_new)) {\n> +\t\t\t\t\tg_print(\"Selected colorimetry %s\\n\", gst_video_colorimetry_to_string(&colorimetry_new));\n> +\t\t\t\t\tbreak;\n> +\t\t\t\t} else\n> +\t\t\t\t\tstream_cfg = pristine_stream_cfg;\n> +\t\t\t}\n> \t\t}\n> \t}\n> +\tif (G_VALUE_TYPE(colorimetry) == G_TYPE_STRING)\n\nnit: Alternatively, G_VALUE_HOLDS_STRING(colorimetry). This branch could be\n\"else if\", as it mutually exclusive to the previous branch. A final else could\nbe nice to trace that something is wrong, colorimetry field type should only be\nstring or list, anything else is user an error (though coming from outside the\nelement/plugin).\n\n> +\t\tconfigure_colorspace_from_caps(stream_cfg, g_value_get_string(colorimetry));\n> }\n> \n> #if !GST_CHECK_VERSION(1, 17, 1)\n> diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h\n> index 164189a2..7afad576 100644\n> --- a/src/gstreamer/gstlibcamera-utils.h\n> +++ b/src/gstreamer/gstlibcamera-utils.h\n> @@ -8,6 +8,7 @@\n> \n> #pragma once\n> \n> +#include \n> #include \n> #include \n> \n> @@ -16,7 +17,8 @@\n> \n> GstCaps *gst_libcamera_stream_formats_to_caps(const libcamera::StreamFormats &formats);\n> GstCaps *gst_libcamera_stream_configuration_to_caps(const libcamera::StreamConfiguration &stream_cfg);\n> -void gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration &stream_cfg,\n> +void gst_libcamera_configure_stream_from_caps(std::unique_ptr &cam_cfg,\n> +\t\t\t\t\t libcamera::StreamConfiguration &stream_cfg,\n> \t\t\t\t\t GstCaps *caps);\n> #if !GST_CHECK_VERSION(1, 17, 1)\n> gboolean gst_task_resume(GstTask *task);\n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index 16d70fea..25059914 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -503,7 +503,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> \n> \t\t/* Fixate caps and configure the stream. */\n> \t\tcaps = gst_caps_make_writable(caps);\n> -\t\tgst_libcamera_configure_stream_from_caps(stream_cfg, caps);\n> +\t\tgst_libcamera_configure_stream_from_caps(state->config_, stream_cfg, caps);\n> \t}\n> \n> \tif (flow_ret != GST_FLOW_OK)","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 5ABDDC3272\n\tfor ;\n\tWed, 17 Aug 2022 18:02:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B8CE661FC0;\n\tWed, 17 Aug 2022 20:02:16 +0200 (CEST)","from madras.collabora.co.uk (madras.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e5ab])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5011A61FA8\n\tfor ;\n\tWed, 17 Aug 2022 20:02:15 +0200 (CEST)","from nicolas-tpx395.localdomain (192-222-136-102.qc.cable.ebox.net\n\t[192.222.136.102])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\tkey-exchange X25519 server-signature RSA-PSS (4096 bits)\n\tserver-digest SHA256)\n\t(No client certificate requested) (Authenticated sender: nicolas)\n\tby madras.collabora.co.uk (Postfix) with ESMTPSA id 4A5DE6601948;\n\tWed, 17 Aug 2022 19:02:14 +0100 (BST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660759336;\n\tbh=BSXFyzTHsoQv8TJNjOHAXJvxK7VxJexpOO5nnHhK1mo=;\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=sk6u0rSzRnVvy4tbjaQuvD2NbA1knZd5tVAKbQ6agmeM3QFj9e2Tio0luogIVbOeG\n\tayAF8IH/ZBrgkwYFossjGRHzYdy7otZ+4m4mnPR2riNu7FA0FRpIjxDMHovSN0AY/S\n\t0PRnoxAv15bZnotuVhbDfgYMLnrns3NsYVINnTHi6czWiBeK+Stm9rL1B8mDEL9vHV\n\tUkyVn71c2z23B2MYq8vVnq+1tSJGBJMV1/FMqbaWPS8qpMIoCIx0QcZfx05Zr+dIuC\n\ty8q7JC6242KgoF416Xu6I0vuLKIzAjirWtYw8x+X9xJ5Ef/KQ8GdiLVKvtXHolRnVw\n\tITjs1YMY97ssQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com;\n\ts=mail; t=1660759335;\n\tbh=BSXFyzTHsoQv8TJNjOHAXJvxK7VxJexpOO5nnHhK1mo=;\n\th=Subject:From:To:Cc:Date:In-Reply-To:References:From;\n\tb=YtDGtfao0bs31JRM9HjpYWRmNNjm671xHqMRpk0gQkgF+UuPJawiGpaO7ELJ4cTDq\n\tz34nlKN65HHHYvNhfCSa+5rdOPu721kajlglRrVIyEKgXyMTLS+2GW03cVaVorC0Pe\n\tJDLxcFzHtD6lPIbloM56GetvSVYYyrM1geW/cwEgBLE+OdqWkc1nLV4kSVdZowTx+w\n\tPwayuQ14Qn1HlFb5S8lxzcONFz4qwdH08FkwEEE2b3DHwW9HYMmmr0hhML/apDMe8N\n\t3JtMaxaHXxfOJNMwFUR5R1X22EFcmGfsUAGDwBKm/3q/WsXv5h12+QYFxEMRh7fX7o\n\tF6gOKztl5G1DQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=collabora.com\n\theader.i=@collabora.com\n\theader.b=\"YtDGtfao\"; dkim-atps=neutral","Message-ID":"<4283cb4a5fb915870ed3775a35f78e69470026b1.camel@collabora.com>","To":"Rishikesh Donadkar , \n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 17 Aug 2022 14:02:04 -0400","In-Reply-To":"<20220817172541.72339-2-rishikeshdonadkar@gmail.com>","References":"<20220817172541.72339-1-rishikeshdonadkar@gmail.com>\n\t<20220817172541.72339-2-rishikeshdonadkar@gmail.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","User-Agent":"Evolution 3.44.4 (3.44.4-1.fc36) ","MIME-Version":"1.0","Subject":"Re: [libcamera-devel] [PATCH v4 1/1] gstreamer: Provide mulitple\n\tcolorimetry support","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":"vedantparanjape160201@gmail.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]