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

12912 diff mbox series

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

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

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

Hi JM,

On 14/07/2021 07:59, Jean-Michel Hautbois wrote:
> 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@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.

When you do this, please try to be clear in the story your patches tell.

This is clearly additional code on top of the code which is imported, so
the series should do something like:

 1/x ipa: libipa: Import Metadata class from src/ipa/raspberrypi
    Just the direct copy, without any specific changes other than making
    it compile in place

 2/x ipa: libipa: Document Metadata class
    Any documentation that /you/ add.

 3/x ipa: libipa: Implement missing Metadata class copy/move operations
    Any code additions that you add

 4/x .... other?


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

On 14/07/2021 10:12, Kieran Bingham wrote:
> Hi JM,
> 
> On 14/07/2021 07:59, Jean-Michel Hautbois wrote:
>> 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@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.
> 
> When you do this, please try to be clear in the story your patches tell.
> 
> This is clearly additional code on top of the code which is imported, so
> the series should do something like:
> 
>  1/x ipa: libipa: Import Metadata class from src/ipa/raspberrypi
>     Just the direct copy, without any specific changes other than making
>     it compile in place

Doxygen documentation needed to make it compile without warnings is not
considered in this first patch ?

>  2/x ipa: libipa: Document Metadata class
>     Any documentation that /you/ add.
> 
>  3/x ipa: libipa: Implement missing Metadata class copy/move operations
>     Any code additions that you add
> 
>  4/x .... other?
> 
> 
>>
>>>
>>>> +
>>>> +	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__ */
>>>>

Hi Kieran and JM,

On Wed, 14 Jul 2021 at 09:12, Kieran Bingham <
kieran.bingham@ideasonboard.com> wrote:

> Hi JM,
>
> On 14/07/2021 07:59, Jean-Michel Hautbois wrote:
> > 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@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.
>
> When you do this, please try to be clear in the story your patches tell.
>

You could re-use the existing copy/move constructors and operators from our
implementation
for these.

Regards,
Naush



>
> This is clearly additional code on top of the code which is imported, so
> the series should do something like:
>
>  1/x ipa: libipa: Import Metadata class from src/ipa/raspberrypi
>     Just the direct copy, without any specific changes other than making
>     it compile in place
>
>  2/x ipa: libipa: Document Metadata class
>     Any documentation that /you/ add.
>
>  3/x ipa: libipa: Implement missing Metadata class copy/move operations
>     Any code additions that you add
>
>  4/x .... other?
>
>
> >
> >>
> >>> +
> >>> +   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__ */
> >>>
>

Hi Naush,

On 14/07/2021 10:45, Naushir Patuck wrote:
> Hi Kieran and JM,
> 
> On Wed, 14 Jul 2021 at 09:12, Kieran Bingham
> <kieran.bingham@ideasonboard.com
> <mailto:kieran.bingham@ideasonboard.com>> wrote:
> 
>     Hi JM,
> 
>     On 14/07/2021 07:59, Jean-Michel Hautbois wrote:
>     > 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@ideasonboard.com
>     <mailto:jeanmichel.hautbois@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
>     <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.
> 
>     When you do this, please try to be clear in the story your patches tell.
> 
> 
> You could re-use the existing copy/move constructors and operators from
> our implementation
> for these.

Yes, sorry about that... :-(.
You don't have a destructor though ?

> Regards,
> Naush
> 
>  
> 
> 
>     This is clearly additional code on top of the code which is imported, so
>     the series should do something like:
> 
>      1/x ipa: libipa: Import Metadata class from src/ipa/raspberrypi
>         Just the direct copy, without any specific changes other than making
>         it compile in place
> 
>      2/x ipa: libipa: Document Metadata class
>         Any documentation that /you/ add.
> 
>      3/x ipa: libipa: Implement missing Metadata class copy/move operations
>         Any code additions that you add
> 
>      4/x .... other?
> 
> 
>     >
>     >>
>     >>> +
>     >>> +   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
>     <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__ */
>     >>>
>

Hi Kieran,

On Wed, Jul 14, 2021 at 09:12:17AM +0100, Kieran Bingham wrote:
> On 14/07/2021 07:59, Jean-Michel Hautbois wrote:
> > On 13/07/2021 16:41, Kieran Bingham wrote:
> >> 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@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.
> 
> When you do this, please try to be clear in the story your patches tell.
> 
> This is clearly additional code on top of the code which is imported, so
> the series should do something like:
> 
>  1/x ipa: libipa: Import Metadata class from src/ipa/raspberrypi
>     Just the direct copy, without any specific changes other than making
>     it compile in place
> 
>  2/x ipa: libipa: Document Metadata class
>     Any documentation that /you/ add.
> 
>  3/x ipa: libipa: Implement missing Metadata class copy/move operations
>     Any code additions that you add
> 
>  4/x .... other?

I would have done it the other way around to be honest. A single patch
with a new implementation would be easier to review. We need to consider
all the design assumptions and see if they match the use cases we
envision for other IPA modules. Copying the existing code as-is will
result in the first patch not being reviewed from a design point of
view.

I haven't looked at this in details, but I want to check the locking
implementation in particular. I also think integer tags would be more
efficient (but there could be drawbacks that we should discuss). I'm
sure I'll have more comments when I'll review the code in details :-)

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

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

