[v2,6/6] ipa: rpi: Rename dropFrameCount_ to startupCount_
diff mbox series

Message ID 20250522075244.1198110-7-naush@raspberrypi.com
State New
Headers show
Series
  • Eliminating startup frames
Related show

Commit Message

Naushir Patuck May 22, 2025, 7:48 a.m. UTC 
Rename dropFrameCount_ to startupCount_ to better reflect its use as
frames are no longer dropped by the pipeline handler.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/ipa/rpi/common/ipa_base.cpp | 10 +++++-----
 src/ipa/rpi/common/ipa_base.h   |  4 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

Comments

Jacopo Mondi June 5, 2025, 7:30 a.m. UTC | #1 
Hi Naush

On Thu, May 22, 2025 at 08:48:22AM +0100, Naushir Patuck wrote:
> Rename dropFrameCount_ to startupCount_ to better reflect its use as
> frames are no longer dropped by the pipeline handler.

Makes sense

>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  src/ipa/rpi/common/ipa_base.cpp | 10 +++++-----
>  src/ipa/rpi/common/ipa_base.h   |  4 ++--
>  2 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> index c15f8a7bf71e..8d591faeceaa 100644
> --- a/src/ipa/rpi/common/ipa_base.cpp
> +++ b/src/ipa/rpi/common/ipa_base.cpp
> @@ -327,7 +327,7 @@ void IpaBase::start(const ControlList &controls, StartResult *result)
>  	unsigned int agcConvergenceFrames = 0, awbConvergenceFrames = 0;
>  	frameCount_ = 0;
>  	if (firstStart_) {
> -		dropFrameCount_ = helper_->hideFramesStartup();
> +		startupCount_ = helper_->hideFramesStartup();
>  		mistrustCount_ = helper_->mistrustFramesStartup();
>
>  		/*
> @@ -353,14 +353,14 @@ void IpaBase::start(const ControlList &controls, StartResult *result)
>  				awbConvergenceFrames += mistrustCount_;
>  		}
>  	} else {
> -		dropFrameCount_ = helper_->hideFramesModeSwitch();
> +		startupCount_ = helper_->hideFramesModeSwitch();
>  		mistrustCount_ = helper_->mistrustFramesModeSwitch();
>  	}
>
>  	result->startupFrameCount = std::max({ agcConvergenceFrames, awbConvergenceFrames });
> -	result->invalidFrameCount = dropFrameCount_;
> +	result->invalidFrameCount = startupCount_;

But here it makese

        result->startupFrameCount = .. convergence frames;
        result->invalidFrameCount = startupCount_;

It's your IPA, so if it's fine with you, fine with me!

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

>
> -	dropFrameCount_ = std::max({ dropFrameCount_, agcConvergenceFrames, awbConvergenceFrames });
> +	startupCount_ = std::max({ startupCount_, agcConvergenceFrames, awbConvergenceFrames });
>
>  	LOG(IPARPI, Debug) << "Startup frames: " << result->startupFrameCount
>  			   << " Invalid frames: " << result->invalidFrameCount;
> @@ -443,7 +443,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_ > startupCount_ &&
>  	    delta < controllerMinFrameDuration * 0.9 && !hdrChange) {
>  		/*
>  		 * Ensure we merge the previous frame's metadata with the current
> diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h
> index 1a811beb31f2..a51afc156a8f 100644
> --- a/src/ipa/rpi/common/ipa_base.h
> +++ b/src/ipa/rpi/common/ipa_base.h
> @@ -115,8 +115,8 @@ private:
>  	/* How many frames we should avoid running control algos on. */
>  	unsigned int mistrustCount_;
>
> -	/* Number of frames that need to be dropped on startup. */
> -	unsigned int dropFrameCount_;
> +	/* Number of frames that need to be marked as dropped on startup. */
> +	unsigned int startupCount_;
>
>  	/* Frame timestamp for the last run of the controller. */
>  	uint64_t lastRunTimestamp_;
> --
> 2.43.0
>
Naushir Patuck June 5, 2025, 11:34 a.m. UTC | #2 
Hi Jacopo,


On Thu, 5 Jun 2025 at 08:30, Jacopo Mondi <jacopo.mondi@ideasonboard.com>
wrote:

> Hi Naush
>
> On Thu, May 22, 2025 at 08:48:22AM +0100, Naushir Patuck wrote:
> > Rename dropFrameCount_ to startupCount_ to better reflect its use as
> > frames are no longer dropped by the pipeline handler.
>
> Makes sense
>
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  src/ipa/rpi/common/ipa_base.cpp | 10 +++++-----
> >  src/ipa/rpi/common/ipa_base.h   |  4 ++--
> >  2 files changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/ipa/rpi/common/ipa_base.cpp
> b/src/ipa/rpi/common/ipa_base.cpp
> > index c15f8a7bf71e..8d591faeceaa 100644
> > --- a/src/ipa/rpi/common/ipa_base.cpp
> > +++ b/src/ipa/rpi/common/ipa_base.cpp
> > @@ -327,7 +327,7 @@ void IpaBase::start(const ControlList &controls,
> StartResult *result)
> >       unsigned int agcConvergenceFrames = 0, awbConvergenceFrames = 0;
> >       frameCount_ = 0;
> >       if (firstStart_) {
> > -             dropFrameCount_ = helper_->hideFramesStartup();
> > +             startupCount_ = helper_->hideFramesStartup();
> >               mistrustCount_ = helper_->mistrustFramesStartup();
> >
> >               /*
> > @@ -353,14 +353,14 @@ void IpaBase::start(const ControlList &controls,
> StartResult *result)
> >                               awbConvergenceFrames += mistrustCount_;
> >               }
> >       } else {
> > -             dropFrameCount_ = helper_->hideFramesModeSwitch();
> > +             startupCount_ = helper_->hideFramesModeSwitch();
> >               mistrustCount_ = helper_->mistrustFramesModeSwitch();
> >       }
> >
> >       result->startupFrameCount = std::max({ agcConvergenceFrames,
> awbConvergenceFrames });
> > -     result->invalidFrameCount = dropFrameCount_;
> > +     result->invalidFrameCount = startupCount_;
>
> But here it makese
>
>         result->startupFrameCount = .. convergence frames;
>         result->invalidFrameCount = startupCount_;
>
> It's your IPA, so if it's fine with you, fine with me!
>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>

It took me a while to realise what was wrong here, and indeed the naming is
completely opposite between the IPA and PH.  I will rename to make these
variables consistent.

Regards,
Naush


>
> Thanks
>   j
>
> >
> > -     dropFrameCount_ = std::max({ dropFrameCount_,
> agcConvergenceFrames, awbConvergenceFrames });
> > +     startupCount_ = std::max({ startupCount_, agcConvergenceFrames,
> awbConvergenceFrames });
> >
> >       LOG(IPARPI, Debug) << "Startup frames: " <<
> result->startupFrameCount
> >                          << " Invalid frames: " <<
> result->invalidFrameCount;
> > @@ -443,7 +443,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_ > startupCount_ &&
> >           delta < controllerMinFrameDuration * 0.9 && !hdrChange) {
> >               /*
> >                * Ensure we merge the previous frame's metadata with the
> current
> > diff --git a/src/ipa/rpi/common/ipa_base.h
> b/src/ipa/rpi/common/ipa_base.h
> > index 1a811beb31f2..a51afc156a8f 100644
> > --- a/src/ipa/rpi/common/ipa_base.h
> > +++ b/src/ipa/rpi/common/ipa_base.h
> > @@ -115,8 +115,8 @@ private:
> >       /* How many frames we should avoid running control algos on. */
> >       unsigned int mistrustCount_;
> >
> > -     /* Number of frames that need to be dropped on startup. */
> > -     unsigned int dropFrameCount_;
> > +     /* Number of frames that need to be marked as dropped on startup.
> */
> > +     unsigned int startupCount_;
> >
> >       /* Frame timestamp for the last run of the controller. */
> >       uint64_t lastRunTimestamp_;
> > --
> > 2.43.0
> >
>

Patch
diff mbox series 

diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
index c15f8a7bf71e..8d591faeceaa 100644
--- a/src/ipa/rpi/common/ipa_base.cpp
+++ b/src/ipa/rpi/common/ipa_base.cpp
@@ -327,7 +327,7 @@  void IpaBase::start(const ControlList &controls, StartResult *result)
 	unsigned int agcConvergenceFrames = 0, awbConvergenceFrames = 0;
 	frameCount_ = 0;
 	if (firstStart_) {
-		dropFrameCount_ = helper_->hideFramesStartup();
+		startupCount_ = helper_->hideFramesStartup();
 		mistrustCount_ = helper_->mistrustFramesStartup();
 
 		/*
@@ -353,14 +353,14 @@  void IpaBase::start(const ControlList &controls, StartResult *result)
 				awbConvergenceFrames += mistrustCount_;
 		}
 	} else {
-		dropFrameCount_ = helper_->hideFramesModeSwitch();
+		startupCount_ = helper_->hideFramesModeSwitch();
 		mistrustCount_ = helper_->mistrustFramesModeSwitch();
 	}
 
 	result->startupFrameCount = std::max({ agcConvergenceFrames, awbConvergenceFrames });
-	result->invalidFrameCount = dropFrameCount_;
+	result->invalidFrameCount = startupCount_;
 
-	dropFrameCount_ = std::max({ dropFrameCount_, agcConvergenceFrames, awbConvergenceFrames });
+	startupCount_ = std::max({ startupCount_, agcConvergenceFrames, awbConvergenceFrames });
 
 	LOG(IPARPI, Debug) << "Startup frames: " << result->startupFrameCount
 			   << " Invalid frames: " << result->invalidFrameCount;
@@ -443,7 +443,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_ > startupCount_ &&
 	    delta < controllerMinFrameDuration * 0.9 && !hdrChange) {
 		/*
 		 * Ensure we merge the previous frame's metadata with the current
diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h
index 1a811beb31f2..a51afc156a8f 100644
--- a/src/ipa/rpi/common/ipa_base.h
+++ b/src/ipa/rpi/common/ipa_base.h
@@ -115,8 +115,8 @@  private:
 	/* How many frames we should avoid running control algos on. */
 	unsigned int mistrustCount_;
 
-	/* Number of frames that need to be dropped on startup. */
-	unsigned int dropFrameCount_;
+	/* Number of frames that need to be marked as dropped on startup. */
+	unsigned int startupCount_;
 
 	/* Frame timestamp for the last run of the controller. */
 	uint64_t lastRunTimestamp_;