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

[Laurent Pinchart]

Hi Milan,

Thank you for the patch.

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 ?

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

> +		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 ?

> +			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. */

-- 
Regards,

Laurent Pinchart