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

[Jean-Michel Hautbois]

On 20/10/2021 10:41, Jean-Michel Hautbois wrote:
> Hi Laurent,
> 
> 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 :-)
>>
>>> 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.
> 
> I am not sure I understand this comment :-/. I started by changing
> - constexpr uint16_t kFixedPoint3_13 = 8191;
> + constexpr uint16_t kFixedPoint3_13 = 8192;
> 
> But now I read it again, and I think I might have misunderstand ?

I have done that, I think this is what you meant ?

+constexpr uint16_t Awb::Threshold(float value)
+{
+       /* AWB Thresholds are in the range [0, 8191] */
+       constexpr uint16_t kMaxThreshold = 8191;
+
+       return value * kMaxThreshold;
+}
+

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