<div dir="auto"><div>Hi Laurent,</div><div dir="auto"><br><br><div class="gmail_quote" dir="auto"><div dir="ltr" class="gmail_attr">On Fri, 6 Mar 2020, 15:56 Laurent Pinchart, <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Kieran,<br>
<br>
On Tue, Mar 03, 2020 at 12:23:10AM +0000, Kieran Bingham wrote:<br>
> On 01/03/2020 17:54, Laurent Pinchart wrote:<br>
> > From: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank" rel="noreferrer">jacopo@jmondi.org</a>><br>
> > <br>
> > Add array controls support to the ControlValue class. The polymorphic<br>
> > class can now store more than a single element and supports access and<br>
> > creation through the use of Span<>.<br>
> <br>
> Oh, but if the creation is through a span, what stores the data in the<br>
> ControlValue ... I guess I'll see below... <aha we create a storage><br>
> <br>
> > <br>
> > Signed-off-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank" rel="noreferrer">jacopo@jmondi.org</a>><br>
> > Signed-off-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank" rel="noreferrer">laurent.pinchart@ideasonboard.com</a>><br>
> <br>
> Some comments, but nothing you can't handle...<br>
> <br>
> Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank" rel="noreferrer">kieran.bingham@ideasonboard.com</a>><br>
> <br>
> > ---<br>
> > Changes since v1:<br>
> > <br>
> > - Use T::value_type instead of T::element_type to benefit from<br>
> > std::remove_cv<br>
> > - Fix ControlTypeNone test in ControlValue::toString()<br>
> > - Separate array elements with ", " in ControlValue::toString()<br>
> > ---<br>
> > include/libcamera/controls.h | 81 ++++++++++++---<br>
> > src/libcamera/controls.cpp | 185 +++++++++++++++++++++++++++++------<br>
> > 2 files changed, 225 insertions(+), 41 deletions(-)<br>
> > <br>
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h<br>
> > index 4538be06af93..1e24ae30ab36 100644<br>
> > --- a/include/libcamera/controls.h<br>
> > +++ b/include/libcamera/controls.h<br>
> > @@ -9,6 +9,7 @@<br>
> > #define __LIBCAMERA_CONTROLS_H__<br>
> > <br>
> > #include <assert.h><br>
> > +#include <stdint.h><br>
> > #include <string><br>
> > #include <unordered_map><br>
> > <br>
> > @@ -51,6 +52,10 @@ struct control_type<int64_t> {<br>
> > static constexpr ControlType value = ControlTypeInteger64;<br>
> > };<br>
> > <br>
> > +template<typename T, std::size_t N><br>
> > +struct control_type<Span<T, N>> : public control_type<std::remove_cv_t<T>> {<br>
> > +};<br>
> > +<br>
> > } /* namespace details */<br>
> > <br>
> > class ControlValue<br>
> > @@ -58,15 +63,35 @@ class ControlValue<br>
> > public:<br>
> > ControlValue();<br>
> > <br>
> > +#ifndef __DOXYGEN__<br>
> > + template<typename T, typename std::enable_if_t<!details::is_span<T>::value, std::nullptr_t> = nullptr><br>
> > + ControlValue(const T &value)<br>
> > + : type_(ControlTypeNone), numElements_(0)<br>
> > + {<br>
> > + set(details::control_type<std::remove_cv_t<T>>::value, false,<br>
> > + &value, 1, sizeof(T));<br>
> > + }<br>
> > +<br>
> > + template<typename T, typename std::enable_if_t<details::is_span<T>::value, std::nullptr_t> = nullptr><br>
> > +#else<br>
> > template<typename T><br>
> > - ControlValue(T value)<br>
> > - : type_(details::control_type<std::remove_cv_t<T>>::value)<br>
> > +#endif<br>
> <br>
> That #iffery is pretty horrible, removes one function and changes the<br>
> template instantiation of this below function ?<br>
> <br>
> Though seeing the repeating pattern below too - I can't offer a better<br>
> solution...<br>
> <br>
> > + ControlValue(const T &value)<br>
> > + : type_(ControlTypeNone), numElements_(0)<br>
> > {<br>
> > - *reinterpret_cast<T *>(&bool_) = value;<br>
> > + set(details::control_type<std::remove_cv_t<T>>::value, true>,<br>
> > + value.data(), value.size(), sizeof(typename T::value_type));<br>
> <br>
> What's true here ? This is not very friendly to read...<br>
<br>
Should I add<br>
<br>
enum {<br>
ControlIsSingle = 1,<br>
ControlIsArray = 1,<br>
};<br>
<br>
? I don't like the name ControlIsSingle though. Maybe this could be done<br>
on top, if you think it's worth it ? If you can propose a better name<br>
I'll submit a patch :-)<br>
<br>
> Ok - so on the plus side the bool_ is gone :-)<br>
> <br>
> > }<br>
> > <br>
> > + ~ControlValue();<br>
> > +<br>
> > + ControlValue(const ControlValue &other);<br>
> > + ControlValue &operator=(const ControlValue &other);<br>
> > +<br>
> > ControlType type() const { return type_; }<br>
> > bool isNone() const { return type_ == ControlTypeNone; }<br>
> > + bool isArray() const { return isArray_; }<br>
> > + std::size_t numElements() const { return numElements_; }<br>
> > Span<const uint8_t> data() const;<br>
> > <br>
> > std::string toString() const;<br>
> > @@ -77,31 +102,61 @@ public:<br>
> > return !(*this == other);<br>
> > }<br>
> > <br>
> > +#ifndef __DOXYGEN__<br>
> > + template<typename T, typename std::enable_if_t<!details::is_span<T>::value, std::nullptr_t> = nullptr><br>
> > + T get() const<br>
> > + {<br>
> > + assert(type_ == details::control_type<std::remove_cv_t<T>>::value);<br>
> > + assert(!isArray_);<br>
> > +<br>
> > + return *reinterpret_cast<const T *>(data().data());<br>
> > + }<br>
> > +<br>
> > + template<typename T, typename std::enable_if_t<details::is_span<T>::value, std::nullptr_t> = nullptr><br>
> > +#else<br>
> > template<typename T><br>
> > +#endif<br>
> > T get() const<br>
> > {<br>
> > assert(type_ == details::control_type<std::remove_cv_t<T>>::value);<br>
> > + assert(isArray_);<br>
> > +<br>
> > + using V = typename T::value_type;<br>
> > + const V *value = reinterpret_cast<const V *>(data().data());<br>
> > + return { value, numElements_ };<br>
> > + }<br>
> > <br>
> > - return *reinterpret_cast<const T *>(&bool_);<br>
> > +#ifndef __DOXYGEN__<br>
> > + template<typename T, typename std::enable_if_t<!details::is_span<T>::value, std::nullptr_t> = nullptr><br>
> > + void set(const T &value)<br>
> > + {<br>
> > + set(details::control_type<std::remove_cv_t<T>>::value, false,<br>
> > + reinterpret_cast<const void *>(&value), 1, sizeof(T));<br>
> > }<br>
> > <br>
> > + template<typename T, typename std::enable_if_t<details::is_span<T>::value, std::nullptr_t> = nullptr><br>
> > +#else<br>
> > template<typename T><br>
> > +#endif<br>
> > void set(const T &value)<br>
> > {<br>
> > - type_ = details::control_type<std::remove_cv_t<T>>::value;<br>
> > - *reinterpret_cast<T *>(&bool_) = value;<br>
> > + set(details::control_type<std::remove_cv_t<T>>::value, true,<br>
> > + value.data(), value.size(), sizeof(typename T::value_type));<br>
> > }<br>
> > <br>
> > private:<br>
> > - ControlType type_;<br>
> > -<br>
> > - union {<br>
> > - bool bool_;<br>
> > - int32_t integer32_;<br>
> > - int64_t integer64_;<br>
> > - };<br>
> > + ControlType type_ : 8;<br>
> > + bool isArray_ : 1;<br>
> > + std::size_t numElements_ : 16;<br>
> > + uint64_t storage_;<br>
> > +<br>
> > + void release();<br>
> > + void set(ControlType type, bool isArray, const void *data,<br>
> > + std::size_t numElements, std::size_t elementSize);<br>
> > };<br>
> > <br>
> > +static_assert(sizeof(ControlValue) == 16, "Invalid size of ControlValue class");<br>
> <br>
> Aha, I knew this ASSERT_ABI_SIZE would come up again :-)<br>
<br>
I think it's a good idea, and I think we should actually try to<br>
automate that through all our classes, to have automatic ABI regression<br>
tests. I envision that as being done through code analysis instead of<br>
static_assert(). And I wouldn't be surprised if tools already existed<br>
for this purpose.<br>
<br>
> > +<br>
> > class ControlId<br>
> > {<br>
> > public:<br>
> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp<br>
> > index b2331ab7540d..a5a385aa1b0a 100644<br>
> > --- a/src/libcamera/controls.cpp<br>
> > +++ b/src/libcamera/controls.cpp<br>
> > @@ -10,6 +10,7 @@<br>
> > #include <iomanip><br>
> > #include <sstream><br>
> > #include <string><br>
> > +#include <string.h><br>
> > <br>
> > #include "control_validator.h"<br>
> > #include "log.h"<br>
> > @@ -50,7 +51,7 @@ LOG_DEFINE_CATEGORY(Controls)<br>
> > namespace {<br>
> > <br>
> > static constexpr size_t ControlValueSize[] = {<br>
> > - [ControlTypeNone] = 1,<br>
> > + [ControlTypeNone] = 0,<br>
> > [ControlTypeBool] = sizeof(bool),<br>
> > [ControlTypeInteger32] = sizeof(int32_t),<br>
> > [ControlTypeInteger64] = sizeof(int64_t),<br>
> > @@ -80,15 +81,63 @@ static constexpr size_t ControlValueSize[] = {<br>
> > * \brief Construct an empty ControlValue.<br>
> > */<br>
> > ControlValue::ControlValue()<br>
> > - : type_(ControlTypeNone)<br>
> > + : type_(ControlTypeNone), isArray_(false), numElements_(0)<br>
> > {<br>
> > }<br>
> > <br>
> > /**<br>
> > - * \fn template<typename T> T ControlValue::ControlValue(T value)<br>
> > + * \fn template<typename T> T ControlValue::ControlValue(const T &value)<br>
> > * \brief Construct a ControlValue of type T<br>
> > * \param[in] value Initial value<br>
> > + *<br>
> > + * This function constructs a new instance of ControlValue and stores the \a<br>
> > + * value inside it. If the type \a T is equivalent to Span<R>, the instance<br>
> > + * stores an array of values of type \a R. Otherwise the instance stores a<br>
> > + * single value of type \a T. The numElements() and type() are updated to<br>
> > + * reflect the stored value.<br>
> > + */<br>
> > +<br>
> > +void ControlValue::release()<br>
> > +{<br>
> > + std::size_t size = numElements_ * ControlValueSize[type_];<br>
> > +<br>
> > + if (size > sizeof storage_) {<br>
> <br>
> sizeof(storage) would read nicer to me here. ...<br>
<br>
sizeof is an operator and doesn't require parentheses:<br>
<a href="https://en.cppreference.com/w/cpp/language/sizeof" rel="noreferrer noreferrer" target="_blank">https://en.cppreference.com/w/cpp/language/sizeof</a>. I'll change it<br>
though.<br>
<br>
> Oh ... uhm isn't storage a uint64_t? And therefore always 8?<br>
<br>
Correct, but that's better than hardcoding the value 8, right ? :-)<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Indeed :-)</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote" dir="auto"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> > + delete[] *reinterpret_cast<char **>(&storage_);<br>
> > + storage_ = 0;<br>
> > + }<br>
> > +}<br>
> > +<br>
> > +ControlValue::~ControlValue()<br>
> > +{<br>
> > + release();<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Who deletes storage_ on destruct?</div><div dir="auto"><br></div><div dir="auto">Release only appears to delete in the event that size is bigger than the storage available to help the set() call?</div><div dir="auto"><br></div><div dir="auto">That's making the reallocation below a bit ugly only to provide a function that doesn't do anything to the destructor?</div><div dir="auto"><br></div><div dir="auto">(Doesn't do anything; based on assumption that after the object is used it has an allocation which is of suitable size already)</div><div dir="auto"><br></div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote" dir="auto"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> > +}<br>
> > +<br>
> > +/**<br>
> > + * \brief Contruct a ControlValue with the content of \a other<br>
> <br>
> /Contruct/Construct/<br>
> <br>
> > + * \param[in] other The ControlValue to copy content from<br>
> > + */<br>
> > +ControlValue::ControlValue(const ControlValue &other)<br>
> > + : type_(ControlTypeNone), numElements_(0)<br>
> > +{<br>
> > + *this = other;<br>
> > +}<br>
> > +<br>
> > +/**<br>
> > + * \brief Replace the content of the ControlValue with the one of \a other<br>
> > + * \param[in] other The ControlValue to copy content from<br>
> > + *<br>
> > + * Deep-copy the content of \a other into the ControlValue by reserving memory<br>
> > + * and copy data there in case \a other transports arrays of values in one of<br>
> > + * its pointer data members.<br>
> <br>
> That's hard to parse...<br>
<br>
Agreed. I'll drop the paragraph as I don't think it brings much.<br>
<br>
> > + *<br>
> > + * \return The ControlValue with its content replaced with the one of \a other<br>
> > */<br>
> > +ControlValue &ControlValue::operator=(const ControlValue &other)<br>
> > +{<br>
> > + set(other.type_, other.isArray_, other.data().data(),<br>
> > + other.numElements_, ControlValueSize[other.type_]);<br>
> > + return *this;<br>
> > +}<br>
> <br>
> Do we need/desire move support to just move the allocated storage over<br>
> at all ?<br>
<br>
I think we do, but I think it can be done on top.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Sure</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote" dir="auto"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> > <br>
> > /**<br>
> > * \fn ControlValue::type()<br>
> > @@ -102,16 +151,33 @@ ControlValue::ControlValue()<br>
> > * \return True if the value type is ControlTypeNone, false otherwise<br>
> > */<br>
> > <br>
> > +/**<br>
> > + * \fn ControlValue::isArray()<br>
> > + * \brief Determine if the value stores an array<br>
> > + * \return True if the value stores an array, false otherwise<br>
> > + */<br>
> > +<br>
> > +/**<br>
> > + * \fn ControlValue::numElements()<br>
> > + * \brief Retrieve the number of elements stored in the ControlValue<br>
> > + *<br>
> > + * For instances storing an array, this function returns the number of elements<br>
> > + * in the array. Otherwise, it returns 1.<br>
> > + *<br>
> > + * \return The number of elements stored in the ControlValue<br>
> > + */<br>
> > +<br>
> > /**<br>
> > * \brief Retrieve the raw of a control value<br>
> > * \return The raw data of the control value as a span of uint8_t<br>
> > */<br>
> > Span<const uint8_t> ControlValue::data() const<br>
> > {<br>
> > - return {<br>
> > - reinterpret_cast<const uint8_t *>(&bool_),<br>
> > - ControlValueSize[type_]<br>
> > - };<br>
> > + std::size_t size = numElements_ * ControlValueSize[type_];<br>
> > + const uint8_t *data = size > sizeof storage_<br>
> <br>
> sizeof without 'containing' what it parses really looks wrong to me :S<br>
<br>
I'll update this.<br>
<br>
> > + ? *reinterpret_cast<const uint8_t * const *>(&storage_)<br>
> > + : reinterpret_cast<const uint8_t *>(&storage_);<br>
> > + return { data, size };<br>
> <br>
> Ahh, at least that looks better than the multiline return statement we<br>
> had before :-)<br>
> <br>
> > }<br>
> > <br>
> > /**<br>
> > @@ -120,18 +186,43 @@ Span<const uint8_t> ControlValue::data() const<br>
> > */<br>
> > std::string ControlValue::toString() const<br>
> > {<br>
> > - switch (type_) {<br>
> > - case ControlTypeNone:<br>
> > - return "<None>";<br>
> > - case ControlTypeBool:<br>
> > - return bool_ ? "True" : "False";<br>
> > - case ControlTypeInteger32:<br>
> > - return std::to_string(integer32_);<br>
> > - case ControlTypeInteger64:<br>
> > - return std::to_string(integer64_);<br>
> > + if (type_ == ControlTypeNone)<br>
> > + return "<ValueType Error>";<br>
> > +<br>
> > + const uint8_t *data = ControlValue::data().data();<br>
> > + std::string str(isArray_ ? "[ " : "");<br>
> > +<br>
> > + for (unsigned int i = 0; i < numElements_; ++i) {<br>
> > + switch (type_) {<br>
> > + case ControlTypeBool: {<br>
> > + const bool *value = reinterpret_cast<const bool *>(data);<br>
> > + str += *value ? "True" : "False";<br>
> > + break;<br>
> > + }<br>
> > + case ControlTypeInteger32: {<br>
> > + const int32_t *value = reinterpret_cast<const int32_t *>(data);<br>
> > + str += std::to_string(*value);<br>
> > + break;<br>
> > + }<br>
> > + case ControlTypeInteger64: {<br>
> > + const int64_t *value = reinterpret_cast<const int64_t *>(data);<br>
> > + str += std::to_string(*value);<br>
> > + break;<br>
> > + }<br>
> > + case ControlTypeNone:<br>
> > + break;<br>
> > + }<br>
> > +<br>
> > + if (i + 1 != numElements_)<br>
> > + str += ", ";<br>
> > +<br>
> > + data += ControlValueSize[type_];<br>
> > }<br>
> > <br>
> > - return "<ValueType Error>";<br>
> > + if (isArray_)<br>
> > + str += " ]";<br>
> > +<br>
> > + return str;<br>
> <br>
> Ohh toString() is neat here :-)<br>
> <br>
> > }<br>
> > <br>
> > /**<br>
> > @@ -143,16 +234,13 @@ bool ControlValue::operator==(const ControlValue &other) const<br>
> > if (type_ != other.type_)<br>
> > return false;<br>
> > <br>
> > - switch (type_) {<br>
> > - case ControlTypeBool:<br>
> > - return bool_ == other.bool_;<br>
> > - case ControlTypeInteger32:<br>
> > - return integer32_ == other.integer32_;<br>
> > - case ControlTypeInteger64:<br>
> > - return integer64_ == other.integer64_;<br>
> > - default:<br>
> > + if (numElements_ != other.numElements())<br>
> > return false;<br>
> > - }<br>
> > +<br>
> > + if (isArray_ != other.isArray_)<br>
> > + return false;<br>
> > +<br>
> > + return memcmp(data().data(), other.data().data(), data().size()) == 0;<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Is there some data involved in that code by any chance? :-)</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote" dir="auto"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> > }<br>
> > <br>
> > /**<br>
> > @@ -165,8 +253,16 @@ bool ControlValue::operator==(const ControlValue &other) const<br>
> > * \fn template<typename T> T ControlValue::get() const<br>
> > * \brief Get the control value<br>
> > *<br>
> > - * The control value type shall match the type T, otherwise the behaviour is<br>
> > - * undefined.<br>
> > + * This function returns the contained value as an instance of \a T. If the<br>
> > + * ControlValue instance stores a single value, the type \a T shall match the<br>
> > + * stored value type(). If the instance stores an array of values, the type<br>
> > + * \a T should be equal to Span<const R>, and the type \a R shall match the<br>
> > + * stored value type(). The behaviour is undefined otherwise.<br>
> > + *<br>
> > + * Note that a ControlValue instance that stores a non-array value is not<br>
> > + * equivalent to an instance that stores an array value containing a single<br>
> > + * element. The latter shall be accessed through a Span<const R> type, while<br>
> > + * the former shall be accessed through a type \a T corresponding to type().<br>
> > *<br>
> > * \return The control value<br>
> > */<br>
> > @@ -175,8 +271,41 @@ bool ControlValue::operator==(const ControlValue &other) const<br>
> > * \fn template<typename T> void ControlValue::set(const T &value)<br>
> > * \brief Set the control value to \a value<br>
> > * \param[in] value The control value<br>
> > + *<br>
> > + * This function stores the \a value in the instance. If the type \a T is<br>
> > + * equivalent to Span<R>, the instance stores an array of values of type \a R.<br>
> > + * Otherwise the instance stores a single value of type \a T. The numElements()<br>
> > + * and type() are updated to reflect the stored value.<br>
> > + *<br>
> > + * The entire content of \a value is copied to the instance, no reference to \a<br>
> > + * value or to the data it references is retained. This may be an expensive<br>
> > + * operation for Span<> values that refer to large arrays.<br>
> > */<br>
> > <br>
> > +void ControlValue::set(ControlType type, bool isArray, const void *data,<br>
> > + std::size_t numElements, std::size_t elementSize)<br>
> > +{<br>
> > + ASSERT(elementSize == ControlValueSize[type]);<br>
> > +<br>
> > + release();<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">I hadn't noticed this here.</div><div dir="auto"><br></div><div dir="auto">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.</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote" dir="auto"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> > +<br>
> > + type_ = type;<br>
> > + numElements_ = numElements;<br>
> > + isArray_ = isArray;<br>
> > +<br>
> > + std::size_t size = elementSize * numElements;<br>
> > + void *storage;<br>
> > +<br>
> > + if (size > sizeof storage_) {<br>
<br>
I'll update this one too.<br>
<br>
> > + storage = reinterpret_cast<void *>(new char[size]);<br>
> > + *reinterpret_cast<void **>(&storage_) = storage;<br>
> <br>
> Doesn't this need to delete/free any existing storage? Or does the<br>
> assignment do that automagically?<br>
<br>
The release() call above handles it.<br>
<br>
> > + } else {<br>
> > + storage = reinterpret_cast<void *>(&storage_);<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Because now you've highlighted the release call, this "looks" like it's setting storage to a "released" allocation :-(</div><div dir="auto"><br></div><div dir="auto">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...</div><div dir="auto"><br></div><div dir="auto">Could you verify that please and maybe simplify the code to make sure it's clear?</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote" dir="auto"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> > + }<br>
> > +<br>
> > + memcpy(storage, data, size);<br>
> > +}<br>
> > +<br>
> > /**<br>
> > * \class ControlId<br>
> > * \brief Control static metadata<br>
> > <br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div></div>