[RFC,v1] libcamera: controls: ControlInfo: Ensure types match
diff mbox series

Message ID 20250128121352.494582-1-pobrn@protonmail.com
State New
Headers show
Series
  • [RFC,v1] libcamera: controls: ControlInfo: Ensure types match
Related show

Commit Message

Barnabás Pőcze Jan. 28, 2025, 12:13 p.m. UTC
It is expected that the min/max/default/etc. values in a `ControlInfo`
have the same type. So add assertions to check that.

Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
---
 src/libcamera/controls.cpp | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

Comments

Kieran Bingham Feb. 13, 2025, 10:57 a.m. UTC | #1
Quoting Barnabás Pőcze (2025-01-28 12:13:55)
> It is expected that the min/max/default/etc. values in a `ControlInfo`
> have the same type. So add assertions to check that.
> 

This patch looks like a very good addition, which could have helped
catch the issue Naush faced yesterday
(https://patchwork.libcamera.org/patch/22778/)

Is there anyway we can make this a compile time assertion instead of a
runtime check?

--
Kieran


> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> ---
>  src/libcamera/controls.cpp | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index 70f6f6092..07f276b35 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -580,6 +580,19 @@ ControlId::ControlId(unsigned int id, const std::string &name,
>   * \brief The Control template type T
>   */
>  
> +namespace {
> +
> +bool sameShape(const ControlValue &a, const ControlValue &b)
> +{
> +       /**
> +        * \todo This is a best effort approach. Without the `ControlId`
> +        * there is no way to check if the sizes of fixed size arrays match.
> +        */
> +       return a.type() == b.type() && a.isArray() == b.isArray();
> +}
> +
> +}
> +
>  /**
>   * \class ControlInfo
>   * \brief Describe the limits of valid values for a Control
> @@ -601,6 +614,7 @@ ControlInfo::ControlInfo(const ControlValue &min,
>                          const ControlValue &def)
>         : min_(min), max_(max), def_(def)
>  {
> +       ASSERT(sameShape(min_, max_) && (def_.isNone() || sameShape(max_, def_)));
>  }
>  
>  /**
> @@ -616,13 +630,19 @@ ControlInfo::ControlInfo(const ControlValue &min,
>  ControlInfo::ControlInfo(Span<const ControlValue> values,
>                          const ControlValue &def)
>  {
> +       ASSERT(!values.empty());
> +
>         min_ = values.front();
>         max_ = values.back();
>         def_ = !def.isNone() ? def : values.front();
>  
> +       ASSERT(sameShape(min_, max_) && sameShape(max_, def_));
> +
>         values_.reserve(values.size());
> -       for (const ControlValue &value : values)
> +       for (const ControlValue &value : values) {
> +               ASSERT(sameShape(def_, value));
>                 values_.push_back(value);
> +       }
>  }
>  
>  /**
> -- 
> 2.48.1
> 
>
Barnabás Pőcze Feb. 13, 2025, 4:21 p.m. UTC | #2
Hi


2025. február 13., csütörtök 11:57 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta:

> Quoting Barnabás Pőcze (2025-01-28 12:13:55)
> > It is expected that the min/max/default/etc. values in a `ControlInfo`
> > have the same type. So add assertions to check that.
> >
> 
> This patch looks like a very good addition, which could have helped
> catch the issue Naush faced yesterday
> (https://patchwork.libcamera.org/patch/22778/)
> 
> Is there anyway we can make this a compile time assertion instead of a
> runtime check?

I think it is definitely possible, at least to some degree. But that would be an
appreciably more intrusive change.


Regards,
Barnabás Pőcze


> 
> --
> Kieran
> 
> 
> > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> > ---
> >  src/libcamera/controls.cpp | 22 +++++++++++++++++++++-
> >  1 file changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > index 70f6f6092..07f276b35 100644
> > --- a/src/libcamera/controls.cpp
> > +++ b/src/libcamera/controls.cpp
> > @@ -580,6 +580,19 @@ ControlId::ControlId(unsigned int id, const std::string &name,
> >   * \brief The Control template type T
> >   */
> >
> > +namespace {
> > +
> > +bool sameShape(const ControlValue &a, const ControlValue &b)
> > +{
> > +       /**
> > +        * \todo This is a best effort approach. Without the `ControlId`
> > +        * there is no way to check if the sizes of fixed size arrays match.
> > +        */
> > +       return a.type() == b.type() && a.isArray() == b.isArray();
> > +}
> > +
> > +}
> > +
> >  /**
> >   * \class ControlInfo
> >   * \brief Describe the limits of valid values for a Control
> > @@ -601,6 +614,7 @@ ControlInfo::ControlInfo(const ControlValue &min,
> >                          const ControlValue &def)
> >         : min_(min), max_(max), def_(def)
> >  {
> > +       ASSERT(sameShape(min_, max_) && (def_.isNone() || sameShape(max_, def_)));
> >  }
> >
> >  /**
> > @@ -616,13 +630,19 @@ ControlInfo::ControlInfo(const ControlValue &min,
> >  ControlInfo::ControlInfo(Span<const ControlValue> values,
> >                          const ControlValue &def)
> >  {
> > +       ASSERT(!values.empty());
> > +
> >         min_ = values.front();
> >         max_ = values.back();
> >         def_ = !def.isNone() ? def : values.front();
> >
> > +       ASSERT(sameShape(min_, max_) && sameShape(max_, def_));
> > +
> >         values_.reserve(values.size());
> > -       for (const ControlValue &value : values)
> > +       for (const ControlValue &value : values) {
> > +               ASSERT(sameShape(def_, value));
> >                 values_.push_back(value);
> > +       }
> >  }
> >
> >  /**
> > --
> > 2.48.1
> >
> >
>
Laurent Pinchart Feb. 13, 2025, 8:57 p.m. UTC | #3
On Thu, Feb 13, 2025 at 04:21:39PM +0000, Barnabás Pőcze wrote:
> Hi
> 
> 
> 2025. február 13., csütörtök 11:57 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta:
> 
> > Quoting Barnabás Pőcze (2025-01-28 12:13:55)
> > > It is expected that the min/max/default/etc. values in a `ControlInfo`
> > > have the same type. So add assertions to check that.
> > >
> > 
> > This patch looks like a very good addition, which could have helped
> > catch the issue Naush faced yesterday
> > (https://patchwork.libcamera.org/patch/22778/)
> > 
> > Is there anyway we can make this a compile time assertion instead of a
> > runtime check?
> 
> I think it is definitely possible, at least to some degree. But that would be an
> appreciably more intrusive change.

I wonder how instrusive it would be. We would need to pass a const
Control<T> reference to the ControlInfo constructor, and therefore make
the constructors inline (part of the constructors could be deferred to a
separate private init function, I'm thinking in particular about the
Span-based constructor). One possibly upside it that we may be able to
replace the ControlValue arguments with const T &, potentially
simplifying the callers. I'm sure I'm missing some of the impact this
would have.

> > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> > > ---
> > >  src/libcamera/controls.cpp | 22 +++++++++++++++++++++-
> > >  1 file changed, 21 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > > index 70f6f6092..07f276b35 100644
> > > --- a/src/libcamera/controls.cpp
> > > +++ b/src/libcamera/controls.cpp
> > > @@ -580,6 +580,19 @@ ControlId::ControlId(unsigned int id, const std::string &name,
> > >   * \brief The Control template type T
> > >   */
> > >
> > > +namespace {
> > > +
> > > +bool sameShape(const ControlValue &a, const ControlValue &b)
> > > +{
> > > +       /**
> > > +        * \todo This is a best effort approach. Without the `ControlId`
> > > +        * there is no way to check if the sizes of fixed size arrays match.
> > > +        */
> > > +       return a.type() == b.type() && a.isArray() == b.isArray();
> > > +}
> > > +
> > > +}
> > > +
> > >  /**
> > >   * \class ControlInfo
> > >   * \brief Describe the limits of valid values for a Control
> > > @@ -601,6 +614,7 @@ ControlInfo::ControlInfo(const ControlValue &min,
> > >                          const ControlValue &def)
> > >         : min_(min), max_(max), def_(def)
> > >  {
> > > +       ASSERT(sameShape(min_, max_) && (def_.isNone() || sameShape(max_, def_)));
> > >  }
> > >
> > >  /**
> > > @@ -616,13 +630,19 @@ ControlInfo::ControlInfo(const ControlValue &min,
> > >  ControlInfo::ControlInfo(Span<const ControlValue> values,
> > >                          const ControlValue &def)
> > >  {
> > > +       ASSERT(!values.empty());
> > > +
> > >         min_ = values.front();
> > >         max_ = values.back();
> > >         def_ = !def.isNone() ? def : values.front();
> > >
> > > +       ASSERT(sameShape(min_, max_) && sameShape(max_, def_));
> > > +
> > >         values_.reserve(values.size());
> > > -       for (const ControlValue &value : values)
> > > +       for (const ControlValue &value : values) {
> > > +               ASSERT(sameShape(def_, value));
> > >                 values_.push_back(value);
> > > +       }
> > >  }
> > >
> > >  /**
Barnabás Pőcze Feb. 13, 2025, 10:20 p.m. UTC | #4
Hi


2025. február 13., csütörtök 21:57 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta:

> On Thu, Feb 13, 2025 at 04:21:39PM +0000, Barnabás Pőcze wrote:
> > Hi
> >
> >
> > 2025. február 13., csütörtök 11:57 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta:
> >
> > > Quoting Barnabás Pőcze (2025-01-28 12:13:55)
> > > > It is expected that the min/max/default/etc. values in a `ControlInfo`
> > > > have the same type. So add assertions to check that.
> > > >
> > >
> > > This patch looks like a very good addition, which could have helped
> > > catch the issue Naush faced yesterday
> > > (https://patchwork.libcamera.org/patch/22778/)
> > >
> > > Is there anyway we can make this a compile time assertion instead of a
> > > runtime check?
> >
> > I think it is definitely possible, at least to some degree. But that would be an
> > appreciably more intrusive change.
> 
> I wonder how instrusive it would be. We would need to pass a const
> Control<T> reference to the ControlInfo constructor, and therefore make
> the constructors inline (part of the constructors could be deferred to a
> separate private init function, I'm thinking in particular about the
> Span-based constructor). One possibly upside it that we may be able to
> replace the ControlValue arguments with const T &, potentially
> simplifying the callers. I'm sure I'm missing some of the impact this
> would have.

Maybe something like this:

	template<typename T, typename... Args>
	ControlInfo(const Control<T>&, std::optional<typename Control<T>::type> def, const Args&... values)
	{
		static_assert(sizeof...(values) > 0);
		static_assert(((details::control_type<T>::value == details::control_type<Args>::value) && ...));
		static_assert(((details::control_type<T>::size == details::control_type<Args>::size) && ...));

		(values_.emplace_back(values), ...);

		min_ = values_.front();
		max_ = values_.back();
		def_ = def ? ControlValue(*def) : values_.front();
	}

But there are still some things:

 1) strings, i.e. handling `const char (&)[N]`, `const char *`, (`std::string_view`),
    and `std::string` (although there are no string controls at the moment);
 2) arrays, i.e. the handling of `vector<T>`, `T (&)[N]`, `span<T>`, etc;
 3) the type information about enums is lost, which is not ideal in my opinion.

I am wondering if there is anything preventing (3) from being addressed?


Regards,
Barnabás Pőcze


> 
> > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> > > > ---
> > > >  src/libcamera/controls.cpp | 22 +++++++++++++++++++++-
> > > >  1 file changed, 21 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > > > index 70f6f6092..07f276b35 100644
> > > > --- a/src/libcamera/controls.cpp
> > > > +++ b/src/libcamera/controls.cpp
> > > > @@ -580,6 +580,19 @@ ControlId::ControlId(unsigned int id, const std::string &name,
> > > >   * \brief The Control template type T
> > > >   */
> > > >
> > > > +namespace {
> > > > +
> > > > +bool sameShape(const ControlValue &a, const ControlValue &b)
> > > > +{
> > > > +       /**
> > > > +        * \todo This is a best effort approach. Without the `ControlId`
> > > > +        * there is no way to check if the sizes of fixed size arrays match.
> > > > +        */
> > > > +       return a.type() == b.type() && a.isArray() == b.isArray();
> > > > +}
> > > > +
> > > > +}
> > > > +
> > > >  /**
> > > >   * \class ControlInfo
> > > >   * \brief Describe the limits of valid values for a Control
> > > > @@ -601,6 +614,7 @@ ControlInfo::ControlInfo(const ControlValue &min,
> > > >                          const ControlValue &def)
> > > >         : min_(min), max_(max), def_(def)
> > > >  {
> > > > +       ASSERT(sameShape(min_, max_) && (def_.isNone() || sameShape(max_, def_)));
> > > >  }
> > > >
> > > >  /**
> > > > @@ -616,13 +630,19 @@ ControlInfo::ControlInfo(const ControlValue &min,
> > > >  ControlInfo::ControlInfo(Span<const ControlValue> values,
> > > >                          const ControlValue &def)
> > > >  {
> > > > +       ASSERT(!values.empty());
> > > > +
> > > >         min_ = values.front();
> > > >         max_ = values.back();
> > > >         def_ = !def.isNone() ? def : values.front();
> > > >
> > > > +       ASSERT(sameShape(min_, max_) && sameShape(max_, def_));
> > > > +
> > > >         values_.reserve(values.size());
> > > > -       for (const ControlValue &value : values)
> > > > +       for (const ControlValue &value : values) {
> > > > +               ASSERT(sameShape(def_, value));
> > > >                 values_.push_back(value);
> > > > +       }
> > > >  }
> > > >
> > > >  /**
> 
> --
> Regards,
> 
> Laurent Pinchart

Patch
diff mbox series

diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
index 70f6f6092..07f276b35 100644
--- a/src/libcamera/controls.cpp
+++ b/src/libcamera/controls.cpp
@@ -580,6 +580,19 @@  ControlId::ControlId(unsigned int id, const std::string &name,
  * \brief The Control template type T
  */
 
+namespace {
+
+bool sameShape(const ControlValue &a, const ControlValue &b)
+{
+	/**
+	 * \todo This is a best effort approach. Without the `ControlId`
+	 * there is no way to check if the sizes of fixed size arrays match.
+	 */
+	return a.type() == b.type() && a.isArray() == b.isArray();
+}
+
+}
+
 /**
  * \class ControlInfo
  * \brief Describe the limits of valid values for a Control
@@ -601,6 +614,7 @@  ControlInfo::ControlInfo(const ControlValue &min,
 			 const ControlValue &def)
 	: min_(min), max_(max), def_(def)
 {
+	ASSERT(sameShape(min_, max_) && (def_.isNone() || sameShape(max_, def_)));
 }
 
 /**
@@ -616,13 +630,19 @@  ControlInfo::ControlInfo(const ControlValue &min,
 ControlInfo::ControlInfo(Span<const ControlValue> values,
 			 const ControlValue &def)
 {
+	ASSERT(!values.empty());
+
 	min_ = values.front();
 	max_ = values.back();
 	def_ = !def.isNone() ? def : values.front();
 
+	ASSERT(sameShape(min_, max_) && sameShape(max_, def_));
+
 	values_.reserve(values.size());
-	for (const ControlValue &value : values)
+	for (const ControlValue &value : values) {
+		ASSERT(sameShape(def_, value));
 		values_.push_back(value);
+	}
 }
 
 /**