[PATCH v4] gstreamer: keep same transfer with that in negotiated caps
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Dec 16 14:27:15 CET 2024
On Mon, Dec 16, 2024 at 03:12:06PM +0200, Laurent Pinchart wrote:
> Hi Hou,
>
> Thank you for the patch.
>
> On Mon, Dec 16, 2024 at 01:30:44PM +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>
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> Nicolas has also provided a R-b tag in v2, which I think is still
> applicable, so I'll add it to the commit.
Patch merged in the master branch.
> > ---
> > src/gstreamer/gstlibcamera-utils.cpp | 20 ++++++++++++--------
> > src/gstreamer/gstlibcamera-utils.h | 5 +++--
> > src/gstreamer/gstlibcamerasrc.cpp | 6 ++++--
> > 3 files changed, 19 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..5e9e843d 100644
> > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > @@ -433,6 +433,8 @@ static bool
> > gst_libcamera_src_negotiate(GstLibcameraSrc *self)
> > {
> > GstLibcameraSrcState *state = self->state;
> > + std::vector<GstVideoTransferFunction> transfer(state->srcpads_.size(),
> > + GST_VIDEO_TRANSFER_UNKNOWN);
> >
> > g_autoptr(GstStructure) element_caps = gst_structure_new_empty("caps");
> >
> > @@ -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