[1/3] libcamera: camera_sensor: Introduce CameraSensorFactory
diff mbox series

Message ID 20241022145321.25923-2-jacopo.mondi@ideasonboard.com
State Accepted
Headers show
Series
  • Introduce CameraSensorFactory
Related show

Commit Message

Jacopo Mondi Oct. 22, 2024, 2:53 p.m. UTC 
From: Jacopo Mondi <jacopo@jmondi.org>

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.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 include/libcamera/internal/camera_sensor.h    |  48 +++++-
 src/libcamera/pipeline/imx8-isi/imx8-isi.cpp  |   9 +-
 src/libcamera/pipeline/ipu3/cio2.cpp          |   7 +-
 src/libcamera/pipeline/mali-c55/mali-c55.cpp  |   5 +-
 src/libcamera/pipeline/rkisp1/rkisp1.cpp      |   7 +-
 .../pipeline/rpi/common/pipeline_base.cpp     |   5 +-
 src/libcamera/pipeline/simple/simple.cpp      |   9 +-
 src/libcamera/pipeline/vimc/vimc.cpp          |   7 +-
 src/libcamera/sensor/camera_sensor.cpp        | 162 ++++++++++++++++++
 test/camera-sensor.cpp                        |   7 +-
 .../v4l2_videodevice_test.cpp                 |   5 +-
 test/v4l2_videodevice/v4l2_videodevice_test.h |   2 +-
 12 files changed, 233 insertions(+), 40 deletions(-)

Comments

Kieran Bingham Nov. 5, 2024, 9:58 a.m. UTC | #1 
Quoting Jacopo Mondi (2024-10-22 15:53:13)
> From: Jacopo Mondi <jacopo@jmondi.org>
> 
> 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.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>  include/libcamera/internal/camera_sensor.h    |  48 +++++-
>  src/libcamera/pipeline/imx8-isi/imx8-isi.cpp  |   9 +-
>  src/libcamera/pipeline/ipu3/cio2.cpp          |   7 +-
>  src/libcamera/pipeline/mali-c55/mali-c55.cpp  |   5 +-
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |   7 +-
>  .../pipeline/rpi/common/pipeline_base.cpp     |   5 +-
>  src/libcamera/pipeline/simple/simple.cpp      |   9 +-
>  src/libcamera/pipeline/vimc/vimc.cpp          |   7 +-
>  src/libcamera/sensor/camera_sensor.cpp        | 162 ++++++++++++++++++
>  test/camera-sensor.cpp                        |   7 +-
>  .../v4l2_videodevice_test.cpp                 |   5 +-
>  test/v4l2_videodevice/v4l2_videodevice_test.h |   2 +-
>  12 files changed, 233 insertions(+), 40 deletions(-)
> 
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index a42c15fa37ce..2d6cb697a870 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -38,7 +38,6 @@ enum class Orientation;
>  class CameraSensor : protected Loggable
>  {
>  public:
> -       explicit CameraSensor(const MediaEntity *entity);
>         ~CameraSensor();
>  
>         int init();
> @@ -81,6 +80,7 @@ public:
>         int setTestPatternMode(controls::draft::TestPatternModeEnum mode);
>  
>  protected:
> +       explicit CameraSensor(const MediaEntity *entity);
>         std::string logPrefix() const override;
>  
>  private:
> @@ -122,4 +122,50 @@ private:
>         std::unique_ptr<CameraLens> focusLens_;
>  };
>  
> +class CameraSensorFactoryBase

Seems I don't have much to comment on here so I might be down to
bikeshedding!

Having read through the usages of the CameraSensorFactoryBase, I would
be tempted to rename this class to:

	CameraSensorFactory


> +{
> +public:
> +       CameraSensorFactoryBase();
> +       virtual ~CameraSensorFactoryBase() = default;
> +
> +       static std::unique_ptr<CameraSensor> create(MediaEntity *entity);
> +
> +private:
> +       LIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorFactoryBase)
> +
> +       static std::vector<CameraSensorFactoryBase *> &factories();
> +
> +       static void registerFactory(CameraSensorFactoryBase *factory);
> +
> +       virtual bool match(const MediaEntity *entity) const = 0;
> +
> +       virtual std::unique_ptr<CameraSensor>
> +       createInstance(MediaEntity *entity) const = 0;
> +};
> +
> +template<typename _CameraSensor>
> +class CameraSensorFactory final : public CameraSensorFactoryBase

And this one to CameraSensorFactoryType (or other name as appropriate)

> +{
> +public:
> +       CameraSensorFactory()
> +               : CameraSensorFactoryBase()
> +       {
> +       }
> +
> +private:
> +       bool match(const MediaEntity *entity) const override
> +       {
> +               return _CameraSensor::match(entity);
> +       }
> +
> +       std::unique_ptr<CameraSensor>
> +       createInstance(MediaEntity *entity) const override
> +       {
> +               return _CameraSensor::create(entity);
> +       }
> +};
> +
> +#define REGISTER_CAMERA_SENSOR(sensor) \
> +static CameraSensorFactory<sensor> global_##sensor##Factory{};

This I think would be the only rename/usage of the simple name
CameraSensorFactory currently so the only place to be renamed to
CameraSensorFactoryType<sensor> ?

> +
>  } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> index 72aa6c75608a..4e66b3368d5a 100644
> --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> @@ -157,11 +157,10 @@ PipelineHandlerISI *ISICameraData::pipe()
>  /* Open and initialize pipe components. */
>  int ISICameraData::init()
>  {
> -       int ret = sensor_->init();
> -       if (ret)
> -               return ret;
> +       if (!sensor_)
> +               return -ENODEV;
>  
> -       ret = csis_->open();
> +       int ret = csis_->open();
>         if (ret)
>                 return ret;
>  
> @@ -1057,7 +1056,7 @@ bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)
>                 std::unique_ptr<ISICameraData> data =
>                         std::make_unique<ISICameraData>(this);
>  
> -               data->sensor_ = std::make_unique<CameraSensor>(sensor);
> +               data->sensor_ = CameraSensorFactoryBase::create(sensor);

So that the users of the Factory create read as :

	data->sensor_ = CameraSensorFactory::create(sensor);

<end of CameraSensorFactory name bikeshed threading>


>                 data->csis_ = std::make_unique<V4L2Subdevice>(csi);
>                 data->xbarSink_ = sink;
>  
> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> index 74a5d93f88ae..aa544d7b0303 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> @@ -134,10 +134,9 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)
>  
>         MediaLink *link = links[0];
>         MediaEntity *sensorEntity = link->source()->entity();
> -       sensor_ = std::make_unique<CameraSensor>(sensorEntity);
> -       ret = sensor_->init();
> -       if (ret)
> -               return ret;
> +       sensor_ = CameraSensorFactoryBase::create(sensorEntity);
> +       if (!sensor_)
> +               return -ENODEV;

I like that the construction is now simplified and there's no need to
create and then initialise the Sensor class in every usage...

