[{"id":36090,"web_url":"https://patchwork.libcamera.org/comment/36090/","msgid":"<175944036966.3754595.12034782109458633224@ping.linuxembedded.co.uk>","date":"2025-10-02T21:26:09","subject":"Re: [PATCH v1 29/33] pipeline: rkisp1: Enable the dewarper based on\n\tthe tuning file","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2025-09-30 13:26:50)\n> To do actual lens dewarping, the dewarper will be configured based on\n> the tuning file. As a first step implement the basic loading of the\n> tuning file and enable/disable the dewarper for the given camera based\n> on the existence of the \"Dewarp\" algorithm in the tuning file.\n> \n> Todo: This is an backwards incompatible change in that the dewarper is\n> currently included in the chain unconditionally. Some users may want to\n> not use the dewarper, so it is sensible to make that configurable. If it\n> should be in the algorithms section or in a different one is open for\n> debate.\n> \n> Signed-off-by: Stefan Klug \n> ---\n> src/ipa/libipa/module.h | 4 ++\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 51 +++++++++++++++++++++++-\n> 2 files changed, 54 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/ipa/libipa/module.h b/src/ipa/libipa/module.h\n> index 0fb51916fff6..84386f901534 100644\n> --- a/src/ipa/libipa/module.h\n> +++ b/src/ipa/libipa/module.h\n> @@ -74,6 +74,10 @@ private:\n> int createAlgorithm(Context &context, const YamlObject &data)\n> {\n> const auto &[name, algoData] = *data.asDict().begin();\n> +\n> + if (name == \"Dewarp\")\n> + return 0;\n\nI think this means that really - we shouldn't put dewarp config under\nthe algorithms.\n\nIt should be a separate base key.\n\nPerhaps under convertors ?\n\n\n> +\n> std::unique_ptr> algo = createAlgorithm(name);\n> if (!algo) {\n> LOG(IPAModuleAlgo, Error)\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 1046930857e1..07dd5dc8d99a 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -16,6 +16,7 @@\n> #include \n> #include \n> \n> +#include \n> #include \n> #include \n> \n> @@ -47,6 +48,7 @@\n> #include \"libcamera/internal/v4l2_request.h\"\n> #include \"libcamera/internal/v4l2_subdevice.h\"\n> #include \"libcamera/internal/v4l2_videodevice.h\"\n> +#include \"libcamera/internal/yaml_parser.h\"\n> \n> #include \"rkisp1_path.h\"\n> \n> @@ -123,6 +125,7 @@ public:\n> */\n> MediaPipeline pipe_;\n> \n> + bool canUseDewarper_;\n> bool usesDewarper_;\n> \n> private:\n> @@ -131,6 +134,7 @@ private:\n> const ControlList &sensorControls);\n> \n> void metadataReady(unsigned int frame, const ControlList &metadata);\n> + int loadTuningFile(const std::string &file);\n> };\n> \n> class RkISP1CameraConfiguration : public CameraConfiguration\n> @@ -416,6 +420,51 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision, uint32_t supportedBlocks)\n> return ret;\n> }\n> \n> + ret = loadTuningFile(ipaTuningFile);\n> + if (ret < 0) {\n> + LOG(RkISP1, Error) << \"Failed to load tuning file\";\n> + return ret;\n> + }\n> +\n> + return 0;\n> +}\n> +\n> +int RkISP1CameraData::loadTuningFile(const std::string &path)\n> +{\n> + if (!pipe()->dewarper_)\n> + /* Nothing to do without dewarper */\n> + return 0;\n> +\n> + LOG(RkISP1, Debug) << \"Load tuning file \" << path;\n> +\n> + File file(path);\n> + if (!file.open(File::OpenModeFlag::ReadOnly)) {\n> + int ret = file.error();\n> + LOG(RkISP1, Error)\n> + << \"Failed to open tuning file \"\n> + << path << \": \" << strerror(-ret);\n> + return ret;\n> + }\n> +\n> + std::unique_ptr data = YamlParser::parse(file);\n> + if (!data)\n> + return -EINVAL;\n> +\n> + if (!data->contains(\"algorithms\")) {\n> + LOG(RkISP1, Error)\n> + << \"Tuning file doesn't contain any algorithm\";\n> + return -EINVAL;\n> + }\n> +\n> + const auto &algos = (*data)[\"algorithms\"].asList();\n> + for (const auto &algo : algos) {\n> + if (algo.contains(\"Dewarp\")) {\n> + const auto ¶ms = algo[\"Dewarp\"];\n\nparams doesn't seem to be used here ?\n\n> +\n> + canUseDewarper_ = true;\n> + }\n> + }\n\nSomewhat getting towards more modular pipeline handlers - perhaps we\nneed a list of convertors supported in the pipeline handler - and then\nwe pass the tuning file down to those to parse.\n\n\n> +\n> return 0;\n> }\n> \n> @@ -572,7 +621,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> */\n> bool transposeAfterIsp = false;\n> bool useDewarper = false;\n> - if (pipe->dewarper_) {\n> + if (data_->canUseDewarper_) {\n> /*\n> * Platforms with dewarper support, such as i.MX8MP, support\n> * only a single stream. We can inspect config_[0] only here.\n> -- \n> 2.48.1\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 C6EEAC324C\n\tfor ;\n\tThu, 2 Oct 2025 21:26:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D05D66B5F0;\n\tThu, 2 Oct 2025 23:26:13 +0200 (CEST)","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 5FCD86B5A2\n\tfor ;\n\tThu, 2 Oct 2025 23:26:12 +0200 (CEST)","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 51350F06;\n\tThu, 2 Oct 2025 23:24:42 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ieRuPUpH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1759440282;\n\tbh=H7qJTpgnUMjOfD7y5jKIExoBcVj2ZPPSDDF31M40aU4=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=ieRuPUpHVUjCx3eugKjuQLmAgGftWKsuDXGtAPOcokRUH7JKfL3ZdLKXIpDpR8guc\n\t9f15ZVRPfB5OktkO67HYQTba1miooxiM8wGBQSsD9gxEdT4kGadawUdsH7vC6VQhOl\n\tDUdUxgjIkwPThDh45D4xMO7z8PA1EJIOmUeyR3jA=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250930122726.1837524-30-stefan.klug@ideasonboard.com>","References":"<20250930122726.1837524-1-stefan.klug@ideasonboard.com>\n\t<20250930122726.1837524-30-stefan.klug@ideasonboard.com>","Subject":"Re: [PATCH v1 29/33] pipeline: rkisp1: Enable the dewarper based on\n\tthe tuning file","From":"Kieran Bingham ","Cc":"Stefan Klug ","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 02 Oct 2025 22:26:09 +0100","Message-ID":"<175944036966.3754595.12034782109458633224@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","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":36135,"web_url":"https://patchwork.libcamera.org/comment/36135/","msgid":"<175974295870.3214037.4652108102724038439@localhost>","date":"2025-10-06T09:29:18","subject":"Re: [PATCH v1 29/33] pipeline: rkisp1: Enable the dewarper based on\n\tthe tuning file","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the review.\n\nQuoting Kieran Bingham (2025-10-02 23:26:09)\n> Quoting Stefan Klug (2025-09-30 13:26:50)\n> > To do actual lens dewarping, the dewarper will be configured based on\n> > the tuning file. As a first step implement the basic loading of the\n> > tuning file and enable/disable the dewarper for the given camera based\n> > on the existence of the \"Dewarp\" algorithm in the tuning file.\n> > \n> > Todo: This is an backwards incompatible change in that the dewarper is\n> > currently included in the chain unconditionally. Some users may want to\n> > not use the dewarper, so it is sensible to make that configurable. If it\n> > should be in the algorithms section or in a different one is open for\n> > debate.\n> > \n> > Signed-off-by: Stefan Klug \n> > ---\n> > src/ipa/libipa/module.h | 4 ++\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 51 +++++++++++++++++++++++-\n> > 2 files changed, 54 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/src/ipa/libipa/module.h b/src/ipa/libipa/module.h\n> > index 0fb51916fff6..84386f901534 100644\n> > --- a/src/ipa/libipa/module.h\n> > +++ b/src/ipa/libipa/module.h\n> > @@ -74,6 +74,10 @@ private:\n> > int createAlgorithm(Context &context, const YamlObject &data)\n> > {\n> > const auto &[name, algoData] = *data.asDict().begin();\n> > +\n> > + if (name == \"Dewarp\")\n> > + return 0;\n> \n> I think this means that really - we shouldn't put dewarp config under\n> the algorithms.\n> \n> It should be a separate base key.\n> \n> Perhaps under convertors ?\n\nYes, I didn't like that either. On the other hand it feels a bit\nsuperfluous to add another top level element for this single instance.\nFor example on the soft-gpu-isp side, implementing this as part of the\ngpu pipeline would be quite easy and then it is suddenly an algorithm\nagain?\n\nOne could also argue that algorithms contains all the algorithms\nimplemented in the pipeline. How that is done hardware wise is an\nimplementation detail. There are similar issues with the current\nalgorithm blocks on rkisp1. These are very centric to the\nhardware-blocks but could/should potentially be more centered around the\nalgorithms supported by libipa.\n\nAhh difficult :-)\n\n> \n> \n> > +\n> > std::unique_ptr> algo = createAlgorithm(name);\n> > if (!algo) {\n> > LOG(IPAModuleAlgo, Error)\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 1046930857e1..07dd5dc8d99a 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -16,6 +16,7 @@\n> > #include \n> > #include \n> > \n> > +#include \n> > #include \n> > #include \n> > \n> > @@ -47,6 +48,7 @@\n> > #include \"libcamera/internal/v4l2_request.h\"\n> > #include \"libcamera/internal/v4l2_subdevice.h\"\n> > #include \"libcamera/internal/v4l2_videodevice.h\"\n> > +#include \"libcamera/internal/yaml_parser.h\"\n> > \n> > #include \"rkisp1_path.h\"\n> > \n> > @@ -123,6 +125,7 @@ public:\n> > */\n> > MediaPipeline pipe_;\n> > \n> > + bool canUseDewarper_;\n> > bool usesDewarper_;\n> > \n> > private:\n> > @@ -131,6 +134,7 @@ private:\n> > const ControlList &sensorControls);\n> > \n> > void metadataReady(unsigned int frame, const ControlList &metadata);\n> > + int loadTuningFile(const std::string &file);\n> > };\n> > \n> > class RkISP1CameraConfiguration : public CameraConfiguration\n> > @@ -416,6 +420,51 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision, uint32_t supportedBlocks)\n> > return ret;\n> > }\n> > \n> > + ret = loadTuningFile(ipaTuningFile);\n> > + if (ret < 0) {\n> > + LOG(RkISP1, Error) << \"Failed to load tuning file\";\n> > + return ret;\n> > + }\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +int RkISP1CameraData::loadTuningFile(const std::string &path)\n> > +{\n> > + if (!pipe()->dewarper_)\n> > + /* Nothing to do without dewarper */\n> > + return 0;\n> > +\n> > + LOG(RkISP1, Debug) << \"Load tuning file \" << path;\n> > +\n> > + File file(path);\n> > + if (!file.open(File::OpenModeFlag::ReadOnly)) {\n> > + int ret = file.error();\n> > + LOG(RkISP1, Error)\n> > + << \"Failed to open tuning file \"\n> > + << path << \": \" << strerror(-ret);\n> > + return ret;\n> > + }\n> > +\n> > + std::unique_ptr data = YamlParser::parse(file);\n> > + if (!data)\n> > + return -EINVAL;\n> > +\n> > + if (!data->contains(\"algorithms\")) {\n> > + LOG(RkISP1, Error)\n> > + << \"Tuning file doesn't contain any algorithm\";\n> > + return -EINVAL;\n> > + }\n> > +\n> > + const auto &algos = (*data)[\"algorithms\"].asList();\n> > + for (const auto &algo : algos) {\n> > + if (algo.contains(\"Dewarp\")) {\n> > + const auto ¶ms = algo[\"Dewarp\"];\n> \n> params doesn't seem to be used here ?\n\nOh yes, that should be moved to the next patch.\n\n> \n> > +\n> > + canUseDewarper_ = true;\n> > + }\n> > + }\n> \n> Somewhat getting towards more modular pipeline handlers - perhaps we\n> need a list of convertors supported in the pipeline handler - and then\n> we pass the tuning file down to those to parse.\n> \n\nYes, I feel these are first tiny steps in that direction, but I still\nmiss a full picture.\n\nBest regards,\nStefan\n\n> \n> > +\n> > return 0;\n> > }\n> > \n> > @@ -572,7 +621,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > */\n> > bool transposeAfterIsp = false;\n> > bool useDewarper = false;\n> > - if (pipe->dewarper_) {\n> > + if (data_->canUseDewarper_) {\n> > /*\n> > * Platforms with dewarper support, such as i.MX8MP, support\n> > * only a single stream. We can inspect config_[0] only here.\n> > -- \n> > 2.48.1\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 CD1E1C324C\n\tfor ;\n\tMon, 6 Oct 2025 09:29:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E4E6D6B5F3;\n\tMon, 6 Oct 2025 11:29:23 +0200 (CEST)","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 2150162C35\n\tfor ;\n\tMon, 6 Oct 2025 11:29:22 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:2e2:f331:f320:caae])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 542CC1741; \n\tMon, 6 Oct 2025 11:27:49 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"T1Yl+Bvk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1759742869;\n\tbh=MKyz/zG5wL3F3kp6phDlQNesVcUW/KqwqQ/SIyw/2lI=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=T1Yl+BvklSPfuPpnoU8/XMPWl2ZlS5kdksQSsPaKTK92QBIfNohP6+BV/3zbhBxWW\n\tBRm0qo3xunSRAX5CuqFf/f1+ZUZeel/7K8GZbDyHm7sea/KJNUV8hUjDEHsdqsUl9l\n\tjLT/btbAUKHqyTXKkKpPsolqUOSTeOmZ7rplfErs=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<175944036966.3754595.12034782109458633224@ping.linuxembedded.co.uk>","References":"<20250930122726.1837524-1-stefan.klug@ideasonboard.com>\n\t<20250930122726.1837524-30-stefan.klug@ideasonboard.com>\n\t<175944036966.3754595.12034782109458633224@ping.linuxembedded.co.uk>","Subject":"Re: [PATCH v1 29/33] pipeline: rkisp1: Enable the dewarper based on\n\tthe tuning file","From":"Stefan Klug ","Cc":"","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 06 Oct 2025 11:29:18 +0200","Message-ID":"<175974295870.3214037.4652108102724038439@localhost>","User-Agent":"alot/0.12.dev8+g2c003385c862.d20250602","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\" "}}]