[libcamera-devel,v3,09/14] ipa: ipu3: agc: Simplify division of exposure/gain
diff mbox series

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

Commit Message

Jean-Michel Hautbois Oct. 21, 2021, 4:43 p.m. UTC 
Until now, the algorithm makes complex assumptions when dividing the
exposure and analogue gain values. Instead, use a simpler clamping of
the shutter speed first, and then of the analogue gain, based on the
limits configured.

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/ipa/ipu3/algorithms/agc.cpp | 36 +++++++++++++++++----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

Comments

Kieran Bingham Oct. 21, 2021, 9:08 p.m. UTC | #1 
Quoting Jean-Michel Hautbois (2021-10-21 17:43:56)
> Until now, the algorithm makes complex assumptions when dividing the
> exposure and analogue gain values. Instead, use a simpler clamping of
> the shutter speed first, and then of the analogue gain, based on the
> limits configured.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/agc.cpp | 36 +++++++++++++++++----------------
>  1 file changed, 19 insertions(+), 17 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index f5bb3328..6f5b6a45 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -147,7 +147,9 @@ void Agc::lockExposureGain(uint32_t &exposure, double &gain)
>                                     << " Gain " << gain;
>  
>                 currentExposure_ = currentExposureNoDg_ * newGain;
> +               utils::Duration minShutterSpeed = minExposureLines_ * lineDuration_;
>                 utils::Duration maxShutterSpeed = maxExposureLines_ * lineDuration_;
> +
>                 utils::Duration maxTotalExposure = maxShutterSpeed * kMaxGain;
>                 currentExposure_ = std::min(currentExposure_, maxTotalExposure);
>                 LOG(IPU3Agc, Debug) << "Target total exposure " << currentExposure_
> @@ -156,23 +158,23 @@ void Agc::lockExposureGain(uint32_t &exposure, double &gain)
>                 /* \todo: estimate if we need to desaturate */
>                 filterExposure();
>  
> -               utils::Duration newExposure = 0.0s;
> -               if (currentShutter < maxShutterSpeed) {
> -                       exposure = std::clamp<uint32_t>(exposure * filteredExposure_ / currentExposureNoDg_,
> -                                                       minExposureLines_,
> -                                                       maxExposureLines_);
> -                       newExposure = filteredExposure_ / exposure;
> -                       gain = std::clamp(gain * filteredExposure_ / newExposure,
> -                                         kMinGain, kMaxGain);
> -               } else {
> -                       gain = std::clamp(gain * filteredExposure_ / currentExposureNoDg_,
> -                                         kMinGain, kMaxGain);
> -                       newExposure = filteredExposure_ / gain;
> -                       exposure = std::clamp<uint32_t>(exposure * filteredExposure_ / newExposure,

Makes the previous patch a bit redundant, but at least we removed code
that was more 'correct' ;-)

> -                                                       minExposureLines_,
> -                                                       maxExposureLines_);
> -               }
> -               LOG(IPU3Agc, Debug) << "Adjust exposure " << exposure * lineDuration_ << " and gain " << gain;
> +               utils::Duration exposureValue = filteredExposure_;
> +               utils::Duration shutterTime = minShutterSpeed;
> +
> +               /*
> +                * Push the shutter time up to the maximum first, and only then
> +                * increase the gain.
> +                */
> +               shutterTime = std::clamp<utils::Duration>(exposureValue / kMinGain,
> +                                                         minShutterSpeed, maxShutterSpeed);
> +               double stepGain = std::clamp(exposureValue / shutterTime,
> +                                            kMinGain, kMaxGain);
> +               LOG(IPU3Agc, Debug) << "Divided up shutter and gain are "
> +                                   << shutterTime << " and "
> +                                   << stepGain;
> +
> +               exposure = shutterTime / lineDuration_;
> +               gain = stepGain;

This looks a lot simpler indeed. I haven't gone through and fully
validated all the equations, but as I believe I've seen this working,
locally on my device, I think I can trust that it's functional.

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

>         }
>         lastFrame_ = frameCount_;
>  }
> -- 
> 2.32.0
>
Laurent Pinchart Oct. 22, 2021, 4:08 a.m. UTC | #2 
On Thu, Oct 21, 2021 at 10:08:37PM +0100, Kieran Bingham wrote:
> Quoting Jean-Michel Hautbois (2021-10-21 17:43:56)
> > Until now, the algorithm makes complex assumptions when dividing the
> > exposure and analogue gain values. Instead, use a simpler clamping of
> > the shutter speed first, and then of the analogue gain, based on the
> > limits configured.
> > 
> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/ipa/ipu3/algorithms/agc.cpp | 36 +++++++++++++++++----------------
> >  1 file changed, 19 insertions(+), 17 deletions(-)
> > 
> > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> > index f5bb3328..6f5b6a45 100644
> > --- a/src/ipa/ipu3/algorithms/agc.cpp
> > +++ b/src/ipa/ipu3/algorithms/agc.cpp
> > @@ -147,7 +147,9 @@ void Agc::lockExposureGain(uint32_t &exposure, double &gain)
> >                                     << " Gain " << gain;
> >  
> >                 currentExposure_ = currentExposureNoDg_ * newGain;
> > +               utils::Duration minShutterSpeed = minExposureLines_ * lineDuration_;
> >                 utils::Duration maxShutterSpeed = maxExposureLines_ * lineDuration_;
> > +
> >                 utils::Duration maxTotalExposure = maxShutterSpeed * kMaxGain;
> >                 currentExposure_ = std::min(currentExposure_, maxTotalExposure);
> >                 LOG(IPU3Agc, Debug) << "Target total exposure " << currentExposure_
> > @@ -156,23 +158,23 @@ void Agc::lockExposureGain(uint32_t &exposure, double &gain)
> >                 /* \todo: estimate if we need to desaturate */
> >                 filterExposure();
> >  
> > -               utils::Duration newExposure = 0.0s;
> > -               if (currentShutter < maxShutterSpeed) {
> > -                       exposure = std::clamp<uint32_t>(exposure * filteredExposure_ / currentExposureNoDg_,
> > -                                                       minExposureLines_,
> > -                                                       maxExposureLines_);
> > -                       newExposure = filteredExposure_ / exposure;
> > -                       gain = std::clamp(gain * filteredExposure_ / newExposure,
> > -                                         kMinGain, kMaxGain);
> > -               } else {
> > -                       gain = std::clamp(gain * filteredExposure_ / currentExposureNoDg_,
> > -                                         kMinGain, kMaxGain);
> > -                       newExposure = filteredExposure_ / gain;
> > -                       exposure = std::clamp<uint32_t>(exposure * filteredExposure_ / newExposure,
> 
> Makes the previous patch a bit redundant, but at least we removed code
> that was more 'correct' ;-)

The two used to be combined in a single patch, and I've pointed out
twice that they were separate changes :-) The switch to
filteredExposure_ was easy to overlook in previous versions.

