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

[Laurent Pinchart]

Hi Milan,

Thank you for the patch.

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.

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

>  	 */
> -	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 ?

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

-- 
Regards,

Laurent Pinchart