Patch Detail
Show a patch.
GET /api/patches/1850/?format=api
{ "id": 1850, "url": "https://patchwork.libcamera.org/api/patches/1850/?format=api", "web_url": "https://patchwork.libcamera.org/patch/1850/", "project": { "id": 1, "url": "https://patchwork.libcamera.org/api/projects/1/?format=api", "name": "libcamera", "link_name": "libcamera", "list_id": "libcamera_core", "list_email": "libcamera-devel@lists.libcamera.org", "web_url": "", "scm_url": "", "webscm_url": "" }, "msgid": "<20190818011329.14499-14-laurent.pinchart@ideasonboard.com>", "date": "2019-08-18T01:13:28", "name": "[libcamera-devel,13/14] libcamera: camera_manager: Construct CameraManager instances manually", "commit_ref": null, "pull_url": null, "state": "accepted", "archived": false, "hash": "85b847d30e31148f3aa622b1cb80897cba6d46bc", "submitter": { "id": 2, "url": "https://patchwork.libcamera.org/api/people/2/?format=api", "name": "Laurent Pinchart", "email": "laurent.pinchart@ideasonboard.com" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/1850/mbox/", "series": [ { "id": 464, "url": "https://patchwork.libcamera.org/api/series/464/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=464", "date": "2019-08-18T01:13:15", "name": "Assorted fixes for Android camera HAL", "version": 1, "mbox": "https://patchwork.libcamera.org/series/464/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/1850/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/1850/checks/", "tags": {}, "headers": { "Return-Path": "<laurent.pinchart@ideasonboard.com>", "Received": [ "from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7E67860BE5\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 18 Aug 2019 03:13:45 +0200 (CEST)", "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 0BC622AF\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 18 Aug 2019 03:13:44 +0200 (CEST)" ], "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1566090825;\n\tbh=Ozo/tS4KTXpJmvT8yIP7zAbZeCOkEqc3vN922bFYEZ8=;\n\th=From:To:Subject:Date:In-Reply-To:References:From;\n\tb=BT3AYMeJDWblWlRna+Xo710fYwXe2/K9CjyTpAgiRB3dzNum/lGWizHmVkc0SISGD\n\tIFFLi9af21qnAk6SHCqnpWRIy4gQKDMfDT+57raNi5ZG6CqaUWHFcNtUA4valmcDkY\n\tSyA8Hjrd8/CwYbdDTgy0Peqw4HbkYA1yXh5eOR3c=", "From": "Laurent Pinchart <laurent.pinchart@ideasonboard.com>", "To": "libcamera-devel@lists.libcamera.org", "Date": "Sun, 18 Aug 2019 04:13:28 +0300", "Message-Id": "<20190818011329.14499-14-laurent.pinchart@ideasonboard.com>", "X-Mailer": "git-send-email 2.21.0", "In-Reply-To": "<20190818011329.14499-1-laurent.pinchart@ideasonboard.com>", "References": "<20190818011329.14499-1-laurent.pinchart@ideasonboard.com>", "MIME-Version": "1.0", "Content-Transfer-Encoding": "8bit", "Subject": "[libcamera-devel] [PATCH 13/14] libcamera: camera_manager:\n\tConstruct CameraManager instances manually", "X-BeenThere": "libcamera-devel@lists.libcamera.org", "X-Mailman-Version": "2.1.23", "Precedence": "list", "List-Id": "<libcamera-devel.lists.libcamera.org>", "List-Unsubscribe": "<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>", "List-Archive": "<https://lists.libcamera.org/pipermail/libcamera-devel/>", "List-Post": "<mailto:libcamera-devel@lists.libcamera.org>", "List-Help": "<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>", "List-Subscribe": "<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>", "X-List-Received-Date": "Sun, 18 Aug 2019 01:13:46 -0000" }, "content": "The CameraManager class is not supposed to be instantiated multiple\ntimes, which led to a singleton implementation. This requires a global\ninstance of the CameraManager, which is destroyed when the global\ndestructors are executed.\n\nRelying on global instances causes issues with cleanup, as the order in\nwhich the global destructors are run can't be controlled. In particular,\nthe Android camera HAL implementation ends up destroying the\nCameraHalManager after the CameraManager, which leads to use-after-free\nproblems.\n\nTo solve this, remove the CameraManager::instance() method and make the\nCameraManager class instantiable directly. Multiple instances are still\nnot allowed, and this is enforced by storing the instance pointer\ninternally to be checked when an instance is created.\n\nSigned-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n---\n include/libcamera/camera_manager.h | 12 ++++----\n src/android/camera_hal_manager.cpp | 5 +++-\n src/cam/main.cpp | 8 ++++-\n src/libcamera/camera_manager.cpp | 36 ++++++++++-------------\n src/qcam/main.cpp | 4 ++-\n test/camera/camera_test.cpp | 3 +-\n test/controls/control_list.cpp | 3 +-\n test/list-cameras.cpp | 11 +++----\n test/pipeline/ipu3/ipu3_pipeline_test.cpp | 3 +-\n 9 files changed, 48 insertions(+), 37 deletions(-)", "diff": "diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h\nindex ff7d4c7c6745..8331898c4219 100644\n--- a/include/libcamera/camera_manager.h\n+++ b/include/libcamera/camera_manager.h\n@@ -23,6 +23,11 @@ class PipelineHandler;\n class CameraManager : public Object\n {\n public:\n+\tCameraManager();\n+\tCameraManager(const CameraManager &) = delete;\n+\tCameraManager &operator=(const CameraManager &) = delete;\n+\t~CameraManager();\n+\n \tint start();\n \tvoid stop();\n \n@@ -32,23 +37,18 @@ public:\n \tvoid addCamera(std::shared_ptr<Camera> camera);\n \tvoid removeCamera(Camera *camera);\n \n-\tstatic CameraManager *instance();\n \tstatic const std::string &version() { return version_; }\n \n \tvoid setEventDispatcher(std::unique_ptr<EventDispatcher> dispatcher);\n \tEventDispatcher *eventDispatcher();\n \n private:\n-\tCameraManager();\n-\tCameraManager(const CameraManager &) = delete;\n-\tCameraManager &operator=(const CameraManager &) = delete;\n-\t~CameraManager();\n-\n \tstd::unique_ptr<DeviceEnumerator> enumerator_;\n \tstd::vector<std::shared_ptr<PipelineHandler>> pipes_;\n \tstd::vector<std::shared_ptr<Camera>> cameras_;\n \n \tstatic const std::string version_;\n+\tstatic CameraManager *self_;\n };\n \n } /* namespace libcamera */\ndiff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\nindex cf981720bca4..22f0323b3ff0 100644\n--- a/src/android/camera_hal_manager.cpp\n+++ b/src/android/camera_hal_manager.cpp\n@@ -59,7 +59,7 @@ void CameraHalManager::run()\n \t * order to bind them to the camera HAL manager thread that\n \t * executes the event dispatcher.\n \t */\n-\tcameraManager_ = libcamera::CameraManager::instance();\n+\tcameraManager_ = new CameraManager();\n \n \tint ret = cameraManager_->start();\n \tif (ret) {\n@@ -93,7 +93,10 @@ void CameraHalManager::run()\n \n \t/* Clean up the resources we have allocated. */\n \tproxies_.clear();\n+\n \tcameraManager_->stop();\n+\tdelete cameraManager_;\n+\tcameraManager_ = nullptr;\n }\n \n CameraProxy *CameraHalManager::open(unsigned int id,\ndiff --git a/src/cam/main.cpp b/src/cam/main.cpp\nindex 77bb20e9622e..9d99f5587cbb 100644\n--- a/src/cam/main.cpp\n+++ b/src/cam/main.cpp\n@@ -23,6 +23,7 @@ class CamApp\n {\n public:\n \tCamApp();\n+\t~CamApp();\n \n \tstatic CamApp *instance();\n \n@@ -54,6 +55,11 @@ CamApp::CamApp()\n \tCamApp::app_ = this;\n }\n \n+CamApp::~CamApp()\n+{\n+\tdelete cm_;\n+}\n+\n CamApp *CamApp::instance()\n {\n \treturn CamApp::app_;\n@@ -67,7 +73,7 @@ int CamApp::init(int argc, char **argv)\n \tif (ret < 0)\n \t\treturn ret;\n \n-\tcm_ = CameraManager::instance();\n+\tcm_ = new CameraManager();\n \n \tret = cm_->start();\n \tif (ret) {\ndiff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\nindex 4a880684c5cb..12cb5a0be859 100644\n--- a/src/libcamera/camera_manager.cpp\n+++ b/src/libcamera/camera_manager.cpp\n@@ -35,11 +35,14 @@ LOG_DEFINE_CATEGORY(Camera)\n * in the system to applications. The manager owns all Camera objects and\n * handles hot-plugging and hot-unplugging to manage the lifetime of cameras.\n *\n- * To interact with libcamera, an application retrieves the camera manager\n- * instance with CameraManager::instance(). The manager is initially stopped,\n- * and shall be configured before being started. In particular a custom event\n- * dispatcher shall be installed if needed with\n- * CameraManager::setEventDispatcher().\n+ * To interact with libcamera, an application starts by creating a camera\n+ * manager instance. Only a single instance of the camera manager may exist at\n+ * a time. Attempting to create a second instance without first deleting the\n+ * existing instance results in undefined behaviour.\n+ *\n+ * The manager is initially stopped, and shall be configured before being\n+ * started. In particular a custom event dispatcher shall be installed if\n+ * needed with CameraManager::setEventDispatcher().\n *\n * Once the camera manager is configured, it shall be started with start().\n * This will enumerate all the cameras present in the system, which can then be\n@@ -56,13 +59,21 @@ LOG_DEFINE_CATEGORY(Camera)\n * removed due to hot-unplug.\n */\n \n+CameraManager *CameraManager::self_ = nullptr;\n+\n CameraManager::CameraManager()\n \t: enumerator_(nullptr)\n {\n+\tif (self_)\n+\t\tLOG(Camera, Fatal)\n+\t\t\t<< \"Multiple CameraManager objects are not allowed\";\n+\n+\tself_ = this;\n }\n \n CameraManager::~CameraManager()\n {\n+\tself_ = nullptr;\n }\n \n /**\n@@ -212,21 +223,6 @@ void CameraManager::removeCamera(Camera *camera)\n \t}\n }\n \n-/**\n- * \\brief Retrieve the camera manager instance\n- *\n- * The CameraManager is a singleton and can't be constructed manually. This\n- * function shall instead be used to retrieve the single global instance of the\n- * manager.\n- *\n- * \\return The camera manager instance\n- */\n-CameraManager *CameraManager::instance()\n-{\n-\tstatic CameraManager manager;\n-\treturn &manager;\n-}\n-\n /**\n * \\fn const std::string &CameraManager::version()\n * \\brief Retrieve the libcamera version string\ndiff --git a/src/qcam/main.cpp b/src/qcam/main.cpp\nindex 05d3b77e9edb..a7ff5c52663b 100644\n--- a/src/qcam/main.cpp\n+++ b/src/qcam/main.cpp\n@@ -63,7 +63,7 @@ int main(int argc, char **argv)\n \tsigaction(SIGINT, &sa, nullptr);\n \n \tstd::unique_ptr<EventDispatcher> dispatcher(new QtEventDispatcher());\n-\tCameraManager *cm = CameraManager::instance();\n+\tCameraManager *cm = new CameraManager();\n \tcm->setEventDispatcher(std::move(dispatcher));\n \n \tret = cm->start();\n@@ -79,5 +79,7 @@ int main(int argc, char **argv)\n \tdelete mainWindow;\n \n \tcm->stop();\n+\tdelete cm;\n+\n \treturn ret;\n }\ndiff --git a/test/camera/camera_test.cpp b/test/camera/camera_test.cpp\nindex 24ff5fe0c64d..0e105414bf46 100644\n--- a/test/camera/camera_test.cpp\n+++ b/test/camera/camera_test.cpp\n@@ -14,7 +14,7 @@ using namespace std;\n \n int CameraTest::init()\n {\n-\tcm_ = CameraManager::instance();\n+\tcm_ = new CameraManager();\n \n \tif (cm_->start()) {\n \t\tcout << \"Failed to start camera manager\" << endl;\n@@ -44,4 +44,5 @@ void CameraTest::cleanup()\n \t}\n \n \tcm_->stop();\n+\tdelete cm_;\n };\ndiff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp\nindex c834edc352f5..f1d79ff8fcfd 100644\n--- a/test/controls/control_list.cpp\n+++ b/test/controls/control_list.cpp\n@@ -21,7 +21,7 @@ class ControlListTest : public Test\n protected:\n \tint init()\n \t{\n-\t\tcm_ = CameraManager::instance();\n+\t\tcm_ = new CameraManager();\n \n \t\tif (cm_->start()) {\n \t\t\tcout << \"Failed to start camera manager\" << endl;\n@@ -203,6 +203,7 @@ protected:\n \t\t}\n \n \t\tcm_->stop();\n+\t\tdelete cm_;\n \t}\n \n private:\ndiff --git a/test/list-cameras.cpp b/test/list-cameras.cpp\nindex 070cbf2be977..55551d7e7e10 100644\n--- a/test/list-cameras.cpp\n+++ b/test/list-cameras.cpp\n@@ -20,8 +20,8 @@ class ListTest : public Test\n protected:\n \tint init()\n \t{\n-\t\tcm = CameraManager::instance();\n-\t\tcm->start();\n+\t\tcm_ = new CameraManager();\n+\t\tcm_->start();\n \n \t\treturn 0;\n \t}\n@@ -30,7 +30,7 @@ protected:\n \t{\n \t\tunsigned int count = 0;\n \n-\t\tfor (const std::shared_ptr<Camera> &camera : cm->cameras()) {\n+\t\tfor (const std::shared_ptr<Camera> &camera : cm_->cameras()) {\n \t\t\tcout << \"- \" << camera->name() << endl;\n \t\t\tcount++;\n \t\t}\n@@ -40,11 +40,12 @@ protected:\n \n \tvoid cleanup()\n \t{\n-\t\tcm->stop();\n+\t\tcm_->stop();\n+\t\tdelete cm_;\n \t}\n \n private:\n-\tCameraManager *cm;\n+\tCameraManager *cm_;\n };\n \n TEST_REGISTER(ListTest)\ndiff --git a/test/pipeline/ipu3/ipu3_pipeline_test.cpp b/test/pipeline/ipu3/ipu3_pipeline_test.cpp\nindex 1d4cc4d4950b..8bfcd609a071 100644\n--- a/test/pipeline/ipu3/ipu3_pipeline_test.cpp\n+++ b/test/pipeline/ipu3/ipu3_pipeline_test.cpp\n@@ -92,7 +92,7 @@ int IPU3PipelineTest::init()\n \n \tenumerator.reset(nullptr);\n \n-\tcameraManager_ = CameraManager::instance();\n+\tcameraManager_ = new CameraManager();\n \tret = cameraManager_->start();\n \tif (ret) {\n \t\tcerr << \"Failed to start the CameraManager\" << endl;\n@@ -120,6 +120,7 @@ int IPU3PipelineTest::run()\n void IPU3PipelineTest::cleanup()\n {\n \tcameraManager_->stop();\n+\tdelete cameraManager_;\n }\n \n TEST_REGISTER(IPU3PipelineTest)\n", "prefixes": [ "libcamera-devel", "13/14" ] }