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

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

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);
> +	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