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

[Qi Hou]

Hi Laurent Pinchart,

Please check my comments. Thanks for your advice. I have sent out v2.

Regards,
Qi Hou

-----Original Message-----
From: Laurent Pinchart <laurent.pinchart at ideasonboard.com> 
Sent: 2024年11月12日 16:35
To: Qi Hou <qi.hou at nxp.com>
Cc: libcamera-devel at lists.libcamera.org; Jared Hu <jared.hu at nxp.com>; Julien Vuillaumier <julien.vuillaumier at nxp.com>; Nicolas Dufresne <nicolas at ndufresne.ca>
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


Hi Hou,

(CC'ing Nicolas Dufresne)

Thank you for the patch.

On Fri, Nov 08, 2024 at 06:12:46PM +0900, Hou Qi wrote:
> 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");

"mode" seems a weird name, how about calling it "value" ?
[Qi Hou] ok, I renamed to "value".

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

No need for curly brackets here.

> +             }

Would the following be simpler and more readable ?
[Qi Hou] Yes, thanks for your advice.

                if (GST_VALUE_HOLDS_LIST(value))
                        value = gst_value_list_get_value(value, 0);
                if (G_VALUE_HOLDS_STRING(value))
                        colorimetry_str = g_value_get_string(value);

What happens if neither condition is true though, will
gst_video_colorimetry_from_string() handle a null colorimetry_str gracefully ?
[Qi Hou] If colorimetry_str is NULL, gst_video_colorimetry_from_string() return default unknown colorimetry 1:1:1:0.

> +
>               if (!gst_video_colorimetry_from_string(&colorimetry, colorimetry_str))
>                       g_critical("Invalid colorimetry %s", 
> colorimetry_str);
>

--
Regards,

Laurent Pinchart