Hi Laurent,

On 14/07/2021 14:48, Laurent Pinchart wrote:
> 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@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.
> 
Indeed :-)

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

I think it may be a good idea. Pros I can see:
- easy to document the tags
- readability (we can always refer to the enum to know how a particular
object is mapped).
- fastest to find the tag
Cons, not much:
- if RPi wants to switch to the libipa Metadata class they will need to
rewrite a not that small piece of code.
- having integer would make the code less dynamic, as we would certainly
be tempted to create a static map (which can also be seen as a pro :-)).

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

On Wed, Jul 14, 2021 at 02:57:11PM +0200, Jean-Michel Hautbois wrote:
> Hi Laurent,
> 
> On 14/07/2021 14:48, Laurent Pinchart wrote:
> > 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@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.
> > 
> Indeed :-)
> 
> >> +		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 ?
> 
> I think it may be a good idea. Pros I can see:
> - easy to document the tags
> - readability (we can always refer to the enum to know how a particular
> object is mapped).
> - fastest to find the tag
> Cons, not much:
> - if RPi wants to switch to the libipa Metadata class they will need to
> rewrite a not that small piece of code.

We can also help ;-)

> - having integer would make the code less dynamic, as we would certainly
> be tempted to create a static map (which can also be seen as a pro :-)).

That's my main question. Do you foresee a need to have some sort of
namespace in tags ? Will the data stored here be specific to each IPA
module, or would there be some level of standardization ? If we want to
create an IPA module from a set of algorithms, who will be responsible
for accessing the storage, would that be code specific to that IPA
module, and/or code from libipa ? There's lots of design questions, I
have little visibility on what you're planning.

The idea of using a template Tag structure as described above is
orthogonal to the decision of using strings or integers as identifiers.
Tags would however need to be defined in headers, which I think is an
upside, as we can enforce documentation in that case. I don't know if it
would be feasible (or even a good idea) to centralize all the tag
definitions though, they could be spread across different headers (some
being device-specific, and some being maybe shared accross different IPA
modules).

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

Hi,


On Wed, 14 Jul 2021 at 18:07, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> On Wed, Jul 14, 2021 at 02:57:11PM +0200, Jean-Michel Hautbois wrote:
> > Hi Laurent,
> >
> > On 14/07/2021 14:48, Laurent Pinchart wrote:
> > > 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@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.
> > >
> > Indeed :-)
> >
> > >> +          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 ?
> >
> > I think it may be a good idea. Pros I can see:
> > - easy to document the tags
> > - readability (we can always refer to the enum to know how a particular
> > object is mapped).
> > - fastest to find the tag
> > Cons, not much:
> > - if RPi wants to switch to the libipa Metadata class they will need to
> > rewrite a not that small piece of code.
>

What is being described here seems suspiciously similar to a ControlList :-)
Perhaps for your usage, that is more appropriate over our Metadata object?

Regards,
Naush


