[{"id":36582,"web_url":"https://patchwork.libcamera.org/comment/36582/","msgid":"","date":"2025-10-31T15:45:21","subject":"Re: [PATCH v1 2/4] ipa: libipa: agc_mean_luminance: Change luminance\n\ttarget to piecewise linear function","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> In some situations it is necessary to specify the target brightness\n> value depending on the overall lux level. This is a rework [1] to fit\n> current master. For backwards compatibility of the tuning files and\n> easier tuning file handling it is still allowed to specify the luminance\n> target as plain value.\n> \n> [1] https://patchwork.libcamera.org/patch/20231/\n> \n> Signed-off-by: Paul Elder \n> Signed-off-by: Stefan Klug \n> ---\n> src/ipa/libipa/agc_mean_luminance.cpp | 59 ++++++++++++++++++++++++---\n> src/ipa/libipa/agc_mean_luminance.h | 11 ++++-\n> src/ipa/rkisp1/algorithms/agc.cpp | 1 +\n> 3 files changed, 63 insertions(+), 8 deletions(-)\n> \n> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> index 64f36bd75dd2..62b1918a45a7 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> @@ -145,16 +153,32 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n> \n> AgcMeanLuminance::AgcMeanLuminance()\n> \t: exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s),\n> -\t relativeLuminanceTarget_(0)\n> +\t 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> +\trelativeLuminanceTarget_.clear();\n> +\n> +\tauto &target = tuningData[\"relativeLuminanceTarget\"];\n> +\tif (target.isValue()) {\n> +\t\tdouble t = target.get(kDefaultRelativeLuminanceTarget);\n> +\t\trelativeLuminanceTarget_.append(0, t);\n\nI think possibly a comment to explain that the intent is to represent a legacy scalar target with a \nflat PWL may spare some confusion in the future...and possibly a warning should shout that it's in \nan inappropriate format here? Otherwise:\n\nReviewed-by: Daniel Scally \n\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tstd::optional 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_.swap(*pwl);\n> +\treturn 0;\n> }\n> \n> void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n> @@ -385,7 +409,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 +429,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 +574,18 @@ 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\tLOG(AgcMeanLuminance, Debug)\n> +\t\t\t<< \"Missing lux value for luminance target calculation, default to \"\n> +\t\t\t<< kDefaultLuxLevel;\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 d7ec548e3e58..1fbff304a72b 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> @@ -81,8 +87,8 @@ public:\n> \n> private:\n> \tvirtual double estimateLuminance(const double gain) const = 0;\n> +\tint parseRelativeLuminanceTarget(const YamlObject &tuningData);\n> \n> -\tvoid 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> @@ -95,7 +101,8 @@ private:\n> \tdouble exposureCompensation_;\n> \tuint64_t frameCount_;\n> \tutils::Duration filteredExposure_;\n> -\tdouble relativeLuminanceTarget_;\n> +\tunsigned int lux_;\n> +\tPwl relativeLuminanceTarget_;\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;","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 47C0CC3259\n\tfor ;\n\tFri, 31 Oct 2025 15:45:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4B74A609E7;\n\tFri, 31 Oct 2025 16:45:26 +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 84AB460947\n\tfor ;\n\tFri, 31 Oct 2025 16:45:24 +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 86EC813E2;\n\tFri, 31 Oct 2025 16:43:33 +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=\"T57EION3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1761925413;\n\tbh=kTPhn35T7/qvQEagxrVVcAA/YO6p09Sba9yQvg/0kpA=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=T57EION3ML6TlUwxbESr3TXtFGfiHkS271ozohccF7EpoudO7N9BncZtRrrdP/9CN\n\t0UxNqLR/HvnbYWqvqqIvMkuuFfKU6ZXw+dV4aqRkSlYUOuytjuX9MpfmBmbCGz3FO6\n\tI654WZXCKXh1TFtC3DSfjjZuRZr8HkbWH+VvCncI=","Message-ID":"","Date":"Fri, 31 Oct 2025 15:45:21 +0000","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1 2/4] ipa: libipa: agc_mean_luminance: Change luminance\n\ttarget to piecewise linear function","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Cc":"Paul Elder ","References":"<20251014142427.3107490-1-stefan.klug@ideasonboard.com>\n\t<20251014142427.3107490-3-stefan.klug@ideasonboard.com>","Content-Language":"en-US","From":"Dan Scally ","In-Reply-To":"<20251014142427.3107490-3-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\" "}}]