[libcamera-devel] [PATCH 2/5] test: camera: Add read default format test

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Wed, Mar 06, 2019 at 03:47:52AM +0100, Niklas Söderlund wrote:
> Add a test to verify reading the default format from a camera works.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  test/camera/camera_test.cpp    | 47 ++++++++++++++++++++++
>  test/camera/camera_test.h      | 32 +++++++++++++++
>  test/camera/format_default.cpp | 71 ++++++++++++++++++++++++++++++++++
>  test/camera/meson.build        | 12 ++++++
>  test/meson.build               |  1 +
>  5 files changed, 163 insertions(+)
>  create mode 100644 test/camera/camera_test.cpp
>  create mode 100644 test/camera/camera_test.h
>  create mode 100644 test/camera/format_default.cpp
>  create mode 100644 test/camera/meson.build
> 
> diff --git a/test/camera/camera_test.cpp b/test/camera/camera_test.cpp
> new file mode 100644
> index 0000000000000000..d39a0ed066665946
> --- /dev/null
> +++ b/test/camera/camera_test.cpp
> @@ -0,0 +1,47 @@
> +/* 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;
> +using namespace libcamera;

Alphabetically sorted please.

> +
> +int CameraTest::init()
> +{
> +	cm_ = CameraManager::instance();
> +
> +	if (cm_->start()) {
> +		cout << "Failed to start camera manager" << endl;
> +		return TestFail;
> +	}
> +
> +	camera_ = cm_->get("VIMC Sensor B");
> +	if (!camera_) {
> +		cout << "Can not find VIMC camera, skip." << endl;
> +		return TestSkip;
> +	}
> +
> +	/* Sanity check that camera have streams. */

s/camera have/the camera has/

> +	if (camera_->streams().size() == 0) {

empty() instead of size() == 0 ?

> +		cout << "Camera have no streams, fail." << endl;

s/have no streams/has no stream/

Do we need the "fail" suffix ? Same for the skip suffix in the previous
message.

> +		return TestFail;
> +	}
> +
> +	return TestPass;
> +}
> +
> +void CameraTest::cleanup()
> +{
> +	if (camera_) {
> +		camera_->release();
> +		camera_.reset();

Is the reset needed ?

> +	}
> +
> +	cm_->stop();
> +};
> diff --git a/test/camera/camera_test.h b/test/camera/camera_test.h
> new file mode 100644
> index 0000000000000000..4aadd027675a5dbd
> --- /dev/null
> +++ b/test/camera/camera_test.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * camera_test.h - libcamera camera test base class
> + */
> +#ifndef __LIBCAMERA_CAMERA_TEST_H_
> +#define __LIBCAMERA_CAMERA_TEST_H_

Double underscore at the end ? Same for the #endif down below.

> +
> +#include <libcamera/libcamera.h>
> +
> +#include "test.h"
> +
> +using namespace libcamera;
> +
> +class CameraTest : public Test
> +{
> +public:
> +	CameraTest()
> +		: cm_(nullptr){};

Space before {} and no need for ;

> +
> +protected:
> +	int init();
> +	void cleanup();
> +
> +	std::shared_ptr<Camera> camera_;
> +
> +private:
> +	CameraManager *cm_;
> +};
> +
> +#endif /* __LIBCAMERA_CAMERA_TEST_H_ */
> diff --git a/test/camera/format_default.cpp b/test/camera/format_default.cpp
> new file mode 100644
> index 0000000000000000..11b947f44c39b40a
> --- /dev/null
> +++ b/test/camera/format_default.cpp
> @@ -0,0 +1,71 @@
> +/* 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 FormatDefault : public CameraTest
> +{
> +protected:
> +	int run()
> +	{
> +		std::map<Stream *, StreamConfiguration> conf;
> +		std::set<Stream *> streams = { *camera_->streams().begin() };
> +
> +		/*
> +		 * Test that asking for default format for a valid array of
> +		 * streams returns formats and that the formats are somewhat
> +		 * sane.

It's not just a default format, but a default configuration. Could you
rename that through the whole patch ?

> +		 */
> +		conf = camera_->streamConfiguration(streams);
> +		if (!conf.size()) {

.empty() ?

> +			cout << "Default format for valid streams test failed" << endl;
> +			return TestFail;
> +		}
> +
> +		StreamConfiguration *sconf = &conf.begin()->second;
> +		if (sconf->width == 0 || sconf->height == 0 ||
> +		    sconf->pixelFormat == 0 || sconf->bufferCount == 0) {
> +			cout << "Default format is set test failed" << endl;
> +			return TestFail;
> +		}

Should you also test that conf contains a single element, with the same
stream as the one passed in streams ? You could make that generic for
any number of streams, and extract the code to a separate method moved
to the CameraTest class to make it reusable by multi-streams tests.

> +
> +		/*
> +		 * Test that asking for format for an empty array of streams
> +		 * returns an empty list of configurations.
> +		 */
> +		std::set<Stream *> streams_empty = {};

You can reuse the streams variable declared above.

> +		conf = camera_->streamConfiguration(streams_empty);
> +		if (conf.size()) {

!.empty()

> +			cout << "Default format for empty streams test failed" << endl;
> +			return TestFail;
> +		}
> +
> +		/*
> +		 * Test that asking for format for an array of streams bad streams
> +		 * returns an empty list of configurations.
> +		 */
> +		Stream *stream_bad = *streams.end();

Ideally you should pass a stream pointer for another camera instance.
Otherwise you can just pass a random value. *streams.end() isn't a good
idea, as according to the std::set documentation "This element acts as a
placeholder; attempting to access it results in undefined behavior".

> +		std::set<Stream *> streams_bad = { stream_bad };
> +		conf = camera_->streamConfiguration(streams_bad);
> +		if (conf.size()) {

!.empty()

> +			cout << "Default format for bad streams test failed" << endl;
> +			return TestFail;
> +		}
> +
> +		return TestPass;
> +	}
> +};
> +
> +} /* namespace */
> +
> +TEST_REGISTER(FormatDefault);
> diff --git a/test/camera/meson.build b/test/camera/meson.build
> new file mode 100644
> index 0000000000000000..4f2ed244a9240512
> --- /dev/null
> +++ b/test/camera/meson.build
> @@ -0,0 +1,12 @@
> +# Tests are listed in order of complexity.
> +# They are not alphabetically sorted.
> +camera_tests = [
> +  [ 'format_default',        'format_default.cpp' ],
> +]
> +
> +foreach t : camera_tests
> +  exe = executable(t[0], [t[1], 'camera_test.cpp'],
> +		   link_with : test_libraries,
> +		   include_directories : test_includes_internal)
> +  test(t[0], exe, suite: 'camera', is_parallel: false)
> +endforeach
> diff --git a/test/meson.build b/test/meson.build
> index 5fb16fa6afb62f8d..71a96921697c0e9e 100644
> --- a/test/meson.build
> +++ b/test/meson.build
> @@ -1,5 +1,6 @@
>  subdir('libtest')
>  
> +subdir('camera')
>  subdir('media_device')
>  subdir('pipeline')
>  subdir('v4l2_device')

-- 
Regards,

Laurent Pinchart