[libcamera-devel] [RFC PATCH v3 01/16] controls: Add boolean constructor for ControlInfo

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Tue, Jul 06, 2021 at 07:22:55PM +0900, paul.elder at ideasonboard.com wrote:
> On Mon, Jul 05, 2021 at 03:39:00PM +0200, Jacopo Mondi wrote:
> > On Fri, Jul 02, 2021 at 07:37:45PM +0900, Paul Elder wrote:
> > > It would be convenient to be able to iterate over available boolean
> > > values, for example for controls that designate if some function can be
> > > enabled/disabled. The current min/max/def constructor is insufficient,
> > > as .values() is empty, so the values cannot be easily iterated over, and
> > > creating a Span of booleans does not work for the values constructor.
> > >
> > > Add a new constructor to ControlInfo that takes a Span of booleans (to
> > > allow specifiying one or two available values), and a default. The
> > > default value is not optional, as it doesn't make sense to have a silent
> > > default for boolean values.
> > 
> > So this is intended to be used as
> >         ControlInfo({true}, true);
> >         ControlInfo({false}, false);
> >         ControlInfo({true, true}, true);
> 
> ControlInfo({false, true}, true)
> 
> >         ControlInfo({true, true}, false);
> 
> ControlInfo({false, true}, false)
> > 
> > ?
> 
> Yeah, that was the idea. tbh I don't feel so well about it but I'm not
> sure how else to it it.
> 
> Although, since we have to declare a Span when we use it anyway, maybe
> we can just use the exsiting Span constructor. Just have to make sure
> false comes first, otherwise min_ = values.front() will choose the wrong
> value.

You could have a special case in the constructor when the type is bool,
and sort the values. I'm not sure if it's worth it though.

Now that I think about it, for enumerated types, would it make sense to
pass an std::set<ControlValue> to the ControlInfo constructor ? It would
require adding a comparison operator to the ControlValue class though.

> So yeah, I guess I'll just drop this patch.
> 
> > What is the purpose of having to specify a default if only one value
> > is available ? Does it even make sense to have a ControlInfo that only
> 
> If we only have ControlInfo({true}) with no second parameter, then it'll
> use the ControlInfo(Span<const ControlValue> values, const ControlValue
> &def = {}) constructor.
> 
> > represents true or false ?
> > 
> > Can you make sure
> >         ControlInfo({true}, false);
> > doesn't happen ?
> 
> atm, not really.
> 
> > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > >
> > > ---
> > > New in v3
> > > ---
> > >  include/libcamera/controls.h |  1 +
> > >  src/libcamera/controls.cpp   | 20 ++++++++++++++++++++
> > >  2 files changed, 21 insertions(+)
> > >
> > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > > index 1bc958a4..2dd147c8 100644
> > > --- a/include/libcamera/controls.h
> > > +++ b/include/libcamera/controls.h
> > > @@ -272,6 +272,7 @@ public:
> > >  			     const ControlValue &def = 0);
> > >  	explicit ControlInfo(Span<const ControlValue> values,
> > >  			     const ControlValue &def = {});
> > > +	explicit ControlInfo(Span<const bool> values, bool def);
> > 
> > The explicit keyword is used to disallow implicit conversion and copy
> > construction. It only applies to constructors with a single parameter
> > afaict, so it is not required here.
> > 
> > >
> > >  	const ControlValue &min() const { return min_; }
> > >  	const ControlValue &max() const { return max_; }
> > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > > index 34317fa0..e32e22e2 100644
> > > --- a/src/libcamera/controls.cpp
> > > +++ b/src/libcamera/controls.cpp
> > > @@ -514,6 +514,26 @@ ControlInfo::ControlInfo(Span<const ControlValue> values,
> > >  		values_.push_back(value);
> > >  }
> > >
> > > +/**
> > > + * \brief Construct a ControlInfo from a list of valid boolean values
> > > + * \param[in] values The control valid boolean vaalues
> > 
> > s/vaalues/values
> > 
> > The values can be just {true, false}, right ?
> > 
> > > + * \param[in] def The control default boolean value
> > > + *
> > > + * Construct a ControlInfo from a list of valid boolean values. The ControlInfo
> > > + * minimum and maximum values are set to the first and last members of the
> > > + * values list respectively. The default value is set to \a def.
> > 
> > So ControlInfo({true, false}) != ControlInfo({false, true}) ?
> > 
> > > + */
> > > +ControlInfo::ControlInfo(Span<const bool> values, bool def)
> > > +	: def_(def)
> > > +{
> > > +	min_ = values.front();
> > > +	max_ = values.back();
> > > +
> > > +	values_.reserve(2);
> > > +	for (const bool &value : values)
> > > +		values_.push_back(value);
> > > +}
> > > +
> > 
> > Could you point me to where this constructor is used, as it seems
> > suspicious to me :)
> 
> The pipeline handler uses this constructor to specify the valid values
> for boolean controls, like AeEnable, AeLock, and AwbLock in this series.
> 
> > >  /**
> > >   * \fn ControlInfo::min()
> > >   * \brief Retrieve the minimum value of the control

-- 
Regards,

Laurent Pinchart