| Message ID | 20201113063815.10288-6-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
On 13/11/2020 06:38, Laurent Pinchart wrote: > The request completion handler is invoked in the camera manager thread, > which shouldn't be blocked for large amounts of time. As writing the > frames to disk can be a time-consuming process, move request processing > to the main thread by queueing an event to the event loop. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/cam/capture.cpp | 9 +++++++++ > src/cam/capture.h | 1 + > 2 files changed, 10 insertions(+) > > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp > index 7580f798288c..43b109d099f6 100644 > --- a/src/cam/capture.cpp > +++ b/src/cam/capture.cpp > @@ -157,6 +157,15 @@ void Capture::requestComplete(Request *request) > if (request->status() == Request::RequestCancelled) > return; > > + /* > + * Defer processing of the completed request to the event loop, to avoid > + * blocking the camera manager thread. > + */ > + loop_->callLater(std::bind(&Capture::processRequest, this, request)); > +} > + > +void Capture::processRequest(Request *request) > +{ > const Request::BufferMap &buffers = request->buffers(); > > /* > diff --git a/src/cam/capture.h b/src/cam/capture.h > index 45e5e8a9ba27..d21c95a26ce7 100644 > --- a/src/cam/capture.h > +++ b/src/cam/capture.h > @@ -33,6 +33,7 @@ private: > int capture(libcamera::FrameBufferAllocator *allocator); > > void requestComplete(libcamera::Request *request); > + void processRequest(libcamera::Request *request); > > std::shared_ptr<libcamera::Camera> camera_; > libcamera::CameraConfiguration *config_; >
Hi Laurent, Thanks for your work. On 2020-11-13 08:38:15 +0200, Laurent Pinchart wrote: > The request completion handler is invoked in the camera manager thread, > which shouldn't be blocked for large amounts of time. As writing the > frames to disk can be a time-consuming process, move request processing > to the main thread by queueing an event to the event loop. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/cam/capture.cpp | 9 +++++++++ > src/cam/capture.h | 1 + > 2 files changed, 10 insertions(+) > > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp > index 7580f798288c..43b109d099f6 100644 > --- a/src/cam/capture.cpp > +++ b/src/cam/capture.cpp > @@ -157,6 +157,15 @@ void Capture::requestComplete(Request *request) > if (request->status() == Request::RequestCancelled) > return; > > + /* > + * Defer processing of the completed request to the event loop, to avoid > + * blocking the camera manager thread. > + */ > + loop_->callLater(std::bind(&Capture::processRequest, this, request)); > +} > + > +void Capture::processRequest(Request *request) > +{ > const Request::BufferMap &buffers = request->buffers(); > > /* > diff --git a/src/cam/capture.h b/src/cam/capture.h > index 45e5e8a9ba27..d21c95a26ce7 100644 > --- a/src/cam/capture.h > +++ b/src/cam/capture.h > @@ -33,6 +33,7 @@ private: > int capture(libcamera::FrameBufferAllocator *allocator); > > void requestComplete(libcamera::Request *request); > + void processRequest(libcamera::Request *request); > > std::shared_ptr<libcamera::Camera> camera_; > libcamera::CameraConfiguration *config_; > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hey Laurent, Thanks for your work Am Fr., 13. Nov. 2020 um 07:38 Uhr schrieb Laurent Pinchart <laurent.pinchart@ideasonboard.com>: > > The request completion handler is invoked in the camera manager thread, > which shouldn't be blocked for large amounts of time. As writing the > frames to disk can be a time-consuming process, move request processing > to the main thread by queueing an event to the event loop. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/cam/capture.cpp | 9 +++++++++ > src/cam/capture.h | 1 + > 2 files changed, 10 insertions(+) > > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp > index 7580f798288c..43b109d099f6 100644 > --- a/src/cam/capture.cpp > +++ b/src/cam/capture.cpp > @@ -157,6 +157,15 @@ void Capture::requestComplete(Request *request) > if (request->status() == Request::RequestCancelled) > return; > > + /* > + * Defer processing of the completed request to the event loop, to avoid > + * blocking the camera manager thread. > + */ > + loop_->callLater(std::bind(&Capture::processRequest, this, request)); You could use a lambda here instead of std::bind: [=] { processRequest(request); } std::bind would otherwise require including `<functional>` > +} > + > +void Capture::processRequest(Request *request) > +{ > const Request::BufferMap &buffers = request->buffers(); > > /* > diff --git a/src/cam/capture.h b/src/cam/capture.h > index 45e5e8a9ba27..d21c95a26ce7 100644 > --- a/src/cam/capture.h > +++ b/src/cam/capture.h > @@ -33,6 +33,7 @@ private: > int capture(libcamera::FrameBufferAllocator *allocator); > > void requestComplete(libcamera::Request *request); > + void processRequest(libcamera::Request *request); > > std::shared_ptr<libcamera::Camera> camera_; > libcamera::CameraConfiguration *config_;
Hi Marvin, On Fri, Nov 13, 2020 at 01:35:40PM +0100, Marvin Schmidt wrote: > Hey Laurent, > > Thanks for your work > > Am Fr., 13. Nov. 2020 um 07:38 Uhr schrieb Laurent Pinchart: > > > > The request completion handler is invoked in the camera manager thread, > > which shouldn't be blocked for large amounts of time. As writing the > > frames to disk can be a time-consuming process, move request processing > > to the main thread by queueing an event to the event loop. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/cam/capture.cpp | 9 +++++++++ > > src/cam/capture.h | 1 + > > 2 files changed, 10 insertions(+) > > > > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp > > index 7580f798288c..43b109d099f6 100644 > > --- a/src/cam/capture.cpp > > +++ b/src/cam/capture.cpp > > @@ -157,6 +157,15 @@ void Capture::requestComplete(Request *request) > > if (request->status() == Request::RequestCancelled) > > return; > > > > + /* > > + * Defer processing of the completed request to the event loop, to avoid > > + * blocking the camera manager thread. > > + */ > > + loop_->callLater(std::bind(&Capture::processRequest, this, request)); > > You could use a lambda here instead of std::bind: > > [=] { processRequest(request); } > > std::bind would otherwise require including `<functional>` <functional> is included by event_loop.h, as the callLater() function takes an std::function parameter. But a lambda would certainly work. I wonder, between std::bind and the proposed lambda function, what would be more efficient ? > > +} > > + > > +void Capture::processRequest(Request *request) > > +{ > > const Request::BufferMap &buffers = request->buffers(); > > > > /* > > diff --git a/src/cam/capture.h b/src/cam/capture.h > > index 45e5e8a9ba27..d21c95a26ce7 100644 > > --- a/src/cam/capture.h > > +++ b/src/cam/capture.h > > @@ -33,6 +33,7 @@ private: > > int capture(libcamera::FrameBufferAllocator *allocator); > > > > void requestComplete(libcamera::Request *request); > > + void processRequest(libcamera::Request *request); > > > > std::shared_ptr<libcamera::Camera> camera_; > > libcamera::CameraConfiguration *config_;
Am Fr., 13. Nov. 2020 um 13:45 Uhr schrieb Laurent Pinchart <laurent.pinchart@ideasonboard.com>: > > Hi Marvin, > > On Fri, Nov 13, 2020 at 01:35:40PM +0100, Marvin Schmidt wrote: > > Hey Laurent, > > > > Thanks for your work > > > > Am Fr., 13. Nov. 2020 um 07:38 Uhr schrieb Laurent Pinchart: > > > > > > The request completion handler is invoked in the camera manager thread, > > > which shouldn't be blocked for large amounts of time. As writing the > > > frames to disk can be a time-consuming process, move request processing > > > to the main thread by queueing an event to the event loop. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > src/cam/capture.cpp | 9 +++++++++ > > > src/cam/capture.h | 1 + > > > 2 files changed, 10 insertions(+) > > > > > > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp > > > index 7580f798288c..43b109d099f6 100644 > > > --- a/src/cam/capture.cpp > > > +++ b/src/cam/capture.cpp > > > @@ -157,6 +157,15 @@ void Capture::requestComplete(Request *request) > > > if (request->status() == Request::RequestCancelled) > > > return; > > > > > > + /* > > > + * Defer processing of the completed request to the event loop, to avoid > > > + * blocking the camera manager thread. > > > + */ > > > + loop_->callLater(std::bind(&Capture::processRequest, this, request)); > > > > You could use a lambda here instead of std::bind: > > > > [=] { processRequest(request); } > > > > std::bind would otherwise require including `<functional>` > > <functional> is included by event_loop.h, as the callLater() function > takes an std::function parameter. But a lambda would certainly work. I > wonder, between std::bind and the proposed lambda function, what would > be more efficient ? Since lambdas are a language feature I would imagine that they can be better optimized by compilers. Other people seem to agree with that and found that lambdas are faster (at least in their use-case / benchmark): https://stackoverflow.com/questions/24852764/stdbind-vs-lambda-performance Also the existence of clang-tidy's `modernize-avoid-bind` check[1] makes me think that lambdas should be used where possible :-) [1] https://clang.llvm.org/extra/clang-tidy/checks/modernize-avoid-bind.html > > > +} > > > + > > > +void Capture::processRequest(Request *request) > > > +{ > > > const Request::BufferMap &buffers = request->buffers(); > > > > > > /* > > > diff --git a/src/cam/capture.h b/src/cam/capture.h > > > index 45e5e8a9ba27..d21c95a26ce7 100644 > > > --- a/src/cam/capture.h > > > +++ b/src/cam/capture.h > > > @@ -33,6 +33,7 @@ private: > > > int capture(libcamera::FrameBufferAllocator *allocator); > > > > > > void requestComplete(libcamera::Request *request); > > > + void processRequest(libcamera::Request *request); > > > > > > std::shared_ptr<libcamera::Camera> camera_; > > > libcamera::CameraConfiguration *config_; > > -- > Regards, > > Laurent Pinchart
Hi Marvin, On Fri, Nov 13, 2020 at 02:31:09PM +0100, Marvin Schmidt wrote: > Am Fr., 13. Nov. 2020 um 13:45 Uhr schrieb Laurent Pinchart: > > On Fri, Nov 13, 2020 at 01:35:40PM +0100, Marvin Schmidt wrote: > > > Hey Laurent, > > > > > > Thanks for your work > > > > > > Am Fr., 13. Nov. 2020 um 07:38 Uhr schrieb Laurent Pinchart: > > > > > > > > The request completion handler is invoked in the camera manager thread, > > > > which shouldn't be blocked for large amounts of time. As writing the > > > > frames to disk can be a time-consuming process, move request processing > > > > to the main thread by queueing an event to the event loop. > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > --- > > > > src/cam/capture.cpp | 9 +++++++++ > > > > src/cam/capture.h | 1 + > > > > 2 files changed, 10 insertions(+) > > > > > > > > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp > > > > index 7580f798288c..43b109d099f6 100644 > > > > --- a/src/cam/capture.cpp > > > > +++ b/src/cam/capture.cpp > > > > @@ -157,6 +157,15 @@ void Capture::requestComplete(Request *request) > > > > if (request->status() == Request::RequestCancelled) > > > > return; > > > > > > > > + /* > > > > + * Defer processing of the completed request to the event loop, to avoid > > > > + * blocking the camera manager thread. > > > > + */ > > > > + loop_->callLater(std::bind(&Capture::processRequest, this, request)); > > > > > > You could use a lambda here instead of std::bind: > > > > > > [=] { processRequest(request); } > > > > > > std::bind would otherwise require including `<functional>` > > > > <functional> is included by event_loop.h, as the callLater() function > > takes an std::function parameter. But a lambda would certainly work. I > > wonder, between std::bind and the proposed lambda function, what would > > be more efficient ? > > Since lambdas are a language feature I would imagine that they can be > better optimized by compilers. Other people seem to agree with that and > found that lambdas are faster (at least in their use-case / benchmark): > https://stackoverflow.com/questions/24852764/stdbind-vs-lambda-performance > > Also the existence of clang-tidy's `modernize-avoid-bind` check[1] makes me > think that lambdas should be used where possible :-) > > [1] https://clang.llvm.org/extra/clang-tidy/checks/modernize-avoid-bind.html Thanks for the interesting information. I'll switch to using a lambda. > > > > +} > > > > + > > > > +void Capture::processRequest(Request *request) > > > > +{ > > > > const Request::BufferMap &buffers = request->buffers(); > > > > > > > > /* > > > > diff --git a/src/cam/capture.h b/src/cam/capture.h > > > > index 45e5e8a9ba27..d21c95a26ce7 100644 > > > > --- a/src/cam/capture.h > > > > +++ b/src/cam/capture.h > > > > @@ -33,6 +33,7 @@ private: > > > > int capture(libcamera::FrameBufferAllocator *allocator); > > > > > > > > void requestComplete(libcamera::Request *request); > > > > + void processRequest(libcamera::Request *request); > > > > > > > > std::shared_ptr<libcamera::Camera> camera_; > > > > libcamera::CameraConfiguration *config_;
diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp index 7580f798288c..43b109d099f6 100644 --- a/src/cam/capture.cpp +++ b/src/cam/capture.cpp @@ -157,6 +157,15 @@ void Capture::requestComplete(Request *request) if (request->status() == Request::RequestCancelled) return; + /* + * Defer processing of the completed request to the event loop, to avoid + * blocking the camera manager thread. + */ + loop_->callLater(std::bind(&Capture::processRequest, this, request)); +} + +void Capture::processRequest(Request *request) +{ const Request::BufferMap &buffers = request->buffers(); /* diff --git a/src/cam/capture.h b/src/cam/capture.h index 45e5e8a9ba27..d21c95a26ce7 100644 --- a/src/cam/capture.h +++ b/src/cam/capture.h @@ -33,6 +33,7 @@ private: int capture(libcamera::FrameBufferAllocator *allocator); void requestComplete(libcamera::Request *request); + void processRequest(libcamera::Request *request); std::shared_ptr<libcamera::Camera> camera_; libcamera::CameraConfiguration *config_;
The request completion handler is invoked in the camera manager thread, which shouldn't be blocked for large amounts of time. As writing the frames to disk can be a time-consuming process, move request processing to the main thread by queueing an event to the event loop. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/cam/capture.cpp | 9 +++++++++ src/cam/capture.h | 1 + 2 files changed, 10 insertions(+)