[libcamera-devel] [PATCH v2 3/7] libcamera: camera_sensor: Introduce CameraSensorFactory

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Feb 06, 2020 at 07:52:43PM +0100, Jacopo Mondi wrote:
> Introduce a factory to create CameraSensor derived classes instances by
> inspecting the sensor media entity name and provide a convenience macro
> to register specialized sensor handlers.

I really like the factory :-)

> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/camera_sensor.cpp               | 134 ++++++++++++++++++
>  src/libcamera/include/camera_sensor.h         |  39 ++++-
>  src/libcamera/pipeline/ipu3/ipu3.cpp          |   2 +-
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |   2 +-
>  src/libcamera/pipeline/vimc.cpp               |   2 +-
>  test/camera-sensor.cpp                        |   2 +-
>  .../v4l2_videodevice_test.cpp                 |   2 +-
>  7 files changed, 177 insertions(+), 6 deletions(-)
> 
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 2219a4307436..fc8452b607a0 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -11,7 +11,9 @@
>  #include <float.h>
>  #include <iomanip>
>  #include <limits.h>
> +#include <map>

No need to include map here, no function in this file make use of it
directly. The header is included in camera_sensor.h for the usage of
std::map there, that's enough.

>  #include <math.h>
> +#include <string.h>
>  
>  #include <libcamera/property_ids.h>
>  
> @@ -42,6 +44,18 @@ LOG_DEFINE_CATEGORY(CameraSensor);
>   * devices as the needs arise.
>   */
>  
> +/**
> + * \fn CameraSensor::entityName()

I wonder, wouldn't it be simpler to pass the entity name string
directly to the REGISTER_CAMERA_SENSOR macro ? It would avoid declaring
and undefined static method in the base class.

If we ever end up needing more complex matching code, a static match
function that takes a media entity pointer would make sense, but for a
fully static name, this seems a bit overkill.

> + * \brief Retrieve the name of the entity which identifies the supported sensor
> + *
> + * Camera sensor handlers have to report with this function the name of the
> + * media entity which represents the camera sensor they support. The here
> + * reported name is then matched against the name of the MediaEntity provided
> + * at createSensor() time to identify the correct CameraSensorFactory.
> + *
> + * \return The name of the media entity that identifies the sensor
> + */
> +
>  /**
>   * \brief Construct a CameraSensor
>   * \param[in] entity The media entity backing the camera sensor
> @@ -366,4 +380,124 @@ std::string CameraSensor::logPrefix() const
>  	return "'" + subdev_->entity()->name() + "'";
>  }
>  
> +/**
> + * \class CameraSensorFactory
> + * \brief Factory of camera sensor handlers

"camera sensor handlers" or "camera sensors" ? We have pipeline
handlers, and if we could avoid using the name handler here, it could
help reducing confusion. This implies s/sensor handler/sensor below.

> + *
> + * The class provides to camera sensor handlers the ability to register
> + * themselves to the factory to allow the creation of their instances by
> + * matching the name of the media entity which represents the sensor.
> + *
> + * Camera sensor handlers use the REGISTER_CAMERA_SENSOR() macro to
> + * add themselves to the camera sensor factory. Users of the factory
> + * creates camera sensor handler instances by using the static
> + * CameraSensorFactory::createInstance() method, which returns a pointer
> + * to the opportune CameraSensor sub-class if any, or a newly created instance
> + * of the generic CameraSensor class.
> + */
> +
> +LOG_DEFINE_CATEGORY(CameraSensorFactory);

I think you can use the CameraSensor log category, I don't really see a
need to split messages in two categories here.

> +
> +/**
> + * \typedef CameraSensorFactory::factoriesMap

factoriesMap is a type, it should thus be called FactoriesMap.

> + * \brief An std::map of sensor handler factories

"A map of entity names to camera sensor factories"

> + *
> + * The registered sensor handler factories are associated in a map with the
> + * names of the media entity that represent the sensor

 * This map associates the name of the media entities that represent
 * camera sensors to the corresponding camera sensor factory.

But I think we can just drop the documentation as the type can be made
private.

> + */
> +
> +/**
> + * \brief Register a camera sensor handler factory

This is the constructor, so

 * \brief Construct a camera sensor factory

> + * \param[in] name The name of the media entity representing the sensor
> + *
> + * Register a new camera sensor factory which creates sensor handler instances
> + * that support camera sensors represented by the media entity with \a name.

How about being consistent with the pipeline handler factory
documentation ?

 * Creating an instance of the factory registers is with the global list of
 * factories, accessible through the factories() function.

> + */
> +CameraSensorFactory::CameraSensorFactory(const char *name)
> +{
> +	CameraSensorFactory::registerSensorFactory(name, this);

No need for the CameraSensorFactory:: prefix.

> +}
> +
> +/**
> + * \brief Retrieve the list of registered camera sensor factories
> + *
> + * The static factories map is defined inside the function to ensures it gets
> + * initialized on first use, without any dependency on link order.
> + *
> + * \return The static list of registered camera sensor factories
> + */
> +CameraSensorFactory::factoriesMap &CameraSensorFactory::factories()
> +{
> +	static factoriesMap factories;
> +	return factories;
> +}
> +
> +/**
> + * \brief Register a camera sensor handler factory
> + * \param[in] name The name of the media entity that represents the sensor
> + * \param[in] factory The factory instance to register
> + *
> + * The newly registered sensor handler \a factory is associated with \a name,
> + * when a new sensor handler is instantiated with createSensor() the name of
> + * the media entity is matched against the \a name registered here to
> + * retrieve the correct factory.

Should we explain the uniqueness requirement for the name parameter, as
done in the pipeline handler factory documentation ?

 * The camera sensor \a factory is registered associated with an entity \a name.
 * When a camera sensor is created for a media entity with createSensor(), the
 * name of the media entity is used to select the corresponding factory.
 *
 * The caller is responsible for guaranteeing the uniqueness of the
 * camera sensor entity name.

> + */
> +void CameraSensorFactory::registerSensorFactory(const char *name,
> +						CameraSensorFactory *factory)

