[PATCH v2 2/7] libcamera: Add a DebugMetadata helper
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Oct 8 16:29:57 CEST 2024
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 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.
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',
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list