>
> We can also help ;-)
>
> > - having integer would make the code less dynamic, as we would certainly
> > be tempted to create a static map (which can also be seen as a pro :-)).
>
> That's my main question. Do you foresee a need to have some sort of
> namespace in tags ? Will the data stored here be specific to each IPA
> module, or would there be some level of standardization ? If we want to
> create an IPA module from a set of algorithms, who will be responsible
> for accessing the storage, would that be code specific to that IPA
> module, and/or code from libipa ? There's lots of design questions, I
> have little visibility on what you're planning.
>
> The idea of using a template Tag structure as described above is
> orthogonal to the decision of using strings or integers as identifiers.
> Tags would however need to be defined in headers, which I think is an
> upside, as we can enforce documentation in that case. I don't know if it
> would be feasible (or even a good idea) to centralize all the tag
> definitions though, they could be spread across different headers (some
> being device-specific, and some being maybe shared accross different IPA
> modules).
>
> > >> +  }
> > >> +
> > >> +  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
>

Hi Naush,

On Wed, Jul 14, 2021 at 06:38:42PM +0100, Naushir Patuck wrote:
> On Wed, 14 Jul 2021 at 18:07, Laurent Pinchart wrote:
> > On Wed, Jul 14, 2021 at 02:57:11PM +0200, Jean-Michel Hautbois wrote:
> > > On 14/07/2021 14:48, Laurent Pinchart wrote:
> > > > 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@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.
> > > >
> > > Indeed :-)
> > >
> > > >> +          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 ?
> > >
> > > I think it may be a good idea. Pros I can see:
> > > - easy to document the tags
> > > - readability (we can always refer to the enum to know how a particular
> > > object is mapped).
> > > - fastest to find the tag
> > > Cons, not much:
> > > - if RPi wants to switch to the libipa Metadata class they will need to
> > > rewrite a not that small piece of code.
> 
> What is being described here seems suspiciously similar to a ControlList :-)

A bit indeed, but with an std::any instead of a small set of supported
types, and no support for ControlInfo. It's "just" a way to ensure we
won't get the type T wrong in a call.

> Perhaps for your usage, that is more appropriate over our Metadata object?

Do you think the usage in the Raspberry Pi IPA differs significantly ?

> > We can also help ;-)
> >
> > > - having integer would make the code less dynamic, as we would certainly
> > > be tempted to create a static map (which can also be seen as a pro :-)).
> >
> > That's my main question. Do you foresee a need to have some sort of
> > namespace in tags ? Will the data stored here be specific to each IPA
> > module, or would there be some level of standardization ? If we want to
> > create an IPA module from a set of algorithms, who will be responsible
> > for accessing the storage, would that be code specific to that IPA
> > module, and/or code from libipa ? There's lots of design questions, I
> > have little visibility on what you're planning.
> >
> > The idea of using a template Tag structure as described above is
> > orthogonal to the decision of using strings or integers as identifiers.
> > Tags would however need to be defined in headers, which I think is an
> > upside, as we can enforce documentation in that case. I don't know if it
> > would be feasible (or even a good idea) to centralize all the tag
> > definitions though, they could be spread across different headers (some
> > being device-specific, and some being maybe shared accross different IPA
> > modules).
> >
> > > >> +  }
> > > >> +
> > > >> +  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__ */

Hi Laurent,

On Wed, 14 Jul 2021 at 18:43, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hi Naush,
>
> On Wed, Jul 14, 2021 at 06:38:42PM +0100, Naushir Patuck wrote:
> > On Wed, 14 Jul 2021 at 18:07, Laurent Pinchart wrote:
> > > On Wed, Jul 14, 2021 at 02:57:11PM +0200, Jean-Michel Hautbois wrote:
> > > > On 14/07/2021 14:48, Laurent Pinchart wrote:
> > > > > 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@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.
> > > > >
> > > > Indeed :-)
> > > >
> > > > >> +          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 ?
> > > >
> > > > I think it may be a good idea. Pros I can see:
> > > > - easy to document the tags
> > > > - readability (we can always refer to the enum to know how a
> particular
> > > > object is mapped).
> > > > - fastest to find the tag
> > > > Cons, not much:
> > > > - if RPi wants to switch to the libipa Metadata class they will need
> to
> > > > rewrite a not that small piece of code.
> >
> > What is being described here seems suspiciously similar to a ControlList
> :-)
>
> A bit indeed, but with an std::any instead of a small set of supported
> types, and no support for ControlInfo. It's "just" a way to ensure we
> won't get the type T wrong in a call.
>

I was thinking along the lines of using ControlValues of type Span<uint8_t>
like
we do in the IPA to set ISP controls. It does not exactly have the
convenience
of std::any usage, but does allow arbitrary structs to be placed into the
ControlList.


