[libcamera-devel] [PATCH v1.2 16/31] libcamera: controls: Support array controls in ControlValue
Kieran Bingham
kieran at bingham.xyz
Sat Mar 7 19:04:23 CET 2020
Hi Laurent,
On Fri, 6 Mar 2020, 15:56 Laurent Pinchart, <
laurent.pinchart at ideasonboard.com> wrote:
> 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 ? :-)
>
Indeed :-)
> > > + delete[] *reinterpret_cast<char **>(&storage_);
> > > + storage_ = 0;
> > > + }
> > > +}
> > > +
> > > +ControlValue::~ControlValue()
> > > +{
> > > + release();
>
Who deletes storage_ on destruct?
Release only appears to delete in the event that size is bigger than the
storage available to help the set() call?
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? :-)
> > }
> > >
> > > /**
> > > @@ -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.
> > +
> > > + 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...
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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20200307/d7926ad3/attachment-0001.htm>
More information about the libcamera-devel
mailing list