Message ID | 20210707021941.20804-16-laurent.pinchart@ideasonboard.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Hi Laurent, On 07/07/2021 03:19, Laurent Pinchart wrote: > To prepare for multiple concurrent camera sessions, move the event loop > exec() call from the CameraSession class to the CamApp class. There we go - now that global loop access is becoming clearer. > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/cam/camera_session.cpp | 43 +++++++++++++++++--------------------- > src/cam/camera_session.h | 6 ++++-- > src/cam/main.cpp | 12 ++++++++++- > 3 files changed, 34 insertions(+), 27 deletions(-) > > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp > index 16c1c66a285a..b1200e60329c 100644 > --- a/src/cam/camera_session.cpp > +++ b/src/cam/camera_session.cpp > @@ -22,11 +22,11 @@ CameraSession::CameraSession(std::shared_ptr<Camera> camera, > CameraConfiguration *config) > : camera_(camera), config_(config), writer_(nullptr), last_(0), > queueCount_(0), captureCount_(0), captureLimit_(0), > - printMetadata_(false) > + printMetadata_(false), allocator_(nullptr) > { > } > > -int CameraSession::run(const OptionsParser::Options &options) > +int CameraSession::start(const OptionsParser::Options &options) > { > int ret; > > @@ -61,36 +61,39 @@ int CameraSession::run(const OptionsParser::Options &options) > writer_ = new BufferWriter(); > } > > - FrameBufferAllocator *allocator = new FrameBufferAllocator(camera_); > + allocator_ = new FrameBufferAllocator(camera_); > > - ret = capture(allocator); > + return startCapture(); > +} > > - if (options.isSet(OptFile)) { > - delete writer_; > - writer_ = nullptr; > - } > +void CameraSession::stop() > +{ > + int ret = camera_->stop(); > + if (ret) > + std::cout << "Failed to stop capture" << std::endl; > + > + delete writer_; > + writer_ = nullptr; > > requests_.clear(); > > - delete allocator; > - > - return ret; > + delete allocator_; > } > > -int CameraSession::capture(FrameBufferAllocator *allocator) > +int CameraSession::startCapture() > { > int ret; > > /* Identify the stream with the least number of buffers. */ > unsigned int nbuffers = UINT_MAX; > for (StreamConfiguration &cfg : *config_) { > - ret = allocator->allocate(cfg.stream()); > + ret = allocator_->allocate(cfg.stream()); > if (ret < 0) { > std::cerr << "Can't allocate buffers" << std::endl; > return -ENOMEM; > } > > - unsigned int allocated = allocator->buffers(cfg.stream()).size(); > + unsigned int allocated = allocator_->buffers(cfg.stream()).size(); > nbuffers = std::min(nbuffers, allocated); > } > > @@ -109,7 +112,7 @@ int CameraSession::capture(FrameBufferAllocator *allocator) > for (StreamConfiguration &cfg : *config_) { > Stream *stream = cfg.stream(); > const std::vector<std::unique_ptr<FrameBuffer>> &buffers = > - allocator->buffers(stream); > + allocator_->buffers(stream); > const std::unique_ptr<FrameBuffer> &buffer = buffers[i]; > > ret = request->addBuffer(stream, buffer.get()); > @@ -146,15 +149,7 @@ int CameraSession::capture(FrameBufferAllocator *allocator) > else > std::cout << "Capture until user interrupts by SIGINT" << std::endl; > > - ret = EventLoop::instance()->exec(); Without the context of the rest of the file at this point, is this the last EventLoop access? I guess we're still waiting on the removal of > + EventLoop::instance()->callLater([=]() { processRequest(request); }); from 13/30 before we can remove event_loop.h from the headers. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > - if (ret) > - std::cout << "Failed to run capture loop" << std::endl; > - > - ret = camera_->stop(); > - if (ret) > - std::cout << "Failed to stop capture" << std::endl; > - > - return ret; > + return 0; > } > > int CameraSession::queueRequest(Request *request) > diff --git a/src/cam/camera_session.h b/src/cam/camera_session.h > index 2728d7607db2..5131cfd48b4e 100644 > --- a/src/cam/camera_session.h > +++ b/src/cam/camera_session.h > @@ -28,12 +28,13 @@ public: > CameraSession(std::shared_ptr<libcamera::Camera> camera, > libcamera::CameraConfiguration *config); > > - int run(const OptionsParser::Options &options); > + int start(const OptionsParser::Options &options); > + void stop(); > > libcamera::Signal<> captureDone; > > private: > - int capture(libcamera::FrameBufferAllocator *allocator); > + int startCapture(); > > int queueRequest(libcamera::Request *request); > void requestComplete(libcamera::Request *request); > @@ -51,6 +52,7 @@ private: > unsigned int captureLimit_; > bool printMetadata_; > > + libcamera::FrameBufferAllocator *allocator_; > std::vector<std::unique_ptr<libcamera::Request>> requests_; > }; > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > index a567a7cc7653..b13230c374ad 100644 > --- a/src/cam/main.cpp > +++ b/src/cam/main.cpp > @@ -371,7 +371,17 @@ int CamApp::run() > if (options_.isSet(OptCapture)) { > CameraSession session(camera_, config_.get()); > session.captureDone.connect(this, &CamApp::captureDone); > - return session.run(options_); > + > + ret = session.start(options_); > + if (ret) { > + std::cout << "Failed to start camera session" << std::endl; > + return ret; > + } > + > + loop_.exec(); > + > + session.stop(); > + return 0; > } > > if (options_.isSet(OptMonitor)) { >
Hi Kieran, On Mon, Jul 12, 2021 at 04:25:07PM +0100, Kieran Bingham wrote: > On 07/07/2021 03:19, Laurent Pinchart wrote: > > To prepare for multiple concurrent camera sessions, move the event loop > > exec() call from the CameraSession class to the CamApp class. > > There we go - now that global loop access is becoming clearer. > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/cam/camera_session.cpp | 43 +++++++++++++++++--------------------- > > src/cam/camera_session.h | 6 ++++-- > > src/cam/main.cpp | 12 ++++++++++- > > 3 files changed, 34 insertions(+), 27 deletions(-) > > > > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp > > index 16c1c66a285a..b1200e60329c 100644 > > --- a/src/cam/camera_session.cpp > > +++ b/src/cam/camera_session.cpp > > @@ -22,11 +22,11 @@ CameraSession::CameraSession(std::shared_ptr<Camera> camera, > > CameraConfiguration *config) > > : camera_(camera), config_(config), writer_(nullptr), last_(0), > > queueCount_(0), captureCount_(0), captureLimit_(0), > > - printMetadata_(false) > > + printMetadata_(false), allocator_(nullptr) > > { > > } > > > > -int CameraSession::run(const OptionsParser::Options &options) > > +int CameraSession::start(const OptionsParser::Options &options) > > { > > int ret; > > > > @@ -61,36 +61,39 @@ int CameraSession::run(const OptionsParser::Options &options) > > writer_ = new BufferWriter(); > > } > > > > - FrameBufferAllocator *allocator = new FrameBufferAllocator(camera_); > > + allocator_ = new FrameBufferAllocator(camera_); > > > > - ret = capture(allocator); > > + return startCapture(); > > +} > > > > - if (options.isSet(OptFile)) { > > - delete writer_; > > - writer_ = nullptr; > > - } > > +void CameraSession::stop() > > +{ > > + int ret = camera_->stop(); > > + if (ret) > > + std::cout << "Failed to stop capture" << std::endl; > > + > > + delete writer_; > > + writer_ = nullptr; > > > > requests_.clear(); > > > > - delete allocator; > > - > > - return ret; > > + delete allocator_; > > } > > > > -int CameraSession::capture(FrameBufferAllocator *allocator) > > +int CameraSession::startCapture() > > { > > int ret; > > > > /* Identify the stream with the least number of buffers. */ > > unsigned int nbuffers = UINT_MAX; > > for (StreamConfiguration &cfg : *config_) { > > - ret = allocator->allocate(cfg.stream()); > > + ret = allocator_->allocate(cfg.stream()); > > if (ret < 0) { > > std::cerr << "Can't allocate buffers" << std::endl; > > return -ENOMEM; > > } > > > > - unsigned int allocated = allocator->buffers(cfg.stream()).size(); > > + unsigned int allocated = allocator_->buffers(cfg.stream()).size(); > > nbuffers = std::min(nbuffers, allocated); > > } > > > > @@ -109,7 +112,7 @@ int CameraSession::capture(FrameBufferAllocator *allocator) > > for (StreamConfiguration &cfg : *config_) { > > Stream *stream = cfg.stream(); > > const std::vector<std::unique_ptr<FrameBuffer>> &buffers = > > - allocator->buffers(stream); > > + allocator_->buffers(stream); > > const std::unique_ptr<FrameBuffer> &buffer = buffers[i]; > > > > ret = request->addBuffer(stream, buffer.get()); > > @@ -146,15 +149,7 @@ int CameraSession::capture(FrameBufferAllocator *allocator) > > else > > std::cout << "Capture until user interrupts by SIGINT" << std::endl; > > > > - ret = EventLoop::instance()->exec(); > > Without the context of the rest of the file at this point, is this the > last EventLoop access? > > I guess we're still waiting on the removal of > > > + EventLoop::instance()->callLater([=]() { processRequest(request); }); > > from 13/30 before we can remove event_loop.h from the headers. This one stays, at least for now. We could probably change that, but it's a common pattern to access the global event loop through a singleton accessor. As this is specific to the cam application (qcam probably has similar patterns, based on the Qt API that allows the same) in any case, I think it's acceptable. > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > - if (ret) > > - std::cout << "Failed to run capture loop" << std::endl; > > - > > - ret = camera_->stop(); > > - if (ret) > > - std::cout << "Failed to stop capture" << std::endl; > > - > > - return ret; > > + return 0; > > } > > > > int CameraSession::queueRequest(Request *request) > > diff --git a/src/cam/camera_session.h b/src/cam/camera_session.h > > index 2728d7607db2..5131cfd48b4e 100644 > > --- a/src/cam/camera_session.h > > +++ b/src/cam/camera_session.h > > @@ -28,12 +28,13 @@ public: > > CameraSession(std::shared_ptr<libcamera::Camera> camera, > > libcamera::CameraConfiguration *config); > > > > - int run(const OptionsParser::Options &options); > > + int start(const OptionsParser::Options &options); > > + void stop(); > > > > libcamera::Signal<> captureDone; > > > > private: > > - int capture(libcamera::FrameBufferAllocator *allocator); > > + int startCapture(); > > > > int queueRequest(libcamera::Request *request); > > void requestComplete(libcamera::Request *request); > > @@ -51,6 +52,7 @@ private: > > unsigned int captureLimit_; > > bool printMetadata_; > > > > + libcamera::FrameBufferAllocator *allocator_; > > std::vector<std::unique_ptr<libcamera::Request>> requests_; > > }; > > > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > > index a567a7cc7653..b13230c374ad 100644 > > --- a/src/cam/main.cpp > > +++ b/src/cam/main.cpp > > @@ -371,7 +371,17 @@ int CamApp::run() > > if (options_.isSet(OptCapture)) { > > CameraSession session(camera_, config_.get()); > > session.captureDone.connect(this, &CamApp::captureDone); > > - return session.run(options_); > > + > > + ret = session.start(options_); > > + if (ret) { > > + std::cout << "Failed to start camera session" << std::endl; > > + return ret; > > + } > > + > > + loop_.exec(); > > + > > + session.stop(); > > + return 0; > > } > > > > if (options_.isSet(OptMonitor)) {
diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp index 16c1c66a285a..b1200e60329c 100644 --- a/src/cam/camera_session.cpp +++ b/src/cam/camera_session.cpp @@ -22,11 +22,11 @@ CameraSession::CameraSession(std::shared_ptr<Camera> camera, CameraConfiguration *config) : camera_(camera), config_(config), writer_(nullptr), last_(0), queueCount_(0), captureCount_(0), captureLimit_(0), - printMetadata_(false) + printMetadata_(false), allocator_(nullptr) { } -int CameraSession::run(const OptionsParser::Options &options) +int CameraSession::start(const OptionsParser::Options &options) { int ret; @@ -61,36 +61,39 @@ int CameraSession::run(const OptionsParser::Options &options) writer_ = new BufferWriter(); } - FrameBufferAllocator *allocator = new FrameBufferAllocator(camera_); + allocator_ = new FrameBufferAllocator(camera_); - ret = capture(allocator); + return startCapture(); +} - if (options.isSet(OptFile)) { - delete writer_; - writer_ = nullptr; - } +void CameraSession::stop() +{ + int ret = camera_->stop(); + if (ret) + std::cout << "Failed to stop capture" << std::endl; + + delete writer_; + writer_ = nullptr; requests_.clear(); - delete allocator; - - return ret; + delete allocator_; } -int CameraSession::capture(FrameBufferAllocator *allocator) +int CameraSession::startCapture() { int ret; /* Identify the stream with the least number of buffers. */ unsigned int nbuffers = UINT_MAX; for (StreamConfiguration &cfg : *config_) { - ret = allocator->allocate(cfg.stream()); + ret = allocator_->allocate(cfg.stream()); if (ret < 0) { std::cerr << "Can't allocate buffers" << std::endl; return -ENOMEM; } - unsigned int allocated = allocator->buffers(cfg.stream()).size(); + unsigned int allocated = allocator_->buffers(cfg.stream()).size(); nbuffers = std::min(nbuffers, allocated); } @@ -109,7 +112,7 @@ int CameraSession::capture(FrameBufferAllocator *allocator) for (StreamConfiguration &cfg : *config_) { Stream *stream = cfg.stream(); const std::vector<std::unique_ptr<FrameBuffer>> &buffers = - allocator->buffers(stream); + allocator_->buffers(stream); const std::unique_ptr<FrameBuffer> &buffer = buffers[i]; ret = request->addBuffer(stream, buffer.get()); @@ -146,15 +149,7 @@ int CameraSession::capture(FrameBufferAllocator *allocator) else std::cout << "Capture until user interrupts by SIGINT" << std::endl; - ret = EventLoop::instance()->exec(); - if (ret) - std::cout << "Failed to run capture loop" << std::endl; - - ret = camera_->stop(); - if (ret) - std::cout << "Failed to stop capture" << std::endl; - - return ret; + return 0; } int CameraSession::queueRequest(Request *request) diff --git a/src/cam/camera_session.h b/src/cam/camera_session.h index 2728d7607db2..5131cfd48b4e 100644 --- a/src/cam/camera_session.h +++ b/src/cam/camera_session.h @@ -28,12 +28,13 @@ public: CameraSession(std::shared_ptr<libcamera::Camera> camera, libcamera::CameraConfiguration *config); - int run(const OptionsParser::Options &options); + int start(const OptionsParser::Options &options); + void stop(); libcamera::Signal<> captureDone; private: - int capture(libcamera::FrameBufferAllocator *allocator); + int startCapture(); int queueRequest(libcamera::Request *request); void requestComplete(libcamera::Request *request); @@ -51,6 +52,7 @@ private: unsigned int captureLimit_; bool printMetadata_; + libcamera::FrameBufferAllocator *allocator_; std::vector<std::unique_ptr<libcamera::Request>> requests_; }; diff --git a/src/cam/main.cpp b/src/cam/main.cpp index a567a7cc7653..b13230c374ad 100644 --- a/src/cam/main.cpp +++ b/src/cam/main.cpp @@ -371,7 +371,17 @@ int CamApp::run() if (options_.isSet(OptCapture)) { CameraSession session(camera_, config_.get()); session.captureDone.connect(this, &CamApp::captureDone); - return session.run(options_); + + ret = session.start(options_); + if (ret) { + std::cout << "Failed to start camera session" << std::endl; + return ret; + } + + loop_.exec(); + + session.stop(); + return 0; } if (options_.isSet(OptMonitor)) {
To prepare for multiple concurrent camera sessions, move the event loop exec() call from the CameraSession class to the CamApp class. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/cam/camera_session.cpp | 43 +++++++++++++++++--------------------- src/cam/camera_session.h | 6 ++++-- src/cam/main.cpp | 12 ++++++++++- 3 files changed, 34 insertions(+), 27 deletions(-)