[{"id":28887,"web_url":"https://patchwork.libcamera.org/comment/28887/","msgid":"<20240306163051.efbx2cy4rvzpfpg7@jasper>","date":"2024-03-06T16:30:51","subject":"Re: [PATCH/RFC 19/32] libcamera: camera_sensor: Introduce\n\tCameraSensorFactory","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Laurent,\n\nOn Fri, Mar 01, 2024 at 11:21:08PM +0200, Laurent Pinchart wrote:\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\nReviewed-by: Stefan Klug \n\nCheers,\nStefan\n\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/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> 11 files changed, 231 insertions(+), 37 deletions(-)\n> \n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index d05f48ebeebe..577af779cd6e 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -39,7 +39,6 @@ enum class Orientation;\n> class CameraSensor : protected Loggable\n> {\n> public:\n> -\texplicit CameraSensor(const MediaEntity *entity);\n> \t~CameraSensor();\n> \n> \tint init();\n> @@ -82,6 +81,7 @@ public:\n> \tint setTestPatternMode(controls::draft::TestPatternModeEnum mode);\n> \n> protected:\n> +\texplicit CameraSensor(const MediaEntity *entity);\n> \tstd::string logPrefix() const override;\n> \n> private:\n> @@ -123,4 +123,50 @@ private:\n> \tstd::unique_ptr focusLens_;\n> };\n> \n> +class CameraSensorFactoryBase\n> +{\n> +public:\n> +\tCameraSensorFactoryBase();\n> +\tvirtual ~CameraSensorFactoryBase() = default;\n> +\n> +\tstatic std::unique_ptr create(MediaEntity *entity);\n> +\n> +private:\n> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorFactoryBase)\n> +\n> +\tstatic std::vector &factories();\n> +\n> +\tstatic void registerFactory(CameraSensorFactoryBase *factory);\n> +\n> +\tvirtual bool match(const MediaEntity *entity) const = 0;\n> +\n> +\tvirtual std::unique_ptr\n> +\tcreateInstance(MediaEntity *entity) const = 0;\n> +};\n> +\n> +template\n> +class CameraSensorFactory final : public CameraSensorFactoryBase\n> +{\n> +public:\n> +\tCameraSensorFactory()\n> +\t\t: CameraSensorFactoryBase()\n> +\t{\n> +\t}\n> +\n> +private:\n> +\tbool match(const MediaEntity *entity) const override\n> +\t{\n> +\t\treturn _CameraSensor::match(entity);\n> +\t}\n> +\n> +\tstd::unique_ptr\n> +\tcreateInstance(MediaEntity *entity) const override\n> +\t{\n> +\t\treturn _CameraSensor::create(entity);\n> +\t}\n> +};\n> +\n> +#define REGISTER_CAMERA_SENSOR(sensor) \\\n> +\tstatic CameraSensorFactory global_##sensor##Factory{};\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 a3782aea2ba9..a00567c6873c 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> -\tint ret = sensor_->init();\n> -\tif (ret)\n> -\t\treturn ret;\n> +\tif (!sensor_)\n> +\t\treturn -ENODEV;\n> \n> -\tret = csis_->open();\n> +\tint ret = csis_->open();\n> \tif (ret)\n> \t\treturn ret;\n> \n> @@ -1063,7 +1062,7 @@ bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)\n> \t\tstd::unique_ptr data =\n> \t\t\tstd::make_unique(this);\n> \n> -\t\tdata->sensor_ = std::make_unique(sensor);\n> +\t\tdata->sensor_ = CameraSensorFactoryBase::create(sensor);\n> \t\tdata->csis_ = std::make_unique(csi);\n> \t\tdata->xbarSink_ = sink;\n> \n> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> index 43c816baf6ef..e09583ea418f 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> \tMediaLink *link = links[0];\n> \tMediaEntity *sensorEntity = link->source()->entity();\n> -\tsensor_ = std::make_unique(sensorEntity);\n> -\tret = sensor_->init();\n> -\tif (ret)\n> -\t\treturn ret;\n> +\tsensor_ = CameraSensorFactoryBase::create(sensorEntity);\n> +\tif (!sensor_)\n> +\t\treturn -ENODEV;\n> \n> \tret = link->setEnabled(true);\n> \tif (ret)\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index abb21968413a..1a3e7938fa91 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -1109,10 +1109,9 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> \t\tstd::make_unique(this, &mainPath_,\n> \t\t\t\t\t\t hasSelfPath_ ? &selfPath_ : nullptr);\n> \n> -\tdata->sensor_ = std::make_unique(sensor);\n> -\tret = data->sensor_->init();\n> -\tif (ret)\n> -\t\treturn ret;\n> +\tdata->sensor_ = CameraSensorFactoryBase::create(sensor);\n> +\tif (!data->sensor_)\n> +\t\treturn -ENODEV;\n> \n> \t/* Initialize the camera properties. */\n> \tdata->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 7e420b3f90a4..d662c8f12145 100644\n> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> @@ -772,13 +772,10 @@ int PipelineHandlerBase::registerCamera(std::unique_ptr &camera\n> \tCameraData *data = cameraData.get();\n> \tint ret;\n> \n> -\tdata->sensor_ = std::make_unique(sensorEntity);\n> +\tdata->sensor_ = CameraSensorFactoryBase::create(sensorEntity);\n> \tif (!data->sensor_)\n> \t\treturn -EINVAL;\n> \n> -\tif (data->sensor_->init())\n> -\t\treturn -EINVAL;\n> -\n> \t/* Populate the map of sensor supported formats and sizes. */\n> \tfor (auto const mbusCode : data->sensor_->mbusCodes())\n> \t\tdata->sensorFormats_.emplace(mbusCode,\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 01f2a97798ba..0bd021388c8f 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -371,8 +371,6 @@ SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,\n> \t\t\t\t MediaEntity *sensor)\n> \t: Camera::Private(pipe), streams_(numStreams)\n> {\n> -\tint ret;\n> -\n> \t/*\n> \t * Find the shortest path from the camera sensor to a video capture\n> \t * device using the breadth-first search algorithm. This heuristic will\n> @@ -463,12 +461,9 @@ SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,\n> \t}\n> \n> \t/* Finally also remember the sensor. */\n> -\tsensor_ = std::make_unique(sensor);\n> -\tret = sensor_->init();\n> -\tif (ret) {\n> -\t\tsensor_.reset();\n> +\tsensor_ = CameraSensorFactoryBase::create(sensor);\n> +\tif (!sensor_)\n> \t\treturn;\n> -\t}\n> \n> \tLOG(SimplePipeline, Debug)\n> \t\t<< \"Found pipeline: \"\n> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> index 5e66ee1d26c1..ae0ca21907ec 100644\n> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> @@ -510,10 +510,9 @@ int VimcCameraData::init()\n> \t\treturn ret;\n> \n> \t/* Create and open the camera sensor, debayer, scaler and video device. */\n> -\tsensor_ = std::make_unique(media_->getEntityByName(\"Sensor B\"));\n> -\tret = sensor_->init();\n> -\tif (ret)\n> -\t\treturn ret;\n> +\tsensor_ = CameraSensorFactoryBase::create(media_->getEntityByName(\"Sensor B\"));\n> +\tif (!sensor_)\n> +\t\treturn -ENODEV;\n> \n> \tdebayer_ = V4L2Subdevice::fromEntityName(media_, \"Debayer B\");\n> \tif (debayer_->open())\n> diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp\n> index 5c4f35324055..a35683eb4b58 100644\n> --- a/src/libcamera/sensor/camera_sensor.cpp\n> +++ b/src/libcamera/sensor/camera_sensor.cpp\n> @@ -12,6 +12,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> \n> @@ -1204,4 +1205,165 @@ std::string CameraSensor::logPrefix() const\n> \treturn \"'\" + entity_->name() + \"'\";\n> }\n> \n> +namespace {\n> +\n> +/* Transitory default camera sensor implementation */\n> +class CameraSensorDefault : public CameraSensor\n> +{\n> +public:\n> +\tCameraSensorDefault(MediaEntity *entity)\n> +\t\t: CameraSensor(entity)\n> +\t{\n> +\t}\n> +\n> +\tstatic bool match([[maybe_unused]] const MediaEntity *entity)\n> +\t{\n> +\t\treturn true;\n> +\t}\n> +\n> +\tstatic std::unique_ptr create(MediaEntity *entity)\n> +\t{\n> +\t\tstd::unique_ptr sensor =\n> +\t\t\tstd::make_unique(entity);\n> +\n> +\t\tif (sensor->init())\n> +\t\t\treturn nullptr;\n> +\n> +\t\treturn sensor;\n> +\t}\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> +\tregisterFactory(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> +\tconst std::vector &factories =\n> +\t\tCameraSensorFactoryBase::factories();\n> +\n> +\tfor (const CameraSensorFactoryBase *factory : factories) {\n> +\t\tif (!factory->match(entity))\n> +\t\t\tcontinue;\n> +\n> +\t\tstd::unique_ptr sensor = factory->createInstance(entity);\n> +\t\tif (!sensor) {\n> +\t\t\tLOG(CameraSensor, Error)\n> +\t\t\t\t<< \"Failed to create sensor for '\"\n> +\t\t\t\t<< entity->name();\n> +\t\t\treturn nullptr;\n> +\t\t}\n> +\n> +\t\treturn sensor;\n> +\t}\n> +\n> +\treturn 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> +\t/*\n> +\t * The static factories map is defined inside the function to ensure\n> +\t * it gets initialized on first use, without any dependency on link\n> +\t * order.\n> +\t */\n> +\tstatic std::vector factories;\n> +\treturn 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> +\tstd::vector &factories =\n> +\t\tCameraSensorFactoryBase::factories();\n> +\n> +\tfactories.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 9503d7753fb9..acd3a1241ae1 100644\n> --- a/test/camera-sensor.cpp\n> +++ b/test/camera-sensor.cpp\n> @@ -52,8 +52,8 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> -\t\tsensor_ = new CameraSensor(entity);\n> -\t\tif (sensor_->init() < 0) {\n> +\t\tsensor_ = CameraSensorFactoryBase::create(entity);\n> +\t\tif (!sensor_) {\n> \t\t\tcerr << \"Unable to initialise camera sensor\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> @@ -118,13 +118,12 @@ protected:\n> \n> \tvoid cleanup()\n> \t{\n> -\t\tdelete sensor_;\n> \t}\n> \n> private:\n> \tstd::unique_ptr enumerator_;\n> \tstd::shared_ptr media_;\n> -\tCameraSensor *sensor_;\n> +\tstd::unique_ptr sensor_;\n> \tCameraLens *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> \tformat.size.height = 480;\n> \n> \tif (driver_ == \"vimc\") {\n> -\t\tsensor_ = new CameraSensor(media_->getEntityByName(\"Sensor A\"));\n> -\t\tif (sensor_->init())\n> +\t\tsensor_ = CameraSensorFactoryBase::create(media_->getEntityByName(\"Sensor A\"));\n> +\t\tif (!sensor_)\n> \t\t\treturn TestFail;\n> \n> \t\tdebayer_ = new V4L2Subdevice(media_->getEntityByName(\"Debayer A\"));\n> @@ -98,6 +98,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 d2de1a6de29f..f06db72a9df5 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> \tstd::string entity_;\n> \tstd::unique_ptr enumerator_;\n> \tstd::shared_ptr media_;\n> -\tlibcamera::CameraSensor *sensor_;\n> +\tstd::unique_ptr sensor_;\n> \tlibcamera::V4L2Subdevice *debayer_;\n> \tlibcamera::V4L2VideoDevice *capture_;\n> \tstd::vector> buffers_;\n> -- \n> Regards,\n> \n> Laurent Pinchart\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 129C1C0F2A\n\tfor ;\n\tWed, 6 Mar 2024 16:30:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7003961C8F;\n\tWed, 6 Mar 2024 17:30:55 +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 B6DC361C8F\n\tfor ;\n\tWed, 6 Mar 2024 17:30:54 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:dd65:a3ea:5f4d:989a])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D21F1BD1;\n\tWed, 6 Mar 2024 17:30:36 +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=\"ZioLHmNL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1709742636;\n\tbh=CdZTjRPZZHDb5O4mtsA16aHFIExGCpdWzp5nijrp8jQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZioLHmNLdM8vOUT5IXyilAG+8Q2mcBQNaa2sqUOgXRJBhQUPDCdTOJd2Hsbtwkd/Z\n\tZcDABYjgby8fRBQ7fVLazhD/47HqTrtJi1W8RFdMcQER/iybWHdLdgUV5xA+13fXju\n\tTmbcEButYc/j6Q71AAPG8ut5BIbzJf2+dejaZc3s=","Date":"Wed, 6 Mar 2024 17:30:51 +0100","From":"Stefan Klug ","To":"Laurent Pinchart ","Subject":"Re: [PATCH/RFC 19/32] libcamera: camera_sensor: Introduce\n\tCameraSensorFactory","Message-ID":"<20240306163051.efbx2cy4rvzpfpg7@jasper>","References":"<20240301212121.9072-1-laurent.pinchart@ideasonboard.com>\n\t<20240301212121.9072-20-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240301212121.9072-20-laurent.pinchart@ideasonboard.com>","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","Cc":"libcamera-devel@lists.libcamera.org, Sakari Ailus ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]