[{"id":29958,"web_url":"https://patchwork.libcamera.org/comment/29958/","msgid":"<20240616171025.GA7378@pendragon.ideasonboard.com>","date":"2024-06-16T17:10:25","subject":"Re: [PATCH v9 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 Fri, Jun 14, 2024 at 08:37:55PM +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 <paul.elder@ideasonboard.com>\n> Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>\n> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> \n> ---\n> No change in v9\n> \n> Changes in v8:\n> - remove readYaml and replace with yaml get\n>   - which makes this patch depend on \"ipa: libipa: Vector and Pwl\n>     improvements\"\n> - make default constructor make a zero matrix, and add an identity()\n>   static function\n> - add operator*=\n> - add operator* for reverse order of scalar-matrix product\n> - s/\\<R\\>/Rows/g, s/\\<C\\>/Cols/g\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  | 149 +++++++++++++++++++++++++++\n>  src/ipa/libipa/matrix.h    | 204 +++++++++++++++++++++++++++++++++++++\n>  src/ipa/libipa/meson.build |   2 +\n>  3 files changed, 355 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..4f49ac4d8b6c\n> --- /dev/null\n> +++ b/src/ipa/libipa/matrix.cpp\n> @@ -0,0 +1,149 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>\n> + *\n> + * Matrix and related operations\n> + */\n> +\n> +#include \"matrix.h\"\n> +\n> +#include <libcamera/base/log.h>\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 Rows Number of rows in the matrix\n> + * \\tparam Cols Number of columns in the matrix\n> + */\n> +\n> +/**\n> + * \\fn Matrix::Matrix()\n> + * \\brief Construct a zero matrix\n> + */\n> +\n> +/**\n> + * \\fn Matrix::Matrix(const std::vector<T> &data)\n> + * \\brief Construct a matrix from supplied data\n> + * \\param[in] data Data from which to construct a matrix\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\ns/RxC/Rows x Cols/\n\n> + */\n> +\n> +/**\n> + * \\fn Matrix::identity()\n> + * \\brief Construct an identity matrix\n> + */\n> +\n> +/**\n> + * \\fn Matrix::toString\n\ns/$/()/\n\n> + * \\brief Assemble and return a string describing the matrix\n> + * \\return A string describing the matrix\n> + */\n> +\n> +/**\n> + * \\fn Span<const T, Cols> Matrix::operator[](size_t i) const\n> + * \\brief Index to a row in the matrix\n> + * \\param[in] 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> + * \\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<T, Rows, Cols> &Matrix::operator*=(U d)\n> + * \\brief Scalar product\n\n * \\brief Multiply the matrix by a scalar in-place\n\n> + * \\tparam U Type of the numerical scalar value\n> + * \\param d Scalar\n> + * \\return Product of this matrix and scalar \\a d\n> + */\n> +\n> +/**\n> + * \\fn Matrix::Matrix<U, Rows, Cols> operator*(T d, const Matrix<U, Rows, Cols> &m)\n> + * \\brief Scalar product\n\n * \\brief Multiply the matrix by a scalar\n\n> + * \\tparam T Type of the numerical scalar value\n> + * \\tparam U Type of numerical values in the matrix\n> + * \\tparam Rows Number of rows in the matrix\n> + * \\tparam Cols 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::Matrix<U, Rows, Cols> operator*(const Matrix<U, Rows, Cols> &m, T d)\n> + * \\copydoc operator*(T d, const Matrix<U, Rows, Cols> &m)\n> + */\n> +\n> +/**\n> + * \\fn Matrix<T, R1, C2> operator*(const Matrix<T, R1, C1> &m1, const Matrix<T, R2, C2> &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<T, Rows, Cols> operator+(const Matrix<T, Rows, Cols> &m1, const Matrix<T, Rows, Cols> &m2)\n> + * \\brief Matrix addition\n> + * \\tparam T Type of numerical values in the matrices\n> + * \\tparam Rows Number of rows in the matrices\n> + * \\tparam Cols 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> +#ifndef __DOXYGEN__\n> +/*\n> + * The YAML data shall be a list of numerical values. Its size shall be equal\n> + * to the product of the number of rows and columns of the matrix (RxC). The\n\nRows x Cols here too.\n\n> + * values shall be stored in row-major order.\n> + */\n> +bool matrixValidateYaml(const YamlObject &obj, unsigned int size)\n> +{\n> +\tif (!obj.isList())\n> +\t\treturn false;\n> +\n> +\tif (obj.size() != size) {\n> +\t\tLOG(Matrix, Error)\n> +\t\t\t<< \"Wrong number of values in matrix: expected \"\n> +\t\t\t<< size << \", got \" << obj.size();\n> +\t\treturn false;\n> +\t}\n> +\n> +\treturn true;\n> +}\n> +#endif /* __DOXYGEN__ */\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..3a6b9b303acc\n> --- /dev/null\n> +++ b/src/ipa/libipa/matrix.h\n> @@ -0,0 +1,204 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>\n> + *\n> + * Matrix and related operations\n> + */\n> +#pragma once\n> +\n> +#include <algorithm>\n> +#include <cmath>\n> +#include <sstream>\n> +#include <vector>\n> +\n> +#include <libcamera/base/log.h>\n> +#include <libcamera/base/span.h>\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<typename T, unsigned int Rows, unsigned int Cols,\n> +\t std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>\n> +#else\n> +template<typename T, unsigned int Rows, unsigned int Cols>\n> +#endif /* __DOXYGEN__ */\n> +class Matrix\n> +{\n> +public:\n> +\tMatrix()\n> +\t{\n> +\t\tdata_.fill(static_cast<T>(0));\n> +\t}\n> +\n> +\tMatrix(const std::vector<T> &data)\n> +\t{\n> +\t\tstd::copy(data.begin(), data.end(), data_.begin());\n> +\t}\n> +\n> +\tstatic Matrix identity()\n> +\t{\n> +\t\tMatrix ret;\n> +\t\tfor (size_t i = 0; i < std::min(Rows, Cols); i++)\n> +\t\t\tret[i][i] = static_cast<T>(1);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\t~Matrix() = default;\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 < Rows; i++) {\n> +\t\t\tout << \"[ \";\n> +\t\t\tfor (unsigned int j = 0; j < Cols; j++) {\n> +\t\t\t\tout << (*this)[i][j];\n> +\t\t\t\tout << ((j + 1 < Cols) ? \", \" : \" \");\n> +\t\t\t}\n> +\t\t\tout << ((i + 1 < Rows) ? \"], \" : \"]\");\n> +\t\t}\n> +\t\tout << \" }\";\n> +\n> +\t\treturn out.str();\n> +\t}\n> +\n> +\tSpan<const T, Cols> operator[](size_t i) const\n> +\t{\n> +\t\treturn Span<const T, Cols>{ &data_.data()[i * Cols], Cols };\n> +\t}\n> +\n> +\tSpan<T, Cols> operator[](size_t i)\n> +\t{\n> +\t\treturn Span<T, Cols>{ &data_.data()[i * Cols], Cols };\n> +\t}\n> +\n> +#ifndef __DOXYGEN__\n> +\ttemplate<typename U, std::enable_if_t<std::is_arithmetic_v<U>>>\n> +#else\n> +\ttemplate<typename U>\n> +#endif /* __DOXYGEN__ */\n> +\tMatrix<T, Rows, Cols> &operator*=(U d)\n> +\t{\n> +\t\tfor (unsigned int i = 0; i < Rows * Cols; i++)\n> +\t\t\tdata_[i] *= d;\n> +\t\treturn *this;\n> +\t}\n> +\n> +private:\n> +\tstd::array<T, Rows * Cols> data_;\n> +};\n> +\n> +#ifndef __DOXYGEN__\n> +template<typename T, typename U, unsigned int Rows, unsigned int Cols,\n> +\t std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>\n> +#else\n> +template<typename T, typename U, unsigned int Rows, unsigned int Cols>\n> +#endif /* __DOXYGEN__ */\n> +Matrix<U, Rows, Cols> operator*(T d, const Matrix<U, Rows, Cols> &m)\n> +{\n> +\tMatrix<U, Rows, Cols> result;\n> +\n> +\tfor (unsigned int i = 0; i < Rows; i++)\n> +\t\tfor (unsigned int j = 0; j < Cols; j++)\n> +\t\t\tresult[i][j] = d * m[i][j];\n\nCurly braces for the outer loop.\n\n> +\n> +\treturn result;\n> +}\n> +\n> +#ifndef __DOXYGEN__\n> +template<typename T, typename U, unsigned int Rows, unsigned int Cols,\n> +\t std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>\n> +#else\n> +template<typename T, typename U, unsigned int Rows, unsigned int Cols>\n> +#endif /* __DOXYGEN__ */\n> +Matrix<U, Rows, Cols> operator*(const Matrix<U, Rows, Cols> &m, T d)\n> +{\n> +\treturn d * m;\n> +}\n> +\n> +#ifndef __DOXYGEN__\n> +template<typename T,\n> +\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> +#else\n> +template<typename T, unsigned int R1, unsigned int C1, unsigned int R2, unsigned in C2>\n> +#endif /* __DOXYGEN__ */\n> +Matrix<T, R1, C2> operator*(const Matrix<T, R1, C1> &m1, const Matrix<T, R2, C2> &m2)\n> +{\n> +\tMatrix<T, R1, C2> 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<typename T, unsigned int Rows, unsigned int Cols>\n> +#endif /* __DOXYGEN__ */\n\nNo need to hide the template from Doxygen.\n\n> +Matrix<T, Rows, Cols> operator+(const Matrix<T, Rows, Cols> &m1, const Matrix<T, Rows, Cols> &m2)\n> +{\n> +\tMatrix<T, Rows, Cols> result;\n> +\n> +\tfor (unsigned int i = 0; i < Rows; i++)\n> +\t\tfor (unsigned int j = 0; j < Cols; j++)\n> +\t\t\tresult[i][j] = m1[i][j] + m2[i][j];\n\nCurly braces for the outer loop.\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n> +\n> +\treturn result;\n> +}\n> +\n> +#ifndef __DOXYGEN__\n> +bool matrixValidateYaml(const YamlObject &obj, unsigned int size);\n> +#endif /* __DOXYGEN__ */\n> +\n> +} /* namespace ipa */\n> +\n> +#ifndef __DOXYGEN__\n> +template<typename T, unsigned int Rows, unsigned int Cols>\n> +std::ostream &operator<<(std::ostream &out, const ipa::Matrix<T, Rows, Cols> &m)\n> +{\n> +\tout << m.toString();\n> +\treturn out;\n> +}\n> +\n> +template<typename T, unsigned int Rows, unsigned int Cols>\n> +struct YamlObject::Getter<ipa::Matrix<T, Rows, Cols>> {\n> +\tstd::optional<ipa::Matrix<T, Rows, Cols>> get(const YamlObject &obj) const\n> +\t{\n> +\t\tif (!ipa::matrixValidateYaml(obj, Rows * Cols))\n> +\t\t\treturn std::nullopt;\n> +\n> +\t\tipa::Matrix<T, Rows, Cols> matrix;\n> +\t\tT *data = &matrix[0][0];\n> +\n> +\t\tunsigned int i = 0;\n> +\t\tfor (const YamlObject &entry : obj.asList()) {\n> +\t\t\tconst auto value = entry.get<T>();\n> +\t\t\tif (!value)\n> +\t\t\t\treturn std::nullopt;\n> +\n> +\t\t\tdata[i++] = *value;\n> +\t\t}\n> +\n> +\t\treturn matrix;\n> +\t}\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 496084233de7..1952dccf6236 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>      'pwl.h',\n>      'vector.h',\n> @@ -19,6 +20,7 @@ libipa_sources = files([\n>      'exposure_mode_helper.cpp',\n>      'fc_queue.cpp',\n>      'histogram.cpp',\n> +    'matrix.cpp',\n>      'module.cpp',\n>      'pwl.cpp',\n>      'vector.cpp',","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 6060FBD87C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSun, 16 Jun 2024 17:10:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 502CB6548D;\n\tSun, 16 Jun 2024 19:10:48 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2F52465456\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 16 Jun 2024 19:10:47 +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 925912D5;\n\tSun, 16 Jun 2024 19:10:30 +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=\"ROdpraxW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718557830;\n\tbh=fMbwhJfcu8oZdkBoQ6PQTFpy37vTjslVnH6m7WUmMKA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ROdpraxWPEsAhwLeHr2UWvkqh0HyGPiPe7Rui0KRF1OdAZlBolvRMzAOH5QSHJWaB\n\tuBp9vpcifZPb02W8kSZokXjdF2a2pZqzYfsRH+n3rQrSQp0r2tH7hr+w5ijRjvI0nm\n\tDeBsxHlz7tSdEzOMOmfZihB89iAIoU2UyoYTzrg8=","Date":"Sun, 16 Jun 2024 20:10:25 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Paul Elder <paul.elder@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org,\n\tStefan Klug <stefan.klug@ideasonboard.com>,\n\tKieran Bingham <kieran.bingham@ideasonboard.com>","Subject":"Re: [PATCH v9 1/3] ipa: libipa: Add Matrix class","Message-ID":"<20240616171025.GA7378@pendragon.ideasonboard.com>","References":"<20240614113757.109452-1-paul.elder@ideasonboard.com>\n\t<20240614113757.109452-2-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240614113757.109452-2-paul.elder@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]