[{"id":34445,"web_url":"https://patchwork.libcamera.org/comment/34445/","msgid":"","date":"2025-06-11T11:59:18","subject":"Re: [RFC PATCH v1 07/23] libcamera: Add `MetadataList`","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:40PM +0200, Barnabás Pőcze wrote:\n> Add a dedicated `MetadataList` type, whose purpose is to store the metadata\n> reported by a camera for a given request. Previously, a `ControlList` was\n> used for this purpose. The reason for introducing a separate type is to\n> simplify the access to the returned metadata during the entire lifetime\n> of a request.\n>\n> Specifically, for early metadata completion to be easily usable it should be\n> guaranteed that any completed metadata item can be accessed and looked up\n> at least until the associated requested is reused with `Request::reuse()`.\n>\n> However, when a metadata item is completed early, the pipeline handler\n> might still work on the request in the `CameraManager`'s private thread,\n> therefore there is an inherent synchronization issue when an application\n> accesses early metadata.\n>\n> Restricting the user to only access the metadata items of a not yet completed\n> request in the early metadata availability signal handler by ways of\n> documenting or enforcing it at runtime could be an option, but it is not\n> too convenient for the user.\n>\n> The current `ControlList` implementation employs an `std::unordered_map`,\n> so pointers remain stable when the container is modified, so an application\n> could keep accessing particular metadata items outside the signal handler,\n> but this fact is far from obvious, and the user would still not be able\n> to make a copy of all metadata or do lookups based on the numeric ids or\n> the usual `libcamera::Control<>` objects, thus some type safety is lost.\n>\n> The above also requires that each metadata item is only completed once for\n> a given request, but this does not appear to be serious limitation,\n> and in fact, this restriction is enforced by `MetadataList`.\n>\n> The introduced `MetadataList` supports single writer - multiple reader\n> scenarios, and it can be set, looked-up, and copied in a wait-free fashion\n> without introducing data races or other synchronization issues. This is\n> achieved by requiring the possible set of metadata items to be known\n> (such set is stored in a `MetadataListPlan` object). Based on the this\n> plan, a single contiguous allocation is made to accommodate all potential\n> metadata items. Due to this single contiguous allocation that is not modified\n> during the lifetime of a `MetadataList` and atomic modifications, it is\n> possible to easily gaurantee thread-safe set, lookup, and copy; assuming\n> there is only ever a single writer.\n>\n> Signed-off-by: Barnabás Pőcze \n> ---\n> include/libcamera/meson.build | 2 +\n> include/libcamera/metadata_list.h | 619 +++++++++++++++++++++++++\n> include/libcamera/metadata_list_plan.h | 109 +++++\n> src/libcamera/meson.build | 1 +\n> src/libcamera/metadata_list.cpp | 315 +++++++++++++\n> test/controls/meson.build | 1 +\n> test/controls/metadata_list.cpp | 171 +++++++\n> 7 files changed, 1218 insertions(+)\n> create mode 100644 include/libcamera/metadata_list.h\n> create mode 100644 include/libcamera/metadata_list_plan.h\n> create mode 100644 src/libcamera/metadata_list.cpp\n> create mode 100644 test/controls/metadata_list.cpp\n>\n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index 30ea76f94..410b548dd 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -12,6 +12,8 @@ libcamera_public_headers = files([\n> 'framebuffer_allocator.h',\n> 'geometry.h',\n> 'logging.h',\n> + 'metadata_list.h',\n> + 'metadata_list_plan.h',\n> 'orientation.h',\n> 'pixel_format.h',\n> 'request.h',\n> diff --git a/include/libcamera/metadata_list.h b/include/libcamera/metadata_list.h\n> new file mode 100644\n> index 000000000..7514bd2ad\n> --- /dev/null\n> +++ b/include/libcamera/metadata_list.h\n> @@ -0,0 +1,619 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2025, Ideas On Board Oy\n> + *\n> + * Metadata list\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +// TODO: want this?\n\nI would say yes, but what if we move this to the main meson.build\nfile ?\n\nI currently see in test/meson.build\n\n------------------------------------------------------------------------------\n\n# When ASan is enabled, find the path to the ASan runtime needed by multiple\n# tests. This currently works with gcc only, as clang uses different file names\n# depending on the compiler version and target architecture.\nasan_enabled = false\nasan_runtime_missing = false\n\nif get_option('b_sanitize').contains('address')\n asan_enabled = true\n\n if cc.get_id() == 'gcc'\n asan_runtime = run_command(cc, '-print-file-name=libasan.so', check : true).stdout().strip()\n else\n asan_runtime_missing = true\n endif\nendif\n\n------------------------------------------------------------------------------\n\n\nCould we define a library-wide HAS_ASAN symbol in src/meson.build in\nexample in your opinion ?\n\n> +#if __has_include()\n> +#if __SANITIZE_ADDRESS__ /* gcc */\n> +#include \n> +#define HAS_ASAN 1\n> +#elif defined(__has_feature)\n> +#if __has_feature(address_sanitizer) /* clang */\n> +#include \n> +#define HAS_ASAN 1\n> +#endif\n> +#endif\n> +#endif\n> +\n> +namespace libcamera {\n> +\n> +class MetadataList\n> +{\n> +private:\n\nNot against it, but we usually don't mix private: and public:\n\nYou could move the types definition at the beginning of the below\nprivate: section\n\n> +\tstruct ValueParams {\n> +\t\tControlType type;\n> +\t\tbool isArray;\n> +\t\tstd::uint32_t numElements;\n\nDoes prefixing a stdint type with std:: brings anything ? It's\ncertainly more C++, I guess\n\n> +\t};\n> +\n> +\tstruct Entry {\n> +\t\tconst std::uint32_t tag;\n> +\t\tconst std::uint32_t capacity;\n> +\t\tconst std::uint32_t alignment;\n> +\t\tconst ControlType type;\n> +\t\tbool isArray;\n> +\n> +\t\tstatic constexpr std::uint32_t invalidOffset = -1;\n\nconstant are usually kInvalidOffset\n\n\nempty line maybe\n\n> +\t\t/*\n> +\t\t* Offset from the beginning of the allocation, and\n> +\t\t* and _not_ relative to `contentOffset_`.\n> +\t\t*/\n\nweird indent\n\n> +\t\tstd::atomic_uint32_t headerOffset = invalidOffset;\n> +\n> +\t\t[[nodiscard]] std::optional hasValue() const\n> +\t\t{\n> +\t\t\tauto offset = headerOffset.load(std::memory_order_relaxed);\n> +\t\t\tif (offset == invalidOffset)\n> +\t\t\t\treturn {};\n> +\n> +\t\t\treturn offset;\n> +\t\t}\n> +\n> +\t\t[[nodiscard]] std::optional acquireData() const\n> +\t\t{\n> +\t\t\tauto offset = hasValue();\n> +\t\t\tif (offset) {\n> +\t\t\t\t/* sync with release-store on `headerOffset` in `MetadataList::set()` */\n> +\t\t\t\tstd::atomic_thread_fence(std::memory_order_acquire);\n> +\t\t\t}\n> +\n> +\t\t\treturn offset;\n> +\t\t}\n> +\t};\n> +\n> +\tstruct ValueHeader {\n> +\t\tstd::uint32_t tag;\n> +\t\tstd::uint32_t size;\n> +\t\tstd::uint32_t alignment;\n> +\t\tValueParams params;\n\nIf I'm not mistaken ValueParams is only used here. Should it be\ninlined in the ValueHeader definition ?\n\nAlso, if I got this right:\n\nEntry = The metadata descriptor in the first half of the serialized\nbuffer\n\nValueHeader = The header before the actual metadata value in the\nsecond half of the serialized buffer\n\nCorrect ?\n\nIn this case, looking at the fields in those two types\n\n\tstruct Entry {\n\t\tconst std::uint32_t tag;\n\t\tconst std::uint32_t capacity;\n\t\tconst std::uint32_t alignment;\n\t\tconst ControlType type;\n\t\tbool isArray;\n\n ...\n\n };\n\n\tstruct ValueHeader {\n\t\tstd::uint32_t tag;\n\t\tstd::uint32_t size;\n\t\tstd::uint32_t alignment;\n\n struct ValueParams {\n ControlType type;\n bool isArray;\n std::uint32_t numElements;\n };\n };\n\nI see quite an overlap.\n\nBasically for an Entry to match a ValueHeader the only missing\ninformation are size and numElements. This makes sense as these are\nthe 'runtime' information while everything else is static and comes\nfrom the control definition.\n\nNow, do we need to duplicate those information ? Can we just use Entry\nand only store data in second half of the buffer ?\n\nThis won't allow you to reconstruct a \"second half\" without the \"first\nhalf\" but is this actually a use case ?\n\n> +\t};\n> +\n> +\tstruct State {\n> +\t\tstd::uint32_t count;\n> +\t\tstd::uint32_t fill;\n> +\t};\n> +\n> +public:\n> +\texplicit MetadataList(const MetadataListPlan &plan)\n> +\t\t: capacity_(plan.size()),\n> +\t\t contentOffset_(MetadataList::contentOffset(capacity_)),\n> +\t\t alloc_(contentOffset_)\n\nWhy is the constructor inlined in the .h file ?\n\nI guess if you move it to the .cpp file you can make MetadataListPlan\nan internal header (just forward declare it here)\n\n> +\t{\n> +\t\tfor (const auto &[tag, e] : plan) {\n> +\t\t\tassert(details::cxx20::has_single_bit(e.alignment));\n> +\n> +\t\t\talloc_ += sizeof(ValueHeader);\n\nalloc_ is initialized with contentOffset_ which if I'm not mistaken\nit's default initialized to -1 ?\n\n> +\t\t\talloc_ += e.alignment - 1; // XXX: this is the maximum\n\nWhat are XXX comments ? Are these leftovers ?\n\n> +\t\t\talloc_ += e.size;\n> +\t\t\talloc_ += alignof(ValueHeader) - 1; // XXX: this is the maximum\n\nMaybe\n\n\t\t\talloc_ += sizeof(ValueHeader) + alignof(ValueHeader) - 1;\n\t\t\talloc_ += e.size + e.alignment - 1;\n\nI might be missing why the -1s ?\n\n> +\t\t}\n> +\n> +\t\tp_ = static_cast(::operator new(alloc_));\n> +\n> +\t\tauto *entries = reinterpret_cast(p_ + entriesOffset());\n\nWhat is entriesOffset() and why it's here if it's defined as\n\n\t[[nodiscard]] static constexpr std::size_t entriesOffset()\n\t{\n\t\treturn 0;\n\t}\n\n> +\t\tauto it = plan.begin();\n> +\n> +\t\tfor (std::size_t i = 0; i < capacity_; i++, ++it) {\n\nit's a shame we can't use utils::enumerate() in public headers\n\n> +\t\t\tconst auto &[tag, e] = *it;\n> +\n> +\t\t\tnew (&entries[i]) Entry{\n> +\t\t\t\t.tag = tag,\n> +\t\t\t\t.capacity = e.size,\n> +\t\t\t\t.alignment = e.alignment,\n> +\t\t\t\t.type = e.type,\n> +\t\t\t\t.isArray = e.isArray,\n> +\t\t\t};\n> +\t\t}\n> +\n> +#if HAS_ASAN\n> +\t\t::__sanitizer_annotate_contiguous_container(\n> +\t\t\tp_ + contentOffset_, p_ + alloc_,\n> +\t\t\tp_ + alloc_, p_ + contentOffset_\n> +\t\t);\n> +#endif\n> +\t}\n> +\n> +\tMetadataList(const MetadataList &other)\n> +\t\t: capacity_(other.capacity_),\n> +\t\t contentOffset_(other.contentOffset_),\n> +\t\t alloc_(other.alloc_),\n> +\t\t p_(static_cast(::operator new(alloc_)))\n> +\t{\n> +\t\tauto *entries = reinterpret_cast(p_ + entriesOffset());\n> +\t\tconst auto otherEntries = other.entries();\n> +\n> +\t\tfor (std::size_t i = 0; i < capacity_; i++) {\n> +\t\t\tauto *e = new (&entries[i]) Entry{\n> +\t\t\t\t.tag = otherEntries[i].tag,\n> +\t\t\t\t.capacity = otherEntries[i].capacity,\n> +\t\t\t\t.alignment = otherEntries[i].alignment,\n> +\t\t\t\t.type = otherEntries[i].type,\n> +\t\t\t\t.isArray = otherEntries[i].isArray,\n> +\t\t\t};\n> +\n> +\t\t\tauto v = other.data_of(otherEntries[i]);\n> +\t\t\tif (!v)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\t[[maybe_unused]] auto r = set(*e, v);\n> +\t\t\tassert(r == SetError());\n> +\t\t}\n> +\n> +#if HAS_ASAN\n> +\t\t::__sanitizer_annotate_contiguous_container(\n> +\t\t\tp_ + contentOffset_, p_ + alloc_,\n> +\t\t\tp_ + alloc_, p_ + contentOffset_\n> +\t\t);\n> +#endif\n> +\t}\n> +\n> +\tMetadataList(MetadataList &&) = delete;\n> +\n> +\tMetadataList &operator=(const MetadataList &) = delete;\n\nIf we have a copy constructor, shouldn't we have a copy assignment\noperator as well ?\n\n> +\tMetadataList &operator=(MetadataList &&) = delete;\n\nRule of 5 would say we need move assignement and constructor as well.\nTo me, given the fact a metadata list is a wrapper around a storage,\nit makes sense not to have them ?\n\n> +\n> +\t~MetadataList()\n> +\t{\n> +#if HAS_ASAN\n> +\t\t/*\n> +\t\t * The documentation says the range apparently has to be\n> +\t\t * restored to its initial state before it is deallocated.\n> +\t\t */\n> +\t\t::__sanitizer_annotate_contiguous_container(\n> +\t\t\tp_ + contentOffset_, p_ + alloc_,\n> +\t\t\tp_ + contentOffset_ + state_.load(std::memory_order_relaxed).fill, p_ + alloc_\n> +\t\t);\n> +#endif\n> +\n> +\t\t::operator delete(p_, alloc_);\n> +\t}\n> +\n> +\t// TODO: want these?\n> +\t[[nodiscard]] std::size_t size() const { return state_.load(std::memory_order_relaxed).count; }\n> +\t[[nodiscard]] bool empty() const { return size() == 0; }\n\nDo we use them ?\n\nI'll stop here, there's enough material to discuss I guess :)\n\nThanks, amazing progress on this front!\n\n> +\n> +\tenum class SetError {\n> +\t\tUnknownTag = 1,\n> +\t\tAlreadySet,\n> +\t\tDataTooLarge,\n> +\t\tTypeMismatch,\n> +\t};\n> +\n> +\t[[nodiscard]] SetError set(std::uint32_t tag, ControlValueView v)\n> +\t{\n> +\t\tauto *e = find(tag);\n> +\t\tif (!e)\n> +\t\t\treturn SetError::UnknownTag;\n> +\n> +\t\treturn set(*e, v);\n> +\t}\n> +\n> +\ttemplate\n> +\t/* TODO: [[nodiscard]] */ SetError set(const Control &ctrl, const details::cxx20::type_identity_t &value)\n> +\t{\n> +\t\tusing TypeInfo = libcamera::details::control_type;\n> +\n> +\t\tif constexpr (TypeInfo::size > 0) {\n> +\t\t\tstatic_assert(std::is_trivially_copyable_v);\n> +\n> +\t\t\treturn set(ctrl.id(), {\n> +\t\t\t\tTypeInfo::value,\n> +\t\t\t\ttrue,\n> +\t\t\t\tvalue.size(),\n> +\t\t\t\treinterpret_cast(value.data()),\n> +\t\t\t});\n> +\t\t} else {\n> +\t\t\tstatic_assert(std::is_trivially_copyable_v);\n> +\n> +\t\t\treturn set(ctrl.id(), {\n> +\t\t\t\tTypeInfo::value,\n> +\t\t\t\tfalse,\n> +\t\t\t\t1,\n> +\t\t\t\treinterpret_cast(&value),\n> +\t\t\t});\n> +\t\t}\n> +\t}\n> +\n> +\ttemplate\n> +\t[[nodiscard]] decltype(auto) get(const Control &ctrl) const\n> +\t{\n> +\t\tControlValueView v = get(ctrl.id());\n> +\n> +\t\treturn v ? std::optional(v.get()) : std::nullopt;\n> +\t}\n> +\n> +\t// TODO: operator ControlListView() const ?\n> +\t// TODO: explicit operator ControlList() const ?\n> +\n> +\t[[nodiscard]] ControlValueView get(std::uint32_t tag) const\n> +\t{\n> +\t\tconst auto *e = find(tag);\n> +\t\tif (!e)\n> +\t\t\treturn {};\n> +\n> +\t\treturn data_of(*e);\n> +\t}\n> +\n> +\tvoid clear()\n> +\t{\n> +\t\tfor (auto &e : entries())\n> +\t\t\te.headerOffset.store(Entry::invalidOffset, std::memory_order_relaxed);\n> +\n> +\t\t[[maybe_unused]] auto s = state_.exchange({}, std::memory_order_relaxed);\n> +\n> +#if HAS_ASAN\n> +\t\t::__sanitizer_annotate_contiguous_container(\n> +\t\t\tp_ + contentOffset_, p_ + alloc_,\n> +\t\t\tp_ + contentOffset_ + s.fill, p_ + contentOffset_\n> +\t\t);\n> +#endif\n> +\t}\n> +\n> +\tclass iterator\n> +\t{\n> +\tpublic:\n> +\t\tusing difference_type = std::ptrdiff_t;\n> +\t\tusing value_type = std::pair;\n> +\t\tusing pointer = void;\n> +\t\tusing reference = value_type;\n> +\t\tusing iterator_category = std::forward_iterator_tag;\n> +\n> +\t\titerator() = default;\n> +\n> +\t\titerator& operator++()\n> +\t\t{\n> +\t\t\tconst auto &h = header();\n> +\n> +\t\t\tp_ += sizeof(h);\n> +\t\t\tp_ = details::align::up(p_, h.alignment);\n> +\t\t\tp_ += h.size;\n> +\t\t\tp_ = details::align::up(p_, alignof(decltype(h)));\n> +\n> +\t\t\treturn *this;\n> +\t\t}\n> +\n> +\t\titerator operator++(int)\n> +\t\t{\n> +\t\t\tauto copy = *this;\n> +\t\t\t++*this;\n> +\t\t\treturn copy;\n> +\t\t}\n> +\n> +\t\t[[nodiscard]] reference operator*() const\n> +\t\t{\n> +\t\t\tconst auto &h = header();\n> +\t\t\tconst auto *data = details::align::up(p_ + sizeof(h), h.alignment);\n> +\n> +\t\t\treturn { h.tag, { h.params.type, h.params.isArray, h.params.numElements, data } };\n> +\t\t}\n> +\n> +\t\t[[nodiscard]] bool operator==(const iterator &other) const\n> +\t\t{\n> +\t\t\treturn p_ == other.p_;\n> +\t\t}\n> +\n> +\t\t[[nodiscard]] bool operator!=(const iterator &other) const\n> +\t\t{\n> +\t\t\treturn !(*this == other);\n> +\t\t}\n> +\n> +\tprivate:\n> +\t\titerator(const std::byte *p)\n> +\t\t\t: p_(p)\n> +\t\t{\n> +\t\t}\n> +\n> +\t\t[[nodiscard]] const ValueHeader &header() const\n> +\t\t{\n> +\t\t\treturn *reinterpret_cast(p_);\n> +\t\t}\n> +\n> +\t\tfriend MetadataList;\n> +\n> +\t\tconst std::byte *p_ = nullptr;\n> +\t};\n> +\n> +\t[[nodiscard]] iterator begin() const\n> +\t{\n> +\t\treturn { p_ + contentOffset_ };\n> +\t}\n> +\n> +\t[[nodiscard]] iterator end() const\n> +\t{\n> +\t\treturn { p_ + contentOffset_ + state_.load(std::memory_order_acquire).fill };\n> +\t}\n> +\n> +\tclass Diff\n> +\t{\n> +\tpublic:\n> +\t\t// TODO: want these?\n> +\t\t[[nodiscard]] explicit operator bool() const { return !empty(); }\n> +\t\t[[nodiscard]] bool empty() const { return start_ == stop_; }\n> +\t\t[[nodiscard]] std::size_t size() const { return changed_; }\n> +\t\t[[nodiscard]] const MetadataList &list() const { return *l_; }\n> +\n> +\t\t[[nodiscard]] ControlValueView get(std::uint32_t tag) const\n> +\t\t{\n> +\t\t\tconst auto *e = l_->find(tag);\n> +\t\t\tif (!e)\n> +\t\t\t\treturn {};\n> +\n> +\t\t\tauto o = e->acquireData();\n> +\t\t\tif (!o)\n> +\t\t\t\treturn {};\n> +\n> +\t\t\tif (!(start_ <= *o && *o < stop_))\n> +\t\t\t\treturn {};\n> +\n> +\t\t\treturn l_->data_of(*o);\n> +\t\t}\n> +\n> +\t\ttemplate\n> +\t\t[[nodiscard]] decltype(auto) get(const Control &ctrl) const\n> +\t\t{\n> +\t\t\tControlValueView v = get(ctrl.id());\n> +\n> +\t\t\treturn v ? std::optional(v.get()) : std::nullopt;\n> +\t\t}\n> +\n> +\t\t[[nodiscard]] iterator begin() const\n> +\t\t{\n> +\t\t\treturn { l_->p_ + start_ };\n> +\t\t}\n> +\n> +\t\t[[nodiscard]] iterator end() const\n> +\t\t{\n> +\t\t\treturn { l_->p_ + stop_ };\n> +\t\t}\n> +\n> +\tprivate:\n> +\t\tDiff(const MetadataList &l, std::size_t changed, std::size_t oldFill, std::size_t newFill)\n> +\t\t\t: l_(&l),\n> +\t\t\t changed_(changed),\n> +\t\t\t start_(l.contentOffset_ + oldFill),\n> +\t\t\t stop_(l.contentOffset_ + newFill)\n> +\t\t{\n> +\t\t}\n> +\n> +\t\tfriend MetadataList;\n> +\t\tfriend struct Checkpoint;\n> +\n> +\t\tconst MetadataList *l_ = nullptr;\n> +\t\tstd::size_t changed_;\n> +\t\tstd::size_t start_;\n> +\t\tstd::size_t stop_;\n> +\t};\n> +\n> +\tDiff merge(const MetadataList &other)\n> +\t{\n> +\t\tconst auto entries = this->entries();\n> +\t\tconst auto otherEntries = other.entries();\n> +\t\tconst auto c = checkpoint();\n> +\n> +\t\tfor (std::size_t i = 0, j = 0; i < entries.size() && j < otherEntries.size(); ) {\n> +\t\t\tif (entries[i].tag < otherEntries[j].tag) {\n> +\t\t\t\ti += 1;\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> +\n> +\t\t\tif (entries[i].tag > otherEntries[j].tag) {\n> +\t\t\t\tj += 1;\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> +\n> +\t\t\tassert(entries[i].alignment >= otherEntries[j].alignment);\n> +\t\t\tassert(entries[i].capacity >= otherEntries[j].capacity);\n> +\n> +\t\t\tif (!entries[i].hasValue()) {\n> +\t\t\t\tauto v = other.data_of(otherEntries[j]);\n> +\t\t\t\tif (v) {\n> +\t\t\t\t\t[[maybe_unused]] auto r = set(entries[i], v);\n> +\t\t\t\t\tassert(r == SetError());\n> +\t\t\t\t}\n> +\t\t\t}\n> +\n> +\t\t\ti += 1;\n> +\t\t\tj += 1;\n> +\t\t}\n> +\n> +\t\treturn c.diffSince();\n> +\t}\n> +\n> +\tDiff merge(const ControlList &other)\n> +\t{\n> +\t\t// TODO: check id map of `other`?\n> +\n> +\t\tconst auto c = checkpoint();\n> +\n> +\t\tfor (auto &&[tag, value] : other) {\n> +\t\t\tauto *e = find(tag);\n> +\t\t\tif (e) {\n> +\t\t\t\t[[maybe_unused]] auto r = set(*e, value);\n> +\t\t\t\tassert(r == SetError() || r == SetError::AlreadySet); // TODO: ?\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\treturn c.diffSince();\n> +\t}\n> +\n> +\tclass Checkpoint\n> +\t{\n> +\tpublic:\n> +\t\t// TODO: want this?\n> +\t\t[[nodiscard]] const MetadataList &list() const { return *l_; }\n> +\n> +\t\t[[nodiscard]] Diff diffSince() const\n> +\t\t{\n> +\t\t\t/* sync with release-store on `state_` in `set()` */\n> +\t\t\tconst auto curr = l_->state_.load(std::memory_order_acquire);\n> +\n> +\t\t\tassert(s_.count <= curr.count);\n> +\t\t\tassert(s_.fill <= curr.fill);\n> +\n> +\t\t\treturn {\n> +\t\t\t\t*l_,\n> +\t\t\t\tcurr.count - s_.count,\n> +\t\t\t\ts_.fill,\n> +\t\t\t\tcurr.fill,\n> +\t\t\t};\n> +\t\t}\n> +\n> +\tprivate:\n> +\t\tCheckpoint(const MetadataList &l)\n> +\t\t\t: l_(&l),\n> +\t\t\t s_(l.state_.load(std::memory_order_relaxed))\n> +\t\t{\n> +\t\t}\n> +\n> +\t\tfriend MetadataList;\n> +\n> +\t\tconst MetadataList *l_ = nullptr;\n> +\t\tState s_ = {};\n> +\t};\n> +\n> +\t[[nodiscard]] Checkpoint checkpoint() const\n> +\t{\n> +\t\treturn { *this };\n> +\t}\n> +\n> +private:\n> +\t[[nodiscard]] static constexpr std::size_t entriesOffset()\n> +\t{\n> +\t\treturn 0;\n> +\t}\n> +\n> +\t[[nodiscard]] static constexpr std::size_t contentOffset(std::size_t entries)\n> +\t{\n> +\t\treturn details::align::up(entriesOffset() + entries * sizeof(Entry), alignof(ValueHeader));\n> +\t}\n> +\n> +\t[[nodiscard]] Span entries() const\n> +\t{\n> +\t\treturn { reinterpret_cast(p_ + entriesOffset()), capacity_ };\n> +\t}\n> +\n> +\t[[nodiscard]] Entry *find(std::uint32_t tag) const\n> +\t{\n> +\t\tconst auto entries = this->entries();\n> +\t\tauto it = std::lower_bound(entries.begin(), entries.end(), tag, [](const auto &e, const auto &t) {\n> +\t\t\treturn e.tag < t;\n> +\t\t});\n> +\n> +\t\tif (it == entries.end() || it->tag != tag)\n> +\t\t\treturn nullptr;\n> +\n> +\t\treturn &*it;\n> +\t}\n> +\n> +\t[[nodiscard]] ControlValueView data_of(const Entry &e) const\n> +\t{\n> +\t\tconst auto o = e.acquireData();\n> +\t\treturn o ? data_of(*o) : ControlValueView{ };\n> +\t}\n> +\n> +\t[[nodiscard]] ControlValueView data_of(std::size_t headerOffset) const\n> +\t{\n> +\t\tassert(headerOffset <= alloc_ - sizeof(ValueHeader));\n> +\t\tassert(details::align::is(p_ + headerOffset, alignof(ValueHeader)));\n> +\n> +\t\tconst auto *vh = reinterpret_cast(p_ + headerOffset);\n> +\t\tconst auto *p = reinterpret_cast(vh) + sizeof(*vh);\n> +\t\tstd::size_t avail = p_ + alloc_ - p;\n> +\n> +\t\tconst auto *data = details::align::up(vh->size, vh->alignment, p, &avail);\n> +\t\tassert(data);\n> +\n> +\t\treturn { vh->params.type, vh->params.isArray, vh->params.numElements, data };\n> +\t}\n> +\n> +\t[[nodiscard]] SetError set(Entry &e, ControlValueView v)\n> +\t{\n> +\t\tif (e.hasValue())\n> +\t\t\treturn SetError::AlreadySet;\n> +\t\tif (e.type != v.type() || e.isArray != v.isArray())\n> +\t\t\treturn SetError::TypeMismatch;\n> +\n> +\t\tconst auto src = v.data();\n> +\t\tif (e.isArray) {\n> +\t\t\tif (src.size_bytes() > e.capacity)\n> +\t\t\t\treturn SetError::DataTooLarge;\n> +\t\t} else {\n> +\t\t\tassert(src.size_bytes() == e.capacity);\n> +\t\t}\n> +\n> +\t\tauto s = state_.load(std::memory_order_relaxed);\n> +\t\tstd::byte *oldEnd = p_ + contentOffset_ + s.fill;\n> +\t\tstd::byte *p = oldEnd;\n> +\n> +\t\tauto *headerPtr = details::align::up(p);\n> +\t\tauto *dataPtr = details::align::up(src.size_bytes(), e.alignment, p);\n> +\t\tdetails::align::up(0, alignof(ValueHeader), p);\n> +\n> +#if HAS_ASAN\n> +\t\t::__sanitizer_annotate_contiguous_container(\n> +\t\t\tp_ + contentOffset_, p_ + alloc_,\n> +\t\t\toldEnd, p\n> +\t\t);\n> +#endif\n> +\n> +\t\tnew (headerPtr) ValueHeader{\n> +\t\t\t.tag = e.tag,\n> +\t\t\t.size = std::uint32_t(src.size_bytes()),\n> +\t\t\t.alignment = e.alignment,\n> +\t\t\t.params = {\n> +\t\t\t\t.type = v.type(),\n> +\t\t\t\t.isArray = v.isArray(),\n> +\t\t\t\t.numElements = std::uint32_t(v.numElements()),\n> +\t\t\t},\n> +\t\t};\n> +\t\tstd::memcpy(dataPtr, src.data(), src.size_bytes());\n> +\t\te.headerOffset.store(reinterpret_cast(headerPtr) - p_, std::memory_order_release);\n> +\n> +\t\ts.fill += p - oldEnd;\n> +\t\ts.count += 1;\n> +\n> +\t\tstate_.store(s, std::memory_order_release);\n> +\n> +\t\treturn {};\n> +\t}\n> +\n> +\tstd::size_t capacity_ = 0;\n> +\tstd::size_t contentOffset_ = -1;\n> +\tstd::size_t alloc_ = 0;\n> +\tstd::atomic state_ = State{};\n> +\tstd::byte *p_ = nullptr;\n> +\t// TODO: ControlIdMap in any way shape or form?\n> +\n> +\t/*\n> +\t * If this is problematic on a 32-bit architecture, then\n> +\t * `count` can be stored in a separate atomic variable\n> +\t * but then `Diff::changed_` must be removed since the fill\n> +\t * level and item count cannot be retrieved atomically.\n> +\t */\n> +\tstatic_assert(decltype(state_)::is_always_lock_free);\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#undef HAS_ASAN\n> diff --git a/include/libcamera/metadata_list_plan.h b/include/libcamera/metadata_list_plan.h\n> new file mode 100644\n> index 000000000..9ad4ae87e\n> --- /dev/null\n> +++ b/include/libcamera/metadata_list_plan.h\n> @@ -0,0 +1,109 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2025, Ideas On Board Oy\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class MetadataListPlan\n> +{\n> +public:\n> +\t[[nodiscard]] bool empty() const { return items_.empty(); }\n> +\t[[nodiscard]] std::size_t size() const { return items_.size(); }\n> +\t[[nodiscard]] decltype(auto) begin() const { return items_.begin(); }\n> +\t[[nodiscard]] decltype(auto) end() const { return items_.end(); }\n> +\tvoid clear() { items_.clear(); }\n> +\n> +\ttemplate<\n> +\t\ttypename T,\n> +\t\tstd::enable_if_t::size != libcamera::dynamic_extent> * = nullptr\n> +\t>\n> +\tdecltype(auto) add(const Control &ctrl)\n> +\t{\n> +\t\tif constexpr (libcamera::details::control_type::size > 0) {\n> +\t\t\tstatic_assert(libcamera::details::control_type::size != libcamera::dynamic_extent);\n> +\n> +\t\t\treturn add(\n> +\t\t\t\tctrl.id(),\n> +\t\t\t\tlibcamera::details::control_type::size,\n> +\t\t\t\ttrue\n> +\t\t\t);\n> +\t\t} else {\n> +\t\t\treturn add(ctrl.id(), 1, false);\n> +\t\t}\n> +\t}\n> +\n> +\ttemplate<\n> +\t\ttypename T,\n> +\t\tstd::enable_if_t::size == libcamera::dynamic_extent> * = nullptr\n> +\t>\n> +\tdecltype(auto) add(const Control &ctrl, std::size_t count)\n> +\t{\n> +\t\treturn add(ctrl.id(), count, true);\n> +\t}\n> +\n> +#ifndef __DOXYGEN__\n> +\tMetadataListPlan &add(std::uint32_t tag,\n> +\t\t\t std::size_t size, std::size_t count, std::size_t alignment,\n> +\t\t\t ControlType type, bool isArray)\n> +\t{\n> +\t\tassert(size > 0 && size <= std::numeric_limits::max());\n> +\t\tassert(count <= std::numeric_limits::max() / size);\n> +\t\tassert(alignment <= std::numeric_limits::max());\n> +\t\tassert(details::cxx20::has_single_bit(alignment));\n> +\t\tassert(isArray || count == 1);\n> +\n> +\t\titems_.insert_or_assign(tag, Entry{\n> +\t\t\t.size = std::uint32_t(size * count),\n> +\t\t\t.alignment = std::uint32_t(alignment),\n> +\t\t\t.type = type,\n> +\t\t\t.isArray = isArray,\n> +\t\t});\n> +\n> +\t\treturn *this;\n> +\t}\n> +#endif\n> +\n> +\tbool remove(std::uint32_t tag)\n> +\t{\n> +\t\treturn items_.erase(tag);\n> +\t}\n> +\n> +\tbool remove(const ControlId &ctrl)\n> +\t{\n> +\t\treturn remove(ctrl.id());\n> +\t}\n> +\n> +private:\n> +\tstruct Entry {\n> +\t\tstd::uint32_t size;\n> +\t\tstd::uint32_t alignment; // TODO: is this necessary?\n> +\t\tControlType type;\n> +\t\tbool isArray;\n> +\t};\n> +\n> +\tstd::map items_;\n> +\n> +\ttemplate\n> +\tdecltype(auto) add(std::uint32_t tag, std::size_t count, bool isArray)\n> +\t{\n> +\t\tstatic_assert(std::is_trivially_copyable_v);\n> +\n> +\t\treturn add(tag, sizeof(T), count, alignof(T), details::control_type::value, isArray);\n> +\t}\n> +};\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 202db1efe..d7a850907 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -9,6 +9,7 @@ libcamera_public_sources = files([\n> 'framebuffer.cpp',\n> 'framebuffer_allocator.cpp',\n> 'geometry.cpp',\n> + 'metadata_list.cpp',\n> 'orientation.cpp',\n> 'pixel_format.cpp',\n> 'request.cpp',\n> diff --git a/src/libcamera/metadata_list.cpp b/src/libcamera/metadata_list.cpp\n> new file mode 100644\n> index 000000000..ccfe37318\n> --- /dev/null\n> +++ b/src/libcamera/metadata_list.cpp\n> @@ -0,0 +1,315 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2025, Ideas On Board Oy\n> + */\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\class MetadataListPlan\n> + * \\brief Class to hold the possible set of metadata items for a \\ref MetadataList\n> + */\n> +\n> +/**\n> + * \\fn MetadataListPlan::begin() const\n> + * \\brief Retrieve the begin iterator\n> + */\n> +\n> +/**\n> + * \\fn MetadataListPlan::end() const\n> + * \\brief Retrieve the end iterator\n> + */\n> +\n> +/**\n> + * \\fn MetadataListPlan::size() const\n> + * \\brief Retrieve the number of controls\n> + */\n> +\n> +/**\n> + * \\fn MetadataListPlan::empty() const\n> + * \\brief Check if empty\n> + */\n> +\n> +/**\n> + * \\fn MetadataListPlan::clear()\n> + * \\brief Remove all controls\n> + */\n> +\n> +/**\n> + * \\fn MetadataListPlan::add(const Control &ctrl)\n> + * \\brief Add a control to the metadata list plan\n> + */\n> +\n> +/**\n> + * \\fn MetadataListPlan::add(const Control &ctrl, std::size_t count)\n> + * \\brief Add a dynamically-sized control to the metadata list plan\n> + * \\param[in] ctrl The control\n> + * \\param[in] count The maximum number of elements\n> + *\n> + * Add the dynamically-sized control \\a ctrl to the metadata list plan with a maximum\n> + * capacity of \\a count elements.\n> + */\n> +\n> +/**\n> + * \\fn MetadataListPlan::remove(std::uint32_t tag)\n> + * \\brief Remove the entry with given identifier from the plan\n> + */\n> +\n> +/**\n> + * \\fn MetadataListPlan::remove(const ControlId &ctrl)\n> + * \\brief Remove \\a ctrl from the plan\n> + */\n> +\n> +/**\n> + * \\class MetadataList\n> + * \\brief Class to hold metadata items\n> + */\n> +\n> +/**\n> + * \\fn MetadataList::MetadataList(const MetadataListPlan &plan)\n> + * \\brief Construct a metadata list according to \\a plan\n> + *\n> + * Construct a metadata list according to the provided \\a plan.\n> + */\n> +\n> +/**\n> + * \\fn MetadataList::MetadataList(const MetadataList &other)\n> + * \\brief Copy constructor\n> + * \\context This function is \\threadsafe wrt. \\a other.\n> + */\n> +\n> +/**\n> + * \\fn MetadataList::size() const\n> + * \\brief Retrieve the number of controls\n> + * \\context This function is \\threadsafe.\n> + * \\note If the list is being modified, the return value may be out of\n> + * date by the time the function returns\n> + */\n> +\n> +/**\n> + * \\fn MetadataList::empty() const\n> + * \\brief Check if empty\n> + * \\context This function is \\threadsafe.\n> + * \\note If the list is being modified, the return value may be out of\n> + * date by the time the function returns\n> + */\n> +\n> +/**\n> + * \\internal\n> + * \\fn MetadataList::clear()\n> + * \\brief Remove all items from the list\n> + * \\note This function in effect resets the list to its original state. As a consequence it invalidates - among others -\n> + * all iterators, Checkpoint, and Diff objects that are associated with the list. No readers must exist\n> + * when this function is called.\n> + */\n> +\n> +/**\n> + * \\fn MetadataList::begin() const\n> + * \\brief Retrieve begin iterator\n> + * \\context This function is \\threadsafe.\n> + */\n> +\n> +/**\n> + * \\fn MetadataList::end() const\n> + * \\brief Retrieve end iterator\n> + * \\context This function is \\threadsafe.\n> + */\n> +\n> +/**\n> + * \\fn MetadataList::get(const Control &ctrl) const\n> + * \\brief Get the value of control \\a ctrl\n> + * \\return A std::optional containing the control value, or std::nullopt if\n> + * the control \\a ctrl is not present in the list\n> + * \\context This function is \\threadsafe.\n> + */\n> +\n> +/**\n> + * \\fn MetadataList::get(std::uint32_t tag) const\n> + * \\brief Get the value of pertaining to the numeric identifier \\a tag\n> + * \\return A std::optional containing the control value, or std::nullopt if\n> + * the control is not present in the list\n> + * \\context This function is \\threadsafe.\n> + */\n> +\n> +/**\n> + * \\fn MetadataList::set(const Control &ctrl, const details::cxx20::type_identity_t &value)\n> + * \\brief Set the value of control \\a ctrl to \\a value\n> + */\n> +\n> +/**\n> + * \\fn MetadataList::set(std::uint32_t tag, ControlValueView v)\n> + * \\brief Set the value of pertaining to the numeric identifier \\a tag to \\a v\n> + */\n> +\n> +/**\n> + * \\internal\n> + * \\fn MetadataList::merge(const MetadataList &other)\n> + * \\brief Add all missing items from \\a other\n> + *\n> + * Add all items from \\a other that are not present in \\a this. If an item\n> + * has a numeric identifier that was not present in the MetadataListPlan\n> + * used to construct \\a this, then the item is ignored.\n> + *\n> + * \\context This function is \\threadsafe wrt. \\a other.\n> + */\n> +\n> +/**\n> + * \\internal\n> + * \\fn MetadataList::merge(const ControlList &other)\n> + * \\copydoc MetadataList::merge(const MetadataList &other)\n> + */\n> +\n> +/**\n> + * \\enum MetadataList::SetError\n> + * \\brief TODO\n> + *\n> + * \\var MetadataList::SetError::UnknownTag\n> + * \\brief The tag is not supported by the metadata list\n> + * \\var MetadataList::SetError::AlreadySet\n> + * \\brief A value has already been added with the given tag\n> + * \\var MetadataList::SetError::DataTooLarge\n> + * \\brief The data is too large for the given tag\n> + * \\var MetadataList::SetError::TypeMismatch\n> + * \\brief The type of the value does not match the expected type\n> + */\n> +\n> +/**\n> + * \\internal\n> + * \\fn MetadataList::checkpoint() const\n> + * \\brief Create a checkpoint\n> + * \\context This function is \\threadsafe.\n> + */\n> +\n> +/**\n> + * \\class MetadataList::iterator\n> + * \\brief Iterator\n> + */\n> +\n> +/**\n> + * \\typedef MetadataList::iterator::difference_type\n> + * \\brief iterator's difference type\n> + */\n> +\n> +/**\n> + * \\typedef MetadataList::iterator::value_type\n> + * \\brief iterator's value type\n> + */\n> +\n> +/**\n> + * \\typedef MetadataList::iterator::pointer\n> + * \\brief iterator's pointer type\n> + */\n> +\n> +/**\n> + * \\typedef MetadataList::iterator::reference\n> + * \\brief iterator's reference type\n> + */\n> +\n> +/**\n> + * \\typedef MetadataList::iterator::iterator_category\n> + * \\brief iterator's category\n> + */\n> +\n> +/**\n> + * \\fn MetadataList::iterator::operator*()\n> + * \\brief Retrieve value at iterator\n> + * \\return A \\a ControlListView representing the value\n> + */\n> +\n> +/**\n> + * \\fn MetadataList::iterator::operator==(const iterator &other) const\n> + * \\brief Check if two iterators are equal\n> + */\n> +\n> +/**\n> + * \\fn MetadataList::iterator::operator!=(const iterator &other) const\n> + * \\brief Check if two iterators are not equal\n> + */\n> +\n> +/**\n> + * \\fn MetadataList::iterator::operator++(int)\n> + * \\brief Advance the iterator\n> + */\n> +\n> +/**\n> + * \\fn MetadataList::iterator::operator++()\n> + * \\brief Advance the iterator\n> + */\n> +\n> +/**\n> + * \\class MetadataList::Diff\n> + * \\brief Designates a set of consecutively added metadata items from a particular MetadataList\n> + * \\sa Camera::metadataAvailable\n> + * \\internal\n> + * \\sa MetadataList::Checkpoint::diffSince()\n> + * \\endinternal\n> + */\n> +\n> +/**\n> + * \\fn MetadataList::Diff::list() const\n> + * \\brief Retrieve the associated MetadataList\n> + */\n> +\n> +/**\n> + * \\fn MetadataList::Diff::size() const\n> + * \\brief Retrieve the number of metadata items designated\n> + */\n> +\n> +/**\n> + * \\fn MetadataList::Diff::empty() const\n> + * \\brief Check if no metadata items are designated\n> + */\n> +\n> +/**\n> + * \\fn MetadataList::Diff::operator bool() const\n> + * \\copydoc MetadataList::Diff::empty() const\n> + */\n> +\n> +/**\n> + * \\fn MetadataList::Diff::get(const Control &ctrl) const\n> + * \\copydoc MetadataList::get(const Control &ctrl) const\n> + * \\note The value pertaining to \\a ctrl will only be returned if it is part of Diff,\n> + * meaning that even if \\a ctrl is part of the backing MetadataList, it will not\n> + * be returned if \\a ctrl is not in the set of controls designated by this Diff object.\n> + */\n> +\n> +/**\n> + * \\fn MetadataList::Diff::get(std::uint32_t tag) const\n> + * \\copydoc MetadataList::Diff::get(const Control&ctrl) const\n> + */\n> +\n> +/**\n> + * \\fn MetadataList::Diff::begin() const\n> + * \\brief Retrieve the begin iterator\n> + */\n> +\n> +/**\n> + * \\fn MetadataList::Diff::end() const\n> + * \\brief Retrieve the end iterator\n> + */\n> +\n> +/**\n> + * \\internal\n> + * \\class MetadataList::Checkpoint\n> + * \\brief Designates a point in the stream of metadata items\n> + *\n> + * A Checkpoint object designates a point in the stream of metadata items in the associated\n> + * MetadataList. Its main use to be able to retrieve the set of metadata items that were\n> + * added to the list after the designated point using diffSince().\n> + */\n> +\n> +/**\n> + * \\internal\n> + * \\fn MetadataList::Checkpoint::list() const\n> + * \\brief Retrieve the associated \\ref MetadataList\n> + */\n> +\n> +/**\n> + * \\internal\n> + * \\fn MetadataList::Checkpoint::diffSince() const\n> + * \\brief Retrieve the set of metadata items added since the checkpoint was created\n> + */\n> +\n> +} /* namespace libcamera */\n> diff --git a/test/controls/meson.build b/test/controls/meson.build\n> index 763f8905e..b68a4fc53 100644\n> --- a/test/controls/meson.build\n> +++ b/test/controls/meson.build\n> @@ -5,6 +5,7 @@ control_tests = [\n> {'name': 'control_info_map', 'sources': ['control_info_map.cpp']},\n> {'name': 'control_list', 'sources': ['control_list.cpp']},\n> {'name': 'control_value', 'sources': ['control_value.cpp']},\n> + {'name': 'metadata_list', 'sources': ['metadata_list.cpp']},\n> ]\n>\n> foreach test : control_tests\n> diff --git a/test/controls/metadata_list.cpp b/test/controls/metadata_list.cpp\n> new file mode 100644\n> index 000000000..e02b4e28e\n> --- /dev/null\n> +++ b/test/controls/metadata_list.cpp\n> @@ -0,0 +1,171 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2025, Ideas On Board Oy\n> + *\n> + * MetadataList tests\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \"test.h\"\n> +\n> +using namespace std;\n> +using namespace libcamera;\n> +\n> +#define ASSERT(x) do { \\\n> +\tif (!static_cast(x)) { \\\n> +\t\tstd::cerr << '`' << #x << \"` failed\" << std::endl; \\\n> +\t\treturn TestFail; \\\n> +\t} \\\n> +} while (false)\n> +\n> +class MetadataListTest : public Test\n> +{\n> +public:\n> +\tMetadataListTest() = default;\n> +\n> +protected:\n> +\tint run() override\n> +\t{\n> +\t\tMetadataListPlan mlp;\n> +\t\tmlp.add(controls::ExposureTime);\n> +\t\tmlp.add(controls::ExposureValue);\n> +\t\tmlp.add(controls::ColourGains);\n> +\t\tmlp.add(controls::AfWindows, 10);\n> +\t\tmlp.add(controls::AeEnable);\n> +\t\tmlp.add(controls::SensorTimestamp);\n> +\n> +\t\tMetadataList ml(mlp);\n> +\n> +\t\tstatic_assert(static_cast(properties::LOCATION) == controls::AE_ENABLE);\n> +\t\tASSERT(ml.set(properties::Location, properties::CameraLocationFront) == MetadataList::SetError::TypeMismatch);\n> +\n> +\t\tASSERT(ml.set(controls::AfWindows, std::array{}) == MetadataList::SetError::DataTooLarge);\n> +\t\tASSERT(ml.set(controls::ColourTemperature, 123) == MetadataList::SetError::UnknownTag);\n> +\n> +\t\tauto f1 = std::async(std::launch::async, [&] {\n> +\t\t\tusing namespace std::chrono_literals;\n> +\n> +\t\t\tstd::this_thread::sleep_for(500ms);\n> +\t\t\tASSERT(ml.set(controls::ExposureTime, 0x1111) == MetadataList::SetError());\n> +\n> +\t\t\tstd::this_thread::sleep_for(500ms);\n> +\t\t\tASSERT(ml.set(controls::ExposureValue, 1) == MetadataList::SetError());\n> +\n> +\t\t\tstd::this_thread::sleep_for(500ms);\n> +\t\t\tASSERT(ml.set(controls::ColourGains, std::array{\n> +\t\t\t\t123.f,\n> +\t\t\t\t456.f\n> +\t\t\t}) == MetadataList::SetError());\n> +\n> +\t\t\tstd::this_thread::sleep_for(500ms);\n> +\t\t\tASSERT(ml.set(controls::AfWindows, std::array{\n> +\t\t\t\tRectangle(),\n> +\t\t\t\tRectangle(1, 2, 3, 4),\n> +\t\t\t\tRectangle(0x1111, 0x2222, 0x3333, 0x4444),\n> +\t\t\t}) == MetadataList::SetError());\n> +\n> +\t\t\treturn TestPass;\n> +\t\t});\n> +\n> +\t\tauto f2 = std::async(std::launch::async, [&] {\n> +\t\t\tfor (;;) {\n> +\t\t\t\tconst auto x = ml.get(controls::ExposureTime);\n> +\t\t\t\tconst auto y = ml.get(controls::ExposureValue);\n> +\t\t\t\tconst auto z = ml.get(controls::ColourGains);\n> +\t\t\t\tconst auto w = ml.get(controls::AfWindows);\n> +\n> +\t\t\t\tif (x)\n> +\t\t\t\t\tASSERT(*x == 0x1111);\n> +\n> +\t\t\t\tif (y)\n> +\t\t\t\t\tASSERT(*y == 1.0f);\n> +\n> +\t\t\t\tif (z) {\n> +\t\t\t\t\tASSERT(z->size() == 2);\n> +\t\t\t\t\tASSERT((*z)[0] == 123.f);\n> +\t\t\t\t\tASSERT((*z)[1] == 456.f);\n> +\t\t\t\t}\n> +\n> +\t\t\t\tif (w) {\n> +\t\t\t\t\tASSERT(w->size() == 3);\n> +\t\t\t\t\tASSERT((*w)[0].isNull());\n> +\t\t\t\t\tASSERT((*w)[1] == Rectangle(1, 2, 3, 4));\n> +\t\t\t\t\tASSERT((*w)[2] == Rectangle(0x1111, 0x2222, 0x3333, 0x4444));\n> +\t\t\t\t}\n> +\n> +\t\t\t\tif (x && y && z && w)\n> +\t\t\t\t\tbreak;\n> +\t\t\t}\n> +\n> +\t\t\treturn TestPass;\n> +\t\t});\n> +\n> +\t\tASSERT(f1.get() == TestPass);\n> +\t\tASSERT(f2.get() == TestPass);\n> +\n> +\t\tASSERT(ml.set(controls::ExposureTime, 0x2222) == MetadataList::SetError::AlreadySet);\n> +\t\tASSERT(ml.set(controls::ExposureValue, 2) == MetadataList::SetError::AlreadySet);\n> +\n> +\t\tASSERT(ml.get(controls::ExposureTime) == 0x1111);\n> +\t\tASSERT(ml.get(controls::ExposureValue) == 1);\n> +\n> +\t\tfor (auto &&[tag, v] : ml)\n> +\t\t\tstd::cout << \"[\" << tag << \"] -> \" << v << '\\n';\n> +\n> +\t\tstd::cout << std::endl;\n> +\n> +\t\tml.clear();\n> +\t\tASSERT(ml.empty());\n> +\t\tASSERT(ml.size() == 0);\n> +\n> +\t\tASSERT(ml.set(controls::ExposureTime, 0x2222) == MetadataList::SetError());\n> +\t\tASSERT(ml.get(controls::ExposureTime) == 0x2222);\n> +\n> +\t\tauto c = ml.checkpoint();\n> +\t\tASSERT(&c.list() == &ml);\n> +\n> +\t\tASSERT(ml.set(controls::ExposureValue, 2) == MetadataList::SetError());\n> +\t\tASSERT(ml.set(controls::SensorTimestamp, 0x99999999) == MetadataList::SetError());\n> +\n> +\t\tauto d = c.diffSince();\n> +\t\tASSERT(&d.list() == &ml);\n> +\n> +\t\tASSERT(ml.set(controls::ColourGains, std::array{ 1.f, 2.f }) == MetadataList::SetError());\n> +\n> +\t\tASSERT(d);\n> +\t\tASSERT(!d.empty());\n> +\t\tASSERT(d.size() == 2);\n> +\t\tASSERT(!d.get(controls::ExposureTime));\n> +\t\tASSERT(!d.get(controls::ColourGains));\n> +\t\tASSERT(!d.get(controls::AfWindows));\n> +\t\tASSERT(d.get(controls::ExposureValue) == 2);\n> +\t\tASSERT(d.get(controls::SensorTimestamp) == 0x99999999);\n> +\n> +\t\tfor (auto &&[tag, v] : d)\n> +\t\t\tstd::cout << \"[\" << tag << \"] -> \" << v << '\\n';\n> +\n> +\t\t/* Test if iterators work with algorithms. */\n> +\t\tstd::ignore = std::find_if(d.begin(), d.end(), [](const auto &) {\n> +\t\t\treturn false;\n> +\t\t});\n> +\n> +#if 0\n> + {\n> + auto it = ml.begin();\n> + ml.clear();\n> + std::ignore = *it; /* Trigger ASAN. */\n> + }\n> +#endif\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +};\n> +\n> +TEST_REGISTER(MetadataListTest)\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 7BEF4C3237\n\tfor ;\n\tWed, 11 Jun 2025 11:59:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5BFA668DBD;\n\tWed, 11 Jun 2025 13:59:24 +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 BE89161551\n\tfor ;\n\tWed, 11 Jun 2025 13:59:22 +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 E87596BE;\n\tWed, 11 Jun 2025 13:59:13 +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=\"IVeKnAu6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1749643154;\n\tbh=7XxOUVzRTMs4LPsgfT5q466p1dYONjhfJtOJGnNC3DA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IVeKnAu6FWCmO45GMrGGxzHg0+GyYJdUbNe/sHYUmZJdL5VaaxreWW+5r/AjPhdno\n\tsDGdkf10zQLrF3a1nF9TyhqCIlZSn9c6dcadg/Tw8qbKc+TjY4UXPAT4KMpIqLDUCT\n\t9hCZ9TpQANPjLlAOkcKwJdPCqOnI25MqOgYtafTQ=","Date":"Wed, 11 Jun 2025 13:59:18 +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 07/23] libcamera: Add `MetadataList`","Message-ID":"","References":"<20250606164156.1442682-1-barnabas.pocze@ideasonboard.com>\n\t<20250606164156.1442682-8-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-8-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":34450,"web_url":"https://patchwork.libcamera.org/comment/34450/","msgid":"<9e6aa8d9-f016-46a7-9266-6491fd8ff4c0@ideasonboard.com>","date":"2025-06-11T13:27:20","subject":"Re: [RFC PATCH v1 07/23] libcamera: Add `MetadataList`","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 06. 11. 13:59 keltezéssel, Jacopo Mondi írta:\n> Hi Barnabás\n> \n> On Fri, Jun 06, 2025 at 06:41:40PM +0200, Barnabás Pőcze wrote:\n> [...]\n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +#include \n>> +#include \n>> +\n>> +// TODO: want this?\n> \n> I would say yes, but what if we move this to the main meson.build\n> file ?\n> \n> I currently see in test/meson.build\n> \n> ------------------------------------------------------------------------------\n> \n> # When ASan is enabled, find the path to the ASan runtime needed by multiple\n> # tests. This currently works with gcc only, as clang uses different file names\n> # depending on the compiler version and target architecture.\n> asan_enabled = false\n> asan_runtime_missing = false\n> \n> if get_option('b_sanitize').contains('address')\n> asan_enabled = true\n> \n> if cc.get_id() == 'gcc'\n> asan_runtime = run_command(cc, '-print-file-name=libasan.so', check : true).stdout().strip()\n> else\n> asan_runtime_missing = true\n> endif\n> endif\n> \n> ------------------------------------------------------------------------------\n> \n> \n> Could we define a library-wide HAS_ASAN symbol in src/meson.build in\n> example in your opinion ?\n\nI think it could make sense, yes. But note that with everything inline,\nthe asan integration also works only the user application is compiled\nwith asan.\n\n\n\n> \n>> +#if __has_include()\n>> +#if __SANITIZE_ADDRESS__ /* gcc */\n>> +#include \n>> +#define HAS_ASAN 1\n>> +#elif defined(__has_feature)\n>> +#if __has_feature(address_sanitizer) /* clang */\n>> +#include \n>> +#define HAS_ASAN 1\n>> +#endif\n>> +#endif\n>> +#endif\n>> +\n>> +namespace libcamera {\n>> +\n>> +class MetadataList\n>> +{\n>> +private:\n> \n> Not against it, but we usually don't mix private: and public:\n> \n> You could move the types definition at the beginning of the below\n> private: section\n\n\nI did try that multiple times, but as far as I remember the compiler was complaining.\n\n\n> \n>> +\tstruct ValueParams {\n>> +\t\tControlType type;\n>> +\t\tbool isArray;\n>> +\t\tstd::uint32_t numElements;\n> \n> Does prefixing a stdint type with std:: brings anything ? It's\n> certainly more C++, I guess\n> \n>> +\t};\n>> +\n>> +\tstruct Entry {\n>> +\t\tconst std::uint32_t tag;\n>> +\t\tconst std::uint32_t capacity;\n>> +\t\tconst std::uint32_t alignment;\n>> +\t\tconst ControlType type;\n>> +\t\tbool isArray;\n>> +\n>> +\t\tstatic constexpr std::uint32_t invalidOffset = -1;\n> \n> constant are usually kInvalidOffset\n> \n> \n> empty line maybe\n> \n>> +\t\t/*\n>> +\t\t* Offset from the beginning of the allocation, and\n>> +\t\t* and _not_ relative to `contentOffset_`.\n>> +\t\t*/\n> \n> weird indent\n> \n>> +\t\tstd::atomic_uint32_t headerOffset = invalidOffset;\n>> +\n>> +\t\t[[nodiscard]] std::optional hasValue() const\n>> +\t\t{\n>> +\t\t\tauto offset = headerOffset.load(std::memory_order_relaxed);\n>> +\t\t\tif (offset == invalidOffset)\n>> +\t\t\t\treturn {};\n>> +\n>> +\t\t\treturn offset;\n>> +\t\t}\n>> +\n>> +\t\t[[nodiscard]] std::optional acquireData() const\n>> +\t\t{\n>> +\t\t\tauto offset = hasValue();\n>> +\t\t\tif (offset) {\n>> +\t\t\t\t/* sync with release-store on `headerOffset` in `MetadataList::set()` */\n>> +\t\t\t\tstd::atomic_thread_fence(std::memory_order_acquire);\n>> +\t\t\t}\n>> +\n>> +\t\t\treturn offset;\n>> +\t\t}\n>> +\t};\n>> +\n>> +\tstruct ValueHeader {\n>> +\t\tstd::uint32_t tag;\n>> +\t\tstd::uint32_t size;\n>> +\t\tstd::uint32_t alignment;\n>> +\t\tValueParams params;\n> \n> If I'm not mistaken ValueParams is only used here. Should it be\n> inlined in the ValueHeader definition ?\n> \n> Also, if I got this right:\n> \n> Entry = The metadata descriptor in the first half of the serialized\n> buffer\n> \n> ValueHeader = The header before the actual metadata value in the\n> second half of the serialized buffer\n> \n> Correct ?\n> \n> In this case, looking at the fields in those two types\n> \n> \tstruct Entry {\n> \t\tconst std::uint32_t tag;\n> \t\tconst std::uint32_t capacity;\n> \t\tconst std::uint32_t alignment;\n> \t\tconst ControlType type;\n> \t\tbool isArray;\n> \n> ...\n> \n> };\n> \n> \tstruct ValueHeader {\n> \t\tstd::uint32_t tag;\n> \t\tstd::uint32_t size;\n> \t\tstd::uint32_t alignment;\n> \n> struct ValueParams {\n> ControlType type;\n> bool isArray;\n> std::uint32_t numElements;\n> };\n> };\n> \n> I see quite an overlap.\n> \n> Basically for an Entry to match a ValueHeader the only missing\n> information are size and numElements. This makes sense as these are\n> the 'runtime' information while everything else is static and comes\n> from the control definition.\n> \n> Now, do we need to duplicate those information ? Can we just use Entry\n> and only store data in second half of the buffer ?\n> \n> This won't allow you to reconstruct a \"second half\" without the \"first\n> half\" but is this actually a use case ?\n\nI mentioned it in the cover letter that I think this is an open question,\nit seems useful, and having some kind of header simplifies the iterators.\n\n\n> \n>> +\t};\n>> +\n>> +\tstruct State {\n>> +\t\tstd::uint32_t count;\n>> +\t\tstd::uint32_t fill;\n>> +\t};\n>> +\n>> +public:\n>> +\texplicit MetadataList(const MetadataListPlan &plan)\n>> +\t\t: capacity_(plan.size()),\n>> +\t\t contentOffset_(MetadataList::contentOffset(capacity_)),\n>> +\t\t alloc_(contentOffset_)\n> \n> Why is the constructor inlined in the .h file ?\n> \n> I guess if you move it to the .cpp file you can make MetadataListPlan\n> an internal header (just forward declare it here)\n> \n>> +\t{\n>> +\t\tfor (const auto &[tag, e] : plan) {\n>> +\t\t\tassert(details::cxx20::has_single_bit(e.alignment));\n>> +\n>> +\t\t\talloc_ += sizeof(ValueHeader);\n> \n> alloc_ is initialized with contentOffset_ which if I'm not mistaken\n> it's default initialized to -1 ?\n\nIn the member initializer list `contentOffset_` is set to `MetadataList::contentOffset(capacity_)`\nbefore that.\n\n\n> \n>> +\t\t\talloc_ += e.alignment - 1; // XXX: this is the maximum\n> \n> What are XXX comments ? Are these leftovers ?\n> \n>> +\t\t\talloc_ += e.size;\n>> +\t\t\talloc_ += alignof(ValueHeader) - 1; // XXX: this is the maximum\n> \n> Maybe\n> \n> \t\t\talloc_ += sizeof(ValueHeader) + alignof(ValueHeader) - 1;\n> \t\t\talloc_ += e.size + e.alignment - 1;\n> \n> I might be missing why the -1s ?\n\nThis assumes arbitrary alignment, but `operator new` does not provide that.\nAt least `size + alignment - 1` bytes need to be allocated to ensure that\nthe `size` bytes can be aligned to `alignment` inside the allocation irrespective\nof the alignment of underlying allocation.\n\nAn open question is whether arbitrary alignment is needed or 8/16/etc is enough.\n\n\n> \n>> +\t\t}\n>> +\n>> +\t\tp_ = static_cast(::operator new(alloc_));\n>> +\n>> +\t\tauto *entries = reinterpret_cast(p_ + entriesOffset());\n> \n> What is entriesOffset() and why it's here if it's defined as\n> \n> \t[[nodiscard]] static constexpr std::size_t entriesOffset()\n> \t{\n> \t\treturn 0;\n> \t}\n> \n>> +\t\tauto it = plan.begin();\n>> +\n>> +\t\tfor (std::size_t i = 0; i < capacity_; i++, ++it) {\n> \n> it's a shame we can't use utils::enumerate() in public headers\n\nI suppose it could be moved to `base/details/...` ?\n\n\n> \n>> +\t\t\tconst auto &[tag, e] = *it;\n>> +\n>> +\t\t\tnew (&entries[i]) Entry{\n>> +\t\t\t\t.tag = tag,\n>> +\t\t\t\t.capacity = e.size,\n>> +\t\t\t\t.alignment = e.alignment,\n>> +\t\t\t\t.type = e.type,\n>> +\t\t\t\t.isArray = e.isArray,\n>> +\t\t\t};\n>> +\t\t}\n>> +\n>> +#if HAS_ASAN\n>> +\t\t::__sanitizer_annotate_contiguous_container(\n>> +\t\t\tp_ + contentOffset_, p_ + alloc_,\n>> +\t\t\tp_ + alloc_, p_ + contentOffset_\n>> +\t\t);\n>> +#endif\n>> +\t}\n>> +\n>> +\tMetadataList(const MetadataList &other)\n>> +\t\t: capacity_(other.capacity_),\n>> +\t\t contentOffset_(other.contentOffset_),\n>> +\t\t alloc_(other.alloc_),\n>> +\t\t p_(static_cast(::operator new(alloc_)))\n>> +\t{\n>> +\t\tauto *entries = reinterpret_cast(p_ + entriesOffset());\n>> +\t\tconst auto otherEntries = other.entries();\n>> +\n>> +\t\tfor (std::size_t i = 0; i < capacity_; i++) {\n>> +\t\t\tauto *e = new (&entries[i]) Entry{\n>> +\t\t\t\t.tag = otherEntries[i].tag,\n>> +\t\t\t\t.capacity = otherEntries[i].capacity,\n>> +\t\t\t\t.alignment = otherEntries[i].alignment,\n>> +\t\t\t\t.type = otherEntries[i].type,\n>> +\t\t\t\t.isArray = otherEntries[i].isArray,\n>> +\t\t\t};\n>> +\n>> +\t\t\tauto v = other.data_of(otherEntries[i]);\n>> +\t\t\tif (!v)\n>> +\t\t\t\tcontinue;\n>> +\n>> +\t\t\t[[maybe_unused]] auto r = set(*e, v);\n>> +\t\t\tassert(r == SetError());\n>> +\t\t}\n>> +\n>> +#if HAS_ASAN\n>> +\t\t::__sanitizer_annotate_contiguous_container(\n>> +\t\t\tp_ + contentOffset_, p_ + alloc_,\n>> +\t\t\tp_ + alloc_, p_ + contentOffset_\n>> +\t\t);\n>> +#endif\n>> +\t}\n>> +\n>> +\tMetadataList(MetadataList &&) = delete;\n>> +\n>> +\tMetadataList &operator=(const MetadataList &) = delete;\n> \n> If we have a copy constructor, shouldn't we have a copy assignment\n> operator as well ?\n> >> +\tMetadataList &operator=(MetadataList &&) = delete;\n> \n> Rule of 5 would say we need move assignement and constructor as well.\n> To me, given the fact a metadata list is a wrapper around a storage,\n> it makes sense not to have them ?\n\nAll of these can be implemented but they would be unsafe wrt. concurrent\nmodifications, so I chose not to implement them since they are not needed.\nI think the copy constructor is not needed either, so it could be removed.\n\n\n> \n>> +\n>> +\t~MetadataList()\n>> +\t{\n>> +#if HAS_ASAN\n>> +\t\t/*\n>> +\t\t * The documentation says the range apparently has to be\n>> +\t\t * restored to its initial state before it is deallocated.\n>> +\t\t */\n>> +\t\t::__sanitizer_annotate_contiguous_container(\n>> +\t\t\tp_ + contentOffset_, p_ + alloc_,\n>> +\t\t\tp_ + contentOffset_ + state_.load(std::memory_order_relaxed).fill, p_ + alloc_\n>> +\t\t);\n>> +#endif\n>> +\n>> +\t\t::operator delete(p_, alloc_);\n>> +\t}\n>> +\n>> +\t// TODO: want these?\n>> +\t[[nodiscard]] std::size_t size() const { return state_.load(std::memory_order_relaxed).count; }\n>> +\t[[nodiscard]] bool empty() const { return size() == 0; }\n> \n> Do we use them ?\n\nI think they are generally useful / \"expected\" of any container-like type.\n\n\n> \n> I'll stop here, there's enough material to discuss I guess :)\n> \n> Thanks, amazing progress on this front!\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 EB003C3237\n\tfor ;\n\tWed, 11 Jun 2025 13:27:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C48F068DC0;\n\tWed, 11 Jun 2025 15:27:26 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F347F61552\n\tfor ;\n\tWed, 11 Jun 2025 15:27:24 +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 96FEA352;\n\tWed, 11 Jun 2025 15:27:16 +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=\"gZgua9tu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1749648437;\n\tbh=fXqGFOtSnr3H1C42N1fJuaoZV0JTWi5TTIqNiUkH2l8=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=gZgua9tuTnzggCOu+XLx7tTrcA+8+bJDpDZC+oiIBE94dGoVPeNZv1tk7dfFvT+dS\n\t5f9/Vhb7PKkZZUD1gOXmY71C84FLEUgyibEhKlNhm4WZwsdbdIGcgX6i4ox7crwDvl\n\tchBBbkwOoFbiYJKFnnVBZScLI44UazATgHYiBYk0=","Message-ID":"<9e6aa8d9-f016-46a7-9266-6491fd8ff4c0@ideasonboard.com>","Date":"Wed, 11 Jun 2025 15:27:20 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v1 07/23] libcamera: Add `MetadataList`","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250606164156.1442682-1-barnabas.pocze@ideasonboard.com>\n\t<20250606164156.1442682-8-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\" "}},{"id":35141,"web_url":"https://patchwork.libcamera.org/comment/35141/","msgid":"","date":"2025-07-25T14:16:49","subject":"Re: [RFC PATCH v1 07/23] libcamera: Add `MetadataList`","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabas\n\n I'll go over and repeat comments from v1 that have not been\naddressed in v2.\n\nPlease clearly tell me if you disagree with them or if you have\noverlooked them, because otherwise I have to repeat the same comments\nin the review of the new version\n\nOn Wed, Jun 11, 2025 at 03:27:20PM +0200, Barnabás Pőcze wrote:\n> Hi\n>\n> 2025. 06. 11. 13:59 keltezéssel, Jacopo Mondi írta:\n> > Hi Barnabás\n> >\n> > On Fri, Jun 06, 2025 at 06:41:40PM +0200, Barnabás Pőcze wrote:\n> > [...]\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +// TODO: want this?\n> >\n> > I would say yes, but what if we move this to the main meson.build\n> > file ?\n> >\n> > I currently see in test/meson.build\n> >\n> > ------------------------------------------------------------------------------\n> >\n> > # When ASan is enabled, find the path to the ASan runtime needed by multiple\n> > # tests. This currently works with gcc only, as clang uses different file names\n> > # depending on the compiler version and target architecture.\n> > asan_enabled = false\n> > asan_runtime_missing = false\n> >\n> > if get_option('b_sanitize').contains('address')\n> > asan_enabled = true\n> >\n> > if cc.get_id() == 'gcc'\n> > asan_runtime = run_command(cc, '-print-file-name=libasan.so', check : true).stdout().strip()\n> > else\n> > asan_runtime_missing = true\n> > endif\n> > endif\n> >\n> > ------------------------------------------------------------------------------\n> >\n> >\n> > Could we define a library-wide HAS_ASAN symbol in src/meson.build in\n> > example in your opinion ?\n>\n> I think it could make sense, yes. But note that with everything inline,\n> the asan integration also works only the user application is compiled\n> with asan.\n>\n\nI'll take this as you prefer to keep it here\n\n>\n>\n> >\n> > > +#if __has_include()\n> > > +#if __SANITIZE_ADDRESS__ /* gcc */\n> > > +#include \n> > > +#define HAS_ASAN 1\n> > > +#elif defined(__has_feature)\n> > > +#if __has_feature(address_sanitizer) /* clang */\n> > > +#include \n> > > +#define HAS_ASAN 1\n> > > +#endif\n> > > +#endif\n> > > +#endif\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +class MetadataList\n> > > +{\n> > > +private:\n> >\n> > Not against it, but we usually don't mix private: and public:\n> >\n> > You could move the types definition at the beginning of the below\n> > private: section\n>\n>\n> I did try that multiple times, but as far as I remember the compiler was complaining.\n>\n>\n> >\n> > > +\tstruct ValueParams {\n> > > +\t\tControlType type;\n> > > +\t\tbool isArray;\n> > > +\t\tstd::uint32_t numElements;\n> >\n> > Does prefixing a stdint type with std:: brings anything ? It's\n> > certainly more C++, I guess\n\nI'll take it as you prefer it however it is not done anywhere in the\nrest of the code base\n\n> >\n> > > +\t};\n> > > +\n> > > +\tstruct Entry {\n> > > +\t\tconst std::uint32_t tag;\n> > > +\t\tconst std::uint32_t capacity;\n> > > +\t\tconst std::uint32_t alignment;\n> > > +\t\tconst ControlType type;\n> > > +\t\tbool isArray;\n> > > +\n> > > +\t\tstatic constexpr std::uint32_t invalidOffset = -1;\n> >\n> > constant are usually kInvalidOffset\n\nThis I insist on\n\n> >\n> >\n> > empty line maybe\n> >\n> > > +\t\t/*\n> > > +\t\t* Offset from the beginning of the allocation, and\n> > > +\t\t* and _not_ relative to `contentOffset_`.\n> > > +\t\t*/\n> >\n> > weird indent\n> >\n> > > +\t\tstd::atomic_uint32_t headerOffset = invalidOffset;\n> > > +\n> > > +\t\t[[nodiscard]] std::optional hasValue() const\n> > > +\t\t{\n> > > +\t\t\tauto offset = headerOffset.load(std::memory_order_relaxed);\n> > > +\t\t\tif (offset == invalidOffset)\n> > > +\t\t\t\treturn {};\n> > > +\n> > > +\t\t\treturn offset;\n> > > +\t\t}\n> > > +\n> > > +\t\t[[nodiscard]] std::optional acquireData() const\n> > > +\t\t{\n> > > +\t\t\tauto offset = hasValue();\n> > > +\t\t\tif (offset) {\n> > > +\t\t\t\t/* sync with release-store on `headerOffset` in `MetadataList::set()` */\n> > > +\t\t\t\tstd::atomic_thread_fence(std::memory_order_acquire);\n> > > +\t\t\t}\n> > > +\n> > > +\t\t\treturn offset;\n> > > +\t\t}\n> > > +\t};\n> > > +\n> > > +\tstruct ValueHeader {\n> > > +\t\tstd::uint32_t tag;\n> > > +\t\tstd::uint32_t size;\n> > > +\t\tstd::uint32_t alignment;\n> > > +\t\tValueParams params;\n> >\n> > If I'm not mistaken ValueParams is only used here. Should it be\n> > inlined in the ValueHeader definition ?\n\nCan you share your opinion ?\n\n> >\n> > Also, if I got this right:\n> >\n> > Entry = The metadata descriptor in the first half of the serialized\n> > buffer\n> >\n> > ValueHeader = The header before the actual metadata value in the\n> > second half of the serialized buffer\n> >\n> > Correct ?\n> >\n> > In this case, looking at the fields in those two types\n> >\n> > \tstruct Entry {\n> > \t\tconst std::uint32_t tag;\n> > \t\tconst std::uint32_t capacity;\n> > \t\tconst std::uint32_t alignment;\n> > \t\tconst ControlType type;\n> > \t\tbool isArray;\n> >\n> > ...\n> >\n> > };\n> >\n> > \tstruct ValueHeader {\n> > \t\tstd::uint32_t tag;\n> > \t\tstd::uint32_t size;\n> > \t\tstd::uint32_t alignment;\n> >\n> > struct ValueParams {\n> > ControlType type;\n> > bool isArray;\n> > std::uint32_t numElements;\n> > };\n> > };\n> >\n> > I see quite an overlap.\n> >\n> > Basically for an Entry to match a ValueHeader the only missing\n> > information are size and numElements. This makes sense as these are\n> > the 'runtime' information while everything else is static and comes\n> > from the control definition.\n> >\n> > Now, do we need to duplicate those information ? Can we just use Entry\n> > and only store data in second half of the buffer ?\n> >\n> > This won't allow you to reconstruct a \"second half\" without the \"first\n> > half\" but is this actually a use case ?\n>\n> I mentioned it in the cover letter that I think this is an open question,\n> it seems useful, and having some kind of header simplifies the iterators.\n>\n>\n> >\n> > > +\t};\n> > > +\n> > > +\tstruct State {\n> > > +\t\tstd::uint32_t count;\n> > > +\t\tstd::uint32_t fill;\n> > > +\t};\n> > > +\n> > > +public:\n> > > +\texplicit MetadataList(const MetadataListPlan &plan)\n> > > +\t\t: capacity_(plan.size()),\n> > > +\t\t contentOffset_(MetadataList::contentOffset(capacity_)),\n> > > +\t\t alloc_(contentOffset_)\n> >\n> > Why is the constructor inlined in the .h file ?\n> >\n> > I guess if you move it to the .cpp file you can make MetadataListPlan\n> > an internal header (just forward declare it here)\n> >\n\nI still think there is no reason to inline all the class definition in\nthe header\n\n> > > +\t{\n> > > +\t\tfor (const auto &[tag, e] : plan) {\n> > > +\t\t\tassert(details::cxx20::has_single_bit(e.alignment));\n> > > +\n> > > +\t\t\talloc_ += sizeof(ValueHeader);\n> >\n> > alloc_ is initialized with contentOffset_ which if I'm not mistaken\n> > it's default initialized to -1 ?\n>\n> In the member initializer list `contentOffset_` is set to `MetadataList::contentOffset(capacity_)`\n> before that.\n>\n>\n> >\n> > > +\t\t\talloc_ += e.alignment - 1; // XXX: this is the maximum\n> >\n> > What are XXX comments ? Are these leftovers ?\n\nWhat are XXX comments ? Are these leftovers ?\n\n\n> >\n> > > +\t\t\talloc_ += e.size;\n> > > +\t\t\talloc_ += alignof(ValueHeader) - 1; // XXX: this is the maximum\n> >\n> > Maybe\n> >\n> > \t\t\talloc_ += sizeof(ValueHeader) + alignof(ValueHeader) - 1;\n> > \t\t\talloc_ += e.size + e.alignment - 1;\n> >\n> > I might be missing why the -1s ?\n>\n> This assumes arbitrary alignment, but `operator new` does not provide that.\n> At least `size + alignment - 1` bytes need to be allocated to ensure that\n> the `size` bytes can be aligned to `alignment` inside the allocation irrespective\n> of the alignment of underlying allocation.\n>\n> An open question is whether arbitrary alignment is needed or 8/16/etc is enough.\n\nI really think powers of 2 are enough\n\n>\n>\n> >\n> > > +\t\t}\n> > > +\n> > > +\t\tp_ = static_cast(::operator new(alloc_));\n> > > +\n> > > +\t\tauto *entries = reinterpret_cast(p_ + entriesOffset());\n> >\n> > What is entriesOffset() and why it's here if it's defined as\n> >\n> > \t[[nodiscard]] static constexpr std::size_t entriesOffset()\n> > \t{\n> > \t\treturn 0;\n> > \t}\n\nWhat is entriesOffset() and why it's here if it's defined as\n\n\t[[nodiscard]] static constexpr std::size_t entriesOffset()\n\t{\n\t\treturn 0;\n\t}\n\n> > > +\t\tauto it = plan.begin();\n> > > +\n> > > +\t\tfor (std::size_t i = 0; i < capacity_; i++, ++it) {\n> >\n> > it's a shame we can't use utils::enumerate() in public headers\n>\n> I suppose it could be moved to `base/details/...` ?\n>\n>\n> >\n> > > +\t\t\tconst auto &[tag, e] = *it;\n> > > +\n> > > +\t\t\tnew (&entries[i]) Entry{\n> > > +\t\t\t\t.tag = tag,\n> > > +\t\t\t\t.capacity = e.size,\n> > > +\t\t\t\t.alignment = e.alignment,\n> > > +\t\t\t\t.type = e.type,\n> > > +\t\t\t\t.isArray = e.isArray,\n> > > +\t\t\t};\n> > > +\t\t}\n> > > +\n> > > +#if HAS_ASAN\n> > > +\t\t::__sanitizer_annotate_contiguous_container(\n> > > +\t\t\tp_ + contentOffset_, p_ + alloc_,\n> > > +\t\t\tp_ + alloc_, p_ + contentOffset_\n> > > +\t\t);\n> > > +#endif\n> > > +\t}\n> > > +\n> > > +\tMetadataList(const MetadataList &other)\n> > > +\t\t: capacity_(other.capacity_),\n> > > +\t\t contentOffset_(other.contentOffset_),\n> > > +\t\t alloc_(other.alloc_),\n> > > +\t\t p_(static_cast(::operator new(alloc_)))\n> > > +\t{\n> > > +\t\tauto *entries = reinterpret_cast(p_ + entriesOffset());\n> > > +\t\tconst auto otherEntries = other.entries();\n> > > +\n> > > +\t\tfor (std::size_t i = 0; i < capacity_; i++) {\n> > > +\t\t\tauto *e = new (&entries[i]) Entry{\n> > > +\t\t\t\t.tag = otherEntries[i].tag,\n> > > +\t\t\t\t.capacity = otherEntries[i].capacity,\n> > > +\t\t\t\t.alignment = otherEntries[i].alignment,\n> > > +\t\t\t\t.type = otherEntries[i].type,\n> > > +\t\t\t\t.isArray = otherEntries[i].isArray,\n> > > +\t\t\t};\n> > > +\n> > > +\t\t\tauto v = other.data_of(otherEntries[i]);\n> > > +\t\t\tif (!v)\n> > > +\t\t\t\tcontinue;\n> > > +\n> > > +\t\t\t[[maybe_unused]] auto r = set(*e, v);\n> > > +\t\t\tassert(r == SetError());\n> > > +\t\t}\n> > > +\n> > > +#if HAS_ASAN\n> > > +\t\t::__sanitizer_annotate_contiguous_container(\n> > > +\t\t\tp_ + contentOffset_, p_ + alloc_,\n> > > +\t\t\tp_ + alloc_, p_ + contentOffset_\n> > > +\t\t);\n> > > +#endif\n> > > +\t}\n> > > +\n> > > +\tMetadataList(MetadataList &&) = delete;\n> > > +\n> > > +\tMetadataList &operator=(const MetadataList &) = delete;\n> >\n> > If we have a copy constructor, shouldn't we have a copy assignment\n> > operator as well ?\n> > >> +\tMetadataList &operator=(MetadataList &&) = delete;\n> >\n> > Rule of 5 would say we need move assignement and constructor as well.\n> > To me, given the fact a metadata list is a wrapper around a storage,\n> > it makes sense not to have them ?\n>\n> All of these can be implemented but they would be unsafe wrt. concurrent\n> modifications, so I chose not to implement them since they are not needed.\n> I think the copy constructor is not needed either, so it could be removed.\n>\n>\n> >\n> > > +\n> > > +\t~MetadataList()\n> > > +\t{\n> > > +#if HAS_ASAN\n> > > +\t\t/*\n> > > +\t\t * The documentation says the range apparently has to be\n> > > +\t\t * restored to its initial state before it is deallocated.\n> > > +\t\t */\n> > > +\t\t::__sanitizer_annotate_contiguous_container(\n> > > +\t\t\tp_ + contentOffset_, p_ + alloc_,\n> > > +\t\t\tp_ + contentOffset_ + state_.load(std::memory_order_relaxed).fill, p_ + alloc_\n> > > +\t\t);\n> > > +#endif\n> > > +\n> > > +\t\t::operator delete(p_, alloc_);\n> > > +\t}\n> > > +\n> > > +\t// TODO: want these?\n> > > +\t[[nodiscard]] std::size_t size() const { return state_.load(std::memory_order_relaxed).count; }\n> > > +\t[[nodiscard]] bool empty() const { return size() == 0; }\n> >\n> > Do we use them ?\n>\n> I think they are generally useful / \"expected\" of any container-like type.\n>\n>\n> >\n> > I'll stop here, there's enough material to discuss I guess :)\n> >\n> > Thanks, amazing progress on this front!\n> > [...]\n>\n> Regards,\n> Barnabá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 609B9BDCC1\n\tfor ;\n\tFri, 25 Jul 2025 14:16:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 27C4A690F1;\n\tFri, 25 Jul 2025 16:16:56 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F27E7690A6\n\tfor ;\n\tFri, 25 Jul 2025 16:16:53 +0200 (CEST)","from ideasonboard.com (mob-5-90-139-29.net.vodafone.it\n\t[5.90.139.29])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5E87A8D4;\n\tFri, 25 Jul 2025 16:16:13 +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=\"gHTtffBg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1753452974;\n\tbh=+5vsFUKZYu8qYnb8+SbHUiJ6uTUDNziWSjUyPHM7t6Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gHTtffBgzPmAuZnOAN87wC8mlV/tT/0A+gNUcCjcFYjQUU8oVaSZfPURgVnwWBjhG\n\tQvzhqgAyWj85AXCpRexuatgStPlaH+iS9DRPHGtCYvAK+ibFb5aVjKbkG2jkSU9h3G\n\tmxDMuOZ+k8PZRif3zwV7VdJPPbuxnb91ScQVABM8=","Date":"Fri, 25 Jul 2025 16:16:49 +0200","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1 07/23] libcamera: Add `MetadataList`","Message-ID":"","References":"<20250606164156.1442682-1-barnabas.pocze@ideasonboard.com>\n\t<20250606164156.1442682-8-barnabas.pocze@ideasonboard.com>\n\t\n\t<9e6aa8d9-f016-46a7-9266-6491fd8ff4c0@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<9e6aa8d9-f016-46a7-9266-6491fd8ff4c0@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":35142,"web_url":"https://patchwork.libcamera.org/comment/35142/","msgid":"<00a8e33f-c057-4aa2-9cfe-8b8f2bbecd34@ideasonboard.com>","date":"2025-07-25T15:15:27","subject":"Re: [RFC PATCH v1 07/23] libcamera: Add `MetadataList`","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n\n2025. 07. 25. 16:16 keltezéssel, Jacopo Mondi írta:\n> Hi Barnabas\n> \n> I'll go over and repeat comments from v1 that have not been\n> addressed in v2.\n> \n> Please clearly tell me if you disagree with them or if you have\n> overlooked them, because otherwise I have to repeat the same comments\n> in the review of the new version\n\nSorry, those questions are still open, I should have communicated this better,\nand probably included them in the cover letter or as a reply to the particular\npatch.\n\n\n> \n> On Wed, Jun 11, 2025 at 03:27:20PM +0200, Barnabás Pőcze wrote:\n>> Hi\n>>\n>> 2025. 06. 11. 13:59 keltezéssel, Jacopo Mondi írta:\n>>> Hi Barnabás\n>>>\n>>> On Fri, Jun 06, 2025 at 06:41:40PM +0200, Barnabás Pőcze wrote:\n>>> [...]\n>>>> +#include \n>>>> +#include \n>>>> +#include \n>>>> +#include \n>>>> +#include \n>>>> +#include \n>>>> +#include \n>>>> +#include \n>>>> +\n>>>> +#include \n>>>> +#include \n>>>> +#include \n>>>> +\n>>>> +#include \n>>>> +#include \n>>>> +\n>>>> +// TODO: want this?\n>>>\n>>> I would say yes, but what if we move this to the main meson.build\n>>> file ?\n>>>\n>>> I currently see in test/meson.build\n>>>\n>>> ------------------------------------------------------------------------------\n>>>\n>>> # When ASan is enabled, find the path to the ASan runtime needed by multiple\n>>> # tests. This currently works with gcc only, as clang uses different file names\n>>> # depending on the compiler version and target architecture.\n>>> asan_enabled = false\n>>> asan_runtime_missing = false\n>>>\n>>> if get_option('b_sanitize').contains('address')\n>>> asan_enabled = true\n>>>\n>>> if cc.get_id() == 'gcc'\n>>> asan_runtime = run_command(cc, '-print-file-name=libasan.so', check : true).stdout().strip()\n>>> else\n>>> asan_runtime_missing = true\n>>> endif\n>>> endif\n>>>\n>>> ------------------------------------------------------------------------------\n>>>\n>>>\n>>> Could we define a library-wide HAS_ASAN symbol in src/meson.build in\n>>> example in your opinion ?\n>>\n>> I think it could make sense, yes. But note that with everything inline,\n>> the asan integration also works only the user application is compiled\n>> with asan.\n>>\n> \n> I'll take this as you prefer to keep it here\n\nI have just realized that my reply above is while technically correct,\nit is practically not useful. So I will make the changes appropriately.\n\n\n> \n>>\n>>\n>>>\n>>>> +#if __has_include()\n>>>> +#if __SANITIZE_ADDRESS__ /* gcc */\n>>>> +#include \n>>>> +#define HAS_ASAN 1\n>>>> +#elif defined(__has_feature)\n>>>> +#if __has_feature(address_sanitizer) /* clang */\n>>>> +#include \n>>>> +#define HAS_ASAN 1\n>>>> +#endif\n>>>> +#endif\n>>>> +#endif\n>>>> +\n>>>> +namespace libcamera {\n>>>> +\n>>>> +class MetadataList\n>>>> +{\n>>>> +private:\n>>>\n>>> Not against it, but we usually don't mix private: and public:\n>>>\n>>> You could move the types definition at the beginning of the below\n>>> private: section\n>>\n>>\n>> I did try that multiple times, but as far as I remember the compiler was complaining.\n>>\n>>\n>>>\n>>>> +\tstruct ValueParams {\n>>>> +\t\tControlType type;\n>>>> +\t\tbool isArray;\n>>>> +\t\tstd::uint32_t numElements;\n>>>\n>>> Does prefixing a stdint type with std:: brings anything ? It's\n>>> certainly more C++, I guess\n> \n> I'll take it as you prefer it however it is not done anywhere in the\n> rest of the code base\n\n(see below)\n\n\n> \n>>>\n>>>> +\t};\n>>>> +\n>>>> +\tstruct Entry {\n>>>> +\t\tconst std::uint32_t tag;\n>>>> +\t\tconst std::uint32_t capacity;\n>>>> +\t\tconst std::uint32_t alignment;\n>>>> +\t\tconst ControlType type;\n>>>> +\t\tbool isArray;\n>>>> +\n>>>> +\t\tstatic constexpr std::uint32_t invalidOffset = -1;\n>>>\n>>> constant are usually kInvalidOffset\n> \n> This I insist on\n\nI could've sworn I changed this (and the above). :(\n\n\n> \n>>>\n>>>\n>>> empty line maybe\n>>>\n>>>> +\t\t/*\n>>>> +\t\t* Offset from the beginning of the allocation, and\n>>>> +\t\t* and _not_ relative to `contentOffset_`.\n>>>> +\t\t*/\n>>>\n>>> weird indent\n>>>\n>>>> +\t\tstd::atomic_uint32_t headerOffset = invalidOffset;\n>>>> +\n>>>> +\t\t[[nodiscard]] std::optional hasValue() const\n>>>> +\t\t{\n>>>> +\t\t\tauto offset = headerOffset.load(std::memory_order_relaxed);\n>>>> +\t\t\tif (offset == invalidOffset)\n>>>> +\t\t\t\treturn {};\n>>>> +\n>>>> +\t\t\treturn offset;\n>>>> +\t\t}\n>>>> +\n>>>> +\t\t[[nodiscard]] std::optional acquireData() const\n>>>> +\t\t{\n>>>> +\t\t\tauto offset = hasValue();\n>>>> +\t\t\tif (offset) {\n>>>> +\t\t\t\t/* sync with release-store on `headerOffset` in `MetadataList::set()` */\n>>>> +\t\t\t\tstd::atomic_thread_fence(std::memory_order_acquire);\n>>>> +\t\t\t}\n>>>> +\n>>>> +\t\t\treturn offset;\n>>>> +\t\t}\n>>>> +\t};\n>>>> +\n>>>> +\tstruct ValueHeader {\n>>>> +\t\tstd::uint32_t tag;\n>>>> +\t\tstd::uint32_t size;\n>>>> +\t\tstd::uint32_t alignment;\n>>>> +\t\tValueParams params;\n>>>\n>>> If I'm not mistaken ValueParams is only used here. Should it be\n>>> inlined in the ValueHeader definition ?\n> \n> Can you share your opinion ?\n\nI don't have a strong opinion either way. The thinking behind this separate\ntype was that it encompasses all parameters (except the data pointer) needed\nto instantiate a `ControlValueView`. I believe at some point it might have\nhad multiple users.\n\n\n> \n>>>\n>>> Also, if I got this right:\n>>>\n>>> Entry = The metadata descriptor in the first half of the serialized\n>>> buffer\n>>>\n>>> ValueHeader = The header before the actual metadata value in the\n>>> second half of the serialized buffer\n>>>\n>>> Correct ?\n>>>\n>>> In this case, looking at the fields in those two types\n>>>\n>>> \tstruct Entry {\n>>> \t\tconst std::uint32_t tag;\n>>> \t\tconst std::uint32_t capacity;\n>>> \t\tconst std::uint32_t alignment;\n>>> \t\tconst ControlType type;\n>>> \t\tbool isArray;\n>>>\n>>> ...\n>>>\n>>> };\n>>>\n>>> \tstruct ValueHeader {\n>>> \t\tstd::uint32_t tag;\n>>> \t\tstd::uint32_t size;\n>>> \t\tstd::uint32_t alignment;\n>>>\n>>> struct ValueParams {\n>>> ControlType type;\n>>> bool isArray;\n>>> std::uint32_t numElements;\n>>> };\n>>> };\n>>>\n>>> I see quite an overlap.\n>>>\n>>> Basically for an Entry to match a ValueHeader the only missing\n>>> information are size and numElements. This makes sense as these are\n>>> the 'runtime' information while everything else is static and comes\n>>> from the control definition.\n>>>\n>>> Now, do we need to duplicate those information ? Can we just use Entry\n>>> and only store data in second half of the buffer ?\n>>>\n>>> This won't allow you to reconstruct a \"second half\" without the \"first\n>>> half\" but is this actually a use case ?\n>>\n>> I mentioned it in the cover letter that I think this is an open question,\n>> it seems useful, and having some kind of header simplifies the iterators.\n>>\n>>\n>>>\n>>>> +\t};\n>>>> +\n>>>> +\tstruct State {\n>>>> +\t\tstd::uint32_t count;\n>>>> +\t\tstd::uint32_t fill;\n>>>> +\t};\n>>>> +\n>>>> +public:\n>>>> +\texplicit MetadataList(const MetadataListPlan &plan)\n>>>> +\t\t: capacity_(plan.size()),\n>>>> +\t\t contentOffset_(MetadataList::contentOffset(capacity_)),\n>>>> +\t\t alloc_(contentOffset_)\n>>>\n>>> Why is the constructor inlined in the .h file ?\n>>>\n>>> I guess if you move it to the .cpp file you can make MetadataListPlan\n>>> an internal header (just forward declare it here)\n>>>\n> \n> I still think there is no reason to inline all the class definition in\n> the header\n\nI have not changed this because I considered the `HAS_ASAN` discussion to be\nnot decided yet, but since that turned out to be moot, I will reduce the amount\nof code in the header.\n\n\n> \n>>>> +\t{\n>>>> +\t\tfor (const auto &[tag, e] : plan) {\n>>>> +\t\t\tassert(details::cxx20::has_single_bit(e.alignment));\n>>>> +\n>>>> +\t\t\talloc_ += sizeof(ValueHeader);\n>>>\n>>> alloc_ is initialized with contentOffset_ which if I'm not mistaken\n>>> it's default initialized to -1 ?\n>>\n>> In the member initializer list `contentOffset_` is set to `MetadataList::contentOffset(capacity_)`\n>> before that.\n>>\n>>\n>>>\n>>>> +\t\t\talloc_ += e.alignment - 1; // XXX: this is the maximum\n>>>\n>>> What are XXX comments ? Are these leftovers ?\n> \n> What are XXX comments ? Are these leftovers ?\n\nI left these in because this is a point that I think can probably be improved,\nbut I am not sure how yet. And I would like to keep the \"XXX\" comments until\nthe final version, should an idea arise. Otherwise I will just remove them in\nthe final version.\n\n\n> \n> \n>>>\n>>>> +\t\t\talloc_ += e.size;\n>>>> +\t\t\talloc_ += alignof(ValueHeader) - 1; // XXX: this is the maximum\n>>>\n>>> Maybe\n>>>\n>>> \t\t\talloc_ += sizeof(ValueHeader) + alignof(ValueHeader) - 1;\n>>> \t\t\talloc_ += e.size + e.alignment - 1;\n>>>\n>>> I might be missing why the -1s ?\n>>\n>> This assumes arbitrary alignment, but `operator new` does not provide that.\n>> At least `size + alignment - 1` bytes need to be allocated to ensure that\n>> the `size` bytes can be aligned to `alignment` inside the allocation irrespective\n>> of the alignment of underlying allocation.\n>>\n>> An open question is whether arbitrary alignment is needed or 8/16/etc is enough.\n> \n> I really think powers of 2 are enough\n\nSorry, what I wrote is probably not clear enough. The question really is: should\nevery value have the same statically defined alignment, or should alignment be\nspecified on a per-value basis?\n\n\n> \n>>\n>>\n>>>\n>>>> +\t\t}\n>>>> +\n>>>> +\t\tp_ = static_cast(::operator new(alloc_));\n>>>> +\n>>>> +\t\tauto *entries = reinterpret_cast(p_ + entriesOffset());\n>>>\n>>> What is entriesOffset() and why it's here if it's defined as\n>>>\n>>> \t[[nodiscard]] static constexpr std::size_t entriesOffset()\n>>> \t{\n>>> \t\treturn 0;\n>>> \t}\n> \n> What is entriesOffset() and why it's here if it's defined as\n> \n> \t[[nodiscard]] static constexpr std::size_t entriesOffset()\n> \t{\n> \t\treturn 0;\n> \t}\n\nIt is the offset of headers in the allocated buffer; it was non-zero\nat some point. It was in v1 because of the uncertainty of how things\nare going to turn out. Same goes for v2. If it is clear that this\napproach will be used, then I can remove it (although I don't think\nit is confusing).\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n>>>> +\t\tauto it = plan.begin();\n>>>> +\n>>>> +\t\tfor (std::size_t i = 0; i < capacity_; i++, ++it) {\n>>>\n>>> it's a shame we can't use utils::enumerate() in public headers\n>>\n>> I suppose it could be moved to `base/details/...` ?\n>>\n>>\n>>>\n>>>> +\t\t\tconst auto &[tag, e] = *it;\n>>>> +\n>>>> +\t\t\tnew (&entries[i]) Entry{\n>>>> +\t\t\t\t.tag = tag,\n>>>> +\t\t\t\t.capacity = e.size,\n>>>> +\t\t\t\t.alignment = e.alignment,\n>>>> +\t\t\t\t.type = e.type,\n>>>> +\t\t\t\t.isArray = e.isArray,\n>>>> +\t\t\t};\n>>>> +\t\t}\n>>>> +\n>>>> +#if HAS_ASAN\n>>>> +\t\t::__sanitizer_annotate_contiguous_container(\n>>>> +\t\t\tp_ + contentOffset_, p_ + alloc_,\n>>>> +\t\t\tp_ + alloc_, p_ + contentOffset_\n>>>> +\t\t);\n>>>> +#endif\n>>>> +\t}\n>>>> +\n>>>> +\tMetadataList(const MetadataList &other)\n>>>> +\t\t: capacity_(other.capacity_),\n>>>> +\t\t contentOffset_(other.contentOffset_),\n>>>> +\t\t alloc_(other.alloc_),\n>>>> +\t\t p_(static_cast(::operator new(alloc_)))\n>>>> +\t{\n>>>> +\t\tauto *entries = reinterpret_cast(p_ + entriesOffset());\n>>>> +\t\tconst auto otherEntries = other.entries();\n>>>> +\n>>>> +\t\tfor (std::size_t i = 0; i < capacity_; i++) {\n>>>> +\t\t\tauto *e = new (&entries[i]) Entry{\n>>>> +\t\t\t\t.tag = otherEntries[i].tag,\n>>>> +\t\t\t\t.capacity = otherEntries[i].capacity,\n>>>> +\t\t\t\t.alignment = otherEntries[i].alignment,\n>>>> +\t\t\t\t.type = otherEntries[i].type,\n>>>> +\t\t\t\t.isArray = otherEntries[i].isArray,\n>>>> +\t\t\t};\n>>>> +\n>>>> +\t\t\tauto v = other.data_of(otherEntries[i]);\n>>>> +\t\t\tif (!v)\n>>>> +\t\t\t\tcontinue;\n>>>> +\n>>>> +\t\t\t[[maybe_unused]] auto r = set(*e, v);\n>>>> +\t\t\tassert(r == SetError());\n>>>> +\t\t}\n>>>> +\n>>>> +#if HAS_ASAN\n>>>> +\t\t::__sanitizer_annotate_contiguous_container(\n>>>> +\t\t\tp_ + contentOffset_, p_ + alloc_,\n>>>> +\t\t\tp_ + alloc_, p_ + contentOffset_\n>>>> +\t\t);\n>>>> +#endif\n>>>> +\t}\n>>>> +\n>>>> +\tMetadataList(MetadataList &&) = delete;\n>>>> +\n>>>> +\tMetadataList &operator=(const MetadataList &) = delete;\n>>>\n>>> If we have a copy constructor, shouldn't we have a copy assignment\n>>> operator as well ?\n>>>>> +\tMetadataList &operator=(MetadataList &&) = delete;\n>>>\n>>> Rule of 5 would say we need move assignement and constructor as well.\n>>> To me, given the fact a metadata list is a wrapper around a storage,\n>>> it makes sense not to have them ?\n>>\n>> All of these can be implemented but they would be unsafe wrt. concurrent\n>> modifications, so I chose not to implement them since they are not needed.\n>> I think the copy constructor is not needed either, so it could be removed.\n>>\n>>\n>>>\n>>>> +\n>>>> +\t~MetadataList()\n>>>> +\t{\n>>>> +#if HAS_ASAN\n>>>> +\t\t/*\n>>>> +\t\t * The documentation says the range apparently has to be\n>>>> +\t\t * restored to its initial state before it is deallocated.\n>>>> +\t\t */\n>>>> +\t\t::__sanitizer_annotate_contiguous_container(\n>>>> +\t\t\tp_ + contentOffset_, p_ + alloc_,\n>>>> +\t\t\tp_ + contentOffset_ + state_.load(std::memory_order_relaxed).fill, p_ + alloc_\n>>>> +\t\t);\n>>>> +#endif\n>>>> +\n>>>> +\t\t::operator delete(p_, alloc_);\n>>>> +\t}\n>>>> +\n>>>> +\t// TODO: want these?\n>>>> +\t[[nodiscard]] std::size_t size() const { return state_.load(std::memory_order_relaxed).count; }\n>>>> +\t[[nodiscard]] bool empty() const { return size() == 0; }\n>>>\n>>> Do we use them ?\n>>\n>> I think they are generally useful / \"expected\" of any container-like type.\n>>\n>>\n>>>\n>>> I'll stop here, there's enough material to discuss I guess :)\n>>>\n>>> Thanks, amazing progress on this front!\n>>> [...]\n>>\n>> Regards,\n>> Barnabá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 6CF59C3237\n\tfor ;\n\tFri, 25 Jul 2025 15:15:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 19BDF690A6;\n\tFri, 25 Jul 2025 17:15:34 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2D097690A6\n\tfor ;\n\tFri, 25 Jul 2025 17:15:31 +0200 (CEST)","from [192.168.33.15] (185.221.140.39.nat.pool.zt.hu\n\t[185.221.140.39])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0B2D5C66;\n\tFri, 25 Jul 2025 17:14:50 +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=\"Uric7OFe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1753456491;\n\tbh=XTTDgQ1cxT9Fd6JqhW59tIJhCYl5xtUeDGtxoRMZu6k=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=Uric7OFe0HUG68314RQ80JL2ux6uqqjl+vWQfoeobAJ4e3+bZ7YLk1zJQCsFkrBP6\n\tRV5+d/WCFd6RrVeiYdK2BfJtHWCa+JqdAT4P0x5n1XkychfBNKVf+10DOljHF2831y\n\tAuCbrFCeTMEQgbPfpOzMY6MGdAokAr0cRZOTvEhA=","Message-ID":"<00a8e33f-c057-4aa2-9cfe-8b8f2bbecd34@ideasonboard.com>","Date":"Fri, 25 Jul 2025 17:15:27 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v1 07/23] libcamera: Add `MetadataList`","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250606164156.1442682-1-barnabas.pocze@ideasonboard.com>\n\t<20250606164156.1442682-8-barnabas.pocze@ideasonboard.com>\n\t\n\t<9e6aa8d9-f016-46a7-9266-6491fd8ff4c0@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\" "}},{"id":35147,"web_url":"https://patchwork.libcamera.org/comment/35147/","msgid":"","date":"2025-07-25T15:49:00","subject":"Re: [RFC PATCH v1 07/23] libcamera: Add `MetadataList`","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, Jul 25, 2025 at 05:15:27PM +0200, Barnabás Pőcze wrote:\n> Hi\n>\n>\n> 2025. 07. 25. 16:16 keltezéssel, Jacopo Mondi írta:\n> > Hi Barnabas\n> >\n> > I'll go over and repeat comments from v1 that have not been\n> > addressed in v2.\n> >\n> > Please clearly tell me if you disagree with them or if you have\n> > overlooked them, because otherwise I have to repeat the same comments\n> > in the review of the new version\n>\n> Sorry, those questions are still open, I should have communicated this better,\n> and probably included them in the cover letter or as a reply to the particular\n> patch.\n>\n\nAck, thanks for clarifying\n\n>\n> >\n> > On Wed, Jun 11, 2025 at 03:27:20PM +0200, Barnabás Pőcze wrote:\n> > > Hi\n> > >\n> > > 2025. 06. 11. 13:59 keltezéssel, Jacopo Mondi írta:\n> > > > Hi Barnabás\n> > > >\n> > > > On Fri, Jun 06, 2025 at 06:41:40PM +0200, Barnabás Pőcze wrote:\n> > > > [...]\n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +\n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +\n> > > > > +#include \n> > > > > +#include \n> > > > > +\n> > > > > +// TODO: want this?\n> > > >\n> > > > I would say yes, but what if we move this to the main meson.build\n> > > > file ?\n> > > >\n> > > > I currently see in test/meson.build\n> > > >\n> > > > ------------------------------------------------------------------------------\n> > > >\n> > > > # When ASan is enabled, find the path to the ASan runtime needed by multiple\n> > > > # tests. This currently works with gcc only, as clang uses different file names\n> > > > # depending on the compiler version and target architecture.\n> > > > asan_enabled = false\n> > > > asan_runtime_missing = false\n> > > >\n> > > > if get_option('b_sanitize').contains('address')\n> > > > asan_enabled = true\n> > > >\n> > > > if cc.get_id() == 'gcc'\n> > > > asan_runtime = run_command(cc, '-print-file-name=libasan.so', check : true).stdout().strip()\n> > > > else\n> > > > asan_runtime_missing = true\n> > > > endif\n> > > > endif\n> > > >\n> > > > ------------------------------------------------------------------------------\n> > > >\n> > > >\n> > > > Could we define a library-wide HAS_ASAN symbol in src/meson.build in\n> > > > example in your opinion ?\n> > >\n> > > I think it could make sense, yes. But note that with everything inline,\n> > > the asan integration also works only the user application is compiled\n> > > with asan.\n> > >\n> >\n> > I'll take this as you prefer to keep it here\n>\n> I have just realized that my reply above is while technically correct,\n> it is practically not useful. So I will make the changes appropriately.\n>\n>\n> >\n> > >\n> > >\n> > > >\n> > > > > +#if __has_include()\n> > > > > +#if __SANITIZE_ADDRESS__ /* gcc */\n> > > > > +#include \n> > > > > +#define HAS_ASAN 1\n> > > > > +#elif defined(__has_feature)\n> > > > > +#if __has_feature(address_sanitizer) /* clang */\n> > > > > +#include \n> > > > > +#define HAS_ASAN 1\n> > > > > +#endif\n> > > > > +#endif\n> > > > > +#endif\n> > > > > +\n> > > > > +namespace libcamera {\n> > > > > +\n> > > > > +class MetadataList\n> > > > > +{\n> > > > > +private:\n> > > >\n> > > > Not against it, but we usually don't mix private: and public:\n> > > >\n> > > > You could move the types definition at the beginning of the below\n> > > > private: section\n> > >\n> > >\n> > > I did try that multiple times, but as far as I remember the compiler was complaining.\n> > >\n> > >\n> > > >\n> > > > > +\tstruct ValueParams {\n> > > > > +\t\tControlType type;\n> > > > > +\t\tbool isArray;\n> > > > > +\t\tstd::uint32_t numElements;\n> > > >\n> > > > Does prefixing a stdint type with std:: brings anything ? It's\n> > > > certainly more C++, I guess\n> >\n> > I'll take it as you prefer it however it is not done anywhere in the\n> > rest of the code base\n>\n> (see below)\n>\n>\n> >\n> > > >\n> > > > > +\t};\n> > > > > +\n> > > > > +\tstruct Entry {\n> > > > > +\t\tconst std::uint32_t tag;\n> > > > > +\t\tconst std::uint32_t capacity;\n> > > > > +\t\tconst std::uint32_t alignment;\n> > > > > +\t\tconst ControlType type;\n> > > > > +\t\tbool isArray;\n> > > > > +\n> > > > > +\t\tstatic constexpr std::uint32_t invalidOffset = -1;\n> > > >\n> > > > constant are usually kInvalidOffset\n> >\n> > This I insist on\n>\n> I could've sworn I changed this (and the above). :(\n>\n>\n> >\n> > > >\n> > > >\n> > > > empty line maybe\n> > > >\n> > > > > +\t\t/*\n> > > > > +\t\t* Offset from the beginning of the allocation, and\n> > > > > +\t\t* and _not_ relative to `contentOffset_`.\n> > > > > +\t\t*/\n> > > >\n> > > > weird indent\n> > > >\n> > > > > +\t\tstd::atomic_uint32_t headerOffset = invalidOffset;\n> > > > > +\n> > > > > +\t\t[[nodiscard]] std::optional hasValue() const\n> > > > > +\t\t{\n> > > > > +\t\t\tauto offset = headerOffset.load(std::memory_order_relaxed);\n> > > > > +\t\t\tif (offset == invalidOffset)\n> > > > > +\t\t\t\treturn {};\n> > > > > +\n> > > > > +\t\t\treturn offset;\n> > > > > +\t\t}\n> > > > > +\n> > > > > +\t\t[[nodiscard]] std::optional acquireData() const\n> > > > > +\t\t{\n> > > > > +\t\t\tauto offset = hasValue();\n> > > > > +\t\t\tif (offset) {\n> > > > > +\t\t\t\t/* sync with release-store on `headerOffset` in `MetadataList::set()` */\n> > > > > +\t\t\t\tstd::atomic_thread_fence(std::memory_order_acquire);\n> > > > > +\t\t\t}\n> > > > > +\n> > > > > +\t\t\treturn offset;\n> > > > > +\t\t}\n> > > > > +\t};\n> > > > > +\n> > > > > +\tstruct ValueHeader {\n> > > > > +\t\tstd::uint32_t tag;\n> > > > > +\t\tstd::uint32_t size;\n> > > > > +\t\tstd::uint32_t alignment;\n> > > > > +\t\tValueParams params;\n> > > >\n> > > > If I'm not mistaken ValueParams is only used here. Should it be\n> > > > inlined in the ValueHeader definition ?\n> >\n> > Can you share your opinion ?\n>\n> I don't have a strong opinion either way. The thinking behind this separate\n> type was that it encompasses all parameters (except the data pointer) needed\n> to instantiate a `ControlValueView`. I believe at some point it might have\n> had multiple users.\n>\n\nOk, if it makes the code easier for you it's fine\n\n>\n> >\n> > > >\n> > > > Also, if I got this right:\n> > > >\n> > > > Entry = The metadata descriptor in the first half of the serialized\n> > > > buffer\n> > > >\n> > > > ValueHeader = The header before the actual metadata value in the\n> > > > second half of the serialized buffer\n> > > >\n> > > > Correct ?\n> > > >\n> > > > In this case, looking at the fields in those two types\n> > > >\n> > > > \tstruct Entry {\n> > > > \t\tconst std::uint32_t tag;\n> > > > \t\tconst std::uint32_t capacity;\n> > > > \t\tconst std::uint32_t alignment;\n> > > > \t\tconst ControlType type;\n> > > > \t\tbool isArray;\n> > > >\n> > > > ...\n> > > >\n> > > > };\n> > > >\n> > > > \tstruct ValueHeader {\n> > > > \t\tstd::uint32_t tag;\n> > > > \t\tstd::uint32_t size;\n> > > > \t\tstd::uint32_t alignment;\n> > > >\n> > > > struct ValueParams {\n> > > > ControlType type;\n> > > > bool isArray;\n> > > > std::uint32_t numElements;\n> > > > };\n> > > > };\n> > > >\n> > > > I see quite an overlap.\n> > > >\n> > > > Basically for an Entry to match a ValueHeader the only missing\n> > > > information are size and numElements. This makes sense as these are\n> > > > the 'runtime' information while everything else is static and comes\n> > > > from the control definition.\n> > > >\n> > > > Now, do we need to duplicate those information ? Can we just use Entry\n> > > > and only store data in second half of the buffer ?\n> > > >\n> > > > This won't allow you to reconstruct a \"second half\" without the \"first\n> > > > half\" but is this actually a use case ?\n> > >\n> > > I mentioned it in the cover letter that I think this is an open question,\n> > > it seems useful, and having some kind of header simplifies the iterators.\n> > >\n> > >\n> > > >\n> > > > > +\t};\n> > > > > +\n> > > > > +\tstruct State {\n> > > > > +\t\tstd::uint32_t count;\n> > > > > +\t\tstd::uint32_t fill;\n> > > > > +\t};\n> > > > > +\n> > > > > +public:\n> > > > > +\texplicit MetadataList(const MetadataListPlan &plan)\n> > > > > +\t\t: capacity_(plan.size()),\n> > > > > +\t\t contentOffset_(MetadataList::contentOffset(capacity_)),\n> > > > > +\t\t alloc_(contentOffset_)\n> > > >\n> > > > Why is the constructor inlined in the .h file ?\n> > > >\n> > > > I guess if you move it to the .cpp file you can make MetadataListPlan\n> > > > an internal header (just forward declare it here)\n> > > >\n> >\n> > I still think there is no reason to inline all the class definition in\n> > the header\n>\n> I have not changed this because I considered the `HAS_ASAN` discussion to be\n> not decided yet, but since that turned out to be moot, I will reduce the amount\n> of code in the header.\n>\n\nThanks.\n\nAgain, if there are valid reasons to keep code in the header please\ntell\n\n>\n> >\n> > > > > +\t{\n> > > > > +\t\tfor (const auto &[tag, e] : plan) {\n> > > > > +\t\t\tassert(details::cxx20::has_single_bit(e.alignment));\n> > > > > +\n> > > > > +\t\t\talloc_ += sizeof(ValueHeader);\n> > > >\n> > > > alloc_ is initialized with contentOffset_ which if I'm not mistaken\n> > > > it's default initialized to -1 ?\n> > >\n> > > In the member initializer list `contentOffset_` is set to `MetadataList::contentOffset(capacity_)`\n> > > before that.\n> > >\n> > >\n> > > >\n> > > > > +\t\t\talloc_ += e.alignment - 1; // XXX: this is the maximum\n> > > >\n> > > > What are XXX comments ? Are these leftovers ?\n> >\n> > What are XXX comments ? Are these leftovers ?\n>\n> I left these in because this is a point that I think can probably be improved,\n> but I am not sure how yet. And I would like to keep the \"XXX\" comments until\n> the final version, should an idea arise. Otherwise I will just remove them in\n\nTo be frank I don't get what open problem \"XXX: this is the maximum\"\nconveys\n\n> the final version.\n>\n\nLet's properly /* \\todo */ for things we know will have to be done and\nremove all the XXX once we get to a version we want to merge\n\n>\n> >\n> >\n> > > >\n> > > > > +\t\t\talloc_ += e.size;\n> > > > > +\t\t\talloc_ += alignof(ValueHeader) - 1; // XXX: this is the maximum\n> > > >\n> > > > Maybe\n> > > >\n> > > > \t\t\talloc_ += sizeof(ValueHeader) + alignof(ValueHeader) - 1;\n> > > > \t\t\talloc_ += e.size + e.alignment - 1;\n> > > >\n> > > > I might be missing why the -1s ?\n> > >\n> > > This assumes arbitrary alignment, but `operator new` does not provide that.\n> > > At least `size + alignment - 1` bytes need to be allocated to ensure that\n> > > the `size` bytes can be aligned to `alignment` inside the allocation irrespective\n> > > of the alignment of underlying allocation.\n> > >\n> > > An open question is whether arbitrary alignment is needed or 8/16/etc is enough.\n> >\n> > I really think powers of 2 are enough\n>\n> Sorry, what I wrote is probably not clear enough. The question really is: should\n\nSorry this was partially a joke, as I don't expect any type to be,\nsay, 3 bytes aligned :)\n\n> every value have the same statically defined alignment, or should alignment be\n> specified on a per-value basis?\n>\n\nCould you force the alignment of known types like ValueHeader when\ndeclaring the type ? I would __attribute__(aligned(8)) to 64-bit align\nit and (possibily) increase access efficiency (I always considered 64\nbits alignement to improve performances on modern systems, but I know\nthere is much more going on under the hood for me to tell if that's\nalways true or not. Among the commonly cited reasons there is\ncache-line alignment and usage of more efficient load/store\noperations, but I've not verified it first hand)\n\n>\n> >\n> > >\n> > >\n> > > >\n> > > > > +\t\t}\n> > > > > +\n> > > > > +\t\tp_ = static_cast(::operator new(alloc_));\n> > > > > +\n> > > > > +\t\tauto *entries = reinterpret_cast(p_ + entriesOffset());\n> > > >\n> > > > What is entriesOffset() and why it's here if it's defined as\n> > > >\n> > > > \t[[nodiscard]] static constexpr std::size_t entriesOffset()\n> > > > \t{\n> > > > \t\treturn 0;\n> > > > \t}\n> >\n> > What is entriesOffset() and why it's here if it's defined as\n> >\n> > \t[[nodiscard]] static constexpr std::size_t entriesOffset()\n> > \t{\n> > \t\treturn 0;\n> > \t}\n>\n> It is the offset of headers in the allocated buffer; it was non-zero\n> at some point. It was in v1 because of the uncertainty of how things\n> are going to turn out. Same goes for v2. If it is clear that this\n> approach will be used, then I can remove it (although I don't think\n> it is confusing).\n\nIt's one more level of indirection for reasons that at the moment do\nnot exist as far as I can tell. What are you trying to prepare for ?\nIs the (even if minimal) increase in complexity justified ?\n\n>\n>\n> Regards,\n> Barnabás Pőcze\n>\n>\n> >\n> > > > > +\t\tauto it = plan.begin();\n> > > > > +\n> > > > > +\t\tfor (std::size_t i = 0; i < capacity_; i++, ++it) {\n> > > >\n> > > > it's a shame we can't use utils::enumerate() in public headers\n> > >\n> > > I suppose it could be moved to `base/details/...` ?\n> > >\n> > >\n> > > >\n> > > > > +\t\t\tconst auto &[tag, e] = *it;\n> > > > > +\n> > > > > +\t\t\tnew (&entries[i]) Entry{\n> > > > > +\t\t\t\t.tag = tag,\n> > > > > +\t\t\t\t.capacity = e.size,\n> > > > > +\t\t\t\t.alignment = e.alignment,\n> > > > > +\t\t\t\t.type = e.type,\n> > > > > +\t\t\t\t.isArray = e.isArray,\n> > > > > +\t\t\t};\n> > > > > +\t\t}\n> > > > > +\n> > > > > +#if HAS_ASAN\n> > > > > +\t\t::__sanitizer_annotate_contiguous_container(\n> > > > > +\t\t\tp_ + contentOffset_, p_ + alloc_,\n> > > > > +\t\t\tp_ + alloc_, p_ + contentOffset_\n> > > > > +\t\t);\n> > > > > +#endif\n> > > > > +\t}\n> > > > > +\n> > > > > +\tMetadataList(const MetadataList &other)\n> > > > > +\t\t: capacity_(other.capacity_),\n> > > > > +\t\t contentOffset_(other.contentOffset_),\n> > > > > +\t\t alloc_(other.alloc_),\n> > > > > +\t\t p_(static_cast(::operator new(alloc_)))\n> > > > > +\t{\n> > > > > +\t\tauto *entries = reinterpret_cast(p_ + entriesOffset());\n> > > > > +\t\tconst auto otherEntries = other.entries();\n> > > > > +\n> > > > > +\t\tfor (std::size_t i = 0; i < capacity_; i++) {\n> > > > > +\t\t\tauto *e = new (&entries[i]) Entry{\n> > > > > +\t\t\t\t.tag = otherEntries[i].tag,\n> > > > > +\t\t\t\t.capacity = otherEntries[i].capacity,\n> > > > > +\t\t\t\t.alignment = otherEntries[i].alignment,\n> > > > > +\t\t\t\t.type = otherEntries[i].type,\n> > > > > +\t\t\t\t.isArray = otherEntries[i].isArray,\n> > > > > +\t\t\t};\n> > > > > +\n> > > > > +\t\t\tauto v = other.data_of(otherEntries[i]);\n> > > > > +\t\t\tif (!v)\n> > > > > +\t\t\t\tcontinue;\n> > > > > +\n> > > > > +\t\t\t[[maybe_unused]] auto r = set(*e, v);\n> > > > > +\t\t\tassert(r == SetError());\n> > > > > +\t\t}\n> > > > > +\n> > > > > +#if HAS_ASAN\n> > > > > +\t\t::__sanitizer_annotate_contiguous_container(\n> > > > > +\t\t\tp_ + contentOffset_, p_ + alloc_,\n> > > > > +\t\t\tp_ + alloc_, p_ + contentOffset_\n> > > > > +\t\t);\n> > > > > +#endif\n> > > > > +\t}\n> > > > > +\n> > > > > +\tMetadataList(MetadataList &&) = delete;\n> > > > > +\n> > > > > +\tMetadataList &operator=(const MetadataList &) = delete;\n> > > >\n> > > > If we have a copy constructor, shouldn't we have a copy assignment\n> > > > operator as well ?\n> > > > > > +\tMetadataList &operator=(MetadataList &&) = delete;\n> > > >\n> > > > Rule of 5 would say we need move assignement and constructor as well.\n> > > > To me, given the fact a metadata list is a wrapper around a storage,\n> > > > it makes sense not to have them ?\n> > >\n> > > All of these can be implemented but they would be unsafe wrt. concurrent\n> > > modifications, so I chose not to implement them since they are not needed.\n> > > I think the copy constructor is not needed either, so it could be removed.\n> > >\n> > >\n> > > >\n> > > > > +\n> > > > > +\t~MetadataList()\n> > > > > +\t{\n> > > > > +#if HAS_ASAN\n> > > > > +\t\t/*\n> > > > > +\t\t * The documentation says the range apparently has to be\n> > > > > +\t\t * restored to its initial state before it is deallocated.\n> > > > > +\t\t */\n> > > > > +\t\t::__sanitizer_annotate_contiguous_container(\n> > > > > +\t\t\tp_ + contentOffset_, p_ + alloc_,\n> > > > > +\t\t\tp_ + contentOffset_ + state_.load(std::memory_order_relaxed).fill, p_ + alloc_\n> > > > > +\t\t);\n> > > > > +#endif\n> > > > > +\n> > > > > +\t\t::operator delete(p_, alloc_);\n> > > > > +\t}\n> > > > > +\n> > > > > +\t// TODO: want these?\n> > > > > +\t[[nodiscard]] std::size_t size() const { return state_.load(std::memory_order_relaxed).count; }\n> > > > > +\t[[nodiscard]] bool empty() const { return size() == 0; }\n> > > >\n> > > > Do we use them ?\n> > >\n> > > I think they are generally useful / \"expected\" of any container-like type.\n> > >\n> > >\n> > > >\n> > > > I'll stop here, there's enough material to discuss I guess :)\n> > > >\n> > > > Thanks, amazing progress on this front!\n> > > > [...]\n> > >\n> > > Regards,\n> > > Barnabás Pőcze\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 1BF65BDCC1\n\tfor ;\n\tFri, 25 Jul 2025 15:49:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DC355690F9;\n\tFri, 25 Jul 2025 17:49:06 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D2015690A6\n\tfor ;\n\tFri, 25 Jul 2025 17:49:04 +0200 (CEST)","from ideasonboard.com (mob-5-90-139-29.net.vodafone.it\n\t[5.90.139.29])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 955A98D4;\n\tFri, 25 Jul 2025 17:48:24 +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=\"uhI8ybMD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1753458505;\n\tbh=E9JFLu1NMe/8CaR2YEOEKyeDxt1e55VmCokqpMEqNQs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uhI8ybMDfCdaFn7Ru2fS5Vxx3MceDSmdnqnj9XeOkRsQYmX3BGtxTiHCcH57QVK+s\n\tDMYywtIuZWRsYcfT0nxkts0xhVvRgQQvC1bA2jCQ8a0oDn5HWkma96IQvNa9Bi/Uhj\n\tvfsdw1pjT7mZuOpqODCDaysSMqTgoisQ0YKY94O0=","Date":"Fri, 25 Jul 2025 17:49:00 +0200","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1 07/23] libcamera: Add `MetadataList`","Message-ID":"","References":"<20250606164156.1442682-1-barnabas.pocze@ideasonboard.com>\n\t<20250606164156.1442682-8-barnabas.pocze@ideasonboard.com>\n\t\n\t<9e6aa8d9-f016-46a7-9266-6491fd8ff4c0@ideasonboard.com>\n\t\n\t<00a8e33f-c057-4aa2-9cfe-8b8f2bbecd34@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<00a8e33f-c057-4aa2-9cfe-8b8f2bbecd34@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\" "}}]