[PATCH/RFC 21/32] libcamera: camera_sensor: Sort factories by priority

[Stefan Klug]

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 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.

> +		: 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
>