[v3,10/16] ipa: rkisp1: algorithms: awb: Fix wrong colour temperature reporting

23126 diff mbox series

In commit b60bd37b1a49 ("ipa: rkisp1: Move calculation of RGB means into
own function") the output of the current measured colour temperature as
metadata was incorrectly added. Remove it.

Fixes: b60bd37b1a49 ("ipa: rkisp1: Move calculation of RGB means into own function")
Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>

---

Changes in v3:
- Added this patch
---
 src/ipa/rkisp1/algorithms/awb.cpp | 3 ---
 1 file changed, 3 deletions(-)

Quoting Stefan Klug (2025-04-03 16:49:15)
> In commit b60bd37b1a49 ("ipa: rkisp1: Move calculation of RGB means into
> own function") the output of the current measured colour temperature as
> metadata was incorrectly added. Remove it.
> 

Ooops, it even says "Commit doesn't contain any functional changes." But
it did!

Is the colour temperature set elsewhere now?



> Fixes: b60bd37b1a49 ("ipa: rkisp1: Move calculation of RGB means into own function")
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> 

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

> ---
> 
> Changes in v3:
> - Added this patch
> ---
>  src/ipa/rkisp1/algorithms/awb.cpp | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> index a9759e53f593..79c4c658406d 100644
> --- a/src/ipa/rkisp1/algorithms/awb.cpp
> +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> @@ -311,9 +311,6 @@ void Awb::process(IPAContext &context,
>  
>         activeState.awb.temperatureK = awbResult.colourTemperature;
>  
> -       /* Metadata shall contain the up to date measurement */
> -       metadata.set(controls::ColourTemperature, activeState.awb.temperatureK);
> -
>         /*
>          * Clamp the gain values to the hardware, which expresses gains as Q2.8
>          * unsigned integer values. Set the minimum just above zero to avoid
> -- 
> 2.43.0
>

On Fri, May 02, 2025 at 09:08:46AM +0100, Kieran Bingham wrote:
> Quoting Stefan Klug (2025-04-03 16:49:15)
> > In commit b60bd37b1a49 ("ipa: rkisp1: Move calculation of RGB means into
> > own function") the output of the current measured colour temperature as
> > metadata was incorrectly added. Remove it.
> > 
> 
> Ooops, it even says "Commit doesn't contain any functional changes." But
> it did!
> 
> Is the colour temperature set elsewhere now?

It's set earlier in the process() function, to
frameContext.awb.temperatureK instead of activeState.awb.temperatureK.

There was a discussion on whether or not we should add a metadata
control to report the colour temperature estimated for the frame, in
addition to the one that was used to process the frame. Using
activeState.awb.temperatureK switches to the latter. Regardless of the
answer to that question, the aforementioned commit introduced a
functional change, so I think this fix is right.

> > Fixes: b60bd37b1a49 ("ipa: rkisp1: Move calculation of RGB means into own function")
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

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

> > ---
> > 
> > Changes in v3:
> > - Added this patch
> > ---
> >  src/ipa/rkisp1/algorithms/awb.cpp | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> > index a9759e53f593..79c4c658406d 100644
> > --- a/src/ipa/rkisp1/algorithms/awb.cpp
> > +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> > @@ -311,9 +311,6 @@ void Awb::process(IPAContext &context,
> >  
> >         activeState.awb.temperatureK = awbResult.colourTemperature;
> >  
> > -       /* Metadata shall contain the up to date measurement */
> > -       metadata.set(controls::ColourTemperature, activeState.awb.temperatureK);
> > -
> >         /*
> >          * Clamp the gain values to the hardware, which expresses gains as Q2.8
> >          * unsigned integer values. Set the minimum just above zero to avoid

On Sat, May 03, 2025 at 12:01:54AM +0300, Laurent Pinchart wrote:
> On Fri, May 02, 2025 at 09:08:46AM +0100, Kieran Bingham wrote:
> > Quoting Stefan Klug (2025-04-03 16:49:15)
> > > In commit b60bd37b1a49 ("ipa: rkisp1: Move calculation of RGB means into
> > > own function") the output of the current measured colour temperature as
> > > metadata was incorrectly added. Remove it.
> > > 
> > 
> > Ooops, it even says "Commit doesn't contain any functional changes." But
> > it did!
> > 
> > Is the colour temperature set elsewhere now?
> 
> It's set earlier in the process() function, to
> frameContext.awb.temperatureK instead of activeState.awb.temperatureK.
> 
> There was a discussion on whether or not we should add a metadata
> control to report the colour temperature estimated for the frame, in
> addition to the one that was used to process the frame. Using
> activeState.awb.temperatureK switches to the latter. Regardless of the
> answer to that question, the aforementioned commit introduced a
> functional change, so I think this fix is right.
> 
> > > Fixes: b60bd37b1a49 ("ipa: rkisp1: Move calculation of RGB means into own function")
> > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > 
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>


Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> 
> > > ---
> > > 
> > > Changes in v3:
> > > - Added this patch
> > > ---
> > >  src/ipa/rkisp1/algorithms/awb.cpp | 3 ---
> > >  1 file changed, 3 deletions(-)
> > > 
> > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> > > index a9759e53f593..79c4c658406d 100644
> > > --- a/src/ipa/rkisp1/algorithms/awb.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> > > @@ -311,9 +311,6 @@ void Awb::process(IPAContext &context,
> > >  
> > >         activeState.awb.temperatureK = awbResult.colourTemperature;
> > >  
> > > -       /* Metadata shall contain the up to date measurement */
> > > -       metadata.set(controls::ColourTemperature, activeState.awb.temperatureK);
> > > -
> > >         /*
> > >          * Clamp the gain values to the hardware, which expresses gains as Q2.8
> > >          * unsigned integer values. Set the minimum just above zero to avoid
> 
> -- 
> Regards,
> 
> Laurent Pinchart