| Message ID | 20260803131435.153927-41-barnabas.pocze@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás, Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > Instead of providing the black separately, adjust the luminance histogram > according to the current black level. This changes the behaviour slightly > as previously everything under the black level was ignored, I think this should be indeed fixed. As I understand it, values below black level are likely to still mean "black" rather than something faulty. > and only the remaining part was split into the 5 bins, but now the > full histogram is split into the 5 bins. This doesn't look correct. The sensor range is black_level..max and this is what should be split between the 5 bins. If the whole histogram (0..max) is split instead then the higher bins get more values than they should, resulting in underexposure. As an extreme example, if black level was max*4/5 then only the last exposure bin would contain any values and the algorithm would always convert the exposure towards the minimum value eventually. > TODO: or should the previous behaviour be preserved and a smaller > histogram be used without adjustments? The smaller histogram should be used, but the values below the black level shouldn't be ignored, I think. > > TODO: is it `i < blackLevelHistIdx` or `i <= blackLevelHistIdx` ??? I think values at black_level are already valid values, so `i < blackLevelHistIdx' is correct. > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > src/ipa/simple/algorithms/agc.cpp | 24 ++++++++++++++---------- > 1 file changed, 14 insertions(+), 10 deletions(-) > > diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp > index cc33b269bf..199b444f5a 100644 > --- a/src/ipa/simple/algorithms/agc.cpp > +++ b/src/ipa/simple/algorithms/agc.cpp > @@ -11,6 +11,7 @@ > #include <cmath> > #include <optional> > #include <stdint.h> > +#include <utility> > > #include <libcamera/base/log.h> > > @@ -66,19 +67,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 +83,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 +190,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 = 1; i < blackLevelHistIdx; i++) > + histogram[0] += std::exchange(histogram[i], 0); Something like for (unsigned int i = 0; i < blackLevelHistIdx; i++) histogram[blackLevelHistIdx] += histogram[i]; would make more sense and then blackLevelHistIdx..size-1 should be split evenly among the 5 exposure bins. > + > + auto exposureMSV = calculateMSV({ histogram }); > if (!exposureMSV) { > LOG(IPASoftExposure, Debug) > << "Not adjusting exposure due to insufficient histogram data";
2026. 08. 04. 13:16 keltezéssel, Milan Zamazal írta: > Hi Barnabás, > > Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > >> Instead of providing the black separately, adjust the luminance histogram >> according to the current black level. This changes the behaviour slightly >> as previously everything under the black level was ignored, > > I think this should be indeed fixed. As I understand it, values below > black level are likely to still mean "black" rather than something > faulty. > >> and only the remaining part was split into the 5 bins, but now the >> full histogram is split into the 5 bins. > > This doesn't look correct. The sensor range is black_level..max and > this is what should be split between the 5 bins. If the whole histogram > (0..max) is split instead then the higher bins get more values than they > should, resulting in underexposure. Yes. I suppose that can be adjusted by moving the MSV target, no? In any case I found the linked paper a bit lacking in detail. For example, they simply state that Using MSV, the image is correctly exposed when μ ≈ 2.5 as in Figure 3. but I couldn't find any indication as to how they arrived at this value and what considerations went into it. Another thing that is confusing to me that a value of 2.5, given that there are 5 bins, "seems" like it should be like 50%. But that is not the case, because the MSV is in [1; 5], with a totally black image giving MSV=1 and a totally white image giving MSV=5, so MSV=2.5 is actually closer to MSV=1, than to MSV=5. Maybe it's intentional, but it definitely did not match my expectations (unless I made a mistake somewhere, that is). Anyways, this is more of just putting my thoughts in writing, not really a concrete question that I expect replies to. > > As an extreme example, if black level was max*4/5 then only the last > exposure bin would contain any values and the algorithm would always > convert the exposure towards the minimum value eventually. > >> TODO: or should the previous behaviour be preserved and a smaller >> histogram be used without adjustments? > > The smaller histogram should be used, but the values below the black > level shouldn't be ignored, I think. > >> >> TODO: is it `i < blackLevelHistIdx` or `i <= blackLevelHistIdx` ??? > > I think values at black_level are already valid values, so > `i < blackLevelHistIdx' is correct. > >> >> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> src/ipa/simple/algorithms/agc.cpp | 24 ++++++++++++++---------- >> 1 file changed, 14 insertions(+), 10 deletions(-) >> >> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp >> index cc33b269bf..199b444f5a 100644 >> --- a/src/ipa/simple/algorithms/agc.cpp >> +++ b/src/ipa/simple/algorithms/agc.cpp >> @@ -11,6 +11,7 @@ >> #include <cmath> >> #include <optional> >> #include <stdint.h> >> +#include <utility> >> >> #include <libcamera/base/log.h> >> >> @@ -66,19 +67,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 +83,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 +190,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 = 1; i < blackLevelHistIdx; i++) >> + histogram[0] += std::exchange(histogram[i], 0); > > Something like > > for (unsigned int i = 0; i < blackLevelHistIdx; i++) > histogram[blackLevelHistIdx] += histogram[i]; > > would make more sense and then blackLevelHistIdx..size-1 should be split > evenly among the 5 exposure bins. The three obvious options that I can see: (1) ignore values under black level (smaller histogram) (current) (2) consider values under black level to be 0 (full histogram) (proposed here) (3) consider values under black level to be black level (smaller histogram) (your proposal) I will switch to (3). > >> + >> + auto exposureMSV = calculateMSV({ histogram }); >> if (!exposureMSV) { >> LOG(IPASoftExposure, Debug) >> << "Not adjusting exposure due to insufficient histogram data"; >
Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > 2026. 08. 04. 13:16 keltezéssel, Milan Zamazal írta: >> Hi Barnabás, >> Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > >> >>> Instead of providing the black separately, adjust the luminance histogram >>> according to the current black level. This changes the behaviour slightly >>> as previously everything under the black level was ignored, >> I think this should be indeed fixed. As I understand it, values below >> black level are likely to still mean "black" rather than something >> faulty. >> >>> and only the remaining part was split into the 5 bins, but now the >>> full histogram is split into the 5 bins. >> This doesn't look correct. The sensor range is black_level..max and >> this is what should be split between the 5 bins. If the whole histogram >> (0..max) is split instead then the higher bins get more values than they >> should, resulting in underexposure. > > Yes. I suppose that can be adjusted by moving the MSV target, no? But even then the fact that the bins wouldn't be spread evenly across the expected values indicates possible trouble. > In any case I found the linked paper a bit lacking in detail. For > example, they simply state that > > Using MSV, the image is correctly exposed when μ ≈ 2.5 as in Figure 3. > > but I couldn't find any indication as to how they arrived at this value > and what considerations went into it. > > Another thing that is confusing to me that a value of 2.5, given that there > are 5 bins, "seems" like it should be like 50%. But that is not the case, > because the MSV is in [1; 5], with a totally black image giving MSV=1 and > a totally white image giving MSV=5, so MSV=2.5 is actually closer to MSV=1, > than to MSV=5. > > Maybe it's intentional, Apparently yes, see Figure 3. I suppose it's before gamma, so the "middle" luminance is below linear 50% middle. But yes, it's non-obvious why ~2.5 is the target value. > but it definitely did not match my expectations (unless I made a > mistake somewhere, that is). Anyways, this is more of just putting my > thoughts in writing, not really a concrete question that I expect > replies to. > > >> As an extreme example, if black level was max*4/5 then only the last >> exposure bin would contain any values and the algorithm would always >> convert the exposure towards the minimum value eventually. >> >>> TODO: or should the previous behaviour be preserved and a smaller >>> histogram be used without adjustments? >> The smaller histogram should be used, but the values below the black >> level shouldn't be ignored, I think. >> >>> >>> TODO: is it `i < blackLevelHistIdx` or `i <= blackLevelHistIdx` ??? >> I think values at black_level are already valid values, so >> `i < blackLevelHistIdx' is correct. >> >>> >>> >>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>> --- >>> src/ipa/simple/algorithms/agc.cpp | 24 ++++++++++++++---------- >>> 1 file changed, 14 insertions(+), 10 deletions(-) >>> >>> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp >>> index cc33b269bf..199b444f5a 100644 >>> --- a/src/ipa/simple/algorithms/agc.cpp >>> +++ b/src/ipa/simple/algorithms/agc.cpp >>> @@ -11,6 +11,7 @@ >>> #include <cmath> >>> #include <optional> >>> #include <stdint.h> >>> +#include <utility> >>> #include <libcamera/base/log.h> >>> @@ -66,19 +67,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 +83,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 +190,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 = 1; i < blackLevelHistIdx; i++) >>> + histogram[0] += std::exchange(histogram[i], 0); >> Something like >> for (unsigned int i = 0; i < blackLevelHistIdx; i++) >> histogram[blackLevelHistIdx] += histogram[i]; >> would make more sense and then blackLevelHistIdx..size-1 should be split >> evenly among the 5 exposure bins. > > The three obvious options that I can see: > > (1) ignore values under black level (smaller histogram) (current) > (2) consider values under black level to be 0 (full histogram) (proposed here) > (3) consider values under black level to be black level (smaller histogram) (your proposal) > > I will switch to (3). Yes, this should be the right choice. >>> + >>> + auto exposureMSV = calculateMSV({ histogram }); >>> if (!exposureMSV) { >>> LOG(IPASoftExposure, Debug) >>> << "Not adjusting exposure due to insufficient histogram data"; >>
diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp index cc33b269bf..199b444f5a 100644 --- a/src/ipa/simple/algorithms/agc.cpp +++ b/src/ipa/simple/algorithms/agc.cpp @@ -11,6 +11,7 @@ #include <cmath> #include <optional> #include <stdint.h> +#include <utility> #include <libcamera/base/log.h> @@ -66,19 +67,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 +83,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 +190,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 = 1; i < blackLevelHistIdx; i++) + histogram[0] += std::exchange(histogram[i], 0); + + auto exposureMSV = calculateMSV({ histogram }); if (!exposureMSV) { LOG(IPASoftExposure, Debug) << "Not adjusting exposure due to insufficient histogram data";
Instead of providing the black separately, adjust the luminance histogram according to the current 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 full histogram is split into the 5 bins. TODO: or should the previous behaviour be preserved and a smaller histogram be used without adjustments? TODO: is it `i < blackLevelHistIdx` or `i <= blackLevelHistIdx` ??? Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- src/ipa/simple/algorithms/agc.cpp | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)