[libcamera-devel] [PATCH 01/13] ipa: ipu3: awb: Set a threshold for the green saturation

[Laurent Pinchart]

Hello,

On Thu, Oct 14, 2021 at 11:23:13AM +0100, Kieran Bingham wrote:
> Quoting Jean-Michel Hautbois (2021-10-13 16:41:13)
> > We can have a saturation ratio per cell, giving the percentage of pixels
> > over a threshold within a cell where 100% is set to 0xff.
> > 
> > The parameter structure 'ipu3_uapi_awb_config_s' contains four fields to
> > set the threshold, one per channel.
> > The blue field is also used to configure the ImgU and make it calculate
> > the saturation ratio or not.
> > 
> > Set a green value saturated when it is more than 230 (90% of the maximum
> > value 255, coded as 8191).
> 
> Why do we only determine a lower saturation on one component?

I assume because we only use the stats for that component :-)

> Shouldn't they be balanced (perhaps in accordance with the white balance
> somehow?).

If the assumption is correct, then we could, and it would make no
difference.

> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> > ---
> >  src/ipa/ipu3/algorithms/awb.cpp | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> > index e2b18336..5574bd44 100644
> > --- a/src/ipa/ipu3/algorithms/awb.cpp
> > +++ b/src/ipa/ipu3/algorithms/awb.cpp
> > @@ -326,9 +326,9 @@ void Awb::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
> >  
> >  void Awb::prepare(IPAContext &context, ipu3_uapi_params *params)
> >  {
> > -       params->acc_param.awb.config.rgbs_thr_gr = 8191;
> > +       params->acc_param.awb.config.rgbs_thr_gr = (90 * 8191 / 100);

No need for parentheses.

> >         params->acc_param.awb.config.rgbs_thr_r = 8191;
> > -       params->acc_param.awb.config.rgbs_thr_gb = 8191;
> > +       params->acc_param.awb.config.rgbs_thr_gb = (90 * 8191 / 100);
> >         params->acc_param.awb.config.rgbs_thr_b = IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT
> >                                                | IPU3_UAPI_AWB_RGBS_THR_B_EN
> >                                                | 8191;
> 
> That's interesting that thr_b has those enable flags. Do they apply
> only to B? or does that enable all of them?

As far as I undertand, the single flag applies to all channels.

> I wonder if a helper function would make the values clearer:
> 
> (Awb private:)
> uint16_t Awb::Threshold(float value)

Make it

constexpr uint16_t Awb::Threshold(float value)

> {
> 	/* AWB Thresholds are represented in FixedPoint 3.13 */
> 	constexpr uint16_t kFixedPoint3_13 = 8191;
> 
> 	return value * kFixedPoint3_13;
> }

In a 3.13 fixed-point format, 1.0 would be 8192, not 8191. The above
would thus be misleading. 8191 is the maximum value of a 12bpp pixel,
it's not a fixed-point number in this context.

> void Awb::prepare(IPAContext &context, ipu3_uapi_params *params)
> {
> 	/*
> 	 * Green saturation thresholds are reduced because...
> 	 */
> 	params->acc_param.awb.config.rgbs_thr_r = Threshold(1.0);
> 	params->acc_param.awb.config.rgbs_thr_gr = Threshold(0.9);
> 	params->acc_param.awb.config.rgbs_thr_gb = Threshold(0.9);
> 	params->acc_param.awb.config.rgbs_thr_b = Threshold(1.0);
> 
> 	/* Enable saturation inclusion on thr_b because ... */
> 	params->acc_param.awb.config.rgbs_thr_b |= IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT |
> 					 	   IPU3_UAPI_AWB_RGBS_THR_B_EN;

I like splitting this from the previous line.

> 	...
> }

-- 
Regards,

Laurent Pinchart