[v2,16/17] ipa: rkisp1: awb: Take the CCM into account for the AWB gains calculation

22994 diff mbox series

The AWB measurements are taken after the CCM. This can be seen by
enabling debug logging on AWB, disabling AWB (stats will still be
processed) and manually chaning the CCM.

This means that the estimated colour temperature and the corresponding
CCM also lead to changed rgbMeans which in turn leads to oscillations.
Fix that by applying the inverse transform on the rgbMeans.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>

---

Changes in v2:
- Improved commit message
- Added comment in the code
---
 src/ipa/rkisp1/algorithms/awb.cpp | 6 ++++++
 1 file changed, 6 insertions(+)

Quoting Stefan Klug (2025-03-19 16:11:21)
> The AWB measurements are taken after the CCM. This can be seen by
> enabling debug logging on AWB, disabling AWB (stats will still be
> processed) and manually chaning the CCM.
> 
> This means that the estimated colour temperature and the corresponding
> CCM also lead to changed rgbMeans which in turn leads to oscillations.
> Fix that by applying the inverse transform on the rgbMeans.
> 

I'm happy to see this one!

> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> 
> ---
> 
> Changes in v2:
> - Improved commit message
> - Added comment in the code
> ---
>  src/ipa/rkisp1/algorithms/awb.cpp | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> index 324bc14b42a0..e8b04d03748d 100644
> --- a/src/ipa/rkisp1/algorithms/awb.cpp
> +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> @@ -412,6 +412,12 @@ RGB<double> Awb::calculateRgbMeans(const IPAFrameContext &frameContext, const rk
>                 rgbMeans = rgbMeans.max(0.0);
>         }
>  
> +       /*
> +        * The ISP computes the AWB means after applying the ccm. Apply the
> +        * inverse as we want to get the raw means before the colour gains.
> +        */
> +       rgbMeans = frameContext.ccm.ccm.inverse() * rgbMeans;
> +

So 'simple' yet so powerful thanks to the helpers before.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


Sounds silly, but I would have written this as :

	rgbMeans = rgbMeans * frameContext.ccm.ccm.inverse();

But I (very much) hope that's identical, but reads as 'multiply the rgb
by the inverse ccm' to me, (as opposed to 'multiply the inverse CCM by
the rgbMeans).

Anyway, it shouldn't make a difference so it's fine either way to me.


>         /*
>          * The ISP computes the AWB means after applying the colour gains,
>          * divide by the gains that were used to get the raw means from the
> -- 
> 2.43.0
>

On Mon, Mar 31, 2025 at 06:56:27PM +0100, Kieran Bingham wrote:
> Quoting Stefan Klug (2025-03-19 16:11:21)
> > The AWB measurements are taken after the CCM. This can be seen by
> > enabling debug logging on AWB, disabling AWB (stats will still be
> > processed) and manually chaning the CCM.
> > 
> > This means that the estimated colour temperature and the corresponding
> > CCM also lead to changed rgbMeans which in turn leads to oscillations.
> > Fix that by applying the inverse transform on the rgbMeans.
> 
> I'm happy to see this one!
> 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > 
> > ---
> > 
> > Changes in v2:
> > - Improved commit message
> > - Added comment in the code
> > ---
> >  src/ipa/rkisp1/algorithms/awb.cpp | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> > index 324bc14b42a0..e8b04d03748d 100644
> > --- a/src/ipa/rkisp1/algorithms/awb.cpp
> > +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> > @@ -412,6 +412,12 @@ RGB<double> Awb::calculateRgbMeans(const IPAFrameContext &frameContext, const rk
> >                 rgbMeans = rgbMeans.max(0.0);
> >         }
> >  
> > +       /*
> > +        * The ISP computes the AWB means after applying the ccm. Apply the

s/ccm/CCM/

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > +        * inverse as we want to get the raw means before the colour gains.
> > +        */
> > +       rgbMeans = frameContext.ccm.ccm.inverse() * rgbMeans;
> > +
> 
> So 'simple' yet so powerful thanks to the helpers before.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> 
> Sounds silly, but I would have written this as :
> 
> 	rgbMeans = rgbMeans * frameContext.ccm.ccm.inverse();
> 
> But I (very much) hope that's identical,

It's not :-)

> but reads as 'multiply the rgb
> by the inverse ccm' to me, (as opposed to 'multiply the inverse CCM by
> the rgbMeans).
> 
> Anyway, it shouldn't make a difference so it's fine either way to me.
> 
> >         /*
> >          * The ISP computes the AWB means after applying the colour gains,
> >          * divide by the gains that were used to get the raw means from the