[{"id":35188,"web_url":"https://patchwork.libcamera.org/comment/35188/","msgid":"","date":"2025-07-28T10:19:11","subject":"Re: [RFC PATCH v2 19/22] libcamera: pipeline_handler: Inject \"debug\"\n\tmetadata","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n\nOn Mon, Jul 21, 2025 at 12:46:19PM +0200, Barnabás Pőcze wrote:\n> Inject all metadata controls in the \"debug\" namespace into\n> the metadata plan of each camera so that they can be used\n> seamlessly. Dynamically sized array-like controls have a\n> hard-coded size of 32 elements.\n>\n> Additionally, a new type is added for inspecting properties of\n> a control type at runtime since that was not available previously.\n>\n> Signed-off-by: Barnabás Pőcze \n> ---\n> include/libcamera/internal/controls.h | 39 +++++++++++++++++++++++\n> src/libcamera/controls.cpp | 2 +-\n> src/libcamera/pipeline_handler.cpp | 45 +++++++++++++++++++++++++++\n> 3 files changed, 85 insertions(+), 1 deletion(-)\n> create mode 100644 include/libcamera/internal/controls.h\n>\n> diff --git a/include/libcamera/internal/controls.h b/include/libcamera/internal/controls.h\n> new file mode 100644\n> index 000000000..be3f93e43\n> --- /dev/null\n> +++ b/include/libcamera/internal/controls.h\n> @@ -0,0 +1,39 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2025, Ideas on Board Oy\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +namespace libcamera::controls::details {\n> +\n> +struct TypeInfo {\n> +\tstd::size_t size = 0;\n> +\tstd::size_t alignment = 0;\n> +\n> +\texplicit operator bool() const { return alignment != 0; }\n> +\n> +\tstatic constexpr TypeInfo get(ControlType t)\n> +\t{\n> +\t\tswitch (t) {\n> +\t\tcase ControlTypeNone: return {};\n> +\t\tcase ControlTypeBool: return { sizeof(bool), alignof(bool) };\n> +\t\tcase ControlTypeByte: return { sizeof(uint8_t), alignof(uint8_t) };\n> +\t\tcase ControlTypeUnsigned16: return { sizeof(uint16_t), alignof(uint16_t) };\n> +\t\tcase ControlTypeUnsigned32: return { sizeof(uint32_t), alignof(uint32_t) };\n> +\t\tcase ControlTypeInteger32: return { sizeof(int32_t), alignof(int32_t) };\n> +\t\tcase ControlTypeInteger64: return { sizeof(int64_t), alignof(int64_t) };\n> +\t\tcase ControlTypeFloat: return { sizeof(float), alignof(float) };\n> +\t\tcase ControlTypeString: return { sizeof(char), alignof(char) };\n> +\t\tcase ControlTypeRectangle: return { sizeof(Rectangle), alignof(Rectangle) };\n> +\t\tcase ControlTypeSize: return { sizeof(Size), alignof(Size) };\n> +\t\tcase ControlTypePoint: return { sizeof(Point), alignof(Point) };\n> +\t\t}\n\nAs suggested in a previous review can these information be added to\ncontrol_type ?\n\nWould\n\ntemplate<>\nstruct control_type {\n\tstatic constexpr ControlType value = ControlTypeBool;\n\tstatic constexpr std::size_t size = 0;\n\tstatic constexpr std::size_t element_size = sizeof(bool);\n\tstatic constexpr std::size_t alignment = alignof(bool);\n};\n\nWork ?\n\n> +\n> +\t\treturn {};\n> +\t}\n> +};\n> +\n> +} /* libcamera::controls::details */\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index a238141a5..d5a86da5e 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -17,7 +17,7 @@\n> #include \"libcamera/internal/control_validator.h\"\n>\n> /**\n> - * \\file controls.h\n> + * \\file libcamera/controls.h\n\nUnrelated ?\n\n> * \\brief Framework to manage controls related to an object\n> *\n> * A control is a mean to govern or influence the operation of an object, and in\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index edfa9cf58..c8ac7a673 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -16,11 +16,13 @@\n> #include \n>\n> #include \n> +#include \n> #include \n> #include \n>\n> #include \"libcamera/internal/camera.h\"\n> #include \"libcamera/internal/camera_manager.h\"\n> +#include \"libcamera/internal/controls.h\"\n> #include \"libcamera/internal/device_enumerator.h\"\n> #include \"libcamera/internal/media_device.h\"\n> #include \"libcamera/internal/request.h\"\n> @@ -749,6 +751,47 @@ std::string PipelineHandler::configurationFile(const std::string &subdir,\n> \treturn std::string();\n> }\n>\n> +namespace {\n> +\n> +/*\n> + * This is kind of hack. The metadata controls in the \"debug\" namespace\n\nI know, but I think for the time being is the best we can do :(\n\n> + * are forcefully injected into each Camera's MetadataListPlan so that\n> + * they work seamlessly without any additional setup.\n> + *\n> + * The dynamically-sized array-like controls have a maximum capacity\n> + * determined by the magic number below.\n> + */\n> +void extendMetadataPlanWithDebugMetadata(MetadataListPlan& mlp)\n> +{\n> +\tconstexpr std::size_t kDynamicArrayCapacity = 32;\n> +\n> +\tfor (const auto &[id, ctrl] : controls::controls) {\n> +\t\tif (!ctrl->isOutput())\n> +\t\t\tcontinue;\n> +\t\tif (ctrl->vendor() != \"debug\")\n> +\t\t\tcontinue;\n> +\t\tif (mlp.get(id))\n> +\t\t\tcontinue;\n> +\n> +\t\tstd::size_t count = ctrl->size();\n> +\t\tif (count == 0)\n> +\t\t\tcount = 1;\n\nWhy this ? Do you mind a comment unless is something trivial I have\nmissed ?\n\n> +\t\telse if (ctrl->size() == libcamera::dynamic_extent)\n> +\t\t\tcount = kDynamicArrayCapacity;\n> +\n> +\t\tconst auto info = controls::details::TypeInfo::get(ctrl->type());\n> +\t\tif (!info)\n> +\t\t\tcontinue;\n> +\n> +\t\t[[maybe_unused]] bool ok = mlp.set(id,\n> +\t\t\t\t\t\t info.size, info.alignment,\n> +\t\t\t\t\t\t count, ctrl->type(), ctrl->isArray());\n> +\t\tASSERT(ok);\n> +\t}\n> +}\n> +\n> +} /* namespace */\n> +\n> /**\n> * \\brief Register a camera to the camera manager and pipeline handler\n> * \\param[in] camera The camera to be added\n> @@ -794,6 +837,8 @@ void PipelineHandler::registerCamera(std::shared_ptr camera)\n> \tCamera::Private *data = camera->_d();\n> \tdata->properties_.set(properties::SystemDevices, devnums);\n>\n> +\textendMetadataPlanWithDebugMetadata(data->metadataPlan_);\n> +\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> \tmanager_->_d()->addCamera(std::move(camera));\n> }\n>\n> --\n> 2.50.1\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 6CD02BDCC1\n\tfor ;\n\tMon, 28 Jul 2025 10:19:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A294269149;\n\tMon, 28 Jul 2025 12:19:19 +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 184A46904C\n\tfor ;\n\tMon, 28 Jul 2025 12:19:18 +0200 (CEST)","from ideasonboard.com (mob-5-90-139-29.net.vodafone.it\n\t[5.90.139.29])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D5C8342B;\n\tMon, 28 Jul 2025 12:18: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=\"HH7Sjjsj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1753697916;\n\tbh=on5ZQJ2Z1I4s1wwiUW2omlTqtbajhZflYGkMtYMOgcQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HH7Sjjsj6fYDEe2MTFHKHbZKQi1h1a2YuWCgKdWUaUM/HWX0PuDLDPjcTPEXyDZCB\n\tOwDRenD7IKnw3bLUnIMU18FMnnXQuObF2wkCwGMoUuUg77GBn7fQrIDfmXbgQSq01S\n\tcTtiWAfFjhmg0eLFf+3J4RYhq3R7ElxMzM/sH6Gw=","Date":"Mon, 28 Jul 2025 12:19:11 +0200","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v2 19/22] libcamera: pipeline_handler: Inject \"debug\"\n\tmetadata","Message-ID":"","References":"<20250721104622.1550908-1-barnabas.pocze@ideasonboard.com>\n\t<20250721104622.1550908-20-barnabas.pocze@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20250721104622.1550908-20-barnabas.pocze@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":35212,"web_url":"https://patchwork.libcamera.org/comment/35212/","msgid":"<45d11954-3747-4cfc-b2e9-59ef6b996088@ideasonboard.com>","date":"2025-07-28T15:19:50","subject":"Re: [RFC PATCH v2 19/22] libcamera: pipeline_handler: Inject \"debug\"\n\tmetadata","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 07. 28. 12:19 keltezéssel, Jacopo Mondi írta:\n> Hi Barnabás\n> \n> On Mon, Jul 21, 2025 at 12:46:19PM +0200, Barnabás Pőcze wrote:\n>> Inject all metadata controls in the \"debug\" namespace into\n>> the metadata plan of each camera so that they can be used\n>> seamlessly. Dynamically sized array-like controls have a\n>> hard-coded size of 32 elements.\n>>\n>> Additionally, a new type is added for inspecting properties of\n>> a control type at runtime since that was not available previously.\n>>\n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>> include/libcamera/internal/controls.h | 39 +++++++++++++++++++++++\n>> src/libcamera/controls.cpp | 2 +-\n>> src/libcamera/pipeline_handler.cpp | 45 +++++++++++++++++++++++++++\n>> 3 files changed, 85 insertions(+), 1 deletion(-)\n>> create mode 100644 include/libcamera/internal/controls.h\n>>\n>> diff --git a/include/libcamera/internal/controls.h b/include/libcamera/internal/controls.h\n>> new file mode 100644\n>> index 000000000..be3f93e43\n>> --- /dev/null\n>> +++ b/include/libcamera/internal/controls.h\n>> @@ -0,0 +1,39 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2025, Ideas on Board Oy\n>> + */\n>> +\n>> +#pragma once\n>> +\n>> +#include \n>> +\n>> +namespace libcamera::controls::details {\n>> +\n>> +struct TypeInfo {\n>> +\tstd::size_t size = 0;\n>> +\tstd::size_t alignment = 0;\n>> +\n>> +\texplicit operator bool() const { return alignment != 0; }\n>> +\n>> +\tstatic constexpr TypeInfo get(ControlType t)\n>> +\t{\n>> +\t\tswitch (t) {\n>> +\t\tcase ControlTypeNone: return {};\n>> +\t\tcase ControlTypeBool: return { sizeof(bool), alignof(bool) };\n>> +\t\tcase ControlTypeByte: return { sizeof(uint8_t), alignof(uint8_t) };\n>> +\t\tcase ControlTypeUnsigned16: return { sizeof(uint16_t), alignof(uint16_t) };\n>> +\t\tcase ControlTypeUnsigned32: return { sizeof(uint32_t), alignof(uint32_t) };\n>> +\t\tcase ControlTypeInteger32: return { sizeof(int32_t), alignof(int32_t) };\n>> +\t\tcase ControlTypeInteger64: return { sizeof(int64_t), alignof(int64_t) };\n>> +\t\tcase ControlTypeFloat: return { sizeof(float), alignof(float) };\n>> +\t\tcase ControlTypeString: return { sizeof(char), alignof(char) };\n>> +\t\tcase ControlTypeRectangle: return { sizeof(Rectangle), alignof(Rectangle) };\n>> +\t\tcase ControlTypeSize: return { sizeof(Size), alignof(Size) };\n>> +\t\tcase ControlTypePoint: return { sizeof(Point), alignof(Point) };\n>> +\t\t}\n> \n> As suggested in a previous review can these information be added to\n> control_type ?\n> \n> Would\n> \n> template<>\n> struct control_type {\n> \tstatic constexpr ControlType value = ControlTypeBool;\n> \tstatic constexpr std::size_t size = 0;\n> \tstatic constexpr std::size_t element_size = sizeof(bool);\n> \tstatic constexpr std::size_t alignment = alignof(bool);\n> };\n> \n> Work ?\n\nThat could be useful yes, but you still need a function like the above to map\nthe \"runtime\" type to the appropriate values.\n\n\n\n> \n>> +\n>> +\t\treturn {};\n>> +\t}\n>> +};\n>> +\n>> +} /* libcamera::controls::details */\n>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n>> index a238141a5..d5a86da5e 100644\n>> --- a/src/libcamera/controls.cpp\n>> +++ b/src/libcamera/controls.cpp\n>> @@ -17,7 +17,7 @@\n>> #include \"libcamera/internal/control_validator.h\"\n>>\n>> /**\n>> - * \\file controls.h\n>> + * \\file libcamera/controls.h\n> \n> Unrelated ?\n\nI can move it to a separate commit but it is needed. Because now we have\ntwo `controls.h`, and otherwise doxygen will complain.\n\n\n> \n>> * \\brief Framework to manage controls related to an object\n>> *\n>> * A control is a mean to govern or influence the operation of an object, and in\n>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n>> index edfa9cf58..c8ac7a673 100644\n>> --- a/src/libcamera/pipeline_handler.cpp\n>> +++ b/src/libcamera/pipeline_handler.cpp\n>> @@ -16,11 +16,13 @@\n>> #include \n>>\n>> #include \n>> +#include \n>> #include \n>> #include \n>>\n>> #include \"libcamera/internal/camera.h\"\n>> #include \"libcamera/internal/camera_manager.h\"\n>> +#include \"libcamera/internal/controls.h\"\n>> #include \"libcamera/internal/device_enumerator.h\"\n>> #include \"libcamera/internal/media_device.h\"\n>> #include \"libcamera/internal/request.h\"\n>> @@ -749,6 +751,47 @@ std::string PipelineHandler::configurationFile(const std::string &subdir,\n>> \treturn std::string();\n>> }\n>>\n>> +namespace {\n>> +\n>> +/*\n>> + * This is kind of hack. The metadata controls in the \"debug\" namespace\n> \n> I know, but I think for the time being is the best we can do :(\n> \n>> + * are forcefully injected into each Camera's MetadataListPlan so that\n>> + * they work seamlessly without any additional setup.\n>> + *\n>> + * The dynamically-sized array-like controls have a maximum capacity\n>> + * determined by the magic number below.\n>> + */\n>> +void extendMetadataPlanWithDebugMetadata(MetadataListPlan& mlp)\n>> +{\n>> +\tconstexpr std::size_t kDynamicArrayCapacity = 32;\n>> +\n>> +\tfor (const auto &[id, ctrl] : controls::controls) {\n>> +\t\tif (!ctrl->isOutput())\n>> +\t\t\tcontinue;\n>> +\t\tif (ctrl->vendor() != \"debug\")\n>> +\t\t\tcontinue;\n>> +\t\tif (mlp.get(id))\n>> +\t\t\tcontinue;\n>> +\n>> +\t\tstd::size_t count = ctrl->size();\n>> +\t\tif (count == 0)\n>> +\t\t\tcount = 1;\n> \n> Why this ? Do you mind a comment unless is something trivial I have\n> missed ?\n\nNon-array controls have their static size set to 0. See `control_type<...>::size`.\nI can add a comment if you think it deserves one. I suppose the `MetadataListPlan`\ncould be modified somehow to make this clearer.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n>> +\t\telse if (ctrl->size() == libcamera::dynamic_extent)\n>> +\t\t\tcount = kDynamicArrayCapacity;\n>> +\n>> +\t\tconst auto info = controls::details::TypeInfo::get(ctrl->type());\n>> +\t\tif (!info)\n>> +\t\t\tcontinue;\n>> +\n>> +\t\t[[maybe_unused]] bool ok = mlp.set(id,\n>> +\t\t\t\t\t\t info.size, info.alignment,\n>> +\t\t\t\t\t\t count, ctrl->type(), ctrl->isArray());\n>> +\t\tASSERT(ok);\n>> +\t}\n>> +}\n>> +\n>> +} /* namespace */\n>> +\n>> /**\n>> * \\brief Register a camera to the camera manager and pipeline handler\n>> * \\param[in] camera The camera to be added\n>> @@ -794,6 +837,8 @@ void PipelineHandler::registerCamera(std::shared_ptr camera)\n>> \tCamera::Private *data = camera->_d();\n>> \tdata->properties_.set(properties::SystemDevices, devnums);\n>>\n>> +\textendMetadataPlanWithDebugMetadata(data->metadataPlan_);\n>> +\n> \n> Reviewed-by: Jacopo Mondi \n> \n> Thanks\n> j\n> \n>> \tmanager_->_d()->addCamera(std::move(camera));\n>> }\n>>\n>> --\n>> 2.50.1\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 76F49C3237\n\tfor ;\n\tMon, 28 Jul 2025 15:19:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2E3556919D;\n\tMon, 28 Jul 2025 17:19:56 +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 60E9F69158\n\tfor ;\n\tMon, 28 Jul 2025 17:19:54 +0200 (CEST)","from [192.168.33.18] (185.221.140.39.nat.pool.zt.hu\n\t[185.221.140.39])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EC7D742B;\n\tMon, 28 Jul 2025 17:19:11 +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=\"cKFW+JQa\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1753715952;\n\tbh=exz+UWRlhEFM7wRt9T9vJyI3POlehvGL8RBym0GgrEo=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=cKFW+JQaPgR8Y4tJmryL/uATDy/dbQSKv9L+PE+QK/7F1+SC5iEUFmHuYM3NOjroD\n\tvpwGNBrDxh85FC4wNZ3aTfobofIoDQ37Pdh5GUvBM5GeDxY6QXtct6Jt/y0ni8WF7B\n\twsBzXYvIhbJ6AOAEFAgcC7mRXyn1bxzl7ePbMu7U=","Message-ID":"<45d11954-3747-4cfc-b2e9-59ef6b996088@ideasonboard.com>","Date":"Mon, 28 Jul 2025 17:19:50 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v2 19/22] libcamera: pipeline_handler: Inject \"debug\"\n\tmetadata","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250721104622.1550908-1-barnabas.pocze@ideasonboard.com>\n\t<20250721104622.1550908-20-barnabas.pocze@ideasonboard.com>\n\t","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":35219,"web_url":"https://patchwork.libcamera.org/comment/35219/","msgid":"","date":"2025-07-28T16:02:21","subject":"Re: [RFC PATCH v2 19/22] libcamera: pipeline_handler: Inject \"debug\"\n\tmetadata","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n\nOn Mon, Jul 28, 2025 at 05:19:50PM +0200, Barnabás Pőcze wrote:\n> Hi\n>\n> 2025. 07. 28. 12:19 keltezéssel, Jacopo Mondi írta:\n> > Hi Barnabás\n> >\n> > On Mon, Jul 21, 2025 at 12:46:19PM +0200, Barnabás Pőcze wrote:\n> > > Inject all metadata controls in the \"debug\" namespace into\n> > > the metadata plan of each camera so that they can be used\n> > > seamlessly. Dynamically sized array-like controls have a\n> > > hard-coded size of 32 elements.\n> > >\n> > > Additionally, a new type is added for inspecting properties of\n> > > a control type at runtime since that was not available previously.\n> > >\n> > > Signed-off-by: Barnabás Pőcze \n> > > ---\n> > > include/libcamera/internal/controls.h | 39 +++++++++++++++++++++++\n> > > src/libcamera/controls.cpp | 2 +-\n> > > src/libcamera/pipeline_handler.cpp | 45 +++++++++++++++++++++++++++\n> > > 3 files changed, 85 insertions(+), 1 deletion(-)\n> > > create mode 100644 include/libcamera/internal/controls.h\n> > >\n> > > diff --git a/include/libcamera/internal/controls.h b/include/libcamera/internal/controls.h\n> > > new file mode 100644\n> > > index 000000000..be3f93e43\n> > > --- /dev/null\n> > > +++ b/include/libcamera/internal/controls.h\n> > > @@ -0,0 +1,39 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2025, Ideas on Board Oy\n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include \n> > > +\n> > > +namespace libcamera::controls::details {\n> > > +\n> > > +struct TypeInfo {\n> > > +\tstd::size_t size = 0;\n> > > +\tstd::size_t alignment = 0;\n> > > +\n> > > +\texplicit operator bool() const { return alignment != 0; }\n> > > +\n> > > +\tstatic constexpr TypeInfo get(ControlType t)\n> > > +\t{\n> > > +\t\tswitch (t) {\n> > > +\t\tcase ControlTypeNone: return {};\n> > > +\t\tcase ControlTypeBool: return { sizeof(bool), alignof(bool) };\n> > > +\t\tcase ControlTypeByte: return { sizeof(uint8_t), alignof(uint8_t) };\n> > > +\t\tcase ControlTypeUnsigned16: return { sizeof(uint16_t), alignof(uint16_t) };\n> > > +\t\tcase ControlTypeUnsigned32: return { sizeof(uint32_t), alignof(uint32_t) };\n> > > +\t\tcase ControlTypeInteger32: return { sizeof(int32_t), alignof(int32_t) };\n> > > +\t\tcase ControlTypeInteger64: return { sizeof(int64_t), alignof(int64_t) };\n> > > +\t\tcase ControlTypeFloat: return { sizeof(float), alignof(float) };\n> > > +\t\tcase ControlTypeString: return { sizeof(char), alignof(char) };\n> > > +\t\tcase ControlTypeRectangle: return { sizeof(Rectangle), alignof(Rectangle) };\n> > > +\t\tcase ControlTypeSize: return { sizeof(Size), alignof(Size) };\n> > > +\t\tcase ControlTypePoint: return { sizeof(Point), alignof(Point) };\n> > > +\t\t}\n> >\n> > As suggested in a previous review can these information be added to\n> > control_type ?\n> >\n> > Would\n> >\n> > template<>\n> > struct control_type {\n> > \tstatic constexpr ControlType value = ControlTypeBool;\n> > \tstatic constexpr std::size_t size = 0;\n> > \tstatic constexpr std::size_t element_size = sizeof(bool);\n> > \tstatic constexpr std::size_t alignment = alignof(bool);\n> > };\n> >\n> > Work ?\n>\n> That could be useful yes, but you still need a function like the above to map\n> the \"runtime\" type to the appropriate values.\n\nIf we use ctrl->type() yes, we need a run-time switch.\n\nI presume there is no easy way to go from Control to\ncontrol_info when iterating over the controls::controls id map..\n\n>\n>\n>\n> >\n> > > +\n> > > +\t\treturn {};\n> > > +\t}\n> > > +};\n> > > +\n> > > +} /* libcamera::controls::details */\n> > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > > index a238141a5..d5a86da5e 100644\n> > > --- a/src/libcamera/controls.cpp\n> > > +++ b/src/libcamera/controls.cpp\n> > > @@ -17,7 +17,7 @@\n> > > #include \"libcamera/internal/control_validator.h\"\n> > >\n> > > /**\n> > > - * \\file controls.h\n> > > + * \\file libcamera/controls.h\n> >\n> > Unrelated ?\n>\n> I can move it to a separate commit but it is needed. Because now we have\n> two `controls.h`, and otherwise doxygen will complain.\n>\n\nAh sorry, I missed the reason and I thought it was just a leftover\n\n>\n> >\n> > > * \\brief Framework to manage controls related to an object\n> > > *\n> > > * A control is a mean to govern or influence the operation of an object, and in\n> > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > index edfa9cf58..c8ac7a673 100644\n> > > --- a/src/libcamera/pipeline_handler.cpp\n> > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > @@ -16,11 +16,13 @@\n> > > #include \n> > >\n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > >\n> > > #include \"libcamera/internal/camera.h\"\n> > > #include \"libcamera/internal/camera_manager.h\"\n> > > +#include \"libcamera/internal/controls.h\"\n> > > #include \"libcamera/internal/device_enumerator.h\"\n> > > #include \"libcamera/internal/media_device.h\"\n> > > #include \"libcamera/internal/request.h\"\n> > > @@ -749,6 +751,47 @@ std::string PipelineHandler::configurationFile(const std::string &subdir,\n> > > \treturn std::string();\n> > > }\n> > >\n> > > +namespace {\n> > > +\n> > > +/*\n> > > + * This is kind of hack. The metadata controls in the \"debug\" namespace\n> >\n> > I know, but I think for the time being is the best we can do :(\n> >\n> > > + * are forcefully injected into each Camera's MetadataListPlan so that\n> > > + * they work seamlessly without any additional setup.\n> > > + *\n> > > + * The dynamically-sized array-like controls have a maximum capacity\n> > > + * determined by the magic number below.\n> > > + */\n> > > +void extendMetadataPlanWithDebugMetadata(MetadataListPlan& mlp)\n> > > +{\n> > > +\tconstexpr std::size_t kDynamicArrayCapacity = 32;\n> > > +\n> > > +\tfor (const auto &[id, ctrl] : controls::controls) {\n> > > +\t\tif (!ctrl->isOutput())\n> > > +\t\t\tcontinue;\n> > > +\t\tif (ctrl->vendor() != \"debug\")\n> > > +\t\t\tcontinue;\n> > > +\t\tif (mlp.get(id))\n> > > +\t\t\tcontinue;\n> > > +\n> > > +\t\tstd::size_t count = ctrl->size();\n> > > +\t\tif (count == 0)\n> > > +\t\t\tcount = 1;\n> >\n> > Why this ? Do you mind a comment unless is something trivial I have\n> > missed ?\n>\n> Non-array controls have their static size set to 0. See `control_type<...>::size`.\n> I can add a comment if you think it deserves one. I suppose the `MetadataListPlan`\n> could be modified somehow to make this clearer.\n\nack, a small comment would make it clear\n\n>\n>\n> Regards,\n> Barnabás Pőcze\n>\n>\n> >\n> > > +\t\telse if (ctrl->size() == libcamera::dynamic_extent)\n> > > +\t\t\tcount = kDynamicArrayCapacity;\n> > > +\n> > > +\t\tconst auto info = controls::details::TypeInfo::get(ctrl->type());\n> > > +\t\tif (!info)\n> > > +\t\t\tcontinue;\n> > > +\n> > > +\t\t[[maybe_unused]] bool ok = mlp.set(id,\n> > > +\t\t\t\t\t\t info.size, info.alignment,\n> > > +\t\t\t\t\t\t count, ctrl->type(), ctrl->isArray());\n> > > +\t\tASSERT(ok);\n> > > +\t}\n> > > +}\n> > > +\n> > > +} /* namespace */\n> > > +\n> > > /**\n> > > * \\brief Register a camera to the camera manager and pipeline handler\n> > > * \\param[in] camera The camera to be added\n> > > @@ -794,6 +837,8 @@ void PipelineHandler::registerCamera(std::shared_ptr camera)\n> > > \tCamera::Private *data = camera->_d();\n> > > \tdata->properties_.set(properties::SystemDevices, devnums);\n> > >\n> > > +\textendMetadataPlanWithDebugMetadata(data->metadataPlan_);\n> > > +\n> >\n> > Reviewed-by: Jacopo Mondi \n> >\n> > Thanks\n> > j\n> >\n> > > \tmanager_->_d()->addCamera(std::move(camera));\n> > > }\n> > >\n> > > --\n> > > 2.50.1\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 B68F6C3237\n\tfor ;\n\tMon, 28 Jul 2025 16:02:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 87530691C1;\n\tMon, 28 Jul 2025 18:02:28 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E3A4D69158\n\tfor ;\n\tMon, 28 Jul 2025 18:02:25 +0200 (CEST)","from ideasonboard.com (mob-5-90-139-29.net.vodafone.it\n\t[5.90.139.29])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 32B9F42B;\n\tMon, 28 Jul 2025 18:01: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=\"tlH3ay4F\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1753718503;\n\tbh=GQmOzG1O9z46dzzsvLrlEfcieo6VNAvDtNodpDd/bZU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tlH3ay4Fy5rUF9ssQNVL+b/GsqqkygLVODKNUk6fwY+FyVLYt/Wsgkc4dfb/CNrc4\n\ta8B2cZb11wg2xOhugK8/62I97WJHCF6gk/DaHZbqRq7JFTLeeRECG84WPJQIxVFAX3\n\tvjqwgKE9wRNdvJIy2OmiEUtv7pQUpSzqOMA62p64=","Date":"Mon, 28 Jul 2025 18:02:21 +0200","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v2 19/22] libcamera: pipeline_handler: Inject \"debug\"\n\tmetadata","Message-ID":"","References":"<20250721104622.1550908-1-barnabas.pocze@ideasonboard.com>\n\t<20250721104622.1550908-20-barnabas.pocze@ideasonboard.com>\n\t\n\t<45d11954-3747-4cfc-b2e9-59ef6b996088@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<45d11954-3747-4cfc-b2e9-59ef6b996088@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":35252,"web_url":"https://patchwork.libcamera.org/comment/35252/","msgid":"","date":"2025-07-30T14:49:13","subject":"Re: [RFC PATCH v2 19/22] libcamera: pipeline_handler: Inject \"debug\"\n\tmetadata","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 07. 28. 18:02 keltezéssel, Jacopo Mondi írta:\n> Hi Barnabás\n> \n> On Mon, Jul 28, 2025 at 05:19:50PM +0200, Barnabás Pőcze wrote:\n>> Hi\n>>\n>> 2025. 07. 28. 12:19 keltezéssel, Jacopo Mondi írta:\n>>> Hi Barnabás\n>>>\n>>> On Mon, Jul 21, 2025 at 12:46:19PM +0200, Barnabás Pőcze wrote:\n>>>> Inject all metadata controls in the \"debug\" namespace into\n>>>> the metadata plan of each camera so that they can be used\n>>>> seamlessly. Dynamically sized array-like controls have a\n>>>> hard-coded size of 32 elements.\n>>>>\n>>>> Additionally, a new type is added for inspecting properties of\n>>>> a control type at runtime since that was not available previously.\n>>>>\n>>>> Signed-off-by: Barnabás Pőcze \n>>>> ---\n>>>> include/libcamera/internal/controls.h | 39 +++++++++++++++++++++++\n>>>> src/libcamera/controls.cpp | 2 +-\n>>>> src/libcamera/pipeline_handler.cpp | 45 +++++++++++++++++++++++++++\n>>>> 3 files changed, 85 insertions(+), 1 deletion(-)\n>>>> create mode 100644 include/libcamera/internal/controls.h\n>>>>\n>>>> diff --git a/include/libcamera/internal/controls.h b/include/libcamera/internal/controls.h\n>>>> new file mode 100644\n>>>> index 000000000..be3f93e43\n>>>> --- /dev/null\n>>>> +++ b/include/libcamera/internal/controls.h\n>>>> @@ -0,0 +1,39 @@\n>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>>> +/*\n>>>> + * Copyright (C) 2025, Ideas on Board Oy\n>>>> + */\n>>>> +\n>>>> +#pragma once\n>>>> +\n>>>> +#include \n>>>> +\n>>>> +namespace libcamera::controls::details {\n>>>> +\n>>>> +struct TypeInfo {\n>>>> +\tstd::size_t size = 0;\n>>>> +\tstd::size_t alignment = 0;\n>>>> +\n>>>> +\texplicit operator bool() const { return alignment != 0; }\n>>>> +\n>>>> +\tstatic constexpr TypeInfo get(ControlType t)\n>>>> +\t{\n>>>> +\t\tswitch (t) {\n>>>> +\t\tcase ControlTypeNone: return {};\n>>>> +\t\tcase ControlTypeBool: return { sizeof(bool), alignof(bool) };\n>>>> +\t\tcase ControlTypeByte: return { sizeof(uint8_t), alignof(uint8_t) };\n>>>> +\t\tcase ControlTypeUnsigned16: return { sizeof(uint16_t), alignof(uint16_t) };\n>>>> +\t\tcase ControlTypeUnsigned32: return { sizeof(uint32_t), alignof(uint32_t) };\n>>>> +\t\tcase ControlTypeInteger32: return { sizeof(int32_t), alignof(int32_t) };\n>>>> +\t\tcase ControlTypeInteger64: return { sizeof(int64_t), alignof(int64_t) };\n>>>> +\t\tcase ControlTypeFloat: return { sizeof(float), alignof(float) };\n>>>> +\t\tcase ControlTypeString: return { sizeof(char), alignof(char) };\n>>>> +\t\tcase ControlTypeRectangle: return { sizeof(Rectangle), alignof(Rectangle) };\n>>>> +\t\tcase ControlTypeSize: return { sizeof(Size), alignof(Size) };\n>>>> +\t\tcase ControlTypePoint: return { sizeof(Point), alignof(Point) };\n>>>> +\t\t}\n>>>\n>>> As suggested in a previous review can these information be added to\n>>> control_type ?\n>>>\n>>> Would\n>>>\n>>> template<>\n>>> struct control_type {\n>>> \tstatic constexpr ControlType value = ControlTypeBool;\n>>> \tstatic constexpr std::size_t size = 0;\n>>> \tstatic constexpr std::size_t element_size = sizeof(bool);\n>>> \tstatic constexpr std::size_t alignment = alignof(bool);\n>>> };\n>>>\n>>> Work ?\n>>\n>> That could be useful yes, but you still need a function like the above to map\n>> the \"runtime\" type to the appropriate values.\n> \n> If we use ctrl->type() yes, we need a run-time switch.\n> \n> I presume there is no easy way to go from Control to\n> control_info when iterating over the controls::controls id map..\n> \n\nOnly a `ControlId` is available, so there is no static type information\navailable. Some kind of runtime switch is needed. This seemed like the\nsimplest method.\n\n\n>>\n>>\n>>\n>>>\n>>>> +\n>>>> +\t\treturn {};\n>>>> +\t}\n>>>> +};\n>>>> +\n>>>> +} /* libcamera::controls::details */\n>>>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n>>>> index a238141a5..d5a86da5e 100644\n>>>> --- a/src/libcamera/controls.cpp\n>>>> +++ b/src/libcamera/controls.cpp\n>>>> @@ -17,7 +17,7 @@\n>>>> #include \"libcamera/internal/control_validator.h\"\n>>>>\n>>>> /**\n>>>> - * \\file controls.h\n>>>> + * \\file libcamera/controls.h\n>>>\n>>> Unrelated ?\n>>\n>> I can move it to a separate commit but it is needed. Because now we have\n>> two `controls.h`, and otherwise doxygen will complain.\n>>\n> \n> Ah sorry, I missed the reason and I thought it was just a leftover\n> \n>>\n>>>\n>>>> * \\brief Framework to manage controls related to an object\n>>>> *\n>>>> * A control is a mean to govern or influence the operation of an object, and in\n>>>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n>>>> index edfa9cf58..c8ac7a673 100644\n>>>> --- a/src/libcamera/pipeline_handler.cpp\n>>>> +++ b/src/libcamera/pipeline_handler.cpp\n>>>> @@ -16,11 +16,13 @@\n>>>> #include \n>>>>\n>>>> #include \n>>>> +#include \n>>>> #include \n>>>> #include \n>>>>\n>>>> #include \"libcamera/internal/camera.h\"\n>>>> #include \"libcamera/internal/camera_manager.h\"\n>>>> +#include \"libcamera/internal/controls.h\"\n>>>> #include \"libcamera/internal/device_enumerator.h\"\n>>>> #include \"libcamera/internal/media_device.h\"\n>>>> #include \"libcamera/internal/request.h\"\n>>>> @@ -749,6 +751,47 @@ std::string PipelineHandler::configurationFile(const std::string &subdir,\n>>>> \treturn std::string();\n>>>> }\n>>>>\n>>>> +namespace {\n>>>> +\n>>>> +/*\n>>>> + * This is kind of hack. The metadata controls in the \"debug\" namespace\n>>>\n>>> I know, but I think for the time being is the best we can do :(\n>>>\n>>>> + * are forcefully injected into each Camera's MetadataListPlan so that\n>>>> + * they work seamlessly without any additional setup.\n>>>> + *\n>>>> + * The dynamically-sized array-like controls have a maximum capacity\n>>>> + * determined by the magic number below.\n>>>> + */\n>>>> +void extendMetadataPlanWithDebugMetadata(MetadataListPlan& mlp)\n>>>> +{\n>>>> +\tconstexpr std::size_t kDynamicArrayCapacity = 32;\n>>>> +\n>>>> +\tfor (const auto &[id, ctrl] : controls::controls) {\n>>>> +\t\tif (!ctrl->isOutput())\n>>>> +\t\t\tcontinue;\n>>>> +\t\tif (ctrl->vendor() != \"debug\")\n>>>> +\t\t\tcontinue;\n>>>> +\t\tif (mlp.get(id))\n>>>> +\t\t\tcontinue;\n>>>> +\n>>>> +\t\tstd::size_t count = ctrl->size();\n>>>> +\t\tif (count == 0)\n>>>> +\t\t\tcount = 1;\n>>>\n>>> Why this ? Do you mind a comment unless is something trivial I have\n>>> missed ?\n>>\n>> Non-array controls have their static size set to 0. See `control_type<...>::size`.\n>> I can add a comment if you think it deserves one. I suppose the `MetadataListPlan`\n>> could be modified somehow to make this clearer.\n> \n> ack, a small comment would make it clear\n\ndone\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n>>\n>>\n>> Regards,\n>> Barnabás Pőcze\n>>\n>>\n>>>\n>>>> +\t\telse if (ctrl->size() == libcamera::dynamic_extent)\n>>>> +\t\t\tcount = kDynamicArrayCapacity;\n>>>> +\n>>>> +\t\tconst auto info = controls::details::TypeInfo::get(ctrl->type());\n>>>> +\t\tif (!info)\n>>>> +\t\t\tcontinue;\n>>>> +\n>>>> +\t\t[[maybe_unused]] bool ok = mlp.set(id,\n>>>> +\t\t\t\t\t\t info.size, info.alignment,\n>>>> +\t\t\t\t\t\t count, ctrl->type(), ctrl->isArray());\n>>>> +\t\tASSERT(ok);\n>>>> +\t}\n>>>> +}\n>>>> +\n>>>> +} /* namespace */\n>>>> +\n>>>> /**\n>>>> * \\brief Register a camera to the camera manager and pipeline handler\n>>>> * \\param[in] camera The camera to be added\n>>>> @@ -794,6 +837,8 @@ void PipelineHandler::registerCamera(std::shared_ptr camera)\n>>>> \tCamera::Private *data = camera->_d();\n>>>> \tdata->properties_.set(properties::SystemDevices, devnums);\n>>>>\n>>>> +\textendMetadataPlanWithDebugMetadata(data->metadataPlan_);\n>>>> +\n>>>\n>>> Reviewed-by: Jacopo Mondi \n>>>\n>>> Thanks\n>>> j\n>>>\n>>>> \tmanager_->_d()->addCamera(std::move(camera));\n>>>> }\n>>>>\n>>>> --\n>>>> 2.50.1\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 8C3F7BDC71\n\tfor ;\n\tWed, 30 Jul 2025 14:49:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4E55D69210;\n\tWed, 30 Jul 2025 16:49:19 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 72E7A69205\n\tfor ;\n\tWed, 30 Jul 2025 16:49:17 +0200 (CEST)","from [192.168.33.11] (185.182.214.105.nat.pool.zt.hu\n\t[185.182.214.105])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F1CEEC7A;\n\tWed, 30 Jul 2025 16:48: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=\"X3H8UWcF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1753886914;\n\tbh=zHv6zp2OTTWdyWs2iPF1f3OricvHKNC0QL6L0LZeTgo=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=X3H8UWcFpOzm+/Mdvjvp0NFi8loVqFjC5VyYnKA3N/Egi871eDIbSKSCmbGxynPuQ\n\t3yiYR+oUwviU9Gfayep1y6B3Qb8e7zgUIHdqSBbngGszkVFI3ZtIG9ilqqD03vYq6h\n\tEsI3qwmpgcjB18jWqFEmm3XPrBdKUjTZ2xK+MdBc=","Message-ID":"","Date":"Wed, 30 Jul 2025 16:49:13 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v2 19/22] libcamera: pipeline_handler: Inject \"debug\"\n\tmetadata","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250721104622.1550908-1-barnabas.pocze@ideasonboard.com>\n\t<20250721104622.1550908-20-barnabas.pocze@ideasonboard.com>\n\t\n\t<45d11954-3747-4cfc-b2e9-59ef6b996088@ideasonboard.com>\n\t","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":35263,"web_url":"https://patchwork.libcamera.org/comment/35263/","msgid":"","date":"2025-08-02T10:36:37","subject":"Re: [RFC PATCH v2 19/22] libcamera: pipeline_handler: Inject \"debug\"\n\tmetadata","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n\nOn Wed, Jul 30, 2025 at 04:49:13PM +0200, Barnabás Pőcze wrote:\n> Hi\n>\n> 2025. 07. 28. 18:02 keltezéssel, Jacopo Mondi írta:\n> > Hi Barnabás\n> >\n> > On Mon, Jul 28, 2025 at 05:19:50PM +0200, Barnabás Pőcze wrote:\n> > > Hi\n> > >\n> > > 2025. 07. 28. 12:19 keltezéssel, Jacopo Mondi írta:\n> > > > Hi Barnabás\n> > > >\n> > > > On Mon, Jul 21, 2025 at 12:46:19PM +0200, Barnabás Pőcze wrote:\n> > > > > Inject all metadata controls in the \"debug\" namespace into\n> > > > > the metadata plan of each camera so that they can be used\n> > > > > seamlessly. Dynamically sized array-like controls have a\n> > > > > hard-coded size of 32 elements.\n> > > > >\n> > > > > Additionally, a new type is added for inspecting properties of\n> > > > > a control type at runtime since that was not available previously.\n> > > > >\n> > > > > Signed-off-by: Barnabás Pőcze \n> > > > > ---\n> > > > > include/libcamera/internal/controls.h | 39 +++++++++++++++++++++++\n> > > > > src/libcamera/controls.cpp | 2 +-\n> > > > > src/libcamera/pipeline_handler.cpp | 45 +++++++++++++++++++++++++++\n> > > > > 3 files changed, 85 insertions(+), 1 deletion(-)\n> > > > > create mode 100644 include/libcamera/internal/controls.h\n> > > > >\n> > > > > diff --git a/include/libcamera/internal/controls.h b/include/libcamera/internal/controls.h\n> > > > > new file mode 100644\n> > > > > index 000000000..be3f93e43\n> > > > > --- /dev/null\n> > > > > +++ b/include/libcamera/internal/controls.h\n> > > > > @@ -0,0 +1,39 @@\n> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2025, Ideas on Board Oy\n> > > > > + */\n> > > > > +\n> > > > > +#pragma once\n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +namespace libcamera::controls::details {\n> > > > > +\n> > > > > +struct TypeInfo {\n> > > > > +\tstd::size_t size = 0;\n> > > > > +\tstd::size_t alignment = 0;\n> > > > > +\n> > > > > +\texplicit operator bool() const { return alignment != 0; }\n> > > > > +\n> > > > > +\tstatic constexpr TypeInfo get(ControlType t)\n> > > > > +\t{\n> > > > > +\t\tswitch (t) {\n> > > > > +\t\tcase ControlTypeNone: return {};\n> > > > > +\t\tcase ControlTypeBool: return { sizeof(bool), alignof(bool) };\n> > > > > +\t\tcase ControlTypeByte: return { sizeof(uint8_t), alignof(uint8_t) };\n> > > > > +\t\tcase ControlTypeUnsigned16: return { sizeof(uint16_t), alignof(uint16_t) };\n> > > > > +\t\tcase ControlTypeUnsigned32: return { sizeof(uint32_t), alignof(uint32_t) };\n> > > > > +\t\tcase ControlTypeInteger32: return { sizeof(int32_t), alignof(int32_t) };\n> > > > > +\t\tcase ControlTypeInteger64: return { sizeof(int64_t), alignof(int64_t) };\n> > > > > +\t\tcase ControlTypeFloat: return { sizeof(float), alignof(float) };\n> > > > > +\t\tcase ControlTypeString: return { sizeof(char), alignof(char) };\n> > > > > +\t\tcase ControlTypeRectangle: return { sizeof(Rectangle), alignof(Rectangle) };\n> > > > > +\t\tcase ControlTypeSize: return { sizeof(Size), alignof(Size) };\n> > > > > +\t\tcase ControlTypePoint: return { sizeof(Point), alignof(Point) };\n> > > > > +\t\t}\n> > > >\n> > > > As suggested in a previous review can these information be added to\n> > > > control_type ?\n> > > >\n> > > > Would\n> > > >\n> > > > template<>\n> > > > struct control_type {\n> > > > \tstatic constexpr ControlType value = ControlTypeBool;\n> > > > \tstatic constexpr std::size_t size = 0;\n> > > > \tstatic constexpr std::size_t element_size = sizeof(bool);\n> > > > \tstatic constexpr std::size_t alignment = alignof(bool);\n> > > > };\n> > > >\n> > > > Work ?\n> > >\n> > > That could be useful yes, but you still need a function like the above to map\n> > > the \"runtime\" type to the appropriate values.\n> >\n> > If we use ctrl->type() yes, we need a run-time switch.\n> >\n> > I presume there is no easy way to go from Control to\n> > control_info when iterating over the controls::controls id map..\n> >\n>\n> Only a `ControlId` is available, so there is no static type information\n> available. Some kind of runtime switch is needed. This seemed like the\n> simplest method.\n\nIndeed.\n\nDo you think it would make sense to centralize the information in the\nexisting control_type and add a function similar in spirit to your\nabove TypeInfo::get() to controls.h or would you prefer keeping them\nseparate ?\n\n\n>\n>\n> > >\n> > >\n> > >\n> > > >\n> > > > > +\n> > > > > +\t\treturn {};\n> > > > > +\t}\n> > > > > +};\n> > > > > +\n> > > > > +} /* libcamera::controls::details */\n> > > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > > > > index a238141a5..d5a86da5e 100644\n> > > > > --- a/src/libcamera/controls.cpp\n> > > > > +++ b/src/libcamera/controls.cpp\n> > > > > @@ -17,7 +17,7 @@\n> > > > > #include \"libcamera/internal/control_validator.h\"\n> > > > >\n> > > > > /**\n> > > > > - * \\file controls.h\n> > > > > + * \\file libcamera/controls.h\n> > > >\n> > > > Unrelated ?\n> > >\n> > > I can move it to a separate commit but it is needed. Because now we have\n> > > two `controls.h`, and otherwise doxygen will complain.\n> > >\n> >\n> > Ah sorry, I missed the reason and I thought it was just a leftover\n> >\n> > >\n> > > >\n> > > > > * \\brief Framework to manage controls related to an object\n> > > > > *\n> > > > > * A control is a mean to govern or influence the operation of an object, and in\n> > > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > > > index edfa9cf58..c8ac7a673 100644\n> > > > > --- a/src/libcamera/pipeline_handler.cpp\n> > > > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > > > @@ -16,11 +16,13 @@\n> > > > > #include \n> > > > >\n> > > > > #include \n> > > > > +#include \n> > > > > #include \n> > > > > #include \n> > > > >\n> > > > > #include \"libcamera/internal/camera.h\"\n> > > > > #include \"libcamera/internal/camera_manager.h\"\n> > > > > +#include \"libcamera/internal/controls.h\"\n> > > > > #include \"libcamera/internal/device_enumerator.h\"\n> > > > > #include \"libcamera/internal/media_device.h\"\n> > > > > #include \"libcamera/internal/request.h\"\n> > > > > @@ -749,6 +751,47 @@ std::string PipelineHandler::configurationFile(const std::string &subdir,\n> > > > > \treturn std::string();\n> > > > > }\n> > > > >\n> > > > > +namespace {\n> > > > > +\n> > > > > +/*\n> > > > > + * This is kind of hack. The metadata controls in the \"debug\" namespace\n> > > >\n> > > > I know, but I think for the time being is the best we can do :(\n> > > >\n> > > > > + * are forcefully injected into each Camera's MetadataListPlan so that\n> > > > > + * they work seamlessly without any additional setup.\n> > > > > + *\n> > > > > + * The dynamically-sized array-like controls have a maximum capacity\n> > > > > + * determined by the magic number below.\n> > > > > + */\n> > > > > +void extendMetadataPlanWithDebugMetadata(MetadataListPlan& mlp)\n> > > > > +{\n> > > > > +\tconstexpr std::size_t kDynamicArrayCapacity = 32;\n> > > > > +\n> > > > > +\tfor (const auto &[id, ctrl] : controls::controls) {\n> > > > > +\t\tif (!ctrl->isOutput())\n> > > > > +\t\t\tcontinue;\n> > > > > +\t\tif (ctrl->vendor() != \"debug\")\n> > > > > +\t\t\tcontinue;\n> > > > > +\t\tif (mlp.get(id))\n> > > > > +\t\t\tcontinue;\n> > > > > +\n> > > > > +\t\tstd::size_t count = ctrl->size();\n> > > > > +\t\tif (count == 0)\n> > > > > +\t\t\tcount = 1;\n> > > >\n> > > > Why this ? Do you mind a comment unless is something trivial I have\n> > > > missed ?\n> > >\n> > > Non-array controls have their static size set to 0. See `control_type<...>::size`.\n> > > I can add a comment if you think it deserves one. I suppose the `MetadataListPlan`\n> > > could be modified somehow to make this clearer.\n> >\n> > ack, a small comment would make it clear\n>\n> done\n>\n>\n> Regards,\n> Barnabás Pőcze\n>\n>\n> >\n> > >\n> > >\n> > > Regards,\n> > > Barnabás Pőcze\n> > >\n> > >\n> > > >\n> > > > > +\t\telse if (ctrl->size() == libcamera::dynamic_extent)\n> > > > > +\t\t\tcount = kDynamicArrayCapacity;\n> > > > > +\n> > > > > +\t\tconst auto info = controls::details::TypeInfo::get(ctrl->type());\n> > > > > +\t\tif (!info)\n> > > > > +\t\t\tcontinue;\n> > > > > +\n> > > > > +\t\t[[maybe_unused]] bool ok = mlp.set(id,\n> > > > > +\t\t\t\t\t\t info.size, info.alignment,\n> > > > > +\t\t\t\t\t\t count, ctrl->type(), ctrl->isArray());\n> > > > > +\t\tASSERT(ok);\n> > > > > +\t}\n> > > > > +}\n> > > > > +\n> > > > > +} /* namespace */\n> > > > > +\n> > > > > /**\n> > > > > * \\brief Register a camera to the camera manager and pipeline handler\n> > > > > * \\param[in] camera The camera to be added\n> > > > > @@ -794,6 +837,8 @@ void PipelineHandler::registerCamera(std::shared_ptr camera)\n> > > > > \tCamera::Private *data = camera->_d();\n> > > > > \tdata->properties_.set(properties::SystemDevices, devnums);\n> > > > >\n> > > > > +\textendMetadataPlanWithDebugMetadata(data->metadataPlan_);\n> > > > > +\n> > > >\n> > > > Reviewed-by: Jacopo Mondi \n> > > >\n> > > > Thanks\n> > > > j\n> > > >\n> > > > > \tmanager_->_d()->addCamera(std::move(camera));\n> > > > > }\n> > > > >\n> > > > > --\n> > > > > 2.50.1\n> > > > >\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 AF42FBDC71\n\tfor ;\n\tSat, 2 Aug 2025 10:36:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0405E69215;\n\tSat, 2 Aug 2025 12:36: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 DD92569137\n\tfor ;\n\tSat, 2 Aug 2025 12:36:40 +0200 (CEST)","from ideasonboard.com (mob-5-90-138-121.net.vodafone.it\n\t[5.90.138.121])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 06B6E3A4;\n\tSat, 2 Aug 2025 12:35: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=\"QJaR2v6X\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1754130955;\n\tbh=xW7sjiaGvPN2YfRyguTa9Jc6QHC+l1avXm0oaZVbvZM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QJaR2v6XT4fc/Ck/kicKXQScVlIOFweAGVMNJDQWLLjNiZAGPLfSnqcvfYm6xMJy1\n\teTD/nBrC7Jr8CoV1Wv+alCVA0A0Iddn+1wSf9izEuY1OCLDlbqbSZSBfay5KN5y8AK\n\t21dydc/qTBgjDNd2KC5UMqTbLLkx7kY0II+skJzs=","Date":"Sat, 2 Aug 2025 12:36:37 +0200","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v2 19/22] libcamera: pipeline_handler: Inject \"debug\"\n\tmetadata","Message-ID":"","References":"<20250721104622.1550908-1-barnabas.pocze@ideasonboard.com>\n\t<20250721104622.1550908-20-barnabas.pocze@ideasonboard.com>\n\t\n\t<45d11954-3747-4cfc-b2e9-59ef6b996088@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","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\" "}},{"id":35264,"web_url":"https://patchwork.libcamera.org/comment/35264/","msgid":"","date":"2025-08-03T11:56:48","subject":"Re: [RFC PATCH v2 19/22] libcamera: pipeline_handler: Inject \"debug\"\n\tmetadata","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"On Sat, Aug 02, 2025 at 12:36:40PM +0200, Jacopo Mondi wrote:\n> Hi Barnabás\n>\n> On Wed, Jul 30, 2025 at 04:49:13PM +0200, Barnabás Pőcze wrote:\n> > Hi\n> >\n> > 2025. 07. 28. 18:02 keltezéssel, Jacopo Mondi írta:\n> > > Hi Barnabás\n> > >\n> > > On Mon, Jul 28, 2025 at 05:19:50PM +0200, Barnabás Pőcze wrote:\n> > > > Hi\n> > > >\n> > > > 2025. 07. 28. 12:19 keltezéssel, Jacopo Mondi írta:\n> > > > > Hi Barnabás\n> > > > >\n> > > > > On Mon, Jul 21, 2025 at 12:46:19PM +0200, Barnabás Pőcze wrote:\n> > > > > > Inject all metadata controls in the \"debug\" namespace into\n> > > > > > the metadata plan of each camera so that they can be used\n> > > > > > seamlessly. Dynamically sized array-like controls have a\n> > > > > > hard-coded size of 32 elements.\n> > > > > >\n> > > > > > Additionally, a new type is added for inspecting properties of\n> > > > > > a control type at runtime since that was not available previously.\n> > > > > >\n> > > > > > Signed-off-by: Barnabás Pőcze \n> > > > > > ---\n> > > > > > include/libcamera/internal/controls.h | 39 +++++++++++++++++++++++\n> > > > > > src/libcamera/controls.cpp | 2 +-\n> > > > > > src/libcamera/pipeline_handler.cpp | 45 +++++++++++++++++++++++++++\n> > > > > > 3 files changed, 85 insertions(+), 1 deletion(-)\n> > > > > > create mode 100644 include/libcamera/internal/controls.h\n> > > > > >\n> > > > > > diff --git a/include/libcamera/internal/controls.h b/include/libcamera/internal/controls.h\n> > > > > > new file mode 100644\n> > > > > > index 000000000..be3f93e43\n> > > > > > --- /dev/null\n> > > > > > +++ b/include/libcamera/internal/controls.h\n> > > > > > @@ -0,0 +1,39 @@\n> > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > > +/*\n> > > > > > + * Copyright (C) 2025, Ideas on Board Oy\n> > > > > > + */\n> > > > > > +\n> > > > > > +#pragma once\n> > > > > > +\n> > > > > > +#include \n> > > > > > +\n> > > > > > +namespace libcamera::controls::details {\n> > > > > > +\n> > > > > > +struct TypeInfo {\n> > > > > > +\tstd::size_t size = 0;\n> > > > > > +\tstd::size_t alignment = 0;\n> > > > > > +\n> > > > > > +\texplicit operator bool() const { return alignment != 0; }\n> > > > > > +\n> > > > > > +\tstatic constexpr TypeInfo get(ControlType t)\n> > > > > > +\t{\n> > > > > > +\t\tswitch (t) {\n> > > > > > +\t\tcase ControlTypeNone: return {};\n> > > > > > +\t\tcase ControlTypeBool: return { sizeof(bool), alignof(bool) };\n> > > > > > +\t\tcase ControlTypeByte: return { sizeof(uint8_t), alignof(uint8_t) };\n> > > > > > +\t\tcase ControlTypeUnsigned16: return { sizeof(uint16_t), alignof(uint16_t) };\n> > > > > > +\t\tcase ControlTypeUnsigned32: return { sizeof(uint32_t), alignof(uint32_t) };\n> > > > > > +\t\tcase ControlTypeInteger32: return { sizeof(int32_t), alignof(int32_t) };\n> > > > > > +\t\tcase ControlTypeInteger64: return { sizeof(int64_t), alignof(int64_t) };\n> > > > > > +\t\tcase ControlTypeFloat: return { sizeof(float), alignof(float) };\n> > > > > > +\t\tcase ControlTypeString: return { sizeof(char), alignof(char) };\n> > > > > > +\t\tcase ControlTypeRectangle: return { sizeof(Rectangle), alignof(Rectangle) };\n> > > > > > +\t\tcase ControlTypeSize: return { sizeof(Size), alignof(Size) };\n> > > > > > +\t\tcase ControlTypePoint: return { sizeof(Point), alignof(Point) };\n> > > > > > +\t\t}\n> > > > >\n> > > > > As suggested in a previous review can these information be added to\n> > > > > control_type ?\n> > > > >\n> > > > > Would\n> > > > >\n> > > > > template<>\n> > > > > struct control_type {\n> > > > > \tstatic constexpr ControlType value = ControlTypeBool;\n> > > > > \tstatic constexpr std::size_t size = 0;\n> > > > > \tstatic constexpr std::size_t element_size = sizeof(bool);\n> > > > > \tstatic constexpr std::size_t alignment = alignof(bool);\n> > > > > };\n> > > > >\n> > > > > Work ?\n> > > >\n> > > > That could be useful yes, but you still need a function like the above to map\n> > > > the \"runtime\" type to the appropriate values.\n> > >\n> > > If we use ctrl->type() yes, we need a run-time switch.\n> > >\n> > > I presume there is no easy way to go from Control to\n> > > control_info when iterating over the controls::controls id map..\n> > >\n> >\n> > Only a `ControlId` is available, so there is no static type information\n> > available. Some kind of runtime switch is needed. This seemed like the\n> > simplest method.\n>\n> Indeed.\n>\n> Do you think it would make sense to centralize the information in the\n> existing control_type and add a function similar in spirit to your\n> above TypeInfo::get() to controls.h or would you prefer keeping them\n> separate ?\n>\n\nmmm, scratch that, I had a quick look and I haven't find a nice lean\nway to specialize a template on a type and a ControlValueType. Unless\nyou have something better ready, scratch this\n\n>\n> >\n> >\n> > > >\n> > > >\n> > > >\n> > > > >\n> > > > > > +\n> > > > > > +\t\treturn {};\n> > > > > > +\t}\n> > > > > > +};\n> > > > > > +\n> > > > > > +} /* libcamera::controls::details */\n> > > > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > > > > > index a238141a5..d5a86da5e 100644\n> > > > > > --- a/src/libcamera/controls.cpp\n> > > > > > +++ b/src/libcamera/controls.cpp\n> > > > > > @@ -17,7 +17,7 @@\n> > > > > > #include \"libcamera/internal/control_validator.h\"\n> > > > > >\n> > > > > > /**\n> > > > > > - * \\file controls.h\n> > > > > > + * \\file libcamera/controls.h\n> > > > >\n> > > > > Unrelated ?\n> > > >\n> > > > I can move it to a separate commit but it is needed. Because now we have\n> > > > two `controls.h`, and otherwise doxygen will complain.\n> > > >\n> > >\n> > > Ah sorry, I missed the reason and I thought it was just a leftover\n> > >\n> > > >\n> > > > >\n> > > > > > * \\brief Framework to manage controls related to an object\n> > > > > > *\n> > > > > > * A control is a mean to govern or influence the operation of an object, and in\n> > > > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > > > > index edfa9cf58..c8ac7a673 100644\n> > > > > > --- a/src/libcamera/pipeline_handler.cpp\n> > > > > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > > > > @@ -16,11 +16,13 @@\n> > > > > > #include \n> > > > > >\n> > > > > > #include \n> > > > > > +#include \n> > > > > > #include \n> > > > > > #include \n> > > > > >\n> > > > > > #include \"libcamera/internal/camera.h\"\n> > > > > > #include \"libcamera/internal/camera_manager.h\"\n> > > > > > +#include \"libcamera/internal/controls.h\"\n> > > > > > #include \"libcamera/internal/device_enumerator.h\"\n> > > > > > #include \"libcamera/internal/media_device.h\"\n> > > > > > #include \"libcamera/internal/request.h\"\n> > > > > > @@ -749,6 +751,47 @@ std::string PipelineHandler::configurationFile(const std::string &subdir,\n> > > > > > \treturn std::string();\n> > > > > > }\n> > > > > >\n> > > > > > +namespace {\n> > > > > > +\n> > > > > > +/*\n> > > > > > + * This is kind of hack. The metadata controls in the \"debug\" namespace\n> > > > >\n> > > > > I know, but I think for the time being is the best we can do :(\n> > > > >\n> > > > > > + * are forcefully injected into each Camera's MetadataListPlan so that\n> > > > > > + * they work seamlessly without any additional setup.\n> > > > > > + *\n> > > > > > + * The dynamically-sized array-like controls have a maximum capacity\n> > > > > > + * determined by the magic number below.\n> > > > > > + */\n> > > > > > +void extendMetadataPlanWithDebugMetadata(MetadataListPlan& mlp)\n> > > > > > +{\n> > > > > > +\tconstexpr std::size_t kDynamicArrayCapacity = 32;\n> > > > > > +\n> > > > > > +\tfor (const auto &[id, ctrl] : controls::controls) {\n> > > > > > +\t\tif (!ctrl->isOutput())\n> > > > > > +\t\t\tcontinue;\n> > > > > > +\t\tif (ctrl->vendor() != \"debug\")\n> > > > > > +\t\t\tcontinue;\n> > > > > > +\t\tif (mlp.get(id))\n> > > > > > +\t\t\tcontinue;\n> > > > > > +\n> > > > > > +\t\tstd::size_t count = ctrl->size();\n> > > > > > +\t\tif (count == 0)\n> > > > > > +\t\t\tcount = 1;\n> > > > >\n> > > > > Why this ? Do you mind a comment unless is something trivial I have\n> > > > > missed ?\n> > > >\n> > > > Non-array controls have their static size set to 0. See `control_type<...>::size`.\n> > > > I can add a comment if you think it deserves one. I suppose the `MetadataListPlan`\n> > > > could be modified somehow to make this clearer.\n> > >\n> > > ack, a small comment would make it clear\n> >\n> > done\n> >\n> >\n> > Regards,\n> > Barnabás Pőcze\n> >\n> >\n> > >\n> > > >\n> > > >\n> > > > Regards,\n> > > > Barnabás Pőcze\n> > > >\n> > > >\n> > > > >\n> > > > > > +\t\telse if (ctrl->size() == libcamera::dynamic_extent)\n> > > > > > +\t\t\tcount = kDynamicArrayCapacity;\n> > > > > > +\n> > > > > > +\t\tconst auto info = controls::details::TypeInfo::get(ctrl->type());\n> > > > > > +\t\tif (!info)\n> > > > > > +\t\t\tcontinue;\n> > > > > > +\n> > > > > > +\t\t[[maybe_unused]] bool ok = mlp.set(id,\n> > > > > > +\t\t\t\t\t\t info.size, info.alignment,\n> > > > > > +\t\t\t\t\t\t count, ctrl->type(), ctrl->isArray());\n> > > > > > +\t\tASSERT(ok);\n> > > > > > +\t}\n> > > > > > +}\n> > > > > > +\n> > > > > > +} /* namespace */\n> > > > > > +\n> > > > > > /**\n> > > > > > * \\brief Register a camera to the camera manager and pipeline handler\n> > > > > > * \\param[in] camera The camera to be added\n> > > > > > @@ -794,6 +837,8 @@ void PipelineHandler::registerCamera(std::shared_ptr camera)\n> > > > > > \tCamera::Private *data = camera->_d();\n> > > > > > \tdata->properties_.set(properties::SystemDevices, devnums);\n> > > > > >\n> > > > > > +\textendMetadataPlanWithDebugMetadata(data->metadataPlan_);\n> > > > > > +\n> > > > >\n> > > > > Reviewed-by: Jacopo Mondi \n> > > > >\n> > > > > Thanks\n> > > > > j\n> > > > >\n> > > > > > \tmanager_->_d()->addCamera(std::move(camera));\n> > > > > > }\n> > > > > >\n> > > > > > --\n> > > > > > 2.50.1\n> > > > > >\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 C0AD3BDCC1\n\tfor ;\n\tSun, 3 Aug 2025 11:56:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DDC566926E;\n\tSun, 3 Aug 2025 13:56:54 +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 CC962691D5\n\tfor ;\n\tSun, 3 Aug 2025 13:56:52 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3C6E8465;\n\tSun, 3 Aug 2025 13:56:06 +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=\"CWlpoeN4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1754222166;\n\tbh=eFQ7KtNeb91wh7sPeNP75j8tcVAB6b3QLNvzWlXWsik=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=CWlpoeN4OyFFMFKkodsvDZ8aA0tvCvUwZiaJLiOf5n+k96F7FbBphqExk0DbJUQKx\n\tZF7S5mnXxsfkNJOPu1870MctvgkW+lSNEfMuBA8O+BzckdQ2cyv8jLEGQORu49EzVB\n\tXpTgAOFn0Z6HepOzMHTzv3fZClUVJh0WWfGlOPR0=","Date":"Sun, 3 Aug 2025 13:56:48 +0200","From":"Jacopo Mondi ","To":"Jacopo Mondi ","Cc":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v2 19/22] libcamera: pipeline_handler: Inject \"debug\"\n\tmetadata","Message-ID":"","References":"<20250721104622.1550908-1-barnabas.pocze@ideasonboard.com>\n\t<20250721104622.1550908-20-barnabas.pocze@ideasonboard.com>\n\t\n\t<45d11954-3747-4cfc-b2e9-59ef6b996088@ideasonboard.com>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","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\" "}},{"id":35265,"web_url":"https://patchwork.libcamera.org/comment/35265/","msgid":"<7hk3f7du3by7w6rsmlga4jlz2pk3e3p4ocb5nyls7plwudhjra@rlkxyd33tia4>","date":"2025-08-03T12:20:02","subject":"Re: [RFC PATCH v2 19/22] libcamera: pipeline_handler: Inject \"debug\"\n\tmetadata","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"On Sun, Aug 03, 2025 at 01:56:48PM +0200, Jacopo Mondi wrote:\n> On Sat, Aug 02, 2025 at 12:36:40PM +0200, Jacopo Mondi wrote:\n> > Hi Barnabás\n> >\n> > On Wed, Jul 30, 2025 at 04:49:13PM +0200, Barnabás Pőcze wrote:\n> > > Hi\n> > >\n> > > 2025. 07. 28. 18:02 keltezéssel, Jacopo Mondi írta:\n> > > > Hi Barnabás\n> > > >\n> > > > On Mon, Jul 28, 2025 at 05:19:50PM +0200, Barnabás Pőcze wrote:\n> > > > > Hi\n> > > > >\n> > > > > 2025. 07. 28. 12:19 keltezéssel, Jacopo Mondi írta:\n> > > > > > Hi Barnabás\n> > > > > >\n> > > > > > On Mon, Jul 21, 2025 at 12:46:19PM +0200, Barnabás Pőcze wrote:\n> > > > > > > Inject all metadata controls in the \"debug\" namespace into\n> > > > > > > the metadata plan of each camera so that they can be used\n> > > > > > > seamlessly. Dynamically sized array-like controls have a\n> > > > > > > hard-coded size of 32 elements.\n> > > > > > >\n> > > > > > > Additionally, a new type is added for inspecting properties of\n> > > > > > > a control type at runtime since that was not available previously.\n> > > > > > >\n> > > > > > > Signed-off-by: Barnabás Pőcze \n> > > > > > > ---\n> > > > > > > include/libcamera/internal/controls.h | 39 +++++++++++++++++++++++\n> > > > > > > src/libcamera/controls.cpp | 2 +-\n> > > > > > > src/libcamera/pipeline_handler.cpp | 45 +++++++++++++++++++++++++++\n> > > > > > > 3 files changed, 85 insertions(+), 1 deletion(-)\n> > > > > > > create mode 100644 include/libcamera/internal/controls.h\n> > > > > > >\n> > > > > > > diff --git a/include/libcamera/internal/controls.h b/include/libcamera/internal/controls.h\n> > > > > > > new file mode 100644\n> > > > > > > index 000000000..be3f93e43\n> > > > > > > --- /dev/null\n> > > > > > > +++ b/include/libcamera/internal/controls.h\n> > > > > > > @@ -0,0 +1,39 @@\n> > > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > > > +/*\n> > > > > > > + * Copyright (C) 2025, Ideas on Board Oy\n> > > > > > > + */\n> > > > > > > +\n> > > > > > > +#pragma once\n> > > > > > > +\n> > > > > > > +#include \n> > > > > > > +\n> > > > > > > +namespace libcamera::controls::details {\n> > > > > > > +\n> > > > > > > +struct TypeInfo {\n> > > > > > > +\tstd::size_t size = 0;\n> > > > > > > +\tstd::size_t alignment = 0;\n> > > > > > > +\n> > > > > > > +\texplicit operator bool() const { return alignment != 0; }\n> > > > > > > +\n> > > > > > > +\tstatic constexpr TypeInfo get(ControlType t)\n> > > > > > > +\t{\n> > > > > > > +\t\tswitch (t) {\n> > > > > > > +\t\tcase ControlTypeNone: return {};\n> > > > > > > +\t\tcase ControlTypeBool: return { sizeof(bool), alignof(bool) };\n> > > > > > > +\t\tcase ControlTypeByte: return { sizeof(uint8_t), alignof(uint8_t) };\n> > > > > > > +\t\tcase ControlTypeUnsigned16: return { sizeof(uint16_t), alignof(uint16_t) };\n> > > > > > > +\t\tcase ControlTypeUnsigned32: return { sizeof(uint32_t), alignof(uint32_t) };\n> > > > > > > +\t\tcase ControlTypeInteger32: return { sizeof(int32_t), alignof(int32_t) };\n> > > > > > > +\t\tcase ControlTypeInteger64: return { sizeof(int64_t), alignof(int64_t) };\n> > > > > > > +\t\tcase ControlTypeFloat: return { sizeof(float), alignof(float) };\n> > > > > > > +\t\tcase ControlTypeString: return { sizeof(char), alignof(char) };\n> > > > > > > +\t\tcase ControlTypeRectangle: return { sizeof(Rectangle), alignof(Rectangle) };\n> > > > > > > +\t\tcase ControlTypeSize: return { sizeof(Size), alignof(Size) };\n> > > > > > > +\t\tcase ControlTypePoint: return { sizeof(Point), alignof(Point) };\n> > > > > > > +\t\t}\n> > > > > >\n> > > > > > As suggested in a previous review can these information be added to\n> > > > > > control_type ?\n> > > > > >\n> > > > > > Would\n> > > > > >\n> > > > > > template<>\n> > > > > > struct control_type {\n> > > > > > \tstatic constexpr ControlType value = ControlTypeBool;\n> > > > > > \tstatic constexpr std::size_t size = 0;\n> > > > > > \tstatic constexpr std::size_t element_size = sizeof(bool);\n> > > > > > \tstatic constexpr std::size_t alignment = alignof(bool);\n> > > > > > };\n> > > > > >\n> > > > > > Work ?\n> > > > >\n> > > > > That could be useful yes, but you still need a function like the above to map\n> > > > > the \"runtime\" type to the appropriate values.\n> > > >\n> > > > If we use ctrl->type() yes, we need a run-time switch.\n> > > >\n> > > > I presume there is no easy way to go from Control to\n> > > > control_info when iterating over the controls::controls id map..\n> > > >\n> > >\n> > > Only a `ControlId` is available, so there is no static type information\n> > > available. Some kind of runtime switch is needed. This seemed like the\n> > > simplest method.\n> >\n> > Indeed.\n> >\n> > Do you think it would make sense to centralize the information in the\n> > existing control_type and add a function similar in spirit to your\n> > above TypeInfo::get() to controls.h or would you prefer keeping them\n> > separate ?\n> >\n>\n> mmm, scratch that, I had a quick look and I haven't find a nice lean\n> way to specialize a template on a type and a ControlValueType. Unless\n> you have something better ready, scratch this\n>\n\nBe aware however that we have a ControlValueSize map in controls.cpp,\nso this one and the newly introduced one should probably be unified\neven if we leave control_type<> alone for now\n\n> >\n> > >\n> > >\n> > > > >\n> > > > >\n> > > > >\n> > > > > >\n> > > > > > > +\n> > > > > > > +\t\treturn {};\n> > > > > > > +\t}\n> > > > > > > +};\n> > > > > > > +\n> > > > > > > +} /* libcamera::controls::details */\n> > > > > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > > > > > > index a238141a5..d5a86da5e 100644\n> > > > > > > --- a/src/libcamera/controls.cpp\n> > > > > > > +++ b/src/libcamera/controls.cpp\n> > > > > > > @@ -17,7 +17,7 @@\n> > > > > > > #include \"libcamera/internal/control_validator.h\"\n> > > > > > >\n> > > > > > > /**\n> > > > > > > - * \\file controls.h\n> > > > > > > + * \\file libcamera/controls.h\n> > > > > >\n> > > > > > Unrelated ?\n> > > > >\n> > > > > I can move it to a separate commit but it is needed. Because now we have\n> > > > > two `controls.h`, and otherwise doxygen will complain.\n> > > > >\n> > > >\n> > > > Ah sorry, I missed the reason and I thought it was just a leftover\n> > > >\n> > > > >\n> > > > > >\n> > > > > > > * \\brief Framework to manage controls related to an object\n> > > > > > > *\n> > > > > > > * A control is a mean to govern or influence the operation of an object, and in\n> > > > > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > > > > > index edfa9cf58..c8ac7a673 100644\n> > > > > > > --- a/src/libcamera/pipeline_handler.cpp\n> > > > > > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > > > > > @@ -16,11 +16,13 @@\n> > > > > > > #include \n> > > > > > >\n> > > > > > > #include \n> > > > > > > +#include \n> > > > > > > #include \n> > > > > > > #include \n> > > > > > >\n> > > > > > > #include \"libcamera/internal/camera.h\"\n> > > > > > > #include \"libcamera/internal/camera_manager.h\"\n> > > > > > > +#include \"libcamera/internal/controls.h\"\n> > > > > > > #include \"libcamera/internal/device_enumerator.h\"\n> > > > > > > #include \"libcamera/internal/media_device.h\"\n> > > > > > > #include \"libcamera/internal/request.h\"\n> > > > > > > @@ -749,6 +751,47 @@ std::string PipelineHandler::configurationFile(const std::string &subdir,\n> > > > > > > \treturn std::string();\n> > > > > > > }\n> > > > > > >\n> > > > > > > +namespace {\n> > > > > > > +\n> > > > > > > +/*\n> > > > > > > + * This is kind of hack. The metadata controls in the \"debug\" namespace\n> > > > > >\n> > > > > > I know, but I think for the time being is the best we can do :(\n> > > > > >\n> > > > > > > + * are forcefully injected into each Camera's MetadataListPlan so that\n> > > > > > > + * they work seamlessly without any additional setup.\n> > > > > > > + *\n> > > > > > > + * The dynamically-sized array-like controls have a maximum capacity\n> > > > > > > + * determined by the magic number below.\n> > > > > > > + */\n> > > > > > > +void extendMetadataPlanWithDebugMetadata(MetadataListPlan& mlp)\n> > > > > > > +{\n> > > > > > > +\tconstexpr std::size_t kDynamicArrayCapacity = 32;\n> > > > > > > +\n> > > > > > > +\tfor (const auto &[id, ctrl] : controls::controls) {\n> > > > > > > +\t\tif (!ctrl->isOutput())\n> > > > > > > +\t\t\tcontinue;\n> > > > > > > +\t\tif (ctrl->vendor() != \"debug\")\n> > > > > > > +\t\t\tcontinue;\n> > > > > > > +\t\tif (mlp.get(id))\n> > > > > > > +\t\t\tcontinue;\n> > > > > > > +\n> > > > > > > +\t\tstd::size_t count = ctrl->size();\n> > > > > > > +\t\tif (count == 0)\n> > > > > > > +\t\t\tcount = 1;\n> > > > > >\n> > > > > > Why this ? Do you mind a comment unless is something trivial I have\n> > > > > > missed ?\n> > > > >\n> > > > > Non-array controls have their static size set to 0. See `control_type<...>::size`.\n> > > > > I can add a comment if you think it deserves one. I suppose the `MetadataListPlan`\n> > > > > could be modified somehow to make this clearer.\n> > > >\n> > > > ack, a small comment would make it clear\n> > >\n> > > done\n> > >\n> > >\n> > > Regards,\n> > > Barnabás Pőcze\n> > >\n> > >\n> > > >\n> > > > >\n> > > > >\n> > > > > Regards,\n> > > > > Barnabás Pőcze\n> > > > >\n> > > > >\n> > > > > >\n> > > > > > > +\t\telse if (ctrl->size() == libcamera::dynamic_extent)\n> > > > > > > +\t\t\tcount = kDynamicArrayCapacity;\n> > > > > > > +\n> > > > > > > +\t\tconst auto info = controls::details::TypeInfo::get(ctrl->type());\n> > > > > > > +\t\tif (!info)\n> > > > > > > +\t\t\tcontinue;\n> > > > > > > +\n> > > > > > > +\t\t[[maybe_unused]] bool ok = mlp.set(id,\n> > > > > > > +\t\t\t\t\t\t info.size, info.alignment,\n> > > > > > > +\t\t\t\t\t\t count, ctrl->type(), ctrl->isArray());\n> > > > > > > +\t\tASSERT(ok);\n> > > > > > > +\t}\n> > > > > > > +}\n> > > > > > > +\n> > > > > > > +} /* namespace */\n> > > > > > > +\n> > > > > > > /**\n> > > > > > > * \\brief Register a camera to the camera manager and pipeline handler\n> > > > > > > * \\param[in] camera The camera to be added\n> > > > > > > @@ -794,6 +837,8 @@ void PipelineHandler::registerCamera(std::shared_ptr camera)\n> > > > > > > \tCamera::Private *data = camera->_d();\n> > > > > > > \tdata->properties_.set(properties::SystemDevices, devnums);\n> > > > > > >\n> > > > > > > +\textendMetadataPlanWithDebugMetadata(data->metadataPlan_);\n> > > > > > > +\n> > > > > >\n> > > > > > Reviewed-by: Jacopo Mondi \n> > > > > >\n> > > > > > Thanks\n> > > > > > j\n> > > > > >\n> > > > > > > \tmanager_->_d()->addCamera(std::move(camera));\n> > > > > > > }\n> > > > > > >\n> > > > > > > --\n> > > > > > > 2.50.1\n> > > > > > >\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 17A05BE086\n\tfor ;\n\tSun, 3 Aug 2025 12:20:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4719169218;\n\tSun, 3 Aug 2025 14:20:08 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 46D7F691D5\n\tfor ;\n\tSun, 3 Aug 2025 14:20:07 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B070D446;\n\tSun, 3 Aug 2025 14:19: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=\"Auikna/R\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1754223560;\n\tbh=6nAx14t2qK+qMmNOWMVHPzOThC6hs8rwqgSh2LAxypw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Auikna/RPk3C8yQRjVTcNHPuSEGK4bzDYI2N96k/v3nn5y3JKljsYgWjoQ4jCwavz\n\tqqQP7Bk045XcBBxcN0Jv2/QY87IK8mWaGocC8/1L+5Uhazq1aDaVQUKH1mZj3ffvzM\n\tFXLsRui/5WOfx+27WVvIRtFm7jI7ZPKR0wD3M83E=","Date":"Sun, 3 Aug 2025 14:20:02 +0200","From":"Jacopo Mondi ","To":"Jacopo Mondi ","Cc":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v2 19/22] libcamera: pipeline_handler: Inject \"debug\"\n\tmetadata","Message-ID":"<7hk3f7du3by7w6rsmlga4jlz2pk3e3p4ocb5nyls7plwudhjra@rlkxyd33tia4>","References":"<20250721104622.1550908-1-barnabas.pocze@ideasonboard.com>\n\t<20250721104622.1550908-20-barnabas.pocze@ideasonboard.com>\n\t\n\t<45d11954-3747-4cfc-b2e9-59ef6b996088@ideasonboard.com>\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","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\" "}},{"id":35888,"web_url":"https://patchwork.libcamera.org/comment/35888/","msgid":"<175819628415.2127323.8088723575210706601@neptunite.rasen.tech>","date":"2025-09-18T11:51:24","subject":"Re: [RFC PATCH v2 19/22] libcamera: pipeline_handler: Inject \"debug\"\n\tmetadata","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2025-07-21 19:46:19)\n> Inject all metadata controls in the \"debug\" namespace into\n> the metadata plan of each camera so that they can be used\n> seamlessly. Dynamically sized array-like controls have a\n> hard-coded size of 32 elements.\n> \n> Additionally, a new type is added for inspecting properties of\n> a control type at runtime since that was not available previously.\n> \n> Signed-off-by: Barnabás Pőcze \n> ---\n> include/libcamera/internal/controls.h | 39 +++++++++++++++++++++++\n> src/libcamera/controls.cpp | 2 +-\n> src/libcamera/pipeline_handler.cpp | 45 +++++++++++++++++++++++++++\n> 3 files changed, 85 insertions(+), 1 deletion(-)\n> create mode 100644 include/libcamera/internal/controls.h\n> \n> diff --git a/include/libcamera/internal/controls.h b/include/libcamera/internal/controls.h\n> new file mode 100644\n> index 000000000..be3f93e43\n> --- /dev/null\n> +++ b/include/libcamera/internal/controls.h\n> @@ -0,0 +1,39 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2025, Ideas on Board Oy\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +namespace libcamera::controls::details {\n> +\n> +struct TypeInfo {\n> + std::size_t size = 0;\n> + std::size_t alignment = 0;\n> +\n> + explicit operator bool() const { return alignment != 0; }\n> +\n> + static constexpr TypeInfo get(ControlType t)\n> + {\n> + switch (t) {\n> + case ControlTypeNone: return {};\n> + case ControlTypeBool: return { sizeof(bool), alignof(bool) };\n> + case ControlTypeByte: return { sizeof(uint8_t), alignof(uint8_t) };\n> + case ControlTypeUnsigned16: return { sizeof(uint16_t), alignof(uint16_t) };\n> + case ControlTypeUnsigned32: return { sizeof(uint32_t), alignof(uint32_t) };\n> + case ControlTypeInteger32: return { sizeof(int32_t), alignof(int32_t) };\n> + case ControlTypeInteger64: return { sizeof(int64_t), alignof(int64_t) };\n> + case ControlTypeFloat: return { sizeof(float), alignof(float) };\n> + case ControlTypeString: return { sizeof(char), alignof(char) };\n> + case ControlTypeRectangle: return { sizeof(Rectangle), alignof(Rectangle) };\n> + case ControlTypeSize: return { sizeof(Size), alignof(Size) };\n> + case ControlTypePoint: return { sizeof(Point), alignof(Point) };\n> + }\n> +\n> + return {};\n> + }\n> +};\n> +\n> +} /* libcamera::controls::details */\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index a238141a5..d5a86da5e 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -17,7 +17,7 @@\n> #include \"libcamera/internal/control_validator.h\"\n> \n> /**\n> - * \\file controls.h\n> + * \\file libcamera/controls.h\n> * \\brief Framework to manage controls related to an object\n> *\n> * A control is a mean to govern or influence the operation of an object, and in\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index edfa9cf58..c8ac7a673 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -16,11 +16,13 @@\n> #include \n> \n> #include \n> +#include \n> #include \n> #include \n> \n> #include \"libcamera/internal/camera.h\"\n> #include \"libcamera/internal/camera_manager.h\"\n> +#include \"libcamera/internal/controls.h\"\n> #include \"libcamera/internal/device_enumerator.h\"\n> #include \"libcamera/internal/media_device.h\"\n> #include \"libcamera/internal/request.h\"\n> @@ -749,6 +751,47 @@ std::string PipelineHandler::configurationFile(const std::string &subdir,\n> return std::string();\n> }\n> \n> +namespace {\n> +\n> +/*\n> + * This is kind of hack. The metadata controls in the \"debug\" namespace\n\nThis looks like a valid solution to me.\n\n> + * are forcefully injected into each Camera's MetadataListPlan so that\n> + * they work seamlessly without any additional setup.\n> + *\n> + * The dynamically-sized array-like controls have a maximum capacity\n> + * determined by the magic number below.\n> + */\n> +void extendMetadataPlanWithDebugMetadata(MetadataListPlan& mlp)\n> +{\n> + constexpr std::size_t kDynamicArrayCapacity = 32;\n> +\n> + for (const auto &[id, ctrl] : controls::controls) {\n> + if (!ctrl->isOutput())\n> + continue;\n> + if (ctrl->vendor() != \"debug\")\n> + continue;\n> + if (mlp.get(id))\n> + continue;\n> +\n> + std::size_t count = ctrl->size();\n> + if (count == 0)\n> + count = 1;\n> + else if (ctrl->size() == libcamera::dynamic_extent)\n> + count = kDynamicArrayCapacity;\n> +\n> + const auto info = controls::details::TypeInfo::get(ctrl->type());\n> + if (!info)\n> + continue;\n> +\n> + [[maybe_unused]] bool ok = mlp.set(id,\n> + info.size, info.alignment,\n> + count, ctrl->type(), ctrl->isArray());\n> + ASSERT(ok);\n\nMight it be nicer to just print a warning and skip the control if it fails to\nadd? These are debug controls and not critical controlling controls.\n\n\nEither way, looks good to me.\n\nReviewed-by: Paul Elder \n\n> + }\n> +}\n> +\n> +} /* namespace */\n> +\n> /**\n> * \\brief Register a camera to the camera manager and pipeline handler\n> * \\param[in] camera The camera to be added\n> @@ -794,6 +837,8 @@ void PipelineHandler::registerCamera(std::shared_ptr camera)\n> Camera::Private *data = camera->_d();\n> data->properties_.set(properties::SystemDevices, devnums);\n> \n> + extendMetadataPlanWithDebugMetadata(data->metadataPlan_);\n> +\n> manager_->_d()->addCamera(std::move(camera));\n> }\n> \n> -- \n> 2.50.1\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 A5420BE173\n\tfor ;\n\tThu, 18 Sep 2025 11:51:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 728B76936F;\n\tThu, 18 Sep 2025 13:51: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 A186B62C3B\n\tfor ;\n\tThu, 18 Sep 2025 13:51:32 +0200 (CEST)","from neptunite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:7cf2:5f58:dd2a:9ec1])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 7A53B520;\n\tThu, 18 Sep 2025 13:50:12 +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=\"co2Xxb3Z\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1758196213;\n\tbh=W7P6R6tyW8/+7QE4uplZPfaL8CaDiOd8J8seQsvSgFE=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=co2Xxb3ZGFH8GkAlLEThltbQYm56qUp5l0dgV3wybMHCUBLjg7jZhM7SAT2hil619\n\tO/HYAWizTJfVhAiQMAaCAOVNFacj3uUV+5WGhzbewgCWqoG9eVmighYfvlVRjNXvuO\n\tJnAvBkL8Pbs5Kq/HlfOsW2x+JrkTeRuQgKaInP94=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250721104622.1550908-20-barnabas.pocze@ideasonboard.com>","References":"<20250721104622.1550908-1-barnabas.pocze@ideasonboard.com>\n\t<20250721104622.1550908-20-barnabas.pocze@ideasonboard.com>","Subject":"Re: [RFC PATCH v2 19/22] libcamera: pipeline_handler: Inject \"debug\"\n\tmetadata","From":"Paul Elder ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 18 Sep 2025 20:51:24 +0900","Message-ID":"<175819628415.2127323.8088723575210706601@neptunite.rasen.tech>","User-Agent":"alot/0.0.0","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":35897,"web_url":"https://patchwork.libcamera.org/comment/35897/","msgid":"<1598b2c9-8837-4de6-977e-9b83faa87a35@ideasonboard.com>","date":"2025-09-18T13:18:36","subject":"Re: [RFC PATCH v2 19/22] libcamera: pipeline_handler: Inject \"debug\"\n\tmetadata","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2025. 09. 18. 13:51 keltezéssel, Paul Elder írta:\n> Quoting Barnabás Pőcze (2025-07-21 19:46:19)\n>> Inject all metadata controls in the \"debug\" namespace into\n>> the metadata plan of each camera so that they can be used\n>> seamlessly. Dynamically sized array-like controls have a\n>> hard-coded size of 32 elements.\n>>\n>> Additionally, a new type is added for inspecting properties of\n>> a control type at runtime since that was not available previously.\n>>\n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>> include/libcamera/internal/controls.h | 39 +++++++++++++++++++++++\n>> src/libcamera/controls.cpp | 2 +-\n>> src/libcamera/pipeline_handler.cpp | 45 +++++++++++++++++++++++++++\n>> 3 files changed, 85 insertions(+), 1 deletion(-)\n>> create mode 100644 include/libcamera/internal/controls.h\n>>\n>> diff --git a/include/libcamera/internal/controls.h b/include/libcamera/internal/controls.h\n>> new file mode 100644\n>> index 000000000..be3f93e43\n>> --- /dev/null\n>> +++ b/include/libcamera/internal/controls.h\n>> @@ -0,0 +1,39 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2025, Ideas on Board Oy\n>> + */\n>> +\n>> +#pragma once\n>> +\n>> +#include \n>> +\n>> +namespace libcamera::controls::details {\n>> +\n>> +struct TypeInfo {\n>> + std::size_t size = 0;\n>> + std::size_t alignment = 0;\n>> +\n>> + explicit operator bool() const { return alignment != 0; }\n>> +\n>> + static constexpr TypeInfo get(ControlType t)\n>> + {\n>> + switch (t) {\n>> + case ControlTypeNone: return {};\n>> + case ControlTypeBool: return { sizeof(bool), alignof(bool) };\n>> + case ControlTypeByte: return { sizeof(uint8_t), alignof(uint8_t) };\n>> + case ControlTypeUnsigned16: return { sizeof(uint16_t), alignof(uint16_t) };\n>> + case ControlTypeUnsigned32: return { sizeof(uint32_t), alignof(uint32_t) };\n>> + case ControlTypeInteger32: return { sizeof(int32_t), alignof(int32_t) };\n>> + case ControlTypeInteger64: return { sizeof(int64_t), alignof(int64_t) };\n>> + case ControlTypeFloat: return { sizeof(float), alignof(float) };\n>> + case ControlTypeString: return { sizeof(char), alignof(char) };\n>> + case ControlTypeRectangle: return { sizeof(Rectangle), alignof(Rectangle) };\n>> + case ControlTypeSize: return { sizeof(Size), alignof(Size) };\n>> + case ControlTypePoint: return { sizeof(Point), alignof(Point) };\n>> + }\n>> +\n>> + return {};\n>> + }\n>> +};\n>> +\n>> +} /* libcamera::controls::details */\n>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n>> index a238141a5..d5a86da5e 100644\n>> --- a/src/libcamera/controls.cpp\n>> +++ b/src/libcamera/controls.cpp\n>> @@ -17,7 +17,7 @@\n>> #include \"libcamera/internal/control_validator.h\"\n>>\n>> /**\n>> - * \\file controls.h\n>> + * \\file libcamera/controls.h\n>> * \\brief Framework to manage controls related to an object\n>> *\n>> * A control is a mean to govern or influence the operation of an object, and in\n>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n>> index edfa9cf58..c8ac7a673 100644\n>> --- a/src/libcamera/pipeline_handler.cpp\n>> +++ b/src/libcamera/pipeline_handler.cpp\n>> @@ -16,11 +16,13 @@\n>> #include \n>>\n>> #include \n>> +#include \n>> #include \n>> #include \n>>\n>> #include \"libcamera/internal/camera.h\"\n>> #include \"libcamera/internal/camera_manager.h\"\n>> +#include \"libcamera/internal/controls.h\"\n>> #include \"libcamera/internal/device_enumerator.h\"\n>> #include \"libcamera/internal/media_device.h\"\n>> #include \"libcamera/internal/request.h\"\n>> @@ -749,6 +751,47 @@ std::string PipelineHandler::configurationFile(const std::string &subdir,\n>> return std::string();\n>> }\n>>\n>> +namespace {\n>> +\n>> +/*\n>> + * This is kind of hack. The metadata controls in the \"debug\" namespace\n> \n> This looks like a valid solution to me.\n> \n>> + * are forcefully injected into each Camera's MetadataListPlan so that\n>> + * they work seamlessly without any additional setup.\n>> + *\n>> + * The dynamically-sized array-like controls have a maximum capacity\n>> + * determined by the magic number below.\n>> + */\n>> +void extendMetadataPlanWithDebugMetadata(MetadataListPlan& mlp)\n>> +{\n>> + constexpr std::size_t kDynamicArrayCapacity = 32;\n>> +\n>> + for (const auto &[id, ctrl] : controls::controls) {\n>> + if (!ctrl->isOutput())\n>> + continue;\n>> + if (ctrl->vendor() != \"debug\")\n>> + continue;\n>> + if (mlp.get(id))\n>> + continue;\n>> +\n>> + std::size_t count = ctrl->size();\n>> + if (count == 0)\n>> + count = 1;\n>> + else if (ctrl->size() == libcamera::dynamic_extent)\n>> + count = kDynamicArrayCapacity;\n>> +\n>> + const auto info = controls::details::TypeInfo::get(ctrl->type());\n>> + if (!info)\n>> + continue;\n>> +\n>> + [[maybe_unused]] bool ok = mlp.set(id,\n>> + info.size, info.alignment,\n>> + count, ctrl->type(), ctrl->isArray());\n>> + ASSERT(ok);\n> \n> Might it be nicer to just print a warning and skip the control if it fails to\n> add? These are debug controls and not critical controlling controls.\n\nThe idea was that if there is a failure here, then that likely suggests\na more serious issue somewhere. So I think an assertion is appropriate.\nThere are also no debug metadata controls in releases (at least yet),\nso no real possibility of this assertion failing.\n\n\n> \n> \n> Either way, looks good to me.\n> \n> Reviewed-by: Paul Elder \n> \n>> + }\n>> +}\n>> +\n>> +} /* namespace */\n>> +\n>> /**\n>> * \\brief Register a camera to the camera manager and pipeline handler\n>> * \\param[in] camera The camera to be added\n>> @@ -794,6 +837,8 @@ void PipelineHandler::registerCamera(std::shared_ptr camera)\n>> Camera::Private *data = camera->_d();\n>> data->properties_.set(properties::SystemDevices, devnums);\n>>\n>> + extendMetadataPlanWithDebugMetadata(data->metadataPlan_);\n>> +\n>> manager_->_d()->addCamera(std::move(camera));\n>> }\n>>\n>> --\n>> 2.50.1\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 38EDABE173\n\tfor ;\n\tThu, 18 Sep 2025 13:18:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1E7776936A;\n\tThu, 18 Sep 2025 15:18: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 C1C6C62C3B\n\tfor ;\n\tThu, 18 Sep 2025 15:18:39 +0200 (CEST)","from [192.168.33.22] (185.221.142.115.nat.pool.zt.hu\n\t[185.221.142.115])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9B817558;\n\tThu, 18 Sep 2025 15:17:19 +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=\"epRp/sm7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1758201439;\n\tbh=YESx0eE4ExJvDF9j3uJrJPWPg7P/8enYOu5m0EXJlM0=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=epRp/sm7YJdulxKBAv8UHB1ckBjb8GnL0/qGQ+ZXBlXjGA07I552EN9BGtSPqj9q+\n\thiTC7G6MQhhKi826Vj43t0vBcEHKRzv1hXm7RgF2HwZu8sjjSrD6psCLvbAJk/A63C\n\tGCk7xjyNMXR9dTxdcsEztvcaqfwpA9GNDG8sSiHw=","Message-ID":"<1598b2c9-8837-4de6-977e-9b83faa87a35@ideasonboard.com>","Date":"Thu, 18 Sep 2025 15:18:36 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v2 19/22] libcamera: pipeline_handler: Inject \"debug\"\n\tmetadata","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250721104622.1550908-1-barnabas.pocze@ideasonboard.com>\n\t<20250721104622.1550908-20-barnabas.pocze@ideasonboard.com>\n\t\n\t<175819628415.2127323.8088723575210706601@neptunite.rasen.tech>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<175819628415.2127323.8088723575210706601@neptunite.rasen.tech>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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\" "}}]