[{"id":29553,"web_url":"https://patchwork.libcamera.org/comment/29553/","msgid":"","date":"2024-05-17T10:29:16","subject":"Re: [PATCH v3 2/3] ipa: libipa: agc: Change luminance target to\n\tpiecewise linear function","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Paul\n\nOn 17/05/2024 10:08, 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>\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>\n> Signed-off-by: Paul Elder \n> Reviewed-by: Stefan Klug \n\n\nThanks for the patch, this looks good to me. We need to remember the same thing for the constraint \ntarget though I guess.\n\n\nReviewed-by: Daniel Scally \n\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 | 31 +++++++++++++++++++++------\n> src/ipa/libipa/agc_mean_luminance.h | 7 +++---\n> src/ipa/rkisp1/algorithms/agc.cpp | 5 ++++-\n> 4 files changed, 36 insertions(+), 12 deletions(-)\n>\n> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> index c9b5548c4..984ed0874 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 2bf84d05b..fe07777dc 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,16 @@ 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 = { { 0, kDefaultRelativeLuminanceTarget } };\n> +\trelativeLuminanceTarget_ = Pwl(points);\n> }\n> \n> void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n> @@ -421,11 +429,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> + *\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 +535,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 +547,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 +557,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 4af397bdc..1c9872d02 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 B8888BD78E\n\tfor ;\n\tFri, 17 May 2024 10:29:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 009EF6347C;\n\tFri, 17 May 2024 12:29:20 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DA5A261A5A\n\tfor ;\n\tFri, 17 May 2024 12:29:19 +0200 (CEST)","from [10.91.1.31] (unknown [149.14.240.163])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C1B4D82E\n\tfor ;\n\tFri, 17 May 2024 12:29:10 +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=\"cm9y1ioN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1715941750;\n\tbh=V2ahgI7pmYtgWHpCrvUsHfu8a8LQda0b36owSUC9Fq0=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=cm9y1ioNWzXlbM7w2ITBXx5zTN4x7kX8rGwkxxiBVggrYDx7d3O6MSvInxNcNUXll\n\tmync8/EnVvB3xHHnqHdEH6XJUbQUY7Uj2SbF9hihI2d9oZ34Cq1Q8aij8oqlmRLEuD\n\t2Xo6FYJFvB9P6qrlQ77mNziwRJt/bLkBCcEdtumc=","Message-ID":"","Date":"Fri, 17 May 2024 12:29:16 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3 2/3] ipa: libipa: agc: Change luminance target to\n\tpiecewise linear function","To":"libcamera-devel@lists.libcamera.org","References":"<20240517080802.3896531-1-paul.elder@ideasonboard.com>\n\t<20240517080802.3896531-3-paul.elder@ideasonboard.com>","Content-Language":"en-US","From":"Dan Scally ","Autocrypt":"addr=dan.scally@ideasonboard.com; keydata=\n\txsFNBGLydlEBEADa5O2s0AbUguprfvXOQun/0a8y2Vk6BqkQALgeD6KnXSWwaoCULp18etYW\n\tB31bfgrdphXQ5kUQibB0ADK8DERB4wrzrUb5CMxLBFE7mQty+v5NsP0OFNK9XTaAOcmD+Ove\n\teIjYvqurAaro91jrRVrS1gBRxIFqyPgNvwwL+alMZhn3/2jU2uvBmuRrgnc/e9cHKiuT3Dtq\n\tMHGPKL2m+plk+7tjMoQFfexoQ1JKugHAjxAhJfrkXh6uS6rc01bYCyo7ybzg53m1HLFJdNGX\n\tsUKR+dQpBs3SY4s66tc1sREJqdYyTsSZf80HjIeJjU/hRunRo4NjRIJwhvnK1GyjOvvuCKVU\n\tRWpY8dNjNu5OeAfdrlvFJOxIE9M8JuYCQTMULqd1NuzbpFMjc9524U3Cngs589T7qUMPb1H1\n\tNTA81LmtJ6Y+IV5/kiTUANflpzBwhu18Ok7kGyCq2a2jsOcVmk8gZNs04gyjuj8JziYwwLbf\n\tvzABwpFVcS8aR+nHIZV1HtOzyw8CsL8OySc3K9y+Y0NRpziMRvutrppzgyMb9V+N31mK9Mxl\n\t1YkgaTl4ciNWpdfUe0yxH03OCuHi3922qhPLF4XX5LN+NaVw5Xz2o3eeWklXdouxwV7QlN33\n\tu4+u2FWzKxDqO6WLQGjxPE0mVB4Gh5Pa1Vb0ct9Ctg0qElvtGQARAQABzShEYW4gU2NhbGx5\n\tIDxkYW4uc2NhbGx5QGlkZWFzb25ib2FyZC5jb20+wsGNBBMBCAA3FiEEsdtt8OWP7+8SNfQe\n\tkiQuh/L+GMQFAmLydlIFCQWjmoACGwMECwkIBwUVCAkKCwUWAgMBAAAKCRCSJC6H8v4YxDI2\n\tEAC2Gz0iyaXJkPInyshrREEWbo0CA6v5KKf3I/HlMPqkZ48bmGoYm4mEQGFWZJAT3K4ir8bg\n\tcEfs9V54gpbrZvdwS4abXbUK4WjKwEs8HK3XJv1WXUN2bsz5oEJWZUImh9gD3naiLLI9QMMm\n\tw/aZkT+NbN5/2KvChRWhdcha7+2Te4foOY66nIM+pw2FZM6zIkInLLUik2zXOhaZtqdeJZQi\n\tHSPU9xu7TRYN4cvdZAnSpG7gQqmLm5/uGZN1/sB3kHTustQtSXKMaIcD/DMNI3JN/t+RJVS7\n\tc0Jh/ThzTmhHyhxx3DRnDIy7kwMI4CFvmhkVC2uNs9kWsj1DuX5kt8513mvfw2OcX9UnNKmZ\n\tnhNCuF6DxVrL8wjOPuIpiEj3V+K7DFF1Cxw1/yrLs8dYdYh8T8vCY2CHBMsqpESROnTazboh\n\tAiQ2xMN1cyXtX11Qwqm5U3sykpLbx2BcmUUUEAKNsM//Zn81QXKG8vOx0ZdMfnzsCaCzt8f6\n\t9dcDBBI3tJ0BI9ByiocqUoL6759LM8qm18x3FYlxvuOs4wSGPfRVaA4yh0pgI+ModVC2Pu3y\n\tejE/IxeatGqJHh6Y+iJzskdi27uFkRixl7YJZvPJAbEn7kzSi98u/5ReEA8Qhc8KO/B7wprj\n\txjNMZNYd0Eth8+WkixHYj752NT5qshKJXcyUU87BTQRi8nZSARAAx0BJayh1Fhwbf4zoY56x\n\txHEpT6DwdTAYAetd3yiKClLVJadYxOpuqyWa1bdfQWPb+h4MeXbWw/53PBgn7gI2EA7ebIRC\n\tPJJhAIkeym7hHZoxqDQTGDJjxFEL11qF+U3rhWiL2Zt0Pl+zFq0eWYYVNiXjsIS4FI2+4m16\n\ttPbDWZFJnSZ828VGtRDQdhXfx3zyVX21lVx1bX4/OZvIET7sVUufkE4hrbqrrufre7wsjD1t\n\t8MQKSapVrr1RltpzPpScdoxknOSBRwOvpp57pJJe5A0L7+WxJ+vQoQXj0j+5tmIWOAV1qBQp\n\thyoyUk9JpPfntk2EKnZHWaApFp5TcL6c5LhUvV7F6XwOjGPuGlZQCWXee9dr7zym8iR3irWT\n\t+49bIh5PMlqSLXJDYbuyFQHFxoiNdVvvf7etvGfqFYVMPVjipqfEQ38ST2nkzx+KBICz7uwj\n\tJwLBdTXzGFKHQNckGMl7F5QdO/35An/QcxBnHVMXqaSd12tkJmoRVWduwuuoFfkTY5mUV3uX\n\txGj3iVCK4V+ezOYA7c2YolfRCNMTza6vcK/P4tDjjsyBBZrCCzhBvd4VVsnnlZhVaIxoky4K\n\taL+AP+zcQrUZmXmgZjXOLryGnsaeoVrIFyrU6ly90s1y3KLoPsDaTBMtnOdwxPmo1xisH8oL\n\ta/VRgpFBfojLPxMAEQEAAcLBfAQYAQgAJhYhBLHbbfDlj+/vEjX0HpIkLofy/hjEBQJi8nZT\n\tBQkFo5qAAhsMAAoJEJIkLofy/hjEXPcQAMIPNqiWiz/HKu9W4QIf1OMUpKn3YkVIj3p3gvfM\n\tRes4fGX94Ji599uLNrPoxKyaytC4R6BTxVriTJjWK8mbo9jZIRM4vkwkZZ2bu98EweSucxbp\n\tvjESsvMXGgxniqV/RQ/3T7LABYRoIUutARYq58p5HwSP0frF0fdFHYdTa2g7MYZl1ur2JzOC\n\tFHRpGadlNzKDE3fEdoMobxHB3Lm6FDml5GyBAA8+dQYVI0oDwJ3gpZPZ0J5Vx9RbqXe8RDuR\n\tdu90hvCJkq7/tzSQ0GeD3BwXb9/R/A4dVXhaDd91Q1qQXidI+2jwhx8iqiYxbT+DoAUkQRQy\n\txBtoCM1CxH7u45URUgD//fxYr3D4B1SlonA6vdaEdHZOGwECnDpTxecENMbz/Bx7qfrmd901\n\tD+N9SjIwrbVhhSyUXYnSUb8F+9g2RDY42Sk7GcYxIeON4VzKqWM7hpkXZ47pkK0YodO+dRKM\n\tyMcoUWrTK0Uz6UzUGKoJVbxmSW/EJLEGoI5p3NWxWtScEVv8mO49gqQdrRIOheZycDmHnItt\n\t9Qjv00uFhEwv2YfiyGk6iGF2W40s2pH2t6oeuGgmiZ7g6d0MEK8Ql/4zPItvr1c1rpwpXUC1\n\tu1kQWgtnNjFHX3KiYdqjcZeRBiry1X0zY+4Y24wUU0KsEewJwjhmCKAsju1RpdlPg2kC","In-Reply-To":"<20240517080802.3896531-3-paul.elder@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29556,"web_url":"https://patchwork.libcamera.org/comment/29556/","msgid":"","date":"2024-05-17T11:47:44","subject":"Re: [PATCH v3 2/3] ipa: libipa: agc: Change luminance target to\n\tpiecewise linear function","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Fri, May 17, 2024 at 12:29:16PM +0200, Dan Scally wrote:\n> Hi Paul\n> \n> On 17/05/2024 10:08, 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> > \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> > \n> > Signed-off-by: Paul Elder \n> > Reviewed-by: Stefan Klug \n> \n> \n> Thanks for the patch, this looks good to me. We need to remember the same\n> thing for the constraint target though I guess.\n\nYeah I just remembered that I had meant to squash that patch in with\nthis, but ig I can send it separately.\n\n\nThanks,\n\nPaul\n\n> \n> \n> Reviewed-by: Daniel Scally \n> \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 | 31 +++++++++++++++++++++------\n> > src/ipa/libipa/agc_mean_luminance.h | 7 +++---\n> > src/ipa/rkisp1/algorithms/agc.cpp | 5 ++++-\n> > 4 files changed, 36 insertions(+), 12 deletions(-)\n> > \n> > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> > index c9b5548c4..984ed0874 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> > +\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> > \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 2bf84d05b..fe07777dc 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> > AgcMeanLuminance::AgcMeanLuminance()\n> > -\t: frameCount_(0), filteredExposure_(0s), relativeLuminanceTarget_(0)\n> > +\t: frameCount_(0), filteredExposure_(0s)\n> > {\n> > }\n> > @@ -142,8 +142,16 @@ AgcMeanLuminance::~AgcMeanLuminance() = default;\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 = { { 0, kDefaultRelativeLuminanceTarget } };\n> > +\trelativeLuminanceTarget_ = Pwl(points);\n> > }\n> > void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n> > @@ -421,11 +429,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> > + *\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> > \t/*\n> > @@ -520,6 +535,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 +547,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 +557,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,\n> > \tstd::shared_ptr exposureModeHelper =\n> > \t\texposureModeHelpers_.at(exposureModeIndex);\n> > -\tdouble gain = estimateInitialGain();\n> > +\tdouble gain = estimateInitialGain(lux);\n> > \tgain = constraintClampGain(constraintModeIndex, yHist, gain);\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> > #include \"exposure_mode_helper.h\"\n> > #include \"histogram.h\"\n> > +#include \"pwl.h\"\n> > namespace libcamera {\n> > @@ -62,7 +63,7 @@ public:\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> > \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> > \tuint64_t frameCount_;\n> > \tutils::Duration filteredExposure_;\n> > -\tdouble relativeLuminanceTarget_;\n> > +\tPwl relativeLuminanceTarget_;\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 4af397bdc..1c9872d02 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> > +\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> > \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 E7509BD78E\n\tfor ;\n\tFri, 17 May 2024 11:47:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 939156347C;\n\tFri, 17 May 2024 13:47:53 +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 A4ADB61A5A\n\tfor ;\n\tFri, 17 May 2024 13:47:51 +0200 (CEST)","from pyrite.rasen.tech (h175-177-049-156.catv02.itscom.jp\n\t[175.177.49.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 81132A9A;\n\tFri, 17 May 2024 13:47: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=\"SoHnAKz8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1715946462;\n\tbh=dT7KuA2RpN6Fg2zcJ4VYRUmw5+vsgJxf1GHh6d0Av6A=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SoHnAKz8TDlpuh0KQp1U/t7N0bsSoZ1nbYqtzSLuXycG9m8gYTl/Lzs32uzS6f0eG\n\tDcVX2rGFBDdINO81y2SnJhXE5nvNQJSa30THJSXKQAXVR5B1NHIKj5oxWpahRkr35M\n\tsD9JyCVC5GtNSV97dhgHT6IXKwF+uiEBgAYumgHo=","Date":"Fri, 17 May 2024 20:47:44 +0900","From":"Paul Elder ","To":"Dan Scally ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3 2/3] ipa: libipa: agc: Change luminance target to\n\tpiecewise linear function","Message-ID":"","References":"<20240517080802.3896531-1-paul.elder@ideasonboard.com>\n\t<20240517080802.3896531-3-paul.elder@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","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":29574,"web_url":"https://patchwork.libcamera.org/comment/29574/","msgid":"<564fc395-c4c0-4fe7-908e-510a15668dad@ideasonboard.com>","date":"2024-05-20T13:12:26","subject":"Re: [PATCH v3 2/3] ipa: libipa: agc: Change luminance target to\n\tpiecewise linear function","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"On 17/05/2024 12:47, Paul Elder wrote:\n> On Fri, May 17, 2024 at 12:29:16PM +0200, Dan Scally wrote:\n>> Hi Paul\n>>\n>> On 17/05/2024 10:08, 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>>>\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>>>\n>>> Signed-off-by: Paul Elder \n>>> Reviewed-by: Stefan Klug \n>>\n>> Thanks for the patch, this looks good to me. We need to remember the same\n>> thing for the constraint target though I guess.\n> Yeah I just remembered that I had meant to squash that patch in with\n> this, but ig I can send it separately.\n\n\nActually, one quick thought below...\n\n>\n>\n> Thanks,\n>\n> Paul\n>\n>>\n>> Reviewed-by: Daniel Scally \n>>\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 | 31 +++++++++++++++++++++------\n>>> src/ipa/libipa/agc_mean_luminance.h | 7 +++---\n>>> src/ipa/rkisp1/algorithms/agc.cpp | 5 ++++-\n>>> 4 files changed, 36 insertions(+), 12 deletions(-)\n>>>\n>>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n>>> index c9b5548c4..984ed0874 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>>> +\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>>> \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 2bf84d05b..fe07777dc 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>>> AgcMeanLuminance::AgcMeanLuminance()\n>>> -\t: frameCount_(0), filteredExposure_(0s), relativeLuminanceTarget_(0)\n>>> +\t: frameCount_(0), filteredExposure_(0s)\n>>> {\n>>> }\n>>> @@ -142,8 +142,16 @@ AgcMeanLuminance::~AgcMeanLuminance() = default;\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 = { { 0, kDefaultRelativeLuminanceTarget } };\n>>> +\trelativeLuminanceTarget_ = Pwl(points);\n\n\nDoes this not need to have two points? The Pwl implementation looks like it expects an even number \nof points with a minimum of 2 - validation would fail in the readYaml() if that weren't met.\n\n>>> }\n>>> void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n>>> @@ -421,11 +429,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>>> + *\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>>> \t/*\n>>> @@ -520,6 +535,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 +547,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 +557,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,\n>>> \tstd::shared_ptr exposureModeHelper =\n>>> \t\texposureModeHelpers_.at(exposureModeIndex);\n>>> -\tdouble gain = estimateInitialGain();\n>>> +\tdouble gain = estimateInitialGain(lux);\n>>> \tgain = constraintClampGain(constraintModeIndex, yHist, gain);\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>>> #include \"exposure_mode_helper.h\"\n>>> #include \"histogram.h\"\n>>> +#include \"pwl.h\"\n>>> namespace libcamera {\n>>> @@ -62,7 +63,7 @@ public:\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>>> \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>>> \tuint64_t frameCount_;\n>>> \tutils::Duration filteredExposure_;\n>>> -\tdouble relativeLuminanceTarget_;\n>>> +\tPwl relativeLuminanceTarget_;\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 4af397bdc..1c9872d02 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>>> +\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>>> \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 4AB4BBDE6B\n\tfor ;\n\tMon, 20 May 2024 13:12:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3102C6347C;\n\tMon, 20 May 2024 15:12:32 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9A53061A58\n\tfor ;\n\tMon, 20 May 2024 15:12:30 +0200 (CEST)","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 0908CC67;\n\tMon, 20 May 2024 15:12:19 +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=\"hJAv8qqo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1716210739;\n\tbh=lx80NIVpTo9YEpzhvSZ2ItXbXYLUVWwlAYyn7lRi4LI=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=hJAv8qqoePcoB2GGfESo/BoSq58L9FP8xBgfdOB8o1mouSabmQi/9nKKrI41w4gPo\n\t/SvdJbonJWLeuKivqHxu0YkHaD7Znu2WU4yxfUg9XBdMDzhzZea53Qd/a7fwUQMn7d\n\t1ZjWWAMwLol2m8dmvretZYLXw4PQgn1n8BZ2xCl4=","Message-ID":"<564fc395-c4c0-4fe7-908e-510a15668dad@ideasonboard.com>","Date":"Mon, 20 May 2024 14:12:26 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3 2/3] ipa: libipa: agc: Change luminance target to\n\tpiecewise linear function","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20240517080802.3896531-1-paul.elder@ideasonboard.com>\n\t<20240517080802.3896531-3-paul.elder@ideasonboard.com>\n\t\n\t","Content-Language":"en-US","From":"Dan Scally ","Autocrypt":"addr=dan.scally@ideasonboard.com; keydata=\n\txsFNBGLydlEBEADa5O2s0AbUguprfvXOQun/0a8y2Vk6BqkQALgeD6KnXSWwaoCULp18etYW\n\tB31bfgrdphXQ5kUQibB0ADK8DERB4wrzrUb5CMxLBFE7mQty+v5NsP0OFNK9XTaAOcmD+Ove\n\teIjYvqurAaro91jrRVrS1gBRxIFqyPgNvwwL+alMZhn3/2jU2uvBmuRrgnc/e9cHKiuT3Dtq\n\tMHGPKL2m+plk+7tjMoQFfexoQ1JKugHAjxAhJfrkXh6uS6rc01bYCyo7ybzg53m1HLFJdNGX\n\tsUKR+dQpBs3SY4s66tc1sREJqdYyTsSZf80HjIeJjU/hRunRo4NjRIJwhvnK1GyjOvvuCKVU\n\tRWpY8dNjNu5OeAfdrlvFJOxIE9M8JuYCQTMULqd1NuzbpFMjc9524U3Cngs589T7qUMPb1H1\n\tNTA81LmtJ6Y+IV5/kiTUANflpzBwhu18Ok7kGyCq2a2jsOcVmk8gZNs04gyjuj8JziYwwLbf\n\tvzABwpFVcS8aR+nHIZV1HtOzyw8CsL8OySc3K9y+Y0NRpziMRvutrppzgyMb9V+N31mK9Mxl\n\t1YkgaTl4ciNWpdfUe0yxH03OCuHi3922qhPLF4XX5LN+NaVw5Xz2o3eeWklXdouxwV7QlN33\n\tu4+u2FWzKxDqO6WLQGjxPE0mVB4Gh5Pa1Vb0ct9Ctg0qElvtGQARAQABzShEYW4gU2NhbGx5\n\tIDxkYW4uc2NhbGx5QGlkZWFzb25ib2FyZC5jb20+wsGNBBMBCAA3FiEEsdtt8OWP7+8SNfQe\n\tkiQuh/L+GMQFAmLydlIFCQWjmoACGwMECwkIBwUVCAkKCwUWAgMBAAAKCRCSJC6H8v4YxDI2\n\tEAC2Gz0iyaXJkPInyshrREEWbo0CA6v5KKf3I/HlMPqkZ48bmGoYm4mEQGFWZJAT3K4ir8bg\n\tcEfs9V54gpbrZvdwS4abXbUK4WjKwEs8HK3XJv1WXUN2bsz5oEJWZUImh9gD3naiLLI9QMMm\n\tw/aZkT+NbN5/2KvChRWhdcha7+2Te4foOY66nIM+pw2FZM6zIkInLLUik2zXOhaZtqdeJZQi\n\tHSPU9xu7TRYN4cvdZAnSpG7gQqmLm5/uGZN1/sB3kHTustQtSXKMaIcD/DMNI3JN/t+RJVS7\n\tc0Jh/ThzTmhHyhxx3DRnDIy7kwMI4CFvmhkVC2uNs9kWsj1DuX5kt8513mvfw2OcX9UnNKmZ\n\tnhNCuF6DxVrL8wjOPuIpiEj3V+K7DFF1Cxw1/yrLs8dYdYh8T8vCY2CHBMsqpESROnTazboh\n\tAiQ2xMN1cyXtX11Qwqm5U3sykpLbx2BcmUUUEAKNsM//Zn81QXKG8vOx0ZdMfnzsCaCzt8f6\n\t9dcDBBI3tJ0BI9ByiocqUoL6759LM8qm18x3FYlxvuOs4wSGPfRVaA4yh0pgI+ModVC2Pu3y\n\tejE/IxeatGqJHh6Y+iJzskdi27uFkRixl7YJZvPJAbEn7kzSi98u/5ReEA8Qhc8KO/B7wprj\n\txjNMZNYd0Eth8+WkixHYj752NT5qshKJXcyUU87BTQRi8nZSARAAx0BJayh1Fhwbf4zoY56x\n\txHEpT6DwdTAYAetd3yiKClLVJadYxOpuqyWa1bdfQWPb+h4MeXbWw/53PBgn7gI2EA7ebIRC\n\tPJJhAIkeym7hHZoxqDQTGDJjxFEL11qF+U3rhWiL2Zt0Pl+zFq0eWYYVNiXjsIS4FI2+4m16\n\ttPbDWZFJnSZ828VGtRDQdhXfx3zyVX21lVx1bX4/OZvIET7sVUufkE4hrbqrrufre7wsjD1t\n\t8MQKSapVrr1RltpzPpScdoxknOSBRwOvpp57pJJe5A0L7+WxJ+vQoQXj0j+5tmIWOAV1qBQp\n\thyoyUk9JpPfntk2EKnZHWaApFp5TcL6c5LhUvV7F6XwOjGPuGlZQCWXee9dr7zym8iR3irWT\n\t+49bIh5PMlqSLXJDYbuyFQHFxoiNdVvvf7etvGfqFYVMPVjipqfEQ38ST2nkzx+KBICz7uwj\n\tJwLBdTXzGFKHQNckGMl7F5QdO/35An/QcxBnHVMXqaSd12tkJmoRVWduwuuoFfkTY5mUV3uX\n\txGj3iVCK4V+ezOYA7c2YolfRCNMTza6vcK/P4tDjjsyBBZrCCzhBvd4VVsnnlZhVaIxoky4K\n\taL+AP+zcQrUZmXmgZjXOLryGnsaeoVrIFyrU6ly90s1y3KLoPsDaTBMtnOdwxPmo1xisH8oL\n\ta/VRgpFBfojLPxMAEQEAAcLBfAQYAQgAJhYhBLHbbfDlj+/vEjX0HpIkLofy/hjEBQJi8nZT\n\tBQkFo5qAAhsMAAoJEJIkLofy/hjEXPcQAMIPNqiWiz/HKu9W4QIf1OMUpKn3YkVIj3p3gvfM\n\tRes4fGX94Ji599uLNrPoxKyaytC4R6BTxVriTJjWK8mbo9jZIRM4vkwkZZ2bu98EweSucxbp\n\tvjESsvMXGgxniqV/RQ/3T7LABYRoIUutARYq58p5HwSP0frF0fdFHYdTa2g7MYZl1ur2JzOC\n\tFHRpGadlNzKDE3fEdoMobxHB3Lm6FDml5GyBAA8+dQYVI0oDwJ3gpZPZ0J5Vx9RbqXe8RDuR\n\tdu90hvCJkq7/tzSQ0GeD3BwXb9/R/A4dVXhaDd91Q1qQXidI+2jwhx8iqiYxbT+DoAUkQRQy\n\txBtoCM1CxH7u45URUgD//fxYr3D4B1SlonA6vdaEdHZOGwECnDpTxecENMbz/Bx7qfrmd901\n\tD+N9SjIwrbVhhSyUXYnSUb8F+9g2RDY42Sk7GcYxIeON4VzKqWM7hpkXZ47pkK0YodO+dRKM\n\tyMcoUWrTK0Uz6UzUGKoJVbxmSW/EJLEGoI5p3NWxWtScEVv8mO49gqQdrRIOheZycDmHnItt\n\t9Qjv00uFhEwv2YfiyGk6iGF2W40s2pH2t6oeuGgmiZ7g6d0MEK8Ql/4zPItvr1c1rpwpXUC1\n\tu1kQWgtnNjFHX3KiYdqjcZeRBiry1X0zY+4Y24wUU0KsEewJwjhmCKAsju1RpdlPg2kC","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29647,"web_url":"https://patchwork.libcamera.org/comment/29647/","msgid":"","date":"2024-05-29T07:42:38","subject":"Re: [PATCH v3 2/3] ipa: libipa: agc: Change luminance target to\n\tpiecewise linear function","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Mon, May 20, 2024 at 02:12:26PM +0100, Dan Scally wrote:\n> \n> On 17/05/2024 12:47, Paul Elder wrote:\n> > On Fri, May 17, 2024 at 12:29:16PM +0200, Dan Scally wrote:\n> > > Hi Paul\n> > > \n> > > On 17/05/2024 10:08, 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> > > > \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> > > > \n> > > > Signed-off-by: Paul Elder \n> > > > Reviewed-by: Stefan Klug \n> > > \n> > > Thanks for the patch, this looks good to me. We need to remember the same\n> > > thing for the constraint target though I guess.\n> > Yeah I just remembered that I had meant to squash that patch in with\n> > this, but ig I can send it separately.\n> \n> \n> Actually, one quick thought below...\n> \n> > \n> > \n> > Thanks,\n> > \n> > Paul\n> > \n> > > \n> > > Reviewed-by: Daniel Scally \n> > > \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 | 31 +++++++++++++++++++++------\n> > > > src/ipa/libipa/agc_mean_luminance.h | 7 +++---\n> > > > src/ipa/rkisp1/algorithms/agc.cpp | 5 ++++-\n> > > > 4 files changed, 36 insertions(+), 12 deletions(-)\n> > > > \n> > > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> > > > index c9b5548c4..984ed0874 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> > > > +\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> > > > \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 2bf84d05b..fe07777dc 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> > > > AgcMeanLuminance::AgcMeanLuminance()\n> > > > -\t: frameCount_(0), filteredExposure_(0s), relativeLuminanceTarget_(0)\n> > > > +\t: frameCount_(0), filteredExposure_(0s)\n> > > > {\n> > > > }\n> > > > @@ -142,8 +142,16 @@ AgcMeanLuminance::~AgcMeanLuminance() = default;\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 = { { 0, kDefaultRelativeLuminanceTarget } };\n> > > > +\trelativeLuminanceTarget_ = Pwl(points);\n> \n> \n> Does this not need to have two points? The Pwl implementation looks like it\n> expects an even number of points with a minimum of 2 - validation would fail\n> in the readYaml() if that weren't met.\n\nIndeed it does. I was getting mixed up with my matrix interpolator which\ndoesn't.\n\n\nThanks,\n\nPaul\n\n> \n> > > > }\n> > > > void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n> > > > @@ -421,11 +429,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> > > > + *\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> > > > \t/*\n> > > > @@ -520,6 +535,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 +547,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 +557,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,\n> > > > \tstd::shared_ptr exposureModeHelper =\n> > > > \t\texposureModeHelpers_.at(exposureModeIndex);\n> > > > -\tdouble gain = estimateInitialGain();\n> > > > +\tdouble gain = estimateInitialGain(lux);\n> > > > \tgain = constraintClampGain(constraintModeIndex, yHist, gain);\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> > > > #include \"exposure_mode_helper.h\"\n> > > > #include \"histogram.h\"\n> > > > +#include \"pwl.h\"\n> > > > namespace libcamera {\n> > > > @@ -62,7 +63,7 @@ public:\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> > > > \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> > > > \tuint64_t frameCount_;\n> > > > \tutils::Duration filteredExposure_;\n> > > > -\tdouble relativeLuminanceTarget_;\n> > > > +\tPwl relativeLuminanceTarget_;\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 4af397bdc..1c9872d02 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> > > > +\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> > > > \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 AB994BDE6B\n\tfor ;\n\tWed, 29 May 2024 07:42:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9C192634B2;\n\tWed, 29 May 2024 09:42:48 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E2EB961A46\n\tfor ;\n\tWed, 29 May 2024 09:42:46 +0200 (CEST)","from pyrite.rasen.tech (h175-177-049-156.catv02.itscom.jp\n\t[175.177.49.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7102966F;\n\tWed, 29 May 2024 09:42:42 +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=\"qsDKGvEo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1716968563;\n\tbh=ecix3QQRaRaVbjgtFsQm9VaMrj/SzCAuOMbEUmMd0yY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qsDKGvEoSunQV3HE9gdV6J2eYiWPAwjktXZJPS8aD8x5gogR+A1SBm0PMSQiRv/Xx\n\t76x5nSpdnf1bAk19Lp56CUIw1+IEC2QluEXoe9e1ZHkRiiDCBXGG1V181mUtHREGQz\n\tLuA7tBeUA4BxyQEZGLj2lJSOI51V29jqpen08hBM=","Date":"Wed, 29 May 2024 16:42:38 +0900","From":"Paul Elder ","To":"Dan Scally ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3 2/3] ipa: libipa: agc: Change luminance target to\n\tpiecewise linear function","Message-ID":"","References":"<20240517080802.3896531-1-paul.elder@ideasonboard.com>\n\t<20240517080802.3896531-3-paul.elder@ideasonboard.com>\n\t\n\t\n\t<564fc395-c4c0-4fe7-908e-510a15668dad@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<564fc395-c4c0-4fe7-908e-510a15668dad@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\" "}}]