[libcamera-devel] [PATCH 3/5] test: camera: Add setting of format test

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Wed, Mar 06, 2019 at 03:47:53AM +0100, Niklas Söderlund wrote:
> Try to set the default format, a modified valid format and an invalid
> format.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  test/camera/format_set.cpp | 88 ++++++++++++++++++++++++++++++++++++++
>  test/camera/meson.build    |  1 +
>  2 files changed, 89 insertions(+)
>  create mode 100644 test/camera/format_set.cpp
> 
> diff --git a/test/camera/format_set.cpp b/test/camera/format_set.cpp
> new file mode 100644
> index 0000000000000000..948428ca6b00d2d0
> --- /dev/null
> +++ b/test/camera/format_set.cpp
> @@ -0,0 +1,88 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * libcamera Camera API tests
> + */
> +
> +#include <iostream>
> +
> +#include "camera_test.h"
> +
> +using namespace std;
> +
> +namespace {
> +
> +class FormatSet : public CameraTest
> +{
> +protected:
> +	int run()
> +	{
> +		if (camera_->acquire()) {
> +			cout << "Acquiring the camera failed" << endl;
> +			return TestFail;
> +		}
> +
> +		std::set<Stream *> streams = { *camera_->streams().begin() };
> +		std::map<Stream *, StreamConfiguration> conf =
> +			camera_->streamConfiguration(streams);
> +		StreamConfiguration *sconf = &conf.begin()->second;
> +		if (conf.size() != 1) {
> +			cout << "Reading default format failed" << endl;
> +			return TestFail;
> +		}

This could use the validation function I mentioned in patch 2/5. Or you
could rely on the test from patch 2/5 performing this check, and skip it
here, up to you.

> +
> +		/*
> +		 * Test setting the default format works.
s/Test/Test that/

> +		 */

This holds on a single line.

> +		if (camera_->configureStreams(conf)) {
> +			cout << "Setting valid format test failed" << endl;
> +			return TestFail;
> +		}
> +
> +		/*
> +		 * Test setting the default format fails the camera is not

s/Test/Test that/
s/the camera/if the camera/

> +		 * acquired.
> +		 */
> +		if (camera_->release()) {
> +			cout << "Releasing the camera failed" << endl;
> +			return TestFail;
> +		}
> +
> +		if (!camera_->configureStreams(conf)) {
> +			cout << "Setting valid format on a camera not acquired failed" << endl;
> +			return TestFail;
> +		}
> +
> +		if (camera_->acquire()) {
> +			cout << "Acquiring the camera failed" << endl;
> +			return TestFail;
> +		}

If you moved this before the previous test you could drop the
releast/acquire. On the other hand there's value in testing
release/acquire. If you want to keep them I'd mention that in the
comment above.

> +
> +		/*
> +		 * Test doubling the resolution works.

s/Test/Test that/

You should expand this comment and mention that VIMC supports it,
otherwise it may not be clear why a double resolution is expected to be
valid.

> +		 */
> +		sconf->width *= 2;
> +		sconf->height *= 2;
> +		if (camera_->configureStreams(conf)) {
> +			cout << "Setting modified valid format test failed" << endl;
> +			return TestFail;
> +		}
> +
> +		/*
> +		 * Test Setting an invalid format fails.
> +		 */
> +		sconf->width = 0;
> +		sconf->height = 0;
> +		if (!camera_->configureStreams(conf)) {
> +			cout << "Setting invalid format test failed" << endl;
> +			return TestFail;
> +		}
> +
> +		return TestPass;
> +	}
> +};
> +
> +} /* namespace */
> +
> +TEST_REGISTER(FormatSet);
> diff --git a/test/camera/meson.build b/test/camera/meson.build
> index 4f2ed244a9240512..f5f27c4229ac307f 100644
> --- a/test/camera/meson.build
> +++ b/test/camera/meson.build
> @@ -2,6 +2,7 @@
>  # They are not alphabetically sorted.
>  camera_tests = [
>    [ 'format_default',        'format_default.cpp' ],
> +  [ 'format_set',            'format_set.cpp' ],
>  ]
>  
>  foreach t : camera_tests

-- 
Regards,

Laurent Pinchart