ipa: libipa: Add Matrix class
diff mbox series

Message ID 20240327085933.281180-1-paul.elder@ideasonboard.com
State New
Headers show
Series
  • ipa: libipa: Add Matrix class
Related show

Commit Message

Paul Elder March 27, 2024, 8:59 a.m. UTC 
Add a class to represent a Matrix object and its corresponding
operations.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 src/ipa/libipa/matrix.cpp  |  17 +++++
 src/ipa/libipa/matrix.h    | 138 +++++++++++++++++++++++++++++++++++++
 src/ipa/libipa/meson.build |   2 +
 3 files changed, 157 insertions(+)
 create mode 100644 src/ipa/libipa/matrix.cpp
 create mode 100644 src/ipa/libipa/matrix.h

Comments

Umang Jain March 28, 2024, 6:44 p.m. UTC | #1 
Hi Paul,

Thank you for the patch.

On 27/03/24 2:29 pm, Paul Elder wrote:
> Add a class to represent a Matrix object and its corresponding
> operations.

I would just expand a bit on which operations are supported:

- Matrix addition
- Matrix multiplication by  scaler constant
- Matrix-to-Matrix Multiplication
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>   src/ipa/libipa/matrix.cpp  |  17 +++++
>   src/ipa/libipa/matrix.h    | 138 +++++++++++++++++++++++++++++++++++++
>   src/ipa/libipa/meson.build |   2 +
>   3 files changed, 157 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..1fd191c7
> --- /dev/null
> +++ b/src/ipa/libipa/matrix.h
> @@ -0,0 +1,138 @@
> +/* 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 <algorithm>

Is this header being used?
> +#include <cmath>
> +#include <sstream>
> +#include <tuple>

Same question here
> +#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 " << *value;

  This doesn't seem right, are you de-referencing null 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'
>   ])
Paul Elder March 29, 2024, 10:15 a.m. UTC | #2 
Hi Umang,

Thanks for the review.

On Fri, Mar 29, 2024 at 12:14:27AM +0530, Umang Jain wrote:
> Hi Paul,
> 
> Thank you for the patch.
> 
> On 27/03/24 2:29 pm, Paul Elder wrote:
> > Add a class to represent a Matrix object and its corresponding
> > operations.
> 
> I would just expand a bit on which operations are supported:
> 
> - Matrix addition
> - Matrix multiplication by  scaler constant
> - Matrix-to-Matrix Multiplication
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > ---
> >   src/ipa/libipa/matrix.cpp  |  17 +++++
> >   src/ipa/libipa/matrix.h    | 138 +++++++++++++++++++++++++++++++++++++
> >   src/ipa/libipa/meson.build |   2 +
> >   3 files changed, 157 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..1fd191c7
> > --- /dev/null
> > +++ b/src/ipa/libipa/matrix.h
> > @@ -0,0 +1,138 @@
> > +/* 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 <algorithm>
> 
> Is this header being used?
> > +#include <cmath>
> > +#include <sstream>
> > +#include <tuple>
> 
> Same question here

Oops, I forgot to remove these when I stopped using them.

> > +#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 " << *value;
> 
>  This doesn't seem right, are you de-referencing null value  ?

I don't think it's null (it's a std::optional) but still yeah it's not
right.


Thanks,

Paul

> > +				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'
> >   ])
>

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..1fd191c7
--- /dev/null
+++ b/src/ipa/libipa/matrix.h
@@ -0,0 +1,138 @@ 
+/* 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 <algorithm>
+#include <cmath>
+#include <sstream>
+#include <tuple>
+#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 " << *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'
 ])