[{"id":29840,"web_url":"https://patchwork.libcamera.org/comment/29840/","msgid":"<20240611163134.GA23074@pendragon.ideasonboard.com>","date":"2024-06-11T16:31:34","subject":"Re: [PATCH v5 2/3] ipa: libipa: agc: Change luminance target to\n\tpiecewise linear function","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Fri, Jun 07, 2024 at 05:03:29PM +0900, Paul Elder wrote:\n> Change the relative luminance target from a scalar value to a piecewise\n> linear function that needs to be sampled by the estimate lux value.\n\nOne thing I would like to capture somewhere is the rationale for this.\nWhy should the target luminance (essentially the average Y value across\nthe image if we simplify it) depend on how bright the scene is ? I\nlooked at the RPi tuning files, and they push the target luminance\nslightly up the brighter the scene is. Is this because we want to image\nto appear brighter to a human eye when we know that the scene is brigher\n?\n\n> Also change the rkisp1 and ipu3 IPAs accordingly, as they use the libipa\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\nSounds fine, but I would still like to see documentation of the format\n:-) Or, as a first step, at least a sample tuning file in-tree.\n\n> Signed-off-by: Paul Elder \n> Reviewed-by: Stefan Klug \n> Reviewed-by: Daniel Scally \n> \n> ---\n> Changes in v5:\n> - use new PointF that's a typedef of Vector\n> \n> Changes in v4:\n> - construct default pwl with *two* points so that it succeeds\n> \n> No change in v3\n> \n> Changes in v2:\n> - s/FPoint/PointF/\n> - add warning when using default relative luminance target when loading\n> from the tuning file fails\n> ---\n> src/ipa/ipu3/algorithms/agc.cpp | 5 ++++-\n> src/ipa/libipa/agc_mean_luminance.cpp | 32 +++++++++++++++++++++------\n> src/ipa/libipa/agc_mean_luminance.h | 7 +++---\n> src/ipa/rkisp1/algorithms/agc.cpp | 5 ++++-\n> 4 files changed, 37 insertions(+), 12 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> index 0e0114f6d..7f71bec06 100644\n> --- a/src/ipa/ipu3/algorithms/agc.cpp\n> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> @@ -222,12 +222,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_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> index 271b5ae4b..10c16850d 100644\n> --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> @@ -134,7 +134,7 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;\n> */\n> \n> AgcMeanLuminance::AgcMeanLuminance()\n> -\t: frameCount_(0), filteredExposure_(0s), relativeLuminanceTarget_(0)\n> +\t: frameCount_(0), filteredExposure_(0s)\n> {\n> }\n> \n> @@ -142,8 +142,17 @@ AgcMeanLuminance::~AgcMeanLuminance() = default;\n> \n> void AgcMeanLuminance::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> +\n> +\tLOG(AgcMeanLuminance, Warning)\n> +\t\t<< \"Failed to load tuning parameter 'relativeLuminanceTarget', \"\n> +\t\t<< \"using default [0, \" << kDefaultRelativeLuminanceTarget << \"]\";\n> +\n> +\tstd::vector points = { Pwl::PointF({ 0, kDefaultRelativeLuminanceTarget }),\n> +\t\t\t\t\t Pwl::PointF({ 10000, kDefaultRelativeLuminanceTarget }) };\n> +\trelativeLuminanceTarget_ = Pwl(points);\n> }\n> \n> void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n> @@ -421,11 +430,18 @@ void AgcMeanLuminance::setLimits(utils::Duration minShutter,\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> + * the relative luminance by the same factor if some image regions are saturated\n\ns/$/./\n\nI would *really* like checkstyle to catch this, but parsing\ndocumentation isn't easy :-S\n\nThis being said, the same comment is already present inside the\nfunction. Is there a reason to duplicate it ?\n\n> + *\n> * \\return The calculated initial gain\n> */\n> -double AgcMeanLuminance::estimateInitialGain() const\n> +double AgcMeanLuminance::estimateInitialGain(double lux) const\n> {\n> -\tdouble yTarget = relativeLuminanceTarget_;\n> +\tdouble yTarget =\n> +\t\trelativeLuminanceTarget_.eval(relativeLuminanceTarget_.domain().clamp(lux));\n> \tdouble yGain = 1.0;\n> \n> \t/*\n> @@ -520,6 +536,7 @@ utils::Duration AgcMeanLuminance::filterExposure(utils::Duration exposureValue)\n> * the calculated gain\n> * \\param[in] effectiveExposureValue The EV applied to the frame from which the\n> * 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> @@ -531,7 +548,8 @@ std::tuple\n> AgcMeanLuminance::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> @@ -540,7 +558,7 @@ AgcMeanLuminance::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_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n> index 0a81c6d28..6ec2a0dc9 100644\n> --- a/src/ipa/libipa/agc_mean_luminance.h\n> +++ b/src/ipa/libipa/agc_mean_luminance.h\n> @@ -18,6 +18,7 @@\n> \n> #include \"exposure_mode_helper.h\"\n> #include \"histogram.h\"\n> +#include \"pwl.h\"\n> \n> namespace libcamera {\n> \n> @@ -62,7 +63,7 @@ public:\n> \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> \n> \tvoid resetFrameCount()\n> \t{\n> @@ -76,7 +77,7 @@ private:\n> \tvoid parseConstraint(const YamlObject &modeDict, int32_t id);\n> \tint parseConstraintModes(const YamlObject &tuningData);\n> \tint parseExposureModes(const YamlObject &tuningData);\n> -\tdouble estimateInitialGain() const;\n> +\tdouble estimateInitialGain(double lux) const;\n> \tdouble constraintClampGain(uint32_t constraintModeIndex,\n> \t\t\t\t const Histogram &hist,\n> \t\t\t\t double gain);\n> @@ -84,7 +85,7 @@ private:\n> \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 3bbafd978..e9f1f2095 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -379,12 +379,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,\n> -\t\t\t hist, effectiveExposureValue);\n> +\t\t\t hist, effectiveExposureValue, lux);\n> \n> \tLOG(RkISP1Agc, Debug)\n> \t\t<< \"Divided up shutter, analogue gain and digital gain are \"","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 04D24BD87C\n\tfor ;\n\tTue, 11 Jun 2024 16:31:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BDF4265456;\n\tTue, 11 Jun 2024 18:31:56 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9ADF661A26\n\tfor ;\n\tTue, 11 Jun 2024 18:31:54 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 75DDAA9A;\n\tTue, 11 Jun 2024 18:31:41 +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=\"fXYX8lWW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718123501;\n\tbh=epLBfq9AosvkDQrAR6erLYTeVm8EPdRBJ03/vXZUA08=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fXYX8lWW07YCMutkF8n8UbRzxfJ8FdIcDBvx/Pt/SreB9nPIMToqUXp8PJzXQu3Xn\n\tmdhR5f8PV6+EimwiCSdZKkVaNIr/6NA3uYvO/Y0RRdpiYCtpAas/6E1CKPB48ua49K\n\tznq2peu9spTCAzjQsjPfcmqJZo5aWfs4eC6M6b1k=","Date":"Tue, 11 Jun 2024 19:31:34 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org,\n\tStefan Klug ,\n\tDaniel Scally ","Subject":"Re: [PATCH v5 2/3] ipa: libipa: agc: Change luminance target to\n\tpiecewise linear function","Message-ID":"<20240611163134.GA23074@pendragon.ideasonboard.com>","References":"<20240607080330.2667994-1-paul.elder@ideasonboard.com>\n\t<20240607080330.2667994-3-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240607080330.2667994-3-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\" "}}]