[libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata class
Jean-Michel Hautbois
jeanmichel.hautbois at ideasonboard.com
Wed Jul 14 14:57:11 CEST 2021
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 at ideasonboard.com>
>> ---
>> src/ipa/ipu3/ipu3.cpp | 1 +
>> src/ipa/libipa/meson.build | 6 ++-
>> src/ipa/libipa/metadata.cpp | 101 ++++++++++++++++++++++++++++++++++++
>> src/ipa/libipa/metadata.h | 90 ++++++++++++++++++++++++++++++++
>> 4 files changed, 196 insertions(+), 2 deletions(-)
>> create mode 100644 src/ipa/libipa/metadata.cpp
>> create mode 100644 src/ipa/libipa/metadata.h
>>
>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>> index 71698d36..091856f5 100644
>> --- a/src/ipa/ipu3/ipu3.cpp
>> +++ b/src/ipa/ipu3/ipu3.cpp
>> @@ -25,6 +25,7 @@
>> #include "ipu3_agc.h"
>> #include "ipu3_awb.h"
>> #include "libipa/camera_sensor_helper.h"
>> +#include "libipa/metadata.h"
>>
>> static constexpr uint32_t kMaxCellWidthPerSet = 160;
>> static constexpr uint32_t kMaxCellHeightPerSet = 56;
>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
>> index 3fda7c00..cc4e1cc9 100644
>> --- a/src/ipa/libipa/meson.build
>> +++ b/src/ipa/libipa/meson.build
>> @@ -3,13 +3,15 @@
>> libipa_headers = files([
>> 'algorithm.h',
>> 'camera_sensor_helper.h',
>> - 'histogram.h'
>> + 'histogram.h',
>> + 'metadata.h'
>> ])
>>
>> libipa_sources = files([
>> 'algorithm.cpp',
>> 'camera_sensor_helper.cpp',
>> - 'histogram.cpp'
>> + 'histogram.cpp',
>> + 'metadata.cpp'
>> ])
>>
>> libipa_includes = include_directories('..')
>> diff --git a/src/ipa/libipa/metadata.cpp b/src/ipa/libipa/metadata.cpp
>> new file mode 100644
>> index 00000000..b6aef897
>> --- /dev/null
>> +++ b/src/ipa/libipa/metadata.cpp
>> @@ -0,0 +1,101 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Based on the implementation from the Raspberry Pi IPA,
>> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.
>> + * Copyright (C) 2021, Ideas On Board
>> + *
>> + * metadata.cpp - libipa metadata class
>> + */
>> +
>> +#include "metadata.h"
>> +
>> +/**
>> + * \file metadata.h
>> + * \brief A metadata class to share objects
>> + */
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa {
>> +
>> +/**
>> + * \class Metadata
>> + * \brief A simple class for carrying arbitrary metadata, for example
>> + * about an image. It is used to exchange data between algorithms.
>> + */
>> +
>> +/**
>> + * \fn Metadata::Metadata(Metadata const &other)
>> + * \param[in] other A Metadata object
>> + *
>> + * Stores the data from one Metadata to another one
>> + */
>> +
>> +/**
>> + * \fn Metadata::set(std::string const &tag, T const &value)
>> + * \param[in] tag A string used as the key in a map
>> + * \param[in] value The value to set into the map
>> + *
>> + * Sets the value in the map to the tag key. The mutex is
>> + * taken for the duration of the block.
>> + */
>> +
>> +/**
>> + * \fn Metadata::get(std::string const &tag, T &value)
>> + * \param[in] tag A string used as the key in a map
>> + * \param[in] value The value to set into the map
>> + *
>> + * Gets the value in the map of the tag key. The mutex is
>> + * taken for the duration of the block.
>> + *
>> + * \return 0 if value is found, -1 if not existent
>> + */
>> +
>> +/**
>> + * \fn Metadata::clear()
>> + * Clear the Metadata map. The mutex is taken for the duration of
>> + * the block.
>> + */
>> +
>> +/**
>> + * \fn Metadata::merge(Metadata &other)
>> + * \param[in] other A metadata to merge with
>> + * Merge two Metadata maps. The mutex is taken for the duration of
>> + * the block.
>> + */
>> +
>> +/**
>> + * \fn Metadata::getLocked(std::string const &tag)
>> + * \param[in] tag A string used as the key in a map
>> + *
>> + * Get the value of the tag key in the map.
>> + * This allows in-place access to the Metadata contents,
>> + * for which you should be holding the lock.
>> + */
>> +
>> +/**
>> + * \fn Metadata::setLocked(std::string const &tag, T const &value)
>> + * \param[in] tag A string used as the key in a map
>> + * \param[in] value The value to set into the map
>> + *
>> + * Set the value to the tag key in the map.
>> + * This allows in-place access to the Metadata contents,
>> + * for which you should be holding the lock.
>> + */
>> +
>> +/**
>> + * \fn Metadata::lock()
>> + * Lock the mutex with the standard classes.
>> + * e.g. std::lock_guard<RPiController::Metadata> lock(metadata)
>> + */
>> +
>> +/**
>> + * \fn Metadata::unlock()
>> + * Unlock the mutex with the standard classes.
>> + * e.g. std::lock_guard<RPiController::Metadata> lock(metadata)
>> + */
>> +
>> +} /* namespace ipa */
>> +
>> +} /* namespace libcamera */
>> +
>> diff --git a/src/ipa/libipa/metadata.h b/src/ipa/libipa/metadata.h
>> new file mode 100644
>> index 00000000..9801bece
>> --- /dev/null
>> +++ b/src/ipa/libipa/metadata.h
>> @@ -0,0 +1,90 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Based on the implementation from the Raspberry Pi IPA,
>> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.
>> + * Copyright (C) 2021, Ideas On Board
>> + *
>> + * metadata.h - libipa metadata class
>> + */
>> +#ifndef __LIBCAMERA_IPA_LIBIPA_METADATA_H__
>> +#define __LIBCAMERA_IPA_LIBIPA_METADATA_H__
>> +
>> +#include <any>
>> +#include <map>
>> +#include <memory>
>> +#include <mutex>
>> +#include <string>
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa {
>> +
>> +class Metadata
>> +{
>> +public:
>> + Metadata() = default;
>> +
>> + Metadata(Metadata const &other)
>> + {
>> + std::scoped_lock other_lock(other.mutex_);
>> + data_ = other.data_;
>> + }
>> +
>> + template<typename T>
>> + void set(std::string const &tag, T const &value)
>> + {
>> + std::scoped_lock lock(mutex_);
>> + data_[tag] = value;
>> + }
>> +
>> + template<typename T>
>> + int get(std::string const &tag, T &value) const
>> + {
>> + std::scoped_lock lock(mutex_);
>
> You can call getLocked() here.
>
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__ */
>
More information about the libcamera-devel
mailing list