| Message ID | 20240725092622.3835420-1-qi.hou@nxp.com |
|---|---|
| State | Accepted |
| Commit | c9152bad5ce905f5a31dbd05b40195f02c0cc2a9 |
| Headers | show |
| Series |
|
| Related | show |
Hi, I would probably suggest we shorten the $SUBJECT/$TITLE to: "gstreamer: Fix width and height range handling" which is a bit more succinct and describes what the patch does rather than the error message. Quoting Hou Qi (2024-07-25 10:26:22) > 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@nxp.com> Otherwise, Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.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); > } > } > > -- > 2.34.1 >
Hi Hou On Thu, Jul 25, 2024 at 06:26:22PM GMT, 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. > > 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@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); The patch seems logically correct, but I defer the call on the gstreamer api usage to people with more knwoledge in that area. With the commit message updated as Kieran's suggested Acked-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > } > } > > -- > 2.34.1 >
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@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. Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > } > } >
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@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@collabora.com> > > > > } > > } > > >
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@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@collabora.com> > > > > > > > } > > > } > > > > >
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); } }
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@nxp.com> --- src/gstreamer/gstlibcamera-utils.cpp | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)