[libcamera-devel,v2,08/10] ipa: ipu3: Remove unneeded function calls in AGC
diff mbox series

Message ID 20210812165243.276977-9-jeanmichel.hautbois@ideasonboard.com
State Superseded
Headers show
Series
  • IPU3: Introduce modularity for algorithms
Related show

Commit Message

Jean-Michel Hautbois Aug. 12, 2021, 4:52 p.m. UTC 
We never use converged_ so remove its declaration.
The controls may not need to be updated at each call, but it should be
decided on the context side and not by a specific call.

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
---
 src/ipa/ipu3/ipu3.cpp     |  6 ++----
 src/ipa/ipu3/ipu3_agc.cpp | 10 ++--------
 src/ipa/ipu3/ipu3_agc.h   |  5 -----
 3 files changed, 4 insertions(+), 17 deletions(-)

Comments

Kieran Bingham Aug. 13, 2021, 10:06 a.m. UTC | #1 
On 12/08/2021 17:52, Jean-Michel Hautbois wrote:
> We never use converged_ so remove its declaration.
> The controls may not need to be updated at each call, but it should be
> decided on the context side and not by a specific call.

Good, I see how this helps move towards making it more modular.
I guess the short term effect is that the controls will be set for every
frame, but if the values don't change, then that's likely a no-op in the
kernel anyway so I dont' think that's an issue.

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

> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3.cpp     |  6 ++----
>  src/ipa/ipu3/ipu3_agc.cpp | 10 ++--------
>  src/ipa/ipu3/ipu3_agc.h   |  5 -----
>  3 files changed, 4 insertions(+), 17 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 2a8225a0..27765aa6 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -309,8 +309,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
>  	for (auto const &algo : algorithms_)
>  		algo->prepare(context_, params_);
>  
> -	if (agcAlgo_->updateControls())
> -		awbAlgo_->prepare(context_, params_);
> +	awbAlgo_->prepare(context_, params_);
>  
>  	*params = params_;
>  
> @@ -338,8 +337,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>  
>  	awbAlgo_->process(context_, stats);
>  
> -	if (agcAlgo_->updateControls())
> -		setControls(frame);
> +	setControls(frame);
>  
>  	/* \todo Use VBlank value calculated from each frame exposure. */
>  	int64_t frameDuration = sensorInfo_.lineLength * (defVBlank_ + sensorInfo_.outputSize.height) /
> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp
> index 0cc35b9f..0d0584a8 100644
> --- a/src/ipa/ipu3/ipu3_agc.cpp
> +++ b/src/ipa/ipu3/ipu3_agc.cpp
> @@ -51,9 +51,8 @@ static constexpr double kEvGainTarget = 0.5;
>  static constexpr uint8_t kCellSize = 8;
>  
>  IPU3Agc::IPU3Agc()
> -	: frameCount_(0), lastFrame_(0), converged_(false),
> -	  updateControls_(false), iqMean_(0.0),
> -	  lineDuration_(0s), maxExposureTime_(0s),
> +	: frameCount_(0), lastFrame_(0),
> +	  iqMean_(0.0), lineDuration_(0s), maxExposureTime_(0s),
>  	  prevExposure_(0s), prevExposureNoDg_(0s),
>  	  currentExposure_(0s), currentExposureNoDg_(0s)
>  {
> @@ -146,8 +145,6 @@ void IPU3Agc::filterExposure()
>  
>  void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)
>  {
> -	updateControls_ = false;
> -
>  	/* Algorithm initialization should wait for first valid frames */
>  	/* \todo - have a number of frames given by DelayedControls ?
>  	 * - implement a function for IIR */
> @@ -157,7 +154,6 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)
>  	/* Are we correctly exposed ? */
>  	if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {
>  		LOG(IPU3Agc, Debug) << "!!! Good exposure with iqMean = " << iqMean_;
> -		converged_ = true;
>  	} else {
>  		double newGain = kEvGainTarget * knumHistogramBins / iqMean_;
>  
> @@ -180,12 +176,10 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)
>  			exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / currentExposureNoDg_), kMinExposure, kMaxExposure);
>  			newExposure = currentExposure_ / exposure;
>  			gain = std::clamp(static_cast<uint32_t>(gain * currentExposure_ / newExposure), kMinGain, kMaxGain);
> -			updateControls_ = true;
>  		} else if (currentShutter >= maxExposureTime_) {
>  			gain = std::clamp(static_cast<uint32_t>(gain * currentExposure_ / currentExposureNoDg_), kMinGain, kMaxGain);
>  			newExposure = currentExposure_ / gain;
>  			exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / newExposure), kMinExposure, kMaxExposure);
> -			updateControls_ = true;
>  		}
>  		LOG(IPU3Agc, Debug) << "Adjust exposure " << exposure * lineDuration_ << " and gain " << gain;
>  	}
> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h
> index e096648b..0e922664 100644
> --- a/src/ipa/ipu3/ipu3_agc.h
> +++ b/src/ipa/ipu3/ipu3_agc.h
> @@ -31,8 +31,6 @@ public:
>  
>  	int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
>  	void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
> -	bool converged() { return converged_; }
> -	bool updateControls() { return updateControls_; }
>  
>  private:
>  	void processBrightness(const ipu3_uapi_stats_3a *stats);
> @@ -44,9 +42,6 @@ private:
>  	uint64_t frameCount_;
>  	uint64_t lastFrame_;
>  
> -	bool converged_;
> -	bool updateControls_;
> -
>  	double iqMean_;
>  
>  	Duration lineDuration_;
>
Laurent Pinchart Aug. 15, 2021, 11:59 p.m. UTC | #2 
Hi Jean-Michel,

