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

[Kieran Bingham]

Quoting Nicolas Dufresne (2024-07-25 17:51:25)
> 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.

I can add that to the commit message then:

"To prevent appending structures that are already expressed with this
update, gst_caps_merge_structure() is used in place of
gst_caps_append_structure()."

if that's correct ?

--
Kieran


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