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

[Kieran Bingham]

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

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