[libcamera-devel] [PATCH v5 1/9] controls: Add boolean constructors for ControlInfo
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jul 26 18:36:54 CEST 2021
Hi Jacopo,
On Mon, Jul 26, 2021 at 04:08:07PM +0200, Jacopo Mondi wrote:
> On Sun, Jul 25, 2021 at 06:05:50AM +0300, Laurent Pinchart wrote:
> > On Fri, Jul 23, 2021 at 02:45:52PM +0900, paul.elder at ideasonboard.com wrote:
> > > On Thu, Jul 22, 2021 at 03:06:09PM +0200, Jacopo Mondi wrote:
> > > > On Tue, Jul 20, 2021 at 07:12:59PM +0900, Paul Elder wrote:
> > > > > It would be convenient to be able to iterate over available boolean
> > > > > values, for example for controls that designate if some function can be
> > > > > enabled/disabled. The current min/max/def constructor is insufficient,
> > > > > as .values() is empty, so the values cannot be easily iterated over, and
> > > > > creating a Span of booleans does not work for the values constructor.
> > > > >
> > > > > Add new constructors to ControlInfo that takes a set of booleans (if
> > > > > both booleans are valid values) plus a default, and another that takes
> > > > > only one boolean (if only one boolean is a valid value).
> > > > >
> > > > > Update the ControlInfo test accordingly.
> > > >
> > > > Thank you for considering my suggestion!
> > > >
> > > > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > > > >
> > > > > ---
> > > > > Changes in v5:
> > > > > - break away the single-value one to a different constructor
> > > > >
> > > > > Changes in v2:
> > > > > - use set instead of span of bools
> > > > > - add assertion to make sure that the default is a valid value
> > > > > - update the test
> > > > > ---
> > > > > include/libcamera/controls.h | 3 +++
> > > > > src/libcamera/controls.cpp | 29 +++++++++++++++++++++++++++++
> > > > > test/controls/control_info.cpp | 33 +++++++++++++++++++++++++++++++++
> > > > > 3 files changed, 65 insertions(+)
> > > > >
> > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > > > > index 1bc958a4..de733bd8 100644
> > > > > --- a/include/libcamera/controls.h
> > > > > +++ b/include/libcamera/controls.h
> > > > > @@ -9,6 +9,7 @@
> > > > > #define __LIBCAMERA_CONTROLS_H__
> > > > >
> > > > > #include <assert.h>
> > > > > +#include <set>
> > > > > #include <stdint.h>
> > > > > #include <string>
> > > > > #include <unordered_map>
> > > > > @@ -272,6 +273,8 @@ public:
> > > > > const ControlValue &def = 0);
> > > > > explicit ControlInfo(Span<const ControlValue> values,
> > > > > const ControlValue &def = {});
> > > > > + explicit ControlInfo(std::set<bool> values, bool def);
> > > >
> > > > No need for explicit if the constructor accepts two arguments
> > > >
> > > > > + explicit ControlInfo(bool value);
> > > > >
> > > > > 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 78109f41..283472c5 100644
> > > > > --- a/src/libcamera/controls.cpp
> > > > > +++ b/src/libcamera/controls.cpp
> > > > > @@ -514,6 +514,35 @@ ControlInfo::ControlInfo(Span<const ControlValue> values,
> > > > > values_.push_back(value);
> > > > > }
> > > > >
> > > > > +/**
> > > > > + * \brief Construct a boolean ControlInfo with both boolean values
> > > > > + * \param[in] values The control valid boolean values (both true and false)
> > > > > + * \param[in] def The control default boolean value
> > > >
> > > > I'm still not super happy with the outcome, as the 'valid values' can
> > > > be true/false and nothing else, so passing them in makes not much
> > > > sense. But as said in the previous review that's how we could
> > > > disambiguate between a ControlInfo that supports both values and one
> > > > that only supports true or false. Unless something smarter comes to
> > >
> > > Yeah that was the idea. It's unnecessary information but it makes the
> > > distinction clear :/
> >
> > So we all agree it's not great. The question is how it could be better
> > :-)
> >
> > Turning brainstorming mode on.
> >
> > We have two types of integer-based controls, numerical and enumerated.
> > The numerical controls have a minimum, maximum and default, while the
> > enumerated controls have a list of possible values and a default.
> > Constructing a ControlInfo for enumerated controls requires providing a
> > span that contains all the possible values.
> >
> > When generating control_ids.h from control_ids.yaml, the python script
> > creates, for each enumerated control, an array containing all possible
> > values. This can be used as an argument to the ControlInfo constructor,
> > as a span can be implicitly constructed from an std::array:
> >
> > ControlInfo info{ controls::AeExposureModeValues };
> >
> > If we were to create a ControlInfo that only supports a subset of those
> > modes, however, the following syntax, would result in a compilation
> > error:
> >
> > ControlInfo info{ {
> > controls::ExposureNormal,
> > controls::ExposureShort,
> > controls::ExposureLong,
> > } };
> >
> > To make it compile, we would need to write
> >
> > ControlInfo info{ std::array<ControlValue, 4>{
> > static_cast<int32_t>(controls::ExposureNormal),
> > static_cast<int32_t>(controls::ExposureShort),
> > static_cast<int32_t>(controls::ExposureLong),
> > } };
> >
> > which isn't exactly great.
> >
> > The bools fall in the same category,
> >
> > ControlInfo info{ { false, true }, true };
>
> I appreciate the thoughtful analysis of the issue, but my concern was
> different and could be expressed as:
> - For a boolean control info, does it make sense to pass in
> {true, false} as possible values as
> - The order is not relevant
> - The only values a boolean control info can support are, by
> definition, true or false
Agreed, hence the alternative, bool-only proposal below :-) The downside
is that it won't help us with enum-based controls, but maybe that can be
addressed separately.
> All of this assuming a boolean control info that only support true OR
> false is constructed by passing in the single supported value.
>
> Even if we make the constructors nicer, my issue was conceptually on
> the requirement to pass {false, true} in, even if that's not required
> by definition
>
> > won't compile, while
> >
> > ControlInfo info{ std::array<ControlValue, 2>{ false, true }, true };
> >
> > would. Arguably not the greatest either.
> >
> > If we want to improve this, the first question we should ask ourselves
> > is if a nicer syntax common to all enumerated controls is achievable.
> > For instance, we could perhaps define the following constructor:
> >
> > template<typename T>
> > ControlInfo(std::initializer_list<T> values, T def);
> >
> > With appropriate implementations for the types we want to support. This
> > should work for bools, allowing us to write
> >
> > ControlInfo info{ { false, true }, true };
>
> This is possible today with the simple constructor added by Paul, but
> does not solve the issue with the two values being passed in
>
> > but won't work nicely for AeExposureMode. Due to the values being
> > enumerated, the compiler will want and implementation for
> >
> > ControlInfo::ControlInfo<controls::AeExposureModeEnum>(std::initializer_list<controls::AeExposureModeEnum>,
> > controls::AeExposureModeEnum)
> >
> > and not
> >
> > ControlInfo::ControlInfo<int32_t(std::initializer_list<int32_t>, int32_t)
> >
> > Maybe we could work around that by creating an inline constructor that
> > will only match enumerated types (using std::enable_if) and casting to
> > int32_t. I'm not sure if
> >
> > static_cast<std::initializer_list<int32_t>>(values)
> >
> > would work though, I have a feeling it won't be that easy.
> >
> > I'm sure other options may be possible too.
If someone has a great idea that would address both the boolean and
enumerated controls, I'm still all ears :-)
> > Maybe a nice solution for this doesn't exist, in which case we could
> > focus on bools only. In that case, I see four different cases for a
> > ControlInfo related to bools:
> >
> > - values = { false, true }, default = false
> > - values = { false, true }, default = true
> > - values = { false }, default = false
> > - values = { true }, default = true
> >
> > Anything else is invalid. Could we improve this patch by catching more
> > invalid combinations are compile time ? One of the things that bother me
>
> I think this version only accepts the 4 of them, according to Paul's
> reply to my comment that ({false, false}, true) was possible.
>
> It currently goes through two constructors, one for 'mutable' booleans
> control info:
> ControlInfo::ControlInfo(std::set<bool> values, bool def)
>
> As this is an std::set, {false, false} or {true, true} will be refused
> by the assertion
> assert(values.count(def) && values.size() == 2);
>
> (note: asserting for values.size() == 2 is probably enough)
>
> And one for 'non mutable' ones
> ControlInfo::ControlInfo(bool value)
>
> Which was suggested as it reduces the possibility of mis-use as
> ControlInfo( {false}, true)
> is not possible with this constructor
>
> All in all I think this version is safe, I'm just bothered by the fact
> {true, false} have to passed in when those are implicitly the only
> values supported by a boolean control info
>
> > is the increase in the number of constructors, which could create
> > ambiguous constructs due to the implicit constructors for ControlValue.
>
> That's a reasonable concern
>
> > 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.
Taking one step back, what are our use cases for boolean controls that
accept a single value ?
> 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