[libcamera-devel] [PATCH v3 04/14] libcamera: controls: Construct from values list

[Laurent Pinchart]

Hi Jacopo,

On Fri, Oct 23, 2020 at 11:13:35AM +0200, Jacopo Mondi wrote:
> On Thu, Oct 22, 2020 at 05:12:14AM +0300, Laurent Pinchart wrote:
> > On Wed, Oct 21, 2020 at 04:36:25PM +0200, Jacopo Mondi wrote:
> > > Add two constructors to the ControlInfo class that allows creating
> >
> > s/allows/allow/
> >
> > > a class instance from the list of the control supported values with
> >
> > I'd talk about "valid values" instead of "supported values" here and in
> > several other places. An enumerated control will have all supported
> > values defined in the yaml file, and ControlInfo will list the valid
> > values for a particular pipeline handler (or maybe 'valid' and
> > 'supported' could be switched actually). The point is to explain that
> > there's a difference between what the API defines and what a particular
> > camera supports. This would be best explained, I think, when documenting
> > the concept of enumerated controls as mentioned in the review of 03/14.
> >
> > > an optional default value.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > >  include/libcamera/controls.h |  3 +++
> > >  src/libcamera/controls.cpp   | 31 +++++++++++++++++++++++++++++++
> > >  2 files changed, 34 insertions(+)
> > >
> > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > > index d1f6d4490c35..0099b6329026 100644
> > > --- a/include/libcamera/controls.h
> > > +++ b/include/libcamera/controls.h
> > > @@ -269,6 +269,9 @@ public:
> > >  			     const ControlValue &max = 0,
> > >  			     const ControlValue &def = 0,
> > >  			     const std::vector<ControlValue> &values = {});
> > > +	explicit ControlInfo(const std::vector<ControlValue> &values,
> > > +			     const ControlValue &def);
> > > +	explicit ControlInfo(const std::vector<ControlValue> &values);
> > >
> > >  	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 61feee37a1b8..389ecd5c519c 100644
> > > --- a/src/libcamera/controls.cpp
> > > +++ b/src/libcamera/controls.cpp
> > > @@ -493,6 +493,37 @@ ControlInfo::ControlInfo(const ControlValue &min,
> > >  {
> > >  }
> > >
> > > +/**
> > > + * \brief Construct a ControlInfo from the list of supported values and a default
> > > + * \param[in] values The control supported values
> > > + * \param[in] def The control default value
> > > + *
> > > + * Construct a ControlInfo from a list of supported values. The ControlInfo
> > > + * minimum and maximum values are assigned to the first and last members of
> >
> > s/assigned/set/ (otherwise it means the opposite, .front() = min)
> > s/members/entries/
> >
> > Same comments for the next function.
> >
> > I think you should mention that the values must be sorted in increasing
> > order. Is it worth adding an assertion if that's not the case ? You can
> > use std::is_sorted. This is a bit of a rabbit hole, as it requires
> > defining operator< for ControlValue, but we can restrict the
> > implementation for now to the int32 type (as that's what we use for
> > enumerated controls), so it may not be too complicated.
> 
> As said to Kieran, we're in control of the definition of the enumerate
> controls. Currently, I don't see a use case for out-of-order
> enumerated values, and I would rather std::sort them if we want to be
> extra paranoid, since it requires operator< anyway.o

When a pipeline handler or IPA passes the full list of values, as
generated by our scripts and stored in the <name>Values vector, there's
no issue. However, a pipeline handler or IPA can also restrict the
values they support by passing a manually-written list of values, in
which case errors could happen. That's what I'd like to catch with an
assertion error, to make sure we don't let this kind of bug creep in.

Please let me know if you would like me to help with the implementation
of operator<.

> > > + * the values list respectively.
> > > + */
> > > +ControlInfo::ControlInfo(const std::vector<ControlValue> &values,
> > > +			 const ControlValue &def)
> > > +	: def_(def), values_(values)
> > > +{
> > > +	min_ = values_.front();
> > > +	max_ = values_.back();
> > > +}
> > > +
> > > +/**
> > > + * \brief Construct a ControlInfo from the list of supported values
> > > + * \param[in] values The control supported values
> > > + *
> > > + * Construct a ControlInfo from a list of supported values. The ControlInfo
> > > + * minimum and maximum values are assigned to the first and last members of
> > > + * the values list respectively. The ControlInfo default value is set to be
> > > + * equal to the minimum one.
> > > + */
> > > +ControlInfo::ControlInfo(const std::vector<ControlValue> &values)
> > > +	: ControlInfo(values, values.front())
> > > +{
> > > +}
> > > +
> > >  /**
> > >   * \fn ControlInfo::min()
> > >   * \brief Retrieve the minimum value of the control

-- 
Regards,

Laurent Pinchart