[{"id":36917,"web_url":"https://patchwork.libcamera.org/comment/36917/","msgid":"<176356138886.1142691.7641314599397775996@ping.linuxembedded.co.uk>","date":"2025-11-19T14:09:48","subject":"Re: [PATCH v3 3/4] ipa: libipa: agc_mean_luminance: Change luminance\n\ttarget to piecewise linear function","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2025-11-19 13:22:12)\n> In some situations it is necessary to specify the target brightness\n> value depending on the overall lux level. Replace the float\n> relativeLuminanceTraget by a PWL. As the PWL loading code loads a plain\n> value as single point PWL, backwards compatibility to existing tuning\n> files is ensured.\n> \n> While at it, order the class members in reverse xmas tree notation.\n> \n> Signed-off-by: Paul Elder \n> Signed-off-by: Stefan Klug \n> Reviewed-by: Daniel Scally \n\nReviewed-by: Kieran Bingham \n\n> \n> ---\n> \n> Changes in v3:\n> - Dropped reference to original patch from commit message. For the\n> record, the old patch was https://patchwork.libcamera.org/patch/20231/\n> - Ordered member variables in reverse xmas tree order\n> - Fallback to kDefaultRelativeLuminanceTarget if relativeLuminanceTraget\n> is missing in the tuning file\n> - Warn once if lux level is not available after a few frames\n> \n> Changes in v2:\n> - Collected tags\n> - Dropped special handling of plain values, as this is now part of the\n> yaml PWL loader.\n> ---\n> src/ipa/libipa/agc_mean_luminance.cpp | 67 ++++++++++++++++++++++++---\n> src/ipa/libipa/agc_mean_luminance.h | 14 ++++--\n> src/ipa/rkisp1/algorithms/agc.cpp | 1 +\n> 3 files changed, 72 insertions(+), 10 deletions(-)\n> \n> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> index 64f36bd75dd2..4f2b5fad2082 100644\n> --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> @@ -54,6 +54,14 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;\n> */\n> static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n> \n> +/*\n> + * Default lux level\n> + *\n> + * If no lux level or a zero lux level is specified, but PWLs are used to\n> + * specify luminance targets, this default level is used.\n> + */\n> +static constexpr unsigned int kDefaultLuxLevel = 500;\n> +\n> /**\n> * \\struct AgcMeanLuminance::AgcConstraint\n> * \\brief The boundaries and target for an AeConstraintMode constraint\n> @@ -144,17 +152,30 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n> */\n> \n> AgcMeanLuminance::AgcMeanLuminance()\n> - : exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s),\n> - relativeLuminanceTarget_(0)\n> + : filteredExposure_(0s), luxWarningEnabled_(true),\n> + exposureCompensation_(1.0), frameCount_(0), lux_(0)\n> {\n> }\n> \n> AgcMeanLuminance::~AgcMeanLuminance() = default;\n> \n> -void AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)\n> +int AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)\n> {\n> - relativeLuminanceTarget_ =\n> - tuningData[\"relativeLuminanceTarget\"].get(kDefaultRelativeLuminanceTarget);\n> + auto &target = tuningData[\"relativeLuminanceTarget\"];\n> + if (!target) {\n> + relativeLuminanceTarget_ = { { { { 0.0, kDefaultRelativeLuminanceTarget } } } };\n> + return 0;\n> + }\n> +\n> + auto pwl = target.get();\n> + if (!pwl) {\n> + LOG(AgcMeanLuminance, Error)\n> + << \"Failed to load relative luminance target.\";\n> + return -EINVAL;\n> + }\n> +\n> + relativeLuminanceTarget_ = std::move(*pwl);\n> + return 0;\n> }\n> \n> void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n> @@ -325,6 +346,8 @@ void AgcMeanLuminance::configure(utils::Duration lineDuration,\n> {\n> for (auto &[id, helper] : exposureModeHelpers_)\n> helper->configure(lineDuration, sensorHelper);\n> +\n> + luxWarningEnabled_ = true;\n> }\n> \n> /**\n> @@ -385,7 +408,9 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)\n> {\n> int ret;\n> \n> - parseRelativeLuminanceTarget(tuningData);\n> + ret = parseRelativeLuminanceTarget(tuningData);\n> + if (ret)\n> + return ret;\n> \n> ret = parseConstraintModes(tuningData);\n> if (ret)\n> @@ -403,6 +428,16 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)\n> * AGC calculations. It is expressed as gain instead of EV.\n> */\n> \n> +/**\n> + * \\fn AgcMeanLuminance::setLux(int lux)\n> + * \\brief Set the lux level\n> + * \\param[in] lux The lux level\n> + *\n> + * This function sets the lux level to be used in the AGC calculations. A value\n> + * of 0 means no measurement and a default value of \\a kDefaultLuxLevel is used\n> + * if necessary.\n> + */\n> +\n> /**\n> * \\brief Set the ExposureModeHelper limits for this class\n> * \\param[in] minExposureTime Minimum exposure time to allow\n> @@ -538,7 +573,25 @@ double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,\n> */\n> double AgcMeanLuminance::effectiveYTarget() const\n> {\n> - return std::min(relativeLuminanceTarget_ * exposureCompensation_,\n> + double lux = lux_;\n> + if (relativeLuminanceTarget_.size() > 1 && lux_ == 0) {\n> + if (frameCount_ > 10 && luxWarningEnabled_) {\n> + luxWarningEnabled_ = false;\n> + LOG(AgcMeanLuminance, Warning)\n> + << \"Missing lux value for luminance target \"\n> + \"calculation, default to \"\n> + << kDefaultLuxLevel\n> + << \". Note that the Lux algorithm must be \"\n> + \"included before the Agc algorithm.\";\n> + }\n> +\n> + lux = kDefaultLuxLevel;\n> + }\n> +\n> + double luminanceTarget = relativeLuminanceTarget_.eval(\n> + relativeLuminanceTarget_.domain().clamp(lux));\n> +\n> + return std::min(luminanceTarget * exposureCompensation_,\n> kMaxRelativeLuminanceTarget);\n> }\n> \n> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n> index e5f164c3186b..746b97b16ffe 100644\n> --- a/src/ipa/libipa/agc_mean_luminance.h\n> +++ b/src/ipa/libipa/agc_mean_luminance.h\n> @@ -20,6 +20,7 @@\n> \n> #include \"exposure_mode_helper.h\"\n> #include \"histogram.h\"\n> +#include \"pwl.h\"\n> \n> namespace libcamera {\n> \n> @@ -50,6 +51,11 @@ public:\n> exposureCompensation_ = gain;\n> }\n> \n> + void setLux(unsigned int lux)\n> + {\n> + lux_ = lux;\n> + }\n> +\n> void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,\n> double minGain, double maxGain, std::vector constraints);\n> \n> @@ -82,7 +88,7 @@ public:\n> private:\n> virtual double estimateLuminance(const double gain) const = 0;\n> \n> - void parseRelativeLuminanceTarget(const YamlObject &tuningData);\n> + int parseRelativeLuminanceTarget(const YamlObject &tuningData);\n> void parseConstraint(const YamlObject &modeDict, int32_t id);\n> int parseConstraintModes(const YamlObject &tuningData);\n> int parseExposureModes(const YamlObject &tuningData);\n> @@ -92,10 +98,12 @@ private:\n> double gain);\n> utils::Duration filterExposure(utils::Duration exposureValue);\n> \n> + utils::Duration filteredExposure_;\n> + mutable bool luxWarningEnabled_;\n> double exposureCompensation_;\n> + Pwl relativeLuminanceTarget_;\n> uint64_t frameCount_;\n> - utils::Duration filteredExposure_;\n> - double relativeLuminanceTarget_;\n> + unsigned int lux_;\n> \n> std::vector additionalConstraints_;\n> std::map> constraintModes_;\n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index f5a3c917cb69..1ecaff680978 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -618,6 +618,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> effectiveExposureValue *= frameContext.agc.quantizationGain;\n> \n> setExposureCompensation(pow(2.0, frameContext.agc.exposureValue));\n> + setLux(frameContext.lux.lux);\n> \n> utils::Duration newExposureTime;\n> double aGain, qGain, dGain;\n> -- \n> 2.51.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 348E8C0F1B\n\tfor ;\n\tWed, 19 Nov 2025 14:09:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4D87860A80;\n\tWed, 19 Nov 2025 15:09:53 +0100 (CET)","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 9614B606A0\n\tfor ;\n\tWed, 19 Nov 2025 15:09:51 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AE0E7DD9;\n\tWed, 19 Nov 2025 15:07:46 +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=\"iS5BtGD/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763561266;\n\tbh=8f98KcN0dKzy4tT2zh4CDu7KH9oAFIkmqeaXU5QDT8A=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=iS5BtGD/JIjhLW3sc2JmKGFLqsNtmuQ+nX2ECIpdopN4zaj5nPhPUrqohNa3OZ7aw\n\t4MLnVAeKiwD7PXA04kQdW03qnigVrdVAZ+iqYTQ6NW2o3hMBUzkjDCzsbiMFbZt4hC\n\t0r1iBKNw5H1eA+3HOOIYspnreghbg0rM/6/b/VHk=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20251119132221.2088013-4-stefan.klug@ideasonboard.com>","References":"<20251119132221.2088013-1-stefan.klug@ideasonboard.com>\n\t<20251119132221.2088013-4-stefan.klug@ideasonboard.com>","Subject":"Re: [PATCH v3 3/4] ipa: libipa: agc_mean_luminance: Change luminance\n\ttarget to piecewise linear function","From":"Kieran Bingham ","Cc":"Stefan Klug ,\n\tPaul Elder ,\n\tDaniel Scally ","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 19 Nov 2025 14:09:48 +0000","Message-ID":"<176356138886.1142691.7641314599397775996@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","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":36953,"web_url":"https://patchwork.libcamera.org/comment/36953/","msgid":"","date":"2025-11-20T17:05:41","subject":"Re: [PATCH v3 3/4] ipa: libipa: agc_mean_luminance: Change luminance\n\ttarget to piecewise linear function","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan\n\nOn Wed, Nov 19, 2025 at 02:22:12PM +0100, Stefan Klug wrote:\n> In some situations it is necessary to specify the target brightness\n> value depending on the overall lux level. Replace the float\n> relativeLuminanceTraget by a PWL. As the PWL loading code loads a plain\n> value as single point PWL, backwards compatibility to existing tuning\n> files is ensured.\n>\n> While at it, order the class members in reverse xmas tree notation.\n>\n> Signed-off-by: Paul Elder \n> Signed-off-by: Stefan Klug \n> Reviewed-by: Daniel Scally \n>\n> ---\n>\n> Changes in v3:\n> - Dropped reference to original patch from commit message. For the\n> record, the old patch was https://patchwork.libcamera.org/patch/20231/\n> - Ordered member variables in reverse xmas tree order\n> - Fallback to kDefaultRelativeLuminanceTarget if relativeLuminanceTraget\n> is missing in the tuning file\n> - Warn once if lux level is not available after a few frames\n>\n> Changes in v2:\n> - Collected tags\n> - Dropped special handling of plain values, as this is now part of the\n> yaml PWL loader.\n> ---\n> src/ipa/libipa/agc_mean_luminance.cpp | 67 ++++++++++++++++++++++++---\n> src/ipa/libipa/agc_mean_luminance.h | 14 ++++--\n> src/ipa/rkisp1/algorithms/agc.cpp | 1 +\n> 3 files changed, 72 insertions(+), 10 deletions(-)\n>\n> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> index 64f36bd75dd2..4f2b5fad2082 100644\n> --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> @@ -54,6 +54,14 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;\n> */\n> static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n>\n> +/*\n> + * Default lux level\n> + *\n> + * If no lux level or a zero lux level is specified, but PWLs are used to\n> + * specify luminance targets, this default level is used.\n> + */\n> +static constexpr unsigned int kDefaultLuxLevel = 500;\n> +\n> /**\n> * \\struct AgcMeanLuminance::AgcConstraint\n> * \\brief The boundaries and target for an AeConstraintMode constraint\n> @@ -144,17 +152,30 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n> */\n>\n> AgcMeanLuminance::AgcMeanLuminance()\n> -\t: exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s),\n> -\t relativeLuminanceTarget_(0)\n> +\t: filteredExposure_(0s), luxWarningEnabled_(true),\n> +\t exposureCompensation_(1.0), frameCount_(0), lux_(0)\n> {\n> }\n>\n> AgcMeanLuminance::~AgcMeanLuminance() = default;\n>\n> -void AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)\n> +int AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)\n> {\n> -\trelativeLuminanceTarget_ =\n> -\t\ttuningData[\"relativeLuminanceTarget\"].get(kDefaultRelativeLuminanceTarget);\n> +\tauto &target = tuningData[\"relativeLuminanceTarget\"];\n> +\tif (!target) {\n> +\t\trelativeLuminanceTarget_ = { { { { 0.0, kDefaultRelativeLuminanceTarget } } } };\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tauto pwl = target.get();\n> +\tif (!pwl) {\n> +\t\tLOG(AgcMeanLuminance, Error)\n> +\t\t\t<< \"Failed to load relative luminance target.\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\trelativeLuminanceTarget_ = std::move(*pwl);\n> +\treturn 0;\n> }\n>\n> void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n> @@ -325,6 +346,8 @@ void AgcMeanLuminance::configure(utils::Duration lineDuration,\n> {\n> \tfor (auto &[id, helper] : exposureModeHelpers_)\n> \t\thelper->configure(lineDuration, sensorHelper);\n> +\n> +\tluxWarningEnabled_ = true;\n> }\n>\n> /**\n> @@ -385,7 +408,9 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)\n> {\n> \tint ret;\n>\n> -\tparseRelativeLuminanceTarget(tuningData);\n> +\tret = parseRelativeLuminanceTarget(tuningData);\n> +\tif (ret)\n> +\t\treturn ret;\n>\n> \tret = parseConstraintModes(tuningData);\n> \tif (ret)\n> @@ -403,6 +428,16 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)\n> * AGC calculations. It is expressed as gain instead of EV.\n> */\n>\n> +/**\n> + * \\fn AgcMeanLuminance::setLux(int lux)\n> + * \\brief Set the lux level\n> + * \\param[in] lux The lux level\n> + *\n> + * This function sets the lux level to be used in the AGC calculations. A value\n> + * of 0 means no measurement and a default value of \\a kDefaultLuxLevel is used\n> + * if necessary.\n> + */\n> +\n> /**\n> * \\brief Set the ExposureModeHelper limits for this class\n> * \\param[in] minExposureTime Minimum exposure time to allow\n> @@ -538,7 +573,25 @@ double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,\n> */\n> double AgcMeanLuminance::effectiveYTarget() const\n> {\n> -\treturn std::min(relativeLuminanceTarget_ * exposureCompensation_,\n> +\tdouble lux = lux_;\n> +\tif (relativeLuminanceTarget_.size() > 1 && lux_ == 0) {\n> +\t\tif (frameCount_ > 10 && luxWarningEnabled_) {\n\nMy intention here was to push for all the messages related to parsing\nerrors to be emitted once at startup, but I understand this is not\npossible here as the error condition depends on two things:\n\n1) relativeLuminanceTarget_ is specified as a PWL which associates a\nlux level to a desired Y target\n2) the IPA doesn't call setLux()\n\nNow, while 1) is relatively easy to spot at parsing time 2) only\ndepend on the IPA implementation.\n\nAgcMeanLuminance::effectiveYTarget() is called in 2 places by IPAs:\n\n1) configure() where I guess it's ok not to have a lux level set yet ?\n2) process() where it is expected to be always called after setLux() ?\n\nif the above are true then I understand we should not Warn on\nconfigure but we should Warn on the very first frame goes through\nprocess for which effectiveYTarget() is called without a setLux() before?\n\nI'm not sure I get why 10 is used, if that's the case.\n\nThanks\n j\n\n> +\t\t\tluxWarningEnabled_ = false;\n> +\t\t\tLOG(AgcMeanLuminance, Warning)\n> +\t\t\t\t<< \"Missing lux value for luminance target \"\n> +\t\t\t\t \"calculation, default to \"\n> +\t\t\t\t<< kDefaultLuxLevel\n> +\t\t\t\t<< \". Note that the Lux algorithm must be \"\n> +\t\t\t\t \"included before the Agc algorithm.\";\n> +\t\t}\n> +\n> +\t\tlux = kDefaultLuxLevel;\n> +\t}\n> +\n> +\tdouble luminanceTarget = relativeLuminanceTarget_.eval(\n> +\t\trelativeLuminanceTarget_.domain().clamp(lux));\n> +\n> +\treturn std::min(luminanceTarget * exposureCompensation_,\n> \t\t\tkMaxRelativeLuminanceTarget);\n> }\n>\n> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n> index e5f164c3186b..746b97b16ffe 100644\n> --- a/src/ipa/libipa/agc_mean_luminance.h\n> +++ b/src/ipa/libipa/agc_mean_luminance.h\n> @@ -20,6 +20,7 @@\n>\n> #include \"exposure_mode_helper.h\"\n> #include \"histogram.h\"\n> +#include \"pwl.h\"\n>\n> namespace libcamera {\n>\n> @@ -50,6 +51,11 @@ public:\n> \t\texposureCompensation_ = gain;\n> \t}\n>\n> +\tvoid setLux(unsigned int lux)\n> +\t{\n> +\t\tlux_ = lux;\n> +\t}\n> +\n> \tvoid setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,\n> \t\t double minGain, double maxGain, std::vector constraints);\n>\n> @@ -82,7 +88,7 @@ public:\n> private:\n> \tvirtual double estimateLuminance(const double gain) const = 0;\n>\n> -\tvoid parseRelativeLuminanceTarget(const YamlObject &tuningData);\n> +\tint parseRelativeLuminanceTarget(const YamlObject &tuningData);\n> \tvoid parseConstraint(const YamlObject &modeDict, int32_t id);\n> \tint parseConstraintModes(const YamlObject &tuningData);\n> \tint parseExposureModes(const YamlObject &tuningData);\n> @@ -92,10 +98,12 @@ private:\n> \t\t\t\t double gain);\n> \tutils::Duration filterExposure(utils::Duration exposureValue);\n>\n> +\tutils::Duration filteredExposure_;\n> +\tmutable bool luxWarningEnabled_;\n> \tdouble exposureCompensation_;\n> +\tPwl relativeLuminanceTarget_;\n> \tuint64_t frameCount_;\n> -\tutils::Duration filteredExposure_;\n> -\tdouble relativeLuminanceTarget_;\n> +\tunsigned int lux_;\n>\n> \tstd::vector additionalConstraints_;\n> \tstd::map> constraintModes_;\n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index f5a3c917cb69..1ecaff680978 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -618,6 +618,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \t\teffectiveExposureValue *= frameContext.agc.quantizationGain;\n>\n> \tsetExposureCompensation(pow(2.0, frameContext.agc.exposureValue));\n> +\tsetLux(frameContext.lux.lux);\n>\n> \tutils::Duration newExposureTime;\n> \tdouble aGain, qGain, dGain;\n> --\n> 2.51.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 15166BD80A\n\tfor ;\n\tThu, 20 Nov 2025 17:05:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7461C609D8;\n\tThu, 20 Nov 2025 18:05:46 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DFCF86069A\n\tfor ;\n\tThu, 20 Nov 2025 18:05:44 +0100 (CET)","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 19B446A8;\n\tThu, 20 Nov 2025 18:03:39 +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=\"Ydp+i78X\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763658219;\n\tbh=Nxhi/V0NRBs9OVEHnxWNpFpI09cKompHaNe8TI+aqMg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Ydp+i78XDAx2Xr4iK1bZ+/qOyy0t6UM9dcM3aPIwV4HyypRBHQ14YVxzVtdVcwaus\n\t5ClJw5XuTcpFVoJjgXObAPSPhSvQCrJfwWw74kjMvjBUTwKxd4UcVDKIHLIWS/rgSA\n\tlWHgdsHc8EE+qQ5Xx6KrvGDHjAF/oZQE2zNNC/5I=","Date":"Thu, 20 Nov 2025 18:05:41 +0100","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org, \n\tPaul Elder ,\n\tDaniel Scally ","Subject":"Re: [PATCH v3 3/4] ipa: libipa: agc_mean_luminance: Change luminance\n\ttarget to piecewise linear function","Message-ID":"","References":"<20251119132221.2088013-1-stefan.klug@ideasonboard.com>\n\t<20251119132221.2088013-4-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20251119132221.2088013-4-stefan.klug@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":36954,"web_url":"https://patchwork.libcamera.org/comment/36954/","msgid":"","date":"2025-11-20T17:16:17","subject":"Re: [PATCH v3 3/4] ipa: libipa: agc_mean_luminance: Change luminance\n\ttarget to piecewise linear function","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2025. 11. 20. 18:05 keltezéssel, Jacopo Mondi írta:\n> Hi Stefan\n> \n> On Wed, Nov 19, 2025 at 02:22:12PM +0100, Stefan Klug wrote:\n>> In some situations it is necessary to specify the target brightness\n>> value depending on the overall lux level. Replace the float\n>> relativeLuminanceTraget by a PWL. As the PWL loading code loads a plain\n>> value as single point PWL, backwards compatibility to existing tuning\n>> files is ensured.\n>>\n>> While at it, order the class members in reverse xmas tree notation.\n>>\n>> Signed-off-by: Paul Elder \n>> Signed-off-by: Stefan Klug \n>> Reviewed-by: Daniel Scally \n>>\n>> ---\n>>\n>> Changes in v3:\n>> - Dropped reference to original patch from commit message. For the\n>> record, the old patch was https://patchwork.libcamera.org/patch/20231/\n>> - Ordered member variables in reverse xmas tree order\n>> - Fallback to kDefaultRelativeLuminanceTarget if relativeLuminanceTraget\n>> is missing in the tuning file\n>> - Warn once if lux level is not available after a few frames\n>>\n>> Changes in v2:\n>> - Collected tags\n>> - Dropped special handling of plain values, as this is now part of the\n>> yaml PWL loader.\n>> ---\n>> src/ipa/libipa/agc_mean_luminance.cpp | 67 ++++++++++++++++++++++++---\n>> src/ipa/libipa/agc_mean_luminance.h | 14 ++++--\n>> src/ipa/rkisp1/algorithms/agc.cpp | 1 +\n>> 3 files changed, 72 insertions(+), 10 deletions(-)\n>>\n>> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n>> index 64f36bd75dd2..4f2b5fad2082 100644\n>> --- a/src/ipa/libipa/agc_mean_luminance.cpp\n>> +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n>> @@ -54,6 +54,14 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;\n>> */\n>> static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n>>\n>> +/*\n>> + * Default lux level\n>> + *\n>> + * If no lux level or a zero lux level is specified, but PWLs are used to\n>> + * specify luminance targets, this default level is used.\n>> + */\n>> +static constexpr unsigned int kDefaultLuxLevel = 500;\n>> +\n>> /**\n>> * \\struct AgcMeanLuminance::AgcConstraint\n>> * \\brief The boundaries and target for an AeConstraintMode constraint\n>> @@ -144,17 +152,30 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n>> */\n>>\n>> AgcMeanLuminance::AgcMeanLuminance()\n>> -\t: exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s),\n>> -\t relativeLuminanceTarget_(0)\n>> +\t: filteredExposure_(0s), luxWarningEnabled_(true),\n>> +\t exposureCompensation_(1.0), frameCount_(0), lux_(0)\n>> {\n>> }\n>>\n>> AgcMeanLuminance::~AgcMeanLuminance() = default;\n>>\n>> -void AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)\n>> +int AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)\n>> {\n>> -\trelativeLuminanceTarget_ =\n>> -\t\ttuningData[\"relativeLuminanceTarget\"].get(kDefaultRelativeLuminanceTarget);\n>> +\tauto &target = tuningData[\"relativeLuminanceTarget\"];\n>> +\tif (!target) {\n>> +\t\trelativeLuminanceTarget_ = { { { { 0.0, kDefaultRelativeLuminanceTarget } } } };\n>> +\t\treturn 0;\n>> +\t}\n>> +\n>> +\tauto pwl = target.get();\n>> +\tif (!pwl) {\n>> +\t\tLOG(AgcMeanLuminance, Error)\n>> +\t\t\t<< \"Failed to load relative luminance target.\";\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\trelativeLuminanceTarget_ = std::move(*pwl);\n>> +\treturn 0;\n>> }\n>>\n>> void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n>> @@ -325,6 +346,8 @@ void AgcMeanLuminance::configure(utils::Duration lineDuration,\n>> {\n>> \tfor (auto &[id, helper] : exposureModeHelpers_)\n>> \t\thelper->configure(lineDuration, sensorHelper);\n>> +\n>> +\tluxWarningEnabled_ = true;\n>> }\n>>\n>> /**\n>> @@ -385,7 +408,9 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)\n>> {\n>> \tint ret;\n>>\n>> -\tparseRelativeLuminanceTarget(tuningData);\n>> +\tret = parseRelativeLuminanceTarget(tuningData);\n>> +\tif (ret)\n>> +\t\treturn ret;\n>>\n>> \tret = parseConstraintModes(tuningData);\n>> \tif (ret)\n>> @@ -403,6 +428,16 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)\n>> * AGC calculations. It is expressed as gain instead of EV.\n>> */\n>>\n>> +/**\n>> + * \\fn AgcMeanLuminance::setLux(int lux)\n>> + * \\brief Set the lux level\n>> + * \\param[in] lux The lux level\n>> + *\n>> + * This function sets the lux level to be used in the AGC calculations. A value\n>> + * of 0 means no measurement and a default value of \\a kDefaultLuxLevel is used\n>> + * if necessary.\n>> + */\n>> +\n>> /**\n>> * \\brief Set the ExposureModeHelper limits for this class\n>> * \\param[in] minExposureTime Minimum exposure time to allow\n>> @@ -538,7 +573,25 @@ double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,\n>> */\n>> double AgcMeanLuminance::effectiveYTarget() const\n>> {\n>> -\treturn std::min(relativeLuminanceTarget_ * exposureCompensation_,\n>> +\tdouble lux = lux_;\n>> +\tif (relativeLuminanceTarget_.size() > 1 && lux_ == 0) {\n>> +\t\tif (frameCount_ > 10 && luxWarningEnabled_) {\n> \n> My intention here was to push for all the messages related to parsing\n> errors to be emitted once at startup, but I understand this is not\n> possible here as the error condition depends on two things:\n> \n> 1) relativeLuminanceTarget_ is specified as a PWL which associates a\n> lux level to a desired Y target\n> 2) the IPA doesn't call setLux()\n\nSomewhat related, wouldn't it be better to have `lux` as a parameter of\n`effectiveYTarget()`? (And hence `calculateNewEv()`, etc?)\n\nIt seems that both `lux` and `exposureCompensation` only need to be parameters\nand need not be stored persistently in the class.\n\n\n> \n> Now, while 1) is relatively easy to spot at parsing time 2) only\n> depend on the IPA implementation.\n> \n> AgcMeanLuminance::effectiveYTarget() is called in 2 places by IPAs:\n> \n> 1) configure() where I guess it's ok not to have a lux level set yet ?\n> 2) process() where it is expected to be always called after setLux() ?\n> \n> if the above are true then I understand we should not Warn on\n> configure but we should Warn on the very first frame goes through\n> process for which effectiveYTarget() is called without a setLux() before?\n> \n> I'm not sure I get why 10 is used, if that's the case.\n> \n> Thanks\n> j\n> \n>> +\t\t\tluxWarningEnabled_ = false;\n>> +\t\t\tLOG(AgcMeanLuminance, Warning)\n>> +\t\t\t\t<< \"Missing lux value for luminance target \"\n>> +\t\t\t\t \"calculation, default to \"\n>> +\t\t\t\t<< kDefaultLuxLevel\n>> +\t\t\t\t<< \". Note that the Lux algorithm must be \"\n>> +\t\t\t\t \"included before the Agc algorithm.\";\n>> +\t\t}\n>> +\n>> +\t\tlux = kDefaultLuxLevel;\n>> +\t}\n>> +\n>> +\tdouble luminanceTarget = relativeLuminanceTarget_.eval(\n>> +\t\trelativeLuminanceTarget_.domain().clamp(lux));\n>> +\n>> +\treturn std::min(luminanceTarget * exposureCompensation_,\n>> \t\t\tkMaxRelativeLuminanceTarget);\n>> }\n>>\n>> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n>> index e5f164c3186b..746b97b16ffe 100644\n>> --- a/src/ipa/libipa/agc_mean_luminance.h\n>> +++ b/src/ipa/libipa/agc_mean_luminance.h\n>> @@ -20,6 +20,7 @@\n>>\n>> #include \"exposure_mode_helper.h\"\n>> #include \"histogram.h\"\n>> +#include \"pwl.h\"\n>>\n>> namespace libcamera {\n>>\n>> @@ -50,6 +51,11 @@ public:\n>> \t\texposureCompensation_ = gain;\n>> \t}\n>>\n>> +\tvoid setLux(unsigned int lux)\n>> +\t{\n>> +\t\tlux_ = lux;\n>> +\t}\n>> +\n>> \tvoid setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,\n>> \t\t double minGain, double maxGain, std::vector constraints);\n>>\n>> @@ -82,7 +88,7 @@ public:\n>> private:\n>> \tvirtual double estimateLuminance(const double gain) const = 0;\n>>\n>> -\tvoid parseRelativeLuminanceTarget(const YamlObject &tuningData);\n>> +\tint parseRelativeLuminanceTarget(const YamlObject &tuningData);\n>> \tvoid parseConstraint(const YamlObject &modeDict, int32_t id);\n>> \tint parseConstraintModes(const YamlObject &tuningData);\n>> \tint parseExposureModes(const YamlObject &tuningData);\n>> @@ -92,10 +98,12 @@ private:\n>> \t\t\t\t double gain);\n>> \tutils::Duration filterExposure(utils::Duration exposureValue);\n>>\n>> +\tutils::Duration filteredExposure_;\n>> +\tmutable bool luxWarningEnabled_;\n>> \tdouble exposureCompensation_;\n>> +\tPwl relativeLuminanceTarget_;\n>> \tuint64_t frameCount_;\n>> -\tutils::Duration filteredExposure_;\n>> -\tdouble relativeLuminanceTarget_;\n>> +\tunsigned int lux_;\n>>\n>> \tstd::vector additionalConstraints_;\n>> \tstd::map> constraintModes_;\n>> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n>> index f5a3c917cb69..1ecaff680978 100644\n>> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n>> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n>> @@ -618,6 +618,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n>> \t\teffectiveExposureValue *= frameContext.agc.quantizationGain;\n>>\n>> \tsetExposureCompensation(pow(2.0, frameContext.agc.exposureValue));\n>> +\tsetLux(frameContext.lux.lux);\n>>\n>> \tutils::Duration newExposureTime;\n>> \tdouble aGain, qGain, dGain;\n>> --\n>> 2.51.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 A7CE9C0F1B\n\tfor ;\n\tThu, 20 Nov 2025 17:16:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B89F060A85;\n\tThu, 20 Nov 2025 18:16:22 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 65008609DE\n\tfor ;\n\tThu, 20 Nov 2025 18:16:21 +0100 (CET)","from [192.168.33.37] (185.221.143.100.nat.pool.zt.hu\n\t[185.221.143.100])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DBEB8C77;\n\tThu, 20 Nov 2025 18:14:15 +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=\"LTui8eM/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763658856;\n\tbh=B/VhGTqY8G9wtchCF96wiSlTtT2GuUkTwRPmMS7dJmU=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=LTui8eM/JEo9Rf6YYEh9sktLGIaCg5jspkhsK+eRDBarpGzJnhajuK7AhdHj0jgtN\n\tU07U8jvNpSLIXHqvhRg/ZxXRC2ZFirNEdrCQlc6CrtqlOD9kgFDu90yZJEB1gH4mjO\n\t56WMAW7KDaZLIsrexHcP2OB5W1c1w2WxT3a3PBpQ=","Message-ID":"","Date":"Thu, 20 Nov 2025 18:16:17 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3 3/4] ipa: libipa: agc_mean_luminance: Change luminance\n\ttarget to piecewise linear function","To":"Jacopo Mondi ,\n\tStefan Klug ","Cc":"libcamera-devel@lists.libcamera.org,\n\tPaul Elder ,\n\tDaniel Scally ","References":"<20251119132221.2088013-1-stefan.klug@ideasonboard.com>\n\t<20251119132221.2088013-4-stefan.klug@ideasonboard.com>\n\t\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":36957,"web_url":"https://patchwork.libcamera.org/comment/36957/","msgid":"<176366126846.2926148.17733284974166953641@localhost>","date":"2025-11-20T17:54:28","subject":"Re: [PATCH v3 3/4] ipa: libipa: agc_mean_luminance: Change luminance\n\ttarget to piecewise linear function","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Jacopo,\n\nQuoting Jacopo Mondi (2025-11-20 18:05:41)\n> Hi Stefan\n> \n> On Wed, Nov 19, 2025 at 02:22:12PM +0100, Stefan Klug wrote:\n> > In some situations it is necessary to specify the target brightness\n> > value depending on the overall lux level. Replace the float\n> > relativeLuminanceTraget by a PWL. As the PWL loading code loads a plain\n> > value as single point PWL, backwards compatibility to existing tuning\n> > files is ensured.\n> >\n> > While at it, order the class members in reverse xmas tree notation.\n> >\n> > Signed-off-by: Paul Elder \n> > Signed-off-by: Stefan Klug \n> > Reviewed-by: Daniel Scally \n> >\n> > ---\n> >\n> > Changes in v3:\n> > - Dropped reference to original patch from commit message. For the\n> > record, the old patch was https://patchwork.libcamera.org/patch/20231/\n> > - Ordered member variables in reverse xmas tree order\n> > - Fallback to kDefaultRelativeLuminanceTarget if relativeLuminanceTraget\n> > is missing in the tuning file\n> > - Warn once if lux level is not available after a few frames\n> >\n> > Changes in v2:\n> > - Collected tags\n> > - Dropped special handling of plain values, as this is now part of the\n> > yaml PWL loader.\n> > ---\n> > src/ipa/libipa/agc_mean_luminance.cpp | 67 ++++++++++++++++++++++++---\n> > src/ipa/libipa/agc_mean_luminance.h | 14 ++++--\n> > src/ipa/rkisp1/algorithms/agc.cpp | 1 +\n> > 3 files changed, 72 insertions(+), 10 deletions(-)\n> >\n> > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> > index 64f36bd75dd2..4f2b5fad2082 100644\n> > --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> > +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> > @@ -54,6 +54,14 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;\n> > */\n> > static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n> >\n> > +/*\n> > + * Default lux level\n> > + *\n> > + * If no lux level or a zero lux level is specified, but PWLs are used to\n> > + * specify luminance targets, this default level is used.\n> > + */\n> > +static constexpr unsigned int kDefaultLuxLevel = 500;\n> > +\n> > /**\n> > * \\struct AgcMeanLuminance::AgcConstraint\n> > * \\brief The boundaries and target for an AeConstraintMode constraint\n> > @@ -144,17 +152,30 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n> > */\n> >\n> > AgcMeanLuminance::AgcMeanLuminance()\n> > - : exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s),\n> > - relativeLuminanceTarget_(0)\n> > + : filteredExposure_(0s), luxWarningEnabled_(true),\n> > + exposureCompensation_(1.0), frameCount_(0), lux_(0)\n> > {\n> > }\n> >\n> > AgcMeanLuminance::~AgcMeanLuminance() = default;\n> >\n> > -void AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)\n> > +int AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)\n> > {\n> > - relativeLuminanceTarget_ =\n> > - tuningData[\"relativeLuminanceTarget\"].get(kDefaultRelativeLuminanceTarget);\n> > + auto &target = tuningData[\"relativeLuminanceTarget\"];\n> > + if (!target) {\n> > + relativeLuminanceTarget_ = { { { { 0.0, kDefaultRelativeLuminanceTarget } } } };\n> > + return 0;\n> > + }\n> > +\n> > + auto pwl = target.get();\n> > + if (!pwl) {\n> > + LOG(AgcMeanLuminance, Error)\n> > + << \"Failed to load relative luminance target.\";\n> > + return -EINVAL;\n> > + }\n> > +\n> > + relativeLuminanceTarget_ = std::move(*pwl);\n> > + return 0;\n> > }\n> >\n> > void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n> > @@ -325,6 +346,8 @@ void AgcMeanLuminance::configure(utils::Duration lineDuration,\n> > {\n> > for (auto &[id, helper] : exposureModeHelpers_)\n> > helper->configure(lineDuration, sensorHelper);\n> > +\n> > + luxWarningEnabled_ = true;\n> > }\n> >\n> > /**\n> > @@ -385,7 +408,9 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)\n> > {\n> > int ret;\n> >\n> > - parseRelativeLuminanceTarget(tuningData);\n> > + ret = parseRelativeLuminanceTarget(tuningData);\n> > + if (ret)\n> > + return ret;\n> >\n> > ret = parseConstraintModes(tuningData);\n> > if (ret)\n> > @@ -403,6 +428,16 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)\n> > * AGC calculations. It is expressed as gain instead of EV.\n> > */\n> >\n> > +/**\n> > + * \\fn AgcMeanLuminance::setLux(int lux)\n> > + * \\brief Set the lux level\n> > + * \\param[in] lux The lux level\n> > + *\n> > + * This function sets the lux level to be used in the AGC calculations. A value\n> > + * of 0 means no measurement and a default value of \\a kDefaultLuxLevel is used\n> > + * if necessary.\n> > + */\n> > +\n> > /**\n> > * \\brief Set the ExposureModeHelper limits for this class\n> > * \\param[in] minExposureTime Minimum exposure time to allow\n> > @@ -538,7 +573,25 @@ double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,\n> > */\n> > double AgcMeanLuminance::effectiveYTarget() const\n> > {\n> > - return std::min(relativeLuminanceTarget_ * exposureCompensation_,\n> > + double lux = lux_;\n> > + if (relativeLuminanceTarget_.size() > 1 && lux_ == 0) {\n> > + if (frameCount_ > 10 && luxWarningEnabled_) {\n> \n> My intention here was to push for all the messages related to parsing\n> errors to be emitted once at startup, but I understand this is not\n> possible here as the error condition depends on two things:\n> \n> 1) relativeLuminanceTarget_ is specified as a PWL which associates a\n> lux level to a desired Y target\n> 2) the IPA doesn't call setLux()\n> \n> Now, while 1) is relatively easy to spot at parsing time 2) only\n> depend on the IPA implementation.\n> \n> AgcMeanLuminance::effectiveYTarget() is called in 2 places by IPAs:\n> \n> 1) configure() where I guess it's ok not to have a lux level set yet ?\n> 2) process() where it is expected to be always called after setLux() ?\n> \n> if the above are true then I understand we should not Warn on\n> configure but we should Warn on the very first frame goes through\n> process for which effectiveYTarget() is called without a setLux() before?\n> \n> I'm not sure I get why 10 is used, if that's the case.\n\n10 was arbitrarily chosen, so that we get at least something. \nReading your message I realize, that I was a few steps further and\nalready planning for the regulation rework which makes this hard to\nfollow.\n\nYou are right, currently it is called within process() where the agc\ncalculation happens. The issue here is that this is plain wrong but\nfixing needs to wait until after the regulation series.\n\nThe agc calculation and also setLux() should happen in prepare(). Reason\nis that for example frameContext.agc.exposureValue feeds into the agc\ncalculations. So we get stats for frame n, calculate new exposure\nsettings based on fc[n].agc.exposureValue and these will then be used in\nprepare(n+lookahead). That is wrong.\n\nThe correct thing to do would be to cache the stats but do the agc\ncalculations in prepare() so that fc[n+lookahed].agc.exposureValue feeds\ninto the calculation.\n\nWhen the above is fixed, there will be no lux value available for the\nfirst few frames as prepare() is run for these frames before the stats\nfor frame 0 have arrived which is the time when the first lux value can\nbe calculated.\n\n10 is bigger than the number of frames that get prepared in advance and\nis small enough to show up early on.\n\nDoes that make sense?\n\nBest regards,\nStefan\n\n> \n> Thanks\n> j\n> \n> > + luxWarningEnabled_ = false;\n> > + LOG(AgcMeanLuminance, Warning)\n> > + << \"Missing lux value for luminance target \"\n> > + \"calculation, default to \"\n> > + << kDefaultLuxLevel\n> > + << \". Note that the Lux algorithm must be \"\n> > + \"included before the Agc algorithm.\";\n> > + }\n> > +\n> > + lux = kDefaultLuxLevel;\n> > + }\n> > +\n> > + double luminanceTarget = relativeLuminanceTarget_.eval(\n> > + relativeLuminanceTarget_.domain().clamp(lux));\n> > +\n> > + return std::min(luminanceTarget * exposureCompensation_,\n> > kMaxRelativeLuminanceTarget);\n> > }\n> >\n> > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n> > index e5f164c3186b..746b97b16ffe 100644\n> > --- a/src/ipa/libipa/agc_mean_luminance.h\n> > +++ b/src/ipa/libipa/agc_mean_luminance.h\n> > @@ -20,6 +20,7 @@\n> >\n> > #include \"exposure_mode_helper.h\"\n> > #include \"histogram.h\"\n> > +#include \"pwl.h\"\n> >\n> > namespace libcamera {\n> >\n> > @@ -50,6 +51,11 @@ public:\n> > exposureCompensation_ = gain;\n> > }\n> >\n> > + void setLux(unsigned int lux)\n> > + {\n> > + lux_ = lux;\n> > + }\n> > +\n> > void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,\n> > double minGain, double maxGain, std::vector constraints);\n> >\n> > @@ -82,7 +88,7 @@ public:\n> > private:\n> > virtual double estimateLuminance(const double gain) const = 0;\n> >\n> > - void parseRelativeLuminanceTarget(const YamlObject &tuningData);\n> > + int parseRelativeLuminanceTarget(const YamlObject &tuningData);\n> > void parseConstraint(const YamlObject &modeDict, int32_t id);\n> > int parseConstraintModes(const YamlObject &tuningData);\n> > int parseExposureModes(const YamlObject &tuningData);\n> > @@ -92,10 +98,12 @@ private:\n> > double gain);\n> > utils::Duration filterExposure(utils::Duration exposureValue);\n> >\n> > + utils::Duration filteredExposure_;\n> > + mutable bool luxWarningEnabled_;\n> > double exposureCompensation_;\n> > + Pwl relativeLuminanceTarget_;\n> > uint64_t frameCount_;\n> > - utils::Duration filteredExposure_;\n> > - double relativeLuminanceTarget_;\n> > + unsigned int lux_;\n> >\n> > std::vector additionalConstraints_;\n> > std::map> constraintModes_;\n> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > index f5a3c917cb69..1ecaff680978 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > @@ -618,6 +618,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > effectiveExposureValue *= frameContext.agc.quantizationGain;\n> >\n> > setExposureCompensation(pow(2.0, frameContext.agc.exposureValue));\n> > + setLux(frameContext.lux.lux);\n> >\n> > utils::Duration newExposureTime;\n> > double aGain, qGain, dGain;\n> > --\n> > 2.51.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 421C0C3330\n\tfor ;\n\tThu, 20 Nov 2025 17:54:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7DD2660A81;\n\tThu, 20 Nov 2025 18:54:33 +0100 (CET)","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 4151F6069A\n\tfor ;\n\tThu, 20 Nov 2025 18:54:32 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:6a98:fbdf:d77e:8a49])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 9257CC77;\n\tThu, 20 Nov 2025 18:52:26 +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=\"HONhomPE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763661146;\n\tbh=SVgoimk3rNoQiKkg6ENWrkl6WRHghBLzzm3GN0Alkvs=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=HONhomPEUr11UPDYd5jL5hA/w73SGZFlAu410KYwq6ATGWsEKZY5PFKYZBBSKZJUV\n\tio0ibW5RP/F30PNplzuos/bcXqRqY8U9ExlUoKqPWUb3Sz1SOZzTEvxfk6AmGxPDCW\n\tE9rcu53NOKB1PvMaqLQFmjTlm+5aH3zkdW9WG+9Y=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20251119132221.2088013-1-stefan.klug@ideasonboard.com>\n\t<20251119132221.2088013-4-stefan.klug@ideasonboard.com>\n\t","Subject":"Re: [PATCH v3 3/4] ipa: libipa: agc_mean_luminance: Change luminance\n\ttarget to piecewise linear function","From":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org,\n\tPaul Elder ,\n\tDaniel Scally ","To":"Jacopo Mondi ","Date":"Thu, 20 Nov 2025 18:54:28 +0100","Message-ID":"<176366126846.2926148.17733284974166953641@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":36967,"web_url":"https://patchwork.libcamera.org/comment/36967/","msgid":"","date":"2025-11-21T09:04:58","subject":"Re: [PATCH v3 3/4] ipa: libipa: agc_mean_luminance: Change luminance\n\ttarget to piecewise linear function","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan\n\nOn Thu, Nov 20, 2025 at 06:54:28PM +0100, Stefan Klug wrote:\n> Hi Jacopo,\n>\n> Quoting Jacopo Mondi (2025-11-20 18:05:41)\n> > Hi Stefan\n> >\n> > On Wed, Nov 19, 2025 at 02:22:12PM +0100, Stefan Klug wrote:\n> > > In some situations it is necessary to specify the target brightness\n> > > value depending on the overall lux level. Replace the float\n> > > relativeLuminanceTraget by a PWL. As the PWL loading code loads a plain\n> > > value as single point PWL, backwards compatibility to existing tuning\n> > > files is ensured.\n> > >\n> > > While at it, order the class members in reverse xmas tree notation.\n> > >\n> > > Signed-off-by: Paul Elder \n> > > Signed-off-by: Stefan Klug \n> > > Reviewed-by: Daniel Scally \n> > >\n> > > ---\n> > >\n> > > Changes in v3:\n> > > - Dropped reference to original patch from commit message. For the\n> > > record, the old patch was https://patchwork.libcamera.org/patch/20231/\n> > > - Ordered member variables in reverse xmas tree order\n> > > - Fallback to kDefaultRelativeLuminanceTarget if relativeLuminanceTraget\n> > > is missing in the tuning file\n> > > - Warn once if lux level is not available after a few frames\n> > >\n> > > Changes in v2:\n> > > - Collected tags\n> > > - Dropped special handling of plain values, as this is now part of the\n> > > yaml PWL loader.\n> > > ---\n> > > src/ipa/libipa/agc_mean_luminance.cpp | 67 ++++++++++++++++++++++++---\n> > > src/ipa/libipa/agc_mean_luminance.h | 14 ++++--\n> > > src/ipa/rkisp1/algorithms/agc.cpp | 1 +\n> > > 3 files changed, 72 insertions(+), 10 deletions(-)\n> > >\n> > > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> > > index 64f36bd75dd2..4f2b5fad2082 100644\n> > > --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> > > +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> > > @@ -54,6 +54,14 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;\n> > > */\n> > > static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n> > >\n> > > +/*\n> > > + * Default lux level\n> > > + *\n> > > + * If no lux level or a zero lux level is specified, but PWLs are used to\n> > > + * specify luminance targets, this default level is used.\n> > > + */\n> > > +static constexpr unsigned int kDefaultLuxLevel = 500;\n> > > +\n> > > /**\n> > > * \\struct AgcMeanLuminance::AgcConstraint\n> > > * \\brief The boundaries and target for an AeConstraintMode constraint\n> > > @@ -144,17 +152,30 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n> > > */\n> > >\n> > > AgcMeanLuminance::AgcMeanLuminance()\n> > > - : exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s),\n> > > - relativeLuminanceTarget_(0)\n> > > + : filteredExposure_(0s), luxWarningEnabled_(true),\n> > > + exposureCompensation_(1.0), frameCount_(0), lux_(0)\n> > > {\n> > > }\n> > >\n> > > AgcMeanLuminance::~AgcMeanLuminance() = default;\n> > >\n> > > -void AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)\n> > > +int AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)\n> > > {\n> > > - relativeLuminanceTarget_ =\n> > > - tuningData[\"relativeLuminanceTarget\"].get(kDefaultRelativeLuminanceTarget);\n> > > + auto &target = tuningData[\"relativeLuminanceTarget\"];\n> > > + if (!target) {\n> > > + relativeLuminanceTarget_ = { { { { 0.0, kDefaultRelativeLuminanceTarget } } } };\n> > > + return 0;\n> > > + }\n> > > +\n> > > + auto pwl = target.get();\n> > > + if (!pwl) {\n> > > + LOG(AgcMeanLuminance, Error)\n> > > + << \"Failed to load relative luminance target.\";\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + relativeLuminanceTarget_ = std::move(*pwl);\n> > > + return 0;\n> > > }\n> > >\n> > > void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n> > > @@ -325,6 +346,8 @@ void AgcMeanLuminance::configure(utils::Duration lineDuration,\n> > > {\n> > > for (auto &[id, helper] : exposureModeHelpers_)\n> > > helper->configure(lineDuration, sensorHelper);\n> > > +\n> > > + luxWarningEnabled_ = true;\n> > > }\n> > >\n> > > /**\n> > > @@ -385,7 +408,9 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)\n> > > {\n> > > int ret;\n> > >\n> > > - parseRelativeLuminanceTarget(tuningData);\n> > > + ret = parseRelativeLuminanceTarget(tuningData);\n> > > + if (ret)\n> > > + return ret;\n> > >\n> > > ret = parseConstraintModes(tuningData);\n> > > if (ret)\n> > > @@ -403,6 +428,16 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)\n> > > * AGC calculations. It is expressed as gain instead of EV.\n> > > */\n> > >\n> > > +/**\n> > > + * \\fn AgcMeanLuminance::setLux(int lux)\n> > > + * \\brief Set the lux level\n> > > + * \\param[in] lux The lux level\n> > > + *\n> > > + * This function sets the lux level to be used in the AGC calculations. A value\n> > > + * of 0 means no measurement and a default value of \\a kDefaultLuxLevel is used\n> > > + * if necessary.\n> > > + */\n> > > +\n> > > /**\n> > > * \\brief Set the ExposureModeHelper limits for this class\n> > > * \\param[in] minExposureTime Minimum exposure time to allow\n> > > @@ -538,7 +573,25 @@ double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,\n> > > */\n> > > double AgcMeanLuminance::effectiveYTarget() const\n> > > {\n> > > - return std::min(relativeLuminanceTarget_ * exposureCompensation_,\n> > > + double lux = lux_;\n> > > + if (relativeLuminanceTarget_.size() > 1 && lux_ == 0) {\n> > > + if (frameCount_ > 10 && luxWarningEnabled_) {\n> >\n> > My intention here was to push for all the messages related to parsing\n> > errors to be emitted once at startup, but I understand this is not\n> > possible here as the error condition depends on two things:\n> >\n> > 1) relativeLuminanceTarget_ is specified as a PWL which associates a\n> > lux level to a desired Y target\n> > 2) the IPA doesn't call setLux()\n> >\n> > Now, while 1) is relatively easy to spot at parsing time 2) only\n> > depend on the IPA implementation.\n> >\n> > AgcMeanLuminance::effectiveYTarget() is called in 2 places by IPAs:\n> >\n> > 1) configure() where I guess it's ok not to have a lux level set yet ?\n> > 2) process() where it is expected to be always called after setLux() ?\n> >\n> > if the above are true then I understand we should not Warn on\n> > configure but we should Warn on the very first frame goes through\n> > process for which effectiveYTarget() is called without a setLux() before?\n> >\n> > I'm not sure I get why 10 is used, if that's the case.\n>\n> 10 was arbitrarily chosen, so that we get at least something.\n> Reading your message I realize, that I was a few steps further and\n> already planning for the regulation rework which makes this hard to\n> follow.\n>\n> You are right, currently it is called within process() where the agc\n> calculation happens. The issue here is that this is plain wrong but\n> fixing needs to wait until after the regulation series.\n>\n> The agc calculation and also setLux() should happen in prepare(). Reason\n> is that for example frameContext.agc.exposureValue feeds into the agc\n> calculations. So we get stats for frame n, calculate new exposure\n> settings based on fc[n].agc.exposureValue and these will then be used in\n> prepare(n+lookahead). That is wrong.\n>\n> The correct thing to do would be to cache the stats but do the agc\n> calculations in prepare() so that fc[n+lookahed].agc.exposureValue feeds\n> into the calculation.\n>\n> When the above is fixed, there will be no lux value available for the\n> first few frames as prepare() is run for these frames before the stats\n> for frame 0 have arrived which is the time when the first lux value can\n> be calculated.\n\nCouldn't we then avoid all error condition by:\n\n- coalesce setLux() in effectiveYTarget() as Barnabas suggested by\n making the Lux value an (optional<>?) parameter\n- use a default Lux value in the caller until we don't get a real lux\n value OR pass nullopt\n\nThis will make mandatory for the IPA to pass a lux in. Is it something\nthat you would expect ?\n\n>\n> 10 is bigger than the number of frames that get prepared in advance and\n> is small enough to show up early on.\n>\n> Does that make sense?\n>\n> Best regards,\n> Stefan\n>\n> >\n> > Thanks\n> > j\n> >\n> > > + luxWarningEnabled_ = false;\n> > > + LOG(AgcMeanLuminance, Warning)\n> > > + << \"Missing lux value for luminance target \"\n> > > + \"calculation, default to \"\n> > > + << kDefaultLuxLevel\n> > > + << \". Note that the Lux algorithm must be \"\n> > > + \"included before the Agc algorithm.\";\n> > > + }\n> > > +\n> > > + lux = kDefaultLuxLevel;\n> > > + }\n> > > +\n> > > + double luminanceTarget = relativeLuminanceTarget_.eval(\n> > > + relativeLuminanceTarget_.domain().clamp(lux));\n> > > +\n> > > + return std::min(luminanceTarget * exposureCompensation_,\n> > > kMaxRelativeLuminanceTarget);\n> > > }\n> > >\n> > > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n> > > index e5f164c3186b..746b97b16ffe 100644\n> > > --- a/src/ipa/libipa/agc_mean_luminance.h\n> > > +++ b/src/ipa/libipa/agc_mean_luminance.h\n> > > @@ -20,6 +20,7 @@\n> > >\n> > > #include \"exposure_mode_helper.h\"\n> > > #include \"histogram.h\"\n> > > +#include \"pwl.h\"\n> > >\n> > > namespace libcamera {\n> > >\n> > > @@ -50,6 +51,11 @@ public:\n> > > exposureCompensation_ = gain;\n> > > }\n> > >\n> > > + void setLux(unsigned int lux)\n> > > + {\n> > > + lux_ = lux;\n> > > + }\n> > > +\n> > > void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,\n> > > double minGain, double maxGain, std::vector constraints);\n> > >\n> > > @@ -82,7 +88,7 @@ public:\n> > > private:\n> > > virtual double estimateLuminance(const double gain) const = 0;\n> > >\n> > > - void parseRelativeLuminanceTarget(const YamlObject &tuningData);\n> > > + int parseRelativeLuminanceTarget(const YamlObject &tuningData);\n> > > void parseConstraint(const YamlObject &modeDict, int32_t id);\n> > > int parseConstraintModes(const YamlObject &tuningData);\n> > > int parseExposureModes(const YamlObject &tuningData);\n> > > @@ -92,10 +98,12 @@ private:\n> > > double gain);\n> > > utils::Duration filterExposure(utils::Duration exposureValue);\n> > >\n> > > + utils::Duration filteredExposure_;\n> > > + mutable bool luxWarningEnabled_;\n> > > double exposureCompensation_;\n> > > + Pwl relativeLuminanceTarget_;\n> > > uint64_t frameCount_;\n> > > - utils::Duration filteredExposure_;\n> > > - double relativeLuminanceTarget_;\n> > > + unsigned int lux_;\n> > >\n> > > std::vector additionalConstraints_;\n> > > std::map> constraintModes_;\n> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > index f5a3c917cb69..1ecaff680978 100644\n> > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > @@ -618,6 +618,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > effectiveExposureValue *= frameContext.agc.quantizationGain;\n> > >\n> > > setExposureCompensation(pow(2.0, frameContext.agc.exposureValue));\n> > > + setLux(frameContext.lux.lux);\n> > >\n> > > utils::Duration newExposureTime;\n> > > double aGain, qGain, dGain;\n> > > --\n> > > 2.51.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 8BDD7C0F1B\n\tfor ;\n\tFri, 21 Nov 2025 09:05:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4973360A80;\n\tFri, 21 Nov 2025 10:05:03 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EA25E608CF\n\tfor ;\n\tFri, 21 Nov 2025 10:05:01 +0100 (CET)","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 B90026A6;\n\tFri, 21 Nov 2025 10:02:55 +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=\"d8tDIzv6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763715775;\n\tbh=WhuGbgh5gFpCLRu1z68cwMs6we6hoXVDBg9rtiXgaC4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=d8tDIzv6dFM0woOB1B4ap4iUlnf+wEch5z0+A6Dz9DZsViqQQfybGz0GE8p2nw6/2\n\tMCEq44bWkVS941apwKfCgs5MF043URdVnwAHZDdhYFF3mDk/+XMlIJflokRPMFgPgt\n\t0/eQjYeWoPMSqvoWEYojI2b0QGVnqntLxnE3QYO4=","Date":"Fri, 21 Nov 2025 10:04:58 +0100","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org,\n\tPaul Elder , \n\tDaniel Scally ","Subject":"Re: [PATCH v3 3/4] ipa: libipa: agc_mean_luminance: Change luminance\n\ttarget to piecewise linear function","Message-ID":"","References":"<20251119132221.2088013-1-stefan.klug@ideasonboard.com>\n\t<20251119132221.2088013-4-stefan.klug@ideasonboard.com>\n\t\n\t<176366126846.2926148.17733284974166953641@localhost>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<176366126846.2926148.17733284974166953641@localhost>","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":36971,"web_url":"https://patchwork.libcamera.org/comment/36971/","msgid":"<176372166934.2176992.5845923102613434505@localhost>","date":"2025-11-21T10:41:09","subject":"Re: [PATCH v3 3/4] ipa: libipa: agc_mean_luminance: Change luminance\n\ttarget to piecewise linear function","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Jacopo,\n\nQuoting Jacopo Mondi (2025-11-21 10:04:58)\n> Hi Stefan\n> \n> On Thu, Nov 20, 2025 at 06:54:28PM +0100, Stefan Klug wrote:\n> > Hi Jacopo,\n> >\n> > Quoting Jacopo Mondi (2025-11-20 18:05:41)\n> > > Hi Stefan\n> > >\n> > > On Wed, Nov 19, 2025 at 02:22:12PM +0100, Stefan Klug wrote:\n> > > > In some situations it is necessary to specify the target brightness\n> > > > value depending on the overall lux level. Replace the float\n> > > > relativeLuminanceTraget by a PWL. As the PWL loading code loads a plain\n> > > > value as single point PWL, backwards compatibility to existing tuning\n> > > > files is ensured.\n> > > >\n> > > > While at it, order the class members in reverse xmas tree notation.\n> > > >\n> > > > Signed-off-by: Paul Elder \n> > > > Signed-off-by: Stefan Klug \n> > > > Reviewed-by: Daniel Scally \n> > > >\n> > > > ---\n> > > >\n> > > > Changes in v3:\n> > > > - Dropped reference to original patch from commit message. For the\n> > > > record, the old patch was https://patchwork.libcamera.org/patch/20231/\n> > > > - Ordered member variables in reverse xmas tree order\n> > > > - Fallback to kDefaultRelativeLuminanceTarget if relativeLuminanceTraget\n> > > > is missing in the tuning file\n> > > > - Warn once if lux level is not available after a few frames\n> > > >\n> > > > Changes in v2:\n> > > > - Collected tags\n> > > > - Dropped special handling of plain values, as this is now part of the\n> > > > yaml PWL loader.\n> > > > ---\n> > > > src/ipa/libipa/agc_mean_luminance.cpp | 67 ++++++++++++++++++++++++---\n> > > > src/ipa/libipa/agc_mean_luminance.h | 14 ++++--\n> > > > src/ipa/rkisp1/algorithms/agc.cpp | 1 +\n> > > > 3 files changed, 72 insertions(+), 10 deletions(-)\n> > > >\n> > > > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> > > > index 64f36bd75dd2..4f2b5fad2082 100644\n> > > > --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> > > > +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> > > > @@ -54,6 +54,14 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;\n> > > > */\n> > > > static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n> > > >\n> > > > +/*\n> > > > + * Default lux level\n> > > > + *\n> > > > + * If no lux level or a zero lux level is specified, but PWLs are used to\n> > > > + * specify luminance targets, this default level is used.\n> > > > + */\n> > > > +static constexpr unsigned int kDefaultLuxLevel = 500;\n> > > > +\n> > > > /**\n> > > > * \\struct AgcMeanLuminance::AgcConstraint\n> > > > * \\brief The boundaries and target for an AeConstraintMode constraint\n> > > > @@ -144,17 +152,30 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n> > > > */\n> > > >\n> > > > AgcMeanLuminance::AgcMeanLuminance()\n> > > > - : exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s),\n> > > > - relativeLuminanceTarget_(0)\n> > > > + : filteredExposure_(0s), luxWarningEnabled_(true),\n> > > > + exposureCompensation_(1.0), frameCount_(0), lux_(0)\n> > > > {\n> > > > }\n> > > >\n> > > > AgcMeanLuminance::~AgcMeanLuminance() = default;\n> > > >\n> > > > -void AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)\n> > > > +int AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)\n> > > > {\n> > > > - relativeLuminanceTarget_ =\n> > > > - tuningData[\"relativeLuminanceTarget\"].get(kDefaultRelativeLuminanceTarget);\n> > > > + auto &target = tuningData[\"relativeLuminanceTarget\"];\n> > > > + if (!target) {\n> > > > + relativeLuminanceTarget_ = { { { { 0.0, kDefaultRelativeLuminanceTarget } } } };\n> > > > + return 0;\n> > > > + }\n> > > > +\n> > > > + auto pwl = target.get();\n> > > > + if (!pwl) {\n> > > > + LOG(AgcMeanLuminance, Error)\n> > > > + << \"Failed to load relative luminance target.\";\n> > > > + return -EINVAL;\n> > > > + }\n> > > > +\n> > > > + relativeLuminanceTarget_ = std::move(*pwl);\n> > > > + return 0;\n> > > > }\n> > > >\n> > > > void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n> > > > @@ -325,6 +346,8 @@ void AgcMeanLuminance::configure(utils::Duration lineDuration,\n> > > > {\n> > > > for (auto &[id, helper] : exposureModeHelpers_)\n> > > > helper->configure(lineDuration, sensorHelper);\n> > > > +\n> > > > + luxWarningEnabled_ = true;\n> > > > }\n> > > >\n> > > > /**\n> > > > @@ -385,7 +408,9 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)\n> > > > {\n> > > > int ret;\n> > > >\n> > > > - parseRelativeLuminanceTarget(tuningData);\n> > > > + ret = parseRelativeLuminanceTarget(tuningData);\n> > > > + if (ret)\n> > > > + return ret;\n> > > >\n> > > > ret = parseConstraintModes(tuningData);\n> > > > if (ret)\n> > > > @@ -403,6 +428,16 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)\n> > > > * AGC calculations. It is expressed as gain instead of EV.\n> > > > */\n> > > >\n> > > > +/**\n> > > > + * \\fn AgcMeanLuminance::setLux(int lux)\n> > > > + * \\brief Set the lux level\n> > > > + * \\param[in] lux The lux level\n> > > > + *\n> > > > + * This function sets the lux level to be used in the AGC calculations. A value\n> > > > + * of 0 means no measurement and a default value of \\a kDefaultLuxLevel is used\n> > > > + * if necessary.\n> > > > + */\n> > > > +\n> > > > /**\n> > > > * \\brief Set the ExposureModeHelper limits for this class\n> > > > * \\param[in] minExposureTime Minimum exposure time to allow\n> > > > @@ -538,7 +573,25 @@ double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,\n> > > > */\n> > > > double AgcMeanLuminance::effectiveYTarget() const\n> > > > {\n> > > > - return std::min(relativeLuminanceTarget_ * exposureCompensation_,\n> > > > + double lux = lux_;\n> > > > + if (relativeLuminanceTarget_.size() > 1 && lux_ == 0) {\n> > > > + if (frameCount_ > 10 && luxWarningEnabled_) {\n> > >\n> > > My intention here was to push for all the messages related to parsing\n> > > errors to be emitted once at startup, but I understand this is not\n> > > possible here as the error condition depends on two things:\n> > >\n> > > 1) relativeLuminanceTarget_ is specified as a PWL which associates a\n> > > lux level to a desired Y target\n> > > 2) the IPA doesn't call setLux()\n> > >\n> > > Now, while 1) is relatively easy to spot at parsing time 2) only\n> > > depend on the IPA implementation.\n> > >\n> > > AgcMeanLuminance::effectiveYTarget() is called in 2 places by IPAs:\n> > >\n> > > 1) configure() where I guess it's ok not to have a lux level set yet ?\n> > > 2) process() where it is expected to be always called after setLux() ?\n> > >\n> > > if the above are true then I understand we should not Warn on\n> > > configure but we should Warn on the very first frame goes through\n> > > process for which effectiveYTarget() is called without a setLux() before?\n> > >\n> > > I'm not sure I get why 10 is used, if that's the case.\n> >\n> > 10 was arbitrarily chosen, so that we get at least something.\n> > Reading your message I realize, that I was a few steps further and\n> > already planning for the regulation rework which makes this hard to\n> > follow.\n> >\n> > You are right, currently it is called within process() where the agc\n> > calculation happens. The issue here is that this is plain wrong but\n> > fixing needs to wait until after the regulation series.\n> >\n> > The agc calculation and also setLux() should happen in prepare(). Reason\n> > is that for example frameContext.agc.exposureValue feeds into the agc\n> > calculations. So we get stats for frame n, calculate new exposure\n> > settings based on fc[n].agc.exposureValue and these will then be used in\n> > prepare(n+lookahead). That is wrong.\n> >\n> > The correct thing to do would be to cache the stats but do the agc\n> > calculations in prepare() so that fc[n+lookahed].agc.exposureValue feeds\n> > into the calculation.\n> >\n> > When the above is fixed, there will be no lux value available for the\n> > first few frames as prepare() is run for these frames before the stats\n> > for frame 0 have arrived which is the time when the first lux value can\n> > be calculated.\n> \n> Couldn't we then avoid all error condition by:\n> \n> - coalesce setLux() in effectiveYTarget() as Barnabas suggested by\n> making the Lux value an (optional<>?) parameter\n> - use a default Lux value in the caller until we don't get a real lux\n> value OR pass nullopt\n\nWe can do that but then the (as we see not trivial) error logging would\nalso be on the caller side. The warning was intended for cases when:\n- The user intended to use a lux dependent yTarget by providing the PWL\n in the tuning file\n- And the user did not put the lux algorithm before the Agc, so that lux\n won't be set.\n\nIn this case the user would most likely not notice that anything is\nwrong as these detailed regulations are difficult to test. So a warning\nmight help.\n\n> \n> This will make mandatory for the IPA to pass a lux in. Is it something\n> that you would expect ?\n\nThe reason for not making them parameters but passing them in via a side\nchannel was to not force us to implement lux on all platforms at the\nsame time (or splatter todos all over the place). Same for exposure\nvalue.\n\nAnother option might be to pass in a configuration struct instead of\nsingle parameters, so that we don't have to touch every IPA, when we\nadd additional features.\n\nI don't think we should mandate a lux value. I'd be fine with a\nstd::optional but then imho in a config struct. But maybe on a separate\nseries?\n\nBest regards,\nStefan\n\n> \n> >\n> > 10 is bigger than the number of frames that get prepared in advance and\n> > is small enough to show up early on.\n> >\n> > Does that make sense?\n> >\n> > Best regards,\n> > Stefan\n> >\n> > >\n> > > Thanks\n> > > j\n> > >\n> > > > + luxWarningEnabled_ = false;\n> > > > + LOG(AgcMeanLuminance, Warning)\n> > > > + << \"Missing lux value for luminance target \"\n> > > > + \"calculation, default to \"\n> > > > + << kDefaultLuxLevel\n> > > > + << \". Note that the Lux algorithm must be \"\n> > > > + \"included before the Agc algorithm.\";\n> > > > + }\n> > > > +\n> > > > + lux = kDefaultLuxLevel;\n> > > > + }\n> > > > +\n> > > > + double luminanceTarget = relativeLuminanceTarget_.eval(\n> > > > + relativeLuminanceTarget_.domain().clamp(lux));\n> > > > +\n> > > > + return std::min(luminanceTarget * exposureCompensation_,\n> > > > kMaxRelativeLuminanceTarget);\n> > > > }\n> > > >\n> > > > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n> > > > index e5f164c3186b..746b97b16ffe 100644\n> > > > --- a/src/ipa/libipa/agc_mean_luminance.h\n> > > > +++ b/src/ipa/libipa/agc_mean_luminance.h\n> > > > @@ -20,6 +20,7 @@\n> > > >\n> > > > #include \"exposure_mode_helper.h\"\n> > > > #include \"histogram.h\"\n> > > > +#include \"pwl.h\"\n> > > >\n> > > > namespace libcamera {\n> > > >\n> > > > @@ -50,6 +51,11 @@ public:\n> > > > exposureCompensation_ = gain;\n> > > > }\n> > > >\n> > > > + void setLux(unsigned int lux)\n> > > > + {\n> > > > + lux_ = lux;\n> > > > + }\n> > > > +\n> > > > void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,\n> > > > double minGain, double maxGain, std::vector constraints);\n> > > >\n> > > > @@ -82,7 +88,7 @@ public:\n> > > > private:\n> > > > virtual double estimateLuminance(const double gain) const = 0;\n> > > >\n> > > > - void parseRelativeLuminanceTarget(const YamlObject &tuningData);\n> > > > + int parseRelativeLuminanceTarget(const YamlObject &tuningData);\n> > > > void parseConstraint(const YamlObject &modeDict, int32_t id);\n> > > > int parseConstraintModes(const YamlObject &tuningData);\n> > > > int parseExposureModes(const YamlObject &tuningData);\n> > > > @@ -92,10 +98,12 @@ private:\n> > > > double gain);\n> > > > utils::Duration filterExposure(utils::Duration exposureValue);\n> > > >\n> > > > + utils::Duration filteredExposure_;\n> > > > + mutable bool luxWarningEnabled_;\n> > > > double exposureCompensation_;\n> > > > + Pwl relativeLuminanceTarget_;\n> > > > uint64_t frameCount_;\n> > > > - utils::Duration filteredExposure_;\n> > > > - double relativeLuminanceTarget_;\n> > > > + unsigned int lux_;\n> > > >\n> > > > std::vector additionalConstraints_;\n> > > > std::map> constraintModes_;\n> > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > index f5a3c917cb69..1ecaff680978 100644\n> > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > @@ -618,6 +618,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > effectiveExposureValue *= frameContext.agc.quantizationGain;\n> > > >\n> > > > setExposureCompensation(pow(2.0, frameContext.agc.exposureValue));\n> > > > + setLux(frameContext.lux.lux);\n> > > >\n> > > > utils::Duration newExposureTime;\n> > > > double aGain, qGain, dGain;\n> > > > --\n> > > > 2.51.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 92109C3335\n\tfor ;\n\tFri, 21 Nov 2025 10:41:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A398760A8B;\n\tFri, 21 Nov 2025 11:41:13 +0100 (CET)","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 07FCA608CF\n\tfor ;\n\tFri, 21 Nov 2025 11:41:12 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:5cfa:32ee:2ce5:d89f])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id B084C66B;\n\tFri, 21 Nov 2025 11:39:05 +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=\"SVBRDFkI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763721545;\n\tbh=NlusKB86UHvOVXrmUSpPqPBfqG/GaEtY+zIbAMV2anA=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=SVBRDFkIqboSZ7IGRD3pzPcZIVlBrL3iJZzb9vfoJzn1PrSK1Y8W6W//aINDFKFLf\n\tunoYb94EwiZDu2hkwMag0W5JVCi8zSz+1jomqQde5wBHGF9Q7xz98Bzq79p85On9QP\n\tnR4NBqnpJGJEi9c1R28sBrq0htt7gC0zNHlerwn0=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20251119132221.2088013-1-stefan.klug@ideasonboard.com>\n\t<20251119132221.2088013-4-stefan.klug@ideasonboard.com>\n\t\n\t<176366126846.2926148.17733284974166953641@localhost>\n\t","Subject":"Re: [PATCH v3 3/4] ipa: libipa: agc_mean_luminance: Change luminance\n\ttarget to piecewise linear function","From":"Stefan Klug ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org,\n\tPaul Elder , \n\tDaniel Scally ","To":"Jacopo Mondi ","Date":"Fri, 21 Nov 2025 11:41:09 +0100","Message-ID":"<176372166934.2176992.5845923102613434505@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\" "}}]