[EXT] Re: [PATCH] gstreamer: Read correct colorimetry from caps into the stream configuration

[Nicolas Dufresne]

Le mercredi 13 novembre 2024 à 10:20 +0000, Qi Hou a écrit :
> Hi Nicolas,
> 
> I see there is already assert check in gst_libcamera_configure_stream_from_caps().
> 
> 	/* First fixate the caps using default configuration value. */
> 	g_assert(gst_caps_is_writable(caps));

Oh, sorry about this indirection, new comment below.

> 
> Regards,
> Qi Hou
> 
> -----Original Message-----
> From: Nicolas Dufresne <nicolas at ndufresne.ca> 
> Sent: 2024年11月13日 2:03
> To: Qi Hou <qi.hou at nxp.com>; libcamera-devel at lists.libcamera.org
> Cc: Jared Hu <jared.hu at nxp.com>; Julien Vuillaumier <julien.vuillaumier at nxp.com>
> Subject: [EXT] Re: [PATCH] gstreamer: Read correct colorimetry from caps into the stream configuration
> 
> 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
> 
> 
> Le vendredi 08 novembre 2024 à 18:12 +0900, Hou Qi a écrit :
> > When libcamerasrc is negotiating with downstream element, it first 
> > extracts colorimetry field from downstream supported caps, then set 
> > this colorimetry to its stream configuration and propagates the 
> > colorimetry downstream.
> > 
> > Currently libamerasrc only considers the case there is one colorimetry 
> > in colorimetry field of downstream caps. But the issue is that 
> > downstream caps may report a list of supported colorimetry, which 
> > causes libcamerasrc to set unknown colorimetry to stream configuration 
> > and negotiate fail with downstream element.
> > 
> > In order to fix the issue, get the first string if colorimetry field 
> > holds string list to set it to stream configuration.
> > 
> > Signed-off-by: Hou Qi <qi.hou at nxp.com>
> > ---
> >  src/gstreamer/gstlibcamera-utils.cpp | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/gstreamer/gstlibcamera-utils.cpp 
> > b/src/gstreamer/gstlibcamera-utils.cpp
> > index 732987ef..2c1ebce8 100644
> > --- a/src/gstreamer/gstlibcamera-utils.cpp
> > +++ b/src/gstreamer/gstlibcamera-utils.cpp
> > @@ -489,9 +489,22 @@ 
> > gst_libcamera_configure_stream_from_caps(StreamConfiguration 
> > &stream_cfg,
> > 
> >       /* Configure colorimetry */
> >       if (gst_structure_has_field(s, "colorimetry")) {
> > -             const gchar *colorimetry_str = gst_structure_get_string(s, "colorimetry");
> > +             const GValue *mode;
> > +             const gchar *colorimetry_str = NULL;
> >               GstVideoColorimetry colorimetry;
> > 
> > +             mode = gst_structure_get_value(s, "colorimetry");

Just do:
		gst_structure_fixate_field (s, "colorimetry");

And keep the original code. The caps implementation seems sub-optimal though. In
an ideal world, we'd do this in stage:

- Fixate the caps
- So caps to GstVideoInfo
- Fill the stream_cfg from the VideoInfo
- Finally do GstVideoInfo to caps in order to push complete caps.

This will fill the proper default of the other video/x-raw field we are not
careful about, like stereo views, pixel-aspect ratio, etc. If you have time,
this can greatly improve caps negotiation an correctness for future users.

> > +
> > +             if (G_VALUE_HOLDS_STRING(mode)) {
> > +                     colorimetry_str = gst_structure_get_string(s, "colorimetry");
> > +             } else if (GST_VALUE_HOLDS_LIST(mode)) {
> > +                     const GValue *first_element = 
> > + gst_value_list_get_value(mode, 0);
> > +
> > +                     if (G_VALUE_HOLDS_STRING(first_element)) {
> > +                             colorimetry_str = g_value_get_string(first_element);
> > +                     }
> > +             }
> > +
> 
> gst_libcamera_configure_stream_from_caps() is exepected to be called with fixed caps. Feel free to add an assert on that, fixation should happen earlier instead. This could be as simple as gst_caps_fixate(), specially that you are picking the first value in the list.
> 
> Nicolas
> 
> >               if (!gst_video_colorimetry_from_string(&colorimetry, colorimetry_str))
> >                       g_critical("Invalid colorimetry %s", 
> > colorimetry_str);
> > 
>