[PATCH v3] gstreamer: keep same transfer with that in negotiated caps
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Dec 16 04:25:22 CET 2024
On Mon, Dec 16, 2024 at 12:13:53PM +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 memorizes downstream required
> transfer function when mapped libcamera transfer is Rec709 in
> gst_libcamera_configure_stream_from_caps(), and restores the transfer
> function in gst_libcamera_stream_formats_to_caps().
>
> [1] https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/-/merge_requests/724
>
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=150
> Signed-off-by: Hou Qi <qi.hou at nxp.com>
> ---
> src/gstreamer/gstlibcamera-utils.cpp | 20 ++++++++++++--------
> src/gstreamer/gstlibcamera-utils.h | 5 +++--
> src/gstreamer/gstlibcamerasrc.cpp | 7 +++++--
> 3 files changed, 20 insertions(+), 12 deletions(-)
>
> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> index 732987ef..a466b305 100644
> --- a/src/gstreamer/gstlibcamera-utils.cpp
> +++ b/src/gstreamer/gstlibcamera-utils.cpp
> @@ -85,7 +85,7 @@ static struct {
> };
>
> static GstVideoColorimetry
> -colorimetry_from_colorspace(const ColorSpace &colorSpace)
> +colorimetry_from_colorspace(const ColorSpace &colorSpace, GstVideoTransferFunction transfer)
> {
> GstVideoColorimetry colorimetry;
>
> @@ -113,6 +113,8 @@ colorimetry_from_colorspace(const ColorSpace &colorSpace)
> break;
> case ColorSpace::TransferFunction::Rec709:
> colorimetry.transfer = GST_VIDEO_TRANSFER_BT709;
> + if (transfer != GST_VIDEO_TRANSFER_UNKNOWN)
> + colorimetry.transfer = transfer;
> break;
> }
>
> @@ -144,7 +146,8 @@ colorimetry_from_colorspace(const ColorSpace &colorSpace)
> }
>
> static std::optional<ColorSpace>
> -colorspace_from_colorimetry(const GstVideoColorimetry &colorimetry)
> +colorspace_from_colorimetry(const GstVideoColorimetry &colorimetry,
> + GstVideoTransferFunction *transfer)
> {
> std::optional<ColorSpace> colorspace = ColorSpace::Raw;
>
> @@ -188,6 +191,7 @@ colorspace_from_colorimetry(const GstVideoColorimetry &colorimetry)
> case GST_VIDEO_TRANSFER_BT2020_12:
> case GST_VIDEO_TRANSFER_BT709:
> colorspace->transferFunction = ColorSpace::TransferFunction::Rec709;
> + *transfer = colorimetry.transfer;
> break;
> default:
> GST_WARNING("Colorimetry transfer function %d not mapped in gstlibcamera",
> @@ -379,7 +383,8 @@ gst_libcamera_stream_formats_to_caps(const StreamFormats &formats)
> }
>
> GstCaps *
> -gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg)
> +gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg,
> + GstVideoTransferFunction transfer)
> {
> GstCaps *caps = gst_caps_new_empty();
> GstStructure *s = bare_structure_from_format(stream_cfg.pixelFormat);
> @@ -390,7 +395,7 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg
> nullptr);
>
> if (stream_cfg.colorSpace) {
> - GstVideoColorimetry colorimetry = colorimetry_from_colorspace(stream_cfg.colorSpace.value());
> + GstVideoColorimetry colorimetry = colorimetry_from_colorspace(stream_cfg.colorSpace.value(), transfer);
> g_autofree gchar *colorimetry_str = gst_video_colorimetry_to_string(&colorimetry);
>
> if (colorimetry_str)
> @@ -405,9 +410,8 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg
> return caps;
> }
>
> -void
> -gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,
> - GstCaps *caps)
> +void gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,
> + GstCaps *caps, GstVideoTransferFunction *transfer)
> {
> GstVideoFormat gst_format = pixel_format_to_gst_format(stream_cfg.pixelFormat);
> guint i;
> @@ -495,7 +499,7 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,
> if (!gst_video_colorimetry_from_string(&colorimetry, colorimetry_str))
> g_critical("Invalid colorimetry %s", colorimetry_str);
>
> - stream_cfg.colorSpace = colorspace_from_colorimetry(colorimetry);
> + stream_cfg.colorSpace = colorspace_from_colorimetry(colorimetry, transfer);
> }
> }
>
> diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h
> index cab1c814..4978987c 100644
> --- a/src/gstreamer/gstlibcamera-utils.h
> +++ b/src/gstreamer/gstlibcamera-utils.h
> @@ -16,9 +16,10 @@
> #include <gst/video/video.h>
>
> GstCaps *gst_libcamera_stream_formats_to_caps(const libcamera::StreamFormats &formats);
> -GstCaps *gst_libcamera_stream_configuration_to_caps(const libcamera::StreamConfiguration &stream_cfg);
> +GstCaps *gst_libcamera_stream_configuration_to_caps(const libcamera::StreamConfiguration &stream_cfg,
> + GstVideoTransferFunction transfer);
> void gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration &stream_cfg,
> - GstCaps *caps);
> + GstCaps *caps, GstVideoTransferFunction *transfer);
> void gst_libcamera_get_framerate_from_caps(GstCaps *caps, GstStructure *element_caps);
> void gst_libcamera_clamp_and_set_frameduration(libcamera::ControlList &controls,
> const libcamera::ControlInfoMap &camera_controls,
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index 8efa25f4..c701b3cf 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -433,12 +433,15 @@ static bool
> gst_libcamera_src_negotiate(GstLibcameraSrc *self)
> {
> GstLibcameraSrcState *state = self->state;
> + gsize srcpads_num = state->srcpads_.size();
> + GstVideoTransferFunction transfer[srcpads_num];
This still fails with clang-19:
../../src/gstreamer/gstlibcamerasrc.cpp:437:36: error: variable length arrays in C++ are a Clang extension [-Werror,-Wvla-cxx-extension]
437 | GstVideoTransferFunction transfer[srcpads_num];
| ^~~~~~~~~~~
../../src/gstreamer/gstlibcamerasrc.cpp:437:36: note: read of non-const variable 'srcpads_num' is not allowed in a constant expression
../../src/gstreamer/gstlibcamerasrc.cpp:436:8: note: declared here
436 | gsize srcpads_num = state->srcpads_.size();
| ^
std::vector<GstVideoTransferFunction> transfer(state->srcpads_.size(),
GST_VIDEO_TRANSFER_UNKNOWN);
should do. This will initialize all the elements of the vector, so ...
>
> g_autoptr(GstStructure) element_caps = gst_structure_new_empty("caps");
>
> for (gsize i = 0; i < state->srcpads_.size(); i++) {
> GstPad *srcpad = state->srcpads_[i];
> StreamConfiguration &stream_cfg = state->config_->at(i);
> + transfer[i] = GST_VIDEO_TRANSFER_UNKNOWN;
... you can drop this.
>
> /* Retrieve the supported caps. */
> g_autoptr(GstCaps) filter = gst_libcamera_stream_formats_to_caps(stream_cfg.formats());
> @@ -448,7 +451,7 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)
>
> /* Fixate caps and configure the stream. */
> caps = gst_caps_make_writable(caps);
> - gst_libcamera_configure_stream_from_caps(stream_cfg, caps);
> + gst_libcamera_configure_stream_from_caps(stream_cfg, caps, &transfer[i]);
> gst_libcamera_get_framerate_from_caps(caps, element_caps);
> }
>
> @@ -476,7 +479,7 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)
> GstPad *srcpad = state->srcpads_[i];
> const StreamConfiguration &stream_cfg = state->config_->at(i);
>
> - g_autoptr(GstCaps) caps = gst_libcamera_stream_configuration_to_caps(stream_cfg);
> + g_autoptr(GstCaps) caps = gst_libcamera_stream_configuration_to_caps(stream_cfg, transfer[i]);
> gst_libcamera_framerate_to_caps(caps, element_caps);
>
> if (!gst_pad_push_event(srcpad, gst_event_new_caps(caps)))
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list