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

Message ID 20230118085953.7027-12-naush@raspberrypi.com
State Superseded
Headers show
Series
  • Raspberry Pi: Platform configuration and buffer allocation improvements
Related show

Commit Message

Naushir Patuck Jan. 18, 2023, 8:59 a.m. UTC
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.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
---
 .../pipeline/raspberrypi/data/example.yaml    |  7 +++++-
 .../pipeline/raspberrypi/raspberrypi.cpp      | 23 +++++++++++++++++++
 2 files changed, 29 insertions(+), 1 deletion(-)

Comments

Kieran Bingham Jan. 20, 2023, 11:22 a.m. UTC | #1
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)

> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@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 ?

> +
> +                # 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' ?

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

There's still the potential for the pipeline to catch up
isn't there?

> +                       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
>
Naushir Patuck Jan. 20, 2023, 3:19 p.m. UTC | #2
Hi Kieran,

Thank you for the feedback!

On Fri, 20 Jan 2023 at 11:22, Kieran Bingham <
kieran.bingham@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@raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman@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
> >
>
Laurent Pinchart Jan. 22, 2023, 11:06 p.m. UTC | #3
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@raspberrypi.com>
> > > Reviewed-by: David Plowman <david.plowman@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
Naushir Patuck Jan. 25, 2023, 2:21 p.m. UTC | #4
Hi Laurent,

Thank you for your feedback.

On Sun, 22 Jan 2023 at 23:06, Laurent Pinchart
<laurent.pinchart@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@raspberrypi.com>
> > > > Reviewed-by: David Plowman <david.plowman@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

Patch
diff mbox series

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,
+
+                # 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
+		 * 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) {
+			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