[{"id":33376,"web_url":"https://patchwork.libcamera.org/comment/33376/","msgid":"<20250217114252.GB16021@pendragon.ideasonboard.com>","date":"2025-02-17T11:42:52","subject":"Re: [PATCH 01/10] libcamera: matrix: Add cast function","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, Feb 17, 2025 at 11:01:42AM +0100, Stefan Klug wrote:\n> The libcamera code base uses matrices and vectors of type float and\n> double. Add a cast function to easily convert between different\n> underlying types.\n\nShould we standardize on one of the floating point precisions to avoid\ncasts ?\n\n> Signed-off-by: Stefan Klug \n> ---\n> include/libcamera/internal/matrix.h | 11 +++++++++++\n> src/libcamera/matrix.cpp | 10 ++++++++++\n> 2 files changed, 21 insertions(+)\n> \n> diff --git a/include/libcamera/internal/matrix.h b/include/libcamera/internal/matrix.h\n> index a055e6926c94..ca81dcda93c4 100644\n> --- a/include/libcamera/internal/matrix.h\n> +++ b/include/libcamera/internal/matrix.h\n> @@ -90,6 +90,17 @@ public:\n> \t\treturn *this;\n> \t}\n> \n> +\ttemplate\n> +\tMatrix cast() const\n\nI wonder if we should use a \"copy\" constructor instead of a cast()\nfunction, to make it clear a copy iw made.\n\n> +\t{\n> +\t\tMatrix ret;\n> +\t\tfor (unsigned int i = 0; i < Rows; i++)\n> +\t\t\tfor (unsigned int j = 0; j < Cols; j++)\n> +\t\t\t\tret[i][j] = static_cast((*this)[i][j]);\n\nYou can operator on data_ directly, with a single loop.\n\n> +\n> +\t\treturn ret;\n> +\t}\n> +\n> private:\n> \tstd::array data_;\n> };\n> diff --git a/src/libcamera/matrix.cpp b/src/libcamera/matrix.cpp\n> index e7e027225666..91a3f16405a3 100644\n> --- a/src/libcamera/matrix.cpp\n> +++ b/src/libcamera/matrix.cpp\n> @@ -77,6 +77,16 @@ LOG_DEFINE_CATEGORY(Matrix)\n> * \\return Row \\a i from the matrix, as a Span\n> */\n> \n> +/**\n> + * \\fn template Matrix Matrix::cast() const\n> + * \\brief Cast the matrix to a different type\n> + *\n> + * This function returns a new matrix with the same size and values but a\n> + * different type.\n> + *\n> + * \\return The new matrix\n> + */\n> +\n> /**\n> * \\fn Matrix::operator[](size_t i)\n> * \\copydoc Matrix::operator[](size_t i) const","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 C4040BE08B\n\tfor ;\n\tMon, 17 Feb 2025 11:43:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C826968666;\n\tMon, 17 Feb 2025 12:43:08 +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 959D961865\n\tfor ;\n\tMon, 17 Feb 2025 12:43:06 +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 2341922F;\n\tMon, 17 Feb 2025 12:41:45 +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=\"Lsbgb6gd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1739792505;\n\tbh=FmKNpL05Ufw1gV1UQd9/DqtkBUei++4KzPTUreZPmqs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Lsbgb6gdYegRQ3DbBjUGiMzkhXtplCAzci5FjThNEslwQiE5PZWysUxeAvUb4b+1A\n\t0AfHrcnWss2vWlKk7LQjWduTgd5b9ZAe/5rHM0K/kZ+dPTfiGIpymmla7N5zb2gvzD\n\tnNIg94rJN/1LlwQk/DfBVT5HRNvRRLgTBHgbY1+0=","Date":"Mon, 17 Feb 2025 13:42:52 +0200","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 01/10] libcamera: matrix: Add cast function","Message-ID":"<20250217114252.GB16021@pendragon.ideasonboard.com>","References":"<20250217100203.297894-1-stefan.klug@ideasonboard.com>\n\t<20250217100203.297894-2-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250217100203.297894-2-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":33385,"web_url":"https://patchwork.libcamera.org/comment/33385/","msgid":"","date":"2025-02-17T15:02:55","subject":"Re: [PATCH 01/10] libcamera: matrix: Add cast function","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, Feb 17, 2025 at 01:42:52PM +0200, Laurent Pinchart wrote:\n> Hi Stefan,\n> \n> Thank you for the patch.\n> \n> On Mon, Feb 17, 2025 at 11:01:42AM +0100, Stefan Klug wrote:\n> > The libcamera code base uses matrices and vectors of type float and\n> > double. Add a cast function to easily convert between different\n> > underlying types.\n> \n> Should we standardize on one of the floating point precisions to avoid\n> casts ?\n\nI thought about that before. But grepping through the source, I don't\nreally see that. All controls are in float (which is fine). For\ncalculations I tend to go for double. These might work in float also,\nbut I don't think it's worth forcing the whole project.\n\n> \n> > Signed-off-by: Stefan Klug \n> > ---\n> > include/libcamera/internal/matrix.h | 11 +++++++++++\n> > src/libcamera/matrix.cpp | 10 ++++++++++\n> > 2 files changed, 21 insertions(+)\n> > \n> > diff --git a/include/libcamera/internal/matrix.h b/include/libcamera/internal/matrix.h\n> > index a055e6926c94..ca81dcda93c4 100644\n> > --- a/include/libcamera/internal/matrix.h\n> > +++ b/include/libcamera/internal/matrix.h\n> > @@ -90,6 +90,17 @@ public:\n> > \t\treturn *this;\n> > \t}\n> > \n> > +\ttemplate\n> > +\tMatrix cast() const\n> \n> I wonder if we should use a \"copy\" constructor instead of a cast()\n> function, to make it clear a copy iw made.\n\nI tried that before. I guess it's a matter of taste and in the end I'm\nfine with either way. The cast function was just easier to type and you\ndon't have to repeat the dimensions.\n\ne.g.:\n\nv = Matrix(m) * a;\n\nvs\n\nv = m.cast() * a;\n\n> \n> > +\t{\n> > +\t\tMatrix ret;\n> > +\t\tfor (unsigned int i = 0; i < Rows; i++)\n> > +\t\t\tfor (unsigned int j = 0; j < Cols; j++)\n> > +\t\t\t\tret[i][j] = static_cast((*this)[i][j]);\n> \n> You can operator on data_ directly, with a single loop.\n\nIn the \"copy\" constructor case, yes. Not in this case because ret.data_ is\nprivate from within this class.\n\nBest regards,\nStefan\n\n> \n> > +\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > private:\n> > \tstd::array data_;\n> > };\n> > diff --git a/src/libcamera/matrix.cpp b/src/libcamera/matrix.cpp\n> > index e7e027225666..91a3f16405a3 100644\n> > --- a/src/libcamera/matrix.cpp\n> > +++ b/src/libcamera/matrix.cpp\n> > @@ -77,6 +77,16 @@ LOG_DEFINE_CATEGORY(Matrix)\n> > * \\return Row \\a i from the matrix, as a Span\n> > */\n> > \n> > +/**\n> > + * \\fn template Matrix Matrix::cast() const\n> > + * \\brief Cast the matrix to a different type\n> > + *\n> > + * This function returns a new matrix with the same size and values but a\n> > + * different type.\n> > + *\n> > + * \\return The new matrix\n> > + */\n> > +\n> > /**\n> > * \\fn Matrix::operator[](size_t i)\n> > * \\copydoc Matrix::operator[](size_t i) const\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 C7318BE08B\n\tfor ;\n\tMon, 17 Feb 2025 15:03:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E235F6866F;\n\tMon, 17 Feb 2025 16:02:59 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F278961865\n\tfor ;\n\tMon, 17 Feb 2025 16:02:58 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:b47f:e20a:c4c7:ece1])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5AD1E22F;\n\tMon, 17 Feb 2025 16:01:37 +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=\"IP9Ua93X\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1739804497;\n\tbh=aabQeDUZ+vrrgEJ8i+XpGw+I9FGOzFdPOlP9eCS55MU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IP9Ua93X9E2aZD1myOJeS8dUWAFzeUCzKaj70ziJR3XadHwZWctVl3evKbZZgaW9b\n\tbqHD5i5D4NGeHxNp+ginFwiRFcxMHgUdbSg1CSZg0Uk0ySm9FWy+7glUS5syIYhNAB\n\tmcgpbunOcXlGxS2vzwFNKdWXJylYEKpUNJZALyK8=","Date":"Mon, 17 Feb 2025 16:02:55 +0100","From":"Stefan Klug ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 01/10] libcamera: matrix: Add cast function","Message-ID":"","References":"<20250217100203.297894-1-stefan.klug@ideasonboard.com>\n\t<20250217100203.297894-2-stefan.klug@ideasonboard.com>\n\t<20250217114252.GB16021@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250217114252.GB16021@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\" "}},{"id":33386,"web_url":"https://patchwork.libcamera.org/comment/33386/","msgid":"","date":"2025-02-17T15:13:47","subject":"Re: [PATCH 01/10] libcamera: matrix: Add cast function","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2025. február 17., hétfő 16:02 keltezéssel, Stefan Klug írta:\n\n> Hi Laurent,\n> \n> Thank you for the review.\n> \n> On Mon, Feb 17, 2025 at 01:42:52PM +0200, Laurent Pinchart wrote:\n> > Hi Stefan,\n> >\n> > Thank you for the patch.\n> >\n> > On Mon, Feb 17, 2025 at 11:01:42AM +0100, Stefan Klug wrote:\n> > > The libcamera code base uses matrices and vectors of type float and\n> > > double. Add a cast function to easily convert between different\n> > > underlying types.\n> >\n> > Should we standardize on one of the floating point precisions to avoid\n> > casts ?\n> \n> I thought about that before. But grepping through the source, I don't\n> really see that. All controls are in float (which is fine). For\n> calculations I tend to go for double. These might work in float also,\n> but I don't think it's worth forcing the whole project.\n> \n> >\n> > > Signed-off-by: Stefan Klug \n> > > ---\n> > > include/libcamera/internal/matrix.h | 11 +++++++++++\n> > > src/libcamera/matrix.cpp | 10 ++++++++++\n> > > 2 files changed, 21 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/internal/matrix.h b/include/libcamera/internal/matrix.h\n> > > index a055e6926c94..ca81dcda93c4 100644\n> > > --- a/include/libcamera/internal/matrix.h\n> > > +++ b/include/libcamera/internal/matrix.h\n> > > @@ -90,6 +90,17 @@ public:\n> > > \t\treturn *this;\n> > > \t}\n> > >\n> > > +\ttemplate\n> > > +\tMatrix cast() const\n> >\n> > I wonder if we should use a \"copy\" constructor instead of a cast()\n> > function, to make it clear a copy iw made.\n> \n> I tried that before. I guess it's a matter of taste and in the end I'm\n> fine with either way. The cast function was just easier to type and you\n> don't have to repeat the dimensions.\n> \n> e.g.:\n> \n> v = Matrix(m) * a;\n> \n> vs\n> \n> v = m.cast() * a;\n> \n> >\n> > > +\t{\n> > > +\t\tMatrix ret;\n> > > +\t\tfor (unsigned int i = 0; i < Rows; i++)\n> > > +\t\t\tfor (unsigned int j = 0; j < Cols; j++)\n> > > +\t\t\t\tret[i][j] = static_cast((*this)[i][j]);\n> >\n> > You can operator on data_ directly, with a single loop.\n> \n> In the \"copy\" constructor case, yes. Not in this case because ret.data_ is\n> private from within this class.\n\nBut this is a non-static member function, so it has access to all members of the class.\nThe visibility rules apply to the type, not to the instance, e.g. even a static\nmember function `T::f()` can access arbitrary members of an object of type T.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n> Best regards,\n> Stefan\n> \n> >\n> > > +\n> > > +\t\treturn ret;\n> > > +\t}\n> > > +\n> > > private:\n> > > \tstd::array data_;\n> > > };\n> > > diff --git a/src/libcamera/matrix.cpp b/src/libcamera/matrix.cpp\n> > > index e7e027225666..91a3f16405a3 100644\n> > > --- a/src/libcamera/matrix.cpp\n> > > +++ b/src/libcamera/matrix.cpp\n> > > @@ -77,6 +77,16 @@ LOG_DEFINE_CATEGORY(Matrix)\n> > > * \\return Row \\a i from the matrix, as a Span\n> > > */\n> > >\n> > > +/**\n> > > + * \\fn template Matrix Matrix::cast() const\n> > > + * \\brief Cast the matrix to a different type\n> > > + *\n> > > + * This function returns a new matrix with the same size and values but a\n> > > + * different type.\n> > > + *\n> > > + * \\return The new matrix\n> > > + */\n> > > +\n> > > /**\n> > > * \\fn Matrix::operator[](size_t i)\n> > > * \\copydoc Matrix::operator[](size_t i) const\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 25304BDE6B\n\tfor ;\n\tMon, 17 Feb 2025 15:13:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 725E96866F;\n\tMon, 17 Feb 2025 16:13:54 +0100 (CET)","from mail-4316.protonmail.ch (mail-4316.protonmail.ch\n\t[185.70.43.16])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2278F61865\n\tfor ;\n\tMon, 17 Feb 2025 16:13:53 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"P9lOPeRT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1739805232; x=1740064432;\n\tbh=fa7xYkEFSKLVRAmDZlePuDKOA5jXz4CXhBBNpEZdGEY=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post;\n\tb=P9lOPeRTK0bli6TDuiz4GgaUS1NZzPDsbEdLV4/iI/gkcbL+6OVS36bcWTImnz1wH\n\ttuh/b+nQnXtU4SprO8k09I8DAGP49qsSBaPp6akuhA2Y9WicamHSjg1l+2HbA+D1G+\n\tFU9Y8UgU/P2sdgQVZcn9e2nRLu87OzORFDqKM8pj2zwEEezttmQkVKn6uBMoSqgk5G\n\tH7w7uxUQIqb+RwNCp+8UrKaVp+041wQh1uUfqk1acBog0zLtiLP+DM/TzM6+oM7O/V\n\tHAY2CRD6Ng6l4eBUzReBi3miu9gX5U09IISAh4aAc8y3lZHgvjVQkCeQW/Xorex53r\n\tiA6KVrOhKZSBA==","Date":"Mon, 17 Feb 2025 15:13:47 +0000","To":"Stefan Klug ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 01/10] libcamera: matrix: Add cast function","Message-ID":"","In-Reply-To":"","References":"<20250217100203.297894-1-stefan.klug@ideasonboard.com>\n\t<20250217100203.297894-2-stefan.klug@ideasonboard.com>\n\t<20250217114252.GB16021@pendragon.ideasonboard.com>\n\t","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"cf27efa3cdd0f1c4fd74f8450e745300d9c1e264","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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":33388,"web_url":"https://patchwork.libcamera.org/comment/33388/","msgid":"","date":"2025-02-17T15:31:26","subject":"Re: [PATCH 01/10] libcamera: matrix: Add cast function","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Barnabás,\n\nOn Mon, Feb 17, 2025 at 03:13:47PM +0000, Barnabás Pőcze wrote:\n> Hi\n> \n> \n> 2025. február 17., hétfő 16:02 keltezéssel, Stefan Klug írta:\n> \n> > Hi Laurent,\n> > \n> > Thank you for the review.\n> > \n> > On Mon, Feb 17, 2025 at 01:42:52PM +0200, Laurent Pinchart wrote:\n> > > Hi Stefan,\n> > >\n> > > Thank you for the patch.\n> > >\n> > > On Mon, Feb 17, 2025 at 11:01:42AM +0100, Stefan Klug wrote:\n> > > > The libcamera code base uses matrices and vectors of type float and\n> > > > double. Add a cast function to easily convert between different\n> > > > underlying types.\n> > >\n> > > Should we standardize on one of the floating point precisions to avoid\n> > > casts ?\n> > \n> > I thought about that before. But grepping through the source, I don't\n> > really see that. All controls are in float (which is fine). For\n> > calculations I tend to go for double. These might work in float also,\n> > but I don't think it's worth forcing the whole project.\n> > \n> > >\n> > > > Signed-off-by: Stefan Klug \n> > > > ---\n> > > > include/libcamera/internal/matrix.h | 11 +++++++++++\n> > > > src/libcamera/matrix.cpp | 10 ++++++++++\n> > > > 2 files changed, 21 insertions(+)\n> > > >\n> > > > diff --git a/include/libcamera/internal/matrix.h b/include/libcamera/internal/matrix.h\n> > > > index a055e6926c94..ca81dcda93c4 100644\n> > > > --- a/include/libcamera/internal/matrix.h\n> > > > +++ b/include/libcamera/internal/matrix.h\n> > > > @@ -90,6 +90,17 @@ public:\n> > > > \t\treturn *this;\n> > > > \t}\n> > > >\n> > > > +\ttemplate\n> > > > +\tMatrix cast() const\n> > >\n> > > I wonder if we should use a \"copy\" constructor instead of a cast()\n> > > function, to make it clear a copy iw made.\n> > \n> > I tried that before. I guess it's a matter of taste and in the end I'm\n> > fine with either way. The cast function was just easier to type and you\n> > don't have to repeat the dimensions.\n> > \n> > e.g.:\n> > \n> > v = Matrix(m) * a;\n> > \n> > vs\n> > \n> > v = m.cast() * a;\n> > \n> > >\n> > > > +\t{\n> > > > +\t\tMatrix ret;\n> > > > +\t\tfor (unsigned int i = 0; i < Rows; i++)\n> > > > +\t\t\tfor (unsigned int j = 0; j < Cols; j++)\n> > > > +\t\t\t\tret[i][j] = static_cast((*this)[i][j]);\n> > >\n> > > You can operator on data_ directly, with a single loop.\n> > \n> > In the \"copy\" constructor case, yes. Not in this case because ret.data_ is\n> > private from within this class.\n> \n> But this is a non-static member function, so it has access to all members of the class.\n> The visibility rules apply to the type, not to the instance, e.g. even a static\n> member function `T::f()` can access arbitrary members of an object of type T.\n\nYes, but in this case T::cast() would try to access T2::data_. The class\ntypes are different and so the visibility rule applies. I tried with a friend\ndeclaration but got nowhere (I'm happy to learn how that would look\nlike) and settled for the 2D loop to get it done :-)\n\nBest regards,\nStefan\n\n> \n> \n> Regards,\n> Barnabás Pőcze\n> \n> \n> > \n> > Best regards,\n> > Stefan\n> > \n> > >\n> > > > +\n> > > > +\t\treturn ret;\n> > > > +\t}\n> > > > +\n> > > > private:\n> > > > \tstd::array data_;\n> > > > };\n> > > > diff --git a/src/libcamera/matrix.cpp b/src/libcamera/matrix.cpp\n> > > > index e7e027225666..91a3f16405a3 100644\n> > > > --- a/src/libcamera/matrix.cpp\n> > > > +++ b/src/libcamera/matrix.cpp\n> > > > @@ -77,6 +77,16 @@ LOG_DEFINE_CATEGORY(Matrix)\n> > > > * \\return Row \\a i from the matrix, as a Span\n> > > > */\n> > > >\n> > > > +/**\n> > > > + * \\fn template Matrix Matrix::cast() const\n> > > > + * \\brief Cast the matrix to a different type\n> > > > + *\n> > > > + * This function returns a new matrix with the same size and values but a\n> > > > + * different type.\n> > > > + *\n> > > > + * \\return The new matrix\n> > > > + */\n> > > > +\n> > > > /**\n> > > > * \\fn Matrix::operator[](size_t i)\n> > > > * \\copydoc Matrix::operator[](size_t i) const\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 070C1BE08B\n\tfor ;\n\tMon, 17 Feb 2025 15:31:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 06F9968675;\n\tMon, 17 Feb 2025 16:31:31 +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 B1D5861865\n\tfor ;\n\tMon, 17 Feb 2025 16:31:29 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:b47f:e20a:c4c7:ece1])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2B10922F;\n\tMon, 17 Feb 2025 16:30:08 +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=\"H2qzDLns\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1739806208;\n\tbh=CjiTd7PekESfjXeEOiZxBeF1qQjLHio0Y53lvWql+4E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=H2qzDLnsL9B3JaYKEqnn/5+Uz3H0ueDQMFNVNOsKEO4yAyumTO1QjzMoVw3m7U9wu\n\teViuwSys2nnCR+6am8NFKSlDc4JG7N2iNPu3usHUnwvK0VHOFtZgHvmtPwQ18ZH6Cr\n\t9sRmiEvvrPNQYtSlP31X3mItsUPYnwgjPfVlcbgw=","Date":"Mon, 17 Feb 2025 16:31:26 +0100","From":"Stefan Klug ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Laurent Pinchart , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 01/10] libcamera: matrix: Add cast function","Message-ID":"","References":"<20250217100203.297894-1-stefan.klug@ideasonboard.com>\n\t<20250217100203.297894-2-stefan.klug@ideasonboard.com>\n\t<20250217114252.GB16021@pendragon.ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33389,"web_url":"https://patchwork.libcamera.org/comment/33389/","msgid":"","date":"2025-02-17T15:33:13","subject":"Re: [PATCH 01/10] libcamera: matrix: Add cast function","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"On Mon, Feb 17, 2025 at 04:31:26PM +0100, Stefan Klug wrote:\n> Hi Barnabás,\n> \n> On Mon, Feb 17, 2025 at 03:13:47PM +0000, Barnabás Pőcze wrote:\n> > Hi\n> > \n> > \n> > 2025. február 17., hétfő 16:02 keltezéssel, Stefan Klug írta:\n> > \n> > > Hi Laurent,\n> > > \n> > > Thank you for the review.\n> > > \n> > > On Mon, Feb 17, 2025 at 01:42:52PM +0200, Laurent Pinchart wrote:\n> > > > Hi Stefan,\n> > > >\n> > > > Thank you for the patch.\n> > > >\n> > > > On Mon, Feb 17, 2025 at 11:01:42AM +0100, Stefan Klug wrote:\n> > > > > The libcamera code base uses matrices and vectors of type float and\n> > > > > double. Add a cast function to easily convert between different\n> > > > > underlying types.\n> > > >\n> > > > Should we standardize on one of the floating point precisions to avoid\n> > > > casts ?\n> > > \n> > > I thought about that before. But grepping through the source, I don't\n> > > really see that. All controls are in float (which is fine). For\n> > > calculations I tend to go for double. These might work in float also,\n> > > but I don't think it's worth forcing the whole project.\n> > > \n> > > >\n> > > > > Signed-off-by: Stefan Klug \n> > > > > ---\n> > > > > include/libcamera/internal/matrix.h | 11 +++++++++++\n> > > > > src/libcamera/matrix.cpp | 10 ++++++++++\n> > > > > 2 files changed, 21 insertions(+)\n> > > > >\n> > > > > diff --git a/include/libcamera/internal/matrix.h b/include/libcamera/internal/matrix.h\n> > > > > index a055e6926c94..ca81dcda93c4 100644\n> > > > > --- a/include/libcamera/internal/matrix.h\n> > > > > +++ b/include/libcamera/internal/matrix.h\n> > > > > @@ -90,6 +90,17 @@ public:\n> > > > > \t\treturn *this;\n> > > > > \t}\n> > > > >\n> > > > > +\ttemplate\n> > > > > +\tMatrix cast() const\n> > > >\n> > > > I wonder if we should use a \"copy\" constructor instead of a cast()\n> > > > function, to make it clear a copy iw made.\n> > > \n> > > I tried that before. I guess it's a matter of taste and in the end I'm\n> > > fine with either way. The cast function was just easier to type and you\n> > > don't have to repeat the dimensions.\n> > > \n> > > e.g.:\n> > > \n> > > v = Matrix(m) * a;\n> > > \n> > > vs\n> > > \n> > > v = m.cast() * a;\n> > > \n> > > >\n> > > > > +\t{\n> > > > > +\t\tMatrix ret;\n> > > > > +\t\tfor (unsigned int i = 0; i < Rows; i++)\n> > > > > +\t\t\tfor (unsigned int j = 0; j < Cols; j++)\n> > > > > +\t\t\t\tret[i][j] = static_cast((*this)[i][j]);\n> > > >\n> > > > You can operator on data_ directly, with a single loop.\n> > > \n> > > In the \"copy\" constructor case, yes. Not in this case because ret.data_ is\n> > > private from within this class.\n> > \n> > But this is a non-static member function, so it has access to all members of the class.\n> > The visibility rules apply to the type, not to the instance, e.g. even a static\n> > member function `T::f()` can access arbitrary members of an object of type T.\n> \n> Yes, but in this case T::cast() would try to access T2::data_. The class\n> types are different and so the visibility rule applies. I tried with a friend\n> declaration but got nowhere (I'm happy to learn how that would look\n> like) and settled for the 2D loop to get it done :-)\n> \n> Best regards,\n> Stefan\n\nMaybe we should add a non-const data() function...\n\n> \n> > \n> > \n> > Regards,\n> > Barnabás Pőcze\n> > \n> > \n> > > \n> > > Best regards,\n> > > Stefan\n> > > \n> > > >\n> > > > > +\n> > > > > +\t\treturn ret;\n> > > > > +\t}\n> > > > > +\n> > > > > private:\n> > > > > \tstd::array data_;\n> > > > > };\n> > > > > diff --git a/src/libcamera/matrix.cpp b/src/libcamera/matrix.cpp\n> > > > > index e7e027225666..91a3f16405a3 100644\n> > > > > --- a/src/libcamera/matrix.cpp\n> > > > > +++ b/src/libcamera/matrix.cpp\n> > > > > @@ -77,6 +77,16 @@ LOG_DEFINE_CATEGORY(Matrix)\n> > > > > * \\return Row \\a i from the matrix, as a Span\n> > > > > */\n> > > > >\n> > > > > +/**\n> > > > > + * \\fn template Matrix Matrix::cast() const\n> > > > > + * \\brief Cast the matrix to a different type\n> > > > > + *\n> > > > > + * This function returns a new matrix with the same size and values but a\n> > > > > + * different type.\n> > > > > + *\n> > > > > + * \\return The new matrix\n> > > > > + */\n> > > > > +\n> > > > > /**\n> > > > > * \\fn Matrix::operator[](size_t i)\n> > > > > * \\copydoc Matrix::operator[](size_t i) const\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 D0F9EBDE6B\n\tfor ;\n\tMon, 17 Feb 2025 15:33:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2005768675;\n\tMon, 17 Feb 2025 16:33:18 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 472B961865\n\tfor ;\n\tMon, 17 Feb 2025 16:33:16 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:b47f:e20a:c4c7:ece1])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D323722F;\n\tMon, 17 Feb 2025 16:31:54 +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=\"u9P7KmNN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1739806314;\n\tbh=22Voozk8jMv/Jux1gUztBZXAWS9apDfSFbox4uRoxV4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=u9P7KmNNOseeSHT2TYfD9z2UDl3p51FgOCP7gUzjm946PhHN19uuOPla6U8dAoCDH\n\txJM36g01eOc+5J6XnzXd+0MFx1csfvh+iBvhS1cGNWKvwJoBJpJyaSN/Wyk5jqctDJ\n\tolcBNImw22+DM9Eq/pDQsGmTRHxRB1jkYK3WfX7Q=","Date":"Mon, 17 Feb 2025 16:33:13 +0100","From":"Stefan Klug ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Laurent Pinchart , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 01/10] libcamera: matrix: Add cast function","Message-ID":"","References":"<20250217100203.297894-1-stefan.klug@ideasonboard.com>\n\t<20250217100203.297894-2-stefan.klug@ideasonboard.com>\n\t<20250217114252.GB16021@pendragon.ideasonboard.com>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33390,"web_url":"https://patchwork.libcamera.org/comment/33390/","msgid":"","date":"2025-02-17T15:47:35","subject":"Re: [PATCH 01/10] libcamera: matrix: Add cast function","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2025. február 17., hétfő 16:33 keltezéssel, Stefan Klug írta:\n\n> On Mon, Feb 17, 2025 at 04:31:26PM +0100, Stefan Klug wrote:\n> > Hi Barnabás,\n> >\n> > On Mon, Feb 17, 2025 at 03:13:47PM +0000, Barnabás Pőcze wrote:\n> > > Hi\n> > >\n> > >\n> > > 2025. február 17., hétfő 16:02 keltezéssel, Stefan Klug írta:\n> > >\n> > > > Hi Laurent,\n> > > >\n> > > > Thank you for the review.\n> > > >\n> > > > On Mon, Feb 17, 2025 at 01:42:52PM +0200, Laurent Pinchart wrote:\n> > > > > Hi Stefan,\n> > > > >\n> > > > > Thank you for the patch.\n> > > > >\n> > > > > On Mon, Feb 17, 2025 at 11:01:42AM +0100, Stefan Klug wrote:\n> > > > > > The libcamera code base uses matrices and vectors of type float and\n> > > > > > double. Add a cast function to easily convert between different\n> > > > > > underlying types.\n> > > > >\n> > > > > Should we standardize on one of the floating point precisions to avoid\n> > > > > casts ?\n> > > >\n> > > > I thought about that before. But grepping through the source, I don't\n> > > > really see that. All controls are in float (which is fine). For\n> > > > calculations I tend to go for double. These might work in float also,\n> > > > but I don't think it's worth forcing the whole project.\n> > > >\n> > > > >\n> > > > > > Signed-off-by: Stefan Klug \n> > > > > > ---\n> > > > > > include/libcamera/internal/matrix.h | 11 +++++++++++\n> > > > > > src/libcamera/matrix.cpp | 10 ++++++++++\n> > > > > > 2 files changed, 21 insertions(+)\n> > > > > >\n> > > > > > diff --git a/include/libcamera/internal/matrix.h b/include/libcamera/internal/matrix.h\n> > > > > > index a055e6926c94..ca81dcda93c4 100644\n> > > > > > --- a/include/libcamera/internal/matrix.h\n> > > > > > +++ b/include/libcamera/internal/matrix.h\n> > > > > > @@ -90,6 +90,17 @@ public:\n> > > > > > \t\treturn *this;\n> > > > > > \t}\n> > > > > >\n> > > > > > +\ttemplate\n> > > > > > +\tMatrix cast() const\n> > > > >\n> > > > > I wonder if we should use a \"copy\" constructor instead of a cast()\n> > > > > function, to make it clear a copy iw made.\n> > > >\n> > > > I tried that before. I guess it's a matter of taste and in the end I'm\n> > > > fine with either way. The cast function was just easier to type and you\n> > > > don't have to repeat the dimensions.\n> > > >\n> > > > e.g.:\n> > > >\n> > > > v = Matrix(m) * a;\n> > > >\n> > > > vs\n> > > >\n> > > > v = m.cast() * a;\n> > > >\n> > > > >\n> > > > > > +\t{\n> > > > > > +\t\tMatrix ret;\n> > > > > > +\t\tfor (unsigned int i = 0; i < Rows; i++)\n> > > > > > +\t\t\tfor (unsigned int j = 0; j < Cols; j++)\n> > > > > > +\t\t\t\tret[i][j] = static_cast((*this)[i][j]);\n> > > > >\n> > > > > You can operator on data_ directly, with a single loop.\n> > > >\n> > > > In the \"copy\" constructor case, yes. Not in this case because ret.data_ is\n> > > > private from within this class.\n> > >\n> > > But this is a non-static member function, so it has access to all members of the class.\n> > > The visibility rules apply to the type, not to the instance, e.g. even a static\n> > > member function `T::f()` can access arbitrary members of an object of type T.\n> >\n> > Yes, but in this case T::cast() would try to access T2::data_. The class\n> > types are different and so the visibility rule applies. I tried with a friend\n> > declaration but got nowhere (I'm happy to learn how that would look\n> > like) and settled for the 2D loop to get it done :-)\n\nAhh, sorry, you're right, the types are indeed different... should've checked twice.\nA friend declaration could look like this:\n\n template friend class Matrix;\n\nI'm not sure if it is possible to make it more restrictive (i.e. require same column/row count).\n\n\n> >\n> > Best regards,\n> > Stefan\n> \n> Maybe we should add a non-const data() function...\n\nSince there is already an operator[] that provides mutable view, I think that makes sense.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n> >\n> > >\n> > >\n> > > Regards,\n> > > Barnabás Pőcze\n> > >\n> > >\n> > > >\n> > > > Best regards,\n> > > > Stefan\n> > > >\n> > > > >\n> > > > > > +\n> > > > > > +\t\treturn ret;\n> > > > > > +\t}\n> > > > > > +\n> > > > > > private:\n> > > > > > \tstd::array data_;\n> > > > > > };\n> > > > > > diff --git a/src/libcamera/matrix.cpp b/src/libcamera/matrix.cpp\n> > > > > > index e7e027225666..91a3f16405a3 100644\n> > > > > > --- a/src/libcamera/matrix.cpp\n> > > > > > +++ b/src/libcamera/matrix.cpp\n> > > > > > @@ -77,6 +77,16 @@ LOG_DEFINE_CATEGORY(Matrix)\n> > > > > > * \\return Row \\a i from the matrix, as a Span\n> > > > > > */\n> > > > > >\n> > > > > > +/**\n> > > > > > + * \\fn template Matrix Matrix::cast() const\n> > > > > > + * \\brief Cast the matrix to a different type\n> > > > > > + *\n> > > > > > + * This function returns a new matrix with the same size and values but a\n> > > > > > + * different type.\n> > > > > > + *\n> > > > > > + * \\return The new matrix\n> > > > > > + */\n> > > > > > +\n> > > > > > /**\n> > > > > > * \\fn Matrix::operator[](size_t i)\n> > > > > > * \\copydoc Matrix::operator[](size_t i) const\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 AA57DBE08B\n\tfor ;\n\tMon, 17 Feb 2025 15:47:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E7FAD6867B;\n\tMon, 17 Feb 2025 16:47:42 +0100 (CET)","from mail-40131.protonmail.ch (mail-40131.protonmail.ch\n\t[185.70.40.131])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E56476866F\n\tfor ;\n\tMon, 17 Feb 2025 16:47:40 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"omqpj5SW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1739807260; x=1740066460;\n\tbh=943ds1JHXSlcvSzhB57b+rw/iLyHfKkB13jiZH/6gyQ=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post;\n\tb=omqpj5SWw0rCNrIdpafbDU0tTm2NprX8thwseDiDN+i+U2vpJbrfcpb9zKa+mDDUN\n\tR9M7obMQi+66MyHYGXH6+8RrbTfy+CKRegwS+/0x5rdNjgfhJmFThFEPi7Sx+qIGk1\n\tMgkI3PkBh47r4MadY46p4re2PmvKNNwVI8TwHc+rd6RvKyuQ5Bv2l+XuAG0Xs9VRbR\n\taJzk/t7STJQ9iyaHbXc63pob7c9P2eQxA8RAzYzgSmfDOXZ2hSp8K4zxdzy+WjF5xy\n\tcGvlGiaXMOvCbJSQj8qIY+VFLT7EDpzh/UHLSbKhgbuubHxxE7o6JEf3SvaCEQDNPQ\n\tvs5yOyourtuUw==","Date":"Mon, 17 Feb 2025 15:47:35 +0000","To":"Stefan Klug ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 01/10] libcamera: matrix: Add cast function","Message-ID":"","In-Reply-To":"","References":"<20250217100203.297894-1-stefan.klug@ideasonboard.com>\n\t<20250217100203.297894-2-stefan.klug@ideasonboard.com>\n\t<20250217114252.GB16021@pendragon.ideasonboard.com>\n\t\n\t\n\t\n\t","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"d36f1619562910890889bc26c27f22de4c5f1f0a","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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":33411,"web_url":"https://patchwork.libcamera.org/comment/33411/","msgid":"<20250220120929.GA20111@pendragon.ideasonboard.com>","date":"2025-02-20T12:09:29","subject":"Re: [PATCH 01/10] libcamera: matrix: Add cast function","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Feb 17, 2025 at 04:02:55PM +0100, Stefan Klug wrote:\n> On Mon, Feb 17, 2025 at 01:42:52PM +0200, Laurent Pinchart wrote:\n> > On Mon, Feb 17, 2025 at 11:01:42AM +0100, Stefan Klug wrote:\n> > > The libcamera code base uses matrices and vectors of type float and\n> > > double. Add a cast function to easily convert between different\n> > > underlying types.\n> > \n> > Should we standardize on one of the floating point precisions to avoid\n> > casts ?\n> \n> I thought about that before. But grepping through the source, I don't\n> really see that. All controls are in float (which is fine). For\n> calculations I tend to go for double. These might work in float also,\n> but I don't think it's worth forcing the whole project.\n> \n> > \n> > > Signed-off-by: Stefan Klug \n> > > ---\n> > > include/libcamera/internal/matrix.h | 11 +++++++++++\n> > > src/libcamera/matrix.cpp | 10 ++++++++++\n> > > 2 files changed, 21 insertions(+)\n> > > \n> > > diff --git a/include/libcamera/internal/matrix.h b/include/libcamera/internal/matrix.h\n> > > index a055e6926c94..ca81dcda93c4 100644\n> > > --- a/include/libcamera/internal/matrix.h\n> > > +++ b/include/libcamera/internal/matrix.h\n> > > @@ -90,6 +90,17 @@ public:\n> > > \t\treturn *this;\n> > > \t}\n> > > \n> > > +\ttemplate\n> > > +\tMatrix cast() const\n> > \n> > I wonder if we should use a \"copy\" constructor instead of a cast()\n> > function, to make it clear a copy iw made.\n> \n> I tried that before. I guess it's a matter of taste and in the end I'm\n> fine with either way. The cast function was just easier to type and you\n> don't have to repeat the dimensions.\n> \n> e.g.:\n> \n> v = Matrix(m) * a;\n> \n> vs\n> \n> v = m.cast() * a;\n\nThe thing that bothers me a bit with cast() is that the name implies\nit's just a cast, while a new object is constructed, and data copied. I\ndo agree the syntax is a bit nicer though.\n\nThis being said, static_cast<> can also end up constructing a new\ninstance, and copying data, so maybe using cast() would be fine. Unless\nthere's a better name (and I can't think of one at the moment) I'm OK\nwith a cast() function. We should really minimize its usage though, and\ntry to standardize on float vs. double as much as possible, at least\ninternally.\n\n> > > +\t{\n> > > +\t\tMatrix ret;\n> > > +\t\tfor (unsigned int i = 0; i < Rows; i++)\n> > > +\t\t\tfor (unsigned int j = 0; j < Cols; j++)\n> > > +\t\t\t\tret[i][j] = static_cast((*this)[i][j]);\n> > \n> > You can operator on data_ directly, with a single loop.\n> \n> In the \"copy\" constructor case, yes. Not in this case because ret.data_ is\n> private from within this class.\n> \n> > > +\n> > > +\t\treturn ret;\n> > > +\t}\n> > > +\n> > > private:\n> > > \tstd::array data_;\n> > > };\n> > > diff --git a/src/libcamera/matrix.cpp b/src/libcamera/matrix.cpp\n> > > index e7e027225666..91a3f16405a3 100644\n> > > --- a/src/libcamera/matrix.cpp\n> > > +++ b/src/libcamera/matrix.cpp\n> > > @@ -77,6 +77,16 @@ LOG_DEFINE_CATEGORY(Matrix)\n> > > * \\return Row \\a i from the matrix, as a Span\n> > > */\n> > > \n> > > +/**\n> > > + * \\fn template Matrix Matrix::cast() const\n> > > + * \\brief Cast the matrix to a different type\n> > > + *\n> > > + * This function returns a new matrix with the same size and values but a\n> > > + * different type.\n> > > + *\n> > > + * \\return The new matrix\n> > > + */\n> > > +\n> > > /**\n> > > * \\fn Matrix::operator[](size_t i)\n> > > * \\copydoc Matrix::operator[](size_t i) const","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 AE7B2BEFBE\n\tfor ;\n\tThu, 20 Feb 2025 12:09:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B1031686A1;\n\tThu, 20 Feb 2025 13:09:45 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 83F036032B\n\tfor ;\n\tThu, 20 Feb 2025 13:09:44 +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 C96ABE79;\n\tThu, 20 Feb 2025 13:08:20 +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=\"cBmr+LXw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1740053301;\n\tbh=Ea1KYhl8sizf1AVVO0jpmmpUFLJI7QuvLsYwEr/V35A=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cBmr+LXw6SiybQm5D2YK1EYN2BnbR+YF3kQPu94jjzlIrEmwzUDMqee9+aoDV9uqd\n\tWT9HMLyhevCvT5K/i1PCjGUJ21k6BAZINyBjwPZ/F62gFwiS13GssOlf9dFHKE3Zhk\n\tsQXcrZoO4O/lFDNrDKgNLbjIgFlfP2AAf8hh3gNY=","Date":"Thu, 20 Feb 2025 14:09:29 +0200","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 01/10] libcamera: matrix: Add cast function","Message-ID":"<20250220120929.GA20111@pendragon.ideasonboard.com>","References":"<20250217100203.297894-1-stefan.klug@ideasonboard.com>\n\t<20250217100203.297894-2-stefan.klug@ideasonboard.com>\n\t<20250217114252.GB16021@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]