[libcamera-devel] [PATCH v5 11/12] pipeline: raspberrypi: Allow pipeline handler to always use the newest frame

[Laurent Pinchart]

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 ?

> > > +                       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