[{"id":3454,"web_url":"https://patchwork.libcamera.org/comment/3454/","msgid":"<20200115195316.GC977577@oden.dyn.berto.se>","date":"2020-01-15T19:53:16","subject":"Re: [libcamera-devel] [RFC 1/7] libcamera: camera_sensor: Introduce\n\tCameraSensorFactory","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2019-12-18 15:49:55 +0100, Jacopo Mondi wrote:\n> Introduce a factory to create CameraSensor derived classes instances by\n> inspecting the sensor media entity name.\n> \n> For now, unconditionally create and return an instance of the only\n> existing CameraSensor class in CameraSensorFactory::create()\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/camera_sensor.cpp | 22 +++++++++++++++++++\n> src/libcamera/include/camera_sensor.h | 10 ++++++++-\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-\n> src/libcamera/pipeline/vimc.cpp | 2 +-\n> test/camera-sensor.cpp | 2 +-\n> .../v4l2_videodevice_test.cpp | 2 +-\n> 7 files changed, 36 insertions(+), 6 deletions(-)\n> \n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index bc8b9e78bc42..ac8878fe336e 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -28,6 +28,28 @@ namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(CameraSensor);\n> \n> +/**\n> + * \\class CameraSensorFactory\n> + * \\brief Factory of camera sensor\n> + *\n> + * The CameraSensorFactory instantiate and return the opportune CameraSensor\n> + * sub-class by inspecting the name of the sensor entity to handle.\n> + */\n> +\n> +/**\n> + * \\brief Create and return a CameraSensor\n> + * \\param[in] entity The media entity that represents the sensor to handle\n> + *\n> + * Instantiate and return the opportune CameraSensor subclass inspecting the\n> + * \\a entity name.\n> + *\n> + * \\return A pointer to a CameraSensor derived class instance\n> + */\n> +CameraSensor *CameraSensorFactory::create(const MediaEntity *entity)\n> +{\n> +\treturn new CameraSensor(entity);\n> +}\n> +\n> /**\n> * \\class CameraSensor\n> * \\brief A camera sensor based on V4L2 subdevices\n> diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h\n> index 99cff98128dc..2c09b1bdb61b 100644\n> --- a/src/libcamera/include/camera_sensor.h\n> +++ b/src/libcamera/include/camera_sensor.h\n> @@ -22,10 +22,16 @@ class V4L2Subdevice;\n> \n> struct V4L2SubdeviceFormat;\n> \n> +class CameraSensor;\n> +class CameraSensorFactory final\n> +{\n> +public:\n> +\tstatic CameraSensor *create(const MediaEntity *entity);\n> +};\n\nI Wonder if it would not make more sens to move this this to \nCameraSensor::create(). Looking at our code this seems to be the pattern \nused in most places.\n\n> +\n> class CameraSensor : protected Loggable\n> {\n> public:\n> -\texplicit CameraSensor(const MediaEntity *entity);\n> \t~CameraSensor();\n> \n> \tCameraSensor(const CameraSensor &) = delete;\n> @@ -49,6 +55,8 @@ public:\n> \tconst ControlList &properties() const { return properties_; }\n> \n> protected:\n> +\tfriend class CameraSensorFactory;\n> +\texplicit CameraSensor(const MediaEntity *entity);\n\nDo we need to keep explicit here?\n\n> \tstd::string logPrefix() const;\n> \n> private:\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 536a63a30018..6de2f548fe1c 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -1362,7 +1362,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::create(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 e9a70755f4c5..276a12755f59 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -883,7 +883,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> \n> \tdata->controlInfo_ = std::move(ctrls);\n> \n> -\tdata->sensor_ = new CameraSensor(sensor);\n> +\tdata->sensor_ = CameraSensorFactory::create(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 2bfab35314c4..5d6baa56ba8b 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -403,7 +403,7 @@ int VimcCameraData::init(MediaDevice *media)\n> \t\treturn ret;\n> \n> \t/* Create and open the camera sensor, debayer, scaler and video device. */\n> -\tsensor_ = new CameraSensor(media->getEntityByName(\"Sensor B\"));\n> +\tsensor_ = CameraSensorFactory::create(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..0e7e3f5585d2 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::create(entity);\n> \t\tif (sensor_->init() < 0) {\n> \t\t\tcerr << \"Unable to initialise camera sensor\" << endl;\n> \t\t\treturn TestFail;\n> diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.cpp b/test/v4l2_videodevice/v4l2_videodevice_test.cpp\n> index 577da4cb601c..3c4ca60b9076 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::create(media_->getEntityByName(\"Sensor A\"));\n> \t\tif (sensor_->init())\n> \t\t\treturn TestFail;\n> \n> -- \n> 2.24.0\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lf1-x133.google.com (mail-lf1-x133.google.com\n\t[IPv6:2a00:1450:4864:20::133])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 426666045C\n\tfor ;\n\tWed, 15 Jan 2020 20:53:18 +0100 (CET)","by mail-lf1-x133.google.com with SMTP id 15so13702429lfr.2\n\tfor ;\n\tWed, 15 Jan 2020 11:53:18 -0800 (PST)","from localhost (h-93-159.A463.priv.bahnhof.se. [46.59.93.159])\n\tby smtp.gmail.com with ESMTPSA id\n\to19sm11610654lji.54.2020.01.15.11.53.16\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 15 Jan 2020 11:53:17 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=rFmj7D0b4Tb0c+DtKxmvs/ifDtHfzNK89gooOPxL3ig=;\n\tb=LCdwm/BfkwSt7U04dyapIficH6y/IxT5iX2dEZaEkdoaJ1fKJcAsJcaQqWU92kTgAq\n\tdFxGh5QxVVTuHfh+strIkO5ejU6C1la4QkKttuE+xMzm1WFWhXGNTLjRNygdSXmG9dsU\n\tMWw4V+q8SeI+4VVfCt70NlykPFOmZXOE+mImpVil+v5nr6gwzfMAS+M+gwNsWFobX/1L\n\tCdzH1H1WZbSE+4aNiz+7xtZr2AHaZ/rvjagQJ8eMFeZo3cA9PiCeGYmXJ+QGrBVLyYM4\n\tbT6U3Cr9xbx5ytzk/NxOrJObdnMNaEMoHTp/Fb9DDbHeArCBFAS4luCBDRlAJoq9EII+\n\tOtEw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=rFmj7D0b4Tb0c+DtKxmvs/ifDtHfzNK89gooOPxL3ig=;\n\tb=LtPuWlVX8d5jnaxDUsjTroxUov55Kk//jYy1NePyxJ0TL7a0Vayc+oyqlcaJQJFjMb\n\tWVNNFjDCloNFTkcbzmXHUdhsHIEZmZNR0eWS9bnMw159ixnFvNmbHo54tJmF/YlglTdz\n\tnXp/jYevMekxw7im44g8xYvh0gaCqFm/ib0gqTcY054fn+q6oU/Z/tYG1phnWYwuNfTp\n\tJtTxTiw3y8MpT3cCSM+8Gi9l/4vRxdF7FXdYxQAwnuJtABadPupgu8iyUF3KOOwS0LtU\n\tPJEGHDpkjx+50IHjcnt7Y4bo9rfUFqEM8bnx1ZCW6Z1oISeqK5rp3bi/FFJxtBG4ibTb\n\tvM8g==","X-Gm-Message-State":"APjAAAUPr2Mb74aZ3MVPyJPz/5yZj7nBDPx7g2LOYUwd8RdJzSt+RHwz\n\tN7EdOtxLm3JIIkStf7mmtiBO0NcmUPo=","X-Google-Smtp-Source":"APXvYqwUEUY+nXb1WDWE3IDjPvZ+FvY5nqAdf83Z/ehknsS4JCGbRtYvpr61AxqRS/k2PRhgJ8LT/A==","X-Received":"by 2002:ac2:5088:: with SMTP id f8mr335474lfm.163.1579117997607; \n\tWed, 15 Jan 2020 11:53:17 -0800 (PST)","Date":"Wed, 15 Jan 2020 20:53:16 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200115195316.GC977577@oden.dyn.berto.se>","References":"<20191218145001.22283-1-jacopo@jmondi.org>\n\t<20191218145001.22283-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191218145001.22283-2-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [RFC 1/7] libcamera: camera_sensor: Introduce\n\tCameraSensorFactory","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 15 Jan 2020 19:53:18 -0000"}}]