[libcamera-devel] [PATCH v5 1/9] controls: Add boolean constructors for ControlInfo
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Aug 2 03:08:19 CEST 2021
Hi Paul,
On Thu, Jul 29, 2021 at 07:34:37PM +0900, paul.elder at ideasonboard.com wrote:
> On Wed, Jul 28, 2021 at 07:39:23PM +0300, Laurent Pinchart wrote:
> > 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" ?
>
> Oh huh, I see. The reason we're having this issue is because we want to
> be able to report that "we support AE" and "we don't support turning off
> AE" (or the reverse, "we don't support AE"). In that case maybe
> properties like "AeAvailable" and "AeTurnoffable" would serve the
> unchangable control?
>
> But then we have properties that just mirror controls... Is using
> controls not the best way to report capabilities? They have attached
> ControlInfos showing the valid values so I thought they were good.
This is an area where we depart from the Android camera HAL API. In
Android, meta-information about controls (does this make it camera
meta-meta-data ? :-)) is reported as static metadata, while in
libcamera, we report them through ControlInfo. I like ControlInfo as it
carries a clear association with controls, while in Android, one has to
know which static metadata corresponds to which control metadata (or
possibly dynamic metadata). Static information that are not related to
controls are reported in libcamera through properties, which as
equivalent to the Android static metadata. There's one shortcoming in
our approach though, we have no way to report information about metadata
(in the libcamera sense, as reported through requests, the equivalent of
the Android dynamic metadata). Maybe we would need Camera::metadata()
for that.
Anyway, at this point I don't think we should switch to creating more
properties to report information about controls.
> That's why imo I think that ControlInfo({ true, false }, true) is fine.
> Sure it's redundant information, but it's clear: "I want a boolean
> control that supports both true and false, and the default value is
> true". We have assertions to protect against dumb/malicious users that
> try weird stuff like ControlInfo({ true, true }, false). And then if you
> the user goes "I want a boolean control that supports only true", then
> the default value is implied, and ControlInfo(true) is sufficient.
At the risk of repeating myself, this bothers me for a few reasons:
- We have a similar issue with enumerated controls (not having a nice
syntax to create a ControlInfo with a set of enum values and a
default), and bool controls can be considered as a special case of an
enumeration with only two values, so it would be nice to have a single
API (or at least single syntax, even if the constructors end up being
different) to handle both. I'm not sure if this is possible.
- There's room for getting it wrong by passing ({ true, true }, false).
As you mentioned, we have asserts to catch this, but making errors
impossible in the first place would create a nicer API.
- ControlInfo(true) (or false) is really confusing to read, more, I
believe, than ControlInfo({ true, true }, true). Furthermore, the more
constructors we have, especially with common types, the more we'll
risk running into a situation where we won't be able to add a new
constructor due to ambiguous overload resolution. I think writing
ControlInfo({ true }, true) would be better than ControlInfo(true)
from that point of view.
None of those are showstoppers, but they make me think we can do better.
> Sorry it's just kinda rambling :p
That's what I've been doing as well so far, so no need to apologize :-)
> > 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
More information about the libcamera-devel
mailing list