[RFC PATCH v1] libcamera: controls: ControlInfo: Ensure types match

[Laurent Pinchart]

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 at 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 at 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