>
> > Perhaps for your usage, that is more appropriate over our Metadata
> object?


> Do you think the usage in the Raspberry Pi IPA differs significantly ?
>

For RPi, we like the convenience and ease of use for the existing method.
You can just call set/get without any ancillary setup, which is a big
reason we
chose strings over enums for key types.  This code predated our IPA, so
we used to take care of std::any type mismatches using exceptions, which
were
removed when we ported our code across. Without this, you do need to be
somewhat careful not to trip over.

Other than that, usage wise, I expect things to be similar to what JM
intends, but will
have to wait and see.  If this is indeed the case, I expect all key/value
pairs to be very
specific to each IPA and doubt  much (if any) could be shared across
various vendors.
Because of this, and the likelihood that these structures will only be used
internally to
the IPA, do you see auto-generated documentation to be required?  Of course
the structs
and fields must be documented in the source files :-)

Regards,
Naush


>
> > > We can also help ;-)
> > >
> > > > - having integer would make the code less dynamic, as we would
> certainly
> > > > be tempted to create a static map (which can also be seen as a pro
> :-)).
> > >
> > > That's my main question. Do you foresee a need to have some sort of
> > > namespace in tags ? Will the data stored here be specific to each IPA
> > > module, or would there be some level of standardization ? If we want to
> > > create an IPA module from a set of algorithms, who will be responsible
> > > for accessing the storage, would that be code specific to that IPA
> > > module, and/or code from libipa ? There's lots of design questions, I
> > > have little visibility on what you're planning.
> > >
> > > The idea of using a template Tag structure as described above is
> > > orthogonal to the decision of using strings or integers as identifiers.
> > > Tags would however need to be defined in headers, which I think is an
> > > upside, as we can enforce documentation in that case. I don't know if
> it
> > > would be feasible (or even a good idea) to centralize all the tag
> > > definitions though, they could be spread across different headers (some
> > > being device-specific, and some being maybe shared accross different
> IPA
> > > modules).
> > >
> > > > >> +  }
> > > > >> +
> > > > >> +  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
>

Hi Naush,

On Wed, Jul 14, 2021 at 07:40:51PM +0100, Naushir Patuck wrote:
> On Wed, 14 Jul 2021 at 18:43, Laurent Pinchart wrote:
> > On Wed, Jul 14, 2021 at 06:38:42PM +0100, Naushir Patuck wrote:
> > > On Wed, 14 Jul 2021 at 18:07, Laurent Pinchart wrote:
> > > > On Wed, Jul 14, 2021 at 02:57:11PM +0200, Jean-Michel Hautbois wrote:
> > > > > On 14/07/2021 14:48, Laurent Pinchart wrote:
> > > > > > 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@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.
> > > > >
> > > > > Indeed :-)
> > > > >
> > > > > >> +          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 ?
> > > > >
> > > > > I think it may be a good idea. Pros I can see:
> > > > > - easy to document the tags
> > > > > - readability (we can always refer to the enum to know how a particular
> > > > > object is mapped).
> > > > > - fastest to find the tag
> > > > > Cons, not much:
> > > > > - if RPi wants to switch to the libipa Metadata class they will need to
> > > > > rewrite a not that small piece of code.
> > >
> > > What is being described here seems suspiciously similar to a ControlList
> > :-)
> >
> > A bit indeed, but with an std::any instead of a small set of supported
> > types, and no support for ControlInfo. It's "just" a way to ensure we
> > won't get the type T wrong in a call.
> 
> I was thinking along the lines of using ControlValues of type Span<uint8_t> like
> we do in the IPA to set ISP controls. It does not exactly have the convenience
> of std::any usage, but does allow arbitrary structs to be placed into the
> ControlList.

That's right, but we then lose type-safety, with a possible buffer
overflow or just misinterpretation of data, and that's even harder to
debug than an uncaught exception being thrown in uncommon cases.

> > > Perhaps for your usage, that is more appropriate over our Metadata object?
> >
> > Do you think the usage in the Raspberry Pi IPA differs significantly ?
> 
> For RPi, we like the convenience and ease of use for the existing method.
> You can just call set/get without any ancillary setup, which is a big reason we
> chose strings over enums for key types.  This code predated our IPA, so
> we used to take care of std::any type mismatches using exceptions, which were
> removed when we ported our code across. Without this, you do need to be
> somewhat careful not to trip over.

