[libcamera-devel] [PATCH v1.2 16/31] libcamera: controls: Support array controls in ControlValue
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Mar 3 01:23:10 CET 2020
On 01/03/2020 17:54, Laurent Pinchart wrote:
> From: Jacopo Mondi <jacopo at jmondi.org>
>
> Add array controls support to the ControlValue class. The polymorphic
> class can now store more than a single element and supports access and
> creation through the use of Span<>.
Oh, but if the creation is through a span, what stores the data in the
ControlValue ... I guess I'll see below... <aha we create a storage>
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
Some comments, but nothing you can't handle...
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
> Changes since v1:
>
> - Use T::value_type instead of T::element_type to benefit from
> std::remove_cv
> - Fix ControlTypeNone test in ControlValue::toString()
> - Separate array elements with ", " in ControlValue::toString()
> ---
> include/libcamera/controls.h | 81 ++++++++++++---
> src/libcamera/controls.cpp | 185 +++++++++++++++++++++++++++++------
> 2 files changed, 225 insertions(+), 41 deletions(-)
>
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 4538be06af93..1e24ae30ab36 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -9,6 +9,7 @@
> #define __LIBCAMERA_CONTROLS_H__
>
> #include <assert.h>
> +#include <stdint.h>
> #include <string>
> #include <unordered_map>
>
> @@ -51,6 +52,10 @@ struct control_type<int64_t> {
> static constexpr ControlType value = ControlTypeInteger64;
> };
>
> +template<typename T, std::size_t N>
> +struct control_type<Span<T, N>> : public control_type<std::remove_cv_t<T>> {
> +};
> +
> } /* namespace details */
>
> class ControlValue
> @@ -58,15 +63,35 @@ class ControlValue
> public:
> ControlValue();
>
> +#ifndef __DOXYGEN__
> + template<typename T, typename std::enable_if_t<!details::is_span<T>::value, std::nullptr_t> = nullptr>
> + ControlValue(const T &value)
> + : type_(ControlTypeNone), numElements_(0)
> + {
> + set(details::control_type<std::remove_cv_t<T>>::value, false,
> + &value, 1, sizeof(T));
> + }
> +
> + template<typename T, typename std::enable_if_t<details::is_span<T>::value, std::nullptr_t> = nullptr>
> +#else
> template<typename T>
> - ControlValue(T value)
> - : type_(details::control_type<std::remove_cv_t<T>>::value)
> +#endif
That #iffery is pretty horrible, removes one function and changes the
template instantiation of this below function ?
Though seeing the repeating pattern below too - I can't offer a better
solution...
> + ControlValue(const T &value)
> + : type_(ControlTypeNone), numElements_(0)
> {
> - *reinterpret_cast<T *>(&bool_) = value;
> + set(details::control_type<std::remove_cv_t<T>>::value, true,> + value.data(), value.size(), sizeof(typename T::value_type));
What's true here ? This is not very friendly to read...
Ok - so on the plus side the bool_ is gone :-)
> }
>
> + ~ControlValue();
> +
> + ControlValue(const ControlValue &other);
> + ControlValue &operator=(const ControlValue &other);
> +
> ControlType type() const { return type_; }
> bool isNone() const { return type_ == ControlTypeNone; }
> + bool isArray() const { return isArray_; }
> + std::size_t numElements() const { return numElements_; }
> Span<const uint8_t> data() const;
>
> std::string toString() const;
> @@ -77,31 +102,61 @@ public:
> return !(*this == other);
> }
>
> +#ifndef __DOXYGEN__
> + template<typename T, typename std::enable_if_t<!details::is_span<T>::value, std::nullptr_t> = nullptr>
> + T get() const
> + {
> + assert(type_ == details::control_type<std::remove_cv_t<T>>::value);
> + assert(!isArray_);
> +
> + return *reinterpret_cast<const T *>(data().data());
> + }
> +
> + template<typename T, typename std::enable_if_t<details::is_span<T>::value, std::nullptr_t> = nullptr>
> +#else
> template<typename T>
> +#endif
> T get() const
> {
> assert(type_ == details::control_type<std::remove_cv_t<T>>::value);
> + assert(isArray_);
> +
> + using V = typename T::value_type;
> + const V *value = reinterpret_cast<const V *>(data().data());
> + return { value, numElements_ };
> + }
>
> - return *reinterpret_cast<const T *>(&bool_);
> +#ifndef __DOXYGEN__
> + template<typename T, typename std::enable_if_t<!details::is_span<T>::value, std::nullptr_t> = nullptr>
> + void set(const T &value)
> + {
> + set(details::control_type<std::remove_cv_t<T>>::value, false,
> + reinterpret_cast<const void *>(&value), 1, sizeof(T));
> }
>
> + template<typename T, typename std::enable_if_t<details::is_span<T>::value, std::nullptr_t> = nullptr>
> +#else
> template<typename T>
> +#endif
> void set(const T &value)
> {
> - type_ = details::control_type<std::remove_cv_t<T>>::value;
> - *reinterpret_cast<T *>(&bool_) = value;
> + set(details::control_type<std::remove_cv_t<T>>::value, true,
> + value.data(), value.size(), sizeof(typename T::value_type));
> }
>
> private:
> - ControlType type_;
> -
> - union {
> - bool bool_;
> - int32_t integer32_;
> - int64_t integer64_;
> - };
> + ControlType type_ : 8;
> + bool isArray_ : 1;
> + std::size_t numElements_ : 16;
> + uint64_t storage_;
> +
> + void release();
> + void set(ControlType type, bool isArray, const void *data,
> + std::size_t numElements, std::size_t elementSize);
> };
>
> +static_assert(sizeof(ControlValue) == 16, "Invalid size of ControlValue class");
Aha, I knew this ASSERT_ABI_SIZE would come up again :-)
> +
> class ControlId
> {
> public:
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index b2331ab7540d..a5a385aa1b0a 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -10,6 +10,7 @@
> #include <iomanip>
> #include <sstream>
> #include <string>
> +#include <string.h>
>
> #include "control_validator.h"
> #include "log.h"
> @@ -50,7 +51,7 @@ LOG_DEFINE_CATEGORY(Controls)
> namespace {
>
> static constexpr size_t ControlValueSize[] = {
> - [ControlTypeNone] = 1,
> + [ControlTypeNone] = 0,
> [ControlTypeBool] = sizeof(bool),
> [ControlTypeInteger32] = sizeof(int32_t),
> [ControlTypeInteger64] = sizeof(int64_t),
> @@ -80,15 +81,63 @@ static constexpr size_t ControlValueSize[] = {
> * \brief Construct an empty ControlValue.
> */
> ControlValue::ControlValue()
> - : type_(ControlTypeNone)
> + : type_(ControlTypeNone), isArray_(false), numElements_(0)
> {
> }
>
> /**
> - * \fn template<typename T> T ControlValue::ControlValue(T value)
> + * \fn template<typename T> T ControlValue::ControlValue(const T &value)
> * \brief Construct a ControlValue of type T
> * \param[in] value Initial value
> + *
> + * This function constructs a new instance of ControlValue and stores the \a
> + * value inside it. If the type \a T is equivalent to Span<R>, the instance
> + * stores an array of values of type \a R. Otherwise the instance stores a
> + * single value of type \a T. The numElements() and type() are updated to
> + * reflect the stored value.
> + */
> +
> +void ControlValue::release()
> +{
> + std::size_t size = numElements_ * ControlValueSize[type_];
> +
> + if (size > sizeof storage_) {
sizeof(storage) would read nicer to me here. ...
Oh ... uhm isn't storage a uint64_t? And therefore always 8?
> + delete[] *reinterpret_cast<char **>(&storage_);
> + storage_ = 0;
> + }
> +}
> +
> +ControlValue::~ControlValue()
> +{
> + release();
> +}
> +
> +/**
> + * \brief Contruct a ControlValue with the content of \a other
/Contruct/Construct/
> + * \param[in] other The ControlValue to copy content from
> + */
> +ControlValue::ControlValue(const ControlValue &other)
> + : type_(ControlTypeNone), numElements_(0)
> +{
> + *this = other;
> +}
> +
> +/**
> + * \brief Replace the content of the ControlValue with the one of \a other
> + * \param[in] other The ControlValue to copy content from
> + *
> + * Deep-copy the content of \a other into the ControlValue by reserving memory
> + * and copy data there in case \a other transports arrays of values in one of
> + * its pointer data members.
That's hard to parse...
> + *
> + * \return The ControlValue with its content replaced with the one of \a other
> */
> +ControlValue &ControlValue::operator=(const ControlValue &other)
> +{
> + set(other.type_, other.isArray_, other.data().data(),
> + other.numElements_, ControlValueSize[other.type_]);
> + return *this;
> +}
Do we need/desire move support to just move the allocated storage over
at all ?
>
> /**
> * \fn ControlValue::type()
> @@ -102,16 +151,33 @@ ControlValue::ControlValue()
> * \return True if the value type is ControlTypeNone, false otherwise
> */
>
> +/**
> + * \fn ControlValue::isArray()
> + * \brief Determine if the value stores an array
> + * \return True if the value stores an array, false otherwise
> + */
> +
> +/**
> + * \fn ControlValue::numElements()
> + * \brief Retrieve the number of elements stored in the ControlValue
> + *
> + * For instances storing an array, this function returns the number of elements
> + * in the array. Otherwise, it returns 1.
> + *
> + * \return The number of elements stored in the ControlValue
> + */
> +
> /**
> * \brief Retrieve the raw of a control value
> * \return The raw data of the control value as a span of uint8_t
> */
> Span<const uint8_t> ControlValue::data() const
> {
> - return {
> - reinterpret_cast<const uint8_t *>(&bool_),
> - ControlValueSize[type_]
> - };
> + std::size_t size = numElements_ * ControlValueSize[type_];
> + const uint8_t *data = size > sizeof storage_
sizeof without 'containing' what it parses really looks wrong to me :S
> + ? *reinterpret_cast<const uint8_t * const *>(&storage_)
> + : reinterpret_cast<const uint8_t *>(&storage_);
> + return { data, size };
Ahh, at least that looks better than the multiline return statement we
had before :-)
> }
>
> /**
> @@ -120,18 +186,43 @@ Span<const uint8_t> ControlValue::data() const
> */
> std::string ControlValue::toString() const
> {
> - switch (type_) {
> - case ControlTypeNone:
> - return "<None>";
> - case ControlTypeBool:
> - return bool_ ? "True" : "False";
> - case ControlTypeInteger32:
> - return std::to_string(integer32_);
> - case ControlTypeInteger64:
> - return std::to_string(integer64_);
> + if (type_ == ControlTypeNone)
> + return "<ValueType Error>";
> +
> + const uint8_t *data = ControlValue::data().data();
> + std::string str(isArray_ ? "[ " : "");
> +
> + for (unsigned int i = 0; i < numElements_; ++i) {
> + switch (type_) {
> + case ControlTypeBool: {
> + const bool *value = reinterpret_cast<const bool *>(data);
> + str += *value ? "True" : "False";
> + break;
> + }
> + case ControlTypeInteger32: {
> + const int32_t *value = reinterpret_cast<const int32_t *>(data);
> + str += std::to_string(*value);
> + break;
> + }
> + case ControlTypeInteger64: {
> + const int64_t *value = reinterpret_cast<const int64_t *>(data);
> + str += std::to_string(*value);
> + break;
> + }
> + case ControlTypeNone:
> + break;
> + }
> +
> + if (i + 1 != numElements_)
> + str += ", ";
> +
> + data += ControlValueSize[type_];
> }
>
> - return "<ValueType Error>";
> + if (isArray_)
> + str += " ]";
> +
> + return str;
Ohh toString() is neat here :-)
> }
>
> /**
> @@ -143,16 +234,13 @@ bool ControlValue::operator==(const ControlValue &other) const
> if (type_ != other.type_)
> return false;
>
> - switch (type_) {
> - case ControlTypeBool:
> - return bool_ == other.bool_;
> - case ControlTypeInteger32:
> - return integer32_ == other.integer32_;
> - case ControlTypeInteger64:
> - return integer64_ == other.integer64_;
> - default:
> + if (numElements_ != other.numElements())
> return false;
> - }
> +
> + if (isArray_ != other.isArray_)
> + return false;
> +
> + return memcmp(data().data(), other.data().data(), data().size()) == 0;
> }
>
> /**
> @@ -165,8 +253,16 @@ bool ControlValue::operator==(const ControlValue &other) const
> * \fn template<typename T> T ControlValue::get() const
> * \brief Get the control value
> *
> - * The control value type shall match the type T, otherwise the behaviour is
> - * undefined.
> + * This function returns the contained value as an instance of \a T. If the
> + * ControlValue instance stores a single value, the type \a T shall match the
> + * stored value type(). If the instance stores an array of values, the type
> + * \a T should be equal to Span<const R>, and the type \a R shall match the
> + * stored value type(). The behaviour is undefined otherwise.
> + *
> + * Note that a ControlValue instance that stores a non-array value is not
> + * equivalent to an instance that stores an array value containing a single
> + * element. The latter shall be accessed through a Span<const R> type, while
> + * the former shall be accessed through a type \a T corresponding to type().
> *
> * \return The control value
> */
> @@ -175,8 +271,41 @@ bool ControlValue::operator==(const ControlValue &other) const
> * \fn template<typename T> void ControlValue::set(const T &value)
> * \brief Set the control value to \a value
> * \param[in] value The control value
> + *
> + * This function stores the \a value in the instance. If the type \a T is
> + * equivalent to Span<R>, the instance stores an array of values of type \a R.
> + * Otherwise the instance stores a single value of type \a T. The numElements()
> + * and type() are updated to reflect the stored value.
> + *
> + * The entire content of \a value is copied to the instance, no reference to \a
> + * value or to the data it references is retained. This may be an expensive
> + * operation for Span<> values that refer to large arrays.
> */
>
> +void ControlValue::set(ControlType type, bool isArray, const void *data,
> + std::size_t numElements, std::size_t elementSize)
> +{
> + ASSERT(elementSize == ControlValueSize[type]);
> +
> + release();
> +
> + type_ = type;
> + numElements_ = numElements;
> + isArray_ = isArray;
> +
> + std::size_t size = elementSize * numElements;
> + void *storage;
> +
> + if (size > sizeof storage_) {
> + storage = reinterpret_cast<void *>(new char[size]);
> + *reinterpret_cast<void **>(&storage_) = storage;
Doesn't this need to delete/free any existing storage? Or does the
assignment do that automagically?
> + } else {
> + storage = reinterpret_cast<void *>(&storage_);
> + }
> +
> + memcpy(storage, data, size);
> +}
> +
> /**
> * \class ControlId
> * \brief Control static metadata
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list