Maybe registerFactory() to shorten the name a bit ?

> +{
> +	factoriesMap &map = CameraSensorFactory::factories();

No need for the prefix here either.

Should we log an error if the name is already present in the map ?

> +	map[name] = factory;
> +}
> +
> +/**
> + * \brief Create and return a CameraSensor

Maybe "Create a camera sensor corresponding to an entity" ?

> + * \param[in] entity The media entity that represents the camera sensor
> + *
> + * Create a new instance of an handler for the sensor represented by \a
> + * entity using one of the registered factories. If no specific handler is
> + * available for the sensor, or creating the handler fails, a newly created

s/, or creating the handler fails//

as that's not what's implemented :-)

> + * instance of the generic CameraSensor base class is returned.

"... fails, an instance of the generic CameraSensor class is created and
returned."

> + *
> + * Ownership of the created camera sensor instance is passed to the caller
> + * which is reponsible for deleting the instance.
> + * FIXME: is it worth using a smart pointer here ?

I think it is. Could you give it a try ?

> + *
> + * \return A newly created camera sensor handler instance
> + */
> +CameraSensor *CameraSensorFactory::createSensor(const MediaEntity *entity)
> +{
> +	/*
> +	 * The entity name contains both the sensor name and the I2C bus ID.
> +	 *
> +	 * Remove the i2c bus part and use the sensor name only as key to

s/i2c/I2C/

> +	 * search for on the sensor handlers map.

s/on/in/ ?

> +	 */
> +	const char *entityName = entity->name().c_str();
> +	const char *delim = strchrnul(entityName, ' ');
> +	std::string sensorName(entityName, delim);

Should you split after the last space, not the first space ? I think the
following would avoid including string.h.

	std::string::size_type delim = entity->name().find_last_of(' ');
	std::string name = entity->name().substr(0, delim);

> +
> +	auto it = CameraSensorFactory::factories().find(sensorName);
> +	if (it == CameraSensorFactory::factories().end()) {
> +		LOG(CameraSensorFactory, Info)
> +			<< "Unsupported sensor '" << entity->name()
> +			<< "': using generic sensor handler";
> +		return new CameraSensor(entity);
> +	}
> +
> +	LOG(CameraSensorFactory, Info) << "Create handler for '"
> +				       << entity->name() << "' sensor";
> +
> +	CameraSensorFactory *factory = it->second;
> +	return factory->create(entity);
> +}
> +
> +/**
> + * \def REGISTER_CAMERA_SENSOR(handler)
> + * \brief Register a camera sensor handler to the sensor factory

s/to the/with the/

> + * \param[in] handler The name of the sensor handler

s/sensor handler/camera sensor class/

> + *
> + * Register a camera sensor handler to the sensor factory to make it available

s/to the/with the/

> + * to the factory users.
> + */
> +
>  } /* namespace libcamera */
> diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h
> index 99cff98128dc..2f4a0cc8ad3f 100644
> --- a/src/libcamera/include/camera_sensor.h
> +++ b/src/libcamera/include/camera_sensor.h
> @@ -7,6 +7,7 @@
>  #ifndef __LIBCAMERA_CAMERA_SENSOR_H__
>  #define __LIBCAMERA_CAMERA_SENSOR_H__
>  
> +#include <map>
>  #include <string>
>  #include <vector>
>  
> @@ -25,7 +26,8 @@ struct V4L2SubdeviceFormat;
>  class CameraSensor : protected Loggable
>  {
>  public:
> -	explicit CameraSensor(const MediaEntity *entity);
> +	static const char *entityName();
> +
>  	~CameraSensor();
>  
>  	CameraSensor(const CameraSensor &) = delete;
> @@ -49,6 +51,8 @@ public:
>  	const ControlList &properties() const { return properties_; }
>  
>  protected:
> +	friend class CameraSensorFactory;
> +	explicit CameraSensor(const MediaEntity *entity);

You can make the constructor private.

>  	std::string logPrefix() const;
>  
>  private:
> @@ -61,6 +65,39 @@ private:
>  	ControlList properties_;
>  };
>  
> +class CameraSensorFactory
> +{
> +public:
> +	using factoriesMap = std::map<const std::string, CameraSensorFactory *>;

You can make this type private.

> +
> +	CameraSensorFactory(const char *name);
> +	virtual ~CameraSensorFactory() {}
> +
> +	static CameraSensor *createSensor(const MediaEntity *entity = nullptr);

Why the default = nullptr ? The argument should be mandatory.

> +
> +private:
> +	static factoriesMap &factories();
> +	static void registerSensorFactory(const char *name,
> +					  CameraSensorFactory *factory);
> +	virtual CameraSensor *create(const MediaEntity *entity) = 0;
> +
> +};
> +
> +#define REGISTER_CAMERA_SENSOR(handler)					\