>  
>         ret = link->setEnabled(true);
>         if (ret)
> diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> index 45c71c1dd619..4b71e0dad15e 100644
> --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> @@ -141,9 +141,8 @@ int MaliC55CameraData::init()
>          * Register a CameraSensor if we connect to a sensor and create
>          * an entity for the connected CSI-2 receiver.
>          */
> -       sensor_ = std::make_unique<CameraSensor>(entity_);
> -       ret = sensor_->init();
> -       if (ret)
> +       sensor_ = CameraSensorFactoryBase::create(entity_);
> +       if (!sensor_)
>                 return ret;
>  
>         const MediaPad *sourcePad = entity_->getPadByIndex(0);
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index d8d1d65fbbf9..95c25e976015 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -1122,10 +1122,9 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>                 std::make_unique<RkISP1CameraData>(this, &mainPath_,
>                                                    hasSelfPath_ ? &selfPath_ : nullptr);
>  
> -       data->sensor_ = std::make_unique<CameraSensor>(sensor);
> -       ret = data->sensor_->init();
> -       if (ret)
> -               return ret;
> +       data->sensor_ = CameraSensorFactoryBase::create(sensor);
> +       if (!data->sensor_)
> +               return -ENODEV;
>  
>         /* Initialize the camera properties. */
>         data->properties_ = data->sensor_->properties();
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> index 3041fd1ed9fd..4f56bd33df05 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> @@ -774,13 +774,10 @@ int PipelineHandlerBase::registerCamera(std::unique_ptr<RPi::CameraData> &camera
>         CameraData *data = cameraData.get();
>         int ret;
>  
> -       data->sensor_ = std::make_unique<CameraSensor>(sensorEntity);
> +       data->sensor_ = CameraSensorFactoryBase::create(sensorEntity);
>         if (!data->sensor_)
>                 return -EINVAL;
>  
> -       if (data->sensor_->init())
> -               return -EINVAL;
> -
>         /* Populate the map of sensor supported formats and sizes. */
>         for (auto const mbusCode : data->sensor_->mbusCodes())
>                 data->sensorFormats_.emplace(mbusCode,
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 3ddce71d3757..67f583b8a22c 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -388,8 +388,6 @@ SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,
>                                    MediaEntity *sensor)
>         : Camera::Private(pipe), streams_(numStreams)
>  {
> -       int ret;
> -
>         /*
>          * Find the shortest path from the camera sensor to a video capture
>          * device using the breadth-first search algorithm. This heuristic will
> @@ -480,12 +478,9 @@ SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,
>         }
>  
>         /* Finally also remember the sensor. */
> -       sensor_ = std::make_unique<CameraSensor>(sensor);
> -       ret = sensor_->init();
> -       if (ret) {
> -               sensor_.reset();
> +       sensor_ = CameraSensorFactoryBase::create(sensor);
> +       if (!sensor_)
>                 return;
> -       }
>  
>         LOG(SimplePipeline, Debug)
>                 << "Found pipeline: "
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index 2165bae890cb..2e474133439c 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -532,10 +532,9 @@ int VimcCameraData::init()
>                 return ret;
>  
>         /* Create and open the camera sensor, debayer, scaler and video device. */
> -       sensor_ = std::make_unique<CameraSensor>(media_->getEntityByName("Sensor B"));
> -       ret = sensor_->init();
> -       if (ret)
> -               return ret;
> +       sensor_ = CameraSensorFactoryBase::create(media_->getEntityByName("Sensor B"));
> +       if (!sensor_)
> +               return -ENODEV;
>  
>         debayer_ = V4L2Subdevice::fromEntityName(media_, "Debayer B");
>         if (debayer_->open())
> diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp
> index 1b224f1989fe..025c9eefdd12 100644
> --- a/src/libcamera/sensor/camera_sensor.cpp
> +++ b/src/libcamera/sensor/camera_sensor.cpp
> @@ -11,6 +11,7 @@
>  #include <cmath>
>  #include <float.h>
>  #include <limits.h>
> +#include <map>
>  #include <string.h>
>  
>  #include <libcamera/base/utils.h>
> @@ -1202,4 +1203,165 @@ std::string CameraSensor::logPrefix() const
>         return "'" + entity_->name() + "'";
>  }
>  
> +namespace {
> +
> +/* Transitory default camera sensor implementation */
> +class CameraSensorDefault : public CameraSensor
> +{
> +public:
> +       CameraSensorDefault(MediaEntity *entity)
> +               : CameraSensor(entity)
> +       {
> +       }
> +
> +       static bool match([[maybe_unused]] const MediaEntity *entity)
> +       {

Do we need any kind of debug/warning/log in here to say its' using a
temporary default wrapper? (Only thinking here in the event that we keep
this as a lowest priority factory ... see comment below in
CameraSensorFactoryBase::create())

> +               return true;
> +       }
> +
> +       static std::unique_ptr<CameraSensorDefault> create(MediaEntity *entity)
> +       {
> +               std::unique_ptr<CameraSensorDefault> sensor =
> +                       std::make_unique<CameraSensorDefault>(entity);
> +
> +               if (sensor->init())
> +                       return nullptr;
> +
> +               return sensor;
> +       }
> +};
> +
> +REGISTER_CAMERA_SENSOR(CameraSensorDefault)
> +
> +}; /* namespace */
> +
> +/**
> + * \class CameraSensorFactoryBase
> + * \brief Base class for camera sensor factories
> + *
> + * The CameraSensorFactoryBase class is the base of all specializations of
> + * the CameraSensorFactory class template. It implements the factory
> + * registration, maintains a registry of factories, and provides access to the
> + * registered factories.
> + */
> +
> +/**
> + * \brief Construct a camera sensor factory base
> + *
> + * Creating an instance of the factory base registers it with the global list of
> + * factories, accessible through the factories() function.
> + */
> +CameraSensorFactoryBase::CameraSensorFactoryBase()
> +{
> +       registerFactory(this);
> +}
> +
> +/**
> + * \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
> + *
> + * \return A unique pointer to a new instance of the CameraSensor subclass
> + * matching the entity, or a null pointer if no such factory exists
> + */
> +std::unique_ptr<CameraSensor> CameraSensorFactoryBase::create(MediaEntity *entity)
> +{
> +       const std::vector<CameraSensorFactoryBase *> &factories =
> +               CameraSensorFactoryBase::factories();
> +
> +       for (const CameraSensorFactoryBase *factory : factories) {

Will the factories need to be sorted in any priority order ? Do we
anticipate multiple Factory classes could match ? (I.e. if the
transitory default were to be kept for instance?)

Fine to handle that when required of course...

Bike shedding aside, there's nothing in here that scares me so:


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


> +               if (!factory->match(entity))
> +                       continue;
> +
> +               std::unique_ptr<CameraSensor> sensor = factory->createInstance(entity);
> +               if (!sensor) {
> +                       LOG(CameraSensor, Error)
> +                               << "Failed to create sensor for '"
> +                               << entity->name();
> +                       return nullptr;
> +               }
> +
> +               return sensor;
> +       }
> +
> +       return nullptr;
> +}
> +
> +/**
> + * \brief Retrieve the list of all camera sensor factories
> + * \return The list of camera sensor factories
> + */
> +std::vector<CameraSensorFactoryBase *> &CameraSensorFactoryBase::factories()
> +{
> +       /*
> +        * The static factories map is defined inside the function to ensure
> +        * it gets initialized on first use, without any dependency on link
> +        * order.
> +        */
> +       static std::vector<CameraSensorFactoryBase *> factories;
> +       return factories;
> +}
> +
> +/**
> + * \brief Add a camera sensor class to the registry
> + * \param[in] factory Factory to use to construct the camera sensor
> + */
> +void CameraSensorFactoryBase::registerFactory(CameraSensorFactoryBase *factory)
> +{
> +       std::vector<CameraSensorFactoryBase *> &factories =
> +               CameraSensorFactoryBase::factories();
> +
> +       factories.push_back(factory);
> +}
> +
> +/**
> + * \class CameraSensorFactory
> + * \brief Registration of CameraSensorFactory classes and creation of instances
> + * \tparam _CameraSensor The camera sensor class type for this factory
> + *
> + * To facilitate discovery and instantiation of CameraSensor classes, the
> + * CameraSensorFactory class implements auto-registration of camera sensors.
> + * Each CameraSensor subclass shall register itself using the
> + * REGISTER_CAMERA_SENSOR() macro, which will create a corresponding instance
> + * of a CameraSensorFactory subclass and register it with the static list of
> + * factories.
> + */
> +
> +/**
> + * \fn CameraSensorFactory::CameraSensorFactory()
> + * \brief Construct a camera sensor factory
> + *
> + * Creating an instance of the factory registers it with the global list of
> + * factories, accessible through the CameraSensorFactoryBase::factories()
> + * function.
> + */
> +
> +/**
> + * \fn CameraSensorFactory::createInstance() const
> + * \brief Create an instance of the CameraSensor corresponding to the factory
> + *
> + * \return A unique pointer to a newly constructed instance of the CameraSensor
> + * subclass corresponding to the factory
> + */
> +
> +/**
> + * \def REGISTER_CAMERA_SENSOR(sensor)
> + * \brief Register a camera sensor type to the sensor factory
> + * \param[in] sensor Class name of the CameraSensor derived class to register
> + *
> + * Register a CameraSensor subclass with the factory and make it available to
> + * try and match sensors. The subclass needs to implement two static functions:
> + *
> + * \code{.cpp}
> + * static bool match(const MediaEntity *entity);
> + * static std::unique_ptr<sensor> create(MediaEntity *entity);
> + * \endcode
> + *
> + * The match() function tests if the sensor class supports the camera sensor
> + * identified by a MediaEntity.
> + *
> + * The create() function creates a new instance of the sensor class. It may
> + * return a null pointer if initialization of the instance fails. It will only
> + * be called if the match() function has returned true for the given entity.
> + */
> +
>  } /* namespace libcamera */
> diff --git a/test/camera-sensor.cpp b/test/camera-sensor.cpp
> index 1d402c43355b..869c788965da 100644
> --- a/test/camera-sensor.cpp
> +++ b/test/camera-sensor.cpp
> @@ -52,8 +52,8 @@ protected:
>                         return TestFail;
>                 }
>  
> -               sensor_ = new CameraSensor(entity);
> -               if (sensor_->init() < 0) {
> +               sensor_ = CameraSensorFactoryBase::create(entity);
> +               if (!sensor_) {
>                         cerr << "Unable to initialise camera sensor" << endl;
>                         return TestFail;
>                 }
> @@ -118,13 +118,12 @@ protected:
>  
>         void cleanup()
>         {
> -               delete sensor_;
>         }
>  
>  private:
>         std::unique_ptr<DeviceEnumerator> enumerator_;
>         std::shared_ptr<MediaDevice> media_;
> -       CameraSensor *sensor_;
> +       std::unique_ptr<CameraSensor> sensor_;
>         CameraLens *lens_;
>  };
>  
> diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.cpp b/test/v4l2_videodevice/v4l2_videodevice_test.cpp
> index 1113cf5bf8cf..9fbd24cc91ea 100644
> --- a/test/v4l2_videodevice/v4l2_videodevice_test.cpp
> +++ b/test/v4l2_videodevice/v4l2_videodevice_test.cpp
> @@ -64,8 +64,8 @@ int V4L2VideoDeviceTest::init()
>         format.size.height = 480;
>  
>         if (driver_ == "vimc") {
> -               sensor_ = new CameraSensor(media_->getEntityByName("Sensor A"));
> -               if (sensor_->init())
> +               sensor_ = CameraSensorFactoryBase::create(media_->getEntityByName("Sensor A"));
> +               if (!sensor_)
>                         return TestFail;
>  
>                 debayer_ = new V4L2Subdevice(media_->getEntityByName("Debayer A"));
> @@ -98,6 +98,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 b5871ce69e18..7c9003ec4c4d 100644
> --- a/test/v4l2_videodevice/v4l2_videodevice_test.h
> +++ b/test/v4l2_videodevice/v4l2_videodevice_test.h
> @@ -36,7 +36,7 @@ protected:
>         std::string entity_;
>         std::unique_ptr<libcamera::DeviceEnumerator> enumerator_;
>         std::shared_ptr<libcamera::MediaDevice> media_;
> -       libcamera::CameraSensor *sensor_;
> +       std::unique_ptr<libcamera::CameraSensor> sensor_;
>         libcamera::V4L2Subdevice *debayer_;
>         libcamera::V4L2VideoDevice *capture_;
>         std::vector<std::unique_ptr<libcamera::FrameBuffer>> buffers_;
> -- 
> 2.47.0
>
Jacopo Mondi Nov. 5, 2024, 1:55 p.m. UTC | #2 
Hi Kieran

On Tue, Nov 05, 2024 at 09:58:04AM +0000, Kieran Bingham wrote:
> Quoting Jacopo Mondi (2024-10-22 15:53:13)
> > From: Jacopo Mondi <jacopo@jmondi.org>
> >
> > 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.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > ---
> >  include/libcamera/internal/camera_sensor.h    |  48 +++++-
> >  src/libcamera/pipeline/imx8-isi/imx8-isi.cpp  |   9 +-
> >  src/libcamera/pipeline/ipu3/cio2.cpp          |   7 +-
> >  src/libcamera/pipeline/mali-c55/mali-c55.cpp  |   5 +-
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |   7 +-
> >  .../pipeline/rpi/common/pipeline_base.cpp     |   5 +-
> >  src/libcamera/pipeline/simple/simple.cpp      |   9 +-
> >  src/libcamera/pipeline/vimc/vimc.cpp          |   7 +-
> >  src/libcamera/sensor/camera_sensor.cpp        | 162 ++++++++++++++++++
> >  test/camera-sensor.cpp                        |   7 +-
> >  .../v4l2_videodevice_test.cpp                 |   5 +-
> >  test/v4l2_videodevice/v4l2_videodevice_test.h |   2 +-
> >  12 files changed, 233 insertions(+), 40 deletions(-)
> >
> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > index a42c15fa37ce..2d6cb697a870 100644
> > --- a/include/libcamera/internal/camera_sensor.h
> > +++ b/include/libcamera/internal/camera_sensor.h
> > @@ -38,7 +38,6 @@ enum class Orientation;
> >  class CameraSensor : protected Loggable
> >  {
> >  public:
> > -       explicit CameraSensor(const MediaEntity *entity);
> >         ~CameraSensor();
> >
> >         int init();
> > @@ -81,6 +80,7 @@ public:
> >         int setTestPatternMode(controls::draft::TestPatternModeEnum mode);
> >
> >  protected:
> > +       explicit CameraSensor(const MediaEntity *entity);
> >         std::string logPrefix() const override;
> >
> >  private:
> > @@ -122,4 +122,50 @@ private:
> >         std::unique_ptr<CameraLens> focusLens_;
> >  };
> >
> > +class CameraSensorFactoryBase
>
> Seems I don't have much to comment on here so I might be down to
> bikeshedding!
>
> Having read through the usages of the CameraSensorFactoryBase, I would
> be tempted to rename this class to:
>
> 	CameraSensorFactory
>
>
> > +{
> > +public:
> > +       CameraSensorFactoryBase();
> > +       virtual ~CameraSensorFactoryBase() = default;
> > +
> > +       static std::unique_ptr<CameraSensor> create(MediaEntity *entity);
> > +
> > +private:
> > +       LIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorFactoryBase)
> > +
> > +       static std::vector<CameraSensorFactoryBase *> &factories();
> > +
> > +       static void registerFactory(CameraSensorFactoryBase *factory);
> > +
> > +       virtual bool match(const MediaEntity *entity) const = 0;
> > +
> > +       virtual std::unique_ptr<CameraSensor>
> > +       createInstance(MediaEntity *entity) const = 0;
> > +};
> > +
> > +template<typename _CameraSensor>
> > +class CameraSensorFactory final : public CameraSensorFactoryBase
>
> And this one to CameraSensorFactoryType (or other name as appropriate)
>
> > +{
> > +public:
> > +       CameraSensorFactory()
> > +               : CameraSensorFactoryBase()
> > +       {
> > +       }
> > +
> > +private:
> > +       bool match(const MediaEntity *entity) const override
> > +       {
> > +               return _CameraSensor::match(entity);
> > +       }
> > +
> > +       std::unique_ptr<CameraSensor>
> > +       createInstance(MediaEntity *entity) const override
> > +       {
> > +               return _CameraSensor::create(entity);
> > +       }
> > +};
> > +
> > +#define REGISTER_CAMERA_SENSOR(sensor) \
> > +static CameraSensorFactory<sensor> global_##sensor##Factory{};
>
> This I think would be the only rename/usage of the simple name
> CameraSensorFactory currently so the only place to be renamed to
> CameraSensorFactoryType<sensor> ?
>
> > +
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> > index 72aa6c75608a..4e66b3368d5a 100644
> > --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> > +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> > @@ -157,11 +157,10 @@ PipelineHandlerISI *ISICameraData::pipe()
> >  /* Open and initialize pipe components. */
> >  int ISICameraData::init()
> >  {
> > -       int ret = sensor_->init();
> > -       if (ret)
> > -               return ret;
> > +       if (!sensor_)
> > +               return -ENODEV;
> >
> > -       ret = csis_->open();
> > +       int ret = csis_->open();
> >         if (ret)
> >                 return ret;
> >
> > @@ -1057,7 +1056,7 @@ bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)
> >                 std::unique_ptr<ISICameraData> data =
> >                         std::make_unique<ISICameraData>(this);
> >
> > -               data->sensor_ = std::make_unique<CameraSensor>(sensor);
> > +               data->sensor_ = CameraSensorFactoryBase::create(sensor);
>
> So that the users of the Factory create read as :
>
> 	data->sensor_ = CameraSensorFactory::create(sensor);
>
> <end of CameraSensorFactory name bikeshed threading>
>
>

