[libcamera-devel,v6,2/5] lc-compliance: Make SimpleCapture::stop() idempotent
diff mbox series

Message ID 20210607181506.51711-3-nfraprado@collabora.com
State Superseded
Headers show
Series
  • lc-compliance: Refactor using Googletest
Related show

Commit Message

Nícolas F. R. A. Prado June 7, 2021, 6:15 p.m. UTC 
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 immediately.

Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
No changes in v6

No changes in v5

No changes in v4

No changes in v3

Changes in v2:
- Suggested by Laurent:
  - Fixed code style

 src/lc-compliance/simple_capture.cpp | 33 +++++++++++-----------------
 1 file changed, 13 insertions(+), 20 deletions(-)

Comments

Jacopo Mondi June 9, 2021, 4:34 p.m. UTC | #1 
Hi Nicolas,

On Mon, Jun 07, 2021 at 03:15:03PM -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 immediately.
>
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> No changes in v6
>
> No changes in v5
>
> No changes in v4
>
> No changes in v3
>
> Changes in v2:
> - Suggested by Laurent:
>   - Fixed code style
>
>  src/lc-compliance/simple_capture.cpp | 33 +++++++++++-----------------
>  1 file changed, 13 insertions(+), 20 deletions(-)
>
> diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp
> index 64e862a08e3a..f90fe6d0f9aa 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)
> @@ -55,12 +56,17 @@ Results::Result SimpleCapture::start()
>
>  void SimpleCapture::stop()
>  {
> -	Stream *stream = config_->at(0).stream();
> +	if (!config_)
> +		return;
>
>  	camera_->stop();
>
>  	camera_->requestCompleted.disconnect(this, &SimpleCapture::requestComplete);

nit: I think these two can be moved after the allocator_->allocated() check
below as in start()

	ASSERT_GE(allocator_->allocate(stream), 0) << "Failed to allocate buffers";

	ASSERT_TRUE(!camera_->start()) << "Failed to start camera";

	camera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);

I would also respect the inverse order and disconnect the signal
first, then stop the camera.

All minors
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

>
> +	if (!allocator_->allocated())
> +		return;
> +
> +	Stream *stream = config_->at(0).stream();
>  	allocator_->free(stream);
>  }
>
> @@ -84,7 +90,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) };
> @@ -98,20 +103,14 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests)
>  	std::vector<std::unique_ptr<libcamera::Request>> requests;
>  	for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
>  		std::unique_ptr<Request> request = camera_->createRequest();
> -		if (!request) {
> -			stop();
> +		if (!request)
>  			return { Results::Fail, "Can't create request" };
> -		}
>
> -		if (request->addBuffer(stream, buffer.get())) {
> -			stop();
> +		if (request->addBuffer(stream, buffer.get()))
>  			return { Results::Fail, "Can't set buffer for request" };
> -		}
>
> -		if (queueRequest(request.get()) < 0) {
> -			stop();
> +		if (queueRequest(request.get()) < 0)
>  			return { Results::Fail, "Failed to queue request" };
> -		}
>
>  		requests.push_back(std::move(request));
>  	}
> @@ -175,20 +174,14 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests)
>  	std::vector<std::unique_ptr<libcamera::Request>> requests;
>  	for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
>  		std::unique_ptr<Request> request = camera_->createRequest();
> -		if (!request) {
> -			stop();
> +		if (!request)
>  			return { Results::Fail, "Can't create request" };
> -		}
>
> -		if (request->addBuffer(stream, buffer.get())) {
> -			stop();
> +		if (request->addBuffer(stream, buffer.get()))
>  			return { Results::Fail, "Can't set buffer for request" };
> -		}
>
> -		if (camera_->queueRequest(request.get()) < 0) {
> -			stop();
> +		if (camera_->queueRequest(request.get()) < 0)
>  			return { Results::Fail, "Failed to queue request" };
> -		}
>
>  		requests.push_back(std::move(request));
>  	}
> --
> 2.31.1
>
Nícolas F. R. A. Prado June 9, 2021, 7:57 p.m. UTC | #2 
Hi Jacopo,

