[PATCH v2 2/7] libcamera: Add a DebugMetadata helper

[Laurent Pinchart]

On Mon, Oct 07, 2024 at 11:54:06AM +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 lots of boilerplate
> code. This can be mitigated by recording the metadata and forwarding it
> to the metadata control list when it becomes available. To solve the
> issue of code that is far away from the metadata context, add a chaining
> mechanism to allow loose coupling at runtime.
> 
> Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> 
> ---
> Changes in v2:
> - Replace assignments of ControlList by a moveEntries function
> - Replace assignUpstream by setParent
> - Improve documentation
> ---
>  include/libcamera/internal/debug_controls.h |  46 +++++++
>  include/libcamera/internal/meson.build      |   1 +
>  src/libcamera/control_ids_core.yaml         |   5 +
>  src/libcamera/control_ids_debug.yaml        |   3 +-
>  src/libcamera/debug_controls.cpp            | 145 ++++++++++++++++++++
>  src/libcamera/meson.build                   |   1 +
>  6 files changed, 199 insertions(+), 2 deletions(-)
>  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..00457d60b1a2
> --- /dev/null
> +++ b/include/libcamera/internal/debug_controls.h
> @@ -0,0 +1,46 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Google Inc.
> + *
> + * 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 setParent(DebugMetadata *parent);
> +	void moveEntries(ControlList &list);
> +
> +	template<typename T, typename V>
> +	void set(const Control<T> &ctrl, const V &value)
> +	{
> +		if (parent_) {
> +			parent_->set<T, V>(ctrl, value);

I think you can drop the explicit types here. Same below.

> +			return;
> +		}
> +
> +		if (!enabled_)
> +			return;
> +
> +		cache_.set<T, V>(ctrl, value);
> +	}
> +
> +	void set(unsigned int id, const ControlValue &value);
> +
> +private:
> +	bool enabled_ = false;
> +	DebugMetadata *parent_ = 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..d34a2d068b60 100644
> --- a/src/libcamera/control_ids_core.yaml
> +++ b/src/libcamera/control_ids_core.yaml
> @@ -968,4 +968,9 @@ 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.
> +
>  ...
> diff --git a/src/libcamera/control_ids_debug.yaml b/src/libcamera/control_ids_debug.yaml
> index 62f742c58129..797532712099 100644
> --- a/src/libcamera/control_ids_debug.yaml
> +++ b/src/libcamera/control_ids_debug.yaml
> @@ -3,5 +3,4 @@
>  %YAML 1.1
>  ---
>  vendor: debug
> -controls:
> -
> +controls: []

Is this needed ? If so, should it go to the previous patch ?

> diff --git a/src/libcamera/debug_controls.cpp b/src/libcamera/debug_controls.cpp
> new file mode 100644
> index 000000000000..84157aca5a74
> --- /dev/null
> +++ b/src/libcamera/debug_controls.cpp
> @@ -0,0 +1,145 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Google Inc.
> + *
> + * Helper to easily record debug metadata inside libcamera.
> + */
> +
> +#include "libcamera/internal/debug_controls.h"
> +
> +#include <libcamera/base/log.h>
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(DebugControls)
> +
> +/**
> + * \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

s/checkForEnable/checkForEnable()/

Same below.

> + * \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)

I'm not a fan of the function name. "check" sounds like this function
performs a check, but it actually enables or disable recording of
metadata.

> +{
> +	const auto &ctrl = controls.get(controls::DebugMetadataEnable);
> +	if (ctrl)
> +		enable(*ctrl);
> +}
> +
> +/**
> + * \fn DebugMetadata::enable
> + * \brief Enables or disabled metadata handling

\brief uses the imperative mood:

 * \brief Enable or disable metadata handling

Or s/handling/recording/

> + * \param[in] enable The enable state
> + *
> + * Enables or disables metadata handling according to \a enable. When \a enable

This sentence needs a subject (see below I've reviewed the patch from
bottom to top).

> + * is true, all calls to set() get cached and can later be retrieved using \a
> + * DebugMetadata::moveEntries(). When \a enable is false, the cache gets cleared
> + * and no further metadata is recorded.
> + *
> + * Forwarding to a parent is independent of the enabled state.
> + */
> +void DebugMetadata::enable(bool enable)
> +{
> +	enabled_ = enable;
> +	if (!enabled_)
> +		cache_.clear();
> +}
> +
> +/**
> + * \fn DebugMetadata::setParent
> + * \brief Assign a parent metadata handler
> + * \param[in] parent Pointer to the parent handler
> + *
> + * When a \a parent gets set, all further calls to DebugMetadata::set are

s/::set/::set()/

> + * forwarded to that instance. It is not allowed to enable a DebugMetadata
> + * object, log entries to it and later set the parent. This is done to keep a
> + * path open for switching to tracing infrastructure later. For tracing one
> + * would need some kind of "context" identifier that needs to be available on
> + * set() time. The parent can be treated as such. The top level object
> + * (the one where enable() get's called) lives in a place where that information
> + * is also available.

There's room for improvement. Some of the information here probably
belongs to the \class documentation. If you re-read the class
documentation without any knowledge of how this class works and what it
does, the documentation isn't very clear.

> + *
> + * The parent can be reset by passing a nullptr.
> + */
> +void DebugMetadata::setParent(DebugMetadata *parent)
> +{
> +	parent_ = parent;
> +
> +	if (!parent_)
> +		return;
> +
> +	if (!cache_.empty())
> +		LOG(DebugControls, Error)
> +			<< "Controls were recorded before setting a parent."
> +			<< " These are dropped.";
> +
> +	cache_.clear();
> +}
> +
> +/**
> + * \fn DebugMetadata::moveEntries
> + * \brief Move all cached entries into a list

s/list/control list/

> + * \param[in] list The list

The control list

> + *
> + * Moves all entries into the list specified by \a list. Duplicate entries in
> + * \a list get overwritten.

"Moves" needs a subject.

 * This function moves all entries into the list specified by \a list. Duplicate
 * entries in \a list get overwritten.

> + */
> +void DebugMetadata::moveEntries(ControlList &list)
> +{
> +	list.merge(std::move(cache_), ControlList::MergePolicy::OverwriteExisting);
> +	cache_.clear();
> +}
> +
> +/**
> + * \fn DebugMetadata::set(const Control<T> &ctrl, const V &value)
> + * \brief Set a value

 * \brief Set the debug metadata for \a ctrl to value \a value

> + * \param[in] ctrl The ctrl to set

s/ctrl/control/

> + * \param[in] value The control value
> + *
> + * Sets the debug metadata for \a ctrl to value \a value. If a parent is set,

Same here and below, "Sets" needs a subject. Or drop the first sentence
as it duplicates the \brief.

> + * the value gets passed there unconditionally. Otherwise it gets cached if the
> + * instance is enabled or dropped silently when disabled.
> + */
> +
> +/**
> + * \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 for \a id to value \a value. If a parent is set,
> + * the value gets passed there unconditionally. Otherwise it gets cached if the
> + * instance is enabled or dropped silently when disabled.
> + */
> +void DebugMetadata::set(unsigned int id, const ControlValue &value)
> +{
> +	if (parent_) {
> +		parent_->set(id, value);
> +		return;
> +	}
> +
> +	if (!enabled_)
> +		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