[PATCH v2] gstreamer: Fix critical warning "gst_value_set_int_range_step: assertion 'start < end' failed"

[Nicolas Dufresne]

Hi,

Le jeudi 25 juillet 2024 à 18:26 +0900, Hou Qi a écrit :
> This changes is fixing critical error message
> "gst_value_set_int_range_step: assertion 'start < end' failed" observed
> when building GStreamer caps from a stream configuration whose size
> range holds a single size.
> 
> GStreamer range step definition requires distinct min and max values
> definitions, otherwise above error message is output.
> 
> libcamera SizeRange instance may return a single size leading to
> identical min and max values. Add a conditional check where the min and
> max of the range are distinct during iterating the supported sizes for
> each pixelformat.
> 
> Signed-off-by: Hou Qi <qi.hou at nxp.com>
> ---
>  src/gstreamer/gstlibcamera-utils.cpp | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> index ec4da435..79f71246 100644
> --- a/src/gstreamer/gstlibcamera-utils.cpp
> +++ b/src/gstreamer/gstlibcamera-utils.cpp
> @@ -359,13 +359,21 @@ gst_libcamera_stream_formats_to_caps(const StreamFormats &formats)
>  			GValue val = G_VALUE_INIT;
>  
>  			g_value_init(&val, GST_TYPE_INT_RANGE);
> -			gst_value_set_int_range_step(&val, range.min.width, range.max.width, range.hStep);
> -			gst_structure_set_value(s, "width", &val);
> -			gst_value_set_int_range_step(&val, range.min.height, range.max.height, range.vStep);
> -			gst_structure_set_value(s, "height", &val);
> +			if (range.min.width == range.max.width) {
> +				gst_structure_set(s, "width", G_TYPE_INT, range.min.width, nullptr);
> +			} else {
> +				gst_value_set_int_range_step(&val, range.min.width, range.max.width, range.hStep);
> +				gst_structure_set_value(s, "width", &val);
> +			}
> +			if (range.min.height == range.max.height) {
> +				gst_structure_set(s, "height", G_TYPE_INT, range.min.height, nullptr);
> +			} else {
> +				gst_value_set_int_range_step(&val, range.min.height, range.max.height, range.vStep);
> +				gst_structure_set_value(s, "height", &val);
> +			}
>  			g_value_unset(&val);
>  
> -			gst_caps_append_structure(caps, s);
> +			caps = gst_caps_merge_structure(caps, s);

You omitted to document this part of the change in the commit message, though
functionally it make sense. Perhaps something could be briefly added suggesting
that we now use gst_caps_merge_structure() to avoid appending structure that are
already expressed in the caps. That being said, the changes seems good to me.

Reviewed-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>


>  		}
>  	}
>