<div dir="ltr"><div dir="ltr">Hi,<div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 14 Jul 2021 at 18:07, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@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">On Wed, Jul 14, 2021 at 02:57:11PM +0200, Jean-Michel Hautbois wrote:<br>
> Hi Laurent,<br>
> <br>
> On 14/07/2021 14:48, Laurent Pinchart wrote:<br>
> > Hi Jean-Michel,<br>
> > <br>
> > Thank you for the patch.<br>
> > <br>
> > On Mon, Jul 12, 2021 at 03:16:29PM +0200, 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>
> >> 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>
> >>  <br>
> >>  libipa_sources = files([<br>
> >>      'algorithm.cpp',<br>
> >>      'camera_sensor_helper.cpp',<br>
> >> -    'histogram.cpp'<br>
> >> +    'histogram.cpp',<br>
> >> +    'metadata.cpp'<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>
> >> +<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>
> >> +<br>
> >> +/**<br>
> >> + * \fn Metadata::Metadata(Metadata const &other)<br>
> >> + * \param[in] other A Metadata object<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>
> >> + * Sets the value in the map to the tag key. The mutex is<br>
> >> + * taken for the duration of the block.<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>
> >> +<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>
> >> +  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>
> > <br>
> > You can call getLocked() here.<br>
> > <br>
> Indeed :-)<br>
> <br>
> >> +          auto it = data_.find(tag);<br>
> >> +          if (it == data_.end())<br>
> >> +                  return -1;<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>
> > This is a bit dangerous, if T doesn't match the type stored for the tag.<br>
> > It would of course be a bug in the caller, but if such code appears in<br>
> > paths that are not frequently taken, it could cause issues that will<br>
> > result in a crash at runtime later on.<br>
> > <br>
> > Could we use a mechanism similar to Control and ControlList to ensure<br>
> > the right case ?<br>
> > <br>
> > template<typename T><br>
> > struct Tag {<br>
> >     std::string tag_;<br>
> > };<br>
> > <br>
> > /* Static list of all tags. */<br>
> > static constexpr Tag<Foo> tagFoo{ "foo" };<br>
> > <br>
> > class Metadata<br>
> > {<br>
> >     ...<br>
> >     template<typename T><br>
> >     T *getLock(const Tag<T> &tag)<br>
> >     {<br>
> >             ...<br>
> >             return std::any_cast<T>(&it->second);<br>
> >     }<br>
> >     ...<br>
> > };<br>
> > ...<br>
> > <br>
> >     Foo *f = metadata.get(tagFoo);<br>
> > <br>
> > Furthermore, have you considered using integers instead of strings for<br>
> > tags ? What are the pros and cons ?<br>
> <br>
> I think it may be a good idea. Pros I can see:<br>
> - easy to document the tags<br>
> - readability (we can always refer to the enum to know how a particular<br>
> object is mapped).<br>
> - fastest to find the tag<br>
> Cons, not much:<br>
> - if RPi wants to switch to the libipa Metadata class they will need to<br>
> rewrite a not that small piece of code.<br></blockquote><div><br></div><div>What is being described here seems suspiciously similar to a ControlList :-)</div><div>Perhaps for your usage, that is more appropriate over our Metadata object?</div><div><br></div><div>Regards,</div><div>Naush</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>
We can also help ;-)<br>
<br>
> - having integer would make the code less dynamic, as we would certainly<br>
> be tempted to create a static map (which can also be seen as a pro :-)).<br>
<br>
That's my main question. Do you foresee a need to have some sort of<br>
namespace in tags ? Will the data stored here be specific to each IPA<br>
module, or would there be some level of standardization ? If we want to<br>
create an IPA module from a set of algorithms, who will be responsible<br>
for accessing the storage, would that be code specific to that IPA<br>
module, and/or code from libipa ? There's lots of design questions, I<br>
have little visibility on what you're planning.<br>
<br>
The idea of using a template Tag structure as described above is<br>
orthogonal to the decision of using strings or integers as identifiers.<br>
Tags would however need to be defined in headers, which I think is an<br>
upside, as we can enforce documentation in that case. I don't know if it<br>
would be feasible (or even a good idea) to centralize all the tag<br>
definitions though, they could be spread across different headers (some<br>
being device-specific, and some being maybe shared accross different IPA<br>
modules).<br>
<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>
> >> +  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>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>