[{"id":32747,"web_url":"https://patchwork.libcamera.org/comment/32747/","msgid":"<20241216032522.GB32204@pendragon.ideasonboard.com>","date":"2024-12-16T03:25:22","subject":"Re: [PATCH v3] gstreamer: keep same transfer with that in negotiated\n\tcaps","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Dec 16, 2024 at 12:13:53PM +0900, Hou Qi wrote:\n> The conversions back and forth between GStreamer colorimetry and\n> libcamera color space are not invariant for the bt601 colorimetry.\n> The reason is that Rec709 transfer function defined in GStreamer\n> as GST_VIDEO_TRANSFER_BT709 (5), is to be replaced by its alias\n> GST_VIDEO_TRANSFER_BT601 (16) only for the case of bt601 (aka 2:4:16:4)\n> colorimetry - see [1].\n> \n> Currently the composition of the GStreamer/libcamera conversions:\n> colorimetry_from_colorspace (colorspace_from_colorimetry (bt601))\n> returns 2:4:5:4 instead of the expected 2:4:16:4 (bt601). This\n> causes negotiation error when the downstream element explicitly\n> expects bt601 colorimetry.\n> \n> Minimal example to reproduce the issue is with a pipeline handler\n> that do not set the optional color space in the stream configuration,\n> for instance vimc or imx8-isi:\n> export LIBCAMERA_PIPELINES_MATCH_LIST=\"vimc,imx8-isi\"\n> gst-launch-1.0 -v libcamerasrc ! video/x-raw,colorimetry=bt601 ! fakesink\n> \n> Above pipeline fails to start. This change memorizes downstream required\n> transfer function when mapped libcamera transfer is Rec709 in\n> gst_libcamera_configure_stream_from_caps(), and restores the transfer\n> function in gst_libcamera_stream_formats_to_caps().\n> \n> [1] https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/-/merge_requests/724\n> \n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=150\n> Signed-off-by: Hou Qi \n> ---\n> src/gstreamer/gstlibcamera-utils.cpp | 20 ++++++++++++--------\n> src/gstreamer/gstlibcamera-utils.h | 5 +++--\n> src/gstreamer/gstlibcamerasrc.cpp | 7 +++++--\n> 3 files changed, 20 insertions(+), 12 deletions(-)\n> \n> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> index 732987ef..a466b305 100644\n> --- a/src/gstreamer/gstlibcamera-utils.cpp\n> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> @@ -85,7 +85,7 @@ static struct {\n> };\n> \n> static GstVideoColorimetry\n> -colorimetry_from_colorspace(const ColorSpace &colorSpace)\n> +colorimetry_from_colorspace(const ColorSpace &colorSpace, GstVideoTransferFunction transfer)\n> {\n> \tGstVideoColorimetry colorimetry;\n> \n> @@ -113,6 +113,8 @@ colorimetry_from_colorspace(const ColorSpace &colorSpace)\n> \t\tbreak;\n> \tcase ColorSpace::TransferFunction::Rec709:\n> \t\tcolorimetry.transfer = GST_VIDEO_TRANSFER_BT709;\n> +\t\tif (transfer != GST_VIDEO_TRANSFER_UNKNOWN)\n> +\t\t\tcolorimetry.transfer = transfer;\n> \t\tbreak;\n> \t}\n> \n> @@ -144,7 +146,8 @@ colorimetry_from_colorspace(const ColorSpace &colorSpace)\n> }\n> \n> static std::optional\n> -colorspace_from_colorimetry(const GstVideoColorimetry &colorimetry)\n> +colorspace_from_colorimetry(const GstVideoColorimetry &colorimetry,\n> +\t\t\t GstVideoTransferFunction *transfer)\n> {\n> \tstd::optional colorspace = ColorSpace::Raw;\n> \n> @@ -188,6 +191,7 @@ colorspace_from_colorimetry(const GstVideoColorimetry &colorimetry)\n> \tcase GST_VIDEO_TRANSFER_BT2020_12:\n> \tcase GST_VIDEO_TRANSFER_BT709:\n> \t\tcolorspace->transferFunction = ColorSpace::TransferFunction::Rec709;\n> +\t\t*transfer = colorimetry.transfer;\n> \t\tbreak;\n> \tdefault:\n> \t\tGST_WARNING(\"Colorimetry transfer function %d not mapped in gstlibcamera\",\n> @@ -379,7 +383,8 @@ gst_libcamera_stream_formats_to_caps(const StreamFormats &formats)\n> }\n> \n> GstCaps *\n> -gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg)\n> +gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg,\n> +\t\t\t\t\t GstVideoTransferFunction transfer)\n> {\n> \tGstCaps *caps = gst_caps_new_empty();\n> \tGstStructure *s = bare_structure_from_format(stream_cfg.pixelFormat);\n> @@ -390,7 +395,7 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg\n> \t\t\t nullptr);\n> \n> \tif (stream_cfg.colorSpace) {\n> -\t\tGstVideoColorimetry colorimetry = colorimetry_from_colorspace(stream_cfg.colorSpace.value());\n> +\t\tGstVideoColorimetry colorimetry = colorimetry_from_colorspace(stream_cfg.colorSpace.value(), transfer);\n> \t\tg_autofree gchar *colorimetry_str = gst_video_colorimetry_to_string(&colorimetry);\n> \n> \t\tif (colorimetry_str)\n> @@ -405,9 +410,8 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg\n> \treturn caps;\n> }\n> \n> -void\n> -gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,\n> -\t\t\t\t\t GstCaps *caps)\n> +void gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,\n> +\t\t\t\t\t GstCaps *caps, GstVideoTransferFunction *transfer)\n> {\n> \tGstVideoFormat gst_format = pixel_format_to_gst_format(stream_cfg.pixelFormat);\n> \tguint i;\n> @@ -495,7 +499,7 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,\n> \t\tif (!gst_video_colorimetry_from_string(&colorimetry, colorimetry_str))\n> \t\t\tg_critical(\"Invalid colorimetry %s\", colorimetry_str);\n> \n> -\t\tstream_cfg.colorSpace = colorspace_from_colorimetry(colorimetry);\n> +\t\tstream_cfg.colorSpace = colorspace_from_colorimetry(colorimetry, transfer);\n> \t}\n> }\n> \n> diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h\n> index cab1c814..4978987c 100644\n> --- a/src/gstreamer/gstlibcamera-utils.h\n> +++ b/src/gstreamer/gstlibcamera-utils.h\n> @@ -16,9 +16,10 @@\n> #include \n> \n> GstCaps *gst_libcamera_stream_formats_to_caps(const libcamera::StreamFormats &formats);\n> -GstCaps *gst_libcamera_stream_configuration_to_caps(const libcamera::StreamConfiguration &stream_cfg);\n> +GstCaps *gst_libcamera_stream_configuration_to_caps(const libcamera::StreamConfiguration &stream_cfg,\n> +\t\t\t\t\t\t GstVideoTransferFunction transfer);\n> void gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration &stream_cfg,\n> -\t\t\t\t\t GstCaps *caps);\n> +\t\t\t\t\t GstCaps *caps, GstVideoTransferFunction *transfer);\n> void gst_libcamera_get_framerate_from_caps(GstCaps *caps, GstStructure *element_caps);\n> void gst_libcamera_clamp_and_set_frameduration(libcamera::ControlList &controls,\n> \t\t\t\t\t const libcamera::ControlInfoMap &camera_controls,\n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index 8efa25f4..c701b3cf 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -433,12 +433,15 @@ static bool\n> gst_libcamera_src_negotiate(GstLibcameraSrc *self)\n> {\n> \tGstLibcameraSrcState *state = self->state;\n> +\tgsize srcpads_num = state->srcpads_.size();\n> +\tGstVideoTransferFunction transfer[srcpads_num];\n\nThis still fails with clang-19:\n\n../../src/gstreamer/gstlibcamerasrc.cpp:437:36: error: variable length arrays in C++ are a Clang extension [-Werror,-Wvla-cxx-extension]\n 437 | GstVideoTransferFunction transfer[srcpads_num];\n | ^~~~~~~~~~~\n../../src/gstreamer/gstlibcamerasrc.cpp:437:36: note: read of non-const variable 'srcpads_num' is not allowed in a constant expression\n../../src/gstreamer/gstlibcamerasrc.cpp:436:8: note: declared here\n 436 | gsize srcpads_num = state->srcpads_.size();\n | ^\n\n\tstd::vector transfer(state->srcpads_.size(),\n\t\t\t\t\t\t GST_VIDEO_TRANSFER_UNKNOWN);\n\nshould do. This will initialize all the elements of the vector, so ...\n\n> \n> \tg_autoptr(GstStructure) element_caps = gst_structure_new_empty(\"caps\");\n> \n> \tfor (gsize i = 0; i < state->srcpads_.size(); i++) {\n> \t\tGstPad *srcpad = state->srcpads_[i];\n> \t\tStreamConfiguration &stream_cfg = state->config_->at(i);\n> +\t\ttransfer[i] = GST_VIDEO_TRANSFER_UNKNOWN;\n\n... you can drop this.\n\n> \n> \t\t/* Retrieve the supported caps. */\n> \t\tg_autoptr(GstCaps) filter = gst_libcamera_stream_formats_to_caps(stream_cfg.formats());\n> @@ -448,7 +451,7 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)\n> \n> \t\t/* Fixate caps and configure the stream. */\n> \t\tcaps = gst_caps_make_writable(caps);\n> -\t\tgst_libcamera_configure_stream_from_caps(stream_cfg, caps);\n> +\t\tgst_libcamera_configure_stream_from_caps(stream_cfg, caps, &transfer[i]);\n> \t\tgst_libcamera_get_framerate_from_caps(caps, element_caps);\n> \t}\n> \n> @@ -476,7 +479,7 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)\n> \t\tGstPad *srcpad = state->srcpads_[i];\n> \t\tconst StreamConfiguration &stream_cfg = state->config_->at(i);\n> \n> -\t\tg_autoptr(GstCaps) caps = gst_libcamera_stream_configuration_to_caps(stream_cfg);\n> +\t\tg_autoptr(GstCaps) caps = gst_libcamera_stream_configuration_to_caps(stream_cfg, transfer[i]);\n> \t\tgst_libcamera_framerate_to_caps(caps, element_caps);\n> \n> \t\tif (!gst_pad_push_event(srcpad, gst_event_new_caps(caps)))","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 F2DD3C32F6\n\tfor ;\n\tMon, 16 Dec 2024 03:25:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2859967F34;\n\tMon, 16 Dec 2024 04:25:41 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 454D867F2E\n\tfor ;\n\tMon, 16 Dec 2024 04:25:39 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C0EFE675;\n\tMon, 16 Dec 2024 04:25:02 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"VM39rtsN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1734319503;\n\tbh=QXn6x+bNB7iZHBGPakTE4eVE/a0/TP2Plxb450qx63M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VM39rtsNaidz5hasHFa34itZA7+YgKs/f+T0d4ceooQOwuxasIWe5tZiTJSerRsFX\n\tcvT7ERKkC7LGkKTRkW/uO1UPp7j6QOPDP56/u/KgdQjsdQQS2x5ja9aSly0le7KzSD\n\tcQWYP0pbfYFqPpOTgnC3DPURItwtNrzLdfq28XnM=","Date":"Mon, 16 Dec 2024 05:25:22 +0200","From":"Laurent Pinchart ","To":"Hou Qi ","Cc":"libcamera-devel@lists.libcamera.org, jared.hu@nxp.com,\n\tjulien.vuillaumier@nxp.com","Subject":"Re: [PATCH v3] gstreamer: keep same transfer with that in negotiated\n\tcaps","Message-ID":"<20241216032522.GB32204@pendragon.ideasonboard.com>","References":"<20241216031353.1622924-1-qi.hou@nxp.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241216031353.1622924-1-qi.hou@nxp.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]