<div dir="ltr"><div dir="ltr">Hi Kieran and JM,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 14 Jul 2021 at 09:12, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi JM,<br>
<br>
On 14/07/2021 07:59, Jean-Michel Hautbois wrote:<br>
> Hi Kieran,<br>
> <br>
> Thanks for the review :-).<br>
> <br>
> On 13/07/2021 16:41, Kieran Bingham wrote:<br>
>> Hi JM,<br>
>><br>
>> On 12/07/2021 14:16, Jean-Michel Hautbois wrote:<br>
>>> The Metadata class comes from RPi from which a bit has been removed<br>
>>> because we don't need it for now.<br>
>>> All functions are inlined in metadata.h because of the template usage.<br>
>><br>
>> Perhaps this could be better worded:<br>
>><br>
>> """<br>
>> Import the Metadata class from src/ipa/raspberrypi/metadata.cpp to be<br>
>> able to make use of the component from other IPA modules.<br>
>> """<br>
>><br>
>><br>
>>><br>
>>> Signed-off-by: Jean-Michel Hautbois <<a href="mailto:jeanmichel.hautbois@ideasonboard.com" target="_blank">jeanmichel.hautbois@ideasonboard.com</a>><br>
>>> ---<br>
>>>  src/ipa/ipu3/ipu3.cpp       |   1 +<br>
>>>  src/ipa/libipa/meson.build  |   6 ++-<br>
>>>  src/ipa/libipa/metadata.cpp | 101 ++++++++++++++++++++++++++++++++++++<br>
>>>  src/ipa/libipa/metadata.h   |  90 ++++++++++++++++++++++++++++++++<br>
>>>  4 files changed, 196 insertions(+), 2 deletions(-)<br>
>>>  create mode 100644 src/ipa/libipa/metadata.cpp<br>
>>>  create mode 100644 src/ipa/libipa/metadata.h<br>
>>><br>
>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp<br>
>>> index 71698d36..091856f5 100644<br>
>>> --- a/src/ipa/ipu3/ipu3.cpp<br>
>>> +++ b/src/ipa/ipu3/ipu3.cpp<br>
>>> @@ -25,6 +25,7 @@<br>
>>>  #include "ipu3_agc.h"<br>
>>>  #include "ipu3_awb.h"<br>
>>>  #include "libipa/camera_sensor_helper.h"<br>
>>> +#include "libipa/metadata.h"<br>
>>>  <br>
>>>  static constexpr uint32_t kMaxCellWidthPerSet = 160;<br>
>>>  static constexpr uint32_t kMaxCellHeightPerSet = 56;<br>
>>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build<br>
>>> index 3fda7c00..cc4e1cc9 100644<br>
>>> --- a/src/ipa/libipa/meson.build<br>
>>> +++ b/src/ipa/libipa/meson.build<br>
>>> @@ -3,13 +3,15 @@<br>
>>>  libipa_headers = files([<br>
>>>      'algorithm.h',<br>
>>>      'camera_sensor_helper.h',<br>
>>> -    'histogram.h'<br>
>>> +    'histogram.h',<br>
>>> +    'metadata.h'<br>
>><br>
>> If you keep a ',' at the end, you don't to modify this line when adding<br>
>> another entry later.<br>
>><br>
>><br>
>>>  ])<br>
>>>  <br>
>>>  libipa_sources = files([<br>
>>>      'algorithm.cpp',<br>
>>>      'camera_sensor_helper.cpp',<br>
>>> -    'histogram.cpp'<br>
>>> +    'histogram.cpp',<br>
>>> +    'metadata.cpp'<br>
>><br>
>> Same here...<br>
>><br>
>><br>
>>>  ])<br>
>>>  <br>
>>>  libipa_includes = include_directories('..')<br>
>>> diff --git a/src/ipa/libipa/metadata.cpp b/src/ipa/libipa/metadata.cpp<br>
>>> new file mode 100644<br>
>>> index 00000000..b6aef897<br>
>>> --- /dev/null<br>
>>> +++ b/src/ipa/libipa/metadata.cpp<br>
>>> @@ -0,0 +1,101 @@<br>
>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
>>> +/*<br>
>>> + * Based on the implementation from the Raspberry Pi IPA,<br>
>>> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.<br>
>>> + * Copyright (C) 2021, Ideas On Board<br>
>>> + *<br>
>>> + * metadata.cpp -  libipa metadata class<br>
>>> + */<br>
>>> +<br>
>>> +#include "metadata.h"<br>
>>> +<br>
>>> +/**<br>
>>> + * \file metadata.h<br>
>>> + * \brief A metadata class to share objects<br>
>><br>
>> I wouldn't call it sharing objects...<br>
>><br>
>> "A metadata class to provide key based access to arbitrary metadata types.<br>
>><br>
>><br>
>>> + */<br>
>>> +<br>
>>> +namespace libcamera {<br>
>>> +<br>
>>> +namespace ipa {<br>
>>> +<br>
>>> +/**<br>
>>> + * \class Metadata<br>
>>> + * \brief A simple class for carrying arbitrary metadata, for example<br>
>>> + * about an image. It is used to exchange data between algorithms.<br>
>><br>
>> I think we need to expand on this to explain some of the constraints too:<br>
>><br>
>><br>
>> """<br>
>> Data is stored as a map with a string based key.<br>
>><br>
>> The metadata is stored through a std::any() type which is definable by<br>
>> the user, and must be correctly known by both the producer and consumer.<br>
>><br>
>> Accessing the metadata with an incorrect type will cause undefined<br>
>> behaviour.<br>
>> """<br>
>><br>
>> Some of that might be too far towards the implementation details, but in<br>
>> this instance, I sort of think those implementation details are<br>
>> important because the implications they introduce.<br>
>><br>
>><br>
>><br>
>>> + */<br>
>>> +<br>
>>> +/**<br>
>>> + * \fn Metadata::Metadata(Metadata const &other)<br>
>>> + * \param[in] other A Metadata object<br>
>>> + *<br>
>><br>
>> I think this is the copy constructor right?<br>
>><br>
>> Lets reference it so:<br>
>><br>
>> "Copy the data from the \a other Metadata object to this one."<br>
>><br>
>><br>
>><br>
>>> + * Stores the data from one Metadata to another one<br>
>>> + */<br>
>>> +<br>
>>> +/**<br>
>>> + * \fn Metadata::set(std::string const &tag, T const &value)<br>
>>> + * \param[in] tag A string used as the key in a map<br>
>>> + * \param[in] value The value to set into the map<br>
>><br>
>> I would probably drop 'in a map' and 'into the map' in these parameter<br>
>> descriptions.<br>
>><br>
>><br>
>>> + *<br>
>>> + * Sets the value in the map to the tag key. The mutex is<br>
>>> + * taken for the duration of the block.<br>
>><br>
>><br>
>> I would express this as..<br>
>><br>
>> "This function locks the metadata to protect from concurrent access"<br>
>><br>
>> (rather than "the mutex is...", and on a line/paragraph of its own to<br>
>> stand out.)<br>
>><br>
>><br>
>>> + */<br>
>>> +<br>
>>> +/**<br>
>>> + * \fn Metadata::get(std::string const &tag, T &value)<br>
>>> + * \param[in] tag A string used as the key in a map<br>
>>> + * \param[in] value The value to set into the map<br>
>>> + *<br>
>>> + * Gets the value in the map of the tag key. The mutex is<br>
>>> + * taken for the duration of the block.<br>
>>> + *<br>
>>> + * \return 0 if value is found, -1 if not existent<br>
>>> + */<br>
>>> +<br>
>>> +/**<br>
>>> + * \fn Metadata::clear()<br>
>>> + * Clear the Metadata map. The mutex is taken for the duration of<br>
>>> + * the block.<br>
>>> + */<br>
>>> +<br>
>>> +/**<br>
>>> + * \fn Metadata::merge(Metadata &other)<br>
>>> + * \param[in] other A metadata to merge with<br>
>>> + * Merge two Metadata maps. The mutex is taken for the duration of<br>
>>> + * the block.<br>
>>> + */<br>
>>> +<br>
>>> +/**<br>
>>> + * \fn Metadata::getLocked(std::string const &tag)<br>
>>> + * \param[in] tag A string used as the key in a map<br>
>>> + *<br>
>>> + * Get the value of the tag key in the map.<br>
>>> + * This allows in-place access to the Metadata contents,<br>
>>> + * for which you should be holding the lock.<br>
>><br>
>> I would expand upon this to also state how to lock it, given that it is<br>
>> part of the API of this class:<br>
>><br>
>><br>
>> """<br>
>> This function does not protect against concurrent access, and it is up<br>
>> to the caller to ensure that the lock is held using \a lock()<br>
>> """<br>
>><br>
>>> + */<br>
>>> +<br>
>>> +/**<br>
>>> + * \fn Metadata::setLocked(std::string const &tag, T const &value)<br>
>>> + * \param[in] tag A string used as the key in a map<br>
>>> + * \param[in] value The value to set into the map<br>
>>> + *<br>
>>> + * Set the value to the tag key in the map.<br>
>>> + * This allows in-place access to the Metadata contents,<br>
>>> + * for which you should be holding the lock.<br>
>>> + */<br>
>>> +<br>
>>> +/**<br>
>>> + * \fn Metadata::lock()<br>
>>> + * Lock the mutex with the standard classes.<br>
>>> + * e.g. std::lock_guard<RPiController::Metadata> lock(metadata)<br>
>>> + */<br>
>>> +<br>
>>> +/**<br>
>>> + * \fn Metadata::unlock()<br>
>>> + * Unlock the mutex with the standard classes.<br>
>>> + * e.g. std::lock_guard<RPiController::Metadata> lock(metadata)<br>
>>> + */<br>
>>> +<br>
>>> +} /* namespace ipa */<br>
>>> +<br>
>>> +} /* namespace libcamera */<br>
>>> +<br>
>>> diff --git a/src/ipa/libipa/metadata.h b/src/ipa/libipa/metadata.h<br>
>>> new file mode 100644<br>
>>> index 00000000..9801bece<br>
>>> --- /dev/null<br>
>>> +++ b/src/ipa/libipa/metadata.h<br>
>>> @@ -0,0 +1,90 @@<br>
>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
>>> +/*<br>
>>> + * Based on the implementation from the Raspberry Pi IPA,<br>
>>> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.<br>
>>> + * Copyright (C) 2021, Ideas On Board<br>
>>> + *<br>
>>> + * metadata.h - libipa metadata class<br>
>>> + */<br>
>>> +#ifndef __LIBCAMERA_IPA_LIBIPA_METADATA_H__<br>
>>> +#define __LIBCAMERA_IPA_LIBIPA_METADATA_H__<br>
>>> +<br>
>>> +#include <any><br>
>>> +#include <map><br>
>>> +#include <memory><br>
>>> +#include <mutex><br>
>>> +#include <string><br>
>>> +<br>
>>> +namespace libcamera {<br>
>>> +<br>
>>> +namespace ipa {<br>
>>> +<br>
>>> +class Metadata<br>
>>> +{<br>
>>> +public:<br>
>>> +   Metadata() = default;<br>
>>> +<br>
>>> +   Metadata(Metadata const &other)<br>
>>> +   {<br>
>>> +           std::scoped_lock other_lock(other.mutex_);<br>
>>> +           data_ = other.data_;<br>
>>> +   }<br>
>><br>
>> The C++ rule of five (or is it six?) says if you need to implement one<br>
>> of the copy/move constructors you need to implement, default (or<br>
>> delete?) them all:<br>
>><br>
>> <a href="https://www.modernescpp.com/index.php/c-core-guidelines-constructors-assignments-and-desctructors" rel="noreferrer" target="_blank">https://www.modernescpp.com/index.php/c-core-guidelines-constructors-assignments-and-desctructors</a><br>
>><br>
>> So we should add appropriate copy assignment, and move constructors with<br>
>> appropriate locking ... (or delete them if they shouldn't be allowed).<br>
>><br>
>><br>
>> I think the constructor and destructor can be = default though, I don't<br>
>> see anything specific to handle there?<br>
>><br>
> <br>
> OK, I will implement those.<br>
<br>
When you do this, please try to be clear in the story your patches tell.<br></blockquote><div><br></div><div>You could re-use the existing copy/move constructors and operators from our implementation</div><div>for these.</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
This is clearly additional code on top of the code which is imported, so<br>
the series should do something like:<br>
<br>
 1/x ipa: libipa: Import Metadata class from src/ipa/raspberrypi<br>
    Just the direct copy, without any specific changes other than making<br>
    it compile in place<br>
<br>
 2/x ipa: libipa: Document Metadata class<br>
    Any documentation that /you/ add.<br>
<br>
 3/x ipa: libipa: Implement missing Metadata class copy/move operations<br>
    Any code additions that you add<br>
<br>
 4/x .... other?<br>
<br>
<br>
> <br>
>><br>
>>> +<br>
>>> +   template<typename T><br>
>>> +   void set(std::string const &tag, T const &value)<br>
>>> +   {<br>
>>> +           std::scoped_lock lock(mutex_);<br>
>>> +           data_[tag] = value;<br>
>>> +   }<br>
>>> +<br>
>>> +   template<typename T><br>
>>> +   int get(std::string const &tag, T &value) const<br>
>>> +   {<br>
>>> +           std::scoped_lock lock(mutex_);<br>
>>> +           auto it = data_.find(tag);<br>
>>> +           if (it == data_.end())<br>
>>> +                   return -1;<br>
>><br>
>> Does std::any provide any way to get the template type of the object so<br>
>> we can assert with an incorrect access?<br>
> <br>
> Something like that ?<br>
> <a href="https://en.cppreference.com/w/cpp/utility/any/type" rel="noreferrer" target="_blank">https://en.cppreference.com/w/cpp/utility/any/type</a><br>
> <br>
>> Perhaps that would then require specific RTTI which maybe we don't want<br>
>> to get into anyway though...<br>
> <br>
> I am not sure if it is worth it...<br>
> <br>
>><br>
>>> +           value = std::any_cast<T>(it->second);<br>
>>> +           return 0;<br>
>>> +   }<br>
>>> +<br>
>>> +   void clear()<br>
>>> +   {<br>
>>> +           std::scoped_lock lock(mutex_);<br>
>>> +           data_.clear();<br>
>>> +   }<br>
>>> +<br>
>>> +   void merge(Metadata &other)<br>
>>> +   {<br>
>>> +           std::scoped_lock lock(mutex_, other.mutex_);<br>
>>> +           data_.merge(other.data_);<br>
>>> +   }<br>
>>> +<br>
>>> +   template<typename T><br>
>>> +   T *getLocked(std::string const &tag)<br>
>>> +   {<br>
>>> +           auto it = data_.find(tag);<br>
>>> +           if (it == data_.end())<br>
>>> +                   return nullptr;<br>
>>> +           return std::any_cast<T>(&it->second);<br>
>>> +   }<br>
>>> +<br>
>>> +   template<typename T><br>
>>> +   void setLocked(std::string const &tag, T const &value)<br>
>>> +   {<br>
>>> +           data_[tag] = value;<br>
>>> +   }<br>
>>> +<br>
>>> +   void lock() { mutex_.lock(); }<br>
>>> +   void unlock() { mutex_.unlock(); }<br>
>>> +<br>
>>> +private:<br>
>>> +   mutable std::mutex mutex_;<br>
>><br>
>> Hrm, I had to look this up. I wonder if we should be using mutable more<br>
>> often.<br>
>><br>
>> But indeed, this sounds right as it means you can 'get' from a const<br>
>> Metadata and lock the mutex (which requires modifying the otherwise<br>
>> const mutex).<br>
>><br>
>><br>
>><br>
>>> +   std::map<std::string, std::any> data_;<br>
>>> +};<br>
>>> +<br>
>>> +} /* namespace ipa */<br>
>>> +<br>
>>> +} /* namespace libcamera */<br>
>>> +<br>
>>> +#endif /* __LIBCAMERA_IPA_LIBIPA_METADATA_H__ */<br>
>>><br>
</blockquote></div></div>