| Message ID | 1792488749.754103.325414999@isene.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series |
|
| Related | show |
Hi Unfortunately there are already two pending patches implementing effectively the exact same change: * https://patchwork.libcamera.org/patch/28502/ * https://patchwork.libcamera.org/patch/28175/ You can help by reviewing / testing the already posted ones. Or if you think your proposed change has any advantage compared to the others, please mention that. 2026. 10. 20. 11:32 keltezéssel, Geir Isene Ãrta: > The software ISP sets white balance by making the mean colour of the > sampled pixels grey. A clipped pixel reads the same in red, green and > blue, whatever its real colour. A bright window in the picture then > pulls the red and blue gains towards 1, and the rest of the picture > turns green. > > Add a pixel to the colour sums only when all its channels are below > 250 of 255 (scaled to the bit depth), and count those pixels in a new > sumPixels field. White balance takes the black level off and divides > by that count instead of the histogram total. When every pixel is > clipped, it returns no means and the last gains stay. > > On a Dell XPS 14 (OV02C10 on IPU6), with daylight windows behind the > person in a video call, 22% of the frame was clipped and a white wall > came out with red/green 0.73. With this change the same scene gives > 0.91, and 0.96 for the whole frame. > > Signed-off-by: Geir Isene <geir@isene.com> > --- > Tested on libcamera 0.7.2 on the laptop above, where it fixed the > picture. On master it builds without warnings and passes checkstyle; > I have not run master on the camera. > > Written with help from an AI model (Claude Opus 5.5). > > /Geir 🤗 > > .../internal/software_isp/swisp_stats.h | 11 ++++++++- > src/ipa/softisp/algorithms/awb.cpp | 8 +++---- > src/libcamera/software_isp/swstats_cpu.cpp | 24 +++++++++++++++---- > 3 files changed, 32 insertions(+), 11 deletions(-) > > diff --git a/include/libcamera/internal/software_isp/swisp_stats.h b/include/libcamera/internal/software_isp/swisp_stats.h > index d9d0d9b..7f4e457 100644 > --- a/include/libcamera/internal/software_isp/swisp_stats.h > +++ b/include/libcamera/internal/software_isp/swisp_stats.h > @@ -28,9 +28,18 @@ 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 > + * clipped > */ > RGB<uint64_t> sum_; > + /** > + * \brief Number of pixels added up in sum_ > + * > + * A pixel with any colour channel at or near the top of the range is > + * left out of sum_: it reads the same in all channels whatever its > + * real colour, and would pull white balance towards grey. > + */ > + uint64_t sumPixels; > /** > * \brief Number of bins in the yHistogram > */ > diff --git a/src/ipa/softisp/algorithms/awb.cpp b/src/ipa/softisp/algorithms/awb.cpp > index 55fd326..71f6997 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> > @@ -115,10 +114,10 @@ void Awb::prepare(IPAContext &context, > SoftIspAwbStats Awb::calculateRgbMeans(IPAContext &context, > const SwIspStats *stats) const > { > - if (!stats->valid) > + /* With every pixel clipped, return no means and keep the last gains. */ > + if (!stats->valid || !stats->sumPixels) > return {}; > > - const SwIspStats::Histogram &histogram = stats->yHistogram; > const uint8_t blackLevel = context.activeState.blc.level; > > /* > @@ -126,8 +125,7 @@ SoftIspAwbStats Awb::calculateRgbMeans(IPAContext &context, > * 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 nPixels = stats->sumPixels; > 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 7fb77ce..996223c 100644 > --- a/src/libcamera/software_isp/swstats_cpu.cpp > +++ b/src/libcamera/software_isp/swstats_cpu.cpp > @@ -177,12 +177,22 @@ static constexpr unsigned int kBlueYMul = 29; /* 0.114 * 256 */ > \ > uint64_t sumR = 0; \ > uint64_t sumG = 0; \ > - uint64_t sumB = 0; > + uint64_t sumB = 0; \ > + uint64_t sumN = 0; > > +/* > + * Leave clipped pixels, with any channel at 250 of 255 or above, out of the > + * colour sums. > + */ > #define SWSTATS_ACCUMULATE_LINE_STATS(div) \ > - sumR += r; \ > - sumG += g; \ > - sumB += b; \ > + if (r < 250 * (div) && \ > + g < 250 * (div) && \ > + b < 250 * (div)) { \ > + sumR += r; \ > + sumG += g; \ > + sumB += b; \ > + sumN++; \ > + } \ > \ > yVal = r * kRedYMul; \ > yVal += g * kGreenYMul; \ > @@ -192,7 +202,8 @@ static constexpr unsigned int kBlueYMul = 29; /* 0.114 * 256 */ > #define SWSTATS_FINISH_LINE_STATS() \ > stats.sum_.r() += sumR; \ > stats.sum_.g() += sumG; \ > - stats.sum_.b() += sumB; > + stats.sum_.b() += sumB; \ > + stats.sumPixels += sumN; > > void SwStatsCpu::statsBGGR8Line0(const uint8_t *src[], SwIspStats &stats) > { > @@ -391,6 +402,7 @@ void SwStatsCpu::startFrame(uint32_t frame) > > for (auto &s : stats_) { > s.sum_ = RGB<uint64_t>({ 0, 0, 0 }); > + s.sumPixels = 0; > s.yHistogram.fill(0); > } > } > @@ -408,9 +420,11 @@ void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId) > > if (valid) { > sharedStats_->sum_ = RGB<uint64_t>({ 0, 0, 0 }); > + sharedStats_->sumPixels = 0; > sharedStats_->yHistogram.fill(0); > for (const auto &s : stats_) { > sharedStats_->sum_ += s.sum_; > + sharedStats_->sumPixels += s.sumPixels; > for (unsigned int j = 0; j < SwIspStats::kYHistogramSize; j++) > sharedStats_->yHistogram[j] += s.yHistogram[j]; > }
diff --git a/include/libcamera/internal/software_isp/swisp_stats.h b/include/libcamera/internal/software_isp/swisp_stats.h index d9d0d9b..7f4e457 100644 --- a/include/libcamera/internal/software_isp/swisp_stats.h +++ b/include/libcamera/internal/software_isp/swisp_stats.h @@ -28,9 +28,18 @@ 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 + * clipped */ RGB<uint64_t> sum_; + /** + * \brief Number of pixels added up in sum_ + * + * A pixel with any colour channel at or near the top of the range is + * left out of sum_: it reads the same in all channels whatever its + * real colour, and would pull white balance towards grey. + */ + uint64_t sumPixels; /** * \brief Number of bins in the yHistogram */ diff --git a/src/ipa/softisp/algorithms/awb.cpp b/src/ipa/softisp/algorithms/awb.cpp index 55fd326..71f6997 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> @@ -115,10 +114,10 @@ void Awb::prepare(IPAContext &context, SoftIspAwbStats Awb::calculateRgbMeans(IPAContext &context, const SwIspStats *stats) const { - if (!stats->valid) + /* With every pixel clipped, return no means and keep the last gains. */ + if (!stats->valid || !stats->sumPixels) return {}; - const SwIspStats::Histogram &histogram = stats->yHistogram; const uint8_t blackLevel = context.activeState.blc.level; /* @@ -126,8 +125,7 @@ SoftIspAwbStats Awb::calculateRgbMeans(IPAContext &context, * 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 nPixels = stats->sumPixels; 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 7fb77ce..996223c 100644 --- a/src/libcamera/software_isp/swstats_cpu.cpp +++ b/src/libcamera/software_isp/swstats_cpu.cpp @@ -177,12 +177,22 @@ static constexpr unsigned int kBlueYMul = 29; /* 0.114 * 256 */ \ uint64_t sumR = 0; \ uint64_t sumG = 0; \ - uint64_t sumB = 0; + uint64_t sumB = 0; \ + uint64_t sumN = 0; +/* + * Leave clipped pixels, with any channel at 250 of 255 or above, out of the + * colour sums. + */ #define SWSTATS_ACCUMULATE_LINE_STATS(div) \ - sumR += r; \ - sumG += g; \ - sumB += b; \ + if (r < 250 * (div) && \ + g < 250 * (div) && \ + b < 250 * (div)) { \ + sumR += r; \ + sumG += g; \ + sumB += b; \ + sumN++; \ + } \ \ yVal = r * kRedYMul; \ yVal += g * kGreenYMul; \ @@ -192,7 +202,8 @@ static constexpr unsigned int kBlueYMul = 29; /* 0.114 * 256 */ #define SWSTATS_FINISH_LINE_STATS() \ stats.sum_.r() += sumR; \ stats.sum_.g() += sumG; \ - stats.sum_.b() += sumB; + stats.sum_.b() += sumB; \ + stats.sumPixels += sumN; void SwStatsCpu::statsBGGR8Line0(const uint8_t *src[], SwIspStats &stats) { @@ -391,6 +402,7 @@ void SwStatsCpu::startFrame(uint32_t frame) for (auto &s : stats_) { s.sum_ = RGB<uint64_t>({ 0, 0, 0 }); + s.sumPixels = 0; s.yHistogram.fill(0); } } @@ -408,9 +420,11 @@ void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId) if (valid) { sharedStats_->sum_ = RGB<uint64_t>({ 0, 0, 0 }); + sharedStats_->sumPixels = 0; sharedStats_->yHistogram.fill(0); for (const auto &s : stats_) { sharedStats_->sum_ += s.sum_; + sharedStats_->sumPixels += s.sumPixels; for (unsigned int j = 0; j < SwIspStats::kYHistogramSize; j++) sharedStats_->yHistogram[j] += s.yHistogram[j]; }
The software ISP sets white balance by making the mean colour of the sampled pixels grey. A clipped pixel reads the same in red, green and blue, whatever its real colour. A bright window in the picture then pulls the red and blue gains towards 1, and the rest of the picture turns green. Add a pixel to the colour sums only when all its channels are below 250 of 255 (scaled to the bit depth), and count those pixels in a new sumPixels field. White balance takes the black level off and divides by that count instead of the histogram total. When every pixel is clipped, it returns no means and the last gains stay. On a Dell XPS 14 (OV02C10 on IPU6), with daylight windows behind the person in a video call, 22% of the frame was clipped and a white wall came out with red/green 0.73. With this change the same scene gives 0.91, and 0.96 for the whole frame. Signed-off-by: Geir Isene <geir@isene.com> --- Tested on libcamera 0.7.2 on the laptop above, where it fixed the picture. On master it builds without warnings and passes checkstyle; I have not run master on the camera. Written with help from an AI model (Claude Opus 5.5). /Geir 🤗 .../internal/software_isp/swisp_stats.h | 11 ++++++++- src/ipa/softisp/algorithms/awb.cpp | 8 +++---- src/libcamera/software_isp/swstats_cpu.cpp | 24 +++++++++++++++---- 3 files changed, 32 insertions(+), 11 deletions(-)