[{"id":24254,"web_url":"https://patchwork.libcamera.org/comment/24254/","msgid":"","date":"2022-07-31T19:44:43","subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Provide colorimetry <>\n\tColorSpace mappings","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang and Rishikesh,\n\n(Cc'ing David)\n\nThank you for the patch.\n\nOn Fri, Jul 29, 2022 at 04:08:46PM +0530, Umang Jain via libcamera-devel wrote:\n> From: Rishikesh Donadkar \n> \n> Provide colorimetry <=> libcamera::ColorSpace mappings via:\n> - GstVideoColorimetry colorimetry_from_colorspace(colorspace);\n> - ColorSpace colorspace_from_colorimetry(colorimetry);\n> \n> Read the colorimetry field from caps into the stream configuration.\n> After stream validation, the sensor supported colorimetry will\n> be retrieved and the caps will be updated accordingly.\n> \n> Colorimetry support for gstlibcamerasrc currently undertakes only one\n\ns/gstlibcamerasrc/libcamerasrc/ (or gstlibcamera if you meant the\nlibrary, not the element name)\n\n> argument. Multiple colorimetry support shall be introduced in\n> subsequent commits.\n\nI'm probably missing something here, what do you mean by multiple\ncolorimetry support ?\n\n> Signed-off-by: Rishikesh Donadkar \n> Signed-off-by: Umang Jain \n> ---\n> \n> Changes in v2:\n> - Drop \"Default\" Colorspace\n> - Improve function signature of colorimetry_from_colorspace() and\n> colorspace_from_colorimetry()\n> - Map GST_VIDEO_COLOR_MATRIX_RGB to Colorspace::YcbcrEncoding::Rec601\n> (comes from the kernel)\n> - Map GST_VIDEO_COLOR_PRIMARIES_UNKNOWN to Raw colorspace\n> - Map ColorSpace::YcbcrEncoding::Rec601 to GST_VIDEO_COLOR_MATRIX_RGB\n> if colorspace is \"sRGB\". GST_VIDEO_COLOR_MATRIX_BT601 for all other\n> cases\n> - Minor nits regarding error reporting strings.\n> ---\n> src/gstreamer/gstlibcamera-utils.cpp | 164 +++++++++++++++++++++++++++\n> 1 file changed, 164 insertions(+)\n> \n> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> index c97c0d43..e4ff1269 100644\n> --- a/src/gstreamer/gstlibcamera-utils.cpp\n> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> @@ -45,6 +45,146 @@ static struct {\n> \t/* \\todo NV42 is used in libcamera but is not mapped in GStreamer yet. */\n> };\n> \n> +static GstVideoColorimetry\n> +colorimetry_from_colorspace(const ColorSpace &colorSpace)\n> +{\n> +\tGstVideoColorimetry colorimetry;\n> +\n> +\tswitch (colorSpace.primaries) {\n> +\tcase ColorSpace::Primaries::Rec709:\n> +\t\tcolorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_BT709;\n> +\t\tbreak;\n> +\tcase ColorSpace::Primaries::Rec2020:\n> +\t\tcolorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_BT2020;\n> +\t\tbreak;\n> +\tcase ColorSpace::Primaries::Smpte170m:\n> +\t\tcolorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_SMPTE170M;\n> +\t\tbreak;\n> +\tcase ColorSpace::Primaries::Raw:\n> +\t\tcolorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_UNKNOWN;\n> +\t\tbreak;\n> +\t}\n\nI'd sort the cases with the order of the enum in color_space.h (below\ntoo). That's very minor.\n\n> +\n> +\tswitch (colorSpace.transferFunction) {\n> +\tcase ColorSpace::TransferFunction::Rec709:\n> +\t\tcolorimetry.transfer = GST_VIDEO_TRANSFER_BT709;\n> +\t\tbreak;\n> +\tcase ColorSpace::TransferFunction::Srgb:\n> +\t\tcolorimetry.transfer = GST_VIDEO_TRANSFER_SRGB;\n> +\t\tbreak;\n> +\tcase ColorSpace::TransferFunction::Linear:\n> +\t\tcolorimetry.transfer = GST_VIDEO_TRANSFER_GAMMA10;\n> +\t\tbreak;\n> +\t}\n> +\n> +\tswitch (colorSpace.ycbcrEncoding) {\n> +\tcase ColorSpace::YcbcrEncoding::None:\n> +\t\tcolorimetry.matrix = GST_VIDEO_COLOR_MATRIX_RGB;\n> +\t\tbreak;\n> +\tcase ColorSpace::YcbcrEncoding::Rec709:\n> +\t\tcolorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT709;\n> +\t\tbreak;\n> +\tcase ColorSpace::YcbcrEncoding::Rec2020:\n> +\t\tcolorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT2020;\n> +\t\tbreak;\n> +\tcase ColorSpace::YcbcrEncoding::Rec601:\n> +\t\tif (colorSpace == ColorSpace::Srgb)\n> +\t\t\tcolorimetry.matrix = GST_VIDEO_COLOR_MATRIX_RGB;\n\nI don't think this one is right. GST_VIDEO_COLOR_MATRIX_RGB is the\nidentity matrix, which means no encoding to YCbCr, and\nColorSpace::YcbcrEncoding::Rec601 clearly defines an encoding.\n\nI think the problem is caused by our definition of ColorSpace::Srgb:\n\nconst ColorSpace ColorSpace::Srgb = {\n\tPrimaries::Rec709,\n\tTransferFunction::Srgb,\n\tYcbcrEncoding::Rec601,\n\tRange::Limited\n};\n\nI propose modifying it to\n\nconst ColorSpace ColorSpace::Srgb = {\n\tPrimaries::Rec709,\n\tTransferFunction::Srgb,\n\tYcbcrEncoding::None,\n\tRange::Full\n};\n\nand adapting the rest of libcamera accordingly.\n\nDavid, this will affect Raspberry Pi, so I'd appreciate your opinion.\n\n> +\t\telse\n> +\t\t\tcolorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT601;\n> +\t\tbreak;\n> +\t}\n> +\n> +\tswitch (colorSpace.range) {\n> +\tcase ColorSpace::Range::Full:\n> +\t\tcolorimetry.range = GST_VIDEO_COLOR_RANGE_0_255;\n> +\t\tbreak;\n> +\tcase ColorSpace::Range::Limited:\n> +\t\tcolorimetry.range = GST_VIDEO_COLOR_RANGE_16_235;\n> +\t\tbreak;\n> +\t}\n> +\n> +\treturn colorimetry;\n> +}\n> +\n> +static ColorSpace\n> +colorspace_from_colorimetry(const GstVideoColorimetry &colorimetry)\n> +{\n> +\tColorSpace colorspace = ColorSpace::Raw;\n> +\n> +\tswitch (colorimetry.primaries) {\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> +\tcase GST_VIDEO_COLOR_PRIMARIES_SMPTE170M:\n> +\t\tcolorspace.primaries = ColorSpace::Primaries::Smpte170m;\n> +\t\tbreak;\n> +\tcase GST_VIDEO_COLOR_PRIMARIES_UNKNOWN:\n> +\t\t/* Unknown primaries map to raw colorspace in gstreamer */\n> +\t\treturn colorspace;\n\nI would\n\n\t\treturn ColorSpace::Raw;\n\nhere to make it more explicit.\n\n> +\tdefault:\n> +\t\tg_error(\"Unsupported colorimetry primaries: %d\", colorimetry.primaries);\n\ng_error() seems very harsh, do we really need to abort() if the user\nasks for unsupported primaries (or other colorimetry fields below) ?\nThis is probably a question for Nicolas.\n\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_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> +\tcase GST_VIDEO_TRANSFER_SRGB:\n> +\t\tcolorspace.transferFunction = ColorSpace::TransferFunction::Srgb;\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tg_error(\"Unsupported colorimetry transfer function: %d\", colorimetry.transfer);\n> +\t}\n> +\n> +\tswitch (colorimetry.matrix) {\n> +\t/* FCC is about the same as BT601 with less digit */\n> +\tcase GST_VIDEO_COLOR_MATRIX_FCC:\n> +\tcase GST_VIDEO_COLOR_MATRIX_BT601:\n> +\t/* v4l2_ycbcr_encoding of sRGB is ENC_601 in the kernel */\n\nThat's right, but I don't think we need to be concerned by that here.\nGST_VIDEO_COLOR_MATRIX_RGB should be mapped to\nColorSpace::YcbcrEncoding::None, and we should handle the V4L2 sRGB YUV\nencoding internally in libcamera.\n\n> +\tcase GST_VIDEO_COLOR_MATRIX_RGB:\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\tg_error(\"Unsupported colorimetry matrix: %d\", colorimetry.matrix);\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\tg_error(\"Unsupported colorimetry range %d\", colorimetry.range);\n> +\t}\n> +\n> +\treturn colorspace;\n> +}\n> +\n> static GstVideoFormat\n> pixel_format_to_gst_format(const PixelFormat &format)\n> {\n> @@ -139,6 +279,18 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg\n> \t\t\t \"width\", G_TYPE_INT, stream_cfg.size.width,\n> \t\t\t \"height\", G_TYPE_INT, stream_cfg.size.height,\n> \t\t\t nullptr);\n> +\n> +\tif (stream_cfg.colorSpace) {\n> +\t\tGstVideoColorimetry colorimetry = colorimetry_from_colorspace(stream_cfg.colorSpace.value());\n> +\t\tgchar *colorimetry_str = gst_video_colorimetry_to_string(&colorimetry);\n> +\n> +\t\tif (colorimetry_str)\n> +\t\t\tgst_structure_set(s, \"colorimetry\", G_TYPE_STRING, colorimetry_str, nullptr);\n> +\t\telse\n> +\t\t\tg_error(\"Got invalid colorimetry from ColorSpace: %s\",\n> +\t\t\t\tColorSpace::toString(stream_cfg.colorSpace).c_str());\n\nSame question as above about g_error(). As far as I understand,\ngst_video_colorimetry_to_string() will return null either if it can't\nalocate memory for the string (in which case we're screwed anyway, so\ng_error() is fine) or if colorimetry is all UNKNOWN. The latter should\nnever happen given the implementation of colorimetry_from_colorspace(),\nso I suppose it's fine ?\n\n> +\t}\n> +\n> \tgst_caps_append_structure(caps, s);\n> \n> \treturn caps;\n> @@ -222,6 +374,18 @@ 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/* Configure colorimetry */\n> +\tif (gst_structure_has_field(s, \"colorimetry\")) {\n> +\t\tconst gchar *colorimetry_caps = gst_structure_get_string(s, \"colorimetry\");\n> +\t\tGstVideoColorimetry colorimetry;\n> +\n> +\t\tif(gst_video_colorimetry_from_string(&colorimetry, colorimetry_caps)) {\n\nMissing space after \"if\".\n\n> +\t\t\tstream_cfg.colorSpace = colorspace_from_colorimetry(colorimetry);\n> +\t\t} else {\n> +\t\t\tg_critical(\"Invalid colorimetry %s\", colorimetry_caps);\n> +\t\t}\n> +\t}\n> }\n> \n> #if !GST_CHECK_VERSION(1, 17, 1)","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 5EB62BE173\n\tfor ;\n\tSun, 31 Jul 2022 19:44:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6432F63312;\n\tSun, 31 Jul 2022 21:44:50 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D7093603E9\n\tfor ;\n\tSun, 31 Jul 2022 21:44:48 +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 1FC6A415;\n\tSun, 31 Jul 2022 21:44:48 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659296690;\n\tbh=vuPG5QfQKImpurS6j+Kx49Ix6uM76PlO7NY1cxabAvQ=;\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=wdCfstc3C/xiVZdgcSEnFW3U7E/WnAOhGXAW/DjZMTuEbW7EDDJlt5WAMpKy/xSUz\n\tojZ9akFyu53yBslDHT4SrDJvL+c07K5GGy48PIYugIpd7LWG9IBj/u93qPTfcezsy/\n\t1g6XL6ClP40erDU9TjSgJuWP6DeHg1WAF4+b4m25WMCOECdEO9xHc5P+egav4cFRAi\n\tfzWgsAGnVitklNOoZDmBf0lfLSs3Z3Xb3JHoXziKTyQqFBlasZfoEAryhSEDVycti0\n\tfqPozmYx6xdKWtI99l5yYuCDNyHkUr8QfjNtckT0Diq8NVQp5sOXe6SHiyyc7JemjJ\n\t8aw1P3B3XUU5g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1659296688;\n\tbh=vuPG5QfQKImpurS6j+Kx49Ix6uM76PlO7NY1cxabAvQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nrf8YZsEc9bytbO4NRPsWbX2F/1bXmTQN186qOXMmYBOMOzcRYZc2m+zUG8pt5Vnd\n\tGPg3yQsZ7nkggqeOgQOx0ca35uPlveJ9dGnuuYxVV0g0XJtk25z0BDWb3iU7EqUFdj\n\tf7qOILdkyfO6tn+I9cWGSPskI/iSuNoyVbmQ4JvQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"nrf8YZsE\"; dkim-atps=neutral","Date":"Sun, 31 Jul 2022 22:44:43 +0300","To":"Umang Jain ","Message-ID":"","References":"<20220729103846.414819-1-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220729103846.414819-1-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Provide colorimetry <>\n\tColorSpace mappings","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":"rishikeshdonadkar@gmail.com, libcamera-devel@lists.libcamera.org,\n\tvedantparanjape160201@gmail.com, nicolas.dufresne@collabora.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24257,"web_url":"https://patchwork.libcamera.org/comment/24257/","msgid":"","date":"2022-08-01T02:12:51","subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Provide colorimetry <>\n\tColorSpace mappings","submitter":{"id":118,"url":"https://patchwork.libcamera.org/api/people/118/","name":"Rishikesh Donadkar","email":"rishikeshdonadkar@gmail.com"},"content":"Hello Laurent,\n\n> > argument. Multiple colorimetry support shall be introduced in\n> > subsequent commits.\n>\n> I'm probably missing something here, what do you mean by multiple\n> colorimetry support ?\n\nIn the GStreamer pipeline multiple colorimetries can be specified\nlike the following:\n\nlibcamerasrc ! \"video/x-raw,colorimetry={bt709,bt2020}\" ! glimagesink\n\nHere we would try bt709 or bt2020, if the colorspace corresponding\nto any one of the above gets applied the negotiation will\npass else it should fail.\n\nRegards,\n\nRishikesh Donadkar\n\nOn Mon, Aug 1, 2022 at 1:14 AM Laurent Pinchart\n wrote:\n>\n> Hi Umang and Rishikesh,\n>\n> (Cc'ing David)\n>\n> Thank you for the patch.\n>\n> On Fri, Jul 29, 2022 at 04:08:46PM +0530, Umang Jain via libcamera-devel wrote:\n> > From: Rishikesh Donadkar \n> >\n> > Provide colorimetry <=> libcamera::ColorSpace mappings via:\n> > - GstVideoColorimetry colorimetry_from_colorspace(colorspace);\n> > - ColorSpace colorspace_from_colorimetry(colorimetry);\n> >\n> > Read the colorimetry field from caps into the stream configuration.\n> > After stream validation, the sensor supported colorimetry will\n> > be retrieved and the caps will be updated accordingly.\n> >\n> > Colorimetry support for gstlibcamerasrc currently undertakes only one\n>\n> s/gstlibcamerasrc/libcamerasrc/ (or gstlibcamera if you meant the\n> library, not the element name)\n>\n> > argument. Multiple colorimetry support shall be introduced in\n> > subsequent commits.\n>\n> I'm probably missing something here, what do you mean by multiple\n> colorimetry support ?\n>\n> > Signed-off-by: Rishikesh Donadkar \n> > Signed-off-by: Umang Jain \n> > ---\n> >\n> > Changes in v2:\n> > - Drop \"Default\" Colorspace\n> > - Improve function signature of colorimetry_from_colorspace() and\n> > colorspace_from_colorimetry()\n> > - Map GST_VIDEO_COLOR_MATRIX_RGB to Colorspace::YcbcrEncoding::Rec601\n> > (comes from the kernel)\n> > - Map GST_VIDEO_COLOR_PRIMARIES_UNKNOWN to Raw colorspace\n> > - Map ColorSpace::YcbcrEncoding::Rec601 to GST_VIDEO_COLOR_MATRIX_RGB\n> > if colorspace is \"sRGB\". GST_VIDEO_COLOR_MATRIX_BT601 for all other\n> > cases\n> > - Minor nits regarding error reporting strings.\n> > ---\n> > src/gstreamer/gstlibcamera-utils.cpp | 164 +++++++++++++++++++++++++++\n> > 1 file changed, 164 insertions(+)\n> >\n> > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> > index c97c0d43..e4ff1269 100644\n> > --- a/src/gstreamer/gstlibcamera-utils.cpp\n> > +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> > @@ -45,6 +45,146 @@ static struct {\n> > /* \\todo NV42 is used in libcamera but is not mapped in GStreamer yet. */\n> > };\n> >\n> > +static GstVideoColorimetry\n> > +colorimetry_from_colorspace(const ColorSpace &colorSpace)\n> > +{\n> > + GstVideoColorimetry colorimetry;\n> > +\n> > + switch (colorSpace.primaries) {\n> > + case ColorSpace::Primaries::Rec709:\n> > + colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_BT709;\n> > + break;\n> > + case ColorSpace::Primaries::Rec2020:\n> > + colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_BT2020;\n> > + break;\n> > + case ColorSpace::Primaries::Smpte170m:\n> > + colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_SMPTE170M;\n> > + break;\n> > + case ColorSpace::Primaries::Raw:\n> > + colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_UNKNOWN;\n> > + break;\n> > + }\n>\n> I'd sort the cases with the order of the enum in color_space.h (below\n> too). That's very minor.\n>\n> > +\n> > + switch (colorSpace.transferFunction) {\n> > + case ColorSpace::TransferFunction::Rec709:\n> > + colorimetry.transfer = GST_VIDEO_TRANSFER_BT709;\n> > + break;\n> > + case ColorSpace::TransferFunction::Srgb:\n> > + colorimetry.transfer = GST_VIDEO_TRANSFER_SRGB;\n> > + break;\n> > + case ColorSpace::TransferFunction::Linear:\n> > + colorimetry.transfer = GST_VIDEO_TRANSFER_GAMMA10;\n> > + break;\n> > + }\n> > +\n> > + switch (colorSpace.ycbcrEncoding) {\n> > + case ColorSpace::YcbcrEncoding::None:\n> > + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_RGB;\n> > + break;\n> > + case ColorSpace::YcbcrEncoding::Rec709:\n> > + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT709;\n> > + break;\n> > + case ColorSpace::YcbcrEncoding::Rec2020:\n> > + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT2020;\n> > + break;\n> > + case ColorSpace::YcbcrEncoding::Rec601:\n> > + if (colorSpace == ColorSpace::Srgb)\n> > + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_RGB;\n>\n> I don't think this one is right. GST_VIDEO_COLOR_MATRIX_RGB is the\n> identity matrix, which means no encoding to YCbCr, and\n> ColorSpace::YcbcrEncoding::Rec601 clearly defines an encoding.\n>\n> I think the problem is caused by our definition of ColorSpace::Srgb:\n>\n> const ColorSpace ColorSpace::Srgb = {\n> Primaries::Rec709,\n> TransferFunction::Srgb,\n> YcbcrEncoding::Rec601,\n> Range::Limited\n> };\n>\n> I propose modifying it to\n>\n> const ColorSpace ColorSpace::Srgb = {\n> Primaries::Rec709,\n> TransferFunction::Srgb,\n> YcbcrEncoding::None,\n> Range::Full\n> };\n>\n> and adapting the rest of libcamera accordingly.\n>\n> David, this will affect Raspberry Pi, so I'd appreciate your opinion.\n>\n> > + else\n> > + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT601;\n> > + break;\n> > + }\n> > +\n> > + switch (colorSpace.range) {\n> > + case ColorSpace::Range::Full:\n> > + colorimetry.range = GST_VIDEO_COLOR_RANGE_0_255;\n> > + break;\n> > + case ColorSpace::Range::Limited:\n> > + colorimetry.range = GST_VIDEO_COLOR_RANGE_16_235;\n> > + break;\n> > + }\n> > +\n> > + return colorimetry;\n> > +}\n> > +\n> > +static ColorSpace\n> > +colorspace_from_colorimetry(const GstVideoColorimetry &colorimetry)\n> > +{\n> > + ColorSpace colorspace = ColorSpace::Raw;\n> > +\n> > + switch (colorimetry.primaries) {\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> > + case GST_VIDEO_COLOR_PRIMARIES_SMPTE170M:\n> > + colorspace.primaries = ColorSpace::Primaries::Smpte170m;\n> > + break;\n> > + case GST_VIDEO_COLOR_PRIMARIES_UNKNOWN:\n> > + /* Unknown primaries map to raw colorspace in gstreamer */\n> > + return colorspace;\n>\n> I would\n>\n> return ColorSpace::Raw;\n>\n> here to make it more explicit.\n>\n> > + default:\n> > + g_error(\"Unsupported colorimetry primaries: %d\", colorimetry.primaries);\n>\n> g_error() seems very harsh, do we really need to abort() if the user\n> asks for unsupported primaries (or other colorimetry fields below) ?\n> This is probably a question for Nicolas.\n>\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 supported\");\n> > + /* fallthrough */\n> > + case GST_VIDEO_TRANSFER_GAMMA10:\n> > + colorspace.transferFunction = ColorSpace::TransferFunction::Linear;\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 = ColorSpace::TransferFunction::Rec709;\n> > + break;\n> > + case GST_VIDEO_TRANSFER_SRGB:\n> > + colorspace.transferFunction = ColorSpace::TransferFunction::Srgb;\n> > + break;\n> > + default:\n> > + g_error(\"Unsupported colorimetry transfer function: %d\", colorimetry.transfer);\n> > + }\n> > +\n> > + switch (colorimetry.matrix) {\n> > + /* FCC is about the same as BT601 with less digit */\n> > + case GST_VIDEO_COLOR_MATRIX_FCC:\n> > + case GST_VIDEO_COLOR_MATRIX_BT601:\n> > + /* v4l2_ycbcr_encoding of sRGB is ENC_601 in the kernel */\n>\n> That's right, but I don't think we need to be concerned by that here.\n> GST_VIDEO_COLOR_MATRIX_RGB should be mapped to\n> ColorSpace::YcbcrEncoding::None, and we should handle the V4L2 sRGB YUV\n> encoding internally in libcamera.\n>\n> > + case GST_VIDEO_COLOR_MATRIX_RGB:\n> > + colorspace.ycbcrEncoding = ColorSpace::YcbcrEncoding::Rec601;\n> > + break;\n> > + case GST_VIDEO_COLOR_MATRIX_BT709:\n> > + colorspace.ycbcrEncoding = ColorSpace::YcbcrEncoding::Rec709;\n> > + break;\n> > + case GST_VIDEO_COLOR_MATRIX_BT2020:\n> > + colorspace.ycbcrEncoding = ColorSpace::YcbcrEncoding::Rec2020;\n> > + break;\n> > + default:\n> > + g_error(\"Unsupported colorimetry matrix: %d\", colorimetry.matrix);\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> > + g_error(\"Unsupported colorimetry range %d\", colorimetry.range);\n> > + }\n> > +\n> > + return colorspace;\n> > +}\n> > +\n> > static GstVideoFormat\n> > pixel_format_to_gst_format(const PixelFormat &format)\n> > {\n> > @@ -139,6 +279,18 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg\n> > \"width\", G_TYPE_INT, stream_cfg.size.width,\n> > \"height\", G_TYPE_INT, stream_cfg.size.height,\n> > nullptr);\n> > +\n> > + if (stream_cfg.colorSpace) {\n> > + GstVideoColorimetry colorimetry = colorimetry_from_colorspace(stream_cfg.colorSpace.value());\n> > + gchar *colorimetry_str = gst_video_colorimetry_to_string(&colorimetry);\n> > +\n> > + if (colorimetry_str)\n> > + gst_structure_set(s, \"colorimetry\", G_TYPE_STRING, colorimetry_str, nullptr);\n> > + else\n> > + g_error(\"Got invalid colorimetry from ColorSpace: %s\",\n> > + ColorSpace::toString(stream_cfg.colorSpace).c_str());\n>\n> Same question as above about g_error(). As far as I understand,\n> gst_video_colorimetry_to_string() will return null either if it can't\n> alocate memory for the string (in which case we're screwed anyway, so\n> g_error() is fine) or if colorimetry is all UNKNOWN. The latter should\n> never happen given the implementation of colorimetry_from_colorspace(),\n> so I suppose it's fine ?\n>\n> > + }\n> > +\n> > gst_caps_append_structure(caps, s);\n> >\n> > return caps;\n> > @@ -222,6 +374,18 @@ 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> > + /* Configure colorimetry */\n> > + if (gst_structure_has_field(s, \"colorimetry\")) {\n> > + const gchar *colorimetry_caps = gst_structure_get_string(s, \"colorimetry\");\n> > + GstVideoColorimetry colorimetry;\n> > +\n> > + if(gst_video_colorimetry_from_string(&colorimetry, colorimetry_caps)) {\n>\n> Missing space after \"if\".\n>\n> > + stream_cfg.colorSpace = colorspace_from_colorimetry(colorimetry);\n> > + } else {\n> > + g_critical(\"Invalid colorimetry %s\", colorimetry_caps);\n> > + }\n> > + }\n> > }\n> >\n> > #if !GST_CHECK_VERSION(1, 17, 1)\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 E2D2AC3275\n\tfor ;\n\tMon, 1 Aug 2022 02:13:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 33B276330D;\n\tMon, 1 Aug 2022 04:13:06 +0200 (CEST)","from mail-vs1-xe35.google.com (mail-vs1-xe35.google.com\n\t[IPv6:2607:f8b0:4864:20::e35])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 75C7B603F2\n\tfor ;\n\tMon, 1 Aug 2022 04:13:04 +0200 (CEST)","by mail-vs1-xe35.google.com with SMTP id j2so3377261vsp.1\n\tfor ;\n\tSun, 31 Jul 2022 19:13:04 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659319986;\n\tbh=14xOMAgM4ZhHAFz3dguTJ8zA6R2gODD9IEIQEisteJw=;\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=hI7sQMW9Pn00p1qTwWJecHH4M6ICgmCO6bupPZ76kn5aMm24B6tBqPjVCBN/TbYE7\n\tPZkmULhFMhHkK5YF8VNhh0Tf4gC1+onscB4z5zMz2VoyBYiXqNKzmTXTb4qh8Lhbh6\n\tX1MaR3VEZMlBJkX8VPR4OLujG6QLet2SXSGgSHUk8uwxpBn9M9ckCvUF2ou9aN9o1k\n\tkiNw+LEbawcdXLmPxW5HG2HTFfmizPib7Sl8vJFD1NOm1Qlv/uW/eHeifaD0IQPPfx\n\tYDZ9i7GFzAbaRRgtfxad7zJysqWEfPaG5yKdrImuMmVO9AYZQDCEgt6/zg09/o/9Kq\n\tg1kEg2q0j6c1Q==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=MhI2JBr5e1Qeu3uDUWHb9yFh7SkvuegzDfvMUgSLUa4=;\n\tb=d9081IVFpnGLNFkGMuUWrnRQsFLf65fyKWksmhC5E1Ngti92lCPyHAj/mVZtGBz9jJ\n\tVz1J0JdfZyZN5HVmZOpTcfdkM70fFtiO+uL0LtPAxBsi7Xfx9Qvx4J4UNsuL8NNN1cuW\n\t/rfGTG4II8hsYvBlamjZlw9OZXkwOJavRSV1uawU0/NerIPPCFXbJXdjKMmxHO583uKe\n\twZsc1C24j13coOcMsHkZbZMMmjZ3H8D/aqM8VgmyL+nt6CNdfROe3iN2h2Cyo+VaMY5j\n\tPHK1Y9g9euoYD4traUsqi3KH5GW6xS6fz8GWwL+OQ4yByg1ob1O5YW8PMq3Xi2+9UypO\n\t+Oaw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"d9081IVF\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=MhI2JBr5e1Qeu3uDUWHb9yFh7SkvuegzDfvMUgSLUa4=;\n\tb=8EI0rHWCE9R+5ejXvxvLX+6uoEVJutamE+qcq+RnebJSPhlQCAPvtz6S3EirRhfVGK\n\tRL3GAHzirsX1NTIvGx9qF0Loodbted9gEqSkBiSNWqsH0cMcCRdRWi6H7AQjbpKqj9vL\n\tYwNH7vgEQcfBdFQKYJ/KlVvb4AkeZ9WGpKsVcvuLneXULn4aVtMj24FXP2frupkX2s+a\n\tJ6S4i59BQ/O9yzUrPhsAUB+kXEdAVFIIu7XTJhO0SRQUXcbsi7YYwUwPXo7gjTmoUQ6j\n\tus69UoYRn7QDttZVVb7l2gWdEvhZsyj8WVwbr/Gfg/WkTPJtyEJlt2YQAnoMxou0HJhU\n\t1leg==","X-Gm-Message-State":"AJIora/4AV+B3xYwTfYXfYuYhqP4cR0wg152DARvVCeCKbBAfR24WtzH\n\t0Q34+nNgMqGxmqol6lth5SCfJ60Bq+p8exMfZwQ=","X-Google-Smtp-Source":"AGRyM1sow95s2guzi8C7Zqht5ILvmI/GR9pya2rbqrt4I8wb7p5Nq6P7Zefi4aNQoRHQEpgm+gLrV01lbfoFD+vnWs4=","X-Received":"by 2002:a67:2486:0:b0:354:565c:62ec with SMTP id\n\tk128-20020a672486000000b00354565c62ecmr4973987vsk.26.1659319983189;\n\tSun, 31 Jul 2022 19:13:03 -0700 (PDT)","MIME-Version":"1.0","References":"<20220729103846.414819-1-umang.jain@ideasonboard.com>\n\t","In-Reply-To":"","Date":"Mon, 1 Aug 2022 07:42:51 +0530","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Provide colorimetry <>\n\tColorSpace mappings","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\" "}},{"id":24262,"web_url":"https://patchwork.libcamera.org/comment/24262/","msgid":"","date":"2022-08-01T12:31:40","subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Provide colorimetry <>\n\tColorSpace mappings","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 8/1/22 01:14, Laurent Pinchart wrote:\n> Hi Umang and Rishikesh,\n>\n> (Cc'ing David)\n>\n> Thank you for the patch.\n>\n> On Fri, Jul 29, 2022 at 04:08:46PM +0530, Umang Jain via libcamera-devel wrote:\n>> From: Rishikesh Donadkar \n>>\n>> Provide colorimetry <=> libcamera::ColorSpace mappings via:\n>> - GstVideoColorimetry colorimetry_from_colorspace(colorspace);\n>> - ColorSpace colorspace_from_colorimetry(colorimetry);\n>>\n>> Read the colorimetry field from caps into the stream configuration.\n>> After stream validation, the sensor supported colorimetry will\n>> be retrieved and the caps will be updated accordingly.\n>>\n>> Colorimetry support for gstlibcamerasrc currently undertakes only one\n> s/gstlibcamerasrc/libcamerasrc/ (or gstlibcamera if you meant the\n> library, not the element name)\n>\n>> argument. Multiple colorimetry support shall be introduced in\n>> subsequent commits.\n> I'm probably missing something here, what do you mean by multiple\n> colorimetry support ?\n>\n>> Signed-off-by: Rishikesh Donadkar \n>> Signed-off-by: Umang Jain \n>> ---\n>>\n>> Changes in v2:\n>> - Drop \"Default\" Colorspace\n>> - Improve function signature of colorimetry_from_colorspace() and\n>> colorspace_from_colorimetry()\n>> - Map GST_VIDEO_COLOR_MATRIX_RGB to Colorspace::YcbcrEncoding::Rec601\n>> (comes from the kernel)\n>> - Map GST_VIDEO_COLOR_PRIMARIES_UNKNOWN to Raw colorspace\n>> - Map ColorSpace::YcbcrEncoding::Rec601 to GST_VIDEO_COLOR_MATRIX_RGB\n>> if colorspace is \"sRGB\". GST_VIDEO_COLOR_MATRIX_BT601 for all other\n>> cases\n>> - Minor nits regarding error reporting strings.\n>> ---\n>> src/gstreamer/gstlibcamera-utils.cpp | 164 +++++++++++++++++++++++++++\n>> 1 file changed, 164 insertions(+)\n>>\n>> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n>> index c97c0d43..e4ff1269 100644\n>> --- a/src/gstreamer/gstlibcamera-utils.cpp\n>> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n>> @@ -45,6 +45,146 @@ static struct {\n>> \t/* \\todo NV42 is used in libcamera but is not mapped in GStreamer yet. */\n>> };\n>> \n>> +static GstVideoColorimetry\n>> +colorimetry_from_colorspace(const ColorSpace &colorSpace)\n>> +{\n>> +\tGstVideoColorimetry colorimetry;\n>> +\n>> +\tswitch (colorSpace.primaries) {\n>> +\tcase ColorSpace::Primaries::Rec709:\n>> +\t\tcolorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_BT709;\n>> +\t\tbreak;\n>> +\tcase ColorSpace::Primaries::Rec2020:\n>> +\t\tcolorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_BT2020;\n>> +\t\tbreak;\n>> +\tcase ColorSpace::Primaries::Smpte170m:\n>> +\t\tcolorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_SMPTE170M;\n>> +\t\tbreak;\n>> +\tcase ColorSpace::Primaries::Raw:\n>> +\t\tcolorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_UNKNOWN;\n>> +\t\tbreak;\n>> +\t}\n> I'd sort the cases with the order of the enum in color_space.h (below\n> too). That's very minor.\n\n\nAck.\n\n>\n>> +\n>> +\tswitch (colorSpace.transferFunction) {\n>> +\tcase ColorSpace::TransferFunction::Rec709:\n>> +\t\tcolorimetry.transfer = GST_VIDEO_TRANSFER_BT709;\n>> +\t\tbreak;\n>> +\tcase ColorSpace::TransferFunction::Srgb:\n>> +\t\tcolorimetry.transfer = GST_VIDEO_TRANSFER_SRGB;\n>> +\t\tbreak;\n>> +\tcase ColorSpace::TransferFunction::Linear:\n>> +\t\tcolorimetry.transfer = GST_VIDEO_TRANSFER_GAMMA10;\n>> +\t\tbreak;\n>> +\t}\n>> +\n>> +\tswitch (colorSpace.ycbcrEncoding) {\n>> +\tcase ColorSpace::YcbcrEncoding::None:\n>> +\t\tcolorimetry.matrix = GST_VIDEO_COLOR_MATRIX_RGB;\n>> +\t\tbreak;\n>> +\tcase ColorSpace::YcbcrEncoding::Rec709:\n>> +\t\tcolorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT709;\n>> +\t\tbreak;\n>> +\tcase ColorSpace::YcbcrEncoding::Rec2020:\n>> +\t\tcolorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT2020;\n>> +\t\tbreak;\n>> +\tcase ColorSpace::YcbcrEncoding::Rec601:\n>> +\t\tif (colorSpace == ColorSpace::Srgb)\n>> +\t\t\tcolorimetry.matrix = GST_VIDEO_COLOR_MATRIX_RGB;\n> I don't think this one is right. GST_VIDEO_COLOR_MATRIX_RGB is the\n> identity matrix, which means no encoding to YCbCr, and\n> ColorSpace::YcbcrEncoding::Rec601 clearly defines an encoding.\n>\n> I think the problem is caused by our definition of ColorSpace::Srgb:\n>\n> const ColorSpace ColorSpace::Srgb = {\n> \tPrimaries::Rec709,\n> \tTransferFunction::Srgb,\n> \tYcbcrEncoding::Rec601,\n> \tRange::Limited\n> };\n>\n> I propose modifying it to\n>\n> const ColorSpace ColorSpace::Srgb = {\n> \tPrimaries::Rec709,\n> \tTransferFunction::Srgb,\n> \tYcbcrEncoding::None,\n> \tRange::Full\n> };\n\n\nack!\n\nMy question which I think I brought up in v1 itself now (you can choose \nto reply anywhere) is, do we need a preset to represent YUV-encoded RGB \nand what would we call it? ColorSpace::sYCC ?\n\n>\n> and adapting the rest of libcamera accordingly.\n>\n> David, this will affect Raspberry Pi, so I'd appreciate your opinion.\n\n\nPatiently waiting ;-)\n\n>\n>> +\t\telse\n>> +\t\t\tcolorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT601;\n>> +\t\tbreak;\n>> +\t}\n>> +\n>> +\tswitch (colorSpace.range) {\n>> +\tcase ColorSpace::Range::Full:\n>> +\t\tcolorimetry.range = GST_VIDEO_COLOR_RANGE_0_255;\n>> +\t\tbreak;\n>> +\tcase ColorSpace::Range::Limited:\n>> +\t\tcolorimetry.range = GST_VIDEO_COLOR_RANGE_16_235;\n>> +\t\tbreak;\n>> +\t}\n>> +\n>> +\treturn colorimetry;\n>> +}\n>> +\n>> +static ColorSpace\n>> +colorspace_from_colorimetry(const GstVideoColorimetry &colorimetry)\n>> +{\n>> +\tColorSpace colorspace = ColorSpace::Raw;\n>> +\n>> +\tswitch (colorimetry.primaries) {\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>> +\tcase GST_VIDEO_COLOR_PRIMARIES_SMPTE170M:\n>> +\t\tcolorspace.primaries = ColorSpace::Primaries::Smpte170m;\n>> +\t\tbreak;\n>> +\tcase GST_VIDEO_COLOR_PRIMARIES_UNKNOWN:\n>> +\t\t/* Unknown primaries map to raw colorspace in gstreamer */\n>> +\t\treturn colorspace;\n> I would\n>\n> \t\treturn ColorSpace::Raw;\n>\n> here to make it more explicit.\n\n\nack\n\n>\n>> +\tdefault:\n>> +\t\tg_error(\"Unsupported colorimetry primaries: %d\", colorimetry.primaries);\n> g_error() seems very harsh, do we really need to abort() if the user\n> asks for unsupported primaries (or other colorimetry fields below) ?\n\n\nI agree it's harsh - initially I kept as 'warning' but then it was \nadvised that 'warnings' always come with mitigation steps.\n\nI guess the only mitigation technique here is, making sure mappings exists!\n\n> This is probably a question for Nicolas.\n>\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_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>> +\tcase GST_VIDEO_TRANSFER_SRGB:\n>> +\t\tcolorspace.transferFunction = ColorSpace::TransferFunction::Srgb;\n>> +\t\tbreak;\n>> +\tdefault:\n>> +\t\tg_error(\"Unsupported colorimetry transfer function: %d\", colorimetry.transfer);\n>> +\t}\n>> +\n>> +\tswitch (colorimetry.matrix) {\n>> +\t/* FCC is about the same as BT601 with less digit */\n>> +\tcase GST_VIDEO_COLOR_MATRIX_FCC:\n>> +\tcase GST_VIDEO_COLOR_MATRIX_BT601:\n>> +\t/* v4l2_ycbcr_encoding of sRGB is ENC_601 in the kernel */\n> That's right, but I don't think we need to be concerned by that here.\n> GST_VIDEO_COLOR_MATRIX_RGB should be mapped to\n> ColorSpace::YcbcrEncoding::None, and we should handle the V4L2 sRGB YUV\n> encoding internally in libcamera.\n\n\nreplied to the latter part, in detail, in v1\n\n>\n>> +\tcase GST_VIDEO_COLOR_MATRIX_RGB:\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\tg_error(\"Unsupported colorimetry matrix: %d\", colorimetry.matrix);\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\tg_error(\"Unsupported colorimetry range %d\", colorimetry.range);\n>> +\t}\n>> +\n>> +\treturn colorspace;\n>> +}\n>> +\n>> static GstVideoFormat\n>> pixel_format_to_gst_format(const PixelFormat &format)\n>> {\n>> @@ -139,6 +279,18 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg\n>> \t\t\t \"width\", G_TYPE_INT, stream_cfg.size.width,\n>> \t\t\t \"height\", G_TYPE_INT, stream_cfg.size.height,\n>> \t\t\t nullptr);\n>> +\n>> +\tif (stream_cfg.colorSpace) {\n>> +\t\tGstVideoColorimetry colorimetry = colorimetry_from_colorspace(stream_cfg.colorSpace.value());\n>> +\t\tgchar *colorimetry_str = gst_video_colorimetry_to_string(&colorimetry);\n>> +\n>> +\t\tif (colorimetry_str)\n>> +\t\t\tgst_structure_set(s, \"colorimetry\", G_TYPE_STRING, colorimetry_str, nullptr);\n>> +\t\telse\n>> +\t\t\tg_error(\"Got invalid colorimetry from ColorSpace: %s\",\n>> +\t\t\t\tColorSpace::toString(stream_cfg.colorSpace).c_str());\n> Same question as above about g_error(). As far as I understand,\n> gst_video_colorimetry_to_string() will return null either if it can't\n> alocate memory for the string (in which case we're screwed anyway, so\n> g_error() is fine) or if colorimetry is all UNKNOWN. The latter should\n> never happen given the implementation of colorimetry_from_colorspace(),\n> so I suppose it's fine ?\n\n\nYes, I think it's fine\n\n>\n>> +\t}\n>> +\n>> \tgst_caps_append_structure(caps, s);\n>> \n>> \treturn caps;\n>> @@ -222,6 +374,18 @@ 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/* Configure colorimetry */\n>> +\tif (gst_structure_has_field(s, \"colorimetry\")) {\n>> +\t\tconst gchar *colorimetry_caps = gst_structure_get_string(s, \"colorimetry\");\n>> +\t\tGstVideoColorimetry colorimetry;\n>> +\n>> +\t\tif(gst_video_colorimetry_from_string(&colorimetry, colorimetry_caps)) {\n> Missing space after \"if\".\n>\n>> +\t\t\tstream_cfg.colorSpace = colorspace_from_colorimetry(colorimetry);\n>> +\t\t} else {\n>> +\t\t\tg_critical(\"Invalid colorimetry %s\", colorimetry_caps);\n>> +\t\t}\n>> +\t}\n>> }\n>> \n>> #if !GST_CHECK_VERSION(1, 17, 1)","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 43DCFC3275\n\tfor ;\n\tMon, 1 Aug 2022 12:31:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9BD3C6330F;\n\tMon, 1 Aug 2022 14:31: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 D0823603E8\n\tfor ;\n\tMon, 1 Aug 2022 14:31:47 +0200 (CEST)","from [IPV6:2401:4900:1f3e:f7a:bc8f:12ed:b45f:c35d] (unknown\n\t[IPv6:2401:4900:1f3e:f7a:bc8f:12ed:b45f:c35d])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9B45A2F3;\n\tMon, 1 Aug 2022 14:31:45 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659357109;\n\tbh=w+gHKNZZJjakTJ4+KfSiK298hU3a56oMr1ZbE+UcdFI=;\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=Vv2Yh7zLB9AJvDAaYjeGmzB1xa1edSggZG31VgB4cglt3xpkJJltNLVvzsDJTOjUb\n\tjH1amGbMr3BfE1LxWiMZQ5URIkyl5tGwT2c4l+OlDm1t9zLE/BReWJSR4H7BLIYIrJ\n\t9WNkwo/sA8kJly8VT0eyu/ge7FaVGQV14DJiUoT3iIASLH4JIMgojS8tni4YomWg9+\n\taiDa7dN+qCG9CRAxrTa2nJNPdAdOvdOBjbqEhK/S6kSP1QizrT6TMq8DPrxOYxRzCa\n\tzBHxO0HmsqMfZHCneJCXAn41Lz2v2sCGB5AKKzJ7yl4Y3RiSNAuogABvYCxWg1t95o\n\tZwCTVlEu6o4+w==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1659357107;\n\tbh=w+gHKNZZJjakTJ4+KfSiK298hU3a56oMr1ZbE+UcdFI=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=M4gZ2CZc/ga1xVWUycfZVd93LZ1vSuxXQfo8fRYL0UM60xKmkU296B4hT0yROgQnS\n\tYW3hxsVJ2y6L+w8+2XgEVgz1e1qo3tPYmxRML0JPcUcvvqktb6qHeEswI8cY43DUlx\n\t5ypyT5Gl1kRAR8oFWJlqBhnGlDjsfytHRpKXQ1H0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"M4gZ2CZc\"; dkim-atps=neutral","Message-ID":"","Date":"Mon, 1 Aug 2022 18:01:40 +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":"Laurent Pinchart ","References":"<20220729103846.414819-1-umang.jain@ideasonboard.com>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Provide colorimetry <>\n\tColorSpace mappings","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":"rishikeshdonadkar@gmail.com, libcamera-devel@lists.libcamera.org,\n\tvedantparanjape160201@gmail.com, nicolas.dufresne@collabora.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24263,"web_url":"https://patchwork.libcamera.org/comment/24263/","msgid":"","date":"2022-08-01T12:42:05","subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Provide colorimetry <>\n\tColorSpace mappings","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Mon, Aug 01, 2022 at 06:01:40PM +0530, Umang Jain wrote:\n> On 8/1/22 01:14, Laurent Pinchart wrote:\n> > Hi Umang and Rishikesh,\n> >\n> > (Cc'ing David)\n> >\n> > Thank you for the patch.\n> >\n> > On Fri, Jul 29, 2022 at 04:08:46PM +0530, Umang Jain via libcamera-devel wrote:\n> >> From: Rishikesh Donadkar \n> >>\n> >> Provide colorimetry <=> libcamera::ColorSpace mappings via:\n> >> - GstVideoColorimetry colorimetry_from_colorspace(colorspace);\n> >> - ColorSpace colorspace_from_colorimetry(colorimetry);\n> >>\n> >> Read the colorimetry field from caps into the stream configuration.\n> >> After stream validation, the sensor supported colorimetry will\n> >> be retrieved and the caps will be updated accordingly.\n> >>\n> >> Colorimetry support for gstlibcamerasrc currently undertakes only one\n> >\n> > s/gstlibcamerasrc/libcamerasrc/ (or gstlibcamera if you meant the\n> > library, not the element name)\n> >\n> >> argument. Multiple colorimetry support shall be introduced in\n> >> subsequent commits.\n> >\n> > I'm probably missing something here, what do you mean by multiple\n> > colorimetry support ?\n> >\n> >> Signed-off-by: Rishikesh Donadkar \n> >> Signed-off-by: Umang Jain \n> >> ---\n> >>\n> >> Changes in v2:\n> >> - Drop \"Default\" Colorspace\n> >> - Improve function signature of colorimetry_from_colorspace() and\n> >> colorspace_from_colorimetry()\n> >> - Map GST_VIDEO_COLOR_MATRIX_RGB to Colorspace::YcbcrEncoding::Rec601\n> >> (comes from the kernel)\n> >> - Map GST_VIDEO_COLOR_PRIMARIES_UNKNOWN to Raw colorspace\n> >> - Map ColorSpace::YcbcrEncoding::Rec601 to GST_VIDEO_COLOR_MATRIX_RGB\n> >> if colorspace is \"sRGB\". GST_VIDEO_COLOR_MATRIX_BT601 for all other\n> >> cases\n> >> - Minor nits regarding error reporting strings.\n> >> ---\n> >> src/gstreamer/gstlibcamera-utils.cpp | 164 +++++++++++++++++++++++++++\n> >> 1 file changed, 164 insertions(+)\n> >>\n> >> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> >> index c97c0d43..e4ff1269 100644\n> >> --- a/src/gstreamer/gstlibcamera-utils.cpp\n> >> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> >> @@ -45,6 +45,146 @@ static struct {\n> >> \t/* \\todo NV42 is used in libcamera but is not mapped in GStreamer yet. */\n> >> };\n> >> \n> >> +static GstVideoColorimetry\n> >> +colorimetry_from_colorspace(const ColorSpace &colorSpace)\n> >> +{\n> >> +\tGstVideoColorimetry colorimetry;\n> >> +\n> >> +\tswitch (colorSpace.primaries) {\n> >> +\tcase ColorSpace::Primaries::Rec709:\n> >> +\t\tcolorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_BT709;\n> >> +\t\tbreak;\n> >> +\tcase ColorSpace::Primaries::Rec2020:\n> >> +\t\tcolorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_BT2020;\n> >> +\t\tbreak;\n> >> +\tcase ColorSpace::Primaries::Smpte170m:\n> >> +\t\tcolorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_SMPTE170M;\n> >> +\t\tbreak;\n> >> +\tcase ColorSpace::Primaries::Raw:\n> >> +\t\tcolorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_UNKNOWN;\n> >> +\t\tbreak;\n> >> +\t}\n> >\n> > I'd sort the cases with the order of the enum in color_space.h (below\n> > too). That's very minor.\n> \n> Ack.\n> \n> >> +\n> >> +\tswitch (colorSpace.transferFunction) {\n> >> +\tcase ColorSpace::TransferFunction::Rec709:\n> >> +\t\tcolorimetry.transfer = GST_VIDEO_TRANSFER_BT709;\n> >> +\t\tbreak;\n> >> +\tcase ColorSpace::TransferFunction::Srgb:\n> >> +\t\tcolorimetry.transfer = GST_VIDEO_TRANSFER_SRGB;\n> >> +\t\tbreak;\n> >> +\tcase ColorSpace::TransferFunction::Linear:\n> >> +\t\tcolorimetry.transfer = GST_VIDEO_TRANSFER_GAMMA10;\n> >> +\t\tbreak;\n> >> +\t}\n> >> +\n> >> +\tswitch (colorSpace.ycbcrEncoding) {\n> >> +\tcase ColorSpace::YcbcrEncoding::None:\n> >> +\t\tcolorimetry.matrix = GST_VIDEO_COLOR_MATRIX_RGB;\n> >> +\t\tbreak;\n> >> +\tcase ColorSpace::YcbcrEncoding::Rec709:\n> >> +\t\tcolorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT709;\n> >> +\t\tbreak;\n> >> +\tcase ColorSpace::YcbcrEncoding::Rec2020:\n> >> +\t\tcolorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT2020;\n> >> +\t\tbreak;\n> >> +\tcase ColorSpace::YcbcrEncoding::Rec601:\n> >> +\t\tif (colorSpace == ColorSpace::Srgb)\n> >> +\t\t\tcolorimetry.matrix = GST_VIDEO_COLOR_MATRIX_RGB;\n> >\n> > I don't think this one is right. GST_VIDEO_COLOR_MATRIX_RGB is the\n> > identity matrix, which means no encoding to YCbCr, and\n> > ColorSpace::YcbcrEncoding::Rec601 clearly defines an encoding.\n> >\n> > I think the problem is caused by our definition of ColorSpace::Srgb:\n> >\n> > const ColorSpace ColorSpace::Srgb = {\n> > \tPrimaries::Rec709,\n> > \tTransferFunction::Srgb,\n> > \tYcbcrEncoding::Rec601,\n> > \tRange::Limited\n> > };\n> >\n> > I propose modifying it to\n> >\n> > const ColorSpace ColorSpace::Srgb = {\n> > \tPrimaries::Rec709,\n> > \tTransferFunction::Srgb,\n> > \tYcbcrEncoding::None,\n> > \tRange::Full\n> > };\n> \n> ack!\n> \n> My question which I think I brought up in v1 itself now (you can choose \n> to reply anywhere) is, do we need a preset to represent YUV-encoded RGB \n> and what would we call it? ColorSpace::sYCC ?\n\nI think it would make sense, the only thing is that I don't know how\nwidely it is used to make it worth a color space preset. Regardless of\nwhether or not we create a preset, the V4L2Device class should be fixed\nif needed to perform the conversion to/from V4L2 correctly.\n\n> > and adapting the rest of libcamera accordingly.\n> >\n> > David, this will affect Raspberry Pi, so I'd appreciate your opinion.\n> \n> Patiently waiting ;-)\n> \n> >> +\t\telse\n> >> +\t\t\tcolorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT601;\n> >> +\t\tbreak;\n> >> +\t}\n> >> +\n> >> +\tswitch (colorSpace.range) {\n> >> +\tcase ColorSpace::Range::Full:\n> >> +\t\tcolorimetry.range = GST_VIDEO_COLOR_RANGE_0_255;\n> >> +\t\tbreak;\n> >> +\tcase ColorSpace::Range::Limited:\n> >> +\t\tcolorimetry.range = GST_VIDEO_COLOR_RANGE_16_235;\n> >> +\t\tbreak;\n> >> +\t}\n> >> +\n> >> +\treturn colorimetry;\n> >> +}\n> >> +\n> >> +static ColorSpace\n> >> +colorspace_from_colorimetry(const GstVideoColorimetry &colorimetry)\n> >> +{\n> >> +\tColorSpace colorspace = ColorSpace::Raw;\n> >> +\n> >> +\tswitch (colorimetry.primaries) {\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> >> +\tcase GST_VIDEO_COLOR_PRIMARIES_SMPTE170M:\n> >> +\t\tcolorspace.primaries = ColorSpace::Primaries::Smpte170m;\n> >> +\t\tbreak;\n> >> +\tcase GST_VIDEO_COLOR_PRIMARIES_UNKNOWN:\n> >> +\t\t/* Unknown primaries map to raw colorspace in gstreamer */\n> >> +\t\treturn colorspace;\n> > \n> > I would\n> >\n> > \t\treturn ColorSpace::Raw;\n> >\n> > here to make it more explicit.\n> \n> ack\n> \n> >> +\tdefault:\n> >> +\t\tg_error(\"Unsupported colorimetry primaries: %d\", colorimetry.primaries);\n> >\n> > g_error() seems very harsh, do we really need to abort() if the user\n> > asks for unsupported primaries (or other colorimetry fields below) ?\n> \n> I agree it's harsh - initially I kept as 'warning' but then it was \n> advised that 'warnings' always come with mitigation steps.\n> \n> I guess the only mitigation technique here is, making sure mappings exists!\n\nGStreamer defines more primaries than libcamera, so it seems to me that\na user could trigger a g_error() simply by specifying an unsupported\nprimary on the command line, which isn't great. Depending on what\nGStreamer expect in that case, we should either fall back to a support\nprimary, or return an error that can be handled by GStreamer to fail\ncaps negotiation instead of aborting.\n\n> > This is probably a question for Nicolas.\n> >\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_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> >> +\tcase GST_VIDEO_TRANSFER_SRGB:\n> >> +\t\tcolorspace.transferFunction = ColorSpace::TransferFunction::Srgb;\n> >> +\t\tbreak;\n> >> +\tdefault:\n> >> +\t\tg_error(\"Unsupported colorimetry transfer function: %d\", colorimetry.transfer);\n> >> +\t}\n> >> +\n> >> +\tswitch (colorimetry.matrix) {\n> >> +\t/* FCC is about the same as BT601 with less digit */\n> >> +\tcase GST_VIDEO_COLOR_MATRIX_FCC:\n> >> +\tcase GST_VIDEO_COLOR_MATRIX_BT601:\n> >> +\t/* v4l2_ycbcr_encoding of sRGB is ENC_601 in the kernel */\n> >\n> > That's right, but I don't think we need to be concerned by that here.\n> > GST_VIDEO_COLOR_MATRIX_RGB should be mapped to\n> > ColorSpace::YcbcrEncoding::None, and we should handle the V4L2 sRGB YUV\n> > encoding internally in libcamera.\n> \n> replied to the latter part, in detail, in v1\n> \n> >> +\tcase GST_VIDEO_COLOR_MATRIX_RGB:\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\tg_error(\"Unsupported colorimetry matrix: %d\", colorimetry.matrix);\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\tg_error(\"Unsupported colorimetry range %d\", colorimetry.range);\n> >> +\t}\n> >> +\n> >> +\treturn colorspace;\n> >> +}\n> >> +\n> >> static GstVideoFormat\n> >> pixel_format_to_gst_format(const PixelFormat &format)\n> >> {\n> >> @@ -139,6 +279,18 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg\n> >> \t\t\t \"width\", G_TYPE_INT, stream_cfg.size.width,\n> >> \t\t\t \"height\", G_TYPE_INT, stream_cfg.size.height,\n> >> \t\t\t nullptr);\n> >> +\n> >> +\tif (stream_cfg.colorSpace) {\n> >> +\t\tGstVideoColorimetry colorimetry = colorimetry_from_colorspace(stream_cfg.colorSpace.value());\n> >> +\t\tgchar *colorimetry_str = gst_video_colorimetry_to_string(&colorimetry);\n> >> +\n> >> +\t\tif (colorimetry_str)\n> >> +\t\t\tgst_structure_set(s, \"colorimetry\", G_TYPE_STRING, colorimetry_str, nullptr);\n> >> +\t\telse\n> >> +\t\t\tg_error(\"Got invalid colorimetry from ColorSpace: %s\",\n> >> +\t\t\t\tColorSpace::toString(stream_cfg.colorSpace).c_str());\n> >\n> > Same question as above about g_error(). As far as I understand,\n> > gst_video_colorimetry_to_string() will return null either if it can't\n> > alocate memory for the string (in which case we're screwed anyway, so\n> > g_error() is fine) or if colorimetry is all UNKNOWN. The latter should\n> > never happen given the implementation of colorimetry_from_colorspace(),\n> > so I suppose it's fine ?\n> \n> Yes, I think it's fine\n> \n> >> +\t}\n> >> +\n> >> \tgst_caps_append_structure(caps, s);\n> >> \n> >> \treturn caps;\n> >> @@ -222,6 +374,18 @@ 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/* Configure colorimetry */\n> >> +\tif (gst_structure_has_field(s, \"colorimetry\")) {\n> >> +\t\tconst gchar *colorimetry_caps = gst_structure_get_string(s, \"colorimetry\");\n> >> +\t\tGstVideoColorimetry colorimetry;\n> >> +\n> >> +\t\tif(gst_video_colorimetry_from_string(&colorimetry, colorimetry_caps)) {\n> > \n> > Missing space after \"if\".\n> >\n> >> +\t\t\tstream_cfg.colorSpace = colorspace_from_colorimetry(colorimetry);\n> >> +\t\t} else {\n> >> +\t\t\tg_critical(\"Invalid colorimetry %s\", colorimetry_caps);\n> >> +\t\t}\n> >> +\t}\n> >> }\n> >> \n> >> #if !GST_CHECK_VERSION(1, 17, 1)","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 65FC6BE173\n\tfor ;\n\tMon, 1 Aug 2022 12:42:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D22CC6330F;\n\tMon, 1 Aug 2022 14:42:11 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D7D0E603E8\n\tfor ;\n\tMon, 1 Aug 2022 14:42:09 +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 2910A2F3;\n\tMon, 1 Aug 2022 14:42:09 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659357731;\n\tbh=T2q3G3B/U4SyttVhoak7EJ6rF/JP4huYRUTlkl73bTA=;\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=jHyvTMUZbF3jooOAQj+V7sM2XAgeYe2A3ulQCdoHE+YkEjFSp5J3iO+0PhCpBbsle\n\tkK7gSaFIeT90pwOWbO/7IfDfMgs1CAdtTifNlPIVyWBHxBd2Slvj1CcdrhOrW2VMTn\n\tKUvF2ENqmEsjPj1mzIbquo7qSgGwtXGaF1ZddGm5fo90nCleIHR5L4KViijD3g5NlI\n\tdZRZe5dMZx9IqHN5lpe2/NJLZ6A9eM0Q4SIaEYGUBe9jfGkwFfJMkj/+uV8dNCzfVI\n\teXmW1ADhfvRk7J/nPKByQp2MdDTUhCQDlOaebWurksyHEOInYfv0xW/yGrVpCYUudO\n\tDZWDI5sazlJlQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1659357729;\n\tbh=T2q3G3B/U4SyttVhoak7EJ6rF/JP4huYRUTlkl73bTA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GIuqZMOmsqKk706ueWChpzMoTZa+aJh+hp1sIg78y9vAdjeq4dWnbx9syi4zjIeYJ\n\tlKO37Dz9JL/JA+ZzvXCdnixdrrMO2io82So/cifxNzSHQ/ay+KQ/kTMT5jwoIqswM/\n\tm+fLRqdbYiIuOqpEv41h+XmjcxIBZZF/9kZDqkkc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"GIuqZMOm\"; dkim-atps=neutral","Date":"Mon, 1 Aug 2022 15:42:05 +0300","To":"Umang Jain ","Message-ID":"","References":"<20220729103846.414819-1-umang.jain@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Provide colorimetry <>\n\tColorSpace mappings","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":"rishikeshdonadkar@gmail.com, libcamera-devel@lists.libcamera.org,\n\tvedantparanjape160201@gmail.com, nicolas.dufresne@collabora.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24265,"web_url":"https://patchwork.libcamera.org/comment/24265/","msgid":"","date":"2022-08-01T13:20:12","subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Provide colorimetry <>\n\tColorSpace mappings","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi everyone\n\nOn Mon, 1 Aug 2022 at 13:31, Umang Jain wrote:\n>\n> Hi Laurent,\n>\n> On 8/1/22 01:14, Laurent Pinchart wrote:\n> > Hi Umang and Rishikesh,\n> >\n> > (Cc'ing David)\n> >\n> > Thank you for the patch.\n> >\n> > On Fri, Jul 29, 2022 at 04:08:46PM +0530, Umang Jain via libcamera-devel wrote:\n> >> From: Rishikesh Donadkar \n> >>\n> >> Provide colorimetry <=> libcamera::ColorSpace mappings via:\n> >> - GstVideoColorimetry colorimetry_from_colorspace(colorspace);\n> >> - ColorSpace colorspace_from_colorimetry(colorimetry);\n> >>\n> >> Read the colorimetry field from caps into the stream configuration.\n> >> After stream validation, the sensor supported colorimetry will\n> >> be retrieved and the caps will be updated accordingly.\n> >>\n> >> Colorimetry support for gstlibcamerasrc currently undertakes only one\n> > s/gstlibcamerasrc/libcamerasrc/ (or gstlibcamera if you meant the\n> > library, not the element name)\n> >\n> >> argument. Multiple colorimetry support shall be introduced in\n> >> subsequent commits.\n> > I'm probably missing something here, what do you mean by multiple\n> > colorimetry support ?\n> >\n> >> Signed-off-by: Rishikesh Donadkar \n> >> Signed-off-by: Umang Jain \n> >> ---\n> >>\n> >> Changes in v2:\n> >> - Drop \"Default\" Colorspace\n> >> - Improve function signature of colorimetry_from_colorspace() and\n> >> colorspace_from_colorimetry()\n> >> - Map GST_VIDEO_COLOR_MATRIX_RGB to Colorspace::YcbcrEncoding::Rec601\n> >> (comes from the kernel)\n> >> - Map GST_VIDEO_COLOR_PRIMARIES_UNKNOWN to Raw colorspace\n> >> - Map ColorSpace::YcbcrEncoding::Rec601 to GST_VIDEO_COLOR_MATRIX_RGB\n> >> if colorspace is \"sRGB\". GST_VIDEO_COLOR_MATRIX_BT601 for all other\n> >> cases\n> >> - Minor nits regarding error reporting strings.\n> >> ---\n> >> src/gstreamer/gstlibcamera-utils.cpp | 164 +++++++++++++++++++++++++++\n> >> 1 file changed, 164 insertions(+)\n> >>\n> >> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> >> index c97c0d43..e4ff1269 100644\n> >> --- a/src/gstreamer/gstlibcamera-utils.cpp\n> >> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> >> @@ -45,6 +45,146 @@ static struct {\n> >> /* \\todo NV42 is used in libcamera but is not mapped in GStreamer yet. */\n> >> };\n> >>\n> >> +static GstVideoColorimetry\n> >> +colorimetry_from_colorspace(const ColorSpace &colorSpace)\n> >> +{\n> >> + GstVideoColorimetry colorimetry;\n> >> +\n> >> + switch (colorSpace.primaries) {\n> >> + case ColorSpace::Primaries::Rec709:\n> >> + colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_BT709;\n> >> + break;\n> >> + case ColorSpace::Primaries::Rec2020:\n> >> + colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_BT2020;\n> >> + break;\n> >> + case ColorSpace::Primaries::Smpte170m:\n> >> + colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_SMPTE170M;\n> >> + break;\n> >> + case ColorSpace::Primaries::Raw:\n> >> + colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_UNKNOWN;\n> >> + break;\n> >> + }\n> > I'd sort the cases with the order of the enum in color_space.h (below\n> > too). That's very minor.\n>\n>\n> Ack.\n>\n> >\n> >> +\n> >> + switch (colorSpace.transferFunction) {\n> >> + case ColorSpace::TransferFunction::Rec709:\n> >> + colorimetry.transfer = GST_VIDEO_TRANSFER_BT709;\n> >> + break;\n> >> + case ColorSpace::TransferFunction::Srgb:\n> >> + colorimetry.transfer = GST_VIDEO_TRANSFER_SRGB;\n> >> + break;\n> >> + case ColorSpace::TransferFunction::Linear:\n> >> + colorimetry.transfer = GST_VIDEO_TRANSFER_GAMMA10;\n> >> + break;\n> >> + }\n> >> +\n> >> + switch (colorSpace.ycbcrEncoding) {\n> >> + case ColorSpace::YcbcrEncoding::None:\n> >> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_RGB;\n> >> + break;\n> >> + case ColorSpace::YcbcrEncoding::Rec709:\n> >> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT709;\n> >> + break;\n> >> + case ColorSpace::YcbcrEncoding::Rec2020:\n> >> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT2020;\n> >> + break;\n> >> + case ColorSpace::YcbcrEncoding::Rec601:\n> >> + if (colorSpace == ColorSpace::Srgb)\n> >> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_RGB;\n> > I don't think this one is right. GST_VIDEO_COLOR_MATRIX_RGB is the\n> > identity matrix, which means no encoding to YCbCr, and\n> > ColorSpace::YcbcrEncoding::Rec601 clearly defines an encoding.\n> >\n> > I think the problem is caused by our definition of ColorSpace::Srgb:\n> >\n> > const ColorSpace ColorSpace::Srgb = {\n> > Primaries::Rec709,\n> > TransferFunction::Srgb,\n> > YcbcrEncoding::Rec601,\n> > Range::Limited\n> > };\n> >\n> > I propose modifying it to\n> >\n> > const ColorSpace ColorSpace::Srgb = {\n> > Primaries::Rec709,\n> > TransferFunction::Srgb,\n> > YcbcrEncoding::None,\n> > Range::Full\n> > };\n>\n>\n> ack!\n>\n> My question which I think I brought up in v1 itself now (you can choose\n> to reply anywhere) is, do we need a preset to represent YUV-encoded RGB\n> and what would we call it? ColorSpace::sYCC ?\n>\n> >\n> > and adapting the rest of libcamera accordingly.\n> >\n> > David, this will affect Raspberry Pi, so I'd appreciate your opinion.\n>\n>\n> Patiently waiting ;-)\n\nSorry, the thread is getting a bit long so it takes me a while to\nnotice things!!\n\nSo the original sRGB colour space definition comes from:\n\nhttps://www.kernel.org/doc/html/latest/userspace-api/media/v4l/colorspaces-details.html#col-srgb\n\nI agree that the values aren't necessarily the ones you first think\nof, there is some history behind some of them.\n\nI don't think it matters for the Pi, though. I believe the only colour\nspaces we ask for are Jpeg, Smpte170m and Rec709. (When we really want\nsRGB for still capture we ask for Jpeg.)\n\nDavid\n\n>\n> >\n> >> + else\n> >> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT601;\n> >> + break;\n> >> + }\n> >> +\n> >> + switch (colorSpace.range) {\n> >> + case ColorSpace::Range::Full:\n> >> + colorimetry.range = GST_VIDEO_COLOR_RANGE_0_255;\n> >> + break;\n> >> + case ColorSpace::Range::Limited:\n> >> + colorimetry.range = GST_VIDEO_COLOR_RANGE_16_235;\n> >> + break;\n> >> + }\n> >> +\n> >> + return colorimetry;\n> >> +}\n> >> +\n> >> +static ColorSpace\n> >> +colorspace_from_colorimetry(const GstVideoColorimetry &colorimetry)\n> >> +{\n> >> + ColorSpace colorspace = ColorSpace::Raw;\n> >> +\n> >> + switch (colorimetry.primaries) {\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> >> + case GST_VIDEO_COLOR_PRIMARIES_SMPTE170M:\n> >> + colorspace.primaries = ColorSpace::Primaries::Smpte170m;\n> >> + break;\n> >> + case GST_VIDEO_COLOR_PRIMARIES_UNKNOWN:\n> >> + /* Unknown primaries map to raw colorspace in gstreamer */\n> >> + return colorspace;\n> > I would\n> >\n> > return ColorSpace::Raw;\n> >\n> > here to make it more explicit.\n>\n>\n> ack\n>\n> >\n> >> + default:\n> >> + g_error(\"Unsupported colorimetry primaries: %d\", colorimetry.primaries);\n> > g_error() seems very harsh, do we really need to abort() if the user\n> > asks for unsupported primaries (or other colorimetry fields below) ?\n>\n>\n> I agree it's harsh - initially I kept as 'warning' but then it was\n> advised that 'warnings' always come with mitigation steps.\n>\n> I guess the only mitigation technique here is, making sure mappings exists!\n>\n> > This is probably a question for Nicolas.\n> >\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 supported\");\n> >> + /* fallthrough */\n> >> + case GST_VIDEO_TRANSFER_GAMMA10:\n> >> + colorspace.transferFunction = ColorSpace::TransferFunction::Linear;\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 = ColorSpace::TransferFunction::Rec709;\n> >> + break;\n> >> + case GST_VIDEO_TRANSFER_SRGB:\n> >> + colorspace.transferFunction = ColorSpace::TransferFunction::Srgb;\n> >> + break;\n> >> + default:\n> >> + g_error(\"Unsupported colorimetry transfer function: %d\", colorimetry.transfer);\n> >> + }\n> >> +\n> >> + switch (colorimetry.matrix) {\n> >> + /* FCC is about the same as BT601 with less digit */\n> >> + case GST_VIDEO_COLOR_MATRIX_FCC:\n> >> + case GST_VIDEO_COLOR_MATRIX_BT601:\n> >> + /* v4l2_ycbcr_encoding of sRGB is ENC_601 in the kernel */\n> > That's right, but I don't think we need to be concerned by that here.\n> > GST_VIDEO_COLOR_MATRIX_RGB should be mapped to\n> > ColorSpace::YcbcrEncoding::None, and we should handle the V4L2 sRGB YUV\n> > encoding internally in libcamera.\n>\n>\n> replied to the latter part, in detail, in v1\n>\n> >\n> >> + case GST_VIDEO_COLOR_MATRIX_RGB:\n> >> + colorspace.ycbcrEncoding = ColorSpace::YcbcrEncoding::Rec601;\n> >> + break;\n> >> + case GST_VIDEO_COLOR_MATRIX_BT709:\n> >> + colorspace.ycbcrEncoding = ColorSpace::YcbcrEncoding::Rec709;\n> >> + break;\n> >> + case GST_VIDEO_COLOR_MATRIX_BT2020:\n> >> + colorspace.ycbcrEncoding = ColorSpace::YcbcrEncoding::Rec2020;\n> >> + break;\n> >> + default:\n> >> + g_error(\"Unsupported colorimetry matrix: %d\", colorimetry.matrix);\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> >> + g_error(\"Unsupported colorimetry range %d\", colorimetry.range);\n> >> + }\n> >> +\n> >> + return colorspace;\n> >> +}\n> >> +\n> >> static GstVideoFormat\n> >> pixel_format_to_gst_format(const PixelFormat &format)\n> >> {\n> >> @@ -139,6 +279,18 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg\n> >> \"width\", G_TYPE_INT, stream_cfg.size.width,\n> >> \"height\", G_TYPE_INT, stream_cfg.size.height,\n> >> nullptr);\n> >> +\n> >> + if (stream_cfg.colorSpace) {\n> >> + GstVideoColorimetry colorimetry = colorimetry_from_colorspace(stream_cfg.colorSpace.value());\n> >> + gchar *colorimetry_str = gst_video_colorimetry_to_string(&colorimetry);\n> >> +\n> >> + if (colorimetry_str)\n> >> + gst_structure_set(s, \"colorimetry\", G_TYPE_STRING, colorimetry_str, nullptr);\n> >> + else\n> >> + g_error(\"Got invalid colorimetry from ColorSpace: %s\",\n> >> + ColorSpace::toString(stream_cfg.colorSpace).c_str());\n> > Same question as above about g_error(). As far as I understand,\n> > gst_video_colorimetry_to_string() will return null either if it can't\n> > alocate memory for the string (in which case we're screwed anyway, so\n> > g_error() is fine) or if colorimetry is all UNKNOWN. The latter should\n> > never happen given the implementation of colorimetry_from_colorspace(),\n> > so I suppose it's fine ?\n>\n>\n> Yes, I think it's fine\n>\n> >\n> >> + }\n> >> +\n> >> gst_caps_append_structure(caps, s);\n> >>\n> >> return caps;\n> >> @@ -222,6 +374,18 @@ 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> >> + /* Configure colorimetry */\n> >> + if (gst_structure_has_field(s, \"colorimetry\")) {\n> >> + const gchar *colorimetry_caps = gst_structure_get_string(s, \"colorimetry\");\n> >> + GstVideoColorimetry colorimetry;\n> >> +\n> >> + if(gst_video_colorimetry_from_string(&colorimetry, colorimetry_caps)) {\n> > Missing space after \"if\".\n> >\n> >> + stream_cfg.colorSpace = colorspace_from_colorimetry(colorimetry);\n> >> + } else {\n> >> + g_critical(\"Invalid colorimetry %s\", colorimetry_caps);\n> >> + }\n> >> + }\n> >> }\n> >>\n> >> #if !GST_CHECK_VERSION(1, 17, 1)","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 3731DC3275\n\tfor ;\n\tMon, 1 Aug 2022 13:20:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 890EC6330F;\n\tMon, 1 Aug 2022 15:20:25 +0200 (CEST)","from mail-ed1-x536.google.com (mail-ed1-x536.google.com\n\t[IPv6:2a00:1450:4864:20::536])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 06BDB603E8\n\tfor ;\n\tMon, 1 Aug 2022 15:20:24 +0200 (CEST)","by mail-ed1-x536.google.com with SMTP id z18so13675704edb.10\n\tfor ;\n\tMon, 01 Aug 2022 06:20:23 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659360025;\n\tbh=V8R3DGKQ1PwkJ/sC9XwFLu+eKJ5HzZuYmxN120F6CHo=;\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=YsI5tKLEsg7MdZThYBq5it1cd8CHrQBCDyausxZZ792GLfepCVcAU96x1XRA+BWjN\n\txg+1VHQ97cAWvzl5Rt6YbGI85t4QhSPb/Bgy2ATY1utQhIWIGxmGLGIisrcJbGx/Xy\n\t8nuxwKcKRlk2W2mEJyXCE5gK0/9bfYwgToaTrmbPQ44MWeI7sbYKDbB+9e/rPvD9Sf\n\tI8pl5Fl66XULKiSKNky1ZaBbLl+hK0og1+SW+rjBt/JOMujoGTUOk4tpiBrvX12XPC\n\tzmWe6n8YMVb35hiccxe8JQ/lkADy34nHA2S8CJ6Pgt713oR08JyWB6O1fNh6H1YYiB\n\tCkeGeQgd9hk3Q==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=ovZ+bOdh3r2a3Ic04bcKQdpD1wc1EPJSiMKC5Ej+H14=;\n\tb=ThEwP6WvkaP1yioDEwY5ZArOjdhW2wckQQZd59AOmLQ1LIMFRpoysMebTefX9G+Q4W\n\tKp3oeoDhFmnT5YShoFeXoK9nBWkC/+JT4Tn939f76rBBpiBRK8H7vQLgZN36/lNS7ydB\n\t1v66MIgwtBYCnIVfNYQMWqT6VQ8onM3UG0NObUE70B446AD0V9D5rEwUlpe/d1lWjqnu\n\tiyXORJHBThlj9GLs2clg6XnKhDnI3JQULQFfX3x7OIxBObyfkgULcrj/3ctu5JGeOy7D\n\t+nE460PTxUVmEuTLTfb2slme0n4Cofv7+MwdMvJKUkUgeEZ1YzeieJpN+tRBuf7rW/vH\n\t6LEQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"ThEwP6Wv\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=ovZ+bOdh3r2a3Ic04bcKQdpD1wc1EPJSiMKC5Ej+H14=;\n\tb=KzvnRlsskLau33mQQRfltp74d5q7pWyPzNZaOdgNPxZmnPQ3Xj89D22MfbX5IxICbp\n\trtU9cWVpkFZh2240Pm/5hrnIr/cw9eSGgrxpXd2EOc67Dg2RG/XnbkzmEkHMhTVUDuCM\n\tEY2htAc4Bt1PW7rbEpLt1Nv15hg11zE5BGTHYIbM8mtdGlOHTe8KFOGTWxny0moWTaNW\n\tfxFT55QzX7ry1rmq9xKoRgtACqom8DiYkQNy+xcri+03oQ6EogESlGmuhBavY9JRzabF\n\tguKFIh+55Lf6+/lJLWunnt2YXjpYYnXO88lZns3WWgt5MOIOMDKzSdgT1jUi1ukP9sCI\n\tF8Sg==","X-Gm-Message-State":"AJIora9mg+cjzO1Q7HtLPHbUgZEleH2dBJM28enlcLv682hwh8EM7hr/\n\tmSC9mHzvKZ9NA7w3MIvJKDj7qk9qsXyHBfNdI8bbhw==","X-Google-Smtp-Source":"AGRyM1vPK9eOmHvnF0ThiOH+L9aL2kSmJOKGgvtAtP6+LjSp4/KPHrg/PfYdkwBHPPg1SvUQaCGuVtfGGffurwEp+9s=","X-Received":"by 2002:a05:6402:35d4:b0:43a:df89:94e2 with SMTP id\n\tz20-20020a05640235d400b0043adf8994e2mr15932032edc.149.1659360023528;\n\tMon, 01 Aug 2022 06:20:23 -0700 (PDT)","MIME-Version":"1.0","References":"<20220729103846.414819-1-umang.jain@ideasonboard.com>\n\t\n\t","In-Reply-To":"","Date":"Mon, 1 Aug 2022 14:20:12 +0100","Message-ID":"","To":"Umang Jain ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Provide colorimetry <>\n\tColorSpace mappings","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":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"rishikeshdonadkar@gmail.com,\n\tlibcamera devel ,\n\tvedantparanjape160201@gmail.com, nicolas.dufresne@collabora.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24266,"web_url":"https://patchwork.libcamera.org/comment/24266/","msgid":"","date":"2022-08-01T13:30:34","subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Provide colorimetry <>\n\tColorSpace mappings","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nOn Mon, Aug 01, 2022 at 02:20:12PM +0100, David Plowman wrote:\n> On Mon, 1 Aug 2022 at 13:31, Umang Jain wrote:\n> > On 8/1/22 01:14, Laurent Pinchart wrote:\n> > > On Fri, Jul 29, 2022 at 04:08:46PM +0530, Umang Jain via libcamera-devel wrote:\n> > >> From: Rishikesh Donadkar \n> > >>\n> > >> Provide colorimetry <=> libcamera::ColorSpace mappings via:\n> > >> - GstVideoColorimetry colorimetry_from_colorspace(colorspace);\n> > >> - ColorSpace colorspace_from_colorimetry(colorimetry);\n> > >>\n> > >> Read the colorimetry field from caps into the stream configuration.\n> > >> After stream validation, the sensor supported colorimetry will\n> > >> be retrieved and the caps will be updated accordingly.\n> > >>\n> > >> Colorimetry support for gstlibcamerasrc currently undertakes only one\n> > >\n> > > s/gstlibcamerasrc/libcamerasrc/ (or gstlibcamera if you meant the\n> > > library, not the element name)\n> > >\n> > >> argument. Multiple colorimetry support shall be introduced in\n> > >> subsequent commits.\n> > >\n> > > I'm probably missing something here, what do you mean by multiple\n> > > colorimetry support ?\n> > >\n> > >> Signed-off-by: Rishikesh Donadkar \n> > >> Signed-off-by: Umang Jain \n> > >> ---\n> > >>\n> > >> Changes in v2:\n> > >> - Drop \"Default\" Colorspace\n> > >> - Improve function signature of colorimetry_from_colorspace() and\n> > >> colorspace_from_colorimetry()\n> > >> - Map GST_VIDEO_COLOR_MATRIX_RGB to Colorspace::YcbcrEncoding::Rec601\n> > >> (comes from the kernel)\n> > >> - Map GST_VIDEO_COLOR_PRIMARIES_UNKNOWN to Raw colorspace\n> > >> - Map ColorSpace::YcbcrEncoding::Rec601 to GST_VIDEO_COLOR_MATRIX_RGB\n> > >> if colorspace is \"sRGB\". GST_VIDEO_COLOR_MATRIX_BT601 for all other\n> > >> cases\n> > >> - Minor nits regarding error reporting strings.\n> > >> ---\n> > >> src/gstreamer/gstlibcamera-utils.cpp | 164 +++++++++++++++++++++++++++\n> > >> 1 file changed, 164 insertions(+)\n> > >>\n> > >> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> > >> index c97c0d43..e4ff1269 100644\n> > >> --- a/src/gstreamer/gstlibcamera-utils.cpp\n> > >> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> > >> @@ -45,6 +45,146 @@ static struct {\n> > >> /* \\todo NV42 is used in libcamera but is not mapped in GStreamer yet. */\n> > >> };\n> > >>\n> > >> +static GstVideoColorimetry\n> > >> +colorimetry_from_colorspace(const ColorSpace &colorSpace)\n> > >> +{\n> > >> + GstVideoColorimetry colorimetry;\n> > >> +\n> > >> + switch (colorSpace.primaries) {\n> > >> + case ColorSpace::Primaries::Rec709:\n> > >> + colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_BT709;\n> > >> + break;\n> > >> + case ColorSpace::Primaries::Rec2020:\n> > >> + colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_BT2020;\n> > >> + break;\n> > >> + case ColorSpace::Primaries::Smpte170m:\n> > >> + colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_SMPTE170M;\n> > >> + break;\n> > >> + case ColorSpace::Primaries::Raw:\n> > >> + colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_UNKNOWN;\n> > >> + break;\n> > >> + }\n> > >\n> > > I'd sort the cases with the order of the enum in color_space.h (below\n> > > too). That's very minor.\n> >\n> > Ack.\n> >\n> > >> +\n> > >> + switch (colorSpace.transferFunction) {\n> > >> + case ColorSpace::TransferFunction::Rec709:\n> > >> + colorimetry.transfer = GST_VIDEO_TRANSFER_BT709;\n> > >> + break;\n> > >> + case ColorSpace::TransferFunction::Srgb:\n> > >> + colorimetry.transfer = GST_VIDEO_TRANSFER_SRGB;\n> > >> + break;\n> > >> + case ColorSpace::TransferFunction::Linear:\n> > >> + colorimetry.transfer = GST_VIDEO_TRANSFER_GAMMA10;\n> > >> + break;\n> > >> + }\n> > >> +\n> > >> + switch (colorSpace.ycbcrEncoding) {\n> > >> + case ColorSpace::YcbcrEncoding::None:\n> > >> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_RGB;\n> > >> + break;\n> > >> + case ColorSpace::YcbcrEncoding::Rec709:\n> > >> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT709;\n> > >> + break;\n> > >> + case ColorSpace::YcbcrEncoding::Rec2020:\n> > >> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT2020;\n> > >> + break;\n> > >> + case ColorSpace::YcbcrEncoding::Rec601:\n> > >> + if (colorSpace == ColorSpace::Srgb)\n> > >> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_RGB;\n> > > \n> > > I don't think this one is right. GST_VIDEO_COLOR_MATRIX_RGB is the\n> > > identity matrix, which means no encoding to YCbCr, and\n> > > ColorSpace::YcbcrEncoding::Rec601 clearly defines an encoding.\n> > >\n> > > I think the problem is caused by our definition of ColorSpace::Srgb:\n> > >\n> > > const ColorSpace ColorSpace::Srgb = {\n> > > Primaries::Rec709,\n> > > TransferFunction::Srgb,\n> > > YcbcrEncoding::Rec601,\n> > > Range::Limited\n> > > };\n> > >\n> > > I propose modifying it to\n> > >\n> > > const ColorSpace ColorSpace::Srgb = {\n> > > Primaries::Rec709,\n> > > TransferFunction::Srgb,\n> > > YcbcrEncoding::None,\n> > > Range::Full\n> > > };\n> >\n> > ack!\n> >\n> > My question which I think I brought up in v1 itself now (you can choose\n> > to reply anywhere) is, do we need a preset to represent YUV-encoded RGB\n> > and what would we call it? ColorSpace::sYCC ?\n> >\n> > > and adapting the rest of libcamera accordingly.\n> > >\n> > > David, this will affect Raspberry Pi, so I'd appreciate your opinion.\n> >\n> > Patiently waiting ;-)\n> \n> Sorry, the thread is getting a bit long so it takes me a while to\n> notice things!!\n> \n> So the original sRGB colour space definition comes from:\n> \n> https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/colorspaces-details.html#col-srgb\n> \n> I agree that the values aren't necessarily the ones you first think\n> of, there is some history behind some of them.\n> \n> I don't think it matters for the Pi, though. I believe the only colour\n> spaces we ask for are Jpeg, Smpte170m and Rec709. (When we really want\n> sRGB for still capture we ask for Jpeg.)\n\nThank you for the information. Unless there's any objection, I then\npropose modifying Srgb to define it as\n\nconst ColorSpace ColorSpace::Srgb = {\n\tPrimaries::Rec709,\n\tTransferFunction::Srgb,\n\tYcbcrEncoding::None,\n\tRange::Full\n};\n\nWe could also define\n\nconst ColorSpace ColorSpace::Sycc = {\n\tPrimaries::Rec709,\n\tTransferFunction::Srgb,\n\tYcbcrEncoding::Rec601,\n\tRange::Limited\n};\n\nbut I'm tempted to hold off until it becomes needed.\n\n> > >> + else\n> > >> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT601;\n> > >> + break;\n> > >> + }\n> > >> +\n> > >> + switch (colorSpace.range) {\n> > >> + case ColorSpace::Range::Full:\n> > >> + colorimetry.range = GST_VIDEO_COLOR_RANGE_0_255;\n> > >> + break;\n> > >> + case ColorSpace::Range::Limited:\n> > >> + colorimetry.range = GST_VIDEO_COLOR_RANGE_16_235;\n> > >> + break;\n> > >> + }\n> > >> +\n> > >> + return colorimetry;\n> > >> +}\n> > >> +\n> > >> +static ColorSpace\n> > >> +colorspace_from_colorimetry(const GstVideoColorimetry &colorimetry)\n> > >> +{\n> > >> + ColorSpace colorspace = ColorSpace::Raw;\n> > >> +\n> > >> + switch (colorimetry.primaries) {\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> > >> + case GST_VIDEO_COLOR_PRIMARIES_SMPTE170M:\n> > >> + colorspace.primaries = ColorSpace::Primaries::Smpte170m;\n> > >> + break;\n> > >> + case GST_VIDEO_COLOR_PRIMARIES_UNKNOWN:\n> > >> + /* Unknown primaries map to raw colorspace in gstreamer */\n> > >> + return colorspace;\n> > >\n> > > I would\n> > >\n> > > return ColorSpace::Raw;\n> > >\n> > > here to make it more explicit.\n> >\n> > ack\n> >\n> > >> + default:\n> > >> + g_error(\"Unsupported colorimetry primaries: %d\", colorimetry.primaries);\n> > >\n> > > g_error() seems very harsh, do we really need to abort() if the user\n> > > asks for unsupported primaries (or other colorimetry fields below) ?\n> >\n> > I agree it's harsh - initially I kept as 'warning' but then it was\n> > advised that 'warnings' always come with mitigation steps.\n> >\n> > I guess the only mitigation technique here is, making sure mappings exists!\n> >\n> > > This is probably a question for Nicolas.\n> > >\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 supported\");\n> > >> + /* fallthrough */\n> > >> + case GST_VIDEO_TRANSFER_GAMMA10:\n> > >> + colorspace.transferFunction = ColorSpace::TransferFunction::Linear;\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 = ColorSpace::TransferFunction::Rec709;\n> > >> + break;\n> > >> + case GST_VIDEO_TRANSFER_SRGB:\n> > >> + colorspace.transferFunction = ColorSpace::TransferFunction::Srgb;\n> > >> + break;\n> > >> + default:\n> > >> + g_error(\"Unsupported colorimetry transfer function: %d\", colorimetry.transfer);\n> > >> + }\n> > >> +\n> > >> + switch (colorimetry.matrix) {\n> > >> + /* FCC is about the same as BT601 with less digit */\n> > >> + case GST_VIDEO_COLOR_MATRIX_FCC:\n> > >> + case GST_VIDEO_COLOR_MATRIX_BT601:\n> > >> + /* v4l2_ycbcr_encoding of sRGB is ENC_601 in the kernel */\n> > >\n> > > That's right, but I don't think we need to be concerned by that here.\n> > > GST_VIDEO_COLOR_MATRIX_RGB should be mapped to\n> > > ColorSpace::YcbcrEncoding::None, and we should handle the V4L2 sRGB YUV\n> > > encoding internally in libcamera.\n> >\n> > replied to the latter part, in detail, in v1\n> >\n> > >\n> > >> + case GST_VIDEO_COLOR_MATRIX_RGB:\n> > >> + colorspace.ycbcrEncoding = ColorSpace::YcbcrEncoding::Rec601;\n> > >> + break;\n> > >> + case GST_VIDEO_COLOR_MATRIX_BT709:\n> > >> + colorspace.ycbcrEncoding = ColorSpace::YcbcrEncoding::Rec709;\n> > >> + break;\n> > >> + case GST_VIDEO_COLOR_MATRIX_BT2020:\n> > >> + colorspace.ycbcrEncoding = ColorSpace::YcbcrEncoding::Rec2020;\n> > >> + break;\n> > >> + default:\n> > >> + g_error(\"Unsupported colorimetry matrix: %d\", colorimetry.matrix);\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> > >> + g_error(\"Unsupported colorimetry range %d\", colorimetry.range);\n> > >> + }\n> > >> +\n> > >> + return colorspace;\n> > >> +}\n> > >> +\n> > >> static GstVideoFormat\n> > >> pixel_format_to_gst_format(const PixelFormat &format)\n> > >> {\n> > >> @@ -139,6 +279,18 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg\n> > >> \"width\", G_TYPE_INT, stream_cfg.size.width,\n> > >> \"height\", G_TYPE_INT, stream_cfg.size.height,\n> > >> nullptr);\n> > >> +\n> > >> + if (stream_cfg.colorSpace) {\n> > >> + GstVideoColorimetry colorimetry = colorimetry_from_colorspace(stream_cfg.colorSpace.value());\n> > >> + gchar *colorimetry_str = gst_video_colorimetry_to_string(&colorimetry);\n> > >> +\n> > >> + if (colorimetry_str)\n> > >> + gst_structure_set(s, \"colorimetry\", G_TYPE_STRING, colorimetry_str, nullptr);\n> > >> + else\n> > >> + g_error(\"Got invalid colorimetry from ColorSpace: %s\",\n> > >> + ColorSpace::toString(stream_cfg.colorSpace).c_str());\n> > >\n> > > Same question as above about g_error(). As far as I understand,\n> > > gst_video_colorimetry_to_string() will return null either if it can't\n> > > alocate memory for the string (in which case we're screwed anyway, so\n> > > g_error() is fine) or if colorimetry is all UNKNOWN. The latter should\n> > > never happen given the implementation of colorimetry_from_colorspace(),\n> > > so I suppose it's fine ?\n> >\n> > Yes, I think it's fine\n> >\n> > >> + }\n> > >> +\n> > >> gst_caps_append_structure(caps, s);\n> > >>\n> > >> return caps;\n> > >> @@ -222,6 +374,18 @@ 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> > >> + /* Configure colorimetry */\n> > >> + if (gst_structure_has_field(s, \"colorimetry\")) {\n> > >> + const gchar *colorimetry_caps = gst_structure_get_string(s, \"colorimetry\");\n> > >> + GstVideoColorimetry colorimetry;\n> > >> +\n> > >> + if(gst_video_colorimetry_from_string(&colorimetry, colorimetry_caps)) {\n> > >\n> > > Missing space after \"if\".\n> > >\n> > >> + stream_cfg.colorSpace = colorspace_from_colorimetry(colorimetry);\n> > >> + } else {\n> > >> + g_critical(\"Invalid colorimetry %s\", colorimetry_caps);\n> > >> + }\n> > >> + }\n> > >> }\n> > >>\n> > >> #if !GST_CHECK_VERSION(1, 17, 1)","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 6141EBE173\n\tfor ;\n\tMon, 1 Aug 2022 13:30:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C1BFC63312;\n\tMon, 1 Aug 2022 15:30:40 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 81703603E8\n\tfor ;\n\tMon, 1 Aug 2022 15:30:39 +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 B9C162F3;\n\tMon, 1 Aug 2022 15:30:38 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659360640;\n\tbh=xSgSdAeiI7tr54OPrzf8xGawq0AR59i7686Oxc4ZIaA=;\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=a3P9K9hnzarnFQQ/ad817H8es8c0iGF6q1YF6Z40FpDW+908+13sp0te7iOhem00G\n\tK/5ut8ndBE3rTnmogLSIqg8O910BRBtsgQ1m1JLZDpL02IXZysXuu3ir24ETRRxMAR\n\tVoR352kX6k9qpMjJXxmGc3mop5BDCLncGI22TQ66Kzqi3N7wwGy/QuLcZJehvgszhM\n\tgL1v7LcJBgK2ZUmM6MNwA/Yd7AK3bwjdH2NCM/MtkXY0l2GvOf4oiOAu297v8qhlGC\n\tfidwVLJwXnX2pyo73M/O4rjHv/i1K+duMD2TjlRCO/a7qCqKDS1wTmxwr2CWPLHNe2\n\tRw3AyhyzLXP4w==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1659360638;\n\tbh=xSgSdAeiI7tr54OPrzf8xGawq0AR59i7686Oxc4ZIaA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fOK8CGX/f+olaHFSXFJytRrXV833WOYdmwTBccFpKe9Yqk5uMPD/YGqNLO/LBkN7f\n\tLHKZ/YA8h4+pZkG5btCpIe9rn3diKQso4Lby2nUKQN2PiWrLvRfDe8uz62VJWvXLsg\n\tpwJFZzofJ2wKO+BnSxrONOccl8/JvHzt51xPVefA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"fOK8CGX/\"; dkim-atps=neutral","Date":"Mon, 1 Aug 2022 16:30:34 +0300","To":"David Plowman ","Message-ID":"","References":"<20220729103846.414819-1-umang.jain@ideasonboard.com>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Provide colorimetry <>\n\tColorSpace mappings","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":"rishikeshdonadkar@gmail.com,\n\tlibcamera devel ,\n\tvedantparanjape160201@gmail.com, nicolas.dufresne@collabora.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24267,"web_url":"https://patchwork.libcamera.org/comment/24267/","msgid":"<2cf4b2d6-69c9-3c85-24bc-03f1367e2936@ideasonboard.com>","date":"2022-08-01T13:55:01","subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Provide colorimetry <>\n\tColorSpace mappings","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent and David,\n\nOn 8/1/22 19:00, Laurent Pinchart wrote:\n> Hi David,\n>\n> On Mon, Aug 01, 2022 at 02:20:12PM +0100, David Plowman wrote:\n>> On Mon, 1 Aug 2022 at 13:31, Umang Jain wrote:\n>>> On 8/1/22 01:14, Laurent Pinchart wrote:\n>>>> On Fri, Jul 29, 2022 at 04:08:46PM +0530, Umang Jain via libcamera-devel wrote:\n>>>>> From: Rishikesh Donadkar \n>>>>>\n>>>>> Provide colorimetry <=> libcamera::ColorSpace mappings via:\n>>>>> - GstVideoColorimetry colorimetry_from_colorspace(colorspace);\n>>>>> - ColorSpace colorspace_from_colorimetry(colorimetry);\n>>>>>\n>>>>> Read the colorimetry field from caps into the stream configuration.\n>>>>> After stream validation, the sensor supported colorimetry will\n>>>>> be retrieved and the caps will be updated accordingly.\n>>>>>\n>>>>> Colorimetry support for gstlibcamerasrc currently undertakes only one\n>>>> s/gstlibcamerasrc/libcamerasrc/ (or gstlibcamera if you meant the\n>>>> library, not the element name)\n>>>>\n>>>>> argument. Multiple colorimetry support shall be introduced in\n>>>>> subsequent commits.\n>>>> I'm probably missing something here, what do you mean by multiple\n>>>> colorimetry support ?\n>>>>\n>>>>> Signed-off-by: Rishikesh Donadkar \n>>>>> Signed-off-by: Umang Jain \n>>>>> ---\n>>>>>\n>>>>> Changes in v2:\n>>>>> - Drop \"Default\" Colorspace\n>>>>> - Improve function signature of colorimetry_from_colorspace() and\n>>>>> colorspace_from_colorimetry()\n>>>>> - Map GST_VIDEO_COLOR_MATRIX_RGB to Colorspace::YcbcrEncoding::Rec601\n>>>>> (comes from the kernel)\n>>>>> - Map GST_VIDEO_COLOR_PRIMARIES_UNKNOWN to Raw colorspace\n>>>>> - Map ColorSpace::YcbcrEncoding::Rec601 to GST_VIDEO_COLOR_MATRIX_RGB\n>>>>> if colorspace is \"sRGB\". GST_VIDEO_COLOR_MATRIX_BT601 for all other\n>>>>> cases\n>>>>> - Minor nits regarding error reporting strings.\n>>>>> ---\n>>>>> src/gstreamer/gstlibcamera-utils.cpp | 164 +++++++++++++++++++++++++++\n>>>>> 1 file changed, 164 insertions(+)\n>>>>>\n>>>>> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n>>>>> index c97c0d43..e4ff1269 100644\n>>>>> --- a/src/gstreamer/gstlibcamera-utils.cpp\n>>>>> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n>>>>> @@ -45,6 +45,146 @@ static struct {\n>>>>> /* \\todo NV42 is used in libcamera but is not mapped in GStreamer yet. */\n>>>>> };\n>>>>>\n>>>>> +static GstVideoColorimetry\n>>>>> +colorimetry_from_colorspace(const ColorSpace &colorSpace)\n>>>>> +{\n>>>>> + GstVideoColorimetry colorimetry;\n>>>>> +\n>>>>> + switch (colorSpace.primaries) {\n>>>>> + case ColorSpace::Primaries::Rec709:\n>>>>> + colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_BT709;\n>>>>> + break;\n>>>>> + case ColorSpace::Primaries::Rec2020:\n>>>>> + colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_BT2020;\n>>>>> + break;\n>>>>> + case ColorSpace::Primaries::Smpte170m:\n>>>>> + colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_SMPTE170M;\n>>>>> + break;\n>>>>> + case ColorSpace::Primaries::Raw:\n>>>>> + colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_UNKNOWN;\n>>>>> + break;\n>>>>> + }\n>>>> I'd sort the cases with the order of the enum in color_space.h (below\n>>>> too). That's very minor.\n>>> Ack.\n>>>\n>>>>> +\n>>>>> + switch (colorSpace.transferFunction) {\n>>>>> + case ColorSpace::TransferFunction::Rec709:\n>>>>> + colorimetry.transfer = GST_VIDEO_TRANSFER_BT709;\n>>>>> + break;\n>>>>> + case ColorSpace::TransferFunction::Srgb:\n>>>>> + colorimetry.transfer = GST_VIDEO_TRANSFER_SRGB;\n>>>>> + break;\n>>>>> + case ColorSpace::TransferFunction::Linear:\n>>>>> + colorimetry.transfer = GST_VIDEO_TRANSFER_GAMMA10;\n>>>>> + break;\n>>>>> + }\n>>>>> +\n>>>>> + switch (colorSpace.ycbcrEncoding) {\n>>>>> + case ColorSpace::YcbcrEncoding::None:\n>>>>> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_RGB;\n>>>>> + break;\n>>>>> + case ColorSpace::YcbcrEncoding::Rec709:\n>>>>> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT709;\n>>>>> + break;\n>>>>> + case ColorSpace::YcbcrEncoding::Rec2020:\n>>>>> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT2020;\n>>>>> + break;\n>>>>> + case ColorSpace::YcbcrEncoding::Rec601:\n>>>>> + if (colorSpace == ColorSpace::Srgb)\n>>>>> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_RGB;\n>>>> I don't think this one is right. GST_VIDEO_COLOR_MATRIX_RGB is the\n>>>> identity matrix, which means no encoding to YCbCr, and\n>>>> ColorSpace::YcbcrEncoding::Rec601 clearly defines an encoding.\n>>>>\n>>>> I think the problem is caused by our definition of ColorSpace::Srgb:\n>>>>\n>>>> const ColorSpace ColorSpace::Srgb = {\n>>>> Primaries::Rec709,\n>>>> TransferFunction::Srgb,\n>>>> YcbcrEncoding::Rec601,\n>>>> Range::Limited\n>>>> };\n>>>>\n>>>> I propose modifying it to\n>>>>\n>>>> const ColorSpace ColorSpace::Srgb = {\n>>>> Primaries::Rec709,\n>>>> TransferFunction::Srgb,\n>>>> YcbcrEncoding::None,\n>>>> Range::Full\n>>>> };\n>>> ack!\n>>>\n>>> My question which I think I brought up in v1 itself now (you can choose\n>>> to reply anywhere) is, do we need a preset to represent YUV-encoded RGB\n>>> and what would we call it? ColorSpace::sYCC ?\n>>>\n>>>> and adapting the rest of libcamera accordingly.\n>>>>\n>>>> David, this will affect Raspberry Pi, so I'd appreciate your opinion.\n>>> Patiently waiting ;-)\n>> Sorry, the thread is getting a bit long so it takes me a while to\n>> notice things!!\n>>\n>> So the original sRGB colour space definition comes from:\n>>\n>> https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/colorspaces-details.html#col-srgb\n>>\n>> I agree that the values aren't necessarily the ones you first think\n>> of, there is some history behind some of them.\n>>\n>> I don't think it matters for the Pi, though. I believe the only colour\n>> spaces we ask for are Jpeg, Smpte170m and Rec709. (When we really want\n>> sRGB for still capture we ask for Jpeg.)\n\n\nThanks!\n\n> Thank you for the information. Unless there's any objection, I then\n> propose modifying Srgb to define it as\n>\n> const ColorSpace ColorSpace::Srgb = {\n> \tPrimaries::Rec709,\n> \tTransferFunction::Srgb,\n> \tYcbcrEncoding::None,\n> \tRange::Full\n> };\n>\n> We could also define\n>\n> const ColorSpace ColorSpace::Sycc = {\n> \tPrimaries::Rec709,\n> \tTransferFunction::Srgb,\n> \tYcbcrEncoding::Rec601,\n> \tRange::Limited\n> };\n>\n> but I'm tempted to hold off until it becomes needed.\n\n\nI am tempted to add it because:\n\n  static const std::map v4l2ToColorSpace = {\n         { V4L2_COLORSPACE_RAW, ColorSpace::Raw },\n         { V4L2_COLORSPACE_JPEG, ColorSpace::Jpeg },\n-       { V4L2_COLORSPACE_SRGB, ColorSpace::Srgb },\n+     { V4L2_COLORSPACE_SRGB, ColorSpace::Sycc },\n\n@@ -772,7 +772,7 @@ static const std::map \nv4l2ToRange = {\n  static const std::vector> \ncolorSpaceToV4l2 = {\n         { ColorSpace::Raw, V4L2_COLORSPACE_RAW },\n         { ColorSpace::Jpeg, V4L2_COLORSPACE_JPEG },\n-       { ColorSpace::Srgb, V4L2_COLORSPACE_SRGB },\n+       { ColorSpace::Sycc, V4L2_COLORSPACE_SRGB },\n\n\nUnless, you want to drop the preset mappings as well?\n\n>\n>>>>> + else\n>>>>> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT601;\n>>>>> + break;\n>>>>> + }\n>>>>> +\n>>>>> + switch (colorSpace.range) {\n>>>>> + case ColorSpace::Range::Full:\n>>>>> + colorimetry.range = GST_VIDEO_COLOR_RANGE_0_255;\n>>>>> + break;\n>>>>> + case ColorSpace::Range::Limited:\n>>>>> + colorimetry.range = GST_VIDEO_COLOR_RANGE_16_235;\n>>>>> + break;\n>>>>> + }\n>>>>> +\n>>>>> + return colorimetry;\n>>>>> +}\n>>>>> +\n>>>>> +static ColorSpace\n>>>>> +colorspace_from_colorimetry(const GstVideoColorimetry &colorimetry)\n>>>>> +{\n>>>>> + ColorSpace colorspace = ColorSpace::Raw;\n>>>>> +\n>>>>> + switch (colorimetry.primaries) {\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>>>>> + case GST_VIDEO_COLOR_PRIMARIES_SMPTE170M:\n>>>>> + colorspace.primaries = ColorSpace::Primaries::Smpte170m;\n>>>>> + break;\n>>>>> + case GST_VIDEO_COLOR_PRIMARIES_UNKNOWN:\n>>>>> + /* Unknown primaries map to raw colorspace in gstreamer */\n>>>>> + return colorspace;\n>>>> I would\n>>>>\n>>>> return ColorSpace::Raw;\n>>>>\n>>>> here to make it more explicit.\n>>> ack\n>>>\n>>>>> + default:\n>>>>> + g_error(\"Unsupported colorimetry primaries: %d\", colorimetry.primaries);\n>>>> g_error() seems very harsh, do we really need to abort() if the user\n>>>> asks for unsupported primaries (or other colorimetry fields below) ?\n>>> I agree it's harsh - initially I kept as 'warning' but then it was\n>>> advised that 'warnings' always come with mitigation steps.\n>>>\n>>> I guess the only mitigation technique here is, making sure mappings exists!\n>>>\n>>>> This is probably a question for Nicolas.\n>>>>\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 supported\");\n>>>>> + /* fallthrough */\n>>>>> + case GST_VIDEO_TRANSFER_GAMMA10:\n>>>>> + colorspace.transferFunction = ColorSpace::TransferFunction::Linear;\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 = ColorSpace::TransferFunction::Rec709;\n>>>>> + break;\n>>>>> + case GST_VIDEO_TRANSFER_SRGB:\n>>>>> + colorspace.transferFunction = ColorSpace::TransferFunction::Srgb;\n>>>>> + break;\n>>>>> + default:\n>>>>> + g_error(\"Unsupported colorimetry transfer function: %d\", colorimetry.transfer);\n>>>>> + }\n>>>>> +\n>>>>> + switch (colorimetry.matrix) {\n>>>>> + /* FCC is about the same as BT601 with less digit */\n>>>>> + case GST_VIDEO_COLOR_MATRIX_FCC:\n>>>>> + case GST_VIDEO_COLOR_MATRIX_BT601:\n>>>>> + /* v4l2_ycbcr_encoding of sRGB is ENC_601 in the kernel */\n>>>> That's right, but I don't think we need to be concerned by that here.\n>>>> GST_VIDEO_COLOR_MATRIX_RGB should be mapped to\n>>>> ColorSpace::YcbcrEncoding::None, and we should handle the V4L2 sRGB YUV\n>>>> encoding internally in libcamera.\n>>> replied to the latter part, in detail, in v1\n>>>\n>>>>> + case GST_VIDEO_COLOR_MATRIX_RGB:\n>>>>> + colorspace.ycbcrEncoding = ColorSpace::YcbcrEncoding::Rec601;\n>>>>> + break;\n>>>>> + case GST_VIDEO_COLOR_MATRIX_BT709:\n>>>>> + colorspace.ycbcrEncoding = ColorSpace::YcbcrEncoding::Rec709;\n>>>>> + break;\n>>>>> + case GST_VIDEO_COLOR_MATRIX_BT2020:\n>>>>> + colorspace.ycbcrEncoding = ColorSpace::YcbcrEncoding::Rec2020;\n>>>>> + break;\n>>>>> + default:\n>>>>> + g_error(\"Unsupported colorimetry matrix: %d\", colorimetry.matrix);\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>>>>> + g_error(\"Unsupported colorimetry range %d\", colorimetry.range);\n>>>>> + }\n>>>>> +\n>>>>> + return colorspace;\n>>>>> +}\n>>>>> +\n>>>>> static GstVideoFormat\n>>>>> pixel_format_to_gst_format(const PixelFormat &format)\n>>>>> {\n>>>>> @@ -139,6 +279,18 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg\n>>>>> \"width\", G_TYPE_INT, stream_cfg.size.width,\n>>>>> \"height\", G_TYPE_INT, stream_cfg.size.height,\n>>>>> nullptr);\n>>>>> +\n>>>>> + if (stream_cfg.colorSpace) {\n>>>>> + GstVideoColorimetry colorimetry = colorimetry_from_colorspace(stream_cfg.colorSpace.value());\n>>>>> + gchar *colorimetry_str = gst_video_colorimetry_to_string(&colorimetry);\n>>>>> +\n>>>>> + if (colorimetry_str)\n>>>>> + gst_structure_set(s, \"colorimetry\", G_TYPE_STRING, colorimetry_str, nullptr);\n>>>>> + else\n>>>>> + g_error(\"Got invalid colorimetry from ColorSpace: %s\",\n>>>>> + ColorSpace::toString(stream_cfg.colorSpace).c_str());\n>>>> Same question as above about g_error(). As far as I understand,\n>>>> gst_video_colorimetry_to_string() will return null either if it can't\n>>>> alocate memory for the string (in which case we're screwed anyway, so\n>>>> g_error() is fine) or if colorimetry is all UNKNOWN. The latter should\n>>>> never happen given the implementation of colorimetry_from_colorspace(),\n>>>> so I suppose it's fine ?\n>>> Yes, I think it's fine\n>>>\n>>>>> + }\n>>>>> +\n>>>>> gst_caps_append_structure(caps, s);\n>>>>>\n>>>>> return caps;\n>>>>> @@ -222,6 +374,18 @@ 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>>>>> + /* Configure colorimetry */\n>>>>> + if (gst_structure_has_field(s, \"colorimetry\")) {\n>>>>> + const gchar *colorimetry_caps = gst_structure_get_string(s, \"colorimetry\");\n>>>>> + GstVideoColorimetry colorimetry;\n>>>>> +\n>>>>> + if(gst_video_colorimetry_from_string(&colorimetry, colorimetry_caps)) {\n>>>> Missing space after \"if\".\n>>>>\n>>>>> + stream_cfg.colorSpace = colorspace_from_colorimetry(colorimetry);\n>>>>> + } else {\n>>>>> + g_critical(\"Invalid colorimetry %s\", colorimetry_caps);\n>>>>> + }\n>>>>> + }\n>>>>> }\n>>>>>\n>>>>> #if !GST_CHECK_VERSION(1, 17, 1)","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 CB044C3275\n\tfor ;\n\tMon, 1 Aug 2022 13:55:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1D06D6330F;\n\tMon, 1 Aug 2022 15:55:10 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E1E37603E8\n\tfor ;\n\tMon, 1 Aug 2022 15:55:08 +0200 (CEST)","from [IPV6:2401:4900:1f3e:f7a:bc8f:12ed:b45f:c35d] (unknown\n\t[IPv6:2401:4900:1f3e:f7a:bc8f:12ed:b45f:c35d])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 905332F3;\n\tMon, 1 Aug 2022 15:55:06 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659362110;\n\tbh=P+ECJMy/fbms4vVyxTyehcf5UBL2kG/FyrZcQtY7N7w=;\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=pciloCbTJbnLxXIRm4rYV8WbFr5lforfIAQInHYUkudx4mBdikqVeCnH8fNNirWNS\n\tFtAZFE2ST682E66ZSEnuHHv68UjxPEeUXszOTo10jA0raXpiov1t17jq1m44ylWy5Z\n\tR0gvRtAHIMaD0yKQAQ2Gj3pKu7X8fwLIWeyxVTO5V9DecDlZH2hTNBFKUvOn8Glm44\n\tJZczwOIwjLIFt9rhadV0Cb8e0FMFizcHVPyGWCZWwiSuDs4hhyIQIPe9KTHd5HbthJ\n\tBwaqmksk/CRDBInwYF7osEcC9bNYNtXJkKnugSWk69OEM5jKCkOl1ad/7nXwRkXinw\n\te1fu96oWi/arA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1659362108;\n\tbh=P+ECJMy/fbms4vVyxTyehcf5UBL2kG/FyrZcQtY7N7w=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=KYV1K1Ajf+jLHz1vUJFMI0ZIMHG08+5Y5DmMqmr7Ev1dz+7J3/WxYaT6wIrUKqFGw\n\tXnpUIxpacj0KPbw5f9jIJHYyZZVk8Zn8Yt5MMT78RUboIhi7n9aeS7XyViyGtPs4Zx\n\tMrvdEZlXAvdtEWMfW8gkvimc0N5K99HmVJWJ8D+Y="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"KYV1K1Aj\"; dkim-atps=neutral","Message-ID":"<2cf4b2d6-69c9-3c85-24bc-03f1367e2936@ideasonboard.com>","Date":"Mon, 1 Aug 2022 19:25:01 +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":"Laurent Pinchart ,\n\tDavid Plowman ","References":"<20220729103846.414819-1-umang.jain@ideasonboard.com>\n\t\n\t\n\t\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Provide colorimetry <>\n\tColorSpace mappings","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":"rishikeshdonadkar@gmail.com,\n\tlibcamera devel ,\n\tvedantparanjape160201@gmail.com, nicolas.dufresne@collabora.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24307,"web_url":"https://patchwork.libcamera.org/comment/24307/","msgid":"","date":"2022-08-02T12:45:28","subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Provide colorimetry <>\n\tColorSpace mappings","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Mon, Aug 01, 2022 at 07:25:01PM +0530, Umang Jain wrote:\n> On 8/1/22 19:00, Laurent Pinchart wrote:\n> > On Mon, Aug 01, 2022 at 02:20:12PM +0100, David Plowman wrote:\n> >> On Mon, 1 Aug 2022 at 13:31, Umang Jain wrote:\n> >>> On 8/1/22 01:14, Laurent Pinchart wrote:\n> >>>> On Fri, Jul 29, 2022 at 04:08:46PM +0530, Umang Jain via libcamera-devel wrote:\n> >>>>> From: Rishikesh Donadkar \n> >>>>>\n> >>>>> Provide colorimetry <=> libcamera::ColorSpace mappings via:\n> >>>>> - GstVideoColorimetry colorimetry_from_colorspace(colorspace);\n> >>>>> - ColorSpace colorspace_from_colorimetry(colorimetry);\n> >>>>>\n> >>>>> Read the colorimetry field from caps into the stream configuration.\n> >>>>> After stream validation, the sensor supported colorimetry will\n> >>>>> be retrieved and the caps will be updated accordingly.\n> >>>>>\n> >>>>> Colorimetry support for gstlibcamerasrc currently undertakes only one\n> >>>>\n> >>>> s/gstlibcamerasrc/libcamerasrc/ (or gstlibcamera if you meant the\n> >>>> library, not the element name)\n> >>>>\n> >>>>> argument. Multiple colorimetry support shall be introduced in\n> >>>>> subsequent commits.\n> >>>>\n> >>>> I'm probably missing something here, what do you mean by multiple\n> >>>> colorimetry support ?\n> >>>>\n> >>>>> Signed-off-by: Rishikesh Donadkar \n> >>>>> Signed-off-by: Umang Jain \n> >>>>> ---\n> >>>>>\n> >>>>> Changes in v2:\n> >>>>> - Drop \"Default\" Colorspace\n> >>>>> - Improve function signature of colorimetry_from_colorspace() and\n> >>>>> colorspace_from_colorimetry()\n> >>>>> - Map GST_VIDEO_COLOR_MATRIX_RGB to Colorspace::YcbcrEncoding::Rec601\n> >>>>> (comes from the kernel)\n> >>>>> - Map GST_VIDEO_COLOR_PRIMARIES_UNKNOWN to Raw colorspace\n> >>>>> - Map ColorSpace::YcbcrEncoding::Rec601 to GST_VIDEO_COLOR_MATRIX_RGB\n> >>>>> if colorspace is \"sRGB\". GST_VIDEO_COLOR_MATRIX_BT601 for all other\n> >>>>> cases\n> >>>>> - Minor nits regarding error reporting strings.\n> >>>>> ---\n> >>>>> src/gstreamer/gstlibcamera-utils.cpp | 164 +++++++++++++++++++++++++++\n> >>>>> 1 file changed, 164 insertions(+)\n> >>>>>\n> >>>>> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> >>>>> index c97c0d43..e4ff1269 100644\n> >>>>> --- a/src/gstreamer/gstlibcamera-utils.cpp\n> >>>>> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> >>>>> @@ -45,6 +45,146 @@ static struct {\n> >>>>> /* \\todo NV42 is used in libcamera but is not mapped in GStreamer yet. */\n> >>>>> };\n> >>>>>\n> >>>>> +static GstVideoColorimetry\n> >>>>> +colorimetry_from_colorspace(const ColorSpace &colorSpace)\n> >>>>> +{\n> >>>>> + GstVideoColorimetry colorimetry;\n> >>>>> +\n> >>>>> + switch (colorSpace.primaries) {\n> >>>>> + case ColorSpace::Primaries::Rec709:\n> >>>>> + colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_BT709;\n> >>>>> + break;\n> >>>>> + case ColorSpace::Primaries::Rec2020:\n> >>>>> + colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_BT2020;\n> >>>>> + break;\n> >>>>> + case ColorSpace::Primaries::Smpte170m:\n> >>>>> + colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_SMPTE170M;\n> >>>>> + break;\n> >>>>> + case ColorSpace::Primaries::Raw:\n> >>>>> + colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_UNKNOWN;\n> >>>>> + break;\n> >>>>> + }\n> >>>>\n> >>>> I'd sort the cases with the order of the enum in color_space.h (below\n> >>>> too). That's very minor.\n> >>>\n> >>> Ack.\n> >>>\n> >>>>> +\n> >>>>> + switch (colorSpace.transferFunction) {\n> >>>>> + case ColorSpace::TransferFunction::Rec709:\n> >>>>> + colorimetry.transfer = GST_VIDEO_TRANSFER_BT709;\n> >>>>> + break;\n> >>>>> + case ColorSpace::TransferFunction::Srgb:\n> >>>>> + colorimetry.transfer = GST_VIDEO_TRANSFER_SRGB;\n> >>>>> + break;\n> >>>>> + case ColorSpace::TransferFunction::Linear:\n> >>>>> + colorimetry.transfer = GST_VIDEO_TRANSFER_GAMMA10;\n> >>>>> + break;\n> >>>>> + }\n> >>>>> +\n> >>>>> + switch (colorSpace.ycbcrEncoding) {\n> >>>>> + case ColorSpace::YcbcrEncoding::None:\n> >>>>> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_RGB;\n> >>>>> + break;\n> >>>>> + case ColorSpace::YcbcrEncoding::Rec709:\n> >>>>> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT709;\n> >>>>> + break;\n> >>>>> + case ColorSpace::YcbcrEncoding::Rec2020:\n> >>>>> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT2020;\n> >>>>> + break;\n> >>>>> + case ColorSpace::YcbcrEncoding::Rec601:\n> >>>>> + if (colorSpace == ColorSpace::Srgb)\n> >>>>> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_RGB;\n> >>>>\n> >>>> I don't think this one is right. GST_VIDEO_COLOR_MATRIX_RGB is the\n> >>>> identity matrix, which means no encoding to YCbCr, and\n> >>>> ColorSpace::YcbcrEncoding::Rec601 clearly defines an encoding.\n> >>>>\n> >>>> I think the problem is caused by our definition of ColorSpace::Srgb:\n> >>>>\n> >>>> const ColorSpace ColorSpace::Srgb = {\n> >>>> Primaries::Rec709,\n> >>>> TransferFunction::Srgb,\n> >>>> YcbcrEncoding::Rec601,\n> >>>> Range::Limited\n> >>>> };\n> >>>>\n> >>>> I propose modifying it to\n> >>>>\n> >>>> const ColorSpace ColorSpace::Srgb = {\n> >>>> Primaries::Rec709,\n> >>>> TransferFunction::Srgb,\n> >>>> YcbcrEncoding::None,\n> >>>> Range::Full\n> >>>> };\n> >>>\n> >>> ack!\n> >>>\n> >>> My question which I think I brought up in v1 itself now (you can choose\n> >>> to reply anywhere) is, do we need a preset to represent YUV-encoded RGB\n> >>> and what would we call it? ColorSpace::sYCC ?\n> >>>\n> >>>> and adapting the rest of libcamera accordingly.\n> >>>>\n> >>>> David, this will affect Raspberry Pi, so I'd appreciate your opinion.\n> >>>\n> >>> Patiently waiting ;-)\n> >>\n> >> Sorry, the thread is getting a bit long so it takes me a while to\n> >> notice things!!\n> >>\n> >> So the original sRGB colour space definition comes from:\n> >>\n> >> https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/colorspaces-details.html#col-srgb\n> >>\n> >> I agree that the values aren't necessarily the ones you first think\n> >> of, there is some history behind some of them.\n> >>\n> >> I don't think it matters for the Pi, though. I believe the only colour\n> >> spaces we ask for are Jpeg, Smpte170m and Rec709. (When we really want\n> >> sRGB for still capture we ask for Jpeg.)\n> \n> Thanks!\n> \n> > Thank you for the information. Unless there's any objection, I then\n> > propose modifying Srgb to define it as\n> >\n> > const ColorSpace ColorSpace::Srgb = {\n> > \tPrimaries::Rec709,\n> > \tTransferFunction::Srgb,\n> > \tYcbcrEncoding::None,\n> > \tRange::Full\n> > };\n> >\n> > We could also define\n> >\n> > const ColorSpace ColorSpace::Sycc = {\n> > \tPrimaries::Rec709,\n> > \tTransferFunction::Srgb,\n> > \tYcbcrEncoding::Rec601,\n> > \tRange::Limited\n> > };\n> >\n> > but I'm tempted to hold off until it becomes needed.\n> \n> \n> I am tempted to add it because:\n> \n>  static const std::map v4l2ToColorSpace = {\n>         { V4L2_COLORSPACE_RAW, ColorSpace::Raw },\n>         { V4L2_COLORSPACE_JPEG, ColorSpace::Jpeg },\n> -       { V4L2_COLORSPACE_SRGB, ColorSpace::Srgb },\n> +     { V4L2_COLORSPACE_SRGB, ColorSpace::Sycc },\n> \n> @@ -772,7 +772,7 @@ static const std::map \n> v4l2ToRange = {\n>  static const std::vector> \n> colorSpaceToV4l2 = {\n>         { ColorSpace::Raw, V4L2_COLORSPACE_RAW },\n>         { ColorSpace::Jpeg, V4L2_COLORSPACE_JPEG },\n> -       { ColorSpace::Srgb, V4L2_COLORSPACE_SRGB },\n> +       { ColorSpace::Sycc, V4L2_COLORSPACE_SRGB },\n> \n> \n> Unless, you want to drop the preset mappings as well?\n\nThese mappings are only used to handle conversion between a ColorSpace\nvalue and V4L2 color space components. There's no need to keep them if\nthey don't fulfil the job at hand, and there's also no reason why you\nwould need a ColorSpace preset in there, you can construct a ColorSpace\nexplicitly.\n\n> >>>>> + else\n> >>>>> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT601;\n> >>>>> + break;\n> >>>>> + }\n> >>>>> +\n> >>>>> + switch (colorSpace.range) {\n> >>>>> + case ColorSpace::Range::Full:\n> >>>>> + colorimetry.range = GST_VIDEO_COLOR_RANGE_0_255;\n> >>>>> + break;\n> >>>>> + case ColorSpace::Range::Limited:\n> >>>>> + colorimetry.range = GST_VIDEO_COLOR_RANGE_16_235;\n> >>>>> + break;\n> >>>>> + }\n> >>>>> +\n> >>>>> + return colorimetry;\n> >>>>> +}\n> >>>>> +\n> >>>>> +static ColorSpace\n> >>>>> +colorspace_from_colorimetry(const GstVideoColorimetry &colorimetry)\n> >>>>> +{\n> >>>>> + ColorSpace colorspace = ColorSpace::Raw;\n> >>>>> +\n> >>>>> + switch (colorimetry.primaries) {\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> >>>>> + case GST_VIDEO_COLOR_PRIMARIES_SMPTE170M:\n> >>>>> + colorspace.primaries = ColorSpace::Primaries::Smpte170m;\n> >>>>> + break;\n> >>>>> + case GST_VIDEO_COLOR_PRIMARIES_UNKNOWN:\n> >>>>> + /* Unknown primaries map to raw colorspace in gstreamer */\n> >>>>> + return colorspace;\n> >>>>\n> >>>> I would\n> >>>>\n> >>>> return ColorSpace::Raw;\n> >>>>\n> >>>> here to make it more explicit.\n> >>>\n> >>> ack\n> >>>\n> >>>>> + default:\n> >>>>> + g_error(\"Unsupported colorimetry primaries: %d\", colorimetry.primaries);\n> >>>>\n> >>>> g_error() seems very harsh, do we really need to abort() if the user\n> >>>> asks for unsupported primaries (or other colorimetry fields below) ?\n> >>>\n> >>> I agree it's harsh - initially I kept as 'warning' but then it was\n> >>> advised that 'warnings' always come with mitigation steps.\n> >>>\n> >>> I guess the only mitigation technique here is, making sure mappings exists!\n> >>>\n> >>>> This is probably a question for Nicolas.\n> >>>>\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 supported\");\n> >>>>> + /* fallthrough */\n> >>>>> + case GST_VIDEO_TRANSFER_GAMMA10:\n> >>>>> + colorspace.transferFunction = ColorSpace::TransferFunction::Linear;\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 = ColorSpace::TransferFunction::Rec709;\n> >>>>> + break;\n> >>>>> + case GST_VIDEO_TRANSFER_SRGB:\n> >>>>> + colorspace.transferFunction = ColorSpace::TransferFunction::Srgb;\n> >>>>> + break;\n> >>>>> + default:\n> >>>>> + g_error(\"Unsupported colorimetry transfer function: %d\", colorimetry.transfer);\n> >>>>> + }\n> >>>>> +\n> >>>>> + switch (colorimetry.matrix) {\n> >>>>> + /* FCC is about the same as BT601 with less digit */\n> >>>>> + case GST_VIDEO_COLOR_MATRIX_FCC:\n> >>>>> + case GST_VIDEO_COLOR_MATRIX_BT601:\n> >>>>> + /* v4l2_ycbcr_encoding of sRGB is ENC_601 in the kernel */\n> >>>>\n> >>>> That's right, but I don't think we need to be concerned by that here.\n> >>>> GST_VIDEO_COLOR_MATRIX_RGB should be mapped to\n> >>>> ColorSpace::YcbcrEncoding::None, and we should handle the V4L2 sRGB YUV\n> >>>> encoding internally in libcamera.\n> >>>\n> >>> replied to the latter part, in detail, in v1\n> >>>\n> >>>>> + case GST_VIDEO_COLOR_MATRIX_RGB:\n> >>>>> + colorspace.ycbcrEncoding = ColorSpace::YcbcrEncoding::Rec601;\n> >>>>> + break;\n> >>>>> + case GST_VIDEO_COLOR_MATRIX_BT709:\n> >>>>> + colorspace.ycbcrEncoding = ColorSpace::YcbcrEncoding::Rec709;\n> >>>>> + break;\n> >>>>> + case GST_VIDEO_COLOR_MATRIX_BT2020:\n> >>>>> + colorspace.ycbcrEncoding = ColorSpace::YcbcrEncoding::Rec2020;\n> >>>>> + break;\n> >>>>> + default:\n> >>>>> + g_error(\"Unsupported colorimetry matrix: %d\", colorimetry.matrix);\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> >>>>> + g_error(\"Unsupported colorimetry range %d\", colorimetry.range);\n> >>>>> + }\n> >>>>> +\n> >>>>> + return colorspace;\n> >>>>> +}\n> >>>>> +\n> >>>>> static GstVideoFormat\n> >>>>> pixel_format_to_gst_format(const PixelFormat &format)\n> >>>>> {\n> >>>>> @@ -139,6 +279,18 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg\n> >>>>> \"width\", G_TYPE_INT, stream_cfg.size.width,\n> >>>>> \"height\", G_TYPE_INT, stream_cfg.size.height,\n> >>>>> nullptr);\n> >>>>> +\n> >>>>> + if (stream_cfg.colorSpace) {\n> >>>>> + GstVideoColorimetry colorimetry = colorimetry_from_colorspace(stream_cfg.colorSpace.value());\n> >>>>> + gchar *colorimetry_str = gst_video_colorimetry_to_string(&colorimetry);\n> >>>>> +\n> >>>>> + if (colorimetry_str)\n> >>>>> + gst_structure_set(s, \"colorimetry\", G_TYPE_STRING, colorimetry_str, nullptr);\n> >>>>> + else\n> >>>>> + g_error(\"Got invalid colorimetry from ColorSpace: %s\",\n> >>>>> + ColorSpace::toString(stream_cfg.colorSpace).c_str());\n> >>>>\n> >>>> Same question as above about g_error(). As far as I understand,\n> >>>> gst_video_colorimetry_to_string() will return null either if it can't\n> >>>> alocate memory for the string (in which case we're screwed anyway, so\n> >>>> g_error() is fine) or if colorimetry is all UNKNOWN. The latter should\n> >>>> never happen given the implementation of colorimetry_from_colorspace(),\n> >>>> so I suppose it's fine ?\n> >>>\n> >>> Yes, I think it's fine\n> >>>\n> >>>>> + }\n> >>>>> +\n> >>>>> gst_caps_append_structure(caps, s);\n> >>>>>\n> >>>>> return caps;\n> >>>>> @@ -222,6 +374,18 @@ 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> >>>>> + /* Configure colorimetry */\n> >>>>> + if (gst_structure_has_field(s, \"colorimetry\")) {\n> >>>>> + const gchar *colorimetry_caps = gst_structure_get_string(s, \"colorimetry\");\n> >>>>> + GstVideoColorimetry colorimetry;\n> >>>>> +\n> >>>>> + if(gst_video_colorimetry_from_string(&colorimetry, colorimetry_caps)) {\n> >>>>\n> >>>> Missing space after \"if\".\n> >>>>\n> >>>>> + stream_cfg.colorSpace = colorspace_from_colorimetry(colorimetry);\n> >>>>> + } else {\n> >>>>> + g_critical(\"Invalid colorimetry %s\", colorimetry_caps);\n> >>>>> + }\n> >>>>> + }\n> >>>>> }\n> >>>>>\n> >>>>> #if !GST_CHECK_VERSION(1, 17, 1)","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 AEFCAC3275\n\tfor ;\n\tTue, 2 Aug 2022 12:45:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 161276330F;\n\tTue, 2 Aug 2022 14:45:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E29E4603E6\n\tfor ;\n\tTue, 2 Aug 2022 14:45:33 +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 3E42225B;\n\tTue, 2 Aug 2022 14:45:33 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659444335;\n\tbh=WtKw7mBmI7bbkMttFOzZFK6xzlPxoVh9WvHCB/WflEw=;\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=gU5T62kW1Akaaao3BcwWtikjHyZVZV8Iky7+vkBulmhhBEsRdG+fUNP1inN2twm0v\n\tnxe8IacB/fDD/GGs0kKwlecLLBlp0+aWduff6A4KBwxX+4oGOwmLRalJIw9rc0rRp9\n\tO2uHxMC731Q+sHMrKt1aQTxaYH2QHyQ7+5b4eOz5OlJ8HpkI7fXZ26p7UVleIa6lQ1\n\tru3wojEAb3xu98LYXBO5yfxSdsGN0J6swghTWzQ+ymA5iDaMAzcTEMtPSZzmoP+tZ9\n\tOUkiYi2D8cXosYZxEXFst5RICQcx+I0fckCvHNMd8vk9s3UtUj75c3KA3THPS5eYWL\n\tg16zmremXpQLA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1659444333;\n\tbh=WtKw7mBmI7bbkMttFOzZFK6xzlPxoVh9WvHCB/WflEw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bqcDea2fc7f8xdLo+HRHPZ8DPGlnJmKlfBJZrXLbGiqYflbLQbhIVo4wqQXA0g3mc\n\tauCDJY8Lb/PdWwCkqRgeVP2fLebZ6Sj83+SieXXvHr9zcGWZhGI8Qdft5QisZyJcge\n\tU8tr8F1wM69vB8YyMbTbNDdNwx6KAXHHqHyusa0c="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"bqcDea2f\"; dkim-atps=neutral","Date":"Tue, 2 Aug 2022 15:45:28 +0300","To":"Umang Jain ","Message-ID":"","References":"<20220729103846.414819-1-umang.jain@ideasonboard.com>\n\t\n\t\n\t\n\t\n\t<2cf4b2d6-69c9-3c85-24bc-03f1367e2936@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<2cf4b2d6-69c9-3c85-24bc-03f1367e2936@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Provide colorimetry <>\n\tColorSpace mappings","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":"rishikeshdonadkar@gmail.com,\n\tlibcamera devel ,\n\tvedantparanjape160201@gmail.com, nicolas.dufresne@collabora.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24575,"web_url":"https://patchwork.libcamera.org/comment/24575/","msgid":"<07a5bfee4c82840f9bc1db2657eed29329e4117b.camel@collabora.com>","date":"2022-08-15T13:03:48","subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Provide colorimetry <>\n\tColorSpace mappings","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Slowly catching up ;-P\n\nLe lundi 01 août 2022 à 18:01 +0530, Umang Jain a écrit :\n> > \n> > > +\tdefault:\n> > > +\t\tg_error(\"Unsupported colorimetry primaries: %d\",\n> > > colorimetry.primaries);\n> > g_error() seems very harsh, do we really need to abort() if the user\n> > asks for unsupported primaries (or other colorimetry fields below) ?\n> \n> \n> I agree it's harsh - initially I kept as 'warning' but then it was \n> advised that 'warnings' always come with mitigation steps.\n> \n> I guess the only mitigation technique here is, making sure mappings exists!\n\nThe colorimetry.primaries is being set by gst_video_colorimetry_from_string()\nwhich ensure a valid value exists. The reduces the \"unsupported\" surface to\nnewly added colorspace in GStreamer itself, something that only occurs perhaps\nonce every 3 years.\n\nPerhaps to make this less harsh (and aligned with the caller g_critical(\"Invalid\ncolorimetry %s\", colorimetry_caps);), we could change\ncolorspace_from_colorimetry() to return a boolean (success/failure) and have the\ncolorSpace a return parameter ? We could then consistently use g_criticical and\nignore the GstCaps colorimetry field like we'd do if the user had set an invalid\nstring.\n\nNicolas","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 302C0BE173\n\tfor ;\n\tMon, 15 Aug 2022 13:04:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3759161FC0;\n\tMon, 15 Aug 2022 15:04:00 +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 356AE61FB9\n\tfor ;\n\tMon, 15 Aug 2022 15:03:58 +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 CEAFD6601DA8;\n\tMon, 15 Aug 2022 14:03:56 +0100 (BST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660568640;\n\tbh=aYjMq+qPj7/rl0qhXE1xISoAtNz3g2zh7CT61AR+gZ4=;\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=JspimIdajZB3gzBYGSCPnF9qIYU4o017lbed7mndM1/gGadSjBU0p/M7XV0WmMydp\n\t6eK91H8jfuQJM3MAn9vXHeatHDAY9BSiAIPbR8vlTABoa8r8mxUusCrkeBul3cZsHH\n\tp63BCg9jGVEdy9nDzWzLS1YU7v98NAH3X3xnOtgPGeqyPQQCISiE4HHOvVt1kDm5tH\n\t8nawi6/tgMVNoB0ohdhOUw7Sc8kX560pILRxWOiHsEwt2mBYfgEDH4l9+IeLbcOv1l\n\tLIqfbWW/0TEs5ZAOdwvFlAP3Yr3PJ7h/ZHvK9di1knVEhG+SpShAyQl0uyvD2OCPRw\n\tAiUzfHbOiK6cg==","v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com;\n\ts=mail; t=1660568637;\n\tbh=aYjMq+qPj7/rl0qhXE1xISoAtNz3g2zh7CT61AR+gZ4=;\n\th=Subject:From:To:Cc:Date:In-Reply-To:References:From;\n\tb=R+cnqs8KUHEL33OpcbE6xJl+ZQf5r8netTFRKXnuPIrI7hvahV4anXVjyXVfP0MI/\n\tdTdybvgc7yI92jhJXVr351vPOiTyxQIsySY9MtTZ8W/VGA1fS5iuC2E7aOPXlKHxEF\n\tpO/iY19dxvFQeG2vmY2XUdLUwYjanqVvt+BVISOUbsmOY+2FtpRxYZIKOBUaRzxsc1\n\tk3pRpBAOFGJH1KJxphR+4sDONmg1/kTxdBM+/uHLjwB6xlBvo6qt5WF6rA/exyE9+e\n\tUlN4Dde7aEailrq+jFxHnYSFAdAFv1o+KH6Q2LJS0UGyjFhGZUFeEgn9Tiv7z6huto\n\tPairTUsRjlUPQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=collabora.com\n\theader.i=@collabora.com\n\theader.b=\"R+cnqs8K\"; dkim-atps=neutral","Message-ID":"<07a5bfee4c82840f9bc1db2657eed29329e4117b.camel@collabora.com>","To":"Umang Jain , Laurent Pinchart\n\t","Date":"Mon, 15 Aug 2022 09:03:48 -0400","In-Reply-To":"","References":"<20220729103846.414819-1-umang.jain@ideasonboard.com>\n\t\n\t","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","User-Agent":"Evolution 3.44.2 (3.44.2-1.fc36) ","MIME-Version":"1.0","Subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Provide colorimetry <>\n\tColorSpace mappings","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":"rishikeshdonadkar@gmail.com, libcamera-devel@lists.libcamera.org,\n\tvedantparanjape160201@gmail.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24592,"web_url":"https://patchwork.libcamera.org/comment/24592/","msgid":"","date":"2022-08-16T02:47:22","subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Provide colorimetry <>\n\tColorSpace mappings","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nicolas,\n\nOn Mon, Aug 15, 2022 at 09:03:48AM -0400, Nicolas Dufresne wrote:\n> Slowly catching up ;-P\n\n:-)\n\n> Le lundi 01 août 2022 à 18:01 +0530, Umang Jain a écrit :\n> > > \n> > > > +\tdefault:\n> > > > +\t\tg_error(\"Unsupported colorimetry primaries: %d\",\n> > > > colorimetry.primaries);\n> > > g_error() seems very harsh, do we really need to abort() if the user\n> > > asks for unsupported primaries (or other colorimetry fields below) ?\n> > \n> > \n> > I agree it's harsh - initially I kept as 'warning' but then it was \n> > advised that 'warnings' always come with mitigation steps.\n> > \n> > I guess the only mitigation technique here is, making sure mappings exists!\n> \n> The colorimetry.primaries is being set by gst_video_colorimetry_from_string()\n> which ensure a valid value exists. The reduces the \"unsupported\" surface to\n> newly added colorspace in GStreamer itself, something that only occurs perhaps\n> once every 3 years.\n> \n> Perhaps to make this less harsh (and aligned with the caller g_critical(\"Invalid\n> colorimetry %s\", colorimetry_caps);), we could change\n> colorspace_from_colorimetry() to return a boolean (success/failure) and have the\n> colorSpace a return parameter ?\n\nI've reviewed v3 and recommended returning a std::optional\nfrom colorspace_from_colorimetry(), so we had the same idea :-)\n\n> We could then consistently use g_criticical and ignore the GstCaps\n> colorimetry field like we'd do if the user had set an invalid string.\n\nThis I'm not sure to follow though, I don't understand what you mean by\nconsistent usage of g_critical().","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 1F213BE173\n\tfor ;\n\tTue, 16 Aug 2022 02:47:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6C0D561FC0;\n\tTue, 16 Aug 2022 04:47:38 +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 D10A2603E3\n\tfor ;\n\tTue, 16 Aug 2022 04:47:36 +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 2368D496;\n\tTue, 16 Aug 2022 04:47:36 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660618058;\n\tbh=WlNSYKeZygboQxGrYMxUqcdJfI+Dk9psBrm4vidq16w=;\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=K+FvnHTDViCAYE4Jg9mXZPraMk+5it7LlvWRCWqNDq86hJnzr0pfvKmSskJJPzTAb\n\tCFuGr1rGgbRj9zLbr7d+Ff0RhgYGlbLTiGg9O6VojGCO4JTx/pwr0hbpkFZn8JGnhp\n\tDeSXgrho1oGmFLeTljEOw+W00PfZepjRtlVnHBB/9w7GcRczjqZ1E3X/9m43Z3/LHz\n\tx2yI8pPV4pNo2t/N6VMPHzu2NUAdGxv+vz5N0jyoWT8hj11O4Ruu0PjHyAGsZWXWXI\n\thGN9Q9fXonDNsfjhjCsT6+7QqOmK9HK2BbBkc4TwwTWJmlY85ZtAGgDDWNPPEhwg8t\n\t2EFVIn+zprjjw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1660618056;\n\tbh=WlNSYKeZygboQxGrYMxUqcdJfI+Dk9psBrm4vidq16w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WMCBkaA76/CMM/1CMiGLOFurY47xkic24ORHJNzL2ncaggqNoRaffTEtEY67InecA\n\toNtP6AAQPyV/MqtQn+Mkj82XAZwNZelL8DKfEmiAAyBYnrrX0a0Zs9Tyh11jsJ/5C+\n\tfagAY0XeIOncFlDh+51WgHIdqeZhVjGWRI/Nl/E8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"WMCBkaA7\"; dkim-atps=neutral","Date":"Tue, 16 Aug 2022 05:47:22 +0300","To":"Nicolas Dufresne ","Message-ID":"","References":"<20220729103846.414819-1-umang.jain@ideasonboard.com>\n\t\n\t\n\t<07a5bfee4c82840f9bc1db2657eed29329e4117b.camel@collabora.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<07a5bfee4c82840f9bc1db2657eed29329e4117b.camel@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Provide colorimetry <>\n\tColorSpace mappings","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":"rishikeshdonadkar@gmail.com, libcamera-devel@lists.libcamera.org,\n\tvedantparanjape160201@gmail.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24608,"web_url":"https://patchwork.libcamera.org/comment/24608/","msgid":"<3481316e9c781aab3080d0aeadbc1f9e7f737638.camel@collabora.com>","date":"2022-08-16T12:47:01","subject":"Re: [libcamera-devel] [PATCH v2] gstreamer: Provide colorimetry <>\n\tColorSpace mappings","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Le mardi 16 août 2022 à 05:47 +0300, Laurent Pinchart a écrit :\n> Hi Nicolas,\n> \n> On Mon, Aug 15, 2022 at 09:03:48AM -0400, Nicolas Dufresne wrote:\n> > Slowly catching up ;-P\n> \n> :-)\n> \n> > Le lundi 01 août 2022 à 18:01 +0530, Umang Jain a écrit :\n> > > > \n> > > > > +\tdefault:\n> > > > > +\t\tg_error(\"Unsupported colorimetry primaries: %d\",\n> > > > > colorimetry.primaries);\n> > > > g_error() seems very harsh, do we really need to abort() if the user\n> > > > asks for unsupported primaries (or other colorimetry fields below) ?\n> > > \n> > > \n> > > I agree it's harsh - initially I kept as 'warning' but then it was \n> > > advised that 'warnings' always come with mitigation steps.\n> > > \n> > > I guess the only mitigation technique here is, making sure mappings exists!\n> > \n> > The colorimetry.primaries is being set by gst_video_colorimetry_from_string()\n> > which ensure a valid value exists. The reduces the \"unsupported\" surface to\n> > newly added colorspace in GStreamer itself, something that only occurs perhaps\n> > once every 3 years.\n> > \n> > Perhaps to make this less harsh (and aligned with the caller g_critical(\"Invalid\n> > colorimetry %s\", colorimetry_caps);), we could change\n> > colorspace_from_colorimetry() to return a boolean (success/failure) and have the\n> > colorSpace a return parameter ?\n> \n> I've reviewed v3 and recommended returning a std::optional\n> from colorspace_from_colorimetry(), so we had the same idea :-)\n> \n> > We could then consistently use g_criticical and ignore the GstCaps\n> > colorimetry field like we'd do if the user had set an invalid string.\n> \n> This I'm not sure to follow though, I don't understand what you mean by\n> consistent usage of g_critical().\n\nThis function call is always preceded by a call to\ngst_video_colorimetry_from_string(), which returns false if the string is\ninvalid. In that case, we use g_critical(), which is the same thing as an\ng_assert() but without the abort. While g_error() does abort the application.\nThe two errors are highly similar, one will just trace, the other (this one) is\naborting the entire process. I think both should equally just trace (or cleanly\nfail), as this is user error, not a programming error.\n\nregards,\nNicolas\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 4F314BE173\n\tfor ;\n\tTue, 16 Aug 2022 12:47:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 99AF261FC0;\n\tTue, 16 Aug 2022 14:47:14 +0200 (CEST)","from madras.collabora.co.uk (madras.collabora.co.uk\n\t[46.235.227.172])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ECAB761FA9\n\tfor ;\n\tTue, 16 Aug 2022 14:47:12 +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 A626C660037F;\n\tTue, 16 Aug 2022 13:47:10 +0100 (BST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660654034;\n\tbh=ZrGNFcteBtJXv3+zHmMocvwSv7Ur1mkX4/Lnde2Jy/I=;\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=EQGNHCeTfJee1NTMqHC2aafVO/o3uu/n4LRqagQ+TwtJMs2Y7lLQ6dJCqV+v3M4FK\n\trvn9f7aO7Jrff6Bb3ogebJkTLZhTizWXVGfVOUmpuCUB/FCYwMQ9tt3OKiUAcQLzNa\n\t3MecudK9c/tvMzCaOyezlrmGEa+wQx1dNuAoso+4jErAaTOzuAsvGlgvMPmOz5UBFi\n\twYm57Dlu61PNCMWHyRHQtPu4NGe3uET/iqteKufVHrs7FguVWreSLjDhtmvSNbvbNy\n\truXYgXU3xLHqdteJ1p+P8gPWB7LwO12v7wGqx8E3n72bHna2yvvCoSHP56fQba/079\n\tqNwuPLcPU0JLw==","v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com;\n\ts=mail; t=1660654031;\n\tbh=ZrGNFcteBtJXv3+zHmMocvwSv7Ur1mkX4/Lnde2Jy/I=;\n\th=Subject:From:To:Cc:Date:In-Reply-To:References:From;\n\tb=N8GJiP43FTnGy7kbcXgyNH6CAx6o53f0JRkxdnz3/shxhBBnXXcstEBZW20LKE+Zt\n\tPSg2EujHSY6DdzEoghg7E3Fj7ze8PrRxJCNkWHrx5+143EJc2LAwAsge+31StsM/fb\n\tP38CsCnNJ8E/pVypMAERcUg6X43ztNRM0Z2XBqcax/loUzF9wzvkaJG0liYovTdzcs\n\tR+R4Ju3t1+q/prW9pjGunRlCpfFwHa0EOtM7sogYqI87Io9jpLI0AEveU0ElIaDJ6N\n\tyMYtdaCSo+loqnsRHS4b7C+MVE0qE2d6Qk9QWDLELleFjxhJVGQTs/TV8Yoe7n8rZx\n\tnEen9m9jjTBvQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=collabora.com\n\theader.i=@collabora.com\n\theader.b=\"N8GJiP43\"; dkim-atps=neutral","Message-ID":"<3481316e9c781aab3080d0aeadbc1f9e7f737638.camel@collabora.com>","To":"Laurent Pinchart ","Date":"Tue, 16 Aug 2022 08:47:01 -0400","In-Reply-To":"","References":"<20220729103846.414819-1-umang.jain@ideasonboard.com>\n\t\n\t\n\t<07a5bfee4c82840f9bc1db2657eed29329e4117b.camel@collabora.com>\n\t","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 v2] gstreamer: Provide colorimetry <>\n\tColorSpace mappings","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":"rishikeshdonadkar@gmail.com, libcamera-devel@lists.libcamera.org,\n\tvedantparanjape160201@gmail.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]