| Message ID | 20260127162923.112675-1-mzamazal@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
On Tue, Jan 27, 2026 at 05:29:23PM +0100, Milan Zamazal wrote: > The black level offset subtracted in AWB is wrong. It assumes that the > stats contain sums of the individual colour pixels. But they actually > contain sums of the colour channels of larger "superpixels" consisting > of the individual colour pixels. Each of the RGB colour values and the > computed luminosity (a histogram entry) are added once to the stats per > such a superpixel. This means the offset computed from the black level > and the number of pixels should be used as it is, not divided. > > The patch fixes the subtracted offset. Since the evaluation is the same > for all the three colours now, the individual class variables are > replaced with a single RGB variable. > > Fixes: 4e13c6f55bd667 ("Honor black level in AWB") > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > .../internal/software_isp/swisp_stats.h | 16 +++++----------- > src/ipa/simple/algorithms/awb.cpp | 10 ++++------ > src/libcamera/software_isp/swstats_cpu.cpp | 10 ++++------ > 3 files changed, 13 insertions(+), 23 deletions(-) > > diff --git a/include/libcamera/internal/software_isp/swisp_stats.h b/include/libcamera/internal/software_isp/swisp_stats.h > index 3c9860185..d9d0d9be8 100644 > --- a/include/libcamera/internal/software_isp/swisp_stats.h > +++ b/include/libcamera/internal/software_isp/swisp_stats.h > @@ -10,6 +10,8 @@ > #include <array> > #include <stdint.h> > > +#include "libcamera/internal/vector.h" > + > namespace libcamera { > > /** > @@ -26,17 +28,9 @@ struct SwIspStats { > */ > bool valid; > /** > - * \brief Holds the sum of all sampled red pixels > - */ > - uint64_t sumR_; > - /** > - * \brief Holds the sum of all sampled green pixels > - */ > - uint64_t sumG_; > - /** > - * \brief Holds the sum of all sampled blue pixels > + * \brief Sums of colour channels of all the sampled pixels > */ > - uint64_t sumB_; > + RGB<uint64_t> sum_; > /** > * \brief Number of bins in the yHistogram > */ > @@ -46,7 +40,7 @@ struct SwIspStats { > */ > using Histogram = std::array<uint32_t, kYHistogramSize>; > /** > - * \brief A histogram of luminance values > + * \brief A histogram of luminance values of all the sampled pixels > */ > Histogram yHistogram; > }; > diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp > index 0080865aa..75e4ae4a4 100644 > --- a/src/ipa/simple/algorithms/awb.cpp > +++ b/src/ipa/simple/algorithms/awb.cpp > @@ -1,6 +1,6 @@ > /* SPDX-License-Identifier: LGPL-2.1-or-later */ > /* > - * Copyright (C) 2024, Red Hat Inc. > + * Copyright (C) 2024-2026 Red Hat Inc. > * > * Auto white balance > */ > @@ -73,9 +73,7 @@ void Awb::process(IPAContext &context, > histogram.begin(), histogram.end(), uint64_t(0)); > const uint64_t offset = blackLevel * nPixels; > const uint64_t minValid = 1; > - const uint64_t sumR = stats->sumR_ > offset / 4 ? stats->sumR_ - offset / 4 : minValid; > - const uint64_t sumG = stats->sumG_ > offset / 2 ? stats->sumG_ - offset / 2 : minValid; > - const uint64_t sumB = stats->sumB_ > offset / 4 ? stats->sumB_ - offset / 4 : minValid; > + const RGB<uint64_t> sum = (stats->sum_ - offset).max(minValid); > > /* > * Calculate red and blue gains for AWB. > @@ -83,9 +81,9 @@ void Awb::process(IPAContext &context, > */ > auto &gains = context.activeState.awb.gains; > gains = { { > - sumR <= sumG / 4 ? 4.0f : static_cast<float>(sumG) / sumR, > + sum.r() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.r(), > 1.0, > - sumB <= sumG / 4 ? 4.0f : static_cast<float>(sumG) / sumB, > + sum.b() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.b(), > } }; > > RGB<double> rgbGains{ { 1 / gains.r(), 1 / gains.g(), 1 / gains.b() } }; > diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp > index c931edb41..5c3011a7f 100644 > --- a/src/libcamera/software_isp/swstats_cpu.cpp > +++ b/src/libcamera/software_isp/swstats_cpu.cpp > @@ -185,9 +185,9 @@ static constexpr unsigned int kBlueYMul = 29; /* 0.114 * 256 */ > stats_.yHistogram[yVal * SwIspStats::kYHistogramSize / (256 * 256 * (div))]++; > > #define SWSTATS_FINISH_LINE_STATS() \ > - stats_.sumR_ += sumR; \ > - stats_.sumG_ += sumG; \ > - stats_.sumB_ += sumB; > + stats_.sum_.r() += sumR; \ > + stats_.sum_.g() += sumG; \ > + stats_.sum_.b() += sumB; > > void SwStatsCpu::statsBGGR8Line0(const uint8_t *src[]) > { > @@ -332,9 +332,7 @@ void SwStatsCpu::startFrame(uint32_t frame) > if (window_.width == 0) > LOG(SwStatsCpu, Error) << "Calling startFrame() without setWindow()"; > > - stats_.sumR_ = 0; > - stats_.sumB_ = 0; > - stats_.sumG_ = 0; > + stats_.sum_ = RGB<uint64_t>({ 0, 0, 0 }); > stats_.yHistogram.fill(0); > } >
On 27.01.26 17:29, Milan Zamazal wrote: > The black level offset subtracted in AWB is wrong. It assumes that the > stats contain sums of the individual colour pixels. But they actually > contain sums of the colour channels of larger "superpixels" consisting > of the individual colour pixels. Each of the RGB colour values and the > computed luminosity (a histogram entry) are added once to the stats per > such a superpixel. This means the offset computed from the black level > and the number of pixels should be used as it is, not divided. > > The patch fixes the subtracted offset. Since the evaluation is the same > for all the three colours now, the individual class variables are > replaced with a single RGB variable. > > Fixes: 4e13c6f55bd667 ("Honor black level in AWB") > Signed-off-by: Milan Zamazal<mzamazal@redhat.com> Reviewed-by: Robert Mader<robert.mader@collabora.com> Tested-by: Robert Mader<robert.mader@collabora.com> > --- > .../internal/software_isp/swisp_stats.h | 16 +++++----------- > src/ipa/simple/algorithms/awb.cpp | 10 ++++------ > src/libcamera/software_isp/swstats_cpu.cpp | 10 ++++------ > 3 files changed, 13 insertions(+), 23 deletions(-) > > diff --git a/include/libcamera/internal/software_isp/swisp_stats.h b/include/libcamera/internal/software_isp/swisp_stats.h > index 3c9860185..d9d0d9be8 100644 > --- a/include/libcamera/internal/software_isp/swisp_stats.h > +++ b/include/libcamera/internal/software_isp/swisp_stats.h > @@ -10,6 +10,8 @@ > #include <array> > #include <stdint.h> > > +#include "libcamera/internal/vector.h" > + > namespace libcamera { > > /** > @@ -26,17 +28,9 @@ struct SwIspStats { > */ > bool valid; > /** > - * \brief Holds the sum of all sampled red pixels > - */ > - uint64_t sumR_; > - /** > - * \brief Holds the sum of all sampled green pixels > - */ > - uint64_t sumG_; > - /** > - * \brief Holds the sum of all sampled blue pixels > + * \brief Sums of colour channels of all the sampled pixels > */ > - uint64_t sumB_; > + RGB<uint64_t> sum_; > /** > * \brief Number of bins in the yHistogram > */ > @@ -46,7 +40,7 @@ struct SwIspStats { > */ > using Histogram = std::array<uint32_t, kYHistogramSize>; > /** > - * \brief A histogram of luminance values > + * \brief A histogram of luminance values of all the sampled pixels > */ > Histogram yHistogram; > }; > diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp > index 0080865aa..75e4ae4a4 100644 > --- a/src/ipa/simple/algorithms/awb.cpp > +++ b/src/ipa/simple/algorithms/awb.cpp > @@ -1,6 +1,6 @@ > /* SPDX-License-Identifier: LGPL-2.1-or-later */ > /* > - * Copyright (C) 2024, Red Hat Inc. > + * Copyright (C) 2024-2026 Red Hat Inc. > * > * Auto white balance > */ > @@ -73,9 +73,7 @@ void Awb::process(IPAContext &context, > histogram.begin(), histogram.end(), uint64_t(0)); > const uint64_t offset = blackLevel * nPixels; > const uint64_t minValid = 1; > - const uint64_t sumR = stats->sumR_ > offset / 4 ? stats->sumR_ - offset / 4 : minValid; > - const uint64_t sumG = stats->sumG_ > offset / 2 ? stats->sumG_ - offset / 2 : minValid; > - const uint64_t sumB = stats->sumB_ > offset / 4 ? stats->sumB_ - offset / 4 : minValid; > + const RGB<uint64_t> sum = (stats->sum_ - offset).max(minValid); > > /* > * Calculate red and blue gains for AWB. > @@ -83,9 +81,9 @@ void Awb::process(IPAContext &context, > */ > auto &gains = context.activeState.awb.gains; > gains = { { > - sumR <= sumG / 4 ? 4.0f : static_cast<float>(sumG) / sumR, > + sum.r() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.r(), > 1.0, > - sumB <= sumG / 4 ? 4.0f : static_cast<float>(sumG) / sumB, > + sum.b() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.b(), > } }; > > RGB<double> rgbGains{ { 1 / gains.r(), 1 / gains.g(), 1 / gains.b() } }; > diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp > index c931edb41..5c3011a7f 100644 > --- a/src/libcamera/software_isp/swstats_cpu.cpp > +++ b/src/libcamera/software_isp/swstats_cpu.cpp > @@ -185,9 +185,9 @@ static constexpr unsigned int kBlueYMul = 29; /* 0.114 * 256 */ > stats_.yHistogram[yVal * SwIspStats::kYHistogramSize / (256 * 256 * (div))]++; > > #define SWSTATS_FINISH_LINE_STATS() \ > - stats_.sumR_ += sumR; \ > - stats_.sumG_ += sumG; \ > - stats_.sumB_ += sumB; > + stats_.sum_.r() += sumR; \ > + stats_.sum_.g() += sumG; \ > + stats_.sum_.b() += sumB; > > void SwStatsCpu::statsBGGR8Line0(const uint8_t *src[]) > { > @@ -332,9 +332,7 @@ void SwStatsCpu::startFrame(uint32_t frame) > if (window_.width == 0) > LOG(SwStatsCpu, Error) << "Calling startFrame() without setWindow()"; > > - stats_.sumR_ = 0; > - stats_.sumB_ = 0; > - stats_.sumG_ = 0; > + stats_.sum_ = RGB<uint64_t>({ 0, 0, 0 }); > stats_.yHistogram.fill(0); > } >
Hi 2026. 01. 27. 17:29 keltezéssel, Milan Zamazal írta: > The black level offset subtracted in AWB is wrong. It assumes that the > stats contain sums of the individual colour pixels. But they actually > contain sums of the colour channels of larger "superpixels" consisting > of the individual colour pixels. Each of the RGB colour values and the > computed luminosity (a histogram entry) are added once to the stats per > such a superpixel. This means the offset computed from the black level > and the number of pixels should be used as it is, not divided. > > The patch fixes the subtracted offset. Since the evaluation is the same > for all the three colours now, the individual class variables are > replaced with a single RGB variable. > > Fixes: 4e13c6f55bd667 ("Honor black level in AWB") > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > .../internal/software_isp/swisp_stats.h | 16 +++++----------- > src/ipa/simple/algorithms/awb.cpp | 10 ++++------ > src/libcamera/software_isp/swstats_cpu.cpp | 10 ++++------ > 3 files changed, 13 insertions(+), 23 deletions(-) > > diff --git a/include/libcamera/internal/software_isp/swisp_stats.h b/include/libcamera/internal/software_isp/swisp_stats.h > index 3c9860185..d9d0d9be8 100644 > --- a/include/libcamera/internal/software_isp/swisp_stats.h > +++ b/include/libcamera/internal/software_isp/swisp_stats.h > @@ -10,6 +10,8 @@ > #include <array> > #include <stdint.h> > > +#include "libcamera/internal/vector.h" > + > namespace libcamera { > > /** > @@ -26,17 +28,9 @@ struct SwIspStats { > */ > bool valid; > /** > - * \brief Holds the sum of all sampled red pixels > - */ > - uint64_t sumR_; > - /** > - * \brief Holds the sum of all sampled green pixels > - */ > - uint64_t sumG_; > - /** > - * \brief Holds the sum of all sampled blue pixels > + * \brief Sums of colour channels of all the sampled pixels > */ > - uint64_t sumB_; > + RGB<uint64_t> sum_; > /** > * \brief Number of bins in the yHistogram > */ > @@ -46,7 +40,7 @@ struct SwIspStats { > */ > using Histogram = std::array<uint32_t, kYHistogramSize>; > /** > - * \brief A histogram of luminance values > + * \brief A histogram of luminance values of all the sampled pixels > */ > Histogram yHistogram; > }; > diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp > index 0080865aa..75e4ae4a4 100644 > --- a/src/ipa/simple/algorithms/awb.cpp > +++ b/src/ipa/simple/algorithms/awb.cpp > @@ -1,6 +1,6 @@ > /* SPDX-License-Identifier: LGPL-2.1-or-later */ > /* > - * Copyright (C) 2024, Red Hat Inc. > + * Copyright (C) 2024-2026 Red Hat Inc. > * > * Auto white balance > */ > @@ -73,9 +73,7 @@ void Awb::process(IPAContext &context, > histogram.begin(), histogram.end(), uint64_t(0)); > const uint64_t offset = blackLevel * nPixels; > const uint64_t minValid = 1; > - const uint64_t sumR = stats->sumR_ > offset / 4 ? stats->sumR_ - offset / 4 : minValid; > - const uint64_t sumG = stats->sumG_ > offset / 2 ? stats->sumG_ - offset / 2 : minValid; > - const uint64_t sumB = stats->sumB_ > offset / 4 ? stats->sumB_ - offset / 4 : minValid; > + const RGB<uint64_t> sum = (stats->sum_ - offset).max(minValid); What happens if the subtraction wraps around? Previously that was avoided by the removed check. Or is wraparound actually impossible now? > [...]
Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > Hi > > 2026. 01. 27. 17:29 keltezéssel, Milan Zamazal írta: >> The black level offset subtracted in AWB is wrong. It assumes that the >> stats contain sums of the individual colour pixels. But they actually >> contain sums of the colour channels of larger "superpixels" consisting >> of the individual colour pixels. Each of the RGB colour values and the >> computed luminosity (a histogram entry) are added once to the stats per >> such a superpixel. This means the offset computed from the black level >> and the number of pixels should be used as it is, not divided. >> The patch fixes the subtracted offset. Since the evaluation is the same >> for all the three colours now, the individual class variables are >> replaced with a single RGB variable. >> Fixes: 4e13c6f55bd667 ("Honor black level in AWB") >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> .../internal/software_isp/swisp_stats.h | 16 +++++----------- >> src/ipa/simple/algorithms/awb.cpp | 10 ++++------ >> src/libcamera/software_isp/swstats_cpu.cpp | 10 ++++------ >> 3 files changed, 13 insertions(+), 23 deletions(-) >> diff --git a/include/libcamera/internal/software_isp/swisp_stats.h b/include/libcamera/internal/software_isp/swisp_stats.h >> index 3c9860185..d9d0d9be8 100644 >> --- a/include/libcamera/internal/software_isp/swisp_stats.h >> +++ b/include/libcamera/internal/software_isp/swisp_stats.h >> @@ -10,6 +10,8 @@ >> #include <array> >> #include <stdint.h> >> +#include "libcamera/internal/vector.h" >> + >> namespace libcamera { >> /** >> @@ -26,17 +28,9 @@ struct SwIspStats { >> */ >> bool valid; >> /** >> - * \brief Holds the sum of all sampled red pixels >> - */ >> - uint64_t sumR_; >> - /** >> - * \brief Holds the sum of all sampled green pixels >> - */ >> - uint64_t sumG_; >> - /** >> - * \brief Holds the sum of all sampled blue pixels >> + * \brief Sums of colour channels of all the sampled pixels >> */ >> - uint64_t sumB_; >> + RGB<uint64_t> sum_; >> /** >> * \brief Number of bins in the yHistogram >> */ >> @@ -46,7 +40,7 @@ struct SwIspStats { >> */ >> using Histogram = std::array<uint32_t, kYHistogramSize>; >> /** >> - * \brief A histogram of luminance values >> + * \brief A histogram of luminance values of all the sampled pixels >> */ >> Histogram yHistogram; >> }; >> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp >> index 0080865aa..75e4ae4a4 100644 >> --- a/src/ipa/simple/algorithms/awb.cpp >> +++ b/src/ipa/simple/algorithms/awb.cpp >> @@ -1,6 +1,6 @@ >> /* SPDX-License-Identifier: LGPL-2.1-or-later */ >> /* >> - * Copyright (C) 2024, Red Hat Inc. >> + * Copyright (C) 2024-2026 Red Hat Inc. >> * >> * Auto white balance >> */ >> @@ -73,9 +73,7 @@ void Awb::process(IPAContext &context, >> histogram.begin(), histogram.end(), uint64_t(0)); >> const uint64_t offset = blackLevel * nPixels; >> const uint64_t minValid = 1; >> - const uint64_t sumR = stats->sumR_ > offset / 4 ? stats->sumR_ - offset / 4 : minValid; >> - const uint64_t sumG = stats->sumG_ > offset / 2 ? stats->sumG_ - offset / 2 : minValid; >> - const uint64_t sumB = stats->sumB_ > offset / 4 ? stats->sumB_ - offset / 4 : minValid; >> + const RGB<uint64_t> sum = (stats->sum_ - offset).max(minValid); > > What happens if the subtraction wraps around? Previously that was avoided > by the removed check. Or is wraparound actually impossible now? Oops, I've forgotten about that trap again, thank you for the reminder. I'll fix it some way in v3.
2026. 01. 27. 18:22 keltezéssel, Milan Zamazal írta: > Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > >> Hi >> >> 2026. 01. 27. 17:29 keltezéssel, Milan Zamazal írta: >>> The black level offset subtracted in AWB is wrong. It assumes that the >>> stats contain sums of the individual colour pixels. But they actually >>> contain sums of the colour channels of larger "superpixels" consisting >>> of the individual colour pixels. Each of the RGB colour values and the >>> computed luminosity (a histogram entry) are added once to the stats per >>> such a superpixel. This means the offset computed from the black level >>> and the number of pixels should be used as it is, not divided. >>> The patch fixes the subtracted offset. Since the evaluation is the same >>> for all the three colours now, the individual class variables are >>> replaced with a single RGB variable. >>> Fixes: 4e13c6f55bd667 ("Honor black level in AWB") >>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >>> --- >>> .../internal/software_isp/swisp_stats.h | 16 +++++----------- >>> src/ipa/simple/algorithms/awb.cpp | 10 ++++------ >>> src/libcamera/software_isp/swstats_cpu.cpp | 10 ++++------ >>> 3 files changed, 13 insertions(+), 23 deletions(-) >>> diff --git a/include/libcamera/internal/software_isp/swisp_stats.h b/include/libcamera/internal/software_isp/swisp_stats.h >>> index 3c9860185..d9d0d9be8 100644 >>> --- a/include/libcamera/internal/software_isp/swisp_stats.h >>> +++ b/include/libcamera/internal/software_isp/swisp_stats.h >>> @@ -10,6 +10,8 @@ >>> #include <array> >>> #include <stdint.h> >>> +#include "libcamera/internal/vector.h" >>> + >>> namespace libcamera { >>> /** >>> @@ -26,17 +28,9 @@ struct SwIspStats { >>> */ >>> bool valid; >>> /** >>> - * \brief Holds the sum of all sampled red pixels >>> - */ >>> - uint64_t sumR_; >>> - /** >>> - * \brief Holds the sum of all sampled green pixels >>> - */ >>> - uint64_t sumG_; >>> - /** >>> - * \brief Holds the sum of all sampled blue pixels >>> + * \brief Sums of colour channels of all the sampled pixels >>> */ >>> - uint64_t sumB_; >>> + RGB<uint64_t> sum_; >>> /** >>> * \brief Number of bins in the yHistogram >>> */ >>> @@ -46,7 +40,7 @@ struct SwIspStats { >>> */ >>> using Histogram = std::array<uint32_t, kYHistogramSize>; >>> /** >>> - * \brief A histogram of luminance values >>> + * \brief A histogram of luminance values of all the sampled pixels >>> */ >>> Histogram yHistogram; >>> }; >>> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp >>> index 0080865aa..75e4ae4a4 100644 >>> --- a/src/ipa/simple/algorithms/awb.cpp >>> +++ b/src/ipa/simple/algorithms/awb.cpp >>> @@ -1,6 +1,6 @@ >>> /* SPDX-License-Identifier: LGPL-2.1-or-later */ >>> /* >>> - * Copyright (C) 2024, Red Hat Inc. >>> + * Copyright (C) 2024-2026 Red Hat Inc. >>> * >>> * Auto white balance >>> */ >>> @@ -73,9 +73,7 @@ void Awb::process(IPAContext &context, >>> histogram.begin(), histogram.end(), uint64_t(0)); >>> const uint64_t offset = blackLevel * nPixels; >>> const uint64_t minValid = 1; >>> - const uint64_t sumR = stats->sumR_ > offset / 4 ? stats->sumR_ - offset / 4 : minValid; >>> - const uint64_t sumG = stats->sumG_ > offset / 2 ? stats->sumG_ - offset / 2 : minValid; >>> - const uint64_t sumB = stats->sumB_ > offset / 4 ? stats->sumB_ - offset / 4 : minValid; >>> + const RGB<uint64_t> sum = (stats->sum_ - offset).max(minValid); >> >> What happens if the subtraction wraps around? Previously that was avoided >> by the removed check. Or is wraparound actually impossible now? > > Oops, I've forgotten about that trap again, thank you for the reminder. > I'll fix it some way in v3. > Maybe const RGB<uint64_t> sum = stats->sum_.max(offset + minValid) - offset; ?
diff --git a/include/libcamera/internal/software_isp/swisp_stats.h b/include/libcamera/internal/software_isp/swisp_stats.h index 3c9860185..d9d0d9be8 100644 --- a/include/libcamera/internal/software_isp/swisp_stats.h +++ b/include/libcamera/internal/software_isp/swisp_stats.h @@ -10,6 +10,8 @@ #include <array> #include <stdint.h> +#include "libcamera/internal/vector.h" + namespace libcamera { /** @@ -26,17 +28,9 @@ struct SwIspStats { */ bool valid; /** - * \brief Holds the sum of all sampled red pixels - */ - uint64_t sumR_; - /** - * \brief Holds the sum of all sampled green pixels - */ - uint64_t sumG_; - /** - * \brief Holds the sum of all sampled blue pixels + * \brief Sums of colour channels of all the sampled pixels */ - uint64_t sumB_; + RGB<uint64_t> sum_; /** * \brief Number of bins in the yHistogram */ @@ -46,7 +40,7 @@ struct SwIspStats { */ using Histogram = std::array<uint32_t, kYHistogramSize>; /** - * \brief A histogram of luminance values + * \brief A histogram of luminance values of all the sampled pixels */ Histogram yHistogram; }; diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp index 0080865aa..75e4ae4a4 100644 --- a/src/ipa/simple/algorithms/awb.cpp +++ b/src/ipa/simple/algorithms/awb.cpp @@ -1,6 +1,6 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ /* - * Copyright (C) 2024, Red Hat Inc. + * Copyright (C) 2024-2026 Red Hat Inc. * * Auto white balance */ @@ -73,9 +73,7 @@ void Awb::process(IPAContext &context, histogram.begin(), histogram.end(), uint64_t(0)); const uint64_t offset = blackLevel * nPixels; const uint64_t minValid = 1; - const uint64_t sumR = stats->sumR_ > offset / 4 ? stats->sumR_ - offset / 4 : minValid; - const uint64_t sumG = stats->sumG_ > offset / 2 ? stats->sumG_ - offset / 2 : minValid; - const uint64_t sumB = stats->sumB_ > offset / 4 ? stats->sumB_ - offset / 4 : minValid; + const RGB<uint64_t> sum = (stats->sum_ - offset).max(minValid); /* * Calculate red and blue gains for AWB. @@ -83,9 +81,9 @@ void Awb::process(IPAContext &context, */ auto &gains = context.activeState.awb.gains; gains = { { - sumR <= sumG / 4 ? 4.0f : static_cast<float>(sumG) / sumR, + sum.r() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.r(), 1.0, - sumB <= sumG / 4 ? 4.0f : static_cast<float>(sumG) / sumB, + sum.b() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.b(), } }; RGB<double> rgbGains{ { 1 / gains.r(), 1 / gains.g(), 1 / gains.b() } }; diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp index c931edb41..5c3011a7f 100644 --- a/src/libcamera/software_isp/swstats_cpu.cpp +++ b/src/libcamera/software_isp/swstats_cpu.cpp @@ -185,9 +185,9 @@ static constexpr unsigned int kBlueYMul = 29; /* 0.114 * 256 */ stats_.yHistogram[yVal * SwIspStats::kYHistogramSize / (256 * 256 * (div))]++; #define SWSTATS_FINISH_LINE_STATS() \ - stats_.sumR_ += sumR; \ - stats_.sumG_ += sumG; \ - stats_.sumB_ += sumB; + stats_.sum_.r() += sumR; \ + stats_.sum_.g() += sumG; \ + stats_.sum_.b() += sumB; void SwStatsCpu::statsBGGR8Line0(const uint8_t *src[]) { @@ -332,9 +332,7 @@ void SwStatsCpu::startFrame(uint32_t frame) if (window_.width == 0) LOG(SwStatsCpu, Error) << "Calling startFrame() without setWindow()"; - stats_.sumR_ = 0; - stats_.sumB_ = 0; - stats_.sumG_ = 0; + stats_.sum_ = RGB<uint64_t>({ 0, 0, 0 }); stats_.yHistogram.fill(0); }
The black level offset subtracted in AWB is wrong. It assumes that the stats contain sums of the individual colour pixels. But they actually contain sums of the colour channels of larger "superpixels" consisting of the individual colour pixels. Each of the RGB colour values and the computed luminosity (a histogram entry) are added once to the stats per such a superpixel. This means the offset computed from the black level and the number of pixels should be used as it is, not divided. The patch fixes the subtracted offset. Since the evaluation is the same for all the three colours now, the individual class variables are replaced with a single RGB variable. Fixes: 4e13c6f55bd667 ("Honor black level in AWB") Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- .../internal/software_isp/swisp_stats.h | 16 +++++----------- src/ipa/simple/algorithms/awb.cpp | 10 ++++------ src/libcamera/software_isp/swstats_cpu.cpp | 10 ++++------ 3 files changed, 13 insertions(+), 23 deletions(-)