[v6,2/3] ipa: libipa: Add MatrixInterpolator class

20235 diff mbox series

Add a class to encapsulate the functionality of fetching a matrix based
on an integer key, and interpolating if there is no exact match. This is
expected to be used by both color correction matrices / crosstalk
correction as well as lens shading correction.

A cache is included only for exact matches of the key. The caller is
expected to decide the tolererance for rounding.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>

---
Changes in v6:
- fix doxygen

Changes in v5:
- improve documentation
- replace count() with find() != end() (in get())
- add cache

Changes in v4:
- remove stray semicolons
- read from the new yaml layout (which mirrors what we have at the
  moment for lsc), and add keys to make it more flexible

Changes in v3:
- add a constructor that takes a map of unsigned int -> matrix
- s/unit/identity
- clear matrices on reset and when reading from yaml
- add assert at get()

Changes in v2:
- initialize to identity matrix
- add a function to reset to identity matrix
- other minor fixes
---
 src/ipa/libipa/matrix_interpolator.cpp | 111 +++++++++++++++++++++
 src/ipa/libipa/matrix_interpolator.h   | 131 +++++++++++++++++++++++++
 src/ipa/libipa/meson.build             |   2 +
 3 files changed, 244 insertions(+)
 create mode 100644 src/ipa/libipa/matrix_interpolator.cpp
 create mode 100644 src/ipa/libipa/matrix_interpolator.h

