[{"id":4264,"web_url":"https://patchwork.libcamera.org/comment/4264/","msgid":"<0df53dcb-5963-15e2-d2ef-9ffff4e1dc66@ideasonboard.com>","date":"2020-03-24T15:37:21","subject":"Re: [libcamera-devel] [PATCH v3 3/6] libcamera: camera_sensor:\n\tIntroduce CameraSensorFactory","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 09/03/2020 18:04, 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 camera sensor sub-classes.\n> \n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n\nOther than documentation fixups:\n\nReviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\n> \n> ---\n> v2 -> v3:\n> - Update documentation\n> - Register sensor name as REGISTER_CAMERA_SENSOR() second parameter\n> - Create camera sensor as unique_ptr\n> - Match entity name and key with std::string::find() in\n>   CameraSensorFactory::createSensor()\n> ---\n>  src/libcamera/camera_sensor.cpp               | 136 ++++++++++++++++++\n>  src/libcamera/include/camera_sensor.h         |  32 +++++\n>  src/libcamera/pipeline/ipu3/ipu3.cpp          |   9 +-\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  17 +--\n>  src/libcamera/pipeline/vimc.cpp               |   6 +-\n>  test/camera-sensor.cpp                        |   9 +-\n>  .../v4l2_videodevice_test.cpp                 |   3 +-\n>  test/v4l2_videodevice/v4l2_videodevice_test.h |   6 +-\n>  8 files changed, 187 insertions(+), 31 deletions(-)\n> \n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index 2219a4307436..8bfe02c9e8ff 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -12,6 +12,8 @@\n>  #include <iomanip>\n>  #include <limits.h>\n>  #include <math.h>\n> +#include <memory>\n> +#include <string.h>\n>  \n>  #include <libcamera/property_ids.h>\n>  \n> @@ -366,4 +368,138 @@ std::string CameraSensor::logPrefix() const\n>  \treturn \"'\" + subdev_->entity()->name() + \"'\";\n>  }\n>  \n> +/**\n> + * \\class CameraSensorFactory\n> + * \\brief Factory of camera sensors\n> + *\n> + * The class provides to camera sensors the ability to register themselves to\n\nThis class provides camera sensors with the ability to...\n\n\n> + * the factory to allow the creation of their instances by matching the name of\n> + * 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\n/creates/create/\n\n> + * CameraSensorFactory::createInstance() method, which returns a pointer\n> + * to the opportune CameraSensor sub-class if any, or a newly created instance\n\nopportune? I'm not sure that's helpful there...\n\n\n\"which returns a pointer to the existing CameraSensor sub-class if it\nexists, or a newly created ...\"\n\n\n\n> + * of the generic CameraSensor class.\n> + */\n> +\n> +/**\n> + * \\brief Construct a camera sensor factory\n> + * \\param[in] name The name of the media entity that represents 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(Trying to figure out how to say this...., the 'handler' verb seems out\nof place to me I'm afraid - and makes me think it's something else...)\n\nRegister a new camera sensor factory which creates CameraSensor\ninstances to support camera sensors represented by the media entity with\n\\a name.\n\n> + *\n> + * Creating an instance of the factory registers it with the global list of\n> + * factories, accessible through the factories() function.\n> + */\n> +CameraSensorFactory::CameraSensorFactory(const char *name)\n> +{\n> +\tregisterFactory(name, this);\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\n/ensures/ensure/\n\n> + * initialized on first use, without any dependency on link order.\n\nThat's an implementation detail, shouldn't that comment go 'in' the\nfunction rather than in the documentation of the function?\n\n> + *\n> + * \\return The static list of registered camera sensor factories\n\nDo we need to state that it's a static list? That makes me think that it\ncan't change ...\n\n> + */\n> +CameraSensorFactory::FactoriesMap &CameraSensorFactory::factories()\n> +{\n> +\tstatic FactoriesMap factories;\n> +\treturn factories;\n> +}\n> +\n> +/**\n> + * \\brief Register a camera sensor 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 camera sensor \\a factory is registered and associated with an entity \\a\n> + * name. When a camera sensor is created for a media entity with the\n> + * createSensor() method, the name of the media entity there provided is used\n> + * to select the corresponding factory.\n\n/name/\\a name/ ? Or does doxygen automatically match...\n/there//\n\n> + *\n> + * The caller is responsible for guaranteeing the uniqueness of the\n> + * camera sensor entity name.\n> + */\n> +void CameraSensorFactory::registerFactory(const char *name,\n> +\t\t\t\t\t  CameraSensorFactory *factory)\n> +{\n> +\tFactoriesMap &map = factories();\n> +\n> +\tif (map.find(name) != map.end()) {\n> +\t\tLOG(CameraSensor, Error)\n> +\t\t\t<< \"Unable to register camera sensor factory '\"\n> +\t\t\t<< name << \"': already registered\";\n> +\t\treturn;\n> +\t}\n> +\n> +\tmap[name] = factory;\n> +}\n> +\n> +/**\n> + * \\brief Create a camera sensor corresponding to \\a entity\n> + * \\param[in] entity The media entity that represents the sensor\n> + *\n> + * Create a new instance of a CameraSensor subclass for the sensor represented\n> + * by \\a entity using one of the registered factories. If no specific class is\n> + * available for the sensor, an instance of the generic CameraSensor class is\n> + * created and returned.\n> + *\n> + * Ownership of the created camera sensor instance is passed to the caller as\n> + * a unique pointer.\n> + *\n> + * \\return A unique pointer to the newly created camera sensor instance\n> + */\n> +std::unique_ptr<CameraSensor> CameraSensorFactory::createSensor(const MediaEntity *entity)\n> +{\n> +\tconst std::string &name = entity->name();\n> +\tfor (const auto &it : factories()) {\n> +\t\tconst std::string &key = it.first;\n> +\n> +\t\t/*\n> +\t\t * Match the name of the entity with the keys in the factories\n> +\t\t * map. The factories map keys are provided by the CameraSensor\n> +\t\t * sub-class at factory registration time as the sensor\n> +\t\t * entity name.\n> +\t\t *\n> +\t\t * To support both canonical sensor entity names (in the\n> +\t\t * \"devname i2c-adapt:i2c-addr\" format) and non-canonical sensor\n> +\t\t * names such (as VIMC's \"Sensor B\" ones), search for the key\n> +\t\t * in the entity name.\n> +\t\t *\n> +\t\t * \\todo Delegate matching to the CameraSensor sub-class\n> +\t\t * to support more complex matching criteria.\n> +\t\t */\n> +\t\tif (name.find(key) == std::string::npos)\n> +\t\t\tcontinue;\n> +\n> +\t\tLOG(CameraSensor, Info) << \"Create camera sensor for '\"\n> +\t\t\t\t\t<< name << \"'\";\n> +\n> +\t\tCameraSensorFactory *factory = it.second;\n> +\t\treturn std::unique_ptr<CameraSensor>(factory->create(entity));\n> +\t}\n> +\n> +\tLOG(CameraSensor, Info) << \"Unsupported sensor '\" << entity->name()\n> +\t\t\t\t<< \"': using generic camera sensor\";\n> +\treturn std::make_unique<CameraSensor>(entity);\n> +}\n> +\n> +/**\n> + * \\def REGISTER_CAMERA_SENSOR(sensor, entityName)\n> + * \\brief Register camera sensor \\a name with the sensor factory\n\nShould that be \"\\a entityName\" ?\n\n\n> + * \\param[in] sensor The name of the camera sensor class to register\n> + * \\param[in] entityName The name of the image sensor supported by the factory\n> + *\n> + * Register camera sensor factory \\a sensor with the global list of factories.\n\nRegister \\a sensor as a camera sensor factory in the global list of\nfactories.\n\n\n> + * The camera sensor factory supports image sensors identified by \\a entityName.\n> + * The sensor name reported by the media entity that identifies it is usually\n> + * composed by the here provided \\a entityName and the I2C bus and device ids.\n\nusually composed of the \\a entityName and the I2C bus and device ids.\n\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..633955591b36 100644\n> --- a/src/libcamera/include/camera_sensor.h\n> +++ b/src/libcamera/include/camera_sensor.h\n> @@ -7,6 +7,8 @@\n>  #ifndef __LIBCAMERA_CAMERA_SENSOR_H__\n>  #define __LIBCAMERA_CAMERA_SENSOR_H__\n>  \n> +#include <map>\n> +#include <memory>\n>  #include <string>\n>  #include <vector>\n>  \n> @@ -61,6 +63,36 @@ private:\n>  \tControlList properties_;\n>  };\n>  \n> +class CameraSensorFactory\n> +{\n> +public:\n> +\tCameraSensorFactory(const char *name);\n> +\tvirtual ~CameraSensorFactory() {}\n> +\n> +\tstatic std::unique_ptr<CameraSensor> createSensor(const MediaEntity *entity);\n> +\n> +private:\n> +\tusing FactoriesMap = std::map<const std::string, CameraSensorFactory *>;\n> +\tstatic FactoriesMap &factories();\n> +\tstatic void registerFactory(const char *name,\n> +\t\t\t\t    CameraSensorFactory *factory);\n> +\tvirtual CameraSensor *create(const MediaEntity *entity) = 0;\n> +};\n> +\n> +#define REGISTER_CAMERA_SENSOR(sensor, entityName)\t\t\t\\\n> +class sensor##Factory final : public CameraSensorFactory\t\t\\\n> +{\t\t\t\t\t\t\t\t\t\\\n> +public:\t\t\t\t\t\t\t\t\t\\\n> +\tsensor##Factory() : CameraSensorFactory(entityName) {}\t\t\\\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 sensor(entity);\t\t\t\t\\\n> +\t}\t\t\t\t\t\t\t\t\\\n> +};\t\t\t\t\t\t\t\t\t\\\n> +static sensor##Factory global_##sensor##Factory\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..adef6c6703b6 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -100,7 +100,7 @@ public:\n>  \tstatic constexpr unsigned int CIO2_BUFFER_COUNT = 4;\n>  \n>  \tCIO2Device()\n> -\t\t: output_(nullptr), csi2_(nullptr), sensor_(nullptr)\n> +\t\t: output_(nullptr), csi2_(nullptr)\n>  \t{\n>  \t}\n>  \n> @@ -108,7 +108,6 @@ public:\n>  \t{\n>  \t\tdelete output_;\n>  \t\tdelete csi2_;\n> -\t\tdelete sensor_;\n>  \t}\n>  \n>  \tint init(const MediaDevice *media, unsigned int index);\n> @@ -125,7 +124,7 @@ public:\n>  \n>  \tV4L2VideoDevice *output_;\n>  \tV4L2Subdevice *csi2_;\n> -\tCameraSensor *sensor_;\n> +\tstd::unique_ptr<CameraSensor> sensor_;\n>  \n>  private:\n>  \tstd::vector<std::unique_ptr<FrameBuffer>> buffers_;\n> @@ -294,7 +293,7 @@ void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)\n>  \n>  CameraConfiguration::Status IPU3CameraConfiguration::validate()\n>  {\n> -\tconst CameraSensor *sensor = data_->cio2_.sensor_;\n> +\tconst CameraSensor *sensor = data_->cio2_.sensor_.get();\n>  \tStatus status = Valid;\n>  \n>  \tif (config_.empty())\n> @@ -1322,7 +1321,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 13433b216747..3e3b22018430 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -114,20 +114,15 @@ class RkISP1CameraData : public CameraData\n>  {\n>  public:\n>  \tRkISP1CameraData(PipelineHandler *pipe)\n> -\t\t: CameraData(pipe), sensor_(nullptr), frame_(0),\n> +\t\t: CameraData(pipe), frame_(0),\n>  \t\t  frameInfo_(pipe)\n>  \t{\n>  \t}\n>  \n> -\t~RkISP1CameraData()\n> -\t{\n> -\t\tdelete sensor_;\n> -\t}\n> -\n>  \tint loadIPA();\n>  \n>  \tStream stream_;\n> -\tCameraSensor *sensor_;\n> +\tstd::unique_ptr<CameraSensor> sensor_;\n>  \tunsigned int frame_;\n>  \tstd::vector<IPABuffer> ipaBuffers_;\n>  \tRkISP1Frames frameInfo_;\n> @@ -389,7 +384,7 @@ void RkISP1CameraData::queueFrameAction(unsigned int frame,\n>  \tcase RKISP1_IPA_ACTION_V4L2_SET: {\n>  \t\tconst ControlList &controls = action.controls[0];\n>  \t\ttimeline_.scheduleAction(std::make_unique<RkISP1ActionSetSensor>(frame,\n> -\t\t\t\t\t\t\t\t\t\t sensor_,\n> +\t\t\t\t\t\t\t\t\t\t sensor_.get(),\n>  \t\t\t\t\t\t\t\t\t\t controls));\n>  \t\tbreak;\n>  \t}\n> @@ -444,7 +439,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>  \t\t/* \\todo Add support for 8-bit greyscale to DRM formats */\n>  \t};\n>  \n> -\tconst CameraSensor *sensor = data_->sensor_;\n> +\tconst CameraSensor *sensor = data_->sensor_.get();\n>  \tStatus status = Valid;\n>  \n>  \tif (config_.empty())\n> @@ -557,7 +552,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n>  \t\tstatic_cast<RkISP1CameraConfiguration *>(c);\n>  \tRkISP1CameraData *data = cameraData(camera);\n>  \tStreamConfiguration &cfg = config->at(0);\n> -\tCameraSensor *sensor = data->sensor_;\n> +\tCameraSensor *sensor = data->sensor_.get();\n>  \tint ret;\n>  \n>  \t/*\n> @@ -907,7 +902,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 529909714bf5..6f725c6dda98 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -8,6 +8,7 @@\n>  #include <algorithm>\n>  #include <array>\n>  #include <iomanip>\n> +#include <memory>\n>  #include <tuple>\n>  \n>  #include <linux/drm_fourcc.h>\n> @@ -47,7 +48,6 @@ public:\n>  \n>  \t~VimcCameraData()\n>  \t{\n> -\t\tdelete sensor_;\n>  \t\tdelete debayer_;\n>  \t\tdelete scaler_;\n>  \t\tdelete video_;\n> @@ -57,7 +57,7 @@ public:\n>  \tint init(MediaDevice *media);\n>  \tvoid bufferReady(FrameBuffer *buffer);\n>  \n> -\tCameraSensor *sensor_;\n> +\tstd::unique_ptr<CameraSensor> sensor_;\n>  \tV4L2Subdevice *debayer_;\n>  \tV4L2Subdevice *scaler_;\n>  \tV4L2VideoDevice *video_;\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..8709e481dc7b 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> @@ -100,15 +100,10 @@ protected:\n>  \t\treturn TestPass;\n>  \t}\n>  \n> -\tvoid cleanup()\n> -\t{\n> -\t\tdelete sensor_;\n> -\t}\n> -\n>  private:\n>  \tstd::unique_ptr<DeviceEnumerator> enumerator_;\n>  \tstd::shared_ptr<MediaDevice> media_;\n> -\tCameraSensor *sensor_;\n> +\tstd::unique_ptr<CameraSensor> sensor_;\n>  };\n>  \n>  TEST_REGISTER(CameraSensorTest)\n> diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.cpp b/test/v4l2_videodevice/v4l2_videodevice_test.cpp\n> index 577da4cb601c..ae038de74864 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> @@ -97,6 +97,5 @@ void V4L2VideoDeviceTest::cleanup()\n>  \tcapture_->close();\n>  \n>  \tdelete debayer_;\n> -\tdelete sensor_;\n>  \tdelete capture_;\n>  }\n> diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.h b/test/v4l2_videodevice/v4l2_videodevice_test.h\n> index 9acaceb84fe0..f635e0929efe 100644\n> --- a/test/v4l2_videodevice/v4l2_videodevice_test.h\n> +++ b/test/v4l2_videodevice/v4l2_videodevice_test.h\n> @@ -25,8 +25,8 @@ class V4L2VideoDeviceTest : public Test\n>  {\n>  public:\n>  \tV4L2VideoDeviceTest(const char *driver, const char *entity)\n> -\t\t: driver_(driver), entity_(entity), sensor_(nullptr),\n> -\t\t  debayer_(nullptr), capture_(nullptr)\n> +\t\t: driver_(driver), entity_(entity), debayer_(nullptr),\n> +\t\t  capture_(nullptr)\n>  \t{\n>  \t}\n>  \n> @@ -38,7 +38,7 @@ protected:\n>  \tstd::string entity_;\n>  \tstd::unique_ptr<DeviceEnumerator> enumerator_;\n>  \tstd::shared_ptr<MediaDevice> media_;\n> -\tCameraSensor *sensor_;\n> +\tstd::unique_ptr<CameraSensor> sensor_;\n>  \tV4L2Subdevice *debayer_;\n>  \tV4L2VideoDevice *capture_;\n>  \tstd::vector<std::unique_ptr<FrameBuffer>> buffers_;\n>","headers":{"Return-Path":"<kieran.bingham@ideasonboard.com>","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 006E960411\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 24 Mar 2020 16:37:26 +0100 (CET)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4C53A308;\n\tTue, 24 Mar 2020 16:37:26 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1585064246;\n\tbh=+n6usEXlEzy3B48zdhWIGc/HLRTbtsc+iQhHXSV1MCY=;\n\th=Reply-To:Subject:To:References:Cc:From:Date:In-Reply-To:From;\n\tb=MQpE/IHsbmhMA6eWC+mmXKAhimPj47QIgYyHWWLhLZslr+Q7QLz9v28QIaDGCQj07\n\tuJUhreeBKMZXqD4waMPUZOxmEV5Qmgu0RyD07y5JONzjejmkgVETjr2vFW1SsSnUg4\n\t4e8vTsE5rfmolUhPx9/je6fMmw5jnhoM+dYYqvZA=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Jacopo Mondi <jacopo@jmondi.org>, libcamera-devel@lists.libcamera.org","References":"<20200309180444.725757-1-jacopo@jmondi.org>\n\t<20200309180444.725757-4-jacopo@jmondi.org>","Cc":"Kaaira Gupta <kgupta@es.iitr.ac.in>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<0df53dcb-5963-15e2-d2ef-9ffff4e1dc66@ideasonboard.com>","Date":"Tue, 24 Mar 2020 15:37:21 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.9.1","MIME-Version":"1.0","In-Reply-To":"<20200309180444.725757-4-jacopo@jmondi.org>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v3 3/6] libcamera: camera_sensor:\n\tIntroduce CameraSensorFactory","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Tue, 24 Mar 2020 15:37:27 -0000"}},{"id":4302,"web_url":"https://patchwork.libcamera.org/comment/4302/","msgid":"<20200326123047.r7lzx56d32bwd7bd@uno.localdomain>","date":"2020-03-26T12:31:08","subject":"Re: [libcamera-devel] [PATCH v3 3/6] 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 Kieran\n\nOn Tue, Mar 24, 2020 at 03:37:21PM +0000, Kieran Bingham wrote:\n> Hi Jacopo,\n>\n> On 09/03/2020 18:04, 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 camera sensor sub-classes.\n> >\n> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n>\n> Other than documentation fixups:\n>\n> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n>\n> >\n> > ---\n> > v2 -> v3:\n> > - Update documentation\n> > - Register sensor name as REGISTER_CAMERA_SENSOR() second parameter\n> > - Create camera sensor as unique_ptr\n> > - Match entity name and key with std::string::find() in\n> >   CameraSensorFactory::createSensor()\n> > ---\n> >  src/libcamera/camera_sensor.cpp               | 136 ++++++++++++++++++\n> >  src/libcamera/include/camera_sensor.h         |  32 +++++\n> >  src/libcamera/pipeline/ipu3/ipu3.cpp          |   9 +-\n> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  17 +--\n> >  src/libcamera/pipeline/vimc.cpp               |   6 +-\n> >  test/camera-sensor.cpp                        |   9 +-\n> >  .../v4l2_videodevice_test.cpp                 |   3 +-\n> >  test/v4l2_videodevice/v4l2_videodevice_test.h |   6 +-\n> >  8 files changed, 187 insertions(+), 31 deletions(-)\n> >\n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index 2219a4307436..8bfe02c9e8ff 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -12,6 +12,8 @@\n> >  #include <iomanip>\n> >  #include <limits.h>\n> >  #include <math.h>\n> > +#include <memory>\n> > +#include <string.h>\n> >\n> >  #include <libcamera/property_ids.h>\n> >\n> > @@ -366,4 +368,138 @@ std::string CameraSensor::logPrefix() const\n> >  \treturn \"'\" + subdev_->entity()->name() + \"'\";\n> >  }\n> >\n> > +/**\n> > + * \\class CameraSensorFactory\n> > + * \\brief Factory of camera sensors\n> > + *\n> > + * The class provides to camera sensors the ability to register themselves to\n>\n> This class provides camera sensors with the ability to...\n>\n>\n> > + * the factory to allow the creation of their instances by matching the name of\n> > + * 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>\n> /creates/create/\n>\n> > + * CameraSensorFactory::createInstance() method, which returns a pointer\n> > + * to the opportune CameraSensor sub-class if any, or a newly created instance\n>\n> opportune? I'm not sure that's helpful there...\n>\n>\n> \"which returns a pointer to the existing CameraSensor sub-class if it\n> exists, or a newly created ...\"\n>\n>\n>\n> > + * of the generic CameraSensor class.\n> > + */\n> > +\n> > +/**\n> > + * \\brief Construct a camera sensor factory\n> > + * \\param[in] name The name of the media entity that represents 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> (Trying to figure out how to say this...., the 'handler' verb seems out\n> of place to me I'm afraid - and makes me think it's something else...)\n>\n> Register a new camera sensor factory which creates CameraSensor\n> instances to support camera sensors represented by the media entity with\n> \\a name.\n>\n> > + *\n> > + * Creating an instance of the factory registers it with the global list of\n> > + * factories, accessible through the factories() function.\n> > + */\n> > +CameraSensorFactory::CameraSensorFactory(const char *name)\n> > +{\n> > +\tregisterFactory(name, this);\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>\n> /ensures/ensure/\n>\n\nAck on all the above ones\n\n> > + * initialized on first use, without any dependency on link order.\n>\n> That's an implementation detail, shouldn't that comment go 'in' the\n> function rather than in the documentation of the function?\n>\n> > + *\n> > + * \\return The static list of registered camera sensor factories\n>\n> Do we need to state that it's a static list? That makes me think that it\n> can't change ...\n>\n\nThose two are (iirc) copied from the pipeline handler factory.\nI would keep the two identical and fix in one go eventually\n\nThanks\n  j\n\n> > + */\n> > +CameraSensorFactory::FactoriesMap &CameraSensorFactory::factories()\n> > +{\n> > +\tstatic FactoriesMap factories;\n> > +\treturn factories;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Register a camera sensor 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 camera sensor \\a factory is registered and associated with an entity \\a\n> > + * name. When a camera sensor is created for a media entity with the\n> > + * createSensor() method, the name of the media entity there provided is used\n> > + * to select the corresponding factory.\n>\n> /name/\\a name/ ? Or does doxygen automatically match...\n> /there//\n>\n> > + *\n> > + * The caller is responsible for guaranteeing the uniqueness of the\n> > + * camera sensor entity name.\n> > + */\n> > +void CameraSensorFactory::registerFactory(const char *name,\n> > +\t\t\t\t\t  CameraSensorFactory *factory)\n> > +{\n> > +\tFactoriesMap &map = factories();\n> > +\n> > +\tif (map.find(name) != map.end()) {\n> > +\t\tLOG(CameraSensor, Error)\n> > +\t\t\t<< \"Unable to register camera sensor factory '\"\n> > +\t\t\t<< name << \"': already registered\";\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tmap[name] = factory;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Create a camera sensor corresponding to \\a entity\n> > + * \\param[in] entity The media entity that represents the sensor\n> > + *\n> > + * Create a new instance of a CameraSensor subclass for the sensor represented\n> > + * by \\a entity using one of the registered factories. If no specific class is\n> > + * available for the sensor, an instance of the generic CameraSensor class is\n> > + * created and returned.\n> > + *\n> > + * Ownership of the created camera sensor instance is passed to the caller as\n> > + * a unique pointer.\n> > + *\n> > + * \\return A unique pointer to the newly created camera sensor instance\n> > + */\n> > +std::unique_ptr<CameraSensor> CameraSensorFactory::createSensor(const MediaEntity *entity)\n> > +{\n> > +\tconst std::string &name = entity->name();\n> > +\tfor (const auto &it : factories()) {\n> > +\t\tconst std::string &key = it.first;\n> > +\n> > +\t\t/*\n> > +\t\t * Match the name of the entity with the keys in the factories\n> > +\t\t * map. The factories map keys are provided by the CameraSensor\n> > +\t\t * sub-class at factory registration time as the sensor\n> > +\t\t * entity name.\n> > +\t\t *\n> > +\t\t * To support both canonical sensor entity names (in the\n> > +\t\t * \"devname i2c-adapt:i2c-addr\" format) and non-canonical sensor\n> > +\t\t * names such (as VIMC's \"Sensor B\" ones), search for the key\n> > +\t\t * in the entity name.\n> > +\t\t *\n> > +\t\t * \\todo Delegate matching to the CameraSensor sub-class\n> > +\t\t * to support more complex matching criteria.\n> > +\t\t */\n> > +\t\tif (name.find(key) == std::string::npos)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tLOG(CameraSensor, Info) << \"Create camera sensor for '\"\n> > +\t\t\t\t\t<< name << \"'\";\n> > +\n> > +\t\tCameraSensorFactory *factory = it.second;\n> > +\t\treturn std::unique_ptr<CameraSensor>(factory->create(entity));\n> > +\t}\n> > +\n> > +\tLOG(CameraSensor, Info) << \"Unsupported sensor '\" << entity->name()\n> > +\t\t\t\t<< \"': using generic camera sensor\";\n> > +\treturn std::make_unique<CameraSensor>(entity);\n> > +}\n> > +\n> > +/**\n> > + * \\def REGISTER_CAMERA_SENSOR(sensor, entityName)\n> > + * \\brief Register camera sensor \\a name with the sensor factory\n>\n> Should that be \"\\a entityName\" ?\n>\n>\n> > + * \\param[in] sensor The name of the camera sensor class to register\n> > + * \\param[in] entityName The name of the image sensor supported by the factory\n> > + *\n> > + * Register camera sensor factory \\a sensor with the global list of factories.\n>\n> Register \\a sensor as a camera sensor factory in the global list of\n> factories.\n>\n>\n> > + * The camera sensor factory supports image sensors identified by \\a entityName.\n> > + * The sensor name reported by the media entity that identifies it is usually\n> > + * composed by the here provided \\a entityName and the I2C bus and device ids.\n>\n> usually composed of the \\a entityName and the I2C bus and device ids.\n>\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..633955591b36 100644\n> > --- a/src/libcamera/include/camera_sensor.h\n> > +++ b/src/libcamera/include/camera_sensor.h\n> > @@ -7,6 +7,8 @@\n> >  #ifndef __LIBCAMERA_CAMERA_SENSOR_H__\n> >  #define __LIBCAMERA_CAMERA_SENSOR_H__\n> >\n> > +#include <map>\n> > +#include <memory>\n> >  #include <string>\n> >  #include <vector>\n> >\n> > @@ -61,6 +63,36 @@ private:\n> >  \tControlList properties_;\n> >  };\n> >\n> > +class CameraSensorFactory\n> > +{\n> > +public:\n> > +\tCameraSensorFactory(const char *name);\n> > +\tvirtual ~CameraSensorFactory() {}\n> > +\n> > +\tstatic std::unique_ptr<CameraSensor> createSensor(const MediaEntity *entity);\n> > +\n> > +private:\n> > +\tusing FactoriesMap = std::map<const std::string, CameraSensorFactory *>;\n> > +\tstatic FactoriesMap &factories();\n> > +\tstatic void registerFactory(const char *name,\n> > +\t\t\t\t    CameraSensorFactory *factory);\n> > +\tvirtual CameraSensor *create(const MediaEntity *entity) = 0;\n> > +};\n> > +\n> > +#define REGISTER_CAMERA_SENSOR(sensor, entityName)\t\t\t\\\n> > +class sensor##Factory final : public CameraSensorFactory\t\t\\\n> > +{\t\t\t\t\t\t\t\t\t\\\n> > +public:\t\t\t\t\t\t\t\t\t\\\n> > +\tsensor##Factory() : CameraSensorFactory(entityName) {}\t\t\\\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 sensor(entity);\t\t\t\t\\\n> > +\t}\t\t\t\t\t\t\t\t\\\n> > +};\t\t\t\t\t\t\t\t\t\\\n> > +static sensor##Factory global_##sensor##Factory\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..adef6c6703b6 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -100,7 +100,7 @@ public:\n> >  \tstatic constexpr unsigned int CIO2_BUFFER_COUNT = 4;\n> >\n> >  \tCIO2Device()\n> > -\t\t: output_(nullptr), csi2_(nullptr), sensor_(nullptr)\n> > +\t\t: output_(nullptr), csi2_(nullptr)\n> >  \t{\n> >  \t}\n> >\n> > @@ -108,7 +108,6 @@ public:\n> >  \t{\n> >  \t\tdelete output_;\n> >  \t\tdelete csi2_;\n> > -\t\tdelete sensor_;\n> >  \t}\n> >\n> >  \tint init(const MediaDevice *media, unsigned int index);\n> > @@ -125,7 +124,7 @@ public:\n> >\n> >  \tV4L2VideoDevice *output_;\n> >  \tV4L2Subdevice *csi2_;\n> > -\tCameraSensor *sensor_;\n> > +\tstd::unique_ptr<CameraSensor> sensor_;\n> >\n> >  private:\n> >  \tstd::vector<std::unique_ptr<FrameBuffer>> buffers_;\n> > @@ -294,7 +293,7 @@ void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)\n> >\n> >  CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> >  {\n> > -\tconst CameraSensor *sensor = data_->cio2_.sensor_;\n> > +\tconst CameraSensor *sensor = data_->cio2_.sensor_.get();\n> >  \tStatus status = Valid;\n> >\n> >  \tif (config_.empty())\n> > @@ -1322,7 +1321,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 13433b216747..3e3b22018430 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -114,20 +114,15 @@ class RkISP1CameraData : public CameraData\n> >  {\n> >  public:\n> >  \tRkISP1CameraData(PipelineHandler *pipe)\n> > -\t\t: CameraData(pipe), sensor_(nullptr), frame_(0),\n> > +\t\t: CameraData(pipe), frame_(0),\n> >  \t\t  frameInfo_(pipe)\n> >  \t{\n> >  \t}\n> >\n> > -\t~RkISP1CameraData()\n> > -\t{\n> > -\t\tdelete sensor_;\n> > -\t}\n> > -\n> >  \tint loadIPA();\n> >\n> >  \tStream stream_;\n> > -\tCameraSensor *sensor_;\n> > +\tstd::unique_ptr<CameraSensor> sensor_;\n> >  \tunsigned int frame_;\n> >  \tstd::vector<IPABuffer> ipaBuffers_;\n> >  \tRkISP1Frames frameInfo_;\n> > @@ -389,7 +384,7 @@ void RkISP1CameraData::queueFrameAction(unsigned int frame,\n> >  \tcase RKISP1_IPA_ACTION_V4L2_SET: {\n> >  \t\tconst ControlList &controls = action.controls[0];\n> >  \t\ttimeline_.scheduleAction(std::make_unique<RkISP1ActionSetSensor>(frame,\n> > -\t\t\t\t\t\t\t\t\t\t sensor_,\n> > +\t\t\t\t\t\t\t\t\t\t sensor_.get(),\n> >  \t\t\t\t\t\t\t\t\t\t controls));\n> >  \t\tbreak;\n> >  \t}\n> > @@ -444,7 +439,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> >  \t\t/* \\todo Add support for 8-bit greyscale to DRM formats */\n> >  \t};\n> >\n> > -\tconst CameraSensor *sensor = data_->sensor_;\n> > +\tconst CameraSensor *sensor = data_->sensor_.get();\n> >  \tStatus status = Valid;\n> >\n> >  \tif (config_.empty())\n> > @@ -557,7 +552,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> >  \t\tstatic_cast<RkISP1CameraConfiguration *>(c);\n> >  \tRkISP1CameraData *data = cameraData(camera);\n> >  \tStreamConfiguration &cfg = config->at(0);\n> > -\tCameraSensor *sensor = data->sensor_;\n> > +\tCameraSensor *sensor = data->sensor_.get();\n> >  \tint ret;\n> >\n> >  \t/*\n> > @@ -907,7 +902,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 529909714bf5..6f725c6dda98 100644\n> > --- a/src/libcamera/pipeline/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc.cpp\n> > @@ -8,6 +8,7 @@\n> >  #include <algorithm>\n> >  #include <array>\n> >  #include <iomanip>\n> > +#include <memory>\n> >  #include <tuple>\n> >\n> >  #include <linux/drm_fourcc.h>\n> > @@ -47,7 +48,6 @@ public:\n> >\n> >  \t~VimcCameraData()\n> >  \t{\n> > -\t\tdelete sensor_;\n> >  \t\tdelete debayer_;\n> >  \t\tdelete scaler_;\n> >  \t\tdelete video_;\n> > @@ -57,7 +57,7 @@ public:\n> >  \tint init(MediaDevice *media);\n> >  \tvoid bufferReady(FrameBuffer *buffer);\n> >\n> > -\tCameraSensor *sensor_;\n> > +\tstd::unique_ptr<CameraSensor> sensor_;\n> >  \tV4L2Subdevice *debayer_;\n> >  \tV4L2Subdevice *scaler_;\n> >  \tV4L2VideoDevice *video_;\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..8709e481dc7b 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> > @@ -100,15 +100,10 @@ protected:\n> >  \t\treturn TestPass;\n> >  \t}\n> >\n> > -\tvoid cleanup()\n> > -\t{\n> > -\t\tdelete sensor_;\n> > -\t}\n> > -\n> >  private:\n> >  \tstd::unique_ptr<DeviceEnumerator> enumerator_;\n> >  \tstd::shared_ptr<MediaDevice> media_;\n> > -\tCameraSensor *sensor_;\n> > +\tstd::unique_ptr<CameraSensor> sensor_;\n> >  };\n> >\n> >  TEST_REGISTER(CameraSensorTest)\n> > diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.cpp b/test/v4l2_videodevice/v4l2_videodevice_test.cpp\n> > index 577da4cb601c..ae038de74864 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> > @@ -97,6 +97,5 @@ void V4L2VideoDeviceTest::cleanup()\n> >  \tcapture_->close();\n> >\n> >  \tdelete debayer_;\n> > -\tdelete sensor_;\n> >  \tdelete capture_;\n> >  }\n> > diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.h b/test/v4l2_videodevice/v4l2_videodevice_test.h\n> > index 9acaceb84fe0..f635e0929efe 100644\n> > --- a/test/v4l2_videodevice/v4l2_videodevice_test.h\n> > +++ b/test/v4l2_videodevice/v4l2_videodevice_test.h\n> > @@ -25,8 +25,8 @@ class V4L2VideoDeviceTest : public Test\n> >  {\n> >  public:\n> >  \tV4L2VideoDeviceTest(const char *driver, const char *entity)\n> > -\t\t: driver_(driver), entity_(entity), sensor_(nullptr),\n> > -\t\t  debayer_(nullptr), capture_(nullptr)\n> > +\t\t: driver_(driver), entity_(entity), debayer_(nullptr),\n> > +\t\t  capture_(nullptr)\n> >  \t{\n> >  \t}\n> >\n> > @@ -38,7 +38,7 @@ protected:\n> >  \tstd::string entity_;\n> >  \tstd::unique_ptr<DeviceEnumerator> enumerator_;\n> >  \tstd::shared_ptr<MediaDevice> media_;\n> > -\tCameraSensor *sensor_;\n> > +\tstd::unique_ptr<CameraSensor> sensor_;\n> >  \tV4L2Subdevice *debayer_;\n> >  \tV4L2VideoDevice *capture_;\n> >  \tstd::vector<std::unique_ptr<FrameBuffer>> buffers_;\n> >\n>\n> --\n> Regards\n> --\n> Kieran","headers":{"Return-Path":"<jacopo@jmondi.org>","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 2CA5160410\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 26 Mar 2020 13:28:16 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id 2A86F200009;\n\tThu, 26 Mar 2020 12:28:09 +0000 (UTC)"],"Date":"Thu, 26 Mar 2020 13:31:08 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org, Kaaira Gupta <kgupta@es.iitr.ac.in>","Message-ID":"<20200326123047.r7lzx56d32bwd7bd@uno.localdomain>","References":"<20200309180444.725757-1-jacopo@jmondi.org>\n\t<20200309180444.725757-4-jacopo@jmondi.org>\n\t<0df53dcb-5963-15e2-d2ef-9ffff4e1dc66@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<0df53dcb-5963-15e2-d2ef-9ffff4e1dc66@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 3/6] libcamera: camera_sensor:\n\tIntroduce CameraSensorFactory","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Thu, 26 Mar 2020 12:28:16 -0000"}}]