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

[Kieran Bingham]

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