Quoting Paul Elder (2024-06-07 09:09:05)
> Add a class to encapsulate the functionality of fetching a matrix based
> on an integer key, and interpolating if there is no exact match. This is
> expected to be used by both color correction matrices / crosstalk
> correction as well as lens shading correction.
> 
> A cache is included only for exact matches of the key. The caller is
> expected to decide the tolererance for rounding.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> 
> ---
> Changes in v6:
> - fix doxygen
> 
> Changes in v5:
> - improve documentation
> - replace count() with find() != end() (in get())
> - add cache
> 
> Changes in v4:
> - remove stray semicolons
> - read from the new yaml layout (which mirrors what we have at the
>   moment for lsc), and add keys to make it more flexible
> 
> Changes in v3:
> - add a constructor that takes a map of unsigned int -> matrix
> - s/unit/identity
> - clear matrices on reset and when reading from yaml
> - add assert at get()
> 
> Changes in v2:
> - initialize to identity matrix
> - add a function to reset to identity matrix
> - other minor fixes
> ---
>  src/ipa/libipa/matrix_interpolator.cpp | 111 +++++++++++++++++++++
>  src/ipa/libipa/matrix_interpolator.h   | 131 +++++++++++++++++++++++++
>  src/ipa/libipa/meson.build             |   2 +
>  3 files changed, 244 insertions(+)
>  create mode 100644 src/ipa/libipa/matrix_interpolator.cpp
>  create mode 100644 src/ipa/libipa/matrix_interpolator.h
> 
> diff --git a/src/ipa/libipa/matrix_interpolator.cpp b/src/ipa/libipa/matrix_interpolator.cpp
> new file mode 100644
> index 000000000..064af9b06
> --- /dev/null
> +++ b/src/ipa/libipa/matrix_interpolator.cpp
> @@ -0,0 +1,111 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2019, Raspberry Pi Ltd
> + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
> + *
> + * Helper class for interpolating maps of matrices
> + */
> +#include "matrix_interpolator.h"
> +
> +#include <algorithm>
> +#include <string>
> +
> +#include <libcamera/base/log.h>
> +
> +#include "libcamera/internal/yaml_parser.h"
> +
> +#include "matrix.h"
> +
> +/**
> + * \file matrix_interpolator.h
> + * \brief Helper class for interpolating maps of matrices
> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(MatrixInterpolator)
> +
> +namespace ipa {
> +
> +/**
> + * \class MatrixInterpolator
> + * \brief Class for storing, retrieving, and interpolating matrices
> + * \tparam T Type of numerical values to be stored in the matrices
> + * \tparam R Number of rows in the matrices
> + * \tparam C Number of columns in the matrices
> + *
> + * The main use case is to pass a map from color temperatures to corresponding
> + * matrices (eg. color correction), and then requesting a matrix for a specific
> + * color temperature. This class will abstract away the interpolation portion.
> + */
> +
> +/**
> + * \fn MatrixInterpolator::MatrixInterpolator(const std::map<unsigned int, Matrix<T, R, C>> &matrices)
> + * \brief Construct a matrix interpolator from a map of matrices
> + * \param matrices Map from which to construct the matrix interpolator
> + */
> +
> +/**
> + * \fn MatrixInterpolator::reset()
> + * \brief Reset the matrix interpolator content to a single identity matrix
> + */
> +
> +/**
> + * \fn int MatrixInterpolator<T, R, C>::readYaml(const libcamera::YamlObject &yaml)
> + * \brief Initialize an MatrixInterpolator instance from yaml
> + * \tparam T Type of data stored in the matrices
> + * \tparam R Number of rows of the matrices
> + * \tparam C Number of columns of the matrices
> + * \param[in] yaml The yaml object that contains the map of unsigned integers to matrices
> + * \param[in] key_name The name of the key in the yaml object
> + * \param[in] matrix_name The name of the matrix in the yaml object

I don't see key_name, matrix_name in the definition above? (But I do see
it below in the implementation).

> + *
> + * The yaml object is expected to be a list of maps. Each map has two or more
> + * pairs: one of \a key_name to the key value (usually color temperature), and
> + * one or more of \a matrix_name to the matrix. This is a bit difficult to
> + * explain, so here is an example (in python, as it is easier to parse than
> + * yaml):
> + *       [
> + *               {
> + *                   'ct': 2860,
> + *                   'ccm': [ 2.12089, -0.52461, -0.59629,
> + *                           -0.85342,  2.80445, -0.95103,
> + *                           -0.26897, -1.14788,  2.41685 ],
> + *                   'offsets': [ 0, 0, 0 ]
> + *               },
> + *
> + *               {
> + *                   'ct': 2960,
> + *                   'ccm': [ 2.26962, -0.54174, -0.72789,
> + *                           -0.77008,  2.60271, -0.83262,
> + *                           -0.26036, -1.51254,  2.77289 ],
> + *                   'offsets': [ 0, 0, 0 ]
> + *               },
> + *
> + *               {
> + *                   'ct': 3603,
> + *                   'ccm': [ 2.18644, -0.66148, -0.52496,
> + *                           -0.77828,  2.69474, -0.91645,
> + *                           -0.25239, -0.83059,  2.08298 ],
> + *                   'offsets': [ 0, 0, 0 ]
> + *               },
> + *       ]
> + *
> + * In this case, \a key_name would be 'ct', and \a matrix_name can be either
> + * 'ccm' or 'offsets'. This way multiple matrix interpolators can be defined in
> + * one set of color temperature ranges in the tuning file, and they can be
> + * retrieved separately with the \a matrix_name parameter.
> + *
> + * \return Zero on success, negative error code otherwise
> + */
> +
> +/**
> + * \fn Matrix<T, R, C> MatrixInterpolator<T, R, C>::get(unsigned int key)
> + * \brief Retrieve a matrix from the list of matrices, interpolating if necessary
> + * \param[in] key The unsigned integer key of the matrix to retrieve
> + * \return The matrix corresponding to the color temperature
> + */
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/matrix_interpolator.h b/src/ipa/libipa/matrix_interpolator.h
> new file mode 100644
> index 000000000..27ab532eb
> --- /dev/null
> +++ b/src/ipa/libipa/matrix_interpolator.h
> @@ -0,0 +1,131 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2019, Raspberry Pi Ltd
> + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
> + *
> + * Helper class for interpolating maps of matrices
> + */
> +
> +#pragma once
> +
> +#include <algorithm>
> +#include <map>
> +#include <string>
> +#include <tuple>
> +
> +#include <libcamera/base/log.h>
> +
> +#include "libcamera/internal/yaml_parser.h"
> +
> +#include "matrix.h"
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(MatrixInterpolator)
> +
> +namespace ipa {
> +
> +#ifndef __DOXYGEN__
> +template<typename T, unsigned int R, unsigned int C,
> +        std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>
> +#else
> +template<typename T, unsigned int R, unsigned int C>
> +#endif /* __DOXYGEN__ */
> +class MatrixInterpolator
> +{
> +public:
> +       MatrixInterpolator()
> +       {
> +               reset();
> +       }
> +
> +       MatrixInterpolator(const std::map<unsigned int, Matrix<T, R, C>> &matrices)
> +       {
> +               for (const auto &pair : matrices)
> +                       matrices_[pair.first] = pair.second;
> +       }
> +
> +       ~MatrixInterpolator() {}
> +
> +       void reset()
> +       {
> +               Matrix<T, R, C> identity;
> +               matrices_.clear();
> +               matrices_[0] = identity;
> +       }
> +
> +       int readYaml(const libcamera::YamlObject &yaml,
> +                    const std::string &key_name,
> +                    const std::string &matrix_name)
> +       {
> +               int ret;
> +
> +               matrices_.clear();
> +
> +               if (!yaml.isList()) {
> +                       LOG(MatrixInterpolator, Error) << "yaml object must be a list";
> +                       return -EINVAL;
> +               }
> +
> +               for (const auto &value : yaml.asList()) {
> +                       unsigned int ct = std::stoul(value[key_name].get<std::string>(""));
> +                       Matrix<T, R, C> matrix;
> +                       if ((ret = matrix.readYaml(value[matrix_name])) < 0) {
> +                               LOG(MatrixInterpolator, Error) << "Failed to read matrix";
> +                               return ret;
> +                       }
> +
> +                       matrices_[ct] = matrix;
> +
> +                       LOG(MatrixInterpolator, Debug)
> +                               << "Read matrix '" << matrix_name << "' for key '"
> +                               << key_name << "' " << ct << ": "
> +                               << matrices_[ct].toString();
> +               }
> +
> +               if (matrices_.size() < 1) {
> +                       LOG(MatrixInterpolator, Error) << "Need at least one matrix";
> +                       return -EINVAL;
> +               }
> +
> +               return 0;
> +       }
> +
> +       Matrix<T, R, C> get(unsigned int ct)
> +       {
> +               ASSERT(matrices_.size() > 0);
> +
> +               if (matrices_.size() == 1 ||
> +                   ct <= matrices_.begin()->first)
> +                       return matrices_.begin()->second;
> +
> +               if (ct >= matrices_.rbegin()->first)
> +                       return matrices_.rbegin()->second;
> +
> +               if (matrices_.find(ct) != matrices_.end())
> +                       return matrices_[ct];
> +
> +               if (cache_.find(ct) != cache_.end())
> +                       return cache_[ct];
> +
> +               /* The above four guarantee that this will succeed */
> +               auto iter = matrices_.upper_bound(ct);
> +               unsigned int ctUpper = iter->first;
> +               unsigned int ctLower = (--iter)->first;
> +
> +               double lambda = (ct - ctLower) / static_cast<double>(ctUpper - ctLower);
> +               Matrix<T, R, C> ret =
> +                       lambda * matrices_[ctUpper] + (1.0 - lambda) * matrices_[ctLower];
> +               cache_[ct] = ret;

Yikes. I like this addition but I'm a little bit worried here.

Imagine some changing light scene causes us to generate 'every' possible
light colour temperature. This would consume a potentially large amount
of memory. Particularly if this was interpolating LSC tables.

The cache_ needs to be limited in size ideally as some form of LRU.

I assumed we'd just keep a single cached Matrix originally (which I
guess is an LRU of size 1...) , but if it's worth adding multiple
then I think it needs some limits already.


As I asked for the cache, I don't mind if it's removed if that's easier
for the short term and added in on top with an LRU, or changed to store
only a single cached ct, or an LRU is implemented.


Otherwise, everything else looks fine to me.



> +               return ret;
> +       }
> +
> +private:
> +       std::map<unsigned int, Matrix<T, R, C>> matrices_;
> +
> +       std::map<unsigned int, Matrix<T, R, C>> cache_;
> +};
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> index 2547a5b83..901d78549 100644
> --- a/src/ipa/libipa/meson.build
> +++ b/src/ipa/libipa/meson.build
> @@ -8,6 +8,7 @@ libipa_headers = files([
>      'fc_queue.h',
>      'histogram.h',
>      'matrix.h',
> +    'matrix_interpolator.h',
>      'module.h',
>      'pwl.h',
>  ])
> @@ -20,6 +21,7 @@ libipa_sources = files([
>      'fc_queue.cpp',
>      'histogram.cpp',
>      'matrix.cpp',
> +    'matrix_interpolator.cpp',
>      'module.cpp',
>      'pwl.cpp',
>  ])
> -- 
> 2.39.2
>

