[{"id":4987,"web_url":"https://patchwork.libcamera.org/comment/4987/","msgid":"<20200603205155.GF5959@pendragon.ideasonboard.com>","date":"2020-06-03T20:51:55","subject":"Re: [libcamera-devel] [PATCH 1/5] IPAManager: make IPAManager\n\tlifetime explicitly managed","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Wed, Jun 03, 2020 at 11:16:05PM +0900, Paul Elder wrote:\n> If any ipa_context instances are destroyed before the IPAManager is\n> destroyed, then a segfault will occur when the IPAManager is\n> deconstructed, as it tries to destroy all of the IPAs that it manages.\n\nIsn't it the other way around, destroying ipa_context after the\nIPAManager, as the modules are unloaded by the IPAManager and the\ncontext function pointers thus become invalid ?\n\n> Fix this by making the lifetime of the IPAManager explicit, and make the\n> CameraManager construct and deconstruct (automatically, via a unique\n> pointer) the IPAManager.\n> \n> Also update the IPA interface test to do the construction and\n> deconstruction of the IPAManager, as it does not use the CameraManager.\n> \n> Signed-off-by: Paul Elder \n> ---\n> include/libcamera/camera_manager.h | 4 ++++\n> include/libcamera/internal/ipa_manager.h | 7 ++++---\n> src/libcamera/camera_manager.cpp | 4 +++-\n> src/libcamera/ipa_manager.cpp | 13 +++++++++++--\n> test/ipa/ipa_interface_test.cpp | 5 +++++\n> 5 files changed, 27 insertions(+), 6 deletions(-)\n> \n> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h\n> index 079f848a..8f9398fa 100644\n> --- a/include/libcamera/camera_manager.h\n> +++ b/include/libcamera/camera_manager.h\n> @@ -14,6 +14,8 @@\n> \n> #include \n> \n> +#include \"libcamera/internal/ipa_manager.h\"\n> +\n\nIncluding an internal header in a public header will cause issues once\nlibcamera is installed, as the internal headers are not installed. As\nyou include ipa_manager.h to get the IPAManager class declaration only,\nyou could just forward-declare it.\n\n> namespace libcamera {\n> \n> class Camera;\n> @@ -48,6 +50,8 @@ private:\n> \n> \tclass Private;\n> \tstd::unique_ptr p_;\n> +\n> +\tstd::unique_ptr ipaManager_;\n\nBut let's move this to the CameraManager::Private class, to avoid\nexposing the IPAManager in a public header, and to help with ABI\ncompatibility in the future (any non-static member data of a class is\npart of the ABI, the less we expose, the less we risk breaking).\n\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h\n> index 2412d757..65303d6d 100644\n> --- a/include/libcamera/internal/ipa_manager.h\n> +++ b/include/libcamera/internal/ipa_manager.h\n> @@ -22,6 +22,9 @@ namespace libcamera {\n> class IPAManager\n> {\n> public:\n> +\tIPAManager();\n> +\t~IPAManager();\n> +\n> \tstatic IPAManager *instance();\n> \n> \tstd::unique_ptr createIPA(PipelineHandler *pipe,\n> @@ -30,9 +33,7 @@ public:\n> \n> private:\n> \tstd::vector modules_;\n> -\n> -\tIPAManager();\n> -\t~IPAManager();\n\nIdeally the constructor should be private, with a friend class\nCameraManager::Private, but that will break the IPA interface test :-S\nIt would be nice if we could avoid making it public, but otherwise I can\nlive with that.\n\n> +\tstatic IPAManager *self_;\n> \n> \tvoid parseDir(const char *libDir, unsigned int maxDepth,\n> \t\t std::vector &files);\n> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> index 849377ad..d19348df 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/device_enumerator.h\"\n> #include \"libcamera/internal/event_dispatcher_poll.h\"\n> +#include \"libcamera/internal/ipa_manager.h\"\n> #include \"libcamera/internal/log.h\"\n> #include \"libcamera/internal/pipeline_handler.h\"\n> #include \"libcamera/internal/thread.h\"\n> @@ -246,7 +247,8 @@ void CameraManager::Private::removeCamera(Camera *camera)\n> CameraManager *CameraManager::self_ = nullptr;\n> \n> CameraManager::CameraManager()\n> -\t: p_(new CameraManager::Private(this))\n> +\t: p_(new CameraManager::Private(this)), ipaManager_(new IPAManager())\n\nAs the IPAManager constructor takes no argument, I think you can embed\nan instance in CameraManager (or rather in CameraManager::Private)\ninstead of using a pointer.\n\n> +\n\nExtra blank line.\n\n> {\n> \tif (self_)\n> \t\tLOG(Camera, Fatal)\n> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp\n> index 505cf610..74112cde 100644\n> --- a/src/libcamera/ipa_manager.cpp\n> +++ b/src/libcamera/ipa_manager.cpp\n> @@ -93,8 +93,14 @@ LOG_DEFINE_CATEGORY(IPAManager)\n> * plain C API, or to transmit the data to the isolated process through IPC.\n> */\n> \n> +IPAManager *IPAManager::self_ = nullptr;\n> +\n\nNow that the constructor is public, it should be documented, especially\nto state that nobody but the CameraManager should create an instance of\nIPAManager.\n\n/**\n * \\brief Construct an IPAManager instance\n *\n * The IPAManager class is meant to only be instantiated once, by the\n * CameraManager. Pipeline handlers shall use the instance() function to access\n * the IPAManager instance.\n */\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> \tunsigned int ipaCount = 0;\n> \n> \t/* User-specified paths take precedence. */\n> @@ -134,12 +140,16 @@ 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> /**\n> @@ -153,8 +163,7 @@ IPAManager::~IPAManager()\n> */\n> IPAManager *IPAManager::instance()\n> {\n> -\tstatic IPAManager ipaManager;\n> -\treturn &ipaManager;\n> +\treturn self_;\n> }\n\nAs the only usage of instance() is to call createIPA() in pipeline\nhandlers, would it make sense (in another patch) to drop the instance()\nfunction, and turn the createIPA() function into a static function\n(using self_ to access the IPA manager instance) ?\n\n> \n> /**\n> diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp\n> index 2f02af49..153493ba 100644\n> --- a/test/ipa/ipa_interface_test.cpp\n> +++ b/test/ipa/ipa_interface_test.cpp\n> @@ -39,11 +39,15 @@ public:\n> \t~IPAInterfaceTest()\n> \t{\n> \t\tdelete notifier_;\n> +\t\tipa_.reset();\n> +\t\tipaManager_.reset();\n> \t}\n> \n> protected:\n> \tint init() override\n> \t{\n> +\t\tipaManager_ = make_unique();\n> +\n> \t\t/* Create a pipeline handler for vimc. */\n> \t\tstd::vector &factories =\n> \t\t\tPipelineHandlerFactory::factories();\n> @@ -161,6 +165,7 @@ private:\n> \n> \tstd::shared_ptr pipe_;\n> \tstd::unique_ptr ipa_;\n> +\tstd::unique_ptr ipaManager_;\n> \tenum IPAOperationCode trace_;\n> \tEventNotifier *notifier_;\n> \tint fd_;","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 6F3B06110A\n\tfor ;\n\tWed, 3 Jun 2020 22:52:12 +0200 (CEST)","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 D0F0029B;\n\tWed, 3 Jun 2020 22:52:11 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"wncr1+Fq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1591217532;\n\tbh=yrNXNahYhWQoh68ZD51o1ymZqUKLRtaElB5vIV3itIg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wncr1+FqOuGMpMG+FSFjUbHIk4jnBa2gANMXzzDh0+RLHivCxrywAQFH5NlTs+1w0\n\tbImeTqgRy9XrYW9BCio28eR4KNh5ntsHfngbX0JPT8TN919gtCR5VSWv2fh3+XnRUy\n\tEMwHNJuuoD9+BA1+OoHi3YoaODHlZzK3JRvVgBOo=","Date":"Wed, 3 Jun 2020 23:51:55 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200603205155.GF5959@pendragon.ideasonboard.com>","References":"<20200603141609.18584-1-paul.elder@ideasonboard.com>\n\t<20200603141609.18584-2-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200603141609.18584-2-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/5] IPAManager: make IPAManager\n\tlifetime explicitly managed","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 03 Jun 2020 20:52:12 -0000"}}]