Message ID | 20200214001810.19302-3-kieran.bingham@ideasonboard.com |
---|---|
State | Accepted |
Headers | show |
Series |
|
Related | show |
Hi Kieran, Thank you for the patch. On Fri, Feb 14, 2020 at 12:18:05AM +0000, Kieran Bingham wrote: > The MainWindow connects a handler to the Camera requestCompleted signal > when the camera is opened, but never disconnects it. > > Move the connection to the startCapture() function, and ensure that it > is disconnected again when the stream is stopped. > > This ensures that we can successfully tear down the stream, and restart > with a new camera. The commit also adds a camera_->stop() call in an error path, it should be mentioned in the commit message (or split to a separate patch). > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> I think we could avoid the nasty error_disconnect by splitting out code to openCamera/closeCamera methods. This patch however does the job for now, so Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/qcam/main_window.cpp | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > index db14245d7f51..38d7063363f0 100644 > --- a/src/qcam/main_window.cpp > +++ b/src/qcam/main_window.cpp > @@ -113,8 +113,6 @@ int MainWindow::openCamera(CameraManager *cm) > > std::cout << "Using camera " << camera_->name() << std::endl; > > - camera_->requestCompleted.connect(this, &MainWindow::requestComplete); > - > return 0; > } > > @@ -212,17 +210,23 @@ int MainWindow::startCapture() > goto error; > } > > + camera_->requestCompleted.connect(this, &MainWindow::requestComplete); > + > for (Request *request : requests) { > ret = camera_->queueRequest(request); > if (ret < 0) { > std::cerr << "Can't queue request" << std::endl; > - goto error; > + goto error_disconnect; > } > } > > isCapturing_ = true; > return 0; > > +error_disconnect: > + camera_->requestCompleted.disconnect(this, &MainWindow::requestComplete); > + camera_->stop(); > + > error: > for (Request *request : requests) > delete request; > @@ -249,6 +253,8 @@ void MainWindow::stopCapture() > if (ret) > std::cout << "Failed to stop capture" << std::endl; > > + camera_->requestCompleted.disconnect(this, &MainWindow::requestComplete); > + > for (auto &iter : mappedBuffers_) { > void *memory = iter.second.first; > unsigned int length = iter.second.second;
Hi Laurent, On 14/02/2020 11:19, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Fri, Feb 14, 2020 at 12:18:05AM +0000, Kieran Bingham wrote: >> The MainWindow connects a handler to the Camera requestCompleted signal >> when the camera is opened, but never disconnects it. >> >> Move the connection to the startCapture() function, and ensure that it >> is disconnected again when the stream is stopped. >> >> This ensures that we can successfully tear down the stream, and restart >> with a new camera. > > The commit also adds a camera_->stop() call in an error path, it should > be mentioned in the commit message (or split to a separate patch). Yes, It looked important to add to the error path, as I was adding it anyway, but I forgot to update the commit. We can add: Introducing the error_disconnect cleanup path in start_capture identified that we left the camera in the start state, thus we also add a call to camera->stop() when disconnecting the signal connection. >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > I think we could avoid the nasty error_disconnect by splitting out code > to openCamera/closeCamera methods. This patch however does the job for > now, so That was already one of the many rabbit holes I went down since your previous review of this series. As we wanted to progress this series I went for the simple paths. We can build on top. -- Kieran > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks. > >> --- >> src/qcam/main_window.cpp | 12 +++++++++--- >> 1 file changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp >> index db14245d7f51..38d7063363f0 100644 >> --- a/src/qcam/main_window.cpp >> +++ b/src/qcam/main_window.cpp >> @@ -113,8 +113,6 @@ int MainWindow::openCamera(CameraManager *cm) >> >> std::cout << "Using camera " << camera_->name() << std::endl; >> >> - camera_->requestCompleted.connect(this, &MainWindow::requestComplete); >> - >> return 0; >> } >> >> @@ -212,17 +210,23 @@ int MainWindow::startCapture() >> goto error; >> } >> >> + camera_->requestCompleted.connect(this, &MainWindow::requestComplete); >> + >> for (Request *request : requests) { >> ret = camera_->queueRequest(request); >> if (ret < 0) { >> std::cerr << "Can't queue request" << std::endl; >> - goto error; >> + goto error_disconnect; >> } >> } >> >> isCapturing_ = true; >> return 0; >> >> +error_disconnect: >> + camera_->requestCompleted.disconnect(this, &MainWindow::requestComplete); >> + camera_->stop(); >> + >> error: >> for (Request *request : requests) >> delete request; >> @@ -249,6 +253,8 @@ void MainWindow::stopCapture() >> if (ret) >> std::cout << "Failed to stop capture" << std::endl; >> >> + camera_->requestCompleted.disconnect(this, &MainWindow::requestComplete); >> + >> for (auto &iter : mappedBuffers_) { >> void *memory = iter.second.first; >> unsigned int length = iter.second.second; >
diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp index db14245d7f51..38d7063363f0 100644 --- a/src/qcam/main_window.cpp +++ b/src/qcam/main_window.cpp @@ -113,8 +113,6 @@ int MainWindow::openCamera(CameraManager *cm) std::cout << "Using camera " << camera_->name() << std::endl; - camera_->requestCompleted.connect(this, &MainWindow::requestComplete); - return 0; } @@ -212,17 +210,23 @@ int MainWindow::startCapture() goto error; } + camera_->requestCompleted.connect(this, &MainWindow::requestComplete); + for (Request *request : requests) { ret = camera_->queueRequest(request); if (ret < 0) { std::cerr << "Can't queue request" << std::endl; - goto error; + goto error_disconnect; } } isCapturing_ = true; return 0; +error_disconnect: + camera_->requestCompleted.disconnect(this, &MainWindow::requestComplete); + camera_->stop(); + error: for (Request *request : requests) delete request; @@ -249,6 +253,8 @@ void MainWindow::stopCapture() if (ret) std::cout << "Failed to stop capture" << std::endl; + camera_->requestCompleted.disconnect(this, &MainWindow::requestComplete); + for (auto &iter : mappedBuffers_) { void *memory = iter.second.first; unsigned int length = iter.second.second;
The MainWindow connects a handler to the Camera requestCompleted signal when the camera is opened, but never disconnects it. Move the connection to the startCapture() function, and ensure that it is disconnected again when the stream is stopped. This ensures that we can successfully tear down the stream, and restart with a new camera. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/qcam/main_window.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)