[PATCH 1/3] libcamera: camera_sensor: Introduce CameraSensorFactory
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Nov 5 10:58:04 CET 2024
Quoting Jacopo Mondi (2024-10-22 15:53:13)
> From: Jacopo Mondi <jacopo at 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 at jmondi.org>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Reviewed-by: Stefan Klug <stefan.klug at 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 at 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
>
More information about the libcamera-devel
mailing list