[{"id":29549,"web_url":"https://patchwork.libcamera.org/comment/29549/","msgid":"<20240517095640.vnxu3q762xtvle4s@jasper>","date":"2024-05-17T09:56:40","subject":"Re: [PATCH v4 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 Fri, May 17, 2024 at 05:01:27PM +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> \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 | 17 ++++\n> src/ipa/libipa/matrix.h | 161 +++++++++++++++++++++++++++++++++++++\n> src/ipa/libipa/meson.build | 2 +\n> 3 files changed, 180 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 000000000..c222c3f03\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 000000000..1bb794870\n> --- /dev/null\n> +++ b/src/ipa/libipa/matrix.h\n> @@ -0,0 +1,161 @@\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> +#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\t: data_(R * C, static_cast(false))\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> +\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() = 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\nAs operator<< is implemented now, the code could go there, and this\nfunction can be dropped, as there is no reason to keep it.\n\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\nI see, this was the last proposal from Barnabás. Actually, I like the\n(i,j) version better, as the span now points to data of the matrix, but\nits lifetime is not tied to the matrix... but that shouldn't be\na blocker.\n\nReviewed-by: Stefan Klug \n\nCheers,\nStefan\n\n\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> +\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[i][j] = d * m[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[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> +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[i][j] = m1[i][j] + m2[i][j];\n> +\n> +\treturn result;\n> +}\n> +\n> +} /* namespace ipa */\n> +\n> +template\n> +std::ostream &operator<<(std::ostream &out, const ipa::Matrix &m)\n> +{\n> +\tout << m.toString();\n> +\treturn out;\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index 1b3faf8d5..1e34355fe 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 4D4C4BDE6B\n\tfor ;\n\tFri, 17 May 2024 09:56:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8645F61A5A;\n\tFri, 17 May 2024 11:56:45 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C5BEC61A5A\n\tfor ;\n\tFri, 17 May 2024 11:56:43 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:d572:8aa2:3e8e:5b99])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8583782E;\n\tFri, 17 May 2024 11:56:34 +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=\"EV3HSrkW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1715939794;\n\tbh=8TzVztnFQVzBiRUaNsY37BDyaOrg2B+oLSefJ4PArYI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=EV3HSrkWXZjSvC6BCPh3A33Wn+UypPt400yy2kAyHZ/O7zbcCHuDbxgUDLl7VTxJ4\n\t1diUA4w56x876+mfxWJ1BedWIGN+mHpwIMAmjpsA5U2IaRaVOhAKSgPTkaccgPTVxf\n\t1uk8v/yHEBVZkrNb03fR6oY6/L9coOXfWlhZD4AI=","Date":"Fri, 17 May 2024 11:56:40 +0200","From":"Stefan Klug ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v4 1/3] ipa: libipa: Add Matrix class","Message-ID":"<20240517095640.vnxu3q762xtvle4s@jasper>","References":"<20240517080129.3876981-1-paul.elder@ideasonboard.com>\n\t<20240517080129.3876981-2-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20240517080129.3876981-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":29581,"web_url":"https://patchwork.libcamera.org/comment/29581/","msgid":"<05a97972-691f-4a4e-a544-8305530d55f3@ideasonboard.com>","date":"2024-05-20T14:08:37","subject":"Re: [PATCH v4 1/3] ipa: libipa: Add Matrix class","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Paul - thanks for the patch\n\nOn 17/05/2024 09:01, 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>\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 | 17 ++++\n> src/ipa/libipa/matrix.h | 161 +++++++++++++++++++++++++++++++++++++\n> src/ipa/libipa/meson.build | 2 +\n> 3 files changed, 180 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 000000000..c222c3f03\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\n\nDo we not need Doxygen comments in here?\n\n> diff --git a/src/ipa/libipa/matrix.h b/src/ipa/libipa/matrix.h\n> new file mode 100644\n> index 000000000..1bb794870\n> --- /dev/null\n> +++ b/src/ipa/libipa/matrix.h\n> @@ -0,0 +1,161 @@\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> +#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\t: data_(R * C, static_cast(false))\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> +\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() = 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> +\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[i][j] = d * m[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)\nDon't we want to restrict this operation to matrices with matching dimensions? I would expect a \nsingle set of R and C for both of the matrices.\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> +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[i][j] = m1[i][j] + m2[i][j];\n> +\n> +\treturn result;\n> +}\n> +\n> +} /* namespace ipa */\n> +\n> +template\n> +std::ostream &operator<<(std::ostream &out, const ipa::Matrix &m)\n> +{\n> +\tout << m.toString();\n> +\treturn out;\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index 1b3faf8d5..1e34355fe 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> ])","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 E6451BDE6B\n\tfor ;\n\tMon, 20 May 2024 14:08:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DAFC163483;\n\tMon, 20 May 2024 16:08:42 +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 91AFE61A57\n\tfor ;\n\tMon, 20 May 2024 16:08:41 +0200 (CEST)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0BC16D01\n\tfor ;\n\tMon, 20 May 2024 16:08:29 +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=\"R5ukX5HM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1716214110;\n\tbh=0t5muyDU/YKULOc5cy7B2I33AC6bPsyB58uIMk80Jjk=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=R5ukX5HMIAWKoXdy3t/Kez5YguU8bO9D+yhqkp6RuKoYc8n1MzdWb9xh8MWtjdMT3\n\tz2zBPQezNOaJCB47cNo/CmF5lhLzOcxfAyOTgrdKqrbAm2aQkK9UpallZkDdcleaiR\n\tAZR4h4Kw+U5NRakZn8TS7IQKG5TJAbfkG3Mtoz9Y=","Message-ID":"<05a97972-691f-4a4e-a544-8305530d55f3@ideasonboard.com>","Date":"Mon, 20 May 2024 15:08:37 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v4 1/3] ipa: libipa: Add Matrix class","To":"libcamera-devel@lists.libcamera.org","References":"<20240517080129.3876981-1-paul.elder@ideasonboard.com>\n\t<20240517080129.3876981-2-paul.elder@ideasonboard.com>","Content-Language":"en-US","From":"Dan Scally ","Autocrypt":"addr=dan.scally@ideasonboard.com; keydata=\n\txsFNBGLydlEBEADa5O2s0AbUguprfvXOQun/0a8y2Vk6BqkQALgeD6KnXSWwaoCULp18etYW\n\tB31bfgrdphXQ5kUQibB0ADK8DERB4wrzrUb5CMxLBFE7mQty+v5NsP0OFNK9XTaAOcmD+Ove\n\teIjYvqurAaro91jrRVrS1gBRxIFqyPgNvwwL+alMZhn3/2jU2uvBmuRrgnc/e9cHKiuT3Dtq\n\tMHGPKL2m+plk+7tjMoQFfexoQ1JKugHAjxAhJfrkXh6uS6rc01bYCyo7ybzg53m1HLFJdNGX\n\tsUKR+dQpBs3SY4s66tc1sREJqdYyTsSZf80HjIeJjU/hRunRo4NjRIJwhvnK1GyjOvvuCKVU\n\tRWpY8dNjNu5OeAfdrlvFJOxIE9M8JuYCQTMULqd1NuzbpFMjc9524U3Cngs589T7qUMPb1H1\n\tNTA81LmtJ6Y+IV5/kiTUANflpzBwhu18Ok7kGyCq2a2jsOcVmk8gZNs04gyjuj8JziYwwLbf\n\tvzABwpFVcS8aR+nHIZV1HtOzyw8CsL8OySc3K9y+Y0NRpziMRvutrppzgyMb9V+N31mK9Mxl\n\t1YkgaTl4ciNWpdfUe0yxH03OCuHi3922qhPLF4XX5LN+NaVw5Xz2o3eeWklXdouxwV7QlN33\n\tu4+u2FWzKxDqO6WLQGjxPE0mVB4Gh5Pa1Vb0ct9Ctg0qElvtGQARAQABzShEYW4gU2NhbGx5\n\tIDxkYW4uc2NhbGx5QGlkZWFzb25ib2FyZC5jb20+wsGNBBMBCAA3FiEEsdtt8OWP7+8SNfQe\n\tkiQuh/L+GMQFAmLydlIFCQWjmoACGwMECwkIBwUVCAkKCwUWAgMBAAAKCRCSJC6H8v4YxDI2\n\tEAC2Gz0iyaXJkPInyshrREEWbo0CA6v5KKf3I/HlMPqkZ48bmGoYm4mEQGFWZJAT3K4ir8bg\n\tcEfs9V54gpbrZvdwS4abXbUK4WjKwEs8HK3XJv1WXUN2bsz5oEJWZUImh9gD3naiLLI9QMMm\n\tw/aZkT+NbN5/2KvChRWhdcha7+2Te4foOY66nIM+pw2FZM6zIkInLLUik2zXOhaZtqdeJZQi\n\tHSPU9xu7TRYN4cvdZAnSpG7gQqmLm5/uGZN1/sB3kHTustQtSXKMaIcD/DMNI3JN/t+RJVS7\n\tc0Jh/ThzTmhHyhxx3DRnDIy7kwMI4CFvmhkVC2uNs9kWsj1DuX5kt8513mvfw2OcX9UnNKmZ\n\tnhNCuF6DxVrL8wjOPuIpiEj3V+K7DFF1Cxw1/yrLs8dYdYh8T8vCY2CHBMsqpESROnTazboh\n\tAiQ2xMN1cyXtX11Qwqm5U3sykpLbx2BcmUUUEAKNsM//Zn81QXKG8vOx0ZdMfnzsCaCzt8f6\n\t9dcDBBI3tJ0BI9ByiocqUoL6759LM8qm18x3FYlxvuOs4wSGPfRVaA4yh0pgI+ModVC2Pu3y\n\tejE/IxeatGqJHh6Y+iJzskdi27uFkRixl7YJZvPJAbEn7kzSi98u/5ReEA8Qhc8KO/B7wprj\n\txjNMZNYd0Eth8+WkixHYj752NT5qshKJXcyUU87BTQRi8nZSARAAx0BJayh1Fhwbf4zoY56x\n\txHEpT6DwdTAYAetd3yiKClLVJadYxOpuqyWa1bdfQWPb+h4MeXbWw/53PBgn7gI2EA7ebIRC\n\tPJJhAIkeym7hHZoxqDQTGDJjxFEL11qF+U3rhWiL2Zt0Pl+zFq0eWYYVNiXjsIS4FI2+4m16\n\ttPbDWZFJnSZ828VGtRDQdhXfx3zyVX21lVx1bX4/OZvIET7sVUufkE4hrbqrrufre7wsjD1t\n\t8MQKSapVrr1RltpzPpScdoxknOSBRwOvpp57pJJe5A0L7+WxJ+vQoQXj0j+5tmIWOAV1qBQp\n\thyoyUk9JpPfntk2EKnZHWaApFp5TcL6c5LhUvV7F6XwOjGPuGlZQCWXee9dr7zym8iR3irWT\n\t+49bIh5PMlqSLXJDYbuyFQHFxoiNdVvvf7etvGfqFYVMPVjipqfEQ38ST2nkzx+KBICz7uwj\n\tJwLBdTXzGFKHQNckGMl7F5QdO/35An/QcxBnHVMXqaSd12tkJmoRVWduwuuoFfkTY5mUV3uX\n\txGj3iVCK4V+ezOYA7c2YolfRCNMTza6vcK/P4tDjjsyBBZrCCzhBvd4VVsnnlZhVaIxoky4K\n\taL+AP+zcQrUZmXmgZjXOLryGnsaeoVrIFyrU6ly90s1y3KLoPsDaTBMtnOdwxPmo1xisH8oL\n\ta/VRgpFBfojLPxMAEQEAAcLBfAQYAQgAJhYhBLHbbfDlj+/vEjX0HpIkLofy/hjEBQJi8nZT\n\tBQkFo5qAAhsMAAoJEJIkLofy/hjEXPcQAMIPNqiWiz/HKu9W4QIf1OMUpKn3YkVIj3p3gvfM\n\tRes4fGX94Ji599uLNrPoxKyaytC4R6BTxVriTJjWK8mbo9jZIRM4vkwkZZ2bu98EweSucxbp\n\tvjESsvMXGgxniqV/RQ/3T7LABYRoIUutARYq58p5HwSP0frF0fdFHYdTa2g7MYZl1ur2JzOC\n\tFHRpGadlNzKDE3fEdoMobxHB3Lm6FDml5GyBAA8+dQYVI0oDwJ3gpZPZ0J5Vx9RbqXe8RDuR\n\tdu90hvCJkq7/tzSQ0GeD3BwXb9/R/A4dVXhaDd91Q1qQXidI+2jwhx8iqiYxbT+DoAUkQRQy\n\txBtoCM1CxH7u45URUgD//fxYr3D4B1SlonA6vdaEdHZOGwECnDpTxecENMbz/Bx7qfrmd901\n\tD+N9SjIwrbVhhSyUXYnSUb8F+9g2RDY42Sk7GcYxIeON4VzKqWM7hpkXZ47pkK0YodO+dRKM\n\tyMcoUWrTK0Uz6UzUGKoJVbxmSW/EJLEGoI5p3NWxWtScEVv8mO49gqQdrRIOheZycDmHnItt\n\t9Qjv00uFhEwv2YfiyGk6iGF2W40s2pH2t6oeuGgmiZ7g6d0MEK8Ql/4zPItvr1c1rpwpXUC1\n\tu1kQWgtnNjFHX3KiYdqjcZeRBiry1X0zY+4Y24wUU0KsEewJwjhmCKAsju1RpdlPg2kC","In-Reply-To":"<20240517080129.3876981-2-paul.elder@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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":29649,"web_url":"https://patchwork.libcamera.org/comment/29649/","msgid":"","date":"2024-05-29T07:59:11","subject":"Re: [PATCH v4 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 Mon, May 20, 2024 at 03:08:37PM +0100, Dan Scally wrote:\n> Hi Paul - thanks for the patch\n> \n> On 17/05/2024 09:01, 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> > \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 | 17 ++++\n> > src/ipa/libipa/matrix.h | 161 +++++++++++++++++++++++++++++++++++++\n> > src/ipa/libipa/meson.build | 2 +\n> > 3 files changed, 180 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 000000000..c222c3f03\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> \n> \n> Do we not need Doxygen comments in here?\n\nTechnically it's not required since it's not public API but it's\nprobably better to have them yeah...\n\n> \n> > diff --git a/src/ipa/libipa/matrix.h b/src/ipa/libipa/matrix.h\n> > new file mode 100644\n> > index 000000000..1bb794870\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/matrix.h\n> > @@ -0,0 +1,161 @@\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> > +#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\t: data_(R * C, static_cast(false))\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> > +\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() = 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> > +\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[i][j] = d * m[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> Don't we want to restrict this operation to matrices with matching\n> dimensions? I would expect a single set of R and C for both of the matrices.\n\nOn one hand yeah, but on the other hand I thought this was more\nthorough. SFINAE ought to protect us against non-matching dimensions.\n\n\nPaul\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> > +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[i][j] = m1[i][j] + m2[i][j];\n> > +\n> > +\treturn result;\n> > +}\n> > +\n> > +} /* namespace ipa */\n> > +\n> > +template\n> > +std::ostream &operator<<(std::ostream &out, const ipa::Matrix &m)\n> > +{\n> > +\tout << m.toString();\n> > +\treturn out;\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> > index 1b3faf8d5..1e34355fe 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> > ])","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 DD475BDE6B\n\tfor ;\n\tWed, 29 May 2024 07:59:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B6B3D634B2;\n\tWed, 29 May 2024 09:59:20 +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 DE44A61A46\n\tfor ;\n\tWed, 29 May 2024 09:59:18 +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 6F8B066F;\n\tWed, 29 May 2024 09:59:14 +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=\"uBIzFGxH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1716969555;\n\tbh=3iVj4ukmoQiL3tZiB7x6N+sfAnLVywGtHXdh6toCafY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uBIzFGxHTb93pCaHjOOalGRX9W3c7dowWXV4UHEd7n4zT8u7tm72ls0tMJEr2cw2q\n\tdJ9cg651n+o7hVEw2XSdNvASdzB1YuCB3YdfOGL+U2gcxL9jEwYuVFdUuX/cNmYr9N\n\tIafEImHN6xV/qPvdVDxT3SGrkkvzj2tWCnAK3Fa8=","Date":"Wed, 29 May 2024 16:59:11 +0900","From":"Paul Elder ","To":"Dan Scally ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v4 1/3] ipa: libipa: Add Matrix class","Message-ID":"","References":"<20240517080129.3876981-1-paul.elder@ideasonboard.com>\n\t<20240517080129.3876981-2-paul.elder@ideasonboard.com>\n\t<05a97972-691f-4a4e-a544-8305530d55f3@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<05a97972-691f-4a4e-a544-8305530d55f3@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\" "}}]