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

21540 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 v3:
- Rename checkForEnable by enableByControl
- Remove explicit template params in set
- Improve documentation

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/debug_controls.cpp            | 164 ++++++++++++++++++++
 src/libcamera/meson.build                   |   1 +
 5 files changed, 217 insertions(+)
 create mode 100644 include/libcamera/internal/debug_controls.h
 create mode 100644 src/libcamera/debug_controls.cpp

Quoting Stefan Klug (2024-10-08 16:29:40)
> 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 v3:
> - Rename checkForEnable by enableByControl
> - Remove explicit template params in set
> - Improve documentation
> 
> 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/debug_controls.cpp            | 164 ++++++++++++++++++++
>  src/libcamera/meson.build                   |   1 +
>  5 files changed, 217 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..0b049f48e246
> --- /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 enableByControl(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(ctrl, value);
> +                       return;
> +               }
> +
> +               if (!enabled_)
> +                       return;
> +
> +               cache_.set(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/debug_controls.cpp b/src/libcamera/debug_controls.cpp
> new file mode 100644
> index 000000000000..27a05592a97a
> --- /dev/null
> +++ b/src/libcamera/debug_controls.cpp
> @@ -0,0 +1,164 @@
> +/* 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
> + *
> + * Metadata is a useful tool for debugging the internal state of libcamera. It
> + * has the benefit that it is easy to use and related tooling is readily
> + * available. The difficulty is that the metadata control list is often not
> + * directly available (either because the variable to debug lives inside
> + * process() of an IPA or inside a closed algorithm class with no direct access
> + * to the ipa and therefore the metadata list).
> + *
> + * This class helps in both cases. It allows to forward the data to a parent or
> + * alternatively record the data and at a later point in time copy it to the
> + * metadata list when it becomes available. Both mechanisms allow easy reuse and
> + * loose coupling.
> + *
> + * The idea is to instantiate a DebugMetadata object in every class/algorithm
> + * where debug metadata shall be recorded (the inner object). If the IPA doesn't
> + * support debug metadata, the object is still usable, but the debug data gets
> + * dropped. If the IPA supports debug metadata it will either register a parent
> + * DebugMetadata object on the inner object or manually retrieve the data using
> + * enable()/moveToList().
> + *
> + * The concepts of forwarding to a parent and recording for later retrieval are
> + * mutually exclusive and the parent takes precedence. E.g. it is not allowed to
> + * enable a DebugMetadata object, log entries to it and later set the parent.
> + *
> + * This is done to keep the path open for using other means of data transport
> + * (like tracing). For every tracing event a corresponding context 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) also lives in a place where that
> + * information is also available.

Oh - that's a nice idea, so we can track this as a tracepoint for debug
data as it's centralised in the helper...

> + */
> +
> +/**
> + * \fn DebugMetadata::enableByControl()
> + * \brief Enable based on controls::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::enableByControl(const ControlList &controls)
> +{
> +       const auto &ctrl = controls.get(controls::DebugMetadataEnable);
> +       if (ctrl)
> +               enable(*ctrl);
> +}
> +
> +/**
> + * \fn DebugMetadata::enable()
> + * \brief Enable or disable metadata handling
> + * \param[in] enable The enable state
> + *
> + * When \a enable is true, all calls to set() get cached and can later be
> + * retrieved using 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 Set the parent metadata handler to \a parent
> + * \param[in] parent Pointer to the parent handler
> + *
> + * When a \a parent is set, all further calls to set() are unconditionally
> + * forwarded to that instance.
> + *
> + * 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 control list \a list
> + * \param[in] list The control list
> + *
> + * 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 the value of \a ctrl to \a value
> + * \param[in] ctrl The control to set
> + * \param[in] value The control 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.
> + *
> + * \sa enable()
> + */
> +
> +/**
> + * \fn DebugMetadata::set(unsigned int id, const ControlValue &value)
> + * \brief Set the value of control \a id to \a value
> + * \param[in] id The id of the control
> + * \param[in] value The control 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.
> + *
> + * \sa enable()
> + */
> +void DebugMetadata::set(unsigned int id, const ControlValue &value)
> +{

This is the only part that I'm not sure I understand,

Why do we set debug controls unconditionally on the parent if the
enabled_ is not set?

I would anticipate some of these debug statements are going to get
committed - and they should only be output when enabled with
DebugMetadataEnable?

But assuming you have specific reasons - and I can't find any blocker on
this (and we can always change that behaviour) so :


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> +       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',
> -- 
> 2.43.0
>

Hi Stefan,

Thank you for the patch.

On Thu, Oct 10, 2024 at 09:10:24AM +0100, Kieran Bingham wrote:
> Quoting Stefan Klug (2024-10-08 16:29:40)
> > 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 v3:
> > - Rename checkForEnable by enableByControl
> > - Remove explicit template params in set
> > - Improve documentation
> > 
> > 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/debug_controls.cpp            | 164 ++++++++++++++++++++
> >  src/libcamera/meson.build                   |   1 +
> >  5 files changed, 217 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..0b049f48e246
> > --- /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 enableByControl(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(ctrl, value);
> > +                       return;
> > +               }
> > +
> > +               if (!enabled_)
> > +                       return;
> > +
> > +               cache_.set(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/debug_controls.cpp b/src/libcamera/debug_controls.cpp
> > new file mode 100644
> > index 000000000000..27a05592a97a
> > --- /dev/null
> > +++ b/src/libcamera/debug_controls.cpp
> > @@ -0,0 +1,164 @@
> > +/* 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
> > + *
> > + * Metadata is a useful tool for debugging the internal state of libcamera. It
> > + * has the benefit that it is easy to use and related tooling is readily
> > + * available. The difficulty is that the metadata control list is often not
> > + * directly available (either because the variable to debug lives inside
> > + * process() of an IPA or inside a closed algorithm class with no direct access
> > + * to the ipa and therefore the metadata list).

s/ipa/IPA/

> > + *
> > + * This class helps in both cases. It allows to forward the data to a parent or
> > + * alternatively record the data and at a later point in time copy it to the
> > + * metadata list when it becomes available. Both mechanisms allow easy reuse and
> > + * loose coupling.
> > + *
> > + * The idea is to instantiate a DebugMetadata object in every class/algorithm

s/The idea/Typical usage/

> > + * where debug metadata shall be recorded (the inner object). If the IPA doesn't
> > + * support debug metadata, the object is still usable, but the debug data gets
> > + * dropped. If the IPA supports debug metadata it will either register a parent
> > + * DebugMetadata object on the inner object or manually retrieve the data using
> > + * enable()/moveToList().
> > + *
> > + * The concepts of forwarding to a parent and recording for later retrieval are
> > + * mutually exclusive and the parent takes precedence. E.g. it is not allowed to
> > + * enable a DebugMetadata object, log entries to it and later set the parent.
> > + *
> > + * This is done to keep the path open for using other means of data transport
> > + * (like tracing). For every tracing event a corresponding context 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) also lives in a place where that
> > + * information is also available.
> 
> Oh - that's a nice idea, so we can track this as a tracepoint for debug
> data as it's centralised in the helper...
> 
> > + */
> > +
> > +/**
> > + * \fn DebugMetadata::enableByControl()
> > + * \brief Enable based on controls::DebugMetadataEnable in the supplied
> > + * ControlList
> > + * \param[in] controls The supplied ControlList
> > + *
> > + * Looks for controls::DebugMetadataEnable and enables or disables debug

s/Looks/This function looks/

> > + * metadata handling accordingly.
> > + */
> > +void DebugMetadata::enableByControl(const ControlList &controls)
> > +{
> > +       const auto &ctrl = controls.get(controls::DebugMetadataEnable);
> > +       if (ctrl)
> > +               enable(*ctrl);
> > +}
> > +
> > +/**
> > + * \fn DebugMetadata::enable()
> > + * \brief Enable or disable metadata handling
> > + * \param[in] enable The enable state
> > + *
> > + * When \a enable is true, all calls to set() get cached and can later be
> > + * retrieved using 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 Set the parent metadata handler to \a parent
> > + * \param[in] parent Pointer to the parent handler

s/Pointer to the/The/

> > + *
> > + * When a \a parent is set, all further calls to set() are unconditionally
> > + * forwarded to that instance.
> > + *
> > + * 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 control list \a list
> > + * \param[in] list The control list
> > + *
> > + * 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 the value of \a ctrl to \a value
> > + * \param[in] ctrl The control to set
> > + * \param[in] value The control 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.
> > + *
> > + * \sa enable()
> > + */
> > +
> > +/**
> > + * \fn DebugMetadata::set(unsigned int id, const ControlValue &value)
> > + * \brief Set the value of control \a id to \a value
> > + * \param[in] id The id of the control
> > + * \param[in] value The control 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.
> > + *
> > + * \sa enable()
> > + */
> > +void DebugMetadata::set(unsigned int id, const ControlValue &value)
> > +{
> 
> This is the only part that I'm not sure I understand,
> 
> Why do we set debug controls unconditionally on the parent if the
> enabled_ is not set?
> 
> I would anticipate some of these debug statements are going to get
> committed - and they should only be output when enabled with
> DebugMetadataEnable?
> 
> But assuming you have specific reasons - and I can't find any blocker on
> this (and we can always change that behaviour) so :
> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > +       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',

Trivia time.

"I was first used in the 1st millennium BCE by Northwest Semitic scribes
using the abjad system. However, a range of other methods of classifying
and ordering material, including geographical, chronological,
hierarchical and by category, were preferred over me.

My first effective use as a cataloging device among scholars may have
been in ancient Alexandria, in the Great Library of Alexandria, which
was founded around 300 BCE. The poet and scholar Callimachus, who worked
there, is thought to have created the world's first library catalog,
known as the Pinakes."

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> >      'device_enumerator.cpp',
> >      'device_enumerator_sysfs.cpp',
> >      'dma_buf_allocator.cpp',