[PATCH v2 1/3] ipa: libipa: Add Matrix class
Paul Elder
paul.elder at ideasonboard.com
Thu May 2 11:38:48 CEST 2024
Hi Stefan,
Thanks for the review.
On Mon, Apr 22, 2024 at 09:16:06PM +0200, Stefan Klug wrote:
> Hi Paul,
>
> thanks for the patch.
>
> On Fri, Apr 19, 2024 at 04:18:17PM +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 at ideasonboard.com>
> > ---
> > Changes in second v2 (v1 was a lone patch so we have two v2s of this
> > specific patch):
> > - make rows and columns into template arguments
> > - initialize to identity matrix on construction
> > - add getter and setter
> > - change from struct to class
> > - fix matrix multiplication
> >
> > Changes in v2:
> > - clean up unused includes
> > - avoid dereferencing an absent std::optional
> > ---
> > src/ipa/libipa/matrix.cpp | 17 +++++
> > src/ipa/libipa/matrix.h | 144 +++++++++++++++++++++++++++++++++++++
> > src/ipa/libipa/meson.build | 2 +
> > 3 files changed, 163 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..ec41fb57
> > --- /dev/null
> > +++ b/src/ipa/libipa/matrix.h
> > @@ -0,0 +1,144 @@
> > +/* 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>
> > +#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, unsigned int R, unsigned int C,
> > + std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>
> > +class Matrix
> > +{
> > +public:
> > + Matrix()
> > + {
> > + for (unsigned int i = 0; i < R; i++)
> > + for (unsigned int j = 0; j < C; j++)
> > + data_.push_back(static_cast<T>(i == j));
> > + };
> > +
> > + Matrix(const std::vector<T> &data)
> > + {
> > + ASSERT(data_.size() == R * C);
>
> I think you wanted to check datainstead of data_ here.
Yes.
>
> > +
> > + for (const T &x : data)
> > + data_.push_back(x);
>
> Without some reset, this will only append to data_.
> A small testcase would be nice to catch this.
This is a constructor so I don't think that would be the case...
But still I suppose better just in case? I added a clear() to this and
the above.
>
> > + }
> > +
> > + ~Matrix(){};
> > +
> > + int readYaml(const libcamera::YamlObject &yaml)
> > + {
> > + if (yaml.size() != R * C) {
> > + LOG(Matrix, Error)
> > + << "Wrong number of values in matrix: expected "
> > + << R * C << ", got " << yaml.size();
> > + return -EINVAL;
> > + }
> > +
> > + unsigned int i = 0;
> > + for (const auto &x : yaml.asList()) {
> > + auto value = x.get<T>();
> > + if (!value) {
> > + LOG(Matrix, Error) << "Failed to read matrix value";
> > + return -EINVAL;
> > + }
> > +
> > + data_[i++] = *value;
> > + }
> > +
> > + return 0;
> > + }
> > +
> > + const std::string toString() const
> > + {
> > + std::stringstream ss;
> > + ss << "Matrix { ";
> > + for (unsigned int i = 0; i < R; i++) {
> > + ss << "[ ";
> > + for (unsigned int j = 0; j < C; j++) {
> > + ss << get(i, j);
> > + ss << ((j + 1 < C) ? ", " : " ");
> > + }
> > + ss << ((i + 1 < R) ? "], " : "]");
> > + }
> > + ss << " }";
> > +
> > + return ss.str();
> > + }
> > +
> > + const T &get(unsigned int i, unsigned int j) const { return data_.at(i * C + j); }
> > + void set(unsigned int i, unsigned int j, T value) { data_[i * C + j] = value; }
> > +
> > +private:
> > + std::vector<T> data_;
> > +};
> > +
> > +template<typename T, unsigned int R, unsigned int C,
> > + std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>
> > +Matrix<T, R, C> operator*(T d, const Matrix<T, R, C> &m)
> > +{
> > + Matrix<T, R, C> result;
> > +
> > + for (unsigned int i = 0; i < R; i++)
> > + for (unsigned int j = 0; j < C; j++)
> > + result.set(i, j, d * m.get(i, j));
> > +
> > + return result;
> > +}
> > +
> > +template<typename T,
> > + unsigned int R1, unsigned int C1,
> > + unsigned int R2, unsigned int C2,
> > + std::enable_if_t<std::is_arithmetic_v<T> && C1 == R2> * = nullptr>
> > +Matrix<T, R1, C2> operator*(const Matrix<T, R1, C1> &m1, const Matrix<T, R2, C2> &m2)
> > +{
>
> Out of curiousity: Why the four template params and not three (C1 and
> R2 as a single one). I see that the enable_if serves the same purpose.
I thought it was clearer.
Paul
>
> > + Matrix<T, R1, C2> result;
> > +
> > + for (unsigned int i = 0; i < R1; i++) {
> > + for (unsigned int j = 0; j < C2; j++) {
> > + T sum = 0;
> > +
> > + for (unsigned int k = 0; k < C1; k++)
> > + sum += m1.get(i, k) * m2.get(k, j);
> > +
> > + result.set(i, j, sum);
> > + }
> > + }
> > +
> > + return result;
> > +}
> > +
> > +template<typename T, unsigned int R, unsigned int C,
> > + std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>
> > +Matrix<T, R, C> operator+(const Matrix<T, R, C> &m1, const Matrix<T, R, C> &m2)
> > +{
> > + Matrix<T, R, C> result;
> > +
> > + for (unsigned int i = 0; i < R; i++)
> > + for (unsigned int j = 0; j < C; j++)
> > + result.set(i, j, m1.get(i, j) + m2.get(i, j));
> > +
> > + return result;
> > +}
> > +
> > +} /* namespace ipa */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> > index 1b3faf8d..1e34355f 100644
> > --- a/src/ipa/libipa/meson.build
> > +++ b/src/ipa/libipa/meson.build
> > @@ -7,6 +7,7 @@ libipa_headers = files([
> > 'exposure_mode_helper.h',
> > 'fc_queue.h',
> > 'histogram.h',
> > + 'matrix.h',
> > 'module.h',
> > 'pwl.h',
> > ])
> > @@ -18,6 +19,7 @@ libipa_sources = files([
> > 'exposure_mode_helper.cpp',
> > 'fc_queue.cpp',
> > 'histogram.cpp',
> > + 'matrix.cpp',
> > 'module.cpp',
> > 'pwl.cpp'
> > ])
> > --
> > 2.39.2
> >
More information about the libcamera-devel
mailing list