[{"id":39883,"web_url":"https://patchwork.libcamera.org/comment/39883/","msgid":"","date":"2026-07-27T14:59:54","subject":"Re: [RFC PATCH v2 29/43] ipa: libipa: agc_mean_luminance: Remove\n\tcontrol handling","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 Thu, Jul 23, 2026 at 05:43:12PM +0200, Barnabás Pőcze wrote:\n> The `AgcMeanLuminance` type does not do any actual control handling,\n> so let's remove the `ControlInfoMap::Map` member, and let the user\n> handle that using the `constraintModes()` / `exposureModeHelpers()`\n> getters.\n>\n> Signed-off-by: Barnabás Pőcze \n> ---\n> src/ipa/libipa/agc.cpp | 23 +++++++++++++++++------\n> src/ipa/libipa/agc_mean_luminance.cpp | 20 +-------------------\n> src/ipa/libipa/agc_mean_luminance.h | 8 --------\n> 3 files changed, 18 insertions(+), 33 deletions(-)\n>\n> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp\n> index 54c5c6c349..27edc96fc6 100644\n> --- a/src/ipa/libipa/agc.cpp\n> +++ b/src/ipa/libipa/agc.cpp\n> @@ -400,16 +400,27 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, cons\n> \tif (session.autoAllowed) {\n> \t\tconfig.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);\n>\n> -\t\tfor (const auto &[id, info] : impl_.controls())\n> -\t\t\tconfig.ctrlMap[id] = info;\n> +\t\t{\n> +\t\t\tstd::vector options;\n> +\t\t\tfor (const auto &[id, _] : impl_.constraintModes())\n> +\t\t\t\toptions.emplace_back(id);\n> +\n> +\t\t\tconfig.ctrlMap[&controls::AeConstraintMode] = ControlInfo(options);\n> +\t\t}\n> +\n> +\t\t{\n> +\t\t\tstd::vector options;\n> +\t\t\tfor (const auto &[id, _] : impl_.exposureModeHelpers())\n> +\t\t\t\toptions.emplace_back(id);\n> +\n> +\t\t\tconfig.ctrlMap[&controls::AeExposureMode] = ControlInfo(options);\n> +\t\t}\n> \t} else {\n> \t\tconfig.ctrlMap.erase(&controls::ExposureValue);\n> -\n> -\t\tfor (const auto &[id, info] : impl_.controls())\n> -\t\t\tconfig.ctrlMap.erase(id);\n> +\t\tconfig.ctrlMap.erase(&controls::AeConstraintMode);\n> +\t\tconfig.ctrlMap.erase(&controls::AeExposureMode);\n> \t}\n\nI'm still not at peace with this, but it need a global solution, not\nfor this patch.\n\nCan we record with a todo item the reason why we need to erase\ncontrols ?\n\n>\n> -\n> \treturn 0;\n> }\n>\n> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> index a40b34b725..2c9c0b352c 100644\n> --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> @@ -237,8 +237,6 @@ int AgcMeanLuminance::parseConstraint(const ValueNode &modeDict, int32_t id)\n>\n> int AgcMeanLuminance::parseConstraintModes(const ValueNode &tuningData)\n> {\n> -\tstd::vector availableConstraintModes;\n> -\n> \tconst ValueNode &constraintModes = tuningData[controls::AeConstraintMode.name()];\n> \tif (constraintModes.isDictionary()) {\n> \t\tfor (const auto &[modeName, modeDict] : constraintModes.asDict()) {\n> @@ -258,8 +256,6 @@ int AgcMeanLuminance::parseConstraintModes(const ValueNode &tuningData)\n> \t\t\tint ret = parseConstraint(modeDict, it->second);\n> \t\t\tif (ret)\n> \t\t\t\treturn ret;\n> -\n> -\t\t\tavailableConstraintModes.push_back(it->second);\n> \t\t}\n> \t}\n>\n> @@ -278,18 +274,13 @@ int AgcMeanLuminance::parseConstraintModes(const ValueNode &tuningData)\n> \t\t};\n>\n> \t\tconstraintModes_[controls::ConstraintNormal].push_back(std::move(constraint));\n> -\t\tavailableConstraintModes.push_back(controls::ConstraintNormal);\n> \t}\n>\n> -\tcontrols_[&controls::AeConstraintMode] = ControlInfo(availableConstraintModes);\n> -\n> \treturn 0;\n> }\n>\n> int AgcMeanLuminance::parseExposureModes(const ValueNode &tuningData)\n> {\n> -\tstd::vector availableExposureModes;\n> -\n> \tconst ValueNode &exposureModes = tuningData[controls::AeExposureMode.name()];\n> \tif (exposureModes.isDictionary()) {\n> \t\tfor (const auto &[modeName, modeValues] : exposureModes.asDict()) {\n> @@ -332,7 +323,6 @@ int AgcMeanLuminance::parseExposureModes(const ValueNode &tuningData)\n> \t\t\t}\n>\n> \t\t\texposureModeHelpers_.try_emplace(it->second, stages);\n> -\t\t\tavailableExposureModes.push_back(it->second);\n> \t\t}\n> \t}\n>\n> @@ -342,14 +332,11 @@ int AgcMeanLuminance::parseExposureModes(const ValueNode &tuningData)\n> \t * in the ExposureModeHelper simply driving the exposure time as high as\n> \t * possible before touching gain.\n> \t */\n> -\tif (availableExposureModes.empty()) {\n> +\tif (exposureModeHelpers_.empty()) {\n\nYou can now drop the {}\n\n> \t\texposureModeHelpers_.try_emplace(controls::ExposureNormal,\n> \t\t\t\t\t\t Span>{});\n> -\t\tavailableExposureModes.push_back(controls::ExposureNormal);\n> \t}\n>\n> -\tcontrols_[&controls::AeExposureMode] = ControlInfo(availableExposureModes);\n> -\n> \treturn 0;\n> }\n>\n> @@ -473,11 +460,6 @@ void AgcMeanLuminance::setLimits(utils::Duration minExposureTime,\n> * \\brief Get the ExposureModeHelpers that have been parsed from tuning data\n> */\n>\n> -/**\n> - * \\fn AgcMeanLuminance::controls()\n> - * \\brief Get the controls that have been generated after parsing tuning data\n> - */\n> -\n> /**\n> * \\brief Estimate the initial gain needed to achieve a relative luminance\n> * target\n> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n> index 5e986be008..46a4d3e3f3 100644\n> --- a/src/ipa/libipa/agc_mean_luminance.h\n> +++ b/src/ipa/libipa/agc_mean_luminance.h\n> @@ -13,8 +13,6 @@\n>\n> #include \n>\n> -#include \n> -\n> #include \"libcamera/internal/value_node.h\"\n>\n> #include \"exposure_mode_helper.h\"\n> @@ -63,11 +61,6 @@ public:\n> \t\treturn exposureModeHelpers_;\n> \t}\n>\n> -\tControlInfoMap::Map controls()\n> -\t{\n> -\t\treturn controls_;\n> -\t}\n> -\n> \tstruct Params {\n> \t\tconst Traits &traits;\n> \t\tconst Histogram &yHist;\n> @@ -108,7 +101,6 @@ private:\n> \tstd::vector additionalConstraints_;\n> \tstd::map> constraintModes_;\n> \tstd::map exposureModeHelpers_;\n> -\tControlInfoMap::Map controls_;\n\nThe general direction makes sense!\n\nReviewed-by: Jacopo Mondi \n\n> };\n>\n> } /* namespace ipa */\n> --\n> 2.55.0\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 66EE5BDE4C\n\tfor ;\n\tMon, 27 Jul 2026 15:00:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 547F267F8A;\n\tMon, 27 Jul 2026 16:59:59 +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 E69EE67EB2\n\tfor ;\n\tMon, 27 Jul 2026 16:59:57 +0200 (CEST)","from ideasonboard.com (mob-5-90-50-102.net.vodafone.it\n\t[5.90.50.102])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E62113A4;\n\tMon, 27 Jul 2026 16:58:53 +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=\"WD1tfiDF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1785164334;\n\tbh=s34RwXwbKUKxkQ3Mt/l7dA4dpvfQqDxDjIQcNeZGw2A=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WD1tfiDFa295CUCWP09IDfXlXbS9bICHDxhWpAUww2eP2tEa+kI16JPZ8nToT9aj8\n\tIHWEa8DC/yVfavJHzdWAJWMGpt5Lu4G084E8tw3a7rtFjEH7qN1c+YeJmcnfPAMxcx\n\tfJv/zRKaa1UFRQZeYpjpS7FqRSbZkmIE2NL+1Jbc=","Date":"Mon, 27 Jul 2026 16:59:54 +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 29/43] ipa: libipa: agc_mean_luminance: Remove\n\tcontrol handling","Message-ID":"","References":"<20260723154327.1357866-1-barnabas.pocze@ideasonboard.com>\n\t<20260723154327.1357866-30-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":"<20260723154327.1357866-30-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":39888,"web_url":"https://patchwork.libcamera.org/comment/39888/","msgid":"<63818a0e-438c-443a-8f28-fed20ecc9dc8@ideasonboard.com>","date":"2026-07-27T15:16:39","subject":"Re: [RFC PATCH v2 29/43] ipa: libipa: agc_mean_luminance: Remove\n\tcontrol handling","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2026. 07. 27. 16:59 keltezéssel, Jacopo Mondi írta:\n> Hi Barnabás\n> \n> On Thu, Jul 23, 2026 at 05:43:12PM +0200, Barnabás Pőcze wrote:\n>> The `AgcMeanLuminance` type does not do any actual control handling,\n>> so let's remove the `ControlInfoMap::Map` member, and let the user\n>> handle that using the `constraintModes()` / `exposureModeHelpers()`\n>> getters.\n>>\n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>> src/ipa/libipa/agc.cpp | 23 +++++++++++++++++------\n>> src/ipa/libipa/agc_mean_luminance.cpp | 20 +-------------------\n>> src/ipa/libipa/agc_mean_luminance.h | 8 --------\n>> 3 files changed, 18 insertions(+), 33 deletions(-)\n>>\n>> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp\n>> index 54c5c6c349..27edc96fc6 100644\n>> --- a/src/ipa/libipa/agc.cpp\n>> +++ b/src/ipa/libipa/agc.cpp\n>> @@ -400,16 +400,27 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, cons\n>> \tif (session.autoAllowed) {\n>> \t\tconfig.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);\n>>\n>> -\t\tfor (const auto &[id, info] : impl_.controls())\n>> -\t\t\tconfig.ctrlMap[id] = info;\n>> +\t\t{\n>> +\t\t\tstd::vector options;\n>> +\t\t\tfor (const auto &[id, _] : impl_.constraintModes())\n>> +\t\t\t\toptions.emplace_back(id);\n>> +\n>> +\t\t\tconfig.ctrlMap[&controls::AeConstraintMode] = ControlInfo(options);\n>> +\t\t}\n>> +\n>> +\t\t{\n>> +\t\t\tstd::vector options;\n>> +\t\t\tfor (const auto &[id, _] : impl_.exposureModeHelpers())\n>> +\t\t\t\toptions.emplace_back(id);\n>> +\n>> +\t\t\tconfig.ctrlMap[&controls::AeExposureMode] = ControlInfo(options);\n>> +\t\t}\n>> \t} else {\n>> \t\tconfig.ctrlMap.erase(&controls::ExposureValue);\n>> -\n>> -\t\tfor (const auto &[id, info] : impl_.controls())\n>> -\t\t\tconfig.ctrlMap.erase(id);\n>> +\t\tconfig.ctrlMap.erase(&controls::AeConstraintMode);\n>> +\t\tconfig.ctrlMap.erase(&controls::AeExposureMode);\n>> \t}\n> \n> I'm still not at peace with this, but it need a global solution, not\n> for this patch.\n\nMe neither. I added an internal discussion point about this.\n\n\n> \n> Can we record with a todo item the reason why we need to erase\n> controls ?\n> \n>>\n>> -\n>> \treturn 0;\n>> }\n>>\n>> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n>> index a40b34b725..2c9c0b352c 100644\n>> --- a/src/ipa/libipa/agc_mean_luminance.cpp\n>> +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n>> @@ -237,8 +237,6 @@ int AgcMeanLuminance::parseConstraint(const ValueNode &modeDict, int32_t id)\n>>\n>> int AgcMeanLuminance::parseConstraintModes(const ValueNode &tuningData)\n>> {\n>> -\tstd::vector availableConstraintModes;\n>> -\n>> \tconst ValueNode &constraintModes = tuningData[controls::AeConstraintMode.name()];\n>> \tif (constraintModes.isDictionary()) {\n>> \t\tfor (const auto &[modeName, modeDict] : constraintModes.asDict()) {\n>> @@ -258,8 +256,6 @@ int AgcMeanLuminance::parseConstraintModes(const ValueNode &tuningData)\n>> \t\t\tint ret = parseConstraint(modeDict, it->second);\n>> \t\t\tif (ret)\n>> \t\t\t\treturn ret;\n>> -\n>> -\t\t\tavailableConstraintModes.push_back(it->second);\n>> \t\t}\n>> \t}\n>>\n>> @@ -278,18 +274,13 @@ int AgcMeanLuminance::parseConstraintModes(const ValueNode &tuningData)\n>> \t\t};\n>>\n>> \t\tconstraintModes_[controls::ConstraintNormal].push_back(std::move(constraint));\n>> -\t\tavailableConstraintModes.push_back(controls::ConstraintNormal);\n>> \t}\n>>\n>> -\tcontrols_[&controls::AeConstraintMode] = ControlInfo(availableConstraintModes);\n>> -\n>> \treturn 0;\n>> }\n>>\n>> int AgcMeanLuminance::parseExposureModes(const ValueNode &tuningData)\n>> {\n>> -\tstd::vector availableExposureModes;\n>> -\n>> \tconst ValueNode &exposureModes = tuningData[controls::AeExposureMode.name()];\n>> \tif (exposureModes.isDictionary()) {\n>> \t\tfor (const auto &[modeName, modeValues] : exposureModes.asDict()) {\n>> @@ -332,7 +323,6 @@ int AgcMeanLuminance::parseExposureModes(const ValueNode &tuningData)\n>> \t\t\t}\n>>\n>> \t\t\texposureModeHelpers_.try_emplace(it->second, stages);\n>> -\t\t\tavailableExposureModes.push_back(it->second);\n>> \t\t}\n>> \t}\n>>\n>> @@ -342,14 +332,11 @@ int AgcMeanLuminance::parseExposureModes(const ValueNode &tuningData)\n>> \t * in the ExposureModeHelper simply driving the exposure time as high as\n>> \t * possible before touching gain.\n>> \t */\n>> -\tif (availableExposureModes.empty()) {\n>> +\tif (exposureModeHelpers_.empty()) {\n> \n> You can now drop the {}\n> \n>> \t\texposureModeHelpers_.try_emplace(controls::ExposureNormal,\n>> \t\t\t\t\t\t Span>{});\n>> -\t\tavailableExposureModes.push_back(controls::ExposureNormal);\n>> \t}\n>>\n>> -\tcontrols_[&controls::AeExposureMode] = ControlInfo(availableExposureModes);\n>> -\n>> \treturn 0;\n>> }\n>>\n>> @@ -473,11 +460,6 @@ void AgcMeanLuminance::setLimits(utils::Duration minExposureTime,\n>> * \\brief Get the ExposureModeHelpers that have been parsed from tuning data\n>> */\n>>\n>> -/**\n>> - * \\fn AgcMeanLuminance::controls()\n>> - * \\brief Get the controls that have been generated after parsing tuning data\n>> - */\n>> -\n>> /**\n>> * \\brief Estimate the initial gain needed to achieve a relative luminance\n>> * target\n>> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n>> index 5e986be008..46a4d3e3f3 100644\n>> --- a/src/ipa/libipa/agc_mean_luminance.h\n>> +++ b/src/ipa/libipa/agc_mean_luminance.h\n>> @@ -13,8 +13,6 @@\n>>\n>> #include \n>>\n>> -#include \n>> -\n>> #include \"libcamera/internal/value_node.h\"\n>>\n>> #include \"exposure_mode_helper.h\"\n>> @@ -63,11 +61,6 @@ public:\n>> \t\treturn exposureModeHelpers_;\n>> \t}\n>>\n>> -\tControlInfoMap::Map controls()\n>> -\t{\n>> -\t\treturn controls_;\n>> -\t}\n>> -\n>> \tstruct Params {\n>> \t\tconst Traits &traits;\n>> \t\tconst Histogram &yHist;\n>> @@ -108,7 +101,6 @@ private:\n>> \tstd::vector additionalConstraints_;\n>> \tstd::map> constraintModes_;\n>> \tstd::map exposureModeHelpers_;\n>> -\tControlInfoMap::Map controls_;\n> \n> The general direction makes sense!\n> \n> Reviewed-by: Jacopo Mondi \n> \n>> };\n>>\n>> } /* namespace ipa */\n>> --\n>> 2.55.0\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 21DFFBDE4C\n\tfor ;\n\tMon, 27 Jul 2026 15:16:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DC31567F8F;\n\tMon, 27 Jul 2026 17:16:44 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3E0AC67EB2\n\tfor ;\n\tMon, 27 Jul 2026 17:16:43 +0200 (CEST)","from [192.168.33.46] (185.182.215.156.nat.pool.zt.hu\n\t[185.182.215.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 58F77492;\n\tMon, 27 Jul 2026 17:15:39 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"t1FpwTe3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1785165339;\n\tbh=7POZ6wdDwpGo5tYntSkzgdaiBD2q/So45CLAwu5bs7A=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=t1FpwTe3FvMKISI6ARsuVX17+1CnyTP48Ygi4C1FojsrikKs8ZbRsr3XS7CqpjAdy\n\tboWLx37O5CmEfGhj0VodTFQxo+4Gy6EUlcUarUDesYAQgnMQxouLzaJ/O6xBoLV/sa\n\tlihmcftovfKcUSSBPY9OuWoYT0pNDqOb9ZM34U5c=","Message-ID":"<63818a0e-438c-443a-8f28-fed20ecc9dc8@ideasonboard.com>","Date":"Mon, 27 Jul 2026 17:16:39 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v2 29/43] ipa: libipa: agc_mean_luminance: Remove\n\tcontrol handling","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20260723154327.1357866-1-barnabas.pocze@ideasonboard.com>\n\t<20260723154327.1357866-30-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\" "}}]