[PATCH v2] gstreamer: keep same transfer with that in negotiated caps
Nicolas Dufresne
nicolas at ndufresne.ca
Fri Dec 13 19:23:54 CET 2024
Hi,
Le vendredi 13 décembre 2024 à 15:03 +0900, Hou Qi a écrit :
> 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>
Looks good now. We could have improved the naming for the variable "transfer",
but can't think of a good suggestion, and it does not change anything to the
logic which I believe is right.
Reviewed-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
Thanks for you work,
Nicolas
> ---
> src/gstreamer/gstlibcamera-utils.cpp | 19 +++++++++++--------
> src/gstreamer/gstlibcamera-utils.h | 5 +++--
> src/gstreamer/gstlibcamerasrc.cpp | 6 ++++--
> 3 files changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> index 732987ef..23756b15 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)
> + colorimetry.transfer = transfer;
> break;
> }
>
> @@ -144,7 +146,7 @@ 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 +190,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 +382,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 +394,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 +409,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 +498,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..0f0d501e 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -433,12 +433,14 @@ static bool
> gst_libcamera_src_negotiate(GstLibcameraSrc *self)
> {
> GstLibcameraSrcState *state = self->state;
> + GstVideoTransferFunction transfer[state->srcpads_.size()];
>
> 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;
>
> /* Retrieve the supported caps. */
> g_autoptr(GstCaps) filter = gst_libcamera_stream_formats_to_caps(stream_cfg.formats());
> @@ -448,7 +450,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 +478,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)))
More information about the libcamera-devel
mailing list