<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 15 Apr 2021 at 17:01, 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>
On Thu, 15 Apr 2021 at 15:40, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>> wrote:<br>
><br>
> Hi David,<br>
><br>
> Thank you for your feedback.<br>
><br>
> On Thu, 15 Apr 2021 at 15:26, David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>> wrote:<br>
>><br>
>> Hi Naush<br>
>><br>
>> Thanks for this patch, I think it's a good idea!<br>
>><br>
>> On Thu, 15 Apr 2021 at 11:18, 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.<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 | 39 ++++++++++++++++++-<br>
>> > .../pipeline/raspberrypi/raspberrypi.cpp | 5 +++<br>
>> > 2 files changed, 42 insertions(+), 2 deletions(-)<br>
>> ><br>
>> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
>> > index dad6395f0823..aa071473d6e7 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>
>> It wasn't clear to me whether prepare() was being rate-limited too?<br>
>> More on that below...<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>
>> > @@ -145,6 +153,12 @@ private:<br>
>> > /* How many frames we should avoid running control algos on. */<br>
>> > unsigned int mistrustCount_;<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>
>> > @@ -406,7 +420,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>
>> Although not related directly to this bit of code, I wonder whether we<br>
>> have an interaction between skipping the algorithms and the frame<br>
>> dropping that we do at startup (while waiting for those algorithms to<br>
>> converge)? i.e. are we elapsing those "drop frames" at a faster rate<br>
>> than the IPA can run in order to achieve convergence during those<br>
>> frames?<br>
><br>
><br>
> Yes, this is possible. Perhaps I should change the logic a bit so that we<br>
> run controller prepare()/process() on every frame when frameCount_ <= mistrustCount_.<br>
> This is not going to cause any problems, as mistrustCount_ is only a few frames.<br>
> What do you think?<br>
<br>
I'm thinking that might not be quite right. Presumably we might want<br>
to consider running the algos more until the PH reaches its<br>
dropFrameCount (or whatever it's called). Presumably that number came<br>
from the IPA originally in some way - so we must know what it is,<br>
right?<br></blockquote><div><br></div><div>Sorry, mistrustCount_ is the wrong variable to test. I meant dropCount.</div><div>I will push a v2 with the changes and let me know what you think.</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>
><br>
>><br>
>><br>
>> ><br>
>> > reportMetadata();<br>
>> > @@ -894,6 +908,7 @@ 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>
>> > struct DeviceStatus deviceStatus = {};<br>
>> > bool success = false;<br>
>> ><br>
>> > @@ -919,6 +934,26 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)<br>
>> > fillDeviceStatus(exposureLines, gainCode, deviceStatus);<br>
>> > }<br>
>> ><br>
>> > + if (lastRunTimestamp_ &&<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>
>> > + rpiMetadata_.Set("device.status", deviceStatus);<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>
>> > rpiMetadata_.Clear();<br>
>><br>
>> So here I'm inferring that we don't skip prepare(). Mostly I think<br>
>> this is fine as prepare() does much less work. Probably ALSC is the<br>
>> worst as it interpolates lens shading tables.<br>
><br>
><br>
> I stopped running prepare() as I wanted to avoid the whole cycle of pulling<br>
> out controller metadata, setting ControlLists and writing controls to the v4l2 device.<br>
> All of this probably adds quite a bit of latency that we probably do not need.<br>
> Besides, running prepare() without running process() durning a normal<br>
> (or steady state) run seems a bit pointless to me, unless I missed something?<br>
><br>
>><br>
>> One little corner we might want to watch out for is something like the<br>
>> AE/AGC lock count. This counts in Agc::Prepare() how long things<br>
>> appear to be "steady", and if Process() runs less often than<br>
>> Prepare(), it seems possible that Prepare() may see things as steadier<br>
>> than they really are. Maybe we're ok, but it's just something to<br>
>> check.<br>
<br>
Ah OK, if you avoid Prepare() and Process() together then there can't<br>
be a problem. Maybe I didn't understand exactly what was happening<br>
here.<br>
<br>
>><br>
>> Also, I have a feeling we may get conflicts here with the<br>
>> CamHelper::Prepare/Process patch. Now it's a race to see who gets in<br>
>> first! :)<br>
><br>
><br>
> Yes, we do have a race on our hands here.<br>
<br>
Hmm, can I grab the Raspberry Pi IPA mutex, please?!!<br>
<br>
Thanks<br>
David<br>
<br>
><br>
> Regards,<br>
> Naush<br>
><br>
><br>
>><br>
>><br>
>> Thanks!<br>
>><br>
>> David<br>
>><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>