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

13129 diff mbox series

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

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


 test/camera-manager.cpp | 106 ++++++++++++++++++++++++++++++++++++++++
 test/meson.build        |   1 +
 2 files changed, 107 insertions(+)
 create mode 100644 test/camera-manager.cpp

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@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'],

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@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'],
>

Hi Kieran,

On Wed, Jul 28, 2021 at 12:17:14PM +0100, Kieran Bingham wrote:
> On 27/07/2021 18:58, Laurent Pinchart wrote:
> > 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@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.

Sure, but the test as done today may fail as there's no guarantee on the
ordering, that part needs to be fixed to avoid false positives.

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

If we really wanted to I think we could, by wrapping the test in another
process, but it will be quite a bit of work.

If you want to keep the above, let's just fix the comment style with
/* ... */ instead of //.

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

Yes I think that's good practice.

> >> +
> >> +		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'],