[libcamera-devel] [PATCH v2] ipa: ipu3: awb: Correct the gains calculation

[Kieran Bingham]

Hi JM,

Quoting Jean-Michel Hautbois via libcamera-devel (2022-06-17 22:28:54)
> The factor used right now in the IPU3 is 8192, as a multiplier of the
> estimated gain. This is wrong, as the isp is adding 1.0 to the gain
> applied, ie Pout = { Pin * (1 + Gx) }.
> 
> Fix it, and to ease the reading, introduce a small helper function.
> 

Aha - so it was just an 'off by one' right ;-)


Having seen the image quality improvements from this, *ship it* ;-)

It looks like the resulting image was a bit darker, so perhaps we've
under-compensated the exposure or gains somewhere else due to these AWB
gains being high.

But that could be identified on top, this is clearly a bug fix.

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

> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/awb.cpp | 27 ++++++++++++++++++++++-----
>  src/ipa/ipu3/algorithms/awb.h   |  1 +
>  2 files changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> index 5c232d92..70426722 100644
> --- a/src/ipa/ipu3/algorithms/awb.cpp
> +++ b/src/ipa/ipu3/algorithms/awb.cpp
> @@ -409,6 +409,23 @@ constexpr uint16_t Awb::threshold(float value)
>         return value * 8191;
>  }
>  
> +constexpr uint16_t Awb::gainValue(double gain)
> +{
> +       /*
> +        * The colour gains applied by the BNR for the four channels (Gr, R, B
> +        * and Gb) are expressed in the parameters structure as 16-bit integers
> +        * that store a fixed-point U3.13 value in the range [0, 8[.
> +        *
> +        * The real gain value is equal to the gain parameter plus one, i.e.
> +        *
> +        * Pout = Pin * (1 + gain / 8192)
> +        *
> +        * where 'Pin' is the input pixel value, 'Pout' the output pixel value,
> +        * and 'gain' the gain in the parameters structure as a 16-bit integer.
> +        */
> +       return std::clamp((gain - 1.0) * 8192, 0.0, 65535.0);
> +}
> +
>  /**
>   * \copydoc libcamera::ipa::Algorithm::prepare
>   */
> @@ -450,11 +467,11 @@ void Awb::prepare(IPAContext &context, ipu3_uapi_params *params)
>                                                         * params->acc_param.bnr.opt_center.x_reset;
>         params->acc_param.bnr.opt_center_sqr.y_sqr_reset = params->acc_param.bnr.opt_center.y_reset
>                                                         * params->acc_param.bnr.opt_center.y_reset;
> -       /* Convert to u3.13 fixed point values */
> -       params->acc_param.bnr.wb_gains.gr = 8192 * context.activeState.awb.gains.green;
> -       params->acc_param.bnr.wb_gains.r  = 8192 * context.activeState.awb.gains.red;
> -       params->acc_param.bnr.wb_gains.b  = 8192 * context.activeState.awb.gains.blue;
> -       params->acc_param.bnr.wb_gains.gb = 8192 * context.activeState.awb.gains.green;
> +
> +       params->acc_param.bnr.wb_gains.gr = gainValue(context.activeState.awb.gains.green);
> +       params->acc_param.bnr.wb_gains.r  = gainValue(context.activeState.awb.gains.red);
> +       params->acc_param.bnr.wb_gains.b  = gainValue(context.activeState.awb.gains.blue);
> +       params->acc_param.bnr.wb_gains.gb = gainValue(context.activeState.awb.gains.green);
>  
>         LOG(IPU3Awb, Debug) << "Color temperature estimated: " << asyncResults_.temperatureK;
>  
> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h
> index 9a50a985..0acd2148 100644
> --- a/src/ipa/ipu3/algorithms/awb.h
> +++ b/src/ipa/ipu3/algorithms/awb.h
> @@ -73,6 +73,7 @@ private:
>         void awbGreyWorld();
>         uint32_t estimateCCT(double red, double green, double blue);
>         static constexpr uint16_t threshold(float value);
> +       static constexpr uint16_t gainValue(double gain);
>  
>         std::vector<RGB> zones_;
>         Accumulator awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];
> -- 
> 2.34.1
>