[libcamera-devel] [PATCH v2 3/3] ipa: raspberrypi: Rate-limit the controller algorithms
Naushir Patuck
naush at raspberrypi.com
Fri Apr 16 18:30:19 CEST 2021
Hi David,
On Fri, 16 Apr 2021, 5:00 pm David Plowman, <david.plowman at raspberrypi.com>
wrote:
> Hi Naush
>
> Thanks for explaining.
>
> On Fri, 16 Apr 2021 at 15:36, Naushir Patuck <naush at raspberrypi.com>
> wrote:
> >
> > Hi David,
> >
> > Thank you for your feedback.
> >
> > On Fri, 16 Apr 2021 at 14:44, David Plowman <
> david.plowman at raspberrypi.com> wrote:
> >>
> >> 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...
> >
> >
> > I think that is gmail causing issues by bunching 0/3 and 3/3 into a
> conversation
> > for some reason, even though the subjects are (slightly) different.
> Happens
> > on my view as well!
> >
> >>
> >>
> >> 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 did try this type of logic initially, but I don't think it would work.
> >
> > We need to call rpiMetadata_.Clear() and fillDeviceStatus() before
> > helper_->Prepare() is called at the beginning of the function. I cannot
> > move the latter call further down as it needs to run before my skip algos
> > check and the returnEmbeddedBuffer() call. So I think the existing
> manoeuvre
> > is needed without more refactoring.
>
> Ah yes, indeed, you're right - that helper_->Prepare() is troublesome.
> Back to Plan A then...
>
> Actually one more little annoyance comes to mind. What happens if
> helper_->Prepare() gives us more metadata than just the DeviceStatus.
> Would we expect that to get copied over the previous metadata too? In
> practice I would guess it won't matter if that metadata can't escape
> to the application, though I think we can already imagine future
> sensors where that might happen. What do you think? Would a "merge"
> method (which "moves" the data items) be the fix for this? Am I
> over-thinking?
>
I think this is one to worry about in the future.
A merge method would be useful in these situations, but given that we only
really care about exposure and gain right now, propose we kick the can down
the road.
Regards,
Naush
> Best regards
> David
>
> >
> > Regards,
> > Naush
> >
> >>
> >>
> >> 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
> >> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210416/39c4aa3d/attachment-0001.htm>
More information about the libcamera-devel
mailing list