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

[Laurent Pinchart]

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" ?

> +
> +		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 ?

		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 ?

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

-- 
Regards,

Laurent Pinchart