[{"id":17495,"web_url":"https://patchwork.libcamera.org/comment/17495/","msgid":"<20210609164306.sh4vusufr2cystoo@uno.localdomain>","date":"2021-06-09T16:43:06","subject":"Re: [libcamera-devel] [PATCH v6 3/5] lc-compliance: Add Environment\n\tsingleton","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Nicolas,\n\nOn Mon, Jun 07, 2021 at 03:15:04PM -0300, Nícolas F. R. A. Prado wrote:\n> Add a singleton Environment class in order to make the camera available\n> inside all tests. This is needed for the Googletest refactor, otherwise\n> the tests, which are statically declared, won't be able to access the\n> camera.\n>\n> Signed-off-by: Nícolas F. R. A. Prado \n> Reviewed-by: Niklas Söderlund \n> ---\n> Changes in v6:\n> - Thanks to Niklas:\n> - Made cameraId() return const&\n>\n> Changes in v5:\n> - Thanks to Laurent:\n> - Stored CameraManager and cameraId instead of Camera in Environment\n> - Thanks to Laurent & Niklas:\n> - Improved Environment singleton class instantiation and destruction\n>\n> Added in v4\n>\n> src/lc-compliance/environment.cpp | 20 ++++++++++++++++++++\n> src/lc-compliance/environment.h | 31 +++++++++++++++++++++++++++++++\n> src/lc-compliance/meson.build | 1 +\n> 3 files changed, 52 insertions(+)\n> create mode 100644 src/lc-compliance/environment.cpp\n> create mode 100644 src/lc-compliance/environment.h\n>\n> diff --git a/src/lc-compliance/environment.cpp b/src/lc-compliance/environment.cpp\n> new file mode 100644\n> index 000000000000..fbe75fcc2cfb\n> --- /dev/null\n> +++ b/src/lc-compliance/environment.cpp\n> @@ -0,0 +1,20 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2021, Collabora Ltd.\n> + *\n> + * environment.cpp - Common environment for tests\n> + */\n> +\n> +#include \"environment.h\"\n> +\n> +Environment *Environment::get()\n> +{\n> +\tstatic Environment instance;\n> +\treturn &instance;\n> +}\n> +\n> +void Environment::setup(std::shared_ptr cm, std::string cameraId)\n> +{\n> +\tcm_ = cm;\n> +\tcameraId_ = cameraId;\n> +}\n> diff --git a/src/lc-compliance/environment.h b/src/lc-compliance/environment.h\n> new file mode 100644\n> index 000000000000..fd05d445620f\n> --- /dev/null\n> +++ b/src/lc-compliance/environment.h\n> @@ -0,0 +1,31 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2021, Collabora Ltd.\n> + *\n> + * environment.h - Common environment for tests\n> + */\n> +#ifndef __LC_COMPLIANCE_ENVIRONMENT_H__\n> +#define __LC_COMPLIANCE_ENVIRONMENT_H__\n> +\n> +#include \n> +\n> +using namespace libcamera;\n> +\n> +class Environment\n> +{\n> +public:\n> +\tstatic Environment *get();\n> +\n> +\tvoid setup(std::shared_ptr cm, std::string cameraId);\n> +\n> +\tconst std::string &cameraId() const{ return cameraId_; }\n\nspace after const\n\n> +\tstd::shared_ptr cm() const { return cm_; }\n> +\n> +private:\n> +\tEnvironment() {};\n> +\n> +\tstd::string cameraId_;\n> +\tstd::shared_ptr cm_;\n\nDo we need this a shared_ptr ?\nCan't it be instantiated as a unique_ptr in the Harness constructor\nand passed as a raw pointer around ? Harness should be destroyed last\nso it shouldn't be a problem\n\nBoth ways are fine, with the minor above fixed\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> +};\n> +\n> +#endif /* __LC_COMPLIANCE_ENVIRONMENT_H__ */\n> diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build\n> index a2bfcceb1259..6dd49085569f 100644\n> --- a/src/lc-compliance/meson.build\n> +++ b/src/lc-compliance/meson.build\n> @@ -12,6 +12,7 @@ lc_compliance_enabled = true\n> lc_compliance_sources = files([\n> '../cam/event_loop.cpp',\n> '../cam/options.cpp',\n> + 'environment.cpp',\n> 'main.cpp',\n> 'results.cpp',\n> 'simple_capture.cpp',\n> --\n> 2.31.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0FB65BD78E\n\tfor ;\n\tWed, 9 Jun 2021 16:42:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6ED356892F;\n\tWed, 9 Jun 2021 18:42:18 +0200 (CEST)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F19C7602A0\n\tfor ;\n\tWed, 9 Jun 2021 18:42:16 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 1A22620008;\n\tWed, 9 Jun 2021 16:42:15 +0000 (UTC)"],"Date":"Wed, 9 Jun 2021 18:43:06 +0200","From":"Jacopo Mondi ","To":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado ","Message-ID":"<20210609164306.sh4vusufr2cystoo@uno.localdomain>","References":"<20210607181506.51711-1-nfraprado@collabora.com>\n\t<20210607181506.51711-4-nfraprado@collabora.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210607181506.51711-4-nfraprado@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH v6 3/5] lc-compliance: Add Environment\n\tsingleton","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?b?bmRyw6k=?= Almeida ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17496,"web_url":"https://patchwork.libcamera.org/comment/17496/","msgid":"<20210609170853.h7sl6neh6p3hhrbk@uno.localdomain>","date":"2021-06-09T17:08:53","subject":"Re: [libcamera-devel] [PATCH v6 3/5] lc-compliance: Add Environment\n\tsingleton","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi again,\n\nOn Mon, Jun 07, 2021 at 03:15:04PM -0300, Nícolas F. R. A. Prado wrote:\n> Add a singleton Environment class in order to make the camera available\n> inside all tests. This is needed for the Googletest refactor, otherwise\n> the tests, which are statically declared, won't be able to access the\n> camera.\n>\n> Signed-off-by: Nícolas F. R. A. Prado \n> Reviewed-by: Niklas Söderlund \n> ---\n> Changes in v6:\n> - Thanks to Niklas:\n> - Made cameraId() return const&\n>\n> Changes in v5:\n> - Thanks to Laurent:\n> - Stored CameraManager and cameraId instead of Camera in Environment\n> - Thanks to Laurent & Niklas:\n> - Improved Environment singleton class instantiation and destruction\n>\n> Added in v4\n>\n> src/lc-compliance/environment.cpp | 20 ++++++++++++++++++++\n> src/lc-compliance/environment.h | 31 +++++++++++++++++++++++++++++++\n> src/lc-compliance/meson.build | 1 +\n> 3 files changed, 52 insertions(+)\n> create mode 100644 src/lc-compliance/environment.cpp\n> create mode 100644 src/lc-compliance/environment.h\n>\n> diff --git a/src/lc-compliance/environment.cpp b/src/lc-compliance/environment.cpp\n> new file mode 100644\n> index 000000000000..fbe75fcc2cfb\n> --- /dev/null\n> +++ b/src/lc-compliance/environment.cpp\n> @@ -0,0 +1,20 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2021, Collabora Ltd.\n> + *\n> + * environment.cpp - Common environment for tests\n> + */\n> +\n> +#include \"environment.h\"\n> +\n> +Environment *Environment::get()\n> +{\n> +\tstatic Environment instance;\n> +\treturn &instance;\n> +}\n> +\n> +void Environment::setup(std::shared_ptr cm, std::string cameraId)\n> +{\n> +\tcm_ = cm;\n> +\tcameraId_ = cameraId;\n\nSeeing how this is used later, do we need to store the id and go\nthrough\n camera_ = env->cm()->get(env->cameraId());\n?\n\nCan't we get the Camera and acquire it in the environment and have\n std::shared_ptr camera() const { return camera_; }\n\nThis way you probably won't need to access the CameraManager from the\nEnvironment\n\n> +}\n> diff --git a/src/lc-compliance/environment.h b/src/lc-compliance/environment.h\n> new file mode 100644\n> index 000000000000..fd05d445620f\n> --- /dev/null\n> +++ b/src/lc-compliance/environment.h\n> @@ -0,0 +1,31 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2021, Collabora Ltd.\n> + *\n> + * environment.h - Common environment for tests\n> + */\n> +#ifndef __LC_COMPLIANCE_ENVIRONMENT_H__\n> +#define __LC_COMPLIANCE_ENVIRONMENT_H__\n> +\n> +#include \n> +\n> +using namespace libcamera;\n> +\n> +class Environment\n> +{\n> +public:\n> +\tstatic Environment *get();\n> +\n> +\tvoid setup(std::shared_ptr cm, std::string cameraId);\n> +\n> +\tconst std::string &cameraId() const{ return cameraId_; }\n> +\tstd::shared_ptr cm() const { return cm_; }\n> +\n> +private:\n> +\tEnvironment() {};\n> +\n> +\tstd::string cameraId_;\n> +\tstd::shared_ptr cm_;\n> +};\n> +\n> +#endif /* __LC_COMPLIANCE_ENVIRONMENT_H__ */\n> diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build\n> index a2bfcceb1259..6dd49085569f 100644\n> --- a/src/lc-compliance/meson.build\n> +++ b/src/lc-compliance/meson.build\n> @@ -12,6 +12,7 @@ lc_compliance_enabled = true\n> lc_compliance_sources = files([\n> '../cam/event_loop.cpp',\n> '../cam/options.cpp',\n> + 'environment.cpp',\n> 'main.cpp',\n> 'results.cpp',\n> 'simple_capture.cpp',\n> --\n> 2.31.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3431AC320B\n\tfor ;\n\tWed, 9 Jun 2021 17:08:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7B8A368931;\n\tWed, 9 Jun 2021 19:08:06 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 09AB5602A0\n\tfor ;\n\tWed, 9 Jun 2021 19:08:05 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 3F6A0FF80B;\n\tWed, 9 Jun 2021 17:08:03 +0000 (UTC)"],"Date":"Wed, 9 Jun 2021 19:08:53 +0200","From":"Jacopo Mondi ","To":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado ","Message-ID":"<20210609170853.h7sl6neh6p3hhrbk@uno.localdomain>","References":"<20210607181506.51711-1-nfraprado@collabora.com>\n\t<20210607181506.51711-4-nfraprado@collabora.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210607181506.51711-4-nfraprado@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH v6 3/5] lc-compliance: Add Environment\n\tsingleton","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?b?bmRyw6k=?= Almeida ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17499,"web_url":"https://patchwork.libcamera.org/comment/17499/","msgid":"<20210609204527.qghuzgofw3wcxmcf@notapiano>","date":"2021-06-09T20:45:27","subject":"Re: [libcamera-devel] [PATCH v6 3/5] lc-compliance: Add Environment\n\tsingleton","submitter":{"id":84,"url":"https://patchwork.libcamera.org/api/people/84/","name":"Nícolas F. R. A. Prado","email":"nfraprado@collabora.com"},"content":"Hi Jacopo,\n\nOn Wed, Jun 09, 2021 at 07:08:53PM +0200, Jacopo Mondi wrote:\n> Hi again,\n> \n> On Mon, Jun 07, 2021 at 03:15:04PM -0300, Nícolas F. R. A. Prado wrote:\n> > Add a singleton Environment class in order to make the camera available\n> > inside all tests. This is needed for the Googletest refactor, otherwise\n> > the tests, which are statically declared, won't be able to access the\n> > camera.\n> >\n> > Signed-off-by: Nícolas F. R. A. Prado \n> > Reviewed-by: Niklas Söderlund \n> > ---\n> > Changes in v6:\n> > - Thanks to Niklas:\n> > - Made cameraId() return const&\n> >\n> > Changes in v5:\n> > - Thanks to Laurent:\n> > - Stored CameraManager and cameraId instead of Camera in Environment\n> > - Thanks to Laurent & Niklas:\n> > - Improved Environment singleton class instantiation and destruction\n> >\n> > Added in v4\n> >\n> > src/lc-compliance/environment.cpp | 20 ++++++++++++++++++++\n> > src/lc-compliance/environment.h | 31 +++++++++++++++++++++++++++++++\n> > src/lc-compliance/meson.build | 1 +\n> > 3 files changed, 52 insertions(+)\n> > create mode 100644 src/lc-compliance/environment.cpp\n> > create mode 100644 src/lc-compliance/environment.h\n> >\n> > diff --git a/src/lc-compliance/environment.cpp b/src/lc-compliance/environment.cpp\n> > new file mode 100644\n> > index 000000000000..fbe75fcc2cfb\n> > --- /dev/null\n> > +++ b/src/lc-compliance/environment.cpp\n> > @@ -0,0 +1,20 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Collabora Ltd.\n> > + *\n> > + * environment.cpp - Common environment for tests\n> > + */\n> > +\n> > +#include \"environment.h\"\n> > +\n> > +Environment *Environment::get()\n> > +{\n> > +\tstatic Environment instance;\n> > +\treturn &instance;\n> > +}\n> > +\n> > +void Environment::setup(std::shared_ptr cm, std::string cameraId)\n> > +{\n> > +\tcm_ = cm;\n> > +\tcameraId_ = cameraId;\n> \n> Seeing how this is used later, do we need to store the id and go\n> through\n> camera_ = env->cm()->get(env->cameraId());\n> ?\n> \n> Can't we get the Camera and acquire it in the environment and have\n> std::shared_ptr camera() const { return camera_; }\n> \n> This way you probably won't need to access the CameraManager from the\n> Environment\n\nThat was what was done in v4, but Laurent suggested there [1] that we keep the\ncameraId instead and acquire the camera for each test in order to ensure that no\nstate is kept between tests. Even though there's the additional dance to acquire\nthe camera and an additional pointer to the CameraManager, it seems worth it to\nensure isolation between the tests.\n\n(I've applied your suggestion to make it a unique_ptr in Harness from the\nprevious email, though)\n\nThanks,\nNícolas\n\n[1] https://lists.libcamera.org/pipermail/libcamera-devel/2021-May/020659.html\n\n> \n> > +}\n> > diff --git a/src/lc-compliance/environment.h b/src/lc-compliance/environment.h\n> > new file mode 100644\n> > index 000000000000..fd05d445620f\n> > --- /dev/null\n> > +++ b/src/lc-compliance/environment.h\n> > @@ -0,0 +1,31 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Collabora Ltd.\n> > + *\n> > + * environment.h - Common environment for tests\n> > + */\n> > +#ifndef __LC_COMPLIANCE_ENVIRONMENT_H__\n> > +#define __LC_COMPLIANCE_ENVIRONMENT_H__\n> > +\n> > +#include \n> > +\n> > +using namespace libcamera;\n> > +\n> > +class Environment\n> > +{\n> > +public:\n> > +\tstatic Environment *get();\n> > +\n> > +\tvoid setup(std::shared_ptr cm, std::string cameraId);\n> > +\n> > +\tconst std::string &cameraId() const{ return cameraId_; }\n> > +\tstd::shared_ptr cm() const { return cm_; }\n> > +\n> > +private:\n> > +\tEnvironment() {};\n> > +\n> > +\tstd::string cameraId_;\n> > +\tstd::shared_ptr cm_;\n> > +};\n> > +\n> > +#endif /* __LC_COMPLIANCE_ENVIRONMENT_H__ */\n> > diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build\n> > index a2bfcceb1259..6dd49085569f 100644\n> > --- a/src/lc-compliance/meson.build\n> > +++ b/src/lc-compliance/meson.build\n> > @@ -12,6 +12,7 @@ lc_compliance_enabled = true\n> > lc_compliance_sources = files([\n> > '../cam/event_loop.cpp',\n> > '../cam/options.cpp',\n> > + 'environment.cpp',\n> > 'main.cpp',\n> > 'results.cpp',\n> > 'simple_capture.cpp',\n> > --\n> > 2.31.1\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 94B2FBD78E\n\tfor ;\n\tWed, 9 Jun 2021 20:46:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E54BF68928;\n\tWed, 9 Jun 2021 22:46:14 +0200 (CEST)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E47D2602A1\n\tfor ;\n\tWed, 9 Jun 2021 22:46:13 +0200 (CEST)","from [127.0.0.1] (localhost [127.0.0.1])\n\t(Authenticated sender: nfraprado) with ESMTPSA id 6865D1F4374C"],"Date":"Wed, 9 Jun 2021 17:45:27 -0300","From":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado ","To":"Jacopo Mondi ","Message-ID":"<20210609204527.qghuzgofw3wcxmcf@notapiano>","References":"<20210607181506.51711-1-nfraprado@collabora.com>\n\t<20210607181506.51711-4-nfraprado@collabora.com>\n\t<20210609170853.h7sl6neh6p3hhrbk@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210609170853.h7sl6neh6p3hhrbk@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v6 3/5] lc-compliance: Add Environment\n\tsingleton","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?b?bmRyw6k=?= Almeida ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]