[libcamera-devel] [PATCH v1.2 16/31] libcamera: controls: Support array controls in ControlValue
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Mar 8 00:20:40 CET 2020
Hi Kieran,
On Sat, Mar 07, 2020 at 10:07:37PM +0000, Kieran Bingham wrote:
> On 07/03/2020 18:17, Laurent Pinchart wrote:
> > 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?
Does "[PATCH] libcamera: controls: Fix strict aliasing violation" help ?
I can also add more documentation on top.
> >>>>> + 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
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list