[{"id":39652,"web_url":"https://patchwork.libcamera.org/comment/39652/","msgid":"<178359923880.2174993.9070488124922540636@ping.linuxembedded.co.uk>","date":"2026-07-09T12:13:58","subject":"Re: [PATCH v5 09/36] ipa: rkisp1: lsc: Re-architecture Lsc support","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2026-07-08 16:50:51)\n> The RkISP1 LSC implementation is realized by the following components:\n> \n> - 2 \"tuning file\" parser classes:\n> \n> - LscPolynomialLoader\n> - LscTableLoader\n> \n> which implement the \"parseLscData\" function that loads from tuning\n> file a polynomial representation of the lsc gains or a table of gains\n> respectively.\n> \n> - A hierachy of \"ShadingDescriptors\"\n> - LscPolynomialShadingDescriptor\n> - LscTableShadingDescriptor\n> \n> that implement the \"sampleForCrop\" interface which expands a\n> polynomial to a list of gains for the LscPolynomialShadingDescriptor\n> and simply return the data parsed from tuning file for\n> LscTableShadingDescriptor.\n> \n> Unify the two functionalities in a single class hierarchy whose\n> interface is defined by the LscImplementation pure virtual class\n> implemented by LscPolynomialImpl and LscTableImpl.\n> \n> The LensShadingCorrection algorithm class instantiates the correct\n> sub-class and uses the file parsing and table expansion functionalities\n> by implementing the composition pattern.\n> \n> Signed-off-by: Jacopo Mondi \n\nI've run through this twice and nothing stands out to me, and this is\nmostly a refactoring for later patches so I think I can just add this - \n\nIf CI's happy, I'm happy:\n\nReviewed-by: Kieran Bingham \n\n> ---\n> src/ipa/rkisp1/algorithms/lsc.cpp | 247 +++++++++++++++++---------------------\n> src/ipa/rkisp1/algorithms/lsc.h | 49 ++++----\n> 2 files changed, 141 insertions(+), 155 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp\n> index 06f5913c5750..38380aa839c6 100644\n> --- a/src/ipa/rkisp1/algorithms/lsc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/lsc.cpp\n> @@ -54,17 +54,6 @@ void Interpolator::\n> \n> namespace ipa::rkisp1::algorithms {\n> \n> -/**\n> - * \\class LensShadingCorrection\n> - * \\brief RkISP1 Lens Shading Correction control\n> - *\n> - * Due to the optical characteristics of the lens, the light intensity received\n> - * by the sensor is not uniform.\n> - *\n> - * The Lens Shading Correction algorithm applies multipliers to all pixels to\n> - * compensate for the lens shading effect. The coefficients are specified in a\n> - * downscaled table in the tuning data.\n> - */\n> \n> LOG_DEFINE_CATEGORY(RkISP1Lsc)\n> \n> @@ -72,54 +61,110 @@ namespace {\n> \n> constexpr int kColourTemperatureQuantization = 10;\n> \n> -class LscPolynomialShadingDescriptor : public LensShadingCorrection::ShadingDescriptor\n> +class LscPolynomialImpl : public LscImplementation\n> {\n> +private:\n> + struct PolynomialComponents {\n> + LscPolynomial pr;\n> + LscPolynomial pgr;\n> + LscPolynomial pgb;\n> + LscPolynomial pb;\n> + };\n> + using PolynomialComponentsMap = std::map;\n> +\n> public:\n> - LscPolynomialShadingDescriptor(const LscPolynomial &pr, const LscPolynomial &pgr,\n> - const LscPolynomial &pgb, const LscPolynomial &pb)\n> - : pr_(pr), pgr_(pgr), pgb_(pgb), pb_(pb)\n> + LscPolynomialImpl(const Size &sensorSize)\n> + : sensorSize_(sensorSize)\n> {\n> }\n> \n> - LensShadingCorrection::Components sampleForCrop(const Rectangle &cropRectangle,\n> - Span xSizes,\n> - Span ySizes) override;\n> + int parseLscData(const ValueNode &sets) override;\n> +\n> + LensShadingCorrection::ComponentsMap\n> + sampleForCrop(const Rectangle &cropRectangle,\n> + Span xSizes,\n> + Span ySizes) override;\n> \n> private:\n> + std::vector sizesListToPositions(Span sizes);\n> std::vector samplePolynomial(const LscPolynomial &poly,\n> Span xPositions,\n> Span yPositions,\n> const Rectangle &cropRectangle);\n> + PolynomialComponentsMap lscData_;\n> + Size sensorSize_;\n> +};\n> \n> - std::vector sizesListToPositions(Span sizes);\n> +int LscPolynomialImpl::parseLscData(const ValueNode &sets)\n> +{\n> + for (const auto &set : sets.asList()) {\n> + std::optional pr, pgr, pgb, pb;\n> + uint32_t ct = set[\"ct\"].get(0);\n> \n> - LscPolynomial pr_;\n> - LscPolynomial pgr_;\n> - LscPolynomial pgb_;\n> - LscPolynomial pb_;\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> + pr = set[\"r\"].get();\n> + pgr = set[\"gr\"].get();\n> + pgb = set[\"gb\"].get();\n> + pb = set[\"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> -LensShadingCorrection::Components\n> -LscPolynomialShadingDescriptor::sampleForCrop(const Rectangle &cropRectangle,\n> - Span xSizes,\n> - Span ySizes)\n> + pr->setReferenceImageSize(sensorSize_);\n> + pgr->setReferenceImageSize(sensorSize_);\n> + pgb->setReferenceImageSize(sensorSize_);\n> + pb->setReferenceImageSize(sensorSize_);\n> +\n> + lscData_.emplace(std::piecewise_construct,\n> + std::forward_as_tuple(ct),\n> + std::forward_as_tuple(PolynomialComponents{ *pr, *pgr, *pgb, *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> +LensShadingCorrection::ComponentsMap\n> +LscPolynomialImpl::sampleForCrop(const Rectangle &cropRectangle,\n> + Span xSizes,\n> + Span ySizes)\n> {\n> std::vector xPos = sizesListToPositions(xSizes);\n> std::vector yPos = sizesListToPositions(ySizes);\n> \n> - return {\n> - .r = samplePolynomial(pr_, xPos, yPos, cropRectangle),\n> - .gr = samplePolynomial(pgr_, xPos, yPos, cropRectangle),\n> - .gb = samplePolynomial(pgb_, xPos, yPos, cropRectangle),\n> - .b = samplePolynomial(pb_, xPos, yPos, cropRectangle)\n> - };\n> + LensShadingCorrection::ComponentsMap components;\n> +\n> + for (const auto &[k, p] : lscData_) {\n> + components[k] = {\n> + samplePolynomial(p.pr, xPos, yPos, cropRectangle),\n> + samplePolynomial(p.pgr, xPos, yPos, cropRectangle),\n> + samplePolynomial(p.pgb, xPos, yPos, cropRectangle),\n> + samplePolynomial(p.pb, xPos, yPos, cropRectangle)\n> + };\n> + }\n> +\n> + return components;\n> }\n> \n> std::vector\n> -LscPolynomialShadingDescriptor::samplePolynomial(const LscPolynomial &poly,\n> - Span xPositions,\n> - Span yPositions,\n> - const Rectangle &cropRectangle)\n> +LscPolynomialImpl::samplePolynomial(const LscPolynomial &poly,\n> + Span xPositions,\n> + Span yPositions,\n> + const Rectangle &cropRectangle)\n> {\n> double m = poly.getM();\n> double x0 = cropRectangle.x / m;\n> @@ -160,7 +205,7 @@ LscPolynomialShadingDescriptor::samplePolynomial(const LscPolynomial &poly,\n> * output: 0.0 0.2 0.5 0.8 1.0\n> */\n> std::vector\n> -LscPolynomialShadingDescriptor::sizesListToPositions(Span sizes)\n> +LscPolynomialImpl::sizesListToPositions(Span sizes)\n> {\n> const int half = sizes.size();\n> std::vector positions(half * 2 + 1);\n> @@ -176,74 +221,12 @@ LscPolynomialShadingDescriptor::sizesListToPositions(Span sizes)\n> return positions;\n> }\n> \n> -class LscPolynomialLoader\n> +class LscTableImpl : public LscImplementation\n> {\n> public:\n> - LscPolynomialLoader(const Size &sensorSize)\n> - : sensorSize_(sensorSize)\n> - {\n> - }\n> -\n> - int parseLscData(const ValueNode &sets,\n> - LensShadingCorrection::ShadingDescriptorMap &lscData);\n> + int parseLscData(const ValueNode &sets) override;\n> \n> -private:\n> - Size sensorSize_;\n> -};\n> -\n> -int LscPolynomialLoader::parseLscData(const ValueNode &sets,\n> - LensShadingCorrection::ShadingDescriptorMap &lscData)\n> -{\n> - for (const auto &set : sets.asList()) {\n> - std::optional pr, pgr, pgb, pb;\n> - uint32_t ct = set[\"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> - pr = set[\"r\"].get();\n> - pgr = set[\"gr\"].get();\n> - pgb = set[\"gb\"].get();\n> - pb = set[\"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> - pr->setReferenceImageSize(sensorSize_);\n> - pgr->setReferenceImageSize(sensorSize_);\n> - pgb->setReferenceImageSize(sensorSize_);\n> - pb->setReferenceImageSize(sensorSize_);\n> -\n> - lscData.emplace(\n> - ct, std::make_unique(\n> - *pr, *pgr, *pgb, *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> -class LscTableShadingDescriptor : public LensShadingCorrection::ShadingDescriptor\n> -{\n> -public:\n> - LscTableShadingDescriptor(LensShadingCorrection::Components components)\n> - : lscData_(std::move(components))\n> - {\n> - }\n> -\n> - LensShadingCorrection::Components\n> + LensShadingCorrection::ComponentsMap\n> sampleForCrop([[maybe_unused]] const Rectangle &cropRectangle,\n> [[maybe_unused]] Span xSizes,\n> [[maybe_unused]] Span ySizes) override\n> @@ -253,28 +236,19 @@ public:\n> return lscData_;\n> }\n> \n> -private:\n> - LensShadingCorrection::Components lscData_;\n> -};\n> -\n> -class LscTableLoader\n> -{\n> -public:\n> - int parseLscData(const ValueNode &sets,\n> - LensShadingCorrection::ShadingDescriptorMap &lscData);\n> -\n> private:\n> std::vector parseTable(const ValueNode &tuningData,\n> const char *prop);\n> +\n> + LensShadingCorrection::ComponentsMap lscData_;\n> };\n> \n> -int LscTableLoader::parseLscData(const ValueNode &sets,\n> - LensShadingCorrection::ShadingDescriptorMap &lscData)\n> +int LscTableImpl::parseLscData(const ValueNode &sets)\n> {\n> for (const auto &set : sets.asList()) {\n> uint32_t ct = set[\"ct\"].get(0);\n> \n> - if (lscData.count(ct)) {\n> + if (lscData_.count(ct)) {\n> LOG(RkISP1Lsc, Error)\n> << \"Multiple sets found for color temperature \"\n> << ct;\n> @@ -295,11 +269,10 @@ int LscTableLoader::parseLscData(const ValueNode &sets,\n> return -EINVAL;\n> }\n> \n> - lscData.emplace(\n> - ct, std::make_unique(std::move(components)));\n> + lscData_.emplace(ct, std::move(components));\n> }\n> \n> - if (lscData.empty()) {\n> + if (lscData_.empty()) {\n> LOG(RkISP1Lsc, Error) << \"Failed to load any sets\";\n> return -EINVAL;\n> }\n> @@ -307,7 +280,7 @@ int LscTableLoader::parseLscData(const ValueNode &sets,\n> return 0;\n> }\n> \n> -std::vector LscTableLoader::parseTable(const ValueNode &tuningData,\n> +std::vector LscTableImpl::parseTable(const ValueNode &tuningData,\n> const char *prop)\n> {\n> static constexpr unsigned int kLscNumSamples =\n> @@ -363,6 +336,18 @@ unsigned int quantize(unsigned int value, unsigned int step)\n> \n> } /* namespace */\n> \n> +/**\n> + * \\class LensShadingCorrection\n> + * \\brief RkISP1 Lens Shading Correction control\n> + *\n> + * Due to the optical characteristics of the lens, the light intensity received\n> + * by the sensor is not uniform.\n> + *\n> + * The Lens Shading Correction algorithm applies multipliers to all pixels to\n> + * compensate for the lens shading effect. The coefficients are specified in a\n> + * downscaled table in the tuning data.\n> + */\n> +\n> LensShadingCorrection::LensShadingCorrection()\n> : lastAppliedCt_(0), lastAppliedQuantizedCt_(0)\n> {\n> @@ -388,22 +373,21 @@ int LensShadingCorrection::init([[maybe_unused]] IPAContext &context,\n> return -EINVAL;\n> }\n> \n> - ShadingDescriptorMap lscData;\n> - int ret = 0;\n> + int ret;\n> \n> std::string type = tuningData[\"type\"].get(\"table\");\n> if (type == \"table\") {\n> LOG(RkISP1Lsc, Debug) << \"Loading tabular LSC data.\";\n> - auto loader = LscTableLoader();\n> - ret = loader.parseLscData(sets, lscData);\n> + algo_ = std::make_unique();\n> + ret = algo_->parseLscData(sets);\n> } else if (type == \"polynomial\") {\n> LOG(RkISP1Lsc, Debug) << \"Loading polynomial LSC data.\";\n> /*\n> * \\todo: Most likely the reference frame should be native_size.\n> * Let's wait how the internal discussions progress.\n> */\n> - auto loader = LscPolynomialLoader(context.sensorInfo.activeAreaSize);\n> - ret = loader.parseLscData(sets, lscData);\n> + algo_ = std::make_unique(context.sensorInfo.activeAreaSize);\n> + ret = algo_->parseLscData(sets);\n> } else {\n> LOG(RkISP1Lsc, Error) << \"Unsupported LSC data type '\"\n> << type << \"'\";\n> @@ -416,8 +400,6 @@ int LensShadingCorrection::init([[maybe_unused]] IPAContext &context,\n> context.ctrlMap[&controls::LensShadingCorrectionEnable] =\n> ControlInfo(false, true, true);\n> \n> - shadingDescriptors_ = std::move(lscData);\n> -\n> return 0;\n> }\n> \n> @@ -453,11 +435,8 @@ int LensShadingCorrection::configure(IPAContext &context,\n> }\n> \n> LOG(RkISP1Lsc, Debug) << \"Sample LSC data for \" << configInfo.analogCrop;\n> - std::map shadingData;\n> - for (const auto &[t, descriptor] : shadingDescriptors_)\n> - shadingData[t] = descriptor->sampleForCrop(configInfo.analogCrop,\n> - xSize_, ySize_);\n> -\n> + ComponentsMap shadingData = algo_->sampleForCrop(configInfo.analogCrop,\n> + xSize_, ySize_);\n> sets_.setData(std::move(shadingData));\n> \n> context.activeState.lsc.enabled = true;\n> diff --git a/src/ipa/rkisp1/algorithms/lsc.h b/src/ipa/rkisp1/algorithms/lsc.h\n> index 0a256e225327..3463531f2ab2 100644\n> --- a/src/ipa/rkisp1/algorithms/lsc.h\n> +++ b/src/ipa/rkisp1/algorithms/lsc.h\n> @@ -18,9 +18,20 @@ namespace libcamera {\n> \n> namespace ipa::rkisp1::algorithms {\n> \n> +class LscImplementation;\n> +\n> class LensShadingCorrection : public Algorithm\n> {\n> public:\n> + struct Components {\n> + std::vector r;\n> + std::vector gr;\n> + std::vector gb;\n> + std::vector b;\n> + };\n> +\n> + using ComponentsMap = std::map;\n> +\n> LensShadingCorrection();\n> ~LensShadingCorrection() = default;\n> \n> @@ -36,31 +47,10 @@ public:\n> IPAFrameContext &frameContext,\n> const rkisp1_stat_buffer *stats,\n> ControlList &metadata) override;\n> -\n> - struct Components {\n> - std::vector r;\n> - std::vector gr;\n> - std::vector gb;\n> - std::vector b;\n> - };\n> -\n> - class ShadingDescriptor\n> - {\n> - public:\n> - virtual ~ShadingDescriptor() = default;\n> - virtual Components sampleForCrop(const Rectangle &cropRectangle,\n> - Span xSizes,\n> - Span ySizes) = 0;\n> - };\n> -\n> - using ShadingDescriptorMap = std::map>;\n> -\n> private:\n> void setParameters(rkisp1_cif_isp_lsc_config &config);\n> void copyTable(rkisp1_cif_isp_lsc_config &config, const Components &set0);\n> \n> - ShadingDescriptorMap shadingDescriptors_;\n> - ipa::Interpolator sets_;\n> std::vector xSize_;\n> std::vector ySize_;\n> uint16_t xGrad_[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];\n> @@ -70,6 +60,23 @@ private:\n> \n> unsigned int lastAppliedCt_;\n> unsigned int lastAppliedQuantizedCt_;\n> +\n> + ipa::Interpolator sets_;\n> +\n> + std::unique_ptr algo_;\n> +};\n> +\n> +class LscImplementation\n> +{\n> +public:\n> + virtual ~LscImplementation() {}\n> +\n> + virtual int parseLscData(const ValueNode &sets) = 0;\n> +\n> + virtual LensShadingCorrection::ComponentsMap\n> + sampleForCrop(const Rectangle &cropRectangle,\n> + Span xSizes,\n> + Span ySizes) = 0;\n> };\n> \n> } /* namespace ipa::rkisp1::algorithms */\n> \n> -- \n> 2.54.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 C0AC7BF415\n\tfor ;\n\tThu, 9 Jul 2026 12:14:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D51C0660E4;\n\tThu, 9 Jul 2026 14:14:02 +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 CAE46660D9\n\tfor ;\n\tThu, 9 Jul 2026 14:14:01 +0200 (CEST)","from monstersaurus.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 26962289;\n\tThu, 9 Jul 2026 14:13:11 +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=\"iRVNxi0F\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1783599191;\n\tbh=q6GJ3Ik1yVEh5oIGD6PN1UMMaWxGvfjolflrLzlJXUU=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=iRVNxi0FQKBQo4fTXCAKzdOBT3NUkzbnD36vP4qx/EPZ6ExwEp3pdjeN43Z+Q4WFK\n\tRR1ykogSukYvXtrqgGL4Ihe34Y7p5/DvzjwXuKR+2MCv823s5FBbaRaJl0TD589kDD\n\toRU4VsTBG9l0X4nY53shHxqxO5Yf0o/REZO32fOs=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20260708-libipa-algorithms-v5-9-0759d0359f52@ideasonboard.com>","References":"<20260708-libipa-algorithms-v5-0-0759d0359f52@ideasonboard.com>\n\t<20260708-libipa-algorithms-v5-9-0759d0359f52@ideasonboard.com>","Subject":"Re: [PATCH v5 09/36] ipa: rkisp1: lsc: Re-architecture Lsc support","From":"Kieran Bingham ","Cc":"Jacopo Mondi ","To":"Jacopo Mondi ,\n\tStefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 09 Jul 2026 13:13:58 +0100","Message-ID":"<178359923880.2174993.9070488124922540636@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\" "}}]