[libcamera-devel,v4,30/32] ipa: rkisp1: awb: Clamp gains to prevent divisions by zero

17339 diff mbox series

The gain values are currently clamped to the range [0.0, 3.996] used by
the hardware. A zero value makes little sense, as it would completely
remove the contribution of the corresponding color channel from the AWB
accumulators, but worse, would lead to divisions by zero when
calculating the raw means in subsequent iterations. Prevent this by
setting the minimum gain value to 1/256.

While at it, clamp the gain values before filtering them, to improve the
stability of the control loop.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/ipa/rkisp1/algorithms/awb.cpp | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:58)
> The gain values are currently clamped to the range [0.0, 3.996] used by
> the hardware. A zero value makes little sense, as it would completely
> remove the contribution of the corresponding color channel from the AWB
> accumulators, but worse, would lead to divisions by zero when
> calculating the raw means in subsequent iterations. Prevent this by
> setting the minimum gain value to 1/256.
> 
> While at it, clamp the gain values before filtering them, to improve the
> stability of the control loop.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/awb.cpp | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> index de54c4d24650..5f2535688c93 100644
> --- a/src/ipa/rkisp1/algorithms/awb.cpp
> +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> @@ -265,21 +265,29 @@ void Awb::process(IPAContext &context,
>  
>         frameContext.awb.temperatureK = estimateCCT(redMean, greenMean, blueMean);
>  
> -       /* Estimate the red and blue gains to apply in a grey world. */
> +       /*
> +        * Estimate the red and blue gains to apply in a grey world. The green
> +        * gain is hardcoded to 0.

0 or 1?

With that 

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

> +        */
>         double redGain = greenMean / (redMean + 1);
>         double blueGain = greenMean / (blueMean + 1);
>  
> +       /*
> +        * Clamp the gain values to the hardware, which expresses gains as Q2.8
> +        * unsigned integer values. Set the minimum just above zero to avoid
> +        * divisions by zero when computing the raw means in subsequent
> +        * iterations.
> +        */
> +       redGain = std::clamp(redGain, 1.0 / 256, 1023.0 / 256);
> +       blueGain = std::clamp(blueGain, 1.0 / 256, 1023.0 / 256);
> +
>         /* Filter the values to avoid oscillations. */
>         double speed = 0.2;
>         redGain = speed * redGain + (1 - speed) * activeState.awb.gains.automatic.red;
>         blueGain = speed * blueGain + (1 - speed) * activeState.awb.gains.automatic.blue;
>  
> -       /*
> -        * Gain values are unsigned integer value, range 0 to 4 with 8 bit
> -        * fractional part. Hardcode the green gain to 1.0.
> -        */
> -       activeState.awb.gains.automatic.red = std::clamp(redGain, 0.0, 1023.0 / 256);
> -       activeState.awb.gains.automatic.blue = std::clamp(blueGain, 0.0, 1023.0 / 256);
> +       activeState.awb.gains.automatic.red = redGain;
> +       activeState.awb.gains.automatic.blue = blueGain;
>         activeState.awb.gains.automatic.green = 1.0;
>  
>         LOG(RkISP1Awb, Debug) << std::showpoint
> -- 
> Regards,
> 
> Laurent Pinchart
>

Hi

On Wed, Sep 21, 2022 at 12:41:24AM +0100, Kieran Bingham via libcamera-devel wrote:
> Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:58)
> > The gain values are currently clamped to the range [0.0, 3.996] used by
> > the hardware. A zero value makes little sense, as it would completely
> > remove the contribution of the corresponding color channel from the AWB
> > accumulators, but worse, would lead to divisions by zero when
> > calculating the raw means in subsequent iterations. Prevent this by
> > setting the minimum gain value to 1/256.
> >
> > While at it, clamp the gain values before filtering them, to improve the
> > stability of the control loop.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/ipa/rkisp1/algorithms/awb.cpp | 22 +++++++++++++++-------
> >  1 file changed, 15 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> > index de54c4d24650..5f2535688c93 100644
> > --- a/src/ipa/rkisp1/algorithms/awb.cpp
> > +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> > @@ -265,21 +265,29 @@ void Awb::process(IPAContext &context,
> >
> >         frameContext.awb.temperatureK = estimateCCT(redMean, greenMean, blueMean);
> >
> > -       /* Estimate the red and blue gains to apply in a grey world. */
> > +       /*
> > +        * Estimate the red and blue gains to apply in a grey world. The green
> > +        * gain is hardcoded to 0.
>
> 0 or 1?

I guess a gain of 0 means multiplying the color channel by 1 :) ?

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

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

>
> > +        */
> >         double redGain = greenMean / (redMean + 1);
> >         double blueGain = greenMean / (blueMean + 1);
> >
> > +       /*
> > +        * Clamp the gain values to the hardware, which expresses gains as Q2.8
> > +        * unsigned integer values. Set the minimum just above zero to avoid
> > +        * divisions by zero when computing the raw means in subsequent
> > +        * iterations.
> > +        */
> > +       redGain = std::clamp(redGain, 1.0 / 256, 1023.0 / 256);
> > +       blueGain = std::clamp(blueGain, 1.0 / 256, 1023.0 / 256);
> > +
> >         /* Filter the values to avoid oscillations. */
> >         double speed = 0.2;
> >         redGain = speed * redGain + (1 - speed) * activeState.awb.gains.automatic.red;
> >         blueGain = speed * blueGain + (1 - speed) * activeState.awb.gains.automatic.blue;
> >
> > -       /*
> > -        * Gain values are unsigned integer value, range 0 to 4 with 8 bit
> > -        * fractional part. Hardcode the green gain to 1.0.
> > -        */
> > -       activeState.awb.gains.automatic.red = std::clamp(redGain, 0.0, 1023.0 / 256);
> > -       activeState.awb.gains.automatic.blue = std::clamp(blueGain, 0.0, 1023.0 / 256);
> > +       activeState.awb.gains.automatic.red = redGain;
> > +       activeState.awb.gains.automatic.blue = blueGain;
> >         activeState.awb.gains.automatic.green = 1.0;
> >
> >         LOG(RkISP1Awb, Debug) << std::showpoint
> > --
> > Regards,
> >
> > Laurent Pinchart
> >