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

20005 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.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
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 |  54 ++++++++++++
 src/ipa/libipa/matrix_interpolator.h   | 116 +++++++++++++++++++++++++
 src/ipa/libipa/meson.build             |   2 +
 3 files changed, 172 insertions(+)
 create mode 100644 src/ipa/libipa/matrix_interpolator.cpp
 create mode 100644 src/ipa/libipa/matrix_interpolator.h

On Tue, May 07, 2024 at 03:10:15PM +0900, Paul Elder wrote:
> 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.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> 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 |  54 ++++++++++++
>  src/ipa/libipa/matrix_interpolator.h   | 116 +++++++++++++++++++++++++
>  src/ipa/libipa/meson.build             |   2 +
>  3 files changed, 172 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 00000000..fc1837e7
> --- /dev/null
> +++ b/src/ipa/libipa/matrix_interpolator.cpp
> @@ -0,0 +1,54 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2019, Raspberry Pi Ltd
> + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
> + *
> + * matrix_interpolator.cpp - 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 ccm.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
> + */
> +
> +/**
> + * \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
> + * \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 00000000..2c6263cb
> --- /dev/null
> +++ b/src/ipa/libipa/matrix_interpolator.h
> @@ -0,0 +1,116 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2019, Raspberry Pi Ltd
> + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
> + *
> + * matrix_interpolator.h - 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 {
> +
> +template<typename T, unsigned int R, unsigned int C,
> +	 std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>
> +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)
> +	{
> +		int ret;
> +
> +		matrices_.clear();
> +
> +		if (!yaml.isDictionary()) {
> +			LOG(MatrixInterpolator, Error) << "yaml object must be a dictionary";
> +			return -EINVAL;
> +		}
> +

I have the feeling that this might not be flexible enough. At the moment
this expects a yaml representation of

2000: [ matrix values... ]
3000: [ ... ]

We might need to add more infos for a given color temperature. In my
prototypes I used a list like this:

- ct: 2000
  ccm: [matrix values]
- ct: 3000
  ccm: [ matrix values ]


This adds the flexibility to e.g. add the offset vector.
My diff is:
-	int readYaml(const libcamera::YamlObject &yaml)
+	int readYaml(const libcamera::YamlObject &yaml, const std::string& key_property, const std::string& value_property)
 	{
 		int ret;
		
		matrices_.clear(); 

-		if (!yaml.isDictionary()) {
-			LOG(MatrixInterpolator, Error) << "yaml object must be a dictionary";
+		if (!yaml.isList()) {
+			LOG(MatrixInterpolator, Error) << "yaml object must be a list";
 			return -EINVAL;
 		}
 
-		for (const auto &[ctStr, value] : yaml.asDict()) {
-			unsigned int ct = std::stoul(ctStr);
+		for (const auto &value : yaml.asList()) {
+			unsigned int ct = std::stoul(value[key_property].get<std::string>(""));
 			Matrix<T, R, C> matrix;
-			if ((ret = matrix.readYaml(value)) < 0) {
+			if ((ret = matrix.readYaml(value[value_property])) < 0) {
 				LOG(MatrixInterpolator, Error) << "Failed to read matrix";
 				return ret;
 			}
 


I'm not yet sure where we will end up. This largely depends on how the
tuning file format evolves. What are your thoughts about that?

> +		for (const auto &[ctStr, value] : yaml.asDict()) {
> +			unsigned int ct = std::stoul(ctStr);
> +			Matrix<T, R, C> matrix;
> +			if ((ret = matrix.readYaml(value)) < 0) {
> +				LOG(MatrixInterpolator, Error) << "Failed to read matrix";
> +				return ret;
> +			}
> +
> +			matrices_[ct] = matrix;
> +
> +			LOG(MatrixInterpolator, Debug)
> +				<< "Read matrix for key " << 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);

I'm a bit concerned here. This check only runs in debug mode. But in
case the readYaml gets an invalid object, it leaves the interpolator
with matrices_.size() == 0 and we would crash here. I think we should at
least catch that in release mode and maybe return a identity matrix (and
then there is need to inject the identity in the constructor anymore)

Cheers,
Stefan

> +
> +		if (matrices_.size() == 1 ||
> +		    ct <= matrices_.begin()->first)
> +			return matrices_.begin()->second;
> +
> +		if (ct >= matrices_.rbegin()->first)
> +			return matrices_.rbegin()->second;
> +
> +		if (matrices_.count(ct))
> +			return matrices_[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);
> +		return lambda * matrices_[ctUpper] + (1.0 - lambda) * matrices_[ctLower];
> +	}
> +
> +private:
> +	std::map<unsigned int, Matrix<T, R, C>> matrices_;
> +};
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> index 1e34355f..305acf64 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 Wed, May 15, 2024 at 11:20:56AM +0200, Stefan Klug wrote:
> On Tue, May 07, 2024 at 03:10:15PM +0900, Paul Elder wrote:
> > 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.
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > 
> > ---
> > 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 |  54 ++++++++++++
> >  src/ipa/libipa/matrix_interpolator.h   | 116 +++++++++++++++++++++++++
> >  src/ipa/libipa/meson.build             |   2 +
> >  3 files changed, 172 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 00000000..fc1837e7
> > --- /dev/null
> > +++ b/src/ipa/libipa/matrix_interpolator.cpp
> > @@ -0,0 +1,54 @@
> > +/* SPDX-License-Identifier: BSD-2-Clause */
> > +/*
> > + * Copyright (C) 2019, Raspberry Pi Ltd
> > + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
> > + *
> > + * matrix_interpolator.cpp - 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 ccm.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
> > + */
> > +
> > +/**
> > + * \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
> > + * \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 00000000..2c6263cb
> > --- /dev/null
> > +++ b/src/ipa/libipa/matrix_interpolator.h
> > @@ -0,0 +1,116 @@
> > +/* SPDX-License-Identifier: BSD-2-Clause */
> > +/*
> > + * Copyright (C) 2019, Raspberry Pi Ltd
> > + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
> > + *
> > + * matrix_interpolator.h - 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 {
> > +
> > +template<typename T, unsigned int R, unsigned int C,
> > +	 std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>
> > +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)
> > +	{
> > +		int ret;
> > +
> > +		matrices_.clear();
> > +
> > +		if (!yaml.isDictionary()) {
> > +			LOG(MatrixInterpolator, Error) << "yaml object must be a dictionary";
> > +			return -EINVAL;
> > +		}
> > +
> 
> I have the feeling that this might not be flexible enough. At the moment
> this expects a yaml representation of
> 
> 2000: [ matrix values... ]
> 3000: [ ... ]
> 
> We might need to add more infos for a given color temperature. In my
> prototypes I used a list like this:
> 
> - ct: 2000
>   ccm: [matrix values]
> - ct: 3000
>   ccm: [ matrix values ]

Yes, I think that will be necessary, also to match the layout with lsc
so that we can use this for lsc in the future.

> 
> 
> This adds the flexibility to e.g. add the offset vector.
> My diff is:
> -	int readYaml(const libcamera::YamlObject &yaml)
> +	int readYaml(const libcamera::YamlObject &yaml, const std::string& key_property, const std::string& value_property)

Ah, good idea. I was trying to figure out how to support different size
matrices in the same map...

>  	{
>  		int ret;
> 		
> 		matrices_.clear(); 
> 
> -		if (!yaml.isDictionary()) {
> -			LOG(MatrixInterpolator, Error) << "yaml object must be a dictionary";
> +		if (!yaml.isList()) {
> +			LOG(MatrixInterpolator, Error) << "yaml object must be a list";
>  			return -EINVAL;
>  		}
>  
> -		for (const auto &[ctStr, value] : yaml.asDict()) {
> -			unsigned int ct = std::stoul(ctStr);
> +		for (const auto &value : yaml.asList()) {
> +			unsigned int ct = std::stoul(value[key_property].get<std::string>(""));
>  			Matrix<T, R, C> matrix;
> -			if ((ret = matrix.readYaml(value)) < 0) {
> +			if ((ret = matrix.readYaml(value[value_property])) < 0) {
>  				LOG(MatrixInterpolator, Error) << "Failed to read matrix";
>  				return ret;
>  			}
>  
> 
> 
> I'm not yet sure where we will end up. This largely depends on how the
> tuning file format evolves. What are your thoughts about that?
> 
> > +		for (const auto &[ctStr, value] : yaml.asDict()) {
> > +			unsigned int ct = std::stoul(ctStr);
> > +			Matrix<T, R, C> matrix;
> > +			if ((ret = matrix.readYaml(value)) < 0) {
> > +				LOG(MatrixInterpolator, Error) << "Failed to read matrix";
> > +				return ret;
> > +			}
> > +
> > +			matrices_[ct] = matrix;
> > +
> > +			LOG(MatrixInterpolator, Debug)
> > +				<< "Read matrix for key " << 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);
> 
> I'm a bit concerned here. This check only runs in debug mode. But in
> case the readYaml gets an invalid object, it leaves the interpolator
> with matrices_.size() == 0 and we would crash here. I think we should at
> least catch that in release mode and maybe return a identity matrix (and
> then there is need to inject the identity in the constructor anymore)

tbh I think it should be fine because it'll be caught in readYaml and we
return -EINVAL. Is that not sufficient?


Thanks,

Paul

> 
> > +
> > +		if (matrices_.size() == 1 ||
> > +		    ct <= matrices_.begin()->first)
> > +			return matrices_.begin()->second;
> > +
> > +		if (ct >= matrices_.rbegin()->first)
> > +			return matrices_.rbegin()->second;
> > +
> > +		if (matrices_.count(ct))
> > +			return matrices_[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);
> > +		return lambda * matrices_[ctUpper] + (1.0 - lambda) * matrices_[ctLower];
> > +	}
> > +
> > +private:
> > +	std::map<unsigned int, Matrix<T, R, C>> matrices_;
> > +};
> > +
> > +} /* namespace ipa */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> > index 1e34355f..305acf64 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
> >