All other factories in the source base use this pattern

In example
src/libcamera/pipeline/simple/simple.cpp:               converter_ = ConverterFactoryBase::create(converter);

I would rather keep this consistent

> >                 data->csis_ = std::make_unique<V4L2Subdevice>(csi);
> >                 data->xbarSink_ = sink;
> >
> > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> > index 74a5d93f88ae..aa544d7b0303 100644
> > --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> > @@ -134,10 +134,9 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)
> >
> >         MediaLink *link = links[0];
> >         MediaEntity *sensorEntity = link->source()->entity();
> > -       sensor_ = std::make_unique<CameraSensor>(sensorEntity);
> > -       ret = sensor_->init();
> > -       if (ret)
> > -               return ret;
> > +       sensor_ = CameraSensorFactoryBase::create(sensorEntity);
> > +       if (!sensor_)
> > +               return -ENODEV;
>
> I like that the construction is now simplified and there's no need to
> create and then initialise the Sensor class in every usage...
>
> >
> >         ret = link->setEnabled(true);
> >         if (ret)
> > diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > index 45c71c1dd619..4b71e0dad15e 100644
> > --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > @@ -141,9 +141,8 @@ int MaliC55CameraData::init()
> >          * Register a CameraSensor if we connect to a sensor and create
> >          * an entity for the connected CSI-2 receiver.
> >          */
> > -       sensor_ = std::make_unique<CameraSensor>(entity_);
> > -       ret = sensor_->init();
> > -       if (ret)
> > +       sensor_ = CameraSensorFactoryBase::create(entity_);
> > +       if (!sensor_)
> >                 return ret;
> >
> >         const MediaPad *sourcePad = entity_->getPadByIndex(0);
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index d8d1d65fbbf9..95c25e976015 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -1122,10 +1122,9 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> >                 std::make_unique<RkISP1CameraData>(this, &mainPath_,
> >                                                    hasSelfPath_ ? &selfPath_ : nullptr);
> >
> > -       data->sensor_ = std::make_unique<CameraSensor>(sensor);
> > -       ret = data->sensor_->init();
> > -       if (ret)
> > -               return ret;
> > +       data->sensor_ = CameraSensorFactoryBase::create(sensor);
> > +       if (!data->sensor_)
> > +               return -ENODEV;
> >
> >         /* Initialize the camera properties. */
> >         data->properties_ = data->sensor_->properties();
> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > index 3041fd1ed9fd..4f56bd33df05 100644
> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > @@ -774,13 +774,10 @@ int PipelineHandlerBase::registerCamera(std::unique_ptr<RPi::CameraData> &camera
> >         CameraData *data = cameraData.get();
> >         int ret;
> >
> > -       data->sensor_ = std::make_unique<CameraSensor>(sensorEntity);
> > +       data->sensor_ = CameraSensorFactoryBase::create(sensorEntity);
> >         if (!data->sensor_)
> >                 return -EINVAL;
> >
> > -       if (data->sensor_->init())
> > -               return -EINVAL;
> > -
> >         /* Populate the map of sensor supported formats and sizes. */
> >         for (auto const mbusCode : data->sensor_->mbusCodes())
> >                 data->sensorFormats_.emplace(mbusCode,
> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > index 3ddce71d3757..67f583b8a22c 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -388,8 +388,6 @@ SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,
> >                                    MediaEntity *sensor)
> >         : Camera::Private(pipe), streams_(numStreams)
> >  {
> > -       int ret;
> > -
> >         /*
> >          * Find the shortest path from the camera sensor to a video capture
> >          * device using the breadth-first search algorithm. This heuristic will
> > @@ -480,12 +478,9 @@ SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,
> >         }
> >
> >         /* Finally also remember the sensor. */
> > -       sensor_ = std::make_unique<CameraSensor>(sensor);
> > -       ret = sensor_->init();
> > -       if (ret) {
> > -               sensor_.reset();
> > +       sensor_ = CameraSensorFactoryBase::create(sensor);
> > +       if (!sensor_)
> >                 return;
> > -       }
> >
> >         LOG(SimplePipeline, Debug)
> >                 << "Found pipeline: "
> > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> > index 2165bae890cb..2e474133439c 100644
> > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > @@ -532,10 +532,9 @@ int VimcCameraData::init()
> >                 return ret;
> >
> >         /* Create and open the camera sensor, debayer, scaler and video device. */
> > -       sensor_ = std::make_unique<CameraSensor>(media_->getEntityByName("Sensor B"));
> > -       ret = sensor_->init();
> > -       if (ret)
> > -               return ret;
> > +       sensor_ = CameraSensorFactoryBase::create(media_->getEntityByName("Sensor B"));
> > +       if (!sensor_)
> > +               return -ENODEV;
> >
> >         debayer_ = V4L2Subdevice::fromEntityName(media_, "Debayer B");
> >         if (debayer_->open())
> > diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp
> > index 1b224f1989fe..025c9eefdd12 100644
> > --- a/src/libcamera/sensor/camera_sensor.cpp
> > +++ b/src/libcamera/sensor/camera_sensor.cpp
> > @@ -11,6 +11,7 @@
> >  #include <cmath>
> >  #include <float.h>
> >  #include <limits.h>
> > +#include <map>
> >  #include <string.h>
> >
> >  #include <libcamera/base/utils.h>
> > @@ -1202,4 +1203,165 @@ std::string CameraSensor::logPrefix() const
> >         return "'" + entity_->name() + "'";
> >  }
> >
> > +namespace {
> > +
> > +/* Transitory default camera sensor implementation */
> > +class CameraSensorDefault : public CameraSensor
> > +{
> > +public:
> > +       CameraSensorDefault(MediaEntity *entity)
> > +               : CameraSensor(entity)
> > +       {
> > +       }
> > +
> > +       static bool match([[maybe_unused]] const MediaEntity *entity)
> > +       {
>
> Do we need any kind of debug/warning/log in here to say its' using a
> temporary default wrapper? (Only thinking here in the event that we keep
> this as a lowest priority factory ... see comment below in
> CameraSensorFactoryBase::create())

I suspect this "Default" implementation will still be used by most
sensors and won't be temporary. There will indeed be specializations
for specific use cases, but most sensors will use this version.

>
> > +               return true;
> > +       }
> > +
> > +       static std::unique_ptr<CameraSensorDefault> create(MediaEntity *entity)
> > +       {
> > +               std::unique_ptr<CameraSensorDefault> sensor =
> > +                       std::make_unique<CameraSensorDefault>(entity);
> > +
> > +               if (sensor->init())
> > +                       return nullptr;
> > +
> > +               return sensor;
> > +       }
> > +};
> > +
> > +REGISTER_CAMERA_SENSOR(CameraSensorDefault)
> > +
> > +}; /* namespace */
> > +
> > +/**
> > + * \class CameraSensorFactoryBase
> > + * \brief Base class for camera sensor factories
> > + *
> > + * The CameraSensorFactoryBase class is the base of all specializations of
> > + * the CameraSensorFactory class template. It implements the factory
> > + * registration, maintains a registry of factories, and provides access to the
> > + * registered factories.
> > + */
> > +
> > +/**
> > + * \brief Construct a camera sensor factory base
> > + *
> > + * Creating an instance of the factory base registers it with the global list of
> > + * factories, accessible through the factories() function.
> > + */
> > +CameraSensorFactoryBase::CameraSensorFactoryBase()
> > +{
> > +       registerFactory(this);
> > +}
> > +
> > +/**
> > + * \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
> > + *
> > + * \return A unique pointer to a new instance of the CameraSensor subclass
> > + * matching the entity, or a null pointer if no such factory exists
> > + */
> > +std::unique_ptr<CameraSensor> CameraSensorFactoryBase::create(MediaEntity *entity)
> > +{
> > +       const std::vector<CameraSensorFactoryBase *> &factories =
> > +               CameraSensorFactoryBase::factories();
> > +
> > +       for (const CameraSensorFactoryBase *factory : factories) {
>
> Will the factories need to be sorted in any priority order ? Do we
> anticipate multiple Factory classes could match ? (I.e. if the
> transitory default were to be kept for instance?)

You should have a look at 3/3 :)
[PATCH 3/3] libcamera: camera_sensor: Sort factories by priority

>
> Fine to handle that when required of course...
>
> Bike shedding aside, there's nothing in here that scares me so:
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>

Thanks, I can go ahead and merge it

>
> > +               if (!factory->match(entity))
> > +                       continue;
> > +
> > +               std::unique_ptr<CameraSensor> sensor = factory->createInstance(entity);
> > +               if (!sensor) {
> > +                       LOG(CameraSensor, Error)
> > +                               << "Failed to create sensor for '"
> > +                               << entity->name();
> > +                       return nullptr;
> > +               }
> > +
> > +               return sensor;
> > +       }
> > +
> > +       return nullptr;
> > +}
> > +
> > +/**
> > + * \brief Retrieve the list of all camera sensor factories
> > + * \return The list of camera sensor factories
> > + */
> > +std::vector<CameraSensorFactoryBase *> &CameraSensorFactoryBase::factories()
> > +{
> > +       /*
> > +        * The static factories map is defined inside the function to ensure
> > +        * it gets initialized on first use, without any dependency on link
> > +        * order.
> > +        */
> > +       static std::vector<CameraSensorFactoryBase *> factories;
> > +       return factories;
> > +}
> > +
> > +/**
> > + * \brief Add a camera sensor class to the registry
> > + * \param[in] factory Factory to use to construct the camera sensor
> > + */
> > +void CameraSensorFactoryBase::registerFactory(CameraSensorFactoryBase *factory)
> > +{
> > +       std::vector<CameraSensorFactoryBase *> &factories =
> > +               CameraSensorFactoryBase::factories();
> > +
> > +       factories.push_back(factory);
> > +}
> > +
> > +/**
> > + * \class CameraSensorFactory
> > + * \brief Registration of CameraSensorFactory classes and creation of instances
> > + * \tparam _CameraSensor The camera sensor class type for this factory
> > + *
> > + * To facilitate discovery and instantiation of CameraSensor classes, the
> > + * CameraSensorFactory class implements auto-registration of camera sensors.
> > + * Each CameraSensor subclass shall register itself using the
> > + * REGISTER_CAMERA_SENSOR() macro, which will create a corresponding instance
> > + * of a CameraSensorFactory subclass and register it with the static list of
> > + * factories.
> > + */
> > +
> > +/**
> > + * \fn CameraSensorFactory::CameraSensorFactory()
> > + * \brief Construct a camera sensor factory
> > + *
> > + * Creating an instance of the factory registers it with the global list of
> > + * factories, accessible through the CameraSensorFactoryBase::factories()
> > + * function.
> > + */
> > +
> > +/**
> > + * \fn CameraSensorFactory::createInstance() const
> > + * \brief Create an instance of the CameraSensor corresponding to the factory
> > + *
> > + * \return A unique pointer to a newly constructed instance of the CameraSensor
> > + * subclass corresponding to the factory
> > + */
> > +
> > +/**
> > + * \def REGISTER_CAMERA_SENSOR(sensor)
> > + * \brief Register a camera sensor type to the sensor factory
> > + * \param[in] sensor Class name of the CameraSensor derived class to register
> > + *
> > + * Register a CameraSensor subclass with the factory and make it available to
> > + * try and match sensors. The subclass needs to implement two static functions:
> > + *
> > + * \code{.cpp}
> > + * static bool match(const MediaEntity *entity);
> > + * static std::unique_ptr<sensor> create(MediaEntity *entity);
> > + * \endcode
> > + *
> > + * The match() function tests if the sensor class supports the camera sensor
> > + * identified by a MediaEntity.
> > + *
> > + * The create() function creates a new instance of the sensor class. It may
> > + * return a null pointer if initialization of the instance fails. It will only
> > + * be called if the match() function has returned true for the given entity.
> > + */
> > +
> >  } /* namespace libcamera */
> > diff --git a/test/camera-sensor.cpp b/test/camera-sensor.cpp
> > index 1d402c43355b..869c788965da 100644
> > --- a/test/camera-sensor.cpp
> > +++ b/test/camera-sensor.cpp
> > @@ -52,8 +52,8 @@ protected:
> >                         return TestFail;
> >                 }
> >
> > -               sensor_ = new CameraSensor(entity);
> > -               if (sensor_->init() < 0) {
> > +               sensor_ = CameraSensorFactoryBase::create(entity);
> > +               if (!sensor_) {
> >                         cerr << "Unable to initialise camera sensor" << endl;
> >                         return TestFail;
> >                 }
> > @@ -118,13 +118,12 @@ protected:
> >
> >         void cleanup()
> >         {
> > -               delete sensor_;
> >         }
> >
> >  private:
> >         std::unique_ptr<DeviceEnumerator> enumerator_;
> >         std::shared_ptr<MediaDevice> media_;
> > -       CameraSensor *sensor_;
> > +       std::unique_ptr<CameraSensor> sensor_;
> >         CameraLens *lens_;
> >  };
> >
> > diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.cpp b/test/v4l2_videodevice/v4l2_videodevice_test.cpp
> > index 1113cf5bf8cf..9fbd24cc91ea 100644
> > --- a/test/v4l2_videodevice/v4l2_videodevice_test.cpp
> > +++ b/test/v4l2_videodevice/v4l2_videodevice_test.cpp
> > @@ -64,8 +64,8 @@ int V4L2VideoDeviceTest::init()
> >         format.size.height = 480;
> >
> >         if (driver_ == "vimc") {
> > -               sensor_ = new CameraSensor(media_->getEntityByName("Sensor A"));
> > -               if (sensor_->init())
> > +               sensor_ = CameraSensorFactoryBase::create(media_->getEntityByName("Sensor A"));
> > +               if (!sensor_)
> >                         return TestFail;
> >
> >                 debayer_ = new V4L2Subdevice(media_->getEntityByName("Debayer A"));
> > @@ -98,6 +98,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 b5871ce69e18..7c9003ec4c4d 100644
> > --- a/test/v4l2_videodevice/v4l2_videodevice_test.h
> > +++ b/test/v4l2_videodevice/v4l2_videodevice_test.h
> > @@ -36,7 +36,7 @@ protected:
> >         std::string entity_;
> >         std::unique_ptr<libcamera::DeviceEnumerator> enumerator_;
> >         std::shared_ptr<libcamera::MediaDevice> media_;
> > -       libcamera::CameraSensor *sensor_;
> > +       std::unique_ptr<libcamera::CameraSensor> sensor_;
> >         libcamera::V4L2Subdevice *debayer_;
> >         libcamera::V4L2VideoDevice *capture_;
> >         std::vector<std::unique_ptr<libcamera::FrameBuffer>> buffers_;
> > --
> > 2.47.0
> >

Patch
diff mbox series 

diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
index a42c15fa37ce..2d6cb697a870 100644
--- a/include/libcamera/internal/camera_sensor.h
+++ b/include/libcamera/internal/camera_sensor.h
@@ -38,7 +38,6 @@  enum class Orientation;
 class CameraSensor : protected Loggable
 {
 public:
-	explicit CameraSensor(const MediaEntity *entity);
 	~CameraSensor();
 
 	int init();
@@ -81,6 +80,7 @@  public:
 	int setTestPatternMode(controls::draft::TestPatternModeEnum mode);
 
 protected:
+	explicit CameraSensor(const MediaEntity *entity);
 	std::string logPrefix() const override;
 
 private:
@@ -122,4 +122,50 @@  private:
 	std::unique_ptr<CameraLens> focusLens_;
 };
 
+class CameraSensorFactoryBase
+{
+public:
+	CameraSensorFactoryBase();
+	virtual ~CameraSensorFactoryBase() = default;
+
+	static std::unique_ptr<CameraSensor> create(MediaEntity *entity);
+
+private:
+	LIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorFactoryBase)
+
+	static std::vector<CameraSensorFactoryBase *> &factories();
+
+	static void registerFactory(CameraSensorFactoryBase *factory);
+
+	virtual bool match(const MediaEntity *entity) const = 0;
+
+	virtual std::unique_ptr<CameraSensor>
+	createInstance(MediaEntity *entity) const = 0;
+};
+
+template<typename _CameraSensor>
+class CameraSensorFactory final : public CameraSensorFactoryBase
+{
+public:
+	CameraSensorFactory()
+		: CameraSensorFactoryBase()
+	{
+	}
+
+private:
+	bool match(const MediaEntity *entity) const override
+	{
+		return _CameraSensor::match(entity);
+	}
+
+	std::unique_ptr<CameraSensor>
+	createInstance(MediaEntity *entity) const override
+	{
+		return _CameraSensor::create(entity);
+	}
+};
+
+#define REGISTER_CAMERA_SENSOR(sensor) \
+static CameraSensorFactory<sensor> global_##sensor##Factory{};
+
 } /* namespace libcamera */
diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
index 72aa6c75608a..4e66b3368d5a 100644
--- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
+++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
@@ -157,11 +157,10 @@  PipelineHandlerISI *ISICameraData::pipe()
 /* Open and initialize pipe components. */
 int ISICameraData::init()
 {
-	int ret = sensor_->init();
-	if (ret)
-		return ret;
+	if (!sensor_)
+		return -ENODEV;
 
-	ret = csis_->open();
+	int ret = csis_->open();
 	if (ret)
 		return ret;
 
@@ -1057,7 +1056,7 @@  bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)
 		std::unique_ptr<ISICameraData> data =
 			std::make_unique<ISICameraData>(this);
 
-		data->sensor_ = std::make_unique<CameraSensor>(sensor);
+		data->sensor_ = CameraSensorFactoryBase::create(sensor);
 		data->csis_ = std::make_unique<V4L2Subdevice>(csi);
 		data->xbarSink_ = sink;
 
diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
index 74a5d93f88ae..aa544d7b0303 100644
--- a/src/libcamera/pipeline/ipu3/cio2.cpp
+++ b/src/libcamera/pipeline/ipu3/cio2.cpp
@@ -134,10 +134,9 @@  int CIO2Device::init(const MediaDevice *media, unsigned int index)
 
 	MediaLink *link = links[0];
 	MediaEntity *sensorEntity = link->source()->entity();
