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