| Message ID | 20241108091203.3751671-1-qi.hou@nxp.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Hou, (how should I address you, is "Hou" correct ?) Thank you for the patch. On Fri, Nov 08, 2024 at 06:12:03PM +0900, Hou Qi wrote: > The conversions back and forth between GStreamer colorimetry and > libcamera color space are not invariant for the bt601 colorimetry. > The reason is that Rec709 transfer function defined in GStreamer > as GST_VIDEO_TRANSFER_BT709 (5), is to be replaced by its alias > GST_VIDEO_TRANSFER_BT601 (16) only for the case of bt601 (aka 2:4:16:4) > colorimetry - see [1]. > > Currently the composition of the GStreamer/libcamera conversions: > colorimetry_from_colorspace (colorspace_from_colorimetry (bt601)) > returns 2:4:5:4 instead of the expected 2:4:16:4 (bt601). This > causes negotiation error when the downstream element explicitly > expects bt601 colorimetry. > > Minimal example to reproduce the issue is with a pipeline handler > that do not set the optional color space in the stream configuration, > for instance vimc or imx8-isi: > export LIBCAMERA_PIPELINES_MATCH_LIST="vimc,imx8-isi" > gst-launch-1.0 -v libcamerasrc ! video/x-raw,colorimetry=bt601 ! fakesink > > Above pipeline fails to start. This change updates colorimetry_from_colorspace() > to use the expected GST_VIDEO_TRANSFER_BT601 (16) alternative > definition for the Rec709 transfer function, in order to fix the > conversion from libcamera color space towards the bt601 colorimetry. > This bt601 colorimetry special case is identified by the usage of > the Rec709 transfer function combined with the primaries > GST_VIDEO_COLOR_PRIMARIES_SMPTE170M (4). > > [1] https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/-/merge_requests/724 > Unless I'm mistaken, this is related to https://bugs.libcamera.org/show_bug.cgi?id=150, so you can add Bug: https://bugs.libcamera.org/show_bug.cgi?id=150 > Signed-off-by: Hou Qi <qi.hou@nxp.com> > --- > src/gstreamer/gstlibcamera-utils.cpp | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp > index 732987ef..b2d64733 100644 > --- a/src/gstreamer/gstlibcamera-utils.cpp > +++ b/src/gstreamer/gstlibcamera-utils.cpp > @@ -112,7 +112,14 @@ colorimetry_from_colorspace(const ColorSpace &colorSpace) > colorimetry.transfer = GST_VIDEO_TRANSFER_SRGB; > break; > case ColorSpace::TransferFunction::Rec709: > +#if GST_CHECK_VERSION(1, 18, 0) > + if (colorSpace.primaries == ColorSpace::Primaries::Smpte170m) > + colorimetry.transfer = GST_VIDEO_TRANSFER_BT601; > + else > + colorimetry.transfer = GST_VIDEO_TRANSFER_BT709; > +#else > colorimetry.transfer = GST_VIDEO_TRANSFER_BT709; > +#endif Won't this patch break caps negotiation if the downstream element explicitly expects 2:4:5:4 ? Given that GST_VIDEO_TRANSFER_BT601 and GST_VIDEO_TRANSFER_BT709 are synonyms, Nicolas recommended in bug 150 to memory the GStreamer transfer function requested by GStreamer, and restore it in case libcamera returns an equivalent. The negotiation is implemented in gst_libcamera_src_negotiate(), using the gst_libcamera_configure_stream_from_caps() and gst_libcamera_stream_configuration_to_caps() functions. The former could return the GstVideoColorimetry retrieved from the caps, which could then be passed to the latter. There may be better options regarding how to architecture the code. > break; > } >
Hi Laurent Pinchart, Thanks for your reveiw. I have sent out "[PATCH v2] gstreamer: keep same transfer with that in negotiated caps". Regards, Qi Hou -----Original Message----- From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Sent: 2024年11月12日 16:27 To: Qi Hou <qi.hou@nxp.com> Cc: libcamera-devel@lists.libcamera.org; Jared Hu <jared.hu@nxp.com>; Julien Vuillaumier <julien.vuillaumier@nxp.com>; Nicolas Dufresne <nicolas.dufresne@collabora.com> Subject: [EXT] Re: [PATCH] gstreamer: Handle GST_VIDEO_TRANSFER_BT601 Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button Hi Hou, (how should I address you, is "Hou" correct ?) Thank you for the patch. On Fri, Nov 08, 2024 at 06:12:03PM +0900, Hou Qi wrote: > The conversions back and forth between GStreamer colorimetry and > libcamera color space are not invariant for the bt601 colorimetry. > The reason is that Rec709 transfer function defined in GStreamer as > GST_VIDEO_TRANSFER_BT709 (5), is to be replaced by its alias > GST_VIDEO_TRANSFER_BT601 (16) only for the case of bt601 (aka > 2:4:16:4) colorimetry - see [1]. > > Currently the composition of the GStreamer/libcamera conversions: > colorimetry_from_colorspace (colorspace_from_colorimetry (bt601)) > returns 2:4:5:4 instead of the expected 2:4:16:4 (bt601). This causes > negotiation error when the downstream element explicitly expects bt601 > colorimetry. > > Minimal example to reproduce the issue is with a pipeline handler that > do not set the optional color space in the stream configuration, for > instance vimc or imx8-isi: > export LIBCAMERA_PIPELINES_MATCH_LIST="vimc,imx8-isi" > gst-launch-1.0 -v libcamerasrc ! video/x-raw,colorimetry=bt601 ! > fakesink > > Above pipeline fails to start. This change updates > colorimetry_from_colorspace() to use the expected > GST_VIDEO_TRANSFER_BT601 (16) alternative definition for the Rec709 > transfer function, in order to fix the conversion from libcamera color space towards the bt601 colorimetry. > This bt601 colorimetry special case is identified by the usage of the > Rec709 transfer function combined with the primaries > GST_VIDEO_COLOR_PRIMARIES_SMPTE170M (4). > > [1] > https://gitl/ > ab.freedesktop.org%2Fgstreamer%2Fgst-plugins-base%2F-%2Fmerge_requests > %2F724&data=05%7C02%7Cqi.hou%40nxp.com%7Cc059dc8be9984c6056ca08dd02f3c > e8e%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638669968373879121%7C > Unknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAi > OiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=QCQiga > yl9hUO1cOM%2BkyfyOlenlylyAyEkxeyWI6%2FAd0%3D&reserved=0 > Unless I'm mistaken, this is related to https://bugs.libcamera.org/show_bug.cgi?id=150, so you can add Bug: https://bugs.libcamera.org/show_bug.cgi?id=150 > Signed-off-by: Hou Qi <qi.hou@nxp.com> > --- > src/gstreamer/gstlibcamera-utils.cpp | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp > b/src/gstreamer/gstlibcamera-utils.cpp > index 732987ef..b2d64733 100644 > --- a/src/gstreamer/gstlibcamera-utils.cpp > +++ b/src/gstreamer/gstlibcamera-utils.cpp > @@ -112,7 +112,14 @@ colorimetry_from_colorspace(const ColorSpace &colorSpace) > colorimetry.transfer = GST_VIDEO_TRANSFER_SRGB; > break; > case ColorSpace::TransferFunction::Rec709: > +#if GST_CHECK_VERSION(1, 18, 0) > + if (colorSpace.primaries == ColorSpace::Primaries::Smpte170m) > + colorimetry.transfer = GST_VIDEO_TRANSFER_BT601; > + else > + colorimetry.transfer = GST_VIDEO_TRANSFER_BT709; > +#else > colorimetry.transfer = GST_VIDEO_TRANSFER_BT709; > +#endif Won't this patch break caps negotiation if the downstream element explicitly expects 2:4:5:4 ? Given that GST_VIDEO_TRANSFER_BT601 and GST_VIDEO_TRANSFER_BT709 are synonyms, Nicolas recommended in bug 150 to memory the GStreamer transfer function requested by GStreamer, and restore it in case libcamera returns an equivalent. The negotiation is implemented in gst_libcamera_src_negotiate(), using the gst_libcamera_configure_stream_from_caps() and gst_libcamera_stream_configuration_to_caps() functions. The former could return the GstVideoColorimetry retrieved from the caps, which could then be passed to the latter. There may be better options regarding how to architecture the code. > break; > } > -- Regards, Laurent Pinchart
diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp index 732987ef..b2d64733 100644 --- a/src/gstreamer/gstlibcamera-utils.cpp +++ b/src/gstreamer/gstlibcamera-utils.cpp @@ -112,7 +112,14 @@ colorimetry_from_colorspace(const ColorSpace &colorSpace) colorimetry.transfer = GST_VIDEO_TRANSFER_SRGB; break; case ColorSpace::TransferFunction::Rec709: +#if GST_CHECK_VERSION(1, 18, 0) + if (colorSpace.primaries == ColorSpace::Primaries::Smpte170m) + colorimetry.transfer = GST_VIDEO_TRANSFER_BT601; + else + colorimetry.transfer = GST_VIDEO_TRANSFER_BT709; +#else colorimetry.transfer = GST_VIDEO_TRANSFER_BT709; +#endif break; }
The conversions back and forth between GStreamer colorimetry and libcamera color space are not invariant for the bt601 colorimetry. The reason is that Rec709 transfer function defined in GStreamer as GST_VIDEO_TRANSFER_BT709 (5), is to be replaced by its alias GST_VIDEO_TRANSFER_BT601 (16) only for the case of bt601 (aka 2:4:16:4) colorimetry - see [1]. Currently the composition of the GStreamer/libcamera conversions: colorimetry_from_colorspace (colorspace_from_colorimetry (bt601)) returns 2:4:5:4 instead of the expected 2:4:16:4 (bt601). This causes negotiation error when the downstream element explicitly expects bt601 colorimetry. Minimal example to reproduce the issue is with a pipeline handler that do not set the optional color space in the stream configuration, for instance vimc or imx8-isi: export LIBCAMERA_PIPELINES_MATCH_LIST="vimc,imx8-isi" gst-launch-1.0 -v libcamerasrc ! video/x-raw,colorimetry=bt601 ! fakesink Above pipeline fails to start. This change updates colorimetry_from_colorspace() to use the expected GST_VIDEO_TRANSFER_BT601 (16) alternative definition for the Rec709 transfer function, in order to fix the conversion from libcamera color space towards the bt601 colorimetry. This bt601 colorimetry special case is identified by the usage of the Rec709 transfer function combined with the primaries GST_VIDEO_COLOR_PRIMARIES_SMPTE170M (4). [1] https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/-/merge_requests/724 Signed-off-by: Hou Qi <qi.hou@nxp.com> --- src/gstreamer/gstlibcamera-utils.cpp | 7 +++++++ 1 file changed, 7 insertions(+)