[PATCH v5 2/2] libcamera: software_isp: Black level from tuning file

[Milan Zamazal]

Hi Kieran,

thank you for review.

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

> Quoting Milan Zamazal (2024-10-18 10:26:28)
>> This patch allows obtaining a black level from a tuning file in addition
>> to the camera sensor helper.  If both of them define a black level, the
>
>> one from the tuning file takes precedence.
>> 
>> The use cases are:
>> 
>> - A user wants to use a different black level, for whatever reason.
>> 
>> - There is a sensor without known gains but with a known black level.
>>   Because a camera sensor helper cannot be defined without specifying
>>   gains, the only way to specify the black level is using the tuning
>>   file.  Software ISP uses its fallback gain handling in such a case.
>> 
>> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> ---
>>  src/ipa/simple/algorithms/blc.cpp | 9 +++++++++
>>  src/ipa/simple/algorithms/blc.h   | 1 +
>>  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 a7af2e12c..8516c4335 100644
>> --- a/src/ipa/simple/algorithms/blc.cpp
>> +++ b/src/ipa/simple/algorithms/blc.cpp
>> @@ -21,11 +21,20 @@ BlackLevel::BlackLevel()
>>  {
>>  }
>>  
>> +int BlackLevel::init(IPAContext &context, const YamlObject &tuningData)
>> +{
>> +       auto blackLevel = tuningData["blackLevel"].get<int16_t>();
>> +       if (blackLevel.has_value())
>> +               context.configuration.black.level = blackLevel.value() / 256;
>
> It's not essential - but for converting between bit depths - I would
> think using bit shifting would be clearer (keep this however you prefer
> though).
>
> 	/*
> 	 * Convert 16 bit values from the tuning file to 8 bit black
> 	 * level for the SoftISP.
> 	 */
> 	.... = blackLevel.value() >> 8;
>
> might be more clear than
> 	.... = blackLevel.value() / 256;

OK.

>> +       return 0;
>> +}
>> +
>>  int BlackLevel::configure(IPAContext &context,
>>                           [[maybe_unused]] const IPAConfigInfo &configInfo)
>>  {
>>         context.activeState.blc.level =
>>                 context.configuration.black.level.value_or(255);
>> +       LOG(IPASoftBL, Info) << "pdm: blll: " << context.activeState.blc.level;
>
> What's pdm: blll? Should this be removed? or changed to be Debug level ?

A forgotten statement for checking the value, should be removed.

> For the update itself with those handled, I think having the Tuning File
> able to specify the black level is helpful:
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
>>         return 0;
>>  }
>>  
>> diff --git a/src/ipa/simple/algorithms/blc.h b/src/ipa/simple/algorithms/blc.h
>> index 828ad8b18..2cf2a8774 100644
>> --- a/src/ipa/simple/algorithms/blc.h
>> +++ b/src/ipa/simple/algorithms/blc.h
>> @@ -19,6 +19,7 @@ public:
>>         BlackLevel();
>>         ~BlackLevel() = default;
>>  
>> +       int init(IPAContext &context, const YamlObject &tuningData) override;
>>         int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
>>         void process(IPAContext &context, const uint32_t frame,
>>                      IPAFrameContext &frameContext,
>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
>> index e96e65bd1..03525b2f2 100644
>> --- a/src/ipa/simple/soft_simple.cpp
>> +++ b/src/ipa/simple/soft_simple.cpp
>> @@ -201,7 +201,8 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
>>                         (context_.configuration.agc.againMax -
>>                          context_.configuration.agc.againMin) /
>>                         100.0;
>> -               if (camHelper_->blackLevel().has_value()) {
>> +               if (!context_.configuration.black.level.has_value() &&
>> +                   camHelper_->blackLevel().has_value()) {
>>                         /*
>>                          * The black level from camHelper_ is a 16 bit value, software ISP
>>                          * works with 8 bit pixel values, both regardless of the actual
>> -- 
>> 2.44.1
>>