[{"id":29802,"web_url":"https://patchwork.libcamera.org/comment/29802/","msgid":"<171801914305.1489778.10039700382651671696@ping.linuxembedded.co.uk>","date":"2024-06-10T11:32:23","subject":"Re: [PATCH v6 2/3] ipa: libipa: Add MatrixInterpolator class","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Paul Elder (2024-06-07 09:09:05)\n> Add a class to encapsulate the functionality of fetching a matrix based\n> on an integer key, and interpolating if there is no exact match. This is\n> expected to be used by both color correction matrices / crosstalk\n> correction as well as lens shading correction.\n> \n> A cache is included only for exact matches of the key. The caller is\n> expected to decide the tolererance for rounding.\n> \n> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>\n> \n> ---\n> Changes in v6:\n> - fix doxygen\n> \n> Changes in v5:\n> - improve documentation\n> - replace count() with find() != end() (in get())\n> - add cache\n> \n> Changes in v4:\n> - remove stray semicolons\n> - read from the new yaml layout (which mirrors what we have at the\n>   moment for lsc), and add keys to make it more flexible\n> \n> Changes in v3:\n> - add a constructor that takes a map of unsigned int -> matrix\n> - s/unit/identity\n> - clear matrices on reset and when reading from yaml\n> - add assert at get()\n> \n> Changes in v2:\n> - initialize to identity matrix\n> - add a function to reset to identity matrix\n> - other minor fixes\n> ---\n>  src/ipa/libipa/matrix_interpolator.cpp | 111 +++++++++++++++++++++\n>  src/ipa/libipa/matrix_interpolator.h   | 131 +++++++++++++++++++++++++\n>  src/ipa/libipa/meson.build             |   2 +\n>  3 files changed, 244 insertions(+)\n>  create mode 100644 src/ipa/libipa/matrix_interpolator.cpp\n>  create mode 100644 src/ipa/libipa/matrix_interpolator.h\n> \n> diff --git a/src/ipa/libipa/matrix_interpolator.cpp b/src/ipa/libipa/matrix_interpolator.cpp\n> new file mode 100644\n> index 000000000..064af9b06\n> --- /dev/null\n> +++ b/src/ipa/libipa/matrix_interpolator.cpp\n> @@ -0,0 +1,111 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2019, Raspberry Pi Ltd\n> + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>\n> + *\n> + * Helper class for interpolating maps of matrices\n> + */\n> +#include \"matrix_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 \"matrix.h\"\n> +\n> +/**\n> + * \\file matrix_interpolator.h\n> + * \\brief Helper class for interpolating maps of matrices\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(MatrixInterpolator)\n> +\n> +namespace ipa {\n> +\n> +/**\n> + * \\class MatrixInterpolator\n> + * \\brief Class for storing, retrieving, and interpolating matrices\n> + * \\tparam T Type of numerical values to be stored in the matrices\n> + * \\tparam R Number of rows in the matrices\n> + * \\tparam C Number of columns in the matrices\n> + *\n> + * The main use case is to pass a map from color temperatures to corresponding\n> + * matrices (eg. color correction), and then requesting a matrix for a specific\n> + * color temperature. This class will abstract away the interpolation portion.\n> + */\n> +\n> +/**\n> + * \\fn MatrixInterpolator::MatrixInterpolator(const std::map<unsigned int, Matrix<T, R, C>> &matrices)\n> + * \\brief Construct a matrix interpolator from a map of matrices\n> + * \\param matrices Map from which to construct the matrix interpolator\n> + */\n> +\n> +/**\n> + * \\fn MatrixInterpolator::reset()\n> + * \\brief Reset the matrix interpolator content to a single identity matrix\n> + */\n> +\n> +/**\n> + * \\fn int MatrixInterpolator<T, R, C>::readYaml(const libcamera::YamlObject &yaml)\n> + * \\brief Initialize an MatrixInterpolator instance from yaml\n> + * \\tparam T Type of data stored in the matrices\n> + * \\tparam R Number of rows of the matrices\n> + * \\tparam C Number of columns of the matrices\n> + * \\param[in] yaml The yaml object that contains the map of unsigned integers to matrices\n> + * \\param[in] key_name The name of the key in the yaml object\n> + * \\param[in] matrix_name The name of the matrix in the yaml object\n\nI don't see key_name, matrix_name in the definition above? (But I do see\nit below in the implementation).\n\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 matrix_name to the matrix. 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 matrix_name can be either\n> + * 'ccm' or 'offsets'. This way multiple matrix 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 matrix_name parameter.\n> + *\n> + * \\return Zero on success, negative error code otherwise\n> + */\n> +\n> +/**\n> + * \\fn Matrix<T, R, C> MatrixInterpolator<T, R, C>::get(unsigned int key)\n> + * \\brief Retrieve a matrix from the list of matrices, interpolating if necessary\n> + * \\param[in] key The unsigned integer key of the matrix to retrieve\n> + * \\return The matrix corresponding to the color temperature\n> + */\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/matrix_interpolator.h b/src/ipa/libipa/matrix_interpolator.h\n> new file mode 100644\n> index 000000000..27ab532eb\n> --- /dev/null\n> +++ b/src/ipa/libipa/matrix_interpolator.h\n> @@ -0,0 +1,131 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2019, Raspberry Pi Ltd\n> + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>\n> + *\n> + * Helper class for interpolating maps of matrices\n> + */\n> +\n> +#pragma once\n> +\n> +#include <algorithm>\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> +#include \"matrix.h\"\n> +\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(MatrixInterpolator)\n> +\n> +namespace ipa {\n> +\n> +#ifndef __DOXYGEN__\n> +template<typename T, unsigned int R, unsigned int C,\n> +        std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>\n> +#else\n> +template<typename T, unsigned int R, unsigned int C>\n> +#endif /* __DOXYGEN__ */\n> +class MatrixInterpolator\n> +{\n> +public:\n> +       MatrixInterpolator()\n> +       {\n> +               reset();\n> +       }\n> +\n> +       MatrixInterpolator(const std::map<unsigned int, Matrix<T, R, C>> &matrices)\n> +       {\n> +               for (const auto &pair : matrices)\n> +                       matrices_[pair.first] = pair.second;\n> +       }\n> +\n> +       ~MatrixInterpolator() {}\n> +\n> +       void reset()\n> +       {\n> +               Matrix<T, R, C> identity;\n> +               matrices_.clear();\n> +               matrices_[0] = identity;\n> +       }\n> +\n> +       int readYaml(const libcamera::YamlObject &yaml,\n> +                    const std::string &key_name,\n> +                    const std::string &matrix_name)\n> +       {\n> +               int ret;\n> +\n> +               matrices_.clear();\n> +\n> +               if (!yaml.isList()) {\n> +                       LOG(MatrixInterpolator, Error) << \"yaml object must be a list\";\n> +                       return -EINVAL;\n> +               }\n> +\n> +               for (const auto &value : yaml.asList()) {\n> +                       unsigned int ct = std::stoul(value[key_name].get<std::string>(\"\"));\n> +                       Matrix<T, R, C> matrix;\n> +                       if ((ret = matrix.readYaml(value[matrix_name])) < 0) {\n> +                               LOG(MatrixInterpolator, Error) << \"Failed to read matrix\";\n> +                               return ret;\n> +                       }\n> +\n> +                       matrices_[ct] = matrix;\n> +\n> +                       LOG(MatrixInterpolator, Debug)\n> +                               << \"Read matrix '\" << matrix_name << \"' for key '\"\n> +                               << key_name << \"' \" << ct << \": \"\n> +                               << matrices_[ct].toString();\n> +               }\n> +\n> +               if (matrices_.size() < 1) {\n> +                       LOG(MatrixInterpolator, Error) << \"Need at least one matrix\";\n> +                       return -EINVAL;\n> +               }\n> +\n> +               return 0;\n> +       }\n> +\n> +       Matrix<T, R, C> get(unsigned int ct)\n> +       {\n> +               ASSERT(matrices_.size() > 0);\n> +\n> +               if (matrices_.size() == 1 ||\n> +                   ct <= matrices_.begin()->first)\n> +                       return matrices_.begin()->second;\n> +\n> +               if (ct >= matrices_.rbegin()->first)\n> +                       return matrices_.rbegin()->second;\n> +\n> +               if (matrices_.find(ct) != matrices_.end())\n> +                       return matrices_[ct];\n> +\n> +               if (cache_.find(ct) != cache_.end())\n> +                       return cache_[ct];\n> +\n> +               /* The above four guarantee that this will succeed */\n> +               auto iter = matrices_.upper_bound(ct);\n> +               unsigned int ctUpper = iter->first;\n> +               unsigned int ctLower = (--iter)->first;\n> +\n> +               double lambda = (ct - ctLower) / static_cast<double>(ctUpper - ctLower);\n> +               Matrix<T, R, C> ret =\n> +                       lambda * matrices_[ctUpper] + (1.0 - lambda) * matrices_[ctLower];\n> +               cache_[ct] = ret;\n\nYikes. I like this addition but I'm a little bit worried here.\n\nImagine some changing light scene causes us to generate 'every' possible\nlight colour temperature. This would consume a potentially large amount\nof memory. Particularly if this was interpolating LSC tables.\n\nThe cache_ needs to be limited in size ideally as some form of LRU.\n\nI assumed we'd just keep a single cached Matrix originally (which I\nguess is an LRU of size 1...) , but if it's worth adding multiple\nthen I think it needs some limits already.\n\n\nAs I asked for the cache, I don't mind if it's removed if that's easier\nfor the short term and added in on top with an LRU, or changed to store\nonly a single cached ct, or an LRU is implemented.\n\n\nOtherwise, everything else looks fine to me.\n\n\n\n> +               return ret;\n> +       }\n> +\n> +private:\n> +       std::map<unsigned int, Matrix<T, R, C>> matrices_;\n> +\n> +       std::map<unsigned int, Matrix<T, R, C>> cache_;\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 2547a5b83..901d78549 100644\n> --- a/src/ipa/libipa/meson.build\n> +++ b/src/ipa/libipa/meson.build\n> @@ -8,6 +8,7 @@ libipa_headers = files([\n>      'fc_queue.h',\n>      'histogram.h',\n>      'matrix.h',\n> +    'matrix_interpolator.h',\n>      'module.h',\n>      'pwl.h',\n>  ])\n> @@ -20,6 +21,7 @@ libipa_sources = files([\n>      'fc_queue.cpp',\n>      'histogram.cpp',\n>      'matrix.cpp',\n> +    'matrix_interpolator.cpp',\n>      'module.cpp',\n>      'pwl.cpp',\n>  ])\n> -- \n> 2.39.2\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 C4524C31E9\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 10 Jun 2024 11:32:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D0DCF6545E;\n\tMon, 10 Jun 2024 13:32:27 +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 0F8AC65446\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 10 Jun 2024 13:32:26 +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 DFBD666F;\n\tMon, 10 Jun 2024 13:32:13 +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=\"vc2G6a/l\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718019134;\n\tbh=NS6n6SRCZ3uLQqYCI9y2wzR06rVih0qvgNRQqK4E9Nw=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=vc2G6a/l8F/uC12L2a6NAc8Rry7sRXs5p/bPr1EOxCdDky/aol25xWHFYs57tKHS7\n\tLjatlBUmkO4mjN8daEp104c08SIQ2B2YjznL9wMGkBNl2m3whHo+7UwQIl+j6yUAsS\n\taWD+lW6JLsBPKq4zlwrmBoe1x8oRX0H0bPvP6MVE=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240607080906.2684579-3-paul.elder@ideasonboard.com>","References":"<20240607080906.2684579-1-paul.elder@ideasonboard.com>\n\t<20240607080906.2684579-3-paul.elder@ideasonboard.com>","Subject":"Re: [PATCH v6 2/3] ipa: libipa: Add MatrixInterpolator class","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"Paul Elder <paul.elder@ideasonboard.com>,\n\tStefan Klug <stefan.klug@ideasonboard.com>","To":"Paul Elder <paul.elder@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 10 Jun 2024 12:32:23 +0100","Message-ID":"<171801914305.1489778.10039700382651671696@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>"}},{"id":29812,"web_url":"https://patchwork.libcamera.org/comment/29812/","msgid":"<ZmcOEIqSuHf_wwKW@pyrite.rasen.tech>","date":"2024-06-10T14:30:40","subject":"Re: [PATCH v6 2/3] ipa: libipa: Add MatrixInterpolator class","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Mon, Jun 10, 2024 at 12:32:23PM +0100, Kieran Bingham wrote:\n> Quoting Paul Elder (2024-06-07 09:09:05)\n> > Add a class to encapsulate the functionality of fetching a matrix based\n> > on an integer key, and interpolating if there is no exact match. This is\n> > expected to be used by both color correction matrices / crosstalk\n> > correction as well as lens shading correction.\n> > \n> > A cache is included only for exact matches of the key. The caller is\n> > expected to decide the tolererance for rounding.\n> > \n> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>\n> > \n> > ---\n> > Changes in v6:\n> > - fix doxygen\n> > \n> > Changes in v5:\n> > - improve documentation\n> > - replace count() with find() != end() (in get())\n> > - add cache\n> > \n> > Changes in v4:\n> > - remove stray semicolons\n> > - read from the new yaml layout (which mirrors what we have at the\n> >   moment for lsc), and add keys to make it more flexible\n> > \n> > Changes in v3:\n> > - add a constructor that takes a map of unsigned int -> matrix\n> > - s/unit/identity\n> > - clear matrices on reset and when reading from yaml\n> > - add assert at get()\n> > \n> > Changes in v2:\n> > - initialize to identity matrix\n> > - add a function to reset to identity matrix\n> > - other minor fixes\n> > ---\n> >  src/ipa/libipa/matrix_interpolator.cpp | 111 +++++++++++++++++++++\n> >  src/ipa/libipa/matrix_interpolator.h   | 131 +++++++++++++++++++++++++\n> >  src/ipa/libipa/meson.build             |   2 +\n> >  3 files changed, 244 insertions(+)\n> >  create mode 100644 src/ipa/libipa/matrix_interpolator.cpp\n> >  create mode 100644 src/ipa/libipa/matrix_interpolator.h\n> > \n> > diff --git a/src/ipa/libipa/matrix_interpolator.cpp b/src/ipa/libipa/matrix_interpolator.cpp\n> > new file mode 100644\n> > index 000000000..064af9b06\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/matrix_interpolator.cpp\n> > @@ -0,0 +1,111 @@\n> > +/* SPDX-License-Identifier: BSD-2-Clause */\n> > +/*\n> > + * Copyright (C) 2019, Raspberry Pi Ltd\n> > + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>\n> > + *\n> > + * Helper class for interpolating maps of matrices\n> > + */\n> > +#include \"matrix_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 \"matrix.h\"\n> > +\n> > +/**\n> > + * \\file matrix_interpolator.h\n> > + * \\brief Helper class for interpolating maps of matrices\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(MatrixInterpolator)\n> > +\n> > +namespace ipa {\n> > +\n> > +/**\n> > + * \\class MatrixInterpolator\n> > + * \\brief Class for storing, retrieving, and interpolating matrices\n> > + * \\tparam T Type of numerical values to be stored in the matrices\n> > + * \\tparam R Number of rows in the matrices\n> > + * \\tparam C Number of columns in the matrices\n> > + *\n> > + * The main use case is to pass a map from color temperatures to corresponding\n> > + * matrices (eg. color correction), and then requesting a matrix for a specific\n> > + * color temperature. This class will abstract away the interpolation portion.\n> > + */\n> > +\n> > +/**\n> > + * \\fn MatrixInterpolator::MatrixInterpolator(const std::map<unsigned int, Matrix<T, R, C>> &matrices)\n> > + * \\brief Construct a matrix interpolator from a map of matrices\n> > + * \\param matrices Map from which to construct the matrix interpolator\n> > + */\n> > +\n> > +/**\n> > + * \\fn MatrixInterpolator::reset()\n> > + * \\brief Reset the matrix interpolator content to a single identity matrix\n> > + */\n> > +\n> > +/**\n> > + * \\fn int MatrixInterpolator<T, R, C>::readYaml(const libcamera::YamlObject &yaml)\n> > + * \\brief Initialize an MatrixInterpolator instance from yaml\n> > + * \\tparam T Type of data stored in the matrices\n> > + * \\tparam R Number of rows of the matrices\n> > + * \\tparam C Number of columns of the matrices\n> > + * \\param[in] yaml The yaml object that contains the map of unsigned integers to matrices\n> > + * \\param[in] key_name The name of the key in the yaml object\n> > + * \\param[in] matrix_name The name of the matrix in the yaml object\n> \n> I don't see key_name, matrix_name in the definition above? (But I do see\n> it below in the implementation).\n\nDoxygen seems to match it fine without, so I'll just remove the\nparameters...\n\n> \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 matrix_name to the matrix. 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 matrix_name can be either\n> > + * 'ccm' or 'offsets'. This way multiple matrix 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 matrix_name parameter.\n> > + *\n> > + * \\return Zero on success, negative error code otherwise\n> > + */\n> > +\n> > +/**\n> > + * \\fn Matrix<T, R, C> MatrixInterpolator<T, R, C>::get(unsigned int key)\n> > + * \\brief Retrieve a matrix from the list of matrices, interpolating if necessary\n> > + * \\param[in] key The unsigned integer key of the matrix to retrieve\n> > + * \\return The matrix corresponding to the color temperature\n> > + */\n> > +\n> > +} /* namespace ipa */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/libipa/matrix_interpolator.h b/src/ipa/libipa/matrix_interpolator.h\n> > new file mode 100644\n> > index 000000000..27ab532eb\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/matrix_interpolator.h\n> > @@ -0,0 +1,131 @@\n> > +/* SPDX-License-Identifier: BSD-2-Clause */\n> > +/*\n> > + * Copyright (C) 2019, Raspberry Pi Ltd\n> > + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>\n> > + *\n> > + * Helper class for interpolating maps of matrices\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include <algorithm>\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> > +#include \"matrix.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DECLARE_CATEGORY(MatrixInterpolator)\n> > +\n> > +namespace ipa {\n> > +\n> > +#ifndef __DOXYGEN__\n> > +template<typename T, unsigned int R, unsigned int C,\n> > +        std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>\n> > +#else\n> > +template<typename T, unsigned int R, unsigned int C>\n> > +#endif /* __DOXYGEN__ */\n> > +class MatrixInterpolator\n> > +{\n> > +public:\n> > +       MatrixInterpolator()\n> > +       {\n> > +               reset();\n> > +       }\n> > +\n> > +       MatrixInterpolator(const std::map<unsigned int, Matrix<T, R, C>> &matrices)\n> > +       {\n> > +               for (const auto &pair : matrices)\n> > +                       matrices_[pair.first] = pair.second;\n> > +       }\n> > +\n> > +       ~MatrixInterpolator() {}\n> > +\n> > +       void reset()\n> > +       {\n> > +               Matrix<T, R, C> identity;\n> > +               matrices_.clear();\n> > +               matrices_[0] = identity;\n> > +       }\n> > +\n> > +       int readYaml(const libcamera::YamlObject &yaml,\n> > +                    const std::string &key_name,\n> > +                    const std::string &matrix_name)\n> > +       {\n> > +               int ret;\n> > +\n> > +               matrices_.clear();\n> > +\n> > +               if (!yaml.isList()) {\n> > +                       LOG(MatrixInterpolator, Error) << \"yaml object must be a list\";\n> > +                       return -EINVAL;\n> > +               }\n> > +\n> > +               for (const auto &value : yaml.asList()) {\n> > +                       unsigned int ct = std::stoul(value[key_name].get<std::string>(\"\"));\n> > +                       Matrix<T, R, C> matrix;\n> > +                       if ((ret = matrix.readYaml(value[matrix_name])) < 0) {\n> > +                               LOG(MatrixInterpolator, Error) << \"Failed to read matrix\";\n> > +                               return ret;\n> > +                       }\n> > +\n> > +                       matrices_[ct] = matrix;\n> > +\n> > +                       LOG(MatrixInterpolator, Debug)\n> > +                               << \"Read matrix '\" << matrix_name << \"' for key '\"\n> > +                               << key_name << \"' \" << ct << \": \"\n> > +                               << matrices_[ct].toString();\n> > +               }\n> > +\n> > +               if (matrices_.size() < 1) {\n> > +                       LOG(MatrixInterpolator, Error) << \"Need at least one matrix\";\n> > +                       return -EINVAL;\n> > +               }\n> > +\n> > +               return 0;\n> > +       }\n> > +\n> > +       Matrix<T, R, C> get(unsigned int ct)\n> > +       {\n> > +               ASSERT(matrices_.size() > 0);\n> > +\n> > +               if (matrices_.size() == 1 ||\n> > +                   ct <= matrices_.begin()->first)\n> > +                       return matrices_.begin()->second;\n> > +\n> > +               if (ct >= matrices_.rbegin()->first)\n> > +                       return matrices_.rbegin()->second;\n> > +\n> > +               if (matrices_.find(ct) != matrices_.end())\n> > +                       return matrices_[ct];\n> > +\n> > +               if (cache_.find(ct) != cache_.end())\n> > +                       return cache_[ct];\n> > +\n> > +               /* The above four guarantee that this will succeed */\n> > +               auto iter = matrices_.upper_bound(ct);\n> > +               unsigned int ctUpper = iter->first;\n> > +               unsigned int ctLower = (--iter)->first;\n> > +\n> > +               double lambda = (ct - ctLower) / static_cast<double>(ctUpper - ctLower);\n> > +               Matrix<T, R, C> ret =\n> > +                       lambda * matrices_[ctUpper] + (1.0 - lambda) * matrices_[ctLower];\n> > +               cache_[ct] = ret;\n> \n> Yikes. I like this addition but I'm a little bit worried here.\n> \n> Imagine some changing light scene causes us to generate 'every' possible\n> light colour temperature. This would consume a potentially large amount\n> of memory. Particularly if this was interpolating LSC tables.\n> \n> The cache_ needs to be limited in size ideally as some form of LRU.\n> \n> I assumed we'd just keep a single cached Matrix originally (which I\n> guess is an LRU of size 1...) , but if it's worth adding multiple\n> then I think it needs some limits already.\n> \n> \n> As I asked for the cache, I don't mind if it's removed if that's easier\n> for the short term and added in on top with an LRU, or changed to store\n> only a single cached ct, or an LRU is implemented.\n\nAlright I'll remove it for now.\n\n> \n> \n> Otherwise, everything else looks fine to me.\n\n\nThanks,\n\nPaul\n\n> \n> \n> \n> > +               return ret;\n> > +       }\n> > +\n> > +private:\n> > +       std::map<unsigned int, Matrix<T, R, C>> matrices_;\n> > +\n> > +       std::map<unsigned int, Matrix<T, R, C>> cache_;\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 2547a5b83..901d78549 100644\n> > --- a/src/ipa/libipa/meson.build\n> > +++ b/src/ipa/libipa/meson.build\n> > @@ -8,6 +8,7 @@ libipa_headers = files([\n> >      'fc_queue.h',\n> >      'histogram.h',\n> >      'matrix.h',\n> > +    'matrix_interpolator.h',\n> >      'module.h',\n> >      'pwl.h',\n> >  ])\n> > @@ -20,6 +21,7 @@ libipa_sources = files([\n> >      'fc_queue.cpp',\n> >      'histogram.cpp',\n> >      'matrix.cpp',\n> > +    'matrix_interpolator.cpp',\n> >      'module.cpp',\n> >      'pwl.cpp',\n> >  ])\n> > -- \n> > 2.39.2\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 9A872BD87C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 10 Jun 2024 14:30:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E06DA6545E;\n\tMon, 10 Jun 2024 16:30:50 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 37AD8634D5\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 10 Jun 2024 16:30:49 +0200 (CEST)","from pyrite.rasen.tech (h175-177-049-156.catv02.itscom.jp\n\t[175.177.49.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 71C51397;\n\tMon, 10 Jun 2024 16:30:35 +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=\"DQegO0R2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718029837;\n\tbh=yDaazpkNfBfHegCI+6WC8IO+DkSqcGqFVQVU/6inVyc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DQegO0R2jy0Z2LjcZMGgLxjSd+JG0+1CYVXkaj7wOd8aPo/+nycToefeFA5VvPyEf\n\t4Fa1GNITbM7vTLLgbYib52PM8CZ7jmENH5eXTd2iS6k/bgIdPqMvAZflDwHEOLA5W/\n\tiIkHzPhMfCJQvoPw4uU/2AkbIX6s3OuUCCSqWFGY=","Date":"Mon, 10 Jun 2024 23:30:40 +0900","From":"Paul Elder <paul.elder@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org,\n\tStefan Klug <stefan.klug@ideasonboard.com>","Subject":"Re: [PATCH v6 2/3] ipa: libipa: Add MatrixInterpolator class","Message-ID":"<ZmcOEIqSuHf_wwKW@pyrite.rasen.tech>","References":"<20240607080906.2684579-1-paul.elder@ideasonboard.com>\n\t<20240607080906.2684579-3-paul.elder@ideasonboard.com>\n\t<171801914305.1489778.10039700382651671696@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<171801914305.1489778.10039700382651671696@ping.linuxembedded.co.uk>","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>"}}]