[{"id":31324,"web_url":"https://patchwork.libcamera.org/comment/31324/","msgid":"<20240923204458.GE7165@pendragon.ideasonboard.com>","date":"2024-09-23T20:44:58","subject":"Re: [PATCH v3 5/9] ipa: rkisp1: Use interpolator in lsc","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Sep 20, 2024 at 03:39:20PM +0200, Stefan Klug wrote:\n> Now, that the generic interpolator is available, use it to do the\n> interpolation of the lens shading tables. This makes the algorithm\n> easier to read and remove some duplicate code.\n> \n> Signed-off-by: Stefan Klug \n> Reviewed-by: Kieran Bingham \n> Reviewed-by: Paul Elder \n> \n> ---\n> \n> Changes in v3:\n> - Collected tags\n> - Fixed a comment\n> ---\n> src/ipa/rkisp1/algorithms/lsc.cpp | 166 +++++++++---------------------\n> src/ipa/rkisp1/algorithms/lsc.h | 13 +--\n> 2 files changed, 57 insertions(+), 122 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp\n> index 5f3a0388075b..fe84062bbc70 100644\n> --- a/src/ipa/rkisp1/algorithms/lsc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/lsc.cpp\n> @@ -24,6 +24,36 @@\n> \n> namespace libcamera {\n> \n> +namespace ipa {\n> +\n> +constexpr int kColourTemperatureChangeThreshhold = 10;\n> +\n> +template\n> +void interpolateVector(const std::vector &a, const std::vector &b,\n> +\t\t std::vector &dest, double lambda)\n> +{\n> +\tassert(a.size() == b.size());\n\nUse ASSERT(), not assert(). Same for the whole patch series.\n\n> +\tdest.resize(a.size());\n> +\tfor (size_t i = 0; i < a.size(); i++) {\n> +\t\tdest[i] = a[i] * (1.0 - lambda) + b[i] * lambda;\n> +\t}\n\nNo need for curly braces.\n\n> +}\n> +\n> +template<>\n> +void Interpolator::\n> +\tinterpolate(const rkisp1::algorithms::LensShadingCorrection::Components &a,\n> +\t\t const rkisp1::algorithms::LensShadingCorrection::Components &b,\n> +\t\t rkisp1::algorithms::LensShadingCorrection::Components &dest,\n> +\t\t double lambda)\n> +{\n> +\tinterpolateVector(a.r, b.r, dest.r, lambda);\n> +\tinterpolateVector(a.gr, b.gr, dest.gr, lambda);\n> +\tinterpolateVector(a.gb, b.gb, dest.gb, lambda);\n> +\tinterpolateVector(a.b, b.b, dest.b, lambda);\n> +}\n\nSounds like there may be an opportunity to refactor the Components\nstructure, but that's probably for later.\n\n> +\n> +} /* namespace ipa */\n> +\n> namespace ipa::rkisp1::algorithms {\n> \n> /**\n> @@ -90,8 +120,9 @@ static std::vector parseTable(const YamlObject &tuningData,\n> }\n> \n> LensShadingCorrection::LensShadingCorrection()\n> -\t: lastCt_({ 0, 0 })\n> +\t: lastAppliedCt_(0), lastAppliedQuantizedCt_(0)\n> {\n> +\tsets_.setQuantization(kColourTemperatureChangeThreshhold);\n> }\n> \n> /**\n> @@ -115,17 +146,18 @@ int LensShadingCorrection::init([[maybe_unused]] IPAContext &context,\n> \t}\n> \n> \tconst auto &sets = yamlSets.asList();\n> +\tstd::map lscData;\n> \tfor (const auto &yamlSet : sets) {\n> \t\tuint32_t ct = yamlSet[\"ct\"].get(0);\n> \n> -\t\tif (sets_.count(ct)) {\n> +\t\tif (lscData.count(ct)) {\n> \t\t\tLOG(RkISP1Lsc, Error)\n> \t\t\t\t<< \"Multiple sets found for color temperature \"\n> \t\t\t\t<< ct;\n> \t\t\treturn -EINVAL;\n> \t\t}\n> \n> -\t\tComponents &set = sets_[ct];\n> +\t\tComponents &set = lscData[ct];\n> \n> \t\tset.ct = ct;\n> \t\tset.r = parseTable(yamlSet, \"r\");\n> @@ -142,11 +174,13 @@ int LensShadingCorrection::init([[maybe_unused]] IPAContext &context,\n> \t\t}\n> \t}\n> \n> -\tif (sets_.empty()) {\n> +\tif (lscData.empty()) {\n> \t\tLOG(RkISP1Lsc, Error) << \"Failed to load any sets\";\n> \t\treturn -EINVAL;\n> \t}\n> \n> +\tsets_.setData(std::move(lscData));\n> +\n> \treturn 0;\n> }\n> \n> @@ -202,134 +236,34 @@ void LensShadingCorrection::copyTable(rkisp1_cif_isp_lsc_config &config,\n> \tstd::copy(set.b.begin(), set.b.end(), &config.b_data_tbl[0][0]);\n> }\n> \n> -/*\n> - * Interpolate LSC parameters based on color temperature value.\n> - */\n> -void LensShadingCorrection::interpolateTable(rkisp1_cif_isp_lsc_config &config,\n> -\t\t\t\t\t const Components &set0,\n> -\t\t\t\t\t const Components &set1,\n> -\t\t\t\t\t const uint32_t ct)\n> -{\n> -\tdouble coeff0 = (set1.ct - ct) / static_cast(set1.ct - set0.ct);\n> -\tdouble coeff1 = (ct - set0.ct) / static_cast(set1.ct - set0.ct);\n> -\n> -\tfor (unsigned int i = 0; i < RKISP1_CIF_ISP_LSC_SAMPLES_MAX; ++i) {\n> -\t\tfor (unsigned int j = 0; j < RKISP1_CIF_ISP_LSC_SAMPLES_MAX; ++j) {\n> -\t\t\tunsigned int sample = i * RKISP1_CIF_ISP_LSC_SAMPLES_MAX + j;\n> -\n> -\t\t\tconfig.r_data_tbl[i][j] =\n> -\t\t\t\tset0.r[sample] * coeff0 +\n> -\t\t\t\tset1.r[sample] * coeff1;\n> -\n> -\t\t\tconfig.gr_data_tbl[i][j] =\n> -\t\t\t\tset0.gr[sample] * coeff0 +\n> -\t\t\t\tset1.gr[sample] * coeff1;\n> -\n> -\t\t\tconfig.gb_data_tbl[i][j] =\n> -\t\t\t\tset0.gb[sample] * coeff0 +\n> -\t\t\t\tset1.gb[sample] * coeff1;\n> -\n> -\t\t\tconfig.b_data_tbl[i][j] =\n> -\t\t\t\tset0.b[sample] * coeff0 +\n> -\t\t\t\tset1.b[sample] * coeff1;\n> -\t\t}\n> -\t}\n> -}\n> -\n> /**\n> * \\copydoc libcamera::ipa::Algorithm::prepare\n> */\n> void LensShadingCorrection::prepare(IPAContext &context,\n> -\t\t\t\t const uint32_t frame,\n> +\t\t\t\t [[maybe_unused]] const uint32_t frame,\n> \t\t\t\t [[maybe_unused]] IPAFrameContext &frameContext,\n> \t\t\t\t RkISP1Params *params)\n> {\n> -\t/*\n> -\t * If there is only one set, the configuration has already been done\n> -\t * for first frame.\n> -\t */\n> -\tif (sets_.size() == 1 && frame > 0)\n> -\t\treturn;\n> -\n> -\t/*\n> -\t * If there is only one set, pick it. We can ignore lastCt_, as it will\n> -\t * never be relevant.\n> -\t */\n> -\tif (sets_.size() == 1) {\n> -\t\tauto config = params->block();\n> -\t\tconfig.setEnabled(true);\n> -\n> -\t\tsetParameters(*config);\n> -\t\tcopyTable(*config, sets_.cbegin()->second);\n> -\t\treturn;\n> -\t}\n> -\n> \tuint32_t ct = context.activeState.awb.temperatureK;\n> -\tct = std::clamp(ct, sets_.cbegin()->first, sets_.crbegin()->first);\n> -\n> -\t/*\n> -\t * If the original is the same, then it means the same adjustment would\n> -\t * be made. If the adjusted is the same, then it means that it's the\n> -\t * same as what was actually applied. Thus in these cases we can skip\n> -\t * reprogramming the LSC.\n> -\t *\n> -\t * original == adjusted can only happen if an interpolation\n> -\t * happened, or if original has an exact entry in sets_. This means\n> -\t * that if original != adjusted, then original was adjusted to\n> -\t * the nearest available entry in sets_, resulting in adjusted.\n> -\t * Clearly, any ct value that is in between original and adjusted\n> -\t * will be adjusted to the same adjusted value, so we can skip\n> -\t * reprogramming the LSC table.\n> -\t *\n> -\t * We also skip updating the original value, as the last one had a\n> -\t * larger bound and thus a larger range of ct values that will be\n> -\t * adjusted to the same adjusted.\n> -\t */\n> -\tif ((lastCt_.original <= ct && ct <= lastCt_.adjusted) ||\n> -\t (lastCt_.adjusted <= ct && ct <= lastCt_.original))\n> +\tif (std::abs(static_cast(ct) - static_cast(lastAppliedCt_)) <\n> +\t kColourTemperatureChangeThreshhold)\n> +\t\treturn;\n> +\tunsigned int quantizedCt;\n> +\tconst Components &set = sets_.getInterpolated(ct, &quantizedCt);\n> +\tif (lastAppliedQuantizedCt_ == quantizedCt)\n> \t\treturn;\n> \n> \tauto config = params->block();\n> \tconfig.setEnabled(true);\n> \tsetParameters(*config);\n> +\tcopyTable(*config, set);\n> \n> -\t/*\n> -\t * The color temperature matches exactly one of the available LSC tables.\n> -\t */\n> -\tif (sets_.count(ct)) {\n> -\t\tcopyTable(*config, sets_[ct]);\n> -\t\tlastCt_ = { ct, ct };\n> -\t\treturn;\n> -\t}\n> +\tlastAppliedCt_ = ct;\n> +\tlastAppliedQuantizedCt_ = quantizedCt;\n> \n> -\t/* No shortcuts left; we need to round or interpolate */\n> -\tauto iter = sets_.upper_bound(ct);\n> -\tconst Components &set1 = iter->second;\n> -\tconst Components &set0 = (--iter)->second;\n> -\tuint32_t ct0 = set0.ct;\n> -\tuint32_t ct1 = set1.ct;\n> -\tuint32_t diff0 = ct - ct0;\n> -\tuint32_t diff1 = ct1 - ct;\n> -\tstatic constexpr double kThreshold = 0.1;\n> -\tfloat threshold = kThreshold * (ct1 - ct0);\n> -\n> -\tif (diff0 < threshold || diff1 < threshold) {\n> -\t\tconst Components &set = diff0 < diff1 ? set0 : set1;\n> -\t\tLOG(RkISP1Lsc, Debug) << \"using LSC table for \" << set.ct;\n> -\t\tcopyTable(*config, set);\n> -\t\tlastCt_ = { ct, set.ct };\n> -\t\treturn;\n> -\t}\n> -\n> -\t/*\n> -\t * ct is not within 10% of the difference between the neighbouring\n> -\t * color temperatures, so we need to interpolate.\n> -\t */\n> \tLOG(RkISP1Lsc, Debug)\n> -\t\t<< \"ct is \" << ct << \", interpolating between \"\n> -\t\t<< ct0 << \" and \" << ct1;\n> -\tinterpolateTable(*config, set0, set1, ct);\n> -\tlastCt_ = { ct, ct };\n> +\t\t<< \"ct is \" << ct << \", quantized to \"\n> +\t\t<< quantizedCt;\n> }\n> \n> REGISTER_IPA_ALGORITHM(LensShadingCorrection, \"LensShadingCorrection\")\n> diff --git a/src/ipa/rkisp1/algorithms/lsc.h b/src/ipa/rkisp1/algorithms/lsc.h\n> index a9c7a230e0fc..5a0824e36dd5 100644\n> --- a/src/ipa/rkisp1/algorithms/lsc.h\n> +++ b/src/ipa/rkisp1/algorithms/lsc.h\n> @@ -9,6 +9,8 @@\n> \n> #include \n> \n> +#include \"libipa/interpolator.h\"\n> +\n> #include \"algorithm.h\"\n> \n> namespace libcamera {\n> @@ -27,7 +29,6 @@ public:\n> \t\t IPAFrameContext &frameContext,\n> \t\t RkISP1Params *params) override;\n> \n> -private:\n> \tstruct Components {\n> \t\tuint32_t ct;\n> \t\tstd::vector r;\n> @@ -36,23 +37,23 @@ private:\n> \t\tstd::vector b;\n> \t};\n> \n> +private:\n> \tvoid setParameters(rkisp1_cif_isp_lsc_config &config);\n> \tvoid copyTable(rkisp1_cif_isp_lsc_config &config, const Components &set0);\n> \tvoid interpolateTable(rkisp1_cif_isp_lsc_config &config,\n> \t\t\t const Components &set0, const Components &set1,\n> \t\t\t const uint32_t ct);\n> \n> -\tstd::map sets_;\n> +\tipa::Interpolator sets_;\n> \tstd::vector xSize_;\n> \tstd::vector ySize_;\n> \tuint16_t xGrad_[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];\n> \tuint16_t yGrad_[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];\n> \tuint16_t xSizes_[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];\n> \tuint16_t ySizes_[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];\n> -\tstruct {\n> -\t\tuint32_t original;\n> -\t\tuint32_t adjusted;\n> -\t} lastCt_;\n> +\n> +\tunsigned int lastAppliedCt_;\n> +\tunsigned int lastAppliedQuantizedCt_;\n> };\n> \n> } /* namespace ipa::rkisp1::algorithms */","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 7F689C0F1B\n\tfor ;\n\tMon, 23 Sep 2024 20:45:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B20EE6350C;\n\tMon, 23 Sep 2024 22:45:31 +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 20E836037E\n\tfor ;\n\tMon, 23 Sep 2024 22:45:30 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 64DAE9CA;\n\tMon, 23 Sep 2024 22:44:03 +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=\"rDMzEYpI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727124243;\n\tbh=igeCFSGxPJ8N2Foz4fzDLAAnVzaz5mbenqSQKIUCtGI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rDMzEYpI/F9AXmTTdR8/sdXVHbnm93kgpFoAyODe+CQDLymA7vm/asEVuSHAsPWgd\n\tMdPgeDbp8wBmy+Q6293DPhRzgVuJ/XHKaTM7vOoHP6/vHde88TGJvNNsIWRqPNg4Pr\n\ttxQF9YWaDN5OH5yxYjPrU9xzebMzqXJou+HhMhKY=","Date":"Mon, 23 Sep 2024 23:44:58 +0300","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham ,\n\tPaul Elder ","Subject":"Re: [PATCH v3 5/9] ipa: rkisp1: Use interpolator in lsc","Message-ID":"<20240923204458.GE7165@pendragon.ideasonboard.com>","References":"<20240920133941.90629-1-stefan.klug@ideasonboard.com>\n\t<20240920133941.90629-6-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240920133941.90629-6-stefan.klug@ideasonboard.com>","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\" "}}]