[{"id":27982,"web_url":"https://patchwork.libcamera.org/comment/27982/","msgid":"<20231018195113.GH1512@pendragon.ideasonboard.com>","date":"2023-10-18T19:51:13","subject":"Re: [libcamera-devel] [PATCH v5 06/12] libcamera: transform: Invert\n\toperator*() operands","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Fri, Sep 01, 2023 at 05:02:09PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> The current definition of operator*(Transform t1, Transform t0) follows\n> the function composition notion, where t0 is applied first then t1 is\n> applied last.\n> \n> In order to introduce operator*(Orientation, Transform) where a\n> Transform is applied on top of an Orientation, invert the operand order\n> of operator*(Transform, Transform) so that usage of operator* with both\n> Orientation and Transform can be made associative.\n> \n> For example:\n> \n> Orientation o;\n> Transform t = t1 * t2\n> Orientation o1 = o * t\n> \t = o * (t1 * t2) = (o * t1) * t2 = o * t1 * t2\n\nSo, if we want to follow mathematical conventions, we shouldn't write\n\n\tOrientation o1 = o * t;\n\nbut\n\n\tOrientation o1 = t(o);\n\nwhich can be done by implementing\n\n\tOrientation Transform::operator(const Orientation *orientation);\n\nThis would lead to\n\n\tt2(t1(o)) == (t2 * t1)(o)\n\nWould that be a better alternative ?\n\n> Signed-off-by: Jacopo Mondi \n> Reviewed-by: David Plowman \n> ---\n> src/libcamera/camera_sensor.cpp | 4 ++--\n> src/libcamera/transform.cpp | 19 +++++++++++--------\n> 2 files changed, 13 insertions(+), 10 deletions(-)\n> \n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index 3ba364c44a40..038d8b959072 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -1051,7 +1051,7 @@ Transform CameraSensor::validateTransform(Transform *transform) const\n> \t * Combine the requested transform to compensate the sensor mounting\n> \t * rotation.\n> \t */\n> -\tTransform combined = *transform * rotationTransform_;\n> +\tTransform combined = rotationTransform_ * *transform;\n> \n> \t/*\n> \t * We combine the platform and user transform, but must \"adjust away\"\n> @@ -1080,7 +1080,7 @@ Transform CameraSensor::validateTransform(Transform *transform) const\n> \t\t * If the sensor can do no transforms, then combined must be\n> \t\t * changed to the identity. The only user transform that gives\n> \t\t * rise to this is the inverse of the rotation. (Recall that\n> -\t\t * combined = transform * rotationTransform.)\n> +\t\t * combined = rotationTransform * transform.)\n> \t\t */\n> \t\t*transform = -rotationTransform_;\n> \t\tcombined = Transform::Identity;\n> diff --git a/src/libcamera/transform.cpp b/src/libcamera/transform.cpp\n> index c70cac3f14ee..d192d63d9290 100644\n> --- a/src/libcamera/transform.cpp\n> +++ b/src/libcamera/transform.cpp\n> @@ -189,14 +189,17 @@ Input image | | goes to output image | |\n> */\n> \n> /**\n> - * \\brief Compose two transforms together\n> - * \\param[in] t1 The second transform\n> - * \\param[in] t0 The first transform\n> + * \\brief Compose two transforms by applying \\a t0 first then \\a t1\n> + * \\param[in] t0 The first transform to apply\n> + * \\param[in] t1 The second transform to apply\n> + *\n> + * Compose two transforms by applying \\a t1 after \\a t0. The operation\n> + * is conceptually equivalent to the canonical notion of function composition,\n> + * with inverse order of operands. If in the canonical function composition\n> + * notation \"f * g\" equals to \"f(g())\", the notation for Transforms composition\n> + * \"t0 * t1\" equals to \"t1(t0()))\" where \\a t0 is applied first, then \\a t1.\n> *\n> - * Composing transforms follows the usual mathematical convention for\n> - * composing functions. That is, when performing `t1 * t0`, \\a t0 is applied\n> - * first, and then \\a t1.\n> - * For example, `Transpose * HFlip` performs `HFlip` first and then the\n> + * For example, `HFlip * Transpose` performs `HFlip` first and then the\n> * `Transpose` yielding `Rot270`, as shown below.\n> ~~~\n> A-B B-A B-D\n> @@ -206,7 +209,7 @@ Input image | | -> HFLip -> | | -> Transpose -> | | = Rot270\n> * Note that composition is generally non-commutative for Transforms,\n> * and not the same as XOR-ing the underlying bit representations.\n> */\n> -Transform operator*(Transform t1, Transform t0)\n> +Transform operator*(Transform t0, Transform t1)\n> {\n> \t/*\n> \t * Reorder the operations so that we imagine doing t0's transpose","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 AC9F2C3272\n\tfor ;\n\tWed, 18 Oct 2023 19:51:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E80B96296F;\n\tWed, 18 Oct 2023 21:51:08 +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 5BB7A61DD1\n\tfor ;\n\tWed, 18 Oct 2023 21:51:07 +0200 (CEST)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9A8CF666;\n\tWed, 18 Oct 2023 21:50:59 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1697658668;\n\tbh=A50ilSFMoIPEsgsrdnp5Hn6X08UAMWWJyPYiPl38nb0=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=kJ1I7ac4TsfshA3TrcYlyKe/0obZZChYTNpcYjwaqabszb8cIIs8cjaXGUT5StyTj\n\t2Dkcao4t7/k7RE7900jNz7Q1/AdTMOugommUKyN19f2FxH0R37U3OXVD2w8w+3fjBC\n\teT6onDu2fZZmPTtMIhZ8PRL4xGH4O1xihlpr+J9QBfm4Drf9AtBMGmLbU1mxojhxTg\n\t5xiC8N+tzrqdKxJYq4+2tz5Zn6YrnD9tXcJ7TujTDS1ZEEFJsbOwY184rCaveoixEm\n\tMSB5Bjq7nE5qb6N/iDUSu+kGFvJ9i/OK/YERD6EzxSpHCKlGRWdfDCUnEtgOQYRNtd\n\tKLInFDjmxT0KA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1697658659;\n\tbh=A50ilSFMoIPEsgsrdnp5Hn6X08UAMWWJyPYiPl38nb0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cXNfbnG5iQThiHgFDi6fuS5RC0jLNg10ELqrBJ8g1WmF6gB6kwxZLKe9BSuukwkR9\n\tGRDcDwolvHxqkZvkyucqoQYdlBHMHK31h63a1ijeudnvZyYMxEuz5yL7lSrOPI74Kl\n\t1rT9+N/DrxZ7UE+/ZzGpHeD0mHywbpSDXdF88IuM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"cXNfbnG5\"; dkim-atps=neutral","Date":"Wed, 18 Oct 2023 22:51:13 +0300","To":"Jacopo Mondi ","Message-ID":"<20231018195113.GH1512@pendragon.ideasonboard.com>","References":"<20230901150215.11585-1-jacopo.mondi@ideasonboard.com>\n\t<20230901150215.11585-7-jacopo.mondi@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230901150215.11585-7-jacopo.mondi@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5 06/12] libcamera: transform: Invert\n\toperator*() operands","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27994,"web_url":"https://patchwork.libcamera.org/comment/27994/","msgid":"","date":"2023-10-19T08:31:24","subject":"Re: [libcamera-devel] [PATCH v5 06/12] libcamera: transform: Invert\n\toperator*() operands","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"On Wed, Oct 18, 2023 at 10:51:13PM +0300, Laurent Pinchart via libcamera-devel wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Fri, Sep 01, 2023 at 05:02:09PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > The current definition of operator*(Transform t1, Transform t0) follows\n> > the function composition notion, where t0 is applied first then t1 is\n> > applied last.\n> >\n> > In order to introduce operator*(Orientation, Transform) where a\n> > Transform is applied on top of an Orientation, invert the operand order\n> > of operator*(Transform, Transform) so that usage of operator* with both\n> > Orientation and Transform can be made associative.\n> >\n> > For example:\n> >\n> > Orientation o;\n> > Transform t = t1 * t2\n> > Orientation o1 = o * t\n> > \t = o * (t1 * t2) = (o * t1) * t2 = o * t1 * t2\n>\n> So, if we want to follow mathematical conventions, we shouldn't write\n>\n> \tOrientation o1 = o * t;\n>\n> but\n>\n> \tOrientation o1 = t(o);\n>\n\nWhy ?\n\nWe're defining operators, the idea to follow the function composition\nnotion was in Transform but was an arbitrary pick.\n\nIf we assign to operator*(t1, t2) the semantic \"apply t1 and then t2\non top\" I think it's equally valid\n\n> which can be done by implementing\n>\n> \tOrientation Transform::operator(const Orientation *orientation);\n>\n> This would lead to\n>\n> \tt2(t1(o)) == (t2 * t1)(o)\n>\n> Would that be a better alternative ?\n>\n\nThis has been agreed with David in August, which has implemented\nsupport in libcamera apps for this notion. If we both have to change\nthis I would like a more detailed reason.\n\n> > Signed-off-by: Jacopo Mondi \n> > Reviewed-by: David Plowman \n> > ---\n> > src/libcamera/camera_sensor.cpp | 4 ++--\n> > src/libcamera/transform.cpp | 19 +++++++++++--------\n> > 2 files changed, 13 insertions(+), 10 deletions(-)\n> >\n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index 3ba364c44a40..038d8b959072 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -1051,7 +1051,7 @@ Transform CameraSensor::validateTransform(Transform *transform) const\n> > \t * Combine the requested transform to compensate the sensor mounting\n> > \t * rotation.\n> > \t */\n> > -\tTransform combined = *transform * rotationTransform_;\n> > +\tTransform combined = rotationTransform_ * *transform;\n> >\n> > \t/*\n> > \t * We combine the platform and user transform, but must \"adjust away\"\n> > @@ -1080,7 +1080,7 @@ Transform CameraSensor::validateTransform(Transform *transform) const\n> > \t\t * If the sensor can do no transforms, then combined must be\n> > \t\t * changed to the identity. The only user transform that gives\n> > \t\t * rise to this is the inverse of the rotation. (Recall that\n> > -\t\t * combined = transform * rotationTransform.)\n> > +\t\t * combined = rotationTransform * transform.)\n> > \t\t */\n> > \t\t*transform = -rotationTransform_;\n> > \t\tcombined = Transform::Identity;\n> > diff --git a/src/libcamera/transform.cpp b/src/libcamera/transform.cpp\n> > index c70cac3f14ee..d192d63d9290 100644\n> > --- a/src/libcamera/transform.cpp\n> > +++ b/src/libcamera/transform.cpp\n> > @@ -189,14 +189,17 @@ Input image | | goes to output image | |\n> > */\n> >\n> > /**\n> > - * \\brief Compose two transforms together\n> > - * \\param[in] t1 The second transform\n> > - * \\param[in] t0 The first transform\n> > + * \\brief Compose two transforms by applying \\a t0 first then \\a t1\n> > + * \\param[in] t0 The first transform to apply\n> > + * \\param[in] t1 The second transform to apply\n> > + *\n> > + * Compose two transforms by applying \\a t1 after \\a t0. The operation\n> > + * is conceptually equivalent to the canonical notion of function composition,\n> > + * with inverse order of operands. If in the canonical function composition\n> > + * notation \"f * g\" equals to \"f(g())\", the notation for Transforms composition\n> > + * \"t0 * t1\" equals to \"t1(t0()))\" where \\a t0 is applied first, then \\a t1.\n> > *\n> > - * Composing transforms follows the usual mathematical convention for\n> > - * composing functions. That is, when performing `t1 * t0`, \\a t0 is applied\n> > - * first, and then \\a t1.\n> > - * For example, `Transpose * HFlip` performs `HFlip` first and then the\n> > + * For example, `HFlip * Transpose` performs `HFlip` first and then the\n> > * `Transpose` yielding `Rot270`, as shown below.\n> > ~~~\n> > A-B B-A B-D\n> > @@ -206,7 +209,7 @@ Input image | | -> HFLip -> | | -> Transpose -> | | = Rot270\n> > * Note that composition is generally non-commutative for Transforms,\n> > * and not the same as XOR-ing the underlying bit representations.\n> > */\n> > -Transform operator*(Transform t1, Transform t0)\n> > +Transform operator*(Transform t0, Transform t1)\n> > {\n> > \t/*\n> > \t * Reorder the operations so that we imagine doing t0's transpose\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 3B411C3272\n\tfor ;\n\tThu, 19 Oct 2023 08:31:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A3DFD6297C;\n\tThu, 19 Oct 2023 10:31:28 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1726F6297B\n\tfor ;\n\tThu, 19 Oct 2023 10:31:27 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 38A8325A;\n\tThu, 19 Oct 2023 10:31:19 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1697704288;\n\tbh=SxBlGO0okbGaH/RIAypvo/p5wc1nhDDqNUXzReO5SeQ=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=FN+aWTT7RC9RzVPvOpNdMWzfx363SuF12QH4n9gMOQcgmPYMIQ/yV0wFXGJHWdBbS\n\tPczYn81OLlX/Rhhj7E48M7Cgy/y+LwIZYttL5cjIT6LHUFojofERnvzhTx4NXttm+7\n\t55y/9FXz4r9St9y+Hds5rQ8IPGFPGaYpkfpDZW/8oP1rjQ1Gu1wYF9ayl4v1kTAzql\n\tojwa2YNrkSU4/LO+bFHCI0of/d3gp4rPA2k4FDkO7WvrpjyxtFyNcbQzaaJ9XqnN1Z\n\twtGWKej/2hUdS1sF2sCWpsTVgEN9WYHBJt+HIiyfnji9DTTG9Tt4mSOlR9k+kKV6kb\n\th3vTiCm5sUagg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1697704279;\n\tbh=SxBlGO0okbGaH/RIAypvo/p5wc1nhDDqNUXzReO5SeQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=V3EcNb/RQ5W36DDgTmPLOFGjs0uYGTKKMDisDBEPtcdELUckRkQmP7VyHAsyCC09M\n\tDgVcBuNXb3MMp5FtmXhf4iguXcDEI2QLlr7rsCp6uoXJISUsBoHwez7sPKMBaJOl6S\n\tJdatmgcU6+uYNEVWbUaxt1fYPN4LPPGufJ6dwouc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"V3EcNb/R\"; dkim-atps=neutral","Date":"Thu, 19 Oct 2023 10:31:24 +0200","To":"Laurent Pinchart ","Message-ID":"","References":"<20230901150215.11585-1-jacopo.mondi@ideasonboard.com>\n\t<20230901150215.11585-7-jacopo.mondi@ideasonboard.com>\n\t<20231018195113.GH1512@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20231018195113.GH1512@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5 06/12] libcamera: transform: Invert\n\toperator*() operands","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28003,"web_url":"https://patchwork.libcamera.org/comment/28003/","msgid":"<20231019132921.GH14832@pendragon.ideasonboard.com>","date":"2023-10-19T13:29:21","subject":"Re: [libcamera-devel] [PATCH v5 06/12] libcamera: transform: Invert\n\toperator*() operands","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nCC'ing David for a question below.\n\nOn Thu, Oct 19, 2023 at 10:31:24AM +0200, Jacopo Mondi wrote:\n> On Wed, Oct 18, 2023 at 10:51:13PM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > On Fri, Sep 01, 2023 at 05:02:09PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > > The current definition of operator*(Transform t1, Transform t0) follows\n> > > the function composition notion, where t0 is applied first then t1 is\n> > > applied last.\n> > >\n> > > In order to introduce operator*(Orientation, Transform) where a\n> > > Transform is applied on top of an Orientation, invert the operand order\n> > > of operator*(Transform, Transform) so that usage of operator* with both\n> > > Orientation and Transform can be made associative.\n> > >\n> > > For example:\n> > >\n> > > Orientation o;\n> > > Transform t = t1 * t2\n> > > Orientation o1 = o * t\n> > > \t = o * (t1 * t2) = (o * t1) * t2 = o * t1 * t2\n> >\n> > So, if we want to follow mathematical conventions, we shouldn't write\n> >\n> > \tOrientation o1 = o * t;\n> >\n> > but\n> >\n> > \tOrientation o1 = t(o);\n> \n> Why ?\n> \n> We're defining operators, the idea to follow the function composition\n> notion was in Transform but was an arbitrary pick.\n> \n> If we assign to operator*(t1, t2) the semantic \"apply t1 and then t2\n> on top\" I think it's equally valid\n>\n> > which can be done by implementing\n> >\n> > \tOrientation Transform::operator(const Orientation *orientation);\n> >\n> > This would lead to\n> >\n> > \tt2(t1(o)) == (t2 * t1)(o)\n> >\n> > Would that be a better alternative ?\n> \n> This has been agreed with David in August, which has implemented\n> support in libcamera apps for this notion. If we both have to change\n> this I would like a more detailed reason.\n\nWhat I mean is that we've followed the mathematical concepts of\ncomposing functions. Transforms are functions, they're composed with the\nāˆ˜ operator, which we replaced with operator*() as C++ doesn't allow\ncustom UTF-8 operators (I could image this being useful, but also\nsoooooo probe to abuse). If we follow this logic, using the call\noperator to apply transforms would be quite natural:\n\n\tOrientation o = Transform::HFlip(Orientation::Rot180);\n\nSure, we can use our own conventions instead, but I think that's more\nerror prone than using established convensions, as users will have to\nlearn and change their frame of mind.\n\nI don't recall how this was decided with David in August (I may not have\nfollowed the discussion, or just forgotten about it, in either case,\nsorry about that). David, was there or is there a particular reason why\ndeparting from the usual function composition order would work better\nfor you ?\n\n> > > Signed-off-by: Jacopo Mondi \n> > > Reviewed-by: David Plowman \n> > > ---\n> > > src/libcamera/camera_sensor.cpp | 4 ++--\n> > > src/libcamera/transform.cpp | 19 +++++++++++--------\n> > > 2 files changed, 13 insertions(+), 10 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > index 3ba364c44a40..038d8b959072 100644\n> > > --- a/src/libcamera/camera_sensor.cpp\n> > > +++ b/src/libcamera/camera_sensor.cpp\n> > > @@ -1051,7 +1051,7 @@ Transform CameraSensor::validateTransform(Transform *transform) const\n> > > \t * Combine the requested transform to compensate the sensor mounting\n> > > \t * rotation.\n> > > \t */\n> > > -\tTransform combined = *transform * rotationTransform_;\n> > > +\tTransform combined = rotationTransform_ * *transform;\n> > >\n> > > \t/*\n> > > \t * We combine the platform and user transform, but must \"adjust away\"\n> > > @@ -1080,7 +1080,7 @@ Transform CameraSensor::validateTransform(Transform *transform) const\n> > > \t\t * If the sensor can do no transforms, then combined must be\n> > > \t\t * changed to the identity. The only user transform that gives\n> > > \t\t * rise to this is the inverse of the rotation. (Recall that\n> > > -\t\t * combined = transform * rotationTransform.)\n> > > +\t\t * combined = rotationTransform * transform.)\n> > > \t\t */\n> > > \t\t*transform = -rotationTransform_;\n> > > \t\tcombined = Transform::Identity;\n> > > diff --git a/src/libcamera/transform.cpp b/src/libcamera/transform.cpp\n> > > index c70cac3f14ee..d192d63d9290 100644\n> > > --- a/src/libcamera/transform.cpp\n> > > +++ b/src/libcamera/transform.cpp\n> > > @@ -189,14 +189,17 @@ Input image | | goes to output image | |\n> > > */\n> > >\n> > > /**\n> > > - * \\brief Compose two transforms together\n> > > - * \\param[in] t1 The second transform\n> > > - * \\param[in] t0 The first transform\n> > > + * \\brief Compose two transforms by applying \\a t0 first then \\a t1\n> > > + * \\param[in] t0 The first transform to apply\n> > > + * \\param[in] t1 The second transform to apply\n> > > + *\n> > > + * Compose two transforms by applying \\a t1 after \\a t0. The operation\n> > > + * is conceptually equivalent to the canonical notion of function composition,\n> > > + * with inverse order of operands. If in the canonical function composition\n> > > + * notation \"f * g\" equals to \"f(g())\", the notation for Transforms composition\n> > > + * \"t0 * t1\" equals to \"t1(t0()))\" where \\a t0 is applied first, then \\a t1.\n> > > *\n> > > - * Composing transforms follows the usual mathematical convention for\n> > > - * composing functions. That is, when performing `t1 * t0`, \\a t0 is applied\n> > > - * first, and then \\a t1.\n> > > - * For example, `Transpose * HFlip` performs `HFlip` first and then the\n> > > + * For example, `HFlip * Transpose` performs `HFlip` first and then the\n> > > * `Transpose` yielding `Rot270`, as shown below.\n> > > ~~~\n> > > A-B B-A B-D\n> > > @@ -206,7 +209,7 @@ Input image | | -> HFLip -> | | -> Transpose -> | | = Rot270\n> > > * Note that composition is generally non-commutative for Transforms,\n> > > * and not the same as XOR-ing the underlying bit representations.\n> > > */\n> > > -Transform operator*(Transform t1, Transform t0)\n> > > +Transform operator*(Transform t0, Transform t1)\n> > > {\n> > > \t/*\n> > > \t * Reorder the operations so that we imagine doing t0's transpose","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 7150CC3272\n\tfor ;\n\tThu, 19 Oct 2023 13:29:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C82BB6297F;\n\tThu, 19 Oct 2023 15:29:15 +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 8B9886055B\n\tfor ;\n\tThu, 19 Oct 2023 15:29:14 +0200 (CEST)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5533525A;\n\tThu, 19 Oct 2023 15:29:06 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1697722155;\n\tbh=FMaW+4X+/2LXMEY3BXWneC1oeCwktT6Iskh44q2jECk=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Eoy+ik3msw/HooH0u2TC7C5cm6nAsEJHnDJqXaCzk69YQ5nvgN3InHtYv2t+yR4rm\n\t+deuenA4tI9oQAqBvk3tWZ/Ok4+6DEHYdfvWxowZ+kCAXCDIdwONF5/0+H3rg0Bwhl\n\tQNCLJLObNwXAdi2yoxIF8duhVFVCNRxf/a+hB5bCnloG+beqS3rYQgkib7SVXdJqsR\n\tvE3cBNYWp3qKbwBWSmEH74mRdpjDNLqctx5MqxcMf3OR0BNg5DyPrtLyl3SqXKC5ja\n\tKiginp5QN8VqjDRabLBkHFrDNOIDCRg6AWQ3hBJ8AsDXewWRvk0n5s0a9NxnJ2pUs3\n\tM7DnX7/axac0g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1697722146;\n\tbh=FMaW+4X+/2LXMEY3BXWneC1oeCwktT6Iskh44q2jECk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=veyev0rlZP3TVdKZeSXTpSHYBb4Fc6icZwNqYbBRAQXYHSDrpPXoJJkMj2xJ640aM\n\t64Z/2inoPEiEg/vRyirp/hWSvQsQXiZuhS9rFyJaikjWzA0V+JsIgBRXyDLdfQZyq8\n\tFAY0f6cXaalncbAojYf0M8cCeGXVBbYPVwM4aMVk="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"veyev0rl\"; dkim-atps=neutral","Date":"Thu, 19 Oct 2023 16:29:21 +0300","To":"Jacopo Mondi ","Message-ID":"<20231019132921.GH14832@pendragon.ideasonboard.com>","References":"<20230901150215.11585-1-jacopo.mondi@ideasonboard.com>\n\t<20230901150215.11585-7-jacopo.mondi@ideasonboard.com>\n\t<20231018195113.GH1512@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v5 06/12] libcamera: transform: Invert\n\toperator*() operands","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28007,"web_url":"https://patchwork.libcamera.org/comment/28007/","msgid":"","date":"2023-10-19T14:00:04","subject":"Re: [libcamera-devel] [PATCH v5 06/12] libcamera: transform: Invert\n\toperator*() operands","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi everyone\n\nYes, this seems like a long time ago!\n\nOn Thu, 19 Oct 2023 at 14:29, Laurent Pinchart\n wrote:\n>\n> Hi Jacopo,\n>\n> CC'ing David for a question below.\n>\n> On Thu, Oct 19, 2023 at 10:31:24AM +0200, Jacopo Mondi wrote:\n> > On Wed, Oct 18, 2023 at 10:51:13PM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > > On Fri, Sep 01, 2023 at 05:02:09PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > > > The current definition of operator*(Transform t1, Transform t0) follows\n> > > > the function composition notion, where t0 is applied first then t1 is\n> > > > applied last.\n> > > >\n> > > > In order to introduce operator*(Orientation, Transform) where a\n> > > > Transform is applied on top of an Orientation, invert the operand order\n> > > > of operator*(Transform, Transform) so that usage of operator* with both\n> > > > Orientation and Transform can be made associative.\n> > > >\n> > > > For example:\n> > > >\n> > > > Orientation o;\n> > > > Transform t = t1 * t2\n> > > > Orientation o1 = o * t\n> > > > = o * (t1 * t2) = (o * t1) * t2 = o * t1 * t2\n> > >\n> > > So, if we want to follow mathematical conventions, we shouldn't write\n> > >\n> > > Orientation o1 = o * t;\n> > >\n> > > but\n> > >\n> > > Orientation o1 = t(o);\n> >\n> > Why ?\n> >\n> > We're defining operators, the idea to follow the function composition\n> > notion was in Transform but was an arbitrary pick.\n> >\n> > If we assign to operator*(t1, t2) the semantic \"apply t1 and then t2\n> > on top\" I think it's equally valid\n> >\n> > > which can be done by implementing\n> > >\n> > > Orientation Transform::operator(const Orientation *orientation);\n> > >\n> > > This would lead to\n> > >\n> > > t2(t1(o)) == (t2 * t1)(o)\n> > >\n> > > Would that be a better alternative ?\n> >\n> > This has been agreed with David in August, which has implemented\n> > support in libcamera apps for this notion. If we both have to change\n> > this I would like a more detailed reason.\n>\n> What I mean is that we've followed the mathematical concepts of\n> composing functions. Transforms are functions, they're composed with the\n> āˆ˜ operator, which we replaced with operator*() as C++ doesn't allow\n> custom UTF-8 operators (I could image this being useful, but also\n> soooooo probe to abuse). If we follow this logic, using the call\n> operator to apply transforms would be quite natural:\n>\n> Orientation o = Transform::HFlip(Orientation::Rot180);\n>\n> Sure, we can use our own conventions instead, but I think that's more\n> error prone than using established convensions, as users will have to\n> learn and change their frame of mind.\n>\n> I don't recall how this was decided with David in August (I may not have\n> followed the discussion, or just forgotten about it, in either case,\n> sorry about that). David, was there or is there a particular reason why\n> departing from the usual function composition order would work better\n> for you ?\n\nMy recollection here was simply that it all felt less confusing when\nwe used the left-to-right convention. You can add, for example, \" * t1\n\" to the end of an existing expression and t1 gets applied last.\nHaving to remember to put it at the front, when it's happening last,\njust seemed to involve more cognitive effort than I would have\nexpected. When we switched, I could easily write stuff down and have\nthe instant \"that's obviously correct\" feeling.\n\nBut I agree... that's just how I felt. Whatever is less bug-prone for\nthe majority is probably the best answer.\n\nDavid\n\n>\n> > > > Signed-off-by: Jacopo Mondi \n> > > > Reviewed-by: David Plowman \n> > > > ---\n> > > > src/libcamera/camera_sensor.cpp | 4 ++--\n> > > > src/libcamera/transform.cpp | 19 +++++++++++--------\n> > > > 2 files changed, 13 insertions(+), 10 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > > index 3ba364c44a40..038d8b959072 100644\n> > > > --- a/src/libcamera/camera_sensor.cpp\n> > > > +++ b/src/libcamera/camera_sensor.cpp\n> > > > @@ -1051,7 +1051,7 @@ Transform CameraSensor::validateTransform(Transform *transform) const\n> > > > * Combine the requested transform to compensate the sensor mounting\n> > > > * rotation.\n> > > > */\n> > > > - Transform combined = *transform * rotationTransform_;\n> > > > + Transform combined = rotationTransform_ * *transform;\n> > > >\n> > > > /*\n> > > > * We combine the platform and user transform, but must \"adjust away\"\n> > > > @@ -1080,7 +1080,7 @@ Transform CameraSensor::validateTransform(Transform *transform) const\n> > > > * If the sensor can do no transforms, then combined must be\n> > > > * changed to the identity. The only user transform that gives\n> > > > * rise to this is the inverse of the rotation. (Recall that\n> > > > - * combined = transform * rotationTransform.)\n> > > > + * combined = rotationTransform * transform.)\n> > > > */\n> > > > *transform = -rotationTransform_;\n> > > > combined = Transform::Identity;\n> > > > diff --git a/src/libcamera/transform.cpp b/src/libcamera/transform.cpp\n> > > > index c70cac3f14ee..d192d63d9290 100644\n> > > > --- a/src/libcamera/transform.cpp\n> > > > +++ b/src/libcamera/transform.cpp\n> > > > @@ -189,14 +189,17 @@ Input image | | goes to output image | |\n> > > > */\n> > > >\n> > > > /**\n> > > > - * \\brief Compose two transforms together\n> > > > - * \\param[in] t1 The second transform\n> > > > - * \\param[in] t0 The first transform\n> > > > + * \\brief Compose two transforms by applying \\a t0 first then \\a t1\n> > > > + * \\param[in] t0 The first transform to apply\n> > > > + * \\param[in] t1 The second transform to apply\n> > > > + *\n> > > > + * Compose two transforms by applying \\a t1 after \\a t0. The operation\n> > > > + * is conceptually equivalent to the canonical notion of function composition,\n> > > > + * with inverse order of operands. If in the canonical function composition\n> > > > + * notation \"f * g\" equals to \"f(g())\", the notation for Transforms composition\n> > > > + * \"t0 * t1\" equals to \"t1(t0()))\" where \\a t0 is applied first, then \\a t1.\n> > > > *\n> > > > - * Composing transforms follows the usual mathematical convention for\n> > > > - * composing functions. That is, when performing `t1 * t0`, \\a t0 is applied\n> > > > - * first, and then \\a t1.\n> > > > - * For example, `Transpose * HFlip` performs `HFlip` first and then the\n> > > > + * For example, `HFlip * Transpose` performs `HFlip` first and then the\n> > > > * `Transpose` yielding `Rot270`, as shown below.\n> > > > ~~~\n> > > > A-B B-A B-D\n> > > > @@ -206,7 +209,7 @@ Input image | | -> HFLip -> | | -> Transpose -> | | = Rot270\n> > > > * Note that composition is generally non-commutative for Transforms,\n> > > > * and not the same as XOR-ing the underlying bit representations.\n> > > > */\n> > > > -Transform operator*(Transform t1, Transform t0)\n> > > > +Transform operator*(Transform t0, Transform t1)\n> > > > {\n> > > > /*\n> > > > * Reorder the operations so that we imagine doing t0's transpose\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 1B1B2C3272\n\tfor ;\n\tThu, 19 Oct 2023 14:00:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 660A262980;\n\tThu, 19 Oct 2023 16:00:18 +0200 (CEST)","from mail-qv1-xf36.google.com (mail-qv1-xf36.google.com\n\t[IPv6:2607:f8b0:4864:20::f36])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 753E361DD2\n\tfor ;\n\tThu, 19 Oct 2023 16:00:16 +0200 (CEST)","by mail-qv1-xf36.google.com with SMTP id\n\t6a1803df08f44-66d03491a1eso43694956d6.2\n\tfor ;\n\tThu, 19 Oct 2023 07:00:16 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1697724018;\n\tbh=eUJoY0Cn/fdIQeSpgPLIllkA4dhUdJUtHZazqn0Vyzs=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=j4hhDNszL3OKJHwP3Sp4s61dk9bmjjdgdgbGvJjPcsTtkwEH8QlWvVcLtDG8nywqK\n\tZGyVGPjS9hMjbKneOF2EA3ZfHtRrj/o9kgw26nyWr0FtaK5tdYVs8b/TuCu8t6GkPk\n\t4v6nz2ojPaLBG6E+Irmp0G83uTJCfIVPwzx4c24tm+pvdDJt4hsgdmpFJPr/jXNdaO\n\tt9UnUqsh+/Jfs4u6UnQU9o2zIBDoIp275obR101omRu74PmEr8HsvCCJf6UQCDDQqX\n\tZojD8psng0JQ/W4RyyHsbEJDsxjFddYUJQ/Bi0kMl+CYzB/ZaoiCJWG4OvOP+uqMoy\n\tVoxYjAOf5ADKg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1697724015; x=1698328815;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=cvOe3h0t1qjhCWuAEOM6Lw28C7qQS9E/9T71SBC71ZA=;\n\tb=Vm0BA3x7hqBt4P5Z74HgDYaNe7DeznbhM8nJ95jBNoSy2pmnFkNPRiBvVDkMLrpQPH\n\tFejADeyihYnzQtjXJNLDgxckoieJu5A5TWa9DNnhkljNDK9avuOG03awMcrNW97PJSkk\n\tfNqWN2STZAJ2NNw6owg7z6CcbcqA0UHROGiA8x605OgPUY5W8wwcXcE/P5hnceiSOLyd\n\tIuxAqQW+vWZFYCiU5wfmCYfoLs0HY7LeTb9TedOAhMZQEHx2mz9P7qcxUkLCP8/0Iy26\n\t0LqQ5udSy0izAnbT63tRlye2ul5/qCXKDQcqTxOcewPnu8GiogTM6s+co4cWLt1cdm4m\n\t8xLw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"Vm0BA3x7\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1697724015; x=1698328815;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=cvOe3h0t1qjhCWuAEOM6Lw28C7qQS9E/9T71SBC71ZA=;\n\tb=iVhcbx5JgTnmTkG/8suBKzS/aADYDhqSkG3GUb+4eDfUkXOK0p0UVgC4/CwWyqNrf4\n\tT9r9Hz+6T92tpPjtyW0Zj4NhvGU6bcgcduIKUZBT6Lv9NLa/Z0JjKchhAvIMqGx6w3xM\n\tlQaIH60hKkt8Nj4dB+OX7/gds4VO6J+ItH8YEKHZZ+TztsIphY3peMKPpkV/2ftca8Q0\n\tP1LMOKpjCt1Qh1WlrGln5GZrORZxi0DFiebvQc0b5tEFrfCqt05OKG36/U77kvmnNbVQ\n\t7LAeDE1bxLgmGm+GNObSBQO1OZ5TAhk5nVO4VjTgsQy+OQv85EsH/x8EdnulrUE9eLVH\n\t2RQw==","X-Gm-Message-State":"AOJu0YzxYXFfRQN++393bX3b7VPQnFH8pZeISZArK5VC91chazjl7iwp\n\tJxrx8omnc6tReB4gOFpqcihrOlTHpphDi8ZsGHEgi2zLh3dEMwQG","X-Google-Smtp-Source":"AGHT+IGHYafS5TTJV8zRgGtAmzmpHigYv7RUolg7iKnjIUJffJWHY7JEyWxIjdrWsicdERMiI7l76sWXvMf3TBtOySg=","X-Received":"by 2002:a05:6214:2241:b0:668:ecf4:d9b7 with SMTP id\n\tc1-20020a056214224100b00668ecf4d9b7mr2919782qvc.8.1697724015301;\n\tThu, 19 Oct 2023 07:00:15 -0700 (PDT)","MIME-Version":"1.0","References":"<20230901150215.11585-1-jacopo.mondi@ideasonboard.com>\n\t<20230901150215.11585-7-jacopo.mondi@ideasonboard.com>\n\t<20231018195113.GH1512@pendragon.ideasonboard.com>\n\t\n\t<20231019132921.GH14832@pendragon.ideasonboard.com>","In-Reply-To":"<20231019132921.GH14832@pendragon.ideasonboard.com>","Date":"Thu, 19 Oct 2023 15:00:04 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH v5 06/12] libcamera: transform: Invert\n\toperator*() operands","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","From":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]