<div dir="ltr"><div dir="ltr">Hi David,<div><br></div><div>Thank you for your feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 10 May 2021 at 14:35, David Plowman <<a href="mailto:david.plowman@raspberrypi.com">david.plowman@raspberrypi.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush<br>
<br>
Thanks for this patch. For reasons unknown, gmail seems to have tacked<br>
it on as a reply to the cover note...<br>
<br>
On Mon, 10 May 2021 at 10:59, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">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. On startup, we don't rate-limit the algorithms<br>
> until after the number of frames required for convergence.<br>
><br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
> src/ipa/raspberrypi/raspberrypi.cpp | 42 +++++++++++++++++--<br>
> .../pipeline/raspberrypi/raspberrypi.cpp | 5 +++<br>
> 2 files changed, 43 insertions(+), 4 deletions(-)<br>
><br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index 07409621a03a..52d91db282ea 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>
> + * 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>
<br>
I guess the only thing that caused my brain to pause for a moment was<br>
the need to convince myself that processPending_ can't be<br>
uninitialised. But prepareISP always sets it and must run before this,<br>
so we're all good...<br></blockquote><div><br></div><div>Yes, this is correct.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<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::SensorTimestamp);<br>
<br>
Just wondering if there's any possibility that the first time stamp<br>
could be zero, thereby matching the initial value of lastRunTimestamp.<br>
The result would be that we start running the control algos a frame or<br>
so late. If we're happy that this doesn't happen, then I'm OK with it<br>
too.<br></blockquote><div><br></div><div>I don't think the first timestamp from the kernel can ever be zero. But I do also test</div><div>that lastRunTimestamp_ > 0 in the calculation below.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<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,24 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)<br>
> if (data.embeddedBufferPresent)<br>
> returnEmbeddedBuffer(data.embeddedBufferId);<br>
><br>
> + /* Allow a 10% margin on the comparison below. */<br>
> + constexpr double eps = controllerMinFrameDuration * 1e3 * 0.1;<br>
> + if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&<br>
> + frameTimestamp - lastRunTimestamp_ + eps < controllerMinFrameDuration * 1e3) {<br>
<br>
Might this be easier to read if we defined all these things in<br>
nanoseconds right from the start? But I really can't get too worked up<br>
over it!<br></blockquote><div><br></div><div>Yes I agree. However, I would prefer to leave this as-is for now, as it matches the other time</div><div>based constants. One of my next tasks is to switch to using std::chrono::duration for our time base</div><div>variables, which remove these const factors.</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
<br>
Best regards<br>
David<br>
<br>
> + /*<br>
> + * Ensure we merge the previous frame's metadata with the current<br>
> + * frame. This will not overwrite exposure/gain values for the<br>
> + * current frame, or any other bits of metadata that were added<br>
> + * in helper_->Prepare().<br>
> + */<br>
> + rpiMetadata_.Merge(lastMetadata);<br>
> + processPending_ = false;<br>
> + return;<br>
> + }<br>
> +<br>
> + lastRunTimestamp_ = frameTimestamp;<br>
> + processPending_ = true;<br>
> +<br>
> ControlList ctrls(ispCtrls_);<br>
><br>
> controller_.Prepare(&rpiMetadata_);<br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index b22564938cdc..d26ae2a9772a 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -1426,6 +1426,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::SensorTimestamp, buffer->metadata().timestamp);<br>
> bayerQueue_.push({ buffer, std::move(ctrl) });<br>
> } else {<br>
> embeddedQueue_.push(buffer);<br>
> --<br>
> 2.25.1<br>
><br>
> _______________________________________________<br>
> libcamera-devel mailing list<br>
> <a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
> <a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
</blockquote></div></div>