[libcamera-devel] [PATCH v6 3/5] lc-compliance: Add Environment singleton

[Nícolas F. R. A. Prado]

Hi Jacopo,

On Wed, Jun 09, 2021 at 07:08:53PM +0200, Jacopo Mondi wrote:
> Hi again,
> 
> On Mon, Jun 07, 2021 at 03:15:04PM -0300, Nícolas F. R. A. Prado wrote:
> > Add a singleton Environment class in order to make the camera available
> > inside all tests. This is needed for the Googletest refactor, otherwise
> > the tests, which are statically declared, won't be able to access the
> > camera.
> >
> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> > Changes in v6:
> > - Thanks to Niklas:
> >   - Made cameraId() return const&
> >
> > Changes in v5:
> > - Thanks to Laurent:
> >   - Stored CameraManager and cameraId instead of Camera in Environment
> > - Thanks to Laurent & Niklas:
> >   - Improved Environment singleton class instantiation and destruction
> >
> > Added in v4
> >
> >  src/lc-compliance/environment.cpp | 20 ++++++++++++++++++++
> >  src/lc-compliance/environment.h   | 31 +++++++++++++++++++++++++++++++
> >  src/lc-compliance/meson.build     |  1 +
> >  3 files changed, 52 insertions(+)
> >  create mode 100644 src/lc-compliance/environment.cpp
> >  create mode 100644 src/lc-compliance/environment.h
> >
> > diff --git a/src/lc-compliance/environment.cpp b/src/lc-compliance/environment.cpp
> > new file mode 100644
> > index 000000000000..fbe75fcc2cfb
> > --- /dev/null
> > +++ b/src/lc-compliance/environment.cpp
> > @@ -0,0 +1,20 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2021, Collabora Ltd.
> > + *
> > + * environment.cpp - Common environment for tests
> > + */
> > +
> > +#include "environment.h"
> > +
> > +Environment *Environment::get()
> > +{
> > +	static Environment instance;
> > +	return &instance;
> > +}
> > +
> > +void Environment::setup(std::shared_ptr<CameraManager> cm, std::string cameraId)
> > +{
> > +	cm_ = cm;
> > +	cameraId_ = cameraId;
> 
> Seeing how this is used later, do we need to store the id and go
> through
>         camera_ = env->cm()->get(env->cameraId());
> ?
> 
> Can't we get the Camera and acquire it in the environment and have
>         std::shared_ptr<Camera> camera() const { return camera_; }
> 
> This way you probably won't need to access the CameraManager from the
> Environment

That was what was done in v4, but Laurent suggested there [1] that we keep the
cameraId instead and acquire the camera for each test in order to ensure that no
state is kept between tests. Even though there's the additional dance to acquire
the camera and an additional pointer to the CameraManager, it seems worth it to
ensure isolation between the tests.

(I've applied your suggestion to make it a unique_ptr in Harness from the
previous email, though)

Thanks,
Nícolas

[1] https://lists.libcamera.org/pipermail/libcamera-devel/2021-May/020659.html

> 
> > +}
> > diff --git a/src/lc-compliance/environment.h b/src/lc-compliance/environment.h
> > new file mode 100644
> > index 000000000000..fd05d445620f
> > --- /dev/null
> > +++ b/src/lc-compliance/environment.h
> > @@ -0,0 +1,31 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2021, Collabora Ltd.
> > + *
> > + * environment.h - Common environment for tests
> > + */
> > +#ifndef __LC_COMPLIANCE_ENVIRONMENT_H__
> > +#define __LC_COMPLIANCE_ENVIRONMENT_H__
> > +
> > +#include <libcamera/libcamera.h>
> > +
> > +using namespace libcamera;
> > +
> > +class Environment
> > +{
> > +public:
> > +	static Environment *get();
> > +
> > +	void setup(std::shared_ptr<CameraManager> cm, std::string cameraId);
> > +
> > +	const std::string &cameraId() const{ return cameraId_; }
> > +	std::shared_ptr<CameraManager> cm() const { return cm_; }
> > +
> > +private:
> > +	Environment() {};
> > +
> > +	std::string cameraId_;
> > +	std::shared_ptr<CameraManager> cm_;
> > +};
> > +
> > +#endif /* __LC_COMPLIANCE_ENVIRONMENT_H__ */
> > diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build
> > index a2bfcceb1259..6dd49085569f 100644
> > --- a/src/lc-compliance/meson.build
> > +++ b/src/lc-compliance/meson.build
> > @@ -12,6 +12,7 @@ lc_compliance_enabled = true
> >  lc_compliance_sources = files([
> >      '../cam/event_loop.cpp',
> >      '../cam/options.cpp',
> > +    'environment.cpp',
> >      'main.cpp',
> >      'results.cpp',
> >      'simple_capture.cpp',
> > --
> > 2.31.1
> >