[libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata class
Naushir Patuck
naush at raspberrypi.com
Wed Jul 14 20:40:51 CEST 2021
Hi Laurent,
On Wed, 14 Jul 2021 at 18:43, Laurent Pinchart <
laurent.pinchart at 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 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.
>
> > 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210714/38c429d4/attachment-0001.htm>
More information about the libcamera-devel
mailing list