[libcamera-devel] [PATCH v2 2/7] qcam: Move requestCompleted signal mapping

[Kieran Bingham]

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 at 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 at 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;
> 

-- 
Regards
--
Kieran