| Message ID | 20240301212121.9072-22-laurent.pinchart@ideasonboard.com |
|---|---|
| State | RFC |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On Fri, Mar 01, 2024 at 11:21:10PM +0200, Laurent Pinchart wrote: > In order to support a default implementation for camera sensors when no > better implementation matches, libcamera needs to try "specialized" > implementations first and pick the default last. Make this possible by > adding a priority value for factories. Newly registered factories are > inserted in the factories list sorted by descending priority, and the > default factory uses a negative priority to be inserted as the last > element. > > This mechanism may be a bit overkill in the sense that there is no > expected use cases for priorities other than trying the default last, > but the implementation is simple and easy to understand. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > include/libcamera/internal/camera_sensor.h | 18 +++++++--- > src/libcamera/sensor/camera_sensor.cpp | 34 +++++++++++++++++-- > src/libcamera/sensor/camera_sensor_legacy.cpp | 2 +- > 3 files changed, 45 insertions(+), 9 deletions(-) > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > index d37e66773195..59785ff62019 100644 > --- a/include/libcamera/internal/camera_sensor.h > +++ b/include/libcamera/internal/camera_sensor.h > @@ -79,11 +79,13 @@ public: > class CameraSensorFactoryBase > { > public: > - CameraSensorFactoryBase(); > + CameraSensorFactoryBase(int priority); > virtual ~CameraSensorFactoryBase() = default; > > static std::unique_ptr<CameraSensor> create(MediaEntity *entity); > > + int priority() const { return priority_; } > + > private: > LIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorFactoryBase) > > @@ -93,14 +95,16 @@ private: > > virtual std::variant<std::unique_ptr<CameraSensor>, int> > match(MediaEntity *entity) const = 0; > + > + int priority_; > }; > > template<typename _CameraSensor> > class CameraSensorFactory final : public CameraSensorFactoryBase > { > public: > - CameraSensorFactory() > - : CameraSensorFactoryBase() > + CameraSensorFactory(int priority = 0) Why the default value? Wouldn't it be clearer to require a priority. This would prevent and accidential registration without thinking about that value and running into unexpected runtime issue. > + : CameraSensorFactoryBase(priority) > { > } > > @@ -112,7 +116,11 @@ private: > } > }; > > -#define REGISTER_CAMERA_SENSOR(sensor) \ > - static CameraSensorFactory<sensor> global_##sensor##Factory{}; > +#ifndef __DOXYGEN__ > +#define REGISTER_CAMERA_SENSOR(sensor, ...) \ > + static CameraSensorFactory<sensor> global_##sensor##Factory{ __VA_ARGS__ }; > +#else > +#define REGISTER_CAMERA_SENSOR(sensor, priority) > +#endif > > } /* namespace libcamera */ > diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp > index 7abbe2c76596..e7b526c2f050 100644 > --- a/src/libcamera/sensor/camera_sensor.cpp > +++ b/src/libcamera/sensor/camera_sensor.cpp > @@ -348,11 +348,13 @@ CameraSensor::~CameraSensor() = default; > > /** > * \brief Construct a camera sensor factory base > + * \param[in] priority Priority order for factory selection > * > * Creating an instance of the factory base registers it with the global list of > * factories, accessible through the factories() function. > */ > -CameraSensorFactoryBase::CameraSensorFactoryBase() > +CameraSensorFactoryBase::CameraSensorFactoryBase(int priority) > + : priority_(priority) > { > registerFactory(this); > } > @@ -361,6 +363,12 @@ CameraSensorFactoryBase::CameraSensorFactoryBase() > * \brief Create an instance of the CameraSensor corresponding to a media entity > * \param[in] entity The media entity on the source end of the sensor > * > + * When multiple factories match the same \a entity, this function selects the > + * matching factory with the highest priority as specified to the > + * REGISTER_CAMERA_SENSOR() macro at factory registration time. If multiple > + * matching factories have the same highest priority value, which factory gets > + * selected is undefined and may vary between runs. That doesn't feel good. Shouldn't we error out if multiple factories have the same priority? As far as I understand the system all factories are registered inside libcamera, so all priorities are under our control. Maybe a bit academical but who knows how many factories the future brings :-) Cheers, Stefan > + * > * \return A unique pointer to a new instance of the CameraSensor subclass > * matching the entity, or a null pointer if no such factory exists > */ > @@ -387,8 +395,17 @@ std::unique_ptr<CameraSensor> CameraSensorFactoryBase::create(MediaEntity *entit > return nullptr; > } > > +/** > + * \fn CameraSensorFactoryBase::priority() > + * \brief Retrieve the priority value for the factory > + * \return The priority value for the factory > + */ > + > /** > * \brief Retrieve the list of all camera sensor factories > + * > + * The factories are sorted in decreasing priority order. > + * > * \return The list of camera sensor factories > */ > std::vector<CameraSensorFactoryBase *> &CameraSensorFactoryBase::factories() > @@ -411,7 +428,12 @@ void CameraSensorFactoryBase::registerFactory(CameraSensorFactoryBase *factory) > std::vector<CameraSensorFactoryBase *> &factories = > CameraSensorFactoryBase::factories(); > > - factories.push_back(factory); > + auto pos = std::upper_bound(factories.begin(), factories.end(), factory, > + [](const CameraSensorFactoryBase *value, > + const CameraSensorFactoryBase *elem) { > + return value->priority() > elem->priority(); > + }); > + factories.insert(pos, factory); > } > > /** > @@ -437,9 +459,10 @@ void CameraSensorFactoryBase::registerFactory(CameraSensorFactoryBase *factory) > */ > > /** > - * \def REGISTER_CAMERA_SENSOR(sensor) > + * \def REGISTER_CAMERA_SENSOR(sensor, priority) > * \brief Register a camera sensor type to the sensor factory > * \param[in] sensor Class name of the CameraSensor derived class to register > + * \param[in] priority Priority order for factory selection > * > * Register a CameraSensor subclass with the factory and make it available to > * try and match sensors. The subclass needs to implement a static match > @@ -457,6 +480,11 @@ void CameraSensorFactoryBase::registerFactory(CameraSensorFactoryBase *factory) > * creation succeeded ; > * - A non-zero error code if the entity matched and the creation failed ; or > * - A zero error code if the entity didn't match. > + * > + * When multiple factories can support the same MediaEntity (as in the match() > + * function of multiple factories returning true for the same entity), the \a > + * priority argument selects which factory will be used. See > + * CameraSensorFactoryBase::create() for more information. > */ > > } /* namespace libcamera */ > diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp > index 34677339241c..f9f889a125d0 100644 > --- a/src/libcamera/sensor/camera_sensor_legacy.cpp > +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp > @@ -1010,6 +1010,6 @@ std::string CameraSensorLegacy::logPrefix() const > return "'" + entity_->name() + "'"; > } > > -REGISTER_CAMERA_SENSOR(CameraSensorLegacy) > +REGISTER_CAMERA_SENSOR(CameraSensorLegacy, -100) > > } /* namespace libcamera */ > -- > Regards, > > Laurent Pinchart >
Hi Stefan, On Wed, Mar 06, 2024 at 06:08:53PM +0100, Stefan Klug wrote: > On Fri, Mar 01, 2024 at 11:21:10PM +0200, Laurent Pinchart wrote: > > In order to support a default implementation for camera sensors when no > > better implementation matches, libcamera needs to try "specialized" > > implementations first and pick the default last. Make this possible by > > adding a priority value for factories. Newly registered factories are > > inserted in the factories list sorted by descending priority, and the > > default factory uses a negative priority to be inserted as the last > > element. > > > > This mechanism may be a bit overkill in the sense that there is no > > expected use cases for priorities other than trying the default last, > > but the implementation is simple and easy to understand. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > include/libcamera/internal/camera_sensor.h | 18 +++++++--- > > src/libcamera/sensor/camera_sensor.cpp | 34 +++++++++++++++++-- > > src/libcamera/sensor/camera_sensor_legacy.cpp | 2 +- > > 3 files changed, 45 insertions(+), 9 deletions(-) > > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > > index d37e66773195..59785ff62019 100644 > > --- a/include/libcamera/internal/camera_sensor.h > > +++ b/include/libcamera/internal/camera_sensor.h > > @@ -79,11 +79,13 @@ public: > > class CameraSensorFactoryBase > > { > > public: > > - CameraSensorFactoryBase(); > > + CameraSensorFactoryBase(int priority); > > virtual ~CameraSensorFactoryBase() = default; > > > > static std::unique_ptr<CameraSensor> create(MediaEntity *entity); > > > > + int priority() const { return priority_; } > > + > > private: > > LIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorFactoryBase) > > > > @@ -93,14 +95,16 @@ private: > > > > virtual std::variant<std::unique_ptr<CameraSensor>, int> > > match(MediaEntity *entity) const = 0; > > + > > + int priority_; > > }; > > > > template<typename _CameraSensor> > > class CameraSensorFactory final : public CameraSensorFactoryBase > > { > > public: > > - CameraSensorFactory() > > - : CameraSensorFactoryBase() > > + CameraSensorFactory(int priority = 0) > > Why the default value? Wouldn't it be clearer to require a priority. > This would prevent and accidential registration without thinking about > that value and running into unexpected runtime issue. Given that I expect few CameraSensor implementations to be added to libcamera, I'm not sure we'll ever have such accidents, but I can require the priority. > > + : CameraSensorFactoryBase(priority) > > { > > } > > > > @@ -112,7 +116,11 @@ private: > > } > > }; > > > > -#define REGISTER_CAMERA_SENSOR(sensor) \ > > - static CameraSensorFactory<sensor> global_##sensor##Factory{}; > > +#ifndef __DOXYGEN__ > > +#define REGISTER_CAMERA_SENSOR(sensor, ...) \ > > + static CameraSensorFactory<sensor> global_##sensor##Factory{ __VA_ARGS__ }; > > +#else > > +#define REGISTER_CAMERA_SENSOR(sensor, priority) > > +#endif > > > > } /* namespace libcamera */ > > diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp > > index 7abbe2c76596..e7b526c2f050 100644 > > --- a/src/libcamera/sensor/camera_sensor.cpp > > +++ b/src/libcamera/sensor/camera_sensor.cpp > > @@ -348,11 +348,13 @@ CameraSensor::~CameraSensor() = default; > > > > /** > > * \brief Construct a camera sensor factory base > > + * \param[in] priority Priority order for factory selection > > * > > * Creating an instance of the factory base registers it with the global list of > > * factories, accessible through the factories() function. > > */ > > -CameraSensorFactoryBase::CameraSensorFactoryBase() > > +CameraSensorFactoryBase::CameraSensorFactoryBase(int priority) > > + : priority_(priority) > > { > > registerFactory(this); > > } > > @@ -361,6 +363,12 @@ CameraSensorFactoryBase::CameraSensorFactoryBase() > > * \brief Create an instance of the CameraSensor corresponding to a media entity > > * \param[in] entity The media entity on the source end of the sensor > > * > > + * When multiple factories match the same \a entity, this function selects the > > + * matching factory with the highest priority as specified to the > > + * REGISTER_CAMERA_SENSOR() macro at factory registration time. If multiple > > + * matching factories have the same highest priority value, which factory gets > > + * selected is undefined and may vary between runs. > > That doesn't feel good. Shouldn't we error out if multiple factories > have the same priority? As far as I understand the system all > factories are registered inside libcamera, so all priorities are under > our control. Maybe a bit academical but who knows how many factories > the future brings :-) We could, but we would then have to try and match all the factories of the same priority. As I expect all factories but the legacy to have priority 0, it means libcamera will try to match all the non-legacy factories, slowing down the initialization process. > > + * > > * \return A unique pointer to a new instance of the CameraSensor subclass > > * matching the entity, or a null pointer if no such factory exists > > */ > > @@ -387,8 +395,17 @@ std::unique_ptr<CameraSensor> CameraSensorFactoryBase::create(MediaEntity *entit > > return nullptr; > > } > > > > +/** > > + * \fn CameraSensorFactoryBase::priority() > > + * \brief Retrieve the priority value for the factory > > + * \return The priority value for the factory > > + */ > > + > > /** > > * \brief Retrieve the list of all camera sensor factories > > + * > > + * The factories are sorted in decreasing priority order. > > + * > > * \return The list of camera sensor factories > > */ > > std::vector<CameraSensorFactoryBase *> &CameraSensorFactoryBase::factories() > > @@ -411,7 +428,12 @@ void CameraSensorFactoryBase::registerFactory(CameraSensorFactoryBase *factory) > > std::vector<CameraSensorFactoryBase *> &factories = > > CameraSensorFactoryBase::factories(); > > > > - factories.push_back(factory); > > + auto pos = std::upper_bound(factories.begin(), factories.end(), factory, > > + [](const CameraSensorFactoryBase *value, > > + const CameraSensorFactoryBase *elem) { > > + return value->priority() > elem->priority(); > > + }); > > + factories.insert(pos, factory); > > } > > > > /** > > @@ -437,9 +459,10 @@ void CameraSensorFactoryBase::registerFactory(CameraSensorFactoryBase *factory) > > */ > > > > /** > > - * \def REGISTER_CAMERA_SENSOR(sensor) > > + * \def REGISTER_CAMERA_SENSOR(sensor, priority) > > * \brief Register a camera sensor type to the sensor factory > > * \param[in] sensor Class name of the CameraSensor derived class to register > > + * \param[in] priority Priority order for factory selection > > * > > * Register a CameraSensor subclass with the factory and make it available to > > * try and match sensors. The subclass needs to implement a static match > > @@ -457,6 +480,11 @@ void CameraSensorFactoryBase::registerFactory(CameraSensorFactoryBase *factory) > > * creation succeeded ; > > * - A non-zero error code if the entity matched and the creation failed ; or > > * - A zero error code if the entity didn't match. > > + * > > + * When multiple factories can support the same MediaEntity (as in the match() > > + * function of multiple factories returning true for the same entity), the \a > > + * priority argument selects which factory will be used. See > > + * CameraSensorFactoryBase::create() for more information. > > */ > > > > } /* namespace libcamera */ > > diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp > > index 34677339241c..f9f889a125d0 100644 > > --- a/src/libcamera/sensor/camera_sensor_legacy.cpp > > +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp > > @@ -1010,6 +1010,6 @@ std::string CameraSensorLegacy::logPrefix() const > > return "'" + entity_->name() + "'"; > > } > > > > -REGISTER_CAMERA_SENSOR(CameraSensorLegacy) > > +REGISTER_CAMERA_SENSOR(CameraSensorLegacy, -100) > > > > } /* namespace libcamera */
On Wed, Mar 13, 2024 at 08:58:43PM +0200, Laurent Pinchart wrote: Hi Laurent, > Hi Stefan, > > On Wed, Mar 06, 2024 at 06:08:53PM +0100, Stefan Klug wrote: > > On Fri, Mar 01, 2024 at 11:21:10PM +0200, Laurent Pinchart wrote: > > > In order to support a default implementation for camera sensors when no > > > better implementation matches, libcamera needs to try "specialized" > > > implementations first and pick the default last. Make this possible by > > > adding a priority value for factories. Newly registered factories are > > > inserted in the factories list sorted by descending priority, and the > > > default factory uses a negative priority to be inserted as the last > > > element. > > > > > > This mechanism may be a bit overkill in the sense that there is no > > > expected use cases for priorities other than trying the default last, > > > but the implementation is simple and easy to understand. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > include/libcamera/internal/camera_sensor.h | 18 +++++++--- > > > src/libcamera/sensor/camera_sensor.cpp | 34 +++++++++++++++++-- > > > src/libcamera/sensor/camera_sensor_legacy.cpp | 2 +- > > > 3 files changed, 45 insertions(+), 9 deletions(-) > > > > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > > > index d37e66773195..59785ff62019 100644 > > > --- a/include/libcamera/internal/camera_sensor.h > > > +++ b/include/libcamera/internal/camera_sensor.h > > > @@ -79,11 +79,13 @@ public: > > > class CameraSensorFactoryBase > > > { > > > public: > > > - CameraSensorFactoryBase(); > > > + CameraSensorFactoryBase(int priority); > > > virtual ~CameraSensorFactoryBase() = default; > > > > > > static std::unique_ptr<CameraSensor> create(MediaEntity *entity); > > > > > > + int priority() const { return priority_; } > > > + > > > private: > > > LIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorFactoryBase) > > > > > > @@ -93,14 +95,16 @@ private: > > > > > > virtual std::variant<std::unique_ptr<CameraSensor>, int> > > > match(MediaEntity *entity) const = 0; > > > + > > > + int priority_; > > > }; > > > > > > template<typename _CameraSensor> > > > class CameraSensorFactory final : public CameraSensorFactoryBase > > > { > > > public: > > > - CameraSensorFactory() > > > - : CameraSensorFactoryBase() > > > + CameraSensorFactory(int priority = 0) > > > > Why the default value? Wouldn't it be clearer to require a priority. > > This would prevent and accidential registration without thinking about > > that value and running into unexpected runtime issue. > > Given that I expect few CameraSensor implementations to be added to > libcamera, I'm not sure we'll ever have such accidents, but I can > require the priority. Yes it's pretty unlikely.Do as you like. > > > > + : CameraSensorFactoryBase(priority) > > > { > > > } > > > > > > @@ -112,7 +116,11 @@ private: > > > } > > > }; > > > > > > -#define REGISTER_CAMERA_SENSOR(sensor) \ > > > - static CameraSensorFactory<sensor> global_##sensor##Factory{}; > > > +#ifndef __DOXYGEN__ > > > +#define REGISTER_CAMERA_SENSOR(sensor, ...) \ > > > + static CameraSensorFactory<sensor> global_##sensor##Factory{ __VA_ARGS__ }; > > > +#else > > > +#define REGISTER_CAMERA_SENSOR(sensor, priority) > > > +#endif > > > > > > } /* namespace libcamera */ > > > diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp > > > index 7abbe2c76596..e7b526c2f050 100644 > > > --- a/src/libcamera/sensor/camera_sensor.cpp > > > +++ b/src/libcamera/sensor/camera_sensor.cpp > > > @@ -348,11 +348,13 @@ CameraSensor::~CameraSensor() = default; > > > > > > /** > > > * \brief Construct a camera sensor factory base > > > + * \param[in] priority Priority order for factory selection > > > * > > > * Creating an instance of the factory base registers it with the global list of > > > * factories, accessible through the factories() function. > > > */ > > > -CameraSensorFactoryBase::CameraSensorFactoryBase() > > > +CameraSensorFactoryBase::CameraSensorFactoryBase(int priority) > > > + : priority_(priority) > > > { > > > registerFactory(this); > > > } > > > @@ -361,6 +363,12 @@ CameraSensorFactoryBase::CameraSensorFactoryBase() > > > * \brief Create an instance of the CameraSensor corresponding to a media entity > > > * \param[in] entity The media entity on the source end of the sensor > > > * > > > + * When multiple factories match the same \a entity, this function selects the > > > + * matching factory with the highest priority as specified to the > > > + * REGISTER_CAMERA_SENSOR() macro at factory registration time. If multiple > > > + * matching factories have the same highest priority value, which factory gets > > > + * selected is undefined and may vary between runs. > > > > That doesn't feel good. Shouldn't we error out if multiple factories > > have the same priority? As far as I understand the system all > > factories are registered inside libcamera, so all priorities are under > > our control. Maybe a bit academical but who knows how many factories > > the future brings :-) > > We could, but we would then have to try and match all the factories of > the same priority. As I expect all factories but the legacy to have > priority 0, it means libcamera will try to match all the non-legacy > factories, slowing down the initialization process. > Fair enough. Thanks for the explanation :-) Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> Cheers, Stefan > > > + * > > > * \return A unique pointer to a new instance of the CameraSensor subclass > > > * matching the entity, or a null pointer if no such factory exists > > > */ > > > @@ -387,8 +395,17 @@ std::unique_ptr<CameraSensor> CameraSensorFactoryBase::create(MediaEntity *entit > > > return nullptr; > > > } > > > > > > +/** > > > + * \fn CameraSensorFactoryBase::priority() > > > + * \brief Retrieve the priority value for the factory > > > + * \return The priority value for the factory > > > + */ > > > + > > > /** > > > * \brief Retrieve the list of all camera sensor factories > > > + * > > > + * The factories are sorted in decreasing priority order. > > > + * > > > * \return The list of camera sensor factories > > > */ > > > std::vector<CameraSensorFactoryBase *> &CameraSensorFactoryBase::factories() > > > @@ -411,7 +428,12 @@ void CameraSensorFactoryBase::registerFactory(CameraSensorFactoryBase *factory) > > > std::vector<CameraSensorFactoryBase *> &factories = > > > CameraSensorFactoryBase::factories(); > > > > > > - factories.push_back(factory); > > > + auto pos = std::upper_bound(factories.begin(), factories.end(), factory, > > > + [](const CameraSensorFactoryBase *value, > > > + const CameraSensorFactoryBase *elem) { > > > + return value->priority() > elem->priority(); > > > + }); > > > + factories.insert(pos, factory); > > > } > > > > > > /** > > > @@ -437,9 +459,10 @@ void CameraSensorFactoryBase::registerFactory(CameraSensorFactoryBase *factory) > > > */ > > > > > > /** > > > - * \def REGISTER_CAMERA_SENSOR(sensor) > > > + * \def REGISTER_CAMERA_SENSOR(sensor, priority) > > > * \brief Register a camera sensor type to the sensor factory > > > * \param[in] sensor Class name of the CameraSensor derived class to register > > > + * \param[in] priority Priority order for factory selection > > > * > > > * Register a CameraSensor subclass with the factory and make it available to > > > * try and match sensors. The subclass needs to implement a static match > > > @@ -457,6 +480,11 @@ void CameraSensorFactoryBase::registerFactory(CameraSensorFactoryBase *factory) > > > * creation succeeded ; > > > * - A non-zero error code if the entity matched and the creation failed ; or > > > * - A zero error code if the entity didn't match. > > > + * > > > + * When multiple factories can support the same MediaEntity (as in the match() > > > + * function of multiple factories returning true for the same entity), the \a > > > + * priority argument selects which factory will be used. See > > > + * CameraSensorFactoryBase::create() for more information. > > > */ > > > > > > } /* namespace libcamera */ > > > diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp > > > index 34677339241c..f9f889a125d0 100644 > > > --- a/src/libcamera/sensor/camera_sensor_legacy.cpp > > > +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp > > > @@ -1010,6 +1010,6 @@ std::string CameraSensorLegacy::logPrefix() const > > > return "'" + entity_->name() + "'"; > > > } > > > > > > -REGISTER_CAMERA_SENSOR(CameraSensorLegacy) > > > +REGISTER_CAMERA_SENSOR(CameraSensorLegacy, -100) > > > > > > } /* namespace libcamera */ > > -- > Regards, > > Laurent Pinchart
diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h index d37e66773195..59785ff62019 100644 --- a/include/libcamera/internal/camera_sensor.h +++ b/include/libcamera/internal/camera_sensor.h @@ -79,11 +79,13 @@ public: class CameraSensorFactoryBase { public: - CameraSensorFactoryBase(); + CameraSensorFactoryBase(int priority); virtual ~CameraSensorFactoryBase() = default; static std::unique_ptr<CameraSensor> create(MediaEntity *entity); + int priority() const { return priority_; } + private: LIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorFactoryBase) @@ -93,14 +95,16 @@ private: virtual std::variant<std::unique_ptr<CameraSensor>, int> match(MediaEntity *entity) const = 0; + + int priority_; }; template<typename _CameraSensor> class CameraSensorFactory final : public CameraSensorFactoryBase { public: - CameraSensorFactory() - : CameraSensorFactoryBase() + CameraSensorFactory(int priority = 0) + : CameraSensorFactoryBase(priority) { } @@ -112,7 +116,11 @@ private: } }; -#define REGISTER_CAMERA_SENSOR(sensor) \ - static CameraSensorFactory<sensor> global_##sensor##Factory{}; +#ifndef __DOXYGEN__ +#define REGISTER_CAMERA_SENSOR(sensor, ...) \ + static CameraSensorFactory<sensor> global_##sensor##Factory{ __VA_ARGS__ }; +#else +#define REGISTER_CAMERA_SENSOR(sensor, priority) +#endif } /* namespace libcamera */ diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp index 7abbe2c76596..e7b526c2f050 100644 --- a/src/libcamera/sensor/camera_sensor.cpp +++ b/src/libcamera/sensor/camera_sensor.cpp @@ -348,11 +348,13 @@ CameraSensor::~CameraSensor() = default; /** * \brief Construct a camera sensor factory base + * \param[in] priority Priority order for factory selection * * Creating an instance of the factory base registers it with the global list of * factories, accessible through the factories() function. */ -CameraSensorFactoryBase::CameraSensorFactoryBase() +CameraSensorFactoryBase::CameraSensorFactoryBase(int priority) + : priority_(priority) { registerFactory(this); } @@ -361,6 +363,12 @@ CameraSensorFactoryBase::CameraSensorFactoryBase() * \brief Create an instance of the CameraSensor corresponding to a media entity * \param[in] entity The media entity on the source end of the sensor * + * When multiple factories match the same \a entity, this function selects the + * matching factory with the highest priority as specified to the + * REGISTER_CAMERA_SENSOR() macro at factory registration time. If multiple + * matching factories have the same highest priority value, which factory gets + * selected is undefined and may vary between runs. + * * \return A unique pointer to a new instance of the CameraSensor subclass * matching the entity, or a null pointer if no such factory exists */ @@ -387,8 +395,17 @@ std::unique_ptr<CameraSensor> CameraSensorFactoryBase::create(MediaEntity *entit return nullptr; } +/** + * \fn CameraSensorFactoryBase::priority() + * \brief Retrieve the priority value for the factory + * \return The priority value for the factory + */ + /** * \brief Retrieve the list of all camera sensor factories + * + * The factories are sorted in decreasing priority order. + * * \return The list of camera sensor factories */ std::vector<CameraSensorFactoryBase *> &CameraSensorFactoryBase::factories() @@ -411,7 +428,12 @@ void CameraSensorFactoryBase::registerFactory(CameraSensorFactoryBase *factory) std::vector<CameraSensorFactoryBase *> &factories = CameraSensorFactoryBase::factories(); - factories.push_back(factory); + auto pos = std::upper_bound(factories.begin(), factories.end(), factory, + [](const CameraSensorFactoryBase *value, + const CameraSensorFactoryBase *elem) { + return value->priority() > elem->priority(); + }); + factories.insert(pos, factory); } /** @@ -437,9 +459,10 @@ void CameraSensorFactoryBase::registerFactory(CameraSensorFactoryBase *factory) */ /** - * \def REGISTER_CAMERA_SENSOR(sensor) + * \def REGISTER_CAMERA_SENSOR(sensor, priority) * \brief Register a camera sensor type to the sensor factory * \param[in] sensor Class name of the CameraSensor derived class to register + * \param[in] priority Priority order for factory selection * * Register a CameraSensor subclass with the factory and make it available to * try and match sensors. The subclass needs to implement a static match @@ -457,6 +480,11 @@ void CameraSensorFactoryBase::registerFactory(CameraSensorFactoryBase *factory) * creation succeeded ; * - A non-zero error code if the entity matched and the creation failed ; or * - A zero error code if the entity didn't match. + * + * When multiple factories can support the same MediaEntity (as in the match() + * function of multiple factories returning true for the same entity), the \a + * priority argument selects which factory will be used. See + * CameraSensorFactoryBase::create() for more information. */ } /* namespace libcamera */ diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp index 34677339241c..f9f889a125d0 100644 --- a/src/libcamera/sensor/camera_sensor_legacy.cpp +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp @@ -1010,6 +1010,6 @@ std::string CameraSensorLegacy::logPrefix() const return "'" + entity_->name() + "'"; } -REGISTER_CAMERA_SENSOR(CameraSensorLegacy) +REGISTER_CAMERA_SENSOR(CameraSensorLegacy, -100) } /* namespace libcamera */
In order to support a default implementation for camera sensors when no better implementation matches, libcamera needs to try "specialized" implementations first and pick the default last. Make this possible by adding a priority value for factories. Newly registered factories are inserted in the factories list sorted by descending priority, and the default factory uses a negative priority to be inserted as the last element. This mechanism may be a bit overkill in the sense that there is no expected use cases for priorities other than trying the default last, but the implementation is simple and easy to understand. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- include/libcamera/internal/camera_sensor.h | 18 +++++++--- src/libcamera/sensor/camera_sensor.cpp | 34 +++++++++++++++++-- src/libcamera/sensor/camera_sensor_legacy.cpp | 2 +- 3 files changed, 45 insertions(+), 9 deletions(-)