[libcamera-devel] [PATCH] ipa: ipu3: awb: Correct the coefficient factor

[Laurent Pinchart]

Hi Jean-Michel,

Thank you for the patch.

I'd write "Correct the gains calculation" in the subject, as it's not
just the factor.

On Fri, Jun 17, 2022 at 10:32:11AM +0200, Jean-Michel Hautbois via libcamera-devel wrote:
> 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.

The effect of this patch on image quality is really great !

> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/awb.cpp | 21 +++++++++++++++++----
>  src/ipa/ipu3/algorithms/awb.h   |  1 +
>  2 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> index 5c232d92..6abaf75f 100644
> --- a/src/ipa/ipu3/algorithms/awb.cpp
> +++ b/src/ipa/ipu3/algorithms/awb.cpp
> @@ -409,6 +409,19 @@ constexpr uint16_t Awb::threshold(float value)
>  	return value * 8191;
>  }
>  
> +constexpr uint16_t Awb::fixedGain(double gain)

Does "fixed" refer to the fact that the return value is a fixed-point
number ? As the function does more than conversion to fixed point, I'd
call it gainValue().

> +{
> +	/*
> +	 * For BNR parameters WB gain factor for the three channels

I count four channels :-)

> +	 * [Ggr, Ggb, Gb, Gr]. Their precision is U3.13 and the range is (0, 8)

Gb and Gr are slightly confusing here, they usually refer to the
green-blue and green-red components, while I suppose they stand for
gain-blue and gain-red in this context.

> +	 * and the actual gain is Gx + 1, it is typically Gx = 1.
> +	 *
> +	 * Pout = {Pin * (1 + Gx)}.
> +	 */

If I may propose a small improvement to the documentation (as it's
important to record this, given that the kernel API doesn't document the
gain format correctly):

	/*
	 * 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.
	 */

You can also write [0, 8) instead of [0, 8[.

> +	gain = std::clamp((gain - 1.0), 0.0, 8.0);

No need for the inner parentheses.

> +	return gain * 8192;

And this holds on a single line:

	return std::clamp(gain - 1.0, 0.0, 8.0) * 8192;

But we should actually limit the value to [0.0, 8.0[, not [0.0, 8.0].
One option would be

	return std::clamp((gain - 1.0) * 8192, 0.0, 65535.0);

> +}
> +
>  /**
>   * \copydoc libcamera::ipa::Algorithm::prepare
>   */
> @@ -451,10 +464,10 @@ void Awb::prepare(IPAContext &context, ipu3_uapi_params *params)
>  	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 */

Let's drop this comment, it's now part of fixedGain().

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

> -	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 = fixedGain(context.activeState.awb.gains.green);
> +	params->acc_param.bnr.wb_gains.r  = fixedGain(context.activeState.awb.gains.red);
> +	params->acc_param.bnr.wb_gains.b  = fixedGain(context.activeState.awb.gains.blue);
> +	params->acc_param.bnr.wb_gains.gb = fixedGain(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..3154541d 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 fixedGain(double gain);
>  
>  	std::vector<RGB> zones_;
>  	Accumulator awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];

-- 
Regards,

Laurent Pinchart