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

[Jean-Michel Hautbois]

Hi !

On 14/10/2021 23:56, Laurent Pinchart wrote:
> 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 :-)
> 

Exactly, and also because most of the perceived luminance is well
represented by the green channel (red and blud have smaller
contributions, 60% for green, 30% for red and 10% for blue to simplify).

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

Yes, it is not a bad thing but does not improve anything :-).

>>> 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.
> 
>> 	...
>> }
>