| Message ID | 20261018102703.33012-1-tobias.predel@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Tobias Predel (2026-10-18 11:27:03) > The software ISP AWB computes grey world means from the colour sums of > all sampled pixels. When part of the image is overexposed, the > strongest raw channel, usually green, clips first, so its mean is > underestimated. The resulting red and blue gains are too low and give > the whole image a green cast. This is easy to trigger with a bright > window or a lamp in view. > > Leave pixels with any colour component at or above 250/255 of full > scale out of the colour sums, and count the pixels that are included, > so that AWB divides by the right number. The luminance histogram still > covers all pixels, as AGC needs to see the saturated ones. > > Tested on a Hynix Hi556 with the simple pipeline handler and a ceiling > lamp in view (about 5% of the output clipped): the G/R and G/B ratios > of the unclipped pixels go from 1.11 and 1.16 to 1.00 and 1.00. With > no clipping in the scene, the AWB gains are unchanged. > > Signed-off-by: Tobias Predel <tobias.predel@gmail.com> > Assisted-by: Claude:claude-opus-5-5 > --- > > Notes: > The change in swstats_cpu.cpp applies to both the CPU and the GPU debayer > paths, as both use SwStatsCpu. I have also backported it to v0.7.2 > (src/ipa/simple) and have been running it as a distribution package. > > This patch was written with the help of an AI coding assistant (hence the > Assisted-by trailer, following the Linux kernel's coding-assistants > guidelines; checkstyle doesn't know that trailer key yet). I reviewed the > code and ran the tests on the hardware myself. > > .../internal/software_isp/swisp_stats.h | 7 +++- > src/ipa/softisp/algorithms/awb.cpp | 9 +++-- > src/libcamera/software_isp/swstats_cpu.cpp | 39 +++++++++++++------ > 3 files changed, 39 insertions(+), 16 deletions(-) > > diff --git a/include/libcamera/internal/software_isp/swisp_stats.h b/include/libcamera/internal/software_isp/swisp_stats.h > index d9d0d9be8..f15c8aae5 100644 > --- a/include/libcamera/internal/software_isp/swisp_stats.h > +++ b/include/libcamera/internal/software_isp/swisp_stats.h > @@ -28,9 +28,14 @@ struct SwIspStats { > */ > bool valid; > /** > - * \brief Sums of colour channels of all the sampled pixels > + * \brief Sums of colour channels of the sampled pixels that are not > + * (near) saturated > */ > RGB<uint64_t> sum_; > + /** > + * \brief Number of sampled pixels included in sum_ > + */ > + uint64_t sumCount_; > /** > * \brief Number of bins in the yHistogram > */ > diff --git a/src/ipa/softisp/algorithms/awb.cpp b/src/ipa/softisp/algorithms/awb.cpp > index 55fd326fc..e6b750d87 100644 > --- a/src/ipa/softisp/algorithms/awb.cpp > +++ b/src/ipa/softisp/algorithms/awb.cpp > @@ -7,7 +7,6 @@ > > #include "awb.h" > > -#include <numeric> > #include <stdint.h> > > #include <libcamera/base/log.h> > @@ -118,16 +117,18 @@ SoftIspAwbStats Awb::calculateRgbMeans(IPAContext &context, > if (!stats->valid) > return {}; > > - const SwIspStats::Histogram &histogram = stats->yHistogram; > const uint8_t blackLevel = context.activeState.blc.level; > > + /* Saturated pixels are not included in the sums. */ > + const uint64_t nPixels = stats->sumCount_; > + if (!nPixels) > + return {}; > + > /* > * Black level must be subtracted to get the correct AWB ratios, they > * would be off if they were computed from the whole brightness range > * rather than from the sensor range. > */ > - const uint64_t nPixels = std::accumulate( > - histogram.begin(), histogram.end(), uint64_t(0)); > const uint64_t offset = blackLevel * nPixels; > const uint64_t minValid = 1; > > diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp > index 7fb77ce7d..c4391a261 100644 > --- a/src/libcamera/software_isp/swstats_cpu.cpp > +++ b/src/libcamera/software_isp/swstats_cpu.cpp > @@ -11,6 +11,8 @@ > > #include "libcamera/internal/software_isp/swstats_cpu.h" > > +#include <algorithm> > + > #include <libcamera/base/log.h> > > #include <libcamera/stream.h> > @@ -171,28 +173,40 @@ static constexpr unsigned int kRedYMul = 77; /* 0.299 * 256 */ > static constexpr unsigned int kGreenYMul = 150; /* 0.587 * 256 */ > static constexpr unsigned int kBlueYMul = 29; /* 0.114 * 256 */ > > +/* > + * Pixels with any colour component at or above this 8-bit level are left out What about 10 bit or 12 bit sensors/modes ? Or is that handled by the div? > + * of the colour sums. Clipped pixels have lost their colour information and > + * would skew the AWB ratios, typically towards the channel that clips last. > + */ > +static constexpr unsigned int kSaturationLevel = 250; > + > #define SWSTATS_START_LINE_STATS(pixel_t) \ > pixel_t r, g, g2, b; \ > uint64_t yVal; \ > \ > uint64_t sumR = 0; \ > uint64_t sumG = 0; \ > - uint64_t sumB = 0; > - > -#define SWSTATS_ACCUMULATE_LINE_STATS(div) \ > - sumR += r; \ > - sumG += g; \ > - sumB += b; \ > - \ > - yVal = r * kRedYMul; \ > - yVal += g * kGreenYMul; \ > - yVal += b * kBlueYMul; \ > + uint64_t sumB = 0; \ > + uint64_t sumCount = 0; > + > +#define SWSTATS_ACCUMULATE_LINE_STATS(div) \ > + if (std::max({ r, g, b }) < kSaturationLevel * (div)) { \ That's a lot of operations per pixel. Is the multiplication optimised out ? > + sumR += r; \ > + sumG += g; \ > + sumB += b; \ > + sumCount++; \ We're in a 'hot path' here where every instruction counts. What's the measured performance penalty here ? > + } \ > + \ > + yVal = r * kRedYMul; \ > + yVal += g * kGreenYMul; \ > + yVal += b * kBlueYMul; \ > stats.yHistogram[yVal * SwIspStats::kYHistogramSize / (256 * 256 * (div))]++; > > #define SWSTATS_FINISH_LINE_STATS() \ > stats.sum_.r() += sumR; \ > stats.sum_.g() += sumG; \ > - stats.sum_.b() += sumB; > + stats.sum_.b() += sumB; \ > + stats.sumCount_ += sumCount; > > void SwStatsCpu::statsBGGR8Line0(const uint8_t *src[], SwIspStats &stats) > { > @@ -391,6 +405,7 @@ void SwStatsCpu::startFrame(uint32_t frame) > > for (auto &s : stats_) { > s.sum_ = RGB<uint64_t>({ 0, 0, 0 }); > + s.sumCount_ = 0; > s.yHistogram.fill(0); > } > } > @@ -408,9 +423,11 @@ void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId) > > if (valid) { > sharedStats_->sum_ = RGB<uint64_t>({ 0, 0, 0 }); > + sharedStats_->sumCount_ = 0; > sharedStats_->yHistogram.fill(0); > for (const auto &s : stats_) { > sharedStats_->sum_ += s.sum_; > + sharedStats_->sumCount_ += s.sumCount_; > for (unsigned int j = 0; j < SwIspStats::kYHistogramSize; j++) > sharedStats_->yHistogram[j] += s.yHistogram[j]; > } > -- > 2.56.0 >
Hi Kieran, Thanks for the review. Quoting Kieran Bingham (2026-10-18 15:40:17) > > +/* > > + * Pixels with any colour component at or above this 8-bit level are left out > > What about 10 bit or 12 bit sensors/modes ? Or is that handled by the > div? It is handled by the div, the same factor that already scales the Y histogram index to 8 bits: the unpacked 10-bit path compares against 250 * 4 = 1000 and the unpacked 12-bit path against 250 * 16 = 4000. The packed 10P/12P paths only read the MSB byte, so they use div 1. All formats thus cut at the same 250/255 of full scale. I can reword the comment to say "at or above this level, scaled to the input bit depth by div" if that is clearer. > > +#define SWSTATS_ACCUMULATE_LINE_STATS(div) \ > > + if (std::max({ r, g, b }) < kSaturationLevel * (div)) { \ > > That's a lot of operations per pixel. > Is the multiplication optimised out ? Yes, div is a literal at every call site, so the product is a constant. In a release build with gcc 16, the 10-bit path compiles to two cmov for the max and a single "cmp $0x3e7" (999). > We're in a 'hot path' here where every instruction counts. What's the > measured performance penalty here ? Measured on an HP Dragonfly G4 (i7-1355U), Hi556 at 2592x1944 10-bit unpacked, CPU soft ISP, master vs master + this patch, alternating runs. To isolate the stats I timed the stats0_ calls with clock_gettime() (same local instrumentation in both builds), 200 stats frames per run, 3 runs each: scene master patched delta no clipping 502 us 580 us +78 us (+16%) lamp in view, ~6% clipped 541 us 623 us +82 us (+15%) (per frame on which stats are computed) As stats only run on one frame in four, that is about +20 us per output frame. The debayer benchmark for the same configuration reports about 19-20 ms/frame with the default 2 threads, so the overall cost is about 0.1%. End to end with 1 thread and 900 measured frames (4 runs each), the Debayer benchmark gives 23.00 ms/frame for master and 23.04 ms/frame for the patched build, which is within the run-to-run noise of about +-0.8 ms. One caveat: the check is a branch, so a scene with clipped pixels scattered randomly across the image (rather than in a contiguous highlight) would mispredict more. In a synthetic test of the line loop alone that roughly doubled the loop time; a branchless masked variant was slower in the common case, so I kept the branch. Real highlights are spatially coherent, as the lamp numbers above show. Best regards, Tobias
Hi, On 18-Oct-26 12:27 PM, Tobias Predel wrote: > The software ISP AWB computes grey world means from the colour sums of > all sampled pixels. When part of the image is overexposed, the > strongest raw channel, usually green, clips first, so its mean is > underestimated. The resulting red and blue gains are too low and give > the whole image a green cast. This is easy to trigger with a bright > window or a lamp in view. > > Leave pixels with any colour component at or above 250/255 of full > scale out of the colour sums, and count the pixels that are included, > so that AWB divides by the right number. The luminance histogram still > covers all pixels, as AGC needs to see the saturated ones. > > Tested on a Hynix Hi556 with the simple pipeline handler and a ceiling > lamp in view (about 5% of the output clipped): the G/R and G/B ratios > of the unclipped pixels go from 1.11 and 1.16 to 1.00 and 1.00. With > no clipping in the scene, the AWB gains are unchanged. > > Signed-off-by: Tobias Predel <tobias.predel@gmail.com> > Assisted-by: Claude:claude-opus-5-5 Nice patch this, is a really useful image-quality improvement: Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com> Regards, Hans > --- > > Notes: > The change in swstats_cpu.cpp applies to both the CPU and the GPU debayer > paths, as both use SwStatsCpu. I have also backported it to v0.7.2 > (src/ipa/simple) and have been running it as a distribution package. > > This patch was written with the help of an AI coding assistant (hence the > Assisted-by trailer, following the Linux kernel's coding-assistants > guidelines; checkstyle doesn't know that trailer key yet). I reviewed the > code and ran the tests on the hardware myself. > > .../internal/software_isp/swisp_stats.h | 7 +++- > src/ipa/softisp/algorithms/awb.cpp | 9 +++-- > src/libcamera/software_isp/swstats_cpu.cpp | 39 +++++++++++++------ > 3 files changed, 39 insertions(+), 16 deletions(-) > > diff --git a/include/libcamera/internal/software_isp/swisp_stats.h b/include/libcamera/internal/software_isp/swisp_stats.h > index d9d0d9be8..f15c8aae5 100644 > --- a/include/libcamera/internal/software_isp/swisp_stats.h > +++ b/include/libcamera/internal/software_isp/swisp_stats.h > @@ -28,9 +28,14 @@ struct SwIspStats { > */ > bool valid; > /** > - * \brief Sums of colour channels of all the sampled pixels > + * \brief Sums of colour channels of the sampled pixels that are not > + * (near) saturated > */ > RGB<uint64_t> sum_; > + /** > + * \brief Number of sampled pixels included in sum_ > + */ > + uint64_t sumCount_; > /** > * \brief Number of bins in the yHistogram > */ > diff --git a/src/ipa/softisp/algorithms/awb.cpp b/src/ipa/softisp/algorithms/awb.cpp > index 55fd326fc..e6b750d87 100644 > --- a/src/ipa/softisp/algorithms/awb.cpp > +++ b/src/ipa/softisp/algorithms/awb.cpp > @@ -7,7 +7,6 @@ > > #include "awb.h" > > -#include <numeric> > #include <stdint.h> > > #include <libcamera/base/log.h> > @@ -118,16 +117,18 @@ SoftIspAwbStats Awb::calculateRgbMeans(IPAContext &context, > if (!stats->valid) > return {}; > > - const SwIspStats::Histogram &histogram = stats->yHistogram; > const uint8_t blackLevel = context.activeState.blc.level; > > + /* Saturated pixels are not included in the sums. */ > + const uint64_t nPixels = stats->sumCount_; > + if (!nPixels) > + return {}; > + > /* > * Black level must be subtracted to get the correct AWB ratios, they > * would be off if they were computed from the whole brightness range > * rather than from the sensor range. > */ > - const uint64_t nPixels = std::accumulate( > - histogram.begin(), histogram.end(), uint64_t(0)); > const uint64_t offset = blackLevel * nPixels; > const uint64_t minValid = 1; > > diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp > index 7fb77ce7d..c4391a261 100644 > --- a/src/libcamera/software_isp/swstats_cpu.cpp > +++ b/src/libcamera/software_isp/swstats_cpu.cpp > @@ -11,6 +11,8 @@ > > #include "libcamera/internal/software_isp/swstats_cpu.h" > > +#include <algorithm> > + > #include <libcamera/base/log.h> > > #include <libcamera/stream.h> > @@ -171,28 +173,40 @@ static constexpr unsigned int kRedYMul = 77; /* 0.299 * 256 */ > static constexpr unsigned int kGreenYMul = 150; /* 0.587 * 256 */ > static constexpr unsigned int kBlueYMul = 29; /* 0.114 * 256 */ > > +/* > + * Pixels with any colour component at or above this 8-bit level are left out > + * of the colour sums. Clipped pixels have lost their colour information and > + * would skew the AWB ratios, typically towards the channel that clips last. > + */ > +static constexpr unsigned int kSaturationLevel = 250; > + > #define SWSTATS_START_LINE_STATS(pixel_t) \ > pixel_t r, g, g2, b; \ > uint64_t yVal; \ > \ > uint64_t sumR = 0; \ > uint64_t sumG = 0; \ > - uint64_t sumB = 0; > - > -#define SWSTATS_ACCUMULATE_LINE_STATS(div) \ > - sumR += r; \ > - sumG += g; \ > - sumB += b; \ > - \ > - yVal = r * kRedYMul; \ > - yVal += g * kGreenYMul; \ > - yVal += b * kBlueYMul; \ > + uint64_t sumB = 0; \ > + uint64_t sumCount = 0; > + > +#define SWSTATS_ACCUMULATE_LINE_STATS(div) \ > + if (std::max({ r, g, b }) < kSaturationLevel * (div)) { \ > + sumR += r; \ > + sumG += g; \ > + sumB += b; \ > + sumCount++; \ > + } \ > + \ > + yVal = r * kRedYMul; \ > + yVal += g * kGreenYMul; \ > + yVal += b * kBlueYMul; \ > stats.yHistogram[yVal * SwIspStats::kYHistogramSize / (256 * 256 * (div))]++; > > #define SWSTATS_FINISH_LINE_STATS() \ > stats.sum_.r() += sumR; \ > stats.sum_.g() += sumG; \ > - stats.sum_.b() += sumB; > + stats.sum_.b() += sumB; \ > + stats.sumCount_ += sumCount; > > void SwStatsCpu::statsBGGR8Line0(const uint8_t *src[], SwIspStats &stats) > { > @@ -391,6 +405,7 @@ void SwStatsCpu::startFrame(uint32_t frame) > > for (auto &s : stats_) { > s.sum_ = RGB<uint64_t>({ 0, 0, 0 }); > + s.sumCount_ = 0; > s.yHistogram.fill(0); > } > } > @@ -408,9 +423,11 @@ void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId) > > if (valid) { > sharedStats_->sum_ = RGB<uint64_t>({ 0, 0, 0 }); > + sharedStats_->sumCount_ = 0; > sharedStats_->yHistogram.fill(0); > for (const auto &s : stats_) { > sharedStats_->sum_ += s.sum_; > + sharedStats_->sumCount_ += s.sumCount_; > for (unsigned int j = 0; j < SwIspStats::kYHistogramSize; j++) > sharedStats_->yHistogram[j] += s.yHistogram[j]; > }
On Mon, Oct 19, 2026 at 01:15:18PM +0200, johannes.goede@oss.qualcomm.com wrote: > On 18-Oct-26 12:27 PM, Tobias Predel wrote: > > The software ISP AWB computes grey world means from the colour sums of > > all sampled pixels. When part of the image is overexposed, the > > strongest raw channel, usually green, clips first, so its mean is > > underestimated. The resulting red and blue gains are too low and give > > the whole image a green cast. This is easy to trigger with a bright > > window or a lamp in view. > > > > Leave pixels with any colour component at or above 250/255 of full > > scale out of the colour sums, and count the pixels that are included, > > so that AWB divides by the right number. The luminance histogram still > > covers all pixels, as AGC needs to see the saturated ones. > > > > Tested on a Hynix Hi556 with the simple pipeline handler and a ceiling > > lamp in view (about 5% of the output clipped): the G/R and G/B ratios > > of the unclipped pixels go from 1.11 and 1.16 to 1.00 and 1.00. With > > no clipping in the scene, the AWB gains are unchanged. > > > > Signed-off-by: Tobias Predel <tobias.predel@gmail.com> > > Assisted-by: Claude:claude-opus-5-5 > > Nice patch this, is a really useful image-quality improvement: What the performance impact ? > Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com> > > > --- > > > > Notes: > > The change in swstats_cpu.cpp applies to both the CPU and the GPU debayer > > paths, as both use SwStatsCpu. I have also backported it to v0.7.2 > > (src/ipa/simple) and have been running it as a distribution package. > > > > This patch was written with the help of an AI coding assistant (hence the > > Assisted-by trailer, following the Linux kernel's coding-assistants > > guidelines; checkstyle doesn't know that trailer key yet). I reviewed the > > code and ran the tests on the hardware myself. I'm close to posting an AI policy document for libcamera that will not allow LLM-generated contributions. > > .../internal/software_isp/swisp_stats.h | 7 +++- > > src/ipa/softisp/algorithms/awb.cpp | 9 +++-- > > src/libcamera/software_isp/swstats_cpu.cpp | 39 +++++++++++++------ > > 3 files changed, 39 insertions(+), 16 deletions(-) > > > > diff --git a/include/libcamera/internal/software_isp/swisp_stats.h b/include/libcamera/internal/software_isp/swisp_stats.h > > index d9d0d9be8..f15c8aae5 100644 > > --- a/include/libcamera/internal/software_isp/swisp_stats.h > > +++ b/include/libcamera/internal/software_isp/swisp_stats.h > > @@ -28,9 +28,14 @@ struct SwIspStats { > > */ > > bool valid; > > /** > > - * \brief Sums of colour channels of all the sampled pixels > > + * \brief Sums of colour channels of the sampled pixels that are not > > + * (near) saturated > > */ > > RGB<uint64_t> sum_; > > + /** > > + * \brief Number of sampled pixels included in sum_ > > + */ > > + uint64_t sumCount_; Not a good name. > > /** > > * \brief Number of bins in the yHistogram > > */ > > diff --git a/src/ipa/softisp/algorithms/awb.cpp b/src/ipa/softisp/algorithms/awb.cpp > > index 55fd326fc..e6b750d87 100644 > > --- a/src/ipa/softisp/algorithms/awb.cpp > > +++ b/src/ipa/softisp/algorithms/awb.cpp > > @@ -7,7 +7,6 @@ > > > > #include "awb.h" > > > > -#include <numeric> > > #include <stdint.h> > > > > #include <libcamera/base/log.h> > > @@ -118,16 +117,18 @@ SoftIspAwbStats Awb::calculateRgbMeans(IPAContext &context, > > if (!stats->valid) > > return {}; > > > > - const SwIspStats::Histogram &histogram = stats->yHistogram; > > const uint8_t blackLevel = context.activeState.blc.level; > > > > + /* Saturated pixels are not included in the sums. */ Unrelated to the code change below > > + const uint64_t nPixels = stats->sumCount_; > > + if (!nPixels) > > + return {}; > > + > > /* > > * Black level must be subtracted to get the correct AWB ratios, they > > * would be off if they were computed from the whole brightness range > > * rather than from the sensor range. > > */ > > - const uint64_t nPixels = std::accumulate( > > - histogram.begin(), histogram.end(), uint64_t(0)); > > const uint64_t offset = blackLevel * nPixels; > > const uint64_t minValid = 1; > > > > diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp > > index 7fb77ce7d..c4391a261 100644 > > --- a/src/libcamera/software_isp/swstats_cpu.cpp > > +++ b/src/libcamera/software_isp/swstats_cpu.cpp > > @@ -11,6 +11,8 @@ > > > > #include "libcamera/internal/software_isp/swstats_cpu.h" > > > > +#include <algorithm> > > + > > #include <libcamera/base/log.h> > > > > #include <libcamera/stream.h> > > @@ -171,28 +173,40 @@ static constexpr unsigned int kRedYMul = 77; /* 0.299 * 256 */ > > static constexpr unsigned int kGreenYMul = 150; /* 0.587 * 256 */ > > static constexpr unsigned int kBlueYMul = 29; /* 0.114 * 256 */ > > > > +/* > > + * Pixels with any colour component at or above this 8-bit level are left out > > + * of the colour sums. Clipped pixels have lost their colour information and > > + * would skew the AWB ratios, typically towards the channel that clips last. What do you mean "typically" ? How can it be otherwise ? > > + */ > > +static constexpr unsigned int kSaturationLevel = 250; Why 250 ? > > + > > #define SWSTATS_START_LINE_STATS(pixel_t) \ > > pixel_t r, g, g2, b; \ > > uint64_t yVal; \ > > \ > > uint64_t sumR = 0; \ > > uint64_t sumG = 0; \ > > - uint64_t sumB = 0; > > - > > -#define SWSTATS_ACCUMULATE_LINE_STATS(div) \ > > - sumR += r; \ > > - sumG += g; \ > > - sumB += b; \ > > - \ > > - yVal = r * kRedYMul; \ > > - yVal += g * kGreenYMul; \ > > - yVal += b * kBlueYMul; \ > > + uint64_t sumB = 0; \ > > + uint64_t sumCount = 0; > > + > > +#define SWSTATS_ACCUMULATE_LINE_STATS(div) \ > > + if (std::max({ r, g, b }) < kSaturationLevel * (div)) { \ > > + sumR += r; \ > > + sumG += g; \ > > + sumB += b; \ > > + sumCount++; \ > > + } \ > > + \ > > + yVal = r * kRedYMul; \ > > + yVal += g * kGreenYMul; \ > > + yVal += b * kBlueYMul; \ > > stats.yHistogram[yVal * SwIspStats::kYHistogramSize / (256 * 256 * (div))]++; > > > > #define SWSTATS_FINISH_LINE_STATS() \ > > stats.sum_.r() += sumR; \ > > stats.sum_.g() += sumG; \ > > - stats.sum_.b() += sumB; > > + stats.sum_.b() += sumB; \ > > + stats.sumCount_ += sumCount; > > > > void SwStatsCpu::statsBGGR8Line0(const uint8_t *src[], SwIspStats &stats) > > { > > @@ -391,6 +405,7 @@ void SwStatsCpu::startFrame(uint32_t frame) > > > > for (auto &s : stats_) { > > s.sum_ = RGB<uint64_t>({ 0, 0, 0 }); > > + s.sumCount_ = 0; > > s.yHistogram.fill(0); > > } > > } > > @@ -408,9 +423,11 @@ void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId) > > > > if (valid) { > > sharedStats_->sum_ = RGB<uint64_t>({ 0, 0, 0 }); > > + sharedStats_->sumCount_ = 0; > > sharedStats_->yHistogram.fill(0); > > for (const auto &s : stats_) { > > sharedStats_->sum_ += s.sum_; > > + sharedStats_->sumCount_ += s.sumCount_; > > for (unsigned int j = 0; j < SwIspStats::kYHistogramSize; j++) > > sharedStats_->yHistogram[j] += s.yHistogram[j]; > > } >
Hi, On 19-Oct-26 1:26 PM, Laurent Pinchart wrote: > On Mon, Oct 19, 2026 at 01:15:18PM +0200, johannes.goede@oss.qualcomm.com wrote: >> On 18-Oct-26 12:27 PM, Tobias Predel wrote: >>> The software ISP AWB computes grey world means from the colour sums of >>> all sampled pixels. When part of the image is overexposed, the >>> strongest raw channel, usually green, clips first, so its mean is >>> underestimated. The resulting red and blue gains are too low and give >>> the whole image a green cast. This is easy to trigger with a bright >>> window or a lamp in view. >>> >>> Leave pixels with any colour component at or above 250/255 of full >>> scale out of the colour sums, and count the pixels that are included, >>> so that AWB divides by the right number. The luminance histogram still >>> covers all pixels, as AGC needs to see the saturated ones. >>> >>> Tested on a Hynix Hi556 with the simple pipeline handler and a ceiling >>> lamp in view (about 5% of the output clipped): the G/R and G/B ratios >>> of the unclipped pixels go from 1.11 and 1.16 to 1.00 and 1.00. With >>> no clipping in the scene, the AWB gains are unchanged. >>> >>> Signed-off-by: Tobias Predel <tobias.predel@gmail.com> >>> Assisted-by: Claude:claude-opus-5-5 >> >> Nice patch this, is a really useful image-quality improvement: > > What the performance impact ? This was discussed in detail further down in the thread and it is not too bad. > >> Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com> >> >>> --- >>> >>> Notes: >>> The change in swstats_cpu.cpp applies to both the CPU and the GPU debayer >>> paths, as both use SwStatsCpu. I have also backported it to v0.7.2 >>> (src/ipa/simple) and have been running it as a distribution package. >>> >>> This patch was written with the help of an AI coding assistant (hence the >>> Assisted-by trailer, following the Linux kernel's coding-assistants >>> guidelines; checkstyle doesn't know that trailer key yet). I reviewed the >>> code and ran the tests on the hardware myself. > > I'm close to posting an AI policy document for libcamera that will not > allow LLM-generated contributions. This patch does not feel like a straight forward copy and paste of LLM output though. It may need a bit more spit and polish based on your remarks about e.g. naming below, but overall it looks good to me and it seems like a valuable improvement of the softISP. I hope that the policy is going to leave room for actual valuable contributions even if LLM was used at some point during the process of writing the contribution. (vs outright LLM slop) Regards, Hans > >>> .../internal/software_isp/swisp_stats.h | 7 +++- >>> src/ipa/softisp/algorithms/awb.cpp | 9 +++-- >>> src/libcamera/software_isp/swstats_cpu.cpp | 39 +++++++++++++------ >>> 3 files changed, 39 insertions(+), 16 deletions(-) >>> >>> diff --git a/include/libcamera/internal/software_isp/swisp_stats.h b/include/libcamera/internal/software_isp/swisp_stats.h >>> index d9d0d9be8..f15c8aae5 100644 >>> --- a/include/libcamera/internal/software_isp/swisp_stats.h >>> +++ b/include/libcamera/internal/software_isp/swisp_stats.h >>> @@ -28,9 +28,14 @@ struct SwIspStats { >>> */ >>> bool valid; >>> /** >>> - * \brief Sums of colour channels of all the sampled pixels >>> + * \brief Sums of colour channels of the sampled pixels that are not >>> + * (near) saturated >>> */ >>> RGB<uint64_t> sum_; >>> + /** >>> + * \brief Number of sampled pixels included in sum_ >>> + */ >>> + uint64_t sumCount_; > > Not a good name. > >>> /** >>> * \brief Number of bins in the yHistogram >>> */ >>> diff --git a/src/ipa/softisp/algorithms/awb.cpp b/src/ipa/softisp/algorithms/awb.cpp >>> index 55fd326fc..e6b750d87 100644 >>> --- a/src/ipa/softisp/algorithms/awb.cpp >>> +++ b/src/ipa/softisp/algorithms/awb.cpp >>> @@ -7,7 +7,6 @@ >>> >>> #include "awb.h" >>> >>> -#include <numeric> >>> #include <stdint.h> >>> >>> #include <libcamera/base/log.h> >>> @@ -118,16 +117,18 @@ SoftIspAwbStats Awb::calculateRgbMeans(IPAContext &context, >>> if (!stats->valid) >>> return {}; >>> >>> - const SwIspStats::Histogram &histogram = stats->yHistogram; >>> const uint8_t blackLevel = context.activeState.blc.level; >>> >>> + /* Saturated pixels are not included in the sums. */ > > Unrelated to the code change below > >>> + const uint64_t nPixels = stats->sumCount_; >>> + if (!nPixels) >>> + return {}; >>> + >>> /* >>> * Black level must be subtracted to get the correct AWB ratios, they >>> * would be off if they were computed from the whole brightness range >>> * rather than from the sensor range. >>> */ >>> - const uint64_t nPixels = std::accumulate( >>> - histogram.begin(), histogram.end(), uint64_t(0)); >>> const uint64_t offset = blackLevel * nPixels; >>> const uint64_t minValid = 1; >>> >>> diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp >>> index 7fb77ce7d..c4391a261 100644 >>> --- a/src/libcamera/software_isp/swstats_cpu.cpp >>> +++ b/src/libcamera/software_isp/swstats_cpu.cpp >>> @@ -11,6 +11,8 @@ >>> >>> #include "libcamera/internal/software_isp/swstats_cpu.h" >>> >>> +#include <algorithm> >>> + >>> #include <libcamera/base/log.h> >>> >>> #include <libcamera/stream.h> >>> @@ -171,28 +173,40 @@ static constexpr unsigned int kRedYMul = 77; /* 0.299 * 256 */ >>> static constexpr unsigned int kGreenYMul = 150; /* 0.587 * 256 */ >>> static constexpr unsigned int kBlueYMul = 29; /* 0.114 * 256 */ >>> >>> +/* >>> + * Pixels with any colour component at or above this 8-bit level are left out >>> + * of the colour sums. Clipped pixels have lost their colour information and >>> + * would skew the AWB ratios, typically towards the channel that clips last. > > What do you mean "typically" ? How can it be otherwise ? > >>> + */ >>> +static constexpr unsigned int kSaturationLevel = 250; > > Why 250 ? > >>> + >>> #define SWSTATS_START_LINE_STATS(pixel_t) \ >>> pixel_t r, g, g2, b; \ >>> uint64_t yVal; \ >>> \ >>> uint64_t sumR = 0; \ >>> uint64_t sumG = 0; \ >>> - uint64_t sumB = 0; >>> - >>> -#define SWSTATS_ACCUMULATE_LINE_STATS(div) \ >>> - sumR += r; \ >>> - sumG += g; \ >>> - sumB += b; \ >>> - \ >>> - yVal = r * kRedYMul; \ >>> - yVal += g * kGreenYMul; \ >>> - yVal += b * kBlueYMul; \ >>> + uint64_t sumB = 0; \ >>> + uint64_t sumCount = 0; >>> + >>> +#define SWSTATS_ACCUMULATE_LINE_STATS(div) \ >>> + if (std::max({ r, g, b }) < kSaturationLevel * (div)) { \ >>> + sumR += r; \ >>> + sumG += g; \ >>> + sumB += b; \ >>> + sumCount++; \ >>> + } \ >>> + \ >>> + yVal = r * kRedYMul; \ >>> + yVal += g * kGreenYMul; \ >>> + yVal += b * kBlueYMul; \ >>> stats.yHistogram[yVal * SwIspStats::kYHistogramSize / (256 * 256 * (div))]++; >>> >>> #define SWSTATS_FINISH_LINE_STATS() \ >>> stats.sum_.r() += sumR; \ >>> stats.sum_.g() += sumG; \ >>> - stats.sum_.b() += sumB; >>> + stats.sum_.b() += sumB; \ >>> + stats.sumCount_ += sumCount; >>> >>> void SwStatsCpu::statsBGGR8Line0(const uint8_t *src[], SwIspStats &stats) >>> { >>> @@ -391,6 +405,7 @@ void SwStatsCpu::startFrame(uint32_t frame) >>> >>> for (auto &s : stats_) { >>> s.sum_ = RGB<uint64_t>({ 0, 0, 0 }); >>> + s.sumCount_ = 0; >>> s.yHistogram.fill(0); >>> } >>> } >>> @@ -408,9 +423,11 @@ void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId) >>> >>> if (valid) { >>> sharedStats_->sum_ = RGB<uint64_t>({ 0, 0, 0 }); >>> + sharedStats_->sumCount_ = 0; >>> sharedStats_->yHistogram.fill(0); >>> for (const auto &s : stats_) { >>> sharedStats_->sum_ += s.sum_; >>> + sharedStats_->sumCount_ += s.sumCount_; >>> for (unsigned int j = 0; j < SwIspStats::kYHistogramSize; j++) >>> sharedStats_->yHistogram[j] += s.yHistogram[j]; >>> } >> >
Hi 2026. 10. 18. 12:27 keltezéssel, Tobias Predel írta: > The software ISP AWB computes grey world means from the colour sums of > all sampled pixels. When part of the image is overexposed, the > strongest raw channel, usually green, clips first, so its mean is > underestimated. The resulting red and blue gains are too low and give > the whole image a green cast. This is easy to trigger with a bright > window or a lamp in view. > > Leave pixels with any colour component at or above 250/255 of full > scale out of the colour sums, and count the pixels that are included, > so that AWB divides by the right number. The luminance histogram still > covers all pixels, as AGC needs to see the saturated ones. > > Tested on a Hynix Hi556 with the simple pipeline handler and a ceiling > lamp in view (about 5% of the output clipped): the G/R and G/B ratios > of the unclipped pixels go from 1.11 and 1.16 to 1.00 and 1.00. With > no clipping in the scene, the AWB gains are unchanged. > > Signed-off-by: Tobias Predel <tobias.predel@gmail.com> > Assisted-by: Claude:claude-opus-5-5 > --- There was an earlier attempt that is effectively the same implementation: https://patchwork.libcamera.org/patch/28175/ Have you looked at / tested that? > > Notes: > The change in swstats_cpu.cpp applies to both the CPU and the GPU debayer > paths, as both use SwStatsCpu. I have also backported it to v0.7.2 > (src/ipa/simple) and have been running it as a distribution package. > > This patch was written with the help of an AI coding assistant (hence the > Assisted-by trailer, following the Linux kernel's coding-assistants > guidelines; checkstyle doesn't know that trailer key yet). I reviewed the > code and ran the tests on the hardware myself. > > .../internal/software_isp/swisp_stats.h | 7 +++- > src/ipa/softisp/algorithms/awb.cpp | 9 +++-- > src/libcamera/software_isp/swstats_cpu.cpp | 39 +++++++++++++------ > 3 files changed, 39 insertions(+), 16 deletions(-) > > diff --git a/include/libcamera/internal/software_isp/swisp_stats.h b/include/libcamera/internal/software_isp/swisp_stats.h > index d9d0d9be8..f15c8aae5 100644 > --- a/include/libcamera/internal/software_isp/swisp_stats.h > +++ b/include/libcamera/internal/software_isp/swisp_stats.h > @@ -28,9 +28,14 @@ struct SwIspStats { > */ > bool valid; > /** > - * \brief Sums of colour channels of all the sampled pixels > + * \brief Sums of colour channels of the sampled pixels that are not > + * (near) saturated > */ > RGB<uint64_t> sum_; > + /** > + * \brief Number of sampled pixels included in sum_ > + */ > + uint64_t sumCount_; > /** > * \brief Number of bins in the yHistogram > */ > diff --git a/src/ipa/softisp/algorithms/awb.cpp b/src/ipa/softisp/algorithms/awb.cpp > index 55fd326fc..e6b750d87 100644 > --- a/src/ipa/softisp/algorithms/awb.cpp > +++ b/src/ipa/softisp/algorithms/awb.cpp > @@ -7,7 +7,6 @@ > > #include "awb.h" > > -#include <numeric> > #include <stdint.h> > > #include <libcamera/base/log.h> > @@ -118,16 +117,18 @@ SoftIspAwbStats Awb::calculateRgbMeans(IPAContext &context, > if (!stats->valid) > return {}; > > - const SwIspStats::Histogram &histogram = stats->yHistogram; > const uint8_t blackLevel = context.activeState.blc.level; > > + /* Saturated pixels are not included in the sums. */ > + const uint64_t nPixels = stats->sumCount_; > + if (!nPixels) > + return {}; > + > /* > * Black level must be subtracted to get the correct AWB ratios, they > * would be off if they were computed from the whole brightness range > * rather than from the sensor range. > */ > - const uint64_t nPixels = std::accumulate( > - histogram.begin(), histogram.end(), uint64_t(0)); > const uint64_t offset = blackLevel * nPixels; > const uint64_t minValid = 1; > > diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp > index 7fb77ce7d..c4391a261 100644 > --- a/src/libcamera/software_isp/swstats_cpu.cpp > +++ b/src/libcamera/software_isp/swstats_cpu.cpp > @@ -11,6 +11,8 @@ > > #include "libcamera/internal/software_isp/swstats_cpu.h" > > +#include <algorithm> > + > #include <libcamera/base/log.h> > > #include <libcamera/stream.h> > @@ -171,28 +173,40 @@ static constexpr unsigned int kRedYMul = 77; /* 0.299 * 256 */ > static constexpr unsigned int kGreenYMul = 150; /* 0.587 * 256 */ > static constexpr unsigned int kBlueYMul = 29; /* 0.114 * 256 */ > > +/* > + * Pixels with any colour component at or above this 8-bit level are left out > + * of the colour sums. Clipped pixels have lost their colour information and > + * would skew the AWB ratios, typically towards the channel that clips last. > + */ > +static constexpr unsigned int kSaturationLevel = 250; > + > #define SWSTATS_START_LINE_STATS(pixel_t) \ > pixel_t r, g, g2, b; \ > uint64_t yVal; \ > \ > uint64_t sumR = 0; \ > uint64_t sumG = 0; \ > - uint64_t sumB = 0; > - > -#define SWSTATS_ACCUMULATE_LINE_STATS(div) \ > - sumR += r; \ > - sumG += g; \ > - sumB += b; \ > - \ > - yVal = r * kRedYMul; \ > - yVal += g * kGreenYMul; \ > - yVal += b * kBlueYMul; \ > + uint64_t sumB = 0; \ > + uint64_t sumCount = 0; > + > +#define SWSTATS_ACCUMULATE_LINE_STATS(div) \ > + if (std::max({ r, g, b }) < kSaturationLevel * (div)) { \ The aforementioned earlier change does separate comparisons for each component. I actually like that a bit more: * the comparisons are trivially independent, making any kind of parallelization by the compiler or cpu easier; * a quick compiler exporer test seems to suggest that the clang code for this `max()` might be a bit suboptimal > + sumR += r; \ > + sumG += g; \ > + sumB += b; \ > + sumCount++; \ > + } \ > + \ > + yVal = r * kRedYMul; \ > + yVal += g * kGreenYMul; \ > + yVal += b * kBlueYMul; \ > stats.yHistogram[yVal * SwIspStats::kYHistogramSize / (256 * 256 * (div))]++; > > #define SWSTATS_FINISH_LINE_STATS() \ > stats.sum_.r() += sumR; \ > stats.sum_.g() += sumG; \ > - stats.sum_.b() += sumB; > + stats.sum_.b() += sumB; \ > + stats.sumCount_ += sumCount; > > void SwStatsCpu::statsBGGR8Line0(const uint8_t *src[], SwIspStats &stats) > { > @@ -391,6 +405,7 @@ void SwStatsCpu::startFrame(uint32_t frame) > > for (auto &s : stats_) { > s.sum_ = RGB<uint64_t>({ 0, 0, 0 }); > + s.sumCount_ = 0; > s.yHistogram.fill(0); > } > } > @@ -408,9 +423,11 @@ void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId) > > if (valid) { > sharedStats_->sum_ = RGB<uint64_t>({ 0, 0, 0 }); > + sharedStats_->sumCount_ = 0; > sharedStats_->yHistogram.fill(0); > for (const auto &s : stats_) { > sharedStats_->sum_ += s.sum_; > + sharedStats_->sumCount_ += s.sumCount_; > for (unsigned int j = 0; j < SwIspStats::kYHistogramSize; j++) > sharedStats_->yHistogram[j] += s.yHistogram[j]; > }
Hi Barnabás, On 19.10.26 15:34, Barnabás Pőcze wrote: > There was an earlier attempt that is effectively the same implementation: > https://patchwork.libcamera.org/patch/28175/ > > Have you looked at / tested that? I hadn't seen it, sorry. Martin's patch came first, so I have tested it now on the Hi556 (master + only that patch, lamp in view, about 7% of pixels clipped). It gives the same result as mine within the run-to-run noise: G/R G/B master 1.13 - 1.14 1.27 - 1.31 Martin's 1.025 - 1.034 1.019 - 1.021 mine 1.023 - 1.028 1.021 - 1.023 I have sent a Tested-by for it, and I withdraw this patch in favour of Martin's. > The aforementioned earlier change does separate comparisons for each > component. I actually like that a bit more For what it's worth, with GCC (release build, Arch Linux) I could not see an advantage. Time spent in the stats line functions per stats frame, same scene: master ~549 us mine (std::max) ~640 us Martin's (&&) ~676 us Martin's with & instead && ~667 us The differences between the variants are within about 5% of the stats time and the run-to-run noise is around +-20 us, so this is about 0.1% of the frame time either way. I did not test clang. Hans, Laurent, thanks for the review. Hans, perhaps your Reviewed-by could go to Martin's patch instead. Regards, Tobias
2026. 10. 19. 16:27 keltezéssel, Tobias Predel írta: > Hi Barnabás, > > On 19.10.26 15:34, Barnabás Pőcze wrote: >> There was an earlier attempt that is effectively the same implementation: >> https://patchwork.libcamera.org/patch/28175/ >> >> Have you looked at / tested that? > > I hadn't seen it, sorry. Martin's patch came first, so I have tested it > now on the Hi556 (master + only that patch, lamp in view, about 7% of > pixels clipped). It gives the same result as mine within the > run-to-run noise: > > G/R G/B > master 1.13 - 1.14 1.27 - 1.31 > Martin's 1.025 - 1.034 1.019 - 1.021 > mine 1.023 - 1.028 1.021 - 1.023 > > I have sent a Tested-by for it, and I withdraw this patch in favour of > Martin's. > >> The aforementioned earlier change does separate comparisons for each >> component. I actually like that a bit more > > For what it's worth, with GCC (release build, Arch Linux) I could not > see an advantage. Time spent in the stats line functions per stats > frame, same scene: > > master ~549 us > mine (std::max) ~640 us > Martin's (&&) ~676 us > Martin's with & instead && ~667 us > > The differences between the variants are within about 5% of the stats > time and the run-to-run noise is around +-20 us, so this is about 0.1% > of the frame time either way. I did not test clang. Ahh, I did not expect such a big difference. Oh well, as they say, one should always measure. Though I wonder what kind of scene it was, and if `[[likely]]` annotation makes a difference. > > Hans, Laurent, thanks for the review. Hans, perhaps your Reviewed-by > could go to Martin's patch instead. > > Regards, > Tobias
diff --git a/include/libcamera/internal/software_isp/swisp_stats.h b/include/libcamera/internal/software_isp/swisp_stats.h index d9d0d9be8..f15c8aae5 100644 --- a/include/libcamera/internal/software_isp/swisp_stats.h +++ b/include/libcamera/internal/software_isp/swisp_stats.h @@ -28,9 +28,14 @@ struct SwIspStats { */ bool valid; /** - * \brief Sums of colour channels of all the sampled pixels + * \brief Sums of colour channels of the sampled pixels that are not + * (near) saturated */ RGB<uint64_t> sum_; + /** + * \brief Number of sampled pixels included in sum_ + */ + uint64_t sumCount_; /** * \brief Number of bins in the yHistogram */ diff --git a/src/ipa/softisp/algorithms/awb.cpp b/src/ipa/softisp/algorithms/awb.cpp index 55fd326fc..e6b750d87 100644 --- a/src/ipa/softisp/algorithms/awb.cpp +++ b/src/ipa/softisp/algorithms/awb.cpp @@ -7,7 +7,6 @@ #include "awb.h" -#include <numeric> #include <stdint.h> #include <libcamera/base/log.h> @@ -118,16 +117,18 @@ SoftIspAwbStats Awb::calculateRgbMeans(IPAContext &context, if (!stats->valid) return {}; - const SwIspStats::Histogram &histogram = stats->yHistogram; const uint8_t blackLevel = context.activeState.blc.level; + /* Saturated pixels are not included in the sums. */ + const uint64_t nPixels = stats->sumCount_; + if (!nPixels) + return {}; + /* * Black level must be subtracted to get the correct AWB ratios, they * would be off if they were computed from the whole brightness range * rather than from the sensor range. */ - const uint64_t nPixels = std::accumulate( - histogram.begin(), histogram.end(), uint64_t(0)); const uint64_t offset = blackLevel * nPixels; const uint64_t minValid = 1; diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp index 7fb77ce7d..c4391a261 100644 --- a/src/libcamera/software_isp/swstats_cpu.cpp +++ b/src/libcamera/software_isp/swstats_cpu.cpp @@ -11,6 +11,8 @@ #include "libcamera/internal/software_isp/swstats_cpu.h" +#include <algorithm> + #include <libcamera/base/log.h> #include <libcamera/stream.h> @@ -171,28 +173,40 @@ static constexpr unsigned int kRedYMul = 77; /* 0.299 * 256 */ static constexpr unsigned int kGreenYMul = 150; /* 0.587 * 256 */ static constexpr unsigned int kBlueYMul = 29; /* 0.114 * 256 */ +/* + * Pixels with any colour component at or above this 8-bit level are left out + * of the colour sums. Clipped pixels have lost their colour information and + * would skew the AWB ratios, typically towards the channel that clips last. + */ +static constexpr unsigned int kSaturationLevel = 250; + #define SWSTATS_START_LINE_STATS(pixel_t) \ pixel_t r, g, g2, b; \ uint64_t yVal; \ \ uint64_t sumR = 0; \ uint64_t sumG = 0; \ - uint64_t sumB = 0; - -#define SWSTATS_ACCUMULATE_LINE_STATS(div) \ - sumR += r; \ - sumG += g; \ - sumB += b; \ - \ - yVal = r * kRedYMul; \ - yVal += g * kGreenYMul; \ - yVal += b * kBlueYMul; \ + uint64_t sumB = 0; \ + uint64_t sumCount = 0; + +#define SWSTATS_ACCUMULATE_LINE_STATS(div) \ + if (std::max({ r, g, b }) < kSaturationLevel * (div)) { \ + sumR += r; \ + sumG += g; \ + sumB += b; \ + sumCount++; \ + } \ + \ + yVal = r * kRedYMul; \ + yVal += g * kGreenYMul; \ + yVal += b * kBlueYMul; \ stats.yHistogram[yVal * SwIspStats::kYHistogramSize / (256 * 256 * (div))]++; #define SWSTATS_FINISH_LINE_STATS() \ stats.sum_.r() += sumR; \ stats.sum_.g() += sumG; \ - stats.sum_.b() += sumB; + stats.sum_.b() += sumB; \ + stats.sumCount_ += sumCount; void SwStatsCpu::statsBGGR8Line0(const uint8_t *src[], SwIspStats &stats) { @@ -391,6 +405,7 @@ void SwStatsCpu::startFrame(uint32_t frame) for (auto &s : stats_) { s.sum_ = RGB<uint64_t>({ 0, 0, 0 }); + s.sumCount_ = 0; s.yHistogram.fill(0); } } @@ -408,9 +423,11 @@ void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId) if (valid) { sharedStats_->sum_ = RGB<uint64_t>({ 0, 0, 0 }); + sharedStats_->sumCount_ = 0; sharedStats_->yHistogram.fill(0); for (const auto &s : stats_) { sharedStats_->sum_ += s.sum_; + sharedStats_->sumCount_ += s.sumCount_; for (unsigned int j = 0; j < SwIspStats::kYHistogramSize; j++) sharedStats_->yHistogram[j] += s.yHistogram[j]; }
The software ISP AWB computes grey world means from the colour sums of all sampled pixels. When part of the image is overexposed, the strongest raw channel, usually green, clips first, so its mean is underestimated. The resulting red and blue gains are too low and give the whole image a green cast. This is easy to trigger with a bright window or a lamp in view. Leave pixels with any colour component at or above 250/255 of full scale out of the colour sums, and count the pixels that are included, so that AWB divides by the right number. The luminance histogram still covers all pixels, as AGC needs to see the saturated ones. Tested on a Hynix Hi556 with the simple pipeline handler and a ceiling lamp in view (about 5% of the output clipped): the G/R and G/B ratios of the unclipped pixels go from 1.11 and 1.16 to 1.00 and 1.00. With no clipping in the scene, the AWB gains are unchanged. Signed-off-by: Tobias Predel <tobias.predel@gmail.com> Assisted-by: Claude:claude-opus-5-5 --- Notes: The change in swstats_cpu.cpp applies to both the CPU and the GPU debayer paths, as both use SwStatsCpu. I have also backported it to v0.7.2 (src/ipa/simple) and have been running it as a distribution package. This patch was written with the help of an AI coding assistant (hence the Assisted-by trailer, following the Linux kernel's coding-assistants guidelines; checkstyle doesn't know that trailer key yet). I reviewed the code and ran the tests on the hardware myself. .../internal/software_isp/swisp_stats.h | 7 +++- src/ipa/softisp/algorithms/awb.cpp | 9 +++-- src/libcamera/software_isp/swstats_cpu.cpp | 39 +++++++++++++------ 3 files changed, 39 insertions(+), 16 deletions(-)