[v2,14/17] ipa: rkisp1: Damp color temperature regulation
diff mbox series

Message ID 20250319161152.63625-15-stefan.klug@ideasonboard.com
State Superseded
Headers show
Series
  • Some rkisp1 awb improvements
Related show

Commit Message

Stefan Klug March 19, 2025, 4:11 p.m. UTC 
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@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@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(-)

Comments

Kieran Bingham March 31, 2025, 5:50 p.m. UTC | #1 
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@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@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.

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.
> -        */
>         if (frame > 0 && ct == ct_) {
>                 frameContext.ccm.ccm = context.activeState.ccm.automatic;
>                 return;
> -- 
> 2.43.0
>
Laurent Pinchart April 1, 2025, 1:40 a.m. UTC | #2 
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@ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@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;
Stefan Klug April 1, 2025, 9:20 a.m. UTC | #3 
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@ideasonboard.com>
> > > Reviewed-by: Kieran Bingham <kieran.bingham@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

Patch
diff mbox series 

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);
 	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.
-	 */
 	if (frame > 0 && ct == ct_) {
 		frameContext.ccm.ccm = context.activeState.ccm.automatic;
 		return;