[{"id":31660,"web_url":"https://patchwork.libcamera.org/comment/31660/","msgid":"<172848863960.532453.2584038725629534645@ping.linuxembedded.co.uk>","date":"2024-10-09T15:43:59","subject":"Re: [PATCH v2 09/10] ipa: mali-c55: Add Lens Shading Correction\n\talgorithm","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Daniel Scally (2024-07-09 15:49:49)\n> Add a lens shading correction algorithm to the mali-c55 IPA. This\n> algorithm parses tables from Yaml in a easy to follow format before\n> munging them into Arm's interleaved mesh to be copied to the ISP.\n> A colour temperature estimate from the AGC statistics is used to\n> select the appropriate table to apply; this can be some interpolation\n> of two tables, in which case the colour temperature estimate is also\n> used to derive the coefficient that does the blending.\n> \n> Acked-by: Nayden Kanchev <nayden.kanchev@arm.com>\n> Co-developed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>\n> ---\n> Changes in v2:\n> \n>         - Use the union rather than reinterpret_cast<>() to abstract the block\n> \n>  src/ipa/mali-c55/algorithms/lsc.cpp     | 216 ++++++++++++++++++++++++\n>  src/ipa/mali-c55/algorithms/lsc.h       |  45 +++++\n>  src/ipa/mali-c55/algorithms/meson.build |   1 +\n>  3 files changed, 262 insertions(+)\n>  create mode 100644 src/ipa/mali-c55/algorithms/lsc.cpp\n>  create mode 100644 src/ipa/mali-c55/algorithms/lsc.h\n> \n> diff --git a/src/ipa/mali-c55/algorithms/lsc.cpp b/src/ipa/mali-c55/algorithms/lsc.cpp\n> new file mode 100644\n> index 00000000..8d574779\n> --- /dev/null\n> +++ b/src/ipa/mali-c55/algorithms/lsc.cpp\n> @@ -0,0 +1,216 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Ideas On Board Oy\n> + *\n> + * lsc.cpp - Mali-C55 Lens shading correction algorithm\n> + */\n> +\n> +#include \"lsc.h\"\n> +\n> +#include \"libcamera/internal/yaml_parser.h\"\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::mali_c55::algorithms {\n> +\n> +LOG_DEFINE_CATEGORY(MaliC55Lsc)\n> +\n> +Lsc::Lsc()\n> +{\n> +}\n\nI don't think we need to declare emtpy/default constructors?\n\n> +\n> +int Lsc::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)\n> +{\n> +       if (!tuningData.contains(\"meshScale\")) {\n> +               LOG(MaliC55Lsc, Error) << \"meshScale missing from tuningData\";\n> +               return -EINVAL;\n> +       }\n> +\n> +       meshScale_ = tuningData[\"meshScale\"].get<uint32_t>(0);\n> +\n> +       const YamlObject &yamlSets = tuningData[\"sets\"];\n> +       if (!yamlSets.isList()) {\n> +               LOG(MaliC55Lsc, Error) << \"LSC tables missing or invalid\";\n> +               return -EINVAL;\n> +       }\n> +\n> +       size_t tableSize = 0;\n> +       const auto &sets = yamlSets.asList();\n> +       for (const auto &yamlSet : sets) {\n> +               uint32_t ct = yamlSet[\"ct\"].get<uint32_t>(0);\n> +\n> +               if (!ct) {\n> +                       LOG(MaliC55Lsc, Error) << \"Invalid colour temperature\";\n> +                       return -EINVAL;\n> +               }\n> +\n> +               if (std::count(colourTemperatures_.begin(),\n> +                              colourTemperatures_.end(), ct)) {\n> +                       LOG(MaliC55Lsc, Error)\n> +                               << \"Multiple sets found for colour temperature\";\n> +                       return -EINVAL;\n> +               }\n> +\n> +               std::vector<uint8_t> rTable =\n> +                       yamlSet[\"r\"].getList<uint8_t>().value_or(std::vector<uint8_t>{});\n> +               std::vector<uint8_t> gTable =\n> +                       yamlSet[\"g\"].getList<uint8_t>().value_or(std::vector<uint8_t>{});\n> +               std::vector<uint8_t> bTable =\n> +                       yamlSet[\"b\"].getList<uint8_t>().value_or(std::vector<uint8_t>{});\n> +\n> +               /*\n> +                * Some validation to do; only 16x16 and 32x32 tables of\n> +                * coefficients are acceptable, and all tables across all of the\n> +                * sets must be the same size. The first time we encounter a\n> +                * table we check that it is an acceptable size and if so make\n> +                * sure all other tables are of equal size.\n> +                */\n> +               if (!tableSize) {\n> +                       if (rTable.size() != 256 && rTable.size() != 1024) {\n> +                               LOG(MaliC55Lsc, Error)\n> +                                       << \"Invalid table size for colour temperature \" << ct;\n> +                               return -EINVAL;\n> +                       }\n> +                       tableSize = rTable.size();\n> +               }\n> +\n> +               if (rTable.size() != tableSize ||\n> +                   gTable.size() != tableSize ||\n> +                   bTable.size() != tableSize) {\n> +                       LOG(MaliC55Lsc, Error)\n> +                               << \"Invalid or mismatched table size for colour temperature \" << ct;\n> +                       return -EINVAL;\n> +               }\n> +\n> +               if (colourTemperatures_.size() >= 3) {\n> +                       LOG(MaliC55Lsc, Error)\n> +                               << \"A maximum of 3 colour temperatures are supported\";\n> +                       return -EINVAL;\n> +               }\n> +\n> +               for (unsigned int i = 0; i < tableSize; i++) {\n> +                       mesh_[kRedOffset + i] |=\n> +                               (rTable[i] << (colourTemperatures_.size() * 8));\n> +                       mesh_[kGreenOffset + i] |=\n> +                               (gTable[i] << (colourTemperatures_.size() * 8));\n> +                       mesh_[kBlueOffset + i] |=\n> +                               (bTable[i] << (colourTemperatures_.size() * 8));\n> +               }\n> +\n> +               colourTemperatures_.push_back(ct);\n> +       }\n> +\n\nA comment here about meshSize_ would be helpful. I have no idea what\nthis next block of code is doing or why.\n\n\nSeems that's all I've got in here for now.\n\nReviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\n> +       if (tableSize == 256)\n> +               meshSize_ = 15;\n> +       else\n> +               meshSize_ = 31;\n> +\n> +       return 0;\n> +}\n> +\n> +size_t Lsc::fillConfigParamsBlock(mali_c55_params_block block) const\n> +{\n> +       block.header->type = MALI_C55_PARAM_MESH_SHADING_CONFIG;\n> +       block.header->enabled = true;\n> +       block.header->size = sizeof(struct mali_c55_params_mesh_shading_config);\n> +\n> +       block.shading_config->mesh_show = false;\n> +       block.shading_config->mesh_scale = meshScale_;\n> +       block.shading_config->mesh_page_r = 0;\n> +       block.shading_config->mesh_page_g = 1;\n> +       block.shading_config->mesh_page_b = 2;\n> +       block.shading_config->mesh_width = meshSize_;\n> +       block.shading_config->mesh_height = meshSize_;\n> +\n> +       std::copy(mesh_.begin(), mesh_.end(), block.shading_config->mesh);\n> +\n> +       return block.header->size;\n> +}\n> +\n> +size_t Lsc::fillSelectionParamsBlock(mali_c55_params_block block, uint8_t bank,\n> +                                    uint8_t alpha) const\n> +{\n> +       block.header->type = MALI_C55_PARAM_MESH_SHADING_SELECTION;\n> +       block.header->enabled = true;\n> +       block.header->size = sizeof(struct mali_c55_params_mesh_shading_selection);\n> +\n> +       block.shading_selection->mesh_alpha_bank_r = bank;\n> +       block.shading_selection->mesh_alpha_bank_g = bank;\n> +       block.shading_selection->mesh_alpha_bank_b = bank;\n> +       block.shading_selection->mesh_alpha_r = alpha;\n> +       block.shading_selection->mesh_alpha_g = alpha;\n> +       block.shading_selection->mesh_alpha_b = alpha;\n> +       block.shading_selection->mesh_strength = 0x1000; /* Otherwise known as 1.0 */\n> +\n> +       return block.header->size;\n> +}\n> +\n> +std::tuple<uint8_t, uint8_t> Lsc::findBankAndAlpha(uint32_t ct) const\n> +{\n> +       unsigned int i;\n> +\n> +       ct = std::clamp<uint32_t>(ct, colourTemperatures_.front(),\n> +                                 colourTemperatures_.back());\n> +\n> +       for (i = 0; i < colourTemperatures_.size() - 1; i++) {\n> +               if (ct >= colourTemperatures_[i] &&\n> +                   ct <= colourTemperatures_[i + 1])\n> +                       break;\n> +       }\n> +\n> +       /*\n> +        * With the clamping, we're guaranteed an index into colourTemperatures_\n> +        * that's <= colourTemperatures_.size() - 1.\n> +        */\n> +       uint8_t alpha = (255 * (ct - colourTemperatures_[i])) /\n> +                       (colourTemperatures_[i + 1] - colourTemperatures_[i]);\n> +\n> +       return { i, alpha };\n> +}\n> +\n> +void Lsc::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> +                 [[maybe_unused]] IPAFrameContext &frameContext,\n> +                 mali_c55_params_buffer *params)\n> +{\n> +       /*\n> +        * For each frame we assess the colour temperature of the **last** frame\n> +        * and then select an appropriately blended table of coefficients based\n> +        * on that ct. As a bit of a shortcut, if we've only a single table the\n> +        * handling is somewhat simpler; if it's the first frame we just select\n> +        * that table and if we're past the first frame then we can just do\n> +        * nothing - the config will never change.\n> +        */\n> +       uint32_t temperatureK = context.activeState.agc.temperatureK;\n> +       uint8_t bank, alpha;\n> +\n> +       if (colourTemperatures_.size() == 1) {\n> +               if (frame > 0)\n> +                       return;\n> +\n> +               bank = 0;\n> +               alpha = 0;\n> +       } else {\n> +               std::tie(bank, alpha) = findBankAndAlpha(temperatureK);\n> +       }\n> +\n> +       mali_c55_params_block block;\n> +       block.data = &params->data[params->total_size];\n> +\n> +       params->total_size += fillSelectionParamsBlock(block, bank, alpha);\n> +\n> +       if (frame > 0)\n> +               return;\n> +\n> +       /*\n> +        * If this is the first frame, we need to load the parsed coefficient\n> +        * tables from tuning data to the ISP.\n> +        */\n> +       block.data = &params->data[params->total_size];\n> +       params->total_size += fillConfigParamsBlock(block);\n> +}\n> +\n> +REGISTER_IPA_ALGORITHM(Lsc, \"Lsc\")\n> +\n> +} /* namespace ipa::mali_c55::algorithms */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/mali-c55/algorithms/lsc.h b/src/ipa/mali-c55/algorithms/lsc.h\n> new file mode 100644\n> index 00000000..ed356f4a\n> --- /dev/null\n> +++ b/src/ipa/mali-c55/algorithms/lsc.h\n> @@ -0,0 +1,45 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Ideas On Board Oy\n> + *\n> + * lsc.h - Mali-C55 Lens shading correction algorithm\n> + */\n> +\n> +#include <map>\n> +#include <tuple>\n> +\n> +#include \"algorithm.h\"\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::mali_c55::algorithms {\n> +\n> +class Lsc : public Algorithm\n> +{\n> +public:\n> +       Lsc();\n> +       ~Lsc() = default;\n> +\n> +       int init(IPAContext &context, const YamlObject &tuningData) override;\n> +       void prepare(IPAContext &context, const uint32_t frame,\n> +                    IPAFrameContext &frameContext,\n> +                    mali_c55_params_buffer *params) override;\n> +private:\n> +       static constexpr unsigned int kRedOffset = 0;\n> +       static constexpr unsigned int kGreenOffset = 1024;\n> +       static constexpr unsigned int kBlueOffset = 2048;\n> +\n> +       size_t fillConfigParamsBlock(mali_c55_params_block block) const;\n> +       size_t fillSelectionParamsBlock(mali_c55_params_block block,\n> +                                       uint8_t bank, uint8_t alpha) const;\n> +       std::tuple<uint8_t, uint8_t> findBankAndAlpha(uint32_t ct) const;\n> +\n> +       std::vector<uint32_t> mesh_ = std::vector<uint32_t>(3072);\n> +       std::vector<uint32_t> colourTemperatures_;\n> +       uint32_t meshScale_;\n> +       uint32_t meshSize_;\n> +};\n> +\n> +} /* namespace ipa::mali_c55::algorithms */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/mali-c55/algorithms/meson.build b/src/ipa/mali-c55/algorithms/meson.build\n> index f11791aa..1665da07 100644\n> --- a/src/ipa/mali-c55/algorithms/meson.build\n> +++ b/src/ipa/mali-c55/algorithms/meson.build\n> @@ -4,4 +4,5 @@ mali_c55_ipa_algorithms = files([\n>      'agc.cpp',\n>      'awb.cpp',\n>      'blc.cpp',\n> +    'lsc.cpp',\n>  ])\n> -- \n> 2.34.1\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 A33C2C32DE\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed,  9 Oct 2024 15:44:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4AFCC65369;\n\tWed,  9 Oct 2024 17:44: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 7272E65369\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  9 Oct 2024 17:44:02 +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 B6D7F670;\n\tWed,  9 Oct 2024 17:42: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=\"n/hN7g5j\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728488544;\n\tbh=lcyiak+HMaaFIo5uRH3FR1sO750cH1zhJVdpIOztoCs=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=n/hN7g5jT3YA0rGZdbdQpGQGM/UDOksz78m1cWslz0pq4H+aYueITscty40na3jD/\n\t5eROqQvud2DrtENQNZotbe0JbJMnkb9Wg4oXVqTGQaC69lUE6MRN9SETlE/Ykbabdl\n\tKT8wFQtaLr0sjLf2KqRe0BAz/Z70mYxqNPpk6zGo=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240709144950.3277837-10-dan.scally@ideasonboard.com>","References":"<20240709144950.3277837-1-dan.scally@ideasonboard.com>\n\t<20240709144950.3277837-10-dan.scally@ideasonboard.com>","Subject":"Re: [PATCH v2 09/10] ipa: mali-c55: Add Lens Shading Correction\n\talgorithm","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"Daniel Scally <dan.scally@ideasonboard.com>,\n\tNayden Kanchev <nayden.kanchev@arm.com>,\n\tJacopo Mondi <jacopo.mondi@ideasonboard.com>","To":"Daniel Scally <dan.scally@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 09 Oct 2024 16:43:59 +0100","Message-ID":"<172848863960.532453.2584038725629534645@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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]