Type mismatch are essentially programming errors. Dealing with them by
catching exceptions can help recovering gracefully, but isn't it best to
catch those errors at compile time ?

> Other than that, usage wise, I expect things to be similar to what JM intends, but will
> have to wait and see.  If this is indeed the case, I expect all key/value pairs to be very
> specific to each IPA and doubt  much (if any) could be shared across various vendors.

That's my expectations too. I'm thinking some keys may become shared
across vendors, there's some data that may be common enough to make this
possible, but the devil is in the details and I have a feeling that the
interoperability this could bring (by using some common pieces in
different IPA modules) is just a dream. Let's see how it turns out, but
it's probably more pain than gain.

I'd like to experiment with the template Tag class (name to be
bikeshedded) to see how it turns out. If we go in that direction, string
vs. integer becomes a moot point as the tag value will be internal. And
if it turns out it's not a good idea, then we'll do something else :-)

> Because of this, and the likelihood that these structures will only be used internally to
> the IPA, do you see auto-generated documentation to be required?  Of course the structs
> and fields must be documented in the source files :-)

No, I wouldn't generate documentation out of this. We should generate
documentation for shared code in libipa, but not for module-specific
code. Having internal documentation in the form of comments is of course
good, to help others understand the code, and increase maintainability.

> > > > We can also help ;-)
> > > >
> > > > > - having integer would make the code less dynamic, as we would certainly
> > > > > be tempted to create a static map (which can also be seen as a pro :-)).
> > > >
> > > > That's my main question. Do you foresee a need to have some sort of
> > > > namespace in tags ? Will the data stored here be specific to each IPA
> > > > module, or would there be some level of standardization ? If we want to
> > > > create an IPA module from a set of algorithms, who will be responsible
> > > > for accessing the storage, would that be code specific to that IPA
> > > > module, and/or code from libipa ? There's lots of design questions, I
> > > > have little visibility on what you're planning.
> > > >
> > > > The idea of using a template Tag structure as described above is
> > > > orthogonal to the decision of using strings or integers as identifiers.
> > > > Tags would however need to be defined in headers, which I think is an
> > > > upside, as we can enforce documentation in that case. I don't know if it
> > > > would be feasible (or even a good idea) to centralize all the tag
> > > > definitions though, they could be spread across different headers (some
> > > > being device-specific, and some being maybe shared accross different IPA
> > > > modules).
> > > >
> > > > > >> +  }
> > > > > >> +
> > > > > >> +  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__ */

Hi Laurent, Naush

Just to add my own two cents worth...

Firstly, I like strings as tags. This will be no surprise as we also
use them as (for example) metering mode names!! But I think it relates
at least partly to how easy we want to make it for folks to experiment
and try their own things - which we've always wanted to encourage.

This is true in the metadata too. Our documentation is quite clear how
we expect people to use certain specific tags ("algo.status") if they
want to play nicely with other RPi algorithms. But they can implement
their own algorithms entirely.

In particular, they can add their own private metadata to frames
("foo.algo.private" for example). I think this will become more
important on future platforms that run less synchronously, where
Process won't be able to assume that the last time Prepare ran, it was
for the same frame.

We also have a rule that the parameters that have been set in the
algorithm must appear in the status object, so you can sit back and
watch frames until the parameters you asked for have taken effect.
This whole area is of course very difficult - as was apparent in the
Khronos group discussion the other week.

David

