[libcamera-devel] [PATCH v2 3/3] ipa: raspberrypi: Rate-limit the controller algorithms

[David Plowman]

Hi Naush

Thanks for version 2 of this patch, and especially for re-basing on
top of my still-pending patch!

Weirdly this one, which I'd have expected to be the "3/3" has ended up
as a reply to the cover note, so not sure what's going on there. But
anyway...

On Fri, 16 Apr 2021 at 11:31, Naushir Patuck <naush at raspberrypi.com> 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.

Is it worth a remark on how we don't rate-limit while dropping frames?
I don't really mind, though.

>
> 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..e96b169ca612 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

Strictly speaking, I guess they're still Prepare() and Process()...
(until such time as the lower-casing elves strike those files!)

> + * 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);
>
> +       if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&
> +           frameTimestamp - lastRunTimestamp_ < controllerMinFrameDuration * 1e3) {
> +               /*
> +                * 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. All other metadata values
> +                * must remain the same as the last frame.
> +                */
> +               DeviceStatus currentDeviceStatus;
> +
> +               rpiMetadata_.Get<DeviceStatus>("device.status", currentDeviceStatus);
> +               rpiMetadata_ = std::move(lastMetadata);
> +               rpiMetadata_.Set("device.status", currentDeviceStatus);
> +               processPending_ = false;
> +               LOG(IPARPI, Debug) << "Rate-limiting the controller! inter-frame duration: "
> +                                  << frameTimestamp - lastRunTimestamp_
> +                                  << ", min duration "
> +                                  << controllerMinFrameDuration * 1e3;
> +               return;
> +       }
> +
> +       lastRunTimestamp_ = frameTimestamp;
> +       processPending_ = true;
> +
>         ControlList ctrls(ispCtrls_);

Hmm, yes, I see there's an interesting manoeuvre going on with the
metadata. I wonder if there's a way to rearrange this that involves a
bit less shuffling, maybe:

first delete these lines:
+       lastMetadata = std::move(rpiMetadata_);
        fillDeviceStatus(data.controls);

next, in place of:
+               DeviceStatus currentDeviceStatus;
+
+               rpiMetadata_.Get<DeviceStatus>("device.status",
currentDeviceStatus);
+               rpiMetadata_ = std::move(lastMetadata);
+               rpiMetadata_.Set("device.status", currentDeviceStatus);

do this:
 fillDeviceStatus(data.controls);

and finally after the if-block for the "skip the algos" case put these back:
       rpiMetadata_.Clear();
        fillDeviceStatus(data.controls);

I don't know if this ends up tidier or not, perhaps it's best if you
decide what you prefer. But I'm fine either way, so:

Reviewed-by: David Plowman <david.plowman at raspberrypi.com>

Thanks!
David

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