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

[Nicolas Dufresne]

Le jeudi 25 juillet 2024 à 21:13 +0100, Kieran Bingham a écrit :
> 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 ?

Ack.

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