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

[Robert Mader]

Hm, we are talking about https://patchwork.libcamera.org/patch/21865/, 
correct? It does not apply on master (a43ea7ff70e) here.

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