[{"id":32023,"web_url":"https://patchwork.libcamera.org/comment/32023/","msgid":"<173080068478.3353069.6161875712529014753@ping.linuxembedded.co.uk>","date":"2024-11-05T09:58:04","subject":"Re: [PATCH 1/3] libcamera: camera_sensor: Introduce\n\tCameraSensorFactory","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2024-10-22 15:53:13)\n> From: Jacopo Mondi \n> \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> Signed-off-by: Jacopo Mondi \n> Signed-off-by: Laurent Pinchart \n> Reviewed-by: Stefan Klug \n> ---\n> include/libcamera/internal/camera_sensor.h | 48 +++++-\n> src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 9 +-\n> src/libcamera/pipeline/ipu3/cio2.cpp | 7 +-\n> src/libcamera/pipeline/mali-c55/mali-c55.cpp | 5 +-\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 +-\n> .../pipeline/rpi/common/pipeline_base.cpp | 5 +-\n> src/libcamera/pipeline/simple/simple.cpp | 9 +-\n> src/libcamera/pipeline/vimc/vimc.cpp | 7 +-\n> src/libcamera/sensor/camera_sensor.cpp | 162 ++++++++++++++++++\n> test/camera-sensor.cpp | 7 +-\n> .../v4l2_videodevice_test.cpp | 5 +-\n> test/v4l2_videodevice/v4l2_videodevice_test.h | 2 +-\n> 12 files changed, 233 insertions(+), 40 deletions(-)\n> \n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index a42c15fa37ce..2d6cb697a870 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -38,7 +38,6 @@ enum class Orientation;\n> class CameraSensor : protected Loggable\n> {\n> public:\n> - explicit CameraSensor(const MediaEntity *entity);\n> ~CameraSensor();\n> \n> int init();\n> @@ -81,6 +80,7 @@ public:\n> int setTestPatternMode(controls::draft::TestPatternModeEnum mode);\n> \n> protected:\n> + explicit CameraSensor(const MediaEntity *entity);\n> std::string logPrefix() const override;\n> \n> private:\n> @@ -122,4 +122,50 @@ private:\n> std::unique_ptr focusLens_;\n> };\n> \n> +class CameraSensorFactoryBase\n\nSeems I don't have much to comment on here so I might be down to\nbikeshedding!\n\nHaving read through the usages of the CameraSensorFactoryBase, I would\nbe tempted to rename this class to:\n\n\tCameraSensorFactory\n\n\n> +{\n> +public:\n> + CameraSensorFactoryBase();\n> + virtual ~CameraSensorFactoryBase() = default;\n> +\n> + static std::unique_ptr create(MediaEntity *entity);\n> +\n> +private:\n> + LIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorFactoryBase)\n> +\n> + static std::vector &factories();\n> +\n> + static void registerFactory(CameraSensorFactoryBase *factory);\n> +\n> + virtual bool match(const MediaEntity *entity) const = 0;\n> +\n> + virtual std::unique_ptr\n> + createInstance(MediaEntity *entity) const = 0;\n> +};\n> +\n> +template\n> +class CameraSensorFactory final : public CameraSensorFactoryBase\n\nAnd this one to CameraSensorFactoryType (or other name as appropriate)\n\n> +{\n> +public:\n> + CameraSensorFactory()\n> + : CameraSensorFactoryBase()\n> + {\n> + }\n> +\n> +private:\n> + bool match(const MediaEntity *entity) const override\n> + {\n> + return _CameraSensor::match(entity);\n> + }\n> +\n> + std::unique_ptr\n> + createInstance(MediaEntity *entity) const override\n> + {\n> + return _CameraSensor::create(entity);\n> + }\n> +};\n> +\n> +#define REGISTER_CAMERA_SENSOR(sensor) \\\n> +static CameraSensorFactory global_##sensor##Factory{};\n\nThis I think would be the only rename/usage of the simple name\nCameraSensorFactory currently so the only place to be renamed to\nCameraSensorFactoryType ?\n\n> +\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> index 72aa6c75608a..4e66b3368d5a 100644\n> --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> @@ -157,11 +157,10 @@ PipelineHandlerISI *ISICameraData::pipe()\n> /* Open and initialize pipe components. */\n> int ISICameraData::init()\n> {\n> - int ret = sensor_->init();\n> - if (ret)\n> - return ret;\n> + if (!sensor_)\n> + return -ENODEV;\n> \n> - ret = csis_->open();\n> + int ret = csis_->open();\n> if (ret)\n> return ret;\n> \n> @@ -1057,7 +1056,7 @@ bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)\n> std::unique_ptr data =\n> std::make_unique(this);\n> \n> - data->sensor_ = std::make_unique(sensor);\n> + data->sensor_ = CameraSensorFactoryBase::create(sensor);\n\nSo that the users of the Factory create read as :\n\n\tdata->sensor_ = CameraSensorFactory::create(sensor);\n\n\n\n\n> data->csis_ = std::make_unique(csi);\n> data->xbarSink_ = sink;\n> \n> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> index 74a5d93f88ae..aa544d7b0303 100644\n> --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> @@ -134,10 +134,9 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)\n> \n> MediaLink *link = links[0];\n> MediaEntity *sensorEntity = link->source()->entity();\n> - sensor_ = std::make_unique(sensorEntity);\n> - ret = sensor_->init();\n> - if (ret)\n> - return ret;\n> + sensor_ = CameraSensorFactoryBase::create(sensorEntity);\n> + if (!sensor_)\n> + return -ENODEV;\n\nI like that the construction is now simplified and there's no need to\ncreate and then initialise the Sensor class in every usage...\n\n> \n> ret = link->setEnabled(true);\n> if (ret)\n> diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n> index 45c71c1dd619..4b71e0dad15e 100644\n> --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n> +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n> @@ -141,9 +141,8 @@ int MaliC55CameraData::init()\n> * Register a CameraSensor if we connect to a sensor and create\n> * an entity for the connected CSI-2 receiver.\n> */\n> - sensor_ = std::make_unique(entity_);\n> - ret = sensor_->init();\n> - if (ret)\n> + sensor_ = CameraSensorFactoryBase::create(entity_);\n> + if (!sensor_)\n> return ret;\n> \n> const MediaPad *sourcePad = entity_->getPadByIndex(0);\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index d8d1d65fbbf9..95c25e976015 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -1122,10 +1122,9 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> std::make_unique(this, &mainPath_,\n> hasSelfPath_ ? &selfPath_ : nullptr);\n> \n> - data->sensor_ = std::make_unique(sensor);\n> - ret = data->sensor_->init();\n> - if (ret)\n> - return ret;\n> + data->sensor_ = CameraSensorFactoryBase::create(sensor);\n> + if (!data->sensor_)\n> + return -ENODEV;\n> \n> /* Initialize the camera properties. */\n> data->properties_ = data->sensor_->properties();\n> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> index 3041fd1ed9fd..4f56bd33df05 100644\n> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> @@ -774,13 +774,10 @@ int PipelineHandlerBase::registerCamera(std::unique_ptr &camera\n> CameraData *data = cameraData.get();\n> int ret;\n> \n> - data->sensor_ = std::make_unique(sensorEntity);\n> + data->sensor_ = CameraSensorFactoryBase::create(sensorEntity);\n> if (!data->sensor_)\n> return -EINVAL;\n> \n> - if (data->sensor_->init())\n> - return -EINVAL;\n> -\n> /* Populate the map of sensor supported formats and sizes. */\n> for (auto const mbusCode : data->sensor_->mbusCodes())\n> data->sensorFormats_.emplace(mbusCode,\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 3ddce71d3757..67f583b8a22c 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -388,8 +388,6 @@ SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,\n> MediaEntity *sensor)\n> : Camera::Private(pipe), streams_(numStreams)\n> {\n> - int ret;\n> -\n> /*\n> * Find the shortest path from the camera sensor to a video capture\n> * device using the breadth-first search algorithm. This heuristic will\n> @@ -480,12 +478,9 @@ SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,\n> }\n> \n> /* Finally also remember the sensor. */\n> - sensor_ = std::make_unique(sensor);\n> - ret = sensor_->init();\n> - if (ret) {\n> - sensor_.reset();\n> + sensor_ = CameraSensorFactoryBase::create(sensor);\n> + if (!sensor_)\n> return;\n> - }\n> \n> LOG(SimplePipeline, Debug)\n> << \"Found pipeline: \"\n> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> index 2165bae890cb..2e474133439c 100644\n> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> @@ -532,10 +532,9 @@ int VimcCameraData::init()\n> return ret;\n> \n> /* Create and open the camera sensor, debayer, scaler and video device. */\n> - sensor_ = std::make_unique(media_->getEntityByName(\"Sensor B\"));\n> - ret = sensor_->init();\n> - if (ret)\n> - return ret;\n> + sensor_ = CameraSensorFactoryBase::create(media_->getEntityByName(\"Sensor B\"));\n> + if (!sensor_)\n> + return -ENODEV;\n> \n> debayer_ = V4L2Subdevice::fromEntityName(media_, \"Debayer B\");\n> if (debayer_->open())\n> diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp\n> index 1b224f1989fe..025c9eefdd12 100644\n> --- a/src/libcamera/sensor/camera_sensor.cpp\n> +++ b/src/libcamera/sensor/camera_sensor.cpp\n> @@ -11,6 +11,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> \n> #include \n> @@ -1202,4 +1203,165 @@ std::string CameraSensor::logPrefix() const\n> return \"'\" + entity_->name() + \"'\";\n> }\n> \n> +namespace {\n> +\n> +/* Transitory default camera sensor implementation */\n> +class CameraSensorDefault : public CameraSensor\n> +{\n> +public:\n> + CameraSensorDefault(MediaEntity *entity)\n> + : CameraSensor(entity)\n> + {\n> + }\n> +\n> + static bool match([[maybe_unused]] const MediaEntity *entity)\n> + {\n\nDo we need any kind of debug/warning/log in here to say its' using a\ntemporary default wrapper? (Only thinking here in the event that we keep\nthis as a lowest priority factory ... see comment below in\nCameraSensorFactoryBase::create())\n\n> + return true;\n> + }\n> +\n> + static std::unique_ptr create(MediaEntity *entity)\n> + {\n> + std::unique_ptr sensor =\n> + std::make_unique(entity);\n> +\n> + if (sensor->init())\n> + return nullptr;\n> +\n> + return sensor;\n> + }\n> +};\n> +\n> +REGISTER_CAMERA_SENSOR(CameraSensorDefault)\n> +\n> +}; /* namespace */\n> +\n> +/**\n> + * \\class CameraSensorFactoryBase\n> + * \\brief Base class for camera sensor factories\n> + *\n> + * The CameraSensorFactoryBase class is the base of all specializations of\n> + * the CameraSensorFactory class template. It implements the factory\n> + * registration, maintains a registry of factories, and provides access to the\n> + * registered factories.\n> + */\n> +\n> +/**\n> + * \\brief Construct a camera sensor factory base\n> + *\n> + * Creating an instance of the factory base registers it with the global list of\n> + * factories, accessible through the factories() function.\n> + */\n> +CameraSensorFactoryBase::CameraSensorFactoryBase()\n> +{\n> + registerFactory(this);\n> +}\n> +\n> +/**\n> + * \\brief Create an instance of the CameraSensor corresponding to a media entity\n> + * \\param[in] entity The media entity on the source end of the sensor\n> + *\n> + * \\return A unique pointer to a new instance of the CameraSensor subclass\n> + * matching the entity, or a null pointer if no such factory exists\n> + */\n> +std::unique_ptr CameraSensorFactoryBase::create(MediaEntity *entity)\n> +{\n> + const std::vector &factories =\n> + CameraSensorFactoryBase::factories();\n> +\n> + for (const CameraSensorFactoryBase *factory : factories) {\n\nWill the factories need to be sorted in any priority order ? Do we\nanticipate multiple Factory classes could match ? (I.e. if the\ntransitory default were to be kept for instance?)\n\nFine to handle that when required of course...\n\nBike shedding aside, there's nothing in here that scares me so:\n\n\nReviewed-by: Kieran Bingham \n\n\n> + if (!factory->match(entity))\n> + continue;\n> +\n> + std::unique_ptr sensor = factory->createInstance(entity);\n> + if (!sensor) {\n> + LOG(CameraSensor, Error)\n> + << \"Failed to create sensor for '\"\n> + << entity->name();\n> + return nullptr;\n> + }\n> +\n> + return sensor;\n> + }\n> +\n> + return nullptr;\n> +}\n> +\n> +/**\n> + * \\brief Retrieve the list of all camera sensor factories\n> + * \\return The list of camera sensor factories\n> + */\n> +std::vector &CameraSensorFactoryBase::factories()\n> +{\n> + /*\n> + * The static factories map is defined inside the function to ensure\n> + * it gets initialized on first use, without any dependency on link\n> + * order.\n> + */\n> + static std::vector factories;\n> + return factories;\n> +}\n> +\n> +/**\n> + * \\brief Add a camera sensor class to the registry\n> + * \\param[in] factory Factory to use to construct the camera sensor\n> + */\n> +void CameraSensorFactoryBase::registerFactory(CameraSensorFactoryBase *factory)\n> +{\n> + std::vector &factories =\n> + CameraSensorFactoryBase::factories();\n> +\n> + factories.push_back(factory);\n> +}\n> +\n> +/**\n> + * \\class CameraSensorFactory\n> + * \\brief Registration of CameraSensorFactory classes and creation of instances\n> + * \\tparam _CameraSensor The camera sensor class type for this factory\n> + *\n> + * To facilitate discovery and instantiation of CameraSensor classes, the\n> + * CameraSensorFactory class implements auto-registration of camera sensors.\n> + * Each CameraSensor subclass shall register itself using the\n> + * REGISTER_CAMERA_SENSOR() macro, which will create a corresponding instance\n> + * of a CameraSensorFactory subclass and register it with the static list of\n> + * factories.\n> + */\n> +\n> +/**\n> + * \\fn CameraSensorFactory::CameraSensorFactory()\n> + * \\brief Construct a camera sensor factory\n> + *\n> + * Creating an instance of the factory registers it with the global list of\n> + * factories, accessible through the CameraSensorFactoryBase::factories()\n> + * function.\n> + */\n> +\n> +/**\n> + * \\fn CameraSensorFactory::createInstance() const\n> + * \\brief Create an instance of the CameraSensor corresponding to the factory\n> + *\n> + * \\return A unique pointer to a newly constructed instance of the CameraSensor\n> + * subclass corresponding to the factory\n> + */\n> +\n> +/**\n> + * \\def REGISTER_CAMERA_SENSOR(sensor)\n> + * \\brief Register a camera sensor type to the sensor factory\n> + * \\param[in] sensor Class name of the CameraSensor derived class to register\n> + *\n> + * Register a CameraSensor subclass with the factory and make it available to\n> + * try and match sensors. The subclass needs to implement two static functions:\n> + *\n> + * \\code{.cpp}\n> + * static bool match(const MediaEntity *entity);\n> + * static std::unique_ptr create(MediaEntity *entity);\n> + * \\endcode\n> + *\n> + * The match() function tests if the sensor class supports the camera sensor\n> + * identified by a MediaEntity.\n> + *\n> + * The create() function creates a new instance of the sensor class. It may\n> + * return a null pointer if initialization of the instance fails. It will only\n> + * be called if the match() function has returned true for the given entity.\n> + */\n> +\n> } /* namespace libcamera */\n> diff --git a/test/camera-sensor.cpp b/test/camera-sensor.cpp\n> index 1d402c43355b..869c788965da 100644\n> --- a/test/camera-sensor.cpp\n> +++ b/test/camera-sensor.cpp\n> @@ -52,8 +52,8 @@ protected:\n> return TestFail;\n> }\n> \n> - sensor_ = new CameraSensor(entity);\n> - if (sensor_->init() < 0) {\n> + sensor_ = CameraSensorFactoryBase::create(entity);\n> + if (!sensor_) {\n> cerr << \"Unable to initialise camera sensor\" << endl;\n> return TestFail;\n> }\n> @@ -118,13 +118,12 @@ protected:\n> \n> void cleanup()\n> {\n> - delete sensor_;\n> }\n> \n> private:\n> std::unique_ptr enumerator_;\n> std::shared_ptr media_;\n> - CameraSensor *sensor_;\n> + std::unique_ptr sensor_;\n> CameraLens *lens_;\n> };\n> \n> diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.cpp b/test/v4l2_videodevice/v4l2_videodevice_test.cpp\n> index 1113cf5bf8cf..9fbd24cc91ea 100644\n> --- a/test/v4l2_videodevice/v4l2_videodevice_test.cpp\n> +++ b/test/v4l2_videodevice/v4l2_videodevice_test.cpp\n> @@ -64,8 +64,8 @@ int V4L2VideoDeviceTest::init()\n> format.size.height = 480;\n> \n> if (driver_ == \"vimc\") {\n> - sensor_ = new CameraSensor(media_->getEntityByName(\"Sensor A\"));\n> - if (sensor_->init())\n> + sensor_ = CameraSensorFactoryBase::create(media_->getEntityByName(\"Sensor A\"));\n> + if (!sensor_)\n> return TestFail;\n> \n> debayer_ = new V4L2Subdevice(media_->getEntityByName(\"Debayer A\"));\n> @@ -98,6 +98,5 @@ void V4L2VideoDeviceTest::cleanup()\n> capture_->close();\n> \n> delete debayer_;\n> - delete sensor_;\n> delete capture_;\n> }\n> diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.h b/test/v4l2_videodevice/v4l2_videodevice_test.h\n> index b5871ce69e18..7c9003ec4c4d 100644\n> --- a/test/v4l2_videodevice/v4l2_videodevice_test.h\n> +++ b/test/v4l2_videodevice/v4l2_videodevice_test.h\n> @@ -36,7 +36,7 @@ protected:\n> std::string entity_;\n> std::unique_ptr enumerator_;\n> std::shared_ptr media_;\n> - libcamera::CameraSensor *sensor_;\n> + std::unique_ptr sensor_;\n> libcamera::V4L2Subdevice *debayer_;\n> libcamera::V4L2VideoDevice *capture_;\n> std::vector> buffers_;\n> -- \n> 2.47.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D9C96BE173\n\tfor ;\n\tTue, 5 Nov 2024 09:58:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C7939653EA;\n\tTue, 5 Nov 2024 10:58:10 +0100 (CET)","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 0950A6035B\n\tfor ;\n\tTue, 5 Nov 2024 10:58:09 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8B0B7874;\n\tTue, 5 Nov 2024 10:58:00 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"nY4J2ogs\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1730800680;\n\tbh=umdWuTrIo71iUJFe3Nua1Dne726uEesAUwYf3aohzW4=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=nY4J2ogsxSmtw/V7uo9UpYPYOnFZd7/JbaYqsT0Ew5bKjye9yPNAqQLPZ8E5heLh/\n\tZ9H2kYG0Su88QmS82BiC/SUZg8dwGf6C/6geNw/0tqRKkZU6eDqzrjOeZ8A7AzXNNI\n\t1wyG20HrTj3x4mwqj9qt9kWUiSQgKCWhb3d2Cn90=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20241022145321.25923-2-jacopo.mondi@ideasonboard.com>","References":"<20241022145321.25923-1-jacopo.mondi@ideasonboard.com>\n\t<20241022145321.25923-2-jacopo.mondi@ideasonboard.com>","Subject":"Re: [PATCH 1/3] libcamera: camera_sensor: Introduce\n\tCameraSensorFactory","From":"Kieran Bingham ","Cc":"Jacopo Mondi ,\n\tJacopo Mondi ,\n\tStefan Klug ","To":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 05 Nov 2024 09:58:04 +0000","Message-ID":"<173080068478.3353069.6161875712529014753@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32032,"web_url":"https://patchwork.libcamera.org/comment/32032/","msgid":"","date":"2024-11-05T13:55:25","subject":"Re: [PATCH 1/3] libcamera: camera_sensor: Introduce\n\tCameraSensorFactory","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Kieran\n\nOn Tue, Nov 05, 2024 at 09:58:04AM +0000, Kieran Bingham wrote:\n> Quoting Jacopo Mondi (2024-10-22 15:53:13)\n> > From: Jacopo Mondi \n> >\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> > Signed-off-by: Jacopo Mondi \n> > Signed-off-by: Laurent Pinchart \n> > Reviewed-by: Stefan Klug \n> > ---\n> > include/libcamera/internal/camera_sensor.h | 48 +++++-\n> > src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 9 +-\n> > src/libcamera/pipeline/ipu3/cio2.cpp | 7 +-\n> > src/libcamera/pipeline/mali-c55/mali-c55.cpp | 5 +-\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 +-\n> > .../pipeline/rpi/common/pipeline_base.cpp | 5 +-\n> > src/libcamera/pipeline/simple/simple.cpp | 9 +-\n> > src/libcamera/pipeline/vimc/vimc.cpp | 7 +-\n> > src/libcamera/sensor/camera_sensor.cpp | 162 ++++++++++++++++++\n> > test/camera-sensor.cpp | 7 +-\n> > .../v4l2_videodevice_test.cpp | 5 +-\n> > test/v4l2_videodevice/v4l2_videodevice_test.h | 2 +-\n> > 12 files changed, 233 insertions(+), 40 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > index a42c15fa37ce..2d6cb697a870 100644\n> > --- a/include/libcamera/internal/camera_sensor.h\n> > +++ b/include/libcamera/internal/camera_sensor.h\n> > @@ -38,7 +38,6 @@ enum class Orientation;\n> > class CameraSensor : protected Loggable\n> > {\n> > public:\n> > - explicit CameraSensor(const MediaEntity *entity);\n> > ~CameraSensor();\n> >\n> > int init();\n> > @@ -81,6 +80,7 @@ public:\n> > int setTestPatternMode(controls::draft::TestPatternModeEnum mode);\n> >\n> > protected:\n> > + explicit CameraSensor(const MediaEntity *entity);\n> > std::string logPrefix() const override;\n> >\n> > private:\n> > @@ -122,4 +122,50 @@ private:\n> > std::unique_ptr focusLens_;\n> > };\n> >\n> > +class CameraSensorFactoryBase\n>\n> Seems I don't have much to comment on here so I might be down to\n> bikeshedding!\n>\n> Having read through the usages of the CameraSensorFactoryBase, I would\n> be tempted to rename this class to:\n>\n> \tCameraSensorFactory\n>\n>\n> > +{\n> > +public:\n> > + CameraSensorFactoryBase();\n> > + virtual ~CameraSensorFactoryBase() = default;\n> > +\n> > + static std::unique_ptr create(MediaEntity *entity);\n> > +\n> > +private:\n> > + LIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorFactoryBase)\n> > +\n> > + static std::vector &factories();\n> > +\n> > + static void registerFactory(CameraSensorFactoryBase *factory);\n> > +\n> > + virtual bool match(const MediaEntity *entity) const = 0;\n> > +\n> > + virtual std::unique_ptr\n> > + createInstance(MediaEntity *entity) const = 0;\n> > +};\n> > +\n> > +template\n> > +class CameraSensorFactory final : public CameraSensorFactoryBase\n>\n> And this one to CameraSensorFactoryType (or other name as appropriate)\n>\n> > +{\n> > +public:\n> > + CameraSensorFactory()\n> > + : CameraSensorFactoryBase()\n> > + {\n> > + }\n> > +\n> > +private:\n> > + bool match(const MediaEntity *entity) const override\n> > + {\n> > + return _CameraSensor::match(entity);\n> > + }\n> > +\n> > + std::unique_ptr\n> > + createInstance(MediaEntity *entity) const override\n> > + {\n> > + return _CameraSensor::create(entity);\n> > + }\n> > +};\n> > +\n> > +#define REGISTER_CAMERA_SENSOR(sensor) \\\n> > +static CameraSensorFactory global_##sensor##Factory{};\n>\n> This I think would be the only rename/usage of the simple name\n> CameraSensorFactory currently so the only place to be renamed to\n> CameraSensorFactoryType ?\n>\n> > +\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> > index 72aa6c75608a..4e66b3368d5a 100644\n> > --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> > +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> > @@ -157,11 +157,10 @@ PipelineHandlerISI *ISICameraData::pipe()\n> > /* Open and initialize pipe components. */\n> > int ISICameraData::init()\n> > {\n> > - int ret = sensor_->init();\n> > - if (ret)\n> > - return ret;\n> > + if (!sensor_)\n> > + return -ENODEV;\n> >\n> > - ret = csis_->open();\n> > + int ret = csis_->open();\n> > if (ret)\n> > return ret;\n> >\n> > @@ -1057,7 +1056,7 @@ bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)\n> > std::unique_ptr data =\n> > std::make_unique(this);\n> >\n> > - data->sensor_ = std::make_unique(sensor);\n> > + data->sensor_ = CameraSensorFactoryBase::create(sensor);\n>\n> So that the users of the Factory create read as :\n>\n> \tdata->sensor_ = CameraSensorFactory::create(sensor);\n>\n> \n>\n>\n\nAll other factories in the source base use this pattern\n\nIn example\nsrc/libcamera/pipeline/simple/simple.cpp: converter_ = ConverterFactoryBase::create(converter);\n\nI would rather keep this consistent\n\n> > data->csis_ = std::make_unique(csi);\n> > data->xbarSink_ = sink;\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > index 74a5d93f88ae..aa544d7b0303 100644\n> > --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > @@ -134,10 +134,9 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)\n> >\n> > MediaLink *link = links[0];\n> > MediaEntity *sensorEntity = link->source()->entity();\n> > - sensor_ = std::make_unique(sensorEntity);\n> > - ret = sensor_->init();\n> > - if (ret)\n> > - return ret;\n> > + sensor_ = CameraSensorFactoryBase::create(sensorEntity);\n> > + if (!sensor_)\n> > + return -ENODEV;\n>\n> I like that the construction is now simplified and there's no need to\n> create and then initialise the Sensor class in every usage...\n>\n> >\n> > ret = link->setEnabled(true);\n> > if (ret)\n> > diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n> > index 45c71c1dd619..4b71e0dad15e 100644\n> > --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n> > +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n> > @@ -141,9 +141,8 @@ int MaliC55CameraData::init()\n> > * Register a CameraSensor if we connect to a sensor and create\n> > * an entity for the connected CSI-2 receiver.\n> > */\n> > - sensor_ = std::make_unique(entity_);\n> > - ret = sensor_->init();\n> > - if (ret)\n> > + sensor_ = CameraSensorFactoryBase::create(entity_);\n> > + if (!sensor_)\n> > return ret;\n> >\n> > const MediaPad *sourcePad = entity_->getPadByIndex(0);\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index d8d1d65fbbf9..95c25e976015 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -1122,10 +1122,9 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > std::make_unique(this, &mainPath_,\n> > hasSelfPath_ ? &selfPath_ : nullptr);\n> >\n> > - data->sensor_ = std::make_unique(sensor);\n> > - ret = data->sensor_->init();\n> > - if (ret)\n> > - return ret;\n> > + data->sensor_ = CameraSensorFactoryBase::create(sensor);\n> > + if (!data->sensor_)\n> > + return -ENODEV;\n> >\n> > /* Initialize the camera properties. */\n> > data->properties_ = data->sensor_->properties();\n> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > index 3041fd1ed9fd..4f56bd33df05 100644\n> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > @@ -774,13 +774,10 @@ int PipelineHandlerBase::registerCamera(std::unique_ptr &camera\n> > CameraData *data = cameraData.get();\n> > int ret;\n> >\n> > - data->sensor_ = std::make_unique(sensorEntity);\n> > + data->sensor_ = CameraSensorFactoryBase::create(sensorEntity);\n> > if (!data->sensor_)\n> > return -EINVAL;\n> >\n> > - if (data->sensor_->init())\n> > - return -EINVAL;\n> > -\n> > /* Populate the map of sensor supported formats and sizes. */\n> > for (auto const mbusCode : data->sensor_->mbusCodes())\n> > data->sensorFormats_.emplace(mbusCode,\n> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > index 3ddce71d3757..67f583b8a22c 100644\n> > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > @@ -388,8 +388,6 @@ SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,\n> > MediaEntity *sensor)\n> > : Camera::Private(pipe), streams_(numStreams)\n> > {\n> > - int ret;\n> > -\n> > /*\n> > * Find the shortest path from the camera sensor to a video capture\n> > * device using the breadth-first search algorithm. This heuristic will\n> > @@ -480,12 +478,9 @@ SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,\n> > }\n> >\n> > /* Finally also remember the sensor. */\n> > - sensor_ = std::make_unique(sensor);\n> > - ret = sensor_->init();\n> > - if (ret) {\n> > - sensor_.reset();\n> > + sensor_ = CameraSensorFactoryBase::create(sensor);\n> > + if (!sensor_)\n> > return;\n> > - }\n> >\n> > LOG(SimplePipeline, Debug)\n> > << \"Found pipeline: \"\n> > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> > index 2165bae890cb..2e474133439c 100644\n> > --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> > @@ -532,10 +532,9 @@ int VimcCameraData::init()\n> > return ret;\n> >\n> > /* Create and open the camera sensor, debayer, scaler and video device. */\n> > - sensor_ = std::make_unique(media_->getEntityByName(\"Sensor B\"));\n> > - ret = sensor_->init();\n> > - if (ret)\n> > - return ret;\n> > + sensor_ = CameraSensorFactoryBase::create(media_->getEntityByName(\"Sensor B\"));\n> > + if (!sensor_)\n> > + return -ENODEV;\n> >\n> > debayer_ = V4L2Subdevice::fromEntityName(media_, \"Debayer B\");\n> > if (debayer_->open())\n> > diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp\n> > index 1b224f1989fe..025c9eefdd12 100644\n> > --- a/src/libcamera/sensor/camera_sensor.cpp\n> > +++ b/src/libcamera/sensor/camera_sensor.cpp\n> > @@ -11,6 +11,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> >\n> > #include \n> > @@ -1202,4 +1203,165 @@ std::string CameraSensor::logPrefix() const\n> > return \"'\" + entity_->name() + \"'\";\n> > }\n> >\n> > +namespace {\n> > +\n> > +/* Transitory default camera sensor implementation */\n> > +class CameraSensorDefault : public CameraSensor\n> > +{\n> > +public:\n> > + CameraSensorDefault(MediaEntity *entity)\n> > + : CameraSensor(entity)\n> > + {\n> > + }\n> > +\n> > + static bool match([[maybe_unused]] const MediaEntity *entity)\n> > + {\n>\n> Do we need any kind of debug/warning/log in here to say its' using a\n> temporary default wrapper? (Only thinking here in the event that we keep\n> this as a lowest priority factory ... see comment below in\n> CameraSensorFactoryBase::create())\n\nI suspect this \"Default\" implementation will still be used by most\nsensors and won't be temporary. There will indeed be specializations\nfor specific use cases, but most sensors will use this version.\n\n>\n> > + return true;\n> > + }\n> > +\n> > + static std::unique_ptr create(MediaEntity *entity)\n> > + {\n> > + std::unique_ptr sensor =\n> > + std::make_unique(entity);\n> > +\n> > + if (sensor->init())\n> > + return nullptr;\n> > +\n> > + return sensor;\n> > + }\n> > +};\n> > +\n> > +REGISTER_CAMERA_SENSOR(CameraSensorDefault)\n> > +\n> > +}; /* namespace */\n> > +\n> > +/**\n> > + * \\class CameraSensorFactoryBase\n> > + * \\brief Base class for camera sensor factories\n> > + *\n> > + * The CameraSensorFactoryBase class is the base of all specializations of\n> > + * the CameraSensorFactory class template. It implements the factory\n> > + * registration, maintains a registry of factories, and provides access to the\n> > + * registered factories.\n> > + */\n> > +\n> > +/**\n> > + * \\brief Construct a camera sensor factory base\n> > + *\n> > + * Creating an instance of the factory base registers it with the global list of\n> > + * factories, accessible through the factories() function.\n> > + */\n> > +CameraSensorFactoryBase::CameraSensorFactoryBase()\n> > +{\n> > + registerFactory(this);\n> > +}\n> > +\n> > +/**\n> > + * \\brief Create an instance of the CameraSensor corresponding to a media entity\n> > + * \\param[in] entity The media entity on the source end of the sensor\n> > + *\n> > + * \\return A unique pointer to a new instance of the CameraSensor subclass\n> > + * matching the entity, or a null pointer if no such factory exists\n> > + */\n> > +std::unique_ptr CameraSensorFactoryBase::create(MediaEntity *entity)\n> > +{\n> > + const std::vector &factories =\n> > + CameraSensorFactoryBase::factories();\n> > +\n> > + for (const CameraSensorFactoryBase *factory : factories) {\n>\n> Will the factories need to be sorted in any priority order ? Do we\n> anticipate multiple Factory classes could match ? (I.e. if the\n> transitory default were to be kept for instance?)\n\nYou should have a look at 3/3 :)\n[PATCH 3/3] libcamera: camera_sensor: Sort factories by priority\n\n>\n> Fine to handle that when required of course...\n>\n> Bike shedding aside, there's nothing in here that scares me so:\n>\n>\n> Reviewed-by: Kieran Bingham \n>\n\nThanks, I can go ahead and merge it\n\n>\n> > + if (!factory->match(entity))\n> > + continue;\n> > +\n> > + std::unique_ptr sensor = factory->createInstance(entity);\n> > + if (!sensor) {\n> > + LOG(CameraSensor, Error)\n> > + << \"Failed to create sensor for '\"\n> > + << entity->name();\n> > + return nullptr;\n> > + }\n> > +\n> > + return sensor;\n> > + }\n> > +\n> > + return nullptr;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Retrieve the list of all camera sensor factories\n> > + * \\return The list of camera sensor factories\n> > + */\n> > +std::vector &CameraSensorFactoryBase::factories()\n> > +{\n> > + /*\n> > + * The static factories map is defined inside the function to ensure\n> > + * it gets initialized on first use, without any dependency on link\n> > + * order.\n> > + */\n> > + static std::vector factories;\n> > + return factories;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Add a camera sensor class to the registry\n> > + * \\param[in] factory Factory to use to construct the camera sensor\n> > + */\n> > +void CameraSensorFactoryBase::registerFactory(CameraSensorFactoryBase *factory)\n> > +{\n> > + std::vector &factories =\n> > + CameraSensorFactoryBase::factories();\n> > +\n> > + factories.push_back(factory);\n> > +}\n> > +\n> > +/**\n> > + * \\class CameraSensorFactory\n> > + * \\brief Registration of CameraSensorFactory classes and creation of instances\n> > + * \\tparam _CameraSensor The camera sensor class type for this factory\n> > + *\n> > + * To facilitate discovery and instantiation of CameraSensor classes, the\n> > + * CameraSensorFactory class implements auto-registration of camera sensors.\n> > + * Each CameraSensor subclass shall register itself using the\n> > + * REGISTER_CAMERA_SENSOR() macro, which will create a corresponding instance\n> > + * of a CameraSensorFactory subclass and register it with the static list of\n> > + * factories.\n> > + */\n> > +\n> > +/**\n> > + * \\fn CameraSensorFactory::CameraSensorFactory()\n> > + * \\brief Construct a camera sensor factory\n> > + *\n> > + * Creating an instance of the factory registers it with the global list of\n> > + * factories, accessible through the CameraSensorFactoryBase::factories()\n> > + * function.\n> > + */\n> > +\n> > +/**\n> > + * \\fn CameraSensorFactory::createInstance() const\n> > + * \\brief Create an instance of the CameraSensor corresponding to the factory\n> > + *\n> > + * \\return A unique pointer to a newly constructed instance of the CameraSensor\n> > + * subclass corresponding to the factory\n> > + */\n> > +\n> > +/**\n> > + * \\def REGISTER_CAMERA_SENSOR(sensor)\n> > + * \\brief Register a camera sensor type to the sensor factory\n> > + * \\param[in] sensor Class name of the CameraSensor derived class to register\n> > + *\n> > + * Register a CameraSensor subclass with the factory and make it available to\n> > + * try and match sensors. The subclass needs to implement two static functions:\n> > + *\n> > + * \\code{.cpp}\n> > + * static bool match(const MediaEntity *entity);\n> > + * static std::unique_ptr create(MediaEntity *entity);\n> > + * \\endcode\n> > + *\n> > + * The match() function tests if the sensor class supports the camera sensor\n> > + * identified by a MediaEntity.\n> > + *\n> > + * The create() function creates a new instance of the sensor class. It may\n> > + * return a null pointer if initialization of the instance fails. It will only\n> > + * be called if the match() function has returned true for the given entity.\n> > + */\n> > +\n> > } /* namespace libcamera */\n> > diff --git a/test/camera-sensor.cpp b/test/camera-sensor.cpp\n> > index 1d402c43355b..869c788965da 100644\n> > --- a/test/camera-sensor.cpp\n> > +++ b/test/camera-sensor.cpp\n> > @@ -52,8 +52,8 @@ protected:\n> > return TestFail;\n> > }\n> >\n> > - sensor_ = new CameraSensor(entity);\n> > - if (sensor_->init() < 0) {\n> > + sensor_ = CameraSensorFactoryBase::create(entity);\n> > + if (!sensor_) {\n> > cerr << \"Unable to initialise camera sensor\" << endl;\n> > return TestFail;\n> > }\n> > @@ -118,13 +118,12 @@ protected:\n> >\n> > void cleanup()\n> > {\n> > - delete sensor_;\n> > }\n> >\n> > private:\n> > std::unique_ptr enumerator_;\n> > std::shared_ptr media_;\n> > - CameraSensor *sensor_;\n> > + std::unique_ptr sensor_;\n> > CameraLens *lens_;\n> > };\n> >\n> > diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.cpp b/test/v4l2_videodevice/v4l2_videodevice_test.cpp\n> > index 1113cf5bf8cf..9fbd24cc91ea 100644\n> > --- a/test/v4l2_videodevice/v4l2_videodevice_test.cpp\n> > +++ b/test/v4l2_videodevice/v4l2_videodevice_test.cpp\n> > @@ -64,8 +64,8 @@ int V4L2VideoDeviceTest::init()\n> > format.size.height = 480;\n> >\n> > if (driver_ == \"vimc\") {\n> > - sensor_ = new CameraSensor(media_->getEntityByName(\"Sensor A\"));\n> > - if (sensor_->init())\n> > + sensor_ = CameraSensorFactoryBase::create(media_->getEntityByName(\"Sensor A\"));\n> > + if (!sensor_)\n> > return TestFail;\n> >\n> > debayer_ = new V4L2Subdevice(media_->getEntityByName(\"Debayer A\"));\n> > @@ -98,6 +98,5 @@ void V4L2VideoDeviceTest::cleanup()\n> > capture_->close();\n> >\n> > delete debayer_;\n> > - delete sensor_;\n> > delete capture_;\n> > }\n> > diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.h b/test/v4l2_videodevice/v4l2_videodevice_test.h\n> > index b5871ce69e18..7c9003ec4c4d 100644\n> > --- a/test/v4l2_videodevice/v4l2_videodevice_test.h\n> > +++ b/test/v4l2_videodevice/v4l2_videodevice_test.h\n> > @@ -36,7 +36,7 @@ protected:\n> > std::string entity_;\n> > std::unique_ptr enumerator_;\n> > std::shared_ptr media_;\n> > - libcamera::CameraSensor *sensor_;\n> > + std::unique_ptr sensor_;\n> > libcamera::V4L2Subdevice *debayer_;\n> > libcamera::V4L2VideoDevice *capture_;\n> > std::vector> buffers_;\n> > --\n> > 2.47.0\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 39F1CBDB1C\n\tfor ;\n\tTue, 5 Nov 2024 13:55:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 346CD653F2;\n\tTue, 5 Nov 2024 14:55:31 +0100 (CET)","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 9AEF76035B\n\tfor ;\n\tTue, 5 Nov 2024 14:55:29 +0100 (CET)","from ideasonboard.com (mob-5-90-48-39.net.vodafone.it [5.90.48.39])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6A8EE874;\n\tTue, 5 Nov 2024 14:55:21 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"lhv7BtOF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1730814921;\n\tbh=it7D22Blm5R38EVuV0lgx5V3d13PNdzGupwdklM/CJ8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lhv7BtOFWvPlX3U+vXdThdugXkseeLST1XZ5euAHDMyx4eEE96Iuv2fXvzpD/K8i5\n\tKGHPbgM6eucu9vzxfDZyvALeo9aO2EtN9ZwLlOUBqIV2O3z9+JoS0O/C99gCTdhDzG\n\tTW9yRvQrRWgSPCQn6ssOofhcztNE+y0aKa7/CPIs=","Date":"Tue, 5 Nov 2024 14:55:25 +0100","From":"Jacopo Mondi ","To":"Kieran Bingham ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org, Jacopo Mondi ,\n\tStefan Klug ","Subject":"Re: [PATCH 1/3] libcamera: camera_sensor: Introduce\n\tCameraSensorFactory","Message-ID":"","References":"<20241022145321.25923-1-jacopo.mondi@ideasonboard.com>\n\t<20241022145321.25923-2-jacopo.mondi@ideasonboard.com>\n\t<173080068478.3353069.6161875712529014753@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<173080068478.3353069.6161875712529014753@ping.linuxembedded.co.uk>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]