[{"id":25002,"web_url":"https://patchwork.libcamera.org/comment/25002/","msgid":"","date":"2022-09-19T17:52:15","subject":"Re: [libcamera-devel] [PATCH v6 1/1] gstreamer: Provide multiple\n\tcolorimetry support for libcamerasrc","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Rishikesh,\n\nThank you for the patch.\n\nOn Mon, Sep 19, 2022 at 08:27:09AM +0530, Rishikesh Donadkar wrote:\n> Iterate over each GValue in the colorimetry list,\n> retrieve the colorimetry string, convert to colorspace,\n> set the colorspace in the stream_cfg and validate the camera configuration.\n> Retrieve the colorspace after validation, convert to colorimetry and\n> check if 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> Reviewed-by: Nicolas Dufresne \n> ---\n> src/gstreamer/gstlibcamera-utils.cpp | 49 +++++++++++++++++++++++-----\n> src/gstreamer/gstlibcamera-utils.h | 4 ++-\n> src/gstreamer/gstlibcamerasrc.cpp | 2 +-\n> 3 files changed, 44 insertions(+), 11 deletions(-)\n> \n> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> index 244a4a79..f73b6349 100644\n> --- a/src/gstreamer/gstlibcamera-utils.cpp\n> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> @@ -316,9 +316,9 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg\n> \treturn caps;\n> }\n> \n> -void\n> -gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,\n> -\t\t\t\t\t GstCaps *caps)\n> +void 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> @@ -395,15 +395,46 @@ 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> +\tconst GValue *colorimetry_ = gst_structure_get_value(s, \"colorimetry\");\n\nTrailing underscores are reserved for class members.\n\n> +\tif (!colorimetry_)\n> +\t\treturn;\n> \n> -\t\tif (!gst_video_colorimetry_from_string(&colorimetry, colorimetry_str))\n> -\t\t\tg_critical(\"Invalid colorimetry %s\", colorimetry_str);\n> +\t/* Configure Colorimetry */\n> +\tGstVideoColorimetry 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> +\n> +\t\t\tif (!gst_video_colorimetry_from_string(&colorimetry, g_value_get_string(val))) {\n> +\t\t\t\tg_warning(\"Invalid colorimetry %s, trying next option\", g_value_get_string(val));\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> +\n> +\t\t\tstd::optional clrSpace = colorspace_from_colorimetry(colorimetry);\n> +\t\t\tstream_cfg.colorSpace = clrSpace;\n> +\n> +\t\t\t/* Validate the configuration and check if the requested\n\nWrong style. On this topic, lines are too long. Line wraps can help, but\nit may also indicate that some code could be split to separate functions\n(I haven't checked).\n\n> +\t\t\t * colorimetry can be applied to the sensor. Otherwise,\n> +\t\t\t * restore the configuration back to pristine one.\n> +\t\t\t */\n> +\t\t\tif (cam_cfg->validate() != CameraConfiguration::Invalid) {\n\nThe RPi pipeline handler (and other pipeline handlers) requires the same\ncolor space to be used for all streams, and it will pick (if I recall\ncorrectly) the color space of the largest non-raw stream and apply it to\nall other streams. For multi-stream use cases, validating the color\nspace on streams individually will probably not work as you're\nexpecting. You'll need a more complex logic.\n\n> +\t\t\t\t/* Check the colorspace returned after validation, with the colorspace before validation */\n> +\t\t\t\tif (stream_cfg.colorSpace == clrSpace) {\n> +\t\t\t\t\tg_print(\"Selected colorimetry %s\\n\", gst_video_colorimetry_to_string(&colorimetry));\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\t}\n> +\t\t\t}\n> +\t\t}\n> +\t} else if (G_VALUE_HOLDS_STRING(colorimetry_)) {\n> +\t\tif (!gst_video_colorimetry_from_string(&colorimetry, g_value_get_string(colorimetry_)))\n> +\t\t\tg_critical(\"Invalid colorimetry %s\", g_value_get_string(colorimetry_));\n> \n> \t\tstream_cfg.colorSpace = colorspace_from_colorimetry(colorimetry);\n\nWhy does this not use validate() ? The asymmetry between the two makes\nme think something is wrong.\n\n> +\t} else {\n> +\t\tGST_WARNING(\"colorimetry field type should only be string or list.\");\n> \t}\n> }\n> \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\nLet's pass a normal reference to the function:\n\nvoid gst_libcamera_configure_stream_from_caps(libcamera::CameraConfiguration &cam_cfg,\n\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\n\t\tgst_libcamera_configure_stream_from_caps(*state->config_, stream_cfg, caps);\n\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 EE261C0DA4\n\tfor ;\n\tMon, 19 Sep 2022 17:52:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 61B0362195;\n\tMon, 19 Sep 2022 19:52:30 +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 6214761F85\n\tfor ;\n\tMon, 19 Sep 2022 19:52:29 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9EAF8499;\n\tMon, 19 Sep 2022 19:52:28 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663609950;\n\tbh=vPdX2rLJFvC8PQqDTIWBnngIAsWwlCtfuIsan4yJqhg=;\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=LFH/2kCiyXIA1Cc679n3+5Oa/QaH/6bfBYTVrrNPeDSpljq9Q8xRvIgE38oQddLlR\n\t8ZXbSSGkXb8eVE/0qzzTzUKtSKHbttlK97hq2vmjew70lFoJMDXxPmVVaMHqsuEKcM\n\tSZLu3nIG2VXP47IO9t5x1jWL9RgcAizr7lapY0Ekp7XCnHztnJVA2xDHtxr7RDB+C8\n\tmmnostbRdnxjGfU2g5OoeP2OlYbsr01y/tvzVpg/dXJrY8hkdpW8NixnRpcNJlPQJV\n\tYjM/uGD3lObl3oZ8DMAUYhLqMj425OfsZR3ZsZf6v7zWLfRgeMLoidm5RBlxEm/fzj\n\tAd6zgmThMw1pg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1663609948;\n\tbh=vPdX2rLJFvC8PQqDTIWBnngIAsWwlCtfuIsan4yJqhg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ItElnNgFUETXrZvmwkhJLm/Y8TG085KatdGAt8xoF94lvsro1DwryDIyZcy8Gyg0+\n\thW0D6H1SIAeTzOsCvbUtGj9/wrHhe/ZVNLG3UljctWQpVQ151yfwKrVq7qWCXcBJBy\n\t7YcJwz4EL5czpvmR7OGE3vCr/jLLNbeftGWjjEak="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ItElnNgF\"; dkim-atps=neutral","Date":"Mon, 19 Sep 2022 20:52:15 +0300","To":"Rishikesh Donadkar ","Message-ID":"","References":"<20220919025709.34528-1-rishikeshdonadkar@gmail.com>\n\t<20220919025709.34528-2-rishikeshdonadkar@gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220919025709.34528-2-rishikeshdonadkar@gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v6 1/1] gstreamer: Provide multiple\n\tcolorimetry support for libcamerasrc","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, vedantparanjape160201@gmail.com,\n\tnicolas.dufresne@collabora.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25003,"web_url":"https://patchwork.libcamera.org/comment/25003/","msgid":"<73a496db-a889-bc85-9e78-2d968061ee24@ideasonboard.com>","date":"2022-09-19T18:48:36","subject":"Re: [libcamera-devel] [PATCH v6 1/1] gstreamer: Provide multiple\n\tcolorimetry support for libcamerasrc","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for the comments\nOn 9/19/22 11:22 PM, Laurent Pinchart wrote:\n> Hi Rishikesh,\n>\n> Thank you for the patch.\n>\n> On Mon, Sep 19, 2022 at 08:27:09AM +0530, Rishikesh Donadkar wrote:\n>> Iterate over each GValue in the colorimetry list,\n>> retrieve the colorimetry string, convert to colorspace,\n>> set the colorspace in the stream_cfg and validate the camera configuration.\n>> Retrieve the colorspace after validation, convert to colorimetry and\n>> check if 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>> Reviewed-by: Nicolas Dufresne \n>> ---\n>> src/gstreamer/gstlibcamera-utils.cpp | 49 +++++++++++++++++++++++-----\n>> src/gstreamer/gstlibcamera-utils.h | 4 ++-\n>> src/gstreamer/gstlibcamerasrc.cpp | 2 +-\n>> 3 files changed, 44 insertions(+), 11 deletions(-)\n>>\n>> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n>> index 244a4a79..f73b6349 100644\n>> --- a/src/gstreamer/gstlibcamera-utils.cpp\n>> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n>> @@ -316,9 +316,9 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg\n>> \treturn caps;\n>> }\n>> \n>> -void\n>> -gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,\n>> -\t\t\t\t\t GstCaps *caps)\n>> +void 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>> @@ -395,15 +395,46 @@ 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>> +\tconst GValue *colorimetry_ = gst_structure_get_value(s, \"colorimetry\");\n> Trailing underscores are reserved for class members.\n>\n>> +\tif (!colorimetry_)\n>> +\t\treturn;\n>> \n>> -\t\tif (!gst_video_colorimetry_from_string(&colorimetry, colorimetry_str))\n>> -\t\t\tg_critical(\"Invalid colorimetry %s\", colorimetry_str);\n>> +\t/* Configure Colorimetry */\n>> +\tGstVideoColorimetry 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>> +\n>> +\t\t\tif (!gst_video_colorimetry_from_string(&colorimetry, g_value_get_string(val))) {\n>> +\t\t\t\tg_warning(\"Invalid colorimetry %s, trying next option\", g_value_get_string(val));\n>> +\t\t\t\tcontinue;\n>> +\t\t\t}\n>> +\n>> +\t\t\tstd::optional clrSpace = colorspace_from_colorimetry(colorimetry);\n>> +\t\t\tstream_cfg.colorSpace = clrSpace;\n>> +\n>> +\t\t\t/* Validate the configuration and check if the requested\n> Wrong style. On this topic, lines are too long. Line wraps can help, but\n> it may also indicate that some code could be split to separate functions\n> (I haven't checked).\n>\n>> +\t\t\t * colorimetry can be applied to the sensor. Otherwise,\n>> +\t\t\t * restore the configuration back to pristine one.\n>> +\t\t\t */\n>> +\t\t\tif (cam_cfg->validate() != CameraConfiguration::Invalid) {\n> The RPi pipeline handler (and other pipeline handlers) requires the same\n> color space to be used for all streams, and it will pick (if I recall\n> correctly) the color space of the largest non-raw stream and apply it to\n> all other streams. For multi-stream use cases, validating the color\n> space on streams individually will probably not work as you're\n> expecting. You'll need a more complex logic.\n\nWe indeed had a discussion around it I think but I think it was my idea \nto develop on top of this actually.\n\nRishikesh, what do you think about handling this?  We can have a \nexploratory session together as I think I haven't been much exposed to \nmulti-stream usage via gstreamer plugin.\n>\n>> +\t\t\t\t/* Check the colorspace returned after validation, with the colorspace before validation */\n>> +\t\t\t\tif (stream_cfg.colorSpace == clrSpace) {\n>> +\t\t\t\t\tg_print(\"Selected colorimetry %s\\n\", gst_video_colorimetry_to_string(&colorimetry));\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\t}\n>> +\t\t\t}\n>> +\t\t}\n>> +\t} else if (G_VALUE_HOLDS_STRING(colorimetry_)) {\n>> +\t\tif (!gst_video_colorimetry_from_string(&colorimetry, g_value_get_string(colorimetry_)))\n>> +\t\t\tg_critical(\"Invalid colorimetry %s\", g_value_get_string(colorimetry_));\n>> \n>> \t\tstream_cfg.colorSpace = colorspace_from_colorimetry(colorimetry);\n> Why does this not use validate() ? The asymmetry between the two makes\n> me think something is wrong.\n\nFor single colorimetry, the validate happens right after the function's \ncall in  src/gstreamer/gstlibcamerasrc.cpp and then whatever is returned \nback, is exposed as caps. If the user-defined colorimetry as whatever is \nexposed in caps, negotiation shall pass.\n\nFor multiple colorimetry, we need to \"select\" a supported colorspace \ncandidate from the user-provided list, hence need to run through \nvalidate() to see what sensor agrees on any-one of them. Which is found \nto be suitable, is set in the stream configuration and re-validated \n(although futile) in \nsrc/gstreamer/gstlibcamerasrc.cpp::gst_libcamera_src_task_enter() and \nthen exposed through caps.\n\nHence, an extra validation is not required for the single-colorimetry \ncase (which currently is the behaviour on master as well).  Does that \nmakes sense?\n\n>\n>> +\t} else {\n>> +\t\tGST_WARNING(\"colorimetry field type should only be string or list.\");\n>> \t}\n>> }\n>> \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> Let's pass a normal reference to the function:\n>\n> void gst_libcamera_configure_stream_from_caps(libcamera::CameraConfiguration &cam_cfg,\n\nah good point, ack\n>\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\tgst_libcamera_configure_stream_from_caps(*state->config_, stream_cfg, caps);\n>\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 B1B44C3272\n\tfor ;\n\tMon, 19 Sep 2022 18:48:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E228362197;\n\tMon, 19 Sep 2022 20:48:49 +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 03B3F61F85\n\tfor ;\n\tMon, 19 Sep 2022 20:48:47 +0200 (CEST)","from [IPV6:2409:4052:61f:3157:5bc5:64a0:9696:6774] (unknown\n\t[IPv6:2409:4052:61f:3157:5bc5:64a0:9696:6774])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A75CD499;\n\tMon, 19 Sep 2022 20:48:44 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663613329;\n\tbh=1fXIVwOIrfIj0tkuMQmdyB0FZRzJSmZ2creBEylnsjE=;\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=zx1VhuSxqn7U+gTGWXqpdzqFgM4XjRMtFxq6lPu/Cu/Cw3YKT6wVViw+EDKtOEtq6\n\tgYXcWFATkpZlO/eQD4Swmik/kHhs2VydlBcoR/THXPEGVPUXhMQdKyedMkXagXBw49\n\t++zUQQk+RH8j+uvevd+4SmabNDIEl8RMs3WBbMaeuBQix9brFdHRxL9h2Vnjpf+LsX\n\t2Wim0xt3XvCwhrxzSIMy8h/Dh2cuTrjreZINtMn6YmUzh/JvGET1M8noCEuxUCuG7X\n\tjsfd463oRSF/Nj3aMZMCRtGY5oyl7eCCy274tdJSVJQFJ4+G2QI1Y1+IgY6jjoqOQm\n\tRzbQO88fiEA+w==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1663613327;\n\tbh=1fXIVwOIrfIj0tkuMQmdyB0FZRzJSmZ2creBEylnsjE=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=cCs7GH1NXYL9lr73kYpS+blImP5Jy02KQ11tAfwPeUHC9Rq3q3sJvS2EoJ3XJ5KT2\n\tG1vKzjmh/ScilCnO2cRzF3UULTJucS1KroiAe6u1CIYaJYjskXPAzSTJokMw6irp+e\n\tvFqKqNhB+uWzco3zHi5q7vpuB4UvDSIPD/umtn9E="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"cCs7GH1N\"; dkim-atps=neutral","Message-ID":"<73a496db-a889-bc85-9e78-2d968061ee24@ideasonboard.com>","Date":"Tue, 20 Sep 2022 00:18:36 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.2.1","To":"Laurent Pinchart ,\n\tRishikesh Donadkar ","References":"<20220919025709.34528-1-rishikeshdonadkar@gmail.com>\n\t<20220919025709.34528-2-rishikeshdonadkar@gmail.com>\n\t","Content-Language":"en-US","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v6 1/1] gstreamer: Provide multiple\n\tcolorimetry support for libcamerasrc","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":"libcamera-devel@lists.libcamera.org, vedantparanjape160201@gmail.com,\n\tnicolas.dufresne@collabora.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25122,"web_url":"https://patchwork.libcamera.org/comment/25122/","msgid":"","date":"2022-09-26T04:45:53","subject":"Re: [libcamera-devel] [PATCH v6 1/1] gstreamer: Provide multiple\n\tcolorimetry support for libcamerasrc","submitter":{"id":118,"url":"https://patchwork.libcamera.org/api/people/118/","name":"Rishikesh Donadkar","email":"rishikeshdonadkar@gmail.com"},"content":"Hello Umang and Laurent,\n\nOn Tue, Sep 20, 2022 at 12:18 AM Umang Jain wrote:\n>\n> Hi Laurent,\n>\n> Thank you for the comments\n> On 9/19/22 11:22 PM, Laurent Pinchart wrote:\n> > Hi Rishikesh,\n> >\n> > Thank you for the patch.\n> >\n> > On Mon, Sep 19, 2022 at 08:27:09AM +0530, Rishikesh Donadkar wrote:\n> >> Iterate over each GValue in the colorimetry list,\n> >> retrieve the colorimetry string, convert to colorspace,\n> >> set the colorspace in the stream_cfg and validate the camera configuration.\n> >> Retrieve the colorspace after validation, convert to colorimetry and\n> >> check if 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> >> Reviewed-by: Nicolas Dufresne \n> >> ---\n> >> src/gstreamer/gstlibcamera-utils.cpp | 49 +++++++++++++++++++++++-----\n> >> src/gstreamer/gstlibcamera-utils.h | 4 ++-\n> >> src/gstreamer/gstlibcamerasrc.cpp | 2 +-\n> >> 3 files changed, 44 insertions(+), 11 deletions(-)\n> >>\n> >> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> >> index 244a4a79..f73b6349 100644\n> >> --- a/src/gstreamer/gstlibcamera-utils.cpp\n> >> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> >> @@ -316,9 +316,9 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg\n> >> return caps;\n> >> }\n> >>\n> >> -void\n> >> -gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,\n> >> - GstCaps *caps)\n> >> +void gst_libcamera_configure_stream_from_caps(std::unique_ptr &cam_cfg,\n> >> + StreamConfiguration &stream_cfg,\n> >> + GstCaps *caps)\n> >> {\n> >> GstVideoFormat gst_format = pixel_format_to_gst_format(stream_cfg.pixelFormat);\n> >> guint i;\n> >> @@ -395,15 +395,46 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,\n> >> stream_cfg.size.width = width;\n> >> stream_cfg.size.height = height;\n> >>\n> >> - /* Configure colorimetry */\n> >> - if (gst_structure_has_field(s, \"colorimetry\")) {\n> >> - const gchar *colorimetry_str = gst_structure_get_string(s, \"colorimetry\");\n> >> - GstVideoColorimetry colorimetry;\n> >> + const GValue *colorimetry_ = gst_structure_get_value(s, \"colorimetry\");\n> > Trailing underscores are reserved for class members.\n> >\n> >> + if (!colorimetry_)\n> >> + return;\n> >>\n> >> - if (!gst_video_colorimetry_from_string(&colorimetry, colorimetry_str))\n> >> - g_critical(\"Invalid colorimetry %s\", colorimetry_str);\n> >> + /* Configure Colorimetry */\n> >> + GstVideoColorimetry colorimetry;\n> >> + if (GST_VALUE_HOLDS_LIST(colorimetry_)) {\n> >> + StreamConfiguration pristine_stream_cfg = stream_cfg;\n> >> + for (i = 0; i < gst_value_list_get_size(colorimetry_); i++) {\n> >> + const GValue *val = gst_value_list_get_value(colorimetry_, i);\n> >> +\n> >> + if (!gst_video_colorimetry_from_string(&colorimetry, g_value_get_string(val))) {\n> >> + g_warning(\"Invalid colorimetry %s, trying next option\", g_value_get_string(val));\n> >> + continue;\n> >> + }\n> >> +\n> >> + std::optional clrSpace = colorspace_from_colorimetry(colorimetry);\n> >> + stream_cfg.colorSpace = clrSpace;\n> >> +\n> >> + /* Validate the configuration and check if the requested\n> > Wrong style. On this topic, lines are too long. Line wraps can help, but\n> > it may also indicate that some code could be split to separate functions\n> > (I haven't checked).\n\nAck.\n>\n> >\n> >> + * colorimetry can be applied to the sensor. Otherwise,\n> >> + * restore the configuration back to pristine one.\n> >> + */\n> >> + if (cam_cfg->validate() != CameraConfiguration::Invalid) {\n> > The RPi pipeline handler (and other pipeline handlers) requires the same\n> > color space to be used for all streams, and it will pick (if I recall\n> > correctly) the color space of the largest non-raw stream and apply it to\n> > all other streams. For multi-stream use cases, validating the color\n> > space on streams individually will probably not work as you're\n> > expecting. You'll need a more complex logic.\n>\n> We indeed had a discussion around it I think but I think it was my idea\n> to develop on top of this actually.\n>\n> Rishikesh, what do you think about handling this? We can have a\n> exploratory session together as I think I haven't been much exposed to\n> multi-stream usage via gstreamer plugin.\n\nSure, even I have not been much exposed to multi-stream usage, maybe\nVedant might be able to help with this.\n>\n> >\n> >> + /* Check the colorspace returned after validation, with the colorspace before validation */\n> >> + if (stream_cfg.colorSpace == clrSpace) {\n> >> + g_print(\"Selected colorimetry %s\\n\", gst_video_colorimetry_to_string(&colorimetry));\n> >> + break;\n> >> + } else {\n> >> + stream_cfg = pristine_stream_cfg;\n> >> + }\n> >> + }\n> >> + }\n> >> + } else if (G_VALUE_HOLDS_STRING(colorimetry_)) {\n> >> + if (!gst_video_colorimetry_from_string(&colorimetry, g_value_get_string(colorimetry_)))\n> >> + g_critical(\"Invalid colorimetry %s\", g_value_get_string(colorimetry_));\n> >>\n> >> stream_cfg.colorSpace = colorspace_from_colorimetry(colorimetry);\n> > Why does this not use validate() ? The asymmetry between the two makes\n> > me think something is wrong.\n>\n> For single colorimetry, the validate happens right after the function's\n> call in src/gstreamer/gstlibcamerasrc.cpp and then whatever is returned\n> back, is exposed as caps. If the user-defined colorimetry as whatever is\n> exposed in caps, negotiation shall pass.\n>\n> For multiple colorimetry, we need to \"select\" a supported colorspace\n> candidate from the user-provided list, hence need to run through\n> validate() to see what sensor agrees on any-one of them. Which is found\n> to be suitable, is set in the stream configuration and re-validated\n> (although futile) in\n> src/gstreamer/gstlibcamerasrc.cpp::gst_libcamera_src_task_enter() and\n> then exposed through caps.\n>\n> Hence, an extra validation is not required for the single-colorimetry\n> case (which currently is the behaviour on master as well). Does that\n> makes sense?\n>\n> >\n> >> + } else {\n> >> + GST_WARNING(\"colorimetry field type should only be string or list.\");\n> >> }\n> >> }\n> >>\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> > Let's pass a normal reference to the function:\n> >\n> > void gst_libcamera_configure_stream_from_caps(libcamera::CameraConfiguration &cam_cfg,\n>\n> ah good point, ack\n> >\n> >> + libcamera::StreamConfiguration &stream_cfg,\n> >> 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> >> /* Fixate caps and configure the stream. */\n> >> caps = gst_caps_make_writable(caps);\n> >> - gst_libcamera_configure_stream_from_caps(stream_cfg, caps);\n> >> + gst_libcamera_configure_stream_from_caps(state->config_, stream_cfg, caps);\n> > gst_libcamera_configure_stream_from_caps(*state->config_, stream_cfg, caps);\n> >\n> >> }\n> >>\n> >> if (flow_ret != GST_FLOW_OK)\n>\n\nRegards,\n\nRishikesh Donadkar","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 320B1BD16B\n\tfor ;\n\tMon, 26 Sep 2022 04:46:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 728A762245;\n\tMon, 26 Sep 2022 06:46:08 +0200 (CEST)","from mail-vk1-xa34.google.com (mail-vk1-xa34.google.com\n\t[IPv6:2607:f8b0:4864:20::a34])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 16581603DC\n\tfor ;\n\tMon, 26 Sep 2022 06:46:06 +0200 (CEST)","by mail-vk1-xa34.google.com with SMTP id y129so2847675vkg.8\n\tfor ;\n\tSun, 25 Sep 2022 21:46:05 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1664167568;\n\tbh=Tde9UC/bI3kkhgPLTkEh3+wq704zC86LLCxAnHmRgYY=;\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=j7aLlu4qrB9mY2goL8WvXTjilparzqeEfz5BuAzoGsonV7KdIIVpwPUZV+S3gFkTY\n\t48iy1HO3VFpBNZ1fujhE3COUwqXX5QJ1M6aNqmHvGGO+lAaD7XKhtQOcXU5Au6wHff\n\tnbtmsyXIVN8n+36RxU6zXhnoyXdoM7p6q+qCMutB9Cc1QhMJkDFevIz0x9DcILdO42\n\tZuGUKNyucJB0x/BCSlS6+p5gM/ZgVblSJZB1sxNXeeKxt1ipMLn7mB/42KuxJ8pdXj\n\tuR+Y5v2zxuGiwIiVA9544qb99qQ8LPSeOrwrGWGi2Hz4SYjTogFj67D6Z93cCN9kAn\n\t9ygROuIPZJ4xg==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date;\n\tbh=Rlp2b4oKOqXMdU+yKhfSdULebX0KMwu+cme0GNJrCBU=;\n\tb=ars43opWE5GmWl74yWmXRzBcJhYTq53r2SN2eqv1tAulLuPpKOopfck30ARhNGxGhW\n\tbnC+Ip/dO1UVAEFwVMB6Fqh33uOYsE37UzFB1XNIK2H+TQBXXSTBhyKC23N9yrwuk7PE\n\t2fKj0Bt5lFNezzpQAytCnXf/eYNLZyPKywLY6kwZrTUUhyCFv8Q38B2xXn7xyvvNrjKw\n\t99+Z0/tW6Qa3wvCFhUBbYxytm84JXPg4gsxh5aYSQDOfAp9TNJ5Dp3FbxO/bpal0YmjC\n\tBCNdhQbQkJJP9kFnqB2iSa20In2634P5FC5nRpsSe/27g+BrHiftp89iR6bP5751qMH1\n\tMJMQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"ars43opW\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\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;\n\tbh=Rlp2b4oKOqXMdU+yKhfSdULebX0KMwu+cme0GNJrCBU=;\n\tb=o8LDv3EHQ2CETTue4S5Hy3Rydt4sKLD+GYxQnqZas9aGcq6DIvS71u0UPmf9+C8wMW\n\tt5yZjdHKyKqB/OSlrUbi78wOQjzg2ONh6mFx/nfK/9R55Ej7upPvgEqu3kBdGVjcN3+Y\n\taUGgyIX02eD9t1KoN7u7ZQwgI+pkuiKnpOHwQX88emOY4oQKYBsA1RhV9mqe7egq2cDT\n\tt+SICAhlN/vbgIG2tBF5Ed9LTYeV3JWuCpU27ageixS2idwQ2YBgu7NFcI9NKGQtWAie\n\tJFLuktC+Z2wVS/0GjnL6uI+VE5huevA6BuglxhyWNvHtWBrSDOGa0+Hsn4cJqHCo+IG1\n\thf/g==","X-Gm-Message-State":"ACrzQf0hHhOk9Ia+yPHKcBXV1ihkZ04hBTXlQAtfG0HYbzI/zXz7a1Oe\n\tIVCmTyuyAcndMSc5Kq+7PXQs7EVVqJbfjC7+Cyc=","X-Google-Smtp-Source":"AMsMyM5tabCbVkWuj216Re9OVV662SJqjVxWTI1JtSkEPYyyDLuA/QotSutelgUnGWBPC7MSfju7wAeG2wIHbivBpng=","X-Received":"by 2002:a05:6122:85:b0:3a7:dc18:97f8 with SMTP id\n\tr5-20020a056122008500b003a7dc1897f8mr369165vka.35.1664167564802;\n\tSun, 25 Sep 2022 21:46:04 -0700 (PDT)","MIME-Version":"1.0","References":"<20220919025709.34528-1-rishikeshdonadkar@gmail.com>\n\t<20220919025709.34528-2-rishikeshdonadkar@gmail.com>\n\t\n\t<73a496db-a889-bc85-9e78-2d968061ee24@ideasonboard.com>","In-Reply-To":"<73a496db-a889-bc85-9e78-2d968061ee24@ideasonboard.com>","Date":"Mon, 26 Sep 2022 10:15:53 +0530","Message-ID":"","To":"Umang Jain ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v6 1/1] gstreamer: Provide multiple\n\tcolorimetry support for libcamerasrc","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":"Rishikesh Donadkar via libcamera-devel\n\t","Reply-To":"Rishikesh Donadkar ","Cc":"libcamera-devel@lists.libcamera.org, vedantparanjape160201@gmail.com,\n\tnicolas.dufresne@collabora.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]