Thank you for the patch.

On Fri, Aug 13, 2021 at 11:06:11AM +0100, Kieran Bingham wrote:
> On 12/08/2021 17:52, Jean-Michel Hautbois wrote:
> > We never use converged_ so remove its declaration.
> > The controls may not need to be updated at each call, but it should be
> > decided on the context side and not by a specific call.

Could you elaborate on this ? Specifically, the comment message seems to
imply that this is a useful feature, but that it should be implemented
differently. How do you plan to implement it ?

> Good, I see how this helps move towards making it more modular.
> I guess the short term effect is that the controls will be set for every
> frame, but if the values don't change, then that's likely a no-op in the
> kernel anyway so I dont' think that's an issue.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> > ---
> >  src/ipa/ipu3/ipu3.cpp     |  6 ++----
> >  src/ipa/ipu3/ipu3_agc.cpp | 10 ++--------
> >  src/ipa/ipu3/ipu3_agc.h   |  5 -----
> >  3 files changed, 4 insertions(+), 17 deletions(-)
> > 
> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > index 2a8225a0..27765aa6 100644
> > --- a/src/ipa/ipu3/ipu3.cpp
> > +++ b/src/ipa/ipu3/ipu3.cpp
> > @@ -309,8 +309,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
> >  	for (auto const &algo : algorithms_)
> >  		algo->prepare(context_, params_);
> >  
> > -	if (agcAlgo_->updateControls())
> > -		awbAlgo_->prepare(context_, params_);
> > +	awbAlgo_->prepare(context_, params_);
> >  
> >  	*params = params_;
> >  
> > @@ -338,8 +337,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,
> >  
> >  	awbAlgo_->process(context_, stats);
> >  
> > -	if (agcAlgo_->updateControls())
> > -		setControls(frame);
> > +	setControls(frame);
> >  
> >  	/* \todo Use VBlank value calculated from each frame exposure. */
> >  	int64_t frameDuration = sensorInfo_.lineLength * (defVBlank_ + sensorInfo_.outputSize.height) /
> > diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp
> > index 0cc35b9f..0d0584a8 100644
> > --- a/src/ipa/ipu3/ipu3_agc.cpp
> > +++ b/src/ipa/ipu3/ipu3_agc.cpp
> > @@ -51,9 +51,8 @@ static constexpr double kEvGainTarget = 0.5;
> >  static constexpr uint8_t kCellSize = 8;
> >  
> >  IPU3Agc::IPU3Agc()
> > -	: frameCount_(0), lastFrame_(0), converged_(false),
> > -	  updateControls_(false), iqMean_(0.0),
> > -	  lineDuration_(0s), maxExposureTime_(0s),
> > +	: frameCount_(0), lastFrame_(0),
> > +	  iqMean_(0.0), lineDuration_(0s), maxExposureTime_(0s),
> >  	  prevExposure_(0s), prevExposureNoDg_(0s),
> >  	  currentExposure_(0s), currentExposureNoDg_(0s)

You can reflow this as

	: frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),
	  maxExposureTime_(0s), prevExposure_(0s), prevExposureNoDg_(0s),
  	  currentExposure_(0s), currentExposureNoDg_(0s)

> >  {
> > @@ -146,8 +145,6 @@ void IPU3Agc::filterExposure()
> >  
> >  void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)
> >  {
> > -	updateControls_ = false;
> > -
> >  	/* Algorithm initialization should wait for first valid frames */
> >  	/* \todo - have a number of frames given by DelayedControls ?
> >  	 * - implement a function for IIR */
> > @@ -157,7 +154,6 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)
> >  	/* Are we correctly exposed ? */
> >  	if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {
> >  		LOG(IPU3Agc, Debug) << "!!! Good exposure with iqMean = " << iqMean_;
> > -		converged_ = true;
> >  	} else {
> >  		double newGain = kEvGainTarget * knumHistogramBins / iqMean_;
> >  
> > @@ -180,12 +176,10 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)
> >  			exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / currentExposureNoDg_), kMinExposure, kMaxExposure);
> >  			newExposure = currentExposure_ / exposure;
> >  			gain = std::clamp(static_cast<uint32_t>(gain * currentExposure_ / newExposure), kMinGain, kMaxGain);
> > -			updateControls_ = true;
> >  		} else if (currentShutter >= maxExposureTime_) {
> >  			gain = std::clamp(static_cast<uint32_t>(gain * currentExposure_ / currentExposureNoDg_), kMinGain, kMaxGain);
> >  			newExposure = currentExposure_ / gain;
> >  			exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / newExposure), kMinExposure, kMaxExposure);
> > -			updateControls_ = true;
> >  		}
> >  		LOG(IPU3Agc, Debug) << "Adjust exposure " << exposure * lineDuration_ << " and gain " << gain;
> >  	}
> > diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h
> > index e096648b..0e922664 100644
> > --- a/src/ipa/ipu3/ipu3_agc.h
> > +++ b/src/ipa/ipu3/ipu3_agc.h
> > @@ -31,8 +31,6 @@ public:
> >  
> >  	int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
> >  	void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
> > -	bool converged() { return converged_; }
> > -	bool updateControls() { return updateControls_; }
> >  
> >  private:
> >  	void processBrightness(const ipu3_uapi_stats_3a *stats);
> > @@ -44,9 +42,6 @@ private:
> >  	uint64_t frameCount_;
> >  	uint64_t lastFrame_;
> >  
> > -	bool converged_;
> > -	bool updateControls_;
> > -
> >  	double iqMean_;
> >  
> >  	Duration lineDuration_;

