[{"id":29854,"web_url":"https://patchwork.libcamera.org/comment/29854/","msgid":"<20240611233333.GE15006@pendragon.ideasonboard.com>","date":"2024-06-11T23:33:33","subject":"Re: [PATCH v7 1/3] ipa: libipa: Add Matrix class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Tue, Jun 11, 2024 at 11:02:05PM +0900, Paul Elder wrote:\n> Add a class to represent a Matrix object and operations for adding\n> matrices, multipling a matrix by a scalar, and multiplying two matrices.\n> \n> Signed-off-by: Paul Elder \n> Reviewed-by: Stefan Klug \n> Reviewed-by: Kieran Bingham \n> \n> ---\n> Changes in v7:\n> - fix copyright and license\n> \n> Changes in v6:\n> - fix doxygen\n> \n> Changes in v5:\n> - add documentation\n> \n> Changes in v4:\n> - remove stray semicolons\n> - add operator<<\n> - clean up/optimize constructor\n> - replace get() and set() with operator[] (and a second [] can be used\n> as operator[] returns a Span)\n> \n> Changes in v3:\n> - fix template parameters of operator* to allow different types for the\n> scalar multiplier and the matrix's number type\n> - clear data in constructors\n> - fix assert in constructor\n> \n> Changes v2:\n> - make rows and columns into template arguments\n> - initialize to identity matrix on construction\n> - add getter and setter\n> - change from struct to class\n> - fix matrix multiplication\n> - clean up unused includes\n> - avoid dereferencing an absent std::optional\n> ---\n> src/ipa/libipa/matrix.cpp | 123 ++++++++++++++++++++++++++\n> src/ipa/libipa/matrix.h | 172 +++++++++++++++++++++++++++++++++++++\n> src/ipa/libipa/meson.build | 2 +\n> 3 files changed, 297 insertions(+)\n> create mode 100644 src/ipa/libipa/matrix.cpp\n> create mode 100644 src/ipa/libipa/matrix.h\n> \n> diff --git a/src/ipa/libipa/matrix.cpp b/src/ipa/libipa/matrix.cpp\n> new file mode 100644\n> index 000000000000..02091a0b4ce1\n> --- /dev/null\n> +++ b/src/ipa/libipa/matrix.cpp\n> @@ -0,0 +1,123 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Paul Elder \n> + *\n> + * Matrix and related operations\n> + */\n> +\n> +#include \"matrix.h\"\n> +\n> +#include \n> +\n> +/**\n> + * \\file matrix.h\n> + * \\brief Matrix class\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(Matrix)\n> +\n> +namespace ipa {\n> +\n> +/**\n> + * \\class Matrix\n> + * \\brief Matrix class\n> + * \\tparam T Type of numerical values to be stored in the matrix\n> + * \\tparam R Number of rows in the matrix\n> + * \\tparam C Number of columns in the matrix\n> + */\n> +\n> +/**\n> + * \\fn Matrix::Matrix()\n> + * \\brief Construct an identity matrix\n\nI would have expected the default constructor to construct a zero\nmatrix. Is there a reason to go for identity instead ?\n\nAs identity matrices are useful too, I would add a static\nMatrix::identity() function that constructs and returns an identity\nmatrix.\n\n> + */\n> +\n> +/**\n> + * \\fn Matrix::Matrix(const std::vector &data)\n\nTurning the argument into a Span would allow passing any type of\ncontiguous data (vector, array, C array, ...) without requiring the\ncaller to construct a std::vector.\n\n> + * \\brief Construct matrix from supplied data\n\ns/matrix/a matrix/\n\n> + * \\param data Data from which to construct a matrix\n\n\\param[in]\n\nSame below.\n\n> + *\n> + * \\a data is a one-dimensional vector and will be turned into a matrix in\n> + * row-major order. The size of \\a data must be equal to the product of the\n> + * number of rows and columns of the matrix (RxC).\n> + */\n> +\n> +/**\n> + * \\fn Matrix::readYaml\n\ns/$/()/\n\nSame below.\n\n> + * \\brief Populate the matrix with yaml data\n> + * \\param yaml Yaml data to populate the matrix with\n> + *\n> + * Any existing data in the matrix will be overwritten. The size of the data\n> + * read from \\a yaml must be equal to the product of the number of rows and\n> + * columns of the matrix (RxC).\n> + *\n> + * The yaml data is expected to be a list with elements of type T.\n\nin row-major order.\n\n> + *\n> + * \\return 0 on success, negative error code otherwise\n> + */\n> +\n> +/**\n> + * \\fn Matrix::toString\n> + * \\brief Assemble and return a string describing the matrix\n> + * \\return A string describing the matrix\n> + */\n> +\n> +/**\n> + * \\fn Span Matrix::operator[](size_t i) const\n> + * \\brief Index to a row in the matrix\n> + * \\param i Index of row to retrieve\n> + *\n> + * This operator[] returns a Span, which can then be indexed into again with\n> + * another operator[], allowing a convenient m[i][j] to access elements of the\n> + * matrix. Note that the lifetime of the Span returned by this first-level\n> + * operator[] is bound to that of the Matrix itself, so it is not recommended\n> + * to save the Span that is the result of this operator[].\n\nThat's a clever idea, I like it.\n\nLonger term we may need a different (and\nunfortunately much more complex) implementation to allow using Matrix[i]\nin APIs that expect a Vector (which itself may need to be reimplemented\nas a matrix with a single column). And the next thing we know is that\nwe'll reimplement uBLAS or Eigen3 :-) I'll cross my fingers and wish we\nwon't reach that point.\n\nhttps://stackoverflow.com/questions/1380371/what-are-the-most-widely-used-c-vector-matrix-math-linear-algebra-libraries-a\n\n\"It seems that many projects slowly come upon a need to do matrix math,\nand fall into the trap of first building some vector classes and slowly\nadding in functionality until they get caught building a half-assed\ncustom linear algebra library, and depending on it.\"\n\n> + *\n> + * \\return Row \\a i from the matrix, as a Span\n> + */\n> +\n> +/**\n> + * \\fn Matrix::operator[](size_t i)\n> + * \\copydoc Matrix::operator[](size_t i) const\n> + */\n> +\n> +/**\n> + * \\fn Matrix::Matrix operator*(T d, const Matrix &m)\n> + * \\brief Scalar product\n> + * \\tparam T Type of the numerical scalar value\n> + * \\tparam U Type of numerical values in the matrix\n> + * \\tparam R Number of rows in the matrix\n> + * \\tparam C Number of columns in the matrix\n> + * \\param d Scalar\n> + * \\param m Matrix\n> + * \\return Product of scalar \\a d and matrix \\a m\n> + */\n> +\n> +/**\n> + * \\fn Matrix operator*(const Matrix &m1, const Matrix &m2)\n> + * \\brief Matrix multiplication\n> + * \\tparam T Type of numerical values in the matrices\n> + * \\tparam R1 Number of rows in the first matrix\n> + * \\tparam C1 Number of columns in the first matrix\n> + * \\tparam R2 Number of rows in the second matrix\n> + * \\tparam C2 Number of columns in the second matrix\n> + * \\param m1 Multiplicand matrix\n> + * \\param m2 Multiplier matrix\n> + * \\return Matrix product of matrices \\a m1 and \\a m2\n> + */\n> +\n> +/**\n> + * \\fn Matrix operator+(const Matrix &m1, const Matrix &m2)\n> + * \\brief Matrix addition\n> + * \\tparam T Type of numerical values in the matrices\n> + * \\tparam R Number of rows in the matrices\n> + * \\tparam C Number of columns in the matrices\n> + * \\param m1 Summand matrix\n> + * \\param m2 Summand matrix\n> + * \\return Matrix sum of matrices \\a m1 and \\a m2\n> + */\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/matrix.h b/src/ipa/libipa/matrix.h\n> new file mode 100644\n> index 000000000000..90eaea03bd14\n> --- /dev/null\n> +++ b/src/ipa/libipa/matrix.h\n> @@ -0,0 +1,172 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Paul Elder \n> + *\n> + * Matrix and related operations\n> + */\n> +#pragma once\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/yaml_parser.h\"\n> +\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(Matrix)\n> +\n> +namespace ipa {\n> +\n> +#ifndef __DOXYGEN__\n> +template +\t std::enable_if_t> * = nullptr>\n> +#else\n> +template\n> +#endif /* __DOXYGEN__ */\n> +class Matrix\n> +{\n> +public:\n> +\tMatrix()\n> +\t\t: data_(R * C, static_cast(false))\n\nfalse ? That's a strange one, anything wrong with 0 ?\n\n> +\t{\n> +\t\tfor (size_t i = 0; i < std::min(R, C); i++)\n> +\t\t\t(*this)[i][i] = static_cast(true);\n\nAnd 1 here.\n\n> +\t}\n> +\n> +\tMatrix(const std::vector &data)\n> +\t{\n> +\t\tASSERT(data.size() == R * C);\n> +\n> +\t\tdata_.clear();\n> +\t\tfor (const T &x : data)\n> +\t\t\tdata_.push_back(x);\n\nThat will perform quite a few reallocations. I think you can simply\nwrite\n\n\t\tstd::copy(data.begin(), data.end(), data_.begin());\n\n> +\t}\n> +\n> +\t~Matrix() = default;\n> +\n> +\tint readYaml(const libcamera::YamlObject &yaml)\n> +\t{\n> +\t\tif (yaml.size() != R * C) {\n> +\t\t\tLOG(Matrix, Error)\n> +\t\t\t\t<< \"Wrong number of values in matrix: expected \"\n> +\t\t\t\t<< R * C << \", got \" << yaml.size();\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\tunsigned int i = 0;\n> +\t\tfor (const auto &x : yaml.asList()) {\n> +\t\t\tauto value = x.get();\n> +\t\t\tif (!value) {\n> +\t\t\t\tLOG(Matrix, Error) << \"Failed to read matrix value\";\n> +\t\t\t\treturn -EINVAL;\n> +\t\t\t}\n> +\n> +\t\t\tdata_[i++] = *value;\n> +\t\t}\n> +\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tconst std::string toString() const\n> +\t{\n> +\t\tstd::stringstream out;\n> +\n> +\t\tout << \"Matrix { \";\n> +\t\tfor (unsigned int i = 0; i < R; i++) {\n> +\t\t\tout << \"[ \";\n> +\t\t\tfor (unsigned int j = 0; j < C; j++) {\n> +\t\t\t\tout << (*this)[i][j];\n> +\t\t\t\tout << ((j + 1 < C) ? \", \" : \" \");\n> +\t\t\t}\n> +\t\t\tout << ((i + 1 < R) ? \"], \" : \"]\");\n> +\t\t}\n> +\t\tout << \" }\";\n> +\n> +\t\treturn out.str();\n> +\t}\n> +\n> +\tSpan operator[](size_t i) const\n> +\t{\n> +\t\treturn Span{ &data_.data()[i * C], C };\n> +\t}\n> +\n> +\tSpan operator[](size_t i)\n> +\t{\n> +\t\treturn Span{ &data_.data()[i * C], C };\n> +\t}\n> +\n> +private:\n> +\tstd::vector data_;\n\nShould this be a std::array, like for the Vector class ?\n\n> +};\n> +\n> +#ifndef __DOXYGEN__\n> +template +\t std::enable_if_t && std::is_arithmetic_v> * = nullptr>\n\nThe latter is ensured by the Matrix class.\n\nYou need an\n\n#else\ntemplate\n\nSame below.\n\n> +#endif /* __DOXYGEN__ */\n> +Matrix operator*(T d, const Matrix &m)\n\nI'm surprised by the order of the operands, as I would have expected\nusers to write\n\n\tMatrix m;\n\tMatrix r = m * factor;\n\nand not\n\n\tMatrix m;\n\tMatrix r = factor * m;\n\nI recommend supporting both, you can easily implement one based on the\nother.\n\nIt would also be useful to support the following with a member operator:\n\n\tMatrix m;\n\tMatrix r;\n\tr *= factor;\n\n> +{\n> +\tMatrix result;\n> +\n> +\tfor (unsigned int i = 0; i < R; i++)\n> +\t\tfor (unsigned int j = 0; j < C; j++)\n> +\t\t\tresult[i][j] = d * m[i][j];\n\nI think\n\n\tfor (unsigned int i = 0; i < R * C; i++)\n\t\tresult.data_[i] = d * m.data_[i];\n\nwould be more efficient.\n\n> +\n> +\treturn result;\n> +}\n> +\n> +#ifndef __DOXYGEN__\n> +template +\t unsigned int R1, unsigned int C1,\n> +\t unsigned int R2, unsigned int C2,\n> +\t std::enable_if_t && C1 == R2> * = nullptr>\n> +#endif /* __DOXYGEN__ */\n> +Matrix operator*(const Matrix &m1, const Matrix &m2)\n\nThis can also be simplified in a similar way as above.\n\n> +{\n> +\tMatrix result;\n> +\n> +\tfor (unsigned int i = 0; i < R1; i++) {\n> +\t\tfor (unsigned int j = 0; j < C2; j++) {\n> +\t\t\tT sum = 0;\n> +\n> +\t\t\tfor (unsigned int k = 0; k < C1; k++)\n> +\t\t\t\tsum += m1[i][k] * m2[k][j];\n> +\n> +\t\t\tresult[i][j] = sum;\n> +\t\t}\n> +\t}\n> +\n> +\treturn result;\n> +}\n> +\n> +#ifndef __DOXYGEN__\n> +template +\t std::enable_if_t> * = nullptr>\n\nEnsured by the Matrix type.\n\n> +#endif /* __DOXYGEN__ */\n> +Matrix operator+(const Matrix &m1, const Matrix &m2)\n> +{\n> +\tMatrix result;\n> +\n> +\tfor (unsigned int i = 0; i < R; i++)\n> +\t\tfor (unsigned int j = 0; j < C; j++)\n> +\t\t\tresult[i][j] = m1[i][j] + m2[i][j];\n\nI think\n\n\tfor (unsigned int i = 0; i < R * C; i++)\n\t\tresult.data_[i] = m1.data_[i] + m2.data_[i];\n\nwould be more efficient.\n\n> +\n> +\treturn result;\n> +}\n> +\n> +} /* namespace ipa */\n> +\n> +#ifndef __DOXYGEN__\n> +template\n> +std::ostream &operator<<(std::ostream &out, const ipa::Matrix &m)\n> +{\n> +\tout << m.toString();\n> +\treturn out;\n> +}\n> +#endif /* __DOXYGEN__ */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index b663afc7d9fe..067d0d273e0a 100644\n> --- a/src/ipa/libipa/meson.build\n> +++ b/src/ipa/libipa/meson.build\n> @@ -7,6 +7,7 @@ libipa_headers = files([\n> 'exposure_mode_helper.h',\n> 'fc_queue.h',\n> 'histogram.h',\n> + 'matrix.h',\n> 'module.h',\n> ])\n> \n> @@ -17,6 +18,7 @@ libipa_sources = files([\n> 'exposure_mode_helper.cpp',\n> 'fc_queue.cpp',\n> 'histogram.cpp',\n> + 'matrix.cpp',\n> 'module.cpp',\n> ])\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id EC8F2BD87C\n\tfor ;\n\tTue, 11 Jun 2024 23:33:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CB00965456;\n\tWed, 12 Jun 2024 01:33:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B34D861A26\n\tfor ;\n\tWed, 12 Jun 2024 01:33:53 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 55689B3;\n\tWed, 12 Jun 2024 01:33:40 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"oXW0geFQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718148820;\n\tbh=tpIrLknrdBVC8AuS4IeHcpv/Ng4bRqxALFmT57AJs+k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=oXW0geFQEICDfxyl4jN6bN9Hrlz36YOYonMp0RgfN+Mq/mMCcrUMaweJz4fCwNvrb\n\tfNIRVECjBSpvbc/yRx2+5xtVopT4yNEfkaEe9tIQWxvv0MqeH6zlURSoEI8RUQ3MT0\n\tb0DtjMAZzdKgQ4nZAalAfr+nvZ3qyJDSVa8vcjgg=","Date":"Wed, 12 Jun 2024 02:33:33 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org,\n\tStefan Klug ,\n\tKieran Bingham ","Subject":"Re: [PATCH v7 1/3] ipa: libipa: Add Matrix class","Message-ID":"<20240611233333.GE15006@pendragon.ideasonboard.com>","References":"<20240611140207.520083-1-paul.elder@ideasonboard.com>\n\t<20240611140207.520083-2-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240611140207.520083-2-paul.elder@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29867,"web_url":"https://patchwork.libcamera.org/comment/29867/","msgid":"","date":"2024-06-12T07:59:39","subject":"Re: [PATCH v7 1/3] ipa: libipa: Add Matrix class","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Wed, Jun 12, 2024 at 02:33:33AM +0300, Laurent Pinchart wrote:\n> Hi Paul,\n> \n> Thank you for the patch.\n> \n> On Tue, Jun 11, 2024 at 11:02:05PM +0900, Paul Elder wrote:\n> > Add a class to represent a Matrix object and operations for adding\n> > matrices, multipling a matrix by a scalar, and multiplying two matrices.\n> > \n> > Signed-off-by: Paul Elder \n> > Reviewed-by: Stefan Klug \n> > Reviewed-by: Kieran Bingham \n> > \n> > ---\n> > Changes in v7:\n> > - fix copyright and license\n> > \n> > Changes in v6:\n> > - fix doxygen\n> > \n> > Changes in v5:\n> > - add documentation\n> > \n> > Changes in v4:\n> > - remove stray semicolons\n> > - add operator<<\n> > - clean up/optimize constructor\n> > - replace get() and set() with operator[] (and a second [] can be used\n> > as operator[] returns a Span)\n> > \n> > Changes in v3:\n> > - fix template parameters of operator* to allow different types for the\n> > scalar multiplier and the matrix's number type\n> > - clear data in constructors\n> > - fix assert in constructor\n> > \n> > Changes v2:\n> > - make rows and columns into template arguments\n> > - initialize to identity matrix on construction\n> > - add getter and setter\n> > - change from struct to class\n> > - fix matrix multiplication\n> > - clean up unused includes\n> > - avoid dereferencing an absent std::optional\n> > ---\n> > src/ipa/libipa/matrix.cpp | 123 ++++++++++++++++++++++++++\n> > src/ipa/libipa/matrix.h | 172 +++++++++++++++++++++++++++++++++++++\n> > src/ipa/libipa/meson.build | 2 +\n> > 3 files changed, 297 insertions(+)\n> > create mode 100644 src/ipa/libipa/matrix.cpp\n> > create mode 100644 src/ipa/libipa/matrix.h\n> > \n> > diff --git a/src/ipa/libipa/matrix.cpp b/src/ipa/libipa/matrix.cpp\n> > new file mode 100644\n> > index 000000000000..02091a0b4ce1\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/matrix.cpp\n> > @@ -0,0 +1,123 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Paul Elder \n> > + *\n> > + * Matrix and related operations\n> > + */\n> > +\n> > +#include \"matrix.h\"\n> > +\n> > +#include \n> > +\n> > +/**\n> > + * \\file matrix.h\n> > + * \\brief Matrix class\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(Matrix)\n> > +\n> > +namespace ipa {\n> > +\n> > +/**\n> > + * \\class Matrix\n> > + * \\brief Matrix class\n> > + * \\tparam T Type of numerical values to be stored in the matrix\n> > + * \\tparam R Number of rows in the matrix\n> > + * \\tparam C Number of columns in the matrix\n> > + */\n> > +\n> > +/**\n> > + * \\fn Matrix::Matrix()\n> > + * \\brief Construct an identity matrix\n> \n> I would have expected the default constructor to construct a zero\n> matrix. Is there a reason to go for identity instead ?\n> \n> As identity matrices are useful too, I would add a static\n> Matrix::identity() function that constructs and returns an identity\n> matrix.\n> \n> > + */\n> > +\n> > +/**\n> > + * \\fn Matrix::Matrix(const std::vector &data)\n> \n> Turning the argument into a Span would allow passing any type of\n> contiguous data (vector, array, C array, ...) without requiring the\n> caller to construct a std::vector.\n> \n\nThat's what I wanted to do but it was refusing to work.\n\n> > + * \\brief Construct matrix from supplied data\n> \n> s/matrix/a matrix/\n> \n> > + * \\param data Data from which to construct a matrix\n> \n> \\param[in]\n> \n> Same below.\n> \n> > + *\n> > + * \\a data is a one-dimensional vector and will be turned into a matrix in\n> > + * row-major order. The size of \\a data must be equal to the product of the\n> > + * number of rows and columns of the matrix (RxC).\n> > + */\n> > +\n> > +/**\n> > + * \\fn Matrix::readYaml\n> \n> s/$/()/\n> \n> Same below.\n> \n> > + * \\brief Populate the matrix with yaml data\n> > + * \\param yaml Yaml data to populate the matrix with\n> > + *\n> > + * Any existing data in the matrix will be overwritten. The size of the data\n> > + * read from \\a yaml must be equal to the product of the number of rows and\n> > + * columns of the matrix (RxC).\n> > + *\n> > + * The yaml data is expected to be a list with elements of type T.\n> \n> in row-major order.\n> \n> > + *\n> > + * \\return 0 on success, negative error code otherwise\n> > + */\n> > +\n> > +/**\n> > + * \\fn Matrix::toString\n> > + * \\brief Assemble and return a string describing the matrix\n> > + * \\return A string describing the matrix\n> > + */\n> > +\n> > +/**\n> > + * \\fn Span Matrix::operator[](size_t i) const\n> > + * \\brief Index to a row in the matrix\n> > + * \\param i Index of row to retrieve\n> > + *\n> > + * This operator[] returns a Span, which can then be indexed into again with\n> > + * another operator[], allowing a convenient m[i][j] to access elements of the\n> > + * matrix. Note that the lifetime of the Span returned by this first-level\n> > + * operator[] is bound to that of the Matrix itself, so it is not recommended\n> > + * to save the Span that is the result of this operator[].\n> \n> That's a clever idea, I like it.\n> \n> Longer term we may need a different (and\n> unfortunately much more complex) implementation to allow using Matrix[i]\n> in APIs that expect a Vector (which itself may need to be reimplemented\n> as a matrix with a single column). And the next thing we know is that\n> we'll reimplement uBLAS or Eigen3 :-) I'll cross my fingers and wish we\n> won't reach that point.\n> \n> https://stackoverflow.com/questions/1380371/what-are-the-most-widely-used-c-vector-matrix-math-linear-algebra-libraries-a\n> \n> \"It seems that many projects slowly come upon a need to do matrix math,\n> and fall into the trap of first building some vector classes and slowly\n> adding in functionality until they get caught building a half-assed\n> custom linear algebra library, and depending on it.\"\n> \n\nOops... :)\n\n> > + *\n> > + * \\return Row \\a i from the matrix, as a Span\n> > + */\n> > +\n> > +/**\n> > + * \\fn Matrix::operator[](size_t i)\n> > + * \\copydoc Matrix::operator[](size_t i) const\n> > + */\n> > +\n> > +/**\n> > + * \\fn Matrix::Matrix operator*(T d, const Matrix &m)\n> > + * \\brief Scalar product\n> > + * \\tparam T Type of the numerical scalar value\n> > + * \\tparam U Type of numerical values in the matrix\n> > + * \\tparam R Number of rows in the matrix\n> > + * \\tparam C Number of columns in the matrix\n> > + * \\param d Scalar\n> > + * \\param m Matrix\n> > + * \\return Product of scalar \\a d and matrix \\a m\n> > + */\n> > +\n> > +/**\n> > + * \\fn Matrix operator*(const Matrix &m1, const Matrix &m2)\n> > + * \\brief Matrix multiplication\n> > + * \\tparam T Type of numerical values in the matrices\n> > + * \\tparam R1 Number of rows in the first matrix\n> > + * \\tparam C1 Number of columns in the first matrix\n> > + * \\tparam R2 Number of rows in the second matrix\n> > + * \\tparam C2 Number of columns in the second matrix\n> > + * \\param m1 Multiplicand matrix\n> > + * \\param m2 Multiplier matrix\n> > + * \\return Matrix product of matrices \\a m1 and \\a m2\n> > + */\n> > +\n> > +/**\n> > + * \\fn Matrix operator+(const Matrix &m1, const Matrix &m2)\n> > + * \\brief Matrix addition\n> > + * \\tparam T Type of numerical values in the matrices\n> > + * \\tparam R Number of rows in the matrices\n> > + * \\tparam C Number of columns in the matrices\n> > + * \\param m1 Summand matrix\n> > + * \\param m2 Summand matrix\n> > + * \\return Matrix sum of matrices \\a m1 and \\a m2\n> > + */\n> > +\n> > +} /* namespace ipa */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/libipa/matrix.h b/src/ipa/libipa/matrix.h\n> > new file mode 100644\n> > index 000000000000..90eaea03bd14\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/matrix.h\n> > @@ -0,0 +1,172 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Paul Elder \n> > + *\n> > + * Matrix and related operations\n> > + */\n> > +#pragma once\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \"libcamera/internal/yaml_parser.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DECLARE_CATEGORY(Matrix)\n> > +\n> > +namespace ipa {\n> > +\n> > +#ifndef __DOXYGEN__\n> > +template \n> Following the Vector class,\n> \n> template \n> > +\t std::enable_if_t> * = nullptr>\n> > +#else\n> > +template\n> > +#endif /* __DOXYGEN__ */\n> > +class Matrix\n> > +{\n> > +public:\n> > +\tMatrix()\n> > +\t\t: data_(R * C, static_cast(false))\n> \n> false ? That's a strange one, anything wrong with 0 ?\n> \n> > +\t{\n> > +\t\tfor (size_t i = 0; i < std::min(R, C); i++)\n> > +\t\t\t(*this)[i][i] = static_cast(true);\n> \n> And 1 here.\n> \n> > +\t}\n> > +\n> > +\tMatrix(const std::vector &data)\n> > +\t{\n> > +\t\tASSERT(data.size() == R * C);\n> > +\n> > +\t\tdata_.clear();\n> > +\t\tfor (const T &x : data)\n> > +\t\t\tdata_.push_back(x);\n> \n> That will perform quite a few reallocations. I think you can simply\n> write\n> \n> \t\tstd::copy(data.begin(), data.end(), data_.begin());\n> \n> > +\t}\n> > +\n> > +\t~Matrix() = default;\n> > +\n> > +\tint readYaml(const libcamera::YamlObject &yaml)\n> > +\t{\n> > +\t\tif (yaml.size() != R * C) {\n> > +\t\t\tLOG(Matrix, Error)\n> > +\t\t\t\t<< \"Wrong number of values in matrix: expected \"\n> > +\t\t\t\t<< R * C << \", got \" << yaml.size();\n> > +\t\t\treturn -EINVAL;\n> > +\t\t}\n> > +\n> > +\t\tunsigned int i = 0;\n> > +\t\tfor (const auto &x : yaml.asList()) {\n> > +\t\t\tauto value = x.get();\n> > +\t\t\tif (!value) {\n> > +\t\t\t\tLOG(Matrix, Error) << \"Failed to read matrix value\";\n> > +\t\t\t\treturn -EINVAL;\n> > +\t\t\t}\n> > +\n> > +\t\t\tdata_[i++] = *value;\n> > +\t\t}\n> > +\n> > +\t\treturn 0;\n> > +\t}\n> > +\n> > +\tconst std::string toString() const\n> > +\t{\n> > +\t\tstd::stringstream out;\n> > +\n> > +\t\tout << \"Matrix { \";\n> > +\t\tfor (unsigned int i = 0; i < R; i++) {\n> > +\t\t\tout << \"[ \";\n> > +\t\t\tfor (unsigned int j = 0; j < C; j++) {\n> > +\t\t\t\tout << (*this)[i][j];\n> > +\t\t\t\tout << ((j + 1 < C) ? \", \" : \" \");\n> > +\t\t\t}\n> > +\t\t\tout << ((i + 1 < R) ? \"], \" : \"]\");\n> > +\t\t}\n> > +\t\tout << \" }\";\n> > +\n> > +\t\treturn out.str();\n> > +\t}\n> > +\n> > +\tSpan operator[](size_t i) const\n> > +\t{\n> > +\t\treturn Span{ &data_.data()[i * C], C };\n> > +\t}\n> > +\n> > +\tSpan operator[](size_t i)\n> > +\t{\n> > +\t\treturn Span{ &data_.data()[i * C], C };\n> > +\t}\n> > +\n> > +private:\n> > +\tstd::vector data_;\n> \n> Should this be a std::array, like for the Vector class ?\n> \n> > +};\n> > +\n> > +#ifndef __DOXYGEN__\n> > +template > +\t std::enable_if_t && std::is_arithmetic_v> * = nullptr>\n> \n> The latter is ensured by the Matrix class.\n> \n> You need an\n> \n> #else\n> template\n> \n> Same below.\n> \n> > +#endif /* __DOXYGEN__ */\n> > +Matrix operator*(T d, const Matrix &m)\n> \n> I'm surprised by the order of the operands, as I would have expected\n> users to write\n> \n> \tMatrix m;\n> \tMatrix r = m * factor;\n> \n> and not\n> \n> \tMatrix m;\n> \tMatrix r = factor * m;\n> \n> I recommend supporting both, you can easily implement one based on the\n> other.\n> \n> It would also be useful to support the following with a member operator:\n> \n> \tMatrix m;\n> \tMatrix r;\n> \tr *= factor;\n> \n> > +{\n> > +\tMatrix result;\n> > +\n> > +\tfor (unsigned int i = 0; i < R; i++)\n> > +\t\tfor (unsigned int j = 0; j < C; j++)\n> > +\t\t\tresult[i][j] = d * m[i][j];\n> \n> I think\n> \n> \tfor (unsigned int i = 0; i < R * C; i++)\n> \t\tresult.data_[i] = d * m.data_[i];\n> \n> would be more efficient.\n\nYeah but data_ is private...\n\n\nPaul\n\n> \n> > +\n> > +\treturn result;\n> > +}\n> > +\n> > +#ifndef __DOXYGEN__\n> > +template > +\t unsigned int R1, unsigned int C1,\n> > +\t unsigned int R2, unsigned int C2,\n> > +\t std::enable_if_t && C1 == R2> * = nullptr>\n> > +#endif /* __DOXYGEN__ */\n> > +Matrix operator*(const Matrix &m1, const Matrix &m2)\n> \n> This can also be simplified in a similar way as above.\n> \n> > +{\n> > +\tMatrix result;\n> > +\n> > +\tfor (unsigned int i = 0; i < R1; i++) {\n> > +\t\tfor (unsigned int j = 0; j < C2; j++) {\n> > +\t\t\tT sum = 0;\n> > +\n> > +\t\t\tfor (unsigned int k = 0; k < C1; k++)\n> > +\t\t\t\tsum += m1[i][k] * m2[k][j];\n> > +\n> > +\t\t\tresult[i][j] = sum;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\treturn result;\n> > +}\n> > +\n> > +#ifndef __DOXYGEN__\n> > +template > +\t std::enable_if_t> * = nullptr>\n> \n> Ensured by the Matrix type.\n> \n> > +#endif /* __DOXYGEN__ */\n> > +Matrix operator+(const Matrix &m1, const Matrix &m2)\n> > +{\n> > +\tMatrix result;\n> > +\n> > +\tfor (unsigned int i = 0; i < R; i++)\n> > +\t\tfor (unsigned int j = 0; j < C; j++)\n> > +\t\t\tresult[i][j] = m1[i][j] + m2[i][j];\n> \n> I think\n> \n> \tfor (unsigned int i = 0; i < R * C; i++)\n> \t\tresult.data_[i] = m1.data_[i] + m2.data_[i];\n> \n> would be more efficient.\n> \n> > +\n> > +\treturn result;\n> > +}\n> > +\n> > +} /* namespace ipa */\n> > +\n> > +#ifndef __DOXYGEN__\n> > +template\n> > +std::ostream &operator<<(std::ostream &out, const ipa::Matrix &m)\n> > +{\n> > +\tout << m.toString();\n> > +\treturn out;\n> > +}\n> > +#endif /* __DOXYGEN__ */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> > index b663afc7d9fe..067d0d273e0a 100644\n> > --- a/src/ipa/libipa/meson.build\n> > +++ b/src/ipa/libipa/meson.build\n> > @@ -7,6 +7,7 @@ libipa_headers = files([\n> > 'exposure_mode_helper.h',\n> > 'fc_queue.h',\n> > 'histogram.h',\n> > + 'matrix.h',\n> > 'module.h',\n> > ])\n> > \n> > @@ -17,6 +18,7 @@ libipa_sources = files([\n> > 'exposure_mode_helper.cpp',\n> > 'fc_queue.cpp',\n> > 'histogram.cpp',\n> > + 'matrix.cpp',\n> > 'module.cpp',\n> > ])\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E4D93C3237\n\tfor ;\n\tWed, 12 Jun 2024 07:59:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C722F6546A;\n\tWed, 12 Jun 2024 09:59:46 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 226186545A\n\tfor ;\n\tWed, 12 Jun 2024 09:59:46 +0200 (CEST)","from pyrite.rasen.tech (h175-177-049-156.catv02.itscom.jp\n\t[175.177.49.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7120D4D0;\n\tWed, 12 Jun 2024 09:59:31 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"kPGoHn3l\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718179172;\n\tbh=SY/rh7vqhS9YmPcAYCVqvue2PcSC/7z0TrOdcTANsSA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kPGoHn3leGfgppTI3haUwyW3np9o++IRQYg40X6YD4l0JEtUHm8lExM1i1TqDa7sl\n\tILXw5hCyXG8C6bIEG0ns/D5+c/YqytaJOqx9WftBpbxVBgwk1QZqkgN/Z4r7gx8L1n\n\ts8PF1bBVCIZGvr4/GGRK0+Bh1R5JpDVhbcQNPvnM=","Date":"Wed, 12 Jun 2024 16:59:39 +0900","From":"Paul Elder ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org,\n\tStefan Klug ,\n\tKieran Bingham ","Subject":"Re: [PATCH v7 1/3] ipa: libipa: Add Matrix class","Message-ID":"","References":"<20240611140207.520083-1-paul.elder@ideasonboard.com>\n\t<20240611140207.520083-2-paul.elder@ideasonboard.com>\n\t<20240611233333.GE15006@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20240611233333.GE15006@pendragon.ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29870,"web_url":"https://patchwork.libcamera.org/comment/29870/","msgid":"<20240612093243.GH28989@pendragon.ideasonboard.com>","date":"2024-06-12T09:32:43","subject":"Re: [PATCH v7 1/3] ipa: libipa: Add Matrix class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Jun 12, 2024 at 04:59:39PM +0900, Paul Elder wrote:\n> On Wed, Jun 12, 2024 at 02:33:33AM +0300, Laurent Pinchart wrote:\n> > On Tue, Jun 11, 2024 at 11:02:05PM +0900, Paul Elder wrote:\n> > > Add a class to represent a Matrix object and operations for adding\n> > > matrices, multipling a matrix by a scalar, and multiplying two matrices.\n> > > \n> > > Signed-off-by: Paul Elder \n> > > Reviewed-by: Stefan Klug \n> > > Reviewed-by: Kieran Bingham \n> > > \n> > > ---\n> > > Changes in v7:\n> > > - fix copyright and license\n> > > \n> > > Changes in v6:\n> > > - fix doxygen\n> > > \n> > > Changes in v5:\n> > > - add documentation\n> > > \n> > > Changes in v4:\n> > > - remove stray semicolons\n> > > - add operator<<\n> > > - clean up/optimize constructor\n> > > - replace get() and set() with operator[] (and a second [] can be used\n> > > as operator[] returns a Span)\n> > > \n> > > Changes in v3:\n> > > - fix template parameters of operator* to allow different types for the\n> > > scalar multiplier and the matrix's number type\n> > > - clear data in constructors\n> > > - fix assert in constructor\n> > > \n> > > Changes v2:\n> > > - make rows and columns into template arguments\n> > > - initialize to identity matrix on construction\n> > > - add getter and setter\n> > > - change from struct to class\n> > > - fix matrix multiplication\n> > > - clean up unused includes\n> > > - avoid dereferencing an absent std::optional\n> > > ---\n> > > src/ipa/libipa/matrix.cpp | 123 ++++++++++++++++++++++++++\n> > > src/ipa/libipa/matrix.h | 172 +++++++++++++++++++++++++++++++++++++\n> > > src/ipa/libipa/meson.build | 2 +\n> > > 3 files changed, 297 insertions(+)\n> > > create mode 100644 src/ipa/libipa/matrix.cpp\n> > > create mode 100644 src/ipa/libipa/matrix.h\n> > > \n> > > diff --git a/src/ipa/libipa/matrix.cpp b/src/ipa/libipa/matrix.cpp\n> > > new file mode 100644\n> > > index 000000000000..02091a0b4ce1\n> > > --- /dev/null\n> > > +++ b/src/ipa/libipa/matrix.cpp\n> > > @@ -0,0 +1,123 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2024, Paul Elder \n> > > + *\n> > > + * Matrix and related operations\n> > > + */\n> > > +\n> > > +#include \"matrix.h\"\n> > > +\n> > > +#include \n> > > +\n> > > +/**\n> > > + * \\file matrix.h\n> > > + * \\brief Matrix class\n> > > + */\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +LOG_DEFINE_CATEGORY(Matrix)\n> > > +\n> > > +namespace ipa {\n> > > +\n> > > +/**\n> > > + * \\class Matrix\n> > > + * \\brief Matrix class\n> > > + * \\tparam T Type of numerical values to be stored in the matrix\n> > > + * \\tparam R Number of rows in the matrix\n> > > + * \\tparam C Number of columns in the matrix\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Matrix::Matrix()\n> > > + * \\brief Construct an identity matrix\n> > \n> > I would have expected the default constructor to construct a zero\n> > matrix. Is there a reason to go for identity instead ?\n> > \n> > As identity matrices are useful too, I would add a static\n> > Matrix::identity() function that constructs and returns an identity\n> > matrix.\n> > \n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Matrix::Matrix(const std::vector &data)\n> > \n> > Turning the argument into a Span would allow passing any type of\n> > contiguous data (vector, array, C array, ...) without requiring the\n> > caller to construct a std::vector.\n> \n> That's what I wanted to do but it was refusing to work.\n\nShow me the failure and I'll help :-)\n\n> > > + * \\brief Construct matrix from supplied data\n> > \n> > s/matrix/a matrix/\n> > \n> > > + * \\param data Data from which to construct a matrix\n> > \n> > \\param[in]\n> > \n> > Same below.\n> > \n> > > + *\n> > > + * \\a data is a one-dimensional vector and will be turned into a matrix in\n> > > + * row-major order. The size of \\a data must be equal to the product of the\n> > > + * number of rows and columns of the matrix (RxC).\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Matrix::readYaml\n> > \n> > s/$/()/\n> > \n> > Same below.\n> > \n> > > + * \\brief Populate the matrix with yaml data\n> > > + * \\param yaml Yaml data to populate the matrix with\n> > > + *\n> > > + * Any existing data in the matrix will be overwritten. The size of the data\n> > > + * read from \\a yaml must be equal to the product of the number of rows and\n> > > + * columns of the matrix (RxC).\n> > > + *\n> > > + * The yaml data is expected to be a list with elements of type T.\n> > \n> > in row-major order.\n> > \n> > > + *\n> > > + * \\return 0 on success, negative error code otherwise\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Matrix::toString\n> > > + * \\brief Assemble and return a string describing the matrix\n> > > + * \\return A string describing the matrix\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Span Matrix::operator[](size_t i) const\n> > > + * \\brief Index to a row in the matrix\n> > > + * \\param i Index of row to retrieve\n> > > + *\n> > > + * This operator[] returns a Span, which can then be indexed into again with\n> > > + * another operator[], allowing a convenient m[i][j] to access elements of the\n> > > + * matrix. Note that the lifetime of the Span returned by this first-level\n> > > + * operator[] is bound to that of the Matrix itself, so it is not recommended\n> > > + * to save the Span that is the result of this operator[].\n> > \n> > That's a clever idea, I like it.\n> > \n> > Longer term we may need a different (and\n> > unfortunately much more complex) implementation to allow using Matrix[i]\n> > in APIs that expect a Vector (which itself may need to be reimplemented\n> > as a matrix with a single column). And the next thing we know is that\n> > we'll reimplement uBLAS or Eigen3 :-) I'll cross my fingers and wish we\n> > won't reach that point.\n> > \n> > https://stackoverflow.com/questions/1380371/what-are-the-most-widely-used-c-vector-matrix-math-linear-algebra-libraries-a\n> > \n> > \"It seems that many projects slowly come upon a need to do matrix math,\n> > and fall into the trap of first building some vector classes and slowly\n> > adding in functionality until they get caught building a half-assed\n> > custom linear algebra library, and depending on it.\"\n> \n> Oops... :)\n> \n> > > + *\n> > > + * \\return Row \\a i from the matrix, as a Span\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Matrix::operator[](size_t i)\n> > > + * \\copydoc Matrix::operator[](size_t i) const\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Matrix::Matrix operator*(T d, const Matrix &m)\n> > > + * \\brief Scalar product\n> > > + * \\tparam T Type of the numerical scalar value\n> > > + * \\tparam U Type of numerical values in the matrix\n> > > + * \\tparam R Number of rows in the matrix\n> > > + * \\tparam C Number of columns in the matrix\n> > > + * \\param d Scalar\n> > > + * \\param m Matrix\n> > > + * \\return Product of scalar \\a d and matrix \\a m\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Matrix operator*(const Matrix &m1, const Matrix &m2)\n> > > + * \\brief Matrix multiplication\n> > > + * \\tparam T Type of numerical values in the matrices\n> > > + * \\tparam R1 Number of rows in the first matrix\n> > > + * \\tparam C1 Number of columns in the first matrix\n> > > + * \\tparam R2 Number of rows in the second matrix\n> > > + * \\tparam C2 Number of columns in the second matrix\n> > > + * \\param m1 Multiplicand matrix\n> > > + * \\param m2 Multiplier matrix\n> > > + * \\return Matrix product of matrices \\a m1 and \\a m2\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Matrix operator+(const Matrix &m1, const Matrix &m2)\n> > > + * \\brief Matrix addition\n> > > + * \\tparam T Type of numerical values in the matrices\n> > > + * \\tparam R Number of rows in the matrices\n> > > + * \\tparam C Number of columns in the matrices\n> > > + * \\param m1 Summand matrix\n> > > + * \\param m2 Summand matrix\n> > > + * \\return Matrix sum of matrices \\a m1 and \\a m2\n> > > + */\n> > > +\n> > > +} /* namespace ipa */\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/ipa/libipa/matrix.h b/src/ipa/libipa/matrix.h\n> > > new file mode 100644\n> > > index 000000000000..90eaea03bd14\n> > > --- /dev/null\n> > > +++ b/src/ipa/libipa/matrix.h\n> > > @@ -0,0 +1,172 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2024, Paul Elder \n> > > + *\n> > > + * Matrix and related operations\n> > > + */\n> > > +#pragma once\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"libcamera/internal/yaml_parser.h\"\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +LOG_DECLARE_CATEGORY(Matrix)\n> > > +\n> > > +namespace ipa {\n> > > +\n> > > +#ifndef __DOXYGEN__\n> > > +template > \n> > Following the Vector class,\n> > \n> > template > \n> > > +\t std::enable_if_t> * = nullptr>\n> > > +#else\n> > > +template\n> > > +#endif /* __DOXYGEN__ */\n> > > +class Matrix\n> > > +{\n> > > +public:\n> > > +\tMatrix()\n> > > +\t\t: data_(R * C, static_cast(false))\n> > \n> > false ? That's a strange one, anything wrong with 0 ?\n> > \n> > > +\t{\n> > > +\t\tfor (size_t i = 0; i < std::min(R, C); i++)\n> > > +\t\t\t(*this)[i][i] = static_cast(true);\n> > \n> > And 1 here.\n> > \n> > > +\t}\n> > > +\n> > > +\tMatrix(const std::vector &data)\n> > > +\t{\n> > > +\t\tASSERT(data.size() == R * C);\n> > > +\n> > > +\t\tdata_.clear();\n> > > +\t\tfor (const T &x : data)\n> > > +\t\t\tdata_.push_back(x);\n> > \n> > That will perform quite a few reallocations. I think you can simply\n> > write\n> > \n> > \t\tstd::copy(data.begin(), data.end(), data_.begin());\n> > \n> > > +\t}\n> > > +\n> > > +\t~Matrix() = default;\n> > > +\n> > > +\tint readYaml(const libcamera::YamlObject &yaml)\n> > > +\t{\n> > > +\t\tif (yaml.size() != R * C) {\n> > > +\t\t\tLOG(Matrix, Error)\n> > > +\t\t\t\t<< \"Wrong number of values in matrix: expected \"\n> > > +\t\t\t\t<< R * C << \", got \" << yaml.size();\n> > > +\t\t\treturn -EINVAL;\n> > > +\t\t}\n> > > +\n> > > +\t\tunsigned int i = 0;\n> > > +\t\tfor (const auto &x : yaml.asList()) {\n> > > +\t\t\tauto value = x.get();\n> > > +\t\t\tif (!value) {\n> > > +\t\t\t\tLOG(Matrix, Error) << \"Failed to read matrix value\";\n> > > +\t\t\t\treturn -EINVAL;\n> > > +\t\t\t}\n> > > +\n> > > +\t\t\tdata_[i++] = *value;\n> > > +\t\t}\n> > > +\n> > > +\t\treturn 0;\n> > > +\t}\n> > > +\n> > > +\tconst std::string toString() const\n> > > +\t{\n> > > +\t\tstd::stringstream out;\n> > > +\n> > > +\t\tout << \"Matrix { \";\n> > > +\t\tfor (unsigned int i = 0; i < R; i++) {\n> > > +\t\t\tout << \"[ \";\n> > > +\t\t\tfor (unsigned int j = 0; j < C; j++) {\n> > > +\t\t\t\tout << (*this)[i][j];\n> > > +\t\t\t\tout << ((j + 1 < C) ? \", \" : \" \");\n> > > +\t\t\t}\n> > > +\t\t\tout << ((i + 1 < R) ? \"], \" : \"]\");\n> > > +\t\t}\n> > > +\t\tout << \" }\";\n> > > +\n> > > +\t\treturn out.str();\n> > > +\t}\n> > > +\n> > > +\tSpan operator[](size_t i) const\n> > > +\t{\n> > > +\t\treturn Span{ &data_.data()[i * C], C };\n> > > +\t}\n> > > +\n> > > +\tSpan operator[](size_t i)\n> > > +\t{\n> > > +\t\treturn Span{ &data_.data()[i * C], C };\n> > > +\t}\n> > > +\n> > > +private:\n> > > +\tstd::vector data_;\n> > \n> > Should this be a std::array, like for the Vector class ?\n> > \n> > > +};\n> > > +\n> > > +#ifndef __DOXYGEN__\n> > > +template > > +\t std::enable_if_t && std::is_arithmetic_v> * = nullptr>\n> > \n> > The latter is ensured by the Matrix class.\n> > \n> > You need an\n> > \n> > #else\n> > template\n> > \n> > Same below.\n> > \n> > > +#endif /* __DOXYGEN__ */\n> > > +Matrix operator*(T d, const Matrix &m)\n> > \n> > I'm surprised by the order of the operands, as I would have expected\n> > users to write\n> > \n> > \tMatrix m;\n> > \tMatrix r = m * factor;\n> > \n> > and not\n> > \n> > \tMatrix m;\n> > \tMatrix r = factor * m;\n> > \n> > I recommend supporting both, you can easily implement one based on the\n> > other.\n> > \n> > It would also be useful to support the following with a member operator:\n> > \n> > \tMatrix m;\n> > \tMatrix r;\n> > \tr *= factor;\n> > \n> > > +{\n> > > +\tMatrix result;\n> > > +\n> > > +\tfor (unsigned int i = 0; i < R; i++)\n> > > +\t\tfor (unsigned int j = 0; j < C; j++)\n> > > +\t\t\tresult[i][j] = d * m[i][j];\n> > \n> > I think\n> > \n> > \tfor (unsigned int i = 0; i < R * C; i++)\n> > \t\tresult.data_[i] = d * m.data_[i];\n> > \n> > would be more efficient.\n> \n> Yeah but data_ is private...\n\nYou can add a friend statement to make it accessible to the out-of-class\noperators. See the other Vector class I shared with you recently.\n\n> > > +\n> > > +\treturn result;\n> > > +}\n> > > +\n> > > +#ifndef __DOXYGEN__\n> > > +template > > +\t unsigned int R1, unsigned int C1,\n> > > +\t unsigned int R2, unsigned int C2,\n> > > +\t std::enable_if_t && C1 == R2> * = nullptr>\n> > > +#endif /* __DOXYGEN__ */\n> > > +Matrix operator*(const Matrix &m1, const Matrix &m2)\n> > \n> > This can also be simplified in a similar way as above.\n> > \n> > > +{\n> > > +\tMatrix result;\n> > > +\n> > > +\tfor (unsigned int i = 0; i < R1; i++) {\n> > > +\t\tfor (unsigned int j = 0; j < C2; j++) {\n> > > +\t\t\tT sum = 0;\n> > > +\n> > > +\t\t\tfor (unsigned int k = 0; k < C1; k++)\n> > > +\t\t\t\tsum += m1[i][k] * m2[k][j];\n> > > +\n> > > +\t\t\tresult[i][j] = sum;\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > +\treturn result;\n> > > +}\n> > > +\n> > > +#ifndef __DOXYGEN__\n> > > +template > > +\t std::enable_if_t> * = nullptr>\n> > \n> > Ensured by the Matrix type.\n> > \n> > > +#endif /* __DOXYGEN__ */\n> > > +Matrix operator+(const Matrix &m1, const Matrix &m2)\n> > > +{\n> > > +\tMatrix result;\n> > > +\n> > > +\tfor (unsigned int i = 0; i < R; i++)\n> > > +\t\tfor (unsigned int j = 0; j < C; j++)\n> > > +\t\t\tresult[i][j] = m1[i][j] + m2[i][j];\n> > \n> > I think\n> > \n> > \tfor (unsigned int i = 0; i < R * C; i++)\n> > \t\tresult.data_[i] = m1.data_[i] + m2.data_[i];\n> > \n> > would be more efficient.\n> > \n> > > +\n> > > +\treturn result;\n> > > +}\n> > > +\n> > > +} /* namespace ipa */\n> > > +\n> > > +#ifndef __DOXYGEN__\n> > > +template\n> > > +std::ostream &operator<<(std::ostream &out, const ipa::Matrix &m)\n> > > +{\n> > > +\tout << m.toString();\n> > > +\treturn out;\n> > > +}\n> > > +#endif /* __DOXYGEN__ */\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> > > index b663afc7d9fe..067d0d273e0a 100644\n> > > --- a/src/ipa/libipa/meson.build\n> > > +++ b/src/ipa/libipa/meson.build\n> > > @@ -7,6 +7,7 @@ libipa_headers = files([\n> > > 'exposure_mode_helper.h',\n> > > 'fc_queue.h',\n> > > 'histogram.h',\n> > > + 'matrix.h',\n> > > 'module.h',\n> > > ])\n> > > \n> > > @@ -17,6 +18,7 @@ libipa_sources = files([\n> > > 'exposure_mode_helper.cpp',\n> > > 'fc_queue.cpp',\n> > > 'histogram.cpp',\n> > > + 'matrix.cpp',\n> > > 'module.cpp',\n> > > ])\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 01891C3237\n\tfor ;\n\tWed, 12 Jun 2024 09:33:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0382E65461;\n\tWed, 12 Jun 2024 11:33:05 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D5AD261A2A\n\tfor ;\n\tWed, 12 Jun 2024 11:33:03 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4E6E1230;\n\tWed, 12 Jun 2024 11:32:50 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"DMtsz5dP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718184770;\n\tbh=MNqeLf6V6EbWiYgI/o0gUGAx7LAGgWHRD6Zr8tF5pQc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DMtsz5dP+/08NPbvTLlFHGODCXXOsL2eoBM8djNNqlx9zTgwhm+ItkcnldHdxqnnc\n\tZTZsd8JSvwDnSmutGAmsRarkA3LENdyXPP0wVAifkaEsIelBkFuwsiBwBMODoRBVng\n\tc9JLx55DWBsvEk4skR1O47KzBxSxiM0JtppcwkDM=","Date":"Wed, 12 Jun 2024 12:32:43 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org,\n\tStefan Klug ,\n\tKieran Bingham ","Subject":"Re: [PATCH v7 1/3] ipa: libipa: Add Matrix class","Message-ID":"<20240612093243.GH28989@pendragon.ideasonboard.com>","References":"<20240611140207.520083-1-paul.elder@ideasonboard.com>\n\t<20240611140207.520083-2-paul.elder@ideasonboard.com>\n\t<20240611233333.GE15006@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]