[PATCH 2/5] libcamera: software_isp: Honor black level in AWB

[Milan Zamazal]

Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:

> Hi Milan,
>
> Thank you for the patch.

Hi Laurent,

thank you for the reviews.

> On Tue, Apr 23, 2024 at 08:19:57PM +0200, Milan Zamazal wrote:
>> The white balance computation didn't consider black level.  This is
>> wrong because then the computed ratios are off when they are computed
>> from the whole brightness range rather than the sensor range.
>> 
>> This patch adjusts white balance computation for the black level.  There
>> is no need to change white balance application in debayering as this is
>> already applied after black level correction.
>
> Makes sense.
>
>> Exposure computation already subtracts black level, no changes are
>> needed there.
>> 
>> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> ---
>>  src/ipa/simple/soft_simple.cpp | 43 ++++++++++++++++++++++++----------
>>  1 file changed, 31 insertions(+), 12 deletions(-)
>> 
>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
>> index b9fb58b5..30956a19 100644
>> --- a/src/ipa/simple/soft_simple.cpp
>> +++ b/src/ipa/simple/soft_simple.cpp
>> @@ -5,6 +5,8 @@
>>   * soft_simple.cpp - Simple Software Image Processing Algorithm module
>>   */
>>  
>> +#include <numeric>
>> +#include <stdint.h>
>>  #include <sys/mman.h>
>>  
>>  #include <linux/v4l2-controls.h>
>> @@ -240,28 +242,45 @@ void IPASoftSimple::stop()
>>  
>>  void IPASoftSimple::processStats(const ControlList &sensorControls)
>>  {
>> +	SwIspStats::Histogram histogram = stats_->yHistogram;
>> +	if (ignoreUpdates_ > 0)
>> +		blackLevel_.update(histogram);
>> +	const uint8_t blackLevel = blackLevel_.get();
>> +	params_->blackLevel = blackLevel;
>> +
>>  	/*
>>  	 * Calculate red and blue gains for AWB.
>>  	 * Clamp max gain at 4.0, this also avoids 0 division.
>>  	 */
>> -	if (stats_->sumR_ <= stats_->sumG_ / 4)
>> -		params_->gainR = 1024;
>> -	else
>> -		params_->gainR = 256 * stats_->sumG_ / stats_->sumR_;
>> +	{
>
> It's not that I dislike proper indentation, but maybe not just for the
> sake of it ? :-) Why do you enclose that block in curly braces,
> especially given that you remove extra indentation in the next patch ?

This is to limit the scope of local variables to the relevant region (which is
extended in the next patch).  Functionally, it's just a matter of style so I can
remove the braces.

> A comment here to explain why you need to subtract the black level would
> be useful.

OK, will add one.

>> +		const uint64_t nPixels = std::accumulate(
>> +			histogram.begin(), histogram.end(), 0);
>> +		auto subtractBlackLevel = [blackLevel, nPixels](
>> +						  uint64_t sum, uint64_t div)
>> +			-> uint64_t {
>> +			uint64_t reducedSum = sum - blackLevel * nPixels / div;
>
> Looks good so far.
>
>> + uint64_t spreadSum = reducedSum * 256 / (256 - blackLevel);
>
> I'm not sure about this. Why do you want to "spread" the sum ?

Right, the multiplication is meaningless here, I'll remove it.

>> +			return spreadSum;
>> +		};
>> +		const uint64_t sumR = subtractBlackLevel(stats_->sumR_, 4);
>> +		const uint64_t sumG = subtractBlackLevel(stats_->sumG_, 2);
>> +		const uint64_t sumB = subtractBlackLevel(stats_->sumB_, 4);
>> +
>> +		if (sumR <= sumG / 4)
>> +			params_->gainR = 1024;
>> +		else
>> +			params_->gainR = 256 * sumG / sumR;
>>  
>> -	if (stats_->sumB_ <= stats_->sumG_ / 4)
>> -		params_->gainB = 1024;
>> -	else
>> -		params_->gainB = 256 * stats_->sumG_ / stats_->sumB_;
>> +		if (sumB <= sumG / 4)
>> +			params_->gainB = 1024;
>> +		else
>> +			params_->gainB = 256 * sumG / sumB;
>> +	}
>>  
>>  	/* Green gain and gamma values are fixed */
>>  	params_->gainG = 256;
>>  	params_->gamma = 0.5;
>>  
>> -	if (ignoreUpdates_ > 0)
>> -		blackLevel_.update(stats_->yHistogram);
>> -	params_->blackLevel = blackLevel_.get();
>> -
>>  	setIspParams.emit();
>>  
>>  	/* \todo Switch to the libipa/algorithm.h API someday. */