[libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata class

Jean-Michel Hautbois jeanmichel.hautbois at ideasonboard.com
Wed Jul 14 10:48:53 CEST 2021


Hi Naush,

On 14/07/2021 10:45, Naushir Patuck wrote:
> Hi Kieran and JM,
> 
> On Wed, 14 Jul 2021 at 09:12, Kieran Bingham
> <kieran.bingham at ideasonboard.com
> <mailto: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
>     <mailto: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
>     <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.

Yes, sorry about that... :-(.
You don't have a destructor though ?

> 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
>     <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__ */
>     >>>
> 


More information about the libcamera-devel mailing list