[libcamera-devel] lc-compliance: Add test to call multiple configure()

12249 diff mbox series

Add a test to lc-compliance that calls configure() multiple times on the
camera before starting to capture. This isn't a common pattern for
applications but is allowed and so should be tested by lc-compliance.

Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
---

This tests multiple generateConfiguration() -> validate() -> configure()
sequences, but maybe it would be valuable to test repetitions of each one of
those steps in the test? Although I think repeating the whole sequence should
already catch any bug, it may be better for lc-compliance to test too much
rather than too little.

Repeating each step individually would duplicate a bit of code, and require
SimpleCapture::config_ to be public, but that may be inevitable if we want to
inspect and fiddle with those properties during the tests.

Also, this doesn't really check if the last configured role is the one in effect
during the capture. Is that even something verifiable? It doesn't seem to be
since each pipeline handler decides what each role means.

 src/lc-compliance/single_stream.cpp | 44 ++++++++++++++++++++++++-----
 1 file changed, 37 insertions(+), 7 deletions(-)

Hi Nícolas,

Thanks for your work.

On 2021-05-10 14:43:04 -0300, Nícolas F. R. A. Prado wrote:
> Add a test to lc-compliance that calls configure() multiple times on the
> camera before starting to capture. This isn't a common pattern for
> applications but is allowed and so should be tested by lc-compliance.
> 
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> ---
> 
> This tests multiple generateConfiguration() -> validate() -> configure()
> sequences, but maybe it would be valuable to test repetitions of each one of
> those steps in the test? Although I think repeating the whole sequence should
> already catch any bug, it may be better for lc-compliance to test too much
> rather than too little.
> 
> Repeating each step individually would duplicate a bit of code, and require
> SimpleCapture::config_ to be public, but that may be inevitable if we want to
> inspect and fiddle with those properties during the tests.
> 
> Also, this doesn't really check if the last configured role is the one in effect
> during the capture. Is that even something verifiable? It doesn't seem to be
> since each pipeline handler decides what each role means.
> 
>  src/lc-compliance/single_stream.cpp | 44 ++++++++++++++++++++++++-----
>  1 file changed, 37 insertions(+), 7 deletions(-)
> 
> diff --git a/src/lc-compliance/single_stream.cpp b/src/lc-compliance/single_stream.cpp
> index 8318b42f42d6..6f1c259547a7 100644
> --- a/src/lc-compliance/single_stream.cpp
> +++ b/src/lc-compliance/single_stream.cpp
> @@ -12,6 +12,13 @@
>  
>  using namespace libcamera;
>  
> +static const std::vector<std::pair<std::string, StreamRole>> roles = {
> +	{ "raw", Raw },
> +	{ "still", StillCapture },
> +	{ "video", VideoRecording },
> +	{ "viewfinder", Viewfinder },
> +};

I would keep this in testSingleStream() for now and create a new list in 
testMultipleConfigures(). The smaller the diff the easier it will be 
once the gtest series is merged :-)

The rest looks good to me.

> +
>  Results::Result testRequestBalance(std::shared_ptr<Camera> camera,
>  				   StreamRole role, unsigned int startCycles,
>  				   unsigned int numRequests)
> @@ -33,6 +40,25 @@ Results::Result testRequestBalance(std::shared_ptr<Camera> camera,
>  		std::to_string(startCycles) + " start cycles" };
>  }
>  
> +Results::Result testMultipleConfigures(std::shared_ptr<Camera> camera,
> +				       StreamRole role, unsigned int numRequests)
> +{
> +	Results::Result ret;
> +
> +	SimpleCaptureBalanced capture(camera);
> +
> +	for (auto r: roles) {
> +		ret = capture.configure(r.second);
> +		if (ret.first == Results::Fail)
> +			return ret;
> +	}
> +	ret = capture.configure(role);
> +	if (ret.first != Results::Pass)
> +		return ret;
> +
> +	return capture.capture(numRequests);
> +}
> +
>  Results::Result testRequestUnbalance(std::shared_ptr<Camera> camera,
>  				     StreamRole role, unsigned int numRequests)
>  {
> @@ -47,15 +73,9 @@ Results::Result testRequestUnbalance(std::shared_ptr<Camera> camera,
>  
>  Results testSingleStream(std::shared_ptr<Camera> camera)
>  {
> -	static const std::vector<std::pair<std::string, StreamRole>> roles = {
> -		{ "raw", Raw },
> -		{ "still", StillCapture },
> -		{ "video", VideoRecording },
> -		{ "viewfinder", Viewfinder },
> -	};
>  	static const std::vector<unsigned int> numRequests = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 };
>  
> -	Results results(numRequests.size() * roles.size() * 3);
> +	Results results(numRequests.size() * roles.size() * 3 + roles.size());
>  
>  	for (const auto &role : roles) {
>  		std::cout << "= Test role " << role.first << std::endl;
> @@ -91,6 +111,16 @@ Results testSingleStream(std::shared_ptr<Camera> camera)
>  		std::cout << "* Test unbalanced stop" << std::endl;
>  		for (unsigned int num : numRequests)
>  			results.add(testRequestUnbalance(camera, role.second, num));
> +
> +		/*
> +		 * Test multiple configure()
> +		 *
> +		 * Makes sure that the camera supports being configured multiple
> +		 * times, with only the last one taking effect, before starting
> +		 * to capture.
> +		 */
> +		std::cout << "* Test multiple configure()" << std::endl;
> +		results.add(testMultipleConfigures(camera, role.second, numRequests.back()));
>  	}
>  
>  	return results;
> -- 
> 2.31.1
>