[libcamera-devel] [PATCH v2 3/7] libcamera: camera_sensor: Introduce CameraSensorFactory
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Feb 18 10:35:11 CET 2020
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 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.
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;
> > >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list