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

[Naushir Patuck]

Hi Kieran,

Thank you for the feedback!

On Fri, 20 Jan 2023 at 11:22, Kieran Bingham <
kieran.bingham at ideasonboard.com> 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...


>
> > +
> > +                # Always process a pending request with the last
> captured sensor
> > +                # frame.  Note that this might lead to avoidable frame
> drops
> > +                # 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.

Regards,
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
> > --
> > 2.25.1
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20230120/a81f10d3/attachment.htm>