<div dir="ltr"><div>Hi Laurent,</div><div><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 5 Dec 2022 at 13:40, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@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">Hi Naush,<br>
<br>
On Mon, Dec 05, 2022 at 10:58:40AM +0000, Naushir Patuck wrote:<br>
> On Fri, 2 Dec 2022 at 15:45, Laurent Pinchart wrote:<br>
> > On Tue, Nov 29, 2022 at 01:45:32PM +0000, Naushir Patuck via libcamera-devel wrote:<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>
> > You're explaining the downside, but not why an application may want to<br>
> > do this. What are the expected use cases ?<br>
> <br>
> This is one of those user requests that I don't necessarily agree with, but easy<br>
> enough to work around :-)<br>
> <br>
> Some users have expressed problems with their custom application where they<br>
> try to emulate timelapse captures by triggering Requests to the period of<br>
> the timelapse value. The complaint is that the Request gets completed with one<br>
> of the Unicam buffers completed well before the Request was issued, which I suppose<br>
> is reasonable.<br>
> <br>
> Of course, there are other work-arounds (recycle requests as soon as possible<br>
> and only use the output at the timelapse interval), but the grumbling still continues,<br>
> so this gives an easy way to "fix" this.<br>
<br>
Fixing this in the pipeline handler seems reasonable. I don't think we<br>
have a written rule about this anywhere, but would it make sense to<br>
require pipeline handlers to never return frames with an earlier<br>
timestamp than the request queue time ?<br></blockquote><div><br></div><div>Adding the logic for this is easy enough, but I am not sure it should be enforced.</div><div>It might cause unnecessary frame drops because unexpected jitters in the</div><div>application/userland might delay the Request call ever so slightly. The unicam</div><div>(output) buffer queue smoothes such jitter away. Maybe this is another hint or</div><div>config parameter ;-)</div><div><br></div><div>Regards,</div><div>Naush</div><div><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>
> > > 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/default.json | 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/default.json b/src/libcamera/pipeline/raspberrypi/data/default.json<br>
> > > index 707414bcc5c5..eda053258c67 100644<br>
> > > --- a/src/libcamera/pipeline/raspberrypi/data/default.json<br>
> > > +++ b/src/libcamera/pipeline/raspberrypi/data/default.json<br>
> > > @@ -18,6 +18,11 @@<br>
> > > "num_output0_buffers": 1,<br>
> > ><br>
> > > # Override any request from the IPA to drop a number of startup frames.<br>
> > > - "disable_startup_frame_drops": false<br>
> > > + "disable_startup_frame_drops": false,<br>
> > > +<br>
> > > + # Always process a pending request with the last captured sensor frame.<br>
> > > + # Note that this might lead to avoidable frame drops during periods<br>
> > > + # 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 ef49d32037af..a369fbd2a528 100644<br>
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > @@ -317,6 +317,11 @@ public:<br>
> > > * frames.<br>
> > > */<br>
> > > bool disableStartupFrameDrops;<br>
> > > + /*<br>
> > > + * Always process a pending request with the last captured sensor<br>
> > > + * frame.<br>
> > > + */<br>
> > > + bool returnNewestFrames;<br>
> > > };<br>
> > ><br>
> > > Config config_;<br>
> > > @@ -1457,6 +1462,7 @@ int PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data)<br>
> > > .minTotalUnicamBuffers = 4,<br>
> > > .numOutput0Buffers = 1,<br>
> > > .disableStartupFrameDrops = false,<br>
> > > + .returnNewestFrames = false,<br>
> > > };<br>
> > ><br>
> > > char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_CONFIG_FILE");<br>
> > > @@ -1493,6 +1499,8 @@ int PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data)<br>
> > > phConfig["num_output0_buffers"].get<unsigned int>(config.numOutput0Buffers);<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>
> > > @@ -2349,6 +2357,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>
> > > + 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>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>