[PATCH v1 2/8] libcamera: Add a DebugMetadata helper

[Laurent Pinchart]

Hi Stefan,

Thank you for the patch.

On Wed, Oct 02, 2024 at 06:19:20PM +0200, Stefan Klug wrote:
> Debug metadata often occurs in places where the metadata control list is
> not available e.g. in queueRequest() or processStatsBuffer() or even in
> a class far away from the metadata handling code. It is therefore
> difficult to add debug metadata without adding lot's of boilerplate

s/lot's/lots/

> code. This can be mitigated by recording the metadata and forwarding it
> to an upstream helper or the metadata control list when they become
> available. Add such a helper.
> 
> Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> ---
>  include/libcamera/internal/debug_controls.h |  52 +++++++
>  include/libcamera/internal/meson.build      |   1 +
>  src/libcamera/control_ids_core.yaml         |   6 +
>  src/libcamera/debug_controls.cpp            | 144 ++++++++++++++++++++
>  src/libcamera/meson.build                   |   1 +
>  5 files changed, 204 insertions(+)
>  create mode 100644 include/libcamera/internal/debug_controls.h
>  create mode 100644 src/libcamera/debug_controls.cpp
> 
> diff --git a/include/libcamera/internal/debug_controls.h b/include/libcamera/internal/debug_controls.h
> new file mode 100644
> index 000000000000..d95c03e1db1f
> --- /dev/null
> +++ b/include/libcamera/internal/debug_controls.h
> @@ -0,0 +1,52 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Ideas on Board Oy
> + *
> + * Debug metadata helpers
> + */
> +
> +#pragma once
> +
> +#include <libcamera/control_ids.h>
> +
> +namespace libcamera {
> +
> +class DebugMetadata
> +{
> +public:
> +	DebugMetadata() = default;
> +
> +	void checkForEnable(const ControlList &controls);
> +	void enable(bool enable = true);
> +	void assignUpstream(DebugMetadata *upstream);
> +	void assignControlList(ControlList *list);
> +
> +	template<typename T, typename V>
> +	void set(const Control<T> &ctrl, const V &value)
> +	{
> +		if (upstream_) {
> +			upstream_->set<T, V>(ctrl, value);

Do you need to explicitly mention <T, V> here, won't type deduction work
automatically ? Same below.

> +			return;
> +		}
> +
> +		if (!enabled_)
> +			return;
> +
> +		if (list_) {
> +			list_->set<T, V>(ctrl, value);
> +			return;
> +		}
> +
> +		cache_.set<T, V>(ctrl, value);
> +	}
> +
> +	void set(unsigned int id, const ControlValue &value);
> +
> +private:
> +	bool enabled_ = false;
> +	ControlList *list_ = nullptr;
> +	DebugMetadata *upstream_ = nullptr;
> +	ControlList cache_;
> +};
> +
> +} /* namespace libcamera */
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> index 1c5eef9cab80..1dddcd50c90b 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build
> @@ -14,6 +14,7 @@ libcamera_internal_headers = files([
>      'control_serializer.h',
>      'control_validator.h',
>      'converter.h',
> +    'debug_controls.h',
>      'delayed_controls.h',
>      'device_enumerator.h',
>      'device_enumerator_sysfs.h',
> diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml
> index 1b1bd9507d25..103bcb593c4a 100644
> --- a/src/libcamera/control_ids_core.yaml
> +++ b/src/libcamera/control_ids_core.yaml
> @@ -967,5 +967,11 @@ controls:
>  
>          The default gamma value must be 2.2 which closely mimics sRGB gamma.
>          Note that this is camera gamma, so it is applied as 1.0/gamma.
> +        
> +  - DebugMetadataEnable:
> +      type: bool
> +      description: |
> +        Enable or disable the debug metadata.
> +

Extra blank line.

>  
>  ...
> diff --git a/src/libcamera/debug_controls.cpp b/src/libcamera/debug_controls.cpp
> new file mode 100644
> index 000000000000..11a92a6b5871
> --- /dev/null
> +++ b/src/libcamera/debug_controls.cpp
> @@ -0,0 +1,144 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Ideas on Board Oy.
> + *
> + * Helper to easily record debug metadata inside libcamera.
> + */
> +
> +#include "libcamera/internal/debug_controls.h"
> +
> +namespace libcamera {
> +
> +/**
> + * \file debug_controls.h
> + * \brief Helper to easily record debug metadata inside libcamera
> + */
> +
> +/**
> + * \class DebugMetadata
> + * \brief Helper to record metadata for later use
> + *
> + * When one wants to record debug metadata, the metadata list is often not
> + * directly available (either because we are inside process() of an IPA or
> + * because we are in a closed module). This class allows to record the data and
> + * at a later point in time forward it either to another DebugMetadata instance
> + * or to a ControlList.
> + */
> +
> +/**
> + * \fn DebugMetadata::checkForEnable
> + * \brief Check for DebugMetadataEnable in the supplied ControlList
> + * \param[in] controls The supplied ControlList
> + *
> + * Looks for controls::DebugMetadataEnable and enables or disables debug
> + * metadata handling accordingly.
> + */
> +void DebugMetadata::checkForEnable(const ControlList &controls)
> +{
> +	const auto &ctrl = controls.get(controls::DebugMetadataEnable);
> +	if (ctrl)
> +		enable(*ctrl);
> +}
> +
> +/**
> + * \fn DebugMetadata::enable
> + * \brief Enables or disabled metadata handling
> + * \param[in] enable The enable state
> + *
> + * Enables or disables metadata handling according to \a enable. When enable is
> + * false, the cache gets cleared and no further metadata is recorded.
> + */
> +void DebugMetadata::enable(bool enable)
> +{
> +	enabled_ = enable;
> +	if (!enabled_)
> +		cache_.clear();
> +}
> +
> +/**
> + * \fn DebugMetadata::assignUpstream
> + * \brief Assign an upstream metadata handler
> + * \param[in] upstream Pointer to the upstream handler
> + *
> + * When a upstream gets set, the cache is copies to that list and all further

s/copies/copied/

> + * calls to DebugMetadata::set are forwarded to that list.
> + *
> + * The upstream can be reset by passing a nullptr.

I think "parent" is a better name than "upstream", the parent concept is
widely used. I'd call the function setParent() as we use "set" rather
than "assign".

> + */
> +void DebugMetadata::assignUpstream(DebugMetadata *upstream)
> +{
> +	upstream_ = upstream;
> +
> +	if (!upstream_)
> +		return;
> +
> +	for (const auto &ctrl : cache_)
> +		upstream_->set(ctrl.first, ctrl.second);
> +
> +	cache_.clear();
> +}
> +
> +/**
> + * \fn DebugMetadata::assignControlList
> + * \brief Assign a control list
> + * \param[in] list Pointer to the list
> + *
> + * When a list gets set, the cache is copies to that list and all further calls

s/copies/copied/

> + * to DebugMetadata::set are forwarded to that list.
> + *
> + * The upstream can be reset by passing a nullptr.

s/upstream/list/

> + */
> +void DebugMetadata::assignControlList(ControlList *list)
> +{
> +	list_ = list;
> +
> +	if (list_) {
> +		list_->merge(cache_);
> +		cache_.clear();
> +	}
> +}
> +
> +/**
> + * \fn DebugMetadata::set(const Control<T> &ctrl, const V &value)
> + * \brief Set a value
> + * \param[in] ctrl The ctrl to set
> + * \param[in] value The control value
> + *
> + * Sets the debug metadata for ctrl to value \a value. If an upstream or a
> + * control list is set, the value gets passed there (upstream takes precedence).
> + * Otherwise the value is cached until one of them gets assigned.
> + *
> + * When the instance is disabled and upstream is not set, the value gets dropped.
> + */
> +
> +/**
> + * \fn DebugMetadata::set(unsigned int id, const ControlValue &value)
> + * \brief Set a value
> + * \param[in] id The id of the control
> + * \param[in] value The control value
> + *
> + * Sets the debug metadata to value \a value. If an upstream or a control list
> + * is set, the value gets passed there (upstream takes precedence).
> + * Otherwise the value is cached until one of them gets assigned.
> + *
> + * When the instance is disabled and upstream is not set, the value gets dropped.
> + */
> +void DebugMetadata::set(unsigned int id, const ControlValue &value)
> +{
> +	if (upstream_) {
> +		upstream_->set(id, value);
> +		return;
> +	}
> +
> +	if (!enabled_)
> +		return;
> +
> +	if (list_) {
> +		list_->set(id, value);
> +		return;
> +	}
> +
> +	cache_.set(id, value);
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index aa9ab0291854..f7b5ee8dcc34 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -25,6 +25,7 @@ libcamera_internal_sources = files([
>      'control_validator.cpp',
>      'converter.cpp',
>      'delayed_controls.cpp',
> +    'debug_controls.cpp',
>      'device_enumerator.cpp',
>      'device_enumerator_sysfs.cpp',
>      'dma_buf_allocator.cpp',

-- 
Regards,

Laurent Pinchart