[libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata class

[Kieran Bingham]

Hi JM,

On 12/07/2021 14:16, Jean-Michel Hautbois wrote:
> The Metadata class comes from RPi from which a bit has been removed
> because we don't need it for now.
> All functions are inlined in metadata.h because of the template usage.

Perhaps this could be better worded:

"""
Import the Metadata class from src/ipa/raspberrypi/metadata.cpp to be
able to make use of the component from other IPA modules.
"""


> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3.cpp       |   1 +
>  src/ipa/libipa/meson.build  |   6 ++-
>  src/ipa/libipa/metadata.cpp | 101 ++++++++++++++++++++++++++++++++++++
>  src/ipa/libipa/metadata.h   |  90 ++++++++++++++++++++++++++++++++
>  4 files changed, 196 insertions(+), 2 deletions(-)
>  create mode 100644 src/ipa/libipa/metadata.cpp
>  create mode 100644 src/ipa/libipa/metadata.h
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 71698d36..091856f5 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -25,6 +25,7 @@
>  #include "ipu3_agc.h"
>  #include "ipu3_awb.h"
>  #include "libipa/camera_sensor_helper.h"
> +#include "libipa/metadata.h"
>  
>  static constexpr uint32_t kMaxCellWidthPerSet = 160;
>  static constexpr uint32_t kMaxCellHeightPerSet = 56;
> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> index 3fda7c00..cc4e1cc9 100644
> --- a/src/ipa/libipa/meson.build
> +++ b/src/ipa/libipa/meson.build
> @@ -3,13 +3,15 @@
>  libipa_headers = files([
>      'algorithm.h',
>      'camera_sensor_helper.h',
> -    'histogram.h'
> +    'histogram.h',
> +    'metadata.h'

If you keep a ',' at the end, you don't to modify this line when adding
another entry later.


>  ])
>  
>  libipa_sources = files([
>      'algorithm.cpp',
>      'camera_sensor_helper.cpp',
> -    'histogram.cpp'
> +    'histogram.cpp',
> +    'metadata.cpp'

Same here...


>  ])
>  
>  libipa_includes = include_directories('..')
> diff --git a/src/ipa/libipa/metadata.cpp b/src/ipa/libipa/metadata.cpp
> new file mode 100644
> index 00000000..b6aef897
> --- /dev/null
> +++ b/src/ipa/libipa/metadata.cpp
> @@ -0,0 +1,101 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Based on the implementation from the Raspberry Pi IPA,
> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.
> + * Copyright (C) 2021, Ideas On Board
> + *
> + * metadata.cpp -  libipa metadata class
> + */
> +
> +#include "metadata.h"
> +
> +/**
> + * \file metadata.h
> + * \brief A metadata class to share objects

I wouldn't call it sharing objects...

"A metadata class to provide key based access to arbitrary metadata types.


> + */
> +
> +namespace libcamera {
> +
> +namespace ipa {
> +
> +/**
> + * \class Metadata
> + * \brief A simple class for carrying arbitrary metadata, for example
> + * about an image. It is used to exchange data between algorithms.

I think we need to expand on this to explain some of the constraints too:


"""
Data is stored as a map with a string based key.

The metadata is stored through a std::any() type which is definable by
the user, and must be correctly known by both the producer and consumer.

Accessing the metadata with an incorrect type will cause undefined
behaviour.
"""

Some of that might be too far towards the implementation details, but in
this instance, I sort of think those implementation details are
important because the implications they introduce.



> + */
> +
> +/**
> + * \fn Metadata::Metadata(Metadata const &other)
> + * \param[in] other A Metadata object
> + *

I think this is the copy constructor right?

Lets reference it so:

"Copy the data from the \a other Metadata object to this one."



> + * Stores the data from one Metadata to another one
> + */
> +
> +/**
> + * \fn Metadata::set(std::string const &tag, T const &value)
> + * \param[in] tag A string used as the key in a map
> + * \param[in] value The value to set into the map

I would probably drop 'in a map' and 'into the map' in these parameter
descriptions.


> + *
> + * Sets the value in the map to the tag key. The mutex is
> + * taken for the duration of the block.


I would express this as..

"This function locks the metadata to protect from concurrent access"

(rather than "the mutex is...", and on a line/paragraph of its own to
stand out.)


> + */
> +
> +/**
> + * \fn Metadata::get(std::string const &tag, T &value)
> + * \param[in] tag A string used as the key in a map
> + * \param[in] value The value to set into the map
> + *
> + * Gets the value in the map of the tag key. The mutex is
> + * taken for the duration of the block.
> + *
> + * \return 0 if value is found, -1 if not existent
> + */
> +
> +/**
> + * \fn Metadata::clear()
> + * Clear the Metadata map. The mutex is taken for the duration of
> + * the block.
> + */
> +
> +/**
> + * \fn Metadata::merge(Metadata &other)
> + * \param[in] other A metadata to merge with
> + * Merge two Metadata maps. The mutex is taken for the duration of
> + * the block.
> + */
> +
> +/**
> + * \fn Metadata::getLocked(std::string const &tag)
> + * \param[in] tag A string used as the key in a map
> + *
> + * Get the value of the tag key in the map.
> + * This allows in-place access to the Metadata contents,
> + * for which you should be holding the lock.

I would expand upon this to also state how to lock it, given that it is
part of the API of this class:


"""
This function does not protect against concurrent access, and it is up
to the caller to ensure that the lock is held using \a lock()
"""

> + */
> +
> +/**
> + * \fn Metadata::setLocked(std::string const &tag, T const &value)
> + * \param[in] tag A string used as the key in a map
> + * \param[in] value The value to set into the map
> + *
> + * Set the value to the tag key in the map.
> + * This allows in-place access to the Metadata contents,
> + * for which you should be holding the lock.
> + */
> +
> +/**
> + * \fn Metadata::lock()
> + * Lock the mutex with the standard classes.
> + * e.g. std::lock_guard<RPiController::Metadata> lock(metadata)
> + */
> +
> +/**
> + * \fn Metadata::unlock()
> + * Unlock the mutex with the standard classes.
> + * e.g. std::lock_guard<RPiController::Metadata> lock(metadata)
> + */
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> +
> diff --git a/src/ipa/libipa/metadata.h b/src/ipa/libipa/metadata.h
> new file mode 100644
> index 00000000..9801bece
> --- /dev/null
> +++ b/src/ipa/libipa/metadata.h
> @@ -0,0 +1,90 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Based on the implementation from the Raspberry Pi IPA,
> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.
> + * Copyright (C) 2021, Ideas On Board
> + *
> + * metadata.h - libipa metadata class
> + */
> +#ifndef __LIBCAMERA_IPA_LIBIPA_METADATA_H__
> +#define __LIBCAMERA_IPA_LIBIPA_METADATA_H__
> +
> +#include <any>
> +#include <map>
> +#include <memory>
> +#include <mutex>
> +#include <string>
> +
> +namespace libcamera {
> +
> +namespace ipa {
> +
> +class Metadata
> +{
> +public:
> +	Metadata() = default;
> +
> +	Metadata(Metadata const &other)
> +	{
> +		std::scoped_lock other_lock(other.mutex_);
> +		data_ = other.data_;
> +	}

The C++ rule of five (or is it six?) says if you need to implement one
of the copy/move constructors you need to implement, default (or
delete?) them all:

https://www.modernescpp.com/index.php/c-core-guidelines-constructors-assignments-and-desctructors

So we should add appropriate copy assignment, and move constructors with
appropriate locking ... (or delete them if they shouldn't be allowed).


I think the constructor and destructor can be = default though, I don't
see anything specific to handle there?



> +
> +	template<typename T>
> +	void set(std::string const &tag, T const &value)
> +	{
> +		std::scoped_lock lock(mutex_);
> +		data_[tag] = value;
> +	}
> +
> +	template<typename T>
> +	int get(std::string const &tag, T &value) const
> +	{
> +		std::scoped_lock lock(mutex_);
> +		auto it = data_.find(tag);
> +		if (it == data_.end())
> +			return -1;

Does std::any provide any way to get the template type of the object so
we can assert with an incorrect access?

Perhaps that would then require specific RTTI which maybe we don't want
to get into anyway though...


> +		value = std::any_cast<T>(it->second);
> +		return 0;
> +	}
> +
> +	void clear()
> +	{
> +		std::scoped_lock lock(mutex_);
> +		data_.clear();
> +	}
> +
> +	void merge(Metadata &other)
> +	{
> +		std::scoped_lock lock(mutex_, other.mutex_);
> +		data_.merge(other.data_);
> +	}
> +
> +	template<typename T>
> +	T *getLocked(std::string const &tag)
> +	{
> +		auto it = data_.find(tag);
> +		if (it == data_.end())
> +			return nullptr;
> +		return std::any_cast<T>(&it->second);
> +	}
> +
> +	template<typename T>
> +	void setLocked(std::string const &tag, T const &value)
> +	{
> +		data_[tag] = value;
> +	}
> +
> +	void lock() { mutex_.lock(); }
> +	void unlock() { mutex_.unlock(); }
> +
> +private:
> +	mutable std::mutex mutex_;

Hrm, I had to look this up. I wonder if we should be using mutable more
often.

But indeed, this sounds right as it means you can 'get' from a const
Metadata and lock the mutex (which requires modifying the otherwise
const mutex).



> +	std::map<std::string, std::any> data_;
> +};
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_IPA_LIBIPA_METADATA_H__ */
>