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

[Qi Hou]

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));

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");
> +
> +             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);
>