| Message ID | 20260817114349.994123-37-barnabas.pocze@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
On Mon, Aug 17, 2026 at 01:43:37PM +0200, Barnabás Pőcze wrote: > Add a separate function that does the MSV calculation. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/ipa/simple/algorithms/agc.cpp | 72 ++++++++++++++++++------------- > 1 file changed, 42 insertions(+), 30 deletions(-) > > diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp > index 93505f7665..f959abeba5 100644 > --- a/src/ipa/simple/algorithms/agc.cpp > +++ b/src/ipa/simple/algorithms/agc.cpp > @@ -9,6 +9,7 @@ > > #include <algorithm> > #include <cmath> > +#include <optional> > #include <stdint.h> > > #include <libcamera/base/log.h> > @@ -61,6 +62,44 @@ static constexpr float kExpProportionalGain = 0.04; > */ > static constexpr float kExpMaxStep = 0.15; > > +namespace { > + > +std::optional<float> calculateMSV(const SwIspStats::Histogram &histogram, uint8_t blackLevel) > +{ > + /* > + * Calculate Mean Sample Value (MSV) according to formula from: > + * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf > + */ > + const unsigned int blackLevelHistIdx = > + blackLevel / (256 / SwIspStats::kYHistogramSize); > + const unsigned int histogramSize = > + SwIspStats::kYHistogramSize - blackLevelHistIdx; > + const unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount; > + const unsigned int yHistValsPerBinMod = > + histogramSize / (histogramSize % kExposureBinsCount + 1); > + int exposureBins[kExposureBinsCount] = {}; > + unsigned int denom = 0; > + unsigned int num = 0; > + > + if (yHistValsPerBin == 0) > + return {}; > + > + for (unsigned int i = 0; i < histogramSize; i++) { > + unsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin; > + exposureBins[idx] += histogram[blackLevelHistIdx + i]; > + } > + > + for (unsigned int i = 0; i < kExposureBinsCount; i++) { > + LOG(IPASoftExposure, Debug) << i << ": " << exposureBins[i]; > + denom += exposureBins[i]; > + num += exposureBins[i] * (i + 1); > + } > + > + return (denom == 0 ? 0 : static_cast<float>(num) / denom); > +} > + > +} /* namespace */ > + > Agc::Agc() > { > } > @@ -158,41 +197,14 @@ void Agc::process(IPAContext &context, > return; > } > > - /* > - * Calculate Mean Sample Value (MSV) according to formula from: > - * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf > - */ > - const auto &histogram = stats->yHistogram; > - const unsigned int blackLevelHistIdx = > - context.activeState.blc.level / (256 / SwIspStats::kYHistogramSize); > - const unsigned int histogramSize = > - SwIspStats::kYHistogramSize - blackLevelHistIdx; > - const unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount; > - const unsigned int yHistValsPerBinMod = > - histogramSize / (histogramSize % kExposureBinsCount + 1); > - int exposureBins[kExposureBinsCount] = {}; > - unsigned int denom = 0; > - unsigned int num = 0; > - > - if (yHistValsPerBin == 0) { > + auto exposureMSV = calculateMSV(stats->yHistogram, context.activeState.blc.level); > + if (!exposureMSV) { > LOG(IPASoftExposure, Debug) > << "Not adjusting exposure due to insufficient histogram data"; > return; > } > > - for (unsigned int i = 0; i < histogramSize; i++) { > - unsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin; > - exposureBins[idx] += histogram[blackLevelHistIdx + i]; > - } > - > - for (unsigned int i = 0; i < kExposureBinsCount; i++) { > - LOG(IPASoftExposure, Debug) << i << ": " << exposureBins[i]; > - denom += exposureBins[i]; > - num += exposureBins[i] * (i + 1); > - } > - > - float exposureMSV = (denom == 0 ? 0 : static_cast<float>(num) / denom); > - updateExposure(context, frameContext, exposureMSV); > + updateExposure(context, frameContext, *exposureMSV); Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > } > > REGISTER_IPA_ALGORITHM(Agc, "Agc") > -- > 2.55.0 >
Hi Barnabás, Quoting Barnabás Pőcze (2026-08-17 13:43:37) > Add a separate function that does the MSV calculation. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/ipa/simple/algorithms/agc.cpp | 72 ++++++++++++++++++------------- > 1 file changed, 42 insertions(+), 30 deletions(-) > > diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp > index 93505f7665..f959abeba5 100644 > --- a/src/ipa/simple/algorithms/agc.cpp > +++ b/src/ipa/simple/algorithms/agc.cpp > @@ -9,6 +9,7 @@ > > #include <algorithm> > #include <cmath> > +#include <optional> > #include <stdint.h> > > #include <libcamera/base/log.h> > @@ -61,6 +62,44 @@ static constexpr float kExpProportionalGain = 0.04; > */ > static constexpr float kExpMaxStep = 0.15; > > +namespace { > + > +std::optional<float> calculateMSV(const SwIspStats::Histogram &histogram, uint8_t blackLevel) > +{ > + /* > + * Calculate Mean Sample Value (MSV) according to formula from: > + * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf > + */ > + const unsigned int blackLevelHistIdx = > + blackLevel / (256 / SwIspStats::kYHistogramSize); > + const unsigned int histogramSize = > + SwIspStats::kYHistogramSize - blackLevelHistIdx; > + const unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount; > + const unsigned int yHistValsPerBinMod = > + histogramSize / (histogramSize % kExposureBinsCount + 1); > + int exposureBins[kExposureBinsCount] = {}; > + unsigned int denom = 0; > + unsigned int num = 0; > + > + if (yHistValsPerBin == 0) > + return {}; Nit: For easier readability I'd prefer a std::nullopt here. Otherwise looks good to me. Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> Best regards, Stefan > + > + for (unsigned int i = 0; i < histogramSize; i++) { > + unsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin; > + exposureBins[idx] += histogram[blackLevelHistIdx + i]; > + } > + > + for (unsigned int i = 0; i < kExposureBinsCount; i++) { > + LOG(IPASoftExposure, Debug) << i << ": " << exposureBins[i]; > + denom += exposureBins[i]; > + num += exposureBins[i] * (i + 1); > + } > + > + return (denom == 0 ? 0 : static_cast<float>(num) / denom); > +} > + > +} /* namespace */ > + > Agc::Agc() > { > } > @@ -158,41 +197,14 @@ void Agc::process(IPAContext &context, > return; > } > > - /* > - * Calculate Mean Sample Value (MSV) according to formula from: > - * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf > - */ > - const auto &histogram = stats->yHistogram; > - const unsigned int blackLevelHistIdx = > - context.activeState.blc.level / (256 / SwIspStats::kYHistogramSize); > - const unsigned int histogramSize = > - SwIspStats::kYHistogramSize - blackLevelHistIdx; > - const unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount; > - const unsigned int yHistValsPerBinMod = > - histogramSize / (histogramSize % kExposureBinsCount + 1); > - int exposureBins[kExposureBinsCount] = {}; > - unsigned int denom = 0; > - unsigned int num = 0; > - > - if (yHistValsPerBin == 0) { > + auto exposureMSV = calculateMSV(stats->yHistogram, context.activeState.blc.level); > + if (!exposureMSV) { > LOG(IPASoftExposure, Debug) > << "Not adjusting exposure due to insufficient histogram data"; > return; > } > > - for (unsigned int i = 0; i < histogramSize; i++) { > - unsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin; > - exposureBins[idx] += histogram[blackLevelHistIdx + i]; > - } > - > - for (unsigned int i = 0; i < kExposureBinsCount; i++) { > - LOG(IPASoftExposure, Debug) << i << ": " << exposureBins[i]; > - denom += exposureBins[i]; > - num += exposureBins[i] * (i + 1); > - } > - > - float exposureMSV = (denom == 0 ? 0 : static_cast<float>(num) / denom); > - updateExposure(context, frameContext, exposureMSV); > + updateExposure(context, frameContext, *exposureMSV); > } > > REGISTER_IPA_ALGORITHM(Agc, "Agc") > -- > 2.55.0 >
2026. 08. 20. 13:06 keltezéssel, Stefan Klug írta: > Hi Barnabás, > > Quoting Barnabás Pőcze (2026-08-17 13:43:37) >> Add a separate function that does the MSV calculation. >> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> Reviewed-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/ipa/simple/algorithms/agc.cpp | 72 ++++++++++++++++++------------- >> 1 file changed, 42 insertions(+), 30 deletions(-) >> >> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp >> index 93505f7665..f959abeba5 100644 >> --- a/src/ipa/simple/algorithms/agc.cpp >> +++ b/src/ipa/simple/algorithms/agc.cpp >> @@ -9,6 +9,7 @@ >> >> #include <algorithm> >> #include <cmath> >> +#include <optional> >> #include <stdint.h> >> >> #include <libcamera/base/log.h> >> @@ -61,6 +62,44 @@ static constexpr float kExpProportionalGain = 0.04; >> */ >> static constexpr float kExpMaxStep = 0.15; >> >> +namespace { >> + >> +std::optional<float> calculateMSV(const SwIspStats::Histogram &histogram, uint8_t blackLevel) >> +{ >> + /* >> + * Calculate Mean Sample Value (MSV) according to formula from: >> + * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf >> + */ >> + const unsigned int blackLevelHistIdx = >> + blackLevel / (256 / SwIspStats::kYHistogramSize); >> + const unsigned int histogramSize = >> + SwIspStats::kYHistogramSize - blackLevelHistIdx; >> + const unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount; >> + const unsigned int yHistValsPerBinMod = >> + histogramSize / (histogramSize % kExposureBinsCount + 1); >> + int exposureBins[kExposureBinsCount] = {}; >> + unsigned int denom = 0; >> + unsigned int num = 0; >> + >> + if (yHistValsPerBin == 0) >> + return {}; > > Nit: For easier readability I'd prefer a std::nullopt here. Okay, although I don't really see a substantial difference from the other numerous places where `{}` is returned. > > Otherwise looks good to me. > > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> > > Best regards, > Stefan > >> + >> + for (unsigned int i = 0; i < histogramSize; i++) { >> + unsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin; >> + exposureBins[idx] += histogram[blackLevelHistIdx + i]; >> + } >> + >> + for (unsigned int i = 0; i < kExposureBinsCount; i++) { >> + LOG(IPASoftExposure, Debug) << i << ": " << exposureBins[i]; >> + denom += exposureBins[i]; >> + num += exposureBins[i] * (i + 1); >> + } >> + >> + return (denom == 0 ? 0 : static_cast<float>(num) / denom); >> +} >> + >> +} /* namespace */ >> + >> Agc::Agc() >> { >> } >> @@ -158,41 +197,14 @@ void Agc::process(IPAContext &context, >> return; >> } >> >> - /* >> - * Calculate Mean Sample Value (MSV) according to formula from: >> - * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf >> - */ >> - const auto &histogram = stats->yHistogram; >> - const unsigned int blackLevelHistIdx = >> - context.activeState.blc.level / (256 / SwIspStats::kYHistogramSize); >> - const unsigned int histogramSize = >> - SwIspStats::kYHistogramSize - blackLevelHistIdx; >> - const unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount; >> - const unsigned int yHistValsPerBinMod = >> - histogramSize / (histogramSize % kExposureBinsCount + 1); >> - int exposureBins[kExposureBinsCount] = {}; >> - unsigned int denom = 0; >> - unsigned int num = 0; >> - >> - if (yHistValsPerBin == 0) { >> + auto exposureMSV = calculateMSV(stats->yHistogram, context.activeState.blc.level); >> + if (!exposureMSV) { >> LOG(IPASoftExposure, Debug) >> << "Not adjusting exposure due to insufficient histogram data"; >> return; >> } >> >> - for (unsigned int i = 0; i < histogramSize; i++) { >> - unsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin; >> - exposureBins[idx] += histogram[blackLevelHistIdx + i]; >> - } >> - >> - for (unsigned int i = 0; i < kExposureBinsCount; i++) { >> - LOG(IPASoftExposure, Debug) << i << ": " << exposureBins[i]; >> - denom += exposureBins[i]; >> - num += exposureBins[i] * (i + 1); >> - } >> - >> - float exposureMSV = (denom == 0 ? 0 : static_cast<float>(num) / denom); >> - updateExposure(context, frameContext, exposureMSV); >> + updateExposure(context, frameContext, *exposureMSV); >> } >> >> REGISTER_IPA_ALGORITHM(Agc, "Agc") >> -- >> 2.55.0 >>
Quoting Barnabás Pőcze (2026-08-20 13:24:13) > 2026. 08. 20. 13:06 keltezéssel, Stefan Klug írta: > > Hi Barnabás, > > > > Quoting Barnabás Pőcze (2026-08-17 13:43:37) > >> Add a separate function that does the MSV calculation. > >> > >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >> Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > >> --- > >> src/ipa/simple/algorithms/agc.cpp | 72 ++++++++++++++++++------------- > >> 1 file changed, 42 insertions(+), 30 deletions(-) > >> > >> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp > >> index 93505f7665..f959abeba5 100644 > >> --- a/src/ipa/simple/algorithms/agc.cpp > >> +++ b/src/ipa/simple/algorithms/agc.cpp > >> @@ -9,6 +9,7 @@ > >> > >> #include <algorithm> > >> #include <cmath> > >> +#include <optional> > >> #include <stdint.h> > >> > >> #include <libcamera/base/log.h> > >> @@ -61,6 +62,44 @@ static constexpr float kExpProportionalGain = 0.04; > >> */ > >> static constexpr float kExpMaxStep = 0.15; > >> > >> +namespace { > >> + > >> +std::optional<float> calculateMSV(const SwIspStats::Histogram &histogram, uint8_t blackLevel) > >> +{ > >> + /* > >> + * Calculate Mean Sample Value (MSV) according to formula from: > >> + * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf > >> + */ > >> + const unsigned int blackLevelHistIdx = > >> + blackLevel / (256 / SwIspStats::kYHistogramSize); > >> + const unsigned int histogramSize = > >> + SwIspStats::kYHistogramSize - blackLevelHistIdx; > >> + const unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount; > >> + const unsigned int yHistValsPerBinMod = > >> + histogramSize / (histogramSize % kExposureBinsCount + 1); > >> + int exposureBins[kExposureBinsCount] = {}; > >> + unsigned int denom = 0; > >> + unsigned int num = 0; > >> + > >> + if (yHistValsPerBin == 0) > >> + return {}; > > > > Nit: For easier readability I'd prefer a std::nullopt here. > > Okay, although I don't really see a substantial difference from > the other numerous places where `{}` is returned. This difference is that I just read that one and not the others :-) While reading I realized that my brain started to spend a few extra cycles to check what gets returned. This would not be required otherwise. No big deal. Do as you like. > > > > > > Otherwise looks good to me. The otherwise was maybe misleading. I didn't want to imply that above must be changed. Best regards, Stefan > > > > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > > Best regards, > > Stefan > > > >> + > >> + for (unsigned int i = 0; i < histogramSize; i++) { > >> + unsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin; > >> + exposureBins[idx] += histogram[blackLevelHistIdx + i]; > >> + } > >> + > >> + for (unsigned int i = 0; i < kExposureBinsCount; i++) { > >> + LOG(IPASoftExposure, Debug) << i << ": " << exposureBins[i]; > >> + denom += exposureBins[i]; > >> + num += exposureBins[i] * (i + 1); > >> + } > >> + > >> + return (denom == 0 ? 0 : static_cast<float>(num) / denom); > >> +} > >> + > >> +} /* namespace */ > >> + > >> Agc::Agc() > >> { > >> } > >> @@ -158,41 +197,14 @@ void Agc::process(IPAContext &context, > >> return; > >> } > >> > >> - /* > >> - * Calculate Mean Sample Value (MSV) according to formula from: > >> - * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf > >> - */ > >> - const auto &histogram = stats->yHistogram; > >> - const unsigned int blackLevelHistIdx = > >> - context.activeState.blc.level / (256 / SwIspStats::kYHistogramSize); > >> - const unsigned int histogramSize = > >> - SwIspStats::kYHistogramSize - blackLevelHistIdx; > >> - const unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount; > >> - const unsigned int yHistValsPerBinMod = > >> - histogramSize / (histogramSize % kExposureBinsCount + 1); > >> - int exposureBins[kExposureBinsCount] = {}; > >> - unsigned int denom = 0; > >> - unsigned int num = 0; > >> - > >> - if (yHistValsPerBin == 0) { > >> + auto exposureMSV = calculateMSV(stats->yHistogram, context.activeState.blc.level); > >> + if (!exposureMSV) { > >> LOG(IPASoftExposure, Debug) > >> << "Not adjusting exposure due to insufficient histogram data"; > >> return; > >> } > >> > >> - for (unsigned int i = 0; i < histogramSize; i++) { > >> - unsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin; > >> - exposureBins[idx] += histogram[blackLevelHistIdx + i]; > >> - } > >> - > >> - for (unsigned int i = 0; i < kExposureBinsCount; i++) { > >> - LOG(IPASoftExposure, Debug) << i << ": " << exposureBins[i]; > >> - denom += exposureBins[i]; > >> - num += exposureBins[i] * (i + 1); > >> - } > >> - > >> - float exposureMSV = (denom == 0 ? 0 : static_cast<float>(num) / denom); > >> - updateExposure(context, frameContext, exposureMSV); > >> + updateExposure(context, frameContext, *exposureMSV); > >> } > >> > >> REGISTER_IPA_ALGORITHM(Agc, "Agc") > >> -- > >> 2.55.0 > >> >
diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp index 93505f7665..f959abeba5 100644 --- a/src/ipa/simple/algorithms/agc.cpp +++ b/src/ipa/simple/algorithms/agc.cpp @@ -9,6 +9,7 @@ #include <algorithm> #include <cmath> +#include <optional> #include <stdint.h> #include <libcamera/base/log.h> @@ -61,6 +62,44 @@ static constexpr float kExpProportionalGain = 0.04; */ static constexpr float kExpMaxStep = 0.15; +namespace { + +std::optional<float> calculateMSV(const SwIspStats::Histogram &histogram, uint8_t blackLevel) +{ + /* + * Calculate Mean Sample Value (MSV) according to formula from: + * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf + */ + const unsigned int blackLevelHistIdx = + blackLevel / (256 / SwIspStats::kYHistogramSize); + const unsigned int histogramSize = + SwIspStats::kYHistogramSize - blackLevelHistIdx; + const unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount; + const unsigned int yHistValsPerBinMod = + histogramSize / (histogramSize % kExposureBinsCount + 1); + int exposureBins[kExposureBinsCount] = {}; + unsigned int denom = 0; + unsigned int num = 0; + + if (yHistValsPerBin == 0) + return {}; + + for (unsigned int i = 0; i < histogramSize; i++) { + unsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin; + exposureBins[idx] += histogram[blackLevelHistIdx + i]; + } + + for (unsigned int i = 0; i < kExposureBinsCount; i++) { + LOG(IPASoftExposure, Debug) << i << ": " << exposureBins[i]; + denom += exposureBins[i]; + num += exposureBins[i] * (i + 1); + } + + return (denom == 0 ? 0 : static_cast<float>(num) / denom); +} + +} /* namespace */ + Agc::Agc() { } @@ -158,41 +197,14 @@ void Agc::process(IPAContext &context, return; } - /* - * Calculate Mean Sample Value (MSV) according to formula from: - * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf - */ - const auto &histogram = stats->yHistogram; - const unsigned int blackLevelHistIdx = - context.activeState.blc.level / (256 / SwIspStats::kYHistogramSize); - const unsigned int histogramSize = - SwIspStats::kYHistogramSize - blackLevelHistIdx; - const unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount; - const unsigned int yHistValsPerBinMod = - histogramSize / (histogramSize % kExposureBinsCount + 1); - int exposureBins[kExposureBinsCount] = {}; - unsigned int denom = 0; - unsigned int num = 0; - - if (yHistValsPerBin == 0) { + auto exposureMSV = calculateMSV(stats->yHistogram, context.activeState.blc.level); + if (!exposureMSV) { LOG(IPASoftExposure, Debug) << "Not adjusting exposure due to insufficient histogram data"; return; } - for (unsigned int i = 0; i < histogramSize; i++) { - unsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin; - exposureBins[idx] += histogram[blackLevelHistIdx + i]; - } - - for (unsigned int i = 0; i < kExposureBinsCount; i++) { - LOG(IPASoftExposure, Debug) << i << ": " << exposureBins[i]; - denom += exposureBins[i]; - num += exposureBins[i] * (i + 1); - } - - float exposureMSV = (denom == 0 ? 0 : static_cast<float>(num) / denom); - updateExposure(context, frameContext, exposureMSV); + updateExposure(context, frameContext, *exposureMSV); } REGISTER_IPA_ALGORITHM(Agc, "Agc")