Hi Paul,

On Fri, May 17, 2024 at 12:50:50PM +0900, Paul Elder wrote:
> On Wed, May 15, 2024 at 11:20:56AM +0200, Stefan Klug wrote:
> > On Tue, May 07, 2024 at 03:10:15PM +0900, Paul Elder wrote:
> > > 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.
> > > 
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > 
> > > ---
> > > 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 |  54 ++++++++++++
> > >  src/ipa/libipa/matrix_interpolator.h   | 116 +++++++++++++++++++++++++
> > >  src/ipa/libipa/meson.build             |   2 +
> > >  3 files changed, 172 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 00000000..fc1837e7
> > > --- /dev/null
> > > +++ b/src/ipa/libipa/matrix_interpolator.cpp
> > > @@ -0,0 +1,54 @@
> > > +/* SPDX-License-Identifier: BSD-2-Clause */
> > > +/*
> > > + * Copyright (C) 2019, Raspberry Pi Ltd
> > > + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
> > > + *
> > > + * matrix_interpolator.cpp - 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 ccm.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
> > > + */
> > > +
> > > +/**
> > > + * \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
> > > + * \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 00000000..2c6263cb
> > > --- /dev/null
> > > +++ b/src/ipa/libipa/matrix_interpolator.h
> > > @@ -0,0 +1,116 @@
> > > +/* SPDX-License-Identifier: BSD-2-Clause */
> > > +/*
> > > + * Copyright (C) 2019, Raspberry Pi Ltd
> > > + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
> > > + *
> > > + * matrix_interpolator.h - 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 {
> > > +
> > > +template<typename T, unsigned int R, unsigned int C,
> > > +	 std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>
> > > +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)
> > > +	{
> > > +		int ret;
> > > +
> > > +		matrices_.clear();
> > > +
> > > +		if (!yaml.isDictionary()) {
> > > +			LOG(MatrixInterpolator, Error) << "yaml object must be a dictionary";
> > > +			return -EINVAL;
> > > +		}
> > > +
> > 
> > I have the feeling that this might not be flexible enough. At the moment
> > this expects a yaml representation of
> > 
> > 2000: [ matrix values... ]
> > 3000: [ ... ]
> > 
> > We might need to add more infos for a given color temperature. In my
> > prototypes I used a list like this:
> > 
> > - ct: 2000
> >   ccm: [matrix values]
> > - ct: 3000
> >   ccm: [ matrix values ]
> 
> Yes, I think that will be necessary, also to match the layout with lsc
> so that we can use this for lsc in the future.
> 
> > 
> > 
> > This adds the flexibility to e.g. add the offset vector.
> > My diff is:
> > -	int readYaml(const libcamera::YamlObject &yaml)
> > +	int readYaml(const libcamera::YamlObject &yaml, const std::string& key_property, const std::string& value_property)
> 
> Ah, good idea. I was trying to figure out how to support different size
> matrices in the same map...
> 
> >  	{
> >  		int ret;
> > 		
> > 		matrices_.clear(); 
> > 
> > -		if (!yaml.isDictionary()) {
> > -			LOG(MatrixInterpolator, Error) << "yaml object must be a dictionary";
> > +		if (!yaml.isList()) {
> > +			LOG(MatrixInterpolator, Error) << "yaml object must be a list";
> >  			return -EINVAL;
> >  		}
> >  
> > -		for (const auto &[ctStr, value] : yaml.asDict()) {
> > -			unsigned int ct = std::stoul(ctStr);
> > +		for (const auto &value : yaml.asList()) {
> > +			unsigned int ct = std::stoul(value[key_property].get<std::string>(""));
> >  			Matrix<T, R, C> matrix;
> > -			if ((ret = matrix.readYaml(value)) < 0) {
> > +			if ((ret = matrix.readYaml(value[value_property])) < 0) {
> >  				LOG(MatrixInterpolator, Error) << "Failed to read matrix";
> >  				return ret;
> >  			}
> >  
> > 
> > 
> > I'm not yet sure where we will end up. This largely depends on how the
> > tuning file format evolves. What are your thoughts about that?
> > 
> > > +		for (const auto &[ctStr, value] : yaml.asDict()) {
> > > +			unsigned int ct = std::stoul(ctStr);
> > > +			Matrix<T, R, C> matrix;
> > > +			if ((ret = matrix.readYaml(value)) < 0) {
> > > +				LOG(MatrixInterpolator, Error) << "Failed to read matrix";
> > > +				return ret;
> > > +			}
> > > +
> > > +			matrices_[ct] = matrix;
> > > +
> > > +			LOG(MatrixInterpolator, Debug)
> > > +				<< "Read matrix for key " << 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);
> > 
> > I'm a bit concerned here. This check only runs in debug mode. But in
> > case the readYaml gets an invalid object, it leaves the interpolator
> > with matrices_.size() == 0 and we would crash here. I think we should at
> > least catch that in release mode and maybe return a identity matrix (and
> > then there is need to inject the identity in the constructor anymore)
> 
> tbh I think it should be fine because it'll be caught in readYaml and we
> return -EINVAL. Is that not sufficient?

Yes, that's right. The error case should be handled in the layers above.
And after the -EINVAL this function *should* not be called. So I think
it's just personal preference (personally I would throw here, which is
equally drastic as getting a possible segfault but more deterministic :-). 
So it's ok for me.

Cheers,
Stefan

> 
> 
> Thanks,
> 
> Paul
> 
> > 
> > > +
> > > +		if (matrices_.size() == 1 ||
> > > +		    ct <= matrices_.begin()->first)
> > > +			return matrices_.begin()->second;
> > > +
> > > +		if (ct >= matrices_.rbegin()->first)
> > > +			return matrices_.rbegin()->second;
> > > +
> > > +		if (matrices_.count(ct))
> > > +			return matrices_[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);
> > > +		return lambda * matrices_[ctUpper] + (1.0 - lambda) * matrices_[ctLower];
> > > +	}
> > > +
> > > +private:
> > > +	std::map<unsigned int, Matrix<T, R, C>> matrices_;
> > > +};
> > > +
> > > +} /* namespace ipa */
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> > > index 1e34355f..305acf64 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
> > >