[libcamera-devel] [PATCH v6 2/5] lc-compliance: Make SimpleCapture::stop() idempotent
Nícolas F. R. A. Prado
nfraprado at collabora.com
Wed Jun 9 21:57:01 CEST 2021
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 at collabora.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at 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 at 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
> >
More information about the libcamera-devel
mailing list