[libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jul 14 19:43:53 CEST 2021
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 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.
>
> 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__ */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list