[{"id":38672,"web_url":"https://patchwork.libcamera.org/comment/38672/","msgid":"<177753576513.45302.13455434582603757341@ping.linuxembedded.co.uk>","date":"2026-04-30T07:56:05","subject":"Re: [PATCH v4 1/3] ipa: simple: agc: Replace bang-bang controller\n\twith proportional","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Milan, Hans,\n\nIf you could check my thoughts below - if you think this is still ok\nwithout a correct gain model I'll be ok merging this (pending Javier's\nrebase)\n\n\n\nQuoting Javier Tia (2026-03-06 18:46:40)\n> The AGC's updateExposure() uses a fixed ~10% step per frame regardless\n> of how far the current exposure is from optimal. With a hysteresis dead\n> band of only +/-4%, the controller overshoots when the correct value\n> falls within one step, causing visible brightness oscillation (flicker).\n> \n> Replace the fixed-step bang-bang controller with a proportional one\n> where the correction factor scales linearly with the MSV error:\n> \n> factor = 1.0 + error * 0.04\n> \n> At maximum error (~2.5), this gives the same ~10% step as before. Near\n> the target, steps shrink to <1%, eliminating overshoot. The existing\n> hysteresis (kExposureSatisfactory) still prevents hunting on noise.\n> \n> Tested on OV2740 behind Intel IPU6 ISYS (ThinkPad X1 Carbon Gen 10)\n> where the old controller produced continuous brightness flicker. The\n> proportional controller converges in ~3 seconds from cold start with\n> no visible oscillation.\n> \n> Signed-off-by: Javier Tia \n> Reviewed-by: Milan Zamazal \n> Tested-by: Barnabás Pőcze \n> ---\n> src/ipa/simple/algorithms/agc.cpp | 65 ++++++++++++++++++++++++---------------\n> 1 file changed, 41 insertions(+), 24 deletions(-)\n> \n> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp\n> index 2f7e040c..ac977d5f 100644\n> --- a/src/ipa/simple/algorithms/agc.cpp\n> +++ b/src/ipa/simple/algorithms/agc.cpp\n> @@ -7,6 +7,8 @@\n> \n> #include \"agc.h\"\n> \n> +#include \n> +#include \n> #include \n> \n> #include \n> @@ -37,52 +39,66 @@ static constexpr float kExposureOptimal = kExposureBinsCount / 2.0;\n> */\n> static constexpr float kExposureSatisfactory = 0.2;\n> \n> +/*\n> + * Proportional gain for exposure/gain adjustment. Maps the MSV error to a\n> + * multiplicative correction factor:\n> + *\n> + * factor = 1.0 + kExpProportionalGain * error\n> + *\n> + * With kExpProportionalGain = 0.04:\n> + * - max error ~2.5 -> factor 1.10 (~10% step, same as before)\n> + * - error 1.0 -> factor 1.04 (~4% step)\n> + * - error 0.3 -> factor 1.012 (~1.2% step)\n> + *\n> + * This replaces the fixed 10% bang-bang step with a proportional correction\n> + * that converges smoothly and avoids overshooting near the target.\n> + */\n> +static constexpr float kExpProportionalGain = 0.04;\n> +\n> Agc::Agc()\n> {\n> }\n> \n> void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, double exposureMSV)\n> {\n> - /*\n> - * kExpDenominator of 10 gives ~10% increment/decrement;\n> - * kExpDenominator of 5 - about ~20%\n> - */\n> - static constexpr uint8_t kExpDenominator = 10;\n> - static constexpr uint8_t kExpNumeratorUp = kExpDenominator + 1;\n> - static constexpr uint8_t kExpNumeratorDown = kExpDenominator - 1;\n> -\n> int32_t &exposure = frameContext.sensor.exposure;\n> double &again = frameContext.sensor.gain;\n> \n> - if (exposureMSV < kExposureOptimal - kExposureSatisfactory) {\n> + double error = kExposureOptimal - exposureMSV;\n> +\n> + if (std::abs(error) <= kExposureSatisfactory)\n> + return;\n> +\n> + /*\n> + * Compute a proportional correction factor. The sign of the error\n> + * determines the direction: positive error means too dark (increase),\n> + * negative means too bright (decrease).\n> + */\n> + float factor = 1.0f + static_cast(error) * kExpProportionalGain;\n> +\n> + if (factor > 1.0f) {\n> + /* Scene too dark: increase exposure first, then gain. */\n> if (exposure < context.configuration.agc.exposureMax) {\n> - int32_t next = exposure * kExpNumeratorUp / kExpDenominator;\n> - if (next - exposure < 1)\n> - exposure += 1;\n> - else\n> - exposure = next;\n> + int32_t next = static_cast(exposure * factor);\n> + exposure = std::max(next, exposure + 1);\n> } else {\n> - double next = again * kExpNumeratorUp / kExpDenominator;\n> + double next = again * factor;\n\nDoes this still work when we don't have a gain model for the device ?\n\nSupporting missing gain models (where the gain code is just an arbitrary\nregister value, and not a linear gain value) is the only reason the\nsimple pipeline handler has a separate AGC implementation from the one\nin libipa ...\n\nI'm happy enough to merge this - as a step towards unifying things\nthough, but it would be helpful if someone with a device that doesn't\nhave a gain model could test and make sure this isn't just fixing one\nset of devices by breaking the actual target use case for keeping this\nas a separate algorithm...\n\n\n\n> if (next - again < context.configuration.agc.againMinStep)\n> again += context.configuration.agc.againMinStep;\n> else\n> again = next;\n> }\n> - }\n> -\n> - if (exposureMSV > kExposureOptimal + kExposureSatisfactory) {\n> + } else {\n> + /* Scene too bright: decrease gain first, then exposure. */\n> if (again > context.configuration.agc.again10) {\n> - double next = again * kExpNumeratorDown / kExpDenominator;\n> + double next = again * factor;\n> if (again - next < context.configuration.agc.againMinStep)\n> again -= context.configuration.agc.againMinStep;\n> else\n> again = next;\n> } else {\n> - int32_t next = exposure * kExpNumeratorDown / kExpDenominator;\n> - if (exposure - next < 1)\n> - exposure -= 1;\n> - else\n> - exposure = next;\n> + int32_t next = static_cast(exposure * factor);\n> + exposure = std::min(next, exposure - 1);\n> }\n> }\n> \n> @@ -96,6 +112,7 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou\n> \n> LOG(IPASoftExposure, Debug)\n> << \"exposureMSV \" << exposureMSV\n> + << \" error \" << error << \" factor \" << factor\n> << \" exp \" << exposure << \" again \" << again;\n> }\n> \n> \n> -- \n> 2.53.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 72D22BE173\n\tfor ;\n\tThu, 30 Apr 2026 07:56:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7577562FE1;\n\tThu, 30 Apr 2026 09:56:10 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6BD8962E6A\n\tfor ;\n\tThu, 30 Apr 2026 09:56:08 +0200 (CEST)","from monstersaurus.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 0D91D8FA;\n\tThu, 30 Apr 2026 09:54:24 +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=\"PgglCNvM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1777535664;\n\tbh=C+DjQ51jOhEJjFY+vXSn5HQuMXpuSkf9jufJJLhEOHs=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=PgglCNvM+XE7r1NFCewUAzeTxeMu4cGS2/q9sK9w+npGl4yrLQS8xYNMAQfrKIPRa\n\tCX1u6xKYTEAGeNxwZK6nMAUtVgNxbAtidnHp4i7BD1mJgv+mW9eZRvluR9M2wQK7eT\n\tyl3ICQ4OM+EGh9L8dCzU6MF+UfbatQjQwJxiGSYk=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20260306-agc-proportional-v4-1-e87c7e0d837a@jetm.me>","References":"<20260306-agc-proportional-v4-0-e87c7e0d837a@jetm.me>\n\t<20260306-agc-proportional-v4-1-e87c7e0d837a@jetm.me>","Subject":"Re: [PATCH v4 1/3] ipa: simple: agc: Replace bang-bang controller\n\twith proportional","From":"Kieran Bingham ","Cc":"Javier Tia , Milan Zamazal ,\n\t=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","To":"Javier Tia , libcamera-devel@lists.libcamera.org,\n\tMilan Zamazal ,\n\tHans de Goede , ","Date":"Thu, 30 Apr 2026 08:56:05 +0100","Message-ID":"<177753576513.45302.13455434582603757341@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":38746,"web_url":"https://patchwork.libcamera.org/comment/38746/","msgid":"<20260506224249.317D41EA006B@mailuser.phl.internal>","date":"2026-05-06T22:28:02","subject":"Re: [PATCH v4 1/3] ipa: simple: agc: Replace bang-bang controller\n\twith proportional","submitter":{"id":261,"url":"https://patchwork.libcamera.org/api/people/261/","name":"Javier Tia","email":"floss@jetm.me"},"content":"Hi Kieran, Milan, Hans,\n\nOn the gain model question: the proportional step `again * factor`\nhas the same linearity assumption as the old fixed step\n`again * 11/10`. Both multiply the current register value by a\nscalar. For devices without a calibrated gain model, the register\nvalue was already being used as a linear proxy in the original code.\nThis change only affects how aggressively the step adjusts -\nnarrower steps near target - not the direction logic or the\nunderlying gain model assumption.\n\nDevices without a gain model should behave the same or better:\nsmaller steps near the target reduce the risk of overshoot, which is\nexactly the problem the proportional controller was designed to fix.\n\nv5 has been sent:\nhttps://lists.libcamera.org/pipermail/libcamera-devel/2026-May/058560.html\n\nBest,\nJavier","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 CE4DBBE173\n\tfor ;\n\tWed, 6 May 2026 22:42:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6DDF06301E;\n\tThu, 7 May 2026 00:42:51 +0200 (CEST)","from fout-a5-smtp.messagingengine.com\n\t(fout-a5-smtp.messagingengine.com [103.168.172.148])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 183866271A\n\tfor ;\n\tThu, 7 May 2026 00:42:50 +0200 (CEST)","from phl-compute-02.internal (phl-compute-02.internal\n\t[10.202.2.42])\n\tby mailfout.phl.internal (Postfix) with ESMTP id 66A16EC013B;\n\tWed, 6 May 2026 18:42:49 -0400 (EDT)","from phl-imap-07 ([10.202.2.97])\n\tby phl-compute-02.internal (MEProxy); Wed, 06 May 2026 18:42:49 -0400","by mailuser.phl.internal (Postfix, from userid 501)\n\tid 317D41EA006B; Wed, 6 May 2026 18:42:49 -0400 (EDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=jetm.me header.i=@jetm.me header.b=\"TniJloQt\";\n\tdkim=pass (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"VOHX6WUc\"; \n\tdkim-atps=neutral","DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/relaxed; d=jetm.me; h=cc:cc\n\t:content-transfer-encoding:content-type:content-type:date:date\n\t:from:from:in-reply-to:in-reply-to:message-id:mime-version\n\t:references:reply-to:subject:subject:to:to; s=fm2; t=1778107369;\n\tx=1778193769; bh=AbrCe750qm1eKc9+mWR0dqt99jA4eSlrb80bfbz7sdE=; b=\n\tTniJloQt+Li0flqWSzs5v+8uvorr7zIBA+9Bt0IUnMizi0QqfQtqG+UXH8z+Irty\n\tDs95DkYtoutFz7PtQ6zsbnpL02grV86CMhDK+mKHAY0UPZf9TSlg0UFgg2A++vZz\n\tNoqCoTvme1vbyX4NSMEwgXUJ5BW3lZsUwXVRxS1NO1+8sHDTwwhprD09fumOyJZ2\n\tyL/J4o6Y8k/5rceIdQXl1jYMnmuFc9C1phHe1vrp9wc7izV3NThtNeAXBq7fqyqv\n\tZSwa+j7oxZ82SMPaMGQjxW0bD1LnLdYNuDIon/JuNP/kSLtDc9WIKxyZYYGbbrnY\n\tvo4uRA8XT4qZCLcNlGy1HQ==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=\n\tmessagingengine.com; h=cc:cc:content-transfer-encoding\n\t:content-type:content-type:date:date:feedback-id:feedback-id\n\t:from:from:in-reply-to:in-reply-to:message-id:mime-version\n\t:references:reply-to:subject:subject:to:to:x-me-proxy\n\t:x-me-sender:x-me-sender:x-sasl-enc; s=fm3; t=1778107369; x=\n\t1778193769; bh=AbrCe750qm1eKc9+mWR0dqt99jA4eSlrb80bfbz7sdE=; b=V\n\tOHX6WUcO4OY7E4+x1lfOJ57FNVc8l9BSkVd1OMrDKDlx7XOKw2ogqLrtJWXliAeV\n\t5BsFDitgVZ+VWymjb5GoCdXuqh6nnEhhCRywJQNsJubBU+h1BntG7oYjS33dsi5W\n\tmiSJd0QLiFHeBMsJ3R/8sxTHMOYJCk81DHHCYTDnmsXZQFyV5q94hyceb8rTzipT\n\tjRkC5Xvb9Ws9LpZRXnyJItayDzZwDbHaWTFO5jtBz09L3pCvS2/Hbgn4Rd/aPo2e\n\twlkls5VQqUDApcYbKsJsJSXKRA27dhOaS39C/MPccpbrmPniRbzRaNNCO6aqt1z/\n\tPspHDrlbV+g3kqgNqo9kQ=="],"X-ME-Sender":"\n\t","X-ME-Proxy-Cause":"gggruggvucftvghtrhhoucdtuddrgeefhedrtddtgddutdehkedvucetufdoteggodetrf\n\tdotffvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfurfetoffkrfgpnffqhgenuceu\n\trghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmnegopf\n\thokfffucdluddtmdenucfjughrpefotggggffhvfffufevjghfsehtkedttdertdejnecu\n\thfhrohhmpeflrghvihgvrhcuvfhirgcuoehflhhoshhssehjvghtmhdrmhgvqeenucggtf\n\tfrrghtthgvrhhnpeeijeekvdfgfeeileettdfffedutedvteegvdekleduuedufefhheef\n\tueekveeitdenucffohhmrghinheplhhisggtrghmvghrrgdrohhrghenucevlhhushhtvg\n\thrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehflhhoshhssehjvghtmhdr\n\tmhgvpdhnsggprhgtphhtthhopeehpdhmohguvgepshhmthhpohhuthdprhgtphhtthhope\n\tgsrghrnhgrsggrshdrphhotgiivgesihguvggrshhonhgsohgrrhgurdgtohhmpdhrtghp\n\tthhtohepkhhivghrrghnrdgsihhnghhhrghmsehiuggvrghsohhnsghorghrugdrtghomh\n\tdprhgtphhtthhopehlihgstggrmhgvrhgrqdguvghvvghlsehlihhsthhsrdhlihgstggr\n\tmhgvrhgrrdhorhhgpdhrtghpthhtohepjhhohhgrnhhnvghsrdhgohgvuggvsehoshhsrd\n\thquhgrlhgtohhmmhdrtghomhdprhgtphhtthhopehmiigrmhgriigrlhesrhgvughhrght\n\trdgtohhm","X-ME-Proxy":"\n\t\n\t\n\t\n\t","Feedback-ID":"i9dde48b3:Fastmail","X-Mailer":"MessagingEngine.com Webmail Interface","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"8bit","From":"Javier Tia ","To":"Kieran Bingham ","Date":"Wed, 06 May 2026 16:28:02 -0600","Subject":"Re: [PATCH v4 1/3] ipa: simple: agc: Replace bang-bang controller\n\twith proportional","Cc":"libcamera-devel@lists.libcamera.org, Milan Zamazal ,\n\tHans de Goede , =?utf-8?q?Barnab?=\n\t=?utf-8?b?w6FzIFDFkWN6ZQ==?= ","In-Reply-To":"<177753576513.45302.13455434582603757341@ping.linuxembedded.co.uk>","References":"<20260306-agc-proportional-v4-0-e87c7e0d837a@jetm.me>\n\t<20260306-agc-proportional-v4-1-e87c7e0d837a@jetm.me>\n\t<177753576513.45302.13455434582603757341@ping.linuxembedded.co.uk>","Message-Id":"<20260506224249.317D41EA006B@mailuser.phl.internal>","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\" "}}]