Patch Detail
Show a patch.
GET /api/patches/2988/?format=api
{ "id": 2988, "url": "https://patchwork.libcamera.org/api/patches/2988/?format=api", "web_url": "https://patchwork.libcamera.org/patch/2988/", "project": { "id": 1, "url": "https://patchwork.libcamera.org/api/projects/1/?format=api", "name": "libcamera", "link_name": "libcamera", "list_id": "libcamera_core", "list_email": "libcamera-devel@lists.libcamera.org", "web_url": "", "scm_url": "", "webscm_url": "" }, "msgid": "<20200306160002.30549-17-laurent.pinchart@ideasonboard.com>", "date": "2020-03-06T15:59:46", "name": "[libcamera-devel,v2,16/32] libcamera: controls: Support array controls in ControlValue", "commit_ref": null, "pull_url": null, "state": "accepted", "archived": false, "hash": "c513ba33becee8ea6f93b8bcdec3a33b04d800d8", "submitter": { "id": 2, "url": "https://patchwork.libcamera.org/api/people/2/?format=api", "name": "Laurent Pinchart", "email": "laurent.pinchart@ideasonboard.com" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/2988/mbox/", "series": [ { "id": 703, "url": "https://patchwork.libcamera.org/api/series/703/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=703", "date": "2020-03-06T15:59:30", "name": "libcamera: Add support for array controls", "version": 2, "mbox": "https://patchwork.libcamera.org/series/703/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/2988/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/2988/checks/", "tags": {}, "headers": { "Return-Path": "<laurent.pinchart@ideasonboard.com>", "Received": [ "from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2277D628B9\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 6 Mar 2020 17:00:22 +0100 (CET)", "from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 99863312;\n\tFri, 6 Mar 2020 17:00:21 +0100 (CET)" ], "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1583510421;\n\tbh=pDY7DliOvE1W90V9SgaI1VTbzwETcaqJ6v5bMGeyKiE=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=WnucqWI/StNAo5AfCrx8eMNAAVHQ0AgHiMu1ia+JUGyM7+t0Sbmm3oGhYTnFvdhEn\n\tZ6dO4MMkLFny/8d8us3118W85r4e9PE+su/fbwyr4D/8jnVyCwrqQ1I04KiY751OE1\n\tVZyLN7zo45/b96WjLv7coQdv3XIp94VBJ/fL4e2k=", "From": "Laurent Pinchart <laurent.pinchart@ideasonboard.com>", "To": "libcamera-devel@lists.libcamera.org", "Date": "Fri, 6 Mar 2020 17:59:46 +0200", "Message-Id": "<20200306160002.30549-17-laurent.pinchart@ideasonboard.com>", "X-Mailer": "git-send-email 2.24.1", "In-Reply-To": "<20200306160002.30549-1-laurent.pinchart@ideasonboard.com>", "References": "<20200306160002.30549-1-laurent.pinchart@ideasonboard.com>", "MIME-Version": "1.0", "Content-Transfer-Encoding": "8bit", "Subject": "[libcamera-devel] [PATCH v2 16/32] libcamera: controls: Support\n\tarray controls in ControlValue", "X-BeenThere": "libcamera-devel@lists.libcamera.org", "X-Mailman-Version": "2.1.29", "Precedence": "list", "List-Id": "<libcamera-devel.lists.libcamera.org>", "List-Unsubscribe": "<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>", "List-Archive": "<https://lists.libcamera.org/pipermail/libcamera-devel/>", "List-Post": "<mailto:libcamera-devel@lists.libcamera.org>", "List-Help": "<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>", "List-Subscribe": "<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>", "X-List-Received-Date": "Fri, 06 Mar 2020 16:00:22 -0000" }, "content": "From: Jacopo Mondi <jacopo@jmondi.org>\n\nAdd array controls support to the ControlValue class. The polymorphic\nclass can now store more than a single element and supports access and\ncreation through the use of Span<>.\n\nSigned-off-by: Jacopo Mondi <jacopo@jmondi.org>\nSigned-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\nReviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n---\nChanges 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- Use parentheses with sizeof operator\n- Drop unneeded documentation paragraph\n- Fix typos\n---\n include/libcamera/controls.h | 81 +++++++++++++---\n src/libcamera/controls.cpp | 181 +++++++++++++++++++++++++++++------\n 2 files changed, 221 insertions(+), 41 deletions(-)", "diff": "diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\nindex 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 <assert.h>\n+#include <stdint.h>\n #include <string>\n #include <unordered_map>\n \n@@ -51,6 +52,10 @@ struct control_type<int64_t> {\n \tstatic constexpr ControlType value = ControlTypeInteger64;\n };\n \n+template<typename T, std::size_t N>\n+struct control_type<Span<T, N>> : public control_type<std::remove_cv_t<T>> {\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<typename T, typename std::enable_if_t<!details::is_span<T>::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<std::remove_cv_t<T>>::value, false,\n+\t\t &value, 1, sizeof(T));\n+\t}\n+\n+\ttemplate<typename T, typename std::enable_if_t<details::is_span<T>::value, std::nullptr_t> = nullptr>\n+#else\n \ttemplate<typename T>\n-\tControlValue(T value)\n-\t\t: type_(details::control_type<std::remove_cv_t<T>>::value)\n+#endif\n+\tControlValue(const T &value)\n+\t\t: type_(ControlTypeNone), numElements_(0)\n \t{\n-\t\t*reinterpret_cast<T *>(&bool_) = value;\n+\t\tset(details::control_type<std::remove_cv_t<T>>::value, true,\n+\t\t value.data(), value.size(), sizeof(typename T::value_type));\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<const uint8_t> 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<typename T, typename std::enable_if_t<!details::is_span<T>::value, std::nullptr_t> = nullptr>\n+\tT get() const\n+\t{\n+\t\tassert(type_ == details::control_type<std::remove_cv_t<T>>::value);\n+\t\tassert(!isArray_);\n+\n+\t\treturn *reinterpret_cast<const T *>(data().data());\n+\t}\n+\n+\ttemplate<typename T, typename std::enable_if_t<details::is_span<T>::value, std::nullptr_t> = nullptr>\n+#else\n \ttemplate<typename T>\n+#endif\n \tT get() const\n \t{\n \t\tassert(type_ == details::control_type<std::remove_cv_t<T>>::value);\n+\t\tassert(isArray_);\n+\n+\t\tusing V = typename T::value_type;\n+\t\tconst V *value = reinterpret_cast<const V *>(data().data());\n+\t\treturn { value, numElements_ };\n+\t}\n \n-\t\treturn *reinterpret_cast<const T *>(&bool_);\n+#ifndef __DOXYGEN__\n+\ttemplate<typename T, typename std::enable_if_t<!details::is_span<T>::value, std::nullptr_t> = nullptr>\n+\tvoid set(const T &value)\n+\t{\n+\t\tset(details::control_type<std::remove_cv_t<T>>::value, false,\n+\t\t reinterpret_cast<const void *>(&value), 1, sizeof(T));\n \t}\n \n+\ttemplate<typename T, typename std::enable_if_t<details::is_span<T>::value, std::nullptr_t> = nullptr>\n+#else\n \ttemplate<typename T>\n+#endif\n \tvoid set(const T &value)\n \t{\n-\t\ttype_ = details::control_type<std::remove_cv_t<T>>::value;\n-\t\t*reinterpret_cast<T *>(&bool_) = value;\n+\t\tset(details::control_type<std::remove_cv_t<T>>::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 class ControlId\n {\n public:\ndiff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\nindex e513188010e2..a1eca992884c 100644\n--- a/src/libcamera/controls.cpp\n+++ b/src/libcamera/controls.cpp\n@@ -10,6 +10,7 @@\n #include <iomanip>\n #include <sstream>\n #include <string>\n+#include <string.h>\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,59 @@ 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<typename T> T ControlValue::ControlValue(T value)\n+ * \\fn template<typename T> 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<R>, 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+\t\tdelete[] *reinterpret_cast<char **>(&storage_);\n+\t\tstorage_ = 0;\n+\t}\n+}\n+\n+ControlValue::~ControlValue()\n+{\n+\trelease();\n+}\n+\n+/**\n+ * \\brief Construct a ControlValue with the content of \\a other\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 a copy of the content\n+ * of \\a other\n+ * \\param[in] other The ControlValue to copy content from\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 /**\n * \\fn ControlValue::type()\n@@ -102,16 +147,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 data of a control value\n * \\return The raw data of the control value as a span of uint8_t\n */\n Span<const uint8_t> ControlValue::data() const\n {\n-\treturn {\n-\t\treinterpret_cast<const uint8_t *>(&bool_),\n-\t\tControlValueSize[type_]\n-\t};\n+\tstd::size_t size = numElements_ * ControlValueSize[type_];\n+\tconst uint8_t *data = size > sizeof(storage_)\n+\t\t\t ? *reinterpret_cast<const uint8_t * const *>(&storage_)\n+\t\t\t : reinterpret_cast<const uint8_t *>(&storage_);\n+\treturn { data, size };\n }\n \n /**\n@@ -120,18 +182,43 @@ Span<const uint8_t> ControlValue::data() const\n */\n std::string ControlValue::toString() const\n {\n-\tswitch (type_) {\n-\tcase ControlTypeNone:\n-\t\treturn \"<None>\";\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 \"<ValueType Error>\";\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<const bool *>(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<const int32_t *>(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<const int64_t *>(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 \"<ValueType Error>\";\n+\tif (isArray_)\n+\t\tstr += \" ]\";\n+\n+\treturn str;\n }\n \n /**\n@@ -143,16 +230,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 +249,16 @@ bool ControlValue::operator==(const ControlValue &other) const\n * \\fn template<typename T> 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<const R>, 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<const R> 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 +267,41 @@ bool ControlValue::operator==(const ControlValue &other) const\n * \\fn template<typename T> 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<R>, 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<void *>(new char[size]);\n+\t\t*reinterpret_cast<void **>(&storage_) = storage;\n+\t} else {\n+\t\tstorage = reinterpret_cast<void *>(&storage_);\n+\t}\n+\n+\tmemcpy(storage, data, size);\n+}\n+\n /**\n * \\class ControlId\n * \\brief Control static metadata\n", "prefixes": [ "libcamera-devel", "v2", "16/32" ] }