[libcamera-devel,v4,03/12] libcamera: controls: Construct from valid values

10222 diff mbox series

Add two constructors to the ControlInfo class that allows creating
a class instance from the list of the control valid values with
an optional default one.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 include/libcamera/controls.h |  5 ++++
 src/libcamera/controls.cpp   | 47 ++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

Hi Jacopo,

Thank you for the patch.

On Fri, Oct 23, 2020 at 07:11:07PM +0200, Jacopo Mondi wrote:
> Add two constructors to the ControlInfo class that allows creating
> a class instance from the list of the control valid values with
> an optional default one.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  include/libcamera/controls.h |  5 ++++
>  src/libcamera/controls.cpp   | 47 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 52 insertions(+)
> 
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 80944efc133a..0ae4fd002726 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -268,10 +268,14 @@ public:
>  	explicit ControlInfo(const ControlValue &min = 0,
>  			     const ControlValue &max = 0,
>  			     const ControlValue &def = 0);
> +	explicit ControlInfo(const ControlValue *values, unsigned int numValues,
> +			     const ControlValue &def);
> +	explicit ControlInfo(const ControlValue *values, unsigned int numValues);

We could use a single constructor, with def = {}. The implementation can
test def.isNone() to decide if the default should be taken from the
first value in the values, or from def.

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

You need to include <vector> at the top of the file for this
(compilation fails with clang-8 + libc++ for me otherwise).

