| Message ID | 20261018102703.33012-1-tobias.predel@gmail.com |
|---|---|
| State | New |
| 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
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(-)