[libcamera-devel] [RFC PATCH 1/2] lc-compliance: Make SimpleCapture::stop() idempotent

[Laurent Pinchart]

Hi Nícolas,

Thank you for the patch.

On Thu, Apr 22, 2021 at 06:06:08PM -0300, Nícolas F. R. A. Prado wrote:
> Make SimpleCapture::stop() be able to be called multiple times and at
> any point so that it can be called from the destructor and an assert
> failure can return immeadiately.

s/immeadiately/immediately/

> Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
> ---
>  src/lc-compliance/simple_capture.cpp | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp
> index 811a62200096..e6715ac07f12 100644
> --- a/src/lc-compliance/simple_capture.cpp
> +++ b/src/lc-compliance/simple_capture.cpp
> @@ -17,6 +17,7 @@ SimpleCapture::SimpleCapture(std::shared_ptr<Camera> camera)
>  
>  SimpleCapture::~SimpleCapture()
>  {
> +	stop();
>  }
>  
>  Results::Result SimpleCapture::configure(StreamRole role)
> @@ -52,12 +53,16 @@ Results::Result SimpleCapture::start()
>  
>  void SimpleCapture::stop()
>  {
> +	if (!config_)
> +		return;

Missing blank line.

>  	Stream *stream = config_->at(0).stream();

You could move this line right before allocator_->free(), as that's
where it's used.

>  
>  	camera_->stop();

We'll get an error message if Camera::start() hasn't been called (or has
failed). It may not be a very big issue, but I think it would make sense
to either add a member data field in SimpleCapture to track the camera
state, or make Camera::start() idempotent too. This can be done on top I
think.

>  	camera_->requestCompleted.disconnect(this, &SimpleCapture::requestComplete);
>  
> +	if (!allocator_->allocated())
> +		return;

Missing blank line here too.

>  	allocator_->free(stream);
>  }
>  
> @@ -81,7 +86,6 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests)
>  	if (buffers.size() > numRequests) {
>  		/* Cache buffers.size() before we destroy it in stop() */
>  		int buffers_size = buffers.size();
> -		stop();
>  
>  		return { Results::Skip, "Camera needs " + std::to_string(buffers_size)
>  			+ " requests, can't test only " + std::to_string(numRequests) };
> @@ -96,17 +100,14 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests)
>  	for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
>  		std::unique_ptr<Request> request = camera_->createRequest();
>  		if (!request) {
> -			stop();
>  			return { Results::Fail, "Can't create request" };
>  		}

You can drop the curly braces here and below.

>  
>  		if (request->addBuffer(stream, buffer.get())) {
> -			stop();
>  			return { Results::Fail, "Can't set buffer for request" };
>  		}
>  
>  		if (queueRequest(request.get()) < 0) {
> -			stop();
>  			return { Results::Fail, "Failed to queue request" };
>  		}
>  
> @@ -173,17 +174,14 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests)
>  	for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
>  		std::unique_ptr<Request> request = camera_->createRequest();
>  		if (!request) {
> -			stop();
>  			return { Results::Fail, "Can't create request" };
>  		}
>  
>  		if (request->addBuffer(stream, buffer.get())) {
> -			stop();
>  			return { Results::Fail, "Can't set buffer for request" };
>  		}
>  
>  		if (camera_->queueRequest(request.get()) < 0) {
> -			stop();
>  			return { Results::Fail, "Failed to queue request" };
>  		}
>  

-- 
Regards,

Laurent Pinchart