[{"id":29191,"web_url":"https://patchwork.libcamera.org/comment/29191/","msgid":"<20240410145313.677i3srnw26vogvt@macbook-air>","date":"2024-04-10T14:55:27","subject":"Re: [PATCH v2 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\nthank you for the patch. I know this is only a utility class so I might\nbe a bit too picky.\n\nOn Fri, Apr 05, 2024 at 05:40:48PM +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 v2:\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 | 136 +++++++++++++++++++++++++++++++++++++\n> src/ipa/libipa/meson.build | 2 +\n> 3 files changed, 155 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..f5520260\n> --- /dev/null\n> +++ b/src/ipa/libipa/matrix.h\n> @@ -0,0 +1,136 @@\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> +\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\n> +struct Matrix {\n> +public:\n> +\tMatrix(){};\n\nI would expect to get a unit matrix after construction. Ahh it doesn't\nhave a size yet. Then rows and cols should be properly initialized.\n\n> +\t~Matrix(){};\n> +\n> +\tMatrix(const unsigned int &_rows, const unsigned int &_cols,\n> +\t const std::vector &_data)\n> +\t\t: rows(_rows), cols(_cols)\n> +\t{\n> +\t\tfor (const T &x : _data)\n> +\t\t\tdata.push_back(x);\n\nAny reason not to use data.insert(data.end(), _data.begin(),\n_data.end())?\nThe size of _data should be checked, so that it's not too big.\n\n> +\n> +\t\twhile (rows * cols < data.size())\n> +\t\t\tdata.push_back(0);\n\nIs this expected? I would treat it as an error.\n\n> +\t}\n> +\n> +\tint readYaml(unsigned int _rows, unsigned int _cols,\n> +\t\t const libcamera::YamlObject &yaml)\n> +\t{\n\nWhy are the arguments to Matrix() passed by reference and these by\nvalue? They are equally const.\n\n> +\t\trows = _rows;\n> +\t\tcols = _cols;\n> +\n> +\t\tif (yaml.size() != rows * cols) {\n> +\t\t\tLOG(Matrix, Error)\n> +\t\t\t\t<< \"Wrong number of values in matrix: expected \"\n> +\t\t\t\t<< rows * cols << \", got \" << yaml.size();\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\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> +\t\t\tdata.push_back(*value);\n\nIf I get it right, this doesn't work when the matrix already had a size\nand data. That should work in all cases. It feels strange that a matrix\nsuddenly changes size. Would it make sense to move that into a static\nfactory function?\n\n> +\t\t}\n> +\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tconst std::string toString() const\n> +\t{\n> +\t\tstd::stringstream ss;\n> +\t\tss << \"Matrix { \";\n> +\t\tfor (unsigned int i = 0; i < rows; i++) {\n> +\t\t\tss << \"[ \";\n> +\t\t\tfor (unsigned int j = 0; j < cols; j++) {\n> +\t\t\t\tss << data[i * cols + j];\n> +\t\t\t\tss << ((j + 1 < cols) ? \", \" : \" \");\n> +\t\t\t}\n> +\t\t\tss << ((i + 1 < rows) ? \"], \" : \"]\");\n> +\t\t}\n> +\t\tss << \" }\";\n> +\n> +\t\treturn ss.str();\n> +\t}\n> +\n> +\tconst T &operator[](unsigned int i) const { return data.at(i); }\n> +\n> +\tunsigned int rows;\n> +\tunsigned int cols;\n> +\tstd::vector data;\n\nI think these should end with a underscore. And they should be\nprotected.\n\n> +};\n> +\n> +template\n> +Matrix operator*(T d, const Matrix &m)\n> +{\n> +\tstd::vector result(m.data.size());\n> +\n> +\tfor (const T &x : m.data)\n> +\t\tresult.push_back(d * x);\n> +\n> +\treturn Matrix(m.rows, m.cols, result);\n\nWhy the temporary vector? I somehow expected something\nlike (pseudocode):\n\nMatrix r(m.rows, m.cols);\n\nfor( int i=0; i +}\n> +\n> +template\n> +Matrix operator*(const Matrix &m1, const Matrix &m2)\n> +{\n> +\tASSERT(m1.cols == m2.rows);\n> +\n> +\tstd::vector result(m1.rows * m2.cols);\n> +\n> +\tfor (unsigned int i = 0; i < m1.rows; i++) {\n> +\t\tT sum = 0;\n> +\n> +\t\tfor (unsigned int j = 0; j < m2.cols; j++)\n> +\t\t\tsum += m1[i * m1.cols + j] * m2[j * m2.cols + i];\n\nReading that makes me think, that it would be nice to be able to do\nwrite \nsum += m1[j][i] * m2[i][j] \n\nBut that might be too much for a utility class. What about an accessor\nget(r,c) to remove the need to write down the calculation over and over\nagain?\nsum += m1.get(i,j) + m2.get(j,i)\n\nRegards,\nStefan\n\n> +\n> +\t\tresult.push_back(sum);\n> +\t}\n> +\n> +\treturn Matrix(m1.rows, m2.cols, result);\n> +}\n> +\n> +template\n> +Matrix operator+(const Matrix &m1, const Matrix &m2)\n> +{\n> +\tASSERT(m1.cols == m2.cols && m1.rows == m2.rows);\n> +\n> +\tunsigned int len = m1.rows * m1.cols;\n> +\tstd::vector result(len);\n> +\n> +\tfor (unsigned int i = 0; i < len; i++)\n> +\t\tresult[i] = m1[i] + m2[i];\n> +\n> +\treturn Matrix(m1.rows, m1.cols, 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 817a09cf..5d5ba5e5 100644\n> --- a/src/ipa/libipa/meson.build\n> +++ b/src/ipa/libipa/meson.build\n> @@ -6,6 +6,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> @@ -16,6 +17,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 07857BE08B\n\tfor ;\n\tWed, 10 Apr 2024 14:55:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 270FA63352;\n\tWed, 10 Apr 2024 16:55:31 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 665AC61B8D\n\tfor ;\n\tWed, 10 Apr 2024 16:55:29 +0200 (CEST)","from ideasonboard.com (unknown [62.91.42.92])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 122283A4;\n\tWed, 10 Apr 2024 16:54:47 +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=\"P1v/s6Kc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1712760887;\n\tbh=8IqRz/UQfPl3WqG8jpS8VutrS3DRsscwNIsQgLYmpzs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=P1v/s6Kczssjdil1PgRxsupm7AfpvEMktexiaU/sdhgMDjAacqvqMlkC/mgCKWJxZ\n\tQUIIjalXsyg+3UwBdWi57tIysS1dgD5oUsjEmolgpd1B3pwRYipZiC8Ksw+cbth0UL\n\tVYXRceQIC9gxommHVE9zQDjE5wOazAaNs48jN2PE=","Date":"Wed, 10 Apr 2024 16:55:27 +0200","From":"Stefan Klug ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 1/3] ipa: libipa: Add Matrix class","Message-ID":"<20240410145313.677i3srnw26vogvt@macbook-air>","References":"<20240405084050.1919105-1-paul.elder@ideasonboard.com>\n\t<20240405084050.1919105-2-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240405084050.1919105-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\" "}}]