[libcamera-devel] [PATCH v3 3/6] libcamera: camera_sensor: Introduce CameraSensorFactory

[Kieran Bingham]

Hi Jacopo,

On 09/03/2020 18:04, 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 camera sensor sub-classes.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>

Other than documentation fixups:

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> 
> ---
> v2 -> v3:
> - Update documentation
> - Register sensor name as REGISTER_CAMERA_SENSOR() second parameter
> - Create camera sensor as unique_ptr
> - Match entity name and key with std::string::find() in
>   CameraSensorFactory::createSensor()
> ---
>  src/libcamera/camera_sensor.cpp               | 136 ++++++++++++++++++
>  src/libcamera/include/camera_sensor.h         |  32 +++++
>  src/libcamera/pipeline/ipu3/ipu3.cpp          |   9 +-
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  17 +--
>  src/libcamera/pipeline/vimc.cpp               |   6 +-
>  test/camera-sensor.cpp                        |   9 +-
>  .../v4l2_videodevice_test.cpp                 |   3 +-
>  test/v4l2_videodevice/v4l2_videodevice_test.h |   6 +-
>  8 files changed, 187 insertions(+), 31 deletions(-)
> 
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 2219a4307436..8bfe02c9e8ff 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -12,6 +12,8 @@
>  #include <iomanip>
>  #include <limits.h>
>  #include <math.h>
> +#include <memory>
> +#include <string.h>
>  
>  #include <libcamera/property_ids.h>
>  
> @@ -366,4 +368,138 @@ std::string CameraSensor::logPrefix() const
>  	return "'" + subdev_->entity()->name() + "'";
>  }
>  
> +/**
> + * \class CameraSensorFactory
> + * \brief Factory of camera sensors
> + *
> + * The class provides to camera sensors the ability to register themselves to

This class provides camera sensors with the ability 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

/creates/create/

> + * CameraSensorFactory::createInstance() method, which returns a pointer
> + * to the opportune CameraSensor sub-class if any, or a newly created instance

opportune? I'm not sure that's helpful there...


"which returns a pointer to the existing CameraSensor sub-class if it
exists, or a newly created ..."



> + * of the generic CameraSensor class.
> + */
> +
> +/**
> + * \brief Construct a camera sensor factory
> + * \param[in] name The name of the media entity that represents 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.

(Trying to figure out how to say this...., the 'handler' verb seems out
of place to me I'm afraid - and makes me think it's something else...)

Register a new camera sensor factory which creates CameraSensor
instances to support camera sensors represented by the media entity with
\a name.

> + *
> + * Creating an instance of the factory registers it with the global list of
> + * factories, accessible through the factories() function.
> + */
> +CameraSensorFactory::CameraSensorFactory(const char *name)
> +{
> +	registerFactory(name, this);
> +}
> +
> +/**
> + * \brief Retrieve the list of registered camera sensor factories
> + *
> + * The static factories map is defined inside the function to ensures it gets

/ensures/ensure/

> + * initialized on first use, without any dependency on link order.

That's an implementation detail, shouldn't that comment go 'in' the
function rather than in the documentation of the function?

> + *
> + * \return The static list of registered camera sensor factories

Do we need to state that it's a static list? That makes me think that it
can't change ...

> + */
> +CameraSensorFactory::FactoriesMap &CameraSensorFactory::factories()
> +{
> +	static FactoriesMap factories;
> +	return factories;
> +}
> +
> +/**
> + * \brief Register a camera sensor factory
> + * \param[in] name The name of the media entity that represents the sensor
> + * \param[in] factory The factory instance to register
> + *
> + * The camera sensor \a factory is registered and associated with an entity \a
> + * name. When a camera sensor is created for a media entity with the
> + * createSensor() method, the name of the media entity there provided is used
> + * to select the corresponding factory.

/name/\a name/ ? Or does doxygen automatically match...
/there//

> + *
> + * The caller is responsible for guaranteeing the uniqueness of the
> + * camera sensor entity name.
> + */
> +void CameraSensorFactory::registerFactory(const char *name,
> +					  CameraSensorFactory *factory)
> +{
> +	FactoriesMap &map = factories();
> +
> +	if (map.find(name) != map.end()) {
> +		LOG(CameraSensor, Error)
> +			<< "Unable to register camera sensor factory '"
> +			<< name << "': already registered";
> +		return;
> +	}
> +
> +	map[name] = factory;
> +}
> +
> +/**
> + * \brief Create a camera sensor corresponding to \a entity
> + * \param[in] entity The media entity that represents the sensor
> + *
> + * Create a new instance of a CameraSensor subclass for the sensor represented
> + * by \a entity using one of the registered factories. If no specific class is
> + * available for the sensor, an instance of the generic CameraSensor class is
> + * created and returned.
> + *
> + * Ownership of the created camera sensor instance is passed to the caller as
> + * a unique pointer.
> + *
> + * \return A unique pointer to the newly created camera sensor instance
> + */
> +std::unique_ptr<CameraSensor> CameraSensorFactory::createSensor(const MediaEntity *entity)
> +{
> +	const std::string &name = entity->name();
> +	for (const auto &it : factories()) {
> +		const std::string &key = it.first;
> +
> +		/*
> +		 * Match the name of the entity with the keys in the factories
> +		 * map. The factories map keys are provided by the CameraSensor
> +		 * sub-class at factory registration time as the sensor
> +		 * entity name.
> +		 *
> +		 * To support both canonical sensor entity names (in the
> +		 * "devname i2c-adapt:i2c-addr" format) and non-canonical sensor
> +		 * names such (as VIMC's "Sensor B" ones), search for the key
> +		 * in the entity name.
> +		 *
> +		 * \todo Delegate matching to the CameraSensor sub-class
> +		 * to support more complex matching criteria.
> +		 */
> +		if (name.find(key) == std::string::npos)
> +			continue;
> +
> +		LOG(CameraSensor, Info) << "Create camera sensor for '"
> +					<< name << "'";
> +
> +		CameraSensorFactory *factory = it.second;
> +		return std::unique_ptr<CameraSensor>(factory->create(entity));
> +	}
> +
> +	LOG(CameraSensor, Info) << "Unsupported sensor '" << entity->name()
> +				<< "': using generic camera sensor";
> +	return std::make_unique<CameraSensor>(entity);
> +}
> +
> +/**
> + * \def REGISTER_CAMERA_SENSOR(sensor, entityName)
> + * \brief Register camera sensor \a name with the sensor factory

Should that be "\a entityName" ?


> + * \param[in] sensor The name of the camera sensor class to register
> + * \param[in] entityName The name of the image sensor supported by the factory
> + *
> + * Register camera sensor factory \a sensor with the global list of factories.

Register \a sensor as a camera sensor factory in the global list of
factories.


> + * The camera sensor factory supports image sensors identified by \a entityName.
> + * The sensor name reported by the media entity that identifies it is usually
> + * composed by the here provided \a entityName and the I2C bus and device ids.

usually composed of the \a entityName and the I2C bus and device ids.

> + */
> +
>  } /* namespace libcamera */
> diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h
> index 99cff98128dc..633955591b36 100644
> --- a/src/libcamera/include/camera_sensor.h
> +++ b/src/libcamera/include/camera_sensor.h
> @@ -7,6 +7,8 @@
>  #ifndef __LIBCAMERA_CAMERA_SENSOR_H__
>  #define __LIBCAMERA_CAMERA_SENSOR_H__
>  
> +#include <map>
> +#include <memory>
>  #include <string>
>  #include <vector>
>  
> @@ -61,6 +63,36 @@ private:
>  	ControlList properties_;
>  };
>  
> +class CameraSensorFactory
> +{
> +public:
> +	CameraSensorFactory(const char *name);
> +	virtual ~CameraSensorFactory() {}
> +
> +	static std::unique_ptr<CameraSensor> createSensor(const MediaEntity *entity);
> +
> +private:
> +	using FactoriesMap = std::map<const std::string, CameraSensorFactory *>;
> +	static FactoriesMap &factories();
> +	static void registerFactory(const char *name,
> +				    CameraSensorFactory *factory);
> +	virtual CameraSensor *create(const MediaEntity *entity) = 0;
> +};
> +
> +#define REGISTER_CAMERA_SENSOR(sensor, entityName)			\
> +class sensor##Factory final : public CameraSensorFactory		\
> +{									\
> +public:									\
> +	sensor##Factory() : CameraSensorFactory(entityName) {}		\
> +									\
> +private:								\
> +	CameraSensor *create(const MediaEntity *entity)			\
> +	{								\
> +		return new sensor(entity);				\
> +	}								\
> +};									\
> +static sensor##Factory global_##sensor##Factory
> +
>  } /* 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..adef6c6703b6 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -100,7 +100,7 @@ public:
>  	static constexpr unsigned int CIO2_BUFFER_COUNT = 4;
>  
>  	CIO2Device()
> -		: output_(nullptr), csi2_(nullptr), sensor_(nullptr)
> +		: output_(nullptr), csi2_(nullptr)
>  	{
>  	}
>  
> @@ -108,7 +108,6 @@ public:
>  	{
>  		delete output_;
>  		delete csi2_;
> -		delete sensor_;
>  	}
>  
>  	int init(const MediaDevice *media, unsigned int index);
> @@ -125,7 +124,7 @@ public:
>  
>  	V4L2VideoDevice *output_;
>  	V4L2Subdevice *csi2_;
> -	CameraSensor *sensor_;
> +	std::unique_ptr<CameraSensor> sensor_;
>  
>  private:
>  	std::vector<std::unique_ptr<FrameBuffer>> buffers_;
> @@ -294,7 +293,7 @@ void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)
>  
>  CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  {
> -	const CameraSensor *sensor = data_->cio2_.sensor_;
> +	const CameraSensor *sensor = data_->cio2_.sensor_.get();
>  	Status status = Valid;
>  
>  	if (config_.empty())
> @@ -1322,7 +1321,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 13433b216747..3e3b22018430 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -114,20 +114,15 @@ class RkISP1CameraData : public CameraData
>  {
>  public:
>  	RkISP1CameraData(PipelineHandler *pipe)
> -		: CameraData(pipe), sensor_(nullptr), frame_(0),
> +		: CameraData(pipe), frame_(0),
>  		  frameInfo_(pipe)
>  	{
>  	}
>  
> -	~RkISP1CameraData()
> -	{
> -		delete sensor_;
> -	}
> -
>  	int loadIPA();
>  
>  	Stream stream_;
> -	CameraSensor *sensor_;
> +	std::unique_ptr<CameraSensor> sensor_;
>  	unsigned int frame_;
>  	std::vector<IPABuffer> ipaBuffers_;
>  	RkISP1Frames frameInfo_;
> @@ -389,7 +384,7 @@ void RkISP1CameraData::queueFrameAction(unsigned int frame,
>  	case RKISP1_IPA_ACTION_V4L2_SET: {
>  		const ControlList &controls = action.controls[0];
>  		timeline_.scheduleAction(std::make_unique<RkISP1ActionSetSensor>(frame,
> -										 sensor_,
> +										 sensor_.get(),
>  										 controls));
>  		break;
>  	}
> @@ -444,7 +439,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  		/* \todo Add support for 8-bit greyscale to DRM formats */
>  	};
>  
> -	const CameraSensor *sensor = data_->sensor_;
> +	const CameraSensor *sensor = data_->sensor_.get();
>  	Status status = Valid;
>  
>  	if (config_.empty())
> @@ -557,7 +552,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>  		static_cast<RkISP1CameraConfiguration *>(c);
>  	RkISP1CameraData *data = cameraData(camera);
>  	StreamConfiguration &cfg = config->at(0);
> -	CameraSensor *sensor = data->sensor_;
> +	CameraSensor *sensor = data->sensor_.get();
>  	int ret;
>  
>  	/*
> @@ -907,7 +902,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 529909714bf5..6f725c6dda98 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -8,6 +8,7 @@
>  #include <algorithm>
>  #include <array>
>  #include <iomanip>
> +#include <memory>
>  #include <tuple>
>  
>  #include <linux/drm_fourcc.h>
> @@ -47,7 +48,6 @@ public:
>  
>  	~VimcCameraData()
>  	{
> -		delete sensor_;
>  		delete debayer_;
>  		delete scaler_;
>  		delete video_;
> @@ -57,7 +57,7 @@ public:
>  	int init(MediaDevice *media);
>  	void bufferReady(FrameBuffer *buffer);
>  
> -	CameraSensor *sensor_;
> +	std::unique_ptr<CameraSensor> sensor_;
>  	V4L2Subdevice *debayer_;
>  	V4L2Subdevice *scaler_;
>  	V4L2VideoDevice *video_;
> @@ -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..8709e481dc7b 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;
> @@ -100,15 +100,10 @@ protected:
>  		return TestPass;
>  	}
>  
> -	void cleanup()
> -	{
> -		delete sensor_;
> -	}
> -
>  private:
>  	std::unique_ptr<DeviceEnumerator> enumerator_;
>  	std::shared_ptr<MediaDevice> media_;
> -	CameraSensor *sensor_;
> +	std::unique_ptr<CameraSensor> sensor_;
>  };
>  
>  TEST_REGISTER(CameraSensorTest)
> diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.cpp b/test/v4l2_videodevice/v4l2_videodevice_test.cpp
> index 577da4cb601c..ae038de74864 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;
>  
> @@ -97,6 +97,5 @@ void V4L2VideoDeviceTest::cleanup()
>  	capture_->close();
>  
>  	delete debayer_;
> -	delete sensor_;
>  	delete capture_;
>  }
> diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.h b/test/v4l2_videodevice/v4l2_videodevice_test.h
> index 9acaceb84fe0..f635e0929efe 100644
> --- a/test/v4l2_videodevice/v4l2_videodevice_test.h
> +++ b/test/v4l2_videodevice/v4l2_videodevice_test.h
> @@ -25,8 +25,8 @@ class V4L2VideoDeviceTest : public Test
>  {
>  public:
>  	V4L2VideoDeviceTest(const char *driver, const char *entity)
> -		: driver_(driver), entity_(entity), sensor_(nullptr),
> -		  debayer_(nullptr), capture_(nullptr)
> +		: driver_(driver), entity_(entity), debayer_(nullptr),
> +		  capture_(nullptr)
>  	{
>  	}
>  
> @@ -38,7 +38,7 @@ protected:
>  	std::string entity_;
>  	std::unique_ptr<DeviceEnumerator> enumerator_;
>  	std::shared_ptr<MediaDevice> media_;
> -	CameraSensor *sensor_;
> +	std::unique_ptr<CameraSensor> sensor_;
>  	V4L2Subdevice *debayer_;
>  	V4L2VideoDevice *capture_;
>  	std::vector<std::unique_ptr<FrameBuffer>> buffers_;
> 

-- 
Regards
--
Kieran