[libcamera-devel] [PATCH v5 11/12] pipeline: raspberrypi: Allow pipeline handler to always use the newest frame
Naushir Patuck
naush at raspberrypi.com
Wed Jan 25 15:21:03 CET 2023
Hi Laurent,
Thank you for your feedback.
On Sun, 22 Jan 2023 at 23:06, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hello,
>
> On Fri, Jan 20, 2023 at 03:19:31PM +0000, Naushir Patuck via libcamera-devel wrote:
> > On Fri, 20 Jan 2023 at 11:22, Kieran Bingham wrote:
> > > Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:52)
> > > > Add a pipeline config parameter "return_newest_frames" to always use the
> > > > most recently captured Unicam frame when processing a request. This effectively
> > > > stops the pipeline handler from queuing Unicam buffers and processing requests
> > > > using the buffer at the front of the queue.
> > > >
> > > > Note that setting this parameter might incur unnecessary frame drops during
> > > > times of high transient CPU loads where the application might not be able to
> > > > provide requests quick enough.
> > >
> > > I'd be tempted to add here, 'But it can lower perceived latency of the
> > > output when recovering from a frame drop scenario' ? (if that's correct)
> >
> > Yes, I think that's valid - I'll add it in.
> >
> > > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > > > ---
> > > > .../pipeline/raspberrypi/data/example.yaml | 7 +++++-
> > > > .../pipeline/raspberrypi/raspberrypi.cpp | 23 +++++++++++++++++++
> > > > 2 files changed, 29 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/libcamera/pipeline/raspberrypi/data/example.yaml b/src/libcamera/pipeline/raspberrypi/data/example.yaml
> > > > index 421f30e62aa3..04a117f38ada 100644
> > > > --- a/src/libcamera/pipeline/raspberrypi/data/example.yaml
> > > > +++ b/src/libcamera/pipeline/raspberrypi/data/example.yaml
> > > > @@ -19,6 +19,11 @@
> > > >
> > > > # Override any request from the IPA to drop a number of startup
> > > > # frames.
> > > > - "disable_startup_frame_drops": false
> > > > + "disable_startup_frame_drops": false,
> > >
> > > Can the options always have a comma so they don't require modifiying
> > > the previous one to add a new one?
> > >
> > > Is that because you're treating this file as json instead of yaml ?
> >
> > This was certainly needed when the files were named *.json in
> > earlier revisions.
> > Not sure now that they are *.yaml with json format...
>
> Not that you have to do so, but there's also the option of formatting
> the files with the "normal" YAML syntax.
>
> > > > +
> > > > + # Always process a pending request with the last captured sensor
> > > > + # frame. Note that this might lead to avoidable frame drops
>
> s/ / /
>
> > > > + # during periods of transient heavy CPU loading.
> > > > + "return_newest_frames": false
> > > > }
> > > > }
> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > index 3529d331deb6..21edc8e05469 100644
> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > @@ -314,6 +314,11 @@ public:
> > > > * frames.
> > > > */
> > > > bool disableStartupFrameDrops;
> > > > + /*
> > > > + * Always process a pending request with the last captured sensor
> > >
> > > last / 'most recently' ?
> >
> > Ack.
> >
> > > > + * frame.
> > > > + */
> > > > + bool returnNewestFrames;
> > > > };
> > > >
> > > > Config config_;
> > > > @@ -1712,6 +1717,7 @@ int RPiCameraData::configurePipeline()
> > > > .minUnicamBuffers = 2,
> > > > .minTotalUnicamBuffers = 4,
> > > > .disableStartupFrameDrops = false,
> > > > + .returnNewestFrames = false,
> > > > };
> > > >
> > > > char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_CONFIG_FILE");
> > > > @@ -1747,6 +1753,8 @@ int RPiCameraData::configurePipeline()
> > > > phConfig["min_total_unicam_buffers"].get<unsigned int>(config_.minTotalUnicamBuffers);
> > > > config_.disableStartupFrameDrops =
> > > > phConfig["disable_startup_frame_drops"].get<bool>(config_.disableStartupFrameDrops);
> > > > + config_.returnNewestFrames =
> > > > + phConfig["return_newest_frames"].get<bool>(config_.returnNewestFrames);
> > > >
> > > > if (config_.minTotalUnicamBuffers < config_.minUnicamBuffers) {
> > > > LOG(RPI, Error) << "Invalid configuration: min_total_unicam_buffers must be >= min_unicam_buffers";
> > > > @@ -2329,6 +2337,21 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&em
> > > > if (bayerQueue_.empty())
> > > > return false;
> > > >
> > > > + /*
> > > > + * If the pipeline is configured to only ever return the most recently
> > > > + * captured frame, empty the buffer queue until a single element is
> > > > + * left, corresponding to the most recent buffer. Note that this will
> > > > + * likely result in possibly avoidable dropped frames.
> > > > + */
> > > > + if (config_.returnNewestFrames && !unicam_[Unicam::Image].isExternal()) {
> > > > + while (bayerQueue_.size() > 1) {
> > >
> > > Is this required?
> > >
> > > I'm a bit puzzled at the need to drop all the other frames. Don't we
> > > just need to choose the oldest buffer to drop so that we can capture a
> > > new image?
> >
> > I think it is needed. We are flushing our internal Unicam buffer queue up-to
> > the latest captured frame to process for the request. Depending on the number
> > of input buffers allocated, this may be > 1.
> >
> > > There's still the potential for the pipeline to catch up
> > > isn't there?
> >
> > Yes it might be possible to catch up, but there is no way of knowing this
> > deterministically. Hence the comment in the config file saying that using this
> > flag might lead to possibly avoidable frame drops.
>
> Might may be a bit of an understatement :-) I wonder if it would be
> possible to flush the queue only when we detect a frame drop.
>
> What's the expected use case for this configuration parameter ?
This is meant to reduce capture-to-display latency by always flushing the
internal buffer queue. This is to accommodate those few users who insist on
doing timelapse by rate limiting the Request queueing. However now that I think
about it, this is easily achievable by simply having a RAW stream configured,
and setting min_unicam_buffers to 0. Or alternatively, set min_unicam_buffers
to 1, and not have a RAW stream - this is not the same, but does reduce latency.
I am tempted to just remove this patch now.
Naush
>
> > > > + FrameBuffer *bayer = bayerQueue_.front().buffer;
> > > > +
> > > > + unicam_[Unicam::Image].returnBuffer(bayer);
> > > > + bayerQueue_.pop();
> > > > + }
> > > > + }
> > > > +
> > > > /*
> > > > * Find the embedded data buffer with a matching timestamp to pass to
> > > > * the IPA. Any embedded buffers with a timestamp lower than the
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list