[RFC,v2,07/22] libcamera: Add `MetadataList`

23875 diff mbox series

Add a dedicated `MetadataList` type, whose purpose is to store the metadata
reported by a camera for a given request. Previously, a `ControlList` was
used for this purpose. The reason for introducing a separate type is to
simplify the access to the returned metadata during the entire lifetime
of a request.

Specifically, for early metadata completion to be easily usable it should be
guaranteed that any completed metadata item can be accessed and looked up
at least until the associated requested is reused with `Request::reuse()`.

However, when a metadata item is completed early, the pipeline handler
might still work on the request in the `CameraManager`'s private thread,
therefore there is an inherent synchronization issue when an application
accesses early metadata.

Restricting the user to only access the metadata items of a not yet completed
request in the early metadata availability signal handler by ways of
documenting or enforcing it at runtime could be an option, but it is not
too convenient for the user.

The current `ControlList` implementation employs an `std::unordered_map`,
so pointers remain stable when the container is modified, so an application
could keep accessing particular metadata items outside the signal handler,
but this fact is far from obvious, and the user would still not be able
to make a copy of all metadata or do lookups based on the numeric ids or
the usual `libcamera::Control<>` objects, thus some type safety is lost.

The above also requires that each metadata item is only completed once for
a given request, but this does not appear to be serious limitation,
and in fact, this restriction is enforced by `MetadataList`.

The introduced `MetadataList` supports single writer - multiple reader
scenarios, and it can be set, looked-up, and copied in a wait-free fashion
without introducing data races or other synchronization issues. This is
achieved by requiring the possible set of metadata items to be known
(such set is stored in a `MetadataListPlan` object). Based on the this
plan, a single contiguous allocation is made to accommodate all potential
metadata items. Due to this single contiguous allocation that is not modified
during the lifetime of a `MetadataList` and atomic modifications, it is
possible to easily gaurantee thread-safe set, lookup, and copy; assuming
there is only ever a single writer.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
changes in v2:
  * remove multiple not strictly necessary functions
---
 include/libcamera/meson.build          |   2 +
 include/libcamera/metadata_list.h      | 547 +++++++++++++++++++++++++
 include/libcamera/metadata_list_plan.h | 130 ++++++
 src/libcamera/meson.build              |   1 +
 src/libcamera/metadata_list.cpp        | 344 ++++++++++++++++
 test/controls/meson.build              |   1 +
 test/controls/metadata_list.cpp        | 170 ++++++++
 7 files changed, 1195 insertions(+)
 create mode 100644 include/libcamera/metadata_list.h
 create mode 100644 include/libcamera/metadata_list_plan.h
 create mode 100644 src/libcamera/metadata_list.cpp
 create mode 100644 test/controls/metadata_list.cpp

Hi Barnabás

