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

[Kieran Bingham]

Hi Qi,

Quoting Qi Hou (2024-07-25 09:59:22)
> Hi @Olivier Crête,
> 
> Thank you very much for pointing out the corner case. I refined the patch based on your kind suggestion and it works.
> But what should I do next? To resend a review mail or continue using this mail?
> 

Please could you send a v2 patch with your prefered updates. When you
generate/save the patch, I think you can add '-v2' to git and it will
update [PATCH] to [PATCH v2] to signify this.

Thanks

Kieran


> 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);
>                 }
>         }
> 
> 
> Regards,
> Qi Hou
> 
> -----Original Message-----
> From: Olivier Crête <olivier.crete at collabora.com> 
> Sent: 2024年7月5日 0:04
> To: Qi Hou <qi.hou at nxp.com>; libcamera-devel at lists.libcamera.org
> Cc: Jared Hu <jared.hu at nxp.com>; Julien Vuillaumier <julien.vuillaumier at nxp.com>
> Subject: [EXT] Re: [PATCH] gstreamer: Fix critical warning "gst_value_set_int_range_step: assertion 'start < end' failed"
> 
> Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button
> 
> 
> Hi,
> 
> On Thu, 2024-06-27 at 10:22 +0900, Hou Qi wrote:
> > --- a/src/gstreamer/gstlibcamera-utils.cpp
> > +++ b/src/gstreamer/gstlibcamera-utils.cpp
> > @@ -354,7 +354,7 @@ gst_libcamera_stream_formats_to_caps(const StreamFormats &formats)
> >               }
> >
> >               const SizeRange &range = formats.range(pixelformat);
> > -             if (range.hStep && range.vStep) {
> > +             if (range.hStep && range.vStep && range.min != 
> > + range.max) {
> >                       GstStructure *s = gst_structure_copy(bare_s);
> >                       GValue val = G_VALUE_INIT;
> 
> What if range.min.width == range.max.widht, but range.min.height != range.max.height ?
> 
> I think a more foolproof implementation is:
> 
> g_value_init(&val, GST_TYPE_INT_RANGE);
> if (range.min.width == range.max.width) {
>   gst_structure_set_int (s, "width", G_TYPE_INT, range.min.width); } 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_int (s, "height", G_TYPE_INT, range.min.height); } else {
>   gst_value_set_int_range_step(&val, range.min.height, range.max.height, range.vStep);
>   gst_structure_set_value(s, "height", &val); }
> 
> gst_caps_merge_structure(caps, s); // Using merge to avoid duplicating existing structures from size)
> 
> 
> 
> --
> Olivier Crête
> olivier.crete at collabora.com
> Multimedia Lead