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

[Milan Zamazal]

Hi Kieran,

thank you for review.

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

> Quoting Milan Zamazal (2024-08-15 09:37:16)
>> 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.
>> 
>> The black level value initialization is moved from init to configure
>> because this is where we get the other configuration (gains) from
>> CameraSensorHelper in software ISP.
>> 
>> 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 | 10 +++++++---
>>  src/ipa/simple/algorithms/blc.h   |  3 ++-
>>  src/ipa/simple/ipa_context.h      |  3 ++-
>>  src/ipa/simple/soft_simple.cpp    |  3 +++
>>  4 files changed, 14 insertions(+), 5 deletions(-)
>> 
>> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
>> index d01c2c14..5dc924bb 100644
>> --- a/src/ipa/simple/algorithms/blc.cpp
>> +++ b/src/ipa/simple/algorithms/blc.cpp
>> @@ -21,10 +21,11 @@ BlackLevel::BlackLevel()
>>  {
>>  }
>>  
>> -int BlackLevel::init(IPAContext &context,
>> -                    [[maybe_unused]] const YamlObject &tuningData)
>> +int BlackLevel::configure(typename Module::Context &context,
>> +                         [[maybe_unused]] const typename Module::Config &configInfo)
>>  {
>> -       context.activeState.black.level = 1.0;
>> +       context.activeState.black.level =
>> +               context.configuration.black.level.value_or(1.0);
>>         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/algorithms/blc.h b/src/ipa/simple/algorithms/blc.h
>> index 9b9daab1..586a23d2 100644
>> --- a/src/ipa/simple/algorithms/blc.h
>> +++ b/src/ipa/simple/algorithms/blc.h
>> @@ -24,7 +24,8 @@ public:
>>         BlackLevel();
>>         ~BlackLevel() = default;
>>  
>> -       int init(IPAContext &context, const YamlObject &tuningData)
>> +       int configure(typename Module::Context &context,
>> +                     const typename Module::Config &configInfo)
>>                 override;
>>         void process(IPAContext &context, const uint32_t frame,
>>                      IPAFrameContext &frameContext,
>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
>> index 0e2096f8..08f965f4 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 <libcamera/controls.h>
>>  
>> @@ -24,7 +25,7 @@ struct IPASessionConfiguration {
>>                 double againMin, againMax, againMinStep;
>>         } agc;
>>         struct {
>> -               double level;
>> +               std::optional<double> level;
>>         } black;
>>  };
>>  
>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
>> index aef5f6e5..ac7a22b7 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() / 65536.0;
>
> Shouldn't this scale the value depending on the cameras configured bit
> depth or such for when you use it in your calculations on pixel values?
>
> Why 65536? 

CameraSensorHelper::blackLevel() docstring says:

  This function returns the black level of the sensor scaled to a 16bit
  pixel width.

> I've recently just pushed the IMX283 black level value as:
>
>> +		/* From datasheet: 0x32 at 10bits. */
>> +		blackLevel_ = 3200;

Which is in accordance with that docstring:
50 (== 0x32) * 64 (== 2^(16-10)) = 3200

> Do you treat black level as value between 0 and 1 ?

Yes, this is what it is expected to be after the current software ISP
refactoring patches, but it may change based on the final version.

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