On Thu, 15 Jul 2021 at 00:24, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Naush,
>
> On Wed, Jul 14, 2021 at 07:40:51PM +0100, Naushir Patuck wrote:
> > On Wed, 14 Jul 2021 at 18:43, Laurent Pinchart wrote:
> > > On Wed, Jul 14, 2021 at 06:38:42PM +0100, Naushir Patuck wrote:
> > > > On Wed, 14 Jul 2021 at 18:07, Laurent Pinchart wrote:
> > > > > On Wed, Jul 14, 2021 at 02:57:11PM +0200, Jean-Michel Hautbois wrote:
> > > > > > On 14/07/2021 14:48, Laurent Pinchart wrote:
> > > > > > > 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@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.
> > > > > >
> > > > > > Indeed :-)
> > > > > >
> > > > > > >> +          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 ?
> > > > > >
> > > > > > I think it may be a good idea. Pros I can see:
> > > > > > - easy to document the tags
> > > > > > - readability (we can always refer to the enum to know how a particular
> > > > > > object is mapped).
> > > > > > - fastest to find the tag
> > > > > > Cons, not much:
> > > > > > - if RPi wants to switch to the libipa Metadata class they will need to
> > > > > > rewrite a not that small piece of code.
> > > >
> > > > What is being described here seems suspiciously similar to a ControlList
> > > :-)
> > >
> > > A bit indeed, but with an std::any instead of a small set of supported
> > > types, and no support for ControlInfo. It's "just" a way to ensure we
> > > won't get the type T wrong in a call.
> >
> > I was thinking along the lines of using ControlValues of type Span<uint8_t> like
> > we do in the IPA to set ISP controls. It does not exactly have the convenience
> > of std::any usage, but does allow arbitrary structs to be placed into the
> > ControlList.
>
> That's right, but we then lose type-safety, with a possible buffer
> overflow or just misinterpretation of data, and that's even harder to
> debug than an uncaught exception being thrown in uncommon cases.
>
> > > > Perhaps for your usage, that is more appropriate over our Metadata object?
> > >
> > > Do you think the usage in the Raspberry Pi IPA differs significantly ?
> >
> > For RPi, we like the convenience and ease of use for the existing method.
> > You can just call set/get without any ancillary setup, which is a big reason we
> > chose strings over enums for key types.  This code predated our IPA, so
> > we used to take care of std::any type mismatches using exceptions, which were
> > removed when we ported our code across. Without this, you do need to be
> > somewhat careful not to trip over.
>
> Type mismatch are essentially programming errors. Dealing with them by
> catching exceptions can help recovering gracefully, but isn't it best to
> catch those errors at compile time ?
>
> > Other than that, usage wise, I expect things to be similar to what JM intends, but will
> > have to wait and see.  If this is indeed the case, I expect all key/value pairs to be very
> > specific to each IPA and doubt  much (if any) could be shared across various vendors.
>
> That's my expectations too. I'm thinking some keys may become shared
> across vendors, there's some data that may be common enough to make this
> possible, but the devil is in the details and I have a feeling that the
> interoperability this could bring (by using some common pieces in
> different IPA modules) is just a dream. Let's see how it turns out, but
> it's probably more pain than gain.
>
> I'd like to experiment with the template Tag class (name to be
> bikeshedded) to see how it turns out. If we go in that direction, string
> vs. integer becomes a moot point as the tag value will be internal. And
> if it turns out it's not a good idea, then we'll do something else :-)
>
> > Because of this, and the likelihood that these structures will only be used internally to
> > the IPA, do you see auto-generated documentation to be required?  Of course the structs
> > and fields must be documented in the source files :-)
>
> No, I wouldn't generate documentation out of this. We should generate
> documentation for shared code in libipa, but not for module-specific
> code. Having internal documentation in the form of comments is of course
> good, to help others understand the code, and increase maintainability.
>
> > > > > We can also help ;-)
> > > > >
> > > > > > - having integer would make the code less dynamic, as we would certainly
> > > > > > be tempted to create a static map (which can also be seen as a pro :-)).
> > > > >
> > > > > That's my main question. Do you foresee a need to have some sort of
> > > > > namespace in tags ? Will the data stored here be specific to each IPA
> > > > > module, or would there be some level of standardization ? If we want to
> > > > > create an IPA module from a set of algorithms, who will be responsible
> > > > > for accessing the storage, would that be code specific to that IPA
> > > > > module, and/or code from libipa ? There's lots of design questions, I
> > > > > have little visibility on what you're planning.
> > > > >
> > > > > The idea of using a template Tag structure as described above is
> > > > > orthogonal to the decision of using strings or integers as identifiers.
> > > > > Tags would however need to be defined in headers, which I think is an
> > > > > upside, as we can enforce documentation in that case. I don't know if it
> > > > > would be feasible (or even a good idea) to centralize all the tag
> > > > > definitions though, they could be spread across different headers (some
> > > > > being device-specific, and some being maybe shared accross different IPA
> > > > > modules).
> > > > >
> > > > > > >> +  }
> > > > > > >> +
> > > > > > >> +  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