[libcamera-devel] [PATCH v2 08/10] ipa: ipu3: Remove unneeded function calls in AGC

[Laurent Pinchart]

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 at ideasonboard.com>
> 
> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at 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_;

-- 
Regards,

Laurent Pinchart