[libcamera-devel] [PATCH v4 03/12] libcamera: controls: Construct from valid values
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Oct 23 23:00:25 CEST 2020
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 at 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
> */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list