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

[Kieran Bingham]

Hi Laurent,

On 27/07/2021 18:58, Laurent Pinchart wrote:
> 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.

The aim was mostly to be sure I interacted with the CameraManager and
could see that it was working - which was what highlighted the
start/stop/start issue.

If we assume the cameras shouldn't change, a set does make sense, so I
can look at that ... if ...


> 
>> +
>> +		/* 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

Certainly helpful to easily highlight the IPA manager issue.

But I quite like that it shows in the tests what is and isn't
possible/expected, much like you have other tests that show expected
compilation failure, though I guess this shows an expected runtime failure.

It's a shame we can't test that something is expected to fire an assert?


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

Yes, that's worth doing.

I wonder if we should recommend that in the other usages.

For example the application developer guide just stores the pointer.
(I now have a local patch to submit later to fix this)


>> +
>> +		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/

Ok.

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