| Message ID | 20240627012204.1949820-1-qi.hou@nxp.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Hou Qi, Thank you for the patch. On 27/06/24 6:52 am, Hou Qi wrote: > 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. Ah, seems legit. > Libcamera SizeRange instance may define a single size leading to s/Libcamera/libcamera s/may define/may return/ > identical min and max values. Add a test to avoid building GStreamer This can be rephrased better. Add a conditional check where the min and max of the range are distinct during iterating the supported sizes for each pixelformat. > range step from a single-size SizeRange to avoid such error. > > Signed-off-by: Hou Qi <qi.hou@nxp.com> I can take care of the commit message while applying the patch, Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/gstreamer/gstlibcamera-utils.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp > index ec4da435..b6672b9f 100644 > --- 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; >
Hi Umang Jain, Thanks for your review. Regards, Qi Hou -----Original Message----- From: Umang Jain <umang.jain@ideasonboard.com> Sent: 2024年6月29日 14:29 To: Qi Hou <qi.hou@nxp.com>; libcamera-devel@lists.libcamera.org Cc: Jared Hu <jared.hu@nxp.com>; Julien Vuillaumier <julien.vuillaumier@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 Hou Qi, Thank you for the patch. On 27/06/24 6:52 am, Hou Qi wrote: > 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. Ah, seems legit. > Libcamera SizeRange instance may define a single size leading to s/Libcamera/libcamera s/may define/may return/ > identical min and max values. Add a test to avoid building GStreamer This can be rephrased better. Add a conditional check where the min and max of the range are distinct during iterating the supported sizes for each pixelformat. > range step from a single-size SizeRange to avoid such error. > > Signed-off-by: Hou Qi <qi.hou@nxp.com> I can take care of the commit message while applying the patch, Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/gstreamer/gstlibcamera-utils.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp > b/src/gstreamer/gstlibcamera-utils.cpp > index ec4da435..b6672b9f 100644 > --- 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; >
On Sat, Jun 29, 2024 at 11:59:07AM +0530, Umang Jain wrote: > Hi Hou Qi, > > Thank you for the patch. > > On 27/06/24 6:52 am, Hou Qi wrote: > > 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. > > Ah, seems legit. > > Libcamera SizeRange instance may define a single size leading to > > s/Libcamera/libcamera > > s/may define/may return/ > > identical min and max values. Add a test to avoid building GStreamer > > This can be rephrased better. > > Add a conditional check where the min and max of the range are distinct > during iterating the supported sizes for each pixelformat. > > > range step from a single-size SizeRange to avoid such error. > > > > Signed-off-by: Hou Qi <qi.hou@nxp.com> > > I can take care of the commit message while applying the patch, > > > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > > src/gstreamer/gstlibcamera-utils.cpp | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp > > index ec4da435..b6672b9f 100644 > > --- 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; >
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)
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?
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@collabora.com>
Sent: 2024年7月5日 0:04
To: Qi Hou <qi.hou@nxp.com>; libcamera-devel@lists.libcamera.org
Cc: Jared Hu <jared.hu@nxp.com>; Julien Vuillaumier <julien.vuillaumier@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@collabora.com
Multimedia Lead
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@collabora.com> > Sent: 2024年7月5日 0:04 > To: Qi Hou <qi.hou@nxp.com>; libcamera-devel@lists.libcamera.org > Cc: Jared Hu <jared.hu@nxp.com>; Julien Vuillaumier <julien.vuillaumier@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@collabora.com > Multimedia Lead
Done. Thanks Regards, Qi Hou -----Original Message----- From: Kieran Bingham <kieran.bingham@ideasonboard.com> Sent: 2024年7月25日 17:04 To: Olivier Crête <olivier.crete@collabora.com>; Qi Hou <qi.hou@nxp.com>; libcamera-devel@lists.libcamera.org Cc: Jared Hu <jared.hu@nxp.com>; Julien Vuillaumier <julien.vuillaumier@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@collabora.com> > Sent: 2024年7月5日 0:04 > To: Qi Hou <qi.hou@nxp.com>; libcamera-devel@lists.libcamera.org > Cc: Jared Hu <jared.hu@nxp.com>; Julien Vuillaumier > <julien.vuillaumier@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@collabora.com > Multimedia Lead
diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp index ec4da435..b6672b9f 100644 --- 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;
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 define a single size leading to identical min and max values. Add a test to avoid building GStreamer range step from a single-size SizeRange to avoid such error. Signed-off-by: Hou Qi <qi.hou@nxp.com> --- src/gstreamer/gstlibcamera-utils.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)