gstreamer: Read correct colorimetry from caps into the stream configuration
diff mbox series

Message ID 20241108091246.3753388-1-qi.hou@nxp.com
State New
Headers show
Series
  • gstreamer: Read correct colorimetry from caps into the stream configuration
Related show

Commit Message

Qi Hou Nov. 8, 2024, 9:12 a.m. UTC 
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@nxp.com>
---
 src/gstreamer/gstlibcamera-utils.cpp | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

Comments

Laurent Pinchart Nov. 12, 2024, 8:34 a.m. UTC | #1 
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@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);
>
Nicolas Dufresne Nov. 12, 2024, 6:02 p.m. UTC | #2 
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@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);
>

Patch
diff mbox series 

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);
+			}
+		}
+
 		if (!gst_video_colorimetry_from_string(&colorimetry, colorimetry_str))
 			g_critical("Invalid colorimetry %s", colorimetry_str);