[libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Jul 15 01:24:31 CEST 2021
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 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.
>
> 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
More information about the libcamera-devel
mailing list