[{"id":36238,"web_url":"https://patchwork.libcamera.org/comment/36238/","msgid":"","date":"2025-10-14T08:25:56","subject":"Re: [PATCH v1 08/12] ipa: rkisp1: lsc: Pass sampling positions into\n\tsamplePolynomial","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 10. 14. 9:52 keltezéssel, Stefan Klug írta:\n> There is no need to recalculate the sampling positions over and over.\n> Pass them as parameter into the sampling function. The vectors are still\n> kept inside the loop as this is also a preparatory change for the\n> upcoming refactoring.\n> \n> Signed-off-by: Stefan Klug \n> ---\n> src/ipa/rkisp1/algorithms/lsc.cpp | 37 +++++++++++++++----------------\n> 1 file changed, 18 insertions(+), 19 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp\n> index 0c1bb470c346..2fbc8bd1b242 100644\n> --- a/src/ipa/rkisp1/algorithms/lsc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/lsc.cpp\n> @@ -89,7 +89,9 @@ public:\n> \n> private:\n> \tstd::vector sizesListToPositions(const std::vector &sizes);\n> -\tstd::vector samplePolynomial(const LscPolynomial &poly);\n> +\tstd::vector samplePolynomial(const LscPolynomial &poly,\n> +\t\t\t\t\t const std::vector &xPositions,\n> +\t\t\t\t\t const std::vector &yPositions);\n> \n> \tSize sensorSize_;\n> \tRectangle cropRectangle_;\n> @@ -129,10 +131,13 @@ int LscPolynomialLoader::parseLscData(const YamlObject &yamlSets,\n> \t\tpgr->setReferenceImageSize(sensorSize_);\n> \t\tpgb->setReferenceImageSize(sensorSize_);\n> \t\tpb->setReferenceImageSize(sensorSize_);\n> -\t\tset.r = samplePolynomial(*pr);\n> -\t\tset.gr = samplePolynomial(*pgr);\n> -\t\tset.gb = samplePolynomial(*pgb);\n> -\t\tset.b = samplePolynomial(*pb);\n> +\n> +\t\tstd::vector xPos(sizesListToPositions(xSizes_));\n> +\t\tstd::vector yPos(sizesListToPositions(ySizes_));\n\nI suppose it might be just me, but I vastly prefer `=` in such cases.\n\n\n> +\t\tset.r = samplePolynomial(*pr, xPos, yPos);\n> +\t\tset.gr = samplePolynomial(*pgr, xPos, yPos);\n> +\t\tset.gb = samplePolynomial(*pgb, xPos, yPos);\n> +\t\tset.b = samplePolynomial(*pb, xPos, yPos);\n> \t}\n> \n> \tif (lscData.empty()) {\n> @@ -169,10 +174,10 @@ std::vector LscPolynomialLoader::sizesListToPositions(const std::vector<\n> \treturn positions;\n> }\n> \n> -std::vector LscPolynomialLoader::samplePolynomial(const LscPolynomial &poly)\n> +std::vector LscPolynomialLoader::samplePolynomial(const LscPolynomial &poly,\n> +\t\t\t\t\t\t\t const std::vector &xPositions,\n> +\t\t\t\t\t\t\t const std::vector &yPositions)\n\n Span xPositions, etc.\n\n?\n\n\n> {\n> -\tconstexpr int k = RKISP1_CIF_ISP_LSC_SAMPLES_MAX;\n> -\n> \tdouble m = poly.getM();\n> \tdouble x0 = cropRectangle_.x / m;\n> \tdouble y0 = cropRectangle_.y / m;\n> @@ -180,18 +185,12 @@ std::vector LscPolynomialLoader::samplePolynomial(const LscPolynomial\n> \tdouble h = cropRectangle_.height / m;\n> \tstd::vector samples;\n> \n> -\tASSERT(xSizes_.size() * 2 + 1 == k);\n> -\tASSERT(ySizes_.size() * 2 + 1 == k);\n\nIs it not worth to move these assertions instead of removing them?\n\n\nReviewed-by: Barnabás Pőcze \n\n\n> -\n> -\tsamples.reserve(k * k);\n> -\n> -\tstd::vector xPos(sizesListToPositions(xSizes_));\n> -\tstd::vector yPos(sizesListToPositions(ySizes_));\n> +\tsamples.reserve(xPositions.size() * yPositions.size());\n> \n> -\tfor (int y = 0; y < k; y++) {\n> -\t\tfor (int x = 0; x < k; x++) {\n> -\t\t\tdouble xp = x0 + xPos[x] * w;\n> -\t\t\tdouble yp = y0 + yPos[y] * h;\n> +\tfor (double y : yPositions) {\n> +\t\tfor (double x : xPositions) {\n> +\t\t\tdouble xp = x0 + x * w;\n> +\t\t\tdouble yp = y0 + y * h;\n> \t\t\t/*\n> \t\t\t * The hardware uses 2.10 fixed point format and limits\n> \t\t\t * the legal values to [1..3.999]. Scale and clamp the","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 1F42EBE080\n\tfor ;\n\tTue, 14 Oct 2025 08:26:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1ECFD605BD;\n\tTue, 14 Oct 2025 10:26:01 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0065060316\n\tfor ;\n\tTue, 14 Oct 2025 10:25:59 +0200 (CEST)","from [192.168.33.20] (185.182.214.105.nat.pool.zt.hu\n\t[185.182.214.105])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 973CCC73;\n\tTue, 14 Oct 2025 10:24:21 +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=\"VgiZ0OnM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1760430261;\n\tbh=wG27U81LAMvKeQYJhKlHo2paNfh0VZL1yDKKho2WcZw=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=VgiZ0OnMaPmwsq0o0JA+AqPGvUs2oOwqdJqzHCEtKkw+ikY8DO8MeKkckaax7Oqh1\n\tIWA2X3oTH7hFQdLUigqw6EbNkgeia4X8ays2W8TkRZ2eMThn6nlKqhxYW5D7nZ2Y4h\n\tM6ixInoi2u+i2eyxIRC1lgtlPasE3RU3RSJ7Tjxw=","Message-ID":"","Date":"Tue, 14 Oct 2025 10:25:56 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1 08/12] ipa: rkisp1: lsc: Pass sampling positions into\n\tsamplePolynomial","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20251014075252.2876485-1-stefan.klug@ideasonboard.com>\n\t<20251014075252.2876485-9-stefan.klug@ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20251014075252.2876485-9-stefan.klug@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":36242,"web_url":"https://patchwork.libcamera.org/comment/36242/","msgid":"<176043737863.2966332.5948821274990634192@localhost>","date":"2025-10-14T10:22:58","subject":"Re: [PATCH v1 08/12] ipa: rkisp1: lsc: Pass sampling positions into\n\tsamplePolynomial","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi,\n\nThank you for the review.\n\nQuoting Barnabás Pőcze (2025-10-14 10:25:56)\n> Hi\n> \n> 2025. 10. 14. 9:52 keltezéssel, Stefan Klug írta:\n> > There is no need to recalculate the sampling positions over and over.\n> > Pass them as parameter into the sampling function. The vectors are still\n> > kept inside the loop as this is also a preparatory change for the\n> > upcoming refactoring.\n> > \n> > Signed-off-by: Stefan Klug \n> > ---\n> > src/ipa/rkisp1/algorithms/lsc.cpp | 37 +++++++++++++++----------------\n> > 1 file changed, 18 insertions(+), 19 deletions(-)\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp\n> > index 0c1bb470c346..2fbc8bd1b242 100644\n> > --- a/src/ipa/rkisp1/algorithms/lsc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/lsc.cpp\n> > @@ -89,7 +89,9 @@ public:\n> > \n> > private:\n> > std::vector sizesListToPositions(const std::vector &sizes);\n> > - std::vector samplePolynomial(const LscPolynomial &poly);\n> > + std::vector samplePolynomial(const LscPolynomial &poly,\n> > + const std::vector &xPositions,\n> > + const std::vector &yPositions);\n> > \n> > Size sensorSize_;\n> > Rectangle cropRectangle_;\n> > @@ -129,10 +131,13 @@ int LscPolynomialLoader::parseLscData(const YamlObject &yamlSets,\n> > pgr->setReferenceImageSize(sensorSize_);\n> > pgb->setReferenceImageSize(sensorSize_);\n> > pb->setReferenceImageSize(sensorSize_);\n> > - set.r = samplePolynomial(*pr);\n> > - set.gr = samplePolynomial(*pgr);\n> > - set.gb = samplePolynomial(*pgb);\n> > - set.b = samplePolynomial(*pb);\n> > +\n> > + std::vector xPos(sizesListToPositions(xSizes_));\n> > + std::vector yPos(sizesListToPositions(ySizes_));\n> \n> I suppose it might be just me, but I vastly prefer `=` in such cases.\n> \n> \n> > + set.r = samplePolynomial(*pr, xPos, yPos);\n> > + set.gr = samplePolynomial(*pgr, xPos, yPos);\n> > + set.gb = samplePolynomial(*pgb, xPos, yPos);\n> > + set.b = samplePolynomial(*pb, xPos, yPos);\n> > }\n> > \n> > if (lscData.empty()) {\n> > @@ -169,10 +174,10 @@ std::vector LscPolynomialLoader::sizesListToPositions(const std::vector<\n> > return positions;\n> > }\n> > \n> > -std::vector LscPolynomialLoader::samplePolynomial(const LscPolynomial &poly)\n> > +std::vector LscPolynomialLoader::samplePolynomial(const LscPolynomial &poly,\n> > + const std::vector &xPositions,\n> > + const std::vector &yPositions)\n> \n> Span xPositions, etc.\n\nAs it is local to that compile unit only, I guess it doesn't make a big\ndifference. But making it a habit might be worthwhile.\n\n> \n> ?\n> \n> \n> > {\n> > - constexpr int k = RKISP1_CIF_ISP_LSC_SAMPLES_MAX;\n> > -\n> > double m = poly.getM();\n> > double x0 = cropRectangle_.x / m;\n> > double y0 = cropRectangle_.y / m;\n> > @@ -180,18 +185,12 @@ std::vector LscPolynomialLoader::samplePolynomial(const LscPolynomial\n> > double h = cropRectangle_.height / m;\n> > std::vector samples;\n> > \n> > - ASSERT(xSizes_.size() * 2 + 1 == k);\n> > - ASSERT(ySizes_.size() * 2 + 1 == k);\n> \n> Is it not worth to move these assertions instead of removing them?\n\nThe sizes vectors are checked in parseSizes(). The sampling is\nindependent of the number of sampling points, so I believ it is fine to\njust drop these.\n\n> \n> \n> Reviewed-by: Barnabás Pőcze \n\nThanks,\nStefan\n\n> \n> \n> > -\n> > - samples.reserve(k * k);\n> > -\n> > - std::vector xPos(sizesListToPositions(xSizes_));\n> > - std::vector yPos(sizesListToPositions(ySizes_));\n> > + samples.reserve(xPositions.size() * yPositions.size());\n> > \n> > - for (int y = 0; y < k; y++) {\n> > - for (int x = 0; x < k; x++) {\n> > - double xp = x0 + xPos[x] * w;\n> > - double yp = y0 + yPos[y] * h;\n> > + for (double y : yPositions) {\n> > + for (double x : xPositions) {\n> > + double xp = x0 + x * w;\n> > + double yp = y0 + y * h;\n> > /*\n> > * The hardware uses 2.10 fixed point format and limits\n> > * the legal values to [1..3.999]. Scale and clamp the\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 78D2BBF415\n\tfor ;\n\tTue, 14 Oct 2025 10:23:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6F017605DE;\n\tTue, 14 Oct 2025 12:23:03 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CD5EB60443\n\tfor ;\n\tTue, 14 Oct 2025 12:23:01 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:61fb:8e55:ff1e:be62])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 60123741;\n\tTue, 14 Oct 2025 12:21:23 +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=\"sOPgXYKk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1760437283;\n\tbh=Y7lrLcrUXQyQUIN0S6JqNfxgl2eEZrlen2ZX8ZrEiEE=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=sOPgXYKkp8LvuMSfyFfCEKf28LttBvrrqHCarYVtMf1JL9pdh9ysSDNHsE67Pzp6f\n\tT4R1sjXwhpMeU4A4KrJMGxZAopfbAQYgLRrWbRgb7CNhm9S2kAtChOCBwlObHNZIJ9\n\t7DZGUBMfePZ0t4DtBU0OuoVl5/MA6il6boX9yNEU=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20251014075252.2876485-1-stefan.klug@ideasonboard.com>\n\t<20251014075252.2876485-9-stefan.klug@ideasonboard.com>\n\t","Subject":"Re: [PATCH v1 08/12] ipa: rkisp1: lsc: Pass sampling positions into\n\tsamplePolynomial","From":"Stefan Klug ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 14 Oct 2025 12:22:58 +0200","Message-ID":"<176043737863.2966332.5948821274990634192@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":36243,"web_url":"https://patchwork.libcamera.org/comment/36243/","msgid":"<176043749982.2966332.15573990223485275682@localhost>","date":"2025-10-14T10:24:59","subject":"Re: [PATCH v1 08/12] ipa: rkisp1: lsc: Pass sampling positions into\n\tsamplePolynomial","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Quoting Stefan Klug (2025-10-14 12:22:58)\n> Hi,\n> \n> Thank you for the review.\n> \n> Quoting Barnabás Pőcze (2025-10-14 10:25:56)\n> > Hi\n> > \n> > 2025. 10. 14. 9:52 keltezéssel, Stefan Klug írta:\n> > > There is no need to recalculate the sampling positions over and over.\n> > > Pass them as parameter into the sampling function. The vectors are still\n> > > kept inside the loop as this is also a preparatory change for the\n> > > upcoming refactoring.\n> > > \n> > > Signed-off-by: Stefan Klug \n> > > ---\n> > > src/ipa/rkisp1/algorithms/lsc.cpp | 37 +++++++++++++++----------------\n> > > 1 file changed, 18 insertions(+), 19 deletions(-)\n> > > \n> > > diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp\n> > > index 0c1bb470c346..2fbc8bd1b242 100644\n> > > --- a/src/ipa/rkisp1/algorithms/lsc.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/lsc.cpp\n> > > @@ -89,7 +89,9 @@ public:\n> > > \n> > > private:\n> > > std::vector sizesListToPositions(const std::vector &sizes);\n> > > - std::vector samplePolynomial(const LscPolynomial &poly);\n> > > + std::vector samplePolynomial(const LscPolynomial &poly,\n> > > + const std::vector &xPositions,\n> > > + const std::vector &yPositions);\n> > > \n> > > Size sensorSize_;\n> > > Rectangle cropRectangle_;\n> > > @@ -129,10 +131,13 @@ int LscPolynomialLoader::parseLscData(const YamlObject &yamlSets,\n> > > pgr->setReferenceImageSize(sensorSize_);\n> > > pgb->setReferenceImageSize(sensorSize_);\n> > > pb->setReferenceImageSize(sensorSize_);\n> > > - set.r = samplePolynomial(*pr);\n> > > - set.gr = samplePolynomial(*pgr);\n> > > - set.gb = samplePolynomial(*pgb);\n> > > - set.b = samplePolynomial(*pb);\n> > > +\n> > > + std::vector xPos(sizesListToPositions(xSizes_));\n> > > + std::vector yPos(sizesListToPositions(ySizes_));\n> > \n> > I suppose it might be just me, but I vastly prefer `=` in such cases.\n\nI missed to comment on this one. Yes, I'll replace it.\n \n> > \n> > \n> > > + set.r = samplePolynomial(*pr, xPos, yPos);\n> > > + set.gr = samplePolynomial(*pgr, xPos, yPos);\n> > > + set.gb = samplePolynomial(*pgb, xPos, yPos);\n> > > + set.b = samplePolynomial(*pb, xPos, yPos);\n> > > }\n> > > \n> > > if (lscData.empty()) {\n> > > @@ -169,10 +174,10 @@ std::vector LscPolynomialLoader::sizesListToPositions(const std::vector<\n> > > return positions;\n> > > }\n> > > \n> > > -std::vector LscPolynomialLoader::samplePolynomial(const LscPolynomial &poly)\n> > > +std::vector LscPolynomialLoader::samplePolynomial(const LscPolynomial &poly,\n> > > + const std::vector &xPositions,\n> > > + const std::vector &yPositions)\n> > \n> > Span xPositions, etc.\n> \n> As it is local to that compile unit only, I guess it doesn't make a big\n> difference. But making it a habit might be worthwhile.\n> \n> > \n> > ?\n> > \n> > \n> > > {\n> > > - constexpr int k = RKISP1_CIF_ISP_LSC_SAMPLES_MAX;\n> > > -\n> > > double m = poly.getM();\n> > > double x0 = cropRectangle_.x / m;\n> > > double y0 = cropRectangle_.y / m;\n> > > @@ -180,18 +185,12 @@ std::vector LscPolynomialLoader::samplePolynomial(const LscPolynomial\n> > > double h = cropRectangle_.height / m;\n> > > std::vector samples;\n> > > \n> > > - ASSERT(xSizes_.size() * 2 + 1 == k);\n> > > - ASSERT(ySizes_.size() * 2 + 1 == k);\n> > \n> > Is it not worth to move these assertions instead of removing them?\n> \n> The sizes vectors are checked in parseSizes(). The sampling is\n> independent of the number of sampling points, so I believ it is fine to\n> just drop these.\n> \n> > \n> > \n> > Reviewed-by: Barnabás Pőcze \n> \n> Thanks,\n> Stefan\n> \n> > \n> > \n> > > -\n> > > - samples.reserve(k * k);\n> > > -\n> > > - std::vector xPos(sizesListToPositions(xSizes_));\n> > > - std::vector yPos(sizesListToPositions(ySizes_));\n> > > + samples.reserve(xPositions.size() * yPositions.size());\n> > > \n> > > - for (int y = 0; y < k; y++) {\n> > > - for (int x = 0; x < k; x++) {\n> > > - double xp = x0 + xPos[x] * w;\n> > > - double yp = y0 + yPos[y] * h;\n> > > + for (double y : yPositions) {\n> > > + for (double x : xPositions) {\n> > > + double xp = x0 + x * w;\n> > > + double yp = y0 + y * h;\n> > > /*\n> > > * The hardware uses 2.10 fixed point format and limits\n> > > * the legal values to [1..3.999]. Scale and clamp the\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 4F27BBF415\n\tfor ;\n\tTue, 14 Oct 2025 10:25:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 527EF605DE;\n\tTue, 14 Oct 2025 12:25:04 +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 DD7DB60443\n\tfor ;\n\tTue, 14 Oct 2025 12:25:02 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:61fb:8e55:ff1e:be62])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 828CE741;\n\tTue, 14 Oct 2025 12:23:24 +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=\"fOfeAdSl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1760437404;\n\tbh=mz8skGiwUQRYrDgvsHwIFIF7ouxBqIOZfzd+FBGYmLM=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=fOfeAdSlxb55gu5QL0iUtdA5/HcVG5n5aoPPOg0N638uU1guY/7x6TbJJ3l9bhoZ9\n\tjcSv8jQkiUtP+zAxe6koOC3cVKlajATb+Wp72MX5lQWJrgx6LcPB/mnE9iXqyihvlj\n\tMoyQyf9K6wWnQj9mmg5YhmNt55o39I1jGDzTI6AU=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<176043737863.2966332.5948821274990634192@localhost>","References":"<20251014075252.2876485-1-stefan.klug@ideasonboard.com>\n\t<20251014075252.2876485-9-stefan.klug@ideasonboard.com>\n\t\n\t<176043737863.2966332.5948821274990634192@localhost>","Subject":"Re: [PATCH v1 08/12] ipa: rkisp1: lsc: Pass sampling positions into\n\tsamplePolynomial","From":"Stefan Klug ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 14 Oct 2025 12:24:59 +0200","Message-ID":"<176043749982.2966332.15573990223485275682@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":36510,"web_url":"https://patchwork.libcamera.org/comment/36510/","msgid":"<20251028094753.GD13023@pendragon.ideasonboard.com>","date":"2025-10-28T09:47:53","subject":"Re: [PATCH v1 08/12] ipa: rkisp1: lsc: Pass sampling positions into\n\tsamplePolynomial","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Oct 14, 2025 at 12:24:59PM +0200, Stefan Klug wrote:\n> Quoting Stefan Klug (2025-10-14 12:22:58)\n> > Quoting Barnabás Pőcze (2025-10-14 10:25:56)\n> > > 2025. 10. 14. 9:52 keltezéssel, Stefan Klug írta:\n> > > > There is no need to recalculate the sampling positions over and over.\n> > > > Pass them as parameter into the sampling function. The vectors are still\n> > > > kept inside the loop as this is also a preparatory change for the\n> > > > upcoming refactoring.\n> > > > \n> > > > Signed-off-by: Stefan Klug \n> > > > ---\n> > > > src/ipa/rkisp1/algorithms/lsc.cpp | 37 +++++++++++++++----------------\n> > > > 1 file changed, 18 insertions(+), 19 deletions(-)\n> > > > \n> > > > diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp\n> > > > index 0c1bb470c346..2fbc8bd1b242 100644\n> > > > --- a/src/ipa/rkisp1/algorithms/lsc.cpp\n> > > > +++ b/src/ipa/rkisp1/algorithms/lsc.cpp\n> > > > @@ -89,7 +89,9 @@ public:\n> > > > \n> > > > private:\n> > > > std::vector sizesListToPositions(const std::vector &sizes);\n> > > > - std::vector samplePolynomial(const LscPolynomial &poly);\n> > > > + std::vector samplePolynomial(const LscPolynomial &poly,\n> > > > + const std::vector &xPositions,\n> > > > + const std::vector &yPositions);\n> > > > \n> > > > Size sensorSize_;\n> > > > Rectangle cropRectangle_;\n> > > > @@ -129,10 +131,13 @@ int LscPolynomialLoader::parseLscData(const YamlObject &yamlSets,\n> > > > pgr->setReferenceImageSize(sensorSize_);\n> > > > pgb->setReferenceImageSize(sensorSize_);\n> > > > pb->setReferenceImageSize(sensorSize_);\n> > > > - set.r = samplePolynomial(*pr);\n> > > > - set.gr = samplePolynomial(*pgr);\n> > > > - set.gb = samplePolynomial(*pgb);\n> > > > - set.b = samplePolynomial(*pb);\n> > > > +\n> > > > + std::vector xPos(sizesListToPositions(xSizes_));\n> > > > + std::vector yPos(sizesListToPositions(ySizes_));\n> > > \n> > > I suppose it might be just me, but I vastly prefer `=` in such cases.\n> \n> I missed to comment on this one. Yes, I'll replace it.\n> \n> > > \n> > > \n> > > > + set.r = samplePolynomial(*pr, xPos, yPos);\n> > > > + set.gr = samplePolynomial(*pgr, xPos, yPos);\n> > > > + set.gb = samplePolynomial(*pgb, xPos, yPos);\n> > > > + set.b = samplePolynomial(*pb, xPos, yPos);\n> > > > }\n> > > > \n> > > > if (lscData.empty()) {\n> > > > @@ -169,10 +174,10 @@ std::vector LscPolynomialLoader::sizesListToPositions(const std::vector<\n> > > > return positions;\n> > > > }\n> > > > \n> > > > -std::vector LscPolynomialLoader::samplePolynomial(const LscPolynomial &poly)\n> > > > +std::vector LscPolynomialLoader::samplePolynomial(const LscPolynomial &poly,\n> > > > + const std::vector &xPositions,\n> > > > + const std::vector &yPositions)\n> > > \n> > > Span xPositions, etc.\n> > \n> > As it is local to that compile unit only, I guess it doesn't make a big\n> > difference. But making it a habit might be worthwhile.\n> > \n> > > \n> > > ?\n> > > \n> > > \n> > > > {\n> > > > - constexpr int k = RKISP1_CIF_ISP_LSC_SAMPLES_MAX;\n> > > > -\n> > > > double m = poly.getM();\n> > > > double x0 = cropRectangle_.x / m;\n> > > > double y0 = cropRectangle_.y / m;\n> > > > @@ -180,18 +185,12 @@ std::vector LscPolynomialLoader::samplePolynomial(const LscPolynomial\n> > > > double h = cropRectangle_.height / m;\n> > > > std::vector samples;\n> > > > \n> > > > - ASSERT(xSizes_.size() * 2 + 1 == k);\n> > > > - ASSERT(ySizes_.size() * 2 + 1 == k);\n> > > \n> > > Is it not worth to move these assertions instead of removing them?\n> > \n> > The sizes vectors are checked in parseSizes(). The sampling is\n> > independent of the number of sampling points, so I believ it is fine to\n> > just drop these.\n> > \n> > > Reviewed-by: Barnabás Pőcze \n\nReviewed-by: Laurent Pinchart \n\n> > > > -\n> > > > - samples.reserve(k * k);\n> > > > -\n> > > > - std::vector xPos(sizesListToPositions(xSizes_));\n> > > > - std::vector yPos(sizesListToPositions(ySizes_));\n> > > > + samples.reserve(xPositions.size() * yPositions.size());\n> > > > \n> > > > - for (int y = 0; y < k; y++) {\n> > > > - for (int x = 0; x < k; x++) {\n> > > > - double xp = x0 + xPos[x] * w;\n> > > > - double yp = y0 + yPos[y] * h;\n> > > > + for (double y : yPositions) {\n> > > > + for (double x : xPositions) {\n> > > > + double xp = x0 + x * w;\n> > > > + double yp = y0 + y * h;\n> > > > /*\n> > > > * The hardware uses 2.10 fixed point format and limits\n> > > > * the legal values to [1..3.999]. Scale and clamp the","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 4C35EC3259\n\tfor ;\n\tTue, 28 Oct 2025 09:48:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CB7B8607B2;\n\tTue, 28 Oct 2025 10:48:08 +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 82929603ED\n\tfor ;\n\tTue, 28 Oct 2025 10:48:06 +0100 (CET)","from pendragon.ideasonboard.com (unknown [193.209.96.36])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 044538BC;\n\tTue, 28 Oct 2025 10:46:17 +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=\"ZcQdEmgH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1761644778;\n\tbh=9T6LabadCVnBvkVl1GHhqNyAnDDxwVkVa99pIyfiZSQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZcQdEmgHAh/jI7ciTH4fZAxl7QbKLtxUQHQvYSJtH9CvR6MwhkBdSE/aDMFrCLW8K\n\tnpOQCn+2BzZnnYfaG9AceT+wsa70eowCg1XEsvRuYUCRgZ9gTHW1lLgFa3LEsf42yX\n\tSvpSsqciMR4Mgo+eJeXA1LAzDaeVtcPLNUGBz5RE=","Date":"Tue, 28 Oct 2025 11:47:53 +0200","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 08/12] ipa: rkisp1: lsc: Pass sampling positions into\n\tsamplePolynomial","Message-ID":"<20251028094753.GD13023@pendragon.ideasonboard.com>","References":"<20251014075252.2876485-1-stefan.klug@ideasonboard.com>\n\t<20251014075252.2876485-9-stefan.klug@ideasonboard.com>\n\t\n\t<176043737863.2966332.5948821274990634192@localhost>\n\t<176043749982.2966332.15573990223485275682@localhost>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<176043749982.2966332.15573990223485275682@localhost>","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\" "}}]