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

[Laurent Pinchart]

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.

> 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