[{"id":25296,"web_url":"https://patchwork.libcamera.org/comment/25296/","msgid":"","date":"2022-10-05T11:07:36","subject":"Re: [libcamera-devel] [PATCH 4/8] ipa: camera_sensor_helper:\n\tImplement factories through class templates","submitter":{"id":107,"url":"https://patchwork.libcamera.org/api/people/107/","name":"Xavier Roumegue","email":"xavier.roumegue@oss.nxp.com"},"content":"Hi Laurent,\n\nThanks for the patch.\n\nOn 10/3/22 23:21, Laurent Pinchart via libcamera-devel wrote:\n> The REGISTER_CAMERA_SENSOR_HELPER() macro defines a class type that\n> inherits from the CameraSensorHelperFactory class, and implements a\n> constructor and createInstance() function. Replace the code generation\n> through macro with the C++ equivalent, a class template, as done by the\n> Algorithm factory.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> src/ipa/ipu3/ipu3.cpp | 2 +-\n> src/ipa/libipa/camera_sensor_helper.cpp | 79 +++++++++++++++----------\n> src/ipa/libipa/camera_sensor_helper.h | 43 ++++++++------\n> src/ipa/rkisp1/rkisp1.cpp | 2 +-\n> 4 files changed, 75 insertions(+), 51 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index b93a09d40c39..852deb710956 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -326,7 +326,7 @@ int IPAIPU3::init(const IPASettings &settings,\n> \t\t const ControlInfoMap &sensorControls,\n> \t\t ControlInfoMap *ipaControls)\n> {\n> -\tcamHelper_ = CameraSensorHelperFactory::create(settings.sensorModel);\n> +\tcamHelper_ = CameraSensorHelperFactoryBase::create(settings.sensorModel);\n> \tif (camHelper_ == nullptr) {\n> \t\tLOG(IPAIPU3, Error)\n> \t\t\t<< \"Failed to create camera sensor helper for \"\n> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> index 3a7d701d8616..e655be255f2b 100644\n> --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> @@ -43,7 +43,8 @@ namespace ipa {\n> * \\brief Construct a CameraSensorHelper instance\n> *\n> * CameraSensorHelper derived class instances shall never be constructed\n> - * manually but always through the CameraSensorHelperFactory::create() function.\n> + * manually but always through the CameraSensorHelperFactoryBase::create()\n> + * function.\n> */\n> \n> /**\n> @@ -217,27 +218,25 @@ double CameraSensorHelper::gain(uint32_t gainCode) const\n> */\n> \n> /**\n> - * \\class CameraSensorHelperFactory\n> - * \\brief Registration of CameraSensorHelperFactory classes and creation of instances\n> + * \\class CameraSensorHelperFactoryBase\n> + * \\brief Base class for camera sensor helper factories\n> *\n> - * To facilitate discovery and instantiation of CameraSensorHelper classes, the\n> - * CameraSensorHelperFactory class maintains a registry of camera sensor helper\n> - * sub-classes. Each CameraSensorHelper subclass shall register itself using the\n> - * REGISTER_CAMERA_SENSOR_HELPER() macro, which will create a corresponding\n> - * instance of a CameraSensorHelperFactory subclass and register it with the\n> - * static list of factories.\n> + * The CameraSensorHelperFactoryBase class is the base of all specializations of\n> + * the CameraSensorHelperFactory class template. It implements the factory\n> + * registration, maintains a registry of factories, and provides access to the\n> + * registered factories.\n> */\n> \n> /**\n> - * \\brief Construct a camera sensor helper factory\n> + * \\brief Construct a camera sensor helper factory base\n> * \\param[in] name Name of the camera sensor helper class\n> *\n> - * Creating an instance of the factory registers it with the global list of\n> + * Creating an instance of the factory base registers it with the global list of\n> * factories, accessible through the factories() function.\n> *\n> * The factory \\a name is used for debug purpose and shall be unique.\n> */\n> -CameraSensorHelperFactory::CameraSensorHelperFactory(const std::string name)\n> +CameraSensorHelperFactoryBase::CameraSensorHelperFactoryBase(const std::string name)\n> \t: name_(name)\n> {\n> \tregisterType(this);\n> @@ -252,12 +251,12 @@ CameraSensorHelperFactory::CameraSensorHelperFactory(const std::string name)\n> * corresponding to the named factory or a null pointer if no such factory\n> * exists\n> */\n> -std::unique_ptr CameraSensorHelperFactory::create(const std::string &name)\n> +std::unique_ptr CameraSensorHelperFactoryBase::create(const std::string &name)\n> {\n> -\tconst std::vector &factories =\n> -\t\tCameraSensorHelperFactory::factories();\n> +\tconst std::vector &factories =\n> +\t\tCameraSensorHelperFactoryBase::factories();\n> \n> -\tfor (const CameraSensorHelperFactory *factory : factories) {\n> +\tfor (const CameraSensorHelperFactoryBase *factory : factories) {\n> \t\tif (name != factory->name_)\n> \t\t\tcontinue;\n> \n> @@ -274,10 +273,10 @@ std::unique_ptr CameraSensorHelperFactory::create(const std:\n> * The caller is responsible to guarantee the uniqueness of the camera sensor\n> * helper name.\n> */\n> -void CameraSensorHelperFactory::registerType(CameraSensorHelperFactory *factory)\n> +void CameraSensorHelperFactoryBase::registerType(CameraSensorHelperFactoryBase *factory)\n> {\n> -\tstd::vector &factories =\n> -\t\tCameraSensorHelperFactory::factories();\n> +\tstd::vector &factories =\n> +\t\tCameraSensorHelperFactoryBase::factories();\n> \n> \tfactories.push_back(factory);\n> }\n> @@ -286,35 +285,55 @@ void CameraSensorHelperFactory::registerType(CameraSensorHelperFactory *factory)\n> * \\brief Retrieve the list of all camera sensor helper factories\n> * \\return The list of camera sensor helper factories\n> */\n> -std::vector &CameraSensorHelperFactory::factories()\n> +std::vector &CameraSensorHelperFactoryBase::factories()\n> {\n> \t/*\n> \t * The static factories map is defined inside the function to ensure\n> \t * it gets initialized on first use, without any dependency on link\n> \t * order.\n> \t */\n> -\tstatic std::vector factories;\n> +\tstatic std::vector factories;\n> \treturn factories;\n> }\n> \n> +/**\n> + * \\var CameraSensorHelperFactoryBase::name_\n> + * \\brief The name of the factory\n> + */\n> +\n> +/**\n> + * \\class CameraSensorHelperFactory\n> + * \\brief Registration of CameraSensorHelperFactory classes and creation of instances\n> + * \\tparam _Helper The camera sensor helepr class type for this factory\ns/helepr/helper/\n> + *\n> + * To facilitate discovery and instantiation of CameraSensorHelper classes, the\n> + * CameraSensorHelperFactory class implements auto-registration of camera sensor\n> + * helpers. Each CameraSensorHelper subclass shall register itself using the\n> + * REGISTER_CAMERA_SENSOR_HELPER() macro, which will create a corresponding\n> + * instance of a CameraSensorHelperFactory subclass and register it with the\n> + * static list of factories.\n> + */\n> +\n> +/**\n> + * \\fn CameraSensorHelperFactory::CameraSensorHelperFactory(const char *name)\n> + * \\brief Construct a camera sensor helper factory\n> + * \\param[in] name Name of the camera sensor helper class\n> + *\n> + * Creating an instance of the factory registers it with the global list of\n> + * factories, accessible through the factories() function.\n> + *\n> + * The factory \\a name is used for debug purpose and shall be unique.\n> + */\n> +\n> /**\n> * \\fn CameraSensorHelperFactory::createInstance() const\n> * \\brief Create an instance of the CameraSensorHelper corresponding to the\n> * factory\n> *\n> - * This virtual function is implemented by the REGISTER_CAMERA_SENSOR_HELPER()\n> - * macro. It creates a camera sensor helper instance associated with the camera\n> - * sensor model.\n> - *\n> * \\return A unique pointer to a newly constructed instance of the\n> * CameraSensorHelper subclass corresponding to the factory\n> */\n> \n> -/**\n> - * \\var CameraSensorHelperFactory::name_\n> - * \\brief The name of the factory\n> - */\n> -\n> /**\n> * \\def REGISTER_CAMERA_SENSOR_HELPER\n> * \\brief Register a camera sensor helper with the camera sensor helper factory\n> diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h\n> index 21ee43cc9f9f..3ea1806cb1fd 100644\n> --- a/src/ipa/libipa/camera_sensor_helper.h\n> +++ b/src/ipa/libipa/camera_sensor_helper.h\n> @@ -58,39 +58,44 @@ private:\n> \tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelper)\n> };\n> \n> -class CameraSensorHelperFactory\n> +class CameraSensorHelperFactoryBase\n> {\n> public:\n> -\tCameraSensorHelperFactory(const std::string name);\n> -\tvirtual ~CameraSensorHelperFactory() = default;\n> +\tCameraSensorHelperFactoryBase(const std::string name);\n> +\tvirtual ~CameraSensorHelperFactoryBase() = default;\n> \n> \tstatic std::unique_ptr create(const std::string &name);\n> \n> -\tstatic std::vector &factories();\n> +\tstatic std::vector &factories();\n> \n> private:\n> -\tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelperFactory)\n> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelperFactoryBase)\n> \n> -\tstatic void registerType(CameraSensorHelperFactory *factory);\n> +\tstatic void registerType(CameraSensorHelperFactoryBase *factory);\n> \n> \tvirtual std::unique_ptr createInstance() const = 0;\n> \n> \tstd::string name_;\n> };\n> \n> -#define REGISTER_CAMERA_SENSOR_HELPER(name, helper)\t\t\\\n> -class helper##Factory final : public CameraSensorHelperFactory\t\\\n> -{\t\t\t\t\t\t\t\t\\\n> -public: \t\t\t\t\t\t\t\\\n> -\thelper##Factory() : CameraSensorHelperFactory(name) {}\t\\\n> -\t\t\t\t\t\t\t\t\\\n> -private:\t\t\t\t\t\t\t\\\n> -\tstd::unique_ptr createInstance() const \\\n> -\t{\t\t\t\t\t\t\t\\\n> -\t\treturn std::make_unique();\t\t\\\n> -\t}\t\t\t\t\t\t\t\\\n> -};\t\t\t\t\t\t\t\t\\\n> -static helper##Factory global_##helper##Factory;\n> +template\n> +class CameraSensorHelperFactory final : public CameraSensorHelperFactoryBase\n> +{\n> +public:\n> +\tCameraSensorHelperFactory(const char *name)\n> +\t\t: CameraSensorHelperFactoryBase(name)\n> +\t{\n> +\t}\n> +\n> +private:\n> +\tstd::unique_ptr createInstance() const\n> +\t{\n> +\t\treturn std::make_unique<_Helper>();\n> +\t}\n> +};\n> +\n> +#define REGISTER_CAMERA_SENSOR_HELPER(name, helper) \\\n> +static CameraSensorHelperFactory global_##helper##Factory(name);\n> \n> } /* namespace ipa */\n> \n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 1335e3d14df2..9451cb03a285 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -143,7 +143,7 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> \t/* Cache the value to set it in configure. */\n> \thwRevision_ = static_cast(hwRevision);\n> \n> -\tcamHelper_ = CameraSensorHelperFactory::create(settings.sensorModel);\n> +\tcamHelper_ = CameraSensorHelperFactoryBase::create(settings.sensorModel);\n> \tif (!camHelper_) {\n> \t\tLOG(IPARkISP1, Error)\n> \t\t\t<< \"Failed to create camera sensor helper for \"\n\n\nWith the typo fixed,\n\nReviewed-by: Xavier Roumegue ","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 EDB90BD16B\n\tfor ;\n\tWed, 5 Oct 2022 11:07:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 41E04622E8;\n\tWed, 5 Oct 2022 13:07:44 +0200 (CEST)","from EUR01-DB5-obe.outbound.protection.outlook.com\n\t(mail-eopbgr150055.outbound.protection.outlook.com [40.107.15.55])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 15408621BC\n\tfor ;\n\tWed, 5 Oct 2022 13:07:42 +0200 (CEST)","from PAXPR04MB8703.eurprd04.prod.outlook.com\n\t(2603:10a6:102:21e::22)\n\tby AM9PR04MB8825.eurprd04.prod.outlook.com (2603:10a6:20b:408::7)\n\twith Microsoft SMTP Server (version=TLS1_2,\n\tcipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5676.31;\n\tWed, 5 Oct 2022 11:07:38 +0000","from PAXPR04MB8703.eurprd04.prod.outlook.com\n\t([fe80::4f72:a35a:8c60:63f1]) by\n\tPAXPR04MB8703.eurprd04.prod.outlook.com\n\t([fe80::4f72:a35a:8c60:63f1%6]) with mapi id 15.20.5676.028;\n\tWed, 5 Oct 2022 11:07:38 +0000"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1664968064;\n\tbh=lxZ19iAQQucSSMNJb32wPZI8mf1DT3Km768vlrjBVwA=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=FZDY3F3qlcZY+fWrv2BmlnnV4UnO5OmUmGACCkDdsBtbjPjIxIAIhMCFeMR55nMQa\n\tdpMMvfqktFfMkT0nu8G7pmS4Fzc5aJY+Py0TiyhBJKdj3UuhlcFglKRlsk3DZkZPWk\n\tRERr5wl5zOYUjwuT9RDuIhmU15haIl9k6QHvZpcT8/7hGhuqg5TEgi4yxXlijSPvah\n\tT0H3DbVVNNwuDNR7KvK+mg7hlCWtXjWzZjEBEEM50ADf5tzSuu5F72bA3p9l7ZJTBx\n\tbvXAAzgwqgPbrcJOMoPQ6fCjSb3qoguyLiHriR9SzYWa4feWHOp/b9qU2/x+d4xMpF\n\tL7W+K2EQRiZeA==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=NXP1.onmicrosoft.com;\n\ts=selector2-NXP1-onmicrosoft-com;\n\th=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck;\n\tbh=gwok6zz03ZRgHnOU/Q7g0DwWYHHSawFPG7HeiDaxW/Y=;\n\tb=elpB1fQzELRM5THtwIfuHx1NaIa1q7Fq0WFBR1uwg3xR58vSOPdj5LNJYgrRxGgltQ4L407bF6+zt8gHUaac4svtaSzlDNPtSfib48giSdGAzpBkdsRNeD1DJhWAoutTeuT6+awlh+nDp86bxrtiwE/Ix7lZC591mAx+KWVQ9ns="],"Authentication-Results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=NXP1.onmicrosoft.com\n\theader.i=@NXP1.onmicrosoft.com\n\theader.b=\"elpB1fQz\"; dkim-atps=neutral","dkim=none (message not signed)\n\theader.d=none;dmarc=none action=none header.from=oss.nxp.com;"],"ARC-Seal":"i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none;\n\tb=mbSINc2ahmg04UL3PgEXqXcscUjbvSLEmR8gnbXm0ozyFw0BzbOBgfp9niNkzn+BA5AyHSYsPU6hQngJckScVw2ObLhZULU2ndzw9j/qH+T04pX06O4ce2KKouiqOq1t08/cMJv2qC49vWY1QZwkfEGU9CxvwcvaYqO9Amp1SrpPFLwOCgZ4ZsJuhkIYS6WDDidb4NpY7uG4uWHcCPSX2iqUF9y4eWsWjrX2F4N8C2Gb5q7gvDzrc0WjBbMH8g2fh9ISDY07mZqT2NQVURICJ+m8684WOgNhGsTFvu0thziqSxOZLsYxLADl6F5r8qp3OP7yzLkk1HcB3kme7MsEdQ==","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com;\n\ts=arcselector9901;\n\th=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1;\n\tbh=gwok6zz03ZRgHnOU/Q7g0DwWYHHSawFPG7HeiDaxW/Y=;\n\tb=HfgutvEilG49HS7cVNkHvVZVHDf+pOCnUSrqlLzvHp0/Y4XHV5zkqraM3eBacVmgKJjhU8zFvxGzRVUjSkp6hKUMNdjaPFCB+xPEunOhdW6Qa22HkRUggld8Jaeg2iDJC+8PMZ81pqDHsyQQAS98MW/awpKDBnDA+tAgtUoVVILYXIyDPLB6RxkavrEnfPCfXOjzyejjZ3p0lEqGjrHH0oLzPguSR3DHAXO+Wuzfy9lGk3dfr6Ff1pNdXq2mIqfVVgZPwIvZhxA4X/8R6jt0iEO2Jf/Au6K5pMgXipQUYLSVWEwD44aJPCPuXagRhvWmDLGtgrzwTHNDTr2q+c15yQ==","ARC-Authentication-Results":"i=1; mx.microsoft.com 1; spf=pass\n\tsmtp.mailfrom=oss.nxp.com;\n\tdmarc=pass action=none header.from=oss.nxp.com; \n\tdkim=pass header.d=oss.nxp.com; arc=none","Message-ID":"","Date":"Wed, 5 Oct 2022 13:07:36 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.3.1","Content-Language":"en-US, fr","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20221003212128.32429-1-laurent.pinchart@ideasonboard.com>\n\t<20221003212128.32429-5-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20221003212128.32429-5-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","X-ClientProxiedBy":"PAZP264CA0230.FRAP264.PROD.OUTLOOK.COM\n\t(2603:10a6:102:238::20) To PAXPR04MB8703.eurprd04.prod.outlook.com\n\t(2603:10a6:102:21e::22)","MIME-Version":"1.0","X-MS-Exchange-MessageSentRepresentingType":"1","X-MS-PublicTrafficType":"Email","X-MS-TrafficTypeDiagnostic":"PAXPR04MB8703:EE_|AM9PR04MB8825:EE_","X-MS-Office365-Filtering-Correlation-Id":"e5a65c16-2333-4454-cf58-08daa6c1d02f","X-MS-Exchange-SharedMailbox-RoutingAgent-Processed":"True","X-MS-Exchange-SenderADCheck":"1","X-MS-Exchange-AntiSpam-Relay":"0","X-Microsoft-Antispam":"BCL:0;","X-Microsoft-Antispam-Message-Info":"hyg+2TohR/4N6lDiu/PQb8MR5khT8u3JoCbkwZXnGSPS0Aicr/A+rtShMNJa7K6FwBTCuUwzTiohQ27uZ6ps5kbB+GzeyrmMw0bj+DJmDt7ZSgCcp9uuugHYRDx7WO4DVidN2a3WFq45LEk6hHo3MCVOjeqptHKkojFa8cnkxFTfKaWar/1EW8M4bDqlui2ljO06aQ5qHXwSvYQJeIJHjdSORBYt6I8ksCiZggZdrr399s2wv8LyHiGdVxlqVkXIiQHItzF30mfSYYM0ctQW+Zu4kLyX/wEzmV38nRJvpsKrzDkbU5NDVCYf5i4AeI7Uollw+EZ/YONs7aCJhZxTwOsjFnRqzUoyn1nw1hyoLs8UoffoLJOz+30ifN2rTfPJb0XYDgTzCKOE9QgozOEHIjMNE05fZNTqTfo1MAOe+rNrUn1k559vRRK/5x/1B9Aw9mm7OlzApLFvq3CuXXY7EFblY2JTIHy40iSYvqpJrxoYbJmBrcI8CViiFWVL/aKufbQ6mcSui8TvTRBQXpsjnmwwMlLgbgS4IKXyLCily+AlyVbelDOkjw4YMtUxVpztWjZXuMXouyfS2FiKinRM2V1E6mNn8yNT4i/8sJXwnZk3CIkDJqihGm6L3e3kOxPGvaU5OrfIiI+CAaW5N+lhd7HUlmhtc4lkFkUU73yHWPFyQlgsXj5jPXE+5cg99sfNqGhIC/MAi+B8v+O7ibIbckyKkeNYZFGmdBvvK27OzQW1pXLh//Sf2Swc8qoRCJKZd4tk5Nuc0JtKaDh6ldtGSADOxIGAz77Ltcpsc9YZPJQ=","X-Forefront-Antispam-Report":"CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:;\n\tIPV:NLI; SFV:NSPM; H:PAXPR04MB8703.eurprd04.prod.outlook.com; PTR:;\n\tCAT:NONE; \n\tSFS:(13230022)(4636009)(39860400002)(376002)(136003)(396003)(366004)(346002)(451199015)(83380400001)(31686004)(2906002)(66556008)(86362001)(66946007)(5660300002)(66476007)(53546011)(31696002)(8936002)(6506007)(41300700001)(52116002)(38100700002)(186003)(2616005)(478600001)(6512007)(30864003)(8676002)(316002)(6486002)(45980500001)(43740500002);\n\tDIR:OUT; SFP:1101; ","X-MS-Exchange-AntiSpam-MessageData-ChunkCount":"1","X-MS-Exchange-AntiSpam-MessageData-0":"=?utf-8?q?y17u4oIt375mFINnmsXbY0qhG?=\n\t=?utf-8?q?TJ9g7HpPfVyDbJjuiZqWXft+sYzgQHWDwiC0MQyK6fCvT9RfgSU9wfO2?=\n\t=?utf-8?q?XnUMsDdySTXZff6ldNm0cpPlMW4WtszeSAUgTRanhEgnaad7xycwnPnj?=\n\t=?utf-8?q?6kqTmtFwl2dUaMK0YgkU3ozUq3gSSuxxVGrc1Jdc6HttspWJB6WY8Zps?=\n\t=?utf-8?q?F+PS3g87481AcbyqexXFeKpxYjFG3ZqZQ4t2dATPzHRPW3tJcV8C0iIX?=\n\t=?utf-8?q?ZGu2V4+GEa/bJjK7BRdsN/l7CkAOEZ5GlxITxiKhPQ/5afWiDIzVvyNf?=\n\t=?utf-8?q?7lihybEdWE09/SERyKCwBUrFutozp8LcjhMpCPdE0Fl6mBJANhMvvYR8?=\n\t=?utf-8?q?gXVg6Q0IboCXdJVCyfS2PoX3EfyuxlDfit4X0w9EZvz0LDs17GO97Oga?=\n\t=?utf-8?q?jzjjWbSUQDL75NgEQDcWc5cBO/uChwRfWBxvyz7NTfiRwEpCrPOyhXLH?=\n\t=?utf-8?q?l7KY3lg3NFlM3S9P5NND+Xno6iejtV+o6/svlUTrPdlp53MJsDYpkkv/?=\n\t=?utf-8?q?qmPP5iXK8eY0mhFHNDy9E7bvIs5d8obzzbXAsY1NzkWi6Z9LlOeSa9Jx?=\n\t=?utf-8?q?yElmCh1FvI7k5ueU0Jfk3s8TpiS0jp2NvHHUh6D6xWPo2dQqzXbv5vNz?=\n\t=?utf-8?q?3oSuM+VwXEKdwPw8Uxaj0LJL0FkzaCrvFY+3LDGQlDC2lLMy/bpGTutS?=\n\t=?utf-8?q?3Gwi005XwlVw3kvumfBZff9L31rJDz6xBPbwhnfOa6KkMOzPOcQfkcY0?=\n\t=?utf-8?q?jxIS1kopCVJ/kr43F2/Zkc6+9C+T7wAIfSl3qcqiMBPGtvE0PFi85GMd?=\n\t=?utf-8?q?gruS+V6A1HiV+vTj0cDL90KckoF4sCy+u10q1/Xyw2RXo7tdBP4RVi4i?=\n\t=?utf-8?q?d5zh/AEpyE3UswKpmBExeT1HbeC1IJeWpLGPPnZA04/z43l5NEUU9P5w?=\n\t=?utf-8?q?nqjTj59uC7VffbDt1Xqzq2KJLWo75LyHwfBfvRCuiMJOvuDP+1be6bXP?=\n\t=?utf-8?q?lRxr9waYRcFN3YT+NOxVR4XLnGLdj0KkX0HSpS4gJX+Vse5O7poFGun8?=\n\t=?utf-8?q?hHzA1kB6bFq2JY7ee0HBvhLOw+fOPgAEfBfZlpXWL2VO8ld3BtXYQhT9?=\n\t=?utf-8?q?mwWa84MehZhxYB6h5Nn2ESTuECjCElL/aY4WkP/HOT/0MpNX6qxIr3Wv?=\n\t=?utf-8?q?q2P5jj5f6k7YMR3KjfFmynfLVfeMFnM+5mhLsH+gqZ4/46LH/a6qRXOl?=\n\t=?utf-8?q?a13JEmLaklH0bp3lrcPOEi9Vzy391+AEYps5yynnYGHth2TxlHOXKLDJ?=\n\t=?utf-8?q?yYpqMSeAoQLgEyFuuRvHkxtyo+InCapkV2+EZxNFj7CYyx/3uv7ibsSa?=\n\t=?utf-8?q?NM5dvS49UKsLkxridjCcOLMTvw/3Hw/j8iK8DgnG4+g8yWXU2VlpNnXJ?=\n\t=?utf-8?q?QygiI2kfXs/+cdME8Yq6D4M1ohHXdRJbsCOb6KM/dZTVZaubfk0Uvh9/?=\n\t=?utf-8?q?TQtp/uTX9Njm4AA2fLse+KcrqnRIkinyaYPY5x8hVRyVRU01cixG1Cg9?=\n\t=?utf-8?q?ciUIGA22aIiAVzoHSmbwlLIE2hEdqOEfPDIpx3gvI5Vpr/2UWWnfbgO7?=\n\t=?utf-8?q?QehpwtyC+h/51w+V2IP1N673uukwmkksrpsPFk2QfbFywqF3vRe2ZVQ3?=\n\t=?utf-8?q?kEDnIc8meEKr7C8u6FagIeNBW0DpVcuCWceSPLtXF2R47RhjFmRCOW4g?=\n\t=?utf-8?q?eUBvAE9J5aVhqyQga3t+17YcjbvJVb4amnZJg=3D=3D?=","X-OriginatorOrg":"oss.nxp.com","X-MS-Exchange-CrossTenant-Network-Message-Id":"e5a65c16-2333-4454-cf58-08daa6c1d02f","X-MS-Exchange-CrossTenant-AuthSource":"PAXPR04MB8703.eurprd04.prod.outlook.com","X-MS-Exchange-CrossTenant-AuthAs":"Internal","X-MS-Exchange-CrossTenant-OriginalArrivalTime":"05 Oct 2022 11:07:38.0083\n\t(UTC)","X-MS-Exchange-CrossTenant-FromEntityHeader":"Hosted","X-MS-Exchange-CrossTenant-Id":"686ea1d3-bc2b-4c6f-a92c-d99c5c301635","X-MS-Exchange-CrossTenant-MailboxType":"HOSTED","X-MS-Exchange-CrossTenant-UserPrincipalName":"eWohYomgT9gcXvTvLAN91epwi6MCOgZMI3tK2JaH/Qs9o3j0R0uCKP0h+paSCXm+JVXK2WfEVsalwxoZIhoSvg==","X-MS-Exchange-Transport-CrossTenantHeadersStamped":"AM9PR04MB8825","Subject":"Re: [libcamera-devel] [PATCH 4/8] ipa: camera_sensor_helper:\n\tImplement factories through class templates","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","From":"\"Xavier Roumegue \\(OSS\\) via libcamera-devel\"\n\t","Reply-To":"\"Xavier Roumegue \\(OSS\\)\" ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25334,"web_url":"https://patchwork.libcamera.org/comment/25334/","msgid":"<20221007140841.cm2uaaodkl7rds5x@uno.localdomain>","date":"2022-10-07T14:08:41","subject":"Re: [libcamera-devel] [PATCH 4/8] ipa: camera_sensor_helper:\n\tImplement factories through class templates","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"On Tue, Oct 04, 2022 at 12:21:24AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> The REGISTER_CAMERA_SENSOR_HELPER() macro defines a class type that\n> inherits from the CameraSensorHelperFactory class, and implements a\n> constructor and createInstance() function. Replace the code generation\n> through macro with the C++ equivalent, a class template, as done by the\n> Algorithm factory.\n>\n\nI am been looking for a way to remove \"Base\" from the factory name,\nbut it seems we're running out of names...\n\n\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/ipa/ipu3/ipu3.cpp | 2 +-\n> src/ipa/libipa/camera_sensor_helper.cpp | 79 +++++++++++++++----------\n> src/ipa/libipa/camera_sensor_helper.h | 43 ++++++++------\n> src/ipa/rkisp1/rkisp1.cpp | 2 +-\n> 4 files changed, 75 insertions(+), 51 deletions(-)\n>\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index b93a09d40c39..852deb710956 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -326,7 +326,7 @@ int IPAIPU3::init(const IPASettings &settings,\n> \t\t const ControlInfoMap &sensorControls,\n> \t\t ControlInfoMap *ipaControls)\n> {\n> -\tcamHelper_ = CameraSensorHelperFactory::create(settings.sensorModel);\n> +\tcamHelper_ = CameraSensorHelperFactoryBase::create(settings.sensorModel);\n> \tif (camHelper_ == nullptr) {\n> \t\tLOG(IPAIPU3, Error)\n> \t\t\t<< \"Failed to create camera sensor helper for \"\n> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> index 3a7d701d8616..e655be255f2b 100644\n> --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> @@ -43,7 +43,8 @@ namespace ipa {\n> * \\brief Construct a CameraSensorHelper instance\n> *\n> * CameraSensorHelper derived class instances shall never be constructed\n> - * manually but always through the CameraSensorHelperFactory::create() function.\n> + * manually but always through the CameraSensorHelperFactoryBase::create()\n> + * function.\n> */\n>\n> /**\n> @@ -217,27 +218,25 @@ double CameraSensorHelper::gain(uint32_t gainCode) const\n> */\n>\n> /**\n> - * \\class CameraSensorHelperFactory\n> - * \\brief Registration of CameraSensorHelperFactory classes and creation of instances\n> + * \\class CameraSensorHelperFactoryBase\n> + * \\brief Base class for camera sensor helper factories\n> *\n> - * To facilitate discovery and instantiation of CameraSensorHelper classes, the\n> - * CameraSensorHelperFactory class maintains a registry of camera sensor helper\n> - * sub-classes. Each CameraSensorHelper subclass shall register itself using the\n> - * REGISTER_CAMERA_SENSOR_HELPER() macro, which will create a corresponding\n> - * instance of a CameraSensorHelperFactory subclass and register it with the\n> - * static list of factories.\n> + * The CameraSensorHelperFactoryBase class is the base of all specializations of\n> + * the CameraSensorHelperFactory class template. It implements the factory\n> + * registration, maintains a registry of factories, and provides access to the\n> + * registered factories.\n> */\n>\n> /**\n> - * \\brief Construct a camera sensor helper factory\n> + * \\brief Construct a camera sensor helper factory base\n> * \\param[in] name Name of the camera sensor helper class\n> *\n> - * Creating an instance of the factory registers it with the global list of\n> + * Creating an instance of the factory base registers it with the global list of\n> * factories, accessible through the factories() function.\n\nAs this is only called by the subclasses to register their types,\nreading \"Creating an instance of the factory base -> registers it\"\nwhile it's actually the construction of the derived class that uses\nthe base class constructor for registration.\n\nA detail though\n\n> *\n> * The factory \\a name is used for debug purpose and shall be unique.\n> */\n> -CameraSensorHelperFactory::CameraSensorHelperFactory(const std::string name)\n> +CameraSensorHelperFactoryBase::CameraSensorHelperFactoryBase(const std::string name)\n> \t: name_(name)\n> {\n> \tregisterType(this);\n> @@ -252,12 +251,12 @@ CameraSensorHelperFactory::CameraSensorHelperFactory(const std::string name)\n> * corresponding to the named factory or a null pointer if no such factory\n> * exists\n> */\n> -std::unique_ptr CameraSensorHelperFactory::create(const std::string &name)\n> +std::unique_ptr CameraSensorHelperFactoryBase::create(const std::string &name)\n> {\n> -\tconst std::vector &factories =\n> -\t\tCameraSensorHelperFactory::factories();\n> +\tconst std::vector &factories =\n> +\t\tCameraSensorHelperFactoryBase::factories();\n>\n> -\tfor (const CameraSensorHelperFactory *factory : factories) {\n> +\tfor (const CameraSensorHelperFactoryBase *factory : factories) {\n> \t\tif (name != factory->name_)\n> \t\t\tcontinue;\n>\n> @@ -274,10 +273,10 @@ std::unique_ptr CameraSensorHelperFactory::create(const std:\n> * The caller is responsible to guarantee the uniqueness of the camera sensor\n> * helper name.\n> */\n> -void CameraSensorHelperFactory::registerType(CameraSensorHelperFactory *factory)\n> +void CameraSensorHelperFactoryBase::registerType(CameraSensorHelperFactoryBase *factory)\n> {\n> -\tstd::vector &factories =\n> -\t\tCameraSensorHelperFactory::factories();\n> +\tstd::vector &factories =\n> +\t\tCameraSensorHelperFactoryBase::factories();\n>\n> \tfactories.push_back(factory);\n> }\n> @@ -286,35 +285,55 @@ void CameraSensorHelperFactory::registerType(CameraSensorHelperFactory *factory)\n> * \\brief Retrieve the list of all camera sensor helper factories\n> * \\return The list of camera sensor helper factories\n> */\n> -std::vector &CameraSensorHelperFactory::factories()\n> +std::vector &CameraSensorHelperFactoryBase::factories()\n> {\n> \t/*\n> \t * The static factories map is defined inside the function to ensure\n> \t * it gets initialized on first use, without any dependency on link\n> \t * order.\n> \t */\n> -\tstatic std::vector factories;\n> +\tstatic std::vector factories;\n> \treturn factories;\n> }\n>\n> +/**\n> + * \\var CameraSensorHelperFactoryBase::name_\n> + * \\brief The name of the factory\n> + */\n\nShould we document a private field ?\n\n> +\n> +/**\n> + * \\class CameraSensorHelperFactory\n> + * \\brief Registration of CameraSensorHelperFactory classes and creation of instances\n> + * \\tparam _Helper The camera sensor helepr class type for this factory\n\ns/helepr/helper\n\n> + *\n> + * To facilitate discovery and instantiation of CameraSensorHelper classes, the\n> + * CameraSensorHelperFactory class implements auto-registration of camera sensor\n> + * helpers. Each CameraSensorHelper subclass shall register itself using the\n> + * REGISTER_CAMERA_SENSOR_HELPER() macro, which will create a corresponding\n> + * instance of a CameraSensorHelperFactory subclass and register it with the\n> + * static list of factories.\n> + */\n> +\n> +/**\n> + * \\fn CameraSensorHelperFactory::CameraSensorHelperFactory(const char *name)\n> + * \\brief Construct a camera sensor helper factory\n> + * \\param[in] name Name of the camera sensor helper class\n> + *\n> + * Creating an instance of the factory registers it with the global list of\n> + * factories, accessible through the factories() function.\n\nfactories() is not part of this class..\n\n> + *\n> + * The factory \\a name is used for debug purpose and shall be unique.\n\nDon't we actually match on the name ?\n\n> + */\n> +\n> /**\n> * \\fn CameraSensorHelperFactory::createInstance() const\n> * \\brief Create an instance of the CameraSensorHelper corresponding to the\n> * factory\n> *\n> - * This virtual function is implemented by the REGISTER_CAMERA_SENSOR_HELPER()\n> - * macro. It creates a camera sensor helper instance associated with the camera\n> - * sensor model.\n> - *\n> * \\return A unique pointer to a newly constructed instance of the\n> * CameraSensorHelper subclass corresponding to the factory\n> */\n>\n> -/**\n> - * \\var CameraSensorHelperFactory::name_\n> - * \\brief The name of the factory\n> - */\n> -\n> /**\n> * \\def REGISTER_CAMERA_SENSOR_HELPER\n> * \\brief Register a camera sensor helper with the camera sensor helper factory\n> diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h\n> index 21ee43cc9f9f..3ea1806cb1fd 100644\n> --- a/src/ipa/libipa/camera_sensor_helper.h\n> +++ b/src/ipa/libipa/camera_sensor_helper.h\n> @@ -58,39 +58,44 @@ private:\n> \tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelper)\n> };\n>\n> -class CameraSensorHelperFactory\n> +class CameraSensorHelperFactoryBase\n> {\n> public:\n> -\tCameraSensorHelperFactory(const std::string name);\n> -\tvirtual ~CameraSensorHelperFactory() = default;\n> +\tCameraSensorHelperFactoryBase(const std::string name);\n\nCan this be made protected now ?\n\n> +\tvirtual ~CameraSensorHelperFactoryBase() = default;\n>\n> \tstatic std::unique_ptr create(const std::string &name);\n>\n> -\tstatic std::vector &factories();\n> +\tstatic std::vector &factories();\n>\n> private:\n> -\tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelperFactory)\n> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelperFactoryBase)\n>\n> -\tstatic void registerType(CameraSensorHelperFactory *factory);\n> +\tstatic void registerType(CameraSensorHelperFactoryBase *factory);\n>\n> \tvirtual std::unique_ptr createInstance() const = 0;\n>\n> \tstd::string name_;\n> };\n>\n> -#define REGISTER_CAMERA_SENSOR_HELPER(name, helper)\t\t\\\n> -class helper##Factory final : public CameraSensorHelperFactory\t\\\n> -{\t\t\t\t\t\t\t\t\\\n> -public: \t\t\t\t\t\t\t\\\n> -\thelper##Factory() : CameraSensorHelperFactory(name) {}\t\\\n> -\t\t\t\t\t\t\t\t\\\n> -private:\t\t\t\t\t\t\t\\\n> -\tstd::unique_ptr createInstance() const \\\n> -\t{\t\t\t\t\t\t\t\\\n> -\t\treturn std::make_unique();\t\t\\\n> -\t}\t\t\t\t\t\t\t\\\n> -};\t\t\t\t\t\t\t\t\\\n> -static helper##Factory global_##helper##Factory;\n> +template\n> +class CameraSensorHelperFactory final : public CameraSensorHelperFactoryBase\n> +{\n> +public:\n> +\tCameraSensorHelperFactory(const char *name)\n> +\t\t: CameraSensorHelperFactoryBase(name)\n> +\t{\n> +\t}\n> +\n> +private:\n> +\tstd::unique_ptr createInstance() const\n> +\t{\n> +\t\treturn std::make_unique<_Helper>();\n> +\t}\n> +};\n> +\n> +#define REGISTER_CAMERA_SENSOR_HELPER(name, helper) \\\n> +static CameraSensorHelperFactory global_##helper##Factory(name);\n\nAll minors\nReviewed-by: Jacopo Mondi \n\n>\n> } /* namespace ipa */\n>\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 1335e3d14df2..9451cb03a285 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -143,7 +143,7 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> \t/* Cache the value to set it in configure. */\n> \thwRevision_ = static_cast(hwRevision);\n>\n> -\tcamHelper_ = CameraSensorHelperFactory::create(settings.sensorModel);\n> +\tcamHelper_ = CameraSensorHelperFactoryBase::create(settings.sensorModel);\n> \tif (!camHelper_) {\n> \t\tLOG(IPARkISP1, Error)\n> \t\t\t<< \"Failed to create camera sensor helper for \"\n> --\n> Regards,\n>\n> Laurent Pinchart\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 4C33BC0DA4\n\tfor ;\n\tFri, 7 Oct 2022 14:08:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 80B4F62D18;\n\tFri, 7 Oct 2022 16:08:44 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::223])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AED1660A88\n\tfor ;\n\tFri, 7 Oct 2022 16:08:43 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 18D846000D;\n\tFri, 7 Oct 2022 14:08:42 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1665151724;\n\tbh=wTt31FB9TOOLm5uDsQggGbOWACHrjqRNvffjwjTrXmE=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=NV3drNDr8ao3xBxQardBe+Pz0rCpHvGGx8cvO73KUAbSXo4tKcIhyLpSHCqEpSXRI\n\tosGIuILbH64pPvmF6XimWihRqUYjszc+4gL7WSpE/GsABgxXccckVU+bBl93Bzoaf2\n\tvsBd4GWEYpRc/JNY7/TNTkwNOUbBiiV+10/zbqPwkutFgNmCwRqpsbLnjVuU6fNVJq\n\t/jMILSDNG+tFoxKRcwJ2fX8f2VZT/iRics/xkapKuaHBFIXhVjrb1fcwISlkOXHGus\n\tttQp0NcxYnir6/4rc9KxQUupI+jUVdhln4OzY9f7Y8x4GM2lXo7En57ztkpAf96g2p\n\tk0oy59vef4MuA==","Date":"Fri, 7 Oct 2022 16:08:41 +0200","To":"Laurent Pinchart ","Message-ID":"<20221007140841.cm2uaaodkl7rds5x@uno.localdomain>","References":"<20221003212128.32429-1-laurent.pinchart@ideasonboard.com>\n\t<20221003212128.32429-5-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221003212128.32429-5-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 4/8] ipa: camera_sensor_helper:\n\tImplement factories through class templates","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25340,"web_url":"https://patchwork.libcamera.org/comment/25340/","msgid":"","date":"2022-10-07T14:27:10","subject":"Re: [libcamera-devel] [PATCH 4/8] ipa: camera_sensor_helper:\n\tImplement factories through class templates","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, Oct 07, 2022 at 04:08:41PM +0200, Jacopo Mondi wrote:\n> On Tue, Oct 04, 2022 at 12:21:24AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > The REGISTER_CAMERA_SENSOR_HELPER() macro defines a class type that\n> > inherits from the CameraSensorHelperFactory class, and implements a\n> > constructor and createInstance() function. Replace the code generation\n> > through macro with the C++ equivalent, a class template, as done by the\n> > Algorithm factory.\n> \n> I am been looking for a way to remove \"Base\" from the factory name,\n> but it seems we're running out of names...\n\n:-)\n\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/ipa/ipu3/ipu3.cpp | 2 +-\n> > src/ipa/libipa/camera_sensor_helper.cpp | 79 +++++++++++++++----------\n> > src/ipa/libipa/camera_sensor_helper.h | 43 ++++++++------\n> > src/ipa/rkisp1/rkisp1.cpp | 2 +-\n> > 4 files changed, 75 insertions(+), 51 deletions(-)\n> >\n> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > index b93a09d40c39..852deb710956 100644\n> > --- a/src/ipa/ipu3/ipu3.cpp\n> > +++ b/src/ipa/ipu3/ipu3.cpp\n> > @@ -326,7 +326,7 @@ int IPAIPU3::init(const IPASettings &settings,\n> > \t\t const ControlInfoMap &sensorControls,\n> > \t\t ControlInfoMap *ipaControls)\n> > {\n> > -\tcamHelper_ = CameraSensorHelperFactory::create(settings.sensorModel);\n> > +\tcamHelper_ = CameraSensorHelperFactoryBase::create(settings.sensorModel);\n> > \tif (camHelper_ == nullptr) {\n> > \t\tLOG(IPAIPU3, Error)\n> > \t\t\t<< \"Failed to create camera sensor helper for \"\n> > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> > index 3a7d701d8616..e655be255f2b 100644\n> > --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> > +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> > @@ -43,7 +43,8 @@ namespace ipa {\n> > * \\brief Construct a CameraSensorHelper instance\n> > *\n> > * CameraSensorHelper derived class instances shall never be constructed\n> > - * manually but always through the CameraSensorHelperFactory::create() function.\n> > + * manually but always through the CameraSensorHelperFactoryBase::create()\n> > + * function.\n> > */\n> >\n> > /**\n> > @@ -217,27 +218,25 @@ double CameraSensorHelper::gain(uint32_t gainCode) const\n> > */\n> >\n> > /**\n> > - * \\class CameraSensorHelperFactory\n> > - * \\brief Registration of CameraSensorHelperFactory classes and creation of instances\n> > + * \\class CameraSensorHelperFactoryBase\n> > + * \\brief Base class for camera sensor helper factories\n> > *\n> > - * To facilitate discovery and instantiation of CameraSensorHelper classes, the\n> > - * CameraSensorHelperFactory class maintains a registry of camera sensor helper\n> > - * sub-classes. Each CameraSensorHelper subclass shall register itself using the\n> > - * REGISTER_CAMERA_SENSOR_HELPER() macro, which will create a corresponding\n> > - * instance of a CameraSensorHelperFactory subclass and register it with the\n> > - * static list of factories.\n> > + * The CameraSensorHelperFactoryBase class is the base of all specializations of\n> > + * the CameraSensorHelperFactory class template. It implements the factory\n> > + * registration, maintains a registry of factories, and provides access to the\n> > + * registered factories.\n> > */\n> >\n> > /**\n> > - * \\brief Construct a camera sensor helper factory\n> > + * \\brief Construct a camera sensor helper factory base\n> > * \\param[in] name Name of the camera sensor helper class\n> > *\n> > - * Creating an instance of the factory registers it with the global list of\n> > + * Creating an instance of the factory base registers it with the global list of\n> > * factories, accessible through the factories() function.\n> \n> As this is only called by the subclasses to register their types,\n> reading \"Creating an instance of the factory base -> registers it\"\n> while it's actually the construction of the derived class that uses\n> the base class constructor for registration.\n\nWhat I meant is that the constructor of the base class registers the\ninstance. It's of course called by the constructor of the derived class.\n\n> A detail though\n> \n> > *\n> > * The factory \\a name is used for debug purpose and shall be unique.\n> > */\n> > -CameraSensorHelperFactory::CameraSensorHelperFactory(const std::string name)\n> > +CameraSensorHelperFactoryBase::CameraSensorHelperFactoryBase(const std::string name)\n> > \t: name_(name)\n> > {\n> > \tregisterType(this);\n> > @@ -252,12 +251,12 @@ CameraSensorHelperFactory::CameraSensorHelperFactory(const std::string name)\n> > * corresponding to the named factory or a null pointer if no such factory\n> > * exists\n> > */\n> > -std::unique_ptr CameraSensorHelperFactory::create(const std::string &name)\n> > +std::unique_ptr CameraSensorHelperFactoryBase::create(const std::string &name)\n> > {\n> > -\tconst std::vector &factories =\n> > -\t\tCameraSensorHelperFactory::factories();\n> > +\tconst std::vector &factories =\n> > +\t\tCameraSensorHelperFactoryBase::factories();\n> >\n> > -\tfor (const CameraSensorHelperFactory *factory : factories) {\n> > +\tfor (const CameraSensorHelperFactoryBase *factory : factories) {\n> > \t\tif (name != factory->name_)\n> > \t\t\tcontinue;\n> >\n> > @@ -274,10 +273,10 @@ std::unique_ptr CameraSensorHelperFactory::create(const std:\n> > * The caller is responsible to guarantee the uniqueness of the camera sensor\n> > * helper name.\n> > */\n> > -void CameraSensorHelperFactory::registerType(CameraSensorHelperFactory *factory)\n> > +void CameraSensorHelperFactoryBase::registerType(CameraSensorHelperFactoryBase *factory)\n> > {\n> > -\tstd::vector &factories =\n> > -\t\tCameraSensorHelperFactory::factories();\n> > +\tstd::vector &factories =\n> > +\t\tCameraSensorHelperFactoryBase::factories();\n> >\n> > \tfactories.push_back(factory);\n> > }\n> > @@ -286,35 +285,55 @@ void CameraSensorHelperFactory::registerType(CameraSensorHelperFactory *factory)\n> > * \\brief Retrieve the list of all camera sensor helper factories\n> > * \\return The list of camera sensor helper factories\n> > */\n> > -std::vector &CameraSensorHelperFactory::factories()\n> > +std::vector &CameraSensorHelperFactoryBase::factories()\n> > {\n> > \t/*\n> > \t * The static factories map is defined inside the function to ensure\n> > \t * it gets initialized on first use, without any dependency on link\n> > \t * order.\n> > \t */\n> > -\tstatic std::vector factories;\n> > +\tstatic std::vector factories;\n> > \treturn factories;\n> > }\n> >\n> > +/**\n> > + * \\var CameraSensorHelperFactoryBase::name_\n> > + * \\brief The name of the factory\n> > + */\n> \n> Should we document a private field ?\n\nThis patch only moves the preexisting documentation, but you're right,\nI'll drop this.\n\n> > +\n> > +/**\n> > + * \\class CameraSensorHelperFactory\n> > + * \\brief Registration of CameraSensorHelperFactory classes and creation of instances\n> > + * \\tparam _Helper The camera sensor helepr class type for this factory\n> \n> s/helepr/helper\n> \n> > + *\n> > + * To facilitate discovery and instantiation of CameraSensorHelper classes, the\n> > + * CameraSensorHelperFactory class implements auto-registration of camera sensor\n> > + * helpers. Each CameraSensorHelper subclass shall register itself using the\n> > + * REGISTER_CAMERA_SENSOR_HELPER() macro, which will create a corresponding\n> > + * instance of a CameraSensorHelperFactory subclass and register it with the\n> > + * static list of factories.\n> > + */\n> > +\n> > +/**\n> > + * \\fn CameraSensorHelperFactory::CameraSensorHelperFactory(const char *name)\n> > + * \\brief Construct a camera sensor helper factory\n> > + * \\param[in] name Name of the camera sensor helper class\n> > + *\n> > + * Creating an instance of the factory registers it with the global list of\n> > + * factories, accessible through the factories() function.\n> \n> factories() is not part of this class..\n\nIndeed it's in the base class. I'll update that.\n\n> > + *\n> > + * The factory \\a name is used for debug purpose and shall be unique.\n> \n> Don't we actually match on the name ?\n\nWe do. I'll fix this.\n\n> > + */\n> > +\n> > /**\n> > * \\fn CameraSensorHelperFactory::createInstance() const\n> > * \\brief Create an instance of the CameraSensorHelper corresponding to the\n> > * factory\n> > *\n> > - * This virtual function is implemented by the REGISTER_CAMERA_SENSOR_HELPER()\n> > - * macro. It creates a camera sensor helper instance associated with the camera\n> > - * sensor model.\n> > - *\n> > * \\return A unique pointer to a newly constructed instance of the\n> > * CameraSensorHelper subclass corresponding to the factory\n> > */\n> >\n> > -/**\n> > - * \\var CameraSensorHelperFactory::name_\n> > - * \\brief The name of the factory\n> > - */\n> > -\n> > /**\n> > * \\def REGISTER_CAMERA_SENSOR_HELPER\n> > * \\brief Register a camera sensor helper with the camera sensor helper factory\n> > diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h\n> > index 21ee43cc9f9f..3ea1806cb1fd 100644\n> > --- a/src/ipa/libipa/camera_sensor_helper.h\n> > +++ b/src/ipa/libipa/camera_sensor_helper.h\n> > @@ -58,39 +58,44 @@ private:\n> > \tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelper)\n> > };\n> >\n> > -class CameraSensorHelperFactory\n> > +class CameraSensorHelperFactoryBase\n> > {\n> > public:\n> > -\tCameraSensorHelperFactory(const std::string name);\n> > -\tvirtual ~CameraSensorHelperFactory() = default;\n> > +\tCameraSensorHelperFactoryBase(const std::string name);\n> \n> Can this be made protected now ?\n\nIt could, and would then make it impossible to instantiate the class\ndirectly, but that's impossible already, as there's a pure virtual\nfunction.\n\n> > +\tvirtual ~CameraSensorHelperFactoryBase() = default;\n> >\n> > \tstatic std::unique_ptr create(const std::string &name);\n> >\n> > -\tstatic std::vector &factories();\n> > +\tstatic std::vector &factories();\n> >\n> > private:\n> > -\tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelperFactory)\n> > +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelperFactoryBase)\n> >\n> > -\tstatic void registerType(CameraSensorHelperFactory *factory);\n> > +\tstatic void registerType(CameraSensorHelperFactoryBase *factory);\n> >\n> > \tvirtual std::unique_ptr createInstance() const = 0;\n> >\n> > \tstd::string name_;\n> > };\n> >\n> > -#define REGISTER_CAMERA_SENSOR_HELPER(name, helper)\t\t\\\n> > -class helper##Factory final : public CameraSensorHelperFactory\t\\\n> > -{\t\t\t\t\t\t\t\t\\\n> > -public: \t\t\t\t\t\t\t\\\n> > -\thelper##Factory() : CameraSensorHelperFactory(name) {}\t\\\n> > -\t\t\t\t\t\t\t\t\\\n> > -private:\t\t\t\t\t\t\t\\\n> > -\tstd::unique_ptr createInstance() const \\\n> > -\t{\t\t\t\t\t\t\t\\\n> > -\t\treturn std::make_unique();\t\t\\\n> > -\t}\t\t\t\t\t\t\t\\\n> > -};\t\t\t\t\t\t\t\t\\\n> > -static helper##Factory global_##helper##Factory;\n> > +template\n> > +class CameraSensorHelperFactory final : public CameraSensorHelperFactoryBase\n> > +{\n> > +public:\n> > +\tCameraSensorHelperFactory(const char *name)\n> > +\t\t: CameraSensorHelperFactoryBase(name)\n> > +\t{\n> > +\t}\n> > +\n> > +private:\n> > +\tstd::unique_ptr createInstance() const\n> > +\t{\n> > +\t\treturn std::make_unique<_Helper>();\n> > +\t}\n> > +};\n> > +\n> > +#define REGISTER_CAMERA_SENSOR_HELPER(name, helper) \\\n> > +static CameraSensorHelperFactory global_##helper##Factory(name);\n> \n> All minors\n> Reviewed-by: Jacopo Mondi \n> \n> >\n> > } /* namespace ipa */\n> >\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index 1335e3d14df2..9451cb03a285 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -143,7 +143,7 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> > \t/* Cache the value to set it in configure. */\n> > \thwRevision_ = static_cast(hwRevision);\n> >\n> > -\tcamHelper_ = CameraSensorHelperFactory::create(settings.sensorModel);\n> > +\tcamHelper_ = CameraSensorHelperFactoryBase::create(settings.sensorModel);\n> > \tif (!camHelper_) {\n> > \t\tLOG(IPARkISP1, Error)\n> > \t\t\t<< \"Failed to create camera sensor helper for \"","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 8883CC0DA4\n\tfor ;\n\tFri, 7 Oct 2022 14:27:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3627362D22;\n\tFri, 7 Oct 2022 16:27:17 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 03EC562D10\n\tfor ;\n\tFri, 7 Oct 2022 16:27:16 +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 6C2E9BBE;\n\tFri, 7 Oct 2022 16:27:15 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1665152837;\n\tbh=05pw7oyAhFIPE5YcvZ/GGRI0lcRC/Eh+7HbUkGmFgJM=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=mQ2UjSzYQT600FxwIJvFTOrJi9M3rbPUGSXW219Yr7Hw0VrKBJbH9V7pS6Z1lNDM/\n\tXrUaXR/J6LSzgIEA6C83hSWZeak8i/+66oCy6bsKgPdmsg1KOdmpc/Qi/0xHc0FDqM\n\tDVNk1pPbcEYdXFk4izSzKll5PmGqYWK/BXXOwsFJEkRDtQcfHawqyUTgccvpeRcfg0\n\tDzlepx9bsMytMGdI+IXR34xS9nK9vb83BKfRyKgTVE2A/cbKvyuSK9bdhPNiU7k6B6\n\tk7y4ipM4YugrgDxoHQFNkF/ek09mh+y4mq37BVgcS+DY5lKA/N9A3m83uG/wzANjiN\n\tyXIC9kVXYywuQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1665152835;\n\tbh=05pw7oyAhFIPE5YcvZ/GGRI0lcRC/Eh+7HbUkGmFgJM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=V6VHp0NcjOBE9BH/ypZ+gs3Ke4UPSX4Va4iC6F7BV8DyRov7at3fTDoY5eeS1HcwF\n\twClh+9NPPBzbFpocALIdMgduei8Y6o/AkDlLaPiI6MzhW8PWfnB3PSAHu1K3JztKdZ\n\taNEHvK5KSh+mjlkYkI6+oJC/jbfUu3rbFC92Q8WQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"V6VHp0Nc\"; dkim-atps=neutral","Date":"Fri, 7 Oct 2022 17:27:10 +0300","To":"Jacopo Mondi ","Message-ID":"","References":"<20221003212128.32429-1-laurent.pinchart@ideasonboard.com>\n\t<20221003212128.32429-5-laurent.pinchart@ideasonboard.com>\n\t<20221007140841.cm2uaaodkl7rds5x@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221007140841.cm2uaaodkl7rds5x@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 4/8] ipa: camera_sensor_helper:\n\tImplement factories through class templates","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]