[libcamera-devel] [PATCH v3 03/14] ipa: ipu3: awb: Use saturation under 90%

[Jean-Michel Hautbois]

Hi Laurent,

On 22/10/2021 05:59, Laurent Pinchart wrote:
> Hi Jean-Michel,
> 
> Thank you for the patch.
> 
> On Thu, Oct 21, 2021 at 06:43:50PM +0200, Jean-Michel Hautbois wrote:
>> The AWB grey world algorithm tries to find a grey value and it can't do
>> it on over-exposed images. To exclude those, the saturation ratio is
>> used for each cell, and the cell is included only if this ratio is 0.
>>
>> Now that we have changed the threshold, more cells may be considered as
>> partially saturated and excluded, preventing the algorithm from running
>> efficiently.
>>
>> Change that behaviour, and consider 90% as a good enough ratio.
>>
>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>> ---
>>   src/ipa/ipu3/algorithms/awb.cpp | 28 ++++++++++++++++++++++++++--
>>   1 file changed, 26 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
>> index 4364928c..ce01791b 100644
>> --- a/src/ipa/ipu3/algorithms/awb.cpp
>> +++ b/src/ipa/ipu3/algorithms/awb.cpp
>> @@ -19,6 +19,18 @@ LOG_DEFINE_CATEGORY(IPU3Awb)
>>   
>>   static constexpr uint32_t kMinGreenLevelInZone = 32;
>>   
>> +/*
>> + * Minimum proportion of non-saturated cells in a zone for the zone to be used
>> + * by the AWB algorithm.
>> + */
>> +static constexpr double kMinRelevantCellsRatio = 0.8;
>> +
>> +/*
>> + * Maximum ratio of saturated pixels in a cell for the cell to be considered
>> + * non-saturated and counted by the AWB algorithm.
>> + */
>> +static constexpr uint32_t kSaturationThreshold = 255 * 90 / 100;
> 
> In addition to the comments, I've also commented on constant names in
> the review of v2. You're of course free to disagree with review
> comments, but with no answer to the review, I can't know if you have
> kept the previous names on purpose or just haven't noticed the proposal.
> 

Oh, yes, you are right, I got the comments, but did not read your 
comment properly. Fixed in v4, thanks and sorry :-/.

>> +
>>   /**
>>    * \struct Accumulator
>>    * \brief RGB statistics for a given zone
>> @@ -160,7 +172,8 @@ int Awb::configure(IPAContext &context,
>>   	 * for it to be relevant for the grey world algorithm.
>>   	 * \todo This proportion could be configured.
>>   	 */
>> -	cellsPerZoneThreshold_ = cellsPerZoneX_ * cellsPerZoneY_ * 80 / 100;
>> +	cellsPerZoneThreshold_ = cellsPerZoneX_ * cellsPerZoneY_ * kMinRelevantCellsRatio;
>> +	LOG(IPU3Awb, Debug) << "Threshold for AWB is set to " << cellsPerZoneThreshold_;
>>   
>>   	return 0;
>>   }
>> @@ -234,7 +247,18 @@ void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)
>>   				reinterpret_cast<const ipu3_uapi_awb_set_item *>(
>>   					&stats->awb_raw_buffer.meta_data[cellPosition]
>>   				);
>> -			if (currentCell->sat_ratio == 0) {
>> +
>> +			/*
>> +			 * Use cells which have less than 90%
>> +			 * saturation as an initial means to include
>> +			 * otherwise bright cells which are not fully
>> +			 * saturated.
>> +			 *
>> +			 * \todo The 90% saturation rate may require
>> +			 * further empirical measurements and
>> +			 * optimisation during camera tuning phases.
>> +			 */
>> +			if (currentCell->sat_ratio <= kSaturationThreshold) {
>>   				/* The cell is not saturated, use the current cell */
>>   				awbStats_[awbZonePosition].counted++;
>>   				uint32_t greenValue = currentCell->Gr_avg + currentCell->Gb_avg;
>