> > -                                                       minExposureLines_,
> > -                                                       maxExposureLines_);
> > -               }
> > -               LOG(IPU3Agc, Debug) << "Adjust exposure " << exposure * lineDuration_ << " and gain " << gain;
> > +               utils::Duration exposureValue = filteredExposure_;
> > +               utils::Duration shutterTime = minShutterSpeed;
> > +
> > +               /*
> > +                * Push the shutter time up to the maximum first, and only then
> > +                * increase the gain.
> > +                */
> > +               shutterTime = std::clamp<utils::Duration>(exposureValue / kMinGain,
> > +                                                         minShutterSpeed, maxShutterSpeed);
> > +               double stepGain = std::clamp(exposureValue / shutterTime,
> > +                                            kMinGain, kMaxGain);
> > +               LOG(IPU3Agc, Debug) << "Divided up shutter and gain are "
> > +                                   << shutterTime << " and "
> > +                                   << stepGain;
> > +
> > +               exposure = shutterTime / lineDuration_;
> > +               gain = stepGain;
> 
> This looks a lot simpler indeed. I haven't gone through and fully
> validated all the equations, but as I believe I've seen this working,
> locally on my device, I think I can trust that it's functional.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> >         }
> >         lastFrame_ = frameCount_;
> >  }

Patch
diff mbox series 

diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
index f5bb3328..6f5b6a45 100644
--- a/src/ipa/ipu3/algorithms/agc.cpp
+++ b/src/ipa/ipu3/algorithms/agc.cpp
@@ -147,7 +147,9 @@  void Agc::lockExposureGain(uint32_t &exposure, double &gain)
 				    << " Gain " << gain;
 
 		currentExposure_ = currentExposureNoDg_ * newGain;
+		utils::Duration minShutterSpeed = minExposureLines_ * lineDuration_;
 		utils::Duration maxShutterSpeed = maxExposureLines_ * lineDuration_;
+
 		utils::Duration maxTotalExposure = maxShutterSpeed * kMaxGain;
 		currentExposure_ = std::min(currentExposure_, maxTotalExposure);
 		LOG(IPU3Agc, Debug) << "Target total exposure " << currentExposure_
@@ -156,23 +158,23 @@  void Agc::lockExposureGain(uint32_t &exposure, double &gain)
 		/* \todo: estimate if we need to desaturate */
 		filterExposure();
 
-		utils::Duration newExposure = 0.0s;
-		if (currentShutter < maxShutterSpeed) {
-			exposure = std::clamp<uint32_t>(exposure * filteredExposure_ / currentExposureNoDg_,
-							minExposureLines_,
-							maxExposureLines_);
-			newExposure = filteredExposure_ / exposure;
-			gain = std::clamp(gain * filteredExposure_ / newExposure,
-					  kMinGain, kMaxGain);
-		} else {
-			gain = std::clamp(gain * filteredExposure_ / currentExposureNoDg_,
-					  kMinGain, kMaxGain);
-			newExposure = filteredExposure_ / gain;
-			exposure = std::clamp<uint32_t>(exposure * filteredExposure_ / newExposure,
-							minExposureLines_,
-							maxExposureLines_);
-		}
-		LOG(IPU3Agc, Debug) << "Adjust exposure " << exposure * lineDuration_ << " and gain " << gain;
+		utils::Duration exposureValue = filteredExposure_;
+		utils::Duration shutterTime = minShutterSpeed;
+
+		/*
+		 * Push the shutter time up to the maximum first, and only then
+		 * increase the gain.
+		 */
+		shutterTime = std::clamp<utils::Duration>(exposureValue / kMinGain,
+							  minShutterSpeed, maxShutterSpeed);
+		double stepGain = std::clamp(exposureValue / shutterTime,
+					     kMinGain, kMaxGain);
+		LOG(IPU3Agc, Debug) << "Divided up shutter and gain are "
+				    << shutterTime << " and "
+				    << stepGain;
+
+		exposure = shutterTime / lineDuration_;
+		gain = stepGain;
 	}
 	lastFrame_ = frameCount_;
 }