[libcamera-devel] [PATCH v5 1/9] controls: Add boolean constructors for ControlInfo
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Aug 12 22:42:56 CEST 2021
Hi Paul,
On Mon, Aug 02, 2021 at 04:08:22AM +0300, Laurent Pinchart wrote:
> 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.
I notice this got merged, even though the discussion hasn't come to a
conclusion. Not really an issue as such, but I'd like to revive this
mail thread. Any opinion on the three points above ?
> > 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