[PATCH v2 14/17] ipa: rkisp1: Damp color temperature regulation

[Stefan Klug]

Hi,

On Tue, Apr 01, 2025 at 04:40:16AM +0300, Laurent Pinchart wrote:
> On Mon, Mar 31, 2025 at 06:50:27PM +0100, Kieran Bingham wrote:
> > Quoting Stefan Klug (2025-03-19 16:11:19)
> > > Damp the regulation of the color temperature with the same factor as the
> > > gains.  Not damping the color temperature leads to visible flicker, as
> > > the CCM changes too much.
> > > 
> > > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > 
> > > ---
> > > 
> > > Changes in v2:
> > > - Collected tags
> > > ---
> > >  src/ipa/rkisp1/algorithms/awb.cpp | 3 +++
> > >  src/ipa/rkisp1/algorithms/ccm.cpp | 4 ----
> > >  2 files changed, 3 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> > > index 286d9e3e2018..149c6aa59c8d 100644
> > > --- a/src/ipa/rkisp1/algorithms/awb.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> > > @@ -342,9 +342,12 @@ void Awb::process(IPAContext &context,
> > >  
> > >         /* Filter the values to avoid oscillations. */
> > >         double speed = 0.2;
> > > +       double ct = estimateCCT(rgbMeans);
> > > +       ct = ct * speed + activeState.awb.automatic.temperatureK * (1 - speed);
> > >         awbResult.gains = awbResult.gains * speed +
> > >                           activeState.awb.automatic.gains * (1 - speed);
> > >  
> > > +       activeState.awb.automatic.temperatureK = static_cast<unsigned int>(ct);
> > 
> > I'm confused by this patch being late in the series and adding a
> > calculation of temperature, without removing or modifying an existing
> > calculation.
> 
> And furthermore, since we moved the AWB algorithms to libipa,
> estimateCCT is not really meant to be called here. Something seems
> weird.

You are both absolutely right. This patch misses a fixup that was
waiting further up the commit tree and was missed in this version:

@@ -327,8 +327,6 @@ void Awb::process(IPAContext &context,
 	RkISP1AwbStats awbStats{ rgbMeans };
 	AwbResult awbResult = awbAlgo_->calculateAwb(awbStats, frameContext.lux.lux);

-	activeState.awb.automatic.temperatureK = awbResult.colourTemperature;
-
 	/* Metadata shall contain the up to date measurement */
 	metadata.set(controls::ColourTemperature, activeState.awb.automatic.temperatureK);

@@ -342,7 +340,7 @@ void Awb::process(IPAContext &context,

 	/* Filter the values to avoid oscillations. */
 	double speed = 0.2;
-	double ct = estimateCCT(rgbMeans);
+	double ct = awbResult.colourTemperature;
 	ct = ct * speed + activeState.awb.automatic.temperatureK * (1 - speed);
 	awbResult.gains = awbResult.gains * speed +
 			  activeState.awb.automatic.gains * (1 - speed);


Will be fixed in v3.

Regards,
Stefan


> 
> > Is this duplicating where the automatic color temperature is estimated?
> > Do we now call estimateCCT twice ?
> > 
> > >         activeState.awb.automatic.gains = awbResult.gains;
> > >  
> > >         LOG(RkISP1Awb, Debug)
> > > diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp
> > > index 303ac3dd2fe2..b7f32f0d5d8a 100644
> > > --- a/src/ipa/rkisp1/algorithms/ccm.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/ccm.cpp
> > > @@ -139,10 +139,6 @@ void Ccm::prepare(IPAContext &context, const uint32_t frame,
> > >         }
> > >  
> > >         uint32_t ct = frameContext.awb.temperatureK;
> > > -       /*
> > > -        * \todo The colour temperature will likely be noisy, add filtering to
> > > -        * avoid updating the CCM matrix all the time.
> > > -        */
> 
> I don't think the comment should be removed. Even with damping, the
> colour temperature is noisy. What we need here is a filter with a
> threshold. Unless updating the CCM every frame is now fine.
> 
> > >         if (frame > 0 && ct == ct_) {
> > >                 frameContext.ccm.ccm = context.activeState.ccm.automatic;
> > >                 return;
> 
> -- 
> Regards,
> 
> Laurent Pinchart