On Wed, Jun 09, 2021 at 06:34:28PM +0200, Jacopo Mondi wrote:
> Hi Nicolas,
> 
> On Mon, Jun 07, 2021 at 03:15:03PM -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 immediately.
> >
> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > No changes in v6
> >
> > No changes in v5
> >
> > No changes in v4
> >
> > No changes in v3
> >
> > Changes in v2:
> > - Suggested by Laurent:
> >   - Fixed code style
> >
> >  src/lc-compliance/simple_capture.cpp | 33 +++++++++++-----------------
> >  1 file changed, 13 insertions(+), 20 deletions(-)
> >
> > diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp
> > index 64e862a08e3a..f90fe6d0f9aa 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)
> > @@ -55,12 +56,17 @@ Results::Result SimpleCapture::start()
> >
> >  void SimpleCapture::stop()
> >  {
> > -	Stream *stream = config_->at(0).stream();
> > +	if (!config_)
> > +		return;
> >
> >  	camera_->stop();
> >
> >  	camera_->requestCompleted.disconnect(this, &SimpleCapture::requestComplete);
> 
> nit: I think these two can be moved after the allocator_->allocated() check
> below as in start()
> 
> 	ASSERT_GE(allocator_->allocate(stream), 0) << "Failed to allocate buffers";
> 
> 	ASSERT_TRUE(!camera_->start()) << "Failed to start camera";
> 
> 	camera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);

Right, makes sense. In that case I'll join both checks for config_ and
allocator_->allocated() in a single if at the beginning of the function.

> 
> I would also respect the inverse order and disconnect the signal
> first, then stop the camera.

Okay. So I suppose it won't cause any issues if requests complete after we
disconnect the signal.

> 
> All minors
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks :)

Nícolas

> 
> Thanks
>    j
> 
> >
> > +	if (!allocator_->allocated())
> > +		return;
> > +
> > +	Stream *stream = config_->at(0).stream();
> >  	allocator_->free(stream);
> >  }
> >
> > @@ -84,7 +90,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) };
> > @@ -98,20 +103,14 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests)
> >  	std::vector<std::unique_ptr<libcamera::Request>> requests;
> >  	for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
> >  		std::unique_ptr<Request> request = camera_->createRequest();
> > -		if (!request) {
> > -			stop();
> > +		if (!request)
> >  			return { Results::Fail, "Can't create request" };
> > -		}
> >
> > -		if (request->addBuffer(stream, buffer.get())) {
> > -			stop();
> > +		if (request->addBuffer(stream, buffer.get()))
> >  			return { Results::Fail, "Can't set buffer for request" };
> > -		}
> >
> > -		if (queueRequest(request.get()) < 0) {
> > -			stop();
> > +		if (queueRequest(request.get()) < 0)
> >  			return { Results::Fail, "Failed to queue request" };
> > -		}
> >
> >  		requests.push_back(std::move(request));
> >  	}
> > @@ -175,20 +174,14 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests)
> >  	std::vector<std::unique_ptr<libcamera::Request>> requests;
> >  	for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
> >  		std::unique_ptr<Request> request = camera_->createRequest();
> > -		if (!request) {
> > -			stop();
> > +		if (!request)
> >  			return { Results::Fail, "Can't create request" };
> > -		}
> >
> > -		if (request->addBuffer(stream, buffer.get())) {
> > -			stop();
> > +		if (request->addBuffer(stream, buffer.get()))
> >  			return { Results::Fail, "Can't set buffer for request" };
> > -		}
> >
> > -		if (camera_->queueRequest(request.get()) < 0) {
> > -			stop();
> > +		if (camera_->queueRequest(request.get()) < 0)
> >  			return { Results::Fail, "Failed to queue request" };
> > -		}
> >
> >  		requests.push_back(std::move(request));
> >  	}
> > --
> > 2.31.1
> >
Laurent Pinchart June 10, 2021, 1:41 a.m. UTC | #3 
Hello,

