[{"id":257,"web_url":"https://patchwork.libcamera.org/comment/257/","msgid":"","date":"2019-01-08T14:48:12","subject":"Re: [libcamera-devel] [PATCH v2 04/11] libcamera: camera_manager:\n\tMake the class a singleton","submitter":{"id":6,"url":"https://patchwork.libcamera.org/api/people/6/","name":"Ricky Liang","email":"jcliang@chromium.org"},"content":"Hi Laurent,\n\n\nOn Tue, Jan 8, 2019 at 7:10 AM Laurent Pinchart\n wrote:\n>\n> There can only be a single camera manager instance in the application.\n> Creating it as a singleton helps avoiding mistakes. It also allows the\n> camera manager to be used as a storage of global data, such as the\n> future event dispatcher.\n>\n> Signed-off-by: Laurent Pinchart \n> Reviewed-by: Niklas Söderlund \n> Reviewed-by: Jacopo Mondi \n> ---\n> Changes since v1:\n>\n> - Delete copy constructor and assignment operator\n> - Fix documentation style issue\n> ---\n> include/libcamera/camera_manager.h | 8 ++++++--\n> src/libcamera/camera_manager.cpp | 15 +++++++++++++++\n> test/list.cpp | 7 +------\n> 3 files changed, 22 insertions(+), 8 deletions(-)\n>\n> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h\n> index 2768a5bd2384..e14da0f893b3 100644\n> --- a/include/libcamera/camera_manager.h\n> +++ b/include/libcamera/camera_manager.h\n> @@ -19,15 +19,19 @@ class PipelineHandler;\n> class CameraManager\n> {\n> public:\n> - CameraManager();\n> -\n> int start();\n> void stop();\n>\n> std::vector list() const;\n> Camera *get(const std::string &name);\n>\n> + static CameraManager *instance();\n> +\n> private:\n> + CameraManager();\n> + CameraManager(const CameraManager &) = delete;\n> + void operator=(const CameraManager &) = delete;\n> +\n> DeviceEnumerator *enumerator_;\n> std::vector pipes_;\n\nNot directly related to this patch, but have we considered making\nthese pointers std::unique_ptr? From our experience working in\nChromium we find it informative, easier to follow the code, and less\nerror-prone if an object's ownership can be clearly identified through\nan std::unique_ptr.\n\nFor example, in the current libcamera code base I noticed there's a\npotential leak in DeviceEnumerator::create() if enumerator->init()\nfails:\n\nhttps://git.linuxtv.org/libcamera.git/tree/src/libcamera/device_enumerator.cpp#n137\n\nIf we instead only create and pass std::unique_ptr\naround then we'd avoid leak like this, and people can also look at the\ncode and clearly understands that CameraManager has the ownership of\nenumerator_.\n\nstd::shared_ptr, on the other hand, shall be used only if absolutely\nnecessary, or it can be a recipe for creating ownership spaghetti.\n\n-Ricky\n\n> };\n> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> index 50a805fc665c..1a9d2f38e3b9 100644\n> --- a/src/libcamera/camera_manager.cpp\n> +++ b/src/libcamera/camera_manager.cpp\n> @@ -161,4 +161,19 @@ Camera *CameraManager::get(const std::string &name)\n> return nullptr;\n> }\n>\n> +/**\n> + * \\brief Retrieve the camera manager instance\n> + *\n> + * The CameraManager is a singleton and can't be constructed manually. This\n> + * function shall instead be used to retrieve the single global instance of the\n> + * manager.\n> + *\n> + * \\return The camera manager instance\n> + */\n> +CameraManager *CameraManager::instance()\n> +{\n> + static CameraManager manager;\n> + return &manager;\n> +}\n> +\n> } /* namespace libcamera */\n> diff --git a/test/list.cpp b/test/list.cpp\n> index 39b8a41d1fef..e2026c99c5b8 100644\n> --- a/test/list.cpp\n> +++ b/test/list.cpp\n> @@ -19,10 +19,7 @@ class ListTest : public Test\n> protected:\n> int init()\n> {\n> - cm = new CameraManager();\n> - if (!cm)\n> - return -ENOMEM;\n> -\n> + cm = CameraManager::instance();\n> cm->start();\n>\n> return 0;\n> @@ -43,8 +40,6 @@ protected:\n> void cleanup()\n> {\n> cm->stop();\n> -\n> - delete cm;\n> }\n>\n> private:\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-wr1-x441.google.com (mail-wr1-x441.google.com\n\t[IPv6:2a00:1450:4864:20::441])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9357160B2E\n\tfor ;\n\tTue, 8 Jan 2019 15:48:25 +0100 (CET)","by mail-wr1-x441.google.com with SMTP id x10so4297164wrs.8\n\tfor ;\n\tTue, 08 Jan 2019 06:48:25 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc:content-transfer-encoding;\n\tbh=guVVOH5l7838RVjY9SVzi7SONSwUKgaeARFDPqG7ELw=;\n\tb=TYbiZF2PXOt9x5RPih+/lzIpHFQ6hFcH87m7bKfKb3J2GjjqSyBrXVXlMUIgGOR2vJ\n\tWka7lw3+Rn3uCKwEKwSFbtfewDDTGMQPu94m6B6c2/sVa5/deheMr41wtQ0SO95vU54U\n\tpVn8Pt3+khcgASUmGw5uyKJDLChLPwY51oKQQ=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc:content-transfer-encoding;\n\tbh=guVVOH5l7838RVjY9SVzi7SONSwUKgaeARFDPqG7ELw=;\n\tb=KMf1ettZ+F8h7O4aXflxOxammeyNz/i+mREXOVuLoocvKqKbhaNt9BQwt2gu7Tfv6T\n\tZHDyJK6t75m4+oGxxIc9jTciy7VsXGXx7mYAjkGfDM0GpEMMEoScNbxznUej/Uq3XfbA\n\t1av26XsSu9wr99CveRdQvHXYtaGzjIvVqR9QHxZOkvlOYpQbhSPyPuecLnuz/ewiXpRF\n\tEqfMJ+aPwJiZGUPDmPg5b/Sw+OVo9G+Wq6D5jSkxxHDkkAsu5liUXDzhZB/aSCBbWCAA\n\tC1P8UDTiS1XRyG4iVxtdIk2KgGYtmvniotvuI52AaD5iNJRFYpEskpG5y81zzoLX80hf\n\t3L0Q==","X-Gm-Message-State":"AJcUukcDSlV+TU4HQlxFTdnzgFq+gv9gIhYCwKuV3RC7UdP710ifF5hI\n\t8Zy1VTWGwmQlbQq982XT3lKIkKZHtJ+pWcqdv4wImMo6s6M=","X-Google-Smtp-Source":"ALg8bN4IFSL0MFaGcKlylyiQc6ZWyI9ivLWGYKuA+/XUqD3e3R/JYzQ3v/uZ6IqA+on9a2jBLoYtVf3wDXUnEH/voqs=","X-Received":"by 2002:adf:9521:: with SMTP id 30mr1602233wrs.192.1546958904621;\n\tTue, 08 Jan 2019 06:48:24 -0800 (PST)","MIME-Version":"1.0","References":"<20190107231151.23291-1-laurent.pinchart@ideasonboard.com>\n\t<20190107231151.23291-5-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20190107231151.23291-5-laurent.pinchart@ideasonboard.com>","From":"Ricky Liang ","Date":"Tue, 8 Jan 2019 22:48:12 +0800","Message-ID":"","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH v2 04/11] libcamera: camera_manager:\n\tMake the class a singleton","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 08 Jan 2019 14:48:25 -0000"}},{"id":288,"web_url":"https://patchwork.libcamera.org/comment/288/","msgid":"<1616763.ZV2yHh8qhD@avalon>","date":"2019-01-11T16:03:52","subject":"Re: [libcamera-devel] [PATCH v2 04/11] libcamera: camera_manager:\n\tMake the class a singleton","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Ricky,\n\nOn Tuesday, 8 January 2019 16:48:12 EET Ricky Liang wrote:\n> On Tue, Jan 8, 2019 at 7:10 AM Laurent Pinchart wrote:\n> > There can only be a single camera manager instance in the application.\n> > Creating it as a singleton helps avoiding mistakes. It also allows the\n> > camera manager to be used as a storage of global data, such as the\n> > future event dispatcher.\n> > \n> > Signed-off-by: Laurent Pinchart \n> > Reviewed-by: Niklas Söderlund \n> > Reviewed-by: Jacopo Mondi \n> > ---\n> > Changes since v1:\n> > \n> > - Delete copy constructor and assignment operator\n> > - Fix documentation style issue\n> > ---\n> > \n> > include/libcamera/camera_manager.h | 8 ++++++--\n> > src/libcamera/camera_manager.cpp | 15 +++++++++++++++\n> > test/list.cpp | 7 +------\n> > 3 files changed, 22 insertions(+), 8 deletions(-)\n> > \n> > diff --git a/include/libcamera/camera_manager.h\n> > b/include/libcamera/camera_manager.h index 2768a5bd2384..e14da0f893b3\n> > 100644\n> > --- a/include/libcamera/camera_manager.h\n> > +++ b/include/libcamera/camera_manager.h\n> > @@ -19,15 +19,19 @@ class PipelineHandler;\n> > class CameraManager\n> > {\n> > public:\n> > - CameraManager();\n> > -\n> > int start();\n> > void stop();\n> > \n> > std::vector list() const;\n> > Camera *get(const std::string &name);\n> > \n> > + static CameraManager *instance();\n> > +\n> > private:\n> > + CameraManager();\n> > + CameraManager(const CameraManager &) = delete;\n> > + void operator=(const CameraManager &) = delete;\n> > +\n> > DeviceEnumerator *enumerator_;\n> > std::vector pipes_;\n> \n> Not directly related to this patch, but have we considered making\n> these pointers std::unique_ptr? From our experience working in\n> Chromium we find it informative, easier to follow the code, and less\n> error-prone if an object's ownership can be clearly identified through\n> an std::unique_ptr.\n> \n> For example, in the current libcamera code base I noticed there's a\n> potential leak in DeviceEnumerator::create() if enumerator->init()\n> fails:\n> \n> https://git.linuxtv.org/libcamera.git/tree/src/libcamera/device_enumerator.c\n> pp#n137\n> \n> If we instead only create and pass std::unique_ptr\n> around then we'd avoid leak like this, and people can also look at the\n> code and clearly understands that CameraManager has the ownership of\n> enumerator_.\n\nI agree with you, std::unique_ptr<> would here both provide the advantages of\na scoped pointer with automatic deletion, and make it clear who owns the\nobject. I gave it a try for enumerator_, and here is what I ended up with\n(quote characters added to comment inline).\n\n> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h\n> index 15e7c162032a..6cfcba3c3933 100644\n> --- a/include/libcamera/camera_manager.h\n> +++ b/include/libcamera/camera_manager.h\n> @@ -7,6 +7,7 @@\n> #ifndef __LIBCAMERA_CAMERA_MANAGER_H__\n> #define __LIBCAMERA_CAMERA_MANAGER_H__\n> \n> +#include \n> #include \n> #include \n> \n> @@ -37,7 +38,7 @@ private:\n> \tvoid operator=(const CameraManager &) = delete;\n> \t~CameraManager();\n> \n> -\tDeviceEnumerator *enumerator_;\n> +\tstd::unique_ptr enumerator_;\n> \tstd::vector pipes_;\n\npipes_ left out as they will disappear in the near future, to be replaced with\na vector of reference-counted camera objects.\n\n> \n> \tEventDispatcher *dispatcher_;\n> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> index be327f5d5638..432f2b0a11c5 100644\n> --- a/src/libcamera/camera_manager.cpp\n> +++ b/src/libcamera/camera_manager.cpp\n> @@ -61,7 +61,6 @@ CameraManager::~CameraManager()\n> */\n> int CameraManager::start()\n> {\n> -\n> \tif (enumerator_)\n> \t\treturn -ENODEV;\n> \n> @@ -84,7 +83,7 @@ int CameraManager::start()\n> \t\t * all pipelines it can provide.\n> \t\t */\n> \t\tdo {\n> -\t\t\tpipe = PipelineHandlerFactory::create(handler, enumerator_);\n> +\t\t\tpipe = PipelineHandlerFactory::create(handler, enumerator_.get());\n\nWe already break the unique_ptr<> promise :-) If this acceptable according to\nyou, or is there a better way ?\n\n> \t\t\tif (pipe)\n> \t\t\t\tpipes_.push_back(pipe);\n> \t\t} while (pipe);\n> @@ -114,10 +113,7 @@ void CameraManager::stop()\n> \n> \tpipes_.clear();\n> \n> -\tif (enumerator_)\n> -\t\tdelete enumerator_;\n> -\n> -\tenumerator_ = nullptr;\n> +\tenumerator_.reset(nullptr);\n> }\n> \n> /**\n> diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp\n> index 0d18e75525af..2b03b191fa5d 100644\n> --- a/src/libcamera/device_enumerator.cpp\n> +++ b/src/libcamera/device_enumerator.cpp\n> @@ -14,6 +14,7 @@\n> #include \"device_enumerator.h\"\n> #include \"log.h\"\n> #include \"media_device.h\"\n> +#include \"utils.h\"\n> \n> /**\n> * \\file device_enumerator.h\n> @@ -128,20 +129,20 @@ bool DeviceMatch::match(const MediaDevice *device) const\n> * \\return A pointer to the newly created device enumerator on success, or\n> * nullptr if an error occurs\n> */\n> -DeviceEnumerator *DeviceEnumerator::create()\n> +std::unique_ptr DeviceEnumerator::create()\n> {\n> -\tDeviceEnumerator *enumerator;\n> +\tstd::unique_ptr enumerator;\n> \n> \t/**\n> \t * \\todo Add compile time checks to only try udev enumerator if libudev\n> \t * is available.\n> \t */\n> -\tenumerator = new DeviceEnumeratorUdev();\n> +\tenumerator = std::make_unique(); /* [1] */\n> +\tenumerator = std::unique_ptr(new DeviceEnumeratorUdev()); /* [2] */\n> +\tenumerator.reset(new DeviceEnumeratorUdev()); /* [3] */\n\nHere are three different ways of implementing the same operation.\nstd::make_unique<> doesn't exist in C++11, so I've defined it in utils.c.\nAdding functions to the std namespace could be considered a big of a hack\nthough, so two other implementations are proposed. The second option is\nexplicit, but a bit too long for my taste, while the third option is short but\nuses reset() which sounds a bit strange for an assignment. Do you have any\nadvice ?\n\n> \tif (!enumerator->init())\n> \t\treturn enumerator;\n> \n> -\tdelete enumerator;\n> -\n> \t/*\n> \t * Either udev is not available or udev initialization failed. Fall back\n> \t * on the sysfs enumerator.\n> diff --git a/src/libcamera/include/device_enumerator.h b/src/libcamera/include/device_enumerator.h\n> index 29737da7a225..0afaf88ce1ca 100644\n> --- a/src/libcamera/include/device_enumerator.h\n> +++ b/src/libcamera/include/device_enumerator.h\n> @@ -8,6 +8,7 @@\n> #define __LIBCAMERA_DEVICE_ENUMERATOR_H__\n> \n> #include \n> +#include \n> #include \n> #include \n> \n> @@ -34,7 +35,7 @@ private:\n> class DeviceEnumerator\n> {\n> public:\n> -\tstatic DeviceEnumerator *create();\n> +\tstatic std::unique_ptr create();\n> \n> \tvirtual ~DeviceEnumerator();\n> \n> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h\n> index 3ffa6f4ea591..6df54e758aa4 100644\n> --- a/src/libcamera/include/utils.h\n> +++ b/src/libcamera/include/utils.h\n> @@ -7,6 +7,19 @@\n> #ifndef __LIBCAMERA_UTILS_H__\n> #define __LIBCAMERA_UTILS_H__\n> \n> +#include \n> +\n> #define ARRAY_SIZE(a)\t(sizeof(a) / sizeof(a[0]))\n> \n> +namespace std {\n> +\n> +/* C++11 doesn't provide std::make_unique */\n> +template\n> +std::unique_ptr make_unique(Args&&... args)\n> +{\n> +\treturn std::unique_ptr(new T(std::forward(args)...));\n> +}\n> +\n> +} /* namespace std */\n> +\n> #endif /* __LIBCAMERA_UTILS_H__ */\n\n\n\n> std::shared_ptr, on the other hand, shall be used only if absolutely\n> necessary, or it can be a recipe for creating ownership spaghetti.\n> \n> > };\n\n[snip]","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5BB5260B13\n\tfor ;\n\tFri, 11 Jan 2019 17:02:45 +0100 (CET)","from avalon.localnet (dfj612ybrt5fhg77mgycy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:2e86:4862:ef6a:2804])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B54F653E;\n\tFri, 11 Jan 2019 17:02:40 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1547222560;\n\tbh=qGIYVgMi1WjCnB45rt+wu/eH2CqzIPb13IFPvpEjwpY=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=wc5VG8VCpQHDYVIiftHxiWd//i3dFZ2BZ1c5qKWkwTMwEHyzJNoNcdYtNUYgHmH1v\n\t10iI6tTVpa47Yzv9xKNFRBaSTu0tbqryweP45v0bTFG6OOo8peE+/JvAGZCbPVjQvm\n\tnI00NABFXa1oIOCGVmok/E5WR0QWNQ4z4yGS96cU=","From":"Laurent Pinchart ","To":"Ricky Liang ","Cc":"libcamera-devel@lists.libcamera.org","Date":"Fri, 11 Jan 2019 18:03:52 +0200","Message-ID":"<1616763.ZV2yHh8qhD@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"","References":"<20190107231151.23291-1-laurent.pinchart@ideasonboard.com>\n\t<20190107231151.23291-5-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","Content-Type":"text/plain; charset=\"iso-8859-1\"","Subject":"Re: [libcamera-devel] [PATCH v2 04/11] libcamera: camera_manager:\n\tMake the class a singleton","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 11 Jan 2019 16:02:45 -0000"}},{"id":293,"web_url":"https://patchwork.libcamera.org/comment/293/","msgid":"","date":"2019-01-14T08:19:50","subject":"Re: [libcamera-devel] [PATCH v2 04/11] libcamera: camera_manager:\n\tMake the class a singleton","submitter":{"id":6,"url":"https://patchwork.libcamera.org/api/people/6/","name":"Ricky Liang","email":"jcliang@chromium.org"},"content":"Hi Laurent,\n\nOn Sat, Jan 12, 2019 at 12:02 AM Laurent Pinchart\n wrote:\n>\n> Hi Ricky,\n>\n> On Tuesday, 8 January 2019 16:48:12 EET Ricky Liang wrote:\n> > On Tue, Jan 8, 2019 at 7:10 AM Laurent Pinchart wrote:\n> > > There can only be a single camera manager instance in the application.\n> > > Creating it as a singleton helps avoiding mistakes. It also allows the\n> > > camera manager to be used as a storage of global data, such as the\n> > > future event dispatcher.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > Reviewed-by: Niklas Söderlund \n> > > Reviewed-by: Jacopo Mondi \n> > > ---\n> > > Changes since v1:\n> > >\n> > > - Delete copy constructor and assignment operator\n> > > - Fix documentation style issue\n> > > ---\n> > >\n> > > include/libcamera/camera_manager.h | 8 ++++++--\n> > > src/libcamera/camera_manager.cpp | 15 +++++++++++++++\n> > > test/list.cpp | 7 +------\n> > > 3 files changed, 22 insertions(+), 8 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/camera_manager.h\n> > > b/include/libcamera/camera_manager.h index 2768a5bd2384..e14da0f893b3\n> > > 100644\n> > > --- a/include/libcamera/camera_manager.h\n> > > +++ b/include/libcamera/camera_manager.h\n> > > @@ -19,15 +19,19 @@ class PipelineHandler;\n> > > class CameraManager\n> > > {\n> > > public:\n> > > - CameraManager();\n> > > -\n> > > int start();\n> > > void stop();\n> > >\n> > > std::vector list() const;\n> > > Camera *get(const std::string &name);\n> > >\n> > > + static CameraManager *instance();\n> > > +\n> > > private:\n> > > + CameraManager();\n> > > + CameraManager(const CameraManager &) = delete;\n> > > + void operator=(const CameraManager &) = delete;\n> > > +\n> > > DeviceEnumerator *enumerator_;\n> > > std::vector pipes_;\n> >\n> > Not directly related to this patch, but have we considered making\n> > these pointers std::unique_ptr? From our experience working in\n> > Chromium we find it informative, easier to follow the code, and less\n> > error-prone if an object's ownership can be clearly identified through\n> > an std::unique_ptr.\n> >\n> > For example, in the current libcamera code base I noticed there's a\n> > potential leak in DeviceEnumerator::create() if enumerator->init()\n> > fails:\n> >\n> > https://git.linuxtv.org/libcamera.git/tree/src/libcamera/device_enumerator.c\n> > pp#n137\n> >\n> > If we instead only create and pass std::unique_ptr\n> > around then we'd avoid leak like this, and people can also look at the\n> > code and clearly understands that CameraManager has the ownership of\n> > enumerator_.\n>\n> I agree with you, std::unique_ptr<> would here both provide the advantages of\n> a scoped pointer with automatic deletion, and make it clear who owns the\n> object. I gave it a try for enumerator_, and here is what I ended up with\n> (quote characters added to comment inline).\n>\n> > diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h\n> > index 15e7c162032a..6cfcba3c3933 100644\n> > --- a/include/libcamera/camera_manager.h\n> > +++ b/include/libcamera/camera_manager.h\n> > @@ -7,6 +7,7 @@\n> > #ifndef __LIBCAMERA_CAMERA_MANAGER_H__\n> > #define __LIBCAMERA_CAMERA_MANAGER_H__\n> >\n> > +#include \n> > #include \n> > #include \n> >\n> > @@ -37,7 +38,7 @@ private:\n> > void operator=(const CameraManager &) = delete;\n> > ~CameraManager();\n> >\n> > - DeviceEnumerator *enumerator_;\n> > + std::unique_ptr enumerator_;\n> > std::vector pipes_;\n>\n> pipes_ left out as they will disappear in the near future, to be replaced with\n> a vector of reference-counted camera objects.\n>\n> >\n> > EventDispatcher *dispatcher_;\n> > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> > index be327f5d5638..432f2b0a11c5 100644\n> > --- a/src/libcamera/camera_manager.cpp\n> > +++ b/src/libcamera/camera_manager.cpp\n> > @@ -61,7 +61,6 @@ CameraManager::~CameraManager()\n> > */\n> > int CameraManager::start()\n> > {\n> > -\n> > if (enumerator_)\n> > return -ENODEV;\n> >\n> > @@ -84,7 +83,7 @@ int CameraManager::start()\n> > * all pipelines it can provide.\n> > */\n> > do {\n> > - pipe = PipelineHandlerFactory::create(handler, enumerator_);\n> > + pipe = PipelineHandlerFactory::create(handler, enumerator_.get());\n>\n> We already break the unique_ptr<> promise :-) If this acceptable according to\n> you, or is there a better way ?\n\nIf we're not going to change the internal state of enumerator_, then\nwe can make PipelineHandlerFactory::create take a const reference\ninstead of a pointer. In general we use const reference for\nmethod/function arguments that stay unchanged after calling the\nmethod/function, and pointer for output arguments.\n\nWdyt?\n\n>\n> > if (pipe)\n> > pipes_.push_back(pipe);\n> > } while (pipe);\n> > @@ -114,10 +113,7 @@ void CameraManager::stop()\n> >\n> > pipes_.clear();\n> >\n> > - if (enumerator_)\n> > - delete enumerator_;\n> > -\n> > - enumerator_ = nullptr;\n> > + enumerator_.reset(nullptr);\n> > }\n> >\n> > /**\n> > diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp\n> > index 0d18e75525af..2b03b191fa5d 100644\n> > --- a/src/libcamera/device_enumerator.cpp\n> > +++ b/src/libcamera/device_enumerator.cpp\n> > @@ -14,6 +14,7 @@\n> > #include \"device_enumerator.h\"\n> > #include \"log.h\"\n> > #include \"media_device.h\"\n> > +#include \"utils.h\"\n> >\n> > /**\n> > * \\file device_enumerator.h\n> > @@ -128,20 +129,20 @@ bool DeviceMatch::match(const MediaDevice *device) const\n> > * \\return A pointer to the newly created device enumerator on success, or\n> > * nullptr if an error occurs\n> > */\n> > -DeviceEnumerator *DeviceEnumerator::create()\n> > +std::unique_ptr DeviceEnumerator::create()\n> > {\n> > - DeviceEnumerator *enumerator;\n> > + std::unique_ptr enumerator;\n> >\n> > /**\n> > * \\todo Add compile time checks to only try udev enumerator if libudev\n> > * is available.\n> > */\n> > - enumerator = new DeviceEnumeratorUdev();\n> > + enumerator = std::make_unique(); /* [1] */\n> > + enumerator = std::unique_ptr(new DeviceEnumeratorUdev()); /* [2] */\n> > + enumerator.reset(new DeviceEnumeratorUdev()); /* [3] */\n>\n> Here are three different ways of implementing the same operation.\n> std::make_unique<> doesn't exist in C++11, so I've defined it in utils.c.\n> Adding functions to the std namespace could be considered a big of a hack\n> though, so two other implementations are proposed. The second option is\n> explicit, but a bit too long for my taste, while the third option is short but\n> uses reset() which sounds a bit strange for an assignment. Do you have any\n> advice ?\n\nBefore we have C++11 in Chromium we also had a base::MakeUnique<>\ntemplate in the Chromium libbase handling creation of unique pointers.\nIf we have to stick with C++11 then we might consider doing the same,\nprobably with a utils:: namespace. Hacking the std:: namespace is\nindeed a bad idea an can cause build errors.\n\nSemantically I'd say [2] is more accurate than [3], but I don't really\nhave strong opinions between these two. If we don't want to define our\nown make_unique template then we can use either.\n\nDo we restrict ourselves in C++11 for compatibility reason?\n\n>\n> > if (!enumerator->init())\n> > return enumerator;\n> >\n> > - delete enumerator;\n> > -\n> > /*\n> > * Either udev is not available or udev initialization failed. Fall back\n> > * on the sysfs enumerator.\n> > diff --git a/src/libcamera/include/device_enumerator.h b/src/libcamera/include/device_enumerator.h\n> > index 29737da7a225..0afaf88ce1ca 100644\n> > --- a/src/libcamera/include/device_enumerator.h\n> > +++ b/src/libcamera/include/device_enumerator.h\n> > @@ -8,6 +8,7 @@\n> > #define __LIBCAMERA_DEVICE_ENUMERATOR_H__\n> >\n> > #include \n> > +#include \n> > #include \n> > #include \n> >\n> > @@ -34,7 +35,7 @@ private:\n> > class DeviceEnumerator\n> > {\n> > public:\n> > - static DeviceEnumerator *create();\n> > + static std::unique_ptr create();\n> >\n> > virtual ~DeviceEnumerator();\n> >\n> > diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h\n> > index 3ffa6f4ea591..6df54e758aa4 100644\n> > --- a/src/libcamera/include/utils.h\n> > +++ b/src/libcamera/include/utils.h\n> > @@ -7,6 +7,19 @@\n> > #ifndef __LIBCAMERA_UTILS_H__\n> > #define __LIBCAMERA_UTILS_H__\n> >\n> > +#include \n> > +\n> > #define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))\n> >\n> > +namespace std {\n> > +\n> > +/* C++11 doesn't provide std::make_unique */\n> > +template\n> > +std::unique_ptr make_unique(Args&&... args)\n> > +{\n> > + return std::unique_ptr(new T(std::forward(args)...));\n> > +}\n> > +\n> > +} /* namespace std */\n> > +\n> > #endif /* __LIBCAMERA_UTILS_H__ */\n>\n>\n>\n> > std::shared_ptr, on the other hand, shall be used only if absolutely\n> > necessary, or it can be a recipe for creating ownership spaghetti.\n> >\n> > > };\n>\n> [snip]\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n>","headers":{"Return-Path":"","Received":["from mail-wm1-x343.google.com (mail-wm1-x343.google.com\n\t[IPv6:2a00:1450:4864:20::343])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BF60A60C68\n\tfor ;\n\tMon, 14 Jan 2019 09:20:03 +0100 (CET)","by mail-wm1-x343.google.com with SMTP id y139so7832917wmc.5\n\tfor ;\n\tMon, 14 Jan 2019 00:20:03 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc:content-transfer-encoding;\n\tbh=cFRlrglZ8RaJBnXUG2uE7/Cpyzza7ItuGJMkbKJQf9Q=;\n\tb=au+JEiCS4NChsxdI7s/EFjYW1clcpb2t9Gzic0cQejucvLAgvm3rJILE1bG0Y+AMx6\n\t101PH45bgvt5JZ3JYOtZi8PSNV/PzuzzgjklAPSAkyiC7BkOpijDr0VCNEmxA1OaYKq2\n\tHTE9proRREOpw4ceUxiH7A+eL4vLE5J1HDFlg=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc:content-transfer-encoding;\n\tbh=cFRlrglZ8RaJBnXUG2uE7/Cpyzza7ItuGJMkbKJQf9Q=;\n\tb=i+OxtWk/u1eTEUV3q8KD29RNiGmL0uBis9XRhE/FJpodi8OsTYSeL7d18KbUeTJBlZ\n\tbVsum5JXF2eSh95DCpH8/XiKui6vMI6f9OhyxjDoJ9SsGiJ9gz0OO92ctIxMxuMqr9+l\n\ta5oxQCen16gz7utu/8y1wF/53SFNm3zJ/32Ed2G79XdJ9hv6AQ/skuLz1tOOaccJLJ0s\n\taEMne9ei9HsTw48kfnfLLij7GLbC6zirhtXjYqYMeCYeksL1eTE1WBaAuFsXkby7sy8k\n\t4GM8Sqy2iillKXcUUS7r1hotQ6I68yMAcdlmgt8K83bdQPzq9oig9T9rfzYqITHoCjV/\n\td8Gg==","X-Gm-Message-State":"AJcUukfnR226OE/nJ2+6Rgw7XBu2IWlVlPYF7Pi3oT6WX9MiI3JOCb6J\n\tB3WMJFPl2SnP79fjZRXOq6V5jZ+apZ5siGB1RA9G0Q==","X-Google-Smtp-Source":"ALg8bN5zW55J0cFsq64Xw60uQpLN56BIBFfggNBpCqVzh42QMUQ4Fpkks6eHgcBzMHwjnhyCA4Wu88jB+m3ptcz2pZo=","X-Received":"by 2002:a7b:c7c2:: with SMTP id z2mr10262101wmk.47.1547454002702;\n\tMon, 14 Jan 2019 00:20:02 -0800 (PST)","MIME-Version":"1.0","References":"<20190107231151.23291-1-laurent.pinchart@ideasonboard.com>\n\t<20190107231151.23291-5-laurent.pinchart@ideasonboard.com>\n\t\n\t<1616763.ZV2yHh8qhD@avalon>","In-Reply-To":"<1616763.ZV2yHh8qhD@avalon>","From":"Ricky Liang ","Date":"Mon, 14 Jan 2019 16:19:50 +0800","Message-ID":"","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH v2 04/11] libcamera: camera_manager:\n\tMake the class a singleton","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 14 Jan 2019 08:20:03 -0000"}},{"id":312,"web_url":"https://patchwork.libcamera.org/comment/312/","msgid":"<3011091.8Ud1DvGsir@avalon>","date":"2019-01-15T02:20:45","subject":"Re: [libcamera-devel] [PATCH v2 04/11] libcamera: camera_manager:\n\tMake the class a singleton","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Ricky,\n\nOn Monday, 14 January 2019 10:19:50 EET Ricky Liang wrote:\n> On Sat, Jan 12, 2019 at 12:02 AM Laurent Pinchart wrote:\n> > On Tuesday, 8 January 2019 16:48:12 EET Ricky Liang wrote:\n> >> On Tue, Jan 8, 2019 at 7:10 AM Laurent Pinchart wrote:\n> >>> There can only be a single camera manager instance in the application.\n> >>> Creating it as a singleton helps avoiding mistakes. It also allows the\n> >>> camera manager to be used as a storage of global data, such as the\n> >>> future event dispatcher.\n> >>> \n> >>> Signed-off-by: Laurent Pinchart \n> >>> Reviewed-by: Niklas Söderlund \n> >>> Reviewed-by: Jacopo Mondi \n> >>> ---\n> >>> Changes since v1:\n> >>> \n> >>> - Delete copy constructor and assignment operator\n> >>> - Fix documentation style issue\n> >>> ---\n> >>> \n> >>> include/libcamera/camera_manager.h | 8 ++++++--\n> >>> src/libcamera/camera_manager.cpp | 15 +++++++++++++++\n> >>> test/list.cpp | 7 +------\n> >>> 3 files changed, 22 insertions(+), 8 deletions(-)\n> >>> \n> >>> diff --git a/include/libcamera/camera_manager.h\n> >>> b/include/libcamera/camera_manager.h index 2768a5bd2384..e14da0f893b3\n> >>> 100644\n> >>> --- a/include/libcamera/camera_manager.h\n> >>> +++ b/include/libcamera/camera_manager.h\n> >>> @@ -19,15 +19,19 @@ class PipelineHandler;\n> >>> class CameraManager\n> >>> {\n> >>> public:\n> >>> - CameraManager();\n> >>> -\n> >>> int start();\n> >>> void stop();\n> >>> \n> >>> std::vector list() const;\n> >>> Camera *get(const std::string &name);\n> >>> \n> >>> + static CameraManager *instance();\n> >>> +\n> >>> private:\n> >>> + CameraManager();\n> >>> + CameraManager(const CameraManager &) = delete;\n> >>> + void operator=(const CameraManager &) = delete;\n> >>> +\n> >>> DeviceEnumerator *enumerator_;\n> >>> std::vector pipes_;\n> >> \n> >> Not directly related to this patch, but have we considered making\n> >> these pointers std::unique_ptr? From our experience working in\n> >> Chromium we find it informative, easier to follow the code, and less\n> >> error-prone if an object's ownership can be clearly identified through\n> >> an std::unique_ptr.\n> >> \n> >> For example, in the current libcamera code base I noticed there's a\n> >> potential leak in DeviceEnumerator::create() if enumerator->init()\n> >> fails:\n> >> \n> >> https://git.linuxtv.org/libcamera.git/tree/src/libcamera/device_enumerat\n> >> or.c pp#n137\n> >> \n> >> If we instead only create and pass std::unique_ptr\n> >> around then we'd avoid leak like this, and people can also look at the\n> >> code and clearly understands that CameraManager has the ownership of\n> >> enumerator_.\n> > \n> > I agree with you, std::unique_ptr<> would here both provide the advantages\n> > of a scoped pointer with automatic deletion, and make it clear who owns\n> > the object. I gave it a try for enumerator_, and here is what I ended up\n> > with (quote characters added to comment inline).\n> > \n> >> diff --git a/include/libcamera/camera_manager.h\n> >> b/include/libcamera/camera_manager.h index 15e7c162032a..6cfcba3c3933\n> >> 100644\n> >> --- a/include/libcamera/camera_manager.h\n> >> +++ b/include/libcamera/camera_manager.h\n> >> @@ -7,6 +7,7 @@\n> >> #ifndef __LIBCAMERA_CAMERA_MANAGER_H__\n> >> #define __LIBCAMERA_CAMERA_MANAGER_H__\n> >> \n> >> +#include \n> >> #include \n> >> #include \n> >> \n> >> @@ -37,7 +38,7 @@ private:\n> >> void operator=(const CameraManager &) = delete;\n> >> ~CameraManager();\n> >> \n> >> - DeviceEnumerator *enumerator_;\n> >> + std::unique_ptr enumerator_;\n> >> \n> >> std::vector pipes_;\n> > \n> > pipes_ left out as they will disappear in the near future, to be replaced\n> > with a vector of reference-counted camera objects.\n> > \n> >> EventDispatcher *dispatcher_;\n> >> \n> >> diff --git a/src/libcamera/camera_manager.cpp\n> >> b/src/libcamera/camera_manager.cpp index be327f5d5638..432f2b0a11c5\n> >> 100644\n> >> --- a/src/libcamera/camera_manager.cpp\n> >> +++ b/src/libcamera/camera_manager.cpp\n> >> @@ -61,7 +61,6 @@ CameraManager::~CameraManager()\n> >> */\n> >> int CameraManager::start()\n> >> {\n> >> -\n> >> if (enumerator_)\n> >> return -ENODEV;\n> >> \n> >> @@ -84,7 +83,7 @@ int CameraManager::start()\n> >> * all pipelines it can provide.\n> >> */\n> >> do {\n> >> - pipe = PipelineHandlerFactory::create(handler,\n> >> enumerator_);\n> >> + pipe = PipelineHandlerFactory::create(handler,\n> >> enumerator_.get());\n> > \n> > We already break the unique_ptr<> promise :-) If this acceptable according\n> > to you, or is there a better way ?\n> \n> If we're not going to change the internal state of enumerator_, then\n> we can make PipelineHandlerFactory::create take a const reference\n> instead of a pointer. In general we use const reference for\n> method/function arguments that stay unchanged after calling the\n> method/function, and pointer for output arguments.\n> \n> Wdyt?\n\nWhile we don't have to change the state of the enumerator strictly speaking, \nwe need to change the state of the MediaDevice instances it stores (as \npointers in a vector). We call the following method for that purpose:\n\nMediaDevice *DeviceEnumerator::search(const DeviceMatch &dm) const\n\nwhich I think should not be const as it allows changing the state of an object \nowned by the enumerator.\n\nWhat we really need to convey through the API is that the called function \nPipelineHandlerFactory::create() receive a borrowed references to the \nenumerator and may not access it after it returns. As far as I know there's no \nsimple construct in C++ that is universally understood as expressing this, \nunlike passing a unique_ptr to express that ownership is transferred. We could \npossibly standardize on using references for that purpose (const and non-\nconst), but in some cases we still need pointers when passing nullptr is \nvalid, so it wouldn't be a great solution. What's your opinion on this, could \nwe set in stone the rule that a reference received by a function means that \nthe reference is borrowed for the duration of the function only ?\n\n> >> if (pipe)\n> >> pipes_.push_back(pipe);\n> >> } while (pipe);\n> >> @@ -114,10 +113,7 @@ void CameraManager::stop()\n> >> \n> >> pipes_.clear();\n> >> \n> >> - if (enumerator_)\n> >> - delete enumerator_;\n> >> -\n> >> - enumerator_ = nullptr;\n> >> + enumerator_.reset(nullptr);\n> >> }\n> >> \n> >> /**\n> >> diff --git a/src/libcamera/device_enumerator.cpp\n> >> b/src/libcamera/device_enumerator.cpp index 0d18e75525af..2b03b191fa5d\n> >> 100644\n> >> --- a/src/libcamera/device_enumerator.cpp\n> >> +++ b/src/libcamera/device_enumerator.cpp\n> >> @@ -14,6 +14,7 @@\n> >> #include \"device_enumerator.h\"\n> >> #include \"log.h\"\n> >> #include \"media_device.h\"\n> >> +#include \"utils.h\"\n> >> \n> >> /**\n> >> * \\file device_enumerator.h\n> >> @@ -128,20 +129,20 @@ bool DeviceMatch::match(const MediaDevice *device)\n> >> const\n> >> * \\return A pointer to the newly created device enumerator on success,\n> >> or\n> >> * nullptr if an error occurs\n> >> */\n> >> -DeviceEnumerator *DeviceEnumerator::create()\n> >> +std::unique_ptr DeviceEnumerator::create()\n> >> {\n> >> - DeviceEnumerator *enumerator;\n> >> + std::unique_ptr enumerator;\n> >> \n> >> /**\n> >> * \\todo Add compile time checks to only try udev enumerator if\n> >> libudev\n> >> * is available.\n> >> */\n> >> - enumerator = new DeviceEnumeratorUdev();\n> >> + enumerator = std::make_unique(); /* [1] */\n> >> + enumerator = std::unique_ptr(new\n> >> DeviceEnumeratorUdev()); /* [2] */\n> >> + enumerator.reset(new> DeviceEnumeratorUdev()); /* [3] */\n> > \n> > Here are three different ways of implementing the same operation.\n> > std::make_unique<> doesn't exist in C++11, so I've defined it in utils.c.\n> > Adding functions to the std namespace could be considered a big of a hack\n> > though, so two other implementations are proposed. The second option is\n> > explicit, but a bit too long for my taste, while the third option is short\n> > but uses reset() which sounds a bit strange for an assignment. Do you\n> > have any advice ?\n> \n> Before we have C++11 in Chromium we also had a base::MakeUnique<>\n> template in the Chromium libbase handling creation of unique pointers.\n> If we have to stick with C++11 then we might consider doing the same,\n> probably with a utils:: namespace. Hacking the std:: namespace is\n> indeed a bad idea an can cause build errors.\n> \n> Semantically I'd say [2] is more accurate than [3], but I don't really\n> have strong opinions between these two. If we don't want to define our\n> own make_unique template then we can use either.\n\nI'm tempted to add our own make_unique implementation then, most likely in the \nlibcamera:: or libcamera::utils:: namespace though.\n\n> Do we restrict ourselves in C++11 for compatibility reason?\n\nThat was the rationale, but it could be reconsidered if needed.\n\n> >> if (!enumerator->init())\n> >> return enumerator;\n> >> \n> >> - delete enumerator;\n> >> -\n> >> /*\n> >> * Either udev is not available or udev initialization failed.\n> >> Fall back\n> >> * on the sysfs enumerator.\n> >> diff --git a/src/libcamera/include/device_enumerator.h\n> >> b/src/libcamera/include/device_enumerator.h index\n> >> 29737da7a225..0afaf88ce1ca 100644\n> >> --- a/src/libcamera/include/device_enumerator.h\n> >> +++ b/src/libcamera/include/device_enumerator.h\n> >> @@ -8,6 +8,7 @@\n> >> #define __LIBCAMERA_DEVICE_ENUMERATOR_H__\n> >> \n> >> #include \n> >> +#include \n> >> #include \n> >> #include \n> >> \n> >> @@ -34,7 +35,7 @@ private:\n> >> class DeviceEnumerator\n> >> {\n> >> public:\n> >> - static DeviceEnumerator *create();\n> >> + static std::unique_ptr create();\n> >> virtual ~DeviceEnumerator();\n> >> \n> >> diff --git a/src/libcamera/include/utils.h\n> >> b/src/libcamera/include/utils.h\n> >> index 3ffa6f4ea591..6df54e758aa4 100644\n> >> --- a/src/libcamera/include/utils.h\n> >> +++ b/src/libcamera/include/utils.h\n> >> @@ -7,6 +7,19 @@\n> >> \n> >> #ifndef __LIBCAMERA_UTILS_H__\n> >> #define __LIBCAMERA_UTILS_H__\n> >> \n> >> +#include \n> >> +\n> >> #define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))\n> >> \n> >> +namespace std {\n> >> +\n> >> +/* C++11 doesn't provide std::make_unique */\n> >> +template\n> >> +std::unique_ptr make_unique(Args&&... args)\n> >> +{\n> >> + return std::unique_ptr(new T(std::forward(args)...));\n> >> +}\n> >> +\n> >> +} /* namespace std */\n> >> +\n> >> #endif /* __LIBCAMERA_UTILS_H__ */\n> >> \n> >> std::shared_ptr, on the other hand, shall be used only if absolutely\n> >> necessary, or it can be a recipe for creating ownership spaghetti.\n> >> \n> >>> };\n> > \n> > [snip]","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BFB2C60B2D\n\tfor ;\n\tTue, 15 Jan 2019 03:19:30 +0100 (CET)","from avalon.localnet (dfj612ybrt5fhg77mgycy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:2e86:4862:ef6a:2804])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 03E0B55C;\n\tTue, 15 Jan 2019 03:19:29 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1547518770;\n\tbh=xXGUAXdFnzeDmMCqtpZNjqFsvZNd1VeHc2AmkzGMDdg=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=rEaj8O3bNzAoncE2m03ByjPEjh4SMz0udb8PpTTn6MLa3rhs5hkowZBWhCs7iJk9z\n\twnlo7SvcA66moULNGG0e5GDQ2VSUrpuIF446MEc5bwM/XIoAM2YLXD2nOx7bt4qEQa\n\tQYagJtkxEV5aOuaKhgxLaliH4P6KObTKkPuEpTfM=","From":"Laurent Pinchart ","To":"Ricky Liang ","Cc":"libcamera-devel@lists.libcamera.org","Date":"Tue, 15 Jan 2019 04:20:45 +0200","Message-ID":"<3011091.8Ud1DvGsir@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"","References":"<20190107231151.23291-1-laurent.pinchart@ideasonboard.com>\n\t<1616763.ZV2yHh8qhD@avalon>\n\t","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","Content-Type":"text/plain; charset=\"iso-8859-1\"","Subject":"Re: [libcamera-devel] [PATCH v2 04/11] libcamera: camera_manager:\n\tMake the class a singleton","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 15 Jan 2019 02:19:30 -0000"}},{"id":313,"web_url":"https://patchwork.libcamera.org/comment/313/","msgid":"","date":"2019-01-15T03:16:00","subject":"Re: [libcamera-devel] [PATCH v2 04/11] libcamera: camera_manager:\n\tMake the class a singleton","submitter":{"id":6,"url":"https://patchwork.libcamera.org/api/people/6/","name":"Ricky Liang","email":"jcliang@chromium.org"},"content":"Hi Laurent,\n\nOn Tue, Jan 15, 2019 at 10:19 AM Laurent Pinchart\n wrote:\n>\n> Hi Ricky,\n>\n> On Monday, 14 January 2019 10:19:50 EET Ricky Liang wrote:\n> > On Sat, Jan 12, 2019 at 12:02 AM Laurent Pinchart wrote:\n> > > On Tuesday, 8 January 2019 16:48:12 EET Ricky Liang wrote:\n> > >> On Tue, Jan 8, 2019 at 7:10 AM Laurent Pinchart wrote:\n> > >>> There can only be a single camera manager instance in the application.\n> > >>> Creating it as a singleton helps avoiding mistakes. It also allows the\n> > >>> camera manager to be used as a storage of global data, such as the\n> > >>> future event dispatcher.\n> > >>>\n> > >>> Signed-off-by: Laurent Pinchart \n> > >>> Reviewed-by: Niklas Söderlund \n> > >>> Reviewed-by: Jacopo Mondi \n> > >>> ---\n> > >>> Changes since v1:\n> > >>>\n> > >>> - Delete copy constructor and assignment operator\n> > >>> - Fix documentation style issue\n> > >>> ---\n> > >>>\n> > >>> include/libcamera/camera_manager.h | 8 ++++++--\n> > >>> src/libcamera/camera_manager.cpp | 15 +++++++++++++++\n> > >>> test/list.cpp | 7 +------\n> > >>> 3 files changed, 22 insertions(+), 8 deletions(-)\n> > >>>\n> > >>> diff --git a/include/libcamera/camera_manager.h\n> > >>> b/include/libcamera/camera_manager.h index 2768a5bd2384..e14da0f893b3\n> > >>> 100644\n> > >>> --- a/include/libcamera/camera_manager.h\n> > >>> +++ b/include/libcamera/camera_manager.h\n> > >>> @@ -19,15 +19,19 @@ class PipelineHandler;\n> > >>> class CameraManager\n> > >>> {\n> > >>> public:\n> > >>> - CameraManager();\n> > >>> -\n> > >>> int start();\n> > >>> void stop();\n> > >>>\n> > >>> std::vector list() const;\n> > >>> Camera *get(const std::string &name);\n> > >>>\n> > >>> + static CameraManager *instance();\n> > >>> +\n> > >>> private:\n> > >>> + CameraManager();\n> > >>> + CameraManager(const CameraManager &) = delete;\n> > >>> + void operator=(const CameraManager &) = delete;\n> > >>> +\n> > >>> DeviceEnumerator *enumerator_;\n> > >>> std::vector pipes_;\n> > >>\n> > >> Not directly related to this patch, but have we considered making\n> > >> these pointers std::unique_ptr? From our experience working in\n> > >> Chromium we find it informative, easier to follow the code, and less\n> > >> error-prone if an object's ownership can be clearly identified through\n> > >> an std::unique_ptr.\n> > >>\n> > >> For example, in the current libcamera code base I noticed there's a\n> > >> potential leak in DeviceEnumerator::create() if enumerator->init()\n> > >> fails:\n> > >>\n> > >> https://git.linuxtv.org/libcamera.git/tree/src/libcamera/device_enumerat\n> > >> or.c pp#n137\n> > >>\n> > >> If we instead only create and pass std::unique_ptr\n> > >> around then we'd avoid leak like this, and people can also look at the\n> > >> code and clearly understands that CameraManager has the ownership of\n> > >> enumerator_.\n> > >\n> > > I agree with you, std::unique_ptr<> would here both provide the advantages\n> > > of a scoped pointer with automatic deletion, and make it clear who owns\n> > > the object. I gave it a try for enumerator_, and here is what I ended up\n> > > with (quote characters added to comment inline).\n> > >\n> > >> diff --git a/include/libcamera/camera_manager.h\n> > >> b/include/libcamera/camera_manager.h index 15e7c162032a..6cfcba3c3933\n> > >> 100644\n> > >> --- a/include/libcamera/camera_manager.h\n> > >> +++ b/include/libcamera/camera_manager.h\n> > >> @@ -7,6 +7,7 @@\n> > >> #ifndef __LIBCAMERA_CAMERA_MANAGER_H__\n> > >> #define __LIBCAMERA_CAMERA_MANAGER_H__\n> > >>\n> > >> +#include \n> > >> #include \n> > >> #include \n> > >>\n> > >> @@ -37,7 +38,7 @@ private:\n> > >> void operator=(const CameraManager &) = delete;\n> > >> ~CameraManager();\n> > >>\n> > >> - DeviceEnumerator *enumerator_;\n> > >> + std::unique_ptr enumerator_;\n> > >>\n> > >> std::vector pipes_;\n> > >\n> > > pipes_ left out as they will disappear in the near future, to be replaced\n> > > with a vector of reference-counted camera objects.\n> > >\n> > >> EventDispatcher *dispatcher_;\n> > >>\n> > >> diff --git a/src/libcamera/camera_manager.cpp\n> > >> b/src/libcamera/camera_manager.cpp index be327f5d5638..432f2b0a11c5\n> > >> 100644\n> > >> --- a/src/libcamera/camera_manager.cpp\n> > >> +++ b/src/libcamera/camera_manager.cpp\n> > >> @@ -61,7 +61,6 @@ CameraManager::~CameraManager()\n> > >> */\n> > >> int CameraManager::start()\n> > >> {\n> > >> -\n> > >> if (enumerator_)\n> > >> return -ENODEV;\n> > >>\n> > >> @@ -84,7 +83,7 @@ int CameraManager::start()\n> > >> * all pipelines it can provide.\n> > >> */\n> > >> do {\n> > >> - pipe = PipelineHandlerFactory::create(handler,\n> > >> enumerator_);\n> > >> + pipe = PipelineHandlerFactory::create(handler,\n> > >> enumerator_.get());\n> > >\n> > > We already break the unique_ptr<> promise :-) If this acceptable according\n> > > to you, or is there a better way ?\n> >\n> > If we're not going to change the internal state of enumerator_, then\n> > we can make PipelineHandlerFactory::create take a const reference\n> > instead of a pointer. In general we use const reference for\n> > method/function arguments that stay unchanged after calling the\n> > method/function, and pointer for output arguments.\n> >\n> > Wdyt?\n>\n> While we don't have to change the state of the enumerator strictly speaking,\n> we need to change the state of the MediaDevice instances it stores (as\n> pointers in a vector). We call the following method for that purpose:\n>\n> MediaDevice *DeviceEnumerator::search(const DeviceMatch &dm) const\n>\n> which I think should not be const as it allows changing the state of an object\n> owned by the enumerator.\n\nI see. Thanks for the detail!\n\n>\n> What we really need to convey through the API is that the called function\n> PipelineHandlerFactory::create() receive a borrowed references to the\n> enumerator and may not access it after it returns. As far as I know there's no\n> simple construct in C++ that is universally understood as expressing this,\n> unlike passing a unique_ptr to express that ownership is transferred. We could\n> possibly standardize on using references for that purpose (const and non-\n> const), but in some cases we still need pointers when passing nullptr is\n> valid, so it wouldn't be a great solution. What's your opinion on this, could\n> we set in stone the rule that a reference received by a function means that\n> the reference is borrowed for the duration of the function only ?\n\nPersonally I'm not a big fan of non-const reference. I find it easily\nconfused when reviewing the code as it has value syntax with pointer\nsemantics. Having raw pointer arguments and/or return values is fine\nand often necessary.\n\nI'd suggest we use std::unique_ptr<> whenever possible to establish\nclear object ownership, and const reference whenever we don't plan to\nalter the state of the object. We then can put most of our attention\nto the remaining raw pointers.\n\nAs we're following the Google C++ coding style I'd suggest we follow:\n\nhttps://google.github.io/styleguide/cppguide.html#Reference_Argument\n\n>\n> > >> if (pipe)\n> > >> pipes_.push_back(pipe);\n> > >> } while (pipe);\n> > >> @@ -114,10 +113,7 @@ void CameraManager::stop()\n> > >>\n> > >> pipes_.clear();\n> > >>\n> > >> - if (enumerator_)\n> > >> - delete enumerator_;\n> > >> -\n> > >> - enumerator_ = nullptr;\n> > >> + enumerator_.reset(nullptr);\n> > >> }\n> > >>\n> > >> /**\n> > >> diff --git a/src/libcamera/device_enumerator.cpp\n> > >> b/src/libcamera/device_enumerator.cpp index 0d18e75525af..2b03b191fa5d\n> > >> 100644\n> > >> --- a/src/libcamera/device_enumerator.cpp\n> > >> +++ b/src/libcamera/device_enumerator.cpp\n> > >> @@ -14,6 +14,7 @@\n> > >> #include \"device_enumerator.h\"\n> > >> #include \"log.h\"\n> > >> #include \"media_device.h\"\n> > >> +#include \"utils.h\"\n> > >>\n> > >> /**\n> > >> * \\file device_enumerator.h\n> > >> @@ -128,20 +129,20 @@ bool DeviceMatch::match(const MediaDevice *device)\n> > >> const\n> > >> * \\return A pointer to the newly created device enumerator on success,\n> > >> or\n> > >> * nullptr if an error occurs\n> > >> */\n> > >> -DeviceEnumerator *DeviceEnumerator::create()\n> > >> +std::unique_ptr DeviceEnumerator::create()\n> > >> {\n> > >> - DeviceEnumerator *enumerator;\n> > >> + std::unique_ptr enumerator;\n> > >>\n> > >> /**\n> > >> * \\todo Add compile time checks to only try udev enumerator if\n> > >> libudev\n> > >> * is available.\n> > >> */\n> > >> - enumerator = new DeviceEnumeratorUdev();\n> > >> + enumerator = std::make_unique(); /* [1] */\n> > >> + enumerator = std::unique_ptr(new\n> > >> DeviceEnumeratorUdev()); /* [2] */\n> > >> + enumerator.reset(new> DeviceEnumeratorUdev()); /* [3] */\n> > >\n> > > Here are three different ways of implementing the same operation.\n> > > std::make_unique<> doesn't exist in C++11, so I've defined it in utils.c.\n> > > Adding functions to the std namespace could be considered a big of a hack\n> > > though, so two other implementations are proposed. The second option is\n> > > explicit, but a bit too long for my taste, while the third option is short\n> > > but uses reset() which sounds a bit strange for an assignment. Do you\n> > > have any advice ?\n> >\n> > Before we have C++11 in Chromium we also had a base::MakeUnique<>\n> > template in the Chromium libbase handling creation of unique pointers.\n> > If we have to stick with C++11 then we might consider doing the same,\n> > probably with a utils:: namespace. Hacking the std:: namespace is\n> > indeed a bad idea an can cause build errors.\n> >\n> > Semantically I'd say [2] is more accurate than [3], but I don't really\n> > have strong opinions between these two. If we don't want to define our\n> > own make_unique template then we can use either.\n>\n> I'm tempted to add our own make_unique implementation then, most likely in the\n> libcamera:: or libcamera::utils:: namespace though.\n\nSounds good!\n\n>\n> > Do we restrict ourselves in C++11 for compatibility reason?\n>\n> That was the rationale, but it could be reconsidered if needed.\n\nI suppose C++11 shall be sufficient. We can indeed reconsider if we\nhave strong use cases for newer standards one day.\n\n>\n> > >> if (!enumerator->init())\n> > >> return enumerator;\n> > >>\n> > >> - delete enumerator;\n> > >> -\n> > >> /*\n> > >> * Either udev is not available or udev initialization failed.\n> > >> Fall back\n> > >> * on the sysfs enumerator.\n> > >> diff --git a/src/libcamera/include/device_enumerator.h\n> > >> b/src/libcamera/include/device_enumerator.h index\n> > >> 29737da7a225..0afaf88ce1ca 100644\n> > >> --- a/src/libcamera/include/device_enumerator.h\n> > >> +++ b/src/libcamera/include/device_enumerator.h\n> > >> @@ -8,6 +8,7 @@\n> > >> #define __LIBCAMERA_DEVICE_ENUMERATOR_H__\n> > >>\n> > >> #include \n> > >> +#include \n> > >> #include \n> > >> #include \n> > >>\n> > >> @@ -34,7 +35,7 @@ private:\n> > >> class DeviceEnumerator\n> > >> {\n> > >> public:\n> > >> - static DeviceEnumerator *create();\n> > >> + static std::unique_ptr create();\n> > >> virtual ~DeviceEnumerator();\n> > >>\n> > >> diff --git a/src/libcamera/include/utils.h\n> > >> b/src/libcamera/include/utils.h\n> > >> index 3ffa6f4ea591..6df54e758aa4 100644\n> > >> --- a/src/libcamera/include/utils.h\n> > >> +++ b/src/libcamera/include/utils.h\n> > >> @@ -7,6 +7,19 @@\n> > >>\n> > >> #ifndef __LIBCAMERA_UTILS_H__\n> > >> #define __LIBCAMERA_UTILS_H__\n> > >>\n> > >> +#include \n> > >> +\n> > >> #define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))\n> > >>\n> > >> +namespace std {\n> > >> +\n> > >> +/* C++11 doesn't provide std::make_unique */\n> > >> +template\n> > >> +std::unique_ptr make_unique(Args&&... args)\n> > >> +{\n> > >> + return std::unique_ptr(new T(std::forward(args)...));\n> > >> +}\n> > >> +\n> > >> +} /* namespace std */\n> > >> +\n> > >> #endif /* __LIBCAMERA_UTILS_H__ */\n> > >>\n> > >> std::shared_ptr, on the other hand, shall be used only if absolutely\n> > >> necessary, or it can be a recipe for creating ownership spaghetti.\n> > >>\n> > >>> };\n> > >\n> > > [snip]\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n>\n>","headers":{"Return-Path":"","Received":["from mail-wm1-x344.google.com (mail-wm1-x344.google.com\n\t[IPv6:2a00:1450:4864:20::344])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F2BE960B2D\n\tfor ;\n\tTue, 15 Jan 2019 04:16:13 +0100 (CET)","by mail-wm1-x344.google.com with SMTP id n190so1797433wmd.0\n\tfor ;\n\tMon, 14 Jan 2019 19:16:13 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc:content-transfer-encoding;\n\tbh=K/o1eeaPCeCOyY4JwYcWK7e/+pxiSSHtkvPGBg239fI=;\n\tb=CWrBn7dG1rxJ40F2HbLeXtypkRU/lJgeNyLAIOy0HLhXbSvf/KTh1Jzoa1PlaNapQ9\n\tjORV6fO1hObYvApZ+PEhyRwrOgWf5lXmIz/0/EWKaDrKEc65lJzQ1+h18HT5vc5jDqH0\n\t2V76mSOQCeSoFbnP/KVDmhlGLTMqrK3544B9g=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc:content-transfer-encoding;\n\tbh=K/o1eeaPCeCOyY4JwYcWK7e/+pxiSSHtkvPGBg239fI=;\n\tb=DTqhKK8Wt/qu3YOB+lpICgPrDpF0i1kwG3gKpbjCzz6vWXVvNo276KqBo5iVpReQm2\n\t/YMIPfc09KAEaVy4EUjhgAiDD8V7w+SD76LevLN2EsbQo0deVUvvLk6pph4dfFeMca+o\n\t2SvEl34/ZKhL21VXGB2/ewq9YwsNUhDfhaNQk4TKPtut+hQCu/L+CfgzEmiRtoeAjbhA\n\twwl4UU5o0SIVMTbxOI+qTTyIQY8bZr2aNVVKYNWqbfMmbK7k5QY4NqkTV3FkxcrAWAF+\n\tw0mvDSYp/PC0cbThrqGMGroFlf8kts2hI0IejMCxKLTE0KIv9tB36EDfKxescUhj3kCZ\n\tUPdw==","X-Gm-Message-State":"AJcUukf4CssPJwHMV4bQIwP8kMsA+2lWeHPEboznnZtw1D09GJPVyFqp\n\tykHi+8BJ96wKAUQDX96xPUfD7te86k0NzYsilvnraA==","X-Google-Smtp-Source":"ALg8bN5itIoNSELRdaYa/f1U9HEE7A+FfqJ2/NnzT+PE8lLdQQgjZfKzQon4D2lrWsWn4yH3GjEEZf12VnhkERXqd7g=","X-Received":"by 2002:a7b:c7c2:: with SMTP id z2mr1413015wmk.47.1547522172852; \n\tMon, 14 Jan 2019 19:16:12 -0800 (PST)","MIME-Version":"1.0","References":"<20190107231151.23291-1-laurent.pinchart@ideasonboard.com>\n\t<1616763.ZV2yHh8qhD@avalon>\n\t\n\t<3011091.8Ud1DvGsir@avalon>","In-Reply-To":"<3011091.8Ud1DvGsir@avalon>","From":"Ricky Liang ","Date":"Tue, 15 Jan 2019 11:16:00 +0800","Message-ID":"","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH v2 04/11] libcamera: camera_manager:\n\tMake the class a singleton","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 15 Jan 2019 03:16:14 -0000"}},{"id":317,"web_url":"https://patchwork.libcamera.org/comment/317/","msgid":"","date":"2019-01-15T14:26:14","subject":"Re: [libcamera-devel] [PATCH v2 04/11] libcamera: camera_manager:\n\tMake the class a singleton","submitter":{"id":6,"url":"https://patchwork.libcamera.org/api/people/6/","name":"Ricky Liang","email":"jcliang@chromium.org"},"content":"+ Shik, who has some ideas about the C++ version to use and useful\nthird-party C++ libraries\n\nOn Tue, Jan 15, 2019 at 11:16 AM Ricky Liang wrote:\n>\n> Hi Laurent,\n>\n> On Tue, Jan 15, 2019 at 10:19 AM Laurent Pinchart\n> wrote:\n> >\n> > Hi Ricky,\n> >\n> > On Monday, 14 January 2019 10:19:50 EET Ricky Liang wrote:\n> > > On Sat, Jan 12, 2019 at 12:02 AM Laurent Pinchart wrote:\n> > > > On Tuesday, 8 January 2019 16:48:12 EET Ricky Liang wrote:\n> > > >> On Tue, Jan 8, 2019 at 7:10 AM Laurent Pinchart wrote:\n> > > >>> There can only be a single camera manager instance in the application.\n> > > >>> Creating it as a singleton helps avoiding mistakes. It also allows the\n> > > >>> camera manager to be used as a storage of global data, such as the\n> > > >>> future event dispatcher.\n> > > >>>\n> > > >>> Signed-off-by: Laurent Pinchart \n> > > >>> Reviewed-by: Niklas Söderlund \n> > > >>> Reviewed-by: Jacopo Mondi \n> > > >>> ---\n> > > >>> Changes since v1:\n> > > >>>\n> > > >>> - Delete copy constructor and assignment operator\n> > > >>> - Fix documentation style issue\n> > > >>> ---\n> > > >>>\n> > > >>> include/libcamera/camera_manager.h | 8 ++++++--\n> > > >>> src/libcamera/camera_manager.cpp | 15 +++++++++++++++\n> > > >>> test/list.cpp | 7 +------\n> > > >>> 3 files changed, 22 insertions(+), 8 deletions(-)\n> > > >>>\n> > > >>> diff --git a/include/libcamera/camera_manager.h\n> > > >>> b/include/libcamera/camera_manager.h index 2768a5bd2384..e14da0f893b3\n> > > >>> 100644\n> > > >>> --- a/include/libcamera/camera_manager.h\n> > > >>> +++ b/include/libcamera/camera_manager.h\n> > > >>> @@ -19,15 +19,19 @@ class PipelineHandler;\n> > > >>> class CameraManager\n> > > >>> {\n> > > >>> public:\n> > > >>> - CameraManager();\n> > > >>> -\n> > > >>> int start();\n> > > >>> void stop();\n> > > >>>\n> > > >>> std::vector list() const;\n> > > >>> Camera *get(const std::string &name);\n> > > >>>\n> > > >>> + static CameraManager *instance();\n> > > >>> +\n> > > >>> private:\n> > > >>> + CameraManager();\n> > > >>> + CameraManager(const CameraManager &) = delete;\n> > > >>> + void operator=(const CameraManager &) = delete;\n> > > >>> +\n> > > >>> DeviceEnumerator *enumerator_;\n> > > >>> std::vector pipes_;\n> > > >>\n> > > >> Not directly related to this patch, but have we considered making\n> > > >> these pointers std::unique_ptr? From our experience working in\n> > > >> Chromium we find it informative, easier to follow the code, and less\n> > > >> error-prone if an object's ownership can be clearly identified through\n> > > >> an std::unique_ptr.\n> > > >>\n> > > >> For example, in the current libcamera code base I noticed there's a\n> > > >> potential leak in DeviceEnumerator::create() if enumerator->init()\n> > > >> fails:\n> > > >>\n> > > >> https://git.linuxtv.org/libcamera.git/tree/src/libcamera/device_enumerat\n> > > >> or.c pp#n137\n> > > >>\n> > > >> If we instead only create and pass std::unique_ptr\n> > > >> around then we'd avoid leak like this, and people can also look at the\n> > > >> code and clearly understands that CameraManager has the ownership of\n> > > >> enumerator_.\n> > > >\n> > > > I agree with you, std::unique_ptr<> would here both provide the advantages\n> > > > of a scoped pointer with automatic deletion, and make it clear who owns\n> > > > the object. I gave it a try for enumerator_, and here is what I ended up\n> > > > with (quote characters added to comment inline).\n> > > >\n> > > >> diff --git a/include/libcamera/camera_manager.h\n> > > >> b/include/libcamera/camera_manager.h index 15e7c162032a..6cfcba3c3933\n> > > >> 100644\n> > > >> --- a/include/libcamera/camera_manager.h\n> > > >> +++ b/include/libcamera/camera_manager.h\n> > > >> @@ -7,6 +7,7 @@\n> > > >> #ifndef __LIBCAMERA_CAMERA_MANAGER_H__\n> > > >> #define __LIBCAMERA_CAMERA_MANAGER_H__\n> > > >>\n> > > >> +#include \n> > > >> #include \n> > > >> #include \n> > > >>\n> > > >> @@ -37,7 +38,7 @@ private:\n> > > >> void operator=(const CameraManager &) = delete;\n> > > >> ~CameraManager();\n> > > >>\n> > > >> - DeviceEnumerator *enumerator_;\n> > > >> + std::unique_ptr enumerator_;\n> > > >>\n> > > >> std::vector pipes_;\n> > > >\n> > > > pipes_ left out as they will disappear in the near future, to be replaced\n> > > > with a vector of reference-counted camera objects.\n> > > >\n> > > >> EventDispatcher *dispatcher_;\n> > > >>\n> > > >> diff --git a/src/libcamera/camera_manager.cpp\n> > > >> b/src/libcamera/camera_manager.cpp index be327f5d5638..432f2b0a11c5\n> > > >> 100644\n> > > >> --- a/src/libcamera/camera_manager.cpp\n> > > >> +++ b/src/libcamera/camera_manager.cpp\n> > > >> @@ -61,7 +61,6 @@ CameraManager::~CameraManager()\n> > > >> */\n> > > >> int CameraManager::start()\n> > > >> {\n> > > >> -\n> > > >> if (enumerator_)\n> > > >> return -ENODEV;\n> > > >>\n> > > >> @@ -84,7 +83,7 @@ int CameraManager::start()\n> > > >> * all pipelines it can provide.\n> > > >> */\n> > > >> do {\n> > > >> - pipe = PipelineHandlerFactory::create(handler,\n> > > >> enumerator_);\n> > > >> + pipe = PipelineHandlerFactory::create(handler,\n> > > >> enumerator_.get());\n> > > >\n> > > > We already break the unique_ptr<> promise :-) If this acceptable according\n> > > > to you, or is there a better way ?\n> > >\n> > > If we're not going to change the internal state of enumerator_, then\n> > > we can make PipelineHandlerFactory::create take a const reference\n> > > instead of a pointer. In general we use const reference for\n> > > method/function arguments that stay unchanged after calling the\n> > > method/function, and pointer for output arguments.\n> > >\n> > > Wdyt?\n> >\n> > While we don't have to change the state of the enumerator strictly speaking,\n> > we need to change the state of the MediaDevice instances it stores (as\n> > pointers in a vector). We call the following method for that purpose:\n> >\n> > MediaDevice *DeviceEnumerator::search(const DeviceMatch &dm) const\n> >\n> > which I think should not be const as it allows changing the state of an object\n> > owned by the enumerator.\n>\n> I see. Thanks for the detail!\n>\n> >\n> > What we really need to convey through the API is that the called function\n> > PipelineHandlerFactory::create() receive a borrowed references to the\n> > enumerator and may not access it after it returns. As far as I know there's no\n> > simple construct in C++ that is universally understood as expressing this,\n> > unlike passing a unique_ptr to express that ownership is transferred. We could\n> > possibly standardize on using references for that purpose (const and non-\n> > const), but in some cases we still need pointers when passing nullptr is\n> > valid, so it wouldn't be a great solution. What's your opinion on this, could\n> > we set in stone the rule that a reference received by a function means that\n> > the reference is borrowed for the duration of the function only ?\n>\n> Personally I'm not a big fan of non-const reference. I find it easily\n> confused when reviewing the code as it has value syntax with pointer\n> semantics. Having raw pointer arguments and/or return values is fine\n> and often necessary.\n>\n> I'd suggest we use std::unique_ptr<> whenever possible to establish\n> clear object ownership, and const reference whenever we don't plan to\n> alter the state of the object. We then can put most of our attention\n> to the remaining raw pointers.\n>\n> As we're following the Google C++ coding style I'd suggest we follow:\n>\n> https://google.github.io/styleguide/cppguide.html#Reference_Argument\n>\n> >\n> > > >> if (pipe)\n> > > >> pipes_.push_back(pipe);\n> > > >> } while (pipe);\n> > > >> @@ -114,10 +113,7 @@ void CameraManager::stop()\n> > > >>\n> > > >> pipes_.clear();\n> > > >>\n> > > >> - if (enumerator_)\n> > > >> - delete enumerator_;\n> > > >> -\n> > > >> - enumerator_ = nullptr;\n> > > >> + enumerator_.reset(nullptr);\n> > > >> }\n> > > >>\n> > > >> /**\n> > > >> diff --git a/src/libcamera/device_enumerator.cpp\n> > > >> b/src/libcamera/device_enumerator.cpp index 0d18e75525af..2b03b191fa5d\n> > > >> 100644\n> > > >> --- a/src/libcamera/device_enumerator.cpp\n> > > >> +++ b/src/libcamera/device_enumerator.cpp\n> > > >> @@ -14,6 +14,7 @@\n> > > >> #include \"device_enumerator.h\"\n> > > >> #include \"log.h\"\n> > > >> #include \"media_device.h\"\n> > > >> +#include \"utils.h\"\n> > > >>\n> > > >> /**\n> > > >> * \\file device_enumerator.h\n> > > >> @@ -128,20 +129,20 @@ bool DeviceMatch::match(const MediaDevice *device)\n> > > >> const\n> > > >> * \\return A pointer to the newly created device enumerator on success,\n> > > >> or\n> > > >> * nullptr if an error occurs\n> > > >> */\n> > > >> -DeviceEnumerator *DeviceEnumerator::create()\n> > > >> +std::unique_ptr DeviceEnumerator::create()\n> > > >> {\n> > > >> - DeviceEnumerator *enumerator;\n> > > >> + std::unique_ptr enumerator;\n> > > >>\n> > > >> /**\n> > > >> * \\todo Add compile time checks to only try udev enumerator if\n> > > >> libudev\n> > > >> * is available.\n> > > >> */\n> > > >> - enumerator = new DeviceEnumeratorUdev();\n> > > >> + enumerator = std::make_unique(); /* [1] */\n> > > >> + enumerator = std::unique_ptr(new\n> > > >> DeviceEnumeratorUdev()); /* [2] */\n> > > >> + enumerator.reset(new> DeviceEnumeratorUdev()); /* [3] */\n> > > >\n> > > > Here are three different ways of implementing the same operation.\n> > > > std::make_unique<> doesn't exist in C++11, so I've defined it in utils.c.\n> > > > Adding functions to the std namespace could be considered a big of a hack\n> > > > though, so two other implementations are proposed. The second option is\n> > > > explicit, but a bit too long for my taste, while the third option is short\n> > > > but uses reset() which sounds a bit strange for an assignment. Do you\n> > > > have any advice ?\n> > >\n> > > Before we have C++11 in Chromium we also had a base::MakeUnique<>\n> > > template in the Chromium libbase handling creation of unique pointers.\n> > > If we have to stick with C++11 then we might consider doing the same,\n> > > probably with a utils:: namespace. Hacking the std:: namespace is\n> > > indeed a bad idea an can cause build errors.\n> > >\n> > > Semantically I'd say [2] is more accurate than [3], but I don't really\n> > > have strong opinions between these two. If we don't want to define our\n> > > own make_unique template then we can use either.\n> >\n> > I'm tempted to add our own make_unique implementation then, most likely in the\n> > libcamera:: or libcamera::utils:: namespace though.\n>\n> Sounds good!\n>\n> >\n> > > Do we restrict ourselves in C++11 for compatibility reason?\n> >\n> > That was the rationale, but it could be reconsidered if needed.\n>\n> I suppose C++11 shall be sufficient. We can indeed reconsider if we\n> have strong use cases for newer standards one day.\n>\n> >\n> > > >> if (!enumerator->init())\n> > > >> return enumerator;\n> > > >>\n> > > >> - delete enumerator;\n> > > >> -\n> > > >> /*\n> > > >> * Either udev is not available or udev initialization failed.\n> > > >> Fall back\n> > > >> * on the sysfs enumerator.\n> > > >> diff --git a/src/libcamera/include/device_enumerator.h\n> > > >> b/src/libcamera/include/device_enumerator.h index\n> > > >> 29737da7a225..0afaf88ce1ca 100644\n> > > >> --- a/src/libcamera/include/device_enumerator.h\n> > > >> +++ b/src/libcamera/include/device_enumerator.h\n> > > >> @@ -8,6 +8,7 @@\n> > > >> #define __LIBCAMERA_DEVICE_ENUMERATOR_H__\n> > > >>\n> > > >> #include \n> > > >> +#include \n> > > >> #include \n> > > >> #include \n> > > >>\n> > > >> @@ -34,7 +35,7 @@ private:\n> > > >> class DeviceEnumerator\n> > > >> {\n> > > >> public:\n> > > >> - static DeviceEnumerator *create();\n> > > >> + static std::unique_ptr create();\n> > > >> virtual ~DeviceEnumerator();\n> > > >>\n> > > >> diff --git a/src/libcamera/include/utils.h\n> > > >> b/src/libcamera/include/utils.h\n> > > >> index 3ffa6f4ea591..6df54e758aa4 100644\n> > > >> --- a/src/libcamera/include/utils.h\n> > > >> +++ b/src/libcamera/include/utils.h\n> > > >> @@ -7,6 +7,19 @@\n> > > >>\n> > > >> #ifndef __LIBCAMERA_UTILS_H__\n> > > >> #define __LIBCAMERA_UTILS_H__\n> > > >>\n> > > >> +#include \n> > > >> +\n> > > >> #define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))\n> > > >>\n> > > >> +namespace std {\n> > > >> +\n> > > >> +/* C++11 doesn't provide std::make_unique */\n> > > >> +template\n> > > >> +std::unique_ptr make_unique(Args&&... args)\n> > > >> +{\n> > > >> + return std::unique_ptr(new T(std::forward(args)...));\n> > > >> +}\n> > > >> +\n> > > >> +} /* namespace std */\n> > > >> +\n> > > >> #endif /* __LIBCAMERA_UTILS_H__ */\n> > > >>\n> > > >> std::shared_ptr, on the other hand, shall be used only if absolutely\n> > > >> necessary, or it can be a recipe for creating ownership spaghetti.\n> > > >>\n> > > >>> };\n> > > >\n> > > > [snip]\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\n> >\n> >\n> >","headers":{"Return-Path":"","Received":["from mail-wm1-x341.google.com (mail-wm1-x341.google.com\n\t[IPv6:2a00:1450:4864:20::341])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A04FD60C78\n\tfor ;\n\tTue, 15 Jan 2019 15:26:28 +0100 (CET)","by mail-wm1-x341.google.com with SMTP id m22so3418811wml.3\n\tfor ;\n\tTue, 15 Jan 2019 06:26:28 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc:content-transfer-encoding;\n\tbh=FEf60dM+SQp/Tu/nZ68N4z61BgBU/0A+RxFz97/xXSA=;\n\tb=OzM4zGRNv0fSHFDxqYnTRvRhPTLKOvGTaj+IVXDJXY5Cuntrvs19Q+E13RC7jog/2W\n\tPfBTs5exZDS7Kuvd6FXzhV+eR7zTHgqRw2QiCdhKgQrPNm34CvfrgNjq+4Zkcga6MV+k\n\tCtE62CJ2FIaFmfd9qhytZRpe1VVegKthGYd1U=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc:content-transfer-encoding;\n\tbh=FEf60dM+SQp/Tu/nZ68N4z61BgBU/0A+RxFz97/xXSA=;\n\tb=YRu1boaogkaU8r7RzXxV9o4Duts6tkD0hxMrLX3Acnydd+wyBThrXgwSzdlyX+0CC2\n\tSTrf0KNNi8ONp3pIpXs+cVU47KODinU2qt3Yy7MxStWsePDjZ0eQfyzVToIMLAvZHnRL\n\t1KgC65evcyvmhufsICSDEFrcfVuEhz17hUofElxJLLciF8a0RHlTDer0YxgSkyO+MaJC\n\toAdwa8GLKOL1pw1fd4WzaPRCbtWK7Nxe3cpxs+kZJFYHyLCqKpladHg0liGy+lbeBkXV\n\tFKQ+hCeOTrNKFSiiTOUihgbGS8eGXx00+oraCOCl0IFYIQPYG4nPHB77NevbqYUpOqM6\n\trXYQ==","X-Gm-Message-State":"AJcUukcoSjo29Rnutloit9TMep7KYsZZFa/7wfBhv+H1DIAr+r4eE8hg\n\tY8s3gWnbKpVCHTKoc2/yGxeS5K+gcLrNrKVCqxK5uA==","X-Google-Smtp-Source":"ALg8bN7TBMbvOvIUedGLTsyJDaecpriVOAxRpWmYe16FiO+vhMiMrKOYKo/DkThuAUCbols3GXsYQG261mD87ZnuA44=","X-Received":"by 2002:a1c:aa0f:: with SMTP id\n\tt15mr3548288wme.108.1547562387546; \n\tTue, 15 Jan 2019 06:26:27 -0800 (PST)","MIME-Version":"1.0","References":"<20190107231151.23291-1-laurent.pinchart@ideasonboard.com>\n\t<1616763.ZV2yHh8qhD@avalon>\n\t\n\t<3011091.8Ud1DvGsir@avalon>\n\t","In-Reply-To":"","From":"Ricky Liang ","Date":"Tue, 15 Jan 2019 22:26:14 +0800","Message-ID":"","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, shik@chromium.org","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH v2 04/11] libcamera: camera_manager:\n\tMake the class a singleton","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 15 Jan 2019 14:26:28 -0000"}},{"id":319,"web_url":"https://patchwork.libcamera.org/comment/319/","msgid":"<2527427.ymo1Ht2vdc@avalon>","date":"2019-01-15T14:58:15","subject":"Re: [libcamera-devel] [PATCH v2 04/11] libcamera: camera_manager:\n\tMake the class a singleton","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Ricky,\n\nOn Tuesday, 15 January 2019 05:16:00 EET Ricky Liang wrote:\n> On Tue, Jan 15, 2019 at 10:19 AM Laurent Pinchart wrote:\n> > On Monday, 14 January 2019 10:19:50 EET Ricky Liang wrote:\n> >> On Sat, Jan 12, 2019 at 12:02 AM Laurent Pinchart wrote:\n> >>> On Tuesday, 8 January 2019 16:48:12 EET Ricky Liang wrote:\n> >>>> On Tue, Jan 8, 2019 at 7:10 AM Laurent Pinchart wrote:\n> >>>>> There can only be a single camera manager instance in the\n> >>>>> application. Creating it as a singleton helps avoiding mistakes. It\n> >>>>> also allows the camera manager to be used as a storage of global\n> >>>>> data, such as the future event dispatcher.\n> >>>>> \n> >>>>> Signed-off-by: Laurent Pinchart \n> >>>>> Reviewed-by: Niklas Söderlund \n> >>>>> Reviewed-by: Jacopo Mondi \n> >>>>> ---\n> >>>>> Changes since v1:\n> >>>>> \n> >>>>> - Delete copy constructor and assignment operator\n> >>>>> - Fix documentation style issue\n> >>>>> ---\n> >>>>> \n> >>>>> include/libcamera/camera_manager.h | 8 ++++++--\n> >>>>> src/libcamera/camera_manager.cpp | 15 +++++++++++++++\n> >>>>> test/list.cpp | 7 +------\n> >>>>> 3 files changed, 22 insertions(+), 8 deletions(-)\n\n[snip]\n\n> >>>> Not directly related to this patch, but have we considered making\n> >>>> these pointers std::unique_ptr? From our experience working in\n> >>>> Chromium we find it informative, easier to follow the code, and less\n> >>>> error-prone if an object's ownership can be clearly identified\n> >>>> through an std::unique_ptr.\n> >>>> \n> >>>> For example, in the current libcamera code base I noticed there's a\n> >>>> potential leak in DeviceEnumerator::create() if enumerator->init()\n> >>>> fails:\n> >>>> \n> >>>> https://git.linuxtv.org/libcamera.git/tree/src/libcamera/device_enume\n> >>>> rator.cpp#n137\n> >>>> \n> >>>> If we instead only create and pass std::unique_ptr\n> >>>> around then we'd avoid leak like this, and people can also look at\n> >>>> the code and clearly understands that CameraManager has the ownership\n> >>>> of enumerator_.\n> >>> \n> >>> I agree with you, std::unique_ptr<> would here both provide the\n> >>> advantages of a scoped pointer with automatic deletion, and make it\n> >>> clear who owns the object. I gave it a try for enumerator_, and here\n> >>> is what I ended up with (quote characters added to comment inline).\n\n[snip]\n\n> >>>> diff --git a/src/libcamera/camera_manager.cpp\n> >>>> b/src/libcamera/camera_manager.cpp index be327f5d5638..432f2b0a11c5\n> >>>> 100644\n> >>>> --- a/src/libcamera/camera_manager.cpp\n> >>>> +++ b/src/libcamera/camera_manager.cpp\n\n[snip]\n\n> >>>> @@ -84,7 +83,7 @@ int CameraManager::start()\n> >>>> * all pipelines it can provide.\n> >>>> */\n> >>>> do {\n> >>>> - pipe = PipelineHandlerFactory::create(handler,\n> >>>> enumerator_);\n> >>>> + pipe = PipelineHandlerFactory::create(handler,\n> >>>> enumerator_.get());\n> >>> \n> >>> We already break the unique_ptr<> promise :-) If this acceptable\n> >>> according to you, or is there a better way ?\n> >> \n> >> If we're not going to change the internal state of enumerator_, then\n> >> we can make PipelineHandlerFactory::create take a const reference\n> >> instead of a pointer. In general we use const reference for\n> >> method/function arguments that stay unchanged after calling the\n> >> method/function, and pointer for output arguments.\n> >> \n> >> Wdyt?\n> > \n> > While we don't have to change the state of the enumerator strictly\n> > speaking, we need to change the state of the MediaDevice instances it\n> > stores (as pointers in a vector). We call the following method for that\n> > purpose:\n> > \n> > MediaDevice *DeviceEnumerator::search(const DeviceMatch &dm) const\n> > \n> > which I think should not be const as it allows changing the state of an\n> > object owned by the enumerator.\n> \n> I see. Thanks for the detail!\n> \n> > What we really need to convey through the API is that the called function\n> > PipelineHandlerFactory::create() receive a borrowed references to the\n> > enumerator and may not access it after it returns. As far as I know\n> > there's no simple construct in C++ that is universally understood as\n> > expressing this, unlike passing a unique_ptr to express that ownership is\n> > transferred. We could possibly standardize on using references for that\n> > purpose (const and non- const), but in some cases we still need pointers\n> > when passing nullptr is valid, so it wouldn't be a great solution. What's\n> > your opinion on this, could we set in stone the rule that a reference\n> > received by a function means that the reference is borrowed for the\n> > duration of the function only ?\n> \n> Personally I'm not a big fan of non-const reference. I find it easily\n> confused when reviewing the code as it has value syntax with pointer\n> semantics. Having raw pointer arguments and/or return values is fine\n> and often necessary.\n> \n> I'd suggest we use std::unique_ptr<> whenever possible to establish\n> clear object ownership, and const reference whenever we don't plan to\n> alter the state of the object. We then can put most of our attention\n> to the remaining raw pointers.\n> \n> As we're following the Google C++ coding style I'd suggest we follow:\n> \n> https://google.github.io/styleguide/cppguide.html#Reference_Argument\n\nWe already do, with one exception that is just an oversight and can easily be \nfixed (I'll submit a patch).\n\nTo recap, the idea woulc be to standardize on the following semantics:\n\n- Passing ownership: std::unique_ptr<>\n- Passing a reference to a shared object: std::shared_ptr<>\n- Passing a borrowed reference when the object doesn't need to be modified: \nconst &\n- Passing a borrowed reference when the object can be modified: pointer\n- Passing a borrowed reference to an output parameter: pointer\n- Passing a borrowed reference to an object that can be null: pointer\n\nThis implies that the callee can never store a reference to a pointer it \nreceives, as all the use cases for pointers pass borrowed references.\n\nAm I missing something ?\n\n> >>>> if (pipe)\n> >>>> pipes_.push_back(pipe);\n> >>>> } while (pipe);\n\n[snip]","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 683EB60C78\n\tfor ;\n\tTue, 15 Jan 2019 15:57:00 +0100 (CET)","from avalon.localnet (dfj612ybrt5fhg77mgycy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:2e86:4862:ef6a:2804])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C347D4F8;\n\tTue, 15 Jan 2019 15:56:59 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1547564219;\n\tbh=O+YHf8DmXyRwfcv7AvOTA7tN7YiqWqyr5wWfWOMzq+8=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=tdBrRysOGtsLAQohtcACxXIjXP5JusN5K4yuW6vvlZiWyL0kfwY1MYQ0lL4RL9f1C\n\tcA0H8UuIohbJKWPjtLhmd5+QTWjCsiI/UJO3SqQr/TdoKsgyVWiuZF5NznAqmUGvl7\n\tFiBGyY3IoVSxp22khSq7si3VeTM0/O54ze37WMEs=","From":"Laurent Pinchart ","To":"Ricky Liang ","Cc":"libcamera-devel@lists.libcamera.org","Date":"Tue, 15 Jan 2019 16:58:15 +0200","Message-ID":"<2527427.ymo1Ht2vdc@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"","References":"<20190107231151.23291-1-laurent.pinchart@ideasonboard.com>\n\t<3011091.8Ud1DvGsir@avalon>\n\t","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","Content-Type":"text/plain; charset=\"iso-8859-1\"","Subject":"Re: [libcamera-devel] [PATCH v2 04/11] libcamera: camera_manager:\n\tMake the class a singleton","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 15 Jan 2019 14:57:00 -0000"}},{"id":328,"web_url":"https://patchwork.libcamera.org/comment/328/","msgid":"","date":"2019-01-15T16:18:18","subject":"Re: [libcamera-devel] [PATCH v2 04/11] libcamera: camera_manager:\n\tMake the class a singleton","submitter":{"id":6,"url":"https://patchwork.libcamera.org/api/people/6/","name":"Ricky Liang","email":"jcliang@chromium.org"},"content":"Hi Laurent,\n\nOn Tue, Jan 15, 2019 at 10:57 PM Laurent Pinchart\n wrote:\n>\n> Hi Ricky,\n>\n> On Tuesday, 15 January 2019 05:16:00 EET Ricky Liang wrote:\n> > On Tue, Jan 15, 2019 at 10:19 AM Laurent Pinchart wrote:\n> > > On Monday, 14 January 2019 10:19:50 EET Ricky Liang wrote:\n> > >> On Sat, Jan 12, 2019 at 12:02 AM Laurent Pinchart wrote:\n> > >>> On Tuesday, 8 January 2019 16:48:12 EET Ricky Liang wrote:\n> > >>>> On Tue, Jan 8, 2019 at 7:10 AM Laurent Pinchart wrote:\n> > >>>>> There can only be a single camera manager instance in the\n> > >>>>> application. Creating it as a singleton helps avoiding mistakes. It\n> > >>>>> also allows the camera manager to be used as a storage of global\n> > >>>>> data, such as the future event dispatcher.\n> > >>>>>\n> > >>>>> Signed-off-by: Laurent Pinchart \n> > >>>>> Reviewed-by: Niklas Söderlund \n> > >>>>> Reviewed-by: Jacopo Mondi \n> > >>>>> ---\n> > >>>>> Changes since v1:\n> > >>>>>\n> > >>>>> - Delete copy constructor and assignment operator\n> > >>>>> - Fix documentation style issue\n> > >>>>> ---\n> > >>>>>\n> > >>>>> include/libcamera/camera_manager.h | 8 ++++++--\n> > >>>>> src/libcamera/camera_manager.cpp | 15 +++++++++++++++\n> > >>>>> test/list.cpp | 7 +------\n> > >>>>> 3 files changed, 22 insertions(+), 8 deletions(-)\n>\n> [snip]\n>\n> > >>>> Not directly related to this patch, but have we considered making\n> > >>>> these pointers std::unique_ptr? From our experience working in\n> > >>>> Chromium we find it informative, easier to follow the code, and less\n> > >>>> error-prone if an object's ownership can be clearly identified\n> > >>>> through an std::unique_ptr.\n> > >>>>\n> > >>>> For example, in the current libcamera code base I noticed there's a\n> > >>>> potential leak in DeviceEnumerator::create() if enumerator->init()\n> > >>>> fails:\n> > >>>>\n> > >>>> https://git.linuxtv.org/libcamera.git/tree/src/libcamera/device_enume\n> > >>>> rator.cpp#n137\n> > >>>>\n> > >>>> If we instead only create and pass std::unique_ptr\n> > >>>> around then we'd avoid leak like this, and people can also look at\n> > >>>> the code and clearly understands that CameraManager has the ownership\n> > >>>> of enumerator_.\n> > >>>\n> > >>> I agree with you, std::unique_ptr<> would here both provide the\n> > >>> advantages of a scoped pointer with automatic deletion, and make it\n> > >>> clear who owns the object. I gave it a try for enumerator_, and here\n> > >>> is what I ended up with (quote characters added to comment inline).\n>\n> [snip]\n>\n> > >>>> diff --git a/src/libcamera/camera_manager.cpp\n> > >>>> b/src/libcamera/camera_manager.cpp index be327f5d5638..432f2b0a11c5\n> > >>>> 100644\n> > >>>> --- a/src/libcamera/camera_manager.cpp\n> > >>>> +++ b/src/libcamera/camera_manager.cpp\n>\n> [snip]\n>\n> > >>>> @@ -84,7 +83,7 @@ int CameraManager::start()\n> > >>>> * all pipelines it can provide.\n> > >>>> */\n> > >>>> do {\n> > >>>> - pipe = PipelineHandlerFactory::create(handler,\n> > >>>> enumerator_);\n> > >>>> + pipe = PipelineHandlerFactory::create(handler,\n> > >>>> enumerator_.get());\n> > >>>\n> > >>> We already break the unique_ptr<> promise :-) If this acceptable\n> > >>> according to you, or is there a better way ?\n> > >>\n> > >> If we're not going to change the internal state of enumerator_, then\n> > >> we can make PipelineHandlerFactory::create take a const reference\n> > >> instead of a pointer. In general we use const reference for\n> > >> method/function arguments that stay unchanged after calling the\n> > >> method/function, and pointer for output arguments.\n> > >>\n> > >> Wdyt?\n> > >\n> > > While we don't have to change the state of the enumerator strictly\n> > > speaking, we need to change the state of the MediaDevice instances it\n> > > stores (as pointers in a vector). We call the following method for that\n> > > purpose:\n> > >\n> > > MediaDevice *DeviceEnumerator::search(const DeviceMatch &dm) const\n> > >\n> > > which I think should not be const as it allows changing the state of an\n> > > object owned by the enumerator.\n> >\n> > I see. Thanks for the detail!\n> >\n> > > What we really need to convey through the API is that the called function\n> > > PipelineHandlerFactory::create() receive a borrowed references to the\n> > > enumerator and may not access it after it returns. As far as I know\n> > > there's no simple construct in C++ that is universally understood as\n> > > expressing this, unlike passing a unique_ptr to express that ownership is\n> > > transferred. We could possibly standardize on using references for that\n> > > purpose (const and non- const), but in some cases we still need pointers\n> > > when passing nullptr is valid, so it wouldn't be a great solution. What's\n> > > your opinion on this, could we set in stone the rule that a reference\n> > > received by a function means that the reference is borrowed for the\n> > > duration of the function only ?\n> >\n> > Personally I'm not a big fan of non-const reference. I find it easily\n> > confused when reviewing the code as it has value syntax with pointer\n> > semantics. Having raw pointer arguments and/or return values is fine\n> > and often necessary.\n> >\n> > I'd suggest we use std::unique_ptr<> whenever possible to establish\n> > clear object ownership, and const reference whenever we don't plan to\n> > alter the state of the object. We then can put most of our attention\n> > to the remaining raw pointers.\n> >\n> > As we're following the Google C++ coding style I'd suggest we follow:\n> >\n> > https://google.github.io/styleguide/cppguide.html#Reference_Argument\n>\n> We already do, with one exception that is just an oversight and can easily be\n> fixed (I'll submit a patch).\n>\n> To recap, the idea woulc be to standardize on the following semantics:\n>\n> - Passing ownership: std::unique_ptr<>\n> - Passing a reference to a shared object: std::shared_ptr<>\n> - Passing a borrowed reference when the object doesn't need to be modified:\n> const &\n> - Passing a borrowed reference when the object can be modified: pointer\n> - Passing a borrowed reference to an output parameter: pointer\n> - Passing a borrowed reference to an object that can be null: pointer\n>\n> This implies that the callee can never store a reference to a pointer it\n> receives, as all the use cases for pointers pass borrowed references.\n>\n> Am I missing something ?\n\nI believe as the code base grows there may be cases where we'll need\nto store a reference to a pointer. When that happens we can document\nin comments what makes it safe to store and access the pointer.\n\nOtherwise your summary looks good to me.\n\n>\n> > >>>> if (pipe)\n> > >>>> pipes_.push_back(pipe);\n> > >>>> } while (pipe);\n>\n> [snip]\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n>\n>","headers":{"Return-Path":"","Received":["from mail-wm1-x332.google.com (mail-wm1-x332.google.com\n\t[IPv6:2a00:1450:4864:20::332])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7C02960C86\n\tfor ;\n\tTue, 15 Jan 2019 17:18:31 +0100 (CET)","by mail-wm1-x332.google.com with SMTP id f188so3798333wmf.5\n\tfor ;\n\tTue, 15 Jan 2019 08:18:31 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc:content-transfer-encoding;\n\tbh=nTwHThe408nwOXATVSF0rxN7VJYhi40uT/mf7pbagSI=;\n\tb=Wu10Rg6ugKbjyEZ/XqkcFbuCgzo9KCi1OK54srgKzhAONsyprTUsGM6em4rsgz5K6F\n\tj/vpoZsWzIFlxTYUGK4bCfEQ374lQ4KFeJC/cjFvhlPXluKKIYYrZm0z10DUgE1JTSio\n\tPoXTQu2+LYrEFYYFfc16upu21tJuXxQPIIQo0=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc:content-transfer-encoding;\n\tbh=nTwHThe408nwOXATVSF0rxN7VJYhi40uT/mf7pbagSI=;\n\tb=t2Lk2B+UeEBXkPil7D5wvD4pQUgkU2VsvrxFgwd2K8ye+IEu63CXZJ15YvubFJ/b7V\n\txa2lWsI2Bg+LBeQOTqUREZee6fEaoAapqKmCkXf6SEMVFbdFs5DdeldgNETNpEZxqIRg\n\tzXjYHNb80of5O/uO3pfaT7XnK4EnGdZhMjBODuuqDJzUpj64cWDByG7lCFWDyl+0J38Q\n\t6HCzQ+rQjiVmaUv2sAQoSI0af38OCu2KSsKE6on9GnWxvjnrbB2aZe6OTl1IIMa3IFfh\n\tQkuHTT3pFdCEmQW0rSHenHKRcGchgj17/3CMvuB8Q0H08NSJpuLKwMbanMmY3frcMVZ5\n\tIP0A==","X-Gm-Message-State":"AJcUukc5kY/n7y9y25KpztAS0cRi25Nz6/kpSpPfPJEjwwTyqL4Z2pPg\n\tYJSMeovNEGKLbyY2BGaE/HUMNedfwei6TyuOn8NWpEL6","X-Google-Smtp-Source":"ALg8bN5TcoOo2jCxoSAMJYmQBGdvmSacYN/veSgJq8rjoBHzbWKwqS0tBdKv73i9Ma7pNVmQbrD2PkgY84LvDEGtzEM=","X-Received":"by 2002:a7b:c04e:: with SMTP id\n\tu14mr3644831wmc.133.1547569110558; \n\tTue, 15 Jan 2019 08:18:30 -0800 (PST)","MIME-Version":"1.0","References":"<20190107231151.23291-1-laurent.pinchart@ideasonboard.com>\n\t<3011091.8Ud1DvGsir@avalon>\n\t\n\t<2527427.ymo1Ht2vdc@avalon>","In-Reply-To":"<2527427.ymo1Ht2vdc@avalon>","From":"Ricky Liang ","Date":"Wed, 16 Jan 2019 00:18:18 +0800","Message-ID":"","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH v2 04/11] libcamera: camera_manager:\n\tMake the class a singleton","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 15 Jan 2019 16:18:31 -0000"}},{"id":361,"web_url":"https://patchwork.libcamera.org/comment/361/","msgid":"","date":"2019-01-16T13:46:30","subject":"Re: [libcamera-devel] [PATCH v2 04/11] libcamera: camera_manager:\n\tMake the class a singleton","submitter":{"id":7,"url":"https://patchwork.libcamera.org/api/people/7/","name":"Shik Chen","email":"shik@chromium.org"},"content":"Hi all,\n\nOn Tue, Jan 15, 2019 at 10:26 PM Ricky Liang wrote:\n>\n> + Shik, who has some ideas about the C++ version to use and useful\n> third-party C++ libraries\n>\n> On Tue, Jan 15, 2019 at 11:16 AM Ricky Liang wrote:\n> >\n> > Hi Laurent,\n> >\n> > On Tue, Jan 15, 2019 at 10:19 AM Laurent Pinchart\n> > wrote:\n> > >\n> > > Hi Ricky,\n> > >\n> > > On Monday, 14 January 2019 10:19:50 EET Ricky Liang wrote:\n> > > > On Sat, Jan 12, 2019 at 12:02 AM Laurent Pinchart wrote:\n> > > > > On Tuesday, 8 January 2019 16:48:12 EET Ricky Liang wrote:\n> > > > >> On Tue, Jan 8, 2019 at 7:10 AM Laurent Pinchart wrote:\n> > > > >>> There can only be a single camera manager instance in the application.\n> > > > >>> Creating it as a singleton helps avoiding mistakes. It also allows the\n> > > > >>> camera manager to be used as a storage of global data, such as the\n> > > > >>> future event dispatcher.\n> > > > >>>\n> > > > >>> Signed-off-by: Laurent Pinchart \n> > > > >>> Reviewed-by: Niklas Söderlund \n> > > > >>> Reviewed-by: Jacopo Mondi \n> > > > >>> ---\n> > > > >>> Changes since v1:\n> > > > >>>\n> > > > >>> - Delete copy constructor and assignment operator\n> > > > >>> - Fix documentation style issue\n> > > > >>> ---\n> > > > >>>\n> > > > >>> include/libcamera/camera_manager.h | 8 ++++++--\n> > > > >>> src/libcamera/camera_manager.cpp | 15 +++++++++++++++\n> > > > >>> test/list.cpp | 7 +------\n> > > > >>> 3 files changed, 22 insertions(+), 8 deletions(-)\n> > > > >>>\n> > > > >>> diff --git a/include/libcamera/camera_manager.h\n> > > > >>> b/include/libcamera/camera_manager.h index 2768a5bd2384..e14da0f893b3\n> > > > >>> 100644\n> > > > >>> --- a/include/libcamera/camera_manager.h\n> > > > >>> +++ b/include/libcamera/camera_manager.h\n> > > > >>> @@ -19,15 +19,19 @@ class PipelineHandler;\n> > > > >>> class CameraManager\n> > > > >>> {\n> > > > >>> public:\n> > > > >>> - CameraManager();\n> > > > >>> -\n> > > > >>> int start();\n> > > > >>> void stop();\n> > > > >>>\n> > > > >>> std::vector list() const;\n> > > > >>> Camera *get(const std::string &name);\n> > > > >>>\n> > > > >>> + static CameraManager *instance();\n> > > > >>> +\n> > > > >>> private:\n> > > > >>> + CameraManager();\n> > > > >>> + CameraManager(const CameraManager &) = delete;\n> > > > >>> + void operator=(const CameraManager &) = delete;\n> > > > >>> +\n> > > > >>> DeviceEnumerator *enumerator_;\n> > > > >>> std::vector pipes_;\n> > > > >>\n> > > > >> Not directly related to this patch, but have we considered making\n> > > > >> these pointers std::unique_ptr? From our experience working in\n> > > > >> Chromium we find it informative, easier to follow the code, and less\n> > > > >> error-prone if an object's ownership can be clearly identified through\n> > > > >> an std::unique_ptr.\n> > > > >>\n> > > > >> For example, in the current libcamera code base I noticed there's a\n> > > > >> potential leak in DeviceEnumerator::create() if enumerator->init()\n> > > > >> fails:\n> > > > >>\n> > > > >> https://git.linuxtv.org/libcamera.git/tree/src/libcamera/device_enumerat\n> > > > >> or.c pp#n137\n> > > > >>\n> > > > >> If we instead only create and pass std::unique_ptr\n> > > > >> around then we'd avoid leak like this, and people can also look at the\n> > > > >> code and clearly understands that CameraManager has the ownership of\n> > > > >> enumerator_.\n> > > > >\n> > > > > I agree with you, std::unique_ptr<> would here both provide the advantages\n> > > > > of a scoped pointer with automatic deletion, and make it clear who owns\n> > > > > the object. I gave it a try for enumerator_, and here is what I ended up\n> > > > > with (quote characters added to comment inline).\n> > > > >\n> > > > >> diff --git a/include/libcamera/camera_manager.h\n> > > > >> b/include/libcamera/camera_manager.h index 15e7c162032a..6cfcba3c3933\n> > > > >> 100644\n> > > > >> --- a/include/libcamera/camera_manager.h\n> > > > >> +++ b/include/libcamera/camera_manager.h\n> > > > >> @@ -7,6 +7,7 @@\n> > > > >> #ifndef __LIBCAMERA_CAMERA_MANAGER_H__\n> > > > >> #define __LIBCAMERA_CAMERA_MANAGER_H__\n> > > > >>\n> > > > >> +#include \n> > > > >> #include \n> > > > >> #include \n> > > > >>\n> > > > >> @@ -37,7 +38,7 @@ private:\n> > > > >> void operator=(const CameraManager &) = delete;\n> > > > >> ~CameraManager();\n> > > > >>\n> > > > >> - DeviceEnumerator *enumerator_;\n> > > > >> + std::unique_ptr enumerator_;\n> > > > >>\n> > > > >> std::vector pipes_;\n> > > > >\n> > > > > pipes_ left out as they will disappear in the near future, to be replaced\n> > > > > with a vector of reference-counted camera objects.\n> > > > >\n> > > > >> EventDispatcher *dispatcher_;\n> > > > >>\n> > > > >> diff --git a/src/libcamera/camera_manager.cpp\n> > > > >> b/src/libcamera/camera_manager.cpp index be327f5d5638..432f2b0a11c5\n> > > > >> 100644\n> > > > >> --- a/src/libcamera/camera_manager.cpp\n> > > > >> +++ b/src/libcamera/camera_manager.cpp\n> > > > >> @@ -61,7 +61,6 @@ CameraManager::~CameraManager()\n> > > > >> */\n> > > > >> int CameraManager::start()\n> > > > >> {\n> > > > >> -\n> > > > >> if (enumerator_)\n> > > > >> return -ENODEV;\n> > > > >>\n> > > > >> @@ -84,7 +83,7 @@ int CameraManager::start()\n> > > > >> * all pipelines it can provide.\n> > > > >> */\n> > > > >> do {\n> > > > >> - pipe = PipelineHandlerFactory::create(handler,\n> > > > >> enumerator_);\n> > > > >> + pipe = PipelineHandlerFactory::create(handler,\n> > > > >> enumerator_.get());\n> > > > >\n> > > > > We already break the unique_ptr<> promise :-) If this acceptable according\n> > > > > to you, or is there a better way ?\n> > > >\n> > > > If we're not going to change the internal state of enumerator_, then\n> > > > we can make PipelineHandlerFactory::create take a const reference\n> > > > instead of a pointer. In general we use const reference for\n> > > > method/function arguments that stay unchanged after calling the\n> > > > method/function, and pointer for output arguments.\n> > > >\n> > > > Wdyt?\n> > >\n> > > While we don't have to change the state of the enumerator strictly speaking,\n> > > we need to change the state of the MediaDevice instances it stores (as\n> > > pointers in a vector). We call the following method for that purpose:\n> > >\n> > > MediaDevice *DeviceEnumerator::search(const DeviceMatch &dm) const\n> > >\n> > > which I think should not be const as it allows changing the state of an object\n> > > owned by the enumerator.\n> >\n> > I see. Thanks for the detail!\n> >\n> > >\n> > > What we really need to convey through the API is that the called function\n> > > PipelineHandlerFactory::create() receive a borrowed references to the\n> > > enumerator and may not access it after it returns. As far as I know there's no\n> > > simple construct in C++ that is universally understood as expressing this,\n> > > unlike passing a unique_ptr to express that ownership is transferred. We could\n> > > possibly standardize on using references for that purpose (const and non-\n> > > const), but in some cases we still need pointers when passing nullptr is\n> > > valid, so it wouldn't be a great solution. What's your opinion on this, could\n> > > we set in stone the rule that a reference received by a function means that\n> > > the reference is borrowed for the duration of the function only ?\n> >\n> > Personally I'm not a big fan of non-const reference. I find it easily\n> > confused when reviewing the code as it has value syntax with pointer\n> > semantics. Having raw pointer arguments and/or return values is fine\n> > and often necessary.\n> >\n> > I'd suggest we use std::unique_ptr<> whenever possible to establish\n> > clear object ownership, and const reference whenever we don't plan to\n> > alter the state of the object. We then can put most of our attention\n> > to the remaining raw pointers.\n> >\n> > As we're following the Google C++ coding style I'd suggest we follow:\n> >\n> > https://google.github.io/styleguide/cppguide.html#Reference_Argument\n> >\n> > >\n> > > > >> if (pipe)\n> > > > >> pipes_.push_back(pipe);\n> > > > >> } while (pipe);\n> > > > >> @@ -114,10 +113,7 @@ void CameraManager::stop()\n> > > > >>\n> > > > >> pipes_.clear();\n> > > > >>\n> > > > >> - if (enumerator_)\n> > > > >> - delete enumerator_;\n> > > > >> -\n> > > > >> - enumerator_ = nullptr;\n> > > > >> + enumerator_.reset(nullptr);\n> > > > >> }\n> > > > >>\n> > > > >> /**\n> > > > >> diff --git a/src/libcamera/device_enumerator.cpp\n> > > > >> b/src/libcamera/device_enumerator.cpp index 0d18e75525af..2b03b191fa5d\n> > > > >> 100644\n> > > > >> --- a/src/libcamera/device_enumerator.cpp\n> > > > >> +++ b/src/libcamera/device_enumerator.cpp\n> > > > >> @@ -14,6 +14,7 @@\n> > > > >> #include \"device_enumerator.h\"\n> > > > >> #include \"log.h\"\n> > > > >> #include \"media_device.h\"\n> > > > >> +#include \"utils.h\"\n> > > > >>\n> > > > >> /**\n> > > > >> * \\file device_enumerator.h\n> > > > >> @@ -128,20 +129,20 @@ bool DeviceMatch::match(const MediaDevice *device)\n> > > > >> const\n> > > > >> * \\return A pointer to the newly created device enumerator on success,\n> > > > >> or\n> > > > >> * nullptr if an error occurs\n> > > > >> */\n> > > > >> -DeviceEnumerator *DeviceEnumerator::create()\n> > > > >> +std::unique_ptr DeviceEnumerator::create()\n> > > > >> {\n> > > > >> - DeviceEnumerator *enumerator;\n> > > > >> + std::unique_ptr enumerator;\n> > > > >>\n> > > > >> /**\n> > > > >> * \\todo Add compile time checks to only try udev enumerator if\n> > > > >> libudev\n> > > > >> * is available.\n> > > > >> */\n> > > > >> - enumerator = new DeviceEnumeratorUdev();\n> > > > >> + enumerator = std::make_unique(); /* [1] */\n> > > > >> + enumerator = std::unique_ptr(new\n> > > > >> DeviceEnumeratorUdev()); /* [2] */\n> > > > >> + enumerator.reset(new> DeviceEnumeratorUdev()); /* [3] */\n> > > > >\n> > > > > Here are three different ways of implementing the same operation.\n> > > > > std::make_unique<> doesn't exist in C++11, so I've defined it in utils.c.\n> > > > > Adding functions to the std namespace could be considered a big of a hack\n> > > > > though, so two other implementations are proposed. The second option is\n> > > > > explicit, but a bit too long for my taste, while the third option is short\n> > > > > but uses reset() which sounds a bit strange for an assignment. Do you\n> > > > > have any advice ?\n> > > >\n> > > > Before we have C++11 in Chromium we also had a base::MakeUnique<>\n> > > > template in the Chromium libbase handling creation of unique pointers.\n> > > > If we have to stick with C++11 then we might consider doing the same,\n> > > > probably with a utils:: namespace. Hacking the std:: namespace is\n> > > > indeed a bad idea an can cause build errors.\n> > > >\n> > > > Semantically I'd say [2] is more accurate than [3], but I don't really\n> > > > have strong opinions between these two. If we don't want to define our\n> > > > own make_unique template then we can use either.\n> > >\n> > > I'm tempted to add our own make_unique implementation then, most likely in the\n> > > libcamera:: or libcamera::utils:: namespace though.\n> >\n> > Sounds good!\n> >\n> > >\n> > > > Do we restrict ourselves in C++11 for compatibility reason?\n> > >\n> > > That was the rationale, but it could be reconsidered if needed.\n> >\n> > I suppose C++11 shall be sufficient. We can indeed reconsider if we\n> > have strong use cases for newer standards one day.\n\nImplementing make_unique or anything already defined in the newer standards need\nbe done with extra care. Users would expect it to work in the same way, so any\ninconsistency with the standards might be a hidden trap. Making things forward\ncompatible is not an easy task. For example, the implementation below does not\nprovide the make_unique(std::size_t) overload defined in C++14.\n\nI'd suggest not to reinvent the wheel if there is no strong objection. Is it\npossible to bump the C++ version to C++14/17? We can still limit the scope of\nnew language features as we did in [1], so we can opt-in the features gradually.\nAnother possibility is adopting some existing library such as abseil [2] which\nincludes make_unique and many other goodies for projects in C++11.\n\n[1] http://www.libcamera.org/coding-style.html#c-specific-rules\n[2] https://abseil.io/\n\n> >\n> > >\n> > > > >> if (!enumerator->init())\n> > > > >> return enumerator;\n> > > > >>\n> > > > >> - delete enumerator;\n> > > > >> -\n> > > > >> /*\n> > > > >> * Either udev is not available or udev initialization failed.\n> > > > >> Fall back\n> > > > >> * on the sysfs enumerator.\n> > > > >> diff --git a/src/libcamera/include/device_enumerator.h\n> > > > >> b/src/libcamera/include/device_enumerator.h index\n> > > > >> 29737da7a225..0afaf88ce1ca 100644\n> > > > >> --- a/src/libcamera/include/device_enumerator.h\n> > > > >> +++ b/src/libcamera/include/device_enumerator.h\n> > > > >> @@ -8,6 +8,7 @@\n> > > > >> #define __LIBCAMERA_DEVICE_ENUMERATOR_H__\n> > > > >>\n> > > > >> #include \n> > > > >> +#include \n> > > > >> #include \n> > > > >> #include \n> > > > >>\n> > > > >> @@ -34,7 +35,7 @@ private:\n> > > > >> class DeviceEnumerator\n> > > > >> {\n> > > > >> public:\n> > > > >> - static DeviceEnumerator *create();\n> > > > >> + static std::unique_ptr create();\n> > > > >> virtual ~DeviceEnumerator();\n> > > > >>\n> > > > >> diff --git a/src/libcamera/include/utils.h\n> > > > >> b/src/libcamera/include/utils.h\n> > > > >> index 3ffa6f4ea591..6df54e758aa4 100644\n> > > > >> --- a/src/libcamera/include/utils.h\n> > > > >> +++ b/src/libcamera/include/utils.h\n> > > > >> @@ -7,6 +7,19 @@\n> > > > >>\n> > > > >> #ifndef __LIBCAMERA_UTILS_H__\n> > > > >> #define __LIBCAMERA_UTILS_H__\n> > > > >>\n> > > > >> +#include \n> > > > >> +\n> > > > >> #define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))\n> > > > >>\n> > > > >> +namespace std {\n> > > > >> +\n> > > > >> +/* C++11 doesn't provide std::make_unique */\n> > > > >> +template\n> > > > >> +std::unique_ptr make_unique(Args&&... args)\n> > > > >> +{\n> > > > >> + return std::unique_ptr(new T(std::forward(args)...));\n> > > > >> +}\n> > > > >> +\n> > > > >> +} /* namespace std */\n> > > > >> +\n> > > > >> #endif /* __LIBCAMERA_UTILS_H__ */\n> > > > >>\n> > > > >> std::shared_ptr, on the other hand, shall be used only if absolutely\n> > > > >> necessary, or it can be a recipe for creating ownership spaghetti.\n> > > > >>\n> > > > >>> };\n> > > > >\n> > > > > [snip]\n> > >\n> > > --\n> > > Regards,\n> > >\n> > > Laurent Pinchart\n> > >\n> > >\n> > >\n\nSincerely,\nShik","headers":{"Return-Path":"","Received":["from mail-lj1-x243.google.com (mail-lj1-x243.google.com\n\t[IPv6:2a00:1450:4864:20::243])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EB1BF60B2D\n\tfor ;\n\tWed, 16 Jan 2019 14:46:42 +0100 (CET)","by mail-lj1-x243.google.com with SMTP id k19-v6so5447423lji.11\n\tfor ;\n\tWed, 16 Jan 2019 05:46:42 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc:content-transfer-encoding;\n\tbh=daIdd7c1v5dYhkh11trP72yzOoWMELtYrJ6YzqW0+Ck=;\n\tb=R2TNnWMGAy94PcOWjGnjSyCRH0m1JLyXf6NOTpUfm4CIGBD0TVJX3i8Z5xDNHBshwV\n\tL2+/3CrKeMC9wxFjtV639DT7hLneZTlw9O/jRQK6b4uQZXdUthvGSqcbhSu1Qk0JDtBj\n\t63dxaFhqIW60vppMHCC1FlPeDZvOgqbpeTJPQ=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc:content-transfer-encoding;\n\tbh=daIdd7c1v5dYhkh11trP72yzOoWMELtYrJ6YzqW0+Ck=;\n\tb=G2ZgST0Fgl2LivRxNMIsGBKP3x/rA8qLTqMUZ8sugYsQUPNcAmyVqw3NqDe+OJpskG\n\tdIp+0dS964/Tqw435JjkGNgYAOZfwB+pQ3Erf4yxAWuiPkLxv7hmXMbP+sqjoJ3DqRrd\n\t1eBRogtIGxHxWUUZLneY+f1rAHqmo4xhr2CM2HNTuPHOzEXFN/yv1bNuSEmJu1AqxOK4\n\tCclKyCqh843i2tqK92ZfC4qlZtDPL62lJurewikh8/QW9MkpMZboI2E/CNgXwcjUKnbi\n\tDoCRC4utzYSpqoupVa7bVKGkDYym9mE+GuhxMZazNstYM/1Tv0nGWPZDM7U+Kyk7YPrg\n\tOVyQ==","X-Gm-Message-State":"AJcUukdsSMXXpYF2KpKYlttrqhZaOBsVThOA289mh/nzlJ4deN+MEX48\n\tHAa0/GnSskkOrMTGBqlo9+y7yXvPLBTM0lT+CLENBA==","X-Google-Smtp-Source":"ALg8bN7W8lMuKtS4wm9Z5a+cyIqFynL4bVTZUtW4NX4Y4dhSwG36EoWyFwDG/Dcc0aetoniDm49YXTicG8xPwyL6oek=","X-Received":"by 2002:a2e:7f04:: with SMTP id\n\ta4-v6mr7079004ljd.156.1547646401997; \n\tWed, 16 Jan 2019 05:46:41 -0800 (PST)","MIME-Version":"1.0","References":"<20190107231151.23291-1-laurent.pinchart@ideasonboard.com>\n\t<1616763.ZV2yHh8qhD@avalon>\n\t\n\t<3011091.8Ud1DvGsir@avalon>\n\t\n\t","In-Reply-To":"","From":"Shik Chen ","Date":"Wed, 16 Jan 2019 21:46:30 +0800","Message-ID":"","To":"Ricky Liang ","Cc":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH v2 04/11] libcamera: camera_manager:\n\tMake the class a singleton","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 16 Jan 2019 13:46:43 -0000"}},{"id":398,"web_url":"https://patchwork.libcamera.org/comment/398/","msgid":"<20190118003014.GI23244@pendragon.ideasonboard.com>","date":"2019-01-18T00:30:14","subject":"Re: [libcamera-devel] [PATCH v2 04/11] libcamera: camera_manager:\n\tMake the class a singleton","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Shik,\n\nOn Wed, Jan 16, 2019 at 09:46:30PM +0800, Shik Chen wrote:\n> On Tue, Jan 15, 2019 at 10:26 PM Ricky Liang wrote:\n> > On Tue, Jan 15, 2019 at 11:16 AM Ricky Liang wrote:\n> >> On Tue, Jan 15, 2019 at 10:19 AM Laurent Pinchart\n> >> wrote:\n> >>> On Monday, 14 January 2019 10:19:50 EET Ricky Liang wrote:\n> >>>> On Sat, Jan 12, 2019 at 12:02 AM Laurent Pinchart wrote:\n> >>>>> On Tuesday, 8 January 2019 16:48:12 EET Ricky Liang wrote:\n> >>>>>> On Tue, Jan 8, 2019 at 7:10 AM Laurent Pinchart wrote:\n> >>>>>>> There can only be a single camera manager instance in the application.\n> >>>>>>> Creating it as a singleton helps avoiding mistakes. It also allows the\n> >>>>>>> camera manager to be used as a storage of global data, such as the\n> >>>>>>> future event dispatcher.\n> >>>>>>>\n> >>>>>>> Signed-off-by: Laurent Pinchart \n> >>>>>>> Reviewed-by: Niklas Söderlund \n> >>>>>>> Reviewed-by: Jacopo Mondi \n> >>>>>>> ---\n> >>>>>>> Changes since v1:\n> >>>>>>>\n> >>>>>>> - Delete copy constructor and assignment operator\n> >>>>>>> - Fix documentation style issue\n> >>>>>>> ---\n> >>>>>>>\n> >>>>>>> include/libcamera/camera_manager.h | 8 ++++++--\n> >>>>>>> src/libcamera/camera_manager.cpp | 15 +++++++++++++++\n> >>>>>>> test/list.cpp | 7 +------\n> >>>>>>> 3 files changed, 22 insertions(+), 8 deletions(-)\n\n[snip]\n\n> >>>>>> diff --git a/src/libcamera/device_enumerator.cpp\n> >>>>>> b/src/libcamera/device_enumerator.cpp index 0d18e75525af..2b03b191fa5d\n> >>>>>> 100644\n> >>>>>> --- a/src/libcamera/device_enumerator.cpp\n> >>>>>> +++ b/src/libcamera/device_enumerator.cpp\n> >>>>>> @@ -14,6 +14,7 @@\n> >>>>>> #include \"device_enumerator.h\"\n> >>>>>> #include \"log.h\"\n> >>>>>> #include \"media_device.h\"\n> >>>>>> +#include \"utils.h\"\n> >>>>>>\n> >>>>>> /**\n> >>>>>> * \\file device_enumerator.h\n> >>>>>> @@ -128,20 +129,20 @@ bool DeviceMatch::match(const MediaDevice *device)\n> >>>>>> const\n> >>>>>> * \\return A pointer to the newly created device enumerator on success,\n> >>>>>> or\n> >>>>>> * nullptr if an error occurs\n> >>>>>> */\n> >>>>>> -DeviceEnumerator *DeviceEnumerator::create()\n> >>>>>> +std::unique_ptr DeviceEnumerator::create()\n> >>>>>> {\n> >>>>>> - DeviceEnumerator *enumerator;\n> >>>>>> + std::unique_ptr enumerator;\n> >>>>>>\n> >>>>>> /**\n> >>>>>> * \\todo Add compile time checks to only try udev enumerator if\n> >>>>>> libudev\n> >>>>>> * is available.\n> >>>>>> */\n> >>>>>> - enumerator = new DeviceEnumeratorUdev();\n> >>>>>> + enumerator = std::make_unique(); /* [1] */\n> >>>>>> + enumerator = std::unique_ptr(new\n> >>>>>> DeviceEnumeratorUdev()); /* [2] */\n> >>>>>> + enumerator.reset(new> DeviceEnumeratorUdev()); /* [3] */\n> >>>>>\n> >>>>> Here are three different ways of implementing the same operation.\n> >>>>> std::make_unique<> doesn't exist in C++11, so I've defined it in utils.c.\n> >>>>> Adding functions to the std namespace could be considered a big of a hack\n> >>>>> though, so two other implementations are proposed. The second option is\n> >>>>> explicit, but a bit too long for my taste, while the third option is short\n> >>>>> but uses reset() which sounds a bit strange for an assignment. Do you\n> >>>>> have any advice ?\n> >>>>\n> >>>> Before we have C++11 in Chromium we also had a base::MakeUnique<>\n> >>>> template in the Chromium libbase handling creation of unique pointers.\n> >>>> If we have to stick with C++11 then we might consider doing the same,\n> >>>> probably with a utils:: namespace. Hacking the std:: namespace is\n> >>>> indeed a bad idea an can cause build errors.\n> >>>>\n> >>>> Semantically I'd say [2] is more accurate than [3], but I don't really\n> >>>> have strong opinions between these two. If we don't want to define our\n> >>>> own make_unique template then we can use either.\n> >>>\n> >>> I'm tempted to add our own make_unique implementation then, most likely in the\n> >>> libcamera:: or libcamera::utils:: namespace though.\n> >>\n> >> Sounds good!\n> >>\n> >>>\n> >>>> Do we restrict ourselves in C++11 for compatibility reason?\n> >>>\n> >>> That was the rationale, but it could be reconsidered if needed.\n> >>\n> >> I suppose C++11 shall be sufficient. We can indeed reconsider if we\n> >> have strong use cases for newer standards one day.\n> \n> Implementing make_unique or anything already defined in the newer standards need\n> be done with extra care. Users would expect it to work in the same way, so any\n> inconsistency with the standards might be a hidden trap. Making things forward\n> compatible is not an easy task. For example, the implementation below does not\n> provide the make_unique(std::size_t) overload defined in C++14.\n> \n> I'd suggest not to reinvent the wheel if there is no strong objection. Is it\n> possible to bump the C++ version to C++14/17? We can still limit the scope of\n> new language features as we did in [1], so we can opt-in the features gradually.\n\nThis worries me for two reasons. The first one is that depending on\nC++14/C++17 lock us out of platform that don't support those newer\nstandards. This is more of a general concern, I don't have any data to\ntell whether this is a valid concern or not.\n\nThe second concern is that we may start using features of C++14/C++17\nwithout noticing, possibly creating problems later if we want to support\nC++11-based systems.\n\n> Another possibility is adopting some existing library such as abseil [2] which\n> includes make_unique and many other goodies for projects in C++11.\n> \n> [1] http://www.libcamera.org/coding-style.html#c-specific-rules\n> [2] https://abseil.io/\n\nThank you for the pointer, that's an interesting library. However, at\nthis point, I would like to avoid adding an external dependency just to\nprovide make_unique support. I'm thus tempted to keep our in-house\nimplementation (which by the way is proposed by\nhttps://en.cppreference.com/w/cpp/memory/unique_ptr/make_unique), and\nrevisit this topic if/when we will need more feature of newer C++\nversions. Does this sound acceptable to you ?\n\n> >>\n> >>>\n> >>>>>> if (!enumerator->init())\n> >>>>>> return enumerator;\n> >>>>>>\n> >>>>>> - delete enumerator;\n> >>>>>> -\n> >>>>>> /*\n> >>>>>> * Either udev is not available or udev initialization failed.\n> >>>>>> Fall back\n> >>>>>> * on the sysfs enumerator.\n\n[snip]","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0A9D160B2D\n\tfor ;\n\tFri, 18 Jan 2019 01:30:15 +0100 (CET)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 69A6653F;\n\tFri, 18 Jan 2019 01:30:14 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1547771414;\n\tbh=UPKizLx2PAQfmVWB6L0p1NxezxasFFpJUGt4+ZqSNI0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fx3lU42p6mLgcHKuWv4yexrwgKpNn0uyDYL1jzEiclbt6zDDaOtGJ7m5F85TR2sXK\n\tIa3mwVMoGRr1zcjDH+JwIdTjMNu2FUKZDb/Ci+VTPUHn0vIe8aLgxsXrSMENY0613h\n\tWIE9qdsJuP2rxLM1C8O+WNjBCOzkvMQs93X9wRwQ=","Date":"Fri, 18 Jan 2019 02:30:14 +0200","From":"Laurent Pinchart ","To":"Shik Chen ","Cc":"Ricky Liang , libcamera-devel@lists.libcamera.org","Message-ID":"<20190118003014.GI23244@pendragon.ideasonboard.com>","References":"<20190107231151.23291-1-laurent.pinchart@ideasonboard.com>\n\t<1616763.ZV2yHh8qhD@avalon>\n\t\n\t<3011091.8Ud1DvGsir@avalon>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 04/11] libcamera: camera_manager:\n\tMake the class a singleton","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 18 Jan 2019 00:30:15 -0000"}},{"id":402,"web_url":"https://patchwork.libcamera.org/comment/402/","msgid":"","date":"2019-01-18T11:12:56","subject":"Re: [libcamera-devel] [PATCH v2 04/11] libcamera: camera_manager:\n\tMake the class a singleton","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 18/01/2019 00:30, Laurent Pinchart wrote:\n> Hi Shik,\n> \n> On Wed, Jan 16, 2019 at 09:46:30PM +0800, Shik Chen wrote:\n>> On Tue, Jan 15, 2019 at 10:26 PM Ricky Liang wrote:\n>>> On Tue, Jan 15, 2019 at 11:16 AM Ricky Liang wrote:\n>>>> On Tue, Jan 15, 2019 at 10:19 AM Laurent Pinchart\n>>>> wrote:\n>>>>> On Monday, 14 January 2019 10:19:50 EET Ricky Liang wrote:\n>>>>>> On Sat, Jan 12, 2019 at 12:02 AM Laurent Pinchart wrote:\n>>>>>>> On Tuesday, 8 January 2019 16:48:12 EET Ricky Liang wrote:\n>>>>>>>> On Tue, Jan 8, 2019 at 7:10 AM Laurent Pinchart wrote:\n>>>>>>>>> There can only be a single camera manager instance in the application.\n>>>>>>>>> Creating it as a singleton helps avoiding mistakes. It also allows the\n>>>>>>>>> camera manager to be used as a storage of global data, such as the\n>>>>>>>>> future event dispatcher.\n>>>>>>>>>\n>>>>>>>>> Signed-off-by: Laurent Pinchart \n>>>>>>>>> Reviewed-by: Niklas Söderlund \n>>>>>>>>> Reviewed-by: Jacopo Mondi \n>>>>>>>>> ---\n>>>>>>>>> Changes since v1:\n>>>>>>>>>\n>>>>>>>>> - Delete copy constructor and assignment operator\n>>>>>>>>> - Fix documentation style issue\n>>>>>>>>> ---\n>>>>>>>>>\n>>>>>>>>> include/libcamera/camera_manager.h | 8 ++++++--\n>>>>>>>>> src/libcamera/camera_manager.cpp | 15 +++++++++++++++\n>>>>>>>>> test/list.cpp | 7 +------\n>>>>>>>>> 3 files changed, 22 insertions(+), 8 deletions(-)\n> \n> [snip]\n> \n>>>>>>>> diff --git a/src/libcamera/device_enumerator.cpp\n>>>>>>>> b/src/libcamera/device_enumerator.cpp index 0d18e75525af..2b03b191fa5d\n>>>>>>>> 100644\n>>>>>>>> --- a/src/libcamera/device_enumerator.cpp\n>>>>>>>> +++ b/src/libcamera/device_enumerator.cpp\n>>>>>>>> @@ -14,6 +14,7 @@\n>>>>>>>> #include \"device_enumerator.h\"\n>>>>>>>> #include \"log.h\"\n>>>>>>>> #include \"media_device.h\"\n>>>>>>>> +#include \"utils.h\"\n>>>>>>>>\n>>>>>>>> /**\n>>>>>>>> * \\file device_enumerator.h\n>>>>>>>> @@ -128,20 +129,20 @@ bool DeviceMatch::match(const MediaDevice *device)\n>>>>>>>> const\n>>>>>>>> * \\return A pointer to the newly created device enumerator on success,\n>>>>>>>> or\n>>>>>>>> * nullptr if an error occurs\n>>>>>>>> */\n>>>>>>>> -DeviceEnumerator *DeviceEnumerator::create()\n>>>>>>>> +std::unique_ptr DeviceEnumerator::create()\n>>>>>>>> {\n>>>>>>>> - DeviceEnumerator *enumerator;\n>>>>>>>> + std::unique_ptr enumerator;\n>>>>>>>>\n>>>>>>>> /**\n>>>>>>>> * \\todo Add compile time checks to only try udev enumerator if\n>>>>>>>> libudev\n>>>>>>>> * is available.\n>>>>>>>> */\n>>>>>>>> - enumerator = new DeviceEnumeratorUdev();\n>>>>>>>> + enumerator = std::make_unique(); /* [1] */\n>>>>>>>> + enumerator = std::unique_ptr(new\n>>>>>>>> DeviceEnumeratorUdev()); /* [2] */\n>>>>>>>> + enumerator.reset(new> DeviceEnumeratorUdev()); /* [3] */\n>>>>>>>\n>>>>>>> Here are three different ways of implementing the same operation.\n>>>>>>> std::make_unique<> doesn't exist in C++11, so I've defined it in utils.c.\n>>>>>>> Adding functions to the std namespace could be considered a big of a hack\n>>>>>>> though, so two other implementations are proposed. The second option is\n>>>>>>> explicit, but a bit too long for my taste, while the third option is short\n>>>>>>> but uses reset() which sounds a bit strange for an assignment. Do you\n>>>>>>> have any advice ?\n>>>>>>\n>>>>>> Before we have C++11 in Chromium we also had a base::MakeUnique<>\n>>>>>> template in the Chromium libbase handling creation of unique pointers.\n>>>>>> If we have to stick with C++11 then we might consider doing the same,\n>>>>>> probably with a utils:: namespace. Hacking the std:: namespace is\n>>>>>> indeed a bad idea an can cause build errors.\n>>>>>>\n>>>>>> Semantically I'd say [2] is more accurate than [3], but I don't really\n>>>>>> have strong opinions between these two. If we don't want to define our\n>>>>>> own make_unique template then we can use either.\n>>>>>\n>>>>> I'm tempted to add our own make_unique implementation then, most likely in the\n>>>>> libcamera:: or libcamera::utils:: namespace though.\n>>>>\n>>>> Sounds good!\n>>>>\n>>>>>\n>>>>>> Do we restrict ourselves in C++11 for compatibility reason?\n>>>>>\n>>>>> That was the rationale, but it could be reconsidered if needed.\n>>>>\n>>>> I suppose C++11 shall be sufficient. We can indeed reconsider if we\n>>>> have strong use cases for newer standards one day.\n>>\n>> Implementing make_unique or anything already defined in the newer standards need\n>> be done with extra care. Users would expect it to work in the same way, so any\n>> inconsistency with the standards might be a hidden trap. Making things forward\n>> compatible is not an easy task. For example, the implementation below does not\n>> provide the make_unique(std::size_t) overload defined in C++14.\n>>\n>> I'd suggest not to reinvent the wheel if there is no strong objection. Is it\n>> possible to bump the C++ version to C++14/17? We can still limit the scope of\n>> new language features as we did in [1], so we can opt-in the features gradually.\n> \n> This worries me for two reasons. The first one is that depending on\n> C++14/C++17 lock us out of platform that don't support those newer\n> standards. This is more of a general concern, I don't have any data to\n> tell whether this is a valid concern or not.\n> \n> The second concern is that we may start using features of C++14/C++17\n> without noticing, possibly creating problems later if we want to support\n> C++11-based systems.\n> \n>> Another possibility is adopting some existing library such as abseil [2] which\n>> includes make_unique and many other goodies for projects in C++11.\n>>\n>> [1] http://www.libcamera.org/coding-style.html#c-specific-rules\n>> [2] https://abseil.io/\n> \n> Thank you for the pointer, that's an interesting library. However, at\n> this point, I would like to avoid adding an external dependency just to\n> provide make_unique support. I'm thus tempted to keep our in-house\n> implementation (which by the way is proposed by\n> https://en.cppreference.com/w/cpp/memory/unique_ptr/make_unique), and\n> revisit this topic if/when we will need more feature of newer C++\n> versions. Does this sound acceptable to you ?\n\nIf we're going to have our 'own' implementation of make_unique - could\nyou add a test to test/meson.build::internal_tests please?\n\n\n\n\n>>>>>>>> if (!enumerator->init())\n>>>>>>>> return enumerator;\n>>>>>>>>\n>>>>>>>> - delete enumerator;\n>>>>>>>> -\n>>>>>>>> /*\n>>>>>>>> * Either udev is not available or udev initialization failed.\n>>>>>>>> Fall back\n>>>>>>>> * on the sysfs enumerator.\n> \n> [snip]\n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3AFED60B21\n\tfor ;\n\tFri, 18 Jan 2019 12:13:01 +0100 (CET)","from [192.168.0.21]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6C3D353E;\n\tFri, 18 Jan 2019 12:13:00 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1547809980;\n\tbh=UekzHn/Nnw8K/jDsy+yzqKkwUlBYxN6WpK+/mD3h4/8=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=dvo0CnxZWHv4HOLBJHWk3SiIcrK2vFfQSAbMqnXUxlvYiCONNQGkETzRoKPXacVlJ\n\t1nBAGWvaSE0fenSvoQVD4iI7zWUT/MxGWm7OpfBgLBDHrv3V7B/4cPWyYybp0zfMGg\n\thTlGy7uqOZ7gleSbeISlQZJETeIJRSu9hJxx97As=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ,\n\tShik Chen ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20190107231151.23291-1-laurent.pinchart@ideasonboard.com>\n\t<1616763.ZV2yHh8qhD@avalon>\n\t\n\t<3011091.8Ud1DvGsir@avalon>\n\t\n\t\n\t\n\t<20190118003014.GI23244@pendragon.ideasonboard.com>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"","Date":"Fri, 18 Jan 2019 11:12:56 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.2.1","MIME-Version":"1.0","In-Reply-To":"<20190118003014.GI23244@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 04/11] libcamera: camera_manager:\n\tMake the class a singleton","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 18 Jan 2019 11:13:01 -0000"}},{"id":403,"web_url":"https://patchwork.libcamera.org/comment/403/","msgid":"<20190118123600.GB5275@pendragon.ideasonboard.com>","date":"2019-01-18T12:36:00","subject":"Re: [libcamera-devel] [PATCH v2 04/11] libcamera: camera_manager:\n\tMake the class a singleton","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Fri, Jan 18, 2019 at 11:12:56AM +0000, Kieran Bingham wrote:\n> On 18/01/2019 00:30, Laurent Pinchart wrote:\n> > On Wed, Jan 16, 2019 at 09:46:30PM +0800, Shik Chen wrote:\n> >> On Tue, Jan 15, 2019 at 10:26 PM Ricky Liang wrote:\n> >>> On Tue, Jan 15, 2019 at 11:16 AM Ricky Liang wrote:\n> >>>> On Tue, Jan 15, 2019 at 10:19 AM Laurent Pinchart\n> >>>> wrote:\n> >>>>> On Monday, 14 January 2019 10:19:50 EET Ricky Liang wrote:\n> >>>>>> On Sat, Jan 12, 2019 at 12:02 AM Laurent Pinchart wrote:\n> >>>>>>> On Tuesday, 8 January 2019 16:48:12 EET Ricky Liang wrote:\n> >>>>>>>> On Tue, Jan 8, 2019 at 7:10 AM Laurent Pinchart wrote:\n> >>>>>>>>> There can only be a single camera manager instance in the application.\n> >>>>>>>>> Creating it as a singleton helps avoiding mistakes. It also allows the\n> >>>>>>>>> camera manager to be used as a storage of global data, such as the\n> >>>>>>>>> future event dispatcher.\n> >>>>>>>>>\n> >>>>>>>>> Signed-off-by: Laurent Pinchart \n> >>>>>>>>> Reviewed-by: Niklas Söderlund \n> >>>>>>>>> Reviewed-by: Jacopo Mondi \n> >>>>>>>>> ---\n> >>>>>>>>> Changes since v1:\n> >>>>>>>>>\n> >>>>>>>>> - Delete copy constructor and assignment operator\n> >>>>>>>>> - Fix documentation style issue\n> >>>>>>>>> ---\n> >>>>>>>>>\n> >>>>>>>>> include/libcamera/camera_manager.h | 8 ++++++--\n> >>>>>>>>> src/libcamera/camera_manager.cpp | 15 +++++++++++++++\n> >>>>>>>>> test/list.cpp | 7 +------\n> >>>>>>>>> 3 files changed, 22 insertions(+), 8 deletions(-)\n> > \n> > [snip]\n> > \n> >>>>>>>> diff --git a/src/libcamera/device_enumerator.cpp\n> >>>>>>>> b/src/libcamera/device_enumerator.cpp index 0d18e75525af..2b03b191fa5d\n> >>>>>>>> 100644\n> >>>>>>>> --- a/src/libcamera/device_enumerator.cpp\n> >>>>>>>> +++ b/src/libcamera/device_enumerator.cpp\n> >>>>>>>> @@ -14,6 +14,7 @@\n> >>>>>>>> #include \"device_enumerator.h\"\n> >>>>>>>> #include \"log.h\"\n> >>>>>>>> #include \"media_device.h\"\n> >>>>>>>> +#include \"utils.h\"\n> >>>>>>>>\n> >>>>>>>> /**\n> >>>>>>>> * \\file device_enumerator.h\n> >>>>>>>> @@ -128,20 +129,20 @@ bool DeviceMatch::match(const MediaDevice *device)\n> >>>>>>>> const\n> >>>>>>>> * \\return A pointer to the newly created device enumerator on success,\n> >>>>>>>> or\n> >>>>>>>> * nullptr if an error occurs\n> >>>>>>>> */\n> >>>>>>>> -DeviceEnumerator *DeviceEnumerator::create()\n> >>>>>>>> +std::unique_ptr DeviceEnumerator::create()\n> >>>>>>>> {\n> >>>>>>>> - DeviceEnumerator *enumerator;\n> >>>>>>>> + std::unique_ptr enumerator;\n> >>>>>>>>\n> >>>>>>>> /**\n> >>>>>>>> * \\todo Add compile time checks to only try udev enumerator if\n> >>>>>>>> libudev\n> >>>>>>>> * is available.\n> >>>>>>>> */\n> >>>>>>>> - enumerator = new DeviceEnumeratorUdev();\n> >>>>>>>> + enumerator = std::make_unique(); /* [1] */\n> >>>>>>>> + enumerator = std::unique_ptr(new\n> >>>>>>>> DeviceEnumeratorUdev()); /* [2] */\n> >>>>>>>> + enumerator.reset(new> DeviceEnumeratorUdev()); /* [3] */\n> >>>>>>>\n> >>>>>>> Here are three different ways of implementing the same operation.\n> >>>>>>> std::make_unique<> doesn't exist in C++11, so I've defined it in utils.c.\n> >>>>>>> Adding functions to the std namespace could be considered a big of a hack\n> >>>>>>> though, so two other implementations are proposed. The second option is\n> >>>>>>> explicit, but a bit too long for my taste, while the third option is short\n> >>>>>>> but uses reset() which sounds a bit strange for an assignment. Do you\n> >>>>>>> have any advice ?\n> >>>>>>\n> >>>>>> Before we have C++11 in Chromium we also had a base::MakeUnique<>\n> >>>>>> template in the Chromium libbase handling creation of unique pointers.\n> >>>>>> If we have to stick with C++11 then we might consider doing the same,\n> >>>>>> probably with a utils:: namespace. Hacking the std:: namespace is\n> >>>>>> indeed a bad idea an can cause build errors.\n> >>>>>>\n> >>>>>> Semantically I'd say [2] is more accurate than [3], but I don't really\n> >>>>>> have strong opinions between these two. If we don't want to define our\n> >>>>>> own make_unique template then we can use either.\n> >>>>>\n> >>>>> I'm tempted to add our own make_unique implementation then, most likely in the\n> >>>>> libcamera:: or libcamera::utils:: namespace though.\n> >>>>\n> >>>> Sounds good!\n> >>>>\n> >>>>>\n> >>>>>> Do we restrict ourselves in C++11 for compatibility reason?\n> >>>>>\n> >>>>> That was the rationale, but it could be reconsidered if needed.\n> >>>>\n> >>>> I suppose C++11 shall be sufficient. We can indeed reconsider if we\n> >>>> have strong use cases for newer standards one day.\n> >>\n> >> Implementing make_unique or anything already defined in the newer standards need\n> >> be done with extra care. Users would expect it to work in the same way, so any\n> >> inconsistency with the standards might be a hidden trap. Making things forward\n> >> compatible is not an easy task. For example, the implementation below does not\n> >> provide the make_unique(std::size_t) overload defined in C++14.\n> >>\n> >> I'd suggest not to reinvent the wheel if there is no strong objection. Is it\n> >> possible to bump the C++ version to C++14/17? We can still limit the scope of\n> >> new language features as we did in [1], so we can opt-in the features gradually.\n> > \n> > This worries me for two reasons. The first one is that depending on\n> > C++14/C++17 lock us out of platform that don't support those newer\n> > standards. This is more of a general concern, I don't have any data to\n> > tell whether this is a valid concern or not.\n> > \n> > The second concern is that we may start using features of C++14/C++17\n> > without noticing, possibly creating problems later if we want to support\n> > C++11-based systems.\n> > \n> >> Another possibility is adopting some existing library such as abseil [2] which\n> >> includes make_unique and many other goodies for projects in C++11.\n> >>\n> >> [1] http://www.libcamera.org/coding-style.html#c-specific-rules\n> >> [2] https://abseil.io/\n> > \n> > Thank you for the pointer, that's an interesting library. However, at\n> > this point, I would like to avoid adding an external dependency just to\n> > provide make_unique support. I'm thus tempted to keep our in-house\n> > implementation (which by the way is proposed by\n> > https://en.cppreference.com/w/cpp/memory/unique_ptr/make_unique), and\n> > revisit this topic if/when we will need more feature of newer C++\n> > versions. Does this sound acceptable to you ?\n> \n> If we're going to have our 'own' implementation of make_unique - could\n> you add a test to test/meson.build::internal_tests please?\n\nThat's a good point. I'm however not sure how to test it meaningfully\nother than making sure it compiles, which is already handled by the\nlibcamera code base. Feel free to propose a test :-)\n\n> >>>>>>>> if (!enumerator->init())\n> >>>>>>>> return enumerator;\n> >>>>>>>>\n> >>>>>>>> - delete enumerator;\n> >>>>>>>> -\n> >>>>>>>> /*\n> >>>>>>>> * Either udev is not available or udev initialization failed.\n> >>>>>>>> Fall back\n> >>>>>>>> * on the sysfs enumerator.\n> > \n> > [snip]\n> >","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DB73C60B21\n\tfor ;\n\tFri, 18 Jan 2019 13:36:00 +0100 (CET)","from pendragon.ideasonboard.com\n\t(dfj612yyyyyyyyyyyyyby-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00::2])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 085A353F;\n\tFri, 18 Jan 2019 13:35:59 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1547814960;\n\tbh=VKune6x3UW2Wex7C9Mka3zlfQj64XAvq6KcYWEeG55A=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wHSUa7r5l1EygCLPIROEZNGdf8LIg5AHB5yfVwaPikFGpHfatOZf3a9qdI9V4uKLX\n\tQHLseDtsSW2B34vsKYKUooJZL2kCYf4FC/R6FafMKA//wsqOmuibbbVUsuVb1TlQ4q\n\tTdmu3piQew5GSO7mJqDiIfkJCfPvdF94pcltSpjo=","Date":"Fri, 18 Jan 2019 14:36:00 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Shik Chen , libcamera-devel@lists.libcamera.org","Message-ID":"<20190118123600.GB5275@pendragon.ideasonboard.com>","References":"<20190107231151.23291-1-laurent.pinchart@ideasonboard.com>\n\t<1616763.ZV2yHh8qhD@avalon>\n\t\n\t<3011091.8Ud1DvGsir@avalon>\n\t\n\t\n\t\n\t<20190118003014.GI23244@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 04/11] libcamera: camera_manager:\n\tMake the class a singleton","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 18 Jan 2019 12:36:01 -0000"}},{"id":404,"web_url":"https://patchwork.libcamera.org/comment/404/","msgid":"","date":"2019-01-18T07:42:28","subject":"Re: [libcamera-devel] [PATCH v2 04/11] libcamera: camera_manager:\n\tMake the class a singleton","submitter":{"id":8,"url":"https://patchwork.libcamera.org/api/people/8/","name":"Shik Chen","email":"shik@google.com"},"content":"Hi Laurent,\n\nOn Fri, Jan 18, 2019 at 8:30 AM Laurent Pinchart\n wrote:\n>\n> Hi Shik,\n>\n> On Wed, Jan 16, 2019 at 09:46:30PM +0800, Shik Chen wrote:\n> > On Tue, Jan 15, 2019 at 10:26 PM Ricky Liang wrote:\n> > > On Tue, Jan 15, 2019 at 11:16 AM Ricky Liang wrote:\n> > >> On Tue, Jan 15, 2019 at 10:19 AM Laurent Pinchart\n> > >> wrote:\n> > >>> On Monday, 14 January 2019 10:19:50 EET Ricky Liang wrote:\n> > >>>> On Sat, Jan 12, 2019 at 12:02 AM Laurent Pinchart wrote:\n> > >>>>> On Tuesday, 8 January 2019 16:48:12 EET Ricky Liang wrote:\n> > >>>>>> On Tue, Jan 8, 2019 at 7:10 AM Laurent Pinchart wrote:\n> > >>>>>>> There can only be a single camera manager instance in the application.\n> > >>>>>>> Creating it as a singleton helps avoiding mistakes. It also allows the\n> > >>>>>>> camera manager to be used as a storage of global data, such as the\n> > >>>>>>> future event dispatcher.\n> > >>>>>>>\n> > >>>>>>> Signed-off-by: Laurent Pinchart \n> > >>>>>>> Reviewed-by: Niklas Söderlund \n> > >>>>>>> Reviewed-by: Jacopo Mondi \n> > >>>>>>> ---\n> > >>>>>>> Changes since v1:\n> > >>>>>>>\n> > >>>>>>> - Delete copy constructor and assignment operator\n> > >>>>>>> - Fix documentation style issue\n> > >>>>>>> ---\n> > >>>>>>>\n> > >>>>>>> include/libcamera/camera_manager.h | 8 ++++++--\n> > >>>>>>> src/libcamera/camera_manager.cpp | 15 +++++++++++++++\n> > >>>>>>> test/list.cpp | 7 +------\n> > >>>>>>> 3 files changed, 22 insertions(+), 8 deletions(-)\n>\n> [snip]\n>\n> > >>>>>> diff --git a/src/libcamera/device_enumerator.cpp\n> > >>>>>> b/src/libcamera/device_enumerator.cpp index 0d18e75525af..2b03b191fa5d\n> > >>>>>> 100644\n> > >>>>>> --- a/src/libcamera/device_enumerator.cpp\n> > >>>>>> +++ b/src/libcamera/device_enumerator.cpp\n> > >>>>>> @@ -14,6 +14,7 @@\n> > >>>>>> #include \"device_enumerator.h\"\n> > >>>>>> #include \"log.h\"\n> > >>>>>> #include \"media_device.h\"\n> > >>>>>> +#include \"utils.h\"\n> > >>>>>>\n> > >>>>>> /**\n> > >>>>>> * \\file device_enumerator.h\n> > >>>>>> @@ -128,20 +129,20 @@ bool DeviceMatch::match(const MediaDevice *device)\n> > >>>>>> const\n> > >>>>>> * \\return A pointer to the newly created device enumerator on success,\n> > >>>>>> or\n> > >>>>>> * nullptr if an error occurs\n> > >>>>>> */\n> > >>>>>> -DeviceEnumerator *DeviceEnumerator::create()\n> > >>>>>> +std::unique_ptr DeviceEnumerator::create()\n> > >>>>>> {\n> > >>>>>> - DeviceEnumerator *enumerator;\n> > >>>>>> + std::unique_ptr enumerator;\n> > >>>>>>\n> > >>>>>> /**\n> > >>>>>> * \\todo Add compile time checks to only try udev enumerator if\n> > >>>>>> libudev\n> > >>>>>> * is available.\n> > >>>>>> */\n> > >>>>>> - enumerator = new DeviceEnumeratorUdev();\n> > >>>>>> + enumerator = std::make_unique(); /* [1] */\n> > >>>>>> + enumerator = std::unique_ptr(new\n> > >>>>>> DeviceEnumeratorUdev()); /* [2] */\n> > >>>>>> + enumerator.reset(new> DeviceEnumeratorUdev()); /* [3] */\n> > >>>>>\n> > >>>>> Here are three different ways of implementing the same operation.\n> > >>>>> std::make_unique<> doesn't exist in C++11, so I've defined it in utils.c.\n> > >>>>> Adding functions to the std namespace could be considered a big of a hack\n> > >>>>> though, so two other implementations are proposed. The second option is\n> > >>>>> explicit, but a bit too long for my taste, while the third option is short\n> > >>>>> but uses reset() which sounds a bit strange for an assignment. Do you\n> > >>>>> have any advice ?\n> > >>>>\n> > >>>> Before we have C++11 in Chromium we also had a base::MakeUnique<>\n> > >>>> template in the Chromium libbase handling creation of unique pointers.\n> > >>>> If we have to stick with C++11 then we might consider doing the same,\n> > >>>> probably with a utils:: namespace. Hacking the std:: namespace is\n> > >>>> indeed a bad idea an can cause build errors.\n> > >>>>\n> > >>>> Semantically I'd say [2] is more accurate than [3], but I don't really\n> > >>>> have strong opinions between these two. If we don't want to define our\n> > >>>> own make_unique template then we can use either.\n> > >>>\n> > >>> I'm tempted to add our own make_unique implementation then, most likely in the\n> > >>> libcamera:: or libcamera::utils:: namespace though.\n> > >>\n> > >> Sounds good!\n> > >>\n> > >>>\n> > >>>> Do we restrict ourselves in C++11 for compatibility reason?\n> > >>>\n> > >>> That was the rationale, but it could be reconsidered if needed.\n> > >>\n> > >> I suppose C++11 shall be sufficient. We can indeed reconsider if we\n> > >> have strong use cases for newer standards one day.\n> >\n> > Implementing make_unique or anything already defined in the newer standards need\n> > be done with extra care. Users would expect it to work in the same way, so any\n> > inconsistency with the standards might be a hidden trap. Making things forward\n> > compatible is not an easy task. For example, the implementation below does not\n> > provide the make_unique(std::size_t) overload defined in C++14.\n> >\n> > I'd suggest not to reinvent the wheel if there is no strong objection. Is it\n> > possible to bump the C++ version to C++14/17? We can still limit the scope of\n> > new language features as we did in [1], so we can opt-in the features gradually.\n>\n> This worries me for two reasons. The first one is that depending on\n> C++14/C++17 lock us out of platform that don't support those newer\n> standards. This is more of a general concern, I don't have any data to\n> tell whether this is a valid concern or not.\n\nYes it's hard to make decision without having the concrete target platforms. The\ncompiler support for C++14 are complete and stable enough in GCC/Clang, and\nit's the default C++ standard version since GCC 5 and Clang 6. But some\npopularish Linux distros might ship with older compilers that does not support\nC++14 well, such as Ubuntu 14.04 and RHEL 6 although they are approaching EOL.\n\n>\n> The second concern is that we may start using features of C++14/C++17\n> without noticing, possibly creating problems later if we want to support\n> C++11-based systems.\n\nThis is actually a good point. Note that the compiler flag \"-std=c++XX\" might\nnot strictly align with the standard. We are already using at least one C++17\nfeature \"nested namespace\" in utils.h, which uses \"namespace libcamera::utils\"\ninstead of \"namespace libcamera { namespace utils\". I found this because it fail\nto compile on one of my machine (Ubuntu 17.10).\n\n>\n> > Another possibility is adopting some existing library such as abseil [2] which\n> > includes make_unique and many other goodies for projects in C++11.\n> >\n> > [1] http://www.libcamera.org/coding-style.html#c-specific-rules\n> > [2] https://abseil.io/\n>\n> Thank you for the pointer, that's an interesting library. However, at\n> this point, I would like to avoid adding an external dependency just to\n> provide make_unique support. I'm thus tempted to keep our in-house\n> implementation (which by the way is proposed by\n> https://en.cppreference.com/w/cpp/memory/unique_ptr/make_unique), and\n> revisit this topic if/when we will need more feature of newer C++\n> versions. Does this sound acceptable to you ?\n\nIt's reasonable that not to add an external dependency just for make_unique. We\ncould revisit this topic as we need it.\n\n>\n> > >>\n> > >>>\n> > >>>>>> if (!enumerator->init())\n> > >>>>>> return enumerator;\n> > >>>>>>\n> > >>>>>> - delete enumerator;\n> > >>>>>> -\n> > >>>>>> /*\n> > >>>>>> * Either udev is not available or udev initialization failed.\n> > >>>>>> Fall back\n> > >>>>>> * on the sysfs enumerator.\n>\n> [snip]\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n\nSincerely,\nShik","headers":{"Return-Path":"","Received":["from mail-qt1-x836.google.com (mail-qt1-x836.google.com\n\t[IPv6:2607:f8b0:4864:20::836])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AAEC860C98\n\tfor ;\n\tFri, 18 Jan 2019 08:42:46 +0100 (CET)","by mail-qt1-x836.google.com with SMTP id y20so14194411qtm.13\n\tfor ;\n\tThu, 17 Jan 2019 23:42:46 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com;\n\ts=20161025; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc:content-transfer-encoding;\n\tbh=SOoYqETueQgyhsa6GU/OouTFHvQBAAhFw+d1u3Do+5A=;\n\tb=TCFNhYJQ/sTTouC3r36+WXm5FMiRwBF8n4Usbp8SJKsx/pPZcE9GlsD1nAiVka+A/D\n\t32nQ5lBU0EtGFhmVQPhXOEgOsZvfMjzzJPvZ4bPD8ZwP4/JC3v3ihaETxKRcPmxRxbw6\n\tyHHmgS2fiEBqo/4i4Vm4la4X7OQZl6+U3mH1mfB3rmtGOFmGJjNMc6MP/Z+JTdSStbON\n\texyhDcSYktXmVR2C9oZyEQqNhf9pyKf5eRSIdyeqIaZCxxRcowXO2eg+PH9HKluTy3Jy\n\tzbns4KzMMUf1s+LfReVqZj6U8NxB5+HUuFoypfj/Ad7zuxm/CkeAfFmg6gWKEuQ4MhyH\n\t/MmA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc:content-transfer-encoding;\n\tbh=SOoYqETueQgyhsa6GU/OouTFHvQBAAhFw+d1u3Do+5A=;\n\tb=VqyCmfmVWNyeTEHt9D4Q4dAh6zhP4PzyEmK/Fl6wCeh/b98Ko16h5s4NX7RRyV9Ofj\n\t0OIQiXfZHNq6O6FNNUHNtrHerAV38aukxvp7IQxXPQ+YbYA7XQ5gsrxfXKhtNRsZIxkh\n\tsJBpDxwHoqqpR7jIjc39DOUdhm7yJxUUbqLjS11shzltYGb0xupBlYxRdqTf9IXrets0\n\tZPzjkGHcKtJRAfNMs5TPPGxZrQX0jJSnjBe7vYcNLyiR4jAhQGl/uNXFCkqmkAreLa/O\n\tQikoxhEXAeFuk+FpgFTGsvMwYfr6QQlL+bZeEmxCHMYGU2/cHwMEk624wiUDqR2S42h5\n\t839g==","X-Gm-Message-State":"AJcUukeG/O4mhIfwFAL3bZNPAXZ3vzKsbkUuQjNZTHPMnYs5htGkPYeu\n\tjsdM6mLgU0JCAd6pYzfQqnaXV7mDfheXrTqGAOdiGw==","X-Google-Smtp-Source":"ALg8bN4ZapHZLuejAgFYxMJJUPpLKCelAahQPQ5ZoCHRQOVUA9CucdE8iQWUDQLsygcYnJ8dXK8oeTvaUaLI126smFM=","X-Received":"by 2002:ac8:3896:: with SMTP id\n\tf22mr14109667qtc.337.1547797365073; \n\tThu, 17 Jan 2019 23:42:45 -0800 (PST)","MIME-Version":"1.0","References":"<20190107231151.23291-1-laurent.pinchart@ideasonboard.com>\n\t<1616763.ZV2yHh8qhD@avalon>\n\t\n\t<3011091.8Ud1DvGsir@avalon>\n\t\n\t\n\t\n\t<20190118003014.GI23244@pendragon.ideasonboard.com>","In-Reply-To":"<20190118003014.GI23244@pendragon.ideasonboard.com>","From":"Shik Chen ","Date":"Fri, 18 Jan 2019 15:42:28 +0800","Message-ID":"","To":"Laurent Pinchart ","Cc":"Shik Chen , Ricky Liang , \n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","X-Mailman-Approved-At":"Fri, 18 Jan 2019 13:49:50 +0100","Subject":"Re: [libcamera-devel] [PATCH v2 04/11] libcamera: camera_manager:\n\tMake the class a singleton","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 18 Jan 2019 07:42:47 -0000"}},{"id":405,"web_url":"https://patchwork.libcamera.org/comment/405/","msgid":"<20190118125733.GC5275@pendragon.ideasonboard.com>","date":"2019-01-18T12:57:33","subject":"Re: [libcamera-devel] [PATCH v2 04/11] libcamera: camera_manager:\n\tMake the class a singleton","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Shik,\n\nOn Fri, Jan 18, 2019 at 03:42:28PM +0800, Shik Chen wrote:\n> On Fri, Jan 18, 2019 at 8:30 AM Laurent Pinchart\n> wrote:\n> > On Wed, Jan 16, 2019 at 09:46:30PM +0800, Shik Chen wrote:\n> >> On Tue, Jan 15, 2019 at 10:26 PM Ricky Liang wrote:\n> >>> On Tue, Jan 15, 2019 at 11:16 AM Ricky Liang wrote:\n> >>>> On Tue, Jan 15, 2019 at 10:19 AM Laurent Pinchart\n> >>>> wrote:\n> >>>>> On Monday, 14 January 2019 10:19:50 EET Ricky Liang wrote:\n> >>>>>> On Sat, Jan 12, 2019 at 12:02 AM Laurent Pinchart wrote:\n> >>>>>>> On Tuesday, 8 January 2019 16:48:12 EET Ricky Liang wrote:\n> >>>>>>>> On Tue, Jan 8, 2019 at 7:10 AM Laurent Pinchart wrote:\n> >>>>>>>>> There can only be a single camera manager instance in the application.\n> >>>>>>>>> Creating it as a singleton helps avoiding mistakes. It also allows the\n> >>>>>>>>> camera manager to be used as a storage of global data, such as the\n> >>>>>>>>> future event dispatcher.\n> >>>>>>>>> \n> >>>>>>>>> Signed-off-by: Laurent Pinchart \n> >>>>>>>>> Reviewed-by: Niklas Söderlund \n> >>>>>>>>> Reviewed-by: Jacopo Mondi \n> >>>>>>>>> ---\n> >>>>>>>>> Changes since v1:\n> >>>>>>>>> \n> >>>>>>>>> - Delete copy constructor and assignment operator\n> >>>>>>>>> - Fix documentation style issue\n> >>>>>>>>> ---\n> >>>>>>>>> \n> >>>>>>>>> include/libcamera/camera_manager.h | 8 ++++++--\n> >>>>>>>>> src/libcamera/camera_manager.cpp | 15 +++++++++++++++\n> >>>>>>>>> test/list.cpp | 7 +------\n> >>>>>>>>> 3 files changed, 22 insertions(+), 8 deletions(-)\n> > \n> > [snip]\n> > \n> >>>>>>>> diff --git a/src/libcamera/device_enumerator.cpp\n> >>>>>>>> b/src/libcamera/device_enumerator.cpp index 0d18e75525af..2b03b191fa5d\n> >>>>>>>> 100644\n> >>>>>>>> --- a/src/libcamera/device_enumerator.cpp\n> >>>>>>>> +++ b/src/libcamera/device_enumerator.cpp\n> >>>>>>>> @@ -14,6 +14,7 @@\n> >>>>>>>> #include \"device_enumerator.h\"\n> >>>>>>>> #include \"log.h\"\n> >>>>>>>> #include \"media_device.h\"\n> >>>>>>>> +#include \"utils.h\"\n> >>>>>>>> \n> >>>>>>>> /**\n> >>>>>>>> * \\file device_enumerator.h\n> >>>>>>>> @@ -128,20 +129,20 @@ bool DeviceMatch::match(const MediaDevice *device)\n> >>>>>>>> const\n> >>>>>>>> * \\return A pointer to the newly created device enumerator on success,\n> >>>>>>>> or\n> >>>>>>>> * nullptr if an error occurs\n> >>>>>>>> */\n> >>>>>>>> -DeviceEnumerator *DeviceEnumerator::create()\n> >>>>>>>> +std::unique_ptr DeviceEnumerator::create()\n> >>>>>>>> {\n> >>>>>>>> - DeviceEnumerator *enumerator;\n> >>>>>>>> + std::unique_ptr enumerator;\n> >>>>>>>> \n> >>>>>>>> /**\n> >>>>>>>> * \\todo Add compile time checks to only try udev enumerator if\n> >>>>>>>> libudev\n> >>>>>>>> * is available.\n> >>>>>>>> */\n> >>>>>>>> - enumerator = new DeviceEnumeratorUdev();\n> >>>>>>>> + enumerator = std::make_unique(); /* [1] */\n> >>>>>>>> + enumerator = std::unique_ptr(new\n> >>>>>>>> DeviceEnumeratorUdev()); /* [2] */\n> >>>>>>>> + enumerator.reset(new> DeviceEnumeratorUdev()); /* [3] */\n> >>>>>>> \n> >>>>>>> Here are three different ways of implementing the same operation.\n> >>>>>>> std::make_unique<> doesn't exist in C++11, so I've defined it in utils.c.\n> >>>>>>> Adding functions to the std namespace could be considered a big of a hack\n> >>>>>>> though, so two other implementations are proposed. The second option is\n> >>>>>>> explicit, but a bit too long for my taste, while the third option is short\n> >>>>>>> but uses reset() which sounds a bit strange for an assignment. Do you\n> >>>>>>> have any advice ?\n> >>>>>> \n> >>>>>> Before we have C++11 in Chromium we also had a base::MakeUnique<>\n> >>>>>> template in the Chromium libbase handling creation of unique pointers.\n> >>>>>> If we have to stick with C++11 then we might consider doing the same,\n> >>>>>> probably with a utils:: namespace. Hacking the std:: namespace is\n> >>>>>> indeed a bad idea an can cause build errors.\n> >>>>>> \n> >>>>>> Semantically I'd say [2] is more accurate than [3], but I don't really\n> >>>>>> have strong opinions between these two. If we don't want to define our\n> >>>>>> own make_unique template then we can use either.\n> >>>>> \n> >>>>> I'm tempted to add our own make_unique implementation then, most likely in the\n> >>>>> libcamera:: or libcamera::utils:: namespace though.\n> >>>> \n> >>>> Sounds good!\n> >>>> \n> >>>>> \n> >>>>>> Do we restrict ourselves in C++11 for compatibility reason?\n> >>>>> \n> >>>>> That was the rationale, but it could be reconsidered if needed.\n> >>>> \n> >>>> I suppose C++11 shall be sufficient. We can indeed reconsider if we\n> >>>> have strong use cases for newer standards one day.\n> >> \n> >> Implementing make_unique or anything already defined in the newer standards need\n> >> be done with extra care. Users would expect it to work in the same way, so any\n> >> inconsistency with the standards might be a hidden trap. Making things forward\n> >> compatible is not an easy task. For example, the implementation below does not\n> >> provide the make_unique(std::size_t) overload defined in C++14.\n> >> \n> >> I'd suggest not to reinvent the wheel if there is no strong objection. Is it\n> >> possible to bump the C++ version to C++14/17? We can still limit the scope of\n> >> new language features as we did in [1], so we can opt-in the features gradually.\n> > \n> > This worries me for two reasons. The first one is that depending on\n> > C++14/C++17 lock us out of platform that don't support those newer\n> > standards. This is more of a general concern, I don't have any data to\n> > tell whether this is a valid concern or not.\n> \n> Yes it's hard to make decision without having the concrete target platforms. The\n> compiler support for C++14 are complete and stable enough in GCC/Clang, and\n> it's the default C++ standard version since GCC 5 and Clang 6. But some\n> popularish Linux distros might ship with older compilers that does not support\n> C++14 well, such as Ubuntu 14.04 and RHEL 6 although they are approaching EOL.\n\nI expect we'll be able to upgrade to C++14, but let's delay that until\nnecessary.\n\n> > The second concern is that we may start using features of C++14/C++17\n> > without noticing, possibly creating problems later if we want to support\n> > C++11-based systems.\n> \n> This is actually a good point. Note that the compiler flag \"-std=c++XX\" might\n> not strictly align with the standard. We are already using at least one C++17\n> feature \"nested namespace\" in utils.h, which uses \"namespace libcamera::utils\"\n> instead of \"namespace libcamera { namespace utils\". I found this because it fail\n> to compile on one of my machine (Ubuntu 17.10).\n\nThank you for reporting this. I'll fix it.\n\n> >> Another possibility is adopting some existing library such as abseil [2] which\n> >> includes make_unique and many other goodies for projects in C++11.\n> >> \n> >> [1] http://www.libcamera.org/coding-style.html#c-specific-rules\n> >> [2] https://abseil.io/\n> > \n> > Thank you for the pointer, that's an interesting library. However, at\n> > this point, I would like to avoid adding an external dependency just to\n> > provide make_unique support. I'm thus tempted to keep our in-house\n> > implementation (which by the way is proposed by\n> > https://en.cppreference.com/w/cpp/memory/unique_ptr/make_unique), and\n> > revisit this topic if/when we will need more feature of newer C++\n> > versions. Does this sound acceptable to you ?\n> \n> It's reasonable that not to add an external dependency just for make_unique. We\n> could revisit this topic as we need it.\n> \n> >>>>>>>> if (!enumerator->init())\n> >>>>>>>> return enumerator;\n> >>>>>>>> \n> >>>>>>>> - delete enumerator;\n> >>>>>>>> -\n> >>>>>>>> /*\n> >>>>>>>> * Either udev is not available or udev initialization failed.\n> >>>>>>>> Fall back\n> >>>>>>>> * on the sysfs enumerator.\n> > \n> > [snip]","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 891FE60B21\n\tfor ;\n\tFri, 18 Jan 2019 13:57:34 +0100 (CET)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9B22D53E;\n\tFri, 18 Jan 2019 13:57:33 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1547816253;\n\tbh=USrSOfqEaNUwG+B+5q+LuKvux8xUdC6VmPyb5bPm18k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IwFrPApOBQ85NcrtJG0Bc14Ja8nP2YMiAfbL3QmvMhW5HZENBBThqkT1e+ZoUTvIH\n\thKBfFyzLXCwKRBvmyOAzG3y1Ce09qV5XhUwg/h1efXfq6F6ljn2/GsSVVQxpX37Lfp\n\tblPA91XJCeMYaOsEfBBbJviRropQQLeMTEiQO560=","Date":"Fri, 18 Jan 2019 14:57:33 +0200","From":"Laurent Pinchart ","To":"Shik Chen ","Cc":"Shik Chen , Ricky Liang ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190118125733.GC5275@pendragon.ideasonboard.com>","References":"<20190107231151.23291-1-laurent.pinchart@ideasonboard.com>\n\t<1616763.ZV2yHh8qhD@avalon>\n\t\n\t<3011091.8Ud1DvGsir@avalon>\n\t\n\t\n\t\n\t<20190118003014.GI23244@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 04/11] libcamera: camera_manager:\n\tMake the class a singleton","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 18 Jan 2019 12:57:34 -0000"}}]