Patch Detail
Show a patch.
GET /api/1.1/patches/20752/?format=api
{ "id": 20752, "url": "https://patchwork.libcamera.org/api/1.1/patches/20752/?format=api", "web_url": "https://patchwork.libcamera.org/patch/20752/", "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": "<20240803212832.22766-1-laurent.pinchart@ideasonboard.com>", "date": "2024-08-03T21:28:32", "name": "libcamera: ipa_manager: Remove singleton requirement", "commit_ref": null, "pull_url": null, "state": "accepted", "archived": false, "hash": "bff2c07aa420a5e090c8afd80fd38a204d95ab76", "submitter": { "id": 2, "url": "https://patchwork.libcamera.org/api/1.1/people/2/?format=api", "name": "Laurent Pinchart", "email": "laurent.pinchart@ideasonboard.com" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/20752/mbox/", "series": [ { "id": 4477, "url": "https://patchwork.libcamera.org/api/1.1/series/4477/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=4477", "date": "2024-08-03T21:28:32", "name": "libcamera: ipa_manager: Remove singleton requirement", "version": 1, "mbox": "https://patchwork.libcamera.org/series/4477/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/20752/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/20752/checks/", "tags": {}, "headers": { "Return-Path": "<libcamera-devel-bounces@lists.libcamera.org>", "X-Original-To": "parsemail@patchwork.libcamera.org", "Delivered-To": "parsemail@patchwork.libcamera.org", "Received": [ "from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 2CD76BDC71\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSat, 3 Aug 2024 21:28:58 +0000 (UTC)", "from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 854EF6337D;\n\tSat, 3 Aug 2024 23:28:57 +0200 (CEST)", "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 A64746336E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 3 Aug 2024 23:28:55 +0200 (CEST)", "from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3EBC4512\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 3 Aug 2024 23:28:05 +0200 (CEST)" ], "Authentication-Results": "lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"keSfTXVL\"; dkim-atps=neutral", "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1722720485;\n\tbh=4gqut8XLD4X2Cj0To3tM+qeALQ04DXOIHlUSQB2/9k8=;\n\th=From:To:Subject:Date:From;\n\tb=keSfTXVL64TmqpbrWcpYEDPy6higlcGARP7R61bg1Sad8W61K1sb8jW80oR6P/8Oo\n\to4KBqByBMjObF7tlVrQrjD5SpG5RsFcpvXepAP9cDUrWuteGGK3vcGQW2IC9BoQs0P\n\tcgEZjC8nNc81fZ8KVM9PimDUkuuXhhNE+ZddCoqA=", "From": "Laurent Pinchart <laurent.pinchart@ideasonboard.com>", "To": "libcamera-devel@lists.libcamera.org", "Subject": "[PATCH] libcamera: ipa_manager: Remove singleton requirement", "Date": "Sun, 4 Aug 2024 00:28:32 +0300", "Message-ID": "<20240803212832.22766-1-laurent.pinchart@ideasonboard.com>", "X-Mailer": "git-send-email 2.44.2", "MIME-Version": "1.0", "Content-Transfer-Encoding": "8bit", "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>", "Errors-To": "libcamera-devel-bounces@lists.libcamera.org", "Sender": "\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>" }, "content": "The IPAManager class implements a singleton pattern due to the need of\naccessing the instance in a static member function. The function now\ntakes a pointer to a PipelineHandler, which we can use to access the\nCameraManager, and from there, the IPAManager.\n\nAdd accessors to the internal API to expose the CameraManager from the\nPipelineHandler, and the IPAManager from the CameraManager. This\nrequires allocating the IPAManager dynamically to avoid a loop in\nincludes. Use those accessors to replace the IPAManager singleton.\n\nUpdate the IPA interface unit test to instantiate a CameraManager\ninstead of an IPAManager and ProcessManager, to reflect the new way that\nthe IPAManager is accessed.\n\nSigned-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n---\n include/libcamera/internal/camera_manager.h | 7 +++++--\n include/libcamera/internal/ipa_manager.h | 9 +++++----\n include/libcamera/internal/pipeline_handler.h | 2 ++\n src/libcamera/camera_manager.cpp | 9 +++++++++\n src/libcamera/ipa_manager.cpp | 10 ----------\n src/libcamera/pipeline_handler.cpp | 7 +++++++\n test/ipa/ipa_interface_test.cpp | 12 +++++-------\n 7 files changed, 33 insertions(+), 23 deletions(-)\n\n\nbase-commit: 19bbca3c0b376ba0183f5db53472c8c46cd402b5", "diff": "diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h\nindex af9ed60a0353..e098cb69aa43 100644\n--- a/include/libcamera/internal/camera_manager.h\n+++ b/include/libcamera/internal/camera_manager.h\n@@ -19,13 +19,14 @@\n #include <libcamera/base/thread.h>\n #include <libcamera/base/thread_annotations.h>\n \n-#include \"libcamera/internal/ipa_manager.h\"\n #include \"libcamera/internal/process.h\"\n \n namespace libcamera {\n \n class Camera;\n class DeviceEnumerator;\n+class IPAManager;\n+class PipelineHandlerFactoryBase;\n \n class CameraManager::Private : public Extensible::Private, public Thread\n {\n@@ -38,6 +39,8 @@ public:\n \tvoid addCamera(std::shared_ptr<Camera> camera) LIBCAMERA_TSA_EXCLUDES(mutex_);\n \tvoid removeCamera(std::shared_ptr<Camera> camera) LIBCAMERA_TSA_EXCLUDES(mutex_);\n \n+\tIPAManager *ipaManager() const { return ipaManager_.get(); }\n+\n protected:\n \tvoid run() override;\n \n@@ -62,7 +65,7 @@ private:\n \n \tstd::unique_ptr<DeviceEnumerator> enumerator_;\n \n-\tIPAManager ipaManager_;\n+\tstd::unique_ptr<IPAManager> ipaManager_;\n \tProcessManager processManager_;\n };\n \ndiff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h\nindex c6f74e11c434..16dede0c5a7e 100644\n--- a/include/libcamera/internal/ipa_manager.h\n+++ b/include/libcamera/internal/ipa_manager.h\n@@ -15,6 +15,7 @@\n #include <libcamera/ipa/ipa_interface.h>\n #include <libcamera/ipa/ipa_module_info.h>\n \n+#include \"libcamera/internal/camera_manager.h\"\n #include \"libcamera/internal/ipa_module.h\"\n #include \"libcamera/internal/pipeline_handler.h\"\n #include \"libcamera/internal/pub_key.h\"\n@@ -34,11 +35,13 @@ public:\n \t\t\t\t\t uint32_t minVersion,\n \t\t\t\t\t uint32_t maxVersion)\n \t{\n-\t\tIPAModule *m = self_->module(pipe, minVersion, maxVersion);\n+\t\tCameraManager *cm = pipe->cameraManager();\n+\t\tIPAManager *self = cm->_d()->ipaManager();\n+\t\tIPAModule *m = self->module(pipe, minVersion, maxVersion);\n \t\tif (!m)\n \t\t\treturn nullptr;\n \n-\t\tstd::unique_ptr<T> proxy = std::make_unique<T>(m, !self_->isSignatureValid(m));\n+\t\tstd::unique_ptr<T> proxy = std::make_unique<T>(m, !self->isSignatureValid(m));\n \t\tif (!proxy->isValid()) {\n \t\t\tLOG(IPAManager, Error) << \"Failed to load proxy\";\n \t\t\treturn nullptr;\n@@ -55,8 +58,6 @@ public:\n #endif\n \n private:\n-\tstatic IPAManager *self_;\n-\n \tvoid parseDir(const char *libDir, unsigned int maxDepth,\n \t\t std::vector<std::string> &files);\n \tunsigned int addDir(const char *libDir, unsigned int maxDepth = 0);\ndiff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\nindex 746a34f8e7bd..cad5812f640c 100644\n--- a/include/libcamera/internal/pipeline_handler.h\n+++ b/include/libcamera/internal/pipeline_handler.h\n@@ -70,6 +70,8 @@ public:\n \n \tconst char *name() const { return name_; }\n \n+\tCameraManager *cameraManager() const { return manager_; }\n+\n protected:\n \tvoid registerCamera(std::shared_ptr<Camera> camera);\n \tvoid hotplugMediaDevice(MediaDevice *media);\ndiff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\nindex 95a9e3264526..31760a8680cc 100644\n--- a/src/libcamera/camera_manager.cpp\n+++ b/src/libcamera/camera_manager.cpp\n@@ -15,6 +15,7 @@\n \n #include \"libcamera/internal/camera.h\"\n #include \"libcamera/internal/device_enumerator.h\"\n+#include \"libcamera/internal/ipa_manager.h\"\n #include \"libcamera/internal/pipeline_handler.h\"\n \n /**\n@@ -37,6 +38,7 @@ LOG_DEFINE_CATEGORY(Camera)\n CameraManager::Private::Private()\n \t: initialized_(false)\n {\n+\tipaManager_ = std::make_unique<IPAManager>();\n }\n \n int CameraManager::Private::start()\n@@ -249,6 +251,13 @@ void CameraManager::Private::removeCamera(std::shared_ptr<Camera> camera)\n \to->cameraRemoved.emit(camera);\n }\n \n+/**\n+ * \\fn CameraManager::Private::ipaManager() const\n+ * \\brief Retrieve the IPAManager\n+ * \\context This function is \\threadsafe.\n+ * \\return The IPAManager for this CameraManager\n+ */\n+\n /**\n * \\class CameraManager\n * \\brief Provide access and manage all cameras in the system\ndiff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp\nindex f4e0b6339f08..cfc24d389c4f 100644\n--- a/src/libcamera/ipa_manager.cpp\n+++ b/src/libcamera/ipa_manager.cpp\n@@ -95,8 +95,6 @@ LOG_DEFINE_CATEGORY(IPAManager)\n * IPC.\n */\n \n-IPAManager *IPAManager::self_ = nullptr;\n-\n /**\n * \\brief Construct an IPAManager instance\n *\n@@ -105,10 +103,6 @@ IPAManager *IPAManager::self_ = nullptr;\n */\n IPAManager::IPAManager()\n {\n-\tif (self_)\n-\t\tLOG(IPAManager, Fatal)\n-\t\t\t<< \"Multiple IPAManager objects are not allowed\";\n-\n #if HAVE_IPA_PUBKEY\n \tif (!pubKey_.isValid())\n \t\tLOG(IPAManager, Warning) << \"Public key not valid\";\n@@ -153,16 +147,12 @@ IPAManager::IPAManager()\n \tif (!ipaCount)\n \t\tLOG(IPAManager, Warning)\n \t\t\t<< \"No IPA found in '\" IPA_MODULE_DIR \"'\";\n-\n-\tself_ = this;\n }\n \n IPAManager::~IPAManager()\n {\n \tfor (IPAModule *module : modules_)\n \t\tdelete module;\n-\n-\tself_ = nullptr;\n }\n \n /**\ndiff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\nindex 5ea2ca780b63..5a6de685b292 100644\n--- a/src/libcamera/pipeline_handler.cpp\n+++ b/src/libcamera/pipeline_handler.cpp\n@@ -719,6 +719,13 @@ void PipelineHandler::disconnect()\n * \\return The pipeline handler name\n */\n \n+/**\n+ * \\fn PipelineHandler::cameraManager() const\n+ * \\brief Retrieve the CameraManager that this pipeline handler belongs to\n+ * \\context This function is \\threadsafe.\n+ * \\return The CameraManager for this pipeline handler\n+ */\n+\n /**\n * \\class PipelineHandlerFactoryBase\n * \\brief Base class for pipeline handler factories\ndiff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp\nindex e840f6ab17c4..b81783664977 100644\n--- a/test/ipa/ipa_interface_test.cpp\n+++ b/test/ipa/ipa_interface_test.cpp\n@@ -20,11 +20,11 @@\n #include <libcamera/base/thread.h>\n #include <libcamera/base/timer.h>\n \n+#include \"libcamera/internal/camera_manager.h\"\n #include \"libcamera/internal/device_enumerator.h\"\n #include \"libcamera/internal/ipa_manager.h\"\n #include \"libcamera/internal/ipa_module.h\"\n #include \"libcamera/internal/pipeline_handler.h\"\n-#include \"libcamera/internal/process.h\"\n \n #include \"test.h\"\n \n@@ -44,20 +44,20 @@ public:\n \t{\n \t\tdelete notifier_;\n \t\tipa_.reset();\n-\t\tipaManager_.reset();\n+\t\tcameraManager_.reset();\n \t}\n \n protected:\n \tint init() override\n \t{\n-\t\tipaManager_ = make_unique<IPAManager>();\n+\t\tcameraManager_ = make_unique<CameraManager>();\n \n \t\t/* Create a pipeline handler for vimc. */\n \t\tconst std::vector<PipelineHandlerFactoryBase *> &factories =\n \t\t\tPipelineHandlerFactoryBase::factories();\n \t\tfor (const PipelineHandlerFactoryBase *factory : factories) {\n \t\t\tif (factory->name() == \"vimc\") {\n-\t\t\t\tpipe_ = factory->create(nullptr);\n+\t\t\t\tpipe_ = factory->create(cameraManager_.get());\n \t\t\t\tbreak;\n \t\t\t}\n \t\t}\n@@ -171,11 +171,9 @@ private:\n \t\t}\n \t}\n \n-\tProcessManager processManager_;\n-\n \tstd::shared_ptr<PipelineHandler> pipe_;\n \tstd::unique_ptr<ipa::vimc::IPAProxyVimc> ipa_;\n-\tstd::unique_ptr<IPAManager> ipaManager_;\n+\tstd::unique_ptr<CameraManager> cameraManager_;\n \tenum ipa::vimc::IPAOperationCode trace_;\n \tEventNotifier *notifier_;\n \tint fd_;\n", "prefixes": [] }