[PATCH v2] gstreamer: keep same transfer with that in negotiated caps
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Dec 15 15:04:28 CET 2024
On Fri, Dec 13, 2024 at 01:23:54PM -0500, Nicolas Dufresne wrote:
> 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.
I have a few extra comments, please see below.
> 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)
I'd write
if (transfer != GST_VIDEO_TRANSFER_UNKNOWN)
to avoid depending on the fact that GST_VIDEO_TRANSFER_UNKNOWN == 0.
> > + 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)
This should be wrapped:
colorspace_from_colorimetry(const GstVideoColorimetry &colorimetry,
GstVideoTransferFunction *transfer)
No need for a v3, I'll make those changes when applying.
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > {
> > 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)))
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list