On Wed, Jun 09, 2021 at 04:57:01PM -0300, Nícolas F. R. A. Prado wrote:
> On Wed, Jun 09, 2021 at 06:34:28PM +0200, Jacopo Mondi wrote:
> > On Mon, Jun 07, 2021 at 03:15:03PM -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 immediately.
> > >
> > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > No changes in v6
> > >
> > > No changes in v5
> > >
> > > No changes in v4
> > >
> > > No changes in v3
> > >
> > > Changes in v2:
> > > - Suggested by Laurent:
> > >   - Fixed code style
> > >
> > >  src/lc-compliance/simple_capture.cpp | 33 +++++++++++-----------------
> > >  1 file changed, 13 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp
> > > index 64e862a08e3a..f90fe6d0f9aa 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)
> > > @@ -55,12 +56,17 @@ Results::Result SimpleCapture::start()
> > >
> > >  void SimpleCapture::stop()
> > >  {
> > > -	Stream *stream = config_->at(0).stream();
> > > +	if (!config_)
> > > +		return;
> > >
> > >  	camera_->stop();
> > >
> > >  	camera_->requestCompleted.disconnect(this, &SimpleCapture::requestComplete);
> > 
> > nit: I think these two can be moved after the allocator_->allocated() check
> > below as in start()
> > 
> > 	ASSERT_GE(allocator_->allocate(stream), 0) << "Failed to allocate buffers";
> > 
> > 	ASSERT_TRUE(!camera_->start()) << "Failed to start camera";
> > 
> > 	camera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);
> 
> Right, makes sense. In that case I'll join both checks for config_ and
> allocator_->allocated() in a single if at the beginning of the function.
> 
> > I would also respect the inverse order and disconnect the signal
> > first, then stop the camera.
> 
> Okay. So I suppose it won't cause any issues if requests complete after we
> disconnect the signal.

It could result in memory leaks, so I wouldn't do so.

> > All minors
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> Thanks :)
> 
> > > +	if (!allocator_->allocated())
> > > +		return;
> > > +
> > > +	Stream *stream = config_->at(0).stream();
> > >  	allocator_->free(stream);
> > >  }
> > >
> > > @@ -84,7 +90,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) };
> > > @@ -98,20 +103,14 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests)
> > >  	std::vector<std::unique_ptr<libcamera::Request>> requests;
> > >  	for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
> > >  		std::unique_ptr<Request> request = camera_->createRequest();
> > > -		if (!request) {
> > > -			stop();
> > > +		if (!request)
> > >  			return { Results::Fail, "Can't create request" };
> > > -		}
> > >
> > > -		if (request->addBuffer(stream, buffer.get())) {
> > > -			stop();
> > > +		if (request->addBuffer(stream, buffer.get()))
> > >  			return { Results::Fail, "Can't set buffer for request" };
> > > -		}
> > >
> > > -		if (queueRequest(request.get()) < 0) {
> > > -			stop();
> > > +		if (queueRequest(request.get()) < 0)
> > >  			return { Results::Fail, "Failed to queue request" };
> > > -		}
> > >
> > >  		requests.push_back(std::move(request));
> > >  	}
> > > @@ -175,20 +174,14 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests)
> > >  	std::vector<std::unique_ptr<libcamera::Request>> requests;
> > >  	for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
> > >  		std::unique_ptr<Request> request = camera_->createRequest();
> > > -		if (!request) {
> > > -			stop();
> > > +		if (!request)
> > >  			return { Results::Fail, "Can't create request" };
> > > -		}
> > >
> > > -		if (request->addBuffer(stream, buffer.get())) {
> > > -			stop();
> > > +		if (request->addBuffer(stream, buffer.get()))
> > >  			return { Results::Fail, "Can't set buffer for request" };
> > > -		}
> > >
> > > -		if (camera_->queueRequest(request.get()) < 0) {
> > > -			stop();
> > > +		if (camera_->queueRequest(request.get()) < 0)
> > >  			return { Results::Fail, "Failed to queue request" };
> > > -		}
> > >
> > >  		requests.push_back(std::move(request));
> > >  	}
Jacopo Mondi June 10, 2021, 7:49 a.m. UTC | #4 
Hello,

On Thu, Jun 10, 2021 at 04:41:20AM +0300, Laurent Pinchart wrote:
> Hello,
>
> > Right, makes sense. In that case I'll join both checks for config_ and
> > allocator_->allocated() in a single if at the beginning of the function.
> >
> > > I would also respect the inverse order and disconnect the signal
> > > first, then stop the camera.
> >
> > Okay. So I suppose it won't cause any issues if requests complete after we
> > disconnect the signal.
>
> It could result in memory leaks, so I wouldn't do so.
>

Yeah indeed, please ignore my comment then :)

