[PATCH] ipa: libipa: Add Matrix class

[Paul Elder]

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