[PATCH 2/5] libcamera: software_isp: Honor black level in AWB
Milan Zamazal
mzamazal at redhat.com
Fri Apr 26 12:37:31 CEST 2024
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. */
More information about the libcamera-devel
mailing list