[{"id":12092,"web_url":"https://patchwork.libcamera.org/comment/12092/","msgid":"<20200823011714.GB25161@pendragon.ideasonboard.com>","date":"2020-08-23T01:17:14","subject":"Re: [libcamera-devel] [PATCH v3 1/5] libcamera: Add Transform enum\n\tto represet 2D plane transforms.","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nThank you for the patch.\n\ns/represet/represent/ in the subject line.\n\nOn Fri, Aug 21, 2020 at 04:56:37PM +0100, David Plowman wrote:\n> We implement 2D transforms as an enum class with 8 elements,\n> consisting of the usual 2D plane transformations (flips, rotations\n> etc.).\n> \n> The transform is made up of 3 bits, indicating whether the transform\n> includes: a transpose, a horizontal flip (mirror) and a vertical flip.\n> \n> Signed-off-by: David Plowman \n> ---\n> include/libcamera/meson.build | 1 +\n> include/libcamera/transform.h | 73 +++++++++\n> src/libcamera/meson.build | 1 +\n> src/libcamera/transform.cpp | 301 ++++++++++++++++++++++++++++++++++\n> 4 files changed, 376 insertions(+)\n> create mode 100644 include/libcamera/transform.h\n> create mode 100644 src/libcamera/transform.cpp\n> \n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index cdb8e03..7fae5e5 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -19,6 +19,7 @@ libcamera_public_headers = files([\n> 'span.h',\n> 'stream.h',\n> 'timer.h',\n> + 'transform.h',\n> ])\n> \n> include_dir = join_paths(libcamera_include_dir, 'libcamera')\n> diff --git a/include/libcamera/transform.h b/include/libcamera/transform.h\n> new file mode 100644\n> index 0000000..7d88937\n> --- /dev/null\n> +++ b/include/libcamera/transform.h\n> @@ -0,0 +1,73 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Raspberry Pi (Trading) Limited\n> + *\n> + * transform.h - 2D plane transforms\n> + */\n> +\n> +#ifndef __LIBCAMERA_TRANSFORM_H__\n> +#define __LIBCAMERA_TRANSFORM_H__\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +enum class Transform : int {\n> +\tIdentity = 0,\n> +\tRot0 = Identity,\n> +\tHFlip = 1,\n> +\tVFlip = 2,\n> +\tHVFlip = HFlip | VFlip,\n> +\tRot180 = HVFlip,\n> +\tTranspose = 4,\n> +\tRot270 = HFlip | Transpose,\n> +\tRot90 = VFlip | Transpose,\n> +\tRot180Transpose = HFlip | VFlip | Transpose\n> +};\n> +\n> +constexpr Transform operator&(Transform t0, Transform t1)\n> +{\n> +\treturn static_cast(static_cast(t0) & static_cast(t1));\n> +}\n> +\n> +constexpr Transform operator|(Transform t0, Transform t1)\n> +{\n> +\treturn static_cast(static_cast(t0) | static_cast(t1));\n> +}\n> +\n> +constexpr Transform operator^(Transform t0, Transform t1)\n> +{\n> +\treturn static_cast(static_cast(t0) ^ static_cast(t1));\n> +}\n> +\n> +constexpr Transform &operator&=(Transform &t0, Transform t1)\n> +{\n> +\treturn t0 = t0 & t1;\n> +}\n> +\n> +constexpr Transform &operator|=(Transform &t0, Transform t1)\n> +{\n> +\treturn t0 = t0 | t1;\n> +}\n> +\n> +constexpr Transform &operator^=(Transform &t0, Transform t1)\n> +{\n> +\treturn t0 = t0 ^ t1;\n> +}\n> +\n> +Transform operator*(Transform t0, Transform t1);\n> +\n> +Transform operator-(Transform t);\n> +\n> +constexpr bool operator!(Transform t)\n> +{\n> +\treturn t == Transform::Identity;\n> +}\n> +\n> +Transform transformFromRotation(int angle, bool *success = nullptr);\n> +\n> +const char *transformToString(Transform t);\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_TRANSFORM_H__ */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index af2f3d9..edec55e 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -44,6 +44,7 @@ libcamera_sources = files([\n> 'sysfs.cpp',\n> 'thread.cpp',\n> 'timer.cpp',\n> + 'transform.cpp',\n> 'utils.cpp',\n> 'v4l2_controls.cpp',\n> 'v4l2_device.cpp',\n> diff --git a/src/libcamera/transform.cpp b/src/libcamera/transform.cpp\n> new file mode 100644\n> index 0000000..2944efc\n> --- /dev/null\n> +++ b/src/libcamera/transform.cpp\n> @@ -0,0 +1,301 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Raspberry Pi (Trading) Limited\n> + *\n> + * transform.cpp - 2D plane transforms.\n\nNitpicking, no need for a period at the end here, or at the end of\n\\brief and \\param statements below.\n\n> + */\n> +\n> +#include \n> +\n> +/**\n> + * \\file transform.h\n> + * \\brief Enum to represent and manipulate 2D plane transforms.\n> + */\n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\enum Transform\n> + * \\brief Enum to represent a 2D plane transform.\n> + *\n> + * The Transform can take 8 distinct values, representing the usual 2D plane\n> + * transforms listed below.\n\nI'm now used to this, but a reader may not find this \"usual\". How about\nlinking to an external resource, such as\nhttps://en.wikipedia.org/wiki/Dihedral_group_of_order_8#The_symmetry_group_of_a_square:_dihedral_group_of_order_8\n? Maybe at the end of this documentation block with a \\sa tag ? But that\nresource may be confusing though, as it defines the symmetry group based\non a combination of Identity, Rot90 and HFlip operations, while we use a\nbinary encoding where bits correspond to HFlip, VFlip and Transpose.\n\n> Each of these transforms can be constructed\n> + * out of 3 atomic operations, namely a horizontal flip (mirror), a vertical\n> + * flip, and a transposition (about the main diagonal). The transforms are\n> + * encoded such that a single bit indicates the presence of each of the 3\n> + * atomic operations:\n> + *\n> + * bit 0 - presence of a horizontal flip\\n\n> + * bit 1 - presence of a vertical flip\\n\n> + * bit 2 - presence of a transposition.\n\nMaybe this should be a list ?\n\n * - bit 0 - presence of a horizontal flip\n * - bit 1 - presence of a vertical flip\n * - bit 2 - presence of a transposition\n\n> + *\n> + * We regard these 3 atomic operations as being applied in a specific order:\n> + * first the two flip operations (actually they commute, so the order between\n> + * them is unimportant) and finally any transpose operation.\n> + *\n> + * Functions are provided to manipulate directly the bits within the transform\n> + * encoding, but there are also higher-level functions to invert and compose\n> + * transforms. Transforms are composed according to the usual mathematical\n> + * convention such that the right transform is applied first, and the left\n> + * transform is applied second.\n> + *\n> + * Finally, we have a total of 8 distinct transformations, as follows (a\n> + * couple of them have additional synonyms for convenience). We illustrate each\n> + * with its nominal effect on a rectangle with vertices labelled A, B, C and D.\n> + *\n> + * **Identity**\n> + *\n> + * Identity transform.\n> +~~~\n> + A-B A-B\n> +Input image | | goes to output image | |\n> + C-D C-D\n> +~~~\n\nFor a minute I've wondered if we could render something more visual,\nsuch as\n\n+-----\n| -----+----+\n+--- | |\n| |\n|\n\n(for a 90° rotation), but I think it's too much hassle for now. We may\nconvert some diagrams to SVG in the documentation in the future, in\nwhich case we'll improve this, but for now I think it's OK as-is (unless\nyou believe it would be good to change the ascii-art representation\nalready).\n\n> + * Numeric value: 0 (no bits set).\n> + *\n> + * **Rot0**\n> + *\n> + * Synonym for `Identity` (zero degree rotation).\n> + *\n> + * **HFlip**\n> + *\n> + * Horizontal flip.\n> +~~~\n> + A-B B-A\n> +Input image | | goes to output image | |\n> + C-D D-C\n> +~~~\n> + * Numeric value: 1 (horizontal flip bit set only).\n> + *\n> + * **VFlip**\n> + *\n> + * Vertical flip.\n> +~~~\n> + A-B C-D\n> +Input image | | goes to output image | |\n> + C-D A-B\n> +~~~\n> + * Numeric value: 2 (vertical flip bit set only).\n> + *\n> + * **HVFlip**\n> + *\n> + * Horizontal and vertical flip (identical to a 180 degree rotation).\n> +~~~\n> + A-B D-C\n> +Input image | | goes to output image | |\n> + C-D B-A\n> +~~~\n> + * Numeric value: 3 (horizontal and vertical flip bits set).\n> + *\n> + * **Rot180**\n> + *\n> + * Synonym for `HVFlip` (180 degree rotation).\n> + *\n> + * **Transpose**\n> + *\n> + * Transpose (about the main diagonal).\n> +~~~\n> + A-B A-C\n> +Input image | | goes to output image | |\n> + C-D B-D\n> +~~~\n> + * Numeric value: 4 (transpose bit set only).\n> + *\n> + * **Rot270**\n> + *\n> + * Rotation by 270 degrees clockwise (90 degrees anticlockwise).\n> +~~~\n> + A-B B-D\n> +Input image | | goes to output image | |\n> + C-D A-C\n> +~~~\n> + * Numeric value: 5 (transpose and horizontal flip bits set).\n> + *\n> + * **Rot90**\n> + *\n> + * Rotation by 90 degrees clockwise (270 degrees anticlockwise).\n> +~~~\n> + A-B C-A\n> +Input image | | goes to output image | |\n> + C-D D-B\n> +~~~\n> + * Numeric value: 6 (transpose and vertical flip bits set).\n> + *\n> + * **Rot180Transpose**\n> + *\n> + * Rotation by 180 degrees followed by transpose (alternatively, transposition\n> + * about the \"opposite diagonal\").\n> +~~~\n> + A-B D-B\n> +Input image | | goes to output image | |\n> + C-D C-A\n> +~~~\n> + * Numeric value: 7 (all bits set).\n> + */\n> +\n> +/**\n> + * \\fn operator &(Transform t0, Transform t1)\n> + * \\brief Apply bitwise AND operator between the bits in the two transforms.\n> + * \\param[in] t0 The first transform.\n> + * \\param[in] t1 The second transform.\n> + */\n> +\n> +/**\n> + * \\fn operator |(Transform t0, Transform t1)\n> + * \\brief Apply bitwise OR operator between the bits in the two transforms.\n> + * \\param[in] t0 The first transform.\n> + * \\param[in] t1 The second transform.\n> + */\n> +\n> +/**\n> + * \\fn operator ^(Transform t0, Transform t1)\n> + * \\brief Apply bitwise XOR operator between the bits in the two transforms.\n> + * \\param[in] t0 The first transform.\n> + * \\param[in] t1 The second transform.\n> + */\n> +\n> +/**\n> + * \\fn operator &=(Transform &t0, Transform t1)\n> + * \\brief Apply bitwise AND-assignment operator between the bits in the two\n> + * transforms.\n> + * \\param[in] t0 The first transform.\n> + * \\param[in] t1 The second transform.\n> + */\n> +\n> +/**\n> + * \\fn operator |=(Transform &t0, Transform t1)\n> + * \\brief Apply bitwise OR-assignment operator between the bits in the two\n> + * transforms.\n> + * \\param[in] t0 The first transform.\n> + * \\param[in] t1 The second transform.\n> + */\n> +\n> +/**\n> + * \\fn operator ^=(Transform &t0, Transform t1)\n> + * \\brief Apply bitwise XOR-assignment operator between the bits in the two\n> + * transforms.\n> + * \\param[in] t0 The first transform.\n> + * \\param[in] t1 The second transform.\n> + */\n> +\n> +/**\n> + * \\brief Compose two transforms together. t1 is applied first, then t0.\n\nYou can use \\a t1 and \\a t0 to refer to the parameters, it will\nhighlight them.\n\n> + * \\param[in] t0 The first transform.\n> + * \\param[in] t1 The second transform.\n\nt1 is apply first, then t0, yet t0 is the first and t1 the second\ntransform. How about\n\n * \\brief Compose two transforms together. t0 is applied first, then t1.\n * \\param[in] t1 The second transform\n * \\param[in] t0 The first transform\n *...\n */\nTransform operator*(Transform t1, Transform t0)\n\n> + *\n> + * For example, `Transpose * HFlip` performs `HFlip` first and then the\n> + * `Transpose` yielding `Rot270`, as shown below.\n\nI'd move the second sentence from the brief here:\n\n * Composing transforms follows the mathematical function composition notation.\n * When performing t1 * t0, \\a t0 is applied first, then \\a t1. For example,\n * `Transpose * HFlip` performs `HFlip` first and then the `Transpose` yielding\n * `Rot270`, as shown below.\n\n> +~~~\n> + A-B B-A B-D\n> +Input image | | -> HFLip -> | | -> Transpose -> | | = Rot270\n> + C-D D-C A-C\n> +~~~\n> + * Note that composition is generally non-commutative for Transforms,\n> + * and not the same as XOR-ing the underlying bit representations.\n> + */\n> +Transform operator*(Transform t0, Transform t1)\n> +{\n> +\t/*\n> +\t * Reorder the operations so that we imagine doing t1's transpose\n> +\t * (if any) after t0's flips. The effect is to swap t0's hflips for\n> +\t * vflips and vice versa, after which we can just xor all the bits.\n> +\t */\n> +\tTransform reordered = t0;\n> +\tif (!!(t1 & Transform::Transpose)) {\n\nIt would be nice to have a bool conversion function to be able to write\n\n\tif (t1 & Transform::Transpose) {\n\nIf we turn the enum into a class one day... :-)\n\nWith these small issues addressed,\n\nReviewed-by: Laurent Pinchart \n\n> +\t\treordered = t0 & Transform::Transpose;\n> +\t\tif (!!(t0 & Transform::HFlip))\n> +\t\t\treordered |= Transform::VFlip;\n> +\t\tif (!!(t0 & Transform::VFlip))\n> +\t\t\treordered |= Transform::HFlip;\n> +\t}\n> +\n> +\treturn reordered ^ t1;\n> +}\n> +\n> +/**\n> + * \\brief Invert a transform.\n> + * \\param[in] t The transform to be inverted.\n> + *\n> + * That is, we return the transform such that `t * (-t)` and `(-t) * t` both\n> + * yield the identity transform.\n> + */\n> +Transform operator-(Transform t)\n> +{\n> +\t/* All are self-inverses, except for Rot270 and Rot90. */\n> +\tstatic const Transform inverses[] = {\n> +\t\tTransform::Identity,\n> +\t\tTransform::HFlip,\n> +\t\tTransform::VFlip,\n> +\t\tTransform::HVFlip,\n> +\t\tTransform::Transpose,\n> +\t\tTransform::Rot90,\n> +\t\tTransform::Rot270,\n> +\t\tTransform::Rot180Transpose\n> +\t};\n> +\n> +\treturn inverses[static_cast(t)];\n> +}\n> +\n> +/**\n> + * \\fn operator!(Transform t)\n> + * \\brief Return `true` if the transform is the `Identity`, otherwise `false`.\n> + * \\param[in] t The transform to be tested.\n> + */\n> +\n> +/**\n> + * \\brief Return the transform representing a rotation of the given angle\n> + * clockwise.\n> + * \\param[in] angle The angle of rotation in a clockwise sense. Negative values\n> + * can be used to represent anticlockwise rotations.\n> + * \\param[out] success Set to `true` if the angle is a multiple of 90 degrees,\n> + * otherwise `false`.\n> + * \\return The transform corresponding to the rotation if success was set to\n> + * `true`, otherwise the `Identity` transform.\n> + */\n> +Transform transformFromRotation(int angle, bool *success)\n> +{\n> +\tangle = angle % 360;\n> +\tif (angle < 0)\n> +\t\tangle += 360;\n> +\n> +\tif (success != nullptr)\n> +\t\t*success = true;\n> +\n> +\tswitch (angle) {\n> +\tcase 0:\n> +\t\treturn Transform::Identity;\n> +\tcase 90:\n> +\t\treturn Transform::Rot90;\n> +\tcase 180:\n> +\t\treturn Transform::Rot180;\n> +\tcase 270:\n> +\t\treturn Transform::Rot270;\n> +\t}\n> +\n> +\tif (success != nullptr)\n> +\t\t*success = false;\n> +\n> +\treturn Transform::Identity;\n> +}\n> +\n> +/**\n> + * \\brief Return a character string describing the transform.\n> + * \\param[in] t The transform to be described.\n> + */\n> +const char *transformToString(Transform t)\n> +{\n> +\tstatic const char *strings[] = {\n> +\t\t\"identity\",\n> +\t\t\"hflip\",\n> +\t\t\"vflip\",\n> +\t\t\"hvflip\",\n> +\t\t\"transpose\",\n> +\t\t\"rot270\",\n> +\t\t\"rot90\",\n> +\t\t\"rot180transpose\"\n> +\t};\n> +\n> +\treturn strings[static_cast(t)];\n> +}\n> +\n> +} /* namespace libcamera */","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 73AB8BD87C\n\tfor ;\n\tSun, 23 Aug 2020 01:17:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ECEF6626DB;\n\tSun, 23 Aug 2020 03:17:34 +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 9F7AE60383\n\tfor ;\n\tSun, 23 Aug 2020 03:17:33 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 86A50276;\n\tSun, 23 Aug 2020 03:17:32 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"WdALDGaJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1598145452;\n\tbh=ajvooR27J0ZKGMu/gyKFyb4JP8IXuo/1vOGWXgNHG24=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WdALDGaJcKCR1w5EKhmunGEHFVLp7+1VHZzbJOMXwuQzF2vX3OAdEY3G/tex91O/d\n\tOJJ6brSG9QA13b4wBT3VyWiGtSwFR2m29UdZgc70sen771YlvIasHJLnzc76eBdRDc\n\twXnPigPajgVtOIxqBejeRRJia3GQDOkq4Qnzdr20=","Date":"Sun, 23 Aug 2020 04:17:14 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"<20200823011714.GB25161@pendragon.ideasonboard.com>","References":"<20200821155641.11839-1-david.plowman@raspberrypi.com>\n\t<20200821155641.11839-2-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200821155641.11839-2-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v3 1/5] libcamera: Add Transform enum\n\tto represet 2D plane transforms.","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]