[libcamera-devel] [PATCH v5 1/9] controls: Add boolean constructors for ControlInfo
Jacopo Mondi
jacopo at jmondi.org
Mon Jul 26 16:08:07 CEST 2021
Hi Laurent,
On Sun, Jul 25, 2021 at 06:05:50AM +0300, Laurent Pinchart wrote:
> Hi Paul,
>
> Thank you for the patch.
>
> 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
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.
>
> 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 ?
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