[{"id":36583,"web_url":"https://patchwork.libcamera.org/comment/36583/","msgid":"<3f0f7b91-b257-49dd-882c-15ed686057e4@ideasonboard.com>","date":"2025-10-31T16:01:02","subject":"Re: [PATCH v1 4/4] ipa: libipa: agc_mean_luminance: Fix yTarget\n\thandling in constraints","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Stefan\n\nOn 14/10/2025 15:24, Stefan Klug wrote:\n> The yTarget loading code is broken and works neither for plain values\n> nor for arrays of values to form a PWL. Fix this by supporting both\n> cases. If a list is provided in the tuning file construct a PWL,\n> otherwise construct a single point PWL with the given value.\n> \n> Fixes: 24247a12c7d3 (\"ipa: libipa: Add AgcMeanLuminance base class\")\n\nThanks :D\n\n> Signed-off-by: Stefan Klug \n> ---\n> src/ipa/libipa/agc_mean_luminance.cpp | 54 +++++++++++++++++++++------\n> src/ipa/libipa/agc_mean_luminance.h | 5 +--\n> src/ipa/rkisp1/algorithms/wdr.cpp | 3 +-\n> 3 files changed, 46 insertions(+), 16 deletions(-)\n> \n> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> index 62b1918a45a7..551f6515e849 100644\n> --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> @@ -181,7 +181,7 @@ int AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)\n> \treturn 0;\n> }\n> \n> -void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n> +int AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n> {\n> \tfor (const auto &[boundName, content] : modeDict.asDict()) {\n> \t\tif (boundName != \"upper\" && boundName != \"lower\") {\n> @@ -194,10 +194,27 @@ void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n> \t\tAgcConstraint::Bound bound = static_cast(idx);\n> \t\tdouble qLo = content[\"qLo\"].get().value_or(0.98);\n> \t\tdouble qHi = content[\"qHi\"].get().value_or(1.0);\n> -\t\tdouble yTarget =\n> -\t\t\tcontent[\"yTarget\"].getList().value_or(std::vector{ 0.5 }).at(0);\n> +\t\tauto &target = content[\"yTarget\"];\n> +\t\tPwl yTarget;\n> +\t\tif (target.isValue()) {\n> +\t\t\tauto v = target.get();\n> +\t\t\tif (!v) {\n> +\t\t\t\tLOG(AgcMeanLuminance, Error)\n> +\t\t\t\t\t<< \"Failed to parse single value yTarget\";\n> +\t\t\t\treturn -EINVAL;\n> +\t\t\t}\n> +\t\t\tyTarget.append(0, *v);\n> +\t\t} else {\n> +\t\t\tauto v = target.get();\n> +\t\t\tif (!v) {\n> +\t\t\t\tLOG(AgcMeanLuminance, Error)\n> +\t\t\t\t\t<< \"Failed to parse PWL based yTarget\";\n> +\t\t\t\treturn -EINVAL;\n> +\t\t\t}\n> +\t\t\tyTarget.swap(*v);\n> +\t\t}\n\nGiven this has much the same structure as ::parseRelativeLuminanceTarget() now it might be nice if \nthey could share an single function that does the parsing >\n> -\t\tAgcConstraint constraint = { bound, qLo, qHi, yTarget };\n> +\t\tAgcConstraint constraint = { bound, qLo, qHi, std::move(yTarget) };\n> \n> \t\tif (!constraintModes_.count(id))\n> \t\t\tconstraintModes_[id] = {};\n> @@ -207,6 +224,8 @@ void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n> \t\telse\n> \t\t\tconstraintModes_[id].insert(constraintModes_[id].begin(), constraint);\n> \t}\n> +\n> +\treturn 0;\n> }\n> \n> int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData)\n> @@ -229,8 +248,11 @@ int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData)\n> \t\t\t\treturn -EINVAL;\n> \t\t\t}\n> \n> -\t\t\tparseConstraint(modeDict,\n> -\t\t\t\t\tAeConstraintModeNameValueMap.at(modeName));\n> +\t\t\tint ret = parseConstraint(modeDict,\n> +\t\t\t\t\t\t AeConstraintModeNameValueMap.at(modeName));\n> +\t\t\tif (ret)\n> +\t\t\t\treturn ret;\n> +\n> \t\t\tavailableConstraintModes.push_back(\n> \t\t\t\tAeConstraintModeNameValueMap.at(modeName));\n> \t\t}\n> @@ -247,7 +269,7 @@ int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData)\n> \t\t\tAgcConstraint::Bound::Lower,\n> \t\t\t0.98,\n> \t\t\t1.0,\n> -\t\t\t0.5\n> +\t\t\t{ 0.0, 0.5 }\n> \t\t};\n> \n> \t\tconstraintModes_[controls::ConstraintNormal].insert(\n> @@ -360,8 +382,9 @@ void AgcMeanLuminance::configure(utils::Duration lineDuration,\n> * the data in a specific format; the Agc algorithm's tuning data should contain\n> * a dictionary called AeConstraintMode containing per-mode setting dictionaries\n> * with the key being a value from \\ref controls::AeConstraintModeNameValueMap.\n> - * Each mode dict may contain either a \"lower\" or \"upper\" key or both, for\n> - * example:\n> + * The yTarget can either be provided as single value or as array in which case\n> + * it is interpreted as a PWL mapping lux levels to yTarget values. Each mode\n> + * dict may contain either a \"lower\" or \"upper\" key or both, for example:\n\nI think maybe make it clearer here that a scalar value is wrong and only supported for legacy reasons.\n\nReviewed-by: Daniel Scally \n\n> *\n> * \\code{.unparsed}\n> * algorithms:\n> @@ -380,7 +403,7 @@ void AgcMeanLuminance::configure(utils::Duration lineDuration,\n> * upper:\n> * qLo: 0.98\n> * qHi: 1.0\n> - * yTarget: 0.8\n> + * yTarget: [ 100, 0.8, 20000, 0.5 ]\n> *\n> * \\endcode\n> *\n> @@ -536,8 +559,15 @@ double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,\n> \t\t\t\t\t const Histogram &hist,\n> \t\t\t\t\t double gain)\n> {\n> -\tauto applyConstraint = [&gain, &hist](const AgcConstraint &constraint) {\n> -\t\tdouble newGain = constraint.yTarget * hist.bins() /\n> +\tauto applyConstraint = [this, &gain, &hist](const AgcConstraint &constraint) {\n> +\t\tdouble lux = lux_;\n> +\n> +\t\tif (relativeLuminanceTarget_.size() > 1 && lux_ == 0)\n> +\t\t\tlux = kDefaultLuxLevel;\n> +\n> +\t\tdouble target = constraint.yTarget.eval(\n> +\t\t\tconstraint.yTarget.domain().clamp(lux));\n> +\t\tdouble newGain = target * hist.bins() /\n> \t\t\t\t hist.interQuantileMean(constraint.qLo, constraint.qHi);\n> \n> \t\tif (constraint.bound == AgcConstraint::Bound::Lower &&\n> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n> index 1fbff304a72b..d0d1fc139509 100644\n> --- a/src/ipa/libipa/agc_mean_luminance.h\n> +++ b/src/ipa/libipa/agc_mean_luminance.h\n> @@ -40,7 +40,7 @@ public:\n> \t\tBound bound;\n> \t\tdouble qLo;\n> \t\tdouble qHi;\n> -\t\tdouble yTarget;\n> +\t\tPwl yTarget;\n> \t};\n> \n> \tvoid configure(utils::Duration lineDuration, const CameraSensorHelper *sensorHelper);\n> @@ -88,8 +88,7 @@ public:\n> private:\n> \tvirtual double estimateLuminance(const double gain) const = 0;\n> \tint parseRelativeLuminanceTarget(const YamlObject &tuningData);\n> -\n> -\tvoid parseConstraint(const YamlObject &modeDict, int32_t id);\n> +\tint parseConstraint(const YamlObject &modeDict, int32_t id);\n> \tint parseConstraintModes(const YamlObject &tuningData);\n> \tint parseExposureModes(const YamlObject &tuningData);\n> \tdouble estimateInitialGain() const;\n> diff --git a/src/ipa/rkisp1/algorithms/wdr.cpp b/src/ipa/rkisp1/algorithms/wdr.cpp\n> index 45144913dcd8..ed81628c032c 100644\n> --- a/src/ipa/rkisp1/algorithms/wdr.cpp\n> +++ b/src/ipa/rkisp1/algorithms/wdr.cpp\n> @@ -175,7 +175,8 @@ int WideDynamicRange::configure(IPAContext &context,\n> \tconstraint.bound = AgcMeanLuminance::AgcConstraint::Bound::Upper;\n> \tconstraint.qHi = 1.0;\n> \tconstraint.qLo = 1.0 - exposureConstraintMaxBrightPixels_;\n> -\tconstraint.yTarget = exposureConstraintY_;\n> +\tconstraint.yTarget.clear();\n> +\tconstraint.yTarget.append(0, exposureConstraintY_);\n> \treturn 0;\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 9348DBE080\n\tfor ;\n\tFri, 31 Oct 2025 16:01:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8EDC0609C0;\n\tFri, 31 Oct 2025 17:01:05 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CECB460947\n\tfor ;\n\tFri, 31 Oct 2025 17:01:04 +0100 (CET)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D6DF7111D;\n\tFri, 31 Oct 2025 16:59:13 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"AWGSL0a7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1761926354;\n\tbh=naWxA46nri8bComR9+7iRsMeN/O9f2rY2ZBZsTSmvxQ=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=AWGSL0a7fDnVKq7oACJgjpZI7hQBZr7kDL0nYGgvUcaoiKoksb01hs76dT9UDOS8I\n\txlyNkKMWdII/CXyRBOjKU9bMmR9uTReNNoVCpqkZjz8aiQRdUQyTdPfdG1wyU07v8I\n\t3qZvE/MjEhfzhBSE7K3aHN4S033iBfTZaUTMZENI=","Message-ID":"<3f0f7b91-b257-49dd-882c-15ed686057e4@ideasonboard.com>","Date":"Fri, 31 Oct 2025 16:01:02 +0000","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1 4/4] ipa: libipa: agc_mean_luminance: Fix yTarget\n\thandling in constraints","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20251014142427.3107490-1-stefan.klug@ideasonboard.com>\n\t<20251014142427.3107490-5-stefan.klug@ideasonboard.com>","Content-Language":"en-US","From":"Dan Scally ","In-Reply-To":"<20251014142427.3107490-5-stefan.klug@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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":36658,"web_url":"https://patchwork.libcamera.org/comment/36658/","msgid":"<176217614022.39745.3814145899737673604@localhost>","date":"2025-11-03T13:22:20","subject":"Re: [PATCH v1 4/4] ipa: libipa: agc_mean_luminance: Fix yTarget\n\thandling in constraints","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Dan,\n\nThank you for the review.\n\nQuoting Dan Scally (2025-10-31 17:01:02)\n> Hi Stefan\n> \n> On 14/10/2025 15:24, Stefan Klug wrote:\n> > The yTarget loading code is broken and works neither for plain values\n> > nor for arrays of values to form a PWL. Fix this by supporting both\n> > cases. If a list is provided in the tuning file construct a PWL,\n> > otherwise construct a single point PWL with the given value.\n> > \n> > Fixes: 24247a12c7d3 (\"ipa: libipa: Add AgcMeanLuminance base class\")\n> \n> Thanks :D\n> \n> > Signed-off-by: Stefan Klug \n> > ---\n> > src/ipa/libipa/agc_mean_luminance.cpp | 54 +++++++++++++++++++++------\n> > src/ipa/libipa/agc_mean_luminance.h | 5 +--\n> > src/ipa/rkisp1/algorithms/wdr.cpp | 3 +-\n> > 3 files changed, 46 insertions(+), 16 deletions(-)\n> > \n> > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> > index 62b1918a45a7..551f6515e849 100644\n> > --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> > +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> > @@ -181,7 +181,7 @@ int AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)\n> > return 0;\n> > }\n> > \n> > -void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n> > +int AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n> > {\n> > for (const auto &[boundName, content] : modeDict.asDict()) {\n> > if (boundName != \"upper\" && boundName != \"lower\") {\n> > @@ -194,10 +194,27 @@ void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n> > AgcConstraint::Bound bound = static_cast(idx);\n> > double qLo = content[\"qLo\"].get().value_or(0.98);\n> > double qHi = content[\"qHi\"].get().value_or(1.0);\n> > - double yTarget =\n> > - content[\"yTarget\"].getList().value_or(std::vector{ 0.5 }).at(0);\n> > + auto &target = content[\"yTarget\"];\n> > + Pwl yTarget;\n> > + if (target.isValue()) {\n> > + auto v = target.get();\n> > + if (!v) {\n> > + LOG(AgcMeanLuminance, Error)\n> > + << \"Failed to parse single value yTarget\";\n> > + return -EINVAL;\n> > + }\n> > + yTarget.append(0, *v);\n> > + } else {\n> > + auto v = target.get();\n> > + if (!v) {\n> > + LOG(AgcMeanLuminance, Error)\n> > + << \"Failed to parse PWL based yTarget\";\n> > + return -EINVAL;\n> > + }\n> > + yTarget.swap(*v);\n> > + }\n> \n> Given this has much the same structure as ::parseRelativeLuminanceTarget() now it might be nice if \n> they could share an single function that does the parsing >\n> > - AgcConstraint constraint = { bound, qLo, qHi, yTarget };\n> > + AgcConstraint constraint = { bound, qLo, qHi, std::move(yTarget) };\n> > \n> > if (!constraintModes_.count(id))\n> > constraintModes_[id] = {};\n> > @@ -207,6 +224,8 @@ void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n> > else\n> > constraintModes_[id].insert(constraintModes_[id].begin(), constraint);\n> > }\n> > +\n> > + return 0;\n> > }\n> > \n> > int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData)\n> > @@ -229,8 +248,11 @@ int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData)\n> > return -EINVAL;\n> > }\n> > \n> > - parseConstraint(modeDict,\n> > - AeConstraintModeNameValueMap.at(modeName));\n> > + int ret = parseConstraint(modeDict,\n> > + AeConstraintModeNameValueMap.at(modeName));\n> > + if (ret)\n> > + return ret;\n> > +\n> > availableConstraintModes.push_back(\n> > AeConstraintModeNameValueMap.at(modeName));\n> > }\n> > @@ -247,7 +269,7 @@ int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData)\n> > AgcConstraint::Bound::Lower,\n> > 0.98,\n> > 1.0,\n> > - 0.5\n> > + { 0.0, 0.5 }\n> > };\n> > \n> > constraintModes_[controls::ConstraintNormal].insert(\n> > @@ -360,8 +382,9 @@ void AgcMeanLuminance::configure(utils::Duration lineDuration,\n> > * the data in a specific format; the Agc algorithm's tuning data should contain\n> > * a dictionary called AeConstraintMode containing per-mode setting dictionaries\n> > * with the key being a value from \\ref controls::AeConstraintModeNameValueMap.\n> > - * Each mode dict may contain either a \"lower\" or \"upper\" key or both, for\n> > - * example:\n> > + * The yTarget can either be provided as single value or as array in which case\n> > + * it is interpreted as a PWL mapping lux levels to yTarget values. Each mode\n> > + * dict may contain either a \"lower\" or \"upper\" key or both, for example:\n> \n> I think maybe make it clearer here that a scalar value is wrong and only supported for legacy reasons.\n\nYou mentioned something similar on one patch before. Actually I didn't\nsee the single value as a legacy feature. I like the idea that you can\nspecify either a value or a PWL. That makes the tuning files easier to\nread and write. Thinking about that we could even go one step further\nand allow the PWL parser to read a single value so we could ditch the\nspecial cases in the loading code. Functionality and tuning file wise I\nlike the idea that we document a value to be a PWL and you can\ntransparently specify it either as constant a list of pairs.\n\nWhat do you think?\n\nBest regards,\nStefan\n\n> \n> Reviewed-by: Daniel Scally \n> \n> > *\n> > * \\code{.unparsed}\n> > * algorithms:\n> > @@ -380,7 +403,7 @@ void AgcMeanLuminance::configure(utils::Duration lineDuration,\n> > * upper:\n> > * qLo: 0.98\n> > * qHi: 1.0\n> > - * yTarget: 0.8\n> > + * yTarget: [ 100, 0.8, 20000, 0.5 ]\n> > *\n> > * \\endcode\n> > *\n> > @@ -536,8 +559,15 @@ double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,\n> > const Histogram &hist,\n> > double gain)\n> > {\n> > - auto applyConstraint = [&gain, &hist](const AgcConstraint &constraint) {\n> > - double newGain = constraint.yTarget * hist.bins() /\n> > + auto applyConstraint = [this, &gain, &hist](const AgcConstraint &constraint) {\n> > + double lux = lux_;\n> > +\n> > + if (relativeLuminanceTarget_.size() > 1 && lux_ == 0)\n> > + lux = kDefaultLuxLevel;\n> > +\n> > + double target = constraint.yTarget.eval(\n> > + constraint.yTarget.domain().clamp(lux));\n> > + double newGain = target * hist.bins() /\n> > hist.interQuantileMean(constraint.qLo, constraint.qHi);\n> > \n> > if (constraint.bound == AgcConstraint::Bound::Lower &&\n> > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n> > index 1fbff304a72b..d0d1fc139509 100644\n> > --- a/src/ipa/libipa/agc_mean_luminance.h\n> > +++ b/src/ipa/libipa/agc_mean_luminance.h\n> > @@ -40,7 +40,7 @@ public:\n> > Bound bound;\n> > double qLo;\n> > double qHi;\n> > - double yTarget;\n> > + Pwl yTarget;\n> > };\n> > \n> > void configure(utils::Duration lineDuration, const CameraSensorHelper *sensorHelper);\n> > @@ -88,8 +88,7 @@ public:\n> > private:\n> > virtual double estimateLuminance(const double gain) const = 0;\n> > int parseRelativeLuminanceTarget(const YamlObject &tuningData);\n> > -\n> > - void parseConstraint(const YamlObject &modeDict, int32_t id);\n> > + int parseConstraint(const YamlObject &modeDict, int32_t id);\n> > int parseConstraintModes(const YamlObject &tuningData);\n> > int parseExposureModes(const YamlObject &tuningData);\n> > double estimateInitialGain() const;\n> > diff --git a/src/ipa/rkisp1/algorithms/wdr.cpp b/src/ipa/rkisp1/algorithms/wdr.cpp\n> > index 45144913dcd8..ed81628c032c 100644\n> > --- a/src/ipa/rkisp1/algorithms/wdr.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/wdr.cpp\n> > @@ -175,7 +175,8 @@ int WideDynamicRange::configure(IPAContext &context,\n> > constraint.bound = AgcMeanLuminance::AgcConstraint::Bound::Upper;\n> > constraint.qHi = 1.0;\n> > constraint.qLo = 1.0 - exposureConstraintMaxBrightPixels_;\n> > - constraint.yTarget = exposureConstraintY_;\n> > + constraint.yTarget.clear();\n> > + constraint.yTarget.append(0, exposureConstraintY_);\n> > return 0;\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 503F6C3241\n\tfor ;\n\tMon, 3 Nov 2025 13:22:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 63CFC60A80;\n\tMon, 3 Nov 2025 14:22:24 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4DB4A606A0\n\tfor ;\n\tMon, 3 Nov 2025 14:22:23 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:1b71:26a2:a362:3fd7])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 653931C6;\n\tMon, 3 Nov 2025 14:20:30 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"EgUjOeOy\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1762176030;\n\tbh=lR6Nivq5z7gV6bR7iwMI/F8gbhbggPMBBuBFFJZpNzs=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=EgUjOeOyzAB93m4o0XRY1LUJbt9FwWTVRCD/JH7UAPB7Vfw5uqTM8Njc0FJoQOhDd\n\tJDOdlXW7BmxQlJAtaQpYuLlcemjt4OhOIwZboQnc+6G2OIBxoZOCoTwI1MWSs9/chJ\n\ttWbONsBnV4vgZth8xbRXYFsyA3AieeeCEc1lc49I=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<3f0f7b91-b257-49dd-882c-15ed686057e4@ideasonboard.com>","References":"<20251014142427.3107490-1-stefan.klug@ideasonboard.com>\n\t<20251014142427.3107490-5-stefan.klug@ideasonboard.com>\n\t<3f0f7b91-b257-49dd-882c-15ed686057e4@ideasonboard.com>","Subject":"Re: [PATCH v1 4/4] ipa: libipa: agc_mean_luminance: Fix yTarget\n\thandling in constraints","From":"Stefan Klug ","To":"Dan Scally ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 03 Nov 2025 14:22:20 +0100","Message-ID":"<176217614022.39745.3814145899737673604@localhost>","User-Agent":"alot/0.12.dev8+g2c003385c862.d20250602","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":36665,"web_url":"https://patchwork.libcamera.org/comment/36665/","msgid":"<83db5474-0165-4073-bcb1-2c25c555970c@ideasonboard.com>","date":"2025-11-03T15:22:10","subject":"Re: [PATCH v1 4/4] ipa: libipa: agc_mean_luminance: Fix yTarget\n\thandling in constraints","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Stefan\n\nOn 03/11/2025 13:22, Stefan Klug wrote:\n> Hi Dan,\n> \n> Thank you for the review.\n> \n> Quoting Dan Scally (2025-10-31 17:01:02)\n>> Hi Stefan\n>>\n>> On 14/10/2025 15:24, Stefan Klug wrote:\n>>> The yTarget loading code is broken and works neither for plain values\n>>> nor for arrays of values to form a PWL. Fix this by supporting both\n>>> cases. If a list is provided in the tuning file construct a PWL,\n>>> otherwise construct a single point PWL with the given value.\n>>>\n>>> Fixes: 24247a12c7d3 (\"ipa: libipa: Add AgcMeanLuminance base class\")\n>>\n>> Thanks :D\n>>\n>>> Signed-off-by: Stefan Klug \n>>> ---\n>>> src/ipa/libipa/agc_mean_luminance.cpp | 54 +++++++++++++++++++++------\n>>> src/ipa/libipa/agc_mean_luminance.h | 5 +--\n>>> src/ipa/rkisp1/algorithms/wdr.cpp | 3 +-\n>>> 3 files changed, 46 insertions(+), 16 deletions(-)\n>>>\n>>> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n>>> index 62b1918a45a7..551f6515e849 100644\n>>> --- a/src/ipa/libipa/agc_mean_luminance.cpp\n>>> +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n>>> @@ -181,7 +181,7 @@ int AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)\n>>> return 0;\n>>> }\n>>> \n>>> -void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n>>> +int AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n>>> {\n>>> for (const auto &[boundName, content] : modeDict.asDict()) {\n>>> if (boundName != \"upper\" && boundName != \"lower\") {\n>>> @@ -194,10 +194,27 @@ void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n>>> AgcConstraint::Bound bound = static_cast(idx);\n>>> double qLo = content[\"qLo\"].get().value_or(0.98);\n>>> double qHi = content[\"qHi\"].get().value_or(1.0);\n>>> - double yTarget =\n>>> - content[\"yTarget\"].getList().value_or(std::vector{ 0.5 }).at(0);\n>>> + auto &target = content[\"yTarget\"];\n>>> + Pwl yTarget;\n>>> + if (target.isValue()) {\n>>> + auto v = target.get();\n>>> + if (!v) {\n>>> + LOG(AgcMeanLuminance, Error)\n>>> + << \"Failed to parse single value yTarget\";\n>>> + return -EINVAL;\n>>> + }\n>>> + yTarget.append(0, *v);\n>>> + } else {\n>>> + auto v = target.get();\n>>> + if (!v) {\n>>> + LOG(AgcMeanLuminance, Error)\n>>> + << \"Failed to parse PWL based yTarget\";\n>>> + return -EINVAL;\n>>> + }\n>>> + yTarget.swap(*v);\n>>> + }\n>>\n>> Given this has much the same structure as ::parseRelativeLuminanceTarget() now it might be nice if\n>> they could share an single function that does the parsing >\n>>> - AgcConstraint constraint = { bound, qLo, qHi, yTarget };\n>>> + AgcConstraint constraint = { bound, qLo, qHi, std::move(yTarget) };\n>>> \n>>> if (!constraintModes_.count(id))\n>>> constraintModes_[id] = {};\n>>> @@ -207,6 +224,8 @@ void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n>>> else\n>>> constraintModes_[id].insert(constraintModes_[id].begin(), constraint);\n>>> }\n>>> +\n>>> + return 0;\n>>> }\n>>> \n>>> int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData)\n>>> @@ -229,8 +248,11 @@ int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData)\n>>> return -EINVAL;\n>>> }\n>>> \n>>> - parseConstraint(modeDict,\n>>> - AeConstraintModeNameValueMap.at(modeName));\n>>> + int ret = parseConstraint(modeDict,\n>>> + AeConstraintModeNameValueMap.at(modeName));\n>>> + if (ret)\n>>> + return ret;\n>>> +\n>>> availableConstraintModes.push_back(\n>>> AeConstraintModeNameValueMap.at(modeName));\n>>> }\n>>> @@ -247,7 +269,7 @@ int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData)\n>>> AgcConstraint::Bound::Lower,\n>>> 0.98,\n>>> 1.0,\n>>> - 0.5\n>>> + { 0.0, 0.5 }\n>>> };\n>>> \n>>> constraintModes_[controls::ConstraintNormal].insert(\n>>> @@ -360,8 +382,9 @@ void AgcMeanLuminance::configure(utils::Duration lineDuration,\n>>> * the data in a specific format; the Agc algorithm's tuning data should contain\n>>> * a dictionary called AeConstraintMode containing per-mode setting dictionaries\n>>> * with the key being a value from \\ref controls::AeConstraintModeNameValueMap.\n>>> - * Each mode dict may contain either a \"lower\" or \"upper\" key or both, for\n>>> - * example:\n>>> + * The yTarget can either be provided as single value or as array in which case\n>>> + * it is interpreted as a PWL mapping lux levels to yTarget values. Each mode\n>>> + * dict may contain either a \"lower\" or \"upper\" key or both, for example:\n>>\n>> I think maybe make it clearer here that a scalar value is wrong and only supported for legacy reasons.\n> \n> You mentioned something similar on one patch before. Actually I didn't\n> see the single value as a legacy feature. I like the idea that you can\n> specify either a value or a PWL. That makes the tuning files easier to\n> read and write. Thinking about that we could even go one step further\n> and allow the PWL parser to read a single value so we could ditch the\n> special cases in the loading code. Functionality and tuning file wise I\n> like the idea that we document a value to be a PWL and you can\n> transparently specify it either as constant a list of pairs.\n> \n> What do you think?\n\nAh! Yeah sure; it certainly will make it easier to handle the tuning files if you only want it \nlux-agnostic anyway. I guess you mean a new constructor in libcamera::ipa::Pwl? If so then yes I \nagree that that would be a nice way of doing it.\n\nThanks\nDan\n\n> \n> Best regards,\n> Stefan\n> \n>>\n>> Reviewed-by: Daniel Scally \n>>\n>>> *\n>>> * \\code{.unparsed}\n>>> * algorithms:\n>>> @@ -380,7 +403,7 @@ void AgcMeanLuminance::configure(utils::Duration lineDuration,\n>>> * upper:\n>>> * qLo: 0.98\n>>> * qHi: 1.0\n>>> - * yTarget: 0.8\n>>> + * yTarget: [ 100, 0.8, 20000, 0.5 ]\n>>> *\n>>> * \\endcode\n>>> *\n>>> @@ -536,8 +559,15 @@ double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,\n>>> const Histogram &hist,\n>>> double gain)\n>>> {\n>>> - auto applyConstraint = [&gain, &hist](const AgcConstraint &constraint) {\n>>> - double newGain = constraint.yTarget * hist.bins() /\n>>> + auto applyConstraint = [this, &gain, &hist](const AgcConstraint &constraint) {\n>>> + double lux = lux_;\n>>> +\n>>> + if (relativeLuminanceTarget_.size() > 1 && lux_ == 0)\n>>> + lux = kDefaultLuxLevel;\n>>> +\n>>> + double target = constraint.yTarget.eval(\n>>> + constraint.yTarget.domain().clamp(lux));\n>>> + double newGain = target * hist.bins() /\n>>> hist.interQuantileMean(constraint.qLo, constraint.qHi);\n>>> \n>>> if (constraint.bound == AgcConstraint::Bound::Lower &&\n>>> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n>>> index 1fbff304a72b..d0d1fc139509 100644\n>>> --- a/src/ipa/libipa/agc_mean_luminance.h\n>>> +++ b/src/ipa/libipa/agc_mean_luminance.h\n>>> @@ -40,7 +40,7 @@ public:\n>>> Bound bound;\n>>> double qLo;\n>>> double qHi;\n>>> - double yTarget;\n>>> + Pwl yTarget;\n>>> };\n>>> \n>>> void configure(utils::Duration lineDuration, const CameraSensorHelper *sensorHelper);\n>>> @@ -88,8 +88,7 @@ public:\n>>> private:\n>>> virtual double estimateLuminance(const double gain) const = 0;\n>>> int parseRelativeLuminanceTarget(const YamlObject &tuningData);\n>>> -\n>>> - void parseConstraint(const YamlObject &modeDict, int32_t id);\n>>> + int parseConstraint(const YamlObject &modeDict, int32_t id);\n>>> int parseConstraintModes(const YamlObject &tuningData);\n>>> int parseExposureModes(const YamlObject &tuningData);\n>>> double estimateInitialGain() const;\n>>> diff --git a/src/ipa/rkisp1/algorithms/wdr.cpp b/src/ipa/rkisp1/algorithms/wdr.cpp\n>>> index 45144913dcd8..ed81628c032c 100644\n>>> --- a/src/ipa/rkisp1/algorithms/wdr.cpp\n>>> +++ b/src/ipa/rkisp1/algorithms/wdr.cpp\n>>> @@ -175,7 +175,8 @@ int WideDynamicRange::configure(IPAContext &context,\n>>> constraint.bound = AgcMeanLuminance::AgcConstraint::Bound::Upper;\n>>> constraint.qHi = 1.0;\n>>> constraint.qLo = 1.0 - exposureConstraintMaxBrightPixels_;\n>>> - constraint.yTarget = exposureConstraintY_;\n>>> + constraint.yTarget.clear();\n>>> + constraint.yTarget.append(0, exposureConstraintY_);\n>>> return 0;\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 99B8ABDE4C\n\tfor ;\n\tMon, 3 Nov 2025 15:22:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C8842609D8;\n\tMon, 3 Nov 2025 16:22:15 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DE154606A0\n\tfor ;\n\tMon, 3 Nov 2025 16:22:13 +0100 (CET)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C9543C73;\n\tMon, 3 Nov 2025 16:20:20 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"CAvvlJ1s\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1762183220;\n\tbh=b76r+p7ho771Bfky4kGGKFddBH9kRhZB4XnDe75lzWo=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=CAvvlJ1sCffqAxHem8xYXN7lTEyN9szRHzGFt4DfPgZsY8BosBGZdoGO+IosxZa7m\n\tz0TANBfrqeUAEHE+YjKaaiQNchlzR6Y8IxZ9znyltGdblHFIhLcLgeBPbES2t7Fhtc\n\t8woAcRguUHeO0XAEAeEgIE2rMRVP9zB32xoYcwUU=","Message-ID":"<83db5474-0165-4073-bcb1-2c25c555970c@ideasonboard.com>","Date":"Mon, 3 Nov 2025 15:22:10 +0000","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1 4/4] ipa: libipa: agc_mean_luminance: Fix yTarget\n\thandling in constraints","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20251014142427.3107490-1-stefan.klug@ideasonboard.com>\n\t<20251014142427.3107490-5-stefan.klug@ideasonboard.com>\n\t<3f0f7b91-b257-49dd-882c-15ed686057e4@ideasonboard.com>\n\t<176217614022.39745.3814145899737673604@localhost>","Content-Language":"en-US","From":"Dan Scally ","In-Reply-To":"<176217614022.39745.3814145899737673604@localhost>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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\" "}}]