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

[Niklas Söderlund]

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

-- 
Regards,
Niklas Söderlund