[libcamera-devel] [PATCH 04/13] ipa: ipu3: awb: Change minimal green threshold value

[Jean-Michel Hautbois]

On 15/10/2021 00:55, Laurent Pinchart wrote:
> On Thu, Oct 14, 2021 at 11:50:22AM +0100, Kieran Bingham wrote:
>> Quoting Jean-Michel Hautbois (2021-10-13 16:41:16)
>>> When the zones are used for the grey world algorithm, we need a minimal
>>> number of zones to make it relevant, and we want the green values to be
>>> at least 32 today (with a maximum of 255).
>>>
>>> This value is a bit high, as we now are correcting the black levels, and
>>> a sudden change of colors can be visible for the end user when the gains
>>> are applied.
>>>
>>> Lower the value to 16.
> 
> If I understand the patch correctly, the most relevant part here is the
> minimal green value for a zone to be considered. I would then write the
> above as
> 
> When zones are used for the grey world algorithm, they are only
> considered if their average green value is at least 32/255 to exclude
> zones that are too dark and don't provide relevant colour information
> (on the opposite side of the spectrum, saturated regions are excluded by
> the ImgU statistics engine).
> 
> The algorithm requires a minimal number of zones that meet this criteria
> in order to run. Now that we correct the black level, the 32/255 minimal
> value is a bit high and prevents the algorithm for running in low-light
> conditions. Lower the value to 16/255 to fix it.
> 

Thanks, it is better wrote (as always ;-)).

>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
>>> ---
>>>  src/ipa/ipu3/algorithms/awb.cpp | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
>>> index 30693923..b18874a3 100644
>>> --- a/src/ipa/ipu3/algorithms/awb.cpp
>>> +++ b/src/ipa/ipu3/algorithms/awb.cpp
>>> @@ -17,7 +17,7 @@ namespace ipa::ipu3::algorithms {
>>>  
>>>  LOG_DEFINE_CATEGORY(IPU3Awb)
>>
>> Do you document the meaning of this constant, and what it is used for /
>> effects of changing it in the later documentation series?
>>
>> If not, could you add something please?
>>
>> (Either in this patch, or later)
> 
> Agreed.
> 
> It could also make sense to capture the commit message or parts of it in
> the documentation.
> 

OK, I will add those in the v2 for the "Document IPAIPU3" series.

> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
>> Acked-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>
>>> -static constexpr uint32_t kMinGreenLevelInZone = 32;
>>> +static constexpr uint32_t kMinGreenLevelInZone = 16;
>>>  
>>>  /**
>>>   * \struct Accumulator
>