[{"id":12538,"web_url":"https://patchwork.libcamera.org/comment/12538/","msgid":"<20200916020020.GJ14954@pendragon.ideasonboard.com>","date":"2020-09-16T02:00:20","subject":"Re: [libcamera-devel] [PATCH 23/23] libcamera: PipelineHandler:\n\tMove printing pipeline names to CameraManager","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 Tue, Sep 15, 2020 at 11:20:38PM +0900, Paul Elder wrote:\n> Since pipeline registration is done with declaring static factory\n> objects, there is a risk that pipeline factories will be constructed\n> before libcamera facilities are ready. For example, logging in the\n> constructor of a pipeline handler factory may cause a segfault if\n> threading isn't ready yet. Avoid this issue by moving printing the\n> registration of the pipeline handler to the camera manager.\n\nThis is an important issue, that should be kept in mind when adding any\nglobal variable (especially global variables that end up running\nconstructors). Could I convince you to document the problem in\nDocumentation/coding-style.rst ? :-) Maybe in a new section after\n\"Object Ownership\". It should summarize the problem as we have\npreviously discussed on privately (the order of global initializations\n*and* destructions can't be reasonably controlled, thus causing\npotential issues when different global variables depend on each other),\nand the solution (avoid global variables when possible, a good example\nis patch 01/23 in this series, and when required, such as when\nimplementing factories with auto-registration, avoid dependencies by\nrunning the minimum amount of code in the constructor and destructor,\nand moving code that contains dependencies to a later point of time, as\ndone in this patch).\n\n> Signed-off-by: Paul Elder \n> ---\n> src/libcamera/camera_manager.cpp | 2 ++\n> src/libcamera/pipeline_handler.cpp | 3 ---\n> 2 files changed, 2 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> index b8d3ccc8..e5af271d 100644\n> --- a/src/libcamera/camera_manager.cpp\n> +++ b/src/libcamera/camera_manager.cpp\n> @@ -142,6 +142,8 @@ void CameraManager::Private::createPipelineHandlers()\n> \t\tPipelineHandlerFactory::factories();\n> \n> \tfor (PipelineHandlerFactory *factory : factories) {\n> +\t\tLOG(Camera, Debug)\n> +\t\t\t<< \"Found registered pipeline handler \\\"\" << factory->name() << \"\\\"\";\n\nWith the line wrapped\n\n\t\t\t<< \"Found registered pipeline handler \\\"\"\n\t\t\t<< factory->name() << \"\\\"\";\n\nand a blank line added here,\n\nReviewed-by: Laurent Pinchart \n\nYou may also want to replace \\\" with '.\n\n> \t\t/*\n> \t\t * Try each pipeline handler until it exhaust\n> \t\t * all pipelines it can provide.\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 918aea1e..894200ee 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -689,9 +689,6 @@ void PipelineHandlerFactory::registerType(PipelineHandlerFactory *factory)\n> \tstd::vector &factories = PipelineHandlerFactory::factories();\n> \n> \tfactories.push_back(factory);\n> -\n> -\tLOG(Pipeline, Debug)\n> -\t\t<< \"Registered pipeline handler \\\"\" << factory->name() << \"\\\"\";\n> }\n> \n> /**","headers":{"Return-Path":"","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 D7E7CBF01C\n\tfor ;\n\tWed, 16 Sep 2020 02:00:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 67B3262E13;\n\tWed, 16 Sep 2020 04:00: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 B15EA62C8C\n\tfor ;\n\tWed, 16 Sep 2020 04:00:55 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B576E57F;\n\tWed, 16 Sep 2020 04:00:49 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"e8ilifjI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1600221649;\n\tbh=VzS+jY/dogc8Q23wI6TcymFx7rqZfXVS0oSRywswC8E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=e8ilifjIrosAnyo82rnhL5XCs0VIX7duZflUd73vP2c2h2ZaA15MQFIsgRhk8a62P\n\tiJKu5FiwRUpMBJtvHUzbqsH5TQwJiKOJDxKButpPmU+WEzvBsqxuEYbXUKAXR7d6F6\n\t3KqB+tLZu+9x+xxxKWOT/FG4oe2bAZ4zoD+kp0FE=","Date":"Wed, 16 Sep 2020 05:00:20 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"<20200916020020.GJ14954@pendragon.ideasonboard.com>","References":"<20200915142038.28757-1-paul.elder@ideasonboard.com>\n\t<20200915142038.28757-24-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200915142038.28757-24-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 23/23] libcamera: PipelineHandler:\n\tMove printing pipeline names to CameraManager","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]