[PATCH v2 1/1] libcamera: software_isp: Get black level from the camera helper

[Milan Zamazal]

Kieran Bingham <kieran.bingham at ideasonboard.com> writes:

> Quoting Milan Zamazal (2024-10-09 20:19:33)
>> The black level in software ISP is unconditionally guessed from the
>> obtained frames.  CameraSensorHelper optionally provides the black level
>
>> from camera specifications now.  Let's use the value if available.
>> 
>> If the black level is not available from the given CameraSensorHelper
>> instance, it's still determined on the fly.
>> 
>> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> ---
>>  src/ipa/simple/algorithms/blc.cpp | 6 +++++-
>>  src/ipa/simple/ipa_context.h      | 4 ++++
>>  src/ipa/simple/soft_simple.cpp    | 3 +++
>>  3 files changed, 12 insertions(+), 1 deletion(-)
>> 
>> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
>> index b9f2aaa6d..a7af2e12c 100644
>> --- a/src/ipa/simple/algorithms/blc.cpp
>> +++ b/src/ipa/simple/algorithms/blc.cpp
>> @@ -24,7 +24,8 @@ BlackLevel::BlackLevel()
>>  int BlackLevel::configure(IPAContext &context,
>>                           [[maybe_unused]] const IPAConfigInfo &configInfo)
>>  {
>> -       context.activeState.blc.level = 255;
>> +       context.activeState.blc.level =
>> +               context.configuration.black.level.value_or(255);
>>         return 0;
>>  }
>>  
>> @@ -34,6 +35,9 @@ void BlackLevel::process(IPAContext &context,
>>                          const SwIspStats *stats,
>>                          [[maybe_unused]] ControlList &metadata)
>>  {
>> +       if (context.configuration.black.level.has_value())
>> +               return;
>> +
>>         if (frameContext.sensor.exposure == exposure_ &&
>>             frameContext.sensor.gain == gain_) {
>>                 return;
>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
>> index 3519f20f6..fd121eebe 100644
>> --- a/src/ipa/simple/ipa_context.h
>> +++ b/src/ipa/simple/ipa_context.h
>> @@ -8,6 +8,7 @@
>>  #pragma once
>>  
>>  #include <array>
>> +#include <optional>
>>  #include <stdint.h>
>>  
>>  #include <libipa/fc_queue.h>
>> @@ -22,6 +23,9 @@ struct IPASessionConfiguration {
>>                 int32_t exposureMin, exposureMax;
>>                 double againMin, againMax, againMinStep;
>>         } agc;
>> +       struct {
>> +               std::optional<uint8_t> level;
>
> Maybe I asked this before. Is 8 bits enough ? I guess it is if you're
> dividing by 256 ... but I don't understand what happens on a 12 bit
> sensor here ...

Software ISP currently works with 8 bits everywhere.  The sensor pixels
are cut to 8bit if needed before processing, see e.g. https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/software_isp/debayer_cpu.cpp#n168.

This might change with GPU processing but in such a case all the
algorithms would have to be revised.

>> +       } black;
>>  };
>>  
>>  struct IPAActiveState {
>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
>> index b28c7039f..ad6334ff6 100644
>> --- a/src/ipa/simple/soft_simple.cpp
>> +++ b/src/ipa/simple/soft_simple.cpp
>> @@ -201,6 +201,9 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
>>                         (context_.configuration.agc.againMax -
>>                          context_.configuration.agc.againMin) /
>>                         100.0;
>> +               if (camHelper_->blackLevel().has_value())
>> +                       context_.configuration.black.level =
>> +                               camHelper_->blackLevel().value() / 256;
>
> Is 256 going to work for all sensor modes here? What happens if a sensor
> supports 8, 10, and 12 bits per pixel?

The black level from CameraSensorHelper is a 16 bit value
(https://libcamera.org/internal-api-html/classlibcamera_1_1ipa_1_1CameraSensorHelper.html#ad6d462fc5828e841563289eef22d3174)
and software ISP works with 8 bit values regardless of the number of
sensor pixel bits.  That means all the values are sensor pixel width
independent.

>>         } else {
>>                 /*
>>                  * The camera sensor gain (g) is usually not equal to the value written
>> -- 
>> 2.44.1
>>