[{"id":410,"web_url":"https://patchwork.libcamera.org/comment/410/","msgid":"<20190118153956.naej4db7jcadclu5@uno.localdomain>","date":"2019-01-18T15:39:56","subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: camera: Handle camera\n\tobjects through shared pointers","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n thanks, very nice :)\n\nOn Fri, Jan 18, 2019 at 01:59:16AM +0200, Laurent Pinchart wrote:\n> The Camera class is explicitly reference-counted to manage the lifetime\n> of camera objects. Replace this open-coded implementation with usage of\n> the std::shared_ptr<> class.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> include/libcamera/camera.h | 13 +++++----\n> include/libcamera/camera_manager.h | 8 +++---\n> src/libcamera/camera.cpp | 46 +++++++++++++++++-------------\n> src/libcamera/camera_manager.cpp | 20 ++++++-------\n> src/libcamera/pipeline/vimc.cpp | 2 +-\n> test/list-cameras.cpp | 2 +-\n> 6 files changed, 50 insertions(+), 41 deletions(-)\n>\n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index 9a7579d61fa3..ef0a794e3a82 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -7,6 +7,7 @@\n> #ifndef __LIBCAMERA_CAMERA_H__\n> #define __LIBCAMERA_CAMERA_H__\n>\n> +#include \n> #include \n>\n> namespace libcamera {\n> @@ -14,15 +15,17 @@ namespace libcamera {\n> class Camera\n\nShould we make this 'final', it cannot be extended anyhow, let's make\nthis a clear design choice.\n\n> {\n> public:\n> -\tCamera(const std::string &name);\n> +\tstatic std::shared_ptr create(const std::string &name);\n> +\n> +\tCamera(const Camera &) = delete;\n> +\tvoid operator=(const Camera &) = delete;\n>\n> \tconst std::string &name() const;\n> -\tvoid get();\n> -\tvoid put();\n>\n> private:\n> -\tvirtual ~Camera() { };\n> -\tint ref_;\n> +\tCamera(const std::string &name);\n> +\t~Camera();\n> +\n> \tstd::string name_;\n> };\n>\n> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h\n> index 4b941fd9183b..738b13f4cfb1 100644\n> --- a/include/libcamera/camera_manager.h\n> +++ b/include/libcamera/camera_manager.h\n> @@ -24,10 +24,10 @@ public:\n> \tint start();\n> \tvoid stop();\n>\n> -\tconst std::vector &cameras() const { return cameras_; }\n> -\tCamera *get(const std::string &name);\n> +\tconst std::vector> &cameras() const { return cameras_; }\n> +\tstd::shared_ptr get(const std::string &name);\n\nThis return by value guarantees that even if the manager dies, cameras \"got()\"\nby the applications will stay around as long as application will be alive,\nright?\n\n>\n> -\tvoid addCamera(Camera *camera);\n> +\tvoid addCamera(std::shared_ptr &camera);\n>\n> \tstatic CameraManager *instance();\n>\n> @@ -42,7 +42,7 @@ private:\n>\n> \tstd::unique_ptr enumerator_;\n> \tstd::vector pipes_;\n> -\tstd::vector cameras_;\n> +\tstd::vector> cameras_;\n>\n> \tstd::unique_ptr dispatcher_;\n> };\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 6da2b20137d4..acf912bee95c 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -41,19 +41,36 @@ namespace libcamera {\n> * streams from a single image source. It provides the main interface to\n> * configuring and controlling the device, and capturing image streams. It is\n> * the central object exposed by libcamera.\n> + *\n> + * To support the central nature of Camera objects, libcamera manages the\n> + * lifetime of camera instances with std::shared_ptr<>. Instances shall be\n> + * created with the create() function which returns a shared pointer. The\n> + * Camera constructors and destructor are private, to prevent instances from\n> + * being constructed and destroyed manually.\n> */\n>\n> /**\n> - * \\brief Construct a named camera device\n> + * \\brief Create a camera instance\n> + * \\param[in] name The name of the camera device\n> *\n> - * \\param[in] name The name to set on the camera device\n> + * The caller is responsible for guaranteeing unicity of the camera name.\n> *\n> - * The caller is responsible for guaranteeing unicity of the camera\n> - * device name.\n> + * \\return A shared pointer to the newly created camera object\n> */\n> -Camera::Camera(const std::string &name)\n> -\t: ref_(1), name_(name)\n> +std::shared_ptr Camera::create(const std::string &name)\n> {\n> +\tstruct Allocator : std::allocator {\n\nout of curiosity, why a struct and not a class?\n\n> +\t\tvoid construct(void *p, const std::string &name)\n> +\t\t{\n> +\t\t\t::new(p) Camera(name);\n\nThis \"new(p)\" scares me :)\n\n> +\t\t}\n> +\t\tvoid destroy(Camera *p)\n> +\t\t{\n> +\t\t\tp->~Camera();\n> +\t\t}\n> +\t};\n> +\n> +\treturn std::allocate_shared(Allocator(), name);\n> }\n>\n> /**\n> @@ -66,24 +83,13 @@ const std::string &Camera::name() const\n> \treturn name_;\n> }\n>\n> -/**\n> - * \\brief Acquire a reference to the camera\n> - */\n> -void Camera::get()\n> +Camera::Camera(const std::string &name)\n> +\t: name_(name)\n> {\n> -\tref_++;\n> }\n>\n> -/**\n> - * \\brief Release a reference to the camera\n> - *\n> - * When the last reference is released the camera device is deleted. Callers\n> - * shall not access the camera device after calling this function.\n> - */\n> -void Camera::put()\n> +Camera::~Camera()\n> {\n> -\tif (--ref_ == 0)\n> -\t\tdelete this;\n> }\n>\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> index 852e5ed70fe3..d12bd42001ae 100644\n> --- a/src/libcamera/camera_manager.cpp\n> +++ b/src/libcamera/camera_manager.cpp\n> @@ -39,9 +39,11 @@ namespace libcamera {\n> * This will enumerate all the cameras present in the system, which can then be\n> * listed with list() and retrieved with get().\n> *\n> - * Cameras are reference-counted, and shall be returned to the camera manager\n> - * with Camera::put() after being used. Once all cameras have been returned to\n> - * the manager, it can be stopped with stop().\n> + * Cameras are shared through std::shared_ptr<>, ensuring that a camera will\n> + * stay valid until the last reference is released without requiring any special\n> + * action from the application. Once the application has released all the\n> + * references it held to cameras, the camera manager can be stopped with\n> + * stop().\n> *\n> * \\todo Add ability to add and remove media devices based on hot-(un)plug\n> * events coming from the device enumerator.\n> @@ -147,15 +149,13 @@ void CameraManager::stop()\n> * \\param[in] name Name of camera to get\n> *\n> * Before calling this function the caller is responsible for ensuring that\n> - * the camera manger is running. A camera fetched this way shall be\n> - * released by the user with the put() method of the Camera object once\n> - * it is done using the camera.\n> + * the camera manger is running.\n> *\n> - * \\return Pointer to Camera object or nullptr if camera not found\n> + * \\return Shared pointer to Camera object or nullptr if camera not found\n> */\n> -Camera *CameraManager::get(const std::string &name)\n> +std::shared_ptr CameraManager::get(const std::string &name)\n> {\n> -\tfor (Camera *camera : cameras_) {\n> +\tfor (std::shared_ptr camera : cameras_) {\n> \t\tif (camera->name() == name)\n> \t\t\treturn camera;\n> \t}\n> @@ -171,7 +171,7 @@ Camera *CameraManager::get(const std::string &name)\n> * handle with the camera manager. Registered cameras are immediately made\n> * available to the system.\n> */\n> -void CameraManager::addCamera(Camera *camera)\n> +void CameraManager::addCamera(std::shared_ptr &camera)\n> {\n> \tcameras_.push_back(camera);\n> }\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index 8742e0bae9a8..306a5b85cd60 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -64,7 +64,7 @@ bool PipeHandlerVimc::match(CameraManager *manager, DeviceEnumerator *enumerator\n> \t * will be chosen depends on how the Camera\n> \t * object is modeled.\n> \t */\n> -\tCamera *camera = new Camera(\"Dummy VIMC Camera\");\n> +\tstd::shared_ptr camera = Camera::create(\"Dummy VIMC Camera\");\n> \tmanager->addCamera(camera);\n>\n> \treturn true;\n> diff --git a/test/list-cameras.cpp b/test/list-cameras.cpp\n> index fdbbda0957b2..070cbf2be977 100644\n> --- a/test/list-cameras.cpp\n> +++ b/test/list-cameras.cpp\n> @@ -30,7 +30,7 @@ protected:\n> \t{\n> \t\tunsigned int count = 0;\n>\n> -\t\tfor (Camera *camera : cm->cameras()) {\n> +\t\tfor (const std::shared_ptr &camera : cm->cameras()) {\n> \t\t\tcout << \"- \" << camera->name() << endl;\n> \t\t\tcount++;\n> \t\t}\n\nThese are just minor comments, documentation apart, feel free to add\nmy tag to the whole series :)\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\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 relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EB39960B21\n\tfor ;\n\tFri, 18 Jan 2019 16:39:46 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 7B6BFFF807;\n\tFri, 18 Jan 2019 15:39:46 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Fri, 18 Jan 2019 16:39:56 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190118153956.naej4db7jcadclu5@uno.localdomain>","References":"<20190117235916.1906-1-laurent.pinchart@ideasonboard.com>\n\t<20190117235916.1906-5-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"46msw4wc66xhbcek\"","Content-Disposition":"inline","In-Reply-To":"<20190117235916.1906-5-laurent.pinchart@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: camera: Handle camera\n\tobjects through shared pointers","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 15:39:47 -0000"}},{"id":417,"web_url":"https://patchwork.libcamera.org/comment/417/","msgid":"<20190118164722.GK5275@pendragon.ideasonboard.com>","date":"2019-01-18T16:47:22","subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: camera: Handle camera\n\tobjects through shared pointers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, Jan 18, 2019 at 04:39:56PM +0100, Jacopo Mondi wrote:\n> On Fri, Jan 18, 2019 at 01:59:16AM +0200, Laurent Pinchart wrote:\n> > The Camera class is explicitly reference-counted to manage the lifetime\n> > of camera objects. Replace this open-coded implementation with usage of\n> > the std::shared_ptr<> class.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > include/libcamera/camera.h | 13 +++++----\n> > include/libcamera/camera_manager.h | 8 +++---\n> > src/libcamera/camera.cpp | 46 +++++++++++++++++-------------\n> > src/libcamera/camera_manager.cpp | 20 ++++++-------\n> > src/libcamera/pipeline/vimc.cpp | 2 +-\n> > test/list-cameras.cpp | 2 +-\n> > 6 files changed, 50 insertions(+), 41 deletions(-)\n> >\n> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > index 9a7579d61fa3..ef0a794e3a82 100644\n> > --- a/include/libcamera/camera.h\n> > +++ b/include/libcamera/camera.h\n> > @@ -7,6 +7,7 @@\n> > #ifndef __LIBCAMERA_CAMERA_H__\n> > #define __LIBCAMERA_CAMERA_H__\n> >\n> > +#include \n> > #include \n> >\n> > namespace libcamera {\n> > @@ -14,15 +15,17 @@ namespace libcamera {\n> > class Camera\n> \n> Should we make this 'final', it cannot be extended anyhow, let's make\n> this a clear design choice.\n\nGood point, I'll do that.\n\n> > {\n> > public:\n> > -\tCamera(const std::string &name);\n> > +\tstatic std::shared_ptr create(const std::string &name);\n> > +\n> > +\tCamera(const Camera &) = delete;\n> > +\tvoid operator=(const Camera &) = delete;\n> >\n> > \tconst std::string &name() const;\n> > -\tvoid get();\n> > -\tvoid put();\n> >\n> > private:\n> > -\tvirtual ~Camera() { };\n> > -\tint ref_;\n> > +\tCamera(const std::string &name);\n> > +\t~Camera();\n> > +\n> > \tstd::string name_;\n> > };\n> >\n> > diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h\n> > index 4b941fd9183b..738b13f4cfb1 100644\n> > --- a/include/libcamera/camera_manager.h\n> > +++ b/include/libcamera/camera_manager.h\n> > @@ -24,10 +24,10 @@ public:\n> > \tint start();\n> > \tvoid stop();\n> >\n> > -\tconst std::vector &cameras() const { return cameras_; }\n> > -\tCamera *get(const std::string &name);\n> > +\tconst std::vector> &cameras() const { return cameras_; }\n> > +\tstd::shared_ptr get(const std::string &name);\n> \n> This return by value guarantees that even if the manager dies, cameras \"got()\"\n> by the applications will stay around as long as application will be alive,\n> right?\n\nCorrect, it returns a new shared reference, so the camera instance won't\ndisappear.\n\n> >\n> > -\tvoid addCamera(Camera *camera);\n> > +\tvoid addCamera(std::shared_ptr &camera);\n> >\n> > \tstatic CameraManager *instance();\n> >\n> > @@ -42,7 +42,7 @@ private:\n> >\n> > \tstd::unique_ptr enumerator_;\n> > \tstd::vector pipes_;\n> > -\tstd::vector cameras_;\n> > +\tstd::vector> cameras_;\n> >\n> > \tstd::unique_ptr dispatcher_;\n> > };\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 6da2b20137d4..acf912bee95c 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -41,19 +41,36 @@ namespace libcamera {\n> > * streams from a single image source. It provides the main interface to\n> > * configuring and controlling the device, and capturing image streams. It is\n> > * the central object exposed by libcamera.\n> > + *\n> > + * To support the central nature of Camera objects, libcamera manages the\n> > + * lifetime of camera instances with std::shared_ptr<>. Instances shall be\n> > + * created with the create() function which returns a shared pointer. The\n> > + * Camera constructors and destructor are private, to prevent instances from\n> > + * being constructed and destroyed manually.\n> > */\n> >\n> > /**\n> > - * \\brief Construct a named camera device\n> > + * \\brief Create a camera instance\n> > + * \\param[in] name The name of the camera device\n> > *\n> > - * \\param[in] name The name to set on the camera device\n> > + * The caller is responsible for guaranteeing unicity of the camera name.\n> > *\n> > - * The caller is responsible for guaranteeing unicity of the camera\n> > - * device name.\n> > + * \\return A shared pointer to the newly created camera object\n> > */\n> > -Camera::Camera(const std::string &name)\n> > -\t: ref_(1), name_(name)\n> > +std::shared_ptr Camera::create(const std::string &name)\n> > {\n> > +\tstruct Allocator : std::allocator {\n> \n> out of curiosity, why a struct and not a class?\n\nNo reason, I think I found this in sample code somewhere. Should I make\nit a class ?\n\n> > +\t\tvoid construct(void *p, const std::string &name)\n> > +\t\t{\n> > +\t\t\t::new(p) Camera(name);\n> \n> This \"new(p)\" scares me :)\n\nShhhhhh... :-)\n\n> > +\t\t}\n> > +\t\tvoid destroy(Camera *p)\n> > +\t\t{\n> > +\t\t\tp->~Camera();\n> > +\t\t}\n> > +\t};\n> > +\n> > +\treturn std::allocate_shared(Allocator(), name);\n> > }\n> >\n> > /**\n> > @@ -66,24 +83,13 @@ const std::string &Camera::name() const\n> > \treturn name_;\n> > }\n> >\n> > -/**\n> > - * \\brief Acquire a reference to the camera\n> > - */\n> > -void Camera::get()\n> > +Camera::Camera(const std::string &name)\n> > +\t: name_(name)\n> > {\n> > -\tref_++;\n> > }\n> >\n> > -/**\n> > - * \\brief Release a reference to the camera\n> > - *\n> > - * When the last reference is released the camera device is deleted. Callers\n> > - * shall not access the camera device after calling this function.\n> > - */\n> > -void Camera::put()\n> > +Camera::~Camera()\n> > {\n> > -\tif (--ref_ == 0)\n> > -\t\tdelete this;\n> > }\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> > index 852e5ed70fe3..d12bd42001ae 100644\n> > --- a/src/libcamera/camera_manager.cpp\n> > +++ b/src/libcamera/camera_manager.cpp\n> > @@ -39,9 +39,11 @@ namespace libcamera {\n> > * This will enumerate all the cameras present in the system, which can then be\n> > * listed with list() and retrieved with get().\n> > *\n> > - * Cameras are reference-counted, and shall be returned to the camera manager\n> > - * with Camera::put() after being used. Once all cameras have been returned to\n> > - * the manager, it can be stopped with stop().\n> > + * Cameras are shared through std::shared_ptr<>, ensuring that a camera will\n> > + * stay valid until the last reference is released without requiring any special\n> > + * action from the application. Once the application has released all the\n> > + * references it held to cameras, the camera manager can be stopped with\n> > + * stop().\n> > *\n> > * \\todo Add ability to add and remove media devices based on hot-(un)plug\n> > * events coming from the device enumerator.\n> > @@ -147,15 +149,13 @@ void CameraManager::stop()\n> > * \\param[in] name Name of camera to get\n> > *\n> > * Before calling this function the caller is responsible for ensuring that\n> > - * the camera manger is running. A camera fetched this way shall be\n> > - * released by the user with the put() method of the Camera object once\n> > - * it is done using the camera.\n> > + * the camera manger is running.\n> > *\n> > - * \\return Pointer to Camera object or nullptr if camera not found\n> > + * \\return Shared pointer to Camera object or nullptr if camera not found\n> > */\n> > -Camera *CameraManager::get(const std::string &name)\n> > +std::shared_ptr CameraManager::get(const std::string &name)\n> > {\n> > -\tfor (Camera *camera : cameras_) {\n> > +\tfor (std::shared_ptr camera : cameras_) {\n> > \t\tif (camera->name() == name)\n> > \t\t\treturn camera;\n> > \t}\n> > @@ -171,7 +171,7 @@ Camera *CameraManager::get(const std::string &name)\n> > * handle with the camera manager. Registered cameras are immediately made\n> > * available to the system.\n> > */\n> > -void CameraManager::addCamera(Camera *camera)\n> > +void CameraManager::addCamera(std::shared_ptr &camera)\n> > {\n> > \tcameras_.push_back(camera);\n> > }\n> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> > index 8742e0bae9a8..306a5b85cd60 100644\n> > --- a/src/libcamera/pipeline/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc.cpp\n> > @@ -64,7 +64,7 @@ bool PipeHandlerVimc::match(CameraManager *manager, DeviceEnumerator *enumerator\n> > \t * will be chosen depends on how the Camera\n> > \t * object is modeled.\n> > \t */\n> > -\tCamera *camera = new Camera(\"Dummy VIMC Camera\");\n> > +\tstd::shared_ptr camera = Camera::create(\"Dummy VIMC Camera\");\n> > \tmanager->addCamera(camera);\n> >\n> > \treturn true;\n> > diff --git a/test/list-cameras.cpp b/test/list-cameras.cpp\n> > index fdbbda0957b2..070cbf2be977 100644\n> > --- a/test/list-cameras.cpp\n> > +++ b/test/list-cameras.cpp\n> > @@ -30,7 +30,7 @@ protected:\n> > \t{\n> > \t\tunsigned int count = 0;\n> >\n> > -\t\tfor (Camera *camera : cm->cameras()) {\n> > +\t\tfor (const std::shared_ptr &camera : cm->cameras()) {\n> > \t\t\tcout << \"- \" << camera->name() << endl;\n> > \t\t\tcount++;\n> > \t\t}\n> \n> These are just minor comments, documentation apart, feel free to add\n> my tag to the whole series :)\n> Reviewed-by: Jacopo Mondi ","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 1D7BC60B21\n\tfor ;\n\tFri, 18 Jan 2019 17:47:24 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6CFBD53E;\n\tFri, 18 Jan 2019 17:47:22 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1547830042;\n\tbh=PVSYvZwd9NdzdvvGeY6raFVXxS3df0nPGU8e00Yfs8w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mqLOZNhCyFR1HmtOUsMyce/mtG6YMYPJ/5pdj1yt/vN1jINWvouust8uRbopcJvkV\n\t4xjZROyxilvtp4mO7k5yj/RM/CvZ4Q7T7Z3raPzY5UDQp7WIJNrPtCzV7NGLwtAs9K\n\tBHtPV77Ld0NEaVaQgeUW/M4FG4SRSxl+Nzc7/czE=","Date":"Fri, 18 Jan 2019 18:47:22 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190118164722.GK5275@pendragon.ideasonboard.com>","References":"<20190117235916.1906-1-laurent.pinchart@ideasonboard.com>\n\t<20190117235916.1906-5-laurent.pinchart@ideasonboard.com>\n\t<20190118153956.naej4db7jcadclu5@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190118153956.naej4db7jcadclu5@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: camera: Handle camera\n\tobjects through shared pointers","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 16:47:24 -0000"}},{"id":419,"web_url":"https://patchwork.libcamera.org/comment/419/","msgid":"<20190118165002.GW6484@bigcity.dyn.berto.se>","date":"2019-01-18T16:50:02","subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: camera: Handle camera\n\tobjects through shared pointers","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your work.\n\nOn 2019-01-18 01:59:16 +0200, Laurent Pinchart wrote:\n> The Camera class is explicitly reference-counted to manage the lifetime\n> of camera objects. Replace this open-coded implementation with usage of\n> the std::shared_ptr<> class.\n> \n> Signed-off-by: Laurent Pinchart \n\nReviewed-by: Niklas Söderlund \n\n> ---\n> include/libcamera/camera.h | 13 +++++----\n> include/libcamera/camera_manager.h | 8 +++---\n> src/libcamera/camera.cpp | 46 +++++++++++++++++-------------\n> src/libcamera/camera_manager.cpp | 20 ++++++-------\n> src/libcamera/pipeline/vimc.cpp | 2 +-\n> test/list-cameras.cpp | 2 +-\n> 6 files changed, 50 insertions(+), 41 deletions(-)\n> \n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index 9a7579d61fa3..ef0a794e3a82 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -7,6 +7,7 @@\n> #ifndef __LIBCAMERA_CAMERA_H__\n> #define __LIBCAMERA_CAMERA_H__\n> \n> +#include \n> #include \n> \n> namespace libcamera {\n> @@ -14,15 +15,17 @@ namespace libcamera {\n> class Camera\n> {\n> public:\n> -\tCamera(const std::string &name);\n> +\tstatic std::shared_ptr create(const std::string &name);\n> +\n> +\tCamera(const Camera &) = delete;\n> +\tvoid operator=(const Camera &) = delete;\n> \n> \tconst std::string &name() const;\n> -\tvoid get();\n> -\tvoid put();\n> \n> private:\n> -\tvirtual ~Camera() { };\n> -\tint ref_;\n> +\tCamera(const std::string &name);\n> +\t~Camera();\n> +\n> \tstd::string name_;\n> };\n> \n> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h\n> index 4b941fd9183b..738b13f4cfb1 100644\n> --- a/include/libcamera/camera_manager.h\n> +++ b/include/libcamera/camera_manager.h\n> @@ -24,10 +24,10 @@ public:\n> \tint start();\n> \tvoid stop();\n> \n> -\tconst std::vector &cameras() const { return cameras_; }\n> -\tCamera *get(const std::string &name);\n> +\tconst std::vector> &cameras() const { return cameras_; }\n> +\tstd::shared_ptr get(const std::string &name);\n> \n> -\tvoid addCamera(Camera *camera);\n> +\tvoid addCamera(std::shared_ptr &camera);\n> \n> \tstatic CameraManager *instance();\n> \n> @@ -42,7 +42,7 @@ private:\n> \n> \tstd::unique_ptr enumerator_;\n> \tstd::vector pipes_;\n> -\tstd::vector cameras_;\n> +\tstd::vector> cameras_;\n> \n> \tstd::unique_ptr dispatcher_;\n> };\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 6da2b20137d4..acf912bee95c 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -41,19 +41,36 @@ namespace libcamera {\n> * streams from a single image source. It provides the main interface to\n> * configuring and controlling the device, and capturing image streams. It is\n> * the central object exposed by libcamera.\n> + *\n> + * To support the central nature of Camera objects, libcamera manages the\n> + * lifetime of camera instances with std::shared_ptr<>. Instances shall be\n> + * created with the create() function which returns a shared pointer. The\n> + * Camera constructors and destructor are private, to prevent instances from\n> + * being constructed and destroyed manually.\n> */\n> \n> /**\n> - * \\brief Construct a named camera device\n> + * \\brief Create a camera instance\n> + * \\param[in] name The name of the camera device\n> *\n> - * \\param[in] name The name to set on the camera device\n> + * The caller is responsible for guaranteeing unicity of the camera name.\n> *\n> - * The caller is responsible for guaranteeing unicity of the camera\n> - * device name.\n> + * \\return A shared pointer to the newly created camera object\n> */\n> -Camera::Camera(const std::string &name)\n> -\t: ref_(1), name_(name)\n> +std::shared_ptr Camera::create(const std::string &name)\n> {\n> +\tstruct Allocator : std::allocator {\n> +\t\tvoid construct(void *p, const std::string &name)\n> +\t\t{\n> +\t\t\t::new(p) Camera(name);\n> +\t\t}\n> +\t\tvoid destroy(Camera *p)\n> +\t\t{\n> +\t\t\tp->~Camera();\n> +\t\t}\n> +\t};\n> +\n> +\treturn std::allocate_shared(Allocator(), name);\n> }\n> \n> /**\n> @@ -66,24 +83,13 @@ const std::string &Camera::name() const\n> \treturn name_;\n> }\n> \n> -/**\n> - * \\brief Acquire a reference to the camera\n> - */\n> -void Camera::get()\n> +Camera::Camera(const std::string &name)\n> +\t: name_(name)\n> {\n> -\tref_++;\n> }\n> \n> -/**\n> - * \\brief Release a reference to the camera\n> - *\n> - * When the last reference is released the camera device is deleted. Callers\n> - * shall not access the camera device after calling this function.\n> - */\n> -void Camera::put()\n> +Camera::~Camera()\n> {\n> -\tif (--ref_ == 0)\n> -\t\tdelete this;\n> }\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> index 852e5ed70fe3..d12bd42001ae 100644\n> --- a/src/libcamera/camera_manager.cpp\n> +++ b/src/libcamera/camera_manager.cpp\n> @@ -39,9 +39,11 @@ namespace libcamera {\n> * This will enumerate all the cameras present in the system, which can then be\n> * listed with list() and retrieved with get().\n> *\n> - * Cameras are reference-counted, and shall be returned to the camera manager\n> - * with Camera::put() after being used. Once all cameras have been returned to\n> - * the manager, it can be stopped with stop().\n> + * Cameras are shared through std::shared_ptr<>, ensuring that a camera will\n> + * stay valid until the last reference is released without requiring any special\n> + * action from the application. Once the application has released all the\n> + * references it held to cameras, the camera manager can be stopped with\n> + * stop().\n> *\n> * \\todo Add ability to add and remove media devices based on hot-(un)plug\n> * events coming from the device enumerator.\n> @@ -147,15 +149,13 @@ void CameraManager::stop()\n> * \\param[in] name Name of camera to get\n> *\n> * Before calling this function the caller is responsible for ensuring that\n> - * the camera manger is running. A camera fetched this way shall be\n> - * released by the user with the put() method of the Camera object once\n> - * it is done using the camera.\n> + * the camera manger is running.\n> *\n> - * \\return Pointer to Camera object or nullptr if camera not found\n> + * \\return Shared pointer to Camera object or nullptr if camera not found\n> */\n> -Camera *CameraManager::get(const std::string &name)\n> +std::shared_ptr CameraManager::get(const std::string &name)\n> {\n> -\tfor (Camera *camera : cameras_) {\n> +\tfor (std::shared_ptr camera : cameras_) {\n> \t\tif (camera->name() == name)\n> \t\t\treturn camera;\n> \t}\n> @@ -171,7 +171,7 @@ Camera *CameraManager::get(const std::string &name)\n> * handle with the camera manager. Registered cameras are immediately made\n> * available to the system.\n> */\n> -void CameraManager::addCamera(Camera *camera)\n> +void CameraManager::addCamera(std::shared_ptr &camera)\n> {\n> \tcameras_.push_back(camera);\n> }\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index 8742e0bae9a8..306a5b85cd60 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -64,7 +64,7 @@ bool PipeHandlerVimc::match(CameraManager *manager, DeviceEnumerator *enumerator\n> \t * will be chosen depends on how the Camera\n> \t * object is modeled.\n> \t */\n> -\tCamera *camera = new Camera(\"Dummy VIMC Camera\");\n> +\tstd::shared_ptr camera = Camera::create(\"Dummy VIMC Camera\");\n> \tmanager->addCamera(camera);\n> \n> \treturn true;\n> diff --git a/test/list-cameras.cpp b/test/list-cameras.cpp\n> index fdbbda0957b2..070cbf2be977 100644\n> --- a/test/list-cameras.cpp\n> +++ b/test/list-cameras.cpp\n> @@ -30,7 +30,7 @@ protected:\n> \t{\n> \t\tunsigned int count = 0;\n> \n> -\t\tfor (Camera *camera : cm->cameras()) {\n> +\t\tfor (const std::shared_ptr &camera : cm->cameras()) {\n> \t\t\tcout << \"- \" << camera->name() << endl;\n> \t\t\tcount++;\n> \t\t}\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-lj1-f179.google.com (mail-lj1-f179.google.com\n\t[209.85.208.179])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 869B260B21\n\tfor ;\n\tFri, 18 Jan 2019 17:51:05 +0100 (CET)","by mail-lj1-f179.google.com with SMTP id s5-v6so12135734ljd.12\n\tfor ;\n\tFri, 18 Jan 2019 08:51:05 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tp10-v6sm806136ljg.19.2019.01.18.08.50.02\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tFri, 18 Jan 2019 08:50:03 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=XsXL/J8+FIDGjuJBqlR5z3x8bPjNTeWhXKuSkaJdszg=;\n\tb=Cj9O45WeOtog49GdPjIFYtg8rx49i7NvwmqL7D37H1FPyHlBeZwg/1WjhEHGBOdqTr\n\tYy1a9Vc6Vv7T4cqmmXLQHtGDtDaEtFkPm61/vBOeZF8m3aAbODDrfawI6qT331JXHyfN\n\tMtD+cWQ6pJrti1Xxg7KzMIT7SRQRlIimDWPVKXyu+M43lTz7RuxhhyDE2hhtR0ADGTsg\n\tB1T75kquGfM/4zwrphA8ATSI1FUQopBwmodHXUZjkyDXPFecJQhNJ2M9VtURridQpvNE\n\tQbEKUcrUL4RK4/M4FLb5iJtO5L/7gjpVQscA4S4essD2sjSmEMzvz44VvPnf8CyvnqG6\n\tPCIw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=XsXL/J8+FIDGjuJBqlR5z3x8bPjNTeWhXKuSkaJdszg=;\n\tb=lZhqsX1D1uJrhJoH1U4dJq6Jh0rGm/v3MhNjcX7DTKWEAhb+iPKta0F688Lqdy1hIP\n\t167GpjCP3+6yzLL4csmblqROpayzUCqyAk5aTofOoEoIt7a7CMhWJRxMYbz24SLqkhyH\n\t7Iuj9pJE7fzDXHOjL4A+nIIzg5St9iFYIpeMCtkw71b7bPjumYIHt5rNAg869WcrvpnV\n\tCGtd2RwlgtnXjPH3jSaulvuRDi+LI8LM1kWFJ2GLNKsimbTakZSwX7r6fjxRWNkbXYuQ\n\tny1kCgGh1D0iGhouZN1w54CECTtLL6lVPNb++M4zP89WdONOUfQixQF1xEOZL0USQzDs\n\tD7fg==","X-Gm-Message-State":"AJcUukfxQw7SqEM3urhYq4z5vyyf2Gv3z4p6ZsEmOiwNrKAAQVkGYfzV\n\tddcTLxuKHmJwK4a+sPPu38rCd+ubWoU=","X-Google-Smtp-Source":"ALg8bN56IlSiLGdcHK+NxGYF1PJuIUEBRAf5AlpVmKbfTXvLtT57JZjpG1ju0CExMFPLbJykk+AIoQ==","X-Received":"by 2002:a2e:3012:: with SMTP id\n\tw18-v6mr12112394ljw.75.1547830203560; \n\tFri, 18 Jan 2019 08:50:03 -0800 (PST)","Date":"Fri, 18 Jan 2019 17:50:02 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190118165002.GW6484@bigcity.dyn.berto.se>","References":"<20190117235916.1906-1-laurent.pinchart@ideasonboard.com>\n\t<20190117235916.1906-5-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190117235916.1906-5-laurent.pinchart@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: camera: Handle camera\n\tobjects through shared pointers","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 16:51:05 -0000"}},{"id":421,"web_url":"https://patchwork.libcamera.org/comment/421/","msgid":"<20190118170551.md2rubbsdmhirblm@uno.localdomain>","date":"2019-01-18T17:05:51","subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: camera: Handle camera\n\tobjects through shared pointers","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Fri, Jan 18, 2019 at 06:47:22PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Fri, Jan 18, 2019 at 04:39:56PM +0100, Jacopo Mondi wrote:\n\n[snip]\n\n> > > +\tstruct Allocator : std::allocator {\n> >\n> > out of curiosity, why a struct and not a class?\n>\n> No reason, I think I found this in sample code somewhere. Should I make\n> it a class ?\n>\n\nNot really if there is not requirement to do so.\n\n\n> > > +\t\tvoid construct(void *p, const std::string &name)\n> > > +\t\t{\n> > > +\t\t\t::new(p) Camera(name);\n> >\n> > This \"new(p)\" scares me :)\n>\n> Shhhhhh... :-)\n>\n\nThere will be one day where I could say \"I finally got C++\" and not be\nprove wrong a few minutes after.\n\nThanks\n j","headers":{"Return-Path":"","Received":["from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B521760B21\n\tfor ;\n\tFri, 18 Jan 2019 18:05:41 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 39225240012;\n\tFri, 18 Jan 2019 17:05:41 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Fri, 18 Jan 2019 18:05:51 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190118170551.md2rubbsdmhirblm@uno.localdomain>","References":"<20190117235916.1906-1-laurent.pinchart@ideasonboard.com>\n\t<20190117235916.1906-5-laurent.pinchart@ideasonboard.com>\n\t<20190118153956.naej4db7jcadclu5@uno.localdomain>\n\t<20190118164722.GK5275@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"r47o36nv2sgdpqod\"","Content-Disposition":"inline","In-Reply-To":"<20190118164722.GK5275@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: camera: Handle camera\n\tobjects through shared pointers","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 17:05:41 -0000"}},{"id":422,"web_url":"https://patchwork.libcamera.org/comment/422/","msgid":"","date":"2019-01-18T17:39:35","subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: camera: Handle camera\n\tobjects through shared pointers","submitter":{"id":6,"url":"https://patchwork.libcamera.org/api/people/6/","name":"Ricky Liang","email":"jcliang@chromium.org"},"content":"Hi Laurent,\n\nOn Fri, Jan 18, 2019 at 7:59 AM Laurent Pinchart\n wrote:\n>\n> The Camera class is explicitly reference-counted to manage the lifetime\n> of camera objects. Replace this open-coded implementation with usage of\n> the std::shared_ptr<> class.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> include/libcamera/camera.h | 13 +++++----\n> include/libcamera/camera_manager.h | 8 +++---\n> src/libcamera/camera.cpp | 46 +++++++++++++++++-------------\n> src/libcamera/camera_manager.cpp | 20 ++++++-------\n> src/libcamera/pipeline/vimc.cpp | 2 +-\n> test/list-cameras.cpp | 2 +-\n> 6 files changed, 50 insertions(+), 41 deletions(-)\n>\n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index 9a7579d61fa3..ef0a794e3a82 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -7,6 +7,7 @@\n> #ifndef __LIBCAMERA_CAMERA_H__\n> #define __LIBCAMERA_CAMERA_H__\n>\n> +#include \n> #include \n>\n> namespace libcamera {\n> @@ -14,15 +15,17 @@ namespace libcamera {\n> class Camera\n> {\n> public:\n> - Camera(const std::string &name);\n> + static std::shared_ptr create(const std::string &name);\n> +\n> + Camera(const Camera &) = delete;\n> + void operator=(const Camera &) = delete;\n\nWe probably want to delete the implicit constructor as well:\n\n Camera() = delete;\n\n>\n> const std::string &name() const;\n> - void get();\n> - void put();\n>\n> private:\n> - virtual ~Camera() { };\n> - int ref_;\n> + Camera(const std::string &name);\n\nCan we add the 'explicit' specifier for constructor with only one\nargument? Not super important for this patch as it's a private\nconstructor here, but may be a good general guideline.\n\n> + ~Camera();\n> +\n> std::string name_;\n> };\n>\n> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h\n> index 4b941fd9183b..738b13f4cfb1 100644\n> --- a/include/libcamera/camera_manager.h\n> +++ b/include/libcamera/camera_manager.h\n> @@ -24,10 +24,10 @@ public:\n> int start();\n> void stop();\n>\n> - const std::vector &cameras() const { return cameras_; }\n> - Camera *get(const std::string &name);\n> + const std::vector> &cameras() const { return cameras_; }\n> + std::shared_ptr get(const std::string &name);\n\nSome high level design questions:\n\n1. Who are the callers of these getter methods?\n2. What's the threading model that we plan to use for exercising the\nCamera objects?\n\nWhile making Camera objects as shared pointers makes the coding\neasier, it may also be hard to track the ownership of the objects once\nthe code base gets large. In some cases it may lead to memory/fd leaks\nthat are hard to debug, e.g. a singleton object holding a shared\npointer reference to an object and never releases the reference.\n\nI tend to agree that fundamental objects like Camera has the necessity\nto be shared pointers, but would also like to know if it's possible to\ndesign the threading model such that CameraManager is the main owner\nof the Camera objects. For example, if we can design it such that all\nthe access to Camera objects is serialized on a thread, then we\nprobably don't need to use shared pointers. Or, we can achieve\nconcurrency by storing Camera objects as std::shared_ptr in\nCameraManager and only giving out std::weak_ptr of the Camera objects.\n\n>\n> - void addCamera(Camera *camera);\n> + void addCamera(std::shared_ptr &camera);\n\nWe can pass by value here so that we provide the caller with the\nfreedom to pass its own shared ownership to the callee through\nstd::move().\n\n>\n> static CameraManager *instance();\n>\n> @@ -42,7 +42,7 @@ private:\n>\n> std::unique_ptr enumerator_;\n> std::vector pipes_;\n> - std::vector cameras_;\n> + std::vector> cameras_;\n\nIf the camera name is required to be unique then perhaps we can store\nthe cameras as\n\n std::unordered_map> cameras_;\n\n>\n> std::unique_ptr dispatcher_;\n> };\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 6da2b20137d4..acf912bee95c 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -41,19 +41,36 @@ namespace libcamera {\n> * streams from a single image source. It provides the main interface to\n> * configuring and controlling the device, and capturing image streams. It is\n> * the central object exposed by libcamera.\n> + *\n> + * To support the central nature of Camera objects, libcamera manages the\n> + * lifetime of camera instances with std::shared_ptr<>. Instances shall be\n> + * created with the create() function which returns a shared pointer. The\n> + * Camera constructors and destructor are private, to prevent instances from\n> + * being constructed and destroyed manually.\n> */\n>\n> /**\n> - * \\brief Construct a named camera device\n> + * \\brief Create a camera instance\n> + * \\param[in] name The name of the camera device\n> *\n> - * \\param[in] name The name to set on the camera device\n> + * The caller is responsible for guaranteeing unicity of the camera name.\n> *\n> - * The caller is responsible for guaranteeing unicity of the camera\n> - * device name.\n> + * \\return A shared pointer to the newly created camera object\n> */\n> -Camera::Camera(const std::string &name)\n> - : ref_(1), name_(name)\n> +std::shared_ptr Camera::create(const std::string &name)\n> {\n> + struct Allocator : std::allocator {\n> + void construct(void *p, const std::string &name)\n> + {\n> + ::new(p) Camera(name);\n> + }\n> + void destroy(Camera *p)\n> + {\n> + p->~Camera();\n> + }\n> + };\n> +\n> + return std::allocate_shared(Allocator(), name);\n> }\n>\n> /**\n> @@ -66,24 +83,13 @@ const std::string &Camera::name() const\n> return name_;\n> }\n>\n> -/**\n> - * \\brief Acquire a reference to the camera\n> - */\n> -void Camera::get()\n> +Camera::Camera(const std::string &name)\n> + : name_(name)\n> {\n> - ref_++;\n> }\n>\n> -/**\n> - * \\brief Release a reference to the camera\n> - *\n> - * When the last reference is released the camera device is deleted. Callers\n> - * shall not access the camera device after calling this function.\n> - */\n> -void Camera::put()\n> +Camera::~Camera()\n> {\n> - if (--ref_ == 0)\n> - delete this;\n> }\n>\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> index 852e5ed70fe3..d12bd42001ae 100644\n> --- a/src/libcamera/camera_manager.cpp\n> +++ b/src/libcamera/camera_manager.cpp\n> @@ -39,9 +39,11 @@ namespace libcamera {\n> * This will enumerate all the cameras present in the system, which can then be\n> * listed with list() and retrieved with get().\n> *\n> - * Cameras are reference-counted, and shall be returned to the camera manager\n> - * with Camera::put() after being used. Once all cameras have been returned to\n> - * the manager, it can be stopped with stop().\n> + * Cameras are shared through std::shared_ptr<>, ensuring that a camera will\n> + * stay valid until the last reference is released without requiring any special\n> + * action from the application. Once the application has released all the\n> + * references it held to cameras, the camera manager can be stopped with\n> + * stop().\n> *\n> * \\todo Add ability to add and remove media devices based on hot-(un)plug\n> * events coming from the device enumerator.\n> @@ -147,15 +149,13 @@ void CameraManager::stop()\n> * \\param[in] name Name of camera to get\n> *\n> * Before calling this function the caller is responsible for ensuring that\n> - * the camera manger is running. A camera fetched this way shall be\n> - * released by the user with the put() method of the Camera object once\n> - * it is done using the camera.\n> + * the camera manger is running.\n> *\n> - * \\return Pointer to Camera object or nullptr if camera not found\n> + * \\return Shared pointer to Camera object or nullptr if camera not found\n> */\n> -Camera *CameraManager::get(const std::string &name)\n> +std::shared_ptr CameraManager::get(const std::string &name)\n> {\n> - for (Camera *camera : cameras_) {\n> + for (std::shared_ptr camera : cameras_) {\n> if (camera->name() == name)\n> return camera;\n> }\n> @@ -171,7 +171,7 @@ Camera *CameraManager::get(const std::string &name)\n> * handle with the camera manager. Registered cameras are immediately made\n> * available to the system.\n> */\n> -void CameraManager::addCamera(Camera *camera)\n> +void CameraManager::addCamera(std::shared_ptr &camera)\n> {\n> cameras_.push_back(camera);\n> }\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index 8742e0bae9a8..306a5b85cd60 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -64,7 +64,7 @@ bool PipeHandlerVimc::match(CameraManager *manager, DeviceEnumerator *enumerator\n> * will be chosen depends on how the Camera\n> * object is modeled.\n> */\n> - Camera *camera = new Camera(\"Dummy VIMC Camera\");\n> + std::shared_ptr camera = Camera::create(\"Dummy VIMC Camera\");\n> manager->addCamera(camera);\n>\n> return true;\n> diff --git a/test/list-cameras.cpp b/test/list-cameras.cpp\n> index fdbbda0957b2..070cbf2be977 100644\n> --- a/test/list-cameras.cpp\n> +++ b/test/list-cameras.cpp\n> @@ -30,7 +30,7 @@ protected:\n> {\n> unsigned int count = 0;\n>\n> - for (Camera *camera : cm->cameras()) {\n> + for (const std::shared_ptr &camera : cm->cameras()) {\n> cout << \"- \" << camera->name() << endl;\n> count++;\n> }\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-x42b.google.com (mail-wr1-x42b.google.com\n\t[IPv6:2a00:1450:4864:20::42b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5E42460B21\n\tfor ;\n\tFri, 18 Jan 2019 18:39:49 +0100 (CET)","by mail-wr1-x42b.google.com with SMTP id c14so16104104wrr.0\n\tfor ;\n\tFri, 18 Jan 2019 09:39:49 -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; bh=ojdJCmfQ5usPNzjlgSYtA3JPfZxJ8Lo47JaRq+bU3RM=;\n\tb=ArDNw3VELYeL8BgV15OxUOoYOUIFT+qSHYfvMPYn3o6se/2EPhHVl957hdaZXyYLxo\n\tRs3Ua9IhCZsXgFqy82cxe4HBhcQRCfhrrrWgbtCSxJjMDR+vFyznEf6FdhfPe10h/HtQ\n\tmE+gYU4X/R3Ef8/EflijK7gy0sH2jQM0ljvrw=","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;\n\tbh=ojdJCmfQ5usPNzjlgSYtA3JPfZxJ8Lo47JaRq+bU3RM=;\n\tb=GrEntX/QQSdXX6OJtkaPL6xSy5AEG2cx/TXBhtn/lj3mkFeB22p0rssERo5xDcD9pg\n\toMNF5Vrk6If38wImkG/k/iptU28KhjusGt/aScl66abnMVOetufW02BDNTfkH5rlCgG2\n\th7peliJvInxR0rOpTY0jQJPccEH3hOtFS6QAmF3g2zvhOYu9J+lMBJrktL2vCaeI4j/y\n\tXCMHOlFlVrB3YkE6iFiUCscbJUMF68LcomuOtgC/NercXOaHAJiPX1BPFkGNKifZvSs6\n\t2XkJSDp7GVaXLEpUxVatcUBNgMgSG8HhippJ5nku2uIGRRXjlV10TCoCF7mB9CyiWyo2\n\tATeA==","X-Gm-Message-State":"AJcUukcvYuadfyW7QASKKidJm8Nw7hZoOPfbsYcRVms39eSkiIV7+kbA\n\tEjefTZLixZCjqokjTX/jZZcEaSwcEMsbWhGR/cg8yw==","X-Google-Smtp-Source":"ALg8bN4mZVSj870/18RbSbT46+7wzanfPVi6d68ujsmTCUoK6l1vhwqcGvYj+lVubg5BbgdIO0W6/GMJgper+Y4YJCY=","X-Received":"by 2002:adf:c452:: with SMTP id\n\ta18mr17765682wrg.145.1547833188304; \n\tFri, 18 Jan 2019 09:39:48 -0800 (PST)","MIME-Version":"1.0","References":"<20190117235916.1906-1-laurent.pinchart@ideasonboard.com>\n\t<20190117235916.1906-5-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20190117235916.1906-5-laurent.pinchart@ideasonboard.com>","From":"Ricky Liang ","Date":"Sat, 19 Jan 2019 01:39:35 +0800","Message-ID":"","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: camera: Handle camera\n\tobjects through shared pointers","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 17:39:49 -0000"}},{"id":425,"web_url":"https://patchwork.libcamera.org/comment/425/","msgid":"<20190118225156.GB1799@pendragon.ideasonboard.com>","date":"2019-01-18T22:51:56","subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: camera: Handle camera\n\tobjects through shared pointers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Ricky,\n\nOn Sat, Jan 19, 2019 at 01:39:35AM +0800, Ricky Liang wrote:\n> On Fri, Jan 18, 2019 at 7:59 AM Laurent Pinchart\n> wrote:\n> >\n> > The Camera class is explicitly reference-counted to manage the lifetime\n> > of camera objects. Replace this open-coded implementation with usage of\n> > the std::shared_ptr<> class.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > include/libcamera/camera.h | 13 +++++----\n> > include/libcamera/camera_manager.h | 8 +++---\n> > src/libcamera/camera.cpp | 46 +++++++++++++++++-------------\n> > src/libcamera/camera_manager.cpp | 20 ++++++-------\n> > src/libcamera/pipeline/vimc.cpp | 2 +-\n> > test/list-cameras.cpp | 2 +-\n> > 6 files changed, 50 insertions(+), 41 deletions(-)\n> >\n> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > index 9a7579d61fa3..ef0a794e3a82 100644\n> > --- a/include/libcamera/camera.h\n> > +++ b/include/libcamera/camera.h\n> > @@ -7,6 +7,7 @@\n> > #ifndef __LIBCAMERA_CAMERA_H__\n> > #define __LIBCAMERA_CAMERA_H__\n> >\n> > +#include \n> > #include \n> >\n> > namespace libcamera {\n> > @@ -14,15 +15,17 @@ namespace libcamera {\n> > class Camera\n> > {\n> > public:\n> > - Camera(const std::string &name);\n> > + static std::shared_ptr create(const std::string &name);\n> > +\n> > + Camera(const Camera &) = delete;\n> > + void operator=(const Camera &) = delete;\n> \n> We probably want to delete the implicit constructor as well:\n> \n> Camera() = delete;\n\nThe compiler only creates an implicit constructor when no explicit\nconstructor is specified. As we define Camera(const std::string &name),\nthere's no need to delete the implicit constructor.\n\n> >\n> > const std::string &name() const;\n> > - void get();\n> > - void put();\n> >\n> > private:\n> > - virtual ~Camera() { };\n> > - int ref_;\n> > + Camera(const std::string &name);\n> \n> Can we add the 'explicit' specifier for constructor with only one\n> argument? Not super important for this patch as it's a private\n> constructor here, but may be a good general guideline.\n\nGood point, I'll do that.\n\n> > + ~Camera();\n> > +\n> > std::string name_;\n> > };\n> >\n> > diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h\n> > index 4b941fd9183b..738b13f4cfb1 100644\n> > --- a/include/libcamera/camera_manager.h\n> > +++ b/include/libcamera/camera_manager.h\n> > @@ -24,10 +24,10 @@ public:\n> > int start();\n> > void stop();\n> >\n> > - const std::vector &cameras() const { return cameras_; }\n> > - Camera *get(const std::string &name);\n> > + const std::vector> &cameras() const { return cameras_; }\n> > + std::shared_ptr get(const std::string &name);\n> \n> Some high level design questions:\n> \n> 1. Who are the callers of these getter methods?\n\nThese methods are meant to be called by applications. I expect the API\nto change as we continue developing though, but the general idea will\nstay the same.\n\n> 2. What's the threading model that we plan to use for exercising the\n> Camera objects?\n\nWe don't support multiple threads yet. This will be fixed down the road,\nbut not all details have been decided yet.\n\n> While making Camera objects as shared pointers makes the coding\n> easier, it may also be hard to track the ownership of the objects once\n> the code base gets large. In some cases it may lead to memory/fd leaks\n> that are hard to debug, e.g. a singleton object holding a shared\n> pointer reference to an object and never releases the reference.\n> \n> I tend to agree that fundamental objects like Camera has the necessity\n> to be shared pointers, but would also like to know if it's possible to\n> design the threading model such that CameraManager is the main owner\n> of the Camera objects. For example, if we can design it such that all\n> the access to Camera objects is serialized on a thread,\n\nI think they will, possibly with the exception of request queuing and\nrequest completion handling. This remains to be decided. In any case, I\ndon't think we should support camera lookup from multiple threads.\n\n> then we probably don't need to use shared pointers. Or, we can achieve\n> concurrency by storing Camera objects as std::shared_ptr in\n> CameraManager and only giving out std::weak_ptr of the Camera objects.\n\nIt's not just a concurrency issue. Support for hot-pluggable cameras\nwill be added in the future, and we'll need to track users of camera\nobjects in order to delete the camera only when no users are left. This\ndoesn't require std::shared_ptr<>, a manual reference count would work\ntoo (and it exists today, this patch is replacing it), but I think it's\nbetter to use std::shared_ptr<> rather than reinventing (and\nopen-coding) the wheel.\n\nGiving out weak pointers would I think cause more issues than it would\nsolve, as any user of a camera instance would need to be prepared for\nthe pointer becoming null at any time when the camera is unplugged.\n\n> > - void addCamera(Camera *camera);\n> > + void addCamera(std::shared_ptr &camera);\n> \n> We can pass by value here so that we provide the caller with the\n> freedom to pass its own shared ownership to the callee through\n> std::move().\n\nIn the typical use case the caller will likely retain a shared pointer\nto the camera. I can pass by value and use std::move() in the\naddCamera() function.\n\n> >\n> > static CameraManager *instance();\n> >\n> > @@ -42,7 +42,7 @@ private:\n> >\n> > std::unique_ptr enumerator_;\n> > std::vector pipes_;\n> > - std::vector cameras_;\n> > + std::vector> cameras_;\n> \n> If the camera name is required to be unique then perhaps we can store\n> the cameras as\n> \n> std::unordered_map> cameras_;\n\nWe used to store them in a std::map<>, but it make it cumbersome to\nretrieve the list of all cameras. As I expect the camera lookup API to\nchange anyway I think we can revisit this later.\n\n> >\n> > std::unique_ptr dispatcher_;\n> > };","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 A373660B21\n\tfor ;\n\tFri, 18 Jan 2019 23:51:57 +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 01F0C53E;\n\tFri, 18 Jan 2019 23:51:56 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1547851917;\n\tbh=Ig3XPoR3JUrNZW5G4LATf0rfa847rGx24NZBphS3AAI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HZtURMwYqRbh2mwSXRBZuUlogIS1nKQ7yY7v37wpSEgs/6tRay8oT6tTTS+U8TH/x\n\tyfk1L2rSs4yRqUbh7865wAarNr35F0dX+0z718ZR5RfSx7nWOj3tinKWmDYfIxJ4SU\n\tXn2D5loGjYOeR2KUQVvYPAS1IwhZYCkLcU3Gj008=","Date":"Sat, 19 Jan 2019 00:51:56 +0200","From":"Laurent Pinchart ","To":"Ricky Liang ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190118225156.GB1799@pendragon.ideasonboard.com>","References":"<20190117235916.1906-1-laurent.pinchart@ideasonboard.com>\n\t<20190117235916.1906-5-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: camera: Handle camera\n\tobjects through shared pointers","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 22:51:57 -0000"}},{"id":468,"web_url":"https://patchwork.libcamera.org/comment/468/","msgid":"","date":"2019-01-22T03:19:01","subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: camera: Handle camera\n\tobjects through shared pointers","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 19, 2019 at 6:51 AM Laurent Pinchart\n wrote:\n>\n> Hi Ricky,\n>\n> On Sat, Jan 19, 2019 at 01:39:35AM +0800, Ricky Liang wrote:\n> > On Fri, Jan 18, 2019 at 7:59 AM Laurent Pinchart\n> > wrote:\n> > >\n> > > The Camera class is explicitly reference-counted to manage the lifetime\n> > > of camera objects. Replace this open-coded implementation with usage of\n> > > the std::shared_ptr<> class.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > include/libcamera/camera.h | 13 +++++----\n> > > include/libcamera/camera_manager.h | 8 +++---\n> > > src/libcamera/camera.cpp | 46 +++++++++++++++++-------------\n> > > src/libcamera/camera_manager.cpp | 20 ++++++-------\n> > > src/libcamera/pipeline/vimc.cpp | 2 +-\n> > > test/list-cameras.cpp | 2 +-\n> > > 6 files changed, 50 insertions(+), 41 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > > index 9a7579d61fa3..ef0a794e3a82 100644\n> > > --- a/include/libcamera/camera.h\n> > > +++ b/include/libcamera/camera.h\n> > > @@ -7,6 +7,7 @@\n> > > #ifndef __LIBCAMERA_CAMERA_H__\n> > > #define __LIBCAMERA_CAMERA_H__\n> > >\n> > > +#include \n> > > #include \n> > >\n> > > namespace libcamera {\n> > > @@ -14,15 +15,17 @@ namespace libcamera {\n> > > class Camera\n> > > {\n> > > public:\n> > > - Camera(const std::string &name);\n> > > + static std::shared_ptr create(const std::string &name);\n> > > +\n> > > + Camera(const Camera &) = delete;\n> > > + void operator=(const Camera &) = delete;\n> >\n> > We probably want to delete the implicit constructor as well:\n> >\n> > Camera() = delete;\n>\n> The compiler only creates an implicit constructor when no explicit\n> constructor is specified. As we define Camera(const std::string &name),\n> there's no need to delete the implicit constructor.\n\nAck.\n\n>\n> > >\n> > > const std::string &name() const;\n> > > - void get();\n> > > - void put();\n> > >\n> > > private:\n> > > - virtual ~Camera() { };\n> > > - int ref_;\n> > > + Camera(const std::string &name);\n> >\n> > Can we add the 'explicit' specifier for constructor with only one\n> > argument? Not super important for this patch as it's a private\n> > constructor here, but may be a good general guideline.\n>\n> Good point, I'll do that.\n>\n> > > + ~Camera();\n> > > +\n> > > std::string name_;\n> > > };\n> > >\n> > > diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h\n> > > index 4b941fd9183b..738b13f4cfb1 100644\n> > > --- a/include/libcamera/camera_manager.h\n> > > +++ b/include/libcamera/camera_manager.h\n> > > @@ -24,10 +24,10 @@ public:\n> > > int start();\n> > > void stop();\n> > >\n> > > - const std::vector &cameras() const { return cameras_; }\n> > > - Camera *get(const std::string &name);\n> > > + const std::vector> &cameras() const { return cameras_; }\n> > > + std::shared_ptr get(const std::string &name);\n> >\n> > Some high level design questions:\n> >\n> > 1. Who are the callers of these getter methods?\n>\n> These methods are meant to be called by applications. I expect the API\n> to change as we continue developing though, but the general idea will\n> stay the same.\n>\n> > 2. What's the threading model that we plan to use for exercising the\n> > Camera objects?\n>\n> We don't support multiple threads yet. This will be fixed down the road,\n> but not all details have been decided yet.\n>\n> > While making Camera objects as shared pointers makes the coding\n> > easier, it may also be hard to track the ownership of the objects once\n> > the code base gets large. In some cases it may lead to memory/fd leaks\n> > that are hard to debug, e.g. a singleton object holding a shared\n> > pointer reference to an object and never releases the reference.\n> >\n> > I tend to agree that fundamental objects like Camera has the necessity\n> > to be shared pointers, but would also like to know if it's possible to\n> > design the threading model such that CameraManager is the main owner\n> > of the Camera objects. For example, if we can design it such that all\n> > the access to Camera objects is serialized on a thread,\n>\n> I think they will, possibly with the exception of request queuing and\n> request completion handling. This remains to be decided. In any case, I\n> don't think we should support camera lookup from multiple threads.\n>\n> > then we probably don't need to use shared pointers. Or, we can achieve\n> > concurrency by storing Camera objects as std::shared_ptr in\n> > CameraManager and only giving out std::weak_ptr of the Camera objects.\n>\n> It's not just a concurrency issue. Support for hot-pluggable cameras\n> will be added in the future, and we'll need to track users of camera\n> objects in order to delete the camera only when no users are left. This\n> doesn't require std::shared_ptr<>, a manual reference count would work\n> too (and it exists today, this patch is replacing it), but I think it's\n> better to use std::shared_ptr<> rather than reinventing (and\n> open-coding) the wheel.\n\nI'd imagine in the hot-pluggable camera case, the action of actually\nremoving the camera (e.g. un-plugging the USB camera) would make the\ncamera no longer usable and what we need would be to handle the\nexception gracefully.\n\nOr, do you mean hot-pluggable in the software sense, like the\nbind/unbind in kernel drivers? Most of the camera devices allow only\none active user at a time. Do we plan to design the APIs to allow\nconcurrent access to a camera?\n\n>\n> Giving out weak pointers would I think cause more issues than it would\n> solve, as any user of a camera instance would need to be prepared for\n> the pointer becoming null at any time when the camera is unplugged.\n\nAck.\n\n>\n> > > - void addCamera(Camera *camera);\n> > > + void addCamera(std::shared_ptr &camera);\n> >\n> > We can pass by value here so that we provide the caller with the\n> > freedom to pass its own shared ownership to the callee through\n> > std::move().\n>\n> In the typical use case the caller will likely retain a shared pointer\n> to the camera. I can pass by value and use std::move() in the\n> addCamera() function.\n>\n> > >\n> > > static CameraManager *instance();\n> > >\n> > > @@ -42,7 +42,7 @@ private:\n> > >\n> > > std::unique_ptr enumerator_;\n> > > std::vector pipes_;\n> > > - std::vector cameras_;\n> > > + std::vector> cameras_;\n> >\n> > If the camera name is required to be unique then perhaps we can store\n> > the cameras as\n> >\n> > std::unordered_map> cameras_;\n>\n> We used to store them in a std::map<>, but it make it cumbersome to\n> retrieve the list of all cameras. As I expect the camera lookup API to\n> change anyway I think we can revisit this later.\n\nAck.\n\n>\n> > >\n> > > std::unique_ptr dispatcher_;\n> > > };\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-wr1-x443.google.com (mail-wr1-x443.google.com\n\t[IPv6:2a00:1450:4864:20::443])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D66FC60B2D\n\tfor ;\n\tTue, 22 Jan 2019 04:19:14 +0100 (CET)","by mail-wr1-x443.google.com with SMTP id p4so25627487wrt.7\n\tfor ;\n\tMon, 21 Jan 2019 19:19:14 -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; bh=joVmmjldmH/NmYfpOREqPzoEzwJtqEvHzE0MZZayiaA=;\n\tb=WIptpu8i2DqsZUlkweeG5NXqUHLlG0KSNkfeaSuRNHscx0uk9Rck73HMWquc7zu1nw\n\t47m80kCBhPvVHe5bsvSq5ACMFvL9mEWefSuZmmNKk9s0CFQrHtSKn3gJ//LFCjBU9HSJ\n\t45ZnvrvghawgbulAwutTeJId5CJOLOaCNxIVQ=","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;\n\tbh=joVmmjldmH/NmYfpOREqPzoEzwJtqEvHzE0MZZayiaA=;\n\tb=tUYuRjITd25gt5YgHkmbESB6QvhIzCjGOIGKO45fponXbAtBGj7ZlTxcIpILg0RP86\n\tnX2++KvmJhChy83bTVINfRETkM+JI9fYfzsSc/hoZLQLEioWar4PYQs1U8KtTMMX2HVD\n\txgyesTZxN446E+GNNB+6+9SjXZp6ivjq/WtDJ1282pNP7UcKS9uIgf3paPFyFhCSjM2/\n\trg+ZeXAKWpL5I8NbgAeK5kmoKN08vBnnjQrlRFf1vmLJf+dDRGb19WzpHvlWb738KE8Z\n\t1nEkjzgSzHfQTpHntxbQWpb5tDAVNfLaaX1Qg0J0MP/IJNsk9AuFREb2mxApKTpByJq6\n\tOe3Q==","X-Gm-Message-State":"AJcUukeQM8Yb1Xn+12SsMHMGbPWAjxncJHwKRkEin87fRzgZ1Bp+0Kdt\n\tuQ41pKFjls3dFaDXD8DRfxVjv6sSmKI5lMFmtWU3AQ==","X-Google-Smtp-Source":"ALg8bN6X2L0E+1ZHAl+4+5KiGRj2cJmcUmnX6MJNjzDx4xFua2waNzxzN+XWKptiNInMZdgeVHHFTRMhJu4wKBJD1/g=","X-Received":"by 2002:adf:c452:: with SMTP id\n\ta18mr31066843wrg.145.1548127153827; \n\tMon, 21 Jan 2019 19:19:13 -0800 (PST)","MIME-Version":"1.0","References":"<20190117235916.1906-1-laurent.pinchart@ideasonboard.com>\n\t<20190117235916.1906-5-laurent.pinchart@ideasonboard.com>\n\t\n\t<20190118225156.GB1799@pendragon.ideasonboard.com>","In-Reply-To":"<20190118225156.GB1799@pendragon.ideasonboard.com>","From":"Ricky Liang ","Date":"Tue, 22 Jan 2019 11:19:01 +0800","Message-ID":"","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: camera: Handle camera\n\tobjects through shared pointers","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, 22 Jan 2019 03:19:15 -0000"}},{"id":484,"web_url":"https://patchwork.libcamera.org/comment/484/","msgid":"<20190122122212.GA4455@pendragon.ideasonboard.com>","date":"2019-01-22T12:22:12","subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: camera: Handle camera\n\tobjects through shared pointers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Ricky,\n\nOn Tue, Jan 22, 2019 at 11:19:01AM +0800, Ricky Liang wrote:\n> On Sat, Jan 19, 2019 at 6:51 AM Laurent Pinchart\n> wrote:\n> > On Sat, Jan 19, 2019 at 01:39:35AM +0800, Ricky Liang wrote:\n> >> On Fri, Jan 18, 2019 at 7:59 AM Laurent Pinchart\n> >> wrote:\n> >>> \n> >>> The Camera class is explicitly reference-counted to manage the lifetime\n> >>> of camera objects. Replace this open-coded implementation with usage of\n> >>> the std::shared_ptr<> class.\n> >>> \n> >>> Signed-off-by: Laurent Pinchart \n> >>> ---\n> >>> include/libcamera/camera.h | 13 +++++----\n> >>> include/libcamera/camera_manager.h | 8 +++---\n> >>> src/libcamera/camera.cpp | 46 +++++++++++++++++-------------\n> >>> src/libcamera/camera_manager.cpp | 20 ++++++-------\n> >>> src/libcamera/pipeline/vimc.cpp | 2 +-\n> >>> test/list-cameras.cpp | 2 +-\n> >>> 6 files changed, 50 insertions(+), 41 deletions(-)\n> >>> \n> >>> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> >>> index 9a7579d61fa3..ef0a794e3a82 100644\n> >>> --- a/include/libcamera/camera.h\n> >>> +++ b/include/libcamera/camera.h\n> >>> @@ -7,6 +7,7 @@\n> >>> #ifndef __LIBCAMERA_CAMERA_H__\n> >>> #define __LIBCAMERA_CAMERA_H__\n> >>> \n> >>> +#include \n> >>> #include \n> >>> \n> >>> namespace libcamera {\n> >>> @@ -14,15 +15,17 @@ namespace libcamera {\n> >>> class Camera\n> >>> {\n> >>> public:\n> >>> - Camera(const std::string &name);\n> >>> + static std::shared_ptr create(const std::string &name);\n> >>> +\n> >>> + Camera(const Camera &) = delete;\n> >>> + void operator=(const Camera &) = delete;\n> >> \n> >> We probably want to delete the implicit constructor as well:\n> >> \n> >> Camera() = delete;\n> > \n> > The compiler only creates an implicit constructor when no explicit\n> > constructor is specified. As we define Camera(const std::string &name),\n> > there's no need to delete the implicit constructor.\n> \n> Ack.\n> \n> >>> \n> >>> const std::string &name() const;\n> >>> - void get();\n> >>> - void put();\n> >>> \n> >>> private:\n> >>> - virtual ~Camera() { };\n> >>> - int ref_;\n> >>> + Camera(const std::string &name);\n> >> \n> >> Can we add the 'explicit' specifier for constructor with only one\n> >> argument? Not super important for this patch as it's a private\n> >> constructor here, but may be a good general guideline.\n> > \n> > Good point, I'll do that.\n> > \n> >>> + ~Camera();\n> >>> +\n> >>> std::string name_;\n> >>> };\n> >>> \n> >>> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h\n> >>> index 4b941fd9183b..738b13f4cfb1 100644\n> >>> --- a/include/libcamera/camera_manager.h\n> >>> +++ b/include/libcamera/camera_manager.h\n> >>> @@ -24,10 +24,10 @@ public:\n> >>> int start();\n> >>> void stop();\n> >>> \n> >>> - const std::vector &cameras() const { return cameras_; }\n> >>> - Camera *get(const std::string &name);\n> >>> + const std::vector> &cameras() const { return cameras_; }\n> >>> + std::shared_ptr get(const std::string &name);\n> >> \n> >> Some high level design questions:\n> >> \n> >> 1. Who are the callers of these getter methods?\n> > \n> > These methods are meant to be called by applications. I expect the API\n> > to change as we continue developing though, but the general idea will\n> > stay the same.\n> > \n> >> 2. What's the threading model that we plan to use for exercising the\n> >> Camera objects?\n> > \n> > We don't support multiple threads yet. This will be fixed down the road,\n> > but not all details have been decided yet.\n> > \n> >> While making Camera objects as shared pointers makes the coding\n> >> easier, it may also be hard to track the ownership of the objects once\n> >> the code base gets large. In some cases it may lead to memory/fd leaks\n> >> that are hard to debug, e.g. a singleton object holding a shared\n> >> pointer reference to an object and never releases the reference.\n> >> \n> >> I tend to agree that fundamental objects like Camera has the necessity\n> >> to be shared pointers, but would also like to know if it's possible to\n> >> design the threading model such that CameraManager is the main owner\n> >> of the Camera objects. For example, if we can design it such that all\n> >> the access to Camera objects is serialized on a thread,\n> > \n> > I think they will, possibly with the exception of request queuing and\n> > request completion handling. This remains to be decided. In any case, I\n> > don't think we should support camera lookup from multiple threads.\n> > \n> >> then we probably don't need to use shared pointers. Or, we can achieve\n> >> concurrency by storing Camera objects as std::shared_ptr in\n> >> CameraManager and only giving out std::weak_ptr of the Camera objects.\n> > \n> > It's not just a concurrency issue. Support for hot-pluggable cameras\n> > will be added in the future, and we'll need to track users of camera\n> > objects in order to delete the camera only when no users are left. This\n> > doesn't require std::shared_ptr<>, a manual reference count would work\n> > too (and it exists today, this patch is replacing it), but I think it's\n> > better to use std::shared_ptr<> rather than reinventing (and\n> > open-coding) the wheel.\n> \n> I'd imagine in the hot-pluggable camera case, the action of actually\n> removing the camera (e.g. un-plugging the USB camera) would make the\n> camera no longer usable and what we need would be to handle the\n> exception gracefully.\n\nCorrect, the camera object would be marked as disconnected, API calls\nwould start failing, and when the camera is released by the application\nit will be deleted.\n\n> Or, do you mean hot-pluggable in the software sense, like the\n> bind/unbind in kernel drivers? Most of the camera devices allow only\n> one active user at a time. Do we plan to design the APIs to allow\n> concurrent access to a camera?\n\nI meant hotplugging a USB webcam for instance, but from a userspace\npoint of view unbinding a device from its kernel driver should achieve\nthe same effect. I'd like pipeline handlers to handle that gracefully if\nit happens, but it's not a priority.\n\nWe don't plan to support concurrent access to a camera by multiple\napplications, that would be handled by a layer above libcamera if\nneeded.\n\n> > Giving out weak pointers would I think cause more issues than it would\n> > solve, as any user of a camera instance would need to be prepared for\n> > the pointer becoming null at any time when the camera is unplugged.\n> \n> Ack.\n> \n> >>> - void addCamera(Camera *camera);\n> >>> + void addCamera(std::shared_ptr &camera);\n> >> \n> >> We can pass by value here so that we provide the caller with the\n> >> freedom to pass its own shared ownership to the callee through\n> >> std::move().\n> > \n> > In the typical use case the caller will likely retain a shared pointer\n> > to the camera. I can pass by value and use std::move() in the\n> > addCamera() function.\n> > \n> >>> \n> >>> static CameraManager *instance();\n> >>> \n> >>> @@ -42,7 +42,7 @@ private:\n> >>> \n> >>> std::unique_ptr enumerator_;\n> >>> std::vector pipes_;\n> >>> - std::vector cameras_;\n> >>> + std::vector> cameras_;\n> >> \n> >> If the camera name is required to be unique then perhaps we can store\n> >> the cameras as\n> >> \n> >> std::unordered_map> cameras_;\n> > \n> > We used to store them in a std::map<>, but it make it cumbersome to\n> > retrieve the list of all cameras. As I expect the camera lookup API to\n> > change anyway I think we can revisit this later.\n> \n> Ack.\n> \n> >>> \n> >>> std::unique_ptr dispatcher_;\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 E2DF360B21\n\tfor ;\n\tTue, 22 Jan 2019 13:22:13 +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 34FAF53E;\n\tTue, 22 Jan 2019 13:22:13 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1548159733;\n\tbh=1+uh2tz4z57ajrC92qc0fBq5Y9uokfiPcWQPMzFzIp0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uf+tD7ylyt1u+yNnp9oRg9Ac9uCYYldHOmQBhRHPiBG1mjxRi6Kg/BUqbTkz3fx3L\n\tdeXQLT4M4vPWL2vPtxKC53fcAaFTqDIaoupqlvdnq6k1uHz2uTrv0Ov2ivqroywqDW\n\tKYg30cWJc2ZJne4jF3WWLBfnrLTHqtJ1NanB8kYw=","Date":"Tue, 22 Jan 2019 14:22:12 +0200","From":"Laurent Pinchart ","To":"Ricky Liang ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190122122212.GA4455@pendragon.ideasonboard.com>","References":"<20190117235916.1906-1-laurent.pinchart@ideasonboard.com>\n\t<20190117235916.1906-5-laurent.pinchart@ideasonboard.com>\n\t\n\t<20190118225156.GB1799@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: camera: Handle camera\n\tobjects through shared pointers","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, 22 Jan 2019 12:22:14 -0000"}},{"id":575,"web_url":"https://patchwork.libcamera.org/comment/575/","msgid":"","date":"2019-01-25T06:55:12","subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: camera: Handle camera\n\tobjects through shared pointers","submitter":{"id":9,"url":"https://patchwork.libcamera.org/api/people/9/","name":"Tomasz Figa","email":"tfiga@chromium.org"},"content":"On Tue, Jan 22, 2019 at 9:22 PM Laurent Pinchart\n wrote:\n>\n> Hi Ricky,\n>\n> On Tue, Jan 22, 2019 at 11:19:01AM +0800, Ricky Liang wrote:\n> > On Sat, Jan 19, 2019 at 6:51 AM Laurent Pinchart\n> > wrote:\n> > > On Sat, Jan 19, 2019 at 01:39:35AM +0800, Ricky Liang wrote:\n> > >> On Fri, Jan 18, 2019 at 7:59 AM Laurent Pinchart\n> > >> wrote:\n> > >>>\n> > >>> The Camera class is explicitly reference-counted to manage the lifetime\n> > >>> of camera objects. Replace this open-coded implementation with usage of\n> > >>> the std::shared_ptr<> class.\n> > >>>\n> > >>> Signed-off-by: Laurent Pinchart \n> > >>> ---\n> > >>> include/libcamera/camera.h | 13 +++++----\n> > >>> include/libcamera/camera_manager.h | 8 +++---\n> > >>> src/libcamera/camera.cpp | 46 +++++++++++++++++-------------\n> > >>> src/libcamera/camera_manager.cpp | 20 ++++++-------\n> > >>> src/libcamera/pipeline/vimc.cpp | 2 +-\n> > >>> test/list-cameras.cpp | 2 +-\n> > >>> 6 files changed, 50 insertions(+), 41 deletions(-)\n> > >>>\n> > >>> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > >>> index 9a7579d61fa3..ef0a794e3a82 100644\n> > >>> --- a/include/libcamera/camera.h\n> > >>> +++ b/include/libcamera/camera.h\n> > >>> @@ -7,6 +7,7 @@\n> > >>> #ifndef __LIBCAMERA_CAMERA_H__\n> > >>> #define __LIBCAMERA_CAMERA_H__\n> > >>>\n> > >>> +#include \n> > >>> #include \n> > >>>\n> > >>> namespace libcamera {\n> > >>> @@ -14,15 +15,17 @@ namespace libcamera {\n> > >>> class Camera\n> > >>> {\n> > >>> public:\n> > >>> - Camera(const std::string &name);\n> > >>> + static std::shared_ptr create(const std::string &name);\n> > >>> +\n> > >>> + Camera(const Camera &) = delete;\n> > >>> + void operator=(const Camera &) = delete;\n> > >>\n> > >> We probably want to delete the implicit constructor as well:\n> > >>\n> > >> Camera() = delete;\n> > >\n> > > The compiler only creates an implicit constructor when no explicit\n> > > constructor is specified. As we define Camera(const std::string &name),\n> > > there's no need to delete the implicit constructor.\n> >\n> > Ack.\n> >\n> > >>>\n> > >>> const std::string &name() const;\n> > >>> - void get();\n> > >>> - void put();\n> > >>>\n> > >>> private:\n> > >>> - virtual ~Camera() { };\n> > >>> - int ref_;\n> > >>> + Camera(const std::string &name);\n> > >>\n> > >> Can we add the 'explicit' specifier for constructor with only one\n> > >> argument? Not super important for this patch as it's a private\n> > >> constructor here, but may be a good general guideline.\n> > >\n> > > Good point, I'll do that.\n> > >\n> > >>> + ~Camera();\n> > >>> +\n> > >>> std::string name_;\n> > >>> };\n> > >>>\n> > >>> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h\n> > >>> index 4b941fd9183b..738b13f4cfb1 100644\n> > >>> --- a/include/libcamera/camera_manager.h\n> > >>> +++ b/include/libcamera/camera_manager.h\n> > >>> @@ -24,10 +24,10 @@ public:\n> > >>> int start();\n> > >>> void stop();\n> > >>>\n> > >>> - const std::vector &cameras() const { return cameras_; }\n> > >>> - Camera *get(const std::string &name);\n> > >>> + const std::vector> &cameras() const { return cameras_; }\n> > >>> + std::shared_ptr get(const std::string &name);\n> > >>\n> > >> Some high level design questions:\n> > >>\n> > >> 1. Who are the callers of these getter methods?\n> > >\n> > > These methods are meant to be called by applications. I expect the API\n> > > to change as we continue developing though, but the general idea will\n> > > stay the same.\n> > >\n> > >> 2. What's the threading model that we plan to use for exercising the\n> > >> Camera objects?\n> > >\n> > > We don't support multiple threads yet. This will be fixed down the road,\n> > > but not all details have been decided yet.\n> > >\n> > >> While making Camera objects as shared pointers makes the coding\n> > >> easier, it may also be hard to track the ownership of the objects once\n> > >> the code base gets large. In some cases it may lead to memory/fd leaks\n> > >> that are hard to debug, e.g. a singleton object holding a shared\n> > >> pointer reference to an object and never releases the reference.\n> > >>\n> > >> I tend to agree that fundamental objects like Camera has the necessity\n> > >> to be shared pointers, but would also like to know if it's possible to\n> > >> design the threading model such that CameraManager is the main owner\n> > >> of the Camera objects. For example, if we can design it such that all\n> > >> the access to Camera objects is serialized on a thread,\n> > >\n> > > I think they will, possibly with the exception of request queuing and\n> > > request completion handling. This remains to be decided. In any case, I\n> > > don't think we should support camera lookup from multiple threads.\n> > >\n> > >> then we probably don't need to use shared pointers. Or, we can achieve\n> > >> concurrency by storing Camera objects as std::shared_ptr in\n> > >> CameraManager and only giving out std::weak_ptr of the Camera objects.\n> > >\n> > > It's not just a concurrency issue. Support for hot-pluggable cameras\n> > > will be added in the future, and we'll need to track users of camera\n> > > objects in order to delete the camera only when no users are left. This\n> > > doesn't require std::shared_ptr<>, a manual reference count would work\n> > > too (and it exists today, this patch is replacing it), but I think it's\n> > > better to use std::shared_ptr<> rather than reinventing (and\n> > > open-coding) the wheel.\n> >\n> > I'd imagine in the hot-pluggable camera case, the action of actually\n> > removing the camera (e.g. un-plugging the USB camera) would make the\n> > camera no longer usable and what we need would be to handle the\n> > exception gracefully.\n>\n> Correct, the camera object would be marked as disconnected, API calls\n> would start failing, and when the camera is released by the application\n> it will be deleted.\n>\n> > Or, do you mean hot-pluggable in the software sense, like the\n> > bind/unbind in kernel drivers? Most of the camera devices allow only\n> > one active user at a time. Do we plan to design the APIs to allow\n> > concurrent access to a camera?\n>\n> I meant hotplugging a USB webcam for instance, but from a userspace\n> point of view unbinding a device from its kernel driver should achieve\n> the same effect. I'd like pipeline handlers to handle that gracefully if\n> it happens, but it's not a priority.\n>\n> We don't plan to support concurrent access to a camera by multiple\n> applications, that would be handled by a layer above libcamera if\n> needed.\n\nSo I'm actually wondering if we're trying to solve a real problem\nhere. Physical camera hotplug is already handled for us by the kernel.\nAs long as the userspace has the V4L2 file descriptors open, it can\ninteract with them and get error codes when the camera goes away. We\ncould just put our internal objects in some error state, fail any\npending capture requests and have any further calls on this Camera\nobject just fail, in a way that would explicitly tell the consumer\nthat the reason for the failure was disconnection. Then it would be up\nto the consumer to actually destroy the Camera object.\n\nBest regards,\nTomasz","headers":{"Return-Path":"","Received":["from mail-ot1-x343.google.com (mail-ot1-x343.google.com\n\t[IPv6:2607:f8b0:4864:20::343])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 49BE260C7D\n\tfor ;\n\tFri, 25 Jan 2019 07:55:27 +0100 (CET)","by mail-ot1-x343.google.com with SMTP id t5so7660434otk.1\n\tfor ;\n\tThu, 24 Jan 2019 22:55:27 -0800 (PST)","from mail-ot1-f53.google.com (mail-ot1-f53.google.com.\n\t[209.85.210.53]) by smtp.gmail.com with ESMTPSA id\n\tm129sm923215oif.50.2019.01.24.22.55.23\n\tfor \n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tThu, 24 Jan 2019 22:55:24 -0800 (PST)","by mail-ot1-f53.google.com with SMTP id u16so7625240otk.8\n\tfor ;\n\tThu, 24 Jan 2019 22:55:23 -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; bh=UuGBy+3cV/gbHu1MnxLaDbQ6KR1qrYqpLABIRgkYz3c=;\n\tb=YRIa6Ruo2iPtwOQYOAr8jm1hZgG4YR3FHCAdzLR/fgT4bydqH+m2C0+btXkDZBqrNV\n\tQKnEwxfp2DMQEIZ7EShiRKqEitMlq742CcS+ZjTsQPPhN/hQEh0dME9GOsekm12MNxPU\n\tyZz6mefgrxAxa8odk5UZBKHmbZ+SUVileCjCg=","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;\n\tbh=UuGBy+3cV/gbHu1MnxLaDbQ6KR1qrYqpLABIRgkYz3c=;\n\tb=iUvbJh0cKvRW3/f+EIGvmXrY1UpAOpPSmTUXDRJQwOlIkPGWRlq1uFQ2xn+r1uCVOV\n\tlsDnhOTbENG95X1mOP/EhWQJ03CYD0rambbB4tqaocrKtUsifvRlVjMtmMyl/1ffmSht\n\tcdto3v5EemiBi+X3ChyAtCIkoMSgh6Aas59QMUpRoCGsD3+wZMM/ThlQhyG6rmQ9iPwm\n\t91pHVbT/2Zvj1E3b7Mx2Z4PLvYB6FQlvApLwXmKktYtiHeZ8IzutqxczIuvhyy3xzzoM\n\t6tDMTTlgpIViCIT4VbkdoGKLaFBOJLtsvLgDLF801RwcrjWDv9t4iMC41fseGECPdx2S\n\tATDw==","X-Gm-Message-State":"AJcUukcALR7ViWVCEkxFeyP8aDuDal1IhbSmRN3dBu8Gm2M8OkFdP9DJ\n\tM1VixFT/ncAYmVytE1o5tKlZpLT08KNJlA==","X-Google-Smtp-Source":"ALg8bN7u6PbGlSpPPwewnEjcTAw7fzJs7R/gkULuTAezV1eKC96pnOqYUh56tPipPbI6sAzflKnX8g==","X-Received":["by 2002:a9d:69d5:: with SMTP id v21mr7067206oto.77.1548399325601;\n\tThu, 24 Jan 2019 22:55:25 -0800 (PST)","by 2002:a9d:5cc2:: with SMTP id r2mr6865357oti.367.1548399323444;\n\tThu, 24 Jan 2019 22:55:23 -0800 (PST)"],"MIME-Version":"1.0","References":"<20190117235916.1906-1-laurent.pinchart@ideasonboard.com>\n\t<20190117235916.1906-5-laurent.pinchart@ideasonboard.com>\n\t\n\t<20190118225156.GB1799@pendragon.ideasonboard.com>\n\t\n\t<20190122122212.GA4455@pendragon.ideasonboard.com>","In-Reply-To":"<20190122122212.GA4455@pendragon.ideasonboard.com>","From":"Tomasz Figa ","Date":"Fri, 25 Jan 2019 15:55:12 +0900","X-Gmail-Original-Message-ID":"","Message-ID":"","To":"Laurent Pinchart ","Cc":"Ricky Liang , libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: camera: Handle camera\n\tobjects through shared pointers","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, 25 Jan 2019 06:55:27 -0000"}},{"id":579,"web_url":"https://patchwork.libcamera.org/comment/579/","msgid":"<20190125103357.GA2934@pendragon.ideasonboard.com>","date":"2019-01-25T10:33:57","subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: camera: Handle camera\n\tobjects through shared pointers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Tomasz,\n\nOn Fri, Jan 25, 2019 at 03:55:12PM +0900, Tomasz Figa wrote:\n> On Tue, Jan 22, 2019 at 9:22 PM Laurent Pinchart\n> wrote:\n> > On Tue, Jan 22, 2019 at 11:19:01AM +0800, Ricky Liang wrote:\n> >> On Sat, Jan 19, 2019 at 6:51 AM Laurent Pinchart\n> >> wrote:\n> >>> On Sat, Jan 19, 2019 at 01:39:35AM +0800, Ricky Liang wrote:\n> >>>> On Fri, Jan 18, 2019 at 7:59 AM Laurent Pinchart\n> >>>> wrote:\n> >>>>>\n> >>>>> The Camera class is explicitly reference-counted to manage the lifetime\n> >>>>> of camera objects. Replace this open-coded implementation with usage of\n> >>>>> the std::shared_ptr<> class.\n> >>>>>\n> >>>>> Signed-off-by: Laurent Pinchart \n> >>>>> ---\n> >>>>> include/libcamera/camera.h | 13 +++++----\n> >>>>> include/libcamera/camera_manager.h | 8 +++---\n> >>>>> src/libcamera/camera.cpp | 46 +++++++++++++++++-------------\n> >>>>> src/libcamera/camera_manager.cpp | 20 ++++++-------\n> >>>>> src/libcamera/pipeline/vimc.cpp | 2 +-\n> >>>>> test/list-cameras.cpp | 2 +-\n> >>>>> 6 files changed, 50 insertions(+), 41 deletions(-)\n> >>>>>\n> >>>>> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> >>>>> index 9a7579d61fa3..ef0a794e3a82 100644\n> >>>>> --- a/include/libcamera/camera.h\n> >>>>> +++ b/include/libcamera/camera.h\n> >>>>> @@ -7,6 +7,7 @@\n> >>>>> #ifndef __LIBCAMERA_CAMERA_H__\n> >>>>> #define __LIBCAMERA_CAMERA_H__\n> >>>>>\n> >>>>> +#include \n> >>>>> #include \n> >>>>>\n> >>>>> namespace libcamera {\n> >>>>> @@ -14,15 +15,17 @@ namespace libcamera {\n> >>>>> class Camera\n> >>>>> {\n> >>>>> public:\n> >>>>> - Camera(const std::string &name);\n> >>>>> + static std::shared_ptr create(const std::string &name);\n> >>>>> +\n> >>>>> + Camera(const Camera &) = delete;\n> >>>>> + void operator=(const Camera &) = delete;\n> >>>>\n> >>>> We probably want to delete the implicit constructor as well:\n> >>>>\n> >>>> Camera() = delete;\n> >>>\n> >>> The compiler only creates an implicit constructor when no explicit\n> >>> constructor is specified. As we define Camera(const std::string &name),\n> >>> there's no need to delete the implicit constructor.\n> >>\n> >> Ack.\n> >>\n> >>>>>\n> >>>>> const std::string &name() const;\n> >>>>> - void get();\n> >>>>> - void put();\n> >>>>>\n> >>>>> private:\n> >>>>> - virtual ~Camera() { };\n> >>>>> - int ref_;\n> >>>>> + Camera(const std::string &name);\n> >>>>\n> >>>> Can we add the 'explicit' specifier for constructor with only one\n> >>>> argument? Not super important for this patch as it's a private\n> >>>> constructor here, but may be a good general guideline.\n> >>>\n> >>> Good point, I'll do that.\n> >>>\n> >>>>> + ~Camera();\n> >>>>> +\n> >>>>> std::string name_;\n> >>>>> };\n> >>>>>\n> >>>>> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h\n> >>>>> index 4b941fd9183b..738b13f4cfb1 100644\n> >>>>> --- a/include/libcamera/camera_manager.h\n> >>>>> +++ b/include/libcamera/camera_manager.h\n> >>>>> @@ -24,10 +24,10 @@ public:\n> >>>>> int start();\n> >>>>> void stop();\n> >>>>>\n> >>>>> - const std::vector &cameras() const { return cameras_; }\n> >>>>> - Camera *get(const std::string &name);\n> >>>>> + const std::vector> &cameras() const { return cameras_; }\n> >>>>> + std::shared_ptr get(const std::string &name);\n> >>>>\n> >>>> Some high level design questions:\n> >>>>\n> >>>> 1. Who are the callers of these getter methods?\n> >>>\n> >>> These methods are meant to be called by applications. I expect the API\n> >>> to change as we continue developing though, but the general idea will\n> >>> stay the same.\n> >>>\n> >>>> 2. What's the threading model that we plan to use for exercising the\n> >>>> Camera objects?\n> >>>\n> >>> We don't support multiple threads yet. This will be fixed down the road,\n> >>> but not all details have been decided yet.\n> >>>\n> >>>> While making Camera objects as shared pointers makes the coding\n> >>>> easier, it may also be hard to track the ownership of the objects once\n> >>>> the code base gets large. In some cases it may lead to memory/fd leaks\n> >>>> that are hard to debug, e.g. a singleton object holding a shared\n> >>>> pointer reference to an object and never releases the reference.\n> >>>>\n> >>>> I tend to agree that fundamental objects like Camera has the necessity\n> >>>> to be shared pointers, but would also like to know if it's possible to\n> >>>> design the threading model such that CameraManager is the main owner\n> >>>> of the Camera objects. For example, if we can design it such that all\n> >>>> the access to Camera objects is serialized on a thread,\n> >>>\n> >>> I think they will, possibly with the exception of request queuing and\n> >>> request completion handling. This remains to be decided. In any case, I\n> >>> don't think we should support camera lookup from multiple threads.\n> >>>\n> >>>> then we probably don't need to use shared pointers. Or, we can achieve\n> >>>> concurrency by storing Camera objects as std::shared_ptr in\n> >>>> CameraManager and only giving out std::weak_ptr of the Camera objects.\n> >>>\n> >>> It's not just a concurrency issue. Support for hot-pluggable cameras\n> >>> will be added in the future, and we'll need to track users of camera\n> >>> objects in order to delete the camera only when no users are left. This\n> >>> doesn't require std::shared_ptr<>, a manual reference count would work\n> >>> too (and it exists today, this patch is replacing it), but I think it's\n> >>> better to use std::shared_ptr<> rather than reinventing (and\n> >>> open-coding) the wheel.\n> >>\n> >> I'd imagine in the hot-pluggable camera case, the action of actually\n> >> removing the camera (e.g. un-plugging the USB camera) would make the\n> >> camera no longer usable and what we need would be to handle the\n> >> exception gracefully.\n> >\n> > Correct, the camera object would be marked as disconnected, API calls\n> > would start failing, and when the camera is released by the application\n> > it will be deleted.\n> >\n> >> Or, do you mean hot-pluggable in the software sense, like the\n> >> bind/unbind in kernel drivers? Most of the camera devices allow only\n> >> one active user at a time. Do we plan to design the APIs to allow\n> >> concurrent access to a camera?\n> >\n> > I meant hotplugging a USB webcam for instance, but from a userspace\n> > point of view unbinding a device from its kernel driver should achieve\n> > the same effect. I'd like pipeline handlers to handle that gracefully if\n> > it happens, but it's not a priority.\n> >\n> > We don't plan to support concurrent access to a camera by multiple\n> > applications, that would be handled by a layer above libcamera if\n> > needed.\n> \n> So I'm actually wondering if we're trying to solve a real problem\n> here. Physical camera hotplug is already handled for us by the kernel.\n> As long as the userspace has the V4L2 file descriptors open, it can\n> interact with them and get error codes when the camera goes away. We\n> could just put our internal objects in some error state, fail any\n> pending capture requests and have any further calls on this Camera\n> object just fail, in a way that would explicitly tell the consumer\n> that the reason for the failure was disconnection. Then it would be up\n> to the consumer to actually destroy the Camera object.\n\nOne core design principle in libcamera is that the library enumerates\ncameras by scanning for media devices (in the MC sense) and associating\nthem with pipeline handlers (which can be seen as a kind of userspace\ncounterpart of the kernel drivers, exposing various MC device nodes as\nhigher level camera objects, the same way a kernel driver exposes\nvarious pieces of hardware as standardized device nodes). The library is\nthus responsible for creating the camera objects, which are then\nretrieved and used by applications. In the context of hotplug handling\nwe need to signal hot-unplug to applications, certainly in the form of\ncompleting pending capture requests with an error and failing futher\ncalls, but I think also as an explicit signal to the application. For\nthis to work I believe handling camera objects as shared pointers is the\nbest option, as the consumer has to release the object, but shouldn't\ndestroy it. Am I biased ? :-)","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 AC5E260C7F\n\tfor ;\n\tFri, 25 Jan 2019 11:33:58 +0100 (CET)","from pendragon.ideasonboard.com (unknown\n\t[IPv6:2a02:a03f:4499:2700:1060:1d4c:d6a:8e80])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D98B3325;\n\tFri, 25 Jan 2019 11:33:57 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1548412438;\n\tbh=qBLmq1lqTXW7ndVdbZk/5sUU9B2/SIr+17vf7UoA1uQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=BpRfF/k5e+dc7gyLgxufk4MLssCoKrikYZnVquU0HF3jsIJ/noePSv3TiC+wl6Jpc\n\t3CB+pQwjO9nWKxOsgEDNovTHMP+dDSMuwEp5GTTiiRLUCvj46mJfWNYrOmyrZHV2CU\n\tcmbMEpTCS+M7o1Opy2OxPp30SlA26jUaLtmrMD18=","Date":"Fri, 25 Jan 2019 12:33:57 +0200","From":"Laurent Pinchart ","To":"Tomasz Figa ","Cc":"Ricky Liang , libcamera-devel@lists.libcamera.org","Message-ID":"<20190125103357.GA2934@pendragon.ideasonboard.com>","References":"<20190117235916.1906-1-laurent.pinchart@ideasonboard.com>\n\t<20190117235916.1906-5-laurent.pinchart@ideasonboard.com>\n\t\n\t<20190118225156.GB1799@pendragon.ideasonboard.com>\n\t\n\t<20190122122212.GA4455@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: camera: Handle camera\n\tobjects through shared pointers","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, 25 Jan 2019 10:33:58 -0000"}},{"id":650,"web_url":"https://patchwork.libcamera.org/comment/650/","msgid":"","date":"2019-01-28T07:53:39","subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: camera: Handle camera\n\tobjects through shared pointers","submitter":{"id":9,"url":"https://patchwork.libcamera.org/api/people/9/","name":"Tomasz Figa","email":"tfiga@chromium.org"},"content":"On Fri, Jan 25, 2019 at 7:34 PM Laurent Pinchart\n wrote:\n>\n> Hi Tomasz,\n>\n> On Fri, Jan 25, 2019 at 03:55:12PM +0900, Tomasz Figa wrote:\n> > On Tue, Jan 22, 2019 at 9:22 PM Laurent Pinchart\n> > wrote:\n> > > On Tue, Jan 22, 2019 at 11:19:01AM +0800, Ricky Liang wrote:\n> > >> On Sat, Jan 19, 2019 at 6:51 AM Laurent Pinchart\n> > >> wrote:\n> > >>> On Sat, Jan 19, 2019 at 01:39:35AM +0800, Ricky Liang wrote:\n> > >>>> On Fri, Jan 18, 2019 at 7:59 AM Laurent Pinchart\n> > >>>> wrote:\n> > >>>>>\n> > >>>>> The Camera class is explicitly reference-counted to manage the lifetime\n> > >>>>> of camera objects. Replace this open-coded implementation with usage of\n> > >>>>> the std::shared_ptr<> class.\n> > >>>>>\n> > >>>>> Signed-off-by: Laurent Pinchart \n> > >>>>> ---\n> > >>>>> include/libcamera/camera.h | 13 +++++----\n> > >>>>> include/libcamera/camera_manager.h | 8 +++---\n> > >>>>> src/libcamera/camera.cpp | 46 +++++++++++++++++-------------\n> > >>>>> src/libcamera/camera_manager.cpp | 20 ++++++-------\n> > >>>>> src/libcamera/pipeline/vimc.cpp | 2 +-\n> > >>>>> test/list-cameras.cpp | 2 +-\n> > >>>>> 6 files changed, 50 insertions(+), 41 deletions(-)\n> > >>>>>\n> > >>>>> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > >>>>> index 9a7579d61fa3..ef0a794e3a82 100644\n> > >>>>> --- a/include/libcamera/camera.h\n> > >>>>> +++ b/include/libcamera/camera.h\n> > >>>>> @@ -7,6 +7,7 @@\n> > >>>>> #ifndef __LIBCAMERA_CAMERA_H__\n> > >>>>> #define __LIBCAMERA_CAMERA_H__\n> > >>>>>\n> > >>>>> +#include \n> > >>>>> #include \n> > >>>>>\n> > >>>>> namespace libcamera {\n> > >>>>> @@ -14,15 +15,17 @@ namespace libcamera {\n> > >>>>> class Camera\n> > >>>>> {\n> > >>>>> public:\n> > >>>>> - Camera(const std::string &name);\n> > >>>>> + static std::shared_ptr create(const std::string &name);\n> > >>>>> +\n> > >>>>> + Camera(const Camera &) = delete;\n> > >>>>> + void operator=(const Camera &) = delete;\n> > >>>>\n> > >>>> We probably want to delete the implicit constructor as well:\n> > >>>>\n> > >>>> Camera() = delete;\n> > >>>\n> > >>> The compiler only creates an implicit constructor when no explicit\n> > >>> constructor is specified. As we define Camera(const std::string &name),\n> > >>> there's no need to delete the implicit constructor.\n> > >>\n> > >> Ack.\n> > >>\n> > >>>>>\n> > >>>>> const std::string &name() const;\n> > >>>>> - void get();\n> > >>>>> - void put();\n> > >>>>>\n> > >>>>> private:\n> > >>>>> - virtual ~Camera() { };\n> > >>>>> - int ref_;\n> > >>>>> + Camera(const std::string &name);\n> > >>>>\n> > >>>> Can we add the 'explicit' specifier for constructor with only one\n> > >>>> argument? Not super important for this patch as it's a private\n> > >>>> constructor here, but may be a good general guideline.\n> > >>>\n> > >>> Good point, I'll do that.\n> > >>>\n> > >>>>> + ~Camera();\n> > >>>>> +\n> > >>>>> std::string name_;\n> > >>>>> };\n> > >>>>>\n> > >>>>> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h\n> > >>>>> index 4b941fd9183b..738b13f4cfb1 100644\n> > >>>>> --- a/include/libcamera/camera_manager.h\n> > >>>>> +++ b/include/libcamera/camera_manager.h\n> > >>>>> @@ -24,10 +24,10 @@ public:\n> > >>>>> int start();\n> > >>>>> void stop();\n> > >>>>>\n> > >>>>> - const std::vector &cameras() const { return cameras_; }\n> > >>>>> - Camera *get(const std::string &name);\n> > >>>>> + const std::vector> &cameras() const { return cameras_; }\n> > >>>>> + std::shared_ptr get(const std::string &name);\n> > >>>>\n> > >>>> Some high level design questions:\n> > >>>>\n> > >>>> 1. Who are the callers of these getter methods?\n> > >>>\n> > >>> These methods are meant to be called by applications. I expect the API\n> > >>> to change as we continue developing though, but the general idea will\n> > >>> stay the same.\n> > >>>\n> > >>>> 2. What's the threading model that we plan to use for exercising the\n> > >>>> Camera objects?\n> > >>>\n> > >>> We don't support multiple threads yet. This will be fixed down the road,\n> > >>> but not all details have been decided yet.\n> > >>>\n> > >>>> While making Camera objects as shared pointers makes the coding\n> > >>>> easier, it may also be hard to track the ownership of the objects once\n> > >>>> the code base gets large. In some cases it may lead to memory/fd leaks\n> > >>>> that are hard to debug, e.g. a singleton object holding a shared\n> > >>>> pointer reference to an object and never releases the reference.\n> > >>>>\n> > >>>> I tend to agree that fundamental objects like Camera has the necessity\n> > >>>> to be shared pointers, but would also like to know if it's possible to\n> > >>>> design the threading model such that CameraManager is the main owner\n> > >>>> of the Camera objects. For example, if we can design it such that all\n> > >>>> the access to Camera objects is serialized on a thread,\n> > >>>\n> > >>> I think they will, possibly with the exception of request queuing and\n> > >>> request completion handling. This remains to be decided. In any case, I\n> > >>> don't think we should support camera lookup from multiple threads.\n> > >>>\n> > >>>> then we probably don't need to use shared pointers. Or, we can achieve\n> > >>>> concurrency by storing Camera objects as std::shared_ptr in\n> > >>>> CameraManager and only giving out std::weak_ptr of the Camera objects.\n> > >>>\n> > >>> It's not just a concurrency issue. Support for hot-pluggable cameras\n> > >>> will be added in the future, and we'll need to track users of camera\n> > >>> objects in order to delete the camera only when no users are left. This\n> > >>> doesn't require std::shared_ptr<>, a manual reference count would work\n> > >>> too (and it exists today, this patch is replacing it), but I think it's\n> > >>> better to use std::shared_ptr<> rather than reinventing (and\n> > >>> open-coding) the wheel.\n> > >>\n> > >> I'd imagine in the hot-pluggable camera case, the action of actually\n> > >> removing the camera (e.g. un-plugging the USB camera) would make the\n> > >> camera no longer usable and what we need would be to handle the\n> > >> exception gracefully.\n> > >\n> > > Correct, the camera object would be marked as disconnected, API calls\n> > > would start failing, and when the camera is released by the application\n> > > it will be deleted.\n> > >\n> > >> Or, do you mean hot-pluggable in the software sense, like the\n> > >> bind/unbind in kernel drivers? Most of the camera devices allow only\n> > >> one active user at a time. Do we plan to design the APIs to allow\n> > >> concurrent access to a camera?\n> > >\n> > > I meant hotplugging a USB webcam for instance, but from a userspace\n> > > point of view unbinding a device from its kernel driver should achieve\n> > > the same effect. I'd like pipeline handlers to handle that gracefully if\n> > > it happens, but it's not a priority.\n> > >\n> > > We don't plan to support concurrent access to a camera by multiple\n> > > applications, that would be handled by a layer above libcamera if\n> > > needed.\n> >\n> > So I'm actually wondering if we're trying to solve a real problem\n> > here. Physical camera hotplug is already handled for us by the kernel.\n> > As long as the userspace has the V4L2 file descriptors open, it can\n> > interact with them and get error codes when the camera goes away. We\n> > could just put our internal objects in some error state, fail any\n> > pending capture requests and have any further calls on this Camera\n> > object just fail, in a way that would explicitly tell the consumer\n> > that the reason for the failure was disconnection. Then it would be up\n> > to the consumer to actually destroy the Camera object.\n>\n> One core design principle in libcamera is that the library enumerates\n> cameras by scanning for media devices (in the MC sense) and associating\n> them with pipeline handlers (which can be seen as a kind of userspace\n> counterpart of the kernel drivers, exposing various MC device nodes as\n> higher level camera objects, the same way a kernel driver exposes\n> various pieces of hardware as standardized device nodes). The library is\n> thus responsible for creating the camera objects, which are then\n> retrieved and used by applications. In the context of hotplug handling\n> we need to signal hot-unplug to applications, certainly in the form of\n> completing pending capture requests with an error and failing futher\n> calls, but I think also as an explicit signal to the application. For\n> this to work I believe handling camera objects as shared pointers is the\n> best option, as the consumer has to release the object, but shouldn't\n> destroy it. Am I biased ? :-)\n\nI'd give the consumer an object it can destroy, which only points to\nthe camera object created by the framework. That would eliminate the\nshared pointers from the public API, but still give you the ability to\nuse them internally. For any signals to the application, we need some\nevent loop on the application side anyway, which would poll the object\nfor messages, so that should work as well with this kind of \"consumer\"\nobject.\n\nBest regards,\nTomasz","headers":{"Return-Path":"","Received":["from mail-ot1-x343.google.com (mail-ot1-x343.google.com\n\t[IPv6:2607:f8b0:4864:20::343])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9680560B2D\n\tfor ;\n\tMon, 28 Jan 2019 08:53:53 +0100 (CET)","by mail-ot1-x343.google.com with SMTP id i20so13897920otl.0\n\tfor ;\n\tSun, 27 Jan 2019 23:53:53 -0800 (PST)","from mail-ot1-f52.google.com (mail-ot1-f52.google.com.\n\t[209.85.210.52]) by smtp.gmail.com with ESMTPSA id\n\tk11sm5142171oib.35.2019.01.27.23.53.50\n\tfor \n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tSun, 27 Jan 2019 23:53:50 -0800 (PST)","by mail-ot1-f52.google.com with SMTP id s5so13849217oth.7\n\tfor ;\n\tSun, 27 Jan 2019 23:53:50 -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; bh=X/7hZTx1TklFVUZ8oSxgBA/2B+T5t4CM+a5I2CQTa3M=;\n\tb=AO2Hhu7P4hcK8PtYXB619SDYSfyoDEwO2k7DvcJDJzZETjWiSC2JQ56FT3cPaM42+o\n\tRB7TB8TrXdweoWLZAyOGzZNplh/ZNPRQdChTxg6QahS3Pok0fHxopnrbvFMyrIarsVw0\n\tXHBQrw8XpDw9xU9CUSa4ujZWFEqCnhjYKgSB0=","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;\n\tbh=X/7hZTx1TklFVUZ8oSxgBA/2B+T5t4CM+a5I2CQTa3M=;\n\tb=aDCLNguTl/aFRNFwmHPa3eBrpm94MUQYUsUqLZehEnfclFjjg0Y6m7YgrrjEJZeu3Y\n\tq/uwVn0doNgjfFhLjQVKwkJMhPswx9VyJT3MZM7LOXCodG2rKycO0rsxRD49mZnl2pD7\n\twfXULjHAyknivDHNbachRDL05i1j9Ed4SaIjRgz4t52VYVfXNsIiXVY222OQGZdBTTMN\n\t4YrlLJtmyNHKb1anrbv/b6QhjTjIE50ZGzOgk1EIhli8V2D5+NHN9XllAGQFhzwascsq\n\tPeaY+2yZkHfYEQAEhxezdLWEnm/sNZMZcUYH2Oq/9x+TlqhFXze1mGranQ9nrQnr31MS\n\tARDg==","X-Gm-Message-State":"AJcUukdRrCbmQwnKOJb9XguXtRjSZ+3h5mj7CpBsfklJQUtNXvZ3/k87\n\tSR+YcCTN3xR2cL+npWd9XH3jibEIToQ=","X-Google-Smtp-Source":"ALg8bN6kRJ0vFIVxMXslQYpMS2qd3pjO7Tebcs4mlIuKDHqy6Sy2oHiH88U1v/ZeI7WGD8QRqOBJfA==","X-Received":["by 2002:a9d:75c5:: with SMTP id\n\tc5mr10370377otl.356.1548662031707; \n\tSun, 27 Jan 2019 23:53:51 -0800 (PST)","by 2002:a9d:6f8e:: with SMTP id\n\th14mr15016798otq.241.1548662030273; \n\tSun, 27 Jan 2019 23:53:50 -0800 (PST)"],"MIME-Version":"1.0","References":"<20190117235916.1906-1-laurent.pinchart@ideasonboard.com>\n\t<20190117235916.1906-5-laurent.pinchart@ideasonboard.com>\n\t\n\t<20190118225156.GB1799@pendragon.ideasonboard.com>\n\t\n\t<20190122122212.GA4455@pendragon.ideasonboard.com>\n\t\n\t<20190125103357.GA2934@pendragon.ideasonboard.com>","In-Reply-To":"<20190125103357.GA2934@pendragon.ideasonboard.com>","From":"Tomasz Figa ","Date":"Mon, 28 Jan 2019 16:53:39 +0900","X-Gmail-Original-Message-ID":"","Message-ID":"","To":"Laurent Pinchart ","Cc":"Ricky Liang , libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: camera: Handle camera\n\tobjects through shared pointers","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, 28 Jan 2019 07:53:54 -0000"}},{"id":705,"web_url":"https://patchwork.libcamera.org/comment/705/","msgid":"<20190130221754.GB4766@pendragon.ideasonboard.com>","date":"2019-01-30T22:17:54","subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: camera: Handle camera\n\tobjects through shared pointers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Tomasz,\n\nOn Mon, Jan 28, 2019 at 04:53:39PM +0900, Tomasz Figa wrote:\n> On Fri, Jan 25, 2019 at 7:34 PM Laurent Pinchart\n> wrote:\n> > On Fri, Jan 25, 2019 at 03:55:12PM +0900, Tomasz Figa wrote:\n> >> On Tue, Jan 22, 2019 at 9:22 PM Laurent Pinchart\n> >> wrote:\n> >>> On Tue, Jan 22, 2019 at 11:19:01AM +0800, Ricky Liang wrote:\n> >>>> On Sat, Jan 19, 2019 at 6:51 AM Laurent Pinchart\n> >>>> wrote:\n> >>>>> On Sat, Jan 19, 2019 at 01:39:35AM +0800, Ricky Liang wrote:\n> >>>>>> On Fri, Jan 18, 2019 at 7:59 AM Laurent Pinchart\n> >>>>>> wrote:\n> >>>>>>>\n> >>>>>>> The Camera class is explicitly reference-counted to manage the lifetime\n> >>>>>>> of camera objects. Replace this open-coded implementation with usage of\n> >>>>>>> the std::shared_ptr<> class.\n> >>>>>>>\n> >>>>>>> Signed-off-by: Laurent Pinchart \n> >>>>>>> ---\n> >>>>>>> include/libcamera/camera.h | 13 +++++----\n> >>>>>>> include/libcamera/camera_manager.h | 8 +++---\n> >>>>>>> src/libcamera/camera.cpp | 46 +++++++++++++++++-------------\n> >>>>>>> src/libcamera/camera_manager.cpp | 20 ++++++-------\n> >>>>>>> src/libcamera/pipeline/vimc.cpp | 2 +-\n> >>>>>>> test/list-cameras.cpp | 2 +-\n> >>>>>>> 6 files changed, 50 insertions(+), 41 deletions(-)\n> >>>>>>>\n> >>>>>>> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> >>>>>>> index 9a7579d61fa3..ef0a794e3a82 100644\n> >>>>>>> --- a/include/libcamera/camera.h\n> >>>>>>> +++ b/include/libcamera/camera.h\n> >>>>>>> @@ -7,6 +7,7 @@\n> >>>>>>> #ifndef __LIBCAMERA_CAMERA_H__\n> >>>>>>> #define __LIBCAMERA_CAMERA_H__\n> >>>>>>>\n> >>>>>>> +#include \n> >>>>>>> #include \n> >>>>>>>\n> >>>>>>> namespace libcamera {\n> >>>>>>> @@ -14,15 +15,17 @@ namespace libcamera {\n> >>>>>>> class Camera\n> >>>>>>> {\n> >>>>>>> public:\n> >>>>>>> - Camera(const std::string &name);\n> >>>>>>> + static std::shared_ptr create(const std::string &name);\n> >>>>>>> +\n> >>>>>>> + Camera(const Camera &) = delete;\n> >>>>>>> + void operator=(const Camera &) = delete;\n> >>>>>>\n> >>>>>> We probably want to delete the implicit constructor as well:\n> >>>>>>\n> >>>>>> Camera() = delete;\n> >>>>>\n> >>>>> The compiler only creates an implicit constructor when no explicit\n> >>>>> constructor is specified. As we define Camera(const std::string &name),\n> >>>>> there's no need to delete the implicit constructor.\n> >>>>\n> >>>> Ack.\n> >>>>\n> >>>>>>>\n> >>>>>>> const std::string &name() const;\n> >>>>>>> - void get();\n> >>>>>>> - void put();\n> >>>>>>>\n> >>>>>>> private:\n> >>>>>>> - virtual ~Camera() { };\n> >>>>>>> - int ref_;\n> >>>>>>> + Camera(const std::string &name);\n> >>>>>>\n> >>>>>> Can we add the 'explicit' specifier for constructor with only one\n> >>>>>> argument? Not super important for this patch as it's a private\n> >>>>>> constructor here, but may be a good general guideline.\n> >>>>>\n> >>>>> Good point, I'll do that.\n> >>>>>\n> >>>>>>> + ~Camera();\n> >>>>>>> +\n> >>>>>>> std::string name_;\n> >>>>>>> };\n> >>>>>>>\n> >>>>>>> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h\n> >>>>>>> index 4b941fd9183b..738b13f4cfb1 100644\n> >>>>>>> --- a/include/libcamera/camera_manager.h\n> >>>>>>> +++ b/include/libcamera/camera_manager.h\n> >>>>>>> @@ -24,10 +24,10 @@ public:\n> >>>>>>> int start();\n> >>>>>>> void stop();\n> >>>>>>>\n> >>>>>>> - const std::vector &cameras() const { return cameras_; }\n> >>>>>>> - Camera *get(const std::string &name);\n> >>>>>>> + const std::vector> &cameras() const { return cameras_; }\n> >>>>>>> + std::shared_ptr get(const std::string &name);\n> >>>>>>\n> >>>>>> Some high level design questions:\n> >>>>>>\n> >>>>>> 1. Who are the callers of these getter methods?\n> >>>>>\n> >>>>> These methods are meant to be called by applications. I expect the API\n> >>>>> to change as we continue developing though, but the general idea will\n> >>>>> stay the same.\n> >>>>>\n> >>>>>> 2. What's the threading model that we plan to use for exercising the\n> >>>>>> Camera objects?\n> >>>>>\n> >>>>> We don't support multiple threads yet. This will be fixed down the road,\n> >>>>> but not all details have been decided yet.\n> >>>>>\n> >>>>>> While making Camera objects as shared pointers makes the coding\n> >>>>>> easier, it may also be hard to track the ownership of the objects once\n> >>>>>> the code base gets large. In some cases it may lead to memory/fd leaks\n> >>>>>> that are hard to debug, e.g. a singleton object holding a shared\n> >>>>>> pointer reference to an object and never releases the reference.\n> >>>>>>\n> >>>>>> I tend to agree that fundamental objects like Camera has the necessity\n> >>>>>> to be shared pointers, but would also like to know if it's possible to\n> >>>>>> design the threading model such that CameraManager is the main owner\n> >>>>>> of the Camera objects. For example, if we can design it such that all\n> >>>>>> the access to Camera objects is serialized on a thread,\n> >>>>>\n> >>>>> I think they will, possibly with the exception of request queuing and\n> >>>>> request completion handling. This remains to be decided. In any case, I\n> >>>>> don't think we should support camera lookup from multiple threads.\n> >>>>>\n> >>>>>> then we probably don't need to use shared pointers. Or, we can achieve\n> >>>>>> concurrency by storing Camera objects as std::shared_ptr in\n> >>>>>> CameraManager and only giving out std::weak_ptr of the Camera objects.\n> >>>>>\n> >>>>> It's not just a concurrency issue. Support for hot-pluggable cameras\n> >>>>> will be added in the future, and we'll need to track users of camera\n> >>>>> objects in order to delete the camera only when no users are left. This\n> >>>>> doesn't require std::shared_ptr<>, a manual reference count would work\n> >>>>> too (and it exists today, this patch is replacing it), but I think it's\n> >>>>> better to use std::shared_ptr<> rather than reinventing (and\n> >>>>> open-coding) the wheel.\n> >>>>\n> >>>> I'd imagine in the hot-pluggable camera case, the action of actually\n> >>>> removing the camera (e.g. un-plugging the USB camera) would make the\n> >>>> camera no longer usable and what we need would be to handle the\n> >>>> exception gracefully.\n> >>>\n> >>> Correct, the camera object would be marked as disconnected, API calls\n> >>> would start failing, and when the camera is released by the application\n> >>> it will be deleted.\n> >>>\n> >>>> Or, do you mean hot-pluggable in the software sense, like the\n> >>>> bind/unbind in kernel drivers? Most of the camera devices allow only\n> >>>> one active user at a time. Do we plan to design the APIs to allow\n> >>>> concurrent access to a camera?\n> >>>\n> >>> I meant hotplugging a USB webcam for instance, but from a userspace\n> >>> point of view unbinding a device from its kernel driver should achieve\n> >>> the same effect. I'd like pipeline handlers to handle that gracefully if\n> >>> it happens, but it's not a priority.\n> >>>\n> >>> We don't plan to support concurrent access to a camera by multiple\n> >>> applications, that would be handled by a layer above libcamera if\n> >>> needed.\n> >>\n> >> So I'm actually wondering if we're trying to solve a real problem\n> >> here. Physical camera hotplug is already handled for us by the kernel.\n> >> As long as the userspace has the V4L2 file descriptors open, it can\n> >> interact with them and get error codes when the camera goes away. We\n> >> could just put our internal objects in some error state, fail any\n> >> pending capture requests and have any further calls on this Camera\n> >> object just fail, in a way that would explicitly tell the consumer\n> >> that the reason for the failure was disconnection. Then it would be up\n> >> to the consumer to actually destroy the Camera object.\n> >\n> > One core design principle in libcamera is that the library enumerates\n> > cameras by scanning for media devices (in the MC sense) and associating\n> > them with pipeline handlers (which can be seen as a kind of userspace\n> > counterpart of the kernel drivers, exposing various MC device nodes as\n> > higher level camera objects, the same way a kernel driver exposes\n> > various pieces of hardware as standardized device nodes). The library is\n> > thus responsible for creating the camera objects, which are then\n> > retrieved and used by applications. In the context of hotplug handling\n> > we need to signal hot-unplug to applications, certainly in the form of\n> > completing pending capture requests with an error and failing futher\n> > calls, but I think also as an explicit signal to the application. For\n> > this to work I believe handling camera objects as shared pointers is the\n> > best option, as the consumer has to release the object, but shouldn't\n> > destroy it. Am I biased ? :-)\n> \n> I'd give the consumer an object it can destroy, which only points to\n> the camera object created by the framework. That would eliminate the\n> shared pointers from the public API, but still give you the ability to\n> use them internally.\n\nThat's an interesting idea. I'll give it a try, even if it means I'll\nhave to find two class names for the external and internal camera\nobjects :-)\n\n> For any signals to the application, we need some event loop on the\n> application side anyway, which would poll the object for messages, so\n> that should work as well with this kind of \"consumer\" object.\n\nlibcamera already contains an event dispatcher API that allows the\nlibrary to plug itself in the application event loop (with a default\npoll-based event loop provided by libcamera itself to ease application\ndevelopment), and a signal/slot mechanism inspired by Qt. I think that\nwill be enough to signal camera disconnection, even though more work\nwill likely be needed to support multi-threaded applications in this\nregard.","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 88CE560C78\n\tfor ;\n\tWed, 30 Jan 2019 23:18:00 +0100 (CET)","from pendragon.ideasonboard.com (85-76-34-136-nat.elisa-mobile.fi\n\t[85.76.34.136])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 534B941;\n\tWed, 30 Jan 2019 23:17:57 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1548886680;\n\tbh=7bfo/JI5/b3+WInT1tV2xZmI10dZLLqaOkGfMtrM0yY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gFcaHR0lrBWO1YB/I2t7FPqfZ3jy/H82PI70ooMqr/fNpZHfbmVsZK0KcblNwPPig\n\t2ABnn8UA9f6drs2FaJlSiBUPT/AW4zIe48psan4ICfM7OPIf9EbJtx/+BVXTb2KNmp\n\t5GT1pwYkHllEiny4w0onZmVe25cgN42Rx+j+ybrI=","Date":"Thu, 31 Jan 2019 00:17:54 +0200","From":"Laurent Pinchart ","To":"Tomasz Figa ","Cc":"Ricky Liang , libcamera-devel@lists.libcamera.org","Message-ID":"<20190130221754.GB4766@pendragon.ideasonboard.com>","References":"<20190117235916.1906-1-laurent.pinchart@ideasonboard.com>\n\t<20190117235916.1906-5-laurent.pinchart@ideasonboard.com>\n\t\n\t<20190118225156.GB1799@pendragon.ideasonboard.com>\n\t\n\t<20190122122212.GA4455@pendragon.ideasonboard.com>\n\t\n\t<20190125103357.GA2934@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: camera: Handle camera\n\tobjects through shared pointers","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, 30 Jan 2019 22:18:00 -0000"}}]