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