Patch
diff mbox series 

diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
index 2a8225a0..27765aa6 100644
--- a/src/ipa/ipu3/ipu3.cpp
+++ b/src/ipa/ipu3/ipu3.cpp
@@ -309,8 +309,7 @@  void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
 	for (auto const &algo : algorithms_)
 		algo->prepare(context_, params_);
 
-	if (agcAlgo_->updateControls())
-		awbAlgo_->prepare(context_, params_);
+	awbAlgo_->prepare(context_, params_);
 
 	*params = params_;
 
@@ -338,8 +337,7 @@  void IPAIPU3::parseStatistics(unsigned int frame,
 
 	awbAlgo_->process(context_, stats);
 
-	if (agcAlgo_->updateControls())
-		setControls(frame);
+	setControls(frame);
 
 	/* \todo Use VBlank value calculated from each frame exposure. */
 	int64_t frameDuration = sensorInfo_.lineLength * (defVBlank_ + sensorInfo_.outputSize.height) /
diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp
index 0cc35b9f..0d0584a8 100644
--- a/src/ipa/ipu3/ipu3_agc.cpp
+++ b/src/ipa/ipu3/ipu3_agc.cpp
@@ -51,9 +51,8 @@  static constexpr double kEvGainTarget = 0.5;
 static constexpr uint8_t kCellSize = 8;
 
 IPU3Agc::IPU3Agc()
-	: frameCount_(0), lastFrame_(0), converged_(false),
-	  updateControls_(false), iqMean_(0.0),
-	  lineDuration_(0s), maxExposureTime_(0s),
+	: frameCount_(0), lastFrame_(0),
+	  iqMean_(0.0), lineDuration_(0s), maxExposureTime_(0s),
 	  prevExposure_(0s), prevExposureNoDg_(0s),
 	  currentExposure_(0s), currentExposureNoDg_(0s)
 {
@@ -146,8 +145,6 @@  void IPU3Agc::filterExposure()
 
 void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)
 {
-	updateControls_ = false;
-
 	/* Algorithm initialization should wait for first valid frames */
 	/* \todo - have a number of frames given by DelayedControls ?
 	 * - implement a function for IIR */
@@ -157,7 +154,6 @@  void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)
 	/* Are we correctly exposed ? */
 	if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {
 		LOG(IPU3Agc, Debug) << "!!! Good exposure with iqMean = " << iqMean_;
-		converged_ = true;
 	} else {
 		double newGain = kEvGainTarget * knumHistogramBins / iqMean_;
 
@@ -180,12 +176,10 @@  void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)
 			exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / currentExposureNoDg_), kMinExposure, kMaxExposure);
 			newExposure = currentExposure_ / exposure;
 			gain = std::clamp(static_cast<uint32_t>(gain * currentExposure_ / newExposure), kMinGain, kMaxGain);
-			updateControls_ = true;
 		} else if (currentShutter >= maxExposureTime_) {
 			gain = std::clamp(static_cast<uint32_t>(gain * currentExposure_ / currentExposureNoDg_), kMinGain, kMaxGain);
 			newExposure = currentExposure_ / gain;
 			exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / newExposure), kMinExposure, kMaxExposure);
-			updateControls_ = true;
 		}
 		LOG(IPU3Agc, Debug) << "Adjust exposure " << exposure * lineDuration_ << " and gain " << gain;
 	}
diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h
index e096648b..0e922664 100644
--- a/src/ipa/ipu3/ipu3_agc.h
+++ b/src/ipa/ipu3/ipu3_agc.h
@@ -31,8 +31,6 @@  public:
 
 	int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
 	void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
-	bool converged() { return converged_; }
-	bool updateControls() { return updateControls_; }
 
 private:
 	void processBrightness(const ipu3_uapi_stats_3a *stats);
@@ -44,9 +42,6 @@  private:
 	uint64_t frameCount_;
 	uint64_t lastFrame_;
 
-	bool converged_;
-	bool updateControls_;
-
 	double iqMean_;
 
 	Duration lineDuration_;