[{"id":11719,"web_url":"https://patchwork.libcamera.org/comment/11719/","msgid":"<20200729151234.GG6183@pendragon.ideasonboard.com>","date":"2020-07-29T15:12:34","subject":"Re: [libcamera-devel] [PATCH 1/5] libcamera: Add Transform class","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\nOn Wed, Jul 29, 2020 at 10:31:50AM +0100, David Plowman wrote:\n> Add implementation of 2d plane transforms, and include a Transform\n> field in the CameraConfiguration to record the application's choice of\n> image transform for this session.\n> ---\n> include/libcamera/camera.h | 3 +\n> include/libcamera/meson.build | 1 +\n> include/libcamera/transform.h | 193 ++++++++++++++++++++++++++++++++++\n> 3 files changed, 197 insertions(+)\n> create mode 100644 include/libcamera/transform.h\n> \n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index 4d1a4a9..fd9355e 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -16,6 +16,7 @@\n> #include \n> #include \n> #include \n> +#include \n> \n> namespace libcamera {\n> \n> @@ -60,6 +61,8 @@ public:\n> \tbool empty() const;\n> \tstd::size_t size() const;\n> \n> +\tTransform userTransform;\n> +\n> protected:\n> \tCameraConfiguration();\n> \n\nThis needs to be documented in the CameraConfiguration class. Didn't\ndoxygen warn you ? I would also split this to a separate patch, and\nupdate the existing pipeline handlers to always set userTransform to\nIdentity in their validate() function.\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..cfd5026\n> --- /dev/null\n> +++ b/include/libcamera/transform.h\n> @@ -0,0 +1,193 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n\nAs mentioned in the reply to the cover letter, this should have your\ncopyright.\n\n> + *\n> + * transform.h - Implementation of 2d plane transforms\n\n'2d' or '2D' ?\n\n> + */\n> +\n> +#ifndef __LIBCAMERA_TRANSFORM_H__\n> +#define __LIBCAMERA_TRANSFORM_H__\n> +\n> +#include \n\nYou should also include stdint.h for int32_t, although I think you could\nuse int instead of int32_t.\n\n> +\n> +namespace libcamera {\n> +\n> +class Transform\n> +{\n> +public:\n> +\tconstexpr Transform()\n> +\t\t: Transform(Identity)\n> +\t{\n> +\t}\n> +\n> +\tconstexpr static Transform identity()\n> +\t{\n> +\t\treturn Transform(Identity);\n> +\t}\n> +\n> +\tconstexpr static Transform rot0()\n> +\t{\n> +\t\treturn identity();\n> +\t}\n> +\n> +\tconstexpr static Transform hflip()\n> +\t{\n> +\t\treturn Transform(HFlip);\n> +\t}\n> +\n> +\tconstexpr static Transform vflip()\n> +\t{\n> +\t\treturn Transform(VFlip);\n> +\t}\n> +\n> +\tconstexpr static Transform hvflip()\n> +\t{\n> +\t\treturn Transform(HVFlip);\n> +\t}\n> +\n> +\tconstexpr static Transform rot180()\n> +\t{\n> +\t\treturn hvflip();\n> +\t}\n> +\n> +\tconstexpr static Transform transpose()\n> +\t{\n> +\t\treturn Transform(Transpose);\n> +\t}\n> +\n> +\tconstexpr static Transform rot270()\n> +\t{\n> +\t\treturn Transform(Rot270);\n> +\t}\n> +\n> +\tconstexpr static Transform rot90()\n> +\t{\n> +\t\treturn Transform(Rot90);\n> +\t}\n> +\n> +\tconstexpr static Transform rot180Transpose()\n> +\t{\n> +\t\treturn Transform(Rot180Transpose);\n> +\t}\n> +\n> +\tconstexpr static Transform hvflipTranspose()\n> +\t{\n> +\t\treturn rot180Transpose();\n> +\t}\n> +\n\nWouldn't it be simpler to make the Type enum public, as well as the\nconstructor that takes a Type ? I will also be way less code to type :-)\n\n> +\tconstexpr static bool rotation(int32_t angle, Transform &xfm)\n> +\t{\n> +\t\tbool success = true;\n> +\t\tangle = angle % 360;\n> +\t\tif (angle < 0)\n> +\t\t\tangle += 360;\n> +\n> +\t\tif (angle == 0)\n> +\t\t\txfm = identity();\n> +\t\telse if (angle == 90)\n> +\t\t\txfm = rot90();\n> +\t\telse if (angle == 180)\n> +\t\t\txfm = rot180();\n> +\t\telse if (angle == 270)\n> +\t\t\txfm = rot270();\n> +\t\telse\n> +\t\t\tsuccess = false;\n> +\n> +\t\treturn success;\n> +\t}\n> +\n> +\tconstexpr bool rotation(int32_t &angle) const\n> +\t{\n> +\t\tbool success = true;\n> +\n> +\t\tif (type_ == Identity)\n> +\t\t\tangle = 0;\n> +\t\telse if (type_ == Rot90)\n> +\t\t\tangle = 90;\n> +\t\telse if (type_ == HVFlip)\n> +\t\t\tangle = 180;\n> +\t\telse if (type_ == Rot270)\n> +\t\t\tangle = 270;\n> +\t\telse\n> +\t\t\tsuccess = false;\n> +\n> +\t\treturn success;\n> +\t}\n\nThat's two \"rotation\" functions, one applying a rotation, the other one\nreturning the rotation. I think it's a bit confusing. I would turn the\nformer into\n\n\tbool rotate(int angle);\n\nand the later into\n\n\tconstexpr unsigned int rotationAngle(bool *success = nullptr) const;\n\nI also think the two functions should be implemented in the .cpp file.\n\n> +\n> +\tconstexpr bool contains(Transform xfm) const\n> +\t{\n> +\t\treturn (type_ & xfm.type_) == xfm.type_;\n> +\t}\n\nThis function is used in the rest of the series to check if a\ntransformation contains hflip, vflip or transpose. For that use case\nit's fine, but it could also be used with\n\n\tTransform::rot180Transpose().contains(Transform::rot270())\n\nand return true, which is quite confusing to read. I'm not sure how to\nimprove that though. Maybe defining three functions, to test for hflip,\nvflip and transpose separately ? isHorizontallyFlipped(),\nisVerticallyFlipped, isTransposed ? Or isHFlipped(), isVFlipped() for\nthe first two ?\n\n> +\n> +\tconstexpr Transform inverse() const\n> +\t{\n> +\t\t/* They're all self-inverses, except for Rot90 and Rot270. */\n> +\t\tconst Type inverses[] = { Identity, HFlip, VFlip, HVFlip,\n> +\t\t\t\t\t Transpose, Rot90, Rot270, Rot180Transpose };\n\nShouldn't this be static const ?\n\n> +\n> +\t\treturn Transform(inverses[type_]);\n> +\t}\n> +\n> +\tconstexpr Transform operator*(Transform xfm) const\n> +\t{\n> +\t\t/*\n> +\t\t * Reorder the operations so that we imagine doing xfm's transpose\n> +\t\t * (if any) after our flips. The effect is to swap our hflips for\n> +\t\t * vflips and vice versa, after which we can just xor all the bits.\n> +\t\t */\n> +\t\tint reorderedType = type_;\n> +\t\tif (xfm.type_ & Transpose)\n> +\t\t\treorderedType = (type_ & 4) | ((type_ & 1) << 1) | ((type_ & 2) >> 1);\n> +\n> +\t\treturn Transform((Type)(reorderedType ^ xfm.type_));\n> +\t}\n\nI would move the implementation of these two functions to the .cpp file\ntoo.\n\n> +\n> +\tbool operator==(Transform xfm) const\n> +\t{\n> +\t\treturn type_ == xfm.type_;\n> +\t}\n> +\n> +\tbool operator!=(Transform xfm) const\n> +\t{\n> +\t\treturn !(*this == xfm);\n> +\t}\n> +\n> +\tstd::string toString() const\n> +\t{\n> +\t\tchar const *strings[] = {\n\nstatic const char ?\n\n> +\t\t\t\"identity\",\n> +\t\t\t\"hflip\",\n> +\t\t\t\"vflip\",\n> +\t\t\t\"hvflip\",\n> +\t\t\t\"transpose\",\n> +\t\t\t\"rot270\",\n> +\t\t\t\"rot90\",\n> +\t\t\t\"rot180transpose\"\n> +\t\t};\n> +\t\treturn strings[type_];\n> +\t}\n\nOf course all this needs documentation :-) A good introduction (in the\n\\class Transform block) would be very useful to explain the concepts I\nthink.\n\n> +\n> +private:\n> +\tenum Type : int {\n> +\t\tIdentity = 0,\n> +\t\tHFlip = 1,\n> +\t\tVFlip = 2,\n> +\t\tHVFlip = HFlip | VFlip,\n> +\t\tTranspose = 4,\n> +\t\tRot270 = HFlip | Transpose,\n> +\t\tRot90 = VFlip | Transpose,\n> +\t\tRot180Transpose = HFlip | VFlip | Transpose\n> +\t};\n> +\n> +\tconstexpr Transform(Type type)\n> +\t\t: type_(type)\n> +\t{\n> +\t}\n> +\n> +\tType type_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_TRANSFORM_H__ */\n> +\n\nExtra blank line.","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 B561EBD86F\n\tfor ;\n\tWed, 29 Jul 2020 15:12:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2F202617AF;\n\tWed, 29 Jul 2020 17:12:45 +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 113E36039D\n\tfor ;\n\tWed, 29 Jul 2020 17:12:44 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7FAEE31F;\n\tWed, 29 Jul 2020 17:12:43 +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=\"odTG8f92\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596035563;\n\tbh=cMoUNlLhKZd1RJLI4ydLrc4b2eayN+mdtSrohMiN0P0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=odTG8f92BTlqPWCDMU7mleleooB2XJnHoN+IGnsRMvpGe+danOeguop9uu3we6Pok\n\t07y8sh7l9LW7bQYrA47whe4VxAC9Zuct6A5y+8rSs2Q0RVizn6lGh94Sme1C0RH+04\n\tFOs2clhntKPEPZczwalvtyrJ00kdYH7qdenfIjwU=","Date":"Wed, 29 Jul 2020 18:12:34 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"<20200729151234.GG6183@pendragon.ideasonboard.com>","References":"<20200729093154.12296-1-david.plowman@raspberrypi.com>\n\t<20200729093154.12296-2-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200729093154.12296-2-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 1/5] libcamera: Add Transform class","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11727,"web_url":"https://patchwork.libcamera.org/comment/11727/","msgid":"","date":"2020-07-29T16:36:16","subject":"Re: [libcamera-devel] [PATCH 1/5] libcamera: Add Transform class","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Laurent\n\nThanks for the feedback. That all looks good, let me just clarify one\nor two points.\n\nOn Wed, 29 Jul 2020 at 16:12, Laurent Pinchart\n wrote:\n>\n> Hi David,\n>\n> Thank you for the patch.\n>\n> On Wed, Jul 29, 2020 at 10:31:50AM +0100, David Plowman wrote:\n> > Add implementation of 2d plane transforms, and include a Transform\n> > field in the CameraConfiguration to record the application's choice of\n> > image transform for this session.\n> > ---\n> > include/libcamera/camera.h | 3 +\n> > include/libcamera/meson.build | 1 +\n> > include/libcamera/transform.h | 193 ++++++++++++++++++++++++++++++++++\n> > 3 files changed, 197 insertions(+)\n> > create mode 100644 include/libcamera/transform.h\n> >\n> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > index 4d1a4a9..fd9355e 100644\n> > --- a/include/libcamera/camera.h\n> > +++ b/include/libcamera/camera.h\n> > @@ -16,6 +16,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> >\n> > namespace libcamera {\n> >\n> > @@ -60,6 +61,8 @@ public:\n> > bool empty() const;\n> > std::size_t size() const;\n> >\n> > + Transform userTransform;\n> > +\n> > protected:\n> > CameraConfiguration();\n> >\n>\n> This needs to be documented in the CameraConfiguration class. Didn't\n> doxygen warn you ? I would also split this to a separate patch, and\n> update the existing pipeline handlers to always set userTransform to\n> Identity in their validate() function.\n\nI wasn't sure I'm qualified to touch existing pipeline handlers, but I\nguess I can do so if necessary! I'm still uncertain whether\n\"adjusting\" the transform is reasonable or whether failing is better.\nIn the Raspberry Pi pipeline handler I felt that adjusting a transform\nto a completely different one isn't a \"reasonable\" change, and the\napplication should know and not carry on with the thing it didn't\nwant. It's not like changing an image size \"a bit\", after all. But I'm\nopen to persuasion...\n\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..cfd5026\n> > --- /dev/null\n> > +++ b/include/libcamera/transform.h\n> > @@ -0,0 +1,193 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Google Inc.\n>\n> As mentioned in the reply to the cover letter, this should have your\n> copyright.\n>\n> > + *\n> > + * transform.h - Implementation of 2d plane transforms\n>\n> '2d' or '2D' ?\n>\n> > + */\n> > +\n> > +#ifndef __LIBCAMERA_TRANSFORM_H__\n> > +#define __LIBCAMERA_TRANSFORM_H__\n> > +\n> > +#include \n>\n> You should also include stdint.h for int32_t, although I think you could\n> use int instead of int32_t.\n>\n> > +\n> > +namespace libcamera {\n> > +\n> > +class Transform\n> > +{\n> > +public:\n> > + constexpr Transform()\n> > + : Transform(Identity)\n> > + {\n> > + }\n> > +\n> > + constexpr static Transform identity()\n> > + {\n> > + return Transform(Identity);\n> > + }\n> > +\n> > + constexpr static Transform rot0()\n> > + {\n> > + return identity();\n> > + }\n> > +\n> > + constexpr static Transform hflip()\n> > + {\n> > + return Transform(HFlip);\n> > + }\n> > +\n> > + constexpr static Transform vflip()\n> > + {\n> > + return Transform(VFlip);\n> > + }\n> > +\n> > + constexpr static Transform hvflip()\n> > + {\n> > + return Transform(HVFlip);\n> > + }\n> > +\n> > + constexpr static Transform rot180()\n> > + {\n> > + return hvflip();\n> > + }\n> > +\n> > + constexpr static Transform transpose()\n> > + {\n> > + return Transform(Transpose);\n> > + }\n> > +\n> > + constexpr static Transform rot270()\n> > + {\n> > + return Transform(Rot270);\n> > + }\n> > +\n> > + constexpr static Transform rot90()\n> > + {\n> > + return Transform(Rot90);\n> > + }\n> > +\n> > + constexpr static Transform rot180Transpose()\n> > + {\n> > + return Transform(Rot180Transpose);\n> > + }\n> > +\n> > + constexpr static Transform hvflipTranspose()\n> > + {\n> > + return rot180Transpose();\n> > + }\n> > +\n>\n> Wouldn't it be simpler to make the Type enum public, as well as the\n> constructor that takes a Type ? I will also be way less code to type :-)\n\nI did wonder about this. I didn't like making the Type public because\neither you use enum class and end up writing stuff like\nTransform(Transform::Type::Identity) instead of Transform::identity(),\nor you could use a straight enum and write\nTransform(Transform::Identity) which I like, but you have to worry\nabout what this means:\nTransform transform = Transform::Identity * Transform::HFlip;\n\nSo I thought typing more in the header file was better than putting\nthe burden on the client code. But I don't feel very strongly...\n\n>\n> > + constexpr static bool rotation(int32_t angle, Transform &xfm)\n> > + {\n> > + bool success = true;\n> > + angle = angle % 360;\n> > + if (angle < 0)\n> > + angle += 360;\n> > +\n> > + if (angle == 0)\n> > + xfm = identity();\n> > + else if (angle == 90)\n> > + xfm = rot90();\n> > + else if (angle == 180)\n> > + xfm = rot180();\n> > + else if (angle == 270)\n> > + xfm = rot270();\n> > + else\n> > + success = false;\n> > +\n> > + return success;\n> > + }\n> > +\n> > + constexpr bool rotation(int32_t &angle) const\n> > + {\n> > + bool success = true;\n> > +\n> > + if (type_ == Identity)\n> > + angle = 0;\n> > + else if (type_ == Rot90)\n> > + angle = 90;\n> > + else if (type_ == HVFlip)\n> > + angle = 180;\n> > + else if (type_ == Rot270)\n> > + angle = 270;\n> > + else\n> > + success = false;\n> > +\n> > + return success;\n> > + }\n>\n> That's two \"rotation\" functions, one applying a rotation, the other one\n> returning the rotation. I think it's a bit confusing. I would turn the\n> former into\n>\n> bool rotate(int angle);\n>\n> and the later into\n>\n> constexpr unsigned int rotationAngle(bool *success = nullptr) const;\n>\n> I also think the two functions should be implemented in the .cpp file.\n\nYes, two functions with the same name was always nasty. The first one\nis actually creating a Transform from a rotation angle, not applying a\nrotation (so obviously documentation will have to be clear on that).\n\nIf we prefer the success code as the optional parameter it could look like:\nconstexpr static Transform rotation(int angle, bool *success = nullptr);\n\nFor the second one I like rotationAngle.\n\n>\n> > +\n> > + constexpr bool contains(Transform xfm) const\n> > + {\n> > + return (type_ & xfm.type_) == xfm.type_;\n> > + }\n>\n> This function is used in the rest of the series to check if a\n> transformation contains hflip, vflip or transpose. For that use case\n> it's fine, but it could also be used with\n>\n> Transform::rot180Transpose().contains(Transform::rot270())\n>\n> and return true, which is quite confusing to read. I'm not sure how to\n> improve that though. Maybe defining three functions, to test for hflip,\n> vflip and transpose separately ? isHorizontallyFlipped(),\n> isVerticallyFlipped, isTransposed ? Or isHFlipped(), isVFlipped() for\n> the first two ?\n\nYep, I could go with isHFlipped(), isVFlipped(), isTransposed().\n\nThanks!\nDavid\n\n>\n> > +\n> > + constexpr Transform inverse() const\n> > + {\n> > + /* They're all self-inverses, except for Rot90 and Rot270. */\n> > + const Type inverses[] = { Identity, HFlip, VFlip, HVFlip,\n> > + Transpose, Rot90, Rot270, Rot180Transpose };\n>\n> Shouldn't this be static const ?\n>\n> > +\n> > + return Transform(inverses[type_]);\n> > + }\n> > +\n> > + constexpr Transform operator*(Transform xfm) const\n> > + {\n> > + /*\n> > + * Reorder the operations so that we imagine doing xfm's transpose\n> > + * (if any) after our flips. The effect is to swap our hflips for\n> > + * vflips and vice versa, after which we can just xor all the bits.\n> > + */\n> > + int reorderedType = type_;\n> > + if (xfm.type_ & Transpose)\n> > + reorderedType = (type_ & 4) | ((type_ & 1) << 1) | ((type_ & 2) >> 1);\n> > +\n> > + return Transform((Type)(reorderedType ^ xfm.type_));\n> > + }\n>\n> I would move the implementation of these two functions to the .cpp file\n> too.\n>\n> > +\n> > + bool operator==(Transform xfm) const\n> > + {\n> > + return type_ == xfm.type_;\n> > + }\n> > +\n> > + bool operator!=(Transform xfm) const\n> > + {\n> > + return !(*this == xfm);\n> > + }\n> > +\n> > + std::string toString() const\n> > + {\n> > + char const *strings[] = {\n>\n> static const char ?\n>\n> > + \"identity\",\n> > + \"hflip\",\n> > + \"vflip\",\n> > + \"hvflip\",\n> > + \"transpose\",\n> > + \"rot270\",\n> > + \"rot90\",\n> > + \"rot180transpose\"\n> > + };\n> > + return strings[type_];\n> > + }\n>\n> Of course all this needs documentation :-) A good introduction (in the\n> \\class Transform block) would be very useful to explain the concepts I\n> think.\n>\n> > +\n> > +private:\n> > + enum Type : int {\n> > + Identity = 0,\n> > + HFlip = 1,\n> > + VFlip = 2,\n> > + HVFlip = HFlip | VFlip,\n> > + Transpose = 4,\n> > + Rot270 = HFlip | Transpose,\n> > + Rot90 = VFlip | Transpose,\n> > + Rot180Transpose = HFlip | VFlip | Transpose\n> > + };\n> > +\n> > + constexpr Transform(Type type)\n> > + : type_(type)\n> > + {\n> > + }\n> > +\n> > + Type type_;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_TRANSFORM_H__ */\n> > +\n>\n> Extra blank line.\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 A5FF0BD878\n\tfor ;\n\tWed, 29 Jul 2020 16:36:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2264A617D6;\n\tWed, 29 Jul 2020 18:36:31 +0200 (CEST)","from mail-ot1-x32a.google.com (mail-ot1-x32a.google.com\n\t[IPv6:2607:f8b0:4864:20::32a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 48E6A60540\n\tfor ;\n\tWed, 29 Jul 2020 18:36:29 +0200 (CEST)","by mail-ot1-x32a.google.com with SMTP id r21so7431209ota.10\n\tfor ;\n\tWed, 29 Jul 2020 09:36:29 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"DJNe9XBU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=PAKexDSwiSqO190OZnJh1pxrHWClJKrNg25LBOWJFH0=;\n\tb=DJNe9XBU6A6vP5HYoT38Etx0Boc9Oet+o/kC4diZoPnPye5nhngCSzwd2HFKAbWrls\n\t3sIcEkJ+YkRUVinY28uUEbrAgrreAiwIsrF8bhaPuFDJaWlEA0/TA5XGG4DYlWPO0SLO\n\t+LHuCgrnrgHL/IgH0WnktW8vgZIaSwVs+HJCgSL+ynpbBsTCxRxA4qy+Em90wFYZWd7w\n\tujq9QRnSkyQnXGTIxaR/2dyTi9RH6GMBIj+DkhXzJNjCr7SpPFCSJIe6X2KbWdTtgVad\n\tFe2DJrSX2bMSuBT3iG96Bgo9h9VdcFeXy6dR5hx5kotF7QsUGndorjiFilpu0Mca3jh1\n\t/aOA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=PAKexDSwiSqO190OZnJh1pxrHWClJKrNg25LBOWJFH0=;\n\tb=R6AoTd2sxWIn6Vr9b5nq1R7bBpPkq+8KHHEJFzLNxH/R1xBZMdMuUh9PNB0sXFaJwO\n\tCZWvaApBGczg5mhMClbjntzzO/jnNFHEYa+aweBJnqFjw1fPPMDFnjpdxqMbz+02y+xy\n\trT93MKrg8hkqxqnG/IyAWH/+UuPF/M8DTRKFcSFFm5CSYUWgmiD2qxD03KGSm5iJC81E\n\tBUBOibOsBIURvWtN0TGkIpzkoXMpz1XVfKDwfh61DKOxkKic2T2gKii9x/GgI3JdTwpc\n\tgCovor4FnJvI8TCVW/Aq82/LfE+0UCC+HsP95hgJvXMi4h6sO5uGoALU8IZm9uaWCFgN\n\tMrkQ==","X-Gm-Message-State":"AOAM531Xiy0iI/hwjuoq1YoVzOHnQwLEfWLUGR4tDxj7CE3K0pB4dojM\n\tvWyeVYsI1oWqJkOzK4WSj/fuErnDmGGMBnniA77Wlw==","X-Google-Smtp-Source":"ABdhPJweDxTQvCD/OHdg1UtMGLn9kumhLh5QlLDaTFtUZnKKWyQMvs/lBzblLiKaXsy98VB1Mdqh/44DYQDNs2QPdjE=","X-Received":"by 2002:a05:6830:14d4:: with SMTP id\n\tt20mr28683287otq.166.1596040587489; \n\tWed, 29 Jul 2020 09:36:27 -0700 (PDT)","MIME-Version":"1.0","References":"<20200729093154.12296-1-david.plowman@raspberrypi.com>\n\t<20200729093154.12296-2-david.plowman@raspberrypi.com>\n\t<20200729151234.GG6183@pendragon.ideasonboard.com>","In-Reply-To":"<20200729151234.GG6183@pendragon.ideasonboard.com>","From":"David Plowman ","Date":"Wed, 29 Jul 2020 17:36:16 +0100","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH 1/5] libcamera: Add Transform class","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11730,"web_url":"https://patchwork.libcamera.org/comment/11730/","msgid":"<20200729164947.GM6183@pendragon.ideasonboard.com>","date":"2020-07-29T16:49:47","subject":"Re: [libcamera-devel] [PATCH 1/5] libcamera: Add Transform class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nOn Wed, Jul 29, 2020 at 05:36:16PM +0100, David Plowman wrote:\n> Hi Laurent\n> \n> Thanks for the feedback. That all looks good, let me just clarify one\n> or two points.\n> \n> On Wed, 29 Jul 2020 at 16:12, Laurent Pinchart wrote:\n> > On Wed, Jul 29, 2020 at 10:31:50AM +0100, David Plowman wrote:\n> > > Add implementation of 2d plane transforms, and include a Transform\n> > > field in the CameraConfiguration to record the application's choice of\n> > > image transform for this session.\n> > > ---\n> > > include/libcamera/camera.h | 3 +\n> > > include/libcamera/meson.build | 1 +\n> > > include/libcamera/transform.h | 193 ++++++++++++++++++++++++++++++++++\n> > > 3 files changed, 197 insertions(+)\n> > > create mode 100644 include/libcamera/transform.h\n> > >\n> > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > > index 4d1a4a9..fd9355e 100644\n> > > --- a/include/libcamera/camera.h\n> > > +++ b/include/libcamera/camera.h\n> > > @@ -16,6 +16,7 @@\n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> > >\n> > > namespace libcamera {\n> > >\n> > > @@ -60,6 +61,8 @@ public:\n> > > bool empty() const;\n> > > std::size_t size() const;\n> > >\n> > > + Transform userTransform;\n> > > +\n> > > protected:\n> > > CameraConfiguration();\n> > >\n> >\n> > This needs to be documented in the CameraConfiguration class. Didn't\n> > doxygen warn you ? I would also split this to a separate patch, and\n> > update the existing pipeline handlers to always set userTransform to\n> > Identity in their validate() function.\n> \n> I wasn't sure I'm qualified to touch existing pipeline handlers, but I\n> guess I can do so if necessary! I'm still uncertain whether\n> \"adjusting\" the transform is reasonable or whether failing is better.\n> In the Raspberry Pi pipeline handler I felt that adjusting a transform\n> to a completely different one isn't a \"reasonable\" change, and the\n> application should know and not carry on with the thing it didn't\n> want. It's not like changing an image size \"a bit\", after all. But I'm\n> open to persuasion...\n\nIn that case we would need a mechanism to report the supported\ntransformations, otherwise an application wouldn't know what do to if\nvalidate() fails. And speaking of that, wouldn't such a mechanism make\nsense in any case ?\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..cfd5026\n> > > --- /dev/null\n> > > +++ b/include/libcamera/transform.h\n> > > @@ -0,0 +1,193 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2020, Google Inc.\n> >\n> > As mentioned in the reply to the cover letter, this should have your\n> > copyright.\n> >\n> > > + *\n> > > + * transform.h - Implementation of 2d plane transforms\n> >\n> > '2d' or '2D' ?\n> >\n> > > + */\n> > > +\n> > > +#ifndef __LIBCAMERA_TRANSFORM_H__\n> > > +#define __LIBCAMERA_TRANSFORM_H__\n> > > +\n> > > +#include \n> >\n> > You should also include stdint.h for int32_t, although I think you could\n> > use int instead of int32_t.\n> >\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +class Transform\n> > > +{\n> > > +public:\n> > > + constexpr Transform()\n> > > + : Transform(Identity)\n> > > + {\n> > > + }\n> > > +\n> > > + constexpr static Transform identity()\n> > > + {\n> > > + return Transform(Identity);\n> > > + }\n> > > +\n> > > + constexpr static Transform rot0()\n> > > + {\n> > > + return identity();\n> > > + }\n> > > +\n> > > + constexpr static Transform hflip()\n> > > + {\n> > > + return Transform(HFlip);\n> > > + }\n> > > +\n> > > + constexpr static Transform vflip()\n> > > + {\n> > > + return Transform(VFlip);\n> > > + }\n> > > +\n> > > + constexpr static Transform hvflip()\n> > > + {\n> > > + return Transform(HVFlip);\n> > > + }\n> > > +\n> > > + constexpr static Transform rot180()\n> > > + {\n> > > + return hvflip();\n> > > + }\n> > > +\n> > > + constexpr static Transform transpose()\n> > > + {\n> > > + return Transform(Transpose);\n> > > + }\n> > > +\n> > > + constexpr static Transform rot270()\n> > > + {\n> > > + return Transform(Rot270);\n> > > + }\n> > > +\n> > > + constexpr static Transform rot90()\n> > > + {\n> > > + return Transform(Rot90);\n> > > + }\n> > > +\n> > > + constexpr static Transform rot180Transpose()\n> > > + {\n> > > + return Transform(Rot180Transpose);\n> > > + }\n> > > +\n> > > + constexpr static Transform hvflipTranspose()\n> > > + {\n> > > + return rot180Transpose();\n> > > + }\n> > > +\n> >\n> > Wouldn't it be simpler to make the Type enum public, as well as the\n> > constructor that takes a Type ? I will also be way less code to type :-)\n> \n> I did wonder about this. I didn't like making the Type public because\n> either you use enum class and end up writing stuff like\n> Transform(Transform::Type::Identity) instead of Transform::identity(),\n> or you could use a straight enum and write\n> Transform(Transform::Identity) which I like, but you have to worry\n> about what this means:\n> Transform transform = Transform::Identity * Transform::HFlip;\n> \n> So I thought typing more in the header file was better than putting\n> the burden on the client code. But I don't feel very strongly...\n\nExposing the enum would avoid the nasty reinterpret_cast in the next\npatch. I think an enum class would make sense, and if we really want to\navoid using Transform::Type::Identity, we could add\n\n\tstatic const Transform Identity = Transform(Transform::Type::Identity);\n\n(same for the other ones) to allow usage of Transform::Identity. It may\nbe a bit overkill though. In any case, I think\n\n\tstatic const Transform Identity = Transform(Transform::Type::Identity);\n\nwould be better than\n\n constexpr static Transform identity()\n {\n return Transform(Identity);\n }\n\nas there's no need for functions.\n\nExposing the enum would also allow using it in a bitfield to expose the\nsupported values if needed, for instance to report the supported values.\nSee https://lists.libcamera.org/pipermail/libcamera-devel/2020-July/011279.html.\n\n> > > + constexpr static bool rotation(int32_t angle, Transform &xfm)\n> > > + {\n> > > + bool success = true;\n> > > + angle = angle % 360;\n> > > + if (angle < 0)\n> > > + angle += 360;\n> > > +\n> > > + if (angle == 0)\n> > > + xfm = identity();\n> > > + else if (angle == 90)\n> > > + xfm = rot90();\n> > > + else if (angle == 180)\n> > > + xfm = rot180();\n> > > + else if (angle == 270)\n> > > + xfm = rot270();\n> > > + else\n> > > + success = false;\n> > > +\n> > > + return success;\n> > > + }\n> > > +\n> > > + constexpr bool rotation(int32_t &angle) const\n> > > + {\n> > > + bool success = true;\n> > > +\n> > > + if (type_ == Identity)\n> > > + angle = 0;\n> > > + else if (type_ == Rot90)\n> > > + angle = 90;\n> > > + else if (type_ == HVFlip)\n> > > + angle = 180;\n> > > + else if (type_ == Rot270)\n> > > + angle = 270;\n> > > + else\n> > > + success = false;\n> > > +\n> > > + return success;\n> > > + }\n> >\n> > That's two \"rotation\" functions, one applying a rotation, the other one\n> > returning the rotation. I think it's a bit confusing. I would turn the\n> > former into\n> >\n> > bool rotate(int angle);\n> >\n> > and the later into\n> >\n> > constexpr unsigned int rotationAngle(bool *success = nullptr) const;\n> >\n> > I also think the two functions should be implemented in the .cpp file.\n> \n> Yes, two functions with the same name was always nasty. The first one\n> is actually creating a Transform from a rotation angle, not applying a\n> rotation (so obviously documentation will have to be clear on that).\n> \n> If we prefer the success code as the optional parameter it could look like:\n> constexpr static Transform rotation(int angle, bool *success = nullptr);\n\nWouldn't it make sense to apply a rotation to an existing Transform ?\nThat's what I envisioned with\n\n\tbool rotate(int angle);\n\nand one could do\n\n\tTransform t = Transform::Identity;\n\tt.rotate(90);\n\nto achieve the same result. Or if we want to support\n\n\tTransform t = Transform::Identity.rotate(90);\n\nthen the function could be defined as\n\n\tTransform rotate(int angle, bool *success = nullptr) const;\n\nIt may make sense to call it rotated() in that case (\"rotate\" modifies\nthe instance by applying a rotation, \"rotated\" returns a new instance\nthat results from applying the rotation). This is the naming scheme we\nwent for in geometry.h, see Size::alignDownTo() vs.\nSize::alignedDownTo(). Both rotate() and rotated() could be provided if\nneeded.\n\n> For the second one I like rotationAngle.\n> \n> > > +\n> > > + constexpr bool contains(Transform xfm) const\n> > > + {\n> > > + return (type_ & xfm.type_) == xfm.type_;\n> > > + }\n> >\n> > This function is used in the rest of the series to check if a\n> > transformation contains hflip, vflip or transpose. For that use case\n> > it's fine, but it could also be used with\n> >\n> > Transform::rot180Transpose().contains(Transform::rot270())\n> >\n> > and return true, which is quite confusing to read. I'm not sure how to\n> > improve that though. Maybe defining three functions, to test for hflip,\n> > vflip and transpose separately ? isHorizontallyFlipped(),\n> > isVerticallyFlipped, isTransposed ? Or isHFlipped(), isVFlipped() for\n> > the first two ?\n> \n> Yep, I could go with isHFlipped(), isVFlipped(), isTransposed().\n> \n> > > +\n> > > + constexpr Transform inverse() const\n> > > + {\n> > > + /* They're all self-inverses, except for Rot90 and Rot270. */\n> > > + const Type inverses[] = { Identity, HFlip, VFlip, HVFlip,\n> > > + Transpose, Rot90, Rot270, Rot180Transpose };\n> >\n> > Shouldn't this be static const ?\n> >\n> > > +\n> > > + return Transform(inverses[type_]);\n> > > + }\n> > > +\n> > > + constexpr Transform operator*(Transform xfm) const\n> > > + {\n> > > + /*\n> > > + * Reorder the operations so that we imagine doing xfm's transpose\n> > > + * (if any) after our flips. The effect is to swap our hflips for\n> > > + * vflips and vice versa, after which we can just xor all the bits.\n> > > + */\n> > > + int reorderedType = type_;\n> > > + if (xfm.type_ & Transpose)\n> > > + reorderedType = (type_ & 4) | ((type_ & 1) << 1) | ((type_ & 2) >> 1);\n> > > +\n> > > + return Transform((Type)(reorderedType ^ xfm.type_));\n> > > + }\n> >\n> > I would move the implementation of these two functions to the .cpp file\n> > too.\n> >\n> > > +\n> > > + bool operator==(Transform xfm) const\n> > > + {\n> > > + return type_ == xfm.type_;\n> > > + }\n> > > +\n> > > + bool operator!=(Transform xfm) const\n> > > + {\n> > > + return !(*this == xfm);\n> > > + }\n> > > +\n> > > + std::string toString() const\n> > > + {\n> > > + char const *strings[] = {\n> >\n> > static const char ?\n> >\n> > > + \"identity\",\n> > > + \"hflip\",\n> > > + \"vflip\",\n> > > + \"hvflip\",\n> > > + \"transpose\",\n> > > + \"rot270\",\n> > > + \"rot90\",\n> > > + \"rot180transpose\"\n> > > + };\n> > > + return strings[type_];\n> > > + }\n> >\n> > Of course all this needs documentation :-) A good introduction (in the\n> > \\class Transform block) would be very useful to explain the concepts I\n> > think.\n> >\n> > > +\n> > > +private:\n> > > + enum Type : int {\n> > > + Identity = 0,\n> > > + HFlip = 1,\n> > > + VFlip = 2,\n> > > + HVFlip = HFlip | VFlip,\n> > > + Transpose = 4,\n> > > + Rot270 = HFlip | Transpose,\n> > > + Rot90 = VFlip | Transpose,\n> > > + Rot180Transpose = HFlip | VFlip | Transpose\n> > > + };\n> > > +\n> > > + constexpr Transform(Type type)\n> > > + : type_(type)\n> > > + {\n> > > + }\n> > > +\n> > > + Type type_;\n> > > +};\n> > > +\n> > > +} /* namespace libcamera */\n> > > +\n> > > +#endif /* __LIBCAMERA_TRANSFORM_H__ */\n> > > +\n> >\n> > Extra blank line.","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 A7025BD878\n\tfor ;\n\tWed, 29 Jul 2020 16:49:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 261BD617D6;\n\tWed, 29 Jul 2020 18:49:58 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A915760540\n\tfor ;\n\tWed, 29 Jul 2020 18:49:56 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1BF8329E;\n\tWed, 29 Jul 2020 18:49:56 +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=\"SguAetMv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596041396;\n\tbh=uP+sDQ+WuBRid85MmljEILZFhQLr5ZVoS0P9aeLwxMk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SguAetMvIVYFBTHva/sGe872JfTK+yI5vLyve1AOrLLYrTgfOL9TfVsgCymSt39cw\n\t8AOrkmUg6dI0y3A5dHt5yBP8FCrirhTg6IynKnWLZZX0MC8QnYlpzgXmF4ZyZ5wAj2\n\t9HQCFglvu8LXxgZp13RZEIMkkA7zrEkwHzKN0YyQ=","Date":"Wed, 29 Jul 2020 19:49:47 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"<20200729164947.GM6183@pendragon.ideasonboard.com>","References":"<20200729093154.12296-1-david.plowman@raspberrypi.com>\n\t<20200729093154.12296-2-david.plowman@raspberrypi.com>\n\t<20200729151234.GG6183@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 1/5] libcamera: Add Transform class","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]