s/handler/sensor/

> +class handler##CameraSensorFactory final : public CameraSensorFactory	\

To shorten lines,

class sensor##Factory ...

> +{									\
> +public:									\
> +	handler##CameraSensorFactory()					\
> +		: CameraSensorFactory(handler##CameraSensor::entityName()) {}\

Let's not add ##CameraSensor, it makes the code more confusing:

class FooCameraSensor : CameraSensor
{
	...
};

REGISTER_CAMERA_SENSOR(Foo);

I'd rather write

class FooCameraSensor : CameraSensor
{
	...
};

REGISTER_CAMERA_SENSOR(FooCameraSensor);

and make it possible for the author to name the class without a
CameraSensor suffix.

> +									\
> +private:								\
> +	CameraSensor *create(const MediaEntity *entity)			\
> +	{								\
> +		return new handler##CameraSensor(entity);		\
> +	}								\
> +};									\
> +static handler##CameraSensorFactory global_##handler##CameraSensorFactory
> +
>  } /* namespace libcamera */
>  
>  #endif /* __LIBCAMERA_CAMERA_SENSOR_H__ */
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 387bb070b505..21934e72eba7 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -1322,7 +1322,7 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)
>  
>  	MediaLink *link = links[0];
>  	MediaEntity *sensorEntity = link->source()->entity();
> -	sensor_ = new CameraSensor(sensorEntity);
> +	sensor_ = CameraSensorFactory::createSensor(sensorEntity);
>  	ret = sensor_->init();
>  	if (ret)
>  		return ret;
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 68f16f03a81e..f2f054596257 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -901,7 +901,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>  
>  	data->controlInfo_ = std::move(ctrls);
>  
> -	data->sensor_ = new CameraSensor(sensor);
> +	data->sensor_ = CameraSensorFactory::createSensor(sensor);
>  	ret = data->sensor_->init();
>  	if (ret)
>  		return ret;
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index fd4df0b03c26..cfeec1aac751 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -403,7 +403,7 @@ int VimcCameraData::init(MediaDevice *media)
>  		return ret;
>  
>  	/* Create and open the camera sensor, debayer, scaler and video device. */
> -	sensor_ = new CameraSensor(media->getEntityByName("Sensor B"));
> +	sensor_ = CameraSensorFactory::createSensor(media->getEntityByName("Sensor B"));
>  	ret = sensor_->init();
>  	if (ret)
>  		return ret;
> diff --git a/test/camera-sensor.cpp b/test/camera-sensor.cpp
> index 27c190fe7ace..24b83a45b656 100644
> --- a/test/camera-sensor.cpp
> +++ b/test/camera-sensor.cpp
> @@ -50,7 +50,7 @@ protected:
>  			return TestFail;
>  		}
>  
> -		sensor_ = new CameraSensor(entity);
> +		sensor_ = CameraSensorFactory::createSensor(entity);
>  		if (sensor_->init() < 0) {
>  			cerr << "Unable to initialise camera sensor" << endl;
>  			return TestFail;
> diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.cpp b/test/v4l2_videodevice/v4l2_videodevice_test.cpp
> index 577da4cb601c..102a8b1b6c1c 100644
> --- a/test/v4l2_videodevice/v4l2_videodevice_test.cpp
> +++ b/test/v4l2_videodevice/v4l2_videodevice_test.cpp
> @@ -61,7 +61,7 @@ int V4L2VideoDeviceTest::init()
>  		return TestFail;
>  
>  	if (driver_ == "vimc") {
> -		sensor_ = new CameraSensor(media_->getEntityByName("Sensor A"));
> +		sensor_ = CameraSensorFactory::createSensor(media_->getEntityByName("Sensor A"));
>  		if (sensor_->init())
>  			return TestFail;
>  

-- 
Regards,

Laurent Pinchart