| Message ID | 20200206185247.202233-4-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Thu, Feb 06, 2020 at 07:52:43PM +0100, 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 sensor handlers. I really like the factory :-) > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/camera_sensor.cpp | 134 ++++++++++++++++++ > src/libcamera/include/camera_sensor.h | 39 ++++- > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +- > src/libcamera/pipeline/vimc.cpp | 2 +- > test/camera-sensor.cpp | 2 +- > .../v4l2_videodevice_test.cpp | 2 +- > 7 files changed, 177 insertions(+), 6 deletions(-) > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > index 2219a4307436..fc8452b607a0 100644 > --- a/src/libcamera/camera_sensor.cpp > +++ b/src/libcamera/camera_sensor.cpp > @@ -11,7 +11,9 @@ > #include <float.h> > #include <iomanip> > #include <limits.h> > +#include <map> No need to include map here, no function in this file make use of it directly. The header is included in camera_sensor.h for the usage of std::map there, that's enough. > #include <math.h> > +#include <string.h> > > #include <libcamera/property_ids.h> > > @@ -42,6 +44,18 @@ LOG_DEFINE_CATEGORY(CameraSensor); > * devices as the needs arise. > */ > > +/** > + * \fn CameraSensor::entityName() I wonder, wouldn't it be simpler to pass the entity name string directly to the REGISTER_CAMERA_SENSOR macro ? It would avoid declaring and undefined static method in the base class. If we ever end up needing more complex matching code, a static match function that takes a media entity pointer would make sense, but for a fully static name, this seems a bit overkill. > + * \brief Retrieve the name of the entity which identifies the supported sensor > + * > + * Camera sensor handlers have to report with this function the name of the > + * media entity which represents the camera sensor they support. The here > + * reported name is then matched against the name of the MediaEntity provided > + * at createSensor() time to identify the correct CameraSensorFactory. > + * > + * \return The name of the media entity that identifies the sensor > + */ > + > /** > * \brief Construct a CameraSensor > * \param[in] entity The media entity backing the camera sensor > @@ -366,4 +380,124 @@ std::string CameraSensor::logPrefix() const > return "'" + subdev_->entity()->name() + "'"; > } > > +/** > + * \class CameraSensorFactory > + * \brief Factory of camera sensor handlers "camera sensor handlers" or "camera sensors" ? We have pipeline handlers, and if we could avoid using the name handler here, it could help reducing confusion. This implies s/sensor handler/sensor below. > + * > + * The class provides to camera sensor handlers the ability to register > + * themselves 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 > + * CameraSensorFactory::createInstance() method, which returns a pointer > + * to the opportune CameraSensor sub-class if any, or a newly created instance > + * of the generic CameraSensor class. > + */ > + > +LOG_DEFINE_CATEGORY(CameraSensorFactory); I think you can use the CameraSensor log category, I don't really see a need to split messages in two categories here. > + > +/** > + * \typedef CameraSensorFactory::factoriesMap factoriesMap is a type, it should thus be called FactoriesMap. > + * \brief An std::map of sensor handler factories "A map of entity names to camera sensor factories" > + * > + * The registered sensor handler factories are associated in a map with the > + * names of the media entity that represent the sensor * This map associates the name of the media entities that represent * camera sensors to the corresponding camera sensor factory. But I think we can just drop the documentation as the type can be made private. > + */ > + > +/** > + * \brief Register a camera sensor handler factory This is the constructor, so * \brief Construct a camera sensor factory > + * \param[in] name The name of the media entity representing 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. How about being consistent with the pipeline handler factory documentation ? * Creating an instance of the factory registers is with the global list of * factories, accessible through the factories() function. > + */ > +CameraSensorFactory::CameraSensorFactory(const char *name) > +{ > + CameraSensorFactory::registerSensorFactory(name, this); No need for the CameraSensorFactory:: prefix. > +} > + > +/** > + * \brief Retrieve the list of registered camera sensor factories > + * > + * The static factories map is defined inside the function to ensures it gets > + * initialized on first use, without any dependency on link order. > + * > + * \return The static list of registered camera sensor factories > + */ > +CameraSensorFactory::factoriesMap &CameraSensorFactory::factories() > +{ > + static factoriesMap factories; > + return factories; > +} > + > +/** > + * \brief Register a camera sensor handler factory > + * \param[in] name The name of the media entity that represents the sensor > + * \param[in] factory The factory instance to register > + * > + * The newly registered sensor handler \a factory is associated with \a name, > + * when a new sensor handler is instantiated with createSensor() the name of > + * the media entity is matched against the \a name registered here to > + * retrieve the correct factory. Should we explain the uniqueness requirement for the name parameter, as done in the pipeline handler factory documentation ? * The camera sensor \a factory is registered associated with an entity \a name. * When a camera sensor is created for a media entity with createSensor(), the * name of the media entity is used to select the corresponding factory. * * The caller is responsible for guaranteeing the uniqueness of the * camera sensor entity name. > + */ > +void CameraSensorFactory::registerSensorFactory(const char *name, > + CameraSensorFactory *factory) Maybe registerFactory() to shorten the name a bit ? > +{ > + factoriesMap &map = CameraSensorFactory::factories(); No need for the prefix here either. Should we log an error if the name is already present in the map ? > + map[name] = factory; > +} > + > +/** > + * \brief Create and return a CameraSensor Maybe "Create a camera sensor corresponding to an entity" ? > + * \param[in] entity The media entity that represents the camera sensor > + * > + * Create a new instance of an handler for the sensor represented by \a > + * entity using one of the registered factories. If no specific handler is > + * available for the sensor, or creating the handler fails, a newly created s/, or creating the handler fails// as that's not what's implemented :-) > + * instance of the generic CameraSensor base class is returned. "... fails, an instance of the generic CameraSensor class is created and returned." > + * > + * Ownership of the created camera sensor instance is passed to the caller > + * which is reponsible for deleting the instance. > + * FIXME: is it worth using a smart pointer here ? I think it is. Could you give it a try ? > + * > + * \return A newly created camera sensor handler instance > + */ > +CameraSensor *CameraSensorFactory::createSensor(const MediaEntity *entity) > +{ > + /* > + * The entity name contains both the sensor name and the I2C bus ID. > + * > + * Remove the i2c bus part and use the sensor name only as key to s/i2c/I2C/ > + * search for on the sensor handlers map. s/on/in/ ? > + */ > + const char *entityName = entity->name().c_str(); > + const char *delim = strchrnul(entityName, ' '); > + std::string sensorName(entityName, delim); Should you split after the last space, not the first space ? I think the following would avoid including string.h. std::string::size_type delim = entity->name().find_last_of(' '); std::string name = entity->name().substr(0, delim); > + > + auto it = CameraSensorFactory::factories().find(sensorName); > + if (it == CameraSensorFactory::factories().end()) { > + LOG(CameraSensorFactory, Info) > + << "Unsupported sensor '" << entity->name() > + << "': using generic sensor handler"; > + return new CameraSensor(entity); > + } > + > + LOG(CameraSensorFactory, Info) << "Create handler for '" > + << entity->name() << "' sensor"; > + > + CameraSensorFactory *factory = it->second; > + return factory->create(entity); > +} > + > +/** > + * \def REGISTER_CAMERA_SENSOR(handler) > + * \brief Register a camera sensor handler to the sensor factory s/to the/with the/ > + * \param[in] handler The name of the sensor handler s/sensor handler/camera sensor class/ > + * > + * Register a camera sensor handler to the sensor factory to make it available s/to the/with the/ > + * to the factory users. > + */ > + > } /* namespace libcamera */ > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h > index 99cff98128dc..2f4a0cc8ad3f 100644 > --- a/src/libcamera/include/camera_sensor.h > +++ b/src/libcamera/include/camera_sensor.h > @@ -7,6 +7,7 @@ > #ifndef __LIBCAMERA_CAMERA_SENSOR_H__ > #define __LIBCAMERA_CAMERA_SENSOR_H__ > > +#include <map> > #include <string> > #include <vector> > > @@ -25,7 +26,8 @@ struct V4L2SubdeviceFormat; > class CameraSensor : protected Loggable > { > public: > - explicit CameraSensor(const MediaEntity *entity); > + static const char *entityName(); > + > ~CameraSensor(); > > CameraSensor(const CameraSensor &) = delete; > @@ -49,6 +51,8 @@ public: > const ControlList &properties() const { return properties_; } > > protected: > + friend class CameraSensorFactory; > + explicit CameraSensor(const MediaEntity *entity); You can make the constructor private. > std::string logPrefix() const; > > private: > @@ -61,6 +65,39 @@ private: > ControlList properties_; > }; > > +class CameraSensorFactory > +{ > +public: > + using factoriesMap = std::map<const std::string, CameraSensorFactory *>; You can make this type private. > + > + CameraSensorFactory(const char *name); > + virtual ~CameraSensorFactory() {} > + > + static CameraSensor *createSensor(const MediaEntity *entity = nullptr); Why the default = nullptr ? The argument should be mandatory. > + > +private: > + static factoriesMap &factories(); > + static void registerSensorFactory(const char *name, > + CameraSensorFactory *factory); > + virtual CameraSensor *create(const MediaEntity *entity) = 0; > + > +}; > + > +#define REGISTER_CAMERA_SENSOR(handler) \ s/handler/sensor/ > +class handler##CameraSensorFactory final : public CameraSensorFactory \ To shorten lines, class sensor##Factory ... > +{ \ > +public: \ > + handler##CameraSensorFactory() \ > + : CameraSensorFactory(handler##CameraSensor::entityName()) {}\ Let's not add ##CameraSensor, it makes the code more confusing: class FooCameraSensor : CameraSensor { ... }; REGISTER_CAMERA_SENSOR(Foo); I'd rather write class FooCameraSensor : CameraSensor { ... }; REGISTER_CAMERA_SENSOR(FooCameraSensor); and make it possible for the author to name the class without a CameraSensor suffix. > + \ > +private: \ > + CameraSensor *create(const MediaEntity *entity) \ > + { \ > + return new handler##CameraSensor(entity); \ > + } \ > +}; \ > +static handler##CameraSensorFactory global_##handler##CameraSensorFactory > + > } /* 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..21934e72eba7 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -1322,7 +1322,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 68f16f03a81e..f2f054596257 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -901,7 +901,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 fd4df0b03c26..cfeec1aac751 100644 > --- a/src/libcamera/pipeline/vimc.cpp > +++ b/src/libcamera/pipeline/vimc.cpp > @@ -403,7 +403,7 @@ int VimcCameraData::init(MediaDevice *media) > return ret; > > /* Create and open the camera sensor, debayer, scaler and video device. */ > - sensor_ = new CameraSensor(media->getEntityByName("Sensor B")); > + sensor_ = CameraSensorFactory::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..24b83a45b656 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; > diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.cpp b/test/v4l2_videodevice/v4l2_videodevice_test.cpp > index 577da4cb601c..102a8b1b6c1c 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; >
Hi Laurent, On Sat, Feb 08, 2020 at 12:35:59AM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Thu, Feb 06, 2020 at 07:52:43PM +0100, 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 sensor handlers. > > I really like the factory :-) > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/camera_sensor.cpp | 134 ++++++++++++++++++ > > src/libcamera/include/camera_sensor.h | 39 ++++- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +- > > src/libcamera/pipeline/vimc.cpp | 2 +- > > test/camera-sensor.cpp | 2 +- > > .../v4l2_videodevice_test.cpp | 2 +- > > 7 files changed, 177 insertions(+), 6 deletions(-) > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > index 2219a4307436..fc8452b607a0 100644 > > --- a/src/libcamera/camera_sensor.cpp > > +++ b/src/libcamera/camera_sensor.cpp > > @@ -11,7 +11,9 @@ > > #include <float.h> > > #include <iomanip> > > #include <limits.h> > > +#include <map> > > No need to include map here, no function in this file make use of it > directly. The header is included in camera_sensor.h for the usage of > std::map there, that's enough. > > > #include <math.h> > > +#include <string.h> > > > > #include <libcamera/property_ids.h> > > > > @@ -42,6 +44,18 @@ LOG_DEFINE_CATEGORY(CameraSensor); > > * devices as the needs arise. > > */ > > > > +/** > > + * \fn CameraSensor::entityName() > > I wonder, wouldn't it be simpler to pass the entity name string > directly to the REGISTER_CAMERA_SENSOR macro ? It would avoid declaring > and undefined static method in the base class. > > If we ever end up needing more complex matching code, a static match > function that takes a media entity pointer would make sense, but for a > fully static name, this seems a bit overkill. > > > + * \brief Retrieve the name of the entity which identifies the supported sensor > > + * > > + * Camera sensor handlers have to report with this function the name of the > > + * media entity which represents the camera sensor they support. The here > > + * reported name is then matched against the name of the MediaEntity provided > > + * at createSensor() time to identify the correct CameraSensorFactory. > > + * > > + * \return The name of the media entity that identifies the sensor > > + */ > > + > > /** > > * \brief Construct a CameraSensor > > * \param[in] entity The media entity backing the camera sensor > > @@ -366,4 +380,124 @@ std::string CameraSensor::logPrefix() const > > return "'" + subdev_->entity()->name() + "'"; > > } > > > > +/** > > + * \class CameraSensorFactory > > + * \brief Factory of camera sensor handlers > > "camera sensor handlers" or "camera sensors" ? We have pipeline > handlers, and if we could avoid using the name handler here, it could > help reducing confusion. This implies s/sensor handler/sensor below. > > > + * > > + * The class provides to camera sensor handlers the ability to register > > + * themselves 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 > > + * CameraSensorFactory::createInstance() method, which returns a pointer > > + * to the opportune CameraSensor sub-class if any, or a newly created instance > > + * of the generic CameraSensor class. > > + */ > > + > > +LOG_DEFINE_CATEGORY(CameraSensorFactory); > > I think you can use the CameraSensor log category, I don't really see a > need to split messages in two categories here. > > > + > > +/** > > + * \typedef CameraSensorFactory::factoriesMap > > factoriesMap is a type, it should thus be called FactoriesMap. > > > + * \brief An std::map of sensor handler factories > > "A map of entity names to camera sensor factories" > > > + * > > + * The registered sensor handler factories are associated in a map with the > > + * names of the media entity that represent the sensor > > * This map associates the name of the media entities that represent > * camera sensors to the corresponding camera sensor factory. > > But I think we can just drop the documentation as the type can be made > private. > > > + */ > > + > > +/** > > + * \brief Register a camera sensor handler factory > > This is the constructor, so > > * \brief Construct a camera sensor factory > > > + * \param[in] name The name of the media entity representing 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. > > How about being consistent with the pipeline handler factory > documentation ? > > * Creating an instance of the factory registers is with the global list of > * factories, accessible through the factories() function. > > > + */ > > +CameraSensorFactory::CameraSensorFactory(const char *name) > > +{ > > + CameraSensorFactory::registerSensorFactory(name, this); > > No need for the CameraSensorFactory:: prefix. > > > +} > > + > > +/** > > + * \brief Retrieve the list of registered camera sensor factories > > + * > > + * The static factories map is defined inside the function to ensures it gets > > + * initialized on first use, without any dependency on link order. > > + * > > + * \return The static list of registered camera sensor factories > > + */ > > +CameraSensorFactory::factoriesMap &CameraSensorFactory::factories() > > +{ > > + static factoriesMap factories; > > + return factories; > > +} > > + > > +/** > > + * \brief Register a camera sensor handler factory > > + * \param[in] name The name of the media entity that represents the sensor > > + * \param[in] factory The factory instance to register > > + * > > + * The newly registered sensor handler \a factory is associated with \a name, > > + * when a new sensor handler is instantiated with createSensor() the name of > > + * the media entity is matched against the \a name registered here to > > + * retrieve the correct factory. > > Should we explain the uniqueness requirement for the name parameter, as > done in the pipeline handler factory documentation ? > > * The camera sensor \a factory is registered associated with an entity \a name. > * When a camera sensor is created for a media entity with createSensor(), the > * name of the media entity is used to select the corresponding factory. > * > * The caller is responsible for guaranteeing the uniqueness of the > * camera sensor entity name. > > > + */ > > +void CameraSensorFactory::registerSensorFactory(const char *name, > > + CameraSensorFactory *factory) > > Maybe registerFactory() to shorten the name a bit ? > > > +{ > > + factoriesMap &map = CameraSensorFactory::factories(); > > No need for the prefix here either. > > Should we log an error if the name is already present in the map ? > And skip registering the new instance or delete the existing one ? I've gone for the former at the moment. > > + map[name] = factory; > > +} > > + > > +/** > > + * \brief Create and return a CameraSensor > > Maybe "Create a camera sensor corresponding to an entity" ? > > > + * \param[in] entity The media entity that represents the camera sensor > > + * > > + * Create a new instance of an handler for the sensor represented by \a > > + * entity using one of the registered factories. If no specific handler is > > + * available for the sensor, or creating the handler fails, a newly created > > s/, or creating the handler fails// > > as that's not what's implemented :-) > > > + * instance of the generic CameraSensor base class is returned. > > "... fails, an instance of the generic CameraSensor class is created and > returned." > > > + * > > + * Ownership of the created camera sensor instance is passed to the caller > > + * which is reponsible for deleting the instance. > > + * FIXME: is it worth using a smart pointer here ? > > I think it is. Could you give it a try ? > I am now using a unique_ptr<> to return the newly created camera sensor. > > + * > > + * \return A newly created camera sensor handler instance > > + */ > > +CameraSensor *CameraSensorFactory::createSensor(const MediaEntity *entity) > > +{ > > + /* > > + * The entity name contains both the sensor name and the I2C bus ID. > > + * > > + * Remove the i2c bus part and use the sensor name only as key to > > s/i2c/I2C/ > > > + * search for on the sensor handlers map. > > s/on/in/ ? > > > + */ > > + const char *entityName = entity->name().c_str(); > > + const char *delim = strchrnul(entityName, ' '); > > + std::string sensorName(entityName, delim); > > Should you split after the last space, not the first space ? I think the > following would avoid including string.h. > > std::string::size_type delim = entity->name().find_last_of(' '); > std::string name = entity->name().substr(0, delim); > Thanks, but I suspect this is not enough. It only happened with vimc, which registers a "Sensor B" or "Sensor A", but I suspect other drivers might as well register subdevices with spaces in their name. Instead of cutting to the first or last space, I suspect we might need to look for the complete substring, even if it's a broader matching criteria than what we have here. I'll give substring matching a try... > > + > > + auto it = CameraSensorFactory::factories().find(sensorName); > > + if (it == CameraSensorFactory::factories().end()) { > > + LOG(CameraSensorFactory, Info) > > + << "Unsupported sensor '" << entity->name() > > + << "': using generic sensor handler"; > > + return new CameraSensor(entity); > > + } > > + > > + LOG(CameraSensorFactory, Info) << "Create handler for '" > > + << entity->name() << "' sensor"; > > + > > + CameraSensorFactory *factory = it->second; > > + return factory->create(entity); > > +} > > + > > +/** > > + * \def REGISTER_CAMERA_SENSOR(handler) > > + * \brief Register a camera sensor handler to the sensor factory > > s/to the/with the/ > > > + * \param[in] handler The name of the sensor handler > > s/sensor handler/camera sensor class/ > > > + * > > + * Register a camera sensor handler to the sensor factory to make it available > > s/to the/with the/ > > > + * to the factory users. > > + */ > > + > > } /* namespace libcamera */ > > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h > > index 99cff98128dc..2f4a0cc8ad3f 100644 > > --- a/src/libcamera/include/camera_sensor.h > > +++ b/src/libcamera/include/camera_sensor.h > > @@ -7,6 +7,7 @@ > > #ifndef __LIBCAMERA_CAMERA_SENSOR_H__ > > #define __LIBCAMERA_CAMERA_SENSOR_H__ > > > > +#include <map> > > #include <string> > > #include <vector> > > > > @@ -25,7 +26,8 @@ struct V4L2SubdeviceFormat; > > class CameraSensor : protected Loggable > > { > > public: > > - explicit CameraSensor(const MediaEntity *entity); > > + static const char *entityName(); > > + > > ~CameraSensor(); > > > > CameraSensor(const CameraSensor &) = delete; > > @@ -49,6 +51,8 @@ public: > > const ControlList &properties() const { return properties_; } > > > > protected: > > + friend class CameraSensorFactory; > > + explicit CameraSensor(const MediaEntity *entity); > > You can make the constructor private. > > > std::string logPrefix() const; > > > > private: > > @@ -61,6 +65,39 @@ private: > > ControlList properties_; > > }; > > > > +class CameraSensorFactory > > +{ > > +public: > > + using factoriesMap = std::map<const std::string, CameraSensorFactory *>; > > You can make this type private. > > > + > > + CameraSensorFactory(const char *name); > > + virtual ~CameraSensorFactory() {} > > + > > + static CameraSensor *createSensor(const MediaEntity *entity = nullptr); > > Why the default = nullptr ? The argument should be mandatory. > > > + > > +private: > > + static factoriesMap &factories(); > > + static void registerSensorFactory(const char *name, > > + CameraSensorFactory *factory); > > + virtual CameraSensor *create(const MediaEntity *entity) = 0; > > + > > +}; > > + > > +#define REGISTER_CAMERA_SENSOR(handler) \ > > s/handler/sensor/ > > > +class handler##CameraSensorFactory final : public CameraSensorFactory \ > > To shorten lines, > > class sensor##Factory ... > > > +{ \ > > +public: \ > > + handler##CameraSensorFactory() \ > > + : CameraSensorFactory(handler##CameraSensor::entityName()) {}\ > > Let's not add ##CameraSensor, it makes the code more confusing: > > class FooCameraSensor : CameraSensor > { > ... > }; > > REGISTER_CAMERA_SENSOR(Foo); > > I'd rather write > > class FooCameraSensor : CameraSensor > { > ... > }; > > REGISTER_CAMERA_SENSOR(FooCameraSensor); > > and make it possible for the author to name the class without a > CameraSensor suffix. That requires implementations to be a bit more careful, as they should come up with a name for the factory. To me, it seems more natural to write class OV5670 : CameraSensor { } REGISTER_CAMERA_SENSOR(OV5670); Than class OV5670 : CameraSensor { } REGISTER_CAMERA_SENSOR(OV5670Factory); Thanks j > > > + \ > > +private: \ > > + CameraSensor *create(const MediaEntity *entity) \ > > + { \ > > + return new handler##CameraSensor(entity); \ > > + } \ > > +}; \ > > +static handler##CameraSensorFactory global_##handler##CameraSensorFactory > > + > > } /* 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..21934e72eba7 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -1322,7 +1322,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 68f16f03a81e..f2f054596257 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -901,7 +901,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 fd4df0b03c26..cfeec1aac751 100644 > > --- a/src/libcamera/pipeline/vimc.cpp > > +++ b/src/libcamera/pipeline/vimc.cpp > > @@ -403,7 +403,7 @@ int VimcCameraData::init(MediaDevice *media) > > return ret; > > > > /* Create and open the camera sensor, debayer, scaler and video device. */ > > - sensor_ = new CameraSensor(media->getEntityByName("Sensor B")); > > + sensor_ = CameraSensorFactory::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..24b83a45b656 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; > > diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.cpp b/test/v4l2_videodevice/v4l2_videodevice_test.cpp > > index 577da4cb601c..102a8b1b6c1c 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; > > > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Tue, Feb 18, 2020 at 10:29:17AM +0100, Jacopo Mondi wrote: > On Sat, Feb 08, 2020 at 12:35:59AM +0200, Laurent Pinchart wrote: > > On Thu, Feb 06, 2020 at 07:52:43PM +0100, 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 sensor handlers. > > > > I really like the factory :-) > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/libcamera/camera_sensor.cpp | 134 ++++++++++++++++++ > > > src/libcamera/include/camera_sensor.h | 39 ++++- > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +- > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +- > > > src/libcamera/pipeline/vimc.cpp | 2 +- > > > test/camera-sensor.cpp | 2 +- > > > .../v4l2_videodevice_test.cpp | 2 +- > > > 7 files changed, 177 insertions(+), 6 deletions(-) > > > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > > index 2219a4307436..fc8452b607a0 100644 > > > --- a/src/libcamera/camera_sensor.cpp > > > +++ b/src/libcamera/camera_sensor.cpp > > > @@ -11,7 +11,9 @@ > > > #include <float.h> > > > #include <iomanip> > > > #include <limits.h> > > > +#include <map> > > > > No need to include map here, no function in this file make use of it > > directly. The header is included in camera_sensor.h for the usage of > > std::map there, that's enough. > > > > > #include <math.h> > > > +#include <string.h> > > > > > > #include <libcamera/property_ids.h> > > > > > > @@ -42,6 +44,18 @@ LOG_DEFINE_CATEGORY(CameraSensor); > > > * devices as the needs arise. > > > */ > > > > > > +/** > > > + * \fn CameraSensor::entityName() > > > > I wonder, wouldn't it be simpler to pass the entity name string > > directly to the REGISTER_CAMERA_SENSOR macro ? It would avoid declaring > > and undefined static method in the base class. > > > > If we ever end up needing more complex matching code, a static match > > function that takes a media entity pointer would make sense, but for a > > fully static name, this seems a bit overkill. > > > > > + * \brief Retrieve the name of the entity which identifies the supported sensor > > > + * > > > + * Camera sensor handlers have to report with this function the name of the > > > + * media entity which represents the camera sensor they support. The here > > > + * reported name is then matched against the name of the MediaEntity provided > > > + * at createSensor() time to identify the correct CameraSensorFactory. > > > + * > > > + * \return The name of the media entity that identifies the sensor > > > + */ > > > + > > > /** > > > * \brief Construct a CameraSensor > > > * \param[in] entity The media entity backing the camera sensor > > > @@ -366,4 +380,124 @@ std::string CameraSensor::logPrefix() const > > > return "'" + subdev_->entity()->name() + "'"; > > > } > > > > > > +/** > > > + * \class CameraSensorFactory > > > + * \brief Factory of camera sensor handlers > > > > "camera sensor handlers" or "camera sensors" ? We have pipeline > > handlers, and if we could avoid using the name handler here, it could > > help reducing confusion. This implies s/sensor handler/sensor below. > > > > > + * > > > + * The class provides to camera sensor handlers the ability to register > > > + * themselves 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 > > > + * CameraSensorFactory::createInstance() method, which returns a pointer > > > + * to the opportune CameraSensor sub-class if any, or a newly created instance > > > + * of the generic CameraSensor class. > > > + */ > > > + > > > +LOG_DEFINE_CATEGORY(CameraSensorFactory); > > > > I think you can use the CameraSensor log category, I don't really see a > > need to split messages in two categories here. > > > > > + > > > +/** > > > + * \typedef CameraSensorFactory::factoriesMap > > > > factoriesMap is a type, it should thus be called FactoriesMap. > > > > > + * \brief An std::map of sensor handler factories > > > > "A map of entity names to camera sensor factories" > > > > > + * > > > + * The registered sensor handler factories are associated in a map with the > > > + * names of the media entity that represent the sensor > > > > * This map associates the name of the media entities that represent > > * camera sensors to the corresponding camera sensor factory. > > > > But I think we can just drop the documentation as the type can be made > > private. > > > > > + */ > > > + > > > +/** > > > + * \brief Register a camera sensor handler factory > > > > This is the constructor, so > > > > * \brief Construct a camera sensor factory > > > > > + * \param[in] name The name of the media entity representing 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. > > > > How about being consistent with the pipeline handler factory > > documentation ? > > > > * Creating an instance of the factory registers is with the global list of > > * factories, accessible through the factories() function. > > > > > + */ > > > +CameraSensorFactory::CameraSensorFactory(const char *name) > > > +{ > > > + CameraSensorFactory::registerSensorFactory(name, this); > > > > No need for the CameraSensorFactory:: prefix. > > > > > +} > > > + > > > +/** > > > + * \brief Retrieve the list of registered camera sensor factories > > > + * > > > + * The static factories map is defined inside the function to ensures it gets > > > + * initialized on first use, without any dependency on link order. > > > + * > > > + * \return The static list of registered camera sensor factories > > > + */ > > > +CameraSensorFactory::factoriesMap &CameraSensorFactory::factories() > > > +{ > > > + static factoriesMap factories; > > > + return factories; > > > +} > > > + > > > +/** > > > + * \brief Register a camera sensor handler factory > > > + * \param[in] name The name of the media entity that represents the sensor > > > + * \param[in] factory The factory instance to register > > > + * > > > + * The newly registered sensor handler \a factory is associated with \a name, > > > + * when a new sensor handler is instantiated with createSensor() the name of > > > + * the media entity is matched against the \a name registered here to > > > + * retrieve the correct factory. > > > > Should we explain the uniqueness requirement for the name parameter, as > > done in the pipeline handler factory documentation ? > > > > * The camera sensor \a factory is registered associated with an entity \a name. > > * When a camera sensor is created for a media entity with createSensor(), the > > * name of the media entity is used to select the corresponding factory. > > * > > * The caller is responsible for guaranteeing the uniqueness of the > > * camera sensor entity name. > > > > > + */ > > > +void CameraSensorFactory::registerSensorFactory(const char *name, > > > + CameraSensorFactory *factory) > > > > Maybe registerFactory() to shorten the name a bit ? > > > > > +{ > > > + factoriesMap &map = CameraSensorFactory::factories(); > > > > No need for the prefix here either. > > > > Should we log an error if the name is already present in the map ? > > And skip registering the new instance or delete the existing one ? > I've gone for the former at the moment. Either is fine, it shouldn't happen anyway. > > > + map[name] = factory; > > > +} > > > + > > > +/** > > > + * \brief Create and return a CameraSensor > > > > Maybe "Create a camera sensor corresponding to an entity" ? > > > > > + * \param[in] entity The media entity that represents the camera sensor > > > + * > > > + * Create a new instance of an handler for the sensor represented by \a > > > + * entity using one of the registered factories. If no specific handler is > > > + * available for the sensor, or creating the handler fails, a newly created > > > > s/, or creating the handler fails// > > > > as that's not what's implemented :-) > > > > > + * instance of the generic CameraSensor base class is returned. > > > > "... fails, an instance of the generic CameraSensor class is created and > > returned." > > > > > + * > > > + * Ownership of the created camera sensor instance is passed to the caller > > > + * which is reponsible for deleting the instance. > > > + * FIXME: is it worth using a smart pointer here ? > > > > I think it is. Could you give it a try ? > > I am now using a unique_ptr<> to return the newly created camera > sensor. \o/ > > > + * > > > + * \return A newly created camera sensor handler instance > > > + */ > > > +CameraSensor *CameraSensorFactory::createSensor(const MediaEntity *entity) > > > +{ > > > + /* > > > + * The entity name contains both the sensor name and the I2C bus ID. > > > + * > > > + * Remove the i2c bus part and use the sensor name only as key to > > > > s/i2c/I2C/ > > > > > + * search for on the sensor handlers map. > > > > s/on/in/ ? > > > > > + */ > > > + const char *entityName = entity->name().c_str(); > > > + const char *delim = strchrnul(entityName, ' '); > > > + std::string sensorName(entityName, delim); > > > > Should you split after the last space, not the first space ? I think the > > following would avoid including string.h. > > > > std::string::size_type delim = entity->name().find_last_of(' '); > > std::string name = entity->name().substr(0, delim); > > Thanks, but I suspect this is not enough. It only happened with vimc, > which registers a "Sensor B" or "Sensor A", but I suspect other > drivers might as well register subdevices with spaces in their name. > > Instead of cutting to the first or last space, I suspect we might need > to look for the complete substring, even if it's a broader matching > criteria than what we have here. > > I'll give substring matching a try... Good point. https://en.cppreference.com/w/cpp/string/basic_string/starts_with would be useful, but is only available in C++20. Time for utils::string_starts_with ? > > > + > > > + auto it = CameraSensorFactory::factories().find(sensorName); > > > + if (it == CameraSensorFactory::factories().end()) { > > > + LOG(CameraSensorFactory, Info) > > > + << "Unsupported sensor '" << entity->name() > > > + << "': using generic sensor handler"; > > > + return new CameraSensor(entity); > > > + } > > > + > > > + LOG(CameraSensorFactory, Info) << "Create handler for '" > > > + << entity->name() << "' sensor"; > > > + > > > + CameraSensorFactory *factory = it->second; > > > + return factory->create(entity); > > > +} > > > + > > > +/** > > > + * \def REGISTER_CAMERA_SENSOR(handler) > > > + * \brief Register a camera sensor handler to the sensor factory > > > > s/to the/with the/ > > > > > + * \param[in] handler The name of the sensor handler > > > > s/sensor handler/camera sensor class/ > > > > > + * > > > + * Register a camera sensor handler to the sensor factory to make it available > > > > s/to the/with the/ > > > > > + * to the factory users. > > > + */ > > > + > > > } /* namespace libcamera */ > > > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h > > > index 99cff98128dc..2f4a0cc8ad3f 100644 > > > --- a/src/libcamera/include/camera_sensor.h > > > +++ b/src/libcamera/include/camera_sensor.h > > > @@ -7,6 +7,7 @@ > > > #ifndef __LIBCAMERA_CAMERA_SENSOR_H__ > > > #define __LIBCAMERA_CAMERA_SENSOR_H__ > > > > > > +#include <map> > > > #include <string> > > > #include <vector> > > > > > > @@ -25,7 +26,8 @@ struct V4L2SubdeviceFormat; > > > class CameraSensor : protected Loggable > > > { > > > public: > > > - explicit CameraSensor(const MediaEntity *entity); > > > + static const char *entityName(); > > > + > > > ~CameraSensor(); > > > > > > CameraSensor(const CameraSensor &) = delete; > > > @@ -49,6 +51,8 @@ public: > > > const ControlList &properties() const { return properties_; } > > > > > > protected: > > > + friend class CameraSensorFactory; > > > + explicit CameraSensor(const MediaEntity *entity); > > > > You can make the constructor private. > > > > > std::string logPrefix() const; > > > > > > private: > > > @@ -61,6 +65,39 @@ private: > > > ControlList properties_; > > > }; > > > > > > +class CameraSensorFactory > > > +{ > > > +public: > > > + using factoriesMap = std::map<const std::string, CameraSensorFactory *>; > > > > You can make this type private. > > > > > + > > > + CameraSensorFactory(const char *name); > > > + virtual ~CameraSensorFactory() {} > > > + > > > + static CameraSensor *createSensor(const MediaEntity *entity = nullptr); > > > > Why the default = nullptr ? The argument should be mandatory. > > > > > + > > > +private: > > > + static factoriesMap &factories(); > > > + static void registerSensorFactory(const char *name, > > > + CameraSensorFactory *factory); > > > + virtual CameraSensor *create(const MediaEntity *entity) = 0; > > > + > > > +}; > > > + > > > +#define REGISTER_CAMERA_SENSOR(handler) \ > > > > s/handler/sensor/ > > > > > +class handler##CameraSensorFactory final : public CameraSensorFactory \ > > > > To shorten lines, > > > > class sensor##Factory ... > > > > > +{ \ > > > +public: \ > > > + handler##CameraSensorFactory() \ > > > + : CameraSensorFactory(handler##CameraSensor::entityName()) {}\ > > > > Let's not add ##CameraSensor, it makes the code more confusing: > > > > class FooCameraSensor : CameraSensor > > { > > ... > > }; > > > > REGISTER_CAMERA_SENSOR(Foo); > > > > I'd rather write > > > > class FooCameraSensor : CameraSensor > > { > > ... > > }; > > > > REGISTER_CAMERA_SENSOR(FooCameraSensor); > > > > and make it possible for the author to name the class without a > > CameraSensor suffix. > > That requires implementations to be a bit more careful, as they should > come up with a name for the factory. To me, it seems more natural to > write > > class OV5670 : CameraSensor > { > } > > REGISTER_CAMERA_SENSOR(OV5670); > > Than > > class OV5670 : CameraSensor > { > } > > REGISTER_CAMERA_SENSOR(OV5670Factory); I haven't expressed myself very clearly, sorry about that. I didn't mean removing the ##Factory part, but the ##CameraSensor in : CameraSensorFactory(handler##CameraSensor::entityName()) {} We should end up writing class OV5670 : CameraSensor { ... }; REGISTER_CAMERA_SENSOR(OV5670); while I think your patch required class OV5670CameraSensor : CameraSensor { ... }; REGISTER_CAMERA_SENSOR(OV5670); > > > + \ > > > +private: \ > > > + CameraSensor *create(const MediaEntity *entity) \ > > > + { \ > > > + return new handler##CameraSensor(entity); \ > > > + } \ > > > +}; \ > > > +static handler##CameraSensorFactory global_##handler##CameraSensorFactory > > > + > > > } /* 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..21934e72eba7 100644 > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > @@ -1322,7 +1322,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 68f16f03a81e..f2f054596257 100644 > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > @@ -901,7 +901,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 fd4df0b03c26..cfeec1aac751 100644 > > > --- a/src/libcamera/pipeline/vimc.cpp > > > +++ b/src/libcamera/pipeline/vimc.cpp > > > @@ -403,7 +403,7 @@ int VimcCameraData::init(MediaDevice *media) > > > return ret; > > > > > > /* Create and open the camera sensor, debayer, scaler and video device. */ > > > - sensor_ = new CameraSensor(media->getEntityByName("Sensor B")); > > > + sensor_ = CameraSensorFactory::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..24b83a45b656 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; > > > diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.cpp b/test/v4l2_videodevice/v4l2_videodevice_test.cpp > > > index 577da4cb601c..102a8b1b6c1c 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; > > >
Hi Laurent, On Tue, Feb 18, 2020 at 11:35:11AM +0200, Laurent Pinchart wrote: [snip] > > > > + */ > > > > + const char *entityName = entity->name().c_str(); > > > > + const char *delim = strchrnul(entityName, ' '); > > > > + std::string sensorName(entityName, delim); > > > > > > Should you split after the last space, not the first space ? I think the > > > following would avoid including string.h. > > > > > > std::string::size_type delim = entity->name().find_last_of(' '); > > > std::string name = entity->name().substr(0, delim); > > > > Thanks, but I suspect this is not enough. It only happened with vimc, > > which registers a "Sensor B" or "Sensor A", but I suspect other > > drivers might as well register subdevices with spaces in their name. > > > > Instead of cutting to the first or last space, I suspect we might need > > to look for the complete substring, even if it's a broader matching > > criteria than what we have here. > > > > I'll give substring matching a try... > > Good point. > > https://en.cppreference.com/w/cpp/string/basic_string/starts_with would > be useful, but is only available in C++20. Time for > utils::string_starts_with ? > I've considered that, but at the same time it means we're now looking for the 'key' (provided by the camera sensor at registration time) in the 'name' (the name of the entity for which we're creating a camera sensor). So to me it seems enough to look for the substring with std::string::find(). Also, and this should not happen, I know, the fact that the entiy names are composed as "$sensorname $i2cbus::$i2caddr" is a convention enforced by the driver usage of "v4l2_i2c_subdev_set_name()" which is called by the canonical "v4l2_i2c_subdev_init()". I know upstream and well behaved drivers should gurantee their names to be assembled with those functions as snprintf(sd->name, sizeof(sd->name), "%s%s %d-%04x", devname, postfix, i2c_adapter_id(client->adapter), client->addr); but that's not guaranteed for downstream users. I know we can't support any crazy BSP hack out there, and we don't want to, but in this case enforcing that sensor name has that specific format in the matching criteria, would tie hands to the ones who want to support custom drivers using custom names with a custom camera sensor subclass. I would then just look for the key in the sensor name, whatever format it has. It will work for us, and support sensor drivers with more exotic naming schemes.. > > > > + > > > > + auto it = CameraSensorFactory::factories().find(sensorName); > > > > + if (it == CameraSensorFactory::factories().end()) { > > > > + LOG(CameraSensorFactory, Info) > > > > + << "Unsupported sensor '" << entity->name() > > > > + << "': using generic sensor handler"; > > > > + return new CameraSensor(entity); > > > > + } > > > > + > > > > + LOG(CameraSensorFactory, Info) << "Create handler for '" > > > > + << entity->name() << "' sensor"; > > > > + > > > > + CameraSensorFactory *factory = it->second; > > > > + return factory->create(entity); > > > > +} > > > > + [snip] > > > > > > Let's not add ##CameraSensor, it makes the code more confusing: > > > > > > class FooCameraSensor : CameraSensor > > > { > > > ... > > > }; > > > > > > REGISTER_CAMERA_SENSOR(Foo); > > > > > > I'd rather write > > > > > > class FooCameraSensor : CameraSensor > > > { > > > ... > > > }; > > > > > > REGISTER_CAMERA_SENSOR(FooCameraSensor); > > > > > > and make it possible for the author to name the class without a > > > CameraSensor suffix. > > > > That requires implementations to be a bit more careful, as they should > > come up with a name for the factory. To me, it seems more natural to > > write > > > > class OV5670 : CameraSensor > > { > > } > > > > REGISTER_CAMERA_SENSOR(OV5670); > > > > Than > > > > class OV5670 : CameraSensor > > { > > } > > > > REGISTER_CAMERA_SENSOR(OV5670Factory); > > I haven't expressed myself very clearly, sorry about that. I didn't mean > removing the ##Factory part, but the ##CameraSensor in > > : CameraSensorFactory(handler##CameraSensor::entityName()) {} > > We should end up writing > > class OV5670 : CameraSensor > { > ... > }; > > REGISTER_CAMERA_SENSOR(OV5670); > > while I think your patch required > > class OV5670CameraSensor : CameraSensor > { > ... > }; > > REGISTER_CAMERA_SENSOR(OV5670); > Correct. Sorry, I missed what you meant. I agree though, and now we use OV5670 both as class name and as registration key. Thanks j
diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp index 2219a4307436..fc8452b607a0 100644 --- a/src/libcamera/camera_sensor.cpp +++ b/src/libcamera/camera_sensor.cpp @@ -11,7 +11,9 @@ #include <float.h> #include <iomanip> #include <limits.h> +#include <map> #include <math.h> +#include <string.h> #include <libcamera/property_ids.h> @@ -42,6 +44,18 @@ LOG_DEFINE_CATEGORY(CameraSensor); * devices as the needs arise. */ +/** + * \fn CameraSensor::entityName() + * \brief Retrieve the name of the entity which identifies the supported sensor + * + * Camera sensor handlers have to report with this function the name of the + * media entity which represents the camera sensor they support. The here + * reported name is then matched against the name of the MediaEntity provided + * at createSensor() time to identify the correct CameraSensorFactory. + * + * \return The name of the media entity that identifies the sensor + */ + /** * \brief Construct a CameraSensor * \param[in] entity The media entity backing the camera sensor @@ -366,4 +380,124 @@ std::string CameraSensor::logPrefix() const return "'" + subdev_->entity()->name() + "'"; } +/** + * \class CameraSensorFactory + * \brief Factory of camera sensor handlers + * + * The class provides to camera sensor handlers the ability to register + * themselves 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 + * CameraSensorFactory::createInstance() method, which returns a pointer + * to the opportune CameraSensor sub-class if any, or a newly created instance + * of the generic CameraSensor class. + */ + +LOG_DEFINE_CATEGORY(CameraSensorFactory); + +/** + * \typedef CameraSensorFactory::factoriesMap + * \brief An std::map of sensor handler factories + * + * The registered sensor handler factories are associated in a map with the + * names of the media entity that represent the sensor + */ + +/** + * \brief Register a camera sensor handler factory + * \param[in] name The name of the media entity representing 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. + */ +CameraSensorFactory::CameraSensorFactory(const char *name) +{ + CameraSensorFactory::registerSensorFactory(name, this); +} + +/** + * \brief Retrieve the list of registered camera sensor factories + * + * The static factories map is defined inside the function to ensures it gets + * initialized on first use, without any dependency on link order. + * + * \return The static list of registered camera sensor factories + */ +CameraSensorFactory::factoriesMap &CameraSensorFactory::factories() +{ + static factoriesMap factories; + return factories; +} + +/** + * \brief Register a camera sensor handler factory + * \param[in] name The name of the media entity that represents the sensor + * \param[in] factory The factory instance to register + * + * The newly registered sensor handler \a factory is associated with \a name, + * when a new sensor handler is instantiated with createSensor() the name of + * the media entity is matched against the \a name registered here to + * retrieve the correct factory. + */ +void CameraSensorFactory::registerSensorFactory(const char *name, + CameraSensorFactory *factory) +{ + factoriesMap &map = CameraSensorFactory::factories(); + map[name] = factory; +} + +/** + * \brief Create and return a CameraSensor + * \param[in] entity The media entity that represents the camera sensor + * + * Create a new instance of an handler for the sensor represented by \a + * entity using one of the registered factories. If no specific handler is + * available for the sensor, or creating the handler fails, a newly created + * instance of the generic CameraSensor base class is returned. + * + * Ownership of the created camera sensor instance is passed to the caller + * which is reponsible for deleting the instance. + * FIXME: is it worth using a smart pointer here ? + * + * \return A newly created camera sensor handler instance + */ +CameraSensor *CameraSensorFactory::createSensor(const MediaEntity *entity) +{ + /* + * The entity name contains both the sensor name and the I2C bus ID. + * + * Remove the i2c bus part and use the sensor name only as key to + * search for on the sensor handlers map. + */ + const char *entityName = entity->name().c_str(); + const char *delim = strchrnul(entityName, ' '); + std::string sensorName(entityName, delim); + + auto it = CameraSensorFactory::factories().find(sensorName); + if (it == CameraSensorFactory::factories().end()) { + LOG(CameraSensorFactory, Info) + << "Unsupported sensor '" << entity->name() + << "': using generic sensor handler"; + return new CameraSensor(entity); + } + + LOG(CameraSensorFactory, Info) << "Create handler for '" + << entity->name() << "' sensor"; + + CameraSensorFactory *factory = it->second; + return factory->create(entity); +} + +/** + * \def REGISTER_CAMERA_SENSOR(handler) + * \brief Register a camera sensor handler to the sensor factory + * \param[in] handler The name of the sensor handler + * + * Register a camera sensor handler to the sensor factory to make it available + * to the factory users. + */ + } /* namespace libcamera */ diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h index 99cff98128dc..2f4a0cc8ad3f 100644 --- a/src/libcamera/include/camera_sensor.h +++ b/src/libcamera/include/camera_sensor.h @@ -7,6 +7,7 @@ #ifndef __LIBCAMERA_CAMERA_SENSOR_H__ #define __LIBCAMERA_CAMERA_SENSOR_H__ +#include <map> #include <string> #include <vector> @@ -25,7 +26,8 @@ struct V4L2SubdeviceFormat; class CameraSensor : protected Loggable { public: - explicit CameraSensor(const MediaEntity *entity); + static const char *entityName(); + ~CameraSensor(); CameraSensor(const CameraSensor &) = delete; @@ -49,6 +51,8 @@ public: const ControlList &properties() const { return properties_; } protected: + friend class CameraSensorFactory; + explicit CameraSensor(const MediaEntity *entity); std::string logPrefix() const; private: @@ -61,6 +65,39 @@ private: ControlList properties_; }; +class CameraSensorFactory +{ +public: + using factoriesMap = std::map<const std::string, CameraSensorFactory *>; + + CameraSensorFactory(const char *name); + virtual ~CameraSensorFactory() {} + + static CameraSensor *createSensor(const MediaEntity *entity = nullptr); + +private: + static factoriesMap &factories(); + static void registerSensorFactory(const char *name, + CameraSensorFactory *factory); + virtual CameraSensor *create(const MediaEntity *entity) = 0; + +}; + +#define REGISTER_CAMERA_SENSOR(handler) \ +class handler##CameraSensorFactory final : public CameraSensorFactory \ +{ \ +public: \ + handler##CameraSensorFactory() \ + : CameraSensorFactory(handler##CameraSensor::entityName()) {}\ + \ +private: \ + CameraSensor *create(const MediaEntity *entity) \ + { \ + return new handler##CameraSensor(entity); \ + } \ +}; \ +static handler##CameraSensorFactory global_##handler##CameraSensorFactory + } /* 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..21934e72eba7 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -1322,7 +1322,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 68f16f03a81e..f2f054596257 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -901,7 +901,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 fd4df0b03c26..cfeec1aac751 100644 --- a/src/libcamera/pipeline/vimc.cpp +++ b/src/libcamera/pipeline/vimc.cpp @@ -403,7 +403,7 @@ int VimcCameraData::init(MediaDevice *media) return ret; /* Create and open the camera sensor, debayer, scaler and video device. */ - sensor_ = new CameraSensor(media->getEntityByName("Sensor B")); + sensor_ = CameraSensorFactory::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..24b83a45b656 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; diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.cpp b/test/v4l2_videodevice/v4l2_videodevice_test.cpp index 577da4cb601c..102a8b1b6c1c 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;
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> --- src/libcamera/camera_sensor.cpp | 134 ++++++++++++++++++ src/libcamera/include/camera_sensor.h | 39 ++++- src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +- src/libcamera/pipeline/vimc.cpp | 2 +- test/camera-sensor.cpp | 2 +- .../v4l2_videodevice_test.cpp | 2 +- 7 files changed, 177 insertions(+), 6 deletions(-)