| Message ID | 20260127123728.70513-1-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
On Tue, Jan 27, 2026 at 01:37:28PM +0100, Milan Zamazal wrote: > The black level offset subtracted in AWB is wrong. It assumes that the > stats contain sums of the individual colour pixels. But they actually > contain sums of the colour channels of larger "superpixels" consisting > of the individual colour pixels. Each of the RGB colour values and the > computed luminosity (a histogram entry) are added once to the stats per > such a superpixel. This means the offset computed from the black level > and the number of pixels should be used as it is, not divided. > > The patch fixes the subtracted offset. Since it is the same for all the > three colours, a SwIspStats helper method returning an RGB instance is > introduced. The individual colour variables are still retained in > SwIspStats for maximum efficiency when gathering the stats. SwIspStats > docstrings are changed to avoid the original confusion. > > Fixes: 4e13c6f55bd667 ("Honor black level in AWB") > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > .../internal/software_isp/swisp_stats.h | 17 +++++++++++++---- > src/ipa/simple/algorithms/awb.cpp | 10 ++++------ > 2 files changed, 17 insertions(+), 10 deletions(-) > > diff --git a/include/libcamera/internal/software_isp/swisp_stats.h b/include/libcamera/internal/software_isp/swisp_stats.h > index 3c9860185..f8816209f 100644 > --- a/include/libcamera/internal/software_isp/swisp_stats.h > +++ b/include/libcamera/internal/software_isp/swisp_stats.h > @@ -10,6 +10,8 @@ > #include <array> > #include <stdint.h> > > +#include "libcamera/internal/vector.h" > + > namespace libcamera { > > /** > @@ -26,17 +28,24 @@ struct SwIspStats { > */ > bool valid; > /** > - * \brief Holds the sum of all sampled red pixels > + * \brief Holds the sum of red channels of all the sampled pixels > */ > uint64_t sumR_; > /** > - * \brief Holds the sum of all sampled green pixels > + * \brief Holds the sum of green channels of all the sampled pixels > */ > uint64_t sumG_; > /** > - * \brief Holds the sum of all sampled blue pixels > + * \brief Holds the sum of blue channels of all the sampled pixels > */ > uint64_t sumB_; > + /** > + * \brief Return the sums of colour channels of all the sampled pixels > + */ > + RGB<uint64_t> rgbSum() const > + { > + return RGB<uint64_t>({ sumR_, sumG_, sumB_ }); > + } I would have replaced all this with RGB<uint64_t> sum_; (with an appropriate documentation block) > /** > * \brief Number of bins in the yHistogram > */ > @@ -46,7 +55,7 @@ struct SwIspStats { > */ > using Histogram = std::array<uint32_t, kYHistogramSize>; > /** > - * \brief A histogram of luminance values > + * \brief A histogram of luminance values of all the sampled pixels > */ > Histogram yHistogram; > }; > diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp > index 0080865aa..4c8bfd2de 100644 > --- a/src/ipa/simple/algorithms/awb.cpp > +++ b/src/ipa/simple/algorithms/awb.cpp > @@ -1,6 +1,6 @@ > /* SPDX-License-Identifier: LGPL-2.1-or-later */ > /* > - * Copyright (C) 2024, Red Hat Inc. > + * Copyright (C) 2024-2026 Red Hat Inc. > * > * Auto white balance > */ > @@ -73,9 +73,7 @@ void Awb::process(IPAContext &context, > histogram.begin(), histogram.end(), uint64_t(0)); > const uint64_t offset = blackLevel * nPixels; > const uint64_t minValid = 1; > - const uint64_t sumR = stats->sumR_ > offset / 4 ? stats->sumR_ - offset / 4 : minValid; > - const uint64_t sumG = stats->sumG_ > offset / 2 ? stats->sumG_ - offset / 2 : minValid; > - const uint64_t sumB = stats->sumB_ > offset / 4 ? stats->sumB_ - offset / 4 : minValid; > + const RGB<uint64_t> sum = (stats->rgbSum() - offset).max(minValid); > > /* > * Calculate red and blue gains for AWB. > @@ -83,9 +81,9 @@ void Awb::process(IPAContext &context, > */ > auto &gains = context.activeState.awb.gains; > gains = { { > - sumR <= sumG / 4 ? 4.0f : static_cast<float>(sumG) / sumR, > + sum.r() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.r(), > 1.0, > - sumB <= sumG / 4 ? 4.0f : static_cast<float>(sumG) / sumB, > + sum.b() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.b(), > } }; > > RGB<double> rgbGains{ { 1 / gains.r(), 1 / gains.g(), 1 / gains.b() } };
Nice, this seems to help quite a bit with reducing green tint on some qcom devices (with black levels set in the tuning files). On 27.01.26 13:37, Milan Zamazal wrote: > The black level offset subtracted in AWB is wrong. It assumes that the > stats contain sums of the individual colour pixels. But they actually > contain sums of the colour channels of larger "superpixels" consisting > of the individual colour pixels. Each of the RGB colour values and the > computed luminosity (a histogram entry) are added once to the stats per > such a superpixel. This means the offset computed from the black level > and the number of pixels should be used as it is, not divided. > > The patch fixes the subtracted offset. Since it is the same for all the > three colours, a SwIspStats helper method returning an RGB instance is > introduced. The individual colour variables are still retained in > SwIspStats for maximum efficiency when gathering the stats. SwIspStats > docstrings are changed to avoid the original confusion. > > Fixes: 4e13c6f55bd667 ("Honor black level in AWB") > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > .../internal/software_isp/swisp_stats.h | 17 +++++++++++++---- > src/ipa/simple/algorithms/awb.cpp | 10 ++++------ > 2 files changed, 17 insertions(+), 10 deletions(-) > > diff --git a/include/libcamera/internal/software_isp/swisp_stats.h b/include/libcamera/internal/software_isp/swisp_stats.h > index 3c9860185..f8816209f 100644 > --- a/include/libcamera/internal/software_isp/swisp_stats.h > +++ b/include/libcamera/internal/software_isp/swisp_stats.h > @@ -10,6 +10,8 @@ > #include <array> > #include <stdint.h> > > +#include "libcamera/internal/vector.h" > + > namespace libcamera { > > /** > @@ -26,17 +28,24 @@ struct SwIspStats { > */ > bool valid; > /** > - * \brief Holds the sum of all sampled red pixels > + * \brief Holds the sum of red channels of all the sampled pixels > */ > uint64_t sumR_; > /** > - * \brief Holds the sum of all sampled green pixels > + * \brief Holds the sum of green channels of all the sampled pixels > */ > uint64_t sumG_; > /** > - * \brief Holds the sum of all sampled blue pixels > + * \brief Holds the sum of blue channels of all the sampled pixels > */ > uint64_t sumB_; > + /** > + * \brief Return the sums of colour channels of all the sampled pixels > + */ > + RGB<uint64_t> rgbSum() const > + { > + return RGB<uint64_t>({ sumR_, sumG_, sumB_ }); > + } > /** > * \brief Number of bins in the yHistogram > */ > @@ -46,7 +55,7 @@ struct SwIspStats { > */ > using Histogram = std::array<uint32_t, kYHistogramSize>; > /** > - * \brief A histogram of luminance values > + * \brief A histogram of luminance values of all the sampled pixels > */ > Histogram yHistogram; > }; > diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp > index 0080865aa..4c8bfd2de 100644 > --- a/src/ipa/simple/algorithms/awb.cpp > +++ b/src/ipa/simple/algorithms/awb.cpp > @@ -1,6 +1,6 @@ > /* SPDX-License-Identifier: LGPL-2.1-or-later */ > /* > - * Copyright (C) 2024, Red Hat Inc. > + * Copyright (C) 2024-2026 Red Hat Inc. > * > * Auto white balance > */ > @@ -73,9 +73,7 @@ void Awb::process(IPAContext &context, > histogram.begin(), histogram.end(), uint64_t(0)); > const uint64_t offset = blackLevel * nPixels; > const uint64_t minValid = 1; > - const uint64_t sumR = stats->sumR_ > offset / 4 ? stats->sumR_ - offset / 4 : minValid; > - const uint64_t sumG = stats->sumG_ > offset / 2 ? stats->sumG_ - offset / 2 : minValid; > - const uint64_t sumB = stats->sumB_ > offset / 4 ? stats->sumB_ - offset / 4 : minValid; > + const RGB<uint64_t> sum = (stats->rgbSum() - offset).max(minValid); > > /* > * Calculate red and blue gains for AWB. > @@ -83,9 +81,9 @@ void Awb::process(IPAContext &context, > */ > auto &gains = context.activeState.awb.gains; > gains = { { > - sumR <= sumG / 4 ? 4.0f : static_cast<float>(sumG) / sumR, > + sum.r() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.r(), > 1.0, > - sumB <= sumG / 4 ? 4.0f : static_cast<float>(sumG) / sumB, > + sum.b() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.b(), > } }; > > RGB<double> rgbGains{ { 1 / gains.r(), 1 / gains.g(), 1 / gains.b() } };
Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > On Tue, Jan 27, 2026 at 01:37:28PM +0100, Milan Zamazal wrote: >> The black level offset subtracted in AWB is wrong. It assumes that the >> stats contain sums of the individual colour pixels. But they actually > >> contain sums of the colour channels of larger "superpixels" consisting >> of the individual colour pixels. Each of the RGB colour values and the >> computed luminosity (a histogram entry) are added once to the stats per >> such a superpixel. This means the offset computed from the black level >> and the number of pixels should be used as it is, not divided. >> >> The patch fixes the subtracted offset. Since it is the same for all the >> three colours, a SwIspStats helper method returning an RGB instance is >> introduced. The individual colour variables are still retained in >> SwIspStats for maximum efficiency when gathering the stats. SwIspStats >> docstrings are changed to avoid the original confusion. >> >> Fixes: 4e13c6f55bd667 ("Honor black level in AWB") >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> .../internal/software_isp/swisp_stats.h | 17 +++++++++++++---- >> src/ipa/simple/algorithms/awb.cpp | 10 ++++------ >> 2 files changed, 17 insertions(+), 10 deletions(-) >> >> diff --git a/include/libcamera/internal/software_isp/swisp_stats.h b/include/libcamera/internal/software_isp/swisp_stats.h >> index 3c9860185..f8816209f 100644 >> --- a/include/libcamera/internal/software_isp/swisp_stats.h >> +++ b/include/libcamera/internal/software_isp/swisp_stats.h >> @@ -10,6 +10,8 @@ >> #include <array> >> #include <stdint.h> >> >> +#include "libcamera/internal/vector.h" >> + >> namespace libcamera { >> >> /** >> @@ -26,17 +28,24 @@ struct SwIspStats { >> */ >> bool valid; >> /** >> - * \brief Holds the sum of all sampled red pixels >> + * \brief Holds the sum of red channels of all the sampled pixels >> */ >> uint64_t sumR_; >> /** >> - * \brief Holds the sum of all sampled green pixels >> + * \brief Holds the sum of green channels of all the sampled pixels >> */ >> uint64_t sumG_; >> /** >> - * \brief Holds the sum of all sampled blue pixels >> + * \brief Holds the sum of blue channels of all the sampled pixels >> */ >> uint64_t sumB_; >> + /** >> + * \brief Return the sums of colour channels of all the sampled pixels >> + */ >> + RGB<uint64_t> rgbSum() const >> + { >> + return RGB<uint64_t>({ sumR_, sumG_, sumB_ }); >> + } > > I would have replaced all this with > > RGB<uint64_t> sum_; OK, I measured the performance impact and there is none in my environment. Done in v2. > (with an appropriate documentation block) I don't feel particularly creative here for an internal structure. Do we need to mention anything special here? >> /** >> * \brief Number of bins in the yHistogram >> */ >> @@ -46,7 +55,7 @@ struct SwIspStats { >> */ >> using Histogram = std::array<uint32_t, kYHistogramSize>; >> /** >> - * \brief A histogram of luminance values >> + * \brief A histogram of luminance values of all the sampled pixels >> */ >> Histogram yHistogram; >> }; >> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp >> index 0080865aa..4c8bfd2de 100644 >> --- a/src/ipa/simple/algorithms/awb.cpp >> +++ b/src/ipa/simple/algorithms/awb.cpp >> @@ -1,6 +1,6 @@ >> /* SPDX-License-Identifier: LGPL-2.1-or-later */ >> /* >> - * Copyright (C) 2024, Red Hat Inc. >> + * Copyright (C) 2024-2026 Red Hat Inc. >> * >> * Auto white balance >> */ >> @@ -73,9 +73,7 @@ void Awb::process(IPAContext &context, >> histogram.begin(), histogram.end(), uint64_t(0)); >> const uint64_t offset = blackLevel * nPixels; >> const uint64_t minValid = 1; >> - const uint64_t sumR = stats->sumR_ > offset / 4 ? stats->sumR_ - offset / 4 : minValid; >> - const uint64_t sumG = stats->sumG_ > offset / 2 ? stats->sumG_ - offset / 2 : minValid; >> - const uint64_t sumB = stats->sumB_ > offset / 4 ? stats->sumB_ - offset / 4 : minValid; >> + const RGB<uint64_t> sum = (stats->rgbSum() - offset).max(minValid); >> >> /* >> * Calculate red and blue gains for AWB. >> @@ -83,9 +81,9 @@ void Awb::process(IPAContext &context, >> */ >> auto &gains = context.activeState.awb.gains; >> gains = { { >> - sumR <= sumG / 4 ? 4.0f : static_cast<float>(sumG) / sumR, >> + sum.r() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.r(), >> 1.0, >> - sumB <= sumG / 4 ? 4.0f : static_cast<float>(sumG) / sumB, >> + sum.b() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.b(), >> } }; >> >> RGB<double> rgbGains{ { 1 / gains.r(), 1 / gains.g(), 1 / gains.b() } };
Robert Mader <robert.mader@collabora.com> writes: > Nice, this seems to help quite a bit with reducing green tint on some qcom devices (with black levels set in the tuning files). Thank you for testing and confirmation it works as expected in properly set up environments. > On 27.01.26 13:37, Milan Zamazal wrote: >> The black level offset subtracted in AWB is wrong. It assumes that the >> stats contain sums of the individual colour pixels. But they actually >> contain sums of the colour channels of larger "superpixels" consisting >> of the individual colour pixels. Each of the RGB colour values and the >> computed luminosity (a histogram entry) are added once to the stats per >> such a superpixel. This means the offset computed from the black level >> and the number of pixels should be used as it is, not divided. >> >> The patch fixes the subtracted offset. Since it is the same for all the >> three colours, a SwIspStats helper method returning an RGB instance is >> introduced. The individual colour variables are still retained in >> SwIspStats for maximum efficiency when gathering the stats. SwIspStats >> docstrings are changed to avoid the original confusion. >> >> Fixes: 4e13c6f55bd667 ("Honor black level in AWB") >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> .../internal/software_isp/swisp_stats.h | 17 +++++++++++++---- >> src/ipa/simple/algorithms/awb.cpp | 10 ++++------ >> 2 files changed, 17 insertions(+), 10 deletions(-) >> >> diff --git a/include/libcamera/internal/software_isp/swisp_stats.h b/include/libcamera/internal/software_isp/swisp_stats.h >> index 3c9860185..f8816209f 100644 >> --- a/include/libcamera/internal/software_isp/swisp_stats.h >> +++ b/include/libcamera/internal/software_isp/swisp_stats.h >> @@ -10,6 +10,8 @@ >> #include <array> >> #include <stdint.h> >> +#include "libcamera/internal/vector.h" >> + >> namespace libcamera { >> /** >> @@ -26,17 +28,24 @@ struct SwIspStats { >> */ >> bool valid; >> /** >> - * \brief Holds the sum of all sampled red pixels >> + * \brief Holds the sum of red channels of all the sampled pixels >> */ >> uint64_t sumR_; >> /** >> - * \brief Holds the sum of all sampled green pixels >> + * \brief Holds the sum of green channels of all the sampled pixels >> */ >> uint64_t sumG_; >> /** >> - * \brief Holds the sum of all sampled blue pixels >> + * \brief Holds the sum of blue channels of all the sampled pixels >> */ >> uint64_t sumB_; >> + /** >> + * \brief Return the sums of colour channels of all the sampled pixels >> + */ >> + RGB<uint64_t> rgbSum() const >> + { >> + return RGB<uint64_t>({ sumR_, sumG_, sumB_ }); >> + } >> /** >> * \brief Number of bins in the yHistogram >> */ >> @@ -46,7 +55,7 @@ struct SwIspStats { >> */ >> using Histogram = std::array<uint32_t, kYHistogramSize>; >> /** >> - * \brief A histogram of luminance values >> + * \brief A histogram of luminance values of all the sampled pixels >> */ >> Histogram yHistogram; >> }; >> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp >> index 0080865aa..4c8bfd2de 100644 >> --- a/src/ipa/simple/algorithms/awb.cpp >> +++ b/src/ipa/simple/algorithms/awb.cpp >> @@ -1,6 +1,6 @@ >> /* SPDX-License-Identifier: LGPL-2.1-or-later */ >> /* >> - * Copyright (C) 2024, Red Hat Inc. >> + * Copyright (C) 2024-2026 Red Hat Inc. >> * >> * Auto white balance >> */ >> @@ -73,9 +73,7 @@ void Awb::process(IPAContext &context, >> histogram.begin(), histogram.end(), uint64_t(0)); >> const uint64_t offset = blackLevel * nPixels; >> const uint64_t minValid = 1; >> - const uint64_t sumR = stats->sumR_ > offset / 4 ? stats->sumR_ - offset / 4 : minValid; >> - const uint64_t sumG = stats->sumG_ > offset / 2 ? stats->sumG_ - offset / 2 : minValid; >> - const uint64_t sumB = stats->sumB_ > offset / 4 ? stats->sumB_ - offset / 4 : minValid; >> + const RGB<uint64_t> sum = (stats->rgbSum() - offset).max(minValid); >> /* >> * Calculate red and blue gains for AWB. >> @@ -83,9 +81,9 @@ void Awb::process(IPAContext &context, >> */ >> auto &gains = context.activeState.awb.gains; >> gains = { { >> - sumR <= sumG / 4 ? 4.0f : static_cast<float>(sumG) / sumR, >> + sum.r() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.r(), >> 1.0, >> - sumB <= sumG / 4 ? 4.0f : static_cast<float>(sumG) / sumB, >> + sum.b() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.b(), >> } }; >> RGB<double> rgbGains{ { 1 / gains.r(), 1 / gains.g(), 1 / gains.b() } };
On Tue, Jan 27, 2026 at 05:31:33PM +0100, Milan Zamazal wrote: > Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > > > On Tue, Jan 27, 2026 at 01:37:28PM +0100, Milan Zamazal wrote: > >> The black level offset subtracted in AWB is wrong. It assumes that the > >> stats contain sums of the individual colour pixels. But they actually > > > >> contain sums of the colour channels of larger "superpixels" consisting > >> of the individual colour pixels. Each of the RGB colour values and the > >> computed luminosity (a histogram entry) are added once to the stats per > >> such a superpixel. This means the offset computed from the black level > >> and the number of pixels should be used as it is, not divided. > >> > >> The patch fixes the subtracted offset. Since it is the same for all the > >> three colours, a SwIspStats helper method returning an RGB instance is > >> introduced. The individual colour variables are still retained in > >> SwIspStats for maximum efficiency when gathering the stats. SwIspStats > >> docstrings are changed to avoid the original confusion. > >> > >> Fixes: 4e13c6f55bd667 ("Honor black level in AWB") > >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > >> --- > >> .../internal/software_isp/swisp_stats.h | 17 +++++++++++++---- > >> src/ipa/simple/algorithms/awb.cpp | 10 ++++------ > >> 2 files changed, 17 insertions(+), 10 deletions(-) > >> > >> diff --git a/include/libcamera/internal/software_isp/swisp_stats.h b/include/libcamera/internal/software_isp/swisp_stats.h > >> index 3c9860185..f8816209f 100644 > >> --- a/include/libcamera/internal/software_isp/swisp_stats.h > >> +++ b/include/libcamera/internal/software_isp/swisp_stats.h > >> @@ -10,6 +10,8 @@ > >> #include <array> > >> #include <stdint.h> > >> > >> +#include "libcamera/internal/vector.h" > >> + > >> namespace libcamera { > >> > >> /** > >> @@ -26,17 +28,24 @@ struct SwIspStats { > >> */ > >> bool valid; > >> /** > >> - * \brief Holds the sum of all sampled red pixels > >> + * \brief Holds the sum of red channels of all the sampled pixels > >> */ > >> uint64_t sumR_; > >> /** > >> - * \brief Holds the sum of all sampled green pixels > >> + * \brief Holds the sum of green channels of all the sampled pixels > >> */ > >> uint64_t sumG_; > >> /** > >> - * \brief Holds the sum of all sampled blue pixels > >> + * \brief Holds the sum of blue channels of all the sampled pixels > >> */ > >> uint64_t sumB_; > >> + /** > >> + * \brief Return the sums of colour channels of all the sampled pixels > >> + */ > >> + RGB<uint64_t> rgbSum() const > >> + { > >> + return RGB<uint64_t>({ sumR_, sumG_, sumB_ }); > >> + } > > > > I would have replaced all this with > > > > RGB<uint64_t> sum_; > > OK, I measured the performance impact and there is none in my > environment. Done in v2. > > > (with an appropriate documentation block) > > I don't feel particularly creative here for an internal structure. Do > we need to mention anything special here? No, I just mentioned it to make it clear I wasn't suggesting dropping the documentation. Something like /** * \brief The sum of all sampled pixels, separately for R, G and B. */ would do. > >> /** > >> * \brief Number of bins in the yHistogram > >> */ > >> @@ -46,7 +55,7 @@ struct SwIspStats { > >> */ > >> using Histogram = std::array<uint32_t, kYHistogramSize>; > >> /** > >> - * \brief A histogram of luminance values > >> + * \brief A histogram of luminance values of all the sampled pixels > >> */ > >> Histogram yHistogram; > >> }; > >> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp > >> index 0080865aa..4c8bfd2de 100644 > >> --- a/src/ipa/simple/algorithms/awb.cpp > >> +++ b/src/ipa/simple/algorithms/awb.cpp > >> @@ -1,6 +1,6 @@ > >> /* SPDX-License-Identifier: LGPL-2.1-or-later */ > >> /* > >> - * Copyright (C) 2024, Red Hat Inc. > >> + * Copyright (C) 2024-2026 Red Hat Inc. > >> * > >> * Auto white balance > >> */ > >> @@ -73,9 +73,7 @@ void Awb::process(IPAContext &context, > >> histogram.begin(), histogram.end(), uint64_t(0)); > >> const uint64_t offset = blackLevel * nPixels; > >> const uint64_t minValid = 1; > >> - const uint64_t sumR = stats->sumR_ > offset / 4 ? stats->sumR_ - offset / 4 : minValid; > >> - const uint64_t sumG = stats->sumG_ > offset / 2 ? stats->sumG_ - offset / 2 : minValid; > >> - const uint64_t sumB = stats->sumB_ > offset / 4 ? stats->sumB_ - offset / 4 : minValid; > >> + const RGB<uint64_t> sum = (stats->rgbSum() - offset).max(minValid); > >> > >> /* > >> * Calculate red and blue gains for AWB. > >> @@ -83,9 +81,9 @@ void Awb::process(IPAContext &context, > >> */ > >> auto &gains = context.activeState.awb.gains; > >> gains = { { > >> - sumR <= sumG / 4 ? 4.0f : static_cast<float>(sumG) / sumR, > >> + sum.r() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.r(), > >> 1.0, > >> - sumB <= sumG / 4 ? 4.0f : static_cast<float>(sumG) / sumB, > >> + sum.b() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.b(), > >> } }; > >> > >> RGB<double> rgbGains{ { 1 / gains.r(), 1 / gains.g(), 1 / gains.b() } };
diff --git a/include/libcamera/internal/software_isp/swisp_stats.h b/include/libcamera/internal/software_isp/swisp_stats.h index 3c9860185..f8816209f 100644 --- a/include/libcamera/internal/software_isp/swisp_stats.h +++ b/include/libcamera/internal/software_isp/swisp_stats.h @@ -10,6 +10,8 @@ #include <array> #include <stdint.h> +#include "libcamera/internal/vector.h" + namespace libcamera { /** @@ -26,17 +28,24 @@ struct SwIspStats { */ bool valid; /** - * \brief Holds the sum of all sampled red pixels + * \brief Holds the sum of red channels of all the sampled pixels */ uint64_t sumR_; /** - * \brief Holds the sum of all sampled green pixels + * \brief Holds the sum of green channels of all the sampled pixels */ uint64_t sumG_; /** - * \brief Holds the sum of all sampled blue pixels + * \brief Holds the sum of blue channels of all the sampled pixels */ uint64_t sumB_; + /** + * \brief Return the sums of colour channels of all the sampled pixels + */ + RGB<uint64_t> rgbSum() const + { + return RGB<uint64_t>({ sumR_, sumG_, sumB_ }); + } /** * \brief Number of bins in the yHistogram */ @@ -46,7 +55,7 @@ struct SwIspStats { */ using Histogram = std::array<uint32_t, kYHistogramSize>; /** - * \brief A histogram of luminance values + * \brief A histogram of luminance values of all the sampled pixels */ Histogram yHistogram; }; diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp index 0080865aa..4c8bfd2de 100644 --- a/src/ipa/simple/algorithms/awb.cpp +++ b/src/ipa/simple/algorithms/awb.cpp @@ -1,6 +1,6 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ /* - * Copyright (C) 2024, Red Hat Inc. + * Copyright (C) 2024-2026 Red Hat Inc. * * Auto white balance */ @@ -73,9 +73,7 @@ void Awb::process(IPAContext &context, histogram.begin(), histogram.end(), uint64_t(0)); const uint64_t offset = blackLevel * nPixels; const uint64_t minValid = 1; - const uint64_t sumR = stats->sumR_ > offset / 4 ? stats->sumR_ - offset / 4 : minValid; - const uint64_t sumG = stats->sumG_ > offset / 2 ? stats->sumG_ - offset / 2 : minValid; - const uint64_t sumB = stats->sumB_ > offset / 4 ? stats->sumB_ - offset / 4 : minValid; + const RGB<uint64_t> sum = (stats->rgbSum() - offset).max(minValid); /* * Calculate red and blue gains for AWB. @@ -83,9 +81,9 @@ void Awb::process(IPAContext &context, */ auto &gains = context.activeState.awb.gains; gains = { { - sumR <= sumG / 4 ? 4.0f : static_cast<float>(sumG) / sumR, + sum.r() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.r(), 1.0, - sumB <= sumG / 4 ? 4.0f : static_cast<float>(sumG) / sumB, + sum.b() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.b(), } }; RGB<double> rgbGains{ { 1 / gains.r(), 1 / gains.g(), 1 / gains.b() } };
The black level offset subtracted in AWB is wrong. It assumes that the stats contain sums of the individual colour pixels. But they actually contain sums of the colour channels of larger "superpixels" consisting of the individual colour pixels. Each of the RGB colour values and the computed luminosity (a histogram entry) are added once to the stats per such a superpixel. This means the offset computed from the black level and the number of pixels should be used as it is, not divided. The patch fixes the subtracted offset. Since it is the same for all the three colours, a SwIspStats helper method returning an RGB instance is introduced. The individual colour variables are still retained in SwIspStats for maximum efficiency when gathering the stats. SwIspStats docstrings are changed to avoid the original confusion. Fixes: 4e13c6f55bd667 ("Honor black level in AWB") Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- .../internal/software_isp/swisp_stats.h | 17 +++++++++++++---- src/ipa/simple/algorithms/awb.cpp | 10 ++++------ 2 files changed, 17 insertions(+), 10 deletions(-)