[{"id":31321,"web_url":"https://patchwork.libcamera.org/comment/31321/","msgid":"<20240923202113.GB7165@pendragon.ideasonboard.com>","date":"2024-09-23T20:21:13","subject":"Re: [PATCH v3 6/9] ipa: rkisp1: Move loader functions into helper\n\tclass","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:21PM +0200, Stefan Klug wrote:\n> In preparation for supporting polynomial LSC data, move the current\n> loader into its own helper class.\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> - Renamed LscClassicLoader to LscTableLoader\n> - Collected tags\n> - Removed type detection from this patch to the one adding the\n> polynomial loader\n> ---\n> src/ipa/rkisp1/algorithms/lsc.cpp | 120 +++++++++++++++++-------------\n> 1 file changed, 69 insertions(+), 51 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp\n> index fe84062bbc70..44c97f0e1a10 100644\n> --- a/src/ipa/rkisp1/algorithms/lsc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/lsc.cpp\n> @@ -70,6 +70,70 @@ namespace ipa::rkisp1::algorithms {\n> \n> LOG_DEFINE_CATEGORY(RkISP1Lsc)\n> \n> +class LscTableLoader\n> +{\n> +public:\n> +\tint parseLscData(const YamlObject &yamlSets,\n> +\t\t\t std::map &lscData)\n> +\t{\n\nAs mentioned in the review of other patches in the series, please don't\nmake those functions inline. In hinders readability.\n\n> +\t\tconst auto &sets = yamlSets.asList();\n> +\n> +\t\tfor (const auto &yamlSet : sets) {\n> +\t\t\tuint32_t ct = yamlSet[\"ct\"].get(0);\n> +\n> +\t\t\tif (lscData.count(ct)) {\n> +\t\t\t\tLOG(RkISP1Lsc, Error)\n> +\t\t\t\t\t<< \"Multiple sets found for color temperature \"\n> +\t\t\t\t\t<< ct;\n> +\t\t\t\treturn -EINVAL;\n> +\t\t\t}\n> +\n> +\t\t\tLensShadingCorrection::Components &set = lscData[ct];\n> +\n> +\t\t\tset.ct = ct;\n> +\t\t\tset.r = parseTable(yamlSet, \"r\");\n> +\t\t\tset.gr = parseTable(yamlSet, \"gr\");\n> +\t\t\tset.gb = parseTable(yamlSet, \"gb\");\n> +\t\t\tset.b = parseTable(yamlSet, \"b\");\n> +\n> +\t\t\tif (set.r.empty() || set.gr.empty() ||\n> +\t\t\t set.gb.empty() || set.b.empty()) {\n> +\t\t\t\tLOG(RkISP1Lsc, Error)\n> +\t\t\t\t\t<< \"Set for color temperature \" << ct\n> +\t\t\t\t\t<< \" is missing tables\";\n> +\t\t\t\treturn -EINVAL;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tif (lscData.empty()) {\n> +\t\t\tLOG(RkISP1Lsc, Error) << \"Failed to load any sets\";\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\treturn 0;\n> +\t}\n> +\n> +private:\n> +\tstd::vector parseTable(const YamlObject &tuningData,\n> +\t\t\t\t\t const char *prop)\n> +\t{\n> +\t\tstatic constexpr unsigned int kLscNumSamples =\n> +\t\t\tRKISP1_CIF_ISP_LSC_SAMPLES_MAX * RKISP1_CIF_ISP_LSC_SAMPLES_MAX;\n> +\n> +\t\tstd::vector table =\n> +\t\t\ttuningData[prop].getList().value_or(std::vector{});\n> +\t\tif (table.size() != kLscNumSamples) {\n> +\t\t\tLOG(RkISP1Lsc, Error)\n> +\t\t\t\t<< \"Invalid '\" << prop << \"' values: expected \"\n> +\t\t\t\t<< kLscNumSamples\n> +\t\t\t\t<< \" elements, got \" << table.size();\n> +\t\t\treturn {};\n> +\t\t}\n> +\n> +\t\treturn table;\n> +\t}\n> +};\n> +\n> static std::vector parseSizes(const YamlObject &tuningData,\n> \t\t\t\t const char *prop)\n> {\n> @@ -100,25 +164,6 @@ static std::vector parseSizes(const YamlObject &tuningData,\n> \treturn sizes;\n> }\n> \n> -static std::vector parseTable(const YamlObject &tuningData,\n> -\t\t\t\t\tconst char *prop)\n> -{\n> -\tstatic constexpr unsigned int kLscNumSamples =\n> -\t\tRKISP1_CIF_ISP_LSC_SAMPLES_MAX * RKISP1_CIF_ISP_LSC_SAMPLES_MAX;\n> -\n> -\tstd::vector table =\n> -\t\ttuningData[prop].getList().value_or(std::vector{});\n> -\tif (table.size() != kLscNumSamples) {\n> -\t\tLOG(RkISP1Lsc, Error)\n> -\t\t\t<< \"Invalid '\" << prop << \"' values: expected \"\n> -\t\t\t<< kLscNumSamples\n> -\t\t\t<< \" elements, got \" << table.size();\n> -\t\treturn {};\n> -\t}\n> -\n> -\treturn table;\n> -}\n> -\n> LensShadingCorrection::LensShadingCorrection()\n> \t: lastAppliedCt_(0), lastAppliedQuantizedCt_(0)\n> {\n> @@ -145,39 +190,12 @@ int LensShadingCorrection::init([[maybe_unused]] IPAContext &context,\n> \t\treturn -EINVAL;\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 (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 = lscData[ct];\n> -\n> -\t\tset.ct = ct;\n> -\t\tset.r = parseTable(yamlSet, \"r\");\n> -\t\tset.gr = parseTable(yamlSet, \"gr\");\n> -\t\tset.gb = parseTable(yamlSet, \"gb\");\n> -\t\tset.b = parseTable(yamlSet, \"b\");\n> -\n> -\t\tif (set.r.empty() || set.gr.empty() ||\n> -\t\t set.gb.empty() || set.b.empty()) {\n> -\t\t\tLOG(RkISP1Lsc, Error)\n> -\t\t\t\t<< \"Set for color temperature \" << ct\n> -\t\t\t\t<< \" is missing tables\";\n> -\t\t\treturn -EINVAL;\n> -\t\t}\n> -\t}\n> -\n> -\tif (lscData.empty()) {\n> -\t\tLOG(RkISP1Lsc, Error) << \"Failed to load any sets\";\n> -\t\treturn -EINVAL;\n> -\t}\n> +\tint res = 0;\n> +\tauto loader = LscTableLoader();\n> +\tres = loader.parseLscData(yamlSets, lscData);\n> +\tif (res)\n> +\t\treturn res;\n> \n> \tsets_.setData(std::move(lscData));\n> \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 ED766C3257\n\tfor ;\n\tMon, 23 Sep 2024 20:21:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B7AD16350B;\n\tMon, 23 Sep 2024 22:21:47 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 521386037E\n\tfor ;\n\tMon, 23 Sep 2024 22:21:46 +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 D305D63D;\n\tMon, 23 Sep 2024 22:20:19 +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=\"sx/qsNNS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727122820;\n\tbh=jkH1lfK/x0UrFxxznfftt9q4JGiVzifTEBPHiRdbGjM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sx/qsNNSuYyLnnJVG8KhFfCVTTwC8bk/CiK1LtVvN1oIoIZjP6nNdm0Z+iqHvFEu+\n\tJhyTZGEXR8/oxXJhBRV3wb5cuuO1+zswi5u5SN3gxlZ+FQdKCli2Ju+3cIsnR4HvoJ\n\t5xStUh7ZFeq5hitbdqeMP52uqqOjKBZ/7Eqo6RWs=","Date":"Mon, 23 Sep 2024 23:21:13 +0300","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham ,\n\tPaul Elder ","Subject":"Re: [PATCH v3 6/9] ipa: rkisp1: Move loader functions into helper\n\tclass","Message-ID":"<20240923202113.GB7165@pendragon.ideasonboard.com>","References":"<20240920133941.90629-1-stefan.klug@ideasonboard.com>\n\t<20240920133941.90629-7-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240920133941.90629-7-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\" "}}]