[PATCH v1 2/8] libcamera: Add a DebugMetadata helper
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Oct 7 23:48:13 CEST 2024
Hi Stefan,
On Fri, Oct 04, 2024 at 04:33:36PM +0200, Stefan Klug wrote:
> On Fri, Oct 04, 2024 at 12:23:05AM +0300, Laurent Pinchart wrote:
> > On Wed, Oct 02, 2024 at 06:19:20PM +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 lot's of boilerplate
> >
> > s/lot's/lots/
> >
> > > code. This can be mitigated by recording the metadata and forwarding it
> > > to an upstream helper or the metadata control list when they become
> > > available. Add such a helper.
> > >
> > > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > > ---
> > > include/libcamera/internal/debug_controls.h | 52 +++++++
> > > include/libcamera/internal/meson.build | 1 +
> > > src/libcamera/control_ids_core.yaml | 6 +
> > > src/libcamera/debug_controls.cpp | 144 ++++++++++++++++++++
> > > src/libcamera/meson.build | 1 +
> > > 5 files changed, 204 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..d95c03e1db1f
> > > --- /dev/null
> > > +++ b/include/libcamera/internal/debug_controls.h
> > > @@ -0,0 +1,52 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2024, Ideas on Board Oy
> > > + *
> > > + * 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 assignUpstream(DebugMetadata *upstream);
> > > + void assignControlList(ControlList *list);
> > > +
> > > + template<typename T, typename V>
> > > + void set(const Control<T> &ctrl, const V &value)
> > > + {
> > > + if (upstream_) {
> > > + upstream_->set<T, V>(ctrl, value);
> >
> > Do you need to explicitly mention <T, V> here, won't type deduction work
> > automatically ? Same below.
>
> Interesting question. I would say it should work. But I can't tell for
> sure if there could be cases where it doesn't work. By specifying it
> explicitly I ensure that the user of the function can specify it
> manually if needed. Can you say for sure, that there no cases
> where auto deduction fails?
I don't see how it could fail.
> > > + return;
> > > + }
> > > +
> > > + if (!enabled_)
> > > + return;
> > > +
> > > + if (list_) {
> > > + list_->set<T, V>(ctrl, value);
> > > + return;
> > > + }
> > > +
> > > + cache_.set<T, V>(ctrl, value);
> > > + }
> > > +
> > > + void set(unsigned int id, const ControlValue &value);
> > > +
> > > +private:
> > > + bool enabled_ = false;
> > > + ControlList *list_ = nullptr;
> > > + DebugMetadata *upstream_ = 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..103bcb593c4a 100644
> > > --- a/src/libcamera/control_ids_core.yaml
> > > +++ b/src/libcamera/control_ids_core.yaml
> > > @@ -967,5 +967,11 @@ 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.
> > > +
> >
> > Extra blank line.
> >
> > >
> > > ...
> > > diff --git a/src/libcamera/debug_controls.cpp b/src/libcamera/debug_controls.cpp
> > > new file mode 100644
> > > index 000000000000..11a92a6b5871
> > > --- /dev/null
> > > +++ b/src/libcamera/debug_controls.cpp
> > > @@ -0,0 +1,144 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2024, Ideas on Board Oy.
> > > + *
> > > + * Helper to easily record debug metadata inside libcamera.
> > > + */
> > > +
> > > +#include "libcamera/internal/debug_controls.h"
> > > +
> > > +namespace libcamera {
> > > +
> > > +/**
> > > + * \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
> > > + * \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)
> > > +{
> > > + const auto &ctrl = controls.get(controls::DebugMetadataEnable);
> > > + if (ctrl)
> > > + enable(*ctrl);
> > > +}
> > > +
> > > +/**
> > > + * \fn DebugMetadata::enable
> > > + * \brief Enables or disabled metadata handling
> > > + * \param[in] enable The enable state
> > > + *
> > > + * Enables or disables metadata handling according to \a enable. When enable is
> > > + * false, the cache gets cleared and no further metadata is recorded.
> > > + */
> > > +void DebugMetadata::enable(bool enable)
> > > +{
> > > + enabled_ = enable;
> > > + if (!enabled_)
> > > + cache_.clear();
> > > +}
> > > +
> > > +/**
> > > + * \fn DebugMetadata::assignUpstream
> > > + * \brief Assign an upstream metadata handler
> > > + * \param[in] upstream Pointer to the upstream handler
> > > + *
> > > + * When a upstream gets set, the cache is copies to that list and all further
> >
> > s/copies/copied/
> >
> > > + * calls to DebugMetadata::set are forwarded to that list.
> > > + *
> > > + * The upstream can be reset by passing a nullptr.
> >
> > I think "parent" is a better name than "upstream", the parent concept is
> > widely used. I'd call the function setParent() as we use "set" rather
> > than "assign".
>
> As discussed, I will change that.
>
> > > + */
> > > +void DebugMetadata::assignUpstream(DebugMetadata *upstream)
> > > +{
> > > + upstream_ = upstream;
> > > +
> > > + if (!upstream_)
> > > + return;
> > > +
> > > + for (const auto &ctrl : cache_)
> > > + upstream_->set(ctrl.first, ctrl.second);
> > > +
> > > + cache_.clear();
> > > +}
> > > +
> > > +/**
> > > + * \fn DebugMetadata::assignControlList
> > > + * \brief Assign a control list
> > > + * \param[in] list Pointer to the list
> > > + *
> > > + * When a list gets set, the cache is copies to that list and all further calls
> >
> > s/copies/copied/
> >
> > > + * to DebugMetadata::set are forwarded to that list.
> > > + *
> > > + * The upstream can be reset by passing a nullptr.
> >
> > s/upstream/list/
> >
> > > + */
> > > +void DebugMetadata::assignControlList(ControlList *list)
> > > +{
> > > + list_ = list;
> > > +
> > > + if (list_) {
> > > + list_->merge(cache_);
> > > + cache_.clear();
> > > + }
> > > +}
> > > +
> > > +/**
> > > + * \fn DebugMetadata::set(const Control<T> &ctrl, const V &value)
> > > + * \brief Set a value
> > > + * \param[in] ctrl The ctrl to set
> > > + * \param[in] value The control value
> > > + *
> > > + * Sets the debug metadata for ctrl to value \a value. If an upstream or a
> > > + * control list is set, the value gets passed there (upstream takes precedence).
> > > + * Otherwise the value is cached until one of them gets assigned.
> > > + *
> > > + * When the instance is disabled and upstream is not set, the value gets dropped.
> > > + */
> > > +
> > > +/**
> > > + * \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 to value \a value. If an upstream or a control list
> > > + * is set, the value gets passed there (upstream takes precedence).
> > > + * Otherwise the value is cached until one of them gets assigned.
> > > + *
> > > + * When the instance is disabled and upstream is not set, the value gets dropped.
> > > + */
> > > +void DebugMetadata::set(unsigned int id, const ControlValue &value)
> > > +{
> > > + if (upstream_) {
> > > + upstream_->set(id, value);
> > > + return;
> > > + }
> > > +
> > > + if (!enabled_)
> > > + return;
> > > +
> > > + if (list_) {
> > > + list_->set(id, value);
> > > + 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