| Message ID | 20241203093814.67984-2-mzamazal@redhat.com |
|---|---|
| State | Accepted |
| Commit | 2a9b0b34f8cb2295cd41a5340a9d26231dcd3e4b |
| Headers | show |
| Series |
|
| Related | show |
Tested-by: Robert Mader <robert.mader@collabora.com> On 03.12.24 10:38, 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@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 b4e32fe1..1d7d370b 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 67c688ae..52d59cab 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: > int32_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 ba3d5265..e1b6d3af 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
Robert Mader <robert.mader@collabora.com> writes: > Tested-by: Robert Mader <robert.mader@collabora.com> Thanks for testing! > On 03.12.24 10:38, 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@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 b4e32fe1..1d7d370b 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 67c688ae..52d59cab 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: >> int32_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 ba3d5265..e1b6d3af 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
On Tue, Dec 03, 2024 at 10:38:13AM +0100, 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@redhat.com> Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.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 b4e32fe1..1d7d370b 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 67c688ae..52d59cab 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: > int32_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 ba3d5265..e1b6d3af 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 > -- > 2.44.2 >
diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp index b4e32fe1..1d7d370b 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 67c688ae..52d59cab 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: int32_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 ba3d5265..e1b6d3af 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
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@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(-)