[{"id":31322,"web_url":"https://patchwork.libcamera.org/comment/31322/","msgid":"<20240923202951.GC7165@pendragon.ideasonboard.com>","date":"2024-09-23T20:29:51","subject":"Re: [PATCH v3 1/9] ipa: libipa: Add generic Interpolator class","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:16PM +0200, Stefan Klug wrote:\n> The MatrixInterpolator is great for interpolation of matrices for\n> different color temperatures. It has however one limitation - it can\n> only handle matrices. For LSC it would be great to interpolate the LSC\n> tables (or even polynomials) using the same approach. Add a generic\n> Interpolator class based on the existing MatrixInterpolator. This class\n> can be adapted to any other type using partial template specialization.\n> \n> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>\n> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>\n> \n> ---\n> \n> Changes in v3:\n> - Fixed typos from review\n> - Collected tags\n> ---\n>  src/ipa/libipa/interpolator.cpp | 157 ++++++++++++++++++++++++++++++++\n>  src/ipa/libipa/interpolator.h   | 131 ++++++++++++++++++++++++++\n>  src/ipa/libipa/meson.build      |   2 +\n>  3 files changed, 290 insertions(+)\n>  create mode 100644 src/ipa/libipa/interpolator.cpp\n>  create mode 100644 src/ipa/libipa/interpolator.h\n> \n> diff --git a/src/ipa/libipa/interpolator.cpp b/src/ipa/libipa/interpolator.cpp\n> new file mode 100644\n> index 000000000000..73e8d3b7de14\n> --- /dev/null\n> +++ b/src/ipa/libipa/interpolator.cpp\n> @@ -0,0 +1,157 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>\n> + *\n> + * Helper class for interpolating objects\n> + */\n> +#include \"interpolator.h\"\n> +\n> +#include <algorithm>\n> +#include <string>\n> +\n> +#include <libcamera/base/log.h>\n> +\n> +#include \"libcamera/internal/yaml_parser.h\"\n> +\n> +#include \"interpolator.h\"\n> +\n> +/**\n> + * \\file interpolator.h\n> + * \\brief Helper class for linear interpolating a set of objects\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(Interpolator)\n> +\n> +namespace ipa {\n> +\n> +/**\n> + * \\class Interpolator\n> + * \\brief Class for storing, retrieving, and interpolating objects\n> + * \\tparam T Type of objects stored in the interpolator\n> + *\n> + * The main use case is to pass a map from color temperatures to corresponding\n> + * objects (eg. matrices for color correction), and then requesting a\n> + * interpolated object for a specific color temperature. This class will\n> + * abstract away the interpolation portion.\n> + */\n> +\n> +/**\n> + * \\fn Interpolator::Interpolator()\n> + * \\brief Construct an empty interpolator\n> + */\n> +\n> +/**\n> + * \\fn Interpolator::Interpolator(const std::map<unsigned int, T> &data)\n> + * \\brief Construct an interpolator from a map of objects\n> + * \\param data Map from which to construct the interpolator\n> + */\n> +\n> +/**\n> + * \\fn Interpolator::Interpolator(std::map<unsigned int, T> &&data)\n> + * \\brief Construct an interpolator from a map of objects\n> + * \\param data Map from which to construct the interpolator\n> + */\n> +\n> +/**\n> + * \\fn int Interpolator<T>::readYaml(const libcamera::YamlObject &yaml,\n> +\t\t                     const std::string &key_name,\n> +\t\t                     const std::string &value_name)\n> + * \\brief Initialize an Interpolator instance from yaml\n> + * \\tparam T Type of data stored in the interpolator\n> + * \\param[in] yaml The yaml object that contains the map of unsigned integers to\n> + * objects\n> + * \\param[in] key_name The name of the key in the yaml object\n> + * \\param[in] value_name The name of the value in the yaml object\n> + *\n> + * The yaml object is expected to be a list of maps. Each map has two or more\n> + * pairs: one of \\a key_name to the key value (usually color temperature), and\n> + * one or more of \\a value_name to the object. This is a bit difficult to\n> + * explain, so here is an example (in python, as it is easier to parse than\n> + * yaml):\n> + *       [\n> + *               {\n> + *                   'ct': 2860,\n> + *                   'ccm': [ 2.12089, -0.52461, -0.59629,\n> + *                           -0.85342,  2.80445, -0.95103,\n> + *                           -0.26897, -1.14788,  2.41685 ],\n> + *                   'offsets': [ 0, 0, 0 ]\n> + *               },\n> + *\n> + *               {\n> + *                   'ct': 2960,\n> + *                   'ccm': [ 2.26962, -0.54174, -0.72789,\n> + *                           -0.77008,  2.60271, -0.83262,\n> + *                           -0.26036, -1.51254,  2.77289 ],\n> + *                   'offsets': [ 0, 0, 0 ]\n> + *               },\n> + *\n> + *               {\n> + *                   'ct': 3603,\n> + *                   'ccm': [ 2.18644, -0.66148, -0.52496,\n> + *                           -0.77828,  2.69474, -0.91645,\n> + *                           -0.25239, -0.83059,  2.08298 ],\n> + *                   'offsets': [ 0, 0, 0 ]\n> + *               },\n> + *       ]\n> + *\n> + * In this case, \\a key_name would be 'ct', and \\a value_name can be either\n> + * 'ccm' or 'offsets'. This way multiple interpolators can be defined in\n> + * one set of color temperature ranges in the tuning file, and they can be\n> + * retrieved separately with the \\a value_name parameter.\n> + *\n> + * \\return Zero on success, negative error code otherwise\n> + */\n> +\n> +/**\n> + * \\fn void Interpolator<T>::setQuantization(const unsigned int q)\n> + * \\brief Set the quantization value\n> + * \\param[in] q The quantization value\n> + *\n> + * Sets the quantization value. When this is set, 'key' gets quantized to this\n> + * size, before doing the interpolation. This can help in reducing the number of\n> + * updates pushed to the hardware.\n> + *\n> + * Note that normally a threshold needs to be combined with quantization.\n> + * Otherwise a value that swings around the edge of the quantization step will\n> + * lead to constant updates.\n> + */\n> +\n> +/**\n> + * \\fn void Interpolator<T>::setData(std::map<unsigned int, T> &&data)\n> + * \\brief Set the internal map\n> + *\n> + * Overwrites the internal map using move semantics.\n> + */\n> +\n> +/**\n> + * \\fn const T& Interpolator<T>::getInterpolated()\n> + * \\brief Retrieve an interpolated value for the given key\n> + * \\param[in] key The unsigned integer key of the object to retrieve\n> + * \\param[out] quantizedKey If provided, the key value after quantization\n> + * \\return The object corresponding to the key. The object is cached internally,\n> + * so on successive calls with the same key (after quantization) interpolation\n> + * is not recalculated.\n> + */\n> +\n> +/**\n> + * \\fn void Interpolator<T>::interpolate(const T &a, const T &b, T &dest, double\n> + * lambda)\n> + * \\brief Interpolate between two instances of T\n> + * \\param a The first value to interpolate\n> + * \\param b The second value to interpolate\n> + * \\param dest The destination for the interpolated value\n> + * \\param lambda The interpolation factor (0..1)\n> + *\n> + * Interpolates between \\a a and \\a b according to \\a lambda. It calculates\n> + * dest = a * (1.0 - lambda) + b * lambda;\n> + *\n> + * If T supports multiplication with double and addition, this function can be\n> + * used as is. For other types this function can be overwritten using partial\n> + * template specialization.\n> + */\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/interpolator.h b/src/ipa/libipa/interpolator.h\n> new file mode 100644\n> index 000000000000..fffce21465fe\n> --- /dev/null\n> +++ b/src/ipa/libipa/interpolator.h\n> @@ -0,0 +1,131 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>\n> + *\n> + * Helper class for interpolating maps of objects\n> + */\n> +\n> +#pragma once\n> +\n> +#include <algorithm>\n> +#include <cmath>\n> +#include <map>\n> +#include <string>\n> +#include <tuple>\n> +\n> +#include <libcamera/base/log.h>\n> +\n> +#include \"libcamera/internal/yaml_parser.h\"\n> +\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(Interpolator)\n> +\n> +namespace ipa {\n> +\n> +template<typename T>\n> +class Interpolator\n> +{\n> +public:\n> +\tInterpolator() = default;\n> +\tInterpolator(const std::map<unsigned int, T> &data)\n> +\t\t: data_(data)\n> +\t{\n> +\t}\n> +\tInterpolator(std::map<unsigned int, T> &&data)\n> +\t\t: data_(std::move(data))\n> +\t{\n> +\t}\n> +\n> +\t~Interpolator() = default;\n> +\n> +\tint readYaml(const libcamera::YamlObject &yaml,\n> +\t\t     const std::string &key_name,\n> +\t\t     const std::string &value_name)\n\nAny reason why this can't use YamlObject::Getter<Interpolator<T>> ?\nThe class API should not depend on the YAML reader.\n\n> +\t{\n> +\t\tdata_.clear();\n> +\t\tlastInterpolatedKey_.reset();\n> +\n> +\t\tif (!yaml.isList()) {\n> +\t\t\tLOG(Interpolator, Error) << \"yaml object must be a list\";\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\tfor (const auto &value : yaml.asList()) {\n> +\t\t\tunsigned int ct = std::stoul(value[key_name].get<std::string>(\"\"));\n> +\t\t\tstd::optional<T> data =\n> +\t\t\t\tvalue[value_name].get<T>();\n> +\t\t\tif (!data) {\n> +\t\t\t\treturn -EINVAL;\n> +\t\t\t}\n> +\n> +\t\t\tdata_[ct] = *data;\n> +\t\t}\n> +\n> +\t\tif (data_.size() < 1) {\n> +\t\t\tLOG(Interpolator, Error) << \"Need at least one element\";\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tvoid setQuantization(const unsigned int q)\n> +\t{\n> +\t\tquantization_ = q;\n> +\t}\n> +\n> +\tvoid setData(std::map<unsigned int, T> &&data)\n> +\t{\n> +\t\tdata_ = std::move(data);\n> +\t\tlastInterpolatedKey_.reset();\n> +\t}\n> +\n> +\tconst T &getInterpolated(unsigned int key, unsigned int *quantizedKey = nullptr)\n> +\t{\n> +\t\tASSERT(data_.size() > 0);\n> +\n> +\t\tif (quantization_ > 0)\n> +\t\t\tkey = std::lround(key / static_cast<double>(quantization_)) * quantization_;\n> +\n> +\t\tif (quantizedKey)\n> +\t\t\t*quantizedKey = key;\n> +\n> +\t\tif (lastInterpolatedKey_.has_value() &&\n> +\t\t    *lastInterpolatedKey_ == key)\n> +\t\t\treturn lastInterpolatedValue_;\n> +\n> +\t\tauto it = data_.lower_bound(key);\n> +\n> +\t\tif (it == data_.begin())\n> +\t\t\treturn it->second;\n> +\n> +\t\tif (it == data_.end())\n> +\t\t\treturn std::prev(it)->second;\n> +\n> +\t\tif (it->first == key)\n> +\t\t\treturn it->second;\n> +\n> +\t\tauto it2 = std::prev(it);\n> +\t\tdouble lambda = (key - it2->first) / static_cast<double>(it->first - it2->first);\n> +\t\tinterpolate(it2->second, it->second, lastInterpolatedValue_, lambda);\n> +\t\tlastInterpolatedKey_ = key;\n> +\n> +\t\treturn lastInterpolatedValue_;\n\nThat's a long inline function. Have you checked how much code this\ngenerates ? Does it get inlined by the compiler ? Is there a way to\nsplit part of the code to a non-inline function ?\n\n> +\t}\n> +\n> +\tvoid interpolate(const T &a, const T &b, T &dest, double lambda)\n> +\t{\n> +\t\tdest = a * (1.0 - lambda) + b * lambda;\n> +\t}\n> +\n> +private:\n> +\tstd::map<unsigned int, T> data_;\n> +\tT lastInterpolatedValue_;\n> +\tstd::optional<unsigned int> lastInterpolatedKey_;\n> +\tunsigned int quantization_ = 0;\n> +};\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index eff8ce2660f1..2c2712a7d252 100644\n> --- a/src/ipa/libipa/meson.build\n> +++ b/src/ipa/libipa/meson.build\n> @@ -7,6 +7,7 @@ libipa_headers = files([\n>      'exposure_mode_helper.h',\n>      'fc_queue.h',\n>      'histogram.h',\n> +    'interpolator.h',\n>      'matrix.h',\n>      'matrix_interpolator.h',\n>      'module.h',\n> @@ -21,6 +22,7 @@ libipa_sources = files([\n>      'exposure_mode_helper.cpp',\n>      'fc_queue.cpp',\n>      'histogram.cpp',\n> +    'interpolator.cpp',\n>      'matrix.cpp',\n>      'matrix_interpolator.cpp',\n>      'module.cpp',\n> -- \n> 2.43.0\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 137F3C0F1B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 23 Sep 2024 20:30:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 42B256350C;\n\tMon, 23 Sep 2024 22:30:25 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 502016037E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 23 Sep 2024 22:30:23 +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 CDA7E63D;\n\tMon, 23 Sep 2024 22:28:56 +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=\"fje6W68t\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727123337;\n\tbh=YHgr7GtD32b+933N7a19qZIYm5rQKov8hdxEQfLo68k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fje6W68tWapaMYJzUUNvsZsuFIzpl/OyiQpQ7YnDS36KgFDR9om2kjwx/AQ+ZU+NF\n\tnQPx+HcTXr3495FEI8/jjhQZmHxbmn61AgcFGGlUUKV5X6gGller6kB7X2GDuZvpqJ\n\thK4AlgwwzDbhicUAwKJi7Wdc8CGuBs1jBzzAPi7Q=","Date":"Mon, 23 Sep 2024 23:29:51 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Stefan Klug <stefan.klug@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham <kieran.bingham@ideasonboard.com>,\n\tPaul Elder <paul.elder@ideasonboard.com>","Subject":"Re: [PATCH v3 1/9] ipa: libipa: Add generic Interpolator class","Message-ID":"<20240923202951.GC7165@pendragon.ideasonboard.com>","References":"<20240920133941.90629-1-stefan.klug@ideasonboard.com>\n\t<20240920133941.90629-2-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240920133941.90629-2-stefan.klug@ideasonboard.com>","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>"}}]