[libcamera-devel,11/13] ipa: ipu3: agc: Remove condition on exposure correction
diff mbox series

Message ID 20211013154125.133419-12-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 
When the exposure is estimated, we verify if it changed enough for
exposure and gain to be updated.

There is no need for that because we are now filtering the value with
the previous one correctly, so if the change is very small is won't
change the exposure and analogue gain.

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
---
 src/ipa/ipu3/algorithms/agc.cpp | 96 ++++++++++++++++-----------------
 1 file changed, 46 insertions(+), 50 deletions(-)

Comments

Kieran Bingham Oct. 14, 2021, 11:33 a.m. UTC | #1 
Quoting Jean-Michel Hautbois (2021-10-13 16:41:23)
> When the exposure is estimated, we verify if it changed enough for
> exposure and gain to be updated.
> 
> There is no need for that because we are now filtering the value with
> the previous one correctly, so if the change is very small is won't
> change the exposure and analogue gain.

Oh excellent ... looks like here comes the refactor I was half
suggesting in the previous patch...

It's hard to see if there are any specific changes in here, and I assume
it's just the ultimate removal of the
 "if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {"

and associated re-indentation.

If so:

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

I'd be tempted to move that lastFrame_ = frameCount_ up to just after it
gets used by the opening checks, as it's not used by any of the code
following.

Unless perhaps it's better at the end to signify it's only updated after
the calculations are completed. Either way, it's fine really.

> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/agc.cpp | 96 ++++++++++++++++-----------------
>  1 file changed, 46 insertions(+), 50 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 7efe0907..b922bcdf 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -132,61 +132,57 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
>         if ((frameCount_ < kInitialFrameMinAECount) || (frameCount_ - lastFrame_ < kFrameSkipCount))
>                 return;
>  
> -       /* Are we correctly exposed ? */
> -       if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {
> -               LOG(IPU3Agc, Debug) << "!!! Good exposure with iqMean = " << iqMean_;
> -       } else {
> -               double evGain = kEvGainTarget * knumHistogramBins / iqMean_;
> -
> -               /* extracted from Rpi::Agc::computeTargetExposure */
> -               Duration currentShutter = exposure * lineDuration_;
> -               currentExposureNoDg_ = currentShutter * analogueGain;
> -               LOG(IPU3Agc, Debug) << "Actual total exposure " << currentExposureNoDg_
> -                                   << " Shutter speed " << currentShutter
> -                                   << " Gain " << analogueGain
> -                                   << " Needed ev gain " << evGain;
> -
> -               currentExposure_ = prevExposureValue * evGain;
> -               Duration maxTotalExposure = kMaxShutterSpeed * kMaxGain;
> -               currentExposure_ = std::min(currentExposure_, maxTotalExposure);
> -               LOG(IPU3Agc, Debug) << "Target total exposure " << currentExposure_
> -                                   << " maximum is " << maxTotalExposure;
> -
> -               /* \todo: estimate if we need to desaturate */
> -               filterExposure();
> -
> -               Duration exposureValue = filteredExposure_;
> -               Duration shutterTime = kMinShutterSpeed;
> -               double stepGain = kMinGain;
> -
> -               if (shutterTime * stepGain < exposureValue) {
> -                       Duration maxShutterMinGain = kMaxShutterSpeed * stepGain;
> -                       if (maxShutterMinGain >= exposureValue) {
> -                               LOG(IPU3Agc, Debug) << "Setting shutterTime to " << shutterTime;
> -                               shutterTime = exposureValue / stepGain;
> -                       } else {
> -                               shutterTime = kMaxShutterSpeed;
> -                       }
> +       double evGain = kEvGainTarget * knumHistogramBins / iqMean_;
> +
> +       /* extracted from Rpi::Agc::computeTargetExposure */
> +       Duration currentShutter = exposure * lineDuration_;
> +       currentExposureNoDg_ = currentShutter * analogueGain;
> +       LOG(IPU3Agc, Debug) << "Actual total exposure " << currentExposureNoDg_
> +                               << " Shutter speed " << currentShutter
> +                               << " Gain " << analogueGain
> +                               << " Needed ev gain " << evGain;
> +
> +       currentExposure_ = prevExposureValue * evGain;
> +       Duration maxTotalExposure = kMaxShutterSpeed * kMaxGain;
> +       currentExposure_ = std::min(currentExposure_, maxTotalExposure);
> +       LOG(IPU3Agc, Debug) << "Target total exposure " << currentExposure_
> +                               << " maximum is " << maxTotalExposure;
> +
> +       /* \todo: estimate if we need to desaturate */
> +       filterExposure();
> +
> +       Duration exposureValue = filteredExposure_;
> +       Duration shutterTime = kMinShutterSpeed;
> +       double stepGain = kMinGain;
> +
> +       if (shutterTime * stepGain < exposureValue) {
> +               Duration maxShutterMinGain = kMaxShutterSpeed * stepGain;
> +               if (maxShutterMinGain >= exposureValue) {
> +                       LOG(IPU3Agc, Debug) << "Setting shutterTime to " << shutterTime;
> +                       shutterTime = exposureValue / stepGain;
> +               } else {
> +                       shutterTime = kMaxShutterSpeed;
> +               }
>  
> -                       Duration maxGainShutter = kMaxGain * shutterTime;
> -                       if (maxGainShutter >= exposureValue) {
> -                               stepGain = exposureValue / shutterTime;
> -                               LOG(IPU3Agc, Debug) << "Setting analogue gain to " << stepGain;
> -                       } else {
> -                               stepGain = kMaxGain;
> -                       }
> +               Duration maxGainShutter = kMaxGain * shutterTime;
> +               if (maxGainShutter >= exposureValue) {
> +                       stepGain = exposureValue / shutterTime;
> +                       LOG(IPU3Agc, Debug) << "Setting analogue gain to " << stepGain;
> +               } else {
> +                       stepGain = kMaxGain;
>                 }
> +       }
>  
> -               LOG(IPU3Agc, Debug) << "Divided up shutter and gain are "
> -                               << shutterTime << " and "
> -                               << stepGain;
> +       LOG(IPU3Agc, Debug) << "Divided up shutter and gain are "
> +                       << shutterTime << " and "
> +                       << stepGain;
>  
> -               exposure = shutterTime / lineDuration_;
> -               analogueGain = stepGain;
> +       exposure = shutterTime / lineDuration_;
> +       analogueGain = stepGain;
> +
> +       /* Update the exposure value for the next process call */
> +       prevExposureValue = shutterTime * analogueGain;
>  
> -               /* Update the exposure value for the next process call */
> -               prevExposureValue = shutterTime * analogueGain;
> -       }
>         lastFrame_ = frameCount_;
>  }
>  
> -- 
> 2.30.2
>
Laurent Pinchart Oct. 15, 2021, 1:31 a.m. UTC | #2 
Hi Jean-Michel,

