[libcamera-devel] [PATCH v2 3/7] libcamera: camera_sensor: Introduce CameraSensorFactory
Jacopo Mondi
jacopo at jmondi.org
Tue Feb 18 10:29:17 CET 2020
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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20200218/eccab19a/attachment.sig>
More information about the libcamera-devel
mailing list