| Message ID | 20250925192856.77881-6-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi 2025. 09. 25. 21:28 keltezéssel, Milan Zamazal írta: > The R/G/B sums computed in AWB simple IPA may be zero or perhaps even > negative. Let's make sure the sums are always positive, to prevent > division by zero or completely nonsense results. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/ipa/simple/algorithms/awb.cpp | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp > index cf567e894..8231a4968 100644 > --- a/src/ipa/simple/algorithms/awb.cpp > +++ b/src/ipa/simple/algorithms/awb.cpp > @@ -7,6 +7,7 @@ > > #include "awb.h" > > +#include <algorithm> > #include <numeric> > #include <stdint.h> > > @@ -68,10 +69,11 @@ void Awb::process(IPAContext &context, > */ > const uint64_t nPixels = std::accumulate( > histogram.begin(), histogram.end(), 0); An unrelated thing I noticed. I think the intention may have been to have something like const uint64_t nPixels = std::accumulate(histogram.begin(), histogram.end(), uint64_t(0)); because otherwise the summation will use the type of `0`, which is just `int`. > - const uint64_t offset = blackLevel * nPixels; > - const uint64_t sumR = stats->sumR_ - offset / 4; > - const uint64_t sumG = stats->sumG_ - offset / 2; > - const uint64_t sumB = stats->sumB_ - offset / 4; > + const int64_t offset = blackLevel * nPixels; > + const int64_t minValid = 1; > + const uint64_t sumR = std::max(static_cast<int64_t>(stats->sumR_) - offset / 4, minValid); > + const uint64_t sumG = std::max(static_cast<int64_t>(stats->sumG_) - offset / 2, minValid); > + const uint64_t sumB = std::max(static_cast<int64_t>(stats->sumB_) - offset / 4, minValid); Have you seen this wrap-around in practice? In any case, the checks looks reasonable, but I would potentially do the following to avoid going through a singed type: sumR = stats->sumR_ > offset / 4 ? stats->sumR - offset / 4 : 1; Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > /* > * Calculate red and blue gains for AWB.
Hi, On 25-Sep-25 21:28, Milan Zamazal wrote: > The R/G/B sums computed in AWB simple IPA may be zero or perhaps even > negative. Let's make sure the sums are always positive, to prevent > division by zero or completely nonsense results. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> Thanks, patch looks good to me: Reviewed-by: Hans de Goede <hansg@kernel.org> Regards, Hans > --- > src/ipa/simple/algorithms/awb.cpp | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp > index cf567e894..8231a4968 100644 > --- a/src/ipa/simple/algorithms/awb.cpp > +++ b/src/ipa/simple/algorithms/awb.cpp > @@ -7,6 +7,7 @@ > > #include "awb.h" > > +#include <algorithm> > #include <numeric> > #include <stdint.h> > > @@ -68,10 +69,11 @@ void Awb::process(IPAContext &context, > */ > const uint64_t nPixels = std::accumulate( > histogram.begin(), histogram.end(), 0); > - const uint64_t offset = blackLevel * nPixels; > - const uint64_t sumR = stats->sumR_ - offset / 4; > - const uint64_t sumG = stats->sumG_ - offset / 2; > - const uint64_t sumB = stats->sumB_ - offset / 4; > + const int64_t offset = blackLevel * nPixels; > + const int64_t minValid = 1; > + const uint64_t sumR = std::max(static_cast<int64_t>(stats->sumR_) - offset / 4, minValid); > + const uint64_t sumG = std::max(static_cast<int64_t>(stats->sumG_) - offset / 2, minValid); > + const uint64_t sumB = std::max(static_cast<int64_t>(stats->sumB_) - offset / 4, minValid); > > /* > * Calculate red and blue gains for AWB.
Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > Hi > > 2025. 09. 25. 21:28 keltezéssel, Milan Zamazal írta: >> The R/G/B sums computed in AWB simple IPA may be zero or perhaps even >> negative. Let's make sure the sums are always positive, to prevent >> division by zero or completely nonsense results. >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/ipa/simple/algorithms/awb.cpp | 10 ++++++---- >> 1 file changed, 6 insertions(+), 4 deletions(-) >> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp >> index cf567e894..8231a4968 100644 >> --- a/src/ipa/simple/algorithms/awb.cpp >> +++ b/src/ipa/simple/algorithms/awb.cpp >> @@ -7,6 +7,7 @@ >> #include "awb.h" >> +#include <algorithm> >> #include <numeric> >> #include <stdint.h> >> @@ -68,10 +69,11 @@ void Awb::process(IPAContext &context, >> */ >> const uint64_t nPixels = std::accumulate( >> histogram.begin(), histogram.end(), 0); > > An unrelated thing I noticed. I think the intention may have been to have something like > > const uint64_t nPixels = std::accumulate(histogram.begin(), histogram.end(), uint64_t(0)); > > because otherwise the summation will use the type of `0`, which is just `int`. Oh, thanks for catching this. >> - const uint64_t offset = blackLevel * nPixels; >> - const uint64_t sumR = stats->sumR_ - offset / 4; >> - const uint64_t sumG = stats->sumG_ - offset / 2; >> - const uint64_t sumB = stats->sumB_ - offset / 4; >> + const int64_t offset = blackLevel * nPixels; >> + const int64_t minValid = 1; >> + const uint64_t sumR = std::max(static_cast<int64_t>(stats->sumR_) - offset / 4, minValid); >> + const uint64_t sumG = std::max(static_cast<int64_t>(stats->sumG_) - offset / 2, minValid); >> + const uint64_t sumB = std::max(static_cast<int64_t>(stats->sumB_) - offset / 4, minValid); > > Have you seen this wrap-around in practice? No. But it could happen at least if the black level value was wrong. Or if the black level values were different for R, G, B. In which cases the computations here would be generally wrong but at least not completely crazy. Better to be on the safe side. > In any case, the checks looks reasonable, but I would potentially do > the following to avoid going through a singed type: > > sumR = stats->sumR_ > offset / 4 ? stats->sumR - offset / 4 : 1; OK. > Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > >> /* >> * Calculate red and blue gains for AWB.
diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp index cf567e894..8231a4968 100644 --- a/src/ipa/simple/algorithms/awb.cpp +++ b/src/ipa/simple/algorithms/awb.cpp @@ -7,6 +7,7 @@ #include "awb.h" +#include <algorithm> #include <numeric> #include <stdint.h> @@ -68,10 +69,11 @@ void Awb::process(IPAContext &context, */ const uint64_t nPixels = std::accumulate( histogram.begin(), histogram.end(), 0); - const uint64_t offset = blackLevel * nPixels; - const uint64_t sumR = stats->sumR_ - offset / 4; - const uint64_t sumG = stats->sumG_ - offset / 2; - const uint64_t sumB = stats->sumB_ - offset / 4; + const int64_t offset = blackLevel * nPixels; + const int64_t minValid = 1; + const uint64_t sumR = std::max(static_cast<int64_t>(stats->sumR_) - offset / 4, minValid); + const uint64_t sumG = std::max(static_cast<int64_t>(stats->sumG_) - offset / 2, minValid); + const uint64_t sumB = std::max(static_cast<int64_t>(stats->sumB_) - offset / 4, minValid); /* * Calculate red and blue gains for AWB.
The R/G/B sums computed in AWB simple IPA may be zero or perhaps even negative. Let's make sure the sums are always positive, to prevent division by zero or completely nonsense results. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- src/ipa/simple/algorithms/awb.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)