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

[Laurent Pinchart]

Hi Jean-Michel,

Thank you for the patch.

On Mon, Jul 12, 2021 at 03:16:29PM +0200, 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.
> 
> 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'
>  ])
>  
>  libipa_sources = files([
>      'algorithm.cpp',
>      'camera_sensor_helper.cpp',
> -    'histogram.cpp'
> +    'histogram.cpp',
> +    'metadata.cpp'
>  ])
>  
>  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
> + */
> +
> +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.
> + */
> +
> +/**
> + * \fn Metadata::Metadata(Metadata const &other)
> + * \param[in] other A Metadata object
> + *
> + * 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
> + *
> + * Sets the value in the map to the tag key. The mutex is
> + * taken for the duration of the block.
> + */
> +
> +/**
> + * \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.
> + */
> +
> +/**
> + * \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_;
> +	}
> +
> +	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_);

You can call getLocked() here.

> +		auto it = data_.find(tag);
> +		if (it == data_.end())
> +			return -1;
> +		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);

This is a bit dangerous, if T doesn't match the type stored for the tag.
It would of course be a bug in the caller, but if such code appears in
paths that are not frequently taken, it could cause issues that will
result in a crash at runtime later on.

Could we use a mechanism similar to Control and ControlList to ensure
the right case ?

template<typename T>
struct Tag {
	std::string tag_;
};

/* Static list of all tags. */
static constexpr Tag<Foo> tagFoo{ "foo" };

class Metadata
{
	...
	template<typename T>
	T *getLock(const Tag<T> &tag)
	{
		...
		return std::any_cast<T>(&it->second);
	}
	...
};
...

	Foo *f = metadata.get(tagFoo);

Furthermore, have you considered using integers instead of strings for
tags ? What are the pros and cons ?

> +	}
> +
> +	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_;
> +	std::map<std::string, std::any> data_;
> +};
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_IPA_LIBIPA_METADATA_H__ */

-- 
Regards,

Laurent Pinchart