[libcamera-devel] [PATCH v1.2 16/31] libcamera: controls: Support array controls in ControlValue
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Mar 7 19:17:31 CET 2020
Hi Kieran,
On Sat, Mar 07, 2020 at 06:04:23PM +0000, Kieran Bingham wrote:
> On Fri, 6 Mar 2020, 15:56 Laurent Pinchart wrote:
> > 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, typenamestd::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 ? :-)
>
> Indeed :-)
>
> > > > + delete[] *reinterpret_cast<char **>(&storage_);
> > > > + storage_ = 0;
> > > > + }
> > > > +}
> > > > +
> > > > +ControlValue::~ControlValue()
> > > > +{
> > > > + release();
>
> Who deletes storage_ on destruct?
release() does.
> Release only appears to delete in the event that size is bigger than the
> storage available to help the set() call?
release() deletes the storage if it has been allocated. We allocate
external storage in case the data won't fit in the internal storage
(size > 8, see the set() function below), and delete it in release()
using the same condition.
> That's making the reallocation below a bit ugly only to provide a function
> that doesn't do anything to the destructor?
>
> (Doesn't do anything; based on assumption that after the object is used it
> has an allocation which is of suitable size already)
>
> > > +}
> > > > +
> > > > +/**
> > > > + * \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.
>
> Sure
>
> > > >
> > > > /**
> > > > * \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;
>
> Is there some data involved in that code by any chance? :-)
So you've spotted the subliminal message ;-)
> > > }
> > > >
> > > > /**
> > > > @@ -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();
>
> I hadn't noticed this here.
>
> What isn't clear here is that release is conditional on size > storage, so
> i think this would be clearer to perform the delete before new below.
The purpose of release() is to free storage if it has been allocated, so
that the callers don't need to care. We need to free storage here if it
has been allocated regardless of the new size (and thus regardless of
this set() call will end up allocating memory or using internal
storage).
> > > +
> > > > + 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_);
>
> Because now you've highlighted the release call, this "looks" like it's
> setting storage to a "released" allocation :-(
>
> And as far as i can tell with a small view on a phone, release() isn't
> going to do anything in the destructor ... Which is a leak...
In this branch we use the internal storage, so release() doesn't need to
free it. In the above branch we allocate external storage, which will be
freed by release() in the destructor (or when this function is called
again).
> Could you verify that please and maybe simplify the code to make sure it's
> clear?
>
> > > + }
> > > > +
> > > > + memcpy(storage, data, size);
> > > > +}
> > > > +
> > > > /**
> > > > * \class ControlId
> > > > * \brief Control static metadata
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list