[PATCH v2 2/7] libcamera: Add a DebugMetadata helper
Stefan Klug
stefan.klug at ideasonboard.com
Tue Oct 8 15:33:21 CEST 2024
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 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.
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
More information about the libcamera-devel
mailing list