| Message ID | 20241213060319.2877936-1-qi.hou@nxp.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@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@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)))
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@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@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@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)))
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@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@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@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)))
Hi Laurent Pinchart, I have sent out v3. Can you help check if build issue is gone? Thanks. Regards, Qi Hou -----Original Message----- From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Sent: 2024年12月15日 23:09 To: Nicolas Dufresne <nicolas@ndufresne.ca> Cc: Qi Hou <qi.hou@nxp.com>; libcamera-devel@lists.libcamera.org; Jared Hu <jared.hu@nxp.com>; Julien Vuillaumier <julien.vuillaumier@nxp.com> Subject: [EXT] Re: [PATCH v2] gstreamer: keep same transfer with that in negotiated caps Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F > > > gitlab.freedesktop.org%2Fgstreamer%2Fgst-plugins-base%2F-%2Fmerge_ > > > requests%2F724&data=05%7C02%7Cqi.hou%40nxp.com%7Cf5b9b27b3aeb4b700 > > > 78908dd1d1a779a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63869 > > > 8721704182571%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYi > > > OiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7 > > > C0%7C%7C%7C&sdata=4bYgndaOutdSPuXKFFFpy4P9gtZQet1R12rtYvCO3wE%3D&r > > > eserved=0 > > > > > > Bug: > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F > > > bugs.libcamera.org%2Fshow_bug.cgi%3Fid%3D150&data=05%7C02%7Cqi.hou > > > %40nxp.com%7Cf5b9b27b3aeb4b70078908dd1d1a779a%7C686ea1d3bc2b4c6fa9 > > > 2cd99c5c301635%7C0%7C0%7C638698721704200278%7CUnknown%7CTWFpbGZsb3 > > > d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFO > > > IjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=R29Q2iwz0%2Bnt9R4M > > > 4rc0aDzTMOhIuppPHNDcLs87WAo%3D&reserved=0 > > > Signed-off-by: Hou Qi <qi.hou@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@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@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
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)))
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@nxp.com> --- src/gstreamer/gstlibcamera-utils.cpp | 19 +++++++++++-------- src/gstreamer/gstlibcamera-utils.h | 5 +++-- src/gstreamer/gstlibcamerasrc.cpp | 6 ++++-- 3 files changed, 18 insertions(+), 12 deletions(-)