[libcamera-devel] [PATCH v1.2 16/31] libcamera: controls: Support array controls in ControlValue
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Mar 6 16:55:56 CET 2020
Hi Kieran,
On Tue, Mar 03, 2020 at 12:23:10AM +0000, Kieran Bingham wrote:
> 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...
Should I add
enum {
ControlIsSingle = 1,
ControlIsArray = 1,
};
? I don't like the name ControlIsSingle though. Maybe this could be done
on top, if you think it's worth it ? If you can propose a better name
I'll submit a patch :-)
> 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 :-)
I think it's a good idea, and I think we should actually try to
automate that through all our classes, to have automatic ABI regression
tests. I envision that as being done through code analysis instead of
static_assert(). And I wouldn't be surprised if tools already existed
for this purpose.
> > +
> > 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. ...
sizeof is an operator and doesn't require parentheses:
https://en.cppreference.com/w/cpp/language/sizeof. I'll change it
though.
> Oh ... uhm isn't storage a uint64_t? And therefore always 8?
Correct, but that's better than hardcoding the value 8, right ? :-)
> > + 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...
Agreed. I'll drop the paragraph as I don't think it brings much.
> > + *
> > + * \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 ?
I think we do, but I think it can be done on top.
> >
> > /**
> > * \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
I'll update this.
> > + ? *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_) {
I'll update this one too.
> > + 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?
The release() call above handles it.
> > + } else {
> > + storage = reinterpret_cast<void *>(&storage_);
> > + }
> > +
> > + memcpy(storage, data, size);
> > +}
> > +
> > /**
> > * \class ControlId
> > * \brief Control static metadata
> >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list