[{"id":31604,"web_url":"https://patchwork.libcamera.org/comment/31604/","msgid":"<20241007220544.GD30699@pendragon.ideasonboard.com>","date":"2024-10-07T22:05:44","subject":"Re: [PATCH v2 2/7] libcamera: Add a DebugMetadata helper","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Oct 07, 2024 at 11:54:06AM +0200, Stefan Klug wrote:\n> Debug metadata often occurs in places where the metadata control list is\n> not available e.g. in queueRequest() or processStatsBuffer() or even in\n> a class far away from the metadata handling code. It is therefore\n> difficult to add debug metadata without adding lots of boilerplate\n> code. This can be mitigated by recording the metadata and forwarding it\n> to the metadata control list when it becomes available. To solve the\n> issue of code that is far away from the metadata context, add a chaining\n> mechanism to allow loose coupling at runtime.\n> \n> Signed-off-by: Stefan Klug \n> \n> ---\n> Changes in v2:\n> - Replace assignments of ControlList by a moveEntries function\n> - Replace assignUpstream by setParent\n> - Improve documentation\n> ---\n> include/libcamera/internal/debug_controls.h | 46 +++++++\n> include/libcamera/internal/meson.build | 1 +\n> src/libcamera/control_ids_core.yaml | 5 +\n> src/libcamera/control_ids_debug.yaml | 3 +-\n> src/libcamera/debug_controls.cpp | 145 ++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 6 files changed, 199 insertions(+), 2 deletions(-)\n> create mode 100644 include/libcamera/internal/debug_controls.h\n> create mode 100644 src/libcamera/debug_controls.cpp\n> \n> diff --git a/include/libcamera/internal/debug_controls.h b/include/libcamera/internal/debug_controls.h\n> new file mode 100644\n> index 000000000000..00457d60b1a2\n> --- /dev/null\n> +++ b/include/libcamera/internal/debug_controls.h\n> @@ -0,0 +1,46 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Google Inc.\n> + *\n> + * Debug metadata helpers\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class DebugMetadata\n> +{\n> +public:\n> +\tDebugMetadata() = default;\n> +\n> +\tvoid checkForEnable(const ControlList &controls);\n> +\tvoid enable(bool enable = true);\n> +\tvoid setParent(DebugMetadata *parent);\n> +\tvoid moveEntries(ControlList &list);\n> +\n> +\ttemplate\n> +\tvoid set(const Control &ctrl, const V &value)\n> +\t{\n> +\t\tif (parent_) {\n> +\t\t\tparent_->set(ctrl, value);\n\nI think you can drop the explicit types here. Same below.\n\n> +\t\t\treturn;\n> +\t\t}\n> +\n> +\t\tif (!enabled_)\n> +\t\t\treturn;\n> +\n> +\t\tcache_.set(ctrl, value);\n> +\t}\n> +\n> +\tvoid set(unsigned int id, const ControlValue &value);\n> +\n> +private:\n> +\tbool enabled_ = false;\n> +\tDebugMetadata *parent_ = nullptr;\n> +\tControlList cache_;\n> +};\n> +\n> +} /* namespace libcamera */\n> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> index 1c5eef9cab80..1dddcd50c90b 100644\n> --- a/include/libcamera/internal/meson.build\n> +++ b/include/libcamera/internal/meson.build\n> @@ -14,6 +14,7 @@ libcamera_internal_headers = files([\n> 'control_serializer.h',\n> 'control_validator.h',\n> 'converter.h',\n> + 'debug_controls.h',\n> 'delayed_controls.h',\n> 'device_enumerator.h',\n> 'device_enumerator_sysfs.h',\n> diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml\n> index 1b1bd9507d25..d34a2d068b60 100644\n> --- a/src/libcamera/control_ids_core.yaml\n> +++ b/src/libcamera/control_ids_core.yaml\n> @@ -968,4 +968,9 @@ controls:\n> The default gamma value must be 2.2 which closely mimics sRGB gamma.\n> Note that this is camera gamma, so it is applied as 1.0/gamma.\n> \n> + - DebugMetadataEnable:\n> + type: bool\n> + description: |\n> + Enable or disable the debug metadata.\n> +\n> ...\n> diff --git a/src/libcamera/control_ids_debug.yaml b/src/libcamera/control_ids_debug.yaml\n> index 62f742c58129..797532712099 100644\n> --- a/src/libcamera/control_ids_debug.yaml\n> +++ b/src/libcamera/control_ids_debug.yaml\n> @@ -3,5 +3,4 @@\n> %YAML 1.1\n> ---\n> vendor: debug\n> -controls:\n> -\n> +controls: []\n\nIs this needed ? If so, should it go to the previous patch ?\n\n> diff --git a/src/libcamera/debug_controls.cpp b/src/libcamera/debug_controls.cpp\n> new file mode 100644\n> index 000000000000..84157aca5a74\n> --- /dev/null\n> +++ b/src/libcamera/debug_controls.cpp\n> @@ -0,0 +1,145 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Google Inc.\n> + *\n> + * Helper to easily record debug metadata inside libcamera.\n> + */\n> +\n> +#include \"libcamera/internal/debug_controls.h\"\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(DebugControls)\n> +\n> +/**\n> + * \\file debug_controls.h\n> + * \\brief Helper to easily record debug metadata inside libcamera\n> + */\n> +\n> +/**\n> + * \\class DebugMetadata\n> + * \\brief Helper to record metadata for later use\n> + *\n> + * When one wants to record debug metadata, the metadata list is often not\n> + * directly available (either because we are inside process() of an IPA or\n> + * because we are in a closed module). This class allows to record the data and\n> + * at a later point in time forward it either to another DebugMetadata instance\n> + * or to a ControlList.\n> + */\n> +\n> +/**\n> + * \\fn DebugMetadata::checkForEnable\n\ns/checkForEnable/checkForEnable()/\n\nSame below.\n\n> + * \\brief Check for DebugMetadataEnable in the supplied ControlList\n> + * \\param[in] controls The supplied ControlList\n> + *\n> + * Looks for controls::DebugMetadataEnable and enables or disables debug\n> + * metadata handling accordingly.\n> + */\n> +void DebugMetadata::checkForEnable(const ControlList &controls)\n\nI'm not a fan of the function name. \"check\" sounds like this function\nperforms a check, but it actually enables or disable recording of\nmetadata.\n\n> +{\n> +\tconst auto &ctrl = controls.get(controls::DebugMetadataEnable);\n> +\tif (ctrl)\n> +\t\tenable(*ctrl);\n> +}\n> +\n> +/**\n> + * \\fn DebugMetadata::enable\n> + * \\brief Enables or disabled metadata handling\n\n\\brief uses the imperative mood:\n\n * \\brief Enable or disable metadata handling\n\nOr s/handling/recording/\n\n> + * \\param[in] enable The enable state\n> + *\n> + * Enables or disables metadata handling according to \\a enable. When \\a enable\n\nThis sentence needs a subject (see below I've reviewed the patch from\nbottom to top).\n\n> + * is true, all calls to set() get cached and can later be retrieved using \\a\n> + * DebugMetadata::moveEntries(). When \\a enable is false, the cache gets cleared\n> + * and no further metadata is recorded.\n> + *\n> + * Forwarding to a parent is independent of the enabled state.\n> + */\n> +void DebugMetadata::enable(bool enable)\n> +{\n> +\tenabled_ = enable;\n> +\tif (!enabled_)\n> +\t\tcache_.clear();\n> +}\n> +\n> +/**\n> + * \\fn DebugMetadata::setParent\n> + * \\brief Assign a parent metadata handler\n> + * \\param[in] parent Pointer to the parent handler\n> + *\n> + * When a \\a parent gets set, all further calls to DebugMetadata::set are\n\ns/::set/::set()/\n\n> + * forwarded to that instance. It is not allowed to enable a DebugMetadata\n> + * object, log entries to it and later set the parent. This is done to keep a\n> + * path open for switching to tracing infrastructure later. For tracing one\n> + * would need some kind of \"context\" identifier that needs to be available on\n> + * set() time. The parent can be treated as such. The top level object\n> + * (the one where enable() get's called) lives in a place where that information\n> + * is also available.\n\nThere's room for improvement. Some of the information here probably\nbelongs to the \\class documentation. If you re-read the class\ndocumentation without any knowledge of how this class works and what it\ndoes, the documentation isn't very clear.\n\n> + *\n> + * The parent can be reset by passing a nullptr.\n> + */\n> +void DebugMetadata::setParent(DebugMetadata *parent)\n> +{\n> +\tparent_ = parent;\n> +\n> +\tif (!parent_)\n> +\t\treturn;\n> +\n> +\tif (!cache_.empty())\n> +\t\tLOG(DebugControls, Error)\n> +\t\t\t<< \"Controls were recorded before setting a parent.\"\n> +\t\t\t<< \" These are dropped.\";\n> +\n> +\tcache_.clear();\n> +}\n> +\n> +/**\n> + * \\fn DebugMetadata::moveEntries\n> + * \\brief Move all cached entries into a list\n\ns/list/control list/\n\n> + * \\param[in] list The list\n\nThe control list\n\n> + *\n> + * Moves all entries into the list specified by \\a list. Duplicate entries in\n> + * \\a list get overwritten.\n\n\"Moves\" needs a subject.\n\n * This function moves all entries into the list specified by \\a list. Duplicate\n * entries in \\a list get overwritten.\n\n> + */\n> +void DebugMetadata::moveEntries(ControlList &list)\n> +{\n> +\tlist.merge(std::move(cache_), ControlList::MergePolicy::OverwriteExisting);\n> +\tcache_.clear();\n> +}\n> +\n> +/**\n> + * \\fn DebugMetadata::set(const Control &ctrl, const V &value)\n> + * \\brief Set a value\n\n * \\brief Set the debug metadata for \\a ctrl to value \\a value\n\n> + * \\param[in] ctrl The ctrl to set\n\ns/ctrl/control/\n\n> + * \\param[in] value The control value\n> + *\n> + * Sets the debug metadata for \\a ctrl to value \\a value. If a parent is set,\n\nSame here and below, \"Sets\" needs a subject. Or drop the first sentence\nas it duplicates the \\brief.\n\n> + * the value gets passed there unconditionally. Otherwise it gets cached if the\n> + * instance is enabled or dropped silently when disabled.\n> + */\n> +\n> +/**\n> + * \\fn DebugMetadata::set(unsigned int id, const ControlValue &value)\n> + * \\brief Set a value\n> + * \\param[in] id The id of the control\n> + * \\param[in] value The control value\n> + *\n> + * Sets the debug metadata for \\a id to value \\a value. If a parent is set,\n> + * the value gets passed there unconditionally. Otherwise it gets cached if the\n> + * instance is enabled or dropped silently when disabled.\n> + */\n> +void DebugMetadata::set(unsigned int id, const ControlValue &value)\n> +{\n> +\tif (parent_) {\n> +\t\tparent_->set(id, value);\n> +\t\treturn;\n> +\t}\n> +\n> +\tif (!enabled_)\n> +\t\treturn;\n> +\n> +\tcache_.set(id, value);\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index aa9ab0291854..f7b5ee8dcc34 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -25,6 +25,7 @@ libcamera_internal_sources = files([\n> 'control_validator.cpp',\n> 'converter.cpp',\n> 'delayed_controls.cpp',\n> + 'debug_controls.cpp',\n> 'device_enumerator.cpp',\n> 'device_enumerator_sysfs.cpp',\n> 'dma_buf_allocator.cpp',","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 9A09DBD80A\n\tfor ;\n\tMon, 7 Oct 2024 22:05:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B99546353A;\n\tTue, 8 Oct 2024 00:05:52 +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 7237962C91\n\tfor ;\n\tTue, 8 Oct 2024 00:05:50 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [132.205.230.14])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BC33C2E0;\n\tTue, 8 Oct 2024 00:04: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=\"OfxS0EcP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728338654;\n\tbh=MJ++oNjrpbe1K34ZCWOZSs4bpCKi+8ovXpSCpKcGgck=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=OfxS0EcPtcr/+dtokxwJvTcdlDHUKo3elN29njRb1h0A625b2zd23NYjWkq6Qn8g2\n\tIwqFGmOst6nV0yaNIFhtF+Bw2jEafhIBZlDQooWUccugd9LpEKm43hpNXwSFbnWJm2\n\tmF72Ru0NLFTHgZwuGOysjeV4HIJEVkNKfihBrKgg=","Date":"Tue, 8 Oct 2024 01:05:44 +0300","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 2/7] libcamera: Add a DebugMetadata helper","Message-ID":"<20241007220544.GD30699@pendragon.ideasonboard.com>","References":"<20241007095425.211158-1-stefan.klug@ideasonboard.com>\n\t<20241007095425.211158-3-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241007095425.211158-3-stefan.klug@ideasonboard.com>","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":31619,"web_url":"https://patchwork.libcamera.org/comment/31619/","msgid":"","date":"2024-10-08T13:33:21","subject":"Re: [PATCH v2 2/7] libcamera: Add a DebugMetadata helper","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Laurent,\n\nOn Tue, Oct 08, 2024 at 01:05:44AM +0300, Laurent Pinchart wrote:\n> On Mon, Oct 07, 2024 at 11:54:06AM +0200, Stefan Klug wrote:\n> > Debug metadata often occurs in places where the metadata control list is\n> > not available e.g. in queueRequest() or processStatsBuffer() or even in\n> > a class far away from the metadata handling code. It is therefore\n> > difficult to add debug metadata without adding lots of boilerplate\n> > code. This can be mitigated by recording the metadata and forwarding it\n> > to the metadata control list when it becomes available. To solve the\n> > issue of code that is far away from the metadata context, add a chaining\n> > mechanism to allow loose coupling at runtime.\n> > \n> > Signed-off-by: Stefan Klug \n> > \n> > ---\n> > Changes in v2:\n> > - Replace assignments of ControlList by a moveEntries function\n> > - Replace assignUpstream by setParent\n> > - Improve documentation\n> > ---\n> > include/libcamera/internal/debug_controls.h | 46 +++++++\n> > include/libcamera/internal/meson.build | 1 +\n> > src/libcamera/control_ids_core.yaml | 5 +\n> > src/libcamera/control_ids_debug.yaml | 3 +-\n> > src/libcamera/debug_controls.cpp | 145 ++++++++++++++++++++\n> > src/libcamera/meson.build | 1 +\n> > 6 files changed, 199 insertions(+), 2 deletions(-)\n> > create mode 100644 include/libcamera/internal/debug_controls.h\n> > create mode 100644 src/libcamera/debug_controls.cpp\n> > \n> > diff --git a/include/libcamera/internal/debug_controls.h b/include/libcamera/internal/debug_controls.h\n> > new file mode 100644\n> > index 000000000000..00457d60b1a2\n> > --- /dev/null\n> > +++ b/include/libcamera/internal/debug_controls.h\n> > @@ -0,0 +1,46 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Google Inc.\n> > + *\n> > + * Debug metadata helpers\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +class DebugMetadata\n> > +{\n> > +public:\n> > +\tDebugMetadata() = default;\n> > +\n> > +\tvoid checkForEnable(const ControlList &controls);\n> > +\tvoid enable(bool enable = true);\n> > +\tvoid setParent(DebugMetadata *parent);\n> > +\tvoid moveEntries(ControlList &list);\n> > +\n> > +\ttemplate\n> > +\tvoid set(const Control &ctrl, const V &value)\n> > +\t{\n> > +\t\tif (parent_) {\n> > +\t\t\tparent_->set(ctrl, value);\n> \n> I think you can drop the explicit types here. Same below.\n\nOk, I convinced myself, that there is no way this could go wrong (also\nfor the generic case). So I'll remove them.\n\n> \n> > +\t\t\treturn;\n> > +\t\t}\n> > +\n> > +\t\tif (!enabled_)\n> > +\t\t\treturn;\n> > +\n> > +\t\tcache_.set(ctrl, value);\n> > +\t}\n> > +\n> > +\tvoid set(unsigned int id, const ControlValue &value);\n> > +\n> > +private:\n> > +\tbool enabled_ = false;\n> > +\tDebugMetadata *parent_ = nullptr;\n> > +\tControlList cache_;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> > index 1c5eef9cab80..1dddcd50c90b 100644\n> > --- a/include/libcamera/internal/meson.build\n> > +++ b/include/libcamera/internal/meson.build\n> > @@ -14,6 +14,7 @@ libcamera_internal_headers = files([\n> > 'control_serializer.h',\n> > 'control_validator.h',\n> > 'converter.h',\n> > + 'debug_controls.h',\n> > 'delayed_controls.h',\n> > 'device_enumerator.h',\n> > 'device_enumerator_sysfs.h',\n> > diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml\n> > index 1b1bd9507d25..d34a2d068b60 100644\n> > --- a/src/libcamera/control_ids_core.yaml\n> > +++ b/src/libcamera/control_ids_core.yaml\n> > @@ -968,4 +968,9 @@ controls:\n> > The default gamma value must be 2.2 which closely mimics sRGB gamma.\n> > Note that this is camera gamma, so it is applied as 1.0/gamma.\n> > \n> > + - DebugMetadataEnable:\n> > + type: bool\n> > + description: |\n> > + Enable or disable the debug metadata.\n> > +\n> > ...\n> > diff --git a/src/libcamera/control_ids_debug.yaml b/src/libcamera/control_ids_debug.yaml\n> > index 62f742c58129..797532712099 100644\n> > --- a/src/libcamera/control_ids_debug.yaml\n> > +++ b/src/libcamera/control_ids_debug.yaml\n> > @@ -3,5 +3,4 @@\n> > %YAML 1.1\n> > ---\n> > vendor: debug\n> > -controls:\n> > -\n> > +controls: []\n> \n> Is this needed ? If so, should it go to the previous patch ?\n\nOh, yes. This ended up in the wrong patch.\n\n> \n> > diff --git a/src/libcamera/debug_controls.cpp b/src/libcamera/debug_controls.cpp\n> > new file mode 100644\n> > index 000000000000..84157aca5a74\n> > --- /dev/null\n> > +++ b/src/libcamera/debug_controls.cpp\n> > @@ -0,0 +1,145 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Google Inc.\n> > + *\n> > + * Helper to easily record debug metadata inside libcamera.\n> > + */\n> > +\n> > +#include \"libcamera/internal/debug_controls.h\"\n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(DebugControls)\n> > +\n> > +/**\n> > + * \\file debug_controls.h\n> > + * \\brief Helper to easily record debug metadata inside libcamera\n> > + */\n> > +\n> > +/**\n> > + * \\class DebugMetadata\n> > + * \\brief Helper to record metadata for later use\n> > + *\n> > + * When one wants to record debug metadata, the metadata list is often not\n> > + * directly available (either because we are inside process() of an IPA or\n> > + * because we are in a closed module). This class allows to record the data and\n> > + * at a later point in time forward it either to another DebugMetadata instance\n> > + * or to a ControlList.\n> > + */\n> > +\n> > +/**\n> > + * \\fn DebugMetadata::checkForEnable\n> \n> s/checkForEnable/checkForEnable()/\n> \n> Same below.\n> \n> > + * \\brief Check for DebugMetadataEnable in the supplied ControlList\n> > + * \\param[in] controls The supplied ControlList\n> > + *\n> > + * Looks for controls::DebugMetadataEnable and enables or disables debug\n> > + * metadata handling accordingly.\n> > + */\n> > +void DebugMetadata::checkForEnable(const ControlList &controls)\n> \n> I'm not a fan of the function name. \"check\" sounds like this function\n> performs a check, but it actually enables or disable recording of\n> metadata.\n\nWhat about the following:\n\n1. Just an overload enable(ControlList&). Looks a bit weird when used:\ndebugMeta.enable(list)\n\n2. enableByControl(ControlList&). I think that is my favourite.\n\n> \n> > +{\n> > +\tconst auto &ctrl = controls.get(controls::DebugMetadataEnable);\n> > +\tif (ctrl)\n> > +\t\tenable(*ctrl);\n> > +}\n> > +\n> > +/**\n> > + * \\fn DebugMetadata::enable\n> > + * \\brief Enables or disabled metadata handling\n> \n> \\brief uses the imperative mood:\n> \n> * \\brief Enable or disable metadata handling\n> \n> Or s/handling/recording/\n> \n> > + * \\param[in] enable The enable state\n> > + *\n> > + * Enables or disables metadata handling according to \\a enable. When \\a enable\n> \n> This sentence needs a subject (see below I've reviewed the patch from\n> bottom to top).\n\nack\n\n> \n> > + * is true, all calls to set() get cached and can later be retrieved using \\a\n> > + * DebugMetadata::moveEntries(). When \\a enable is false, the cache gets cleared\n> > + * and no further metadata is recorded.\n> > + *\n> > + * Forwarding to a parent is independent of the enabled state.\n> > + */\n> > +void DebugMetadata::enable(bool enable)\n> > +{\n> > +\tenabled_ = enable;\n> > +\tif (!enabled_)\n> > +\t\tcache_.clear();\n> > +}\n> > +\n> > +/**\n> > + * \\fn DebugMetadata::setParent\n> > + * \\brief Assign a parent metadata handler\n> > + * \\param[in] parent Pointer to the parent handler\n> > + *\n> > + * When a \\a parent gets set, all further calls to DebugMetadata::set are\n> \n> s/::set/::set()/\n> \n> > + * forwarded to that instance. It is not allowed to enable a DebugMetadata\n> > + * object, log entries to it and later set the parent. This is done to keep a\n> > + * path open for switching to tracing infrastructure later. For tracing one\n> > + * would need some kind of \"context\" identifier that needs to be available on\n> > + * set() time. The parent can be treated as such. The top level object\n> > + * (the one where enable() get's called) lives in a place where that information\n> > + * is also available.\n> \n> There's room for improvement. Some of the information here probably\n> belongs to the \\class documentation. If you re-read the class\n> documentation without any knowledge of how this class works and what it\n> does, the documentation isn't very clear.\n> \n\nI rewrote parts of it. Let's see if v3 is clearer :-)\nThe comments below where also fixed during that rework.\n\nRegards,\nStefan\n\n> > + *\n> > + * The parent can be reset by passing a nullptr.\n> > + */\n> > +void DebugMetadata::setParent(DebugMetadata *parent)\n> > +{\n> > +\tparent_ = parent;\n> > +\n> > +\tif (!parent_)\n> > +\t\treturn;\n> > +\n> > +\tif (!cache_.empty())\n> > +\t\tLOG(DebugControls, Error)\n> > +\t\t\t<< \"Controls were recorded before setting a parent.\"\n> > +\t\t\t<< \" These are dropped.\";\n> > +\n> > +\tcache_.clear();\n> > +}\n> > +\n> > +/**\n> > + * \\fn DebugMetadata::moveEntries\n> > + * \\brief Move all cached entries into a list\n> \n> s/list/control list/\n> \n> > + * \\param[in] list The list\n> \n> The control list\n> \n> > + *\n> > + * Moves all entries into the list specified by \\a list. Duplicate entries in\n> > + * \\a list get overwritten.\n> \n> \"Moves\" needs a subject.\n> \n> * This function moves all entries into the list specified by \\a list. Duplicate\n> * entries in \\a list get overwritten.\n> \n> > + */\n> > +void DebugMetadata::moveEntries(ControlList &list)\n> > +{\n> > +\tlist.merge(std::move(cache_), ControlList::MergePolicy::OverwriteExisting);\n> > +\tcache_.clear();\n> > +}\n> > +\n> > +/**\n> > + * \\fn DebugMetadata::set(const Control &ctrl, const V &value)\n> > + * \\brief Set a value\n> \n> * \\brief Set the debug metadata for \\a ctrl to value \\a value\n> \n> > + * \\param[in] ctrl The ctrl to set\n> \n> s/ctrl/control/\n> \n> > + * \\param[in] value The control value\n> > + *\n> > + * Sets the debug metadata for \\a ctrl to value \\a value. If a parent is set,\n> \n> Same here and below, \"Sets\" needs a subject. Or drop the first sentence\n> as it duplicates the \\brief.\n> \n> > + * the value gets passed there unconditionally. Otherwise it gets cached if the\n> > + * instance is enabled or dropped silently when disabled.\n> > + */\n> > +\n> > +/**\n> > + * \\fn DebugMetadata::set(unsigned int id, const ControlValue &value)\n> > + * \\brief Set a value\n> > + * \\param[in] id The id of the control\n> > + * \\param[in] value The control value\n> > + *\n> > + * Sets the debug metadata for \\a id to value \\a value. If a parent is set,\n> > + * the value gets passed there unconditionally. Otherwise it gets cached if the\n> > + * instance is enabled or dropped silently when disabled.\n> > + */\n> > +void DebugMetadata::set(unsigned int id, const ControlValue &value)\n> > +{\n> > +\tif (parent_) {\n> > +\t\tparent_->set(id, value);\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tif (!enabled_)\n> > +\t\treturn;\n> > +\n> > +\tcache_.set(id, value);\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index aa9ab0291854..f7b5ee8dcc34 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -25,6 +25,7 @@ libcamera_internal_sources = files([\n> > 'control_validator.cpp',\n> > 'converter.cpp',\n> > 'delayed_controls.cpp',\n> > + 'debug_controls.cpp',\n> > 'device_enumerator.cpp',\n> > 'device_enumerator_sysfs.cpp',\n> > 'dma_buf_allocator.cpp',\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 F2B72BE080\n\tfor ;\n\tTue, 8 Oct 2024 13:33:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E775A6352E;\n\tTue, 8 Oct 2024 15:33:25 +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 97CF2618C9\n\tfor ;\n\tTue, 8 Oct 2024 15:33:24 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:d:f30b:aa60:fabf])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BAA99514;\n\tTue, 8 Oct 2024 15:31:47 +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=\"e7CdKUq8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728394307;\n\tbh=pCjROF80NC5aUBry6uWRrfaI2OOeC8WjSYQkUAEfays=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=e7CdKUq8YpQDGG4LxDcVQQ7XlDsvxEdZZVTRgPAI7fqpuV4S4+nlehKIa8AD7/xN/\n\tfMtCzVqL1uMWWi0dDdKXnfKaM/LTLTJQki2qRjZsmpbz9iMhyImJ8ByHSCNhnpjxnE\n\tEAZsFxbGOiDXAez91GncBQNT8Cv/dornEekNjwyM=","Date":"Tue, 8 Oct 2024 15:33:21 +0200","From":"Stefan Klug ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 2/7] libcamera: Add a DebugMetadata helper","Message-ID":"","References":"<20241007095425.211158-1-stefan.klug@ideasonboard.com>\n\t<20241007095425.211158-3-stefan.klug@ideasonboard.com>\n\t<20241007220544.GD30699@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241007220544.GD30699@pendragon.ideasonboard.com>","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":31621,"web_url":"https://patchwork.libcamera.org/comment/31621/","msgid":"<20241008142957.GA16232@pendragon.ideasonboard.com>","date":"2024-10-08T14:29:57","subject":"Re: [PATCH v2 2/7] libcamera: Add a DebugMetadata helper","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Oct 08, 2024 at 03:33:21PM +0200, Stefan Klug wrote:\n> Hi Laurent,\n> \n> On Tue, Oct 08, 2024 at 01:05:44AM +0300, Laurent Pinchart wrote:\n> > On Mon, Oct 07, 2024 at 11:54:06AM +0200, Stefan Klug wrote:\n> > > Debug metadata often occurs in places where the metadata control list is\n> > > not available e.g. in queueRequest() or processStatsBuffer() or even in\n> > > a class far away from the metadata handling code. It is therefore\n> > > difficult to add debug metadata without adding lots of boilerplate\n> > > code. This can be mitigated by recording the metadata and forwarding it\n> > > to the metadata control list when it becomes available. To solve the\n> > > issue of code that is far away from the metadata context, add a chaining\n> > > mechanism to allow loose coupling at runtime.\n> > > \n> > > Signed-off-by: Stefan Klug \n> > > \n> > > ---\n> > > Changes in v2:\n> > > - Replace assignments of ControlList by a moveEntries function\n> > > - Replace assignUpstream by setParent\n> > > - Improve documentation\n> > > ---\n> > > include/libcamera/internal/debug_controls.h | 46 +++++++\n> > > include/libcamera/internal/meson.build | 1 +\n> > > src/libcamera/control_ids_core.yaml | 5 +\n> > > src/libcamera/control_ids_debug.yaml | 3 +-\n> > > src/libcamera/debug_controls.cpp | 145 ++++++++++++++++++++\n> > > src/libcamera/meson.build | 1 +\n> > > 6 files changed, 199 insertions(+), 2 deletions(-)\n> > > create mode 100644 include/libcamera/internal/debug_controls.h\n> > > create mode 100644 src/libcamera/debug_controls.cpp\n> > > \n> > > diff --git a/include/libcamera/internal/debug_controls.h b/include/libcamera/internal/debug_controls.h\n> > > new file mode 100644\n> > > index 000000000000..00457d60b1a2\n> > > --- /dev/null\n> > > +++ b/include/libcamera/internal/debug_controls.h\n> > > @@ -0,0 +1,46 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2024, Google Inc.\n> > > + *\n> > > + * Debug metadata helpers\n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include \n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +class DebugMetadata\n> > > +{\n> > > +public:\n> > > +\tDebugMetadata() = default;\n> > > +\n> > > +\tvoid checkForEnable(const ControlList &controls);\n> > > +\tvoid enable(bool enable = true);\n> > > +\tvoid setParent(DebugMetadata *parent);\n> > > +\tvoid moveEntries(ControlList &list);\n> > > +\n> > > +\ttemplate\n> > > +\tvoid set(const Control &ctrl, const V &value)\n> > > +\t{\n> > > +\t\tif (parent_) {\n> > > +\t\t\tparent_->set(ctrl, value);\n> > \n> > I think you can drop the explicit types here. Same below.\n> \n> Ok, I convinced myself, that there is no way this could go wrong (also\n> for the generic case). So I'll remove them.\n> \n> > \n> > > +\t\t\treturn;\n> > > +\t\t}\n> > > +\n> > > +\t\tif (!enabled_)\n> > > +\t\t\treturn;\n> > > +\n> > > +\t\tcache_.set(ctrl, value);\n> > > +\t}\n> > > +\n> > > +\tvoid set(unsigned int id, const ControlValue &value);\n> > > +\n> > > +private:\n> > > +\tbool enabled_ = false;\n> > > +\tDebugMetadata *parent_ = nullptr;\n> > > +\tControlList cache_;\n> > > +};\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> > > index 1c5eef9cab80..1dddcd50c90b 100644\n> > > --- a/include/libcamera/internal/meson.build\n> > > +++ b/include/libcamera/internal/meson.build\n> > > @@ -14,6 +14,7 @@ libcamera_internal_headers = files([\n> > > 'control_serializer.h',\n> > > 'control_validator.h',\n> > > 'converter.h',\n> > > + 'debug_controls.h',\n> > > 'delayed_controls.h',\n> > > 'device_enumerator.h',\n> > > 'device_enumerator_sysfs.h',\n> > > diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml\n> > > index 1b1bd9507d25..d34a2d068b60 100644\n> > > --- a/src/libcamera/control_ids_core.yaml\n> > > +++ b/src/libcamera/control_ids_core.yaml\n> > > @@ -968,4 +968,9 @@ controls:\n> > > The default gamma value must be 2.2 which closely mimics sRGB gamma.\n> > > Note that this is camera gamma, so it is applied as 1.0/gamma.\n> > > \n> > > + - DebugMetadataEnable:\n> > > + type: bool\n> > > + description: |\n> > > + Enable or disable the debug metadata.\n> > > +\n> > > ...\n> > > diff --git a/src/libcamera/control_ids_debug.yaml b/src/libcamera/control_ids_debug.yaml\n> > > index 62f742c58129..797532712099 100644\n> > > --- a/src/libcamera/control_ids_debug.yaml\n> > > +++ b/src/libcamera/control_ids_debug.yaml\n> > > @@ -3,5 +3,4 @@\n> > > %YAML 1.1\n> > > ---\n> > > vendor: debug\n> > > -controls:\n> > > -\n> > > +controls: []\n> > \n> > Is this needed ? If so, should it go to the previous patch ?\n> \n> Oh, yes. This ended up in the wrong patch.\n> \n> > \n> > > diff --git a/src/libcamera/debug_controls.cpp b/src/libcamera/debug_controls.cpp\n> > > new file mode 100644\n> > > index 000000000000..84157aca5a74\n> > > --- /dev/null\n> > > +++ b/src/libcamera/debug_controls.cpp\n> > > @@ -0,0 +1,145 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2024, Google Inc.\n> > > + *\n> > > + * Helper to easily record debug metadata inside libcamera.\n> > > + */\n> > > +\n> > > +#include \"libcamera/internal/debug_controls.h\"\n> > > +\n> > > +#include \n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +LOG_DEFINE_CATEGORY(DebugControls)\n> > > +\n> > > +/**\n> > > + * \\file debug_controls.h\n> > > + * \\brief Helper to easily record debug metadata inside libcamera\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\class DebugMetadata\n> > > + * \\brief Helper to record metadata for later use\n> > > + *\n> > > + * When one wants to record debug metadata, the metadata list is often not\n> > > + * directly available (either because we are inside process() of an IPA or\n> > > + * because we are in a closed module). This class allows to record the data and\n> > > + * at a later point in time forward it either to another DebugMetadata instance\n> > > + * or to a ControlList.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn DebugMetadata::checkForEnable\n> > \n> > s/checkForEnable/checkForEnable()/\n> > \n> > Same below.\n> > \n> > > + * \\brief Check for DebugMetadataEnable in the supplied ControlList\n> > > + * \\param[in] controls The supplied ControlList\n> > > + *\n> > > + * Looks for controls::DebugMetadataEnable and enables or disables debug\n> > > + * metadata handling accordingly.\n> > > + */\n> > > +void DebugMetadata::checkForEnable(const ControlList &controls)\n> > \n> > I'm not a fan of the function name. \"check\" sounds like this function\n> > performs a check, but it actually enables or disable recording of\n> > metadata.\n> \n> What about the following:\n> \n> 1. Just an overload enable(ControlList&). Looks a bit weird when used:\n> debugMeta.enable(list)\n> \n> 2. enableByControl(ControlList&). I think that is my favourite.\n\nI can't think of a better name, so I'm OK with this. I wonder however if\nwe shouldn't drop this function and call controls.get() in the caller.\n\n> > > +{\n> > > +\tconst auto &ctrl = controls.get(controls::DebugMetadataEnable);\n> > > +\tif (ctrl)\n> > > +\t\tenable(*ctrl);\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn DebugMetadata::enable\n> > > + * \\brief Enables or disabled metadata handling\n> > \n> > \\brief uses the imperative mood:\n> > \n> > * \\brief Enable or disable metadata handling\n> > \n> > Or s/handling/recording/\n> > \n> > > + * \\param[in] enable The enable state\n> > > + *\n> > > + * Enables or disables metadata handling according to \\a enable. When \\a enable\n> > \n> > This sentence needs a subject (see below I've reviewed the patch from\n> > bottom to top).\n> \n> ack\n> \n> > > + * is true, all calls to set() get cached and can later be retrieved using \\a\n> > > + * DebugMetadata::moveEntries(). When \\a enable is false, the cache gets cleared\n> > > + * and no further metadata is recorded.\n> > > + *\n> > > + * Forwarding to a parent is independent of the enabled state.\n> > > + */\n> > > +void DebugMetadata::enable(bool enable)\n> > > +{\n> > > +\tenabled_ = enable;\n> > > +\tif (!enabled_)\n> > > +\t\tcache_.clear();\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn DebugMetadata::setParent\n> > > + * \\brief Assign a parent metadata handler\n> > > + * \\param[in] parent Pointer to the parent handler\n> > > + *\n> > > + * When a \\a parent gets set, all further calls to DebugMetadata::set are\n> > \n> > s/::set/::set()/\n> > \n> > > + * forwarded to that instance. It is not allowed to enable a DebugMetadata\n> > > + * object, log entries to it and later set the parent. This is done to keep a\n> > > + * path open for switching to tracing infrastructure later. For tracing one\n> > > + * would need some kind of \"context\" identifier that needs to be available on\n> > > + * set() time. The parent can be treated as such. The top level object\n> > > + * (the one where enable() get's called) lives in a place where that information\n> > > + * is also available.\n> > \n> > There's room for improvement. Some of the information here probably\n> > belongs to the \\class documentation. If you re-read the class\n> > documentation without any knowledge of how this class works and what it\n> > does, the documentation isn't very clear.\n> \n> I rewrote parts of it. Let's see if v3 is clearer :-)\n> The comments below where also fixed during that rework.\n> \n> > > + *\n> > > + * The parent can be reset by passing a nullptr.\n> > > + */\n> > > +void DebugMetadata::setParent(DebugMetadata *parent)\n> > > +{\n> > > +\tparent_ = parent;\n> > > +\n> > > +\tif (!parent_)\n> > > +\t\treturn;\n> > > +\n> > > +\tif (!cache_.empty())\n> > > +\t\tLOG(DebugControls, Error)\n> > > +\t\t\t<< \"Controls were recorded before setting a parent.\"\n> > > +\t\t\t<< \" These are dropped.\";\n> > > +\n> > > +\tcache_.clear();\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn DebugMetadata::moveEntries\n> > > + * \\brief Move all cached entries into a list\n> > \n> > s/list/control list/\n> > \n> > > + * \\param[in] list The list\n> > \n> > The control list\n> > \n> > > + *\n> > > + * Moves all entries into the list specified by \\a list. Duplicate entries in\n> > > + * \\a list get overwritten.\n> > \n> > \"Moves\" needs a subject.\n> > \n> > * This function moves all entries into the list specified by \\a list. Duplicate\n> > * entries in \\a list get overwritten.\n> > \n> > > + */\n> > > +void DebugMetadata::moveEntries(ControlList &list)\n> > > +{\n> > > +\tlist.merge(std::move(cache_), ControlList::MergePolicy::OverwriteExisting);\n> > > +\tcache_.clear();\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn DebugMetadata::set(const Control &ctrl, const V &value)\n> > > + * \\brief Set a value\n> > \n> > * \\brief Set the debug metadata for \\a ctrl to value \\a value\n> > \n> > > + * \\param[in] ctrl The ctrl to set\n> > \n> > s/ctrl/control/\n> > \n> > > + * \\param[in] value The control value\n> > > + *\n> > > + * Sets the debug metadata for \\a ctrl to value \\a value. If a parent is set,\n> > \n> > Same here and below, \"Sets\" needs a subject. Or drop the first sentence\n> > as it duplicates the \\brief.\n> > \n> > > + * the value gets passed there unconditionally. Otherwise it gets cached if the\n> > > + * instance is enabled or dropped silently when disabled.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn DebugMetadata::set(unsigned int id, const ControlValue &value)\n> > > + * \\brief Set a value\n> > > + * \\param[in] id The id of the control\n> > > + * \\param[in] value The control value\n> > > + *\n> > > + * Sets the debug metadata for \\a id to value \\a value. If a parent is set,\n> > > + * the value gets passed there unconditionally. Otherwise it gets cached if the\n> > > + * instance is enabled or dropped silently when disabled.\n> > > + */\n> > > +void DebugMetadata::set(unsigned int id, const ControlValue &value)\n> > > +{\n> > > +\tif (parent_) {\n> > > +\t\tparent_->set(id, value);\n> > > +\t\treturn;\n> > > +\t}\n> > > +\n> > > +\tif (!enabled_)\n> > > +\t\treturn;\n> > > +\n> > > +\tcache_.set(id, value);\n> > > +}\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > index aa9ab0291854..f7b5ee8dcc34 100644\n> > > --- a/src/libcamera/meson.build\n> > > +++ b/src/libcamera/meson.build\n> > > @@ -25,6 +25,7 @@ libcamera_internal_sources = files([\n> > > 'control_validator.cpp',\n> > > 'converter.cpp',\n> > > 'delayed_controls.cpp',\n> > > + 'debug_controls.cpp',\n> > > 'device_enumerator.cpp',\n> > > 'device_enumerator_sysfs.cpp',\n> > > 'dma_buf_allocator.cpp',","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 F00ABBE080\n\tfor ;\n\tTue, 8 Oct 2024 14:30:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EC8BC6352E;\n\tTue, 8 Oct 2024 16:30:04 +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 591B8618C9\n\tfor ;\n\tTue, 8 Oct 2024 16:30:03 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [132.205.230.3])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 42F5EA47;\n\tTue, 8 Oct 2024 16:28:26 +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=\"rGeGc0RZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728397706;\n\tbh=FKBlRAtBysVKe0d62vS8/wifn3CFXdOPg/lwhLxMar0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rGeGc0RZSoCfsxezkSqdpVL5JonjG0JFXFqDaKE5rgO+qzKLmrL3a6t/aWeScWJhD\n\tPDbtFdHVR4djS0HhbYATy8jRiJljyW9Cqo7e612i1YP49T6i+l6SFnC8Ufqth9Eve0\n\tOWTZINbK2qsWwv5iUv8YqAnzPpV61nPK8FiucMr0=","Date":"Tue, 8 Oct 2024 17:29:57 +0300","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 2/7] libcamera: Add a DebugMetadata helper","Message-ID":"<20241008142957.GA16232@pendragon.ideasonboard.com>","References":"<20241007095425.211158-1-stefan.klug@ideasonboard.com>\n\t<20241007095425.211158-3-stefan.klug@ideasonboard.com>\n\t<20241007220544.GD30699@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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\" "}}]