[PATCH v1 2/8] libcamera: Add a DebugMetadata helper

[Stefan Klug]

Hi Laurent,

Thank you for the review. 

On Fri, Oct 04, 2024 at 12:23:05AM +0300, Laurent Pinchart wrote:
> Hi Stefan,
> 
> Thank you for the patch.
> 
> 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?

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