[libcamera-devel] [PATCH v2 3/3] ipa: raspberrypi: Rate-limit the controller algorithms
David Plowman
david.plowman at raspberrypi.com
Fri Apr 16 17:59:48 CEST 2021
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?
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
>> >
More information about the libcamera-devel
mailing list