[libcamera-devel] [PATCH v6 5/5] ipa: raspberrypi: Rate-limit the controller algorithms

[Laurent Pinchart]

Hi Naush,

On Sat, May 08, 2021 at 03:30:31AM +0300, Laurent Pinchart wrote:
> On Fri, May 07, 2021 at 09:40:42AM +0100, Naushir Patuck wrote:
> > The controller algorithms currently run on every frame provided to the
> > IPA by the pipeline handler. This may be undesirable for very fast fps
> > operating modes where it could significantly increase the computation
> > cycles (per unit time) without providing any significant changes to the
> > IQ parameters. The added latencies could also cause dropped frames.
> > 
> > Pass the FrameBuffer timestamp to the IPA through the controls. This
> > timestamp will be used to rate-limit the controller algorithms to run
> > with a minimum inter-frame time given by a compile time constant,
> > currently set to 16.66ms. On startup, we don't rate-limit the algorithms
> > until after the number of frames required for convergence.
> > 
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> >  src/ipa/raspberrypi/raspberrypi.cpp           | 48 +++++++++++++++++--
> >  .../pipeline/raspberrypi/raspberrypi.cpp      |  5 ++
> >  2 files changed, 49 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index f6d1ab16a290..57f5497a6e6b 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -61,6 +61,14 @@ constexpr unsigned int DefaultExposureTime = 20000;
> >  constexpr double defaultMinFrameDuration = 1e6 / 30.0;
> >  constexpr double defaultMaxFrameDuration = 1e6 / 0.01;
> >  
> > +/*
> > + * Determine the minimum allowable inter-frame duration (in us) to run the
> > + * controller algorithms. If the pipeline handler provider frames at a rate
> > + * higher than this, we rate-limit the controller Prepare() and Process() calls
> > + * to lower than or equal to this rate.
> > + */
> > +constexpr double controllerMinFrameDuration = 1e6 / 60.0;
> > +
> >  LOG_DEFINE_CATEGORY(IPARPI)
> >  
> >  class IPARPi : public ipa::RPi::IPARPiInterface
> > @@ -68,7 +76,7 @@ class IPARPi : public ipa::RPi::IPARPiInterface
> >  public:
> >  	IPARPi()
> >  		: controller_(), frameCount_(0), checkCount_(0), mistrustCount_(0),
> > -		  lsTable_(nullptr), firstStart_(true)
> > +		  lastRunTimestamp_(0), lsTable_(nullptr), firstStart_(true)
> >  	{
> >  	}
> >  
> > @@ -146,6 +154,12 @@ private:
> >  	/* Number of frames that need to be dropped on startup. */
> >  	unsigned int dropFrameCount_;
> >  
> > +	/* Frame timestamp for the last run of the controller. */
> > +	uint64_t lastRunTimestamp_;
> > +
> > +	/* Do we run a Controller::process() for this frame? */
> > +	bool processPending_;
> > +
> >  	/* LS table allocation passed in from the pipeline handler. */
> >  	FileDescriptor lsTableHandle_;
> >  	void *lsTable_;
> > @@ -262,6 +276,7 @@ void IPARPi::start(const ControlList &controls, ipa::RPi::StartConfig *startConf
> >  	startConfig->dropFrameCount = dropFrameCount_;
> >  
> >  	firstStart_ = false;
> > +	lastRunTimestamp_ = 0;
> >  }
> >  
> >  void IPARPi::setMode(const CameraSensorInfo &sensorInfo)
> > @@ -406,7 +421,7 @@ void IPARPi::signalStatReady(uint32_t bufferId)
> >  {
> >  	if (++checkCount_ != frameCount_) /* assert here? */
> >  		LOG(IPARPI, Error) << "WARNING: Prepare/Process mismatch!!!";
> > -	if (frameCount_ > mistrustCount_)
> > +	if (processPending_ && frameCount_ > mistrustCount_)
> >  		processStats(bufferId);
> >  
> >  	reportMetadata();
> > @@ -894,10 +909,11 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)
> >  
> >  void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)
> >  {
> > +	int64_t frameTimestamp = data.controls.get(controls::draft::SensorTimestamp);

Actually the series doesn't compile, as controls::draft::SensorTimestamp
has become controls::SensorTimestamp. This should be easy to fix, but
would you mind sending a v7 ? I'd be more comfortable merging a properly
tested series than updating this when applying.

> > +	RPiController::Metadata lastMetadata;
> >  	Span<uint8_t> embeddedBuffer;
> >  
> > -	rpiMetadata_.Clear();
> > -
> > +	lastMetadata = std::move(rpiMetadata_);
> >  	fillDeviceStatus(data.controls);
> >  
> >  	if (data.embeddedBufferPresent) {
> > @@ -920,6 +936,30 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)
> >  	if (data.embeddedBufferPresent)
> >  		returnEmbeddedBuffer(data.embeddedBufferId);
> >  
> > +	/* Allow a 10% margin on the comparison below. */
> > +	constexpr double eps = controllerMinFrameDuration * 1e3 * 0.1;
> > +	if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&
> > +	    frameTimestamp - lastRunTimestamp_ + eps < controllerMinFrameDuration * 1e3) {
> 
> I would likely have written this
> 
> 	if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&
> 	    frameTimestamp - lastRunTimestamp_ + eps < controllerMinFrameDuration * 1e3 * 0.9) {
> 
> but it doesn't matter much.
> 
> > +		/*
> > +		 * Ensure we update the controller metadata with the new frame's
> > +		 * exposure/gain values so that the correct values are returned
> > +		 * out in libcamera metadata later on. Any other bits of metadata
> > +		 * that were added in helper_->Prepare() will also be moved across.
> > +		 * All other metadata values must remain the same as the last frame.
> > +		 *
> > +		 * We do this by overwriting the current frame's metadata into the
> > +		 * previous frame's metadata object, and then swapping the latter
> 
> "overwriting ... into" sounds weird. If you can provide an updated text,
> I can fix when applying.
> 
> > +		 * with the former.
> > +		 */
> > +		lastMetadata.Overwrite(rpiMetadata_);
> > +		std::swap(rpiMetadata_, lastMetadata);
> 
> This could have been written
> 
> 		rpiMetadata_.Merge(lastMetadata);
> 
> without a swap, with std::map::merge semantics for Metadata::Merge() :-)
> The double swap seems a bit cumbersome.
> 
> If you're happy with the current implementation though, that's fine with
> me. In either case,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> If you think Merge() would be cleaner, I'll merge v7.
> 
> > +		processPending_ = false;
> > +		return;
> > +	}
> > +
> > +	lastRunTimestamp_ = frameTimestamp;
> > +	processPending_ = true;
> > +
> >  	ControlList ctrls(ispCtrls_);
> >  
> >  	controller_.Prepare(&rpiMetadata_);
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 2a917455500f..9cf9c8c6cebd 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -1414,6 +1414,11 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
> >  		 * DelayedControl and queue them along with the frame buffer.
> >  		 */
> >  		ControlList ctrl = delayedCtrls_->get(buffer->metadata().sequence);
> > +		/*
> > +		 * Add the frame timestamp to the ControlList for the IPA to use
> > +		 * as it does not receive the FrameBuffer object.
> > +		 */
> > +		ctrl.set(controls::draft::SensorTimestamp, buffer->metadata().timestamp);
> >  		bayerQueue_.push({ buffer, std::move(ctrl) });
> >  	} else {
> >  		embeddedQueue_.push(buffer);

-- 
Regards,

Laurent Pinchart