> > > All minors
> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> >
> > Thanks :)
> >
> > > > +	if (!allocator_->allocated())
> > > > +		return;
> > > > +
> > > > +	Stream *stream = config_->at(0).stream();
> > > >  	allocator_->free(stream);
> > > >  }
> > > >
> > > > @@ -84,7 +90,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) };
> > > > @@ -98,20 +103,14 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests)
> > > >  	std::vector<std::unique_ptr<libcamera::Request>> requests;
> > > >  	for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
> > > >  		std::unique_ptr<Request> request = camera_->createRequest();
> > > > -		if (!request) {
> > > > -			stop();
> > > > +		if (!request)
> > > >  			return { Results::Fail, "Can't create request" };
> > > > -		}
> > > >
> > > > -		if (request->addBuffer(stream, buffer.get())) {
> > > > -			stop();
> > > > +		if (request->addBuffer(stream, buffer.get()))
> > > >  			return { Results::Fail, "Can't set buffer for request" };
> > > > -		}
> > > >
> > > > -		if (queueRequest(request.get()) < 0) {
> > > > -			stop();
> > > > +		if (queueRequest(request.get()) < 0)
> > > >  			return { Results::Fail, "Failed to queue request" };
> > > > -		}
> > > >
> > > >  		requests.push_back(std::move(request));
> > > >  	}
> > > > @@ -175,20 +174,14 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests)
> > > >  	std::vector<std::unique_ptr<libcamera::Request>> requests;
> > > >  	for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
> > > >  		std::unique_ptr<Request> request = camera_->createRequest();
> > > > -		if (!request) {
> > > > -			stop();
> > > > +		if (!request)
> > > >  			return { Results::Fail, "Can't create request" };
> > > > -		}
> > > >
> > > > -		if (request->addBuffer(stream, buffer.get())) {
> > > > -			stop();
> > > > +		if (request->addBuffer(stream, buffer.get()))
> > > >  			return { Results::Fail, "Can't set buffer for request" };
> > > > -		}
> > > >
> > > > -		if (camera_->queueRequest(request.get()) < 0) {
> > > > -			stop();
> > > > +		if (camera_->queueRequest(request.get()) < 0)
> > > >  			return { Results::Fail, "Failed to queue request" };
> > > > -		}
> > > >
> > > >  		requests.push_back(std::move(request));
> > > >  	}
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp
index 64e862a08e3a..f90fe6d0f9aa 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)
@@ -55,12 +56,17 @@  Results::Result SimpleCapture::start()
 
 void SimpleCapture::stop()
 {
-	Stream *stream = config_->at(0).stream();
+	if (!config_)
+		return;
 
 	camera_->stop();
 
 	camera_->requestCompleted.disconnect(this, &SimpleCapture::requestComplete);
 
+	if (!allocator_->allocated())
+		return;
+
+	Stream *stream = config_->at(0).stream();
 	allocator_->free(stream);
 }
 
@@ -84,7 +90,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) };
@@ -98,20 +103,14 @@  Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests)
 	std::vector<std::unique_ptr<libcamera::Request>> requests;
 	for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
 		std::unique_ptr<Request> request = camera_->createRequest();
-		if (!request) {
-			stop();
+		if (!request)
 			return { Results::Fail, "Can't create request" };
-		}
 
-		if (request->addBuffer(stream, buffer.get())) {
-			stop();
+		if (request->addBuffer(stream, buffer.get()))
 			return { Results::Fail, "Can't set buffer for request" };
-		}
 
-		if (queueRequest(request.get()) < 0) {
-			stop();
+		if (queueRequest(request.get()) < 0)
 			return { Results::Fail, "Failed to queue request" };
-		}
 
 		requests.push_back(std::move(request));
 	}
@@ -175,20 +174,14 @@  Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests)
 	std::vector<std::unique_ptr<libcamera::Request>> requests;
 	for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
 		std::unique_ptr<Request> request = camera_->createRequest();
-		if (!request) {
-			stop();
+		if (!request)
 			return { Results::Fail, "Can't create request" };
-		}
 
-		if (request->addBuffer(stream, buffer.get())) {
-			stop();
+		if (request->addBuffer(stream, buffer.get()))
 			return { Results::Fail, "Can't set buffer for request" };
-		}
 
-		if (camera_->queueRequest(request.get()) < 0) {
-			stop();
+		if (camera_->queueRequest(request.get()) < 0)
 			return { Results::Fail, "Failed to queue request" };
-		}
 
 		requests.push_back(std::move(request));
 	}