[libcamera-devel] [PATCH v7 04/11] lc-compliance: Factor common capture code into SimpleCapture

[Laurent Pinchart]

Hi Nícolas,

Thank you for the patch.

On Thu, Jul 22, 2021 at 08:28:44PM -0300, Nícolas F. R. A. Prado wrote:
> Factor common code from SimpleCapture* subclasses into the SimpleCapture
> parent class in order to avoid duplicated code.
> 
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
> ---
> 
> No changes in v7
> 
> Changes in v6:
> - Added missing blank line before return in captureCompleted()
> - Switched queueRequests()'s 'buffers' and 'requests' parameters order, since
>   'requests' is an output variable
> - Added comment and blank line to runCaptureSession() 
> 
>  src/lc-compliance/simple_capture.cpp | 101 ++++++++++++++++-----------
>  src/lc-compliance/simple_capture.h   |  16 +++--
>  2 files changed, 72 insertions(+), 45 deletions(-)
> 
> diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp
> index 8683d9050806..06ef44ef7e42 100644
> --- a/src/lc-compliance/simple_capture.cpp
> +++ b/src/lc-compliance/simple_capture.cpp
> @@ -78,6 +78,45 @@ void SimpleCapture::stop()
>  	allocator_->free(stream);
>  }
>  
> +bool SimpleCapture::captureCompleted()
> +{
> +	captureCount_++;
> +	if (captureCount_ >= captureLimit_) {
> +		loop_->exit(0);
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +void SimpleCapture::queueRequests(Stream *stream,
> +				  const std::vector<std::unique_ptr<FrameBuffer>> &buffers,
> +				  std::vector<std::unique_ptr<libcamera::Request>> &requests)

The fact that the caller is supposed to pass a vector of requests and
keep it valid for the duration of the capture session is a bit
confusing, especially without documentation. One option would be to
store the requests in a member variable that you could clear in stop().

> +{
> +	for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
> +		std::unique_ptr<Request> request = camera_->createRequest();
> +		ASSERT_TRUE(request) << "Can't create request";
> +
> +		ASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << "Can't set buffer for request";
> +
> +		ASSERT_EQ(camera_->queueRequest(request.get()), 0) << "Failed to queue request";

If you added a

	virtual int queueRequest(Request *request);

implemented as

int SimpleCapture::queueRequest(Request *request)
{
	return camera_->queueRequest(request);
}

and called queueRequest() here instead of camera_->queueRequest(), you
wouldn't need to implement a separate
SimpleCaptureBalanced::queueRequests().

> +
> +		requests.push_back(std::move(request));
> +	}
> +}
> +
> +int SimpleCapture::runCaptureSession()
> +{
> +	loop_ = new EventLoop();
> +	int status = loop_->exec();
> +
> +	/* After session ended */
> +	stop();
> +	delete loop_;
> +
> +	return status;
> +}
> +
>  /* SimpleCaptureBalanced */
>  
>  SimpleCaptureBalanced::SimpleCaptureBalanced(std::shared_ptr<Camera> camera)
> @@ -85,6 +124,22 @@ SimpleCaptureBalanced::SimpleCaptureBalanced(std::shared_ptr<Camera> camera)
>  {
>  }
>  
> +void SimpleCaptureBalanced::queueRequests(Stream *stream,
> +					  const std::vector<std::unique_ptr<FrameBuffer>> &buffers,
> +					  std::vector<std::unique_ptr<libcamera::Request>> &requests)
> +{
> +	for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
> +		std::unique_ptr<Request> request = camera_->createRequest();
> +		ASSERT_TRUE(request) << "Can't create request";
> +
> +		ASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << "Can't set buffer for request";
> +
> +		ASSERT_EQ(queueRequest(request.get()), 0) << "Failed to queue request";
> +
> +		requests.push_back(std::move(request));
> +	}
> +}
> +
>  void SimpleCaptureBalanced::capture(unsigned int numRequests)
>  {
>  	start();
> @@ -104,24 +159,10 @@ void SimpleCaptureBalanced::capture(unsigned int numRequests)
>  	captureCount_ = 0;
>  	captureLimit_ = numRequests;
>  
> -	/* Queue the recommended number of reqeuests. */
>  	std::vector<std::unique_ptr<libcamera::Request>> requests;
> -	for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
> -		std::unique_ptr<Request> request = camera_->createRequest();
> -		ASSERT_TRUE(request) << "Can't create request";
> -
> -		ASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << "Can't set buffer for request";
> -
> -		ASSERT_EQ(queueRequest(request.get()), 0) << "Failed to queue request";
> +	queueRequests(stream, buffers, requests);
>  
> -		requests.push_back(std::move(request));
> -	}
> -
> -	/* Run capture session. */
> -	loop_ = new EventLoop();
> -	loop_->exec();
> -	stop();
> -	delete loop_;
> +	runCaptureSession();
>  
>  	ASSERT_EQ(captureCount_, captureLimit_);
>  }
> @@ -137,11 +178,8 @@ int SimpleCaptureBalanced::queueRequest(Request *request)
>  
>  void SimpleCaptureBalanced::requestComplete(Request *request)
>  {
> -	captureCount_++;
> -	if (captureCount_ >= captureLimit_) {
> -		loop_->exit(0);
> +	if (captureCompleted())
>  		return;
> -	}
>  
>  	request->reuse(Request::ReuseBuffers);
>  	if (queueRequest(request))
> @@ -165,35 +203,18 @@ void SimpleCaptureUnbalanced::capture(unsigned int numRequests)
>  	captureCount_ = 0;
>  	captureLimit_ = numRequests;
>  
> -	/* Queue the recommended number of reqeuests. */
>  	std::vector<std::unique_ptr<libcamera::Request>> requests;
> -	for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
> -		std::unique_ptr<Request> request = camera_->createRequest();
> -		ASSERT_TRUE(request) << "Can't create request";
> -
> -		ASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << "Can't set buffer for request";
> -
> -		ASSERT_EQ(camera_->queueRequest(request.get()), 0) << "Failed to queue request";
> +	queueRequests(stream, buffers, requests);
>  
> -		requests.push_back(std::move(request));
> -	}
> -
> -	/* Run capture session. */
> -	loop_ = new EventLoop();
> -	int status = loop_->exec();
> -	stop();
> -	delete loop_;
> +	int status = runCaptureSession();
>  
>  	ASSERT_EQ(status, 0);
>  }
>  
>  void SimpleCaptureUnbalanced::requestComplete(Request *request)
>  {
> -	captureCount_++;
> -	if (captureCount_ >= captureLimit_) {
> -		loop_->exit(0);
> +	if (captureCompleted())
>  		return;
> -	}
>  
>  	request->reuse(Request::ReuseBuffers);
>  	if (camera_->queueRequest(request))
> diff --git a/src/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h
> index 1a1e874a528c..0f9a060fece3 100644
> --- a/src/lc-compliance/simple_capture.h
> +++ b/src/lc-compliance/simple_capture.h
> @@ -26,11 +26,19 @@ protected:
>  
>  	void start();
>  	void stop();
> +	void queueRequests(libcamera::Stream *stream,
> +			   const std::vector<std::unique_ptr<libcamera::FrameBuffer>> &buffers,
> +			   std::vector<std::unique_ptr<libcamera::Request>> &requests);
> +	bool captureCompleted();
> +	int runCaptureSession();
>  
>  	virtual void requestComplete(libcamera::Request *request) = 0;
>  
>  	EventLoop *loop_;
>  
> +	unsigned int captureCount_;
> +	unsigned int captureLimit_;
> +
>  	std::shared_ptr<libcamera::Camera> camera_;
>  	std::unique_ptr<libcamera::FrameBufferAllocator> allocator_;
>  	std::unique_ptr<libcamera::CameraConfiguration> config_;
> @@ -46,10 +54,11 @@ public:
>  private:
>  	int queueRequest(libcamera::Request *request);
>  	void requestComplete(libcamera::Request *request) override;
> +	void queueRequests(libcamera::Stream *stream,
> +			   const std::vector<std::unique_ptr<libcamera::FrameBuffer>> &buffers,
> +			   std::vector<std::unique_ptr<libcamera::Request>> &requests);
>  
>  	unsigned int queueCount_;
> -	unsigned int captureCount_;
> -	unsigned int captureLimit_;
>  };
>  
>  class SimpleCaptureUnbalanced : public SimpleCapture
> @@ -61,9 +70,6 @@ public:
>  
>  private:
>  	void requestComplete(libcamera::Request *request) override;
> -
> -	unsigned int captureCount_;
> -	unsigned int captureLimit_;
>  };
>  
>  #endif /* __LC_COMPLIANCE_SIMPLE_CAPTURE_H__ */

-- 
Regards,

Laurent Pinchart