[libcamera-devel,v4,3/3] cam: Only queue the exact number of requests asked for
diff mbox series

Message ID 20210202221051.1740237-4-niklas.soderlund@ragnatech.se
State Accepted
Headers show
Series
  • cam: Fix races in event loop and long request processing times
Related show

Commit Message

Niklas Söderlund Feb. 2, 2021, 10:10 p.m. UTC 
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
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@ragnatech.se>
---
 src/cam/capture.cpp | 16 +++++++++++++---
 src/cam/capture.h   |  2 ++
 2 files changed, 15 insertions(+), 3 deletions(-)

Comments

Jacopo Mondi Feb. 3, 2021, 11:36 a.m. UTC | #1 
Hi Niklas,

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
> 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@ragnatech.se>

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> ---
>  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;
> +
> +	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_;
>
> --
> 2.30.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Feb. 4, 2021, 12:49 a.m. UTC | #2 
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@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@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_;
>

Patch
diff mbox series 

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;
+
+	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_;