On Mon, Jun 10, 2024 at 12:32:23PM +0100, Kieran Bingham wrote:
> Quoting Paul Elder (2024-06-07 09:09:05)
> > Add a class to encapsulate the functionality of fetching a matrix based
> > on an integer key, and interpolating if there is no exact match. This is
> > expected to be used by both color correction matrices / crosstalk
> > correction as well as lens shading correction.
> > 
> > A cache is included only for exact matches of the key. The caller is
> > expected to decide the tolererance for rounding.
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > 
> > ---
> > Changes in v6:
> > - fix doxygen
> > 
> > Changes in v5:
> > - improve documentation
> > - replace count() with find() != end() (in get())
> > - add cache
> > 
> > Changes in v4:
> > - remove stray semicolons
> > - read from the new yaml layout (which mirrors what we have at the
> >   moment for lsc), and add keys to make it more flexible
> > 
> > Changes in v3:
> > - add a constructor that takes a map of unsigned int -> matrix
> > - s/unit/identity
> > - clear matrices on reset and when reading from yaml
> > - add assert at get()
> > 
> > Changes in v2:
> > - initialize to identity matrix
> > - add a function to reset to identity matrix
> > - other minor fixes
> > ---
> >  src/ipa/libipa/matrix_interpolator.cpp | 111 +++++++++++++++++++++
> >  src/ipa/libipa/matrix_interpolator.h   | 131 +++++++++++++++++++++++++
> >  src/ipa/libipa/meson.build             |   2 +
> >  3 files changed, 244 insertions(+)
> >  create mode 100644 src/ipa/libipa/matrix_interpolator.cpp
> >  create mode 100644 src/ipa/libipa/matrix_interpolator.h
> > 
> > diff --git a/src/ipa/libipa/matrix_interpolator.cpp b/src/ipa/libipa/matrix_interpolator.cpp
> > new file mode 100644
> > index 000000000..064af9b06
> > --- /dev/null
> > +++ b/src/ipa/libipa/matrix_interpolator.cpp
> > @@ -0,0 +1,111 @@
> > +/* SPDX-License-Identifier: BSD-2-Clause */
> > +/*
> > + * Copyright (C) 2019, Raspberry Pi Ltd
> > + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
> > + *
> > + * Helper class for interpolating maps of matrices
> > + */
> > +#include "matrix_interpolator.h"
> > +
> > +#include <algorithm>
> > +#include <string>
> > +
> > +#include <libcamera/base/log.h>
> > +
> > +#include "libcamera/internal/yaml_parser.h"
> > +
> > +#include "matrix.h"
> > +
> > +/**
> > + * \file matrix_interpolator.h
> > + * \brief Helper class for interpolating maps of matrices
> > + */
> > +
> > +namespace libcamera {
> > +
> > +LOG_DEFINE_CATEGORY(MatrixInterpolator)
> > +
> > +namespace ipa {
> > +
> > +/**
> > + * \class MatrixInterpolator
> > + * \brief Class for storing, retrieving, and interpolating matrices
> > + * \tparam T Type of numerical values to be stored in the matrices
> > + * \tparam R Number of rows in the matrices
> > + * \tparam C Number of columns in the matrices
> > + *
> > + * The main use case is to pass a map from color temperatures to corresponding
> > + * matrices (eg. color correction), and then requesting a matrix for a specific
> > + * color temperature. This class will abstract away the interpolation portion.
> > + */
> > +
> > +/**
> > + * \fn MatrixInterpolator::MatrixInterpolator(const std::map<unsigned int, Matrix<T, R, C>> &matrices)
> > + * \brief Construct a matrix interpolator from a map of matrices
> > + * \param matrices Map from which to construct the matrix interpolator
> > + */
> > +
> > +/**
> > + * \fn MatrixInterpolator::reset()
> > + * \brief Reset the matrix interpolator content to a single identity matrix
> > + */
> > +
> > +/**
> > + * \fn int MatrixInterpolator<T, R, C>::readYaml(const libcamera::YamlObject &yaml)
> > + * \brief Initialize an MatrixInterpolator instance from yaml
> > + * \tparam T Type of data stored in the matrices
> > + * \tparam R Number of rows of the matrices
> > + * \tparam C Number of columns of the matrices
> > + * \param[in] yaml The yaml object that contains the map of unsigned integers to matrices
> > + * \param[in] key_name The name of the key in the yaml object
> > + * \param[in] matrix_name The name of the matrix in the yaml object
> 
> I don't see key_name, matrix_name in the definition above? (But I do see
> it below in the implementation).

Doxygen seems to match it fine without, so I'll just remove the
parameters...

> 
> > + *
> > + * The yaml object is expected to be a list of maps. Each map has two or more
> > + * pairs: one of \a key_name to the key value (usually color temperature), and
> > + * one or more of \a matrix_name to the matrix. This is a bit difficult to
> > + * explain, so here is an example (in python, as it is easier to parse than
> > + * yaml):
> > + *       [
> > + *               {
> > + *                   'ct': 2860,
> > + *                   'ccm': [ 2.12089, -0.52461, -0.59629,
> > + *                           -0.85342,  2.80445, -0.95103,
> > + *                           -0.26897, -1.14788,  2.41685 ],
> > + *                   'offsets': [ 0, 0, 0 ]
> > + *               },
> > + *
> > + *               {
> > + *                   'ct': 2960,
> > + *                   'ccm': [ 2.26962, -0.54174, -0.72789,
> > + *                           -0.77008,  2.60271, -0.83262,
> > + *                           -0.26036, -1.51254,  2.77289 ],
> > + *                   'offsets': [ 0, 0, 0 ]
> > + *               },
> > + *
> > + *               {
> > + *                   'ct': 3603,
> > + *                   'ccm': [ 2.18644, -0.66148, -0.52496,
> > + *                           -0.77828,  2.69474, -0.91645,
> > + *                           -0.25239, -0.83059,  2.08298 ],
> > + *                   'offsets': [ 0, 0, 0 ]
> > + *               },
> > + *       ]
> > + *
> > + * In this case, \a key_name would be 'ct', and \a matrix_name can be either
> > + * 'ccm' or 'offsets'. This way multiple matrix interpolators can be defined in
> > + * one set of color temperature ranges in the tuning file, and they can be
> > + * retrieved separately with the \a matrix_name parameter.
> > + *
> > + * \return Zero on success, negative error code otherwise
> > + */
> > +
> > +/**
> > + * \fn Matrix<T, R, C> MatrixInterpolator<T, R, C>::get(unsigned int key)
> > + * \brief Retrieve a matrix from the list of matrices, interpolating if necessary
> > + * \param[in] key The unsigned integer key of the matrix to retrieve
> > + * \return The matrix corresponding to the color temperature
> > + */
> > +
> > +} /* namespace ipa */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/libipa/matrix_interpolator.h b/src/ipa/libipa/matrix_interpolator.h
> > new file mode 100644
> > index 000000000..27ab532eb
> > --- /dev/null
> > +++ b/src/ipa/libipa/matrix_interpolator.h
> > @@ -0,0 +1,131 @@
> > +/* SPDX-License-Identifier: BSD-2-Clause */
> > +/*
> > + * Copyright (C) 2019, Raspberry Pi Ltd
> > + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
> > + *
> > + * Helper class for interpolating maps of matrices
> > + */
> > +
> > +#pragma once
> > +
> > +#include <algorithm>
> > +#include <map>
> > +#include <string>
> > +#include <tuple>
> > +
> > +#include <libcamera/base/log.h>
> > +
> > +#include "libcamera/internal/yaml_parser.h"
> > +
> > +#include "matrix.h"
> > +
> > +namespace libcamera {
> > +
> > +LOG_DECLARE_CATEGORY(MatrixInterpolator)
> > +
> > +namespace ipa {
> > +
> > +#ifndef __DOXYGEN__
> > +template<typename T, unsigned int R, unsigned int C,
> > +        std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>
> > +#else
> > +template<typename T, unsigned int R, unsigned int C>
> > +#endif /* __DOXYGEN__ */
> > +class MatrixInterpolator
> > +{
> > +public:
> > +       MatrixInterpolator()
> > +       {
> > +               reset();
> > +       }
> > +
> > +       MatrixInterpolator(const std::map<unsigned int, Matrix<T, R, C>> &matrices)
> > +       {
> > +               for (const auto &pair : matrices)
> > +                       matrices_[pair.first] = pair.second;
> > +       }
> > +
> > +       ~MatrixInterpolator() {}
> > +
> > +       void reset()
> > +       {
> > +               Matrix<T, R, C> identity;
> > +               matrices_.clear();
> > +               matrices_[0] = identity;
> > +       }
> > +
> > +       int readYaml(const libcamera::YamlObject &yaml,
> > +                    const std::string &key_name,
> > +                    const std::string &matrix_name)
> > +       {
> > +               int ret;
> > +
> > +               matrices_.clear();
> > +
> > +               if (!yaml.isList()) {
> > +                       LOG(MatrixInterpolator, Error) << "yaml object must be a list";
> > +                       return -EINVAL;
> > +               }
> > +
> > +               for (const auto &value : yaml.asList()) {
> > +                       unsigned int ct = std::stoul(value[key_name].get<std::string>(""));
> > +                       Matrix<T, R, C> matrix;
> > +                       if ((ret = matrix.readYaml(value[matrix_name])) < 0) {
> > +                               LOG(MatrixInterpolator, Error) << "Failed to read matrix";
> > +                               return ret;
> > +                       }
> > +
> > +                       matrices_[ct] = matrix;
> > +
> > +                       LOG(MatrixInterpolator, Debug)
> > +                               << "Read matrix '" << matrix_name << "' for key '"
> > +                               << key_name << "' " << ct << ": "
> > +                               << matrices_[ct].toString();
> > +               }
> > +
> > +               if (matrices_.size() < 1) {
> > +                       LOG(MatrixInterpolator, Error) << "Need at least one matrix";
> > +                       return -EINVAL;
> > +               }
> > +
> > +               return 0;
> > +       }
> > +
> > +       Matrix<T, R, C> get(unsigned int ct)
> > +       {
> > +               ASSERT(matrices_.size() > 0);
> > +
> > +               if (matrices_.size() == 1 ||
> > +                   ct <= matrices_.begin()->first)
> > +                       return matrices_.begin()->second;
> > +
> > +               if (ct >= matrices_.rbegin()->first)
> > +                       return matrices_.rbegin()->second;
> > +
> > +               if (matrices_.find(ct) != matrices_.end())
> > +                       return matrices_[ct];
> > +
> > +               if (cache_.find(ct) != cache_.end())
> > +                       return cache_[ct];
> > +
> > +               /* The above four guarantee that this will succeed */
> > +               auto iter = matrices_.upper_bound(ct);
> > +               unsigned int ctUpper = iter->first;
> > +               unsigned int ctLower = (--iter)->first;
> > +
> > +               double lambda = (ct - ctLower) / static_cast<double>(ctUpper - ctLower);
> > +               Matrix<T, R, C> ret =
> > +                       lambda * matrices_[ctUpper] + (1.0 - lambda) * matrices_[ctLower];
> > +               cache_[ct] = ret;
> 
> Yikes. I like this addition but I'm a little bit worried here.
> 
> Imagine some changing light scene causes us to generate 'every' possible
> light colour temperature. This would consume a potentially large amount
> of memory. Particularly if this was interpolating LSC tables.
> 
> The cache_ needs to be limited in size ideally as some form of LRU.
> 
> I assumed we'd just keep a single cached Matrix originally (which I
> guess is an LRU of size 1...) , but if it's worth adding multiple
> then I think it needs some limits already.
> 
> 
> As I asked for the cache, I don't mind if it's removed if that's easier
> for the short term and added in on top with an LRU, or changed to store
> only a single cached ct, or an LRU is implemented.

Alright I'll remove it for now.

> 
> 
> Otherwise, everything else looks fine to me.


Thanks,

Paul

> 
> 
> 
> > +               return ret;
> > +       }
> > +
> > +private:
> > +       std::map<unsigned int, Matrix<T, R, C>> matrices_;
> > +
> > +       std::map<unsigned int, Matrix<T, R, C>> cache_;
> > +};
> > +
> > +} /* namespace ipa */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> > index 2547a5b83..901d78549 100644
> > --- a/src/ipa/libipa/meson.build
> > +++ b/src/ipa/libipa/meson.build
> > @@ -8,6 +8,7 @@ libipa_headers = files([
> >      'fc_queue.h',
> >      'histogram.h',
> >      'matrix.h',
> > +    'matrix_interpolator.h',
> >      'module.h',
> >      'pwl.h',
> >  ])
> > @@ -20,6 +21,7 @@ libipa_sources = files([
> >      'fc_queue.cpp',
> >      'histogram.cpp',
> >      'matrix.cpp',
> > +    'matrix_interpolator.cpp',
> >      'module.cpp',
> >      'pwl.cpp',
> >  ])
> > -- 
> > 2.39.2
> >