[{"id":27989,"web_url":"https://patchwork.libcamera.org/comment/27989/","msgid":"<20231018222234.GP1512@pendragon.ideasonboard.com>","date":"2023-10-18T22:22:34","subject":"Re: [libcamera-devel] [PATCH v5 05/12] libcamera: transform: Add\n\tfunctions to convert Orientation","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:08PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> Add two helper functions to the transform.cpp file that allows to\n> convert to and from an Orientation.\n> \n> Signed-off-by: Jacopo Mondi \n> Reviewed-by: David Plowman \n> ---\n> include/libcamera/transform.h | 4 +++\n> src/libcamera/transform.cpp | 60 +++++++++++++++++++++++++++++++++++\n> 2 files changed, 64 insertions(+)\n> \n> diff --git a/include/libcamera/transform.h b/include/libcamera/transform.h\n> index 2a6838c78369..14f1db0e8572 100644\n> --- a/include/libcamera/transform.h\n> +++ b/include/libcamera/transform.h\n> @@ -11,6 +11,8 @@\n> \n> namespace libcamera {\n> \n> +enum class Orientation;\n> +\n> enum class Transform : int {\n> \tIdentity = 0,\n> \tRot0 = Identity,\n> @@ -69,6 +71,8 @@ constexpr Transform operator~(Transform t)\n> }\n> \n> Transform transformFromRotation(int angle, bool *success = nullptr);\n> +Transform transformFromOrientation(const Orientation &orientation);\n> +Orientation transformToOrientation(const Transform &transform);\n> \n> const char *transformToString(Transform t);\n> \n> diff --git a/src/libcamera/transform.cpp b/src/libcamera/transform.cpp\n> index 4668303d0676..c70cac3f14ee 100644\n> --- a/src/libcamera/transform.cpp\n> +++ b/src/libcamera/transform.cpp\n> @@ -7,6 +7,8 @@\n> \n> #include \n> \n> +#include \n> +\n> /**\n> * \\file transform.h\n> * \\brief Enum to represent and manipulate 2D plane transforms\n> @@ -299,6 +301,64 @@ Transform transformFromRotation(int angle, bool *success)\n> \treturn Transform::Identity;\n> }\n> \n> +/**\n> + * \\brief Return the transform representing \\a orientation\n> + * \\param[in] orientation The orientation to convert\n> + * \\return The transform corresponding to \\a orientation\n> + */\n\nI don't like these functions much, as orientations and transforms are\nnot intrinsincly convertible. There's no cardinal concept such as \"a\ntransform representing an orientation\".\n\nFortunately, transformToOrientation() can easily be dropped on top of this\nseries, as one of its callers can go away (see reviews of other patches\nin the series), and the code can then be folded in the other caller,\n`Orientation operator*(const Orientation &o, const Transform &t)`.\n\nSimilarly, I would like to drop the call to transformFromOrientation()\nin RPi::CameraData::configureIPA() and replace it there with usage of\n`Transform operator/(const Orientation &o1, const Orientation &o2)`.\n\nThis can be done on top, as avoiding the introduction of these two\nfunctions in the series may cause annoying conflicts when rebasing and\nrefactoring.\n\n> +Transform transformFromOrientation(const Orientation &orientation)\n> +{\n> +\tswitch (orientation) {\n> +\tcase Orientation::rotate0:\n> +\t\treturn Transform::Identity;\n> +\tcase Orientation::rotate0Flip:\n> +\t\treturn Transform::HFlip;\n> +\tcase Orientation::rotate180:\n> +\t\treturn Transform::Rot180;\n> +\tcase Orientation::rotate180Flip:\n> +\t\treturn Transform::VFlip;\n> +\tcase Orientation::rotate90Flip:\n> +\t\treturn Transform::Transpose;\n> +\tcase Orientation::rotate90:\n> +\t\treturn Transform::Rot90;\n> +\tcase Orientation::rotate270Flip:\n> +\t\treturn Transform::Rot180Transpose;\n> +\tcase Orientation::rotate270:\n> +\t\treturn Transform::Rot270;\n> +\t}\n> +\n> +\treturn Transform::Identity;\n> +}\n> +\n> +/**\n> + * \\brief Return the Orientation representing \\a transform\n> + * \\param[in] transform The transform to convert\n> + * \\return The Orientation corresponding to \\a transform\n> + */\n> +Orientation transformToOrientation(const Transform &transform)\n> +{\n> +\tswitch (transform) {\n> +\tcase Transform::Identity:\n> +\t\treturn Orientation::rotate0;\n> +\tcase Transform::HFlip:\n> +\t\treturn Orientation::rotate0Flip;\n> +\tcase Transform::VFlip:\n> +\t\treturn Orientation::rotate180Flip;\n> +\tcase Transform::Rot180:\n> +\t\treturn Orientation::rotate180;\n> +\tcase Transform::Transpose:\n> +\t\treturn Orientation::rotate90Flip;\n> +\tcase Transform::Rot270:\n> +\t\treturn Orientation::rotate270;\n> +\tcase Transform::Rot90:\n> +\t\treturn Orientation::rotate90;\n> +\tcase Transform::Rot180Transpose:\n> +\t\treturn Orientation::rotate270Flip;\n> +\t}\n> +\n> +\treturn Orientation::rotate0;\n> +}\n> +\n> /**\n> * \\brief Return a character string describing the transform\n> * \\param[in] t The transform to be described.","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 CB724C3272\n\tfor ;\n\tWed, 18 Oct 2023 22:22:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EEE866297F;\n\tThu, 19 Oct 2023 00:22:29 +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 24BB76295F\n\tfor ;\n\tThu, 19 Oct 2023 00:22:28 +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 4A00E291;\n\tThu, 19 Oct 2023 00:22:20 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1697667750;\n\tbh=hFhmlg/zBYqitW4ani5eoQ3YH0c5Tc1agMKzveVZV6I=;\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=Um7AZWlC0sMrkDjfIDa54hD7t4cFgv0y9NWo2zeY5B/vK3d8Zp3H5mNGwflodU3CZ\n\tz2clZTl+SFGGvqICShPIXX66SpjWR1ZZiitlEALNK6rGmTOyJkCFYMvAFND3qXky5S\n\tkjBFgUGkx0asHzDjz2JNtkHlT7qnEvy6FgyUo0qygbGwuEfelngK1FxThgV9MFfWEi\n\tJWe7s+zGBdo8hAnJQQDa2Soly+HbAIlqZUCgloRX9jIVNjvMo5qbZyTwAcxwSJHrJu\n\t0rr4109IXH1kt8sik/BrJwDsONwoQdcNComGDy012H5fH9d70V6FHoNqf3YdYyyh6j\n\tyXIgAHm4aNaFQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1697667740;\n\tbh=hFhmlg/zBYqitW4ani5eoQ3YH0c5Tc1agMKzveVZV6I=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LVwcTosOL75C5wvcmbayai2U2YREdN1h5TzR7ny8nGezHOSRMQ/1ggI9bZSa1yxYC\n\tN/BbBD98v+CrfCmDbtuvc5WFz5lqLIdtZLR8wXUtEIIV2yQM4ZzV2HWXNPmuwqkRx+\n\tA3xZE+zmTaUg+lDoQTM/8fUnoE3dWzsi8C97m/gg="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"LVwcTosO\"; dkim-atps=neutral","Date":"Thu, 19 Oct 2023 01:22:34 +0300","To":"Jacopo Mondi ","Message-ID":"<20231018222234.GP1512@pendragon.ideasonboard.com>","References":"<20230901150215.11585-1-jacopo.mondi@ideasonboard.com>\n\t<20230901150215.11585-6-jacopo.mondi@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230901150215.11585-6-jacopo.mondi@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5 05/12] libcamera: transform: Add\n\tfunctions to convert 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":"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":27998,"web_url":"https://patchwork.libcamera.org/comment/27998/","msgid":"","date":"2023-10-19T10:34:08","subject":"Re: [libcamera-devel] [PATCH v5 05/12] libcamera: transform: Add\n\tfunctions to convert Orientation","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Laurent\n\nOn Thu, Oct 19, 2023 at 01:22:34AM +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:08PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > Add two helper functions to the transform.cpp file that allows to\n> > convert to and from an Orientation.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > Reviewed-by: David Plowman \n> > ---\n> > include/libcamera/transform.h | 4 +++\n> > src/libcamera/transform.cpp | 60 +++++++++++++++++++++++++++++++++++\n> > 2 files changed, 64 insertions(+)\n> >\n> > diff --git a/include/libcamera/transform.h b/include/libcamera/transform.h\n> > index 2a6838c78369..14f1db0e8572 100644\n> > --- a/include/libcamera/transform.h\n> > +++ b/include/libcamera/transform.h\n> > @@ -11,6 +11,8 @@\n> >\n> > namespace libcamera {\n> >\n> > +enum class Orientation;\n> > +\n> > enum class Transform : int {\n> > \tIdentity = 0,\n> > \tRot0 = Identity,\n> > @@ -69,6 +71,8 @@ constexpr Transform operator~(Transform t)\n> > }\n> >\n> > Transform transformFromRotation(int angle, bool *success = nullptr);\n> > +Transform transformFromOrientation(const Orientation &orientation);\n> > +Orientation transformToOrientation(const Transform &transform);\n> >\n> > const char *transformToString(Transform t);\n> >\n> > diff --git a/src/libcamera/transform.cpp b/src/libcamera/transform.cpp\n> > index 4668303d0676..c70cac3f14ee 100644\n> > --- a/src/libcamera/transform.cpp\n> > +++ b/src/libcamera/transform.cpp\n> > @@ -7,6 +7,8 @@\n> >\n> > #include \n> >\n> > +#include \n> > +\n> > /**\n> > * \\file transform.h\n> > * \\brief Enum to represent and manipulate 2D plane transforms\n> > @@ -299,6 +301,64 @@ Transform transformFromRotation(int angle, bool *success)\n> > \treturn Transform::Identity;\n> > }\n> >\n> > +/**\n> > + * \\brief Return the transform representing \\a orientation\n> > + * \\param[in] orientation The orientation to convert\n> > + * \\return The transform corresponding to \\a orientation\n> > + */\n>\n> I don't like these functions much, as orientations and transforms are\n> not intrinsincly convertible. There's no cardinal concept such as \"a\n> transform representing an orientation\".\n>\n\nAre we sure ? Orientation and Transform describe exactly the same set\nof 2d plane transformations, isn't there a 1-to-1 relationship ?\n\n> Fortunately, transformToOrientation() can easily be dropped on top of this\n> series, as one of its callers can go away (see reviews of other patches\n> in the series), and the code can then be folded in the other caller,\n> `Orientation operator*(const Orientation &o, const Transform &t)`.\n\nYou know, I can't find any comment related to this. Can you provide a\nreference ?\n\n>\n> Similarly, I would like to drop the call to transformFromOrientation()\n> in RPi::CameraData::configureIPA() and replace it there with usage of\n> `Transform operator/(const Orientation &o1, const Orientation &o2)`.\n>\n> This can be done on top, as avoiding the introduction of these two\n> functions in the series may cause annoying conflicts when rebasing and\n> refactoring.\n>\n> > +Transform transformFromOrientation(const Orientation &orientation)\n> > +{\n> > +\tswitch (orientation) {\n> > +\tcase Orientation::rotate0:\n> > +\t\treturn Transform::Identity;\n> > +\tcase Orientation::rotate0Flip:\n> > +\t\treturn Transform::HFlip;\n> > +\tcase Orientation::rotate180:\n> > +\t\treturn Transform::Rot180;\n> > +\tcase Orientation::rotate180Flip:\n> > +\t\treturn Transform::VFlip;\n> > +\tcase Orientation::rotate90Flip:\n> > +\t\treturn Transform::Transpose;\n> > +\tcase Orientation::rotate90:\n> > +\t\treturn Transform::Rot90;\n> > +\tcase Orientation::rotate270Flip:\n> > +\t\treturn Transform::Rot180Transpose;\n> > +\tcase Orientation::rotate270:\n> > +\t\treturn Transform::Rot270;\n> > +\t}\n> > +\n> > +\treturn Transform::Identity;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Return the Orientation representing \\a transform\n> > + * \\param[in] transform The transform to convert\n> > + * \\return The Orientation corresponding to \\a transform\n> > + */\n> > +Orientation transformToOrientation(const Transform &transform)\n> > +{\n> > +\tswitch (transform) {\n> > +\tcase Transform::Identity:\n> > +\t\treturn Orientation::rotate0;\n> > +\tcase Transform::HFlip:\n> > +\t\treturn Orientation::rotate0Flip;\n> > +\tcase Transform::VFlip:\n> > +\t\treturn Orientation::rotate180Flip;\n> > +\tcase Transform::Rot180:\n> > +\t\treturn Orientation::rotate180;\n> > +\tcase Transform::Transpose:\n> > +\t\treturn Orientation::rotate90Flip;\n> > +\tcase Transform::Rot270:\n> > +\t\treturn Orientation::rotate270;\n> > +\tcase Transform::Rot90:\n> > +\t\treturn Orientation::rotate90;\n> > +\tcase Transform::Rot180Transpose:\n> > +\t\treturn Orientation::rotate270Flip;\n> > +\t}\n> > +\n> > +\treturn Orientation::rotate0;\n> > +}\n> > +\n> > /**\n> > * \\brief Return a character string describing the transform\n> > * \\param[in] t The transform to be described.\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 5B75EC3272\n\tfor ;\n\tThu, 19 Oct 2023 10:34:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B18A86297F;\n\tThu, 19 Oct 2023 12:34:12 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F244E6055B\n\tfor ;\n\tThu, 19 Oct 2023 12:34:11 +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 D5CC3276;\n\tThu, 19 Oct 2023 12:34:03 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1697711652;\n\tbh=KD7QGJ5jA5GpdSVW0z6Mpr93/vB0BnobUZFqABKXoEo=;\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=oN719q2wSS0LeVsa9VAkKl6xIlHzWHdR8ILSfVTEunehgOHVHmGE1RdVWGUgMlysI\n\tjP5SCTDi4JAJ2dyfl1m0nq1bZeSA7Z7qvrXFFlnAibDJuX/CPS+zpaKwYAahNThDTT\n\tew8m9nYQlp/er3roVLdmOYIpZyIAx86z5HUrJ1x76ssZDQlEv5GLC/pbx3JcGFmCJt\n\tTIG8s5FJpweySmdqWa+Pex03LeVfII8cQm6xPUXuBv2bY2OyNktOMosMI/mcZyrpIC\n\ttMl+O2Qr2xpLbDcMD3hRNTKaKwImUiWLlasgKej0YM773j+FHtfB6B9P4kTQmNR81j\n\tG4OJYZJBsowlg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1697711644;\n\tbh=KD7QGJ5jA5GpdSVW0z6Mpr93/vB0BnobUZFqABKXoEo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UP9n9PxACemE4AACTli5eCJhuUySIgsnAdMZQX1F5ERDWxxrMnfvBQ9lmxw3N842v\n\tx+HIhRhLreI1jzpEG1KgX7JiXLEZr+7okAquhSESTiB4JH2hu8iOiZ9FJ7OBpn7H5Z\n\tN5NVQJ0WerJ7iN7DTrN9P7ih41WOnL5ZkG14d0+c="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"UP9n9PxA\"; dkim-atps=neutral","Date":"Thu, 19 Oct 2023 12:34:08 +0200","To":"Laurent Pinchart ","Message-ID":"","References":"<20230901150215.11585-1-jacopo.mondi@ideasonboard.com>\n\t<20230901150215.11585-6-jacopo.mondi@ideasonboard.com>\n\t<20231018222234.GP1512@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20231018222234.GP1512@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5 05/12] libcamera: transform: Add\n\tfunctions to convert 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":28001,"web_url":"https://patchwork.libcamera.org/comment/28001/","msgid":"","date":"2023-10-19T12:34:25","subject":"Re: [libcamera-devel] [PATCH v5 05/12] libcamera: transform: Add\n\tfunctions to convert Orientation","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi again\n\nOn Thu, Oct 19, 2023 at 12:34:08PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> Hi Laurent\n>\n> On Thu, Oct 19, 2023 at 01:22:34AM +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:08PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > > Add two helper functions to the transform.cpp file that allows to\n> > > convert to and from an Orientation.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > Reviewed-by: David Plowman \n> > > ---\n> > > include/libcamera/transform.h | 4 +++\n> > > src/libcamera/transform.cpp | 60 +++++++++++++++++++++++++++++++++++\n> > > 2 files changed, 64 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/transform.h b/include/libcamera/transform.h\n> > > index 2a6838c78369..14f1db0e8572 100644\n> > > --- a/include/libcamera/transform.h\n> > > +++ b/include/libcamera/transform.h\n> > > @@ -11,6 +11,8 @@\n> > >\n> > > namespace libcamera {\n> > >\n> > > +enum class Orientation;\n> > > +\n> > > enum class Transform : int {\n> > > \tIdentity = 0,\n> > > \tRot0 = Identity,\n> > > @@ -69,6 +71,8 @@ constexpr Transform operator~(Transform t)\n> > > }\n> > >\n> > > Transform transformFromRotation(int angle, bool *success = nullptr);\n> > > +Transform transformFromOrientation(const Orientation &orientation);\n> > > +Orientation transformToOrientation(const Transform &transform);\n> > >\n> > > const char *transformToString(Transform t);\n> > >\n> > > diff --git a/src/libcamera/transform.cpp b/src/libcamera/transform.cpp\n> > > index 4668303d0676..c70cac3f14ee 100644\n> > > --- a/src/libcamera/transform.cpp\n> > > +++ b/src/libcamera/transform.cpp\n> > > @@ -7,6 +7,8 @@\n> > >\n> > > #include \n> > >\n> > > +#include \n> > > +\n> > > /**\n> > > * \\file transform.h\n> > > * \\brief Enum to represent and manipulate 2D plane transforms\n> > > @@ -299,6 +301,64 @@ Transform transformFromRotation(int angle, bool *success)\n> > > \treturn Transform::Identity;\n> > > }\n> > >\n> > > +/**\n> > > + * \\brief Return the transform representing \\a orientation\n> > > + * \\param[in] orientation The orientation to convert\n> > > + * \\return The transform corresponding to \\a orientation\n> > > + */\n> >\n> > I don't like these functions much, as orientations and transforms are\n> > not intrinsincly convertible. There's no cardinal concept such as \"a\n> > transform representing an orientation\".\n> >\n>\n> Are we sure ? Orientation and Transform describe exactly the same set\n> of 2d plane transformations, isn't there a 1-to-1 relationship ?\n>\n> > Fortunately, transformToOrientation() can easily be dropped on top of this\n> > series, as one of its callers can go away (see reviews of other patches\n> > in the series), and the code can then be folded in the other caller,\n> > `Orientation operator*(const Orientation &o, const Transform &t)`.\n>\n> You know, I can't find any comment related to this. Can you provide a\n> reference ?\n>\n\nFound it in 10/12\n\nI think we can remove these two functions by:\n\n- Introduce a orientationFromRotation() to replace\n\n rotationTransform_ = transformFromRotation(propertyValue, &success);\n\n in CameraSensor::initProperties()\n\n so that we can remove transformToOrientation() in CameraSensor::computeTransform()\n\n- Move the RPi pipeline and IPA to use Orientation and remove\n\n\tparams.transform =\n\t\tstatic_cast(transformFromOrientation(config->orientation));\n\n so we can remove transformFromOrientation()\n\n- Replace py_transform which uses Transform\n\nThis is all doable, but I would like to sync with RPi as I presume\nthey have code in their apps which is based on these interfaces\n\nCC-ed David and Naush\n\n\n> >\n> > Similarly, I would like to drop the call to transformFromOrientation()\n> > in RPi::CameraData::configureIPA() and replace it there with usage of\n> > `Transform operator/(const Orientation &o1, const Orientation &o2)`.\n> >\n> > This can be done on top, as avoiding the introduction of these two\n> > functions in the series may cause annoying conflicts when rebasing and\n> > refactoring.\n> >\n> > > +Transform transformFromOrientation(const Orientation &orientation)\n> > > +{\n> > > +\tswitch (orientation) {\n> > > +\tcase Orientation::rotate0:\n> > > +\t\treturn Transform::Identity;\n> > > +\tcase Orientation::rotate0Flip:\n> > > +\t\treturn Transform::HFlip;\n> > > +\tcase Orientation::rotate180:\n> > > +\t\treturn Transform::Rot180;\n> > > +\tcase Orientation::rotate180Flip:\n> > > +\t\treturn Transform::VFlip;\n> > > +\tcase Orientation::rotate90Flip:\n> > > +\t\treturn Transform::Transpose;\n> > > +\tcase Orientation::rotate90:\n> > > +\t\treturn Transform::Rot90;\n> > > +\tcase Orientation::rotate270Flip:\n> > > +\t\treturn Transform::Rot180Transpose;\n> > > +\tcase Orientation::rotate270:\n> > > +\t\treturn Transform::Rot270;\n> > > +\t}\n> > > +\n> > > +\treturn Transform::Identity;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Return the Orientation representing \\a transform\n> > > + * \\param[in] transform The transform to convert\n> > > + * \\return The Orientation corresponding to \\a transform\n> > > + */\n> > > +Orientation transformToOrientation(const Transform &transform)\n> > > +{\n> > > +\tswitch (transform) {\n> > > +\tcase Transform::Identity:\n> > > +\t\treturn Orientation::rotate0;\n> > > +\tcase Transform::HFlip:\n> > > +\t\treturn Orientation::rotate0Flip;\n> > > +\tcase Transform::VFlip:\n> > > +\t\treturn Orientation::rotate180Flip;\n> > > +\tcase Transform::Rot180:\n> > > +\t\treturn Orientation::rotate180;\n> > > +\tcase Transform::Transpose:\n> > > +\t\treturn Orientation::rotate90Flip;\n> > > +\tcase Transform::Rot270:\n> > > +\t\treturn Orientation::rotate270;\n> > > +\tcase Transform::Rot90:\n> > > +\t\treturn Orientation::rotate90;\n> > > +\tcase Transform::Rot180Transpose:\n> > > +\t\treturn Orientation::rotate270Flip;\n> > > +\t}\n> > > +\n> > > +\treturn Orientation::rotate0;\n> > > +}\n> > > +\n> > > /**\n> > > * \\brief Return a character string describing the transform\n> > > * \\param[in] t The transform to be described.\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 D4D1FC3272\n\tfor ;\n\tThu, 19 Oct 2023 12:34:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4234561DD2;\n\tThu, 19 Oct 2023 14:34:31 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 88A226055B\n\tfor ;\n\tThu, 19 Oct 2023 14:34:29 +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 4EAF5276;\n\tThu, 19 Oct 2023 14:34:21 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1697718871;\n\tbh=D5a01rZCN+dC+mNefPJo6NsgnJySBQHp4lGa8iRuffo=;\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=UhJ6811VUWllxlctnTnXE+pvQKeI3M3pecgvkKOnxeClHsdsnGx5B+T1XWEBP3aR5\n\te1oiEReXufUBfP7CevgggNsRjMsMuuJqf+g3MeFxNiKcHUaDaojnsJEuKRdII+BMga\n\tl619koqprRLqxDIb3rsJWGNfZmrvASVhXEJkvq4YJMvOh60DX9g54dSv7xLbY+D2+y\n\to2RYDNXUA+3qP51KTZ/eDKqYxeH/eRfjhdyYiElxQ/gcdit7ew0ftSVFzGiML4bWim\n\tB56OkEUDhtsuaVOlW+BIsXjHOlhar1X+uRkLhr1WdtsUDkVRpQ1e/+euHxN5vEOLue\n\t8VlULkDfzHC2w==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1697718861;\n\tbh=D5a01rZCN+dC+mNefPJo6NsgnJySBQHp4lGa8iRuffo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZJgjrYwkg8y3OC8pXAtmRjRESO3iWj6FNKXxZbWNcphg3p5kGunLEoc9g9BxiFVzy\n\tP8OiN6HdeToTDPL/Cw4o5sonqmVB/YmCk/OC2ZDGjPXUu4sUiIIeQ0T8EEm524FEMc\n\tV8A+GLvJTCZtOkPXDAHGb7jWBVZAFOzQ0Em5pv+U="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ZJgjrYwk\"; dkim-atps=neutral","Date":"Thu, 19 Oct 2023 14:34:25 +0200","To":"Jacopo Mondi ","Message-ID":"","References":"<20230901150215.11585-1-jacopo.mondi@ideasonboard.com>\n\t<20230901150215.11585-6-jacopo.mondi@ideasonboard.com>\n\t<20231018222234.GP1512@pendragon.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 v5 05/12] libcamera: transform: Add\n\tfunctions to convert 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":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28002,"web_url":"https://patchwork.libcamera.org/comment/28002/","msgid":"","date":"2023-10-19T13:05:38","subject":"Re: [libcamera-devel] [PATCH v5 05/12] libcamera: transform: Add\n\tfunctions to convert 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 Thu, 19 Oct 2023 at 13:34, Jacopo Mondi\n wrote:\n>\n> Hi again\n>\n> On Thu, Oct 19, 2023 at 12:34:08PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > Hi Laurent\n> >\n> > On Thu, Oct 19, 2023 at 01:22:34AM +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:08PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > > > Add two helper functions to the transform.cpp file that allows to\n> > > > convert to and from an Orientation.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > Reviewed-by: David Plowman \n> > > > ---\n> > > > include/libcamera/transform.h | 4 +++\n> > > > src/libcamera/transform.cpp | 60 +++++++++++++++++++++++++++++++++++\n> > > > 2 files changed, 64 insertions(+)\n> > > >\n> > > > diff --git a/include/libcamera/transform.h b/include/libcamera/transform.h\n> > > > index 2a6838c78369..14f1db0e8572 100644\n> > > > --- a/include/libcamera/transform.h\n> > > > +++ b/include/libcamera/transform.h\n> > > > @@ -11,6 +11,8 @@\n> > > >\n> > > > namespace libcamera {\n> > > >\n> > > > +enum class Orientation;\n> > > > +\n> > > > enum class Transform : int {\n> > > > Identity = 0,\n> > > > Rot0 = Identity,\n> > > > @@ -69,6 +71,8 @@ constexpr Transform operator~(Transform t)\n> > > > }\n> > > >\n> > > > Transform transformFromRotation(int angle, bool *success = nullptr);\n> > > > +Transform transformFromOrientation(const Orientation &orientation);\n> > > > +Orientation transformToOrientation(const Transform &transform);\n> > > >\n> > > > const char *transformToString(Transform t);\n> > > >\n> > > > diff --git a/src/libcamera/transform.cpp b/src/libcamera/transform.cpp\n> > > > index 4668303d0676..c70cac3f14ee 100644\n> > > > --- a/src/libcamera/transform.cpp\n> > > > +++ b/src/libcamera/transform.cpp\n> > > > @@ -7,6 +7,8 @@\n> > > >\n> > > > #include \n> > > >\n> > > > +#include \n> > > > +\n> > > > /**\n> > > > * \\file transform.h\n> > > > * \\brief Enum to represent and manipulate 2D plane transforms\n> > > > @@ -299,6 +301,64 @@ Transform transformFromRotation(int angle, bool *success)\n> > > > return Transform::Identity;\n> > > > }\n> > > >\n> > > > +/**\n> > > > + * \\brief Return the transform representing \\a orientation\n> > > > + * \\param[in] orientation The orientation to convert\n> > > > + * \\return The transform corresponding to \\a orientation\n> > > > + */\n> > >\n> > > I don't like these functions much, as orientations and transforms are\n> > > not intrinsincly convertible. There's no cardinal concept such as \"a\n> > > transform representing an orientation\".\n> > >\n> >\n> > Are we sure ? Orientation and Transform describe exactly the same set\n> > of 2d plane transformations, isn't there a 1-to-1 relationship ?\n> >\n> > > Fortunately, transformToOrientation() can easily be dropped on top of this\n> > > series, as one of its callers can go away (see reviews of other patches\n> > > in the series), and the code can then be folded in the other caller,\n> > > `Orientation operator*(const Orientation &o, const Transform &t)`.\n> >\n> > You know, I can't find any comment related to this. Can you provide a\n> > reference ?\n> >\n>\n> Found it in 10/12\n>\n> I think we can remove these two functions by:\n>\n> - Introduce a orientationFromRotation() to replace\n>\n> rotationTransform_ = transformFromRotation(propertyValue, &success);\n>\n> in CameraSensor::initProperties()\n>\n> so that we can remove transformToOrientation() in CameraSensor::computeTransform()\n>\n> - Move the RPi pipeline and IPA to use Orientation and remove\n>\n> params.transform =\n> static_cast(transformFromOrientation(config->orientation));\n>\n> so we can remove transformFromOrientation()\n>\n> - Replace py_transform which uses Transform\n>\n> This is all doable, but I would like to sync with RPi as I presume\n> they have code in their apps which is based on these interfaces\n>\n> CC-ed David and Naush\n\nI don't think we care too much in our C++ apps. When these patches go\nin, we might need to convert between Transforms and Orientations here\nand there but that's easily accomplished using the * operator that we\ndefined, or / (in the other direction).\n\nI don't recall if we thought about the Python world - it's easy to\noverlook. Here we have an API that's in use by users and exposes\nTransforms. We'd have the option either to change those over to\nOrientations and break things, or we could leave the Transforms alone\nand convert them internally. In the worst case one could just convert\nwith a lookup table, but it would be nice to compose orientations and\ntransforms as we can in the C++ world.\n\nDavid\n\n>\n>\n> > >\n> > > Similarly, I would like to drop the call to transformFromOrientation()\n> > > in RPi::CameraData::configureIPA() and replace it there with usage of\n> > > `Transform operator/(const Orientation &o1, const Orientation &o2)`.\n> > >\n> > > This can be done on top, as avoiding the introduction of these two\n> > > functions in the series may cause annoying conflicts when rebasing and\n> > > refactoring.\n> > >\n> > > > +Transform transformFromOrientation(const Orientation &orientation)\n> > > > +{\n> > > > + switch (orientation) {\n> > > > + case Orientation::rotate0:\n> > > > + return Transform::Identity;\n> > > > + case Orientation::rotate0Flip:\n> > > > + return Transform::HFlip;\n> > > > + case Orientation::rotate180:\n> > > > + return Transform::Rot180;\n> > > > + case Orientation::rotate180Flip:\n> > > > + return Transform::VFlip;\n> > > > + case Orientation::rotate90Flip:\n> > > > + return Transform::Transpose;\n> > > > + case Orientation::rotate90:\n> > > > + return Transform::Rot90;\n> > > > + case Orientation::rotate270Flip:\n> > > > + return Transform::Rot180Transpose;\n> > > > + case Orientation::rotate270:\n> > > > + return Transform::Rot270;\n> > > > + }\n> > > > +\n> > > > + return Transform::Identity;\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Return the Orientation representing \\a transform\n> > > > + * \\param[in] transform The transform to convert\n> > > > + * \\return The Orientation corresponding to \\a transform\n> > > > + */\n> > > > +Orientation transformToOrientation(const Transform &transform)\n> > > > +{\n> > > > + switch (transform) {\n> > > > + case Transform::Identity:\n> > > > + return Orientation::rotate0;\n> > > > + case Transform::HFlip:\n> > > > + return Orientation::rotate0Flip;\n> > > > + case Transform::VFlip:\n> > > > + return Orientation::rotate180Flip;\n> > > > + case Transform::Rot180:\n> > > > + return Orientation::rotate180;\n> > > > + case Transform::Transpose:\n> > > > + return Orientation::rotate90Flip;\n> > > > + case Transform::Rot270:\n> > > > + return Orientation::rotate270;\n> > > > + case Transform::Rot90:\n> > > > + return Orientation::rotate90;\n> > > > + case Transform::Rot180Transpose:\n> > > > + return Orientation::rotate270Flip;\n> > > > + }\n> > > > +\n> > > > + return Orientation::rotate0;\n> > > > +}\n> > > > +\n> > > > /**\n> > > > * \\brief Return a character string describing the transform\n> > > > * \\param[in] t The transform to be described.\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 66078BDCBD\n\tfor ;\n\tThu, 19 Oct 2023 13:05:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B25AD6297F;\n\tThu, 19 Oct 2023 15:05:52 +0200 (CEST)","from mail-qv1-xf31.google.com (mail-qv1-xf31.google.com\n\t[IPv6:2607:f8b0:4864:20::f31])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 20CC16055B\n\tfor ;\n\tThu, 19 Oct 2023 15:05:51 +0200 (CEST)","by mail-qv1-xf31.google.com with SMTP id\n\t6a1803df08f44-66d12b3b479so50711506d6.1\n\tfor ;\n\tThu, 19 Oct 2023 06:05:51 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1697720752;\n\tbh=j5hhUg5dWHblz4fHhsFaMqIRxSxSE1jV7CC69G/fpZY=;\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=QooX1qES2ii0FzulFUzlvIPgkb4IpFxiGizOeio7FU74MFOWBf5IE2h2xsHZIWXqo\n\tgn0AkL46OEqJmYHPbUi9VIVYOcz1iwHDdpyXCP3m+iAnCnl1WPeOYSyJ+0+NQMUBfB\n\t/CIDfJoghGQ3Nc+mBLr6bLrMf7nGihHncgnJJpQmlGcvczuHUzZ6Qsqy4Sp2Qy88Gl\n\tN5Y8JMTgeGYLqK8DTqGJxEdySUbFMlr8r8gZnp/gxxGowx3nNFRvRjhVa9DNGR0BMx\n\tv68N1+lWoIbchACr0zKH3KT1KGHy4DvAdjEX6/PAJnuZoKtqJOKe5iUKGMTyLkSdTR\n\tPBr4PiebKv4/A==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1697720750; x=1698325550;\n\tdarn=lists.libcamera.org; \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=apsxnQaY9/tUmr4RBbyp809oGYE0l5yE1CxDO1XGo7c=;\n\tb=PwYy2Lx15W5ME+R/C4W9yqJz2uO0XBZPrAzkh5be+wzM7M3NtSWd8bzzJNDDz6nSdW\n\td9Q0uQd4TiRuQ6VDj6m/GJVAArXx+11i20ZKdtgE/IySrYUZVPicOCdC1WKp/SzXiEg9\n\tswRKRq7m7sIC5xADNT2tjT2nBNKx3H71ySPrEQs6qqer50PHjLbtGqGIvnigHRxNhjGz\n\thwnrbs19fzHAgrC5cE1yE+VKbqDSIFIjFrbDdSG1HxqqwE3xvifjz3CLUtAJvH452j2J\n\tPyRYQ+6EXW/mqL+39Vdks8f7HHoOAU/BgAhd9mAey0gZu1DOYrXjgeR1s0Xk3ObWDseF\n\tVzTA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"PwYy2Lx1\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1697720750; x=1698325550;\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=apsxnQaY9/tUmr4RBbyp809oGYE0l5yE1CxDO1XGo7c=;\n\tb=FfEbMiWvJMpjFSGn3tBvW3hklRkY23Zkdduhxjhl2uFhfuKELGRTeqEt3Na570NLNb\n\tMAAn/Tx5Vq1FRvt3lTdXSSdL9O7D5Uk8RLkMT9N/qd2DkVDUyFj6BB5+4dEpYnRhkq1R\n\tkplkKxtvUlmr7SPhZuJbh9s1GUKXaLIfdjj1/A/Nf3PZgchfxHX0SOk80wO6ALDeZcnC\n\tLGLNKHn9nFvH2gUPJoytFpjN3zA+05F5W5SMBmqcMNPQLxs9asGDS5i0YssK5G+2hRBi\n\toPyy0UtBjfl17sj4Oob2WLxz48dE3cvvqWTWAAP1JO05ye2j22ryZNktZfEduCIPtnJQ\n\tWRGw==","X-Gm-Message-State":"AOJu0YxHgNRlZWfCVMJgynTuztaydcLhUbNOhdFJ6bhkC7b8AMtfzS5j\n\tJxjuLLEWtzhcAit8tpGipqGfwiGrnJnMUP1MBmURrzrAlJ7SEFSz","X-Google-Smtp-Source":"AGHT+IGvQDX0J1WyxdmMguYFuuhbWZC6OwlKXFVuyM6H65krOronzc+swILAqTPbzrDuNL+nX9Ryy1i/pO0GrquLn+U=","X-Received":"by 2002:a05:6214:2aa6:b0:66d:1d92:c692 with SMTP id\n\tjs6-20020a0562142aa600b0066d1d92c692mr2118274qvb.16.1697720749602;\n\tThu, 19 Oct 2023 06:05:49 -0700 (PDT)","MIME-Version":"1.0","References":"<20230901150215.11585-1-jacopo.mondi@ideasonboard.com>\n\t<20230901150215.11585-6-jacopo.mondi@ideasonboard.com>\n\t<20231018222234.GP1512@pendragon.ideasonboard.com>\n\t\n\t","In-Reply-To":"","Date":"Thu, 19 Oct 2023 14:05:38 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v5 05/12] libcamera: transform: Add\n\tfunctions to convert 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":28004,"web_url":"https://patchwork.libcamera.org/comment/28004/","msgid":"<20231019133729.GI14832@pendragon.ideasonboard.com>","date":"2023-10-19T13:37:29","subject":"Re: [libcamera-devel] [PATCH v5 05/12] libcamera: transform: Add\n\tfunctions to convert Orientation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Thu, Oct 19, 2023 at 02:05:38PM +0100, David Plowman wrote:\n> On Thu, 19 Oct 2023 at 13:34, Jacopo Mondi wrote:\n> > On Thu, Oct 19, 2023 at 12:34:08PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > > On Thu, Oct 19, 2023 at 01:22:34AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > > > On Fri, Sep 01, 2023 at 05:02:08PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > > > > Add two helper functions to the transform.cpp file that allows to\n> > > > > convert to and from an Orientation.\n> > > > >\n> > > > > Signed-off-by: Jacopo Mondi \n> > > > > Reviewed-by: David Plowman \n> > > > > ---\n> > > > > include/libcamera/transform.h | 4 +++\n> > > > > src/libcamera/transform.cpp | 60 +++++++++++++++++++++++++++++++++++\n> > > > > 2 files changed, 64 insertions(+)\n> > > > >\n> > > > > diff --git a/include/libcamera/transform.h b/include/libcamera/transform.h\n> > > > > index 2a6838c78369..14f1db0e8572 100644\n> > > > > --- a/include/libcamera/transform.h\n> > > > > +++ b/include/libcamera/transform.h\n> > > > > @@ -11,6 +11,8 @@\n> > > > >\n> > > > > namespace libcamera {\n> > > > >\n> > > > > +enum class Orientation;\n> > > > > +\n> > > > > enum class Transform : int {\n> > > > > Identity = 0,\n> > > > > Rot0 = Identity,\n> > > > > @@ -69,6 +71,8 @@ constexpr Transform operator~(Transform t)\n> > > > > }\n> > > > >\n> > > > > Transform transformFromRotation(int angle, bool *success = nullptr);\n> > > > > +Transform transformFromOrientation(const Orientation &orientation);\n> > > > > +Orientation transformToOrientation(const Transform &transform);\n> > > > >\n> > > > > const char *transformToString(Transform t);\n> > > > >\n> > > > > diff --git a/src/libcamera/transform.cpp b/src/libcamera/transform.cpp\n> > > > > index 4668303d0676..c70cac3f14ee 100644\n> > > > > --- a/src/libcamera/transform.cpp\n> > > > > +++ b/src/libcamera/transform.cpp\n> > > > > @@ -7,6 +7,8 @@\n> > > > >\n> > > > > #include \n> > > > >\n> > > > > +#include \n> > > > > +\n> > > > > /**\n> > > > > * \\file transform.h\n> > > > > * \\brief Enum to represent and manipulate 2D plane transforms\n> > > > > @@ -299,6 +301,64 @@ Transform transformFromRotation(int angle, bool *success)\n> > > > > return Transform::Identity;\n> > > > > }\n> > > > >\n> > > > > +/**\n> > > > > + * \\brief Return the transform representing \\a orientation\n> > > > > + * \\param[in] orientation The orientation to convert\n> > > > > + * \\return The transform corresponding to \\a orientation\n> > > > > + */\n> > > >\n> > > > I don't like these functions much, as orientations and transforms are\n> > > > not intrinsincly convertible. There's no cardinal concept such as \"a\n> > > > transform representing an orientation\".\n> > >\n> > > Are we sure ? Orientation and Transform describe exactly the same set\n> > > of 2d plane transformations, isn't there a 1-to-1 relationship ?\n\nOrientation describes how an image is stored in a buffer, while\nTransform describes how it is processed. I think saying\n\n\tOrientation::Rot0 * Transform::HVFlip == Orientation::Rot180\n\nmakes sense, but saying\n\n\tTransform::HVFlip == Orientation::Rot180\n\ndoesn't.\n\n> > > > Fortunately, transformToOrientation() can easily be dropped on top of this\n> > > > series, as one of its callers can go away (see reviews of other patches\n> > > > in the series), and the code can then be folded in the other caller,\n> > > > `Orientation operator*(const Orientation &o, const Transform &t)`.\n> > >\n> > > You know, I can't find any comment related to this. Can you provide a\n> > > reference ?\n> >\n> > Found it in 10/12\n> >\n> > I think we can remove these two functions by:\n> >\n> > - Introduce a orientationFromRotation() to replace\n> >\n> > rotationTransform_ = transformFromRotation(propertyValue, &success);\n> >\n> > in CameraSensor::initProperties()\n> >\n> > so that we can remove transformToOrientation() in CameraSensor::computeTransform()\n> >\n> > - Move the RPi pipeline and IPA to use Orientation and remove\n> >\n> > params.transform =\n> > static_cast(transformFromOrientation(config->orientation));\n> >\n> > so we can remove transformFromOrientation()\n> >\n> > - Replace py_transform which uses Transform\n> >\n> > This is all doable, but I would like to sync with RPi as I presume\n> > they have code in their apps which is based on these interfaces\n> >\n> > CC-ed David and Naush\n> \n> I don't think we care too much in our C++ apps. When these patches go\n> in, we might need to convert between Transforms and Orientations here\n> and there but that's easily accomplished using the * operator that we\n> defined, or / (in the other direction).\n\nThat's what I had in mind. I'll experiment with removing these two\nfunctions and replacing them with the * and / operators.\n\n> I don't recall if we thought about the Python world - it's easy to\n> overlook.\n\nIndeed. Fortunately we have you and Tomi to remind us about Python when\nneeded :-)\n\n> Here we have an API that's in use by users and exposes\n> Transforms. We'd have the option either to change those over to\n> Orientations and break things, or we could leave the Transforms alone\n> and convert them internally. In the worst case one could just convert\n> with a lookup table, but it would be nice to compose orientations and\n> transforms as we can in the C++ world.\n\nIdeally we should also standardize on Orientation in Python, but if\nthere are clear needs to be able to transform orientations in\napplication code, then exposing a Transform class would also make sense\nas a convenience helper (it's *so* easy to get these things wrong). It\ncould be coded entirely in Python, no need to involve the Python\nbindings there.\n\n> > > > Similarly, I would like to drop the call to transformFromOrientation()\n> > > > in RPi::CameraData::configureIPA() and replace it there with usage of\n> > > > `Transform operator/(const Orientation &o1, const Orientation &o2)`.\n> > > >\n> > > > This can be done on top, as avoiding the introduction of these two\n> > > > functions in the series may cause annoying conflicts when rebasing and\n> > > > refactoring.\n> > > >\n> > > > > +Transform transformFromOrientation(const Orientation &orientation)\n> > > > > +{\n> > > > > + switch (orientation) {\n> > > > > + case Orientation::rotate0:\n> > > > > + return Transform::Identity;\n> > > > > + case Orientation::rotate0Flip:\n> > > > > + return Transform::HFlip;\n> > > > > + case Orientation::rotate180:\n> > > > > + return Transform::Rot180;\n> > > > > + case Orientation::rotate180Flip:\n> > > > > + return Transform::VFlip;\n> > > > > + case Orientation::rotate90Flip:\n> > > > > + return Transform::Transpose;\n> > > > > + case Orientation::rotate90:\n> > > > > + return Transform::Rot90;\n> > > > > + case Orientation::rotate270Flip:\n> > > > > + return Transform::Rot180Transpose;\n> > > > > + case Orientation::rotate270:\n> > > > > + return Transform::Rot270;\n> > > > > + }\n> > > > > +\n> > > > > + return Transform::Identity;\n> > > > > +}\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Return the Orientation representing \\a transform\n> > > > > + * \\param[in] transform The transform to convert\n> > > > > + * \\return The Orientation corresponding to \\a transform\n> > > > > + */\n> > > > > +Orientation transformToOrientation(const Transform &transform)\n> > > > > +{\n> > > > > + switch (transform) {\n> > > > > + case Transform::Identity:\n> > > > > + return Orientation::rotate0;\n> > > > > + case Transform::HFlip:\n> > > > > + return Orientation::rotate0Flip;\n> > > > > + case Transform::VFlip:\n> > > > > + return Orientation::rotate180Flip;\n> > > > > + case Transform::Rot180:\n> > > > > + return Orientation::rotate180;\n> > > > > + case Transform::Transpose:\n> > > > > + return Orientation::rotate90Flip;\n> > > > > + case Transform::Rot270:\n> > > > > + return Orientation::rotate270;\n> > > > > + case Transform::Rot90:\n> > > > > + return Orientation::rotate90;\n> > > > > + case Transform::Rot180Transpose:\n> > > > > + return Orientation::rotate270Flip;\n> > > > > + }\n> > > > > +\n> > > > > + return Orientation::rotate0;\n> > > > > +}\n> > > > > +\n> > > > > /**\n> > > > > * \\brief Return a character string describing the transform\n> > > > > * \\param[in] t The transform to be described.","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 D2DBEBDCBD\n\tfor ;\n\tThu, 19 Oct 2023 13:37:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2B9B362980;\n\tThu, 19 Oct 2023 15:37:24 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4A98161DD2\n\tfor ;\n\tThu, 19 Oct 2023 15:37:23 +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 0EE5425A;\n\tThu, 19 Oct 2023 15:37:15 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1697722644;\n\tbh=dVhZYnrix0m7pJBjoGMyIqsfP2KtjbkRZFTXX7Szq1M=;\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=Kfa9INv2VWIDB2AIXOjYaACTFC4p2VbfVQQIMHQuNGOMN7JOotrDeG/Bfa6AfCbGt\n\tp9qeK/G7Ee6WstkLXfpLg5aR5sCIopPU90sYp9eJ6YkCUf2pi9aRUtEvV7+5Zx8Wri\n\tTaVuzWqXy8y9GF/tvojpwUk/Hc6tEJ0ccbuTOl7NLcPLYSHl21sctErqHvy3xlVGtB\n\tTfYuN02JEz8gDzbnB2r9iT6j04Uwe7MHcAeRB+sOepY40DHjHXunWqQ7esqYohrzmm\n\tfMIRaPc8cTBoezwyjaLCmH/uXVWUm46hI3sY+BkZHqJ/pcY4yPGfBElWz66eoyzIrO\n\tAJaoJWx4OslEw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1697722635;\n\tbh=dVhZYnrix0m7pJBjoGMyIqsfP2KtjbkRZFTXX7Szq1M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AbXRcajE+gzjd65w81hwusBPCKyEd20yCtuv9YiA57FH4SC5Ar4raVul62IGLnKkw\n\tKkzZXtmpsV8+jTytsgpUC9GP0ITL2jzjAt7TMAfwpOO7mH/cwGuUp75Ec6+CItt0JM\n\tDd2mg2rvhWBRTP5AGY5zyOHwuiYlnrbcB/bqGaZQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"AbXRcajE\"; dkim-atps=neutral","Date":"Thu, 19 Oct 2023 16:37:29 +0300","To":"David Plowman ","Message-ID":"<20231019133729.GI14832@pendragon.ideasonboard.com>","References":"<20230901150215.11585-1-jacopo.mondi@ideasonboard.com>\n\t<20230901150215.11585-6-jacopo.mondi@ideasonboard.com>\n\t<20231018222234.GP1512@pendragon.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 v5 05/12] libcamera: transform: Add\n\tfunctions to convert 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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]