[{"id":28890,"web_url":"https://patchwork.libcamera.org/comment/28890/","msgid":"<20240306170853.zswzbcssnp5vqrnx@jasper>","date":"2024-03-06T17:08:53","subject":"Re: [PATCH/RFC 21/32] libcamera: camera_sensor: Sort factories by\n\tpriority","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Laurent,\n\nOn Fri, Mar 01, 2024 at 11:21:10PM +0200, Laurent Pinchart wrote:\n> In order to support a default implementation for camera sensors when no\n> better implementation matches, libcamera needs to try \"specialized\"\n> implementations first and pick the default last. Make this possible by\n> adding a priority value for factories. Newly registered factories are\n> inserted in the factories list sorted by descending priority, and the\n> default factory uses a negative priority to be inserted as the last\n> element.\n> \n> This mechanism may be a bit overkill in the sense that there is no\n> expected use cases for priorities other than trying the default last,\n> but the implementation is simple and easy to understand.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> include/libcamera/internal/camera_sensor.h | 18 +++++++---\n> src/libcamera/sensor/camera_sensor.cpp | 34 +++++++++++++++++--\n> src/libcamera/sensor/camera_sensor_legacy.cpp | 2 +-\n> 3 files changed, 45 insertions(+), 9 deletions(-)\n> \n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index d37e66773195..59785ff62019 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -79,11 +79,13 @@ public:\n> class CameraSensorFactoryBase\n> {\n> public:\n> -\tCameraSensorFactoryBase();\n> +\tCameraSensorFactoryBase(int priority);\n> \tvirtual ~CameraSensorFactoryBase() = default;\n> \n> \tstatic std::unique_ptr create(MediaEntity *entity);\n> \n> +\tint priority() const { return priority_; }\n> +\n> private:\n> \tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorFactoryBase)\n> \n> @@ -93,14 +95,16 @@ private:\n> \n> \tvirtual std::variant, int>\n> \tmatch(MediaEntity *entity) const = 0;\n> +\n> +\tint priority_;\n> };\n> \n> template\n> class CameraSensorFactory final : public CameraSensorFactoryBase\n> {\n> public:\n> -\tCameraSensorFactory()\n> -\t\t: CameraSensorFactoryBase()\n> +\tCameraSensorFactory(int priority = 0)\n\nWhy the default value? Wouldn't it be clearer to require a priority.\nThis would prevent and accidential registration without thinking about that value and \nrunning into unexpected runtime issue.\n\n> +\t\t: CameraSensorFactoryBase(priority)\n> \t{\n> \t}\n> \n> @@ -112,7 +116,11 @@ private:\n> \t}\n> };\n> \n> -#define REGISTER_CAMERA_SENSOR(sensor) \\\n> -\tstatic CameraSensorFactory global_##sensor##Factory{};\n> +#ifndef __DOXYGEN__\n> +#define REGISTER_CAMERA_SENSOR(sensor, ...) \\\n> +\tstatic CameraSensorFactory global_##sensor##Factory{ __VA_ARGS__ };\n> +#else\n> +#define REGISTER_CAMERA_SENSOR(sensor, priority)\n> +#endif\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp\n> index 7abbe2c76596..e7b526c2f050 100644\n> --- a/src/libcamera/sensor/camera_sensor.cpp\n> +++ b/src/libcamera/sensor/camera_sensor.cpp\n> @@ -348,11 +348,13 @@ CameraSensor::~CameraSensor() = default;\n> \n> /**\n> * \\brief Construct a camera sensor factory base\n> + * \\param[in] priority Priority order for factory selection\n> *\n> * Creating an instance of the factory base registers it with the global list of\n> * factories, accessible through the factories() function.\n> */\n> -CameraSensorFactoryBase::CameraSensorFactoryBase()\n> +CameraSensorFactoryBase::CameraSensorFactoryBase(int priority)\n> +\t: priority_(priority)\n> {\n> \tregisterFactory(this);\n> }\n> @@ -361,6 +363,12 @@ CameraSensorFactoryBase::CameraSensorFactoryBase()\n> * \\brief Create an instance of the CameraSensor corresponding to a media entity\n> * \\param[in] entity The media entity on the source end of the sensor\n> *\n> + * When multiple factories match the same \\a entity, this function selects the\n> + * matching factory with the highest priority as specified to the\n> + * REGISTER_CAMERA_SENSOR() macro at factory registration time. If multiple\n> + * matching factories have the same highest priority value, which factory gets\n> + * selected is undefined and may vary between runs.\n\nThat doesn't feel good. Shouldn't we error out if multiple factories have the same priority?\nAs far as I understand the system all factories are registered inside libcamera, so all priorities\nare under our control. Maybe a bit academical but who knows how many factories the future brings :-)\n\nCheers,\nStefan\n\n> + *\n> * \\return A unique pointer to a new instance of the CameraSensor subclass\n> * matching the entity, or a null pointer if no such factory exists\n> */\n> @@ -387,8 +395,17 @@ std::unique_ptr CameraSensorFactoryBase::create(MediaEntity *entit\n> \treturn nullptr;\n> }\n> \n> +/**\n> + * \\fn CameraSensorFactoryBase::priority()\n> + * \\brief Retrieve the priority value for the factory\n> + * \\return The priority value for the factory\n> + */\n> +\n> /**\n> * \\brief Retrieve the list of all camera sensor factories\n> + *\n> + * The factories are sorted in decreasing priority order.\n> + *\n> * \\return The list of camera sensor factories\n> */\n> std::vector &CameraSensorFactoryBase::factories()\n> @@ -411,7 +428,12 @@ void CameraSensorFactoryBase::registerFactory(CameraSensorFactoryBase *factory)\n> \tstd::vector &factories =\n> \t\tCameraSensorFactoryBase::factories();\n> \n> -\tfactories.push_back(factory);\n> +\tauto pos = std::upper_bound(factories.begin(), factories.end(), factory,\n> +\t\t\t\t [](const CameraSensorFactoryBase *value,\n> +\t\t\t\t const CameraSensorFactoryBase *elem) {\n> +\t\t\t\t\t return value->priority() > elem->priority();\n> +\t\t\t\t });\n> +\tfactories.insert(pos, factory);\n> }\n> \n> /**\n> @@ -437,9 +459,10 @@ void CameraSensorFactoryBase::registerFactory(CameraSensorFactoryBase *factory)\n> */\n> \n> /**\n> - * \\def REGISTER_CAMERA_SENSOR(sensor)\n> + * \\def REGISTER_CAMERA_SENSOR(sensor, priority)\n> * \\brief Register a camera sensor type to the sensor factory\n> * \\param[in] sensor Class name of the CameraSensor derived class to register\n> + * \\param[in] priority Priority order for factory selection\n> *\n> * Register a CameraSensor subclass with the factory and make it available to\n> * try and match sensors. The subclass needs to implement a static match\n> @@ -457,6 +480,11 @@ void CameraSensorFactoryBase::registerFactory(CameraSensorFactoryBase *factory)\n> * creation succeeded ;\n> * - A non-zero error code if the entity matched and the creation failed ; or\n> * - A zero error code if the entity didn't match.\n> + *\n> + * When multiple factories can support the same MediaEntity (as in the match()\n> + * function of multiple factories returning true for the same entity), the \\a\n> + * priority argument selects which factory will be used. See\n> + * CameraSensorFactoryBase::create() for more information.\n> */\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp\n> index 34677339241c..f9f889a125d0 100644\n> --- a/src/libcamera/sensor/camera_sensor_legacy.cpp\n> +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp\n> @@ -1010,6 +1010,6 @@ std::string CameraSensorLegacy::logPrefix() const\n> \treturn \"'\" + entity_->name() + \"'\";\n> }\n> \n> -REGISTER_CAMERA_SENSOR(CameraSensorLegacy)\n> +REGISTER_CAMERA_SENSOR(CameraSensorLegacy, -100)\n> \n> } /* namespace libcamera */\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 78F63BD160\n\tfor ;\n\tWed, 6 Mar 2024 17:08:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EA6166286F;\n\tWed, 6 Mar 2024 18:08:58 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 75FAB61C8F\n\tfor ;\n\tWed, 6 Mar 2024 18:08:56 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:dd65:a3ea:5f4d:989a])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7CF6E552;\n\tWed, 6 Mar 2024 18:08:38 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"N9l20Khg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1709744918;\n\tbh=3mW8fakUaya027fVsj+xBWGSNBfoB1vgA5Cb7Mal5Wc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=N9l20KhgSMAVnVcviRLm2UcwwRTKGWt1xK0TsJX1xTU4Q1h2WS7IWWBhjC3//ReiA\n\tS7WbX+ZooXKBP2PDzDSsFm0tAqUEZoQkJG+/n58Guk3gyIIHu9Zg9RXGcxM44mbvSP\n\tSqy/PdZvEAvei8uySMkYMksC9promExHt/1YeH6c=","Date":"Wed, 6 Mar 2024 18:08:53 +0100","From":"Stefan Klug ","To":"Laurent Pinchart ","Subject":"Re: [PATCH/RFC 21/32] libcamera: camera_sensor: Sort factories by\n\tpriority","Message-ID":"<20240306170853.zswzbcssnp5vqrnx@jasper>","References":"<20240301212121.9072-1-laurent.pinchart@ideasonboard.com>\n\t<20240301212121.9072-22-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240301212121.9072-22-laurent.pinchart@ideasonboard.com>","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, Sakari Ailus ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28949,"web_url":"https://patchwork.libcamera.org/comment/28949/","msgid":"<20240313185843.GB8399@pendragon.ideasonboard.com>","date":"2024-03-13T18:58:43","subject":"Re: [PATCH/RFC 21/32] libcamera: camera_sensor: Sort factories by\n\tpriority","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Stefan,\n\nOn Wed, Mar 06, 2024 at 06:08:53PM +0100, Stefan Klug wrote:\n> On Fri, Mar 01, 2024 at 11:21:10PM +0200, Laurent Pinchart wrote:\n> > In order to support a default implementation for camera sensors when no\n> > better implementation matches, libcamera needs to try \"specialized\"\n> > implementations first and pick the default last. Make this possible by\n> > adding a priority value for factories. Newly registered factories are\n> > inserted in the factories list sorted by descending priority, and the\n> > default factory uses a negative priority to be inserted as the last\n> > element.\n> > \n> > This mechanism may be a bit overkill in the sense that there is no\n> > expected use cases for priorities other than trying the default last,\n> > but the implementation is simple and easy to understand.\n> > \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > include/libcamera/internal/camera_sensor.h | 18 +++++++---\n> > src/libcamera/sensor/camera_sensor.cpp | 34 +++++++++++++++++--\n> > src/libcamera/sensor/camera_sensor_legacy.cpp | 2 +-\n> > 3 files changed, 45 insertions(+), 9 deletions(-)\n> > \n> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > index d37e66773195..59785ff62019 100644\n> > --- a/include/libcamera/internal/camera_sensor.h\n> > +++ b/include/libcamera/internal/camera_sensor.h\n> > @@ -79,11 +79,13 @@ public:\n> > class CameraSensorFactoryBase\n> > {\n> > public:\n> > -\tCameraSensorFactoryBase();\n> > +\tCameraSensorFactoryBase(int priority);\n> > \tvirtual ~CameraSensorFactoryBase() = default;\n> > \n> > \tstatic std::unique_ptr create(MediaEntity *entity);\n> > \n> > +\tint priority() const { return priority_; }\n> > +\n> > private:\n> > \tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorFactoryBase)\n> > \n> > @@ -93,14 +95,16 @@ private:\n> > \n> > \tvirtual std::variant, int>\n> > \tmatch(MediaEntity *entity) const = 0;\n> > +\n> > +\tint priority_;\n> > };\n> > \n> > template\n> > class CameraSensorFactory final : public CameraSensorFactoryBase\n> > {\n> > public:\n> > -\tCameraSensorFactory()\n> > -\t\t: CameraSensorFactoryBase()\n> > +\tCameraSensorFactory(int priority = 0)\n> \n> Why the default value? Wouldn't it be clearer to require a priority.\n> This would prevent and accidential registration without thinking about\n> that value and running into unexpected runtime issue.\n\nGiven that I expect few CameraSensor implementations to be added to\nlibcamera, I'm not sure we'll ever have such accidents, but I can\nrequire the priority.\n\n> > +\t\t: CameraSensorFactoryBase(priority)\n> > \t{\n> > \t}\n> > \n> > @@ -112,7 +116,11 @@ private:\n> > \t}\n> > };\n> > \n> > -#define REGISTER_CAMERA_SENSOR(sensor) \\\n> > -\tstatic CameraSensorFactory global_##sensor##Factory{};\n> > +#ifndef __DOXYGEN__\n> > +#define REGISTER_CAMERA_SENSOR(sensor, ...) \\\n> > +\tstatic CameraSensorFactory global_##sensor##Factory{ __VA_ARGS__ };\n> > +#else\n> > +#define REGISTER_CAMERA_SENSOR(sensor, priority)\n> > +#endif\n> > \n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp\n> > index 7abbe2c76596..e7b526c2f050 100644\n> > --- a/src/libcamera/sensor/camera_sensor.cpp\n> > +++ b/src/libcamera/sensor/camera_sensor.cpp\n> > @@ -348,11 +348,13 @@ CameraSensor::~CameraSensor() = default;\n> > \n> > /**\n> > * \\brief Construct a camera sensor factory base\n> > + * \\param[in] priority Priority order for factory selection\n> > *\n> > * Creating an instance of the factory base registers it with the global list of\n> > * factories, accessible through the factories() function.\n> > */\n> > -CameraSensorFactoryBase::CameraSensorFactoryBase()\n> > +CameraSensorFactoryBase::CameraSensorFactoryBase(int priority)\n> > +\t: priority_(priority)\n> > {\n> > \tregisterFactory(this);\n> > }\n> > @@ -361,6 +363,12 @@ CameraSensorFactoryBase::CameraSensorFactoryBase()\n> > * \\brief Create an instance of the CameraSensor corresponding to a media entity\n> > * \\param[in] entity The media entity on the source end of the sensor\n> > *\n> > + * When multiple factories match the same \\a entity, this function selects the\n> > + * matching factory with the highest priority as specified to the\n> > + * REGISTER_CAMERA_SENSOR() macro at factory registration time. If multiple\n> > + * matching factories have the same highest priority value, which factory gets\n> > + * selected is undefined and may vary between runs.\n> \n> That doesn't feel good. Shouldn't we error out if multiple factories\n> have the same priority? As far as I understand the system all\n> factories are registered inside libcamera, so all priorities are under\n> our control. Maybe a bit academical but who knows how many factories\n> the future brings :-)\n\nWe could, but we would then have to try and match all the factories of\nthe same priority. As I expect all factories but the legacy to have\npriority 0, it means libcamera will try to match all the non-legacy\nfactories, slowing down the initialization process.\n\n> > + *\n> > * \\return A unique pointer to a new instance of the CameraSensor subclass\n> > * matching the entity, or a null pointer if no such factory exists\n> > */\n> > @@ -387,8 +395,17 @@ std::unique_ptr CameraSensorFactoryBase::create(MediaEntity *entit\n> > \treturn nullptr;\n> > }\n> > \n> > +/**\n> > + * \\fn CameraSensorFactoryBase::priority()\n> > + * \\brief Retrieve the priority value for the factory\n> > + * \\return The priority value for the factory\n> > + */\n> > +\n> > /**\n> > * \\brief Retrieve the list of all camera sensor factories\n> > + *\n> > + * The factories are sorted in decreasing priority order.\n> > + *\n> > * \\return The list of camera sensor factories\n> > */\n> > std::vector &CameraSensorFactoryBase::factories()\n> > @@ -411,7 +428,12 @@ void CameraSensorFactoryBase::registerFactory(CameraSensorFactoryBase *factory)\n> > \tstd::vector &factories =\n> > \t\tCameraSensorFactoryBase::factories();\n> > \n> > -\tfactories.push_back(factory);\n> > +\tauto pos = std::upper_bound(factories.begin(), factories.end(), factory,\n> > +\t\t\t\t [](const CameraSensorFactoryBase *value,\n> > +\t\t\t\t const CameraSensorFactoryBase *elem) {\n> > +\t\t\t\t\t return value->priority() > elem->priority();\n> > +\t\t\t\t });\n> > +\tfactories.insert(pos, factory);\n> > }\n> > \n> > /**\n> > @@ -437,9 +459,10 @@ void CameraSensorFactoryBase::registerFactory(CameraSensorFactoryBase *factory)\n> > */\n> > \n> > /**\n> > - * \\def REGISTER_CAMERA_SENSOR(sensor)\n> > + * \\def REGISTER_CAMERA_SENSOR(sensor, priority)\n> > * \\brief Register a camera sensor type to the sensor factory\n> > * \\param[in] sensor Class name of the CameraSensor derived class to register\n> > + * \\param[in] priority Priority order for factory selection\n> > *\n> > * Register a CameraSensor subclass with the factory and make it available to\n> > * try and match sensors. The subclass needs to implement a static match\n> > @@ -457,6 +480,11 @@ void CameraSensorFactoryBase::registerFactory(CameraSensorFactoryBase *factory)\n> > * creation succeeded ;\n> > * - A non-zero error code if the entity matched and the creation failed ; or\n> > * - A zero error code if the entity didn't match.\n> > + *\n> > + * When multiple factories can support the same MediaEntity (as in the match()\n> > + * function of multiple factories returning true for the same entity), the \\a\n> > + * priority argument selects which factory will be used. See\n> > + * CameraSensorFactoryBase::create() for more information.\n> > */\n> > \n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp\n> > index 34677339241c..f9f889a125d0 100644\n> > --- a/src/libcamera/sensor/camera_sensor_legacy.cpp\n> > +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp\n> > @@ -1010,6 +1010,6 @@ std::string CameraSensorLegacy::logPrefix() const\n> > \treturn \"'\" + entity_->name() + \"'\";\n> > }\n> > \n> > -REGISTER_CAMERA_SENSOR(CameraSensorLegacy)\n> > +REGISTER_CAMERA_SENSOR(CameraSensorLegacy, -100)\n> > \n> > } /* namespace libcamera */","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 E4715BD160\n\tfor ;\n\tWed, 13 Mar 2024 18:58:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 117D262C8B;\n\tWed, 13 Mar 2024 19:58:44 +0100 (CET)","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 8542D62C80\n\tfor ;\n\tWed, 13 Mar 2024 19:58:42 +0100 (CET)","from pendragon.ideasonboard.com (213-209-177-254.ip.skylogicnet.com\n\t[213.209.177.254])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D7782A8F;\n\tWed, 13 Mar 2024 19:58:18 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"uJBre0jL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1710356299;\n\tbh=zJ4Wf9tzIBduLOujXLByDQAsBF90oKD6AoFU1azKY78=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uJBre0jLAgShoA+X+iXcuaILn80Olaui7AlIrs3AVd8ZTQeUKEHv+0/hIgdwmTXE9\n\t7UCgnC5H7AI3rc+mq3QLXOh/U/Q805vwVmmEz2i6I7LTaKY2cnMtoHqBnGzexJh839\n\tOTXQ9CLLPafGRKO/wK1oA1Sj2exRnpb/mNYD7wSk=","Date":"Wed, 13 Mar 2024 20:58:43 +0200","From":"Laurent Pinchart ","To":"Stefan Klug ","Subject":"Re: [PATCH/RFC 21/32] libcamera: camera_sensor: Sort factories by\n\tpriority","Message-ID":"<20240313185843.GB8399@pendragon.ideasonboard.com>","References":"<20240301212121.9072-1-laurent.pinchart@ideasonboard.com>\n\t<20240301212121.9072-22-laurent.pinchart@ideasonboard.com>\n\t<20240306170853.zswzbcssnp5vqrnx@jasper>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240306170853.zswzbcssnp5vqrnx@jasper>","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, Sakari Ailus ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28954,"web_url":"https://patchwork.libcamera.org/comment/28954/","msgid":"<20240314095243.b6v5ylgfv2wouzbu@jasper>","date":"2024-03-14T09:52:43","subject":"Re: [PATCH/RFC 21/32] libcamera: camera_sensor: Sort factories by\n\tpriority","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"On Wed, Mar 13, 2024 at 08:58:43PM +0200, Laurent Pinchart wrote:\nHi Laurent,\n\n> Hi Stefan,\n> \n> On Wed, Mar 06, 2024 at 06:08:53PM +0100, Stefan Klug wrote:\n> > On Fri, Mar 01, 2024 at 11:21:10PM +0200, Laurent Pinchart wrote:\n> > > In order to support a default implementation for camera sensors when no\n> > > better implementation matches, libcamera needs to try \"specialized\"\n> > > implementations first and pick the default last. Make this possible by\n> > > adding a priority value for factories. Newly registered factories are\n> > > inserted in the factories list sorted by descending priority, and the\n> > > default factory uses a negative priority to be inserted as the last\n> > > element.\n> > > \n> > > This mechanism may be a bit overkill in the sense that there is no\n> > > expected use cases for priorities other than trying the default last,\n> > > but the implementation is simple and easy to understand.\n> > > \n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > include/libcamera/internal/camera_sensor.h | 18 +++++++---\n> > > src/libcamera/sensor/camera_sensor.cpp | 34 +++++++++++++++++--\n> > > src/libcamera/sensor/camera_sensor_legacy.cpp | 2 +-\n> > > 3 files changed, 45 insertions(+), 9 deletions(-)\n> > > \n> > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > > index d37e66773195..59785ff62019 100644\n> > > --- a/include/libcamera/internal/camera_sensor.h\n> > > +++ b/include/libcamera/internal/camera_sensor.h\n> > > @@ -79,11 +79,13 @@ public:\n> > > class CameraSensorFactoryBase\n> > > {\n> > > public:\n> > > -\tCameraSensorFactoryBase();\n> > > +\tCameraSensorFactoryBase(int priority);\n> > > \tvirtual ~CameraSensorFactoryBase() = default;\n> > > \n> > > \tstatic std::unique_ptr create(MediaEntity *entity);\n> > > \n> > > +\tint priority() const { return priority_; }\n> > > +\n> > > private:\n> > > \tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorFactoryBase)\n> > > \n> > > @@ -93,14 +95,16 @@ private:\n> > > \n> > > \tvirtual std::variant, int>\n> > > \tmatch(MediaEntity *entity) const = 0;\n> > > +\n> > > +\tint priority_;\n> > > };\n> > > \n> > > template\n> > > class CameraSensorFactory final : public CameraSensorFactoryBase\n> > > {\n> > > public:\n> > > -\tCameraSensorFactory()\n> > > -\t\t: CameraSensorFactoryBase()\n> > > +\tCameraSensorFactory(int priority = 0)\n> > \n> > Why the default value? Wouldn't it be clearer to require a priority.\n> > This would prevent and accidential registration without thinking about\n> > that value and running into unexpected runtime issue.\n> \n> Given that I expect few CameraSensor implementations to be added to\n> libcamera, I'm not sure we'll ever have such accidents, but I can\n> require the priority.\n\nYes it's pretty unlikely.Do as you like.\n\n> \n> > > +\t\t: CameraSensorFactoryBase(priority)\n> > > \t{\n> > > \t}\n> > > \n> > > @@ -112,7 +116,11 @@ private:\n> > > \t}\n> > > };\n> > > \n> > > -#define REGISTER_CAMERA_SENSOR(sensor) \\\n> > > -\tstatic CameraSensorFactory global_##sensor##Factory{};\n> > > +#ifndef __DOXYGEN__\n> > > +#define REGISTER_CAMERA_SENSOR(sensor, ...) \\\n> > > +\tstatic CameraSensorFactory global_##sensor##Factory{ __VA_ARGS__ };\n> > > +#else\n> > > +#define REGISTER_CAMERA_SENSOR(sensor, priority)\n> > > +#endif\n> > > \n> > > } /* namespace libcamera */\n> > > diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp\n> > > index 7abbe2c76596..e7b526c2f050 100644\n> > > --- a/src/libcamera/sensor/camera_sensor.cpp\n> > > +++ b/src/libcamera/sensor/camera_sensor.cpp\n> > > @@ -348,11 +348,13 @@ CameraSensor::~CameraSensor() = default;\n> > > \n> > > /**\n> > > * \\brief Construct a camera sensor factory base\n> > > + * \\param[in] priority Priority order for factory selection\n> > > *\n> > > * Creating an instance of the factory base registers it with the global list of\n> > > * factories, accessible through the factories() function.\n> > > */\n> > > -CameraSensorFactoryBase::CameraSensorFactoryBase()\n> > > +CameraSensorFactoryBase::CameraSensorFactoryBase(int priority)\n> > > +\t: priority_(priority)\n> > > {\n> > > \tregisterFactory(this);\n> > > }\n> > > @@ -361,6 +363,12 @@ CameraSensorFactoryBase::CameraSensorFactoryBase()\n> > > * \\brief Create an instance of the CameraSensor corresponding to a media entity\n> > > * \\param[in] entity The media entity on the source end of the sensor\n> > > *\n> > > + * When multiple factories match the same \\a entity, this function selects the\n> > > + * matching factory with the highest priority as specified to the\n> > > + * REGISTER_CAMERA_SENSOR() macro at factory registration time. If multiple\n> > > + * matching factories have the same highest priority value, which factory gets\n> > > + * selected is undefined and may vary between runs.\n> > \n> > That doesn't feel good. Shouldn't we error out if multiple factories\n> > have the same priority? As far as I understand the system all\n> > factories are registered inside libcamera, so all priorities are under\n> > our control. Maybe a bit academical but who knows how many factories\n> > the future brings :-)\n> \n> We could, but we would then have to try and match all the factories of\n> the same priority. As I expect all factories but the legacy to have\n> priority 0, it means libcamera will try to match all the non-legacy\n> factories, slowing down the initialization process.\n> \n\nFair enough. Thanks for the explanation :-)\n\nReviewed-by: Stefan Klug \n\nCheers,\nStefan \n\n> > > + *\n> > > * \\return A unique pointer to a new instance of the CameraSensor subclass\n> > > * matching the entity, or a null pointer if no such factory exists\n> > > */\n> > > @@ -387,8 +395,17 @@ std::unique_ptr CameraSensorFactoryBase::create(MediaEntity *entit\n> > > \treturn nullptr;\n> > > }\n> > > \n> > > +/**\n> > > + * \\fn CameraSensorFactoryBase::priority()\n> > > + * \\brief Retrieve the priority value for the factory\n> > > + * \\return The priority value for the factory\n> > > + */\n> > > +\n> > > /**\n> > > * \\brief Retrieve the list of all camera sensor factories\n> > > + *\n> > > + * The factories are sorted in decreasing priority order.\n> > > + *\n> > > * \\return The list of camera sensor factories\n> > > */\n> > > std::vector &CameraSensorFactoryBase::factories()\n> > > @@ -411,7 +428,12 @@ void CameraSensorFactoryBase::registerFactory(CameraSensorFactoryBase *factory)\n> > > \tstd::vector &factories =\n> > > \t\tCameraSensorFactoryBase::factories();\n> > > \n> > > -\tfactories.push_back(factory);\n> > > +\tauto pos = std::upper_bound(factories.begin(), factories.end(), factory,\n> > > +\t\t\t\t [](const CameraSensorFactoryBase *value,\n> > > +\t\t\t\t const CameraSensorFactoryBase *elem) {\n> > > +\t\t\t\t\t return value->priority() > elem->priority();\n> > > +\t\t\t\t });\n> > > +\tfactories.insert(pos, factory);\n> > > }\n> > > \n> > > /**\n> > > @@ -437,9 +459,10 @@ void CameraSensorFactoryBase::registerFactory(CameraSensorFactoryBase *factory)\n> > > */\n> > > \n> > > /**\n> > > - * \\def REGISTER_CAMERA_SENSOR(sensor)\n> > > + * \\def REGISTER_CAMERA_SENSOR(sensor, priority)\n> > > * \\brief Register a camera sensor type to the sensor factory\n> > > * \\param[in] sensor Class name of the CameraSensor derived class to register\n> > > + * \\param[in] priority Priority order for factory selection\n> > > *\n> > > * Register a CameraSensor subclass with the factory and make it available to\n> > > * try and match sensors. The subclass needs to implement a static match\n> > > @@ -457,6 +480,11 @@ void CameraSensorFactoryBase::registerFactory(CameraSensorFactoryBase *factory)\n> > > * creation succeeded ;\n> > > * - A non-zero error code if the entity matched and the creation failed ; or\n> > > * - A zero error code if the entity didn't match.\n> > > + *\n> > > + * When multiple factories can support the same MediaEntity (as in the match()\n> > > + * function of multiple factories returning true for the same entity), the \\a\n> > > + * priority argument selects which factory will be used. See\n> > > + * CameraSensorFactoryBase::create() for more information.\n> > > */\n> > > \n> > > } /* namespace libcamera */\n> > > diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp\n> > > index 34677339241c..f9f889a125d0 100644\n> > > --- a/src/libcamera/sensor/camera_sensor_legacy.cpp\n> > > +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp\n> > > @@ -1010,6 +1010,6 @@ std::string CameraSensorLegacy::logPrefix() const\n> > > \treturn \"'\" + entity_->name() + \"'\";\n> > > }\n> > > \n> > > -REGISTER_CAMERA_SENSOR(CameraSensorLegacy)\n> > > +REGISTER_CAMERA_SENSOR(CameraSensorLegacy, -100)\n> > > \n> > > } /* namespace libcamera */\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 ED5A4BD160\n\tfor ;\n\tThu, 14 Mar 2024 09:52:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BAC4D62C97;\n\tThu, 14 Mar 2024 10:52:48 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AE6F362C92\n\tfor ;\n\tThu, 14 Mar 2024 10:52:46 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:c467:7cd1:67a9:bc4c])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 648496BE;\n\tThu, 14 Mar 2024 10:52:23 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Q4qV47oY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1710409943;\n\tbh=Vq+cMAUU/JJsXocp7jFfdM8SaEk9EBIPMLKu7NjTXo8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Q4qV47oYA48l4D7PBTTd27SJ7JT8XL/fgJpN0/gajmxqx37H0VUYc+N6rw/UH1hdg\n\trn3S5HxaLauA48zho3ScBqJPTM8aj2E7nzrvY2AE5n6ndJUud4SUaIkSyyUFW1p2iu\n\tL8LbDASZPCHeCbuomcg4QQ+scPKoPPKE8UkRXe/8=","Date":"Thu, 14 Mar 2024 10:52:43 +0100","From":"Stefan Klug ","To":"Laurent Pinchart ","Subject":"Re: [PATCH/RFC 21/32] libcamera: camera_sensor: Sort factories by\n\tpriority","Message-ID":"<20240314095243.b6v5ylgfv2wouzbu@jasper>","References":"<20240301212121.9072-1-laurent.pinchart@ideasonboard.com>\n\t<20240301212121.9072-22-laurent.pinchart@ideasonboard.com>\n\t<20240306170853.zswzbcssnp5vqrnx@jasper>\n\t<20240313185843.GB8399@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240313185843.GB8399@pendragon.ideasonboard.com>","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, Sakari Ailus ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]