[PATCH/RFC 21/32] libcamera: camera_sensor: Sort factories by priority
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Mar 13 19:58:43 CET 2024
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 at 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 */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list