[libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata class
Jean-Michel Hautbois
jeanmichel.hautbois at ideasonboard.com
Wed Jul 14 10:18:52 CEST 2021
On 14/07/2021 10:12, Kieran Bingham 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.
>
> 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
Doxygen documentation needed to make it compile without warnings is not
considered in this first patch ?
> 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__ */
>>>>
More information about the libcamera-devel
mailing list