[PATCH 1/1] libcamera: software_isp: Actually apply black level from tuning data
Kieran Bingham
kieran.bingham at ideasonboard.com
Sat Nov 30 11:01:57 CET 2024
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
More information about the libcamera-devel
mailing list