| Message ID | 20260810103846.1075936-40-barnabas.pocze@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás On Mon, Aug 10, 2026 at 12:38:35PM +0200, Barnabás Pőcze wrote: > Use the generic `Histogram` type. This is needed to decouple the > implementation from the software isp, in order to make this usable > by other components. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > Reviewed-by: Milan Zamazal <mzamazal@redhat.com> It looks like this is almost a search and replace. I wonder what is the value of SwIspStats::Histogram. Do you happen to know why it was introduced instead of using the library Histogram class ? > --- > src/ipa/simple/algorithms/agc.cpp | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp > index 5307fda953..e24eacde72 100644 > --- a/src/ipa/simple/algorithms/agc.cpp > +++ b/src/ipa/simple/algorithms/agc.cpp > @@ -14,6 +14,8 @@ > > #include <libcamera/base/log.h> > > +#include <libipa/histogram.h> > + > #include "control_ids.h" > > namespace libcamera { > @@ -64,16 +66,16 @@ static constexpr float kExpMaxStep = 0.15; > > namespace { > > -std::optional<float> calculateMSV(const SwIspStats::Histogram &histogram, uint8_t blackLevel) > +std::optional<float> calculateMSV(const 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); > + blackLevel * histogram.bins() / 256; > const unsigned int histogramSize = > - SwIspStats::kYHistogramSize - blackLevelHistIdx; > + histogram.bins() - blackLevelHistIdx; > const unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount; > const unsigned int yHistValsPerBinMod = > histogramSize / (histogramSize % kExposureBinsCount + 1); > @@ -191,7 +193,7 @@ void Agc::process(IPAContext &context, > return; > } > > - auto exposureMSV = calculateMSV(stats->yHistogram, context.activeState.blc.level); > + auto exposureMSV = calculateMSV({ stats->yHistogram }, context.activeState.blc.level); > if (!exposureMSV) { > LOG(IPASoftExposure, Debug) > << "Not adjusting exposure due to insufficient histogram data"; > -- > 2.55.0 >
2026. 08. 10. 16:04 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > > On Mon, Aug 10, 2026 at 12:38:35PM +0200, Barnabás Pőcze wrote: >> Use the generic `Histogram` type. This is needed to decouple the >> implementation from the software isp, in order to make this usable >> by other components. >> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > > It looks like this is almost a search and replace. I wonder what is > the value of SwIspStats::Histogram. > > Do you happen to know why it was introduced instead of using the > library Histogram class ? The `SwIspStats` object is in shared memory, so having it be a statically sized array makes most sense. > >> --- >> src/ipa/simple/algorithms/agc.cpp | 10 ++++++---- >> 1 file changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp >> index 5307fda953..e24eacde72 100644 >> --- a/src/ipa/simple/algorithms/agc.cpp >> +++ b/src/ipa/simple/algorithms/agc.cpp >> @@ -14,6 +14,8 @@ >> >> #include <libcamera/base/log.h> >> >> +#include <libipa/histogram.h> >> + >> #include "control_ids.h" >> >> namespace libcamera { >> @@ -64,16 +66,16 @@ static constexpr float kExpMaxStep = 0.15; >> >> namespace { >> >> -std::optional<float> calculateMSV(const SwIspStats::Histogram &histogram, uint8_t blackLevel) >> +std::optional<float> calculateMSV(const 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); >> + blackLevel * histogram.bins() / 256; >> const unsigned int histogramSize = >> - SwIspStats::kYHistogramSize - blackLevelHistIdx; >> + histogram.bins() - blackLevelHistIdx; >> const unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount; >> const unsigned int yHistValsPerBinMod = >> histogramSize / (histogramSize % kExposureBinsCount + 1); >> @@ -191,7 +193,7 @@ void Agc::process(IPAContext &context, >> return; >> } >> >> - auto exposureMSV = calculateMSV(stats->yHistogram, context.activeState.blc.level); >> + auto exposureMSV = calculateMSV({ stats->yHistogram }, context.activeState.blc.level); >> if (!exposureMSV) { >> LOG(IPASoftExposure, Debug) >> << "Not adjusting exposure due to insufficient histogram data"; >> -- >> 2.55.0 >>
diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp index 5307fda953..e24eacde72 100644 --- a/src/ipa/simple/algorithms/agc.cpp +++ b/src/ipa/simple/algorithms/agc.cpp @@ -14,6 +14,8 @@ #include <libcamera/base/log.h> +#include <libipa/histogram.h> + #include "control_ids.h" namespace libcamera { @@ -64,16 +66,16 @@ static constexpr float kExpMaxStep = 0.15; namespace { -std::optional<float> calculateMSV(const SwIspStats::Histogram &histogram, uint8_t blackLevel) +std::optional<float> calculateMSV(const 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); + blackLevel * histogram.bins() / 256; const unsigned int histogramSize = - SwIspStats::kYHistogramSize - blackLevelHistIdx; + histogram.bins() - blackLevelHistIdx; const unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount; const unsigned int yHistValsPerBinMod = histogramSize / (histogramSize % kExposureBinsCount + 1); @@ -191,7 +193,7 @@ void Agc::process(IPAContext &context, return; } - auto exposureMSV = calculateMSV(stats->yHistogram, context.activeState.blc.level); + auto exposureMSV = calculateMSV({ stats->yHistogram }, context.activeState.blc.level); if (!exposureMSV) { LOG(IPASoftExposure, Debug) << "Not adjusting exposure due to insufficient histogram data";