[libcamera-devel] [PATCH] test: CameraManager: Add start/stop tests

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Tue, Jul 27, 2021 at 01:07:54PM +0100, Kieran Bingham wrote:
> Validate the CameraManager can start, stop, and restart successfully, as
> well as highlight that we can not construct a second CameraManager
> without hitting assertions.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
> This introduces some basic tests on the lifetime of the CameraManager.
> 
> Integration is optional, but I'm posting to higlight the investigations
> I did on this yesterday.
> 
>  - We can successfully construct, use and destroy a CameraManager
>    and then reconstruct a new one and use that too.
> 
>  - Construction of a CameraManager, with a start()/stop() cycle will
>    not function using the same CameraManager if we try to re-start()
>    the same instance.

That's not expected, and should be fixed.

>  - Construction of two CameraManager instances causes a FATAL assertion
>    though somewhat confusingly, it will face a FATAL assertion on the
>    second IPAManager construction, before the second CameraManager gets
>    to execute its constructor.

This part I'd keep out of the test, as it's an API limitation.

>  test/camera-manager.cpp | 106 ++++++++++++++++++++++++++++++++++++++++
>  test/meson.build        |   1 +
>  2 files changed, 107 insertions(+)
>  create mode 100644 test/camera-manager.cpp
> 
> diff --git a/test/camera-manager.cpp b/test/camera-manager.cpp
> new file mode 100644
> index 000000000000..9e9c494af21c
> --- /dev/null
> +++ b/test/camera-manager.cpp
> @@ -0,0 +1,106 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * libcamera Camera Manager API tests
> + */
> +
> +#include <iostream>
> +#include <memory>
> +
> +#include <libcamera/camera.h>
> +#include <libcamera/camera_manager.h>
> +
> +#include "test.h"
> +
> +using namespace libcamera;
> +using namespace std;
> +
> +class CameraManagerTest : public Test
> +{
> +protected:
> +	int validate()
> +	{
> +		std::shared_ptr<Camera> camera;
> +
> +		if (cm_->start()) {
> +			cerr << "Failed to start camera manager" << endl;
> +			return TestFail;
> +		}
> +
> +		if (cm_->cameras().size() <= 0) {
> +			cerr << "No cameras available" << endl;
> +			return TestFail;
> +		}
> +
> +		camera = cm_->cameras()[0];
> +		if (!camera) {
> +			cerr << "Can not obtain a camera at index 0" << endl;
> +			return TestFail;
> +		}
> +
> +		/* Store the camera id that we get */
> +		cameraId_ = camera->id();
> +
> +		return TestPass;
> +	}
> +
> +	int run()
> +	{
> +		std::string firstCamera;
> +
> +		/* Construct and validate the CameraManager */
> +		cm_ = new CameraManager();
> +		if (validate()) {
> +			cerr << "Failed first construction" << endl;
> +			return TestFail;
> +		}
> +
> +		/* Get camera ID stored by validate */
> +		firstCamera = cameraId_;
> +
> +		/* Now stop everything and reconstruct the CameraManager */
> +		cm_->stop();
> +		delete cm_;
> +
> +		/* Restart and assert we can still get a camera */
> +		cm_ = new CameraManager();
> +		if (validate()) {
> +			cerr << "Failed after re-construction" << endl;
> +			return TestFail;
> +		}
> +
> +		if (firstCamera != cameraId_) {
> +			cerr << "Expected to get the same camera after re-construction" << endl;
> +			return TestFail;
> +		}

There's no guarantee that cameras will be presented in the same order,
we only guarantee camera ID stability. Assuming no camera is plugged or
unplugged while the test is running, which I think is a fair assumption,
but should be documeted in a comment here, you could store all the
camera IDs in a std::set and compare the two sets.

> +
> +		/* Test stop and start (without re-create) */
> +		cm_->stop();
> +
> +		/* validate will call start() */
> +		if (validate()) {
> +			cerr << "Failed after re-starting CameraManager" << endl;
> +			return TestFail;
> +		}
> +
> +		/*
> +		 * Creating a second camera manager is not permitted
> +		 *
> +		 * This will fail with a FATAL in constructing a second IPA
> +		 * Manager, even though we also have a FATAL in the
> +		 * CameraManager construction, but the CameraManager tries
> +		 * to construct an IPA manager, which fails before the
> +		 * CameraManager executes any of it's constructor.
> +		 */
> +		//CameraManager *cm2 = new CameraManager();

Ah, you keep it out :-) We could keep it commented out, but I'm not sure
what value it brings. As an out-of-tree patch, to support work on
addressing the assertion in the IPA manager constructor, it's useful,
but I'm not sure I'd merge it.

You're leaking cm_ here. I'd store it in a std::unique_ptr<>.

> +
> +		return TestPass;
> +	}
> +
> +private:
> +	CameraManager *cm_;
> +	std::string cameraId_;
> +};
> +
> +TEST_REGISTER(CameraManagerTest)
> diff --git a/test/meson.build b/test/meson.build
> index 2c3e76546fbc..cd23c07e1f16 100644
> --- a/test/meson.build
> +++ b/test/meson.build
> @@ -24,6 +24,7 @@ subdir('v4l2_subdevice')
>  subdir('v4l2_videodevice')
>  
>  public_tests = [
> +    ['camera-manager',                  'camera-manager.cpp'],

I would have put this in test/camera/

>      ['geometry',                        'geometry.cpp'],
>      ['public-api',                      'public-api.cpp'],
>      ['signal',                          'signal.cpp'],

-- 
Regards,

Laurent Pinchart