| Message ID | 20230928094124.21801-1-david.plowman@raspberrypi.com |
|---|---|
| State | Accepted |
| Commit | ff41de7ba1f92ab445473177f9c9107baf2debda |
| Headers | show |
| Series |
|
| Related | show |
Hi David, Thank you for fixing this, quite a subtle bug! On Thu, 28 Sept 2023 at 10:41, David Plowman via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote: > > We avoid skipping the IPAs while frameCount_ is less then s/then/than/ > dropFrameCount_, indicating that these frames will not be sent to the > application. This means that when these numbers are equal then this is > the first frame the application will get, so again, we must avoid > skipping the IPAs. Consequently the test here must avoid the case of > equality. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> Perhaps add a Fixes: 51533fecae8 ("ipa: rpi: Fix frame count logic when running algorithms")? Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/ipa/rpi/common/ipa_base.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > index 551adadf..5df1998c 100644 > --- a/src/ipa/rpi/common/ipa_base.cpp > +++ b/src/ipa/rpi/common/ipa_base.cpp > @@ -409,7 +409,7 @@ void IpaBase::prepareIsp(const PrepareParams ¶ms) > > /* Allow a 10% margin on the comparison below. */ > Duration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns; > - if (lastRunTimestamp_ && frameCount_ >= dropFrameCount_ && > + if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ && > delta < controllerMinFrameDuration * 0.9) { > /* > * Ensure we merge the previous frame's metadata with the current > -- > 2.39.2 >
Quoting Naushir Patuck via libcamera-devel (2023-09-28 10:52:09) > Hi David, > > Thank you for fixing this, quite a subtle bug! > > On Thu, 28 Sept 2023 at 10:41, David Plowman via libcamera-devel > <libcamera-devel@lists.libcamera.org> wrote: > > > > We avoid skipping the IPAs while frameCount_ is less then > > s/then/than/ > > > dropFrameCount_, indicating that these frames will not be sent to the > > application. This means that when these numbers are equal then this is > > the first frame the application will get, so again, we must avoid > > skipping the IPAs. Consequently the test here must avoid the case of > > equality. > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > Perhaps add a Fixes: 51533fecae8 ("ipa: rpi: Fix frame count logic > when running algorithms")? > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> This looks reasonable to me. I'll apply the fixups above while applying. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > --- > > src/ipa/rpi/common/ipa_base.cpp | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > > index 551adadf..5df1998c 100644 > > --- a/src/ipa/rpi/common/ipa_base.cpp > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > @@ -409,7 +409,7 @@ void IpaBase::prepareIsp(const PrepareParams ¶ms) > > > > /* Allow a 10% margin on the comparison below. */ > > Duration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns; > > - if (lastRunTimestamp_ && frameCount_ >= dropFrameCount_ && > > + if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ && > > delta < controllerMinFrameDuration * 0.9) { > > /* > > * Ensure we merge the previous frame's metadata with the current > > -- > > 2.39.2 > >
diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp index 551adadf..5df1998c 100644 --- a/src/ipa/rpi/common/ipa_base.cpp +++ b/src/ipa/rpi/common/ipa_base.cpp @@ -409,7 +409,7 @@ void IpaBase::prepareIsp(const PrepareParams ¶ms) /* Allow a 10% margin on the comparison below. */ Duration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns; - if (lastRunTimestamp_ && frameCount_ >= dropFrameCount_ && + if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ && delta < controllerMinFrameDuration * 0.9) { /* * Ensure we merge the previous frame's metadata with the current
We avoid skipping the IPAs while frameCount_ is less then dropFrameCount_, indicating that these frames will not be sent to the application. This means that when these numbers are equal then this is the first frame the application will get, so again, we must avoid skipping the IPAs. Consequently the test here must avoid the case of equality. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- src/ipa/rpi/common/ipa_base.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)