[libcamera-devel] [PATCH v5 5/5] ipa: raspberrypi: Rate-limit the controller algorithms
Naushir Patuck
naush at raspberrypi.com
Tue Apr 27 11:39:03 CEST 2021
Hi Laurent,
Thank you for your review feedback.
On Tue, 27 Apr 2021 at 08:34, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> Hi Naush,
>
> Thank you for the patch.
>
> On Mon, Apr 19, 2021 at 02:34:51PM +0100, Naushir Patuck 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. On startup, we don't rate-limit the algorithms
> > until after the number of frames required for convergence.
>
> Out of curiosity, is the inter-frame time something you would envision
> being made dynamic in the future, maybe coming from the IPA
> configuration file ?
>
I did question this myself, and to be honest I don't really know. Initially
I decided to make it a constant, but did wonder if we should make it
a config parameter for devices that are somewhat underpowered (e.g.
Pi Zero). In the end, I went with a constant value to avoid users
having another config parameter to think about. If we do decide the
other way, it is easy enough to switch.
>
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > ---
> > src/ipa/raspberrypi/raspberrypi.cpp | 50 +++++++++++++++++--
> > .../pipeline/raspberrypi/raspberrypi.cpp | 5 ++
> > 2 files changed, 51 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > index f6d1ab16a290..eac1cf8cfe44 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
> > + * 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) {
>
> This doesn't apply cleanly. I'm afraid I've lost track, is this series
> based on another pending series, or have other patches been merged in
> the meantime that would require a rebase ?
>
This commit will need to be applied over
https://patchwork.libcamera.org/patch/11730/
>
> > @@ -920,6 +936,32 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig
> &data)
> > if (data.embeddedBufferPresent)
> > returnEmbeddedBuffer(data.embeddedBufferId);
> >
> > + if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&
> > + frameTimestamp - lastRunTimestamp_ <
> controllerMinFrameDuration * 1e3) {
>
> The intent, I assume, is to run the algorithms at max 60fps. Shouldn't
> we allow for an error margin in the comparison ? Otherwise, with the
> jitter, we would skip IPA runs for some frames when running at exactly
> 60fps.
>
Good point, I will add a 10% margin on the comparison above.
>
> > + /*
> > + * 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. Any other bits of
> metadata
> > + * that were added in helper_->Prepare() will also be
> moved across.
> > + * All other metadata values must remain the same as the
> last frame.
> > + *
> > + * We do this by merging the current frame's metadata into
> the
> > + * previous frame's metadata object, and then moving the
> latter
> > + * into the former.
> > + */
> > + lastMetadata.Merge(rpiMetadata_);
> > + rpiMetadata_ = std::move(lastMetadata);
> > + processPending_ = false;
> > + LOG(IPARPI, Debug) << "Rate-limiting the controller!
> inter-frame duration: "
> > + << frameTimestamp - lastRunTimestamp_
> > + << ", min duration: "
> > + << controllerMinFrameDuration * 1e3;
>
> That would result in loooots of debug messages with a > 60fps frame
> rate. Is it intended ?
>
No, I certainly don't want to be too noisy.
What is the overhead of adding a log point like this, but the logging level
is set to
not display the message? I would like to keep some indication of us
rate-limiting
the IPA run if possible, but not at the cost of excess overhead.
Regards,
Naush
>
> > + return;
> > + }
> > +
> > + lastRunTimestamp_ = frameTimestamp;
> > + processPending_ = true;
> > +
> > ControlList ctrls(ispCtrls_);
> >
> > 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);
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210427/88adb038/attachment-0001.htm>
More information about the libcamera-devel
mailing list