[libcamera-devel] [PATCH 2/2] cam: Fix capturing an precis number of requests

Niklas Söderlund niklas.soderlund at ragnatech.se
Tue Jan 19 14:14:53 CET 2021


Hi Laurent,

Thanks for your feedback.

On 2021-01-19 14:50:04 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Tue, Jan 19, 2021 at 01:31:10PM +0100, Niklas Söderlund wrote:
> > When moving request processing from the camera manager thread to the
> > main thread a subtle race condition where added when running with the
> > --capture=N option.
> > 
> > Before the change the check of how many request had been queued was ran
> > in the camera manager thread and thus could not race with the producer
> > thread. After the change if requests are completed faster then they are
> > consumed (the consumer writes them to disk for example) the check may be
> > delayed and more then the expected number of request may be asked to
> > processed.
> 
> I'm not sure to see the problem. Capture::processRequest() is called
> asynchronously on request completion, but the calls are serialized. As
> we don't requeue requests in Capture::requestComplete(), but in
> Capture::processRequest(), and only after the captureLimit_ check, we
> should never queue more than captureLimit_ requests. With this change
> we'll terminate the event loop early, which means that some completed
> requests may fail to reach Capture::processRequest(), and won't be
> written to a file.

The problem is that we have more then one request queued to libcamera at 
a time. If the check is at the end of Capture::processRequest() and the 
processing time for each request is large more then N requests are 
completed and placed on the callback queue and process. Worst case is 
that we write N + pipeline depth of frames to disk.

One could argue that the design in cam is bad and we should not count 
number of completed requests but rather how many we queue to the camera.  
I think this is something we could switch. But I feel a lot more 
comfortable if we first had a compliance tool to verify that all 
pipelines actually return N requests if N requests where queue ;-P

> 
> > Sebastian Fricke <sebastian.fricke at posteo.net>
> > Fixes: 02eae70e15bdbb24 ("cam: Move request processing to main thread")
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  src/cam/capture.cpp | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> > index 113ea49d50046e5b..ef1601c716bfa137 100644
> > --- a/src/cam/capture.cpp
> > +++ b/src/cam/capture.cpp
> > @@ -157,6 +157,12 @@ void Capture::requestComplete(Request *request)
> >  	if (request->status() == Request::RequestCancelled)
> >  		return;
> >  
> > +	captureCount_++;
> > +	if (captureLimit_ && captureCount_ > captureLimit_) {
> > +		loop_->exit(0);
> > +		return;
> > +	}
> > +
> >  	/*
> >  	 * Defer processing of the completed request to the event loop, to avoid
> >  	 * blocking the camera manager thread.
> > @@ -206,12 +212,6 @@ void Capture::processRequest(Request *request)
> >  
> >  	std::cout << info.str() << std::endl;
> >  
> > -	captureCount_++;
> > -	if (captureLimit_ && captureCount_ >= captureLimit_) {
> > -		loop_->exit(0);
> > -		return;
> > -	}
> > -
> >  	request->reuse(Request::ReuseBuffers);
> >  	camera_->queueRequest(request);
> >  }
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund


More information about the libcamera-devel mailing list