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

[Jacopo Mondi]

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