[{"id":31215,"web_url":"https://patchwork.libcamera.org/comment/31215/","msgid":"<172622370732.3474483.3290771303420267725@ping.linuxembedded.co.uk>","date":"2024-09-13T10:35:07","subject":"Re: [PATCH v2 9/9] ipa: rkisp1: Add polynomial LSC loader","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2024-09-13 08:57:27)\n> Add a loader that is capable to load polynomial coefficients from the\n> tuning files. The polynoms are sampled at load time to reduce the\n\nI think 'polynom' might be a german term? I haven't heard it used before\nand google suddenly started giving me lots of non-english hits.\n\nPerhaps just \"The polynomials are sampled\" ... but I understood it\nanyway so I don't mind keeping it ;-)\n\n> computational overhead at runtime.\n> \n> Signed-off-by: Stefan Klug \n> ---\n> src/ipa/rkisp1/algorithms/lsc.cpp | 126 +++++++++++++++++++++++++++++-\n> 1 file changed, 125 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp\n> index 12867b8f7d0f..b0ad2234cd11 100644\n> --- a/src/ipa/rkisp1/algorithms/lsc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/lsc.cpp\n> @@ -16,6 +16,7 @@\n> \n> #include \"libcamera/internal/yaml_parser.h\"\n> \n> +#include \"libipa/lsc_polynomial.h\"\n> #include \"linux/rkisp1-config.h\"\n> \n> /**\n> @@ -70,6 +71,123 @@ namespace ipa::rkisp1::algorithms {\n> \n> LOG_DEFINE_CATEGORY(RkISP1Lsc)\n> \n> +class LscPolynomialLoader\n> +{\n> +public:\n> + LscPolynomialLoader(const Size &sensorSize,\n> + const Rectangle &cropRectangle,\n> + const std::vector &xSizes,\n> + const std::vector &ySizes)\n> + : sensorSize_(sensorSize),\n> + cropRectangle_(cropRectangle),\n> + xSizes_(xSizes),\n> + ySizes_(ySizes)\n> + {\n> + }\n> +\n> + int parseLscData(const YamlObject &yamlSets,\n> + std::map &lscData)\n> + {\n> + const auto &sets = yamlSets.asList();\n> + for (const auto &yamlSet : sets) {\n> + std::optional pr, pgr, pgb, pb;\n> + uint32_t ct = yamlSet[\"ct\"].get(0);\n> +\n> + if (lscData.count(ct)) {\n> + LOG(RkISP1Lsc, Error)\n> + << \"Multiple sets found for \"\n> + << \"color temperature \" << ct;\n> + return -EINVAL;\n> + }\n> +\n> + LensShadingCorrection::Components &set = lscData[ct];\n> + pr = yamlSet[\"r\"].get();\n> + pgr = yamlSet[\"gr\"].get();\n> + pgb = yamlSet[\"gb\"].get();\n> + pb = yamlSet[\"b\"].get();\n> +\n> + if (!(pr || pgr || pgb || pb)) {\n> + LOG(RkISP1Lsc, Error)\n> + << \"Failed to parse polynomial for \"\n> + << \"colour temperature \" << ct;\n> + return -EINVAL;\n> + }\n> +\n> + set.ct = ct;\n> + pr->setReferenceImageSize(sensorSize_);\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> +\n> + if (lscData.empty()) {\n> + LOG(RkISP1Lsc, Error) << \"Failed to load any sets\";\n> + return -EINVAL;\n> + }\n> +\n> + return 0;\n> + }\n> +\n> +private:\n\nIt's private implementation detail, but this might be a good place to\nadd a comment or explicit reference to any external documentation that\ndetails the mirroring going on here.\n\nI was confused to start with ...\n\n> + std::vector sizesListToPositions(const std::vector &sizes)\n> + {\n> + const int half = sizes.size();\n> + std::vector res(half * 2 + 1);\n> + double x = 0.0;\n> +\n> + res[half] = 0.5;\n> + for (int i = 1; i <= half; i++) {\n> + x += sizes[half - i];\n> + res[half - i] = 0.5 - x;\n> + res[half + i] = 0.5 + x;\n> + }\n> +\n> + return res;\n> + }\n> +\n> + std::vector samplePolynomial(const LscPolynomial &poly)\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> + double w = cropRectangle_.width / m;\n> + double h = cropRectangle_.height / m;\n> + std::vector res;\n> +\n> + assert(xSizes_.size() * 2 + 1 == k);\n\nDo we evaluate/validate this when loading ?\n\n> +\n> + res.reserve(k * k);\n> +\n> + std::vector xPos(sizesListToPositions(xSizes_));\n> + std::vector yPos(sizesListToPositions(ySizes_));\n> +\n> + for (int x = 0; x < k; x++) {\n> + for (int y = 0; y < k; y++) {\n> + double xp = x0 + xPos[x] * w;\n> + double yp = y0 + yPos[y] * h;\n> + int v = static_cast(\n> + poly.sampleAtNormalizedPixelPos(xp, yp) *\n> + 1024);\n> +\n> + v = std::min(std::max(v, 1024), 4095);\n> + res.push_back(v);\n> + }\n> + }\n> + return res;\n> + }\n> +\n> + Size sensorSize_;\n> + Rectangle cropRectangle_;\n> + const std::vector &xSizes_;\n> + const std::vector &ySizes_;\n> +};\n> +\n> class LscClassicLoader\n> {\n> public:\n> @@ -193,11 +311,17 @@ int LensShadingCorrection::init([[maybe_unused]] IPAContext &context,\n> std::map lscData;\n> int res = 0;\n> std::optional type = tuningData[\"type\"].get();\n> - if (!type.has_value()) {\n> + if (!tuningData.contains(\"type\")) {\n\nDoes this need to change? We're not removing type variable so we can\nstill use it in both cases right?\n\n> LOG(RkISP1Lsc, Warning) << \"LSC data is in classic format. \"\n> << \"This will be deprecated soon.\";\n> auto loader = LscClassicLoader();\n> res = loader.parseLscData(yamlSets, lscData);\n> + } else if (*type == \"polynomial\") {\n> + auto loader = LscPolynomialLoader(context.sensorInfo.activeAreaSize,\n> + context.sensorInfo.analogCrop,\n> + xSize_,\n> + ySize_);\n> + res = loader.parseLscData(yamlSets, lscData);\n> } else {\n> LOG(RkISP1Lsc, Error) << \"Unsupported LSC type '\" << *type << \"'\";\n> res = -EINVAL;\n> -- \n> 2.43.0\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 39A65C324C\n\tfor ;\n\tFri, 13 Sep 2024 10:35:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E0F10634FB;\n\tFri, 13 Sep 2024 12:35:12 +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 32643634F4\n\tfor ;\n\tFri, 13 Sep 2024 12:35:11 +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 1371873;\n\tFri, 13 Sep 2024 12:33:52 +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=\"WYR/5v7b\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1726223632;\n\tbh=ZJ3XL6P9P1r1Tanzas7OJRpJJvLZSjno51G5obMCndM=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=WYR/5v7bqOVVOFrlqJblFkNenpvxlwG9FAcd+xjDDOKXXqMfLzptV1PBuP/klo9v/\n\tu+Jtk3y0dh7+AtfAxDz0KD0Vt/VaLQTTM6GNwq5HIZ6BbkjaEQYyDIrG2CCBaJvWuz\n\trTe9zyf3sqA0yNIKFMwyg/vjBcJNsUl9Q7BhdkF8=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240913075750.35115-10-stefan.klug@ideasonboard.com>","References":"<20240913075750.35115-1-stefan.klug@ideasonboard.com>\n\t<20240913075750.35115-10-stefan.klug@ideasonboard.com>","Subject":"Re: [PATCH v2 9/9] ipa: rkisp1: Add polynomial LSC loader","From":"Kieran Bingham ","Cc":"Stefan Klug ","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 13 Sep 2024 11:35:07 +0100","Message-ID":"<172622370732.3474483.3290771303420267725@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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":31222,"web_url":"https://patchwork.libcamera.org/comment/31222/","msgid":"","date":"2024-09-13T13:00:22","subject":"Re: [PATCH v2 9/9] ipa: rkisp1: Add polynomial LSC loader","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Fri, Sep 13, 2024 at 11:35:07AM +0100, Kieran Bingham wrote:\n> Quoting Stefan Klug (2024-09-13 08:57:27)\n> > Add a loader that is capable to load polynomial coefficients from the\n\ns/to/of/\n\nOther than that I second all of Kieran's questions/comments.\n\n\nPaul\n\n> > tuning files. The polynoms are sampled at load time to reduce the\n> \n> I think 'polynom' might be a german term? I haven't heard it used before\n> and google suddenly started giving me lots of non-english hits.\n> \n> Perhaps just \"The polynomials are sampled\" ... but I understood it\n> anyway so I don't mind keeping it ;-)\n> \n> > computational overhead at runtime.\n> > \n> > Signed-off-by: Stefan Klug \n> > ---\n> > src/ipa/rkisp1/algorithms/lsc.cpp | 126 +++++++++++++++++++++++++++++-\n> > 1 file changed, 125 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp\n> > index 12867b8f7d0f..b0ad2234cd11 100644\n> > --- a/src/ipa/rkisp1/algorithms/lsc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/lsc.cpp\n> > @@ -16,6 +16,7 @@\n> > \n> > #include \"libcamera/internal/yaml_parser.h\"\n> > \n> > +#include \"libipa/lsc_polynomial.h\"\n> > #include \"linux/rkisp1-config.h\"\n> > \n> > /**\n> > @@ -70,6 +71,123 @@ namespace ipa::rkisp1::algorithms {\n> > \n> > LOG_DEFINE_CATEGORY(RkISP1Lsc)\n> > \n> > +class LscPolynomialLoader\n> > +{\n> > +public:\n> > + LscPolynomialLoader(const Size &sensorSize,\n> > + const Rectangle &cropRectangle,\n> > + const std::vector &xSizes,\n> > + const std::vector &ySizes)\n> > + : sensorSize_(sensorSize),\n> > + cropRectangle_(cropRectangle),\n> > + xSizes_(xSizes),\n> > + ySizes_(ySizes)\n> > + {\n> > + }\n> > +\n> > + int parseLscData(const YamlObject &yamlSets,\n> > + std::map &lscData)\n> > + {\n> > + const auto &sets = yamlSets.asList();\n> > + for (const auto &yamlSet : sets) {\n> > + std::optional pr, pgr, pgb, pb;\n> > + uint32_t ct = yamlSet[\"ct\"].get(0);\n> > +\n> > + if (lscData.count(ct)) {\n> > + LOG(RkISP1Lsc, Error)\n> > + << \"Multiple sets found for \"\n> > + << \"color temperature \" << ct;\n> > + return -EINVAL;\n> > + }\n> > +\n> > + LensShadingCorrection::Components &set = lscData[ct];\n> > + pr = yamlSet[\"r\"].get();\n> > + pgr = yamlSet[\"gr\"].get();\n> > + pgb = yamlSet[\"gb\"].get();\n> > + pb = yamlSet[\"b\"].get();\n> > +\n> > + if (!(pr || pgr || pgb || pb)) {\n> > + LOG(RkISP1Lsc, Error)\n> > + << \"Failed to parse polynomial for \"\n> > + << \"colour temperature \" << ct;\n> > + return -EINVAL;\n> > + }\n> > +\n> > + set.ct = ct;\n> > + pr->setReferenceImageSize(sensorSize_);\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> > +\n> > + if (lscData.empty()) {\n> > + LOG(RkISP1Lsc, Error) << \"Failed to load any sets\";\n> > + return -EINVAL;\n> > + }\n> > +\n> > + return 0;\n> > + }\n> > +\n> > +private:\n> \n> It's private implementation detail, but this might be a good place to\n> add a comment or explicit reference to any external documentation that\n> details the mirroring going on here.\n> \n> I was confused to start with ...\n> \n> > + std::vector sizesListToPositions(const std::vector &sizes)\n> > + {\n> > + const int half = sizes.size();\n> > + std::vector res(half * 2 + 1);\n> > + double x = 0.0;\n> > +\n> > + res[half] = 0.5;\n> > + for (int i = 1; i <= half; i++) {\n> > + x += sizes[half - i];\n> > + res[half - i] = 0.5 - x;\n> > + res[half + i] = 0.5 + x;\n> > + }\n> > +\n> > + return res;\n> > + }\n> > +\n> > + std::vector samplePolynomial(const LscPolynomial &poly)\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> > + double w = cropRectangle_.width / m;\n> > + double h = cropRectangle_.height / m;\n> > + std::vector res;\n> > +\n> > + assert(xSizes_.size() * 2 + 1 == k);\n> \n> Do we evaluate/validate this when loading ?\n> \n> > +\n> > + res.reserve(k * k);\n> > +\n> > + std::vector xPos(sizesListToPositions(xSizes_));\n> > + std::vector yPos(sizesListToPositions(ySizes_));\n> > +\n> > + for (int x = 0; x < k; x++) {\n> > + for (int y = 0; y < k; y++) {\n> > + double xp = x0 + xPos[x] * w;\n> > + double yp = y0 + yPos[y] * h;\n> > + int v = static_cast(\n> > + poly.sampleAtNormalizedPixelPos(xp, yp) *\n> > + 1024);\n> > +\n> > + v = std::min(std::max(v, 1024), 4095);\n> > + res.push_back(v);\n> > + }\n> > + }\n> > + return res;\n> > + }\n> > +\n> > + Size sensorSize_;\n> > + Rectangle cropRectangle_;\n> > + const std::vector &xSizes_;\n> > + const std::vector &ySizes_;\n> > +};\n> > +\n> > class LscClassicLoader\n> > {\n> > public:\n> > @@ -193,11 +311,17 @@ int LensShadingCorrection::init([[maybe_unused]] IPAContext &context,\n> > std::map lscData;\n> > int res = 0;\n> > std::optional type = tuningData[\"type\"].get();\n> > - if (!type.has_value()) {\n> > + if (!tuningData.contains(\"type\")) {\n> \n> Does this need to change? We're not removing type variable so we can\n> still use it in both cases right?\n> \n> > LOG(RkISP1Lsc, Warning) << \"LSC data is in classic format. \"\n> > << \"This will be deprecated soon.\";\n> > auto loader = LscClassicLoader();\n> > res = loader.parseLscData(yamlSets, lscData);\n> > + } else if (*type == \"polynomial\") {\n> > + auto loader = LscPolynomialLoader(context.sensorInfo.activeAreaSize,\n> > + context.sensorInfo.analogCrop,\n> > + xSize_,\n> > + ySize_);\n> > + res = loader.parseLscData(yamlSets, lscData);\n> > } else {\n> > LOG(RkISP1Lsc, Error) << \"Unsupported LSC type '\" << *type << \"'\";\n> > res = -EINVAL;\n> > -- \n> > 2.43.0\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 85A80C3257\n\tfor ;\n\tFri, 13 Sep 2024 13:00:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4E63A634F8;\n\tFri, 13 Sep 2024 15:00:28 +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 C4BF1634E3\n\tfor ;\n\tFri, 13 Sep 2024 15:00:26 +0200 (CEST)","from pyrite.rasen.tech (213-229-8-243.static.upcbusiness.at\n\t[213.229.8.243])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 79BB5E0D;\n\tFri, 13 Sep 2024 14:59:07 +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=\"ij2gmzPJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1726232347;\n\tbh=kIqe9AA2ESuwotOyvPNTF9T2IneJgGIfzih/68yr4rM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ij2gmzPJvOeeelFPjvJWCu1Y5FW6XPCHrYPFP43ylVAvtDM7F5c4P5Rf9JeOBR4jn\n\t/vk86l9g5jpmO5oKIIM/Th9Rt5gt7RNttUb91TeoagDgF2mYNBeEyJubOYyJYwkmln\n\tL2ighdAc3A/nUOrfnWx7iwvLB29hZH/+Z4F/E28U=","Date":"Fri, 13 Sep 2024 15:00:22 +0200","From":"Paul Elder ","To":"Kieran Bingham ","Cc":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 9/9] ipa: rkisp1: Add polynomial LSC loader","Message-ID":"","References":"<20240913075750.35115-1-stefan.klug@ideasonboard.com>\n\t<20240913075750.35115-10-stefan.klug@ideasonboard.com>\n\t<172622370732.3474483.3290771303420267725@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<172622370732.3474483.3290771303420267725@ping.linuxembedded.co.uk>","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":31253,"web_url":"https://patchwork.libcamera.org/comment/31253/","msgid":"","date":"2024-09-17T07:25:44","subject":"Re: [PATCH v2 9/9] ipa: rkisp1: Add polynomial LSC loader","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\nOn Fri, Sep 13, 2024 at 11:35:07AM +0100, Kieran Bingham wrote:\n> Quoting Stefan Klug (2024-09-13 08:57:27)\n> > Add a loader that is capable to load polynomial coefficients from the\n> > tuning files. The polynoms are sampled at load time to reduce the\n> \n> I think 'polynom' might be a german term? I haven't heard it used before\n> and google suddenly started giving me lots of non-english hits.\n> \n> Perhaps just \"The polynomials are sampled\" ... but I understood it\n> anyway so I don't mind keeping it ;-)\n> \n> > computational overhead at runtime.\n> > \n> > Signed-off-by: Stefan Klug \n> > ---\n> > src/ipa/rkisp1/algorithms/lsc.cpp | 126 +++++++++++++++++++++++++++++-\n> > 1 file changed, 125 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp\n> > index 12867b8f7d0f..b0ad2234cd11 100644\n> > --- a/src/ipa/rkisp1/algorithms/lsc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/lsc.cpp\n> > @@ -16,6 +16,7 @@\n> > \n> > #include \"libcamera/internal/yaml_parser.h\"\n> > \n> > +#include \"libipa/lsc_polynomial.h\"\n> > #include \"linux/rkisp1-config.h\"\n> > \n> > /**\n> > @@ -70,6 +71,123 @@ namespace ipa::rkisp1::algorithms {\n> > \n> > LOG_DEFINE_CATEGORY(RkISP1Lsc)\n> > \n> > +class LscPolynomialLoader\n> > +{\n> > +public:\n> > + LscPolynomialLoader(const Size &sensorSize,\n> > + const Rectangle &cropRectangle,\n> > + const std::vector &xSizes,\n> > + const std::vector &ySizes)\n> > + : sensorSize_(sensorSize),\n> > + cropRectangle_(cropRectangle),\n> > + xSizes_(xSizes),\n> > + ySizes_(ySizes)\n> > + {\n> > + }\n> > +\n> > + int parseLscData(const YamlObject &yamlSets,\n> > + std::map &lscData)\n> > + {\n> > + const auto &sets = yamlSets.asList();\n> > + for (const auto &yamlSet : sets) {\n> > + std::optional pr, pgr, pgb, pb;\n> > + uint32_t ct = yamlSet[\"ct\"].get(0);\n> > +\n> > + if (lscData.count(ct)) {\n> > + LOG(RkISP1Lsc, Error)\n> > + << \"Multiple sets found for \"\n> > + << \"color temperature \" << ct;\n> > + return -EINVAL;\n> > + }\n> > +\n> > + LensShadingCorrection::Components &set = lscData[ct];\n> > + pr = yamlSet[\"r\"].get();\n> > + pgr = yamlSet[\"gr\"].get();\n> > + pgb = yamlSet[\"gb\"].get();\n> > + pb = yamlSet[\"b\"].get();\n> > +\n> > + if (!(pr || pgr || pgb || pb)) {\n> > + LOG(RkISP1Lsc, Error)\n> > + << \"Failed to parse polynomial for \"\n> > + << \"colour temperature \" << ct;\n> > + return -EINVAL;\n> > + }\n> > +\n> > + set.ct = ct;\n> > + pr->setReferenceImageSize(sensorSize_);\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> > +\n> > + if (lscData.empty()) {\n> > + LOG(RkISP1Lsc, Error) << \"Failed to load any sets\";\n> > + return -EINVAL;\n> > + }\n> > +\n> > + return 0;\n> > + }\n> > +\n> > +private:\n> \n> It's private implementation detail, but this might be a good place to\n> add a comment or explicit reference to any external documentation that\n> details the mirroring going on here.\n> \n> I was confused to start with ...\n\nI added documentation for that in v3.\n\n> \n> > + std::vector sizesListToPositions(const std::vector &sizes)\n> > + {\n> > + const int half = sizes.size();\n> > + std::vector res(half * 2 + 1);\n> > + double x = 0.0;\n> > +\n> > + res[half] = 0.5;\n> > + for (int i = 1; i <= half; i++) {\n> > + x += sizes[half - i];\n> > + res[half - i] = 0.5 - x;\n> > + res[half + i] = 0.5 + x;\n> > + }\n> > +\n> > + return res;\n> > + }\n> > +\n> > + std::vector samplePolynomial(const LscPolynomial &poly)\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> > + double w = cropRectangle_.width / m;\n> > + double h = cropRectangle_.height / m;\n> > + std::vector res;\n> > +\n> > + assert(xSizes_.size() * 2 + 1 == k);\n> \n> Do we evaluate/validate this when loading ?\n\nYes, this get's checked in parseSizes(). For completeness sake I added\nthe same assert for ySizes.\n\n> \n> > +\n> > + res.reserve(k * k);\n> > +\n> > + std::vector xPos(sizesListToPositions(xSizes_));\n> > + std::vector yPos(sizesListToPositions(ySizes_));\n> > +\n> > + for (int x = 0; x < k; x++) {\n> > + for (int y = 0; y < k; y++) {\n> > + double xp = x0 + xPos[x] * w;\n> > + double yp = y0 + yPos[y] * h;\n> > + int v = static_cast(\n> > + poly.sampleAtNormalizedPixelPos(xp, yp) *\n> > + 1024);\n> > +\n> > + v = std::min(std::max(v, 1024), 4095);\n> > + res.push_back(v);\n> > + }\n> > + }\n> > + return res;\n> > + }\n> > +\n> > + Size sensorSize_;\n> > + Rectangle cropRectangle_;\n> > + const std::vector &xSizes_;\n> > + const std::vector &ySizes_;\n> > +};\n> > +\n> > class LscClassicLoader\n> > {\n> > public:\n> > @@ -193,11 +311,17 @@ int LensShadingCorrection::init([[maybe_unused]] IPAContext &context,\n> > std::map lscData;\n> > int res = 0;\n> > std::optional type = tuningData[\"type\"].get();\n> > - if (!type.has_value()) {\n> > + if (!tuningData.contains(\"type\")) {\n> \n> Does this need to change? We're not removing type variable so we can\n> still use it in both cases right?\n\nRereading that sentence a few times I realized that this check shouldn't\nhave gone into patch 6. I cleaned that up for v3.\n\nBest regards,\nStefan\n\n> \n> > LOG(RkISP1Lsc, Warning) << \"LSC data is in classic format. \"\n> > << \"This will be deprecated soon.\";\n> > auto loader = LscClassicLoader();\n> > res = loader.parseLscData(yamlSets, lscData);\n> > + } else if (*type == \"polynomial\") {\n> > + auto loader = LscPolynomialLoader(context.sensorInfo.activeAreaSize,\n> > + context.sensorInfo.analogCrop,\n> > + xSize_,\n> > + ySize_);\n> > + res = loader.parseLscData(yamlSets, lscData);\n> > } else {\n> > LOG(RkISP1Lsc, Error) << \"Unsupported LSC type '\" << *type << \"'\";\n> > res = -EINVAL;\n> > -- \n> > 2.43.0\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 4B7FEC324C\n\tfor ;\n\tTue, 17 Sep 2024 07:25:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D30A5634FD;\n\tTue, 17 Sep 2024 09:25:48 +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 AE577618E8\n\tfor ;\n\tTue, 17 Sep 2024 09:25:47 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2001:871:25a:ec59:e135:30e5:315d:a7c1])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D3ED93C7;\n\tTue, 17 Sep 2024 09:24:25 +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=\"I7Hwe9dA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1726557865;\n\tbh=B+vLuznIjwuaFTPhX84LEUJlHKuw8O+CKV5AhYUJiDA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=I7Hwe9dA1sQZD4xyfyFbxWNV1DnoAdrBfkNUYDq++0Z2wC1j25+eDWuvaeQUIiCVv\n\tnRcjF/xY58DChJJ0CGj4QX/P0kmreSR19/Euji7iHzackgULxsxzOFuINvqTgRqRqr\n\tCNlLLiUKwObAEcMvfA/+UVYxc2E8uJcwMSMpAKlQ=","Date":"Tue, 17 Sep 2024 09:25:44 +0200","From":"Stefan Klug ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 9/9] ipa: rkisp1: Add polynomial LSC loader","Message-ID":"","References":"<20240913075750.35115-1-stefan.klug@ideasonboard.com>\n\t<20240913075750.35115-10-stefan.klug@ideasonboard.com>\n\t<172622370732.3474483.3290771303420267725@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<172622370732.3474483.3290771303420267725@ping.linuxembedded.co.uk>","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\" "}}]