| Message ID | 20260817114349.994123-40-barnabas.pocze@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás On Mon, Aug 17, 2026 at 01:43:40PM +0200, Barnabás Pőcze wrote: > Instead of providing the black separately, adjust the luminance histogram > according to the current black level. Everything under the black level is > summed into the bin corresponding to the current black level, and the MSV > calculation is carried out on the partial histogram that starts at the > bin of the black level. > > This changes the behaviour slightly as previously everything under the > black level was ignored, and only the remaining part was split into the > 5 bins, but now the ignored bins are instead summed into the "first" bin > of the "new" histogram. I might be confused... First you say everything below the bin corresponding to the black level is ignored, but then you say that ignored bins are summed into the first valid bin... If you discard it, isn't the behaviour un-modified then ? > > This can result in the same image with the same statistics being perceived > as darker by the algorithm. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/ipa/simple/algorithms/agc.cpp | 23 +++++++++++++---------- > 1 file changed, 13 insertions(+), 10 deletions(-) > > diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp > index e24eacde72..a3e0ebbc3d 100644 > --- a/src/ipa/simple/algorithms/agc.cpp > +++ b/src/ipa/simple/algorithms/agc.cpp > @@ -66,19 +66,15 @@ static constexpr float kExpMaxStep = 0.15; > > namespace { > > -std::optional<float> calculateMSV(const Histogram &histogram, uint8_t blackLevel) > +std::optional<float> calculateMSV(const Histogram &histogram) > { > /* > * Calculate Mean Sample Value (MSV) according to formula from: > * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf > */ > - const unsigned int blackLevelHistIdx = > - blackLevel * histogram.bins() / 256; > - const unsigned int histogramSize = > - histogram.bins() - blackLevelHistIdx; > - const unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount; > + const unsigned int yHistValsPerBin = histogram.bins() / kExposureBinsCount; > const unsigned int yHistValsPerBinMod = > - histogramSize / (histogramSize % kExposureBinsCount + 1); > + histogram.bins() / (histogram.bins() % kExposureBinsCount + 1); > int exposureBins[kExposureBinsCount] = {}; > unsigned int denom = 0; > unsigned int num = 0; > @@ -86,9 +82,9 @@ std::optional<float> calculateMSV(const Histogram &histogram, uint8_t blackLevel > if (yHistValsPerBin == 0) > return {}; > > - for (unsigned int i = 0; i < histogramSize; i++) { > + for (unsigned int i = 0; i < histogram.bins(); i++) { > unsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin; > - exposureBins[idx] += histogram[blackLevelHistIdx + i]; > + exposureBins[idx] += histogram[i]; > } > > for (unsigned int i = 0; i < kExposureBinsCount; i++) { > @@ -193,7 +189,14 @@ void Agc::process(IPAContext &context, > return; > } > > - auto exposureMSV = calculateMSV({ stats->yHistogram }, context.activeState.blc.level); > + auto histogram = stats->yHistogram; > + const unsigned int blackLevelHistIdx = > + context.activeState.blc.level * std::size(histogram) / 256; > + > + for (unsigned int i = 0; i < blackLevelHistIdx; i++) > + histogram[blackLevelHistIdx] += histogram[i]; > + > + auto exposureMSV = calculateMSV({ { histogram.begin() + blackLevelHistIdx, histogram.end() } }); > if (!exposureMSV) { > LOG(IPASoftExposure, Debug) > << "Not adjusting exposure due to insufficient histogram data"; > -- > 2.55.0 >
2026. 08. 17. 17:23 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > > On Mon, Aug 17, 2026 at 01:43:40PM +0200, Barnabás Pőcze wrote: >> Instead of providing the black separately, adjust the luminance histogram >> according to the current black level. Everything under the black level is >> summed into the bin corresponding to the current black level, and the MSV >> calculation is carried out on the partial histogram that starts at the >> bin of the black level. >> >> This changes the behaviour slightly as previously everything under the >> black level was ignored, and only the remaining part was split into the >> 5 bins, but now the ignored bins are instead summed into the "first" bin >> of the "new" histogram. > > I might be confused... First you say everything below the bin > corresponding to the black level is ignored, but then you say that That is what it was previously. > ignored bins are summed into the first valid bin... If you discard it, > isn't the behaviour un-modified then ? But now they are not discarded, instead they are added to the first bin. > >> >> This can result in the same image with the same statistics being perceived >> as darker by the algorithm. >> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> Reviewed-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/ipa/simple/algorithms/agc.cpp | 23 +++++++++++++---------- >> 1 file changed, 13 insertions(+), 10 deletions(-) >> >> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp >> index e24eacde72..a3e0ebbc3d 100644 >> --- a/src/ipa/simple/algorithms/agc.cpp >> +++ b/src/ipa/simple/algorithms/agc.cpp >> @@ -66,19 +66,15 @@ static constexpr float kExpMaxStep = 0.15; >> >> namespace { >> >> -std::optional<float> calculateMSV(const Histogram &histogram, uint8_t blackLevel) >> +std::optional<float> calculateMSV(const Histogram &histogram) >> { >> /* >> * Calculate Mean Sample Value (MSV) according to formula from: >> * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf >> */ >> - const unsigned int blackLevelHistIdx = >> - blackLevel * histogram.bins() / 256; >> - const unsigned int histogramSize = >> - histogram.bins() - blackLevelHistIdx; >> - const unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount; >> + const unsigned int yHistValsPerBin = histogram.bins() / kExposureBinsCount; >> const unsigned int yHistValsPerBinMod = >> - histogramSize / (histogramSize % kExposureBinsCount + 1); >> + histogram.bins() / (histogram.bins() % kExposureBinsCount + 1); >> int exposureBins[kExposureBinsCount] = {}; >> unsigned int denom = 0; >> unsigned int num = 0; >> @@ -86,9 +82,9 @@ std::optional<float> calculateMSV(const Histogram &histogram, uint8_t blackLevel >> if (yHistValsPerBin == 0) >> return {}; >> >> - for (unsigned int i = 0; i < histogramSize; i++) { >> + for (unsigned int i = 0; i < histogram.bins(); i++) { >> unsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin; >> - exposureBins[idx] += histogram[blackLevelHistIdx + i]; >> + exposureBins[idx] += histogram[i]; >> } >> >> for (unsigned int i = 0; i < kExposureBinsCount; i++) { >> @@ -193,7 +189,14 @@ void Agc::process(IPAContext &context, >> return; >> } >> >> - auto exposureMSV = calculateMSV({ stats->yHistogram }, context.activeState.blc.level); >> + auto histogram = stats->yHistogram; >> + const unsigned int blackLevelHistIdx = >> + context.activeState.blc.level * std::size(histogram) / 256; >> + >> + for (unsigned int i = 0; i < blackLevelHistIdx; i++) >> + histogram[blackLevelHistIdx] += histogram[i]; >> + >> + auto exposureMSV = calculateMSV({ { histogram.begin() + blackLevelHistIdx, histogram.end() } }); >> if (!exposureMSV) { >> LOG(IPASoftExposure, Debug) >> << "Not adjusting exposure due to insufficient histogram data"; >> -- >> 2.55.0 >>
Hi Barnabás On Tue, Aug 18, 2026 at 02:49:06PM +0200, Barnabás Pőcze wrote: > 2026. 08. 17. 17:23 keltezéssel, Jacopo Mondi írta: > > Hi Barnabás > > > > On Mon, Aug 17, 2026 at 01:43:40PM +0200, Barnabás Pőcze wrote: > > > Instead of providing the black separately, adjust the luminance histogram > > > according to the current black level. Everything under the black level is > > > summed into the bin corresponding to the current black level, and the MSV > > > calculation is carried out on the partial histogram that starts at the > > > bin of the black level. > > > > > > This changes the behaviour slightly as previously everything under the > > > black level was ignored, and only the remaining part was split into the > > > 5 bins, but now the ignored bins are instead summed into the "first" bin > > > of the "new" histogram. > > > > I might be confused... First you say everything below the bin > > corresponding to the black level is ignored, but then you say that > > That is what it was previously. > > > > ignored bins are summed into the first valid bin... If you discard it, > > isn't the behaviour un-modified then ? > > But now they are not discarded, instead they are added to the first bin. > Sorry, my question was maybe not clear. Why modify the behaviour, can't you just ignore everything that was below the black level like it used to be ? Or is the change intentional ? Does it relate to the commit message on the next patch ? ------------------------------------------------------------------------------ This move also removes the dependency on the black level and makes it use the `Histogram` type instead of the software isp specific types. With the removal of the black level information, it is assumed to be 0 and a black level corrected histogram is expected. ------------------------------------------------------------------------------ > > > > > > > > This can result in the same image with the same statistics being perceived > > > as darker by the algorithm. > > > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > > > --- > > > src/ipa/simple/algorithms/agc.cpp | 23 +++++++++++++---------- > > > 1 file changed, 13 insertions(+), 10 deletions(-) > > > > > > diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp > > > index e24eacde72..a3e0ebbc3d 100644 > > > --- a/src/ipa/simple/algorithms/agc.cpp > > > +++ b/src/ipa/simple/algorithms/agc.cpp > > > @@ -66,19 +66,15 @@ static constexpr float kExpMaxStep = 0.15; > > > > > > namespace { > > > > > > -std::optional<float> calculateMSV(const Histogram &histogram, uint8_t blackLevel) > > > +std::optional<float> calculateMSV(const Histogram &histogram) > > > { > > > /* > > > * Calculate Mean Sample Value (MSV) according to formula from: > > > * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf > > > */ > > > - const unsigned int blackLevelHistIdx = > > > - blackLevel * histogram.bins() / 256; > > > - const unsigned int histogramSize = > > > - histogram.bins() - blackLevelHistIdx; > > > - const unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount; > > > + const unsigned int yHistValsPerBin = histogram.bins() / kExposureBinsCount; > > > const unsigned int yHistValsPerBinMod = > > > - histogramSize / (histogramSize % kExposureBinsCount + 1); > > > + histogram.bins() / (histogram.bins() % kExposureBinsCount + 1); > > > int exposureBins[kExposureBinsCount] = {}; > > > unsigned int denom = 0; > > > unsigned int num = 0; > > > @@ -86,9 +82,9 @@ std::optional<float> calculateMSV(const Histogram &histogram, uint8_t blackLevel > > > if (yHistValsPerBin == 0) > > > return {}; > > > > > > - for (unsigned int i = 0; i < histogramSize; i++) { > > > + for (unsigned int i = 0; i < histogram.bins(); i++) { > > > unsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin; > > > - exposureBins[idx] += histogram[blackLevelHistIdx + i]; > > > + exposureBins[idx] += histogram[i]; > > > } > > > > > > for (unsigned int i = 0; i < kExposureBinsCount; i++) { > > > @@ -193,7 +189,14 @@ void Agc::process(IPAContext &context, > > > return; > > > } > > > > > > - auto exposureMSV = calculateMSV({ stats->yHistogram }, context.activeState.blc.level); > > > + auto histogram = stats->yHistogram; > > > + const unsigned int blackLevelHistIdx = > > > + context.activeState.blc.level * std::size(histogram) / 256; > > > + > > > + for (unsigned int i = 0; i < blackLevelHistIdx; i++) > > > + histogram[blackLevelHistIdx] += histogram[i]; > > > + > > > + auto exposureMSV = calculateMSV({ { histogram.begin() + blackLevelHistIdx, histogram.end() } }); > > > if (!exposureMSV) { > > > LOG(IPASoftExposure, Debug) > > > << "Not adjusting exposure due to insufficient histogram data"; > > > -- > > > 2.55.0 > > > >
2026. 08. 19. 8:46 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > > On Tue, Aug 18, 2026 at 02:49:06PM +0200, Barnabás Pőcze wrote: >> 2026. 08. 17. 17:23 keltezéssel, Jacopo Mondi írta: >>> Hi Barnabás >>> >>> On Mon, Aug 17, 2026 at 01:43:40PM +0200, Barnabás Pőcze wrote: >>>> Instead of providing the black separately, adjust the luminance histogram >>>> according to the current black level. Everything under the black level is >>>> summed into the bin corresponding to the current black level, and the MSV >>>> calculation is carried out on the partial histogram that starts at the >>>> bin of the black level. >>>> >>>> This changes the behaviour slightly as previously everything under the >>>> black level was ignored, and only the remaining part was split into the >>>> 5 bins, but now the ignored bins are instead summed into the "first" bin >>>> of the "new" histogram. >>> >>> I might be confused... First you say everything below the bin >>> corresponding to the black level is ignored, but then you say that >> >> That is what it was previously. >> >> >>> ignored bins are summed into the first valid bin... If you discard it, >>> isn't the behaviour un-modified then ? >> >> But now they are not discarded, instead they are added to the first bin. >> > > Sorry, my question was maybe not clear. Why modify the behaviour, > can't you just ignore everything that was below the black level like > it used to be ? Or is the change intentional ? > > Does it relate to the commit message on the next patch ? Yes, although I have now removed that part from the commit message because it does not accurately reflect what is happening. > ------------------------------------------------------------------------------ > This move also removes the dependency on the black level and makes it > use the `Histogram` type instead of the software isp specific types. > With the removal of the black level information, it is assumed to be 0 > and a black level corrected histogram is expected. > ------------------------------------------------------------------------------ > The change is intentional. In a previous version it has been concluded that this is (hopefully) the correct thing to do: https://patchwork.libcamera.org/patch/27589/ And given that `AgcMeanLuminance` is later also provided this histogram, it seems like it's best if this is changed "now". > >>> >>>> >>>> This can result in the same image with the same statistics being perceived >>>> as darker by the algorithm. >>>> >>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>>> Reviewed-by: Milan Zamazal <mzamazal@redhat.com> >>>> --- >>>> src/ipa/simple/algorithms/agc.cpp | 23 +++++++++++++---------- >>>> 1 file changed, 13 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp >>>> index e24eacde72..a3e0ebbc3d 100644 >>>> --- a/src/ipa/simple/algorithms/agc.cpp >>>> +++ b/src/ipa/simple/algorithms/agc.cpp >>>> @@ -66,19 +66,15 @@ static constexpr float kExpMaxStep = 0.15; >>>> >>>> namespace { >>>> >>>> -std::optional<float> calculateMSV(const Histogram &histogram, uint8_t blackLevel) >>>> +std::optional<float> calculateMSV(const Histogram &histogram) >>>> { >>>> /* >>>> * Calculate Mean Sample Value (MSV) according to formula from: >>>> * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf >>>> */ >>>> - const unsigned int blackLevelHistIdx = >>>> - blackLevel * histogram.bins() / 256; >>>> - const unsigned int histogramSize = >>>> - histogram.bins() - blackLevelHistIdx; >>>> - const unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount; >>>> + const unsigned int yHistValsPerBin = histogram.bins() / kExposureBinsCount; >>>> const unsigned int yHistValsPerBinMod = >>>> - histogramSize / (histogramSize % kExposureBinsCount + 1); >>>> + histogram.bins() / (histogram.bins() % kExposureBinsCount + 1); >>>> int exposureBins[kExposureBinsCount] = {}; >>>> unsigned int denom = 0; >>>> unsigned int num = 0; >>>> @@ -86,9 +82,9 @@ std::optional<float> calculateMSV(const Histogram &histogram, uint8_t blackLevel >>>> if (yHistValsPerBin == 0) >>>> return {}; >>>> >>>> - for (unsigned int i = 0; i < histogramSize; i++) { >>>> + for (unsigned int i = 0; i < histogram.bins(); i++) { >>>> unsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin; >>>> - exposureBins[idx] += histogram[blackLevelHistIdx + i]; >>>> + exposureBins[idx] += histogram[i]; >>>> } >>>> >>>> for (unsigned int i = 0; i < kExposureBinsCount; i++) { >>>> @@ -193,7 +189,14 @@ void Agc::process(IPAContext &context, >>>> return; >>>> } >>>> >>>> - auto exposureMSV = calculateMSV({ stats->yHistogram }, context.activeState.blc.level); >>>> + auto histogram = stats->yHistogram; >>>> + const unsigned int blackLevelHistIdx = >>>> + context.activeState.blc.level * std::size(histogram) / 256; >>>> + >>>> + for (unsigned int i = 0; i < blackLevelHistIdx; i++) >>>> + histogram[blackLevelHistIdx] += histogram[i]; >>>> + >>>> + auto exposureMSV = calculateMSV({ { histogram.begin() + blackLevelHistIdx, histogram.end() } }); >>>> 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 e24eacde72..a3e0ebbc3d 100644 --- a/src/ipa/simple/algorithms/agc.cpp +++ b/src/ipa/simple/algorithms/agc.cpp @@ -66,19 +66,15 @@ static constexpr float kExpMaxStep = 0.15; namespace { -std::optional<float> calculateMSV(const Histogram &histogram, uint8_t blackLevel) +std::optional<float> calculateMSV(const Histogram &histogram) { /* * Calculate Mean Sample Value (MSV) according to formula from: * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf */ - const unsigned int blackLevelHistIdx = - blackLevel * histogram.bins() / 256; - const unsigned int histogramSize = - histogram.bins() - blackLevelHistIdx; - const unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount; + const unsigned int yHistValsPerBin = histogram.bins() / kExposureBinsCount; const unsigned int yHistValsPerBinMod = - histogramSize / (histogramSize % kExposureBinsCount + 1); + histogram.bins() / (histogram.bins() % kExposureBinsCount + 1); int exposureBins[kExposureBinsCount] = {}; unsigned int denom = 0; unsigned int num = 0; @@ -86,9 +82,9 @@ std::optional<float> calculateMSV(const Histogram &histogram, uint8_t blackLevel if (yHistValsPerBin == 0) return {}; - for (unsigned int i = 0; i < histogramSize; i++) { + for (unsigned int i = 0; i < histogram.bins(); i++) { unsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin; - exposureBins[idx] += histogram[blackLevelHistIdx + i]; + exposureBins[idx] += histogram[i]; } for (unsigned int i = 0; i < kExposureBinsCount; i++) { @@ -193,7 +189,14 @@ void Agc::process(IPAContext &context, return; } - auto exposureMSV = calculateMSV({ stats->yHistogram }, context.activeState.blc.level); + auto histogram = stats->yHistogram; + const unsigned int blackLevelHistIdx = + context.activeState.blc.level * std::size(histogram) / 256; + + for (unsigned int i = 0; i < blackLevelHistIdx; i++) + histogram[blackLevelHistIdx] += histogram[i]; + + auto exposureMSV = calculateMSV({ { histogram.begin() + blackLevelHistIdx, histogram.end() } }); if (!exposureMSV) { LOG(IPASoftExposure, Debug) << "Not adjusting exposure due to insufficient histogram data";