[{"id":36730,"web_url":"https://patchwork.libcamera.org/comment/36730/","msgid":"<176243147468.3666756.7440065195284876672@ping.linuxembedded.co.uk>","date":"2025-11-06T12:17:54","subject":"Re: [PATCH v2 31/35] pipeline: rkisp1: Load dewarp parameters from\n\ttuning 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-10-23 15:48:32)\n> Load the dewarp parameters from the tuning file.\n> \n> Signed-off-by: Stefan Klug \n> \n> ---\n> \n> Changes in v2:\n> - Dropped unused variable\n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 33 +++++++++++++++++++++++-\n> 1 file changed, 32 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index eaf82d0f1097..2280a5554f5a 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -125,6 +125,11 @@ public:\n> */\n> MediaPipeline pipe_;\n> \n> + struct DewarpParms {\n> + Matrix cm;\n> + std::vector coeffs;\n> + };\n> + std::optional dewarpParams_;\n\nCan we push these out of the rkisp1.cpp and into the dw100 ?\n\n\n\n> bool canUseDewarper_;\n> bool usesDewarper_;\n> \n> @@ -458,8 +463,30 @@ int RkISP1CameraData::loadTuningFile(const std::string &path)\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\nI don't remember if I've said this on the series yet, but I don't think\nDewarp should be in the algorithms section of the tuning file.\n\nI think we should have a specific 'convertors'(or otherwise) key to put this under.\n\n\nI really don't want the IPA to have to have \n\nfor algo in algorithms:\n\tif algo in [list of probably not algo]\n\t\treturn;\n\n\tprocess(algo);\n\n\n\n> + if (params) {\n> canUseDewarper_ = true;\n> + DewarpParms dp;\n> + if (params[\"cm\"]) {\n> + const auto &cm = params[\"cm\"].get>();\n> + if (!cm) {\n> + LOG(RkISP1, Error) << \"Dewarp parameters are missing 'cm' value\";\n> + return -EINVAL;\n> + }\n> + dp.cm = *cm;\n> + }\n> +\n> + if (params[\"coefficients\"]) {\n> + const auto &coeffs = params[\"coefficients\"].getList();\n> + if (!coeffs) {\n> + LOG(RkISP1, Error) << \"Dewarp parameters 'coefficients' value is not a list\";\n> + return -EINVAL;\n> + }\n> + dp.coeffs = *coeffs;\n> + }\n> +\n\nSame here - can we put this parsing into the dw100.cpp if it's specific\nto that.\n\nI'd like to think about how can we make it minimally possible to\nintroduce the dw100 on the ISI pipeline handler as well as the RKISP1.\n\nWhat can we do to make the interface such that all the common handling\nis not duplicated.\n\n--\nKieran\n\n\n> + dewarpParams_ = dp;\n> + }\n> }\n> \n> return 0;\n> @@ -1050,6 +1077,10 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> auto &vertexMap = dewarper_->vertexMap(cfg.stream());\n> vertexMap.setSensorCrop(sensorCrop);\n> vertexMap.setTransform(config->combinedTransform());\n> + if (data->dewarpParams_) {\n> + vertexMap.setDewarpParams(data->dewarpParams_->cm,\n> + data->dewarpParams_->coeffs);\n> + }\n> data->properties_.set(properties::ScalerCropMaximum, sensorCrop);\n> \n> /*\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 C3D76C3241\n\tfor ;\n\tThu, 6 Nov 2025 12:17:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 07BBB60A81;\n\tThu, 6 Nov 2025 13:17:58 +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 46949606E6\n\tfor ;\n\tThu, 6 Nov 2025 13:17:57 +0100 (CET)","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 498826F3;\n\tThu, 6 Nov 2025 13:16:02 +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=\"ArYw7QCe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1762431362;\n\tbh=L+g855wZIeLU9BwBll9THi/1/emSGq51LIRyY0I4LVY=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=ArYw7QCexUjv6Yiu2qLhj+46Wgkd8ZAB0vEhtHbiHRPvAX17yKQ2+QXc9MLdT+p4q\n\tWixmtfFaQe3TAcCs8fHNwRg5p3+9RWvOKK+NoAvAcb0ZVLuTENEDuW4SlB+PjblKG5\n\tjxO1k4xEqhH5rs+fqjHYt9/aLcASwR47qNfZBx3g=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20251023144841.403689-32-stefan.klug@ideasonboard.com>","References":"<20251023144841.403689-1-stefan.klug@ideasonboard.com>\n\t<20251023144841.403689-32-stefan.klug@ideasonboard.com>","Subject":"Re: [PATCH v2 31/35] pipeline: rkisp1: Load dewarp parameters from\n\ttuning file","From":"Kieran Bingham ","Cc":"Stefan Klug ","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 06 Nov 2025 12:17:54 +0000","Message-ID":"<176243147468.3666756.7440065195284876672@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":36746,"web_url":"https://patchwork.libcamera.org/comment/36746/","msgid":"<176250938222.2116251.9658759608026917710@neptunite.rasen.tech>","date":"2025-11-07T09:56:22","subject":"Re: [PATCH v2 31/35] pipeline: rkisp1: Load dewarp parameters from\n\ttuning file","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Quoting Stefan Klug (2025-10-23 23:48:32)\n> Load the dewarp parameters from the tuning file.\n> \n> Signed-off-by: Stefan Klug \n\nLooks good to me.\n\nReviewed-by: Paul Elder \n\n> \n> ---\n> \n> Changes in v2:\n> - Dropped unused variable\n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 33 +++++++++++++++++++++++-\n> 1 file changed, 32 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index eaf82d0f1097..2280a5554f5a 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -125,6 +125,11 @@ public:\n> */\n> MediaPipeline pipe_;\n> \n> + struct DewarpParms {\n> + Matrix cm;\n> + std::vector coeffs;\n> + };\n> + std::optional dewarpParams_;\n> bool canUseDewarper_;\n> bool usesDewarper_;\n> \n> @@ -458,8 +463,30 @@ int RkISP1CameraData::loadTuningFile(const std::string &path)\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> + if (params) {\n> canUseDewarper_ = true;\n> + DewarpParms dp;\n> + if (params[\"cm\"]) {\n> + const auto &cm = params[\"cm\"].get>();\n> + if (!cm) {\n> + LOG(RkISP1, Error) << \"Dewarp parameters are missing 'cm' value\";\n> + return -EINVAL;\n> + }\n> + dp.cm = *cm;\n> + }\n> +\n> + if (params[\"coefficients\"]) {\n> + const auto &coeffs = params[\"coefficients\"].getList();\n> + if (!coeffs) {\n> + LOG(RkISP1, Error) << \"Dewarp parameters 'coefficients' value is not a list\";\n> + return -EINVAL;\n> + }\n> + dp.coeffs = *coeffs;\n> + }\n> +\n> + dewarpParams_ = dp;\n> + }\n> }\n> \n> return 0;\n> @@ -1050,6 +1077,10 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> auto &vertexMap = dewarper_->vertexMap(cfg.stream());\n> vertexMap.setSensorCrop(sensorCrop);\n> vertexMap.setTransform(config->combinedTransform());\n> + if (data->dewarpParams_) {\n> + vertexMap.setDewarpParams(data->dewarpParams_->cm,\n> + data->dewarpParams_->coeffs);\n> + }\n> data->properties_.set(properties::ScalerCropMaximum, sensorCrop);\n> \n> /*\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 9FB5FBDE4C\n\tfor ;\n\tFri, 7 Nov 2025 09:56:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C1E3D60A8B;\n\tFri, 7 Nov 2025 10:56:29 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3552A606D5\n\tfor ;\n\tFri, 7 Nov 2025 10:56:28 +0100 (CET)","from neptunite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:8ce5:1b8c:5343:a04e])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id EA2BD593;\n\tFri, 7 Nov 2025 10:54:31 +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=\"GkWrVd1A\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1762509272;\n\tbh=v45Gz0AgUKly6XZzmwDyRp9W2r7FXPZkkWxJIDZ8xyc=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=GkWrVd1A0f9CyhT2vFIXAApKqzgikAoqUuv0uRgUI4o1EC0wzzwXcIollv9hnmlc5\n\tdvEvPmCCsiDS1a9mLEszfc5r5QgXfXKqvZJJCLc50TEn6p8Ces76uBDnB3SEXmMiRv\n\t5kmRa5DzMqp6KKdlvVedz+JLzBIY8e0xyq2YHJQo=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20251023144841.403689-32-stefan.klug@ideasonboard.com>","References":"<20251023144841.403689-1-stefan.klug@ideasonboard.com>\n\t<20251023144841.403689-32-stefan.klug@ideasonboard.com>","Subject":"Re: [PATCH v2 31/35] pipeline: rkisp1: Load dewarp parameters from\n\ttuning file","From":"Paul Elder ","Cc":"Stefan Klug ","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 07 Nov 2025 18:56:22 +0900","Message-ID":"<176250938222.2116251.9658759608026917710@neptunite.rasen.tech>","User-Agent":"alot/0.0.0","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":36753,"web_url":"https://patchwork.libcamera.org/comment/36753/","msgid":"<176253353441.509848.1242168558466526603@localhost>","date":"2025-11-07T16:38:54","subject":"Re: [PATCH v2 31/35] pipeline: rkisp1: Load dewarp parameters from\n\ttuning 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-11-06 13:17:54)\n> Quoting Stefan Klug (2025-10-23 15:48:32)\n> > Load the dewarp parameters from the tuning file.\n> > \n> > Signed-off-by: Stefan Klug \n> > \n> > ---\n> > \n> > Changes in v2:\n> > - Dropped unused variable\n> > ---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 33 +++++++++++++++++++++++-\n> > 1 file changed, 32 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index eaf82d0f1097..2280a5554f5a 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -125,6 +125,11 @@ public:\n> > */\n> > MediaPipeline pipe_;\n> > \n> > + struct DewarpParms {\n> > + Matrix cm;\n> > + std::vector coeffs;\n> > + };\n> > + std::optional dewarpParams_;\n> \n> Can we push these out of the rkisp1.cpp and into the dw100 ?\n\nYou mean passing that struct to dewarper->setDewarpeParams as they are\nnever set alone? Yes, I can do that.\n\n> \n> \n> \n> > bool canUseDewarper_;\n> > bool usesDewarper_;\n> > \n> > @@ -458,8 +463,30 @@ int RkISP1CameraData::loadTuningFile(const std::string &path)\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> \n> I don't remember if I've said this on the series yet, but I don't think\n> Dewarp should be in the algorithms section of the tuning file.\n> \n> I think we should have a specific 'convertors'(or otherwise) key to put this under.\n\nYes, you did in https://patchwork.libcamera.org/patch/24528/ . And for\nthe same reasons I mentioned there I'm still unsure if we should do\nthat. Maybe time for a Tuesday topic...\n\n> \n> \n> I really don't want the IPA to have to have \n> \n> for algo in algorithms:\n> if algo in [list of probably not algo]\n> return;\n> \n> process(algo);\n> \n> \n> \n> > + if (params) {\n> > canUseDewarper_ = true;\n> > + DewarpParms dp;\n> > + if (params[\"cm\"]) {\n> > + const auto &cm = params[\"cm\"].get>();\n> > + if (!cm) {\n> > + LOG(RkISP1, Error) << \"Dewarp parameters are missing 'cm' value\";\n> > + return -EINVAL;\n> > + }\n> > + dp.cm = *cm;\n> > + }\n> > +\n> > + if (params[\"coefficients\"]) {\n> > + const auto &coeffs = params[\"coefficients\"].getList();\n> > + if (!coeffs) {\n> > + LOG(RkISP1, Error) << \"Dewarp parameters 'coefficients' value is not a list\";\n> > + return -EINVAL;\n> > + }\n> > + dp.coeffs = *coeffs;\n> > + }\n> > +\n> \n> Same here - can we put this parsing into the dw100.cpp if it's specific\n> to that.\n> \n> I'd like to think about how can we make it minimally possible to\n> introduce the dw100 on the ISI pipeline handler as well as the RKISP1.\n> \n> What can we do to make the interface such that all the common handling\n> is not duplicated.\n\nI understand the rationale. My understanding or maybe gut feeling was\nthat the classes in libcamera/converter should only provide the\ntechnical base and the actual control handling lives inside libipa or\nthe pipeline handlers. Do we really want to mix yaml/control handling\ninto the dw100 class? I agree that it also pollutes the rkisp1 class and\nmakes it difficult to reuse.\n\nMaybe I need to give it a try. Ahh I wish we had a concept for modular\npipelines...\n\nBest regards,\nStefan\n\n> \n> --\n> Kieran\n> \n> \n> > + dewarpParams_ = dp;\n> > + }\n> > }\n> > \n> > return 0;\n> > @@ -1050,6 +1077,10 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> > auto &vertexMap = dewarper_->vertexMap(cfg.stream());\n> > vertexMap.setSensorCrop(sensorCrop);\n> > vertexMap.setTransform(config->combinedTransform());\n> > + if (data->dewarpParams_) {\n> > + vertexMap.setDewarpParams(data->dewarpParams_->cm,\n> > + data->dewarpParams_->coeffs);\n> > + }\n> > data->properties_.set(properties::ScalerCropMaximum, sensorCrop);\n> > \n> > /*\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 D70B2BDE4C\n\tfor ;\n\tFri, 7 Nov 2025 16:38:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 326CA60A80;\n\tFri, 7 Nov 2025 17:38:58 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C3280608CF\n\tfor ;\n\tFri, 7 Nov 2025 17:38:56 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:8809:eccd:216c:b9a0])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id B87C71189; \n\tFri, 7 Nov 2025 17:37:00 +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=\"ITiOjGbF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1762533420;\n\tbh=C2+/NGw+hb5oWeomXi5NB6EdBSK0D+4IE06+SJ//qBg=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=ITiOjGbF9jpviV4vPrzVUDJTaGcIHo+wvfNWPbaMwBwP8mnH+lNG8yK6CmY41oQtz\n\tOMxuiy6l2M31ZSQl4FROwW+PkiBsuL6xXW45kt4wAkMpJ1vPJGrtpo1sjqKW1qwLU/\n\thVCIHWS3Lwkx0LxkkwhWn/totJB6gN2i+WlCUwNM=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<176243147468.3666756.7440065195284876672@ping.linuxembedded.co.uk>","References":"<20251023144841.403689-1-stefan.klug@ideasonboard.com>\n\t<20251023144841.403689-32-stefan.klug@ideasonboard.com>\n\t<176243147468.3666756.7440065195284876672@ping.linuxembedded.co.uk>","Subject":"Re: [PATCH v2 31/35] pipeline: rkisp1: Load dewarp parameters from\n\ttuning file","From":"Stefan Klug ","Cc":"","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 07 Nov 2025 17:38:54 +0100","Message-ID":"<176253353441.509848.1242168558466526603@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\" "}},{"id":36760,"web_url":"https://patchwork.libcamera.org/comment/36760/","msgid":"<176277002654.567526.2525586735607412696@ping.linuxembedded.co.uk>","date":"2025-11-10T10:20:26","subject":"Re: [PATCH v2 31/35] pipeline: rkisp1: Load dewarp parameters from\n\ttuning 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-11-07 16:38:54)\n> Hi Kieran,\n> \n> Thank you for the review.\n> \n> Quoting Kieran Bingham (2025-11-06 13:17:54)\n> > Quoting Stefan Klug (2025-10-23 15:48:32)\n> > > Load the dewarp parameters from the tuning file.\n> > > \n> > > Signed-off-by: Stefan Klug \n> > > \n> > > ---\n> > > \n> > > Changes in v2:\n> > > - Dropped unused variable\n> > > ---\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 33 +++++++++++++++++++++++-\n> > > 1 file changed, 32 insertions(+), 1 deletion(-)\n> > > \n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index eaf82d0f1097..2280a5554f5a 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -125,6 +125,11 @@ public:\n> > > */\n> > > MediaPipeline pipe_;\n> > > \n> > > + struct DewarpParms {\n> > > + Matrix cm;\n> > > + std::vector coeffs;\n> > > + };\n> > > + std::optional dewarpParams_;\n> > \n> > Can we push these out of the rkisp1.cpp and into the dw100 ?\n> \n> You mean passing that struct to dewarper->setDewarpeParams as they are\n> never set alone? Yes, I can do that.\n> \n> > \n> > \n> > \n> > > bool canUseDewarper_;\n> > > bool usesDewarper_;\n> > > \n> > > @@ -458,8 +463,30 @@ int RkISP1CameraData::loadTuningFile(const std::string &path)\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> > \n> > I don't remember if I've said this on the series yet, but I don't think\n> > Dewarp should be in the algorithms section of the tuning file.\n> > \n> > I think we should have a specific 'convertors'(or otherwise) key to put this under.\n> \n> Yes, you did in https://patchwork.libcamera.org/patch/24528/ . And for\n> the same reasons I mentioned there I'm still unsure if we should do\n> that. Maybe time for a Tuesday topic...\n\nAha, bringing that comment forward to here:\n\n\n> Kieran said:\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\n===== Stefan Reply =====\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===== Stefan Reply =====\n\nI think the ability to have a GPU dewarp *proves* my point that we\nshould not have code that does this in libipa:\n\n\n\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\nbecause now - dewarp can't be managed by the IPA there if we did want it\nto.\n\n\nIn the RKISP1 case, it is not the IPA/ISP handling the dewarp - it's a\nconvertor/post-processor.\n\nIf the (Soft)ISP were in control of the dewarp, I would agree there can\nbe a dewarp key under algorithms at that point.\n\nBut I think we have to distinguish the separation.\n\nMaybe 'algorithms' itself should actually be:\n\n ipa:\n algorithms\n\n\nor maybe we assume that is implicit already.\n\n\n\n> > I really don't want the IPA to have to have \n> > \n> > for algo in algorithms:\n> > if algo in [list of probably not algo]\n> > return;\n> > \n> > process(algo);\n> > \n> > \n> > \n> > > + if (params) {\n> > > canUseDewarper_ = true;\n> > > + DewarpParms dp;\n> > > + if (params[\"cm\"]) {\n> > > + const auto &cm = params[\"cm\"].get>();\n> > > + if (!cm) {\n> > > + LOG(RkISP1, Error) << \"Dewarp parameters are missing 'cm' value\";\n> > > + return -EINVAL;\n> > > + }\n> > > + dp.cm = *cm;\n> > > + }\n> > > +\n> > > + if (params[\"coefficients\"]) {\n> > > + const auto &coeffs = params[\"coefficients\"].getList();\n> > > + if (!coeffs) {\n> > > + LOG(RkISP1, Error) << \"Dewarp parameters 'coefficients' value is not a list\";\n> > > + return -EINVAL;\n> > > + }\n> > > + dp.coeffs = *coeffs;\n> > > + }\n> > > +\n> > \n> > Same here - can we put this parsing into the dw100.cpp if it's specific\n> > to that.\n> > \n> > I'd like to think about how can we make it minimally possible to\n> > introduce the dw100 on the ISI pipeline handler as well as the RKISP1.\n> > \n> > What can we do to make the interface such that all the common handling\n> > is not duplicated.\n> \n> I understand the rationale. My understanding or maybe gut feeling was\n> that the classes in libcamera/converter should only provide the\n> technical base and the actual control handling lives inside libipa or\n> the pipeline handlers. Do we really want to mix yaml/control handling\n> into the dw100 class? I agree that it also pollutes the rkisp1 class and\n> makes it difficult to reuse.\n\n\nYes, I really mean I think that yaml + control handling for the dw100\nshould be managed by the dw100 class!\n\nAt most - all the rkisp1 pipeline handler should be doing is passing the\nyaml and requests in through an interface to let dw100 make it's\ndecisionss.\n\n> \n> Maybe I need to give it a try. Ahh I wish we had a concept for modular\n> pipelines...\n\nYes, that's what the above works towards in my view.\n\nInstead of having decisions for modules made by the rkisp1 - let the\nmodules have control.\n\nDW100 is just 'one module'.\n\n--\nKieran\n\n> \n> Best regards,\n> Stefan\n> \n> > \n> > --\n> > Kieran\n> > \n> > \n> > > + dewarpParams_ = dp;\n> > > + }\n> > > }\n> > > \n> > > return 0;\n> > > @@ -1050,6 +1077,10 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> > > auto &vertexMap = dewarper_->vertexMap(cfg.stream());\n> > > vertexMap.setSensorCrop(sensorCrop);\n> > > vertexMap.setTransform(config->combinedTransform());\n> > > + if (data->dewarpParams_) {\n> > > + vertexMap.setDewarpParams(data->dewarpParams_->cm,\n> > > + data->dewarpParams_->coeffs);\n> > > + }\n> > > data->properties_.set(properties::ScalerCropMaximum, sensorCrop);\n> > > \n> > > /*\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 11511C3263\n\tfor ;\n\tMon, 10 Nov 2025 10:20:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 212E760A80;\n\tMon, 10 Nov 2025 11:20:31 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 82CDC6069A\n\tfor ;\n\tMon, 10 Nov 2025 11:20:29 +0100 (CET)","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 71D17C9;\n\tMon, 10 Nov 2025 11:18:31 +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=\"OLpmtSQB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1762769911;\n\tbh=G/wLtYlyPi4mTEdySHADwI0Emd0odpX2dclxtlDHoYI=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=OLpmtSQBI0u79Eg1wCR0LGg2ZvYrfMq40CmXVR5fRM1wF27fzW7aD2QoFdBeTE0ez\n\tCvGS2FsHhGQX1Lxzxsn+801z55PIEbp2q9hMV6xoCwA7QWhSp7RqzCevSwNJ/YXWb9\n\tocWTgCkIlhUpqA7v/S9S/aJC5cPLHYxxVpi9qMto=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<176253353441.509848.1242168558466526603@localhost>","References":"<20251023144841.403689-1-stefan.klug@ideasonboard.com>\n\t<20251023144841.403689-32-stefan.klug@ideasonboard.com>\n\t<176243147468.3666756.7440065195284876672@ping.linuxembedded.co.uk>\n\t<176253353441.509848.1242168558466526603@localhost>","Subject":"Re: [PATCH v2 31/35] pipeline: rkisp1: Load dewarp parameters from\n\ttuning file","From":"Kieran Bingham ","Cc":"","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 10 Nov 2025 10:20:26 +0000","Message-ID":"<176277002654.567526.2525586735607412696@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\" "}}]