[libcamera-devel] [PATCH v6 6/6] ipa: raspberrypi: Rate-limit the controller algorithms
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue May 11 01:58:08 CEST 2021
Hi Naush,
Thank you for the patch.
On Mon, May 10, 2021 at 02:43:42PM +0100, Naushir Patuck wrote:
> On Mon, 10 May 2021 at 14:35, David Plowman wrote:
> > Hi Naush
> >
> > Thanks for this patch. For reasons unknown, gmail seems to have tacked
> > it on as a reply to the cover note...
> >
> > On Mon, 10 May 2021 at 10:59, 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 | 42 +++++++++++++++++--
> > > .../pipeline/raspberrypi/raspberrypi.cpp | 5 +++
> > > 2 files changed, 43 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > > index 07409621a03a..52d91db282ea 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_)
> >
> > I guess the only thing that caused my brain to pause for a moment was
> > the need to convince myself that processPending_ can't be
> > uninitialised. But prepareISP always sets it and must run before this,
> > so we're all good...
>
> Yes, this is correct.
>
> > > 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::SensorTimestamp);
> >
> > Just wondering if there's any possibility that the first time stamp
> > could be zero, thereby matching the initial value of lastRunTimestamp.
> > The result would be that we start running the control algos a frame or
> > so late. If we're happy that this doesn't happen, then I'm OK with it
> > too.
>
> I don't think the first timestamp from the kernel can ever be zero. But I
> do also test
> that lastRunTimestamp_ > 0 in the calculation below.
>
> > > + RPiController::Metadata lastMetadata;
> > > Span<uint8_t> embeddedBuffer;
> > >
> > > - rpiMetadata_.Clear();
> > > -
> > > + lastMetadata = std::move(rpiMetadata_);
> > > fillDeviceStatus(data.controls);
> > >
> > > if (data.embeddedBufferPresent) {
> > > @@ -920,6 +936,24 @@ 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) {
> >
> > Might this be easier to read if we defined all these things in
> > nanoseconds right from the start? But I really can't get too worked up
> > over it!
>
> Yes I agree. However, I would prefer to leave this as-is for now, as it
> matches the other time
> based constants. One of my next tasks is to switch to using
> std::chrono::duration for our time base
> variables, which remove these const factors.
I'm thinking about doing the same in the libcamera core. And as part of
that work, switching everything to nanoseconds may be a code idea, to be
done with it once and for all.
> > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > + /*
> > > + * Ensure we merge the previous frame's metadata with the current
> > > + * frame. This will not overwrite exposure/gain values for the
> > > + * current frame, or any other bits of metadata that were added
> > > + * in helper_->Prepare().
> > > + */
> > > + rpiMetadata_.Merge(lastMetadata);
> > > + 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 b22564938cdc..d26ae2a9772a 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -1426,6 +1426,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::SensorTimestamp, buffer->metadata().timestamp);
> > > bayerQueue_.push({ buffer, std::move(ctrl) });
> > > } else {
> > > embeddedQueue_.push(buffer);
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list