[libcamera-devel] [PATCH v5 1/9] controls: Add boolean constructors for ControlInfo

Sun Jul 25 05:05:50 CEST 2021

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 };

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 };

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
is the increase in the number of constructors, which could create
ambiguous constructs due to the implicit constructors for ControlValue.

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);

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