| Message ID | 20210422210609.425987-2-nfraprado@collabora.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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" }; > } >
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; Stream *stream = config_->at(0).stream(); camera_->stop(); camera_->requestCompleted.disconnect(this, &SimpleCapture::requestComplete); + if (!allocator_->allocated()) + return; 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" }; } 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" }; }
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. Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> --- src/lc-compliance/simple_capture.cpp | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)