[{"id":33658,"web_url":"https://patchwork.libcamera.org/comment/33658/","msgid":"<f96bdd71-55c8-4627-aadc-a23c296ea2cf@ideasonboard.com>","date":"2025-03-19T17:12:21","subject":"Re: [PATCH v2 06/17] test: Add minimal test for Matrix","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n\n2025. 03. 19. 17:11 keltezéssel, Stefan Klug írta:\n> For calculations in upcoming algorithm patches, the inverse of a matrix\n> is required. Add an implementation of the inverse() function for square\n> matrices.\n> \n> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>\n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> \n> ---\n> \n> Changes in v2:\n> - Replaced the implementation by a generic one provided by Laurent that\n>    supports arbitrary square matrices instead of 2x2 and 3x3 only.\n> - Moved the implementation into the cpp file.\n> ---\n>   include/libcamera/internal/matrix.h |  16 +++\n>   src/libcamera/matrix.cpp            | 160 ++++++++++++++++++++++++++++\n>   2 files changed, 176 insertions(+)\n> \n> diff --git a/include/libcamera/internal/matrix.h b/include/libcamera/internal/matrix.h\n> index 9b80521e3cb0..6e3c190286fe 100644\n> --- a/include/libcamera/internal/matrix.h\n> +++ b/include/libcamera/internal/matrix.h\n> @@ -19,6 +19,11 @@ namespace libcamera {\n>   \n>   LOG_DECLARE_CATEGORY(Matrix)\n>   \n> +#ifndef __DOXYGEN__\n> +template<typename T>\n> +bool matrixInvert(Span<const T> dataIn, Span<T> dataOut, unsigned int dim);\n> +#endif /* __DOXYGEN__ */\n> +\n>   template<typename T, unsigned int Rows, unsigned int Cols>\n>   class Matrix\n>   {\n> @@ -88,6 +93,17 @@ public:\n>   \t\treturn *this;\n>   \t}\n>   \n> +\tMatrix<T, Rows, Cols> inverse(bool *ok = nullptr) const\n\nReturning `std::optional<...>`? Or `std::pair<Matrix<...>, bool>` if the\nreturned matrix is in any way useful?\n\n\n> +\t{\n> +\t\tstatic_assert(Rows == Cols, \"Matrix must be square\");\n> +\n> +\t\tMatrix<T, Rows, Cols> inverse;\n> +\t\tbool res = matrixInvert(Span<const T>(data_), Span<T>(inverse.data_), Rows);\n> +\t\tif (ok)\n> +\t\t\t*ok = res;\n> +\t\treturn inverse;\n> +\t}\n> +\n>   private:\n>   \tstd::array<T, Rows * Cols> data_{};\n>   };\n> diff --git a/src/libcamera/matrix.cpp b/src/libcamera/matrix.cpp\n> index 6dca7498cab3..8590f8efeff3 100644\n> --- a/src/libcamera/matrix.cpp\n> +++ b/src/libcamera/matrix.cpp\n> @@ -7,6 +7,12 @@\n>   \n>   #include \"libcamera/internal/matrix.h\"\n>   \n> +#include <algorithm>\n> +#include <assert.h>\n> +#include <cmath>\n> +#include <numeric>\n> +#include <vector>\n> +\n>   #include <libcamera/base/log.h>\n>   \n>   /**\n> @@ -87,6 +93,20 @@ LOG_DEFINE_CATEGORY(Matrix)\n>    * \\return Row \\a i from the matrix, as a Span\n>    */\n>   \n> +/**\n> + * \\fn Matrix::inverse(bool *ok) const\n> + * \\param[out] ok Indicate if the matrix was successfully inverted\n> + * \\brief Compute the inverse of the matrix\n> + *\n> + * This function computes the inverse of the matrix. It is only implemented for\n> + * matrices of float and double types. If \\a ok is provided it will be set to a\n> + * boolean value to indicate of the inversion was successful. This can be used\n> + * to check if the matrix is singular, in which case the function will return\n> + * an identity matrix.\n> + *\n> + * \\return The inverse of the matrix\n> + */\n> +\n>   /**\n>    * \\fn Matrix::operator[](size_t i)\n>    * \\copydoc Matrix::operator[](size_t i) const\n> @@ -142,6 +162,146 @@ LOG_DEFINE_CATEGORY(Matrix)\n>    */\n>   \n>   #ifndef __DOXYGEN__\n> +template<typename T>\n> +bool matrixInvert(Span<const T> dataIn, Span<T> dataOut, unsigned int dim)\n\nSorry, but I think this is a bit of an overkill:\n\n   (1) it is (most likely) slower than hard-coding the inversion of a 3x3 matrix\n       (where it would be used);\n   (2) it adds dynamic memory allocations.\n\nOr am I missing something? I suppose (2) could be addressed by providing a\n\"scratch buffer\" as well, but I think (1) still stands.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> +{\n> +\t/*\n> +\t * Convenience class to access matrix data, providing a row-major (i,j)\n> +\t * element accessor through the call operator, and the ability to swap\n> +\t * rows without modifying the backing storage.\n> +\t */\n> +\tclass MatrixAccessor\n> +\t{\n> +\tpublic:\n> +\t\tMatrixAccessor(Span<T> data, unsigned int rows, unsigned int cols)\n> +\t\t\t: data_(data), swap_(rows), rows_(rows), cols_(cols)\n> +\t\t{\n> +\t\t\tstd::iota(swap_.begin(), swap_.end(), T{ 0 });\n> +\t\t}\n> +\n> +\t\tT &operator()(unsigned int row, unsigned int col)\n> +\t\t{\n> +\t\t\tassert(row < rows_ && col < cols_);\n> +\t\t\treturn data_[index(row, col)];\n> +\t\t}\n> +\n> +\t\tvoid swap(unsigned int a, unsigned int b)\n> +\t\t{\n> +\t\t\tassert(a < rows_ && a < cols_);\n> +\t\t\tstd::swap(swap_[a], swap_[b]);\n> +\t\t}\n> +\n> +\tprivate:\n> +\t\tunsigned int index(unsigned int row, unsigned int col) const\n> +\t\t{\n> +\t\t\treturn swap_[row] * cols_ + col;\n> +\t\t}\n> +\n> +\t\tSpan<T> data_;\n> +\t\tstd::vector<unsigned int> swap_;\n> +\t\tunsigned int rows_;\n> +\t\tunsigned int cols_;\n> +\t};\n> +\n> +\t/*\n> +\t * Matrix inversion using Gaussian elimination.\n> +\t *\n> +\t * Start by augmenting the original matrix with an identiy matrix of\n> +\t * the same size.\n> +\t */\n> +\tstd::vector<T> data(dim * dim * 2);\n> +\tMatrixAccessor matrix(data, dim, dim * 2);\n> +\n> +\tfor (unsigned int i = 0; i < dim; ++i) {\n> +\t\tfor (unsigned int j = 0; j < dim; ++j)\n> +\t\t\tmatrix(i, j) = dataIn[i * dim + j];\n> +\t\tmatrix(i, i + dim) = T{ 1 };\n> +\t}\n> +\n> +\t/* Start by triangularizing the input . */\n> +\tfor (unsigned int pivot = 0; pivot < dim; ++pivot) {\n> +\t\t/*\n> +\t\t * Locate the next pivot. To improve numerical stability, use\n> +\t\t * the row with the largest value in the pivot's column.\n> +\t\t */\n> +\t\tunsigned int row;\n> +\t\tT maxValue{ 0 };\n> +\n> +\t\tfor (unsigned int i = pivot; i < dim; ++i) {\n> +\t\t\tT value = std::abs(matrix(i, pivot));\n> +\t\t\tif (maxValue < value) {\n> +\t\t\t\tmaxValue = value;\n> +\t\t\t\trow = i;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\t/*\n> +\t\t * If no pivot is found in the column, the matrix is not\n> +\t\t * invertible. Return an identity matrix.\n> +\t\t */\n> +\t\tif (maxValue == 0) {\n> +\t\t\tstd::fill(dataOut.begin(), dataOut.end(), T{ 0 });\n> +\t\t\tfor (unsigned int i = 0; i < dim; ++i)\n> +\t\t\t\tdataOut[i * dim + i] = T{ 1 };\n> +\t\t\treturn false;\n> +\t\t}\n> +\n> +\t\t/* Swap rows to bring the pivot in the right location. */\n> +\t\tmatrix.swap(pivot, row);\n> +\n> +\t\t/* Process all rows below the pivot to zero the pivot column. */\n> +\t\tconst T pivotValue = matrix(pivot, pivot);\n> +\n> +\t\tfor (unsigned int i = pivot + 1; i < dim; ++i) {\n> +\t\t\tconst T factor = matrix(i, pivot) / pivotValue;\n> +\n> +\t\t\t/*\n> +\t\t\t * We know the element in the pivot column will be 0,\n> +\t\t\t * hardcode it instead of computing it.\n> +\t\t\t */\n> +\t\t\tmatrix(i, pivot) = T{ 0 };\n> +\n> +\t\t\tfor (unsigned int j = pivot + 1; j < dim * 2; ++j)\n> +\t\t\t\tmatrix(i, j) -= matrix(pivot, j) * factor;\n> +\t\t}\n> +\t}\n> +\n> +\t/*\n> +\t * Then diagonalize the input, walking the diagonal backwards. There's\n> +\t * no need to update the input matrix, as all the values we would write\n> +\t * in the top-right triangle aren't used in further calculations (and\n> +\t * would all by definition be zero).\n> +\t */\n> +\tfor (unsigned int pivot = dim - 1; pivot > 0; --pivot) {\n> +\t\tconst T pivotValue = matrix(pivot, pivot);\n> +\n> +\t\tfor (unsigned int i = 0; i < pivot; ++i) {\n> +\t\t\tconst T factor = matrix(i, pivot) / pivotValue;\n> +\n> +\t\t\tfor (unsigned int j = dim; j < dim * 2; ++j)\n> +\t\t\t\tmatrix(i, j) -= matrix(pivot, j) * factor;\n> +\t\t}\n> +\t}\n> +\n> +\t/*\n> +\t * Finally, normalize the diagonal and store the result in the output\n> +\t * data.\n> +\t */\n> +\tfor (unsigned int i = 0; i < dim; ++i) {\n> +\t\tconst T factor = matrix(i, i);\n> +\n> +\t\tfor (unsigned int j = 0; j < dim; ++j)\n> +\t\t\tdataOut[i * dim + j] = matrix(i, j + dim) / factor;\n> +\t}\n> +\n> +\treturn true;\n> +}\n> +\n> +template bool matrixInvert<float>(Span<const float> dataIn, Span<float> dataOut,\n> +\t\t\t\t  unsigned int dim);\n> +template bool matrixInvert<double>(Span<const double> data, Span<double> dataOut,\n> +\t\t\t\t   unsigned int dim);\n> +\n>   /*\n>    * The YAML data shall be a list of numerical values. Its size shall be equal\n>    * to the product of the number of rows and columns of the matrix (Rows x","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 10C78C32FE\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 19 Mar 2025 17:12:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5252C68951;\n\tWed, 19 Mar 2025 18:12:27 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6ACAB687FA\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 19 Mar 2025 18:12:25 +0100 (CET)","from [192.168.33.18] (185.221.143.221.nat.pool.zt.hu\n\t[185.221.143.221])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 379811E6;\n\tWed, 19 Mar 2025 18:10:42 +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=\"gaQJU0qd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1742404242;\n\tbh=9FolJE262JkgtLzlh2GWWZ+JqosxX1+P0lmI+ip5bG8=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=gaQJU0qdEusKSDTl3KtaubEZcQBJ0Iz9own7PRmniozvkxQvsdph7Sk1sZ6YORapW\n\tNv305acaABuxAp8jPJL0JE1Sq9oILLWJCPhxtkEKEY5LSrJFk+HEbVUt9rRcZCDlhL\n\tYH+oGBuDTLKSiwrpkaOlNonFM8v4F0FC/cxj5OYI=","Message-ID":"<f96bdd71-55c8-4627-aadc-a23c296ea2cf@ideasonboard.com>","Date":"Wed, 19 Mar 2025 18:12:21 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2 06/17] test: Add minimal test for Matrix","To":"Stefan Klug <stefan.klug@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org,\n\tLaurent Pinchart <laurent.pinchart@ideasonboard.com>","References":"<20250319161152.63625-1-stefan.klug@ideasonboard.com>\n\t<20250319161152.63625-7-stefan.klug@ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <barnabas.pocze@ideasonboard.com>","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250319161152.63625-7-stefan.klug@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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":33766,"web_url":"https://patchwork.libcamera.org/comment/33766/","msgid":"<20250331131511.GA27882@pendragon.ideasonboard.com>","date":"2025-03-31T13:15:11","subject":"Re: [PATCH v2 06/17] test: Add minimal test for Matrix","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"I'm not sure what happens, this is a review of 05/17 but it cames as a\nreply to 06/17.\n\nOn Wed, Mar 19, 2025 at 06:12:21PM +0100, Barnabás Pőcze wrote:\n> 2025. 03. 19. 17:11 keltezéssel, Stefan Klug írta:\n> > For calculations in upcoming algorithm patches, the inverse of a matrix\n> > is required. Add an implementation of the inverse() function for square\n> > matrices.\n> > \n> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>\n> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > \n> > ---\n> > \n> > Changes in v2:\n> > - Replaced the implementation by a generic one provided by Laurent that\n> >    supports arbitrary square matrices instead of 2x2 and 3x3 only.\n> > - Moved the implementation into the cpp file.\n> > ---\n> >   include/libcamera/internal/matrix.h |  16 +++\n> >   src/libcamera/matrix.cpp            | 160 ++++++++++++++++++++++++++++\n> >   2 files changed, 176 insertions(+)\n> > \n> > diff --git a/include/libcamera/internal/matrix.h b/include/libcamera/internal/matrix.h\n> > index 9b80521e3cb0..6e3c190286fe 100644\n> > --- a/include/libcamera/internal/matrix.h\n> > +++ b/include/libcamera/internal/matrix.h\n> > @@ -19,6 +19,11 @@ namespace libcamera {\n> >   \n> >   LOG_DECLARE_CATEGORY(Matrix)\n> >   \n> > +#ifndef __DOXYGEN__\n> > +template<typename T>\n> > +bool matrixInvert(Span<const T> dataIn, Span<T> dataOut, unsigned int dim);\n> > +#endif /* __DOXYGEN__ */\n> > +\n> >   template<typename T, unsigned int Rows, unsigned int Cols>\n> >   class Matrix\n> >   {\n> > @@ -88,6 +93,17 @@ public:\n> >   \t\treturn *this;\n> >   \t}\n> >   \n> > +\tMatrix<T, Rows, Cols> inverse(bool *ok = nullptr) const\n> \n> Returning `std::optional<...>`? Or `std::pair<Matrix<...>, bool>` if the\n> returned matrix is in any way useful?\n> \n> \n> > +\t{\n> > +\t\tstatic_assert(Rows == Cols, \"Matrix must be square\");\n> > +\n> > +\t\tMatrix<T, Rows, Cols> inverse;\n> > +\t\tbool res = matrixInvert(Span<const T>(data_), Span<T>(inverse.data_), Rows);\n> > +\t\tif (ok)\n> > +\t\t\t*ok = res;\n> > +\t\treturn inverse;\n> > +\t}\n> > +\n> >   private:\n> >   \tstd::array<T, Rows * Cols> data_{};\n> >   };\n> > diff --git a/src/libcamera/matrix.cpp b/src/libcamera/matrix.cpp\n> > index 6dca7498cab3..8590f8efeff3 100644\n> > --- a/src/libcamera/matrix.cpp\n> > +++ b/src/libcamera/matrix.cpp\n> > @@ -7,6 +7,12 @@\n> >   \n> >   #include \"libcamera/internal/matrix.h\"\n> >   \n> > +#include <algorithm>\n> > +#include <assert.h>\n> > +#include <cmath>\n> > +#include <numeric>\n> > +#include <vector>\n> > +\n> >   #include <libcamera/base/log.h>\n> >   \n> >   /**\n> > @@ -87,6 +93,20 @@ LOG_DEFINE_CATEGORY(Matrix)\n> >    * \\return Row \\a i from the matrix, as a Span\n> >    */\n> >   \n> > +/**\n> > + * \\fn Matrix::inverse(bool *ok) const\n> > + * \\param[out] ok Indicate if the matrix was successfully inverted\n> > + * \\brief Compute the inverse of the matrix\n> > + *\n> > + * This function computes the inverse of the matrix. It is only implemented for\n> > + * matrices of float and double types. If \\a ok is provided it will be set to a\n> > + * boolean value to indicate of the inversion was successful. This can be used\n> > + * to check if the matrix is singular, in which case the function will return\n> > + * an identity matrix.\n> > + *\n> > + * \\return The inverse of the matrix\n> > + */\n> > +\n> >   /**\n> >    * \\fn Matrix::operator[](size_t i)\n> >    * \\copydoc Matrix::operator[](size_t i) const\n> > @@ -142,6 +162,146 @@ LOG_DEFINE_CATEGORY(Matrix)\n> >    */\n> >   \n> >   #ifndef __DOXYGEN__\n> > +template<typename T>\n> > +bool matrixInvert(Span<const T> dataIn, Span<T> dataOut, unsigned int dim)\n> \n> Sorry, but I think this is a bit of an overkill:\n> \n>    (1) it is (most likely) slower than hard-coding the inversion of a 3x3 matrix\n>        (where it would be used);\n\nIs this a real concern, given our usage patterns ? If so we could use\ntemplate specialization for 3x3 matrices.\n\n>    (2) it adds dynamic memory allocations.\n> \n> Or am I missing something? I suppose (2) could be addressed by providing a\n> \"scratch buffer\" as well, but I think (1) still stands.\n> \n> > +{\n> > +\t/*\n> > +\t * Convenience class to access matrix data, providing a row-major (i,j)\n> > +\t * element accessor through the call operator, and the ability to swap\n> > +\t * rows without modifying the backing storage.\n> > +\t */\n> > +\tclass MatrixAccessor\n> > +\t{\n> > +\tpublic:\n> > +\t\tMatrixAccessor(Span<T> data, unsigned int rows, unsigned int cols)\n> > +\t\t\t: data_(data), swap_(rows), rows_(rows), cols_(cols)\n> > +\t\t{\n> > +\t\t\tstd::iota(swap_.begin(), swap_.end(), T{ 0 });\n> > +\t\t}\n> > +\n> > +\t\tT &operator()(unsigned int row, unsigned int col)\n> > +\t\t{\n> > +\t\t\tassert(row < rows_ && col < cols_);\n> > +\t\t\treturn data_[index(row, col)];\n> > +\t\t}\n> > +\n> > +\t\tvoid swap(unsigned int a, unsigned int b)\n> > +\t\t{\n> > +\t\t\tassert(a < rows_ && a < cols_);\n> > +\t\t\tstd::swap(swap_[a], swap_[b]);\n> > +\t\t}\n> > +\n> > +\tprivate:\n> > +\t\tunsigned int index(unsigned int row, unsigned int col) const\n> > +\t\t{\n> > +\t\t\treturn swap_[row] * cols_ + col;\n> > +\t\t}\n> > +\n> > +\t\tSpan<T> data_;\n> > +\t\tstd::vector<unsigned int> swap_;\n> > +\t\tunsigned int rows_;\n> > +\t\tunsigned int cols_;\n> > +\t};\n> > +\n> > +\t/*\n> > +\t * Matrix inversion using Gaussian elimination.\n> > +\t *\n> > +\t * Start by augmenting the original matrix with an identiy matrix of\n> > +\t * the same size.\n> > +\t */\n> > +\tstd::vector<T> data(dim * dim * 2);\n> > +\tMatrixAccessor matrix(data, dim, dim * 2);\n> > +\n> > +\tfor (unsigned int i = 0; i < dim; ++i) {\n> > +\t\tfor (unsigned int j = 0; j < dim; ++j)\n> > +\t\t\tmatrix(i, j) = dataIn[i * dim + j];\n> > +\t\tmatrix(i, i + dim) = T{ 1 };\n> > +\t}\n> > +\n> > +\t/* Start by triangularizing the input . */\n> > +\tfor (unsigned int pivot = 0; pivot < dim; ++pivot) {\n> > +\t\t/*\n> > +\t\t * Locate the next pivot. To improve numerical stability, use\n> > +\t\t * the row with the largest value in the pivot's column.\n> > +\t\t */\n> > +\t\tunsigned int row;\n> > +\t\tT maxValue{ 0 };\n> > +\n> > +\t\tfor (unsigned int i = pivot; i < dim; ++i) {\n> > +\t\t\tT value = std::abs(matrix(i, pivot));\n> > +\t\t\tif (maxValue < value) {\n> > +\t\t\t\tmaxValue = value;\n> > +\t\t\t\trow = i;\n> > +\t\t\t}\n> > +\t\t}\n> > +\n> > +\t\t/*\n> > +\t\t * If no pivot is found in the column, the matrix is not\n> > +\t\t * invertible. Return an identity matrix.\n> > +\t\t */\n> > +\t\tif (maxValue == 0) {\n> > +\t\t\tstd::fill(dataOut.begin(), dataOut.end(), T{ 0 });\n> > +\t\t\tfor (unsigned int i = 0; i < dim; ++i)\n> > +\t\t\t\tdataOut[i * dim + i] = T{ 1 };\n> > +\t\t\treturn false;\n> > +\t\t}\n> > +\n> > +\t\t/* Swap rows to bring the pivot in the right location. */\n> > +\t\tmatrix.swap(pivot, row);\n> > +\n> > +\t\t/* Process all rows below the pivot to zero the pivot column. */\n> > +\t\tconst T pivotValue = matrix(pivot, pivot);\n> > +\n> > +\t\tfor (unsigned int i = pivot + 1; i < dim; ++i) {\n> > +\t\t\tconst T factor = matrix(i, pivot) / pivotValue;\n> > +\n> > +\t\t\t/*\n> > +\t\t\t * We know the element in the pivot column will be 0,\n> > +\t\t\t * hardcode it instead of computing it.\n> > +\t\t\t */\n> > +\t\t\tmatrix(i, pivot) = T{ 0 };\n> > +\n> > +\t\t\tfor (unsigned int j = pivot + 1; j < dim * 2; ++j)\n> > +\t\t\t\tmatrix(i, j) -= matrix(pivot, j) * factor;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * Then diagonalize the input, walking the diagonal backwards. There's\n> > +\t * no need to update the input matrix, as all the values we would write\n> > +\t * in the top-right triangle aren't used in further calculations (and\n> > +\t * would all by definition be zero).\n> > +\t */\n> > +\tfor (unsigned int pivot = dim - 1; pivot > 0; --pivot) {\n> > +\t\tconst T pivotValue = matrix(pivot, pivot);\n> > +\n> > +\t\tfor (unsigned int i = 0; i < pivot; ++i) {\n> > +\t\t\tconst T factor = matrix(i, pivot) / pivotValue;\n> > +\n> > +\t\t\tfor (unsigned int j = dim; j < dim * 2; ++j)\n> > +\t\t\t\tmatrix(i, j) -= matrix(pivot, j) * factor;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * Finally, normalize the diagonal and store the result in the output\n> > +\t * data.\n> > +\t */\n> > +\tfor (unsigned int i = 0; i < dim; ++i) {\n> > +\t\tconst T factor = matrix(i, i);\n> > +\n> > +\t\tfor (unsigned int j = 0; j < dim; ++j)\n> > +\t\t\tdataOut[i * dim + j] = matrix(i, j + dim) / factor;\n> > +\t}\n> > +\n> > +\treturn true;\n> > +}\n> > +\n> > +template bool matrixInvert<float>(Span<const float> dataIn, Span<float> dataOut,\n> > +\t\t\t\t  unsigned int dim);\n> > +template bool matrixInvert<double>(Span<const double> data, Span<double> dataOut,\n> > +\t\t\t\t   unsigned int dim);\n> > +\n> >   /*\n> >    * The YAML data shall be a list of numerical values. Its size shall be equal\n> >    * to the product of the number of rows and columns of the matrix (Rows x","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 87864C323E\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 31 Mar 2025 13:15:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4D8B968981;\n\tMon, 31 Mar 2025 15:15:37 +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 1128E68967\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 31 Mar 2025 15:15:36 +0200 (CEST)","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 8F836703;\n\tMon, 31 Mar 2025 15:13:43 +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=\"sS6ypTQI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1743426823;\n\tbh=Ag6mIcGQwLGSE6zK1UWHi6tedMcjkeOSwrQPjEnFGkU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sS6ypTQIN6DR6MkoqYluhAPdDick5paR/ZKadUKRpgRWIw1iqdkBiViHJzcgwC8hI\n\tF+TTkeMB4PU1D7/kF3+IOjDceHnxVkbeepDHrPEykigQ2D92jrhcivVDS56mGvFSD/\n\ttPcBkR6x2FQw/jtK+nn2pHjV/0GYhFokB2RGcAcQ=","Date":"Mon, 31 Mar 2025 16:15:11 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <barnabas.pocze@ideasonboard.com>","Cc":"Stefan Klug <stefan.klug@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 06/17] test: Add minimal test for Matrix","Message-ID":"<20250331131511.GA27882@pendragon.ideasonboard.com>","References":"<20250319161152.63625-1-stefan.klug@ideasonboard.com>\n\t<20250319161152.63625-7-stefan.klug@ideasonboard.com>\n\t<f96bdd71-55c8-4627-aadc-a23c296ea2cf@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<f96bdd71-55c8-4627-aadc-a23c296ea2cf@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":33767,"web_url":"https://patchwork.libcamera.org/comment/33767/","msgid":"<5c53336e-12aa-41de-9e23-d98182b7c7af@ideasonboard.com>","date":"2025-03-31T13:52:07","subject":"Re: [PATCH v2 06/17] test: Add minimal test for Matrix","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n\n2025. 03. 31. 15:15 keltezéssel, Laurent Pinchart írta:\n> I'm not sure what happens, this is a review of 05/17 but it cames as a\n> reply to 06/17.\n\nOops, you're right. I'm not sure what happened.\n\n\n> \n> On Wed, Mar 19, 2025 at 06:12:21PM +0100, Barnabás Pőcze wrote:\n>> 2025. 03. 19. 17:11 keltezéssel, Stefan Klug írta:\n>>> For calculations in upcoming algorithm patches, the inverse of a matrix\n>>> is required. Add an implementation of the inverse() function for square\n>>> matrices.\n>>>\n>>> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>\n>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n>>>\n>>> ---\n>>>\n>>> Changes in v2:\n>>> - Replaced the implementation by a generic one provided by Laurent that\n>>>     supports arbitrary square matrices instead of 2x2 and 3x3 only.\n>>> - Moved the implementation into the cpp file.\n>>> ---\n>>>    include/libcamera/internal/matrix.h |  16 +++\n>>>    src/libcamera/matrix.cpp            | 160 ++++++++++++++++++++++++++++\n>>>    2 files changed, 176 insertions(+)\n>>>\n>>> diff --git a/include/libcamera/internal/matrix.h b/include/libcamera/internal/matrix.h\n>>> index 9b80521e3cb0..6e3c190286fe 100644\n>>> --- a/include/libcamera/internal/matrix.h\n>>> +++ b/include/libcamera/internal/matrix.h\n>>> @@ -19,6 +19,11 @@ namespace libcamera {\n>>>    \n>>>    LOG_DECLARE_CATEGORY(Matrix)\n>>>    \n>>> +#ifndef __DOXYGEN__\n>>> +template<typename T>\n>>> +bool matrixInvert(Span<const T> dataIn, Span<T> dataOut, unsigned int dim);\n>>> +#endif /* __DOXYGEN__ */\n>>> +\n>>>    template<typename T, unsigned int Rows, unsigned int Cols>\n>>>    class Matrix\n>>>    {\n>>> @@ -88,6 +93,17 @@ public:\n>>>    \t\treturn *this;\n>>>    \t}\n>>>    \n>>> +\tMatrix<T, Rows, Cols> inverse(bool *ok = nullptr) const\n>>\n>> Returning `std::optional<...>`? Or `std::pair<Matrix<...>, bool>` if the\n>> returned matrix is in any way useful?\n>>\n>>\n>>> +\t{\n>>> +\t\tstatic_assert(Rows == Cols, \"Matrix must be square\");\n>>> +\n>>> +\t\tMatrix<T, Rows, Cols> inverse;\n>>> +\t\tbool res = matrixInvert(Span<const T>(data_), Span<T>(inverse.data_), Rows);\n>>> +\t\tif (ok)\n>>> +\t\t\t*ok = res;\n>>> +\t\treturn inverse;\n>>> +\t}\n>>> +\n>>>    private:\n>>>    \tstd::array<T, Rows * Cols> data_{};\n>>>    };\n>>> diff --git a/src/libcamera/matrix.cpp b/src/libcamera/matrix.cpp\n>>> index 6dca7498cab3..8590f8efeff3 100644\n>>> --- a/src/libcamera/matrix.cpp\n>>> +++ b/src/libcamera/matrix.cpp\n>>> @@ -7,6 +7,12 @@\n>>>    \n>>>    #include \"libcamera/internal/matrix.h\"\n>>>    \n>>> +#include <algorithm>\n>>> +#include <assert.h>\n>>> +#include <cmath>\n>>> +#include <numeric>\n>>> +#include <vector>\n>>> +\n>>>    #include <libcamera/base/log.h>\n>>>    \n>>>    /**\n>>> @@ -87,6 +93,20 @@ LOG_DEFINE_CATEGORY(Matrix)\n>>>     * \\return Row \\a i from the matrix, as a Span\n>>>     */\n>>>    \n>>> +/**\n>>> + * \\fn Matrix::inverse(bool *ok) const\n>>> + * \\param[out] ok Indicate if the matrix was successfully inverted\n>>> + * \\brief Compute the inverse of the matrix\n>>> + *\n>>> + * This function computes the inverse of the matrix. It is only implemented for\n>>> + * matrices of float and double types. If \\a ok is provided it will be set to a\n>>> + * boolean value to indicate of the inversion was successful. This can be used\n>>> + * to check if the matrix is singular, in which case the function will return\n>>> + * an identity matrix.\n>>> + *\n>>> + * \\return The inverse of the matrix\n>>> + */\n>>> +\n>>>    /**\n>>>     * \\fn Matrix::operator[](size_t i)\n>>>     * \\copydoc Matrix::operator[](size_t i) const\n>>> @@ -142,6 +162,146 @@ LOG_DEFINE_CATEGORY(Matrix)\n>>>     */\n>>>    \n>>>    #ifndef __DOXYGEN__\n>>> +template<typename T>\n>>> +bool matrixInvert(Span<const T> dataIn, Span<T> dataOut, unsigned int dim)\n>>\n>> Sorry, but I think this is a bit of an overkill:\n>>\n>>     (1) it is (most likely) slower than hard-coding the inversion of a 3x3 matrix\n>>         (where it would be used);\n> \n> Is this a real concern, given our usage patterns ? If so we could use\n> template specialization for 3x3 matrices.\n\nThe only user I can see is `libcamera::ipa::rkisp1::algorithms::Awb::calculateRgbMeans()`,\nand as far as I can tell that runs on every frame. So I don't think this is ideal, and while\nof course this in and of itself is not a big issue, I think many small things add up over time.\n\n\nRegards,\nBarnabás Pőcze\n\n> \n>>     (2) it adds dynamic memory allocations.\n>>\n>> Or am I missing something? I suppose (2) could be addressed by providing a\n>> \"scratch buffer\" as well, but I think (1) still stands.\n>>\n>>> +{\n>>> +\t/*\n>>> +\t * Convenience class to access matrix data, providing a row-major (i,j)\n>>> +\t * element accessor through the call operator, and the ability to swap\n>>> +\t * rows without modifying the backing storage.\n>>> +\t */\n>>> +\tclass MatrixAccessor\n>>> +\t{\n>>> +\tpublic:\n>>> +\t\tMatrixAccessor(Span<T> data, unsigned int rows, unsigned int cols)\n>>> +\t\t\t: data_(data), swap_(rows), rows_(rows), cols_(cols)\n>>> +\t\t{\n>>> +\t\t\tstd::iota(swap_.begin(), swap_.end(), T{ 0 });\n>>> +\t\t}\n>>> +\n>>> +\t\tT &operator()(unsigned int row, unsigned int col)\n>>> +\t\t{\n>>> +\t\t\tassert(row < rows_ && col < cols_);\n>>> +\t\t\treturn data_[index(row, col)];\n>>> +\t\t}\n>>> +\n>>> +\t\tvoid swap(unsigned int a, unsigned int b)\n>>> +\t\t{\n>>> +\t\t\tassert(a < rows_ && a < cols_);\n>>> +\t\t\tstd::swap(swap_[a], swap_[b]);\n>>> +\t\t}\n>>> +\n>>> +\tprivate:\n>>> +\t\tunsigned int index(unsigned int row, unsigned int col) const\n>>> +\t\t{\n>>> +\t\t\treturn swap_[row] * cols_ + col;\n>>> +\t\t}\n>>> +\n>>> +\t\tSpan<T> data_;\n>>> +\t\tstd::vector<unsigned int> swap_;\n>>> +\t\tunsigned int rows_;\n>>> +\t\tunsigned int cols_;\n>>> +\t};\n>>> +\n>>> +\t/*\n>>> +\t * Matrix inversion using Gaussian elimination.\n>>> +\t *\n>>> +\t * Start by augmenting the original matrix with an identiy matrix of\n>>> +\t * the same size.\n>>> +\t */\n>>> +\tstd::vector<T> data(dim * dim * 2);\n>>> +\tMatrixAccessor matrix(data, dim, dim * 2);\n>>> +\n>>> +\tfor (unsigned int i = 0; i < dim; ++i) {\n>>> +\t\tfor (unsigned int j = 0; j < dim; ++j)\n>>> +\t\t\tmatrix(i, j) = dataIn[i * dim + j];\n>>> +\t\tmatrix(i, i + dim) = T{ 1 };\n>>> +\t}\n>>> +\n>>> +\t/* Start by triangularizing the input . */\n>>> +\tfor (unsigned int pivot = 0; pivot < dim; ++pivot) {\n>>> +\t\t/*\n>>> +\t\t * Locate the next pivot. To improve numerical stability, use\n>>> +\t\t * the row with the largest value in the pivot's column.\n>>> +\t\t */\n>>> +\t\tunsigned int row;\n>>> +\t\tT maxValue{ 0 };\n>>> +\n>>> +\t\tfor (unsigned int i = pivot; i < dim; ++i) {\n>>> +\t\t\tT value = std::abs(matrix(i, pivot));\n>>> +\t\t\tif (maxValue < value) {\n>>> +\t\t\t\tmaxValue = value;\n>>> +\t\t\t\trow = i;\n>>> +\t\t\t}\n>>> +\t\t}\n>>> +\n>>> +\t\t/*\n>>> +\t\t * If no pivot is found in the column, the matrix is not\n>>> +\t\t * invertible. Return an identity matrix.\n>>> +\t\t */\n>>> +\t\tif (maxValue == 0) {\n>>> +\t\t\tstd::fill(dataOut.begin(), dataOut.end(), T{ 0 });\n>>> +\t\t\tfor (unsigned int i = 0; i < dim; ++i)\n>>> +\t\t\t\tdataOut[i * dim + i] = T{ 1 };\n>>> +\t\t\treturn false;\n>>> +\t\t}\n>>> +\n>>> +\t\t/* Swap rows to bring the pivot in the right location. */\n>>> +\t\tmatrix.swap(pivot, row);\n>>> +\n>>> +\t\t/* Process all rows below the pivot to zero the pivot column. */\n>>> +\t\tconst T pivotValue = matrix(pivot, pivot);\n>>> +\n>>> +\t\tfor (unsigned int i = pivot + 1; i < dim; ++i) {\n>>> +\t\t\tconst T factor = matrix(i, pivot) / pivotValue;\n>>> +\n>>> +\t\t\t/*\n>>> +\t\t\t * We know the element in the pivot column will be 0,\n>>> +\t\t\t * hardcode it instead of computing it.\n>>> +\t\t\t */\n>>> +\t\t\tmatrix(i, pivot) = T{ 0 };\n>>> +\n>>> +\t\t\tfor (unsigned int j = pivot + 1; j < dim * 2; ++j)\n>>> +\t\t\t\tmatrix(i, j) -= matrix(pivot, j) * factor;\n>>> +\t\t}\n>>> +\t}\n>>> +\n>>> +\t/*\n>>> +\t * Then diagonalize the input, walking the diagonal backwards. There's\n>>> +\t * no need to update the input matrix, as all the values we would write\n>>> +\t * in the top-right triangle aren't used in further calculations (and\n>>> +\t * would all by definition be zero).\n>>> +\t */\n>>> +\tfor (unsigned int pivot = dim - 1; pivot > 0; --pivot) {\n>>> +\t\tconst T pivotValue = matrix(pivot, pivot);\n>>> +\n>>> +\t\tfor (unsigned int i = 0; i < pivot; ++i) {\n>>> +\t\t\tconst T factor = matrix(i, pivot) / pivotValue;\n>>> +\n>>> +\t\t\tfor (unsigned int j = dim; j < dim * 2; ++j)\n>>> +\t\t\t\tmatrix(i, j) -= matrix(pivot, j) * factor;\n>>> +\t\t}\n>>> +\t}\n>>> +\n>>> +\t/*\n>>> +\t * Finally, normalize the diagonal and store the result in the output\n>>> +\t * data.\n>>> +\t */\n>>> +\tfor (unsigned int i = 0; i < dim; ++i) {\n>>> +\t\tconst T factor = matrix(i, i);\n>>> +\n>>> +\t\tfor (unsigned int j = 0; j < dim; ++j)\n>>> +\t\t\tdataOut[i * dim + j] = matrix(i, j + dim) / factor;\n>>> +\t}\n>>> +\n>>> +\treturn true;\n>>> +}\n>>> +\n>>> +template bool matrixInvert<float>(Span<const float> dataIn, Span<float> dataOut,\n>>> +\t\t\t\t  unsigned int dim);\n>>> +template bool matrixInvert<double>(Span<const double> data, Span<double> dataOut,\n>>> +\t\t\t\t   unsigned int dim);\n>>> +\n>>>    /*\n>>>     * The YAML data shall be a list of numerical values. Its size shall be equal\n>>>     * to the product of the number of rows and columns of the matrix (Rows x\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 9CD50C3213\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 31 Mar 2025 13:52:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B989368981;\n\tMon, 31 Mar 2025 15:52:16 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 71D0968967\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 31 Mar 2025 15:52:15 +0200 (CEST)","from [192.168.33.23] (185.221.143.221.nat.pool.zt.hu\n\t[185.221.143.221])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BE707703;\n\tMon, 31 Mar 2025 15:50:23 +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=\"bUPQKg3Y\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1743429024;\n\tbh=wnpTShedumXSf6ItnJEzTvqAeZmjYH0gRNklt47khlk=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=bUPQKg3Y7dGswLtfN6zz4nL/2DuStT0hAKqMed4mmlMGVB7kDRrmYFCnwaf4oUoCN\n\tzLolmFujq92TSJ8tx8sihM39ltmxhfsrIUszoIP3GwPA+d6XcfrQQsRj6tZLMD+ny5\n\tiY9JFoheK71hlvs+zO6k2yCT4IJddDppXK2mLEPM=","Message-ID":"<5c53336e-12aa-41de-9e23-d98182b7c7af@ideasonboard.com>","Date":"Mon, 31 Mar 2025 15:52:07 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2 06/17] test: Add minimal test for Matrix","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"Stefan Klug <stefan.klug@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250319161152.63625-1-stefan.klug@ideasonboard.com>\n\t<20250319161152.63625-7-stefan.klug@ideasonboard.com>\n\t<f96bdd71-55c8-4627-aadc-a23c296ea2cf@ideasonboard.com>\n\t<20250331131511.GA27882@pendragon.ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <barnabas.pocze@ideasonboard.com>","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250331131511.GA27882@pendragon.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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":33768,"web_url":"https://patchwork.libcamera.org/comment/33768/","msgid":"<20250331151112.GA11469@pendragon.ideasonboard.com>","date":"2025-03-31T15:11:12","subject":"Re: [PATCH v2 06/17] test: Add minimal test for Matrix","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Mar 31, 2025 at 03:52:07PM +0200, Barnabás Pőcze wrote:\n> 2025. 03. 31. 15:15 keltezéssel, Laurent Pinchart írta:\n> > I'm not sure what happens, this is a review of 05/17 but it cames as a\n> > reply to 06/17.\n> \n> Oops, you're right. I'm not sure what happened.\n> \n> > On Wed, Mar 19, 2025 at 06:12:21PM +0100, Barnabás Pőcze wrote:\n> >> 2025. 03. 19. 17:11 keltezéssel, Stefan Klug írta:\n> >>> For calculations in upcoming algorithm patches, the inverse of a matrix\n> >>> is required. Add an implementation of the inverse() function for square\n> >>> matrices.\n> >>>\n> >>> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>\n> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> >>>\n> >>> ---\n> >>>\n> >>> Changes in v2:\n> >>> - Replaced the implementation by a generic one provided by Laurent that\n> >>>     supports arbitrary square matrices instead of 2x2 and 3x3 only.\n> >>> - Moved the implementation into the cpp file.\n> >>> ---\n> >>>    include/libcamera/internal/matrix.h |  16 +++\n> >>>    src/libcamera/matrix.cpp            | 160 ++++++++++++++++++++++++++++\n> >>>    2 files changed, 176 insertions(+)\n> >>>\n> >>> diff --git a/include/libcamera/internal/matrix.h b/include/libcamera/internal/matrix.h\n> >>> index 9b80521e3cb0..6e3c190286fe 100644\n> >>> --- a/include/libcamera/internal/matrix.h\n> >>> +++ b/include/libcamera/internal/matrix.h\n> >>> @@ -19,6 +19,11 @@ namespace libcamera {\n> >>>    \n> >>>    LOG_DECLARE_CATEGORY(Matrix)\n> >>>    \n> >>> +#ifndef __DOXYGEN__\n> >>> +template<typename T>\n> >>> +bool matrixInvert(Span<const T> dataIn, Span<T> dataOut, unsigned int dim);\n> >>> +#endif /* __DOXYGEN__ */\n> >>> +\n> >>>    template<typename T, unsigned int Rows, unsigned int Cols>\n> >>>    class Matrix\n> >>>    {\n> >>> @@ -88,6 +93,17 @@ public:\n> >>>    \t\treturn *this;\n> >>>    \t}\n> >>>    \n> >>> +\tMatrix<T, Rows, Cols> inverse(bool *ok = nullptr) const\n> >>\n> >> Returning `std::optional<...>`? Or `std::pair<Matrix<...>, bool>` if the\n> >> returned matrix is in any way useful?\n> >>\n> >>\n> >>> +\t{\n> >>> +\t\tstatic_assert(Rows == Cols, \"Matrix must be square\");\n> >>> +\n> >>> +\t\tMatrix<T, Rows, Cols> inverse;\n> >>> +\t\tbool res = matrixInvert(Span<const T>(data_), Span<T>(inverse.data_), Rows);\n> >>> +\t\tif (ok)\n> >>> +\t\t\t*ok = res;\n> >>> +\t\treturn inverse;\n> >>> +\t}\n> >>> +\n> >>>    private:\n> >>>    \tstd::array<T, Rows * Cols> data_{};\n> >>>    };\n> >>> diff --git a/src/libcamera/matrix.cpp b/src/libcamera/matrix.cpp\n> >>> index 6dca7498cab3..8590f8efeff3 100644\n> >>> --- a/src/libcamera/matrix.cpp\n> >>> +++ b/src/libcamera/matrix.cpp\n> >>> @@ -7,6 +7,12 @@\n> >>>    \n> >>>    #include \"libcamera/internal/matrix.h\"\n> >>>    \n> >>> +#include <algorithm>\n> >>> +#include <assert.h>\n> >>> +#include <cmath>\n> >>> +#include <numeric>\n> >>> +#include <vector>\n> >>> +\n> >>>    #include <libcamera/base/log.h>\n> >>>    \n> >>>    /**\n> >>> @@ -87,6 +93,20 @@ LOG_DEFINE_CATEGORY(Matrix)\n> >>>     * \\return Row \\a i from the matrix, as a Span\n> >>>     */\n> >>>    \n> >>> +/**\n> >>> + * \\fn Matrix::inverse(bool *ok) const\n> >>> + * \\param[out] ok Indicate if the matrix was successfully inverted\n> >>> + * \\brief Compute the inverse of the matrix\n> >>> + *\n> >>> + * This function computes the inverse of the matrix. It is only implemented for\n> >>> + * matrices of float and double types. If \\a ok is provided it will be set to a\n> >>> + * boolean value to indicate of the inversion was successful. This can be used\n> >>> + * to check if the matrix is singular, in which case the function will return\n> >>> + * an identity matrix.\n> >>> + *\n> >>> + * \\return The inverse of the matrix\n> >>> + */\n> >>> +\n> >>>    /**\n> >>>     * \\fn Matrix::operator[](size_t i)\n> >>>     * \\copydoc Matrix::operator[](size_t i) const\n> >>> @@ -142,6 +162,146 @@ LOG_DEFINE_CATEGORY(Matrix)\n> >>>     */\n> >>>    \n> >>>    #ifndef __DOXYGEN__\n> >>> +template<typename T>\n> >>> +bool matrixInvert(Span<const T> dataIn, Span<T> dataOut, unsigned int dim)\n> >>\n> >> Sorry, but I think this is a bit of an overkill:\n> >>\n> >>     (1) it is (most likely) slower than hard-coding the inversion of a 3x3 matrix\n> >>         (where it would be used);\n> > \n> > Is this a real concern, given our usage patterns ? If so we could use\n> > template specialization for 3x3 matrices.\n> \n> The only user I can see is `libcamera::ipa::rkisp1::algorithms::Awb::calculateRgbMeans()`,\n> and as far as I can tell that runs on every frame. So I don't think this is ideal, and while\n> of course this in and of itself is not a big issue, I think many small things add up over time.\n\nWe can easily add an optimized template specialization, right ? I'd be\ncurious to know how big the improvement is.\n\nSmall things do that up, but if none of them are significant by\nthemselves, it means we'll have to optimize them all. Eventually, we\nshould consider switching to an off-the-shelf linear algebra library.\n\n> >>     (2) it adds dynamic memory allocations.\n> >>\n> >> Or am I missing something? I suppose (2) could be addressed by providing a\n> >> \"scratch buffer\" as well, but I think (1) still stands.\n\nI forgot to reply to this. We could easily allocate the memory on the\nstack, yes.\n\n> >>> +{\n> >>> +\t/*\n> >>> +\t * Convenience class to access matrix data, providing a row-major (i,j)\n> >>> +\t * element accessor through the call operator, and the ability to swap\n> >>> +\t * rows without modifying the backing storage.\n> >>> +\t */\n> >>> +\tclass MatrixAccessor\n> >>> +\t{\n> >>> +\tpublic:\n> >>> +\t\tMatrixAccessor(Span<T> data, unsigned int rows, unsigned int cols)\n> >>> +\t\t\t: data_(data), swap_(rows), rows_(rows), cols_(cols)\n> >>> +\t\t{\n> >>> +\t\t\tstd::iota(swap_.begin(), swap_.end(), T{ 0 });\n> >>> +\t\t}\n> >>> +\n> >>> +\t\tT &operator()(unsigned int row, unsigned int col)\n> >>> +\t\t{\n> >>> +\t\t\tassert(row < rows_ && col < cols_);\n> >>> +\t\t\treturn data_[index(row, col)];\n> >>> +\t\t}\n> >>> +\n> >>> +\t\tvoid swap(unsigned int a, unsigned int b)\n> >>> +\t\t{\n> >>> +\t\t\tassert(a < rows_ && a < cols_);\n> >>> +\t\t\tstd::swap(swap_[a], swap_[b]);\n> >>> +\t\t}\n> >>> +\n> >>> +\tprivate:\n> >>> +\t\tunsigned int index(unsigned int row, unsigned int col) const\n> >>> +\t\t{\n> >>> +\t\t\treturn swap_[row] * cols_ + col;\n> >>> +\t\t}\n> >>> +\n> >>> +\t\tSpan<T> data_;\n> >>> +\t\tstd::vector<unsigned int> swap_;\n> >>> +\t\tunsigned int rows_;\n> >>> +\t\tunsigned int cols_;\n> >>> +\t};\n> >>> +\n> >>> +\t/*\n> >>> +\t * Matrix inversion using Gaussian elimination.\n> >>> +\t *\n> >>> +\t * Start by augmenting the original matrix with an identiy matrix of\n> >>> +\t * the same size.\n> >>> +\t */\n> >>> +\tstd::vector<T> data(dim * dim * 2);\n> >>> +\tMatrixAccessor matrix(data, dim, dim * 2);\n> >>> +\n> >>> +\tfor (unsigned int i = 0; i < dim; ++i) {\n> >>> +\t\tfor (unsigned int j = 0; j < dim; ++j)\n> >>> +\t\t\tmatrix(i, j) = dataIn[i * dim + j];\n> >>> +\t\tmatrix(i, i + dim) = T{ 1 };\n> >>> +\t}\n> >>> +\n> >>> +\t/* Start by triangularizing the input . */\n> >>> +\tfor (unsigned int pivot = 0; pivot < dim; ++pivot) {\n> >>> +\t\t/*\n> >>> +\t\t * Locate the next pivot. To improve numerical stability, use\n> >>> +\t\t * the row with the largest value in the pivot's column.\n> >>> +\t\t */\n> >>> +\t\tunsigned int row;\n> >>> +\t\tT maxValue{ 0 };\n> >>> +\n> >>> +\t\tfor (unsigned int i = pivot; i < dim; ++i) {\n> >>> +\t\t\tT value = std::abs(matrix(i, pivot));\n> >>> +\t\t\tif (maxValue < value) {\n> >>> +\t\t\t\tmaxValue = value;\n> >>> +\t\t\t\trow = i;\n> >>> +\t\t\t}\n> >>> +\t\t}\n> >>> +\n> >>> +\t\t/*\n> >>> +\t\t * If no pivot is found in the column, the matrix is not\n> >>> +\t\t * invertible. Return an identity matrix.\n> >>> +\t\t */\n> >>> +\t\tif (maxValue == 0) {\n> >>> +\t\t\tstd::fill(dataOut.begin(), dataOut.end(), T{ 0 });\n> >>> +\t\t\tfor (unsigned int i = 0; i < dim; ++i)\n> >>> +\t\t\t\tdataOut[i * dim + i] = T{ 1 };\n> >>> +\t\t\treturn false;\n> >>> +\t\t}\n> >>> +\n> >>> +\t\t/* Swap rows to bring the pivot in the right location. */\n> >>> +\t\tmatrix.swap(pivot, row);\n> >>> +\n> >>> +\t\t/* Process all rows below the pivot to zero the pivot column. */\n> >>> +\t\tconst T pivotValue = matrix(pivot, pivot);\n> >>> +\n> >>> +\t\tfor (unsigned int i = pivot + 1; i < dim; ++i) {\n> >>> +\t\t\tconst T factor = matrix(i, pivot) / pivotValue;\n> >>> +\n> >>> +\t\t\t/*\n> >>> +\t\t\t * We know the element in the pivot column will be 0,\n> >>> +\t\t\t * hardcode it instead of computing it.\n> >>> +\t\t\t */\n> >>> +\t\t\tmatrix(i, pivot) = T{ 0 };\n> >>> +\n> >>> +\t\t\tfor (unsigned int j = pivot + 1; j < dim * 2; ++j)\n> >>> +\t\t\t\tmatrix(i, j) -= matrix(pivot, j) * factor;\n> >>> +\t\t}\n> >>> +\t}\n> >>> +\n> >>> +\t/*\n> >>> +\t * Then diagonalize the input, walking the diagonal backwards. There's\n> >>> +\t * no need to update the input matrix, as all the values we would write\n> >>> +\t * in the top-right triangle aren't used in further calculations (and\n> >>> +\t * would all by definition be zero).\n> >>> +\t */\n> >>> +\tfor (unsigned int pivot = dim - 1; pivot > 0; --pivot) {\n> >>> +\t\tconst T pivotValue = matrix(pivot, pivot);\n> >>> +\n> >>> +\t\tfor (unsigned int i = 0; i < pivot; ++i) {\n> >>> +\t\t\tconst T factor = matrix(i, pivot) / pivotValue;\n> >>> +\n> >>> +\t\t\tfor (unsigned int j = dim; j < dim * 2; ++j)\n> >>> +\t\t\t\tmatrix(i, j) -= matrix(pivot, j) * factor;\n> >>> +\t\t}\n> >>> +\t}\n> >>> +\n> >>> +\t/*\n> >>> +\t * Finally, normalize the diagonal and store the result in the output\n> >>> +\t * data.\n> >>> +\t */\n> >>> +\tfor (unsigned int i = 0; i < dim; ++i) {\n> >>> +\t\tconst T factor = matrix(i, i);\n> >>> +\n> >>> +\t\tfor (unsigned int j = 0; j < dim; ++j)\n> >>> +\t\t\tdataOut[i * dim + j] = matrix(i, j + dim) / factor;\n> >>> +\t}\n> >>> +\n> >>> +\treturn true;\n> >>> +}\n> >>> +\n> >>> +template bool matrixInvert<float>(Span<const float> dataIn, Span<float> dataOut,\n> >>> +\t\t\t\t  unsigned int dim);\n> >>> +template bool matrixInvert<double>(Span<const double> data, Span<double> dataOut,\n> >>> +\t\t\t\t   unsigned int dim);\n> >>> +\n> >>>    /*\n> >>>     * The YAML data shall be a list of numerical values. Its size shall be equal\n> >>>     * to the product of the number of rows and columns of the matrix (Rows x","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 03AA9C3213\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 31 Mar 2025 15:11:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1C3CB68986;\n\tMon, 31 Mar 2025 17:11:38 +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 DCDFC68967\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 31 Mar 2025 17:11:36 +0200 (CEST)","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 0C7C4703;\n\tMon, 31 Mar 2025 17:09:44 +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=\"XeupFtNR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1743433785;\n\tbh=FTEfCc00e/jVN1PJ3PXoLszEDcexah3nHomrro+uIxQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XeupFtNR4zY5L4w3xma+JDUljql3mLME5jR2wD/HOQ1UefoK9lWdb0jZ8VoXvbASn\n\tvkJ6ZCXoYYEq3J0jyOAWs4nZ+0deWwx+9vMFLIO9el6iXQll2M+vUecGtkUpXe1Tsi\n\tPyAsIHwELACAvMsMrbtvACsmFJITnHbmqxAape2o=","Date":"Mon, 31 Mar 2025 18:11:12 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <barnabas.pocze@ideasonboard.com>","Cc":"Stefan Klug <stefan.klug@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 06/17] test: Add minimal test for Matrix","Message-ID":"<20250331151112.GA11469@pendragon.ideasonboard.com>","References":"<20250319161152.63625-1-stefan.klug@ideasonboard.com>\n\t<20250319161152.63625-7-stefan.klug@ideasonboard.com>\n\t<f96bdd71-55c8-4627-aadc-a23c296ea2cf@ideasonboard.com>\n\t<20250331131511.GA27882@pendragon.ideasonboard.com>\n\t<5c53336e-12aa-41de-9e23-d98182b7c7af@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<5c53336e-12aa-41de-9e23-d98182b7c7af@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":33793,"web_url":"https://patchwork.libcamera.org/comment/33793/","msgid":"<174344084421.3394313.1792162953474602054@ping.linuxembedded.co.uk>","date":"2025-03-31T17:07:24","subject":"Re: [PATCH v2 06/17] test: Add minimal test for Matrix","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2025-03-19 16:11:11)\n> Add a few tests for the Matrix class. This is not full fledged but at\n> least a starter.\n> \n> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>\n> \n> ---\n> \n> Changes in v2:\n> - Added this patch\n> ---\n>  test/matrix.cpp  | 53 ++++++++++++++++++++++++++++++++++++++++++++++++\n>  test/meson.build |  1 +\n>  2 files changed, 54 insertions(+)\n>  create mode 100644 test/matrix.cpp\n> \n> diff --git a/test/matrix.cpp b/test/matrix.cpp\n> new file mode 100644\n> index 000000000000..93a3b95db2dc\n> --- /dev/null\n> +++ b/test/matrix.cpp\n> @@ -0,0 +1,53 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2024, Ideas on Board Oy\n> + *\n> + * Vector tests\n> + */\n> +\n> +#include \"libcamera/internal/matrix.h\"\n> +\n> +#include <cmath>\n> +#include <iostream>\n> +\n> +#include \"test.h\"\n> +\n> +using namespace libcamera;\n> +\n> +#define ASSERT_EQ(a, b)                                                   \\\n> +       if ((a) != (b)) {                                                 \\\n> +               std::cout << #a \" != \" #b << \" (line \" << __LINE__ << \")\" \\\n> +                         << std::endl;                                   \\\n> +               return TestFail;                                          \\\n> +       }\n> +\n> +class VectorTest : public Test\n> +{\n> +protected:\n> +       int run()\n> +       {\n> +               Matrix<double, 3, 3> m1;\n> +\n> +               ASSERT_EQ(m1[0][0], 0.0);\n> +               ASSERT_EQ(m1[0][1], 0.0);\n> +\n> +               constexpr Matrix<float, 2, 2> m2 = Matrix<float, 2, 2>().identity();\n> +               ASSERT_EQ(m2[0][0], 1.0);\n> +               ASSERT_EQ(m2[0][1], 0.0);\n> +               ASSERT_EQ(m2[1][0], 0.0);\n> +               ASSERT_EQ(m2[1][1], 1.0);\n> +\n> +               Matrix<float, 2, 2> m3{ { 2.0, 0.0, 0.0, 2.0 } };\n> +               Matrix<float, 2, 2> m4 = m3.inverse();\n> +\n> +               Matrix<float, 2, 2> m5 = m3 * m4;\n> +               ASSERT_EQ(m5[0][0], 1.0);\n> +               ASSERT_EQ(m5[0][1], 0.0);\n> +               ASSERT_EQ(m5[1][0], 0.0);\n> +               ASSERT_EQ(m5[1][1], 1.0);\n> +\n> +               return TestPass;\n> +       }\n> +};\n> +\n> +TEST_REGISTER(VectorTest)\n\nNot 'MatrixTest' ?\n\nAside from that, I'd put a tag, as tests here are helpful ... I'll leave\nthe implementation detail on how to implement the Matrix features to\nBarnabas+Laurent to continue to discuss....\n\n> diff --git a/test/meson.build b/test/meson.build\n> index 4095664994fd..52f04364e4fc 100644\n> --- a/test/meson.build\n> +++ b/test/meson.build\n> @@ -60,6 +60,7 @@ internal_tests = [\n>      {'name': 'file', 'sources': ['file.cpp']},\n>      {'name': 'flags', 'sources': ['flags.cpp']},\n>      {'name': 'hotplug-cameras', 'sources': ['hotplug-cameras.cpp']},\n> +    {'name': 'matrix', 'sources': ['matrix.cpp']},\n>      {'name': 'message', 'sources': ['message.cpp']},\n>      {'name': 'object', 'sources': ['object.cpp']},\n>      {'name': 'object-delete', 'sources': ['object-delete.cpp']},\n> -- \n> 2.43.0\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 3228CC3213\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 31 Mar 2025 17:07:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2111868985;\n\tMon, 31 Mar 2025 19:07:29 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 51DAC6897A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 31 Mar 2025 19:07:27 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9EA4A703;\n\tMon, 31 Mar 2025 19:05:35 +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=\"Qf7uRHP9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1743440735;\n\tbh=pqm19jAb61ofUQLN5grNAbIQVH5Rzc9bT3N/Zxx+zmM=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=Qf7uRHP9cKOCG2p8IAfQeM4Va7yMBgQG9wToesPvysBjJMrXYsdC09FjffSWtMNx4\n\tuMkMq7nmlNcLV4zKkR4qzYaxB5ITd0jUSY3gzarQIN6ql55FfCEJxbWdIMsXGHChm8\n\teHishbPqavz07PPOI1g6Y7UM8AQBiyOxKzQ9y+rA=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250319161152.63625-7-stefan.klug@ideasonboard.com>","References":"<20250319161152.63625-1-stefan.klug@ideasonboard.com>\n\t<20250319161152.63625-7-stefan.klug@ideasonboard.com>","Subject":"Re: [PATCH v2 06/17] test: Add minimal test for Matrix","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"Stefan Klug <stefan.klug@ideasonboard.com>","To":"Stefan Klug <stefan.klug@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 31 Mar 2025 18:07:24 +0100","Message-ID":"<174344084421.3394313.1792162953474602054@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":33828,"web_url":"https://patchwork.libcamera.org/comment/33828/","msgid":"<20250401010908.GS14432@pendragon.ideasonboard.com>","date":"2025-04-01T01:09:08","subject":"Re: [PATCH v2 06/17] test: Add minimal test for Matrix","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Mar 31, 2025 at 06:07:24PM +0100, Kieran Bingham wrote:\n> Quoting Stefan Klug (2025-03-19 16:11:11)\n> > Add a few tests for the Matrix class. This is not full fledged but at\n> > least a starter.\n> > \n> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>\n> > \n> > ---\n> > \n> > Changes in v2:\n> > - Added this patch\n> > ---\n> >  test/matrix.cpp  | 53 ++++++++++++++++++++++++++++++++++++++++++++++++\n> >  test/meson.build |  1 +\n> >  2 files changed, 54 insertions(+)\n> >  create mode 100644 test/matrix.cpp\n> > \n> > diff --git a/test/matrix.cpp b/test/matrix.cpp\n> > new file mode 100644\n> > index 000000000000..93a3b95db2dc\n> > --- /dev/null\n> > +++ b/test/matrix.cpp\n> > @@ -0,0 +1,53 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Ideas on Board Oy\n> > + *\n> > + * Vector tests\n> > + */\n> > +\n> > +#include \"libcamera/internal/matrix.h\"\n> > +\n> > +#include <cmath>\n> > +#include <iostream>\n> > +\n> > +#include \"test.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +#define ASSERT_EQ(a, b)                                                   \\\n> > +       if ((a) != (b)) {                                                 \\\n> > +               std::cout << #a \" != \" #b << \" (line \" << __LINE__ << \")\" \\\n> > +                         << std::endl;                                   \\\n> > +               return TestFail;                                          \\\n> > +       }\n> > +\n> > +class VectorTest : public Test\n> > +{\n> > +protected:\n> > +       int run()\n> > +       {\n> > +               Matrix<double, 3, 3> m1;\n> > +\n> > +               ASSERT_EQ(m1[0][0], 0.0);\n> > +               ASSERT_EQ(m1[0][1], 0.0);\n> > +\n> > +               constexpr Matrix<float, 2, 2> m2 = Matrix<float, 2, 2>().identity();\n> > +               ASSERT_EQ(m2[0][0], 1.0);\n> > +               ASSERT_EQ(m2[0][1], 0.0);\n> > +               ASSERT_EQ(m2[1][0], 0.0);\n> > +               ASSERT_EQ(m2[1][1], 1.0);\n> > +\n> > +               Matrix<float, 2, 2> m3{ { 2.0, 0.0, 0.0, 2.0 } };\n> > +               Matrix<float, 2, 2> m4 = m3.inverse();\n> > +\n> > +               Matrix<float, 2, 2> m5 = m3 * m4;\n> > +               ASSERT_EQ(m5[0][0], 1.0);\n> > +               ASSERT_EQ(m5[0][1], 0.0);\n> > +               ASSERT_EQ(m5[1][0], 0.0);\n> > +               ASSERT_EQ(m5[1][1], 1.0);\n> > +\n> > +               return TestPass;\n> > +       }\n> > +};\n> > +\n> > +TEST_REGISTER(VectorTest)\n> \n> Not 'MatrixTest' ?\n> \n> Aside from that, I'd put a tag, as tests here are helpful ... I'll leave\n> the implementation detail on how to implement the Matrix features to\n> Barnabas+Laurent to continue to discuss....\n\nWith the test class renamed,\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n> > diff --git a/test/meson.build b/test/meson.build\n> > index 4095664994fd..52f04364e4fc 100644\n> > --- a/test/meson.build\n> > +++ b/test/meson.build\n> > @@ -60,6 +60,7 @@ internal_tests = [\n> >      {'name': 'file', 'sources': ['file.cpp']},\n> >      {'name': 'flags', 'sources': ['flags.cpp']},\n> >      {'name': 'hotplug-cameras', 'sources': ['hotplug-cameras.cpp']},\n> > +    {'name': 'matrix', 'sources': ['matrix.cpp']},\n> >      {'name': 'message', 'sources': ['message.cpp']},\n> >      {'name': 'object', 'sources': ['object.cpp']},\n> >      {'name': 'object-delete', 'sources': ['object-delete.cpp']},","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 F0E8AC323E\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue,  1 Apr 2025 01:09:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9806168985;\n\tTue,  1 Apr 2025 03:09:33 +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 8864262C66\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue,  1 Apr 2025 03:09:32 +0200 (CEST)","from pendragon.ideasonboard.com (85-76-147-224-nat.elisa-mobile.fi\n\t[85.76.147.224])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9FE26564;\n\tTue,  1 Apr 2025 03:07:40 +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=\"bqEDMON0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1743469660;\n\tbh=zDYjm61BmC4j+Uw5+lSA7kSFKmx8QGgIs92SCUHyt0I=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bqEDMON0nivm71GsoXTAJJgCjjQkajn7UHmD6t8eoz30+WPasNlI+UfEukjrEXQbA\n\tLy5dxWYleoeaYObIlicse4vpPagxgorD0e5U82QQBwcM+6eSfRHhdfvCSjq/Ole3pG\n\tp9YTaJYiuXDK40D965D1Bi+U5CTWXQDo9+5ITRpQ=","Date":"Tue, 1 Apr 2025 04:09:08 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"Stefan Klug <stefan.klug@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 06/17] test: Add minimal test for Matrix","Message-ID":"<20250401010908.GS14432@pendragon.ideasonboard.com>","References":"<20250319161152.63625-1-stefan.klug@ideasonboard.com>\n\t<20250319161152.63625-7-stefan.klug@ideasonboard.com>\n\t<174344084421.3394313.1792162953474602054@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<174344084421.3394313.1792162953474602054@ping.linuxembedded.co.uk>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]