[{"id":3915,"web_url":"https://patchwork.libcamera.org/comment/3915/","msgid":"<5b0e6fd3-7600-5910-118c-18d37b5f6872@ideasonboard.com>","date":"2020-03-03T00:23:10","subject":"Re: [libcamera-devel] [PATCH v1.2 16/31] libcamera: controls:\n\tSupport array controls in ControlValue","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 01/03/2020 17:54, Laurent Pinchart wrote:\n> From: Jacopo Mondi \n> \n> Add array controls support to the ControlValue class. The polymorphic\n> class can now store more than a single element and supports access and\n> creation through the use of Span<>.\n\nOh, but if the creation is through a span, what stores the data in the\nControlValue ... I guess I'll see below... \n\n> \n> Signed-off-by: Jacopo Mondi \n> Signed-off-by: Laurent Pinchart \n\nSome comments, but nothing you can't handle...\n\nReviewed-by: Kieran Bingham \n\n> ---\n> Changes since v1:\n> \n> - Use T::value_type instead of T::element_type to benefit from\n> std::remove_cv\n> - Fix ControlTypeNone test in ControlValue::toString()\n> - Separate array elements with \", \" in ControlValue::toString()\n> ---\n> include/libcamera/controls.h | 81 ++++++++++++---\n> src/libcamera/controls.cpp | 185 +++++++++++++++++++++++++++++------\n> 2 files changed, 225 insertions(+), 41 deletions(-)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 4538be06af93..1e24ae30ab36 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -9,6 +9,7 @@\n> #define __LIBCAMERA_CONTROLS_H__\n> \n> #include \n> +#include \n> #include \n> #include \n> \n> @@ -51,6 +52,10 @@ struct control_type {\n> \tstatic constexpr ControlType value = ControlTypeInteger64;\n> };\n> \n> +template\n> +struct control_type> : public control_type> {\n> +};\n> +\n> } /* namespace details */\n> \n> class ControlValue\n> @@ -58,15 +63,35 @@ class ControlValue\n> public:\n> \tControlValue();\n> \n> +#ifndef __DOXYGEN__\n> +\ttemplate::value, std::nullptr_t> = nullptr>\n> +\tControlValue(const T &value)\n> +\t\t: type_(ControlTypeNone), numElements_(0)\n> +\t{\n> +\t\tset(details::control_type>::value, false,\n> +\t\t &value, 1, sizeof(T));\n> +\t}\n> +\n> +\ttemplate::value, std::nullptr_t> = nullptr>\n> +#else\n> \ttemplate\n> -\tControlValue(T value)\n> -\t\t: type_(details::control_type>::value)\n> +#endif\n\nThat #iffery is pretty horrible, removes one function and changes the\ntemplate instantiation of this below function ?\n\nThough seeing the repeating pattern below too - I can't offer a better\nsolution...\n\n> +\tControlValue(const T &value)\n> +\t\t: type_(ControlTypeNone), numElements_(0)\n> \t{\n> -\t\t*reinterpret_cast(&bool_) = value;\n> +\t\tset(details::control_type>::value, true,> +\t\t value.data(), value.size(), sizeof(typename T::value_type));\n\n\nWhat's true here ? This is not very friendly to read...\n\nOk - so on the plus side the bool_ is gone :-)\n\n> \t}\n> \n> +\t~ControlValue();\n> +\n> +\tControlValue(const ControlValue &other);\n> +\tControlValue &operator=(const ControlValue &other);\n> +\n> \tControlType type() const { return type_; }\n> \tbool isNone() const { return type_ == ControlTypeNone; }\n> +\tbool isArray() const { return isArray_; }\n> +\tstd::size_t numElements() const { return numElements_; }\n> \tSpan data() const;\n> \n> \tstd::string toString() const;\n> @@ -77,31 +102,61 @@ public:\n> \t\treturn !(*this == other);\n> \t}\n> \n> +#ifndef __DOXYGEN__\n> +\ttemplate::value, std::nullptr_t> = nullptr>\n> +\tT get() const\n> +\t{\n> +\t\tassert(type_ == details::control_type>::value);\n> +\t\tassert(!isArray_);\n> +\n> +\t\treturn *reinterpret_cast(data().data());\n> +\t}\n> +\n> +\ttemplate::value, std::nullptr_t> = nullptr>\n> +#else\n> \ttemplate\n> +#endif\n> \tT get() const\n> \t{\n> \t\tassert(type_ == details::control_type>::value);\n> +\t\tassert(isArray_);\n> +\n> +\t\tusing V = typename T::value_type;\n> +\t\tconst V *value = reinterpret_cast(data().data());\n> +\t\treturn { value, numElements_ };\n> +\t}\n> \n> -\t\treturn *reinterpret_cast(&bool_);\n> +#ifndef __DOXYGEN__\n> +\ttemplate::value, std::nullptr_t> = nullptr>\n> +\tvoid set(const T &value)\n> +\t{\n> +\t\tset(details::control_type>::value, false,\n> +\t\t reinterpret_cast(&value), 1, sizeof(T));\n> \t}\n> \n> +\ttemplate::value, std::nullptr_t> = nullptr>\n> +#else\n> \ttemplate\n> +#endif\n> \tvoid set(const T &value)\n> \t{\n> -\t\ttype_ = details::control_type>::value;\n> -\t\t*reinterpret_cast(&bool_) = value;\n> +\t\tset(details::control_type>::value, true,\n> +\t\t value.data(), value.size(), sizeof(typename T::value_type));\n> \t}\n> \n> private:\n> -\tControlType type_;\n> -\n> -\tunion {\n> -\t\tbool bool_;\n> -\t\tint32_t integer32_;\n> -\t\tint64_t integer64_;\n> -\t};\n> +\tControlType type_ : 8;\n> +\tbool isArray_ : 1;\n> +\tstd::size_t numElements_ : 16;\n> +\tuint64_t storage_;\n> +\n> +\tvoid release();\n> +\tvoid set(ControlType type, bool isArray, const void *data,\n> +\t\t std::size_t numElements, std::size_t elementSize);\n> };\n> \n> +static_assert(sizeof(ControlValue) == 16, \"Invalid size of ControlValue class\");\n\nAha, I knew this ASSERT_ABI_SIZE would come up again :-)\n\n\n> +\n> class ControlId\n> {\n> public:\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index b2331ab7540d..a5a385aa1b0a 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -10,6 +10,7 @@\n> #include \n> #include \n> #include \n> +#include \n> \n> #include \"control_validator.h\"\n> #include \"log.h\"\n> @@ -50,7 +51,7 @@ LOG_DEFINE_CATEGORY(Controls)\n> namespace {\n> \n> static constexpr size_t ControlValueSize[] = {\n> -\t[ControlTypeNone]\t\t= 1,\n> +\t[ControlTypeNone]\t\t= 0,\n> \t[ControlTypeBool]\t\t= sizeof(bool),\n> \t[ControlTypeInteger32]\t\t= sizeof(int32_t),\n> \t[ControlTypeInteger64]\t\t= sizeof(int64_t),\n> @@ -80,15 +81,63 @@ static constexpr size_t ControlValueSize[] = {\n> * \\brief Construct an empty ControlValue.\n> */\n> ControlValue::ControlValue()\n> -\t: type_(ControlTypeNone)\n> +\t: type_(ControlTypeNone), isArray_(false), numElements_(0)\n> {\n> }\n> \n> /**\n> - * \\fn template T ControlValue::ControlValue(T value)\n> + * \\fn template T ControlValue::ControlValue(const T &value)\n> * \\brief Construct a ControlValue of type T\n> * \\param[in] value Initial value\n> + *\n> + * This function constructs a new instance of ControlValue and stores the \\a\n> + * value inside it. If the type \\a T is equivalent to Span, the instance\n> + * stores an array of values of type \\a R. Otherwise the instance stores a\n> + * single value of type \\a T. The numElements() and type() are updated to\n> + * reflect the stored value.\n> + */\n> +\n> +void ControlValue::release()\n> +{\n> +\tstd::size_t size = numElements_ * ControlValueSize[type_];\n> +\n> +\tif (size > sizeof storage_) {\n\nsizeof(storage) would read nicer to me here. ...\n\nOh ... uhm isn't storage a uint64_t? And therefore always 8?\n\n> +\t\tdelete[] *reinterpret_cast(&storage_);\n> +\t\tstorage_ = 0;\n> +\t}\n> +}\n> +\n> +ControlValue::~ControlValue()\n> +{\n> +\trelease();\n> +}\n> +\n> +/**\n> + * \\brief Contruct a ControlValue with the content of \\a other\n\n/Contruct/Construct/\n\n> + * \\param[in] other The ControlValue to copy content from\n> + */\n> +ControlValue::ControlValue(const ControlValue &other)\n> +\t: type_(ControlTypeNone), numElements_(0)\n> +{\n> +\t*this = other;\n> +}\n> +\n> +/**\n> + * \\brief Replace the content of the ControlValue with the one of \\a other\n> + * \\param[in] other The ControlValue to copy content from\n> + *\n> + * Deep-copy the content of \\a other into the ControlValue by reserving memory\n> + * and copy data there in case \\a other transports arrays of values in one of\n> + * its pointer data members.\n\nThat's hard to parse...\n\n> + *\n> + * \\return The ControlValue with its content replaced with the one of \\a other\n> */\n> +ControlValue &ControlValue::operator=(const ControlValue &other)\n> +{\n> +\tset(other.type_, other.isArray_, other.data().data(),\n> +\t other.numElements_, ControlValueSize[other.type_]);\n> +\treturn *this;\n> +}\n\nDo we need/desire move support to just move the allocated storage over\nat all ?\n\n> \n> /**\n> * \\fn ControlValue::type()\n> @@ -102,16 +151,33 @@ ControlValue::ControlValue()\n> * \\return True if the value type is ControlTypeNone, false otherwise\n> */\n> \n> +/**\n> + * \\fn ControlValue::isArray()\n> + * \\brief Determine if the value stores an array\n> + * \\return True if the value stores an array, false otherwise\n> + */\n> +\n> +/**\n> + * \\fn ControlValue::numElements()\n> + * \\brief Retrieve the number of elements stored in the ControlValue\n> + *\n> + * For instances storing an array, this function returns the number of elements\n> + * in the array. Otherwise, it returns 1.\n> + *\n> + * \\return The number of elements stored in the ControlValue\n> + */\n> +\n> /**\n> * \\brief Retrieve the raw of a control value\n> * \\return The raw data of the control value as a span of uint8_t\n> */\n> Span ControlValue::data() const\n> {\n> -\treturn {\n> -\t\treinterpret_cast(&bool_),\n> -\t\tControlValueSize[type_]\n> -\t};\n> +\tstd::size_t size = numElements_ * ControlValueSize[type_];\n> +\tconst uint8_t *data = size > sizeof storage_\n\nsizeof without 'containing' what it parses really looks wrong to me :S\n\n> +\t\t\t ? *reinterpret_cast(&storage_)\n> +\t\t\t : reinterpret_cast(&storage_);\n> +\treturn { data, size };\n\nAhh, at least that looks better than the multiline return statement we\nhad before :-)\n\n\n> }\n> \n> /**\n> @@ -120,18 +186,43 @@ Span ControlValue::data() const\n> */\n> std::string ControlValue::toString() const\n> {\n> -\tswitch (type_) {\n> -\tcase ControlTypeNone:\n> -\t\treturn \"\";\n> -\tcase ControlTypeBool:\n> -\t\treturn bool_ ? \"True\" : \"False\";\n> -\tcase ControlTypeInteger32:\n> -\t\treturn std::to_string(integer32_);\n> -\tcase ControlTypeInteger64:\n> -\t\treturn std::to_string(integer64_);\n> +\tif (type_ == ControlTypeNone)\n> +\t\treturn \"\";\n> +\n> +\tconst uint8_t *data = ControlValue::data().data();\n> +\tstd::string str(isArray_ ? \"[ \" : \"\");\n> +\n> +\tfor (unsigned int i = 0; i < numElements_; ++i) {\n> +\t\tswitch (type_) {\n> +\t\tcase ControlTypeBool: {\n> +\t\t\tconst bool *value = reinterpret_cast(data);\n> +\t\t\tstr += *value ? \"True\" : \"False\";\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tcase ControlTypeInteger32: {\n> +\t\t\tconst int32_t *value = reinterpret_cast(data);\n> +\t\t\tstr += std::to_string(*value);\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tcase ControlTypeInteger64: {\n> +\t\t\tconst int64_t *value = reinterpret_cast(data);\n> +\t\t\tstr += std::to_string(*value);\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tcase ControlTypeNone:\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tif (i + 1 != numElements_)\n> +\t\t\tstr += \", \";\n> +\n> +\t\tdata += ControlValueSize[type_];\n> \t}\n> \n> -\treturn \"\";\n> +\tif (isArray_)\n> +\t\tstr += \" ]\";\n> +\n> +\treturn str;\n\nOhh toString() is neat here :-)\n\n> }\n> \n> /**\n> @@ -143,16 +234,13 @@ bool ControlValue::operator==(const ControlValue &other) const\n> \tif (type_ != other.type_)\n> \t\treturn false;\n> \n> -\tswitch (type_) {\n> -\tcase ControlTypeBool:\n> -\t\treturn bool_ == other.bool_;\n> -\tcase ControlTypeInteger32:\n> -\t\treturn integer32_ == other.integer32_;\n> -\tcase ControlTypeInteger64:\n> -\t\treturn integer64_ == other.integer64_;\n> -\tdefault:\n> +\tif (numElements_ != other.numElements())\n> \t\treturn false;\n> -\t}\n> +\n> +\tif (isArray_ != other.isArray_)\n> +\t\treturn false;\n> +\n> +\treturn memcmp(data().data(), other.data().data(), data().size()) == 0;\n> }\n> \n> /**\n> @@ -165,8 +253,16 @@ bool ControlValue::operator==(const ControlValue &other) const\n> * \\fn template T ControlValue::get() const\n> * \\brief Get the control value\n> *\n> - * The control value type shall match the type T, otherwise the behaviour is\n> - * undefined.\n> + * This function returns the contained value as an instance of \\a T. If the\n> + * ControlValue instance stores a single value, the type \\a T shall match the\n> + * stored value type(). If the instance stores an array of values, the type\n> + * \\a T should be equal to Span, and the type \\a R shall match the\n> + * stored value type(). The behaviour is undefined otherwise.\n> + *\n> + * Note that a ControlValue instance that stores a non-array value is not\n> + * equivalent to an instance that stores an array value containing a single\n> + * element. The latter shall be accessed through a Span type, while\n> + * the former shall be accessed through a type \\a T corresponding to type().\n> *\n> * \\return The control value\n> */\n> @@ -175,8 +271,41 @@ bool ControlValue::operator==(const ControlValue &other) const\n> * \\fn template void ControlValue::set(const T &value)\n> * \\brief Set the control value to \\a value\n> * \\param[in] value The control value\n> + *\n> + * This function stores the \\a value in the instance. If the type \\a T is\n> + * equivalent to Span, the instance stores an array of values of type \\a R.\n> + * Otherwise the instance stores a single value of type \\a T. The numElements()\n> + * and type() are updated to reflect the stored value.\n> + *\n> + * The entire content of \\a value is copied to the instance, no reference to \\a\n> + * value or to the data it references is retained. This may be an expensive\n> + * operation for Span<> values that refer to large arrays.\n> */\n> \n> +void ControlValue::set(ControlType type, bool isArray, const void *data,\n> +\t\t std::size_t numElements, std::size_t elementSize)\n> +{\n> +\tASSERT(elementSize == ControlValueSize[type]);\n> +\n> +\trelease();\n> +\n> +\ttype_ = type;\n> +\tnumElements_ = numElements;\n> +\tisArray_ = isArray;\n> +\n> +\tstd::size_t size = elementSize * numElements;\n> +\tvoid *storage;\n> +\n> +\tif (size > sizeof storage_) {\n> +\t\tstorage = reinterpret_cast(new char[size]);\n> +\t\t*reinterpret_cast(&storage_) = storage;\n\nDoesn't this need to delete/free any existing storage? Or does the\nassignment do that automagically?\n\n> +\t} else {\n> +\t\tstorage = reinterpret_cast(&storage_);\n> +\t}\n> +\n> +\tmemcpy(storage, data, size);\n> +}\n> +\n> /**\n> * \\class ControlId\n> * \\brief Control static metadata\n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 606996279B\n\tfor ;\n\tTue, 3 Mar 2020 01:23:14 +0100 (CET)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 51A639D0;\n\tTue, 3 Mar 2020 01:23:13 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1583194993;\n\tbh=6Ly55Gkj4+1M0GjnlPjnr0YkgZOBU0Z8WeGeRMGgrBE=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=SlzS+A7u0D7lZTWbALt3VcqaPDKGGIc9uU6ZoJiaufDIlXAEbaW10StJOWomIzToU\n\tPBw4wyMZ3NiMvLhz3Ur7m6D2s1J8m9jFbx6ZPNQLPb2QD+8OuukVO8hpQjbgBC3aA2\n\tc8BSX9MEwIf5VrNExOQXZ8BURa1lrwasplfCSEzk=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20200229164254.23604-17-laurent.pinchart@ideasonboard.com>\n\t<20200301175454.10921-1-laurent.pinchart@ideasonboard.com>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<5b0e6fd3-7600-5910-118c-18d37b5f6872@ideasonboard.com>","Date":"Tue, 3 Mar 2020 00:23:10 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.9.1","MIME-Version":"1.0","In-Reply-To":"<20200301175454.10921-1-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v1.2 16/31] libcamera: controls:\n\tSupport array controls in ControlValue","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 03 Mar 2020 00:23:14 -0000"}},{"id":3959,"web_url":"https://patchwork.libcamera.org/comment/3959/","msgid":"<20200306155556.GP4878@pendragon.ideasonboard.com>","date":"2020-03-06T15:55:56","subject":"Re: [libcamera-devel] [PATCH v1.2 16/31] libcamera: controls:\n\tSupport array controls in ControlValue","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Tue, Mar 03, 2020 at 12:23:10AM +0000, Kieran Bingham wrote:\n> On 01/03/2020 17:54, Laurent Pinchart wrote:\n> > From: Jacopo Mondi \n> > \n> > Add array controls support to the ControlValue class. The polymorphic\n> > class can now store more than a single element and supports access and\n> > creation through the use of Span<>.\n> \n> Oh, but if the creation is through a span, what stores the data in the\n> ControlValue ... I guess I'll see below... \n> \n> > \n> > Signed-off-by: Jacopo Mondi \n> > Signed-off-by: Laurent Pinchart \n> \n> Some comments, but nothing you can't handle...\n> \n> Reviewed-by: Kieran Bingham \n> \n> > ---\n> > Changes since v1:\n> > \n> > - Use T::value_type instead of T::element_type to benefit from\n> > std::remove_cv\n> > - Fix ControlTypeNone test in ControlValue::toString()\n> > - Separate array elements with \", \" in ControlValue::toString()\n> > ---\n> > include/libcamera/controls.h | 81 ++++++++++++---\n> > src/libcamera/controls.cpp | 185 +++++++++++++++++++++++++++++------\n> > 2 files changed, 225 insertions(+), 41 deletions(-)\n> > \n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > index 4538be06af93..1e24ae30ab36 100644\n> > --- a/include/libcamera/controls.h\n> > +++ b/include/libcamera/controls.h\n> > @@ -9,6 +9,7 @@\n> > #define __LIBCAMERA_CONTROLS_H__\n> > \n> > #include \n> > +#include \n> > #include \n> > #include \n> > \n> > @@ -51,6 +52,10 @@ struct control_type {\n> > \tstatic constexpr ControlType value = ControlTypeInteger64;\n> > };\n> > \n> > +template\n> > +struct control_type> : public control_type> {\n> > +};\n> > +\n> > } /* namespace details */\n> > \n> > class ControlValue\n> > @@ -58,15 +63,35 @@ class ControlValue\n> > public:\n> > \tControlValue();\n> > \n> > +#ifndef __DOXYGEN__\n> > +\ttemplate::value, std::nullptr_t> = nullptr>\n> > +\tControlValue(const T &value)\n> > +\t\t: type_(ControlTypeNone), numElements_(0)\n> > +\t{\n> > +\t\tset(details::control_type>::value, false,\n> > +\t\t &value, 1, sizeof(T));\n> > +\t}\n> > +\n> > +\ttemplate::value, std::nullptr_t> = nullptr>\n> > +#else\n> > \ttemplate\n> > -\tControlValue(T value)\n> > -\t\t: type_(details::control_type>::value)\n> > +#endif\n> \n> That #iffery is pretty horrible, removes one function and changes the\n> template instantiation of this below function ?\n> \n> Though seeing the repeating pattern below too - I can't offer a better\n> solution...\n> \n> > +\tControlValue(const T &value)\n> > +\t\t: type_(ControlTypeNone), numElements_(0)\n> > \t{\n> > -\t\t*reinterpret_cast(&bool_) = value;\n> > +\t\tset(details::control_type>::value, true>,\n> > +\t\t value.data(), value.size(), sizeof(typename T::value_type));\n> \n> What's true here ? This is not very friendly to read...\n\nShould I add\n\nenum {\n\tControlIsSingle = 1,\n\tControlIsArray = 1,\n};\n\n? I don't like the name ControlIsSingle though. Maybe this could be done\non top, if you think it's worth it ? If you can propose a better name\nI'll submit a patch :-)\n\n> Ok - so on the plus side the bool_ is gone :-)\n> \n> > \t}\n> > \n> > +\t~ControlValue();\n> > +\n> > +\tControlValue(const ControlValue &other);\n> > +\tControlValue &operator=(const ControlValue &other);\n> > +\n> > \tControlType type() const { return type_; }\n> > \tbool isNone() const { return type_ == ControlTypeNone; }\n> > +\tbool isArray() const { return isArray_; }\n> > +\tstd::size_t numElements() const { return numElements_; }\n> > \tSpan data() const;\n> > \n> > \tstd::string toString() const;\n> > @@ -77,31 +102,61 @@ public:\n> > \t\treturn !(*this == other);\n> > \t}\n> > \n> > +#ifndef __DOXYGEN__\n> > +\ttemplate::value, std::nullptr_t> = nullptr>\n> > +\tT get() const\n> > +\t{\n> > +\t\tassert(type_ == details::control_type>::value);\n> > +\t\tassert(!isArray_);\n> > +\n> > +\t\treturn *reinterpret_cast(data().data());\n> > +\t}\n> > +\n> > +\ttemplate::value, std::nullptr_t> = nullptr>\n> > +#else\n> > \ttemplate\n> > +#endif\n> > \tT get() const\n> > \t{\n> > \t\tassert(type_ == details::control_type>::value);\n> > +\t\tassert(isArray_);\n> > +\n> > +\t\tusing V = typename T::value_type;\n> > +\t\tconst V *value = reinterpret_cast(data().data());\n> > +\t\treturn { value, numElements_ };\n> > +\t}\n> > \n> > -\t\treturn *reinterpret_cast(&bool_);\n> > +#ifndef __DOXYGEN__\n> > +\ttemplate::value, std::nullptr_t> = nullptr>\n> > +\tvoid set(const T &value)\n> > +\t{\n> > +\t\tset(details::control_type>::value, false,\n> > +\t\t reinterpret_cast(&value), 1, sizeof(T));\n> > \t}\n> > \n> > +\ttemplate::value, std::nullptr_t> = nullptr>\n> > +#else\n> > \ttemplate\n> > +#endif\n> > \tvoid set(const T &value)\n> > \t{\n> > -\t\ttype_ = details::control_type>::value;\n> > -\t\t*reinterpret_cast(&bool_) = value;\n> > +\t\tset(details::control_type>::value, true,\n> > +\t\t value.data(), value.size(), sizeof(typename T::value_type));\n> > \t}\n> > \n> > private:\n> > -\tControlType type_;\n> > -\n> > -\tunion {\n> > -\t\tbool bool_;\n> > -\t\tint32_t integer32_;\n> > -\t\tint64_t integer64_;\n> > -\t};\n> > +\tControlType type_ : 8;\n> > +\tbool isArray_ : 1;\n> > +\tstd::size_t numElements_ : 16;\n> > +\tuint64_t storage_;\n> > +\n> > +\tvoid release();\n> > +\tvoid set(ControlType type, bool isArray, const void *data,\n> > +\t\t std::size_t numElements, std::size_t elementSize);\n> > };\n> > \n> > +static_assert(sizeof(ControlValue) == 16, \"Invalid size of ControlValue class\");\n> \n> Aha, I knew this ASSERT_ABI_SIZE would come up again :-)\n\nI think it's a good idea, and I think we should actually try to\nautomate that through all our classes, to have automatic ABI regression\ntests. I envision that as being done through code analysis instead of\nstatic_assert(). And I wouldn't be surprised if tools already existed\nfor this purpose.\n\n> > +\n> > class ControlId\n> > {\n> > public:\n> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > index b2331ab7540d..a5a385aa1b0a 100644\n> > --- a/src/libcamera/controls.cpp\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -10,6 +10,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > \n> > #include \"control_validator.h\"\n> > #include \"log.h\"\n> > @@ -50,7 +51,7 @@ LOG_DEFINE_CATEGORY(Controls)\n> > namespace {\n> > \n> > static constexpr size_t ControlValueSize[] = {\n> > -\t[ControlTypeNone]\t\t= 1,\n> > +\t[ControlTypeNone]\t\t= 0,\n> > \t[ControlTypeBool]\t\t= sizeof(bool),\n> > \t[ControlTypeInteger32]\t\t= sizeof(int32_t),\n> > \t[ControlTypeInteger64]\t\t= sizeof(int64_t),\n> > @@ -80,15 +81,63 @@ static constexpr size_t ControlValueSize[] = {\n> > * \\brief Construct an empty ControlValue.\n> > */\n> > ControlValue::ControlValue()\n> > -\t: type_(ControlTypeNone)\n> > +\t: type_(ControlTypeNone), isArray_(false), numElements_(0)\n> > {\n> > }\n> > \n> > /**\n> > - * \\fn template T ControlValue::ControlValue(T value)\n> > + * \\fn template T ControlValue::ControlValue(const T &value)\n> > * \\brief Construct a ControlValue of type T\n> > * \\param[in] value Initial value\n> > + *\n> > + * This function constructs a new instance of ControlValue and stores the \\a\n> > + * value inside it. If the type \\a T is equivalent to Span, the instance\n> > + * stores an array of values of type \\a R. Otherwise the instance stores a\n> > + * single value of type \\a T. The numElements() and type() are updated to\n> > + * reflect the stored value.\n> > + */\n> > +\n> > +void ControlValue::release()\n> > +{\n> > +\tstd::size_t size = numElements_ * ControlValueSize[type_];\n> > +\n> > +\tif (size > sizeof storage_) {\n> \n> sizeof(storage) would read nicer to me here. ...\n\nsizeof is an operator and doesn't require parentheses:\nhttps://en.cppreference.com/w/cpp/language/sizeof. I'll change it\nthough.\n\n> Oh ... uhm isn't storage a uint64_t? And therefore always 8?\n\nCorrect, but that's better than hardcoding the value 8, right ? :-)\n\n> > +\t\tdelete[] *reinterpret_cast(&storage_);\n> > +\t\tstorage_ = 0;\n> > +\t}\n> > +}\n> > +\n> > +ControlValue::~ControlValue()\n> > +{\n> > +\trelease();\n> > +}\n> > +\n> > +/**\n> > + * \\brief Contruct a ControlValue with the content of \\a other\n> \n> /Contruct/Construct/\n> \n> > + * \\param[in] other The ControlValue to copy content from\n> > + */\n> > +ControlValue::ControlValue(const ControlValue &other)\n> > +\t: type_(ControlTypeNone), numElements_(0)\n> > +{\n> > +\t*this = other;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Replace the content of the ControlValue with the one of \\a other\n> > + * \\param[in] other The ControlValue to copy content from\n> > + *\n> > + * Deep-copy the content of \\a other into the ControlValue by reserving memory\n> > + * and copy data there in case \\a other transports arrays of values in one of\n> > + * its pointer data members.\n> \n> That's hard to parse...\n\nAgreed. I'll drop the paragraph as I don't think it brings much.\n\n> > + *\n> > + * \\return The ControlValue with its content replaced with the one of \\a other\n> > */\n> > +ControlValue &ControlValue::operator=(const ControlValue &other)\n> > +{\n> > +\tset(other.type_, other.isArray_, other.data().data(),\n> > +\t other.numElements_, ControlValueSize[other.type_]);\n> > +\treturn *this;\n> > +}\n> \n> Do we need/desire move support to just move the allocated storage over\n> at all ?\n\nI think we do, but I think it can be done on top.\n\n> > \n> > /**\n> > * \\fn ControlValue::type()\n> > @@ -102,16 +151,33 @@ ControlValue::ControlValue()\n> > * \\return True if the value type is ControlTypeNone, false otherwise\n> > */\n> > \n> > +/**\n> > + * \\fn ControlValue::isArray()\n> > + * \\brief Determine if the value stores an array\n> > + * \\return True if the value stores an array, false otherwise\n> > + */\n> > +\n> > +/**\n> > + * \\fn ControlValue::numElements()\n> > + * \\brief Retrieve the number of elements stored in the ControlValue\n> > + *\n> > + * For instances storing an array, this function returns the number of elements\n> > + * in the array. Otherwise, it returns 1.\n> > + *\n> > + * \\return The number of elements stored in the ControlValue\n> > + */\n> > +\n> > /**\n> > * \\brief Retrieve the raw of a control value\n> > * \\return The raw data of the control value as a span of uint8_t\n> > */\n> > Span ControlValue::data() const\n> > {\n> > -\treturn {\n> > -\t\treinterpret_cast(&bool_),\n> > -\t\tControlValueSize[type_]\n> > -\t};\n> > +\tstd::size_t size = numElements_ * ControlValueSize[type_];\n> > +\tconst uint8_t *data = size > sizeof storage_\n> \n> sizeof without 'containing' what it parses really looks wrong to me :S\n\nI'll update this.\n\n> > +\t\t\t ? *reinterpret_cast(&storage_)\n> > +\t\t\t : reinterpret_cast(&storage_);\n> > +\treturn { data, size };\n> \n> Ahh, at least that looks better than the multiline return statement we\n> had before :-)\n> \n> > }\n> > \n> > /**\n> > @@ -120,18 +186,43 @@ Span ControlValue::data() const\n> > */\n> > std::string ControlValue::toString() const\n> > {\n> > -\tswitch (type_) {\n> > -\tcase ControlTypeNone:\n> > -\t\treturn \"\";\n> > -\tcase ControlTypeBool:\n> > -\t\treturn bool_ ? \"True\" : \"False\";\n> > -\tcase ControlTypeInteger32:\n> > -\t\treturn std::to_string(integer32_);\n> > -\tcase ControlTypeInteger64:\n> > -\t\treturn std::to_string(integer64_);\n> > +\tif (type_ == ControlTypeNone)\n> > +\t\treturn \"\";\n> > +\n> > +\tconst uint8_t *data = ControlValue::data().data();\n> > +\tstd::string str(isArray_ ? \"[ \" : \"\");\n> > +\n> > +\tfor (unsigned int i = 0; i < numElements_; ++i) {\n> > +\t\tswitch (type_) {\n> > +\t\tcase ControlTypeBool: {\n> > +\t\t\tconst bool *value = reinterpret_cast(data);\n> > +\t\t\tstr += *value ? \"True\" : \"False\";\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t\tcase ControlTypeInteger32: {\n> > +\t\t\tconst int32_t *value = reinterpret_cast(data);\n> > +\t\t\tstr += std::to_string(*value);\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t\tcase ControlTypeInteger64: {\n> > +\t\t\tconst int64_t *value = reinterpret_cast(data);\n> > +\t\t\tstr += std::to_string(*value);\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t\tcase ControlTypeNone:\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\n> > +\t\tif (i + 1 != numElements_)\n> > +\t\t\tstr += \", \";\n> > +\n> > +\t\tdata += ControlValueSize[type_];\n> > \t}\n> > \n> > -\treturn \"\";\n> > +\tif (isArray_)\n> > +\t\tstr += \" ]\";\n> > +\n> > +\treturn str;\n> \n> Ohh toString() is neat here :-)\n> \n> > }\n> > \n> > /**\n> > @@ -143,16 +234,13 @@ bool ControlValue::operator==(const ControlValue &other) const\n> > \tif (type_ != other.type_)\n> > \t\treturn false;\n> > \n> > -\tswitch (type_) {\n> > -\tcase ControlTypeBool:\n> > -\t\treturn bool_ == other.bool_;\n> > -\tcase ControlTypeInteger32:\n> > -\t\treturn integer32_ == other.integer32_;\n> > -\tcase ControlTypeInteger64:\n> > -\t\treturn integer64_ == other.integer64_;\n> > -\tdefault:\n> > +\tif (numElements_ != other.numElements())\n> > \t\treturn false;\n> > -\t}\n> > +\n> > +\tif (isArray_ != other.isArray_)\n> > +\t\treturn false;\n> > +\n> > +\treturn memcmp(data().data(), other.data().data(), data().size()) == 0;\n> > }\n> > \n> > /**\n> > @@ -165,8 +253,16 @@ bool ControlValue::operator==(const ControlValue &other) const\n> > * \\fn template T ControlValue::get() const\n> > * \\brief Get the control value\n> > *\n> > - * The control value type shall match the type T, otherwise the behaviour is\n> > - * undefined.\n> > + * This function returns the contained value as an instance of \\a T. If the\n> > + * ControlValue instance stores a single value, the type \\a T shall match the\n> > + * stored value type(). If the instance stores an array of values, the type\n> > + * \\a T should be equal to Span, and the type \\a R shall match the\n> > + * stored value type(). The behaviour is undefined otherwise.\n> > + *\n> > + * Note that a ControlValue instance that stores a non-array value is not\n> > + * equivalent to an instance that stores an array value containing a single\n> > + * element. The latter shall be accessed through a Span type, while\n> > + * the former shall be accessed through a type \\a T corresponding to type().\n> > *\n> > * \\return The control value\n> > */\n> > @@ -175,8 +271,41 @@ bool ControlValue::operator==(const ControlValue &other) const\n> > * \\fn template void ControlValue::set(const T &value)\n> > * \\brief Set the control value to \\a value\n> > * \\param[in] value The control value\n> > + *\n> > + * This function stores the \\a value in the instance. If the type \\a T is\n> > + * equivalent to Span, the instance stores an array of values of type \\a R.\n> > + * Otherwise the instance stores a single value of type \\a T. The numElements()\n> > + * and type() are updated to reflect the stored value.\n> > + *\n> > + * The entire content of \\a value is copied to the instance, no reference to \\a\n> > + * value or to the data it references is retained. This may be an expensive\n> > + * operation for Span<> values that refer to large arrays.\n> > */\n> > \n> > +void ControlValue::set(ControlType type, bool isArray, const void *data,\n> > +\t\t std::size_t numElements, std::size_t elementSize)\n> > +{\n> > +\tASSERT(elementSize == ControlValueSize[type]);\n> > +\n> > +\trelease();\n> > +\n> > +\ttype_ = type;\n> > +\tnumElements_ = numElements;\n> > +\tisArray_ = isArray;\n> > +\n> > +\tstd::size_t size = elementSize * numElements;\n> > +\tvoid *storage;\n> > +\n> > +\tif (size > sizeof storage_) {\n\nI'll update this one too.\n\n> > +\t\tstorage = reinterpret_cast(new char[size]);\n> > +\t\t*reinterpret_cast(&storage_) = storage;\n> \n> Doesn't this need to delete/free any existing storage? Or does the\n> assignment do that automagically?\n\nThe release() call above handles it.\n\n> > +\t} else {\n> > +\t\tstorage = reinterpret_cast(&storage_);\n> > +\t}\n> > +\n> > +\tmemcpy(storage, data, size);\n> > +}\n> > +\n> > /**\n> > * \\class ControlId\n> > * \\brief Control static metadata\n> >","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D8A1860424\n\tfor ;\n\tFri, 6 Mar 2020 16:56:00 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D963424B;\n\tFri, 6 Mar 2020 16:55:59 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1583510160;\n\tbh=QBDYkrhCh6WSwvWIdR7jkJQQWpp2TRbVgYzLz69mS00=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fpFe1e7ZWB0n+uPBP8BuSM5yXN6Hb12Ssl58/tvHU59A5eZjNyODxrGxUqpv/Xiqz\n\tJzWraQi4n9LHmqLOdMFydAvteAa2EoS1QnFtT2hc6Zr50fl8v/+Rf0AEmycTtraf00\n\tFOmhxFbmOUyzLKHsMupW1GUStcbbVRLm+p1gSl2o=","Date":"Fri, 6 Mar 2020 17:55:56 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200306155556.GP4878@pendragon.ideasonboard.com>","References":"<20200229164254.23604-17-laurent.pinchart@ideasonboard.com>\n\t<20200301175454.10921-1-laurent.pinchart@ideasonboard.com>\n\t<5b0e6fd3-7600-5910-118c-18d37b5f6872@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<5b0e6fd3-7600-5910-118c-18d37b5f6872@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v1.2 16/31] libcamera: controls:\n\tSupport array controls in ControlValue","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 06 Mar 2020 15:56:01 -0000"}},{"id":3983,"web_url":"https://patchwork.libcamera.org/comment/3983/","msgid":"","date":"2020-03-07T18:04:23","subject":"Re: [libcamera-devel] [PATCH v1.2 16/31] libcamera: controls:\n\tSupport array controls in ControlValue","submitter":{"id":38,"url":"https://patchwork.libcamera.org/api/people/38/","name":"Kieran Bingham","email":"kieran@bingham.xyz"},"content":"Hi Laurent,\n\n\nOn Fri, 6 Mar 2020, 15:56 Laurent Pinchart, <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Kieran,\n>\n> On Tue, Mar 03, 2020 at 12:23:10AM +0000, Kieran Bingham wrote:\n> > On 01/03/2020 17:54, Laurent Pinchart wrote:\n> > > From: Jacopo Mondi \n> > >\n> > > Add array controls support to the ControlValue class. The polymorphic\n> > > class can now store more than a single element and supports access and\n> > > creation through the use of Span<>.\n> >\n> > Oh, but if the creation is through a span, what stores the data in the\n> > ControlValue ... I guess I'll see below... \n> >\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > Signed-off-by: Laurent Pinchart \n> >\n> > Some comments, but nothing you can't handle...\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> > > ---\n> > > Changes since v1:\n> > >\n> > > - Use T::value_type instead of T::element_type to benefit from\n> > > std::remove_cv\n> > > - Fix ControlTypeNone test in ControlValue::toString()\n> > > - Separate array elements with \", \" in ControlValue::toString()\n> > > ---\n> > > include/libcamera/controls.h | 81 ++++++++++++---\n> > > src/libcamera/controls.cpp | 185 +++++++++++++++++++++++++++++------\n> > > 2 files changed, 225 insertions(+), 41 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/controls.h\n> b/include/libcamera/controls.h\n> > > index 4538be06af93..1e24ae30ab36 100644\n> > > --- a/include/libcamera/controls.h\n> > > +++ b/include/libcamera/controls.h\n> > > @@ -9,6 +9,7 @@\n> > > #define __LIBCAMERA_CONTROLS_H__\n> > >\n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > >\n> > > @@ -51,6 +52,10 @@ struct control_type {\n> > > static constexpr ControlType value = ControlTypeInteger64;\n> > > };\n> > >\n> > > +template\n> > > +struct control_type> : public\n> control_type> {\n> > > +};\n> > > +\n> > > } /* namespace details */\n> > >\n> > > class ControlValue\n> > > @@ -58,15 +63,35 @@ class ControlValue\n> > > public:\n> > > ControlValue();\n> > >\n> > > +#ifndef __DOXYGEN__\n> > > + template std::enable_if_t::value, std::nullptr_t> = nullptr>\n> > > + ControlValue(const T &value)\n> > > + : type_(ControlTypeNone), numElements_(0)\n> > > + {\n> > > + set(details::control_type>::value,\n> false,\n> > > + &value, 1, sizeof(T));\n> > > + }\n> > > +\n> > > + template std::enable_if_t::value, std::nullptr_t> = nullptr>\n> > > +#else\n> > > template\n> > > - ControlValue(T value)\n> > > - : type_(details::control_type>::value)\n> > > +#endif\n> >\n> > That #iffery is pretty horrible, removes one function and changes the\n> > template instantiation of this below function ?\n> >\n> > Though seeing the repeating pattern below too - I can't offer a better\n> > solution...\n> >\n> > > + ControlValue(const T &value)\n> > > + : type_(ControlTypeNone), numElements_(0)\n> > > {\n> > > - *reinterpret_cast(&bool_) = value;\n> > > + set(details::control_type>::value,\n> true>,\n> > > + value.data(), value.size(), sizeof(typename\n> T::value_type));\n> >\n> > What's true here ? This is not very friendly to read...\n>\n> Should I add\n>\n> enum {\n> ControlIsSingle = 1,\n> ControlIsArray = 1,\n> };\n>\n> ? I don't like the name ControlIsSingle though. Maybe this could be done\n> on top, if you think it's worth it ? If you can propose a better name\n> I'll submit a patch :-)\n>\n> > Ok - so on the plus side the bool_ is gone :-)\n> >\n> > > }\n> > >\n> > > + ~ControlValue();\n> > > +\n> > > + ControlValue(const ControlValue &other);\n> > > + ControlValue &operator=(const ControlValue &other);\n> > > +\n> > > ControlType type() const { return type_; }\n> > > bool isNone() const { return type_ == ControlTypeNone; }\n> > > + bool isArray() const { return isArray_; }\n> > > + std::size_t numElements() const { return numElements_; }\n> > > Span data() const;\n> > >\n> > > std::string toString() const;\n> > > @@ -77,31 +102,61 @@ public:\n> > > return !(*this == other);\n> > > }\n> > >\n> > > +#ifndef __DOXYGEN__\n> > > + template std::enable_if_t::value, std::nullptr_t> = nullptr>\n> > > + T get() const\n> > > + {\n> > > + assert(type_ ==\n> details::control_type>::value);\n> > > + assert(!isArray_);\n> > > +\n> > > + return *reinterpret_cast(data().data());\n> > > + }\n> > > +\n> > > + template std::enable_if_t::value, std::nullptr_t> = nullptr>\n> > > +#else\n> > > template\n> > > +#endif\n> > > T get() const\n> > > {\n> > > assert(type_ ==\n> details::control_type>::value);\n> > > + assert(isArray_);\n> > > +\n> > > + using V = typename T::value_type;\n> > > + const V *value = reinterpret_cast *>(data().data());\n> > > + return { value, numElements_ };\n> > > + }\n> > >\n> > > - return *reinterpret_cast(&bool_);\n> > > +#ifndef __DOXYGEN__\n> > > + template std::enable_if_t::value, std::nullptr_t> = nullptr>\n> > > + void set(const T &value)\n> > > + {\n> > > + set(details::control_type>::value,\n> false,\n> > > + reinterpret_cast(&value), 1, sizeof(T));\n> > > }\n> > >\n> > > + template std::enable_if_t::value, std::nullptr_t> = nullptr>\n> > > +#else\n> > > template\n> > > +#endif\n> > > void set(const T &value)\n> > > {\n> > > - type_ = details::control_type>::value;\n> > > - *reinterpret_cast(&bool_) = value;\n> > > + set(details::control_type>::value,\n> true,\n> > > + value.data(), value.size(), sizeof(typename\n> T::value_type));\n> > > }\n> > >\n> > > private:\n> > > - ControlType type_;\n> > > -\n> > > - union {\n> > > - bool bool_;\n> > > - int32_t integer32_;\n> > > - int64_t integer64_;\n> > > - };\n> > > + ControlType type_ : 8;\n> > > + bool isArray_ : 1;\n> > > + std::size_t numElements_ : 16;\n> > > + uint64_t storage_;\n> > > +\n> > > + void release();\n> > > + void set(ControlType type, bool isArray, const void *data,\n> > > + std::size_t numElements, std::size_t elementSize);\n> > > };\n> > >\n> > > +static_assert(sizeof(ControlValue) == 16, \"Invalid size of\n> ControlValue class\");\n> >\n> > Aha, I knew this ASSERT_ABI_SIZE would come up again :-)\n>\n> I think it's a good idea, and I think we should actually try to\n> automate that through all our classes, to have automatic ABI regression\n> tests. I envision that as being done through code analysis instead of\n> static_assert(). And I wouldn't be surprised if tools already existed\n> for this purpose.\n>\n> > > +\n> > > class ControlId\n> > > {\n> > > public:\n> > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > > index b2331ab7540d..a5a385aa1b0a 100644\n> > > --- a/src/libcamera/controls.cpp\n> > > +++ b/src/libcamera/controls.cpp\n> > > @@ -10,6 +10,7 @@\n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> > >\n> > > #include \"control_validator.h\"\n> > > #include \"log.h\"\n> > > @@ -50,7 +51,7 @@ LOG_DEFINE_CATEGORY(Controls)\n> > > namespace {\n> > >\n> > > static constexpr size_t ControlValueSize[] = {\n> > > - [ControlTypeNone] = 1,\n> > > + [ControlTypeNone] = 0,\n> > > [ControlTypeBool] = sizeof(bool),\n> > > [ControlTypeInteger32] = sizeof(int32_t),\n> > > [ControlTypeInteger64] = sizeof(int64_t),\n> > > @@ -80,15 +81,63 @@ static constexpr size_t ControlValueSize[] = {\n> > > * \\brief Construct an empty ControlValue.\n> > > */\n> > > ControlValue::ControlValue()\n> > > - : type_(ControlTypeNone)\n> > > + : type_(ControlTypeNone), isArray_(false), numElements_(0)\n> > > {\n> > > }\n> > >\n> > > /**\n> > > - * \\fn template T ControlValue::ControlValue(T value)\n> > > + * \\fn template T ControlValue::ControlValue(const T\n> &value)\n> > > * \\brief Construct a ControlValue of type T\n> > > * \\param[in] value Initial value\n> > > + *\n> > > + * This function constructs a new instance of ControlValue and stores\n> the \\a\n> > > + * value inside it. If the type \\a T is equivalent to Span, the\n> instance\n> > > + * stores an array of values of type \\a R. Otherwise the instance\n> stores a\n> > > + * single value of type \\a T. The numElements() and type() are\n> updated to\n> > > + * reflect the stored value.\n> > > + */\n> > > +\n> > > +void ControlValue::release()\n> > > +{\n> > > + std::size_t size = numElements_ * ControlValueSize[type_];\n> > > +\n> > > + if (size > sizeof storage_) {\n> >\n> > sizeof(storage) would read nicer to me here. ...\n>\n> sizeof is an operator and doesn't require parentheses:\n> https://en.cppreference.com/w/cpp/language/sizeof. I'll change it\n> though.\n>\n> > Oh ... uhm isn't storage a uint64_t? And therefore always 8?\n>\n> Correct, but that's better than hardcoding the value 8, right ? :-)\n>\n\nIndeed :-)\n\n\n> > > + delete[] *reinterpret_cast(&storage_);\n> > > + storage_ = 0;\n> > > + }\n> > > +}\n> > > +\n> > > +ControlValue::~ControlValue()\n> > > +{\n> > > + release();\n>\n\nWho deletes storage_ on destruct?\n\nRelease only appears to delete in the event that size is bigger than the\nstorage available to help the set() call?\n\nThat's making the reallocation below a bit ugly only to provide a function\nthat doesn't do anything to the destructor?\n\n(Doesn't do anything; based on assumption that after the object is used it\nhas an allocation which is of suitable size already)\n\n\n> > +}\n> > > +\n> > > +/**\n> > > + * \\brief Contruct a ControlValue with the content of \\a other\n> >\n> > /Contruct/Construct/\n> >\n> > > + * \\param[in] other The ControlValue to copy content from\n> > > + */\n> > > +ControlValue::ControlValue(const ControlValue &other)\n> > > + : type_(ControlTypeNone), numElements_(0)\n> > > +{\n> > > + *this = other;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Replace the content of the ControlValue with the one of \\a\n> other\n> > > + * \\param[in] other The ControlValue to copy content from\n> > > + *\n> > > + * Deep-copy the content of \\a other into the ControlValue by\n> reserving memory\n> > > + * and copy data there in case \\a other transports arrays of values\n> in one of\n> > > + * its pointer data members.\n> >\n> > That's hard to parse...\n>\n> Agreed. I'll drop the paragraph as I don't think it brings much.\n>\n> > > + *\n> > > + * \\return The ControlValue with its content replaced with the one of\n> \\a other\n> > > */\n> > > +ControlValue &ControlValue::operator=(const ControlValue &other)\n> > > +{\n> > > + set(other.type_, other.isArray_, other.data().data(),\n> > > + other.numElements_, ControlValueSize[other.type_]);\n> > > + return *this;\n> > > +}\n> >\n> > Do we need/desire move support to just move the allocated storage over\n> > at all ?\n>\n> I think we do, but I think it can be done on top.\n>\n\nSure\n\n\n> > >\n> > > /**\n> > > * \\fn ControlValue::type()\n> > > @@ -102,16 +151,33 @@ ControlValue::ControlValue()\n> > > * \\return True if the value type is ControlTypeNone, false otherwise\n> > > */\n> > >\n> > > +/**\n> > > + * \\fn ControlValue::isArray()\n> > > + * \\brief Determine if the value stores an array\n> > > + * \\return True if the value stores an array, false otherwise\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn ControlValue::numElements()\n> > > + * \\brief Retrieve the number of elements stored in the ControlValue\n> > > + *\n> > > + * For instances storing an array, this function returns the number\n> of elements\n> > > + * in the array. Otherwise, it returns 1.\n> > > + *\n> > > + * \\return The number of elements stored in the ControlValue\n> > > + */\n> > > +\n> > > /**\n> > > * \\brief Retrieve the raw of a control value\n> > > * \\return The raw data of the control value as a span of uint8_t\n> > > */\n> > > Span ControlValue::data() const\n> > > {\n> > > - return {\n> > > - reinterpret_cast(&bool_),\n> > > - ControlValueSize[type_]\n> > > - };\n> > > + std::size_t size = numElements_ * ControlValueSize[type_];\n> > > + const uint8_t *data = size > sizeof storage_\n> >\n> > sizeof without 'containing' what it parses really looks wrong to me :S\n>\n> I'll update this.\n>\n> > > + ? *reinterpret_cast *>(&storage_)\n> > > + : reinterpret_cast(&storage_);\n> > > + return { data, size };\n> >\n> > Ahh, at least that looks better than the multiline return statement we\n> > had before :-)\n> >\n> > > }\n> > >\n> > > /**\n> > > @@ -120,18 +186,43 @@ Span ControlValue::data() const\n> > > */\n> > > std::string ControlValue::toString() const\n> > > {\n> > > - switch (type_) {\n> > > - case ControlTypeNone:\n> > > - return \"\";\n> > > - case ControlTypeBool:\n> > > - return bool_ ? \"True\" : \"False\";\n> > > - case ControlTypeInteger32:\n> > > - return std::to_string(integer32_);\n> > > - case ControlTypeInteger64:\n> > > - return std::to_string(integer64_);\n> > > + if (type_ == ControlTypeNone)\n> > > + return \"\";\n> > > +\n> > > + const uint8_t *data = ControlValue::data().data();\n> > > + std::string str(isArray_ ? \"[ \" : \"\");\n> > > +\n> > > + for (unsigned int i = 0; i < numElements_; ++i) {\n> > > + switch (type_) {\n> > > + case ControlTypeBool: {\n> > > + const bool *value = reinterpret_cast *>(data);\n> > > + str += *value ? \"True\" : \"False\";\n> > > + break;\n> > > + }\n> > > + case ControlTypeInteger32: {\n> > > + const int32_t *value = reinterpret_cast int32_t *>(data);\n> > > + str += std::to_string(*value);\n> > > + break;\n> > > + }\n> > > + case ControlTypeInteger64: {\n> > > + const int64_t *value = reinterpret_cast int64_t *>(data);\n> > > + str += std::to_string(*value);\n> > > + break;\n> > > + }\n> > > + case ControlTypeNone:\n> > > + break;\n> > > + }\n> > > +\n> > > + if (i + 1 != numElements_)\n> > > + str += \", \";\n> > > +\n> > > + data += ControlValueSize[type_];\n> > > }\n> > >\n> > > - return \"\";\n> > > + if (isArray_)\n> > > + str += \" ]\";\n> > > +\n> > > + return str;\n> >\n> > Ohh toString() is neat here :-)\n> >\n> > > }\n> > >\n> > > /**\n> > > @@ -143,16 +234,13 @@ bool ControlValue::operator==(const ControlValue\n> &other) const\n> > > if (type_ != other.type_)\n> > > return false;\n> > >\n> > > - switch (type_) {\n> > > - case ControlTypeBool:\n> > > - return bool_ == other.bool_;\n> > > - case ControlTypeInteger32:\n> > > - return integer32_ == other.integer32_;\n> > > - case ControlTypeInteger64:\n> > > - return integer64_ == other.integer64_;\n> > > - default:\n> > > + if (numElements_ != other.numElements())\n> > > return false;\n> > > - }\n> > > +\n> > > + if (isArray_ != other.isArray_)\n> > > + return false;\n> > > +\n> > > + return memcmp(data().data(), other.data().data(), data().size())\n> == 0;\n>\n\nIs there some data involved in that code by any chance? :-)\n\n> > }\n> > >\n> > > /**\n> > > @@ -165,8 +253,16 @@ bool ControlValue::operator==(const ControlValue\n> &other) const\n> > > * \\fn template T ControlValue::get() const\n> > > * \\brief Get the control value\n> > > *\n> > > - * The control value type shall match the type T, otherwise the\n> behaviour is\n> > > - * undefined.\n> > > + * This function returns the contained value as an instance of \\a T.\n> If the\n> > > + * ControlValue instance stores a single value, the type \\a T shall\n> match the\n> > > + * stored value type(). If the instance stores an array of values,\n> the type\n> > > + * \\a T should be equal to Span, and the type \\a R shall\n> match the\n> > > + * stored value type(). The behaviour is undefined otherwise.\n> > > + *\n> > > + * Note that a ControlValue instance that stores a non-array value is\n> not\n> > > + * equivalent to an instance that stores an array value containing a\n> single\n> > > + * element. The latter shall be accessed through a Span\n> type, while\n> > > + * the former shall be accessed through a type \\a T corresponding to\n> type().\n> > > *\n> > > * \\return The control value\n> > > */\n> > > @@ -175,8 +271,41 @@ bool ControlValue::operator==(const ControlValue\n> &other) const\n> > > * \\fn template void ControlValue::set(const T &value)\n> > > * \\brief Set the control value to \\a value\n> > > * \\param[in] value The control value\n> > > + *\n> > > + * This function stores the \\a value in the instance. If the type \\a\n> T is\n> > > + * equivalent to Span, the instance stores an array of values of\n> type \\a R.\n> > > + * Otherwise the instance stores a single value of type \\a T. The\n> numElements()\n> > > + * and type() are updated to reflect the stored value.\n> > > + *\n> > > + * The entire content of \\a value is copied to the instance, no\n> reference to \\a\n> > > + * value or to the data it references is retained. This may be an\n> expensive\n> > > + * operation for Span<> values that refer to large arrays.\n> > > */\n> > >\n> > > +void ControlValue::set(ControlType type, bool isArray, const void\n> *data,\n> > > + std::size_t numElements, std::size_t elementSize)\n> > > +{\n> > > + ASSERT(elementSize == ControlValueSize[type]);\n> > > +\n> > > + release();\n>\n\nI hadn't noticed this here.\n\nWhat isn't clear here is that release is conditional on size > storage, so\ni think this would be clearer to perform the delete before new below.\n\n> > +\n> > > + type_ = type;\n> > > + numElements_ = numElements;\n> > > + isArray_ = isArray;\n> > > +\n> > > + std::size_t size = elementSize * numElements;\n> > > + void *storage;\n> > > +\n> > > + if (size > sizeof storage_) {\n>\n> I'll update this one too.\n>\n> > > + storage = reinterpret_cast(new char[size]);\n> > > + *reinterpret_cast(&storage_) = storage;\n> >\n> > Doesn't this need to delete/free any existing storage? Or does the\n> > assignment do that automagically?\n>\n> The release() call above handles it.\n>\n> > > + } else {\n> > > + storage = reinterpret_cast(&storage_);\n>\n\nBecause now you've highlighted the release call, this \"looks\" like it's\nsetting storage to a \"released\" allocation :-(\n\nAnd as far as i can tell with a small view on a phone, release() isn't\ngoing to do anything in the destructor ... Which is a leak...\n\nCould you verify that please and maybe simplify the code to make sure it's\nclear?\n\n> > + }\n> > > +\n> > > + memcpy(storage, data, size);\n> > > +}\n> > > +\n> > > /**\n> > > * \\class ControlId\n> > > * \\brief Control static metadata\n> > >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","Received":["from mail-lf1-x135.google.com (mail-lf1-x135.google.com\n\t[IPv6:2a00:1450:4864:20::135])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A003E60425\n\tfor ;\n\tSat, 7 Mar 2020 19:04:39 +0100 (CET)","by mail-lf1-x135.google.com with SMTP id b13so4374967lfb.12\n\tfor ;\n\tSat, 07 Mar 2020 10:04:39 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=bingham-xyz.20150623.gappssmtp.com; s=20150623;\n\th=mime-version:references:in-reply-to:reply-to:from:date:message-id\n\t:subject:to:cc;\n\tbh=BAZJjAnIMvG7nEG9LHUJ4cXApgxD/8h2VZG/LcD7Ba4=;\n\tb=V176CS5hJGf+ZsrMbZDjHyWD2ohbtz26oyvQi/Z0nedIGLC5RpgykuVEdYcb9OZ+v3\n\tZ179hoyUkj28t0ETRrY6iRRq/NHaSi0EZkKE7ER7f8inpyrrDTBC0KoXykb0RlSjA5BG\n\tk2BkREarRwo8fhz9apZ460LtBAG7Aqe6uG70Z28lCsnnlW18JfhlF9v5T1mREAgz47Sq\n\tSXgMykbUrov5JGQZpqPRXe6EZTwPsTtAhWTPfdd0KYtq8GTNj7575QybT5EPcr4wBZnE\n\t8EHMr4BH3Ry8Xrbvd21zTrrz2flQXUUN5z+fMch/chbsItfXqhDd5sZm1qLoagh7hXCB\n\tvMnA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:reply-to\n\t:from:date:message-id:subject:to:cc;\n\tbh=BAZJjAnIMvG7nEG9LHUJ4cXApgxD/8h2VZG/LcD7Ba4=;\n\tb=T88XHHw9cpd9w1LU9GqbG2Q+nfXejTd3fgoRZ4hID2K9hav+fZoTLhtF4atdyz2Yc8\n\tuuTQAeBjwjZ1fHckA1JJcl1LiQVzOeSZkvOxrSLlS8El6cF10kzO4MtBTQjOsSgLbsU8\n\to85kKoyLSuuXj0j7j9kUVb/tVA5MuLRIcuLgyDA8JOQNwo/a9aL/rR74b9TsGkT8YDXn\n\tj45Wm8v9PGFY9zr468+IKbHhQBebQcf03aHm+wjjrbLj4KqrO2ChejY7VYC1J9Gqe8tK\n\t9setT7HnA/P+43MYwGNS3LA3hCkl0NmwL7ud6Y85ePLAr4IgfKryqjdG16PzYAinL758\n\tKnyQ==","X-Gm-Message-State":"ANhLgQ2pnHi4oC6GSffdntt3TyBKVPn1c2m7F4jhHKY7/XQhox7J13uV\n\tFufPEuagUwkWpD29Crvq1aMaYrt0Xvu6jsQttgMKlA==","X-Google-Smtp-Source":"ADFU+vtBeAtn0FB1a9BDXwpc9Uwyh2CSJ3svECM0Jy+8Cn8IXpnQHqBnDrijS3lo9H9QKazRaFXguXM16u4mzuP/Jdo=","X-Received":"by 2002:ac2:531a:: with SMTP id\n\tc26mr4639167lfh.195.1583604278752; \n\tSat, 07 Mar 2020 10:04:38 -0800 (PST)","MIME-Version":"1.0","References":"<20200229164254.23604-17-laurent.pinchart@ideasonboard.com>\n\t<20200301175454.10921-1-laurent.pinchart@ideasonboard.com>\n\t<5b0e6fd3-7600-5910-118c-18d37b5f6872@ideasonboard.com>\n\t<20200306155556.GP4878@pendragon.ideasonboard.com>","In-Reply-To":"<20200306155556.GP4878@pendragon.ideasonboard.com>","Reply-To":"kieran@bingham.xyz","From":"Kieran Bingham ","Date":"Sat, 7 Mar 2020 18:04:23 +0000","Message-ID":"","To":"Laurent Pinchart ","Cc":"Kieran Bingham , \n\tlibcamera devel ","Content-Type":"multipart/alternative; boundary=\"00000000000080126705a0479a1b\"","Subject":"Re: [libcamera-devel] [PATCH v1.2 16/31] libcamera: controls:\n\tSupport array controls in ControlValue","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sat, 07 Mar 2020 18:04:39 -0000"}},{"id":3984,"web_url":"https://patchwork.libcamera.org/comment/3984/","msgid":"<20200307181731.GA5021@pendragon.ideasonboard.com>","date":"2020-03-07T18:17:31","subject":"Re: [libcamera-devel] [PATCH v1.2 16/31] libcamera: controls:\n\tSupport array controls in ControlValue","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Sat, Mar 07, 2020 at 06:04:23PM +0000, Kieran Bingham wrote:\n> On Fri, 6 Mar 2020, 15:56 Laurent Pinchart wrote:\n> > On Tue, Mar 03, 2020 at 12:23:10AM +0000, Kieran Bingham wrote:\n> > > On 01/03/2020 17:54, Laurent Pinchart wrote:\n> > > > From: Jacopo Mondi \n> > > >\n> > > > Add array controls support to the ControlValue class. The polymorphic\n> > > > class can now store more than a single element and supports access and\n> > > > creation through the use of Span<>.\n> > >\n> > > Oh, but if the creation is through a span, what stores the data in the\n> > > ControlValue ... I guess I'll see below... \n> > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > Signed-off-by: Laurent Pinchart \n> > >\n> > > Some comments, but nothing you can't handle...\n> > >\n> > > Reviewed-by: Kieran Bingham \n> > >\n> > > > ---\n> > > > Changes since v1:\n> > > >\n> > > > - Use T::value_type instead of T::element_type to benefit from\n> > > > std::remove_cv\n> > > > - Fix ControlTypeNone test in ControlValue::toString()\n> > > > - Separate array elements with \", \" in ControlValue::toString()\n> > > > ---\n> > > > include/libcamera/controls.h | 81 ++++++++++++---\n> > > > src/libcamera/controls.cpp | 185 +++++++++++++++++++++++++++++------\n> > > > 2 files changed, 225 insertions(+), 41 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > > > index 4538be06af93..1e24ae30ab36 100644\n> > > > --- a/include/libcamera/controls.h\n> > > > +++ b/include/libcamera/controls.h\n> > > > @@ -9,6 +9,7 @@\n> > > > #define __LIBCAMERA_CONTROLS_H__\n> > > >\n> > > > #include \n> > > > +#include \n> > > > #include \n> > > > #include \n> > > >\n> > > > @@ -51,6 +52,10 @@ struct control_type {\n> > > > static constexpr ControlType value = ControlTypeInteger64;\n> > > > };\n> > > >\n> > > > +template\n> > > > +struct control_type> : public control_type> {\n> > > > +};\n> > > > +\n> > > > } /* namespace details */\n> > > >\n> > > > class ControlValue\n> > > > @@ -58,15 +63,35 @@ class ControlValue\n> > > > public:\n> > > > ControlValue();\n> > > >\n> > > > +#ifndef __DOXYGEN__\n> > > > + template::value, std::nullptr_t> = nullptr>\n> > > > + ControlValue(const T &value)\n> > > > + : type_(ControlTypeNone), numElements_(0)\n> > > > + {\n> > > > + set(details::control_type>::value, false,\n> > > > + &value, 1, sizeof(T));\n> > > > + }\n> > > > +\n> > > > + template::value, std::nullptr_t> = nullptr>\n> > > > +#else\n> > > > template\n> > > > - ControlValue(T value)\n> > > > - : type_(details::control_type>::value)\n> > > > +#endif\n> > >\n> > > That #iffery is pretty horrible, removes one function and changes the\n> > > template instantiation of this below function ?\n> > >\n> > > Though seeing the repeating pattern below too - I can't offer a better\n> > > solution...\n> > >\n> > > > + ControlValue(const T &value)\n> > > > + : type_(ControlTypeNone), numElements_(0)\n> > > > {\n> > > > - *reinterpret_cast(&bool_) = value;\n> > > > + set(details::control_type>::value, true>,\n> > > > + value.data(), value.size(), sizeof(typename T::value_type));\n> > >\n> > > What's true here ? This is not very friendly to read...\n> >\n> > Should I add\n> >\n> > enum {\n> > ControlIsSingle = 1,\n> > ControlIsArray = 1,\n> > };\n> >\n> > ? I don't like the name ControlIsSingle though. Maybe this could be done\n> > on top, if you think it's worth it ? If you can propose a better name\n> > I'll submit a patch :-)\n> >\n> > > Ok - so on the plus side the bool_ is gone :-)\n> > >\n> > > > }\n> > > >\n> > > > + ~ControlValue();\n> > > > +\n> > > > + ControlValue(const ControlValue &other);\n> > > > + ControlValue &operator=(const ControlValue &other);\n> > > > +\n> > > > ControlType type() const { return type_; }\n> > > > bool isNone() const { return type_ == ControlTypeNone; }\n> > > > + bool isArray() const { return isArray_; }\n> > > > + std::size_t numElements() const { return numElements_; }\n> > > > Span data() const;\n> > > >\n> > > > std::string toString() const;\n> > > > @@ -77,31 +102,61 @@ public:\n> > > > return !(*this == other);\n> > > > }\n> > > >\n> > > > +#ifndef __DOXYGEN__\n> > > > + template::value, std::nullptr_t> = nullptr>\n> > > > + T get() const\n> > > > + {\n> > > > + assert(type_ == details::control_type>::value);\n> > > > + assert(!isArray_);\n> > > > +\n> > > > + return *reinterpret_cast(data().data());\n> > > > + }\n> > > > +\n> > > > + template::value, std::nullptr_t> = nullptr>\n> > > > +#else\n> > > > template\n> > > > +#endif\n> > > > T get() const\n> > > > {\n> > > > assert(type_ == details::control_type>::value);\n> > > > + assert(isArray_);\n> > > > +\n> > > > + using V = typename T::value_type;\n> > > > + const V *value = reinterpret_cast(data().data());\n> > > > + return { value, numElements_ };\n> > > > + }\n> > > >\n> > > > - return *reinterpret_cast(&bool_);\n> > > > +#ifndef __DOXYGEN__\n> > > > + template::value, std::nullptr_t> = nullptr>\n> > > > + void set(const T &value)\n> > > > + {\n> > > > + set(details::control_type>::value, false,\n> > > > + reinterpret_cast(&value), 1, sizeof(T));\n> > > > }\n> > > >\n> > > > + template::value, std::nullptr_t> = nullptr>\n> > > > +#else\n> > > > template\n> > > > +#endif\n> > > > void set(const T &value)\n> > > > {\n> > > > - type_ = details::control_type>::value;\n> > > > - *reinterpret_cast(&bool_) = value;\n> > > > + set(details::control_type>::value, true,\n> > > > + value.data(), value.size(), sizeof(typename T::value_type));\n> > > > }\n> > > >\n> > > > private:\n> > > > - ControlType type_;\n> > > > -\n> > > > - union {\n> > > > - bool bool_;\n> > > > - int32_t integer32_;\n> > > > - int64_t integer64_;\n> > > > - };\n> > > > + ControlType type_ : 8;\n> > > > + bool isArray_ : 1;\n> > > > + std::size_t numElements_ : 16;\n> > > > + uint64_t storage_;\n> > > > +\n> > > > + void release();\n> > > > + void set(ControlType type, bool isArray, const void *data,\n> > > > + std::size_t numElements, std::size_t elementSize);\n> > > > };\n> > > >\n> > > > +static_assert(sizeof(ControlValue) == 16, \"Invalid size of ControlValue class\");\n> > >\n> > > Aha, I knew this ASSERT_ABI_SIZE would come up again :-)\n> >\n> > I think it's a good idea, and I think we should actually try to\n> > automate that through all our classes, to have automatic ABI regression\n> > tests. I envision that as being done through code analysis instead of\n> > static_assert(). And I wouldn't be surprised if tools already existed\n> > for this purpose.\n> >\n> > > > +\n> > > > class ControlId\n> > > > {\n> > > > public:\n> > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > > > index b2331ab7540d..a5a385aa1b0a 100644\n> > > > --- a/src/libcamera/controls.cpp\n> > > > +++ b/src/libcamera/controls.cpp\n> > > > @@ -10,6 +10,7 @@\n> > > > #include \n> > > > #include \n> > > > #include \n> > > > +#include \n> > > >\n> > > > #include \"control_validator.h\"\n> > > > #include \"log.h\"\n> > > > @@ -50,7 +51,7 @@ LOG_DEFINE_CATEGORY(Controls)\n> > > > namespace {\n> > > >\n> > > > static constexpr size_t ControlValueSize[] = {\n> > > > - [ControlTypeNone] = 1,\n> > > > + [ControlTypeNone] = 0,\n> > > > [ControlTypeBool] = sizeof(bool),\n> > > > [ControlTypeInteger32] = sizeof(int32_t),\n> > > > [ControlTypeInteger64] = sizeof(int64_t),\n> > > > @@ -80,15 +81,63 @@ static constexpr size_t ControlValueSize[] = {\n> > > > * \\brief Construct an empty ControlValue.\n> > > > */\n> > > > ControlValue::ControlValue()\n> > > > - : type_(ControlTypeNone)\n> > > > + : type_(ControlTypeNone), isArray_(false), numElements_(0)\n> > > > {\n> > > > }\n> > > >\n> > > > /**\n> > > > - * \\fn template T ControlValue::ControlValue(T value)\n> > > > + * \\fn template T ControlValue::ControlValue(const T &value)\n> > > > * \\brief Construct a ControlValue of type T\n> > > > * \\param[in] value Initial value\n> > > > + *\n> > > > + * This function constructs a new instance of ControlValue and stores the \\a\n> > > > + * value inside it. If the type \\a T is equivalent to Span, the instance\n> > > > + * stores an array of values of type \\a R. Otherwise the instance stores a\n> > > > + * single value of type \\a T. The numElements() and type() are updated to\n> > > > + * reflect the stored value.\n> > > > + */\n> > > > +\n> > > > +void ControlValue::release()\n> > > > +{\n> > > > + std::size_t size = numElements_ * ControlValueSize[type_];\n> > > > +\n> > > > + if (size > sizeof storage_) {\n> > >\n> > > sizeof(storage) would read nicer to me here. ...\n> >\n> > sizeof is an operator and doesn't require parentheses:\n> > https://en.cppreference.com/w/cpp/language/sizeof. I'll change it\n> > though.\n> >\n> > > Oh ... uhm isn't storage a uint64_t? And therefore always 8?\n> >\n> > Correct, but that's better than hardcoding the value 8, right ? :-)\n> \n> Indeed :-)\n> \n> > > > + delete[] *reinterpret_cast(&storage_);\n> > > > + storage_ = 0;\n> > > > + }\n> > > > +}\n> > > > +\n> > > > +ControlValue::~ControlValue()\n> > > > +{\n> > > > + release();\n> \n> Who deletes storage_ on destruct?\n\nrelease() does.\n\n> Release only appears to delete in the event that size is bigger than the\n> storage available to help the set() call?\n\nrelease() deletes the storage if it has been allocated. We allocate\nexternal storage in case the data won't fit in the internal storage\n(size > 8, see the set() function below), and delete it in release()\nusing the same condition.\n\n> That's making the reallocation below a bit ugly only to provide a function\n> that doesn't do anything to the destructor?\n> \n> (Doesn't do anything; based on assumption that after the object is used it\n> has an allocation which is of suitable size already)\n> \n> > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Contruct a ControlValue with the content of \\a other\n> > >\n> > > /Contruct/Construct/\n> > >\n> > > > + * \\param[in] other The ControlValue to copy content from\n> > > > + */\n> > > > +ControlValue::ControlValue(const ControlValue &other)\n> > > > + : type_(ControlTypeNone), numElements_(0)\n> > > > +{\n> > > > + *this = other;\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Replace the content of the ControlValue with the one of \\a other\n> > > > + * \\param[in] other The ControlValue to copy content from\n> > > > + *\n> > > > + * Deep-copy the content of \\a other into the ControlValue by reserving memory\n> > > > + * and copy data there in case \\a other transports arrays of values in one of\n> > > > + * its pointer data members.\n> > >\n> > > That's hard to parse...\n> >\n> > Agreed. I'll drop the paragraph as I don't think it brings much.\n> >\n> > > > + *\n> > > > + * \\return The ControlValue with its content replaced with the one of \\a other\n> > > > */\n> > > > +ControlValue &ControlValue::operator=(const ControlValue &other)\n> > > > +{\n> > > > + set(other.type_, other.isArray_, other.data().data(),\n> > > > + other.numElements_, ControlValueSize[other.type_]);\n> > > > + return *this;\n> > > > +}\n> > >\n> > > Do we need/desire move support to just move the allocated storage over\n> > > at all ?\n> >\n> > I think we do, but I think it can be done on top.\n> \n> Sure\n> \n> > > >\n> > > > /**\n> > > > * \\fn ControlValue::type()\n> > > > @@ -102,16 +151,33 @@ ControlValue::ControlValue()\n> > > > * \\return True if the value type is ControlTypeNone, false otherwise\n> > > > */\n> > > >\n> > > > +/**\n> > > > + * \\fn ControlValue::isArray()\n> > > > + * \\brief Determine if the value stores an array\n> > > > + * \\return True if the value stores an array, false otherwise\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn ControlValue::numElements()\n> > > > + * \\brief Retrieve the number of elements stored in the ControlValue\n> > > > + *\n> > > > + * For instances storing an array, this function returns the number of elements\n> > > > + * in the array. Otherwise, it returns 1.\n> > > > + *\n> > > > + * \\return The number of elements stored in the ControlValue\n> > > > + */\n> > > > +\n> > > > /**\n> > > > * \\brief Retrieve the raw of a control value\n> > > > * \\return The raw data of the control value as a span of uint8_t\n> > > > */\n> > > > Span ControlValue::data() const\n> > > > {\n> > > > - return {\n> > > > - reinterpret_cast(&bool_),\n> > > > - ControlValueSize[type_]\n> > > > - };\n> > > > + std::size_t size = numElements_ * ControlValueSize[type_];\n> > > > + const uint8_t *data = size > sizeof storage_\n> > >\n> > > sizeof without 'containing' what it parses really looks wrong to me :S\n> >\n> > I'll update this.\n> >\n> > > > + ? *reinterpret_cast(&storage_)\n> > > > + : reinterpret_cast(&storage_);\n> > > > + return { data, size };\n> > >\n> > > Ahh, at least that looks better than the multiline return statement we\n> > > had before :-)\n> > >\n> > > > }\n> > > >\n> > > > /**\n> > > > @@ -120,18 +186,43 @@ Span ControlValue::data() const\n> > > > */\n> > > > std::string ControlValue::toString() const\n> > > > {\n> > > > - switch (type_) {\n> > > > - case ControlTypeNone:\n> > > > - return \"\";\n> > > > - case ControlTypeBool:\n> > > > - return bool_ ? \"True\" : \"False\";\n> > > > - case ControlTypeInteger32:\n> > > > - return std::to_string(integer32_);\n> > > > - case ControlTypeInteger64:\n> > > > - return std::to_string(integer64_);\n> > > > + if (type_ == ControlTypeNone)\n> > > > + return \"\";\n> > > > +\n> > > > + const uint8_t *data = ControlValue::data().data();\n> > > > + std::string str(isArray_ ? \"[ \" : \"\");\n> > > > +\n> > > > + for (unsigned int i = 0; i < numElements_; ++i) {\n> > > > + switch (type_) {\n> > > > + case ControlTypeBool: {\n> > > > + const bool *value = reinterpret_cast(data);\n> > > > + str += *value ? \"True\" : \"False\";\n> > > > + break;\n> > > > + }\n> > > > + case ControlTypeInteger32: {\n> > > > + const int32_t *value = reinterpret_cast(data);\n> > > > + str += std::to_string(*value);\n> > > > + break;\n> > > > + }\n> > > > + case ControlTypeInteger64: {\n> > > > + const int64_t *value = reinterpret_cast(data);\n> > > > + str += std::to_string(*value);\n> > > > + break;\n> > > > + }\n> > > > + case ControlTypeNone:\n> > > > + break;\n> > > > + }\n> > > > +\n> > > > + if (i + 1 != numElements_)\n> > > > + str += \", \";\n> > > > +\n> > > > + data += ControlValueSize[type_];\n> > > > }\n> > > >\n> > > > - return \"\";\n> > > > + if (isArray_)\n> > > > + str += \" ]\";\n> > > > +\n> > > > + return str;\n> > >\n> > > Ohh toString() is neat here :-)\n> > >\n> > > > }\n> > > >\n> > > > /**\n> > > > @@ -143,16 +234,13 @@ bool ControlValue::operator==(const ControlValue &other) const\n> > > > if (type_ != other.type_)\n> > > > return false;\n> > > >\n> > > > - switch (type_) {\n> > > > - case ControlTypeBool:\n> > > > - return bool_ == other.bool_;\n> > > > - case ControlTypeInteger32:\n> > > > - return integer32_ == other.integer32_;\n> > > > - case ControlTypeInteger64:\n> > > > - return integer64_ == other.integer64_;\n> > > > - default:\n> > > > + if (numElements_ != other.numElements())\n> > > > return false;\n> > > > - }\n> > > > +\n> > > > + if (isArray_ != other.isArray_)\n> > > > + return false;\n> > > > +\n> > > > + return memcmp(data().data(), other.data().data(), data().size()) == 0;\n> \n> Is there some data involved in that code by any chance? :-)\n\nSo you've spotted the subliminal message ;-)\n\n> > > }\n> > > >\n> > > > /**\n> > > > @@ -165,8 +253,16 @@ bool ControlValue::operator==(const ControlValue\n> > &other) const\n> > > > * \\fn template T ControlValue::get() const\n> > > > * \\brief Get the control value\n> > > > *\n> > > > - * The control value type shall match the type T, otherwise the behaviour is\n> > > > - * undefined.\n> > > > + * This function returns the contained value as an instance of \\a T. If the\n> > > > + * ControlValue instance stores a single value, the type \\a T shall match the\n> > > > + * stored value type(). If the instance stores an array of values, the type\n> > > > + * \\a T should be equal to Span, and the type \\a R shall match the\n> > > > + * stored value type(). The behaviour is undefined otherwise.\n> > > > + *\n> > > > + * Note that a ControlValue instance that stores a non-array value is not\n> > > > + * equivalent to an instance that stores an array value containing a single\n> > > > + * element. The latter shall be accessed through a Span type, while\n> > > > + * the former shall be accessed through a type \\a T corresponding to type().\n> > > > *\n> > > > * \\return The control value\n> > > > */\n> > > > @@ -175,8 +271,41 @@ bool ControlValue::operator==(const ControlValue &other) const\n> > > > * \\fn template void ControlValue::set(const T &value)\n> > > > * \\brief Set the control value to \\a value\n> > > > * \\param[in] value The control value\n> > > > + *\n> > > > + * This function stores the \\a value in the instance. If the type \\a T is\n> > > > + * equivalent to Span, the instance stores an array of values of type \\a R.\n> > > > + * Otherwise the instance stores a single value of type \\a T. The numElements()\n> > > > + * and type() are updated to reflect the stored value.\n> > > > + *\n> > > > + * The entire content of \\a value is copied to the instance, no reference to \\a\n> > > > + * value or to the data it references is retained. This may be an expensive\n> > > > + * operation for Span<> values that refer to large arrays.\n> > > > */\n> > > >\n> > > > +void ControlValue::set(ControlType type, bool isArray, const void *data,\n> > > > + std::size_t numElements, std::size_t elementSize)\n> > > > +{\n> > > > + ASSERT(elementSize == ControlValueSize[type]);\n> > > > +\n> > > > + release();\n> \n> I hadn't noticed this here.\n> \n> What isn't clear here is that release is conditional on size > storage, so\n> i think this would be clearer to perform the delete before new below.\n\nThe purpose of release() is to free storage if it has been allocated, so\nthat the callers don't need to care. We need to free storage here if it\nhas been allocated regardless of the new size (and thus regardless of\nthis set() call will end up allocating memory or using internal\nstorage).\n\n> > > +\n> > > > + type_ = type;\n> > > > + numElements_ = numElements;\n> > > > + isArray_ = isArray;\n> > > > +\n> > > > + std::size_t size = elementSize * numElements;\n> > > > + void *storage;\n> > > > +\n> > > > + if (size > sizeof storage_) {\n> >\n> > I'll update this one too.\n> >\n> > > > + storage = reinterpret_cast(new char[size]);\n> > > > + *reinterpret_cast(&storage_) = storage;\n> > >\n> > > Doesn't this need to delete/free any existing storage? Or does the\n> > > assignment do that automagically?\n> >\n> > The release() call above handles it.\n> >\n> > > > + } else {\n> > > > + storage = reinterpret_cast(&storage_);\n> \n> Because now you've highlighted the release call, this \"looks\" like it's\n> setting storage to a \"released\" allocation :-(\n> \n> And as far as i can tell with a small view on a phone, release() isn't\n> going to do anything in the destructor ... Which is a leak...\n\nIn this branch we use the internal storage, so release() doesn't need to\nfree it. In the above branch we allocate external storage, which will be\nfreed by release() in the destructor (or when this function is called\nagain).\n\n> Could you verify that please and maybe simplify the code to make sure it's\n> clear?\n> \n> > > + }\n> > > > +\n> > > > + memcpy(storage, data, size);\n> > > > +}\n> > > > +\n> > > > /**\n> > > > * \\class ControlId\n> > > > * \\brief Control static metadata","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 361BC60425\n\tfor ;\n\tSat, 7 Mar 2020 19:17:36 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8EE215F;\n\tSat, 7 Mar 2020 19:17:35 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1583605055;\n\tbh=W1w/9I2DHC19iU9j+7RiJR0T40TRyo6O6l8UKXvwQtQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cOUBqJVBrQUiRrsndhAK9c/Sri6W55oeCJj5CtYmmU1JeqB3SypsBf1GLkrwJ/AEy\n\tR/mSSbae2hMdzM8QAMmBqnb+69xUKjzhsW+4tuy/gZ5zfOivUfENuNqhLa9bBhPqD4\n\tGeKOePDjFUr3xyLbmmMuEJGUCFYNxtDwAhwffTHA=","Date":"Sat, 7 Mar 2020 20:17:31 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Kieran Bingham ,\n\tlibcamera devel ","Message-ID":"<20200307181731.GA5021@pendragon.ideasonboard.com>","References":"<20200229164254.23604-17-laurent.pinchart@ideasonboard.com>\n\t<20200301175454.10921-1-laurent.pinchart@ideasonboard.com>\n\t<5b0e6fd3-7600-5910-118c-18d37b5f6872@ideasonboard.com>\n\t<20200306155556.GP4878@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v1.2 16/31] libcamera: controls:\n\tSupport array controls in ControlValue","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sat, 07 Mar 2020 18:17:36 -0000"}},{"id":3990,"web_url":"https://patchwork.libcamera.org/comment/3990/","msgid":"<40a7fcf2-33aa-098c-15d2-ff0798400302@bingham.xyz>","date":"2020-03-07T22:07:37","subject":"Re: [libcamera-devel] [PATCH v1.2 16/31] libcamera: controls:\n\tSupport array controls in ControlValue","submitter":{"id":38,"url":"https://patchwork.libcamera.org/api/people/38/","name":"Kieran Bingham","email":"kieran@bingham.xyz"},"content":"Hi Laurent\n\nOn 07/03/2020 18:17, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Sat, Mar 07, 2020 at 06:04:23PM +0000, Kieran Bingham wrote:\n>> On Fri, 6 Mar 2020, 15:56 Laurent Pinchart wrote:\n>>> On Tue, Mar 03, 2020 at 12:23:10AM +0000, Kieran Bingham wrote:\n>>>> On 01/03/2020 17:54, Laurent Pinchart wrote:\n>>>>> From: Jacopo Mondi \n>>>>>\n>>>>> Add array controls support to the ControlValue class. The polymorphic\n>>>>> class can now store more than a single element and supports access and\n>>>>> creation through the use of Span<>.\n>>>>\n>>>> Oh, but if the creation is through a span, what stores the data in the\n>>>> ControlValue ... I guess I'll see below... \n>>>>\n>>>>> Signed-off-by: Jacopo Mondi \n>>>>> Signed-off-by: Laurent Pinchart \n>>>>\n>>>> Some comments, but nothing you can't handle...\n>>>>\n\nContinued ramblings below are unfortunately out-of-order due to replying\nin situ on existing comments but the general message is:\n\nOk - so no leak like I feared, and except the fact that I misinterpreted\nthe definition/usage of storage_ which might need some comment to make\nit clearer (or might not because I might just be overtired), the\nfollowing still stands ... and I don't think there's any blocker on this\nseries :-)\n\n>>>> Reviewed-by: Kieran Bingham \n\n\n\n>>>>\n>>>>> ---\n>>>>> Changes since v1:\n>>>>>\n>>>>> - Use T::value_type instead of T::element_type to benefit from\n>>>>> std::remove_cv\n>>>>> - Fix ControlTypeNone test in ControlValue::toString()\n>>>>> - Separate array elements with \", \" in ControlValue::toString()\n>>>>> ---\n>>>>> include/libcamera/controls.h | 81 ++++++++++++---\n>>>>> src/libcamera/controls.cpp | 185 +++++++++++++++++++++++++++++------\n>>>>> 2 files changed, 225 insertions(+), 41 deletions(-)\n>>>>>\n>>>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n>>>>> index 4538be06af93..1e24ae30ab36 100644\n>>>>> --- a/include/libcamera/controls.h\n>>>>> +++ b/include/libcamera/controls.h\n>>>>> @@ -9,6 +9,7 @@\n>>>>> #define __LIBCAMERA_CONTROLS_H__\n>>>>>\n>>>>> #include \n>>>>> +#include \n>>>>> #include \n>>>>> #include \n>>>>>\n>>>>> @@ -51,6 +52,10 @@ struct control_type {\n>>>>> static constexpr ControlType value = ControlTypeInteger64;\n>>>>> };\n>>>>>\n>>>>> +template\n>>>>> +struct control_type> : public control_type> {\n>>>>> +};\n>>>>> +\n>>>>> } /* namespace details */\n>>>>>\n>>>>> class ControlValue\n>>>>> @@ -58,15 +63,35 @@ class ControlValue\n>>>>> public:\n>>>>> ControlValue();\n>>>>>\n>>>>> +#ifndef __DOXYGEN__\n>>>>> + template::value, std::nullptr_t> = nullptr>\n>>>>> + ControlValue(const T &value)\n>>>>> + : type_(ControlTypeNone), numElements_(0)\n>>>>> + {\n>>>>> + set(details::control_type>::value, false,\n>>>>> + &value, 1, sizeof(T));\n>>>>> + }\n>>>>> +\n>>>>> + template::value, std::nullptr_t> = nullptr>\n>>>>> +#else\n>>>>> template\n>>>>> - ControlValue(T value)\n>>>>> - : type_(details::control_type>::value)\n>>>>> +#endif\n>>>>\n>>>> That #iffery is pretty horrible, removes one function and changes the\n>>>> template instantiation of this below function ?\n>>>>\n>>>> Though seeing the repeating pattern below too - I can't offer a better\n>>>> solution...\n>>>>\n>>>>> + ControlValue(const T &value)\n>>>>> + : type_(ControlTypeNone), numElements_(0)\n>>>>> {\n>>>>> - *reinterpret_cast(&bool_) = value;\n>>>>> + set(details::control_type>::value, true>,\n>>>>> + value.data(), value.size(), sizeof(typename T::value_type));\n>>>>\n>>>> What's true here ? This is not very friendly to read...\n>>>\n>>> Should I add\n>>>\n>>> enum {\n>>> ControlIsSingle = 1,\n>>> ControlIsArray = 1,\n>>> };\n>>>\n>>> ? I don't like the name ControlIsSingle though. Maybe this could be done\n>>> on top, if you think it's worth it ? If you can propose a better name\n>>> I'll submit a patch :-)\n>>>\n>>>> Ok - so on the plus side the bool_ is gone :-)\n>>>>\n>>>>> }\n>>>>>\n>>>>> + ~ControlValue();\n>>>>> +\n>>>>> + ControlValue(const ControlValue &other);\n>>>>> + ControlValue &operator=(const ControlValue &other);\n>>>>> +\n>>>>> ControlType type() const { return type_; }\n>>>>> bool isNone() const { return type_ == ControlTypeNone; }\n>>>>> + bool isArray() const { return isArray_; }\n>>>>> + std::size_t numElements() const { return numElements_; }\n>>>>> Span data() const;\n>>>>>\n>>>>> std::string toString() const;\n>>>>> @@ -77,31 +102,61 @@ public:\n>>>>> return !(*this == other);\n>>>>> }\n>>>>>\n>>>>> +#ifndef __DOXYGEN__\n>>>>> + template::value, std::nullptr_t> = nullptr>\n>>>>> + T get() const\n>>>>> + {\n>>>>> + assert(type_ == details::control_type>::value);\n>>>>> + assert(!isArray_);\n>>>>> +\n>>>>> + return *reinterpret_cast(data().data());\n>>>>> + }\n>>>>> +\n>>>>> + template::value, std::nullptr_t> = nullptr>\n>>>>> +#else\n>>>>> template\n>>>>> +#endif\n>>>>> T get() const\n>>>>> {\n>>>>> assert(type_ == details::control_type>::value);\n>>>>> + assert(isArray_);\n>>>>> +\n>>>>> + using V = typename T::value_type;\n>>>>> + const V *value = reinterpret_cast(data().data());\n>>>>> + return { value, numElements_ };\n>>>>> + }\n>>>>>\n>>>>> - return *reinterpret_cast(&bool_);\n>>>>> +#ifndef __DOXYGEN__\n>>>>> + template::value, std::nullptr_t> = nullptr>\n>>>>> + void set(const T &value)\n>>>>> + {\n>>>>> + set(details::control_type>::value, false,\n>>>>> + reinterpret_cast(&value), 1, sizeof(T));\n>>>>> }\n>>>>>\n>>>>> + template::value, std::nullptr_t> = nullptr>\n>>>>> +#else\n>>>>> template\n>>>>> +#endif\n>>>>> void set(const T &value)\n>>>>> {\n>>>>> - type_ = details::control_type>::value;\n>>>>> - *reinterpret_cast(&bool_) = value;\n>>>>> + set(details::control_type>::value, true,\n>>>>> + value.data(), value.size(), sizeof(typename T::value_type));\n>>>>> }\n>>>>>\n>>>>> private:\n>>>>> - ControlType type_;\n>>>>> -\n>>>>> - union {\n>>>>> - bool bool_;\n>>>>> - int32_t integer32_;\n>>>>> - int64_t integer64_;\n>>>>> - };\n>>>>> + ControlType type_ : 8;\n>>>>> + bool isArray_ : 1;\n>>>>> + std::size_t numElements_ : 16;\n>>>>> + uint64_t storage_;\n>>>>> +\n>>>>> + void release();\n>>>>> + void set(ControlType type, bool isArray, const void *data,\n>>>>> + std::size_t numElements, std::size_t elementSize);\n>>>>> };\n>>>>>\n>>>>> +static_assert(sizeof(ControlValue) == 16, \"Invalid size of ControlValue class\");\n>>>>\n>>>> Aha, I knew this ASSERT_ABI_SIZE would come up again :-)\n>>>\n>>> I think it's a good idea, and I think we should actually try to\n>>> automate that through all our classes, to have automatic ABI regression\n>>> tests. I envision that as being done through code analysis instead of\n>>> static_assert(). And I wouldn't be surprised if tools already existed\n>>> for this purpose.\n>>>\n>>>>> +\n>>>>> class ControlId\n>>>>> {\n>>>>> public:\n>>>>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n>>>>> index b2331ab7540d..a5a385aa1b0a 100644\n>>>>> --- a/src/libcamera/controls.cpp\n>>>>> +++ b/src/libcamera/controls.cpp\n>>>>> @@ -10,6 +10,7 @@\n>>>>> #include \n>>>>> #include \n>>>>> #include \n>>>>> +#include \n>>>>>\n>>>>> #include \"control_validator.h\"\n>>>>> #include \"log.h\"\n>>>>> @@ -50,7 +51,7 @@ LOG_DEFINE_CATEGORY(Controls)\n>>>>> namespace {\n>>>>>\n>>>>> static constexpr size_t ControlValueSize[] = {\n>>>>> - [ControlTypeNone] = 1,\n>>>>> + [ControlTypeNone] = 0,\n>>>>> [ControlTypeBool] = sizeof(bool),\n>>>>> [ControlTypeInteger32] = sizeof(int32_t),\n>>>>> [ControlTypeInteger64] = sizeof(int64_t),\n>>>>> @@ -80,15 +81,63 @@ static constexpr size_t ControlValueSize[] = {\n>>>>> * \\brief Construct an empty ControlValue.\n>>>>> */\n>>>>> ControlValue::ControlValue()\n>>>>> - : type_(ControlTypeNone)\n>>>>> + : type_(ControlTypeNone), isArray_(false), numElements_(0)\n>>>>> {\n>>>>> }\n>>>>>\n>>>>> /**\n>>>>> - * \\fn template T ControlValue::ControlValue(T value)\n>>>>> + * \\fn template T ControlValue::ControlValue(const T &value)\n>>>>> * \\brief Construct a ControlValue of type T\n>>>>> * \\param[in] value Initial value\n>>>>> + *\n>>>>> + * This function constructs a new instance of ControlValue and stores the \\a\n>>>>> + * value inside it. If the type \\a T is equivalent to Span, the instance\n>>>>> + * stores an array of values of type \\a R. Otherwise the instance stores a\n>>>>> + * single value of type \\a T. The numElements() and type() are updated to\n>>>>> + * reflect the stored value.\n>>>>> + */\n>>>>> +\n>>>>> +void ControlValue::release()\n>>>>> +{\n>>>>> + std::size_t size = numElements_ * ControlValueSize[type_];\n>>>>> +\n>>>>> + if (size > sizeof storage_) {\n>>>>\n>>>> sizeof(storage) would read nicer to me here. ...\n>>>\n>>> sizeof is an operator and doesn't require parentheses:\n>>> https://en.cppreference.com/w/cpp/language/sizeof. I'll change it\n>>> though.\n>>>\n>>>> Oh ... uhm isn't storage a uint64_t? And therefore always 8?\n\nAh, my (incorrect) inference here was that it was supposed to not always\nbe 8...\n\n>>>\n>>> Correct, but that's better than hardcoding the value 8, right ? :-)\n>>\n>> Indeed :-)\n\n\nAnd now I read that again it clears up my earlier confusion. I had seen\nthat storage_ was being used as a pointer, and thus was expecting the\nsizeofs to be determining the size of the available memory to store.\n\nIt's the fact that this > sizeof(storage_) determines if the storage_ is\nused as those bytes directly or as a pointer to an allocation.\n\nIt seems that's easy to miss - and can cause confusion. Have I missed\nthe documentation or comments that clears that up? If not - could\nsomething be added please?\n\n\n\n>>\n>>>>> + delete[] *reinterpret_cast(&storage_);\n>>>>> + storage_ = 0;\n>>>>> + }\n>>>>> +}\n>>>>> +\n>>>>> +ControlValue::~ControlValue()\n>>>>> +{\n>>>>> + release();\n>>\n>> Who deletes storage_ on destruct?\n> \n> release() does.\n> \n>> Release only appears to delete in the event that size is bigger than the\n>> storage available to help the set() call?\n> \n> release() deletes the storage if it has been allocated. We allocate\n> external storage in case the data won't fit in the internal storage\n\nAhhhhhhhh (that's both an 'Aha, and an Argghhh' in one)\n\nEven here where you call it 'internal' storage got confusing. I suddenly\nthought - oh there's some other storage being pointed to somewehre :-(\n\nIf that was a 'union' the code would have been self documenting - but\nbecause it wasn't .. I got confused.\n\nI'm not necessarily suggesting turn it into a union, as that might just\nbe adding lines for no real gain - but a comment somewhere to directly\nstate how it's used could help.\n\n\n> (size > 8, see the set() function below), and delete it in release()\n> using the same condition.\n\n>> That's making the reallocation below a bit ugly only to provide a function\n>> that doesn't do anything to the destructor?\n>>\n>> (Doesn't do anything; based on assumption that after the object is used it\n>> has an allocation which is of suitable size already)\n>>\n>>>> +}\n>>>>> +\n>>>>> +/**\n>>>>> + * \\brief Contruct a ControlValue with the content of \\a other\n>>>>\n>>>> /Contruct/Construct/\n>>>>\n>>>>> + * \\param[in] other The ControlValue to copy content from\n>>>>> + */\n>>>>> +ControlValue::ControlValue(const ControlValue &other)\n>>>>> + : type_(ControlTypeNone), numElements_(0)\n>>>>> +{\n>>>>> + *this = other;\n>>>>> +}\n>>>>> +\n>>>>> +/**\n>>>>> + * \\brief Replace the content of the ControlValue with the one of \\a other\n>>>>> + * \\param[in] other The ControlValue to copy content from\n>>>>> + *\n>>>>> + * Deep-copy the content of \\a other into the ControlValue by reserving memory\n>>>>> + * and copy data there in case \\a other transports arrays of values in one of\n>>>>> + * its pointer data members.\n>>>>\n>>>> That's hard to parse...\n>>>\n>>> Agreed. I'll drop the paragraph as I don't think it brings much.\n>>>\n>>>>> + *\n>>>>> + * \\return The ControlValue with its content replaced with the one of \\a other\n>>>>> */\n>>>>> +ControlValue &ControlValue::operator=(const ControlValue &other)\n>>>>> +{\n>>>>> + set(other.type_, other.isArray_, other.data().data(),\n>>>>> + other.numElements_, ControlValueSize[other.type_]);\n>>>>> + return *this;\n>>>>> +}\n>>>>\n>>>> Do we need/desire move support to just move the allocated storage over\n>>>> at all ?\n>>>\n>>> I think we do, but I think it can be done on top.\n>>\n>> Sure\n>>\n>>>>>\n>>>>> /**\n>>>>> * \\fn ControlValue::type()\n>>>>> @@ -102,16 +151,33 @@ ControlValue::ControlValue()\n>>>>> * \\return True if the value type is ControlTypeNone, false otherwise\n>>>>> */\n>>>>>\n>>>>> +/**\n>>>>> + * \\fn ControlValue::isArray()\n>>>>> + * \\brief Determine if the value stores an array\n>>>>> + * \\return True if the value stores an array, false otherwise\n>>>>> + */\n>>>>> +\n>>>>> +/**\n>>>>> + * \\fn ControlValue::numElements()\n>>>>> + * \\brief Retrieve the number of elements stored in the ControlValue\n>>>>> + *\n>>>>> + * For instances storing an array, this function returns the number of elements\n>>>>> + * in the array. Otherwise, it returns 1.\n>>>>> + *\n>>>>> + * \\return The number of elements stored in the ControlValue\n>>>>> + */\n>>>>> +\n>>>>> /**\n>>>>> * \\brief Retrieve the raw of a control value\n>>>>> * \\return The raw data of the control value as a span of uint8_t\n>>>>> */\n>>>>> Span ControlValue::data() const\n>>>>> {\n>>>>> - return {\n>>>>> - reinterpret_cast(&bool_),\n>>>>> - ControlValueSize[type_]\n>>>>> - };\n>>>>> + std::size_t size = numElements_ * ControlValueSize[type_];\n>>>>> + const uint8_t *data = size > sizeof storage_\n>>>>\n>>>> sizeof without 'containing' what it parses really looks wrong to me :S\n>>>\n>>> I'll update this.\n>>>\n>>>>> + ? *reinterpret_cast(&storage_)\n>>>>> + : reinterpret_cast(&storage_);\n>>>>> + return { data, size };\n>>>>\n>>>> Ahh, at least that looks better than the multiline return statement we\n>>>> had before :-)\n>>>>\n>>>>> }\n>>>>>\n>>>>> /**\n>>>>> @@ -120,18 +186,43 @@ Span ControlValue::data() const\n>>>>> */\n>>>>> std::string ControlValue::toString() const\n>>>>> {\n>>>>> - switch (type_) {\n>>>>> - case ControlTypeNone:\n>>>>> - return \"\";\n>>>>> - case ControlTypeBool:\n>>>>> - return bool_ ? \"True\" : \"False\";\n>>>>> - case ControlTypeInteger32:\n>>>>> - return std::to_string(integer32_);\n>>>>> - case ControlTypeInteger64:\n>>>>> - return std::to_string(integer64_);\n>>>>> + if (type_ == ControlTypeNone)\n>>>>> + return \"\";\n>>>>> +\n>>>>> + const uint8_t *data = ControlValue::data().data();\n>>>>> + std::string str(isArray_ ? \"[ \" : \"\");\n>>>>> +\n>>>>> + for (unsigned int i = 0; i < numElements_; ++i) {\n>>>>> + switch (type_) {\n>>>>> + case ControlTypeBool: {\n>>>>> + const bool *value = reinterpret_cast(data);\n>>>>> + str += *value ? \"True\" : \"False\";\n>>>>> + break;\n>>>>> + }\n>>>>> + case ControlTypeInteger32: {\n>>>>> + const int32_t *value = reinterpret_cast(data);\n>>>>> + str += std::to_string(*value);\n>>>>> + break;\n>>>>> + }\n>>>>> + case ControlTypeInteger64: {\n>>>>> + const int64_t *value = reinterpret_cast(data);\n>>>>> + str += std::to_string(*value);\n>>>>> + break;\n>>>>> + }\n>>>>> + case ControlTypeNone:\n>>>>> + break;\n>>>>> + }\n>>>>> +\n>>>>> + if (i + 1 != numElements_)\n>>>>> + str += \", \";\n>>>>> +\n>>>>> + data += ControlValueSize[type_];\n>>>>> }\n>>>>>\n>>>>> - return \"\";\n>>>>> + if (isArray_)\n>>>>> + str += \" ]\";\n>>>>> +\n>>>>> + return str;\n>>>>\n>>>> Ohh toString() is neat here :-)\n>>>>\n>>>>> }\n>>>>>\n>>>>> /**\n>>>>> @@ -143,16 +234,13 @@ bool ControlValue::operator==(const ControlValue &other) const\n>>>>> if (type_ != other.type_)\n>>>>> return false;\n>>>>>\n>>>>> - switch (type_) {\n>>>>> - case ControlTypeBool:\n>>>>> - return bool_ == other.bool_;\n>>>>> - case ControlTypeInteger32:\n>>>>> - return integer32_ == other.integer32_;\n>>>>> - case ControlTypeInteger64:\n>>>>> - return integer64_ == other.integer64_;\n>>>>> - default:\n>>>>> + if (numElements_ != other.numElements())\n>>>>> return false;\n>>>>> - }\n>>>>> +\n>>>>> + if (isArray_ != other.isArray_)\n>>>>> + return false;\n>>>>> +\n>>>>> + return memcmp(data().data(), other.data().data(), data().size()) == 0;\n>>\n>> Is there some data involved in that code by any chance? :-)\n> \n> So you've spotted the subliminal message ;-)\n> \n>>>> }\n>>>>>\n>>>>> /**\n>>>>> @@ -165,8 +253,16 @@ bool ControlValue::operator==(const ControlValue\n>>> &other) const\n>>>>> * \\fn template T ControlValue::get() const\n>>>>> * \\brief Get the control value\n>>>>> *\n>>>>> - * The control value type shall match the type T, otherwise the behaviour is\n>>>>> - * undefined.\n>>>>> + * This function returns the contained value as an instance of \\a T. If the\n>>>>> + * ControlValue instance stores a single value, the type \\a T shall match the\n>>>>> + * stored value type(). If the instance stores an array of values, the type\n>>>>> + * \\a T should be equal to Span, and the type \\a R shall match the\n>>>>> + * stored value type(). The behaviour is undefined otherwise.\n>>>>> + *\n>>>>> + * Note that a ControlValue instance that stores a non-array value is not\n>>>>> + * equivalent to an instance that stores an array value containing a single\n>>>>> + * element. The latter shall be accessed through a Span type, while\n>>>>> + * the former shall be accessed through a type \\a T corresponding to type().\n>>>>> *\n>>>>> * \\return The control value\n>>>>> */\n>>>>> @@ -175,8 +271,41 @@ bool ControlValue::operator==(const ControlValue &other) const\n>>>>> * \\fn template void ControlValue::set(const T &value)\n>>>>> * \\brief Set the control value to \\a value\n>>>>> * \\param[in] value The control value\n>>>>> + *\n>>>>> + * This function stores the \\a value in the instance. If the type \\a T is\n>>>>> + * equivalent to Span, the instance stores an array of values of type \\a R.\n>>>>> + * Otherwise the instance stores a single value of type \\a T. The numElements()\n>>>>> + * and type() are updated to reflect the stored value.\n>>>>> + *\n>>>>> + * The entire content of \\a value is copied to the instance, no reference to \\a\n>>>>> + * value or to the data it references is retained. This may be an expensive\n>>>>> + * operation for Span<> values that refer to large arrays.\n>>>>> */\n>>>>>\n>>>>> +void ControlValue::set(ControlType type, bool isArray, const void *data,\n>>>>> + std::size_t numElements, std::size_t elementSize)\n>>>>> +{\n>>>>> + ASSERT(elementSize == ControlValueSize[type]);\n>>>>> +\n>>>>> + release();\n>>\n>> I hadn't noticed this here.\n>>\n>> What isn't clear here is that release is conditional on size > storage, so\n>> i think this would be clearer to perform the delete before new below.\n> \n> The purpose of release() is to free storage if it has been allocated, so\n> that the callers don't need to care. We need to free storage here if it\n> has been allocated regardless of the new size (and thus regardless of\n> this set() call will end up allocating memory or using internal\n> storage).\n\nYes, it's the fact that the 'internal' storage gets 'used' as a pointer,\nrather than it always being a pointer that I seem to have completely\nglossed over.\n\nOf course if it was always a pointer it would have been declared as such\nso that maybe should have given the game away. ... maybe I was/am just\ntoo tired.\n\nHo hum ... all the more reason to go on holiday next week :-)\n\n\n\n>>>> +\n>>>>> + type_ = type;\n>>>>> + numElements_ = numElements;\n>>>>> + isArray_ = isArray;\n>>>>> +\n>>>>> + std::size_t size = elementSize * numElements;\n>>>>> + void *storage;\n>>>>> +\n>>>>> + if (size > sizeof storage_) {\n>>>\n>>> I'll update this one too.\n>>>\n>>>>> + storage = reinterpret_cast(new char[size]);\n>>>>> + *reinterpret_cast(&storage_) = storage;\n>>>>\n>>>> Doesn't this need to delete/free any existing storage? Or does the\n>>>> assignment do that automagically?\n>>>\n>>> The release() call above handles it.\n>>>\n>>>>> + } else {\n>>>>> + storage = reinterpret_cast(&storage_);\n>>\n>> Because now you've highlighted the release call, this \"looks\" like it's\n>> setting storage to a \"released\" allocation :-(\n>>\n>> And as far as i can tell with a small view on a phone, release() isn't\n>> going to do anything in the destructor ... Which is a leak...\n> \n> In this branch we use the internal storage, so release() doesn't need to\n> free it. In the above branch we allocate external storage, which will be\n> freed by release() in the destructor (or when this function is called\n> again).\n> \n\nAha so uint64_t storage_ is actually:\n\nunion {\n uint64_t int64; /* Store Control Values which fit directly */\n void *ptr; /* External allocation required */\n}\n\nWhere it will either store the data in an int64 or use storage_ as a\npointer to allocated memory.\n\nI completely misinterpreted it on first read as it only being only\nessentially a void *ptr (or I guess by that definition it's a uintptr_t?)\n\n\n\n\n\n>> Could you verify that please and maybe simplify the code to make sure it's\n>> clear?\n>>\n>>>> + }\n>>>>> +\n>>>>> + memcpy(storage, data, size);\n>>>>> +}\n>>>>> +\n>>>>> /**\n>>>>> * \\class ControlId\n>>>>> * \\brief Control static metadata\n>","headers":{"Return-Path":"","Received":["from mail-wr1-x432.google.com (mail-wr1-x432.google.com\n\t[IPv6:2a00:1450:4864:20::432])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5AA8762826\n\tfor ;\n\tSat, 7 Mar 2020 23:07:40 +0100 (CET)","by mail-wr1-x432.google.com with SMTP id r7so6521295wro.2\n\tfor ;\n\tSat, 07 Mar 2020 14:07:40 -0800 (PST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net. [86.31.129.233])\n\tby smtp.gmail.com with ESMTPSA id\n\to16sm39919617wrj.5.2020.03.07.14.07.38\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tSat, 07 Mar 2020 14:07:38 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=bingham-xyz.20150623.gappssmtp.com; s=20150623;\n\th=subject:to:cc:references:from:openpgp:autocrypt:message-id:date\n\t:user-agent:mime-version:in-reply-to:content-language\n\t:content-transfer-encoding;\n\tbh=u5/e5UT1E0Ycc76i9/7WxSgrqCQxGw06fLhkVQIg998=;\n\tb=k8y7vUwgoe99gXOhsBpRJHVUXUc8E9cTo57+QCx1TmTvJgRRjpV1ETq0Tk4KoEviD2\n\trc1gNZbetgJmVt3ZBTJKjWZoa8cFa3fwNzlakfh0q2wsSFCDmU/PxTniXAwsM8THMM0l\n\tvzbZzgP+qh9zqmoCMlpFp/M/hvZg+hiNjmCOQeMLX9aDyHTF1JDDCnAMZbhQl1t6mGg5\n\t87BiS2PrdxJqhJj+L63tKr1KlmyGPwwPBpa58YWnDO8HMmq/d3K5o55kLGB5HSEvns8o\n\t14XPl7fQTPbKa5mgsISHp8UQAlUi9T5Z9bD7W8E16abhwjmTCbgz3UH4PxGe1/D0zYEz\n\tA2aA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:subject:to:cc:references:from:openpgp:autocrypt\n\t:message-id:date:user-agent:mime-version:in-reply-to\n\t:content-language:content-transfer-encoding;\n\tbh=u5/e5UT1E0Ycc76i9/7WxSgrqCQxGw06fLhkVQIg998=;\n\tb=fBvaT6ajoYLGaOwb8+kY7qbDHNoPS14FBAnmMLKeovc2w3CFg+AmGFjJB1TN+vfqAn\n\tYw+GBtiagQinG4lNzv3rINDja+mPKk9sZTaDuqtSDloHM8146JmU+pw+EnFXMKwLRIvR\n\tyhOFNa5camuYC4fnNSZWjAJpSOyKOV9bfw3QBaF5I9BIF8i1T43U17CVznoV7hR91FSa\n\tlyHrqiORdDEcQ3bHWaVw4lFXeQUAJsHydxyNLU4TEsv5v+qccPCANvAObRvEPDGLVboH\n\tKajhs6/vgzu8QkyDuCDfnGo+r8HtS4AkAoOY4ueDihYcP9ktvVIyML5IAC5hAUUvTMQ0\n\tR+4w==","X-Gm-Message-State":"ANhLgQ04nbeVMgT0j+9oWx9/Z2qUrTFNcUo6py61C3psrGsmtGLzWrL4\n\tF5+sOS3AZgRih4doMkRVFUj52qQi/4w=","X-Google-Smtp-Source":"ADFU+vtDjbyx8YCnAQ2FRDJBXPZhE+Xz59ppPb+IGr4AgkyS7y0UoQBwihmAdmtbiTwezMWgc5z8tw==","X-Received":"by 2002:adf:ffc4:: with SMTP id\n\tx4mr11492328wrs.306.1583618859436; \n\tSat, 07 Mar 2020 14:07:39 -0800 (PST)","To":"Laurent Pinchart ","Cc":"Kieran Bingham ,\n\tlibcamera devel ","References":"<20200229164254.23604-17-laurent.pinchart@ideasonboard.com>\n\t<20200301175454.10921-1-laurent.pinchart@ideasonboard.com>\n\t<5b0e6fd3-7600-5910-118c-18d37b5f6872@ideasonboard.com>\n\t<20200306155556.GP4878@pendragon.ideasonboard.com>\n\t\n\t<20200307181731.GA5021@pendragon.ideasonboard.com>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran@bingham.xyz; keydata=\n\tmQINBFMtlTkBEADvhPl7usumM98GeJgEv0R+atr1fwfMtV2pkpqkTc7RrO+VKc++WDDXGqWG\n\twnNX0FzJ7/TEJoO5BZ+VyHqB1kMjxAckmCKQIrj2/UxkZ/R5lxKzvbve7XDvihnTgQrZv3bw\n\t52Tz81DMTFG+N0yeUOZWnq+mPoNCf9OnkKkPnyWVPdtYeLJmi2oE5ql7/ZEBU6m0BAzRKYny\n\tk69pyQO1zzTb3U6GHGEUc+8CgGolqBQ63qp+MmaQYlA2ytOw8DMiBLJZipVUWS/WgvCvIWkH\n\tlVoI4r8cBSgN4pgRJEKeVXVw+uY8xAbOU3r2y/MfyykzJn99oiaHeNer39EIVRdxKnazYw95\n\tq/RE6dtbroSGcAfa7hIqfqya5nTGzONbxNPdUaWpj3vkej/o5aESXcRk98fH+XCKlS+a/tri\n\t7dfq3/Daoq0LR3wmHvEXN8p52NQlbMCnfEhE+haSLqLEgxTqCMpBt4cgwaW9CmKW8pR91oXF\n\tkIDVY9e/VU9tw3IuoHVK5JXmZeaUe1wLmot2oiq2hmuRonQNGEYWqU6lnoDHTQArLfZPaT9Y\n\thQqf9C7faWF/VvEwXYYquWOX+waY8YPyH16hycmWoePM+oMpIG+04lpjEefSHDUvOciC0p1o\n\tCfePg3iVEKB56V0j9nMAfTr/5oOvTP5EeHHvT6a5ZcGanJYmbQARAQABtCNLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuQGJpbmdoYW0ueHl6PokCVQQTAQoAPwIbAwYLCQgHAwIGFQgCCQoLBBYC\n\tAwECHgECF4AWIQSor+z47OVRZQR+u5Yjhj3Dgx2ysQUCXWTt6gUJDfm/sQAKCRAjhj3Dgx2y\n\tsXNuEACOOFM9Kwq1U8a1hC57HCD37GAcwPXEe5+elO6ORGALzjjHmq9GJf3FbIuV9b0pzyGU\n\tXsNiZKqxmFga9+FocN28REHzKp5eo9/5yFcDsZJYqgEwbqQ5Yw9ZONr6Gw+x+a4VeMVao9+w\n\tBAwWK3nNqsfbW6Y+ewq1EIg0BajfHEaESGizyQ5DnOefTf+uGcmZ+XYASwUTkqXvwSVoRTS0\n\t4nXCOVG2LGhM9bc5zLXXsgPjH2xx8vLSqebXyIuam0d8X2/R6mFHkI9Oh0n5feEs0i80vMyB\n\teEYDeZGNnkrPkosWKYo6KeC/QmpAIqYytDuevhJMD/cK5ugWc9tfzpwkKb7mFm+7aUU7wUhl\n\t9OO/lhAAO5B8uVgv55ZxFS1wVrgi/0DnWZx7dDj+b0xubexMoRqdtNMBcw4ey9sQ2TMfLuLX\n\tsaq93eNA8tmKLRZrFKuGeSQBj0u/1KGKitDUxGEOjCkZZ5R7i0IhOmMXCCpSlRH6TYzHtkLC\n\tqLMGnCSuHv0AUtXE37OlRPLf3cga8SqJJyLJ+2jwDCr1xT32cLiD19jYgfsnS0+gvl52gn9a\n\tf4K76WtYlFf/RMGl4N1fLLcVLMt3QuYjPbVQVcMxXWS5cIQFpUSWo2d8Z7kWrHJ8jL4/ZxxZ\n\tmPkwI2lLHEmvvlBO0tsnECtkApB/hc9/aQCa1gUWzLkCDQRTLZU5ARAAsqUr9WS+cuZ3aZP/\n\tUV2vO6HZ6L8gHJQcMVV22uBRccuet4QEPQ9UgURac9lWjqUlCOmWU1HgISjM1oD3siakeqRB\n\tTHvRv3p7Za55DJOlYj+HhM7q4l2m7FlSKqlEABIuL02FvjtRMsobPhpTu1vjBGe0VMKafqkG\n\t0CbLKnFwkRxjVMZSqVMws1hlXEeTK27IJxzoxptfDHKj6w54J367tO0ofubxLA3RvebxZG7D\n\t1vWe8NTrNYItuMaXtq4tbbxGY3In2YE+8G9mAQsG1p+XSIm6UBO0lBZJ+NURy/aYmpma39Ji\n\t9hE1YZmcDhuRfBPXKSXJa8VavEAON8VbFAtqcXtS/8GbXLzSmUKf/fULHbiWWgspKoMhoWCD\n\tryOgABqoc8pu1+XL6uTsr2VksbgXun0IdadI1EVXzc9Hgtra7bZ7C8KzTOgp8u1MFHTyynlO\n\tQnAosbxVcXSQ95KcEb3V1nMhmzJ5r85Nvlxs2ROqM+/e/Cf16DYPe4iaoHhxuPrAe0ul4/21\n\tdoJq4WVkknqIUpTZkVV/6rLfuFhjKszF5sUXIcOqOn3tYCz/eCxQsXXaq0DBw1IOsQpnq8yP\n\tMXJ7mNV7ZcKd/4ocX3F6PLFMf2SBGoeive37xf3wdM1Nf4s342D778suPHJmf5+0BQLSv1R0\n\tVhTpst0W0c7ge0ozFOcAEQEAAYkCHwQYAQIACQUCUy2VOQIbDAAKCRAjhj3Dgx2ysQmtEADF\n\tKynuTGR5fIVFM0wkAvPBWkh9kMcQwK+PjDR1p7JqNXnlIraBOHlRfxXdu6uYabQ4pyAAPiHt\n\tfCoCzIvsebXsArbdl7IGBc7gBw/pBXAo7Bt24JfbGCrKkpzu6y2iKT/G8oZP37TlkK6D86nm\n\tYBY/UqbMbNe28CUeIhTyeVDx28gbDJc1rndOL2cz4BIlzg3Di47woMWnEuaCQ536KM61LnY7\n\tp/pJ9RcvLrOIm2ESy5M5gHouH7iXNzn5snKFhfi1zbTT/UrtEuY1VjCtiTcCXzXbzy2oy/zw\n\tERaDwkRzhcVrFdsttMYDyaNY3GQfJSBq4Q9rADG2nn/87e3g7dmPecVYS5YFxocCk77Zg7xx\n\tGxSDtXgJEVmdGTGYCrM+SrW8ywj03kfwnURqOnxbsbHaSUmJtVovA+ZzdpHV1e7S91AvxbXt\n\tLrxWADsl+pzz9rJ25+Hh7f/HeflGaUDYbOycQVzcyKekKkuIlibpv+S0nPiitxlV91agRV0i\n\tcpG0pX8PrmjQ0YV8pvfUFyrfHtHzTMA4ktMNzF5FhNkE1WNwXZHD+P6nmPEZiOi45tqI7Ro6\n\tmX/IKTr6GLCzg0OVP6NSsgSJeR6Hd2GvSI2Vw1jfnZI4tCNU2BmODPBkGBRLhNR+L5eRqOMm\n\tQglrIkyNWSZm4Hhw98VxYDwOwmYhoXmAFg==","Message-ID":"<40a7fcf2-33aa-098c-15d2-ff0798400302@bingham.xyz>","Date":"Sat, 7 Mar 2020 22:07:37 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.9.1","MIME-Version":"1.0","In-Reply-To":"<20200307181731.GA5021@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v1.2 16/31] libcamera: controls:\n\tSupport array controls in ControlValue","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sat, 07 Mar 2020 22:07:40 -0000"}},{"id":3997,"web_url":"https://patchwork.libcamera.org/comment/3997/","msgid":"<20200307232040.GA31018@pendragon.ideasonboard.com>","date":"2020-03-07T23:20:40","subject":"Re: [libcamera-devel] [PATCH v1.2 16/31] libcamera: controls:\n\tSupport array controls in ControlValue","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Sat, Mar 07, 2020 at 10:07:37PM +0000, Kieran Bingham wrote:\n> On 07/03/2020 18:17, Laurent Pinchart wrote:\n> > On Sat, Mar 07, 2020 at 06:04:23PM +0000, Kieran Bingham wrote:\n> >> On Fri, 6 Mar 2020, 15:56 Laurent Pinchart wrote:\n> >>> On Tue, Mar 03, 2020 at 12:23:10AM +0000, Kieran Bingham wrote:\n> >>>> On 01/03/2020 17:54, Laurent Pinchart wrote:\n> >>>>> From: Jacopo Mondi \n> >>>>>\n> >>>>> Add array controls support to the ControlValue class. The polymorphic\n> >>>>> class can now store more than a single element and supports access and\n> >>>>> creation through the use of Span<>.\n> >>>>\n> >>>> Oh, but if the creation is through a span, what stores the data in the\n> >>>> ControlValue ... I guess I'll see below... \n> >>>>\n> >>>>> Signed-off-by: Jacopo Mondi \n> >>>>> Signed-off-by: Laurent Pinchart \n> >>>>\n> >>>> Some comments, but nothing you can't handle...\n> >>>>\n> \n> Continued ramblings below are unfortunately out-of-order due to replying\n> in situ on existing comments but the general message is:\n> \n> Ok - so no leak like I feared, and except the fact that I misinterpreted\n> the definition/usage of storage_ which might need some comment to make\n> it clearer (or might not because I might just be overtired), the\n> following still stands ... and I don't think there's any blocker on this\n> series :-)\n> \n> >>>> Reviewed-by: Kieran Bingham \n> >>>>\n> >>>>> ---\n> >>>>> Changes since v1:\n> >>>>>\n> >>>>> - Use T::value_type instead of T::element_type to benefit from\n> >>>>> std::remove_cv\n> >>>>> - Fix ControlTypeNone test in ControlValue::toString()\n> >>>>> - Separate array elements with \", \" in ControlValue::toString()\n> >>>>> ---\n> >>>>> include/libcamera/controls.h | 81 ++++++++++++---\n> >>>>> src/libcamera/controls.cpp | 185 +++++++++++++++++++++++++++++------\n> >>>>> 2 files changed, 225 insertions(+), 41 deletions(-)\n> >>>>>\n> >>>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> >>>>> index 4538be06af93..1e24ae30ab36 100644\n> >>>>> --- a/include/libcamera/controls.h\n> >>>>> +++ b/include/libcamera/controls.h\n> >>>>> @@ -9,6 +9,7 @@\n> >>>>> #define __LIBCAMERA_CONTROLS_H__\n> >>>>>\n> >>>>> #include \n> >>>>> +#include \n> >>>>> #include \n> >>>>> #include \n> >>>>>\n> >>>>> @@ -51,6 +52,10 @@ struct control_type {\n> >>>>> static constexpr ControlType value = ControlTypeInteger64;\n> >>>>> };\n> >>>>>\n> >>>>> +template\n> >>>>> +struct control_type> : public control_type> {\n> >>>>> +};\n> >>>>> +\n> >>>>> } /* namespace details */\n> >>>>>\n> >>>>> class ControlValue\n> >>>>> @@ -58,15 +63,35 @@ class ControlValue\n> >>>>> public:\n> >>>>> ControlValue();\n> >>>>>\n> >>>>> +#ifndef __DOXYGEN__\n> >>>>> + template::value, std::nullptr_t> = nullptr>\n> >>>>> + ControlValue(const T &value)\n> >>>>> + : type_(ControlTypeNone), numElements_(0)\n> >>>>> + {\n> >>>>> + set(details::control_type>::value, false,\n> >>>>> + &value, 1, sizeof(T));\n> >>>>> + }\n> >>>>> +\n> >>>>> + template::value, std::nullptr_t> = nullptr>\n> >>>>> +#else\n> >>>>> template\n> >>>>> - ControlValue(T value)\n> >>>>> - : type_(details::control_type>::value)\n> >>>>> +#endif\n> >>>>\n> >>>> That #iffery is pretty horrible, removes one function and changes the\n> >>>> template instantiation of this below function ?\n> >>>>\n> >>>> Though seeing the repeating pattern below too - I can't offer a better\n> >>>> solution...\n> >>>>\n> >>>>> + ControlValue(const T &value)\n> >>>>> + : type_(ControlTypeNone), numElements_(0)\n> >>>>> {\n> >>>>> - *reinterpret_cast(&bool_) = value;\n> >>>>> + set(details::control_type>::value, true>,\n> >>>>> + value.data(), value.size(), sizeof(typename T::value_type));\n> >>>>\n> >>>> What's true here ? This is not very friendly to read...\n> >>>\n> >>> Should I add\n> >>>\n> >>> enum {\n> >>> ControlIsSingle = 1,\n> >>> ControlIsArray = 1,\n> >>> };\n> >>>\n> >>> ? I don't like the name ControlIsSingle though. Maybe this could be done\n> >>> on top, if you think it's worth it ? If you can propose a better name\n> >>> I'll submit a patch :-)\n> >>>\n> >>>> Ok - so on the plus side the bool_ is gone :-)\n> >>>>\n> >>>>> }\n> >>>>>\n> >>>>> + ~ControlValue();\n> >>>>> +\n> >>>>> + ControlValue(const ControlValue &other);\n> >>>>> + ControlValue &operator=(const ControlValue &other);\n> >>>>> +\n> >>>>> ControlType type() const { return type_; }\n> >>>>> bool isNone() const { return type_ == ControlTypeNone; }\n> >>>>> + bool isArray() const { return isArray_; }\n> >>>>> + std::size_t numElements() const { return numElements_; }\n> >>>>> Span data() const;\n> >>>>>\n> >>>>> std::string toString() const;\n> >>>>> @@ -77,31 +102,61 @@ public:\n> >>>>> return !(*this == other);\n> >>>>> }\n> >>>>>\n> >>>>> +#ifndef __DOXYGEN__\n> >>>>> + template::value, std::nullptr_t> = nullptr>\n> >>>>> + T get() const\n> >>>>> + {\n> >>>>> + assert(type_ == details::control_type>::value);\n> >>>>> + assert(!isArray_);\n> >>>>> +\n> >>>>> + return *reinterpret_cast(data().data());\n> >>>>> + }\n> >>>>> +\n> >>>>> + template::value, std::nullptr_t> = nullptr>\n> >>>>> +#else\n> >>>>> template\n> >>>>> +#endif\n> >>>>> T get() const\n> >>>>> {\n> >>>>> assert(type_ == details::control_type>::value);\n> >>>>> + assert(isArray_);\n> >>>>> +\n> >>>>> + using V = typename T::value_type;\n> >>>>> + const V *value = reinterpret_cast(data().data());\n> >>>>> + return { value, numElements_ };\n> >>>>> + }\n> >>>>>\n> >>>>> - return *reinterpret_cast(&bool_);\n> >>>>> +#ifndef __DOXYGEN__\n> >>>>> + template::value, std::nullptr_t> = nullptr>\n> >>>>> + void set(const T &value)\n> >>>>> + {\n> >>>>> + set(details::control_type>::value, false,\n> >>>>> + reinterpret_cast(&value), 1, sizeof(T));\n> >>>>> }\n> >>>>>\n> >>>>> + template::value, std::nullptr_t> = nullptr>\n> >>>>> +#else\n> >>>>> template\n> >>>>> +#endif\n> >>>>> void set(const T &value)\n> >>>>> {\n> >>>>> - type_ = details::control_type>::value;\n> >>>>> - *reinterpret_cast(&bool_) = value;\n> >>>>> + set(details::control_type>::value, true,\n> >>>>> + value.data(), value.size(), sizeof(typename T::value_type));\n> >>>>> }\n> >>>>>\n> >>>>> private:\n> >>>>> - ControlType type_;\n> >>>>> -\n> >>>>> - union {\n> >>>>> - bool bool_;\n> >>>>> - int32_t integer32_;\n> >>>>> - int64_t integer64_;\n> >>>>> - };\n> >>>>> + ControlType type_ : 8;\n> >>>>> + bool isArray_ : 1;\n> >>>>> + std::size_t numElements_ : 16;\n> >>>>> + uint64_t storage_;\n> >>>>> +\n> >>>>> + void release();\n> >>>>> + void set(ControlType type, bool isArray, const void *data,\n> >>>>> + std::size_t numElements, std::size_t elementSize);\n> >>>>> };\n> >>>>>\n> >>>>> +static_assert(sizeof(ControlValue) == 16, \"Invalid size of ControlValue class\");\n> >>>>\n> >>>> Aha, I knew this ASSERT_ABI_SIZE would come up again :-)\n> >>>\n> >>> I think it's a good idea, and I think we should actually try to\n> >>> automate that through all our classes, to have automatic ABI regression\n> >>> tests. I envision that as being done through code analysis instead of\n> >>> static_assert(). And I wouldn't be surprised if tools already existed\n> >>> for this purpose.\n> >>>\n> >>>>> +\n> >>>>> class ControlId\n> >>>>> {\n> >>>>> public:\n> >>>>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> >>>>> index b2331ab7540d..a5a385aa1b0a 100644\n> >>>>> --- a/src/libcamera/controls.cpp\n> >>>>> +++ b/src/libcamera/controls.cpp\n> >>>>> @@ -10,6 +10,7 @@\n> >>>>> #include \n> >>>>> #include \n> >>>>> #include \n> >>>>> +#include \n> >>>>>\n> >>>>> #include \"control_validator.h\"\n> >>>>> #include \"log.h\"\n> >>>>> @@ -50,7 +51,7 @@ LOG_DEFINE_CATEGORY(Controls)\n> >>>>> namespace {\n> >>>>>\n> >>>>> static constexpr size_t ControlValueSize[] = {\n> >>>>> - [ControlTypeNone] = 1,\n> >>>>> + [ControlTypeNone] = 0,\n> >>>>> [ControlTypeBool] = sizeof(bool),\n> >>>>> [ControlTypeInteger32] = sizeof(int32_t),\n> >>>>> [ControlTypeInteger64] = sizeof(int64_t),\n> >>>>> @@ -80,15 +81,63 @@ static constexpr size_t ControlValueSize[] = {\n> >>>>> * \\brief Construct an empty ControlValue.\n> >>>>> */\n> >>>>> ControlValue::ControlValue()\n> >>>>> - : type_(ControlTypeNone)\n> >>>>> + : type_(ControlTypeNone), isArray_(false), numElements_(0)\n> >>>>> {\n> >>>>> }\n> >>>>>\n> >>>>> /**\n> >>>>> - * \\fn template T ControlValue::ControlValue(T value)\n> >>>>> + * \\fn template T ControlValue::ControlValue(const T &value)\n> >>>>> * \\brief Construct a ControlValue of type T\n> >>>>> * \\param[in] value Initial value\n> >>>>> + *\n> >>>>> + * This function constructs a new instance of ControlValue and stores the \\a\n> >>>>> + * value inside it. If the type \\a T is equivalent to Span, the instance\n> >>>>> + * stores an array of values of type \\a R. Otherwise the instance stores a\n> >>>>> + * single value of type \\a T. The numElements() and type() are updated to\n> >>>>> + * reflect the stored value.\n> >>>>> + */\n> >>>>> +\n> >>>>> +void ControlValue::release()\n> >>>>> +{\n> >>>>> + std::size_t size = numElements_ * ControlValueSize[type_];\n> >>>>> +\n> >>>>> + if (size > sizeof storage_) {\n> >>>>\n> >>>> sizeof(storage) would read nicer to me here. ...\n> >>>\n> >>> sizeof is an operator and doesn't require parentheses:\n> >>> https://en.cppreference.com/w/cpp/language/sizeof. I'll change it\n> >>> though.\n> >>>\n> >>>> Oh ... uhm isn't storage a uint64_t? And therefore always 8?\n> \n> Ah, my (incorrect) inference here was that it was supposed to not always\n> be 8...\n> \n> >>>\n> >>> Correct, but that's better than hardcoding the value 8, right ? :-)\n> >>\n> >> Indeed :-)\n> \n> \n> And now I read that again it clears up my earlier confusion. I had seen\n> that storage_ was being used as a pointer, and thus was expecting the\n> sizeofs to be determining the size of the available memory to store.\n> \n> It's the fact that this > sizeof(storage_) determines if the storage_ is\n> used as those bytes directly or as a pointer to an allocation.\n> \n> It seems that's easy to miss - and can cause confusion. Have I missed\n> the documentation or comments that clears that up? If not - could\n> something be added please?\n\nDoes \"[PATCH] libcamera: controls: Fix strict aliasing violation\" help ?\nI can also add more documentation on top.\n\n> >>>>> + delete[] *reinterpret_cast(&storage_);\n> >>>>> + storage_ = 0;\n> >>>>> + }\n> >>>>> +}\n> >>>>> +\n> >>>>> +ControlValue::~ControlValue()\n> >>>>> +{\n> >>>>> + release();\n> >>\n> >> Who deletes storage_ on destruct?\n> > \n> > release() does.\n> > \n> >> Release only appears to delete in the event that size is bigger than the\n> >> storage available to help the set() call?\n> > \n> > release() deletes the storage if it has been allocated. We allocate\n> > external storage in case the data won't fit in the internal storage\n> \n> Ahhhhhhhh (that's both an 'Aha, and an Argghhh' in one)\n> \n> Even here where you call it 'internal' storage got confusing. I suddenly\n> thought - oh there's some other storage being pointed to somewehre :-(\n> \n> If that was a 'union' the code would have been self documenting - but\n> because it wasn't .. I got confused.\n> \n> I'm not necessarily suggesting turn it into a union, as that might just\n> be adding lines for no real gain - but a comment somewhere to directly\n> state how it's used could help.\n> \n> > (size > 8, see the set() function below), and delete it in release()\n> > using the same condition.\n> >\n> >> That's making the reallocation below a bit ugly only to provide a function\n> >> that doesn't do anything to the destructor?\n> >>\n> >> (Doesn't do anything; based on assumption that after the object is used it\n> >> has an allocation which is of suitable size already)\n> >>\n> >>>> +}\n> >>>>> +\n> >>>>> +/**\n> >>>>> + * \\brief Contruct a ControlValue with the content of \\a other\n> >>>>\n> >>>> /Contruct/Construct/\n> >>>>\n> >>>>> + * \\param[in] other The ControlValue to copy content from\n> >>>>> + */\n> >>>>> +ControlValue::ControlValue(const ControlValue &other)\n> >>>>> + : type_(ControlTypeNone), numElements_(0)\n> >>>>> +{\n> >>>>> + *this = other;\n> >>>>> +}\n> >>>>> +\n> >>>>> +/**\n> >>>>> + * \\brief Replace the content of the ControlValue with the one of \\a other\n> >>>>> + * \\param[in] other The ControlValue to copy content from\n> >>>>> + *\n> >>>>> + * Deep-copy the content of \\a other into the ControlValue by reserving memory\n> >>>>> + * and copy data there in case \\a other transports arrays of values in one of\n> >>>>> + * its pointer data members.\n> >>>>\n> >>>> That's hard to parse...\n> >>>\n> >>> Agreed. I'll drop the paragraph as I don't think it brings much.\n> >>>\n> >>>>> + *\n> >>>>> + * \\return The ControlValue with its content replaced with the one of \\a other\n> >>>>> */\n> >>>>> +ControlValue &ControlValue::operator=(const ControlValue &other)\n> >>>>> +{\n> >>>>> + set(other.type_, other.isArray_, other.data().data(),\n> >>>>> + other.numElements_, ControlValueSize[other.type_]);\n> >>>>> + return *this;\n> >>>>> +}\n> >>>>\n> >>>> Do we need/desire move support to just move the allocated storage over\n> >>>> at all ?\n> >>>\n> >>> I think we do, but I think it can be done on top.\n> >>\n> >> Sure\n> >>\n> >>>>>\n> >>>>> /**\n> >>>>> * \\fn ControlValue::type()\n> >>>>> @@ -102,16 +151,33 @@ ControlValue::ControlValue()\n> >>>>> * \\return True if the value type is ControlTypeNone, false otherwise\n> >>>>> */\n> >>>>>\n> >>>>> +/**\n> >>>>> + * \\fn ControlValue::isArray()\n> >>>>> + * \\brief Determine if the value stores an array\n> >>>>> + * \\return True if the value stores an array, false otherwise\n> >>>>> + */\n> >>>>> +\n> >>>>> +/**\n> >>>>> + * \\fn ControlValue::numElements()\n> >>>>> + * \\brief Retrieve the number of elements stored in the ControlValue\n> >>>>> + *\n> >>>>> + * For instances storing an array, this function returns the number of elements\n> >>>>> + * in the array. Otherwise, it returns 1.\n> >>>>> + *\n> >>>>> + * \\return The number of elements stored in the ControlValue\n> >>>>> + */\n> >>>>> +\n> >>>>> /**\n> >>>>> * \\brief Retrieve the raw of a control value\n> >>>>> * \\return The raw data of the control value as a span of uint8_t\n> >>>>> */\n> >>>>> Span ControlValue::data() const\n> >>>>> {\n> >>>>> - return {\n> >>>>> - reinterpret_cast(&bool_),\n> >>>>> - ControlValueSize[type_]\n> >>>>> - };\n> >>>>> + std::size_t size = numElements_ * ControlValueSize[type_];\n> >>>>> + const uint8_t *data = size > sizeof storage_\n> >>>>\n> >>>> sizeof without 'containing' what it parses really looks wrong to me :S\n> >>>\n> >>> I'll update this.\n> >>>\n> >>>>> + ? *reinterpret_cast(&storage_)\n> >>>>> + : reinterpret_cast(&storage_);\n> >>>>> + return { data, size };\n> >>>>\n> >>>> Ahh, at least that looks better than the multiline return statement we\n> >>>> had before :-)\n> >>>>\n> >>>>> }\n> >>>>>\n> >>>>> /**\n> >>>>> @@ -120,18 +186,43 @@ Span ControlValue::data() const\n> >>>>> */\n> >>>>> std::string ControlValue::toString() const\n> >>>>> {\n> >>>>> - switch (type_) {\n> >>>>> - case ControlTypeNone:\n> >>>>> - return \"\";\n> >>>>> - case ControlTypeBool:\n> >>>>> - return bool_ ? \"True\" : \"False\";\n> >>>>> - case ControlTypeInteger32:\n> >>>>> - return std::to_string(integer32_);\n> >>>>> - case ControlTypeInteger64:\n> >>>>> - return std::to_string(integer64_);\n> >>>>> + if (type_ == ControlTypeNone)\n> >>>>> + return \"\";\n> >>>>> +\n> >>>>> + const uint8_t *data = ControlValue::data().data();\n> >>>>> + std::string str(isArray_ ? \"[ \" : \"\");\n> >>>>> +\n> >>>>> + for (unsigned int i = 0; i < numElements_; ++i) {\n> >>>>> + switch (type_) {\n> >>>>> + case ControlTypeBool: {\n> >>>>> + const bool *value = reinterpret_cast(data);\n> >>>>> + str += *value ? \"True\" : \"False\";\n> >>>>> + break;\n> >>>>> + }\n> >>>>> + case ControlTypeInteger32: {\n> >>>>> + const int32_t *value = reinterpret_cast(data);\n> >>>>> + str += std::to_string(*value);\n> >>>>> + break;\n> >>>>> + }\n> >>>>> + case ControlTypeInteger64: {\n> >>>>> + const int64_t *value = reinterpret_cast(data);\n> >>>>> + str += std::to_string(*value);\n> >>>>> + break;\n> >>>>> + }\n> >>>>> + case ControlTypeNone:\n> >>>>> + break;\n> >>>>> + }\n> >>>>> +\n> >>>>> + if (i + 1 != numElements_)\n> >>>>> + str += \", \";\n> >>>>> +\n> >>>>> + data += ControlValueSize[type_];\n> >>>>> }\n> >>>>>\n> >>>>> - return \"\";\n> >>>>> + if (isArray_)\n> >>>>> + str += \" ]\";\n> >>>>> +\n> >>>>> + return str;\n> >>>>\n> >>>> Ohh toString() is neat here :-)\n> >>>>\n> >>>>> }\n> >>>>>\n> >>>>> /**\n> >>>>> @@ -143,16 +234,13 @@ bool ControlValue::operator==(const ControlValue &other) const\n> >>>>> if (type_ != other.type_)\n> >>>>> return false;\n> >>>>>\n> >>>>> - switch (type_) {\n> >>>>> - case ControlTypeBool:\n> >>>>> - return bool_ == other.bool_;\n> >>>>> - case ControlTypeInteger32:\n> >>>>> - return integer32_ == other.integer32_;\n> >>>>> - case ControlTypeInteger64:\n> >>>>> - return integer64_ == other.integer64_;\n> >>>>> - default:\n> >>>>> + if (numElements_ != other.numElements())\n> >>>>> return false;\n> >>>>> - }\n> >>>>> +\n> >>>>> + if (isArray_ != other.isArray_)\n> >>>>> + return false;\n> >>>>> +\n> >>>>> + return memcmp(data().data(), other.data().data(), data().size()) == 0;\n> >>\n> >> Is there some data involved in that code by any chance? :-)\n> > \n> > So you've spotted the subliminal message ;-)\n> > \n> >>>> }\n> >>>>>\n> >>>>> /**\n> >>>>> @@ -165,8 +253,16 @@ bool ControlValue::operator==(const ControlValue\n> >>> &other) const\n> >>>>> * \\fn template T ControlValue::get() const\n> >>>>> * \\brief Get the control value\n> >>>>> *\n> >>>>> - * The control value type shall match the type T, otherwise the behaviour is\n> >>>>> - * undefined.\n> >>>>> + * This function returns the contained value as an instance of \\a T. If the\n> >>>>> + * ControlValue instance stores a single value, the type \\a T shall match the\n> >>>>> + * stored value type(). If the instance stores an array of values, the type\n> >>>>> + * \\a T should be equal to Span, and the type \\a R shall match the\n> >>>>> + * stored value type(). The behaviour is undefined otherwise.\n> >>>>> + *\n> >>>>> + * Note that a ControlValue instance that stores a non-array value is not\n> >>>>> + * equivalent to an instance that stores an array value containing a single\n> >>>>> + * element. The latter shall be accessed through a Span type, while\n> >>>>> + * the former shall be accessed through a type \\a T corresponding to type().\n> >>>>> *\n> >>>>> * \\return The control value\n> >>>>> */\n> >>>>> @@ -175,8 +271,41 @@ bool ControlValue::operator==(const ControlValue &other) const\n> >>>>> * \\fn template void ControlValue::set(const T &value)\n> >>>>> * \\brief Set the control value to \\a value\n> >>>>> * \\param[in] value The control value\n> >>>>> + *\n> >>>>> + * This function stores the \\a value in the instance. If the type \\a T is\n> >>>>> + * equivalent to Span, the instance stores an array of values of type \\a R.\n> >>>>> + * Otherwise the instance stores a single value of type \\a T. The numElements()\n> >>>>> + * and type() are updated to reflect the stored value.\n> >>>>> + *\n> >>>>> + * The entire content of \\a value is copied to the instance, no reference to \\a\n> >>>>> + * value or to the data it references is retained. This may be an expensive\n> >>>>> + * operation for Span<> values that refer to large arrays.\n> >>>>> */\n> >>>>>\n> >>>>> +void ControlValue::set(ControlType type, bool isArray, const void *data,\n> >>>>> + std::size_t numElements, std::size_t elementSize)\n> >>>>> +{\n> >>>>> + ASSERT(elementSize == ControlValueSize[type]);\n> >>>>> +\n> >>>>> + release();\n> >>\n> >> I hadn't noticed this here.\n> >>\n> >> What isn't clear here is that release is conditional on size > storage, so\n> >> i think this would be clearer to perform the delete before new below.\n> > \n> > The purpose of release() is to free storage if it has been allocated, so\n> > that the callers don't need to care. We need to free storage here if it\n> > has been allocated regardless of the new size (and thus regardless of\n> > this set() call will end up allocating memory or using internal\n> > storage).\n> \n> Yes, it's the fact that the 'internal' storage gets 'used' as a pointer,\n> rather than it always being a pointer that I seem to have completely\n> glossed over.\n> \n> Of course if it was always a pointer it would have been declared as such\n> so that maybe should have given the game away. ... maybe I was/am just\n> too tired.\n> \n> Ho hum ... all the more reason to go on holiday next week :-)\n> \n> >>>> +\n> >>>>> + type_ = type;\n> >>>>> + numElements_ = numElements;\n> >>>>> + isArray_ = isArray;\n> >>>>> +\n> >>>>> + std::size_t size = elementSize * numElements;\n> >>>>> + void *storage;\n> >>>>> +\n> >>>>> + if (size > sizeof storage_) {\n> >>>\n> >>> I'll update this one too.\n> >>>\n> >>>>> + storage = reinterpret_cast(new char[size]);\n> >>>>> + *reinterpret_cast(&storage_) = storage;\n> >>>>\n> >>>> Doesn't this need to delete/free any existing storage? Or does the\n> >>>> assignment do that automagically?\n> >>>\n> >>> The release() call above handles it.\n> >>>\n> >>>>> + } else {\n> >>>>> + storage = reinterpret_cast(&storage_);\n> >>\n> >> Because now you've highlighted the release call, this \"looks\" like it's\n> >> setting storage to a \"released\" allocation :-(\n> >>\n> >> And as far as i can tell with a small view on a phone, release() isn't\n> >> going to do anything in the destructor ... Which is a leak...\n> > \n> > In this branch we use the internal storage, so release() doesn't need to\n> > free it. In the above branch we allocate external storage, which will be\n> > freed by release() in the destructor (or when this function is called\n> > again).\n> > \n> \n> Aha so uint64_t storage_ is actually:\n> \n> union {\n> uint64_t int64; /* Store Control Values which fit directly */\n> void *ptr; /* External allocation required */\n> }\n> \n> Where it will either store the data in an int64 or use storage_ as a\n> pointer to allocated memory.\n> \n> I completely misinterpreted it on first read as it only being only\n> essentially a void *ptr (or I guess by that definition it's a uintptr_t?)\n> \n> >> Could you verify that please and maybe simplify the code to make sure it's\n> >> clear?\n> >>\n> >>>> + }\n> >>>>> +\n> >>>>> + memcpy(storage, data, size);\n> >>>>> +}\n> >>>>> +\n> >>>>> /**\n> >>>>> * \\class ControlId\n> >>>>> * \\brief Control static metadata","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1C10560424\n\tfor ;\n\tSun, 8 Mar 2020 00:20:46 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6BE735F;\n\tSun, 8 Mar 2020 00:20:45 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1583623245;\n\tbh=kemSH21cvGYi/cvvyWhsHbMaV7Yh+4VLEHIWinGrZg4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UVt4dKtWnDYmUPGEXDSvIXCAJcSOKxF6K7Iu5KpQp3zdQ+56ompmSGCFfwboWGA9Z\n\tiXLCxPzB52mnbTJwMv1lZe0jWTXaVTildLmfd/F2aoW45XvO14Qnthgj9ejQZgCIMe\n\tL+u5RA0qBbrvhJMTVmn7gojgrmKLdPXwKntZjHBI=","Date":"Sun, 8 Mar 2020 01:20:40 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Kieran Bingham ,\n\tlibcamera devel ","Message-ID":"<20200307232040.GA31018@pendragon.ideasonboard.com>","References":"<20200229164254.23604-17-laurent.pinchart@ideasonboard.com>\n\t<20200301175454.10921-1-laurent.pinchart@ideasonboard.com>\n\t<5b0e6fd3-7600-5910-118c-18d37b5f6872@ideasonboard.com>\n\t<20200306155556.GP4878@pendragon.ideasonboard.com>\n\t\n\t<20200307181731.GA5021@pendragon.ideasonboard.com>\n\t<40a7fcf2-33aa-098c-15d2-ff0798400302@bingham.xyz>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<40a7fcf2-33aa-098c-15d2-ff0798400302@bingham.xyz>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v1.2 16/31] libcamera: controls:\n\tSupport array controls in ControlValue","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sat, 07 Mar 2020 23:20:46 -0000"}},{"id":3999,"web_url":"https://patchwork.libcamera.org/comment/3999/","msgid":"<296a1240-80a8-3a19-160b-100f197a652c@bingham.xyz>","date":"2020-03-07T23:28:59","subject":"Re: [libcamera-devel] [PATCH v1.2 16/31] libcamera: controls:\n\tSupport array controls in ControlValue","submitter":{"id":38,"url":"https://patchwork.libcamera.org/api/people/38/","name":"Kieran Bingham","email":"kieran@bingham.xyz"},"content":"Hi Laurent,\n\nOn 07/03/2020 23:20, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Sat, Mar 07, 2020 at 10:07:37PM +0000, Kieran Bingham wrote:\n>> On 07/03/2020 18:17, Laurent Pinchart wrote:\n>>> On Sat, Mar 07, 2020 at 06:04:23PM +0000, Kieran Bingham wrote:\n>>>> On Fri, 6 Mar 2020, 15:56 Laurent Pinchart wrote:\n>>>>> On Tue, Mar 03, 2020 at 12:23:10AM +0000, Kieran Bingham wrote:\n>>>>>> On 01/03/2020 17:54, Laurent Pinchart wrote:\n>>>>>>> From: Jacopo Mondi \n>>>>>>>\n>>>>>>> Add array controls support to the ControlValue class. The polymorphic\n>>>>>>> class can now store more than a single element and supports access and\n>>>>>>> creation through the use of Span<>.\n>>>>>>\n>>>>>> Oh, but if the creation is through a span, what stores the data in the\n>>>>>> ControlValue ... I guess I'll see below... \n>>>>>>\n>>>>>>> Signed-off-by: Jacopo Mondi \n>>>>>>> Signed-off-by: Laurent Pinchart \n>>>>>>\n>>>>>> Some comments, but nothing you can't handle...\n>>>>>>\n>>\n>> Continued ramblings below are unfortunately out-of-order due to replying\n>> in situ on existing comments but the general message is:\n>>\n>> Ok - so no leak like I feared, and except the fact that I misinterpreted\n>> the definition/usage of storage_ which might need some comment to make\n>> it clearer (or might not because I might just be overtired), the\n>> following still stands ... and I don't think there's any blocker on this\n>> series :-)\n>>\n>>>>>> Reviewed-by: Kieran Bingham \n>>>>>>\n>>>>>>> ---\n>>>>>>> Changes since v1:\n>>>>>>>\n>>>>>>> - Use T::value_type instead of T::element_type to benefit from\n>>>>>>> std::remove_cv\n>>>>>>> - Fix ControlTypeNone test in ControlValue::toString()\n>>>>>>> - Separate array elements with \", \" in ControlValue::toString()\n>>>>>>> ---\n>>>>>>> include/libcamera/controls.h | 81 ++++++++++++---\n>>>>>>> src/libcamera/controls.cpp | 185 +++++++++++++++++++++++++++++------\n>>>>>>> 2 files changed, 225 insertions(+), 41 deletions(-)\n>>>>>>>\n>>>>>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n>>>>>>> index 4538be06af93..1e24ae30ab36 100644\n>>>>>>> --- a/include/libcamera/controls.h\n>>>>>>> +++ b/include/libcamera/controls.h\n>>>>>>> @@ -9,6 +9,7 @@\n>>>>>>> #define __LIBCAMERA_CONTROLS_H__\n>>>>>>>\n>>>>>>> #include \n>>>>>>> +#include \n>>>>>>> #include \n>>>>>>> #include \n>>>>>>>\n>>>>>>> @@ -51,6 +52,10 @@ struct control_type {\n>>>>>>> static constexpr ControlType value = ControlTypeInteger64;\n>>>>>>> };\n>>>>>>>\n>>>>>>> +template\n>>>>>>> +struct control_type> : public control_type> {\n>>>>>>> +};\n>>>>>>> +\n>>>>>>> } /* namespace details */\n>>>>>>>\n>>>>>>> class ControlValue\n>>>>>>> @@ -58,15 +63,35 @@ class ControlValue\n>>>>>>> public:\n>>>>>>> ControlValue();\n>>>>>>>\n>>>>>>> +#ifndef __DOXYGEN__\n>>>>>>> + template::value, std::nullptr_t> = nullptr>\n>>>>>>> + ControlValue(const T &value)\n>>>>>>> + : type_(ControlTypeNone), numElements_(0)\n>>>>>>> + {\n>>>>>>> + set(details::control_type>::value, false,\n>>>>>>> + &value, 1, sizeof(T));\n>>>>>>> + }\n>>>>>>> +\n>>>>>>> + template::value, std::nullptr_t> = nullptr>\n>>>>>>> +#else\n>>>>>>> template\n>>>>>>> - ControlValue(T value)\n>>>>>>> - : type_(details::control_type>::value)\n>>>>>>> +#endif\n>>>>>>\n>>>>>> That #iffery is pretty horrible, removes one function and changes the\n>>>>>> template instantiation of this below function ?\n>>>>>>\n>>>>>> Though seeing the repeating pattern below too - I can't offer a better\n>>>>>> solution...\n>>>>>>\n>>>>>>> + ControlValue(const T &value)\n>>>>>>> + : type_(ControlTypeNone), numElements_(0)\n>>>>>>> {\n>>>>>>> - *reinterpret_cast(&bool_) = value;\n>>>>>>> + set(details::control_type>::value, true>,\n>>>>>>> + value.data(), value.size(), sizeof(typename T::value_type));\n>>>>>>\n>>>>>> What's true here ? This is not very friendly to read...\n>>>>>\n>>>>> Should I add\n>>>>>\n>>>>> enum {\n>>>>> ControlIsSingle = 1,\n>>>>> ControlIsArray = 1,\n>>>>> };\n>>>>>\n>>>>> ? I don't like the name ControlIsSingle though. Maybe this could be done\n>>>>> on top, if you think it's worth it ? If you can propose a better name\n>>>>> I'll submit a patch :-)\n>>>>>\n>>>>>> Ok - so on the plus side the bool_ is gone :-)\n>>>>>>\n>>>>>>> }\n>>>>>>>\n>>>>>>> + ~ControlValue();\n>>>>>>> +\n>>>>>>> + ControlValue(const ControlValue &other);\n>>>>>>> + ControlValue &operator=(const ControlValue &other);\n>>>>>>> +\n>>>>>>> ControlType type() const { return type_; }\n>>>>>>> bool isNone() const { return type_ == ControlTypeNone; }\n>>>>>>> + bool isArray() const { return isArray_; }\n>>>>>>> + std::size_t numElements() const { return numElements_; }\n>>>>>>> Span data() const;\n>>>>>>>\n>>>>>>> std::string toString() const;\n>>>>>>> @@ -77,31 +102,61 @@ public:\n>>>>>>> return !(*this == other);\n>>>>>>> }\n>>>>>>>\n>>>>>>> +#ifndef __DOXYGEN__\n>>>>>>> + template::value, std::nullptr_t> = nullptr>\n>>>>>>> + T get() const\n>>>>>>> + {\n>>>>>>> + assert(type_ == details::control_type>::value);\n>>>>>>> + assert(!isArray_);\n>>>>>>> +\n>>>>>>> + return *reinterpret_cast(data().data());\n>>>>>>> + }\n>>>>>>> +\n>>>>>>> + template::value, std::nullptr_t> = nullptr>\n>>>>>>> +#else\n>>>>>>> template\n>>>>>>> +#endif\n>>>>>>> T get() const\n>>>>>>> {\n>>>>>>> assert(type_ == details::control_type>::value);\n>>>>>>> + assert(isArray_);\n>>>>>>> +\n>>>>>>> + using V = typename T::value_type;\n>>>>>>> + const V *value = reinterpret_cast(data().data());\n>>>>>>> + return { value, numElements_ };\n>>>>>>> + }\n>>>>>>>\n>>>>>>> - return *reinterpret_cast(&bool_);\n>>>>>>> +#ifndef __DOXYGEN__\n>>>>>>> + template::value, std::nullptr_t> = nullptr>\n>>>>>>> + void set(const T &value)\n>>>>>>> + {\n>>>>>>> + set(details::control_type>::value, false,\n>>>>>>> + reinterpret_cast(&value), 1, sizeof(T));\n>>>>>>> }\n>>>>>>>\n>>>>>>> + template::value, std::nullptr_t> = nullptr>\n>>>>>>> +#else\n>>>>>>> template\n>>>>>>> +#endif\n>>>>>>> void set(const T &value)\n>>>>>>> {\n>>>>>>> - type_ = details::control_type>::value;\n>>>>>>> - *reinterpret_cast(&bool_) = value;\n>>>>>>> + set(details::control_type>::value, true,\n>>>>>>> + value.data(), value.size(), sizeof(typename T::value_type));\n>>>>>>> }\n>>>>>>>\n>>>>>>> private:\n>>>>>>> - ControlType type_;\n>>>>>>> -\n>>>>>>> - union {\n>>>>>>> - bool bool_;\n>>>>>>> - int32_t integer32_;\n>>>>>>> - int64_t integer64_;\n>>>>>>> - };\n>>>>>>> + ControlType type_ : 8;\n>>>>>>> + bool isArray_ : 1;\n>>>>>>> + std::size_t numElements_ : 16;\n>>>>>>> + uint64_t storage_;\n>>>>>>> +\n>>>>>>> + void release();\n>>>>>>> + void set(ControlType type, bool isArray, const void *data,\n>>>>>>> + std::size_t numElements, std::size_t elementSize);\n>>>>>>> };\n>>>>>>>\n>>>>>>> +static_assert(sizeof(ControlValue) == 16, \"Invalid size of ControlValue class\");\n>>>>>>\n>>>>>> Aha, I knew this ASSERT_ABI_SIZE would come up again :-)\n>>>>>\n>>>>> I think it's a good idea, and I think we should actually try to\n>>>>> automate that through all our classes, to have automatic ABI regression\n>>>>> tests. I envision that as being done through code analysis instead of\n>>>>> static_assert(). And I wouldn't be surprised if tools already existed\n>>>>> for this purpose.\n>>>>>\n>>>>>>> +\n>>>>>>> class ControlId\n>>>>>>> {\n>>>>>>> public:\n>>>>>>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n>>>>>>> index b2331ab7540d..a5a385aa1b0a 100644\n>>>>>>> --- a/src/libcamera/controls.cpp\n>>>>>>> +++ b/src/libcamera/controls.cpp\n>>>>>>> @@ -10,6 +10,7 @@\n>>>>>>> #include \n>>>>>>> #include \n>>>>>>> #include \n>>>>>>> +#include \n>>>>>>>\n>>>>>>> #include \"control_validator.h\"\n>>>>>>> #include \"log.h\"\n>>>>>>> @@ -50,7 +51,7 @@ LOG_DEFINE_CATEGORY(Controls)\n>>>>>>> namespace {\n>>>>>>>\n>>>>>>> static constexpr size_t ControlValueSize[] = {\n>>>>>>> - [ControlTypeNone] = 1,\n>>>>>>> + [ControlTypeNone] = 0,\n>>>>>>> [ControlTypeBool] = sizeof(bool),\n>>>>>>> [ControlTypeInteger32] = sizeof(int32_t),\n>>>>>>> [ControlTypeInteger64] = sizeof(int64_t),\n>>>>>>> @@ -80,15 +81,63 @@ static constexpr size_t ControlValueSize[] = {\n>>>>>>> * \\brief Construct an empty ControlValue.\n>>>>>>> */\n>>>>>>> ControlValue::ControlValue()\n>>>>>>> - : type_(ControlTypeNone)\n>>>>>>> + : type_(ControlTypeNone), isArray_(false), numElements_(0)\n>>>>>>> {\n>>>>>>> }\n>>>>>>>\n>>>>>>> /**\n>>>>>>> - * \\fn template T ControlValue::ControlValue(T value)\n>>>>>>> + * \\fn template T ControlValue::ControlValue(const T &value)\n>>>>>>> * \\brief Construct a ControlValue of type T\n>>>>>>> * \\param[in] value Initial value\n>>>>>>> + *\n>>>>>>> + * This function constructs a new instance of ControlValue and stores the \\a\n>>>>>>> + * value inside it. If the type \\a T is equivalent to Span, the instance\n>>>>>>> + * stores an array of values of type \\a R. Otherwise the instance stores a\n>>>>>>> + * single value of type \\a T. The numElements() and type() are updated to\n>>>>>>> + * reflect the stored value.\n>>>>>>> + */\n>>>>>>> +\n>>>>>>> +void ControlValue::release()\n>>>>>>> +{\n>>>>>>> + std::size_t size = numElements_ * ControlValueSize[type_];\n>>>>>>> +\n>>>>>>> + if (size > sizeof storage_) {\n>>>>>>\n>>>>>> sizeof(storage) would read nicer to me here. ...\n>>>>>\n>>>>> sizeof is an operator and doesn't require parentheses:\n>>>>> https://en.cppreference.com/w/cpp/language/sizeof. I'll change it\n>>>>> though.\n>>>>>\n>>>>>> Oh ... uhm isn't storage a uint64_t? And therefore always 8?\n>>\n>> Ah, my (incorrect) inference here was that it was supposed to not always\n>> be 8...\n>>\n>>>>>\n>>>>> Correct, but that's better than hardcoding the value 8, right ? :-)\n>>>>\n>>>> Indeed :-)\n>>\n>>\n>> And now I read that again it clears up my earlier confusion. I had seen\n>> that storage_ was being used as a pointer, and thus was expecting the\n>> sizeofs to be determining the size of the available memory to store.\n>>\n>> It's the fact that this > sizeof(storage_) determines if the storage_ is\n>> used as those bytes directly or as a pointer to an allocation.\n>>\n>> It seems that's easy to miss - and can cause confusion. Have I missed\n>> the documentation or comments that clears that up? If not - could\n>> something be added please?\n> \n> Does \"[PATCH] libcamera: controls: Fix strict aliasing violation\" help ?\n> I can also add more documentation on top.\n\nAha! :-)\n\nI don't feel so bad now - I thought it was my just my head being tired\nbut if the compiler complains too :-D\n\nYes - that's fine - I initially thought the use of a union might be\noverkill, but it clearly marks the duality of use on that memory location.\n\nPresumably that could be squashed in here, or applied on top.\nI'm fine with either option, it's up to you.\n\n--\nKieran\n\n\n> \n>>>>>>> + delete[] *reinterpret_cast(&storage_);\n>>>>>>> + storage_ = 0;\n>>>>>>> + }\n>>>>>>> +}\n>>>>>>> +\n>>>>>>> +ControlValue::~ControlValue()\n>>>>>>> +{\n>>>>>>> + release();\n>>>>\n>>>> Who deletes storage_ on destruct?\n>>>\n>>> release() does.\n>>>\n>>>> Release only appears to delete in the event that size is bigger than the\n>>>> storage available to help the set() call?\n>>>\n>>> release() deletes the storage if it has been allocated. We allocate\n>>> external storage in case the data won't fit in the internal storage\n>>\n>> Ahhhhhhhh (that's both an 'Aha, and an Argghhh' in one)\n>>\n>> Even here where you call it 'internal' storage got confusing. I suddenly\n>> thought - oh there's some other storage being pointed to somewehre :-(\n>>\n>> If that was a 'union' the code would have been self documenting - but\n>> because it wasn't .. I got confused.\n>>\n>> I'm not necessarily suggesting turn it into a union, as that might just\n>> be adding lines for no real gain - but a comment somewhere to directly\n>> state how it's used could help.\n>>\n>>> (size > 8, see the set() function below), and delete it in release()\n>>> using the same condition.\n>>>\n>>>> That's making the reallocation below a bit ugly only to provide a function\n>>>> that doesn't do anything to the destructor?\n>>>>\n>>>> (Doesn't do anything; based on assumption that after the object is used it\n>>>> has an allocation which is of suitable size already)\n>>>>\n>>>>>> +}\n>>>>>>> +\n>>>>>>> +/**\n>>>>>>> + * \\brief Contruct a ControlValue with the content of \\a other\n>>>>>>\n>>>>>> /Contruct/Construct/\n>>>>>>\n>>>>>>> + * \\param[in] other The ControlValue to copy content from\n>>>>>>> + */\n>>>>>>> +ControlValue::ControlValue(const ControlValue &other)\n>>>>>>> + : type_(ControlTypeNone), numElements_(0)\n>>>>>>> +{\n>>>>>>> + *this = other;\n>>>>>>> +}\n>>>>>>> +\n>>>>>>> +/**\n>>>>>>> + * \\brief Replace the content of the ControlValue with the one of \\a other\n>>>>>>> + * \\param[in] other The ControlValue to copy content from\n>>>>>>> + *\n>>>>>>> + * Deep-copy the content of \\a other into the ControlValue by reserving memory\n>>>>>>> + * and copy data there in case \\a other transports arrays of values in one of\n>>>>>>> + * its pointer data members.\n>>>>>>\n>>>>>> That's hard to parse...\n>>>>>\n>>>>> Agreed. I'll drop the paragraph as I don't think it brings much.\n>>>>>\n>>>>>>> + *\n>>>>>>> + * \\return The ControlValue with its content replaced with the one of \\a other\n>>>>>>> */\n>>>>>>> +ControlValue &ControlValue::operator=(const ControlValue &other)\n>>>>>>> +{\n>>>>>>> + set(other.type_, other.isArray_, other.data().data(),\n>>>>>>> + other.numElements_, ControlValueSize[other.type_]);\n>>>>>>> + return *this;\n>>>>>>> +}\n>>>>>>\n>>>>>> Do we need/desire move support to just move the allocated storage over\n>>>>>> at all ?\n>>>>>\n>>>>> I think we do, but I think it can be done on top.\n>>>>\n>>>> Sure\n>>>>\n>>>>>>>\n>>>>>>> /**\n>>>>>>> * \\fn ControlValue::type()\n>>>>>>> @@ -102,16 +151,33 @@ ControlValue::ControlValue()\n>>>>>>> * \\return True if the value type is ControlTypeNone, false otherwise\n>>>>>>> */\n>>>>>>>\n>>>>>>> +/**\n>>>>>>> + * \\fn ControlValue::isArray()\n>>>>>>> + * \\brief Determine if the value stores an array\n>>>>>>> + * \\return True if the value stores an array, false otherwise\n>>>>>>> + */\n>>>>>>> +\n>>>>>>> +/**\n>>>>>>> + * \\fn ControlValue::numElements()\n>>>>>>> + * \\brief Retrieve the number of elements stored in the ControlValue\n>>>>>>> + *\n>>>>>>> + * For instances storing an array, this function returns the number of elements\n>>>>>>> + * in the array. Otherwise, it returns 1.\n>>>>>>> + *\n>>>>>>> + * \\return The number of elements stored in the ControlValue\n>>>>>>> + */\n>>>>>>> +\n>>>>>>> /**\n>>>>>>> * \\brief Retrieve the raw of a control value\n>>>>>>> * \\return The raw data of the control value as a span of uint8_t\n>>>>>>> */\n>>>>>>> Span ControlValue::data() const\n>>>>>>> {\n>>>>>>> - return {\n>>>>>>> - reinterpret_cast(&bool_),\n>>>>>>> - ControlValueSize[type_]\n>>>>>>> - };\n>>>>>>> + std::size_t size = numElements_ * ControlValueSize[type_];\n>>>>>>> + const uint8_t *data = size > sizeof storage_\n>>>>>>\n>>>>>> sizeof without 'containing' what it parses really looks wrong to me :S\n>>>>>\n>>>>> I'll update this.\n>>>>>\n>>>>>>> + ? *reinterpret_cast(&storage_)\n>>>>>>> + : reinterpret_cast(&storage_);\n>>>>>>> + return { data, size };\n>>>>>>\n>>>>>> Ahh, at least that looks better than the multiline return statement we\n>>>>>> had before :-)\n>>>>>>\n>>>>>>> }\n>>>>>>>\n>>>>>>> /**\n>>>>>>> @@ -120,18 +186,43 @@ Span ControlValue::data() const\n>>>>>>> */\n>>>>>>> std::string ControlValue::toString() const\n>>>>>>> {\n>>>>>>> - switch (type_) {\n>>>>>>> - case ControlTypeNone:\n>>>>>>> - return \"\";\n>>>>>>> - case ControlTypeBool:\n>>>>>>> - return bool_ ? \"True\" : \"False\";\n>>>>>>> - case ControlTypeInteger32:\n>>>>>>> - return std::to_string(integer32_);\n>>>>>>> - case ControlTypeInteger64:\n>>>>>>> - return std::to_string(integer64_);\n>>>>>>> + if (type_ == ControlTypeNone)\n>>>>>>> + return \"\";\n>>>>>>> +\n>>>>>>> + const uint8_t *data = ControlValue::data().data();\n>>>>>>> + std::string str(isArray_ ? \"[ \" : \"\");\n>>>>>>> +\n>>>>>>> + for (unsigned int i = 0; i < numElements_; ++i) {\n>>>>>>> + switch (type_) {\n>>>>>>> + case ControlTypeBool: {\n>>>>>>> + const bool *value = reinterpret_cast(data);\n>>>>>>> + str += *value ? \"True\" : \"False\";\n>>>>>>> + break;\n>>>>>>> + }\n>>>>>>> + case ControlTypeInteger32: {\n>>>>>>> + const int32_t *value = reinterpret_cast(data);\n>>>>>>> + str += std::to_string(*value);\n>>>>>>> + break;\n>>>>>>> + }\n>>>>>>> + case ControlTypeInteger64: {\n>>>>>>> + const int64_t *value = reinterpret_cast(data);\n>>>>>>> + str += std::to_string(*value);\n>>>>>>> + break;\n>>>>>>> + }\n>>>>>>> + case ControlTypeNone:\n>>>>>>> + break;\n>>>>>>> + }\n>>>>>>> +\n>>>>>>> + if (i + 1 != numElements_)\n>>>>>>> + str += \", \";\n>>>>>>> +\n>>>>>>> + data += ControlValueSize[type_];\n>>>>>>> }\n>>>>>>>\n>>>>>>> - return \"\";\n>>>>>>> + if (isArray_)\n>>>>>>> + str += \" ]\";\n>>>>>>> +\n>>>>>>> + return str;\n>>>>>>\n>>>>>> Ohh toString() is neat here :-)\n>>>>>>\n>>>>>>> }\n>>>>>>>\n>>>>>>> /**\n>>>>>>> @@ -143,16 +234,13 @@ bool ControlValue::operator==(const ControlValue &other) const\n>>>>>>> if (type_ != other.type_)\n>>>>>>> return false;\n>>>>>>>\n>>>>>>> - switch (type_) {\n>>>>>>> - case ControlTypeBool:\n>>>>>>> - return bool_ == other.bool_;\n>>>>>>> - case ControlTypeInteger32:\n>>>>>>> - return integer32_ == other.integer32_;\n>>>>>>> - case ControlTypeInteger64:\n>>>>>>> - return integer64_ == other.integer64_;\n>>>>>>> - default:\n>>>>>>> + if (numElements_ != other.numElements())\n>>>>>>> return false;\n>>>>>>> - }\n>>>>>>> +\n>>>>>>> + if (isArray_ != other.isArray_)\n>>>>>>> + return false;\n>>>>>>> +\n>>>>>>> + return memcmp(data().data(), other.data().data(), data().size()) == 0;\n>>>>\n>>>> Is there some data involved in that code by any chance? :-)\n>>>\n>>> So you've spotted the subliminal message ;-)\n>>>\n>>>>>> }\n>>>>>>>\n>>>>>>> /**\n>>>>>>> @@ -165,8 +253,16 @@ bool ControlValue::operator==(const ControlValue\n>>>>> &other) const\n>>>>>>> * \\fn template T ControlValue::get() const\n>>>>>>> * \\brief Get the control value\n>>>>>>> *\n>>>>>>> - * The control value type shall match the type T, otherwise the behaviour is\n>>>>>>> - * undefined.\n>>>>>>> + * This function returns the contained value as an instance of \\a T. If the\n>>>>>>> + * ControlValue instance stores a single value, the type \\a T shall match the\n>>>>>>> + * stored value type(). If the instance stores an array of values, the type\n>>>>>>> + * \\a T should be equal to Span, and the type \\a R shall match the\n>>>>>>> + * stored value type(). The behaviour is undefined otherwise.\n>>>>>>> + *\n>>>>>>> + * Note that a ControlValue instance that stores a non-array value is not\n>>>>>>> + * equivalent to an instance that stores an array value containing a single\n>>>>>>> + * element. The latter shall be accessed through a Span type, while\n>>>>>>> + * the former shall be accessed through a type \\a T corresponding to type().\n>>>>>>> *\n>>>>>>> * \\return The control value\n>>>>>>> */\n>>>>>>> @@ -175,8 +271,41 @@ bool ControlValue::operator==(const ControlValue &other) const\n>>>>>>> * \\fn template void ControlValue::set(const T &value)\n>>>>>>> * \\brief Set the control value to \\a value\n>>>>>>> * \\param[in] value The control value\n>>>>>>> + *\n>>>>>>> + * This function stores the \\a value in the instance. If the type \\a T is\n>>>>>>> + * equivalent to Span, the instance stores an array of values of type \\a R.\n>>>>>>> + * Otherwise the instance stores a single value of type \\a T. The numElements()\n>>>>>>> + * and type() are updated to reflect the stored value.\n>>>>>>> + *\n>>>>>>> + * The entire content of \\a value is copied to the instance, no reference to \\a\n>>>>>>> + * value or to the data it references is retained. This may be an expensive\n>>>>>>> + * operation for Span<> values that refer to large arrays.\n>>>>>>> */\n>>>>>>>\n>>>>>>> +void ControlValue::set(ControlType type, bool isArray, const void *data,\n>>>>>>> + std::size_t numElements, std::size_t elementSize)\n>>>>>>> +{\n>>>>>>> + ASSERT(elementSize == ControlValueSize[type]);\n>>>>>>> +\n>>>>>>> + release();\n>>>>\n>>>> I hadn't noticed this here.\n>>>>\n>>>> What isn't clear here is that release is conditional on size > storage, so\n>>>> i think this would be clearer to perform the delete before new below.\n>>>\n>>> The purpose of release() is to free storage if it has been allocated, so\n>>> that the callers don't need to care. We need to free storage here if it\n>>> has been allocated regardless of the new size (and thus regardless of\n>>> this set() call will end up allocating memory or using internal\n>>> storage).\n>>\n>> Yes, it's the fact that the 'internal' storage gets 'used' as a pointer,\n>> rather than it always being a pointer that I seem to have completely\n>> glossed over.\n>>\n>> Of course if it was always a pointer it would have been declared as such\n>> so that maybe should have given the game away. ... maybe I was/am just\n>> too tired.\n>>\n>> Ho hum ... all the more reason to go on holiday next week :-)\n>>\n>>>>>> +\n>>>>>>> + type_ = type;\n>>>>>>> + numElements_ = numElements;\n>>>>>>> + isArray_ = isArray;\n>>>>>>> +\n>>>>>>> + std::size_t size = elementSize * numElements;\n>>>>>>> + void *storage;\n>>>>>>> +\n>>>>>>> + if (size > sizeof storage_) {\n>>>>>\n>>>>> I'll update this one too.\n>>>>>\n>>>>>>> + storage = reinterpret_cast(new char[size]);\n>>>>>>> + *reinterpret_cast(&storage_) = storage;\n>>>>>>\n>>>>>> Doesn't this need to delete/free any existing storage? Or does the\n>>>>>> assignment do that automagically?\n>>>>>\n>>>>> The release() call above handles it.\n>>>>>\n>>>>>>> + } else {\n>>>>>>> + storage = reinterpret_cast(&storage_);\n>>>>\n>>>> Because now you've highlighted the release call, this \"looks\" like it's\n>>>> setting storage to a \"released\" allocation :-(\n>>>>\n>>>> And as far as i can tell with a small view on a phone, release() isn't\n>>>> going to do anything in the destructor ... Which is a leak...\n>>>\n>>> In this branch we use the internal storage, so release() doesn't need to\n>>> free it. In the above branch we allocate external storage, which will be\n>>> freed by release() in the destructor (or when this function is called\n>>> again).\n>>>\n>>\n>> Aha so uint64_t storage_ is actually:\n>>\n>> union {\n>> uint64_t int64; /* Store Control Values which fit directly */\n>> void *ptr; /* External allocation required */\n>> }\n>>\n>> Where it will either store the data in an int64 or use storage_ as a\n>> pointer to allocated memory.\n>>\n>> I completely misinterpreted it on first read as it only being only\n>> essentially a void *ptr (or I guess by that definition it's a uintptr_t?)\n>>\n>>>> Could you verify that please and maybe simplify the code to make sure it's\n>>>> clear?\n>>>>\n>>>>>> + }\n>>>>>>> +\n>>>>>>> + memcpy(storage, data, size);\n>>>>>>> +}\n>>>>>>> +\n>>>>>>> /**\n>>>>>>> * \\class ControlId\n>>>>>>> * \\brief Control static metadata\n>","headers":{"Return-Path":"","Received":["from mail-wm1-x332.google.com (mail-wm1-x332.google.com\n\t[IPv6:2a00:1450:4864:20::332])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 73E3B60424\n\tfor ;\n\tSun, 8 Mar 2020 00:29:05 +0100 (CET)","by mail-wm1-x332.google.com with SMTP id g62so1597976wme.1\n\tfor ;\n\tSat, 07 Mar 2020 15:29:05 -0800 (PST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net. [86.31.129.233])\n\tby smtp.gmail.com with ESMTPSA id\n\ti204sm19544567wma.44.2020.03.07.15.28.59\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tSat, 07 Mar 2020 15:29:04 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=bingham-xyz.20150623.gappssmtp.com; s=20150623;\n\th=subject:to:cc:references:from:openpgp:autocrypt:message-id:date\n\t:user-agent:mime-version:in-reply-to:content-language\n\t:content-transfer-encoding;\n\tbh=18Rl/2x3/p/Z+ojhl4KjKvwGY0gt0KCFxi272ksN9Tc=;\n\tb=yBC7MGdE21OAlCUOugiSxNsi8WOyLCapUAYn3WIdrCEAJkcMThakAO6yqq15UEAW89\n\tI30T+zrWCqbwRBmxanqQo5sI+pDDOPm91lF12F4ha87ibU0twsCjRqP7bv7LpQhGHCL+\n\tm3df4U4REVD9hvOqbkzgFT+G3Mu+b2wf+qHCqPjAY631vqsouuQEbrzVQa/AJVzR8LOT\n\tc0P+7bI4+123IKttIg0vbVimsKIawXCoVLh8IX+eWovRibuMDyVKNf/Y4r1JWETEr/lo\n\tZ3EUDh11ryftoXcTAgb+Ykx6+qaJCRgdUMc53LSBgk2G2qUacpoxzx1gO7CzkXOsQ9Or\n\tbdMQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:subject:to:cc:references:from:openpgp:autocrypt\n\t:message-id:date:user-agent:mime-version:in-reply-to\n\t:content-language:content-transfer-encoding;\n\tbh=18Rl/2x3/p/Z+ojhl4KjKvwGY0gt0KCFxi272ksN9Tc=;\n\tb=LIOylm2oiCpblHW5OVedlfzle1YXSvIjrS8GQbsoXa8Tg90L3KAElzGlAOd5dapfu9\n\tV5EQRXU1v+nzqUI2CVqMTOtBuRBVAl3ZOf1vRefwO7vRUUg9gGEE3/zpQk9wkk/P4Lit\n\tvr7t7sKHJlzaG39HXkUiPS5N0P86wXEKVhkP0bQmWaLOT2mjPA0v0XFFNNLop3AKiBSh\n\tHRi3MzQgKPkKLLNjxs9gHjo86x6s/HR6qWacSpj8reekW9e1PROsRjRUCmctuCL/iYYA\n\tfFsGn7D7rQbBgYb6tpYWcw9jx/g2hxElU51w+/yB+Jx3TbGijTP4LAdZJkIS0AK3uXC+\n\t5kCQ==","X-Gm-Message-State":"ANhLgQ2NWrHNeYtPGBdZUfQ4UUopVjcudEU+Zigk2nnq36ZFVr8OljYZ\n\tvqie2YwkqHO/YDxNH+ZbGqaipg==","X-Google-Smtp-Source":"ADFU+vundm6E/Yt8osRCtEAdbKrPKw222Q/wbO2zrFhkF3/D3NkxGBT2Hy64IAXPFJxSElqt+pqSMw==","X-Received":"by 2002:a05:600c:2951:: with SMTP id\n\tn17mr11150782wmd.97.1583623744677; \n\tSat, 07 Mar 2020 15:29:04 -0800 (PST)","To":"Laurent Pinchart ","Cc":"Kieran Bingham ,\n\tlibcamera devel ","References":"<20200229164254.23604-17-laurent.pinchart@ideasonboard.com>\n\t<20200301175454.10921-1-laurent.pinchart@ideasonboard.com>\n\t<5b0e6fd3-7600-5910-118c-18d37b5f6872@ideasonboard.com>\n\t<20200306155556.GP4878@pendragon.ideasonboard.com>\n\t\n\t<20200307181731.GA5021@pendragon.ideasonboard.com>\n\t<40a7fcf2-33aa-098c-15d2-ff0798400302@bingham.xyz>\n\t<20200307232040.GA31018@pendragon.ideasonboard.com>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran@bingham.xyz; keydata=\n\tmQINBFMtlTkBEADvhPl7usumM98GeJgEv0R+atr1fwfMtV2pkpqkTc7RrO+VKc++WDDXGqWG\n\twnNX0FzJ7/TEJoO5BZ+VyHqB1kMjxAckmCKQIrj2/UxkZ/R5lxKzvbve7XDvihnTgQrZv3bw\n\t52Tz81DMTFG+N0yeUOZWnq+mPoNCf9OnkKkPnyWVPdtYeLJmi2oE5ql7/ZEBU6m0BAzRKYny\n\tk69pyQO1zzTb3U6GHGEUc+8CgGolqBQ63qp+MmaQYlA2ytOw8DMiBLJZipVUWS/WgvCvIWkH\n\tlVoI4r8cBSgN4pgRJEKeVXVw+uY8xAbOU3r2y/MfyykzJn99oiaHeNer39EIVRdxKnazYw95\n\tq/RE6dtbroSGcAfa7hIqfqya5nTGzONbxNPdUaWpj3vkej/o5aESXcRk98fH+XCKlS+a/tri\n\t7dfq3/Daoq0LR3wmHvEXN8p52NQlbMCnfEhE+haSLqLEgxTqCMpBt4cgwaW9CmKW8pR91oXF\n\tkIDVY9e/VU9tw3IuoHVK5JXmZeaUe1wLmot2oiq2hmuRonQNGEYWqU6lnoDHTQArLfZPaT9Y\n\thQqf9C7faWF/VvEwXYYquWOX+waY8YPyH16hycmWoePM+oMpIG+04lpjEefSHDUvOciC0p1o\n\tCfePg3iVEKB56V0j9nMAfTr/5oOvTP5EeHHvT6a5ZcGanJYmbQARAQABtCNLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuQGJpbmdoYW0ueHl6PokCVQQTAQoAPwIbAwYLCQgHAwIGFQgCCQoLBBYC\n\tAwECHgECF4AWIQSor+z47OVRZQR+u5Yjhj3Dgx2ysQUCXWTt6gUJDfm/sQAKCRAjhj3Dgx2y\n\tsXNuEACOOFM9Kwq1U8a1hC57HCD37GAcwPXEe5+elO6ORGALzjjHmq9GJf3FbIuV9b0pzyGU\n\tXsNiZKqxmFga9+FocN28REHzKp5eo9/5yFcDsZJYqgEwbqQ5Yw9ZONr6Gw+x+a4VeMVao9+w\n\tBAwWK3nNqsfbW6Y+ewq1EIg0BajfHEaESGizyQ5DnOefTf+uGcmZ+XYASwUTkqXvwSVoRTS0\n\t4nXCOVG2LGhM9bc5zLXXsgPjH2xx8vLSqebXyIuam0d8X2/R6mFHkI9Oh0n5feEs0i80vMyB\n\teEYDeZGNnkrPkosWKYo6KeC/QmpAIqYytDuevhJMD/cK5ugWc9tfzpwkKb7mFm+7aUU7wUhl\n\t9OO/lhAAO5B8uVgv55ZxFS1wVrgi/0DnWZx7dDj+b0xubexMoRqdtNMBcw4ey9sQ2TMfLuLX\n\tsaq93eNA8tmKLRZrFKuGeSQBj0u/1KGKitDUxGEOjCkZZ5R7i0IhOmMXCCpSlRH6TYzHtkLC\n\tqLMGnCSuHv0AUtXE37OlRPLf3cga8SqJJyLJ+2jwDCr1xT32cLiD19jYgfsnS0+gvl52gn9a\n\tf4K76WtYlFf/RMGl4N1fLLcVLMt3QuYjPbVQVcMxXWS5cIQFpUSWo2d8Z7kWrHJ8jL4/ZxxZ\n\tmPkwI2lLHEmvvlBO0tsnECtkApB/hc9/aQCa1gUWzLkCDQRTLZU5ARAAsqUr9WS+cuZ3aZP/\n\tUV2vO6HZ6L8gHJQcMVV22uBRccuet4QEPQ9UgURac9lWjqUlCOmWU1HgISjM1oD3siakeqRB\n\tTHvRv3p7Za55DJOlYj+HhM7q4l2m7FlSKqlEABIuL02FvjtRMsobPhpTu1vjBGe0VMKafqkG\n\t0CbLKnFwkRxjVMZSqVMws1hlXEeTK27IJxzoxptfDHKj6w54J367tO0ofubxLA3RvebxZG7D\n\t1vWe8NTrNYItuMaXtq4tbbxGY3In2YE+8G9mAQsG1p+XSIm6UBO0lBZJ+NURy/aYmpma39Ji\n\t9hE1YZmcDhuRfBPXKSXJa8VavEAON8VbFAtqcXtS/8GbXLzSmUKf/fULHbiWWgspKoMhoWCD\n\tryOgABqoc8pu1+XL6uTsr2VksbgXun0IdadI1EVXzc9Hgtra7bZ7C8KzTOgp8u1MFHTyynlO\n\tQnAosbxVcXSQ95KcEb3V1nMhmzJ5r85Nvlxs2ROqM+/e/Cf16DYPe4iaoHhxuPrAe0ul4/21\n\tdoJq4WVkknqIUpTZkVV/6rLfuFhjKszF5sUXIcOqOn3tYCz/eCxQsXXaq0DBw1IOsQpnq8yP\n\tMXJ7mNV7ZcKd/4ocX3F6PLFMf2SBGoeive37xf3wdM1Nf4s342D778suPHJmf5+0BQLSv1R0\n\tVhTpst0W0c7ge0ozFOcAEQEAAYkCHwQYAQIACQUCUy2VOQIbDAAKCRAjhj3Dgx2ysQmtEADF\n\tKynuTGR5fIVFM0wkAvPBWkh9kMcQwK+PjDR1p7JqNXnlIraBOHlRfxXdu6uYabQ4pyAAPiHt\n\tfCoCzIvsebXsArbdl7IGBc7gBw/pBXAo7Bt24JfbGCrKkpzu6y2iKT/G8oZP37TlkK6D86nm\n\tYBY/UqbMbNe28CUeIhTyeVDx28gbDJc1rndOL2cz4BIlzg3Di47woMWnEuaCQ536KM61LnY7\n\tp/pJ9RcvLrOIm2ESy5M5gHouH7iXNzn5snKFhfi1zbTT/UrtEuY1VjCtiTcCXzXbzy2oy/zw\n\tERaDwkRzhcVrFdsttMYDyaNY3GQfJSBq4Q9rADG2nn/87e3g7dmPecVYS5YFxocCk77Zg7xx\n\tGxSDtXgJEVmdGTGYCrM+SrW8ywj03kfwnURqOnxbsbHaSUmJtVovA+ZzdpHV1e7S91AvxbXt\n\tLrxWADsl+pzz9rJ25+Hh7f/HeflGaUDYbOycQVzcyKekKkuIlibpv+S0nPiitxlV91agRV0i\n\tcpG0pX8PrmjQ0YV8pvfUFyrfHtHzTMA4ktMNzF5FhNkE1WNwXZHD+P6nmPEZiOi45tqI7Ro6\n\tmX/IKTr6GLCzg0OVP6NSsgSJeR6Hd2GvSI2Vw1jfnZI4tCNU2BmODPBkGBRLhNR+L5eRqOMm\n\tQglrIkyNWSZm4Hhw98VxYDwOwmYhoXmAFg==","Message-ID":"<296a1240-80a8-3a19-160b-100f197a652c@bingham.xyz>","Date":"Sat, 7 Mar 2020 23:28:59 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.9.1","MIME-Version":"1.0","In-Reply-To":"<20200307232040.GA31018@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v1.2 16/31] libcamera: controls:\n\tSupport array controls in ControlValue","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sat, 07 Mar 2020 23:29:05 -0000"}}]