[libcamera-devel] [PATCH v7 07/11] lc-compliance: Add test to queue more requests than hardware depth

[Laurent Pinchart]

Hi Nícolas,

Thank you for the patch.

On Thu, Jul 22, 2021 at 08:28:47PM -0300, Nícolas F. R. A. Prado wrote:
> A pipeline handler should be able to handle an arbitrary number of
> simultaneous requests by submitting what it can to the video device and
> queuing the rest internally until resources are available. This isn't
> currently done by some pipeline handlers however [1].
> 
> Add a new test to lc-compliance that submits a lot of requests at once
> to check if the pipeline handler is behaving well in this situation.
> 
> [1] https://bugs.libcamera.org/show_bug.cgi?id=24

The commit message may need to be updated depending on what gets merged
first :-)

> Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
> ---
> 
> No changes in v7
> 
> Changes in v6:
> - Made MAX_SIMULTANEOUS_REQUESTS a constexpr
> 
> Changes in v5:
> - Updated to use googletest, and CameraHolder and roleToString from previous
>   patches
> 
>  src/lc-compliance/capture_test.cpp   | 45 ++++++++++++++++++++++++++++
>  src/lc-compliance/simple_capture.cpp | 30 +++++++++++++++++++
>  src/lc-compliance/simple_capture.h   | 11 +++++++
>  3 files changed, 86 insertions(+)
> 
> diff --git a/src/lc-compliance/capture_test.cpp b/src/lc-compliance/capture_test.cpp
> index b4807486ee07..a7ba7448a21b 100644
> --- a/src/lc-compliance/capture_test.cpp
> +++ b/src/lc-compliance/capture_test.cpp
> @@ -18,6 +18,8 @@ using namespace libcamera;
>  const std::vector<int> NUMREQUESTS = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 };
>  const std::vector<StreamRole> ROLES = { Raw, StillCapture, VideoRecording, Viewfinder };
>  
> +static constexpr unsigned int MAX_SIMULTANEOUS_REQUESTS = 8;
> +
>  static const std::string &roleToString(const StreamRole &role)
>  {
>  	static std::map<StreamRole, std::string> rolesMap = { { Raw, "Raw" },
> @@ -79,12 +81,37 @@ void SingleStream::TearDown()
>  	releaseCamera();
>  }
>  
> +class RoleParametrizedTest : public testing::TestWithParam<StreamRole>, public CameraHolder

Maybe it's due to my bad understanding of the test framework, but the
name of this class confuses me. I understand it as a base class for
tests that are parametrized by roles, but below it's used directly as
the test fixture when creating tests.

> +{
> +public:
> +	static std::string nameParameters(const testing::TestParamInfo<RoleParametrizedTest::ParamType> &info);
> +
> +protected:
> +	void SetUp() override;
> +	void TearDown() override;
> +};
> +
> +void RoleParametrizedTest::SetUp()
> +{
> +	acquireCamera();
> +}
> +
> +void RoleParametrizedTest::TearDown()
> +{
> +	releaseCamera();
> +}
> +
>  std::string SingleStream::nameParameters(const testing::TestParamInfo<SingleStream::ParamType> &info)
>  {
>  	return roleToString(std::get<0>(info.param)) + "_" +
>  	       std::to_string(std::get<1>(info.param));
>  }
>  
> +std::string RoleParametrizedTest::nameParameters(const testing::TestParamInfo<RoleParametrizedTest::ParamType> &info)
> +{
> +	return roleToString(info.param);
> +}
> +
>  /*
>   * Test single capture cycles
>   *
> @@ -147,8 +174,26 @@ TEST_P(SingleStream, UnbalancedStop)
>  	capture.capture(numRequests);
>  }
>  
> +TEST_P(RoleParametrizedTest, Overflow)
> +{
> +	auto role = GetParam();
> +
> +	SimpleCaptureOverflow capture(camera_);
> +
> +	capture.configure(role);
> +
> +	capture.allocateBuffers(MAX_SIMULTANEOUS_REQUESTS);
> +
> +	capture.capture();
> +}
> +
>  INSTANTIATE_TEST_SUITE_P(CaptureTests,
>  			 SingleStream,
>  			 testing::Combine(testing::ValuesIn(ROLES),
>  					  testing::ValuesIn(NUMREQUESTS)),
>  			 SingleStream::nameParameters);
> +
> +INSTANTIATE_TEST_SUITE_P(CaptureTests,
> +			 RoleParametrizedTest,
> +			 testing::ValuesIn(ROLES),
> +			 RoleParametrizedTest::nameParameters);
> diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp
> index 06ef44ef7e42..48ce8f088e71 100644
> --- a/src/lc-compliance/simple_capture.cpp
> +++ b/src/lc-compliance/simple_capture.cpp
> @@ -220,3 +220,33 @@ void SimpleCaptureUnbalanced::requestComplete(Request *request)
>  	if (camera_->queueRequest(request))
>  		loop_->exit(-EINVAL);
>  }
> +
> +/* SimpleCaptureOverflow */
> +
> +SimpleCaptureOverflow::SimpleCaptureOverflow(std::shared_ptr<Camera> camera)
> +	: SimpleCapture(camera)
> +{
> +}
> +
> +void SimpleCaptureOverflow::capture()
> +{
> +	start();
> +
> +	Stream *stream = config_->at(0).stream();
> +	const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream);
> +
> +	captureCount_ = 0;
> +	captureLimit_ = buffers.size();
> +
> +	std::vector<std::unique_ptr<libcamera::Request>> requests;
> +	queueRequests(stream, buffers, requests);
> +
> +	runCaptureSession();
> +
> +	ASSERT_EQ(captureCount_, captureLimit_);
> +}
> +
> +void SimpleCaptureOverflow::requestComplete([[maybe_unused]] Request *request)
> +{
> +	captureCompleted();
> +}
> diff --git a/src/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h
> index 0f9a060fece3..2f4960584642 100644
> --- a/src/lc-compliance/simple_capture.h
> +++ b/src/lc-compliance/simple_capture.h
> @@ -72,4 +72,15 @@ private:
>  	void requestComplete(libcamera::Request *request) override;
>  };
>  
> +class SimpleCaptureOverflow : public SimpleCapture
> +{
> +public:
> +	SimpleCaptureOverflow(std::shared_ptr<libcamera::Camera> camera);
> +
> +	void capture();
> +
> +private:
> +	void requestComplete(libcamera::Request *request) override;
> +};
> +
>  #endif /* __LC_COMPLIANCE_SIMPLE_CAPTURE_H__ */

-- 
Regards,

Laurent Pinchart