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

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Mon, Apr 19, 2021 at 02:34:51PM +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.

Out of curiosity, is the inter-frame time something you would envision
being made dynamic in the future, maybe coming from the IPA
configuration file ?

> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  src/ipa/raspberrypi/raspberrypi.cpp           | 50 +++++++++++++++++--
>  .../pipeline/raspberrypi/raspberrypi.cpp      |  5 ++
>  2 files changed, 51 insertions(+), 4 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index f6d1ab16a290..eac1cf8cfe44 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) {

This doesn't apply cleanly. I'm afraid I've lost track, is this series
based on another pending series, or have other patches been merged in
the meantime that would require a rebase ?

> @@ -920,6 +936,32 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)
>  	if (data.embeddedBufferPresent)
>  		returnEmbeddedBuffer(data.embeddedBufferId);
>  
> +	if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&
> +	    frameTimestamp - lastRunTimestamp_ < controllerMinFrameDuration * 1e3) {

The intent, I assume, is to run the algorithms at max 60fps. Shouldn't
we allow for an error margin in the comparison ? Otherwise, with the
jitter, we would skip IPA runs for some frames when running at exactly
60fps.

> +		/*
> +		 * 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 merging the current frame's metadata into the
> +		 * previous frame's metadata object, and then moving the latter
> +		 * into the former.
> +		 */
> +		lastMetadata.Merge(rpiMetadata_);
> +		rpiMetadata_ = std::move(lastMetadata);
> +		processPending_ = false;
> +		LOG(IPARPI, Debug) << "Rate-limiting the controller! inter-frame duration: "
> +				   << frameTimestamp - lastRunTimestamp_
> +				   << ", min duration: "
> +				   << controllerMinFrameDuration * 1e3;

That would result in loooots of debug messages with a > 60fps frame
rate. Is it intended ?

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