| Message ID | 20241108094333.332643-2-mzamazal@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Quickly retested on top of latest master, including "libcamera: software_isp: Initialize exposure+gain before agc calculations" Tested-by: Robert Mader <robert.mader@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@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
Wait, sorry, take that back. This commit is *not* needed any more. Blacklevels from tuning files now get applied on master. 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@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@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
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@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@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
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. 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@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@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
Hi Robert, Robert Mader <robert.mader@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@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@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
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@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@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@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
Hi Robert, Robert Mader <robert.mader@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@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@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@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
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
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(-)