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

[Laurent Pinchart]

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.

> 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