-	sensor_ = std::make_unique<CameraSensor>(sensorEntity);
-	ret = sensor_->init();
-	if (ret)
-		return ret;
+	sensor_ = CameraSensorFactoryBase::create(sensorEntity);
+	if (!sensor_)
+		return -ENODEV;
 
 	ret = link->setEnabled(true);
 	if (ret)
diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
index 45c71c1dd619..4b71e0dad15e 100644
--- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
+++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
@@ -141,9 +141,8 @@  int MaliC55CameraData::init()
 	 * Register a CameraSensor if we connect to a sensor and create
 	 * an entity for the connected CSI-2 receiver.
 	 */
-	sensor_ = std::make_unique<CameraSensor>(entity_);
-	ret = sensor_->init();
-	if (ret)
+	sensor_ = CameraSensorFactoryBase::create(entity_);
+	if (!sensor_)
 		return ret;
 
 	const MediaPad *sourcePad = entity_->getPadByIndex(0);
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index d8d1d65fbbf9..95c25e976015 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -1122,10 +1122,9 @@  int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
 		std::make_unique<RkISP1CameraData>(this, &mainPath_,
 						   hasSelfPath_ ? &selfPath_ : nullptr);
 
-	data->sensor_ = std::make_unique<CameraSensor>(sensor);
-	ret = data->sensor_->init();
-	if (ret)
-		return ret;
+	data->sensor_ = CameraSensorFactoryBase::create(sensor);
+	if (!data->sensor_)
+		return -ENODEV;
 
 	/* Initialize the camera properties. */
 	data->properties_ = data->sensor_->properties();
diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
index 3041fd1ed9fd..4f56bd33df05 100644
--- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
+++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
@@ -774,13 +774,10 @@  int PipelineHandlerBase::registerCamera(std::unique_ptr<RPi::CameraData> &camera
 	CameraData *data = cameraData.get();
 	int ret;
 
-	data->sensor_ = std::make_unique<CameraSensor>(sensorEntity);
+	data->sensor_ = CameraSensorFactoryBase::create(sensorEntity);
 	if (!data->sensor_)
 		return -EINVAL;
 
-	if (data->sensor_->init())
-		return -EINVAL;
-
 	/* Populate the map of sensor supported formats and sizes. */
 	for (auto const mbusCode : data->sensor_->mbusCodes())
 		data->sensorFormats_.emplace(mbusCode,
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
index 3ddce71d3757..67f583b8a22c 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -388,8 +388,6 @@  SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,
 				   MediaEntity *sensor)
 	: Camera::Private(pipe), streams_(numStreams)
 {
-	int ret;
-
 	/*
 	 * Find the shortest path from the camera sensor to a video capture
 	 * device using the breadth-first search algorithm. This heuristic will
@@ -480,12 +478,9 @@  SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,
 	}
 
 	/* Finally also remember the sensor. */
-	sensor_ = std::make_unique<CameraSensor>(sensor);
-	ret = sensor_->init();
-	if (ret) {
-		sensor_.reset();
+	sensor_ = CameraSensorFactoryBase::create(sensor);
+	if (!sensor_)
 		return;
-	}
 
 	LOG(SimplePipeline, Debug)
 		<< "Found pipeline: "
diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
index 2165bae890cb..2e474133439c 100644
--- a/src/libcamera/pipeline/vimc/vimc.cpp
+++ b/src/libcamera/pipeline/vimc/vimc.cpp
@@ -532,10 +532,9 @@  int VimcCameraData::init()
 		return ret;
 
 	/* Create and open the camera sensor, debayer, scaler and video device. */
-	sensor_ = std::make_unique<CameraSensor>(media_->getEntityByName("Sensor B"));
-	ret = sensor_->init();
-	if (ret)
-		return ret;
+	sensor_ = CameraSensorFactoryBase::create(media_->getEntityByName("Sensor B"));
+	if (!sensor_)
+		return -ENODEV;
 
 	debayer_ = V4L2Subdevice::fromEntityName(media_, "Debayer B");
 	if (debayer_->open())
diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp
index 1b224f1989fe..025c9eefdd12 100644
--- a/src/libcamera/sensor/camera_sensor.cpp
+++ b/src/libcamera/sensor/camera_sensor.cpp
@@ -11,6 +11,7 @@ 
 #include <cmath>
 #include <float.h>
 #include <limits.h>
+#include <map>
 #include <string.h>
 
 #include <libcamera/base/utils.h>
@@ -1202,4 +1203,165 @@  std::string CameraSensor::logPrefix() const
 	return "'" + entity_->name() + "'";
 }
 
+namespace {
+
+/* Transitory default camera sensor implementation */
+class CameraSensorDefault : public CameraSensor
+{
+public:
+	CameraSensorDefault(MediaEntity *entity)
+		: CameraSensor(entity)
+	{
+	}
+
+	static bool match([[maybe_unused]] const MediaEntity *entity)
+	{
+		return true;
+	}
+
+	static std::unique_ptr<CameraSensorDefault> create(MediaEntity *entity)
+	{
+		std::unique_ptr<CameraSensorDefault> sensor =
+			std::make_unique<CameraSensorDefault>(entity);
+
+		if (sensor->init())
+			return nullptr;
+
+		return sensor;
+	}
+};
+
+REGISTER_CAMERA_SENSOR(CameraSensorDefault)
+
+}; /* namespace */
+
+/**
+ * \class CameraSensorFactoryBase
+ * \brief Base class for camera sensor factories
+ *
+ * The CameraSensorFactoryBase class is the base of all specializations of
+ * the CameraSensorFactory class template. It implements the factory
+ * registration, maintains a registry of factories, and provides access to the
+ * registered factories.
+ */
+
+/**
+ * \brief Construct a camera sensor factory base
+ *
+ * Creating an instance of the factory base registers it with the global list of
+ * factories, accessible through the factories() function.
+ */
+CameraSensorFactoryBase::CameraSensorFactoryBase()
+{
+	registerFactory(this);
+}
+
+/**
+ * \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
+ *
+ * \return A unique pointer to a new instance of the CameraSensor subclass
+ * matching the entity, or a null pointer if no such factory exists
+ */
+std::unique_ptr<CameraSensor> CameraSensorFactoryBase::create(MediaEntity *entity)
+{
+	const std::vector<CameraSensorFactoryBase *> &factories =
+		CameraSensorFactoryBase::factories();
+
+	for (const CameraSensorFactoryBase *factory : factories) {
+		if (!factory->match(entity))
+			continue;
+
+		std::unique_ptr<CameraSensor> sensor = factory->createInstance(entity);
+		if (!sensor) {
+			LOG(CameraSensor, Error)
+				<< "Failed to create sensor for '"
+				<< entity->name();
+			return nullptr;
+		}
+
+		return sensor;
+	}
+
+	return nullptr;
+}
+
+/**
+ * \brief Retrieve the list of all camera sensor factories
+ * \return The list of camera sensor factories
+ */
+std::vector<CameraSensorFactoryBase *> &CameraSensorFactoryBase::factories()
+{
+	/*
+	 * The static factories map is defined inside the function to ensure
+	 * it gets initialized on first use, without any dependency on link
+	 * order.
+	 */
+	static std::vector<CameraSensorFactoryBase *> factories;
+	return factories;
+}
+
+/**
+ * \brief Add a camera sensor class to the registry
+ * \param[in] factory Factory to use to construct the camera sensor
+ */
+void CameraSensorFactoryBase::registerFactory(CameraSensorFactoryBase *factory)
+{
+	std::vector<CameraSensorFactoryBase *> &factories =
+		CameraSensorFactoryBase::factories();
+
+	factories.push_back(factory);
+}
+
+/**
+ * \class CameraSensorFactory
+ * \brief Registration of CameraSensorFactory classes and creation of instances
+ * \tparam _CameraSensor The camera sensor class type for this factory
+ *
+ * To facilitate discovery and instantiation of CameraSensor classes, the
+ * CameraSensorFactory class implements auto-registration of camera sensors.
+ * Each CameraSensor subclass shall register itself using the
+ * REGISTER_CAMERA_SENSOR() macro, which will create a corresponding instance
+ * of a CameraSensorFactory subclass and register it with the static list of
+ * factories.
+ */
+
+/**
+ * \fn CameraSensorFactory::CameraSensorFactory()
+ * \brief Construct a camera sensor factory
+ *
+ * Creating an instance of the factory registers it with the global list of
+ * factories, accessible through the CameraSensorFactoryBase::factories()
+ * function.
+ */
+
+/**
+ * \fn CameraSensorFactory::createInstance() const
+ * \brief Create an instance of the CameraSensor corresponding to the factory
+ *
+ * \return A unique pointer to a newly constructed instance of the CameraSensor
+ * subclass corresponding to the factory
+ */
+
+/**
+ * \def REGISTER_CAMERA_SENSOR(sensor)
+ * \brief Register a camera sensor type to the sensor factory
+ * \param[in] sensor Class name of the CameraSensor derived class to register
+ *
+ * Register a CameraSensor subclass with the factory and make it available to
+ * try and match sensors. The subclass needs to implement two static functions:
+ *
+ * \code{.cpp}
+ * static bool match(const MediaEntity *entity);
+ * static std::unique_ptr<sensor> create(MediaEntity *entity);
+ * \endcode
+ *
+ * The match() function tests if the sensor class supports the camera sensor
+ * identified by a MediaEntity.
+ *
+ * The create() function creates a new instance of the sensor class. It may
+ * return a null pointer if initialization of the instance fails. It will only
+ * be called if the match() function has returned true for the given entity.
+ */
+
 } /* namespace libcamera */
