[PATCH 1/1] libcamera: software_isp: Actually apply black level from tuning data

[Milan Zamazal]

Hi Robert,

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

> Sorry, turned out that I tested with https://patchwork.libcamera.org/patch/21703/ applied, which applies cleanly on master
> (unlike the patch here). And without that, i.e. on master, tuning file values don't get applied yet. So either of these
> patches is still needed.

Yes.  I think my patch is the right fix for the given bit.  It applies
cleanly on master.  Have you already tested it or would you like to test
it?

> On 30.11.24 11:01, Kieran Bingham wrote:
>> Quoting Robert Mader (2024-11-29 23:17:35)
>>> Wait, sorry, take that back. This commit is *not* needed any more.
>>> Blacklevels from tuning files now get applied on master.
>> Oh - ok - Milan, do you think this patch is still needed for any other
>> use case?
>>
>> Let me know if you think this should still be applied or dropped.
>>
>> --
>> Kieran
>>
>>> On 30.11.24 00:07, Robert Mader wrote:
>>>> Quickly retested on top of latest master, including "libcamera:
>>>> software_isp: Initialize exposure+gain before agc calculations"
>>>>
>>>> Tested-by: Robert Mader <robert.mader at collabora.com>
>>>>
>>>> On 08.11.24 10:43, Milan Zamazal wrote:
>>>>> The black level obtained from the tuning file in software ISP is
>>>>> retrieved in init (because this is the standard algorithm method with
>>>>> access to tuning data) and stored into context.  But the context gets
>>>>> reset in configure and the black level is lost and never applied.
>>>>>
>>>>> Let's store the black level from the tuning file into an algorithm
>>>>> instance variable and put it into the context only later in configure.
>>>>> This is similar to what rkisp1 IPA does with the values obtained from
>>>>> the tuning file.
>>>>>
>>>>> Fixes: 41e3d61c745153779ed5a38634d7266bffd60d61 ("libcamera:
>>>>> software_isp: Clear IPA context on configure and stop")
>>>>> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>>>>> ---
>>>>>    src/ipa/simple/algorithms/blc.cpp | 7 +++++--
>>>>>    src/ipa/simple/algorithms/blc.h   | 4 ++++
>>>>>    src/ipa/simple/soft_simple.cpp    | 3 +--
>>>>>    3 files changed, 10 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/src/ipa/simple/algorithms/blc.cpp
>>>>> b/src/ipa/simple/algorithms/blc.cpp
>>>>> index b4e32fe1c..1d7d370b5 100644
>>>>> --- a/src/ipa/simple/algorithms/blc.cpp
>>>>> +++ b/src/ipa/simple/algorithms/blc.cpp
>>>>> @@ -21,7 +21,8 @@ BlackLevel::BlackLevel()
>>>>>    {
>>>>>    }
>>>>>    -int BlackLevel::init(IPAContext &context, const YamlObject
>>>>> &tuningData)
>>>>> +int BlackLevel::init([[maybe_unused]] IPAContext &context,
>>>>> +             const YamlObject &tuningData)
>>>>>    {
>>>>>        auto blackLevel = tuningData["blackLevel"].get<int16_t>();
>>>>>        if (blackLevel.has_value()) {
>>>>> @@ -29,7 +30,7 @@ int BlackLevel::init(IPAContext &context, const
>>>>> YamlObject &tuningData)
>>>>>             * Convert 16 bit values from the tuning file to 8 bit black
>>>>>             * level for the SoftISP.
>>>>>             */
>>>>> -        context.configuration.black.level = blackLevel.value() >> 8;
>>>>> +        definedLevel_ = blackLevel.value() >> 8;
>>>>>        }
>>>>>        return 0;
>>>>>    }
>>>>> @@ -37,6 +38,8 @@ int BlackLevel::init(IPAContext &context, const
>>>>> YamlObject &tuningData)
>>>>>    int BlackLevel::configure(IPAContext &context,
>>>>>                  [[maybe_unused]] const IPAConfigInfo &configInfo)
>>>>>    {
>>>>> +    if (definedLevel_.has_value())
>>>>> +        context.configuration.black.level = definedLevel_;
>>>>>        context.activeState.blc.level =
>>>>>            context.configuration.black.level.value_or(255);
>>>>>        return 0;
>>>>> diff --git a/src/ipa/simple/algorithms/blc.h
>>>>> b/src/ipa/simple/algorithms/blc.h
>>>>> index 2cf2a8774..453123d27 100644
>>>>> --- a/src/ipa/simple/algorithms/blc.h
>>>>> +++ b/src/ipa/simple/algorithms/blc.h
>>>>> @@ -7,6 +7,9 @@
>>>>>      #pragma once
>>>>>    +#include <optional>
>>>>> +#include <stdint.h>
>>>>> +
>>>>>    #include "algorithm.h"
>>>>>      namespace libcamera {
>>>>> @@ -29,6 +32,7 @@ public:
>>>>>    private:
>>>>>        uint32_t exposure_;
>>>>>        double gain_;
>>>>> +    std::optional<uint8_t> definedLevel_;
>>>>>    };
>>>>>      } /* namespace ipa::soft::algorithms */
>>>>> diff --git a/src/ipa/simple/soft_simple.cpp
>>>>> b/src/ipa/simple/soft_simple.cpp
>>>>> index c8ad55a21..825c06757 100644
>>>>> --- a/src/ipa/simple/soft_simple.cpp
>>>>> +++ b/src/ipa/simple/soft_simple.cpp
>>>>> @@ -206,8 +206,7 @@ int IPASoftSimple::configure(const IPAConfigInfo
>>>>> &configInfo)
>>>>>                (context_.configuration.agc.againMax -
>>>>>                 context_.configuration.agc.againMin) /
>>>>>                100.0;
>>>>> -        if (!context_.configuration.black.level.has_value() &&
>>>>> -            camHelper_->blackLevel().has_value()) {
>>>>> +        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