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

[Laurent Pinchart]

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

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

-- 
Regards,

Laurent Pinchart