Message ID | 20250328160736.94529-1-barnabas.pocze@ideasonboard.com |
---|---|
State | New |
Headers | show |
Series |
|
Related | show |
Hi Barnabás, Thank you for the patch. On Fri, Mar 28, 2025 at 05:07:36PM +0100, Barnabás Pőcze wrote: > `g_value_get_boolean()` returns `gboolean`, which is actually `int`. Thus > > // ControlValue x; > auto val = g_value_get_boolean(...); > x.set(val); > > will cause `ControlValue::set<int, ...>(const int&)` to be called, which > will save the value as `ControlTypeInteger32`, not `ControlTypeBoolean`. Oops... good catch ! > Then, if something tries to retrieve the boolean value, it will run into an > assertion failure: > > Assertion `type_ == details::control_type<std::remove_cv_t<T>>::value' failed. > > in `ControlValue::set()`. > > Fix this by using the appropriately typed `Control<>` object when setting > the value in the `ControlList` as this ensures that the value will be > converted to the actual type of the control. > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=261 > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > There is a simpler fix to this issue as well: > > diff --git a/src/gstreamer/gstlibcamera-controls.cpp.in b/src/gstreamer/gstlibcamera-controls.cpp.in > index d937b19e6..8378b5fd0 100644 > --- a/src/gstreamer/gstlibcamera-controls.cpp.in > +++ b/src/gstreamer/gstlibcamera-controls.cpp.in > @@ -271,7 +271,7 @@ bool GstCameraControls::setProperty(guint propId, const GValue *value, > } > Rectangle val = value_get_rectangle(value); > {%- else %} > - auto val = g_value_get_{{ ctrl.gtype }}(value); > + {{ ctrl.element_type }} val = g_value_get_{{ ctrl.gtype }}(value); I knew auto was bad :-D Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> You fixed the bug even before I had a chance to look at the bug report. Kudos ! > {%- endif %} > control.set(val); > {%- endif %} > > --- > src/gstreamer/gstlibcamera-controls.cpp.in | 13 ++++--------- > 1 file changed, 4 insertions(+), 9 deletions(-) > > diff --git a/src/gstreamer/gstlibcamera-controls.cpp.in b/src/gstreamer/gstlibcamera-controls.cpp.in > index d937b19e6..89c530da0 100644 > --- a/src/gstreamer/gstlibcamera-controls.cpp.in > +++ b/src/gstreamer/gstlibcamera-controls.cpp.in > @@ -223,7 +223,6 @@ bool GstCameraControls::setProperty(guint propId, const GValue *value, > {%- for ctrl in ctrls %} > > case controls::{{ ctrl.namespace }}{{ ctrl.name|snake_case|upper }}: { > - ControlValue control; > {%- if ctrl.is_array %} > size_t size = gst_value_array_get_size(value); > {%- if ctrl.size != 0 %} > @@ -254,12 +253,9 @@ bool GstCameraControls::setProperty(guint propId, const GValue *value, > } > > {%- if ctrl.size == 0 %} > - control.set(Span<const {{ ctrl.element_type }}>(values.data(), > - size)); > + Span<const {{ ctrl.element_type }}> val(values.data(), size); > {%- else %} > - control.set(Span<const {{ ctrl.element_type }}, > - {{ ctrl.size }}>(values.data(), > - {{ ctrl.size }})); > + Span<const {{ ctrl.element_type }}, {{ ctrl.size }}> val(values.data(), size); > {%- endif %} > {%- else %} > {%- if ctrl.is_rectangle %} > @@ -273,10 +269,9 @@ bool GstCameraControls::setProperty(guint propId, const GValue *value, > {%- else %} > auto val = g_value_get_{{ ctrl.gtype }}(value); > {%- endif %} > - control.set(val); > {%- endif %} > - controls_.set(propId, control); > - controls_acc_.set(propId, control); > + controls_.set(controls::{{ ctrl.namespace }}{{ ctrl.name }}, val); > + controls_acc_.set(controls::{{ ctrl.namespace }}{{ ctrl.name }}, val); > return true; > } > {%- endfor %}
Quoting Laurent Pinchart (2025-03-28 17:33:49) > Hi Barnabás, > > Thank you for the patch. > > On Fri, Mar 28, 2025 at 05:07:36PM +0100, Barnabás Pőcze wrote: > > `g_value_get_boolean()` returns `gboolean`, which is actually `int`. Thus > > > > // ControlValue x; > > auto val = g_value_get_boolean(...); > > x.set(val); > > > > will cause `ControlValue::set<int, ...>(const int&)` to be called, which > > will save the value as `ControlTypeInteger32`, not `ControlTypeBoolean`. > > Oops... good catch ! > > > Then, if something tries to retrieve the boolean value, it will run into an > > assertion failure: > > > > Assertion `type_ == details::control_type<std::remove_cv_t<T>>::value' failed. > > > > in `ControlValue::set()`. > > > > Fix this by using the appropriately typed `Control<>` object when setting > > the value in the `ControlList` as this ensures that the value will be > > converted to the actual type of the control. > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=261 > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > --- > > There is a simpler fix to this issue as well: > > > > diff --git a/src/gstreamer/gstlibcamera-controls.cpp.in b/src/gstreamer/gstlibcamera-controls.cpp.in > > index d937b19e6..8378b5fd0 100644 > > --- a/src/gstreamer/gstlibcamera-controls.cpp.in > > +++ b/src/gstreamer/gstlibcamera-controls.cpp.in > > @@ -271,7 +271,7 @@ bool GstCameraControls::setProperty(guint propId, const GValue *value, > > } > > Rectangle val = value_get_rectangle(value); > > {%- else %} > > - auto val = g_value_get_{{ ctrl.gtype }}(value); > > + {{ ctrl.element_type }} val = g_value_get_{{ ctrl.gtype }}(value); > > I knew auto was bad :-D > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Is this reviewing the simple fix, or the fix below? If there's a simpler fix - what's the rationale for the longer fix below? I don't mind which one is merged, they both have lots of templating magic which makes this hard to review, but fixing the bug is nice ;-) For which ever of these approaches is desired/appropriate: Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com> -- Kieran > > You fixed the bug even before I had a chance to look at the bug report. > Kudos ! > > > {%- endif %} > > control.set(val); > > {%- endif %} > > > > --- > > src/gstreamer/gstlibcamera-controls.cpp.in | 13 ++++--------- > > 1 file changed, 4 insertions(+), 9 deletions(-) > > > > diff --git a/src/gstreamer/gstlibcamera-controls.cpp.in b/src/gstreamer/gstlibcamera-controls.cpp.in > > index d937b19e6..89c530da0 100644 > > --- a/src/gstreamer/gstlibcamera-controls.cpp.in > > +++ b/src/gstreamer/gstlibcamera-controls.cpp.in > > @@ -223,7 +223,6 @@ bool GstCameraControls::setProperty(guint propId, const GValue *value, > > {%- for ctrl in ctrls %} > > > > case controls::{{ ctrl.namespace }}{{ ctrl.name|snake_case|upper }}: { > > - ControlValue control; > > {%- if ctrl.is_array %} > > size_t size = gst_value_array_get_size(value); > > {%- if ctrl.size != 0 %} > > @@ -254,12 +253,9 @@ bool GstCameraControls::setProperty(guint propId, const GValue *value, > > } > > > > {%- if ctrl.size == 0 %} > > - control.set(Span<const {{ ctrl.element_type }}>(values.data(), > > - size)); > > + Span<const {{ ctrl.element_type }}> val(values.data(), size); > > {%- else %} > > - control.set(Span<const {{ ctrl.element_type }}, > > - {{ ctrl.size }}>(values.data(), > > - {{ ctrl.size }})); > > + Span<const {{ ctrl.element_type }}, {{ ctrl.size }}> val(values.data(), size); > > {%- endif %} > > {%- else %} > > {%- if ctrl.is_rectangle %} > > @@ -273,10 +269,9 @@ bool GstCameraControls::setProperty(guint propId, const GValue *value, > > {%- else %} > > auto val = g_value_get_{{ ctrl.gtype }}(value); > > {%- endif %} > > - control.set(val); > > {%- endif %} > > - controls_.set(propId, control); > > - controls_acc_.set(propId, control); > > + controls_.set(controls::{{ ctrl.namespace }}{{ ctrl.name }}, val); > > + controls_acc_.set(controls::{{ ctrl.namespace }}{{ ctrl.name }}, val); > > return true; > > } > > {%- endfor %} > > -- > Regards, > > Laurent Pinchart
On Tue, Apr 01, 2025 at 05:57:24PM +0100, Kieran Bingham wrote: > Quoting Laurent Pinchart (2025-03-28 17:33:49) > > Hi Barnabás, > > > > Thank you for the patch. > > > > On Fri, Mar 28, 2025 at 05:07:36PM +0100, Barnabás Pőcze wrote: > > > `g_value_get_boolean()` returns `gboolean`, which is actually `int`. Thus > > > > > > // ControlValue x; > > > auto val = g_value_get_boolean(...); > > > x.set(val); > > > > > > will cause `ControlValue::set<int, ...>(const int&)` to be called, which > > > will save the value as `ControlTypeInteger32`, not `ControlTypeBoolean`. > > > > Oops... good catch ! > > > > > Then, if something tries to retrieve the boolean value, it will run into an > > > assertion failure: > > > > > > Assertion `type_ == details::control_type<std::remove_cv_t<T>>::value' failed. > > > > > > in `ControlValue::set()`. > > > > > > Fix this by using the appropriately typed `Control<>` object when setting > > > the value in the `ControlList` as this ensures that the value will be > > > converted to the actual type of the control. > > > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=261 > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > --- > > > There is a simpler fix to this issue as well: > > > > > > diff --git a/src/gstreamer/gstlibcamera-controls.cpp.in b/src/gstreamer/gstlibcamera-controls.cpp.in > > > index d937b19e6..8378b5fd0 100644 > > > --- a/src/gstreamer/gstlibcamera-controls.cpp.in > > > +++ b/src/gstreamer/gstlibcamera-controls.cpp.in > > > @@ -271,7 +271,7 @@ bool GstCameraControls::setProperty(guint propId, const GValue *value, > > > } > > > Rectangle val = value_get_rectangle(value); > > > {%- else %} > > > - auto val = g_value_get_{{ ctrl.gtype }}(value); > > > + {{ ctrl.element_type }} val = g_value_get_{{ ctrl.gtype }}(value); > > > > I knew auto was bad :-D > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Is this reviewing the simple fix, or the fix below? > > If there's a simpler fix - what's the rationale for the longer fix > below? The fix below, I should have been more explicit. > I don't mind which one is merged, they both have lots of templating > magic which makes this hard to review, but fixing the bug is nice ;-) > > For which ever of these approaches is desired/appropriate: > > Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > You fixed the bug even before I had a chance to look at the bug report. > > Kudos ! > > > > > {%- endif %} > > > control.set(val); > > > {%- endif %} > > > > > > --- > > > src/gstreamer/gstlibcamera-controls.cpp.in | 13 ++++--------- > > > 1 file changed, 4 insertions(+), 9 deletions(-) > > > > > > diff --git a/src/gstreamer/gstlibcamera-controls.cpp.in b/src/gstreamer/gstlibcamera-controls.cpp.in > > > index d937b19e6..89c530da0 100644 > > > --- a/src/gstreamer/gstlibcamera-controls.cpp.in > > > +++ b/src/gstreamer/gstlibcamera-controls.cpp.in > > > @@ -223,7 +223,6 @@ bool GstCameraControls::setProperty(guint propId, const GValue *value, > > > {%- for ctrl in ctrls %} > > > > > > case controls::{{ ctrl.namespace }}{{ ctrl.name|snake_case|upper }}: { > > > - ControlValue control; > > > {%- if ctrl.is_array %} > > > size_t size = gst_value_array_get_size(value); > > > {%- if ctrl.size != 0 %} > > > @@ -254,12 +253,9 @@ bool GstCameraControls::setProperty(guint propId, const GValue *value, > > > } > > > > > > {%- if ctrl.size == 0 %} > > > - control.set(Span<const {{ ctrl.element_type }}>(values.data(), > > > - size)); > > > + Span<const {{ ctrl.element_type }}> val(values.data(), size); > > > {%- else %} > > > - control.set(Span<const {{ ctrl.element_type }}, > > > - {{ ctrl.size }}>(values.data(), > > > - {{ ctrl.size }})); > > > + Span<const {{ ctrl.element_type }}, {{ ctrl.size }}> val(values.data(), size); > > > {%- endif %} > > > {%- else %} > > > {%- if ctrl.is_rectangle %} > > > @@ -273,10 +269,9 @@ bool GstCameraControls::setProperty(guint propId, const GValue *value, > > > {%- else %} > > > auto val = g_value_get_{{ ctrl.gtype }}(value); > > > {%- endif %} > > > - control.set(val); > > > {%- endif %} > > > - controls_.set(propId, control); > > > - controls_acc_.set(propId, control); > > > + controls_.set(controls::{{ ctrl.namespace }}{{ ctrl.name }}, val); > > > + controls_acc_.set(controls::{{ ctrl.namespace }}{{ ctrl.name }}, val); > > > return true; > > > } > > > {%- endfor %}
diff --git a/src/gstreamer/gstlibcamera-controls.cpp.in b/src/gstreamer/gstlibcamera-controls.cpp.in index d937b19e6..89c530da0 100644 --- a/src/gstreamer/gstlibcamera-controls.cpp.in +++ b/src/gstreamer/gstlibcamera-controls.cpp.in @@ -223,7 +223,6 @@ bool GstCameraControls::setProperty(guint propId, const GValue *value, {%- for ctrl in ctrls %} case controls::{{ ctrl.namespace }}{{ ctrl.name|snake_case|upper }}: { - ControlValue control; {%- if ctrl.is_array %} size_t size = gst_value_array_get_size(value); {%- if ctrl.size != 0 %} @@ -254,12 +253,9 @@ bool GstCameraControls::setProperty(guint propId, const GValue *value, } {%- if ctrl.size == 0 %} - control.set(Span<const {{ ctrl.element_type }}>(values.data(), - size)); + Span<const {{ ctrl.element_type }}> val(values.data(), size); {%- else %} - control.set(Span<const {{ ctrl.element_type }}, - {{ ctrl.size }}>(values.data(), - {{ ctrl.size }})); + Span<const {{ ctrl.element_type }}, {{ ctrl.size }}> val(values.data(), size); {%- endif %} {%- else %} {%- if ctrl.is_rectangle %} @@ -273,10 +269,9 @@ bool GstCameraControls::setProperty(guint propId, const GValue *value, {%- else %} auto val = g_value_get_{{ ctrl.gtype }}(value); {%- endif %} - control.set(val); {%- endif %} - controls_.set(propId, control); - controls_acc_.set(propId, control); + controls_.set(controls::{{ ctrl.namespace }}{{ ctrl.name }}, val); + controls_acc_.set(controls::{{ ctrl.namespace }}{{ ctrl.name }}, val); return true; } {%- endfor %}
`g_value_get_boolean()` returns `gboolean`, which is actually `int`. Thus // ControlValue x; auto val = g_value_get_boolean(...); x.set(val); will cause `ControlValue::set<int, ...>(const int&)` to be called, which will save the value as `ControlTypeInteger32`, not `ControlTypeBoolean`. Then, if something tries to retrieve the boolean value, it will run into an assertion failure: Assertion `type_ == details::control_type<std::remove_cv_t<T>>::value' failed. in `ControlValue::set()`. Fix this by using the appropriately typed `Control<>` object when setting the value in the `ControlList` as this ensures that the value will be converted to the actual type of the control. Bug: https://bugs.libcamera.org/show_bug.cgi?id=261 Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- There is a simpler fix to this issue as well: diff --git a/src/gstreamer/gstlibcamera-controls.cpp.in b/src/gstreamer/gstlibcamera-controls.cpp.in index d937b19e6..8378b5fd0 100644 --- a/src/gstreamer/gstlibcamera-controls.cpp.in +++ b/src/gstreamer/gstlibcamera-controls.cpp.in @@ -271,7 +271,7 @@ bool GstCameraControls::setProperty(guint propId, const GValue *value, } Rectangle val = value_get_rectangle(value); {%- else %} - auto val = g_value_get_{{ ctrl.gtype }}(value); + {{ ctrl.element_type }} val = g_value_get_{{ ctrl.gtype }}(value); {%- endif %} control.set(val); {%- endif %} --- src/gstreamer/gstlibcamera-controls.cpp.in | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) -- 2.49.0