[{"id":2714,"web_url":"https://patchwork.libcamera.org/comment/2714/","msgid":"<20190928172450.GB4865@pendragon.ideasonboard.com>","date":"2019-09-28T17:24:50","subject":"Re: [libcamera-devel] [PATCH v3 01/13] libcamera: pipeline: Move\n\tIPA from pipeline to camera data","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Fri, Sep 27, 2019 at 04:44:05AM +0200, Niklas Söderlund wrote:\n> The IPA acts on a camera and not on a pipeline which can expose more\n> then one camera. Move the IPA reference to the CameraData and move the\n> loading of an IPA from the specific pipeline handler implementation to\n> base PipelineHandler.\n> \n> It's still possible to expose a camera without an IPA but if an IPA is\n> request the camera is not valid and will not be registered in the system\n> if a suiting IPA module can't be found.\n\ns/suiting/suitable/\n\nPlease take into consideration the development use cases, where we often\nrun the cam and qcam utilities directly from the build directory,\nwithout installing IPAs first. With this patch the vimc camera isn't\ncreated anymore.\n\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/include/pipeline_handler.h | 4 +++\n> src/libcamera/pipeline/vimc.cpp | 17 ++++++------\n> src/libcamera/pipeline_handler.cpp | 33 ++++++++++++++++++++++++\n> 3 files changed, 46 insertions(+), 8 deletions(-)\n> \n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index 1fdef9cea40f1f0a..4400c7ed835f551d 100644\n> --- a/src/libcamera/include/pipeline_handler.h\n> +++ b/src/libcamera/include/pipeline_handler.h\n> @@ -14,6 +14,7 @@\n> #include \n> #include \n> \n> +#include \n> #include \n> #include \n> \n> @@ -39,10 +40,13 @@ public:\n> \t}\n> \tvirtual ~CameraData() {}\n> \n> +\tvirtual int loadIPA() { return 0; };\n> +\n> \tCamera *camera_;\n> \tPipelineHandler *pipe_;\n> \tstd::list queuedRequests_;\n> \tControlInfoMap controlInfo_;\n> +\tstd::unique_ptr ipa_;\n> \n> private:\n> \tCameraData(const CameraData &) = delete;\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index f26a91f86ec1794c..2fa48586fea0b80e 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -52,6 +52,15 @@ public:\n> \t\tdelete raw_;\n> \t}\n> \n> +\tint loadIPA() override\n> +\t{\n> +\t\tipa_ = IPAManager::instance()->createIPA(pipe_, 0, 0);\n> +\t\tif (!ipa_)\n> +\t\t\treturn -ENOENT;\n> +\n> +\t\treturn 0;\n> +\t}\n> +\n> \tint init(MediaDevice *media);\n> \tvoid bufferReady(Buffer *buffer);\n> \n> @@ -100,8 +109,6 @@ private:\n> \t\treturn static_cast(\n> \t\t\tPipelineHandler::cameraData(camera));\n> \t}\n> -\n> -\tstd::unique_ptr ipa_;\n> };\n> \n> VimcCameraConfiguration::VimcCameraConfiguration()\n> @@ -361,12 +368,6 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)\n> \tif (!media)\n> \t\treturn false;\n> \n> -\tipa_ = IPAManager::instance()->createIPA(this, 0, 0);\n> -\tif (ipa_ == nullptr)\n> -\t\tLOG(VIMC, Warning) << \"no matching IPA found\";\n> -\telse\n> -\t\tipa_->init();\n> -\n> \tstd::unique_ptr data = utils::make_unique(this);\n> \n> \t/* Locate and open the capture video node. */\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 3e54aa23d92b9a36..b8a3787e10a587b5 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -12,6 +12,7 @@\n> #include \n> \n> #include \"device_enumerator.h\"\n> +#include \"ipa_manager.h\"\n> #include \"log.h\"\n> #include \"media_device.h\"\n> #include \"utils.h\"\n> @@ -58,6 +59,20 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> * exists.\n> */\n> \n> +/**\n> + * \\fn CameraData::loadIPA()\n> + * \\brief Load an IPA for the camera\n> + *\n> + * This function shall be implemented by pipeline handlers that wish to have an\n> + * IPA. The function must locate and load an IPA, storing a pointer to it in\n> + * the \\a ipa_ or fail.\n> + *\n> + * If the pipeline handler do not wish to use an IPA this function shall not\n> + * be implemented.\n> + *\n> + * \\return True if a IPA could be loaded, false otherwise\n> + */\n> +\n> /**\n> * \\var CameraData::camera_\n> * \\brief The camera related to this CameraData instance\n> @@ -96,6 +111,14 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> * creating the camera, and shall not be modified afterwards.\n> */\n> \n> +/**\n> + * \\var CameraData::ipa_\n> + * \\brief The IPA module used by the camera\n> + *\n> + * Reference to the Image Processing Algorithms (IPA) operating on the camera's\n> + * stream(s). If no IPAs are in operation this should be set to nullptr.\n> + */\n> +\n> /**\n> * \\class PipelineHandler\n> * \\brief Create and manage cameras based on a set of media devices\n> @@ -425,6 +448,16 @@ void PipelineHandler::registerCamera(std::shared_ptr camera,\n> \t\t\t\t std::unique_ptr data)\n> {\n> \tdata->camera_ = camera.get();\n> +\n> +\tif (data->loadIPA()) {\n> +\t\tLOG(Pipeline, Warning) << \"Skipping \" << camera->name()\n> +\t\t\t\t << \" no IPA found\";\n> +\t\treturn;\n\nDo we need to propagate this error up ? With this patch the pipeline\nhandler will believe the camera is correctly registered, couldn't it\ncause issues ? What if the pipeline handler provides multiple cameras\nand IPA load fails for some of them only ? I think we need to take the\ndifferent scenarios into account and handle them properly.\n\n> +\t}\n\nI wonder if all this is really worth it, or if it's a bit\nover-engineered. I agree with the move the of ipa_ pointer to the\nCameraData class, but calling loadIPA() here with the method being\nimplemented by pipeline handlers seems a bit overkill.\n\nHow about splitting this in two, with one patch that moves the ipa\npointer to the CameraData class, and another patch that automates the\nIPA loading ? The former shouldn't be controversial and could be merged\nquickly, while the latter could then been discussed.\n\nI think that at the end of the day it really depends on how far you want\nto go with automated IPA loading.\n\n> +\n> +\tif (data->ipa_)\n> +\t\tdata->ipa_->init();\n> +\n> \tcameraData_[camera.get()] = std::move(data);\n> \tcameras_.push_back(camera);\n> \tmanager_->addCamera(std::move(camera));","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3225560BBA\n\tfor ;\n\tSat, 28 Sep 2019 19:25:02 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9E4B953B;\n\tSat, 28 Sep 2019 19:25:01 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1569691501;\n\tbh=ilHZnswlXK5XCz0jJgNfrnD5wa2raJ5JpRZ/bR4mCmw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Dtn2y0WUra2hRZWNi+OK37yKTNTyzOkeiMHsfdIvcywO0CzScxjO6bRVY6IsvZUah\n\tAeOaKpSvLW1i6pGs0BG4vHFuN91FwatNtTg4zv8DSDJDQmnTYZwwU2tOnUU+pfPAL5\n\tcvAAZ1Y2lGixuqzIwprHGcdIggChEIPz2Nt+47ew=","Date":"Sat, 28 Sep 2019 20:24:50 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190928172450.GB4865@pendragon.ideasonboard.com>","References":"<20190927024417.725906-1-niklas.soderlund@ragnatech.se>\n\t<20190927024417.725906-2-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190927024417.725906-2-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 01/13] libcamera: pipeline: Move\n\tIPA from pipeline to camera data","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":"Sat, 28 Sep 2019 17:25:02 -0000"}}]