[{"id":27979,"web_url":"https://patchwork.libcamera.org/comment/27979/","msgid":"<20231018183710.GA1512@pendragon.ideasonboard.com>","date":"2023-10-18T18:37:10","subject":"Re: [libcamera-devel] [PATCH v5 02/12] libcamera: camera: Introduce\n\tOrientation","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:05PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> Introduce the Orientation enumeration which describes the possible 2D\n> transformations that can be applied to an image using two basic plane\n> transformations.\n> \n> Add to the CameraConfiguration class a new member 'orientation' which is\n> used to specify the image orientation in the memory buffers delivered to\n> applications.\n> \n> The enumeration values follow the ones defined by the EXIF specification at\n> revision 2.32, Tag 274 'orientation'.\n> \n> The newly introduced field is meant to replace\n> CameraConfiguration::transform which is not removed yet not to break\n> compilation.\n> \n> Signed-off-by: Jacopo Mondi \n> Reviewed-by: David Plowman \n> ---\n> include/libcamera/camera.h | 2 +\n> include/libcamera/meson.build | 1 +\n> include/libcamera/orientation.h | 28 ++++++++++++\n> src/libcamera/camera.cpp | 18 +++++++-\n> src/libcamera/meson.build | 1 +\n> src/libcamera/orientation.cpp | 78 +++++++++++++++++++++++++++++++++\n> 6 files changed, 127 insertions(+), 1 deletion(-)\n> create mode 100644 include/libcamera/orientation.h\n> create mode 100644 src/libcamera/orientation.cpp\n> \n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index 004bc89455f5..6e9342773962 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -19,6 +19,7 @@\n> #include \n> \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -67,6 +68,7 @@ public:\n> \tstd::size_t size() const;\n> \n> \tTransform transform;\n> +\tOrientation orientation;\n> \n> protected:\n> \tCameraConfiguration();\n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index 408b7acf152c..a24c50d66a82 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -12,6 +12,7 @@ libcamera_public_headers = files([\n> 'framebuffer_allocator.h',\n> 'geometry.h',\n> 'logging.h',\n> + 'orientation.h',\n> 'pixel_format.h',\n> 'request.h',\n> 'stream.h',\n> diff --git a/include/libcamera/orientation.h b/include/libcamera/orientation.h\n> new file mode 100644\n> index 000000000000..63ac4aba07ce\n> --- /dev/null\n> +++ b/include/libcamera/orientation.h\n> @@ -0,0 +1,28 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2023, Ideas On Board Oy\n> + *\n> + * orientation.h - Image orientation\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +enum class Orientation {\n> +\t/* EXIF tag 274 starts from '1' */\n> +\trotate0 = 1,\n\nEnumerators should start with an uppercase letter.\n\n> +\trotate0Flip,\n> +\trotate180,\n> +\trotate180Flip,\n> +\trotate90Flip,\n> +\trotate270,\n\nI think this should be rotate90.\n\n> +\trotate270Flip,\n> +\trotate90,\n\nAnd here, rotate270.\n\n> +};\n> +\n> +std::ostream &operator<<(std::ostream &out, const Orientation &orientation);\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 0eecee766f00..d4ad4a553752 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -160,7 +160,8 @@ LOG_DECLARE_CATEGORY(Camera)\n> * \\brief Create an empty camera configuration\n> */\n> CameraConfiguration::CameraConfiguration()\n> -\t: transform(Transform::Identity), config_({})\n> +\t: transform(Transform::Identity), orientation(Orientation::rotate0),\n> +\t config_({})\n> {\n> }\n> \n> @@ -404,6 +405,21 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF\n> * may adjust this field at its discretion if the selection is not supported.\n> */\n> \n> +/**\n> + * \\var CameraConfiguration::orientation\n> + * \\brief The desired orientation of the images produced by the camera\n> + *\n> + * The orientation field is a user-specified 2D plane transformation that\n\ns/field/member/\n\nThe word \"field\" in the C++ specification is used for bit-field only.\n\"fields\" of a class or structure are named \"members\" (or \"data member\"\nif you want to differentiate them from member functions).\n\n> + * specifies how the application wants the camera images to be rotated in\n> + * the memory buffers.\n\nAs the whole point of this series is to move from transform to\norientation, can we try to avoid using \"transformation\" to define the\norientation ? How about the following ?\n\n * The orientation specifies how the application wants images to be oriented in\n * memory buffers. The camera will rotate and flip images as appropriate to\n * obtain the desired orientation.\n\n> + *\n> + * If the application requested orientation cannot be obtained the validate()\n\ns/obtained/obtained,/\n\n> + * function will Adjust this field to the actual orientation of the images\n\ns/this field/the orientation member/\n\n> + * as produced by the camera pipeline.\n\nI recall we discussed that, in this case, the camera shouldn't try to\npick a value it considers the closest to the requested orientation, but\ndon't transform the image at all. Is that right ? If so, let's document\nit explicitly:\n\n * If the orientation requested by the application cannot be obtained, the\n * camera will not rotate or flip the images, and the validate() function will\n * Adjust this value to the native image orientation produced by the camera.\n\n> + *\n> + * By default the orientation field is set to Orientation::rotate0.\n\ns/field/member/\n\n> + */\n> +\n> /**\n> * \\var CameraConfiguration::config_\n> * \\brief The vector of stream configurations\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index b24f82965764..d0e26f6b4141 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -34,6 +34,7 @@ libcamera_sources = files([\n> 'mapped_framebuffer.cpp',\n> 'media_device.cpp',\n> 'media_object.cpp',\n> + 'orientation.cpp',\n> 'pipeline_handler.cpp',\n> 'pixel_format.cpp',\n> 'process.cpp',\n> diff --git a/src/libcamera/orientation.cpp b/src/libcamera/orientation.cpp\n> new file mode 100644\n> index 000000000000..f2ee14dd4182\n> --- /dev/null\n> +++ b/src/libcamera/orientation.cpp\n> @@ -0,0 +1,78 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2023, Ideas On Board Oy\n> + *\n> + * orientation.cpp - Image orientation\n> + */\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +\n> +/**\n> + * \\file libcamera/orientation.h\n> + * \\brief Image orientation definition\n> + */\n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\enum Orientation\n> + * \\brief The image orientation in a memory buffer\n> + *\n> + * The Orientation enumeration describes the orientation of the images\n> + * produced by the camera pipeline as they get received by the application\n> + * inside memory buffers.\n> + *\n\nFrom here to ...\n\n> + * All the possible 2D plane transformations of an image are expressed as the\n> + * combination of two basic operations:\n> + *\n> + * 'a': rotate the image 90 degrees clockwise\n> + * 'b': flip the image horizontally (mirroring)\n> + *\n> + * plus an identity operator 'e'.\n> + *\n> + * The composition of operations 'a' and 'b' according to the canonical function\n> + * composition notion 'b * a' (where 'a' is applied first, then 'b' is applied\n> + * next) gives origin to a symmetric group of order 8, or dihedral group.\n> + *\n> + * See https://en.wikipedia.org/wiki/Dihedral_group#/media/File:Dih4_cycle_graph.svg\n> + * as an example of the 2D plane transformation and how they originate from\n> + * the composition of the 'a' and 'b' operations.\n> + *\n> + * The image orientation expressed using the Orientation enumeration can be\n> + * then inferred by applying a multiple of a 90 degrees rotation from the origin\n> + * and then applying any horizontal mirroring to a base image assume to be\n> + * naturally oriented (no rotation and no mirroring applied).\n\n... here, is this a leftover of a previous implementation, or copied\nfrom somewhere else ? It doesn't seem to match the enumeration.\n\n> + *\n> + * The enumeration numerical values follow the ones defined by the EXIF\n> + * Specification version 2.32, Tag 274 \"Orientation\", while the names of the\n> + * enumerated values report the rotation and mirroring operations performed.\n> + *\n> + * In example Orientation::rotate90Flip describes the image transformation\n\ns/In example/For example,/\n\n> + * obtained by rotating 90 degrees clockwise first and then applying an\n> + * horizontal mirroring.\n> + */\n> +\n> +/**\n> + * \\brief Prints human-friendly names for Orientation items\n\nLet's be consistent with the other operator<<() implementations:\n\n * \\brief Insert a text representation of an Orientation into an output stream\n\n> + * \\param[in] out The output stream\n> + * \\param[in] orientation The Orientation item\n\ns/ item//\n\n> + * \\return The output stream \\a out\n> + */\n> +std::ostream &operator<<(std::ostream &out, const Orientation &orientation)\n> +{\n> +\tconstexpr std::array orientationNames = {\n> +\t\t\"\", /* Orientation starts counting from 1. */\n> +\t\t\"rotate0\", \"rotate0Flip\",\n> +\t\t\"rotate180\", \"rotate180Flip\",\n> +\t\t\"rotate90Flip\", \"rotate270\",\n> +\t\t\"rotate270Flip\", \"rotate90\",\n> +\t};\n> +\n> +\tout << orientationNames[static_cast(orientation)];\n\nYou can write\n\n\t/* The Orientation enumeration starts counting from 1. */\n\tout << orientationNames[static_cast(orientation) - 1];\n\nAnd drop the first entry from the array.\n\n> +\treturn out;\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 6DF88C3272\n\tfor ;\n\tWed, 18 Oct 2023 18:37:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A8CC56296F;\n\tWed, 18 Oct 2023 20:37:05 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C5ADA61DD1\n\tfor ;\n\tWed, 18 Oct 2023 20:37:03 +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 21843AE;\n\tWed, 18 Oct 2023 20:36:56 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1697654225;\n\tbh=uF8SQvj1jHh9zah4tJpHSD0n7JIlLwzJg2lJ4yzcYok=;\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=3+FuP66sSELWblJZjOjF2WTLgF67T8DCvITut2AYDwJLblQ0M4VkTwjjqLWJOf3ZD\n\tSeP0NNUr/5p8H22zYwc5LKm+tA5FmMMjkoNIto53e6MaN9gFFmL4pbFmpSeB8QfRuq\n\t/5a61x+p5JOGP2usLNod/X1/6ieURrOR1b8Y41QYCbHJrCgRpa1ZIyXqk1ofNXTKfZ\n\theu+NU5cu2BdaoIq0S8Da3fLzFEsMTqEDKfS6a3yeTljOgHV0kuwl64xxx/LfKB+Rc\n\te6JocgutbvGej6hMefU/LbeamzOENEy2v7dQtMybXxLvKG2lA9205IZS8710fO/lvv\n\tUAxgTSET1ppjw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1697654216;\n\tbh=uF8SQvj1jHh9zah4tJpHSD0n7JIlLwzJg2lJ4yzcYok=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Zd2EuuNKF+wSL71BGbT2ZqE4XUt7dpq3uj9tfISlLP8+Qv0MtOBr33fhzvzMtO8Ha\n\ttoWH0i6C8Ubrl/6FODFylh+JJ9lK1gQTE8Zh8d/fgrfnkfNFjmM/yn6OGbXIfH9O5H\n\tG03qYFIrSSEuUdt7QfB0PQXz7j4rxgWBOotPazPs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Zd2EuuNK\"; dkim-atps=neutral","Date":"Wed, 18 Oct 2023 21:37:10 +0300","To":"Jacopo Mondi ","Message-ID":"<20231018183710.GA1512@pendragon.ideasonboard.com>","References":"<20230901150215.11585-1-jacopo.mondi@ideasonboard.com>\n\t<20230901150215.11585-3-jacopo.mondi@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230901150215.11585-3-jacopo.mondi@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5 02/12] libcamera: camera: Introduce\n\tOrientation","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":27992,"web_url":"https://patchwork.libcamera.org/comment/27992/","msgid":"","date":"2023-10-19T08:19:11","subject":"Re: [libcamera-devel] [PATCH v5 02/12] libcamera: camera: Introduce\n\tOrientation","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Laurent\n\nOn Wed, Oct 18, 2023 at 09:37:10PM +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:05PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > Introduce the Orientation enumeration which describes the possible 2D\n> > transformations that can be applied to an image using two basic plane\n> > transformations.\n> >\n> > Add to the CameraConfiguration class a new member 'orientation' which is\n> > used to specify the image orientation in the memory buffers delivered to\n> > applications.\n> >\n> > The enumeration values follow the ones defined by the EXIF specification at\n> > revision 2.32, Tag 274 'orientation'.\n> >\n> > The newly introduced field is meant to replace\n> > CameraConfiguration::transform which is not removed yet not to break\n> > compilation.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > Reviewed-by: David Plowman \n> > ---\n> > include/libcamera/camera.h | 2 +\n> > include/libcamera/meson.build | 1 +\n> > include/libcamera/orientation.h | 28 ++++++++++++\n> > src/libcamera/camera.cpp | 18 +++++++-\n> > src/libcamera/meson.build | 1 +\n> > src/libcamera/orientation.cpp | 78 +++++++++++++++++++++++++++++++++\n> > 6 files changed, 127 insertions(+), 1 deletion(-)\n> > create mode 100644 include/libcamera/orientation.h\n> > create mode 100644 src/libcamera/orientation.cpp\n> >\n> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > index 004bc89455f5..6e9342773962 100644\n> > --- a/include/libcamera/camera.h\n> > +++ b/include/libcamera/camera.h\n> > @@ -19,6 +19,7 @@\n> > #include \n> >\n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -67,6 +68,7 @@ public:\n> > \tstd::size_t size() const;\n> >\n> > \tTransform transform;\n> > +\tOrientation orientation;\n> >\n> > protected:\n> > \tCameraConfiguration();\n> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > index 408b7acf152c..a24c50d66a82 100644\n> > --- a/include/libcamera/meson.build\n> > +++ b/include/libcamera/meson.build\n> > @@ -12,6 +12,7 @@ libcamera_public_headers = files([\n> > 'framebuffer_allocator.h',\n> > 'geometry.h',\n> > 'logging.h',\n> > + 'orientation.h',\n> > 'pixel_format.h',\n> > 'request.h',\n> > 'stream.h',\n> > diff --git a/include/libcamera/orientation.h b/include/libcamera/orientation.h\n> > new file mode 100644\n> > index 000000000000..63ac4aba07ce\n> > --- /dev/null\n> > +++ b/include/libcamera/orientation.h\n> > @@ -0,0 +1,28 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2023, Ideas On Board Oy\n> > + *\n> > + * orientation.h - Image orientation\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +enum class Orientation {\n> > +\t/* EXIF tag 274 starts from '1' */\n> > +\trotate0 = 1,\n>\n> Enumerators should start with an uppercase letter.\n>\n> > +\trotate0Flip,\n> > +\trotate180,\n> > +\trotate180Flip,\n> > +\trotate90Flip,\n> > +\trotate270,\n>\n> I think this should be rotate90.\n>\n\nAh!\n\nThe EXIF tag 6 is described as\n\nThe 0th row is the visual right-hand side of the image, and the 0th\ncolumn is the visual top.\n\n> > +\trotate270Flip,\n> > +\trotate90,\n>\n> And here, rotate270.\n\nEXIF tag 8\n\nThe 0th row is the visual left-hand side of the image, and the 0th\ncolumn is the visual bottom.\n\nI'll correct both\n\n>\n> > +};\n> > +\n> > +std::ostream &operator<<(std::ostream &out, const Orientation &orientation);\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 0eecee766f00..d4ad4a553752 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -160,7 +160,8 @@ LOG_DECLARE_CATEGORY(Camera)\n> > * \\brief Create an empty camera configuration\n> > */\n> > CameraConfiguration::CameraConfiguration()\n> > -\t: transform(Transform::Identity), config_({})\n> > +\t: transform(Transform::Identity), orientation(Orientation::rotate0),\n> > +\t config_({})\n> > {\n> > }\n> >\n> > @@ -404,6 +405,21 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF\n> > * may adjust this field at its discretion if the selection is not supported.\n> > */\n> >\n> > +/**\n> > + * \\var CameraConfiguration::orientation\n> > + * \\brief The desired orientation of the images produced by the camera\n> > + *\n> > + * The orientation field is a user-specified 2D plane transformation that\n>\n> s/field/member/\n>\n> The word \"field\" in the C++ specification is used for bit-field only.\n> \"fields\" of a class or structure are named \"members\" (or \"data member\"\n> if you want to differentiate them from member functions).\n>\n> > + * specifies how the application wants the camera images to be rotated in\n> > + * the memory buffers.\n>\n> As the whole point of this series is to move from transform to\n> orientation, can we try to avoid using \"transformation\" to define the\n> orientation ? How about the following ?\n>\n> * The orientation specifies how the application wants images to be oriented in\n> * memory buffers. The camera will rotate and flip images as appropriate to\n> * obtain the desired orientation.\n>\n\nI think there's difference between the Transform we want to move away\nfrom and a 2d plane transformation, which is a specific term, but if\njust seeing \"transformation\" spelled out irritates you.. ok...\n\n\n> > + *\n> > + * If the application requested orientation cannot be obtained the validate()\n>\n> s/obtained/obtained,/\n>\n> > + * function will Adjust this field to the actual orientation of the images\n>\n> s/this field/the orientation member/\n>\n> > + * as produced by the camera pipeline.\n>\n> I recall we discussed that, in this case, the camera shouldn't try to\n> pick a value it considers the closest to the requested orientation, but\n> don't transform the image at all. Is that right ? If so, let's document\n> it explicitly:\n\nYes, that's what the proposed documentation says\n\n * If the application requested orientation cannot be obtained the validate()\n * function will Adjust this field to the actual orientation of the images\n * as produced by the camera pipeline.\n\n>\n> * If the orientation requested by the application cannot be obtained, the\n> * camera will not rotate or flip the images, and the validate() function will\n> * Adjust this value to the native image orientation produced by the camera.\n>\n\nThe two says the same thing, but.. ok\n\n> > + *\n> > + * By default the orientation field is set to Orientation::rotate0.\n>\n> s/field/member/\n>\n> > + */\n> > +\n> > /**\n> > * \\var CameraConfiguration::config_\n> > * \\brief The vector of stream configurations\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index b24f82965764..d0e26f6b4141 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -34,6 +34,7 @@ libcamera_sources = files([\n> > 'mapped_framebuffer.cpp',\n> > 'media_device.cpp',\n> > 'media_object.cpp',\n> > + 'orientation.cpp',\n> > 'pipeline_handler.cpp',\n> > 'pixel_format.cpp',\n> > 'process.cpp',\n> > diff --git a/src/libcamera/orientation.cpp b/src/libcamera/orientation.cpp\n> > new file mode 100644\n> > index 000000000000..f2ee14dd4182\n> > --- /dev/null\n> > +++ b/src/libcamera/orientation.cpp\n> > @@ -0,0 +1,78 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2023, Ideas On Board Oy\n> > + *\n> > + * orientation.cpp - Image orientation\n> > + */\n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +/**\n> > + * \\file libcamera/orientation.h\n> > + * \\brief Image orientation definition\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +/**\n> > + * \\enum Orientation\n> > + * \\brief The image orientation in a memory buffer\n> > + *\n> > + * The Orientation enumeration describes the orientation of the images\n> > + * produced by the camera pipeline as they get received by the application\n> > + * inside memory buffers.\n> > + *\n>\n> From here to ...\n>\n> > + * All the possible 2D plane transformations of an image are expressed as the\n> > + * combination of two basic operations:\n> > + *\n> > + * 'a': rotate the image 90 degrees clockwise\n> > + * 'b': flip the image horizontally (mirroring)\n> > + *\n> > + * plus an identity operator 'e'.\n> > + *\n> > + * The composition of operations 'a' and 'b' according to the canonical function\n> > + * composition notion 'b * a' (where 'a' is applied first, then 'b' is applied\n> > + * next) gives origin to a symmetric group of order 8, or dihedral group.\n> > + *\n> > + * See https://en.wikipedia.org/wiki/Dihedral_group#/media/File:Dih4_cycle_graph.svg\n> > + * as an example of the 2D plane transformation and how they originate from\n> > + * the composition of the 'a' and 'b' operations.\n> > + *\n> > + * The image orientation expressed using the Orientation enumeration can be\n> > + * then inferred by applying a multiple of a 90 degrees rotation from the origin\n> > + * and then applying any horizontal mirroring to a base image assume to be\n> > + * naturally oriented (no rotation and no mirroring applied).\n>\n> ... here, is this a leftover of a previous implementation, or copied\n> from somewhere else ? It doesn't seem to match the enumeration.\n\nWhy it doesn't match the enumeration ? The enumerated members are\nnamed as \"90 degrees rotation\" + \"optional horizontal mirroring\".\n\nWhat is wrong here ?\n\n>\n> > + *\n> > + * The enumeration numerical values follow the ones defined by the EXIF\n> > + * Specification version 2.32, Tag 274 \"Orientation\", while the names of the\n> > + * enumerated values report the rotation and mirroring operations performed.\n> > + *\n> > + * In example Orientation::rotate90Flip describes the image transformation\n>\n> s/In example/For example,/\n>\n> > + * obtained by rotating 90 degrees clockwise first and then applying an\n> > + * horizontal mirroring.\n> > + */\n> > +\n> > +/**\n> > + * \\brief Prints human-friendly names for Orientation items\n>\n> Let's be consistent with the other operator<<() implementations:\n>\n> * \\brief Insert a text representation of an Orientation into an output stream\n>\n> > + * \\param[in] out The output stream\n> > + * \\param[in] orientation The Orientation item\n>\n> s/ item//\n>\n> > + * \\return The output stream \\a out\n> > + */\n> > +std::ostream &operator<<(std::ostream &out, const Orientation &orientation)\n> > +{\n> > +\tconstexpr std::array orientationNames = {\n> > +\t\t\"\", /* Orientation starts counting from 1. */\n> > +\t\t\"rotate0\", \"rotate0Flip\",\n> > +\t\t\"rotate180\", \"rotate180Flip\",\n> > +\t\t\"rotate90Flip\", \"rotate270\",\n> > +\t\t\"rotate270Flip\", \"rotate90\",\n> > +\t};\n> > +\n> > +\tout << orientationNames[static_cast(orientation)];\n>\n> You can write\n>\n> \t/* The Orientation enumeration starts counting from 1. */\n> \tout << orientationNames[static_cast(orientation) - 1];\n>\n> And drop the first entry from the array.\n>\n> > +\treturn out;\n> > +}\n> > +\n> > +} /* namespace libcamera */\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 A322DBDCBD\n\tfor ;\n\tThu, 19 Oct 2023 08:19:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B3B746297F;\n\tThu, 19 Oct 2023 10:19:17 +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 E3DEA61DD0\n\tfor ;\n\tThu, 19 Oct 2023 10:19:15 +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 7B8E425A;\n\tThu, 19 Oct 2023 10:19:07 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1697703557;\n\tbh=H9n7GcS+4EucMRFPa/cNVT7AdkhdrLq7xOquMaEHEVU=;\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=Cq0h7OJJ4phoxKqlaXMW/AU5Q9k1PUGlupyQTPSQCTI12B80UbVFT/MN63pg1GT0x\n\t/aF9cmjvxj4GizJR+4FcgKCRakqhOzTSuANXCjghdkZDFstLajSs8T38aDYkGDUuXL\n\tHb+PyQwGlqE3eoLjKjS41BE6ICpfY7WWULFFNJmiVp+q+YFTEOtP7GKazjZ8GOniOu\n\tPHvvyJ/96UZ4hHTmISEE/uPUqqxyZzdAHCJBlz5Ptimtv39UonB4+niSx7Rlo0/fa5\n\tP/bo7t9F3S6ybUkLta4+LvW+q7uQmTrFccTEbi0rb/PxVO7RqdmCX9qr1INq1Z7dNj\n\tBtrhxziDyx8hQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1697703547;\n\tbh=H9n7GcS+4EucMRFPa/cNVT7AdkhdrLq7xOquMaEHEVU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VHiX5Q4qcnFHdES9W/qUujhbuDfXS9hoQZdtm9BEz5FDA5P3qOB7ENE6yoq4PXmcM\n\tboTcNpoW9pxODMFdmKhEyWT5bat54gmu/bTvfO7Sy3jbQ0VzGebYOnytrLcdEhgdrh\n\teA4adWzbcs43VvpmEr41MduYuy6Kd3KoGknSN+7Y="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"VHiX5Q4q\"; dkim-atps=neutral","Date":"Thu, 19 Oct 2023 10:19:11 +0200","To":"Laurent Pinchart ","Message-ID":"","References":"<20230901150215.11585-1-jacopo.mondi@ideasonboard.com>\n\t<20230901150215.11585-3-jacopo.mondi@ideasonboard.com>\n\t<20231018183710.GA1512@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20231018183710.GA1512@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5 02/12] libcamera: camera: Introduce\n\tOrientation","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":27996,"web_url":"https://patchwork.libcamera.org/comment/27996/","msgid":"<20231019083752.GY1512@pendragon.ideasonboard.com>","date":"2023-10-19T08:37:52","subject":"Re: [libcamera-devel] [PATCH v5 02/12] libcamera: camera: Introduce\n\tOrientation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Thu, Oct 19, 2023 at 10:19:11AM +0200, Jacopo Mondi wrote:\n> On Wed, Oct 18, 2023 at 09:37:10PM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > On Fri, Sep 01, 2023 at 05:02:05PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > > Introduce the Orientation enumeration which describes the possible 2D\n> > > transformations that can be applied to an image using two basic plane\n> > > transformations.\n> > >\n> > > Add to the CameraConfiguration class a new member 'orientation' which is\n> > > used to specify the image orientation in the memory buffers delivered to\n> > > applications.\n> > >\n> > > The enumeration values follow the ones defined by the EXIF specification at\n> > > revision 2.32, Tag 274 'orientation'.\n> > >\n> > > The newly introduced field is meant to replace\n> > > CameraConfiguration::transform which is not removed yet not to break\n> > > compilation.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > Reviewed-by: David Plowman \n> > > ---\n> > > include/libcamera/camera.h | 2 +\n> > > include/libcamera/meson.build | 1 +\n> > > include/libcamera/orientation.h | 28 ++++++++++++\n> > > src/libcamera/camera.cpp | 18 +++++++-\n> > > src/libcamera/meson.build | 1 +\n> > > src/libcamera/orientation.cpp | 78 +++++++++++++++++++++++++++++++++\n> > > 6 files changed, 127 insertions(+), 1 deletion(-)\n> > > create mode 100644 include/libcamera/orientation.h\n> > > create mode 100644 src/libcamera/orientation.cpp\n> > >\n> > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > > index 004bc89455f5..6e9342773962 100644\n> > > --- a/include/libcamera/camera.h\n> > > +++ b/include/libcamera/camera.h\n> > > @@ -19,6 +19,7 @@\n> > > #include \n> > >\n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -67,6 +68,7 @@ public:\n> > > \tstd::size_t size() const;\n> > >\n> > > \tTransform transform;\n> > > +\tOrientation orientation;\n> > >\n> > > protected:\n> > > \tCameraConfiguration();\n> > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > > index 408b7acf152c..a24c50d66a82 100644\n> > > --- a/include/libcamera/meson.build\n> > > +++ b/include/libcamera/meson.build\n> > > @@ -12,6 +12,7 @@ libcamera_public_headers = files([\n> > > 'framebuffer_allocator.h',\n> > > 'geometry.h',\n> > > 'logging.h',\n> > > + 'orientation.h',\n> > > 'pixel_format.h',\n> > > 'request.h',\n> > > 'stream.h',\n> > > diff --git a/include/libcamera/orientation.h b/include/libcamera/orientation.h\n> > > new file mode 100644\n> > > index 000000000000..63ac4aba07ce\n> > > --- /dev/null\n> > > +++ b/include/libcamera/orientation.h\n> > > @@ -0,0 +1,28 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2023, Ideas On Board Oy\n> > > + *\n> > > + * orientation.h - Image orientation\n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include \n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +enum class Orientation {\n> > > +\t/* EXIF tag 274 starts from '1' */\n> > > +\trotate0 = 1,\n> >\n> > Enumerators should start with an uppercase letter.\n> >\n> > > +\trotate0Flip,\n> > > +\trotate180,\n> > > +\trotate180Flip,\n> > > +\trotate90Flip,\n> > > +\trotate270,\n> >\n> > I think this should be rotate90.\n> \n> Ah!\n> \n> The EXIF tag 6 is described as\n> \n> The 0th row is the visual right-hand side of the image, and the 0th\n> column is the visual top.\n\nLooks like I've misread it then, 270 is right.\n\n> > > +\trotate270Flip,\n> > > +\trotate90,\n> >\n> > And here, rotate270.\n> \n> EXIF tag 8\n> \n> The 0th row is the visual left-hand side of the image, and the 0th\n> column is the visual bottom.\n> \n> I'll correct both\n\nNo need to :-)\n\n> > > +};\n> > > +\n> > > +std::ostream &operator<<(std::ostream &out, const Orientation &orientation);\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > index 0eecee766f00..d4ad4a553752 100644\n> > > --- a/src/libcamera/camera.cpp\n> > > +++ b/src/libcamera/camera.cpp\n> > > @@ -160,7 +160,8 @@ LOG_DECLARE_CATEGORY(Camera)\n> > > * \\brief Create an empty camera configuration\n> > > */\n> > > CameraConfiguration::CameraConfiguration()\n> > > -\t: transform(Transform::Identity), config_({})\n> > > +\t: transform(Transform::Identity), orientation(Orientation::rotate0),\n> > > +\t config_({})\n> > > {\n> > > }\n> > >\n> > > @@ -404,6 +405,21 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF\n> > > * may adjust this field at its discretion if the selection is not supported.\n> > > */\n> > >\n> > > +/**\n> > > + * \\var CameraConfiguration::orientation\n> > > + * \\brief The desired orientation of the images produced by the camera\n> > > + *\n> > > + * The orientation field is a user-specified 2D plane transformation that\n> >\n> > s/field/member/\n> >\n> > The word \"field\" in the C++ specification is used for bit-field only.\n> > \"fields\" of a class or structure are named \"members\" (or \"data member\"\n> > if you want to differentiate them from member functions).\n> >\n> > > + * specifies how the application wants the camera images to be rotated in\n> > > + * the memory buffers.\n> >\n> > As the whole point of this series is to move from transform to\n> > orientation, can we try to avoid using \"transformation\" to define the\n> > orientation ? How about the following ?\n> >\n> > * The orientation specifies how the application wants images to be oriented in\n> > * memory buffers. The camera will rotate and flip images as appropriate to\n> > * obtain the desired orientation.\n> \n> I think there's difference between the Transform we want to move away\n> from and a 2d plane transformation, which is a specific term, but if\n> just seeing \"transformation\" spelled out irritates you.. ok...\n\nYuo're right that the word \"transformation\" has a more general meaning,\nI didn't think about it during the review. We could indeed keep using\nit, when it's clear we're not referring to the Transform enum.\n\n> > > + *\n> > > + * If the application requested orientation cannot be obtained the validate()\n> >\n> > s/obtained/obtained,/\n> >\n> > > + * function will Adjust this field to the actual orientation of the images\n> >\n> > s/this field/the orientation member/\n> >\n> > > + * as produced by the camera pipeline.\n> >\n> > I recall we discussed that, in this case, the camera shouldn't try to\n> > pick a value it considers the closest to the requested orientation, but\n> > don't transform the image at all. Is that right ? If so, let's document\n> > it explicitly:\n> \n> Yes, that's what the proposed documentation says\n> \n> * If the application requested orientation cannot be obtained the validate()\n> * function will Adjust this field to the actual orientation of the images\n> * as produced by the camera pipeline.\n\n\"as produced by the camera pipeline\" means that the orientation field\ngets adjusted to reflect what the camera produces, but it doesn't tell\nif the camera can/should still apply a transformation.\n\n> > * If the orientation requested by the application cannot be obtained, the\n> > * camera will not rotate or flip the images, and the validate() function will\n> > * Adjust this value to the native image orientation produced by the camera.\n> \n> The two says the same thing, but.. ok\n> \n> > > + *\n> > > + * By default the orientation field is set to Orientation::rotate0.\n> >\n> > s/field/member/\n> >\n> > > + */\n> > > +\n> > > /**\n> > > * \\var CameraConfiguration::config_\n> > > * \\brief The vector of stream configurations\n> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > index b24f82965764..d0e26f6b4141 100644\n> > > --- a/src/libcamera/meson.build\n> > > +++ b/src/libcamera/meson.build\n> > > @@ -34,6 +34,7 @@ libcamera_sources = files([\n> > > 'mapped_framebuffer.cpp',\n> > > 'media_device.cpp',\n> > > 'media_object.cpp',\n> > > + 'orientation.cpp',\n> > > 'pipeline_handler.cpp',\n> > > 'pixel_format.cpp',\n> > > 'process.cpp',\n> > > diff --git a/src/libcamera/orientation.cpp b/src/libcamera/orientation.cpp\n> > > new file mode 100644\n> > > index 000000000000..f2ee14dd4182\n> > > --- /dev/null\n> > > +++ b/src/libcamera/orientation.cpp\n> > > @@ -0,0 +1,78 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2023, Ideas On Board Oy\n> > > + *\n> > > + * orientation.cpp - Image orientation\n> > > + */\n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +/**\n> > > + * \\file libcamera/orientation.h\n> > > + * \\brief Image orientation definition\n> > > + */\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +/**\n> > > + * \\enum Orientation\n> > > + * \\brief The image orientation in a memory buffer\n> > > + *\n> > > + * The Orientation enumeration describes the orientation of the images\n> > > + * produced by the camera pipeline as they get received by the application\n> > > + * inside memory buffers.\n> > > + *\n> >\n> > From here to ...\n> >\n> > > + * All the possible 2D plane transformations of an image are expressed as the\n> > > + * combination of two basic operations:\n> > > + *\n> > > + * 'a': rotate the image 90 degrees clockwise\n> > > + * 'b': flip the image horizontally (mirroring)\n> > > + *\n> > > + * plus an identity operator 'e'.\n> > > + *\n> > > + * The composition of operations 'a' and 'b' according to the canonical function\n> > > + * composition notion 'b * a' (where 'a' is applied first, then 'b' is applied\n> > > + * next) gives origin to a symmetric group of order 8, or dihedral group.\n> > > + *\n> > > + * See https://en.wikipedia.org/wiki/Dihedral_group#/media/File:Dih4_cycle_graph.svg\n> > > + * as an example of the 2D plane transformation and how they originate from\n> > > + * the composition of the 'a' and 'b' operations.\n> > > + *\n> > > + * The image orientation expressed using the Orientation enumeration can be\n> > > + * then inferred by applying a multiple of a 90 degrees rotation from the origin\n> > > + * and then applying any horizontal mirroring to a base image assume to be\n> > > + * naturally oriented (no rotation and no mirroring applied).\n> >\n> > ... here, is this a leftover of a previous implementation, or copied\n> > from somewhere else ? It doesn't seem to match the enumeration.\n> \n> Why it doesn't match the enumeration ? The enumerated members are\n> named as \"90 degrees rotation\" + \"optional horizontal mirroring\".\n> \n> What is wrong here ?\n\nThe text above is not incorrect, but reading it, it appears barely\nconnected to the enumerators, and I think it's also overly complicated.\n\n> > > + *\n> > > + * The enumeration numerical values follow the ones defined by the EXIF\n> > > + * Specification version 2.32, Tag 274 \"Orientation\", while the names of the\n> > > + * enumerated values report the rotation and mirroring operations performed.\n> > > + *\n> > > + * In example Orientation::rotate90Flip describes the image transformation\n> >\n> > s/In example/For example,/\n> >\n> > > + * obtained by rotating 90 degrees clockwise first and then applying an\n> > > + * horizontal mirroring.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Prints human-friendly names for Orientation items\n> >\n> > Let's be consistent with the other operator<<() implementations:\n> >\n> > * \\brief Insert a text representation of an Orientation into an output stream\n> >\n> > > + * \\param[in] out The output stream\n> > > + * \\param[in] orientation The Orientation item\n> >\n> > s/ item//\n> >\n> > > + * \\return The output stream \\a out\n> > > + */\n> > > +std::ostream &operator<<(std::ostream &out, const Orientation &orientation)\n> > > +{\n> > > +\tconstexpr std::array orientationNames = {\n> > > +\t\t\"\", /* Orientation starts counting from 1. */\n> > > +\t\t\"rotate0\", \"rotate0Flip\",\n> > > +\t\t\"rotate180\", \"rotate180Flip\",\n> > > +\t\t\"rotate90Flip\", \"rotate270\",\n> > > +\t\t\"rotate270Flip\", \"rotate90\",\n> > > +\t};\n> > > +\n> > > +\tout << orientationNames[static_cast(orientation)];\n> >\n> > You can write\n> >\n> > \t/* The Orientation enumeration starts counting from 1. */\n> > \tout << orientationNames[static_cast(orientation) - 1];\n> >\n> > And drop the first entry from the array.\n> >\n> > > +\treturn out;\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 B49BCC3272\n\tfor ;\n\tThu, 19 Oct 2023 08:37:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2146F6297C;\n\tThu, 19 Oct 2023 10:37:47 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CE1E46297B\n\tfor ;\n\tThu, 19 Oct 2023 10:37:45 +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 B431A25A;\n\tThu, 19 Oct 2023 10:37:37 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1697704667;\n\tbh=4G5rWJuyNlpeerTg6UCunfdp3JUZdYBt3E3Obg/xgE4=;\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=p83oI6fJyPtiQfp5r894udTLkH31jyoIF9FdDHlulkdAOvcc7lbiduwIuN5RMtSmF\n\tlVOb9m/EilRe+Xs95+3wYoydoNQ79aBH+fRjFETGHrFVGJ6MK1K0yBowwwwypv/l0j\n\tdVpKRffvakSxvKJPDBxqbfU/qb3Ax8Ik2pFIWIdDAAmxaR0CcRRDbflc9ikqOWs+Tv\n\thcg2H/N4LqfCSLQ5qFlvoN+gLYZqQWlx7JeMJWWpSfaocwLAOcC4bR098eXdtSPamN\n\tIXqqVLwQq4Q/EC8Fw5aGemDN6A+0FTarQnFoXiPDoXD1wJg1n1oT7R0CtAiL3SgeqK\n\tfQQhXQY6EDQMg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1697704657;\n\tbh=4G5rWJuyNlpeerTg6UCunfdp3JUZdYBt3E3Obg/xgE4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=P3dL8J2SlNGsGb1pQO0zzCt+Vi292s3KitGC0WNd40Ob1YQFyZaX8sp3szzo+Khfo\n\tg+Fl/TKKVbwcvNyz/V6FIaM0i5eVCZ9b6B7ui/D1h533eyHt4AiY16NGtBUGLf6WyO\n\tfRjp16PpqVv205tmf/oaHoKlMa3zQA3AX1NigXH0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"P3dL8J2S\"; dkim-atps=neutral","Date":"Thu, 19 Oct 2023 11:37:52 +0300","To":"Jacopo Mondi ","Message-ID":"<20231019083752.GY1512@pendragon.ideasonboard.com>","References":"<20230901150215.11585-1-jacopo.mondi@ideasonboard.com>\n\t<20230901150215.11585-3-jacopo.mondi@ideasonboard.com>\n\t<20231018183710.GA1512@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 02/12] libcamera: camera: Introduce\n\tOrientation","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":27997,"web_url":"https://patchwork.libcamera.org/comment/27997/","msgid":"","date":"2023-10-19T09:19:33","subject":"Re: [libcamera-devel] [PATCH v5 02/12] libcamera: camera: Introduce\n\tOrientation","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 11:37:52AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Thu, Oct 19, 2023 at 10:19:11AM +0200, Jacopo Mondi wrote:\n> > On Wed, Oct 18, 2023 at 09:37:10PM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > > On Fri, Sep 01, 2023 at 05:02:05PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > > > Introduce the Orientation enumeration which describes the possible 2D\n> > > > transformations that can be applied to an image using two basic plane\n> > > > transformations.\n> > > >\n> > > > Add to the CameraConfiguration class a new member 'orientation' which is\n> > > > used to specify the image orientation in the memory buffers delivered to\n> > > > applications.\n> > > >\n> > > > The enumeration values follow the ones defined by the EXIF specification at\n> > > > revision 2.32, Tag 274 'orientation'.\n> > > >\n> > > > The newly introduced field is meant to replace\n> > > > CameraConfiguration::transform which is not removed yet not to break\n> > > > compilation.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > Reviewed-by: David Plowman \n> > > > ---\n> > > > include/libcamera/camera.h | 2 +\n> > > > include/libcamera/meson.build | 1 +\n> > > > include/libcamera/orientation.h | 28 ++++++++++++\n> > > > src/libcamera/camera.cpp | 18 +++++++-\n> > > > src/libcamera/meson.build | 1 +\n> > > > src/libcamera/orientation.cpp | 78 +++++++++++++++++++++++++++++++++\n> > > > 6 files changed, 127 insertions(+), 1 deletion(-)\n> > > > create mode 100644 include/libcamera/orientation.h\n> > > > create mode 100644 src/libcamera/orientation.cpp\n> > > >\n> > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > > > index 004bc89455f5..6e9342773962 100644\n> > > > --- a/include/libcamera/camera.h\n> > > > +++ b/include/libcamera/camera.h\n> > > > @@ -19,6 +19,7 @@\n> > > > #include \n> > > >\n> > > > #include \n> > > > +#include \n> > > > #include \n> > > > #include \n> > > > #include \n> > > > @@ -67,6 +68,7 @@ public:\n> > > > \tstd::size_t size() const;\n> > > >\n> > > > \tTransform transform;\n> > > > +\tOrientation orientation;\n> > > >\n> > > > protected:\n> > > > \tCameraConfiguration();\n> > > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > > > index 408b7acf152c..a24c50d66a82 100644\n> > > > --- a/include/libcamera/meson.build\n> > > > +++ b/include/libcamera/meson.build\n> > > > @@ -12,6 +12,7 @@ libcamera_public_headers = files([\n> > > > 'framebuffer_allocator.h',\n> > > > 'geometry.h',\n> > > > 'logging.h',\n> > > > + 'orientation.h',\n> > > > 'pixel_format.h',\n> > > > 'request.h',\n> > > > 'stream.h',\n> > > > diff --git a/include/libcamera/orientation.h b/include/libcamera/orientation.h\n> > > > new file mode 100644\n> > > > index 000000000000..63ac4aba07ce\n> > > > --- /dev/null\n> > > > +++ b/include/libcamera/orientation.h\n> > > > @@ -0,0 +1,28 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2023, Ideas On Board Oy\n> > > > + *\n> > > > + * orientation.h - Image orientation\n> > > > + */\n> > > > +\n> > > > +#pragma once\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +enum class Orientation {\n> > > > +\t/* EXIF tag 274 starts from '1' */\n> > > > +\trotate0 = 1,\n> > >\n> > > Enumerators should start with an uppercase letter.\n> > >\n> > > > +\trotate0Flip,\n> > > > +\trotate180,\n> > > > +\trotate180Flip,\n> > > > +\trotate90Flip,\n> > > > +\trotate270,\n> > >\n> > > I think this should be rotate90.\n> >\n> > Ah!\n> >\n> > The EXIF tag 6 is described as\n> >\n> > The 0th row is the visual right-hand side of the image, and the 0th\n> > column is the visual top.\n>\n> Looks like I've misread it then, 270 is right.\n>\n> > > > +\trotate270Flip,\n> > > > +\trotate90,\n> > >\n> > > And here, rotate270.\n> >\n> > EXIF tag 8\n> >\n> > The 0th row is the visual left-hand side of the image, and the 0th\n> > column is the visual bottom.\n> >\n> > I'll correct both\n>\n> No need to :-)\n>\n\nI keep reading the EXIF descriptions wrong\n\nThe \"Oth row\" refers to what is displayed, not the first row of the\nimage...\n\nAt least it seems like I finally got them right two months ago\n\n\n> > > > +};\n> > > > +\n> > > > +std::ostream &operator<<(std::ostream &out, const Orientation &orientation);\n> > > > +\n> > > > +} /* namespace libcamera */\n> > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > > index 0eecee766f00..d4ad4a553752 100644\n> > > > --- a/src/libcamera/camera.cpp\n> > > > +++ b/src/libcamera/camera.cpp\n> > > > @@ -160,7 +160,8 @@ LOG_DECLARE_CATEGORY(Camera)\n> > > > * \\brief Create an empty camera configuration\n> > > > */\n> > > > CameraConfiguration::CameraConfiguration()\n> > > > -\t: transform(Transform::Identity), config_({})\n> > > > +\t: transform(Transform::Identity), orientation(Orientation::rotate0),\n> > > > +\t config_({})\n> > > > {\n> > > > }\n> > > >\n> > > > @@ -404,6 +405,21 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF\n> > > > * may adjust this field at its discretion if the selection is not supported.\n> > > > */\n> > > >\n> > > > +/**\n> > > > + * \\var CameraConfiguration::orientation\n> > > > + * \\brief The desired orientation of the images produced by the camera\n> > > > + *\n> > > > + * The orientation field is a user-specified 2D plane transformation that\n> > >\n> > > s/field/member/\n> > >\n> > > The word \"field\" in the C++ specification is used for bit-field only.\n> > > \"fields\" of a class or structure are named \"members\" (or \"data member\"\n> > > if you want to differentiate them from member functions).\n> > >\n> > > > + * specifies how the application wants the camera images to be rotated in\n> > > > + * the memory buffers.\n> > >\n> > > As the whole point of this series is to move from transform to\n> > > orientation, can we try to avoid using \"transformation\" to define the\n> > > orientation ? How about the following ?\n> > >\n> > > * The orientation specifies how the application wants images to be oriented in\n> > > * memory buffers. The camera will rotate and flip images as appropriate to\n> > > * obtain the desired orientation.\n> >\n> > I think there's difference between the Transform we want to move away\n> > from and a 2d plane transformation, which is a specific term, but if\n> > just seeing \"transformation\" spelled out irritates you.. ok...\n>\n> Yuo're right that the word \"transformation\" has a more general meaning,\n> I didn't think about it during the review. We could indeed keep using\n> it, when it's clear we're not referring to the Transform enum.\n>\n> > > > + *\n> > > > + * If the application requested orientation cannot be obtained the validate()\n> > >\n> > > s/obtained/obtained,/\n> > >\n> > > > + * function will Adjust this field to the actual orientation of the images\n> > >\n> > > s/this field/the orientation member/\n> > >\n> > > > + * as produced by the camera pipeline.\n> > >\n> > > I recall we discussed that, in this case, the camera shouldn't try to\n> > > pick a value it considers the closest to the requested orientation, but\n> > > don't transform the image at all. Is that right ? If so, let's document\n> > > it explicitly:\n> >\n> > Yes, that's what the proposed documentation says\n> >\n> > * If the application requested orientation cannot be obtained the validate()\n> > * function will Adjust this field to the actual orientation of the images\n> > * as produced by the camera pipeline.\n>\n> \"as produced by the camera pipeline\" means that the orientation field\n> gets adjusted to reflect what the camera produces, but it doesn't tell\n> if the camera can/should still apply a transformation.\n>\n\nOh, I see why it was confusing now\n\n> > > * If the orientation requested by the application cannot be obtained, the\n> > > * camera will not rotate or flip the images, and the validate() function will\n> > > * Adjust this value to the native image orientation produced by the camera.\n> >\n> > The two says the same thing, but.. ok\n> >\n> > > > + *\n> > > > + * By default the orientation field is set to Orientation::rotate0.\n> > >\n> > > s/field/member/\n> > >\n> > > > + */\n> > > > +\n> > > > /**\n> > > > * \\var CameraConfiguration::config_\n> > > > * \\brief The vector of stream configurations\n> > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > > index b24f82965764..d0e26f6b4141 100644\n> > > > --- a/src/libcamera/meson.build\n> > > > +++ b/src/libcamera/meson.build\n> > > > @@ -34,6 +34,7 @@ libcamera_sources = files([\n> > > > 'mapped_framebuffer.cpp',\n> > > > 'media_device.cpp',\n> > > > 'media_object.cpp',\n> > > > + 'orientation.cpp',\n> > > > 'pipeline_handler.cpp',\n> > > > 'pixel_format.cpp',\n> > > > 'process.cpp',\n> > > > diff --git a/src/libcamera/orientation.cpp b/src/libcamera/orientation.cpp\n> > > > new file mode 100644\n> > > > index 000000000000..f2ee14dd4182\n> > > > --- /dev/null\n> > > > +++ b/src/libcamera/orientation.cpp\n> > > > @@ -0,0 +1,78 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2023, Ideas On Board Oy\n> > > > + *\n> > > > + * orientation.cpp - Image orientation\n> > > > + */\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +/**\n> > > > + * \\file libcamera/orientation.h\n> > > > + * \\brief Image orientation definition\n> > > > + */\n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +/**\n> > > > + * \\enum Orientation\n> > > > + * \\brief The image orientation in a memory buffer\n> > > > + *\n> > > > + * The Orientation enumeration describes the orientation of the images\n> > > > + * produced by the camera pipeline as they get received by the application\n> > > > + * inside memory buffers.\n> > > > + *\n> > >\n> > > From here to ...\n> > >\n> > > > + * All the possible 2D plane transformations of an image are expressed as the\n> > > > + * combination of two basic operations:\n> > > > + *\n> > > > + * 'a': rotate the image 90 degrees clockwise\n> > > > + * 'b': flip the image horizontally (mirroring)\n> > > > + *\n> > > > + * plus an identity operator 'e'.\n> > > > + *\n> > > > + * The composition of operations 'a' and 'b' according to the canonical function\n> > > > + * composition notion 'b * a' (where 'a' is applied first, then 'b' is applied\n> > > > + * next) gives origin to a symmetric group of order 8, or dihedral group.\n> > > > + *\n> > > > + * See https://en.wikipedia.org/wiki/Dihedral_group#/media/File:Dih4_cycle_graph.svg\n> > > > + * as an example of the 2D plane transformation and how they originate from\n> > > > + * the composition of the 'a' and 'b' operations.\n> > > > + *\n> > > > + * The image orientation expressed using the Orientation enumeration can be\n> > > > + * then inferred by applying a multiple of a 90 degrees rotation from the origin\n> > > > + * and then applying any horizontal mirroring to a base image assume to be\n> > > > + * naturally oriented (no rotation and no mirroring applied).\n> > >\n> > > ... here, is this a leftover of a previous implementation, or copied\n> > > from somewhere else ? It doesn't seem to match the enumeration.\n> >\n> > Why it doesn't match the enumeration ? The enumerated members are\n> > named as \"90 degrees rotation\" + \"optional horizontal mirroring\".\n> >\n> > What is wrong here ?\n>\n> The text above is not incorrect, but reading it, it appears barely\n> connected to the enumerators, and I think it's also overly complicated.\n>\n\nWhen it comes to being possibily too complicated, I agree :)\n\nI tried to keep on par with David's documentation about Transform\nwhich provided a bit more of a formal background.\n\nIs it ok to remove the part about the the dihedral group but keep the\nlast paragraph (\"The image orientation expressed ..\") or should it be\nremoved as well ?\n\n> > > > + *\n> > > > + * The enumeration numerical values follow the ones defined by the EXIF\n> > > > + * Specification version 2.32, Tag 274 \"Orientation\", while the names of the\n> > > > + * enumerated values report the rotation and mirroring operations performed.\n> > > > + *\n> > > > + * In example Orientation::rotate90Flip describes the image transformation\n> > >\n> > > s/In example/For example,/\n> > >\n> > > > + * obtained by rotating 90 degrees clockwise first and then applying an\n> > > > + * horizontal mirroring.\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\brief Prints human-friendly names for Orientation items\n> > >\n> > > Let's be consistent with the other operator<<() implementations:\n> > >\n> > > * \\brief Insert a text representation of an Orientation into an output stream\n> > >\n> > > > + * \\param[in] out The output stream\n> > > > + * \\param[in] orientation The Orientation item\n> > >\n> > > s/ item//\n> > >\n> > > > + * \\return The output stream \\a out\n> > > > + */\n> > > > +std::ostream &operator<<(std::ostream &out, const Orientation &orientation)\n> > > > +{\n> > > > +\tconstexpr std::array orientationNames = {\n> > > > +\t\t\"\", /* Orientation starts counting from 1. */\n> > > > +\t\t\"rotate0\", \"rotate0Flip\",\n> > > > +\t\t\"rotate180\", \"rotate180Flip\",\n> > > > +\t\t\"rotate90Flip\", \"rotate270\",\n> > > > +\t\t\"rotate270Flip\", \"rotate90\",\n> > > > +\t};\n> > > > +\n> > > > +\tout << orientationNames[static_cast(orientation)];\n> > >\n> > > You can write\n> > >\n> > > \t/* The Orientation enumeration starts counting from 1. */\n> > > \tout << orientationNames[static_cast(orientation) - 1];\n> > >\n> > > And drop the first entry from the array.\n> > >\n> > > > +\treturn out;\n> > > > +}\n> > > > +\n> > > > +} /* namespace libcamera */\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 6BD33BDCBD\n\tfor ;\n\tThu, 19 Oct 2023 09:19:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7CCA46297F;\n\tThu, 19 Oct 2023 11:19:37 +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 B28F761DD0\n\tfor ;\n\tThu, 19 Oct 2023 11:19:36 +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 A8092276;\n\tThu, 19 Oct 2023 11:19:28 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1697707177;\n\tbh=lJBHYsWcMvEMGE6IHKeU3NOReBk/PZzYLHlTBk38CDY=;\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=wYvK+r+iuZfxor5JwAyZEAIsLXJxZpswKFPlZ2bSIIS5dJQyagL0scyRzTKo6jdi9\n\toEa0ztwMgfTBnDDzmibgXsweFsN0a/0kQwVoNQxi8Ox3rzSrDO7E0oqfOiFEnjqsey\n\thtpmJ231RRzS4hTSO0Z2EqDVo3oa+6ablJ5+ImufLB/+B0eRZWjaHXk+a4nZVSiqR7\n\tm+S3SXLDJNGYMqHwe2k6yZs/44d1ovnidkDzgbtLJ8ZIaRsFPRFAITR3JDBV8uSxZm\n\tPCs/Trn4dBObf2epGr5TTn3g5Kba++JeGS3gIeKDeFOUykJm217PwLq5bXvIRPJ7Do\n\tzhTROyU2VzG5g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1697707168;\n\tbh=lJBHYsWcMvEMGE6IHKeU3NOReBk/PZzYLHlTBk38CDY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=P5M43SXFatzZQGl0CMjOPyuMVMitaiDLyWF1Sq2F4SCygFX/NfHO0aR/VRME2/ZfY\n\te2/8TLAGqtCLGk9U9TqKAw9ecQXpl7DbQURd6sdflbeAbSVvt/zT356y3lon/K7VTj\n\tz2rzRZbuDSrUk5aa8uZRsS2BQ5LIwOe6mDaBvaFc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"P5M43SXF\"; dkim-atps=neutral","Date":"Thu, 19 Oct 2023 11:19:33 +0200","To":"Laurent Pinchart ","Message-ID":"","References":"<20230901150215.11585-1-jacopo.mondi@ideasonboard.com>\n\t<20230901150215.11585-3-jacopo.mondi@ideasonboard.com>\n\t<20231018183710.GA1512@pendragon.ideasonboard.com>\n\t\n\t<20231019083752.GY1512@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20231019083752.GY1512@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5 02/12] libcamera: camera: Introduce\n\tOrientation","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":28006,"web_url":"https://patchwork.libcamera.org/comment/28006/","msgid":"<20231019135434.GK14832@pendragon.ideasonboard.com>","date":"2023-10-19T13:54:34","subject":"Re: [libcamera-devel] [PATCH v5 02/12] libcamera: camera: Introduce\n\tOrientation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Thu, Oct 19, 2023 at 11:19:33AM +0200, Jacopo Mondi wrote:\n> On Thu, Oct 19, 2023 at 11:37:52AM +0300, Laurent Pinchart wrote:\n> > On Thu, Oct 19, 2023 at 10:19:11AM +0200, Jacopo Mondi wrote:\n> > > On Wed, Oct 18, 2023 at 09:37:10PM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > > > On Fri, Sep 01, 2023 at 05:02:05PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > > > > Introduce the Orientation enumeration which describes the possible 2D\n> > > > > transformations that can be applied to an image using two basic plane\n> > > > > transformations.\n> > > > >\n> > > > > Add to the CameraConfiguration class a new member 'orientation' which is\n> > > > > used to specify the image orientation in the memory buffers delivered to\n> > > > > applications.\n> > > > >\n> > > > > The enumeration values follow the ones defined by the EXIF specification at\n> > > > > revision 2.32, Tag 274 'orientation'.\n> > > > >\n> > > > > The newly introduced field is meant to replace\n> > > > > CameraConfiguration::transform which is not removed yet not to break\n> > > > > compilation.\n> > > > >\n> > > > > Signed-off-by: Jacopo Mondi \n> > > > > Reviewed-by: David Plowman \n> > > > > ---\n> > > > > include/libcamera/camera.h | 2 +\n> > > > > include/libcamera/meson.build | 1 +\n> > > > > include/libcamera/orientation.h | 28 ++++++++++++\n> > > > > src/libcamera/camera.cpp | 18 +++++++-\n> > > > > src/libcamera/meson.build | 1 +\n> > > > > src/libcamera/orientation.cpp | 78 +++++++++++++++++++++++++++++++++\n> > > > > 6 files changed, 127 insertions(+), 1 deletion(-)\n> > > > > create mode 100644 include/libcamera/orientation.h\n> > > > > create mode 100644 src/libcamera/orientation.cpp\n> > > > >\n> > > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > > > > index 004bc89455f5..6e9342773962 100644\n> > > > > --- a/include/libcamera/camera.h\n> > > > > +++ b/include/libcamera/camera.h\n> > > > > @@ -19,6 +19,7 @@\n> > > > > #include \n> > > > >\n> > > > > #include \n> > > > > +#include \n> > > > > #include \n> > > > > #include \n> > > > > #include \n> > > > > @@ -67,6 +68,7 @@ public:\n> > > > > \tstd::size_t size() const;\n> > > > >\n> > > > > \tTransform transform;\n> > > > > +\tOrientation orientation;\n> > > > >\n> > > > > protected:\n> > > > > \tCameraConfiguration();\n> > > > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > > > > index 408b7acf152c..a24c50d66a82 100644\n> > > > > --- a/include/libcamera/meson.build\n> > > > > +++ b/include/libcamera/meson.build\n> > > > > @@ -12,6 +12,7 @@ libcamera_public_headers = files([\n> > > > > 'framebuffer_allocator.h',\n> > > > > 'geometry.h',\n> > > > > 'logging.h',\n> > > > > + 'orientation.h',\n> > > > > 'pixel_format.h',\n> > > > > 'request.h',\n> > > > > 'stream.h',\n> > > > > diff --git a/include/libcamera/orientation.h b/include/libcamera/orientation.h\n> > > > > new file mode 100644\n> > > > > index 000000000000..63ac4aba07ce\n> > > > > --- /dev/null\n> > > > > +++ b/include/libcamera/orientation.h\n> > > > > @@ -0,0 +1,28 @@\n> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2023, Ideas On Board Oy\n> > > > > + *\n> > > > > + * orientation.h - Image orientation\n> > > > > + */\n> > > > > +\n> > > > > +#pragma once\n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +namespace libcamera {\n> > > > > +\n> > > > > +enum class Orientation {\n> > > > > +\t/* EXIF tag 274 starts from '1' */\n> > > > > +\trotate0 = 1,\n> > > >\n> > > > Enumerators should start with an uppercase letter.\n> > > >\n> > > > > +\trotate0Flip,\n> > > > > +\trotate180,\n> > > > > +\trotate180Flip,\n> > > > > +\trotate90Flip,\n> > > > > +\trotate270,\n> > > >\n> > > > I think this should be rotate90.\n> > >\n> > > Ah!\n> > >\n> > > The EXIF tag 6 is described as\n> > >\n> > > The 0th row is the visual right-hand side of the image, and the 0th\n> > > column is the visual top.\n> >\n> > Looks like I've misread it then, 270 is right.\n> >\n> > > > > +\trotate270Flip,\n> > > > > +\trotate90,\n> > > >\n> > > > And here, rotate270.\n> > >\n> > > EXIF tag 8\n> > >\n> > > The 0th row is the visual left-hand side of the image, and the 0th\n> > > column is the visual bottom.\n> > >\n> > > I'll correct both\n> >\n> > No need to :-)\n> >\n> \n> I keep reading the EXIF descriptions wrong\n> \n> The \"Oth row\" refers to what is displayed, not the first row of the\n> image...\n> \n> At least it seems like I finally got them right two months ago\n\n:-)\n\n> > > > > +};\n> > > > > +\n> > > > > +std::ostream &operator<<(std::ostream &out, const Orientation &orientation);\n> > > > > +\n> > > > > +} /* namespace libcamera */\n> > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > > > index 0eecee766f00..d4ad4a553752 100644\n> > > > > --- a/src/libcamera/camera.cpp\n> > > > > +++ b/src/libcamera/camera.cpp\n> > > > > @@ -160,7 +160,8 @@ LOG_DECLARE_CATEGORY(Camera)\n> > > > > * \\brief Create an empty camera configuration\n> > > > > */\n> > > > > CameraConfiguration::CameraConfiguration()\n> > > > > -\t: transform(Transform::Identity), config_({})\n> > > > > +\t: transform(Transform::Identity), orientation(Orientation::rotate0),\n> > > > > +\t config_({})\n> > > > > {\n> > > > > }\n> > > > >\n> > > > > @@ -404,6 +405,21 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF\n> > > > > * may adjust this field at its discretion if the selection is not supported.\n> > > > > */\n> > > > >\n> > > > > +/**\n> > > > > + * \\var CameraConfiguration::orientation\n> > > > > + * \\brief The desired orientation of the images produced by the camera\n> > > > > + *\n> > > > > + * The orientation field is a user-specified 2D plane transformation that\n> > > >\n> > > > s/field/member/\n> > > >\n> > > > The word \"field\" in the C++ specification is used for bit-field only.\n> > > > \"fields\" of a class or structure are named \"members\" (or \"data member\"\n> > > > if you want to differentiate them from member functions).\n> > > >\n> > > > > + * specifies how the application wants the camera images to be rotated in\n> > > > > + * the memory buffers.\n> > > >\n> > > > As the whole point of this series is to move from transform to\n> > > > orientation, can we try to avoid using \"transformation\" to define the\n> > > > orientation ? How about the following ?\n> > > >\n> > > > * The orientation specifies how the application wants images to be oriented in\n> > > > * memory buffers. The camera will rotate and flip images as appropriate to\n> > > > * obtain the desired orientation.\n> > >\n> > > I think there's difference between the Transform we want to move away\n> > > from and a 2d plane transformation, which is a specific term, but if\n> > > just seeing \"transformation\" spelled out irritates you.. ok...\n> >\n> > Yuo're right that the word \"transformation\" has a more general meaning,\n> > I didn't think about it during the review. We could indeed keep using\n> > it, when it's clear we're not referring to the Transform enum.\n> >\n> > > > > + *\n> > > > > + * If the application requested orientation cannot be obtained the validate()\n> > > >\n> > > > s/obtained/obtained,/\n> > > >\n> > > > > + * function will Adjust this field to the actual orientation of the images\n> > > >\n> > > > s/this field/the orientation member/\n> > > >\n> > > > > + * as produced by the camera pipeline.\n> > > >\n> > > > I recall we discussed that, in this case, the camera shouldn't try to\n> > > > pick a value it considers the closest to the requested orientation, but\n> > > > don't transform the image at all. Is that right ? If so, let's document\n> > > > it explicitly:\n> > >\n> > > Yes, that's what the proposed documentation says\n> > >\n> > > * If the application requested orientation cannot be obtained the validate()\n> > > * function will Adjust this field to the actual orientation of the images\n> > > * as produced by the camera pipeline.\n> >\n> > \"as produced by the camera pipeline\" means that the orientation field\n> > gets adjusted to reflect what the camera produces, but it doesn't tell\n> > if the camera can/should still apply a transformation.\n> >\n> \n> Oh, I see why it was confusing now\n> \n> > > > * If the orientation requested by the application cannot be obtained, the\n> > > > * camera will not rotate or flip the images, and the validate() function will\n> > > > * Adjust this value to the native image orientation produced by the camera.\n> > >\n> > > The two says the same thing, but.. ok\n> > >\n> > > > > + *\n> > > > > + * By default the orientation field is set to Orientation::rotate0.\n> > > >\n> > > > s/field/member/\n> > > >\n> > > > > + */\n> > > > > +\n> > > > > /**\n> > > > > * \\var CameraConfiguration::config_\n> > > > > * \\brief The vector of stream configurations\n> > > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > > > index b24f82965764..d0e26f6b4141 100644\n> > > > > --- a/src/libcamera/meson.build\n> > > > > +++ b/src/libcamera/meson.build\n> > > > > @@ -34,6 +34,7 @@ libcamera_sources = files([\n> > > > > 'mapped_framebuffer.cpp',\n> > > > > 'media_device.cpp',\n> > > > > 'media_object.cpp',\n> > > > > + 'orientation.cpp',\n> > > > > 'pipeline_handler.cpp',\n> > > > > 'pixel_format.cpp',\n> > > > > 'process.cpp',\n> > > > > diff --git a/src/libcamera/orientation.cpp b/src/libcamera/orientation.cpp\n> > > > > new file mode 100644\n> > > > > index 000000000000..f2ee14dd4182\n> > > > > --- /dev/null\n> > > > > +++ b/src/libcamera/orientation.cpp\n> > > > > @@ -0,0 +1,78 @@\n> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2023, Ideas On Board Oy\n> > > > > + *\n> > > > > + * orientation.cpp - Image orientation\n> > > > > + */\n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +#include \n> > > > > +#include \n> > > > > +\n> > > > > +/**\n> > > > > + * \\file libcamera/orientation.h\n> > > > > + * \\brief Image orientation definition\n> > > > > + */\n> > > > > +\n> > > > > +namespace libcamera {\n> > > > > +\n> > > > > +/**\n> > > > > + * \\enum Orientation\n> > > > > + * \\brief The image orientation in a memory buffer\n> > > > > + *\n> > > > > + * The Orientation enumeration describes the orientation of the images\n> > > > > + * produced by the camera pipeline as they get received by the application\n> > > > > + * inside memory buffers.\n> > > > > + *\n> > > >\n> > > > From here to ...\n> > > >\n> > > > > + * All the possible 2D plane transformations of an image are expressed as the\n> > > > > + * combination of two basic operations:\n> > > > > + *\n> > > > > + * 'a': rotate the image 90 degrees clockwise\n> > > > > + * 'b': flip the image horizontally (mirroring)\n> > > > > + *\n> > > > > + * plus an identity operator 'e'.\n> > > > > + *\n> > > > > + * The composition of operations 'a' and 'b' according to the canonical function\n> > > > > + * composition notion 'b * a' (where 'a' is applied first, then 'b' is applied\n> > > > > + * next) gives origin to a symmetric group of order 8, or dihedral group.\n> > > > > + *\n> > > > > + * See https://en.wikipedia.org/wiki/Dihedral_group#/media/File:Dih4_cycle_graph.svg\n> > > > > + * as an example of the 2D plane transformation and how they originate from\n> > > > > + * the composition of the 'a' and 'b' operations.\n> > > > > + *\n> > > > > + * The image orientation expressed using the Orientation enumeration can be\n> > > > > + * then inferred by applying a multiple of a 90 degrees rotation from the origin\n> > > > > + * and then applying any horizontal mirroring to a base image assume to be\n> > > > > + * naturally oriented (no rotation and no mirroring applied).\n> > > >\n> > > > ... here, is this a leftover of a previous implementation, or copied\n> > > > from somewhere else ? It doesn't seem to match the enumeration.\n> > >\n> > > Why it doesn't match the enumeration ? The enumerated members are\n> > > named as \"90 degrees rotation\" + \"optional horizontal mirroring\".\n> > >\n> > > What is wrong here ?\n> >\n> > The text above is not incorrect, but reading it, it appears barely\n> > connected to the enumerators, and I think it's also overly complicated.\n> \n> When it comes to being possibily too complicated, I agree :)\n> \n> I tried to keep on par with David's documentation about Transform\n> which provided a bit more of a formal background.\n> \n> Is it ok to remove the part about the the dihedral group but keep the\n> last paragraph (\"The image orientation expressed ..\") or should it be\n> removed as well ?\n\nI think that's fine for v6. I'll experiment with a few changes on top as\ndiscussed in other e-mails in this thread, and if I can find ways to\nimprove the documentation, I'll do so as well. You should be freed from\nthe hassle of a v7 :-)\n\n> > > > > + *\n> > > > > + * The enumeration numerical values follow the ones defined by the EXIF\n> > > > > + * Specification version 2.32, Tag 274 \"Orientation\", while the names of the\n> > > > > + * enumerated values report the rotation and mirroring operations performed.\n> > > > > + *\n> > > > > + * In example Orientation::rotate90Flip describes the image transformation\n> > > >\n> > > > s/In example/For example,/\n> > > >\n> > > > > + * obtained by rotating 90 degrees clockwise first and then applying an\n> > > > > + * horizontal mirroring.\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Prints human-friendly names for Orientation items\n> > > >\n> > > > Let's be consistent with the other operator<<() implementations:\n> > > >\n> > > > * \\brief Insert a text representation of an Orientation into an output stream\n> > > >\n> > > > > + * \\param[in] out The output stream\n> > > > > + * \\param[in] orientation The Orientation item\n> > > >\n> > > > s/ item//\n> > > >\n> > > > > + * \\return The output stream \\a out\n> > > > > + */\n> > > > > +std::ostream &operator<<(std::ostream &out, const Orientation &orientation)\n> > > > > +{\n> > > > > +\tconstexpr std::array orientationNames = {\n> > > > > +\t\t\"\", /* Orientation starts counting from 1. */\n> > > > > +\t\t\"rotate0\", \"rotate0Flip\",\n> > > > > +\t\t\"rotate180\", \"rotate180Flip\",\n> > > > > +\t\t\"rotate90Flip\", \"rotate270\",\n> > > > > +\t\t\"rotate270Flip\", \"rotate90\",\n> > > > > +\t};\n> > > > > +\n> > > > > +\tout << orientationNames[static_cast(orientation)];\n> > > >\n> > > > You can write\n> > > >\n> > > > \t/* The Orientation enumeration starts counting from 1. */\n> > > > \tout << orientationNames[static_cast(orientation) - 1];\n> > > >\n> > > > And drop the first entry from the array.\n> > > >\n> > > > > +\treturn out;\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 DF527BDCBD\n\tfor ;\n\tThu, 19 Oct 2023 13:54:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 52D276297F;\n\tThu, 19 Oct 2023 15:54:29 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 14A786055B\n\tfor ;\n\tThu, 19 Oct 2023 15:54: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 E9D7E25A;\n\tThu, 19 Oct 2023 15:54:19 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1697723669;\n\tbh=Vb2piGMBSr7I+NfavKOTFfjqZLV9PfFsSVXYPLoKRag=;\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=bo22Gngk1tUfAhqwKBjVSlZ9MkArJKs0qqWM+usxObUkZmYGtxZlPZO3D/OSwMf0V\n\tdFF6jFvCRm0y6MNz6L1JGQzdMDNZ8bcUPo7oLTZgTp4GMBu/uLjO/MvEUE4HcCEmDl\n\tqkfxULuWrV6E5dSXhNxtfsf7xdud5Rl3M9x4MIfI29aXoO2ym7/Tr4ouI4LchUCKpn\n\t4pRosjfVrg1ec0BVfTv3JowunGWOuqzJ+edNkyhFlkupBGtcaEpZF9/Z5EhvHaO74x\n\tpiZ/quFU+DZgzaEPTdjKT7HqUJWHw+4lVaP+emhL3dKrTQlOk1UnrLub2cuWK9C91E\n\tpf4afr3qLIN3Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1697723660;\n\tbh=Vb2piGMBSr7I+NfavKOTFfjqZLV9PfFsSVXYPLoKRag=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AfPwDGxe2enU2cFUxk+POQXSJOoMwKrxKD05yuGUsdK9mANGxMoVruDehqh/D3kvx\n\trroWkkMGtij9RsrU6honm/3ybqkHHWrUT+r+nfJ9SBdfV7dK70YFE2mLlVYHGSjMCN\n\t+Q/oLKuJayLOJ68SizwfRhGxcc24QJp1GFjdA5C8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"AfPwDGxe\"; dkim-atps=neutral","Date":"Thu, 19 Oct 2023 16:54:34 +0300","To":"Jacopo Mondi ","Message-ID":"<20231019135434.GK14832@pendragon.ideasonboard.com>","References":"<20230901150215.11585-1-jacopo.mondi@ideasonboard.com>\n\t<20230901150215.11585-3-jacopo.mondi@ideasonboard.com>\n\t<20231018183710.GA1512@pendragon.ideasonboard.com>\n\t\n\t<20231019083752.GY1512@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 02/12] libcamera: camera: Introduce\n\tOrientation","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\" "}}]