[{"id":29230,"web_url":"https://patchwork.libcamera.org/comment/29230/","msgid":"<20240415142145.pte4imoauay3ymem@jasper>","date":"2024-04-15T14:21:45","subject":"Re: [PATCH 4/5] ipa: libipa: agc: Change luminance target to\n\tpiecewise linear function","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Paul,\n\nthanks for the patch.\n\nOn Fri, Apr 05, 2024 at 11:47:28PM +0900, Paul Elder wrote:\n> Change the relative luminance target from a scalar valur to a piecewise\n> linear function that needs to be sampled by the estimate lux value.\n> \n> Also chagne the rkisp1 and ipu3 IPAs according, as they use the libipa\n\ns/chagne/change/\n\n> agc. As they both don't yet have lux modules, hardcode them to a single\n> lux value for now.\n> \n> This affects the format of the tuning files, but as there aren't yet any\n> this shouldn't be an issue.\n> \n> Signed-off-by: Paul Elder \n> ---\n> src/ipa/ipu3/algorithms/agc.cpp | 5 ++++-\n> src/ipa/libipa/agc.cpp | 22 +++++++++++++++-------\n> src/ipa/libipa/agc.h | 7 ++++---\n> src/ipa/rkisp1/algorithms/agc.cpp | 5 ++++-\n> 4 files changed, 27 insertions(+), 12 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> index 08deff0c..8e07c89e 100644\n> --- a/src/ipa/ipu3/algorithms/agc.cpp\n> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> @@ -228,12 +228,15 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \tdouble analogueGain = frameContext.sensor.gain;\n> \tutils::Duration effectiveExposureValue = exposureTime * analogueGain;\n> \n> +\t/* \\todo Plumb in the lux value. Requires a lux algo + tuning */\n> +\tdouble lux = 400;\n> +\n> \tutils::Duration shutterTime;\n> \tdouble aGain, dGain;\n> \tstd::tie(shutterTime, aGain, dGain) =\n> \t\tcalculateNewEv(context.activeState.agc.constraintMode,\n> \t\t\t context.activeState.agc.exposureMode, hist_,\n> -\t\t\t effectiveExposureValue);\n> +\t\t\t effectiveExposureValue, lux);\n> \n> \tLOG(IPU3Agc, Debug)\n> \t\t<< \"Divided up shutter, analogue gain and digital gain are \"\n> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp\n> index af57a571..bcb036e6 100644\n> --- a/src/ipa/libipa/agc.cpp\n> +++ b/src/ipa/libipa/agc.cpp\n> @@ -110,7 +110,7 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;\n> */\n> \n> MeanLuminanceAgc::MeanLuminanceAgc()\n> -\t: frameCount_(0), filteredExposure_(0s), relativeLuminanceTarget_(0)\n> +\t: frameCount_(0), filteredExposure_(0s)\n> {\n> }\n> \n> @@ -120,8 +120,12 @@ MeanLuminanceAgc::MeanLuminanceAgc()\n> */\n> void MeanLuminanceAgc::parseRelativeLuminanceTarget(const YamlObject &tuningData)\n> {\n> -\trelativeLuminanceTarget_ =\n> -\t\ttuningData[\"relativeLuminanceTarget\"].get(kDefaultRelativeLuminanceTarget);\n> +\tint ret = relativeLuminanceTarget_.readYaml(tuningData[\"relativeLuminanceTarget\"]);\n> +\tif (ret == 0)\n> +\t\treturn;\n\nMaybe a warning would be nice, that we fall back to the default value.\n\n> +\n> +\tstd::vector points = { { 0, kDefaultRelativeLuminanceTarget } };\n> +\trelativeLuminanceTarget_ = Pwl(points);\n> }\n> \n> /**\n> @@ -378,6 +382,7 @@ int MeanLuminanceAgc::parseExposureModes(const YamlObject &tuningData)\n> /**\n> * \\brief Estimate the initial gain needed to achieve a relative luminance\n> * target\n> + * \\param[in] lux The lux value at which to sample the luminance target pwl\n> *\n> * To account for non-linearity caused by saturation, the value needs to be\n> * estimated in an iterative process, as multiplying by a gain will not increase\n> @@ -385,9 +390,10 @@ int MeanLuminanceAgc::parseExposureModes(const YamlObject &tuningData)\n> *\n> * \\return The calculated initial gain\n> */\n> -double MeanLuminanceAgc::estimateInitialGain()\n> +double MeanLuminanceAgc::estimateInitialGain(double lux)\n> {\n> -\tdouble yTarget = relativeLuminanceTarget_;\n> +\tdouble yTarget =\n> +\t\trelativeLuminanceTarget_.eval(relativeLuminanceTarget_.domain().clamp(lux));\n> \tdouble yGain = 1.0;\n> \n> \tfor (unsigned int i = 0; i < 8; i++) {\n> @@ -476,6 +482,7 @@ utils::Duration MeanLuminanceAgc::filterExposure(utils::Duration exposureValue)\n> *\t the calculated gain\n> * \\param[in] effectiveExposureValue The EV applied to the frame from which the\n> *\t statistics in use derive\n> + * \\param[in] lux The lux value at which to sample the luminance target pwl\n> *\n> * Calculate a new exposure value to try to obtain the target. The calculated\n> * exposure value is filtered to prevent rapid changes from frame to frame, and\n> @@ -487,7 +494,8 @@ std::tuple\n> MeanLuminanceAgc::calculateNewEv(uint32_t constraintModeIndex,\n> \t\t\t\t uint32_t exposureModeIndex,\n> \t\t\t\t const Histogram &yHist,\n> -\t\t\t\t utils::Duration effectiveExposureValue)\n> +\t\t\t\t utils::Duration effectiveExposureValue,\n> +\t\t\t\t double lux)\n> {\n> \t/*\n> \t * The pipeline handler should validate that we have received an allowed\n> @@ -496,7 +504,7 @@ MeanLuminanceAgc::calculateNewEv(uint32_t constraintModeIndex,\n> \tstd::shared_ptr exposureModeHelper =\n> \t\texposureModeHelpers_.at(exposureModeIndex);\n> \n> -\tdouble gain = estimateInitialGain();\n> +\tdouble gain = estimateInitialGain(lux);\n> \tgain = constraintClampGain(constraintModeIndex, yHist, gain);\n> \n> \t/*\n> diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h\n> index 902a359a..f187dbc8 100644\n> --- a/src/ipa/libipa/agc.h\n> +++ b/src/ipa/libipa/agc.h\n> @@ -16,6 +16,7 @@\n> \n> #include \"exposure_mode_helper.h\"\n> #include \"histogram.h\"\n> +#include \"pwl.h\"\n> \n> namespace libcamera {\n> \n> @@ -59,18 +60,18 @@ public:\n> \t}\n> \n> \tvirtual double estimateLuminance(const double gain) = 0;\n> -\tdouble estimateInitialGain();\n> +\tdouble estimateInitialGain(double lux);\n> \tdouble constraintClampGain(uint32_t constraintModeIndex,\n> \t\t\t\t const Histogram &hist,\n> \t\t\t\t double gain);\n> \tutils::Duration filterExposure(utils::Duration exposureValue);\n> \tstd::tuple\n> \tcalculateNewEv(uint32_t constraintModeIndex, uint32_t exposureModeIndex,\n> -\t\t const Histogram &yHist, utils::Duration effectiveExposureValue);\n> +\t\t const Histogram &yHist, utils::Duration effectiveExposureValue, double lux);\n> private:\n> \tuint64_t frameCount_;\n> \tutils::Duration filteredExposure_;\n> -\tdouble relativeLuminanceTarget_;\n> +\tPwl relativeLuminanceTarget_;\n> \n> \tstd::map> constraintModes_;\n> \tstd::map> exposureModeHelpers_;\n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 1dfc4aaa..a1b6eb39 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -389,12 +389,15 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \tdouble analogueGain = frameContext.sensor.gain;\n> \tutils::Duration effectiveExposureValue = exposureTime * analogueGain;\n> \n> +\t/* \\todo Plumb in the lux value. Requires a lux algo + tuning */\n> +\tdouble lux = 400;\n> +\n> \tutils::Duration shutterTime;\n> \tdouble aGain, dGain;\n> \tstd::tie(shutterTime, aGain, dGain) =\n> \t\tcalculateNewEv(context.activeState.agc.constraintMode,\n> \t\t\t context.activeState.agc.exposureMode, hist_,\n> -\t\t\t effectiveExposureValue);\n> +\t\t\t effectiveExposureValue, lux);\n\nI don't yet fully understand the benefits of the lux algo. But otherwise\nit looks reasonable to me.\n\nReviewed-by: Stefan Klug \n\nCheers,\nStefan\n\n> \n> \tLOG(RkISP1Agc, Debug)\n> \t\t<< \"Divided up shutter, analogue gain and digital gain are \"\n> -- \n> 2.39.2\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 B3EA2BE08B\n\tfor ;\n\tMon, 15 Apr 2024 14:21:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8FC1163352;\n\tMon, 15 Apr 2024 16:21:50 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 17ECE63339\n\tfor ;\n\tMon, 15 Apr 2024 16:21:49 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:7a0d:dd2e:881a:db83])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EF557236;\n\tMon, 15 Apr 2024 16:21:02 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"jrKQD0H5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1713190863;\n\tbh=TQBoMkz5FqApmKt+siR4dAGo9Ugnt4htS4aNNACRkWg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jrKQD0H5IK3O4kMkVMaSCeKJ1PFEQrtutS6+qdKBbZPm2CF1p4yZlhOBspzXxK9Lt\n\t11GCzIt8Fn2OJ2EfnB4Bk7AAGH6qNCBOWHiMMzcAzgFN8/oZ0SySCHd06x6ZJXaoja\n\t7Q6v7VeE3fnm7jH6olzWmGGG5NppL1pEK2f1H4mg=","Date":"Mon, 15 Apr 2024 16:21:45 +0200","From":"Stefan Klug ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 4/5] ipa: libipa: agc: Change luminance target to\n\tpiecewise linear function","Message-ID":"<20240415142145.pte4imoauay3ymem@jasper>","References":"<20240405144729.2992219-1-paul.elder@ideasonboard.com>\n\t<20240405144729.2992219-5-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240405144729.2992219-5-paul.elder@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\" "}}]