[{"id":27557,"web_url":"https://patchwork.libcamera.org/comment/27557/","msgid":"<CAHW6GYLSZJ6PEwDTefFF2DdPFDT+bV8-YmoTVBUFwkPnf6yG0w@mail.gmail.com>","date":"2023-07-17T09:46:04","subject":"Re: [libcamera-devel] [PATCH v2 5/9] libcamera: transform: Add\n\tfunctions to convert Orientation","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nThanks for the patch.\n\nOn Fri, 14 Jul 2023 at 15:16, Jacopo Mondi\n<jacopo.mondi@ideasonboard.com> wrote:\n>\n> Add two helper functions to the transform.cpp file that allows to\n> convert to and from an Orientation.\n>\n> As transform.h now needs to include camera.h remove \"#include\n> <transform.h>\" from camera.h to avoid a cyclic inclusion loop.\n>\n> Adjust all files that need to include transform.h directly and didn't do\n> so because it was implicitly included by camera.h.\n>\n> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n\nThe functions here seem fine to me, I guess I just wanted to ask the\nquestion about include files. Part of me feels that transforms are\nkind of like orientations, so if transforms have their own header,\nmaybe orientations should too? Transforms seem, to me at least, like\nreally simple things, so dragging in camera.h felt a bit weird (even\nthough, in practice, an application would probably do so anyway).\n\nBut honestly, I'm not sure. It was just the lack of symmetry between\nthe two that left me feeling slightly uncomfortable.\n\nAnyway, subject to some saying they like it how it is:\n\nReviewed-by: David Plowman <david.plowman@raspberrypi.com>\n\nThanks!\nDavid\n\n> ---\n>  include/libcamera/camera.h                   |  3 +-\n>  include/libcamera/transform.h                |  4 ++\n>  src/libcamera/camera.cpp                     |  1 +\n>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp |  1 +\n>  src/libcamera/transform.cpp                  | 58 ++++++++++++++++++++\n>  5 files changed, 66 insertions(+), 1 deletion(-)\n>\n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index 8132ef2506a4..55359d53f2ab 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -22,7 +22,6 @@\n>  #include <libcamera/controls.h>\n>  #include <libcamera/request.h>\n>  #include <libcamera/stream.h>\n> -#include <libcamera/transform.h>\n>\n>  namespace libcamera {\n>\n> @@ -31,6 +30,8 @@ class FrameBufferAllocator;\n>  class PipelineHandler;\n>  class Request;\n>\n> +enum class Transform;\n> +\n>  class CameraConfiguration\n>  {\n>  public:\n> diff --git a/include/libcamera/transform.h b/include/libcamera/transform.h\n> index 2a6838c78369..573adf18715d 100644\n> --- a/include/libcamera/transform.h\n> +++ b/include/libcamera/transform.h\n> @@ -9,6 +9,8 @@\n>\n>  #include <string>\n>\n> +#include <libcamera/camera.h>\n> +\n>  namespace libcamera {\n>\n>  enum class Transform : int {\n> @@ -69,6 +71,8 @@ constexpr Transform operator~(Transform t)\n>  }\n>\n>  Transform transformFromRotation(int angle, bool *success = nullptr);\n> +Transform transformFromOrientation(const CameraConfiguration::Orientation &orientation);\n> +CameraConfiguration::Orientation transformToOrientation(const Transform &transform);\n>\n>  const char *transformToString(Transform t);\n>\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index c8bb56d4af7f..af842e70dcb0 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -18,6 +18,7 @@\n>  #include <libcamera/framebuffer_allocator.h>\n>  #include <libcamera/request.h>\n>  #include <libcamera/stream.h>\n> +#include <libcamera/transform.h>\n>\n>  #include \"libcamera/internal/camera.h\"\n>  #include \"libcamera/internal/camera_controls.h\"\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 38f48a5d9269..02a6117c7955 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -21,6 +21,7 @@\n>  #include <libcamera/property_ids.h>\n>  #include <libcamera/request.h>\n>  #include <libcamera/stream.h>\n> +#include <libcamera/transform.h>\n>\n>  #include \"libcamera/internal/camera.h\"\n>  #include \"libcamera/internal/device_enumerator.h\"\n> diff --git a/src/libcamera/transform.cpp b/src/libcamera/transform.cpp\n> index 4668303d0676..03d2b52e7f38 100644\n> --- a/src/libcamera/transform.cpp\n> +++ b/src/libcamera/transform.cpp\n> @@ -299,6 +299,64 @@ Transform transformFromRotation(int angle, bool *success)\n>         return Transform::Identity;\n>  }\n>\n> +/**\n> + * \\brief Return the transform representing \\a orientation\n> + * \\param[in] orientation The orientation to convert\n> + * \\return The transform corresponding to \\a orientation\n> + */\n> +Transform transformFromOrientation(const CameraConfiguration::Orientation &orientation)\n> +{\n> +       switch (orientation) {\n> +       case CameraConfiguration::rotate0:\n> +               return Transform::Identity;\n> +       case CameraConfiguration::rotate0Flip:\n> +               return Transform::HFlip;\n> +       case CameraConfiguration::rotate180:\n> +               return Transform::Rot180;\n> +       case CameraConfiguration::rotate180Flip:\n> +               return Transform::VFlip;\n> +       case CameraConfiguration::rotate90Flip:\n> +               return Transform::Transpose;\n> +       case CameraConfiguration::rotate90:\n> +               return Transform::Rot90;\n> +       case CameraConfiguration::rotate270Flip:\n> +               return Transform::Rot180Transpose;\n> +       case CameraConfiguration::rotate270:\n> +               return Transform::Rot270;\n> +       }\n> +\n> +       return Transform::Identity;\n> +}\n> +\n> +/**\n> + * \\brief Return the CameraConfiguration::Orientation representing \\a transform\n> + * \\param[in] transform The transform to convert\n> + * \\return The Orientation corresponding to \\a transform\n> + */\n> +CameraConfiguration::Orientation transformToOrientation(const Transform &transform)\n> +{\n> +       switch (transform) {\n> +       case Transform::Identity:\n> +               return CameraConfiguration::rotate0;\n> +       case Transform::HFlip:\n> +               return CameraConfiguration::rotate0Flip;\n> +       case Transform::VFlip:\n> +               return CameraConfiguration::rotate180Flip;\n> +       case Transform::Rot180:\n> +               return CameraConfiguration::rotate180;\n> +       case Transform::Transpose:\n> +               return CameraConfiguration::rotate90Flip;\n> +       case Transform::Rot270:\n> +               return CameraConfiguration::rotate270;\n> +       case Transform::Rot90:\n> +               return CameraConfiguration::rotate90;\n> +       case Transform::Rot180Transpose:\n> +               return CameraConfiguration::rotate270Flip;\n> +       }\n> +\n> +       return CameraConfiguration::rotate0;\n> +}\n> +\n>  /**\n>   * \\brief Return a character string describing the transform\n>   * \\param[in] t The transform to be described.\n> --\n> 2.40.1\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 2281CBDC71\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 17 Jul 2023 09:46:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 69D0D61E2B;\n\tMon, 17 Jul 2023 11:46:18 +0200 (CEST)","from mail-oo1-xc2b.google.com (mail-oo1-xc2b.google.com\n\t[IPv6:2607:f8b0:4864:20::c2b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3F87E60381\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 17 Jul 2023 11:46:16 +0200 (CEST)","by mail-oo1-xc2b.google.com with SMTP id\n\t006d021491bc7-55b8f1c930eso2694267eaf.3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 17 Jul 2023 02:46:16 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1689587178;\n\tbh=DFPBFg1X3yk8bVjw/sh4p55iqkXvvm7dfsmafjCazhw=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=iODX6rRtbW1WsiQzFL4cBvlYx++RnfnlV4hnjQPdyzuTmOECMep9nU3A96fRnU1Yl\n\tCUwNCh32HVFInm0J+suIY5JtzZOkTM079Oli8jZWFdScUwLXOD7DB3Y90ultgi1Zeq\n\tIPS9ySwNCRTsaefnC8HN7ZA2/jurXA++eNz08oJAiPWe24HjUOK6tLC4MR2U4CcqyK\n\tAIv0jDh/xJYqIhHv+WjDmFGVtAdbBsAWHUkVrTGfeJT1QOLWiZ5gq0NUYBHmnV9vnT\n\tVITvYzxl7Gvg3P398cf7hLZ+zShldL4CrzmEJt7NtnSbxbgCxeVr+00ewXvc1pQyFq\n\tt3C5CCxgTJGkQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1689587175; x=1692179175;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=/AzVLbGlgT3zoelQo+E+I15IFmVTXukYMJuRXuHJcZQ=;\n\tb=e+CH7OZ/M+lSF5tQcCvfEHlbmrX0yiaxdLJBSNVbaZ617Ya8gZC0RHjkb/kEQHqo7a\n\tUzv7S0N4KyVvnF4pkKZiq1yBvHcukS8f++VyA9eFloVYSaiJ4r6B2OhS3Rys/8wsreX5\n\t+CCAxm4yiNtwf49Wd3zUF3l5FwmkoMsizfJaFYFWMAHXASZgR7KDZg6sqnZAHeHsah46\n\tmfn3h67IWkKB0/VxEqwM1RnoX4ZZwZvhOpN89c/HAtZdTDhrcH0ImSZsb7E3Nymk3ksV\n\tmDagG7a6yZn867Wt9wdy+0bpIHBVzwK8/On4h2ND7R3HOPlId/UpHzsodz0DRCTJr++q\n\tG33A=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"e+CH7OZ/\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1689587175; x=1692179175;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=/AzVLbGlgT3zoelQo+E+I15IFmVTXukYMJuRXuHJcZQ=;\n\tb=BRCH6U4uRgJDQuft6dOgzFTifB3sQJ2/j5Tgm1sZtJ8JEiJyZRcTNIOBlcQIH9HH+E\n\tP6ayuRmUy3O8QHuB1S9gzsJAxLeR55kcoNvDRcb+6HrMDIy0jFmV9ibSLHiIzwEgiQF+\n\tRu6jq2oSr2HETGcaOw016a4y+RgkNzB8sgs/D1ykGecNndJnXOQo6q+jNNWMQR9SZ1aw\n\tDiYRqg+rgkLyvOtj2vxi0YHoYjbluCX58xhrW/ASbQS8UZ8jhr63zyRRScqZdCzV/RyP\n\tCusi9hYkjRusRzCU47mcSgmndAsamYj+8Rryk5zzJ+ShbdOkzEl9XrIqSi5m8SzzqkIZ\n\t0ucw==","X-Gm-Message-State":"ABy/qLZ+/JUzFnbD6Olq3lM3qcSg8W880aHO7S5QcLOvRSj4eg/njlIL\n\tnRNOPPYhFo/GNTLLKfQ8ZaWYyKM6XyupRormJ4exXw==","X-Google-Smtp-Source":"APBJJlFT3auI0x2JrfEVinemqzZ00w2kGjue/0Ap1oputmCHZ/gvwVDzQTYp2PJEkBeSBWehdYjhojnqp/n3T46YCQE=","X-Received":"by 2002:a05:6358:2815:b0:134:c785:7ea6 with SMTP id\n\tk21-20020a056358281500b00134c7857ea6mr11821590rwb.23.1689587174844;\n\tMon, 17 Jul 2023 02:46:14 -0700 (PDT)","MIME-Version":"1.0","References":"<20230714141549.11085-1-jacopo.mondi@ideasonboard.com>\n\t<20230714141549.11085-6-jacopo.mondi@ideasonboard.com>","In-Reply-To":"<20230714141549.11085-6-jacopo.mondi@ideasonboard.com>","Date":"Mon, 17 Jul 2023 10:46:04 +0100","Message-ID":"<CAHW6GYLSZJ6PEwDTefFF2DdPFDT+bV8-YmoTVBUFwkPnf6yG0w@mail.gmail.com>","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 5/9] libcamera: transform: Add\n\tfunctions to convert Orientation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"David Plowman via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"David Plowman <david.plowman@raspberrypi.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":27561,"web_url":"https://patchwork.libcamera.org/comment/27561/","msgid":"<e7qazbv3vvttq5lmgklg6n5xxelc24dfe3wcrtudmvcpqcltjg@a7dptbtf32li>","date":"2023-07-17T12:53:52","subject":"Re: [libcamera-devel] [PATCH v2 5/9] libcamera: transform: Add\n\tfunctions to convert Orientation","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi David\n\nOn Mon, Jul 17, 2023 at 10:46:04AM +0100, David Plowman via libcamera-devel wrote:\n> Hi Jacopo\n>\n> Thanks for the patch.\n>\n> On Fri, 14 Jul 2023 at 15:16, Jacopo Mondi\n> <jacopo.mondi@ideasonboard.com> wrote:\n> >\n> > Add two helper functions to the transform.cpp file that allows to\n> > convert to and from an Orientation.\n> >\n> > As transform.h now needs to include camera.h remove \"#include\n> > <transform.h>\" from camera.h to avoid a cyclic inclusion loop.\n> >\n> > Adjust all files that need to include transform.h directly and didn't do\n> > so because it was implicitly included by camera.h.\n> >\n> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n>\n> The functions here seem fine to me, I guess I just wanted to ask the\n> question about include files. Part of me feels that transforms are\n> kind of like orientations, so if transforms have their own header,\n> maybe orientations should too? Transforms seem, to me at least, like\n\nI considered this, but so far Orientation is a simple enum, I didn't\nconsider it enough to break it out to a dedicated file..\n\n> really simple things, so dragging in camera.h felt a bit weird (even\n> though, in practice, an application would probably do so anyway).\n>\n\nLooking ahead to the C API introduction, Transform seems to be more\nopportunely placed in the C++ API which will be built on top of the\nC one.\n\nNow, as you correctly noticed, including transform.h will drag in\ncamera.h. As the camera orientation lives in CameraConfiguration it\nreally seemed un-likely for an application to use Transform without\nincluding camera.h\n\nActually, I initially wanted to forward declare Orientation and avoid\nincluding <camera.h> in transform.h. Unfrtunately forward-declaring\ninner types in C++ is not possible. One possible option would be to\nmove Orientation out of CameraConfiguration, possibily to its own\nfile as you were suggesting. This would remove the need to include\n<camera.h>\n\n\n> But honestly, I'm not sure. It was just the lack of symmetry between\n> the two that left me feeling slightly uncomfortable.\n>\n> Anyway, subject to some saying they like it how it is:\n\nYeah opinions welcome!\n\n>\n> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>\n>\n> Thanks!\n> David\n>\n> > ---\n> >  include/libcamera/camera.h                   |  3 +-\n> >  include/libcamera/transform.h                |  4 ++\n> >  src/libcamera/camera.cpp                     |  1 +\n> >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp |  1 +\n> >  src/libcamera/transform.cpp                  | 58 ++++++++++++++++++++\n> >  5 files changed, 66 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > index 8132ef2506a4..55359d53f2ab 100644\n> > --- a/include/libcamera/camera.h\n> > +++ b/include/libcamera/camera.h\n> > @@ -22,7 +22,6 @@\n> >  #include <libcamera/controls.h>\n> >  #include <libcamera/request.h>\n> >  #include <libcamera/stream.h>\n> > -#include <libcamera/transform.h>\n> >\n> >  namespace libcamera {\n> >\n> > @@ -31,6 +30,8 @@ class FrameBufferAllocator;\n> >  class PipelineHandler;\n> >  class Request;\n> >\n> > +enum class Transform;\n> > +\n> >  class CameraConfiguration\n> >  {\n> >  public:\n> > diff --git a/include/libcamera/transform.h b/include/libcamera/transform.h\n> > index 2a6838c78369..573adf18715d 100644\n> > --- a/include/libcamera/transform.h\n> > +++ b/include/libcamera/transform.h\n> > @@ -9,6 +9,8 @@\n> >\n> >  #include <string>\n> >\n> > +#include <libcamera/camera.h>\n> > +\n> >  namespace libcamera {\n> >\n> >  enum class Transform : int {\n> > @@ -69,6 +71,8 @@ constexpr Transform operator~(Transform t)\n> >  }\n> >\n> >  Transform transformFromRotation(int angle, bool *success = nullptr);\n> > +Transform transformFromOrientation(const CameraConfiguration::Orientation &orientation);\n> > +CameraConfiguration::Orientation transformToOrientation(const Transform &transform);\n> >\n> >  const char *transformToString(Transform t);\n> >\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index c8bb56d4af7f..af842e70dcb0 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -18,6 +18,7 @@\n> >  #include <libcamera/framebuffer_allocator.h>\n> >  #include <libcamera/request.h>\n> >  #include <libcamera/stream.h>\n> > +#include <libcamera/transform.h>\n> >\n> >  #include \"libcamera/internal/camera.h\"\n> >  #include \"libcamera/internal/camera_controls.h\"\n> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > index 38f48a5d9269..02a6117c7955 100644\n> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > @@ -21,6 +21,7 @@\n> >  #include <libcamera/property_ids.h>\n> >  #include <libcamera/request.h>\n> >  #include <libcamera/stream.h>\n> > +#include <libcamera/transform.h>\n> >\n> >  #include \"libcamera/internal/camera.h\"\n> >  #include \"libcamera/internal/device_enumerator.h\"\n> > diff --git a/src/libcamera/transform.cpp b/src/libcamera/transform.cpp\n> > index 4668303d0676..03d2b52e7f38 100644\n> > --- a/src/libcamera/transform.cpp\n> > +++ b/src/libcamera/transform.cpp\n> > @@ -299,6 +299,64 @@ Transform transformFromRotation(int angle, bool *success)\n> >         return Transform::Identity;\n> >  }\n> >\n> > +/**\n> > + * \\brief Return the transform representing \\a orientation\n> > + * \\param[in] orientation The orientation to convert\n> > + * \\return The transform corresponding to \\a orientation\n> > + */\n> > +Transform transformFromOrientation(const CameraConfiguration::Orientation &orientation)\n> > +{\n> > +       switch (orientation) {\n> > +       case CameraConfiguration::rotate0:\n> > +               return Transform::Identity;\n> > +       case CameraConfiguration::rotate0Flip:\n> > +               return Transform::HFlip;\n> > +       case CameraConfiguration::rotate180:\n> > +               return Transform::Rot180;\n> > +       case CameraConfiguration::rotate180Flip:\n> > +               return Transform::VFlip;\n> > +       case CameraConfiguration::rotate90Flip:\n> > +               return Transform::Transpose;\n> > +       case CameraConfiguration::rotate90:\n> > +               return Transform::Rot90;\n> > +       case CameraConfiguration::rotate270Flip:\n> > +               return Transform::Rot180Transpose;\n> > +       case CameraConfiguration::rotate270:\n> > +               return Transform::Rot270;\n> > +       }\n> > +\n> > +       return Transform::Identity;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Return the CameraConfiguration::Orientation representing \\a transform\n> > + * \\param[in] transform The transform to convert\n> > + * \\return The Orientation corresponding to \\a transform\n> > + */\n> > +CameraConfiguration::Orientation transformToOrientation(const Transform &transform)\n> > +{\n> > +       switch (transform) {\n> > +       case Transform::Identity:\n> > +               return CameraConfiguration::rotate0;\n> > +       case Transform::HFlip:\n> > +               return CameraConfiguration::rotate0Flip;\n> > +       case Transform::VFlip:\n> > +               return CameraConfiguration::rotate180Flip;\n> > +       case Transform::Rot180:\n> > +               return CameraConfiguration::rotate180;\n> > +       case Transform::Transpose:\n> > +               return CameraConfiguration::rotate90Flip;\n> > +       case Transform::Rot270:\n> > +               return CameraConfiguration::rotate270;\n> > +       case Transform::Rot90:\n> > +               return CameraConfiguration::rotate90;\n> > +       case Transform::Rot180Transpose:\n> > +               return CameraConfiguration::rotate270Flip;\n> > +       }\n> > +\n> > +       return CameraConfiguration::rotate0;\n> > +}\n> > +\n> >  /**\n> >   * \\brief Return a character string describing the transform\n> >   * \\param[in] t The transform to be described.\n> > --\n> > 2.40.1\n> >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 0D82FBDC71\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 17 Jul 2023 12:53:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9F31D628C0;\n\tMon, 17 Jul 2023 14:53:57 +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 A3E8960383\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 17 Jul 2023 14:53:56 +0200 (CEST)","from ideasonboard.com (mob-5-90-54-150.net.vodafone.it\n\t[5.90.54.150])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 15DFA331B;\n\tMon, 17 Jul 2023 14:53:02 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1689598437;\n\tbh=NnBNqkzcEia9Eh/EqlA6wshnorWENaEzqBe8q1wcmQs=;\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=f4gauXvFM+5oIVNTYp7iWD/B/WIaD1gH7CrMUq9/X7SQgnzHj90qeJRMfgnSKd4xg\n\tLxv4ctQ37S9ALe8zf5SJLqN8rZpeMJF3o5LygFGSU/E5kxHvgXOtzmps+C+/co1ytA\n\t59cBRN2xLNI4WxF899uj0/67PoTe8cSrC+vtqh0iQa1H2rDMY4XYWCAiv/rI/TOee+\n\thmeE+m7rE2oyzfmK8ZzTUif0grwiuJ+Wwzy7aozxaUrq5N84mfk94GoZjjamY5pCRJ\n\tzTAcEAVZ8CIYauPWLXU4hLk9I3odvxP+y1JJEsx++pvVfFxISKY523rrfeDgXrcLud\n\tsuVdjNo4wbCgg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1689598383;\n\tbh=NnBNqkzcEia9Eh/EqlA6wshnorWENaEzqBe8q1wcmQs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=g2cOffCiPvV0bMznUarPa9kvUJ2Lr+duW8u2Y7Ee67yOjFk9KzA7YPKeh4dxM/3Rx\n\t+4ZMVZ5vycDrEFz6fHoHHguS6tOwZpVv0VQlWm2MVKyfIaElELEwCvAGB6IzRzGk96\n\tpARAzgGD7/HUKBwq8VXDcVZj+O9+c9vgGeAW9ad8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"g2cOffCi\"; dkim-atps=neutral","Date":"Mon, 17 Jul 2023 14:53:52 +0200","To":"David Plowman <david.plowman@raspberrypi.com>","Message-ID":"<e7qazbv3vvttq5lmgklg6n5xxelc24dfe3wcrtudmvcpqcltjg@a7dptbtf32li>","References":"<20230714141549.11085-1-jacopo.mondi@ideasonboard.com>\n\t<20230714141549.11085-6-jacopo.mondi@ideasonboard.com>\n\t<CAHW6GYLSZJ6PEwDTefFF2DdPFDT+bV8-YmoTVBUFwkPnf6yG0w@mail.gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<CAHW6GYLSZJ6PEwDTefFF2DdPFDT+bV8-YmoTVBUFwkPnf6yG0w@mail.gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v2 5/9] libcamera: transform: Add\n\tfunctions to convert Orientation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Jacopo Mondi via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Cc":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":27564,"web_url":"https://patchwork.libcamera.org/comment/27564/","msgid":"<CAHW6GYK7Gaanpo29wyfpRh5SQNKRD-vooRj-6mU3ohVcECSb3A@mail.gmail.com>","date":"2023-07-17T13:35:31","subject":"Re: [libcamera-devel] [PATCH v2 5/9] libcamera: transform: Add\n\tfunctions to convert Orientation","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi again\n\nOn Mon, 17 Jul 2023 at 13:53, Jacopo Mondi\n<jacopo.mondi@ideasonboard.com> wrote:\n>\n> Hi David\n>\n> On Mon, Jul 17, 2023 at 10:46:04AM +0100, David Plowman via libcamera-devel wrote:\n> > Hi Jacopo\n> >\n> > Thanks for the patch.\n> >\n> > On Fri, 14 Jul 2023 at 15:16, Jacopo Mondi\n> > <jacopo.mondi@ideasonboard.com> wrote:\n> > >\n> > > Add two helper functions to the transform.cpp file that allows to\n> > > convert to and from an Orientation.\n> > >\n> > > As transform.h now needs to include camera.h remove \"#include\n> > > <transform.h>\" from camera.h to avoid a cyclic inclusion loop.\n> > >\n> > > Adjust all files that need to include transform.h directly and didn't do\n> > > so because it was implicitly included by camera.h.\n> > >\n> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n> >\n> > The functions here seem fine to me, I guess I just wanted to ask the\n> > question about include files. Part of me feels that transforms are\n> > kind of like orientations, so if transforms have their own header,\n> > maybe orientations should too? Transforms seem, to me at least, like\n>\n> I considered this, but so far Orientation is a simple enum, I didn't\n> consider it enough to break it out to a dedicated file..\n>\n> > really simple things, so dragging in camera.h felt a bit weird (even\n> > though, in practice, an application would probably do so anyway).\n> >\n>\n> Looking ahead to the C API introduction, Transform seems to be more\n> opportunely placed in the C++ API which will be built on top of the\n> C one.\n>\n> Now, as you correctly noticed, including transform.h will drag in\n> camera.h. As the camera orientation lives in CameraConfiguration it\n> really seemed un-likely for an application to use Transform without\n> including camera.h\n>\n> Actually, I initially wanted to forward declare Orientation and avoid\n> including <camera.h> in transform.h. Unfrtunately forward-declaring\n> inner types in C++ is not possible. One possible option would be to\n> move Orientation out of CameraConfiguration, possibily to its own\n> file as you were suggesting. This would remove the need to include\n> <camera.h>\n>\n>\n> > But honestly, I'm not sure. It was just the lack of symmetry between\n> > the two that left me feeling slightly uncomfortable.\n> >\n> > Anyway, subject to some saying they like it how it is:\n>\n> Yeah opinions welcome!\n\nAnother thing to consider is what happens in the Python bindings. Are\nwe going to have both orientations and transforms? I think you could\nget by with only transforms (and convert them implicitly when needed),\nor we could expose them there too. One to think about!\n\nDavid\n\n>\n> >\n> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>\n> >\n> > Thanks!\n> > David\n> >\n> > > ---\n> > >  include/libcamera/camera.h                   |  3 +-\n> > >  include/libcamera/transform.h                |  4 ++\n> > >  src/libcamera/camera.cpp                     |  1 +\n> > >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp |  1 +\n> > >  src/libcamera/transform.cpp                  | 58 ++++++++++++++++++++\n> > >  5 files changed, 66 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > > index 8132ef2506a4..55359d53f2ab 100644\n> > > --- a/include/libcamera/camera.h\n> > > +++ b/include/libcamera/camera.h\n> > > @@ -22,7 +22,6 @@\n> > >  #include <libcamera/controls.h>\n> > >  #include <libcamera/request.h>\n> > >  #include <libcamera/stream.h>\n> > > -#include <libcamera/transform.h>\n> > >\n> > >  namespace libcamera {\n> > >\n> > > @@ -31,6 +30,8 @@ class FrameBufferAllocator;\n> > >  class PipelineHandler;\n> > >  class Request;\n> > >\n> > > +enum class Transform;\n> > > +\n> > >  class CameraConfiguration\n> > >  {\n> > >  public:\n> > > diff --git a/include/libcamera/transform.h b/include/libcamera/transform.h\n> > > index 2a6838c78369..573adf18715d 100644\n> > > --- a/include/libcamera/transform.h\n> > > +++ b/include/libcamera/transform.h\n> > > @@ -9,6 +9,8 @@\n> > >\n> > >  #include <string>\n> > >\n> > > +#include <libcamera/camera.h>\n> > > +\n> > >  namespace libcamera {\n> > >\n> > >  enum class Transform : int {\n> > > @@ -69,6 +71,8 @@ constexpr Transform operator~(Transform t)\n> > >  }\n> > >\n> > >  Transform transformFromRotation(int angle, bool *success = nullptr);\n> > > +Transform transformFromOrientation(const CameraConfiguration::Orientation &orientation);\n> > > +CameraConfiguration::Orientation transformToOrientation(const Transform &transform);\n> > >\n> > >  const char *transformToString(Transform t);\n> > >\n> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > index c8bb56d4af7f..af842e70dcb0 100644\n> > > --- a/src/libcamera/camera.cpp\n> > > +++ b/src/libcamera/camera.cpp\n> > > @@ -18,6 +18,7 @@\n> > >  #include <libcamera/framebuffer_allocator.h>\n> > >  #include <libcamera/request.h>\n> > >  #include <libcamera/stream.h>\n> > > +#include <libcamera/transform.h>\n> > >\n> > >  #include \"libcamera/internal/camera.h\"\n> > >  #include \"libcamera/internal/camera_controls.h\"\n> > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > index 38f48a5d9269..02a6117c7955 100644\n> > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > @@ -21,6 +21,7 @@\n> > >  #include <libcamera/property_ids.h>\n> > >  #include <libcamera/request.h>\n> > >  #include <libcamera/stream.h>\n> > > +#include <libcamera/transform.h>\n> > >\n> > >  #include \"libcamera/internal/camera.h\"\n> > >  #include \"libcamera/internal/device_enumerator.h\"\n> > > diff --git a/src/libcamera/transform.cpp b/src/libcamera/transform.cpp\n> > > index 4668303d0676..03d2b52e7f38 100644\n> > > --- a/src/libcamera/transform.cpp\n> > > +++ b/src/libcamera/transform.cpp\n> > > @@ -299,6 +299,64 @@ Transform transformFromRotation(int angle, bool *success)\n> > >         return Transform::Identity;\n> > >  }\n> > >\n> > > +/**\n> > > + * \\brief Return the transform representing \\a orientation\n> > > + * \\param[in] orientation The orientation to convert\n> > > + * \\return The transform corresponding to \\a orientation\n> > > + */\n> > > +Transform transformFromOrientation(const CameraConfiguration::Orientation &orientation)\n> > > +{\n> > > +       switch (orientation) {\n> > > +       case CameraConfiguration::rotate0:\n> > > +               return Transform::Identity;\n> > > +       case CameraConfiguration::rotate0Flip:\n> > > +               return Transform::HFlip;\n> > > +       case CameraConfiguration::rotate180:\n> > > +               return Transform::Rot180;\n> > > +       case CameraConfiguration::rotate180Flip:\n> > > +               return Transform::VFlip;\n> > > +       case CameraConfiguration::rotate90Flip:\n> > > +               return Transform::Transpose;\n> > > +       case CameraConfiguration::rotate90:\n> > > +               return Transform::Rot90;\n> > > +       case CameraConfiguration::rotate270Flip:\n> > > +               return Transform::Rot180Transpose;\n> > > +       case CameraConfiguration::rotate270:\n> > > +               return Transform::Rot270;\n> > > +       }\n> > > +\n> > > +       return Transform::Identity;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Return the CameraConfiguration::Orientation representing \\a transform\n> > > + * \\param[in] transform The transform to convert\n> > > + * \\return The Orientation corresponding to \\a transform\n> > > + */\n> > > +CameraConfiguration::Orientation transformToOrientation(const Transform &transform)\n> > > +{\n> > > +       switch (transform) {\n> > > +       case Transform::Identity:\n> > > +               return CameraConfiguration::rotate0;\n> > > +       case Transform::HFlip:\n> > > +               return CameraConfiguration::rotate0Flip;\n> > > +       case Transform::VFlip:\n> > > +               return CameraConfiguration::rotate180Flip;\n> > > +       case Transform::Rot180:\n> > > +               return CameraConfiguration::rotate180;\n> > > +       case Transform::Transpose:\n> > > +               return CameraConfiguration::rotate90Flip;\n> > > +       case Transform::Rot270:\n> > > +               return CameraConfiguration::rotate270;\n> > > +       case Transform::Rot90:\n> > > +               return CameraConfiguration::rotate90;\n> > > +       case Transform::Rot180Transpose:\n> > > +               return CameraConfiguration::rotate270Flip;\n> > > +       }\n> > > +\n> > > +       return CameraConfiguration::rotate0;\n> > > +}\n> > > +\n> > >  /**\n> > >   * \\brief Return a character string describing the transform\n> > >   * \\param[in] t The transform to be described.\n> > > --\n> > > 2.40.1\n> > >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 1A05FBDB1C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 17 Jul 2023 13:35:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6E8A0628C1;\n\tMon, 17 Jul 2023 15:35:44 +0200 (CEST)","from mail-qv1-xf35.google.com (mail-qv1-xf35.google.com\n\t[IPv6:2607:f8b0:4864:20::f35])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3EA2B61E2C\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 17 Jul 2023 15:35:43 +0200 (CEST)","by mail-qv1-xf35.google.com with SMTP id\n\t6a1803df08f44-63719ad32e7so27216776d6.1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 17 Jul 2023 06:35:43 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1689600944;\n\tbh=RD7cQKYJYi0ZS0fsUiCHq/Cu/inxgUhcDCrMqVKTWn4=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Ec/+UrDSesndDtWHzF8yBA8yJbYTRsv+K+ZF97nR2+SBxMYLhvpXTrPYiaEf4YvAO\n\tTAajPX40SFnvUi/3hY16EhEFatD2RxZ/5hvNrKJxLF0WKjV8T5O98TSPAx52sLKd7H\n\tr0lbS4VUKtRJZ5YyJhsRjMteQ/d1te1WkIeEYHYCdgQHw1G1yVcUKHnj3uL7c4/rYv\n\tHzXC9TIzS8cRgxb2rGbLUXE0W4NjWq4hdEnow+dEVWNQn0glODR/ymC74Aw5T0NaQ7\n\tf/phRx2+/eswGX9Nnr0G5gY6HnExqCI+E1l2Xzb5D4GtLm4Uckbst4cUcgdSfSS/c3\n\t9IcNVvXRO2MOA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1689600942; x=1692192942;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=R+bUJWwuJxKvzU5KIn0Teb+SKzV+uN7Ki4TaPjMfOhM=;\n\tb=j5kADWJRL1BNS85xtBn0F3NZDCh2V45NBZAoGlEvhBil6iUm7DVyOfy6PEtCxPAkb+\n\tSauKyHfewSd3McXWnLj8k50NihnQIB4I9OrFC+KWVKON0pXndqtL+9yCsY0q8dZeec/E\n\tl4c0tR/J9JfV+JaSoQSefY7ZRk7aX6lhNkq/NJXH9NAbMBiLcFG96G1xG+CBppFqO58y\n\tuehGOn/z+tk6lmt4a2vmcR6tX4ryLvzBtjkGUE+OE78HbYFVAfL1CU8MpRT8U1GJmfdG\n\tRnlXD13Umth5DKqv2uO5kGja07KHFITZir24/r2fOSH6tvmVy2CeqjXo83x+pBo9Y2GX\n\tbXnA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"j5kADWJR\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1689600942; x=1692192942;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=R+bUJWwuJxKvzU5KIn0Teb+SKzV+uN7Ki4TaPjMfOhM=;\n\tb=UwnRRltIeCe+WX5lxOGLj2oDonBuFlURhdqc065PSsg601IzO1OXGEdRfiKNVuwqN6\n\tmiHRhwsk8EeJd5p8uBC1lMx06+pz151V9Mg4yq9La5rxth5WYOTLDJ+OHHXUrbfD7yBx\n\t5GUffHzW8eMORBN3BM2d6fZ2X1uDY3ed7JZHGZLq7xly16qdFK/EbQMlBMaMGlYtSHD3\n\t3KaIIyd6jWe6f3/3f+sKUguwJALLW8amYbg4RqQH1Zl8TnQnSD4GwRWyLdN8tN13E2kc\n\t8QKNhNaqR8wlxaebzJIPweccpOItTEta8lk1D4MsLAL9s7+dpCszGDa6rC8q/obmtfXI\n\t0xWQ==","X-Gm-Message-State":"ABy/qLZMVOOw2nHoP9YVWDaEUaIpouPPGyEL30Q98oIbustm9dr/XJHN\n\tO6TDxE4I8+R7UvEz1lPaxwvgTJnub3HKTa9hg1rmDF6TpSLJrD2y","X-Google-Smtp-Source":"APBJJlHbFMQodO7V8CYodHdnB2CgTRmpAAq3xsf9oxdhFU8IIUVRWk7EvEh0WLf56siGRQ2zUHQ687UiGwwmazZBeCg=","X-Received":"by 2002:a0c:be8b:0:b0:636:a9a3:9035 with SMTP id\n\tn11-20020a0cbe8b000000b00636a9a39035mr9734190qvi.42.1689600942004;\n\tMon, 17 Jul 2023 06:35:42 -0700 (PDT)","MIME-Version":"1.0","References":"<20230714141549.11085-1-jacopo.mondi@ideasonboard.com>\n\t<20230714141549.11085-6-jacopo.mondi@ideasonboard.com>\n\t<CAHW6GYLSZJ6PEwDTefFF2DdPFDT+bV8-YmoTVBUFwkPnf6yG0w@mail.gmail.com>\n\t<e7qazbv3vvttq5lmgklg6n5xxelc24dfe3wcrtudmvcpqcltjg@a7dptbtf32li>","In-Reply-To":"<e7qazbv3vvttq5lmgklg6n5xxelc24dfe3wcrtudmvcpqcltjg@a7dptbtf32li>","Date":"Mon, 17 Jul 2023 14:35:31 +0100","Message-ID":"<CAHW6GYK7Gaanpo29wyfpRh5SQNKRD-vooRj-6mU3ohVcECSb3A@mail.gmail.com>","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 5/9] libcamera: transform: Add\n\tfunctions to convert Orientation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"David Plowman via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"David Plowman <david.plowman@raspberrypi.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":27565,"web_url":"https://patchwork.libcamera.org/comment/27565/","msgid":"<k4ulozwrl6lblsrsojzdxpeioecutwxpqzdvsfh6a4hmdrlcid@53bhoew3vvp6>","date":"2023-07-17T13:39:30","subject":"Re: [libcamera-devel] [PATCH v2 5/9] libcamera: transform: Add\n\tfunctions to convert Orientation","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi David\n\nOn Mon, Jul 17, 2023 at 02:35:31PM +0100, David Plowman via libcamera-devel wrote:\n> Hi again\n>\n> On Mon, 17 Jul 2023 at 13:53, Jacopo Mondi\n> <jacopo.mondi@ideasonboard.com> wrote:\n> >\n> > Hi David\n> >\n> > On Mon, Jul 17, 2023 at 10:46:04AM +0100, David Plowman via libcamera-devel wrote:\n> > > Hi Jacopo\n> > >\n> > > Thanks for the patch.\n> > >\n> > > On Fri, 14 Jul 2023 at 15:16, Jacopo Mondi\n> > > <jacopo.mondi@ideasonboard.com> wrote:\n> > > >\n> > > > Add two helper functions to the transform.cpp file that allows to\n> > > > convert to and from an Orientation.\n> > > >\n> > > > As transform.h now needs to include camera.h remove \"#include\n> > > > <transform.h>\" from camera.h to avoid a cyclic inclusion loop.\n> > > >\n> > > > Adjust all files that need to include transform.h directly and didn't do\n> > > > so because it was implicitly included by camera.h.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n> > >\n> > > The functions here seem fine to me, I guess I just wanted to ask the\n> > > question about include files. Part of me feels that transforms are\n> > > kind of like orientations, so if transforms have their own header,\n> > > maybe orientations should too? Transforms seem, to me at least, like\n> >\n> > I considered this, but so far Orientation is a simple enum, I didn't\n> > consider it enough to break it out to a dedicated file..\n> >\n> > > really simple things, so dragging in camera.h felt a bit weird (even\n> > > though, in practice, an application would probably do so anyway).\n> > >\n> >\n> > Looking ahead to the C API introduction, Transform seems to be more\n> > opportunely placed in the C++ API which will be built on top of the\n> > C one.\n> >\n> > Now, as you correctly noticed, including transform.h will drag in\n> > camera.h. As the camera orientation lives in CameraConfiguration it\n> > really seemed un-likely for an application to use Transform without\n> > including camera.h\n> >\n> > Actually, I initially wanted to forward declare Orientation and avoid\n> > including <camera.h> in transform.h. Unfrtunately forward-declaring\n> > inner types in C++ is not possible. One possible option would be to\n> > move Orientation out of CameraConfiguration, possibily to its own\n> > file as you were suggesting. This would remove the need to include\n> > <camera.h>\n> >\n> >\n> > > But honestly, I'm not sure. It was just the lack of symmetry between\n> > > the two that left me feeling slightly uncomfortable.\n> > >\n> > > Anyway, subject to some saying they like it how it is:\n> >\n> > Yeah opinions welcome!\n>\n> Another thing to consider is what happens in the Python bindings. Are\n> we going to have both orientations and transforms? I think you could\n> get by with only transforms (and convert them implicitly when needed),\n> or we could expose them there too. One to think about!\n>\n\nWe're strving to express the image orientation using the EXIF values\nfor greater interoperability, I don't see a reason not to have them\nexposed in Python or any other language bindings.\n\n> David\n>\n> >\n> > >\n> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>\n> > >\n> > > Thanks!\n> > > David\n> > >\n> > > > ---\n> > > >  include/libcamera/camera.h                   |  3 +-\n> > > >  include/libcamera/transform.h                |  4 ++\n> > > >  src/libcamera/camera.cpp                     |  1 +\n> > > >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp |  1 +\n> > > >  src/libcamera/transform.cpp                  | 58 ++++++++++++++++++++\n> > > >  5 files changed, 66 insertions(+), 1 deletion(-)\n> > > >\n> > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > > > index 8132ef2506a4..55359d53f2ab 100644\n> > > > --- a/include/libcamera/camera.h\n> > > > +++ b/include/libcamera/camera.h\n> > > > @@ -22,7 +22,6 @@\n> > > >  #include <libcamera/controls.h>\n> > > >  #include <libcamera/request.h>\n> > > >  #include <libcamera/stream.h>\n> > > > -#include <libcamera/transform.h>\n> > > >\n> > > >  namespace libcamera {\n> > > >\n> > > > @@ -31,6 +30,8 @@ class FrameBufferAllocator;\n> > > >  class PipelineHandler;\n> > > >  class Request;\n> > > >\n> > > > +enum class Transform;\n> > > > +\n> > > >  class CameraConfiguration\n> > > >  {\n> > > >  public:\n> > > > diff --git a/include/libcamera/transform.h b/include/libcamera/transform.h\n> > > > index 2a6838c78369..573adf18715d 100644\n> > > > --- a/include/libcamera/transform.h\n> > > > +++ b/include/libcamera/transform.h\n> > > > @@ -9,6 +9,8 @@\n> > > >\n> > > >  #include <string>\n> > > >\n> > > > +#include <libcamera/camera.h>\n> > > > +\n> > > >  namespace libcamera {\n> > > >\n> > > >  enum class Transform : int {\n> > > > @@ -69,6 +71,8 @@ constexpr Transform operator~(Transform t)\n> > > >  }\n> > > >\n> > > >  Transform transformFromRotation(int angle, bool *success = nullptr);\n> > > > +Transform transformFromOrientation(const CameraConfiguration::Orientation &orientation);\n> > > > +CameraConfiguration::Orientation transformToOrientation(const Transform &transform);\n> > > >\n> > > >  const char *transformToString(Transform t);\n> > > >\n> > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > > index c8bb56d4af7f..af842e70dcb0 100644\n> > > > --- a/src/libcamera/camera.cpp\n> > > > +++ b/src/libcamera/camera.cpp\n> > > > @@ -18,6 +18,7 @@\n> > > >  #include <libcamera/framebuffer_allocator.h>\n> > > >  #include <libcamera/request.h>\n> > > >  #include <libcamera/stream.h>\n> > > > +#include <libcamera/transform.h>\n> > > >\n> > > >  #include \"libcamera/internal/camera.h\"\n> > > >  #include \"libcamera/internal/camera_controls.h\"\n> > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > > index 38f48a5d9269..02a6117c7955 100644\n> > > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > > @@ -21,6 +21,7 @@\n> > > >  #include <libcamera/property_ids.h>\n> > > >  #include <libcamera/request.h>\n> > > >  #include <libcamera/stream.h>\n> > > > +#include <libcamera/transform.h>\n> > > >\n> > > >  #include \"libcamera/internal/camera.h\"\n> > > >  #include \"libcamera/internal/device_enumerator.h\"\n> > > > diff --git a/src/libcamera/transform.cpp b/src/libcamera/transform.cpp\n> > > > index 4668303d0676..03d2b52e7f38 100644\n> > > > --- a/src/libcamera/transform.cpp\n> > > > +++ b/src/libcamera/transform.cpp\n> > > > @@ -299,6 +299,64 @@ Transform transformFromRotation(int angle, bool *success)\n> > > >         return Transform::Identity;\n> > > >  }\n> > > >\n> > > > +/**\n> > > > + * \\brief Return the transform representing \\a orientation\n> > > > + * \\param[in] orientation The orientation to convert\n> > > > + * \\return The transform corresponding to \\a orientation\n> > > > + */\n> > > > +Transform transformFromOrientation(const CameraConfiguration::Orientation &orientation)\n> > > > +{\n> > > > +       switch (orientation) {\n> > > > +       case CameraConfiguration::rotate0:\n> > > > +               return Transform::Identity;\n> > > > +       case CameraConfiguration::rotate0Flip:\n> > > > +               return Transform::HFlip;\n> > > > +       case CameraConfiguration::rotate180:\n> > > > +               return Transform::Rot180;\n> > > > +       case CameraConfiguration::rotate180Flip:\n> > > > +               return Transform::VFlip;\n> > > > +       case CameraConfiguration::rotate90Flip:\n> > > > +               return Transform::Transpose;\n> > > > +       case CameraConfiguration::rotate90:\n> > > > +               return Transform::Rot90;\n> > > > +       case CameraConfiguration::rotate270Flip:\n> > > > +               return Transform::Rot180Transpose;\n> > > > +       case CameraConfiguration::rotate270:\n> > > > +               return Transform::Rot270;\n> > > > +       }\n> > > > +\n> > > > +       return Transform::Identity;\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Return the CameraConfiguration::Orientation representing \\a transform\n> > > > + * \\param[in] transform The transform to convert\n> > > > + * \\return The Orientation corresponding to \\a transform\n> > > > + */\n> > > > +CameraConfiguration::Orientation transformToOrientation(const Transform &transform)\n> > > > +{\n> > > > +       switch (transform) {\n> > > > +       case Transform::Identity:\n> > > > +               return CameraConfiguration::rotate0;\n> > > > +       case Transform::HFlip:\n> > > > +               return CameraConfiguration::rotate0Flip;\n> > > > +       case Transform::VFlip:\n> > > > +               return CameraConfiguration::rotate180Flip;\n> > > > +       case Transform::Rot180:\n> > > > +               return CameraConfiguration::rotate180;\n> > > > +       case Transform::Transpose:\n> > > > +               return CameraConfiguration::rotate90Flip;\n> > > > +       case Transform::Rot270:\n> > > > +               return CameraConfiguration::rotate270;\n> > > > +       case Transform::Rot90:\n> > > > +               return CameraConfiguration::rotate90;\n> > > > +       case Transform::Rot180Transpose:\n> > > > +               return CameraConfiguration::rotate270Flip;\n> > > > +       }\n> > > > +\n> > > > +       return CameraConfiguration::rotate0;\n> > > > +}\n> > > > +\n> > > >  /**\n> > > >   * \\brief Return a character string describing the transform\n> > > >   * \\param[in] t The transform to be described.\n> > > > --\n> > > > 2.40.1\n> > > >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 DFADDBDC71\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 17 Jul 2023 13:39:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4C386628BD;\n\tMon, 17 Jul 2023 15:39:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7941661E2C\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 17 Jul 2023 15:39:35 +0200 (CEST)","from ideasonboard.com (mob-5-90-54-150.net.vodafone.it\n\t[5.90.54.150])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2BD29331B;\n\tMon, 17 Jul 2023 15:38:41 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1689601177;\n\tbh=NHOtfDnYgSVDEWswyNang2Wh+3O4Wf6Z3dz4t6i2Wxo=;\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=gP107Ubd3YCJBuYwpDhuNJC64NQmJoW+cX+5tkW2sLF0vtrvdG5jnStrTqH4s1jOJ\n\tAPYDl9WgcpnewLdXGo+WFHd+97wkU9kuJQmQiPSdlBuFEYfSKJ32L56fQ/AMw5b4QE\n\tZ5V0geNs39vGv6kVL9NtZjDp+M5oica/tIRwh9q7WNKzj2mUrGyCMYLHG+8VYL4si0\n\t6MQdNQ7jza3QPK3DtXHFsLcNrroWiMhU1K0gqPvQB1GO8it7p04KgWHKAHCTqn4zXr\n\tOUSAzZVrSd4a2LdLLPawGzpnSUAKubzLeXO7Mjdo4Vo19IURDLX7hag0yY8IMOAC9z\n\thasqRgpW4UvyQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1689601122;\n\tbh=NHOtfDnYgSVDEWswyNang2Wh+3O4Wf6Z3dz4t6i2Wxo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gTN62WWdki3NfAvMax2nnQ4hTbw6PYtY0HeHYjtvTCf6mSn59lViJROEYZFuryZ5q\n\tYnSB/2oBO3okxPa9bxN7dOMMowAoU/P0D83RAYlEe1irQWqYfEd6ednOSzmCp1sYQ9\n\tPFuW77LTu6HgAPY2StbfF//Unbx10Z4889k4u3xI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"gTN62WWd\"; dkim-atps=neutral","Date":"Mon, 17 Jul 2023 15:39:30 +0200","To":"David Plowman <david.plowman@raspberrypi.com>","Message-ID":"<k4ulozwrl6lblsrsojzdxpeioecutwxpqzdvsfh6a4hmdrlcid@53bhoew3vvp6>","References":"<20230714141549.11085-1-jacopo.mondi@ideasonboard.com>\n\t<20230714141549.11085-6-jacopo.mondi@ideasonboard.com>\n\t<CAHW6GYLSZJ6PEwDTefFF2DdPFDT+bV8-YmoTVBUFwkPnf6yG0w@mail.gmail.com>\n\t<e7qazbv3vvttq5lmgklg6n5xxelc24dfe3wcrtudmvcpqcltjg@a7dptbtf32li>\n\t<CAHW6GYK7Gaanpo29wyfpRh5SQNKRD-vooRj-6mU3ohVcECSb3A@mail.gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<CAHW6GYK7Gaanpo29wyfpRh5SQNKRD-vooRj-6mU3ohVcECSb3A@mail.gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v2 5/9] libcamera: transform: Add\n\tfunctions to convert Orientation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Jacopo Mondi via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Cc":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]