[PATCH v4 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 review.

> On Tue, May 28, 2024 at 06:11:23PM +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.
>> 
>> Exposure computation already subtracts black level, no changes are
>> needed there.
>> 
>> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> Reviewed-by: Andrei Konovalov <andrey.konovalov.ynk at gmail.com>
>> ---
>>  src/ipa/simple/soft_simple.cpp | 36 ++++++++++++++++++++++------------
>>  1 file changed, 23 insertions(+), 13 deletions(-)
>> 
>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
>> index a5bb2bbf..722aac83 100644
>> --- a/src/ipa/simple/soft_simple.cpp
>> +++ b/src/ipa/simple/soft_simple.cpp
>> @@ -5,6 +5,8 @@
>>   * Simple Software Image Processing Algorithm module
>>   */
>>  
>> +#include <numeric>
>> +#include <stdint.h>
>>  #include <sys/mman.h>
>>  
>>  #include <linux/v4l2-controls.h>
>> @@ -240,28 +242,36 @@ 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.
>
> These two lines should be moved to [*] below.

OK.

>> +	 * Black level must be subtracted to get the correct AWB ratios,
>> +	 * from the sensor range.
>
> Thanks for adding a comment, but could you explain *why* this is needed
> ?

OK.

>>  	 */
>> -	if (stats_->sumR_ <= stats_->sumG_ / 4)
>> -		params_->gainR = 1024;
>> -	else
>> -		params_->gainR = 256 * stats_->sumG_ / stats_->sumR_;
>> -
>> -	if (stats_->sumB_ <= stats_->sumG_ / 4)
>> -		params_->gainB = 1024;
>> -	else
>> -		params_->gainB = 256 * stats_->sumG_ / stats_->sumB_;
>> +	const uint64_t nPixels = std::accumulate(
>> +		histogram.begin(), histogram.end(), 0);
>> +	auto subtractBlackLevel = [blackLevel, nPixels](
>> +					  uint64_t sum, uint64_t div)
>
> I'd avoid the line break between the previous two lines, it makes the
> code harder to read.
>
>> +		-> uint64_t {
>> +		return sum - blackLevel * nPixels / div;
>> +	};
>> +	const uint64_t sumR = subtractBlackLevel(stats_->sumR_, 4);
>> +	const uint64_t sumG = subtractBlackLevel(stats_->sumG_, 2);
>> +	const uint64_t sumB = subtractBlackLevel(stats_->sumB_, 4);
>
> Wouldn't the following be both simpler and more readable ?

Yes, will change it.

> 	const uint64_t offset = blackLevel * nPixels;
> 	const uint64_t sumR = stats_->sumR_ - offset / 4;
> 	const uint64_t sumG = stats_->sumG_ - offset / 2;
> 	const uint64_t sumB = stats_->sumB_ - offset / 4;
>
> [*]
>
> With these small issues addressed,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
>> +
>> +	params_->gainR = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;
>> +	params_->gainB = sumB <= sumG / 4 ? 1024 : 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. */