<div dir="ltr"><div dir="ltr">Hi Kieran,<div><br></div><div>Thank you for the feedback!</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 20 Jan 2023 at 11:22, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:52)<br>
> Add a pipeline config parameter "return_newest_frames" to always use the<br>
> most recently captured Unicam frame when processing a request. This effectively<br>
> stops the pipeline handler from queuing Unicam buffers and processing requests<br>
> using the buffer at the front of the queue.<br>
> <br>
> Note that setting this parameter might incur unnecessary frame drops during<br>
> times of high transient CPU loads where the application might not be able to<br>
> provide requests quick enough.<br>
<br>
I'd be tempted to add here, 'But it can lower perceived latency of the<br>
output when recovering from a frame drop scenario' ? (if that's correct)<br></blockquote><div><br></div><div>Yes, I think that's valid - I'll add it in.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> <br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> ---<br>
>  .../pipeline/raspberrypi/data/example.yaml    |  7 +++++-<br>
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 23 +++++++++++++++++++<br>
>  2 files changed, 29 insertions(+), 1 deletion(-)<br>
> <br>
> diff --git a/src/libcamera/pipeline/raspberrypi/data/example.yaml b/src/libcamera/pipeline/raspberrypi/data/example.yaml<br>
> index 421f30e62aa3..04a117f38ada 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/data/example.yaml<br>
> +++ b/src/libcamera/pipeline/raspberrypi/data/example.yaml<br>
> @@ -19,6 +19,11 @@<br>
>  <br>
>                  # Override any request from the IPA to drop a number of startup<br>
>                  # frames.<br>
> -                "disable_startup_frame_drops": false<br>
> +                "disable_startup_frame_drops": false,<br>
<br>
Can the options always have a comma so they don't require modifiying<br>
the previous one to add a new one?<br>
<br>
Is that because you're treating this file as json instead of yaml ?<br></blockquote><div><br></div><div>This was certainly needed when the files were named *.json in earlier revisions.</div><div>Not sure now that they are *.yaml with json format...</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +<br>
> +                # Always process a pending request with the last captured sensor<br>
> +                # frame.  Note that this might lead to avoidable frame drops<br>
> +                # during periods of transient heavy CPU loading.<br>
> +                "return_newest_frames": false<br>
>          }<br>
>  }<br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 3529d331deb6..21edc8e05469 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -314,6 +314,11 @@ public:<br>
>                  * frames.<br>
>                  */<br>
>                 bool disableStartupFrameDrops;<br>
> +               /*<br>
> +                * Always process a pending request with the last captured sensor<br>
<br>
last / 'most recently' ?<br></blockquote><div><br></div><div>Ack.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +                * frame.<br>
> +                */<br>
> +               bool returnNewestFrames;<br>
>         };<br>
>  <br>
>         Config config_;<br>
> @@ -1712,6 +1717,7 @@ int RPiCameraData::configurePipeline()<br>
>                 .minUnicamBuffers = 2,<br>
>                 .minTotalUnicamBuffers = 4,<br>
>                 .disableStartupFrameDrops = false,<br>
> +               .returnNewestFrames = false,<br>
>         };<br>
>  <br>
>         char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_CONFIG_FILE");<br>
> @@ -1747,6 +1753,8 @@ int RPiCameraData::configurePipeline()<br>
>                 phConfig["min_total_unicam_buffers"].get<unsigned int>(config_.minTotalUnicamBuffers);<br>
>         config_.disableStartupFrameDrops =<br>
>                 phConfig["disable_startup_frame_drops"].get<bool>(config_.disableStartupFrameDrops);<br>
> +       config_.returnNewestFrames =<br>
> +               phConfig["return_newest_frames"].get<bool>(config_.returnNewestFrames);<br>
>  <br>
>         if (config_.minTotalUnicamBuffers < config_.minUnicamBuffers) {<br>
>                 LOG(RPI, Error) << "Invalid configuration: min_total_unicam_buffers must be >= min_unicam_buffers";<br>
> @@ -2329,6 +2337,21 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&em<br>
>         if (bayerQueue_.empty())<br>
>                 return false;<br>
>  <br>
> +       /*<br>
> +        * If the pipeline is configured to only ever return the most recently<br>
> +        * captured frame, empty the buffer queue until a single element is<br>
> +        * left, corresponding to the most recent buffer. Note that this will<br>
> +        * likely result in possibly avoidable dropped frames.<br>
> +        */<br>
> +       if (config_.returnNewestFrames && !unicam_[Unicam::Image].isExternal()) {<br>
> +               while (bayerQueue_.size() > 1) {<br>
<br>
Is this required?<br>
<br>
I'm a bit puzzled at the need to drop all the other frames. Don't we<br>
just need to choose the oldest buffer to drop so that we can capture a<br>
new image?<br></blockquote><div><br></div><div>I think it is needed.  We are flushing our internal Unicam buffer queue up-to<br>the latest captured frame to process for the request.  Depending on the number<br>of input buffers allocated, this may be > 1.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
There's still the potential for the pipeline to catch up<br>
isn't there?<br></blockquote><div><br></div><div>Yes it might be possible to catch up, but there is no way of knowing this<br>deterministically.  Hence the comment in the config file saying that using this<br>flag might lead to possibly avoidable frame drops.<br></div><div><br></div><div>Regards,</div><div>Naush</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +                       FrameBuffer *bayer = bayerQueue_.front().buffer;<br>
> +<br>
> +                       unicam_[Unicam::Image].returnBuffer(bayer);<br>
> +                       bayerQueue_.pop();<br>
> +               }<br>
> +       }<br>
> +<br>
>         /*<br>
>          * Find the embedded data buffer with a matching timestamp to pass to<br>
>          * the IPA. Any embedded buffers with a timestamp lower than the<br>
> -- <br>
> 2.25.1<br>
><br>
</blockquote></div></div>