[{"id":24082,"web_url":"https://patchwork.libcamera.org/comment/24082/","msgid":"","date":"2022-07-24T16:33:04","subject":"Re: [libcamera-devel] [PATCH 3/3] gstreamer: Provide colorimetry <>\n\tlibcamera::ColorSpace mappings","submitter":{"id":118,"url":"https://patchwork.libcamera.org/api/people/118/","name":"Rishikesh Donadkar","email":"rishikeshdonadkar@gmail.com"},"content":"> default:\n> + GST_WARNING(\"Colorspace YcbcrEncoding not mapped in GstLibcameraSrc\");\n> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_UNKNOWN;\n> + }\n\nBack in v1 of colorspace and colorimetry integration nicolas pointed\nout that we should not use the\n_UNKNOWN to set the fields.\nhttps://lists.libcamera.org/pipermail/libcamera-devel/2022-July/031941.html\nI am not sure whether we should set the field to _UNKNOWN in the\ndefault switch cases.\n\n\nOn Sun, Jul 24, 2022 at 8:14 PM Umang Jain wrote:\n>\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> argument. Multiple colorimetry support shall be introduced in\n> subsequent commits.\n>\n> Signed-off-by: Rishikesh Donadkar \n> Signed-off-by: Umang Jain \n> ---\n> src/gstreamer/gstlibcamera-utils.cpp | 175 +++++++++++++++++++++++++++\n> 1 file changed, 175 insertions(+)\n>\n> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> index c97c0d43..fb4f0e5c 100644\n> --- a/src/gstreamer/gstlibcamera-utils.cpp\n> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> @@ -45,6 +45,157 @@ 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> + default:\n> + GST_WARNING(\"ColorSpace primaries not mapped in GstLibcameraSrc\");\n> + colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_UNKNOWN;\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> + default:\n> + GST_WARNING(\"ColorSpace transfer function not mapped in GstLibcameraSrc\");\n> + colorimetry.transfer = GST_VIDEO_TRANSFER_UNKNOWN;\n> + }\n> +\n> + switch (colorSpace.ycbcrEncoding) {\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> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT601;\n> + break;\n> + default:\n> + GST_WARNING(\"Colorspace YcbcrEncoding not mapped in GstLibcameraSrc\");\n> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_UNKNOWN;\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> + default:\n> + GST_WARNING(\"Colorspace range not mapped in GstLibcameraSrc\");\n> + colorimetry.range = GST_VIDEO_COLOR_RANGE_UNKNOWN;\n> + }\n> +\n> + return colorimetry;\n> +}\n> +\n> +static std::optional\n> +colorspace_from_colorimetry(GstVideoColorimetry *colorimetry)\n> +{\n> + std::optional colorspace = ColorSpace::Default;\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> + colorspace->primaries = ColorSpace::Primaries::Default;\n> + break;\n> + default:\n> + GST_WARNING(\"Unknown primaries in colorimetry %d\", colorimetry->primaries);\n> + }\n> +\n> + switch (colorimetry->transfer) {\n> + /* Tranfer 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> + case GST_VIDEO_TRANSFER_UNKNOWN:\n> + colorspace->transferFunction = ColorSpace::TransferFunction::Default;\n> + break;\n> + default:\n> + GST_WARNING(\"Unknown colorimetry transfer %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> + 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> + case GST_VIDEO_COLOR_MATRIX_RGB:\n> + case GST_VIDEO_COLOR_MATRIX_UNKNOWN:\n> + colorspace->ycbcrEncoding = ColorSpace::YcbcrEncoding::Default;\n> + break;\n> + default:\n> + GST_WARNING(\"Unknown 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> + case GST_VIDEO_COLOR_RANGE_UNKNOWN:\n> + colorspace->range = ColorSpace::Range::Default;\n> + break;\n> + default:\n> + GST_WARNING(\"Unknown range in colorimetry %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 +290,17 @@ 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 != nullptr)\n> + gst_structure_set(s, \"colorimetry\", G_TYPE_STRING, colorimetry_str, nullptr);\n> + else\n> + g_warning(\"libcamera::ColorSpace found but GstVideoColorimetry unknown\");\n> + }\n> +\n> gst_caps_append_structure(caps, s);\n>\n> return caps;\n> @@ -222,6 +384,19 @@ 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> + std::optional colorSpace = colorspace_from_colorimetry(&colorimetry);\n> + stream_cfg.colorSpace = colorSpace;\n> + } else {\n> + g_print(\"Invalid colorimetry %s\", colorimetry_caps);\n> + }\n> + }\n> }\n>\n> #if !GST_CHECK_VERSION(1, 17, 1)\n> --\n> 2.31.1\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 D63EDC3275\n\tfor ;\n\tSun, 24 Jul 2022 16:33:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1DB2E63312;\n\tSun, 24 Jul 2022 18:33:18 +0200 (CEST)","from mail-vs1-xe2a.google.com (mail-vs1-xe2a.google.com\n\t[IPv6:2607:f8b0:4864:20::e2a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 08AF86330B\n\tfor ;\n\tSun, 24 Jul 2022 18:33:17 +0200 (CEST)","by mail-vs1-xe2a.google.com with SMTP id t28so2838616vsr.11\n\tfor ;\n\tSun, 24 Jul 2022 09:33:16 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658680398;\n\tbh=mnKEqGteo2sdOIWKuOprRPyRP0/yihaJEdGziGerl0o=;\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=aOMHyYaStrOonAOJrDPw+1E7XoLHCzqP2op1dAuAojjWgZrcdvdetsAY2FPrR5Cal\n\tmexrLshOXRE1bicRXe7T5KGN0XlCgjbRkdbEDWvymVRCo9G8sywO8Ycp1zavcIZNBz\n\ttbAchEJSkfmYjPcSQD1/Wn1LlBr02WCA8xItEBqsP5ZW3t1f4fwWsLqDyuY6kAH3Eq\n\tqMsAOKesDxVPaVd9nKSLotQP4F34pUMcUHdLGOb6k0i3bYTA0lugfwcC6I0LZTFv09\n\t1XYSQ1Z4oDQIiQWqT3nAXrxizZs0UssUwgA7xGJxsCsI3OVgkLvaakvVuyRIig3zR/\n\t/oMNNmVGTKd5g==","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=YBBbPmCZECJdrasGCSRCgv7mUQOWvQFF1nQsrkqi4RA=;\n\tb=AEpnimlC/NQ6ed8IzqfgSycK2BclBWCZi5XRdffPIf/M1AW9KwfDMUbcdTAGRE/q/T\n\t0IdS3XDGS6MiSc09sZ5yklEtJgCbRZYQxePxBcB/dU68Ok986ELwUP08SpJy0b2lJaw8\n\t+Xe8sir3aK1OR6p1HHy6jwZ0b9Rg7v2mjeGEFZXBtUKd0Ip4Ui5XeB3hh9Nu0GSgfe15\n\tQvU8ANGeAqThPljdrHr5Pz0b+H+XyPxvQNeqibA4weq6I1J10WCzeW8zqzqakmyBuCBM\n\tn87VVanZ1NLVVrvfAFLc4WUjcrPbv7/lZAVSuJvDhtd6UbDVd1p2nsZ6VlTKnHKULjy5\n\ttuuQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"AEpnimlC\"; 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=YBBbPmCZECJdrasGCSRCgv7mUQOWvQFF1nQsrkqi4RA=;\n\tb=v1dO/YO7TCqI5sUn4ZcFgOZF2lcyGO3AMHGZ0BYQ7qAy97NGiXJM/DhLXGo5uC0vE/\n\tDxRV3FDGboPASoo+fChXUb+lM6lrjm5OExK/fQkNvTjO4Sa4kQDMRT63vohqTL+Ip+9G\n\t3YHcl1E5cF1Xp+lxArxVCcivOvrDIAX9PdEi7AVzFx4kc90+XwQmcGT0HSMu/Y96vkPN\n\tAVXcxuAd5BiHufh8/Q9+ZNXwSU6Iv4wA76cDZBH51C883Oeij8RA0xpGg70ZXA6Xv1lr\n\t9rQEW1FCxtUBPfnLdF2V9+a2x+70qlp+dmsc/MWXSHdefJO7F6QPf2uw5763cyKOn7LO\n\tTsPg==","X-Gm-Message-State":"AJIora+2tXHvNL4FgZy9iM9pUyfhCxK00bB6gpGo+cadAHV722Zb+ZIC\n\tVdwfCYZHbdb9BGm5VR6iaHWrlLGH43KBqswe8Q4=","X-Google-Smtp-Source":"AGRyM1uXMeGQavzGh6zlbv1IhrsaT9gqDcBLj5hf2YmqZj7fBhpkkh4TFeuk2gaVsBU7Z13VFfsyxNDLg3fk3/P7cNc=","X-Received":"by 2002:a67:d714:0:b0:358:4e8b:423b with SMTP id\n\tp20-20020a67d714000000b003584e8b423bmr1362475vsj.13.1658680395598;\n\tSun, 24 Jul 2022 09:33:15 -0700 (PDT)","MIME-Version":"1.0","References":"<20220724144355.104978-1-umang.jain@ideasonboard.com>\n\t<20220724144355.104978-4-umang.jain@ideasonboard.com>","In-Reply-To":"<20220724144355.104978-4-umang.jain@ideasonboard.com>","Date":"Sun, 24 Jul 2022 22:03:04 +0530","Message-ID":"","To":"Umang Jain ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 3/3] gstreamer: Provide colorimetry <>\n\tlibcamera::ColorSpace 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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24083,"web_url":"https://patchwork.libcamera.org/comment/24083/","msgid":"<82b3d587-fdc4-4032-32a5-22d486d7d8bd@ideasonboard.com>","date":"2022-07-24T18:18:51","subject":"Re: [libcamera-devel] [PATCH 3/3] gstreamer: Provide colorimetry <>\n\tlibcamera::ColorSpace mappings","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Rishi,\n\nOn 7/24/22 22:03, Rishikesh Donadkar wrote:\n>> default:\n>> + GST_WARNING(\"Colorspace YcbcrEncoding not mapped in GstLibcameraSrc\");\n>> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_UNKNOWN;\n>> + }\n> Back in v1 of colorspace and colorimetry integration nicolas pointed\n> out that we should not use the\n> _UNKNOWN to set the fields.\n> https://lists.libcamera.org/pipermail/libcamera-devel/2022-July/031941.html\n> I am not sure whether we should set the field to _UNKNOWN in the\n> default switch cases.\n\n\nThat's what is in happening here. It's a parsing error hence we use \n_UNKNOWN along with a warning.\n\nThis means a identifier exists in the libcamera(probably a new \nidentifier is added to libcamera) but not mapped in gstlibcamerasrc, \nhence, it shall pop the warning and the mapping for the new identifier \nneeds to be added in gstlibcamerasrc-utils :-)\n\nAs discussed on the IRC, we are working on case-by-case basis here, so \nas we get reports of missing identifiers, we shall extend the mappings. \nHence the entire mapping/warnings has been set up that way.\n\n>\n>\n> On Sun, Jul 24, 2022 at 8:14 PM Umang Jain 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>> argument. Multiple colorimetry support shall be introduced in\n>> subsequent commits.\n>>\n>> Signed-off-by: Rishikesh Donadkar \n>> Signed-off-by: Umang Jain \n>> ---\n>> src/gstreamer/gstlibcamera-utils.cpp | 175 +++++++++++++++++++++++++++\n>> 1 file changed, 175 insertions(+)\n>>\n>> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n>> index c97c0d43..fb4f0e5c 100644\n>> --- a/src/gstreamer/gstlibcamera-utils.cpp\n>> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n>> @@ -45,6 +45,157 @@ 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>> + default:\n>> + GST_WARNING(\"ColorSpace primaries not mapped in GstLibcameraSrc\");\n>> + colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_UNKNOWN;\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>> + default:\n>> + GST_WARNING(\"ColorSpace transfer function not mapped in GstLibcameraSrc\");\n>> + colorimetry.transfer = GST_VIDEO_TRANSFER_UNKNOWN;\n>> + }\n>> +\n>> + switch (colorSpace.ycbcrEncoding) {\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>> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT601;\n>> + break;\n>> + default:\n>> + GST_WARNING(\"Colorspace YcbcrEncoding not mapped in GstLibcameraSrc\");\n>> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_UNKNOWN;\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>> + default:\n>> + GST_WARNING(\"Colorspace range not mapped in GstLibcameraSrc\");\n>> + colorimetry.range = GST_VIDEO_COLOR_RANGE_UNKNOWN;\n>> + }\n>> +\n>> + return colorimetry;\n>> +}\n>> +\n>> +static std::optional\n>> +colorspace_from_colorimetry(GstVideoColorimetry *colorimetry)\n>> +{\n>> + std::optional colorspace = ColorSpace::Default;\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>> + colorspace->primaries = ColorSpace::Primaries::Default;\n>> + break;\n>> + default:\n>> + GST_WARNING(\"Unknown primaries in colorimetry %d\", colorimetry->primaries);\n>> + }\n>> +\n>> + switch (colorimetry->transfer) {\n>> + /* Tranfer 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>> + case GST_VIDEO_TRANSFER_UNKNOWN:\n>> + colorspace->transferFunction = ColorSpace::TransferFunction::Default;\n>> + break;\n>> + default:\n>> + GST_WARNING(\"Unknown colorimetry transfer %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>> + 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>> + case GST_VIDEO_COLOR_MATRIX_RGB:\n>> + case GST_VIDEO_COLOR_MATRIX_UNKNOWN:\n>> + colorspace->ycbcrEncoding = ColorSpace::YcbcrEncoding::Default;\n>> + break;\n>> + default:\n>> + GST_WARNING(\"Unknown 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>> + case GST_VIDEO_COLOR_RANGE_UNKNOWN:\n>> + colorspace->range = ColorSpace::Range::Default;\n>> + break;\n>> + default:\n>> + GST_WARNING(\"Unknown range in colorimetry %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 +290,17 @@ 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 != nullptr)\n>> + gst_structure_set(s, \"colorimetry\", G_TYPE_STRING, colorimetry_str, nullptr);\n>> + else\n>> + g_warning(\"libcamera::ColorSpace found but GstVideoColorimetry unknown\");\n>> + }\n>> +\n>> gst_caps_append_structure(caps, s);\n>>\n>> return caps;\n>> @@ -222,6 +384,19 @@ 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>> + std::optional colorSpace = colorspace_from_colorimetry(&colorimetry);\n>> + stream_cfg.colorSpace = colorSpace;\n>> + } else {\n>> + g_print(\"Invalid colorimetry %s\", colorimetry_caps);\n>> + }\n>> + }\n>> }\n>>\n>> #if !GST_CHECK_VERSION(1, 17, 1)\n>> --\n>> 2.31.1\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 51734BE173\n\tfor ;\n\tSun, 24 Jul 2022 18:19:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 87FC663312;\n\tSun, 24 Jul 2022 20:19:00 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7B6596330B\n\tfor ;\n\tSun, 24 Jul 2022 20:18:59 +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 EE3B4898;\n\tSun, 24 Jul 2022 20:18:57 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658686740;\n\tbh=tkYoPYA+ghIxE9xQfhoE20auW0r5JbZ4gvxrUAWwQQQ=;\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=l5lfpCqlXvzkZ1h2uqrGULfczafyXuMlv/tL8Ot+ViW/3AwwLlnw/K+pXK4p5gKju\n\tlbrLHe+o6uC7BdlD2Hx5LxBRh+8exG+lQa0quwAAYbFj5n6pg8endRlXLJssiwTVsv\n\taiFXE1nXt/Fz7UIfxrtpXRPkzk+aK0t4gFBu3gnTtCwkLoQ5Paf5FlSvsDbQKsRFx4\n\t7aTgyvnuCyEi4sTZQvwBlp94mrvuop03NY1Ot0yaKjc902roLVkJ8xpQha1ZHLypRv\n\tBQ2aOa2Hte8zhgr0uJBARptIjP+zm74urKqFySE0+5DoKxnW7/qzyGMAhRrl3oSXLZ\n\tAprsama7JQIYw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1658686739;\n\tbh=tkYoPYA+ghIxE9xQfhoE20auW0r5JbZ4gvxrUAWwQQQ=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=d0AtxcioAfoqUzP1Ho3ckYyne+Sbmd8QAtcNtAYE+S7RMYL8Rk/ukvHIx+TFaM8CD\n\tLKtCbr0p3oE9g8zmG4Az3nbifEtHvia7dNpG3N/rKmHQu9r8i2sbCnFJaAde8Gvr4n\n\tM38kqdsuSILsjqwYYyqas8mMw90jUtwuAPTr/ro0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"d0Atxcio\"; dkim-atps=neutral","Message-ID":"<82b3d587-fdc4-4032-32a5-22d486d7d8bd@ideasonboard.com>","Date":"Sun, 24 Jul 2022 23:48:51 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"Rishikesh Donadkar ","References":"<20220724144355.104978-1-umang.jain@ideasonboard.com>\n\t<20220724144355.104978-4-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 3/3] gstreamer: Provide colorimetry <>\n\tlibcamera::ColorSpace 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":"libcamera-devel@lists.libcamera.org, vedantparanjape160201@gmail.com,\n\tNicolas Dufresne ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24164,"web_url":"https://patchwork.libcamera.org/comment/24164/","msgid":"","date":"2022-07-27T01:17:21","subject":"Re: [libcamera-devel] [PATCH 3/3] gstreamer: Provide colorimetry <>\n\tlibcamera::ColorSpace 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\nThank you for the patch.\n\nOn Sun, Jul 24, 2022 at 08:13:55PM +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> argument. Multiple colorimetry support shall be introduced in\n> subsequent commits.\n> \n> Signed-off-by: Rishikesh Donadkar \n> Signed-off-by: Umang Jain \n> ---\n> src/gstreamer/gstlibcamera-utils.cpp | 175 +++++++++++++++++++++++++++\n> 1 file changed, 175 insertions(+)\n> \n> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> index c97c0d43..fb4f0e5c 100644\n> --- a/src/gstreamer/gstlibcamera-utils.cpp\n> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> @@ -45,6 +45,157 @@ 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\nI would initialize the structure with all fields set to UNKNOWN here...\n\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> +\tdefault:\n> +\t\tGST_WARNING(\"ColorSpace primaries not mapped in GstLibcameraSrc\");\n> +\t\tcolorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_UNKNOWN;\n\n... and drop the default case. As ColorSpace::Primaries is an enum\nclass, if you have cases for all the values, the compiler will warn only\nif one (or more) of the enum values isn't handled in explicit cases.\nThis way you can avoid warnings, and have a guarantee that if the\nlibcamera colorspaces are later extended, the compiler will remind that\nthis function has to be updated. Same for the other components of the\ncolorspace below.\n\n> +\t}\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> +\tdefault:\n> +\t\tGST_WARNING(\"ColorSpace transfer function not mapped in GstLibcameraSrc\");\n> +\t\tcolorimetry.transfer = GST_VIDEO_TRANSFER_UNKNOWN;\n> +\t}\n> +\n> +\tswitch (colorSpace.ycbcrEncoding) {\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\tcolorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT601;\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tGST_WARNING(\"Colorspace YcbcrEncoding not mapped in GstLibcameraSrc\");\n> +\t\tcolorimetry.matrix = GST_VIDEO_COLOR_MATRIX_UNKNOWN;\n\nShould we map YcbcrEncoding::None to GST_VIDEO_COLOR_MATRIX_RGB ?\nOtherwise capturing RGB or raw bayer will produce a warning. Same for\nthe Raw primaries, it seems that the best mapping would be\nGST_VIDEO_COLOR_PRIMARIES_UNKNOWN (unless I'm missing something), but I\ndon't think we should warn in that case.\n\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> +\tdefault:\n> +\t\tGST_WARNING(\"Colorspace range not mapped in GstLibcameraSrc\");\n> +\t\tcolorimetry.range = GST_VIDEO_COLOR_RANGE_UNKNOWN;\n> +\t}\n> +\n> +\treturn colorimetry;\n> +}\n> +\n> +static std::optional\n> +colorspace_from_colorimetry(GstVideoColorimetry *colorimetry)\n\nI'd pass a const reference instead of a pointer, as there's no use case\nfor a null colorimetry.\n\n> +{\n> +\tstd::optional colorspace = ColorSpace::Default;\n\nNo need for std::optional here or in the return type.\n\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\tcolorspace->primaries = ColorSpace::Primaries::Default;\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tGST_WARNING(\"Unknown primaries in colorimetry %d\", colorimetry->primaries);\n> +\t}\n> +\n> +\tswitch (colorimetry->transfer) {\n> +\t/* Tranfer 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> +\tcase GST_VIDEO_TRANSFER_UNKNOWN:\n> +\t\tcolorspace->transferFunction = ColorSpace::TransferFunction::Default;\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tGST_WARNING(\"Unknown colorimetry transfer %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\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> +\tcase GST_VIDEO_COLOR_MATRIX_RGB:\n\nShouldn't this map to YcbcrEncoding::None ?\n\n> +\tcase GST_VIDEO_COLOR_MATRIX_UNKNOWN:\n> +\t\tcolorspace->ycbcrEncoding = ColorSpace::YcbcrEncoding::Default;\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tGST_WARNING(\"Unknown 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> +\tcase GST_VIDEO_COLOR_RANGE_UNKNOWN:\n> +\t\tcolorspace->range = ColorSpace::Range::Default;\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tGST_WARNING(\"Unknown range in colorimetry %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 +290,17 @@ 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 != nullptr)\n\n\t\tif (colorimetry_str)\n\n> +\t\t\tgst_structure_set(s, \"colorimetry\", G_TYPE_STRING, colorimetry_str, nullptr);\n> +\t\telse\n> +\t\t\tg_warning(\"libcamera::ColorSpace found but GstVideoColorimetry unknown\");\n> +\t}\n> +\n> \tgst_caps_append_structure(caps, s);\n> \n> \treturn caps;\n> @@ -222,6 +384,19 @@ 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\tstd::optional colorSpace = colorspace_from_colorimetry(&colorimetry);\n> +\t\t\tstream_cfg.colorSpace = colorSpace;\n\n\t\t\tstream_cfg.colorSpace = colorspace_from_colorimetry(&colorimetry);\n\n> +\t\t} else {\n> +\t\t\tg_print(\"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 9A59ABE173\n\tfor ;\n\tWed, 27 Jul 2022 01:17:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 97EE563312;\n\tWed, 27 Jul 2022 03:17:24 +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 E409B603E8\n\tfor ;\n\tWed, 27 Jul 2022 03:17:23 +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 31A0756D;\n\tWed, 27 Jul 2022 03:17:23 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658884644;\n\tbh=8HHmkkxG6KiwwyoINMmk8F9f/AHcCP5+1yiyc39lCQ0=;\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=e1hVQ6d8j/DlpEo2o8aLGjcpn7dDmpMOMzAPijQ9cCyYSQ4b0epi6K8K6+wPiK9Ru\n\txyK9LHT1Ysu3i41Kdi2ItFhwTYKlz39J0Ltvd2x39l+KQ7Yj12Nkkt4aUDJ4vWgzYL\n\tg7RX+fXHP0QoIfnd+z6nYeYtcpJUBkohDXpiAh5GpRpFzF+EbxY6emuHJjajqm8m8s\n\tZHSovUMKwGG9yrP2RZHKbOifn/KwwsocVkb0cy1AMxuQena185WHekp+gIdvXyChTo\n\trybTwc/gI86ZWQqyGXWEVCBj3hPSXj2xy+JHVOEi/kpKlveX9/4wJ0pw8t9LAPZUW/\n\tNt4rNvju1aVNw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1658884643;\n\tbh=8HHmkkxG6KiwwyoINMmk8F9f/AHcCP5+1yiyc39lCQ0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=h59RXlEopYpGgi/zHt01i8Yw3RrGxHtaLW8JG+yl3Q+iWF3WgzeC3H2m7/JkRqVf7\n\txoIJBQ+lYTrISm2utsl+/ay9pq8w1It8IfH6zJH59WiLoylmzeCYW+6WE0dJfdHjvp\n\tbfFZQ2VJeF4O+qth0XTGFCiw9R3zIw1e3+Ij2WYY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"h59RXlEo\"; dkim-atps=neutral","Date":"Wed, 27 Jul 2022 04:17:21 +0300","To":"Umang Jain ","Message-ID":"","References":"<20220724144355.104978-1-umang.jain@ideasonboard.com>\n\t<20220724144355.104978-4-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220724144355.104978-4-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 3/3] gstreamer: Provide colorimetry <>\n\tlibcamera::ColorSpace 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":24192,"web_url":"https://patchwork.libcamera.org/comment/24192/","msgid":"<70c69471052d32ec870c7a91cbf9f10a0eefa714.camel@collabora.com>","date":"2022-07-27T14:48:35","subject":"Re: [libcamera-devel] [PATCH 3/3] gstreamer: Provide colorimetry <>\n\tlibcamera::ColorSpace mappings","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Le dimanche 24 juillet 2022 à 22:03 +0530, Rishikesh Donadkar via libcamera-\ndevel a écrit :\n> > default:\n> > + GST_WARNING(\"Colorspace YcbcrEncoding not mapped in\n> > GstLibcameraSrc\");\n> > + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_UNKNOWN;\n> > + }\n> \n> Back in v1 of colorspace and colorimetry integration nicolas pointed\n> out that we should not use the\n> _UNKNOWN to set the fields.\n> https://lists.libcamera.org/pipermail/libcamera-devel/2022-July/031941.html\n> I am not sure whether we should set the field to _UNKNOWN in the\n> default switch cases.\n\nIts perverse, since the use of a WARNING would mean you want to mitigate the\nmissing mapping, but in some case it will fail to negotiate. I suggest to post\nan error (perhaps GST_STREAM_ERROR_NOT_IMPLEMENTED ?) Or properly mitigate it.\n\nIf this should just never be reached, then there is g_assert_not_reached() or\ng_error() that will abort the program.\n\n> \n> \n> On Sun, Jul 24, 2022 at 8:14 PM Umang Jain \n> wrote:\n> > \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> > argument. Multiple colorimetry support shall be introduced in\n> > subsequent commits.\n> > \n> > Signed-off-by: Rishikesh Donadkar \n> > Signed-off-by: Umang Jain \n> > ---\n> >  src/gstreamer/gstlibcamera-utils.cpp | 175 +++++++++++++++++++++++++++\n> >  1 file changed, 175 insertions(+)\n> > \n> > diff --git a/src/gstreamer/gstlibcamera-utils.cpp\n> > b/src/gstreamer/gstlibcamera-utils.cpp\n> > index c97c0d43..fb4f0e5c 100644\n> > --- a/src/gstreamer/gstlibcamera-utils.cpp\n> > +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> > @@ -45,6 +45,157 @@ static struct {\n> >         /* \\todo NV42 is used in libcamera but is not mapped in GStreamer\n> > 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> > + default:\n> > + GST_WARNING(\"ColorSpace primaries not mapped in\n> > GstLibcameraSrc\");\n> > + colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_UNKNOWN;\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> > + default:\n> > + GST_WARNING(\"ColorSpace transfer function not mapped in\n> > GstLibcameraSrc\");\n> > + colorimetry.transfer = GST_VIDEO_TRANSFER_UNKNOWN;\n> > + }\n> > +\n> > + switch (colorSpace.ycbcrEncoding) {\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> > + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT601;\n> > + break;\n> > + default:\n> > + GST_WARNING(\"Colorspace YcbcrEncoding not mapped in\n> > GstLibcameraSrc\");\n> > + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_UNKNOWN;\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> > + default:\n> > + GST_WARNING(\"Colorspace range not mapped in\n> > GstLibcameraSrc\");\n> > + colorimetry.range = GST_VIDEO_COLOR_RANGE_UNKNOWN;\n> > + }\n> > +\n> > + return colorimetry;\n> > +}\n> > +\n> > +static std::optional\n> > +colorspace_from_colorimetry(GstVideoColorimetry *colorimetry)\n> > +{\n> > + std::optional colorspace = ColorSpace::Default;\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> > + colorspace->primaries = ColorSpace::Primaries::Default;\n> > + break;\n> > + default:\n> > + GST_WARNING(\"Unknown primaries in colorimetry %d\",\n> > colorimetry->primaries);\n> > + }\n> > +\n> > + switch (colorimetry->transfer) {\n> > + /* Tranfer function mappings inspired from v4l2src plugin */\n> > + case GST_VIDEO_TRANSFER_GAMMA18:\n> > + case GST_VIDEO_TRANSFER_GAMMA20:\n> > + case GST_VIDEO_TRANSFER_GAMMA22:\n> > + case GST_VIDEO_TRANSFER_GAMMA28:\n> > + GST_WARNING(\"GAMMA 18, 20, 22, 28 transfer functions not\n> > supported\");\n> > + /* fallthrough */\n> > + case GST_VIDEO_TRANSFER_GAMMA10:\n> > + colorspace->transferFunction =\n> > ColorSpace::TransferFunction::Linear;\n> > + break;\n> > + case GST_VIDEO_TRANSFER_BT601:\n> > + case GST_VIDEO_TRANSFER_BT2020_12:\n> > + case GST_VIDEO_TRANSFER_BT2020_10:\n> > + case GST_VIDEO_TRANSFER_BT709:\n> > + colorspace->transferFunction =\n> > ColorSpace::TransferFunction::Rec709;\n> > + break;\n> > + case GST_VIDEO_TRANSFER_SRGB:\n> > + colorspace->transferFunction =\n> > ColorSpace::TransferFunction::Srgb;\n> > + break;\n> > + case GST_VIDEO_TRANSFER_UNKNOWN:\n> > + colorspace->transferFunction =\n> > ColorSpace::TransferFunction::Default;\n> > + break;\n> > + default:\n> > + GST_WARNING(\"Unknown colorimetry transfer %d\", colorimetry-\n> > >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> > + colorspace->ycbcrEncoding =\n> > ColorSpace::YcbcrEncoding::Rec601;\n> > + break;\n> > + case GST_VIDEO_COLOR_MATRIX_BT709:\n> > + colorspace->ycbcrEncoding =\n> > ColorSpace::YcbcrEncoding::Rec709;\n> > + break;\n> > + case GST_VIDEO_COLOR_MATRIX_BT2020:\n> > + colorspace->ycbcrEncoding =\n> > ColorSpace::YcbcrEncoding::Rec2020;\n> > + break;\n> > + case GST_VIDEO_COLOR_MATRIX_RGB:\n> > + case GST_VIDEO_COLOR_MATRIX_UNKNOWN:\n> > + colorspace->ycbcrEncoding =\n> > ColorSpace::YcbcrEncoding::Default;\n> > + break;\n> > + default:\n> > + GST_WARNING(\"Unknown colorimetry matrix %d\", colorimetry-\n> > >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> > + case GST_VIDEO_COLOR_RANGE_UNKNOWN:\n> > + colorspace->range = ColorSpace::Range::Default;\n> > + break;\n> > + default:\n> > + GST_WARNING(\"Unknown range in colorimetry %d\", colorimetry-\n> > >range);\n> > + }\n> > +\n> > + return colorspace;\n> > +}\n> > +\n> >  static GstVideoFormat\n> >  pixel_format_to_gst_format(const PixelFormat &format)\n> >  {\n> > @@ -139,6 +290,17 @@ gst_libcamera_stream_configuration_to_caps(const\n> > 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 =\n> > colorimetry_from_colorspace(stream_cfg.colorSpace.value());\n> > + gchar *colorimetry_str =\n> > gst_video_colorimetry_to_string(&colorimetry);\n> > +\n> > + if (colorimetry_str != nullptr)\n> > + gst_structure_set(s, \"colorimetry\", G_TYPE_STRING,\n> > colorimetry_str, nullptr);\n> > + else\n> > + g_warning(\"libcamera::ColorSpace found but\n> > GstVideoColorimetry unknown\");\n> > + }\n> > +\n> >         gst_caps_append_structure(caps, s);\n> > \n> >         return caps;\n> > @@ -222,6 +384,19 @@\n> > gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,\n> >         gst_structure_get_int(s, \"height\", &height);\n> >         stream_cfg.size.width = width;\n> >         stream_cfg.size.height = height;\n> > +\n> > + /* Configure colorimetry */\n> > + if (gst_structure_has_field(s, \"colorimetry\")) {\n> > + const gchar *colorimetry_caps = gst_structure_get_string(s,\n> > \"colorimetry\");\n> > + GstVideoColorimetry colorimetry;\n> > +\n> > + if(gst_video_colorimetry_from_string(&colorimetry,\n> > colorimetry_caps)) {\n> > + std::optional colorSpace =\n> > colorspace_from_colorimetry(&colorimetry);\n> > + stream_cfg.colorSpace = colorSpace;\n> > + } else {\n> > + g_print(\"Invalid colorimetry %s\", colorimetry_caps);\n> > + }\n> > + }\n> >  }\n> > \n> >  #if !GST_CHECK_VERSION(1, 17, 1)\n> > --\n> > 2.31.1\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 AF73EC3275\n\tfor ;\n\tWed, 27 Jul 2022 14:48:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0CF5C63311;\n\tWed, 27 Jul 2022 16:48:47 +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 EFE2263309\n\tfor ;\n\tWed, 27 Jul 2022 16:48:45 +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 DD3B76601ABD;\n\tWed, 27 Jul 2022 15:48:44 +0100 (BST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658933327;\n\tbh=U+X4r4fL7pJkfyyH4H9wnP7iuwIhDs6B3aZRL1e/2jc=;\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=NLFTQ1gYOxWFBN8IY3NW0Cpj72KT9yuPAtWbV74V4JWOimH9BHXXSe6UTt6Z1Vp2H\n\tT+6upEUEsjFgYuETq8nxU5+A3tfJR89FnR5dZn2l5KW06oqJ7mYbEGeCF9FDqAzIzJ\n\tvqVnKpJWesP+N5pQVtEQuqfMQGhDnEc7hxU1HgLeuyOo8hEtmta7Z1wg8AGElZrZ+R\n\tlhOAexTUy5EEoTmPeTgP8iXs7l3DjnxchIXzt2WAcfXo5I6it97fD59aR0tBbIToFm\n\t5AZDhkqeXfMPoxD1lKR/C9JUFJeftAPh1a1JlwGSROKgf5O+Lm3D076IPSlWtj4Z1p\n\tzMtSlWaiRqV3Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com;\n\ts=mail; t=1658933325;\n\tbh=U+X4r4fL7pJkfyyH4H9wnP7iuwIhDs6B3aZRL1e/2jc=;\n\th=Subject:From:To:Cc:Date:In-Reply-To:References:From;\n\tb=ThxwK80mDSXLT5I2aeCPKBKlN73ojYwJF4uVRQQ7vk9IITnP/zpeWXuLUXqXhd/7l\n\tiqkcviL9vuPKTek8FOuKLyyhdSrDM1Zu4iLgPhDraCTzzfcE4Gl9ZPRhldggKwXYhC\n\tPgJiMFFQdBv+7DYZpg++GeyKB7MquxzdWCZUxPXEmVt+zsjg1AgIz+CxeABPE88V5Z\n\tX6sw5yXWKDTGVsfreHYQItNFLuiQ/J3MFw5WOisR9lpYvx/8A+PlMKB7VZFQ0BtWmJ\n\tSh/+TfLUPXnpPpskU0SiwLfDaSfDgrd7a+f7nRT/Upb1SOQPpcb31JDTH3BmkTKjvV\n\tPLKBiE0Bv09ew=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=collabora.com\n\theader.i=@collabora.com\n\theader.b=\"ThxwK80m\"; dkim-atps=neutral","Message-ID":"<70c69471052d32ec870c7a91cbf9f10a0eefa714.camel@collabora.com>","To":"Rishikesh Donadkar , Umang Jain\n\t","Date":"Wed, 27 Jul 2022 10:48:35 -0400","In-Reply-To":"","References":"<20220724144355.104978-1-umang.jain@ideasonboard.com>\n\t<20220724144355.104978-4-umang.jain@ideasonboard.com>\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 3/3] gstreamer: Provide colorimetry <>\n\tlibcamera::ColorSpace 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":"libcamera-devel@lists.libcamera.org, vedantparanjape160201@gmail.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24193,"web_url":"https://patchwork.libcamera.org/comment/24193/","msgid":"","date":"2022-07-27T14:50:24","subject":"Re: [libcamera-devel] [PATCH 3/3] gstreamer: Provide colorimetry <>\n\tlibcamera::ColorSpace mappings","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Le dimanche 24 juillet 2022 à 23:48 +0530, Umang Jain via libcamera-devel a\nécrit :\n> Hi Rishi,\n> \n> On 7/24/22 22:03, Rishikesh Donadkar wrote:\n> > > default:\n> > > + GST_WARNING(\"Colorspace YcbcrEncoding not mapped in GstLibcameraSrc\");\n> > > + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_UNKNOWN;\n> > > + }\n> > Back in v1 of colorspace and colorimetry integration nicolas pointed\n> > out that we should not use the\n> > _UNKNOWN to set the fields.\n> > https://lists.libcamera.org/pipermail/libcamera-devel/2022-July/031941.html\n> > I am not sure whether we should set the field to _UNKNOWN in the\n> > default switch cases.\n> \n> \n> That's what is in happening here. It's a parsing error hence we use \n> _UNKNOWN along with a warning.\n> \n> This means a identifier exists in the libcamera(probably a new \n> identifier is added to libcamera) but not mapped in gstlibcamerasrc, \n> hence, it shall pop the warning and the mapping for the new identifier \n> needs to be added in gstlibcamerasrc-utils :-)\n> \n> As discussed on the IRC, we are working on case-by-case basis here, so \n> as we get reports of missing identifiers, we shall extend the mappings. \n> Hence the entire mapping/warnings has been set up that way.\n\nPerhaps you should add some more info to the trace in you want to \"get reports\nof missing identifiers\" ?\n\nAs said in previous answers, WARNING should come with proper mitigation, UNKNOWN\nmay lead to a pipeline error later, as its an invalid value.\n\n> > \n> > \n> > On Sun, Jul 24, 2022 at 8:14 PM Umang Jain 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> > > argument. Multiple colorimetry support shall be introduced in\n> > > subsequent commits.\n> > > \n> > > Signed-off-by: Rishikesh Donadkar \n> > > Signed-off-by: Umang Jain \n> > > ---\n> > > src/gstreamer/gstlibcamera-utils.cpp | 175 +++++++++++++++++++++++++++\n> > > 1 file changed, 175 insertions(+)\n> > > \n> > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> > > index c97c0d43..fb4f0e5c 100644\n> > > --- a/src/gstreamer/gstlibcamera-utils.cpp\n> > > +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> > > @@ -45,6 +45,157 @@ 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> > > + default:\n> > > + GST_WARNING(\"ColorSpace primaries not mapped in GstLibcameraSrc\");\n> > > + colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_UNKNOWN;\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> > > + default:\n> > > + GST_WARNING(\"ColorSpace transfer function not mapped in GstLibcameraSrc\");\n> > > + colorimetry.transfer = GST_VIDEO_TRANSFER_UNKNOWN;\n> > > + }\n> > > +\n> > > + switch (colorSpace.ycbcrEncoding) {\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> > > + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT601;\n> > > + break;\n> > > + default:\n> > > + GST_WARNING(\"Colorspace YcbcrEncoding not mapped in GstLibcameraSrc\");\n> > > + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_UNKNOWN;\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> > > + default:\n> > > + GST_WARNING(\"Colorspace range not mapped in GstLibcameraSrc\");\n> > > + colorimetry.range = GST_VIDEO_COLOR_RANGE_UNKNOWN;\n> > > + }\n> > > +\n> > > + return colorimetry;\n> > > +}\n> > > +\n> > > +static std::optional\n> > > +colorspace_from_colorimetry(GstVideoColorimetry *colorimetry)\n> > > +{\n> > > + std::optional colorspace = ColorSpace::Default;\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> > > + colorspace->primaries = ColorSpace::Primaries::Default;\n> > > + break;\n> > > + default:\n> > > + GST_WARNING(\"Unknown primaries in colorimetry %d\", colorimetry->primaries);\n> > > + }\n> > > +\n> > > + switch (colorimetry->transfer) {\n> > > + /* Tranfer 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> > > + case GST_VIDEO_TRANSFER_UNKNOWN:\n> > > + colorspace->transferFunction = ColorSpace::TransferFunction::Default;\n> > > + break;\n> > > + default:\n> > > + GST_WARNING(\"Unknown colorimetry transfer %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> > > + 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> > > + case GST_VIDEO_COLOR_MATRIX_RGB:\n> > > + case GST_VIDEO_COLOR_MATRIX_UNKNOWN:\n> > > + colorspace->ycbcrEncoding = ColorSpace::YcbcrEncoding::Default;\n> > > + break;\n> > > + default:\n> > > + GST_WARNING(\"Unknown 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> > > + case GST_VIDEO_COLOR_RANGE_UNKNOWN:\n> > > + colorspace->range = ColorSpace::Range::Default;\n> > > + break;\n> > > + default:\n> > > + GST_WARNING(\"Unknown range in colorimetry %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 +290,17 @@ 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 != nullptr)\n> > > + gst_structure_set(s, \"colorimetry\", G_TYPE_STRING, colorimetry_str, nullptr);\n> > > + else\n> > > + g_warning(\"libcamera::ColorSpace found but GstVideoColorimetry unknown\");\n> > > + }\n> > > +\n> > > gst_caps_append_structure(caps, s);\n> > > \n> > > return caps;\n> > > @@ -222,6 +384,19 @@ 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> > > + std::optional colorSpace = colorspace_from_colorimetry(&colorimetry);\n> > > + stream_cfg.colorSpace = colorSpace;\n> > > + } else {\n> > > + g_print(\"Invalid colorimetry %s\", colorimetry_caps);\n> > > + }\n> > > + }\n> > > }\n> > > \n> > > #if !GST_CHECK_VERSION(1, 17, 1)\n> > > --\n> > > 2.31.1\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 4090DC3275\n\tfor ;\n\tWed, 27 Jul 2022 14:50:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A41EE63311;\n\tWed, 27 Jul 2022 16:50:36 +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 14D7563309\n\tfor ;\n\tWed, 27 Jul 2022 16:50:35 +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\t(No client certificate requested) (Authenticated sender: nicolas)\n\tby madras.collabora.co.uk (Postfix) with ESMTPSA id 235FA6601ABD;\n\tWed, 27 Jul 2022 15:50:34 +0100 (BST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658933436;\n\tbh=Zz5/1bh/e8o3TMdYVR7EQHX+7JCwkaArfaI9UrpDdjc=;\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=r3l4lNiT/Q62TCP1M0icQuZaNr0oOHI0pCPrFVT5i7zKTNdTqyj3N53xd06XdpnCB\n\trohWXsux+6xD5hfuEv8sGdG4uaqqPx+IlJxK42E4poxZVCVI29W8KtiW67vuLJUSmU\n\tq76RUx02LMb8+cIH2t82NNnVCdj0AAmSPCIViqEF6BMoed+ZX6MCbMlpotY0Anpy4V\n\tj7iY72yRmilGfY39gb+Huvw7EMiiItTfMAn1s3HvMnLHHUCLkrn1yciqfKW1wDmWSD\n\tbj4zc5mEXK2wEo6Lbdw0JOTr/4frJLB6ZbxqmdLnJzCdoQ9DSPaXUJsAR7uHjCGP3E\n\tn8LDnxcxqymEg==","v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com;\n\ts=mail; t=1658933434;\n\tbh=Zz5/1bh/e8o3TMdYVR7EQHX+7JCwkaArfaI9UrpDdjc=;\n\th=Subject:From:To:Cc:Date:In-Reply-To:References:From;\n\tb=gZIsqhqDzDu38Hf6tg4wXRSxcSxjTppHg46ADVMQIspTKjwGMR2lkkI7Jj54aK6Vq\n\tsVzpr3v43SjC+uU98uynXxfpyVGNctW+QCmUod6dNCX73Ee15b2Rhza+jpkty4U/E3\n\tY/TaowmZyN22xuEnGGnLibniaA+2Y4al7ppjR+Q3N0No1Xpb4i6gTzhNRYU8rJ1s5I\n\te1P9aZi/lN9mq9UepER6AeMoVzP8asVNqmGMkBEWXtqx20DJmdO1UD1hZM0EPWS182\n\tI/wHj2R9KyZ/TJ36ZNhmttiq+tBzfM04MTZYvOo7/Db65n69Kf0AQIxN8mduCxl8ex\n\tWWNvl/vlLY09w=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=collabora.com\n\theader.i=@collabora.com\n\theader.b=\"gZIsqhqD\"; dkim-atps=neutral","Message-ID":"","To":"Umang Jain , Rishikesh Donadkar\n\t","Date":"Wed, 27 Jul 2022 10:50:24 -0400","In-Reply-To":"<82b3d587-fdc4-4032-32a5-22d486d7d8bd@ideasonboard.com>","References":"<20220724144355.104978-1-umang.jain@ideasonboard.com>\n\t<20220724144355.104978-4-umang.jain@ideasonboard.com>\n\t\n\t<82b3d587-fdc4-4032-32a5-22d486d7d8bd@ideasonboard.com>","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 3/3] gstreamer: Provide colorimetry <>\n\tlibcamera::ColorSpace 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":"libcamera-devel@lists.libcamera.org, vedantparanjape160201@gmail.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24238,"web_url":"https://patchwork.libcamera.org/comment/24238/","msgid":"<09f1366c-5088-3daa-c31e-4f5597af2180@ideasonboard.com>","date":"2022-07-29T08:06:06","subject":"Re: [libcamera-devel] [PATCH 3/3] gstreamer: Provide colorimetry <>\n\tlibcamera::ColorSpace mappings","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nMy anaylsis on sRGB mapping ...\n\nOn 7/27/22 06:47, Laurent Pinchart wrote:\n> Hi Umang and Rishikesh,\n>\n> Thank you for the patch.\n>\n> On Sun, Jul 24, 2022 at 08:13:55PM +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>> argument. Multiple colorimetry support shall be introduced in\n>> subsequent commits.\n>>\n>> Signed-off-by: Rishikesh Donadkar \n>> Signed-off-by: Umang Jain \n>> ---\n>> src/gstreamer/gstlibcamera-utils.cpp | 175 +++++++++++++++++++++++++++\n>> 1 file changed, 175 insertions(+)\n>>\n>> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n>> index c97c0d43..fb4f0e5c 100644\n>> --- a/src/gstreamer/gstlibcamera-utils.cpp\n>> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n>> @@ -45,6 +45,157 @@ 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> I would initialize the structure with all fields set to UNKNOWN here...\n>\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>> +\tdefault:\n>> +\t\tGST_WARNING(\"ColorSpace primaries not mapped in GstLibcameraSrc\");\n>> +\t\tcolorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_UNKNOWN;\n> ... and drop the default case. As ColorSpace::Primaries is an enum\n> class, if you have cases for all the values, the compiler will warn only\n> if one (or more) of the enum values isn't handled in explicit cases.\n> This way you can avoid warnings, and have a guarantee that if the\n> libcamera colorspaces are later extended, the compiler will remind that\n> this function has to be updated. Same for the other components of the\n> colorspace below.\n>\n>> +\t}\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>> +\tdefault:\n>> +\t\tGST_WARNING(\"ColorSpace transfer function not mapped in GstLibcameraSrc\");\n>> +\t\tcolorimetry.transfer = GST_VIDEO_TRANSFER_UNKNOWN;\n>> +\t}\n>> +\n>> +\tswitch (colorSpace.ycbcrEncoding) {\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\tcolorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT601;\n>> +\t\tbreak;\n>> +\tdefault:\n>> +\t\tGST_WARNING(\"Colorspace YcbcrEncoding not mapped in GstLibcameraSrc\");\n>> +\t\tcolorimetry.matrix = GST_VIDEO_COLOR_MATRIX_UNKNOWN;\n> Should we map YcbcrEncoding::None to GST_VIDEO_COLOR_MATRIX_RGB ?\n\n\nSee below.\n\n> Otherwise capturing RGB or raw bayer will produce a warning. Same for\n> the Raw primaries, it seems that the best mapping would be\n> GST_VIDEO_COLOR_PRIMARIES_UNKNOWN (unless I'm missing something), but I\n> don't think we should warn in that case.\n>\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>> +\tdefault:\n>> +\t\tGST_WARNING(\"Colorspace range not mapped in GstLibcameraSrc\");\n>> +\t\tcolorimetry.range = GST_VIDEO_COLOR_RANGE_UNKNOWN;\n>> +\t}\n>> +\n>> +\treturn colorimetry;\n>> +}\n>> +\n>> +static std::optional\n>> +colorspace_from_colorimetry(GstVideoColorimetry *colorimetry)\n> I'd pass a const reference instead of a pointer, as there's no use case\n> for a null colorimetry.\n>\n>> +{\n>> +\tstd::optional colorspace = ColorSpace::Default;\n> No need for std::optional here or in the return type.\n>\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\tcolorspace->primaries = ColorSpace::Primaries::Default;\n>> +\t\tbreak;\n>> +\tdefault:\n>> +\t\tGST_WARNING(\"Unknown primaries in colorimetry %d\", colorimetry->primaries);\n>> +\t}\n>> +\n>> +\tswitch (colorimetry->transfer) {\n>> +\t/* Tranfer 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>> +\tcase GST_VIDEO_TRANSFER_UNKNOWN:\n>> +\t\tcolorspace->transferFunction = ColorSpace::TransferFunction::Default;\n>> +\t\tbreak;\n>> +\tdefault:\n>> +\t\tGST_WARNING(\"Unknown colorimetry transfer %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\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>> +\tcase GST_VIDEO_COLOR_MATRIX_RGB:\n> Shouldn't this map to YcbcrEncoding::None ?\n\n\nYcbcrEncoding for sRGB as per Kernel is V4L2_YCBCR_ENC_SYCC which is \ndeprecated so V4L2_YCBCR_ENC_601  is used instead [1]\n\nThe same has been applied *already* in libcamera codebase:\n\nconst ColorSpace ColorSpace::Srgb = {\n         Primaries::Rec709,\n         TransferFunction::Srgb,\n         YcbcrEncoding::Rec601,\n         Range::Limited\n};\n\nSo we shouldn't map GST_VIDEO_COLOR_MATRIX_RGB to YcbcrEncoding::None \nbut to Rec601 instead.\n\nNow two question arises for reverse mapping it i.e. what should:\n\n    a) YcbcrEncding::None map to\n\n    b) YcbcrEncding::Rec601 map to.\n\nFor b) we have two contenders:\n\n     GST_VIDEO_COLOR_MATRIX_BT601\n     GST_VIDEO_COLOR_MATRIX_RGB\n\nFor a) I think GST_VIDEO_COLOR_MATRIX_RGB would be better ?\n\n[1] \nhttps://git.linuxtv.org/media_tree.git/tree/include/uapi/linux/videodev2.h#n344\n\n>\n>> +\tcase GST_VIDEO_COLOR_MATRIX_UNKNOWN:\n>> +\t\tcolorspace->ycbcrEncoding = ColorSpace::YcbcrEncoding::Default;\n>> +\t\tbreak;\n>> +\tdefault:\n>> +\t\tGST_WARNING(\"Unknown 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>> +\tcase GST_VIDEO_COLOR_RANGE_UNKNOWN:\n>> +\t\tcolorspace->range = ColorSpace::Range::Default;\n>> +\t\tbreak;\n>> +\tdefault:\n>> +\t\tGST_WARNING(\"Unknown range in colorimetry %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 +290,17 @@ 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 != nullptr)\n> \t\tif (colorimetry_str)\n>\n>> +\t\t\tgst_structure_set(s, \"colorimetry\", G_TYPE_STRING, colorimetry_str, nullptr);\n>> +\t\telse\n>> +\t\t\tg_warning(\"libcamera::ColorSpace found but GstVideoColorimetry unknown\");\n>> +\t}\n>> +\n>> \tgst_caps_append_structure(caps, s);\n>> \n>> \treturn caps;\n>> @@ -222,6 +384,19 @@ 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\tstd::optional colorSpace = colorspace_from_colorimetry(&colorimetry);\n>> +\t\t\tstream_cfg.colorSpace = colorSpace;\n> \t\t\tstream_cfg.colorSpace = colorspace_from_colorimetry(&colorimetry);\n>\n>> +\t\t} else {\n>> +\t\t\tg_print(\"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 4851BBE173\n\tfor ;\n\tFri, 29 Jul 2022 08:06:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7C90D63312;\n\tFri, 29 Jul 2022 10:06:15 +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 7B57460486\n\tfor ;\n\tFri, 29 Jul 2022 10:06:13 +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 AC5D6415;\n\tFri, 29 Jul 2022 10:06:11 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659081975;\n\tbh=blsIv+fozR/Zs2axEBx0cysWlqHruOa7EU7lRoKSWkk=;\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=rVd+MMkMvY6JKaQ6407bVvTyjVBRrvg9QTDqI8mBR+PtpG+Blk8COidwirAmMUIna\n\teGGvMcY3bEsT8BRydvUoKRAG93KxWpVPiLrvhBl03eyzkwJSwhbZBXf1WXQBfg15fO\n\tVNsVmiX/tae+a8CpXHaYIHFx8QCZ3UZgZ8SWP5LjZR+qRT95YJJ8bRtrbr5nXbIfAl\n\t8SKisihnlZ4+TP84Ivv9D3qddQYdiTt0frDYCiP5ZCuyW2eJLZldhzkvkjtGqPxjuI\n\tNGsO5HAS619hhavYhTIlAw+9QrhURT4g5D0kjdghsmHaIOGf3sZDkEVdtCEvmGU2sY\n\tHWkx3BmdDI2Ng==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1659081972;\n\tbh=blsIv+fozR/Zs2axEBx0cysWlqHruOa7EU7lRoKSWkk=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=GbBhP/LOFDUQhtYce61gVFfNffuAMf5I4kqVYUNeLUfCJMuBxu75BvIEUB+gmIccy\n\tNbeFRz+qyRtkmUuw4VvgaokxZU/8SJGVTORSafsuCQedcE91GcrINgr5vNs5DKt1HB\n\tSbv1wbjQQWofk65lGd0J/qxONYK30/B0IL/US7bk="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"GbBhP/LO\"; dkim-atps=neutral","Message-ID":"<09f1366c-5088-3daa-c31e-4f5597af2180@ideasonboard.com>","Date":"Fri, 29 Jul 2022 13:36:06 +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":"<20220724144355.104978-1-umang.jain@ideasonboard.com>\n\t<20220724144355.104978-4-umang.jain@ideasonboard.com>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 3/3] gstreamer: Provide colorimetry <>\n\tlibcamera::ColorSpace 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\tnicolas.dufresne@collabora.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24239,"web_url":"https://patchwork.libcamera.org/comment/24239/","msgid":"<69342c1c-af1f-5451-4d0d-61693facc56c@ideasonboard.com>","date":"2022-07-29T10:10:52","subject":"Re: [libcamera-devel] [PATCH 3/3] gstreamer: Provide colorimetry <>\n\tlibcamera::ColorSpace mappings","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hello,\n\nOn 7/29/22 13:36, Umang Jain via libcamera-devel wrote:\n> Hi Laurent,\n>\n> My anaylsis on sRGB mapping ...\n>\n> On 7/27/22 06:47, Laurent Pinchart wrote:\n>> Hi Umang and Rishikesh,\n>>\n>> Thank you for the patch.\n>>\n>> On Sun, Jul 24, 2022 at 08:13:55PM +0530, Umang Jain via \n>> 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>>> argument. Multiple colorimetry support shall be introduced in\n>>> subsequent commits.\n>>>\n>>> Signed-off-by: Rishikesh Donadkar \n>>> Signed-off-by: Umang Jain \n>>> ---\n>>>   src/gstreamer/gstlibcamera-utils.cpp | 175 \n>>> +++++++++++++++++++++++++++\n>>>   1 file changed, 175 insertions(+)\n>>>\n>>> diff --git a/src/gstreamer/gstlibcamera-utils.cpp \n>>> b/src/gstreamer/gstlibcamera-utils.cpp\n>>> index c97c0d43..fb4f0e5c 100644\n>>> --- a/src/gstreamer/gstlibcamera-utils.cpp\n>>> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n>>> @@ -45,6 +45,157 @@ static struct {\n>>>       /* \\todo NV42 is used in libcamera but is not mapped in \n>>> GStreamer yet. */\n>>>   };\n>>>   +static GstVideoColorimetry\n>>> +colorimetry_from_colorspace(const ColorSpace &colorSpace)\n>>> +{\n>>> +    GstVideoColorimetry colorimetry;\n>> I would initialize the structure with all fields set to UNKNOWN here...\n>>\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>>> +    default:\n>>> +        GST_WARNING(\"ColorSpace primaries not mapped in \n>>> GstLibcameraSrc\");\n>>> +        colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_UNKNOWN;\n>> ... and drop the default case. As ColorSpace::Primaries is an enum\n>> class, if you have cases for all the values, the compiler will warn only\n>> if one (or more) of the enum values isn't handled in explicit cases.\n>> This way you can avoid warnings, and have a guarantee that if the\n>> libcamera colorspaces are later extended, the compiler will remind that\n>> this function has to be updated. Same for the other components of the\n>> colorspace below.\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>>> +    default:\n>>> +        GST_WARNING(\"ColorSpace transfer function not mapped in \n>>> GstLibcameraSrc\");\n>>> +        colorimetry.transfer = GST_VIDEO_TRANSFER_UNKNOWN;\n>>> +    }\n>>> +\n>>> +    switch (colorSpace.ycbcrEncoding) {\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>>> +        colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT601;\n>>> +        break;\n>>> +    default:\n>>> +        GST_WARNING(\"Colorspace YcbcrEncoding not mapped in \n>>> GstLibcameraSrc\");\n>>> +        colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_UNKNOWN;\n>> Should we map YcbcrEncoding::None to GST_VIDEO_COLOR_MATRIX_RGB ?\n>\n>\n> See below.\n>\n>> Otherwise capturing RGB or raw bayer will produce a warning. Same for\n>> the Raw primaries, it seems that the best mapping would be\n>> GST_VIDEO_COLOR_PRIMARIES_UNKNOWN (unless I'm missing something), but I\n>> don't think we should warn in that case.\n>>\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>>> +    default:\n>>> +        GST_WARNING(\"Colorspace range not mapped in GstLibcameraSrc\");\n>>> +        colorimetry.range = GST_VIDEO_COLOR_RANGE_UNKNOWN;\n>>> +    }\n>>> +\n>>> +    return colorimetry;\n>>> +}\n>>> +\n>>> +static std::optional\n>>> +colorspace_from_colorimetry(GstVideoColorimetry *colorimetry)\n>> I'd pass a const reference instead of a pointer, as there's no use case\n>> for a null colorimetry.\n>>\n>>> +{\n>>> +    std::optional colorspace = ColorSpace::Default;\n>> No need for std::optional here or in the return type.\n>>\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>>> +        colorspace->primaries = ColorSpace::Primaries::Default;\n>>> +        break;\n>>> +    default:\n>>> +        GST_WARNING(\"Unknown primaries in colorimetry %d\", \n>>> colorimetry->primaries);\n>>> +    }\n>>> +\n>>> +    switch (colorimetry->transfer) {\n>>> +    /* Tranfer function mappings inspired from v4l2src plugin */\n>>> +    case GST_VIDEO_TRANSFER_GAMMA18:\n>>> +    case GST_VIDEO_TRANSFER_GAMMA20:\n>>> +    case GST_VIDEO_TRANSFER_GAMMA22:\n>>> +    case GST_VIDEO_TRANSFER_GAMMA28:\n>>> +        GST_WARNING(\"GAMMA 18, 20, 22, 28 transfer functions not \n>>> supported\");\n>>> +    /* fallthrough */\n>>> +    case GST_VIDEO_TRANSFER_GAMMA10:\n>>> +        colorspace->transferFunction = \n>>> ColorSpace::TransferFunction::Linear;\n>>> +        break;\n>>> +    case GST_VIDEO_TRANSFER_BT601:\n>>> +    case GST_VIDEO_TRANSFER_BT2020_12:\n>>> +    case GST_VIDEO_TRANSFER_BT2020_10:\n>>> +    case GST_VIDEO_TRANSFER_BT709:\n>>> +        colorspace->transferFunction = \n>>> ColorSpace::TransferFunction::Rec709;\n>>> +        break;\n>>> +    case GST_VIDEO_TRANSFER_SRGB:\n>>> +        colorspace->transferFunction = \n>>> ColorSpace::TransferFunction::Srgb;\n>>> +        break;\n>>> +    case GST_VIDEO_TRANSFER_UNKNOWN:\n>>> +        colorspace->transferFunction = \n>>> ColorSpace::TransferFunction::Default;\n>>> +        break;\n>>> +    default:\n>>> +        GST_WARNING(\"Unknown colorimetry transfer %d\", \n>>> 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>>> +        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 = \n>>> ColorSpace::YcbcrEncoding::Rec2020;\n>>> +        break;\n>>> +    case GST_VIDEO_COLOR_MATRIX_RGB:\n>> Shouldn't this map to YcbcrEncoding::None ?\n>\n>\n> YcbcrEncoding for sRGB as per Kernel is V4L2_YCBCR_ENC_SYCC which is \n> deprecated so V4L2_YCBCR_ENC_601  is used instead [1]\n>\n> The same has been applied *already* in libcamera codebase:\n>\n> const ColorSpace ColorSpace::Srgb = {\n>         Primaries::Rec709,\n>         TransferFunction::Srgb,\n>         YcbcrEncoding::Rec601,\n>         Range::Limited\n> };\n>\n> So we shouldn't map GST_VIDEO_COLOR_MATRIX_RGB to YcbcrEncoding::None \n> but to Rec601 instead.\n>\n> Now two question arises for reverse mapping it i.e. what should:\n>\n>    a) YcbcrEncding::None map to\n>\n>    b) YcbcrEncding::Rec601 map to.\n>\n> For b) we have two contenders:\n>\n>     GST_VIDEO_COLOR_MATRIX_BT601\n>     GST_VIDEO_COLOR_MATRIX_RGB\n\n\nb) is currently handled with:\n\n+       case ColorSpace::YcbcrEncoding::Rec601:\n+               if (colorSpace == ColorSpace::Srgb)\n+                       colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_RGB;\n+               else\n+                       colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT601;\n+               break;\n\nLet's review this in v2 (submitting shortly)\n\n>\n> For a) I think GST_VIDEO_COLOR_MATRIX_RGB would be better ?\n>\n> [1] \n> https://git.linuxtv.org/media_tree.git/tree/include/uapi/linux/videodev2.h#n344\n>\n>>\n>>> +    case GST_VIDEO_COLOR_MATRIX_UNKNOWN:\n>>> +        colorspace->ycbcrEncoding = \n>>> ColorSpace::YcbcrEncoding::Default;\n>>> +        break;\n>>> +    default:\n>>> +        GST_WARNING(\"Unknown colorimetry matrix %d\", \n>>> 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>>> +    case GST_VIDEO_COLOR_RANGE_UNKNOWN:\n>>> +        colorspace->range = ColorSpace::Range::Default;\n>>> +        break;\n>>> +    default:\n>>> +        GST_WARNING(\"Unknown range in colorimetry %d\", \n>>> 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 +290,17 @@ \n>>> gst_libcamera_stream_configuration_to_caps(const StreamConfiguration \n>>> &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 = \n>>> colorimetry_from_colorspace(stream_cfg.colorSpace.value());\n>>> +        gchar *colorimetry_str = \n>>> gst_video_colorimetry_to_string(&colorimetry);\n>>> +\n>>> +        if (colorimetry_str != nullptr)\n>>         if (colorimetry_str)\n>>\n>>> +            gst_structure_set(s, \"colorimetry\", G_TYPE_STRING, \n>>> colorimetry_str, nullptr);\n>>> +        else\n>>> +            g_warning(\"libcamera::ColorSpace found but \n>>> GstVideoColorimetry unknown\");\n>>> +    }\n>>> +\n>>>       gst_caps_append_structure(caps, s);\n>>>         return caps;\n>>> @@ -222,6 +384,19 @@ \n>>> gst_libcamera_configure_stream_from_caps(StreamConfiguration \n>>> &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, \n>>> \"colorimetry\");\n>>> +        GstVideoColorimetry colorimetry;\n>>> +\n>>> + if(gst_video_colorimetry_from_string(&colorimetry, \n>>> colorimetry_caps)) {\n>> Missing space after \"if\".\n>>\n>>> + std::optional colorSpace = \n>>> colorspace_from_colorimetry(&colorimetry);\n>>> +            stream_cfg.colorSpace = colorSpace;\n>>             stream_cfg.colorSpace = \n>> colorspace_from_colorimetry(&colorimetry);\n>>\n>>> +        } else {\n>>> +            g_print(\"Invalid colorimetry %s\", colorimetry_caps);\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 26FDDBE173\n\tfor ;\n\tFri, 29 Jul 2022 10:11:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5775763312;\n\tFri, 29 Jul 2022 12:11:02 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 28DF9603EC\n\tfor ;\n\tFri, 29 Jul 2022 12:11:01 +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 D0B7952F;\n\tFri, 29 Jul 2022 12:10:58 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659089462;\n\tbh=WzxTFPprnsLTZ6FHTE8ucU25bmUjc2sUEKvLikrpE/Q=;\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=08jqGfYgLBzEkhpOZyDIOkpOAAV11rOt4H9p5fYtR7l+M9rtTevJcxaZ4HLLTW7Gr\n\ty3M4oGsWbW/UBBwSaAkrX9/Xq1SJnpAdDN7sVjn7cyzV9HVFKk3nMVlmVoDcop47tb\n\tzLnqBuTBkSw6JIzUHMxfs7tUXxZz6b8GmGXFj0aHm348l79mIWaVJ/RrvGIVU8rew1\n\t7OhAtAHKI+vA3cygEBfEriBQFndouLFSyCK74DqEpGj7rTelRirfEyx5K+SzXUB99R\n\tY/GWbvxjewqcztbvfeiiZ6/A+zjbB5e/VaNGEA9Il9WQQlBLwft1RdNb/9rDeHwdu7\n\twA0O0GGupUW5A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1659089460;\n\tbh=WzxTFPprnsLTZ6FHTE8ucU25bmUjc2sUEKvLikrpE/Q=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=YKDzBP3cDdKC+s6JYvM+fPhNZDYDOMoIO9GxpvTQWvAKH6Ly5sQgaiB7iY0cvYT8R\n\tIk6fezMB/gYUgvI2THpMpknV6STnXyYvfJWCEhOTGYSBgIfN0x2VnAG1dDMyWWNjZB\n\tHvbe92/ZcVky2l/R+ZD5K5EVMwYjYuz3W+0JlwRs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"YKDzBP3c\"; dkim-atps=neutral","Message-ID":"<69342c1c-af1f-5451-4d0d-61693facc56c@ideasonboard.com>","Date":"Fri, 29 Jul 2022 15:40:52 +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":"<20220724144355.104978-1-umang.jain@ideasonboard.com>\n\t<20220724144355.104978-4-umang.jain@ideasonboard.com>\n\t\n\t<09f1366c-5088-3daa-c31e-4f5597af2180@ideasonboard.com>","In-Reply-To":"<09f1366c-5088-3daa-c31e-4f5597af2180@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 3/3] gstreamer: Provide colorimetry <>\n\tlibcamera::ColorSpace 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\tnicolas.dufresne@collabora.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24243,"web_url":"https://patchwork.libcamera.org/comment/24243/","msgid":"","date":"2022-07-29T14:54:43","subject":"Re: [libcamera-devel] [PATCH 3/3] gstreamer: Provide colorimetry <>\n\tlibcamera::ColorSpace 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 Fri, Jul 29, 2022 at 03:40:52PM +0530, Umang Jain wrote:\n> On 7/29/22 13:36, Umang Jain via libcamera-devel wrote:\n> > Hi Laurent,\n> >\n> > My anaylsis on sRGB mapping ...\n> >\n> > On 7/27/22 06:47, Laurent Pinchart wrote:\n> >> Hi Umang and Rishikesh,\n> >>\n> >> Thank you for the patch.\n> >>\n> >> On Sun, Jul 24, 2022 at 08:13:55PM +0530, Umang Jain via \n> >> 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> >>> argument. Multiple colorimetry support shall be introduced in\n> >>> subsequent commits.\n> >>>\n> >>> Signed-off-by: Rishikesh Donadkar \n> >>> Signed-off-by: Umang Jain \n> >>> ---\n> >>>   src/gstreamer/gstlibcamera-utils.cpp | 175 +++++++++++++++++++++++++++\n> >>>   1 file changed, 175 insertions(+)\n> >>>\n> >>> diff --git a/src/gstreamer/gstlibcamera-utils.cpp \n> >>> b/src/gstreamer/gstlibcamera-utils.cpp\n> >>> index c97c0d43..fb4f0e5c 100644\n> >>> --- a/src/gstreamer/gstlibcamera-utils.cpp\n> >>> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> >>> @@ -45,6 +45,157 @@ static struct {\n> >>>       /* \\todo NV42 is used in libcamera but is not mapped in GStreamer yet. */\n> >>>   };\n> >>>   +static GstVideoColorimetry\n> >>> +colorimetry_from_colorspace(const ColorSpace &colorSpace)\n> >>> +{\n> >>> +    GstVideoColorimetry colorimetry;\n> >> \n> >> I would initialize the structure with all fields set to UNKNOWN here...\n> >>\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> >>> +    default:\n> >>> +        GST_WARNING(\"ColorSpace primaries not mapped in \n> >>> GstLibcameraSrc\");\n> >>> +        colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_UNKNOWN;\n> >> \n> >> ... and drop the default case. As ColorSpace::Primaries is an enum\n> >> class, if you have cases for all the values, the compiler will warn only\n> >> if one (or more) of the enum values isn't handled in explicit cases.\n> >> This way you can avoid warnings, and have a guarantee that if the\n> >> libcamera colorspaces are later extended, the compiler will remind that\n> >> this function has to be updated. Same for the other components of the\n> >> colorspace below.\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> >>> +    default:\n> >>> +        GST_WARNING(\"ColorSpace transfer function not mapped in GstLibcameraSrc\");\n> >>> +        colorimetry.transfer = GST_VIDEO_TRANSFER_UNKNOWN;\n> >>> +    }\n> >>> +\n> >>> +    switch (colorSpace.ycbcrEncoding) {\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> >>> +        colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT601;\n> >>> +        break;\n> >>> +    default:\n> >>> +        GST_WARNING(\"Colorspace YcbcrEncoding not mapped in GstLibcameraSrc\");\n> >>> +        colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_UNKNOWN;\n> >> \n> >> Should we map YcbcrEncoding::None to GST_VIDEO_COLOR_MATRIX_RGB ?\n> >\n> > See below.\n> >\n> >> Otherwise capturing RGB or raw bayer will produce a warning. Same for\n> >> the Raw primaries, it seems that the best mapping would be\n> >> GST_VIDEO_COLOR_PRIMARIES_UNKNOWN (unless I'm missing something), but I\n> >> don't think we should warn in that case.\n> >>\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> >>> +    default:\n> >>> +        GST_WARNING(\"Colorspace range not mapped in GstLibcameraSrc\");\n> >>> +        colorimetry.range = GST_VIDEO_COLOR_RANGE_UNKNOWN;\n> >>> +    }\n> >>> +\n> >>> +    return colorimetry;\n> >>> +}\n> >>> +\n> >>> +static std::optional\n> >>> +colorspace_from_colorimetry(GstVideoColorimetry *colorimetry)\n> >> \n> >> I'd pass a const reference instead of a pointer, as there's no use case\n> >> for a null colorimetry.\n> >>\n> >>> +{\n> >>> +    std::optional colorspace = ColorSpace::Default;\n> >> \n> >> No need for std::optional here or in the return type.\n> >>\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> >>> +        colorspace->primaries = ColorSpace::Primaries::Default;\n> >>> +        break;\n> >>> +    default:\n> >>> +        GST_WARNING(\"Unknown primaries in colorimetry %d\", colorimetry->primaries);\n> >>> +    }\n> >>> +\n> >>> +    switch (colorimetry->transfer) {\n> >>> +    /* Tranfer 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> >>> +    case GST_VIDEO_TRANSFER_UNKNOWN:\n> >>> +        colorspace->transferFunction = ColorSpace::TransferFunction::Default;\n> >>> +        break;\n> >>> +    default:\n> >>> +        GST_WARNING(\"Unknown colorimetry transfer %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> >>> +        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> >>> +    case GST_VIDEO_COLOR_MATRIX_RGB:\n> >> \n> >> Shouldn't this map to YcbcrEncoding::None ?\n> >\n> > YcbcrEncoding for sRGB as per Kernel is V4L2_YCBCR_ENC_SYCC which is \n> > deprecated so V4L2_YCBCR_ENC_601  is used instead [1]\n> >\n> > The same has been applied *already* in libcamera codebase:\n> >\n> > const ColorSpace ColorSpace::Srgb = {\n> >         Primaries::Rec709,\n> >         TransferFunction::Srgb,\n> >         YcbcrEncoding::Rec601,\n> >         Range::Limited\n> > };\n> >\n> > So we shouldn't map GST_VIDEO_COLOR_MATRIX_RGB to YcbcrEncoding::None \n> > but to Rec601 instead.\n\nI'm not sure that's right. GST_VIDEO_COLOR_MATRIX_RGB is defined as \n\nGST_VIDEO_COLOR_MATRIX_RGB (1) – identity matrix. Order of coefficients\nis actually GBR, also IEC 61966-2-1 (sRGB) \n\nI understand this as meaning that the RGB data is not converted to YUV,\nwhich is expressed in libcamera as YcbcrEncoding::None:\n\n * \\var ColorSpace::YcbcrEncoding::None\n * \\brief There is no defined Y'CbCr encoding (used for non-YUV formats)\n\nThe V4L2 sRGB colorspace [2] definition is confusing. sRGB is an RGB\ncolorspace, so it should have no YUV encoding, and use full range. The\nsYCC colorspace is sRGB encoded in YUV using the Rec601 encoding, also\nusing full range. As it's typical for devices that produce sRGB images\nto encode them to YUV using the Rec601 encoding and limited range, the\nV4L2 sRGB colorspace defines default encoding and quantization range as\nRec601 and limited. However, when the pixels are stored as RGB, those\ntwo parameters don't apply.\n\nNote also how GStreamer defines the SRGB colorspace ([3]):\n\n MAKE_COLORIMETRY (SRGB, _0_255, RGB, SRGB, BT709),\n\nWe could do the same and define sRGB as\n\nconst ColorSpace ColorSpace::Srgb = {\n        Primaries::Rec709,\n        TransferFunction::Srgb,\n        YcbcrEncoding::None,\n        Range::Full\n};\n\nbut we would then need to adapt code that produce YUV-encoded sRGB. We\ncould define, for that purpose,\n\nconst ColorSpace ColorSpace::Sycc = {\n        Primaries::Rec709,\n        TransferFunction::Srgb,\n        YcbcrEncoding::Rec601,\n        Range::Full\n};\n\nbut that still wouldn't match the V4L2 definition. It's not necessarily\na problem though, devices can produce { Rec709, Srgb, Rec601, Limited }\nwithout the need to explicitly define a ColorSpace preset in libcamera.\nI've alos just realized that the above is identical to ColorSpace::Jpeg.\n\nAnother option would be to keep the ColorSpace::Srgb definition as-is\nand document that the YCbCrEncoding and Range must be ignored for\nnon-YUV formats, but I don't like that much, it sounds confusing.\n\nI'm tempted to bite the bullet and define ColorSpace::Srgb with\nYcbcrEncoding::None and Range::Full, and never return ColorSpace::Srgb\nwhen capturing YUV data. A pipeline handler that gets ColorSpace::Srgb\nfrom an application would turn it into { Rec709, Srgb, Rec601, Limited }\nfor YUV streams (assuming that the hardware supports that of course).\n\nDavid, how is ColorSpace::Srgb interpreted by the Raspberry Pi ISP when\nproducing YUV data ?\n\n[2] https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/colorspaces-details.html#colorspace-srgb-v4l2-colorspace-srgb\n[3] https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/main/subprojects/gst-plugins-base/gst-libs/gst/video/video-color.c#L66\n\n> > Now two question arises for reverse mapping it i.e. what should:\n> >\n> >    a) YcbcrEncding::None map to\n> >\n> >    b) YcbcrEncding::Rec601 map to.\n> >\n> > For b) we have two contenders:\n> >\n> >     GST_VIDEO_COLOR_MATRIX_BT601\n> >     GST_VIDEO_COLOR_MATRIX_RGB\n> \n> b) is currently handled with:\n> \n> +       case ColorSpace::YcbcrEncoding::Rec601:\n> +               if (colorSpace == ColorSpace::Srgb)\n> +                       colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_RGB;\n> +               else\n> +                       colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT601;\n> +               break;\n> \n> Let's review this in v2 (submitting shortly)\n\nAs per the above, I think YcbcrEncding::None should map to\nGST_VIDEO_COLOR_MATRIX_RGB and YcbcrEncding::Rec601 to\nGST_VIDEO_COLOR_MATRIX_BT601, unconditionally.\n\n> > For a) I think GST_VIDEO_COLOR_MATRIX_RGB would be better ?\n> >\n> > [1] https://git.linuxtv.org/media_tree.git/tree/include/uapi/linux/videodev2.h#n344\n> >\n> >>> +    case GST_VIDEO_COLOR_MATRIX_UNKNOWN:\n> >>> +        colorspace->ycbcrEncoding = ColorSpace::YcbcrEncoding::Default;\n> >>> +        break;\n> >>> +    default:\n> >>> +        GST_WARNING(\"Unknown 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> >>> +    case GST_VIDEO_COLOR_RANGE_UNKNOWN:\n> >>> +        colorspace->range = ColorSpace::Range::Default;\n> >>> +        break;\n> >>> +    default:\n> >>> +        GST_WARNING(\"Unknown range in colorimetry %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 +290,17 @@ 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 != nullptr)\n> >> \n> >>         if (colorimetry_str)\n> >>\n> >>> +            gst_structure_set(s, \"colorimetry\", G_TYPE_STRING, colorimetry_str, nullptr);\n> >>> +        else\n> >>> +            g_warning(\"libcamera::ColorSpace found but GstVideoColorimetry unknown\");\n> >>> +    }\n> >>> +\n> >>>       gst_caps_append_structure(caps, s);\n> >>>         return caps;\n> >>> @@ -222,6 +384,19 @@ 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> >>> + std::optional colorSpace = colorspace_from_colorimetry(&colorimetry);\n> >>> +            stream_cfg.colorSpace = colorSpace;\n> >> \n> >>             stream_cfg.colorSpace = colorspace_from_colorimetry(&colorimetry);\n> >>\n> >>> +        } else {\n> >>> +            g_print(\"Invalid colorimetry %s\", colorimetry_caps);\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 6829FC3275\n\tfor ;\n\tFri, 29 Jul 2022 14:54:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BBC9363312;\n\tFri, 29 Jul 2022 16:54:47 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A4FBC603EC\n\tfor ;\n\tFri, 29 Jul 2022 16:54:46 +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 F1A486D4;\n\tFri, 29 Jul 2022 16:54:45 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659106487;\n\tbh=QX68wLIJvDZgcFmHifxRnQlc3TOE5UpPrnbuex0uRuw=;\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=hoqnf5XF8eWhGcFLhAn5zle/SGuzdBaSW/lzPYXRwCouw61+nPT6fU+Hs70oIP5OU\n\tpHYWs5DnT8F8FXkqfXm11Z84iviiiuh9vshBu3AUQc0EC7mVzJjr+Ka9jaq1Ekd5aZ\n\tVridlQjQmDXnUuBcsFPKAcqwr70sLeSXDw8xy/18BYZMxrOa9T5h4wkLHACvD8+53g\n\tTS1wlkofwLO46nrxbv/br53hEjwUi1Vqhhy+VNPMGWqbY1rpncX45V8WljCfk/NDq+\n\t3510ud8NmkczsMrC6Je/XByOEHQ1zQSkP42/9Ec5zo2r8CpX7oYrmcQOIfh8hNrhr3\n\tRcNK9MfGO6Qhg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1659106486;\n\tbh=QX68wLIJvDZgcFmHifxRnQlc3TOE5UpPrnbuex0uRuw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZGoRf4UpyLbjcRDP1wS1P2OkHmgOlgnyVD0F2h8wBR4lpoURAQw0Y3VUzQxkLXBBp\n\tqAyLV81OQ2918zJs8sq4UFZiYojtAU4UN00Sf3Sep6ogboJXFixSgKYiU+ZXEZqjb/\n\tx/BHG7CIL04Lnt/xv00nR2peXeU0jJFykYwuzqrk="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ZGoRf4Up\"; dkim-atps=neutral","Date":"Fri, 29 Jul 2022 17:54:43 +0300","To":"Umang Jain ","Message-ID":"","References":"<20220724144355.104978-1-umang.jain@ideasonboard.com>\n\t<20220724144355.104978-4-umang.jain@ideasonboard.com>\n\t\n\t<09f1366c-5088-3daa-c31e-4f5597af2180@ideasonboard.com>\n\t<69342c1c-af1f-5451-4d0d-61693facc56c@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<69342c1c-af1f-5451-4d0d-61693facc56c@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 3/3] gstreamer: Provide colorimetry <>\n\tlibcamera::ColorSpace 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\tnicolas.dufresne@collabora.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24244,"web_url":"https://patchwork.libcamera.org/comment/24244/","msgid":"","date":"2022-07-29T15:15:20","subject":"Re: [libcamera-devel] [PATCH 3/3] gstreamer: Provide colorimetry <>\n\tlibcamera::ColorSpace mappings","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Le vendredi 29 juillet 2022 à 17:54 +0300, Laurent Pinchart a écrit :\n> Hi Umang,\n> \n> On Fri, Jul 29, 2022 at 03:40:52PM +0530, Umang Jain wrote:\n> > On 7/29/22 13:36, Umang Jain via libcamera-devel wrote:\n> > > Hi Laurent,\n> > > \n> > > My anaylsis on sRGB mapping ...\n> > > \n> > > On 7/27/22 06:47, Laurent Pinchart wrote:\n> > > > Hi Umang and Rishikesh,\n> > > > \n> > > > Thank you for the patch.\n> > > > \n> > > > On Sun, Jul 24, 2022 at 08:13:55PM +0530, Umang Jain via \n> > > > 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> > > > > argument. Multiple colorimetry support shall be introduced in\n> > > > > subsequent commits.\n> > > > > \n> > > > > Signed-off-by: Rishikesh Donadkar \n> > > > > Signed-off-by: Umang Jain \n> > > > > ---\n> > > > >   src/gstreamer/gstlibcamera-utils.cpp | 175 +++++++++++++++++++++++++++\n> > > > >   1 file changed, 175 insertions(+)\n> > > > > \n> > > > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp \n> > > > > b/src/gstreamer/gstlibcamera-utils.cpp\n> > > > > index c97c0d43..fb4f0e5c 100644\n> > > > > --- a/src/gstreamer/gstlibcamera-utils.cpp\n> > > > > +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> > > > > @@ -45,6 +45,157 @@ static struct {\n> > > > >       /* \\todo NV42 is used in libcamera but is not mapped in GStreamer yet. */\n> > > > >   };\n> > > > >   +static GstVideoColorimetry\n> > > > > +colorimetry_from_colorspace(const ColorSpace &colorSpace)\n> > > > > +{\n> > > > > +    GstVideoColorimetry colorimetry;\n> > > > \n> > > > I would initialize the structure with all fields set to UNKNOWN here...\n> > > > \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> > > > > +    default:\n> > > > > +        GST_WARNING(\"ColorSpace primaries not mapped in \n> > > > > GstLibcameraSrc\");\n> > > > > +        colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_UNKNOWN;\n> > > > \n> > > > ... and drop the default case. As ColorSpace::Primaries is an enum\n> > > > class, if you have cases for all the values, the compiler will warn only\n> > > > if one (or more) of the enum values isn't handled in explicit cases.\n> > > > This way you can avoid warnings, and have a guarantee that if the\n> > > > libcamera colorspaces are later extended, the compiler will remind that\n> > > > this function has to be updated. Same for the other components of the\n> > > > colorspace below.\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> > > > > +    default:\n> > > > > +        GST_WARNING(\"ColorSpace transfer function not mapped in GstLibcameraSrc\");\n> > > > > +        colorimetry.transfer = GST_VIDEO_TRANSFER_UNKNOWN;\n> > > > > +    }\n> > > > > +\n> > > > > +    switch (colorSpace.ycbcrEncoding) {\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> > > > > +        colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT601;\n> > > > > +        break;\n> > > > > +    default:\n> > > > > +        GST_WARNING(\"Colorspace YcbcrEncoding not mapped in GstLibcameraSrc\");\n> > > > > +        colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_UNKNOWN;\n> > > > \n> > > > Should we map YcbcrEncoding::None to GST_VIDEO_COLOR_MATRIX_RGB ?\n> > > \n> > > See below.\n> > > \n> > > > Otherwise capturing RGB or raw bayer will produce a warning. Same for\n> > > > the Raw primaries, it seems that the best mapping would be\n> > > > GST_VIDEO_COLOR_PRIMARIES_UNKNOWN (unless I'm missing something), but I\n> > > > don't think we should warn in that case.\n> > > > \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> > > > > +    default:\n> > > > > +        GST_WARNING(\"Colorspace range not mapped in GstLibcameraSrc\");\n> > > > > +        colorimetry.range = GST_VIDEO_COLOR_RANGE_UNKNOWN;\n> > > > > +    }\n> > > > > +\n> > > > > +    return colorimetry;\n> > > > > +}\n> > > > > +\n> > > > > +static std::optional\n> > > > > +colorspace_from_colorimetry(GstVideoColorimetry *colorimetry)\n> > > > \n> > > > I'd pass a const reference instead of a pointer, as there's no use case\n> > > > for a null colorimetry.\n> > > > \n> > > > > +{\n> > > > > +    std::optional colorspace = ColorSpace::Default;\n> > > > \n> > > > No need for std::optional here or in the return type.\n> > > > \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> > > > > +        colorspace->primaries = ColorSpace::Primaries::Default;\n> > > > > +        break;\n> > > > > +    default:\n> > > > > +        GST_WARNING(\"Unknown primaries in colorimetry %d\", colorimetry->primaries);\n> > > > > +    }\n> > > > > +\n> > > > > +    switch (colorimetry->transfer) {\n> > > > > +    /* Tranfer 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> > > > > +    case GST_VIDEO_TRANSFER_UNKNOWN:\n> > > > > +        colorspace->transferFunction = ColorSpace::TransferFunction::Default;\n> > > > > +        break;\n> > > > > +    default:\n> > > > > +        GST_WARNING(\"Unknown colorimetry transfer %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> > > > > +        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> > > > > +    case GST_VIDEO_COLOR_MATRIX_RGB:\n> > > > \n> > > > Shouldn't this map to YcbcrEncoding::None ?\n> > > \n> > > YcbcrEncoding for sRGB as per Kernel is V4L2_YCBCR_ENC_SYCC which is \n> > > deprecated so V4L2_YCBCR_ENC_601  is used instead [1]\n> > > \n> > > The same has been applied *already* in libcamera codebase:\n> > > \n> > > const ColorSpace ColorSpace::Srgb = {\n> > >         Primaries::Rec709,\n> > >         TransferFunction::Srgb,\n> > >         YcbcrEncoding::Rec601,\n> > >         Range::Limited\n> > > };\n> > > \n> > > So we shouldn't map GST_VIDEO_COLOR_MATRIX_RGB to YcbcrEncoding::None \n> > > but to Rec601 instead.\n> \n> I'm not sure that's right. GST_VIDEO_COLOR_MATRIX_RGB is defined as \n> \n> GST_VIDEO_COLOR_MATRIX_RGB (1) – identity matrix. Order of coefficients\n> is actually GBR, also IEC 61966-2-1 (sRGB) \n\nTo add to this, sRGB and GST_VIDEO_COLOR_MATRIX_RGB have absolutely no\nrelationship. Please try not to confuse things. GST_VIDEO_COLOR_MATRIX_RGB is\nstrictly used for the case the matrix is not applicable. Notably for all the RGB\ntype of data. This is clearly a 1 to 1 match with YcbcrEncoding::None.\n\n> \n> I understand this as meaning that the RGB data is not converted to YUV,\n> which is expressed in libcamera as YcbcrEncoding::None:\n> \n> * \\var ColorSpace::YcbcrEncoding::None\n> * \\brief There is no defined Y'CbCr encoding (used for non-YUV formats)\n> \n> The V4L2 sRGB colorspace [2] definition is confusing. sRGB is an RGB\n> colorspace, so it should have no YUV encoding, and use full range. The\n> sYCC colorspace is sRGB encoded in YUV using the Rec601 encoding, also\n> using full range. As it's typical for devices that produce sRGB images\n> to encode them to YUV using the Rec601 encoding and limited range, the\n> V4L2 sRGB colorspace defines default encoding and quantization range as\n> Rec601 and limited. However, when the pixels are stored as RGB, those\n> two parameters don't apply.\n> \n> Note also how GStreamer defines the SRGB colorspace ([3]):\n> \n> MAKE_COLORIMETRY (SRGB, _0_255, RGB, SRGB, BT709),\n> \n> We could do the same and define sRGB as\n> \n> const ColorSpace ColorSpace::Srgb = {\n>         Primaries::Rec709,\n>         TransferFunction::Srgb,\n>         YcbcrEncoding::None,\n>         Range::Full\n> };\n> \n> but we would then need to adapt code that produce YUV-encoded sRGB. We\n> could define, for that purpose,\n> \n> const ColorSpace ColorSpace::Sycc = {\n>         Primaries::Rec709,\n>         TransferFunction::Srgb,\n>         YcbcrEncoding::Rec601,\n>         Range::Full\n> };\n> \n> but that still wouldn't match the V4L2 definition. It's not necessarily\n> a problem though, devices can produce { Rec709, Srgb, Rec601, Limited }\n> without the need to explicitly define a ColorSpace preset in libcamera.\n> I've alos just realized that the above is identical to ColorSpace::Jpeg.\n> \n> Another option would be to keep the ColorSpace::Srgb definition as-is\n> and document that the YCbCrEncoding and Range must be ignored for\n> non-YUV formats, but I don't like that much, it sounds confusing.\n> \n> I'm tempted to bite the bullet and define ColorSpace::Srgb with\n> YcbcrEncoding::None and Range::Full, and never return ColorSpace::Srgb\n> when capturing YUV data. A pipeline handler that gets ColorSpace::Srgb\n> from an application would turn it into { Rec709, Srgb, Rec601, Limited }\n> for YUV streams (assuming that the hardware supports that of course).\n> \n> David, how is ColorSpace::Srgb interpreted by the Raspberry Pi ISP when\n> producing YUV data ?\n> \n> [2] https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/colorspaces-details.html#colorspace-srgb-v4l2-colorspace-srgb\n> [3] https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/main/subprojects/gst-plugins-base/gst-libs/gst/video/video-color.c#L66\n> \n> > > Now two question arises for reverse mapping it i.e. what should:\n> > > \n> > >    a) YcbcrEncding::None map to\n> > > \n> > >    b) YcbcrEncding::Rec601 map to.\n> > > \n> > > For b) we have two contenders:\n> > > \n> > >     GST_VIDEO_COLOR_MATRIX_BT601\n> > >     GST_VIDEO_COLOR_MATRIX_RGB\n> > \n> > b) is currently handled with:\n> > \n> > +       case ColorSpace::YcbcrEncoding::Rec601:\n> > +               if (colorSpace == ColorSpace::Srgb)\n> > +                       colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_RGB;\n> > +               else\n> > +                       colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT601;\n> > +               break;\n> > \n> > Let's review this in v2 (submitting shortly)\n> \n> As per the above, I think YcbcrEncding::None should map to\n> GST_VIDEO_COLOR_MATRIX_RGB and YcbcrEncding::Rec601 to\n> GST_VIDEO_COLOR_MATRIX_BT601, unconditionally.\n> \n> > > For a) I think GST_VIDEO_COLOR_MATRIX_RGB would be better ?\n> > > \n> > > [1] https://git.linuxtv.org/media_tree.git/tree/include/uapi/linux/videodev2.h#n344\n> > > \n> > > > > +    case GST_VIDEO_COLOR_MATRIX_UNKNOWN:\n> > > > > +        colorspace->ycbcrEncoding = ColorSpace::YcbcrEncoding::Default;\n> > > > > +        break;\n> > > > > +    default:\n> > > > > +        GST_WARNING(\"Unknown 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> > > > > +    case GST_VIDEO_COLOR_RANGE_UNKNOWN:\n> > > > > +        colorspace->range = ColorSpace::Range::Default;\n> > > > > +        break;\n> > > > > +    default:\n> > > > > +        GST_WARNING(\"Unknown range in colorimetry %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 +290,17 @@ 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 != nullptr)\n> > > > \n> > > >         if (colorimetry_str)\n> > > > \n> > > > > +            gst_structure_set(s, \"colorimetry\", G_TYPE_STRING, colorimetry_str, nullptr);\n> > > > > +        else\n> > > > > +            g_warning(\"libcamera::ColorSpace found but GstVideoColorimetry unknown\");\n> > > > > +    }\n> > > > > +\n> > > > >       gst_caps_append_structure(caps, s);\n> > > > >         return caps;\n> > > > > @@ -222,6 +384,19 @@ 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> > > > > + std::optional colorSpace = colorspace_from_colorimetry(&colorimetry);\n> > > > > +            stream_cfg.colorSpace = colorSpace;\n> > > > \n> > > >             stream_cfg.colorSpace = colorspace_from_colorimetry(&colorimetry);\n> > > > \n> > > > > +        } else {\n> > > > > +            g_print(\"Invalid colorimetry %s\", colorimetry_caps);\n> > > > > +        }\n> > > > > +    }\n> > > > >   }\n> > > > >     #if !GST_CHECK_VERSION(1, 17, 1)\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 6A316BE173\n\tfor ;\n\tFri, 29 Jul 2022 15:15:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BAE8063312;\n\tFri, 29 Jul 2022 17:15:32 +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 9DD15603EC\n\tfor ;\n\tFri, 29 Jul 2022 17:15:30 +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\t(No client certificate requested) (Authenticated sender: nicolas)\n\tby madras.collabora.co.uk (Postfix) with ESMTPSA id B041D6601B74;\n\tFri, 29 Jul 2022 16:15:29 +0100 (BST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659107732;\n\tbh=NAzkc0Q4y3ECI9BDSGAw/Sonwpj/VGNHsVP+6IjptIM=;\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=r4+P31dNsAoDicp6XqKED60n0jYakf9DjS5fpznCV/BbzC2hGvbolgxZB+zhjwsZa\n\tRXnOLamc4P/QBJ7MJaQwhsYHfBAHsWUcRm5HfkI2FE+ZSwaodOiTMQQKAqhVW+CAqC\n\tg8GmjVx250K3OjZaJJPdRkQiJtX43B3JpfSn6u8wex4yVy4hynawgsg1YBB5Mcnkrz\n\tFbLtZDv/7tSfrYksebJEI6Od69vh0kTYZvqGnoI+763XDNpwYIB0Y9MKyyhvvq/b+w\n\tLryuMqQQ/gX4pNAlo2G2gDOFbwumY1rj2inekteTd0KFRnNM2Sw4d2Ag7fgMRdyCuq\n\tzv4bs+PRQprvg==","v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com;\n\ts=mail; t=1659107730;\n\tbh=NAzkc0Q4y3ECI9BDSGAw/Sonwpj/VGNHsVP+6IjptIM=;\n\th=Subject:From:To:Cc:Date:In-Reply-To:References:From;\n\tb=LWWVS5nUhU2DnIZe/R16EijULa4JPeUYOvLpSNWLWWVoXAhkYjIGTQOiBMipjlDU5\n\ti6DWEFcHD1ZG/j89LI/MpWDmApXGDAo3DWt/+R0hDpl3PPTko9RgEMUmeg9Of2GGP+\n\tZFSJg9B5JvBg1QytZ6+EybrHnine64sbhMA8nSmC3UDBcFxCJMu41rtSTVdhu8N1rv\n\tBG5Jcyjr+GAyp5/R5FaUU4I5vSuKvycPSuhPLjwmisB9ITJYaYT1hue5+4zr5nsK8b\n\t3sZ38eCnBmfI4cH9e2j+DNblTflXcnyILgwCXOc1LW7tWC6g9nNATdurG3t757MVcd\n\t08depWMZjiHAQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=collabora.com\n\theader.i=@collabora.com\n\theader.b=\"LWWVS5nU\"; dkim-atps=neutral","Message-ID":"","To":"Laurent Pinchart , Umang Jain\n\t","Date":"Fri, 29 Jul 2022 11:15:20 -0400","In-Reply-To":"","References":"<20220724144355.104978-1-umang.jain@ideasonboard.com>\n\t<20220724144355.104978-4-umang.jain@ideasonboard.com>\n\t\n\t<09f1366c-5088-3daa-c31e-4f5597af2180@ideasonboard.com>\n\t<69342c1c-af1f-5451-4d0d-61693facc56c@ideasonboard.com>\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 3/3] gstreamer: Provide colorimetry <>\n\tlibcamera::ColorSpace 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24246,"web_url":"https://patchwork.libcamera.org/comment/24246/","msgid":"","date":"2022-07-29T15:29:30","subject":"Re: [libcamera-devel] [PATCH 3/3] gstreamer: Provide colorimetry <>\n\tlibcamera::ColorSpace mappings","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Hi Umang,\n\nHave you considered creating your baseline based on v4lsrc existing mapping ?\nYou can check you V4L2 <-> libcamera <-> gstreamer mapping against the v4l2src\nv4l2 <-> gst mapping ? I don't pretend this will have no issues at all, but at\nleas the mapping exercise base on V4L2 text have been done there once.\n\nhttps://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/main/subprojects/gst-plugins-good/sys/v4l2/gstv4l2object.c#L2055\n\nregards,\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 6D426C3275\n\tfor ;\n\tFri, 29 Jul 2022 15:29:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A37B863312;\n\tFri, 29 Jul 2022 17:29:41 +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 E98FF603EC\n\tfor ;\n\tFri, 29 Jul 2022 17:29:39 +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 D549A6601B77;\n\tFri, 29 Jul 2022 16:29:38 +0100 (BST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659108581;\n\tbh=eaZ0A+GWSbejWEnT7buR5429vcmwz5FfVEOpaMtChJI=;\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=sEmaxNHpbzZz526KEQ2BGmQAMd6u/eqilkUmh+lQXeVgzN/03tr/4nGqMYjocvCCf\n\tiuu0/c1Dwd/nrFWDKtrsq7L1/fWymcHBAhD/pjtBaIpUsEoKfP6TRTTWKGblIRDaD2\n\tT7BpCRKnWpHyL30QVNXUgFFNZrixBwVe5fiRVgEbHsldPipQIykEhxvLeMTfhLN4qh\n\tyeAUJ6CRcNxTpeJIDHW5Zkw/qerCEtaVdw5rcv4m8357GBf/2NYRHfu9rxkpKt3XaL\n\t40p+kOMDUKDPs9HMds2KD+Y0p7MYRwhb3QqIB0iNbVMG3TRWnn5B3Gzf/y98wjdIAN\n\tFj/sW4bN5KPvw==","v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com;\n\ts=mail; t=1659108579;\n\tbh=eaZ0A+GWSbejWEnT7buR5429vcmwz5FfVEOpaMtChJI=;\n\th=Subject:From:To:Cc:Date:In-Reply-To:References:From;\n\tb=WC9jKKm7hVRtWGH2XsB28bRm22KTDC0ModxuCQGtp88BbWXYGew+uFce0l1PTnjdO\n\tbyi2Oa81UYWmzMOAcJvORO00bCmLlX2qMCqs1/L+VGcm9EpUywWeWGkCwNV5VvCSCg\n\tUzk6xYQUQ3PvqY3GKRELh01qZpZbcS91XqCLzZmWDCD252HWfNMjaYtyXRyUPWHgCx\n\t1e27RQTmdARjY+6IGRAvcQ8Wq9BTxJ3H7uPSM2x3TbTZl1bBW6dLNAgBGy27yi0CaT\n\tmUB/XiTFgoKQDsnkGye5gN0JppMljKRwdrBjwHep0PC0DOwgNi7edE079kAOaYsTXF\n\ta1zNu5N0UCfPw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=collabora.com\n\theader.i=@collabora.com\n\theader.b=\"WC9jKKm7\"; dkim-atps=neutral","Message-ID":"","To":"Laurent Pinchart , Umang Jain\n\t","Date":"Fri, 29 Jul 2022 11:29:30 -0400","In-Reply-To":"","References":"<20220724144355.104978-1-umang.jain@ideasonboard.com>\n\t<20220724144355.104978-4-umang.jain@ideasonboard.com>\n\t\n\t<09f1366c-5088-3daa-c31e-4f5597af2180@ideasonboard.com>\n\t<69342c1c-af1f-5451-4d0d-61693facc56c@ideasonboard.com>\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 3/3] gstreamer: Provide colorimetry <>\n\tlibcamera::ColorSpace 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24256,"web_url":"https://patchwork.libcamera.org/comment/24256/","msgid":"","date":"2022-07-31T20:28:46","subject":"Re: [libcamera-devel] [PATCH 3/3] gstreamer: Provide colorimetry <>\n\tlibcamera::ColorSpace mappings","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nicolas,\n\n(CC'ing David)\n\nOn Fri, Jul 29, 2022 at 11:29:30AM -0400, Nicolas Dufresne wrote:\n> Hi Umang,\n> \n> Have you considered creating your baseline based on v4lsrc existing mapping ?\n> You can check you V4L2 <-> libcamera <-> gstreamer mapping against the v4l2src\n> v4l2 <-> gst mapping ? I don't pretend this will have no issues at all, but at\n> leas the mapping exercise base on V4L2 text have been done there once.\n> \n> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/main/subprojects/gst-plugins-good/sys/v4l2/gstv4l2object.c#L2055\n\nThanks for the pointer.\n\nThat logic seems sensible, and likely something we'll want to do in\nV4L2Device::fromColorSpace() and V4L2Device::toColorSpace().","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 EBC56BE173\n\tfor ;\n\tSun, 31 Jul 2022 20:28:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 43CA263312;\n\tSun, 31 Jul 2022 22:28:53 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2477F603E9\n\tfor ;\n\tSun, 31 Jul 2022 22:28:51 +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 7AA4C415;\n\tSun, 31 Jul 2022 22:28:50 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659299333;\n\tbh=su8fsGOMiYyOxWbwzaBAYwF0Lm1lID64wufYPj+KJe8=;\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=xmU/DVKGvAUhu9O11veB2SckuSrI0aR/2vWx7xJ1HSN/v9KSmtpczwX74tmr4N+OI\n\txl6mjpImHAcvM5GkeelGEDMjv8C67mwDa2+qBui79YCkfruDa6Npss1e070er598i+\n\tTzPT5IBdhQ6RRviesWfQoqLDjRJlhQTMaPDH5N0wf/pJfQGVjIt6YIdVJfnYDi7Rnv\n\tVwQw5pZMR9hfwThSMNOV9rtjuFJxZN2WvxcnVU5Bbcx/6roZi3lg1OLHTXlI7oJNDA\n\t1mqTScjzTu1LjZZijBbxvK7DvRWZPHXW49+k/NAP81ywdzav/ptWmvNmEMclodQq/3\n\tf8C8f8KGwnacg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1659299330;\n\tbh=su8fsGOMiYyOxWbwzaBAYwF0Lm1lID64wufYPj+KJe8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NvBpnbiSppeJosmTSnnMLQv5jaSn3Ilvr/uORlwkBpVM4F5Y76S99O8kbdDepSies\n\t3EEJJeLWniGlGAKJ/+4fzLzkCClhKzldAK9OxJgFEu8bJdZ951jafP148d04s/blfK\n\tP5CIEYksPdQVL2Duf6bTtCaC4XN1DboQAFOn1z9k="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"NvBpnbiS\"; dkim-atps=neutral","Date":"Sun, 31 Jul 2022 23:28:46 +0300","To":"Nicolas Dufresne ","Message-ID":"","References":"<20220724144355.104978-1-umang.jain@ideasonboard.com>\n\t<20220724144355.104978-4-umang.jain@ideasonboard.com>\n\t\n\t<09f1366c-5088-3daa-c31e-4f5597af2180@ideasonboard.com>\n\t<69342c1c-af1f-5451-4d0d-61693facc56c@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 3/3] gstreamer: Provide colorimetry <>\n\tlibcamera::ColorSpace 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24261,"web_url":"https://patchwork.libcamera.org/comment/24261/","msgid":"","date":"2022-08-01T11:08:43","subject":"Re: [libcamera-devel] [PATCH 3/3] gstreamer: Provide colorimetry <>\n\tlibcamera::ColorSpace 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 7/29/22 20:24, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> On Fri, Jul 29, 2022 at 03:40:52PM +0530, Umang Jain wrote:\n>> On 7/29/22 13:36, Umang Jain via libcamera-devel wrote:\n>>> Hi Laurent,\n>>>\n>>> My anaylsis on sRGB mapping ...\n>>>\n>>> On 7/27/22 06:47, Laurent Pinchart wrote:\n>>>> Hi Umang and Rishikesh,\n>>>>\n>>>> Thank you for the patch.\n>>>>\n>>>> On Sun, Jul 24, 2022 at 08:13:55PM +0530, Umang Jain via\n>>>> 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>>>>> argument. Multiple colorimetry support shall be introduced in\n>>>>> subsequent commits.\n>>>>>\n>>>>> Signed-off-by: Rishikesh Donadkar \n>>>>> Signed-off-by: Umang Jain \n>>>>> ---\n>>>>>   src/gstreamer/gstlibcamera-utils.cpp | 175 +++++++++++++++++++++++++++\n>>>>>   1 file changed, 175 insertions(+)\n>>>>>\n>>>>> diff --git a/src/gstreamer/gstlibcamera-utils.cpp\n>>>>> b/src/gstreamer/gstlibcamera-utils.cpp\n>>>>> index c97c0d43..fb4f0e5c 100644\n>>>>> --- a/src/gstreamer/gstlibcamera-utils.cpp\n>>>>> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n>>>>> @@ -45,6 +45,157 @@ static struct {\n>>>>>       /* \\todo NV42 is used in libcamera but is not mapped in GStreamer yet. */\n>>>>>   };\n>>>>>   +static GstVideoColorimetry\n>>>>> +colorimetry_from_colorspace(const ColorSpace &colorSpace)\n>>>>> +{\n>>>>> +    GstVideoColorimetry colorimetry;\n>>>> I would initialize the structure with all fields set to UNKNOWN here...\n>>>>\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>>>>> +    default:\n>>>>> +        GST_WARNING(\"ColorSpace primaries not mapped in\n>>>>> GstLibcameraSrc\");\n>>>>> +        colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_UNKNOWN;\n>>>> ... and drop the default case. As ColorSpace::Primaries is an enum\n>>>> class, if you have cases for all the values, the compiler will warn only\n>>>> if one (or more) of the enum values isn't handled in explicit cases.\n>>>> This way you can avoid warnings, and have a guarantee that if the\n>>>> libcamera colorspaces are later extended, the compiler will remind that\n>>>> this function has to be updated. Same for the other components of the\n>>>> colorspace below.\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>>>>> +    default:\n>>>>> +        GST_WARNING(\"ColorSpace transfer function not mapped in GstLibcameraSrc\");\n>>>>> +        colorimetry.transfer = GST_VIDEO_TRANSFER_UNKNOWN;\n>>>>> +    }\n>>>>> +\n>>>>> +    switch (colorSpace.ycbcrEncoding) {\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>>>>> +        colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT601;\n>>>>> +        break;\n>>>>> +    default:\n>>>>> +        GST_WARNING(\"Colorspace YcbcrEncoding not mapped in GstLibcameraSrc\");\n>>>>> +        colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_UNKNOWN;\n>>>> Should we map YcbcrEncoding::None to GST_VIDEO_COLOR_MATRIX_RGB ?\n>>> See below.\n>>>\n>>>> Otherwise capturing RGB or raw bayer will produce a warning. Same for\n>>>> the Raw primaries, it seems that the best mapping would be\n>>>> GST_VIDEO_COLOR_PRIMARIES_UNKNOWN (unless I'm missing something), but I\n>>>> don't think we should warn in that case.\n>>>>\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>>>>> +    default:\n>>>>> +        GST_WARNING(\"Colorspace range not mapped in GstLibcameraSrc\");\n>>>>> +        colorimetry.range = GST_VIDEO_COLOR_RANGE_UNKNOWN;\n>>>>> +    }\n>>>>> +\n>>>>> +    return colorimetry;\n>>>>> +}\n>>>>> +\n>>>>> +static std::optional\n>>>>> +colorspace_from_colorimetry(GstVideoColorimetry *colorimetry)\n>>>> I'd pass a const reference instead of a pointer, as there's no use case\n>>>> for a null colorimetry.\n>>>>\n>>>>> +{\n>>>>> +    std::optional colorspace = ColorSpace::Default;\n>>>> No need for std::optional here or in the return type.\n>>>>\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>>>>> +        colorspace->primaries = ColorSpace::Primaries::Default;\n>>>>> +        break;\n>>>>> +    default:\n>>>>> +        GST_WARNING(\"Unknown primaries in colorimetry %d\", colorimetry->primaries);\n>>>>> +    }\n>>>>> +\n>>>>> +    switch (colorimetry->transfer) {\n>>>>> +    /* Tranfer 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>>>>> +    case GST_VIDEO_TRANSFER_UNKNOWN:\n>>>>> +        colorspace->transferFunction = ColorSpace::TransferFunction::Default;\n>>>>> +        break;\n>>>>> +    default:\n>>>>> +        GST_WARNING(\"Unknown colorimetry transfer %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>>>>> +        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>>>>> +    case GST_VIDEO_COLOR_MATRIX_RGB:\n>>>> Shouldn't this map to YcbcrEncoding::None ?\n>>> YcbcrEncoding for sRGB as per Kernel is V4L2_YCBCR_ENC_SYCC which is\n>>> deprecated so V4L2_YCBCR_ENC_601  is used instead [1]\n>>>\n>>> The same has been applied *already* in libcamera codebase:\n>>>\n>>> const ColorSpace ColorSpace::Srgb = {\n>>>         Primaries::Rec709,\n>>>         TransferFunction::Srgb,\n>>>         YcbcrEncoding::Rec601,\n>>>         Range::Limited\n>>> };\n>>>\n>>> So we shouldn't map GST_VIDEO_COLOR_MATRIX_RGB to YcbcrEncoding::None\n>>> but to Rec601 instead.\n> I'm not sure that's right. GST_VIDEO_COLOR_MATRIX_RGB is defined as\n>\n> GST_VIDEO_COLOR_MATRIX_RGB (1) – identity matrix. Order of coefficients\n> is actually GBR, also IEC 61966-2-1 (sRGB)\n>\n> I understand this as meaning that the RGB data is not converted to YUV,\n> which is expressed in libcamera as YcbcrEncoding::None:\n>\n> * \\var ColorSpace::YcbcrEncoding::None\n> * \\brief There is no defined Y'CbCr encoding (used for non-YUV formats)\n>\n> The V4L2 sRGB colorspace [2] definition is confusing. sRGB is an RGB\n> colorspace, so it should have no YUV encoding, and use full range. The\n> sYCC colorspace is sRGB encoded in YUV using the Rec601 encoding, also\n> using full range. As it's typical for devices that produce sRGB images\n> to encode them to YUV using the Rec601 encoding and limited range, the\n> V4L2 sRGB colorspace defines default encoding and quantization range as\n> Rec601 and limited. However, when the pixels are stored as RGB, those\n> two parameters don't apply.\n>\n> Note also how GStreamer defines the SRGB colorspace ([3]):\n>\n> MAKE_COLORIMETRY (SRGB, _0_255, RGB, SRGB, BT709),\n>\n> We could do the same and define sRGB as\n>\n> const ColorSpace ColorSpace::Srgb = {\n>         Primaries::Rec709,\n>         TransferFunction::Srgb,\n>         YcbcrEncoding::None,\n>         Range::Full\n> };\n>\n> but we would then need to adapt code that produce YUV-encoded sRGB. We\n> could define, for that purpose,\n>\n> const ColorSpace ColorSpace::Sycc = {\n>         Primaries::Rec709,\n>         TransferFunction::Srgb,\n>         YcbcrEncoding::Rec601,\n>         Range::Full\n> };\n>\n> but that still wouldn't match the V4L2 definition. It's not necessarily\n> a problem though, devices can produce { Rec709, Srgb, Rec601, Limited }\n> without the need to explicitly define a ColorSpace preset in libcamera.\n> I've alos just realized that the above is identical to ColorSpace::Jpeg.\n>\n> Another option would be to keep the ColorSpace::Srgb definition as-is\n> and document that the YCbCrEncoding and Range must be ignored for\n> non-YUV formats, but I don't like that much, it sounds confusing.\n>\n> I'm tempted to bite the bullet and define ColorSpace::Srgb with\n> YcbcrEncoding::None and Range::Full, and never return ColorSpace::Srgb\n> when capturing YUV data. A pipeline handler that gets ColorSpace::Srgb\n> from an application would turn it into { Rec709, Srgb, Rec601, Limited }\n> for YUV streams (assuming that the hardware supports that of course).\n\n\nSo I am getting a bit confused here for the part where YUV encoded RGB \ndata needs to use which range? Limited or Full? You seem to have \nreferred to limited, it seems - but I don't get the difference.\n\nI guess I'm asking the difference between ColorSpace::Jpeg vs { Rec709, \nSrgb, Rec601, Limited } really\n\nThe pipeline-handler specific handling on sRGB colorspace is \ninteresting. To re-iterate and if I am understanding things correctly \n(from API point of view):\n\na) ColorSpace::sRGB preset will be used to present both sRGB and \nYUV-encoded RGB colorspace, by the user (i.e. the user doesn't seem to \ncare/understand)\n\n     - libcamera PH being intelligent enough to deduce the stream \nconfiguration and convert ColorSpace::sRGB to { Rec709, Srgb, Rec601, \nLimited } and validate + return back to application\n\n     - If there is no YUV stream requested, ColorSpace::sRGB preset is \nused, as-is.\n\nb) Application intelligent enough that it's requesting a YUV stream and \nColorSpace::sRGB doesn't mean YUV-encoded RGB(which will be true after \nwe modify sRGB preset)\n\n     - In that case, application send in directly { Rec709, Srgb, \nRec601, Limited } as colorspace.\n\nIs this understanding correct on my part? If yes, follow up question is:\n\n     - The pipeline-handler colorspace adjustment logic(for YUV streams) \n- do you see it per-PH or something generic that all pipeline-handlers \nwould use ? (I guess the latter)\n\n>\n> David, how is ColorSpace::Srgb interpreted by the Raspberry Pi ISP when\n> producing YUV data ?\n>\n> [2] https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/colorspaces-details.html#colorspace-srgb-v4l2-colorspace-srgb\n> [3] https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/main/subprojects/gst-plugins-base/gst-libs/gst/video/video-color.c#L66\n>\n>>> Now two question arises for reverse mapping it i.e. what should:\n>>>\n>>>    a) YcbcrEncding::None map to\n>>>\n>>>    b) YcbcrEncding::Rec601 map to.\n>>>\n>>> For b) we have two contenders:\n>>>\n>>>     GST_VIDEO_COLOR_MATRIX_BT601\n>>>     GST_VIDEO_COLOR_MATRIX_RGB\n>> b) is currently handled with:\n>>\n>> +       case ColorSpace::YcbcrEncoding::Rec601:\n>> +               if (colorSpace == ColorSpace::Srgb)\n>> +                       colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_RGB;\n>> +               else\n>> +                       colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT601;\n>> +               break;\n>>\n>> Let's review this in v2 (submitting shortly)\n> As per the above, I think YcbcrEncding::None should map to\n> GST_VIDEO_COLOR_MATRIX_RGB and YcbcrEncding::Rec601 to\n> GST_VIDEO_COLOR_MATRIX_BT601, unconditionally.\n>\n>>> For a) I think GST_VIDEO_COLOR_MATRIX_RGB would be better ?\n>>>\n>>> [1] https://git.linuxtv.org/media_tree.git/tree/include/uapi/linux/videodev2.h#n344\n>>>\n>>>>> +    case GST_VIDEO_COLOR_MATRIX_UNKNOWN:\n>>>>> +        colorspace->ycbcrEncoding = ColorSpace::YcbcrEncoding::Default;\n>>>>> +        break;\n>>>>> +    default:\n>>>>> +        GST_WARNING(\"Unknown 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>>>>> +    case GST_VIDEO_COLOR_RANGE_UNKNOWN:\n>>>>> +        colorspace->range = ColorSpace::Range::Default;\n>>>>> +        break;\n>>>>> +    default:\n>>>>> +        GST_WARNING(\"Unknown range in colorimetry %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 +290,17 @@ 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 != nullptr)\n>>>>         if (colorimetry_str)\n>>>>\n>>>>> +            gst_structure_set(s, \"colorimetry\", G_TYPE_STRING, colorimetry_str, nullptr);\n>>>>> +        else\n>>>>> +            g_warning(\"libcamera::ColorSpace found but GstVideoColorimetry unknown\");\n>>>>> +    }\n>>>>> +\n>>>>>       gst_caps_append_structure(caps, s);\n>>>>>         return caps;\n>>>>> @@ -222,6 +384,19 @@ 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>>>>> + std::optional colorSpace = colorspace_from_colorimetry(&colorimetry);\n>>>>> +            stream_cfg.colorSpace = colorSpace;\n>>>>             stream_cfg.colorSpace = colorspace_from_colorimetry(&colorimetry);\n>>>>\n>>>>> +        } else {\n>>>>> +            g_print(\"Invalid colorimetry %s\", colorimetry_caps);\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 CA3C1BE173\n\tfor ;\n\tMon, 1 Aug 2022 11:08:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 16B2D63312;\n\tMon, 1 Aug 2022 13:08:53 +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 E9378603E8\n\tfor ;\n\tMon, 1 Aug 2022 13:08:50 +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 3E22D2F3;\n\tMon, 1 Aug 2022 13:08:49 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659352133;\n\tbh=1zWyGSFXIXFQPkll6bNjpOW/wl7tFGqTSHllTN5pZ5E=;\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=RfQdo4vIKgzbEWmf2CWjjrqc8G8c0nFZsyAvG9FsCHegwvQN2OEav1mf7uyqoeDq8\n\t32aTeSojbiftpPw4noWCXnS6Pe7WsPzSZxG0Eyzk/kUic+4ktY+Qxo93bku3U68yO+\n\to+zy0ZTzikyU+1g1Z8r66+CNI+sl8PVq8+zlZRFmdRfRZuwUJDAvQSbYvGMnoQnt5P\n\t2zFU6TA29I2g35AUV5oMGPyDepubk3XoRBtsWiwiIzJCSK1D2R6FNvcSWoCw8EJ3s7\n\tYhp6sgDoAn/dd0ysyFGbuOo+odBqs9TxMoKerEzIx1z+W0nEjAH25WPpyFqbxMRskq\n\tcSbDoS+YufEYw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1659352130;\n\tbh=1zWyGSFXIXFQPkll6bNjpOW/wl7tFGqTSHllTN5pZ5E=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=EdJTR/YLuaQRXI222I6WQYHF02jgY5L8GOua3mu7kHDaJuP9WZPDmaJA8cNonZ++o\n\tAFRF1EMF+YFHOpJ1kjx5nFc7f7UfMsU14cbmQRD0Gt15nonYqJnFD2GY+LwgZH2ECB\n\teZlFqSBwOJGqcQbDTNlVdXF5k1LiRVl9sLd1Idfw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"EdJTR/YL\"; dkim-atps=neutral","Message-ID":"","Date":"Mon, 1 Aug 2022 16:38:43 +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":"<20220724144355.104978-1-umang.jain@ideasonboard.com>\n\t<20220724144355.104978-4-umang.jain@ideasonboard.com>\n\t\n\t<09f1366c-5088-3daa-c31e-4f5597af2180@ideasonboard.com>\n\t<69342c1c-af1f-5451-4d0d-61693facc56c@ideasonboard.com>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 3/3] gstreamer: Provide colorimetry <>\n\tlibcamera::ColorSpace 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\tnicolas.dufresne@collabora.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24264,"web_url":"https://patchwork.libcamera.org/comment/24264/","msgid":"","date":"2022-08-01T13:17:50","subject":"Re: [libcamera-devel] [PATCH 3/3] gstreamer: Provide colorimetry <>\n\tlibcamera::ColorSpace 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 04:38:43PM +0530, Umang Jain wrote:\n> On 7/29/22 20:24, Laurent Pinchart wrote:\n> > On Fri, Jul 29, 2022 at 03:40:52PM +0530, Umang Jain wrote:\n> >> On 7/29/22 13:36, Umang Jain via libcamera-devel wrote:\n> >>> On 7/27/22 06:47, Laurent Pinchart wrote:\n> >>>> On Sun, Jul 24, 2022 at 08:13:55PM +0530, Umang Jain via\n> >>>> 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> >>>>> argument. Multiple colorimetry support shall be introduced in\n> >>>>> subsequent commits.\n> >>>>>\n> >>>>> Signed-off-by: Rishikesh Donadkar \n> >>>>> Signed-off-by: Umang Jain \n> >>>>> ---\n> >>>>>   src/gstreamer/gstlibcamera-utils.cpp | 175 +++++++++++++++++++++++++++\n> >>>>>   1 file changed, 175 insertions(+)\n> >>>>>\n> >>>>> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> >>>>> index c97c0d43..fb4f0e5c 100644\n> >>>>> --- a/src/gstreamer/gstlibcamera-utils.cpp\n> >>>>> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> >>>>> @@ -45,6 +45,157 @@ 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> >>>> I would initialize the structure with all fields set to UNKNOWN here...\n> >>>>\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> >>>>> +    default:\n> >>>>> +        GST_WARNING(\"ColorSpace primaries not mapped in GstLibcameraSrc\");\n> >>>>> +        colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_UNKNOWN;\n> >>>>\n> >>>> ... and drop the default case. As ColorSpace::Primaries is an enum\n> >>>> class, if you have cases for all the values, the compiler will warn only\n> >>>> if one (or more) of the enum values isn't handled in explicit cases.\n> >>>> This way you can avoid warnings, and have a guarantee that if the\n> >>>> libcamera colorspaces are later extended, the compiler will remind that\n> >>>> this function has to be updated. Same for the other components of the\n> >>>> colorspace below.\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> >>>>> +    default:\n> >>>>> +        GST_WARNING(\"ColorSpace transfer function not mapped in GstLibcameraSrc\");\n> >>>>> +        colorimetry.transfer = GST_VIDEO_TRANSFER_UNKNOWN;\n> >>>>> +    }\n> >>>>> +\n> >>>>> +    switch (colorSpace.ycbcrEncoding) {\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> >>>>> +        colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT601;\n> >>>>> +        break;\n> >>>>> +    default:\n> >>>>> +        GST_WARNING(\"Colorspace YcbcrEncoding not mapped in GstLibcameraSrc\");\n> >>>>> +        colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_UNKNOWN;\n> >>>> \n> >>>> Should we map YcbcrEncoding::None to GST_VIDEO_COLOR_MATRIX_RGB ?\n> >>>\n> >>> See below.\n> >>>\n> >>>> Otherwise capturing RGB or raw bayer will produce a warning. Same for\n> >>>> the Raw primaries, it seems that the best mapping would be\n> >>>> GST_VIDEO_COLOR_PRIMARIES_UNKNOWN (unless I'm missing something), but I\n> >>>> don't think we should warn in that case.\n> >>>>\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> >>>>> +    default:\n> >>>>> +        GST_WARNING(\"Colorspace range not mapped in GstLibcameraSrc\");\n> >>>>> +        colorimetry.range = GST_VIDEO_COLOR_RANGE_UNKNOWN;\n> >>>>> +    }\n> >>>>> +\n> >>>>> +    return colorimetry;\n> >>>>> +}\n> >>>>> +\n> >>>>> +static std::optional\n> >>>>> +colorspace_from_colorimetry(GstVideoColorimetry *colorimetry)\n> >>>> \n> >>>> I'd pass a const reference instead of a pointer, as there's no use case\n> >>>> for a null colorimetry.\n> >>>>\n> >>>>> +{\n> >>>>> +    std::optional colorspace = ColorSpace::Default;\n> >>>> \n> >>>> No need for std::optional here or in the return type.\n> >>>>\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> >>>>> +        colorspace->primaries = ColorSpace::Primaries::Default;\n> >>>>> +        break;\n> >>>>> +    default:\n> >>>>> +        GST_WARNING(\"Unknown primaries in colorimetry %d\", colorimetry->primaries);\n> >>>>> +    }\n> >>>>> +\n> >>>>> +    switch (colorimetry->transfer) {\n> >>>>> +    /* Tranfer 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> >>>>> +    case GST_VIDEO_TRANSFER_UNKNOWN:\n> >>>>> +        colorspace->transferFunction = ColorSpace::TransferFunction::Default;\n> >>>>> +        break;\n> >>>>> +    default:\n> >>>>> +        GST_WARNING(\"Unknown colorimetry transfer %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> >>>>> +        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> >>>>> +    case GST_VIDEO_COLOR_MATRIX_RGB:\n> >>>> \n> >>>> Shouldn't this map to YcbcrEncoding::None ?\n> >>> \n> >>> YcbcrEncoding for sRGB as per Kernel is V4L2_YCBCR_ENC_SYCC which is\n> >>> deprecated so V4L2_YCBCR_ENC_601  is used instead [1]\n> >>>\n> >>> The same has been applied *already* in libcamera codebase:\n> >>>\n> >>> const ColorSpace ColorSpace::Srgb = {\n> >>>         Primaries::Rec709,\n> >>>         TransferFunction::Srgb,\n> >>>         YcbcrEncoding::Rec601,\n> >>>         Range::Limited\n> >>> };\n> >>>\n> >>> So we shouldn't map GST_VIDEO_COLOR_MATRIX_RGB to YcbcrEncoding::None\n> >>> but to Rec601 instead.\n> > \n> > I'm not sure that's right. GST_VIDEO_COLOR_MATRIX_RGB is defined as\n> >\n> > GST_VIDEO_COLOR_MATRIX_RGB (1) – identity matrix. Order of coefficients\n> > is actually GBR, also IEC 61966-2-1 (sRGB)\n> >\n> > I understand this as meaning that the RGB data is not converted to YUV,\n> > which is expressed in libcamera as YcbcrEncoding::None:\n> >\n> > * \\var ColorSpace::YcbcrEncoding::None\n> > * \\brief There is no defined Y'CbCr encoding (used for non-YUV formats)\n> >\n> > The V4L2 sRGB colorspace [2] definition is confusing. sRGB is an RGB\n> > colorspace, so it should have no YUV encoding, and use full range. The\n> > sYCC colorspace is sRGB encoded in YUV using the Rec601 encoding, also\n> > using full range. As it's typical for devices that produce sRGB images\n> > to encode them to YUV using the Rec601 encoding and limited range, the\n> > V4L2 sRGB colorspace defines default encoding and quantization range as\n> > Rec601 and limited. However, when the pixels are stored as RGB, those\n> > two parameters don't apply.\n> >\n> > Note also how GStreamer defines the SRGB colorspace ([3]):\n> >\n> > MAKE_COLORIMETRY (SRGB, _0_255, RGB, SRGB, BT709),\n> >\n> > We could do the same and define sRGB as\n> >\n> > const ColorSpace ColorSpace::Srgb = {\n> >         Primaries::Rec709,\n> >         TransferFunction::Srgb,\n> >         YcbcrEncoding::None,\n> >         Range::Full\n> > };\n> >\n> > but we would then need to adapt code that produce YUV-encoded sRGB. We\n> > could define, for that purpose,\n> >\n> > const ColorSpace ColorSpace::Sycc = {\n> >         Primaries::Rec709,\n> >         TransferFunction::Srgb,\n> >         YcbcrEncoding::Rec601,\n> >         Range::Full\n> > };\n> >\n> > but that still wouldn't match the V4L2 definition. It's not necessarily\n> > a problem though, devices can produce { Rec709, Srgb, Rec601, Limited }\n> > without the need to explicitly define a ColorSpace preset in libcamera.\n> > I've alos just realized that the above is identical to ColorSpace::Jpeg.\n> >\n> > Another option would be to keep the ColorSpace::Srgb definition as-is\n> > and document that the YCbCrEncoding and Range must be ignored for\n> > non-YUV formats, but I don't like that much, it sounds confusing.\n> >\n> > I'm tempted to bite the bullet and define ColorSpace::Srgb with\n> > YcbcrEncoding::None and Range::Full, and never return ColorSpace::Srgb\n> > when capturing YUV data. A pipeline handler that gets ColorSpace::Srgb\n> > from an application would turn it into { Rec709, Srgb, Rec601, Limited }\n> > for YUV streams (assuming that the hardware supports that of course).\n> \n> So I am getting a bit confused here for the part where YUV encoded RGB \n> data needs to use which range? Limited or Full? You seem to have \n> referred to limited, it seems - but I don't get the difference.\n> \n> I guess I'm asking the difference between ColorSpace::Jpeg vs { Rec709, \n> Srgb, Rec601, Limited } really\n\nThe difference is that, after conversion from RGB to YUV using the\nRec601 matrix, the Y and U/V values will be limited to [16, 235] and\n[16, 240] respectively (by shifting and scaling them to that range).\nsYCC mandates usage of limited range, but many devices that use the sRGB\ncolor space encoded in YUV produce full range instead, which led to the\nnaming of the informal JPEG color space.\n\n> The pipeline-handler specific handling on sRGB colorspace is \n> interesting. To re-iterate and if I am understanding things correctly \n> (from API point of view):\n> \n> a) ColorSpace::sRGB preset will be used to present both sRGB and \n> YUV-encoded RGB colorspace, by the user (i.e. the user doesn't seem to \n> care/understand)\n\nTo be clear, assuming we will redefine sRGB the same way as GStreamer,\n\nconst ColorSpace ColorSpace::Srgb = {\n         Primaries::Rec709,\n         TransferFunction::Srgb,\n         YcbcrEncoding::None,\n         Range::Full\n};\n\nthen it can represent RGB data only. If an application picks it to\ncapture RGB data, all is fine. If an application picks it to capture YUV\ndata, that's technically wrong, but libcamera should adjust it as our\ncolor space API requires pipeline handlers to adjust color spaces\nrequested by applications and not supported by the device.\n\n>     - libcamera PH being intelligent enough to deduce the stream \n> configuration and convert ColorSpace::sRGB to { Rec709, Srgb, Rec601, \n> Limited } and validate + return back to application\n\nNearly. I think the logic should check the transfer function, and if\nit's Srgb and the encoding is None for a YUV format, it should pick\nRec601 as the encoding. Actually, looking at the presets we have,\nsomething like the following could work:\n\n\tif (format == YUV && encoding == None) {\n\t\tif (transferFunction == Rec709) {\n\t\t\tswitch (primaries) {\n\t\t\tcase Raw:\n\t\t\tcase Smpte170m:\n\t\t\t\tencoding = Rec601;\n\t\t\t\tbreak;\n\t\t\tcase Rec709:\n\t\t\t\tencoding = Rec709;\n\t\t\t\tbreak;\n\t\t\tcase Rec2020:\n\t\t\t\tencoding = Rec2020;\n\t\t\t\tbreak;\n\t\t\t}\n\t\t} else {\n\t\t\tencoding = Rec601;\n\t\t}\n\t}\n\n(the Raw case should never happen, I've picked a default encoding in\nthat case.)\n\nI'm sure there are other ways to express the logic, looking at the\nGStreamer v4l2src element could help. It would also be nice to\ncentralize this kind of adjustement logic somewhere (possibly in the\nColorSpace class).\n\nI said \"nearly\" above as I'm not sure about the default range we should\nselect (limited vs. full). sYCC is limited, JPEG is full. Should we\nassume that an application that incorrectly asks for ColorSpace::Srgb\nfor a YUV stream will actually want sYCC or JPEG ?\n\n>     - If there is no YUV stream requested, ColorSpace::sRGB preset is \n> used, as-is.\n\nRight.\n\n> b) Application intelligent enough that it's requesting a YUV stream and \n> ColorSpace::sRGB doesn't mean YUV-encoded RGB(which will be true after \n> we modify sRGB preset)\n> \n>     - In that case, application send in directly { Rec709, Srgb, \n> Rec601, Limited } as colorspace.\n\nThat, or the same with Full range, depending on what the application\nwants.\n\n> Is this understanding correct on my part? If yes, follow up question is:\n\nI think so.\n\n>     - The pipeline-handler colorspace adjustment logic(for YUV streams) \n> - do you see it per-PH or something generic that all pipeline-handlers \n> would use ? (I guess the latter)\n\nI think I've answered that above :-) I'd like to centralize as much\nlogic as possible, and at least the logic that picks a default YCbCr\nencoding for YUV formats. For the rest, the adjustment also depends on\nwhat the hardware supports, so fully generic helpers may not be\npossible.\n\nEven if we can't implement the adjustment code in shared code that would\nwork with all pipeline handlers, I would like to document the adjustment\nheuristics to have consistent behaviour across pipeline handlers.\n\n> > David, how is ColorSpace::Srgb interpreted by the Raspberry Pi ISP when\n> > producing YUV data ?\n> >\n> > [2] https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/colorspaces-details.html#colorspace-srgb-v4l2-colorspace-srgb\n> > [3] https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/main/subprojects/gst-plugins-base/gst-libs/gst/video/video-color.c#L66\n> >\n> >>> Now two question arises for reverse mapping it i.e. what should:\n> >>>\n> >>>    a) YcbcrEncding::None map to\n> >>>\n> >>>    b) YcbcrEncding::Rec601 map to.\n> >>>\n> >>> For b) we have two contenders:\n> >>>\n> >>>     GST_VIDEO_COLOR_MATRIX_BT601\n> >>>     GST_VIDEO_COLOR_MATRIX_RGB\n> >> \n> >> b) is currently handled with:\n> >>\n> >> +       case ColorSpace::YcbcrEncoding::Rec601:\n> >> +               if (colorSpace == ColorSpace::Srgb)\n> >> +                       colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_RGB;\n> >> +               else\n> >> +                       colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT601;\n> >> +               break;\n> >>\n> >> Let's review this in v2 (submitting shortly)\n> > \n> > As per the above, I think YcbcrEncding::None should map to\n> > GST_VIDEO_COLOR_MATRIX_RGB and YcbcrEncding::Rec601 to\n> > GST_VIDEO_COLOR_MATRIX_BT601, unconditionally.\n> >\n> >>> For a) I think GST_VIDEO_COLOR_MATRIX_RGB would be better ?\n> >>>\n> >>> [1] https://git.linuxtv.org/media_tree.git/tree/include/uapi/linux/videodev2.h#n344\n> >>>\n> >>>>> +    case GST_VIDEO_COLOR_MATRIX_UNKNOWN:\n> >>>>> +        colorspace->ycbcrEncoding = ColorSpace::YcbcrEncoding::Default;\n> >>>>> +        break;\n> >>>>> +    default:\n> >>>>> +        GST_WARNING(\"Unknown 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> >>>>> +    case GST_VIDEO_COLOR_RANGE_UNKNOWN:\n> >>>>> +        colorspace->range = ColorSpace::Range::Default;\n> >>>>> +        break;\n> >>>>> +    default:\n> >>>>> +        GST_WARNING(\"Unknown range in colorimetry %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 +290,17 @@ 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 != nullptr)\n> >>>> \n> >>>>         if (colorimetry_str)\n> >>>>\n> >>>>> +            gst_structure_set(s, \"colorimetry\", G_TYPE_STRING, colorimetry_str, nullptr);\n> >>>>> +        else\n> >>>>> +            g_warning(\"libcamera::ColorSpace found but GstVideoColorimetry unknown\");\n> >>>>> +    }\n> >>>>> +\n> >>>>>       gst_caps_append_structure(caps, s);\n> >>>>>         return caps;\n> >>>>> @@ -222,6 +384,19 @@ 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> >>>>> + std::optional colorSpace = colorspace_from_colorimetry(&colorimetry);\n> >>>>> +            stream_cfg.colorSpace = colorSpace;\n> >>>> \n> >>>>             stream_cfg.colorSpace = colorspace_from_colorimetry(&colorimetry);\n> >>>>\n> >>>>> +        } else {\n> >>>>> +            g_print(\"Invalid colorimetry %s\", colorimetry_caps);\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 5F190C3275\n\tfor ;\n\tMon, 1 Aug 2022 13:17:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CEA3E6330F;\n\tMon, 1 Aug 2022 15:17:55 +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 0522E603E8\n\tfor ;\n\tMon, 1 Aug 2022 15:17:55 +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 4B39F2F3;\n\tMon, 1 Aug 2022 15:17:54 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659359875;\n\tbh=qdnXBP6n/qNPkOIBaX4kzm4iNzZCUAnC3h3/X8r7JTs=;\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=cLTpXPn5sNK5iGcIFojDDnhaCaH/SYu696H/Rdt51DiLI0JJTStwJro8iJJGzMKJw\n\tTGND2tMxUe8ozRtB/qaI2BVfp9DXTX1EVUEXywnWhVmG5K1ZHec0vxJVDGIXw6FWiT\n\tekDOTbmwtzs05taAiXnNwJ8hQPJTTsr0brOfDaXA+NLX1MAw0MSiK/8nsDQ74EaRm6\n\t/xDlefIT2gbRVadEhDQtGT4gtbMVlF1xEtn90i4PAVfYAGGju1K3Tj67aj4z6eXHTH\n\tefGfsfoVAuxWVGgM1Vi7J5TBx81owBLXvK4+JoYQOVpXzncjLoXoHyNvaZWDr5S9Bw\n\tbhiQ4vwKOSsXA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1659359874;\n\tbh=qdnXBP6n/qNPkOIBaX4kzm4iNzZCUAnC3h3/X8r7JTs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tJXHEw4lxZ1ukTarw9q9arTkKwB1u9cXsvVQKXqdNh02+rqci5+Fpj4YfE8lpL48c\n\tIpwpr8qY01J8L+bFqueRcbq68lYhZEsu/rLvCqlv3wsTrg2hwps3wSRpMcdMsYqK+Q\n\t83+rriulmVPP3YhqejQCkHM7WFwJOnhk7zQ1pTt4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"tJXHEw4l\"; dkim-atps=neutral","Date":"Mon, 1 Aug 2022 16:17:50 +0300","To":"Umang Jain ","Message-ID":"","References":"<20220724144355.104978-1-umang.jain@ideasonboard.com>\n\t<20220724144355.104978-4-umang.jain@ideasonboard.com>\n\t\n\t<09f1366c-5088-3daa-c31e-4f5597af2180@ideasonboard.com>\n\t<69342c1c-af1f-5451-4d0d-61693facc56c@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 3/3] gstreamer: Provide colorimetry <>\n\tlibcamera::ColorSpace 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\tnicolas.dufresne@collabora.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]