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

[Jean-Michel Hautbois]

Hi Laurent,

On 17/06/2022 22:39, Laurent Pinchart wrote:
> 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.
> 	 */

Thanks for the documentation rewriting !
I think it should be:

- * Pout = Pin * (1 * gain / 8192)
+ * Pout = Pin * (1 + gain / 8192)

> 
> 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);

I like this one :-) !

> 
>> +}
>> +
>>   /**
>>    * \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().
Indeed.

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

Thanks.

> 
>> -	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];
>