On Mon, Jul 21, 2025 at 12:46:07PM +0200, Barnabás Pőcze wrote:
> Add a dedicated `MetadataList` type, whose purpose is to store the metadata
> reported by a camera for a given request. Previously, a `ControlList` was
> used for this purpose. The reason for introducing a separate type is to
> simplify the access to the returned metadata during the entire lifetime
> of a request.
>
> Specifically, for early metadata completion to be easily usable it should be
> guaranteed that any completed metadata item can be accessed and looked up
> at least until the associated requested is reused with `Request::reuse()`.
>
> However, when a metadata item is completed early, the pipeline handler
> might still work on the request in the `CameraManager`'s private thread,
> therefore there is an inherent synchronization issue when an application
> accesses early metadata.
>
> Restricting the user to only access the metadata items of a not yet completed
> request in the early metadata availability signal handler by ways of
> documenting or enforcing it at runtime could be an option, but it is not
> too convenient for the user.
>
> The current `ControlList` implementation employs an `std::unordered_map`,
> so pointers remain stable when the container is modified, so an application
> could keep accessing particular metadata items outside the signal handler,
> but this fact is far from obvious, and the user would still not be able
> to make a copy of all metadata or do lookups based on the numeric ids or
> the usual `libcamera::Control<>` objects, thus some type safety is lost.
>
> The above also requires that each metadata item is only completed once for
> a given request, but this does not appear to be serious limitation,
> and in fact, this restriction is enforced by `MetadataList`.
>
> The introduced `MetadataList` supports single writer - multiple reader
> scenarios, and it can be set, looked-up, and copied in a wait-free fashion
> without introducing data races or other synchronization issues. This is
> achieved by requiring the possible set of metadata items to be known
> (such set is stored in a `MetadataListPlan` object). Based on the this
> plan, a single contiguous allocation is made to accommodate all potential
> metadata items. Due to this single contiguous allocation that is not modified
> during the lifetime of a `MetadataList` and atomic modifications, it is
> possible to easily gaurantee thread-safe set, lookup, and copy; assuming
> there is only ever a single writer.
>
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
> changes in v2:
>   * remove multiple not strictly necessary functions
> ---
>  include/libcamera/meson.build          |   2 +
>  include/libcamera/metadata_list.h      | 547 +++++++++++++++++++++++++
>  include/libcamera/metadata_list_plan.h | 130 ++++++
>  src/libcamera/meson.build              |   1 +
>  src/libcamera/metadata_list.cpp        | 344 ++++++++++++++++
>  test/controls/meson.build              |   1 +
>  test/controls/metadata_list.cpp        | 170 ++++++++
>  7 files changed, 1195 insertions(+)
>  create mode 100644 include/libcamera/metadata_list.h
>  create mode 100644 include/libcamera/metadata_list_plan.h
>  create mode 100644 src/libcamera/metadata_list.cpp
>  create mode 100644 test/controls/metadata_list.cpp
>
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index 30ea76f94..410b548dd 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -12,6 +12,8 @@ libcamera_public_headers = files([
>      'framebuffer_allocator.h',
>      'geometry.h',
>      'logging.h',
> +    'metadata_list.h',
> +    'metadata_list_plan.h',
>      'orientation.h',
>      'pixel_format.h',
>      'request.h',
> diff --git a/include/libcamera/metadata_list.h b/include/libcamera/metadata_list.h
> new file mode 100644
> index 000000000..7fe3dbbab
> --- /dev/null
> +++ b/include/libcamera/metadata_list.h
> @@ -0,0 +1,547 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2025, Ideas On Board Oy
> + *
> + * Metadata list
> + */
> +
> +#pragma once
> +
> +#include <algorithm>
> +#include <atomic>
> +#include <cassert>
> +#include <cstdint>
> +#include <cstring>
> +#include <new>
> +#include <optional>
> +#include <type_traits>
> +
> +#include <libcamera/base/details/align.h>
> +#include <libcamera/base/details/cxx20.h>
> +#include <libcamera/base/span.h>
> +
> +#include <libcamera/controls.h>
> +#include <libcamera/metadata_list_plan.h>
> +
> +// TODO: want this?
> +#if __has_include(<sanitizer/asan_interface.h>)
> +#if __SANITIZE_ADDRESS__ /* gcc */
> +#include <sanitizer/asan_interface.h>
> +#define HAS_ASAN 1
> +#elif defined(__has_feature)
> +#if __has_feature(address_sanitizer) /* clang */
> +#include <sanitizer/asan_interface.h>
> +#define HAS_ASAN 1
> +#endif
> +#endif
> +#endif
> +
> +namespace libcamera {
> +
> +class MetadataList
> +{
> +private:
> +	struct ValueParams {
> +		ControlType type;
> +		bool isArray;
> +		std::uint32_t numElements;
> +	};
> +
> +	struct Entry {
> +		const std::uint32_t tag;
> +		const std::uint32_t capacity;
> +		const std::uint32_t alignment;
> +		const ControlType type;
> +		bool isArray;
> +
> +		static constexpr std::uint32_t invalidOffset = -1;
> +		/*
> +		 * Offset from the beginning of the allocation, and
> +		 * and _not_ relative to `contentOffset_`.
> +		 */
> +		std::atomic_uint32_t headerOffset = invalidOffset;
> +
> +		[[nodiscard]] std::optional<std::uint32_t> hasValue() const
> +		{
> +			auto offset = headerOffset.load(std::memory_order_relaxed);
> +			if (offset == invalidOffset)
> +				return {};
> +
> +			return offset;
> +		}
> +
> +		[[nodiscard]] std::optional<std::uint32_t> acquireData() const
> +		{
> +			auto offset = hasValue();
> +			if (offset) {
> +				/* sync with release-store on `headerOffset` in `MetadataList::set()` */
> +				std::atomic_thread_fence(std::memory_order_acquire);
> +			}
> +
> +			return offset;
> +		}
> +	};
> +
> +	struct ValueHeader {
> +		std::uint32_t tag;
> +		std::uint32_t size;
> +		std::uint32_t alignment;
> +		ValueParams params;
> +	};
> +
> +	struct State {
> +		std::uint32_t count;
> +		std::uint32_t fill;
> +	};
> +
> +public:
> +	explicit MetadataList(const MetadataListPlan &plan)
> +		: capacity_(plan.size()),
> +		  contentOffset_(MetadataList::contentOffset(capacity_)),
> +		  alloc_(contentOffset_)
> +	{
> +		for (const auto &[tag, e] : plan) {
> +			alloc_ += sizeof(ValueHeader);
> +			alloc_ += e.alignment - 1; // XXX: this is the maximum
> +			alloc_ += e.size * e.numElements;
> +			alloc_ += alignof(ValueHeader) - 1; // XXX: this is the maximum
> +		}
> +
> +		p_ = static_cast<std::byte *>(::operator new(alloc_));
> +
> +		auto *entries = reinterpret_cast<Entry *>(p_ + entriesOffset());
> +		auto it = plan.begin();
> +
> +		for (std::size_t i = 0; i < capacity_; i++, ++it) {
> +			const auto &[tag, e] = *it;
> +
> +			new (&entries[i]) Entry{
> +				.tag = tag,
> +				.capacity = e.size * e.numElements,
> +				.alignment = e.alignment,
> +				.type = e.type,
> +				.isArray = e.isArray,
> +			};

I was already about to rant against C++'s "do the same usual thing in
some different way" style and wanted to suggest

			entries[i].tag = tag;
			entries[i].capacity = e.size * e.numElements,
			entries[i].alignment = e.alignment;
			entries[i].type = e.type;
			entries[i].isArray = e.isArray;

But then you loose the const-ness of entries[i]'s fields.

Let's use "placement new" then...


> +		}
> +
> +#if HAS_ASAN
> +		::__sanitizer_annotate_contiguous_container(
> +			p_ + contentOffset_, p_ + alloc_,
> +			p_ + alloc_, p_ + contentOffset_
> +		);
> +#endif
> +	}
> +
> +	MetadataList(const MetadataList &) = delete;
> +	MetadataList(MetadataList &&) = delete;
> +
> +	MetadataList &operator=(const MetadataList &) = delete;
> +	MetadataList &operator=(MetadataList &&) = delete;
> +
> +	~MetadataList()
> +	{
> +#if HAS_ASAN
> +		/*
> +		 * The documentation says the range apparently has to be
> +		 * restored to its initial state before it is deallocated.
> +		 */
> +		::__sanitizer_annotate_contiguous_container(
> +			p_ + contentOffset_, p_ + alloc_,
> +			p_ + contentOffset_ + state_.load(std::memory_order_relaxed).fill, p_ + alloc_
> +		);
> +#endif
> +
> +		::operator delete(p_, alloc_);
> +	}
> +
> +	// TODO: want these?

Why not ?

> +	[[nodiscard]] std::size_t size() const { return state_.load(std::memory_order_relaxed).count; }
> +	[[nodiscard]] bool empty() const { return state_.load(std::memory_order_relaxed).fill == 0; }
> +
> +	enum class SetError {
> +		UnknownTag = 1,
> +		AlreadySet,
> +		SizeMismatch,
> +		TypeMismatch,
> +	};

The only user of "set" will be pipeline handler, so I wonder if coding
the errors as enum is really something that gives any value here or
it's just overdesign. It's not like pipelines will do

        if (AlreadySet)
                doSomething

If they get an error when adding metadata it will be caught during
development I presume.

> +
> +	[[nodiscard]] SetError set(std::uint32_t tag, ControlValueView v)

Since we end up copying the data into the MetadataList, why use a
ControlValueView and not a ControlValue ?

> +	{
> +		auto *e = find(tag);
> +		if (!e)
> +			return SetError::UnknownTag;
> +
> +		return set(*e, v);
> +	}
> +
> +	template<typename T>
> +	/* TODO: [[nodiscard]] */ SetError set(const Control<T> &ctrl, const details::cxx20::type_identity_t<T> &value)

Why TODO ?

Here and everywhere else could you shorten lines length where is
trivial to do so ?

	/* TODO: [[nodiscard]] */ SetError set(const Control<T> &ctrl,
                                               const details::cxx20::type_identity_t<T> &value)


What does type_identity_t<> gives us here ? Isn't T fully specified ?

> +	{
> +		using TypeInfo = libcamera::details::control_type<T>;
> +
> +		if constexpr (TypeInfo::size > 0) {
> +			static_assert(std::is_trivially_copyable_v<typename T::value_type>);
> +
> +			return set(ctrl.id(), {
> +				TypeInfo::value,
> +				true,
> +				value.size(),
> +				reinterpret_cast<const std::byte *>(value.data()),
> +			});
> +		} else {
> +			static_assert(std::is_trivially_copyable_v<T>);
> +
> +			return set(ctrl.id(), {
> +				TypeInfo::value,
> +				false,
> +				1,
> +				reinterpret_cast<const std::byte *>(&value),
> +			});
> +		}

The private set() overload works with Views to avoid copies here, I
get it, but as said before should the public set(tag, value) accept a
ControlValue & or do we want callers to go through a View ?

> +	}
> +
> +	template<typename T>
> +	[[nodiscard]] decltype(auto) get(const Control<T> &ctrl) const

isn't this simply an
	[[nodiscard]] std::optional<T> get(const Control<T> &ctrl) const

why use decltype(auto) ?

> +	{
> +		ControlValueView v = get(ctrl.id());
> +
> +		return v ? std::optional(v.get<T>()) : std::nullopt;
> +	}
> +
> +	// TODO: operator ControlListView() const ?
> +	// TODO: explicit operator ControlList() const ?

Do these still apply ?

> +
> +	[[nodiscard]] ControlValueView get(std::uint32_t tag) const
> +	{
> +		const auto *e = find(tag);
> +		if (!e)
> +			return {};
> +
> +		return data_of(*e);

no snake_case but use camelCase please

> +	}
> +
> +	void clear()
> +	{
> +		for (auto &e : entries())
> +			e.headerOffset.store(Entry::invalidOffset, std::memory_order_relaxed);
> +
> +		[[maybe_unused]] auto s = state_.exchange({}, std::memory_order_relaxed);
> +
> +#if HAS_ASAN
> +		::__sanitizer_annotate_contiguous_container(
> +			p_ + contentOffset_, p_ + alloc_,
> +			p_ + contentOffset_ + s.fill, p_ + contentOffset_
> +		);
> +#endif
> +	}
> +
> +	class iterator
> +	{
> +	public:
> +		using difference_type = std::ptrdiff_t;
> +		using value_type = std::pair<std::uint32_t, ControlValueView>;
> +		using pointer = void;
> +		using reference = value_type;

Why can't you use 'value_type' and need another indirection symbol ?

Are those symbols required when defining a custom iterator ?

Also shouldn't this be "value_type &" ?

> +		using iterator_category = std::forward_iterator_tag;
> +
> +		iterator() = default;
> +
> +		iterator& operator++()
> +		{
> +			const auto &h = header();
> +
> +			p_ += sizeof(h);
> +			p_ = details::align::up(p_, h.alignment);
> +			p_ += h.size;
> +			p_ = details::align::up(p_, alignof(decltype(h)));
> +
> +			return *this;
> +		}
> +
> +		iterator operator++(int)
> +		{
> +			auto copy = *this;
> +			++*this;
> +			return copy;
> +		}
> +
> +		[[nodiscard]] reference operator*() const
> +		{
> +			const auto &h = header();
> +			const auto *data = details::align::up(p_ + sizeof(h), h.alignment);
> +
> +			return { h.tag, { h.params.type, h.params.isArray, h.params.numElements, data } };
> +		}
> +
> +		[[nodiscard]] bool operator==(const iterator &other) const
> +		{
> +			return p_ == other.p_;
> +		}
> +
> +		[[nodiscard]] bool operator!=(const iterator &other) const
> +		{
> +			return !(*this == other);
> +		}
> +
> +	private:
> +		iterator(const std::byte *p)
> +			: p_(p)
> +		{
> +		}
> +
> +		[[nodiscard]] const ValueHeader &header() const
> +		{
> +			return *reinterpret_cast<const ValueHeader *>(p_);
> +		}
> +
> +		friend MetadataList;
> +
> +		const std::byte *p_ = nullptr;
> +	};
> +
> +	[[nodiscard]] iterator begin() const
> +	{
> +		return { p_ + contentOffset_ };
> +	}
> +
> +	[[nodiscard]] iterator end() const
> +	{
> +		return { p_ + contentOffset_ + state_.load(std::memory_order_acquire).fill };
> +	}
> +
> +	class Diff
> +	{
> +	public:
> +		// TODO: want these?

Why not you think ?

> +		[[nodiscard]] explicit operator bool() const { return !empty(); }
> +		[[nodiscard]] bool empty() const { return start_ == stop_; }
> +		[[nodiscard]] std::size_t size() const { return changed_; }
> +		[[nodiscard]] const MetadataList &list() const { return *l_; }
> +
> +		[[nodiscard]] ControlValueView get(std::uint32_t tag) const
> +		{
> +			const auto *e = l_->find(tag);
> +			if (!e)
> +				return {};
> +
> +			auto o = e->acquireData();
> +			if (!o)
> +				return {};
> +
> +			if (!(start_ <= *o && *o < stop_))
> +				return {};
> +
> +			return l_->data_of(*o);
> +		}
> +
> +		template<typename T>
> +		[[nodiscard]] decltype(auto) get(const Control<T> &ctrl) const
> +		{
> +			ControlValueView v = get(ctrl.id());
> +
> +			return v ? std::optional(v.get<T>()) : std::nullopt;
> +		}
> +
> +		[[nodiscard]] iterator begin() const
> +		{
> +			return { l_->p_ + start_ };
> +		}
> +
> +		[[nodiscard]] iterator end() const
> +		{
> +			return { l_->p_ + stop_ };
> +		}
> +
> +	private:
> +		Diff(const MetadataList &l, std::size_t changed, std::size_t oldFill, std::size_t newFill)
> +			: l_(&l),
> +			  changed_(changed),
> +			  start_(l.contentOffset_ + oldFill),
> +			  stop_(l.contentOffset_ + newFill)
> +		{
> +		}
> +
> +		friend MetadataList;
> +		friend struct Checkpoint;
> +
> +		const MetadataList *l_ = nullptr;
> +		std::size_t changed_;
> +		std::size_t start_;
> +		std::size_t stop_;
> +	};
> +
> +	Diff merge(const ControlList &other)

So the "merge" function is how pipeline handlers adds metadata to a
MetadataList ?

> +	{
> +		// TODO: check id map of `other`?

This already is a proper /* \todo */

> +
> +		const auto c = checkpoint();
> +
> +		for (const auto &[tag, value] : other) {
> +			auto *e = find(tag);
> +			if (e) {
> +				[[maybe_unused]] auto r = set(*e, value);
> +				assert(r == SetError() || r == SetError::AlreadySet); // TODO: ?

does "r == SetError()" means r == 0 ? What's the TODO for ?

> +			}
> +		}
> +
> +		return c.diffSince();
> +	}
> +
> +	class Checkpoint
> +	{
> +	public:
> +		[[nodiscard]] Diff diffSince() const
> +		{
> +			/* sync with release-store on `state_` in `set()` */
> +			const auto curr = l_->state_.load(std::memory_order_acquire);
> +
> +			assert(s_.count <= curr.count);
> +			assert(s_.fill <= curr.fill);
> +
> +			return {
> +				*l_,
> +				curr.count - s_.count,
> +				s_.fill,
> +				curr.fill,
> +			};
> +		}
> +
> +	private:
> +		Checkpoint(const MetadataList &l)
> +			: l_(&l),
> +			  s_(l.state_.load(std::memory_order_relaxed))
> +		{
> +		}
> +
> +		friend MetadataList;
> +
> +		const MetadataList *l_ = nullptr;

Is there in your opinion a case where a Checkpoint can overlive the
MetadataList it refers to ?

> +		State s_ = {};
> +	};
> +
> +	[[nodiscard]] Checkpoint checkpoint() const
> +	{
> +		return { *this };
> +	}
> +
> +private:
> +	[[nodiscard]] static constexpr std::size_t entriesOffset()
> +	{
> +		return 0;
> +	}
> +
> +	[[nodiscard]] static constexpr std::size_t contentOffset(std::size_t entries)
> +	{
> +		return details::align::up(entriesOffset() + entries * sizeof(Entry), alignof(ValueHeader));
> +	}
> +
> +	[[nodiscard]] Span<Entry> entries() const
> +	{
> +		return { reinterpret_cast<Entry *>(p_ + entriesOffset()), capacity_ };
> +	}
> +
> +	[[nodiscard]] Entry *find(std::uint32_t tag) const
> +	{
> +		const auto entries = this->entries();
> +		auto it = std::partition_point(entries.begin(), entries.end(), [&](const auto &e) {

how is this different from find_if() ? Just out of curiosity

I'll stop here for the time being. Just one more note, you moved
MetadataListPlan out this header, so you probably need a dedicated
.cpp file as well


> +			return e.tag < tag;
> +		});
> +
> +		if (it == entries.end() || it->tag != tag)
> +			return nullptr;
> +
> +		return &*it;
> +	}
> +
> +	[[nodiscard]] ControlValueView data_of(const Entry &e) const
> +	{
> +		const auto o = e.acquireData();
> +		return o ? data_of(*o) : ControlValueView{ };
> +	}
> +
> +	[[nodiscard]] ControlValueView data_of(std::size_t headerOffset) const
> +	{
> +		assert(headerOffset <= alloc_ - sizeof(ValueHeader));
> +		assert(details::align::is(p_ + headerOffset, alignof(ValueHeader)));
> +
> +		const auto *vh = reinterpret_cast<const ValueHeader *>(p_ + headerOffset);
> +		const auto *p = reinterpret_cast<const std::byte *>(vh) + sizeof(*vh);
> +		std::size_t avail = p_ + alloc_ - p;
> +
> +		const auto *data = details::align::up(vh->size, vh->alignment, p, &avail);
> +		assert(data);
> +
> +		return { vh->params.type, vh->params.isArray, vh->params.numElements, data };
> +	}
> +
> +	[[nodiscard]] SetError set(Entry &e, ControlValueView v)
> +	{
> +		if (e.hasValue())
> +			return SetError::AlreadySet;
> +		if (e.type != v.type() || e.isArray != v.isArray())
> +			return SetError::TypeMismatch;
> +
> +		const auto src = v.data();
> +		if (e.isArray) {
> +			if (src.size_bytes() > e.capacity)
> +				return SetError::SizeMismatch;
> +		} else {
> +			if (src.size_bytes() != e.capacity)
> +				return SetError::SizeMismatch;
> +		}
> +
> +		auto s = state_.load(std::memory_order_relaxed);
> +		std::byte *oldEnd = p_ + contentOffset_ + s.fill;
> +		std::byte *p = oldEnd;
> +
> +		auto *headerPtr = details::align::up<ValueHeader>(p);
> +		auto *dataPtr = details::align::up(src.size_bytes(), e.alignment, p);
> +		details::align::up(0, alignof(ValueHeader), p);
> +
> +#if HAS_ASAN
> +		::__sanitizer_annotate_contiguous_container(
> +			p_ + contentOffset_, p_ + alloc_,
> +			oldEnd, p
> +		);
> +#endif
> +
> +		new (headerPtr) ValueHeader{
> +			.tag = e.tag,
> +			.size = std::uint32_t(src.size_bytes()),
> +			.alignment = e.alignment,
> +			.params = {
> +				.type = v.type(),
> +				.isArray = v.isArray(),
> +				.numElements = std::uint32_t(v.numElements()),
> +			},
> +		};
> +		std::memcpy(dataPtr, src.data(), src.size_bytes());
> +		e.headerOffset.store(reinterpret_cast<std::byte *>(headerPtr) - p_, std::memory_order_release);
> +
> +		s.fill += p - oldEnd;
> +		s.count += 1;
> +
> +		state_.store(s, std::memory_order_release);
> +
> +		return {};
> +	}
> +
> +	std::size_t capacity_ = 0;
> +	std::size_t contentOffset_ = -1;
> +	std::size_t alloc_ = 0;
> +	std::atomic<State> state_ = State{};
> +	std::byte *p_ = nullptr;
> +	// TODO: ControlIdMap in any way shape or form?
> +
> +	/*
> +	 * If this is problematic on a 32-bit architecture, then
> +	 * `count` can be stored in a separate atomic variable
> +	 * but then `Diff::changed_` must be removed since the fill
> +	 * level and item count cannot be retrieved atomically.
> +	 */
> +	static_assert(decltype(state_)::is_always_lock_free);
> +};
> +
> +} /* namespace libcamera */
> +
> +#undef HAS_ASAN
> diff --git a/include/libcamera/metadata_list_plan.h b/include/libcamera/metadata_list_plan.h
> new file mode 100644
> index 000000000..2ed35c54f
> --- /dev/null
> +++ b/include/libcamera/metadata_list_plan.h
> @@ -0,0 +1,130 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2025, Ideas On Board Oy
> + */
> +
> +#pragma once
> +
> +#include <cassert>
> +#include <cstddef>
> +#include <cstdint>
> +#include <limits>
> +#include <map>
> +#include <type_traits>
> +
> +#include <libcamera/base/details/cxx20.h>
> +
> +#include <libcamera/controls.h>
> +
> +namespace libcamera {
> +
> +class MetadataListPlan
> +{
> +public:
> +	struct Entry {
> +		std::uint32_t size;
> +		std::uint32_t alignment; // TODO: is this necessary?
> +		std::uint32_t numElements;
> +		ControlType type;
> +		bool isArray;
> +	};
> +
> +	[[nodiscard]] bool empty() const { return items_.empty(); }
> +	[[nodiscard]] std::size_t size() const { return items_.size(); }
> +	[[nodiscard]] decltype(auto) begin() const { return items_.begin(); }
> +	[[nodiscard]] decltype(auto) end() const { return items_.end(); }
> +	void clear() { items_.clear(); }
> +
> +	template<
> +		typename T,
> +		std::enable_if_t<libcamera::details::control_type<T>::size != libcamera::dynamic_extent> * = nullptr
> +	>
> +	decltype(auto) set(const Control<T> &ctrl)
> +	{
> +		if constexpr (libcamera::details::control_type<T>::size > 0) {
> +			static_assert(libcamera::details::control_type<T>::size != libcamera::dynamic_extent);
> +
> +			return set<typename T::value_type>(
> +				ctrl.id(),
> +				libcamera::details::control_type<T>::size,
> +				true
> +			);
> +		} else {
> +			return set<T>(ctrl.id(), 1, false);
> +		}
> +	}
> +
> +	template<
> +		typename T,
> +		std::enable_if_t<libcamera::details::control_type<T>::size == libcamera::dynamic_extent> * = nullptr
> +	>
> +	decltype(auto) set(const Control<T> &ctrl, std::size_t numElements)
> +	{
> +		return set<typename T::value_type>(ctrl.id(), numElements, true);
> +	}
> +
> +	[[nodiscard]] bool set(std::uint32_t tag,
> +			       std::size_t size, std::size_t alignment,
> +			       std::size_t numElements, ControlType type, bool isArray)
> +	{
> +		if (size == 0 || size > std::numeric_limits<std::uint32_t>::max())
> +			return false;
> +		if (alignment > std::numeric_limits<std::uint32_t>::max())
> +			return false;
> +		if (!details::cxx20::has_single_bit(alignment))
> +			return false;
> +		if (numElements > std::numeric_limits<std::uint32_t>::max() / size)
> +			return false;
> +		if (!isArray && numElements != 1)
> +			return false;
> +
> +		items_.insert_or_assign(tag, Entry{
> +			.size = std::uint32_t(size),
> +			.alignment = std::uint32_t(alignment),
> +			.numElements = std::uint32_t(numElements),
> +			.type = type,
> +			.isArray = isArray,
> +		});
> +
> +		return true;
> +	}
> +
> +	[[nodiscard]] const Entry *get(std::uint32_t tag) const
> +	{
> +		auto it = items_.find(tag);
> +		if (it == items_.end())
> +			return nullptr;
> +
> +		return &it->second;
> +	}
> +
> +	[[nodiscard]] const Entry *get(const ControlId &cid) const
> +	{
> +		const auto *e = get(cid.id());
> +		if (!e)
> +			return nullptr;
> +
> +		if (e->type != cid.type() || e->isArray != cid.isArray())
> +			return nullptr;
> +
> +		return e;
> +	}
> +
> +private:
> +	std::map<std::uint32_t, Entry> items_;
> +
> +	template<typename T>
> +	decltype(auto) set(std::uint32_t tag, std::size_t numElements, bool isArray)
> +	{
> +		static_assert(std::is_trivially_copyable_v<T>);
> +
> +		[[maybe_unused]] bool ok = set(tag,
> +					       sizeof(T), alignof(T),
> +					       numElements, details::control_type<T>::value, isArray);
> +		assert(ok);
> +
> +		return *this;
> +	}
> +};
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index de1eb99b2..8c5ce4503 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -9,6 +9,7 @@ libcamera_public_sources = files([
>      'framebuffer.cpp',
>      'framebuffer_allocator.cpp',
>      'geometry.cpp',
> +    'metadata_list.cpp',
>      'orientation.cpp',
>      'pixel_format.cpp',
>      'request.cpp',
> diff --git a/src/libcamera/metadata_list.cpp b/src/libcamera/metadata_list.cpp
> new file mode 100644
> index 000000000..ebefdfdad
> --- /dev/null
> +++ b/src/libcamera/metadata_list.cpp
> @@ -0,0 +1,344 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2025, Ideas On Board Oy
> + */
> +
> +#include <libcamera/metadata_list.h>
> +
> +namespace libcamera {
> +
> +/**
> + * \class MetadataListPlan
> + * \brief Class to hold the possible set of metadata items for a MetadataList
> + */
> +
> +/**
> + * \class MetadataListPlan::Entry
> + * \brief Details of a metadata item
> + */
> +
> +/**
> + * \internal
> + * \var MetadataListPlan::Entry::size
> + * \brief Number of bytes in a single element
> + *
> + * \var MetadataListPlan::Entry::alignment
> + * \brief Required alignment of the elements
> + * \endinternal
> + *
> + * \var MetadataListPlan::Entry::numElements
> + * \brief Number of elements in the value
> + * \sa ControlValueView::numElements()
> + *
> + * \var MetadataListPlan::Entry::type
> + * \brief The type of the value
> + * \sa ControlValueView::type()
> + *
> + * \var MetadataListPlan::Entry::isArray
> + * \brief Whether or not the value is array-like
> + * \sa ControlValueView::isArray()
> + */
> +
> +/**
> + * \fn MetadataListPlan::begin() const
> + * \brief Retrieve the begin iterator
> + */
> +
> +/**
> + * \fn MetadataListPlan::end() const
> + * \brief Retrieve the end iterator
> + */
> +
> +/**
> + * \fn MetadataListPlan::size() const
> + * \brief Retrieve the number of entries
> + */
> +
> +/**
> + * \fn MetadataListPlan::empty() const
> + * \brief Check if empty
> + */
> +
> +/**
> + * \internal
> + * \fn MetadataListPlan::clear()
> + * \brief Remove all controls
> + */
> +
> +/**
> + * \internal
> + * \fn MetadataListPlan::set(const Control<T> &ctrl)
> + * \brief Add an entry for the given control to the metadata list plan
> + * \param[in] ctrl The control
> + */
> +
> +/**
> + * \internal
> + * \fn MetadataListPlan::set(const Control<T> &ctrl, std::size_t count)
> + * \brief Add an entry for the given dynamically-sized control to the metadata list plan
> + * \param[in] ctrl The control
> + * \param[in] count The maximum number of elements
> + *
> + * Add the dynamically-sized control \a ctrl to the metadata list plan with a maximum
> + * capacity of \a count elements.
> + */
> +
> +/**
> + * \internal
> + * \fn MetadataListPlan::set(std::uint32_t tag,
> + *			     std::size_t size, std::size_t alignment,
> + *			     std::size_t count, ControlType type, bool isArray)
> + * \brief Add an entry to the metadata list plan
> + * \return \a true if the entry has been added, or \a false if the given parameters
> + *         would result in an invalid entry
> + *
> + * This functions adds an entry with essentially arbitrary parameters, without deriving
> + * them from a given ControlId instance. This is mainly used when deserializing.
> + */
> +
> +/**
> + * \fn MetadataListPlan::get(std::uint32_t tag) const
> + * \brief Find the \ref Entry "entry" with the given identifier
> + */
> +
> +/**
> + * \fn MetadataListPlan::get(const ControlId &cid) const
> + * \brief Find the \ref Entry "entry" for the given ControlId
> + *
> + * The \ref Entry "entry" is only returned if ControlId::type() and ControlId::isArray()
> + * of \a cid matches Entry::type and Entry::isArray, respectively.
> + */
> +
> +/**
> + * \class MetadataList
> + * \brief Class to hold metadata items
> + */
> +
> +/**
> + * \fn MetadataList::MetadataList(const MetadataListPlan &plan)
> + * \brief Construct a metadata list according to \a plan
> + *
> + * Construct a metadata list according to the provided \a plan.
> + */
> +
> +/**
> + * \fn MetadataList::size() const
> + * \brief Retrieve the number of controls
> + * \context This function is \threadsafe.
> + * \note If the list is being modified, the return value may be out of
> + *       date by the time the function returns
> + */
> +
> +/**
> + * \fn MetadataList::empty() const
> + * \brief Check if empty
> + * \context This function is \threadsafe.
> + * \note If the list is being modified, the return value may be out of
> + *       date by the time the function returns
> + */
> +
> +/**
> + * \internal
> + * \fn MetadataList::clear()
> + * \brief Remove all items from the list
> + * \note This function in effect resets the list to its original state. As a consequence it invalidates - among others -
> + *       all iterators, Checkpoint, and Diff objects that are associated with the list. No readers must exist
> + *       when this function is called.
> + */
> +
> +/**
> + * \fn MetadataList::begin() const
> + * \brief Retrieve begin iterator
> + * \context This function is \threadsafe.
> + */
> +
> +/**
> + * \fn MetadataList::end() const
> + * \brief Retrieve end iterator
> + * \context This function is \threadsafe.
> + */
> +
> +/**
> + * \fn MetadataList::get(const Control<T> &ctrl) const
> + * \brief Get the value of control \a ctrl
> + * \return A std::optional<T> containing the control value, or std::nullopt if
> + *         the control \a ctrl is not present in the list
> + * \context This function is \threadsafe.
> + */
> +
> +/**
> + * \fn MetadataList::get(std::uint32_t tag) const
> + * \brief Get the value of pertaining to the numeric identifier \a tag
> + * \return A std::optional<T> containing the control value, or std::nullopt if
> + *         the control is not present in the list
> + * \context This function is \threadsafe.
> + */
> +
> +/**
> + * \internal
> + * \fn MetadataList::set(const Control<T> &ctrl, const details::cxx20::type_identity_t<T> &value)
> + * \brief Set the value of control \a ctrl to \a value
> + */
> +
> +/**
> + * \internal
> + * \fn MetadataList::set(std::uint32_t tag, ControlValueView v)
> + * \brief Set the value of pertaining to the numeric identifier \a tag to \a v
> + */
> +
> +/**
> + * \internal
> + * \fn MetadataList::merge(const ControlList &other)
> + * \brief Add all missing items from \a other
> + *
> + * Add all items from \a other that are not present in \a this.
> + */
> +
> +/**
> + * \internal
> + * \enum MetadataList::SetError
> + * \brief Error code returned by a set operation
> + *
> + * \var MetadataList::SetError::UnknownTag
> + * \brief The tag is not supported by the metadata list
> + * \var MetadataList::SetError::AlreadySet
> + * \brief A value has already been added with the given tag
> + * \var MetadataList::SetError::SizeMismatch
> + * \brief The size of the data is not appropriate for the given tag
> + * \var MetadataList::SetError::TypeMismatch
> + * \brief The type of the value does not match the expected type
> + */
> +
> +/**
> + * \internal
> + * \fn MetadataList::checkpoint() const
> + * \brief Create a checkpoint
> + * \context This function is \threadsafe.
> + */
> +
> +/**
> + * \class MetadataList::iterator
> + * \brief Iterator
> + */
> +
> +/**
> + * \typedef MetadataList::iterator::difference_type
> + * \brief iterator's difference type
> + */
> +
> +/**
> + * \typedef MetadataList::iterator::value_type
> + * \brief iterator's value type
> + */
> +
> +/**
> + * \typedef MetadataList::iterator::pointer
> + * \brief iterator's pointer type
> + */
> +
> +/**
> + * \typedef MetadataList::iterator::reference
> + * \brief iterator's reference type
> + */
> +
> +/**
> + * \typedef MetadataList::iterator::iterator_category
> + * \brief iterator's category
> + */
> +
> +/**
> + * \fn MetadataList::iterator::operator*()
> + * \brief Retrieve value at iterator
> + * \return A \a ControlListView representing the value
> + */
> +
> +/**
> + * \fn MetadataList::iterator::operator==(const iterator &other) const
> + * \brief Check if two iterators are equal
> + */
> +
> +/**
> + * \fn MetadataList::iterator::operator!=(const iterator &other) const
> + * \brief Check if two iterators are not equal
> + */
> +
> +/**
> + * \fn MetadataList::iterator::operator++(int)
> + * \brief Advance the iterator
> + */
> +
> +/**
> + * \fn MetadataList::iterator::operator++()
> + * \brief Advance the iterator
> + */
> +
> +/**
> + * \class MetadataList::Diff
> + * \brief Designates a set of consecutively added metadata items from a particular MetadataList
> + * \sa Camera::metadataAvailable
> + * \internal
> + * \sa MetadataList::Checkpoint::diffSince()
> + * \endinternal
> + */
> +
> +/**
> + * \fn MetadataList::Diff::list() const
> + * \brief Retrieve the associated MetadataList
> + */
> +
> +/**
> + * \fn MetadataList::Diff::size() const
> + * \brief Retrieve the number of metadata items designated
> + */
> +
> +/**
> + * \fn MetadataList::Diff::empty() const
> + * \brief Check if any metadata items are designated
> + */
> +
> +/**
> + * \fn MetadataList::Diff::operator bool() const
> + * \copydoc MetadataList::Diff::empty() const
> + */
> +
> +/**
> + * \fn MetadataList::Diff::get(const Control<T> &ctrl) const
> + * \copydoc MetadataList::get(const Control<T> &ctrl) const
> + * \note The lookup will fail if the metadata item is not designated by this Diff object,
> + *       even if it is otherwise present in the backing MetadataList.
> + */
> +
> +/**
> + * \fn MetadataList::Diff::get(std::uint32_t tag) const
> + * \copydoc MetadataList::get(std::uint32_t tag) const
> + * \note The lookup will fail if the metadata item is not designated by this Diff object,
> + *       even if it is otherwise present in the backing MetadataList.
> + */
> +
> +/**
> + * \fn MetadataList::Diff::begin() const
> + * \brief Retrieve the begin iterator
> + */
> +
> +/**
> + * \fn MetadataList::Diff::end() const
> + * \brief Retrieve the end iterator
> + */
> +
> +/**
> + * \internal
> + * \class MetadataList::Checkpoint
> + * \brief Designates a point in the stream of metadata items
> + *
> + * A Checkpoint object designates a point in the stream of metadata items in the associated
> + * MetadataList. Its main use to be able to retrieve the set of metadata items that were
> + * added to the list after the designated point using diffSince().
> + */
> +
> +/**
> + * \internal
> + * \fn MetadataList::Checkpoint::diffSince() const
> + * \brief Retrieve the set of metadata items added since the checkpoint was created
> + */
> +
> +} /* namespace libcamera */
> diff --git a/test/controls/meson.build b/test/controls/meson.build
> index 763f8905e..b68a4fc53 100644
> --- a/test/controls/meson.build
> +++ b/test/controls/meson.build
> @@ -5,6 +5,7 @@ control_tests = [
>      {'name': 'control_info_map', 'sources': ['control_info_map.cpp']},
>      {'name': 'control_list', 'sources': ['control_list.cpp']},
>      {'name': 'control_value', 'sources': ['control_value.cpp']},
> +    {'name': 'metadata_list', 'sources': ['metadata_list.cpp']},
>  ]
>
>  foreach test : control_tests
> diff --git a/test/controls/metadata_list.cpp b/test/controls/metadata_list.cpp
> new file mode 100644
> index 000000000..f0872acd9
> --- /dev/null
> +++ b/test/controls/metadata_list.cpp
> @@ -0,0 +1,170 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2025, Ideas On Board Oy
> + *
> + * MetadataList tests
> + */
> +
> +#include <future>
> +#include <iostream>
> +#include <thread>
> +
> +#include <libcamera/control_ids.h>
> +#include <libcamera/metadata_list.h>
> +#include <libcamera/property_ids.h>
> +
> +#include "test.h"
> +
> +using namespace std;
> +using namespace libcamera;
> +
> +#define ASSERT(x) do { \
> +	if (!static_cast<bool>(x)) { \
> +		std::cerr << '`' << #x << "` failed" << std::endl; \
> +		return TestFail; \
> +	} \
> +} while (false)
> +
> +class MetadataListTest : public Test
> +{
> +public:
> +	MetadataListTest() = default;
> +
> +protected:
> +	int run() override
> +	{
> +		MetadataListPlan mlp;
> +		mlp.set(controls::ExposureTime);
> +		mlp.set(controls::ExposureValue);
> +		mlp.set(controls::ColourGains);
> +		mlp.set(controls::AfWindows, 10);
> +		mlp.set(controls::AeEnable);
> +		mlp.set(controls::SensorTimestamp);
> +
> +		MetadataList ml(mlp);
> +
> +		static_assert(static_cast<unsigned int>(properties::LOCATION) == controls::AE_ENABLE);
> +		ASSERT(ml.set(properties::Location, properties::CameraLocationFront) == MetadataList::SetError::TypeMismatch);
> +
> +		ASSERT(ml.set(controls::AfWindows, std::array<Rectangle, 11>{}) == MetadataList::SetError::SizeMismatch);
> +		ASSERT(ml.set(controls::ColourTemperature, 123) == MetadataList::SetError::UnknownTag);
> +
> +		auto f1 = std::async(std::launch::async, [&] {
> +			using namespace std::chrono_literals;
> +
> +			std::this_thread::sleep_for(500ms);
> +			ASSERT(ml.set(controls::ExposureTime, 0x1111) == MetadataList::SetError());
> +
> +			std::this_thread::sleep_for(500ms);
> +			ASSERT(ml.set(controls::ExposureValue, 1) == MetadataList::SetError());
> +
> +			std::this_thread::sleep_for(500ms);
> +			ASSERT(ml.set(controls::ColourGains, std::array{
> +				123.f,
> +				456.f
> +			}) == MetadataList::SetError());
> +
> +			std::this_thread::sleep_for(500ms);
> +			ASSERT(ml.set(controls::AfWindows, std::array{
> +				Rectangle(),
> +				Rectangle(1, 2, 3, 4),
> +				Rectangle(0x1111, 0x2222, 0x3333, 0x4444),
> +			}) == MetadataList::SetError());
> +
> +			return TestPass;
> +		});
> +
> +		auto f2 = std::async(std::launch::async, [&] {
> +			for (;;) {
> +				const auto x = ml.get(controls::ExposureTime);
> +				const auto y = ml.get(controls::ExposureValue);
> +				const auto z = ml.get(controls::ColourGains);
> +				const auto w = ml.get(controls::AfWindows);
> +
> +				if (x)
> +					ASSERT(*x == 0x1111);
> +
> +				if (y)
> +					ASSERT(*y == 1.0f);
> +
> +				if (z) {
> +					ASSERT(z->size() == 2);
> +					ASSERT((*z)[0] == 123.f);
> +					ASSERT((*z)[1] == 456.f);
> +				}
> +
> +				if (w) {
> +					ASSERT(w->size() == 3);
> +					ASSERT((*w)[0].isNull());
> +					ASSERT((*w)[1] == Rectangle(1, 2, 3, 4));
> +					ASSERT((*w)[2] == Rectangle(0x1111, 0x2222, 0x3333, 0x4444));
> +				}
> +
> +				if (x && y && z && w)
> +					break;
> +			}
> +
> +			return TestPass;
> +		});
> +
> +		ASSERT(f1.get() == TestPass);
> +		ASSERT(f2.get() == TestPass);
> +
> +		ASSERT(ml.set(controls::ExposureTime, 0x2222) == MetadataList::SetError::AlreadySet);
> +		ASSERT(ml.set(controls::ExposureValue, 2) == MetadataList::SetError::AlreadySet);
> +
> +		ASSERT(ml.get(controls::ExposureTime) == 0x1111);
> +		ASSERT(ml.get(controls::ExposureValue) == 1);
> +
> +		for (auto &&[tag, v] : ml)
> +			std::cout << "[" << tag << "] -> " << v << '\n';
> +
> +		std::cout << std::endl;
> +
> +		ml.clear();
> +		ASSERT(ml.empty());
> +		ASSERT(ml.size() == 0);
> +
> +		ASSERT(ml.set(controls::ExposureTime, 0x2222) == MetadataList::SetError());
> +		ASSERT(ml.get(controls::ExposureTime) == 0x2222);
> +
> +		auto c = ml.checkpoint();
> +
> +		ASSERT(ml.set(controls::ExposureValue, 2) == MetadataList::SetError());
> +		ASSERT(ml.set(controls::SensorTimestamp, 0x99999999) == MetadataList::SetError());
> +
> +		auto d = c.diffSince();
> +		ASSERT(&d.list() == &ml);
> +
> +		ASSERT(ml.set(controls::ColourGains, std::array{ 1.f, 2.f }) == MetadataList::SetError());
> +
> +		ASSERT(d);
> +		ASSERT(!d.empty());
> +		ASSERT(d.size() == 2);
> +		ASSERT(!d.get(controls::ExposureTime));
> +		ASSERT(!d.get(controls::ColourGains));
> +		ASSERT(!d.get(controls::AfWindows));
> +		ASSERT(d.get(controls::ExposureValue) == 2);
> +		ASSERT(d.get(controls::SensorTimestamp) == 0x99999999);
> +
> +		for (auto &&[tag, v] : d)
> +			std::cout << "[" << tag << "] -> " << v << '\n';
> +
> +		/* Test if iterators work with algorithms. */
> +		std::ignore = std::find_if(d.begin(), d.end(), [](const auto &) {
> +			return false;
> +		});
> +
> +#if 0
> +               {
> +                       auto it = ml.begin();
> +                       ml.clear();
> +                       std::ignore = *it; /* Trigger ASAN. */
> +               }
> +#endif
> +
> +		return TestPass;
> +	}
> +};
> +
> +TEST_REGISTER(MetadataListTest)
> --
> 2.50.1
>

Hi Barnabás
  let me resume the review from where I stopped last time

On Fri, Jul 25, 2025 at 06:08:28PM +0200, Jacopo Mondi wrote:
> Hi Barnabás
>
> On Mon, Jul 21, 2025 at 12:46:07PM +0200, Barnabás Pőcze wrote:
> > Add a dedicated `MetadataList` type, whose purpose is to store the metadata
> > reported by a camera for a given request. Previously, a `ControlList` was
> > used for this purpose. The reason for introducing a separate type is to
> > simplify the access to the returned metadata during the entire lifetime
> > of a request.
> >
> > Specifically, for early metadata completion to be easily usable it should be
> > guaranteed that any completed metadata item can be accessed and looked up
> > at least until the associated requested is reused with `Request::reuse()`.
> >
> > However, when a metadata item is completed early, the pipeline handler
> > might still work on the request in the `CameraManager`'s private thread,
> > therefore there is an inherent synchronization issue when an application
> > accesses early metadata.
> >
> > Restricting the user to only access the metadata items of a not yet completed
> > request in the early metadata availability signal handler by ways of
> > documenting or enforcing it at runtime could be an option, but it is not
> > too convenient for the user.
> >
> > The current `ControlList` implementation employs an `std::unordered_map`,
> > so pointers remain stable when the container is modified, so an application
> > could keep accessing particular metadata items outside the signal handler,
> > but this fact is far from obvious, and the user would still not be able
> > to make a copy of all metadata or do lookups based on the numeric ids or
> > the usual `libcamera::Control<>` objects, thus some type safety is lost.
> >
> > The above also requires that each metadata item is only completed once for
> > a given request, but this does not appear to be serious limitation,
> > and in fact, this restriction is enforced by `MetadataList`.
> >
> > The introduced `MetadataList` supports single writer - multiple reader
> > scenarios, and it can be set, looked-up, and copied in a wait-free fashion
> > without introducing data races or other synchronization issues. This is
> > achieved by requiring the possible set of metadata items to be known
> > (such set is stored in a `MetadataListPlan` object). Based on the this
> > plan, a single contiguous allocation is made to accommodate all potential
> > metadata items. Due to this single contiguous allocation that is not modified
> > during the lifetime of a `MetadataList` and atomic modifications, it is
> > possible to easily gaurantee thread-safe set, lookup, and copy; assuming
> > there is only ever a single writer.
> >
> > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > ---
> > changes in v2:
> >   * remove multiple not strictly necessary functions
> > ---
> >  include/libcamera/meson.build          |   2 +
> >  include/libcamera/metadata_list.h      | 547 +++++++++++++++++++++++++
> >  include/libcamera/metadata_list_plan.h | 130 ++++++
> >  src/libcamera/meson.build              |   1 +
> >  src/libcamera/metadata_list.cpp        | 344 ++++++++++++++++
> >  test/controls/meson.build              |   1 +
> >  test/controls/metadata_list.cpp        | 170 ++++++++
> >  7 files changed, 1195 insertions(+)
> >  create mode 100644 include/libcamera/metadata_list.h
> >  create mode 100644 include/libcamera/metadata_list_plan.h
> >  create mode 100644 src/libcamera/metadata_list.cpp
> >  create mode 100644 test/controls/metadata_list.cpp
> >
> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > index 30ea76f94..410b548dd 100644
> > --- a/include/libcamera/meson.build
> > +++ b/include/libcamera/meson.build
> > @@ -12,6 +12,8 @@ libcamera_public_headers = files([
> >      'framebuffer_allocator.h',
> >      'geometry.h',
> >      'logging.h',
> > +    'metadata_list.h',
> > +    'metadata_list_plan.h',
> >      'orientation.h',
> >      'pixel_format.h',
> >      'request.h',
> > diff --git a/include/libcamera/metadata_list.h b/include/libcamera/metadata_list.h
> > new file mode 100644
> > index 000000000..7fe3dbbab
> > --- /dev/null
> > +++ b/include/libcamera/metadata_list.h
> > @@ -0,0 +1,547 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2025, Ideas On Board Oy
> > + *
> > + * Metadata list
> > + */
> > +
> > +#pragma once
> > +
> > +#include <algorithm>
> > +#include <atomic>
> > +#include <cassert>
> > +#include <cstdint>
> > +#include <cstring>
> > +#include <new>
> > +#include <optional>
> > +#include <type_traits>
> > +
> > +#include <libcamera/base/details/align.h>
> > +#include <libcamera/base/details/cxx20.h>
> > +#include <libcamera/base/span.h>
> > +
> > +#include <libcamera/controls.h>
> > +#include <libcamera/metadata_list_plan.h>
> > +
> > +// TODO: want this?
> > +#if __has_include(<sanitizer/asan_interface.h>)
> > +#if __SANITIZE_ADDRESS__ /* gcc */
> > +#include <sanitizer/asan_interface.h>
> > +#define HAS_ASAN 1
> > +#elif defined(__has_feature)
> > +#if __has_feature(address_sanitizer) /* clang */
> > +#include <sanitizer/asan_interface.h>
> > +#define HAS_ASAN 1
> > +#endif
> > +#endif
> > +#endif
> > +
> > +namespace libcamera {
> > +
> > +class MetadataList
> > +{
> > +private:
> > +	struct ValueParams {
> > +		ControlType type;
> > +		bool isArray;
> > +		std::uint32_t numElements;
> > +	};
> > +
> > +	struct Entry {
> > +		const std::uint32_t tag;
> > +		const std::uint32_t capacity;
> > +		const std::uint32_t alignment;
> > +		const ControlType type;
> > +		bool isArray;
> > +
> > +		static constexpr std::uint32_t invalidOffset = -1;
> > +		/*
> > +		 * Offset from the beginning of the allocation, and
> > +		 * and _not_ relative to `contentOffset_`.
> > +		 */
> > +		std::atomic_uint32_t headerOffset = invalidOffset;
> > +
> > +		[[nodiscard]] std::optional<std::uint32_t> hasValue() const
> > +		{
> > +			auto offset = headerOffset.load(std::memory_order_relaxed);
> > +			if (offset == invalidOffset)
> > +				return {};
> > +
> > +			return offset;
> > +		}
> > +
> > +		[[nodiscard]] std::optional<std::uint32_t> acquireData() const
> > +		{
> > +			auto offset = hasValue();
> > +			if (offset) {
> > +				/* sync with release-store on `headerOffset` in `MetadataList::set()` */
> > +				std::atomic_thread_fence(std::memory_order_acquire);
> > +			}
> > +
> > +			return offset;
> > +		}
> > +	};
> > +
> > +	struct ValueHeader {
> > +		std::uint32_t tag;
> > +		std::uint32_t size;
> > +		std::uint32_t alignment;
> > +		ValueParams params;
> > +	};
> > +
> > +	struct State {
> > +		std::uint32_t count;
> > +		std::uint32_t fill;
> > +	};
> > +
> > +public:
> > +	explicit MetadataList(const MetadataListPlan &plan)
> > +		: capacity_(plan.size()),
> > +		  contentOffset_(MetadataList::contentOffset(capacity_)),
> > +		  alloc_(contentOffset_)
> > +	{
> > +		for (const auto &[tag, e] : plan) {
> > +			alloc_ += sizeof(ValueHeader);
> > +			alloc_ += e.alignment - 1; // XXX: this is the maximum
> > +			alloc_ += e.size * e.numElements;
> > +			alloc_ += alignof(ValueHeader) - 1; // XXX: this is the maximum
> > +		}
> > +
> > +		p_ = static_cast<std::byte *>(::operator new(alloc_));
> > +
> > +		auto *entries = reinterpret_cast<Entry *>(p_ + entriesOffset());
> > +		auto it = plan.begin();
> > +
> > +		for (std::size_t i = 0; i < capacity_; i++, ++it) {
> > +			const auto &[tag, e] = *it;
> > +
> > +			new (&entries[i]) Entry{
> > +				.tag = tag,
> > +				.capacity = e.size * e.numElements,
> > +				.alignment = e.alignment,
> > +				.type = e.type,
> > +				.isArray = e.isArray,
> > +			};
>
> I was already about to rant against C++'s "do the same usual thing in
> some different way" style and wanted to suggest
>
> 			entries[i].tag = tag;
> 			entries[i].capacity = e.size * e.numElements,
> 			entries[i].alignment = e.alignment;
> 			entries[i].type = e.type;
> 			entries[i].isArray = e.isArray;
>
> But then you loose the const-ness of entries[i]'s fields.
>
> Let's use "placement new" then...
>
>
> > +		}
> > +
> > +#if HAS_ASAN
> > +		::__sanitizer_annotate_contiguous_container(
> > +			p_ + contentOffset_, p_ + alloc_,
> > +			p_ + alloc_, p_ + contentOffset_
> > +		);
> > +#endif
> > +	}
> > +
> > +	MetadataList(const MetadataList &) = delete;
> > +	MetadataList(MetadataList &&) = delete;
> > +
> > +	MetadataList &operator=(const MetadataList &) = delete;
> > +	MetadataList &operator=(MetadataList &&) = delete;
> > +
> > +	~MetadataList()
> > +	{
> > +#if HAS_ASAN
> > +		/*
> > +		 * The documentation says the range apparently has to be
> > +		 * restored to its initial state before it is deallocated.
> > +		 */
> > +		::__sanitizer_annotate_contiguous_container(
> > +			p_ + contentOffset_, p_ + alloc_,
> > +			p_ + contentOffset_ + state_.load(std::memory_order_relaxed).fill, p_ + alloc_
> > +		);
> > +#endif
> > +
> > +		::operator delete(p_, alloc_);
> > +	}
> > +
> > +	// TODO: want these?
>
> Why not ?
>
> > +	[[nodiscard]] std::size_t size() const { return state_.load(std::memory_order_relaxed).count; }
> > +	[[nodiscard]] bool empty() const { return state_.load(std::memory_order_relaxed).fill == 0; }
> > +
> > +	enum class SetError {
> > +		UnknownTag = 1,
> > +		AlreadySet,
> > +		SizeMismatch,
> > +		TypeMismatch,
> > +	};
>
> The only user of "set" will be pipeline handler, so I wonder if coding
> the errors as enum is really something that gives any value here or
> it's just overdesign. It's not like pipelines will do
>
>         if (AlreadySet)
>                 doSomething
>
> If they get an error when adding metadata it will be caught during
> development I presume.
>
> > +
> > +	[[nodiscard]] SetError set(std::uint32_t tag, ControlValueView v)
>
> Since we end up copying the data into the MetadataList, why use a
> ControlValueView and not a ControlValue ?
>
> > +	{
> > +		auto *e = find(tag);
> > +		if (!e)
> > +			return SetError::UnknownTag;
> > +
> > +		return set(*e, v);
> > +	}
> > +
> > +	template<typename T>
> > +	/* TODO: [[nodiscard]] */ SetError set(const Control<T> &ctrl, const details::cxx20::type_identity_t<T> &value)
>
> Why TODO ?
>
> Here and everywhere else could you shorten lines length where is
> trivial to do so ?
>
> 	/* TODO: [[nodiscard]] */ SetError set(const Control<T> &ctrl,
>                                                const details::cxx20::type_identity_t<T> &value)
>
>
> What does type_identity_t<> gives us here ? Isn't T fully specified ?
>
> > +	{
> > +		using TypeInfo = libcamera::details::control_type<T>;
> > +
> > +		if constexpr (TypeInfo::size > 0) {
> > +			static_assert(std::is_trivially_copyable_v<typename T::value_type>);
> > +
> > +			return set(ctrl.id(), {
> > +				TypeInfo::value,
> > +				true,
> > +				value.size(),
> > +				reinterpret_cast<const std::byte *>(value.data()),
> > +			});
> > +		} else {
> > +			static_assert(std::is_trivially_copyable_v<T>);
> > +
> > +			return set(ctrl.id(), {
> > +				TypeInfo::value,
> > +				false,
> > +				1,
> > +				reinterpret_cast<const std::byte *>(&value),
> > +			});
> > +		}
>
> The private set() overload works with Views to avoid copies here, I
> get it, but as said before should the public set(tag, value) accept a
> ControlValue & or do we want callers to go through a View ?
>
> > +	}
> > +
> > +	template<typename T>
> > +	[[nodiscard]] decltype(auto) get(const Control<T> &ctrl) const
>
> isn't this simply an
> 	[[nodiscard]] std::optional<T> get(const Control<T> &ctrl) const
>
> why use decltype(auto) ?
>
> > +	{
> > +		ControlValueView v = get(ctrl.id());
> > +
> > +		return v ? std::optional(v.get<T>()) : std::nullopt;
> > +	}
> > +
> > +	// TODO: operator ControlListView() const ?
> > +	// TODO: explicit operator ControlList() const ?
>
> Do these still apply ?
>
> > +
> > +	[[nodiscard]] ControlValueView get(std::uint32_t tag) const
> > +	{
> > +		const auto *e = find(tag);
> > +		if (!e)
> > +			return {};
> > +
> > +		return data_of(*e);
>
> no snake_case but use camelCase please
>
> > +	}
> > +
> > +	void clear()
> > +	{
> > +		for (auto &e : entries())
> > +			e.headerOffset.store(Entry::invalidOffset, std::memory_order_relaxed);
> > +
> > +		[[maybe_unused]] auto s = state_.exchange({}, std::memory_order_relaxed);
> > +
> > +#if HAS_ASAN
> > +		::__sanitizer_annotate_contiguous_container(
> > +			p_ + contentOffset_, p_ + alloc_,
> > +			p_ + contentOffset_ + s.fill, p_ + contentOffset_
> > +		);
> > +#endif
> > +	}
> > +
> > +	class iterator
> > +	{
> > +	public:
> > +		using difference_type = std::ptrdiff_t;
> > +		using value_type = std::pair<std::uint32_t, ControlValueView>;
> > +		using pointer = void;
> > +		using reference = value_type;
>
> Why can't you use 'value_type' and need another indirection symbol ?
>
> Are those symbols required when defining a custom iterator ?
>
> Also shouldn't this be "value_type &" ?
>
> > +		using iterator_category = std::forward_iterator_tag;
> > +
> > +		iterator() = default;
> > +
> > +		iterator& operator++()
> > +		{
> > +			const auto &h = header();
> > +
> > +			p_ += sizeof(h);
> > +			p_ = details::align::up(p_, h.alignment);
> > +			p_ += h.size;
> > +			p_ = details::align::up(p_, alignof(decltype(h)));
> > +
> > +			return *this;
> > +		}
> > +
> > +		iterator operator++(int)
> > +		{
> > +			auto copy = *this;
> > +			++*this;
> > +			return copy;
> > +		}
> > +
> > +		[[nodiscard]] reference operator*() const
> > +		{
> > +			const auto &h = header();
> > +			const auto *data = details::align::up(p_ + sizeof(h), h.alignment);
> > +
> > +			return { h.tag, { h.params.type, h.params.isArray, h.params.numElements, data } };
> > +		}
> > +
> > +		[[nodiscard]] bool operator==(const iterator &other) const
> > +		{
> > +			return p_ == other.p_;
> > +		}
> > +
> > +		[[nodiscard]] bool operator!=(const iterator &other) const
> > +		{
> > +			return !(*this == other);
> > +		}
> > +
> > +	private:
> > +		iterator(const std::byte *p)
> > +			: p_(p)
> > +		{
> > +		}
> > +
> > +		[[nodiscard]] const ValueHeader &header() const
> > +		{
> > +			return *reinterpret_cast<const ValueHeader *>(p_);
> > +		}
> > +
> > +		friend MetadataList;
> > +
> > +		const std::byte *p_ = nullptr;
> > +	};
> > +
> > +	[[nodiscard]] iterator begin() const
> > +	{
> > +		return { p_ + contentOffset_ };
> > +	}
> > +
> > +	[[nodiscard]] iterator end() const
> > +	{
> > +		return { p_ + contentOffset_ + state_.load(std::memory_order_acquire).fill };
> > +	}
> > +
> > +	class Diff
> > +	{
> > +	public:
> > +		// TODO: want these?
>
> Why not you think ?
>
> > +		[[nodiscard]] explicit operator bool() const { return !empty(); }
> > +		[[nodiscard]] bool empty() const { return start_ == stop_; }
> > +		[[nodiscard]] std::size_t size() const { return changed_; }
> > +		[[nodiscard]] const MetadataList &list() const { return *l_; }
> > +
> > +		[[nodiscard]] ControlValueView get(std::uint32_t tag) const
> > +		{
> > +			const auto *e = l_->find(tag);
> > +			if (!e)
> > +				return {};
> > +
> > +			auto o = e->acquireData();
> > +			if (!o)
> > +				return {};
> > +
> > +			if (!(start_ <= *o && *o < stop_))
> > +				return {};
> > +
> > +			return l_->data_of(*o);
> > +		}
> > +
> > +		template<typename T>
> > +		[[nodiscard]] decltype(auto) get(const Control<T> &ctrl) const
> > +		{
> > +			ControlValueView v = get(ctrl.id());
> > +
> > +			return v ? std::optional(v.get<T>()) : std::nullopt;
> > +		}
> > +
> > +		[[nodiscard]] iterator begin() const
> > +		{
> > +			return { l_->p_ + start_ };
> > +		}
> > +
> > +		[[nodiscard]] iterator end() const
> > +		{
> > +			return { l_->p_ + stop_ };
> > +		}
> > +
> > +	private:
> > +		Diff(const MetadataList &l, std::size_t changed, std::size_t oldFill, std::size_t newFill)
> > +			: l_(&l),
> > +			  changed_(changed),
> > +			  start_(l.contentOffset_ + oldFill),
> > +			  stop_(l.contentOffset_ + newFill)
> > +		{
> > +		}
> > +
> > +		friend MetadataList;
> > +		friend struct Checkpoint;
> > +
> > +		const MetadataList *l_ = nullptr;
> > +		std::size_t changed_;
> > +		std::size_t start_;
> > +		std::size_t stop_;
> > +	};
> > +
> > +	Diff merge(const ControlList &other)
>
> So the "merge" function is how pipeline handlers adds metadata to a
> MetadataList ?
>
> > +	{
> > +		// TODO: check id map of `other`?
>
> This already is a proper /* \todo */
>
> > +
> > +		const auto c = checkpoint();
> > +
> > +		for (const auto &[tag, value] : other) {
> > +			auto *e = find(tag);
> > +			if (e) {
> > +				[[maybe_unused]] auto r = set(*e, value);
> > +				assert(r == SetError() || r == SetError::AlreadySet); // TODO: ?
>
> does "r == SetError()" means r == 0 ? What's the TODO for ?
>
> > +			}
> > +		}
> > +
> > +		return c.diffSince();
> > +	}
> > +
> > +	class Checkpoint
> > +	{
> > +	public:
> > +		[[nodiscard]] Diff diffSince() const
> > +		{
> > +			/* sync with release-store on `state_` in `set()` */
> > +			const auto curr = l_->state_.load(std::memory_order_acquire);
> > +
> > +			assert(s_.count <= curr.count);
> > +			assert(s_.fill <= curr.fill);
> > +
> > +			return {
> > +				*l_,
> > +				curr.count - s_.count,
> > +				s_.fill,
> > +				curr.fill,
> > +			};
> > +		}
> > +
> > +	private:
> > +		Checkpoint(const MetadataList &l)
> > +			: l_(&l),
> > +			  s_(l.state_.load(std::memory_order_relaxed))
> > +		{
> > +		}
> > +
> > +		friend MetadataList;
> > +
> > +		const MetadataList *l_ = nullptr;
>
> Is there in your opinion a case where a Checkpoint can overlive the
> MetadataList it refers to ?
>
> > +		State s_ = {};
> > +	};
> > +
> > +	[[nodiscard]] Checkpoint checkpoint() const
> > +	{
> > +		return { *this };
> > +	}
> > +
> > +private:
> > +	[[nodiscard]] static constexpr std::size_t entriesOffset()
> > +	{
> > +		return 0;
> > +	}
> > +
> > +	[[nodiscard]] static constexpr std::size_t contentOffset(std::size_t entries)
> > +	{
> > +		return details::align::up(entriesOffset() + entries * sizeof(Entry), alignof(ValueHeader));
> > +	}
> > +
> > +	[[nodiscard]] Span<Entry> entries() const
> > +	{
> > +		return { reinterpret_cast<Entry *>(p_ + entriesOffset()), capacity_ };
> > +	}
> > +
> > +	[[nodiscard]] Entry *find(std::uint32_t tag) const
> > +	{
> > +		const auto entries = this->entries();
> > +		auto it = std::partition_point(entries.begin(), entries.end(), [&](const auto &e) {
>
> how is this different from find_if() ? Just out of curiosity
>
> I'll stop here for the time being. Just one more note, you moved
> MetadataListPlan out this header, so you probably need a dedicated
> .cpp file as well
>
>
> > +			return e.tag < tag;
> > +		});
> > +
> > +		if (it == entries.end() || it->tag != tag)
> > +			return nullptr;
> > +
> > +		return &*it;
> > +	}
> > +
> > +	[[nodiscard]] ControlValueView data_of(const Entry &e) const
> > +	{
> > +		const auto o = e.acquireData();
> > +		return o ? data_of(*o) : ControlValueView{ };
> > +	}
> > +
> > +	[[nodiscard]] ControlValueView data_of(std::size_t headerOffset) const
> > +	{
> > +		assert(headerOffset <= alloc_ - sizeof(ValueHeader));
> > +		assert(details::align::is(p_ + headerOffset, alignof(ValueHeader)));
> > +
> > +		const auto *vh = reinterpret_cast<const ValueHeader *>(p_ + headerOffset);
> > +		const auto *p = reinterpret_cast<const std::byte *>(vh) + sizeof(*vh);
> > +		std::size_t avail = p_ + alloc_ - p;
> > +
> > +		const auto *data = details::align::up(vh->size, vh->alignment, p, &avail);
> > +		assert(data);
> > +
> > +		return { vh->params.type, vh->params.isArray, vh->params.numElements, data };
> > +	}
> > +
> > +	[[nodiscard]] SetError set(Entry &e, ControlValueView v)
> > +	{
> > +		if (e.hasValue())
> > +			return SetError::AlreadySet;
> > +		if (e.type != v.type() || e.isArray != v.isArray())
> > +			return SetError::TypeMismatch;

Isn't this better expressed as an assertion ?

> > +
> > +		const auto src = v.data();
> > +		if (e.isArray) {
> > +			if (src.size_bytes() > e.capacity)
> > +				return SetError::SizeMismatch;
> > +		} else {
> > +			if (src.size_bytes() != e.capacity)
> > +				return SetError::SizeMismatch;
> > +		}

If any of these fails, it means the pipeline handler has wrongly sized
the MetadataPlan, should we assert here as well ?

> > +
> > +		auto s = state_.load(std::memory_order_relaxed);
> > +		std::byte *oldEnd = p_ + contentOffset_ + s.fill;
> > +		std::byte *p = oldEnd;
> > +
> > +		auto *headerPtr = details::align::up<ValueHeader>(p);

This aligns to the next ValueHeader

> > +		auto *dataPtr = details::align::up(src.size_bytes(), e.alignment, p);

This to size_bytes() + alignment

> > +		details::align::up(0, alignof(ValueHeader), p);

What does this do ?

> > +
> > +#if HAS_ASAN
> > +		::__sanitizer_annotate_contiguous_container(
> > +			p_ + contentOffset_, p_ + alloc_,
> > +			oldEnd, p
> > +		);
> > +#endif
> > +
> > +		new (headerPtr) ValueHeader{
> > +			.tag = e.tag,
> > +			.size = std::uint32_t(src.size_bytes()),
> > +			.alignment = e.alignment,
> > +			.params = {
> > +				.type = v.type(),
> > +				.isArray = v.isArray(),
> > +				.numElements = std::uint32_t(v.numElements()),
> > +			},
> > +		};
> > +		std::memcpy(dataPtr, src.data(), src.size_bytes());
> > +		e.headerOffset.store(reinterpret_cast<std::byte *>(headerPtr) - p_, std::memory_order_release);

break these long lines when it's trivial to do so

> > +
> > +		s.fill += p - oldEnd;
> > +		s.count += 1;
> > +
> > +		state_.store(s, std::memory_order_release);
> > +
> > +		return {};
> > +	}
> > +
> > +	std::size_t capacity_ = 0;
> > +	std::size_t contentOffset_ = -1;
> > +	std::size_t alloc_ = 0;
> > +	std::atomic<State> state_ = State{};
> > +	std::byte *p_ = nullptr;
> > +	// TODO: ControlIdMap in any way shape or form?

What do you mean here ?

> > +
> > +	/*
> > +	 * If this is problematic on a 32-bit architecture, then
> > +	 * `count` can be stored in a separate atomic variable
> > +	 * but then `Diff::changed_` must be removed since the fill
> > +	 * level and item count cannot be retrieved atomically.

or we can use a mutex or a different lock primitive ?

> > +	 */
> > +	static_assert(decltype(state_)::is_always_lock_free);
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#undef HAS_ASAN
> > diff --git a/include/libcamera/metadata_list_plan.h b/include/libcamera/metadata_list_plan.h
> > new file mode 100644
> > index 000000000..2ed35c54f
> > --- /dev/null
> > +++ b/include/libcamera/metadata_list_plan.h
> > @@ -0,0 +1,130 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2025, Ideas On Board Oy
> > + */
> > +
> > +#pragma once
> > +
> > +#include <cassert>
> > +#include <cstddef>
> > +#include <cstdint>
> > +#include <limits>
> > +#include <map>
> > +#include <type_traits>
> > +
> > +#include <libcamera/base/details/cxx20.h>
> > +
> > +#include <libcamera/controls.h>
> > +
> > +namespace libcamera {
> > +
> > +class MetadataListPlan
> > +{
> > +public:
> > +	struct Entry {
> > +		std::uint32_t size;
> > +		std::uint32_t alignment; // TODO: is this necessary?

I wonder if the alignment shouldn't come from controls.h associated
with ControlType

Something like control_type<T>::alignment

I wonder if this would simplify the handling of alignment in
MetadataList as well

> > +		std::uint32_t numElements;
> > +		ControlType type;
> > +		bool isArray;
> > +	};
> > +
> > +	[[nodiscard]] bool empty() const { return items_.empty(); }
> > +	[[nodiscard]] std::size_t size() const { return items_.size(); }
> > +	[[nodiscard]] decltype(auto) begin() const { return items_.begin(); }
> > +	[[nodiscard]] decltype(auto) end() const { return items_.end(); }
> > +	void clear() { items_.clear(); }
> > +
> > +	template<
> > +		typename T,
> > +		std::enable_if_t<libcamera::details::control_type<T>::size != libcamera::dynamic_extent> * = nullptr
> > +	>
> > +	decltype(auto) set(const Control<T> &ctrl)

This calls the private overload

	template<typename T>
	decltype(auto) set(std::uint32_t tag, std::size_t numElements, bool isArray)

Which returns

        *this;

which if I'm not mistaken means "MetadataListPlan *". Can this function
returns anything else ? If the return type is known and trivial to
type out why hide it behind decltype(auto) ?

> > +	{
> > +		if constexpr (libcamera::details::control_type<T>::size > 0) {
> > +			static_assert(libcamera::details::control_type<T>::size != libcamera::dynamic_extent);
> > +
> > +			return set<typename T::value_type>(
> > +				ctrl.id(),
> > +				libcamera::details::control_type<T>::size,
> > +				true
> > +			);
> > +		} else {
> > +			return set<T>(ctrl.id(), 1, false);
> > +		}
> > +	}
> > +
> > +	template<
> > +		typename T,
> > +		std::enable_if_t<libcamera::details::control_type<T>::size == libcamera::dynamic_extent> * = nullptr

is this for strings only ? It seems to me that only std::string has
control_type::size == dynamic_extent

> > +	>
> > +	decltype(auto) set(const Control<T> &ctrl, std::size_t numElements)
> > +	{
> > +		return set<typename T::value_type>(ctrl.id(), numElements, true);
> > +	}
> > +
> > +	[[nodiscard]] bool set(std::uint32_t tag,
> > +			       std::size_t size, std::size_t alignment,
> > +			       std::size_t numElements, ControlType type, bool isArray)

Is this overload meant for the public interface of the class ?

It seems that the two previous overloads call the private one

	template<typename T>
	decltype(auto) set(std::uint32_t tag, std::size_t numElements, bool isArray)

which then calls this public one. Is this intentional ?

> > +	{
> > +		if (size == 0 || size > std::numeric_limits<std::uint32_t>::max())
> > +			return false;
> > +		if (alignment > std::numeric_limits<std::uint32_t>::max())
> > +			return false;
> > +		if (!details::cxx20::has_single_bit(alignment))
> > +			return false;
> > +		if (numElements > std::numeric_limits<std::uint32_t>::max() / size)
> > +			return false;
> > +		if (!isArray && numElements != 1)
> > +			return false;

Should these be assertions (where and if possible) ?

> > +
> > +		items_.insert_or_assign(tag, Entry{

Do we allow to re-write an entry in the MetadataListPlan ?

> > +			.size = std::uint32_t(size),
> > +			.alignment = std::uint32_t(alignment),
> > +			.numElements = std::uint32_t(numElements),
> > +			.type = type,
> > +			.isArray = isArray,
> > +		});
> > +
> > +		return true;
> > +	}
> > +
> > +	[[nodiscard]] const Entry *get(std::uint32_t tag) const
> > +	{
> > +		auto it = items_.find(tag);
> > +		if (it == items_.end())
> > +			return nullptr;
> > +
> > +		return &it->second;
> > +	}
> > +
> > +	[[nodiscard]] const Entry *get(const ControlId &cid) const
> > +	{
> > +		const auto *e = get(cid.id());
> > +		if (!e)
> > +			return nullptr;
> > +
> > +		if (e->type != cid.type() || e->isArray != cid.isArray())
> > +			return nullptr;

How can this happen ?

> > +
> > +		return e;
> > +	}
> > +
> > +private:
> > +	std::map<std::uint32_t, Entry> items_;
> > +
> > +	template<typename T>
> > +	decltype(auto) set(std::uint32_t tag, std::size_t numElements, bool isArray)
> > +	{
> > +		static_assert(std::is_trivially_copyable_v<T>);
> > +
> > +		[[maybe_unused]] bool ok = set(tag,
> > +					       sizeof(T), alignof(T),
> > +					       numElements, details::control_type<T>::value, isArray);
> > +		assert(ok);
> > +
> > +		return *this;
> > +	}
> > +};
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index de1eb99b2..8c5ce4503 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -9,6 +9,7 @@ libcamera_public_sources = files([
> >      'framebuffer.cpp',
> >      'framebuffer_allocator.cpp',
> >      'geometry.cpp',
> > +    'metadata_list.cpp',
> >      'orientation.cpp',
> >      'pixel_format.cpp',
> >      'request.cpp',
> > diff --git a/src/libcamera/metadata_list.cpp b/src/libcamera/metadata_list.cpp
> > new file mode 100644
> > index 000000000..ebefdfdad
> > --- /dev/null
> > +++ b/src/libcamera/metadata_list.cpp
> > @@ -0,0 +1,344 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2025, Ideas On Board Oy
> > + */
> > +
> > +#include <libcamera/metadata_list.h>
> > +
> > +namespace libcamera {
> > +
> > +/**
> > + * \class MetadataListPlan
> > + * \brief Class to hold the possible set of metadata items for a MetadataList
> > + */
> > +
> > +/**
> > + * \class MetadataListPlan::Entry
> > + * \brief Details of a metadata item
> > + */
> > +
> > +/**
> > + * \internal
> > + * \var MetadataListPlan::Entry::size
> > + * \brief Number of bytes in a single element
> > + *
> > + * \var MetadataListPlan::Entry::alignment
> > + * \brief Required alignment of the elements
> > + * \endinternal
> > + *
> > + * \var MetadataListPlan::Entry::numElements
> > + * \brief Number of elements in the value
> > + * \sa ControlValueView::numElements()
> > + *
> > + * \var MetadataListPlan::Entry::type
> > + * \brief The type of the value
> > + * \sa ControlValueView::type()
> > + *
> > + * \var MetadataListPlan::Entry::isArray
> > + * \brief Whether or not the value is array-like
> > + * \sa ControlValueView::isArray()
> > + */
> > +
> > +/**
> > + * \fn MetadataListPlan::begin() const
> > + * \brief Retrieve the begin iterator
> > + */
> > +
> > +/**
> > + * \fn MetadataListPlan::end() const
> > + * \brief Retrieve the end iterator
> > + */
> > +
> > +/**
> > + * \fn MetadataListPlan::size() const
> > + * \brief Retrieve the number of entries
> > + */
> > +
> > +/**
> > + * \fn MetadataListPlan::empty() const
> > + * \brief Check if empty
> > + */
> > +
> > +/**
> > + * \internal
> > + * \fn MetadataListPlan::clear()
> > + * \brief Remove all controls
> > + */
> > +
> > +/**
> > + * \internal
> > + * \fn MetadataListPlan::set(const Control<T> &ctrl)
> > + * \brief Add an entry for the given control to the metadata list plan
> > + * \param[in] ctrl The control
> > + */
> > +
> > +/**
> > + * \internal
> > + * \fn MetadataListPlan::set(const Control<T> &ctrl, std::size_t count)
> > + * \brief Add an entry for the given dynamically-sized control to the metadata list plan
> > + * \param[in] ctrl The control
> > + * \param[in] count The maximum number of elements
> > + *
> > + * Add the dynamically-sized control \a ctrl to the metadata list plan with a maximum
> > + * capacity of \a count elements.
> > + */
> > +
> > +/**
> > + * \internal
> > + * \fn MetadataListPlan::set(std::uint32_t tag,
> > + *			     std::size_t size, std::size_t alignment,
> > + *			     std::size_t count, ControlType type, bool isArray)
> > + * \brief Add an entry to the metadata list plan
> > + * \return \a true if the entry has been added, or \a false if the given parameters
> > + *         would result in an invalid entry
> > + *
> > + * This functions adds an entry with essentially arbitrary parameters, without deriving
> > + * them from a given ControlId instance. This is mainly used when deserializing.
> > + */
> > +
> > +/**
> > + * \fn MetadataListPlan::get(std::uint32_t tag) const
> > + * \brief Find the \ref Entry "entry" with the given identifier
> > + */
> > +
> > +/**
> > + * \fn MetadataListPlan::get(const ControlId &cid) const
> > + * \brief Find the \ref Entry "entry" for the given ControlId
> > + *
> > + * The \ref Entry "entry" is only returned if ControlId::type() and ControlId::isArray()
> > + * of \a cid matches Entry::type and Entry::isArray, respectively.
> > + */
> > +
> > +/**
> > + * \class MetadataList
> > + * \brief Class to hold metadata items
> > + */
> > +
> > +/**
> > + * \fn MetadataList::MetadataList(const MetadataListPlan &plan)
> > + * \brief Construct a metadata list according to \a plan
> > + *
> > + * Construct a metadata list according to the provided \a plan.
> > + */
> > +
> > +/**
> > + * \fn MetadataList::size() const
> > + * \brief Retrieve the number of controls
> > + * \context This function is \threadsafe.
> > + * \note If the list is being modified, the return value may be out of
> > + *       date by the time the function returns
> > + */
> > +
> > +/**
> > + * \fn MetadataList::empty() const
> > + * \brief Check if empty
> > + * \context This function is \threadsafe.
> > + * \note If the list is being modified, the return value may be out of
> > + *       date by the time the function returns
> > + */
> > +
> > +/**
> > + * \internal
> > + * \fn MetadataList::clear()
> > + * \brief Remove all items from the list
> > + * \note This function in effect resets the list to its original state. As a consequence it invalidates - among others -
> > + *       all iterators, Checkpoint, and Diff objects that are associated with the list. No readers must exist
> > + *       when this function is called.
> > + */
> > +
> > +/**
> > + * \fn MetadataList::begin() const
> > + * \brief Retrieve begin iterator
> > + * \context This function is \threadsafe.
> > + */
> > +
> > +/**
> > + * \fn MetadataList::end() const
> > + * \brief Retrieve end iterator
> > + * \context This function is \threadsafe.
> > + */
> > +
> > +/**
> > + * \fn MetadataList::get(const Control<T> &ctrl) const
> > + * \brief Get the value of control \a ctrl
> > + * \return A std::optional<T> containing the control value, or std::nullopt if
> > + *         the control \a ctrl is not present in the list
> > + * \context This function is \threadsafe.
> > + */
> > +
> > +/**
> > + * \fn MetadataList::get(std::uint32_t tag) const
> > + * \brief Get the value of pertaining to the numeric identifier \a tag
> > + * \return A std::optional<T> containing the control value, or std::nullopt if
> > + *         the control is not present in the list
> > + * \context This function is \threadsafe.
> > + */
> > +
> > +/**
> > + * \internal
> > + * \fn MetadataList::set(const Control<T> &ctrl, const details::cxx20::type_identity_t<T> &value)
> > + * \brief Set the value of control \a ctrl to \a value
> > + */
> > +
> > +/**
> > + * \internal
> > + * \fn MetadataList::set(std::uint32_t tag, ControlValueView v)
> > + * \brief Set the value of pertaining to the numeric identifier \a tag to \a v
> > + */
> > +
> > +/**
> > + * \internal
> > + * \fn MetadataList::merge(const ControlList &other)
> > + * \brief Add all missing items from \a other
> > + *
> > + * Add all items from \a other that are not present in \a this.
> > + */
> > +
> > +/**
> > + * \internal
> > + * \enum MetadataList::SetError
> > + * \brief Error code returned by a set operation
> > + *
> > + * \var MetadataList::SetError::UnknownTag
> > + * \brief The tag is not supported by the metadata list
> > + * \var MetadataList::SetError::AlreadySet
> > + * \brief A value has already been added with the given tag
> > + * \var MetadataList::SetError::SizeMismatch
> > + * \brief The size of the data is not appropriate for the given tag
> > + * \var MetadataList::SetError::TypeMismatch
> > + * \brief The type of the value does not match the expected type
> > + */
> > +
> > +/**
> > + * \internal
> > + * \fn MetadataList::checkpoint() const
> > + * \brief Create a checkpoint
> > + * \context This function is \threadsafe.
> > + */
> > +
> > +/**
> > + * \class MetadataList::iterator
> > + * \brief Iterator
> > + */
> > +
> > +/**
> > + * \typedef MetadataList::iterator::difference_type
> > + * \brief iterator's difference type
> > + */
> > +
> > +/**
> > + * \typedef MetadataList::iterator::value_type
> > + * \brief iterator's value type
> > + */
> > +
> > +/**
> > + * \typedef MetadataList::iterator::pointer
> > + * \brief iterator's pointer type
> > + */
> > +
> > +/**
> > + * \typedef MetadataList::iterator::reference
> > + * \brief iterator's reference type
> > + */
> > +
> > +/**
> > + * \typedef MetadataList::iterator::iterator_category
> > + * \brief iterator's category
> > + */
> > +
> > +/**
> > + * \fn MetadataList::iterator::operator*()
> > + * \brief Retrieve value at iterator
> > + * \return A \a ControlListView representing the value
> > + */
> > +
> > +/**
> > + * \fn MetadataList::iterator::operator==(const iterator &other) const
> > + * \brief Check if two iterators are equal
> > + */
> > +
> > +/**
> > + * \fn MetadataList::iterator::operator!=(const iterator &other) const
> > + * \brief Check if two iterators are not equal
> > + */
> > +
> > +/**
> > + * \fn MetadataList::iterator::operator++(int)
> > + * \brief Advance the iterator
> > + */
> > +
> > +/**
> > + * \fn MetadataList::iterator::operator++()
> > + * \brief Advance the iterator
> > + */
> > +
> > +/**
> > + * \class MetadataList::Diff
> > + * \brief Designates a set of consecutively added metadata items from a particular MetadataList
> > + * \sa Camera::metadataAvailable
> > + * \internal
> > + * \sa MetadataList::Checkpoint::diffSince()
> > + * \endinternal
> > + */
> > +
> > +/**
> > + * \fn MetadataList::Diff::list() const
> > + * \brief Retrieve the associated MetadataList
> > + */
> > +
> > +/**
> > + * \fn MetadataList::Diff::size() const
> > + * \brief Retrieve the number of metadata items designated
> > + */
> > +
> > +/**
> > + * \fn MetadataList::Diff::empty() const
> > + * \brief Check if any metadata items are designated
> > + */
> > +
> > +/**
> > + * \fn MetadataList::Diff::operator bool() const
> > + * \copydoc MetadataList::Diff::empty() const
> > + */
> > +
> > +/**
> > + * \fn MetadataList::Diff::get(const Control<T> &ctrl) const
> > + * \copydoc MetadataList::get(const Control<T> &ctrl) const
> > + * \note The lookup will fail if the metadata item is not designated by this Diff object,
> > + *       even if it is otherwise present in the backing MetadataList.
> > + */
> > +
> > +/**
> > + * \fn MetadataList::Diff::get(std::uint32_t tag) const
> > + * \copydoc MetadataList::get(std::uint32_t tag) const
> > + * \note The lookup will fail if the metadata item is not designated by this Diff object,
> > + *       even if it is otherwise present in the backing MetadataList.
> > + */
> > +
> > +/**
> > + * \fn MetadataList::Diff::begin() const
> > + * \brief Retrieve the begin iterator
> > + */
> > +
> > +/**
> > + * \fn MetadataList::Diff::end() const
> > + * \brief Retrieve the end iterator
> > + */
> > +
> > +/**
> > + * \internal
> > + * \class MetadataList::Checkpoint
> > + * \brief Designates a point in the stream of metadata items
> > + *
> > + * A Checkpoint object designates a point in the stream of metadata items in the associated
> > + * MetadataList. Its main use to be able to retrieve the set of metadata items that were
> > + * added to the list after the designated point using diffSince().
> > + */
> > +
> > +/**
> > + * \internal
> > + * \fn MetadataList::Checkpoint::diffSince() const
> > + * \brief Retrieve the set of metadata items added since the checkpoint was created
> > + */
> > +
> > +} /* namespace libcamera */
> > diff --git a/test/controls/meson.build b/test/controls/meson.build
> > index 763f8905e..b68a4fc53 100644
> > --- a/test/controls/meson.build
> > +++ b/test/controls/meson.build
> > @@ -5,6 +5,7 @@ control_tests = [
> >      {'name': 'control_info_map', 'sources': ['control_info_map.cpp']},
> >      {'name': 'control_list', 'sources': ['control_list.cpp']},
> >      {'name': 'control_value', 'sources': ['control_value.cpp']},
> > +    {'name': 'metadata_list', 'sources': ['metadata_list.cpp']},
> >  ]
> >
> >  foreach test : control_tests
> > diff --git a/test/controls/metadata_list.cpp b/test/controls/metadata_list.cpp
> > new file mode 100644
> > index 000000000..f0872acd9
> > --- /dev/null
> > +++ b/test/controls/metadata_list.cpp
> > @@ -0,0 +1,170 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2025, Ideas On Board Oy
> > + *
> > + * MetadataList tests
> > + */
> > +
> > +#include <future>
> > +#include <iostream>
> > +#include <thread>
> > +
> > +#include <libcamera/control_ids.h>
> > +#include <libcamera/metadata_list.h>
> > +#include <libcamera/property_ids.h>
> > +
> > +#include "test.h"
> > +
> > +using namespace std;
> > +using namespace libcamera;
> > +
> > +#define ASSERT(x) do { \
> > +	if (!static_cast<bool>(x)) { \
> > +		std::cerr << '`' << #x << "` failed" << std::endl; \
> > +		return TestFail; \
> > +	} \
> > +} while (false)
> > +
> > +class MetadataListTest : public Test
> > +{
> > +public:
> > +	MetadataListTest() = default;
> > +
> > +protected:
> > +	int run() override
> > +	{
> > +		MetadataListPlan mlp;
> > +		mlp.set(controls::ExposureTime);
> > +		mlp.set(controls::ExposureValue);
> > +		mlp.set(controls::ColourGains);
> > +		mlp.set(controls::AfWindows, 10);
> > +		mlp.set(controls::AeEnable);
> > +		mlp.set(controls::SensorTimestamp);
> > +
> > +		MetadataList ml(mlp);
> > +
> > +		static_assert(static_cast<unsigned int>(properties::LOCATION) == controls::AE_ENABLE);
> > +		ASSERT(ml.set(properties::Location, properties::CameraLocationFront) == MetadataList::SetError::TypeMismatch);

Can you add a comment to explain what this tests ?

> > +
> > +		ASSERT(ml.set(controls::AfWindows, std::array<Rectangle, 11>{}) == MetadataList::SetError::SizeMismatch);
> > +		ASSERT(ml.set(controls::ColourTemperature, 123) == MetadataList::SetError::UnknownTag);
> > +
> > +		auto f1 = std::async(std::launch::async, [&] {
> > +			using namespace std::chrono_literals;
> > +
> > +			std::this_thread::sleep_for(500ms);
> > +			ASSERT(ml.set(controls::ExposureTime, 0x1111) == MetadataList::SetError());
> > +
> > +			std::this_thread::sleep_for(500ms);
> > +			ASSERT(ml.set(controls::ExposureValue, 1) == MetadataList::SetError());
> > +
> > +			std::this_thread::sleep_for(500ms);
> > +			ASSERT(ml.set(controls::ColourGains, std::array{
> > +				123.f,
> > +				456.f
> > +			}) == MetadataList::SetError());
> > +
> > +			std::this_thread::sleep_for(500ms);
> > +			ASSERT(ml.set(controls::AfWindows, std::array{
> > +				Rectangle(),
> > +				Rectangle(1, 2, 3, 4),
> > +				Rectangle(0x1111, 0x2222, 0x3333, 0x4444),
> > +			}) == MetadataList::SetError());

I don't get why you expect these to fail..

> > +
> > +			return TestPass;
> > +		});
> > +
> > +		auto f2 = std::async(std::launch::async, [&] {
> > +			for (;;) {
> > +				const auto x = ml.get(controls::ExposureTime);
> > +				const auto y = ml.get(controls::ExposureValue);
> > +				const auto z = ml.get(controls::ColourGains);
> > +				const auto w = ml.get(controls::AfWindows);
> > +
> > +				if (x)
> > +					ASSERT(*x == 0x1111);
> > +
> > +				if (y)
> > +					ASSERT(*y == 1.0f);
> > +
> > +				if (z) {
> > +					ASSERT(z->size() == 2);
> > +					ASSERT((*z)[0] == 123.f);
> > +					ASSERT((*z)[1] == 456.f);
> > +				}
> > +
> > +				if (w) {
> > +					ASSERT(w->size() == 3);
> > +					ASSERT((*w)[0].isNull());
> > +					ASSERT((*w)[1] == Rectangle(1, 2, 3, 4));
> > +					ASSERT((*w)[2] == Rectangle(0x1111, 0x2222, 0x3333, 0x4444));
> > +				}
> > +
> > +				if (x && y && z && w)
> > +					break;
> > +			}
> > +
> > +			return TestPass;

and if f1 is expected to fail how can f2 pass.

I'm surely missing something

> > +		});
> > +
> > +		ASSERT(f1.get() == TestPass);
> > +		ASSERT(f2.get() == TestPass);
> > +
> > +		ASSERT(ml.set(controls::ExposureTime, 0x2222) == MetadataList::SetError::AlreadySet);
> > +		ASSERT(ml.set(controls::ExposureValue, 2) == MetadataList::SetError::AlreadySet);
> > +
> > +		ASSERT(ml.get(controls::ExposureTime) == 0x1111);
> > +		ASSERT(ml.get(controls::ExposureValue) == 1);
> > +
> > +		for (auto &&[tag, v] : ml)
> > +			std::cout << "[" << tag << "] -> " << v << '\n';

Are we sure we want this in tests ? I mean, it doesn't hurt..

> > +
> > +		std::cout << std::endl;
> > +
> > +		ml.clear();
> > +		ASSERT(ml.empty());
> > +		ASSERT(ml.size() == 0);
> > +
> > +		ASSERT(ml.set(controls::ExposureTime, 0x2222) == MetadataList::SetError());
> > +		ASSERT(ml.get(controls::ExposureTime) == 0x2222);
> > +
> > +		auto c = ml.checkpoint();
> > +
> > +		ASSERT(ml.set(controls::ExposureValue, 2) == MetadataList::SetError());
> > +		ASSERT(ml.set(controls::SensorTimestamp, 0x99999999) == MetadataList::SetError());
> > +
> > +		auto d = c.diffSince();
> > +		ASSERT(&d.list() == &ml);
> > +
> > +		ASSERT(ml.set(controls::ColourGains, std::array{ 1.f, 2.f }) == MetadataList::SetError());
> > +
> > +		ASSERT(d);
> > +		ASSERT(!d.empty());
> > +		ASSERT(d.size() == 2);
> > +		ASSERT(!d.get(controls::ExposureTime));
> > +		ASSERT(!d.get(controls::ColourGains));
> > +		ASSERT(!d.get(controls::AfWindows));
> > +		ASSERT(d.get(controls::ExposureValue) == 2);
> > +		ASSERT(d.get(controls::SensorTimestamp) == 0x99999999);
> > +
> > +		for (auto &&[tag, v] : d)
> > +			std::cout << "[" << tag << "] -> " << v << '\n';
> > +
> > +		/* Test if iterators work with algorithms. */
> > +		std::ignore = std::find_if(d.begin(), d.end(), [](const auto &) {
> > +			return false;
> > +		});
> > +
> > +#if 0
> > +               {
> > +                       auto it = ml.begin();
> > +                       ml.clear();
> > +                       std::ignore = *it; /* Trigger ASAN. */
> > +               }
> > +#endif

Maybe remove this ?

> > +
> > +		return TestPass;
> > +	}
> > +};
> > +
> > +TEST_REGISTER(MetadataListTest)

Thanks again for your hard work on this!

> > --
> > 2.50.1
> >

Hi

2025. 07. 28. 10:36 keltezéssel, Jacopo Mondi írta:
> Hi Barnabás
>    let me resume the review from where I stopped last time
> 
> On Fri, Jul 25, 2025 at 06:08:28PM +0200, Jacopo Mondi wrote:
>> Hi Barnabás
>>
>> On Mon, Jul 21, 2025 at 12:46:07PM +0200, Barnabás Pőcze wrote:
>>> Add a dedicated `MetadataList` type, whose purpose is to store the metadata
>>> reported by a camera for a given request. Previously, a `ControlList` was
>>> used for this purpose. The reason for introducing a separate type is to
>>> simplify the access to the returned metadata during the entire lifetime
>>> of a request.
>>>
>>> Specifically, for early metadata completion to be easily usable it should be
>>> guaranteed that any completed metadata item can be accessed and looked up
>>> at least until the associated requested is reused with `Request::reuse()`.
>>>
>>> However, when a metadata item is completed early, the pipeline handler
>>> might still work on the request in the `CameraManager`'s private thread,
>>> therefore there is an inherent synchronization issue when an application
>>> accesses early metadata.
>>>
>>> Restricting the user to only access the metadata items of a not yet completed
>>> request in the early metadata availability signal handler by ways of
>>> documenting or enforcing it at runtime could be an option, but it is not
>>> too convenient for the user.
>>>
>>> The current `ControlList` implementation employs an `std::unordered_map`,
>>> so pointers remain stable when the container is modified, so an application
>>> could keep accessing particular metadata items outside the signal handler,
>>> but this fact is far from obvious, and the user would still not be able
>>> to make a copy of all metadata or do lookups based on the numeric ids or
>>> the usual `libcamera::Control<>` objects, thus some type safety is lost.
>>>
>>> The above also requires that each metadata item is only completed once for
>>> a given request, but this does not appear to be serious limitation,
>>> and in fact, this restriction is enforced by `MetadataList`.
>>>
>>> The introduced `MetadataList` supports single writer - multiple reader
>>> scenarios, and it can be set, looked-up, and copied in a wait-free fashion
>>> without introducing data races or other synchronization issues. This is
>>> achieved by requiring the possible set of metadata items to be known
>>> (such set is stored in a `MetadataListPlan` object). Based on the this
>>> plan, a single contiguous allocation is made to accommodate all potential
>>> metadata items. Due to this single contiguous allocation that is not modified
>>> during the lifetime of a `MetadataList` and atomic modifications, it is
>>> possible to easily gaurantee thread-safe set, lookup, and copy; assuming
>>> there is only ever a single writer.
>>>
>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>>> ---
>>> changes in v2:
>>>    * remove multiple not strictly necessary functions
>>> ---
>>>   include/libcamera/meson.build          |   2 +
>>>   include/libcamera/metadata_list.h      | 547 +++++++++++++++++++++++++
>>>   include/libcamera/metadata_list_plan.h | 130 ++++++
>>>   src/libcamera/meson.build              |   1 +
>>>   src/libcamera/metadata_list.cpp        | 344 ++++++++++++++++
>>>   test/controls/meson.build              |   1 +
>>>   test/controls/metadata_list.cpp        | 170 ++++++++
>>>   7 files changed, 1195 insertions(+)
>>>   create mode 100644 include/libcamera/metadata_list.h
>>>   create mode 100644 include/libcamera/metadata_list_plan.h
>>>   create mode 100644 src/libcamera/metadata_list.cpp
>>>   create mode 100644 test/controls/metadata_list.cpp
>>>
>>> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
>>> index 30ea76f94..410b548dd 100644
>>> --- a/include/libcamera/meson.build
>>> +++ b/include/libcamera/meson.build
>>> @@ -12,6 +12,8 @@ libcamera_public_headers = files([
>>>       'framebuffer_allocator.h',
>>>       'geometry.h',
>>>       'logging.h',
>>> +    'metadata_list.h',
>>> +    'metadata_list_plan.h',
>>>       'orientation.h',
>>>       'pixel_format.h',
>>>       'request.h',
>>> diff --git a/include/libcamera/metadata_list.h b/include/libcamera/metadata_list.h
>>> new file mode 100644
>>> index 000000000..7fe3dbbab
>>> --- /dev/null
>>> +++ b/include/libcamera/metadata_list.h
>>> @@ -0,0 +1,547 @@
>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>> +/*
>>> + * Copyright (C) 2025, Ideas On Board Oy
>>> + *
>>> + * Metadata list
>>> + */
>>> +
>>> +#pragma once
>>> +
>>> +#include <algorithm>
>>> +#include <atomic>
>>> +#include <cassert>
>>> +#include <cstdint>
>>> +#include <cstring>
>>> +#include <new>
>>> +#include <optional>
>>> +#include <type_traits>
>>> +
>>> +#include <libcamera/base/details/align.h>
>>> +#include <libcamera/base/details/cxx20.h>
>>> +#include <libcamera/base/span.h>
>>> +
>>> +#include <libcamera/controls.h>
>>> +#include <libcamera/metadata_list_plan.h>
>>> +
>>> +// TODO: want this?
>>> +#if __has_include(<sanitizer/asan_interface.h>)
>>> +#if __SANITIZE_ADDRESS__ /* gcc */
>>> +#include <sanitizer/asan_interface.h>
>>> +#define HAS_ASAN 1
>>> +#elif defined(__has_feature)
>>> +#if __has_feature(address_sanitizer) /* clang */
>>> +#include <sanitizer/asan_interface.h>
>>> +#define HAS_ASAN 1
>>> +#endif
>>> +#endif
>>> +#endif
>>> +
>>> +namespace libcamera {
>>> +
>>> +class MetadataList
>>> +{
>>> +private:
>>> +	struct ValueParams {
>>> +		ControlType type;
>>> +		bool isArray;
>>> +		std::uint32_t numElements;
>>> +	};
>>> +
>>> +	struct Entry {
>>> +		const std::uint32_t tag;
>>> +		const std::uint32_t capacity;
>>> +		const std::uint32_t alignment;
>>> +		const ControlType type;
>>> +		bool isArray;
>>> +
>>> +		static constexpr std::uint32_t invalidOffset = -1;
>>> +		/*
>>> +		 * Offset from the beginning of the allocation, and
>>> +		 * and _not_ relative to `contentOffset_`.
>>> +		 */
>>> +		std::atomic_uint32_t headerOffset = invalidOffset;
>>> +
>>> +		[[nodiscard]] std::optional<std::uint32_t> hasValue() const
>>> +		{
>>> +			auto offset = headerOffset.load(std::memory_order_relaxed);
>>> +			if (offset == invalidOffset)
>>> +				return {};
>>> +
>>> +			return offset;
>>> +		}
>>> +
>>> +		[[nodiscard]] std::optional<std::uint32_t> acquireData() const
>>> +		{
>>> +			auto offset = hasValue();
>>> +			if (offset) {
>>> +				/* sync with release-store on `headerOffset` in `MetadataList::set()` */
>>> +				std::atomic_thread_fence(std::memory_order_acquire);
>>> +			}
>>> +
>>> +			return offset;
>>> +		}
>>> +	};
>>> +
>>> +	struct ValueHeader {
>>> +		std::uint32_t tag;
>>> +		std::uint32_t size;
>>> +		std::uint32_t alignment;
>>> +		ValueParams params;
>>> +	};
>>> +
>>> +	struct State {
>>> +		std::uint32_t count;
>>> +		std::uint32_t fill;
>>> +	};
>>> +
>>> +public:
>>> +	explicit MetadataList(const MetadataListPlan &plan)
>>> +		: capacity_(plan.size()),
>>> +		  contentOffset_(MetadataList::contentOffset(capacity_)),
>>> +		  alloc_(contentOffset_)
>>> +	{
>>> +		for (const auto &[tag, e] : plan) {
>>> +			alloc_ += sizeof(ValueHeader);
>>> +			alloc_ += e.alignment - 1; // XXX: this is the maximum
>>> +			alloc_ += e.size * e.numElements;
>>> +			alloc_ += alignof(ValueHeader) - 1; // XXX: this is the maximum
>>> +		}
>>> +
>>> +		p_ = static_cast<std::byte *>(::operator new(alloc_));
>>> +
>>> +		auto *entries = reinterpret_cast<Entry *>(p_ + entriesOffset());
>>> +		auto it = plan.begin();
>>> +
>>> +		for (std::size_t i = 0; i < capacity_; i++, ++it) {
>>> +			const auto &[tag, e] = *it;
>>> +
>>> +			new (&entries[i]) Entry{
>>> +				.tag = tag,
>>> +				.capacity = e.size * e.numElements,
>>> +				.alignment = e.alignment,
>>> +				.type = e.type,
>>> +				.isArray = e.isArray,
>>> +			};
>>
>> I was already about to rant against C++'s "do the same usual thing in
>> some different way" style and wanted to suggest
>>
>> 			entries[i].tag = tag;
>> 			entries[i].capacity = e.size * e.numElements,
>> 			entries[i].alignment = e.alignment;
>> 			entries[i].type = e.type;
>> 			entries[i].isArray = e.isArray;
>>
>> But then you loose the const-ness of entries[i]'s fields.
>>
>> Let's use "placement new" then...

Due to the way everything is in one allocation, the objects have to be
constructed manually, so even without the `const` fields, something like
this would be required. Technically the `entries` could be move into a
separate allocation with e.g. an std::vector or std::unique_ptr.


>>
>>
>>> +		}
>>> +
>>> +#if HAS_ASAN
>>> +		::__sanitizer_annotate_contiguous_container(
>>> +			p_ + contentOffset_, p_ + alloc_,
>>> +			p_ + alloc_, p_ + contentOffset_
>>> +		);
>>> +#endif
>>> +	}
>>> +
>>> +	MetadataList(const MetadataList &) = delete;
>>> +	MetadataList(MetadataList &&) = delete;
>>> +
>>> +	MetadataList &operator=(const MetadataList &) = delete;
>>> +	MetadataList &operator=(MetadataList &&) = delete;
>>> +
>>> +	~MetadataList()
>>> +	{
>>> +#if HAS_ASAN
>>> +		/*
>>> +		 * The documentation says the range apparently has to be
>>> +		 * restored to its initial state before it is deallocated.
>>> +		 */
>>> +		::__sanitizer_annotate_contiguous_container(
>>> +			p_ + contentOffset_, p_ + alloc_,
>>> +			p_ + contentOffset_ + state_.load(std::memory_order_relaxed).fill, p_ + alloc_
>>> +		);
>>> +#endif
>>> +
>>> +		::operator delete(p_, alloc_);
>>> +	}
>>> +
>>> +	// TODO: want these?
>>
>> Why not ?

This is related to the `is_always_lock_free` assertion below.
Depending on what works on the target platforms, the interface
may need adjustments. I think `empty()` could always be provided,
but `size()` may not.


>>
>>> +	[[nodiscard]] std::size_t size() const { return state_.load(std::memory_order_relaxed).count; }
>>> +	[[nodiscard]] bool empty() const { return state_.load(std::memory_order_relaxed).fill == 0; }
>>> +
>>> +	enum class SetError {
>>> +		UnknownTag = 1,
>>> +		AlreadySet,
>>> +		SizeMismatch,
>>> +		TypeMismatch,
>>> +	};
>>
>> The only user of "set" will be pipeline handler, so I wonder if coding
>> the errors as enum is really something that gives any value here or
>> it's just overdesign. It's not like pipelines will do
>>
>>          if (AlreadySet)
>>                  doSomething
>>
>> If they get an error when adding metadata it will be caught during
>> development I presume.

See `MetadataList::set(Entry &e, ControlValueView v)`.


>>
>>> +
>>> +	[[nodiscard]] SetError set(std::uint32_t tag, ControlValueView v)
>>
>> Since we end up copying the data into the MetadataList, why use a
>> ControlValueView and not a ControlValue ?

Please see below the other `set()`, I believe it's answered there.


>>
>>> +	{
>>> +		auto *e = find(tag);
>>> +		if (!e)
>>> +			return SetError::UnknownTag;
>>> +
>>> +		return set(*e, v);
>>> +	}
>>> +
>>> +	template<typename T>
>>> +	/* TODO: [[nodiscard]] */ SetError set(const Control<T> &ctrl, const details::cxx20::type_identity_t<T> &value)
>>
>> Why TODO ?
>>
>> Here and everywhere else could you shorten lines length where is
>> trivial to do so ?
>>
>> 	/* TODO: [[nodiscard]] */ SetError set(const Control<T> &ctrl,
>>                                                 const details::cxx20::type_identity_t<T> &value)
>>
>>
>> What does type_identity_t<> gives us here ? Isn't T fully specified ?

It prevents potential deduction failures and e.g. allows the use of
initializer lists.


>>
>>> +	{
>>> +		using TypeInfo = libcamera::details::control_type<T>;
>>> +
>>> +		if constexpr (TypeInfo::size > 0) {
>>> +			static_assert(std::is_trivially_copyable_v<typename T::value_type>);
>>> +
>>> +			return set(ctrl.id(), {
>>> +				TypeInfo::value,
>>> +				true,
>>> +				value.size(),
>>> +				reinterpret_cast<const std::byte *>(value.data()),
>>> +			});
>>> +		} else {
>>> +			static_assert(std::is_trivially_copyable_v<T>);
>>> +
>>> +			return set(ctrl.id(), {
>>> +				TypeInfo::value,
>>> +				false,
>>> +				1,
>>> +				reinterpret_cast<const std::byte *>(&value),
>>> +			});
>>> +		}
>>
>> The private set() overload works with Views to avoid copies here, I
>> get it, but as said before should the public set(tag, value) accept a
>> ControlValue & or do we want callers to go through a View ?

A `ControlValue` is implicitly convertible to `ControlValueView`, so
I think this works out fine.


>>
>>> +	}
>>> +
>>> +	template<typename T>
>>> +	[[nodiscard]] decltype(auto) get(const Control<T> &ctrl) const
>>
>> isn't this simply an
>> 	[[nodiscard]] std::optional<T> get(const Control<T> &ctrl) const
>>
>> why use decltype(auto) ?

It is.


>>
>>> +	{
>>> +		ControlValueView v = get(ctrl.id());
>>> +
>>> +		return v ? std::optional(v.get<T>()) : std::nullopt;
>>> +	}
>>> +
>>> +	// TODO: operator ControlListView() const ?
>>> +	// TODO: explicit operator ControlList() const ?
>>
>> Do these still apply ?

No use case has come up so far, so I think these can be removed
for now.


>>
>>> +
>>> +	[[nodiscard]] ControlValueView get(std::uint32_t tag) const
>>> +	{
>>> +		const auto *e = find(tag);
>>> +		if (!e)
>>> +			return {};
>>> +
>>> +		return data_of(*e);
>>
>> no snake_case but use camelCase please

done


>>
>>> +	}
>>> +
>>> +	void clear()
>>> +	{
>>> +		for (auto &e : entries())
>>> +			e.headerOffset.store(Entry::invalidOffset, std::memory_order_relaxed);
>>> +
>>> +		[[maybe_unused]] auto s = state_.exchange({}, std::memory_order_relaxed);
>>> +
>>> +#if HAS_ASAN
>>> +		::__sanitizer_annotate_contiguous_container(
>>> +			p_ + contentOffset_, p_ + alloc_,
>>> +			p_ + contentOffset_ + s.fill, p_ + contentOffset_
>>> +		);
>>> +#endif
>>> +	}
>>> +
>>> +	class iterator
>>> +	{
>>> +	public:
>>> +		using difference_type = std::ptrdiff_t;
>>> +		using value_type = std::pair<std::uint32_t, ControlValueView>;
>>> +		using pointer = void;
>>> +		using reference = value_type;
>>
>> Why can't you use 'value_type' and need another indirection symbol ?
>>
>> Are those symbols required when defining a custom iterator ?

Yes.


>>
>> Also shouldn't this be "value_type &" ?

This iterator cannot provide references since it creates the values on the fly.
Setting `reference = value_type` is probably not entirely correct, but it tends
to work with most (STL) algorithms.



>>
>>> +		using iterator_category = std::forward_iterator_tag;
>>> +
>>> +		iterator() = default;
>>> +
>>> +		iterator& operator++()
>>> +		{
>>> +			const auto &h = header();
>>> +
>>> +			p_ += sizeof(h);
>>> +			p_ = details::align::up(p_, h.alignment);
>>> +			p_ += h.size;
>>> +			p_ = details::align::up(p_, alignof(decltype(h)));
>>> +
>>> +			return *this;
>>> +		}
>>> +
>>> +		iterator operator++(int)
>>> +		{
>>> +			auto copy = *this;
>>> +			++*this;
>>> +			return copy;
>>> +		}
>>> +
>>> +		[[nodiscard]] reference operator*() const
>>> +		{
>>> +			const auto &h = header();
>>> +			const auto *data = details::align::up(p_ + sizeof(h), h.alignment);
>>> +
>>> +			return { h.tag, { h.params.type, h.params.isArray, h.params.numElements, data } };
>>> +		}
>>> +
>>> +		[[nodiscard]] bool operator==(const iterator &other) const
>>> +		{
>>> +			return p_ == other.p_;
>>> +		}
>>> +
>>> +		[[nodiscard]] bool operator!=(const iterator &other) const
>>> +		{
>>> +			return !(*this == other);
>>> +		}
>>> +
>>> +	private:
>>> +		iterator(const std::byte *p)
>>> +			: p_(p)
>>> +		{
>>> +		}
>>> +
>>> +		[[nodiscard]] const ValueHeader &header() const
>>> +		{
>>> +			return *reinterpret_cast<const ValueHeader *>(p_);
>>> +		}
>>> +
>>> +		friend MetadataList;
>>> +
>>> +		const std::byte *p_ = nullptr;
>>> +	};
>>> +
>>> +	[[nodiscard]] iterator begin() const
>>> +	{
>>> +		return { p_ + contentOffset_ };
>>> +	}
>>> +
>>> +	[[nodiscard]] iterator end() const
>>> +	{
>>> +		return { p_ + contentOffset_ + state_.load(std::memory_order_acquire).fill };
>>> +	}
>>> +
>>> +	class Diff
>>> +	{
>>> +	public:
>>> +		// TODO: want these?
>>
>> Why not you think ?

I am not entirely sure how useful they are, e.g. whether or not
`size()` can be provided is kind of depends on the `is_always_lock_free`
assertion below.


>>
>>> +		[[nodiscard]] explicit operator bool() const { return !empty(); }
>>> +		[[nodiscard]] bool empty() const { return start_ == stop_; }
>>> +		[[nodiscard]] std::size_t size() const { return changed_; }
>>> +		[[nodiscard]] const MetadataList &list() const { return *l_; }
>>> +
>>> +		[[nodiscard]] ControlValueView get(std::uint32_t tag) const
>>> +		{
>>> +			const auto *e = l_->find(tag);
>>> +			if (!e)
>>> +				return {};
>>> +
>>> +			auto o = e->acquireData();
>>> +			if (!o)
>>> +				return {};
>>> +
>>> +			if (!(start_ <= *o && *o < stop_))
>>> +				return {};
>>> +
>>> +			return l_->data_of(*o);
>>> +		}
>>> +
>>> +		template<typename T>
>>> +		[[nodiscard]] decltype(auto) get(const Control<T> &ctrl) const
>>> +		{
>>> +			ControlValueView v = get(ctrl.id());
>>> +
>>> +			return v ? std::optional(v.get<T>()) : std::nullopt;
>>> +		}
>>> +
>>> +		[[nodiscard]] iterator begin() const
>>> +		{
>>> +			return { l_->p_ + start_ };
>>> +		}
>>> +
>>> +		[[nodiscard]] iterator end() const
>>> +		{
>>> +			return { l_->p_ + stop_ };
>>> +		}
>>> +
>>> +	private:
>>> +		Diff(const MetadataList &l, std::size_t changed, std::size_t oldFill, std::size_t newFill)
>>> +			: l_(&l),
>>> +			  changed_(changed),
>>> +			  start_(l.contentOffset_ + oldFill),
>>> +			  stop_(l.contentOffset_ + newFill)
>>> +		{
>>> +		}
>>> +
>>> +		friend MetadataList;
>>> +		friend struct Checkpoint;
>>> +
>>> +		const MetadataList *l_ = nullptr;
>>> +		std::size_t changed_;
>>> +		std::size_t start_;
>>> +		std::size_t stop_;
>>> +	};
>>> +
>>> +	Diff merge(const ControlList &other)
>>
>> So the "merge" function is how pipeline handlers adds metadata to a
>> MetadataList ?

I think it's mainly `metadataAvailable()`, but yes, most pipeline handlers
will go through this merge() function when adding metadata from the IPA.


>>
>>> +	{
>>> +		// TODO: check id map of `other`?
>>
>> This already is a proper /* \todo */

My editor does not highlight "\todo" :(

>>
>>> +
>>> +		const auto c = checkpoint();
>>> +
>>> +		for (const auto &[tag, value] : other) {
>>> +			auto *e = find(tag);
>>> +			if (e) {
>>> +				[[maybe_unused]] auto r = set(*e, value);
>>> +				assert(r == SetError() || r == SetError::AlreadySet); // TODO: ?
>>
>> does "r == SetError()" means r == 0 ? What's the TODO for ?

The TODO is about how to handle (and which) errors here.
See `MetadataList::set()`.


>>
>>> +			}
>>> +		}
>>> +
>>> +		return c.diffSince();
>>> +	}
>>> +
>>> +	class Checkpoint
>>> +	{
>>> +	public:
>>> +		[[nodiscard]] Diff diffSince() const
>>> +		{
>>> +			/* sync with release-store on `state_` in `set()` */
>>> +			const auto curr = l_->state_.load(std::memory_order_acquire);
>>> +
>>> +			assert(s_.count <= curr.count);
>>> +			assert(s_.fill <= curr.fill);
>>> +
>>> +			return {
>>> +				*l_,
>>> +				curr.count - s_.count,
>>> +				s_.fill,
>>> +				curr.fill,
>>> +			};
>>> +		}
>>> +
>>> +	private:
>>> +		Checkpoint(const MetadataList &l)
>>> +			: l_(&l),
>>> +			  s_(l.state_.load(std::memory_order_relaxed))
>>> +		{
>>> +		}
>>> +
>>> +		friend MetadataList;
>>> +
>>> +		const MetadataList *l_ = nullptr;
>>
>> Is there in your opinion a case where a Checkpoint can overlive the
>> MetadataList it refers to ?

I don't see how that could be useful. Technically we can add `diffSince(const Checkpoint&)`
to `MetadataList` itself, and then this pointer is not needed. I originally
designed it like this to prevent any confusion as to which checkpoint
belongs to which list. On the other hand dropping the pointer would
prevent potential use after free issues.



>>
>>> +		State s_ = {};
>>> +	};
>>> +
>>> +	[[nodiscard]] Checkpoint checkpoint() const
>>> +	{
>>> +		return { *this };
>>> +	}
>>> +
>>> +private:
>>> +	[[nodiscard]] static constexpr std::size_t entriesOffset()
>>> +	{
>>> +		return 0;
>>> +	}
>>> +
>>> +	[[nodiscard]] static constexpr std::size_t contentOffset(std::size_t entries)
>>> +	{
>>> +		return details::align::up(entriesOffset() + entries * sizeof(Entry), alignof(ValueHeader));
>>> +	}
>>> +
>>> +	[[nodiscard]] Span<Entry> entries() const
>>> +	{
>>> +		return { reinterpret_cast<Entry *>(p_ + entriesOffset()), capacity_ };
>>> +	}
>>> +
>>> +	[[nodiscard]] Entry *find(std::uint32_t tag) const
>>> +	{
>>> +		const auto entries = this->entries();
>>> +		auto it = std::partition_point(entries.begin(), entries.end(), [&](const auto &e) {
>>
>> how is this different from find_if() ? Just out of curiosity

`std::partition_point` (and `std::{lower,upper}_bound`) implements binary search,
while `std::find_if()` is linear.



>>
>> I'll stop here for the time being. Just one more note, you moved
>> MetadataListPlan out this header, so you probably need a dedicated
>> .cpp file as well
>>
>>
>>> +			return e.tag < tag;
>>> +		});
>>> +
>>> +		if (it == entries.end() || it->tag != tag)
>>> +			return nullptr;
>>> +
>>> +		return &*it;
>>> +	}
>>> +
>>> +	[[nodiscard]] ControlValueView data_of(const Entry &e) const
>>> +	{
>>> +		const auto o = e.acquireData();
>>> +		return o ? data_of(*o) : ControlValueView{ };
>>> +	}
>>> +
>>> +	[[nodiscard]] ControlValueView data_of(std::size_t headerOffset) const
>>> +	{
>>> +		assert(headerOffset <= alloc_ - sizeof(ValueHeader));
>>> +		assert(details::align::is(p_ + headerOffset, alignof(ValueHeader)));
>>> +
>>> +		const auto *vh = reinterpret_cast<const ValueHeader *>(p_ + headerOffset);
>>> +		const auto *p = reinterpret_cast<const std::byte *>(vh) + sizeof(*vh);
>>> +		std::size_t avail = p_ + alloc_ - p;
>>> +
>>> +		const auto *data = details::align::up(vh->size, vh->alignment, p, &avail);
>>> +		assert(data);
>>> +
>>> +		return { vh->params.type, vh->params.isArray, vh->params.numElements, data };
>>> +	}
>>> +
>>> +	[[nodiscard]] SetError set(Entry &e, ControlValueView v)
>>> +	{
>>> +		if (e.hasValue())
>>> +			return SetError::AlreadySet;
>>> +		if (e.type != v.type() || e.isArray != v.isArray())
>>> +			return SetError::TypeMismatch;
> 
> Isn't this better expressed as an assertion ?

See below.


> 
>>> +
>>> +		const auto src = v.data();
>>> +		if (e.isArray) {
>>> +			if (src.size_bytes() > e.capacity)
>>> +				return SetError::SizeMismatch;
>>> +		} else {
>>> +			if (src.size_bytes() != e.capacity)
>>> +				return SetError::SizeMismatch;
>>> +		}
> 
> If any of these fails, it means the pipeline handler has wrongly sized
> the MetadataPlan, should we assert here as well ?

I am not sure about what the best error handling strategy is. I feel like
this is good for testing. I imagine we can add the necessary assertions
in `PipelineHandler::metadataAvailable()`.

The question of error handling in `MetadataList::merge()` still remains,
but I think we can make it so that it returns `std::optional<Diff>` to
signal errors, and that can be checked in the other in `PipelineHandler::metadataAvailable()`
overload.

Thoughts?


> 
>>> +
>>> +		auto s = state_.load(std::memory_order_relaxed);
>>> +		std::byte *oldEnd = p_ + contentOffset_ + s.fill;
>>> +		std::byte *p = oldEnd;
>>> +
>>> +		auto *headerPtr = details::align::up<ValueHeader>(p);
> 
> This aligns to the next ValueHeader
> 
>>> +		auto *dataPtr = details::align::up(src.size_bytes(), e.alignment, p);
> 
> This to size_bytes() + alignment
> 
>>> +		details::align::up(0, alignof(ValueHeader), p);
> 
> What does this do ?

This aligns `p` to the beginning of the next value header. This is required
because of how iteration works: every iterator is always aligned to the value
header, which means that the end iterator has to be aligned to that as well.
(Otherwise `==` and `!=` on the iterators would not be as simple as they are now.)

This can be done by essentially adding tail padding after the data, which
is what the above statement does. Technically `auto *headerPtr = details::align::up<ValueHeader>(p);`
could be rewritten as `auto *headerPtr = reinterpret_cast<ValueHeader *>(p);`
because `p` should already be aligned (due to the previous value's tail padding).


> 
>>> +
>>> +#if HAS_ASAN
>>> +		::__sanitizer_annotate_contiguous_container(
>>> +			p_ + contentOffset_, p_ + alloc_,
>>> +			oldEnd, p
>>> +		);
>>> +#endif
>>> +
>>> +		new (headerPtr) ValueHeader{
>>> +			.tag = e.tag,
>>> +			.size = std::uint32_t(src.size_bytes()),
>>> +			.alignment = e.alignment,
>>> +			.params = {
>>> +				.type = v.type(),
>>> +				.isArray = v.isArray(),
>>> +				.numElements = std::uint32_t(v.numElements()),
>>> +			},
>>> +		};
>>> +		std::memcpy(dataPtr, src.data(), src.size_bytes());
>>> +		e.headerOffset.store(reinterpret_cast<std::byte *>(headerPtr) - p_, std::memory_order_release);
> 
> break these long lines when it's trivial to do so

done


> 
>>> +
>>> +		s.fill += p - oldEnd;
>>> +		s.count += 1;
>>> +
>>> +		state_.store(s, std::memory_order_release);
>>> +
>>> +		return {};
>>> +	}
>>> +
>>> +	std::size_t capacity_ = 0;
>>> +	std::size_t contentOffset_ = -1;
>>> +	std::size_t alloc_ = 0;
>>> +	std::atomic<State> state_ = State{};
>>> +	std::byte *p_ = nullptr;
>>> +	// TODO: ControlIdMap in any way shape or form?
> 
> What do you mean here ?

A `ControlList` has an associated id map, info map, even validator.
Here I am mainly wondering there should be anything like that.
I personally don't see a convincing argument at the moment.


> 
>>> +
>>> +	/*
>>> +	 * If this is problematic on a 32-bit architecture, then
>>> +	 * `count` can be stored in a separate atomic variable
>>> +	 * but then `Diff::changed_` must be removed since the fill
>>> +	 * level and item count cannot be retrieved atomically.
> 
> or we can use a mutex or a different lock primitive ?

That's true, but I believe that would be a bit suboptimal.


> 
>>> +	 */
>>> +	static_assert(decltype(state_)::is_always_lock_free);
>>> +};
>>> +
>>> +} /* namespace libcamera */
>>> +
>>> +#undef HAS_ASAN
>>> diff --git a/include/libcamera/metadata_list_plan.h b/include/libcamera/metadata_list_plan.h
>>> new file mode 100644
>>> index 000000000..2ed35c54f
>>> --- /dev/null
>>> +++ b/include/libcamera/metadata_list_plan.h
>>> @@ -0,0 +1,130 @@
>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>> +/*
>>> + * Copyright (C) 2025, Ideas On Board Oy
>>> + */
>>> +
>>> +#pragma once
>>> +
>>> +#include <cassert>
>>> +#include <cstddef>
>>> +#include <cstdint>
>>> +#include <limits>
>>> +#include <map>
>>> +#include <type_traits>
>>> +
>>> +#include <libcamera/base/details/cxx20.h>
>>> +
>>> +#include <libcamera/controls.h>
>>> +
>>> +namespace libcamera {
>>> +
>>> +class MetadataListPlan
>>> +{
>>> +public:
>>> +	struct Entry {
>>> +		std::uint32_t size;
>>> +		std::uint32_t alignment; // TODO: is this necessary?
> 
> I wonder if the alignment shouldn't come from controls.h associated
> with ControlType
> 
> Something like control_type<T>::alignment

It could, and but that needs the addition of `alignment` to `control_type<>`
and some minimal changes to `MetadataListPlan` to query it.


> 
> I wonder if this would simplify the handling of alignment in
> MetadataList as well

If we agree on a fixed static alignment, e.g. 8 or 16, then this
field will no longer be needed.


> 
>>> +		std::uint32_t numElements;
>>> +		ControlType type;
>>> +		bool isArray;
>>> +	};
>>> +
>>> +	[[nodiscard]] bool empty() const { return items_.empty(); }
>>> +	[[nodiscard]] std::size_t size() const { return items_.size(); }
>>> +	[[nodiscard]] decltype(auto) begin() const { return items_.begin(); }
>>> +	[[nodiscard]] decltype(auto) end() const { return items_.end(); }
>>> +	void clear() { items_.clear(); }
>>> +
>>> +	template<
>>> +		typename T,
>>> +		std::enable_if_t<libcamera::details::control_type<T>::size != libcamera::dynamic_extent> * = nullptr
>>> +	>
>>> +	decltype(auto) set(const Control<T> &ctrl)
> 
> This calls the private overload
> 
> 	template<typename T>
> 	decltype(auto) set(std::uint32_t tag, std::size_t numElements, bool isArray)
> 
> Which returns
> 
>          *this;
> 
> which if I'm not mistaken means "MetadataListPlan *". Can this function
> returns anything else ? If the return type is known and trivial to
> type out why hide it behind decltype(auto) ?

It is `MetadataListPlan&`, my motivation in these cases is that development
is easier if you don't have to change all the types up the call stack. Here
I planned to replace it with the actual type when I would feel that the design
is mostly fixed, but I will do so now.



> 
>>> +	{
>>> +		if constexpr (libcamera::details::control_type<T>::size > 0) {
>>> +			static_assert(libcamera::details::control_type<T>::size != libcamera::dynamic_extent);
>>> +
>>> +			return set<typename T::value_type>(
>>> +				ctrl.id(),
>>> +				libcamera::details::control_type<T>::size,
>>> +				true
>>> +			);
>>> +		} else {
>>> +			return set<T>(ctrl.id(), 1, false);
>>> +		}
>>> +	}
>>> +
>>> +	template<
>>> +		typename T,
>>> +		std::enable_if_t<libcamera::details::control_type<T>::size == libcamera::dynamic_extent> * = nullptr
> 
> is this for strings only ? It seems to me that only std::string has
> control_type::size == dynamic_extent

Also for `libcamera::Span<T>` (i.e. dynamically sized arrays).


> 
>>> +	>
>>> +	decltype(auto) set(const Control<T> &ctrl, std::size_t numElements)
>>> +	{
>>> +		return set<typename T::value_type>(ctrl.id(), numElements, true);
>>> +	}
>>> +
>>> +	[[nodiscard]] bool set(std::uint32_t tag,
>>> +			       std::size_t size, std::size_t alignment,
>>> +			       std::size_t numElements, ControlType type, bool isArray)
> 
> Is this overload meant for the public interface of the class ?

This is public to allow calls from the IPC deserialization code.


> 
> It seems that the two previous overloads call the private one
> 
> 	template<typename T>
> 	decltype(auto) set(std::uint32_t tag, std::size_t numElements, bool isArray)
> 
> which then calls this public one. Is this intentional ?

Yes, the order of overloads is intentional.


> 
>>> +	{
>>> +		if (size == 0 || size > std::numeric_limits<std::uint32_t>::max())
>>> +			return false;
>>> +		if (alignment > std::numeric_limits<std::uint32_t>::max())
>>> +			return false;
>>> +		if (!details::cxx20::has_single_bit(alignment))
>>> +			return false;
>>> +		if (numElements > std::numeric_limits<std::uint32_t>::max() / size)
>>> +			return false;
>>> +		if (!isArray && numElements != 1)
>>> +			return false;
> 
> Should these be assertions (where and if possible) ?

This overload is used by the deserialization layer. I feel that these
should not be assertions, allowing the IPC deserialization to detect
the error, see https://patchwork.libcamera.org/project/libcamera/list/?series=5173
Now, the IPC layer will most likely abort at the moment, but I would
like to keep this function without assertions.


> 
>>> +
>>> +		items_.insert_or_assign(tag, Entry{
> 
> Do we allow to re-write an entry in the MetadataListPlan ?

Yes, this is for easy updating. See the rpi5 pipeline handler configure().


> 
>>> +			.size = std::uint32_t(size),
>>> +			.alignment = std::uint32_t(alignment),
>>> +			.numElements = std::uint32_t(numElements),
>>> +			.type = type,
>>> +			.isArray = isArray,
>>> +		});
>>> +
>>> +		return true;
>>> +	}
>>> +
>>> +	[[nodiscard]] const Entry *get(std::uint32_t tag) const
>>> +	{
>>> +		auto it = items_.find(tag);
>>> +		if (it == items_.end())
>>> +			return nullptr;
>>> +
>>> +		return &it->second;
>>> +	}
>>> +
>>> +	[[nodiscard]] const Entry *get(const ControlId &cid) const
>>> +	{
>>> +		const auto *e = get(cid.id());
>>> +		if (!e)
>>> +			return nullptr;
>>> +
>>> +		if (e->type != cid.type() || e->isArray != cid.isArray())
>>> +			return nullptr;
> 
> How can this happen ?

This could happen if someone does e.g. `get(properties::Location)` for some reason
when the metadata plan contains `controls::AeEnable`. They have the same numeric
id. I think it can be argued that this should be an assertion.


> 
>>> +
>>> +		return e;
>>> +	}
>>> +
>>> +private:
>>> +	std::map<std::uint32_t, Entry> items_;
>>> +
>>> +	template<typename T>
>>> +	decltype(auto) set(std::uint32_t tag, std::size_t numElements, bool isArray)
>>> +	{
>>> +		static_assert(std::is_trivially_copyable_v<T>);
>>> +
>>> +		[[maybe_unused]] bool ok = set(tag,
>>> +					       sizeof(T), alignof(T),
>>> +					       numElements, details::control_type<T>::value, isArray);
>>> +		assert(ok);
>>> +
>>> +		return *this;
>>> +	}
>>> +};
>>> +
>>> +} /* namespace libcamera */
>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
>>> index de1eb99b2..8c5ce4503 100644
>>> --- a/src/libcamera/meson.build
>>> +++ b/src/libcamera/meson.build
>>> @@ -9,6 +9,7 @@ libcamera_public_sources = files([
>>>       'framebuffer.cpp',
>>>       'framebuffer_allocator.cpp',
>>>       'geometry.cpp',
>>> +    'metadata_list.cpp',
>>>       'orientation.cpp',
>>>       'pixel_format.cpp',
>>>       'request.cpp',
>>> diff --git a/src/libcamera/metadata_list.cpp b/src/libcamera/metadata_list.cpp
>>> new file mode 100644
>>> index 000000000..ebefdfdad
>>> --- /dev/null
>>> +++ b/src/libcamera/metadata_list.cpp
>>> @@ -0,0 +1,344 @@
>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>> +/*
>>> + * Copyright (C) 2025, Ideas On Board Oy
>>> + */
>>> +
>>> +#include <libcamera/metadata_list.h>
>>> +
>>> +namespace libcamera {
>>> +
>>> +/**
>>> + * \class MetadataListPlan
>>> + * \brief Class to hold the possible set of metadata items for a MetadataList
>>> + */
>>> +
>>> +/**
>>> + * \class MetadataListPlan::Entry
>>> + * \brief Details of a metadata item
>>> + */
>>> +
>>> +/**
>>> + * \internal
>>> + * \var MetadataListPlan::Entry::size
>>> + * \brief Number of bytes in a single element
>>> + *
>>> + * \var MetadataListPlan::Entry::alignment
>>> + * \brief Required alignment of the elements
>>> + * \endinternal
>>> + *
>>> + * \var MetadataListPlan::Entry::numElements
>>> + * \brief Number of elements in the value
>>> + * \sa ControlValueView::numElements()
>>> + *
>>> + * \var MetadataListPlan::Entry::type
>>> + * \brief The type of the value
>>> + * \sa ControlValueView::type()
>>> + *
>>> + * \var MetadataListPlan::Entry::isArray
>>> + * \brief Whether or not the value is array-like
>>> + * \sa ControlValueView::isArray()
>>> + */
>>> +
>>> +/**
>>> + * \fn MetadataListPlan::begin() const
>>> + * \brief Retrieve the begin iterator
>>> + */
>>> +
>>> +/**
>>> + * \fn MetadataListPlan::end() const
>>> + * \brief Retrieve the end iterator
>>> + */
>>> +
>>> +/**
>>> + * \fn MetadataListPlan::size() const
>>> + * \brief Retrieve the number of entries
>>> + */
>>> +
>>> +/**
>>> + * \fn MetadataListPlan::empty() const
>>> + * \brief Check if empty
>>> + */
>>> +
>>> +/**
>>> + * \internal
>>> + * \fn MetadataListPlan::clear()
>>> + * \brief Remove all controls
>>> + */
>>> +
>>> +/**
>>> + * \internal
>>> + * \fn MetadataListPlan::set(const Control<T> &ctrl)
>>> + * \brief Add an entry for the given control to the metadata list plan
>>> + * \param[in] ctrl The control
>>> + */
>>> +
>>> +/**
>>> + * \internal
>>> + * \fn MetadataListPlan::set(const Control<T> &ctrl, std::size_t count)
>>> + * \brief Add an entry for the given dynamically-sized control to the metadata list plan
>>> + * \param[in] ctrl The control
>>> + * \param[in] count The maximum number of elements
>>> + *
>>> + * Add the dynamically-sized control \a ctrl to the metadata list plan with a maximum
>>> + * capacity of \a count elements.
>>> + */
>>> +
>>> +/**
>>> + * \internal
>>> + * \fn MetadataListPlan::set(std::uint32_t tag,
>>> + *			     std::size_t size, std::size_t alignment,
>>> + *			     std::size_t count, ControlType type, bool isArray)
>>> + * \brief Add an entry to the metadata list plan
>>> + * \return \a true if the entry has been added, or \a false if the given parameters
>>> + *         would result in an invalid entry
>>> + *
>>> + * This functions adds an entry with essentially arbitrary parameters, without deriving
>>> + * them from a given ControlId instance. This is mainly used when deserializing.
>>> + */
>>> +
>>> +/**
>>> + * \fn MetadataListPlan::get(std::uint32_t tag) const
>>> + * \brief Find the \ref Entry "entry" with the given identifier
>>> + */
>>> +
>>> +/**
>>> + * \fn MetadataListPlan::get(const ControlId &cid) const
>>> + * \brief Find the \ref Entry "entry" for the given ControlId
>>> + *
>>> + * The \ref Entry "entry" is only returned if ControlId::type() and ControlId::isArray()
>>> + * of \a cid matches Entry::type and Entry::isArray, respectively.
>>> + */
>>> +
>>> +/**
>>> + * \class MetadataList
>>> + * \brief Class to hold metadata items
>>> + */
>>> +
>>> +/**
>>> + * \fn MetadataList::MetadataList(const MetadataListPlan &plan)
>>> + * \brief Construct a metadata list according to \a plan
>>> + *
>>> + * Construct a metadata list according to the provided \a plan.
>>> + */
>>> +
>>> +/**
>>> + * \fn MetadataList::size() const
>>> + * \brief Retrieve the number of controls
>>> + * \context This function is \threadsafe.
>>> + * \note If the list is being modified, the return value may be out of
>>> + *       date by the time the function returns
>>> + */
>>> +
>>> +/**
>>> + * \fn MetadataList::empty() const
>>> + * \brief Check if empty
>>> + * \context This function is \threadsafe.
>>> + * \note If the list is being modified, the return value may be out of
>>> + *       date by the time the function returns
>>> + */
>>> +
>>> +/**
>>> + * \internal
>>> + * \fn MetadataList::clear()
>>> + * \brief Remove all items from the list
>>> + * \note This function in effect resets the list to its original state. As a consequence it invalidates - among others -
>>> + *       all iterators, Checkpoint, and Diff objects that are associated with the list. No readers must exist
>>> + *       when this function is called.
>>> + */
>>> +
>>> +/**
>>> + * \fn MetadataList::begin() const
>>> + * \brief Retrieve begin iterator
>>> + * \context This function is \threadsafe.
>>> + */
>>> +
>>> +/**
>>> + * \fn MetadataList::end() const
>>> + * \brief Retrieve end iterator
>>> + * \context This function is \threadsafe.
>>> + */
>>> +
>>> +/**
>>> + * \fn MetadataList::get(const Control<T> &ctrl) const
>>> + * \brief Get the value of control \a ctrl
>>> + * \return A std::optional<T> containing the control value, or std::nullopt if
>>> + *         the control \a ctrl is not present in the list
>>> + * \context This function is \threadsafe.
>>> + */
>>> +
>>> +/**
>>> + * \fn MetadataList::get(std::uint32_t tag) const
>>> + * \brief Get the value of pertaining to the numeric identifier \a tag
>>> + * \return A std::optional<T> containing the control value, or std::nullopt if
>>> + *         the control is not present in the list
>>> + * \context This function is \threadsafe.
>>> + */
>>> +
>>> +/**
>>> + * \internal
>>> + * \fn MetadataList::set(const Control<T> &ctrl, const details::cxx20::type_identity_t<T> &value)
>>> + * \brief Set the value of control \a ctrl to \a value
>>> + */
>>> +
>>> +/**
>>> + * \internal
>>> + * \fn MetadataList::set(std::uint32_t tag, ControlValueView v)
>>> + * \brief Set the value of pertaining to the numeric identifier \a tag to \a v
>>> + */
>>> +
>>> +/**
>>> + * \internal
>>> + * \fn MetadataList::merge(const ControlList &other)
>>> + * \brief Add all missing items from \a other
>>> + *
>>> + * Add all items from \a other that are not present in \a this.
>>> + */
>>> +
>>> +/**
>>> + * \internal
>>> + * \enum MetadataList::SetError
>>> + * \brief Error code returned by a set operation
>>> + *
>>> + * \var MetadataList::SetError::UnknownTag
>>> + * \brief The tag is not supported by the metadata list
>>> + * \var MetadataList::SetError::AlreadySet
>>> + * \brief A value has already been added with the given tag
>>> + * \var MetadataList::SetError::SizeMismatch
>>> + * \brief The size of the data is not appropriate for the given tag
>>> + * \var MetadataList::SetError::TypeMismatch
>>> + * \brief The type of the value does not match the expected type
>>> + */
>>> +
>>> +/**
>>> + * \internal
>>> + * \fn MetadataList::checkpoint() const
>>> + * \brief Create a checkpoint
>>> + * \context This function is \threadsafe.
>>> + */
>>> +
>>> +/**
>>> + * \class MetadataList::iterator
>>> + * \brief Iterator
>>> + */
>>> +
>>> +/**
>>> + * \typedef MetadataList::iterator::difference_type
>>> + * \brief iterator's difference type
>>> + */
>>> +
>>> +/**
>>> + * \typedef MetadataList::iterator::value_type
>>> + * \brief iterator's value type
>>> + */
>>> +
>>> +/**
>>> + * \typedef MetadataList::iterator::pointer
>>> + * \brief iterator's pointer type
>>> + */
>>> +
>>> +/**
>>> + * \typedef MetadataList::iterator::reference
>>> + * \brief iterator's reference type
>>> + */
>>> +
>>> +/**
>>> + * \typedef MetadataList::iterator::iterator_category
>>> + * \brief iterator's category
>>> + */
>>> +
>>> +/**
>>> + * \fn MetadataList::iterator::operator*()
>>> + * \brief Retrieve value at iterator
>>> + * \return A \a ControlListView representing the value
>>> + */
>>> +
>>> +/**
>>> + * \fn MetadataList::iterator::operator==(const iterator &other) const
>>> + * \brief Check if two iterators are equal
>>> + */
>>> +
>>> +/**
>>> + * \fn MetadataList::iterator::operator!=(const iterator &other) const
>>> + * \brief Check if two iterators are not equal
>>> + */
>>> +
>>> +/**
>>> + * \fn MetadataList::iterator::operator++(int)
>>> + * \brief Advance the iterator
>>> + */
>>> +
>>> +/**
>>> + * \fn MetadataList::iterator::operator++()
>>> + * \brief Advance the iterator
>>> + */
>>> +
>>> +/**
>>> + * \class MetadataList::Diff
>>> + * \brief Designates a set of consecutively added metadata items from a particular MetadataList
>>> + * \sa Camera::metadataAvailable
>>> + * \internal
>>> + * \sa MetadataList::Checkpoint::diffSince()
>>> + * \endinternal
>>> + */
>>> +
>>> +/**
>>> + * \fn MetadataList::Diff::list() const
>>> + * \brief Retrieve the associated MetadataList
>>> + */
>>> +
>>> +/**
>>> + * \fn MetadataList::Diff::size() const
>>> + * \brief Retrieve the number of metadata items designated
>>> + */
>>> +
>>> +/**
>>> + * \fn MetadataList::Diff::empty() const
>>> + * \brief Check if any metadata items are designated
>>> + */
>>> +
>>> +/**
>>> + * \fn MetadataList::Diff::operator bool() const
>>> + * \copydoc MetadataList::Diff::empty() const
>>> + */
>>> +
>>> +/**
>>> + * \fn MetadataList::Diff::get(const Control<T> &ctrl) const
>>> + * \copydoc MetadataList::get(const Control<T> &ctrl) const
>>> + * \note The lookup will fail if the metadata item is not designated by this Diff object,
>>> + *       even if it is otherwise present in the backing MetadataList.
>>> + */
>>> +
>>> +/**
>>> + * \fn MetadataList::Diff::get(std::uint32_t tag) const
>>> + * \copydoc MetadataList::get(std::uint32_t tag) const
>>> + * \note The lookup will fail if the metadata item is not designated by this Diff object,
>>> + *       even if it is otherwise present in the backing MetadataList.
>>> + */
>>> +
>>> +/**
>>> + * \fn MetadataList::Diff::begin() const
>>> + * \brief Retrieve the begin iterator
>>> + */
>>> +
>>> +/**
>>> + * \fn MetadataList::Diff::end() const
>>> + * \brief Retrieve the end iterator
>>> + */
>>> +
>>> +/**
>>> + * \internal
>>> + * \class MetadataList::Checkpoint
>>> + * \brief Designates a point in the stream of metadata items
>>> + *
>>> + * A Checkpoint object designates a point in the stream of metadata items in the associated
>>> + * MetadataList. Its main use to be able to retrieve the set of metadata items that were
>>> + * added to the list after the designated point using diffSince().
>>> + */
>>> +
>>> +/**
>>> + * \internal
>>> + * \fn MetadataList::Checkpoint::diffSince() const
>>> + * \brief Retrieve the set of metadata items added since the checkpoint was created
>>> + */
>>> +
>>> +} /* namespace libcamera */
>>> diff --git a/test/controls/meson.build b/test/controls/meson.build
>>> index 763f8905e..b68a4fc53 100644
>>> --- a/test/controls/meson.build
>>> +++ b/test/controls/meson.build
>>> @@ -5,6 +5,7 @@ control_tests = [
>>>       {'name': 'control_info_map', 'sources': ['control_info_map.cpp']},
>>>       {'name': 'control_list', 'sources': ['control_list.cpp']},
>>>       {'name': 'control_value', 'sources': ['control_value.cpp']},
>>> +    {'name': 'metadata_list', 'sources': ['metadata_list.cpp']},
>>>   ]
>>>
>>>   foreach test : control_tests
>>> diff --git a/test/controls/metadata_list.cpp b/test/controls/metadata_list.cpp
>>> new file mode 100644
>>> index 000000000..f0872acd9
>>> --- /dev/null
>>> +++ b/test/controls/metadata_list.cpp
>>> @@ -0,0 +1,170 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>> +/*
>>> + * Copyright (C) 2025, Ideas On Board Oy
>>> + *
>>> + * MetadataList tests
>>> + */
>>> +
>>> +#include <future>
>>> +#include <iostream>
>>> +#include <thread>
>>> +
>>> +#include <libcamera/control_ids.h>
>>> +#include <libcamera/metadata_list.h>
>>> +#include <libcamera/property_ids.h>
>>> +
>>> +#include "test.h"
>>> +
>>> +using namespace std;
>>> +using namespace libcamera;
>>> +
>>> +#define ASSERT(x) do { \
>>> +	if (!static_cast<bool>(x)) { \
>>> +		std::cerr << '`' << #x << "` failed" << std::endl; \
>>> +		return TestFail; \
>>> +	} \
>>> +} while (false)
>>> +
>>> +class MetadataListTest : public Test
>>> +{
>>> +public:
>>> +	MetadataListTest() = default;
>>> +
>>> +protected:
>>> +	int run() override
>>> +	{
>>> +		MetadataListPlan mlp;
>>> +		mlp.set(controls::ExposureTime);
>>> +		mlp.set(controls::ExposureValue);
>>> +		mlp.set(controls::ColourGains);
>>> +		mlp.set(controls::AfWindows, 10);
>>> +		mlp.set(controls::AeEnable);
>>> +		mlp.set(controls::SensorTimestamp);
>>> +
>>> +		MetadataList ml(mlp);
>>> +
>>> +		static_assert(static_cast<unsigned int>(properties::LOCATION) == controls::AE_ENABLE);
>>> +		ASSERT(ml.set(properties::Location, properties::CameraLocationFront) == MetadataList::SetError::TypeMismatch);
> 
> Can you add a comment to explain what this tests ?

Please see

		/*
		 *`properties::Location` has the same numeric id as `controls::AeEnable` (checked by the `static_assert`
		 * below), but they have different types; check that this is detected.
		 */
		static_assert(static_cast<unsigned int>(properties::LOCATION) == controls::AE_ENABLE);
		ASSERT(ml.set(properties::Location, properties::CameraLocationFront) == MetadataList::SetError::TypeMismatch);

> 
>>> +
>>> +		ASSERT(ml.set(controls::AfWindows, std::array<Rectangle, 11>{}) == MetadataList::SetError::SizeMismatch);
>>> +		ASSERT(ml.set(controls::ColourTemperature, 123) == MetadataList::SetError::UnknownTag);
>>> +
>>> +		auto f1 = std::async(std::launch::async, [&] {
>>> +			using namespace std::chrono_literals;
>>> +
>>> +			std::this_thread::sleep_for(500ms);
>>> +			ASSERT(ml.set(controls::ExposureTime, 0x1111) == MetadataList::SetError());
>>> +
>>> +			std::this_thread::sleep_for(500ms);
>>> +			ASSERT(ml.set(controls::ExposureValue, 1) == MetadataList::SetError());
>>> +
>>> +			std::this_thread::sleep_for(500ms);
>>> +			ASSERT(ml.set(controls::ColourGains, std::array{
>>> +				123.f,
>>> +				456.f
>>> +			}) == MetadataList::SetError());
>>> +
>>> +			std::this_thread::sleep_for(500ms);
>>> +			ASSERT(ml.set(controls::AfWindows, std::array{
>>> +				Rectangle(),
>>> +				Rectangle(1, 2, 3, 4),
>>> +				Rectangle(0x1111, 0x2222, 0x3333, 0x4444),
>>> +			}) == MetadataList::SetError());
> 
> I don't get why you expect these to fail..

Hmm... I think my intention is not clear. A default constructed `SetError` object,
i.e. `MetadataList::SetError()` is supposed to denote success. This is modelled
after `std::errc`. I can rename it to `SetResult` and add another enumeration like so:

   enum class SetResult {
     Success,
     /* rest of the failure cases */
   };

do you think that would be clearer?


> 
>>> +
>>> +			return TestPass;
>>> +		});
>>> +
>>> +		auto f2 = std::async(std::launch::async, [&] {
>>> +			for (;;) {
>>> +				const auto x = ml.get(controls::ExposureTime);
>>> +				const auto y = ml.get(controls::ExposureValue);
>>> +				const auto z = ml.get(controls::ColourGains);
>>> +				const auto w = ml.get(controls::AfWindows);
>>> +
>>> +				if (x)
>>> +					ASSERT(*x == 0x1111);
>>> +
>>> +				if (y)
>>> +					ASSERT(*y == 1.0f);
>>> +
>>> +				if (z) {
>>> +					ASSERT(z->size() == 2);
>>> +					ASSERT((*z)[0] == 123.f);
>>> +					ASSERT((*z)[1] == 456.f);
>>> +				}
>>> +
>>> +				if (w) {
>>> +					ASSERT(w->size() == 3);
>>> +					ASSERT((*w)[0].isNull());
>>> +					ASSERT((*w)[1] == Rectangle(1, 2, 3, 4));
>>> +					ASSERT((*w)[2] == Rectangle(0x1111, 0x2222, 0x3333, 0x4444));
>>> +				}
>>> +
>>> +				if (x && y && z && w)
>>> +					break;
>>> +			}
>>> +
>>> +			return TestPass;
> 
> and if f1 is expected to fail how can f2 pass.
> 
> I'm surely missing something

See my comment above, both are expected to pass.


> 
>>> +		});
>>> +
>>> +		ASSERT(f1.get() == TestPass);
>>> +		ASSERT(f2.get() == TestPass);
>>> +
>>> +		ASSERT(ml.set(controls::ExposureTime, 0x2222) == MetadataList::SetError::AlreadySet);
>>> +		ASSERT(ml.set(controls::ExposureValue, 2) == MetadataList::SetError::AlreadySet);
>>> +
>>> +		ASSERT(ml.get(controls::ExposureTime) == 0x1111);
>>> +		ASSERT(ml.get(controls::ExposureValue) == 1);
>>> +
>>> +		for (auto &&[tag, v] : ml)
>>> +			std::cout << "[" << tag << "] -> " << v << '\n';
> 
> Are we sure we want this in tests ? I mean, it doesn't hurt..

I don't know... I have been sorely missing prints when I was looking
at other tests. You're essentially forced to use a debugger.


> 
>>> +
>>> +		std::cout << std::endl;
>>> +
>>> +		ml.clear();
>>> +		ASSERT(ml.empty());
>>> +		ASSERT(ml.size() == 0);
>>> +
>>> +		ASSERT(ml.set(controls::ExposureTime, 0x2222) == MetadataList::SetError());
>>> +		ASSERT(ml.get(controls::ExposureTime) == 0x2222);
>>> +
>>> +		auto c = ml.checkpoint();
>>> +
>>> +		ASSERT(ml.set(controls::ExposureValue, 2) == MetadataList::SetError());
>>> +		ASSERT(ml.set(controls::SensorTimestamp, 0x99999999) == MetadataList::SetError());
>>> +
>>> +		auto d = c.diffSince();
>>> +		ASSERT(&d.list() == &ml);
>>> +
>>> +		ASSERT(ml.set(controls::ColourGains, std::array{ 1.f, 2.f }) == MetadataList::SetError());
>>> +
>>> +		ASSERT(d);
>>> +		ASSERT(!d.empty());
>>> +		ASSERT(d.size() == 2);
>>> +		ASSERT(!d.get(controls::ExposureTime));
>>> +		ASSERT(!d.get(controls::ColourGains));
>>> +		ASSERT(!d.get(controls::AfWindows));
>>> +		ASSERT(d.get(controls::ExposureValue) == 2);
>>> +		ASSERT(d.get(controls::SensorTimestamp) == 0x99999999);
>>> +
>>> +		for (auto &&[tag, v] : d)
>>> +			std::cout << "[" << tag << "] -> " << v << '\n';
>>> +
>>> +		/* Test if iterators work with algorithms. */
>>> +		std::ignore = std::find_if(d.begin(), d.end(), [](const auto &) {
>>> +			return false;
>>> +		});
>>> +
>>> +#if 0
>>> +               {
>>> +                       auto it = ml.begin();
>>> +                       ml.clear();
>>> +                       std::ignore = *it; /* Trigger ASAN. */
>>> +               }
>>> +#endif
> 
> Maybe remove this ?

I think I will try to move it into a separate test that is only active
when ASAN is used and expects failure.

---

I hope I haven't missed anything.

Regards,
Barnabás Pőcze

> 
>>> +
>>> +		return TestPass;
>>> +	}
>>> +};
>>> +
>>> +TEST_REGISTER(MetadataListTest)
> 
> Thanks again for your hard work on this!
> 
>>> --
>>> 2.50.1
>>>