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

[Robert Mader]

Turns out I have to ask for/suggest some more changes:

On 12.10.24 21:49, Robert Mader wrote:
> I gave this a try on a Pixel 3a (IMX 355 and IMX 363). In both cases 
> using 4096 made the resulting image fully black when covering the 
> sensor, which IIUC is the desired effect. Not putting any values or 
> using lower ones such as 3200 results in a greenish image - so there's 
> definitely an improvement here. Thus fell free to add a Tested-by: 
> Robert Mader <robert.mader at collabora.com>
>
> At least in the case of the IMX355 the output overall becomes much 
> darker - too dark IMO. Is that expected - and can/will it be 
> compensated by improvements of the AWB algorithm?
>
> P.S.: I guess I can submit a follow-up adding the black level for the 
> IMX355 - the IMX363 driver hasn't been submitted to upstream yet.
>
> On 11.10.24 17:39, Milan Zamazal wrote:
>> 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>
>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> ---
>>   src/ipa/simple/algorithms/blc.cpp | 6 +++++-
>>   src/ipa/simple/ipa_context.h      | 4 ++++
>>   src/ipa/simple/soft_simple.cpp    | 9 +++++++++
>>   3 files changed, 18 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;
>> +    } black;
>>   };
>>     struct IPAActiveState {
>> diff --git a/src/ipa/simple/soft_simple.cpp 
>> b/src/ipa/simple/soft_simple.cpp
>> index b28c7039f..079409f6d 100644
>> --- a/src/ipa/simple/soft_simple.cpp
>> +++ b/src/ipa/simple/soft_simple.cpp
>> @@ -201,6 +201,15 @@ int IPASoftSimple::configure(const IPAConfigInfo 
>> &configInfo)
>>               (context_.configuration.agc.againMax -
>>                context_.configuration.agc.againMin) /
>>               100.0;
>> +        if (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
>> +             * sensor pixel width. Hence we obtain the pixel-based 
>> black value
>> +             * by dividing the value from the helper by 256.
>> +             */
>> +            context_.configuration.black.level =
>> +                camHelper_->blackLevel().value() / 256;

Style nit: braces around this block would be good now that there's the 
comment.

More importantly though: Assuming setting a value for blackLevel_ 
without providing values for gain makes sense, then it would be great to 
adapt the code above to not assume so. It currently only works in builds 
with asserts disabled - which I used yesterday when trying it. There are 
two more similar places in processStats() (camHelper_ ? ...).

Forcing the non-cam-helper code for gain seems to fix the regression I 
mentioned in the previous mail.

>>       } else {
>>           /*
>>            * The camera sensor gain (g) is usually not equal to the 
>> value written
>
-- 
Robert Mader
Consultant Software Developer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
Registered in England & Wales, no. 5513718