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

[Kieran Bingham]

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