[{"id":33412,"web_url":"https://patchwork.libcamera.org/comment/33412/","msgid":"<174005401879.3771432.4689584838028479438@ping.linuxembedded.co.uk>","date":"2025-02-20T12:20:18","subject":"Re: [PATCH 07/10] ipa: rkisp1: Damp color temperature regulation","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2025-02-17 10:01:48)\n> Damp the regulation of the color temperature with the same factor as the\n> gains. Not damping the color temperature leads to visible flicker, as\n> the CCM changes too much.\n\nInteresting, I thought we had something on the CCM interplotator so it\nwouldn't change too much on small movements of the colour temperature,\nbut I guess smoothing out the colour temperature is reasonable anyway -\nespecially as it's an estimate. I wonder if we should have some sort of\nIIR instances to make these a bit more specific, but the speed is fine\nnow.\n\n\nReviewed-by: Kieran Bingham \n\n\nOr maybe we should report the real calculated value to metadata, but\nfilter it on the input to the CCM interpolator ?\n\n> \n> Signed-off-by: Stefan Klug \n> ---\n> src/ipa/rkisp1/algorithms/awb.cpp | 5 +++--\n> 1 file changed, 3 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> index 9244a1e64f41..347d38d226b4 100644\n> --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> @@ -320,8 +320,6 @@ void Awb::process(IPAContext &context,\n> rgbMeans.b() < kMeanMinThreshold)\n> return;\n> \n> - activeState.awb.automatic.temperatureK = estimateCCT(rgbMeans);\n> -\n> /*\n> * Estimate the red and blue gains to apply in a grey world. The green\n> * gain is hardcoded to 1.0. Avoid divisions by zero by clamping the\n> @@ -343,8 +341,11 @@ void Awb::process(IPAContext &context,\n> \n> /* Filter the values to avoid oscillations. */\n> double speed = 0.2;\n> + double ct = estimateCCT(rgbMeans);\n> + ct = ct * speed + activeState.awb.automatic.temperatureK * (1 - speed);\n> gains = gains * speed + activeState.awb.automatic.gains * (1 - speed);\n> \n> + activeState.awb.automatic.temperatureK = static_cast(ct);\n> activeState.awb.automatic.gains = gains;\n> \n> LOG(RkISP1Awb, Debug)\n> -- \n> 2.43.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 34516BE175\n\tfor ;\n\tThu, 20 Feb 2025 12:20:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 325B4686A1;\n\tThu, 20 Feb 2025 13:20:24 +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 5396E6032B\n\tfor ;\n\tThu, 20 Feb 2025 13:20:22 +0100 (CET)","from pendragon.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 A39A9526;\n\tThu, 20 Feb 2025 13:18:58 +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=\"QPbS5Gdj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1740053938;\n\tbh=gB+KsBY/lieouSni4UjEZJscZeGzbEppM9Yit3ktunM=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=QPbS5Gdjui6O1lmIbJRC6JPfOLQG+ib2f44XFJz6kGUyBoHSt4dliCtVg854Dg0hA\n\tiKmC9/t6AMl5drA/dyavoAHA+RaU5A97xuDHtCTp8daQ7Mff4NNKxLWWQ3GcpSBsq8\n\tTfZvSJl9EZcuBPM50Z8GVLfCq0DqthHMrgsxDxJA=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250217100203.297894-8-stefan.klug@ideasonboard.com>","References":"<20250217100203.297894-1-stefan.klug@ideasonboard.com>\n\t<20250217100203.297894-8-stefan.klug@ideasonboard.com>","Subject":"Re: [PATCH 07/10] ipa: rkisp1: Damp color temperature regulation","From":"Kieran Bingham ","Cc":"Stefan Klug ","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 20 Feb 2025 12:20:18 +0000","Message-ID":"<174005401879.3771432.4689584838028479438@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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":33653,"web_url":"https://patchwork.libcamera.org/comment/33653/","msgid":"","date":"2025-03-19T15:33:48","subject":"Re: [PATCH 07/10] ipa: rkisp1: Damp color temperature regulation","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the review. \n\nOn Thu, Feb 20, 2025 at 12:20:18PM +0000, Kieran Bingham wrote:\n> Quoting Stefan Klug (2025-02-17 10:01:48)\n> > Damp the regulation of the color temperature with the same factor as the\n> > gains. Not damping the color temperature leads to visible flicker, as\n> > the CCM changes too much.\n> \n> Interesting, I thought we had something on the CCM interplotator so it\n> wouldn't change too much on small movements of the colour temperature,\n> but I guess smoothing out the colour temperature is reasonable anyway -\n> especially as it's an estimate. I wonder if we should have some sort of\n> IIR instances to make these a bit more specific, but the speed is fine\n> now.\n\nThe interpolator has support for quantizations. On lsc we use that. But\nI'm not really confident if that is the best strategy. The damping is\nmerely to damp larger spikes, which wouldn't get caught by quantization.\nWith the IIR instance you are completely right. I even had some\nprototype somewhere. But properly tuning an IIR filter can also be a\npain :-)\n\n> \n> \n> Reviewed-by: Kieran Bingham \n> \n> \n> Or maybe we should report the real calculated value to metadata, but\n> filter it on the input to the CCM interpolator ?\n\nI'm quite undecided here. The filtered data might be better than the\nunfiltered one? I think I'd stick to the filtered one, as it is in sync\nwith the gains.\n\nRegards,\nStefan\n\n> \n> > \n> > Signed-off-by: Stefan Klug \n> > ---\n> > src/ipa/rkisp1/algorithms/awb.cpp | 5 +++--\n> > 1 file changed, 3 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> > index 9244a1e64f41..347d38d226b4 100644\n> > --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> > @@ -320,8 +320,6 @@ void Awb::process(IPAContext &context,\n> > rgbMeans.b() < kMeanMinThreshold)\n> > return;\n> > \n> > - activeState.awb.automatic.temperatureK = estimateCCT(rgbMeans);\n> > -\n> > /*\n> > * Estimate the red and blue gains to apply in a grey world. The green\n> > * gain is hardcoded to 1.0. Avoid divisions by zero by clamping the\n> > @@ -343,8 +341,11 @@ void Awb::process(IPAContext &context,\n> > \n> > /* Filter the values to avoid oscillations. */\n> > double speed = 0.2;\n> > + double ct = estimateCCT(rgbMeans);\n> > + ct = ct * speed + activeState.awb.automatic.temperatureK * (1 - speed);\n> > gains = gains * speed + activeState.awb.automatic.gains * (1 - speed);\n> > \n> > + activeState.awb.automatic.temperatureK = static_cast(ct);\n> > activeState.awb.automatic.gains = gains;\n> > \n> > LOG(RkISP1Awb, Debug)\n> > -- \n> > 2.43.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 1DFF8C3301\n\tfor ;\n\tWed, 19 Mar 2025 15:33:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 69A4868951;\n\tWed, 19 Mar 2025 16:33:52 +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 3B05A687FA\n\tfor ;\n\tWed, 19 Mar 2025 16:33:51 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:760:e5ca:4814:99c7])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2F3D61E6;\n\tWed, 19 Mar 2025 16:32:08 +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=\"lB5fOWBy\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1742398328;\n\tbh=0cGAS3eRqWK9uCLvcr/6hxGWgX60ESmD44IaIemjvsE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lB5fOWByeiGem56rrS4wIOpxjIT/5irvFrFII0ia0GHqOxBmz0MxHwhL8xdZayOF9\n\tRXTbCD1iYwiG+WpNoe0ulIEl9ENIh0l+FQOfU1hm17QzAbtVK1v0D0FERfnu4iTw5l\n\tRdpAcj87TLw5S4Nra4E8nEbCCLg70PM9iPTq2xeE=","Date":"Wed, 19 Mar 2025 16:33:48 +0100","From":"Stefan Klug ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 07/10] ipa: rkisp1: Damp color temperature regulation","Message-ID":"","References":"<20250217100203.297894-1-stefan.klug@ideasonboard.com>\n\t<20250217100203.297894-8-stefan.klug@ideasonboard.com>\n\t<174005401879.3771432.4689584838028479438@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<174005401879.3771432.4689584838028479438@ping.linuxembedded.co.uk>","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":33655,"web_url":"https://patchwork.libcamera.org/comment/33655/","msgid":"<174239866619.783128.17356899134816042594@ping.linuxembedded.co.uk>","date":"2025-03-19T15:37:46","subject":"Re: [PATCH 07/10] ipa: rkisp1: Damp color temperature regulation","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2025-03-19 15:33:48)\n> Hi Kieran,\n> \n> Thank you for the review. \n> \n> On Thu, Feb 20, 2025 at 12:20:18PM +0000, Kieran Bingham wrote:\n> > Quoting Stefan Klug (2025-02-17 10:01:48)\n> > > Damp the regulation of the color temperature with the same factor as the\n> > > gains. Not damping the color temperature leads to visible flicker, as\n> > > the CCM changes too much.\n> > \n> > Interesting, I thought we had something on the CCM interplotator so it\n> > wouldn't change too much on small movements of the colour temperature,\n> > but I guess smoothing out the colour temperature is reasonable anyway -\n> > especially as it's an estimate. I wonder if we should have some sort of\n> > IIR instances to make these a bit more specific, but the speed is fine\n> > now.\n> \n> The interpolator has support for quantizations. On lsc we use that. But\n> I'm not really confident if that is the best strategy. The damping is\n> merely to damp larger spikes, which wouldn't get caught by quantization.\n> With the IIR instance you are completely right. I even had some\n> prototype somewhere. But properly tuning an IIR filter can also be a\n> pain :-)\n\nYes, indeed! they're difficult to get right for sure.\nNo need to complicate things here now.\n\n> > Reviewed-by: Kieran Bingham \n> > \n> > \n> > Or maybe we should report the real calculated value to metadata, but\n> > filter it on the input to the CCM interpolator ?\n> \n> I'm quite undecided here. The filtered data might be better than the\n> unfiltered one? I think I'd stick to the filtered one, as it is in sync\n> with the gains.\n\nI think that makes a lot of sense! If we report a colour temperature, we\nshould make sure the gains we use correspond to it!\n\n> \n> Regards,\n> Stefan\n> \n> > \n> > > \n> > > Signed-off-by: Stefan Klug \n> > > ---\n> > > src/ipa/rkisp1/algorithms/awb.cpp | 5 +++--\n> > > 1 file changed, 3 insertions(+), 2 deletions(-)\n> > > \n> > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> > > index 9244a1e64f41..347d38d226b4 100644\n> > > --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> > > @@ -320,8 +320,6 @@ void Awb::process(IPAContext &context,\n> > > rgbMeans.b() < kMeanMinThreshold)\n> > > return;\n> > > \n> > > - activeState.awb.automatic.temperatureK = estimateCCT(rgbMeans);\n> > > -\n> > > /*\n> > > * Estimate the red and blue gains to apply in a grey world. The green\n> > > * gain is hardcoded to 1.0. Avoid divisions by zero by clamping the\n> > > @@ -343,8 +341,11 @@ void Awb::process(IPAContext &context,\n> > > \n> > > /* Filter the values to avoid oscillations. */\n> > > double speed = 0.2;\n> > > + double ct = estimateCCT(rgbMeans);\n> > > + ct = ct * speed + activeState.awb.automatic.temperatureK * (1 - speed);\n> > > gains = gains * speed + activeState.awb.automatic.gains * (1 - speed);\n> > > \n> > > + activeState.awb.automatic.temperatureK = static_cast(ct);\n> > > activeState.awb.automatic.gains = gains;\n> > > \n> > > LOG(RkISP1Awb, Debug)\n> > > -- \n> > > 2.43.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 CB566C32FE\n\tfor ;\n\tWed, 19 Mar 2025 15:37:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3945F68951;\n\tWed, 19 Mar 2025 16:37:50 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C8D0C68945\n\tfor ;\n\tWed, 19 Mar 2025 16:37:48 +0100 (CET)","from pendragon.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 D54A51E6;\n\tWed, 19 Mar 2025 16:36:05 +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=\"fIkVoj9N\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1742398565;\n\tbh=kEjuzbE56TdB4qw2ErxM97P+zQNp+KSzGGrSsn6opao=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=fIkVoj9N7ZJGYnNJr8FHjUOJTEsGsIe8z0I/xzvLcRCj6CPptAN+lvmtg5lweW7bG\n\t79PcvbcrlYSNDovJvMTRBgrw/7V6TGNfmg0WT3oBqytgyuHf91SMnhhHded9BFqn4w\n\tjE3YvnTzoD43eGQ4LDP3NyoWp1rYKEEAETUu3Bo0=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20250217100203.297894-1-stefan.klug@ideasonboard.com>\n\t<20250217100203.297894-8-stefan.klug@ideasonboard.com>\n\t<174005401879.3771432.4689584838028479438@ping.linuxembedded.co.uk>\n\t","Subject":"Re: [PATCH 07/10] ipa: rkisp1: Damp color temperature regulation","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","To":"Stefan Klug ","Date":"Wed, 19 Mar 2025 15:37:46 +0000","Message-ID":"<174239866619.783128.17356899134816042594@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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\" "}}]