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

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

Hi,

Em 2021-05-23 17:49, Laurent Pinchart escreveu:

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

Yep, that looks better. I'll change it for v5.

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

Yeah, that does make sense. The tests should be as independent as possible. I've
went ahead and made that change for v5 as well. Was simple enough and it's a
good improvement.

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

Sure.

Thanks,
Nícolas

>
> > >      'simple_capture.cpp',
> > >      'single_stream.cpp',
> > >  ])
>
> --
> Regards,
>
> Laurent Pinchart
>
> --
> To unsubscribe, send mail to kernel-unsubscribe at lists.collabora.co.uk.