[libcamera-devel,05/13] ipa: ipu3: agc: Rename exposure values properly
diff mbox series

Message ID 20211013154125.133419-6-jeanmichel.hautbois@ideasonboard.com
State Superseded
Headers show
Series
  • ipa: ipu3: Fix AGC bugs
Related show

Commit Message

Jean-Michel Hautbois Oct. 13, 2021, 3:41 p.m. UTC 
The exposure value is filtered in filterExposure() using the
currentExposure_ and setting a prevExposure_ variable. This is misnamed
as it is not the previous exposure, but a filtered value.

Rename it accordingly.

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
---
 src/ipa/ipu3/algorithms/agc.cpp | 28 ++++++++++++++--------------
 src/ipa/ipu3/algorithms/agc.h   |  4 ++--
 2 files changed, 16 insertions(+), 16 deletions(-)

Comments

Kieran Bingham Oct. 14, 2021, 10:51 a.m. UTC | #1 
Quoting Jean-Michel Hautbois (2021-10-13 16:41:17)
> The exposure value is filtered in filterExposure() using the
> currentExposure_ and setting a prevExposure_ variable. This is misnamed
> as it is not the previous exposure, but a filtered value.
> 
> Rename it accordingly.
> 

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

> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/agc.cpp | 28 ++++++++++++++--------------
>  src/ipa/ipu3/algorithms/agc.h   |  4 ++--
>  2 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 5eafe82c..7c2d4201 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -49,7 +49,7 @@ static constexpr double kEvGainTarget = 0.5;
>  
>  Agc::Agc()
>         : frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),
> -         maxExposureTime_(0s), prevExposure_(0s), prevExposureNoDg_(0s),
> +         maxExposureTime_(0s), filteredExposure_(0s), filteredExposureNoDg_(0s),
>           currentExposure_(0s), currentExposureNoDg_(0s)
>  {
>  }
> @@ -94,24 +94,24 @@ void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,
>  void Agc::filterExposure()
>  {
>         double speed = 0.2;
> -       if (prevExposure_ == 0s) {
> +       if (filteredExposure_ == 0s) {
>                 /* DG stands for digital gain.*/
> -               prevExposure_ = currentExposure_;
> -               prevExposureNoDg_ = currentExposureNoDg_;
> +               filteredExposure_ = currentExposure_;
> +               filteredExposureNoDg_ = currentExposureNoDg_;
>         } else {
>                 /*
>                  * If we are close to the desired result, go faster to avoid making
>                  * multiple micro-adjustments.
>                  * \ todo: Make this customisable?
>                  */
> -               if (prevExposure_ < 1.2 * currentExposure_ &&
> -                   prevExposure_ > 0.8 * currentExposure_)
> +               if (filteredExposure_ < 1.2 * currentExposure_ &&
> +                   filteredExposure_ > 0.8 * currentExposure_)
>                         speed = sqrt(speed);
>  
> -               prevExposure_ = speed * currentExposure_ +
> -                               prevExposure_ * (1.0 - speed);
> -               prevExposureNoDg_ = speed * currentExposureNoDg_ +
> -                               prevExposureNoDg_ * (1.0 - speed);
> +               filteredExposure_ = speed * currentExposure_ +
> +                               filteredExposure_ * (1.0 - speed);
> +               filteredExposureNoDg_ = speed * currentExposureNoDg_ +
> +                               filteredExposureNoDg_ * (1.0 - speed);
>         }
>         /*
>          * We can't let the no_dg exposure deviate too far below the
> @@ -119,10 +119,10 @@ void Agc::filterExposure()
>          * in the ISP to hide it (which will cause nasty oscillation).
>          */
>         double fastReduceThreshold = 0.4;
> -       if (prevExposureNoDg_ <
> -           prevExposure_ * fastReduceThreshold)
> -               prevExposureNoDg_ = prevExposure_ * fastReduceThreshold;
> -       LOG(IPU3Agc, Debug) << "After filtering, total_exposure " << prevExposure_;
> +       if (filteredExposureNoDg_ <
> +           filteredExposure_ * fastReduceThreshold)
> +               filteredExposureNoDg_ = filteredExposure_ * fastReduceThreshold;
> +       LOG(IPU3Agc, Debug) << "After filtering, total_exposure " << filteredExposure_;
>  }
>  
>  void Agc::lockExposureGain(uint32_t &exposure, double &gain)
> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> index 64b71c65..cd4d4855 100644
> --- a/src/ipa/ipu3/algorithms/agc.h
> +++ b/src/ipa/ipu3/algorithms/agc.h
> @@ -46,8 +46,8 @@ private:
>         Duration lineDuration_;
>         Duration maxExposureTime_;
>  
> -       Duration prevExposure_;
> -       Duration prevExposureNoDg_;
> +       Duration filteredExposure_;
> +       Duration filteredExposureNoDg_;
>         Duration currentExposure_;
>         Duration currentExposureNoDg_;
>  
> -- 
> 2.30.2
>
Laurent Pinchart Oct. 14, 2021, 10:58 p.m. UTC | #2 
Hi Jean-Michel,

Thank you for the patch.

On Wed, Oct 13, 2021 at 05:41:17PM +0200, Jean-Michel Hautbois wrote:
> The exposure value is filtered in filterExposure() using the
> currentExposure_ and setting a prevExposure_ variable. This is misnamed
> as it is not the previous exposure, but a filtered value.
> 
> Rename it accordingly.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  src/ipa/ipu3/algorithms/agc.cpp | 28 ++++++++++++++--------------
>  src/ipa/ipu3/algorithms/agc.h   |  4 ++--
>  2 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 5eafe82c..7c2d4201 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -49,7 +49,7 @@ static constexpr double kEvGainTarget = 0.5;
>  
>  Agc::Agc()
>  	: frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),
> -	  maxExposureTime_(0s), prevExposure_(0s), prevExposureNoDg_(0s),
> +	  maxExposureTime_(0s), filteredExposure_(0s), filteredExposureNoDg_(0s),
>  	  currentExposure_(0s), currentExposureNoDg_(0s)
>  {
>  }
> @@ -94,24 +94,24 @@ void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,
>  void Agc::filterExposure()
>  {
>  	double speed = 0.2;
> -	if (prevExposure_ == 0s) {
> +	if (filteredExposure_ == 0s) {
>  		/* DG stands for digital gain.*/
> -		prevExposure_ = currentExposure_;
> -		prevExposureNoDg_ = currentExposureNoDg_;
> +		filteredExposure_ = currentExposure_;
> +		filteredExposureNoDg_ = currentExposureNoDg_;
>  	} else {
>  		/*
>  		 * If we are close to the desired result, go faster to avoid making
>  		 * multiple micro-adjustments.
>  		 * \ todo: Make this customisable?
>  		 */
> -		if (prevExposure_ < 1.2 * currentExposure_ &&
> -		    prevExposure_ > 0.8 * currentExposure_)
> +		if (filteredExposure_ < 1.2 * currentExposure_ &&
> +		    filteredExposure_ > 0.8 * currentExposure_)
>  			speed = sqrt(speed);
>  
> -		prevExposure_ = speed * currentExposure_ +
> -				prevExposure_ * (1.0 - speed);
> -		prevExposureNoDg_ = speed * currentExposureNoDg_ +
> -				prevExposureNoDg_ * (1.0 - speed);
> +		filteredExposure_ = speed * currentExposure_ +
> +				filteredExposure_ * (1.0 - speed);
> +		filteredExposureNoDg_ = speed * currentExposureNoDg_ +
> +				filteredExposureNoDg_ * (1.0 - speed);
>  	}
>  	/*
>  	 * We can't let the no_dg exposure deviate too far below the
> @@ -119,10 +119,10 @@ void Agc::filterExposure()
>  	 * in the ISP to hide it (which will cause nasty oscillation).
>  	 */
>  	double fastReduceThreshold = 0.4;
> -	if (prevExposureNoDg_ <
> -	    prevExposure_ * fastReduceThreshold)
> -		prevExposureNoDg_ = prevExposure_ * fastReduceThreshold;
> -	LOG(IPU3Agc, Debug) << "After filtering, total_exposure " << prevExposure_;
> +	if (filteredExposureNoDg_ <
> +	    filteredExposure_ * fastReduceThreshold)
> +		filteredExposureNoDg_ = filteredExposure_ * fastReduceThreshold;
> +	LOG(IPU3Agc, Debug) << "After filtering, total_exposure " << filteredExposure_;
>  }
>  
>  void Agc::lockExposureGain(uint32_t &exposure, double &gain)
> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> index 64b71c65..cd4d4855 100644
> --- a/src/ipa/ipu3/algorithms/agc.h
> +++ b/src/ipa/ipu3/algorithms/agc.h
> @@ -46,8 +46,8 @@ private:
>  	Duration lineDuration_;
>  	Duration maxExposureTime_;
>  
> -	Duration prevExposure_;
> -	Duration prevExposureNoDg_;
> +	Duration filteredExposure_;
> +	Duration filteredExposureNoDg_;
>  	Duration currentExposure_;
>  	Duration currentExposureNoDg_;
>

Patch
diff mbox series 

diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
index 5eafe82c..7c2d4201 100644
--- a/src/ipa/ipu3/algorithms/agc.cpp
+++ b/src/ipa/ipu3/algorithms/agc.cpp
@@ -49,7 +49,7 @@  static constexpr double kEvGainTarget = 0.5;
 
 Agc::Agc()
 	: frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),
