[{"id":18161,"web_url":"https://patchwork.libcamera.org/comment/18161/","msgid":"<62596562-ae49-4277-c428-5380198f0559@ideasonboard.com>","date":"2021-07-13T14:41:31","subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata\n\tclass","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi JM,\n\nOn 12/07/2021 14:16, Jean-Michel Hautbois wrote:\n> The Metadata class comes from RPi from which a bit has been removed\n> because we don't need it for now.\n> All functions are inlined in metadata.h because of the template usage.\n\nPerhaps this could be better worded:\n\n\"\"\"\nImport the Metadata class from src/ipa/raspberrypi/metadata.cpp to be\nable to make use of the component from other IPA modules.\n\"\"\"\n\n\n> \n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/ipu3.cpp | 1 +\n> src/ipa/libipa/meson.build | 6 ++-\n> src/ipa/libipa/metadata.cpp | 101 ++++++++++++++++++++++++++++++++++++\n> src/ipa/libipa/metadata.h | 90 ++++++++++++++++++++++++++++++++\n> 4 files changed, 196 insertions(+), 2 deletions(-)\n> create mode 100644 src/ipa/libipa/metadata.cpp\n> create mode 100644 src/ipa/libipa/metadata.h\n> \n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 71698d36..091856f5 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -25,6 +25,7 @@\n> #include \"ipu3_agc.h\"\n> #include \"ipu3_awb.h\"\n> #include \"libipa/camera_sensor_helper.h\"\n> +#include \"libipa/metadata.h\"\n> \n> static constexpr uint32_t kMaxCellWidthPerSet = 160;\n> static constexpr uint32_t kMaxCellHeightPerSet = 56;\n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index 3fda7c00..cc4e1cc9 100644\n> --- a/src/ipa/libipa/meson.build\n> +++ b/src/ipa/libipa/meson.build\n> @@ -3,13 +3,15 @@\n> libipa_headers = files([\n> 'algorithm.h',\n> 'camera_sensor_helper.h',\n> - 'histogram.h'\n> + 'histogram.h',\n> + 'metadata.h'\n\nIf you keep a ',' at the end, you don't to modify this line when adding\nanother entry later.\n\n\n> ])\n> \n> libipa_sources = files([\n> 'algorithm.cpp',\n> 'camera_sensor_helper.cpp',\n> - 'histogram.cpp'\n> + 'histogram.cpp',\n> + 'metadata.cpp'\n\nSame here...\n\n\n> ])\n> \n> libipa_includes = include_directories('..')\n> diff --git a/src/ipa/libipa/metadata.cpp b/src/ipa/libipa/metadata.cpp\n> new file mode 100644\n> index 00000000..b6aef897\n> --- /dev/null\n> +++ b/src/ipa/libipa/metadata.cpp\n> @@ -0,0 +1,101 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Based on the implementation from the Raspberry Pi IPA,\n> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.\n> + * Copyright (C) 2021, Ideas On Board\n> + *\n> + * metadata.cpp - libipa metadata class\n> + */\n> +\n> +#include \"metadata.h\"\n> +\n> +/**\n> + * \\file metadata.h\n> + * \\brief A metadata class to share objects\n\nI wouldn't call it sharing objects...\n\n\"A metadata class to provide key based access to arbitrary metadata types.\n\n\n> + */\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +/**\n> + * \\class Metadata\n> + * \\brief A simple class for carrying arbitrary metadata, for example\n> + * about an image. It is used to exchange data between algorithms.\n\nI think we need to expand on this to explain some of the constraints too:\n\n\n\"\"\"\nData is stored as a map with a string based key.\n\nThe metadata is stored through a std::any() type which is definable by\nthe user, and must be correctly known by both the producer and consumer.\n\nAccessing the metadata with an incorrect type will cause undefined\nbehaviour.\n\"\"\"\n\nSome of that might be too far towards the implementation details, but in\nthis instance, I sort of think those implementation details are\nimportant because the implications they introduce.\n\n\n\n> + */\n> +\n> +/**\n> + * \\fn Metadata::Metadata(Metadata const &other)\n> + * \\param[in] other A Metadata object\n> + *\n\nI think this is the copy constructor right?\n\nLets reference it so:\n\n\"Copy the data from the \\a other Metadata object to this one.\"\n\n\n\n> + * Stores the data from one Metadata to another one\n> + */\n> +\n> +/**\n> + * \\fn Metadata::set(std::string const &tag, T const &value)\n> + * \\param[in] tag A string used as the key in a map\n> + * \\param[in] value The value to set into the map\n\nI would probably drop 'in a map' and 'into the map' in these parameter\ndescriptions.\n\n\n> + *\n> + * Sets the value in the map to the tag key. The mutex is\n> + * taken for the duration of the block.\n\n\nI would express this as..\n\n\"This function locks the metadata to protect from concurrent access\"\n\n(rather than \"the mutex is...\", and on a line/paragraph of its own to\nstand out.)\n\n\n> + */\n> +\n> +/**\n> + * \\fn Metadata::get(std::string const &tag, T &value)\n> + * \\param[in] tag A string used as the key in a map\n> + * \\param[in] value The value to set into the map\n> + *\n> + * Gets the value in the map of the tag key. The mutex is\n> + * taken for the duration of the block.\n> + *\n> + * \\return 0 if value is found, -1 if not existent\n> + */\n> +\n> +/**\n> + * \\fn Metadata::clear()\n> + * Clear the Metadata map. The mutex is taken for the duration of\n> + * the block.\n> + */\n> +\n> +/**\n> + * \\fn Metadata::merge(Metadata &other)\n> + * \\param[in] other A metadata to merge with\n> + * Merge two Metadata maps. The mutex is taken for the duration of\n> + * the block.\n> + */\n> +\n> +/**\n> + * \\fn Metadata::getLocked(std::string const &tag)\n> + * \\param[in] tag A string used as the key in a map\n> + *\n> + * Get the value of the tag key in the map.\n> + * This allows in-place access to the Metadata contents,\n> + * for which you should be holding the lock.\n\nI would expand upon this to also state how to lock it, given that it is\npart of the API of this class:\n\n\n\"\"\"\nThis function does not protect against concurrent access, and it is up\nto the caller to ensure that the lock is held using \\a lock()\n\"\"\"\n\n> + */\n> +\n> +/**\n> + * \\fn Metadata::setLocked(std::string const &tag, T const &value)\n> + * \\param[in] tag A string used as the key in a map\n> + * \\param[in] value The value to set into the map\n> + *\n> + * Set the value to the tag key in the map.\n> + * This allows in-place access to the Metadata contents,\n> + * for which you should be holding the lock.\n> + */\n> +\n> +/**\n> + * \\fn Metadata::lock()\n> + * Lock the mutex with the standard classes.\n> + * e.g. std::lock_guard lock(metadata)\n> + */\n> +\n> +/**\n> + * \\fn Metadata::unlock()\n> + * Unlock the mutex with the standard classes.\n> + * e.g. std::lock_guard lock(metadata)\n> + */\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> +\n> diff --git a/src/ipa/libipa/metadata.h b/src/ipa/libipa/metadata.h\n> new file mode 100644\n> index 00000000..9801bece\n> --- /dev/null\n> +++ b/src/ipa/libipa/metadata.h\n> @@ -0,0 +1,90 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Based on the implementation from the Raspberry Pi IPA,\n> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.\n> + * Copyright (C) 2021, Ideas On Board\n> + *\n> + * metadata.h - libipa metadata class\n> + */\n> +#ifndef __LIBCAMERA_IPA_LIBIPA_METADATA_H__\n> +#define __LIBCAMERA_IPA_LIBIPA_METADATA_H__\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +class Metadata\n> +{\n> +public:\n> +\tMetadata() = default;\n> +\n> +\tMetadata(Metadata const &other)\n> +\t{\n> +\t\tstd::scoped_lock other_lock(other.mutex_);\n> +\t\tdata_ = other.data_;\n> +\t}\n\nThe C++ rule of five (or is it six?) says if you need to implement one\nof the copy/move constructors you need to implement, default (or\ndelete?) them all:\n\nhttps://www.modernescpp.com/index.php/c-core-guidelines-constructors-assignments-and-desctructors\n\nSo we should add appropriate copy assignment, and move constructors with\nappropriate locking ... (or delete them if they shouldn't be allowed).\n\n\nI think the constructor and destructor can be = default though, I don't\nsee anything specific to handle there?\n\n\n\n> +\n> +\ttemplate\n> +\tvoid set(std::string const &tag, T const &value)\n> +\t{\n> +\t\tstd::scoped_lock lock(mutex_);\n> +\t\tdata_[tag] = value;\n> +\t}\n> +\n> +\ttemplate\n> +\tint get(std::string const &tag, T &value) const\n> +\t{\n> +\t\tstd::scoped_lock lock(mutex_);\n> +\t\tauto it = data_.find(tag);\n> +\t\tif (it == data_.end())\n> +\t\t\treturn -1;\n\nDoes std::any provide any way to get the template type of the object so\nwe can assert with an incorrect access?\n\nPerhaps that would then require specific RTTI which maybe we don't want\nto get into anyway though...\n\n\n> +\t\tvalue = std::any_cast(it->second);\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tvoid clear()\n> +\t{\n> +\t\tstd::scoped_lock lock(mutex_);\n> +\t\tdata_.clear();\n> +\t}\n> +\n> +\tvoid merge(Metadata &other)\n> +\t{\n> +\t\tstd::scoped_lock lock(mutex_, other.mutex_);\n> +\t\tdata_.merge(other.data_);\n> +\t}\n> +\n> +\ttemplate\n> +\tT *getLocked(std::string const &tag)\n> +\t{\n> +\t\tauto it = data_.find(tag);\n> +\t\tif (it == data_.end())\n> +\t\t\treturn nullptr;\n> +\t\treturn std::any_cast(&it->second);\n> +\t}\n> +\n> +\ttemplate\n> +\tvoid setLocked(std::string const &tag, T const &value)\n> +\t{\n> +\t\tdata_[tag] = value;\n> +\t}\n> +\n> +\tvoid lock() { mutex_.lock(); }\n> +\tvoid unlock() { mutex_.unlock(); }\n> +\n> +private:\n> +\tmutable std::mutex mutex_;\n\nHrm, I had to look this up. I wonder if we should be using mutable more\noften.\n\nBut indeed, this sounds right as it means you can 'get' from a const\nMetadata and lock the mutex (which requires modifying the otherwise\nconst mutex).\n\n\n\n> +\tstd::map data_;\n> +};\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_IPA_LIBIPA_METADATA_H__ */\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8D025C3225\n\tfor ;\n\tTue, 13 Jul 2021 14:41:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D16E5684F3;\n\tTue, 13 Jul 2021 16:41:36 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CD099684F3\n\tfor ;\n\tTue, 13 Jul 2021 16:41:34 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4D54ECC;\n\tTue, 13 Jul 2021 16:41:34 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ZUPLMf/r\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626187294;\n\tbh=d2TQaG2LTjLWCliBWFooe1gZ4rlxWecBuT78wmzQ8NI=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=ZUPLMf/r3gtAnqIubA28/PsOSYwJxVOyznxOzy3Z8Q6gJVGlEO8WTLeXcMpyRdbMv\n\tcsEU6j1Qg6hux2GrlaozD1gVflFveT2TMn6/xu4tBXnt4mFbCI8vteJGEbzzKtYLQx\n\t3jpb++UilVdXWIz4xTu6vP6nN8wow/z46O4QlfcQ=","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210712131630.73914-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210712131630.73914-2-jeanmichel.hautbois@ideasonboard.com>","From":"Kieran Bingham ","Message-ID":"<62596562-ae49-4277-c428-5380198f0559@ideasonboard.com>","Date":"Tue, 13 Jul 2021 15:41:31 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210712131630.73914-2-jeanmichel.hautbois@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata\n\tclass","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18163,"web_url":"https://patchwork.libcamera.org/comment/18163/","msgid":"<6ec0e4f8-b386-348f-7a9a-b45039f22ed7@ideasonboard.com>","date":"2021-07-14T06:59:30","subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata\n\tclass","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Kieran,\n\nThanks for the review :-).\n\nOn 13/07/2021 16:41, Kieran Bingham wrote:\n> Hi JM,\n> \n> On 12/07/2021 14:16, Jean-Michel Hautbois wrote:\n>> The Metadata class comes from RPi from which a bit has been removed\n>> because we don't need it for now.\n>> All functions are inlined in metadata.h because of the template usage.\n> \n> Perhaps this could be better worded:\n> \n> \"\"\"\n> Import the Metadata class from src/ipa/raspberrypi/metadata.cpp to be\n> able to make use of the component from other IPA modules.\n> \"\"\"\n> \n> \n>>\n>> Signed-off-by: Jean-Michel Hautbois \n>> ---\n>> src/ipa/ipu3/ipu3.cpp | 1 +\n>> src/ipa/libipa/meson.build | 6 ++-\n>> src/ipa/libipa/metadata.cpp | 101 ++++++++++++++++++++++++++++++++++++\n>> src/ipa/libipa/metadata.h | 90 ++++++++++++++++++++++++++++++++\n>> 4 files changed, 196 insertions(+), 2 deletions(-)\n>> create mode 100644 src/ipa/libipa/metadata.cpp\n>> create mode 100644 src/ipa/libipa/metadata.h\n>>\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index 71698d36..091856f5 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -25,6 +25,7 @@\n>> #include \"ipu3_agc.h\"\n>> #include \"ipu3_awb.h\"\n>> #include \"libipa/camera_sensor_helper.h\"\n>> +#include \"libipa/metadata.h\"\n>> \n>> static constexpr uint32_t kMaxCellWidthPerSet = 160;\n>> static constexpr uint32_t kMaxCellHeightPerSet = 56;\n>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n>> index 3fda7c00..cc4e1cc9 100644\n>> --- a/src/ipa/libipa/meson.build\n>> +++ b/src/ipa/libipa/meson.build\n>> @@ -3,13 +3,15 @@\n>> libipa_headers = files([\n>> 'algorithm.h',\n>> 'camera_sensor_helper.h',\n>> - 'histogram.h'\n>> + 'histogram.h',\n>> + 'metadata.h'\n> \n> If you keep a ',' at the end, you don't to modify this line when adding\n> another entry later.\n> \n> \n>> ])\n>> \n>> libipa_sources = files([\n>> 'algorithm.cpp',\n>> 'camera_sensor_helper.cpp',\n>> - 'histogram.cpp'\n>> + 'histogram.cpp',\n>> + 'metadata.cpp'\n> \n> Same here...\n> \n> \n>> ])\n>> \n>> libipa_includes = include_directories('..')\n>> diff --git a/src/ipa/libipa/metadata.cpp b/src/ipa/libipa/metadata.cpp\n>> new file mode 100644\n>> index 00000000..b6aef897\n>> --- /dev/null\n>> +++ b/src/ipa/libipa/metadata.cpp\n>> @@ -0,0 +1,101 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Based on the implementation from the Raspberry Pi IPA,\n>> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.\n>> + * Copyright (C) 2021, Ideas On Board\n>> + *\n>> + * metadata.cpp - libipa metadata class\n>> + */\n>> +\n>> +#include \"metadata.h\"\n>> +\n>> +/**\n>> + * \\file metadata.h\n>> + * \\brief A metadata class to share objects\n> \n> I wouldn't call it sharing objects...\n> \n> \"A metadata class to provide key based access to arbitrary metadata types.\n> \n> \n>> + */\n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa {\n>> +\n>> +/**\n>> + * \\class Metadata\n>> + * \\brief A simple class for carrying arbitrary metadata, for example\n>> + * about an image. It is used to exchange data between algorithms.\n> \n> I think we need to expand on this to explain some of the constraints too:\n> \n> \n> \"\"\"\n> Data is stored as a map with a string based key.\n> \n> The metadata is stored through a std::any() type which is definable by\n> the user, and must be correctly known by both the producer and consumer.\n> \n> Accessing the metadata with an incorrect type will cause undefined\n> behaviour.\n> \"\"\"\n> \n> Some of that might be too far towards the implementation details, but in\n> this instance, I sort of think those implementation details are\n> important because the implications they introduce.\n> \n> \n> \n>> + */\n>> +\n>> +/**\n>> + * \\fn Metadata::Metadata(Metadata const &other)\n>> + * \\param[in] other A Metadata object\n>> + *\n> \n> I think this is the copy constructor right?\n> \n> Lets reference it so:\n> \n> \"Copy the data from the \\a other Metadata object to this one.\"\n> \n> \n> \n>> + * Stores the data from one Metadata to another one\n>> + */\n>> +\n>> +/**\n>> + * \\fn Metadata::set(std::string const &tag, T const &value)\n>> + * \\param[in] tag A string used as the key in a map\n>> + * \\param[in] value The value to set into the map\n> \n> I would probably drop 'in a map' and 'into the map' in these parameter\n> descriptions.\n> \n> \n>> + *\n>> + * Sets the value in the map to the tag key. The mutex is\n>> + * taken for the duration of the block.\n> \n> \n> I would express this as..\n> \n> \"This function locks the metadata to protect from concurrent access\"\n> \n> (rather than \"the mutex is...\", and on a line/paragraph of its own to\n> stand out.)\n> \n> \n>> + */\n>> +\n>> +/**\n>> + * \\fn Metadata::get(std::string const &tag, T &value)\n>> + * \\param[in] tag A string used as the key in a map\n>> + * \\param[in] value The value to set into the map\n>> + *\n>> + * Gets the value in the map of the tag key. The mutex is\n>> + * taken for the duration of the block.\n>> + *\n>> + * \\return 0 if value is found, -1 if not existent\n>> + */\n>> +\n>> +/**\n>> + * \\fn Metadata::clear()\n>> + * Clear the Metadata map. The mutex is taken for the duration of\n>> + * the block.\n>> + */\n>> +\n>> +/**\n>> + * \\fn Metadata::merge(Metadata &other)\n>> + * \\param[in] other A metadata to merge with\n>> + * Merge two Metadata maps. The mutex is taken for the duration of\n>> + * the block.\n>> + */\n>> +\n>> +/**\n>> + * \\fn Metadata::getLocked(std::string const &tag)\n>> + * \\param[in] tag A string used as the key in a map\n>> + *\n>> + * Get the value of the tag key in the map.\n>> + * This allows in-place access to the Metadata contents,\n>> + * for which you should be holding the lock.\n> \n> I would expand upon this to also state how to lock it, given that it is\n> part of the API of this class:\n> \n> \n> \"\"\"\n> This function does not protect against concurrent access, and it is up\n> to the caller to ensure that the lock is held using \\a lock()\n> \"\"\"\n> \n>> + */\n>> +\n>> +/**\n>> + * \\fn Metadata::setLocked(std::string const &tag, T const &value)\n>> + * \\param[in] tag A string used as the key in a map\n>> + * \\param[in] value The value to set into the map\n>> + *\n>> + * Set the value to the tag key in the map.\n>> + * This allows in-place access to the Metadata contents,\n>> + * for which you should be holding the lock.\n>> + */\n>> +\n>> +/**\n>> + * \\fn Metadata::lock()\n>> + * Lock the mutex with the standard classes.\n>> + * e.g. std::lock_guard lock(metadata)\n>> + */\n>> +\n>> +/**\n>> + * \\fn Metadata::unlock()\n>> + * Unlock the mutex with the standard classes.\n>> + * e.g. std::lock_guard lock(metadata)\n>> + */\n>> +\n>> +} /* namespace ipa */\n>> +\n>> +} /* namespace libcamera */\n>> +\n>> diff --git a/src/ipa/libipa/metadata.h b/src/ipa/libipa/metadata.h\n>> new file mode 100644\n>> index 00000000..9801bece\n>> --- /dev/null\n>> +++ b/src/ipa/libipa/metadata.h\n>> @@ -0,0 +1,90 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Based on the implementation from the Raspberry Pi IPA,\n>> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.\n>> + * Copyright (C) 2021, Ideas On Board\n>> + *\n>> + * metadata.h - libipa metadata class\n>> + */\n>> +#ifndef __LIBCAMERA_IPA_LIBIPA_METADATA_H__\n>> +#define __LIBCAMERA_IPA_LIBIPA_METADATA_H__\n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa {\n>> +\n>> +class Metadata\n>> +{\n>> +public:\n>> +\tMetadata() = default;\n>> +\n>> +\tMetadata(Metadata const &other)\n>> +\t{\n>> +\t\tstd::scoped_lock other_lock(other.mutex_);\n>> +\t\tdata_ = other.data_;\n>> +\t}\n> \n> The C++ rule of five (or is it six?) says if you need to implement one\n> of the copy/move constructors you need to implement, default (or\n> delete?) them all:\n> \n> https://www.modernescpp.com/index.php/c-core-guidelines-constructors-assignments-and-desctructors\n> \n> So we should add appropriate copy assignment, and move constructors with\n> appropriate locking ... (or delete them if they shouldn't be allowed).\n> \n> \n> I think the constructor and destructor can be = default though, I don't\n> see anything specific to handle there?\n> \n\nOK, I will implement those.\n\n> \n>> +\n>> +\ttemplate\n>> +\tvoid set(std::string const &tag, T const &value)\n>> +\t{\n>> +\t\tstd::scoped_lock lock(mutex_);\n>> +\t\tdata_[tag] = value;\n>> +\t}\n>> +\n>> +\ttemplate\n>> +\tint get(std::string const &tag, T &value) const\n>> +\t{\n>> +\t\tstd::scoped_lock lock(mutex_);\n>> +\t\tauto it = data_.find(tag);\n>> +\t\tif (it == data_.end())\n>> +\t\t\treturn -1;\n> \n> Does std::any provide any way to get the template type of the object so\n> we can assert with an incorrect access?\n\nSomething like that ?\nhttps://en.cppreference.com/w/cpp/utility/any/type\n\n> Perhaps that would then require specific RTTI which maybe we don't want\n> to get into anyway though...\n\nI am not sure if it is worth it...\n\n> \n>> +\t\tvalue = std::any_cast(it->second);\n>> +\t\treturn 0;\n>> +\t}\n>> +\n>> +\tvoid clear()\n>> +\t{\n>> +\t\tstd::scoped_lock lock(mutex_);\n>> +\t\tdata_.clear();\n>> +\t}\n>> +\n>> +\tvoid merge(Metadata &other)\n>> +\t{\n>> +\t\tstd::scoped_lock lock(mutex_, other.mutex_);\n>> +\t\tdata_.merge(other.data_);\n>> +\t}\n>> +\n>> +\ttemplate\n>> +\tT *getLocked(std::string const &tag)\n>> +\t{\n>> +\t\tauto it = data_.find(tag);\n>> +\t\tif (it == data_.end())\n>> +\t\t\treturn nullptr;\n>> +\t\treturn std::any_cast(&it->second);\n>> +\t}\n>> +\n>> +\ttemplate\n>> +\tvoid setLocked(std::string const &tag, T const &value)\n>> +\t{\n>> +\t\tdata_[tag] = value;\n>> +\t}\n>> +\n>> +\tvoid lock() { mutex_.lock(); }\n>> +\tvoid unlock() { mutex_.unlock(); }\n>> +\n>> +private:\n>> +\tmutable std::mutex mutex_;\n> \n> Hrm, I had to look this up. I wonder if we should be using mutable more\n> often.\n> \n> But indeed, this sounds right as it means you can 'get' from a const\n> Metadata and lock the mutex (which requires modifying the otherwise\n> const mutex).\n> \n> \n> \n>> +\tstd::map data_;\n>> +};\n>> +\n>> +} /* namespace ipa */\n>> +\n>> +} /* namespace libcamera */\n>> +\n>> +#endif /* __LIBCAMERA_IPA_LIBIPA_METADATA_H__ */\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 10E92C3225\n\tfor ;\n\tWed, 14 Jul 2021 06:59:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4B85B68525;\n\tWed, 14 Jul 2021 08:59:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A1F926027E\n\tfor ;\n\tWed, 14 Jul 2021 08:59:33 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:e67c:9465:36df:f139])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 26FB9CC;\n\tWed, 14 Jul 2021 08:59:33 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"SCj2JMbb\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626245973;\n\tbh=rxfZ2vUgL/164otcparYhlCLGZjPVKQJvwo9IaL6yzI=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=SCj2JMbbYYCRqQhiEFgC915cKu7H3MRztRsHJTLoxx8RuReH2qbnV3IRZlNM6X7PV\n\tRDfO6fAVLamLAshrMqJK3zuJ8pxXhkh4Dokwns8xrat3JBXeMPKBGt1N3BqVQx7aux\n\tOgSz0QyDpGF7V7j/lJp52FXqKJ6rPGc+BK4Fp1Sg=","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210712131630.73914-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210712131630.73914-2-jeanmichel.hautbois@ideasonboard.com>\n\t<62596562-ae49-4277-c428-5380198f0559@ideasonboard.com>","From":"Jean-Michel Hautbois ","Message-ID":"<6ec0e4f8-b386-348f-7a9a-b45039f22ed7@ideasonboard.com>","Date":"Wed, 14 Jul 2021 08:59:30 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<62596562-ae49-4277-c428-5380198f0559@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata\n\tclass","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18165,"web_url":"https://patchwork.libcamera.org/comment/18165/","msgid":"<0461953b-aa47-c64f-211a-a8f2589f5ea8@ideasonboard.com>","date":"2021-07-14T08:12:17","subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata\n\tclass","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi JM,\n\nOn 14/07/2021 07:59, Jean-Michel Hautbois wrote:\n> Hi Kieran,\n> \n> Thanks for the review :-).\n> \n> On 13/07/2021 16:41, Kieran Bingham wrote:\n>> Hi JM,\n>>\n>> On 12/07/2021 14:16, Jean-Michel Hautbois wrote:\n>>> The Metadata class comes from RPi from which a bit has been removed\n>>> because we don't need it for now.\n>>> All functions are inlined in metadata.h because of the template usage.\n>>\n>> Perhaps this could be better worded:\n>>\n>> \"\"\"\n>> Import the Metadata class from src/ipa/raspberrypi/metadata.cpp to be\n>> able to make use of the component from other IPA modules.\n>> \"\"\"\n>>\n>>\n>>>\n>>> Signed-off-by: Jean-Michel Hautbois \n>>> ---\n>>> src/ipa/ipu3/ipu3.cpp | 1 +\n>>> src/ipa/libipa/meson.build | 6 ++-\n>>> src/ipa/libipa/metadata.cpp | 101 ++++++++++++++++++++++++++++++++++++\n>>> src/ipa/libipa/metadata.h | 90 ++++++++++++++++++++++++++++++++\n>>> 4 files changed, 196 insertions(+), 2 deletions(-)\n>>> create mode 100644 src/ipa/libipa/metadata.cpp\n>>> create mode 100644 src/ipa/libipa/metadata.h\n>>>\n>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>>> index 71698d36..091856f5 100644\n>>> --- a/src/ipa/ipu3/ipu3.cpp\n>>> +++ b/src/ipa/ipu3/ipu3.cpp\n>>> @@ -25,6 +25,7 @@\n>>> #include \"ipu3_agc.h\"\n>>> #include \"ipu3_awb.h\"\n>>> #include \"libipa/camera_sensor_helper.h\"\n>>> +#include \"libipa/metadata.h\"\n>>> \n>>> static constexpr uint32_t kMaxCellWidthPerSet = 160;\n>>> static constexpr uint32_t kMaxCellHeightPerSet = 56;\n>>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n>>> index 3fda7c00..cc4e1cc9 100644\n>>> --- a/src/ipa/libipa/meson.build\n>>> +++ b/src/ipa/libipa/meson.build\n>>> @@ -3,13 +3,15 @@\n>>> libipa_headers = files([\n>>> 'algorithm.h',\n>>> 'camera_sensor_helper.h',\n>>> - 'histogram.h'\n>>> + 'histogram.h',\n>>> + 'metadata.h'\n>>\n>> If you keep a ',' at the end, you don't to modify this line when adding\n>> another entry later.\n>>\n>>\n>>> ])\n>>> \n>>> libipa_sources = files([\n>>> 'algorithm.cpp',\n>>> 'camera_sensor_helper.cpp',\n>>> - 'histogram.cpp'\n>>> + 'histogram.cpp',\n>>> + 'metadata.cpp'\n>>\n>> Same here...\n>>\n>>\n>>> ])\n>>> \n>>> libipa_includes = include_directories('..')\n>>> diff --git a/src/ipa/libipa/metadata.cpp b/src/ipa/libipa/metadata.cpp\n>>> new file mode 100644\n>>> index 00000000..b6aef897\n>>> --- /dev/null\n>>> +++ b/src/ipa/libipa/metadata.cpp\n>>> @@ -0,0 +1,101 @@\n>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>> +/*\n>>> + * Based on the implementation from the Raspberry Pi IPA,\n>>> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.\n>>> + * Copyright (C) 2021, Ideas On Board\n>>> + *\n>>> + * metadata.cpp - libipa metadata class\n>>> + */\n>>> +\n>>> +#include \"metadata.h\"\n>>> +\n>>> +/**\n>>> + * \\file metadata.h\n>>> + * \\brief A metadata class to share objects\n>>\n>> I wouldn't call it sharing objects...\n>>\n>> \"A metadata class to provide key based access to arbitrary metadata types.\n>>\n>>\n>>> + */\n>>> +\n>>> +namespace libcamera {\n>>> +\n>>> +namespace ipa {\n>>> +\n>>> +/**\n>>> + * \\class Metadata\n>>> + * \\brief A simple class for carrying arbitrary metadata, for example\n>>> + * about an image. It is used to exchange data between algorithms.\n>>\n>> I think we need to expand on this to explain some of the constraints too:\n>>\n>>\n>> \"\"\"\n>> Data is stored as a map with a string based key.\n>>\n>> The metadata is stored through a std::any() type which is definable by\n>> the user, and must be correctly known by both the producer and consumer.\n>>\n>> Accessing the metadata with an incorrect type will cause undefined\n>> behaviour.\n>> \"\"\"\n>>\n>> Some of that might be too far towards the implementation details, but in\n>> this instance, I sort of think those implementation details are\n>> important because the implications they introduce.\n>>\n>>\n>>\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn Metadata::Metadata(Metadata const &other)\n>>> + * \\param[in] other A Metadata object\n>>> + *\n>>\n>> I think this is the copy constructor right?\n>>\n>> Lets reference it so:\n>>\n>> \"Copy the data from the \\a other Metadata object to this one.\"\n>>\n>>\n>>\n>>> + * Stores the data from one Metadata to another one\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn Metadata::set(std::string const &tag, T const &value)\n>>> + * \\param[in] tag A string used as the key in a map\n>>> + * \\param[in] value The value to set into the map\n>>\n>> I would probably drop 'in a map' and 'into the map' in these parameter\n>> descriptions.\n>>\n>>\n>>> + *\n>>> + * Sets the value in the map to the tag key. The mutex is\n>>> + * taken for the duration of the block.\n>>\n>>\n>> I would express this as..\n>>\n>> \"This function locks the metadata to protect from concurrent access\"\n>>\n>> (rather than \"the mutex is...\", and on a line/paragraph of its own to\n>> stand out.)\n>>\n>>\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn Metadata::get(std::string const &tag, T &value)\n>>> + * \\param[in] tag A string used as the key in a map\n>>> + * \\param[in] value The value to set into the map\n>>> + *\n>>> + * Gets the value in the map of the tag key. The mutex is\n>>> + * taken for the duration of the block.\n>>> + *\n>>> + * \\return 0 if value is found, -1 if not existent\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn Metadata::clear()\n>>> + * Clear the Metadata map. The mutex is taken for the duration of\n>>> + * the block.\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn Metadata::merge(Metadata &other)\n>>> + * \\param[in] other A metadata to merge with\n>>> + * Merge two Metadata maps. The mutex is taken for the duration of\n>>> + * the block.\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn Metadata::getLocked(std::string const &tag)\n>>> + * \\param[in] tag A string used as the key in a map\n>>> + *\n>>> + * Get the value of the tag key in the map.\n>>> + * This allows in-place access to the Metadata contents,\n>>> + * for which you should be holding the lock.\n>>\n>> I would expand upon this to also state how to lock it, given that it is\n>> part of the API of this class:\n>>\n>>\n>> \"\"\"\n>> This function does not protect against concurrent access, and it is up\n>> to the caller to ensure that the lock is held using \\a lock()\n>> \"\"\"\n>>\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn Metadata::setLocked(std::string const &tag, T const &value)\n>>> + * \\param[in] tag A string used as the key in a map\n>>> + * \\param[in] value The value to set into the map\n>>> + *\n>>> + * Set the value to the tag key in the map.\n>>> + * This allows in-place access to the Metadata contents,\n>>> + * for which you should be holding the lock.\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn Metadata::lock()\n>>> + * Lock the mutex with the standard classes.\n>>> + * e.g. std::lock_guard lock(metadata)\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn Metadata::unlock()\n>>> + * Unlock the mutex with the standard classes.\n>>> + * e.g. std::lock_guard lock(metadata)\n>>> + */\n>>> +\n>>> +} /* namespace ipa */\n>>> +\n>>> +} /* namespace libcamera */\n>>> +\n>>> diff --git a/src/ipa/libipa/metadata.h b/src/ipa/libipa/metadata.h\n>>> new file mode 100644\n>>> index 00000000..9801bece\n>>> --- /dev/null\n>>> +++ b/src/ipa/libipa/metadata.h\n>>> @@ -0,0 +1,90 @@\n>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>> +/*\n>>> + * Based on the implementation from the Raspberry Pi IPA,\n>>> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.\n>>> + * Copyright (C) 2021, Ideas On Board\n>>> + *\n>>> + * metadata.h - libipa metadata class\n>>> + */\n>>> +#ifndef __LIBCAMERA_IPA_LIBIPA_METADATA_H__\n>>> +#define __LIBCAMERA_IPA_LIBIPA_METADATA_H__\n>>> +\n>>> +#include \n>>> +#include \n>>> +#include \n>>> +#include \n>>> +#include \n>>> +\n>>> +namespace libcamera {\n>>> +\n>>> +namespace ipa {\n>>> +\n>>> +class Metadata\n>>> +{\n>>> +public:\n>>> +\tMetadata() = default;\n>>> +\n>>> +\tMetadata(Metadata const &other)\n>>> +\t{\n>>> +\t\tstd::scoped_lock other_lock(other.mutex_);\n>>> +\t\tdata_ = other.data_;\n>>> +\t}\n>>\n>> The C++ rule of five (or is it six?) says if you need to implement one\n>> of the copy/move constructors you need to implement, default (or\n>> delete?) them all:\n>>\n>> https://www.modernescpp.com/index.php/c-core-guidelines-constructors-assignments-and-desctructors\n>>\n>> So we should add appropriate copy assignment, and move constructors with\n>> appropriate locking ... (or delete them if they shouldn't be allowed).\n>>\n>>\n>> I think the constructor and destructor can be = default though, I don't\n>> see anything specific to handle there?\n>>\n> \n> OK, I will implement those.\n\nWhen you do this, please try to be clear in the story your patches tell.\n\nThis is clearly additional code on top of the code which is imported, so\nthe series should do something like:\n\n 1/x ipa: libipa: Import Metadata class from src/ipa/raspberrypi\n Just the direct copy, without any specific changes other than making\n it compile in place\n\n 2/x ipa: libipa: Document Metadata class\n Any documentation that /you/ add.\n\n 3/x ipa: libipa: Implement missing Metadata class copy/move operations\n Any code additions that you add\n\n 4/x .... other?\n\n\n> \n>>\n>>> +\n>>> +\ttemplate\n>>> +\tvoid set(std::string const &tag, T const &value)\n>>> +\t{\n>>> +\t\tstd::scoped_lock lock(mutex_);\n>>> +\t\tdata_[tag] = value;\n>>> +\t}\n>>> +\n>>> +\ttemplate\n>>> +\tint get(std::string const &tag, T &value) const\n>>> +\t{\n>>> +\t\tstd::scoped_lock lock(mutex_);\n>>> +\t\tauto it = data_.find(tag);\n>>> +\t\tif (it == data_.end())\n>>> +\t\t\treturn -1;\n>>\n>> Does std::any provide any way to get the template type of the object so\n>> we can assert with an incorrect access?\n> \n> Something like that ?\n> https://en.cppreference.com/w/cpp/utility/any/type\n> \n>> Perhaps that would then require specific RTTI which maybe we don't want\n>> to get into anyway though...\n> \n> I am not sure if it is worth it...\n> \n>>\n>>> +\t\tvalue = std::any_cast(it->second);\n>>> +\t\treturn 0;\n>>> +\t}\n>>> +\n>>> +\tvoid clear()\n>>> +\t{\n>>> +\t\tstd::scoped_lock lock(mutex_);\n>>> +\t\tdata_.clear();\n>>> +\t}\n>>> +\n>>> +\tvoid merge(Metadata &other)\n>>> +\t{\n>>> +\t\tstd::scoped_lock lock(mutex_, other.mutex_);\n>>> +\t\tdata_.merge(other.data_);\n>>> +\t}\n>>> +\n>>> +\ttemplate\n>>> +\tT *getLocked(std::string const &tag)\n>>> +\t{\n>>> +\t\tauto it = data_.find(tag);\n>>> +\t\tif (it == data_.end())\n>>> +\t\t\treturn nullptr;\n>>> +\t\treturn std::any_cast(&it->second);\n>>> +\t}\n>>> +\n>>> +\ttemplate\n>>> +\tvoid setLocked(std::string const &tag, T const &value)\n>>> +\t{\n>>> +\t\tdata_[tag] = value;\n>>> +\t}\n>>> +\n>>> +\tvoid lock() { mutex_.lock(); }\n>>> +\tvoid unlock() { mutex_.unlock(); }\n>>> +\n>>> +private:\n>>> +\tmutable std::mutex mutex_;\n>>\n>> Hrm, I had to look this up. I wonder if we should be using mutable more\n>> often.\n>>\n>> But indeed, this sounds right as it means you can 'get' from a const\n>> Metadata and lock the mutex (which requires modifying the otherwise\n>> const mutex).\n>>\n>>\n>>\n>>> +\tstd::map data_;\n>>> +};\n>>> +\n>>> +} /* namespace ipa */\n>>> +\n>>> +} /* namespace libcamera */\n>>> +\n>>> +#endif /* __LIBCAMERA_IPA_LIBIPA_METADATA_H__ */\n>>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 361BBC3225\n\tfor ;\n\tWed, 14 Jul 2021 08:12:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9674D68525;\n\tWed, 14 Jul 2021 10:12:21 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C2E7D60282\n\tfor ;\n\tWed, 14 Jul 2021 10:12:20 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4980DCC;\n\tWed, 14 Jul 2021 10:12:20 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"KrMgTnci\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626250340;\n\tbh=cDql4gZtcR/Qp38JInhQraqQxP8eHkzCEyFDuJR213k=;\n\th=From:To:References:Subject:Date:In-Reply-To:From;\n\tb=KrMgTnciPoMRZLXSGMw6Xi4d5YqqBuWdl08x9CzuEfdntxqFFlc3NVzplWPPYOgIg\n\tqU8jd5erSByCrsxrc3W6n/PdjhKtZPQOeoDgKoCLls5aGu1uahjf64m6PXBezectrg\n\tF1AM8XUjuOHe2lVY6d35lhE8Sqn/pXc5H2q/3RJ8=","From":"Kieran Bingham ","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210712131630.73914-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210712131630.73914-2-jeanmichel.hautbois@ideasonboard.com>\n\t<62596562-ae49-4277-c428-5380198f0559@ideasonboard.com>\n\t<6ec0e4f8-b386-348f-7a9a-b45039f22ed7@ideasonboard.com>","Message-ID":"<0461953b-aa47-c64f-211a-a8f2589f5ea8@ideasonboard.com>","Date":"Wed, 14 Jul 2021 09:12:17 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<6ec0e4f8-b386-348f-7a9a-b45039f22ed7@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata\n\tclass","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18166,"web_url":"https://patchwork.libcamera.org/comment/18166/","msgid":"<9f074fcc-6c49-4fa3-2722-cbf0209e696f@ideasonboard.com>","date":"2021-07-14T08:18:52","subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata\n\tclass","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"On 14/07/2021 10:12, Kieran Bingham wrote:\n> Hi JM,\n> \n> On 14/07/2021 07:59, Jean-Michel Hautbois wrote:\n>> Hi Kieran,\n>>\n>> Thanks for the review :-).\n>>\n>> On 13/07/2021 16:41, Kieran Bingham wrote:\n>>> Hi JM,\n>>>\n>>> On 12/07/2021 14:16, Jean-Michel Hautbois wrote:\n>>>> The Metadata class comes from RPi from which a bit has been removed\n>>>> because we don't need it for now.\n>>>> All functions are inlined in metadata.h because of the template usage.\n>>>\n>>> Perhaps this could be better worded:\n>>>\n>>> \"\"\"\n>>> Import the Metadata class from src/ipa/raspberrypi/metadata.cpp to be\n>>> able to make use of the component from other IPA modules.\n>>> \"\"\"\n>>>\n>>>\n>>>>\n>>>> Signed-off-by: Jean-Michel Hautbois \n>>>> ---\n>>>> src/ipa/ipu3/ipu3.cpp | 1 +\n>>>> src/ipa/libipa/meson.build | 6 ++-\n>>>> src/ipa/libipa/metadata.cpp | 101 ++++++++++++++++++++++++++++++++++++\n>>>> src/ipa/libipa/metadata.h | 90 ++++++++++++++++++++++++++++++++\n>>>> 4 files changed, 196 insertions(+), 2 deletions(-)\n>>>> create mode 100644 src/ipa/libipa/metadata.cpp\n>>>> create mode 100644 src/ipa/libipa/metadata.h\n>>>>\n>>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>>>> index 71698d36..091856f5 100644\n>>>> --- a/src/ipa/ipu3/ipu3.cpp\n>>>> +++ b/src/ipa/ipu3/ipu3.cpp\n>>>> @@ -25,6 +25,7 @@\n>>>> #include \"ipu3_agc.h\"\n>>>> #include \"ipu3_awb.h\"\n>>>> #include \"libipa/camera_sensor_helper.h\"\n>>>> +#include \"libipa/metadata.h\"\n>>>> \n>>>> static constexpr uint32_t kMaxCellWidthPerSet = 160;\n>>>> static constexpr uint32_t kMaxCellHeightPerSet = 56;\n>>>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n>>>> index 3fda7c00..cc4e1cc9 100644\n>>>> --- a/src/ipa/libipa/meson.build\n>>>> +++ b/src/ipa/libipa/meson.build\n>>>> @@ -3,13 +3,15 @@\n>>>> libipa_headers = files([\n>>>> 'algorithm.h',\n>>>> 'camera_sensor_helper.h',\n>>>> - 'histogram.h'\n>>>> + 'histogram.h',\n>>>> + 'metadata.h'\n>>>\n>>> If you keep a ',' at the end, you don't to modify this line when adding\n>>> another entry later.\n>>>\n>>>\n>>>> ])\n>>>> \n>>>> libipa_sources = files([\n>>>> 'algorithm.cpp',\n>>>> 'camera_sensor_helper.cpp',\n>>>> - 'histogram.cpp'\n>>>> + 'histogram.cpp',\n>>>> + 'metadata.cpp'\n>>>\n>>> Same here...\n>>>\n>>>\n>>>> ])\n>>>> \n>>>> libipa_includes = include_directories('..')\n>>>> diff --git a/src/ipa/libipa/metadata.cpp b/src/ipa/libipa/metadata.cpp\n>>>> new file mode 100644\n>>>> index 00000000..b6aef897\n>>>> --- /dev/null\n>>>> +++ b/src/ipa/libipa/metadata.cpp\n>>>> @@ -0,0 +1,101 @@\n>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>>> +/*\n>>>> + * Based on the implementation from the Raspberry Pi IPA,\n>>>> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.\n>>>> + * Copyright (C) 2021, Ideas On Board\n>>>> + *\n>>>> + * metadata.cpp - libipa metadata class\n>>>> + */\n>>>> +\n>>>> +#include \"metadata.h\"\n>>>> +\n>>>> +/**\n>>>> + * \\file metadata.h\n>>>> + * \\brief A metadata class to share objects\n>>>\n>>> I wouldn't call it sharing objects...\n>>>\n>>> \"A metadata class to provide key based access to arbitrary metadata types.\n>>>\n>>>\n>>>> + */\n>>>> +\n>>>> +namespace libcamera {\n>>>> +\n>>>> +namespace ipa {\n>>>> +\n>>>> +/**\n>>>> + * \\class Metadata\n>>>> + * \\brief A simple class for carrying arbitrary metadata, for example\n>>>> + * about an image. It is used to exchange data between algorithms.\n>>>\n>>> I think we need to expand on this to explain some of the constraints too:\n>>>\n>>>\n>>> \"\"\"\n>>> Data is stored as a map with a string based key.\n>>>\n>>> The metadata is stored through a std::any() type which is definable by\n>>> the user, and must be correctly known by both the producer and consumer.\n>>>\n>>> Accessing the metadata with an incorrect type will cause undefined\n>>> behaviour.\n>>> \"\"\"\n>>>\n>>> Some of that might be too far towards the implementation details, but in\n>>> this instance, I sort of think those implementation details are\n>>> important because the implications they introduce.\n>>>\n>>>\n>>>\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\fn Metadata::Metadata(Metadata const &other)\n>>>> + * \\param[in] other A Metadata object\n>>>> + *\n>>>\n>>> I think this is the copy constructor right?\n>>>\n>>> Lets reference it so:\n>>>\n>>> \"Copy the data from the \\a other Metadata object to this one.\"\n>>>\n>>>\n>>>\n>>>> + * Stores the data from one Metadata to another one\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\fn Metadata::set(std::string const &tag, T const &value)\n>>>> + * \\param[in] tag A string used as the key in a map\n>>>> + * \\param[in] value The value to set into the map\n>>>\n>>> I would probably drop 'in a map' and 'into the map' in these parameter\n>>> descriptions.\n>>>\n>>>\n>>>> + *\n>>>> + * Sets the value in the map to the tag key. The mutex is\n>>>> + * taken for the duration of the block.\n>>>\n>>>\n>>> I would express this as..\n>>>\n>>> \"This function locks the metadata to protect from concurrent access\"\n>>>\n>>> (rather than \"the mutex is...\", and on a line/paragraph of its own to\n>>> stand out.)\n>>>\n>>>\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\fn Metadata::get(std::string const &tag, T &value)\n>>>> + * \\param[in] tag A string used as the key in a map\n>>>> + * \\param[in] value The value to set into the map\n>>>> + *\n>>>> + * Gets the value in the map of the tag key. The mutex is\n>>>> + * taken for the duration of the block.\n>>>> + *\n>>>> + * \\return 0 if value is found, -1 if not existent\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\fn Metadata::clear()\n>>>> + * Clear the Metadata map. The mutex is taken for the duration of\n>>>> + * the block.\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\fn Metadata::merge(Metadata &other)\n>>>> + * \\param[in] other A metadata to merge with\n>>>> + * Merge two Metadata maps. The mutex is taken for the duration of\n>>>> + * the block.\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\fn Metadata::getLocked(std::string const &tag)\n>>>> + * \\param[in] tag A string used as the key in a map\n>>>> + *\n>>>> + * Get the value of the tag key in the map.\n>>>> + * This allows in-place access to the Metadata contents,\n>>>> + * for which you should be holding the lock.\n>>>\n>>> I would expand upon this to also state how to lock it, given that it is\n>>> part of the API of this class:\n>>>\n>>>\n>>> \"\"\"\n>>> This function does not protect against concurrent access, and it is up\n>>> to the caller to ensure that the lock is held using \\a lock()\n>>> \"\"\"\n>>>\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\fn Metadata::setLocked(std::string const &tag, T const &value)\n>>>> + * \\param[in] tag A string used as the key in a map\n>>>> + * \\param[in] value The value to set into the map\n>>>> + *\n>>>> + * Set the value to the tag key in the map.\n>>>> + * This allows in-place access to the Metadata contents,\n>>>> + * for which you should be holding the lock.\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\fn Metadata::lock()\n>>>> + * Lock the mutex with the standard classes.\n>>>> + * e.g. std::lock_guard lock(metadata)\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\fn Metadata::unlock()\n>>>> + * Unlock the mutex with the standard classes.\n>>>> + * e.g. std::lock_guard lock(metadata)\n>>>> + */\n>>>> +\n>>>> +} /* namespace ipa */\n>>>> +\n>>>> +} /* namespace libcamera */\n>>>> +\n>>>> diff --git a/src/ipa/libipa/metadata.h b/src/ipa/libipa/metadata.h\n>>>> new file mode 100644\n>>>> index 00000000..9801bece\n>>>> --- /dev/null\n>>>> +++ b/src/ipa/libipa/metadata.h\n>>>> @@ -0,0 +1,90 @@\n>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>>> +/*\n>>>> + * Based on the implementation from the Raspberry Pi IPA,\n>>>> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.\n>>>> + * Copyright (C) 2021, Ideas On Board\n>>>> + *\n>>>> + * metadata.h - libipa metadata class\n>>>> + */\n>>>> +#ifndef __LIBCAMERA_IPA_LIBIPA_METADATA_H__\n>>>> +#define __LIBCAMERA_IPA_LIBIPA_METADATA_H__\n>>>> +\n>>>> +#include \n>>>> +#include \n>>>> +#include \n>>>> +#include \n>>>> +#include \n>>>> +\n>>>> +namespace libcamera {\n>>>> +\n>>>> +namespace ipa {\n>>>> +\n>>>> +class Metadata\n>>>> +{\n>>>> +public:\n>>>> +\tMetadata() = default;\n>>>> +\n>>>> +\tMetadata(Metadata const &other)\n>>>> +\t{\n>>>> +\t\tstd::scoped_lock other_lock(other.mutex_);\n>>>> +\t\tdata_ = other.data_;\n>>>> +\t}\n>>>\n>>> The C++ rule of five (or is it six?) says if you need to implement one\n>>> of the copy/move constructors you need to implement, default (or\n>>> delete?) them all:\n>>>\n>>> https://www.modernescpp.com/index.php/c-core-guidelines-constructors-assignments-and-desctructors\n>>>\n>>> So we should add appropriate copy assignment, and move constructors with\n>>> appropriate locking ... (or delete them if they shouldn't be allowed).\n>>>\n>>>\n>>> I think the constructor and destructor can be = default though, I don't\n>>> see anything specific to handle there?\n>>>\n>>\n>> OK, I will implement those.\n> \n> When you do this, please try to be clear in the story your patches tell.\n> \n> This is clearly additional code on top of the code which is imported, so\n> the series should do something like:\n> \n> 1/x ipa: libipa: Import Metadata class from src/ipa/raspberrypi\n> Just the direct copy, without any specific changes other than making\n> it compile in place\n\nDoxygen documentation needed to make it compile without warnings is not\nconsidered in this first patch ?\n\n> 2/x ipa: libipa: Document Metadata class\n> Any documentation that /you/ add.\n> \n> 3/x ipa: libipa: Implement missing Metadata class copy/move operations\n> Any code additions that you add\n> \n> 4/x .... other?\n> \n> \n>>\n>>>\n>>>> +\n>>>> +\ttemplate\n>>>> +\tvoid set(std::string const &tag, T const &value)\n>>>> +\t{\n>>>> +\t\tstd::scoped_lock lock(mutex_);\n>>>> +\t\tdata_[tag] = value;\n>>>> +\t}\n>>>> +\n>>>> +\ttemplate\n>>>> +\tint get(std::string const &tag, T &value) const\n>>>> +\t{\n>>>> +\t\tstd::scoped_lock lock(mutex_);\n>>>> +\t\tauto it = data_.find(tag);\n>>>> +\t\tif (it == data_.end())\n>>>> +\t\t\treturn -1;\n>>>\n>>> Does std::any provide any way to get the template type of the object so\n>>> we can assert with an incorrect access?\n>>\n>> Something like that ?\n>> https://en.cppreference.com/w/cpp/utility/any/type\n>>\n>>> Perhaps that would then require specific RTTI which maybe we don't want\n>>> to get into anyway though...\n>>\n>> I am not sure if it is worth it...\n>>\n>>>\n>>>> +\t\tvalue = std::any_cast(it->second);\n>>>> +\t\treturn 0;\n>>>> +\t}\n>>>> +\n>>>> +\tvoid clear()\n>>>> +\t{\n>>>> +\t\tstd::scoped_lock lock(mutex_);\n>>>> +\t\tdata_.clear();\n>>>> +\t}\n>>>> +\n>>>> +\tvoid merge(Metadata &other)\n>>>> +\t{\n>>>> +\t\tstd::scoped_lock lock(mutex_, other.mutex_);\n>>>> +\t\tdata_.merge(other.data_);\n>>>> +\t}\n>>>> +\n>>>> +\ttemplate\n>>>> +\tT *getLocked(std::string const &tag)\n>>>> +\t{\n>>>> +\t\tauto it = data_.find(tag);\n>>>> +\t\tif (it == data_.end())\n>>>> +\t\t\treturn nullptr;\n>>>> +\t\treturn std::any_cast(&it->second);\n>>>> +\t}\n>>>> +\n>>>> +\ttemplate\n>>>> +\tvoid setLocked(std::string const &tag, T const &value)\n>>>> +\t{\n>>>> +\t\tdata_[tag] = value;\n>>>> +\t}\n>>>> +\n>>>> +\tvoid lock() { mutex_.lock(); }\n>>>> +\tvoid unlock() { mutex_.unlock(); }\n>>>> +\n>>>> +private:\n>>>> +\tmutable std::mutex mutex_;\n>>>\n>>> Hrm, I had to look this up. I wonder if we should be using mutable more\n>>> often.\n>>>\n>>> But indeed, this sounds right as it means you can 'get' from a const\n>>> Metadata and lock the mutex (which requires modifying the otherwise\n>>> const mutex).\n>>>\n>>>\n>>>\n>>>> +\tstd::map data_;\n>>>> +};\n>>>> +\n>>>> +} /* namespace ipa */\n>>>> +\n>>>> +} /* namespace libcamera */\n>>>> +\n>>>> +#endif /* __LIBCAMERA_IPA_LIBIPA_METADATA_H__ */\n>>>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 02201C3226\n\tfor ;\n\tWed, 14 Jul 2021 08:18:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 74A3268525;\n\tWed, 14 Jul 2021 10:18:56 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C090860282\n\tfor ;\n\tWed, 14 Jul 2021 10:18:54 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:e67c:9465:36df:f139])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5A170CC;\n\tWed, 14 Jul 2021 10:18:54 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"b2WgXTHM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626250734;\n\tbh=kxP5VEQu7ytbs3zhnpyqSTqtC/JjPJii8dVvnducZTo=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=b2WgXTHMJhMw+X3W0Oj0paQadnzaPtV8UTSZmnir7A43FmP02Qh2BlGY193ub2QkB\n\tiNAHueZQBiEsZtU1vWsa1VvUgRl70kNDAN5ps+dWuy91aGfqacUMx++NYpWEArYTru\n\tbc28S8ugNYXVU0L9LZqd3lpTuZmt3/LYIUrydpfk=","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210712131630.73914-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210712131630.73914-2-jeanmichel.hautbois@ideasonboard.com>\n\t<62596562-ae49-4277-c428-5380198f0559@ideasonboard.com>\n\t<6ec0e4f8-b386-348f-7a9a-b45039f22ed7@ideasonboard.com>\n\t<0461953b-aa47-c64f-211a-a8f2589f5ea8@ideasonboard.com>","From":"Jean-Michel Hautbois ","Message-ID":"<9f074fcc-6c49-4fa3-2722-cbf0209e696f@ideasonboard.com>","Date":"Wed, 14 Jul 2021 10:18:52 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<0461953b-aa47-c64f-211a-a8f2589f5ea8@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata\n\tclass","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18167,"web_url":"https://patchwork.libcamera.org/comment/18167/","msgid":"","date":"2021-07-14T08:45:21","subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata\n\tclass","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Kieran and JM,\n\nOn Wed, 14 Jul 2021 at 09:12, Kieran Bingham <\nkieran.bingham@ideasonboard.com> wrote:\n\n> Hi JM,\n>\n> On 14/07/2021 07:59, Jean-Michel Hautbois wrote:\n> > Hi Kieran,\n> >\n> > Thanks for the review :-).\n> >\n> > On 13/07/2021 16:41, Kieran Bingham wrote:\n> >> Hi JM,\n> >>\n> >> On 12/07/2021 14:16, Jean-Michel Hautbois wrote:\n> >>> The Metadata class comes from RPi from which a bit has been removed\n> >>> because we don't need it for now.\n> >>> All functions are inlined in metadata.h because of the template usage.\n> >>\n> >> Perhaps this could be better worded:\n> >>\n> >> \"\"\"\n> >> Import the Metadata class from src/ipa/raspberrypi/metadata.cpp to be\n> >> able to make use of the component from other IPA modules.\n> >> \"\"\"\n> >>\n> >>\n> >>>\n> >>> Signed-off-by: Jean-Michel Hautbois <\n> jeanmichel.hautbois@ideasonboard.com>\n> >>> ---\n> >>> src/ipa/ipu3/ipu3.cpp | 1 +\n> >>> src/ipa/libipa/meson.build | 6 ++-\n> >>> src/ipa/libipa/metadata.cpp | 101 ++++++++++++++++++++++++++++++++++++\n> >>> src/ipa/libipa/metadata.h | 90 ++++++++++++++++++++++++++++++++\n> >>> 4 files changed, 196 insertions(+), 2 deletions(-)\n> >>> create mode 100644 src/ipa/libipa/metadata.cpp\n> >>> create mode 100644 src/ipa/libipa/metadata.h\n> >>>\n> >>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> >>> index 71698d36..091856f5 100644\n> >>> --- a/src/ipa/ipu3/ipu3.cpp\n> >>> +++ b/src/ipa/ipu3/ipu3.cpp\n> >>> @@ -25,6 +25,7 @@\n> >>> #include \"ipu3_agc.h\"\n> >>> #include \"ipu3_awb.h\"\n> >>> #include \"libipa/camera_sensor_helper.h\"\n> >>> +#include \"libipa/metadata.h\"\n> >>>\n> >>> static constexpr uint32_t kMaxCellWidthPerSet = 160;\n> >>> static constexpr uint32_t kMaxCellHeightPerSet = 56;\n> >>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> >>> index 3fda7c00..cc4e1cc9 100644\n> >>> --- a/src/ipa/libipa/meson.build\n> >>> +++ b/src/ipa/libipa/meson.build\n> >>> @@ -3,13 +3,15 @@\n> >>> libipa_headers = files([\n> >>> 'algorithm.h',\n> >>> 'camera_sensor_helper.h',\n> >>> - 'histogram.h'\n> >>> + 'histogram.h',\n> >>> + 'metadata.h'\n> >>\n> >> If you keep a ',' at the end, you don't to modify this line when adding\n> >> another entry later.\n> >>\n> >>\n> >>> ])\n> >>>\n> >>> libipa_sources = files([\n> >>> 'algorithm.cpp',\n> >>> 'camera_sensor_helper.cpp',\n> >>> - 'histogram.cpp'\n> >>> + 'histogram.cpp',\n> >>> + 'metadata.cpp'\n> >>\n> >> Same here...\n> >>\n> >>\n> >>> ])\n> >>>\n> >>> libipa_includes = include_directories('..')\n> >>> diff --git a/src/ipa/libipa/metadata.cpp b/src/ipa/libipa/metadata.cpp\n> >>> new file mode 100644\n> >>> index 00000000..b6aef897\n> >>> --- /dev/null\n> >>> +++ b/src/ipa/libipa/metadata.cpp\n> >>> @@ -0,0 +1,101 @@\n> >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >>> +/*\n> >>> + * Based on the implementation from the Raspberry Pi IPA,\n> >>> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.\n> >>> + * Copyright (C) 2021, Ideas On Board\n> >>> + *\n> >>> + * metadata.cpp - libipa metadata class\n> >>> + */\n> >>> +\n> >>> +#include \"metadata.h\"\n> >>> +\n> >>> +/**\n> >>> + * \\file metadata.h\n> >>> + * \\brief A metadata class to share objects\n> >>\n> >> I wouldn't call it sharing objects...\n> >>\n> >> \"A metadata class to provide key based access to arbitrary metadata\n> types.\n> >>\n> >>\n> >>> + */\n> >>> +\n> >>> +namespace libcamera {\n> >>> +\n> >>> +namespace ipa {\n> >>> +\n> >>> +/**\n> >>> + * \\class Metadata\n> >>> + * \\brief A simple class for carrying arbitrary metadata, for example\n> >>> + * about an image. It is used to exchange data between algorithms.\n> >>\n> >> I think we need to expand on this to explain some of the constraints\n> too:\n> >>\n> >>\n> >> \"\"\"\n> >> Data is stored as a map with a string based key.\n> >>\n> >> The metadata is stored through a std::any() type which is definable by\n> >> the user, and must be correctly known by both the producer and consumer.\n> >>\n> >> Accessing the metadata with an incorrect type will cause undefined\n> >> behaviour.\n> >> \"\"\"\n> >>\n> >> Some of that might be too far towards the implementation details, but in\n> >> this instance, I sort of think those implementation details are\n> >> important because the implications they introduce.\n> >>\n> >>\n> >>\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn Metadata::Metadata(Metadata const &other)\n> >>> + * \\param[in] other A Metadata object\n> >>> + *\n> >>\n> >> I think this is the copy constructor right?\n> >>\n> >> Lets reference it so:\n> >>\n> >> \"Copy the data from the \\a other Metadata object to this one.\"\n> >>\n> >>\n> >>\n> >>> + * Stores the data from one Metadata to another one\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn Metadata::set(std::string const &tag, T const &value)\n> >>> + * \\param[in] tag A string used as the key in a map\n> >>> + * \\param[in] value The value to set into the map\n> >>\n> >> I would probably drop 'in a map' and 'into the map' in these parameter\n> >> descriptions.\n> >>\n> >>\n> >>> + *\n> >>> + * Sets the value in the map to the tag key. The mutex is\n> >>> + * taken for the duration of the block.\n> >>\n> >>\n> >> I would express this as..\n> >>\n> >> \"This function locks the metadata to protect from concurrent access\"\n> >>\n> >> (rather than \"the mutex is...\", and on a line/paragraph of its own to\n> >> stand out.)\n> >>\n> >>\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn Metadata::get(std::string const &tag, T &value)\n> >>> + * \\param[in] tag A string used as the key in a map\n> >>> + * \\param[in] value The value to set into the map\n> >>> + *\n> >>> + * Gets the value in the map of the tag key. The mutex is\n> >>> + * taken for the duration of the block.\n> >>> + *\n> >>> + * \\return 0 if value is found, -1 if not existent\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn Metadata::clear()\n> >>> + * Clear the Metadata map. The mutex is taken for the duration of\n> >>> + * the block.\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn Metadata::merge(Metadata &other)\n> >>> + * \\param[in] other A metadata to merge with\n> >>> + * Merge two Metadata maps. The mutex is taken for the duration of\n> >>> + * the block.\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn Metadata::getLocked(std::string const &tag)\n> >>> + * \\param[in] tag A string used as the key in a map\n> >>> + *\n> >>> + * Get the value of the tag key in the map.\n> >>> + * This allows in-place access to the Metadata contents,\n> >>> + * for which you should be holding the lock.\n> >>\n> >> I would expand upon this to also state how to lock it, given that it is\n> >> part of the API of this class:\n> >>\n> >>\n> >> \"\"\"\n> >> This function does not protect against concurrent access, and it is up\n> >> to the caller to ensure that the lock is held using \\a lock()\n> >> \"\"\"\n> >>\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn Metadata::setLocked(std::string const &tag, T const &value)\n> >>> + * \\param[in] tag A string used as the key in a map\n> >>> + * \\param[in] value The value to set into the map\n> >>> + *\n> >>> + * Set the value to the tag key in the map.\n> >>> + * This allows in-place access to the Metadata contents,\n> >>> + * for which you should be holding the lock.\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn Metadata::lock()\n> >>> + * Lock the mutex with the standard classes.\n> >>> + * e.g. std::lock_guard lock(metadata)\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn Metadata::unlock()\n> >>> + * Unlock the mutex with the standard classes.\n> >>> + * e.g. std::lock_guard lock(metadata)\n> >>> + */\n> >>> +\n> >>> +} /* namespace ipa */\n> >>> +\n> >>> +} /* namespace libcamera */\n> >>> +\n> >>> diff --git a/src/ipa/libipa/metadata.h b/src/ipa/libipa/metadata.h\n> >>> new file mode 100644\n> >>> index 00000000..9801bece\n> >>> --- /dev/null\n> >>> +++ b/src/ipa/libipa/metadata.h\n> >>> @@ -0,0 +1,90 @@\n> >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >>> +/*\n> >>> + * Based on the implementation from the Raspberry Pi IPA,\n> >>> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.\n> >>> + * Copyright (C) 2021, Ideas On Board\n> >>> + *\n> >>> + * metadata.h - libipa metadata class\n> >>> + */\n> >>> +#ifndef __LIBCAMERA_IPA_LIBIPA_METADATA_H__\n> >>> +#define __LIBCAMERA_IPA_LIBIPA_METADATA_H__\n> >>> +\n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +\n> >>> +namespace libcamera {\n> >>> +\n> >>> +namespace ipa {\n> >>> +\n> >>> +class Metadata\n> >>> +{\n> >>> +public:\n> >>> + Metadata() = default;\n> >>> +\n> >>> + Metadata(Metadata const &other)\n> >>> + {\n> >>> + std::scoped_lock other_lock(other.mutex_);\n> >>> + data_ = other.data_;\n> >>> + }\n> >>\n> >> The C++ rule of five (or is it six?) says if you need to implement one\n> >> of the copy/move constructors you need to implement, default (or\n> >> delete?) them all:\n> >>\n> >>\n> https://www.modernescpp.com/index.php/c-core-guidelines-constructors-assignments-and-desctructors\n> >>\n> >> So we should add appropriate copy assignment, and move constructors with\n> >> appropriate locking ... (or delete them if they shouldn't be allowed).\n> >>\n> >>\n> >> I think the constructor and destructor can be = default though, I don't\n> >> see anything specific to handle there?\n> >>\n> >\n> > OK, I will implement those.\n>\n> When you do this, please try to be clear in the story your patches tell.\n>\n\nYou could re-use the existing copy/move constructors and operators from our\nimplementation\nfor these.\n\nRegards,\nNaush\n\n\n\n>\n> This is clearly additional code on top of the code which is imported, so\n> the series should do something like:\n>\n> 1/x ipa: libipa: Import Metadata class from src/ipa/raspberrypi\n> Just the direct copy, without any specific changes other than making\n> it compile in place\n>\n> 2/x ipa: libipa: Document Metadata class\n> Any documentation that /you/ add.\n>\n> 3/x ipa: libipa: Implement missing Metadata class copy/move operations\n> Any code additions that you add\n>\n> 4/x .... other?\n>\n>\n> >\n> >>\n> >>> +\n> >>> + template\n> >>> + void set(std::string const &tag, T const &value)\n> >>> + {\n> >>> + std::scoped_lock lock(mutex_);\n> >>> + data_[tag] = value;\n> >>> + }\n> >>> +\n> >>> + template\n> >>> + int get(std::string const &tag, T &value) const\n> >>> + {\n> >>> + std::scoped_lock lock(mutex_);\n> >>> + auto it = data_.find(tag);\n> >>> + if (it == data_.end())\n> >>> + return -1;\n> >>\n> >> Does std::any provide any way to get the template type of the object so\n> >> we can assert with an incorrect access?\n> >\n> > Something like that ?\n> > https://en.cppreference.com/w/cpp/utility/any/type\n> >\n> >> Perhaps that would then require specific RTTI which maybe we don't want\n> >> to get into anyway though...\n> >\n> > I am not sure if it is worth it...\n> >\n> >>\n> >>> + value = std::any_cast(it->second);\n> >>> + return 0;\n> >>> + }\n> >>> +\n> >>> + void clear()\n> >>> + {\n> >>> + std::scoped_lock lock(mutex_);\n> >>> + data_.clear();\n> >>> + }\n> >>> +\n> >>> + void merge(Metadata &other)\n> >>> + {\n> >>> + std::scoped_lock lock(mutex_, other.mutex_);\n> >>> + data_.merge(other.data_);\n> >>> + }\n> >>> +\n> >>> + template\n> >>> + T *getLocked(std::string const &tag)\n> >>> + {\n> >>> + auto it = data_.find(tag);\n> >>> + if (it == data_.end())\n> >>> + return nullptr;\n> >>> + return std::any_cast(&it->second);\n> >>> + }\n> >>> +\n> >>> + template\n> >>> + void setLocked(std::string const &tag, T const &value)\n> >>> + {\n> >>> + data_[tag] = value;\n> >>> + }\n> >>> +\n> >>> + void lock() { mutex_.lock(); }\n> >>> + void unlock() { mutex_.unlock(); }\n> >>> +\n> >>> +private:\n> >>> + mutable std::mutex mutex_;\n> >>\n> >> Hrm, I had to look this up. I wonder if we should be using mutable more\n> >> often.\n> >>\n> >> But indeed, this sounds right as it means you can 'get' from a const\n> >> Metadata and lock the mutex (which requires modifying the otherwise\n> >> const mutex).\n> >>\n> >>\n> >>\n> >>> + std::map data_;\n> >>> +};\n> >>> +\n> >>> +} /* namespace ipa */\n> >>> +\n> >>> +} /* namespace libcamera */\n> >>> +\n> >>> +#endif /* __LIBCAMERA_IPA_LIBIPA_METADATA_H__ */\n> >>>\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 89DA9C3225\n\tfor ;\n\tWed, 14 Jul 2021 08:45:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F312868525;\n\tWed, 14 Jul 2021 10:45:39 +0200 (CEST)","from mail-lj1-x234.google.com (mail-lj1-x234.google.com\n\t[IPv6:2a00:1450:4864:20::234])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 56B3460282\n\tfor ;\n\tWed, 14 Jul 2021 10:45:38 +0200 (CEST)","by mail-lj1-x234.google.com with SMTP id r16so2226947ljk.9\n\tfor ;\n\tWed, 14 Jul 2021 01:45:38 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"jpt2bntB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=OruSV797W8LIjaaWbsU5Kqq6KW3xo+O8uI4W8klgb2M=;\n\tb=jpt2bntBbgvMfBHUZNgQugy/lOPRxCGxcokGu2oTrfehjH2a83r2UaIhVegv3piFus\n\tI7bHS45sHL+oDGix/oi+6MZvLN05sCpzH5P5JX8vDGyC0WOvF+BkuONWzn5Gf24LXDj7\n\tf/EY/FR2qr48KHbSaWcwPnzbDqOZoOG9Za+SPqIme1SRTYSFVcOeXEMCXK9z+4RkD4z+\n\tjIc2FhMEo2grxg2v7O2RZ6n1IYnZOzYynoRXgDpe4MrZJWOvOEN+fpa05GfxlY+2CgtV\n\to9aeDKXOrZbXRXZ1ZUXv4B/Do41Mn1FBzIsmSCNRlzqCTRS7nzcqedOyxAci9XeG4x5W\n\tSwLQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=OruSV797W8LIjaaWbsU5Kqq6KW3xo+O8uI4W8klgb2M=;\n\tb=kfUTIvh2/UBXNf3vGxDA0QBwyFMsK1OSV4auXGAQ1Ao5FCnuUAXp9Vg9xnOlZIUi9m\n\ttSSTmIvIamzKAg5RzCGeIa64bmUU2pVIVV4o5Zr0FdOESN3ozUbTPAOoD2faLHdclZd+\n\tA7MleOFLe6TgmfTujYTjaJZ+bp7m5r874E/4kBcd2r6FGFG9y7n5l1WuxBYgeIcMDHhR\n\t6ZVY0puB457GQ72dhCP4hwnJOnlDMoYZOgZDAn6wqA4b2ct0d8BkSVjWoSin9M/eyEJu\n\tNVntVLx4fwsZNraj4/+bn++/mKeDT0CUVjK0Z1KqFJways6y5kJCbTBrqCUoM6AfHmAB\n\tr9/A==","X-Gm-Message-State":"AOAM530Il/n1P9uF6xBf6vVF7uabdygbh8BKQJe4BP4p15zQzjtU2Pm8\n\tKzOjZuBw1R43Dpdc2+3Zl14GAESJFpy2icmDsxqrx0OoGaM=","X-Google-Smtp-Source":"ABdhPJytGEuKYPwSzY9cm2Urh+86e7QFzSy1ZsGJplgq/OapByM/P9uU0CsPNNLm9xxrC6jwBPDAk+up9KRb73oJwTo=","X-Received":"by 2002:a2e:8905:: with SMTP id d5mr8008228lji.400.1626252337525;\n\tWed, 14 Jul 2021 01:45:37 -0700 (PDT)","MIME-Version":"1.0","References":"<20210712131630.73914-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210712131630.73914-2-jeanmichel.hautbois@ideasonboard.com>\n\t<62596562-ae49-4277-c428-5380198f0559@ideasonboard.com>\n\t<6ec0e4f8-b386-348f-7a9a-b45039f22ed7@ideasonboard.com>\n\t<0461953b-aa47-c64f-211a-a8f2589f5ea8@ideasonboard.com>","In-Reply-To":"<0461953b-aa47-c64f-211a-a8f2589f5ea8@ideasonboard.com>","From":"Naushir Patuck ","Date":"Wed, 14 Jul 2021 09:45:21 +0100","Message-ID":"","To":"Kieran Bingham ","Content-Type":"multipart/alternative; boundary=\"000000000000e4caf805c7116086\"","Subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata\n\tclass","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18168,"web_url":"https://patchwork.libcamera.org/comment/18168/","msgid":"","date":"2021-07-14T08:48:53","subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata\n\tclass","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Naush,\n\nOn 14/07/2021 10:45, Naushir Patuck wrote:\n> Hi Kieran and JM,\n> \n> On Wed, 14 Jul 2021 at 09:12, Kieran Bingham\n> > wrote:\n> \n> Hi JM,\n> \n> On 14/07/2021 07:59, Jean-Michel Hautbois wrote:\n> > Hi Kieran,\n> >\n> > Thanks for the review :-).\n> >\n> > On 13/07/2021 16:41, Kieran Bingham wrote:\n> >> Hi JM,\n> >>\n> >> On 12/07/2021 14:16, Jean-Michel Hautbois wrote:\n> >>> The Metadata class comes from RPi from which a bit has been removed\n> >>> because we don't need it for now.\n> >>> All functions are inlined in metadata.h because of the template\n> usage.\n> >>\n> >> Perhaps this could be better worded:\n> >>\n> >> \"\"\"\n> >> Import the Metadata class from src/ipa/raspberrypi/metadata.cpp to be\n> >> able to make use of the component from other IPA modules.\n> >> \"\"\"\n> >>\n> >>\n> >>>\n> >>> Signed-off-by: Jean-Michel Hautbois\n> >\n> >>> ---\n> >>>  src/ipa/ipu3/ipu3.cpp       |   1 +\n> >>>  src/ipa/libipa/meson.build  |   6 ++-\n> >>>  src/ipa/libipa/metadata.cpp | 101\n> ++++++++++++++++++++++++++++++++++++\n> >>>  src/ipa/libipa/metadata.h   |  90 ++++++++++++++++++++++++++++++++\n> >>>  4 files changed, 196 insertions(+), 2 deletions(-)\n> >>>  create mode 100644 src/ipa/libipa/metadata.cpp\n> >>>  create mode 100644 src/ipa/libipa/metadata.h\n> >>>\n> >>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> >>> index 71698d36..091856f5 100644\n> >>> --- a/src/ipa/ipu3/ipu3.cpp\n> >>> +++ b/src/ipa/ipu3/ipu3.cpp\n> >>> @@ -25,6 +25,7 @@\n> >>>  #include \"ipu3_agc.h\"\n> >>>  #include \"ipu3_awb.h\"\n> >>>  #include \"libipa/camera_sensor_helper.h\"\n> >>> +#include \"libipa/metadata.h\"\n> >>> \n> >>>  static constexpr uint32_t kMaxCellWidthPerSet = 160;\n> >>>  static constexpr uint32_t kMaxCellHeightPerSet = 56;\n> >>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> >>> index 3fda7c00..cc4e1cc9 100644\n> >>> --- a/src/ipa/libipa/meson.build\n> >>> +++ b/src/ipa/libipa/meson.build\n> >>> @@ -3,13 +3,15 @@\n> >>>  libipa_headers = files([\n> >>>      'algorithm.h',\n> >>>      'camera_sensor_helper.h',\n> >>> -    'histogram.h'\n> >>> +    'histogram.h',\n> >>> +    'metadata.h'\n> >>\n> >> If you keep a ',' at the end, you don't to modify this line when\n> adding\n> >> another entry later.\n> >>\n> >>\n> >>>  ])\n> >>> \n> >>>  libipa_sources = files([\n> >>>      'algorithm.cpp',\n> >>>      'camera_sensor_helper.cpp',\n> >>> -    'histogram.cpp'\n> >>> +    'histogram.cpp',\n> >>> +    'metadata.cpp'\n> >>\n> >> Same here...\n> >>\n> >>\n> >>>  ])\n> >>> \n> >>>  libipa_includes = include_directories('..')\n> >>> diff --git a/src/ipa/libipa/metadata.cpp\n> b/src/ipa/libipa/metadata.cpp\n> >>> new file mode 100644\n> >>> index 00000000..b6aef897\n> >>> --- /dev/null\n> >>> +++ b/src/ipa/libipa/metadata.cpp\n> >>> @@ -0,0 +1,101 @@\n> >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >>> +/*\n> >>> + * Based on the implementation from the Raspberry Pi IPA,\n> >>> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.\n> >>> + * Copyright (C) 2021, Ideas On Board\n> >>> + *\n> >>> + * metadata.cpp -  libipa metadata class\n> >>> + */\n> >>> +\n> >>> +#include \"metadata.h\"\n> >>> +\n> >>> +/**\n> >>> + * \\file metadata.h\n> >>> + * \\brief A metadata class to share objects\n> >>\n> >> I wouldn't call it sharing objects...\n> >>\n> >> \"A metadata class to provide key based access to arbitrary\n> metadata types.\n> >>\n> >>\n> >>> + */\n> >>> +\n> >>> +namespace libcamera {\n> >>> +\n> >>> +namespace ipa {\n> >>> +\n> >>> +/**\n> >>> + * \\class Metadata\n> >>> + * \\brief A simple class for carrying arbitrary metadata, for\n> example\n> >>> + * about an image. It is used to exchange data between algorithms.\n> >>\n> >> I think we need to expand on this to explain some of the\n> constraints too:\n> >>\n> >>\n> >> \"\"\"\n> >> Data is stored as a map with a string based key.\n> >>\n> >> The metadata is stored through a std::any() type which is\n> definable by\n> >> the user, and must be correctly known by both the producer and\n> consumer.\n> >>\n> >> Accessing the metadata with an incorrect type will cause undefined\n> >> behaviour.\n> >> \"\"\"\n> >>\n> >> Some of that might be too far towards the implementation details,\n> but in\n> >> this instance, I sort of think those implementation details are\n> >> important because the implications they introduce.\n> >>\n> >>\n> >>\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn Metadata::Metadata(Metadata const &other)\n> >>> + * \\param[in] other A Metadata object\n> >>> + *\n> >>\n> >> I think this is the copy constructor right?\n> >>\n> >> Lets reference it so:\n> >>\n> >> \"Copy the data from the \\a other Metadata object to this one.\"\n> >>\n> >>\n> >>\n> >>> + * Stores the data from one Metadata to another one\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn Metadata::set(std::string const &tag, T const &value)\n> >>> + * \\param[in] tag A string used as the key in a map\n> >>> + * \\param[in] value The value to set into the map\n> >>\n> >> I would probably drop 'in a map' and 'into the map' in these\n> parameter\n> >> descriptions.\n> >>\n> >>\n> >>> + *\n> >>> + * Sets the value in the map to the tag key. The mutex is\n> >>> + * taken for the duration of the block.\n> >>\n> >>\n> >> I would express this as..\n> >>\n> >> \"This function locks the metadata to protect from concurrent access\"\n> >>\n> >> (rather than \"the mutex is...\", and on a line/paragraph of its own to\n> >> stand out.)\n> >>\n> >>\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn Metadata::get(std::string const &tag, T &value)\n> >>> + * \\param[in] tag A string used as the key in a map\n> >>> + * \\param[in] value The value to set into the map\n> >>> + *\n> >>> + * Gets the value in the map of the tag key. The mutex is\n> >>> + * taken for the duration of the block.\n> >>> + *\n> >>> + * \\return 0 if value is found, -1 if not existent\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn Metadata::clear()\n> >>> + * Clear the Metadata map. The mutex is taken for the duration of\n> >>> + * the block.\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn Metadata::merge(Metadata &other)\n> >>> + * \\param[in] other A metadata to merge with\n> >>> + * Merge two Metadata maps. The mutex is taken for the duration of\n> >>> + * the block.\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn Metadata::getLocked(std::string const &tag)\n> >>> + * \\param[in] tag A string used as the key in a map\n> >>> + *\n> >>> + * Get the value of the tag key in the map.\n> >>> + * This allows in-place access to the Metadata contents,\n> >>> + * for which you should be holding the lock.\n> >>\n> >> I would expand upon this to also state how to lock it, given that\n> it is\n> >> part of the API of this class:\n> >>\n> >>\n> >> \"\"\"\n> >> This function does not protect against concurrent access, and it\n> is up\n> >> to the caller to ensure that the lock is held using \\a lock()\n> >> \"\"\"\n> >>\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn Metadata::setLocked(std::string const &tag, T const &value)\n> >>> + * \\param[in] tag A string used as the key in a map\n> >>> + * \\param[in] value The value to set into the map\n> >>> + *\n> >>> + * Set the value to the tag key in the map.\n> >>> + * This allows in-place access to the Metadata contents,\n> >>> + * for which you should be holding the lock.\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn Metadata::lock()\n> >>> + * Lock the mutex with the standard classes.\n> >>> + * e.g. std::lock_guard lock(metadata)\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn Metadata::unlock()\n> >>> + * Unlock the mutex with the standard classes.\n> >>> + * e.g. std::lock_guard lock(metadata)\n> >>> + */\n> >>> +\n> >>> +} /* namespace ipa */\n> >>> +\n> >>> +} /* namespace libcamera */\n> >>> +\n> >>> diff --git a/src/ipa/libipa/metadata.h b/src/ipa/libipa/metadata.h\n> >>> new file mode 100644\n> >>> index 00000000..9801bece\n> >>> --- /dev/null\n> >>> +++ b/src/ipa/libipa/metadata.h\n> >>> @@ -0,0 +1,90 @@\n> >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >>> +/*\n> >>> + * Based on the implementation from the Raspberry Pi IPA,\n> >>> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.\n> >>> + * Copyright (C) 2021, Ideas On Board\n> >>> + *\n> >>> + * metadata.h - libipa metadata class\n> >>> + */\n> >>> +#ifndef __LIBCAMERA_IPA_LIBIPA_METADATA_H__\n> >>> +#define __LIBCAMERA_IPA_LIBIPA_METADATA_H__\n> >>> +\n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +\n> >>> +namespace libcamera {\n> >>> +\n> >>> +namespace ipa {\n> >>> +\n> >>> +class Metadata\n> >>> +{\n> >>> +public:\n> >>> +   Metadata() = default;\n> >>> +\n> >>> +   Metadata(Metadata const &other)\n> >>> +   {\n> >>> +           std::scoped_lock other_lock(other.mutex_);\n> >>> +           data_ = other.data_;\n> >>> +   }\n> >>\n> >> The C++ rule of five (or is it six?) says if you need to\n> implement one\n> >> of the copy/move constructors you need to implement, default (or\n> >> delete?) them all:\n> >>\n> >>\n> https://www.modernescpp.com/index.php/c-core-guidelines-constructors-assignments-and-desctructors\n> \n> >>\n> >> So we should add appropriate copy assignment, and move\n> constructors with\n> >> appropriate locking ... (or delete them if they shouldn't be\n> allowed).\n> >>\n> >>\n> >> I think the constructor and destructor can be = default though, I\n> don't\n> >> see anything specific to handle there?\n> >>\n> >\n> > OK, I will implement those.\n> \n> When you do this, please try to be clear in the story your patches tell.\n> \n> \n> You could re-use the existing copy/move constructors and operators from\n> our implementation\n> for these.\n\nYes, sorry about that... :-(.\nYou don't have a destructor though ?\n\n> Regards,\n> Naush\n> \n>  \n> \n> \n> This is clearly additional code on top of the code which is imported, so\n> the series should do something like:\n> \n>  1/x ipa: libipa: Import Metadata class from src/ipa/raspberrypi\n>     Just the direct copy, without any specific changes other than making\n>     it compile in place\n> \n>  2/x ipa: libipa: Document Metadata class\n>     Any documentation that /you/ add.\n> \n>  3/x ipa: libipa: Implement missing Metadata class copy/move operations\n>     Any code additions that you add\n> \n>  4/x .... other?\n> \n> \n> >\n> >>\n> >>> +\n> >>> +   template\n> >>> +   void set(std::string const &tag, T const &value)\n> >>> +   {\n> >>> +           std::scoped_lock lock(mutex_);\n> >>> +           data_[tag] = value;\n> >>> +   }\n> >>> +\n> >>> +   template\n> >>> +   int get(std::string const &tag, T &value) const\n> >>> +   {\n> >>> +           std::scoped_lock lock(mutex_);\n> >>> +           auto it = data_.find(tag);\n> >>> +           if (it == data_.end())\n> >>> +                   return -1;\n> >>\n> >> Does std::any provide any way to get the template type of the\n> object so\n> >> we can assert with an incorrect access?\n> >\n> > Something like that ?\n> > https://en.cppreference.com/w/cpp/utility/any/type\n> \n> >\n> >> Perhaps that would then require specific RTTI which maybe we\n> don't want\n> >> to get into anyway though...\n> >\n> > I am not sure if it is worth it...\n> >\n> >>\n> >>> +           value = std::any_cast(it->second);\n> >>> +           return 0;\n> >>> +   }\n> >>> +\n> >>> +   void clear()\n> >>> +   {\n> >>> +           std::scoped_lock lock(mutex_);\n> >>> +           data_.clear();\n> >>> +   }\n> >>> +\n> >>> +   void merge(Metadata &other)\n> >>> +   {\n> >>> +           std::scoped_lock lock(mutex_, other.mutex_);\n> >>> +           data_.merge(other.data_);\n> >>> +   }\n> >>> +\n> >>> +   template\n> >>> +   T *getLocked(std::string const &tag)\n> >>> +   {\n> >>> +           auto it = data_.find(tag);\n> >>> +           if (it == data_.end())\n> >>> +                   return nullptr;\n> >>> +           return std::any_cast(&it->second);\n> >>> +   }\n> >>> +\n> >>> +   template\n> >>> +   void setLocked(std::string const &tag, T const &value)\n> >>> +   {\n> >>> +           data_[tag] = value;\n> >>> +   }\n> >>> +\n> >>> +   void lock() { mutex_.lock(); }\n> >>> +   void unlock() { mutex_.unlock(); }\n> >>> +\n> >>> +private:\n> >>> +   mutable std::mutex mutex_;\n> >>\n> >> Hrm, I had to look this up. I wonder if we should be using\n> mutable more\n> >> often.\n> >>\n> >> But indeed, this sounds right as it means you can 'get' from a const\n> >> Metadata and lock the mutex (which requires modifying the otherwise\n> >> const mutex).\n> >>\n> >>\n> >>\n> >>> +   std::map data_;\n> >>> +};\n> >>> +\n> >>> +} /* namespace ipa */\n> >>> +\n> >>> +} /* namespace libcamera */\n> >>> +\n> >>> +#endif /* __LIBCAMERA_IPA_LIBIPA_METADATA_H__ */\n> >>>\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E2E08C3226\n\tfor ;\n\tWed, 14 Jul 2021 08:48:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 53E7468523;\n\tWed, 14 Jul 2021 10:48:58 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 652A660282\n\tfor ;\n\tWed, 14 Jul 2021 10:48:56 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:e67c:9465:36df:f139])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 172B3CC;\n\tWed, 14 Jul 2021 10:48:56 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"VSgidtVM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626252536;\n\tbh=QDzYe9fn3bapaaWRlQE/H8KKaZ2HpWEpADzTu7KcMWw=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=VSgidtVM50CVVRardnQ0cvaIXIeq/3Mq4EJwFI7QR0xJMlEWcFysI3z0yvOjrXs5S\n\tlCjTr0OkVbjF0/imZowyohTEh716EvNth/Wt72TvqXkpqo0xcg67a/OMCv41Zf7VNF\n\tJUXK/GQRi+Q/gWfF/c2EGx3X4eHLn+V6Xx7/0J7E=","To":"Naushir Patuck ,\n\tKieran Bingham ","References":"<20210712131630.73914-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210712131630.73914-2-jeanmichel.hautbois@ideasonboard.com>\n\t<62596562-ae49-4277-c428-5380198f0559@ideasonboard.com>\n\t<6ec0e4f8-b386-348f-7a9a-b45039f22ed7@ideasonboard.com>\n\t<0461953b-aa47-c64f-211a-a8f2589f5ea8@ideasonboard.com>\n\t","From":"Jean-Michel Hautbois ","Message-ID":"","Date":"Wed, 14 Jul 2021 10:48:53 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata\n\tclass","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18169,"web_url":"https://patchwork.libcamera.org/comment/18169/","msgid":"","date":"2021-07-14T08:57:52","subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata\n\tclass","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Wed, Jul 14, 2021 at 09:12:17AM +0100, Kieran Bingham wrote:\n> On 14/07/2021 07:59, Jean-Michel Hautbois wrote:\n> > On 13/07/2021 16:41, Kieran Bingham wrote:\n> >> On 12/07/2021 14:16, Jean-Michel Hautbois wrote:\n> >>> The Metadata class comes from RPi from which a bit has been removed\n> >>> because we don't need it for now.\n> >>> All functions are inlined in metadata.h because of the template usage.\n> >>\n> >> Perhaps this could be better worded:\n> >>\n> >> \"\"\"\n> >> Import the Metadata class from src/ipa/raspberrypi/metadata.cpp to be\n> >> able to make use of the component from other IPA modules.\n> >> \"\"\"\n> >>\n> >>> Signed-off-by: Jean-Michel Hautbois \n> >>> ---\n> >>> src/ipa/ipu3/ipu3.cpp | 1 +\n> >>> src/ipa/libipa/meson.build | 6 ++-\n> >>> src/ipa/libipa/metadata.cpp | 101 ++++++++++++++++++++++++++++++++++++\n> >>> src/ipa/libipa/metadata.h | 90 ++++++++++++++++++++++++++++++++\n> >>> 4 files changed, 196 insertions(+), 2 deletions(-)\n> >>> create mode 100644 src/ipa/libipa/metadata.cpp\n> >>> create mode 100644 src/ipa/libipa/metadata.h\n> >>>\n> >>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> >>> index 71698d36..091856f5 100644\n> >>> --- a/src/ipa/ipu3/ipu3.cpp\n> >>> +++ b/src/ipa/ipu3/ipu3.cpp\n> >>> @@ -25,6 +25,7 @@\n> >>> #include \"ipu3_agc.h\"\n> >>> #include \"ipu3_awb.h\"\n> >>> #include \"libipa/camera_sensor_helper.h\"\n> >>> +#include \"libipa/metadata.h\"\n> >>> \n> >>> static constexpr uint32_t kMaxCellWidthPerSet = 160;\n> >>> static constexpr uint32_t kMaxCellHeightPerSet = 56;\n> >>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> >>> index 3fda7c00..cc4e1cc9 100644\n> >>> --- a/src/ipa/libipa/meson.build\n> >>> +++ b/src/ipa/libipa/meson.build\n> >>> @@ -3,13 +3,15 @@\n> >>> libipa_headers = files([\n> >>> 'algorithm.h',\n> >>> 'camera_sensor_helper.h',\n> >>> - 'histogram.h'\n> >>> + 'histogram.h',\n> >>> + 'metadata.h'\n> >>\n> >> If you keep a ',' at the end, you don't to modify this line when adding\n> >> another entry later.\n> >>\n> >>\n> >>> ])\n> >>> \n> >>> libipa_sources = files([\n> >>> 'algorithm.cpp',\n> >>> 'camera_sensor_helper.cpp',\n> >>> - 'histogram.cpp'\n> >>> + 'histogram.cpp',\n> >>> + 'metadata.cpp'\n> >>\n> >> Same here...\n> >>\n> >>\n> >>> ])\n> >>> \n> >>> libipa_includes = include_directories('..')\n> >>> diff --git a/src/ipa/libipa/metadata.cpp b/src/ipa/libipa/metadata.cpp\n> >>> new file mode 100644\n> >>> index 00000000..b6aef897\n> >>> --- /dev/null\n> >>> +++ b/src/ipa/libipa/metadata.cpp\n> >>> @@ -0,0 +1,101 @@\n> >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >>> +/*\n> >>> + * Based on the implementation from the Raspberry Pi IPA,\n> >>> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.\n> >>> + * Copyright (C) 2021, Ideas On Board\n> >>> + *\n> >>> + * metadata.cpp - libipa metadata class\n> >>> + */\n> >>> +\n> >>> +#include \"metadata.h\"\n> >>> +\n> >>> +/**\n> >>> + * \\file metadata.h\n> >>> + * \\brief A metadata class to share objects\n> >>\n> >> I wouldn't call it sharing objects...\n> >>\n> >> \"A metadata class to provide key based access to arbitrary metadata types.\n> >>\n> >>\n> >>> + */\n> >>> +\n> >>> +namespace libcamera {\n> >>> +\n> >>> +namespace ipa {\n> >>> +\n> >>> +/**\n> >>> + * \\class Metadata\n> >>> + * \\brief A simple class for carrying arbitrary metadata, for example\n> >>> + * about an image. It is used to exchange data between algorithms.\n> >>\n> >> I think we need to expand on this to explain some of the constraints too:\n> >>\n> >>\n> >> \"\"\"\n> >> Data is stored as a map with a string based key.\n> >>\n> >> The metadata is stored through a std::any() type which is definable by\n> >> the user, and must be correctly known by both the producer and consumer.\n> >>\n> >> Accessing the metadata with an incorrect type will cause undefined\n> >> behaviour.\n> >> \"\"\"\n> >>\n> >> Some of that might be too far towards the implementation details, but in\n> >> this instance, I sort of think those implementation details are\n> >> important because the implications they introduce.\n> >>\n> >>\n> >>\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn Metadata::Metadata(Metadata const &other)\n> >>> + * \\param[in] other A Metadata object\n> >>> + *\n> >>\n> >> I think this is the copy constructor right?\n> >>\n> >> Lets reference it so:\n> >>\n> >> \"Copy the data from the \\a other Metadata object to this one.\"\n> >>\n> >>\n> >>\n> >>> + * Stores the data from one Metadata to another one\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn Metadata::set(std::string const &tag, T const &value)\n> >>> + * \\param[in] tag A string used as the key in a map\n> >>> + * \\param[in] value The value to set into the map\n> >>\n> >> I would probably drop 'in a map' and 'into the map' in these parameter\n> >> descriptions.\n> >>\n> >>\n> >>> + *\n> >>> + * Sets the value in the map to the tag key. The mutex is\n> >>> + * taken for the duration of the block.\n> >>\n> >>\n> >> I would express this as..\n> >>\n> >> \"This function locks the metadata to protect from concurrent access\"\n> >>\n> >> (rather than \"the mutex is...\", and on a line/paragraph of its own to\n> >> stand out.)\n> >>\n> >>\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn Metadata::get(std::string const &tag, T &value)\n> >>> + * \\param[in] tag A string used as the key in a map\n> >>> + * \\param[in] value The value to set into the map\n> >>> + *\n> >>> + * Gets the value in the map of the tag key. The mutex is\n> >>> + * taken for the duration of the block.\n> >>> + *\n> >>> + * \\return 0 if value is found, -1 if not existent\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn Metadata::clear()\n> >>> + * Clear the Metadata map. The mutex is taken for the duration of\n> >>> + * the block.\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn Metadata::merge(Metadata &other)\n> >>> + * \\param[in] other A metadata to merge with\n> >>> + * Merge two Metadata maps. The mutex is taken for the duration of\n> >>> + * the block.\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn Metadata::getLocked(std::string const &tag)\n> >>> + * \\param[in] tag A string used as the key in a map\n> >>> + *\n> >>> + * Get the value of the tag key in the map.\n> >>> + * This allows in-place access to the Metadata contents,\n> >>> + * for which you should be holding the lock.\n> >>\n> >> I would expand upon this to also state how to lock it, given that it is\n> >> part of the API of this class:\n> >>\n> >>\n> >> \"\"\"\n> >> This function does not protect against concurrent access, and it is up\n> >> to the caller to ensure that the lock is held using \\a lock()\n> >> \"\"\"\n> >>\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn Metadata::setLocked(std::string const &tag, T const &value)\n> >>> + * \\param[in] tag A string used as the key in a map\n> >>> + * \\param[in] value The value to set into the map\n> >>> + *\n> >>> + * Set the value to the tag key in the map.\n> >>> + * This allows in-place access to the Metadata contents,\n> >>> + * for which you should be holding the lock.\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn Metadata::lock()\n> >>> + * Lock the mutex with the standard classes.\n> >>> + * e.g. std::lock_guard lock(metadata)\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn Metadata::unlock()\n> >>> + * Unlock the mutex with the standard classes.\n> >>> + * e.g. std::lock_guard lock(metadata)\n> >>> + */\n> >>> +\n> >>> +} /* namespace ipa */\n> >>> +\n> >>> +} /* namespace libcamera */\n> >>> +\n> >>> diff --git a/src/ipa/libipa/metadata.h b/src/ipa/libipa/metadata.h\n> >>> new file mode 100644\n> >>> index 00000000..9801bece\n> >>> --- /dev/null\n> >>> +++ b/src/ipa/libipa/metadata.h\n> >>> @@ -0,0 +1,90 @@\n> >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >>> +/*\n> >>> + * Based on the implementation from the Raspberry Pi IPA,\n> >>> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.\n> >>> + * Copyright (C) 2021, Ideas On Board\n> >>> + *\n> >>> + * metadata.h - libipa metadata class\n> >>> + */\n> >>> +#ifndef __LIBCAMERA_IPA_LIBIPA_METADATA_H__\n> >>> +#define __LIBCAMERA_IPA_LIBIPA_METADATA_H__\n> >>> +\n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +\n> >>> +namespace libcamera {\n> >>> +\n> >>> +namespace ipa {\n> >>> +\n> >>> +class Metadata\n> >>> +{\n> >>> +public:\n> >>> +\tMetadata() = default;\n> >>> +\n> >>> +\tMetadata(Metadata const &other)\n> >>> +\t{\n> >>> +\t\tstd::scoped_lock other_lock(other.mutex_);\n> >>> +\t\tdata_ = other.data_;\n> >>> +\t}\n> >>\n> >> The C++ rule of five (or is it six?) says if you need to implement one\n> >> of the copy/move constructors you need to implement, default (or\n> >> delete?) them all:\n> >>\n> >> https://www.modernescpp.com/index.php/c-core-guidelines-constructors-assignments-and-desctructors\n> >>\n> >> So we should add appropriate copy assignment, and move constructors with\n> >> appropriate locking ... (or delete them if they shouldn't be allowed).\n> >>\n> >>\n> >> I think the constructor and destructor can be = default though, I don't\n> >> see anything specific to handle there?\n> >>\n> > \n> > OK, I will implement those.\n> \n> When you do this, please try to be clear in the story your patches tell.\n> \n> This is clearly additional code on top of the code which is imported, so\n> the series should do something like:\n> \n> 1/x ipa: libipa: Import Metadata class from src/ipa/raspberrypi\n> Just the direct copy, without any specific changes other than making\n> it compile in place\n> \n> 2/x ipa: libipa: Document Metadata class\n> Any documentation that /you/ add.\n> \n> 3/x ipa: libipa: Implement missing Metadata class copy/move operations\n> Any code additions that you add\n> \n> 4/x .... other?\n\nI would have done it the other way around to be honest. A single patch\nwith a new implementation would be easier to review. We need to consider\nall the design assumptions and see if they match the use cases we\nenvision for other IPA modules. Copying the existing code as-is will\nresult in the first patch not being reviewed from a design point of\nview.\n\nI haven't looked at this in details, but I want to check the locking\nimplementation in particular. I also think integer tags would be more\nefficient (but there could be drawbacks that we should discuss). I'm\nsure I'll have more comments when I'll review the code in details :-)\n\n> >>> +\n> >>> +\ttemplate\n> >>> +\tvoid set(std::string const &tag, T const &value)\n> >>> +\t{\n> >>> +\t\tstd::scoped_lock lock(mutex_);\n> >>> +\t\tdata_[tag] = value;\n> >>> +\t}\n> >>> +\n> >>> +\ttemplate\n> >>> +\tint get(std::string const &tag, T &value) const\n> >>> +\t{\n> >>> +\t\tstd::scoped_lock lock(mutex_);\n> >>> +\t\tauto it = data_.find(tag);\n> >>> +\t\tif (it == data_.end())\n> >>> +\t\t\treturn -1;\n> >>\n> >> Does std::any provide any way to get the template type of the object so\n> >> we can assert with an incorrect access?\n> > \n> > Something like that ?\n> > https://en.cppreference.com/w/cpp/utility/any/type\n> > \n> >> Perhaps that would then require specific RTTI which maybe we don't want\n> >> to get into anyway though...\n> > \n> > I am not sure if it is worth it...\n> > \n> >>> +\t\tvalue = std::any_cast(it->second);\n> >>> +\t\treturn 0;\n> >>> +\t}\n> >>> +\n> >>> +\tvoid clear()\n> >>> +\t{\n> >>> +\t\tstd::scoped_lock lock(mutex_);\n> >>> +\t\tdata_.clear();\n> >>> +\t}\n> >>> +\n> >>> +\tvoid merge(Metadata &other)\n> >>> +\t{\n> >>> +\t\tstd::scoped_lock lock(mutex_, other.mutex_);\n> >>> +\t\tdata_.merge(other.data_);\n> >>> +\t}\n> >>> +\n> >>> +\ttemplate\n> >>> +\tT *getLocked(std::string const &tag)\n> >>> +\t{\n> >>> +\t\tauto it = data_.find(tag);\n> >>> +\t\tif (it == data_.end())\n> >>> +\t\t\treturn nullptr;\n> >>> +\t\treturn std::any_cast(&it->second);\n> >>> +\t}\n> >>> +\n> >>> +\ttemplate\n> >>> +\tvoid setLocked(std::string const &tag, T const &value)\n> >>> +\t{\n> >>> +\t\tdata_[tag] = value;\n> >>> +\t}\n> >>> +\n> >>> +\tvoid lock() { mutex_.lock(); }\n> >>> +\tvoid unlock() { mutex_.unlock(); }\n> >>> +\n> >>> +private:\n> >>> +\tmutable std::mutex mutex_;\n> >>\n> >> Hrm, I had to look this up. I wonder if we should be using mutable more\n> >> often.\n> >>\n> >> But indeed, this sounds right as it means you can 'get' from a const\n> >> Metadata and lock the mutex (which requires modifying the otherwise\n> >> const mutex).\n> >>\n> >>> +\tstd::map data_;\n> >>> +};\n> >>> +\n> >>> +} /* namespace ipa */\n> >>> +\n> >>> +} /* namespace libcamera */\n> >>> +\n> >>> +#endif /* __LIBCAMERA_IPA_LIBIPA_METADATA_H__ */","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id F160EC3225\n\tfor ;\n\tWed, 14 Jul 2021 08:57:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 602CE68525;\n\tWed, 14 Jul 2021 10:57:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D18B260282\n\tfor ;\n\tWed, 14 Jul 2021 10:57:53 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3D3E2CC;\n\tWed, 14 Jul 2021 10:57:53 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"gYejLD4/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626253073;\n\tbh=T07oCHI6bxMt57y8XxZaoeNCVNfqZUGi8RzGCqX3FLk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gYejLD4/mz8eEgXzliDMA2PBaXQUcaMyjMT31kvAu+legZSFBsXasyHrZTQ3oX3yb\n\tgrw8CdaA23dx1Xwp3hHAg693Eiqs4e1M1S+qTgV4mWU+HYIp3gC4Qi7F40Bk9nspyb\n\tY+2ndTWa1cZeySXuinXuc6J2fVV7a6Yz/xA1rW9o=","Date":"Wed, 14 Jul 2021 11:57:52 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210712131630.73914-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210712131630.73914-2-jeanmichel.hautbois@ideasonboard.com>\n\t<62596562-ae49-4277-c428-5380198f0559@ideasonboard.com>\n\t<6ec0e4f8-b386-348f-7a9a-b45039f22ed7@ideasonboard.com>\n\t<0461953b-aa47-c64f-211a-a8f2589f5ea8@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<0461953b-aa47-c64f-211a-a8f2589f5ea8@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata\n\tclass","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18172,"web_url":"https://patchwork.libcamera.org/comment/18172/","msgid":"","date":"2021-07-14T12:48:38","subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata\n\tclass","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jean-Michel,\n\nThank you for the patch.\n\nOn Mon, Jul 12, 2021 at 03:16:29PM +0200, Jean-Michel Hautbois wrote:\n> The Metadata class comes from RPi from which a bit has been removed\n> because we don't need it for now.\n> All functions are inlined in metadata.h because of the template usage.\n> \n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/ipu3.cpp | 1 +\n> src/ipa/libipa/meson.build | 6 ++-\n> src/ipa/libipa/metadata.cpp | 101 ++++++++++++++++++++++++++++++++++++\n> src/ipa/libipa/metadata.h | 90 ++++++++++++++++++++++++++++++++\n> 4 files changed, 196 insertions(+), 2 deletions(-)\n> create mode 100644 src/ipa/libipa/metadata.cpp\n> create mode 100644 src/ipa/libipa/metadata.h\n> \n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 71698d36..091856f5 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -25,6 +25,7 @@\n> #include \"ipu3_agc.h\"\n> #include \"ipu3_awb.h\"\n> #include \"libipa/camera_sensor_helper.h\"\n> +#include \"libipa/metadata.h\"\n> \n> static constexpr uint32_t kMaxCellWidthPerSet = 160;\n> static constexpr uint32_t kMaxCellHeightPerSet = 56;\n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index 3fda7c00..cc4e1cc9 100644\n> --- a/src/ipa/libipa/meson.build\n> +++ b/src/ipa/libipa/meson.build\n> @@ -3,13 +3,15 @@\n> libipa_headers = files([\n> 'algorithm.h',\n> 'camera_sensor_helper.h',\n> - 'histogram.h'\n> + 'histogram.h',\n> + 'metadata.h'\n> ])\n> \n> libipa_sources = files([\n> 'algorithm.cpp',\n> 'camera_sensor_helper.cpp',\n> - 'histogram.cpp'\n> + 'histogram.cpp',\n> + 'metadata.cpp'\n> ])\n> \n> libipa_includes = include_directories('..')\n> diff --git a/src/ipa/libipa/metadata.cpp b/src/ipa/libipa/metadata.cpp\n> new file mode 100644\n> index 00000000..b6aef897\n> --- /dev/null\n> +++ b/src/ipa/libipa/metadata.cpp\n> @@ -0,0 +1,101 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Based on the implementation from the Raspberry Pi IPA,\n> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.\n> + * Copyright (C) 2021, Ideas On Board\n> + *\n> + * metadata.cpp - libipa metadata class\n> + */\n> +\n> +#include \"metadata.h\"\n> +\n> +/**\n> + * \\file metadata.h\n> + * \\brief A metadata class to share objects\n> + */\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +/**\n> + * \\class Metadata\n> + * \\brief A simple class for carrying arbitrary metadata, for example\n> + * about an image. It is used to exchange data between algorithms.\n> + */\n> +\n> +/**\n> + * \\fn Metadata::Metadata(Metadata const &other)\n> + * \\param[in] other A Metadata object\n> + *\n> + * Stores the data from one Metadata to another one\n> + */\n> +\n> +/**\n> + * \\fn Metadata::set(std::string const &tag, T const &value)\n> + * \\param[in] tag A string used as the key in a map\n> + * \\param[in] value The value to set into the map\n> + *\n> + * Sets the value in the map to the tag key. The mutex is\n> + * taken for the duration of the block.\n> + */\n> +\n> +/**\n> + * \\fn Metadata::get(std::string const &tag, T &value)\n> + * \\param[in] tag A string used as the key in a map\n> + * \\param[in] value The value to set into the map\n> + *\n> + * Gets the value in the map of the tag key. The mutex is\n> + * taken for the duration of the block.\n> + *\n> + * \\return 0 if value is found, -1 if not existent\n> + */\n> +\n> +/**\n> + * \\fn Metadata::clear()\n> + * Clear the Metadata map. The mutex is taken for the duration of\n> + * the block.\n> + */\n> +\n> +/**\n> + * \\fn Metadata::merge(Metadata &other)\n> + * \\param[in] other A metadata to merge with\n> + * Merge two Metadata maps. The mutex is taken for the duration of\n> + * the block.\n> + */\n> +\n> +/**\n> + * \\fn Metadata::getLocked(std::string const &tag)\n> + * \\param[in] tag A string used as the key in a map\n> + *\n> + * Get the value of the tag key in the map.\n> + * This allows in-place access to the Metadata contents,\n> + * for which you should be holding the lock.\n> + */\n> +\n> +/**\n> + * \\fn Metadata::setLocked(std::string const &tag, T const &value)\n> + * \\param[in] tag A string used as the key in a map\n> + * \\param[in] value The value to set into the map\n> + *\n> + * Set the value to the tag key in the map.\n> + * This allows in-place access to the Metadata contents,\n> + * for which you should be holding the lock.\n> + */\n> +\n> +/**\n> + * \\fn Metadata::lock()\n> + * Lock the mutex with the standard classes.\n> + * e.g. std::lock_guard lock(metadata)\n> + */\n> +\n> +/**\n> + * \\fn Metadata::unlock()\n> + * Unlock the mutex with the standard classes.\n> + * e.g. std::lock_guard lock(metadata)\n> + */\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> +\n> diff --git a/src/ipa/libipa/metadata.h b/src/ipa/libipa/metadata.h\n> new file mode 100644\n> index 00000000..9801bece\n> --- /dev/null\n> +++ b/src/ipa/libipa/metadata.h\n> @@ -0,0 +1,90 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Based on the implementation from the Raspberry Pi IPA,\n> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.\n> + * Copyright (C) 2021, Ideas On Board\n> + *\n> + * metadata.h - libipa metadata class\n> + */\n> +#ifndef __LIBCAMERA_IPA_LIBIPA_METADATA_H__\n> +#define __LIBCAMERA_IPA_LIBIPA_METADATA_H__\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +class Metadata\n> +{\n> +public:\n> +\tMetadata() = default;\n> +\n> +\tMetadata(Metadata const &other)\n> +\t{\n> +\t\tstd::scoped_lock other_lock(other.mutex_);\n> +\t\tdata_ = other.data_;\n> +\t}\n> +\n> +\ttemplate\n> +\tvoid set(std::string const &tag, T const &value)\n> +\t{\n> +\t\tstd::scoped_lock lock(mutex_);\n> +\t\tdata_[tag] = value;\n> +\t}\n> +\n> +\ttemplate\n> +\tint get(std::string const &tag, T &value) const\n> +\t{\n> +\t\tstd::scoped_lock lock(mutex_);\n\nYou can call getLocked() here.\n\n> +\t\tauto it = data_.find(tag);\n> +\t\tif (it == data_.end())\n> +\t\t\treturn -1;\n> +\t\tvalue = std::any_cast(it->second);\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tvoid clear()\n> +\t{\n> +\t\tstd::scoped_lock lock(mutex_);\n> +\t\tdata_.clear();\n> +\t}\n> +\n> +\tvoid merge(Metadata &other)\n> +\t{\n> +\t\tstd::scoped_lock lock(mutex_, other.mutex_);\n> +\t\tdata_.merge(other.data_);\n> +\t}\n> +\n> +\ttemplate\n> +\tT *getLocked(std::string const &tag)\n> +\t{\n> +\t\tauto it = data_.find(tag);\n> +\t\tif (it == data_.end())\n> +\t\t\treturn nullptr;\n> +\t\treturn std::any_cast(&it->second);\n\nThis is a bit dangerous, if T doesn't match the type stored for the tag.\nIt would of course be a bug in the caller, but if such code appears in\npaths that are not frequently taken, it could cause issues that will\nresult in a crash at runtime later on.\n\nCould we use a mechanism similar to Control and ControlList to ensure\nthe right case ?\n\ntemplate\nstruct Tag {\n\tstd::string tag_;\n};\n\n/* Static list of all tags. */\nstatic constexpr Tag tagFoo{ \"foo\" };\n\nclass Metadata\n{\n\t...\n\ttemplate\n\tT *getLock(const Tag &tag)\n\t{\n\t\t...\n\t\treturn std::any_cast(&it->second);\n\t}\n\t...\n};\n...\n\n\tFoo *f = metadata.get(tagFoo);\n\nFurthermore, have you considered using integers instead of strings for\ntags ? What are the pros and cons ?\n\n> +\t}\n> +\n> +\ttemplate\n> +\tvoid setLocked(std::string const &tag, T const &value)\n> +\t{\n> +\t\tdata_[tag] = value;\n> +\t}\n> +\n> +\tvoid lock() { mutex_.lock(); }\n> +\tvoid unlock() { mutex_.unlock(); }\n> +\n> +private:\n> +\tmutable std::mutex mutex_;\n> +\tstd::map data_;\n> +};\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_IPA_LIBIPA_METADATA_H__ */","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 99332C3225\n\tfor ;\n\tWed, 14 Jul 2021 12:48:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D968E68523;\n\tWed, 14 Jul 2021 14:48:41 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8A0AB68503\n\tfor ;\n\tWed, 14 Jul 2021 14:48:40 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F36B0CC;\n\tWed, 14 Jul 2021 14:48:39 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"sp4dTme9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626266920;\n\tbh=qK2APAEHAEAT3X93b9tQeWw5XNk1xhIAzfubyqWphqc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sp4dTme9+2RHPzUIpLA+sWKvB0bACg9IauNAkuKo4/LdSISfP57XFncAoEX5vfCS4\n\tfDQTIxx/5mj0ik+en6iYzvfkO8XcHhbnSj5g/rdAvEuDKbMMnC+5MBpJUZtBgcSC8v\n\tVPr7/eCYmWevNMXAhYQU8RmXSTak4VCyaVuyRi6U=","Date":"Wed, 14 Jul 2021 15:48:38 +0300","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20210712131630.73914-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210712131630.73914-2-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210712131630.73914-2-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata\n\tclass","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18173,"web_url":"https://patchwork.libcamera.org/comment/18173/","msgid":"<62a0ec5d-0c19-8971-c46f-612c767a6b52@ideasonboard.com>","date":"2021-07-14T12:57:11","subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata\n\tclass","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 14/07/2021 14:48, Laurent Pinchart wrote:\n> Hi Jean-Michel,\n> \n> Thank you for the patch.\n> \n> On Mon, Jul 12, 2021 at 03:16:29PM +0200, Jean-Michel Hautbois wrote:\n>> The Metadata class comes from RPi from which a bit has been removed\n>> because we don't need it for now.\n>> All functions are inlined in metadata.h because of the template usage.\n>>\n>> Signed-off-by: Jean-Michel Hautbois \n>> ---\n>> src/ipa/ipu3/ipu3.cpp | 1 +\n>> src/ipa/libipa/meson.build | 6 ++-\n>> src/ipa/libipa/metadata.cpp | 101 ++++++++++++++++++++++++++++++++++++\n>> src/ipa/libipa/metadata.h | 90 ++++++++++++++++++++++++++++++++\n>> 4 files changed, 196 insertions(+), 2 deletions(-)\n>> create mode 100644 src/ipa/libipa/metadata.cpp\n>> create mode 100644 src/ipa/libipa/metadata.h\n>>\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index 71698d36..091856f5 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -25,6 +25,7 @@\n>> #include \"ipu3_agc.h\"\n>> #include \"ipu3_awb.h\"\n>> #include \"libipa/camera_sensor_helper.h\"\n>> +#include \"libipa/metadata.h\"\n>> \n>> static constexpr uint32_t kMaxCellWidthPerSet = 160;\n>> static constexpr uint32_t kMaxCellHeightPerSet = 56;\n>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n>> index 3fda7c00..cc4e1cc9 100644\n>> --- a/src/ipa/libipa/meson.build\n>> +++ b/src/ipa/libipa/meson.build\n>> @@ -3,13 +3,15 @@\n>> libipa_headers = files([\n>> 'algorithm.h',\n>> 'camera_sensor_helper.h',\n>> - 'histogram.h'\n>> + 'histogram.h',\n>> + 'metadata.h'\n>> ])\n>> \n>> libipa_sources = files([\n>> 'algorithm.cpp',\n>> 'camera_sensor_helper.cpp',\n>> - 'histogram.cpp'\n>> + 'histogram.cpp',\n>> + 'metadata.cpp'\n>> ])\n>> \n>> libipa_includes = include_directories('..')\n>> diff --git a/src/ipa/libipa/metadata.cpp b/src/ipa/libipa/metadata.cpp\n>> new file mode 100644\n>> index 00000000..b6aef897\n>> --- /dev/null\n>> +++ b/src/ipa/libipa/metadata.cpp\n>> @@ -0,0 +1,101 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Based on the implementation from the Raspberry Pi IPA,\n>> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.\n>> + * Copyright (C) 2021, Ideas On Board\n>> + *\n>> + * metadata.cpp - libipa metadata class\n>> + */\n>> +\n>> +#include \"metadata.h\"\n>> +\n>> +/**\n>> + * \\file metadata.h\n>> + * \\brief A metadata class to share objects\n>> + */\n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa {\n>> +\n>> +/**\n>> + * \\class Metadata\n>> + * \\brief A simple class for carrying arbitrary metadata, for example\n>> + * about an image. It is used to exchange data between algorithms.\n>> + */\n>> +\n>> +/**\n>> + * \\fn Metadata::Metadata(Metadata const &other)\n>> + * \\param[in] other A Metadata object\n>> + *\n>> + * Stores the data from one Metadata to another one\n>> + */\n>> +\n>> +/**\n>> + * \\fn Metadata::set(std::string const &tag, T const &value)\n>> + * \\param[in] tag A string used as the key in a map\n>> + * \\param[in] value The value to set into the map\n>> + *\n>> + * Sets the value in the map to the tag key. The mutex is\n>> + * taken for the duration of the block.\n>> + */\n>> +\n>> +/**\n>> + * \\fn Metadata::get(std::string const &tag, T &value)\n>> + * \\param[in] tag A string used as the key in a map\n>> + * \\param[in] value The value to set into the map\n>> + *\n>> + * Gets the value in the map of the tag key. The mutex is\n>> + * taken for the duration of the block.\n>> + *\n>> + * \\return 0 if value is found, -1 if not existent\n>> + */\n>> +\n>> +/**\n>> + * \\fn Metadata::clear()\n>> + * Clear the Metadata map. The mutex is taken for the duration of\n>> + * the block.\n>> + */\n>> +\n>> +/**\n>> + * \\fn Metadata::merge(Metadata &other)\n>> + * \\param[in] other A metadata to merge with\n>> + * Merge two Metadata maps. The mutex is taken for the duration of\n>> + * the block.\n>> + */\n>> +\n>> +/**\n>> + * \\fn Metadata::getLocked(std::string const &tag)\n>> + * \\param[in] tag A string used as the key in a map\n>> + *\n>> + * Get the value of the tag key in the map.\n>> + * This allows in-place access to the Metadata contents,\n>> + * for which you should be holding the lock.\n>> + */\n>> +\n>> +/**\n>> + * \\fn Metadata::setLocked(std::string const &tag, T const &value)\n>> + * \\param[in] tag A string used as the key in a map\n>> + * \\param[in] value The value to set into the map\n>> + *\n>> + * Set the value to the tag key in the map.\n>> + * This allows in-place access to the Metadata contents,\n>> + * for which you should be holding the lock.\n>> + */\n>> +\n>> +/**\n>> + * \\fn Metadata::lock()\n>> + * Lock the mutex with the standard classes.\n>> + * e.g. std::lock_guard lock(metadata)\n>> + */\n>> +\n>> +/**\n>> + * \\fn Metadata::unlock()\n>> + * Unlock the mutex with the standard classes.\n>> + * e.g. std::lock_guard lock(metadata)\n>> + */\n>> +\n>> +} /* namespace ipa */\n>> +\n>> +} /* namespace libcamera */\n>> +\n>> diff --git a/src/ipa/libipa/metadata.h b/src/ipa/libipa/metadata.h\n>> new file mode 100644\n>> index 00000000..9801bece\n>> --- /dev/null\n>> +++ b/src/ipa/libipa/metadata.h\n>> @@ -0,0 +1,90 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Based on the implementation from the Raspberry Pi IPA,\n>> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.\n>> + * Copyright (C) 2021, Ideas On Board\n>> + *\n>> + * metadata.h - libipa metadata class\n>> + */\n>> +#ifndef __LIBCAMERA_IPA_LIBIPA_METADATA_H__\n>> +#define __LIBCAMERA_IPA_LIBIPA_METADATA_H__\n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa {\n>> +\n>> +class Metadata\n>> +{\n>> +public:\n>> +\tMetadata() = default;\n>> +\n>> +\tMetadata(Metadata const &other)\n>> +\t{\n>> +\t\tstd::scoped_lock other_lock(other.mutex_);\n>> +\t\tdata_ = other.data_;\n>> +\t}\n>> +\n>> +\ttemplate\n>> +\tvoid set(std::string const &tag, T const &value)\n>> +\t{\n>> +\t\tstd::scoped_lock lock(mutex_);\n>> +\t\tdata_[tag] = value;\n>> +\t}\n>> +\n>> +\ttemplate\n>> +\tint get(std::string const &tag, T &value) const\n>> +\t{\n>> +\t\tstd::scoped_lock lock(mutex_);\n> \n> You can call getLocked() here.\n> \nIndeed :-)\n\n>> +\t\tauto it = data_.find(tag);\n>> +\t\tif (it == data_.end())\n>> +\t\t\treturn -1;\n>> +\t\tvalue = std::any_cast(it->second);\n>> +\t\treturn 0;\n>> +\t}\n>> +\n>> +\tvoid clear()\n>> +\t{\n>> +\t\tstd::scoped_lock lock(mutex_);\n>> +\t\tdata_.clear();\n>> +\t}\n>> +\n>> +\tvoid merge(Metadata &other)\n>> +\t{\n>> +\t\tstd::scoped_lock lock(mutex_, other.mutex_);\n>> +\t\tdata_.merge(other.data_);\n>> +\t}\n>> +\n>> +\ttemplate\n>> +\tT *getLocked(std::string const &tag)\n>> +\t{\n>> +\t\tauto it = data_.find(tag);\n>> +\t\tif (it == data_.end())\n>> +\t\t\treturn nullptr;\n>> +\t\treturn std::any_cast(&it->second);\n> \n> This is a bit dangerous, if T doesn't match the type stored for the tag.\n> It would of course be a bug in the caller, but if such code appears in\n> paths that are not frequently taken, it could cause issues that will\n> result in a crash at runtime later on.\n> \n> Could we use a mechanism similar to Control and ControlList to ensure\n> the right case ?\n> \n> template\n> struct Tag {\n> \tstd::string tag_;\n> };\n> \n> /* Static list of all tags. */\n> static constexpr Tag tagFoo{ \"foo\" };\n> \n> class Metadata\n> {\n> \t...\n> \ttemplate\n> \tT *getLock(const Tag &tag)\n> \t{\n> \t\t...\n> \t\treturn std::any_cast(&it->second);\n> \t}\n> \t...\n> };\n> ...\n> \n> \tFoo *f = metadata.get(tagFoo);\n> \n> Furthermore, have you considered using integers instead of strings for\n> tags ? What are the pros and cons ?\n> \n\nI think it may be a good idea. Pros I can see:\n- easy to document the tags\n- readability (we can always refer to the enum to know how a particular\nobject is mapped).\n- fastest to find the tag\nCons, not much:\n- if RPi wants to switch to the libipa Metadata class they will need to\nrewrite a not that small piece of code.\n- having integer would make the code less dynamic, as we would certainly\nbe tempted to create a static map (which can also be seen as a pro :-)).\n\n>> +\t}\n>> +\n>> +\ttemplate\n>> +\tvoid setLocked(std::string const &tag, T const &value)\n>> +\t{\n>> +\t\tdata_[tag] = value;\n>> +\t}\n>> +\n>> +\tvoid lock() { mutex_.lock(); }\n>> +\tvoid unlock() { mutex_.unlock(); }\n>> +\n>> +private:\n>> +\tmutable std::mutex mutex_;\n>> +\tstd::map data_;\n>> +};\n>> +\n>> +} /* namespace ipa */\n>> +\n>> +} /* namespace libcamera */\n>> +\n>> +#endif /* __LIBCAMERA_IPA_LIBIPA_METADATA_H__ */\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E5A45C3226\n\tfor ;\n\tWed, 14 Jul 2021 12:57:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3F02868525;\n\tWed, 14 Jul 2021 14:57:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CC24368503\n\tfor ;\n\tWed, 14 Jul 2021 14:57:13 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:e67c:9465:36df:f139])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 59FD4CC;\n\tWed, 14 Jul 2021 14:57:13 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"gW4rBOT3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626267433;\n\tbh=WtZykGKtHWemfGpd8V+7Z0H5iSIXlmxvQ/H4WGyptsQ=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=gW4rBOT3TM3jJLSXw/r2l5HUIFTmm3mhc3ZAtDHdyYhbyU7peJcd1vMxw0FyKXmNh\n\ttOO1Rgi0R2CEIetHuhec4me90P4MX2+I67t5xSWnfl9eVnLXIfUzw0wEEdxJZfSVgY\n\tIZ26RgyvIZlz1/vT/I27ssk/zLURaicp98zIR23g=","To":"Laurent Pinchart ","References":"<20210712131630.73914-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210712131630.73914-2-jeanmichel.hautbois@ideasonboard.com>\n\t","From":"Jean-Michel Hautbois ","Message-ID":"<62a0ec5d-0c19-8971-c46f-612c767a6b52@ideasonboard.com>","Date":"Wed, 14 Jul 2021 14:57:11 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata\n\tclass","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18177,"web_url":"https://patchwork.libcamera.org/comment/18177/","msgid":"","date":"2021-07-14T17:07:52","subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata\n\tclass","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Jul 14, 2021 at 02:57:11PM +0200, Jean-Michel Hautbois wrote:\n> Hi Laurent,\n> \n> On 14/07/2021 14:48, Laurent Pinchart wrote:\n> > Hi Jean-Michel,\n> > \n> > Thank you for the patch.\n> > \n> > On Mon, Jul 12, 2021 at 03:16:29PM +0200, Jean-Michel Hautbois wrote:\n> >> The Metadata class comes from RPi from which a bit has been removed\n> >> because we don't need it for now.\n> >> All functions are inlined in metadata.h because of the template usage.\n> >>\n> >> Signed-off-by: Jean-Michel Hautbois \n> >> ---\n> >> src/ipa/ipu3/ipu3.cpp | 1 +\n> >> src/ipa/libipa/meson.build | 6 ++-\n> >> src/ipa/libipa/metadata.cpp | 101 ++++++++++++++++++++++++++++++++++++\n> >> src/ipa/libipa/metadata.h | 90 ++++++++++++++++++++++++++++++++\n> >> 4 files changed, 196 insertions(+), 2 deletions(-)\n> >> create mode 100644 src/ipa/libipa/metadata.cpp\n> >> create mode 100644 src/ipa/libipa/metadata.h\n> >>\n> >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> >> index 71698d36..091856f5 100644\n> >> --- a/src/ipa/ipu3/ipu3.cpp\n> >> +++ b/src/ipa/ipu3/ipu3.cpp\n> >> @@ -25,6 +25,7 @@\n> >> #include \"ipu3_agc.h\"\n> >> #include \"ipu3_awb.h\"\n> >> #include \"libipa/camera_sensor_helper.h\"\n> >> +#include \"libipa/metadata.h\"\n> >> \n> >> static constexpr uint32_t kMaxCellWidthPerSet = 160;\n> >> static constexpr uint32_t kMaxCellHeightPerSet = 56;\n> >> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> >> index 3fda7c00..cc4e1cc9 100644\n> >> --- a/src/ipa/libipa/meson.build\n> >> +++ b/src/ipa/libipa/meson.build\n> >> @@ -3,13 +3,15 @@\n> >> libipa_headers = files([\n> >> 'algorithm.h',\n> >> 'camera_sensor_helper.h',\n> >> - 'histogram.h'\n> >> + 'histogram.h',\n> >> + 'metadata.h'\n> >> ])\n> >> \n> >> libipa_sources = files([\n> >> 'algorithm.cpp',\n> >> 'camera_sensor_helper.cpp',\n> >> - 'histogram.cpp'\n> >> + 'histogram.cpp',\n> >> + 'metadata.cpp'\n> >> ])\n> >> \n> >> libipa_includes = include_directories('..')\n> >> diff --git a/src/ipa/libipa/metadata.cpp b/src/ipa/libipa/metadata.cpp\n> >> new file mode 100644\n> >> index 00000000..b6aef897\n> >> --- /dev/null\n> >> +++ b/src/ipa/libipa/metadata.cpp\n> >> @@ -0,0 +1,101 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +/*\n> >> + * Based on the implementation from the Raspberry Pi IPA,\n> >> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.\n> >> + * Copyright (C) 2021, Ideas On Board\n> >> + *\n> >> + * metadata.cpp - libipa metadata class\n> >> + */\n> >> +\n> >> +#include \"metadata.h\"\n> >> +\n> >> +/**\n> >> + * \\file metadata.h\n> >> + * \\brief A metadata class to share objects\n> >> + */\n> >> +\n> >> +namespace libcamera {\n> >> +\n> >> +namespace ipa {\n> >> +\n> >> +/**\n> >> + * \\class Metadata\n> >> + * \\brief A simple class for carrying arbitrary metadata, for example\n> >> + * about an image. It is used to exchange data between algorithms.\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn Metadata::Metadata(Metadata const &other)\n> >> + * \\param[in] other A Metadata object\n> >> + *\n> >> + * Stores the data from one Metadata to another one\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn Metadata::set(std::string const &tag, T const &value)\n> >> + * \\param[in] tag A string used as the key in a map\n> >> + * \\param[in] value The value to set into the map\n> >> + *\n> >> + * Sets the value in the map to the tag key. The mutex is\n> >> + * taken for the duration of the block.\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn Metadata::get(std::string const &tag, T &value)\n> >> + * \\param[in] tag A string used as the key in a map\n> >> + * \\param[in] value The value to set into the map\n> >> + *\n> >> + * Gets the value in the map of the tag key. The mutex is\n> >> + * taken for the duration of the block.\n> >> + *\n> >> + * \\return 0 if value is found, -1 if not existent\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn Metadata::clear()\n> >> + * Clear the Metadata map. The mutex is taken for the duration of\n> >> + * the block.\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn Metadata::merge(Metadata &other)\n> >> + * \\param[in] other A metadata to merge with\n> >> + * Merge two Metadata maps. The mutex is taken for the duration of\n> >> + * the block.\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn Metadata::getLocked(std::string const &tag)\n> >> + * \\param[in] tag A string used as the key in a map\n> >> + *\n> >> + * Get the value of the tag key in the map.\n> >> + * This allows in-place access to the Metadata contents,\n> >> + * for which you should be holding the lock.\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn Metadata::setLocked(std::string const &tag, T const &value)\n> >> + * \\param[in] tag A string used as the key in a map\n> >> + * \\param[in] value The value to set into the map\n> >> + *\n> >> + * Set the value to the tag key in the map.\n> >> + * This allows in-place access to the Metadata contents,\n> >> + * for which you should be holding the lock.\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn Metadata::lock()\n> >> + * Lock the mutex with the standard classes.\n> >> + * e.g. std::lock_guard lock(metadata)\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn Metadata::unlock()\n> >> + * Unlock the mutex with the standard classes.\n> >> + * e.g. std::lock_guard lock(metadata)\n> >> + */\n> >> +\n> >> +} /* namespace ipa */\n> >> +\n> >> +} /* namespace libcamera */\n> >> +\n> >> diff --git a/src/ipa/libipa/metadata.h b/src/ipa/libipa/metadata.h\n> >> new file mode 100644\n> >> index 00000000..9801bece\n> >> --- /dev/null\n> >> +++ b/src/ipa/libipa/metadata.h\n> >> @@ -0,0 +1,90 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +/*\n> >> + * Based on the implementation from the Raspberry Pi IPA,\n> >> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.\n> >> + * Copyright (C) 2021, Ideas On Board\n> >> + *\n> >> + * metadata.h - libipa metadata class\n> >> + */\n> >> +#ifndef __LIBCAMERA_IPA_LIBIPA_METADATA_H__\n> >> +#define __LIBCAMERA_IPA_LIBIPA_METADATA_H__\n> >> +\n> >> +#include \n> >> +#include \n> >> +#include \n> >> +#include \n> >> +#include \n> >> +\n> >> +namespace libcamera {\n> >> +\n> >> +namespace ipa {\n> >> +\n> >> +class Metadata\n> >> +{\n> >> +public:\n> >> +\tMetadata() = default;\n> >> +\n> >> +\tMetadata(Metadata const &other)\n> >> +\t{\n> >> +\t\tstd::scoped_lock other_lock(other.mutex_);\n> >> +\t\tdata_ = other.data_;\n> >> +\t}\n> >> +\n> >> +\ttemplate\n> >> +\tvoid set(std::string const &tag, T const &value)\n> >> +\t{\n> >> +\t\tstd::scoped_lock lock(mutex_);\n> >> +\t\tdata_[tag] = value;\n> >> +\t}\n> >> +\n> >> +\ttemplate\n> >> +\tint get(std::string const &tag, T &value) const\n> >> +\t{\n> >> +\t\tstd::scoped_lock lock(mutex_);\n> > \n> > You can call getLocked() here.\n> > \n> Indeed :-)\n> \n> >> +\t\tauto it = data_.find(tag);\n> >> +\t\tif (it == data_.end())\n> >> +\t\t\treturn -1;\n> >> +\t\tvalue = std::any_cast(it->second);\n> >> +\t\treturn 0;\n> >> +\t}\n> >> +\n> >> +\tvoid clear()\n> >> +\t{\n> >> +\t\tstd::scoped_lock lock(mutex_);\n> >> +\t\tdata_.clear();\n> >> +\t}\n> >> +\n> >> +\tvoid merge(Metadata &other)\n> >> +\t{\n> >> +\t\tstd::scoped_lock lock(mutex_, other.mutex_);\n> >> +\t\tdata_.merge(other.data_);\n> >> +\t}\n> >> +\n> >> +\ttemplate\n> >> +\tT *getLocked(std::string const &tag)\n> >> +\t{\n> >> +\t\tauto it = data_.find(tag);\n> >> +\t\tif (it == data_.end())\n> >> +\t\t\treturn nullptr;\n> >> +\t\treturn std::any_cast(&it->second);\n> > \n> > This is a bit dangerous, if T doesn't match the type stored for the tag.\n> > It would of course be a bug in the caller, but if such code appears in\n> > paths that are not frequently taken, it could cause issues that will\n> > result in a crash at runtime later on.\n> > \n> > Could we use a mechanism similar to Control and ControlList to ensure\n> > the right case ?\n> > \n> > template\n> > struct Tag {\n> > \tstd::string tag_;\n> > };\n> > \n> > /* Static list of all tags. */\n> > static constexpr Tag tagFoo{ \"foo\" };\n> > \n> > class Metadata\n> > {\n> > \t...\n> > \ttemplate\n> > \tT *getLock(const Tag &tag)\n> > \t{\n> > \t\t...\n> > \t\treturn std::any_cast(&it->second);\n> > \t}\n> > \t...\n> > };\n> > ...\n> > \n> > \tFoo *f = metadata.get(tagFoo);\n> > \n> > Furthermore, have you considered using integers instead of strings for\n> > tags ? What are the pros and cons ?\n> \n> I think it may be a good idea. Pros I can see:\n> - easy to document the tags\n> - readability (we can always refer to the enum to know how a particular\n> object is mapped).\n> - fastest to find the tag\n> Cons, not much:\n> - if RPi wants to switch to the libipa Metadata class they will need to\n> rewrite a not that small piece of code.\n\nWe can also help ;-)\n\n> - having integer would make the code less dynamic, as we would certainly\n> be tempted to create a static map (which can also be seen as a pro :-)).\n\nThat's my main question. Do you foresee a need to have some sort of\nnamespace in tags ? Will the data stored here be specific to each IPA\nmodule, or would there be some level of standardization ? If we want to\ncreate an IPA module from a set of algorithms, who will be responsible\nfor accessing the storage, would that be code specific to that IPA\nmodule, and/or code from libipa ? There's lots of design questions, I\nhave little visibility on what you're planning.\n\nThe idea of using a template Tag structure as described above is\northogonal to the decision of using strings or integers as identifiers.\nTags would however need to be defined in headers, which I think is an\nupside, as we can enforce documentation in that case. I don't know if it\nwould be feasible (or even a good idea) to centralize all the tag\ndefinitions though, they could be spread across different headers (some\nbeing device-specific, and some being maybe shared accross different IPA\nmodules).\n\n> >> +\t}\n> >> +\n> >> +\ttemplate\n> >> +\tvoid setLocked(std::string const &tag, T const &value)\n> >> +\t{\n> >> +\t\tdata_[tag] = value;\n> >> +\t}\n> >> +\n> >> +\tvoid lock() { mutex_.lock(); }\n> >> +\tvoid unlock() { mutex_.unlock(); }\n> >> +\n> >> +private:\n> >> +\tmutable std::mutex mutex_;\n> >> +\tstd::map data_;\n> >> +};\n> >> +\n> >> +} /* namespace ipa */\n> >> +\n> >> +} /* namespace libcamera */\n> >> +\n> >> +#endif /* __LIBCAMERA_IPA_LIBIPA_METADATA_H__ */","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D1701C3226\n\tfor ;\n\tWed, 14 Jul 2021 17:07:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1815268525;\n\tWed, 14 Jul 2021 19:07:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2EC0868503\n\tfor ;\n\tWed, 14 Jul 2021 19:07:55 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9FC35CC;\n\tWed, 14 Jul 2021 19:07:54 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"M1XdizZ6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626282474;\n\tbh=/yDp93Sz4t8S901lECfOgQMPJVLxpVap/ClDrKZuaio=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=M1XdizZ6zk+ojlzamwHpRfQN+pCTJeq8l+XoNztrJ3LdLZtodOQIoR89BZux6OBGV\n\tMeuhbQuM9Kf+d0Wi3fDWVgNPw0EW4NWcHUwACX7dp8pimQ3XjA2wY9uV290XbDKSAa\n\tEUSoRwIVm+N7bx03PqNmupIn4cAhEnEAbBFEsyak=","Date":"Wed, 14 Jul 2021 20:07:52 +0300","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20210712131630.73914-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210712131630.73914-2-jeanmichel.hautbois@ideasonboard.com>\n\t\n\t<62a0ec5d-0c19-8971-c46f-612c767a6b52@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<62a0ec5d-0c19-8971-c46f-612c767a6b52@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata\n\tclass","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18178,"web_url":"https://patchwork.libcamera.org/comment/18178/","msgid":"","date":"2021-07-14T17:38:42","subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata\n\tclass","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi,\n\n\nOn Wed, 14 Jul 2021 at 18:07, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> On Wed, Jul 14, 2021 at 02:57:11PM +0200, Jean-Michel Hautbois wrote:\n> > Hi Laurent,\n> >\n> > On 14/07/2021 14:48, Laurent Pinchart wrote:\n> > > Hi Jean-Michel,\n> > >\n> > > Thank you for the patch.\n> > >\n> > > On Mon, Jul 12, 2021 at 03:16:29PM +0200, Jean-Michel Hautbois wrote:\n> > >> The Metadata class comes from RPi from which a bit has been removed\n> > >> because we don't need it for now.\n> > >> All functions are inlined in metadata.h because of the template usage.\n> > >>\n> > >> Signed-off-by: Jean-Michel Hautbois <\n> jeanmichel.hautbois@ideasonboard.com>\n> > >> ---\n> > >> src/ipa/ipu3/ipu3.cpp | 1 +\n> > >> src/ipa/libipa/meson.build | 6 ++-\n> > >> src/ipa/libipa/metadata.cpp | 101\n> ++++++++++++++++++++++++++++++++++++\n> > >> src/ipa/libipa/metadata.h | 90 ++++++++++++++++++++++++++++++++\n> > >> 4 files changed, 196 insertions(+), 2 deletions(-)\n> > >> create mode 100644 src/ipa/libipa/metadata.cpp\n> > >> create mode 100644 src/ipa/libipa/metadata.h\n> > >>\n> > >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > >> index 71698d36..091856f5 100644\n> > >> --- a/src/ipa/ipu3/ipu3.cpp\n> > >> +++ b/src/ipa/ipu3/ipu3.cpp\n> > >> @@ -25,6 +25,7 @@\n> > >> #include \"ipu3_agc.h\"\n> > >> #include \"ipu3_awb.h\"\n> > >> #include \"libipa/camera_sensor_helper.h\"\n> > >> +#include \"libipa/metadata.h\"\n> > >>\n> > >> static constexpr uint32_t kMaxCellWidthPerSet = 160;\n> > >> static constexpr uint32_t kMaxCellHeightPerSet = 56;\n> > >> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> > >> index 3fda7c00..cc4e1cc9 100644\n> > >> --- a/src/ipa/libipa/meson.build\n> > >> +++ b/src/ipa/libipa/meson.build\n> > >> @@ -3,13 +3,15 @@\n> > >> libipa_headers = files([\n> > >> 'algorithm.h',\n> > >> 'camera_sensor_helper.h',\n> > >> - 'histogram.h'\n> > >> + 'histogram.h',\n> > >> + 'metadata.h'\n> > >> ])\n> > >>\n> > >> libipa_sources = files([\n> > >> 'algorithm.cpp',\n> > >> 'camera_sensor_helper.cpp',\n> > >> - 'histogram.cpp'\n> > >> + 'histogram.cpp',\n> > >> + 'metadata.cpp'\n> > >> ])\n> > >>\n> > >> libipa_includes = include_directories('..')\n> > >> diff --git a/src/ipa/libipa/metadata.cpp b/src/ipa/libipa/metadata.cpp\n> > >> new file mode 100644\n> > >> index 00000000..b6aef897\n> > >> --- /dev/null\n> > >> +++ b/src/ipa/libipa/metadata.cpp\n> > >> @@ -0,0 +1,101 @@\n> > >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > >> +/*\n> > >> + * Based on the implementation from the Raspberry Pi IPA,\n> > >> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.\n> > >> + * Copyright (C) 2021, Ideas On Board\n> > >> + *\n> > >> + * metadata.cpp - libipa metadata class\n> > >> + */\n> > >> +\n> > >> +#include \"metadata.h\"\n> > >> +\n> > >> +/**\n> > >> + * \\file metadata.h\n> > >> + * \\brief A metadata class to share objects\n> > >> + */\n> > >> +\n> > >> +namespace libcamera {\n> > >> +\n> > >> +namespace ipa {\n> > >> +\n> > >> +/**\n> > >> + * \\class Metadata\n> > >> + * \\brief A simple class for carrying arbitrary metadata, for example\n> > >> + * about an image. It is used to exchange data between algorithms.\n> > >> + */\n> > >> +\n> > >> +/**\n> > >> + * \\fn Metadata::Metadata(Metadata const &other)\n> > >> + * \\param[in] other A Metadata object\n> > >> + *\n> > >> + * Stores the data from one Metadata to another one\n> > >> + */\n> > >> +\n> > >> +/**\n> > >> + * \\fn Metadata::set(std::string const &tag, T const &value)\n> > >> + * \\param[in] tag A string used as the key in a map\n> > >> + * \\param[in] value The value to set into the map\n> > >> + *\n> > >> + * Sets the value in the map to the tag key. The mutex is\n> > >> + * taken for the duration of the block.\n> > >> + */\n> > >> +\n> > >> +/**\n> > >> + * \\fn Metadata::get(std::string const &tag, T &value)\n> > >> + * \\param[in] tag A string used as the key in a map\n> > >> + * \\param[in] value The value to set into the map\n> > >> + *\n> > >> + * Gets the value in the map of the tag key. The mutex is\n> > >> + * taken for the duration of the block.\n> > >> + *\n> > >> + * \\return 0 if value is found, -1 if not existent\n> > >> + */\n> > >> +\n> > >> +/**\n> > >> + * \\fn Metadata::clear()\n> > >> + * Clear the Metadata map. The mutex is taken for the duration of\n> > >> + * the block.\n> > >> + */\n> > >> +\n> > >> +/**\n> > >> + * \\fn Metadata::merge(Metadata &other)\n> > >> + * \\param[in] other A metadata to merge with\n> > >> + * Merge two Metadata maps. The mutex is taken for the duration of\n> > >> + * the block.\n> > >> + */\n> > >> +\n> > >> +/**\n> > >> + * \\fn Metadata::getLocked(std::string const &tag)\n> > >> + * \\param[in] tag A string used as the key in a map\n> > >> + *\n> > >> + * Get the value of the tag key in the map.\n> > >> + * This allows in-place access to the Metadata contents,\n> > >> + * for which you should be holding the lock.\n> > >> + */\n> > >> +\n> > >> +/**\n> > >> + * \\fn Metadata::setLocked(std::string const &tag, T const &value)\n> > >> + * \\param[in] tag A string used as the key in a map\n> > >> + * \\param[in] value The value to set into the map\n> > >> + *\n> > >> + * Set the value to the tag key in the map.\n> > >> + * This allows in-place access to the Metadata contents,\n> > >> + * for which you should be holding the lock.\n> > >> + */\n> > >> +\n> > >> +/**\n> > >> + * \\fn Metadata::lock()\n> > >> + * Lock the mutex with the standard classes.\n> > >> + * e.g. std::lock_guard lock(metadata)\n> > >> + */\n> > >> +\n> > >> +/**\n> > >> + * \\fn Metadata::unlock()\n> > >> + * Unlock the mutex with the standard classes.\n> > >> + * e.g. std::lock_guard lock(metadata)\n> > >> + */\n> > >> +\n> > >> +} /* namespace ipa */\n> > >> +\n> > >> +} /* namespace libcamera */\n> > >> +\n> > >> diff --git a/src/ipa/libipa/metadata.h b/src/ipa/libipa/metadata.h\n> > >> new file mode 100644\n> > >> index 00000000..9801bece\n> > >> --- /dev/null\n> > >> +++ b/src/ipa/libipa/metadata.h\n> > >> @@ -0,0 +1,90 @@\n> > >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > >> +/*\n> > >> + * Based on the implementation from the Raspberry Pi IPA,\n> > >> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.\n> > >> + * Copyright (C) 2021, Ideas On Board\n> > >> + *\n> > >> + * metadata.h - libipa metadata class\n> > >> + */\n> > >> +#ifndef __LIBCAMERA_IPA_LIBIPA_METADATA_H__\n> > >> +#define __LIBCAMERA_IPA_LIBIPA_METADATA_H__\n> > >> +\n> > >> +#include \n> > >> +#include \n> > >> +#include \n> > >> +#include \n> > >> +#include \n> > >> +\n> > >> +namespace libcamera {\n> > >> +\n> > >> +namespace ipa {\n> > >> +\n> > >> +class Metadata\n> > >> +{\n> > >> +public:\n> > >> + Metadata() = default;\n> > >> +\n> > >> + Metadata(Metadata const &other)\n> > >> + {\n> > >> + std::scoped_lock other_lock(other.mutex_);\n> > >> + data_ = other.data_;\n> > >> + }\n> > >> +\n> > >> + template\n> > >> + void set(std::string const &tag, T const &value)\n> > >> + {\n> > >> + std::scoped_lock lock(mutex_);\n> > >> + data_[tag] = value;\n> > >> + }\n> > >> +\n> > >> + template\n> > >> + int get(std::string const &tag, T &value) const\n> > >> + {\n> > >> + std::scoped_lock lock(mutex_);\n> > >\n> > > You can call getLocked() here.\n> > >\n> > Indeed :-)\n> >\n> > >> + auto it = data_.find(tag);\n> > >> + if (it == data_.end())\n> > >> + return -1;\n> > >> + value = std::any_cast(it->second);\n> > >> + return 0;\n> > >> + }\n> > >> +\n> > >> + void clear()\n> > >> + {\n> > >> + std::scoped_lock lock(mutex_);\n> > >> + data_.clear();\n> > >> + }\n> > >> +\n> > >> + void merge(Metadata &other)\n> > >> + {\n> > >> + std::scoped_lock lock(mutex_, other.mutex_);\n> > >> + data_.merge(other.data_);\n> > >> + }\n> > >> +\n> > >> + template\n> > >> + T *getLocked(std::string const &tag)\n> > >> + {\n> > >> + auto it = data_.find(tag);\n> > >> + if (it == data_.end())\n> > >> + return nullptr;\n> > >> + return std::any_cast(&it->second);\n> > >\n> > > This is a bit dangerous, if T doesn't match the type stored for the\n> tag.\n> > > It would of course be a bug in the caller, but if such code appears in\n> > > paths that are not frequently taken, it could cause issues that will\n> > > result in a crash at runtime later on.\n> > >\n> > > Could we use a mechanism similar to Control and ControlList to ensure\n> > > the right case ?\n> > >\n> > > template\n> > > struct Tag {\n> > > std::string tag_;\n> > > };\n> > >\n> > > /* Static list of all tags. */\n> > > static constexpr Tag tagFoo{ \"foo\" };\n> > >\n> > > class Metadata\n> > > {\n> > > ...\n> > > template\n> > > T *getLock(const Tag &tag)\n> > > {\n> > > ...\n> > > return std::any_cast(&it->second);\n> > > }\n> > > ...\n> > > };\n> > > ...\n> > >\n> > > Foo *f = metadata.get(tagFoo);\n> > >\n> > > Furthermore, have you considered using integers instead of strings for\n> > > tags ? What are the pros and cons ?\n> >\n> > I think it may be a good idea. Pros I can see:\n> > - easy to document the tags\n> > - readability (we can always refer to the enum to know how a particular\n> > object is mapped).\n> > - fastest to find the tag\n> > Cons, not much:\n> > - if RPi wants to switch to the libipa Metadata class they will need to\n> > rewrite a not that small piece of code.\n>\n\nWhat is being described here seems suspiciously similar to a ControlList :-)\nPerhaps for your usage, that is more appropriate over our Metadata object?\n\nRegards,\nNaush\n\n\n>\n> We can also help ;-)\n>\n> > - having integer would make the code less dynamic, as we would certainly\n> > be tempted to create a static map (which can also be seen as a pro :-)).\n>\n> That's my main question. Do you foresee a need to have some sort of\n> namespace in tags ? Will the data stored here be specific to each IPA\n> module, or would there be some level of standardization ? If we want to\n> create an IPA module from a set of algorithms, who will be responsible\n> for accessing the storage, would that be code specific to that IPA\n> module, and/or code from libipa ? There's lots of design questions, I\n> have little visibility on what you're planning.\n>\n> The idea of using a template Tag structure as described above is\n> orthogonal to the decision of using strings or integers as identifiers.\n> Tags would however need to be defined in headers, which I think is an\n> upside, as we can enforce documentation in that case. I don't know if it\n> would be feasible (or even a good idea) to centralize all the tag\n> definitions though, they could be spread across different headers (some\n> being device-specific, and some being maybe shared accross different IPA\n> modules).\n>\n> > >> + }\n> > >> +\n> > >> + template\n> > >> + void setLocked(std::string const &tag, T const &value)\n> > >> + {\n> > >> + data_[tag] = value;\n> > >> + }\n> > >> +\n> > >> + void lock() { mutex_.lock(); }\n> > >> + void unlock() { mutex_.unlock(); }\n> > >> +\n> > >> +private:\n> > >> + mutable std::mutex mutex_;\n> > >> + std::map data_;\n> > >> +};\n> > >> +\n> > >> +} /* namespace ipa */\n> > >> +\n> > >> +} /* namespace libcamera */\n> > >> +\n> > >> +#endif /* __LIBCAMERA_IPA_LIBIPA_METADATA_H__ */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 197D3C3225\n\tfor ;\n\tWed, 14 Jul 2021 17:39:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2498968528;\n\tWed, 14 Jul 2021 19:39:02 +0200 (CEST)","from mail-lj1-x22d.google.com (mail-lj1-x22d.google.com\n\t[IPv6:2a00:1450:4864:20::22d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5AC2768503\n\tfor ;\n\tWed, 14 Jul 2021 19:39:00 +0200 (CEST)","by mail-lj1-x22d.google.com with SMTP id r16so4599754ljk.9\n\tfor ;\n\tWed, 14 Jul 2021 10:39:00 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"eLA7sZkk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=J9slSOQPTKnyYaFQpq1PowvJD0/Fsdnts+MBKLg9LOI=;\n\tb=eLA7sZkkEZkgfKp2F64bLpL0kPDOhwTPrUshmWZn+VRKUVpuMNR4C263dVROi542ZG\n\tgAtRdh+ZfzwhjosNmMNKRU/1lwI+4P58HSqwz0kZhQx/peYfAkZ/6/yeHCJnB0O1LzgD\n\teWrPiaaJo44qwv+MxCUUELUX8p67BY/ZIhmeAI7HcUppOLzYNPHtxOt9wfr0wWdiBn79\n\t6ZLQoV6HJ+yHT3/u+UPLGd7/iCS7B8oPHkvv29Yn2+F3nEFnVFZgIaiKkqTVl4EggFh6\n\tb5gQSFUMcll9dKQoZyqNu8vjT6OlzTB8CO4y7EunSP7PZueEri0f1AA6evUMJdz7Lu8b\n\tkRDQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=J9slSOQPTKnyYaFQpq1PowvJD0/Fsdnts+MBKLg9LOI=;\n\tb=iW4cRLnHzFrGmoefamGI6ZQW1Ir0Y7GJE0OdiB/Y3xSgKhFlAf8+mTgAxowgy8QwNH\n\tBvqec2h2i4q26oHnBqwn0uJ3lp6KlE9Q/7bFFse6lOO8DFowYNHjliNzRONcWJ0srwbO\n\tQEzKrmG+MdStFGxx+kC1X/QhyLj126la4KIkqQ8ydo6ZvirgLnosF3CXB4vAkV30rG4G\n\tEiZJOaPN3IzRczBIi/lruw+wstajaAXYz5kr3hflnurq/OHvFLz0brQal8SbtEo2ckTH\n\tFTphtnPKQL8msrb1TdY0WDgOW1zPhc6bcyPTJNqKBXkyRljRBF+yE4nx0W5fcV56cD00\n\t/Vdw==","X-Gm-Message-State":"AOAM5315xhmFK8jUPHPSCPAMPWTGxVEWOCsZ1KoQc7iQai8Okkj+pN2U\n\tw/q/yfDBS86b8TMZPUkmWaDxoOw9Y6ZrSl01H364eQ==","X-Google-Smtp-Source":"ABdhPJytP3FZ9ggoFXcxzs9BIJ/TSX6qiAteR3Rdpghf1DHBpB51OgqXi8QlD8G0WkhQrJLwycIjRpT6kC2oDwUMj1I=","X-Received":"by 2002:a05:651c:3d0:: with SMTP id\n\tf16mr9292118ljp.169.1626284339552; \n\tWed, 14 Jul 2021 10:38:59 -0700 (PDT)","MIME-Version":"1.0","References":"<20210712131630.73914-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210712131630.73914-2-jeanmichel.hautbois@ideasonboard.com>\n\t\n\t<62a0ec5d-0c19-8971-c46f-612c767a6b52@ideasonboard.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Wed, 14 Jul 2021 18:38:42 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"0000000000005d3f0205c718d46e\"","Subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata\n\tclass","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18179,"web_url":"https://patchwork.libcamera.org/comment/18179/","msgid":"","date":"2021-07-14T17:43:53","subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata\n\tclass","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Wed, Jul 14, 2021 at 06:38:42PM +0100, Naushir Patuck wrote:\n> On Wed, 14 Jul 2021 at 18:07, Laurent Pinchart wrote:\n> > On Wed, Jul 14, 2021 at 02:57:11PM +0200, Jean-Michel Hautbois wrote:\n> > > On 14/07/2021 14:48, Laurent Pinchart wrote:\n> > > > On Mon, Jul 12, 2021 at 03:16:29PM +0200, Jean-Michel Hautbois wrote:\n> > > >> The Metadata class comes from RPi from which a bit has been removed\n> > > >> because we don't need it for now.\n> > > >> All functions are inlined in metadata.h because of the template usage.\n> > > >>\n> > > >> Signed-off-by: Jean-Michel Hautbois < jeanmichel.hautbois@ideasonboard.com>\n> > > >> ---\n> > > >> src/ipa/ipu3/ipu3.cpp | 1 +\n> > > >> src/ipa/libipa/meson.build | 6 ++-\n> > > >> src/ipa/libipa/metadata.cpp | 101 ++++++++++++++++++++++++++++++++++++\n> > > >> src/ipa/libipa/metadata.h | 90 ++++++++++++++++++++++++++++++++\n> > > >> 4 files changed, 196 insertions(+), 2 deletions(-)\n> > > >> create mode 100644 src/ipa/libipa/metadata.cpp\n> > > >> create mode 100644 src/ipa/libipa/metadata.h\n> > > >>\n> > > >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > > >> index 71698d36..091856f5 100644\n> > > >> --- a/src/ipa/ipu3/ipu3.cpp\n> > > >> +++ b/src/ipa/ipu3/ipu3.cpp\n> > > >> @@ -25,6 +25,7 @@\n> > > >> #include \"ipu3_agc.h\"\n> > > >> #include \"ipu3_awb.h\"\n> > > >> #include \"libipa/camera_sensor_helper.h\"\n> > > >> +#include \"libipa/metadata.h\"\n> > > >>\n> > > >> static constexpr uint32_t kMaxCellWidthPerSet = 160;\n> > > >> static constexpr uint32_t kMaxCellHeightPerSet = 56;\n> > > >> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> > > >> index 3fda7c00..cc4e1cc9 100644\n> > > >> --- a/src/ipa/libipa/meson.build\n> > > >> +++ b/src/ipa/libipa/meson.build\n> > > >> @@ -3,13 +3,15 @@\n> > > >> libipa_headers = files([\n> > > >> 'algorithm.h',\n> > > >> 'camera_sensor_helper.h',\n> > > >> - 'histogram.h'\n> > > >> + 'histogram.h',\n> > > >> + 'metadata.h'\n> > > >> ])\n> > > >>\n> > > >> libipa_sources = files([\n> > > >> 'algorithm.cpp',\n> > > >> 'camera_sensor_helper.cpp',\n> > > >> - 'histogram.cpp'\n> > > >> + 'histogram.cpp',\n> > > >> + 'metadata.cpp'\n> > > >> ])\n> > > >>\n> > > >> libipa_includes = include_directories('..')\n> > > >> diff --git a/src/ipa/libipa/metadata.cpp b/src/ipa/libipa/metadata.cpp\n> > > >> new file mode 100644\n> > > >> index 00000000..b6aef897\n> > > >> --- /dev/null\n> > > >> +++ b/src/ipa/libipa/metadata.cpp\n> > > >> @@ -0,0 +1,101 @@\n> > > >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > >> +/*\n> > > >> + * Based on the implementation from the Raspberry Pi IPA,\n> > > >> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.\n> > > >> + * Copyright (C) 2021, Ideas On Board\n> > > >> + *\n> > > >> + * metadata.cpp - libipa metadata class\n> > > >> + */\n> > > >> +\n> > > >> +#include \"metadata.h\"\n> > > >> +\n> > > >> +/**\n> > > >> + * \\file metadata.h\n> > > >> + * \\brief A metadata class to share objects\n> > > >> + */\n> > > >> +\n> > > >> +namespace libcamera {\n> > > >> +\n> > > >> +namespace ipa {\n> > > >> +\n> > > >> +/**\n> > > >> + * \\class Metadata\n> > > >> + * \\brief A simple class for carrying arbitrary metadata, for example\n> > > >> + * about an image. It is used to exchange data between algorithms.\n> > > >> + */\n> > > >> +\n> > > >> +/**\n> > > >> + * \\fn Metadata::Metadata(Metadata const &other)\n> > > >> + * \\param[in] other A Metadata object\n> > > >> + *\n> > > >> + * Stores the data from one Metadata to another one\n> > > >> + */\n> > > >> +\n> > > >> +/**\n> > > >> + * \\fn Metadata::set(std::string const &tag, T const &value)\n> > > >> + * \\param[in] tag A string used as the key in a map\n> > > >> + * \\param[in] value The value to set into the map\n> > > >> + *\n> > > >> + * Sets the value in the map to the tag key. The mutex is\n> > > >> + * taken for the duration of the block.\n> > > >> + */\n> > > >> +\n> > > >> +/**\n> > > >> + * \\fn Metadata::get(std::string const &tag, T &value)\n> > > >> + * \\param[in] tag A string used as the key in a map\n> > > >> + * \\param[in] value The value to set into the map\n> > > >> + *\n> > > >> + * Gets the value in the map of the tag key. The mutex is\n> > > >> + * taken for the duration of the block.\n> > > >> + *\n> > > >> + * \\return 0 if value is found, -1 if not existent\n> > > >> + */\n> > > >> +\n> > > >> +/**\n> > > >> + * \\fn Metadata::clear()\n> > > >> + * Clear the Metadata map. The mutex is taken for the duration of\n> > > >> + * the block.\n> > > >> + */\n> > > >> +\n> > > >> +/**\n> > > >> + * \\fn Metadata::merge(Metadata &other)\n> > > >> + * \\param[in] other A metadata to merge with\n> > > >> + * Merge two Metadata maps. The mutex is taken for the duration of\n> > > >> + * the block.\n> > > >> + */\n> > > >> +\n> > > >> +/**\n> > > >> + * \\fn Metadata::getLocked(std::string const &tag)\n> > > >> + * \\param[in] tag A string used as the key in a map\n> > > >> + *\n> > > >> + * Get the value of the tag key in the map.\n> > > >> + * This allows in-place access to the Metadata contents,\n> > > >> + * for which you should be holding the lock.\n> > > >> + */\n> > > >> +\n> > > >> +/**\n> > > >> + * \\fn Metadata::setLocked(std::string const &tag, T const &value)\n> > > >> + * \\param[in] tag A string used as the key in a map\n> > > >> + * \\param[in] value The value to set into the map\n> > > >> + *\n> > > >> + * Set the value to the tag key in the map.\n> > > >> + * This allows in-place access to the Metadata contents,\n> > > >> + * for which you should be holding the lock.\n> > > >> + */\n> > > >> +\n> > > >> +/**\n> > > >> + * \\fn Metadata::lock()\n> > > >> + * Lock the mutex with the standard classes.\n> > > >> + * e.g. std::lock_guard lock(metadata)\n> > > >> + */\n> > > >> +\n> > > >> +/**\n> > > >> + * \\fn Metadata::unlock()\n> > > >> + * Unlock the mutex with the standard classes.\n> > > >> + * e.g. std::lock_guard lock(metadata)\n> > > >> + */\n> > > >> +\n> > > >> +} /* namespace ipa */\n> > > >> +\n> > > >> +} /* namespace libcamera */\n> > > >> +\n> > > >> diff --git a/src/ipa/libipa/metadata.h b/src/ipa/libipa/metadata.h\n> > > >> new file mode 100644\n> > > >> index 00000000..9801bece\n> > > >> --- /dev/null\n> > > >> +++ b/src/ipa/libipa/metadata.h\n> > > >> @@ -0,0 +1,90 @@\n> > > >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > >> +/*\n> > > >> + * Based on the implementation from the Raspberry Pi IPA,\n> > > >> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.\n> > > >> + * Copyright (C) 2021, Ideas On Board\n> > > >> + *\n> > > >> + * metadata.h - libipa metadata class\n> > > >> + */\n> > > >> +#ifndef __LIBCAMERA_IPA_LIBIPA_METADATA_H__\n> > > >> +#define __LIBCAMERA_IPA_LIBIPA_METADATA_H__\n> > > >> +\n> > > >> +#include \n> > > >> +#include \n> > > >> +#include \n> > > >> +#include \n> > > >> +#include \n> > > >> +\n> > > >> +namespace libcamera {\n> > > >> +\n> > > >> +namespace ipa {\n> > > >> +\n> > > >> +class Metadata\n> > > >> +{\n> > > >> +public:\n> > > >> + Metadata() = default;\n> > > >> +\n> > > >> + Metadata(Metadata const &other)\n> > > >> + {\n> > > >> + std::scoped_lock other_lock(other.mutex_);\n> > > >> + data_ = other.data_;\n> > > >> + }\n> > > >> +\n> > > >> + template\n> > > >> + void set(std::string const &tag, T const &value)\n> > > >> + {\n> > > >> + std::scoped_lock lock(mutex_);\n> > > >> + data_[tag] = value;\n> > > >> + }\n> > > >> +\n> > > >> + template\n> > > >> + int get(std::string const &tag, T &value) const\n> > > >> + {\n> > > >> + std::scoped_lock lock(mutex_);\n> > > >\n> > > > You can call getLocked() here.\n> > > >\n> > > Indeed :-)\n> > >\n> > > >> + auto it = data_.find(tag);\n> > > >> + if (it == data_.end())\n> > > >> + return -1;\n> > > >> + value = std::any_cast(it->second);\n> > > >> + return 0;\n> > > >> + }\n> > > >> +\n> > > >> + void clear()\n> > > >> + {\n> > > >> + std::scoped_lock lock(mutex_);\n> > > >> + data_.clear();\n> > > >> + }\n> > > >> +\n> > > >> + void merge(Metadata &other)\n> > > >> + {\n> > > >> + std::scoped_lock lock(mutex_, other.mutex_);\n> > > >> + data_.merge(other.data_);\n> > > >> + }\n> > > >> +\n> > > >> + template\n> > > >> + T *getLocked(std::string const &tag)\n> > > >> + {\n> > > >> + auto it = data_.find(tag);\n> > > >> + if (it == data_.end())\n> > > >> + return nullptr;\n> > > >> + return std::any_cast(&it->second);\n> > > >\n> > > > This is a bit dangerous, if T doesn't match the type stored for the tag.\n> > > > It would of course be a bug in the caller, but if such code appears in\n> > > > paths that are not frequently taken, it could cause issues that will\n> > > > result in a crash at runtime later on.\n> > > >\n> > > > Could we use a mechanism similar to Control and ControlList to ensure\n> > > > the right case ?\n> > > >\n> > > > template\n> > > > struct Tag {\n> > > > std::string tag_;\n> > > > };\n> > > >\n> > > > /* Static list of all tags. */\n> > > > static constexpr Tag tagFoo{ \"foo\" };\n> > > >\n> > > > class Metadata\n> > > > {\n> > > > ...\n> > > > template\n> > > > T *getLock(const Tag &tag)\n> > > > {\n> > > > ...\n> > > > return std::any_cast(&it->second);\n> > > > }\n> > > > ...\n> > > > };\n> > > > ...\n> > > >\n> > > > Foo *f = metadata.get(tagFoo);\n> > > >\n> > > > Furthermore, have you considered using integers instead of strings for\n> > > > tags ? What are the pros and cons ?\n> > >\n> > > I think it may be a good idea. Pros I can see:\n> > > - easy to document the tags\n> > > - readability (we can always refer to the enum to know how a particular\n> > > object is mapped).\n> > > - fastest to find the tag\n> > > Cons, not much:\n> > > - if RPi wants to switch to the libipa Metadata class they will need to\n> > > rewrite a not that small piece of code.\n> \n> What is being described here seems suspiciously similar to a ControlList :-)\n\nA bit indeed, but with an std::any instead of a small set of supported\ntypes, and no support for ControlInfo. It's \"just\" a way to ensure we\nwon't get the type T wrong in a call.\n\n> Perhaps for your usage, that is more appropriate over our Metadata object?\n\nDo you think the usage in the Raspberry Pi IPA differs significantly ?\n\n> > We can also help ;-)\n> >\n> > > - having integer would make the code less dynamic, as we would certainly\n> > > be tempted to create a static map (which can also be seen as a pro :-)).\n> >\n> > That's my main question. Do you foresee a need to have some sort of\n> > namespace in tags ? Will the data stored here be specific to each IPA\n> > module, or would there be some level of standardization ? If we want to\n> > create an IPA module from a set of algorithms, who will be responsible\n> > for accessing the storage, would that be code specific to that IPA\n> > module, and/or code from libipa ? There's lots of design questions, I\n> > have little visibility on what you're planning.\n> >\n> > The idea of using a template Tag structure as described above is\n> > orthogonal to the decision of using strings or integers as identifiers.\n> > Tags would however need to be defined in headers, which I think is an\n> > upside, as we can enforce documentation in that case. I don't know if it\n> > would be feasible (or even a good idea) to centralize all the tag\n> > definitions though, they could be spread across different headers (some\n> > being device-specific, and some being maybe shared accross different IPA\n> > modules).\n> >\n> > > >> + }\n> > > >> +\n> > > >> + template\n> > > >> + void setLocked(std::string const &tag, T const &value)\n> > > >> + {\n> > > >> + data_[tag] = value;\n> > > >> + }\n> > > >> +\n> > > >> + void lock() { mutex_.lock(); }\n> > > >> + void unlock() { mutex_.unlock(); }\n> > > >> +\n> > > >> +private:\n> > > >> + mutable std::mutex mutex_;\n> > > >> + std::map data_;\n> > > >> +};\n> > > >> +\n> > > >> +} /* namespace ipa */\n> > > >> +\n> > > >> +} /* namespace libcamera */\n> > > >> +\n> > > >> +#endif /* __LIBCAMERA_IPA_LIBIPA_METADATA_H__ */","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 32568C3226\n\tfor ;\n\tWed, 14 Jul 2021 17:43:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A14F668525;\n\tWed, 14 Jul 2021 19:43:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C7E5168503\n\tfor ;\n\tWed, 14 Jul 2021 19:43:54 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 39A34CC;\n\tWed, 14 Jul 2021 19:43:54 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"enaykWRD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626284634;\n\tbh=LgY7jFlp/U+a2i7oV/u7q+eQJNLA/j6Spyca0o46tVw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=enaykWRDwdNCJG52kr7Rphs3Sr7hfBbgzB1VOG6gLHH6QaJmoCuNLFt7Dta72wQMF\n\tVcDO97eo0uyns6b5pgx9re6SsstZM2RpgkksvWVp67wWlGuFbRPyI7B2sGWYHcJaJ2\n\t2mpaOzjh4hNaIaiNZJ7F5ZemDmgb6MM7tc418iAU=","Date":"Wed, 14 Jul 2021 20:43:53 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210712131630.73914-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210712131630.73914-2-jeanmichel.hautbois@ideasonboard.com>\n\t\n\t<62a0ec5d-0c19-8971-c46f-612c767a6b52@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata\n\tclass","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18180,"web_url":"https://patchwork.libcamera.org/comment/18180/","msgid":"","date":"2021-07-14T18:40:51","subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata\n\tclass","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Wed, 14 Jul 2021 at 18:43, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> On Wed, Jul 14, 2021 at 06:38:42PM +0100, Naushir Patuck wrote:\n> > On Wed, 14 Jul 2021 at 18:07, Laurent Pinchart wrote:\n> > > On Wed, Jul 14, 2021 at 02:57:11PM +0200, Jean-Michel Hautbois wrote:\n> > > > On 14/07/2021 14:48, Laurent Pinchart wrote:\n> > > > > On Mon, Jul 12, 2021 at 03:16:29PM +0200, Jean-Michel Hautbois\n> wrote:\n> > > > >> The Metadata class comes from RPi from which a bit has been\n> removed\n> > > > >> because we don't need it for now.\n> > > > >> All functions are inlined in metadata.h because of the template\n> usage.\n> > > > >>\n> > > > >> Signed-off-by: Jean-Michel Hautbois <\n> jeanmichel.hautbois@ideasonboard.com>\n> > > > >> ---\n> > > > >> src/ipa/ipu3/ipu3.cpp | 1 +\n> > > > >> src/ipa/libipa/meson.build | 6 ++-\n> > > > >> src/ipa/libipa/metadata.cpp | 101\n> ++++++++++++++++++++++++++++++++++++\n> > > > >> src/ipa/libipa/metadata.h | 90\n> ++++++++++++++++++++++++++++++++\n> > > > >> 4 files changed, 196 insertions(+), 2 deletions(-)\n> > > > >> create mode 100644 src/ipa/libipa/metadata.cpp\n> > > > >> create mode 100644 src/ipa/libipa/metadata.h\n> > > > >>\n> > > > >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > > > >> index 71698d36..091856f5 100644\n> > > > >> --- a/src/ipa/ipu3/ipu3.cpp\n> > > > >> +++ b/src/ipa/ipu3/ipu3.cpp\n> > > > >> @@ -25,6 +25,7 @@\n> > > > >> #include \"ipu3_agc.h\"\n> > > > >> #include \"ipu3_awb.h\"\n> > > > >> #include \"libipa/camera_sensor_helper.h\"\n> > > > >> +#include \"libipa/metadata.h\"\n> > > > >>\n> > > > >> static constexpr uint32_t kMaxCellWidthPerSet = 160;\n> > > > >> static constexpr uint32_t kMaxCellHeightPerSet = 56;\n> > > > >> diff --git a/src/ipa/libipa/meson.build\n> b/src/ipa/libipa/meson.build\n> > > > >> index 3fda7c00..cc4e1cc9 100644\n> > > > >> --- a/src/ipa/libipa/meson.build\n> > > > >> +++ b/src/ipa/libipa/meson.build\n> > > > >> @@ -3,13 +3,15 @@\n> > > > >> libipa_headers = files([\n> > > > >> 'algorithm.h',\n> > > > >> 'camera_sensor_helper.h',\n> > > > >> - 'histogram.h'\n> > > > >> + 'histogram.h',\n> > > > >> + 'metadata.h'\n> > > > >> ])\n> > > > >>\n> > > > >> libipa_sources = files([\n> > > > >> 'algorithm.cpp',\n> > > > >> 'camera_sensor_helper.cpp',\n> > > > >> - 'histogram.cpp'\n> > > > >> + 'histogram.cpp',\n> > > > >> + 'metadata.cpp'\n> > > > >> ])\n> > > > >>\n> > > > >> libipa_includes = include_directories('..')\n> > > > >> diff --git a/src/ipa/libipa/metadata.cpp\n> b/src/ipa/libipa/metadata.cpp\n> > > > >> new file mode 100644\n> > > > >> index 00000000..b6aef897\n> > > > >> --- /dev/null\n> > > > >> +++ b/src/ipa/libipa/metadata.cpp\n> > > > >> @@ -0,0 +1,101 @@\n> > > > >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > >> +/*\n> > > > >> + * Based on the implementation from the Raspberry Pi IPA,\n> > > > >> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.\n> > > > >> + * Copyright (C) 2021, Ideas On Board\n> > > > >> + *\n> > > > >> + * metadata.cpp - libipa metadata class\n> > > > >> + */\n> > > > >> +\n> > > > >> +#include \"metadata.h\"\n> > > > >> +\n> > > > >> +/**\n> > > > >> + * \\file metadata.h\n> > > > >> + * \\brief A metadata class to share objects\n> > > > >> + */\n> > > > >> +\n> > > > >> +namespace libcamera {\n> > > > >> +\n> > > > >> +namespace ipa {\n> > > > >> +\n> > > > >> +/**\n> > > > >> + * \\class Metadata\n> > > > >> + * \\brief A simple class for carrying arbitrary metadata, for\n> example\n> > > > >> + * about an image. It is used to exchange data between\n> algorithms.\n> > > > >> + */\n> > > > >> +\n> > > > >> +/**\n> > > > >> + * \\fn Metadata::Metadata(Metadata const &other)\n> > > > >> + * \\param[in] other A Metadata object\n> > > > >> + *\n> > > > >> + * Stores the data from one Metadata to another one\n> > > > >> + */\n> > > > >> +\n> > > > >> +/**\n> > > > >> + * \\fn Metadata::set(std::string const &tag, T const &value)\n> > > > >> + * \\param[in] tag A string used as the key in a map\n> > > > >> + * \\param[in] value The value to set into the map\n> > > > >> + *\n> > > > >> + * Sets the value in the map to the tag key. The mutex is\n> > > > >> + * taken for the duration of the block.\n> > > > >> + */\n> > > > >> +\n> > > > >> +/**\n> > > > >> + * \\fn Metadata::get(std::string const &tag, T &value)\n> > > > >> + * \\param[in] tag A string used as the key in a map\n> > > > >> + * \\param[in] value The value to set into the map\n> > > > >> + *\n> > > > >> + * Gets the value in the map of the tag key. The mutex is\n> > > > >> + * taken for the duration of the block.\n> > > > >> + *\n> > > > >> + * \\return 0 if value is found, -1 if not existent\n> > > > >> + */\n> > > > >> +\n> > > > >> +/**\n> > > > >> + * \\fn Metadata::clear()\n> > > > >> + * Clear the Metadata map. The mutex is taken for the duration of\n> > > > >> + * the block.\n> > > > >> + */\n> > > > >> +\n> > > > >> +/**\n> > > > >> + * \\fn Metadata::merge(Metadata &other)\n> > > > >> + * \\param[in] other A metadata to merge with\n> > > > >> + * Merge two Metadata maps. The mutex is taken for the duration\n> of\n> > > > >> + * the block.\n> > > > >> + */\n> > > > >> +\n> > > > >> +/**\n> > > > >> + * \\fn Metadata::getLocked(std::string const &tag)\n> > > > >> + * \\param[in] tag A string used as the key in a map\n> > > > >> + *\n> > > > >> + * Get the value of the tag key in the map.\n> > > > >> + * This allows in-place access to the Metadata contents,\n> > > > >> + * for which you should be holding the lock.\n> > > > >> + */\n> > > > >> +\n> > > > >> +/**\n> > > > >> + * \\fn Metadata::setLocked(std::string const &tag, T const\n> &value)\n> > > > >> + * \\param[in] tag A string used as the key in a map\n> > > > >> + * \\param[in] value The value to set into the map\n> > > > >> + *\n> > > > >> + * Set the value to the tag key in the map.\n> > > > >> + * This allows in-place access to the Metadata contents,\n> > > > >> + * for which you should be holding the lock.\n> > > > >> + */\n> > > > >> +\n> > > > >> +/**\n> > > > >> + * \\fn Metadata::lock()\n> > > > >> + * Lock the mutex with the standard classes.\n> > > > >> + * e.g. std::lock_guard lock(metadata)\n> > > > >> + */\n> > > > >> +\n> > > > >> +/**\n> > > > >> + * \\fn Metadata::unlock()\n> > > > >> + * Unlock the mutex with the standard classes.\n> > > > >> + * e.g. std::lock_guard lock(metadata)\n> > > > >> + */\n> > > > >> +\n> > > > >> +} /* namespace ipa */\n> > > > >> +\n> > > > >> +} /* namespace libcamera */\n> > > > >> +\n> > > > >> diff --git a/src/ipa/libipa/metadata.h b/src/ipa/libipa/metadata.h\n> > > > >> new file mode 100644\n> > > > >> index 00000000..9801bece\n> > > > >> --- /dev/null\n> > > > >> +++ b/src/ipa/libipa/metadata.h\n> > > > >> @@ -0,0 +1,90 @@\n> > > > >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > >> +/*\n> > > > >> + * Based on the implementation from the Raspberry Pi IPA,\n> > > > >> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.\n> > > > >> + * Copyright (C) 2021, Ideas On Board\n> > > > >> + *\n> > > > >> + * metadata.h - libipa metadata class\n> > > > >> + */\n> > > > >> +#ifndef __LIBCAMERA_IPA_LIBIPA_METADATA_H__\n> > > > >> +#define __LIBCAMERA_IPA_LIBIPA_METADATA_H__\n> > > > >> +\n> > > > >> +#include \n> > > > >> +#include \n> > > > >> +#include \n> > > > >> +#include \n> > > > >> +#include \n> > > > >> +\n> > > > >> +namespace libcamera {\n> > > > >> +\n> > > > >> +namespace ipa {\n> > > > >> +\n> > > > >> +class Metadata\n> > > > >> +{\n> > > > >> +public:\n> > > > >> + Metadata() = default;\n> > > > >> +\n> > > > >> + Metadata(Metadata const &other)\n> > > > >> + {\n> > > > >> + std::scoped_lock other_lock(other.mutex_);\n> > > > >> + data_ = other.data_;\n> > > > >> + }\n> > > > >> +\n> > > > >> + template\n> > > > >> + void set(std::string const &tag, T const &value)\n> > > > >> + {\n> > > > >> + std::scoped_lock lock(mutex_);\n> > > > >> + data_[tag] = value;\n> > > > >> + }\n> > > > >> +\n> > > > >> + template\n> > > > >> + int get(std::string const &tag, T &value) const\n> > > > >> + {\n> > > > >> + std::scoped_lock lock(mutex_);\n> > > > >\n> > > > > You can call getLocked() here.\n> > > > >\n> > > > Indeed :-)\n> > > >\n> > > > >> + auto it = data_.find(tag);\n> > > > >> + if (it == data_.end())\n> > > > >> + return -1;\n> > > > >> + value = std::any_cast(it->second);\n> > > > >> + return 0;\n> > > > >> + }\n> > > > >> +\n> > > > >> + void clear()\n> > > > >> + {\n> > > > >> + std::scoped_lock lock(mutex_);\n> > > > >> + data_.clear();\n> > > > >> + }\n> > > > >> +\n> > > > >> + void merge(Metadata &other)\n> > > > >> + {\n> > > > >> + std::scoped_lock lock(mutex_, other.mutex_);\n> > > > >> + data_.merge(other.data_);\n> > > > >> + }\n> > > > >> +\n> > > > >> + template\n> > > > >> + T *getLocked(std::string const &tag)\n> > > > >> + {\n> > > > >> + auto it = data_.find(tag);\n> > > > >> + if (it == data_.end())\n> > > > >> + return nullptr;\n> > > > >> + return std::any_cast(&it->second);\n> > > > >\n> > > > > This is a bit dangerous, if T doesn't match the type stored for\n> the tag.\n> > > > > It would of course be a bug in the caller, but if such code\n> appears in\n> > > > > paths that are not frequently taken, it could cause issues that\n> will\n> > > > > result in a crash at runtime later on.\n> > > > >\n> > > > > Could we use a mechanism similar to Control and ControlList to\n> ensure\n> > > > > the right case ?\n> > > > >\n> > > > > template\n> > > > > struct Tag {\n> > > > > std::string tag_;\n> > > > > };\n> > > > >\n> > > > > /* Static list of all tags. */\n> > > > > static constexpr Tag tagFoo{ \"foo\" };\n> > > > >\n> > > > > class Metadata\n> > > > > {\n> > > > > ...\n> > > > > template\n> > > > > T *getLock(const Tag &tag)\n> > > > > {\n> > > > > ...\n> > > > > return std::any_cast(&it->second);\n> > > > > }\n> > > > > ...\n> > > > > };\n> > > > > ...\n> > > > >\n> > > > > Foo *f = metadata.get(tagFoo);\n> > > > >\n> > > > > Furthermore, have you considered using integers instead of strings\n> for\n> > > > > tags ? What are the pros and cons ?\n> > > >\n> > > > I think it may be a good idea. Pros I can see:\n> > > > - easy to document the tags\n> > > > - readability (we can always refer to the enum to know how a\n> particular\n> > > > object is mapped).\n> > > > - fastest to find the tag\n> > > > Cons, not much:\n> > > > - if RPi wants to switch to the libipa Metadata class they will need\n> to\n> > > > rewrite a not that small piece of code.\n> >\n> > What is being described here seems suspiciously similar to a ControlList\n> :-)\n>\n> A bit indeed, but with an std::any instead of a small set of supported\n> types, and no support for ControlInfo. It's \"just\" a way to ensure we\n> won't get the type T wrong in a call.\n>\n\nI was thinking along the lines of using ControlValues of type Span\nlike\nwe do in the IPA to set ISP controls. It does not exactly have the\nconvenience\nof std::any usage, but does allow arbitrary structs to be placed into the\nControlList.\n\n\n>\n> > Perhaps for your usage, that is more appropriate over our Metadata\n> object?\n\n\n> Do you think the usage in the Raspberry Pi IPA differs significantly ?\n>\n\nFor RPi, we like the convenience and ease of use for the existing method.\nYou can just call set/get without any ancillary setup, which is a big\nreason we\nchose strings over enums for key types. This code predated our IPA, so\nwe used to take care of std::any type mismatches using exceptions, which\nwere\nremoved when we ported our code across. Without this, you do need to be\nsomewhat careful not to trip over.\n\nOther than that, usage wise, I expect things to be similar to what JM\nintends, but will\nhave to wait and see. If this is indeed the case, I expect all key/value\npairs to be very\nspecific to each IPA and doubt much (if any) could be shared across\nvarious vendors.\nBecause of this, and the likelihood that these structures will only be used\ninternally to\nthe IPA, do you see auto-generated documentation to be required? Of course\nthe structs\nand fields must be documented in the source files :-)\n\nRegards,\nNaush\n\n\n>\n> > > We can also help ;-)\n> > >\n> > > > - having integer would make the code less dynamic, as we would\n> certainly\n> > > > be tempted to create a static map (which can also be seen as a pro\n> :-)).\n> > >\n> > > That's my main question. Do you foresee a need to have some sort of\n> > > namespace in tags ? Will the data stored here be specific to each IPA\n> > > module, or would there be some level of standardization ? If we want to\n> > > create an IPA module from a set of algorithms, who will be responsible\n> > > for accessing the storage, would that be code specific to that IPA\n> > > module, and/or code from libipa ? There's lots of design questions, I\n> > > have little visibility on what you're planning.\n> > >\n> > > The idea of using a template Tag structure as described above is\n> > > orthogonal to the decision of using strings or integers as identifiers.\n> > > Tags would however need to be defined in headers, which I think is an\n> > > upside, as we can enforce documentation in that case. I don't know if\n> it\n> > > would be feasible (or even a good idea) to centralize all the tag\n> > > definitions though, they could be spread across different headers (some\n> > > being device-specific, and some being maybe shared accross different\n> IPA\n> > > modules).\n> > >\n> > > > >> + }\n> > > > >> +\n> > > > >> + template\n> > > > >> + void setLocked(std::string const &tag, T const &value)\n> > > > >> + {\n> > > > >> + data_[tag] = value;\n> > > > >> + }\n> > > > >> +\n> > > > >> + void lock() { mutex_.lock(); }\n> > > > >> + void unlock() { mutex_.unlock(); }\n> > > > >> +\n> > > > >> +private:\n> > > > >> + mutable std::mutex mutex_;\n> > > > >> + std::map data_;\n> > > > >> +};\n> > > > >> +\n> > > > >> +} /* namespace ipa */\n> > > > >> +\n> > > > >> +} /* namespace libcamera */\n> > > > >> +\n> > > > >> +#endif /* __LIBCAMERA_IPA_LIBIPA_METADATA_H__ */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C7974C3225\n\tfor ;\n\tWed, 14 Jul 2021 18:41:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6F57468525;\n\tWed, 14 Jul 2021 20:41:11 +0200 (CEST)","from mail-lf1-x131.google.com (mail-lf1-x131.google.com\n\t[IPv6:2a00:1450:4864:20::131])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1FC2A68503\n\tfor ;\n\tWed, 14 Jul 2021 20:41:09 +0200 (CEST)","by mail-lf1-x131.google.com with SMTP id i5so5316306lfe.2\n\tfor ;\n\tWed, 14 Jul 2021 11:41:09 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"Gm8sux2R\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=6OCySEbacmga84NA24H6KEZ8GLm8TbC3lL7m6mVYIAw=;\n\tb=Gm8sux2R3bhV1V6O3IfhLykY8TrJiPaVxW5WnM1geyCY888yHhIeGsK0Z8uGTw+wip\n\tGTqM4+wyXr+ZjYOnxy1h94HI80KpPtq7/E7p4VB/SiY9fSDMKh/eqYBgWDXX3T/q0pcY\n\tWXmpVIHfDgYitpyy5lqAv3CQjJZM7Px4hOc/CVBDFaOG5shJfoIKL7XFzmAT1nmisfa0\n\t48NgBIwkShM+aFjnO1uaZyAl8fLIoApzGPEIlNmOe2TIkrD/nBjAqSgOElfuyOWDmOqB\n\tNIE3wA8zma6sH1Ky7GFS0jbD2q3SmjTeOWTyN0ZbZSNKTdeMA+P81lhVZztJcnAhuSyq\n\t+yEg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=6OCySEbacmga84NA24H6KEZ8GLm8TbC3lL7m6mVYIAw=;\n\tb=sdUw3OCqWwmMG+HyaCq3Qy50syAUuXAxFWZ9LvjhmNMPBalIKo5UNLsNruQlCVa3JP\n\tGGUi7fvhdXVYgXX8HmiRK8mQ5ch+jwKL3FRDQ0iRWXVIKskjCxNfghLXI3eEmJB/zrVd\n\teb4Lc25PLAXEgqJNYSnNl7B3k4RpjxCu0P5XbqvCPF7Plw8DH9zq7FV58v5IoNk5C8ut\n\tjt+4m4JGdnG7GbXPcuVM5uH5M3IxkjDNbnsLSA/7ZNgFzcr/fjy5y8J50SCDGNHm6uma\n\tINyiiEfsz9fhYcojoEVcMaUGwpFKPNHaVL4F1B7s+aFYBxhcl9+1m1pgjcmBt1g//Nh1\n\t3awg==","X-Gm-Message-State":"AOAM532itDWzNOCaGQ7PW/EVKtj0ffJBUmCSQZdKAnLyzLIRusSOZlcq\n\tYGq74IwVViSH0oDInMmiJOhhEbYAjP/KU9Gwm+UikTnJHJb/QA==","X-Google-Smtp-Source":"ABdhPJxBzi7IbcqrZSRIZFZr9IuZrmYLTj63TXdVsFNs4koMLReIhzrwON38zCFBKKQXnlgcoxKiYdWDq+ve16+SGUg=","X-Received":"by 2002:a19:48d3:: with SMTP id\n\tv202mr8851410lfa.617.1626288068395; \n\tWed, 14 Jul 2021 11:41:08 -0700 (PDT)","MIME-Version":"1.0","References":"<20210712131630.73914-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210712131630.73914-2-jeanmichel.hautbois@ideasonboard.com>\n\t\n\t<62a0ec5d-0c19-8971-c46f-612c767a6b52@ideasonboard.com>\n\t\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Wed, 14 Jul 2021 19:40:51 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"0000000000009e9d0d05c719b2aa\"","Subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata\n\tclass","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18181,"web_url":"https://patchwork.libcamera.org/comment/18181/","msgid":"","date":"2021-07-14T23:24:31","subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata\n\tclass","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Wed, Jul 14, 2021 at 07:40:51PM +0100, Naushir Patuck wrote:\n> On Wed, 14 Jul 2021 at 18:43, Laurent Pinchart wrote:\n> > On Wed, Jul 14, 2021 at 06:38:42PM +0100, Naushir Patuck wrote:\n> > > On Wed, 14 Jul 2021 at 18:07, Laurent Pinchart wrote:\n> > > > On Wed, Jul 14, 2021 at 02:57:11PM +0200, Jean-Michel Hautbois wrote:\n> > > > > On 14/07/2021 14:48, Laurent Pinchart wrote:\n> > > > > > On Mon, Jul 12, 2021 at 03:16:29PM +0200, Jean-Michel Hautbois wrote:\n> > > > > >> The Metadata class comes from RPi from which a bit has been removed\n> > > > > >> because we don't need it for now.\n> > > > > >> All functions are inlined in metadata.h because of the template usage.\n> > > > > >>\n> > > > > >> Signed-off-by: Jean-Michel Hautbois < jeanmichel.hautbois@ideasonboard.com>\n> > > > > >> ---\n> > > > > >> src/ipa/ipu3/ipu3.cpp | 1 +\n> > > > > >> src/ipa/libipa/meson.build | 6 ++-\n> > > > > >> src/ipa/libipa/metadata.cpp | 101 ++++++++++++++++++++++++++++++++++++\n> > > > > >> src/ipa/libipa/metadata.h | 90 ++++++++++++++++++++++++++++++++\n> > > > > >> 4 files changed, 196 insertions(+), 2 deletions(-)\n> > > > > >> create mode 100644 src/ipa/libipa/metadata.cpp\n> > > > > >> create mode 100644 src/ipa/libipa/metadata.h\n> > > > > >>\n> > > > > >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > > > > >> index 71698d36..091856f5 100644\n> > > > > >> --- a/src/ipa/ipu3/ipu3.cpp\n> > > > > >> +++ b/src/ipa/ipu3/ipu3.cpp\n> > > > > >> @@ -25,6 +25,7 @@\n> > > > > >> #include \"ipu3_agc.h\"\n> > > > > >> #include \"ipu3_awb.h\"\n> > > > > >> #include \"libipa/camera_sensor_helper.h\"\n> > > > > >> +#include \"libipa/metadata.h\"\n> > > > > >>\n> > > > > >> static constexpr uint32_t kMaxCellWidthPerSet = 160;\n> > > > > >> static constexpr uint32_t kMaxCellHeightPerSet = 56;\n> > > > > >> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> > > > > >> index 3fda7c00..cc4e1cc9 100644\n> > > > > >> --- a/src/ipa/libipa/meson.build\n> > > > > >> +++ b/src/ipa/libipa/meson.build\n> > > > > >> @@ -3,13 +3,15 @@\n> > > > > >> libipa_headers = files([\n> > > > > >> 'algorithm.h',\n> > > > > >> 'camera_sensor_helper.h',\n> > > > > >> - 'histogram.h'\n> > > > > >> + 'histogram.h',\n> > > > > >> + 'metadata.h'\n> > > > > >> ])\n> > > > > >>\n> > > > > >> libipa_sources = files([\n> > > > > >> 'algorithm.cpp',\n> > > > > >> 'camera_sensor_helper.cpp',\n> > > > > >> - 'histogram.cpp'\n> > > > > >> + 'histogram.cpp',\n> > > > > >> + 'metadata.cpp'\n> > > > > >> ])\n> > > > > >>\n> > > > > >> libipa_includes = include_directories('..')\n> > > > > >> diff --git a/src/ipa/libipa/metadata.cpp b/src/ipa/libipa/metadata.cpp\n> > > > > >> new file mode 100644\n> > > > > >> index 00000000..b6aef897\n> > > > > >> --- /dev/null\n> > > > > >> +++ b/src/ipa/libipa/metadata.cpp\n> > > > > >> @@ -0,0 +1,101 @@\n> > > > > >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > >> +/*\n> > > > > >> + * Based on the implementation from the Raspberry Pi IPA,\n> > > > > >> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.\n> > > > > >> + * Copyright (C) 2021, Ideas On Board\n> > > > > >> + *\n> > > > > >> + * metadata.cpp - libipa metadata class\n> > > > > >> + */\n> > > > > >> +\n> > > > > >> +#include \"metadata.h\"\n> > > > > >> +\n> > > > > >> +/**\n> > > > > >> + * \\file metadata.h\n> > > > > >> + * \\brief A metadata class to share objects\n> > > > > >> + */\n> > > > > >> +\n> > > > > >> +namespace libcamera {\n> > > > > >> +\n> > > > > >> +namespace ipa {\n> > > > > >> +\n> > > > > >> +/**\n> > > > > >> + * \\class Metadata\n> > > > > >> + * \\brief A simple class for carrying arbitrary metadata, for example\n> > > > > >> + * about an image. It is used to exchange data between algorithms.\n> > > > > >> + */\n> > > > > >> +\n> > > > > >> +/**\n> > > > > >> + * \\fn Metadata::Metadata(Metadata const &other)\n> > > > > >> + * \\param[in] other A Metadata object\n> > > > > >> + *\n> > > > > >> + * Stores the data from one Metadata to another one\n> > > > > >> + */\n> > > > > >> +\n> > > > > >> +/**\n> > > > > >> + * \\fn Metadata::set(std::string const &tag, T const &value)\n> > > > > >> + * \\param[in] tag A string used as the key in a map\n> > > > > >> + * \\param[in] value The value to set into the map\n> > > > > >> + *\n> > > > > >> + * Sets the value in the map to the tag key. The mutex is\n> > > > > >> + * taken for the duration of the block.\n> > > > > >> + */\n> > > > > >> +\n> > > > > >> +/**\n> > > > > >> + * \\fn Metadata::get(std::string const &tag, T &value)\n> > > > > >> + * \\param[in] tag A string used as the key in a map\n> > > > > >> + * \\param[in] value The value to set into the map\n> > > > > >> + *\n> > > > > >> + * Gets the value in the map of the tag key. The mutex is\n> > > > > >> + * taken for the duration of the block.\n> > > > > >> + *\n> > > > > >> + * \\return 0 if value is found, -1 if not existent\n> > > > > >> + */\n> > > > > >> +\n> > > > > >> +/**\n> > > > > >> + * \\fn Metadata::clear()\n> > > > > >> + * Clear the Metadata map. The mutex is taken for the duration of\n> > > > > >> + * the block.\n> > > > > >> + */\n> > > > > >> +\n> > > > > >> +/**\n> > > > > >> + * \\fn Metadata::merge(Metadata &other)\n> > > > > >> + * \\param[in] other A metadata to merge with\n> > > > > >> + * Merge two Metadata maps. The mutex is taken for the duration of\n> > > > > >> + * the block.\n> > > > > >> + */\n> > > > > >> +\n> > > > > >> +/**\n> > > > > >> + * \\fn Metadata::getLocked(std::string const &tag)\n> > > > > >> + * \\param[in] tag A string used as the key in a map\n> > > > > >> + *\n> > > > > >> + * Get the value of the tag key in the map.\n> > > > > >> + * This allows in-place access to the Metadata contents,\n> > > > > >> + * for which you should be holding the lock.\n> > > > > >> + */\n> > > > > >> +\n> > > > > >> +/**\n> > > > > >> + * \\fn Metadata::setLocked(std::string const &tag, T const &value)\n> > > > > >> + * \\param[in] tag A string used as the key in a map\n> > > > > >> + * \\param[in] value The value to set into the map\n> > > > > >> + *\n> > > > > >> + * Set the value to the tag key in the map.\n> > > > > >> + * This allows in-place access to the Metadata contents,\n> > > > > >> + * for which you should be holding the lock.\n> > > > > >> + */\n> > > > > >> +\n> > > > > >> +/**\n> > > > > >> + * \\fn Metadata::lock()\n> > > > > >> + * Lock the mutex with the standard classes.\n> > > > > >> + * e.g. std::lock_guard lock(metadata)\n> > > > > >> + */\n> > > > > >> +\n> > > > > >> +/**\n> > > > > >> + * \\fn Metadata::unlock()\n> > > > > >> + * Unlock the mutex with the standard classes.\n> > > > > >> + * e.g. std::lock_guard lock(metadata)\n> > > > > >> + */\n> > > > > >> +\n> > > > > >> +} /* namespace ipa */\n> > > > > >> +\n> > > > > >> +} /* namespace libcamera */\n> > > > > >> +\n> > > > > >> diff --git a/src/ipa/libipa/metadata.h b/src/ipa/libipa/metadata.h\n> > > > > >> new file mode 100644\n> > > > > >> index 00000000..9801bece\n> > > > > >> --- /dev/null\n> > > > > >> +++ b/src/ipa/libipa/metadata.h\n> > > > > >> @@ -0,0 +1,90 @@\n> > > > > >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > >> +/*\n> > > > > >> + * Based on the implementation from the Raspberry Pi IPA,\n> > > > > >> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.\n> > > > > >> + * Copyright (C) 2021, Ideas On Board\n> > > > > >> + *\n> > > > > >> + * metadata.h - libipa metadata class\n> > > > > >> + */\n> > > > > >> +#ifndef __LIBCAMERA_IPA_LIBIPA_METADATA_H__\n> > > > > >> +#define __LIBCAMERA_IPA_LIBIPA_METADATA_H__\n> > > > > >> +\n> > > > > >> +#include \n> > > > > >> +#include \n> > > > > >> +#include \n> > > > > >> +#include \n> > > > > >> +#include \n> > > > > >> +\n> > > > > >> +namespace libcamera {\n> > > > > >> +\n> > > > > >> +namespace ipa {\n> > > > > >> +\n> > > > > >> +class Metadata\n> > > > > >> +{\n> > > > > >> +public:\n> > > > > >> + Metadata() = default;\n> > > > > >> +\n> > > > > >> + Metadata(Metadata const &other)\n> > > > > >> + {\n> > > > > >> + std::scoped_lock other_lock(other.mutex_);\n> > > > > >> + data_ = other.data_;\n> > > > > >> + }\n> > > > > >> +\n> > > > > >> + template\n> > > > > >> + void set(std::string const &tag, T const &value)\n> > > > > >> + {\n> > > > > >> + std::scoped_lock lock(mutex_);\n> > > > > >> + data_[tag] = value;\n> > > > > >> + }\n> > > > > >> +\n> > > > > >> + template\n> > > > > >> + int get(std::string const &tag, T &value) const\n> > > > > >> + {\n> > > > > >> + std::scoped_lock lock(mutex_);\n> > > > > >\n> > > > > > You can call getLocked() here.\n> > > > >\n> > > > > Indeed :-)\n> > > > >\n> > > > > >> + auto it = data_.find(tag);\n> > > > > >> + if (it == data_.end())\n> > > > > >> + return -1;\n> > > > > >> + value = std::any_cast(it->second);\n> > > > > >> + return 0;\n> > > > > >> + }\n> > > > > >> +\n> > > > > >> + void clear()\n> > > > > >> + {\n> > > > > >> + std::scoped_lock lock(mutex_);\n> > > > > >> + data_.clear();\n> > > > > >> + }\n> > > > > >> +\n> > > > > >> + void merge(Metadata &other)\n> > > > > >> + {\n> > > > > >> + std::scoped_lock lock(mutex_, other.mutex_);\n> > > > > >> + data_.merge(other.data_);\n> > > > > >> + }\n> > > > > >> +\n> > > > > >> + template\n> > > > > >> + T *getLocked(std::string const &tag)\n> > > > > >> + {\n> > > > > >> + auto it = data_.find(tag);\n> > > > > >> + if (it == data_.end())\n> > > > > >> + return nullptr;\n> > > > > >> + return std::any_cast(&it->second);\n> > > > > >\n> > > > > > This is a bit dangerous, if T doesn't match the type stored for the tag.\n> > > > > > It would of course be a bug in the caller, but if such code appears in\n> > > > > > paths that are not frequently taken, it could cause issues that will\n> > > > > > result in a crash at runtime later on.\n> > > > > >\n> > > > > > Could we use a mechanism similar to Control and ControlList to ensure\n> > > > > > the right case ?\n> > > > > >\n> > > > > > template\n> > > > > > struct Tag {\n> > > > > > std::string tag_;\n> > > > > > };\n> > > > > >\n> > > > > > /* Static list of all tags. */\n> > > > > > static constexpr Tag tagFoo{ \"foo\" };\n> > > > > >\n> > > > > > class Metadata\n> > > > > > {\n> > > > > > ...\n> > > > > > template\n> > > > > > T *getLock(const Tag &tag)\n> > > > > > {\n> > > > > > ...\n> > > > > > return std::any_cast(&it->second);\n> > > > > > }\n> > > > > > ...\n> > > > > > };\n> > > > > > ...\n> > > > > >\n> > > > > > Foo *f = metadata.get(tagFoo);\n> > > > > >\n> > > > > > Furthermore, have you considered using integers instead of strings for\n> > > > > > tags ? What are the pros and cons ?\n> > > > >\n> > > > > I think it may be a good idea. Pros I can see:\n> > > > > - easy to document the tags\n> > > > > - readability (we can always refer to the enum to know how a particular\n> > > > > object is mapped).\n> > > > > - fastest to find the tag\n> > > > > Cons, not much:\n> > > > > - if RPi wants to switch to the libipa Metadata class they will need to\n> > > > > rewrite a not that small piece of code.\n> > >\n> > > What is being described here seems suspiciously similar to a ControlList\n> > :-)\n> >\n> > A bit indeed, but with an std::any instead of a small set of supported\n> > types, and no support for ControlInfo. It's \"just\" a way to ensure we\n> > won't get the type T wrong in a call.\n> \n> I was thinking along the lines of using ControlValues of type Span like\n> we do in the IPA to set ISP controls. It does not exactly have the convenience\n> of std::any usage, but does allow arbitrary structs to be placed into the\n> ControlList.\n\nThat's right, but we then lose type-safety, with a possible buffer\noverflow or just misinterpretation of data, and that's even harder to\ndebug than an uncaught exception being thrown in uncommon cases.\n\n> > > Perhaps for your usage, that is more appropriate over our Metadata object?\n> >\n> > Do you think the usage in the Raspberry Pi IPA differs significantly ?\n> \n> For RPi, we like the convenience and ease of use for the existing method.\n> You can just call set/get without any ancillary setup, which is a big reason we\n> chose strings over enums for key types. This code predated our IPA, so\n> we used to take care of std::any type mismatches using exceptions, which were\n> removed when we ported our code across. Without this, you do need to be\n> somewhat careful not to trip over.\n\nType mismatch are essentially programming errors. Dealing with them by\ncatching exceptions can help recovering gracefully, but isn't it best to\ncatch those errors at compile time ?\n\n> Other than that, usage wise, I expect things to be similar to what JM intends, but will\n> have to wait and see. If this is indeed the case, I expect all key/value pairs to be very\n> specific to each IPA and doubt much (if any) could be shared across various vendors.\n\nThat's my expectations too. I'm thinking some keys may become shared\nacross vendors, there's some data that may be common enough to make this\npossible, but the devil is in the details and I have a feeling that the\ninteroperability this could bring (by using some common pieces in\ndifferent IPA modules) is just a dream. Let's see how it turns out, but\nit's probably more pain than gain.\n\nI'd like to experiment with the template Tag class (name to be\nbikeshedded) to see how it turns out. If we go in that direction, string\nvs. integer becomes a moot point as the tag value will be internal. And\nif it turns out it's not a good idea, then we'll do something else :-)\n\n> Because of this, and the likelihood that these structures will only be used internally to\n> the IPA, do you see auto-generated documentation to be required? Of course the structs\n> and fields must be documented in the source files :-)\n\nNo, I wouldn't generate documentation out of this. We should generate\ndocumentation for shared code in libipa, but not for module-specific\ncode. Having internal documentation in the form of comments is of course\ngood, to help others understand the code, and increase maintainability.\n\n> > > > We can also help ;-)\n> > > >\n> > > > > - having integer would make the code less dynamic, as we would certainly\n> > > > > be tempted to create a static map (which can also be seen as a pro :-)).\n> > > >\n> > > > That's my main question. Do you foresee a need to have some sort of\n> > > > namespace in tags ? Will the data stored here be specific to each IPA\n> > > > module, or would there be some level of standardization ? If we want to\n> > > > create an IPA module from a set of algorithms, who will be responsible\n> > > > for accessing the storage, would that be code specific to that IPA\n> > > > module, and/or code from libipa ? There's lots of design questions, I\n> > > > have little visibility on what you're planning.\n> > > >\n> > > > The idea of using a template Tag structure as described above is\n> > > > orthogonal to the decision of using strings or integers as identifiers.\n> > > > Tags would however need to be defined in headers, which I think is an\n> > > > upside, as we can enforce documentation in that case. I don't know if it\n> > > > would be feasible (or even a good idea) to centralize all the tag\n> > > > definitions though, they could be spread across different headers (some\n> > > > being device-specific, and some being maybe shared accross different IPA\n> > > > modules).\n> > > >\n> > > > > >> + }\n> > > > > >> +\n> > > > > >> + template\n> > > > > >> + void setLocked(std::string const &tag, T const &value)\n> > > > > >> + {\n> > > > > >> + data_[tag] = value;\n> > > > > >> + }\n> > > > > >> +\n> > > > > >> + void lock() { mutex_.lock(); }\n> > > > > >> + void unlock() { mutex_.unlock(); }\n> > > > > >> +\n> > > > > >> +private:\n> > > > > >> + mutable std::mutex mutex_;\n> > > > > >> + std::map data_;\n> > > > > >> +};\n> > > > > >> +\n> > > > > >> +} /* namespace ipa */\n> > > > > >> +\n> > > > > >> +} /* namespace libcamera */\n> > > > > >> +\n> > > > > >> +#endif /* __LIBCAMERA_IPA_LIBIPA_METADATA_H__ */","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5574CC3225\n\tfor ;\n\tWed, 14 Jul 2021 23:24:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3A20768525;\n\tThu, 15 Jul 2021 01:24:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 106E96027D\n\tfor ;\n\tThu, 15 Jul 2021 01:24:33 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 611F3A24;\n\tThu, 15 Jul 2021 01:24:32 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"J9367m0g\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626305072;\n\tbh=OkAMfCNdxghiDsMCoxk5KsL8FZTYtKmSr2xpoMXnSPU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=J9367m0gzn6ZxysGHi91Q71Ahcuq4nsz6GSzWfHmHzCIYiqTmJvqOQ4nZaj2EQ1tC\n\tRqAvyu6mgekMM1FIEhE4fOcOZ0uFBeqvGohuB5x803xpsDeTih5lQF/zhqABPRWx/l\n\tVNgQDZY8fs9knZ08/LfdZFs3eq8v1bTg/DjqRaOE=","Date":"Thu, 15 Jul 2021 02:24:31 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210712131630.73914-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210712131630.73914-2-jeanmichel.hautbois@ideasonboard.com>\n\t\n\t<62a0ec5d-0c19-8971-c46f-612c767a6b52@ideasonboard.com>\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata\n\tclass","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18215,"web_url":"https://patchwork.libcamera.org/comment/18215/","msgid":"","date":"2021-07-18T11:40:34","subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata\n\tclass","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Laurent, Naush\n\nJust to add my own two cents worth...\n\nFirstly, I like strings as tags. This will be no surprise as we also\nuse them as (for example) metering mode names!! But I think it relates\nat least partly to how easy we want to make it for folks to experiment\nand try their own things - which we've always wanted to encourage.\n\nThis is true in the metadata too. Our documentation is quite clear how\nwe expect people to use certain specific tags (\"algo.status\") if they\nwant to play nicely with other RPi algorithms. But they can implement\ntheir own algorithms entirely.\n\nIn particular, they can add their own private metadata to frames\n(\"foo.algo.private\" for example). I think this will become more\nimportant on future platforms that run less synchronously, where\nProcess won't be able to assume that the last time Prepare ran, it was\nfor the same frame.\n\nWe also have a rule that the parameters that have been set in the\nalgorithm must appear in the status object, so you can sit back and\nwatch frames until the parameters you asked for have taken effect.\nThis whole area is of course very difficult - as was apparent in the\nKhronos group discussion the other week.\n\nDavid\n\nOn Thu, 15 Jul 2021 at 00:24, Laurent Pinchart\n wrote:\n>\n> Hi Naush,\n>\n> On Wed, Jul 14, 2021 at 07:40:51PM +0100, Naushir Patuck wrote:\n> > On Wed, 14 Jul 2021 at 18:43, Laurent Pinchart wrote:\n> > > On Wed, Jul 14, 2021 at 06:38:42PM +0100, Naushir Patuck wrote:\n> > > > On Wed, 14 Jul 2021 at 18:07, Laurent Pinchart wrote:\n> > > > > On Wed, Jul 14, 2021 at 02:57:11PM +0200, Jean-Michel Hautbois wrote:\n> > > > > > On 14/07/2021 14:48, Laurent Pinchart wrote:\n> > > > > > > On Mon, Jul 12, 2021 at 03:16:29PM +0200, Jean-Michel Hautbois wrote:\n> > > > > > >> The Metadata class comes from RPi from which a bit has been removed\n> > > > > > >> because we don't need it for now.\n> > > > > > >> All functions are inlined in metadata.h because of the template usage.\n> > > > > > >>\n> > > > > > >> Signed-off-by: Jean-Michel Hautbois < jeanmichel.hautbois@ideasonboard.com>\n> > > > > > >> ---\n> > > > > > >> src/ipa/ipu3/ipu3.cpp | 1 +\n> > > > > > >> src/ipa/libipa/meson.build | 6 ++-\n> > > > > > >> src/ipa/libipa/metadata.cpp | 101 ++++++++++++++++++++++++++++++++++++\n> > > > > > >> src/ipa/libipa/metadata.h | 90 ++++++++++++++++++++++++++++++++\n> > > > > > >> 4 files changed, 196 insertions(+), 2 deletions(-)\n> > > > > > >> create mode 100644 src/ipa/libipa/metadata.cpp\n> > > > > > >> create mode 100644 src/ipa/libipa/metadata.h\n> > > > > > >>\n> > > > > > >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > > > > > >> index 71698d36..091856f5 100644\n> > > > > > >> --- a/src/ipa/ipu3/ipu3.cpp\n> > > > > > >> +++ b/src/ipa/ipu3/ipu3.cpp\n> > > > > > >> @@ -25,6 +25,7 @@\n> > > > > > >> #include \"ipu3_agc.h\"\n> > > > > > >> #include \"ipu3_awb.h\"\n> > > > > > >> #include \"libipa/camera_sensor_helper.h\"\n> > > > > > >> +#include \"libipa/metadata.h\"\n> > > > > > >>\n> > > > > > >> static constexpr uint32_t kMaxCellWidthPerSet = 160;\n> > > > > > >> static constexpr uint32_t kMaxCellHeightPerSet = 56;\n> > > > > > >> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> > > > > > >> index 3fda7c00..cc4e1cc9 100644\n> > > > > > >> --- a/src/ipa/libipa/meson.build\n> > > > > > >> +++ b/src/ipa/libipa/meson.build\n> > > > > > >> @@ -3,13 +3,15 @@\n> > > > > > >> libipa_headers = files([\n> > > > > > >> 'algorithm.h',\n> > > > > > >> 'camera_sensor_helper.h',\n> > > > > > >> - 'histogram.h'\n> > > > > > >> + 'histogram.h',\n> > > > > > >> + 'metadata.h'\n> > > > > > >> ])\n> > > > > > >>\n> > > > > > >> libipa_sources = files([\n> > > > > > >> 'algorithm.cpp',\n> > > > > > >> 'camera_sensor_helper.cpp',\n> > > > > > >> - 'histogram.cpp'\n> > > > > > >> + 'histogram.cpp',\n> > > > > > >> + 'metadata.cpp'\n> > > > > > >> ])\n> > > > > > >>\n> > > > > > >> libipa_includes = include_directories('..')\n> > > > > > >> diff --git a/src/ipa/libipa/metadata.cpp b/src/ipa/libipa/metadata.cpp\n> > > > > > >> new file mode 100644\n> > > > > > >> index 00000000..b6aef897\n> > > > > > >> --- /dev/null\n> > > > > > >> +++ b/src/ipa/libipa/metadata.cpp\n> > > > > > >> @@ -0,0 +1,101 @@\n> > > > > > >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > > >> +/*\n> > > > > > >> + * Based on the implementation from the Raspberry Pi IPA,\n> > > > > > >> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.\n> > > > > > >> + * Copyright (C) 2021, Ideas On Board\n> > > > > > >> + *\n> > > > > > >> + * metadata.cpp - libipa metadata class\n> > > > > > >> + */\n> > > > > > >> +\n> > > > > > >> +#include \"metadata.h\"\n> > > > > > >> +\n> > > > > > >> +/**\n> > > > > > >> + * \\file metadata.h\n> > > > > > >> + * \\brief A metadata class to share objects\n> > > > > > >> + */\n> > > > > > >> +\n> > > > > > >> +namespace libcamera {\n> > > > > > >> +\n> > > > > > >> +namespace ipa {\n> > > > > > >> +\n> > > > > > >> +/**\n> > > > > > >> + * \\class Metadata\n> > > > > > >> + * \\brief A simple class for carrying arbitrary metadata, for example\n> > > > > > >> + * about an image. It is used to exchange data between algorithms.\n> > > > > > >> + */\n> > > > > > >> +\n> > > > > > >> +/**\n> > > > > > >> + * \\fn Metadata::Metadata(Metadata const &other)\n> > > > > > >> + * \\param[in] other A Metadata object\n> > > > > > >> + *\n> > > > > > >> + * Stores the data from one Metadata to another one\n> > > > > > >> + */\n> > > > > > >> +\n> > > > > > >> +/**\n> > > > > > >> + * \\fn Metadata::set(std::string const &tag, T const &value)\n> > > > > > >> + * \\param[in] tag A string used as the key in a map\n> > > > > > >> + * \\param[in] value The value to set into the map\n> > > > > > >> + *\n> > > > > > >> + * Sets the value in the map to the tag key. The mutex is\n> > > > > > >> + * taken for the duration of the block.\n> > > > > > >> + */\n> > > > > > >> +\n> > > > > > >> +/**\n> > > > > > >> + * \\fn Metadata::get(std::string const &tag, T &value)\n> > > > > > >> + * \\param[in] tag A string used as the key in a map\n> > > > > > >> + * \\param[in] value The value to set into the map\n> > > > > > >> + *\n> > > > > > >> + * Gets the value in the map of the tag key. The mutex is\n> > > > > > >> + * taken for the duration of the block.\n> > > > > > >> + *\n> > > > > > >> + * \\return 0 if value is found, -1 if not existent\n> > > > > > >> + */\n> > > > > > >> +\n> > > > > > >> +/**\n> > > > > > >> + * \\fn Metadata::clear()\n> > > > > > >> + * Clear the Metadata map. The mutex is taken for the duration of\n> > > > > > >> + * the block.\n> > > > > > >> + */\n> > > > > > >> +\n> > > > > > >> +/**\n> > > > > > >> + * \\fn Metadata::merge(Metadata &other)\n> > > > > > >> + * \\param[in] other A metadata to merge with\n> > > > > > >> + * Merge two Metadata maps. The mutex is taken for the duration of\n> > > > > > >> + * the block.\n> > > > > > >> + */\n> > > > > > >> +\n> > > > > > >> +/**\n> > > > > > >> + * \\fn Metadata::getLocked(std::string const &tag)\n> > > > > > >> + * \\param[in] tag A string used as the key in a map\n> > > > > > >> + *\n> > > > > > >> + * Get the value of the tag key in the map.\n> > > > > > >> + * This allows in-place access to the Metadata contents,\n> > > > > > >> + * for which you should be holding the lock.\n> > > > > > >> + */\n> > > > > > >> +\n> > > > > > >> +/**\n> > > > > > >> + * \\fn Metadata::setLocked(std::string const &tag, T const &value)\n> > > > > > >> + * \\param[in] tag A string used as the key in a map\n> > > > > > >> + * \\param[in] value The value to set into the map\n> > > > > > >> + *\n> > > > > > >> + * Set the value to the tag key in the map.\n> > > > > > >> + * This allows in-place access to the Metadata contents,\n> > > > > > >> + * for which you should be holding the lock.\n> > > > > > >> + */\n> > > > > > >> +\n> > > > > > >> +/**\n> > > > > > >> + * \\fn Metadata::lock()\n> > > > > > >> + * Lock the mutex with the standard classes.\n> > > > > > >> + * e.g. std::lock_guard lock(metadata)\n> > > > > > >> + */\n> > > > > > >> +\n> > > > > > >> +/**\n> > > > > > >> + * \\fn Metadata::unlock()\n> > > > > > >> + * Unlock the mutex with the standard classes.\n> > > > > > >> + * e.g. std::lock_guard lock(metadata)\n> > > > > > >> + */\n> > > > > > >> +\n> > > > > > >> +} /* namespace ipa */\n> > > > > > >> +\n> > > > > > >> +} /* namespace libcamera */\n> > > > > > >> +\n> > > > > > >> diff --git a/src/ipa/libipa/metadata.h b/src/ipa/libipa/metadata.h\n> > > > > > >> new file mode 100644\n> > > > > > >> index 00000000..9801bece\n> > > > > > >> --- /dev/null\n> > > > > > >> +++ b/src/ipa/libipa/metadata.h\n> > > > > > >> @@ -0,0 +1,90 @@\n> > > > > > >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > > >> +/*\n> > > > > > >> + * Based on the implementation from the Raspberry Pi IPA,\n> > > > > > >> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.\n> > > > > > >> + * Copyright (C) 2021, Ideas On Board\n> > > > > > >> + *\n> > > > > > >> + * metadata.h - libipa metadata class\n> > > > > > >> + */\n> > > > > > >> +#ifndef __LIBCAMERA_IPA_LIBIPA_METADATA_H__\n> > > > > > >> +#define __LIBCAMERA_IPA_LIBIPA_METADATA_H__\n> > > > > > >> +\n> > > > > > >> +#include \n> > > > > > >> +#include \n> > > > > > >> +#include \n> > > > > > >> +#include \n> > > > > > >> +#include \n> > > > > > >> +\n> > > > > > >> +namespace libcamera {\n> > > > > > >> +\n> > > > > > >> +namespace ipa {\n> > > > > > >> +\n> > > > > > >> +class Metadata\n> > > > > > >> +{\n> > > > > > >> +public:\n> > > > > > >> + Metadata() = default;\n> > > > > > >> +\n> > > > > > >> + Metadata(Metadata const &other)\n> > > > > > >> + {\n> > > > > > >> + std::scoped_lock other_lock(other.mutex_);\n> > > > > > >> + data_ = other.data_;\n> > > > > > >> + }\n> > > > > > >> +\n> > > > > > >> + template\n> > > > > > >> + void set(std::string const &tag, T const &value)\n> > > > > > >> + {\n> > > > > > >> + std::scoped_lock lock(mutex_);\n> > > > > > >> + data_[tag] = value;\n> > > > > > >> + }\n> > > > > > >> +\n> > > > > > >> + template\n> > > > > > >> + int get(std::string const &tag, T &value) const\n> > > > > > >> + {\n> > > > > > >> + std::scoped_lock lock(mutex_);\n> > > > > > >\n> > > > > > > You can call getLocked() here.\n> > > > > >\n> > > > > > Indeed :-)\n> > > > > >\n> > > > > > >> + auto it = data_.find(tag);\n> > > > > > >> + if (it == data_.end())\n> > > > > > >> + return -1;\n> > > > > > >> + value = std::any_cast(it->second);\n> > > > > > >> + return 0;\n> > > > > > >> + }\n> > > > > > >> +\n> > > > > > >> + void clear()\n> > > > > > >> + {\n> > > > > > >> + std::scoped_lock lock(mutex_);\n> > > > > > >> + data_.clear();\n> > > > > > >> + }\n> > > > > > >> +\n> > > > > > >> + void merge(Metadata &other)\n> > > > > > >> + {\n> > > > > > >> + std::scoped_lock lock(mutex_, other.mutex_);\n> > > > > > >> + data_.merge(other.data_);\n> > > > > > >> + }\n> > > > > > >> +\n> > > > > > >> + template\n> > > > > > >> + T *getLocked(std::string const &tag)\n> > > > > > >> + {\n> > > > > > >> + auto it = data_.find(tag);\n> > > > > > >> + if (it == data_.end())\n> > > > > > >> + return nullptr;\n> > > > > > >> + return std::any_cast(&it->second);\n> > > > > > >\n> > > > > > > This is a bit dangerous, if T doesn't match the type stored for the tag.\n> > > > > > > It would of course be a bug in the caller, but if such code appears in\n> > > > > > > paths that are not frequently taken, it could cause issues that will\n> > > > > > > result in a crash at runtime later on.\n> > > > > > >\n> > > > > > > Could we use a mechanism similar to Control and ControlList to ensure\n> > > > > > > the right case ?\n> > > > > > >\n> > > > > > > template\n> > > > > > > struct Tag {\n> > > > > > > std::string tag_;\n> > > > > > > };\n> > > > > > >\n> > > > > > > /* Static list of all tags. */\n> > > > > > > static constexpr Tag tagFoo{ \"foo\" };\n> > > > > > >\n> > > > > > > class Metadata\n> > > > > > > {\n> > > > > > > ...\n> > > > > > > template\n> > > > > > > T *getLock(const Tag &tag)\n> > > > > > > {\n> > > > > > > ...\n> > > > > > > return std::any_cast(&it->second);\n> > > > > > > }\n> > > > > > > ...\n> > > > > > > };\n> > > > > > > ...\n> > > > > > >\n> > > > > > > Foo *f = metadata.get(tagFoo);\n> > > > > > >\n> > > > > > > Furthermore, have you considered using integers instead of strings for\n> > > > > > > tags ? What are the pros and cons ?\n> > > > > >\n> > > > > > I think it may be a good idea. Pros I can see:\n> > > > > > - easy to document the tags\n> > > > > > - readability (we can always refer to the enum to know how a particular\n> > > > > > object is mapped).\n> > > > > > - fastest to find the tag\n> > > > > > Cons, not much:\n> > > > > > - if RPi wants to switch to the libipa Metadata class they will need to\n> > > > > > rewrite a not that small piece of code.\n> > > >\n> > > > What is being described here seems suspiciously similar to a ControlList\n> > > :-)\n> > >\n> > > A bit indeed, but with an std::any instead of a small set of supported\n> > > types, and no support for ControlInfo. It's \"just\" a way to ensure we\n> > > won't get the type T wrong in a call.\n> >\n> > I was thinking along the lines of using ControlValues of type Span like\n> > we do in the IPA to set ISP controls. It does not exactly have the convenience\n> > of std::any usage, but does allow arbitrary structs to be placed into the\n> > ControlList.\n>\n> That's right, but we then lose type-safety, with a possible buffer\n> overflow or just misinterpretation of data, and that's even harder to\n> debug than an uncaught exception being thrown in uncommon cases.\n>\n> > > > Perhaps for your usage, that is more appropriate over our Metadata object?\n> > >\n> > > Do you think the usage in the Raspberry Pi IPA differs significantly ?\n> >\n> > For RPi, we like the convenience and ease of use for the existing method.\n> > You can just call set/get without any ancillary setup, which is a big reason we\n> > chose strings over enums for key types. This code predated our IPA, so\n> > we used to take care of std::any type mismatches using exceptions, which were\n> > removed when we ported our code across. Without this, you do need to be\n> > somewhat careful not to trip over.\n>\n> Type mismatch are essentially programming errors. Dealing with them by\n> catching exceptions can help recovering gracefully, but isn't it best to\n> catch those errors at compile time ?\n>\n> > Other than that, usage wise, I expect things to be similar to what JM intends, but will\n> > have to wait and see. If this is indeed the case, I expect all key/value pairs to be very\n> > specific to each IPA and doubt much (if any) could be shared across various vendors.\n>\n> That's my expectations too. I'm thinking some keys may become shared\n> across vendors, there's some data that may be common enough to make this\n> possible, but the devil is in the details and I have a feeling that the\n> interoperability this could bring (by using some common pieces in\n> different IPA modules) is just a dream. Let's see how it turns out, but\n> it's probably more pain than gain.\n>\n> I'd like to experiment with the template Tag class (name to be\n> bikeshedded) to see how it turns out. If we go in that direction, string\n> vs. integer becomes a moot point as the tag value will be internal. And\n> if it turns out it's not a good idea, then we'll do something else :-)\n>\n> > Because of this, and the likelihood that these structures will only be used internally to\n> > the IPA, do you see auto-generated documentation to be required? Of course the structs\n> > and fields must be documented in the source files :-)\n>\n> No, I wouldn't generate documentation out of this. We should generate\n> documentation for shared code in libipa, but not for module-specific\n> code. Having internal documentation in the form of comments is of course\n> good, to help others understand the code, and increase maintainability.\n>\n> > > > > We can also help ;-)\n> > > > >\n> > > > > > - having integer would make the code less dynamic, as we would certainly\n> > > > > > be tempted to create a static map (which can also be seen as a pro :-)).\n> > > > >\n> > > > > That's my main question. Do you foresee a need to have some sort of\n> > > > > namespace in tags ? Will the data stored here be specific to each IPA\n> > > > > module, or would there be some level of standardization ? If we want to\n> > > > > create an IPA module from a set of algorithms, who will be responsible\n> > > > > for accessing the storage, would that be code specific to that IPA\n> > > > > module, and/or code from libipa ? There's lots of design questions, I\n> > > > > have little visibility on what you're planning.\n> > > > >\n> > > > > The idea of using a template Tag structure as described above is\n> > > > > orthogonal to the decision of using strings or integers as identifiers.\n> > > > > Tags would however need to be defined in headers, which I think is an\n> > > > > upside, as we can enforce documentation in that case. I don't know if it\n> > > > > would be feasible (or even a good idea) to centralize all the tag\n> > > > > definitions though, they could be spread across different headers (some\n> > > > > being device-specific, and some being maybe shared accross different IPA\n> > > > > modules).\n> > > > >\n> > > > > > >> + }\n> > > > > > >> +\n> > > > > > >> + template\n> > > > > > >> + void setLocked(std::string const &tag, T const &value)\n> > > > > > >> + {\n> > > > > > >> + data_[tag] = value;\n> > > > > > >> + }\n> > > > > > >> +\n> > > > > > >> + void lock() { mutex_.lock(); }\n> > > > > > >> + void unlock() { mutex_.unlock(); }\n> > > > > > >> +\n> > > > > > >> +private:\n> > > > > > >> + mutable std::mutex mutex_;\n> > > > > > >> + std::map data_;\n> > > > > > >> +};\n> > > > > > >> +\n> > > > > > >> +} /* namespace ipa */\n> > > > > > >> +\n> > > > > > >> +} /* namespace libcamera */\n> > > > > > >> +\n> > > > > > >> +#endif /* __LIBCAMERA_IPA_LIBIPA_METADATA_H__ */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D32D6C0109\n\tfor ;\n\tSun, 18 Jul 2021 11:40:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3A5396853A;\n\tSun, 18 Jul 2021 13:40:51 +0200 (CEST)","from mail-il1-x12b.google.com (mail-il1-x12b.google.com\n\t[IPv6:2607:f8b0:4864:20::12b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 41D0660280\n\tfor ;\n\tSun, 18 Jul 2021 13:40:48 +0200 (CEST)","by mail-il1-x12b.google.com with SMTP id b14so13114099ilf.7\n\tfor ;\n\tSun, 18 Jul 2021 04:40:48 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"r78+/UhK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=rLJ5oSmGjhZyyWJIFLSKFNVyX+EBDqj36AWrcPpv4YM=;\n\tb=r78+/UhKnHd7ZMthOJkBFIgQyDRDZ0CwheNxIEYFGnPdbk9vP/Aa/a7I2Q3bn5jkSL\n\tStzcyRlTvIaf177iZhvQ/vmL7UGvkClxqlTzIq/kJJGw7krCIwo2sJNVd3brRWhPSYJ4\n\tTapa6zZgNPvzPJObjYYZBOuHE5KTgwQHhaz5iL6CSLL6kfxyHItUkj4a3N3+nrkj21X3\n\tL8Nw0K5Klg7+XXY2yFZH1/H/dDpZsoHLiyGlIrEdlBoIPTTu6sdst8YcX1p9NF6OHA7G\n\tqLqfvfaow0h1SrKMWpSfqjWQ7xxmDzS84QY+Egx/eyGNEzOddemu8+dYPLM6wLYr7+UW\n\tKejQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=rLJ5oSmGjhZyyWJIFLSKFNVyX+EBDqj36AWrcPpv4YM=;\n\tb=AfKvzSaFo2LFYlnJxp9XuXDpKVtNBeoeUExiumDjMs9tp2YNmwYZCxgg/jOarluQfh\n\tAkJj1b4s6b0wxBVJ8NfywmGkIrVJkEQmsja5krWaqLrHC6BJjSZ6FWHAxYrsT6elBGwY\n\ttdrMFT0eEi1h0nA851VDYHHVDWkq9rhScmvhtTdMshJNr8nHt/ljx5i6ipwoslKhAdhv\n\tm0VzqOk4vcXH+PXjviCjf70fR13MpjHr13SRswVzrtIMH/AnCZB3kYPmYQyEflXuTRU5\n\telgIBx5XkG4A6TEgxUHHiqdYdYdM3fb77lUBagl0kGZjCnpvZW5Tiij4vTOY8DD0RZL0\n\tuQAA==","X-Gm-Message-State":"AOAM530G1nJeR5kPYJHBvxAYWE+lXu9EJqbST8moksU7v1TyasV0qLZC\n\tpYVu+v9O6qURguN29VJVqtq9u4YiRGZ7NXYd53MOfg==","X-Google-Smtp-Source":"ABdhPJzLnwnNVLNvyNeLqXHKWiAlDtC4aG+obANgCaDEvrk4XgvKhxI25F/Y38GKONGVIaN5fPGcNmZMIOszg3R7jMs=","X-Received":"by 2002:a05:6e02:12e1:: with SMTP id\n\tl1mr12949879iln.0.1626608446855; \n\tSun, 18 Jul 2021 04:40:46 -0700 (PDT)","MIME-Version":"1.0","References":"<20210712131630.73914-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210712131630.73914-2-jeanmichel.hautbois@ideasonboard.com>\n\t\n\t<62a0ec5d-0c19-8971-c46f-612c767a6b52@ideasonboard.com>\n\t\n\t\n\t\n\t\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Sun, 18 Jul 2021 12:40:34 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: libipa: Introduce Metadata\n\tclass","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]