[libcamera-devel] [PATCH v5 1/9] controls: Add boolean constructors for ControlInfo
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jul 28 18:39:23 CEST 2021
Hi Jacopo,
On Wed, Jul 28, 2021 at 06:31:59PM +0200, Jacopo Mondi wrote:
> On Mon, Jul 26, 2021 at 07:36:54PM +0300, Laurent Pinchart wrote:
> > Hi Jacopo,
>
> [snip]
>
> > > > The following would be one way to add a bool-specific constructor that
> > > > will have no risk of clashing with anything in the future:
> > > >
> > > > enum Mutable {
> > > > ReadOnly,
> > > > ReadWrite,
> > > > };
> > > >
> > > > ControlInfo(bool default, Mutable mutable);
> > > >
> > > > (the Mutable, ReadOnly and ReadWrite names can most probably be
> > > > improved). One would use this, mapping to the four cases above, as
> > > >
> > > > ControlInfo(false, ControlInfo::ReadWrite);
> > > > ControlInfo(true, ControlInfo::ReadWrite);
> > > > ControlInfo(false, ControlInfo::ReadOnly);
> > > > ControlInfo(true, ControlInfo::ReadOnly);
> > > >
> > > > Maybe writing it differently would be more explicit:
> > > >
> > > > ControlInfo(ControlInfo::MutableBool, false);
> > > > ControlInfo(ControlInfo::MutableBool, true);
> > > > ControlInfo(ControlInfo::ImmutableBool, false);
> > > > ControlInfo(ControlInfo::ImmutableBool, true);
> > >
> > > Ah, this could solve the concerns I expressed above.
> > >
> > > We have the general need to express when a Control is RO or RW, don't
> > > we ? We could add an rw_ flag to the ControlInfo class for that and
> > > derive a read-only variant maybe ?
> >
> > Thinking more about this, the read-only concept bothers me a bit. A
> > read-only control is essentially either a property or a metadata in the
> > general case (depending on whether the value is static or dynamic). What
> > we're dealing with here is a control that is meant to be writable, but
> > happens to only have a single supported value.
>
> This has bothered me all the time I tried to find a good term to
> identify those "booleans with a single value". I proposed 'identity'
> but that's not really up to the point. The fact I can't find a term
> for that makes me think I'm missing something and we should probably
> be doing something different. Hence the proposal to have a dedicated
> constructor, but yes, more constructors are ambiguos.
>
> >
> > Taking one step back, what are our use cases for boolean controls that
> > accept a single value ?
> >
>
> I think the original intent was to be able to express things like: the
> IPA only supports AE_ENABLE, so that you register a
>
> {controls::AeEnable, ControlInfo(true) }
>
> While if you can enable/disable it, and it's enabled by default:
>
> {controls::AeEnable, ControlInfo({true, false}, true) }
>
> I would now suggest to simply not register the control at all if it's
> not mutable, but I'm sure right now I'm missing why this was not
> possible in first place. Maybe it's just a matter of defining a policy
> like this one: if a control (not a property, nor a metadata) cannot be
> changed, just don't register it as part of the Camera::controls" ?
In that case applications wouldn't be able to differentiate a device
that doesn't support auto-exposure and a device that doesn't support
manual exposure. OK, not entirely true, applications could check for the
presence of a manual exposure time control, but in general, we could
have other boolean controls that would suffer from this issue.
> > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > > index 1bc958a43b22..5ef84a92f219 100644
> > > --- a/include/libcamera/controls.h
> > > +++ b/include/libcamera/controls.h
> > > @@ -272,11 +272,16 @@ public:
> > > const ControlValue &def = 0);
> > > explicit ControlInfo(Span<const ControlValue> values,
> > > const ControlValue &def = {});
> > > + explicit ControlInfo(bool value)
> > > + : min_(false), max_(true), def_(value)
> > > + {
> > > + }
> > >
> > > const ControlValue &min() const { return min_; }
> > > const ControlValue &max() const { return max_; }
> > > const ControlValue &def() const { return def_; }
> > > const std::vector<ControlValue> &values() const { return values_; }
> > > + bool rw() const { return rw_; }
> > >
> > > std::string toString() const;
> > >
> > > @@ -290,13 +295,41 @@ public:
> > > return !(*this == other);
> > > }
> > >
> > > -private:
> > > +protected:
> > > + bool rw_ : true;
> > > ControlValue min_;
> > > ControlValue max_;
> > > ControlValue def_;
> > > std::vector<ControlValue> values_;
> > > };
> > >
> > > +class ROControlInfo : public ControlInfo
> > > +{
> > > + LIBCAMERA_DISABLE_COPY_AND_MOVE(ROControlInfo);
> > > +
> > > + explicit ROControlInfo(const ControlValue &min = 0,
> > > + const ControlValue &max = 0,
> > > + const ControlValue &def = 0)
> > > + : ControlInfo(min, max, def)
> > > + {
> > > + rw_ = false;
> > > + }
> > > +
> > > + explicit ROControlInfo(Span<const ControlValue> values,
> > > + const ControlValue &def = {})
> > > + : ControlInfo(values, def)
> > > + {
> > > + rw_ = false;
> > > + }
> > > +
> > > + explicit ROControlInfo(bool value)
> > > + {
> > > + min_ = max_ = def_ = value;
> > > + rw_ = false;
> > > + }
> > > +};
> > > +
> > >
> > > I'm sure it could be done in a smarter way than this, I'm just wonder
> > > if it's worth it and IPA/ph should be in charge of deciding what type
> > > of ControlInfo to construct. For the generator, once we have a
> > > read_only flag, this should be trivial instead.
> > >
> > > > Now that I've started the brainstorming, I'll let you unleash your brain
> > > > power to shoot this down or improve it :-)
> > > >
> > > > > > mind, I guess we could now live with this ?
> > > > > >
> > > > > > > + *
> > > > > > > + * Construct a ControlInfo for a boolean control, where both true and false are
> > > > > > > + * valid values. \a values must be { false, true } (the order is irrelevant).
> > > > > > > + * The minimum value will always be false, and the maximum always true. The
> > > > > > > + * default value is \a def.
> > > > > > > + */
> > > > > > > +ControlInfo::ControlInfo(std::set<bool> values, bool def)
> > > > > > > + : min_(false), max_(true), def_(def), values_({ false, true })
> > > > > > > +{
> > > > > > > + assert(values.count(def) && values.size() == 2);
> > > > > >
> > > > > > Be aware this will accept ControlInfo({true, true}, false);
> > > > >
> > > > > It's a std::set<bool> so if you do { true, true } then values.size()
> > > > > should be 1.
> > > > >
> > > > > At least that was my reasoning. Is it wrong?
> > > > >
> > > > > > > +}
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * \brief Construct a boolean ControlInfo with only one valid value
> > > > > > > + * \param[in] value The control valid boolean value
> > > > > > > + *
> > > > > > > + * Construct a ControlInfo for a boolean control, where there is only valid
> > > > > > > + * value. The minimum, maximum, and default values will all be \a value.
> > > > > > > + */
> > > > > > > +ControlInfo::ControlInfo(bool value)
> > > > > > > + : min_(value), max_(value), def_(value)
> > > > > > > +{
> > > > > > > + values_ = { value };
> > > > > > > +}
> > > > > > > +
> > > > > > > /**
> > > > > > > * \fn ControlInfo::min()
> > > > > > > * \brief Retrieve the minimum value of the control
> > > > > > > diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp
> > > > > > > index 1e05e131..2827473b 100644
> > > > > > > --- a/test/controls/control_info.cpp
> > > > > > > +++ b/test/controls/control_info.cpp
> > > > > > > @@ -44,6 +44,39 @@ protected:
> > > > > > > return TestFail;
> > > > > > > }
> > > > > > >
> > > > > > > + /*
> > > > > > > + * Test information retrieval from a control with boolean
> > > > > > > + * values.
> > > > > > > + */
> > > > > > > + ControlInfo aeEnable({ false, true }, false);
> > > > > > > +
> > > > > > > + if (aeEnable.min().get<bool>() != false ||
> > > > > > > + aeEnable.def().get<bool>() != false ||
> > > > > > > + aeEnable.max().get<bool>() != true) {
> > > > > > > + cout << "Invalid control range for AeEnable" << endl;
> > > > > > > + return TestFail;
> > > > > > > + }
> > > > > > > +
> > > > > > > + if (aeEnable.values()[0].get<bool>() != false ||
> > > > > > > + aeEnable.values()[1].get<bool>() != true) {
> > > > > > > + cout << "Invalid control values for AeEnable" << endl;
> > > > > > > + return TestFail;
> > > > > > > + }
> > > > > > > +
> > > > > > > + ControlInfo awbEnable(true);
> > > > > > > +
> > > > > > > + if (awbEnable.min().get<bool>() != true ||
> > > > > > > + awbEnable.def().get<bool>() != true ||
> > > > > > > + awbEnable.max().get<bool>() != true) {
> > > > > > > + cout << "Invalid control range for AwbEnable" << endl;
> > > > > > > + return TestFail;
> > > > > > > + }
> > > > > > > +
> > > > > > > + if (awbEnable.values()[0].get<bool>() != true) {
> > > > > > > + cout << "Invalid control values for AwbEnable" << endl;
> > > > > > > + return TestFail;
> > > > > > > + }
> > > > > > > +
> > > > > >
> > > > > > As said, not in love with this, but I don't have anything better to
> > > > > > suggest :)
> > > > > >
> > > > > > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > > >
> > > > > > > return TestPass;
> > > > > > > }
> > > > > > > };
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list