[{"id":29540,"web_url":"https://patchwork.libcamera.org/comment/29540/","msgid":"<20240515084358.24ru6ryi5svudot6@jasper>","date":"2024-05-15T08:43:58","subject":"Re: [PATCH v3 1/3] ipa: libipa: Add Matrix class","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Paul,\n\nthanks for the patch.\n\nOn Tue, May 07, 2024 at 03:10:14PM +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> ---\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 | 17 +++++\n> src/ipa/libipa/matrix.h | 146 +++++++++++++++++++++++++++++++++++++\n> src/ipa/libipa/meson.build | 2 +\n> 3 files changed, 165 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 00000000..c222c3f0\n> --- /dev/null\n> +++ b/src/ipa/libipa/matrix.cpp\n> @@ -0,0 +1,17 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2019, Raspberry Pi Ltd\n> + * Copyright (C) 2024, Paul Elder \n> + *\n> + * matrix.cpp - Matrix and related operations\n> + */\n> +\n> +#include \"matrix.h\"\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(Matrix)\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 00000000..bbdb838b\n> --- /dev/null\n> +++ b/src/ipa/libipa/matrix.h\n> @@ -0,0 +1,146 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2019, Raspberry Pi Ltd\n> + * Copyright (C) 2024, Paul Elder \n> + *\n> + * matrix.cpp - Matrix and related operations\n> + */\n> +#pragma once\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\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> +template +\t std::enable_if_t> * = nullptr>\n> +class Matrix\n> +{\n> +public:\n> +\tMatrix()\n> +\t{\n> +\t\tdata_.clear();\n> +\t\tfor (unsigned int i = 0; i < R; i++)\n> +\t\t\tfor (unsigned int j = 0; j < C; j++)\n> +\t\t\t\tdata_.push_back(static_cast(i == j));\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> +\t}\n> +\n> +\t~Matrix(){};\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\nI don't recall if I mentioned that. For consistency you could move this\ninto a ostream& operator<<(...). \n\nAside from that I think we should get this in. We'll later see what else\nis needed.\n\nReviewed-by: Stefan Klug \n\nCheers,\nStefan\n\n> +\t{\n> +\t\tstd::stringstream ss;\n> +\t\tss << \"Matrix { \";\n> +\t\tfor (unsigned int i = 0; i < R; i++) {\n> +\t\t\tss << \"[ \";\n> +\t\t\tfor (unsigned int j = 0; j < C; j++) {\n> +\t\t\t\tss << get(i, j);\n> +\t\t\t\tss << ((j + 1 < C) ? \", \" : \" \");\n> +\t\t\t}\n> +\t\t\tss << ((i + 1 < R) ? \"], \" : \"]\");\n> +\t\t}\n> +\t\tss << \" }\";\n> +\n> +\t\treturn ss.str();\n> +\t}\n> +\n> +\tconst T &get(unsigned int i, unsigned int j) const { return data_.at(i * C + j); }\n> +\tvoid set(unsigned int i, unsigned int j, T value) { data_[i * C + j] = value; }\n> +\n> +private:\n> +\tstd::vector data_;\n> +};\n> +\n> +template +\t std::enable_if_t && std::is_arithmetic_v> * = nullptr>\n> +Matrix operator*(T d, const Matrix &m)\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.set(i, j, d * m.get(i, j));\n> +\n> +\treturn result;\n> +}\n> +\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> +Matrix operator*(const Matrix &m1, const Matrix &m2)\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.get(i, k) * m2.get(k, j);\n> +\n> +\t\t\tresult.set(i, j, sum);\n> +\t\t}\n> +\t}\n> +\n> +\treturn result;\n> +}\n> +\n> +template +\t std::enable_if_t> * = nullptr>\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.set(i, j, m1.get(i, j) + m2.get(i, j));\n> +\n> +\treturn result;\n> +}\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index 1b3faf8d..1e34355f 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> ])\n> @@ -18,6 +19,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> ])\n> -- \n> 2.39.2\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 DB822BDE6B\n\tfor ;\n\tWed, 15 May 2024 08:44:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1823463469;\n\tWed, 15 May 2024 10:44:04 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B937063466\n\tfor ;\n\tWed, 15 May 2024 10:44:01 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:f83e:cbfd:31ad:3642])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E07394D1;\n\tWed, 15 May 2024 10:43:53 +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=\"j/MSDYOW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1715762634;\n\tbh=IJCy7PHYyVUT94aTgdpnJTq/MmEw/Cca9ZtYIDvU5eE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=j/MSDYOW4yhcW69Wqqs0Og2+xSyKGd7H8BftCOfEy88HJ6dVtvRNnv3+bR4cLzkDj\n\tfcDWmCYToMDIqZWy+1uUwOi9UgYFIEORXNuXvf8wdETbayY7DZATcrJD/XJ4agkhHl\n\tK1xRtmWo1gHsXzaaKd9/M+ZDbp5Ogb9mFEvPVggI=","Date":"Wed, 15 May 2024 10:43:58 +0200","From":"Stefan Klug ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3 1/3] ipa: libipa: Add Matrix class","Message-ID":"<20240515084358.24ru6ryi5svudot6@jasper>","References":"<20240507061016.1716450-1-paul.elder@ideasonboard.com>\n\t<20240507061016.1716450-2-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240507061016.1716450-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":29542,"web_url":"https://patchwork.libcamera.org/comment/29542/","msgid":"","date":"2024-05-15T09:31:55","subject":"Re: [PATCH v3 1/3] ipa: libipa: Add Matrix class","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2024. május 7., kedd 8:10 keltezéssel, Paul Elder írta:\n\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> ---\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 | 17 +++++\n> src/ipa/libipa/matrix.h | 146 +++++++++++++++++++++++++++++++++++++\n> src/ipa/libipa/meson.build | 2 +\n> 3 files changed, 165 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 00000000..c222c3f0\n> --- /dev/null\n> +++ b/src/ipa/libipa/matrix.cpp\n> @@ -0,0 +1,17 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2019, Raspberry Pi Ltd\n> + * Copyright (C) 2024, Paul Elder \n> + *\n> + * matrix.cpp - Matrix and related operations\n> + */\n> +\n> +#include \"matrix.h\"\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(Matrix)\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 00000000..bbdb838b\n> --- /dev/null\n> +++ b/src/ipa/libipa/matrix.h\n> @@ -0,0 +1,146 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2019, Raspberry Pi Ltd\n> + * Copyright (C) 2024, Paul Elder \n> + *\n> + * matrix.cpp - Matrix and related operations\n> + */\n> +#pragma once\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\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> +template +\t std::enable_if_t> * = nullptr>\n> +class Matrix\n> +{\n> +public:\n> +\tMatrix()\n> +\t{\n> +\t\tdata_.clear();\n> +\t\tfor (unsigned int i = 0; i < R; i++)\n> +\t\t\tfor (unsigned int j = 0; j < C; j++)\n> +\t\t\t\tdata_.push_back(static_cast(i == j));\n\nMatrix()\n\t: data_(R * C, static_cast(false))\n{\n\tfor (std::size_t i = 0; i < std::min(R, C); i++)\n\t\t(*this)[i][i] = static_cast(true); // see below for indexing\n}\n\n\n> +\t};\n\n`;` not needed.\n\n\n> +\n> +\tMatrix(const std::vector &data)\n\nAny reason why `Span` (or just `Span`) is not acceptable?\n\n\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> +\t}\n> +\n> +\t~Matrix(){};\n\n`= default;` or just omit it completely please.\n\n\n> [...]\n> +\n> +\tconst T &get(unsigned int i, unsigned int j) const { return data_.at(i * C + j); }\n> +\tvoid set(unsigned int i, unsigned int j, T value) { data_[i * C + j] = value; }\n\nMaybe\n\n const T& operator()(std::size_t i, std::size_t j) const { return data[i * C + ]; }\n T& operator()(std::size_t i, std::size_t j) { return data[i * C + ]; }\n\n?\n\nI believe this is a pretty standard way to implement n-dimensional indexing in C++\nsince operator[] cannot have multiple arguments until C++23.\n\nOr I think the following also works, which might be \"better\":\n\n Span operator[](std::size_t i) const { return { &data[i * C], C }; }\n Span operator[](std::size_t i) { return { &data[i * C], C }; }\n\n\n> +\n> +private:\n> +\tstd::vector data_;\n> +};\n> [...]\n\n\nRegards,\nBarnabás Pőcze","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 D9B0CBDE6B\n\tfor ;\n\tWed, 15 May 2024 09:32:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6A45D6347E;\n\tWed, 15 May 2024 11:32:03 +0200 (CEST)","from mail-4322.protonmail.ch (mail-4322.protonmail.ch\n\t[185.70.43.22])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EA4BC63466\n\tfor ;\n\tWed, 15 May 2024 11:32:00 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"XT2bps0Z\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1715765520; x=1716024720;\n\tbh=zGlF2l/HlWxUBoexVcDFqHxQtBa8uTLq7Yxp+VvuULY=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector;\n\tb=XT2bps0Z3HbXpcd+IRW4Anqitlsof2Etbb+daiQguVa8fSvBCyz6jx0jA+rvnq0ed\n\tYDFHGJIL12KO5gajS6ZSHvHEldpoXukkaEwgrWDbILVvjs7hUtt9U52AqgH6EzOKwq\n\txosmeLovO4EEQ3obFtzLGMNHUjNZL/jUuAQQBufz91hQeqqoeQLQUTTLQe6Qj52Wyx\n\tcWm0RK7nlHx7ZmhWRLkJ01HdmE4Pr6o2Dqt42VgsnmB0/f9uBl1kHHVSfhD0nQ7TtJ\n\tM30r76Ik57RSlaaELQVB8NL/7hZt3FxHfMEv2OXhpRgaQ7RDb5qquqvpiaOoCS8MJB\n\tk6jt7ft4j34CA==","Date":"Wed, 15 May 2024 09:31:55 +0000","To":"Paul Elder ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3 1/3] ipa: libipa: Add Matrix class","Message-ID":"","In-Reply-To":"<20240507061016.1716450-2-paul.elder@ideasonboard.com>","References":"<20240507061016.1716450-1-paul.elder@ideasonboard.com>\n\t<20240507061016.1716450-2-paul.elder@ideasonboard.com>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"0e8d66a3f80ed451456ceb19fcb1a110d9545577","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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\" "}}]