| Message ID | 20241013120450.149837-1-robert.mader@collabora.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Robert, On 13/10/24 5:34 pm, Robert Mader wrote: > Like the hardware pipelines do. Otherwise we might hit the following > assert: "Frame context for ... has been overwritten by ...". Would you like to mention here briefly, on what use-case you hit this assertion. > > Fixes: 04d171e6 ("libcamera: software_isp: Call Algorithm::queueRequest") > Signed-off-by: Robert Mader <robert.mader@collabora.com> > --- > src/ipa/simple/soft_simple.cpp | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp > index b28c7039..ac8847cb 100644 > --- a/src/ipa/simple/soft_simple.cpp > +++ b/src/ipa/simple/soft_simple.cpp > @@ -245,6 +245,7 @@ int IPASoftSimple::start() > > void IPASoftSimple::stop() > { > + context_.frameContexts.clear(); Looks good to me but additional hardware pipelines also reset/clear at configure() time, for e.g. /* Clear the IPA context before the streaming session. */ context_.configuration = {}; context_.activeState = {}; context_.frameContexts.clear(); Would it make sense to do it in Soft IPA as well?
On 13.10.24 17:50, Umang Jain wrote: > Hi Robert, > > On 13/10/24 5:34 pm, Robert Mader wrote: >> Like the hardware pipelines do. Otherwise we might hit the following >> assert: "Frame context for ... has been overwritten by ...". > > Would you like to mention here briefly, on what use-case you hit this > assertion. Can do. Most easily this can be reproduced when switching back and forth between two cameras using the swISP, e.g. on a phone like the Pixel 3a. >> >> Fixes: 04d171e6 ("libcamera: software_isp: Call >> Algorithm::queueRequest") >> Signed-off-by: Robert Mader <robert.mader@collabora.com> >> --- >> src/ipa/simple/soft_simple.cpp | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/src/ipa/simple/soft_simple.cpp >> b/src/ipa/simple/soft_simple.cpp >> index b28c7039..ac8847cb 100644 >> --- a/src/ipa/simple/soft_simple.cpp >> +++ b/src/ipa/simple/soft_simple.cpp >> @@ -245,6 +245,7 @@ int IPASoftSimple::start() >> void IPASoftSimple::stop() >> { >> + context_.frameContexts.clear(); > > Looks good to me but additional hardware pipelines also reset/clear at > configure() time, for e.g. > > /* Clear the IPA context before the streaming session. */ > context_.configuration = {}; > context_.activeState = {}; > context_.frameContexts.clear(); > > Would it make sense to do it in Soft IPA as well? Yeah, good point - looks like cleaning up configuration and activeState so far wasn't really required for gain, but will be for the blacklevel.
diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp index b28c7039..ac8847cb 100644 --- a/src/ipa/simple/soft_simple.cpp +++ b/src/ipa/simple/soft_simple.cpp @@ -245,6 +245,7 @@ int IPASoftSimple::start() void IPASoftSimple::stop() { + context_.frameContexts.clear(); } void IPASoftSimple::queueRequest(const uint32_t frame, const ControlList &controls)
Like the hardware pipelines do. Otherwise we might hit the following assert: "Frame context for ... has been overwritten by ...". Fixes: 04d171e6 ("libcamera: software_isp: Call Algorithm::queueRequest") Signed-off-by: Robert Mader <robert.mader@collabora.com> --- src/ipa/simple/soft_simple.cpp | 1 + 1 file changed, 1 insertion(+)