[{"id":3637,"web_url":"https://patchwork.libcamera.org/comment/3637/","msgid":"<20200207223559.GE4726@pendragon.ideasonboard.com>","date":"2020-02-07T22:35:59","subject":"Re: [libcamera-devel] [PATCH v2 3/7] libcamera: camera_sensor:\n\tIntroduce CameraSensorFactory","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Thu, Feb 06, 2020 at 07:52:43PM +0100, Jacopo Mondi wrote:\n> Introduce a factory to create CameraSensor derived classes instances by\n> inspecting the sensor media entity name and provide a convenience macro\n> to register specialized sensor handlers.\n\nI really like the factory :-)\n\n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/camera_sensor.cpp | 134 ++++++++++++++++++\n> src/libcamera/include/camera_sensor.h | 39 ++++-\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-\n> src/libcamera/pipeline/vimc.cpp | 2 +-\n> test/camera-sensor.cpp | 2 +-\n> .../v4l2_videodevice_test.cpp | 2 +-\n> 7 files changed, 177 insertions(+), 6 deletions(-)\n> \n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index 2219a4307436..fc8452b607a0 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -11,7 +11,9 @@\n> #include \n> #include \n> #include \n> +#include \n\nNo need to include map here, no function in this file make use of it\ndirectly. The header is included in camera_sensor.h for the usage of\nstd::map there, that's enough.\n\n> #include \n> +#include \n> \n> #include \n> \n> @@ -42,6 +44,18 @@ LOG_DEFINE_CATEGORY(CameraSensor);\n> * devices as the needs arise.\n> */\n> \n> +/**\n> + * \\fn CameraSensor::entityName()\n\nI wonder, wouldn't it be simpler to pass the entity name string\ndirectly to the REGISTER_CAMERA_SENSOR macro ? It would avoid declaring\nand undefined static method in the base class.\n\nIf we ever end up needing more complex matching code, a static match\nfunction that takes a media entity pointer would make sense, but for a\nfully static name, this seems a bit overkill.\n\n> + * \\brief Retrieve the name of the entity which identifies the supported sensor\n> + *\n> + * Camera sensor handlers have to report with this function the name of the\n> + * media entity which represents the camera sensor they support. The here\n> + * reported name is then matched against the name of the MediaEntity provided\n> + * at createSensor() time to identify the correct CameraSensorFactory.\n> + *\n> + * \\return The name of the media entity that identifies the sensor\n> + */\n> +\n> /**\n> * \\brief Construct a CameraSensor\n> * \\param[in] entity The media entity backing the camera sensor\n> @@ -366,4 +380,124 @@ std::string CameraSensor::logPrefix() const\n> \treturn \"'\" + subdev_->entity()->name() + \"'\";\n> }\n> \n> +/**\n> + * \\class CameraSensorFactory\n> + * \\brief Factory of camera sensor handlers\n\n\"camera sensor handlers\" or \"camera sensors\" ? We have pipeline\nhandlers, and if we could avoid using the name handler here, it could\nhelp reducing confusion. This implies s/sensor handler/sensor below.\n\n> + *\n> + * The class provides to camera sensor handlers the ability to register\n> + * themselves to the factory to allow the creation of their instances by\n> + * matching the name of the media entity which represents the sensor.\n> + *\n> + * Camera sensor handlers use the REGISTER_CAMERA_SENSOR() macro to\n> + * add themselves to the camera sensor factory. Users of the factory\n> + * creates camera sensor handler instances by using the static\n> + * CameraSensorFactory::createInstance() method, which returns a pointer\n> + * to the opportune CameraSensor sub-class if any, or a newly created instance\n> + * of the generic CameraSensor class.\n> + */\n> +\n> +LOG_DEFINE_CATEGORY(CameraSensorFactory);\n\nI think you can use the CameraSensor log category, I don't really see a\nneed to split messages in two categories here.\n\n> +\n> +/**\n> + * \\typedef CameraSensorFactory::factoriesMap\n\nfactoriesMap is a type, it should thus be called FactoriesMap.\n\n> + * \\brief An std::map of sensor handler factories\n\n\"A map of entity names to camera sensor factories\"\n\n> + *\n> + * The registered sensor handler factories are associated in a map with the\n> + * names of the media entity that represent the sensor\n\n * This map associates the name of the media entities that represent\n * camera sensors to the corresponding camera sensor factory.\n\nBut I think we can just drop the documentation as the type can be made\nprivate.\n\n> + */\n> +\n> +/**\n> + * \\brief Register a camera sensor handler factory\n\nThis is the constructor, so\n\n * \\brief Construct a camera sensor factory\n\n> + * \\param[in] name The name of the media entity representing the sensor\n> + *\n> + * Register a new camera sensor factory which creates sensor handler instances\n> + * that support camera sensors represented by the media entity with \\a name.\n\nHow about being consistent with the pipeline handler factory\ndocumentation ?\n\n * Creating an instance of the factory registers is with the global list of\n * factories, accessible through the factories() function.\n\n> + */\n> +CameraSensorFactory::CameraSensorFactory(const char *name)\n> +{\n> +\tCameraSensorFactory::registerSensorFactory(name, this);\n\nNo need for the CameraSensorFactory:: prefix.\n\n> +}\n> +\n> +/**\n> + * \\brief Retrieve the list of registered camera sensor factories\n> + *\n> + * The static factories map is defined inside the function to ensures it gets\n> + * initialized on first use, without any dependency on link order.\n> + *\n> + * \\return The static list of registered camera sensor factories\n> + */\n> +CameraSensorFactory::factoriesMap &CameraSensorFactory::factories()\n> +{\n> +\tstatic factoriesMap factories;\n> +\treturn factories;\n> +}\n> +\n> +/**\n> + * \\brief Register a camera sensor handler factory\n> + * \\param[in] name The name of the media entity that represents the sensor\n> + * \\param[in] factory The factory instance to register\n> + *\n> + * The newly registered sensor handler \\a factory is associated with \\a name,\n> + * when a new sensor handler is instantiated with createSensor() the name of\n> + * the media entity is matched against the \\a name registered here to\n> + * retrieve the correct factory.\n\nShould we explain the uniqueness requirement for the name parameter, as\ndone in the pipeline handler factory documentation ?\n\n * The camera sensor \\a factory is registered associated with an entity \\a name.\n * When a camera sensor is created for a media entity with createSensor(), the\n * name of the media entity is used to select the corresponding factory.\n *\n * The caller is responsible for guaranteeing the uniqueness of the\n * camera sensor entity name.\n\n> + */\n> +void CameraSensorFactory::registerSensorFactory(const char *name,\n> +\t\t\t\t\t\tCameraSensorFactory *factory)\n\nMaybe registerFactory() to shorten the name a bit ?\n\n> +{\n> +\tfactoriesMap &map = CameraSensorFactory::factories();\n\nNo need for the prefix here either.\n\nShould we log an error if the name is already present in the map ?\n\n> +\tmap[name] = factory;\n> +}\n> +\n> +/**\n> + * \\brief Create and return a CameraSensor\n\nMaybe \"Create a camera sensor corresponding to an entity\" ?\n\n> + * \\param[in] entity The media entity that represents the camera sensor\n> + *\n> + * Create a new instance of an handler for the sensor represented by \\a\n> + * entity using one of the registered factories. If no specific handler is\n> + * available for the sensor, or creating the handler fails, a newly created\n\ns/, or creating the handler fails//\n\nas that's not what's implemented :-)\n\n> + * instance of the generic CameraSensor base class is returned.\n\n\"... fails, an instance of the generic CameraSensor class is created and\nreturned.\"\n\n> + *\n> + * Ownership of the created camera sensor instance is passed to the caller\n> + * which is reponsible for deleting the instance.\n> + * FIXME: is it worth using a smart pointer here ?\n\nI think it is. Could you give it a try ?\n\n> + *\n> + * \\return A newly created camera sensor handler instance\n> + */\n> +CameraSensor *CameraSensorFactory::createSensor(const MediaEntity *entity)\n> +{\n> +\t/*\n> +\t * The entity name contains both the sensor name and the I2C bus ID.\n> +\t *\n> +\t * Remove the i2c bus part and use the sensor name only as key to\n\ns/i2c/I2C/\n\n> +\t * search for on the sensor handlers map.\n\ns/on/in/ ?\n\n> +\t */\n> +\tconst char *entityName = entity->name().c_str();\n> +\tconst char *delim = strchrnul(entityName, ' ');\n> +\tstd::string sensorName(entityName, delim);\n\nShould you split after the last space, not the first space ? I think the\nfollowing would avoid including string.h.\n\n\tstd::string::size_type delim = entity->name().find_last_of(' ');\n\tstd::string name = entity->name().substr(0, delim);\n\n> +\n> +\tauto it = CameraSensorFactory::factories().find(sensorName);\n> +\tif (it == CameraSensorFactory::factories().end()) {\n> +\t\tLOG(CameraSensorFactory, Info)\n> +\t\t\t<< \"Unsupported sensor '\" << entity->name()\n> +\t\t\t<< \"': using generic sensor handler\";\n> +\t\treturn new CameraSensor(entity);\n> +\t}\n> +\n> +\tLOG(CameraSensorFactory, Info) << \"Create handler for '\"\n> +\t\t\t\t << entity->name() << \"' sensor\";\n> +\n> +\tCameraSensorFactory *factory = it->second;\n> +\treturn factory->create(entity);\n> +}\n> +\n> +/**\n> + * \\def REGISTER_CAMERA_SENSOR(handler)\n> + * \\brief Register a camera sensor handler to the sensor factory\n\ns/to the/with the/\n\n> + * \\param[in] handler The name of the sensor handler\n\ns/sensor handler/camera sensor class/\n\n> + *\n> + * Register a camera sensor handler to the sensor factory to make it available\n\ns/to the/with the/\n\n> + * to the factory users.\n> + */\n> +\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h\n> index 99cff98128dc..2f4a0cc8ad3f 100644\n> --- a/src/libcamera/include/camera_sensor.h\n> +++ b/src/libcamera/include/camera_sensor.h\n> @@ -7,6 +7,7 @@\n> #ifndef __LIBCAMERA_CAMERA_SENSOR_H__\n> #define __LIBCAMERA_CAMERA_SENSOR_H__\n> \n> +#include \n> #include \n> #include \n> \n> @@ -25,7 +26,8 @@ struct V4L2SubdeviceFormat;\n> class CameraSensor : protected Loggable\n> {\n> public:\n> -\texplicit CameraSensor(const MediaEntity *entity);\n> +\tstatic const char *entityName();\n> +\n> \t~CameraSensor();\n> \n> \tCameraSensor(const CameraSensor &) = delete;\n> @@ -49,6 +51,8 @@ public:\n> \tconst ControlList &properties() const { return properties_; }\n> \n> protected:\n> +\tfriend class CameraSensorFactory;\n> +\texplicit CameraSensor(const MediaEntity *entity);\n\nYou can make the constructor private.\n\n> \tstd::string logPrefix() const;\n> \n> private:\n> @@ -61,6 +65,39 @@ private:\n> \tControlList properties_;\n> };\n> \n> +class CameraSensorFactory\n> +{\n> +public:\n> +\tusing factoriesMap = std::map;\n\nYou can make this type private.\n\n> +\n> +\tCameraSensorFactory(const char *name);\n> +\tvirtual ~CameraSensorFactory() {}\n> +\n> +\tstatic CameraSensor *createSensor(const MediaEntity *entity = nullptr);\n\nWhy the default = nullptr ? The argument should be mandatory.\n\n> +\n> +private:\n> +\tstatic factoriesMap &factories();\n> +\tstatic void registerSensorFactory(const char *name,\n> +\t\t\t\t\t CameraSensorFactory *factory);\n> +\tvirtual CameraSensor *create(const MediaEntity *entity) = 0;\n> +\n> +};\n> +\n> +#define REGISTER_CAMERA_SENSOR(handler)\t\t\t\t\t\\\n\ns/handler/sensor/\n\n> +class handler##CameraSensorFactory final : public CameraSensorFactory\t\\\n\nTo shorten lines,\n\nclass sensor##Factory ...\n\n> +{\t\t\t\t\t\t\t\t\t\\\n> +public:\t\t\t\t\t\t\t\t\t\\\n> +\thandler##CameraSensorFactory()\t\t\t\t\t\\\n> +\t\t: CameraSensorFactory(handler##CameraSensor::entityName()) {}\\\n\nLet's not add ##CameraSensor, it makes the code more confusing:\n\nclass FooCameraSensor : CameraSensor\n{\n\t...\n};\n\nREGISTER_CAMERA_SENSOR(Foo);\n\nI'd rather write\n\nclass FooCameraSensor : CameraSensor\n{\n\t...\n};\n\nREGISTER_CAMERA_SENSOR(FooCameraSensor);\n\nand make it possible for the author to name the class without a\nCameraSensor suffix.\n\n> +\t\t\t\t\t\t\t\t\t\\\n> +private:\t\t\t\t\t\t\t\t\\\n> +\tCameraSensor *create(const MediaEntity *entity)\t\t\t\\\n> +\t{\t\t\t\t\t\t\t\t\\\n> +\t\treturn new handler##CameraSensor(entity);\t\t\\\n> +\t}\t\t\t\t\t\t\t\t\\\n> +};\t\t\t\t\t\t\t\t\t\\\n> +static handler##CameraSensorFactory global_##handler##CameraSensorFactory\n> +\n> } /* namespace libcamera */\n> \n> #endif /* __LIBCAMERA_CAMERA_SENSOR_H__ */\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 387bb070b505..21934e72eba7 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -1322,7 +1322,7 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)\n> \n> \tMediaLink *link = links[0];\n> \tMediaEntity *sensorEntity = link->source()->entity();\n> -\tsensor_ = new CameraSensor(sensorEntity);\n> +\tsensor_ = CameraSensorFactory::createSensor(sensorEntity);\n> \tret = sensor_->init();\n> \tif (ret)\n> \t\treturn ret;\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 68f16f03a81e..f2f054596257 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -901,7 +901,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> \n> \tdata->controlInfo_ = std::move(ctrls);\n> \n> -\tdata->sensor_ = new CameraSensor(sensor);\n> +\tdata->sensor_ = CameraSensorFactory::createSensor(sensor);\n> \tret = data->sensor_->init();\n> \tif (ret)\n> \t\treturn ret;\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index fd4df0b03c26..cfeec1aac751 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -403,7 +403,7 @@ int VimcCameraData::init(MediaDevice *media)\n> \t\treturn ret;\n> \n> \t/* Create and open the camera sensor, debayer, scaler and video device. */\n> -\tsensor_ = new CameraSensor(media->getEntityByName(\"Sensor B\"));\n> +\tsensor_ = CameraSensorFactory::createSensor(media->getEntityByName(\"Sensor B\"));\n> \tret = sensor_->init();\n> \tif (ret)\n> \t\treturn ret;\n> diff --git a/test/camera-sensor.cpp b/test/camera-sensor.cpp\n> index 27c190fe7ace..24b83a45b656 100644\n> --- a/test/camera-sensor.cpp\n> +++ b/test/camera-sensor.cpp\n> @@ -50,7 +50,7 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> -\t\tsensor_ = new CameraSensor(entity);\n> +\t\tsensor_ = CameraSensorFactory::createSensor(entity);\n> \t\tif (sensor_->init() < 0) {\n> \t\t\tcerr << \"Unable to initialise camera sensor\" << endl;\n> \t\t\treturn TestFail;\n> diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.cpp b/test/v4l2_videodevice/v4l2_videodevice_test.cpp\n> index 577da4cb601c..102a8b1b6c1c 100644\n> --- a/test/v4l2_videodevice/v4l2_videodevice_test.cpp\n> +++ b/test/v4l2_videodevice/v4l2_videodevice_test.cpp\n> @@ -61,7 +61,7 @@ int V4L2VideoDeviceTest::init()\n> \t\treturn TestFail;\n> \n> \tif (driver_ == \"vimc\") {\n> -\t\tsensor_ = new CameraSensor(media_->getEntityByName(\"Sensor A\"));\n> +\t\tsensor_ = CameraSensorFactory::createSensor(media_->getEntityByName(\"Sensor A\"));\n> \t\tif (sensor_->init())\n> \t\t\treturn TestFail;\n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 460EF60445\n\tfor ;\n\tFri, 7 Feb 2020 23:36:16 +0100 (CET)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 645799F3;\n\tFri, 7 Feb 2020 23:36:15 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1581114975;\n\tbh=TOMa7a0Y/JohLri2Qps5V6/hP0AqiHjgnOK4w5dojgU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PjDgCuGe+bEYBzioFxf3hbuncsIseRbq9VopSxEAZTXRlCl0XAziAfEeYTvfyH0N1\n\t1aG5fYB8pWR2wLP5WVTieJUHT6hPgPM3zJ+c6y0rBGcXK4IHOXO9tWkdVH5J6W3XEF\n\tD0ZCJDu+PV4YJsY1QjzXRMaqf+f7M16J0WSOavUk=","Date":"Sat, 8 Feb 2020 00:35:59 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200207223559.GE4726@pendragon.ideasonboard.com>","References":"<20200206185247.202233-1-jacopo@jmondi.org>\n\t<20200206185247.202233-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200206185247.202233-4-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 3/7] libcamera: camera_sensor:\n\tIntroduce CameraSensorFactory","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 07 Feb 2020 22:36:16 -0000"}},{"id":3801,"web_url":"https://patchwork.libcamera.org/comment/3801/","msgid":"<20200218092917.6bh3m3w7hhywhojf@uno.localdomain>","date":"2020-02-18T09:29:17","subject":"Re: [libcamera-devel] [PATCH v2 3/7] libcamera: camera_sensor:\n\tIntroduce CameraSensorFactory","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sat, Feb 08, 2020 at 12:35:59AM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Thu, Feb 06, 2020 at 07:52:43PM +0100, Jacopo Mondi wrote:\n> > Introduce a factory to create CameraSensor derived classes instances by\n> > inspecting the sensor media entity name and provide a convenience macro\n> > to register specialized sensor handlers.\n>\n> I really like the factory :-)\n>\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/camera_sensor.cpp | 134 ++++++++++++++++++\n> > src/libcamera/include/camera_sensor.h | 39 ++++-\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-\n> > src/libcamera/pipeline/vimc.cpp | 2 +-\n> > test/camera-sensor.cpp | 2 +-\n> > .../v4l2_videodevice_test.cpp | 2 +-\n> > 7 files changed, 177 insertions(+), 6 deletions(-)\n> >\n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index 2219a4307436..fc8452b607a0 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -11,7 +11,9 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n>\n> No need to include map here, no function in this file make use of it\n> directly. The header is included in camera_sensor.h for the usage of\n> std::map there, that's enough.\n>\n> > #include \n> > +#include \n> >\n> > #include \n> >\n> > @@ -42,6 +44,18 @@ LOG_DEFINE_CATEGORY(CameraSensor);\n> > * devices as the needs arise.\n> > */\n> >\n> > +/**\n> > + * \\fn CameraSensor::entityName()\n>\n> I wonder, wouldn't it be simpler to pass the entity name string\n> directly to the REGISTER_CAMERA_SENSOR macro ? It would avoid declaring\n> and undefined static method in the base class.\n>\n> If we ever end up needing more complex matching code, a static match\n> function that takes a media entity pointer would make sense, but for a\n> fully static name, this seems a bit overkill.\n>\n> > + * \\brief Retrieve the name of the entity which identifies the supported sensor\n> > + *\n> > + * Camera sensor handlers have to report with this function the name of the\n> > + * media entity which represents the camera sensor they support. The here\n> > + * reported name is then matched against the name of the MediaEntity provided\n> > + * at createSensor() time to identify the correct CameraSensorFactory.\n> > + *\n> > + * \\return The name of the media entity that identifies the sensor\n> > + */\n> > +\n> > /**\n> > * \\brief Construct a CameraSensor\n> > * \\param[in] entity The media entity backing the camera sensor\n> > @@ -366,4 +380,124 @@ std::string CameraSensor::logPrefix() const\n> > \treturn \"'\" + subdev_->entity()->name() + \"'\";\n> > }\n> >\n> > +/**\n> > + * \\class CameraSensorFactory\n> > + * \\brief Factory of camera sensor handlers\n>\n> \"camera sensor handlers\" or \"camera sensors\" ? We have pipeline\n> handlers, and if we could avoid using the name handler here, it could\n> help reducing confusion. This implies s/sensor handler/sensor below.\n>\n> > + *\n> > + * The class provides to camera sensor handlers the ability to register\n> > + * themselves to the factory to allow the creation of their instances by\n> > + * matching the name of the media entity which represents the sensor.\n> > + *\n> > + * Camera sensor handlers use the REGISTER_CAMERA_SENSOR() macro to\n> > + * add themselves to the camera sensor factory. Users of the factory\n> > + * creates camera sensor handler instances by using the static\n> > + * CameraSensorFactory::createInstance() method, which returns a pointer\n> > + * to the opportune CameraSensor sub-class if any, or a newly created instance\n> > + * of the generic CameraSensor class.\n> > + */\n> > +\n> > +LOG_DEFINE_CATEGORY(CameraSensorFactory);\n>\n> I think you can use the CameraSensor log category, I don't really see a\n> need to split messages in two categories here.\n>\n> > +\n> > +/**\n> > + * \\typedef CameraSensorFactory::factoriesMap\n>\n> factoriesMap is a type, it should thus be called FactoriesMap.\n>\n> > + * \\brief An std::map of sensor handler factories\n>\n> \"A map of entity names to camera sensor factories\"\n>\n> > + *\n> > + * The registered sensor handler factories are associated in a map with the\n> > + * names of the media entity that represent the sensor\n>\n> * This map associates the name of the media entities that represent\n> * camera sensors to the corresponding camera sensor factory.\n>\n> But I think we can just drop the documentation as the type can be made\n> private.\n>\n> > + */\n> > +\n> > +/**\n> > + * \\brief Register a camera sensor handler factory\n>\n> This is the constructor, so\n>\n> * \\brief Construct a camera sensor factory\n>\n> > + * \\param[in] name The name of the media entity representing the sensor\n> > + *\n> > + * Register a new camera sensor factory which creates sensor handler instances\n> > + * that support camera sensors represented by the media entity with \\a name.\n>\n> How about being consistent with the pipeline handler factory\n> documentation ?\n>\n> * Creating an instance of the factory registers is with the global list of\n> * factories, accessible through the factories() function.\n>\n> > + */\n> > +CameraSensorFactory::CameraSensorFactory(const char *name)\n> > +{\n> > +\tCameraSensorFactory::registerSensorFactory(name, this);\n>\n> No need for the CameraSensorFactory:: prefix.\n>\n> > +}\n> > +\n> > +/**\n> > + * \\brief Retrieve the list of registered camera sensor factories\n> > + *\n> > + * The static factories map is defined inside the function to ensures it gets\n> > + * initialized on first use, without any dependency on link order.\n> > + *\n> > + * \\return The static list of registered camera sensor factories\n> > + */\n> > +CameraSensorFactory::factoriesMap &CameraSensorFactory::factories()\n> > +{\n> > +\tstatic factoriesMap factories;\n> > +\treturn factories;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Register a camera sensor handler factory\n> > + * \\param[in] name The name of the media entity that represents the sensor\n> > + * \\param[in] factory The factory instance to register\n> > + *\n> > + * The newly registered sensor handler \\a factory is associated with \\a name,\n> > + * when a new sensor handler is instantiated with createSensor() the name of\n> > + * the media entity is matched against the \\a name registered here to\n> > + * retrieve the correct factory.\n>\n> Should we explain the uniqueness requirement for the name parameter, as\n> done in the pipeline handler factory documentation ?\n>\n> * The camera sensor \\a factory is registered associated with an entity \\a name.\n> * When a camera sensor is created for a media entity with createSensor(), the\n> * name of the media entity is used to select the corresponding factory.\n> *\n> * The caller is responsible for guaranteeing the uniqueness of the\n> * camera sensor entity name.\n>\n> > + */\n> > +void CameraSensorFactory::registerSensorFactory(const char *name,\n> > +\t\t\t\t\t\tCameraSensorFactory *factory)\n>\n> Maybe registerFactory() to shorten the name a bit ?\n>\n> > +{\n> > +\tfactoriesMap &map = CameraSensorFactory::factories();\n>\n> No need for the prefix here either.\n>\n> Should we log an error if the name is already present in the map ?\n>\n\nAnd skip registering the new instance or delete the existing one ?\nI've gone for the former at the moment.\n\n> > +\tmap[name] = factory;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Create and return a CameraSensor\n>\n> Maybe \"Create a camera sensor corresponding to an entity\" ?\n>\n> > + * \\param[in] entity The media entity that represents the camera sensor\n> > + *\n> > + * Create a new instance of an handler for the sensor represented by \\a\n> > + * entity using one of the registered factories. If no specific handler is\n> > + * available for the sensor, or creating the handler fails, a newly created\n>\n> s/, or creating the handler fails//\n>\n> as that's not what's implemented :-)\n>\n> > + * instance of the generic CameraSensor base class is returned.\n>\n> \"... fails, an instance of the generic CameraSensor class is created and\n> returned.\"\n>\n> > + *\n> > + * Ownership of the created camera sensor instance is passed to the caller\n> > + * which is reponsible for deleting the instance.\n> > + * FIXME: is it worth using a smart pointer here ?\n>\n> I think it is. Could you give it a try ?\n>\n\nI am now using a unique_ptr<> to return the newly created camera\nsensor.\n\n> > + *\n> > + * \\return A newly created camera sensor handler instance\n> > + */\n> > +CameraSensor *CameraSensorFactory::createSensor(const MediaEntity *entity)\n> > +{\n> > +\t/*\n> > +\t * The entity name contains both the sensor name and the I2C bus ID.\n> > +\t *\n> > +\t * Remove the i2c bus part and use the sensor name only as key to\n>\n> s/i2c/I2C/\n>\n> > +\t * search for on the sensor handlers map.\n>\n> s/on/in/ ?\n>\n> > +\t */\n> > +\tconst char *entityName = entity->name().c_str();\n> > +\tconst char *delim = strchrnul(entityName, ' ');\n> > +\tstd::string sensorName(entityName, delim);\n>\n> Should you split after the last space, not the first space ? I think the\n> following would avoid including string.h.\n>\n> \tstd::string::size_type delim = entity->name().find_last_of(' ');\n> \tstd::string name = entity->name().substr(0, delim);\n>\n\nThanks, but I suspect this is not enough. It only happened with vimc,\nwhich registers a \"Sensor B\" or \"Sensor A\", but I suspect other\ndrivers might as well register subdevices with spaces in their name.\n\nInstead of cutting to the first or last space, I suspect we might need\nto look for the complete substring, even if it's a broader matching\ncriteria than what we have here.\n\nI'll give substring matching a try...\n\n> > +\n> > +\tauto it = CameraSensorFactory::factories().find(sensorName);\n> > +\tif (it == CameraSensorFactory::factories().end()) {\n> > +\t\tLOG(CameraSensorFactory, Info)\n> > +\t\t\t<< \"Unsupported sensor '\" << entity->name()\n> > +\t\t\t<< \"': using generic sensor handler\";\n> > +\t\treturn new CameraSensor(entity);\n> > +\t}\n> > +\n> > +\tLOG(CameraSensorFactory, Info) << \"Create handler for '\"\n> > +\t\t\t\t << entity->name() << \"' sensor\";\n> > +\n> > +\tCameraSensorFactory *factory = it->second;\n> > +\treturn factory->create(entity);\n> > +}\n> > +\n> > +/**\n> > + * \\def REGISTER_CAMERA_SENSOR(handler)\n> > + * \\brief Register a camera sensor handler to the sensor factory\n>\n> s/to the/with the/\n>\n> > + * \\param[in] handler The name of the sensor handler\n>\n> s/sensor handler/camera sensor class/\n>\n> > + *\n> > + * Register a camera sensor handler to the sensor factory to make it available\n>\n> s/to the/with the/\n>\n> > + * to the factory users.\n> > + */\n> > +\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h\n> > index 99cff98128dc..2f4a0cc8ad3f 100644\n> > --- a/src/libcamera/include/camera_sensor.h\n> > +++ b/src/libcamera/include/camera_sensor.h\n> > @@ -7,6 +7,7 @@\n> > #ifndef __LIBCAMERA_CAMERA_SENSOR_H__\n> > #define __LIBCAMERA_CAMERA_SENSOR_H__\n> >\n> > +#include \n> > #include \n> > #include \n> >\n> > @@ -25,7 +26,8 @@ struct V4L2SubdeviceFormat;\n> > class CameraSensor : protected Loggable\n> > {\n> > public:\n> > -\texplicit CameraSensor(const MediaEntity *entity);\n> > +\tstatic const char *entityName();\n> > +\n> > \t~CameraSensor();\n> >\n> > \tCameraSensor(const CameraSensor &) = delete;\n> > @@ -49,6 +51,8 @@ public:\n> > \tconst ControlList &properties() const { return properties_; }\n> >\n> > protected:\n> > +\tfriend class CameraSensorFactory;\n> > +\texplicit CameraSensor(const MediaEntity *entity);\n>\n> You can make the constructor private.\n>\n> > \tstd::string logPrefix() const;\n> >\n> > private:\n> > @@ -61,6 +65,39 @@ private:\n> > \tControlList properties_;\n> > };\n> >\n> > +class CameraSensorFactory\n> > +{\n> > +public:\n> > +\tusing factoriesMap = std::map;\n>\n> You can make this type private.\n>\n> > +\n> > +\tCameraSensorFactory(const char *name);\n> > +\tvirtual ~CameraSensorFactory() {}\n> > +\n> > +\tstatic CameraSensor *createSensor(const MediaEntity *entity = nullptr);\n>\n> Why the default = nullptr ? The argument should be mandatory.\n>\n> > +\n> > +private:\n> > +\tstatic factoriesMap &factories();\n> > +\tstatic void registerSensorFactory(const char *name,\n> > +\t\t\t\t\t CameraSensorFactory *factory);\n> > +\tvirtual CameraSensor *create(const MediaEntity *entity) = 0;\n> > +\n> > +};\n> > +\n> > +#define REGISTER_CAMERA_SENSOR(handler)\t\t\t\t\t\\\n>\n> s/handler/sensor/\n>\n> > +class handler##CameraSensorFactory final : public CameraSensorFactory\t\\\n>\n> To shorten lines,\n>\n> class sensor##Factory ...\n>\n> > +{\t\t\t\t\t\t\t\t\t\\\n> > +public:\t\t\t\t\t\t\t\t\t\\\n> > +\thandler##CameraSensorFactory()\t\t\t\t\t\\\n> > +\t\t: CameraSensorFactory(handler##CameraSensor::entityName()) {}\\\n>\n> Let's not add ##CameraSensor, it makes the code more confusing:\n>\n> class FooCameraSensor : CameraSensor\n> {\n> \t...\n> };\n>\n> REGISTER_CAMERA_SENSOR(Foo);\n>\n> I'd rather write\n>\n> class FooCameraSensor : CameraSensor\n> {\n> \t...\n> };\n>\n> REGISTER_CAMERA_SENSOR(FooCameraSensor);\n>\n> and make it possible for the author to name the class without a\n> CameraSensor suffix.\n\nThat requires implementations to be a bit more careful, as they should\ncome up with a name for the factory. To me, it seems more natural to\nwrite\n\nclass OV5670 : CameraSensor\n{\n}\n\nREGISTER_CAMERA_SENSOR(OV5670);\n\nThan\n\nclass OV5670 : CameraSensor\n{\n}\n\nREGISTER_CAMERA_SENSOR(OV5670Factory);\n\nThanks\n j\n\n>\n> > +\t\t\t\t\t\t\t\t\t\\\n> > +private:\t\t\t\t\t\t\t\t\\\n> > +\tCameraSensor *create(const MediaEntity *entity)\t\t\t\\\n> > +\t{\t\t\t\t\t\t\t\t\\\n> > +\t\treturn new handler##CameraSensor(entity);\t\t\\\n> > +\t}\t\t\t\t\t\t\t\t\\\n> > +};\t\t\t\t\t\t\t\t\t\\\n> > +static handler##CameraSensorFactory global_##handler##CameraSensorFactory\n> > +\n> > } /* namespace libcamera */\n> >\n> > #endif /* __LIBCAMERA_CAMERA_SENSOR_H__ */\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 387bb070b505..21934e72eba7 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -1322,7 +1322,7 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)\n> >\n> > \tMediaLink *link = links[0];\n> > \tMediaEntity *sensorEntity = link->source()->entity();\n> > -\tsensor_ = new CameraSensor(sensorEntity);\n> > +\tsensor_ = CameraSensorFactory::createSensor(sensorEntity);\n> > \tret = sensor_->init();\n> > \tif (ret)\n> > \t\treturn ret;\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 68f16f03a81e..f2f054596257 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -901,7 +901,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> >\n> > \tdata->controlInfo_ = std::move(ctrls);\n> >\n> > -\tdata->sensor_ = new CameraSensor(sensor);\n> > +\tdata->sensor_ = CameraSensorFactory::createSensor(sensor);\n> > \tret = data->sensor_->init();\n> > \tif (ret)\n> > \t\treturn ret;\n> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> > index fd4df0b03c26..cfeec1aac751 100644\n> > --- a/src/libcamera/pipeline/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc.cpp\n> > @@ -403,7 +403,7 @@ int VimcCameraData::init(MediaDevice *media)\n> > \t\treturn ret;\n> >\n> > \t/* Create and open the camera sensor, debayer, scaler and video device. */\n> > -\tsensor_ = new CameraSensor(media->getEntityByName(\"Sensor B\"));\n> > +\tsensor_ = CameraSensorFactory::createSensor(media->getEntityByName(\"Sensor B\"));\n> > \tret = sensor_->init();\n> > \tif (ret)\n> > \t\treturn ret;\n> > diff --git a/test/camera-sensor.cpp b/test/camera-sensor.cpp\n> > index 27c190fe7ace..24b83a45b656 100644\n> > --- a/test/camera-sensor.cpp\n> > +++ b/test/camera-sensor.cpp\n> > @@ -50,7 +50,7 @@ protected:\n> > \t\t\treturn TestFail;\n> > \t\t}\n> >\n> > -\t\tsensor_ = new CameraSensor(entity);\n> > +\t\tsensor_ = CameraSensorFactory::createSensor(entity);\n> > \t\tif (sensor_->init() < 0) {\n> > \t\t\tcerr << \"Unable to initialise camera sensor\" << endl;\n> > \t\t\treturn TestFail;\n> > diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.cpp b/test/v4l2_videodevice/v4l2_videodevice_test.cpp\n> > index 577da4cb601c..102a8b1b6c1c 100644\n> > --- a/test/v4l2_videodevice/v4l2_videodevice_test.cpp\n> > +++ b/test/v4l2_videodevice/v4l2_videodevice_test.cpp\n> > @@ -61,7 +61,7 @@ int V4L2VideoDeviceTest::init()\n> > \t\treturn TestFail;\n> >\n> > \tif (driver_ == \"vimc\") {\n> > -\t\tsensor_ = new CameraSensor(media_->getEntityByName(\"Sensor A\"));\n> > +\t\tsensor_ = CameraSensorFactory::createSensor(media_->getEntityByName(\"Sensor A\"));\n> > \t\tif (sensor_->init())\n> > \t\t\treturn TestFail;\n> >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3B94B61D61\n\tfor ;\n\tTue, 18 Feb 2020 10:26:33 +0100 (CET)","from uno.localdomain (93-34-114-233.ip49.fastwebnet.it\n\t[93.34.114.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id B4717200006;\n\tTue, 18 Feb 2020 09:26:32 +0000 (UTC)"],"Date":"Tue, 18 Feb 2020 10:29:17 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200218092917.6bh3m3w7hhywhojf@uno.localdomain>","References":"<20200206185247.202233-1-jacopo@jmondi.org>\n\t<20200206185247.202233-4-jacopo@jmondi.org>\n\t<20200207223559.GE4726@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"xwiu33qdqogfdjoz\"","Content-Disposition":"inline","In-Reply-To":"<20200207223559.GE4726@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 3/7] libcamera: camera_sensor:\n\tIntroduce CameraSensorFactory","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 18 Feb 2020 09:26:33 -0000"}},{"id":3803,"web_url":"https://patchwork.libcamera.org/comment/3803/","msgid":"<20200218093511.GB4828@pendragon.ideasonboard.com>","date":"2020-02-18T09:35:11","subject":"Re: [libcamera-devel] [PATCH v2 3/7] libcamera: camera_sensor:\n\tIntroduce CameraSensorFactory","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Tue, Feb 18, 2020 at 10:29:17AM +0100, Jacopo Mondi wrote:\n> On Sat, Feb 08, 2020 at 12:35:59AM +0200, Laurent Pinchart wrote:\n> > On Thu, Feb 06, 2020 at 07:52:43PM +0100, Jacopo Mondi wrote:\n> > > Introduce a factory to create CameraSensor derived classes instances by\n> > > inspecting the sensor media entity name and provide a convenience macro\n> > > to register specialized sensor handlers.\n> >\n> > I really like the factory :-)\n> >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/libcamera/camera_sensor.cpp | 134 ++++++++++++++++++\n> > > src/libcamera/include/camera_sensor.h | 39 ++++-\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-\n> > > src/libcamera/pipeline/vimc.cpp | 2 +-\n> > > test/camera-sensor.cpp | 2 +-\n> > > .../v4l2_videodevice_test.cpp | 2 +-\n> > > 7 files changed, 177 insertions(+), 6 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > index 2219a4307436..fc8452b607a0 100644\n> > > --- a/src/libcamera/camera_sensor.cpp\n> > > +++ b/src/libcamera/camera_sensor.cpp\n> > > @@ -11,7 +11,9 @@\n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> >\n> > No need to include map here, no function in this file make use of it\n> > directly. The header is included in camera_sensor.h for the usage of\n> > std::map there, that's enough.\n> >\n> > > #include \n> > > +#include \n> > >\n> > > #include \n> > >\n> > > @@ -42,6 +44,18 @@ LOG_DEFINE_CATEGORY(CameraSensor);\n> > > * devices as the needs arise.\n> > > */\n> > >\n> > > +/**\n> > > + * \\fn CameraSensor::entityName()\n> >\n> > I wonder, wouldn't it be simpler to pass the entity name string\n> > directly to the REGISTER_CAMERA_SENSOR macro ? It would avoid declaring\n> > and undefined static method in the base class.\n> >\n> > If we ever end up needing more complex matching code, a static match\n> > function that takes a media entity pointer would make sense, but for a\n> > fully static name, this seems a bit overkill.\n> >\n> > > + * \\brief Retrieve the name of the entity which identifies the supported sensor\n> > > + *\n> > > + * Camera sensor handlers have to report with this function the name of the\n> > > + * media entity which represents the camera sensor they support. The here\n> > > + * reported name is then matched against the name of the MediaEntity provided\n> > > + * at createSensor() time to identify the correct CameraSensorFactory.\n> > > + *\n> > > + * \\return The name of the media entity that identifies the sensor\n> > > + */\n> > > +\n> > > /**\n> > > * \\brief Construct a CameraSensor\n> > > * \\param[in] entity The media entity backing the camera sensor\n> > > @@ -366,4 +380,124 @@ std::string CameraSensor::logPrefix() const\n> > > \treturn \"'\" + subdev_->entity()->name() + \"'\";\n> > > }\n> > >\n> > > +/**\n> > > + * \\class CameraSensorFactory\n> > > + * \\brief Factory of camera sensor handlers\n> >\n> > \"camera sensor handlers\" or \"camera sensors\" ? We have pipeline\n> > handlers, and if we could avoid using the name handler here, it could\n> > help reducing confusion. This implies s/sensor handler/sensor below.\n> >\n> > > + *\n> > > + * The class provides to camera sensor handlers the ability to register\n> > > + * themselves to the factory to allow the creation of their instances by\n> > > + * matching the name of the media entity which represents the sensor.\n> > > + *\n> > > + * Camera sensor handlers use the REGISTER_CAMERA_SENSOR() macro to\n> > > + * add themselves to the camera sensor factory. Users of the factory\n> > > + * creates camera sensor handler instances by using the static\n> > > + * CameraSensorFactory::createInstance() method, which returns a pointer\n> > > + * to the opportune CameraSensor sub-class if any, or a newly created instance\n> > > + * of the generic CameraSensor class.\n> > > + */\n> > > +\n> > > +LOG_DEFINE_CATEGORY(CameraSensorFactory);\n> >\n> > I think you can use the CameraSensor log category, I don't really see a\n> > need to split messages in two categories here.\n> >\n> > > +\n> > > +/**\n> > > + * \\typedef CameraSensorFactory::factoriesMap\n> >\n> > factoriesMap is a type, it should thus be called FactoriesMap.\n> >\n> > > + * \\brief An std::map of sensor handler factories\n> >\n> > \"A map of entity names to camera sensor factories\"\n> >\n> > > + *\n> > > + * The registered sensor handler factories are associated in a map with the\n> > > + * names of the media entity that represent the sensor\n> >\n> > * This map associates the name of the media entities that represent\n> > * camera sensors to the corresponding camera sensor factory.\n> >\n> > But I think we can just drop the documentation as the type can be made\n> > private.\n> >\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Register a camera sensor handler factory\n> >\n> > This is the constructor, so\n> >\n> > * \\brief Construct a camera sensor factory\n> >\n> > > + * \\param[in] name The name of the media entity representing the sensor\n> > > + *\n> > > + * Register a new camera sensor factory which creates sensor handler instances\n> > > + * that support camera sensors represented by the media entity with \\a name.\n> >\n> > How about being consistent with the pipeline handler factory\n> > documentation ?\n> >\n> > * Creating an instance of the factory registers is with the global list of\n> > * factories, accessible through the factories() function.\n> >\n> > > + */\n> > > +CameraSensorFactory::CameraSensorFactory(const char *name)\n> > > +{\n> > > +\tCameraSensorFactory::registerSensorFactory(name, this);\n> >\n> > No need for the CameraSensorFactory:: prefix.\n> >\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Retrieve the list of registered camera sensor factories\n> > > + *\n> > > + * The static factories map is defined inside the function to ensures it gets\n> > > + * initialized on first use, without any dependency on link order.\n> > > + *\n> > > + * \\return The static list of registered camera sensor factories\n> > > + */\n> > > +CameraSensorFactory::factoriesMap &CameraSensorFactory::factories()\n> > > +{\n> > > +\tstatic factoriesMap factories;\n> > > +\treturn factories;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Register a camera sensor handler factory\n> > > + * \\param[in] name The name of the media entity that represents the sensor\n> > > + * \\param[in] factory The factory instance to register\n> > > + *\n> > > + * The newly registered sensor handler \\a factory is associated with \\a name,\n> > > + * when a new sensor handler is instantiated with createSensor() the name of\n> > > + * the media entity is matched against the \\a name registered here to\n> > > + * retrieve the correct factory.\n> >\n> > Should we explain the uniqueness requirement for the name parameter, as\n> > done in the pipeline handler factory documentation ?\n> >\n> > * The camera sensor \\a factory is registered associated with an entity \\a name.\n> > * When a camera sensor is created for a media entity with createSensor(), the\n> > * name of the media entity is used to select the corresponding factory.\n> > *\n> > * The caller is responsible for guaranteeing the uniqueness of the\n> > * camera sensor entity name.\n> >\n> > > + */\n> > > +void CameraSensorFactory::registerSensorFactory(const char *name,\n> > > +\t\t\t\t\t\tCameraSensorFactory *factory)\n> >\n> > Maybe registerFactory() to shorten the name a bit ?\n> >\n> > > +{\n> > > +\tfactoriesMap &map = CameraSensorFactory::factories();\n> >\n> > No need for the prefix here either.\n> >\n> > Should we log an error if the name is already present in the map ?\n> \n> And skip registering the new instance or delete the existing one ?\n> I've gone for the former at the moment.\n\nEither is fine, it shouldn't happen anyway.\n\n> > > +\tmap[name] = factory;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Create and return a CameraSensor\n> >\n> > Maybe \"Create a camera sensor corresponding to an entity\" ?\n> >\n> > > + * \\param[in] entity The media entity that represents the camera sensor\n> > > + *\n> > > + * Create a new instance of an handler for the sensor represented by \\a\n> > > + * entity using one of the registered factories. If no specific handler is\n> > > + * available for the sensor, or creating the handler fails, a newly created\n> >\n> > s/, or creating the handler fails//\n> >\n> > as that's not what's implemented :-)\n> >\n> > > + * instance of the generic CameraSensor base class is returned.\n> >\n> > \"... fails, an instance of the generic CameraSensor class is created and\n> > returned.\"\n> >\n> > > + *\n> > > + * Ownership of the created camera sensor instance is passed to the caller\n> > > + * which is reponsible for deleting the instance.\n> > > + * FIXME: is it worth using a smart pointer here ?\n> >\n> > I think it is. Could you give it a try ?\n> \n> I am now using a unique_ptr<> to return the newly created camera\n> sensor.\n\n\\o/\n\n> > > + *\n> > > + * \\return A newly created camera sensor handler instance\n> > > + */\n> > > +CameraSensor *CameraSensorFactory::createSensor(const MediaEntity *entity)\n> > > +{\n> > > +\t/*\n> > > +\t * The entity name contains both the sensor name and the I2C bus ID.\n> > > +\t *\n> > > +\t * Remove the i2c bus part and use the sensor name only as key to\n> >\n> > s/i2c/I2C/\n> >\n> > > +\t * search for on the sensor handlers map.\n> >\n> > s/on/in/ ?\n> >\n> > > +\t */\n> > > +\tconst char *entityName = entity->name().c_str();\n> > > +\tconst char *delim = strchrnul(entityName, ' ');\n> > > +\tstd::string sensorName(entityName, delim);\n> >\n> > Should you split after the last space, not the first space ? I think the\n> > following would avoid including string.h.\n> >\n> > \tstd::string::size_type delim = entity->name().find_last_of(' ');\n> > \tstd::string name = entity->name().substr(0, delim);\n> \n> Thanks, but I suspect this is not enough. It only happened with vimc,\n> which registers a \"Sensor B\" or \"Sensor A\", but I suspect other\n> drivers might as well register subdevices with spaces in their name.\n> \n> Instead of cutting to the first or last space, I suspect we might need\n> to look for the complete substring, even if it's a broader matching\n> criteria than what we have here.\n> \n> I'll give substring matching a try...\n\nGood point.\n\nhttps://en.cppreference.com/w/cpp/string/basic_string/starts_with would\nbe useful, but is only available in C++20. Time for\nutils::string_starts_with ?\n\n> > > +\n> > > +\tauto it = CameraSensorFactory::factories().find(sensorName);\n> > > +\tif (it == CameraSensorFactory::factories().end()) {\n> > > +\t\tLOG(CameraSensorFactory, Info)\n> > > +\t\t\t<< \"Unsupported sensor '\" << entity->name()\n> > > +\t\t\t<< \"': using generic sensor handler\";\n> > > +\t\treturn new CameraSensor(entity);\n> > > +\t}\n> > > +\n> > > +\tLOG(CameraSensorFactory, Info) << \"Create handler for '\"\n> > > +\t\t\t\t << entity->name() << \"' sensor\";\n> > > +\n> > > +\tCameraSensorFactory *factory = it->second;\n> > > +\treturn factory->create(entity);\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\def REGISTER_CAMERA_SENSOR(handler)\n> > > + * \\brief Register a camera sensor handler to the sensor factory\n> >\n> > s/to the/with the/\n> >\n> > > + * \\param[in] handler The name of the sensor handler\n> >\n> > s/sensor handler/camera sensor class/\n> >\n> > > + *\n> > > + * Register a camera sensor handler to the sensor factory to make it available\n> >\n> > s/to the/with the/\n> >\n> > > + * to the factory users.\n> > > + */\n> > > +\n> > > } /* namespace libcamera */\n> > > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h\n> > > index 99cff98128dc..2f4a0cc8ad3f 100644\n> > > --- a/src/libcamera/include/camera_sensor.h\n> > > +++ b/src/libcamera/include/camera_sensor.h\n> > > @@ -7,6 +7,7 @@\n> > > #ifndef __LIBCAMERA_CAMERA_SENSOR_H__\n> > > #define __LIBCAMERA_CAMERA_SENSOR_H__\n> > >\n> > > +#include \n> > > #include \n> > > #include \n> > >\n> > > @@ -25,7 +26,8 @@ struct V4L2SubdeviceFormat;\n> > > class CameraSensor : protected Loggable\n> > > {\n> > > public:\n> > > -\texplicit CameraSensor(const MediaEntity *entity);\n> > > +\tstatic const char *entityName();\n> > > +\n> > > \t~CameraSensor();\n> > >\n> > > \tCameraSensor(const CameraSensor &) = delete;\n> > > @@ -49,6 +51,8 @@ public:\n> > > \tconst ControlList &properties() const { return properties_; }\n> > >\n> > > protected:\n> > > +\tfriend class CameraSensorFactory;\n> > > +\texplicit CameraSensor(const MediaEntity *entity);\n> >\n> > You can make the constructor private.\n> >\n> > > \tstd::string logPrefix() const;\n> > >\n> > > private:\n> > > @@ -61,6 +65,39 @@ private:\n> > > \tControlList properties_;\n> > > };\n> > >\n> > > +class CameraSensorFactory\n> > > +{\n> > > +public:\n> > > +\tusing factoriesMap = std::map;\n> >\n> > You can make this type private.\n> >\n> > > +\n> > > +\tCameraSensorFactory(const char *name);\n> > > +\tvirtual ~CameraSensorFactory() {}\n> > > +\n> > > +\tstatic CameraSensor *createSensor(const MediaEntity *entity = nullptr);\n> >\n> > Why the default = nullptr ? The argument should be mandatory.\n> >\n> > > +\n> > > +private:\n> > > +\tstatic factoriesMap &factories();\n> > > +\tstatic void registerSensorFactory(const char *name,\n> > > +\t\t\t\t\t CameraSensorFactory *factory);\n> > > +\tvirtual CameraSensor *create(const MediaEntity *entity) = 0;\n> > > +\n> > > +};\n> > > +\n> > > +#define REGISTER_CAMERA_SENSOR(handler)\t\t\t\t\t\\\n> >\n> > s/handler/sensor/\n> >\n> > > +class handler##CameraSensorFactory final : public CameraSensorFactory\t\\\n> >\n> > To shorten lines,\n> >\n> > class sensor##Factory ...\n> >\n> > > +{\t\t\t\t\t\t\t\t\t\\\n> > > +public:\t\t\t\t\t\t\t\t\t\\\n> > > +\thandler##CameraSensorFactory()\t\t\t\t\t\\\n> > > +\t\t: CameraSensorFactory(handler##CameraSensor::entityName()) {}\\\n> >\n> > Let's not add ##CameraSensor, it makes the code more confusing:\n> >\n> > class FooCameraSensor : CameraSensor\n> > {\n> > \t...\n> > };\n> >\n> > REGISTER_CAMERA_SENSOR(Foo);\n> >\n> > I'd rather write\n> >\n> > class FooCameraSensor : CameraSensor\n> > {\n> > \t...\n> > };\n> >\n> > REGISTER_CAMERA_SENSOR(FooCameraSensor);\n> >\n> > and make it possible for the author to name the class without a\n> > CameraSensor suffix.\n> \n> That requires implementations to be a bit more careful, as they should\n> come up with a name for the factory. To me, it seems more natural to\n> write\n> \n> class OV5670 : CameraSensor\n> {\n> }\n> \n> REGISTER_CAMERA_SENSOR(OV5670);\n> \n> Than\n> \n> class OV5670 : CameraSensor\n> {\n> }\n> \n> REGISTER_CAMERA_SENSOR(OV5670Factory);\n\nI haven't expressed myself very clearly, sorry about that. I didn't mean\nremoving the ##Factory part, but the ##CameraSensor in\n\n\t: CameraSensorFactory(handler##CameraSensor::entityName()) {}\n\nWe should end up writing\n\nclass OV5670 : CameraSensor\n{\n\t...\n};\n\nREGISTER_CAMERA_SENSOR(OV5670);\n\nwhile I think your patch required\n\nclass OV5670CameraSensor : CameraSensor\n{\n\t...\n};\n\nREGISTER_CAMERA_SENSOR(OV5670);\n\n> > > +\t\t\t\t\t\t\t\t\t\\\n> > > +private:\t\t\t\t\t\t\t\t\\\n> > > +\tCameraSensor *create(const MediaEntity *entity)\t\t\t\\\n> > > +\t{\t\t\t\t\t\t\t\t\\\n> > > +\t\treturn new handler##CameraSensor(entity);\t\t\\\n> > > +\t}\t\t\t\t\t\t\t\t\\\n> > > +};\t\t\t\t\t\t\t\t\t\\\n> > > +static handler##CameraSensorFactory global_##handler##CameraSensorFactory\n> > > +\n> > > } /* namespace libcamera */\n> > >\n> > > #endif /* __LIBCAMERA_CAMERA_SENSOR_H__ */\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index 387bb070b505..21934e72eba7 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -1322,7 +1322,7 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)\n> > >\n> > > \tMediaLink *link = links[0];\n> > > \tMediaEntity *sensorEntity = link->source()->entity();\n> > > -\tsensor_ = new CameraSensor(sensorEntity);\n> > > +\tsensor_ = CameraSensorFactory::createSensor(sensorEntity);\n> > > \tret = sensor_->init();\n> > > \tif (ret)\n> > > \t\treturn ret;\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index 68f16f03a81e..f2f054596257 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -901,7 +901,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > >\n> > > \tdata->controlInfo_ = std::move(ctrls);\n> > >\n> > > -\tdata->sensor_ = new CameraSensor(sensor);\n> > > +\tdata->sensor_ = CameraSensorFactory::createSensor(sensor);\n> > > \tret = data->sensor_->init();\n> > > \tif (ret)\n> > > \t\treturn ret;\n> > > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> > > index fd4df0b03c26..cfeec1aac751 100644\n> > > --- a/src/libcamera/pipeline/vimc.cpp\n> > > +++ b/src/libcamera/pipeline/vimc.cpp\n> > > @@ -403,7 +403,7 @@ int VimcCameraData::init(MediaDevice *media)\n> > > \t\treturn ret;\n> > >\n> > > \t/* Create and open the camera sensor, debayer, scaler and video device. */\n> > > -\tsensor_ = new CameraSensor(media->getEntityByName(\"Sensor B\"));\n> > > +\tsensor_ = CameraSensorFactory::createSensor(media->getEntityByName(\"Sensor B\"));\n> > > \tret = sensor_->init();\n> > > \tif (ret)\n> > > \t\treturn ret;\n> > > diff --git a/test/camera-sensor.cpp b/test/camera-sensor.cpp\n> > > index 27c190fe7ace..24b83a45b656 100644\n> > > --- a/test/camera-sensor.cpp\n> > > +++ b/test/camera-sensor.cpp\n> > > @@ -50,7 +50,7 @@ protected:\n> > > \t\t\treturn TestFail;\n> > > \t\t}\n> > >\n> > > -\t\tsensor_ = new CameraSensor(entity);\n> > > +\t\tsensor_ = CameraSensorFactory::createSensor(entity);\n> > > \t\tif (sensor_->init() < 0) {\n> > > \t\t\tcerr << \"Unable to initialise camera sensor\" << endl;\n> > > \t\t\treturn TestFail;\n> > > diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.cpp b/test/v4l2_videodevice/v4l2_videodevice_test.cpp\n> > > index 577da4cb601c..102a8b1b6c1c 100644\n> > > --- a/test/v4l2_videodevice/v4l2_videodevice_test.cpp\n> > > +++ b/test/v4l2_videodevice/v4l2_videodevice_test.cpp\n> > > @@ -61,7 +61,7 @@ int V4L2VideoDeviceTest::init()\n> > > \t\treturn TestFail;\n> > >\n> > > \tif (driver_ == \"vimc\") {\n> > > -\t\tsensor_ = new CameraSensor(media_->getEntityByName(\"Sensor A\"));\n> > > +\t\tsensor_ = CameraSensorFactory::createSensor(media_->getEntityByName(\"Sensor A\"));\n> > > \t\tif (sensor_->init())\n> > > \t\t\treturn TestFail;\n> > >","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BA31C61D61\n\tfor ;\n\tTue, 18 Feb 2020 10:35:29 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 27B762F9;\n\tTue, 18 Feb 2020 10:35:29 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1582018529;\n\tbh=x8qhP1jzwBs0fkYZbk/K2Vcs0aDi9LXvl1l1kIQiquo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PimRZaOGMof4KeHwlGACd2JEMyQsHCTCi9OJN5DSuDgngyGj9hY0pOaS+kISvwfrV\n\tNkDcuYzi4ccvseM4jnITm0/3zfukvlQXGLo/ebsqwkC0tK+CDHf4KaVvPKgT6bQGQW\n\tbvjefBGuNXqlz++xsGVXO6yN7E3AnDgCyHs4phuY=","Date":"Tue, 18 Feb 2020 11:35:11 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200218093511.GB4828@pendragon.ideasonboard.com>","References":"<20200206185247.202233-1-jacopo@jmondi.org>\n\t<20200206185247.202233-4-jacopo@jmondi.org>\n\t<20200207223559.GE4726@pendragon.ideasonboard.com>\n\t<20200218092917.6bh3m3w7hhywhojf@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200218092917.6bh3m3w7hhywhojf@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 3/7] libcamera: camera_sensor:\n\tIntroduce CameraSensorFactory","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 18 Feb 2020 09:35:29 -0000"}},{"id":3805,"web_url":"https://patchwork.libcamera.org/comment/3805/","msgid":"<20200218100738.fvdlmcj2ltv4e6jc@uno.localdomain>","date":"2020-02-18T10:07:38","subject":"Re: [libcamera-devel] [PATCH v2 3/7] libcamera: camera_sensor:\n\tIntroduce CameraSensorFactory","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Tue, Feb 18, 2020 at 11:35:11AM +0200, Laurent Pinchart wrote:\n\n[snip]\n\n> > > > +\t */\n> > > > +\tconst char *entityName = entity->name().c_str();\n> > > > +\tconst char *delim = strchrnul(entityName, ' ');\n> > > > +\tstd::string sensorName(entityName, delim);\n> > >\n> > > Should you split after the last space, not the first space ? I think the\n> > > following would avoid including string.h.\n> > >\n> > > \tstd::string::size_type delim = entity->name().find_last_of(' ');\n> > > \tstd::string name = entity->name().substr(0, delim);\n> >\n> > Thanks, but I suspect this is not enough. It only happened with vimc,\n> > which registers a \"Sensor B\" or \"Sensor A\", but I suspect other\n> > drivers might as well register subdevices with spaces in their name.\n> >\n> > Instead of cutting to the first or last space, I suspect we might need\n> > to look for the complete substring, even if it's a broader matching\n> > criteria than what we have here.\n> >\n> > I'll give substring matching a try...\n>\n> Good point.\n>\n> https://en.cppreference.com/w/cpp/string/basic_string/starts_with would\n> be useful, but is only available in C++20. Time for\n> utils::string_starts_with ?\n>\n\nI've considered that, but at the same time it means we're now looking\nfor the 'key' (provided by the camera sensor at registration time) in\nthe 'name' (the name of the entity for which we're creating a camera\nsensor).\n\nSo to me it seems enough to look for the substring with\nstd::string::find(). Also, and this should not happen, I know, the fact that\nthe entiy names are composed as \"$sensorname $i2cbus::$i2caddr\" is a\nconvention enforced by the driver usage of \"v4l2_i2c_subdev_set_name()\"\nwhich is called by the canonical \"v4l2_i2c_subdev_init()\". I know upstream\nand well behaved drivers should gurantee their names to be assembled with\nthose functions as\n\n\tsnprintf(sd->name, sizeof(sd->name), \"%s%s %d-%04x\", devname, postfix,\n\t\t i2c_adapter_id(client->adapter), client->addr);\n\nbut that's not guaranteed for downstream users. I know we can't\nsupport any crazy BSP hack out there, and we don't want to, but in\nthis case enforcing that sensor name has that specific format in the\nmatching criteria, would tie hands to the ones who want to support\ncustom drivers using custom names with a custom camera sensor\nsubclass.\n\nI would then just look for the key in the sensor name, whatever format\nit has. It will work for us, and support sensor drivers with more\nexotic naming schemes..\n\n\n> > > > +\n> > > > +\tauto it = CameraSensorFactory::factories().find(sensorName);\n> > > > +\tif (it == CameraSensorFactory::factories().end()) {\n> > > > +\t\tLOG(CameraSensorFactory, Info)\n> > > > +\t\t\t<< \"Unsupported sensor '\" << entity->name()\n> > > > +\t\t\t<< \"': using generic sensor handler\";\n> > > > +\t\treturn new CameraSensor(entity);\n> > > > +\t}\n> > > > +\n> > > > +\tLOG(CameraSensorFactory, Info) << \"Create handler for '\"\n> > > > +\t\t\t\t << entity->name() << \"' sensor\";\n> > > > +\n> > > > +\tCameraSensorFactory *factory = it->second;\n> > > > +\treturn factory->create(entity);\n> > > > +}\n> > > > +\n\n[snip]\n\n> > >\n> > > Let's not add ##CameraSensor, it makes the code more confusing:\n> > >\n> > > class FooCameraSensor : CameraSensor\n> > > {\n> > > \t...\n> > > };\n> > >\n> > > REGISTER_CAMERA_SENSOR(Foo);\n> > >\n> > > I'd rather write\n> > >\n> > > class FooCameraSensor : CameraSensor\n> > > {\n> > > \t...\n> > > };\n> > >\n> > > REGISTER_CAMERA_SENSOR(FooCameraSensor);\n> > >\n> > > and make it possible for the author to name the class without a\n> > > CameraSensor suffix.\n> >\n> > That requires implementations to be a bit more careful, as they should\n> > come up with a name for the factory. To me, it seems more natural to\n> > write\n> >\n> > class OV5670 : CameraSensor\n> > {\n> > }\n> >\n> > REGISTER_CAMERA_SENSOR(OV5670);\n> >\n> > Than\n> >\n> > class OV5670 : CameraSensor\n> > {\n> > }\n> >\n> > REGISTER_CAMERA_SENSOR(OV5670Factory);\n>\n> I haven't expressed myself very clearly, sorry about that. I didn't mean\n> removing the ##Factory part, but the ##CameraSensor in\n>\n> \t: CameraSensorFactory(handler##CameraSensor::entityName()) {}\n>\n> We should end up writing\n>\n> class OV5670 : CameraSensor\n> {\n> \t...\n> };\n>\n> REGISTER_CAMERA_SENSOR(OV5670);\n>\n> while I think your patch required\n>\n> class OV5670CameraSensor : CameraSensor\n> {\n> \t...\n> };\n>\n> REGISTER_CAMERA_SENSOR(OV5670);\n>\n\nCorrect. Sorry, I missed what you meant. I agree though, and now we\nuse OV5670 both as class name and as registration key.\n\nThanks\n j","headers":{"Return-Path":"","Received":["from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5936A61D61\n\tfor ;\n\tTue, 18 Feb 2020 11:04:54 +0100 (CET)","from uno.localdomain (93-34-114-233.ip49.fastwebnet.it\n\t[93.34.114.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id D5710200005;\n\tTue, 18 Feb 2020 10:04:53 +0000 (UTC)"],"Date":"Tue, 18 Feb 2020 11:07:38 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200218100738.fvdlmcj2ltv4e6jc@uno.localdomain>","References":"<20200206185247.202233-1-jacopo@jmondi.org>\n\t<20200206185247.202233-4-jacopo@jmondi.org>\n\t<20200207223559.GE4726@pendragon.ideasonboard.com>\n\t<20200218092917.6bh3m3w7hhywhojf@uno.localdomain>\n\t<20200218093511.GB4828@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"t3lfwlvfzbj6zqea\"","Content-Disposition":"inline","In-Reply-To":"<20200218093511.GB4828@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 3/7] libcamera: camera_sensor:\n\tIntroduce CameraSensorFactory","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 18 Feb 2020 10:04:54 -0000"}}]