(CC'ing David)

On Wed, Oct 13, 2021 at 05:41:23PM +0200, Jean-Michel Hautbois wrote:
> When the exposure is estimated, we verify if it changed enough for
> exposure and gain to be updated.
> 
> There is no need for that because we are now filtering the value with
> the previous one correctly, so if the change is very small is won't
> change the exposure and analogue gain.

I fail to see why a small change won't change the exposure and analogue
gain. As far as I understand (maybe David can confirm), this removes a
hysteresis, which I believe will cause oscillations.

> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/agc.cpp | 96 ++++++++++++++++-----------------
>  1 file changed, 46 insertions(+), 50 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 7efe0907..b922bcdf 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -132,61 +132,57 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
>  	if ((frameCount_ < kInitialFrameMinAECount) || (frameCount_ - lastFrame_ < kFrameSkipCount))
>  		return;
>  
> -	/* Are we correctly exposed ? */
> -	if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {
> -		LOG(IPU3Agc, Debug) << "!!! Good exposure with iqMean = " << iqMean_;
> -	} else {
> -		double evGain = kEvGainTarget * knumHistogramBins / iqMean_;
> -
> -		/* extracted from Rpi::Agc::computeTargetExposure */
> -		Duration currentShutter = exposure * lineDuration_;
> -		currentExposureNoDg_ = currentShutter * analogueGain;
> -		LOG(IPU3Agc, Debug) << "Actual total exposure " << currentExposureNoDg_
> -				    << " Shutter speed " << currentShutter
> -				    << " Gain " << analogueGain
> -				    << " Needed ev gain " << evGain;
> -
> -		currentExposure_ = prevExposureValue * evGain;
> -		Duration maxTotalExposure = kMaxShutterSpeed * kMaxGain;
> -		currentExposure_ = std::min(currentExposure_, maxTotalExposure);
> -		LOG(IPU3Agc, Debug) << "Target total exposure " << currentExposure_
> -				    << " maximum is " << maxTotalExposure;
> -
> -		/* \todo: estimate if we need to desaturate */
> -		filterExposure();
> -
> -		Duration exposureValue = filteredExposure_;
> -		Duration shutterTime = kMinShutterSpeed;
> -		double stepGain = kMinGain;
> -
> -		if (shutterTime * stepGain < exposureValue) {
> -			Duration maxShutterMinGain = kMaxShutterSpeed * stepGain;
> -			if (maxShutterMinGain >= exposureValue) {
> -				LOG(IPU3Agc, Debug) << "Setting shutterTime to " << shutterTime;
> -				shutterTime = exposureValue / stepGain;
> -			} else {
> -				shutterTime = kMaxShutterSpeed;
> -			}
> +	double evGain = kEvGainTarget * knumHistogramBins / iqMean_;
> +
> +	/* extracted from Rpi::Agc::computeTargetExposure */
> +	Duration currentShutter = exposure * lineDuration_;
> +	currentExposureNoDg_ = currentShutter * analogueGain;
> +	LOG(IPU3Agc, Debug) << "Actual total exposure " << currentExposureNoDg_
> +				<< " Shutter speed " << currentShutter
> +				<< " Gain " << analogueGain
> +				<< " Needed ev gain " << evGain;
> +
> +	currentExposure_ = prevExposureValue * evGain;
> +	Duration maxTotalExposure = kMaxShutterSpeed * kMaxGain;
> +	currentExposure_ = std::min(currentExposure_, maxTotalExposure);
> +	LOG(IPU3Agc, Debug) << "Target total exposure " << currentExposure_
> +				<< " maximum is " << maxTotalExposure;
> +
> +	/* \todo: estimate if we need to desaturate */
> +	filterExposure();
> +
> +	Duration exposureValue = filteredExposure_;
> +	Duration shutterTime = kMinShutterSpeed;
> +	double stepGain = kMinGain;
> +
> +	if (shutterTime * stepGain < exposureValue) {
> +		Duration maxShutterMinGain = kMaxShutterSpeed * stepGain;
> +		if (maxShutterMinGain >= exposureValue) {
> +			LOG(IPU3Agc, Debug) << "Setting shutterTime to " << shutterTime;
> +			shutterTime = exposureValue / stepGain;
> +		} else {
> +			shutterTime = kMaxShutterSpeed;
> +		}
>  
> -			Duration maxGainShutter = kMaxGain * shutterTime;
> -			if (maxGainShutter >= exposureValue) {
> -				stepGain = exposureValue / shutterTime;
> -				LOG(IPU3Agc, Debug) << "Setting analogue gain to " << stepGain;
> -			} else {
> -				stepGain = kMaxGain;
> -			}
> +		Duration maxGainShutter = kMaxGain * shutterTime;
> +		if (maxGainShutter >= exposureValue) {
> +			stepGain = exposureValue / shutterTime;
> +			LOG(IPU3Agc, Debug) << "Setting analogue gain to " << stepGain;
> +		} else {
> +			stepGain = kMaxGain;
>  		}
> +	}
>  
> -		LOG(IPU3Agc, Debug) << "Divided up shutter and gain are "
> -				<< shutterTime << " and "
> -				<< stepGain;
> +	LOG(IPU3Agc, Debug) << "Divided up shutter and gain are "
> +			<< shutterTime << " and "
> +			<< stepGain;
>  
> -		exposure = shutterTime / lineDuration_;
> -		analogueGain = stepGain;
> +	exposure = shutterTime / lineDuration_;
> +	analogueGain = stepGain;
> +
> +	/* Update the exposure value for the next process call */
> +	prevExposureValue = shutterTime * analogueGain;
>  
> -		/* Update the exposure value for the next process call */
> -		prevExposureValue = shutterTime * analogueGain;
> -	}
>  	lastFrame_ = frameCount_;
>  }
>

Patch
diff mbox series 

diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
index 7efe0907..b922bcdf 100644
--- a/src/ipa/ipu3/algorithms/agc.cpp
+++ b/src/ipa/ipu3/algorithms/agc.cpp
@@ -132,61 +132,57 @@  void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
 	if ((frameCount_ < kInitialFrameMinAECount) || (frameCount_ - lastFrame_ < kFrameSkipCount))
 		return;
 
-	/* Are we correctly exposed ? */
-	if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {
-		LOG(IPU3Agc, Debug) << "!!! Good exposure with iqMean = " << iqMean_;
-	} else {
-		double evGain = kEvGainTarget * knumHistogramBins / iqMean_;
-
-		/* extracted from Rpi::Agc::computeTargetExposure */
-		Duration currentShutter = exposure * lineDuration_;
-		currentExposureNoDg_ = currentShutter * analogueGain;
-		LOG(IPU3Agc, Debug) << "Actual total exposure " << currentExposureNoDg_
-				    << " Shutter speed " << currentShutter
-				    << " Gain " << analogueGain
-				    << " Needed ev gain " << evGain;
-
-		currentExposure_ = prevExposureValue * evGain;
-		Duration maxTotalExposure = kMaxShutterSpeed * kMaxGain;
-		currentExposure_ = std::min(currentExposure_, maxTotalExposure);
-		LOG(IPU3Agc, Debug) << "Target total exposure " << currentExposure_
-				    << " maximum is " << maxTotalExposure;
-
-		/* \todo: estimate if we need to desaturate */
-		filterExposure();
-
-		Duration exposureValue = filteredExposure_;
-		Duration shutterTime = kMinShutterSpeed;
-		double stepGain = kMinGain;
-
-		if (shutterTime * stepGain < exposureValue) {
-			Duration maxShutterMinGain = kMaxShutterSpeed * stepGain;
-			if (maxShutterMinGain >= exposureValue) {
-				LOG(IPU3Agc, Debug) << "Setting shutterTime to " << shutterTime;
-				shutterTime = exposureValue / stepGain;
-			} else {
-				shutterTime = kMaxShutterSpeed;
-			}
+	double evGain = kEvGainTarget * knumHistogramBins / iqMean_;
+
+	/* extracted from Rpi::Agc::computeTargetExposure */
+	Duration currentShutter = exposure * lineDuration_;
+	currentExposureNoDg_ = currentShutter * analogueGain;
+	LOG(IPU3Agc, Debug) << "Actual total exposure " << currentExposureNoDg_
+				<< " Shutter speed " << currentShutter
+				<< " Gain " << analogueGain
+				<< " Needed ev gain " << evGain;
+
+	currentExposure_ = prevExposureValue * evGain;
+	Duration maxTotalExposure = kMaxShutterSpeed * kMaxGain;
+	currentExposure_ = std::min(currentExposure_, maxTotalExposure);
+	LOG(IPU3Agc, Debug) << "Target total exposure " << currentExposure_
+				<< " maximum is " << maxTotalExposure;
+
+	/* \todo: estimate if we need to desaturate */
+	filterExposure();
+
+	Duration exposureValue = filteredExposure_;
+	Duration shutterTime = kMinShutterSpeed;
+	double stepGain = kMinGain;
+
+	if (shutterTime * stepGain < exposureValue) {
+		Duration maxShutterMinGain = kMaxShutterSpeed * stepGain;
+		if (maxShutterMinGain >= exposureValue) {
+			LOG(IPU3Agc, Debug) << "Setting shutterTime to " << shutterTime;
+			shutterTime = exposureValue / stepGain;
+		} else {
+			shutterTime = kMaxShutterSpeed;
+		}
 
-			Duration maxGainShutter = kMaxGain * shutterTime;
-			if (maxGainShutter >= exposureValue) {
-				stepGain = exposureValue / shutterTime;
-				LOG(IPU3Agc, Debug) << "Setting analogue gain to " << stepGain;
-			} else {
-				stepGain = kMaxGain;
-			}
+		Duration maxGainShutter = kMaxGain * shutterTime;
+		if (maxGainShutter >= exposureValue) {
+			stepGain = exposureValue / shutterTime;
+			LOG(IPU3Agc, Debug) << "Setting analogue gain to " << stepGain;
+		} else {
+			stepGain = kMaxGain;
 		}
+	}
 
-		LOG(IPU3Agc, Debug) << "Divided up shutter and gain are "
-				<< shutterTime << " and "
-				<< stepGain;
+	LOG(IPU3Agc, Debug) << "Divided up shutter and gain are "
+			<< shutterTime << " and "
+			<< stepGain;
 
-		exposure = shutterTime / lineDuration_;
-		analogueGain = stepGain;
+	exposure = shutterTime / lineDuration_;
+	analogueGain = stepGain;
+
+	/* Update the exposure value for the next process call */
+	prevExposureValue = shutterTime * analogueGain;
 
-		/* Update the exposure value for the next process call */
-		prevExposureValue = shutterTime * analogueGain;
-	}
 	lastFrame_ = frameCount_;
 }