[libcamera-devel] [RFC 1/7] libcamera: camera_sensor: Introduce CameraSensorFactory

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work.

On 2019-12-18 15:49:55 +0100, Jacopo Mondi wrote:
> Introduce a factory to create CameraSensor derived classes instances by
> inspecting the sensor media entity name.
> 
> For now, unconditionally create and return an instance of the only
> existing CameraSensor class in CameraSensorFactory::create()
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/camera_sensor.cpp               | 22 +++++++++++++++++++
>  src/libcamera/include/camera_sensor.h         | 10 ++++++++-
>  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, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index bc8b9e78bc42..ac8878fe336e 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -28,6 +28,28 @@ namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(CameraSensor);
>  
> +/**
> + * \class CameraSensorFactory
> + * \brief Factory of camera sensor
> + *
> + * The CameraSensorFactory instantiate and return the opportune CameraSensor
> + * sub-class by inspecting the name of the sensor entity to handle.
> + */
> +
> +/**
> + * \brief Create and return a CameraSensor
> + * \param[in] entity The media entity that represents the sensor to handle
> + *
> + * Instantiate and return the opportune CameraSensor subclass inspecting the
> + * \a entity name.
> + *
> + * \return A pointer to a CameraSensor derived class instance
> + */
> +CameraSensor *CameraSensorFactory::create(const MediaEntity *entity)
> +{
> +	return new CameraSensor(entity);
> +}
> +
>  /**
>   * \class CameraSensor
>   * \brief A camera sensor based on V4L2 subdevices
> diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h
> index 99cff98128dc..2c09b1bdb61b 100644
> --- a/src/libcamera/include/camera_sensor.h
> +++ b/src/libcamera/include/camera_sensor.h
> @@ -22,10 +22,16 @@ class V4L2Subdevice;
>  
>  struct V4L2SubdeviceFormat;
>  
> +class CameraSensor;
> +class CameraSensorFactory final
> +{
> +public:
> +	static CameraSensor *create(const MediaEntity *entity);
> +};

I Wonder if it would not make more sens to move this this to 
CameraSensor::create(). Looking at our code this seems to be the pattern 
used in most places.

> +
>  class CameraSensor : protected Loggable
>  {
>  public:
> -	explicit CameraSensor(const MediaEntity *entity);
>  	~CameraSensor();
>  
>  	CameraSensor(const CameraSensor &) = delete;
> @@ -49,6 +55,8 @@ public:
>  	const ControlList &properties() const { return properties_; }
>  
>  protected:
> +	friend class CameraSensorFactory;
> +	explicit CameraSensor(const MediaEntity *entity);

Do we need to keep explicit here?

>  	std::string logPrefix() const;
>  
>  private:
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 536a63a30018..6de2f548fe1c 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -1362,7 +1362,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::create(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 e9a70755f4c5..276a12755f59 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -883,7 +883,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>  
>  	data->controlInfo_ = std::move(ctrls);
>  
> -	data->sensor_ = new CameraSensor(sensor);
> +	data->sensor_ = CameraSensorFactory::create(sensor);
>  	ret = data->sensor_->init();
>  	if (ret)
>  		return ret;
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index 2bfab35314c4..5d6baa56ba8b 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::create(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..0e7e3f5585d2 100644
> --- a/test/camera-sensor.cpp
> +++ b/test/camera-sensor.cpp
> @@ -50,7 +50,7 @@ protected:
>  			return TestFail;
>  		}
>  
> -		sensor_ = new CameraSensor(entity);
> +		sensor_ = CameraSensorFactory::create(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..3c4ca60b9076 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::create(media_->getEntityByName("Sensor A"));
>  		if (sensor_->init())
>  			return TestFail;
>  
> -- 
> 2.24.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund