[v2,1/3] ipa: libipa: Add Matrix class
diff mbox series

Message ID 20240405084050.1919105-2-paul.elder@ideasonboard.com
State Superseded
Headers show
Series
  • ipa: rkisp1: Add crosstalk algorithm
Related show

Commit Message

Paul Elder April 5, 2024, 8:40 a.m. UTC
Add a class to represent a Matrix object and operations for adding
matrices, multipling a matrix by a scalar, and multiplying two matrices.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
Changes in v2:
- clean up unused includes
- avoid dereferencing an absent std::optional
---
 src/ipa/libipa/matrix.cpp  |  17 +++++
 src/ipa/libipa/matrix.h    | 136 +++++++++++++++++++++++++++++++++++++
 src/ipa/libipa/meson.build |   2 +
 3 files changed, 155 insertions(+)
 create mode 100644 src/ipa/libipa/matrix.cpp
 create mode 100644 src/ipa/libipa/matrix.h

Comments

Stefan Klug April 10, 2024, 2:55 p.m. UTC | #1
Hi Paul,

thank you for the patch. I know this is only a utility class so I might
be a bit too picky.

On Fri, Apr 05, 2024 at 05:40:48PM +0900, Paul Elder wrote:
> Add a class to represent a Matrix object and operations for adding
> matrices, multipling a matrix by a scalar, and multiplying two matrices.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
> Changes in v2:
> - clean up unused includes
> - avoid dereferencing an absent std::optional
> ---
>  src/ipa/libipa/matrix.cpp  |  17 +++++
>  src/ipa/libipa/matrix.h    | 136 +++++++++++++++++++++++++++++++++++++
>  src/ipa/libipa/meson.build |   2 +
>  3 files changed, 155 insertions(+)
>  create mode 100644 src/ipa/libipa/matrix.cpp
>  create mode 100644 src/ipa/libipa/matrix.h
> 
> diff --git a/src/ipa/libipa/matrix.cpp b/src/ipa/libipa/matrix.cpp
> new file mode 100644
> index 00000000..c222c3f0
> --- /dev/null
> +++ b/src/ipa/libipa/matrix.cpp
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2019, Raspberry Pi Ltd
> + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
> + *
> + * matrix.cpp - Matrix and related operations
> + */
> +
> +#include "matrix.h"
> +
> +#include <libcamera/base/log.h>
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(Matrix)
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/matrix.h b/src/ipa/libipa/matrix.h
> new file mode 100644
> index 00000000..f5520260
> --- /dev/null
> +++ b/src/ipa/libipa/matrix.h
> @@ -0,0 +1,136 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2019, Raspberry Pi Ltd
> + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
> + *
> + * matrix.cpp - Matrix and related operations
> + */
> +#pragma once
> +
> +#include <cmath>
> +#include <sstream>
> +#include <vector>
> +
> +#include <libcamera/base/log.h>
> +
> +#include "libcamera/internal/yaml_parser.h"
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(Matrix)
> +
> +namespace ipa {
> +
> +template<typename T>
> +struct Matrix {
> +public:
> +	Matrix(){};

I would expect to get a unit matrix after construction. Ahh it doesn't
have a size yet. Then rows and cols should be properly initialized.

> +	~Matrix(){};
> +
> +	Matrix(const unsigned int &_rows, const unsigned int &_cols,
> +	       const std::vector<T> &_data)
> +		: rows(_rows), cols(_cols)
> +	{
> +		for (const T &x : _data)
> +			data.push_back(x);

Any reason not to use data.insert(data.end(), _data.begin(),
_data.end())?
The size of _data should be checked, so that it's not too big.

> +
> +		while (rows * cols < data.size())
> +			data.push_back(0);

Is this expected? I would treat it as an error.

> +	}
> +
> +	int readYaml(unsigned int _rows, unsigned int _cols,
> +		     const libcamera::YamlObject &yaml)
> +	{

Why are the arguments to Matrix() passed by reference and these by
value? They are equally const.

> +		rows = _rows;
> +		cols = _cols;
> +
> +		if (yaml.size() != rows * cols) {
> +			LOG(Matrix, Error)
> +				<< "Wrong number of values in matrix: expected "
> +				<< rows * cols << ", got " << yaml.size();
> +			return -EINVAL;
> +		}
> +
> +		for (const auto &x : yaml.asList()) {
> +			auto value = x.get<T>();
> +			if (!value) {
> +				LOG(Matrix, Error) << "Failed to read matrix value";
> +				return -EINVAL;
> +			}
> +			data.push_back(*value);

If I get it right, this doesn't work when the matrix already had a size
and data. That should work in all cases. It feels strange that a matrix
suddenly changes size. Would it make sense to move that into a static
factory function?

> +		}
> +
> +		return 0;
> +	}
> +
> +	const std::string toString() const
> +	{
> +		std::stringstream ss;
> +		ss << "Matrix { ";
> +		for (unsigned int i = 0; i < rows; i++) {
> +			ss << "[ ";
> +			for (unsigned int j = 0; j < cols; j++) {
> +				ss << data[i * cols + j];
> +				ss << ((j + 1 < cols) ? ", " : " ");
> +			}
> +			ss << ((i + 1 < rows) ? "], " : "]");
> +		}
> +		ss << " }";
> +
> +		return ss.str();
> +	}
> +
> +	const T &operator[](unsigned int i) const { return data.at(i); }
> +
> +	unsigned int rows;
> +	unsigned int cols;
> +	std::vector<T> data;

I think these should end with a underscore. And they should be
protected.

> +};
> +
> +template<typename T>
> +Matrix<T> operator*(T d, const Matrix<T> &m)
> +{
> +	std::vector<T> result(m.data.size());
> +
> +	for (const T &x : m.data)
> +		result.push_back(d * x);
> +
> +	return Matrix(m.rows, m.cols, result);

Why the temporary vector? I somehow expected something
like (pseudocode):

Matrix r(m.rows, m.cols);

for( int i=0; i<m.data.size(); i++)
	r[i] = m[i]*d;

return r;

That might not be more efficient, but to me it is easier to read.

> +}
> +
> +template<typename T>
> +Matrix<T> operator*(const Matrix<T> &m1, const Matrix<T> &m2)
> +{
> +	ASSERT(m1.cols == m2.rows);
> +
> +	std::vector<T> result(m1.rows * m2.cols);
> +
> +	for (unsigned int i = 0; i < m1.rows; i++) {
> +		T sum = 0;
> +
> +		for (unsigned int j = 0; j < m2.cols; j++)
> +			sum += m1[i * m1.cols + j] * m2[j * m2.cols + i];

Reading that makes me think, that it would be nice to be able to do
write 
sum += m1[j][i] * m2[i][j] 

But that might be too much for a utility class. What about an accessor
get(r,c) to remove the need to write down the calculation over and over
again?
sum += m1.get(i,j) + m2.get(j,i)

Regards,
Stefan

> +
> +		result.push_back(sum);
> +	}
> +
> +	return Matrix<T>(m1.rows, m2.cols, result);
> +}
> +
> +template<typename T>
> +Matrix<T> operator+(const Matrix<T> &m1, const Matrix<T> &m2)
> +{
> +	ASSERT(m1.cols == m2.cols && m1.rows == m2.rows);
> +
> +	unsigned int len = m1.rows * m1.cols;
> +	std::vector<T> result(len);
> +
> +	for (unsigned int i = 0; i < len; i++)
> +		result[i] = m1[i] + m2[i];
> +
> +	return Matrix<T>(m1.rows, m1.cols, result);
> +}
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> index 817a09cf..5d5ba5e5 100644
> --- a/src/ipa/libipa/meson.build
> +++ b/src/ipa/libipa/meson.build
> @@ -6,6 +6,7 @@ libipa_headers = files([
>      'exposure_mode_helper.h',
>      'fc_queue.h',
>      'histogram.h',
> +    'matrix.h',
>      'module.h',
>      'pwl.h',
>  ])
> @@ -16,6 +17,7 @@ libipa_sources = files([
>      'exposure_mode_helper.cpp',
>      'fc_queue.cpp',
>      'histogram.cpp',
> +    'matrix.cpp',
>      'module.cpp',
>      'pwl.cpp'
>  ])
> -- 
> 2.39.2
>

Patch
diff mbox series

diff --git a/src/ipa/libipa/matrix.cpp b/src/ipa/libipa/matrix.cpp
new file mode 100644
index 00000000..c222c3f0
--- /dev/null
+++ b/src/ipa/libipa/matrix.cpp
@@ -0,0 +1,17 @@ 
+/* SPDX-License-Identifier: BSD-2-Clause */
+/*
+ * Copyright (C) 2019, Raspberry Pi Ltd
+ * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
+ *
+ * matrix.cpp - Matrix and related operations
+ */
+
+#include "matrix.h"
+
+#include <libcamera/base/log.h>
+
+namespace libcamera {
+
+LOG_DEFINE_CATEGORY(Matrix)
+
+} /* namespace libcamera */
diff --git a/src/ipa/libipa/matrix.h b/src/ipa/libipa/matrix.h
new file mode 100644
index 00000000..f5520260
--- /dev/null
+++ b/src/ipa/libipa/matrix.h
@@ -0,0 +1,136 @@ 
+/* SPDX-License-Identifier: BSD-2-Clause */
+/*
+ * Copyright (C) 2019, Raspberry Pi Ltd
+ * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
+ *
+ * matrix.cpp - Matrix and related operations
+ */
+#pragma once
+
+#include <cmath>
+#include <sstream>
+#include <vector>
+
+#include <libcamera/base/log.h>
+
+#include "libcamera/internal/yaml_parser.h"
+
+namespace libcamera {
+
+LOG_DECLARE_CATEGORY(Matrix)
+
+namespace ipa {
+
+template<typename T>
+struct Matrix {
+public:
+	Matrix(){};
+	~Matrix(){};
+
+	Matrix(const unsigned int &_rows, const unsigned int &_cols,
+	       const std::vector<T> &_data)
+		: rows(_rows), cols(_cols)
+	{
+		for (const T &x : _data)
+			data.push_back(x);
+
+		while (rows * cols < data.size())
+			data.push_back(0);
+	}
+
+	int readYaml(unsigned int _rows, unsigned int _cols,
+		     const libcamera::YamlObject &yaml)
+	{
+		rows = _rows;
+		cols = _cols;
+
+		if (yaml.size() != rows * cols) {
+			LOG(Matrix, Error)
+				<< "Wrong number of values in matrix: expected "
+				<< rows * cols << ", got " << yaml.size();
+			return -EINVAL;
+		}
+
+		for (const auto &x : yaml.asList()) {
+			auto value = x.get<T>();
+			if (!value) {
+				LOG(Matrix, Error) << "Failed to read matrix value";
+				return -EINVAL;
+			}
+			data.push_back(*value);
+		}
+
+		return 0;
+	}
+
+	const std::string toString() const
+	{
+		std::stringstream ss;
+		ss << "Matrix { ";
+		for (unsigned int i = 0; i < rows; i++) {
+			ss << "[ ";
+			for (unsigned int j = 0; j < cols; j++) {
+				ss << data[i * cols + j];
+				ss << ((j + 1 < cols) ? ", " : " ");
+			}
+			ss << ((i + 1 < rows) ? "], " : "]");
+		}
+		ss << " }";
+
+		return ss.str();
+	}
+
+	const T &operator[](unsigned int i) const { return data.at(i); }
+
+	unsigned int rows;
+	unsigned int cols;
+	std::vector<T> data;
+};
+
+template<typename T>
+Matrix<T> operator*(T d, const Matrix<T> &m)
+{
+	std::vector<T> result(m.data.size());
+
+	for (const T &x : m.data)
+		result.push_back(d * x);
+
+	return Matrix(m.rows, m.cols, result);
+}
+
+template<typename T>
+Matrix<T> operator*(const Matrix<T> &m1, const Matrix<T> &m2)
+{
+	ASSERT(m1.cols == m2.rows);
+
+	std::vector<T> result(m1.rows * m2.cols);
+
+	for (unsigned int i = 0; i < m1.rows; i++) {
+		T sum = 0;
+
+		for (unsigned int j = 0; j < m2.cols; j++)
+			sum += m1[i * m1.cols + j] * m2[j * m2.cols + i];
+
+		result.push_back(sum);
+	}
+
+	return Matrix<T>(m1.rows, m2.cols, result);
+}
+
+template<typename T>
+Matrix<T> operator+(const Matrix<T> &m1, const Matrix<T> &m2)
+{
+	ASSERT(m1.cols == m2.cols && m1.rows == m2.rows);
+
+	unsigned int len = m1.rows * m1.cols;
+	std::vector<T> result(len);
+
+	for (unsigned int i = 0; i < len; i++)
+		result[i] = m1[i] + m2[i];
+
+	return Matrix<T>(m1.rows, m1.cols, result);
+}
+
+} /* namespace ipa */
+
+} /* namespace libcamera */
diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
index 817a09cf..5d5ba5e5 100644
--- a/src/ipa/libipa/meson.build
+++ b/src/ipa/libipa/meson.build
@@ -6,6 +6,7 @@  libipa_headers = files([
     'exposure_mode_helper.h',
     'fc_queue.h',
     'histogram.h',
+    'matrix.h',
     'module.h',
     'pwl.h',
 ])
@@ -16,6 +17,7 @@  libipa_sources = files([
     'exposure_mode_helper.cpp',
     'fc_queue.cpp',
     'histogram.cpp',
+    'matrix.cpp',
     'module.cpp',
     'pwl.cpp'
 ])