[{"id":34439,"web_url":"https://patchwork.libcamera.org/comment/34439/","msgid":"","date":"2025-06-10T16:01:29","subject":"Re: [RFC PATCH v1 02/23] libcamera: controls: Add `ControlValueView`","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n\nOn Fri, Jun 06, 2025 at 06:41:35PM +0200, Barnabás Pőcze wrote:\n> Add `ControlValueView`, which is a non-owning read-only handle to a control\n> value with a very similar interface.\n>\n> Signed-off-by: Barnabás Pőcze \n> ---\n> include/libcamera/controls.h | 69 +++++++++++++\n> src/libcamera/controls.cpp | 194 +++++++++++++++++++++++++++++++++++\n> 2 files changed, 263 insertions(+)\n>\n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index b170e30cb..8734479cf 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -8,6 +8,7 @@\n> #pragma once\n>\n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -25,6 +26,7 @@\n> namespace libcamera {\n>\n> class ControlValidator;\n> +class ControlValueView;\n>\n> enum ControlType {\n> \tControlTypeNone,\n> @@ -160,6 +162,8 @@ public:\n> \t\t value.data(), value.size(), sizeof(typename T::value_type));\n> \t}\n>\n> +\texplicit ControlValue(const ControlValueView &cvv);\n> +\n> \t~ControlValue();\n>\n> \tControlValue(const ControlValue &other);\n> @@ -247,6 +251,71 @@ private:\n> \t\t std::size_t numElements, std::size_t elementSize);\n> };\n>\n> +class ControlValueView\n> +{\n> +public:\n> +\tconstexpr ControlValueView() noexcept\n\nWe don't disable exception at compile time as far as I remember but in\nany case we don't thrown any. Do you think for public API it might be\nuseful to mark a function noexcept anyway ?\n\n> +\t\t: type_(ControlTypeNone)\n> +\t{\n> +\t}\n> +\n> +\tControlValueView(const ControlValue &cv) noexcept\n> +\t\t: ControlValueView(cv.type(), cv.isArray(), cv.numElements(),\n> +\t\t\t\t reinterpret_cast(cv.data().data()))\n> +\t{\n> +\t}\n> +\n> +#ifndef __DOXYGEN__\n> +\t// TODO: should have restricted access?\n\nWho would you like to be able to call this function ? Would making it\nprivate be enough\n\n> +\tControlValueView(ControlType type, bool isArray, std::size_t numElements, const std::byte *data) noexcept\n\nbetter fit in 2 lines\n\n> +\t\t: type_(type),\n> +\t\t isArray_(isArray),\n> +\t\t numElements_(numElements),\n> +\t\t data_(data)\n\nFits on 2 lines\n\n\tControlValueView(ControlType type, bool isArray, std::size_t numElements,\n\t\t\t const std::byte *data) noexcept\n\t\t: type_(type), isArray_(isArray), numElements_(numElements),\n\t\t data_(data)\n\n> +\t{\n> +\t\tassert(isArray || numElements == 1);\n> +\t}\n> +#endif\n> +\n> +\t[[nodiscard]] explicit operator bool() const { return type_ != ControlTypeNone; }\n> +\t[[nodiscard]] ControlType type() const { return type_; }\n> +\t[[nodiscard]] bool isNone() const { return type_ == ControlTypeNone; }\n> +\t[[nodiscard]] bool isArray() const { return isArray_; }\n> +\t[[nodiscard]] std::size_t numElements() const { return numElements_; }\n> +\t[[nodiscard]] Span data() const;\n\nI wonder if we should really make these nodiscard. I agree it doesn't\nmake sense to ignore the return value, but at the same time it has no\nside effects..\n\n> +\n> +\t[[nodiscard]] bool operator==(const ControlValueView &other) const;\n> +\n> +\t[[nodiscard]] bool operator!=(const ControlValueView &other) const\n> +\t{\n> +\t\treturn !(*this == other);\n> +\t}\n> +\n> +\ttemplate\n> +\t[[nodiscard]] auto get() const\n> +\t{\n> +\t\tusing TypeInfo = details::control_type>;\n> +\n> +\t\tassert(type_ == TypeInfo::value);\n> +\t\tassert(isArray_ == (TypeInfo::size > 0));\n> +\n> +\t\tif constexpr (TypeInfo::size > 0) {\n\nI guess you could also check numElements_ but static compile-time\nbranching is probably more efficient ?\n\n> +\t\t\treturn T(reinterpret_cast(data().data()), numElements_);\n> +\t\t} else {\n> +\t\t\tassert(numElements_ == 1);\n> +\t\t\treturn *reinterpret_cast(data().data());\n> +\t\t}\n> +\t}\n> +\n> +private:\n> +\tControlType type_ : 8;\n\nThe base type of the ControlType enumeration is a int32, right ?\nDoes this have any implications ?\n\n> +\tbool isArray_ = false;\n> +\tuint32_t numElements_ = 0;\n> +\tconst std::byte *data_ = nullptr;\n> +};\n> +\n> +std::ostream &operator<<(std::ostream &s, const ControlValueView &v);\n> +\n> class ControlId\n> {\n> public:\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index 98fa7583d..e7e2781e8 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -106,6 +106,16 @@ ControlValue::ControlValue()\n> {\n> }\n>\n> +/**\n> + * \\brief Construct a ControlValue from a ControlValueView\n> + */\n> +ControlValue::ControlValue(const ControlValueView &cvv)\n> +\t: ControlValue()\n\nDoes calling the default constructor help ?\n\n> +{\n> +\tset(cvv.type(), cvv.isArray(), cvv.data().data(),\n> +\t cvv.numElements(), ControlValueSize[cvv.type()]);\n> +}\n> +\n> /**\n> * \\fn template T ControlValue::ControlValue(const T &value)\n> * \\brief Construct a ControlValue of type T\n> @@ -395,6 +405,190 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen\n> \t\tstorage_ = reinterpret_cast(new uint8_t[newSize]);\n> }\n>\n> +/**\n> + * \\class ControlValueView\n> + * \\brief A non-owning view-like type to the value of a control\n\nI was about to ask for more documentation, but seeing how little we\nhave for ControlValue I'm not sure it's fair :)\n\n> + */\n> +\n> +/**\n> + * \\fn ControlValueView::ControlValueView()\n> + * \\brief Construct an empty view\n> + * \\sa ControlValue::ControlValue()\n> + */\n> +\n> +/**\n> + * \\fn ControlValueView::ControlValueView(const ControlValue &v)\n> + * \\brief Construct a view referring to \\a v\n> + *\n> + * The constructed view will refer to the value stored by \\a v, and\n> + * thus \\a v must not be modified or destroyed before the view.\n\nRight, we have a view now, which refers to the data buffer owned by a\nControlValue. If the control value goes away the storage gets released\nand the view will point to invalid memory ?\n\n(also, I'm not sure why it can't be modified)\n\n\n> + *\n> + * \\sa ControlValue::ControlValue()\n> + */\n> +\n> +/**\n> + * \\fn ControlValueView::operator bool() const\n> + * \\brief Determine if the referred value is valid\n> + * \\sa ControlValueView::isNone()\n\nMakes me wonder if we shouldn't instead check for data_ to be !=\nnullptr to protect against the above \"release the Value before the\nView situation\". However, thinking a bit more, the\nControlValueView::data_ pointer won't be set to nullptr when a\nControlValue is released. It will just point to invalid memory if I'm\nnot mistaken.\n\nOk, we just need to wrap ControlValue with shared_ptr<> everywhere,\nwhat could it take.... (kidding)\n\n> + */\n> +\n> +/**\n> + * \\fn ControlType ControlValueView::type() const\n> + * \\copydoc ControlValue::type()\n> + * \\sa ControlValue::type()\n> + */\n> +\n> +/**\n> + * \\fn ControlValueView::isNone() const\n> + * \\copydoc ControlValue::isNone()\n> + * \\sa ControlValue::isNone()\n> + */\n> +\n> +/**\n> + * \\fn ControlValueView::isArray() const\n> + * \\copydoc ControlValue::isArray()\n> + * \\sa ControlValue::isArray()\n> + */\n> +\n> +/**\n> + * \\fn ControlValueView::numElements() const\n> + * \\copydoc ControlValue::numElements()\n> + * \\sa ControlValue::numElements()\n> + */\n> +\n> +/**\n> + * \\copydoc ControlValue::data()\n> + * \\sa ControlValue::data()\n> + */\n> +Span ControlValueView::data() const\n> +{\n> +\treturn { data_, numElements_ * ControlValueSize[type_] };\n> +}\n> +\n> +/**\n> + * \\copydoc ControlValue::operator==()\n> + * \\sa ControlValue::operator==()\n> + * \\sa ControlValueView::operator!=()\n> + */\n> +bool ControlValueView::operator==(const ControlValueView &other) const\n> +{\n> +\tif (type_ != other.type_)\n> +\t\treturn false;\n> +\n> +\tif (numElements_ != other.numElements_)\n> +\t\treturn false;\n> +\n> +\tif (isArray_ != other.isArray_)\n> +\t\treturn false;\n> +\n> +\tconst auto d = data();\n> +\n> +\treturn memcmp(d.data(), other.data_, d.size_bytes()) == 0;\n\nAs these are views, isn't enough to compare the address pointed to by\ndata_ ?\n\n> +}\n> +\n> +/**\n> + * \\fn ControlValueView::operator!=() const\n> + * \\copydoc ControlValue::operator!=()\n> + * \\sa ControlValue::operator!=()\n> + * \\sa ControlValueView::operator==()\n> + */\n> +\n> +/**\n> + * \\fn template T ControlValueView::get() const\n> + * \\copydoc ControlValue::get()\n> + * \\sa ControlValue::get()\n> + */\n> +\n> +/**\n> + * \\brief Insert a text representation of a value into an output stream\n> + * \\sa ControlValue::toString()\n> + */\n> +std::ostream &operator<<(std::ostream &s, const ControlValueView &v)\n\nUnsurprisingly similar to ControlValue::toString().\n\nHave you considered factoring out the common code parts ?\n\n> +{\n> +\tconst auto type = v.type();\n> +\tif (type == ControlTypeNone)\n> +\t\treturn s << \"None\";\n> +\n> +\tconst auto *data = v.data().data();\n> +\tconst auto numElements = v.numElements();\n> +\n> +\tif (type == ControlTypeString)\n> +\t\treturn s << std::string_view(reinterpret_cast(data),\n> +\t\t\t\t\t numElements);\n> +\n> +\tconst bool isArray = v.isArray();\n> +\tif (isArray)\n> +\t\ts << \"[ \";\n> +\n> +\tfor (std::size_t i = 0; i < numElements; ++i) {\n> +\t\tif (i > 0)\n> +\t\t\ts << \", \";\n> +\n> +\t\tswitch (type) {\n> +\t\tcase ControlTypeBool: {\n> +\t\t\tconst bool *value = reinterpret_cast(data);\n> +\t\t\ts << (*value ? \"true\" : \"false\");\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tcase ControlTypeByte: {\n> +\t\t\tconst auto *value = reinterpret_cast(data);\n> +\t\t\ts << static_cast(*value);\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tcase ControlTypeUnsigned16: {\n> +\t\t\tconst auto *value = reinterpret_cast(data);\n> +\t\t\ts << *value;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tcase ControlTypeUnsigned32: {\n> +\t\t\tconst auto *value = reinterpret_cast(data);\n> +\t\t\ts << *value;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tcase ControlTypeInteger32: {\n> +\t\t\tconst auto *value = reinterpret_cast(data);\n> +\t\t\ts << *value;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tcase ControlTypeInteger64: {\n> +\t\t\tconst auto *value = reinterpret_cast(data);\n> +\t\t\ts << *value;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tcase ControlTypeFloat: {\n> +\t\t\tconst auto *value = reinterpret_cast(data);\n> +\t\t\ts << std::fixed << *value;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tcase ControlTypeRectangle: {\n> +\t\t\tconst auto *value = reinterpret_cast(data);\n> +\t\t\ts << *value;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tcase ControlTypeSize: {\n> +\t\t\tconst auto *value = reinterpret_cast(data);\n> +\t\t\ts << *value;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tcase ControlTypePoint: {\n> +\t\t\tconst auto *value = reinterpret_cast(data);\n> +\t\t\ts << *value;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tcase ControlTypeNone:\n> +\t\tcase ControlTypeString:\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tdata += ControlValueSize[type];\n> +\t}\n> +\n> +\tif (isArray)\n> +\t\ts << \" ]\";\n> +\n> +\treturn s;\n> +}\n> +\n> /**\n> * \\class ControlId\n> * \\brief Control static metadata\n> --\n> 2.49.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C5E67BDE6B\n\tfor ;\n\tTue, 10 Jun 2025 16:01:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8216B68DBD;\n\tTue, 10 Jun 2025 18:01:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 958C861869\n\tfor ;\n\tTue, 10 Jun 2025 18:01:33 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D459C22B;\n\tTue, 10 Jun 2025 18:01:25 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"aqaUZk1S\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1749571286;\n\tbh=cLhPA79NkJgtACrg6G7LJ1bCCqWfbpBBhwkbPYwjw20=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=aqaUZk1SqRaziqoaJG51FH6i4TRl5lnUqBHH9LZZPlsbYa20jj0NvMQ9418MIldOo\n\t69okThcAxb4YzsRzbwP4ovl/5jlxPzzM+vzTkD4GUsF67EQhTia81Qs4ZiX94pl18T\n\tWd/Ssb2f+5JIU4qQ22b9AUXUFq6akiu1JfOyzskk=","Date":"Tue, 10 Jun 2025 18:01:29 +0200","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1 02/23] libcamera: controls: Add `ControlValueView`","Message-ID":"","References":"<20250606164156.1442682-1-barnabas.pocze@ideasonboard.com>\n\t<20250606164156.1442682-3-barnabas.pocze@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20250606164156.1442682-3-barnabas.pocze@ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34459,"web_url":"https://patchwork.libcamera.org/comment/34459/","msgid":"","date":"2025-06-11T15:16:57","subject":"Re: [RFC PATCH v1 02/23] libcamera: controls: Add `ControlValueView`","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2025. 06. 10. 18:01 keltezéssel, Jacopo Mondi írta:\n> Hi Barnabás\n> \n> On Fri, Jun 06, 2025 at 06:41:35PM +0200, Barnabás Pőcze wrote:\n>> Add `ControlValueView`, which is a non-owning read-only handle to a control\n>> value with a very similar interface.\n>>\n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>> include/libcamera/controls.h | 69 +++++++++++++\n>> src/libcamera/controls.cpp | 194 +++++++++++++++++++++++++++++++++++\n>> 2 files changed, 263 insertions(+)\n>>\n>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n>> index b170e30cb..8734479cf 100644\n>> --- a/include/libcamera/controls.h\n>> +++ b/include/libcamera/controls.h\n>> @@ -8,6 +8,7 @@\n>> #pragma once\n>>\n>> #include \n>> +#include \n>> #include \n>> #include \n>> #include \n>> @@ -25,6 +26,7 @@\n>> namespace libcamera {\n>>\n>> class ControlValidator;\n>> +class ControlValueView;\n>>\n>> enum ControlType {\n>> \tControlTypeNone,\n>> @@ -160,6 +162,8 @@ public:\n>> \t\t value.data(), value.size(), sizeof(typename T::value_type));\n>> \t}\n>>\n>> +\texplicit ControlValue(const ControlValueView &cvv);\n>> +\n>> \t~ControlValue();\n>>\n>> \tControlValue(const ControlValue &other);\n>> @@ -247,6 +251,71 @@ private:\n>> \t\t std::size_t numElements, std::size_t elementSize);\n>> };\n>>\n>> +class ControlValueView\n>> +{\n>> +public:\n>> +\tconstexpr ControlValueView() noexcept\n> \n> We don't disable exception at compile time as far as I remember but in\n> any case we don't thrown any. Do you think for public API it might be\n> useful to mark a function noexcept anyway ?\n\nHaving noexcept on constructors, assignment operators, and certain functions\n(e.g. swap()) can result in better code in STL or third-party libraries. Most\nsignificantly e.g. resizing std::vector is more efficient if `T` has a noexcept\nmove constructor.\n\n\n> \n>> +\t\t: type_(ControlTypeNone)\n>> +\t{\n>> +\t}\n>> +\n>> +\tControlValueView(const ControlValue &cv) noexcept\n>> +\t\t: ControlValueView(cv.type(), cv.isArray(), cv.numElements(),\n>> +\t\t\t\t reinterpret_cast(cv.data().data()))\n>> +\t{\n>> +\t}\n>> +\n>> +#ifndef __DOXYGEN__\n>> +\t// TODO: should have restricted access?\n> \n> Who would you like to be able to call this function ? Would making it\n> private be enough\n\n`MetadataList` is the main user of this constructor, so making it\nprivate is not enough.\n\n\n> \n>> +\tControlValueView(ControlType type, bool isArray, std::size_t numElements, const std::byte *data) noexcept\n> \n> better fit in 2 lines\n> \n>> +\t\t: type_(type),\n>> +\t\t isArray_(isArray),\n>> +\t\t numElements_(numElements),\n>> +\t\t data_(data)\n> \n> Fits on 2 lines\n> \n> \tControlValueView(ControlType type, bool isArray, std::size_t numElements,\n> \t\t\t const std::byte *data) noexcept\n> \t\t: type_(type), isArray_(isArray), numElements_(numElements),\n> \t\t data_(data)\n> \n>> +\t{\n>> +\t\tassert(isArray || numElements == 1);\n>> +\t}\n>> +#endif\n>> +\n>> +\t[[nodiscard]] explicit operator bool() const { return type_ != ControlTypeNone; }\n>> +\t[[nodiscard]] ControlType type() const { return type_; }\n>> +\t[[nodiscard]] bool isNone() const { return type_ == ControlTypeNone; }\n>> +\t[[nodiscard]] bool isArray() const { return isArray_; }\n>> +\t[[nodiscard]] std::size_t numElements() const { return numElements_; }\n>> +\t[[nodiscard]] Span data() const;\n> \n> I wonder if we should really make these nodiscard. I agree it doesn't\n> make sense to ignore the return value, but at the same time it has no\n> side effects..\n\nI think the fact that they make no side effects is one reason why you want\nto make them nodiscard.\n\n\n> \n>> +\n>> +\t[[nodiscard]] bool operator==(const ControlValueView &other) const;\n>> +\n>> +\t[[nodiscard]] bool operator!=(const ControlValueView &other) const\n>> +\t{\n>> +\t\treturn !(*this == other);\n>> +\t}\n>> +\n>> +\ttemplate\n>> +\t[[nodiscard]] auto get() const\n>> +\t{\n>> +\t\tusing TypeInfo = details::control_type>;\n>> +\n>> +\t\tassert(type_ == TypeInfo::value);\n>> +\t\tassert(isArray_ == (TypeInfo::size > 0));\n>> +\n>> +\t\tif constexpr (TypeInfo::size > 0) {\n> \n> I guess you could also check numElements_ but static compile-time\n> branching is probably more efficient ?\n> \n\nWith a runtime branch, the code wouldn't compile in all cases. E.g. `T == bool`\nwould try to access `bool::value_type` and such a thing does not exist.\n\n\n>> +\t\t\treturn T(reinterpret_cast(data().data()), numElements_);\n>> +\t\t} else {\n>> +\t\t\tassert(numElements_ == 1);\n>> +\t\t\treturn *reinterpret_cast(data().data());\n>> +\t\t}\n>> +\t}\n>> +\n>> +private:\n>> +\tControlType type_ : 8;\n> \n> The base type of the ControlType enumeration is a int32, right ?\n> Does this have any implications ?\n\nThe `ControlValue` class has the same 8-bit wide field, so I just copied it from there.\nCurrently it is wide enough for every enumerator.\n\n\n> \n>> +\tbool isArray_ = false;\n>> +\tuint32_t numElements_ = 0;\n>> +\tconst std::byte *data_ = nullptr;\n>> +};\n>> +\n>> +std::ostream &operator<<(std::ostream &s, const ControlValueView &v);\n>> +\n>> class ControlId\n>> {\n>> public:\n>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n>> index 98fa7583d..e7e2781e8 100644\n>> --- a/src/libcamera/controls.cpp\n>> +++ b/src/libcamera/controls.cpp\n>> @@ -106,6 +106,16 @@ ControlValue::ControlValue()\n>> {\n>> }\n>>\n>> +/**\n>> + * \\brief Construct a ControlValue from a ControlValueView\n>> + */\n>> +ControlValue::ControlValue(const ControlValueView &cvv)\n>> +\t: ControlValue()\n> \n> Does calling the default constructor help ?\n\nYes because otherwise `type_`, etc. would not be set, which would cause\nissues when `set()` tries to free the current content.\n\n\n> \n>> +{\n>> +\tset(cvv.type(), cvv.isArray(), cvv.data().data(),\n>> +\t cvv.numElements(), ControlValueSize[cvv.type()]);\n>> +}\n>> +\n>> /**\n>> * \\fn template T ControlValue::ControlValue(const T &value)\n>> * \\brief Construct a ControlValue of type T\n>> @@ -395,6 +405,190 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen\n>> \t\tstorage_ = reinterpret_cast(new uint8_t[newSize]);\n>> }\n>>\n>> +/**\n>> + * \\class ControlValueView\n>> + * \\brief A non-owning view-like type to the value of a control\n> \n> I was about to ask for more documentation, but seeing how little we\n> have for ControlValue I'm not sure it's fair :)\n> \n>> + */\n>> +\n>> +/**\n>> + * \\fn ControlValueView::ControlValueView()\n>> + * \\brief Construct an empty view\n>> + * \\sa ControlValue::ControlValue()\n>> + */\n>> +\n>> +/**\n>> + * \\fn ControlValueView::ControlValueView(const ControlValue &v)\n>> + * \\brief Construct a view referring to \\a v\n>> + *\n>> + * The constructed view will refer to the value stored by \\a v, and\n>> + * thus \\a v must not be modified or destroyed before the view.\n> \n> Right, we have a view now, which refers to the data buffer owned by a\n> ControlValue. If the control value goes away the storage gets released\n> and the view will point to invalid memory ?\n> \n> (also, I'm not sure why it can't be modified)\n\nThe type/size cannot be modified, but the value(s) themselves could indeed\nbe modified. But given that the main use case of this is `MetadataList`\nwhere concurrent modifications are undesirable I want to disallow modifications.\n\n\n> \n> \n>> + *\n>> + * \\sa ControlValue::ControlValue()\n>> + */\n>> +\n>> +/**\n>> + * \\fn ControlValueView::operator bool() const\n>> + * \\brief Determine if the referred value is valid\n>> + * \\sa ControlValueView::isNone()\n> \n> Makes me wonder if we shouldn't instead check for data_ to be !=\n> nullptr to protect against the above \"release the Value before the\n> View situation\". However, thinking a bit more, the\n> ControlValueView::data_ pointer won't be set to nullptr when a\n> ControlValue is released. It will just point to invalid memory if I'm\n> not mistaken.\n> \n> Ok, we just need to wrap ControlValue with shared_ptr<> everywhere,\n> what could it take.... (kidding)\n> \n>> + */\n>> +\n>> +/**\n>> + * \\fn ControlType ControlValueView::type() const\n>> + * \\copydoc ControlValue::type()\n>> + * \\sa ControlValue::type()\n>> + */\n>> +\n>> +/**\n>> + * \\fn ControlValueView::isNone() const\n>> + * \\copydoc ControlValue::isNone()\n>> + * \\sa ControlValue::isNone()\n>> + */\n>> +\n>> +/**\n>> + * \\fn ControlValueView::isArray() const\n>> + * \\copydoc ControlValue::isArray()\n>> + * \\sa ControlValue::isArray()\n>> + */\n>> +\n>> +/**\n>> + * \\fn ControlValueView::numElements() const\n>> + * \\copydoc ControlValue::numElements()\n>> + * \\sa ControlValue::numElements()\n>> + */\n>> +\n>> +/**\n>> + * \\copydoc ControlValue::data()\n>> + * \\sa ControlValue::data()\n>> + */\n>> +Span ControlValueView::data() const\n>> +{\n>> +\treturn { data_, numElements_ * ControlValueSize[type_] };\n>> +}\n>> +\n>> +/**\n>> + * \\copydoc ControlValue::operator==()\n>> + * \\sa ControlValue::operator==()\n>> + * \\sa ControlValueView::operator!=()\n>> + */\n>> +bool ControlValueView::operator==(const ControlValueView &other) const\n>> +{\n>> +\tif (type_ != other.type_)\n>> +\t\treturn false;\n>> +\n>> +\tif (numElements_ != other.numElements_)\n>> +\t\treturn false;\n>> +\n>> +\tif (isArray_ != other.isArray_)\n>> +\t\treturn false;\n>> +\n>> +\tconst auto d = data();\n>> +\n>> +\treturn memcmp(d.data(), other.data_, d.size_bytes()) == 0;\n> \n> As these are views, isn't enough to compare the address pointed to by\n> data_ ?\n\nI think that largely depends on how one defines equality. I think considering the values\nthemselves makes sense since this is what `ControlValue` does, and the following might be\nsurprising:\n\n ControlValue a(1), b(1);\n assert(a == b);\n assert(ControlValueView(a) != ControlValueView(b));\n\n\n> \n>> +}\n>> +\n>> +/**\n>> + * \\fn ControlValueView::operator!=() const\n>> + * \\copydoc ControlValue::operator!=()\n>> + * \\sa ControlValue::operator!=()\n>> + * \\sa ControlValueView::operator==()\n>> + */\n>> +\n>> +/**\n>> + * \\fn template T ControlValueView::get() const\n>> + * \\copydoc ControlValue::get()\n>> + * \\sa ControlValue::get()\n>> + */\n>> +\n>> +/**\n>> + * \\brief Insert a text representation of a value into an output stream\n>> + * \\sa ControlValue::toString()\n>> + */\n>> +std::ostream &operator<<(std::ostream &s, const ControlValueView &v)\n> \n> Unsurprisingly similar to ControlValue::toString().\n> \n> Have you considered factoring out the common code parts ?\n\nYes, it's a copy of that. I'll modify `ControlValue::toString()` to avoid\nthe repetition.\n\n\n> [...]\n\n\nRegards,\nBarnabás Pőcze","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 41317C3237\n\tfor ;\n\tWed, 11 Jun 2025 15:17:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A7C8C68DC0;\n\tWed, 11 Jun 2025 17:17:02 +0200 (CEST)","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 A712361552\n\tfor ;\n\tWed, 11 Jun 2025 17:17:00 +0200 (CEST)","from [192.168.33.24] (185.221.142.30.nat.pool.zt.hu\n\t[185.221.142.30])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2591E352;\n\tWed, 11 Jun 2025 17:16:52 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"m8Fucn8F\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1749655012;\n\tbh=3suEfUAtdD4XXNDtj/7jRRdyZXpW20VhJCth++B4Nlk=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=m8Fucn8FIrEgJKHMCA+VXzkbJ5BtYvcHMFImDfH7/9Zuk7Cc5BYTC8+cAO/oRpWtV\n\tl9fyYwauiaYA3ljHRW+SRhdUCFkiJ7TdgDx88nCHksdDmooFhb90cmDqS+QaHEL/Tx\n\t5nWSyvqHtLoXMNeHyFPDdvSwlIYTbMe58iZsAkQQ=","Message-ID":"","Date":"Wed, 11 Jun 2025 17:16:57 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v1 02/23] libcamera: controls: Add `ControlValueView`","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250606164156.1442682-1-barnabas.pocze@ideasonboard.com>\n\t<20250606164156.1442682-3-barnabas.pocze@ideasonboard.com>\n\t","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]