[libcamera-devel] ipa: rpi: Fix frame count logic when running algorithms
diff mbox series

Message ID 20230925102714.31780-1-naush@raspberrypi.com
State Accepted
Commit 51533fecae8d9efb82a436eb26504cb8a8db9170
Headers show
Series
  • [libcamera-devel] ipa: rpi: Fix frame count logic when running algorithms
Related show

Commit Message

Naushir Patuck Sept. 25, 2023, 10:27 a.m. UTC 
The frame counter test to determine if we run the IPA algorithms has a
logic bug where it treats dropFrameCount_ and mistrustCount_ as frame
numbers, not counts of frames (which it is). The implication is that
startup convergence and initial settings take one extra frame to apply.
Fix this.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/ipa/rpi/common/ipa_base.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

David Plowman Sept. 25, 2023, 10:40 a.m. UTC | #1 
Hi Naush

Thanks for the fixes! Indeed, I was noticing this...

On Mon, 25 Sept 2023 at 11:27, Naushir Patuck <naush@raspberrypi.com> wrote:
>
> The frame counter test to determine if we run the IPA algorithms has a
> logic bug where it treats dropFrameCount_ and mistrustCount_ as frame
> numbers, not counts of frames (which it is). The implication is that
> startup convergence and initial settings take one extra frame to apply.
> Fix this.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>

Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
Tested-by: David Plowman <david.plowman@raspberrypi.com>

Thanks!
David

> ---
>  src/ipa/rpi/common/ipa_base.cpp | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> index f7e7ad5ee499..5d8344e3e7e3 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 &params)
>
>         /* 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
> @@ -451,7 +451,7 @@ void IpaBase::processStats(const ProcessParams &params)
>  {
>         unsigned int ipaContext = params.ipaContext % rpiMetadata_.size();
>
> -       if (processPending_ && frameCount_ > mistrustCount_) {
> +       if (processPending_ && frameCount_ >= mistrustCount_) {
>                 RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];
>
>                 auto it = buffers_.find(params.buffers.stats);
> --
> 2.34.1
>
Kieran Bingham Sept. 25, 2023, 2:45 p.m. UTC | #2 
Quoting David Plowman via libcamera-devel (2023-09-25 11:40:11)
> Hi Naush
> 
> Thanks for the fixes! Indeed, I was noticing this...
> 
> On Mon, 25 Sept 2023 at 11:27, Naushir Patuck <naush@raspberrypi.com> wrote:
> >
> > The frame counter test to determine if we run the IPA algorithms has a
> > logic bug where it treats dropFrameCount_ and mistrustCount_ as frame
> > numbers, not counts of frames (which it is). The implication is that
> > startup convergence and initial settings take one extra frame to apply.
> > Fix this.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> 
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> Tested-by: David Plowman <david.plowman@raspberrypi.com>


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Collecting this and the YV444 format now.

--
Kieran


> 
> Thanks!
> David
> 
> > ---
> >  src/ipa/rpi/common/ipa_base.cpp | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> > index f7e7ad5ee499..5d8344e3e7e3 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 &params)
> >
> >         /* 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
> > @@ -451,7 +451,7 @@ void IpaBase::processStats(const ProcessParams &params)
> >  {
> >         unsigned int ipaContext = params.ipaContext % rpiMetadata_.size();
> >
> > -       if (processPending_ && frameCount_ > mistrustCount_) {
> > +       if (processPending_ && frameCount_ >= mistrustCount_) {
> >                 RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];
> >
> >                 auto it = buffers_.find(params.buffers.stats);
> > --
> > 2.34.1
> >

Patch
diff mbox series 

diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
index f7e7ad5ee499..5d8344e3e7e3 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 &params)
 
 	/* 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
@@ -451,7 +451,7 @@  void IpaBase::processStats(const ProcessParams &params)
 {
 	unsigned int ipaContext = params.ipaContext % rpiMetadata_.size();
 
-	if (processPending_ && frameCount_ > mistrustCount_) {
+	if (processPending_ && frameCount_ >= mistrustCount_) {
 		RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];
 
 		auto it = buffers_.find(params.buffers.stats);