[libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata class
Naushir Patuck
naush at raspberrypi.com
Wed Jul 14 10:45:21 CEST 2021
Hi Kieran and JM,
On Wed, 14 Jul 2021 at 09:12, Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:
> Hi JM,
>
> On 14/07/2021 07:59, Jean-Michel Hautbois wrote:
> > Hi Kieran,
> >
> > Thanks for the review :-).
> >
> > On 13/07/2021 16:41, Kieran Bingham wrote:
> >> Hi JM,
> >>
> >> 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.
>
You could re-use the existing copy/move constructors and operators from our
implementation
for these.
Regards,
Naush
>
> 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?
>
>
> >
> >>
> >>> +
> >>> + 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__ */
> >>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210714/686b5d00/attachment-0001.htm>
More information about the libcamera-devel
mailing list