diff --git a/test/camera-sensor.cpp b/test/camera-sensor.cpp
index 1d402c43355b..869c788965da 100644
--- a/test/camera-sensor.cpp
+++ b/test/camera-sensor.cpp
@@ -52,8 +52,8 @@  protected:
 			return TestFail;
 		}
 
-		sensor_ = new CameraSensor(entity);
-		if (sensor_->init() < 0) {
+		sensor_ = CameraSensorFactoryBase::create(entity);
+		if (!sensor_) {
 			cerr << "Unable to initialise camera sensor" << endl;
 			return TestFail;
 		}
@@ -118,13 +118,12 @@  protected:
 
 	void cleanup()
 	{
-		delete sensor_;
 	}
 
 private:
 	std::unique_ptr<DeviceEnumerator> enumerator_;
 	std::shared_ptr<MediaDevice> media_;
-	CameraSensor *sensor_;
+	std::unique_ptr<CameraSensor> sensor_;
 	CameraLens *lens_;
 };
 
diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.cpp b/test/v4l2_videodevice/v4l2_videodevice_test.cpp
index 1113cf5bf8cf..9fbd24cc91ea 100644
--- a/test/v4l2_videodevice/v4l2_videodevice_test.cpp
+++ b/test/v4l2_videodevice/v4l2_videodevice_test.cpp
@@ -64,8 +64,8 @@  int V4L2VideoDeviceTest::init()
 	format.size.height = 480;
 
 	if (driver_ == "vimc") {
-		sensor_ = new CameraSensor(media_->getEntityByName("Sensor A"));
-		if (sensor_->init())
+		sensor_ = CameraSensorFactoryBase::create(media_->getEntityByName("Sensor A"));
+		if (!sensor_)
 			return TestFail;
 
 		debayer_ = new V4L2Subdevice(media_->getEntityByName("Debayer A"));
@@ -98,6 +98,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 b5871ce69e18..7c9003ec4c4d 100644
--- a/test/v4l2_videodevice/v4l2_videodevice_test.h
+++ b/test/v4l2_videodevice/v4l2_videodevice_test.h
@@ -36,7 +36,7 @@  protected:
 	std::string entity_;
 	std::unique_ptr<libcamera::DeviceEnumerator> enumerator_;
 	std::shared_ptr<libcamera::MediaDevice> media_;
-	libcamera::CameraSensor *sensor_;
+	std::unique_ptr<libcamera::CameraSensor> sensor_;
 	libcamera::V4L2Subdevice *debayer_;
 	libcamera::V4L2VideoDevice *capture_;
 	std::vector<std::unique_ptr<libcamera::FrameBuffer>> buffers_;