[libcamera-devel] [PATCH v5 1/9] controls: Add boolean constructors for ControlInfo
Jacopo Mondi
jacopo at jmondi.org
Wed Jul 28 18:31:59 CEST 2021
Hi Laurent,
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" ?
Thanks
j
> > 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
More information about the libcamera-devel
mailing list