[PATCH v2] gstreamer: keep same transfer with that in negotiated caps
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Dec 15 16:09:10 CET 2024
I'm afraid this patch breaks compilation with clang 18 and 19 :-(
../../src/gstreamer/gstlibcamerasrc.cpp:436:36: error: variable length arrays in C++ are a Clang extension [-Werror,-Wvla-cxx-extension]
436 | GstVideoTransferFunction transfer[state->srcpads_.size()];
| ^~~~~~~~~~~~~~~~~~~~~~
../../src/gstreamer/gstlibcamerasrc.cpp:436:36: note: read of non-constexpr variable 'state' is not allowed in a constant expression
../../src/gstreamer/gstlibcamerasrc.cpp:435:24: note: declared here
435 | GstLibcameraSrcState *state = self->state;
|
I've now enabled build tests with clang 19 in CI, see
https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1330267 for
a full test report.
Hou, could I ask you for a v3 that addresses those issues, as well as
the couple of comments below ?
On Sun, Dec 15, 2024 at 04:04:28PM +0200, Laurent Pinchart wrote:
> On Fri, Dec 13, 2024 at 01:23:54PM -0500, Nicolas Dufresne wrote:
> > 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