[{"id":27558,"web_url":"https://patchwork.libcamera.org/comment/27558/","msgid":"","date":"2023-07-17T10:50:16","subject":"Re: [libcamera-devel] [PATCH v2 6/9] libcamera: transform: Add\n\toperations with Orientation","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nThanks for the patch!\n\nOn Fri, 14 Jul 2023 at 15:16, Jacopo Mondi\n wrote:\n>\n> Add two operations that allows to combine Transform with Orientation.\n\nI see that the English version is a bit awkward here. Maybe s/allows/allow us/?\n\n>\n> - Transform operator/(Orientation1, Orientation2)\n> allows to easily get back the Transform that needs to be applied to\n> Orientation2 to get Orientation1\n>\n> - Orientation operator*(Orientation1, Transform)\n> allows to apply a Transform to an Orientation and obtain back the\n\ns/back//? The \"back\" seems marginally unnecessary to me.\n\n> combination of the two\n>\n> The two operations allow application that are willing to use Transform\n> to operate with the Orientation part of CameraConfiguration.\n\nNot sure about the wording, \"are willing\" sounds a bit like we're\ntrying to force them, or something. Maybe\n\n\"These two operations allow applications to use Transforms to\nmanipulate the Orientation inside the CameraConfiguration, if they\nwish.\"\n\nBut please adjust it to something you're comfortable with!\n\n>\n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/transform.h | 5 ++\n> src/libcamera/transform.cpp | 98 +++++++++++++++++++++++++++++++++++\n> 2 files changed, 103 insertions(+)\n>\n> diff --git a/include/libcamera/transform.h b/include/libcamera/transform.h\n> index 573adf18715d..a7470c70a755 100644\n> --- a/include/libcamera/transform.h\n> +++ b/include/libcamera/transform.h\n> @@ -74,6 +74,11 @@ Transform transformFromRotation(int angle, bool *success = nullptr);\n> Transform transformFromOrientation(const CameraConfiguration::Orientation &orientation);\n> CameraConfiguration::Orientation transformToOrientation(const Transform &transform);\n>\n> +Transform operator/(CameraConfiguration::Orientation o1,\n> + CameraConfiguration::Orientation o2);\n> +CameraConfiguration::Orientation operator*(CameraConfiguration::Orientation o1,\n> + Transform t);\n> +\n> const char *transformToString(Transform t);\n>\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/transform.cpp b/src/libcamera/transform.cpp\n> index 03d2b52e7f38..61dbb1ff1dd5 100644\n> --- a/src/libcamera/transform.cpp\n> +++ b/src/libcamera/transform.cpp\n> @@ -357,6 +357,104 @@ CameraConfiguration::Orientation transformToOrientation(const Transform &transfo\n> return CameraConfiguration::rotate0;\n> }\n>\n> +/**\n> + * \\brief Return the Transform that applied to \\a o2 gives \\a o1\n> + * \\param o1 The Orientation to obtain\n> + * \\param o2 The base Orientation\n> + *\n> + * This operation can be used to easily compute the Transform to apply to a\n> + * base orientation \\a o2 to get the desired orientation \\a o1.\n> + *\n> + * The CameraSensor class uses this function to compute what Transform to apply\n> + * to a camera with mounting rotation \\a o2 (the base) to obtain the user\n> + * requested orientation \\a o1.\n> + *\n> + * \\return A Transform that applied to \\a o2 gives \\a o1\n> + */\n> +Transform operator/(CameraConfiguration::Orientation o1,\n> + CameraConfiguration::Orientation o2)\n> +{\n> + Transform t1 = transformFromOrientation(o1);\n> + Transform t2 = transformFromOrientation(o2);\n> +\n> + /* Return immediately if the two are identical. */\n> + if (t1 == t2)\n> + return Transform::Identity;\n> +\n> + /* If t1 is identity we need to apply -t2 to get it. */\n> + if (t1 == Transform::Identity)\n> + return -t2;\n> +\n> + /* If t2 is identity, to get t1 we still need to do... t1. */\n> + if (t2 == Transform::Identity)\n> + return t1;\n> +\n> + Transform combined = t1 ^ t2;\n> + Transform result = combined;\n> +\n> + /*\n> + * If the base orientation is transposed we need to invert the flips on\n> + * the combined result.\n> + *\n> + * Using Rot90 as an example:\n> + *\n> + * Rot180 / Rot90 = Rot90\n> + * = (H|V) ^ (T|V) = (T|H)\n> + * = invert_flips(T|H) = (T|V) = Rot90\n> + *\n> + * Rot0 / Rot90 = Rot270\n> + * = () ^ (T|V) = (T|V)\n> + * = invert_flips(T|V) = (T|H) = Rot270\n> + *\n> + * Rot270 / Rot90 = Rot180\n> + * = (T|H) ^ (T|V) = (H|V)\n> + * = invert_flips(H|V) = (V|H) = Rot180\n> + *\n> + * Rot180Transpose / Rot90 = V\n> + * = (T|H|V) ^ (T|V) = (H)\n> + * = invert_flips(H) = (V)\n> + *\n> + * Rot270 / Transpose = V\n> + * = (T|H) ^ T = H\n> + * = invert_flips(H) = (V)\n> + */\n> + if (!!(t2 & Transform::Transpose)) {\n> + result = combined & Transform::Transpose;\n> + if (!!(combined & Transform::HFlip))\n> + result |= Transform::VFlip;\n> + if (!!(combined & Transform::VFlip))\n> + result |= Transform::HFlip;\n> + }\n> +\n> + return result;\n> +}\n\nI was expecting something a bit simpler here, maybe\n\n Transform t1 = transformFromOrientation(o1);\n Transform t2 = transformFromOrientation(o2);\n\n return transformToOrientation(t1 * -t2);\n\nAnd it does have the benefit that you can glance at it and think\n\"yeah, it does what I expect\". And if it isn't as simple as that, then\nthere's probably something wrong!!\n\n> +\n> +/**\n> + * \\brief Apply the Transform \\a t on the base orientation \\a o\n> + * \\param o The base orientation\n> + * \\param t The transform to apply on \\a o\n> + * \\return The Orientation resulting from applying \\a t on \\a o\n> + */\n> +CameraConfiguration::Orientation operator*(CameraConfiguration::Orientation o,\n> + Transform t)\n> +{\n> + /* Apply an Indentity always gives us back the other operand. */\n> + if (t == Transform::Identity)\n> + return o;\n> +\n> + /*\n> + * As operator*(Transform t2, Transform t1) composes two Tranform\n> + * by applying t1 first then t2 according to the canonical function\n> + * composition notion, we here first apply a Transform corresponding to\n> + * the base orientation and the apply \\a t to it.\n> + */\n> + Transform t1 = transformFromOrientation(o);\n> + if (t1 == Transform::Identity)\n> + return transformToOrientation(t);\n> +\n> + return transformToOrientation(t * t1);\n\nI would probably have gone straight to the final \"return\" statement,\nwithout those other tests. I think it makes the whole function easier\nto read and understand, and I don't think there's a meaningful gain\nfrom the special cases.\n\nOn a semi-related note, I find myself second-guessing the order of\ntransform composition. The documentation is pretty clear that we're\nusing traditional mathematical composition rules, but does that still\nmake sense?\n\nWe defined operator*(orientation, transform) so that we have:\n\no1 * t = o2\n\nbut if t = t1 * t2 then (according to the current rules)\n\no2 * (t1 * t2) = o2 and therefore (o2 * t2) * t1 = o2 so in fact o2 *\nt2 * t1 = o2. Or in other words, these two flavours of * aren't\nassociative. It all feels wrong.\n\nHaving a prefix version, namely operator*(transform, orientation)\nwould fix this nastiness, but I do wonder whether it would be more\nnatural to change the transform order when we compose them. Opinions\nsought!!\n\nThanks\nDavid\n\n> +}\n> +\n> /**\n> * \\brief Return a character string describing the transform\n> * \\param[in] t The transform to be described.\n> --\n> 2.40.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CBB45BEFBE\n\tfor ;\n\tMon, 17 Jul 2023 10:50:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 323FE61E2C;\n\tMon, 17 Jul 2023 12:50:31 +0200 (CEST)","from mail-qv1-xf32.google.com (mail-qv1-xf32.google.com\n\t[IPv6:2607:f8b0:4864:20::f32])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0D79960383\n\tfor ;\n\tMon, 17 Jul 2023 12:50:29 +0200 (CEST)","by mail-qv1-xf32.google.com with SMTP id\n\t6a1803df08f44-635fa79d7c0so24110856d6.1\n\tfor ;\n\tMon, 17 Jul 2023 03:50:28 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1689591031;\n\tbh=vHlKZa69859lLNVVUK8mDLuweOqZSYpj5NqvIP2kqYQ=;\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=k0HB3IVevfzqCSXVZM+bicF1yAx39aAo0heSgcnYGAIhgPwTmCH5IyGgXraO2YUJe\n\tzjUKXTqdmYhih3NRMCJsBnj+333SfkF4KFPb75b/7uOJYg2n2C6eDDT2owQN35ULj+\n\tbpKSs0AaVR6xWvLYUd+iq1Q6gHdeiKcjqzHmvSUxQaU+eNBPiewEpW+JEenKy/KDY+\n\taCVI2kn7CtMmmyl9eWshlhoJHPo9s6k2fKGvolr48s3Aaf7HrfuDGIoAng5BPzrX7z\n\tuwofipSf/IvykBW7GNXmLsj1/kLmutOOoJ/DhxB4DMJO7VhbkIqY85R1yeBK39nPmm\n\tip9KMSgy53ipw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1689591028; x=1692183028;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=9EtqTp4hWVJaHbQwkiE+j90iCa/dtBPs5QB//lFjdG0=;\n\tb=Aq1YnZqHtOeOJ2zrzZWt6m6awLsJ/Lgk6/WBqDTggRjYMmG+ukIMyyS3bLUYau1R4+\n\tG6sV182LEcRRwBripov2Ri3iqZufCKpTppFmfiv4/+cZbanaq5ZUdHdnH2+2mwOSemq2\n\temxwKqYEdGymW41OCopasOZ07trrgbrdtaSSJbav1SXIL6sdXSv80jQ0YPPXYYY/n81K\n\tKT/xD30uYqwdMDIKUm4qeRa4iznm+uP1de5J4+BMt+AeetyWaI8NHpM7jKn+Ou0aFhqZ\n\tADzxwjK8mHAd2sW+v+q7uFbKbrDUvt6lgNTGlpY6HaTWhI9QeM8w1c4tjJDqJQg0+W15\n\tlEWw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"Aq1YnZqH\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1689591028; x=1692183028;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=9EtqTp4hWVJaHbQwkiE+j90iCa/dtBPs5QB//lFjdG0=;\n\tb=VXdphBRcPahtBl33mBljUqsfUIJFuamud3tlJjtDmqmOXmtvfk/oZ/vlbWsYlItkw4\n\tWFXV9nPH0T9mUeKGfLkDfnP59Hiz7D2s087wURqFnahBuJKGvW3qwJMS6IkaShcuzuec\n\t4/SXm/7lEGtSANoDvCdQeb7xmiH+jtrnZjnNEwItyKOmbAE7+mZEiwHZ4X13ELmo9KdY\n\tc0XnCjw24eWkgHoYhumiLWYt+5FYfo/Js0RdrfepwqELEabmeXpWRs6iukWoKdoy5UiS\n\tmXBEsh0//Ny1KUKyH5jrvxw8Hv7h8+FVtt1PbgQ3w7qtAshYCu6CR4Fid3hfRcbb7t5K\n\tp/ZA==","X-Gm-Message-State":"ABy/qLaYYn8aR24Q4ZLt9IbnKuSeSwJ8HwFa9mZgpAt7K2IoJE4RMfKN\n\t+knU3NvReQmCrEFiZQkMsm3YffeogKm/F4ivtIhalg==","X-Google-Smtp-Source":"APBJJlHmZKkl1II+7UT5HSeiGoJaa00U0Mv3PtsEYrfg+KF2wqvy2JPxRSBGkjeq1WoT7E6BngztFbNwSHZ8XV/00cY=","X-Received":"by 2002:ad4:4eee:0:b0:62b:3e24:cfd with SMTP id\n\tdv14-20020ad44eee000000b0062b3e240cfdmr11788314qvb.5.1689591027867;\n\tMon, 17 Jul 2023 03:50:27 -0700 (PDT)","MIME-Version":"1.0","References":"<20230714141549.11085-1-jacopo.mondi@ideasonboard.com>\n\t<20230714141549.11085-7-jacopo.mondi@ideasonboard.com>","In-Reply-To":"<20230714141549.11085-7-jacopo.mondi@ideasonboard.com>","Date":"Mon, 17 Jul 2023 11:50:16 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 6/9] libcamera: transform: Add\n\toperations with Orientation","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":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27562,"web_url":"https://patchwork.libcamera.org/comment/27562/","msgid":"","date":"2023-07-17T13:28:16","subject":"Re: [libcamera-devel] [PATCH v2 6/9] libcamera: transform: Add\n\toperations with Orientation","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi David\n\nOn Mon, Jul 17, 2023 at 11:50:16AM +0100, David Plowman via libcamera-devel wrote:\n> Hi Jacopo\n>\n> Thanks for the patch!\n>\n> On Fri, 14 Jul 2023 at 15:16, Jacopo Mondi\n> wrote:\n> >\n> > Add two operations that allows to combine Transform with Orientation.\n>\n> I see that the English version is a bit awkward here. Maybe s/allows/allow us/?\n>\n> >\n> > - Transform operator/(Orientation1, Orientation2)\n> > allows to easily get back the Transform that needs to be applied to\n> > Orientation2 to get Orientation1\n> >\n> > - Orientation operator*(Orientation1, Transform)\n> > allows to apply a Transform to an Orientation and obtain back the\n>\n> s/back//? The \"back\" seems marginally unnecessary to me.\n>\n> > combination of the two\n> >\n> > The two operations allow application that are willing to use Transform\n> > to operate with the Orientation part of CameraConfiguration.\n>\n> Not sure about the wording, \"are willing\" sounds a bit like we're\n> trying to force them, or something. Maybe\n>\n> \"These two operations allow applications to use Transforms to\n> manipulate the Orientation inside the CameraConfiguration, if they\n> wish.\"\n>\n> But please adjust it to something you're comfortable with!\n\nAh well, I'm comfortable with whatever makes the most sense to native\nspeakers :)\n\n>\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/transform.h | 5 ++\n> > src/libcamera/transform.cpp | 98 +++++++++++++++++++++++++++++++++++\n> > 2 files changed, 103 insertions(+)\n> >\n> > diff --git a/include/libcamera/transform.h b/include/libcamera/transform.h\n> > index 573adf18715d..a7470c70a755 100644\n> > --- a/include/libcamera/transform.h\n> > +++ b/include/libcamera/transform.h\n> > @@ -74,6 +74,11 @@ Transform transformFromRotation(int angle, bool *success = nullptr);\n> > Transform transformFromOrientation(const CameraConfiguration::Orientation &orientation);\n> > CameraConfiguration::Orientation transformToOrientation(const Transform &transform);\n> >\n> > +Transform operator/(CameraConfiguration::Orientation o1,\n> > + CameraConfiguration::Orientation o2);\n> > +CameraConfiguration::Orientation operator*(CameraConfiguration::Orientation o1,\n> > + Transform t);\n> > +\n> > const char *transformToString(Transform t);\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/transform.cpp b/src/libcamera/transform.cpp\n> > index 03d2b52e7f38..61dbb1ff1dd5 100644\n> > --- a/src/libcamera/transform.cpp\n> > +++ b/src/libcamera/transform.cpp\n> > @@ -357,6 +357,104 @@ CameraConfiguration::Orientation transformToOrientation(const Transform &transfo\n> > return CameraConfiguration::rotate0;\n> > }\n> >\n> > +/**\n> > + * \\brief Return the Transform that applied to \\a o2 gives \\a o1\n> > + * \\param o1 The Orientation to obtain\n> > + * \\param o2 The base Orientation\n> > + *\n> > + * This operation can be used to easily compute the Transform to apply to a\n> > + * base orientation \\a o2 to get the desired orientation \\a o1.\n> > + *\n> > + * The CameraSensor class uses this function to compute what Transform to apply\n> > + * to a camera with mounting rotation \\a o2 (the base) to obtain the user\n> > + * requested orientation \\a o1.\n> > + *\n> > + * \\return A Transform that applied to \\a o2 gives \\a o1\n> > + */\n> > +Transform operator/(CameraConfiguration::Orientation o1,\n> > + CameraConfiguration::Orientation o2)\n> > +{\n> > + Transform t1 = transformFromOrientation(o1);\n> > + Transform t2 = transformFromOrientation(o2);\n> > +\n> > + /* Return immediately if the two are identical. */\n> > + if (t1 == t2)\n> > + return Transform::Identity;\n> > +\n> > + /* If t1 is identity we need to apply -t2 to get it. */\n> > + if (t1 == Transform::Identity)\n> > + return -t2;\n> > +\n> > + /* If t2 is identity, to get t1 we still need to do... t1. */\n> > + if (t2 == Transform::Identity)\n> > + return t1;\n> > +\n> > + Transform combined = t1 ^ t2;\n> > + Transform result = combined;\n> > +\n> > + /*\n> > + * If the base orientation is transposed we need to invert the flips on\n> > + * the combined result.\n> > + *\n> > + * Using Rot90 as an example:\n> > + *\n> > + * Rot180 / Rot90 = Rot90\n> > + * = (H|V) ^ (T|V) = (T|H)\n> > + * = invert_flips(T|H) = (T|V) = Rot90\n> > + *\n> > + * Rot0 / Rot90 = Rot270\n> > + * = () ^ (T|V) = (T|V)\n> > + * = invert_flips(T|V) = (T|H) = Rot270\n> > + *\n> > + * Rot270 / Rot90 = Rot180\n> > + * = (T|H) ^ (T|V) = (H|V)\n> > + * = invert_flips(H|V) = (V|H) = Rot180\n> > + *\n> > + * Rot180Transpose / Rot90 = V\n> > + * = (T|H|V) ^ (T|V) = (H)\n> > + * = invert_flips(H) = (V)\n> > + *\n> > + * Rot270 / Transpose = V\n> > + * = (T|H) ^ T = H\n> > + * = invert_flips(H) = (V)\n> > + */\n> > + if (!!(t2 & Transform::Transpose)) {\n> > + result = combined & Transform::Transpose;\n> > + if (!!(combined & Transform::HFlip))\n> > + result |= Transform::VFlip;\n> > + if (!!(combined & Transform::VFlip))\n> > + result |= Transform::HFlip;\n> > + }\n> > +\n> > + return result;\n> > +}\n>\n> I was expecting something a bit simpler here, maybe\n>\n> Transform t1 = transformFromOrientation(o1);\n> Transform t2 = transformFromOrientation(o2);\n>\n> return transformToOrientation(t1 * -t2);\n\nNo need for transformToOrientation(), we're returning a transform\nhere.\n\n>\n\nsdeng\n\nI burnt quite some brain power for coming up with the implementation\nand an explanation that made sense for the \"if transposed then invert\nflips\" and now this single line does equally equally (I just re-ran the\ntest and it passes)...\n\n> And it does have the benefit that you can glance at it and think\n> \"yeah, it does what I expect\". And if it isn't as simple as that, then\n> there's probably something wrong!!\n>\n\nI admit I still don't clearly see why \"apply the inverse of t2 then\napply t1\" gives me the transform I need to get from t2 to t1. I understand\nit works because rot90 and rot270 are the inverse one of the other and\nthis \"flips\" the h/v bits like my code did, but if I have to reason in\nterms of actual plane transformations it is not immediately clear.\nTranspositions are not easy to visualize, but yes, it works so I guess\nit also validates that the test is correct as it does what you expect.\n\n> > +\n> > +/**\n> > + * \\brief Apply the Transform \\a t on the base orientation \\a o\n> > + * \\param o The base orientation\n> > + * \\param t The transform to apply on \\a o\n> > + * \\return The Orientation resulting from applying \\a t on \\a o\n> > + */\n> > +CameraConfiguration::Orientation operator*(CameraConfiguration::Orientation o,\n> > + Transform t)\n> > +{\n> > + /* Apply an Indentity always gives us back the other operand. */\n> > + if (t == Transform::Identity)\n> > + return o;\n> > +\n> > + /*\n> > + * As operator*(Transform t2, Transform t1) composes two Tranform\n> > + * by applying t1 first then t2 according to the canonical function\n> > + * composition notion, we here first apply a Transform corresponding to\n> > + * the base orientation and the apply \\a t to it.\n> > + */\n> > + Transform t1 = transformFromOrientation(o);\n> > + if (t1 == Transform::Identity)\n> > + return transformToOrientation(t);\n> > +\n> > + return transformToOrientation(t * t1);\n>\n> I would probably have gone straight to the final \"return\" statement,\n> without those other tests. I think it makes the whole function easier\n> to read and understand, and I don't think there's a meaningful gain\n> from the special cases.\n>\n\nPossibly, yes\n\n> On a semi-related note, I find myself second-guessing the order of\n> transform composition. The documentation is pretty clear that we're\n> using traditional mathematical composition rules, but does that still\n> make sense?\n>\n> We defined operator*(orientation, transform) so that we have:\n>\n> o1 * t = o2\n>\n> but if t = t1 * t2 then (according to the current rules)\n>\n> o2 * (t1 * t2) = o2 and therefore (o2 * t2) * t1 = o2 so in fact o2 *\n> t2 * t1 = o2. Or in other words, these two flavours of * aren't\n> associative. It all feels wrong.\n\nI got the same doubt to be honest. I just found more natural to have\n\nOrientation = Orientation * transform\n\nwhich reads \"The Transform you have to apply to an Orientation o1 to\nget Orientation o2\" as an API, but I agree the fact I then need to\nswap operands in the implementation to apply the orientation first\nthen the transform is misleading, even more so with multiple\ncompositions as per your example\n\n> Having a prefix version, namely operator*(transform, orientation)\n> would fix this nastiness, but I do wonder whether it would be more\n> natural to change the transform order when we compose them. Opinions\n> sought!!\n\nI honestly find the composition notion implemented by\noperator*(Transform t1, Transform t2) a little bit confusing, but it\nis indeed more formally correct.\n\nI guess we have to chose between\n\n1)\n\noperator*(Transform t1, Transform t2) = Apply t2 then t1\noperator*(Transform t, Orientation o) = Apply o then t\n\n2)\n\noperator*(Transform t1, Transform t2) = Apply t1 then t2\noperator*(Orientation o, Transform t) = Apply o then t\n\nThe only argument I have is the one I expressed above:\n\n\"Orientation * transform\" as \"the Transform t applied to Orientation o\"\n\nreads more naturally than\n\n\"Transform * Orientation\" as \"the Transform t applied to Orientation o\"\n\nBut it's not a strong opinion. What is your preference ?\n\n>\n> Thanks\n> David\n>\n> > +}\n> > +\n> > /**\n> > * \\brief Return a character string describing the transform\n> > * \\param[in] t The transform to be described.\n> > --\n> > 2.40.1\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3528DBDB1C\n\tfor ;\n\tMon, 17 Jul 2023 13:28:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9573A61E2C;\n\tMon, 17 Jul 2023 15:28:23 +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 55B7860383\n\tfor ;\n\tMon, 17 Jul 2023 15:28:21 +0200 (CEST)","from ideasonboard.com (mob-5-90-54-150.net.vodafone.it\n\t[5.90.54.150])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4E51C331B;\n\tMon, 17 Jul 2023 15:27:27 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1689600503;\n\tbh=6wBxu3NVH82j32nvgmrSuINBIT5+V24O5zSwgv/F678=;\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=EBkgSCBTJFP8EhmTK7iR9eIT/SYuRAeTYSVdKxtly7CXgctB1YEedKf7Ps5qNZyDW\n\tx5yivchli+6qJxSa6hkOftVJg8FhmsaBNYkwW0YRzJzo+mBySttdZFknjs649QRG4u\n\tioJCkS6BJR0Ah/SJnBD3N9qxCe3I7GYxbtA3ZsojJWty8+BHMAJcJ+4p9fDZvnW7Ab\n\t1xjgkjnc2KkwD1zKNdDawNACwU0nwaeUOQd3ph6MnfBWH3zzGes+radlXiMmI1CQX8\n\tsgo5WNnxAc3eLl0aQB7oVl+5u2xwjD2CKqaAoL+3jKjWDYmPSZmYmqBj/0NSdwrqW2\n\t3fPRebauX2UbQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1689600448;\n\tbh=6wBxu3NVH82j32nvgmrSuINBIT5+V24O5zSwgv/F678=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=E4FgsoDPqZhGxm7TeDLiKd2hjIwMZ/s292xBqT+IwquyaE0yeqSbve01j25s2FAoH\n\tmEqkZSF1QoVORT3zgFXLILrPGCQRonqXQofcNZNgsTt5tcTx1tYR9L55cQBdu7lLAx\n\tvjSvboVk+YogO+E4PrPVptNelPSwqWYH5oOpH0Z4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"E4FgsoDP\"; dkim-atps=neutral","Date":"Mon, 17 Jul 2023 15:28:16 +0200","To":"David Plowman ","Message-ID":"","References":"<20230714141549.11085-1-jacopo.mondi@ideasonboard.com>\n\t<20230714141549.11085-7-jacopo.mondi@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 6/9] libcamera: transform: Add\n\toperations with Orientation","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":27567,"web_url":"https://patchwork.libcamera.org/comment/27567/","msgid":"","date":"2023-07-17T14:01:52","subject":"Re: [libcamera-devel] [PATCH v2 6/9] libcamera: transform: Add\n\toperations with Orientation","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi\n\nOn Mon, 17 Jul 2023 at 14:28, Jacopo Mondi\n wrote:\n>\n> Hi David\n>\n> On Mon, Jul 17, 2023 at 11:50:16AM +0100, David Plowman via libcamera-devel wrote:\n> > Hi Jacopo\n> >\n> > Thanks for the patch!\n> >\n> > On Fri, 14 Jul 2023 at 15:16, Jacopo Mondi\n> > wrote:\n> > >\n> > > Add two operations that allows to combine Transform with Orientation.\n> >\n> > I see that the English version is a bit awkward here. Maybe s/allows/allow us/?\n> >\n> > >\n> > > - Transform operator/(Orientation1, Orientation2)\n> > > allows to easily get back the Transform that needs to be applied to\n> > > Orientation2 to get Orientation1\n> > >\n> > > - Orientation operator*(Orientation1, Transform)\n> > > allows to apply a Transform to an Orientation and obtain back the\n> >\n> > s/back//? The \"back\" seems marginally unnecessary to me.\n> >\n> > > combination of the two\n> > >\n> > > The two operations allow application that are willing to use Transform\n> > > to operate with the Orientation part of CameraConfiguration.\n> >\n> > Not sure about the wording, \"are willing\" sounds a bit like we're\n> > trying to force them, or something. Maybe\n> >\n> > \"These two operations allow applications to use Transforms to\n> > manipulate the Orientation inside the CameraConfiguration, if they\n> > wish.\"\n> >\n> > But please adjust it to something you're comfortable with!\n>\n> Ah well, I'm comfortable with whatever makes the most sense to native\n> speakers :)\n>\n> >\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > include/libcamera/transform.h | 5 ++\n> > > src/libcamera/transform.cpp | 98 +++++++++++++++++++++++++++++++++++\n> > > 2 files changed, 103 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/transform.h b/include/libcamera/transform.h\n> > > index 573adf18715d..a7470c70a755 100644\n> > > --- a/include/libcamera/transform.h\n> > > +++ b/include/libcamera/transform.h\n> > > @@ -74,6 +74,11 @@ Transform transformFromRotation(int angle, bool *success = nullptr);\n> > > Transform transformFromOrientation(const CameraConfiguration::Orientation &orientation);\n> > > CameraConfiguration::Orientation transformToOrientation(const Transform &transform);\n> > >\n> > > +Transform operator/(CameraConfiguration::Orientation o1,\n> > > + CameraConfiguration::Orientation o2);\n> > > +CameraConfiguration::Orientation operator*(CameraConfiguration::Orientation o1,\n> > > + Transform t);\n> > > +\n> > > const char *transformToString(Transform t);\n> > >\n> > > } /* namespace libcamera */\n> > > diff --git a/src/libcamera/transform.cpp b/src/libcamera/transform.cpp\n> > > index 03d2b52e7f38..61dbb1ff1dd5 100644\n> > > --- a/src/libcamera/transform.cpp\n> > > +++ b/src/libcamera/transform.cpp\n> > > @@ -357,6 +357,104 @@ CameraConfiguration::Orientation transformToOrientation(const Transform &transfo\n> > > return CameraConfiguration::rotate0;\n> > > }\n> > >\n> > > +/**\n> > > + * \\brief Return the Transform that applied to \\a o2 gives \\a o1\n> > > + * \\param o1 The Orientation to obtain\n> > > + * \\param o2 The base Orientation\n> > > + *\n> > > + * This operation can be used to easily compute the Transform to apply to a\n> > > + * base orientation \\a o2 to get the desired orientation \\a o1.\n> > > + *\n> > > + * The CameraSensor class uses this function to compute what Transform to apply\n> > > + * to a camera with mounting rotation \\a o2 (the base) to obtain the user\n> > > + * requested orientation \\a o1.\n> > > + *\n> > > + * \\return A Transform that applied to \\a o2 gives \\a o1\n> > > + */\n> > > +Transform operator/(CameraConfiguration::Orientation o1,\n> > > + CameraConfiguration::Orientation o2)\n> > > +{\n> > > + Transform t1 = transformFromOrientation(o1);\n> > > + Transform t2 = transformFromOrientation(o2);\n> > > +\n> > > + /* Return immediately if the two are identical. */\n> > > + if (t1 == t2)\n> > > + return Transform::Identity;\n> > > +\n> > > + /* If t1 is identity we need to apply -t2 to get it. */\n> > > + if (t1 == Transform::Identity)\n> > > + return -t2;\n> > > +\n> > > + /* If t2 is identity, to get t1 we still need to do... t1. */\n> > > + if (t2 == Transform::Identity)\n> > > + return t1;\n> > > +\n> > > + Transform combined = t1 ^ t2;\n> > > + Transform result = combined;\n> > > +\n> > > + /*\n> > > + * If the base orientation is transposed we need to invert the flips on\n> > > + * the combined result.\n> > > + *\n> > > + * Using Rot90 as an example:\n> > > + *\n> > > + * Rot180 / Rot90 = Rot90\n> > > + * = (H|V) ^ (T|V) = (T|H)\n> > > + * = invert_flips(T|H) = (T|V) = Rot90\n> > > + *\n> > > + * Rot0 / Rot90 = Rot270\n> > > + * = () ^ (T|V) = (T|V)\n> > > + * = invert_flips(T|V) = (T|H) = Rot270\n> > > + *\n> > > + * Rot270 / Rot90 = Rot180\n> > > + * = (T|H) ^ (T|V) = (H|V)\n> > > + * = invert_flips(H|V) = (V|H) = Rot180\n> > > + *\n> > > + * Rot180Transpose / Rot90 = V\n> > > + * = (T|H|V) ^ (T|V) = (H)\n> > > + * = invert_flips(H) = (V)\n> > > + *\n> > > + * Rot270 / Transpose = V\n> > > + * = (T|H) ^ T = H\n> > > + * = invert_flips(H) = (V)\n> > > + */\n> > > + if (!!(t2 & Transform::Transpose)) {\n> > > + result = combined & Transform::Transpose;\n> > > + if (!!(combined & Transform::HFlip))\n> > > + result |= Transform::VFlip;\n> > > + if (!!(combined & Transform::VFlip))\n> > > + result |= Transform::HFlip;\n> > > + }\n> > > +\n> > > + return result;\n> > > +}\n> >\n> > I was expecting something a bit simpler here, maybe\n> >\n> > Transform t1 = transformFromOrientation(o1);\n> > Transform t2 = transformFromOrientation(o2);\n> >\n> > return transformToOrientation(t1 * -t2);\n>\n> No need for transformToOrientation(), we're returning a transform\n> here.\n>\n> >\n>\n> sdeng\n>\n> I burnt quite some brain power for coming up with the implementation\n> and an explanation that made sense for the \"if transposed then invert\n> flips\" and now this single line does equally equally (I just re-ran the\n> test and it passes)...\n>\n> > And it does have the benefit that you can glance at it and think\n> > \"yeah, it does what I expect\". And if it isn't as simple as that, then\n> > there's probably something wrong!!\n> >\n>\n> I admit I still don't clearly see why \"apply the inverse of t2 then\n> apply t1\" gives me the transform I need to get from t2 to t1. I understand\n> it works because rot90 and rot270 are the inverse one of the other and\n> this \"flips\" the h/v bits like my code did, but if I have to reason in\n> terms of actual plane transformations it is not immediately clear.\n> Transpositions are not easy to visualize, but yes, it works so I guess\n> it also validates that the test is correct as it does what you expect.\n\nI think about it like this. If you apply the inverse of t2, then\nyou've undone t2 and you're back at orientation rotate0 (or\n\"identity\"). Now you just need to go forwards by t1 and you're done.\nSo long as we've tied our orientations and transforms together\ncorrectly, this _has_ to work, or else there's something seriously\nwrong!\n\n>\n> > > +\n> > > +/**\n> > > + * \\brief Apply the Transform \\a t on the base orientation \\a o\n> > > + * \\param o The base orientation\n> > > + * \\param t The transform to apply on \\a o\n> > > + * \\return The Orientation resulting from applying \\a t on \\a o\n> > > + */\n> > > +CameraConfiguration::Orientation operator*(CameraConfiguration::Orientation o,\n> > > + Transform t)\n> > > +{\n> > > + /* Apply an Indentity always gives us back the other operand. */\n> > > + if (t == Transform::Identity)\n> > > + return o;\n> > > +\n> > > + /*\n> > > + * As operator*(Transform t2, Transform t1) composes two Tranform\n> > > + * by applying t1 first then t2 according to the canonical function\n> > > + * composition notion, we here first apply a Transform corresponding to\n> > > + * the base orientation and the apply \\a t to it.\n> > > + */\n> > > + Transform t1 = transformFromOrientation(o);\n> > > + if (t1 == Transform::Identity)\n> > > + return transformToOrientation(t);\n> > > +\n> > > + return transformToOrientation(t * t1);\n> >\n> > I would probably have gone straight to the final \"return\" statement,\n> > without those other tests. I think it makes the whole function easier\n> > to read and understand, and I don't think there's a meaningful gain\n> > from the special cases.\n> >\n>\n> Possibly, yes\n>\n> > On a semi-related note, I find myself second-guessing the order of\n> > transform composition. The documentation is pretty clear that we're\n> > using traditional mathematical composition rules, but does that still\n> > make sense?\n> >\n> > We defined operator*(orientation, transform) so that we have:\n> >\n> > o1 * t = o2\n> >\n> > but if t = t1 * t2 then (according to the current rules)\n> >\n> > o2 * (t1 * t2) = o2 and therefore (o2 * t2) * t1 = o2 so in fact o2 *\n> > t2 * t1 = o2. Or in other words, these two flavours of * aren't\n> > associative. It all feels wrong.\n>\n> I got the same doubt to be honest. I just found more natural to have\n>\n> Orientation = Orientation * transform\n>\n> which reads \"The Transform you have to apply to an Orientation o1 to\n> get Orientation o2\" as an API, but I agree the fact I then need to\n> swap operands in the implementation to apply the orientation first\n> then the transform is misleading, even more so with multiple\n> compositions as per your example\n>\n> > Having a prefix version, namely operator*(transform, orientation)\n> > would fix this nastiness, but I do wonder whether it would be more\n> > natural to change the transform order when we compose them. Opinions\n> > sought!!\n>\n> I honestly find the composition notion implemented by\n> operator*(Transform t1, Transform t2) a little bit confusing, but it\n> is indeed more formally correct.\n>\n> I guess we have to chose between\n>\n> 1)\n>\n> operator*(Transform t1, Transform t2) = Apply t2 then t1\n> operator*(Transform t, Orientation o) = Apply o then t\n>\n> 2)\n>\n> operator*(Transform t1, Transform t2) = Apply t1 then t2\n> operator*(Orientation o, Transform t) = Apply o then t\n>\n> The only argument I have is the one I expressed above:\n>\n> \"Orientation * transform\" as \"the Transform t applied to Orientation o\"\n>\n> reads more naturally than\n>\n> \"Transform * Orientation\" as \"the Transform t applied to Orientation o\"\n>\n> But it's not a strong opinion. What is your preference ?\n\nYes, this is exactly it, we need to choose either 1 or 2 to be\nconsistent. The mere fact that, the other week, I automatically went\nfor operator*(Orientation, Transform) probably underlines that 2 is\nmore natural, and I would therefore prefer to change to this view of\nthe world. But maybe we need to see if others are OK for us to do\nthis!\n\nDavid\n\n>\n> >\n> > Thanks\n> > David\n> >\n> > > +}\n> > > +\n> > > /**\n> > > * \\brief Return a character string describing the transform\n> > > * \\param[in] t The transform to be described.\n> > > --\n> > > 2.40.1\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 2BCFEBDB1C\n\tfor ;\n\tMon, 17 Jul 2023 14:02:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6D6F9628C0;\n\tMon, 17 Jul 2023 16:02:05 +0200 (CEST)","from mail-qv1-xf29.google.com (mail-qv1-xf29.google.com\n\t[IPv6:2607:f8b0:4864:20::f29])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1E0AA60383\n\tfor ;\n\tMon, 17 Jul 2023 16:02:04 +0200 (CEST)","by mail-qv1-xf29.google.com with SMTP id\n\t6a1803df08f44-63770af327fso32777186d6.2\n\tfor ;\n\tMon, 17 Jul 2023 07:02:04 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1689602525;\n\tbh=qOav2N80dgUyt7DDPs2Pb0mRXFBTcWYmUjNAwjupU5c=;\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=C/KUycoMhaC9X3kCr3Qhu1tRN3ayjy0nUSiAqOQLCi1R8HVSHLfbSkvlTyxCK52FO\n\tWJqLuIk1oZNEpF1ecc51FiVj6uhurjxQ5D5H1obt/VhHtWBMVIQHQDgtKev9l84NlU\n\tS02wYVEWnvOqgVnnaoUyzYxW6du1d/m+yMUOpC2kE6LriJKCxc+if4oFtPjTFBNqvf\n\tYaJ2Zb/25jmQHTWkjIjIlUX+77/d9L98hhzshSv60bCgBBLL0UnvGHbfXy2Rxf4GTA\n\tN8A0Oufj+xmGmJmopHgTbbKugf7DXDSGqaY5csU2Al4oZVdk2Fk9Cubn3BN+GTtBUe\n\tOM3Un6ngumq+g==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1689602523; x=1692194523;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=HO52Q8tQZk/pmvx0qnyYwpqrj4c57ZYSkYGhNH2D3bI=;\n\tb=Kz+tX7ZdIDpPvPnhWFpP/TWOknReJthTKZ2oSgZsR9nQmAv16xuww7HonM6ftyPg84\n\tqx6J3lshTW+VBV3Z19lnOtmoPCjUPeziNn0HJ57H1De76wiHTJD3Z7bndRoattXDIYzD\n\tqsaI3eSVX6ikFqlEMrK1mx60HeKSq43Rhi8O/wL+sYKaZ+BSoXVNnIv/1rNV0FmchOJJ\n\tQoPffbsgs9gT/CWuy9hM4DcMpfklLHhMxtjO8l5KpcqRoo2VTUs1JMWwUoa47N9XYhZa\n\tw/lQ/jqd8CNyMcY9/o8LcoTZ984Rp/ADozbmeTpR8DPwe7SqAJeXPcnmv8sxsnTZpaL1\n\th3EQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"Kz+tX7Zd\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1689602523; x=1692194523;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=HO52Q8tQZk/pmvx0qnyYwpqrj4c57ZYSkYGhNH2D3bI=;\n\tb=Ju7YJF0bNjit9Z6Fe8K3kK+2NgY29xUIkao30z3joC9Fc6m13L9eIw0rvh2XhMMsLJ\n\t+W3AdTG1hKHQxHpQmMr6hZ8cfdxlAYrQ0oX0MNhFTIthj9j5hlZegTafNYQMKhOWnLgT\n\tQfR7UF1Tw//YCtuTGm+onjhnc+cMYZkKXOqaf1/or16wgolVymMgyca2X8cMbx9RLCMb\n\tA40ti7YUO4o068bP6OZi5PNy4k5vxdGLNo1SyxthC2Wch+nBZuFtDTafwytPUY4keA2w\n\tyXCmNdEBXT214aOElZ8U8G4uqXdUDEyXvfKkm0DhSt9gEOyFOmX0sGQifpHPWo1ruj2T\n\tpJyQ==","X-Gm-Message-State":"ABy/qLb/y6xvzzoctFhbPbrRIWdCLRqv0QRhFrXmWHzhxhTP16A0vu1H\n\t0p/VP9+wRwrzSrs1JSnwwNEhsR/RhX/oGMj5mfhzpZbuTIWsivka","X-Google-Smtp-Source":"APBJJlFpjt0ZmSthJhso5LLgcXANkHDlx/L+0PnD1JVlS5EQbELczmDlhk+fUOERX0k5eqytm1/jXg/tkRBBviR1pyI=","X-Received":"by 2002:a0c:a889:0:b0:630:21a6:bb5e with SMTP id\n\tx9-20020a0ca889000000b0063021a6bb5emr10387329qva.30.1689602522839;\n\tMon, 17 Jul 2023 07:02:02 -0700 (PDT)","MIME-Version":"1.0","References":"<20230714141549.11085-1-jacopo.mondi@ideasonboard.com>\n\t<20230714141549.11085-7-jacopo.mondi@ideasonboard.com>\n\t\n\t","In-Reply-To":"","Date":"Mon, 17 Jul 2023 15:01:52 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 6/9] libcamera: transform: Add\n\toperations with Orientation","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":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27568,"web_url":"https://patchwork.libcamera.org/comment/27568/","msgid":"","date":"2023-07-17T15:03:49","subject":"Re: [libcamera-devel] [PATCH v2 6/9] libcamera: transform: Add\n\toperations with Orientation","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi David\n\nOn Mon, Jul 17, 2023 at 03:01:52PM +0100, David Plowman via libcamera-devel wrote:\n> Hi\n\n[snip]\n\n> > > On a semi-related note, I find myself second-guessing the order of\n> > > transform composition. The documentation is pretty clear that we're\n> > > using traditional mathematical composition rules, but does that still\n> > > make sense?\n> > >\n> > > We defined operator*(orientation, transform) so that we have:\n> > >\n> > > o1 * t = o2\n> > >\n> > > but if t = t1 * t2 then (according to the current rules)\n> > >\n> > > o2 * (t1 * t2) = o2 and therefore (o2 * t2) * t1 = o2 so in fact o2 *\n> > > t2 * t1 = o2. Or in other words, these two flavours of * aren't\n> > > associative. It all feels wrong.\n> >\n> > I got the same doubt to be honest. I just found more natural to have\n> >\n> > Orientation = Orientation * transform\n> >\n> > which reads \"The Transform you have to apply to an Orientation o1 to\n> > get Orientation o2\" as an API, but I agree the fact I then need to\n> > swap operands in the implementation to apply the orientation first\n> > then the transform is misleading, even more so with multiple\n> > compositions as per your example\n> >\n> > > Having a prefix version, namely operator*(transform, orientation)\n> > > would fix this nastiness, but I do wonder whether it would be more\n> > > natural to change the transform order when we compose them. Opinions\n> > > sought!!\n> >\n> > I honestly find the composition notion implemented by\n> > operator*(Transform t1, Transform t2) a little bit confusing, but it\n> > is indeed more formally correct.\n> >\n> > I guess we have to chose between\n> >\n> > 1)\n> >\n> > operator*(Transform t1, Transform t2) = Apply t2 then t1\n> > operator*(Transform t, Orientation o) = Apply o then t\n> >\n> > 2)\n> >\n> > operator*(Transform t1, Transform t2) = Apply t1 then t2\n> > operator*(Orientation o, Transform t) = Apply o then t\n> >\n> > The only argument I have is the one I expressed above:\n> >\n> > \"Orientation * transform\" as \"the Transform t applied to Orientation o\"\n> >\n> > reads more naturally than\n> >\n> > \"Transform * Orientation\" as \"the Transform t applied to Orientation o\"\n> >\n> > But it's not a strong opinion. What is your preference ?\n>\n> Yes, this is exactly it, we need to choose either 1 or 2 to be\n> consistent. The mere fact that, the other week, I automatically went\n> for operator*(Orientation, Transform) probably underlines that 2 is\n> more natural, and I would therefore prefer to change to this view of\n> the world. But maybe we need to see if others are OK for us to do\n> this!\n\nAssuming nobody else has different ideas, would you like me to change\nthe operation semantic as part of this series or do you prefer to do\nit yourself and update your consumers (libcamera-apps and picamera2)\nat the same time ?\n\n>\n> David\n>\n> >\n> > >\n> > > Thanks\n> > > David\n> > >\n> > > > +}\n> > > > +\n> > > > /**\n> > > > * \\brief Return a character string describing the transform\n> > > > * \\param[in] t The transform to be described.\n> > > > --\n> > > > 2.40.1\n> > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 70261BDC71\n\tfor ;\n\tMon, 17 Jul 2023 15:03:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BDCD4628C0;\n\tMon, 17 Jul 2023 17:03:54 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EBB7660383\n\tfor ;\n\tMon, 17 Jul 2023 17:03:53 +0200 (CEST)","from ideasonboard.com (mob-5-90-54-150.net.vodafone.it\n\t[5.90.54.150])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 948B1331B;\n\tMon, 17 Jul 2023 17:03:00 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1689606234;\n\tbh=U3GAhlvbvkuHTWY6rJ3aAp1XjdOz7OFKRNb2RX1+syA=;\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=IP2W9u+ZMA3KSmmXeIx88iBUCzIoB3PxUuNe97ZSp9x076emLSv4Rq2ZMz9dFuSFo\n\tIxzyxq9u3g7SN3AnpkrCOKqBeP4vOEQ5DibmXr7Zjwbs+aWbuirOqlvXzUw0uHxTTa\n\te+5U/4MMN3jQRSzfuDpxlpiSVKojVVyQ5oeemwHi1KNCJ+nS6HHl/E8GYrzcwVncOR\n\tho/W319rQBI+BOkRY0GjnxGuwI3Dcn97uPPdTQ4Q+HWjKSpI+mpJvb3jVAhcbpcHQ0\n\tUZcophiw1Zb1s15zFAHQJXnb35pYgFjE7vU0WpJulKyAvDk9JjaaEybhMTNgwdpBa3\n\tCIWqyRRGKCg0w==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1689606180;\n\tbh=U3GAhlvbvkuHTWY6rJ3aAp1XjdOz7OFKRNb2RX1+syA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AJJeZcAGDNr5KtDzkg0ss4WvTbVkmkayZu1DSYBC3Wdmern6KU0iKWzJLeoXw7IDD\n\tS74dTQNlca4Bmfb7ooKYIi7e82C/icztYX+lzhHadInfkJow/72jHPGkaFMXAX+Dhz\n\tW6dDXV0GjlqW4Newz5Igfe9pXQ2NsBkXvSd54VlU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"AJJeZcAG\"; dkim-atps=neutral","Date":"Mon, 17 Jul 2023 17:03:49 +0200","To":"David Plowman ","Message-ID":"","References":"<20230714141549.11085-1-jacopo.mondi@ideasonboard.com>\n\t<20230714141549.11085-7-jacopo.mondi@ideasonboard.com>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 6/9] libcamera: transform: Add\n\toperations with Orientation","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":27573,"web_url":"https://patchwork.libcamera.org/comment/27573/","msgid":"","date":"2023-07-18T07:55:02","subject":"Re: [libcamera-devel] [PATCH v2 6/9] libcamera: transform: Add\n\toperations with Orientation","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nOn Mon, 17 Jul 2023 at 16:03, Jacopo Mondi\n wrote:\n>\n> Hi David\n>\n> On Mon, Jul 17, 2023 at 03:01:52PM +0100, David Plowman via libcamera-devel wrote:\n> > Hi\n>\n> [snip]\n>\n> > > > On a semi-related note, I find myself second-guessing the order of\n> > > > transform composition. The documentation is pretty clear that we're\n> > > > using traditional mathematical composition rules, but does that still\n> > > > make sense?\n> > > >\n> > > > We defined operator*(orientation, transform) so that we have:\n> > > >\n> > > > o1 * t = o2\n> > > >\n> > > > but if t = t1 * t2 then (according to the current rules)\n> > > >\n> > > > o2 * (t1 * t2) = o2 and therefore (o2 * t2) * t1 = o2 so in fact o2 *\n> > > > t2 * t1 = o2. Or in other words, these two flavours of * aren't\n> > > > associative. It all feels wrong.\n> > >\n> > > I got the same doubt to be honest. I just found more natural to have\n> > >\n> > > Orientation = Orientation * transform\n> > >\n> > > which reads \"The Transform you have to apply to an Orientation o1 to\n> > > get Orientation o2\" as an API, but I agree the fact I then need to\n> > > swap operands in the implementation to apply the orientation first\n> > > then the transform is misleading, even more so with multiple\n> > > compositions as per your example\n> > >\n> > > > Having a prefix version, namely operator*(transform, orientation)\n> > > > would fix this nastiness, but I do wonder whether it would be more\n> > > > natural to change the transform order when we compose them. Opinions\n> > > > sought!!\n> > >\n> > > I honestly find the composition notion implemented by\n> > > operator*(Transform t1, Transform t2) a little bit confusing, but it\n> > > is indeed more formally correct.\n> > >\n> > > I guess we have to chose between\n> > >\n> > > 1)\n> > >\n> > > operator*(Transform t1, Transform t2) = Apply t2 then t1\n> > > operator*(Transform t, Orientation o) = Apply o then t\n> > >\n> > > 2)\n> > >\n> > > operator*(Transform t1, Transform t2) = Apply t1 then t2\n> > > operator*(Orientation o, Transform t) = Apply o then t\n> > >\n> > > The only argument I have is the one I expressed above:\n> > >\n> > > \"Orientation * transform\" as \"the Transform t applied to Orientation o\"\n> > >\n> > > reads more naturally than\n> > >\n> > > \"Transform * Orientation\" as \"the Transform t applied to Orientation o\"\n> > >\n> > > But it's not a strong opinion. What is your preference ?\n> >\n> > Yes, this is exactly it, we need to choose either 1 or 2 to be\n> > consistent. The mere fact that, the other week, I automatically went\n> > for operator*(Orientation, Transform) probably underlines that 2 is\n> > more natural, and I would therefore prefer to change to this view of\n> > the world. But maybe we need to see if others are OK for us to do\n> > this!\n>\n> Assuming nobody else has different ideas, would you like me to change\n> the operation semantic as part of this series or do you prefer to do\n> it yourself and update your consumers (libcamera-apps and picamera2)\n> at the same time ?\n\nThanks for the offer, and I'd be very happy for you to do it!\n\nlibcamera-apps and Picamera2 both just pass the requested transform\nthrough and don't do anything with it - that's because they're not\nreally \"end-user applications\" in the conventional sense, they're more\n\"tools for building the end-user application\". So it would only be\nuser code that calls out to these that might be affected, and I doubt\nthere's very much.\n\nThanks!\nDavid\n\n>\n> >\n> > David\n> >\n> > >\n> > > >\n> > > > Thanks\n> > > > David\n> > > >\n> > > > > +}\n> > > > > +\n> > > > > /**\n> > > > > * \\brief Return a character string describing the transform\n> > > > > * \\param[in] t The transform to be described.\n> > > > > --\n> > > > > 2.40.1\n> > > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D5D86BDC71\n\tfor ;\n\tTue, 18 Jul 2023 07:55:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3CE9A628C0;\n\tTue, 18 Jul 2023 09:55:16 +0200 (CEST)","from mail-qt1-x82b.google.com (mail-qt1-x82b.google.com\n\t[IPv6:2607:f8b0:4864:20::82b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8FBBD61E2A\n\tfor ;\n\tTue, 18 Jul 2023 09:55:14 +0200 (CEST)","by mail-qt1-x82b.google.com with SMTP id\n\td75a77b69052e-403fab47687so2249401cf.1\n\tfor ;\n\tTue, 18 Jul 2023 00:55:14 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1689666916;\n\tbh=NiMVRPqdVCmWX8lMwltXyYEV6xP93J9WRLqS3J7kyTE=;\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=chFxaUeStd7XuNlCELSUxQc56zcnha0wADccAn+mfupNECru+oFFBA8a1qYw97e8q\n\tEIZFE+CkJOF/j5HpmMVMFRtnT7JQYOtK3XxiDYdi9YRH09+cBTONKGQr7fqmf+SFQu\n\tQBtVhZFwjEbjFPRhHL9u0G3CuYpJ3Bh4JHfC+RF2k8rORF1pLbn5W0B18P+NpgpDnp\n\tZUMenrJn9gUtrvG7cOHy3aF0nVutuVyIBL9eIvhw6iY39WHao1lwi+x427x12BfkT5\n\txqj5IIgOYxw/g9g9ML5Tc/nzkC5UcFJGjqCaf3v3DD+LGNlIptdyflrI0PZg6guQpr\n\t5zCJy1/JXQgBA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1689666913; x=1692258913;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=ScwDdW+KMeWbigBwZXKUsu2DUn2pxFZXg6gRSq2wFSc=;\n\tb=MPNOijQvgQnISLLN677xGWAyLEnRUUprshH9kFnIJy6KKkmP3Ew8Vqj1ENjnWf+tZA\n\ttesmiE0PmtaFVo4SdZvo2uLTeV39rybzHt3V5btEhAAAM3M3QouTUo8wmqx+4CRrmo/S\n\tGnBk/52l6cqKatnV4L9/rRrW5xK3nCs9zk1PpsK09ygBn7WmK+DIdBUlaE5ajFELuWRE\n\tVo+mqgzhuacl004qixEMrOlUQcErcIDJ+mv7cHpvbD2ve6aTdczVK31p4acF2WfW5hYl\n\t+cILa51OTpzbSJuHUQYwhp15S9wOwRipiRthE34a51UKp+nIQ91ch5+e0cs+Uber0poi\n\tzUeQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"MPNOijQv\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1689666913; x=1692258913;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=ScwDdW+KMeWbigBwZXKUsu2DUn2pxFZXg6gRSq2wFSc=;\n\tb=QRlg4L00iSv6iKASs6bIyaqpdSDykbSktL7lwmjHbtCGaDF7wAgAioTyxchM/b84vo\n\tdyAiQgoisK5daINVgyz2hfx0qSNhyrNyEJgMr77R78+7o3TUjX960L2uXaK3fiqtVFLI\n\tdm1U58nKzbolTtf2/8XbiGg1nw8oEdjZ+5+RXJVkriT2ajuGcPLfEY6lmLMnzgdFgaVz\n\tEc+zElC63YQ3gW7m9iXJTHhJTiJHYp8U4Hn2zyta7U+tcoTkuVKWkUaBdHHePkHyL/2c\n\tz948UR4bWl+AacWqNwHgEu8A6KiBIBLDjRUYnluvyCgBPDjk50fpF/Fy8phmThzEh7bB\n\tEUZg==","X-Gm-Message-State":"ABy/qLY0OhRYOoOlsuXocJJKMrGi8pzk2avKEKymd6MyK71hIrHs3/PL\n\t4QEds3hDqOnwJsNxQ31jNF/rXZpMv8FoUILfPnDgh89BvNoz/uBZ","X-Google-Smtp-Source":"APBJJlGJp6vqdmOvdOcUntRyGpgTmEhtZGjrbSNuJSXgedM93tdHTR2X9yTJnzAJide71fe+2dZOs3PGNkcp4OnLa5w=","X-Received":"by 2002:a05:622a:30e:b0:402:8ecd:810f with SMTP id\n\tq14-20020a05622a030e00b004028ecd810fmr18788726qtw.40.1689666913447;\n\tTue, 18 Jul 2023 00:55:13 -0700 (PDT)","MIME-Version":"1.0","References":"<20230714141549.11085-1-jacopo.mondi@ideasonboard.com>\n\t<20230714141549.11085-7-jacopo.mondi@ideasonboard.com>\n\t\n\t\n\t\n\t","In-Reply-To":"","Date":"Tue, 18 Jul 2023 08:55:02 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 6/9] libcamera: transform: Add\n\toperations with Orientation","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":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]