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