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

13081 diff mbox series

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

Signed-off-by: Nícolas F. R. A. Prado <nfraprado@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(+)

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@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__ */

Hi Laurent,

On Mon, Aug 02, 2021 at 03:13:56AM +0300, Laurent Pinchart wrote:
> 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 :-)

Indeed, I'll keep an eye on that ;).

> 
> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@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.

I'm not sure I get the concern, but a better name is certainly welcome. I tried
to give it a name that conveyed exactly what it is, which is like you said, a
base class for tests that are parametrized by roles. Maybe still not the best
name, but I feel it's too early to know which kind of tests will subclass this
class to give it a better name.

In any case see my comments below, perhaps it clears any misunderstanding.

> 
> > +{
> > +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();
> > +}

This defines the Overflow test, which subclasses RoleParametrizedTest through
this definition. This means that the Overflow test should receive a role
parameter, which is accessible through GetParam().

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

This instantiates all tests that subclass RoleParametrizedTest by creating a
test for each of the available roles. This means that if we add another test,
Overflow2, also subclassing RoleParametrizedTest, it will also get instantiated
for each of the roles through this same call. This is already currently done
with SingleStream above, which instantiates tests for Capture, CaptureStartStop,
UnbalancedStop all at once. (Btw, the CaptureTests name is mostly irrelevant)

Hopefully this solves any misunderstanding. If not we can discuss a better name
for the class :).

Thanks,
Nícolas

> > 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