<div dir="auto">Hi David,<br><br><div class="gmail_quote" dir="auto"><div dir="ltr" class="gmail_attr">On Fri, 16 Apr 2021, 5:00 pm David Plowman, <<a href="mailto:david.plowman@raspberrypi.com">david.plowman@raspberrypi.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Naush<br>
<br>
Thanks for explaining.<br>
<br>
On Fri, 16 Apr 2021 at 15:36, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank" rel="noreferrer">naush@raspberrypi.com</a>> wrote:<br>
><br>
> Hi David,<br>
><br>
> Thank you for your feedback.<br>
><br>
> On Fri, 16 Apr 2021 at 14:44, David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank" rel="noreferrer">david.plowman@raspberrypi.com</a>> wrote:<br>
>><br>
>> Hi Naush<br>
>><br>
>> Thanks for version 2 of this patch, and especially for re-basing on<br>
>> top of my still-pending patch!<br>
>><br>
>> Weirdly this one, which I'd have expected to be the "3/3" has ended up<br>
>> as a reply to the cover note, so not sure what's going on there. But<br>
>> anyway...<br>
><br>
><br>
> I think that is gmail causing issues by bunching 0/3 and 3/3 into a conversation<br>
> for some reason, even though the subjects are (slightly) different. Happens<br>
> on my view as well!<br>
><br>
>><br>
>><br>
>> On Fri, 16 Apr 2021 at 11:31, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank" rel="noreferrer">naush@raspberrypi.com</a>> wrote:<br>
>> ><br>
>> > The controller algorithms currently run on every frame provided to the<br>
>> > IPA by the pipeline handler. This may be undesirable for very fast fps<br>
>> > operating modes where it could significantly increase the computation<br>
>> > cycles (per unit time) without providing any significant changes to the<br>
>> > IQ parameters. The added latencies could also cause dropped frames.<br>
>> ><br>
>> > Pass the FrameBuffer timestamp to the IPA through the controls. This<br>
>> > timestamp will be used to rate-limit the controller algorithms to run<br>
>> > with a minimum inter-frame time given by a compile time constant,<br>
>> > currently set to 16.66ms.<br>
>><br>
>> Is it worth a remark on how we don't rate-limit while dropping frames?<br>
>> I don't really mind, though.<br>
>><br>
>><br>
>> ><br>
>> > Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank" rel="noreferrer">naush@raspberrypi.com</a>><br>
>> > ---<br>
>> > src/ipa/raspberrypi/raspberrypi.cpp | 48 +++++++++++++++++--<br>
>> > .../pipeline/raspberrypi/raspberrypi.cpp | 5 ++<br>
>> > 2 files changed, 49 insertions(+), 4 deletions(-)<br>
>> ><br>
>> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
>> > index f6d1ab16a290..e96b169ca612 100644<br>
>> > --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
>> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
>> > @@ -61,6 +61,14 @@ constexpr unsigned int DefaultExposureTime = 20000;<br>
>> > constexpr double defaultMinFrameDuration = 1e6 / 30.0;<br>
>> > constexpr double defaultMaxFrameDuration = 1e6 / 0.01;<br>
>> ><br>
>> > +/*<br>
>> > + * Determine the minimum allowable inter-frame duration (in us) to run the<br>
>> > + * controller algorithms. If the pipeline handler provider frames at a rate<br>
>> > + * higher than this, we rate-limit the controller prepare() and process() calls<br>
>><br>
>> Strictly speaking, I guess they're still Prepare() and Process()...<br>
>> (until such time as the lower-casing elves strike those files!)<br>
>><br>
>> > + * to lower than or equal to this rate.<br>
>> > + */<br>
>> > +constexpr double controllerMinFrameDuration = 1e6 / 60.0;<br>
>> > +<br>
>> > LOG_DEFINE_CATEGORY(IPARPI)<br>
>> ><br>
>> > class IPARPi : public ipa::RPi::IPARPiInterface<br>
>> > @@ -68,7 +76,7 @@ class IPARPi : public ipa::RPi::IPARPiInterface<br>
>> > public:<br>
>> > IPARPi()<br>
>> > : controller_(), frameCount_(0), checkCount_(0), mistrustCount_(0),<br>
>> > - lsTable_(nullptr), firstStart_(true)<br>
>> > + lastRunTimestamp_(0), lsTable_(nullptr), firstStart_(true)<br>
>> > {<br>
>> > }<br>
>> ><br>
>> > @@ -146,6 +154,12 @@ private:<br>
>> > /* Number of frames that need to be dropped on startup. */<br>
>> > unsigned int dropFrameCount_;<br>
>> ><br>
>> > + /* Frame timestamp for the last run of the controller. */<br>
>> > + uint64_t lastRunTimestamp_;<br>
>> > +<br>
>> > + /* Do we run a Controller::process() for this frame? */<br>
>> > + bool processPending_;<br>
>> > +<br>
>> > /* LS table allocation passed in from the pipeline handler. */<br>
>> > FileDescriptor lsTableHandle_;<br>
>> > void *lsTable_;<br>
>> > @@ -262,6 +276,7 @@ void IPARPi::start(const ControlList &controls, ipa::RPi::StartConfig *startConf<br>
>> > startConfig->dropFrameCount = dropFrameCount_;<br>
>> ><br>
>> > firstStart_ = false;<br>
>> > + lastRunTimestamp_ = 0;<br>
>> > }<br>
>> ><br>
>> > void IPARPi::setMode(const CameraSensorInfo &sensorInfo)<br>
>> > @@ -406,7 +421,7 @@ void IPARPi::signalStatReady(uint32_t bufferId)<br>
>> > {<br>
>> > if (++checkCount_ != frameCount_) /* assert here? */<br>
>> > LOG(IPARPI, Error) << "WARNING: Prepare/Process mismatch!!!";<br>
>> > - if (frameCount_ > mistrustCount_)<br>
>> > + if (processPending_ && frameCount_ > mistrustCount_)<br>
>> > processStats(bufferId);<br>
>> ><br>
>> > reportMetadata();<br>
>> > @@ -894,10 +909,11 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)<br>
>> ><br>
>> > void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)<br>
>> > {<br>
>> > + int64_t frameTimestamp = data.controls.get(controls::draft::SensorTimestamp);<br>
>> > + RPiController::Metadata lastMetadata;<br>
>> > Span<uint8_t> embeddedBuffer;<br>
>> ><br>
>> > - rpiMetadata_.Clear();<br>
>> > -<br>
>> > + lastMetadata = std::move(rpiMetadata_);<br>
>> > fillDeviceStatus(data.controls);<br>
>> ><br>
>> > if (data.embeddedBufferPresent) {<br>
>> > @@ -920,6 +936,30 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)<br>
>> > if (data.embeddedBufferPresent)<br>
>> > returnEmbeddedBuffer(data.embeddedBufferId);<br>
>> ><br>
>> > + if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&<br>
>> > + frameTimestamp - lastRunTimestamp_ < controllerMinFrameDuration * 1e3) {<br>
>> > + /*<br>
>> > + * Ensure we update the controller metadata with the new frame's<br>
>> > + * exposure/gain values so that the correct values are returned<br>
>> > + * out in libcamera metadata later on. All other metadata values<br>
>> > + * must remain the same as the last frame.<br>
>> > + */<br>
>> > + DeviceStatus currentDeviceStatus;<br>
>> > +<br>
>> > + rpiMetadata_.Get<DeviceStatus>("device.status", currentDeviceStatus);<br>
>> > + rpiMetadata_ = std::move(lastMetadata);<br>
>> > + rpiMetadata_.Set("device.status", currentDeviceStatus);<br>
>> > + processPending_ = false;<br>
>> > + LOG(IPARPI, Debug) << "Rate-limiting the controller! inter-frame duration: "<br>
>> > + << frameTimestamp - lastRunTimestamp_<br>
>> > + << ", min duration "<br>
>> > + << controllerMinFrameDuration * 1e3;<br>
>> > + return;<br>
>> > + }<br>
>> > +<br>
>> > + lastRunTimestamp_ = frameTimestamp;<br>
>> > + processPending_ = true;<br>
>> > +<br>
>> > ControlList ctrls(ispCtrls_);<br>
>><br>
>> Hmm, yes, I see there's an interesting manoeuvre going on with the<br>
>> metadata. I wonder if there's a way to rearrange this that involves a<br>
>> bit less shuffling, maybe:<br>
>><br>
>> first delete these lines:<br>
>> + lastMetadata = std::move(rpiMetadata_);<br>
>> fillDeviceStatus(data.controls);<br>
>><br>
>> next, in place of:<br>
>> + DeviceStatus currentDeviceStatus;<br>
>> +<br>
>> + rpiMetadata_.Get<DeviceStatus>("device.status",<br>
>> currentDeviceStatus);<br>
>> + rpiMetadata_ = std::move(lastMetadata);<br>
>> + rpiMetadata_.Set("device.status", currentDeviceStatus);<br>
>><br>
>> do this:<br>
>> fillDeviceStatus(data.controls);<br>
>><br>
>> and finally after the if-block for the "skip the algos" case put these back:<br>
>> rpiMetadata_.Clear();<br>
>> fillDeviceStatus(data.controls);<br>
><br>
><br>
> I did try this type of logic initially, but I don't think it would work.<br>
><br>
> We need to call rpiMetadata_.Clear() and fillDeviceStatus() before<br>
> helper_->Prepare() is called at the beginning of the function. I cannot<br>
> move the latter call further down as it needs to run before my skip algos<br>
> check and the returnEmbeddedBuffer() call. So I think the existing manoeuvre<br>
> is needed without more refactoring.<br>
<br>
Ah yes, indeed, you're right - that helper_->Prepare() is troublesome.<br>
Back to Plan A then...<br>
<br>
Actually one more little annoyance comes to mind. What happens if<br>
helper_->Prepare() gives us more metadata than just the DeviceStatus.<br>
Would we expect that to get copied over the previous metadata too? In<br>
practice I would guess it won't matter if that metadata can't escape<br>
to the application, though I think we can already imagine future<br>
sensors where that might happen. What do you think? Would a "merge"<br>
method (which "moves" the data items) be the fix for this? Am I<br>
over-thinking?<br></blockquote></div><div dir="auto"><br></div><div dir="auto">I think this is one to worry about in the future.</div><div dir="auto">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.</div><div dir="auto"><br></div><div dir="auto">Regards,</div><div dir="auto">Naush</div><div dir="auto"><br></div><div class="gmail_quote" dir="auto"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Best regards<br>
David<br>
<br>
><br>
> Regards,<br>
> Naush<br>
><br>
>><br>
>><br>
>> I don't know if this ends up tidier or not, perhaps it's best if you<br>
>> decide what you prefer. But I'm fine either way, so:<br>
>><br>
>> Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank" rel="noreferrer">david.plowman@raspberrypi.com</a>><br>
>><br>
>> Thanks!<br>
>> David<br>
>><br>
>> ><br>
>> > controller_.Prepare(&rpiMetadata_);<br>
>> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
>> > index 2a917455500f..9cf9c8c6cebd 100644<br>
>> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
>> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
>> > @@ -1414,6 +1414,11 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)<br>
>> > * DelayedControl and queue them along with the frame buffer.<br>
>> > */<br>
>> > ControlList ctrl = delayedCtrls_->get(buffer->metadata().sequence);<br>
>> > + /*<br>
>> > + * Add the frame timestamp to the ControlList for the IPA to use<br>
>> > + * as it does not receive the FrameBuffer object.<br>
>> > + */<br>
>> > + ctrl.set(controls::draft::SensorTimestamp, buffer->metadata().timestamp);<br>
>> > bayerQueue_.push({ buffer, std::move(ctrl) });<br>
>> > } else {<br>
>> > embeddedQueue_.push(buffer);<br>
>> > --<br>
>> > 2.25.1<br>
>> ><br>
</blockquote></div></div>