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

[Laurent Pinchart]

On Fri, Apr 26, 2024 at 12:37:31PM +0200, Milan Zamazal wrote:
> 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).

The next patch removes the nested scope, so I don't really see why it's
needed here.

I'm fine restricing scope of variables when there's a technical reason
to do so, for instance to use pattenrs RAII for locks. That doesn't seem
to be the case here, and the code compiles fine without the nested
scope.

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

-- 
Regards,

Laurent Pinchart