[v8,39/47] ipa: softisp: agc: Adjust histogram for black level

28071 diff mbox series

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.

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/softisp/algorithms/agc.cpp | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

Hi Barnabás,

Quoting Barnabás Pőcze (2026-08-24 11:13:58)
> 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 wrote the bottom of the email first, now coming back here. After
thinking a bit more about this I wonder if the old behavior was just
broken and you now fixed/improved it? If that is the case it should be
mentioned here.

> 
> 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/softisp/algorithms/agc.cpp | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/src/ipa/softisp/algorithms/agc.cpp b/src/ipa/softisp/algorithms/agc.cpp
> index 0027365b33..b56d57cfeb 100644
> --- a/src/ipa/softisp/algorithms/agc.cpp
> +++ b/src/ipa/softisp/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 std::nullopt;
>  
> -       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 * histogram.size() / 256;
> +
> +       for (unsigned int i = 0; i < blackLevelHistIdx; i++)
> +               histogram[blackLevelHistIdx] += histogram[i];

Actually I'm a bit concerned about this patch. I see that it was a
necessary workaround to get the blacklevel out of the algorithm before
moving it to libipa. My understanding is that before, we only looked at
a portion of the histogram (I don't exactly understand where the
compensation for not looking at the full histogram happens), now we move
the lower values into the blackLevelHistIdx bin.

I don't really understand how the old code worked as you now basically
do a histogram stretching and move everything below the blacklevel into
the first bin which seems to be hacky but not wrong either.

My concern is that this is structurally at the wrong place as imho the
histogram should be calculated without blacklevel and therefor be moved
into the stats generation process.

If I got that right we should at least add a big todo here that says
that this is a workaround and needs fixing. Otherwise we will run into
issues down the road when more algorithms try to make use of the
histogram.

With the todo added and the commit message adjusted:
Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>

Best regards,
Stefan

> +
> +       auto exposureMSV = calculateMSV({ { histogram.begin() + blackLevelHistIdx, histogram.end() } });
>         if (!exposureMSV) {
>                 LOG(IPASoftIspExposure, Debug)
>                         << "Not adjusting exposure due to insufficient histogram data";
> -- 
> 2.55.0
>

2026. 08. 26. 11:41 keltezéssel, Stefan Klug írta:
> Hi Barnabás,
> 
> Quoting Barnabás Pőcze (2026-08-24 11:13:58)
>> 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 wrote the bottom of the email first, now coming back here. After
> thinking a bit more about this I wonder if the old behavior was just
> broken and you now fixed/improved it? If that is the case it should be
> mentioned here.

I believe Milan and I agreed that this "new" behaviour better.


> 
>>
>> 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/softisp/algorithms/agc.cpp | 23 +++++++++++++----------
>>   1 file changed, 13 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/ipa/softisp/algorithms/agc.cpp b/src/ipa/softisp/algorithms/agc.cpp
>> index 0027365b33..b56d57cfeb 100644
>> --- a/src/ipa/softisp/algorithms/agc.cpp
>> +++ b/src/ipa/softisp/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 std::nullopt;
>>   
>> -       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 * histogram.size() / 256;
>> +
>> +       for (unsigned int i = 0; i < blackLevelHistIdx; i++)
>> +               histogram[blackLevelHistIdx] += histogram[i];
> 
> Actually I'm a bit concerned about this patch. I see that it was a
> necessary workaround to get the blacklevel out of the algorithm before
> moving it to libipa. My understanding is that before, we only looked at
> a portion of the histogram (I don't exactly understand where the
> compensation for not looking at the full histogram happens), now we move
> the lower values into the blackLevelHistIdx bin.
> 
> I don't really understand how the old code worked as you now basically
> do a histogram stretching and move everything below the blacklevel into
> the first bin which seems to be hacky but not wrong either.
> 
> My concern is that this is structurally at the wrong place as imho the
> histogram should be calculated without blacklevel and therefor be moved
> into the stats generation process.

I agree, and if you look at e.g. awb.cpp, you can see a "similar" adjustment
based on the black level.

It should probably be elsewhere, but given that there is blc.cpp, that tries to
"estimate" the black level (if not available) and that the whole param buffer
situation is a bit of a mess in the software-isp, it does not look trivial to me.

> 
> If I got that right we should at least add a big todo here that says
> that this is a workaround and needs fixing. Otherwise we will run into
> issues down the road when more algorithms try to make use of the
> histogram.
> 
> With the todo added and the commit message adjusted:

Done.


> Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> 
> Best regards,
> Stefan
> 
>> +
>> +       auto exposureMSV = calculateMSV({ { histogram.begin() + blackLevelHistIdx, histogram.end() } });
>>          if (!exposureMSV) {
>>                  LOG(IPASoftIspExposure, Debug)
>>                          << "Not adjusting exposure due to insufficient histogram data";
>> -- 
>> 2.55.0
>>