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

[Milan Zamazal]

Hi Robert,

thank you for testing and review.

Robert Mader <robert.mader at collabora.com> writes:

> 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. 

Yes.

>> 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?

No.  Blacks and shadows should be darker but overall the image should
have about the same brightness.  This is handled by auto-exposure, which
is applied after subtracting the black level.

>> 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.

OK.

> 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.

I'm not sure I understand what you mean exactly.  What the patch does is
that if a camera sensor helper is available for the given sensor and it
provides black level then that black level is used; otherwise black
level auto-detection is used (as before).  If there is no helper for the
given sensor, nothing changes.  Do you mean there is some trouble in the
current code in such a case (it seems to work fine for me)?  Or do you
talk about a situation when the helper provides black level but not
the corresponding gains?  And what assertion fails?

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