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

[Laurent Pinchart]

Hi Jean-Michel,

On Fri, Jun 17, 2022 at 11:25:53PM +0200, Jean-Michel Hautbois wrote:
> 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)

Indeed, that's what I meant. If we cross-review our reviews we'll
achieve a good result :-)

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

-- 
Regards,

Laurent Pinchart