{"id":358,"url":"https://patchwork.libcamera.org/api/1.1/patches/358/?format=json","web_url":"https://patchwork.libcamera.org/patch/358/","project":{"id":1,"url":"https://patchwork.libcamera.org/api/1.1/projects/1/?format=json","name":"libcamera","link_name":"libcamera","list_id":"libcamera_core","list_email":"libcamera-devel@lists.libcamera.org","web_url":"","scm_url":"","webscm_url":""},"msgid":"<20190124101651.9993-4-laurent.pinchart@ideasonboard.com>","date":"2019-01-24T10:16:44","name":"[libcamera-devel,03/10] libcamera: camera: Associate cameras with their pipeline handler","commit_ref":null,"pull_url":null,"state":"accepted","archived":false,"hash":"17875290e7d3c38c6c5608c36df9875666334621","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/1.1/people/2/?format=json","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"delegate":null,"mbox":"https://patchwork.libcamera.org/patch/358/mbox/","series":[{"id":125,"url":"https://patchwork.libcamera.org/api/1.1/series/125/?format=json","web_url":"https://patchwork.libcamera.org/project/libcamera/list/?series=125","date":"2019-01-24T10:16:41","name":"Hotplug support and object lifetime management","version":1,"mbox":"https://patchwork.libcamera.org/series/125/mbox/"}],"comments":"https://patchwork.libcamera.org/api/patches/358/comments/","check":"pending","checks":"https://patchwork.libcamera.org/api/patches/358/checks/","tags":{},"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 035E760C78\n\tfor ;\n\tThu, 24 Jan 2019 11:16:58 +0100 (CET)","from pendragon.bb.dnainternet.fi\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 8D94623A;\n\tThu, 24 Jan 2019 11:16:57 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1548325017;\n\tbh=6gV9aidWQeYC32QKe+bYH03vMjAHzkuR/hfxsbBAwoA=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=g0blyj8aHBAmVs5BwTA8+IeCpDndUF0OztU7t0HFRO4+cRfEXJt75tmy4JjkrzfoU\n\t6bQbBlbwV4WTPGqH3AMvvpfNEEGyvIlkww+r0vESWTdE867IIUbcEcbhtDebdfS2Is\n\tIC3O4BBt2dHsZjgYX/Z65mT4aniZL5mTTjSzBNAc=","From":"Laurent Pinchart ","To":"libcamera-devel@lists.libcamera.org","Date":"Thu, 24 Jan 2019 12:16:44 +0200","Message-Id":"<20190124101651.9993-4-laurent.pinchart@ideasonboard.com>","X-Mailer":"git-send-email 2.19.2","In-Reply-To":"<20190124101651.9993-1-laurent.pinchart@ideasonboard.com>","References":"<20190124101651.9993-1-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"8bit","Subject":"[libcamera-devel] [PATCH 03/10] libcamera: camera: Associate\n\tcameras with their pipeline handler","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":"Thu, 24 Jan 2019 10:16:58 -0000"},"content":"From: Niklas Söderlund \n\nThe PipelineHandler which creates a Camera is responsible for serving\nany operation requested by the user. In order forward the public API\ncalls, the camera needs to store a reference to its pipeline handler.\n\nSigned-off-by: Niklas Söderlund \nSigned-off-by: Laurent Pinchart \n---\nChanges since v1:\n\n- Create pipeline handlers is shared pointers, make them inherit from\n std::enable_shared_from_this<> and stored them in shared pointers.\n---\n include/libcamera/camera.h | 8 ++++++--\n include/libcamera/camera_manager.h | 2 +-\n src/libcamera/camera.cpp | 16 ++++++++++------\n src/libcamera/camera_manager.cpp | 17 +++++++++--------\n src/libcamera/include/pipeline_handler.h | 9 +++++----\n src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n src/libcamera/pipeline/uvcvideo.cpp | 2 +-\n src/libcamera/pipeline/vimc.cpp | 9 +--------\n src/libcamera/pipeline_handler.cpp | 10 ++++++++++\n 9 files changed, 44 insertions(+), 31 deletions(-)","diff":"diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\nindex 2ea1a6883311..efafb9e28c56 100644\n--- a/include/libcamera/camera.h\n+++ b/include/libcamera/camera.h\n@@ -12,10 +12,13 @@\n \n namespace libcamera {\n \n+class PipelineHandler;\n+\n class Camera final\n {\n public:\n-\tstatic std::shared_ptr create(const std::string &name);\n+\tstatic std::shared_ptr create(PipelineHandler *pipe,\n+\t\t\t\t\t const std::string &name);\n \n \tCamera(const Camera &) = delete;\n \tvoid operator=(const Camera &) = delete;\n@@ -23,9 +26,10 @@ public:\n \tconst std::string &name() const;\n \n private:\n-\texplicit Camera(const std::string &name);\n+\tCamera(PipelineHandler *pipe, const std::string &name);\n \t~Camera();\n \n+\tstd::shared_ptr pipe_;\n \tstd::string name_;\n };\n \ndiff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h\nindex 9ade29d76692..45e72df0ef65 100644\n--- a/include/libcamera/camera_manager.h\n+++ b/include/libcamera/camera_manager.h\n@@ -41,7 +41,7 @@ private:\n \t~CameraManager();\n \n \tstd::unique_ptr enumerator_;\n-\tstd::vector pipes_;\n+\tstd::vector> pipes_;\n \tstd::vector> cameras_;\n \n \tstd::unique_ptr dispatcher_;\ndiff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\nindex acf912bee95c..3a531c7e4d8f 100644\n--- a/src/libcamera/camera.cpp\n+++ b/src/libcamera/camera.cpp\n@@ -8,6 +8,7 @@\n #include \n \n #include \"log.h\"\n+#include \"pipeline_handler.h\"\n \n /**\n * \\file camera.h\n@@ -52,17 +53,20 @@ namespace libcamera {\n /**\n * \\brief Create a camera instance\n * \\param[in] name The name of the camera device\n+ * \\param[in] pipe The pipeline handler responsible for the camera device\n *\n * The caller is responsible for guaranteeing unicity of the camera name.\n *\n * \\return A shared pointer to the newly created camera object\n */\n-std::shared_ptr Camera::create(const std::string &name)\n+std::shared_ptr Camera::create(PipelineHandler *pipe,\n+\t\t\t\t const std::string &name)\n {\n \tstruct Allocator : std::allocator {\n-\t\tvoid construct(void *p, const std::string &name)\n+\t\tvoid construct(void *p, PipelineHandler *pipe,\n+\t\t\t const std::string &name)\n \t\t{\n-\t\t\t::new(p) Camera(name);\n+\t\t\t::new(p) Camera(pipe, name);\n \t\t}\n \t\tvoid destroy(Camera *p)\n \t\t{\n@@ -70,7 +74,7 @@ std::shared_ptr Camera::create(const std::string &name)\n \t\t}\n \t};\n \n-\treturn std::allocate_shared(Allocator(), name);\n+\treturn std::allocate_shared(Allocator(), pipe, name);\n }\n \n /**\n@@ -83,8 +87,8 @@ const std::string &Camera::name() const\n \treturn name_;\n }\n \n-Camera::Camera(const std::string &name)\n-\t: name_(name)\n+Camera::Camera(PipelineHandler *pipe, const std::string &name)\n+\t: pipe_(pipe->shared_from_this()), name_(name)\n {\n }\n \ndiff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\nindex 14410d4dcda7..3eccf20c4ce9 100644\n--- a/src/libcamera/camera_manager.cpp\n+++ b/src/libcamera/camera_manager.cpp\n@@ -98,16 +98,14 @@ int CameraManager::start()\n \t\t * all pipelines it can provide.\n \t\t */\n \t\twhile (1) {\n-\t\t\tPipelineHandler *pipe = factory->create(this);\n-\t\t\tif (!pipe->match(enumerator_.get())) {\n-\t\t\t\tdelete pipe;\n+\t\t\tstd::shared_ptr pipe = factory->create(this);\n+\t\t\tif (!pipe->match(enumerator_.get()))\n \t\t\t\tbreak;\n-\t\t\t}\n \n \t\t\tLOG(Camera, Debug)\n \t\t\t\t<< \"Pipeline handler \\\"\" << factory->name()\n \t\t\t\t<< \"\\\" matched\";\n-\t\t\tpipes_.push_back(pipe);\n+\t\t\tpipes_.push_back(std::move(pipe));\n \t\t}\n \t}\n \n@@ -130,10 +128,13 @@ void CameraManager::stop()\n {\n \t/* TODO: unregister hot-plug callback here */\n \n-\tfor (PipelineHandler *pipe : pipes_)\n-\t\tdelete pipe;\n-\n+\t/*\n+\t * Release all references to cameras and pipeline handlers to ensure\n+\t * they all get destroyed before the device enumerator deletes the\n+\t * media devices.\n+\t */\n \tpipes_.clear();\n+\tcameras_.clear();\n \n \tenumerator_.reset(nullptr);\n }\ndiff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\nindex 1da6dc758ca6..e1d6369eb0c4 100644\n--- a/src/libcamera/include/pipeline_handler.h\n+++ b/src/libcamera/include/pipeline_handler.h\n@@ -8,6 +8,7 @@\n #define __LIBCAMERA_PIPELINE_HANDLER_H__\n \n #include \n+#include \n #include \n #include \n \n@@ -16,7 +17,7 @@ namespace libcamera {\n class CameraManager;\n class DeviceEnumerator;\n \n-class PipelineHandler\n+class PipelineHandler : public std::enable_shared_from_this\n {\n public:\n \tPipelineHandler(CameraManager *manager);\n@@ -34,7 +35,7 @@ public:\n \tPipelineHandlerFactory(const char *name);\n \tvirtual ~PipelineHandlerFactory() { };\n \n-\tvirtual PipelineHandler *create(CameraManager *manager) = 0;\n+\tvirtual std::shared_ptr create(CameraManager *manager) = 0;\n \n \tconst std::string &name() const { return name_; }\n \n@@ -50,9 +51,9 @@ class handler##Factory final : public PipelineHandlerFactory\t\t\\\n {\t\t\t\t\t\t\t\t\t\\\n public:\t\t\t\t\t\t\t\t\t\\\n \thandler##Factory() : PipelineHandlerFactory(#handler) {}\t\\\n-\tPipelineHandler *create(CameraManager *manager) \t\t\\\n+\tstd::shared_ptr create(CameraManager *manager)\t\\\n \t{\t\t\t\t\t\t\t\t\\\n-\t\treturn new handler(manager);\t\t\t\t\\\n+\t\treturn std::make_shared(manager);\t\t\\\n \t}\t\t\t\t\t\t\t\t\\\n };\t\t\t\t\t\t\t\t\t\\\n static handler##Factory global_##handler##Factory;\ndiff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\nindex 589b3078f301..13ff7da4c99e 100644\n--- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n@@ -171,7 +171,7 @@ void PipelineHandlerIPU3::registerCameras()\n \t\t\tcontinue;\n \n \t\tstd::string cameraName = sensor->name() + \" \" + std::to_string(id);\n-\t\tstd::shared_ptr camera = Camera::create(cameraName);\n+\t\tstd::shared_ptr camera = Camera::create(this, cameraName);\n \t\tmanager_->addCamera(std::move(camera));\n \n \t\tLOG(IPU3, Info)\ndiff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\nindex 92b23da73901..3ebc074093ab 100644\n--- a/src/libcamera/pipeline/uvcvideo.cpp\n+++ b/src/libcamera/pipeline/uvcvideo.cpp\n@@ -48,7 +48,7 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n \n \tdev_->acquire();\n \n-\tstd::shared_ptr camera = Camera::create(dev_->model());\n+\tstd::shared_ptr camera = Camera::create(this, dev_->model());\n \tmanager_->addCamera(std::move(camera));\n \n \treturn true;\ndiff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\nindex f12d007cd956..68bfe9b12ab6 100644\n--- a/src/libcamera/pipeline/vimc.cpp\n+++ b/src/libcamera/pipeline/vimc.cpp\n@@ -57,14 +57,7 @@ bool PipeHandlerVimc::match(DeviceEnumerator *enumerator)\n \n \tdev_->acquire();\n \n-\t/*\n-\t * NOTE: A more complete Camera implementation could\n-\t * be passed the MediaDevice(s) it controls here or\n-\t * a reference to the PipelineHandler. Which method\n-\t * will be chosen depends on how the Camera\n-\t * object is modeled.\n-\t */\n-\tstd::shared_ptr camera = Camera::create(\"Dummy VIMC Camera\");\n+\tstd::shared_ptr camera = Camera::create(this, \"Dummy VIMC Camera\");\n \tmanager_->addCamera(std::move(camera));\n \n \treturn true;\ndiff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\nindex 45788487b5c0..3850ea8fadb5 100644\n--- a/src/libcamera/pipeline_handler.cpp\n+++ b/src/libcamera/pipeline_handler.cpp\n@@ -32,11 +32,21 @@ LOG_DEFINE_CATEGORY(Pipeline)\n *\n * The PipelineHandler matches the media devices provided by a DeviceEnumerator\n * with the pipelines it supports and creates corresponding Camera devices.\n+ *\n+ * Pipeline handler instances are reference-counted through std::shared_ptr<>.\n+ * They implement std::enable_shared_from_this<> in order to create new\n+ * std::shared_ptr<> in code paths originating from member functions of the\n+ * PipelineHandler class where only the 'this' pointer is available.\n */\n \n /**\n * \\brief Construct a PipelineHandler instance\n * \\param[in] manager The camera manager\n+ *\n+ * In order to honour the std::enable_shared_from_this<> contract,\n+ * PipelineHandler instances shall never be constructed manually, but always\n+ * through the PipelineHandlerFactory::create() method implemented by the\n+ * respective factories.\n */\n PipelineHandler::PipelineHandler(CameraManager *manager)\n \t: manager_(manager)\n","prefixes":["libcamera-devel","03/10"]}