[libcamera-devel] [PATCH v4 3/3] cam: Only queue the exact number of requests asked for

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tue, Feb 02, 2021 at 11:10:51PM +0100, Niklas Söderlund wrote:
> The cam option --capture=N is suppose to only capture N requests. But if
> the processing done for each request is large (such as writing it to a
> slow disk) the current implementation could queue more then N requests

s/then/than/

> before the exit condition is detected and capturing stopped.
> 
> Solve this by only queueing N requests while still waiting for N
> requests to complete before exiting.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/cam/capture.cpp | 16 +++++++++++++---
>  src/cam/capture.h   |  2 ++
>  2 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> index 113ea49d50046e5b..e498b562826b8794 100644
> --- a/src/cam/capture.cpp
> +++ b/src/cam/capture.cpp
> @@ -18,7 +18,7 @@ using namespace libcamera;
>  Capture::Capture(std::shared_ptr<Camera> camera, CameraConfiguration *config,
>  		 EventLoop *loop)
>  	: camera_(camera), config_(config), writer_(nullptr), last_(0), loop_(loop),
> -	  captureCount_(0), captureLimit_(0)
> +	  queueCount_(0), captureCount_(0), captureLimit_(0)
>  {
>  }
>  
> @@ -26,6 +26,7 @@ int Capture::run(const OptionsParser::Options &options)
>  {
>  	int ret;
>  
> +	queueCount_ = 0;
>  	captureCount_ = 0;
>  	captureLimit_ = options[OptCapture].toInteger();
>  
> @@ -128,7 +129,7 @@ int Capture::capture(FrameBufferAllocator *allocator)
>  	}
>  
>  	for (std::unique_ptr<Request> &request : requests_) {
> -		ret = camera_->queueRequest(request.get());
> +		ret = queueRequest(request.get());
>  		if (ret < 0) {
>  			std::cerr << "Can't queue request" << std::endl;
>  			camera_->stop();
> @@ -152,6 +153,15 @@ int Capture::capture(FrameBufferAllocator *allocator)
>  	return ret;
>  }
>  
> +int Capture::queueRequest(Request *request)
> +{
> +	queueCount_++;
> +	if (captureLimit_ && queueCount_ > captureLimit_)
> +		return 0;

I'd write this

	if (captureLimit_ && queueCount_ >= captureLimit_)
		return 0;

	queueCount_++;

so that queueCount_ will never go above captureLimit_. The behaviour
will be the same, but it could be less confusing during debugging
sessions when someone may wondering why the queueCount_ value seems to
indicate that more requests are queued than the limit that has been set.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +
> +	return camera_->queueRequest(request);
> +}
> +
>  void Capture::requestComplete(Request *request)
>  {
>  	if (request->status() == Request::RequestCancelled)
> @@ -213,5 +223,5 @@ void Capture::processRequest(Request *request)
>  	}
>  
>  	request->reuse(Request::ReuseBuffers);
> -	camera_->queueRequest(request);
> +	queueRequest(request);
>  }
> diff --git a/src/cam/capture.h b/src/cam/capture.h
> index d21c95a26ce7d83a..c7c9dc00d30f06d6 100644
> --- a/src/cam/capture.h
> +++ b/src/cam/capture.h
> @@ -32,6 +32,7 @@ public:
>  private:
>  	int capture(libcamera::FrameBufferAllocator *allocator);
>  
> +	int queueRequest(libcamera::Request *request);
>  	void requestComplete(libcamera::Request *request);
>  	void processRequest(libcamera::Request *request);
>  
> @@ -43,6 +44,7 @@ private:
>  	uint64_t last_;
>  
>  	EventLoop *loop_;
> +	unsigned int queueCount_;
>  	unsigned int captureCount_;
>  	unsigned int captureLimit_;
>  

-- 
Regards,

Laurent Pinchart