[libcamera-devel] [PATCH v3 3/6] libcamera: camera_sensor: Introduce CameraSensorFactory
Jacopo Mondi
jacopo at jmondi.org
Thu Mar 26 13:31:08 CET 2020
Hi Kieran
On Tue, Mar 24, 2020 at 03:37:21PM +0000, Kieran Bingham wrote:
> 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/
>
Ack on all the above ones
> > + * 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 ...
>
Those two are (iirc) copied from the pipeline handler factory.
I would keep the two identical and fix in one go eventually
Thanks
j
> > + */
> > +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
More information about the libcamera-devel
mailing list