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

[Qi Hou]

Done. Thanks

Regards,
Qi Hou

-----Original Message-----
From: Kieran Bingham <kieran.bingham at ideasonboard.com> 
Sent: 2024年7月25日 17:04
To: Olivier Crête <olivier.crete at collabora.com>; 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: RE: [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 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