[{"id":33801,"web_url":"https://patchwork.libcamera.org/comment/33801/","msgid":"<174344342783.3394313.17614483638134861616@ping.linuxembedded.co.uk>","date":"2025-03-31T17:50:27","subject":"Re: [PATCH v2 14/17] 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 16:11:19)\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> Signed-off-by: Stefan Klug \n> Reviewed-by: Kieran Bingham \n> \n> ---\n> \n> Changes in v2:\n> - Collected tags\n> ---\n> src/ipa/rkisp1/algorithms/awb.cpp | 3 +++\n> src/ipa/rkisp1/algorithms/ccm.cpp | 4 ----\n> 2 files changed, 3 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> index 286d9e3e2018..149c6aa59c8d 100644\n> --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> @@ -342,9 +342,12 @@ 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> awbResult.gains = awbResult.gains * speed +\n> activeState.awb.automatic.gains * (1 - speed);\n> \n> + activeState.awb.automatic.temperatureK = static_cast(ct);\n\nI'm confused by this patch being late in the series and adding a\ncalculation of temperature, without removing or modifying an existing\ncalculation.\n\nIs this duplicating where the automatic color temperature is estimated?\nDo we now call estimateCCT twice ?\n\n> activeState.awb.automatic.gains = awbResult.gains;\n> \n> LOG(RkISP1Awb, Debug)\n> diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp\n> index 303ac3dd2fe2..b7f32f0d5d8a 100644\n> --- a/src/ipa/rkisp1/algorithms/ccm.cpp\n> +++ b/src/ipa/rkisp1/algorithms/ccm.cpp\n> @@ -139,10 +139,6 @@ void Ccm::prepare(IPAContext &context, const uint32_t frame,\n> }\n> \n> uint32_t ct = frameContext.awb.temperatureK;\n> - /*\n> - * \\todo The colour temperature will likely be noisy, add filtering to\n> - * avoid updating the CCM matrix all the time.\n> - */\n> if (frame > 0 && ct == ct_) {\n> frameContext.ccm.ccm = context.activeState.ccm.automatic;\n> return;\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 59582C3213\n\tfor ;\n\tMon, 31 Mar 2025 17:50:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B21EE68986;\n\tMon, 31 Mar 2025 19:50:31 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D675E68967\n\tfor ;\n\tMon, 31 Mar 2025 19:50:30 +0200 (CEST)","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 383C1703;\n\tMon, 31 Mar 2025 19:48:39 +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=\"QHa956wU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1743443319;\n\tbh=v5CQaiaYhrSmVbFx9+sGUYf8Eu71xhf049AH1e7md5M=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=QHa956wUfsvlDRybOcZ+/A3X78hGk+Ir8l271rhlNjpRwmPxEkVp5+7cVF1KpIMQg\n\tTpevPqLbijMY7XuPZ+snguyOFRcy9+wWfS4tkKtJ9SyZy7YHlzZA+ZL5y0xoUonktM\n\tIito2daBzPkLhk8jvptg8YxNLQw5dTHVVW7QUw2g=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250319161152.63625-15-stefan.klug@ideasonboard.com>","References":"<20250319161152.63625-1-stefan.klug@ideasonboard.com>\n\t<20250319161152.63625-15-stefan.klug@ideasonboard.com>","Subject":"Re: [PATCH v2 14/17] ipa: rkisp1: Damp color temperature regulation","From":"Kieran Bingham ","Cc":"Stefan Klug ","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 31 Mar 2025 18:50:27 +0100","Message-ID":"<174344342783.3394313.17614483638134861616@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":33832,"web_url":"https://patchwork.libcamera.org/comment/33832/","msgid":"<20250401014016.GV14432@pendragon.ideasonboard.com>","date":"2025-04-01T01:40:16","subject":"Re: [PATCH v2 14/17] ipa: rkisp1: Damp color temperature regulation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Mar 31, 2025 at 06:50:27PM +0100, Kieran Bingham wrote:\n> Quoting Stefan Klug (2025-03-19 16:11:19)\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> > Signed-off-by: Stefan Klug \n> > Reviewed-by: Kieran Bingham \n> > \n> > ---\n> > \n> > Changes in v2:\n> > - Collected tags\n> > ---\n> > src/ipa/rkisp1/algorithms/awb.cpp | 3 +++\n> > src/ipa/rkisp1/algorithms/ccm.cpp | 4 ----\n> > 2 files changed, 3 insertions(+), 4 deletions(-)\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> > index 286d9e3e2018..149c6aa59c8d 100644\n> > --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> > @@ -342,9 +342,12 @@ 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> > awbResult.gains = awbResult.gains * speed +\n> > activeState.awb.automatic.gains * (1 - speed);\n> > \n> > + activeState.awb.automatic.temperatureK = static_cast(ct);\n> \n> I'm confused by this patch being late in the series and adding a\n> calculation of temperature, without removing or modifying an existing\n> calculation.\n\nAnd furthermore, since we moved the AWB algorithms to libipa,\nestimateCCT is not really meant to be called here. Something seems\nweird.\n\n> Is this duplicating where the automatic color temperature is estimated?\n> Do we now call estimateCCT twice ?\n> \n> > activeState.awb.automatic.gains = awbResult.gains;\n> > \n> > LOG(RkISP1Awb, Debug)\n> > diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp\n> > index 303ac3dd2fe2..b7f32f0d5d8a 100644\n> > --- a/src/ipa/rkisp1/algorithms/ccm.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/ccm.cpp\n> > @@ -139,10 +139,6 @@ void Ccm::prepare(IPAContext &context, const uint32_t frame,\n> > }\n> > \n> > uint32_t ct = frameContext.awb.temperatureK;\n> > - /*\n> > - * \\todo The colour temperature will likely be noisy, add filtering to\n> > - * avoid updating the CCM matrix all the time.\n> > - */\n\nI don't think the comment should be removed. Even with damping, the\ncolour temperature is noisy. What we need here is a filter with a\nthreshold. Unless updating the CCM every frame is now fine.\n\n> > if (frame > 0 && ct == ct_) {\n> > frameContext.ccm.ccm = context.activeState.ccm.automatic;\n> > return;","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 60754C323E\n\tfor ;\n\tTue, 1 Apr 2025 01:40:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B1E4C62C66;\n\tTue, 1 Apr 2025 03:40:41 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EBA7862C66\n\tfor ;\n\tTue, 1 Apr 2025 03:40:40 +0200 (CEST)","from pendragon.ideasonboard.com (85-76-147-224-nat.elisa-mobile.fi\n\t[85.76.147.224])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C4AB16F9;\n\tTue, 1 Apr 2025 03:38:48 +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=\"eNVFzCE9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1743471529;\n\tbh=X2jrU7o0rhbRG6cSgOHuHxoYgY3RchtBSOyhbikv3OE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=eNVFzCE9eJETpVvdNIwZPb7PGnUSygPODyKCMM3eyL0l4DiUCXuUFEVLFlB9ZyEnM\n\tOKiOgK5UNzCpgAZAOc+Qfs/AmZtOCaaDMxaP7Z1BAZwV62QUG3izPtwzL1I3AbMDN7\n\txjepKKRPQCzIfPewJTS3qLm2j+eb9ui6ryZvdVKQ=","Date":"Tue, 1 Apr 2025 04:40:16 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 14/17] ipa: rkisp1: Damp color temperature regulation","Message-ID":"<20250401014016.GV14432@pendragon.ideasonboard.com>","References":"<20250319161152.63625-1-stefan.klug@ideasonboard.com>\n\t<20250319161152.63625-15-stefan.klug@ideasonboard.com>\n\t<174344342783.3394313.17614483638134861616@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<174344342783.3394313.17614483638134861616@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":33839,"web_url":"https://patchwork.libcamera.org/comment/33839/","msgid":"<3t5iwbspabdmytlxh3oqeh7uj4xoixksqlfv43kuqujhanoahk@z4y3jizvq6gd>","date":"2025-04-01T09:20:17","subject":"Re: [PATCH v2 14/17] 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,\n\nOn Tue, Apr 01, 2025 at 04:40:16AM +0300, Laurent Pinchart wrote:\n> On Mon, Mar 31, 2025 at 06:50:27PM +0100, Kieran Bingham wrote:\n> > Quoting Stefan Klug (2025-03-19 16:11:19)\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> > > Signed-off-by: Stefan Klug \n> > > Reviewed-by: Kieran Bingham \n> > > \n> > > ---\n> > > \n> > > Changes in v2:\n> > > - Collected tags\n> > > ---\n> > > src/ipa/rkisp1/algorithms/awb.cpp | 3 +++\n> > > src/ipa/rkisp1/algorithms/ccm.cpp | 4 ----\n> > > 2 files changed, 3 insertions(+), 4 deletions(-)\n> > > \n> > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> > > index 286d9e3e2018..149c6aa59c8d 100644\n> > > --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> > > @@ -342,9 +342,12 @@ 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> > > awbResult.gains = awbResult.gains * speed +\n> > > activeState.awb.automatic.gains * (1 - speed);\n> > > \n> > > + activeState.awb.automatic.temperatureK = static_cast(ct);\n> > \n> > I'm confused by this patch being late in the series and adding a\n> > calculation of temperature, without removing or modifying an existing\n> > calculation.\n> \n> And furthermore, since we moved the AWB algorithms to libipa,\n> estimateCCT is not really meant to be called here. Something seems\n> weird.\n\nYou are both absolutely right. This patch misses a fixup that was\nwaiting further up the commit tree and was missed in this version:\n\n@@ -327,8 +327,6 @@ void Awb::process(IPAContext &context,\n \tRkISP1AwbStats awbStats{ rgbMeans };\n \tAwbResult awbResult = awbAlgo_->calculateAwb(awbStats, frameContext.lux.lux);\n\n-\tactiveState.awb.automatic.temperatureK = awbResult.colourTemperature;\n-\n \t/* Metadata shall contain the up to date measurement */\n \tmetadata.set(controls::ColourTemperature, activeState.awb.automatic.temperatureK);\n\n@@ -342,7 +340,7 @@ void Awb::process(IPAContext &context,\n\n \t/* Filter the values to avoid oscillations. */\n \tdouble speed = 0.2;\n-\tdouble ct = estimateCCT(rgbMeans);\n+\tdouble ct = awbResult.colourTemperature;\n \tct = ct * speed + activeState.awb.automatic.temperatureK * (1 - speed);\n \tawbResult.gains = awbResult.gains * speed +\n \t\t\t activeState.awb.automatic.gains * (1 - speed);\n\n\nWill be fixed in v3.\n\nRegards,\nStefan\n\n\n> \n> > Is this duplicating where the automatic color temperature is estimated?\n> > Do we now call estimateCCT twice ?\n> > \n> > > activeState.awb.automatic.gains = awbResult.gains;\n> > > \n> > > LOG(RkISP1Awb, Debug)\n> > > diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp\n> > > index 303ac3dd2fe2..b7f32f0d5d8a 100644\n> > > --- a/src/ipa/rkisp1/algorithms/ccm.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/ccm.cpp\n> > > @@ -139,10 +139,6 @@ void Ccm::prepare(IPAContext &context, const uint32_t frame,\n> > > }\n> > > \n> > > uint32_t ct = frameContext.awb.temperatureK;\n> > > - /*\n> > > - * \\todo The colour temperature will likely be noisy, add filtering to\n> > > - * avoid updating the CCM matrix all the time.\n> > > - */\n> \n> I don't think the comment should be removed. Even with damping, the\n> colour temperature is noisy. What we need here is a filter with a\n> threshold. Unless updating the CCM every frame is now fine.\n> \n> > > if (frame > 0 && ct == ct_) {\n> > > frameContext.ccm.ccm = context.activeState.ccm.automatic;\n> > > return;\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 107B4C3213\n\tfor ;\n\tTue, 1 Apr 2025 09:20:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A588F68986;\n\tTue, 1 Apr 2025 11:20:22 +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 C300468947\n\tfor ;\n\tTue, 1 Apr 2025 11:20:20 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:14c7:4fcc:495b:719f])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C060D8DB;\n\tTue, 1 Apr 2025 11:18:28 +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=\"miRlK2sm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1743499108;\n\tbh=pqbL9JpMU2swY8fC0tmec699e8pRgAEbIsXaG2xMhHQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=miRlK2smSNxWQGktYyL8KbvxBtzMdWbpIEdtByLv/0h1iLD3C5uOAD9/eAOE0y+ez\n\txwQQJ5CnQCI/8yCQHy9IsSpnJ1wtH6nlItpWU5zYZGrYBsnpxMMRmChG+KsA69wIp6\n\tDl/KotdseQJ2nzEH+7P8xYm51MyTiQmh/4D/t/Ng=","Date":"Tue, 1 Apr 2025 11:20:17 +0200","From":"Stefan Klug ","To":"Laurent Pinchart ","Cc":"Kieran Bingham , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 14/17] ipa: rkisp1: Damp color temperature regulation","Message-ID":"<3t5iwbspabdmytlxh3oqeh7uj4xoixksqlfv43kuqujhanoahk@z4y3jizvq6gd>","References":"<20250319161152.63625-1-stefan.klug@ideasonboard.com>\n\t<20250319161152.63625-15-stefan.klug@ideasonboard.com>\n\t<174344342783.3394313.17614483638134861616@ping.linuxembedded.co.uk>\n\t<20250401014016.GV14432@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250401014016.GV14432@pendragon.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\" "}}]