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

[Laurent Pinchart]

Hello,

On Sat, May 22, 2021 at 12:12:10PM +0200, Niklas Söderlund wrote:
> On 2021-05-21 10:30:52 -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>
> > ---
> > Added in v4
> > 
> >  src/lc-compliance/environment.cpp | 35 +++++++++++++++++++++++++++++++
> >  src/lc-compliance/environment.h   | 31 +++++++++++++++++++++++++++
> >  src/lc-compliance/meson.build     |  1 +
> >  3 files changed, 67 insertions(+)
> >  create mode 100644 src/lc-compliance/environment.cpp
> >  create mode 100644 src/lc-compliance/environment.h
> > 
> > ronment::instance()diff --git a/src/lc-compliance/environment.cpp 
> > b/src/lc-compliance/environment.cpp
> > new file mode 100644
> > index 000000000000..de7fa5bbadd1
> > --- /dev/null
> > +++ b/src/lc-compliance/environment.cpp
> > @@ -0,0 +1,35 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2021, Collabora Ltd.
> > + *
> > + * environment.cpp - Common environment for tests
> > + */
> > +
> > +#include "environment.h"
> > +
> > +Environment *Environment::instance_ = nullptr;
> > +std::shared_ptr<Camera> Environment::camera_ = nullptr;
> > +
> > +bool Environment::create(std::shared_ptr<Camera> camera)
> > +{
> > +	if (instance_)
> > +		return false;
> > +
> > +	camera_ = camera;
> > +	instance_ = new Environment;
> > +	return true;
> > +}
> > +
> > +void Environment::destroy()
> > +{
> > +	if (!camera_)
> > +		return;
> > +
> > +	camera_->release();
> > +	camera_.reset();
> > +}
> > +
> > +Environment *Environment::instance()
> > +{
> > +	return instance_;
> > +}
> 
> Would it make sens to refactor this as Environment::get() while making 
> instance_ a shared pointer and keeping camera_ as a non-static member?

Storing the camera in the enviroment is better than storing it in a
global variable, yes..

> std::shared_ptr<Environment> Environment::instance_ = nullptr;

I don't think the environment itself needs to be a shared_ptr.

> Environment *Environment::get()
> {
>     if (!instance_)
>         instance_ = new Environment;
> 
>     return instance_;

You could also write

	static Environment instance;
	return &instance;

which will construct the environment the first time this function is
called. It will get destroyed after main() returns (but in an
unspecified order relatively to other global variables), so you could
drop the destroy() function.

> }
> 
> void Environment::setup(std::shared_ptr<Camera> camera)
> {
>         camera_ = camera;
> }

Not something that has to be addressed now, but would it be better to
acquire the camera at the beginning of each test, to ensure that no
state is kept between tests ? Only the camera ID would then be stored in
the environment.

> void Environment::destroy()
> {
> 	if (!camera_)
> 		return;
> 
> 	camera_->release();
> 	camera_.reset();
> }
> 
> > diff --git a/src/lc-compliance/environment.h b/src/lc-compliance/environment.h
> > new file mode 100644
> > index 000000000000..f4832d371b6c
> > --- /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 bool create(std::shared_ptr<Camera> camera);
> > +	void destroy();
> > +
> > +	static Environment *instance();
> > +
> > +	std::shared_ptr<Camera> camera() const { return camera_; }
> > +
> > +private:
> > +	Environment() {};
> > +
> > +	static Environment *instance_;
> > +	static std::shared_ptr<Camera> camera_;
> > +};
> > +
> > +#endif /* __LC_COMPLIANCE_ENVIRONMENT_H__ */
> > diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build
> > index a2bfcceb1259..c287017575bd 100644
> > --- a/src/lc-compliance/meson.build
> > +++ b/src/lc-compliance/meson.build
> > @@ -14,6 +14,7 @@ lc_compliance_sources = files([
> >      '../cam/options.cpp',
> >      'main.cpp',
> >      'results.cpp',
> > +    'environment.cpp',

Could you please keep these alphatetically sorted ?

> >      'simple_capture.cpp',
> >      'single_stream.cpp',
> >  ])

-- 
Regards,

Laurent Pinchart