[libcamera-devel] [PATCH v1.2 16/31] libcamera: controls: Support array controls in ControlValue
Kieran Bingham
kieran at bingham.xyz
Sat Mar 7 23:07:37 CET 2020
Hi Laurent
On 07/03/2020 18:17, Laurent Pinchart wrote:
> 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...
>>>>
Continued ramblings below are unfortunately out-of-order due to replying
in situ on existing comments but the general message is:
Ok - so no leak like I feared, and except the fact that I misinterpreted
the definition/usage of storage_ which might need some comment to make
it clearer (or might not because I might just be overtired), the
following still stands ... and I don't think there's any blocker on this
series :-)
>>>> 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?
Ah, my (incorrect) inference here was that it was supposed to not always
be 8...
>>>
>>> Correct, but that's better than hardcoding the value 8, right ? :-)
>>
>> Indeed :-)
And now I read that again it clears up my earlier confusion. I had seen
that storage_ was being used as a pointer, and thus was expecting the
sizeofs to be determining the size of the available memory to store.
It's the fact that this > sizeof(storage_) determines if the storage_ is
used as those bytes directly or as a pointer to an allocation.
It seems that's easy to miss - and can cause confusion. Have I missed
the documentation or comments that clears that up? If not - could
something be added please?
>>
>>>>> + 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
Ahhhhhhhh (that's both an 'Aha, and an Argghhh' in one)
Even here where you call it 'internal' storage got confusing. I suddenly
thought - oh there's some other storage being pointed to somewehre :-(
If that was a 'union' the code would have been self documenting - but
because it wasn't .. I got confused.
I'm not necessarily suggesting turn it into a union, as that might just
be adding lines for no real gain - but a comment somewhere to directly
state how it's used could help.
> (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).
Yes, it's the fact that the 'internal' storage gets 'used' as a pointer,
rather than it always being a pointer that I seem to have completely
glossed over.
Of course if it was always a pointer it would have been declared as such
so that maybe should have given the game away. ... maybe I was/am just
too tired.
Ho hum ... all the more reason to go on holiday next week :-)
>>>> +
>>>>> + 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).
>
Aha so uint64_t storage_ is actually:
union {
uint64_t int64; /* Store Control Values which fit directly */
void *ptr; /* External allocation required */
}
Where it will either store the data in an int64 or use storage_ as a
pointer to allocated memory.
I completely misinterpreted it on first read as it only being only
essentially a void *ptr (or I guess by that definition it's a uintptr_t?)
>> 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
>
More information about the libcamera-devel
mailing list