-	  maxExposureTime_(0s), prevExposure_(0s), prevExposureNoDg_(0s),
+	  maxExposureTime_(0s), filteredExposure_(0s), filteredExposureNoDg_(0s),
 	  currentExposure_(0s), currentExposureNoDg_(0s)
 {
 }
@@ -94,24 +94,24 @@  void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,
 void Agc::filterExposure()
 {
 	double speed = 0.2;
-	if (prevExposure_ == 0s) {
+	if (filteredExposure_ == 0s) {
 		/* DG stands for digital gain.*/
-		prevExposure_ = currentExposure_;
-		prevExposureNoDg_ = currentExposureNoDg_;
+		filteredExposure_ = currentExposure_;
+		filteredExposureNoDg_ = currentExposureNoDg_;
 	} else {
 		/*
 		 * If we are close to the desired result, go faster to avoid making
 		 * multiple micro-adjustments.
 		 * \ todo: Make this customisable?
 		 */
-		if (prevExposure_ < 1.2 * currentExposure_ &&
-		    prevExposure_ > 0.8 * currentExposure_)
+		if (filteredExposure_ < 1.2 * currentExposure_ &&
+		    filteredExposure_ > 0.8 * currentExposure_)
 			speed = sqrt(speed);
 
-		prevExposure_ = speed * currentExposure_ +
-				prevExposure_ * (1.0 - speed);
-		prevExposureNoDg_ = speed * currentExposureNoDg_ +
-				prevExposureNoDg_ * (1.0 - speed);
+		filteredExposure_ = speed * currentExposure_ +
+				filteredExposure_ * (1.0 - speed);
+		filteredExposureNoDg_ = speed * currentExposureNoDg_ +
+				filteredExposureNoDg_ * (1.0 - speed);
 	}
 	/*
 	 * We can't let the no_dg exposure deviate too far below the
@@ -119,10 +119,10 @@  void Agc::filterExposure()
 	 * in the ISP to hide it (which will cause nasty oscillation).
 	 */
 	double fastReduceThreshold = 0.4;
-	if (prevExposureNoDg_ <
-	    prevExposure_ * fastReduceThreshold)
-		prevExposureNoDg_ = prevExposure_ * fastReduceThreshold;
-	LOG(IPU3Agc, Debug) << "After filtering, total_exposure " << prevExposure_;
+	if (filteredExposureNoDg_ <
+	    filteredExposure_ * fastReduceThreshold)
+		filteredExposureNoDg_ = filteredExposure_ * fastReduceThreshold;
+	LOG(IPU3Agc, Debug) << "After filtering, total_exposure " << filteredExposure_;
 }
 
 void Agc::lockExposureGain(uint32_t &exposure, double &gain)
diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
index 64b71c65..cd4d4855 100644
--- a/src/ipa/ipu3/algorithms/agc.h
+++ b/src/ipa/ipu3/algorithms/agc.h
@@ -46,8 +46,8 @@  private:
 	Duration lineDuration_;
 	Duration maxExposureTime_;
 
-	Duration prevExposure_;
-	Duration prevExposureNoDg_;
+	Duration filteredExposure_;
+	Duration filteredExposureNoDg_;
 	Duration currentExposure_;
 	Duration currentExposureNoDg_;