[{"id":32240,"web_url":"https://patchwork.libcamera.org/comment/32240/","msgid":"<20241118155913.GS31681@pendragon.ideasonboard.com>","date":"2024-11-18T15:59:13","subject":"Re: [PATCH 2/4] libcamera: Enable and use matrix implementation from\n\tlibcamera/internal","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Stefan,\n\nThank you for the patch.\n\nOn Mon, Nov 18, 2024 at 04:05:05PM +0100, Stefan Klug wrote:\n> The matrix code copied from libipa needs to be adapted to compile and\n> work from the new location. To keep the project buildable at all times,\n> these changes are not split into multiple commits but kept as a single\n> one.\n\nIf you first rename the Matrix class in the RPi IPA to Matrix3x3 then\nyou will be able to split this patch better, with the rework of the\nMatrix class (and wiring up to the build system) in separate patches..\n\n> The changes are:\n> - Add matrix class to meson.build\n> - Move matrix class from libipa namespace into libcamera\n> - Add std::initializer_list based constructor to be able to replace the\n> Matrix implementation contained in the rpi pipeline\n> - Replace Matrix class in rpi pipeline with the new one to prevent name\n> clashes\n> \n> Signed-off-by: Stefan Klug \n> ---\n> include/libcamera/internal/matrix.h | 20 ++++-----\n> include/libcamera/internal/meson.build | 1 +\n> src/ipa/rpi/controller/rpi/ccm.cpp | 56 +++++++-------------------\n> src/ipa/rpi/controller/rpi/ccm.h | 35 +---------------\n> src/libcamera/matrix.cpp | 8 +---\n> src/libcamera/meson.build | 1 +\n> 6 files changed, 32 insertions(+), 89 deletions(-)\n> \n> diff --git a/include/libcamera/internal/matrix.h b/include/libcamera/internal/matrix.h\n> index 5471e6975b74..b82d33f98658 100644\n> --- a/include/libcamera/internal/matrix.h\n> +++ b/include/libcamera/internal/matrix.h\n> @@ -19,8 +19,6 @@ namespace libcamera {\n> \n> LOG_DECLARE_CATEGORY(Matrix)\n> \n> -namespace ipa {\n> -\n> #ifndef __DOXYGEN__\n> template \t std::enable_if_t> * = nullptr>\n> @@ -35,6 +33,12 @@ public:\n> \t\tdata_.fill(static_cast(0));\n> \t}\n> \n> +\tMatrix(std::initializer_list data)\n> +\t{\n> +\t\tASSERT(data.size() == Rows * Cols);\n\nI considered adding a similar constructor to Vector in my latest series,\nbut it's problematic for two reasons. One is that the ASSERT() will only\ntrigger at runtime, while this should be a compile-time check. We\ncoulduse static_assert() instead (and mark the constructor as\nconstexpr):\n\nconstexpr Matrix(std::initializer_list data)\n{\n\tstatic_assert(data.size() == Rows * Cols);\n\tstd::copy(data.begin(), data.end(), data_.begin());\n}\n\nBut this will only work if the initializer list is a compile-time\nconstant. The following will compile fine:\n\nvoid foo()\n{\n\tMatrix m{ 1.0, 2.0, 3.0, 4.0 };\n\t...\n}\n\nBut this won't:\n\nvoid foo(double v)\n{\n\tMatrix m{ v, v, v, v };\n\t...\n}\n\nas the initializer list isn't a compile-time constant.\n\nThis is why I didn't add a constructor taking an initializer_list, but\ninstead rely on the constructor taking an std::array. For the Matrix\nclass, this would be\n\nconstexpr Matrix(const std::array &data)\n{\n\tstd::copy(data.begin(), data.end(), data_.begin());\n}\n\nThe user needs to be slightly adapted (note the double curly braces):\n\nvoid foo(double v)\n{\n\tMatrix m{{ v, v, v, v }};\n\t...\n}\n\nThe addition of the new constructor could go to a separate patch, before\nthis one.\n\n> +\t\tstd::copy(data.begin(), data.end(), data_.begin());\n> +\t}\n> +\n> \tMatrix(const std::vector &data)\n> \t{\n> \t\tstd::copy(data.begin(), data.end(), data_.begin());\n\nThis seems very unsafe :-S Let's see if the constructor taking an\nstd::array could replace this one.\n\n> @@ -166,24 +170,22 @@ Matrix operator+(const Matrix &m1, const Matrix bool matrixValidateYaml(const YamlObject &obj, unsigned int size);\n> #endif /* __DOXYGEN__ */\n> \n> -} /* namespace ipa */\n> -\n> #ifndef __DOXYGEN__\n> template\n> -std::ostream &operator<<(std::ostream &out, const ipa::Matrix &m)\n> +std::ostream &operator<<(std::ostream &out, const Matrix &m)\n> {\n> \tout << m.toString();\n> \treturn out;\n> }\n> \n> template\n> -struct YamlObject::Getter> {\n> -\tstd::optional> get(const YamlObject &obj) const\n> +struct YamlObject::Getter> {\n> +\tstd::optional> get(const YamlObject &obj) const\n> \t{\n> -\t\tif (!ipa::matrixValidateYaml(obj, Rows * Cols))\n> +\t\tif (!matrixValidateYaml(obj, Rows * Cols))\n> \t\t\treturn std::nullopt;\n> \n> -\t\tipa::Matrix matrix;\n> +\t\tMatrix matrix;\n> \t\tT *data = &matrix[0][0];\n> \n> \t\tunsigned int i = 0;\n> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> index 1dddcd50c90b..7d6aa8b72bd7 100644\n> --- a/include/libcamera/internal/meson.build\n> +++ b/include/libcamera/internal/meson.build\n> @@ -29,6 +29,7 @@ libcamera_internal_headers = files([\n> 'ipc_pipe.h',\n> 'ipc_unixsocket.h',\n> 'mapped_framebuffer.h',\n> + 'matrix.h',\n> 'media_device.h',\n> 'media_object.h',\n> 'pipeline_handler.h',\n> diff --git a/src/ipa/rpi/controller/rpi/ccm.cpp b/src/ipa/rpi/controller/rpi/ccm.cpp\n> index aefa580c9a4b..418bead56ecd 100644\n> --- a/src/ipa/rpi/controller/rpi/ccm.cpp\n> +++ b/src/ipa/rpi/controller/rpi/ccm.cpp\n> @@ -29,34 +29,7 @@ LOG_DEFINE_CATEGORY(RPiCcm)\n> \n> #define NAME \"rpi.ccm\"\n> \n> -Matrix::Matrix()\n> -{\n> -\tmemset(m, 0, sizeof(m));\n> -}\n> -Matrix::Matrix(double m0, double m1, double m2, double m3, double m4, double m5,\n> -\t double m6, double m7, double m8)\n> -{\n> -\tm[0][0] = m0, m[0][1] = m1, m[0][2] = m2, m[1][0] = m3, m[1][1] = m4,\n> -\tm[1][2] = m5, m[2][0] = m6, m[2][1] = m7, m[2][2] = m8;\n> -}\n> -int Matrix::read(const libcamera::YamlObject ¶ms)\n> -{\n> -\tdouble *ptr = (double *)m;\n> -\n> -\tif (params.size() != 9) {\n> -\t\tLOG(RPiCcm, Error) << \"Wrong number of values in CCM\";\n> -\t\treturn -EINVAL;\n> -\t}\n> -\n> -\tfor (const auto ¶m : params.asList()) {\n> -\t\tauto value = param.get();\n> -\t\tif (!value)\n> -\t\t\treturn -EINVAL;\n> -\t\t*ptr++ = *value;\n> -\t}\n> -\n> -\treturn 0;\n> -}\n> +using Matrix3x3 = Matrix;\n> \n> Ccm::Ccm(Controller *controller)\n> \t: CcmAlgorithm(controller), saturation_(1.0) {}\n> @@ -68,8 +41,6 @@ char const *Ccm::name() const\n> \n> int Ccm::read(const libcamera::YamlObject ¶ms)\n> {\n> -\tint ret;\n> -\n> \tif (params.contains(\"saturation\")) {\n> \t\tconfig_.saturation = params[\"saturation\"].get(ipa::Pwl{});\n> \t\tif (config_.saturation.empty())\n> @@ -83,9 +54,12 @@ int Ccm::read(const libcamera::YamlObject ¶ms)\n> \n> \t\tCtCcm ctCcm;\n> \t\tctCcm.ct = *value;\n> -\t\tret = ctCcm.ccm.read(p[\"ccm\"]);\n> -\t\tif (ret)\n> -\t\t\treturn ret;\n> +\n> +\t\tauto ccm = p[\"ccm\"].get();\n> +\t\tif (!ccm)\n> +\t\t\treturn -EINVAL;\n> +\n> +\t\tctCcm.ccm = *ccm;\n> \n> \t\tif (!config_.ccms.empty() && ctCcm.ct <= config_.ccms.back().ct) {\n> \t\t\tLOG(RPiCcm, Error)\n> @@ -125,7 +99,7 @@ bool getLocked(Metadata *metadata, std::string const &tag, T &value)\n> \treturn true;\n> }\n> \n> -Matrix calculateCcm(std::vector const &ccms, double ct)\n> +Matrix3x3 calculateCcm(std::vector const &ccms, double ct)\n> {\n> \tif (ct <= ccms.front().ct)\n> \t\treturn ccms.front().ccm;\n> @@ -141,13 +115,13 @@ Matrix calculateCcm(std::vector const &ccms, double ct)\n> \t}\n> }\n> \n> -Matrix applySaturation(Matrix const &ccm, double saturation)\n> +Matrix3x3 applySaturation(Matrix3x3 const &ccm, double saturation)\n> {\n> -\tMatrix RGB2Y(0.299, 0.587, 0.114, -0.169, -0.331, 0.500, 0.500, -0.419,\n> -\t\t -0.081);\n> -\tMatrix Y2RGB(1.000, 0.000, 1.402, 1.000, -0.345, -0.714, 1.000, 1.771,\n> -\t\t 0.000);\n> -\tMatrix S(1, 0, 0, 0, saturation, 0, 0, 0, saturation);\n> +\tMatrix3x3 RGB2Y({ 0.299, 0.587, 0.114, -0.169, -0.331, 0.500, 0.500,\n> +\t\t\t -0.419, -0.081 });\n> +\tMatrix3x3 Y2RGB({ 1.000, 0.000, 1.402, 1.000, -0.345, -0.714, 1.000,\n> +\t\t\t 1.771, 0.000 });\n\nWhile at it, these two should be static const.\n\n> +\tMatrix3x3 S({ 1, 0, 0, 0, saturation, 0, 0, 0, saturation });\n> \treturn Y2RGB * S * RGB2Y * ccm;\n> }\n> \n> @@ -181,7 +155,7 @@ void Ccm::prepare(Metadata *imageMetadata)\n> \tfor (int j = 0; j < 3; j++)\n> \t\tfor (int i = 0; i < 3; i++)\n> \t\t\tccmStatus.matrix[j * 3 + i] =\n> -\t\t\t\tstd::max(-8.0, std::min(7.9999, ccm.m[j][i]));\n> +\t\t\t\tstd::max(-8.0, std::min(7.9999, ccm[j][i]));\n> \tLOG(RPiCcm, Debug)\n> \t\t<< \"colour temperature \" << awb.temperatureK << \"K\";\n> \tLOG(RPiCcm, Debug)\n> diff --git a/src/ipa/rpi/controller/rpi/ccm.h b/src/ipa/rpi/controller/rpi/ccm.h\n> index 4e5b33fefa4e..c05dbb17a264 100644\n> --- a/src/ipa/rpi/controller/rpi/ccm.h\n> +++ b/src/ipa/rpi/controller/rpi/ccm.h\n> @@ -8,6 +8,7 @@\n> \n> #include \n> \n> +#include \"libcamera/internal/matrix.h\"\n\nMissing blank line.\n\n> #include \n> \n> #include \"../ccm_algorithm.h\"\n> @@ -16,41 +17,9 @@ namespace RPiController {\n> \n> /* Algorithm to calculate colour matrix. Should be placed after AWB. */\n> \n> -struct Matrix {\n> -\tMatrix(double m0, double m1, double m2, double m3, double m4, double m5,\n> -\t double m6, double m7, double m8);\n> -\tMatrix();\n> -\tdouble m[3][3];\n> -\tint read(const libcamera::YamlObject ¶ms);\n> -};\n> -static inline Matrix operator*(double d, Matrix const &m)\n> -{\n> -\treturn Matrix(m.m[0][0] * d, m.m[0][1] * d, m.m[0][2] * d,\n> -\t\t m.m[1][0] * d, m.m[1][1] * d, m.m[1][2] * d,\n> -\t\t m.m[2][0] * d, m.m[2][1] * d, m.m[2][2] * d);\n> -}\n> -static inline Matrix operator*(Matrix const &m1, Matrix const &m2)\n> -{\n> -\tMatrix m;\n> -\tfor (int i = 0; i < 3; i++)\n> -\t\tfor (int j = 0; j < 3; j++)\n> -\t\t\tm.m[i][j] = m1.m[i][0] * m2.m[0][j] +\n> -\t\t\t\t m1.m[i][1] * m2.m[1][j] +\n> -\t\t\t\t m1.m[i][2] * m2.m[2][j];\n> -\treturn m;\n> -}\n> -static inline Matrix operator+(Matrix const &m1, Matrix const &m2)\n> -{\n> -\tMatrix m;\n> -\tfor (int i = 0; i < 3; i++)\n> -\t\tfor (int j = 0; j < 3; j++)\n> -\t\t\tm.m[i][j] = m1.m[i][j] + m2.m[i][j];\n> -\treturn m;\n> -}\n> -\n> struct CtCcm {\n> \tdouble ct;\n> -\tMatrix ccm;\n> +\tlibcamera::Matrix ccm;\n> };\n> \n> struct CcmConfig {\n> diff --git a/src/libcamera/matrix.cpp b/src/libcamera/matrix.cpp\n> index 8346f0d34160..c3ed54895b22 100644\n> --- a/src/libcamera/matrix.cpp\n> +++ b/src/libcamera/matrix.cpp\n> @@ -5,7 +5,7 @@\n> * Matrix and related operations\n> */\n> \n> -#include \"matrix.h\"\n> +#include \"libcamera/internal/matrix.h\"\n> \n> #include \n> \n> @@ -18,8 +18,6 @@ namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(Matrix)\n> \n> -namespace ipa {\n> -\n> /**\n> * \\class Matrix\n> * \\brief Matrix class\n> @@ -34,7 +32,7 @@ namespace ipa {\n> */\n> \n> /**\n> - * \\fn Matrix::Matrix(const std::vector &data)\n> + * \\fn Matrix::Matrix(const Span &data)\n> * \\brief Construct a matrix from supplied data\n> * \\param[in] data Data from which to construct a matrix\n> *\n> @@ -144,6 +142,4 @@ bool matrixValidateYaml(const YamlObject &obj, unsigned int size)\n> }\n> #endif /* __DOXYGEN__ */\n> \n> -} /* namespace ipa */\n> -\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 21cae11756d6..57fde8a8fab0 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -40,6 +40,7 @@ libcamera_internal_sources = files([\n> 'ipc_pipe_unixsocket.cpp',\n> 'ipc_unixsocket.cpp',\n> 'mapped_framebuffer.cpp',\n> + 'matrix.cpp',\n> 'media_device.cpp',\n> 'media_object.cpp',\n> 'pipeline_handler.cpp',","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 1FC20C32DD\n\tfor ;\n\tMon, 18 Nov 2024 15:59:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F2085658E6;\n\tMon, 18 Nov 2024 16:59:23 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0C754658DC\n\tfor ;\n\tMon, 18 Nov 2024 16:59:22 +0100 (CET)","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 1742FB2B;\n\tMon, 18 Nov 2024 16:59:05 +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=\"PkO87KtD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1731945545;\n\tbh=rSWrUHwC7181rmvi0/XtGCPVjyJorOla8OXAckrapSY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PkO87KtDi/trKtMspSCrvQIh2Dj1InypYkPy+QTNGek2rVmqnemN1S9uvBYdpTWiW\n\tVifx4uACEuteNEonLtn+1BTmiLYAHAlMNUgGq65dT+PdS/V88p2mLUXiiQCwjxFD7C\n\ttL/8BbZxxNQsguRtFasc4PWjZ050K3WDwj7W7GV0=","Date":"Mon, 18 Nov 2024 17:59:13 +0200","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 2/4] libcamera: Enable and use matrix implementation from\n\tlibcamera/internal","Message-ID":"<20241118155913.GS31681@pendragon.ideasonboard.com>","References":"<20241118150528.1856797-1-stefan.klug@ideasonboard.com>\n\t<20241118150528.1856797-3-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241118150528.1856797-3-stefan.klug@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":32244,"web_url":"https://patchwork.libcamera.org/comment/32244/","msgid":"","date":"2024-11-18T16:52:41","subject":"Re: [PATCH 2/4] libcamera: Enable and use matrix implementation from\n\tlibcamera/internal","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for the review. \n\nOn Mon, Nov 18, 2024 at 05:59:13PM +0200, Laurent Pinchart wrote:\n> Hi Stefan,\n> \n> Thank you for the patch.\n> \n> On Mon, Nov 18, 2024 at 04:05:05PM +0100, Stefan Klug wrote:\n> > The matrix code copied from libipa needs to be adapted to compile and\n> > work from the new location. To keep the project buildable at all times,\n> > these changes are not split into multiple commits but kept as a single\n> > one.\n> \n> If you first rename the Matrix class in the RPi IPA to Matrix3x3 then\n> you will be able to split this patch better, with the rework of the\n> Matrix class (and wiring up to the build system) in separate patches..\n\nIs that really worth the effort? But yes, I can do it.\n\n> \n> > The changes are:\n> > - Add matrix class to meson.build\n> > - Move matrix class from libipa namespace into libcamera\n> > - Add std::initializer_list based constructor to be able to replace the\n> > Matrix implementation contained in the rpi pipeline\n> > - Replace Matrix class in rpi pipeline with the new one to prevent name\n> > clashes\n> > \n> > Signed-off-by: Stefan Klug \n> > ---\n> > include/libcamera/internal/matrix.h | 20 ++++-----\n> > include/libcamera/internal/meson.build | 1 +\n> > src/ipa/rpi/controller/rpi/ccm.cpp | 56 +++++++-------------------\n> > src/ipa/rpi/controller/rpi/ccm.h | 35 +---------------\n> > src/libcamera/matrix.cpp | 8 +---\n> > src/libcamera/meson.build | 1 +\n> > 6 files changed, 32 insertions(+), 89 deletions(-)\n> > \n> > diff --git a/include/libcamera/internal/matrix.h b/include/libcamera/internal/matrix.h\n> > index 5471e6975b74..b82d33f98658 100644\n> > --- a/include/libcamera/internal/matrix.h\n> > +++ b/include/libcamera/internal/matrix.h\n> > @@ -19,8 +19,6 @@ namespace libcamera {\n> > \n> > LOG_DECLARE_CATEGORY(Matrix)\n> > \n> > -namespace ipa {\n> > -\n> > #ifndef __DOXYGEN__\n> > template > \t std::enable_if_t> * = nullptr>\n> > @@ -35,6 +33,12 @@ public:\n> > \t\tdata_.fill(static_cast(0));\n> > \t}\n> > \n> > +\tMatrix(std::initializer_list data)\n> > +\t{\n> > +\t\tASSERT(data.size() == Rows * Cols);\n> \n> I considered adding a similar constructor to Vector in my latest series,\n> but it's problematic for two reasons. One is that the ASSERT() will only\n> trigger at runtime, while this should be a compile-time check. We\n> coulduse static_assert() instead (and mark the constructor as\n> constexpr):\n> \n> constexpr Matrix(std::initializer_list data)\n> {\n> \tstatic_assert(data.size() == Rows * Cols);\n> \tstd::copy(data.begin(), data.end(), data_.begin());\n> }\n> \n> But this will only work if the initializer list is a compile-time\n> constant. The following will compile fine:\n> \n> void foo()\n> {\n> \tMatrix m{ 1.0, 2.0, 3.0, 4.0 };\n> \t...\n> }\n> \n> But this won't:\n> \n> void foo(double v)\n> {\n> \tMatrix m{ v, v, v, v };\n> \t...\n> }\n> \n> as the initializer list isn't a compile-time constant.\n> \n> This is why I didn't add a constructor taking an initializer_list, but\n> instead rely on the constructor taking an std::array. For the Matrix\n> class, this would be\n> \n> constexpr Matrix(const std::array &data)\n> {\n> \tstd::copy(data.begin(), data.end(), data_.begin());\n> }\n> \n> The user needs to be slightly adapted (note the double curly braces):\n> \n> void foo(double v)\n> {\n> \tMatrix m{{ v, v, v, v }};\n\nI had exactly the same process (static_assert etc.). Just my conclusion\nwas a bit different. I would rate the syntax (the double braces are\nunexpected and require too much knowledge on the internals) over the\nmissing compile time check (as the assert is on a pretty likely path and\nwill happen during testing). So I chose the initializer_list. Now I\ntried the array and funny enough, there is no need to switch to double\nbraces - even with our old compilers. They manage to fold it.\n\n> \t...\n> }\n> \n> The addition of the new constructor could go to a separate patch, before\n> this one.\n> \n> > +\t\tstd::copy(data.begin(), data.end(), data_.begin());\n> > +\t}\n> > +\n> > \tMatrix(const std::vector &data)\n> > \t{\n> > \t\tstd::copy(data.begin(), data.end(), data_.begin());\n> \n> This seems very unsafe :-S Let's see if the constructor taking an\n> std::array could replace this one.\n\nI already read your comment on the other patch, so I'll remove it.\n\nCheers,\nStefan\n\n> \n> > @@ -166,24 +170,22 @@ Matrix operator+(const Matrix &m1, const Matrix > bool matrixValidateYaml(const YamlObject &obj, unsigned int size);\n> > #endif /* __DOXYGEN__ */\n> > \n> > -} /* namespace ipa */\n> > -\n> > #ifndef __DOXYGEN__\n> > template\n> > -std::ostream &operator<<(std::ostream &out, const ipa::Matrix &m)\n> > +std::ostream &operator<<(std::ostream &out, const Matrix &m)\n> > {\n> > \tout << m.toString();\n> > \treturn out;\n> > }\n> > \n> > template\n> > -struct YamlObject::Getter> {\n> > -\tstd::optional> get(const YamlObject &obj) const\n> > +struct YamlObject::Getter> {\n> > +\tstd::optional> get(const YamlObject &obj) const\n> > \t{\n> > -\t\tif (!ipa::matrixValidateYaml(obj, Rows * Cols))\n> > +\t\tif (!matrixValidateYaml(obj, Rows * Cols))\n> > \t\t\treturn std::nullopt;\n> > \n> > -\t\tipa::Matrix matrix;\n> > +\t\tMatrix matrix;\n> > \t\tT *data = &matrix[0][0];\n> > \n> > \t\tunsigned int i = 0;\n> > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> > index 1dddcd50c90b..7d6aa8b72bd7 100644\n> > --- a/include/libcamera/internal/meson.build\n> > +++ b/include/libcamera/internal/meson.build\n> > @@ -29,6 +29,7 @@ libcamera_internal_headers = files([\n> > 'ipc_pipe.h',\n> > 'ipc_unixsocket.h',\n> > 'mapped_framebuffer.h',\n> > + 'matrix.h',\n> > 'media_device.h',\n> > 'media_object.h',\n> > 'pipeline_handler.h',\n> > diff --git a/src/ipa/rpi/controller/rpi/ccm.cpp b/src/ipa/rpi/controller/rpi/ccm.cpp\n> > index aefa580c9a4b..418bead56ecd 100644\n> > --- a/src/ipa/rpi/controller/rpi/ccm.cpp\n> > +++ b/src/ipa/rpi/controller/rpi/ccm.cpp\n> > @@ -29,34 +29,7 @@ LOG_DEFINE_CATEGORY(RPiCcm)\n> > \n> > #define NAME \"rpi.ccm\"\n> > \n> > -Matrix::Matrix()\n> > -{\n> > -\tmemset(m, 0, sizeof(m));\n> > -}\n> > -Matrix::Matrix(double m0, double m1, double m2, double m3, double m4, double m5,\n> > -\t double m6, double m7, double m8)\n> > -{\n> > -\tm[0][0] = m0, m[0][1] = m1, m[0][2] = m2, m[1][0] = m3, m[1][1] = m4,\n> > -\tm[1][2] = m5, m[2][0] = m6, m[2][1] = m7, m[2][2] = m8;\n> > -}\n> > -int Matrix::read(const libcamera::YamlObject ¶ms)\n> > -{\n> > -\tdouble *ptr = (double *)m;\n> > -\n> > -\tif (params.size() != 9) {\n> > -\t\tLOG(RPiCcm, Error) << \"Wrong number of values in CCM\";\n> > -\t\treturn -EINVAL;\n> > -\t}\n> > -\n> > -\tfor (const auto ¶m : params.asList()) {\n> > -\t\tauto value = param.get();\n> > -\t\tif (!value)\n> > -\t\t\treturn -EINVAL;\n> > -\t\t*ptr++ = *value;\n> > -\t}\n> > -\n> > -\treturn 0;\n> > -}\n> > +using Matrix3x3 = Matrix;\n> > \n> > Ccm::Ccm(Controller *controller)\n> > \t: CcmAlgorithm(controller), saturation_(1.0) {}\n> > @@ -68,8 +41,6 @@ char const *Ccm::name() const\n> > \n> > int Ccm::read(const libcamera::YamlObject ¶ms)\n> > {\n> > -\tint ret;\n> > -\n> > \tif (params.contains(\"saturation\")) {\n> > \t\tconfig_.saturation = params[\"saturation\"].get(ipa::Pwl{});\n> > \t\tif (config_.saturation.empty())\n> > @@ -83,9 +54,12 @@ int Ccm::read(const libcamera::YamlObject ¶ms)\n> > \n> > \t\tCtCcm ctCcm;\n> > \t\tctCcm.ct = *value;\n> > -\t\tret = ctCcm.ccm.read(p[\"ccm\"]);\n> > -\t\tif (ret)\n> > -\t\t\treturn ret;\n> > +\n> > +\t\tauto ccm = p[\"ccm\"].get();\n> > +\t\tif (!ccm)\n> > +\t\t\treturn -EINVAL;\n> > +\n> > +\t\tctCcm.ccm = *ccm;\n> > \n> > \t\tif (!config_.ccms.empty() && ctCcm.ct <= config_.ccms.back().ct) {\n> > \t\t\tLOG(RPiCcm, Error)\n> > @@ -125,7 +99,7 @@ bool getLocked(Metadata *metadata, std::string const &tag, T &value)\n> > \treturn true;\n> > }\n> > \n> > -Matrix calculateCcm(std::vector const &ccms, double ct)\n> > +Matrix3x3 calculateCcm(std::vector const &ccms, double ct)\n> > {\n> > \tif (ct <= ccms.front().ct)\n> > \t\treturn ccms.front().ccm;\n> > @@ -141,13 +115,13 @@ Matrix calculateCcm(std::vector const &ccms, double ct)\n> > \t}\n> > }\n> > \n> > -Matrix applySaturation(Matrix const &ccm, double saturation)\n> > +Matrix3x3 applySaturation(Matrix3x3 const &ccm, double saturation)\n> > {\n> > -\tMatrix RGB2Y(0.299, 0.587, 0.114, -0.169, -0.331, 0.500, 0.500, -0.419,\n> > -\t\t -0.081);\n> > -\tMatrix Y2RGB(1.000, 0.000, 1.402, 1.000, -0.345, -0.714, 1.000, 1.771,\n> > -\t\t 0.000);\n> > -\tMatrix S(1, 0, 0, 0, saturation, 0, 0, 0, saturation);\n> > +\tMatrix3x3 RGB2Y({ 0.299, 0.587, 0.114, -0.169, -0.331, 0.500, 0.500,\n> > +\t\t\t -0.419, -0.081 });\n> > +\tMatrix3x3 Y2RGB({ 1.000, 0.000, 1.402, 1.000, -0.345, -0.714, 1.000,\n> > +\t\t\t 1.771, 0.000 });\n> \n> While at it, these two should be static const.\n> \n> > +\tMatrix3x3 S({ 1, 0, 0, 0, saturation, 0, 0, 0, saturation });\n> > \treturn Y2RGB * S * RGB2Y * ccm;\n> > }\n> > \n> > @@ -181,7 +155,7 @@ void Ccm::prepare(Metadata *imageMetadata)\n> > \tfor (int j = 0; j < 3; j++)\n> > \t\tfor (int i = 0; i < 3; i++)\n> > \t\t\tccmStatus.matrix[j * 3 + i] =\n> > -\t\t\t\tstd::max(-8.0, std::min(7.9999, ccm.m[j][i]));\n> > +\t\t\t\tstd::max(-8.0, std::min(7.9999, ccm[j][i]));\n> > \tLOG(RPiCcm, Debug)\n> > \t\t<< \"colour temperature \" << awb.temperatureK << \"K\";\n> > \tLOG(RPiCcm, Debug)\n> > diff --git a/src/ipa/rpi/controller/rpi/ccm.h b/src/ipa/rpi/controller/rpi/ccm.h\n> > index 4e5b33fefa4e..c05dbb17a264 100644\n> > --- a/src/ipa/rpi/controller/rpi/ccm.h\n> > +++ b/src/ipa/rpi/controller/rpi/ccm.h\n> > @@ -8,6 +8,7 @@\n> > \n> > #include \n> > \n> > +#include \"libcamera/internal/matrix.h\"\n> \n> Missing blank line.\n> \n> > #include \n> > \n> > #include \"../ccm_algorithm.h\"\n> > @@ -16,41 +17,9 @@ namespace RPiController {\n> > \n> > /* Algorithm to calculate colour matrix. Should be placed after AWB. */\n> > \n> > -struct Matrix {\n> > -\tMatrix(double m0, double m1, double m2, double m3, double m4, double m5,\n> > -\t double m6, double m7, double m8);\n> > -\tMatrix();\n> > -\tdouble m[3][3];\n> > -\tint read(const libcamera::YamlObject ¶ms);\n> > -};\n> > -static inline Matrix operator*(double d, Matrix const &m)\n> > -{\n> > -\treturn Matrix(m.m[0][0] * d, m.m[0][1] * d, m.m[0][2] * d,\n> > -\t\t m.m[1][0] * d, m.m[1][1] * d, m.m[1][2] * d,\n> > -\t\t m.m[2][0] * d, m.m[2][1] * d, m.m[2][2] * d);\n> > -}\n> > -static inline Matrix operator*(Matrix const &m1, Matrix const &m2)\n> > -{\n> > -\tMatrix m;\n> > -\tfor (int i = 0; i < 3; i++)\n> > -\t\tfor (int j = 0; j < 3; j++)\n> > -\t\t\tm.m[i][j] = m1.m[i][0] * m2.m[0][j] +\n> > -\t\t\t\t m1.m[i][1] * m2.m[1][j] +\n> > -\t\t\t\t m1.m[i][2] * m2.m[2][j];\n> > -\treturn m;\n> > -}\n> > -static inline Matrix operator+(Matrix const &m1, Matrix const &m2)\n> > -{\n> > -\tMatrix m;\n> > -\tfor (int i = 0; i < 3; i++)\n> > -\t\tfor (int j = 0; j < 3; j++)\n> > -\t\t\tm.m[i][j] = m1.m[i][j] + m2.m[i][j];\n> > -\treturn m;\n> > -}\n> > -\n> > struct CtCcm {\n> > \tdouble ct;\n> > -\tMatrix ccm;\n> > +\tlibcamera::Matrix ccm;\n> > };\n> > \n> > struct CcmConfig {\n> > diff --git a/src/libcamera/matrix.cpp b/src/libcamera/matrix.cpp\n> > index 8346f0d34160..c3ed54895b22 100644\n> > --- a/src/libcamera/matrix.cpp\n> > +++ b/src/libcamera/matrix.cpp\n> > @@ -5,7 +5,7 @@\n> > * Matrix and related operations\n> > */\n> > \n> > -#include \"matrix.h\"\n> > +#include \"libcamera/internal/matrix.h\"\n> > \n> > #include \n> > \n> > @@ -18,8 +18,6 @@ namespace libcamera {\n> > \n> > LOG_DEFINE_CATEGORY(Matrix)\n> > \n> > -namespace ipa {\n> > -\n> > /**\n> > * \\class Matrix\n> > * \\brief Matrix class\n> > @@ -34,7 +32,7 @@ namespace ipa {\n> > */\n> > \n> > /**\n> > - * \\fn Matrix::Matrix(const std::vector &data)\n> > + * \\fn Matrix::Matrix(const Span &data)\n> > * \\brief Construct a matrix from supplied data\n> > * \\param[in] data Data from which to construct a matrix\n> > *\n> > @@ -144,6 +142,4 @@ bool matrixValidateYaml(const YamlObject &obj, unsigned int size)\n> > }\n> > #endif /* __DOXYGEN__ */\n> > \n> > -} /* namespace ipa */\n> > -\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index 21cae11756d6..57fde8a8fab0 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -40,6 +40,7 @@ libcamera_internal_sources = files([\n> > 'ipc_pipe_unixsocket.cpp',\n> > 'ipc_unixsocket.cpp',\n> > 'mapped_framebuffer.cpp',\n> > + 'matrix.cpp',\n> > 'media_device.cpp',\n> > 'media_object.cpp',\n> > 'pipeline_handler.cpp',\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 8128CC32EA\n\tfor ;\n\tMon, 18 Nov 2024 16:52:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 86491658EC;\n\tMon, 18 Nov 2024 17:52:45 +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 BCE7C658DC\n\tfor ;\n\tMon, 18 Nov 2024 17:52:43 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:4424:4869:bb88:5c61])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D4636A57;\n\tMon, 18 Nov 2024 17:52:26 +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=\"hjzKKcau\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1731948746;\n\tbh=HT9ut1zwzdY4YZrzq01pkbIsIRt4/kKpuMIQ5yMRUdM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hjzKKcaunqz1r8/Qk6oV9EPjV5smxMt+H05JO8vSLCJz5Ag73NRVJ44sTWG70VixJ\n\tmRTsO7HdYgODUJn5CzKjuoPtBv9Fvjh1NKbFEcOQWBAtTxvNdp/2wBgczgcEzUleE+\n\tVZQx/+nU3iEenJcDiC+3KOHrfh8IhJMNXrE320r4=","Date":"Mon, 18 Nov 2024 17:52:41 +0100","From":"Stefan Klug ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 2/4] libcamera: Enable and use matrix implementation from\n\tlibcamera/internal","Message-ID":"","References":"<20241118150528.1856797-1-stefan.klug@ideasonboard.com>\n\t<20241118150528.1856797-3-stefan.klug@ideasonboard.com>\n\t<20241118155913.GS31681@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241118155913.GS31681@pendragon.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\" "}}]