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

21525 diff mbox series

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@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

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@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',

Hi Laurent,

On Tue, Oct 08, 2024 at 01:05:44AM +0300, Laurent Pinchart wrote:
> 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@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.

Ok, I convinced myself, that there is no way this could go wrong (also
for the generic case). So I'll remove them.

> 
> > +			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 ?

Oh, yes. This ended up in the wrong 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.

What about the following:

1. Just an overload enable(ControlList&). Looks a bit weird when used:
debugMeta.enable(list)

2. enableByControl(ControlList&). I think that is my favourite.

> 
> > +{
> > +	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).

ack

> 
> > + * 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.
> 

I rewrote parts of it. Let's see if v3 is clearer :-)
The comments below where also fixed during that rework.

Regards,
Stefan

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

On Tue, Oct 08, 2024 at 03:33:21PM +0200, Stefan Klug wrote:
> Hi Laurent,
> 
> On Tue, Oct 08, 2024 at 01:05:44AM +0300, Laurent Pinchart wrote:
> > 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@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.
> 
> Ok, I convinced myself, that there is no way this could go wrong (also
> for the generic case). So I'll remove them.
> 
> > 
> > > +			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 ?
> 
> Oh, yes. This ended up in the wrong 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.
> 
> What about the following:
> 
> 1. Just an overload enable(ControlList&). Looks a bit weird when used:
> debugMeta.enable(list)
> 
> 2. enableByControl(ControlList&). I think that is my favourite.

I can't think of a better name, so I'm OK with this. I wonder however if
we shouldn't drop this function and call controls.get() in the caller.

> > > +{
> > > +	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).
> 
> ack
> 
> > > + * 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.
> 
> I rewrote parts of it. Let's see if v3 is clearer :-)
> The comments below where also fixed during that rework.
> 
> > > + *
> > > + * 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',