[{"id":29126,"web_url":"https://patchwork.libcamera.org/comment/29126/","msgid":"","date":"2024-03-28T18:44:27","subject":"Re: [PATCH] ipa: libipa: Add Matrix class","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn 27/03/24 2:29 pm, Paul Elder wrote:\n> Add a class to represent a Matrix object and its corresponding\n> operations.\n\nI would just expand a bit on which operations are supported:\n\n- Matrix addition\n- Matrix multiplication by  scaler constant\n- Matrix-to-Matrix Multiplication\n>\n> Signed-off-by: Paul Elder \n> ---\n> src/ipa/libipa/matrix.cpp | 17 +++++\n> src/ipa/libipa/matrix.h | 138 +++++++++++++++++++++++++++++++++++++\n> src/ipa/libipa/meson.build | 2 +\n> 3 files changed, 157 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..1fd191c7\n> --- /dev/null\n> +++ b/src/ipa/libipa/matrix.h\n> @@ -0,0 +1,138 @@\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\nIs this header being used?\n> +#include \n> +#include \n> +#include \n\nSame question here\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> +\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> +\n> +\t\twhile (rows * cols < data.size())\n> +\t\t\tdata.push_back(0);\n> +\t}\n> +\n> +\tint readYaml(unsigned int _rows, unsigned int _cols,\n> +\t\t const libcamera::YamlObject &yaml)\n> +\t{\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 \" << *value;\n\n  This doesn't seem right, are you de-referencing null value  ?\n> +\t\t\t\treturn -EINVAL;\n> +\t\t\t}\n> +\t\t\tdata.push_back(*value);\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> +};\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> +}\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> +\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> ])","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 816BFBDCBD\n\tfor ;\n\tThu, 28 Mar 2024 18:44:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8CCB66333B;\n\tThu, 28 Mar 2024 19:44:34 +0100 (CET)","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 62D8C632EA\n\tfor ;\n\tThu, 28 Mar 2024 19:44:32 +0100 (CET)","from [192.168.1.105] (unknown [103.251.226.73])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AC1C2475;\n\tThu, 28 Mar 2024 19:43:58 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"mcNa9jre\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711651439;\n\tbh=ZZ03Vk6hxHKhv0xBx622ddam6RUej1LUQz53CgDMYzI=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=mcNa9jreNjeYCyOQpmDaFSuP6rFv05wtI6zSQvDV4Kgb8IpgtcO/rVr3wdlf0YJ+s\n\tDFvTQEjSGuIG9z0SR/7cqKfnn49TZuNjjs0xbMAWOuXBlNnEp60B1tbvV4U8uCC6Gl\n\tEQqCnZrJMKYDFZ/ADhbBpnQ9/i8BGtEPtaPre4wc=","Message-ID":"","Date":"Fri, 29 Mar 2024 00:14:27 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH] ipa: libipa: Add Matrix class","Content-Language":"en-US","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20240327085933.281180-1-paul.elder@ideasonboard.com>","From":"Umang Jain ","In-Reply-To":"<20240327085933.281180-1-paul.elder@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":29127,"web_url":"https://patchwork.libcamera.org/comment/29127/","msgid":"","date":"2024-03-29T10:15:15","subject":"Re: [PATCH] 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":"Hi Umang,\n\nThanks for the review.\n\nOn Fri, Mar 29, 2024 at 12:14:27AM +0530, Umang Jain wrote:\n> Hi Paul,\n> \n> Thank you for the patch.\n> \n> On 27/03/24 2:29 pm, Paul Elder wrote:\n> > Add a class to represent a Matrix object and its corresponding\n> > operations.\n> \n> I would just expand a bit on which operations are supported:\n> \n> - Matrix addition\n> - Matrix multiplication by  scaler constant\n> - Matrix-to-Matrix Multiplication\n> > \n> > Signed-off-by: Paul Elder \n> > ---\n> > src/ipa/libipa/matrix.cpp | 17 +++++\n> > src/ipa/libipa/matrix.h | 138 +++++++++++++++++++++++++++++++++++++\n> > src/ipa/libipa/meson.build | 2 +\n> > 3 files changed, 157 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..1fd191c7\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/matrix.h\n> > @@ -0,0 +1,138 @@\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> \n> Is this header being used?\n> > +#include \n> > +#include \n> > +#include \n> \n> Same question here\n\nOops, I forgot to remove these when I stopped using them.\n\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> > +\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> > +\n> > +\t\twhile (rows * cols < data.size())\n> > +\t\t\tdata.push_back(0);\n> > +\t}\n> > +\n> > +\tint readYaml(unsigned int _rows, unsigned int _cols,\n> > +\t\t const libcamera::YamlObject &yaml)\n> > +\t{\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 \" << *value;\n> \n>  This doesn't seem right, are you de-referencing null value  ?\n\nI don't think it's null (it's a std::optional) but still yeah it's not\nright.\n\n\nThanks,\n\nPaul\n\n> > +\t\t\t\treturn -EINVAL;\n> > +\t\t\t}\n> > +\t\t\tdata.push_back(*value);\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> > +};\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> > +}\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> > +\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>","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 9BB3DC0DA4\n\tfor ;\n\tFri, 29 Mar 2024 10:15:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4A3A86333B;\n\tFri, 29 Mar 2024 11:15:26 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 37FEE6308D\n\tfor ;\n\tFri, 29 Mar 2024 11:15:24 +0100 (CET)","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 DB7374D4;\n\tFri, 29 Mar 2024 11:14:48 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"SeKn72Lx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711707290;\n\tbh=OK9AmBojW+KZ8TNii8V7TIDnsPu+kpZAwGDVKUJ6f4U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SeKn72LxPA0+OoyyQHBPquBgmwGn1R1V6ETOxZcgN1L6fhoUoF+oudOTnXHWphjJC\n\txZCY0gNhBfLiCvN6Xr8klU0mnufO/YEd1j1DD0f7zNMllk0dPfn6jYJUjqLG2YXe6s\n\t8XiWGnKxbEDkN1aG/tyM6MAn/USj8WjcvqLd3JQw=","Date":"Fri, 29 Mar 2024 19:15:15 +0900","From":"Paul Elder ","To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH] ipa: libipa: Add Matrix class","Message-ID":"","References":"<20240327085933.281180-1-paul.elder@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","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\" "}}]