[{"id":2543,"web_url":"https://patchwork.libcamera.org/comment/2543/","msgid":"<64ade229-0de8-3c4e-f982-0643d1de33b4@ideasonboard.com>","date":"2019-08-29T13:52:28","subject":"Re: [libcamera-devel] [PATCH 02/13] libcamera: pipeline: Move IPA\n\tfrom pipeline to camera data","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Niklas,\n\nOn 28/08/2019 02:16, Niklas Söderlund wrote:\n> The IPA acts on a camera and not on a pipeline which can expose more\n> then once camera. Move the IPA reference to the CameraData and move the\n\ns/once/one/\n\n\n> loading of an IPA from the specific pipeline handler implementation to\n> base PipelineHandler.\n\nThis sounds like a good thing, and removes the code duplication for\nloading IPAs.\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> \n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/include/pipeline_handler.h | 12 ++++++--\n> src/libcamera/pipeline/vimc.cpp | 10 +-----\n> src/libcamera/pipeline_handler.cpp | 39 +++++++++++++++++++++++-\n> 3 files changed, 49 insertions(+), 12 deletions(-)\n> \n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index 1fdef9cea40f1f0a..ffc7adb802215313 100644\n> --- a/src/libcamera/include/pipeline_handler.h\n> +++ b/src/libcamera/include/pipeline_handler.h\n> @@ -15,6 +15,7 @@\n> #include \n> \n> #include \n> +#include \n> #include \n> \n> namespace libcamera {\n> @@ -33,8 +34,11 @@ class Request;\n> class CameraData\n> {\n> public:\n> -\texplicit CameraData(PipelineHandler *pipe)\n> -\t\t: pipe_(pipe)\n> +\texplicit CameraData(PipelineHandler *pipe,\n> +\t\t\t uint32_t minIPAVersion = 0,\n> +\t\t\t uint32_t maxIPAVersion = 0)\n> +\t\t: pipe_(pipe), ipa_(nullptr), minIPAVersion_(minIPAVersion),\n> +\t\t maxIPAVersion_(maxIPAVersion)\n> \t{\n> \t}\n> \tvirtual ~CameraData() {}\n> @@ -44,6 +48,10 @@ public:\n> \tstd::list queuedRequests_;\n> \tControlInfoMap controlInfo_;\n> \n> +\tstd::unique_ptr ipa_;\n\nIs this ok to be public? Or should it be more restricted?\n\n> +\tconst uint32_t minIPAVersion_;\n> +\tconst uint32_t maxIPAVersion_;\n\nAnd these?\n\nHrm ... looking at CameraData - we treat it as an internal 'public'\nobject anyway, so I guess it's matching the existing precedence on that\nclass.\n\n\n> +\n> private:\n> \tCameraData(const CameraData &) = delete;\n> \tCameraData &operator=(const CameraData &) = delete;\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index e5c4890501db71c8..be6507cd4bc0d1b9 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -38,7 +38,7 @@ class VimcCameraData : public CameraData\n> {\n> public:\n> \tVimcCameraData(PipelineHandler *pipe)\n> -\t\t: CameraData(pipe), sensor_(nullptr), debayer_(nullptr),\n> +\t\t: CameraData(pipe, 1, 1), sensor_(nullptr), debayer_(nullptr),\n> \t\t scaler_(nullptr), video_(nullptr), raw_(nullptr)\n> \t{\n> \t}\n> @@ -100,8 +100,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 +359,6 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)\n> \tif (!media)\n> \t\treturn false;\n> \n> -\tipa_ = IPAManager::instance()->createIPA(this, 1, 1);\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..89b67806597728f9 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> @@ -49,13 +50,20 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> */\n> \n> /**\n> - * \\fn CameraData::CameraData(PipelineHandler *pipe)\n> + * \\fn CameraData::CameraData(PipelineHandler *pipe, uint32_t minIPAVersion,\n> + * uint32_t maxIPAVersion)\n> * \\brief Construct a CameraData instance for the given pipeline handler\n> * \\param[in] pipe The pipeline handler\n> + * \\param[in] minIPAVersion Minimum acceptable version of IPA module\n> + * \\param[in] maxIPAVersion Maximum acceptable version of IPA module\n> *\n> * The reference to the pipeline handler is stored internally, the caller shall\n> * guarantee that the pointer remains valid as long as the CameraData instance\n> * exists.\n> + *\n> + * The IPA maximum and minimum version numbers are used to match with an IPA\n> + * interface that would be compatible with the Camera. If no IPA interface\n> + * is needed for the camera both parameters should be set to 0.\n> */\n> \n> /**\n> @@ -96,6 +104,24 @@ 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 IPA are in operation this should be set to nullptr.\n\nI'm not sure how to handle the pluralism there. I interpret \"IPA\" as\nsingular, which would mean this should read \"If no IPA is in operation...\"\n\nBut you do define IPA as \"...Algorithms\" ... but I'm not sure the\nacronym can convey the pluralism, so it would have to be \"If no IPAs are\nin operation...\"\n\nA bit of googling makes me believe we should pluralise the acronym,\nmaking it the \"If no IPAs are in operation...\" option.\n\n> + */\n> +\n> +/**\n> + * \\var CameraData::minIPAVersion_\n> + * \\brief Minimum acceptable version of IPA module\n> + */\n> +\n> +/**\n> + * \\var CameraData::maxIPAVersion_\n> + * \\brief Maximum acceptable version of IPA module\n> + */\n> +\n> /**\n> * \\class PipelineHandler\n> * \\brief Create and manage cameras based on a set of media devices\n> @@ -424,6 +450,17 @@ void PipelineHandler::completeRequest(Camera *camera, Request *request)\n> void PipelineHandler::registerCamera(std::shared_ptr camera,\n> \t\t\t\t std::unique_ptr data)\n> {\n> +\tif (data->minIPAVersion_ || data->maxIPAVersion_) {\n> +\t\tdata->ipa_ = IPAManager::instance()->createIPA(this,\n> +\t\t\t\t\t\t\t data->minIPAVersion_,\n> +\t\t\t\t\t\t\t data->maxIPAVersion_);\n> +\t\tif (!data->ipa_) {\n> +\t\t\tLOG(Pipeline, Warning) << \"Skipping \" << camera->name()\n> +\t\t\t\t\t << \" no IPA found\";\n> +\t\t\treturn;\n> +\t\t}\n> +\t}\n> +\n\nThe code removed from VIMC calls:\n\n> -\t\tipa_->init();\n\nShouldn't that be added here somewhere?\nOr is that going to happen somewhere else?\n\n\nDo we need to manually init() an ipa? Can't that happen as part of\ncreateIPA() ?\n\n\nWith the correct resolution for the missing data->ipa_->init() call,\nwhich perhaps is handled elsewhere, or can be added if required:\n\nReviewed-by: Kieran Bingham \n\n\n> \tdata->camera_ = camera.get();\n> \tcameraData_[camera.get()] = std::move(data);\n> \tcameras_.push_back(camera);\n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AD09860BB0\n\tfor ;\n\tThu, 29 Aug 2019 15:52:31 +0200 (CEST)","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 2DCD52E5;\n\tThu, 29 Aug 2019 15:52:31 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1567086751;\n\tbh=BZGXcYFQ+VHfzv1gqm3zDQp71cineEqTx3i+TmDBzT8=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=sSFdDucCrUIQpcd/UTbaynynDUcLrqwt6Jeubj59uob66W0e6dTnpZQWwp8B0yG3Q\n\tN/L2aniq/BS3O13yDbgkAfJYQ3vlSh69Warm+GRrl4TfI6/lYnZQ6nSZSjBXdFzKhc\n\t6dreT8u30Kg5fyTZDoRAg8XGc6wSZTN5hxqDnp5Y=","Reply-To":"kieran.bingham@ideasonboard.com","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20190828011710.32128-1-niklas.soderlund@ragnatech.se>\n\t<20190828011710.32128-3-niklas.soderlund@ragnatech.se>","From":"Kieran Bingham ","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+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCJQQYAQoADwIbDAUCWcOUawUJ\n\tB4D+AgAKCRChHkZyEKRh/XJhEACr5iidt/0MZ0rWRMCbZFMWD7D2g6nZeOp+F2zY8CEUW+sd\n\tCDVd9BH9QX9KN5SZo6YtJzMzSzpcx45VwTvtQW0n/6Eujg9EUqblfU9xqvqDmbjEapr5d/OL\n\t21GTALb0owKhA5qDUGEcKGCphpQffKhTNo/BP99jvmJUj7IPSKH97qPypi8/ym8bAxB+uY31\n\tgHTMHf1jMJJ1pRo2tYYPeIIHGDqXBI4sp5GHHF+JcIhgR/e/A6w/dgzHYmQPl2ix5eZYEZbV\n\tTRP+gkX4NV8oHqa/lR+xPOlWElGB57viOSOoWriqxQbFy8XbG1GR8cWlkNtGBGVWaJaSoORP\n\tiowD7irXL91bCyFIqL+7BVk3Jy4uzP744PzE80KwxOp5SQAp9sPzFbgsJrLev90PZySjFHG0\n\twP144DK7nBjOj/J0g9OHVASP1JjK+nw7SDoKnETDIdRC0XmiHXk7TXzPdkvO0UkpHdEPjZUp\n\tWyuc0MqehjR/hTTPt4m/Y14XzEcy6JREIjOrFfUZVho2QpOdv9CNryGdieRTNjUtz463CIaZ\n\tdPBiw9mOMBoNffkn9FIoCjLnAaj9gUAnEHWBZOEviQ5NuyqpeP0YtzI4iaRbSUkYZHej99X3\n\tVmHrdLlMqd/ZgYYbPGSL4AN3FVACb5CxuxEHwo029VcE5U3CSjzqtCoX12tm7A==","Organization":"Ideas on Board","Message-ID":"<64ade229-0de8-3c4e-f982-0643d1de33b4@ideasonboard.com>","Date":"Thu, 29 Aug 2019 14:52:28 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.8.0","MIME-Version":"1.0","In-Reply-To":"<20190828011710.32128-3-niklas.soderlund@ragnatech.se>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 02/13] libcamera: pipeline: Move IPA\n\tfrom pipeline to camera data","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 29 Aug 2019 13:52:31 -0000"}},{"id":2553,"web_url":"https://patchwork.libcamera.org/comment/2553/","msgid":"<20190829174953.GA8479@bigcity.dyn.berto.se>","date":"2019-08-29T17:49:53","subject":"Re: [libcamera-devel] [PATCH 02/13] libcamera: pipeline: Move IPA\n\tfrom pipeline to camera data","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Kieran,\n\nThanks for your feedback.\n\nOn 2019-08-29 14:52:28 +0100, Kieran Bingham wrote:\n> Hi Niklas,\n> \n> On 28/08/2019 02:16, Niklas Söderlund wrote:\n> > The IPA acts on a camera and not on a pipeline which can expose more\n> > then once camera. Move the IPA reference to the CameraData and move the\n> \n> s/once/one/\n> \n> \n> > loading of an IPA from the specific pipeline handler implementation to\n> > base PipelineHandler.\n> \n> This sounds like a good thing, and removes the code duplication for\n> loading IPAs.\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> > \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/libcamera/include/pipeline_handler.h | 12 ++++++--\n> > src/libcamera/pipeline/vimc.cpp | 10 +-----\n> > src/libcamera/pipeline_handler.cpp | 39 +++++++++++++++++++++++-\n> > 3 files changed, 49 insertions(+), 12 deletions(-)\n> > \n> > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> > index 1fdef9cea40f1f0a..ffc7adb802215313 100644\n> > --- a/src/libcamera/include/pipeline_handler.h\n> > +++ b/src/libcamera/include/pipeline_handler.h\n> > @@ -15,6 +15,7 @@\n> > #include \n> > \n> > #include \n> > +#include \n> > #include \n> > \n> > namespace libcamera {\n> > @@ -33,8 +34,11 @@ class Request;\n> > class CameraData\n> > {\n> > public:\n> > -\texplicit CameraData(PipelineHandler *pipe)\n> > -\t\t: pipe_(pipe)\n> > +\texplicit CameraData(PipelineHandler *pipe,\n> > +\t\t\t uint32_t minIPAVersion = 0,\n> > +\t\t\t uint32_t maxIPAVersion = 0)\n> > +\t\t: pipe_(pipe), ipa_(nullptr), minIPAVersion_(minIPAVersion),\n> > +\t\t maxIPAVersion_(maxIPAVersion)\n> > \t{\n> > \t}\n> > \tvirtual ~CameraData() {}\n> > @@ -44,6 +48,10 @@ public:\n> > \tstd::list queuedRequests_;\n> > \tControlInfoMap controlInfo_;\n> > \n> > +\tstd::unique_ptr ipa_;\n> \n> Is this ok to be public? Or should it be more restricted?\n> \n> > +\tconst uint32_t minIPAVersion_;\n> > +\tconst uint32_t maxIPAVersion_;\n> \n> And these?\n> \n> Hrm ... looking at CameraData - we treat it as an internal 'public'\n> object anyway, so I guess it's matching the existing precedence on that\n> class.\n\nYes I think in the future we might strict up the access around \nCameraData, but for now I think this is sane. It follows what we already \ndo and it's not visible to the application.\n\n> \n> \n> > +\n> > private:\n> > \tCameraData(const CameraData &) = delete;\n> > \tCameraData &operator=(const CameraData &) = delete;\n> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> > index e5c4890501db71c8..be6507cd4bc0d1b9 100644\n> > --- a/src/libcamera/pipeline/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc.cpp\n> > @@ -38,7 +38,7 @@ class VimcCameraData : public CameraData\n> > {\n> > public:\n> > \tVimcCameraData(PipelineHandler *pipe)\n> > -\t\t: CameraData(pipe), sensor_(nullptr), debayer_(nullptr),\n> > +\t\t: CameraData(pipe, 1, 1), sensor_(nullptr), debayer_(nullptr),\n> > \t\t scaler_(nullptr), video_(nullptr), raw_(nullptr)\n> > \t{\n> > \t}\n> > @@ -100,8 +100,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 +359,6 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)\n> > \tif (!media)\n> > \t\treturn false;\n> > \n> > -\tipa_ = IPAManager::instance()->createIPA(this, 1, 1);\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..89b67806597728f9 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> > @@ -49,13 +50,20 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> > */\n> > \n> > /**\n> > - * \\fn CameraData::CameraData(PipelineHandler *pipe)\n> > + * \\fn CameraData::CameraData(PipelineHandler *pipe, uint32_t minIPAVersion,\n> > + * uint32_t maxIPAVersion)\n> > * \\brief Construct a CameraData instance for the given pipeline handler\n> > * \\param[in] pipe The pipeline handler\n> > + * \\param[in] minIPAVersion Minimum acceptable version of IPA module\n> > + * \\param[in] maxIPAVersion Maximum acceptable version of IPA module\n> > *\n> > * The reference to the pipeline handler is stored internally, the caller shall\n> > * guarantee that the pointer remains valid as long as the CameraData instance\n> > * exists.\n> > + *\n> > + * The IPA maximum and minimum version numbers are used to match with an IPA\n> > + * interface that would be compatible with the Camera. If no IPA interface\n> > + * is needed for the camera both parameters should be set to 0.\n> > */\n> > \n> > /**\n> > @@ -96,6 +104,24 @@ 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 IPA are in operation this should be set to nullptr.\n> \n> I'm not sure how to handle the pluralism there. I interpret \"IPA\" as\n> singular, which would mean this should read \"If no IPA is in operation...\"\n> \n> But you do define IPA as \"...Algorithms\" ... but I'm not sure the\n> acronym can convey the pluralism, so it would have to be \"If no IPAs are\n> in operation...\"\n> \n> A bit of googling makes me believe we should pluralise the acronym,\n> making it the \"If no IPAs are in operation...\" option.\n\nGood point.\n\nDo you still think the definition, 'Image Processing Algorithms (IPA)' \nis OK?\n\n> \n> > + */\n> > +\n> > +/**\n> > + * \\var CameraData::minIPAVersion_\n> > + * \\brief Minimum acceptable version of IPA module\n> > + */\n> > +\n> > +/**\n> > + * \\var CameraData::maxIPAVersion_\n> > + * \\brief Maximum acceptable version of IPA module\n> > + */\n> > +\n> > /**\n> > * \\class PipelineHandler\n> > * \\brief Create and manage cameras based on a set of media devices\n> > @@ -424,6 +450,17 @@ void PipelineHandler::completeRequest(Camera *camera, Request *request)\n> > void PipelineHandler::registerCamera(std::shared_ptr camera,\n> > \t\t\t\t std::unique_ptr data)\n> > {\n> > +\tif (data->minIPAVersion_ || data->maxIPAVersion_) {\n> > +\t\tdata->ipa_ = IPAManager::instance()->createIPA(this,\n> > +\t\t\t\t\t\t\t data->minIPAVersion_,\n> > +\t\t\t\t\t\t\t data->maxIPAVersion_);\n> > +\t\tif (!data->ipa_) {\n> > +\t\t\tLOG(Pipeline, Warning) << \"Skipping \" << camera->name()\n> > +\t\t\t\t\t << \" no IPA found\";\n> > +\t\t\treturn;\n> > +\t\t}\n> > +\t}\n> > +\n> \n> The code removed from VIMC calls:\n> \n> > -\t\tipa_->init();\n> \n> Shouldn't that be added here somewhere?\n> Or is that going to happen somewhere else?\n> \n> \n> Do we need to manually init() an ipa? Can't that happen as part of\n> createIPA() ?\n> \n> \n> With the correct resolution for the missing data->ipa_->init() call,\n> which perhaps is handled elsewhere, or can be added if required:\n\nThe IPAInterface::init() function is removed in patch 10/13. As it never \npreformed anything useful (other then printing hello world) it was not \nnoticed when testing. I will create a separate patch where it's removed \nbefore this patch in the series to make things clear, thanks for \nspotting this!\n\n> \n> Reviewed-by: Kieran Bingham \n> \n> \n> > \tdata->camera_ = camera.get();\n> > \tcameraData_[camera.get()] = std::move(data);\n> > \tcameras_.push_back(camera);\n> > \n> \n> -- \n> Regards\n> --\n> Kieran","headers":{"Return-Path":"","Received":["from mail-lf1-x141.google.com (mail-lf1-x141.google.com\n\t[IPv6:2a00:1450:4864:20::141])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 156DA60BE5\n\tfor ;\n\tThu, 29 Aug 2019 19:49:55 +0200 (CEST)","by mail-lf1-x141.google.com with SMTP id h27so3238379lfp.2\n\tfor ;\n\tThu, 29 Aug 2019 10:49:55 -0700 (PDT)","from localhost (h-177-236.A463.priv.bahnhof.se. [217.31.177.236])\n\tby smtp.gmail.com with ESMTPSA id\n\ts4sm581453lfd.77.2019.08.29.10.49.53\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 29 Aug 2019 10:49:53 -0700 (PDT)"],"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\t:user-agent; bh=H5tnPrSdgwm5yKISUMCp4oNIoTWDRZjXpfqtfqn7isQ=;\n\tb=01MaN0nqnVzb7ysdZtAdeWJPlPKZUetiEIq/glk0Ryr8A11p/QC2lCWVY6rPhVHk5g\n\t7KAwtN0pGXjZgrZQ9lGYBQqIibzpZ+KpWGDkna/jQarbvefEpwwyGS/d56e3bKfJRUqb\n\tJCfOCrye18OCbuFVqvlghqJL+0VKgnaimZ/DcL3m8bCNytbiJP9ooyrTJiC7T/EO97iI\n\tEGUTecbvJ00QBJ4EVBmeysxHdmveRBFxBd7Q/PhTqZxHc9TJzW870ihMYVd0YHqq6IrO\n\tC5p2LSFapQZvT91/nyG0FSeBBm4j1za29/UY2trojMb2MeECJ7mSyKtEsC0ms/TD+wVs\n\twqjg==","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:user-agent;\n\tbh=H5tnPrSdgwm5yKISUMCp4oNIoTWDRZjXpfqtfqn7isQ=;\n\tb=gfBojsHheYgxxeq0oFvhy+wy34XMKda0ZNcvvkgETqLaJCRrkiVLzeDRMS+sHf3AXZ\n\tWJ2KjRofL7QdBb3tH4LSjSM0Irm4MfJ9vbqCLiMtyJJpsFLFW/ubSKwhjji1O+OzwqCq\n\t0nav+YxohmrUhjVB2VSndnqwnMyQ9WCSYUwGhkn5McqG702rEl2tzx8zZxZ2cAF9uk5T\n\tacoWXYYbKPZjOomc0uDjc2sUKS7Slw72b/zJTviwgouaLJYq2E6h8WK3m+8eBN0nwrgD\n\tz3vjzBj+NY6H5OG/Pcq6LCbQ0RG6UfsZRpVggmWK7qb4PdTKQ0w5S6AWEnqEjte+DAQ7\n\toDsw==","X-Gm-Message-State":"APjAAAXVxVkiQ7FvopGZEpwwZLOVMS0LFxzuyKFdx8t3uL2qaWarWjxN\n\thCmnwOT2crEi0PkeEcDOVzt6/GleJRY=","X-Google-Smtp-Source":"APXvYqxgZIT/nVhdnhe/LDBPJ3OlpDbbBrZBXSe8JiOHbrj9XvZPqa1MVcl0Rsy/n5d5KRJitGXHQw==","X-Received":"by 2002:a19:6f09:: with SMTP id k9mr7036450lfc.102.1567100994235;\n\tThu, 29 Aug 2019 10:49:54 -0700 (PDT)","Date":"Thu, 29 Aug 2019 19:49:53 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190829174953.GA8479@bigcity.dyn.berto.se>","References":"<20190828011710.32128-1-niklas.soderlund@ragnatech.se>\n\t<20190828011710.32128-3-niklas.soderlund@ragnatech.se>\n\t<64ade229-0de8-3c4e-f982-0643d1de33b4@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<64ade229-0de8-3c4e-f982-0643d1de33b4@ideasonboard.com>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH 02/13] libcamera: pipeline: Move IPA\n\tfrom pipeline to camera data","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 29 Aug 2019 17:49:55 -0000"}}]