[v1] gstreamer: Use `Control<>` objects when setting controls
diff mbox series

Message ID 20250328160736.94529-1-barnabas.pocze@ideasonboard.com
State Accepted
Headers show
Series
  • [v1] gstreamer: Use `Control<>` objects when setting controls
Related show

Commit Message

Barnabás Pőcze March 28, 2025, 4:07 p.m. UTC 
`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

Comments

Laurent Pinchart March 28, 2025, 5:33 p.m. UTC | #1 
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 %}
Kieran Bingham April 1, 2025, 4:57 p.m. UTC | #2 
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
Laurent Pinchart April 1, 2025, 6:25 p.m. UTC | #3 
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 %}

Patch
diff mbox series 

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 %}