Patch Detail
Show a patch.
GET /api/1.1/patches/4064/?format=api
{ "id": 4064, "url": "https://patchwork.libcamera.org/api/1.1/patches/4064/?format=api", "web_url": "https://patchwork.libcamera.org/patch/4064/", "project": { "id": 1, "url": "https://patchwork.libcamera.org/api/1.1/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": "<20200616194523.23268-2-email@uajain.com>", "date": "2020-06-16T19:45:33", "name": "[libcamera-devel,v5,1/6] libcamera: CameraManager: Drop the vector of created PipelineHandlers", "commit_ref": "e34d3a4f278ec8491ce3b710e989fb2083af92f3", "pull_url": null, "state": "accepted", "archived": false, "hash": "56b8d76234049d98137214ead7bc605a340c534f", "submitter": { "id": 1, "url": "https://patchwork.libcamera.org/api/1.1/people/1/?format=api", "name": "Umang Jain", "email": "email@uajain.com" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/4064/mbox/", "series": [ { "id": 1009, "url": "https://patchwork.libcamera.org/api/1.1/series/1009/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=1009", "date": "2020-06-16T19:45:33", "name": "Introduce UVC hotplugging support", "version": 5, "mbox": "https://patchwork.libcamera.org/series/1009/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/4064/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/4064/checks/", "tags": {}, "headers": { "Return-Path": "<bounces+15657259-5c31-libcamera-devel=lists.libcamera.org@em7280.uajain.com>", "Received": [ "from o1.f.az.sendgrid.net (o1.f.az.sendgrid.net [208.117.55.132])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 01F0261167\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 16 Jun 2020 21:45:34 +0200 (CEST)", "by filterdrecv-p3mdw1-6f5df8956d-4d2bg with SMTP id\n\tfilterdrecv-p3mdw1-6f5df8956d-4d2bg-21-5EE9215D-2A\n\t2020-06-16 19:45:33.47457207 +0000 UTC m=+1121502.324567696", "from mail.uajain.com (unknown)\n\tby ismtpd0004p1maa1.sendgrid.net (SG) with ESMTP\n\tid _BnG-p6kSZuoZoFr7UImzA Tue, 16 Jun 2020 19:45:33.074 +0000 (UTC)" ], "Authentication-Results": "lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=uajain.com\n\theader.i=@uajain.com header.b=\"VED8T0Gc\"; \n\tdkim-atps=neutral", "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/relaxed; d=uajain.com;\n\th=from:subject:in-reply-to:references:mime-version:to:cc:content-type:\n\tcontent-transfer-encoding;\n\ts=s1; bh=yx6dU7ckU6ShtvwUOxgtaiD4W7+suRpe2YocURNhkf8=;\n\tb=VED8T0GcnyhgnatFQj4aUQSxS47llaeCDxpWcTkAWm/kdJztg9J6rbEJyYyresCP0vWz\n\tdl7IMswKa/Ru5Z/i5+RcJ1Y8uNVhQe7VmS796k1Yw8uCvRVwQ3Mno/xID4wCV2y2RlgHwj\n\twjakyLSTACzXrH8o4j5eSvpY3787WrOG4=", "From": "Umang Jain <email@uajain.com>", "Date": "Tue, 16 Jun 2020 19:45:33 +0000 (UTC)", "Message-Id": "<20200616194523.23268-2-email@uajain.com>", "In-Reply-To": "<20200616194523.23268-1-email@uajain.com>", "References": "<20200616194523.23268-1-email@uajain.com>", "Mime-Version": "1.0", "X-SG-EID": "1Q40EQ7YGir8a9gjSIAdTjhngY657NMk9ckeo4dbHZDiOpywc/L3L9rFqlwE4KPc9vLW9W8H4bP+KZ0PPVtun2GUUTio/M2EKpbAqho5duHtKP8eDtaspFrCVhJ3PR4vH4mRMd6r9cvmCvIQ41w0V5s6xB89i6oWBMoIuY7NpmIkHD8QkD8HoD7yLL+opDFU9zWt6CvP0UsGm++XnnIalUSTSuKsRfmnw79WvGN0AY/sQXGanPBxpsGKAHATyFM45p9lFjfFgueV2J9k7IurlA==", "To": "laurent.pinchart@ideasonboard.com, kieran.bingham@ideasonboard.com,\n\tlibcamera-devel@lists.libcamera.org", "Content-Type": "text/plain; charset=utf-8", "Content-Transfer-Encoding": "8bit", "Subject": "[libcamera-devel] [PATCH v5 1/6] libcamera: CameraManager: Drop the\n\tvector of created PipelineHandlers", "X-BeenThere": "libcamera-devel@lists.libcamera.org", "X-Mailman-Version": "2.1.29", "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": "Tue, 16 Jun 2020 19:45:35 -0000" }, "content": "The pipes_ vector was initially used to store pipeline handlers\ninstances with the CameraManager when it cannot be referenced from\nanywhere else. It was used to retrieve cameras and deleting pipeline\nhandlers when stopping the camera manager.\n\nIn f3695e9b09ce (\"libcamera: camera_manager: Register cameras with the\ncamera manager\"), cameras started to get registered directly with camera\nmanager and in 5b02e03199b7 (\"libcamera: camera: Associate cameras with\ntheir pipeline handler\") pipeline handler started to get stored in a\nstd::shared_ptr<> with each camera starting to hold a strong reference\nto its associated pipeline-handler. At this point, both the camera\nmanager and the camera held a strong reference to the pipeline handler.\n\nSince the additional reference held by the camera manager gets released\nonly on cleanup(), this lurking reference held on pipeline handler, did\nnot allow it to get destroyed the even when cameras instances have been\ndestroyed. This situation of having a pipeline handler instance around\nwithout having a camera, may lead to problems (one of them explained\nbelow) especially when the camera manager is still running.\n\nIt was noticed that, there was a dangling driver directory issue (tested\nfor UVC camera - in /sys/bus/usb/drivers/uvcvideo) on 'unbind' → 'bind'\noperation while the CameraManager is running. The directories were still\nkept around even after 'unbind' because of the lurking reference of\npipeline handler holding onto them. That reference would clear if and\nonly if the CameraManager is stopped and then only directories were\ngetting removed in the above stated path.\n\nRather than writing a fix to release the pipeline handlers' reference\nfrom camera manager on camera disconnection, it is decided to eliminate\nthe pipes_ vector from CameraManager moving forwards. There is no\npoint in holding a reference to it from camera manager's point-of-view\nat this stage. It also helps us to fix the issue as explained above.\n\nNow that the pipeline handler instances are referenced via cameras\nonly, it can happen that the destruction of last camera instance may\nresult into destruction of pipeline handler itself. Such a possibility\nexists PipelineHandler::disconnect(), where the pipeline handler itself\ncan get destroyed while removing the camera. This is acceptable as long\nas we make sure that there is no access of pipeline handler's members\nlater on in the code-path. Address this situation and also add a\ndetailed comment about it.\n\nSigned-off-by: Umang Jain <email@uajain.com>\n---\n src/libcamera/camera_manager.cpp | 8 ++------\n src/libcamera/pipeline_handler.cpp | 18 +++++++++++++++---\n 2 files changed, 17 insertions(+), 9 deletions(-)", "diff": "diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\nindex 576856a..dbdc78e 100644\n--- a/src/libcamera/camera_manager.cpp\n+++ b/src/libcamera/camera_manager.cpp\n@@ -63,7 +63,6 @@ private:\n \tbool initialized_;\n \tint status_;\n \n-\tstd::vector<std::shared_ptr<PipelineHandler>> pipes_;\n \tstd::unique_ptr<DeviceEnumerator> enumerator_;\n \n \tIPAManager ipaManager_;\n@@ -144,7 +143,6 @@ int CameraManager::Private::init()\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(std::move(pipe));\n \t\t}\n \t}\n \n@@ -158,11 +156,9 @@ void CameraManager::Private::cleanup()\n \t/* TODO: unregister hot-plug callback here */\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 * Release all references to cameras to ensure they all get destroyed\n+\t * before the device enumerator deletes the media devices.\n \t */\n-\tpipes_.clear();\n \tcameras_.clear();\n \n \tenumerator_.reset(nullptr);\ndiff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\nindex a0f6b0f..c457be9 100644\n--- a/src/libcamera/pipeline_handler.cpp\n+++ b/src/libcamera/pipeline_handler.cpp\n@@ -559,7 +559,21 @@ void PipelineHandler::mediaDeviceDisconnected(MediaDevice *media)\n */\n void PipelineHandler::disconnect()\n {\n-\tfor (std::weak_ptr<Camera> ptr : cameras_) {\n+\t/*\n+\t * Each camera holds a reference to its associated pipeline handler\n+\t * instance. Hence, when the last camera is dropped, the pipeline\n+\t * handler will get destroyed by the last manager_->removeCamera(camera)\n+\t * call in the loop below.\n+\t *\n+\t * This is acceptable as long as we make sure that the code path does not\n+\t * access any member of the (already destroyed)pipeline handler instance\n+\t * afterwards. Therefore, we move the cameras_ vector to a local temporary\n+\t * container to avoid accessing freed memory later i.e. to explicitly run\n+\t * cameras_.clear().\n+\t */\n+\tstd::vector<std::weak_ptr<Camera>> cameras{ std::move(cameras_) };\n+\n+\tfor (std::weak_ptr<Camera> ptr : cameras) {\n \t\tstd::shared_ptr<Camera> camera = ptr.lock();\n \t\tif (!camera)\n \t\t\tcontinue;\n@@ -567,8 +581,6 @@ void PipelineHandler::disconnect()\n \t\tcamera->disconnect();\n \t\tmanager_->removeCamera(camera.get());\n \t}\n-\n-\tcameras_.clear();\n }\n \n /**\n", "prefixes": [ "libcamera-devel", "v5", "1/6" ] }