| Message ID | 20260902191554.84922-3-martincarvalho@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series |
|
| Related | show |
Hi Martin,
I posted a patch doing the same thing last week [1] without knowing about
yours, so I have tested yours on my camera instead.
Setup: Hynix Hi556, simple pipeline handler, CPU debayer, master at
1288b1524 with only this patch applied (plus the Hi556 sensor helper,
not patches 1, 3, 4 or 5 of this series). Ceiling lamp in view, about 7%
of the output pixels clipped. Mean G/R and G/B of the unclipped output
pixels, three interleaved runs:
G/R G/B
master 1.13 - 1.14 1.27 - 1.31
with this patch 1.025 - 1.034 1.019 - 1.021
The visible green cast is gone. The remaining offset of about 1.02-1.03
is also there in a scene with nothing clipped, so it comes from my
tuning file, not from this patch.
Tested-by: Tobias Predel <tobias.predel@gmail.com>
[1] https://patchwork.libcamera.org/patch/28502/
Regards,
Tobias
Hi Martin,
I sent the same change today without seeing yours [1]. I withdraw mine,
and have tested yours on my camera instead.
Setup:
- Dell XPS 14 (DA14250), OV02C10 on IPU6, simple pipeline handler
- master at 34911a1 with only this patch applied
- a tuning file with Intel's colour matrices for this sensor
- a bright window behind me, about 25% of the output clipped
Mean R/G and B/G of the unclipped output pixels at frame 150, with
master run before and after:
R/G B/G
master 0.69 - 0.78 0.71 - 0.76
with this patch 0.97 0.98
The green cast is gone. In a room with 4% clipped, both give 0.99.
Patch 4/5 no longer applies to master at 34911a1 (in
src/ipa/softisp/softisp.cpp).
Tested-by: Geir Isene <geir@isene.com>
[1] https://patchwork.libcamera.org/patch/28532/
/Geir 🤗
--
{ } Geir Isene :: +47-4550 2470 :: Make it Simple AS :: http://isene.com
On 2026-09-02 21:15, Martin Neiva de Carvalho wrote:
> The grey world estimate takes the sums of the colour channels over the
> frame and assumes the scene averages to grey. Saturated pixels break
> that: whatever colour a pixel was, once it clips it reads the same in
> every channel, so it carries no information about the illuminant. It is
> however the brightest kind of pixel there is, and the sums are weighted
> by brightness, so it carries a large share of the vote.
>
> Measured on a scene with a sunlit white wall filling part of the frame,
> 9% of the photosites were clipped and, being at full scale, accounted
> for roughly a third of the total sums. The estimate was dragged towards
> neutral and left a visible cyan cast: surfaces that should have been
> neutral came out with a linear R/G of 0.855 and B/G of 0.942. Leaving
> the saturated pixels out brings those to 0.944 and 0.986.
>
> Keep a separate count of the pixels that did contribute, because the
> black level the IPA subtracts from the sums is a per-pixel offset and
> would otherwise be scaled by the wrong number.
>
> Signed-off-by: Martin Neiva de Carvalho <martincarvalho@gmail.com>
> Tested-by: Tobias Predel <tobias.predel@gmail.com>
> ---
> .../internal/software_isp/swisp_stats.h | 8 ++++
> src/ipa/softisp/algorithms/awb.cpp | 15 +++++--
> src/libcamera/software_isp/swstats_cpu.cpp | 41 ++++++++++++++-----
> 3 files changed, 50 insertions(+), 14 deletions(-)
>
> diff --git a/include/libcamera/internal/software_isp/swisp_stats.h b/include/libcamera/internal/software_isp/swisp_stats.h
> index d9d0d9be8..77b70a251 100644
> --- a/include/libcamera/internal/software_isp/swisp_stats.h
> +++ b/include/libcamera/internal/software_isp/swisp_stats.h
> @@ -31,6 +31,14 @@ struct SwIspStats {
> * \brief Sums of colour channels of all the sampled pixels
> */
> RGB<uint64_t> sum_;
> + /**
> + * \brief Number of sampled pixels that contributed to sum_
> + *
> + * Saturated pixels are left out of the sums, so this is smaller than
> + * the number of pixels sampled. It is needed to subtract the black
> + * level from the sums, which is a per-pixel offset.
> + */
> + uint64_t sumSamples_;
> /**
> * \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..b48441987 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,24 @@ 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 counted in the sums, so the number of
> + * contributing pixels comes from the statistics rather than from the
> + * histogram, which counts every pixel sampled.
> + */
> + const uint64_t nPixels = stats->sumSamples_;
> +
> + /* Nothing but saturation was sampled, so there is nothing to say. */
> + 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..4349adf15 100644
> --- a/src/libcamera/software_isp/swstats_cpu.cpp
> +++ b/src/libcamera/software_isp/swstats_cpu.cpp
> @@ -167,6 +167,17 @@ SwStatsCpu::SwStatsCpu(const CameraManager &cm)
> << "Failed to create shared memory for statistics";
> }
>
> +/*
> + * Pixels at or above this fraction of full scale are treated as saturated and
> + * excluded from the colour sums. A saturated pixel reads the same in every
> + * channel whatever colour it was, so it says nothing about the illuminant -
> + * but it is the brightest kind of pixel there is, so it carries a lot of
> + * weight in a sum. A scene with a blown out window or wall can put a third of
> + * the total into pixels that carry no information, dragging the grey world
> + * estimate towards neutral and leaving a visible cast on everything else.
> + */
> +static constexpr unsigned int kSaturationThreshold = 248;
> +
> 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 */
> @@ -177,22 +188,29 @@ 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 nSamples = 0;
>
> -#define SWSTATS_ACCUMULATE_LINE_STATS(div) \
> - sumR += r; \
> - sumG += g; \
> - sumB += b; \
> - \
> - yVal = r * kRedYMul; \
> - yVal += g * kGreenYMul; \
> - yVal += b * kBlueYMul; \
> +#define SWSTATS_ACCUMULATE_LINE_STATS(div) \
> + if (r < kSaturationThreshold * (div) && \
> + g < kSaturationThreshold * (div) && \
> + b < kSaturationThreshold * (div)) { \
> + sumR += r; \
> + sumG += g; \
> + sumB += b; \
> + nSamples++; \
> + } \
> + \
> + 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.sumSamples_ += nSamples;
>
> void SwStatsCpu::statsBGGR8Line0(const uint8_t *src[], SwIspStats &stats)
> {
> @@ -391,6 +409,7 @@ void SwStatsCpu::startFrame(uint32_t frame)
>
> for (auto &s : stats_) {
> s.sum_ = RGB<uint64_t>({ 0, 0, 0 });
> + s.sumSamples_ = 0;
> s.yHistogram.fill(0);
> }
> }
> @@ -408,9 +427,11 @@ void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId)
>
> if (valid) {
> sharedStats_->sum_ = RGB<uint64_t>({ 0, 0, 0 });
> + sharedStats_->sumSamples_ = 0;
> sharedStats_->yHistogram.fill(0);
> for (const auto &s : stats_) {
> sharedStats_->sum_ += s.sum_;
> + sharedStats_->sumSamples_ += s.sumSamples_;
> for (unsigned int j = 0; j < SwIspStats::kYHistogramSize; j++)
> sharedStats_->yHistogram[j] += s.yHistogram[j];
> }
Wow - you guys look like you're copying each others homework assignment before submitting it to the teacher individually before class. Even your replies to the first iteration are practically identical. - https://lists.libcamera.org/pipermail/libcamera-devel/2026-October/062677.html What made all three of you find this exact same bug and solve it in exactly the same way? I think this is probably a correct thing to fix - but can a *human* explain things without using AI please so this isn't just us maintainers talking to a machine? How have you generated your local tuning file ? What images have you captured to validate this? Can you share some pictuers? or a graph of your findings ? Or is this all just straight out of the llm (including the e-mail replies). Are we really into an age where it would be better if you guys just told us what prompt you used instead ? -- Regards Kieran Quoting Geir Isene (2026-10-20 11:58:01) > Hi Martin, > > I sent the same change today without seeing yours [1]. I withdraw mine, > and have tested yours on my camera instead. > > Setup: > - Dell XPS 14 (DA14250), OV02C10 on IPU6, simple pipeline handler > - master at 34911a1 with only this patch applied > - a tuning file with Intel's colour matrices for this sensor > - a bright window behind me, about 25% of the output clipped > > Mean R/G and B/G of the unclipped output pixels at frame 150, with > master run before and after: > > R/G B/G > master 0.69 - 0.78 0.71 - 0.76 > with this patch 0.97 0.98 > > The green cast is gone. In a room with 4% clipped, both give 0.99. > > Patch 4/5 no longer applies to master at 34911a1 (in > src/ipa/softisp/softisp.cpp). > > Tested-by: Geir Isene <geir@isene.com> > > [1] https://patchwork.libcamera.org/patch/28532/ > > /Geir 🤗 > > -- > { } Geir Isene :: +47-4550 2470 :: Make it Simple AS :: http://isene.com > > On 2026-09-02 21:15, Martin Neiva de Carvalho wrote: > > The grey world estimate takes the sums of the colour channels over the > > frame and assumes the scene averages to grey. Saturated pixels break > > that: whatever colour a pixel was, once it clips it reads the same in > > every channel, so it carries no information about the illuminant. It is > > however the brightest kind of pixel there is, and the sums are weighted > > by brightness, so it carries a large share of the vote. > > > > Measured on a scene with a sunlit white wall filling part of the frame, > > 9% of the photosites were clipped and, being at full scale, accounted > > for roughly a third of the total sums. The estimate was dragged towards > > neutral and left a visible cyan cast: surfaces that should have been > > neutral came out with a linear R/G of 0.855 and B/G of 0.942. Leaving > > the saturated pixels out brings those to 0.944 and 0.986. > > > > Keep a separate count of the pixels that did contribute, because the > > black level the IPA subtracts from the sums is a per-pixel offset and > > would otherwise be scaled by the wrong number. > > > > Signed-off-by: Martin Neiva de Carvalho <martincarvalho@gmail.com> > > Tested-by: Tobias Predel <tobias.predel@gmail.com> > > --- > > .../internal/software_isp/swisp_stats.h | 8 ++++ > > src/ipa/softisp/algorithms/awb.cpp | 15 +++++-- > > src/libcamera/software_isp/swstats_cpu.cpp | 41 ++++++++++++++----- > > 3 files changed, 50 insertions(+), 14 deletions(-) > > > > diff --git a/include/libcamera/internal/software_isp/swisp_stats.h b/include/libcamera/internal/software_isp/swisp_stats.h > > index d9d0d9be8..77b70a251 100644 > > --- a/include/libcamera/internal/software_isp/swisp_stats.h > > +++ b/include/libcamera/internal/software_isp/swisp_stats.h > > @@ -31,6 +31,14 @@ struct SwIspStats { > > * \brief Sums of colour channels of all the sampled pixels > > */ > > RGB<uint64_t> sum_; > > + /** > > + * \brief Number of sampled pixels that contributed to sum_ > > + * > > + * Saturated pixels are left out of the sums, so this is smaller than > > + * the number of pixels sampled. It is needed to subtract the black > > + * level from the sums, which is a per-pixel offset. > > + */ > > + uint64_t sumSamples_; > > /** > > * \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..b48441987 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,24 @@ 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 counted in the sums, so the number of > > + * contributing pixels comes from the statistics rather than from the > > + * histogram, which counts every pixel sampled. > > + */ > > + const uint64_t nPixels = stats->sumSamples_; > > + > > + /* Nothing but saturation was sampled, so there is nothing to say. */ > > + 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..4349adf15 100644 > > --- a/src/libcamera/software_isp/swstats_cpu.cpp > > +++ b/src/libcamera/software_isp/swstats_cpu.cpp > > @@ -167,6 +167,17 @@ SwStatsCpu::SwStatsCpu(const CameraManager &cm) > > << "Failed to create shared memory for statistics"; > > } > > > > +/* > > + * Pixels at or above this fraction of full scale are treated as saturated and > > + * excluded from the colour sums. A saturated pixel reads the same in every > > + * channel whatever colour it was, so it says nothing about the illuminant - > > + * but it is the brightest kind of pixel there is, so it carries a lot of > > + * weight in a sum. A scene with a blown out window or wall can put a third of > > + * the total into pixels that carry no information, dragging the grey world > > + * estimate towards neutral and leaving a visible cast on everything else. > > + */ > > +static constexpr unsigned int kSaturationThreshold = 248; > > + > > 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 */ > > @@ -177,22 +188,29 @@ 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 nSamples = 0; > > > > -#define SWSTATS_ACCUMULATE_LINE_STATS(div) \ > > - sumR += r; \ > > - sumG += g; \ > > - sumB += b; \ > > - \ > > - yVal = r * kRedYMul; \ > > - yVal += g * kGreenYMul; \ > > - yVal += b * kBlueYMul; \ > > +#define SWSTATS_ACCUMULATE_LINE_STATS(div) \ > > + if (r < kSaturationThreshold * (div) && \ > > + g < kSaturationThreshold * (div) && \ > > + b < kSaturationThreshold * (div)) { \ > > + sumR += r; \ > > + sumG += g; \ > > + sumB += b; \ > > + nSamples++; \ > > + } \ > > + \ > > + 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.sumSamples_ += nSamples; > > > > void SwStatsCpu::statsBGGR8Line0(const uint8_t *src[], SwIspStats &stats) > > { > > @@ -391,6 +409,7 @@ void SwStatsCpu::startFrame(uint32_t frame) > > > > for (auto &s : stats_) { > > s.sum_ = RGB<uint64_t>({ 0, 0, 0 }); > > + s.sumSamples_ = 0; > > s.yHistogram.fill(0); > > } > > } > > @@ -408,9 +427,11 @@ void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId) > > > > if (valid) { > > sharedStats_->sum_ = RGB<uint64_t>({ 0, 0, 0 }); > > + sharedStats_->sumSamples_ = 0; > > sharedStats_->yHistogram.fill(0); > > for (const auto &s : stats_) { > > sharedStats_->sum_ += s.sum_; > > + sharedStats_->sumSamples_ += s.sumSamples_; > > for (unsigned int j = 0; j < SwIspStats::kYHistogramSize; j++) > > sharedStats_->yHistogram[j] += s.yHistogram[j]; > > }
Wow - you guys look like you're copying each others homework assignment before submitting it to the teacher individually before class. Even your replies to the first iteration are practically identical. - https://lists.libcamera.org/pipermail/libcamera-devel/2026-October/062732.html What made all three of you find this exact same bug and solve it in exactly the same way? I think this is probably a correct thing to fix - but can a *human* explain things without using AI please so this isn't just us maintainers talking to a machine? How have you generated your local tuning file ? What images have you captured to validate this? Can you share some pictuers? or a graph of your findings ? Or is this all just straight out of the llm (including the e-mail replies). Are we really into an age where it would be better if you guys just told us what prompt you used instead ? -- Regards Kieran Quoting Tobias Predel (2026-10-19 15:27:39) > Hi Martin, > > I posted a patch doing the same thing last week [1] without knowing about > yours, so I have tested yours on my camera instead. > > Setup: Hynix Hi556, simple pipeline handler, CPU debayer, master at > 1288b1524 with only this patch applied (plus the Hi556 sensor helper, > not patches 1, 3, 4 or 5 of this series). Ceiling lamp in view, about 7% > of the output pixels clipped. Mean G/R and G/B of the unclipped output > pixels, three interleaved runs: > > G/R G/B > master 1.13 - 1.14 1.27 - 1.31 > with this patch 1.025 - 1.034 1.019 - 1.021 > > The visible green cast is gone. The remaining offset of about 1.02-1.03 > is also there in a scene with nothing clipped, so it comes from my > tuning file, not from this patch. > > Tested-by: Tobias Predel <tobias.predel@gmail.com> > > [1] https://patchwork.libcamera.org/patch/28502/ > > Regards, > Tobias
On 2026-10-20 13:29, Kieran Bingham wrote: > > Are we really into an age where it would be better if you guys just told > us what prompt you used instead ? This can hardly be reduced to "what prompts?" In my case I have a meticulously tuned setup that has been a continuous work in progress over the past 16 months with plenty of pre/stop-hooks, CLAUDE.md files, output filters with retries, session and agent coordinations, my fleet setup etc. The prompts themselves are maybe 5%-10% of what is relevant for the outputs I send. In this case, when several solutions agree and the output turns out to work, then that should be an indication. Whatever the sentiments running, I have a working webcam on my laptop now. Regards /Geir 🤗
On Tue, Oct 20, 2026 at 01:40:14PM +0200, Geir wrote: > On 2026-10-20 13:29, Kieran Bingham wrote: > > > > Are we really into an age where it would be better if you guys just told > > us what prompt you used instead ? > > This can hardly be reduced to "what prompts?" > > In my case I have a meticulously tuned setup that has been a continuous > work in progress over the past 16 months with plenty of pre/stop-hooks, > CLAUDE.md files, output filters with retries, session and agent > coordinations, my fleet setup etc. The prompts themselves are maybe > 5%-10% of what is relevant for the outputs I send. > > In this case, when several solutions agree and the output turns out to work, > then that should be an indication. > > Whatever the sentiments running, I have a working webcam on my laptop now. And we have a project to maintain.
On 2026-10-20 14:48, Laurent Pinchart wrote: > > And we have a project to maintain. You sure do. May it give you fun. /Geir
Hi, On 2-Sep-26 21:15, Martin Neiva de Carvalho wrote: > The grey world estimate takes the sums of the colour channels over the > frame and assumes the scene averages to grey. Saturated pixels break > that: whatever colour a pixel was, once it clips it reads the same in > every channel, so it carries no information about the illuminant. It is > however the brightest kind of pixel there is, and the sums are weighted > by brightness, so it carries a large share of the vote. Actually the problem is bigger the just fully saturated pixels which contain "no illuminant info" being a big part of the sums. At least as big of a problem is partly saturated pixels where likely green has saturated and thus clipped but red / blue are not yet saturated these pixels do not contain "no illuminant info" they contain "wrong illuminant info". It would be good to update the commit message to mention that, with that fixed: > > Measured on a scene with a sunlit white wall filling part of the frame, > 9% of the photosites were clipped and, being at full scale, accounted > for roughly a third of the total sums. The estimate was dragged towards > neutral and left a visible cyan cast: surfaces that should have been > neutral came out with a linear R/G of 0.855 and B/G of 0.942. Leaving > the saturated pixels out brings those to 0.944 and 0.986. > > Keep a separate count of the pixels that did contribute, because the > black level the IPA subtracts from the sums is a per-pixel offset and > would otherwise be scaled by the wrong number. > > Signed-off-by: Martin Neiva de Carvalho <martincarvalho@gmail.com> > Tested-by: Tobias Predel <tobias.predel@gmail.com> > Tested-by: Geir Isene <geir@isene.com> > --- > .../internal/software_isp/swisp_stats.h | 8 ++++ > src/ipa/softisp/algorithms/awb.cpp | 15 +++++-- > src/libcamera/software_isp/swstats_cpu.cpp | 41 ++++++++++++++----- > 3 files changed, 50 insertions(+), 14 deletions(-) > > diff --git a/include/libcamera/internal/software_isp/swisp_stats.h b/include/libcamera/internal/software_isp/swisp_stats.h > index d9d0d9be8..77b70a251 100644 > --- a/include/libcamera/internal/software_isp/swisp_stats.h > +++ b/include/libcamera/internal/software_isp/swisp_stats.h > @@ -31,6 +31,14 @@ struct SwIspStats { > * \brief Sums of colour channels of all the sampled pixels > */ > RGB<uint64_t> sum_; > + /** > + * \brief Number of sampled pixels that contributed to sum_ > + * > + * Saturated pixels are left out of the sums, so this is smaller than > + * the number of pixels sampled. It is needed to subtract the black > + * level from the sums, which is a per-pixel offset. > + */ > + uint64_t sumSamples_; > /** > * \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..b48441987 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,24 @@ 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 counted in the sums, so the number of > + * contributing pixels comes from the statistics rather than from the > + * histogram, which counts every pixel sampled. > + */ > + const uint64_t nPixels = stats->sumSamples_; > + > + /* Nothing but saturation was sampled, so there is nothing to say. */ > + 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..4349adf15 100644 > --- a/src/libcamera/software_isp/swstats_cpu.cpp > +++ b/src/libcamera/software_isp/swstats_cpu.cpp > @@ -167,6 +167,17 @@ SwStatsCpu::SwStatsCpu(const CameraManager &cm) > << "Failed to create shared memory for statistics"; > } > > +/* > + * Pixels at or above this fraction of full scale are treated as saturated and > + * excluded from the colour sums. A saturated pixel reads the same in every > + * channel whatever colour it was, so it says nothing about the illuminant - > + * but it is the brightest kind of pixel there is, so it carries a lot of > + * weight in a sum. A scene with a blown out window or wall can put a third of > + * the total into pixels that carry no information, dragging the grey world > + * estimate towards neutral and leaving a visible cast on everything else. > + */ > +static constexpr unsigned int kSaturationThreshold = 248; > + > 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 */ > @@ -177,22 +188,29 @@ 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 nSamples = 0; > > -#define SWSTATS_ACCUMULATE_LINE_STATS(div) \ > - sumR += r; \ > - sumG += g; \ > - sumB += b; \ > - \ > - yVal = r * kRedYMul; \ > - yVal += g * kGreenYMul; \ > - yVal += b * kBlueYMul; \ > +#define SWSTATS_ACCUMULATE_LINE_STATS(div) \ > + if (r < kSaturationThreshold * (div) && \ > + g < kSaturationThreshold * (div) && \ > + b < kSaturationThreshold * (div)) { \ > + sumR += r; \ > + sumG += g; \ > + sumB += b; \ > + nSamples++; \ > + } \ > + \ > + 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.sumSamples_ += nSamples; > > void SwStatsCpu::statsBGGR8Line0(const uint8_t *src[], SwIspStats &stats) > { > @@ -391,6 +409,7 @@ void SwStatsCpu::startFrame(uint32_t frame) > > for (auto &s : stats_) { > s.sum_ = RGB<uint64_t>({ 0, 0, 0 }); > + s.sumSamples_ = 0; > s.yHistogram.fill(0); > } > } > @@ -408,9 +427,11 @@ void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId) > > if (valid) { > sharedStats_->sum_ = RGB<uint64_t>({ 0, 0, 0 }); > + sharedStats_->sumSamples_ = 0; > sharedStats_->yHistogram.fill(0); > for (const auto &s : stats_) { > sharedStats_->sum_ += s.sum_; > + sharedStats_->sumSamples_ += s.sumSamples_; > for (unsigned int j = 0; j < SwIspStats::kYHistogramSize; j++) > sharedStats_->yHistogram[j] += s.yHistogram[j]; > }
<hit send to early, take 2> Hi, On 2-Sep-26 21:15, Martin Neiva de Carvalho wrote: > The grey world estimate takes the sums of the colour channels over the > frame and assumes the scene averages to grey. Saturated pixels break > that: whatever colour a pixel was, once it clips it reads the same in > every channel, so it carries no information about the illuminant. It is > however the brightest kind of pixel there is, and the sums are weighted > by brightness, so it carries a large share of the vote. Actually the problem is bigger the just fully saturated pixels which contain "no illuminant info" being a big part of the sums. At least as big of a problem is partly saturated pixels where likely green has saturated and thus clipped but red / blue are not yet saturated these pixels do not contain "no illuminant info" they contain "wrong illuminant info". It would be good to update the commit message and the comment in the code about this to take clipping just one channel into account, with that fixed: Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com> Regards, Hans > > Measured on a scene with a sunlit white wall filling part of the frame, > 9% of the photosites were clipped and, being at full scale, accounted > for roughly a third of the total sums. The estimate was dragged towards > neutral and left a visible cyan cast: surfaces that should have been > neutral came out with a linear R/G of 0.855 and B/G of 0.942. Leaving > the saturated pixels out brings those to 0.944 and 0.986. > > Keep a separate count of the pixels that did contribute, because the > black level the IPA subtracts from the sums is a per-pixel offset and > would otherwise be scaled by the wrong number. > > Signed-off-by: Martin Neiva de Carvalho <martincarvalho@gmail.com> > Tested-by: Tobias Predel <tobias.predel@gmail.com> > Tested-by: Geir Isene <geir@isene.com> > --- > .../internal/software_isp/swisp_stats.h | 8 ++++ > src/ipa/softisp/algorithms/awb.cpp | 15 +++++-- > src/libcamera/software_isp/swstats_cpu.cpp | 41 ++++++++++++++----- > 3 files changed, 50 insertions(+), 14 deletions(-) > > diff --git a/include/libcamera/internal/software_isp/swisp_stats.h b/include/libcamera/internal/software_isp/swisp_stats.h > index d9d0d9be8..77b70a251 100644 > --- a/include/libcamera/internal/software_isp/swisp_stats.h > +++ b/include/libcamera/internal/software_isp/swisp_stats.h > @@ -31,6 +31,14 @@ struct SwIspStats { > * \brief Sums of colour channels of all the sampled pixels > */ > RGB<uint64_t> sum_; > + /** > + * \brief Number of sampled pixels that contributed to sum_ > + * > + * Saturated pixels are left out of the sums, so this is smaller than > + * the number of pixels sampled. It is needed to subtract the black > + * level from the sums, which is a per-pixel offset. > + */ > + uint64_t sumSamples_; > /** > * \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..b48441987 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,24 @@ 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 counted in the sums, so the number of > + * contributing pixels comes from the statistics rather than from the > + * histogram, which counts every pixel sampled. > + */ > + const uint64_t nPixels = stats->sumSamples_; > + > + /* Nothing but saturation was sampled, so there is nothing to say. */ > + 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..4349adf15 100644 > --- a/src/libcamera/software_isp/swstats_cpu.cpp > +++ b/src/libcamera/software_isp/swstats_cpu.cpp > @@ -167,6 +167,17 @@ SwStatsCpu::SwStatsCpu(const CameraManager &cm) > << "Failed to create shared memory for statistics"; > } > > +/* > + * Pixels at or above this fraction of full scale are treated as saturated and > + * excluded from the colour sums. A saturated pixel reads the same in every > + * channel whatever colour it was, so it says nothing about the illuminant - > + * but it is the brightest kind of pixel there is, so it carries a lot of > + * weight in a sum. A scene with a blown out window or wall can put a third of > + * the total into pixels that carry no information, dragging the grey world > + * estimate towards neutral and leaving a visible cast on everything else. > + */ > +static constexpr unsigned int kSaturationThreshold = 248; > + > 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 */ > @@ -177,22 +188,29 @@ 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 nSamples = 0; > > -#define SWSTATS_ACCUMULATE_LINE_STATS(div) \ > - sumR += r; \ > - sumG += g; \ > - sumB += b; \ > - \ > - yVal = r * kRedYMul; \ > - yVal += g * kGreenYMul; \ > - yVal += b * kBlueYMul; \ > +#define SWSTATS_ACCUMULATE_LINE_STATS(div) \ > + if (r < kSaturationThreshold * (div) && \ > + g < kSaturationThreshold * (div) && \ > + b < kSaturationThreshold * (div)) { \ > + sumR += r; \ > + sumG += g; \ > + sumB += b; \ > + nSamples++; \ > + } \ > + \ > + 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.sumSamples_ += nSamples; > > void SwStatsCpu::statsBGGR8Line0(const uint8_t *src[], SwIspStats &stats) > { > @@ -391,6 +409,7 @@ void SwStatsCpu::startFrame(uint32_t frame) > > for (auto &s : stats_) { > s.sum_ = RGB<uint64_t>({ 0, 0, 0 }); > + s.sumSamples_ = 0; > s.yHistogram.fill(0); > } > } > @@ -408,9 +427,11 @@ void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId) > > if (valid) { > sharedStats_->sum_ = RGB<uint64_t>({ 0, 0, 0 }); > + sharedStats_->sumSamples_ = 0; > sharedStats_->yHistogram.fill(0); > for (const auto &s : stats_) { > sharedStats_->sum_ += s.sum_; > + sharedStats_->sumSamples_ += s.sumSamples_; > for (unsigned int j = 0; j < SwIspStats::kYHistogramSize; j++) > sharedStats_->yHistogram[j] += s.yHistogram[j]; > }
Martin Neiva de Carvalho <martincarvalho@gmail.com> writes: > The grey world estimate takes the sums of the colour channels over the > frame and assumes the scene averages to grey. Saturated pixels break > that: whatever colour a pixel was, once it clips it reads the same in > every channel, so it carries no information about the illuminant. It is > however the brightest kind of pixel there is, and the sums are weighted > by brightness, so it carries a large share of the vote. > > Measured on a scene with a sunlit white wall filling part of the frame, > 9% of the photosites were clipped and, being at full scale, accounted > for roughly a third of the total sums. The estimate was dragged towards > neutral and left a visible cyan cast: surfaces that should have been > neutral came out with a linear R/G of 0.855 and B/G of 0.942. Leaving > the saturated pixels out brings those to 0.944 and 0.986. > > Keep a separate count of the pixels that did contribute, because the > black level the IPA subtracts from the sums is a per-pixel offset and > would otherwise be scaled by the wrong number. > > Signed-off-by: Martin Neiva de Carvalho <martincarvalho@gmail.com> > Tested-by: Tobias Predel <tobias.predel@gmail.com> > Tested-by: Geir Isene <geir@isene.com> > Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com> > --- > .../internal/software_isp/swisp_stats.h | 8 ++++ > src/ipa/softisp/algorithms/awb.cpp | 15 +++++-- > src/libcamera/software_isp/swstats_cpu.cpp | 41 ++++++++++++++----- > 3 files changed, 50 insertions(+), 14 deletions(-) > > diff --git a/include/libcamera/internal/software_isp/swisp_stats.h b/include/libcamera/internal/software_isp/swisp_stats.h > index d9d0d9be8..77b70a251 100644 > --- a/include/libcamera/internal/software_isp/swisp_stats.h > +++ b/include/libcamera/internal/software_isp/swisp_stats.h > @@ -31,6 +31,14 @@ struct SwIspStats { > * \brief Sums of colour channels of all the sampled pixels > */ > RGB<uint64_t> sum_; > + /** > + * \brief Number of sampled pixels that contributed to sum_ > + * > + * Saturated pixels are left out of the sums, so this is smaller than ... this can be smaller than (If there are no saturated pixels, it's equal.) > + * the number of pixels sampled. It is needed to subtract the black > + * level from the sums, which is a per-pixel offset. > + */ > + uint64_t sumSamples_; > /** > * \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..b48441987 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,24 @@ 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 counted in the sums, so the number of > + * contributing pixels comes from the statistics rather than from the > + * histogram, which counts every pixel sampled. > + */ > + const uint64_t nPixels = stats->sumSamples_; > + > + /* Nothing but saturation was sampled, so there is nothing to say. */ > + 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..4349adf15 100644 > --- a/src/libcamera/software_isp/swstats_cpu.cpp > +++ b/src/libcamera/software_isp/swstats_cpu.cpp > @@ -167,6 +167,17 @@ SwStatsCpu::SwStatsCpu(const CameraManager &cm) > << "Failed to create shared memory for statistics"; > } > > +/* > + * Pixels at or above this fraction of full scale are treated as saturated and The value is not a fraction. > + * excluded from the colour sums. A saturated pixel reads the same in every > + * channel whatever colour it was, so it says nothing about the illuminant - The code actually (rightly) considers a pixel saturated if any (rather than every) of the colour channels is above the threshold. I think the whole comment is unnecessarily verbose and could be shortened, e.g.: Pixels with any colour channel at or above this value are treated as saturated. Fully saturated pixels don't represent the values for white balance computation correctly; nearly saturated pixels may provide inaccurate values from hardware. Other than the inaccuracies in the comments, the patch looks good to me. With the comments fixed: Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > + * but it is the brightest kind of pixel there is, so it carries a lot of > + * weight in a sum. A scene with a blown out window or wall can put a third of > + * the total into pixels that carry no information, dragging the grey world > + * estimate towards neutral and leaving a visible cast on everything else. > + */ > +static constexpr unsigned int kSaturationThreshold = 248; > + > 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 */ > @@ -177,22 +188,29 @@ 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 nSamples = 0; > > -#define SWSTATS_ACCUMULATE_LINE_STATS(div) \ > - sumR += r; \ > - sumG += g; \ > - sumB += b; \ > - \ > - yVal = r * kRedYMul; \ > - yVal += g * kGreenYMul; \ > - yVal += b * kBlueYMul; \ > +#define SWSTATS_ACCUMULATE_LINE_STATS(div) \ > + if (r < kSaturationThreshold * (div) && \ > + g < kSaturationThreshold * (div) && \ > + b < kSaturationThreshold * (div)) { \ > + sumR += r; \ > + sumG += g; \ > + sumB += b; \ > + nSamples++; \ > + } \ > + \ > + 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.sumSamples_ += nSamples; > > void SwStatsCpu::statsBGGR8Line0(const uint8_t *src[], SwIspStats &stats) > { > @@ -391,6 +409,7 @@ void SwStatsCpu::startFrame(uint32_t frame) > > for (auto &s : stats_) { > s.sum_ = RGB<uint64_t>({ 0, 0, 0 }); > + s.sumSamples_ = 0; > s.yHistogram.fill(0); > } > } > @@ -408,9 +427,11 @@ void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId) > > if (valid) { > sharedStats_->sum_ = RGB<uint64_t>({ 0, 0, 0 }); > + sharedStats_->sumSamples_ = 0; > sharedStats_->yHistogram.fill(0); > for (const auto &s : stats_) { > sharedStats_->sum_ += s.sum_; > + sharedStats_->sumSamples_ += s.sumSamples_; > for (unsigned int j = 0; j < SwIspStats::kYHistogramSize; j++) > sharedStats_->yHistogram[j] += s.yHistogram[j]; > }
Hi All, As discussed earlier today I've benchmarked this patch on a UNO Q to see the performance impact on a platform wirh relatively low performance CPU cores. Here are the results of 3 runs with an imx219 sensor running 1600x1200 binned mode with EGL gpuISP rendering and gles qcam rendering. The numbers come from the builtin CPU stats benchmark. I've done 3 runs with this config with each of: 1. Plain libcamera master, 2. this patch, 3 the alternative "libcamera: software_isp: Exclude saturated pixels from AWB sums" patch which first does a std::max on r, g and b and then compares the result of the max() with the threshold instead of comparing each pixel individiually. This was done on Debian Trixie with the standard Debian gcc: plain: 18172 us/frame 16928 us/frame 17417 us/frame [PATCH 2/5] libcamera: software_isp: Exclude saturated pixels from colour sums 18393 us/frame 19002 us/frame 18737 us/frame [PATCH] libcamera: software_isp: Exclude saturated pixels from AWB sums 41940 us/frame 41667 us/frame 41973 us/frame So it seems that the performance impact of this patch is ok, whwre as the performance impact of the std::max approach is much more significant. Regards, Hans
diff --git a/include/libcamera/internal/software_isp/swisp_stats.h b/include/libcamera/internal/software_isp/swisp_stats.h index d9d0d9be8..77b70a251 100644 --- a/include/libcamera/internal/software_isp/swisp_stats.h +++ b/include/libcamera/internal/software_isp/swisp_stats.h @@ -31,6 +31,14 @@ struct SwIspStats { * \brief Sums of colour channels of all the sampled pixels */ RGB<uint64_t> sum_; + /** + * \brief Number of sampled pixels that contributed to sum_ + * + * Saturated pixels are left out of the sums, so this is smaller than + * the number of pixels sampled. It is needed to subtract the black + * level from the sums, which is a per-pixel offset. + */ + uint64_t sumSamples_; /** * \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..b48441987 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,24 @@ 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 counted in the sums, so the number of + * contributing pixels comes from the statistics rather than from the + * histogram, which counts every pixel sampled. + */ + const uint64_t nPixels = stats->sumSamples_; + + /* Nothing but saturation was sampled, so there is nothing to say. */ + 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..4349adf15 100644 --- a/src/libcamera/software_isp/swstats_cpu.cpp +++ b/src/libcamera/software_isp/swstats_cpu.cpp @@ -167,6 +167,17 @@ SwStatsCpu::SwStatsCpu(const CameraManager &cm) << "Failed to create shared memory for statistics"; } +/* + * Pixels at or above this fraction of full scale are treated as saturated and + * excluded from the colour sums. A saturated pixel reads the same in every + * channel whatever colour it was, so it says nothing about the illuminant - + * but it is the brightest kind of pixel there is, so it carries a lot of + * weight in a sum. A scene with a blown out window or wall can put a third of + * the total into pixels that carry no information, dragging the grey world + * estimate towards neutral and leaving a visible cast on everything else. + */ +static constexpr unsigned int kSaturationThreshold = 248; + 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 */ @@ -177,22 +188,29 @@ 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 nSamples = 0; -#define SWSTATS_ACCUMULATE_LINE_STATS(div) \ - sumR += r; \ - sumG += g; \ - sumB += b; \ - \ - yVal = r * kRedYMul; \ - yVal += g * kGreenYMul; \ - yVal += b * kBlueYMul; \ +#define SWSTATS_ACCUMULATE_LINE_STATS(div) \ + if (r < kSaturationThreshold * (div) && \ + g < kSaturationThreshold * (div) && \ + b < kSaturationThreshold * (div)) { \ + sumR += r; \ + sumG += g; \ + sumB += b; \ + nSamples++; \ + } \ + \ + 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.sumSamples_ += nSamples; void SwStatsCpu::statsBGGR8Line0(const uint8_t *src[], SwIspStats &stats) { @@ -391,6 +409,7 @@ void SwStatsCpu::startFrame(uint32_t frame) for (auto &s : stats_) { s.sum_ = RGB<uint64_t>({ 0, 0, 0 }); + s.sumSamples_ = 0; s.yHistogram.fill(0); } } @@ -408,9 +427,11 @@ void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId) if (valid) { sharedStats_->sum_ = RGB<uint64_t>({ 0, 0, 0 }); + sharedStats_->sumSamples_ = 0; sharedStats_->yHistogram.fill(0); for (const auto &s : stats_) { sharedStats_->sum_ += s.sum_; + sharedStats_->sumSamples_ += s.sumSamples_; for (unsigned int j = 0; j < SwIspStats::kYHistogramSize; j++) sharedStats_->yHistogram[j] += s.yHistogram[j]; }
The grey world estimate takes the sums of the colour channels over the frame and assumes the scene averages to grey. Saturated pixels break that: whatever colour a pixel was, once it clips it reads the same in every channel, so it carries no information about the illuminant. It is however the brightest kind of pixel there is, and the sums are weighted by brightness, so it carries a large share of the vote. Measured on a scene with a sunlit white wall filling part of the frame, 9% of the photosites were clipped and, being at full scale, accounted for roughly a third of the total sums. The estimate was dragged towards neutral and left a visible cyan cast: surfaces that should have been neutral came out with a linear R/G of 0.855 and B/G of 0.942. Leaving the saturated pixels out brings those to 0.944 and 0.986. Keep a separate count of the pixels that did contribute, because the black level the IPA subtracts from the sums is a per-pixel offset and would otherwise be scaled by the wrong number. Signed-off-by: Martin Neiva de Carvalho <martincarvalho@gmail.com> --- .../internal/software_isp/swisp_stats.h | 8 ++++ src/ipa/softisp/algorithms/awb.cpp | 15 +++++-- src/libcamera/software_isp/swstats_cpu.cpp | 41 ++++++++++++++----- 3 files changed, 50 insertions(+), 14 deletions(-)