[libcamera-devel] [PATCH v5 5/5] ipa: raspberrypi: Rate-limit the controller algorithms
David Plowman
david.plowman at raspberrypi.com
Tue Apr 27 11:19:04 CEST 2021
Hi Laurent
Thanks for the review. Answering at least one of your questions for
Naush (as it relates to one of my patches)...
On Tue, 27 Apr 2021 at 08:34, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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 ?
I believe Naush helpfully rebased this patch on top of my " ipa:
raspberrypi: Use CamHelpers to generalise sensor metadata parsing"
patch. I think that one is still pending...?
Thanks
David
>
> > @@ -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
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list