[{"id":31578,"web_url":"https://patchwork.libcamera.org/comment/31578/","msgid":"<20241003212305.GC5484@pendragon.ideasonboard.com>","date":"2024-10-03T21:23:05","subject":"Re: [PATCH v1 2/8] 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":"Hi Stefan,\n\nThank you for the patch.\n\nOn Wed, Oct 02, 2024 at 06:19:20PM +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 lot's of boilerplate\n\ns/lot's/lots/\n\n> code. This can be mitigated by recording the metadata and forwarding it\n> to an upstream helper or the metadata control list when they become\n> available. Add such a helper.\n> \n> Signed-off-by: Stefan Klug \n> ---\n> include/libcamera/internal/debug_controls.h | 52 +++++++\n> include/libcamera/internal/meson.build | 1 +\n> src/libcamera/control_ids_core.yaml | 6 +\n> src/libcamera/debug_controls.cpp | 144 ++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 5 files changed, 204 insertions(+)\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..d95c03e1db1f\n> --- /dev/null\n> +++ b/include/libcamera/internal/debug_controls.h\n> @@ -0,0 +1,52 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Ideas on Board Oy\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 assignUpstream(DebugMetadata *upstream);\n> +\tvoid assignControlList(ControlList *list);\n> +\n> +\ttemplate\n> +\tvoid set(const Control &ctrl, const V &value)\n> +\t{\n> +\t\tif (upstream_) {\n> +\t\t\tupstream_->set(ctrl, value);\n\nDo you need to explicitly mention here, won't type deduction work\nautomatically ? Same below.\n\n> +\t\t\treturn;\n> +\t\t}\n> +\n> +\t\tif (!enabled_)\n> +\t\t\treturn;\n> +\n> +\t\tif (list_) {\n> +\t\t\tlist_->set(ctrl, value);\n> +\t\t\treturn;\n> +\t\t}\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> +\tControlList *list_ = nullptr;\n> +\tDebugMetadata *upstream_ = 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..103bcb593c4a 100644\n> --- a/src/libcamera/control_ids_core.yaml\n> +++ b/src/libcamera/control_ids_core.yaml\n> @@ -967,5 +967,11 @@ controls:\n> \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\nExtra blank line.\n\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..11a92a6b5871\n> --- /dev/null\n> +++ b/src/libcamera/debug_controls.cpp\n> @@ -0,0 +1,144 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Ideas on Board Oy.\n> + *\n> + * Helper to easily record debug metadata inside libcamera.\n> + */\n> +\n> +#include \"libcamera/internal/debug_controls.h\"\n> +\n> +namespace libcamera {\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> + * \\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> +\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> + * \\param[in] enable The enable state\n> + *\n> + * Enables or disables metadata handling according to \\a enable. When enable is\n> + * false, the cache gets cleared and no further metadata is recorded.\n> + */\n> +void DebugMetadata::enable(bool enable)\n> +{\n> +\tenabled_ = enable;\n> +\tif (!enabled_)\n> +\t\tcache_.clear();\n> +}\n> +\n> +/**\n> + * \\fn DebugMetadata::assignUpstream\n> + * \\brief Assign an upstream metadata handler\n> + * \\param[in] upstream Pointer to the upstream handler\n> + *\n> + * When a upstream gets set, the cache is copies to that list and all further\n\ns/copies/copied/\n\n> + * calls to DebugMetadata::set are forwarded to that list.\n> + *\n> + * The upstream can be reset by passing a nullptr.\n\nI think \"parent\" is a better name than \"upstream\", the parent concept is\nwidely used. I'd call the function setParent() as we use \"set\" rather\nthan \"assign\".\n\n> + */\n> +void DebugMetadata::assignUpstream(DebugMetadata *upstream)\n> +{\n> +\tupstream_ = upstream;\n> +\n> +\tif (!upstream_)\n> +\t\treturn;\n> +\n> +\tfor (const auto &ctrl : cache_)\n> +\t\tupstream_->set(ctrl.first, ctrl.second);\n> +\n> +\tcache_.clear();\n> +}\n> +\n> +/**\n> + * \\fn DebugMetadata::assignControlList\n> + * \\brief Assign a control list\n> + * \\param[in] list Pointer to the list\n> + *\n> + * When a list gets set, the cache is copies to that list and all further calls\n\ns/copies/copied/\n\n> + * to DebugMetadata::set are forwarded to that list.\n> + *\n> + * The upstream can be reset by passing a nullptr.\n\ns/upstream/list/\n\n> + */\n> +void DebugMetadata::assignControlList(ControlList *list)\n> +{\n> +\tlist_ = list;\n> +\n> +\tif (list_) {\n> +\t\tlist_->merge(cache_);\n> +\t\tcache_.clear();\n> +\t}\n> +}\n> +\n> +/**\n> + * \\fn DebugMetadata::set(const Control &ctrl, const V &value)\n> + * \\brief Set a value\n> + * \\param[in] ctrl The ctrl to set\n> + * \\param[in] value The control value\n> + *\n> + * Sets the debug metadata for ctrl to value \\a value. If an upstream or a\n> + * control list is set, the value gets passed there (upstream takes precedence).\n> + * Otherwise the value is cached until one of them gets assigned.\n> + *\n> + * When the instance is disabled and upstream is not set, the value gets dropped.\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 to value \\a value. If an upstream or a control list\n> + * is set, the value gets passed there (upstream takes precedence).\n> + * Otherwise the value is cached until one of them gets assigned.\n> + *\n> + * When the instance is disabled and upstream is not set, the value gets dropped.\n> + */\n> +void DebugMetadata::set(unsigned int id, const ControlValue &value)\n> +{\n> +\tif (upstream_) {\n> +\t\tupstream_->set(id, value);\n> +\t\treturn;\n> +\t}\n> +\n> +\tif (!enabled_)\n> +\t\treturn;\n> +\n> +\tif (list_) {\n> +\t\tlist_->set(id, value);\n> +\t\treturn;\n> +\t}\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 827EDBD80A\n\tfor ;\n\tThu, 3 Oct 2024 21:23:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 30F6A6352C;\n\tThu, 3 Oct 2024 23:23:10 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6BB6C6351F\n\tfor ;\n\tThu, 3 Oct 2024 23:23:09 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0A89766F;\n\tThu, 3 Oct 2024 23:21:35 +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=\"Z3rXN/ln\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727990496;\n\tbh=l5oF9KIhl3e2Qmt5vUyv2mPrd5GgGyzdKyw7oXDgA6E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Z3rXN/lnaExcHw0Vhxr2zglErwKtXu4KyyfJN+A3pJwr3q7SY8n13jR070JfV06Mh\n\tYt7LgrNraS0HoIGFn7PcOdz7Zv2cG3z99vVYpeFjtV9eWZiyZW6cZ0s14EpnFgEB+B\n\tE6vr+r5p/anOVOIwcJEByFiFtT4IMjNuFQ/uJ2bk=","Date":"Fri, 4 Oct 2024 00:23:05 +0300","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 2/8] libcamera: Add a DebugMetadata helper","Message-ID":"<20241003212305.GC5484@pendragon.ideasonboard.com>","References":"<20241002161933.247091-1-stefan.klug@ideasonboard.com>\n\t<20241002161933.247091-3-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241002161933.247091-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":31596,"web_url":"https://patchwork.libcamera.org/comment/31596/","msgid":"","date":"2024-10-04T14:33:36","subject":"Re: [PATCH v1 2/8] 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\nThank you for the review. \n\nOn Fri, Oct 04, 2024 at 12:23:05AM +0300, Laurent Pinchart wrote:\n> Hi Stefan,\n> \n> Thank you for the patch.\n> \n> On Wed, Oct 02, 2024 at 06:19:20PM +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 lot's of boilerplate\n> \n> s/lot's/lots/\n> \n> > code. This can be mitigated by recording the metadata and forwarding it\n> > to an upstream helper or the metadata control list when they become\n> > available. Add such a helper.\n> > \n> > Signed-off-by: Stefan Klug \n> > ---\n> > include/libcamera/internal/debug_controls.h | 52 +++++++\n> > include/libcamera/internal/meson.build | 1 +\n> > src/libcamera/control_ids_core.yaml | 6 +\n> > src/libcamera/debug_controls.cpp | 144 ++++++++++++++++++++\n> > src/libcamera/meson.build | 1 +\n> > 5 files changed, 204 insertions(+)\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..d95c03e1db1f\n> > --- /dev/null\n> > +++ b/include/libcamera/internal/debug_controls.h\n> > @@ -0,0 +1,52 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Ideas on Board Oy\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 assignUpstream(DebugMetadata *upstream);\n> > +\tvoid assignControlList(ControlList *list);\n> > +\n> > +\ttemplate\n> > +\tvoid set(const Control &ctrl, const V &value)\n> > +\t{\n> > +\t\tif (upstream_) {\n> > +\t\t\tupstream_->set(ctrl, value);\n> \n> Do you need to explicitly mention here, won't type deduction work\n> automatically ? Same below.\n\nInteresting question. I would say it should work. But I can't tell for\nsure if there could be cases where it doesn't work. By specifying it\nexplicitly I ensure that the user of the function can specify it\nmanually if needed. Can you say for sure, that there no cases\nwhere auto deduction fails?\n\n> \n> > +\t\t\treturn;\n> > +\t\t}\n> > +\n> > +\t\tif (!enabled_)\n> > +\t\t\treturn;\n> > +\n> > +\t\tif (list_) {\n> > +\t\t\tlist_->set(ctrl, value);\n> > +\t\t\treturn;\n> > +\t\t}\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> > +\tControlList *list_ = nullptr;\n> > +\tDebugMetadata *upstream_ = 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..103bcb593c4a 100644\n> > --- a/src/libcamera/control_ids_core.yaml\n> > +++ b/src/libcamera/control_ids_core.yaml\n> > @@ -967,5 +967,11 @@ controls:\n> > \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> Extra blank line.\n> \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..11a92a6b5871\n> > --- /dev/null\n> > +++ b/src/libcamera/debug_controls.cpp\n> > @@ -0,0 +1,144 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Ideas on Board Oy.\n> > + *\n> > + * Helper to easily record debug metadata inside libcamera.\n> > + */\n> > +\n> > +#include \"libcamera/internal/debug_controls.h\"\n> > +\n> > +namespace libcamera {\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> > + * \\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> > +\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> > + * \\param[in] enable The enable state\n> > + *\n> > + * Enables or disables metadata handling according to \\a enable. When enable is\n> > + * false, the cache gets cleared and no further metadata is recorded.\n> > + */\n> > +void DebugMetadata::enable(bool enable)\n> > +{\n> > +\tenabled_ = enable;\n> > +\tif (!enabled_)\n> > +\t\tcache_.clear();\n> > +}\n> > +\n> > +/**\n> > + * \\fn DebugMetadata::assignUpstream\n> > + * \\brief Assign an upstream metadata handler\n> > + * \\param[in] upstream Pointer to the upstream handler\n> > + *\n> > + * When a upstream gets set, the cache is copies to that list and all further\n> \n> s/copies/copied/\n> \n> > + * calls to DebugMetadata::set are forwarded to that list.\n> > + *\n> > + * The upstream can be reset by passing a nullptr.\n> \n> I think \"parent\" is a better name than \"upstream\", the parent concept is\n> widely used. I'd call the function setParent() as we use \"set\" rather\n> than \"assign\".\n\nAs discussed, I will change that.\n\n> \n> > + */\n> > +void DebugMetadata::assignUpstream(DebugMetadata *upstream)\n> > +{\n> > +\tupstream_ = upstream;\n> > +\n> > +\tif (!upstream_)\n> > +\t\treturn;\n> > +\n> > +\tfor (const auto &ctrl : cache_)\n> > +\t\tupstream_->set(ctrl.first, ctrl.second);\n> > +\n> > +\tcache_.clear();\n> > +}\n> > +\n> > +/**\n> > + * \\fn DebugMetadata::assignControlList\n> > + * \\brief Assign a control list\n> > + * \\param[in] list Pointer to the list\n> > + *\n> > + * When a list gets set, the cache is copies to that list and all further calls\n> \n> s/copies/copied/\n> \n> > + * to DebugMetadata::set are forwarded to that list.\n> > + *\n> > + * The upstream can be reset by passing a nullptr.\n> \n> s/upstream/list/\n> \n> > + */\n> > +void DebugMetadata::assignControlList(ControlList *list)\n> > +{\n> > +\tlist_ = list;\n> > +\n> > +\tif (list_) {\n> > +\t\tlist_->merge(cache_);\n> > +\t\tcache_.clear();\n> > +\t}\n> > +}\n> > +\n> > +/**\n> > + * \\fn DebugMetadata::set(const Control &ctrl, const V &value)\n> > + * \\brief Set a value\n> > + * \\param[in] ctrl The ctrl to set\n> > + * \\param[in] value The control value\n> > + *\n> > + * Sets the debug metadata for ctrl to value \\a value. If an upstream or a\n> > + * control list is set, the value gets passed there (upstream takes precedence).\n> > + * Otherwise the value is cached until one of them gets assigned.\n> > + *\n> > + * When the instance is disabled and upstream is not set, the value gets dropped.\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 to value \\a value. If an upstream or a control list\n> > + * is set, the value gets passed there (upstream takes precedence).\n> > + * Otherwise the value is cached until one of them gets assigned.\n> > + *\n> > + * When the instance is disabled and upstream is not set, the value gets dropped.\n> > + */\n> > +void DebugMetadata::set(unsigned int id, const ControlValue &value)\n> > +{\n> > +\tif (upstream_) {\n> > +\t\tupstream_->set(id, value);\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tif (!enabled_)\n> > +\t\treturn;\n> > +\n> > +\tif (list_) {\n> > +\t\tlist_->set(id, value);\n> > +\t\treturn;\n> > +\t}\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 34DFAC3257\n\tfor ;\n\tFri, 4 Oct 2024 14:33:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C881663523;\n\tFri, 4 Oct 2024 16:33:42 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 314F662C8F\n\tfor ;\n\tFri, 4 Oct 2024 16:33:40 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:e38d:2c68:7c38:a446])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E6C698A9;\n\tFri, 4 Oct 2024 16:32:05 +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=\"ECqhPtxf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728052326;\n\tbh=0b6WNRGg2PxTCHEKkBrYk6hXniTdRjRaEQNVHgLkFDc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ECqhPtxfqPEvGauPywHEOiHrSdj/GSyLwkqzTAulAC79ynnqR4d5j/joLjmEpuLK7\n\t3ps0WLfmzesm0TCUorVwypPdLjepaI1HdVc/Vq7Ue0WQOs7Wua/nS7449nQNj2ups8\n\tFxWmRlunk1isBJLG41ToxB0XeI16wYBmi5LOFnns=","Date":"Fri, 4 Oct 2024 16:33:36 +0200","From":"Stefan Klug ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 2/8] libcamera: Add a DebugMetadata helper","Message-ID":"","References":"<20241002161933.247091-1-stefan.klug@ideasonboard.com>\n\t<20241002161933.247091-3-stefan.klug@ideasonboard.com>\n\t<20241003212305.GC5484@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241003212305.GC5484@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":31603,"web_url":"https://patchwork.libcamera.org/comment/31603/","msgid":"<20241007214813.GC30699@pendragon.ideasonboard.com>","date":"2024-10-07T21:48:13","subject":"Re: [PATCH v1 2/8] 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":"Hi Stefan,\n\nOn Fri, Oct 04, 2024 at 04:33:36PM +0200, Stefan Klug wrote:\n> On Fri, Oct 04, 2024 at 12:23:05AM +0300, Laurent Pinchart wrote:\n> > On Wed, Oct 02, 2024 at 06:19:20PM +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 lot's of boilerplate\n> > \n> > s/lot's/lots/\n> > \n> > > code. This can be mitigated by recording the metadata and forwarding it\n> > > to an upstream helper or the metadata control list when they become\n> > > available. Add such a helper.\n> > > \n> > > Signed-off-by: Stefan Klug \n> > > ---\n> > > include/libcamera/internal/debug_controls.h | 52 +++++++\n> > > include/libcamera/internal/meson.build | 1 +\n> > > src/libcamera/control_ids_core.yaml | 6 +\n> > > src/libcamera/debug_controls.cpp | 144 ++++++++++++++++++++\n> > > src/libcamera/meson.build | 1 +\n> > > 5 files changed, 204 insertions(+)\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..d95c03e1db1f\n> > > --- /dev/null\n> > > +++ b/include/libcamera/internal/debug_controls.h\n> > > @@ -0,0 +1,52 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2024, Ideas on Board Oy\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 assignUpstream(DebugMetadata *upstream);\n> > > +\tvoid assignControlList(ControlList *list);\n> > > +\n> > > +\ttemplate\n> > > +\tvoid set(const Control &ctrl, const V &value)\n> > > +\t{\n> > > +\t\tif (upstream_) {\n> > > +\t\t\tupstream_->set(ctrl, value);\n> > \n> > Do you need to explicitly mention here, won't type deduction work\n> > automatically ? Same below.\n> \n> Interesting question. I would say it should work. But I can't tell for\n> sure if there could be cases where it doesn't work. By specifying it\n> explicitly I ensure that the user of the function can specify it\n> manually if needed. Can you say for sure, that there no cases\n> where auto deduction fails?\n\nI don't see how it could fail.\n\n> > > +\t\t\treturn;\n> > > +\t\t}\n> > > +\n> > > +\t\tif (!enabled_)\n> > > +\t\t\treturn;\n> > > +\n> > > +\t\tif (list_) {\n> > > +\t\t\tlist_->set(ctrl, value);\n> > > +\t\t\treturn;\n> > > +\t\t}\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> > > +\tControlList *list_ = nullptr;\n> > > +\tDebugMetadata *upstream_ = 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..103bcb593c4a 100644\n> > > --- a/src/libcamera/control_ids_core.yaml\n> > > +++ b/src/libcamera/control_ids_core.yaml\n> > > @@ -967,5 +967,11 @@ controls:\n> > > \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> > Extra blank line.\n> > \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..11a92a6b5871\n> > > --- /dev/null\n> > > +++ b/src/libcamera/debug_controls.cpp\n> > > @@ -0,0 +1,144 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2024, Ideas on Board Oy.\n> > > + *\n> > > + * Helper to easily record debug metadata inside libcamera.\n> > > + */\n> > > +\n> > > +#include \"libcamera/internal/debug_controls.h\"\n> > > +\n> > > +namespace libcamera {\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> > > + * \\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> > > +\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> > > + * \\param[in] enable The enable state\n> > > + *\n> > > + * Enables or disables metadata handling according to \\a enable. When enable is\n> > > + * false, the cache gets cleared and no further metadata is recorded.\n> > > + */\n> > > +void DebugMetadata::enable(bool enable)\n> > > +{\n> > > +\tenabled_ = enable;\n> > > +\tif (!enabled_)\n> > > +\t\tcache_.clear();\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn DebugMetadata::assignUpstream\n> > > + * \\brief Assign an upstream metadata handler\n> > > + * \\param[in] upstream Pointer to the upstream handler\n> > > + *\n> > > + * When a upstream gets set, the cache is copies to that list and all further\n> > \n> > s/copies/copied/\n> > \n> > > + * calls to DebugMetadata::set are forwarded to that list.\n> > > + *\n> > > + * The upstream can be reset by passing a nullptr.\n> > \n> > I think \"parent\" is a better name than \"upstream\", the parent concept is\n> > widely used. I'd call the function setParent() as we use \"set\" rather\n> > than \"assign\".\n> \n> As discussed, I will change that.\n> \n> > > + */\n> > > +void DebugMetadata::assignUpstream(DebugMetadata *upstream)\n> > > +{\n> > > +\tupstream_ = upstream;\n> > > +\n> > > +\tif (!upstream_)\n> > > +\t\treturn;\n> > > +\n> > > +\tfor (const auto &ctrl : cache_)\n> > > +\t\tupstream_->set(ctrl.first, ctrl.second);\n> > > +\n> > > +\tcache_.clear();\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn DebugMetadata::assignControlList\n> > > + * \\brief Assign a control list\n> > > + * \\param[in] list Pointer to the list\n> > > + *\n> > > + * When a list gets set, the cache is copies to that list and all further calls\n> > \n> > s/copies/copied/\n> > \n> > > + * to DebugMetadata::set are forwarded to that list.\n> > > + *\n> > > + * The upstream can be reset by passing a nullptr.\n> > \n> > s/upstream/list/\n> > \n> > > + */\n> > > +void DebugMetadata::assignControlList(ControlList *list)\n> > > +{\n> > > +\tlist_ = list;\n> > > +\n> > > +\tif (list_) {\n> > > +\t\tlist_->merge(cache_);\n> > > +\t\tcache_.clear();\n> > > +\t}\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn DebugMetadata::set(const Control &ctrl, const V &value)\n> > > + * \\brief Set a value\n> > > + * \\param[in] ctrl The ctrl to set\n> > > + * \\param[in] value The control value\n> > > + *\n> > > + * Sets the debug metadata for ctrl to value \\a value. If an upstream or a\n> > > + * control list is set, the value gets passed there (upstream takes precedence).\n> > > + * Otherwise the value is cached until one of them gets assigned.\n> > > + *\n> > > + * When the instance is disabled and upstream is not set, the value gets dropped.\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 to value \\a value. If an upstream or a control list\n> > > + * is set, the value gets passed there (upstream takes precedence).\n> > > + * Otherwise the value is cached until one of them gets assigned.\n> > > + *\n> > > + * When the instance is disabled and upstream is not set, the value gets dropped.\n> > > + */\n> > > +void DebugMetadata::set(unsigned int id, const ControlValue &value)\n> > > +{\n> > > +\tif (upstream_) {\n> > > +\t\tupstream_->set(id, value);\n> > > +\t\treturn;\n> > > +\t}\n> > > +\n> > > +\tif (!enabled_)\n> > > +\t\treturn;\n> > > +\n> > > +\tif (list_) {\n> > > +\t\tlist_->set(id, value);\n> > > +\t\treturn;\n> > > +\t}\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 4ECCFBE080\n\tfor ;\n\tMon, 7 Oct 2024 21:48:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 260396353A;\n\tMon, 7 Oct 2024 23:48: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 DFD826351F\n\tfor ;\n\tMon, 7 Oct 2024 23:48:19 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [132.205.230.14])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 56A6755;\n\tMon, 7 Oct 2024 23:46:43 +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=\"RCDRZ0XU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728337603;\n\tbh=76TmOCGzfOQQ47foiSbr3IvICoTvV+Rk5sR9jnl/BEI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=RCDRZ0XUSY2t6PsVq68ycL0g9uqqdZ70tdiu5c7wr9595K90lwpHf67UujQMx1oqP\n\t/lHX8nLD4OnO0alUzlnbN6nYMQY1SstPsLhGndoFVB1suYn6A6u5PTCZaY9wJ5i714\n\tK23jzvCwOzxiwqnsHpfkaQWwhx6NaB+llU+0Hkow=","Date":"Tue, 8 Oct 2024 00:48:13 +0300","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 2/8] libcamera: Add a DebugMetadata helper","Message-ID":"<20241007214813.GC30699@pendragon.ideasonboard.com>","References":"<20241002161933.247091-1-stefan.klug@ideasonboard.com>\n\t<20241002161933.247091-3-stefan.klug@ideasonboard.com>\n\t<20241003212305.GC5484@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\" "}}]