[{"id":24503,"web_url":"https://patchwork.libcamera.org/comment/24503/","msgid":"","date":"2022-08-10T05:50:08","subject":"Re: [libcamera-devel] [PATCH v2 2/2] gstreamer: Add multiple\n\tcolorimetry support","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Rishikesh,\n\nOn 8/9/22 20:05, Rishikesh Donadkar wrote:\n> This patch provides the following:\n> - Add support to handle colorimetry in the GStreamer->libcamera\n> direction.\n> - Add support for multiple colorimetry.\n>\n> The function colorspace_from_colorimetry() takes in a\n> GstVideoColorimetry and returns libcamera::ColorSpace\n>\n> Append copy of the structure with best suitable resolution into a\n> fresh new caps and normalize the caps using gst_caps_normalize().\n> This will return caps where the colorimetry list is expanded.\n> The ncaps will contain as many structures as the number of\n> colorimetry specified in the GStreamer pipeline.\n>\n> Iterate over each structure in the ncaps, retrieve the colorimetry string,\n> convert to colorspace using the function colorspace_from_colorimetry()\n> and validate the camera configuration.Retrieve the colorspace after\n> validation, convert to colorimetry and check if the it is same as\n> 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 | 152 ++++++++++++++++++++++++++-\n> src/gstreamer/gstlibcamera-utils.h | 4 +-\n> src/gstreamer/gstlibcamerasrc.cpp | 3 +-\n> 3 files changed, 153 insertions(+), 6 deletions(-)\n>\n> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> index dbb47c5a..078b9343 100644\n> --- a/src/gstreamer/gstlibcamera-utils.cpp\n> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> @@ -104,6 +104,93 @@ colorimetry_from_colorspace(const ColorSpace &colorSpace)\n> \treturn colorimetry;\n> }\n> \n> +static ColorSpace\n> +colorspace_from_colorimetry(const GstVideoColorimetry &colorimetry)\n\n\nAs mentioned in my earlier reply, this is a part of colorimetry <> \nlibcamera::Colorspace mappings which are currently handled in\n\n[v3 PATCH 4/4] gstreamer: Provide colorimetry <> ColorSpace mappings\n\nCan you please rebase over that series and provide a true multiple \ncolorimetry support patch ? Which which then be easier to review \n(because it will contain multiple colorimetry bits only, replacing the \nsingle colorimetry handling) hence I believe there will some code sharing.\n\nMeanwhile I have left a comments below which you can incorporate in next \nversion.\n\n> +{\n> +\tColorSpace colorspace = ColorSpace::Raw;\n> +\n> +\tswitch (colorimetry.primaries) {\n> +\tcase GST_VIDEO_COLOR_PRIMARIES_UNKNOWN:\n> +\t\t/* Unknown primaries map to raw colorspace in GStreamer */\n> +\t\treturn ColorSpace::Raw;\n> +\tcase GST_VIDEO_COLOR_PRIMARIES_SMPTE170M:\n> +\t\tcolorspace.primaries = ColorSpace::Primaries::Smpte170m;\n> +\t\tbreak;\n> +\tcase GST_VIDEO_COLOR_PRIMARIES_BT709:\n> +\t\tcolorspace.primaries = ColorSpace::Primaries::Rec709;\n> +\t\tbreak;\n> +\tcase GST_VIDEO_COLOR_PRIMARIES_BT2020:\n> +\t\tcolorspace.primaries = ColorSpace::Primaries::Rec2020;\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tGST_WARNING(\"Colorimetry primaries %d not mapped in gstlibcamera\",\n> +\t\t\t colorimetry.primaries);\n> +\t\treturn ColorSpace::Raw;\n> +\t}\n> +\n> +\tswitch (colorimetry.transfer) {\n> +\t/* Transfer function mappings inspired from v4l2src plugin */\n> +\tcase GST_VIDEO_TRANSFER_GAMMA18:\n> +\tcase GST_VIDEO_TRANSFER_GAMMA20:\n> +\tcase GST_VIDEO_TRANSFER_GAMMA22:\n> +\tcase GST_VIDEO_TRANSFER_GAMMA28:\n> +\t\tGST_WARNING(\"GAMMA 18, 20, 22, 28 transfer functions not supported\");\n> +\t/* fallthrough */\n> +\tcase GST_VIDEO_TRANSFER_GAMMA10:\n> +\t\tcolorspace.transferFunction = ColorSpace::TransferFunction::Linear;\n> +\t\tbreak;\n> +\tcase GST_VIDEO_TRANSFER_SRGB:\n> +\t\tcolorspace.transferFunction = ColorSpace::TransferFunction::Srgb;\n> +\t\tbreak;\n> +\tcase GST_VIDEO_TRANSFER_BT601:\n> +\tcase GST_VIDEO_TRANSFER_BT2020_12:\n> +\tcase GST_VIDEO_TRANSFER_BT2020_10:\n> +\tcase GST_VIDEO_TRANSFER_BT709:\n> +\t\tcolorspace.transferFunction = ColorSpace::TransferFunction::Rec709;\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tGST_WARNING(\"Colorimetry transfer function %d not mapped in gstlibcamera\",\n> +\t\t\t colorimetry.transfer);\n> +\t\treturn ColorSpace::Raw;\n> +\t}\n> +\n> +\tswitch (colorimetry.matrix) {\n> +\tcase GST_VIDEO_COLOR_MATRIX_RGB:\n> +\t\tcolorspace.ycbcrEncoding = ColorSpace::YcbcrEncoding::None;\n> +\t\tbreak;\n> +\t/* FCC is about the same as BT601 with less digits */\n> +\tcase GST_VIDEO_COLOR_MATRIX_FCC:\n> +\tcase GST_VIDEO_COLOR_MATRIX_BT601:\n> +\t\tcolorspace.ycbcrEncoding = ColorSpace::YcbcrEncoding::Rec601;\n> +\t\tbreak;\n> +\tcase GST_VIDEO_COLOR_MATRIX_BT709:\n> +\t\tcolorspace.ycbcrEncoding = ColorSpace::YcbcrEncoding::Rec709;\n> +\t\tbreak;\n> +\tcase GST_VIDEO_COLOR_MATRIX_BT2020:\n> +\t\tcolorspace.ycbcrEncoding = ColorSpace::YcbcrEncoding::Rec2020;\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tGST_WARNING(\"Colorimetry matrix %d not mapped in gstlibcamera\",\n> +\t\t\t colorimetry.matrix);\n> +\t\treturn ColorSpace::Raw;\n> +\t}\n> +\n> +\tswitch (colorimetry.range) {\n> +\tcase GST_VIDEO_COLOR_RANGE_0_255:\n> +\t\tcolorspace.range = ColorSpace::Range::Full;\n> +\t\tbreak;\n> +\tcase GST_VIDEO_COLOR_RANGE_16_235:\n> +\t\tcolorspace.range = ColorSpace::Range::Limited;\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tGST_WARNING(\"Colorimetry range %d not mapped in gstlibcamera\",\n> +\t\t\t colorimetry.range);\n> +\t\treturn ColorSpace::Raw;\n> +\t}\n> +\n> +\treturn colorspace;\n> +}\n> +\n> static GstVideoFormat\n> pixel_format_to_gst_format(const PixelFormat &format)\n> {\n> @@ -215,13 +302,47 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg\n> \treturn caps;\n> }\n> \n> +static void\n> +configure_colorspace_from_caps(StreamConfiguration &stream_cfg,\n> +\t\t\t GstStructure *s)\n> +{\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> +\t\t}\n\n\nThe rebase will reflect that this code block as been 'moved', which is \nnot quite apparent right now.\n\n> +\t}\n> +}\n> +\n> +static gboolean\n> +check_colorspace(const ColorSpace colorSpace, const gchar *colorimetry_old)\n\n\nMore suitable : 'compare_colorspace(old, new)'\n\n> +{\n> +\tGstVideoColorimetry colorimetry = colorimetry_from_colorspace(colorSpace);\n> +\tg_autofree gchar *colorimetry_new = gst_video_colorimetry_to_string(&colorimetry);\n> +\tif (!g_strcmp0(colorimetry_old, colorimetry_new)) {\n\n\nHave you tried using the comparator gst_video_colorimetry_is_equal() ? I \nguess that shall be more appropriate than comparing strings?\n\n> +\t\treturn true;\n> +\t}\n\n\nNo need for {} as only single line after \"if\"\n\n> +\treturn false;\n> +}\n> +\n> void\n> -gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,\n> +gst_libcamera_configure_stream_from_caps(CameraConfiguration &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, colorimetry_index = -1;\n> \tGstStructure *s;\n> \n> \t/*\n> @@ -267,10 +388,13 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,\n> \t}\n> \n> \t/* Prefer reliable fixed value over ranges */\n> -\tif (best_fixed >= 0)\n> +\tif (best_fixed >= 0) {\n> \t\ts = gst_caps_get_structure(caps, best_fixed);\n> -\telse\n> +\t\tcolorimetry_index = best_fixed;\n> +\t} else {\n> \t\ts = gst_caps_get_structure(caps, best_in_range);\n> +\t\tcolorimetry_index = best_in_range;\n> +\t}\n> \n> \tif (gst_structure_has_name(s, \"video/x-raw\")) {\n> \t\tconst gchar *format = gst_video_format_to_string(gst_format);\n> @@ -293,6 +417,26 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,\n> \tgst_structure_get_int(s, \"height\", &height);\n> \tstream_cfg.size.width = width;\n> \tstream_cfg.size.height = height;\n> +\n> +\t/* Create new caps, copy the structure with best resolutions\n> +\t * and normalize the caps.\n> +\t */\n> +\tGstCaps *ncaps = gst_caps_copy_nth(caps, colorimetry_index);\n> +\tncaps = gst_caps_normalize(ncaps);\n> +\n> +\t/* Configure Colorimetry */\n> +\tStreamConfiguration dup_stream_cfg = stream_cfg;\n\n\nThis is used when no colorimetry candidate is found, so probably rename \nas pristine_stream_cfg?\n\n> +\tfor (i = 0; i < gst_caps_get_size(ncaps); i++) {\n> +\t\tGstStructure *ns = gst_caps_get_structure(ncaps, i);\n> +\t\tconfigure_colorspace_from_caps(stream_cfg, ns);\n> +\t\tg_autofree const gchar *colorimetry_old = gst_structure_get_string(ns, \"colorimetry\");\n\n\ns/colorimetry_old/colorimetry\n\n> +\t\tif (cam_cfg.validate() != CameraConfiguration::Invalid) {\n> +\t\t\tif (check_colorspace(stream_cfg.colorSpace.value(), colorimetry_old))\n> +\t\t\t\tbreak;\n\n\nShould we log a INFO log here: \"Selected colorimetry: \" ?\n\n> +\t\t\telse\n> +\t\t\t\tstream_cfg = dup_stream_cfg;\n> +\t\t}\n> +\t}\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..90be6abe 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(libcamera::CameraConfiguration &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..3617170e 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -492,6 +492,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> \tfor (gsize i = 0; i < state->srcpads_.size(); i++) {\n> \t\tGstPad *srcpad = state->srcpads_[i];\n> \t\tStreamConfiguration &stream_cfg = state->config_->at(i);\n> +\t\tCameraConfiguration &cam_cfg = *(state->config_);\n> \n> \t\t/* Retrieve the supported caps. */\n> \t\tg_autoptr(GstCaps) filter = gst_libcamera_stream_formats_to_caps(stream_cfg.formats());\n> @@ -503,7 +504,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(cam_cfg, stream_cfg, caps);\n\n\nYou can directly pass state->config_ here instead of taking it in a \nholder variable reference cam_cfg\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 66881C3272\n\tfor ;\n\tWed, 10 Aug 2022 05:50:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AA5426332B;\n\tWed, 10 Aug 2022 07:50:17 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C6F4561FA9\n\tfor ;\n\tWed, 10 Aug 2022 07:50:15 +0200 (CEST)","from [IPV6:2401:4900:1f3f:c7a1:27b3:9637:38a7:6084] (unknown\n\t[IPv6:2401:4900:1f3f:c7a1:27b3:9637:38a7:6084])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F2580481;\n\tWed, 10 Aug 2022 07:50:13 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660110617;\n\tbh=CY/C06+nfJR30rBlZSYmRVPv500v+kpN/y3CWEM5NGk=;\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=DwZXl4D7OjEJXP2xzhkh8ZxEVvE+ATRjMkC0aU0kO4t13HaJGS7MfB6fG0hT7+oIK\n\tt63nG5XOnIDJTsc4o3s+5V5BevTst3RDwaw690EKXXOywKbaWbDpxSBmdfWvEYH2+m\n\tkAsI7SaZOFZUeheLjFcyN0ZYfKL0m7/dvc75kPWM3ohp1JthZqdre1Dmsa/Cjw6MI/\n\tdufIbyhoCfU1xcGhhP3a42VYmS+CM2ShMObYFnMninUR0E3ZIP2SEF10HSmanEmVkw\n\tg+E9TTpzr+x8c4ae/6l9Dy7TmjG21Ef2kXYsPvAyLvckMa+l84g3R3sMqHTgrib7ss\n\tCTzaR7tv2Mb9g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1660110615;\n\tbh=CY/C06+nfJR30rBlZSYmRVPv500v+kpN/y3CWEM5NGk=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=gLjRznpnbc9ZsmEJQF40436ywkXuJImWr1qYsTTVzMAMMQhR+Ud6BavooDJsZl32y\n\tctJN810OWt/rwX0QL6644T1R4e0etfj0vazCaY2vWgBRafbuWo4jyepGNC+ivHAtX2\n\tnS8QUnqHdeoWWeaF7vfjHQnEUUaYeTat9APfovuA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"gLjRznpn\"; dkim-atps=neutral","Message-ID":"","Date":"Wed, 10 Aug 2022 11:20:08 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"Rishikesh Donadkar ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20220809143531.7473-1-rishikeshdonadkar@gmail.com>\n\t<20220809143531.7473-3-rishikeshdonadkar@gmail.com>","In-Reply-To":"<20220809143531.7473-3-rishikeshdonadkar@gmail.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] gstreamer: Add multiple\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":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"nicolas.dufresne@collabora.com, vedantparanjape160201@gmail.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24548,"web_url":"https://patchwork.libcamera.org/comment/24548/","msgid":"","date":"2022-08-11T10:22:56","subject":"Re: [libcamera-devel] [PATCH v2 2/2] gstreamer: Add multiple\n\tcolorimetry support","submitter":{"id":118,"url":"https://patchwork.libcamera.org/api/people/118/","name":"Rishikesh Donadkar","email":"rishikeshdonadkar@gmail.com"},"content":">\n> Can you please rebase over that series and provide a true multiple\n> colorimetry support patch ? Which which then be easier to review\n> (because it will contain multiple colorimetry bits only, replacing the\n> single colorimetry handling) hence I believe there will some code sharing.\n>\nSure.\n\n> More suitable : 'compare_colorspace(old, new)'\n>\n> > +{\n> > + GstVideoColorimetry colorimetry =\n> colorimetry_from_colorspace(colorSpace);\n> > + g_autofree gchar *colorimetry_new =\n> gst_video_colorimetry_to_string(&colorimetry);\n> > + if (!g_strcmp0(colorimetry_old, colorimetry_new)) {\n\nNoted.\n\n> Have you tried using the comparator gst_video_colorimetry_is_equal() ? I\n> guess that shall be more appropriate than comparing strings?\n>\n> > + return true;\n> > + }\n>\nThanks for the pointer. Using this comparator eliminated the need for the\ncompare_colorspace() function.\n\n> > + /* Create new caps, copy the structure with best resolutions\n> > + * and normalize the caps.\n> > + */\n> > + GstCaps *ncaps = gst_caps_copy_nth(caps, colorimetry_index);\n> > + ncaps = gst_caps_normalize(ncaps);\n> > +\n> > + /* Configure Colorimetry */\n> > + StreamConfiguration dup_stream_cfg = stream_cfg;\n>\n>\n> This is used when no colorimetry candidate is found, so probably rename\n> as pristine_stream_cfg?\n\nRight.\n\n> > + if (cam_cfg.validate() != CameraConfiguration::Invalid) {\n> > + if\n> (check_colorspace(stream_cfg.colorSpace.value(), colorimetry_old))\n> > + break;\n>\n>\n> Should we log a INFO log here: \"Selected colorimetry: \" ?\n>\nYes, that would be quite useful.\n\n> > for (gsize i = 0; i < state->srcpads_.size(); i++) {\n> > GstPad *srcpad = state->srcpads_[i];\n> > StreamConfiguration &stream_cfg = state->config_->at(i);\n> > + CameraConfiguration &cam_cfg = *(state->config_);\n> >\n> > /* Retrieve the supported caps. */\n> > g_autoptr(GstCaps) filter =\n> gst_libcamera_stream_formats_to_caps(stream_cfg.formats());\n> > @@ -503,7 +504,7 @@ gst_libcamera_src_task_enter(GstTask *task,\n> [[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(cam_cfg,\n> stream_cfg, caps);\n>\n>\n> You can directly pass state->config_ here instead of taking it in a\n> holder variable reference cam_cfg\n\nNoted.\n\n\nOn Wed, Aug 10, 2022 at 11:20 AM Umang Jain \nwrote:\n\n> Hi Rishikesh,\n>\n> On 8/9/22 20:05, Rishikesh Donadkar wrote:\n> > This patch provides the following:\n> > - Add support to handle colorimetry in the GStreamer->libcamera\n> > direction.\n> > - Add support for multiple colorimetry.\n> >\n> > The function colorspace_from_colorimetry() takes in a\n> > GstVideoColorimetry and returns libcamera::ColorSpace\n> >\n> > Append copy of the structure with best suitable resolution into a\n> > fresh new caps and normalize the caps using gst_caps_normalize().\n> > This will return caps where the colorimetry list is expanded.\n> > The ncaps will contain as many structures as the number of\n> > colorimetry specified in the GStreamer pipeline.\n> >\n> > Iterate over each structure in the ncaps, retrieve the colorimetry\n> string,\n> > convert to colorspace using the function colorspace_from_colorimetry()\n> > and validate the camera configuration.Retrieve the colorspace after\n> > validation, convert to colorimetry and check if the it is same as\n> > 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 | 152 ++++++++++++++++++++++++++-\n> > src/gstreamer/gstlibcamera-utils.h | 4 +-\n> > src/gstreamer/gstlibcamerasrc.cpp | 3 +-\n> > 3 files changed, 153 insertions(+), 6 deletions(-)\n> >\n> > diff --git a/src/gstreamer/gstlibcamera-utils.cpp\n> b/src/gstreamer/gstlibcamera-utils.cpp\n> > index dbb47c5a..078b9343 100644\n> > --- a/src/gstreamer/gstlibcamera-utils.cpp\n> > +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> > @@ -104,6 +104,93 @@ colorimetry_from_colorspace(const ColorSpace\n> &colorSpace)\n> > return colorimetry;\n> > }\n> >\n> > +static ColorSpace\n> > +colorspace_from_colorimetry(const GstVideoColorimetry &colorimetry)\n>\n>\n> As mentioned in my earlier reply, this is a part of colorimetry <>\n> libcamera::Colorspace mappings which are currently handled in\n>\n> [v3 PATCH 4/4] gstreamer: Provide colorimetry <> ColorSpace mappings\n>\n> Can you please rebase over that series and provide a true multiple\n> colorimetry support patch ? Which which then be easier to review\n> (because it will contain multiple colorimetry bits only, replacing the\n> single colorimetry handling) hence I believe there will some code sharing.\n>\n> Meanwhile I have left a comments below which you can incorporate in next\n> version.\n>\n> > +{\n> > + ColorSpace colorspace = ColorSpace::Raw;\n> > +\n> > + switch (colorimetry.primaries) {\n> > + case GST_VIDEO_COLOR_PRIMARIES_UNKNOWN:\n> > + /* Unknown primaries map to raw colorspace in GStreamer */\n> > + return ColorSpace::Raw;\n> > + case GST_VIDEO_COLOR_PRIMARIES_SMPTE170M:\n> > + colorspace.primaries = ColorSpace::Primaries::Smpte170m;\n> > + break;\n> > + case GST_VIDEO_COLOR_PRIMARIES_BT709:\n> > + colorspace.primaries = ColorSpace::Primaries::Rec709;\n> > + break;\n> > + case GST_VIDEO_COLOR_PRIMARIES_BT2020:\n> > + colorspace.primaries = ColorSpace::Primaries::Rec2020;\n> > + break;\n> > + default:\n> > + GST_WARNING(\"Colorimetry primaries %d not mapped in\n> gstlibcamera\",\n> > + colorimetry.primaries);\n> > + return ColorSpace::Raw;\n> > + }\n> > +\n> > + switch (colorimetry.transfer) {\n> > + /* Transfer function mappings inspired from v4l2src plugin */\n> > + case GST_VIDEO_TRANSFER_GAMMA18:\n> > + case GST_VIDEO_TRANSFER_GAMMA20:\n> > + case GST_VIDEO_TRANSFER_GAMMA22:\n> > + case GST_VIDEO_TRANSFER_GAMMA28:\n> > + GST_WARNING(\"GAMMA 18, 20, 22, 28 transfer functions not\n> supported\");\n> > + /* fallthrough */\n> > + case GST_VIDEO_TRANSFER_GAMMA10:\n> > + colorspace.transferFunction =\n> ColorSpace::TransferFunction::Linear;\n> > + break;\n> > + case GST_VIDEO_TRANSFER_SRGB:\n> > + colorspace.transferFunction =\n> ColorSpace::TransferFunction::Srgb;\n> > + break;\n> > + case GST_VIDEO_TRANSFER_BT601:\n> > + case GST_VIDEO_TRANSFER_BT2020_12:\n> > + case GST_VIDEO_TRANSFER_BT2020_10:\n> > + case GST_VIDEO_TRANSFER_BT709:\n> > + colorspace.transferFunction =\n> ColorSpace::TransferFunction::Rec709;\n> > + break;\n> > + default:\n> > + GST_WARNING(\"Colorimetry transfer function %d not mapped\n> in gstlibcamera\",\n> > + colorimetry.transfer);\n> > + return ColorSpace::Raw;\n> > + }\n> > +\n> > + switch (colorimetry.matrix) {\n> > + case GST_VIDEO_COLOR_MATRIX_RGB:\n> > + colorspace.ycbcrEncoding = ColorSpace::YcbcrEncoding::None;\n> > + break;\n> > + /* FCC is about the same as BT601 with less digits */\n> > + case GST_VIDEO_COLOR_MATRIX_FCC:\n> > + case GST_VIDEO_COLOR_MATRIX_BT601:\n> > + colorspace.ycbcrEncoding =\n> ColorSpace::YcbcrEncoding::Rec601;\n> > + break;\n> > + case GST_VIDEO_COLOR_MATRIX_BT709:\n> > + colorspace.ycbcrEncoding =\n> ColorSpace::YcbcrEncoding::Rec709;\n> > + break;\n> > + case GST_VIDEO_COLOR_MATRIX_BT2020:\n> > + colorspace.ycbcrEncoding =\n> ColorSpace::YcbcrEncoding::Rec2020;\n> > + break;\n> > + default:\n> > + GST_WARNING(\"Colorimetry matrix %d not mapped in\n> gstlibcamera\",\n> > + colorimetry.matrix);\n> > + return ColorSpace::Raw;\n> > + }\n> > +\n> > + switch (colorimetry.range) {\n> > + case GST_VIDEO_COLOR_RANGE_0_255:\n> > + colorspace.range = ColorSpace::Range::Full;\n> > + break;\n> > + case GST_VIDEO_COLOR_RANGE_16_235:\n> > + colorspace.range = ColorSpace::Range::Limited;\n> > + break;\n> > + default:\n> > + GST_WARNING(\"Colorimetry range %d not mapped in\n> gstlibcamera\",\n> > + colorimetry.range);\n> > + return ColorSpace::Raw;\n> > + }\n> > +\n> > + return colorspace;\n> > +}\n> > +\n> > static GstVideoFormat\n> > pixel_format_to_gst_format(const PixelFormat &format)\n> > {\n> > @@ -215,13 +302,47 @@ gst_libcamera_stream_configuration_to_caps(const\n> StreamConfiguration &stream_cfg\n> > return caps;\n> > }\n> >\n> > +static void\n> > +configure_colorspace_from_caps(StreamConfiguration &stream_cfg,\n> > + GstStructure *s)\n> > +{\n> > + if (gst_structure_has_field(s, \"colorimetry\")) {\n> > + const gchar *colorimetry_str = gst_structure_get_string(s,\n> \"colorimetry\");\n> > + GstVideoColorimetry colorimetry;\n> > +\n> > + if (!gst_video_colorimetry_from_string(&colorimetry,\n> colorimetry_str))\n> > + g_critical(\"Invalid colorimetry %s\",\n> colorimetry_str);\n> > +\n> > + stream_cfg.colorSpace =\n> colorspace_from_colorimetry(colorimetry);\n> > + /* Check if colorimetry had any identifiers which did not\n> map */\n> > + if (colorimetry.primaries !=\n> GST_VIDEO_COLOR_PRIMARIES_UNKNOWN &&\n> > + stream_cfg.colorSpace == ColorSpace::Raw) {\n> > + GST_ERROR(\"One or more identifiers could not be\n> mapped for %s colorimetry\",\n> > + colorimetry_str);\n> > + stream_cfg.colorSpace = std::nullopt;\n> > + }\n>\n>\n> The rebase will reflect that this code block as been 'moved', which is\n> not quite apparent right now.\n>\n> > + }\n> > +}\n> > +\n> > +static gboolean\n> > +check_colorspace(const ColorSpace colorSpace, const gchar\n> *colorimetry_old)\n>\n>\n> More suitable : 'compare_colorspace(old, new)'\n>\n> > +{\n> > + GstVideoColorimetry colorimetry =\n> colorimetry_from_colorspace(colorSpace);\n> > + g_autofree gchar *colorimetry_new =\n> gst_video_colorimetry_to_string(&colorimetry);\n> > + if (!g_strcmp0(colorimetry_old, colorimetry_new)) {\n>\n>\n> Have you tried using the comparator gst_video_colorimetry_is_equal() ? I\n> guess that shall be more appropriate than comparing strings?\n>\n> > + return true;\n> > + }\n>\n>\n> No need for {} as only single line after \"if\"\n>\n> > + return false;\n> > +}\n> > +\n> > void\n> > -gst_libcamera_configure_stream_from_caps(StreamConfiguration\n> &stream_cfg,\n> > +gst_libcamera_configure_stream_from_caps(CameraConfiguration &cam_cfg,\n> > + StreamConfiguration &stream_cfg,\n> > GstCaps *caps)\n> > {\n> > GstVideoFormat gst_format =\n> pixel_format_to_gst_format(stream_cfg.pixelFormat);\n> > guint i;\n> > - gint best_fixed = -1, best_in_range = -1;\n> > + gint best_fixed = -1, best_in_range = -1, colorimetry_index = -1;\n> > GstStructure *s;\n> >\n> > /*\n> > @@ -267,10 +388,13 @@\n> gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,\n> > }\n> >\n> > /* Prefer reliable fixed value over ranges */\n> > - if (best_fixed >= 0)\n> > + if (best_fixed >= 0) {\n> > s = gst_caps_get_structure(caps, best_fixed);\n> > - else\n> > + colorimetry_index = best_fixed;\n> > + } else {\n> > s = gst_caps_get_structure(caps, best_in_range);\n> > + colorimetry_index = best_in_range;\n> > + }\n> >\n> > if (gst_structure_has_name(s, \"video/x-raw\")) {\n> > const gchar *format =\n> gst_video_format_to_string(gst_format);\n> > @@ -293,6 +417,26 @@\n> gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,\n> > gst_structure_get_int(s, \"height\", &height);\n> > stream_cfg.size.width = width;\n> > stream_cfg.size.height = height;\n> > +\n> > + /* Create new caps, copy the structure with best resolutions\n> > + * and normalize the caps.\n> > + */\n> > + GstCaps *ncaps = gst_caps_copy_nth(caps, colorimetry_index);\n> > + ncaps = gst_caps_normalize(ncaps);\n> > +\n> > + /* Configure Colorimetry */\n> > + StreamConfiguration dup_stream_cfg = stream_cfg;\n>\n>\n> This is used when no colorimetry candidate is found, so probably rename\n> as pristine_stream_cfg?\n>\n> > + for (i = 0; i < gst_caps_get_size(ncaps); i++) {\n> > + GstStructure *ns = gst_caps_get_structure(ncaps, i);\n> > + configure_colorspace_from_caps(stream_cfg, ns);\n> > + g_autofree const gchar *colorimetry_old =\n> gst_structure_get_string(ns, \"colorimetry\");\n>\n>\n> s/colorimetry_old/colorimetry\n>\n> > + if (cam_cfg.validate() != CameraConfiguration::Invalid) {\n> > + if\n> (check_colorspace(stream_cfg.colorSpace.value(), colorimetry_old))\n> > + break;\n>\n>\n> Should we log a INFO log here: \"Selected colorimetry: \" ?\n>\n> > + else\n> > + stream_cfg = dup_stream_cfg;\n> > + }\n> > + }\n> > }\n> >\n> > #if !GST_CHECK_VERSION(1, 17, 1)\n> > diff --git a/src/gstreamer/gstlibcamera-utils.h\n> b/src/gstreamer/gstlibcamera-utils.h\n> > index 164189a2..90be6abe 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\n> libcamera::StreamFormats &formats);\n> > GstCaps *gst_libcamera_stream_configuration_to_caps(const\n> libcamera::StreamConfiguration &stream_cfg);\n> > -void\n> gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration\n> &stream_cfg,\n> > +void\n> gst_libcamera_configure_stream_from_caps(libcamera::CameraConfiguration\n> &cam_cfg,\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\n> b/src/gstreamer/gstlibcamerasrc.cpp\n> > index 16d70fea..3617170e 100644\n> > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > @@ -492,6 +492,7 @@ gst_libcamera_src_task_enter(GstTask *task,\n> [[maybe_unused]] GThread *thread,\n> > for (gsize i = 0; i < state->srcpads_.size(); i++) {\n> > GstPad *srcpad = state->srcpads_[i];\n> > StreamConfiguration &stream_cfg = state->config_->at(i);\n> > + CameraConfiguration &cam_cfg = *(state->config_);\n> >\n> > /* Retrieve the supported caps. */\n> > g_autoptr(GstCaps) filter =\n> gst_libcamera_stream_formats_to_caps(stream_cfg.formats());\n> > @@ -503,7 +504,7 @@ gst_libcamera_src_task_enter(GstTask *task,\n> [[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(cam_cfg,\n> stream_cfg, caps);\n>\n>\n> You can directly pass state->config_ here instead of taking it in a\n> holder variable reference cam_cfg\n>\n> > }\n> >\n> > if (flow_ret != GST_FLOW_OK)\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 030C0BE173\n\tfor ;\n\tThu, 11 Aug 2022 10:23:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 478696332C;\n\tThu, 11 Aug 2022 12:23:11 +0200 (CEST)","from mail-vs1-xe2f.google.com (mail-vs1-xe2f.google.com\n\t[IPv6:2607:f8b0:4864:20::e2f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2140E6330E\n\tfor ;\n\tThu, 11 Aug 2022 12:23:09 +0200 (CEST)","by mail-vs1-xe2f.google.com with SMTP id d126so13511125vsd.13\n\tfor ;\n\tThu, 11 Aug 2022 03:23:09 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660213391;\n\tbh=/U+cWZGNuXMgMC3J1r4axL1aTX17htq1zYO0hm4/ZYs=;\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=DZZ+jzyDWKKINd3O49rxcpVvFi7Hc+jfnDH/e04pw8OnHNYNd1RTfyf2/Jt8NviZ4\n\t0DuI9KCf8rZ1WaqKvzFlA8+EC/8S9GJ4c5wEYtHDikZFk9ViLtXeaGA3Aj1AhV+ooH\n\tYGBLbImrnn/nVAODUKdMKE39aUXWtFtKkNWp+xDCiNoWmlA7/TY4f4T/bBCb718g1o\n\tBNjtmdCjZRMCTlFHXid8fL9x8gLddPqQT4ia57jbyqw8Z3VoDMe87L34LqpEr5Ff04\n\t8Sqj7KmDUX21KF5JIyh+2cxuntty1q2zOCITAHloSO/FGwZE7eFNtb5ICRP/k6z2lu\n\to8ERTgsQuYVnQ==","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;\n\tbh=4owYgH3ig+addaVw/2YFBZJIHKtdj+ALNyQPJtXZc0s=;\n\tb=SJtbPP3sqfdC8i7/6VOLnrpBh0NTQu46crcez6WUmVEX5yi/I62a1bqMtTbBLWvhkw\n\tSmIX0I6q4jrvS3ghYeli4YT7nOw5wiBr41/ULlb/1it1pdugLHxJmxwH14eONqdD8mKt\n\t/EHbQhTiMLkov/fwyBZrToBKeYJyxn7JVQ8XaunSLt3P64f8ce4sXQrL548Y6E529l1+\n\tNQm1fgsUBgr+99EbRNPfv731Pe2QVp9lZ1EQkWyxLfVn7uscufiaqLwfpPkRDfda2+yH\n\tqhapkXjkl5L/MyvcXTBwIfgxoSpL6R7QFZXWJciHVudCrZebb09doWBP7gTCM/tdyd8b\n\tGqnw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"SJtbPP3s\"; 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;\n\tbh=4owYgH3ig+addaVw/2YFBZJIHKtdj+ALNyQPJtXZc0s=;\n\tb=Kfppr5CplbNmsm3pyT0nU5LlEhqcW4K6gM3atARHQW0mfpDZUFZLH4yjLK0kG8pI6N\n\t8DUoxI0fLPW8PmOI6jOnN9+PReVxIMKBWYnhNhwND5cBmBb2xv3WeIDBC8tzOXkJUGaP\n\t3Vlc0DSnt6gHLamcoawj4+RtCR+szRvUuEMduSRrw9/sfaDjdAYnxO9mDPkSGqLQWYVD\n\t8RiWcFBj0viNxKOTakzWzcb6aWM9myqMCNnzPmSKwWa7x3r1TfsGaJ0F1b75M9wIfP4n\n\t5vbTCR6JiNYwU6pXMg5NB9qqYhHWcDgnIW2IV0urCc3ysAJ3Nde/TtD0lT5fp3Xkc0mk\n\tI+Fg==","X-Gm-Message-State":"ACgBeo1/huu1/nZL1mv/JV1CBmF3vMo3G2Kqu1qsbzg/jr5CqzW9r2YJ\n\tkvLWUGxHcRPXo5aCP9bEfyxD0gcFzfUvlyrEzPA=","X-Google-Smtp-Source":"AA6agR6/Z9tP/tTJoOXw5xkWLcuhhV9JQE3VrEsdFfs+JI9zB9W0EemMhpR8dIKbUoQ2xE5xWqDIifBuvtXn43ye16k=","X-Received":"by 2002:a67:cb14:0:b0:35a:23fd:9287 with SMTP id\n\tb20-20020a67cb14000000b0035a23fd9287mr14470786vsl.11.1660213387493;\n\tThu, 11 Aug 2022 03:23:07 -0700 (PDT)","MIME-Version":"1.0","References":"<20220809143531.7473-1-rishikeshdonadkar@gmail.com>\n\t<20220809143531.7473-3-rishikeshdonadkar@gmail.com>\n\t","In-Reply-To":"","Date":"Thu, 11 Aug 2022 15:52:56 +0530","Message-ID":"","To":"Umang Jain ","Content-Type":"multipart/alternative; boundary=\"0000000000003685b305e5f48dac\"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] gstreamer: Add multiple\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":"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\" "}}]