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

[Jean-Michel Hautbois]

Hi Kieran,

Thanks for the review :-).

On 13/07/2021 16:41, Kieran Bingham wrote:
> 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?
> 

OK, I will implement those.

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

Something like that ?
https://en.cppreference.com/w/cpp/utility/any/type

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

I am not sure if it is worth it...

> 
>> +		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__ */
>>