[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:

> Hm, we are talking about https://patchwork.libcamera.org/patch/21865/,
> correct? 

Yes.

> It does not apply on master (a43ea7ff70e) here.

Oh, sorry, either `git rebase' is smarter or I resolved it in the
meantime.

Posted v2 rebased on master.

> On 02.12.24 12:00, Milan Zamazal wrote:
>> 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