>  
>  	std::string toString() const;
>  
> @@ -289,6 +293,7 @@ private:
>  	ControlValue min_;
>  	ControlValue max_;
>  	ControlValue def_;
> +	std::vector<ControlValue> values_;
>  };
>  
>  using ControlIdMap = std::unordered_map<unsigned int, const ControlId *>;
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index dca782667d88..2a2f04a598b6 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -491,6 +491,42 @@ ControlInfo::ControlInfo(const ControlValue &min,
>  {
>  }
>  
> +/**
> + * \brief Construct a ControlInfo from the list of values values and a default

s/values values/valid values/

> + * \param[in] values The control valid values
> + * \param[in] numValues The number or valid values

s/number of/number of/

> + * \param[in] def The control default value
> + *
> + * Construct a ControlInfo from a list of valid values. The ControlInfo
> + * minimum and maximum values are assigned to the first and last members of
> + * the values list respectively.
> + */
> +ControlInfo::ControlInfo(const ControlValue *values, unsigned int numValues,
> +			 const ControlValue &def)
> +	: def_(def)
> +{
> +	min_ = values[0];
> +	max_ = values[numValues - 1];
> +

Maybe

	values_.reserve(numValues);

here ?

> +	for (unsigned int i = 0; i < numValues; ++i)
> +		values_.push_back(values[i]);
> +}

I was puzzled by your earlier report regarding usage of Span here so I
had a look. Would the following make sense ?

diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
index 011c5867045b..3d7f4b0dc1a7 100644
--- a/include/libcamera/controls.h
+++ b/include/libcamera/controls.h
@@ -269,9 +269,8 @@ public:
 	explicit ControlInfo(const ControlValue &min = 0,
 			     const ControlValue &max = 0,
 			     const ControlValue &def = 0);
-	explicit ControlInfo(const ControlValue *values, unsigned int numValues,
-			     const ControlValue &def);
-	explicit ControlInfo(const ControlValue *values, unsigned int numValues);
+	explicit ControlInfo(Span<const ControlValue> values,
+			     const ControlValue &def = {});

 	const ControlValue &min() const { return min_; }
 	const ControlValue &max() const { return max_; }
diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
index 1d3827404966..78ccb90eae2d 100644
--- a/include/libcamera/ipa/raspberrypi.h
+++ b/include/libcamera/ipa/raspberrypi.h
@@ -50,13 +50,13 @@ static const ControlInfoMap Controls = {
 	{ &controls::AeEnable, ControlInfo(false, true) },
 	{ &controls::ExposureTime, ControlInfo(0, 999999) },
 	{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },
-	{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues.data(), controls::AeMeteringModeValues.size()) },
-	{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues.data(), controls::AeConstraintModeValues.size()) },
-	{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues.data(), controls::AeExposureModeValues.size()) },
+	{ &controls::AeMeteringMode, ControlInfo(Span<const ControlValue>(controls::AeMeteringModeValues)) },
+	{ &controls::AeConstraintMode, ControlInfo(Span<const ControlValue>(controls::AeConstraintModeValues)) },
+	{ &controls::AeExposureMode, ControlInfo(Span<const ControlValue>(controls::AeExposureModeValues)) },
 	{ &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },
 	{ &controls::AwbEnable, ControlInfo(false, true) },
 	{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
-	{ &controls::AwbMode, ControlInfo(controls::AwbModeValues.data(), controls::AwbModeValues.size()) },
+	{ &controls::AwbMode, ControlInfo(Span<const ControlValue>(controls::AwbModeValues)) },
 	{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },
 	{ &controls::Contrast, ControlInfo(0.0f, 32.0f) },
 	{ &controls::Saturation, ControlInfo(0.0f, 32.0f) },
diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
index 2a2f04a598b6..aad461121651 100644
--- a/src/libcamera/controls.cpp
+++ b/src/libcamera/controls.cpp
@@ -491,40 +491,26 @@ ControlInfo::ControlInfo(const ControlValue &min,
 {
 }

-/**
- * \brief Construct a ControlInfo from the list of values values and a default
- * \param[in] values The control valid values
- * \param[in] numValues The number or valid values
- * \param[in] def The control default value
- *
- * Construct a ControlInfo from a list of valid values. The ControlInfo
- * minimum and maximum values are assigned to the first and last members of
- * the values list respectively.
- */
-ControlInfo::ControlInfo(const ControlValue *values, unsigned int numValues,
-			 const ControlValue &def)
-	: def_(def)
-{
-	min_ = values[0];
-	max_ = values[numValues - 1];
-
-	for (unsigned int i = 0; i < numValues; ++i)
-		values_.push_back(values[i]);
-}
-
 /**
  * \brief Construct a ControlInfo from the list of valid values
  * \param[in] values The control valid values
- * \param[in] numValues The number or valid values
+ * \param[in] def The control default value
  *
  * Construct a ControlInfo from a list of valid values. The ControlInfo
- * minimum and maximum values are assigned to the first and last members of
- * the values list respectively. The ControlInfo default value is set to be
- * equal to the minimum one.
+ * minimum and maximum values are set to the first and last members of the
+ * values list respectively. The default value is set to \a def if provided, or
+ * to the minimum value otherwise.
  */
-ControlInfo::ControlInfo(const ControlValue *values, unsigned int numValues)
-	: ControlInfo(values, numValues, values[0])
+ControlInfo::ControlInfo(Span<const ControlValue> values,
+			 const ControlValue &def)
 {
+	min_ = values.front();
+	max_ = values.back();
+	def_ = !def.isNone() ? def : values.front();
+
+	values_.reserve(values.size());
+	for (const ControlValue &value : values)
+		values_.push_back(value);
 }

 /**

I like the ControlInfo::ControlInfo() constructor better, but I'm not
too fond of the explicit Span constructor in
include/libcamera/ipa/raspberrypi.h. Without that, relying on the Span
implicit constructor, the compiler complains:

In file included from ../../src/ipa/raspberrypi/raspberrypi.cpp:21:
../../include/libcamera/ipa/raspberrypi.h:53:31: error: call to constructor of 'libcamera::ControlInfo' is ambiguous
        { &controls::AeMeteringMode, ControlInfo{controls::AeMeteringModeValues} },
                                     ^          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../include/libcamera/controls.h:269:11: note: candidate constructor
        explicit ControlInfo(const ControlValue &min = 0,
                 ^
../../include/libcamera/controls.h:272:11: note: candidate constructor
        explicit ControlInfo(Span<const ControlValue> values,

(that's why clang, gcc is more cryptic)

Investigating a bit more, it can be fixed with

diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
index 011c5867045b..3b7f3347761e 100644
--- a/include/libcamera/controls.h
+++ b/include/libcamera/controls.h
@@ -97,6 +97,7 @@ public:

 #ifndef __DOXYGEN__
 	template<typename T, typename std::enable_if_t<!details::is_span<T>::value &&
+						       details::control_type<T>::value &&
 						       !std::is_same<std::string, std::remove_cv_t<T>>::value,
 						       std::nullptr_t> = nullptr>
 	ControlValue(const T &value)

I'll submit this change as a patch as I think it makes sense on its own,
do you think the above changes could be integrated in the next version
of this series ?

> +
> +/**
> + * \brief Construct a ControlInfo from the list of valid values
> + * \param[in] values The control valid values
> + * \param[in] numValues The number or valid values
> + *
> + * Construct a ControlInfo from a list of valid values. The ControlInfo
> + * minimum and maximum values are assigned to the first and last members of
> + * the values list respectively. The ControlInfo default value is set to be
> + * equal to the minimum one.
> + */
> +ControlInfo::ControlInfo(const ControlValue *values, unsigned int numValues)
> +	: ControlInfo(values, numValues, values[0])
> +{
> +}
> +
>  /**
>   * \fn ControlInfo::min()
>   * \brief Retrieve the minimum value of the control
> @@ -519,6 +555,17 @@ ControlInfo::ControlInfo(const ControlValue &min,
>   * \return A ControlValue with the default value for the control
>   */
>  
> +/**
> + * \fn ControlInfo::values()
> + * \brief Retrieve the list of valid values
> + *
> + * For controls that support a pre-defined number of values, the enumeration of
> + * those is reported through a vector of ControlValue instances accessible with
> + * this method.
> + *
> + * \return A vector of ControlValue representing the control valid values
> + */
> +
>  /**
>   * \brief Provide a string representation of the ControlInfo
>   */