[{"id":27403,"web_url":"https://patchwork.libcamera.org/comment/27403/","msgid":"","date":"2023-06-21T09:08:25","subject":"Re: [libcamera-devel] [RFC 0/4] libcamera: Replace\n\tCameraConfiguration::transform","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 sending this proposal. I wasn't sure I really understood\nit, perhaps you can help me out!\n\nOn Tue, 20 Jun 2023 at 15:29, Jacopo Mondi\n wrote:\n>\n> Hello everyone\n>\n> We have been discussing rotation/transformations for a long time, and\n> the currently implemented CameraConfiguration::transform behaviour has\n> proven to be confusing for several reasons: it was poorly documented,\n> translating it to something consumable for upper layer frameworks was\n> difficult and it behaved differently than any other field part of the\n> CameraConfiguration, as it was meant to tell libcamera \"what to do\" instead\n> of expressing what applications want.\n\nAs far as I understand it, I don't believe this is correct, and the\ntransform is where you say what you want.\n\nFor example, on a Raspberry Pi I always set the transform to\nTransform::Identity because I want the images \"the right way up\". (I\nknow that internally, it will calculate and apply a 180 rotation to\ncounteract the sensor mounting rotation.) Does that sound right?\n\nIf the problem is actually to do with documentation, I would always\nagree that it can be improved, and am very happy to help!\n\n>\n> For these reasons this series proposes to replace the usage to Transform\n> in the public API in favour of a new CameraConfiguration::Orientation type,\n> defined based on the EXIF specification, tag 274, 'orientation'. For reference\n> this is the same as implemented by gstreamer:\n> https://gstreamer.freedesktop.org/documentation/gstreamer/gsttaglist.html?gi-language=c#GST_TAG_IMAGE_ORIENTATION\n\nI must confess to not liking the term \"orientation\". There is such a\nthing as a \"2D plane transformation\" and I'd expect to refer to them\nas transformations or maybe transforms for short. I don't understand\nwhat an \"orientation\" is without reading extra documentation. Are\n\"orientations\" composable? Is there such a thing as an \"inverse\norientation\"?\n\n>\n> The newly introduced CameraConfiguration::orientation replaces the\n> existing CameraConfiguration::tranform, and it is meant for application to\n> express how they would like the images to be oriented, not to tell libcamera\n> what to do. As an example, passing in 'rotation0' means that the application\n> expects the images to be rotated upright, and doesn't tell libcamera not to\n> apply any rotation like passing in \"Transform::Identity\" did.\n\nMy understanding is that passing \"Transform::Identity\" has always\nmeant that the application wants its images \"upright\". (Are there\ncases where this doesn't happen?)\n\n>\n> The value CameraConfiguration::orientation is set to after a validate() also\n> differs in meaning, as instead of reporting \"what applications have to do\n> to obtain what they originally asked for\" it simply reports the actual\n> orientation of the stream: this means that if libcamera cannot fully satisfy the\n> user request it will set ::orientation to report the native images rotation\n> and the CameraConfiguration::status will be set to Adjusted.\n\nI believe this to be the current behaviour of the transform field.\n\n>\n> Handling of 90 and 270 degrees rotation has also changed: as the camera sensor\n> cannot correct rotations that include a transposition, requests for a 90/270\n> degrees are ignored, and cameras with a mounting rotation of 90/270 degrees\n> are never re-oriented. This makes it clear and less confusing for applications\n> that they have to deal with correction fully by themselves. As an example, with\n> the current implementation if the application requires a Rot270 (HFlip +\n> Transpose) libcamera will do the HFlip and leave transposition to the upper\n> layers. There is no clear advantage in doing so, and display compositors are\n> better suited for handling transpositions and flipping in a single pass instead\n> of having the library try to handle part of that.\n\nAgain, I'm not really understanding this.\n\nThe current behaviour is such that, if it can't give you the\ntransformation that you ask for (on account of not being able to do\ntransposition), then it guarantees to give you what you requested\n_apart_ from the transposition. So the application only has one case\nto deal with - applying a transpose. It doesn't have to worry about\nthe other 3 \"transforms-that-I-can't-handle\", thereby making life\neasier for the application.\n\nBecause transforms are composable, it's easy to calculate what\ntransform to ask for to get a different behaviour (for example, do\nnothing at all if there's a transpose involved). I could imagine a\nslightly different API might be helpful, perhaps where you get to say\nnot only \"what you want\", but also what you want the \"residual\ntransform\" to be (being the one the application has to apply after) if\nit can't deliver what you wanted. Would that be useful?\n\nI also recall that there was some discussion a while back about the\nprecise orientation of the rotations which could affect some of the\ndetails, but I don't think it was ever concluded. I don't know if\nthat's causing any problems?\n\n>\n> This series clearly breaks the application API as it removes a member from\n> CameraConfiguration, so it should be introduced probably only when a new release\n> is cut.\n\nI can't comment on this as I haven't really understood the change\nhere. Obviously we have many end users for whom an API change would be\nunwelcome, but I suppose the change could be hidden from them.\n\nI apologise if there have been some discussions flying around that I\nhaven't followed sufficiently. I'm afraid I don't normally study\ngstreamer related posts in detail, perhaps I need to try harder...\n\nThanks\nDavid\n\n>\n> Tested on RPi imx219 with the newly introduced cam option, flip, mirror and\n> rotation work as expected.\n>\n> Tested on Pinephone Pro, where sensors are 90/270 degrees rotated (libcamera\n> performs no correction)\n>\n> Comments welcome of course.\n>\n> Thanks\n> j\n>\n> Jacopo Mondi (4):\n> libcamera: camera_sensor: Cache rotationTransform_\n> libcamera: camera: Introduce CameraConfiguration::orientation\n> libcamera: Move to use CameraConfiguration::orientation\n> apsp: cam: Add option to set stream orientation\n>\n> Documentation/Doxyfile.in | 2 +\n> Documentation/rotation/flip-rotate-0.eps | 170 +++++++++++++++\n> Documentation/rotation/flip-rotate-0.png | Bin 0 -> 16488 bytes\n> Documentation/rotation/flip-rotate-180.eps | 191 +++++++++++++++++\n> Documentation/rotation/flip-rotate-180.png | Bin 0 -> 22198 bytes\n> Documentation/rotation/flip-rotate-270.eps | 197 ++++++++++++++++++\n> Documentation/rotation/flip-rotate-270.png | Bin 0 -> 21367 bytes\n> Documentation/rotation/flip-rotate-90.eps | 195 +++++++++++++++++\n> Documentation/rotation/flip-rotate-90.png | Bin 0 -> 21020 bytes\n> Documentation/rotation/rotate-0.eps | 169 +++++++++++++++\n> Documentation/rotation/rotate-0.png | Bin 0 -> 9086 bytes\n> Documentation/rotation/rotate-180.eps | 189 +++++++++++++++++\n> Documentation/rotation/rotate-180.png | Bin 0 -> 22182 bytes\n> Documentation/rotation/rotate-270.eps | 196 +++++++++++++++++\n> Documentation/rotation/rotate-270.png | Bin 0 -> 21016 bytes\n> Documentation/rotation/rotate-90.eps | 196 +++++++++++++++++\n> Documentation/rotation/rotate-90.png | Bin 0 -> 21528 bytes\n> include/libcamera/camera.h | 14 +-\n> include/libcamera/internal/camera_sensor.h | 4 +-\n> include/libcamera/transform.h | 4 +\n> src/apps/cam/camera_session.cpp | 17 ++\n> src/apps/cam/main.cpp | 5 +\n> src/apps/cam/main.h | 1 +\n> src/libcamera/camera.cpp | 54 ++++-\n> src/libcamera/camera_sensor.cpp | 137 ++++++------\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +-\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +-\n> .../pipeline/rpi/common/pipeline_base.cpp | 9 +-\n> src/libcamera/pipeline/simple/simple.cpp | 6 +-\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +-\n> src/libcamera/pipeline/vimc/vimc.cpp | 4 +-\n> src/libcamera/transform.cpp | 58 ++++++\n> 32 files changed, 1739 insertions(+), 97 deletions(-)\n> create mode 100644 Documentation/rotation/flip-rotate-0.eps\n> create mode 100644 Documentation/rotation/flip-rotate-0.png\n> create mode 100644 Documentation/rotation/flip-rotate-180.eps\n> create mode 100644 Documentation/rotation/flip-rotate-180.png\n> create mode 100644 Documentation/rotation/flip-rotate-270.eps\n> create mode 100644 Documentation/rotation/flip-rotate-270.png\n> create mode 100644 Documentation/rotation/flip-rotate-90.eps\n> create mode 100644 Documentation/rotation/flip-rotate-90.png\n> create mode 100644 Documentation/rotation/rotate-0.eps\n> create mode 100644 Documentation/rotation/rotate-0.png\n> create mode 100644 Documentation/rotation/rotate-180.eps\n> create mode 100644 Documentation/rotation/rotate-180.png\n> create mode 100644 Documentation/rotation/rotate-270.eps\n> create mode 100644 Documentation/rotation/rotate-270.png\n> create mode 100644 Documentation/rotation/rotate-90.eps\n> create mode 100644 Documentation/rotation/rotate-90.png\n>\n> --\n> 2.40.1\n>","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 58048C3237\n\tfor ;\n\tWed, 21 Jun 2023 09:08:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 62F74628BD;\n\tWed, 21 Jun 2023 11:08:38 +0200 (CEST)","from mail-qv1-xf34.google.com (mail-qv1-xf34.google.com\n\t[IPv6:2607:f8b0:4864:20::f34])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ED26C61E3C\n\tfor ;\n\tWed, 21 Jun 2023 11:08:36 +0200 (CEST)","by mail-qv1-xf34.google.com with SMTP id\n\t6a1803df08f44-5ed99ebe076so53244696d6.2\n\tfor ;\n\tWed, 21 Jun 2023 02:08:36 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1687338518;\n\tbh=MPc8p4EK/3xE9Y0d89YZEVsnmj0FBOv+25ZWRhCLgbQ=;\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=DQm0WsoEGr8Bj/0cDtKyjk2dGAjVT1fr5RoybfJdqmkAbnku1zMesB6oFYdKZkm4E\n\tZ4VoFPzuEIlCc8Bn7EcwHRC472Xywj1ueFWKClC50TfWND4gBr/DR6yUIby1/9t3Rm\n\t1kTE5XrhLtv+5V1zWHhHD4VxJ75Qp1/JFVnOv7vQmw0X9SSL4gVz7KiTGBInLGX63n\n\teIAiD8orG/Z1mIW5FQMCzCcCOVbFxVEfs1+igzInB+NXjLnhya65jK2MJgQ60UxIHG\n\tmsr+/24aLHtHEiBRtBLQf2A3vIO6g7vWmWXMoZuCPG1+X3qcmS1QBg8AhWXee9dCSK\n\teIcvLgPgdqAnQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1687338516; x=1689930516;\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=SGVaLxsLpZEPemfBlV0P46ruHXqo6o+f2ZcV189r4z4=;\n\tb=MmQ0nnPBHY00RvGHN1TskLAeZssni27Tq56o1HOm6N4TDX0QPFCM/IhSrRR7AzEu0/\n\t2C8sZgb/paTrQTGbpHbWOXFq+D1W0omceErlDt/y1irvL5TqTZ0kZzWB5zpIc8j+2BdJ\n\tvpXrnjrHlcSExYBnct3tSgwBvZcPJ2LXCn4biogPWaqsqa1efnL8xHh5PZNzFqi8I+Az\n\tkDclf9BI/reWbu2NscSkbsCl+gWFmPbdtKA2c03JLbvW+0OkCacIMxdFT68GoiISksDS\n\tjMUbIBZ3Ghnhy1cBGF9/Keso/arxVZBzY/7XAVNbVOqNoxz23ZA9pN9oy++ULCmwR/xy\n\tEbpQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"MmQ0nnPB\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1687338516; x=1689930516;\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=SGVaLxsLpZEPemfBlV0P46ruHXqo6o+f2ZcV189r4z4=;\n\tb=HcHL8nLsFx1m+e/ekbyze97dATpy/d3SPPjZUjnxCgfS84HKPqSclFGU1JR0G3BrAf\n\tXHOvtMV315GRqsR2WSrXEQIqO1ohgMaBnX9PLpV9Ho79+TVNwuyieh7e/dIMla68C/X/\n\tapMakOMyt4/WB5XO7ko7Xr4KSKPS2ifuTpz9TdlygcoUfm80oOIWsMLwF3IG930YJhUF\n\tzY17om2tWAW7jDanQZdLDxE+IN7A+mHI/9jdMDIDHPe/z+nnW4Wc0otcA3paEclnCdXT\n\tBZwrC79XXf19DXFMPHjwUkBb4sHFvWO+xoB7IEoIjYh3fi0C/a6vBF0+zkSo69KGRAMo\n\t1DNQ==","X-Gm-Message-State":"AC+VfDyCMz7uMVU5+otunR7voNE15GXjw3TPCZSN8fLF9qb1dLVhAek3\n\tsag7qjB5wLVc72NAbVYVJrNjCtkXDbJYKsJTv6Zfuw==","X-Google-Smtp-Source":"ACHHUZ7OXBSfwmQNYQ6YaDreKcPjTwW9Iudlza70zie5V6YRbPOsAuDZ1fwwFK6hCBsvnr/Ynbj0xWNs58/OYO0lrvo=","X-Received":"by 2002:a05:6214:2b08:b0:623:65d4:f646 with SMTP id\n\tjx8-20020a0562142b0800b0062365d4f646mr8545830qvb.42.1687338515802;\n\tWed, 21 Jun 2023 02:08:35 -0700 (PDT)","MIME-Version":"1.0","References":"<20230620142904.72600-1-jacopo.mondi@ideasonboard.com>","In-Reply-To":"<20230620142904.72600-1-jacopo.mondi@ideasonboard.com>","Date":"Wed, 21 Jun 2023 10:08:25 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC 0/4] libcamera: Replace\n\tCameraConfiguration::transform","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27404,"web_url":"https://patchwork.libcamera.org/comment/27404/","msgid":"","date":"2023-06-21T09:23:49","subject":"Re: [libcamera-devel] [RFC 0/4] libcamera: Replace\n\tCameraConfiguration::transform","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi David\n\nOn Wed, Jun 21, 2023 at 10:08:25AM +0100, David Plowman wrote:\n> Hi Jacopo\n>\n> Thanks for sending this proposal. I wasn't sure I really understood\n> it, perhaps you can help me out!\n>\n> On Tue, 20 Jun 2023 at 15:29, Jacopo Mondi\n> wrote:\n> >\n> > Hello everyone\n> >\n> > We have been discussing rotation/transformations for a long time, and\n> > the currently implemented CameraConfiguration::transform behaviour has\n> > proven to be confusing for several reasons: it was poorly documented,\n> > translating it to something consumable for upper layer frameworks was\n> > difficult and it behaved differently than any other field part of the\n> > CameraConfiguration, as it was meant to tell libcamera \"what to do\" instead\n> > of expressing what applications want.\n>\n> As far as I understand it, I don't believe this is correct, and the\n> transform is where you say what you want.\n>\n> For example, on a Raspberry Pi I always set the transform to\n> Transform::Identity because I want the images \"the right way up\". (I\n> know that internally, it will calculate and apply a 180 rotation to\n> counteract the sensor mounting rotation.) Does that sound right?\n\nYes, now that we correct properties::Rotation to 0 as the library\ncompensate the mounting rotation behind your back, that's what you\nget.\n\nBefore we had properties::Rotation = 180; transform = Identity and you\ngot an upright image. Confusing, isn't it ?\n\n>\n> If the problem is actually to do with documentation, I would always\n> agree that it can be improved, and am very happy to help!\n>\n> >\n> > For these reasons this series proposes to replace the usage to Transform\n> > in the public API in favour of a new CameraConfiguration::Orientation type,\n> > defined based on the EXIF specification, tag 274, 'orientation'. For reference\n> > this is the same as implemented by gstreamer:\n> > https://gstreamer.freedesktop.org/documentation/gstreamer/gsttaglist.html?gi-language=c#GST_TAG_IMAGE_ORIENTATION\n>\n> I must confess to not liking the term \"orientation\". There is such a\n> thing as a \"2D plane transformation\" and I'd expect to refer to them\n> as transformations or maybe transforms for short. I don't understand\n> what an \"orientation\" is without reading extra documentation. Are\n> \"orientations\" composable? Is there such a thing as an \"inverse\n> orientation\"?\n>\n\nI'm fine bikeshedding on the name. What matters to me is that now we\nconform to an existing specification adopeted by most frameworks\ninstead of defining our own Transform.\n\n> >\n> > The newly introduced CameraConfiguration::orientation replaces the\n> > existing CameraConfiguration::tranform, and it is meant for application to\n> > express how they would like the images to be oriented, not to tell libcamera\n> > what to do. As an example, passing in 'rotation0' means that the application\n> > expects the images to be rotated upright, and doesn't tell libcamera not to\n> > apply any rotation like passing in \"Transform::Identity\" did.\n>\n> My understanding is that passing \"Transform::Identity\" has always\n> meant that the application wants its images \"upright\". (Are there\n> cases where this doesn't happen?)\n>\n\nAs we have auto-correction in place, yes.\n\n> >\n> > The value CameraConfiguration::orientation is set to after a validate() also\n> > differs in meaning, as instead of reporting \"what applications have to do\n> > to obtain what they originally asked for\" it simply reports the actual\n> > orientation of the stream: this means that if libcamera cannot fully satisfy the\n> > user request it will set ::orientation to report the native images rotation\n> > and the CameraConfiguration::status will be set to Adjusted.\n>\n> I believe this to be the current behaviour of the transform field.\n>\n\nNot really, see below\n\n> >\n> > Handling of 90 and 270 degrees rotation has also changed: as the camera sensor\n> > cannot correct rotations that include a transposition, requests for a 90/270\n> > degrees are ignored, and cameras with a mounting rotation of 90/270 degrees\n> > are never re-oriented. This makes it clear and less confusing for applications\n> > that they have to deal with correction fully by themselves. As an example, with\n> > the current implementation if the application requires a Rot270 (HFlip +\n> > Transpose) libcamera will do the HFlip and leave transposition to the upper\n> > layers. There is no clear advantage in doing so, and display compositors are\n> > better suited for handling transpositions and flipping in a single pass instead\n> > of having the library try to handle part of that.\n>\n> Again, I'm not really understanding this.\n>\n> The current behaviour is such that, if it can't give you the\n> transformation that you ask for (on account of not being able to do\n> transposition), then it guarantees to give you what you requested\n> _apart_ from the transposition. So the application only has one case\n> to deal with - applying a transpose. It doesn't have to worry about\n> the other 3 \"transforms-that-I-can't-handle\", thereby making life\n> easier for the application.\n\nThe _apart_ is the key here.\n\nIf you currently ask for Rot270 the library will do an HFlip and set\ntransform to Transpose (ie what applications still have to do to\nobtain Rot270). This is not how the other fields of\nCameraConfiguration work, and mostly, applying a flip and letting apps\nonly deal with transpose is an \"optimization\" that might actually make\nlife harder for apps, as display compositors usually have means to\ndeal with known rotations, while in this case you only ask them to do\na Transpose, which might require them more work ?\n\nIn any case, I think it's saner if the library cannot do what it has\nbeen asked for to set ::transform to what the stream orientation\nactually is, instead of telling apps what \"they still have to do\".\n\n>\n> Because transforms are composable, it's easy to calculate what\n> transform to ask for to get a different behaviour (for example, do\n> nothing at all if there's a transpose involved). I could imagine a\n> slightly different API might be helpful, perhaps where you get to say\n> not only \"what you want\", but also what you want the \"residual\n> transform\" to be (being the one the application has to apply after) if\n> it can't deliver what you wanted. Would that be useful?\n>\n\nI think it's better to leave the stream orientation unmodified if it\ncannot be corrected. This matches the behaviour of the other\nCameraConfiguration fields, and seems easier to deal with from\napplications.\n\n> I also recall that there was some discussion a while back about the\n> precise orientation of the rotations which could affect some of the\n> details, but I don't think it was ever concluded. I don't know if\n> that's causing any problems?\n\nSorry, I don't remember such discussion. Any link ?\n\n>\n> >\n> > This series clearly breaks the application API as it removes a member from\n> > CameraConfiguration, so it should be introduced probably only when a new release\n> > is cut.\n>\n> I can't comment on this as I haven't really understood the change\n> here. Obviously we have many end users for whom an API change would be\n> unwelcome, but I suppose the change could be hidden from them.\n>\n> I apologise if there have been some discussions flying around that I\n> haven't followed sufficiently. I'm afraid I don't normally study\n> gstreamer related posts in detail, perhaps I need to try harder...\n>\n> Thanks\n> David\n>\n> >\n> > Tested on RPi imx219 with the newly introduced cam option, flip, mirror and\n> > rotation work as expected.\n> >\n> > Tested on Pinephone Pro, where sensors are 90/270 degrees rotated (libcamera\n> > performs no correction)\n> >\n> > Comments welcome of course.\n> >\n> > Thanks\n> > j\n> >\n> > Jacopo Mondi (4):\n> > libcamera: camera_sensor: Cache rotationTransform_\n> > libcamera: camera: Introduce CameraConfiguration::orientation\n> > libcamera: Move to use CameraConfiguration::orientation\n> > apsp: cam: Add option to set stream orientation\n> >\n> > Documentation/Doxyfile.in | 2 +\n> > Documentation/rotation/flip-rotate-0.eps | 170 +++++++++++++++\n> > Documentation/rotation/flip-rotate-0.png | Bin 0 -> 16488 bytes\n> > Documentation/rotation/flip-rotate-180.eps | 191 +++++++++++++++++\n> > Documentation/rotation/flip-rotate-180.png | Bin 0 -> 22198 bytes\n> > Documentation/rotation/flip-rotate-270.eps | 197 ++++++++++++++++++\n> > Documentation/rotation/flip-rotate-270.png | Bin 0 -> 21367 bytes\n> > Documentation/rotation/flip-rotate-90.eps | 195 +++++++++++++++++\n> > Documentation/rotation/flip-rotate-90.png | Bin 0 -> 21020 bytes\n> > Documentation/rotation/rotate-0.eps | 169 +++++++++++++++\n> > Documentation/rotation/rotate-0.png | Bin 0 -> 9086 bytes\n> > Documentation/rotation/rotate-180.eps | 189 +++++++++++++++++\n> > Documentation/rotation/rotate-180.png | Bin 0 -> 22182 bytes\n> > Documentation/rotation/rotate-270.eps | 196 +++++++++++++++++\n> > Documentation/rotation/rotate-270.png | Bin 0 -> 21016 bytes\n> > Documentation/rotation/rotate-90.eps | 196 +++++++++++++++++\n> > Documentation/rotation/rotate-90.png | Bin 0 -> 21528 bytes\n> > include/libcamera/camera.h | 14 +-\n> > include/libcamera/internal/camera_sensor.h | 4 +-\n> > include/libcamera/transform.h | 4 +\n> > src/apps/cam/camera_session.cpp | 17 ++\n> > src/apps/cam/main.cpp | 5 +\n> > src/apps/cam/main.h | 1 +\n> > src/libcamera/camera.cpp | 54 ++++-\n> > src/libcamera/camera_sensor.cpp | 137 ++++++------\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +-\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +-\n> > .../pipeline/rpi/common/pipeline_base.cpp | 9 +-\n> > src/libcamera/pipeline/simple/simple.cpp | 6 +-\n> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +-\n> > src/libcamera/pipeline/vimc/vimc.cpp | 4 +-\n> > src/libcamera/transform.cpp | 58 ++++++\n> > 32 files changed, 1739 insertions(+), 97 deletions(-)\n> > create mode 100644 Documentation/rotation/flip-rotate-0.eps\n> > create mode 100644 Documentation/rotation/flip-rotate-0.png\n> > create mode 100644 Documentation/rotation/flip-rotate-180.eps\n> > create mode 100644 Documentation/rotation/flip-rotate-180.png\n> > create mode 100644 Documentation/rotation/flip-rotate-270.eps\n> > create mode 100644 Documentation/rotation/flip-rotate-270.png\n> > create mode 100644 Documentation/rotation/flip-rotate-90.eps\n> > create mode 100644 Documentation/rotation/flip-rotate-90.png\n> > create mode 100644 Documentation/rotation/rotate-0.eps\n> > create mode 100644 Documentation/rotation/rotate-0.png\n> > create mode 100644 Documentation/rotation/rotate-180.eps\n> > create mode 100644 Documentation/rotation/rotate-180.png\n> > create mode 100644 Documentation/rotation/rotate-270.eps\n> > create mode 100644 Documentation/rotation/rotate-270.png\n> > create mode 100644 Documentation/rotation/rotate-90.eps\n> > create mode 100644 Documentation/rotation/rotate-90.png\n> >\n> > --\n> > 2.40.1\n> >","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 C4CD6BD87C\n\tfor ;\n\tWed, 21 Jun 2023 09:23:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2A8F1628BD;\n\tWed, 21 Jun 2023 11:23:54 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0B71561E3C\n\tfor ;\n\tWed, 21 Jun 2023 11:23:53 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:5d2e:52c9:1cf0:b3bc:c785:4625])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 83604BC;\n\tWed, 21 Jun 2023 11:23:17 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1687339434;\n\tbh=mDUrdz8pRoVVP4ql/N9TiNppRFIR2DIfOwpLSEwo9sI=;\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=BNefEltO60Z/JoEYXJbd5rgxHoZpCbOR52XPSViNlighnKkTpjPnpzEJHkAAy1/q9\n\tQYivNBLTJ2yM2HuZ4UMok7GiI72q4/6PL5oopXVuRmyXXIcC/g+lX3t3zgS6X8Oom1\n\the7GdtK9xDrTQC56O3aqDRiNGN0uXy+vHcfr2aTotLK6D/eXtih1thiJUIyop/000s\n\tdhsK4jVUB+z1vwdKMG8RMEY7RAuxx9hCU5rC4+oPnT1pOYzGT1XQkRbR+6BqYY5JMF\n\tPM13bsn2uDy12qCobWToBFFH0vCEjJ5dU9/cwC9ghKi+XI2y4h1bg/O9bngHK0WNif\n\tHILmPOAfxpWQw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1687339397;\n\tbh=mDUrdz8pRoVVP4ql/N9TiNppRFIR2DIfOwpLSEwo9sI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jqtRxwY3KnnOYHNiIAp30/U2nh5jNqQsR1kekUM/RdR1FPOMRV9lmV/iwW7eKzPNW\n\tqNxmH2KJc+KSz28C6HarXIuSk5NoD5JBPaLzhfgRgMzv51VXbyKasH/CKAOPyDGmGw\n\tY7a/AD4IhzDt6ouJPPBaRqGBc5G3g+q/YIrqXJB4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"jqtRxwY3\"; dkim-atps=neutral","Date":"Wed, 21 Jun 2023 11:23:49 +0200","To":"David Plowman ","Message-ID":"","References":"<20230620142904.72600-1-jacopo.mondi@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] [RFC 0/4] libcamera: Replace\n\tCameraConfiguration::transform","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":27406,"web_url":"https://patchwork.libcamera.org/comment/27406/","msgid":"","date":"2023-06-21T12:37:11","subject":"Re: [libcamera-devel] [RFC 0/4] libcamera: Replace\n\tCameraConfiguration::transform","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 reply.\n\nOn Wed, 21 Jun 2023 at 10:23, Jacopo Mondi\n wrote:\n>\n> Hi David\n>\n> On Wed, Jun 21, 2023 at 10:08:25AM +0100, David Plowman wrote:\n> > Hi Jacopo\n> >\n> > Thanks for sending this proposal. I wasn't sure I really understood\n> > it, perhaps you can help me out!\n> >\n> > On Tue, 20 Jun 2023 at 15:29, Jacopo Mondi\n> > wrote:\n> > >\n> > > Hello everyone\n> > >\n> > > We have been discussing rotation/transformations for a long time, and\n> > > the currently implemented CameraConfiguration::transform behaviour has\n> > > proven to be confusing for several reasons: it was poorly documented,\n> > > translating it to something consumable for upper layer frameworks was\n> > > difficult and it behaved differently than any other field part of the\n> > > CameraConfiguration, as it was meant to tell libcamera \"what to do\" instead\n> > > of expressing what applications want.\n> >\n> > As far as I understand it, I don't believe this is correct, and the\n> > transform is where you say what you want.\n> >\n> > For example, on a Raspberry Pi I always set the transform to\n> > Transform::Identity because I want the images \"the right way up\". (I\n> > know that internally, it will calculate and apply a 180 rotation to\n> > counteract the sensor mounting rotation.) Does that sound right?\n>\n> Yes, now that we correct properties::Rotation to 0 as the library\n> compensate the mounting rotation behind your back, that's what you\n> get.\n>\n> Before we had properties::Rotation = 180; transform = Identity and you\n> got an upright image. Confusing, isn't it ?\n\nI understood the previous arrangement, but the new one seems\ncounter-intuitive to me.\n\nOn a Raspberry Pi, I expect the properties::Rotation to report 180\ndegrees because the sensor is mounted upside down. Then I can ask for\nTransform::Identity and internally a 180 degree rotation will be\napplied to give me what I asked for. I understand this.\n\nAre we saying that now the rotation will report 0 degrees? Will it now\nalways report 0 degrees, whatever the sensor and platform? That would\nobviously break things...\n\n>\n> >\n> > If the problem is actually to do with documentation, I would always\n> > agree that it can be improved, and am very happy to help!\n> >\n> > >\n> > > For these reasons this series proposes to replace the usage to Transform\n> > > in the public API in favour of a new CameraConfiguration::Orientation type,\n> > > defined based on the EXIF specification, tag 274, 'orientation'. For reference\n> > > this is the same as implemented by gstreamer:\n> > > https://gstreamer.freedesktop.org/documentation/gstreamer/gsttaglist.html?gi-language=c#GST_TAG_IMAGE_ORIENTATION\n> >\n> > I must confess to not liking the term \"orientation\". There is such a\n> > thing as a \"2D plane transformation\" and I'd expect to refer to them\n> > as transformations or maybe transforms for short. I don't understand\n> > what an \"orientation\" is without reading extra documentation. Are\n> > \"orientations\" composable? Is there such a thing as an \"inverse\n> > orientation\"?\n> >\n>\n> I'm fine bikeshedding on the name. What matters to me is that now we\n> conform to an existing specification adopeted by most frameworks\n> instead of defining our own Transform.\n\nI'd like to find something that works for all libcamera users.\n\nThe Transform class allows composition, inverse operations and so on,\nthe idea being that users can calculate for themselves what to ask for\nif the default behaviour isn't what they want. I did comment that\nmaking it easier for folks to get a specific behaviour might be\nsomething to look into.\n\n>\n> > >\n> > > The newly introduced CameraConfiguration::orientation replaces the\n> > > existing CameraConfiguration::tranform, and it is meant for application to\n> > > express how they would like the images to be oriented, not to tell libcamera\n> > > what to do. As an example, passing in 'rotation0' means that the application\n> > > expects the images to be rotated upright, and doesn't tell libcamera not to\n> > > apply any rotation like passing in \"Transform::Identity\" did.\n> >\n> > My understanding is that passing \"Transform::Identity\" has always\n> > meant that the application wants its images \"upright\". (Are there\n> > cases where this doesn't happen?)\n> >\n>\n> As we have auto-correction in place, yes.\n\nMy understanding is that everything got \"auto-corrected\" before. I'm\nstill struggling to understand how it seems to be broken now.\n\n>\n> > >\n> > > The value CameraConfiguration::orientation is set to after a validate() also\n> > > differs in meaning, as instead of reporting \"what applications have to do\n> > > to obtain what they originally asked for\" it simply reports the actual\n> > > orientation of the stream: this means that if libcamera cannot fully satisfy the\n> > > user request it will set ::orientation to report the native images rotation\n> > > and the CameraConfiguration::status will be set to Adjusted.\n> >\n> > I believe this to be the current behaviour of the transform field.\n> >\n>\n> Not really, see below\n>\n> > >\n> > > Handling of 90 and 270 degrees rotation has also changed: as the camera sensor\n> > > cannot correct rotations that include a transposition, requests for a 90/270\n> > > degrees are ignored, and cameras with a mounting rotation of 90/270 degrees\n> > > are never re-oriented. This makes it clear and less confusing for applications\n> > > that they have to deal with correction fully by themselves. As an example, with\n> > > the current implementation if the application requires a Rot270 (HFlip +\n> > > Transpose) libcamera will do the HFlip and leave transposition to the upper\n> > > layers. There is no clear advantage in doing so, and display compositors are\n> > > better suited for handling transpositions and flipping in a single pass instead\n> > > of having the library try to handle part of that.\n> >\n> > Again, I'm not really understanding this.\n> >\n> > The current behaviour is such that, if it can't give you the\n> > transformation that you ask for (on account of not being able to do\n> > transposition), then it guarantees to give you what you requested\n> > _apart_ from the transposition. So the application only has one case\n> > to deal with - applying a transpose. It doesn't have to worry about\n> > the other 3 \"transforms-that-I-can't-handle\", thereby making life\n> > easier for the application.\n>\n> The _apart_ is the key here.\n>\n> If you currently ask for Rot270 the library will do an HFlip and set\n> transform to Transpose (ie what applications still have to do to\n> obtain Rot270). This is not how the other fields of\n\nWhat it _should_ do is set the transform field to the actual transform\nthat the user will perceive the images to have. I believe this to be\n\"proper\" libcamera configuration behaviour. I'm not understanding why\nthings are different now.\n\nBeyond this, the current behaviour is to leave the application to do\nthe transpose, but that's just a choice. The point of transforms is\nthat it's easy to use them to produce something different if you want.\nI don't mind looking at that if we can make it easier for everyone to\nget their preferred behaviour, which I suggested in my original reply.\n\n> CameraConfiguration work, and mostly, applying a flip and letting apps\n> only deal with transpose is an \"optimization\" that might actually make\n> life harder for apps, as display compositors usually have means to\n> deal with known rotations, while in this case you only ask them to do\n> a Transpose, which might require them more work ?\n>\n> In any case, I think it's saner if the library cannot do what it has\n> been asked for to set ::transform to what the stream orientation\n> actually is, instead of telling apps what \"they still have to do\".\n\nI don't understand the repeated statement that the transform is\ntelling apps \"what they still have to do\". That was never what it\nmeant, unless it got changed somewhere.\n\n>\n> >\n> > Because transforms are composable, it's easy to calculate what\n> > transform to ask for to get a different behaviour (for example, do\n> > nothing at all if there's a transpose involved). I could imagine a\n> > slightly different API might be helpful, perhaps where you get to say\n> > not only \"what you want\", but also what you want the \"residual\n> > transform\" to be (being the one the application has to apply after) if\n> > it can't deliver what you wanted. Would that be useful?\n> >\n>\n> I think it's better to leave the stream orientation unmodified if it\n> cannot be corrected. This matches the behaviour of the other\n> CameraConfiguration fields, and seems easier to deal with from\n> applications.\n\nI feel we ought to be able to come up with something that makes it\nconvenient for everyone to get what they want.\n\n>\n> > I also recall that there was some discussion a while back about the\n> > precise orientation of the rotations which could affect some of the\n> > details, but I don't think it was ever concluded. I don't know if\n> > that's causing any problems?\n>\n> Sorry, I don't remember such discussion. Any link ?\n\nI think the last email I sent on the subject was this one, after which\nthings went quiet:\nhttps://lists.libcamera.org/pipermail/libcamera-devel/2023-January/036445.html\nActually it seems to echo quite a bit of what I say above.\n\nThanks\nDavid\n\n>\n> >\n> > >\n> > > This series clearly breaks the application API as it removes a member from\n> > > CameraConfiguration, so it should be introduced probably only when a new release\n> > > is cut.\n> >\n> > I can't comment on this as I haven't really understood the change\n> > here. Obviously we have many end users for whom an API change would be\n> > unwelcome, but I suppose the change could be hidden from them.\n> >\n> > I apologise if there have been some discussions flying around that I\n> > haven't followed sufficiently. I'm afraid I don't normally study\n> > gstreamer related posts in detail, perhaps I need to try harder...\n> >\n> > Thanks\n> > David\n> >\n> > >\n> > > Tested on RPi imx219 with the newly introduced cam option, flip, mirror and\n> > > rotation work as expected.\n> > >\n> > > Tested on Pinephone Pro, where sensors are 90/270 degrees rotated (libcamera\n> > > performs no correction)\n> > >\n> > > Comments welcome of course.\n> > >\n> > > Thanks\n> > > j\n> > >\n> > > Jacopo Mondi (4):\n> > > libcamera: camera_sensor: Cache rotationTransform_\n> > > libcamera: camera: Introduce CameraConfiguration::orientation\n> > > libcamera: Move to use CameraConfiguration::orientation\n> > > apsp: cam: Add option to set stream orientation\n> > >\n> > > Documentation/Doxyfile.in | 2 +\n> > > Documentation/rotation/flip-rotate-0.eps | 170 +++++++++++++++\n> > > Documentation/rotation/flip-rotate-0.png | Bin 0 -> 16488 bytes\n> > > Documentation/rotation/flip-rotate-180.eps | 191 +++++++++++++++++\n> > > Documentation/rotation/flip-rotate-180.png | Bin 0 -> 22198 bytes\n> > > Documentation/rotation/flip-rotate-270.eps | 197 ++++++++++++++++++\n> > > Documentation/rotation/flip-rotate-270.png | Bin 0 -> 21367 bytes\n> > > Documentation/rotation/flip-rotate-90.eps | 195 +++++++++++++++++\n> > > Documentation/rotation/flip-rotate-90.png | Bin 0 -> 21020 bytes\n> > > Documentation/rotation/rotate-0.eps | 169 +++++++++++++++\n> > > Documentation/rotation/rotate-0.png | Bin 0 -> 9086 bytes\n> > > Documentation/rotation/rotate-180.eps | 189 +++++++++++++++++\n> > > Documentation/rotation/rotate-180.png | Bin 0 -> 22182 bytes\n> > > Documentation/rotation/rotate-270.eps | 196 +++++++++++++++++\n> > > Documentation/rotation/rotate-270.png | Bin 0 -> 21016 bytes\n> > > Documentation/rotation/rotate-90.eps | 196 +++++++++++++++++\n> > > Documentation/rotation/rotate-90.png | Bin 0 -> 21528 bytes\n> > > include/libcamera/camera.h | 14 +-\n> > > include/libcamera/internal/camera_sensor.h | 4 +-\n> > > include/libcamera/transform.h | 4 +\n> > > src/apps/cam/camera_session.cpp | 17 ++\n> > > src/apps/cam/main.cpp | 5 +\n> > > src/apps/cam/main.h | 1 +\n> > > src/libcamera/camera.cpp | 54 ++++-\n> > > src/libcamera/camera_sensor.cpp | 137 ++++++------\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +-\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +-\n> > > .../pipeline/rpi/common/pipeline_base.cpp | 9 +-\n> > > src/libcamera/pipeline/simple/simple.cpp | 6 +-\n> > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +-\n> > > src/libcamera/pipeline/vimc/vimc.cpp | 4 +-\n> > > src/libcamera/transform.cpp | 58 ++++++\n> > > 32 files changed, 1739 insertions(+), 97 deletions(-)\n> > > create mode 100644 Documentation/rotation/flip-rotate-0.eps\n> > > create mode 100644 Documentation/rotation/flip-rotate-0.png\n> > > create mode 100644 Documentation/rotation/flip-rotate-180.eps\n> > > create mode 100644 Documentation/rotation/flip-rotate-180.png\n> > > create mode 100644 Documentation/rotation/flip-rotate-270.eps\n> > > create mode 100644 Documentation/rotation/flip-rotate-270.png\n> > > create mode 100644 Documentation/rotation/flip-rotate-90.eps\n> > > create mode 100644 Documentation/rotation/flip-rotate-90.png\n> > > create mode 100644 Documentation/rotation/rotate-0.eps\n> > > create mode 100644 Documentation/rotation/rotate-0.png\n> > > create mode 100644 Documentation/rotation/rotate-180.eps\n> > > create mode 100644 Documentation/rotation/rotate-180.png\n> > > create mode 100644 Documentation/rotation/rotate-270.eps\n> > > create mode 100644 Documentation/rotation/rotate-270.png\n> > > create mode 100644 Documentation/rotation/rotate-90.eps\n> > > create mode 100644 Documentation/rotation/rotate-90.png\n> > >\n> > > --\n> > > 2.40.1\n> > >","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 7D6DDBD87C\n\tfor ;\n\tWed, 21 Jun 2023 12:37:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 243B9628C4;\n\tWed, 21 Jun 2023 14:37:26 +0200 (CEST)","from mail-qv1-xf34.google.com (mail-qv1-xf34.google.com\n\t[IPv6:2607:f8b0:4864:20::f34])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8CD8C628C2\n\tfor ;\n\tWed, 21 Jun 2023 14:37:23 +0200 (CEST)","by mail-qv1-xf34.google.com with SMTP id\n\t6a1803df08f44-62fe6580f17so46537686d6.0\n\tfor ;\n\tWed, 21 Jun 2023 05:37:23 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1687351046;\n\tbh=vHxJVTF0wp9o68e1oUzy8fniysieZ2IPeA8EyrMG188=;\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=DMNfnE0djvgBUnPj2D0H9MRy7BE5JDMHjVkrSV6vVB7PQd3nWOlGGHc6rFof9tIKF\n\tmGWLkGwZBYYVvG9NSh6rd4JV3P0UB442oGRDC0Iw1RxYoEU59ngTIN7pjH5RIvuUH/\n\t9jobMdHAjx1cmIWvKh8X+rwyieD1nPfMx1J9ynXJfDTXyfoWS22xzVCZb76V5+qIw8\n\tCXYrMK/QqrDRMnU1P3vstI0g8ofuGHB26BZtv553lYcyBnDnyCNP06scP5xLQpZu0O\n\t+osbVIjCs+7AaaLOesI0OrmhurvimaZ6g18SptupczVN8r5SwX7eblHCUAPRftQ3D2\n\t1XSRlQKSiXh5Q==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1687351042; x=1689943042;\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=Eewv7NNeneAk+7wuAFHiFDMNmyFDvsDfe3YE1F4G4Kk=;\n\tb=IERgDy+QqPum07/a4XMoSWU8t2uoa1r+xEGMYFgDCnVcN+KG04xGFxrmEhjR7X83vo\n\tCKgrbSg3dCbOZupP1wBIBMxDVSzsUm5qTp/gOiUJMFjIR3qQ02LorXSlrh/O3lcjnaSM\n\taQZcefNF/jpqL33XFCe85apj+BbdjZA5SV9fZSSJqGnSYw0WV2HQV+lFPOfbixFHUzxc\n\tTV46CisXjsjFof7wriHHFp1n3jtqGalTvSRbhVdHP8zQZwS61nV00OSKfZCBxDIdgf4T\n\tI+u8aMUNprB/9q99FrZiRCyTAev3v5XNWied28S3Ygezj+A/zOre0i6irGrfkYhD8z3H\n\th75g=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"IERgDy+Q\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1687351042; x=1689943042;\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=Eewv7NNeneAk+7wuAFHiFDMNmyFDvsDfe3YE1F4G4Kk=;\n\tb=c82B3/vDkd37qtI50WSZavOqXWkmGLct3AWbJ0tCiioAMVlR4QXntlSeEptutxOYbx\n\tMSKVb4lasRNwow85lJtYWYxQLTNKp0wW/WUQ4qj406wuW15BMQobldPeFSAUecszrW3A\n\tbGy3omPed8NutGtY+j2eRuiDy/ff4CNm/TbG+enn7zebXp7H4LEfSzKFyfnJ+Vx1SSxl\n\tLebidVF9k4O+HW7pBSOol7s9LQtzLjJQh4T9zZvfGxb19UF3BlkfwNM1BMiHi6ISEQnM\n\txbQb+W+yirG/7kBlhslN7sYaq7IoHiRvN3uocaE9eRExHRnWL7ut3sDnJydGMgSfepXQ\n\td5gA==","X-Gm-Message-State":"AC+VfDxv4NakuPOr8RJzdMDyFJdcfi0Zk5faCJrX0goT8UHGVz7Z7280\n\tZJYM7xiTxGLJKTHlFls2P59yku0ZOiz+cTTXfT/9hQ==","X-Google-Smtp-Source":"ACHHUZ7dXDQN7UifW0pKf/DEiGX0EMozYTLFqvOPb+4n7A/oiwQzCyK6WDQH6WsigoTowgZkVIw8vd8tXsA49pKssKE=","X-Received":"by 2002:a05:6214:2aa9:b0:625:b517:6dcb with SMTP id\n\tjs9-20020a0562142aa900b00625b5176dcbmr18195542qvb.5.1687351042228;\n\tWed, 21 Jun 2023 05:37:22 -0700 (PDT)","MIME-Version":"1.0","References":"<20230620142904.72600-1-jacopo.mondi@ideasonboard.com>\n\t\n\t","In-Reply-To":"","Date":"Wed, 21 Jun 2023 13:37:11 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC 0/4] libcamera: Replace\n\tCameraConfiguration::transform","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27407,"web_url":"https://patchwork.libcamera.org/comment/27407/","msgid":"","date":"2023-06-21T12:57:47","subject":"Re: [libcamera-devel] [RFC 0/4] libcamera: Replace\n\tCameraConfiguration::transform","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi David\n\nOn Wed, Jun 21, 2023 at 01:37:11PM +0100, David Plowman wrote:\n> Hi Jacopo\n>\n> Thanks for the reply.\n>\n> On Wed, 21 Jun 2023 at 10:23, Jacopo Mondi\n> wrote:\n> >\n> > Hi David\n> >\n> > On Wed, Jun 21, 2023 at 10:08:25AM +0100, David Plowman wrote:\n> > > Hi Jacopo\n> > >\n> > > Thanks for sending this proposal. I wasn't sure I really understood\n> > > it, perhaps you can help me out!\n> > >\n> > > On Tue, 20 Jun 2023 at 15:29, Jacopo Mondi\n> > > wrote:\n> > > >\n> > > > Hello everyone\n> > > >\n> > > > We have been discussing rotation/transformations for a long time, and\n> > > > the currently implemented CameraConfiguration::transform behaviour has\n> > > > proven to be confusing for several reasons: it was poorly documented,\n> > > > translating it to something consumable for upper layer frameworks was\n> > > > difficult and it behaved differently than any other field part of the\n> > > > CameraConfiguration, as it was meant to tell libcamera \"what to do\" instead\n> > > > of expressing what applications want.\n> > >\n> > > As far as I understand it, I don't believe this is correct, and the\n> > > transform is where you say what you want.\n> > >\n> > > For example, on a Raspberry Pi I always set the transform to\n> > > Transform::Identity because I want the images \"the right way up\". (I\n> > > know that internally, it will calculate and apply a 180 rotation to\n> > > counteract the sensor mounting rotation.) Does that sound right?\n> >\n> > Yes, now that we correct properties::Rotation to 0 as the library\n> > compensate the mounting rotation behind your back, that's what you\n> > get.\n> >\n> > Before we had properties::Rotation = 180; transform = Identity and you\n> > got an upright image. Confusing, isn't it ?\n>\n> I understood the previous arrangement, but the new one seems\n> counter-intuitive to me.\n>\n> On a Raspberry Pi, I expect the properties::Rotation to report 180\n> degrees because the sensor is mounted upside down. Then I can ask for\n> Transform::Identity and internally a 180 degree rotation will be\n> applied to give me what I asked for. I understand this.\n>\n> Are we saying that now the rotation will report 0 degrees? Will it now\n> always report 0 degrees, whatever the sensor and platform? That would\n> obviously break things...\n>\n\nSo you're an app, you read properties::Rotation and you get back 180.\nThen you star streaming and you get a stream oriented \"correctly\".\n\nIsn't this wrong ?\n\n> >\n> > >\n> > > If the problem is actually to do with documentation, I would always\n> > > agree that it can be improved, and am very happy to help!\n> > >\n> > > >\n> > > > For these reasons this series proposes to replace the usage to Transform\n> > > > in the public API in favour of a new CameraConfiguration::Orientation type,\n> > > > defined based on the EXIF specification, tag 274, 'orientation'. For reference\n> > > > this is the same as implemented by gstreamer:\n> > > > https://gstreamer.freedesktop.org/documentation/gstreamer/gsttaglist.html?gi-language=c#GST_TAG_IMAGE_ORIENTATION\n> > >\n> > > I must confess to not liking the term \"orientation\". There is such a\n> > > thing as a \"2D plane transformation\" and I'd expect to refer to them\n> > > as transformations or maybe transforms for short. I don't understand\n> > > what an \"orientation\" is without reading extra documentation. Are\n> > > \"orientations\" composable? Is there such a thing as an \"inverse\n> > > orientation\"?\n> > >\n> >\n> > I'm fine bikeshedding on the name. What matters to me is that now we\n> > conform to an existing specification adopeted by most frameworks\n> > instead of defining our own Transform.\n>\n> I'd like to find something that works for all libcamera users.\n>\n> The Transform class allows composition, inverse operations and so on,\n> the idea being that users can calculate for themselves what to ask for\n> if the default behaviour isn't what they want. I did comment that\n> making it easier for folks to get a specific behaviour might be\n> something to look into.\n>\n> >\n> > > >\n> > > > The newly introduced CameraConfiguration::orientation replaces the\n> > > > existing CameraConfiguration::tranform, and it is meant for application to\n> > > > express how they would like the images to be oriented, not to tell libcamera\n> > > > what to do. As an example, passing in 'rotation0' means that the application\n> > > > expects the images to be rotated upright, and doesn't tell libcamera not to\n> > > > apply any rotation like passing in \"Transform::Identity\" did.\n> > >\n> > > My understanding is that passing \"Transform::Identity\" has always\n> > > meant that the application wants its images \"upright\". (Are there\n> > > cases where this doesn't happen?)\n> > >\n> >\n> > As we have auto-correction in place, yes.\n>\n> My understanding is that everything got \"auto-corrected\" before. I'm\n> still struggling to understand how it seems to be broken now.\n>\n\nIt is not 'broken'. Simply nobody got how thing worked in practice and\nthe existing behaviour (when it comes to adjusting ::transform) is not\nwhat most users expects. See below\n\n> >\n> > > >\n> > > > The value CameraConfiguration::orientation is set to after a validate() also\n> > > > differs in meaning, as instead of reporting \"what applications have to do\n> > > > to obtain what they originally asked for\" it simply reports the actual\n> > > > orientation of the stream: this means that if libcamera cannot fully satisfy the\n> > > > user request it will set ::orientation to report the native images rotation\n> > > > and the CameraConfiguration::status will be set to Adjusted.\n> > >\n> > > I believe this to be the current behaviour of the transform field.\n> > >\n> >\n> > Not really, see below\n> >\n> > > >\n> > > > Handling of 90 and 270 degrees rotation has also changed: as the camera sensor\n> > > > cannot correct rotations that include a transposition, requests for a 90/270\n> > > > degrees are ignored, and cameras with a mounting rotation of 90/270 degrees\n> > > > are never re-oriented. This makes it clear and less confusing for applications\n> > > > that they have to deal with correction fully by themselves. As an example, with\n> > > > the current implementation if the application requires a Rot270 (HFlip +\n> > > > Transpose) libcamera will do the HFlip and leave transposition to the upper\n> > > > layers. There is no clear advantage in doing so, and display compositors are\n> > > > better suited for handling transpositions and flipping in a single pass instead\n> > > > of having the library try to handle part of that.\n> > >\n> > > Again, I'm not really understanding this.\n> > >\n> > > The current behaviour is such that, if it can't give you the\n> > > transformation that you ask for (on account of not being able to do\n> > > transposition), then it guarantees to give you what you requested\n> > > _apart_ from the transposition. So the application only has one case\n> > > to deal with - applying a transpose. It doesn't have to worry about\n> > > the other 3 \"transforms-that-I-can't-handle\", thereby making life\n> > > easier for the application.\n> >\n> > The _apart_ is the key here.\n> >\n> > If you currently ask for Rot270 the library will do an HFlip and set\n> > transform to Transpose (ie what applications still have to do to\n> > obtain Rot270). This is not how the other fields of\n>\n> What it _should_ do is set the transform field to the actual transform\n> that the user will perceive the images to have. I believe this to be\n> \"proper\" libcamera configuration behaviour. I'm not understanding why\n> things are different now.\n>\n\nLet's start with the fact that \"transpose\" is a concept that is\nintroduced in Transform and it's not part of the EXIF standard which\ngstreamer/pipewire expect to use.\n\nAgain, you set transform = rot270, libcamera does HFLIP and you have\nto do Transpose, correct ?\n\nFor compositor this is more work that just do \"rot270\"\n\n> Beyond this, the current behaviour is to leave the application to do\n> the transpose, but that's just a choice. The point of transforms is\n> that it's easy to use them to produce something different if you want.\n> I don't mind looking at that if we can make it easier for everyone to\n> get their preferred behaviour, which I suggested in my original reply.\n>\n\nI don't get why we should be imposing other frameworks/apps to learn\nTranspose when there is a standard to follow to be honest.\n\n> > CameraConfiguration work, and mostly, applying a flip and letting apps\n> > only deal with transpose is an \"optimization\" that might actually make\n> > life harder for apps, as display compositors usually have means to\n> > deal with known rotations, while in this case you only ask them to do\n> > a Transpose, which might require them more work ?\n> >\n> > In any case, I think it's saner if the library cannot do what it has\n> > been asked for to set ::transform to what the stream orientation\n> > actually is, instead of telling apps what \"they still have to do\".\n>\n> I don't understand the repeated statement that the transform is\n> telling apps \"what they still have to do\". That was never what it\n> meant, unless it got changed somewhere.\n>\n\nIsn't it ? I'm not repeating the Rot270 example, but applications are\nleft with ::transform = Transpose. Isn't this what they have to do to\nobtain Rot270 once HFlip has been done by libcamera ?\n\nSo they have to \"I asked for Rot270, I got back Transpose. Ah!\nlibcamera did HFlip for me, so now I have to do Transpose.\"\n\nIs it what happens ?\n\n> >\n> > >\n> > > Because transforms are composable, it's easy to calculate what\n> > > transform to ask for to get a different behaviour (for example, do\n> > > nothing at all if there's a transpose involved). I could imagine a\n> > > slightly different API might be helpful, perhaps where you get to say\n> > > not only \"what you want\", but also what you want the \"residual\n> > > transform\" to be (being the one the application has to apply after) if\n> > > it can't deliver what you wanted. Would that be useful?\n> > >\n> >\n> > I think it's better to leave the stream orientation unmodified if it\n> > cannot be corrected. This matches the behaviour of the other\n> > CameraConfiguration fields, and seems easier to deal with from\n> > applications.\n>\n> I feel we ought to be able to come up with something that makes it\n> convenient for everyone to get what they want.\n>\n> >\n> > > I also recall that there was some discussion a while back about the\n> > > precise orientation of the rotations which could affect some of the\n> > > details, but I don't think it was ever concluded. I don't know if\n> > > that's causing any problems?\n> >\n> > Sorry, I don't remember such discussion. Any link ?\n>\n> I think the last email I sent on the subject was this one, after which\n> things went quiet:\n> https://lists.libcamera.org/pipermail/libcamera-devel/2023-January/036445.html\n> Actually it seems to echo quite a bit of what I say above.\n>\n> Thanks\n> David\n>\n> >\n> > >\n> > > >\n> > > > This series clearly breaks the application API as it removes a member from\n> > > > CameraConfiguration, so it should be introduced probably only when a new release\n> > > > is cut.\n> > >\n> > > I can't comment on this as I haven't really understood the change\n> > > here. Obviously we have many end users for whom an API change would be\n> > > unwelcome, but I suppose the change could be hidden from them.\n> > >\n> > > I apologise if there have been some discussions flying around that I\n> > > haven't followed sufficiently. I'm afraid I don't normally study\n> > > gstreamer related posts in detail, perhaps I need to try harder...\n> > >\n> > > Thanks\n> > > David\n> > >\n> > > >\n> > > > Tested on RPi imx219 with the newly introduced cam option, flip, mirror and\n> > > > rotation work as expected.\n> > > >\n> > > > Tested on Pinephone Pro, where sensors are 90/270 degrees rotated (libcamera\n> > > > performs no correction)\n> > > >\n> > > > Comments welcome of course.\n> > > >\n> > > > Thanks\n> > > > j\n> > > >\n> > > > Jacopo Mondi (4):\n> > > > libcamera: camera_sensor: Cache rotationTransform_\n> > > > libcamera: camera: Introduce CameraConfiguration::orientation\n> > > > libcamera: Move to use CameraConfiguration::orientation\n> > > > apsp: cam: Add option to set stream orientation\n> > > >\n> > > > Documentation/Doxyfile.in | 2 +\n> > > > Documentation/rotation/flip-rotate-0.eps | 170 +++++++++++++++\n> > > > Documentation/rotation/flip-rotate-0.png | Bin 0 -> 16488 bytes\n> > > > Documentation/rotation/flip-rotate-180.eps | 191 +++++++++++++++++\n> > > > Documentation/rotation/flip-rotate-180.png | Bin 0 -> 22198 bytes\n> > > > Documentation/rotation/flip-rotate-270.eps | 197 ++++++++++++++++++\n> > > > Documentation/rotation/flip-rotate-270.png | Bin 0 -> 21367 bytes\n> > > > Documentation/rotation/flip-rotate-90.eps | 195 +++++++++++++++++\n> > > > Documentation/rotation/flip-rotate-90.png | Bin 0 -> 21020 bytes\n> > > > Documentation/rotation/rotate-0.eps | 169 +++++++++++++++\n> > > > Documentation/rotation/rotate-0.png | Bin 0 -> 9086 bytes\n> > > > Documentation/rotation/rotate-180.eps | 189 +++++++++++++++++\n> > > > Documentation/rotation/rotate-180.png | Bin 0 -> 22182 bytes\n> > > > Documentation/rotation/rotate-270.eps | 196 +++++++++++++++++\n> > > > Documentation/rotation/rotate-270.png | Bin 0 -> 21016 bytes\n> > > > Documentation/rotation/rotate-90.eps | 196 +++++++++++++++++\n> > > > Documentation/rotation/rotate-90.png | Bin 0 -> 21528 bytes\n> > > > include/libcamera/camera.h | 14 +-\n> > > > include/libcamera/internal/camera_sensor.h | 4 +-\n> > > > include/libcamera/transform.h | 4 +\n> > > > src/apps/cam/camera_session.cpp | 17 ++\n> > > > src/apps/cam/main.cpp | 5 +\n> > > > src/apps/cam/main.h | 1 +\n> > > > src/libcamera/camera.cpp | 54 ++++-\n> > > > src/libcamera/camera_sensor.cpp | 137 ++++++------\n> > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +-\n> > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +-\n> > > > .../pipeline/rpi/common/pipeline_base.cpp | 9 +-\n> > > > src/libcamera/pipeline/simple/simple.cpp | 6 +-\n> > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +-\n> > > > src/libcamera/pipeline/vimc/vimc.cpp | 4 +-\n> > > > src/libcamera/transform.cpp | 58 ++++++\n> > > > 32 files changed, 1739 insertions(+), 97 deletions(-)\n> > > > create mode 100644 Documentation/rotation/flip-rotate-0.eps\n> > > > create mode 100644 Documentation/rotation/flip-rotate-0.png\n> > > > create mode 100644 Documentation/rotation/flip-rotate-180.eps\n> > > > create mode 100644 Documentation/rotation/flip-rotate-180.png\n> > > > create mode 100644 Documentation/rotation/flip-rotate-270.eps\n> > > > create mode 100644 Documentation/rotation/flip-rotate-270.png\n> > > > create mode 100644 Documentation/rotation/flip-rotate-90.eps\n> > > > create mode 100644 Documentation/rotation/flip-rotate-90.png\n> > > > create mode 100644 Documentation/rotation/rotate-0.eps\n> > > > create mode 100644 Documentation/rotation/rotate-0.png\n> > > > create mode 100644 Documentation/rotation/rotate-180.eps\n> > > > create mode 100644 Documentation/rotation/rotate-180.png\n> > > > create mode 100644 Documentation/rotation/rotate-270.eps\n> > > > create mode 100644 Documentation/rotation/rotate-270.png\n> > > > create mode 100644 Documentation/rotation/rotate-90.eps\n> > > > create mode 100644 Documentation/rotation/rotate-90.png\n> > > >\n> > > > --\n> > > > 2.40.1\n> > > >","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 D0FFAC3237\n\tfor ;\n\tWed, 21 Jun 2023 12:57:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2F1E8628BB;\n\tWed, 21 Jun 2023 14:57:53 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1E9E861E41\n\tfor ;\n\tWed, 21 Jun 2023 14:57:51 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:5d2e:52c9:1cf0:b3bc:c785:4625])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6F5F5105;\n\tWed, 21 Jun 2023 14:57:15 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1687352273;\n\tbh=ueD3Th0m0MaC7KZ+GQht3OFvV1uMwlaoiN1jT2iyu+w=;\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=2PEBFf3zInrA5bsSKxrTYMvRS6bZvknB32CiA/MY4hMx1XmR3DcYeItaoFFweaEQ6\n\t58KBvO+dsYTfg5h5N0MhyUI2XHg9hXZnS8SgxSUArD6C0+6oOnLTJ4DY2Nof9MVkPA\n\t+qpKbpDKF0srAifpt56/mgiGbMWg6o71rZ/yklJZr4Fp9UtnW2mNx8AoJ+wp65Z4NQ\n\tFF/EKXN18KgramsXTIEK/sjB630gd5B8dojWY39AdtumSYmNNBjGxdYsHiFSV+J62j\n\temDJgLcA5RJ4wwVV824zrf0vhh5kqH37/k0EntKVJ+vu502d/CapBujvE+pIhcNDhw\n\tOcoaqNcpus6Og==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1687352235;\n\tbh=ueD3Th0m0MaC7KZ+GQht3OFvV1uMwlaoiN1jT2iyu+w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IfPGgRPYAgrXO6+tuyGgZtUCSsYr+Dq41n7ED4deAAJTS96GXmIG2WCfoSx/Iwttd\n\t9VW4zKKU05WfIB2Rz5uqDu41sWe3NDu7yOEHTRo3N4dlmnkjVSnjsMXoJnXJu5tKzC\n\tgYtHQOCOWUe/6Afra7azPHk90/jPSXmFfTuw+Sp8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"IfPGgRPY\"; dkim-atps=neutral","Date":"Wed, 21 Jun 2023 14:57:47 +0200","To":"David Plowman ","Message-ID":"","References":"<20230620142904.72600-1-jacopo.mondi@ideasonboard.com>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC 0/4] libcamera: Replace\n\tCameraConfiguration::transform","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":27408,"web_url":"https://patchwork.libcamera.org/comment/27408/","msgid":"","date":"2023-06-21T14:14:28","subject":"Re: [libcamera-devel] [RFC 0/4] libcamera: Replace\n\tCameraConfiguration::transform","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi again\n\nOn Wed, 21 Jun 2023 at 13:57, Jacopo Mondi\n wrote:\n>\n> Hi David\n>\n> On Wed, Jun 21, 2023 at 01:37:11PM +0100, David Plowman wrote:\n> > Hi Jacopo\n> >\n> > Thanks for the reply.\n> >\n> > On Wed, 21 Jun 2023 at 10:23, Jacopo Mondi\n> > wrote:\n> > >\n> > > Hi David\n> > >\n> > > On Wed, Jun 21, 2023 at 10:08:25AM +0100, David Plowman wrote:\n> > > > Hi Jacopo\n> > > >\n> > > > Thanks for sending this proposal. I wasn't sure I really understood\n> > > > it, perhaps you can help me out!\n> > > >\n> > > > On Tue, 20 Jun 2023 at 15:29, Jacopo Mondi\n> > > > wrote:\n> > > > >\n> > > > > Hello everyone\n> > > > >\n> > > > > We have been discussing rotation/transformations for a long time, and\n> > > > > the currently implemented CameraConfiguration::transform behaviour has\n> > > > > proven to be confusing for several reasons: it was poorly documented,\n> > > > > translating it to something consumable for upper layer frameworks was\n> > > > > difficult and it behaved differently than any other field part of the\n> > > > > CameraConfiguration, as it was meant to tell libcamera \"what to do\" instead\n> > > > > of expressing what applications want.\n> > > >\n> > > > As far as I understand it, I don't believe this is correct, and the\n> > > > transform is where you say what you want.\n> > > >\n> > > > For example, on a Raspberry Pi I always set the transform to\n> > > > Transform::Identity because I want the images \"the right way up\". (I\n> > > > know that internally, it will calculate and apply a 180 rotation to\n> > > > counteract the sensor mounting rotation.) Does that sound right?\n> > >\n> > > Yes, now that we correct properties::Rotation to 0 as the library\n> > > compensate the mounting rotation behind your back, that's what you\n> > > get.\n> > >\n> > > Before we had properties::Rotation = 180; transform = Identity and you\n> > > got an upright image. Confusing, isn't it ?\n> >\n> > I understood the previous arrangement, but the new one seems\n> > counter-intuitive to me.\n> >\n> > On a Raspberry Pi, I expect the properties::Rotation to report 180\n> > degrees because the sensor is mounted upside down. Then I can ask for\n> > Transform::Identity and internally a 180 degree rotation will be\n> > applied to give me what I asked for. I understand this.\n> >\n> > Are we saying that now the rotation will report 0 degrees? Will it now\n> > always report 0 degrees, whatever the sensor and platform? That would\n> > obviously break things...\n> >\n>\n> So you're an app, you read properties::Rotation and you get back 180.\n> Then you star streaming and you get a stream oriented \"correctly\".\n>\n> Isn't this wrong ?\n\nI think a lot of this hinges on what the Rotation property is.\n\nIt _used_ to be the mounting rotation of the sensor (is that right?),\nat which point everything was as I have described it. But I think it\nis being changed now, which is indeed something that was under\ndiscussion in that thread back in January. We're free to do that, but\nperhaps there have been some unintended consequences?\n\n>\n> > >\n> > > >\n> > > > If the problem is actually to do with documentation, I would always\n> > > > agree that it can be improved, and am very happy to help!\n> > > >\n> > > > >\n> > > > > For these reasons this series proposes to replace the usage to Transform\n> > > > > in the public API in favour of a new CameraConfiguration::Orientation type,\n> > > > > defined based on the EXIF specification, tag 274, 'orientation'. For reference\n> > > > > this is the same as implemented by gstreamer:\n> > > > > https://gstreamer.freedesktop.org/documentation/gstreamer/gsttaglist.html?gi-language=c#GST_TAG_IMAGE_ORIENTATION\n> > > >\n> > > > I must confess to not liking the term \"orientation\". There is such a\n> > > > thing as a \"2D plane transformation\" and I'd expect to refer to them\n> > > > as transformations or maybe transforms for short. I don't understand\n> > > > what an \"orientation\" is without reading extra documentation. Are\n> > > > \"orientations\" composable? Is there such a thing as an \"inverse\n> > > > orientation\"?\n> > > >\n> > >\n> > > I'm fine bikeshedding on the name. What matters to me is that now we\n> > > conform to an existing specification adopeted by most frameworks\n> > > instead of defining our own Transform.\n> >\n> > I'd like to find something that works for all libcamera users.\n> >\n> > The Transform class allows composition, inverse operations and so on,\n> > the idea being that users can calculate for themselves what to ask for\n> > if the default behaviour isn't what they want. I did comment that\n> > making it easier for folks to get a specific behaviour might be\n> > something to look into.\n> >\n> > >\n> > > > >\n> > > > > The newly introduced CameraConfiguration::orientation replaces the\n> > > > > existing CameraConfiguration::tranform, and it is meant for application to\n> > > > > express how they would like the images to be oriented, not to tell libcamera\n> > > > > what to do. As an example, passing in 'rotation0' means that the application\n> > > > > expects the images to be rotated upright, and doesn't tell libcamera not to\n> > > > > apply any rotation like passing in \"Transform::Identity\" did.\n> > > >\n> > > > My understanding is that passing \"Transform::Identity\" has always\n> > > > meant that the application wants its images \"upright\". (Are there\n> > > > cases where this doesn't happen?)\n> > > >\n> > >\n> > > As we have auto-correction in place, yes.\n> >\n> > My understanding is that everything got \"auto-corrected\" before. I'm\n> > still struggling to understand how it seems to be broken now.\n> >\n>\n> It is not 'broken'. Simply nobody got how thing worked in practice and\n> the existing behaviour (when it comes to adjusting ::transform) is not\n> what most users expects. See below\n>\n> > >\n> > > > >\n> > > > > The value CameraConfiguration::orientation is set to after a validate() also\n> > > > > differs in meaning, as instead of reporting \"what applications have to do\n> > > > > to obtain what they originally asked for\" it simply reports the actual\n> > > > > orientation of the stream: this means that if libcamera cannot fully satisfy the\n> > > > > user request it will set ::orientation to report the native images rotation\n> > > > > and the CameraConfiguration::status will be set to Adjusted.\n> > > >\n> > > > I believe this to be the current behaviour of the transform field.\n> > > >\n> > >\n> > > Not really, see below\n> > >\n> > > > >\n> > > > > Handling of 90 and 270 degrees rotation has also changed: as the camera sensor\n> > > > > cannot correct rotations that include a transposition, requests for a 90/270\n> > > > > degrees are ignored, and cameras with a mounting rotation of 90/270 degrees\n> > > > > are never re-oriented. This makes it clear and less confusing for applications\n> > > > > that they have to deal with correction fully by themselves. As an example, with\n> > > > > the current implementation if the application requires a Rot270 (HFlip +\n> > > > > Transpose) libcamera will do the HFlip and leave transposition to the upper\n> > > > > layers. There is no clear advantage in doing so, and display compositors are\n> > > > > better suited for handling transpositions and flipping in a single pass instead\n> > > > > of having the library try to handle part of that.\n> > > >\n> > > > Again, I'm not really understanding this.\n> > > >\n> > > > The current behaviour is such that, if it can't give you the\n> > > > transformation that you ask for (on account of not being able to do\n> > > > transposition), then it guarantees to give you what you requested\n> > > > _apart_ from the transposition. So the application only has one case\n> > > > to deal with - applying a transpose. It doesn't have to worry about\n> > > > the other 3 \"transforms-that-I-can't-handle\", thereby making life\n> > > > easier for the application.\n> > >\n> > > The _apart_ is the key here.\n> > >\n> > > If you currently ask for Rot270 the library will do an HFlip and set\n> > > transform to Transpose (ie what applications still have to do to\n> > > obtain Rot270). This is not how the other fields of\n> >\n> > What it _should_ do is set the transform field to the actual transform\n> > that the user will perceive the images to have. I believe this to be\n> > \"proper\" libcamera configuration behaviour. I'm not understanding why\n> > things are different now.\n> >\n>\n> Let's start with the fact that \"transpose\" is a concept that is\n> introduced in Transform and it's not part of the EXIF standard which\n> gstreamer/pipewire expect to use.\n>\n> Again, you set transform = rot270, libcamera does HFLIP and you have\n> to do Transpose, correct ?\n\nThat's one option. Or, if you want to do a \"rot270\" in the application\nyou just set the transform to \"userTransform * -Transform::rot270\".\n\nAs I've said, I'm really happy to try and make this easier for people.\n\n>\n> For compositor this is more work that just do \"rot270\"\n>\n> > Beyond this, the current behaviour is to leave the application to do\n> > the transpose, but that's just a choice. The point of transforms is\n> > that it's easy to use them to produce something different if you want.\n> > I don't mind looking at that if we can make it easier for everyone to\n> > get their preferred behaviour, which I suggested in my original reply.\n> >\n>\n> I don't get why we should be imposing other frameworks/apps to learn\n> Transpose when there is a standard to follow to be honest.\n>\n> > > CameraConfiguration work, and mostly, applying a flip and letting apps\n> > > only deal with transpose is an \"optimization\" that might actually make\n> > > life harder for apps, as display compositors usually have means to\n> > > deal with known rotations, while in this case you only ask them to do\n> > > a Transpose, which might require them more work ?\n> > >\n> > > In any case, I think it's saner if the library cannot do what it has\n> > > been asked for to set ::transform to what the stream orientation\n> > > actually is, instead of telling apps what \"they still have to do\".\n> >\n> > I don't understand the repeated statement that the transform is\n> > telling apps \"what they still have to do\". That was never what it\n> > meant, unless it got changed somewhere.\n> >\n>\n> Isn't it ? I'm not repeating the Rot270 example, but applications are\n> left with ::transform = Transpose. Isn't this what they have to do to\n> obtain Rot270 once HFlip has been done by libcamera ?\n>\n> So they have to \"I asked for Rot270, I got back Transpose. Ah!\n> libcamera did HFlip for me, so now I have to do Transpose.\"\n>\n> Is it what happens ?\n\nThe code that I am currently running returns HFlip as the transform\nwhen I ask for Rot270, which is indeed what the application will get.\n\nIn this case yes, I still have to do a transpose. But if I had, for\nexample, code that does a 90 degree rotation, I'd ask for \"Rot270 *\n-Rot90\" instead. The transform field would come back as HVFlip telling\nme that my images will be upside down, and all I have to do is call my\nrotate 90 function.\n\nThe idea is that it should be simple to get whatever behaviour you\nwant, but I'm very open to trying to make this easier.\n\n>\n> > >\n> > > >\n> > > > Because transforms are composable, it's easy to calculate what\n> > > > transform to ask for to get a different behaviour (for example, do\n> > > > nothing at all if there's a transpose involved). I could imagine a\n> > > > slightly different API might be helpful, perhaps where you get to say\n> > > > not only \"what you want\", but also what you want the \"residual\n> > > > transform\" to be (being the one the application has to apply after) if\n> > > > it can't deliver what you wanted. Would that be useful?\n> > > >\n> > >\n> > > I think it's better to leave the stream orientation unmodified if it\n> > > cannot be corrected. This matches the behaviour of the other\n> > > CameraConfiguration fields, and seems easier to deal with from\n> > > applications.\n> >\n> > I feel we ought to be able to come up with something that makes it\n> > convenient for everyone to get what they want.\n> >\n> > >\n> > > > I also recall that there was some discussion a while back about the\n> > > > precise orientation of the rotations which could affect some of the\n> > > > details, but I don't think it was ever concluded. I don't know if\n> > > > that's causing any problems?\n> > >\n> > > Sorry, I don't remember such discussion. Any link ?\n> >\n> > I think the last email I sent on the subject was this one, after which\n> > things went quiet:\n> > https://lists.libcamera.org/pipermail/libcamera-devel/2023-January/036445.html\n> > Actually it seems to echo quite a bit of what I say above.\n\nI think one of the sticking points for me is that I don't really\nunderstand what the problems are. Perhaps we could revisit some of\nthis so that I can be more helpful?\n\nThanks\nDavid\n\n> >\n> > Thanks\n> > David\n> >\n> > >\n> > > >\n> > > > >\n> > > > > This series clearly breaks the application API as it removes a member from\n> > > > > CameraConfiguration, so it should be introduced probably only when a new release\n> > > > > is cut.\n> > > >\n> > > > I can't comment on this as I haven't really understood the change\n> > > > here. Obviously we have many end users for whom an API change would be\n> > > > unwelcome, but I suppose the change could be hidden from them.\n> > > >\n> > > > I apologise if there have been some discussions flying around that I\n> > > > haven't followed sufficiently. I'm afraid I don't normally study\n> > > > gstreamer related posts in detail, perhaps I need to try harder...\n> > > >\n> > > > Thanks\n> > > > David\n> > > >\n> > > > >\n> > > > > Tested on RPi imx219 with the newly introduced cam option, flip, mirror and\n> > > > > rotation work as expected.\n> > > > >\n> > > > > Tested on Pinephone Pro, where sensors are 90/270 degrees rotated (libcamera\n> > > > > performs no correction)\n> > > > >\n> > > > > Comments welcome of course.\n> > > > >\n> > > > > Thanks\n> > > > > j\n> > > > >\n> > > > > Jacopo Mondi (4):\n> > > > > libcamera: camera_sensor: Cache rotationTransform_\n> > > > > libcamera: camera: Introduce CameraConfiguration::orientation\n> > > > > libcamera: Move to use CameraConfiguration::orientation\n> > > > > apsp: cam: Add option to set stream orientation\n> > > > >\n> > > > > Documentation/Doxyfile.in | 2 +\n> > > > > Documentation/rotation/flip-rotate-0.eps | 170 +++++++++++++++\n> > > > > Documentation/rotation/flip-rotate-0.png | Bin 0 -> 16488 bytes\n> > > > > Documentation/rotation/flip-rotate-180.eps | 191 +++++++++++++++++\n> > > > > Documentation/rotation/flip-rotate-180.png | Bin 0 -> 22198 bytes\n> > > > > Documentation/rotation/flip-rotate-270.eps | 197 ++++++++++++++++++\n> > > > > Documentation/rotation/flip-rotate-270.png | Bin 0 -> 21367 bytes\n> > > > > Documentation/rotation/flip-rotate-90.eps | 195 +++++++++++++++++\n> > > > > Documentation/rotation/flip-rotate-90.png | Bin 0 -> 21020 bytes\n> > > > > Documentation/rotation/rotate-0.eps | 169 +++++++++++++++\n> > > > > Documentation/rotation/rotate-0.png | Bin 0 -> 9086 bytes\n> > > > > Documentation/rotation/rotate-180.eps | 189 +++++++++++++++++\n> > > > > Documentation/rotation/rotate-180.png | Bin 0 -> 22182 bytes\n> > > > > Documentation/rotation/rotate-270.eps | 196 +++++++++++++++++\n> > > > > Documentation/rotation/rotate-270.png | Bin 0 -> 21016 bytes\n> > > > > Documentation/rotation/rotate-90.eps | 196 +++++++++++++++++\n> > > > > Documentation/rotation/rotate-90.png | Bin 0 -> 21528 bytes\n> > > > > include/libcamera/camera.h | 14 +-\n> > > > > include/libcamera/internal/camera_sensor.h | 4 +-\n> > > > > include/libcamera/transform.h | 4 +\n> > > > > src/apps/cam/camera_session.cpp | 17 ++\n> > > > > src/apps/cam/main.cpp | 5 +\n> > > > > src/apps/cam/main.h | 1 +\n> > > > > src/libcamera/camera.cpp | 54 ++++-\n> > > > > src/libcamera/camera_sensor.cpp | 137 ++++++------\n> > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +-\n> > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +-\n> > > > > .../pipeline/rpi/common/pipeline_base.cpp | 9 +-\n> > > > > src/libcamera/pipeline/simple/simple.cpp | 6 +-\n> > > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +-\n> > > > > src/libcamera/pipeline/vimc/vimc.cpp | 4 +-\n> > > > > src/libcamera/transform.cpp | 58 ++++++\n> > > > > 32 files changed, 1739 insertions(+), 97 deletions(-)\n> > > > > create mode 100644 Documentation/rotation/flip-rotate-0.eps\n> > > > > create mode 100644 Documentation/rotation/flip-rotate-0.png\n> > > > > create mode 100644 Documentation/rotation/flip-rotate-180.eps\n> > > > > create mode 100644 Documentation/rotation/flip-rotate-180.png\n> > > > > create mode 100644 Documentation/rotation/flip-rotate-270.eps\n> > > > > create mode 100644 Documentation/rotation/flip-rotate-270.png\n> > > > > create mode 100644 Documentation/rotation/flip-rotate-90.eps\n> > > > > create mode 100644 Documentation/rotation/flip-rotate-90.png\n> > > > > create mode 100644 Documentation/rotation/rotate-0.eps\n> > > > > create mode 100644 Documentation/rotation/rotate-0.png\n> > > > > create mode 100644 Documentation/rotation/rotate-180.eps\n> > > > > create mode 100644 Documentation/rotation/rotate-180.png\n> > > > > create mode 100644 Documentation/rotation/rotate-270.eps\n> > > > > create mode 100644 Documentation/rotation/rotate-270.png\n> > > > > create mode 100644 Documentation/rotation/rotate-90.eps\n> > > > > create mode 100644 Documentation/rotation/rotate-90.png\n> > > > >\n> > > > > --\n> > > > > 2.40.1\n> > > > >","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 0B9F3BD87C\n\tfor ;\n\tWed, 21 Jun 2023 14:14:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 37DCC628C0;\n\tWed, 21 Jun 2023 16:14:42 +0200 (CEST)","from mail-qv1-xf34.google.com (mail-qv1-xf34.google.com\n\t[IPv6:2607:f8b0:4864:20::f34])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D3D5361E41\n\tfor ;\n\tWed, 21 Jun 2023 16:14:40 +0200 (CEST)","by mail-qv1-xf34.google.com with SMTP id\n\t6a1803df08f44-630038f13daso40706286d6.3\n\tfor ;\n\tWed, 21 Jun 2023 07:14:40 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1687356882;\n\tbh=UV3Ph3xpUj/Ts2R/1F47jaNo+4zffVN4SS4IFiNkZYw=;\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=gVeraLeiBXRerIX4fxKcXKT+8r18EfYPUl+EvDpGMWg4fMjMc95wle5s17f86Ifuz\n\tj8SXe2VBFvDHkQTzew/pDlCt3w1JY2B5nofsr5N8ifOHwkvCrKLUr/LXtr70sHjnbV\n\tkJBrRGnp4fb7AOgpeMvTmi1zmRm4pbmj6oeECRLBUOCXw8h4p1Foq5igg6ZvZcX/uW\n\tYrxMOUdJyOFqo6ftS8+b8RhAlR5KlS9fbFFU6JhCtUKUN3mgGbx1C6ODpVWx8vTNTB\n\t6OWVPP6EMx+utX+JkQ7OGFYSaVeX/gj5HecHVs9uwO8JapraSEEFtq6xI2FuPxYePD\n\tao7h57ayf51Zw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1687356879; x=1689948879;\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=ydYGtUWmAzij5+8j65kn6uvMxlc175TvpBWMP0H5JNM=;\n\tb=ESB5pxpYW6iqarqc8A1vu6qaJRk5Nt6xs63V85DXI10dzbWHDlSWWoUr3pbwD4T1qh\n\tR+2iIJi7EkNptLcjEvNyq2tcdIHBqEEkV5jOF5XD3XsI9BvQ/DPjXDZ/1YJcFgqn4oa8\n\tFJEQQP3ZpD82GO19xp/1wxe2ec8upZCjCIk0Uakbg70bZleFDHDL6fIZJOteX1aulvm4\n\t0VC7OHGhyOR/8CyJ/YBgOZTrInY2niMD+SbgYmSis3zrZvi9UuHc7r7cbzRQn6p3Otr+\n\t2u0NiqEqxJBE43gGnrjWBqpbxa8uLL1lX9K+FZwggUEJodo0yWNcwttVbHT053t0on2K\n\tgYLg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"ESB5pxpY\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1687356879; x=1689948879;\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=ydYGtUWmAzij5+8j65kn6uvMxlc175TvpBWMP0H5JNM=;\n\tb=FQafBNNKBch8t0Lm2qiPqjt9e5W69C1baPLtbaY7wbUIAttCIAEDK1bLuuKl0aVOWI\n\t4u1M05ZuCCj4FSyOxH6tkRFRs5XjkZaFmGK0z1ekr8oJoOUqKwWUD1bni+9KQdnYr4xD\n\tu8l2O0wiN290YePbhgBQSMSKKP5NKsvLPaKqfvNqkH3s/YaqYt0E9wraiIBFh3UbjH1P\n\tiTpbxQ1umvcuN8DSIi2WwG5IsraNukCwoC1XLeYvJbB1Rffv6yiBhd+cmCQG2MBrtPV2\n\t9ozF3DuPSUC5IaM6ZNieQF22QjoiZCpg79GkxZQP6w9Sjj39IGq1O/emMfCfzDg+YY+F\n\twBsQ==","X-Gm-Message-State":"AC+VfDw6/6OHlTaqB9lv+FQ9vWNR9Ew93qSeK4KuatuUIibPuBXaYk11\n\tj3yjjBr7ygzKsT/8J6z5xaiLAsF7M1IUeOMAwDUXkg==","X-Google-Smtp-Source":"ACHHUZ6OJWpAm4eUqtbKk/hgYMyuih6Myl6GL5Xz24n2oy/FQ0LECxhr16jvUgY3elk5H7FF+3FiLrcPEynj5ZG6D/o=","X-Received":"by 2002:ad4:5c85:0:b0:5ef:8c79:fe99 with SMTP id\n\to5-20020ad45c85000000b005ef8c79fe99mr21997215qvh.7.1687356879426;\n\tWed, 21 Jun 2023 07:14:39 -0700 (PDT)","MIME-Version":"1.0","References":"<20230620142904.72600-1-jacopo.mondi@ideasonboard.com>\n\t\n\t\n\t\n\t","In-Reply-To":"","Date":"Wed, 21 Jun 2023 15:14:28 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC 0/4] libcamera: Replace\n\tCameraConfiguration::transform","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27409,"web_url":"https://patchwork.libcamera.org/comment/27409/","msgid":"","date":"2023-06-21T14:30:06","subject":"Re: [libcamera-devel] [RFC 0/4] libcamera: Replace\n\tCameraConfiguration::transform","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi David\n let me summarize, as usual the discussion become hard to follow.\n\nOn Wed, Jun 21, 2023 at 03:14:28PM +0100, David Plowman wrote:\n\n[snip]\n\n> > >\n> > > I think the last email I sent on the subject was this one, after which\n> > > things went quiet:\n> > > https://lists.libcamera.org/pipermail/libcamera-devel/2023-January/036445.html\n> > > Actually it seems to echo quite a bit of what I say above.\n>\n> I think one of the sticking points for me is that I don't really\n> understand what the problems are. Perhaps we could revisit some of\n> this so that I can be more helpful?\n>\n\n1) Transform is a libcamera concept, other frameworks have to learn\nhow to use it. A specification exists and it's wildely adopted, why\nshould we diverge from it ? Because there is code already using the\ncurrent implementation ?\n\n2) The library applies partial transforms (like \"I'll do HFlip if I'm\nasked Rot270 and I can't do Transpose\"). This is confusing as\napplications have to compose transforms and do computations. This\npatch simplifies it: either you can do it in full, or leave the stream\nun-touched. There are no clear advantages that I can think of in\npartially applying transforms.\n\n3) properties::Rotation, the documentation does not report 'mounting\nrotation' but I admit we (I myself for one) have often used that term.\nThat's because, originally, my understanding was that no\nautocorrection should have been performed. Since it has been\nintroduced (by copying your PH implementation) and others liked it,\nnow properties::Rotation reports the stream rotation when no\n::transform is specified. I think it's confusing to have\nproperties::Rotation=180 and a stream oriented correctly, applications\nmight wonder what's happening and if libcamera is doing something\nbehind their back.\n\nI think these are the main discussion points, aren't they ?\n\n> Thanks\n> David\n>\n> > >\n> > > Thanks\n> > > David\n> > >\n> > > >\n> > > > >\n> > > > > >\n> > > > > > This series clearly breaks the application API as it removes a member from\n> > > > > > CameraConfiguration, so it should be introduced probably only when a new release\n> > > > > > is cut.\n> > > > >\n> > > > > I can't comment on this as I haven't really understood the change\n> > > > > here. Obviously we have many end users for whom an API change would be\n> > > > > unwelcome, but I suppose the change could be hidden from them.\n> > > > >\n> > > > > I apologise if there have been some discussions flying around that I\n> > > > > haven't followed sufficiently. I'm afraid I don't normally study\n> > > > > gstreamer related posts in detail, perhaps I need to try harder...\n> > > > >\n> > > > > Thanks\n> > > > > David\n> > > > >\n> > > > > >\n> > > > > > Tested on RPi imx219 with the newly introduced cam option, flip, mirror and\n> > > > > > rotation work as expected.\n> > > > > >\n> > > > > > Tested on Pinephone Pro, where sensors are 90/270 degrees rotated (libcamera\n> > > > > > performs no correction)\n> > > > > >\n> > > > > > Comments welcome of course.\n> > > > > >\n> > > > > > Thanks\n> > > > > > j\n> > > > > >\n> > > > > > Jacopo Mondi (4):\n> > > > > > libcamera: camera_sensor: Cache rotationTransform_\n> > > > > > libcamera: camera: Introduce CameraConfiguration::orientation\n> > > > > > libcamera: Move to use CameraConfiguration::orientation\n> > > > > > apsp: cam: Add option to set stream orientation\n> > > > > >\n> > > > > > Documentation/Doxyfile.in | 2 +\n> > > > > > Documentation/rotation/flip-rotate-0.eps | 170 +++++++++++++++\n> > > > > > Documentation/rotation/flip-rotate-0.png | Bin 0 -> 16488 bytes\n> > > > > > Documentation/rotation/flip-rotate-180.eps | 191 +++++++++++++++++\n> > > > > > Documentation/rotation/flip-rotate-180.png | Bin 0 -> 22198 bytes\n> > > > > > Documentation/rotation/flip-rotate-270.eps | 197 ++++++++++++++++++\n> > > > > > Documentation/rotation/flip-rotate-270.png | Bin 0 -> 21367 bytes\n> > > > > > Documentation/rotation/flip-rotate-90.eps | 195 +++++++++++++++++\n> > > > > > Documentation/rotation/flip-rotate-90.png | Bin 0 -> 21020 bytes\n> > > > > > Documentation/rotation/rotate-0.eps | 169 +++++++++++++++\n> > > > > > Documentation/rotation/rotate-0.png | Bin 0 -> 9086 bytes\n> > > > > > Documentation/rotation/rotate-180.eps | 189 +++++++++++++++++\n> > > > > > Documentation/rotation/rotate-180.png | Bin 0 -> 22182 bytes\n> > > > > > Documentation/rotation/rotate-270.eps | 196 +++++++++++++++++\n> > > > > > Documentation/rotation/rotate-270.png | Bin 0 -> 21016 bytes\n> > > > > > Documentation/rotation/rotate-90.eps | 196 +++++++++++++++++\n> > > > > > Documentation/rotation/rotate-90.png | Bin 0 -> 21528 bytes\n> > > > > > include/libcamera/camera.h | 14 +-\n> > > > > > include/libcamera/internal/camera_sensor.h | 4 +-\n> > > > > > include/libcamera/transform.h | 4 +\n> > > > > > src/apps/cam/camera_session.cpp | 17 ++\n> > > > > > src/apps/cam/main.cpp | 5 +\n> > > > > > src/apps/cam/main.h | 1 +\n> > > > > > src/libcamera/camera.cpp | 54 ++++-\n> > > > > > src/libcamera/camera_sensor.cpp | 137 ++++++------\n> > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +-\n> > > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +-\n> > > > > > .../pipeline/rpi/common/pipeline_base.cpp | 9 +-\n> > > > > > src/libcamera/pipeline/simple/simple.cpp | 6 +-\n> > > > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +-\n> > > > > > src/libcamera/pipeline/vimc/vimc.cpp | 4 +-\n> > > > > > src/libcamera/transform.cpp | 58 ++++++\n> > > > > > 32 files changed, 1739 insertions(+), 97 deletions(-)\n> > > > > > create mode 100644 Documentation/rotation/flip-rotate-0.eps\n> > > > > > create mode 100644 Documentation/rotation/flip-rotate-0.png\n> > > > > > create mode 100644 Documentation/rotation/flip-rotate-180.eps\n> > > > > > create mode 100644 Documentation/rotation/flip-rotate-180.png\n> > > > > > create mode 100644 Documentation/rotation/flip-rotate-270.eps\n> > > > > > create mode 100644 Documentation/rotation/flip-rotate-270.png\n> > > > > > create mode 100644 Documentation/rotation/flip-rotate-90.eps\n> > > > > > create mode 100644 Documentation/rotation/flip-rotate-90.png\n> > > > > > create mode 100644 Documentation/rotation/rotate-0.eps\n> > > > > > create mode 100644 Documentation/rotation/rotate-0.png\n> > > > > > create mode 100644 Documentation/rotation/rotate-180.eps\n> > > > > > create mode 100644 Documentation/rotation/rotate-180.png\n> > > > > > create mode 100644 Documentation/rotation/rotate-270.eps\n> > > > > > create mode 100644 Documentation/rotation/rotate-270.png\n> > > > > > create mode 100644 Documentation/rotation/rotate-90.eps\n> > > > > > create mode 100644 Documentation/rotation/rotate-90.png\n> > > > > >\n> > > > > > --\n> > > > > > 2.40.1\n> > > > > >","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 95ED3C3237\n\tfor ;\n\tWed, 21 Jun 2023 14:30:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EAB64628C0;\n\tWed, 21 Jun 2023 16:30:10 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AE9E061E41\n\tfor ;\n\tWed, 21 Jun 2023 16:30:09 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:5d2e:52c9:1cf0:b3bc:c785:4625])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 137A0A2D;\n\tWed, 21 Jun 2023 16:29:34 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1687357811;\n\tbh=Doir77u6KF5WXwMMHA6MFIEy3p3iMdnt43dWbj0VXyw=;\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=w/kaLmX2HKIKochFGCwfRxZfxgX+RoUH5k/0qn4BqKHeJJXTFQxsNIDEHN14JFFDx\n\tvAZG2gMbaMfhivf9B+LunKmbiABG5OW8A9BIKI+j6WnoShhLQjdzl45hsteXKQaZxj\n\t4enBNcpawZQi7ep9wqg8adG4cLHdGtuMteGhGWSUPK6cTXAJfsEqIMoKyWBWKDKLv1\n\tz0nCe3YsfZ8fX24ULOchpxnls/OzdMgne98zc7RjQB026XTmwgflIgmp8PlnzrzZny\n\tNcLtLEIgY8gWe/PESGShI+odna2vhTa4HqUNL7RsVr5Vt9II1oKwZwaZd6k0yZyRun\n\tWjvaZpFXCZfhQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1687357774;\n\tbh=Doir77u6KF5WXwMMHA6MFIEy3p3iMdnt43dWbj0VXyw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=K9Pvwio1I2wGAbB3WMByWn0Bn4T/xyEP8F/taDevv8a5ArGKiqbHtDlkYrzAgVkBQ\n\tdJ8qY1lPwgwuvC1/HA6XINlvHRcJlbz57R4Gci/ZPSuyUDhKb9xfY1rwYTUHEYHTN8\n\tzReZWhavo4Q6OCUS54yPq8YWIzCNNyfYis9Biz+4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"K9Pvwio1\"; dkim-atps=neutral","Date":"Wed, 21 Jun 2023 16:30:06 +0200","To":"David Plowman ","Message-ID":"","References":"<20230620142904.72600-1-jacopo.mondi@ideasonboard.com>\n\t\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC 0/4] libcamera: Replace\n\tCameraConfiguration::transform","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":27410,"web_url":"https://patchwork.libcamera.org/comment/27410/","msgid":"","date":"2023-06-21T16:33:45","subject":"Re: [libcamera-devel] [RFC 0/4] libcamera: Replace\n\tCameraConfiguration::transform","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi again\n\nOn Wed, 21 Jun 2023 at 15:30, Jacopo Mondi\n wrote:\n>\n> Hi David\n> let me summarize, as usual the discussion become hard to follow.\n>\n> On Wed, Jun 21, 2023 at 03:14:28PM +0100, David Plowman wrote:\n>\n> [snip]\n>\n> > > >\n> > > > I think the last email I sent on the subject was this one, after which\n> > > > things went quiet:\n> > > > https://lists.libcamera.org/pipermail/libcamera-devel/2023-January/036445.html\n> > > > Actually it seems to echo quite a bit of what I say above.\n> >\n> > I think one of the sticking points for me is that I don't really\n> > understand what the problems are. Perhaps we could revisit some of\n> > this so that I can be more helpful?\n> >\n>\n> 1) Transform is a libcamera concept, other frameworks have to learn\n> how to use it. A specification exists and it's wildely adopted, why\n> should we diverge from it ? Because there is code already using the\n> current implementation ?\n\nThe orientation flag is completely identical to the transform, just\nwith different names, so I don't feel this should be an area of\ndifficulty. We can change the names, or add synonyms, I don't really\nmind. But the transform does have useful compose and inverse functions\nwhich I would not want to lose.\n\nThis code is in quite wide use in the Pi community, so I do generally\nwant to inconvenience users as little as possible.\n\n>\n> 2) The library applies partial transforms (like \"I'll do HFlip if I'm\n> asked Rot270 and I can't do Transpose\"). This is confusing as\n> applications have to compose transforms and do computations. This\n> patch simplifies it: either you can do it in full, or leave the stream\n> un-touched. There are no clear advantages that I can think of in\n> partially applying transforms.\n\nI like the partial transforms because it means I have only one\n\"exception\" case to deal with, and not four. In particular, I can\ncontrol what the \"left over\" transform will be, if I want to. Though\nof course, I don't have to.\n\nI think it would be OK to change the default behaviour to what you\nhave described (i.e. do nothing at all if it can't do the whole\ntransform), so long as I can still generate the transform that I want,\nor my users are able to.\n\n>\n> 3) properties::Rotation, the documentation does not report 'mounting\n> rotation' but I admit we (I myself for one) have often used that term.\n> That's because, originally, my understanding was that no\n> autocorrection should have been performed. Since it has been\n> introduced (by copying your PH implementation) and others liked it,\n> now properties::Rotation reports the stream rotation when no\n> ::transform is specified. I think it's confusing to have\n> properties::Rotation=180 and a stream oriented correctly, applications\n> might wonder what's happening and if libcamera is doing something\n> behind their back.\n\nI don't mind changing the meaning of the Rotation property, but I\nthink we should also be fixing up the existing code so that its\nbehaviour doesn't change. It would also be nice if you could query the\nmounting rotation from an application, though I would agree an\napplication should never need to use it.\n\n>\n> I think these are the main discussion points, aren't they ?\n\nI'd like to make a proposal, if I may.\n\n1. I'm happy to change the Rotation property, but at the same time we\nneed to fix the code so that the behaviour of transforms is not\nchanged. I would still like there to be a property so that an\napplication can query the mounting rotation if it wants to.\n2. We change the default behaviour to apply no transform at all if it\ncan't apply the full transform. (Applications that don't want this can\neasily sort themselves out now.)\n\nAt this point I think everyone has the functionality that they want.\nJust to try and flesh out the application code a bit so that we are\nforced to understand it, we have the following.\n\nApplications that want the simple behaviour:\n\nTransform requestedTransform; // the transform the user wants\nTransform actualTransform; // what the user will get, from checking\nthe transform field after validate()\n\nif (requestedTransform != actualTransform)\n /* The application will have to apply a transform equivalent to\n-actualTransform * requestedTransform */;\n\nApplications that want to control the transform that they apply (let's\ncall this the \"leftoverTransform\"):\n\nif (requestedTransform != actualTransform)\n requestedTransform *= -leftoverTransform; // now just try again\n\nBut maybe double-check if that all feels right!!\n\n3. We can consider swapping the transform in the configuration for an\n\"orientation\" with a different set of names. We need to add\ntransform/orientation conversion functions so that I (and my users)\ncan still use it.\n\nHow does that sound?\n\nDavid\n\n>\n> > Thanks\n> > David\n> >\n> > > >\n> > > > Thanks\n> > > > David\n> > > >\n> > > > >\n> > > > > >\n> > > > > > >\n> > > > > > > This series clearly breaks the application API as it removes a member from\n> > > > > > > CameraConfiguration, so it should be introduced probably only when a new release\n> > > > > > > is cut.\n> > > > > >\n> > > > > > I can't comment on this as I haven't really understood the change\n> > > > > > here. Obviously we have many end users for whom an API change would be\n> > > > > > unwelcome, but I suppose the change could be hidden from them.\n> > > > > >\n> > > > > > I apologise if there have been some discussions flying around that I\n> > > > > > haven't followed sufficiently. I'm afraid I don't normally study\n> > > > > > gstreamer related posts in detail, perhaps I need to try harder...\n> > > > > >\n> > > > > > Thanks\n> > > > > > David\n> > > > > >\n> > > > > > >\n> > > > > > > Tested on RPi imx219 with the newly introduced cam option, flip, mirror and\n> > > > > > > rotation work as expected.\n> > > > > > >\n> > > > > > > Tested on Pinephone Pro, where sensors are 90/270 degrees rotated (libcamera\n> > > > > > > performs no correction)\n> > > > > > >\n> > > > > > > Comments welcome of course.\n> > > > > > >\n> > > > > > > Thanks\n> > > > > > > j\n> > > > > > >\n> > > > > > > Jacopo Mondi (4):\n> > > > > > > libcamera: camera_sensor: Cache rotationTransform_\n> > > > > > > libcamera: camera: Introduce CameraConfiguration::orientation\n> > > > > > > libcamera: Move to use CameraConfiguration::orientation\n> > > > > > > apsp: cam: Add option to set stream orientation\n> > > > > > >\n> > > > > > > Documentation/Doxyfile.in | 2 +\n> > > > > > > Documentation/rotation/flip-rotate-0.eps | 170 +++++++++++++++\n> > > > > > > Documentation/rotation/flip-rotate-0.png | Bin 0 -> 16488 bytes\n> > > > > > > Documentation/rotation/flip-rotate-180.eps | 191 +++++++++++++++++\n> > > > > > > Documentation/rotation/flip-rotate-180.png | Bin 0 -> 22198 bytes\n> > > > > > > Documentation/rotation/flip-rotate-270.eps | 197 ++++++++++++++++++\n> > > > > > > Documentation/rotation/flip-rotate-270.png | Bin 0 -> 21367 bytes\n> > > > > > > Documentation/rotation/flip-rotate-90.eps | 195 +++++++++++++++++\n> > > > > > > Documentation/rotation/flip-rotate-90.png | Bin 0 -> 21020 bytes\n> > > > > > > Documentation/rotation/rotate-0.eps | 169 +++++++++++++++\n> > > > > > > Documentation/rotation/rotate-0.png | Bin 0 -> 9086 bytes\n> > > > > > > Documentation/rotation/rotate-180.eps | 189 +++++++++++++++++\n> > > > > > > Documentation/rotation/rotate-180.png | Bin 0 -> 22182 bytes\n> > > > > > > Documentation/rotation/rotate-270.eps | 196 +++++++++++++++++\n> > > > > > > Documentation/rotation/rotate-270.png | Bin 0 -> 21016 bytes\n> > > > > > > Documentation/rotation/rotate-90.eps | 196 +++++++++++++++++\n> > > > > > > Documentation/rotation/rotate-90.png | Bin 0 -> 21528 bytes\n> > > > > > > include/libcamera/camera.h | 14 +-\n> > > > > > > include/libcamera/internal/camera_sensor.h | 4 +-\n> > > > > > > include/libcamera/transform.h | 4 +\n> > > > > > > src/apps/cam/camera_session.cpp | 17 ++\n> > > > > > > src/apps/cam/main.cpp | 5 +\n> > > > > > > src/apps/cam/main.h | 1 +\n> > > > > > > src/libcamera/camera.cpp | 54 ++++-\n> > > > > > > src/libcamera/camera_sensor.cpp | 137 ++++++------\n> > > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +-\n> > > > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +-\n> > > > > > > .../pipeline/rpi/common/pipeline_base.cpp | 9 +-\n> > > > > > > src/libcamera/pipeline/simple/simple.cpp | 6 +-\n> > > > > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +-\n> > > > > > > src/libcamera/pipeline/vimc/vimc.cpp | 4 +-\n> > > > > > > src/libcamera/transform.cpp | 58 ++++++\n> > > > > > > 32 files changed, 1739 insertions(+), 97 deletions(-)\n> > > > > > > create mode 100644 Documentation/rotation/flip-rotate-0.eps\n> > > > > > > create mode 100644 Documentation/rotation/flip-rotate-0.png\n> > > > > > > create mode 100644 Documentation/rotation/flip-rotate-180.eps\n> > > > > > > create mode 100644 Documentation/rotation/flip-rotate-180.png\n> > > > > > > create mode 100644 Documentation/rotation/flip-rotate-270.eps\n> > > > > > > create mode 100644 Documentation/rotation/flip-rotate-270.png\n> > > > > > > create mode 100644 Documentation/rotation/flip-rotate-90.eps\n> > > > > > > create mode 100644 Documentation/rotation/flip-rotate-90.png\n> > > > > > > create mode 100644 Documentation/rotation/rotate-0.eps\n> > > > > > > create mode 100644 Documentation/rotation/rotate-0.png\n> > > > > > > create mode 100644 Documentation/rotation/rotate-180.eps\n> > > > > > > create mode 100644 Documentation/rotation/rotate-180.png\n> > > > > > > create mode 100644 Documentation/rotation/rotate-270.eps\n> > > > > > > create mode 100644 Documentation/rotation/rotate-270.png\n> > > > > > > create mode 100644 Documentation/rotation/rotate-90.eps\n> > > > > > > create mode 100644 Documentation/rotation/rotate-90.png\n> > > > > > >\n> > > > > > > --\n> > > > > > > 2.40.1\n> > > > > > >","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 5DBFABD87C\n\tfor ;\n\tWed, 21 Jun 2023 16:34:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 98E27628BB;\n\tWed, 21 Jun 2023 18:33:59 +0200 (CEST)","from mail-qk1-x729.google.com (mail-qk1-x729.google.com\n\t[IPv6:2607:f8b0:4864:20::729])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AE92361E41\n\tfor ;\n\tWed, 21 Jun 2023 18:33:57 +0200 (CEST)","by mail-qk1-x729.google.com with SMTP id\n\taf79cd13be357-76240c53846so466534485a.2\n\tfor ;\n\tWed, 21 Jun 2023 09:33:57 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1687365239;\n\tbh=BsF4+nPaCXmDAkHJexglIj/+jHHZqGD2wmpE2sRiCkA=;\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=cTQ0NX6jfbco/7qK5N1wvW08jyz61Wi9oBfR0SQHiIw4aEUU5/FmQg2ENTRJujOoR\n\tHhI11aQSy0EeBI3cqu510w5vJLZHEOFJ6Di4RsRZdwfMku70Nn4CtjBgnKIysc/FUf\n\tbBneMd0Fxzfk8X5HmS0l8Ly1RfCXL45BqqgVzcFNgeOepr20cFwB8ezECkMEnYgCt6\n\tJ4t4KgDhrwD2HX3fo3wFfvAedpqJaYj2q87G84tFpHIppsXSFi22lE8mjzd500o40K\n\tYU/KogzDvC544ezCQzQ7lLNsUsBI4nUbkIIQiqKkBJsDqqWq7OC7xkuCX4oUJ2flyC\n\t5ultRiyYLDIRw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1687365236; x=1689957236;\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=QFEX3Mr4KAC3fncT2gNOVA/SZ8uCwuq536gj56KAHjU=;\n\tb=UVCdqcjHWOpGUJbJ/qHvafDIXhrVgFmxOhb3qoVrB7rJ/QyDMaDbw3Ex7ieBoDE2AU\n\tsOicP/hgFyXLrzYREfMQvEcLGk5LfPfuoPDyRiJf175wOKBeiTvFzIxUnps2BOJHIsiy\n\t93omLJ5IL6MoV+VbQqtRuLizJ/85iobpXAv0jJKDkbJINUSewKaZ/8YXL8u+h/EGIAO4\n\t5yyfbneY/qpNz8WZyFD3ivq+xrYApFjZo+s1A/ZSZ9R71lluVdt/VxxR7PojvmcFi9kY\n\t/PNgsPuq+D4zcOE38fNgvxQtttIszFBo4Va3P320o18f6wCQxgIgH88zc9rUlsGctgOX\n\tPlkA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"UVCdqcjH\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1687365236; x=1689957236;\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=QFEX3Mr4KAC3fncT2gNOVA/SZ8uCwuq536gj56KAHjU=;\n\tb=NJa7d4av6zEOHjQMsaKr/b1MMMCw8ee2P2ZfNiWFMPssGzGcuDax5Wlz9YuHFjdaLJ\n\tiaNTh1yZ0My8jSudCQw7G8xdJh3zxPJXtL5frYMPTuOHIHr+9ZhDpKpUe5B+/u0BmUkZ\n\t5BISyGEiFsvmeDJ6WUQaErBBXoz0TxeaOCvBKN0oFVigbzh+xnNgVUHtF76T02nYtuky\n\t9R4nIHQAAPrIYuIqoDLQEoa6R/8EHpkt8ZFcPPE58o70c5Wc6OrcJwG/yQqwvuFYZ3iy\n\t4WP3Plm2K5QecvND4Mk4lXSD6rKbxB66+pnDVox22T9R2OMUyoRwkgxjYGxQj1b8XC7w\n\t6EkQ==","X-Gm-Message-State":"AC+VfDwBjqHiq6wJK7WCWZz7i39EuF5Bv68TzFLjTC/OO7foZK9EqwBp\n\ttaYrpi5U0PS7GxVriaB5bQhljLbLaS/KfUeO98q06A==","X-Google-Smtp-Source":"ACHHUZ5xLehjj44n54GCoRba8ZfVzSz8iKQ2+ONso7nC3+K08q5gt0veDe+oDMmYqUwgTkn34n1sbBB3ArNspy375HM=","X-Received":"by 2002:ad4:5b8a:0:b0:62f:e4dd:d607 with SMTP id\n\t10-20020ad45b8a000000b0062fe4ddd607mr19587928qvp.23.1687365236365;\n\tWed, 21 Jun 2023 09:33:56 -0700 (PDT)","MIME-Version":"1.0","References":"<20230620142904.72600-1-jacopo.mondi@ideasonboard.com>\n\t\n\t\n\t\n\t\n\t\n\t","In-Reply-To":"","Date":"Wed, 21 Jun 2023 17:33:45 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC 0/4] libcamera: Replace\n\tCameraConfiguration::transform","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27411,"web_url":"https://patchwork.libcamera.org/comment/27411/","msgid":"<73lhowfjv54atljwwggxb6jfs2rc6cgjyksp6d3ks4bsmw2y6k@m5jwwbwlpugz>","date":"2023-06-22T08:08:09","subject":"Re: [libcamera-devel] [RFC 0/4] libcamera: Replace\n\tCameraConfiguration::transform","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi David\n\nOn Wed, Jun 21, 2023 at 05:33:45PM +0100, David Plowman wrote:\n> Hi again\n>\n> On Wed, 21 Jun 2023 at 15:30, Jacopo Mondi\n> wrote:\n> >\n> > Hi David\n> > let me summarize, as usual the discussion become hard to follow.\n> >\n> > On Wed, Jun 21, 2023 at 03:14:28PM +0100, David Plowman wrote:\n> >\n> > [snip]\n> >\n> > > > >\n> > > > > I think the last email I sent on the subject was this one, after which\n> > > > > things went quiet:\n> > > > > https://lists.libcamera.org/pipermail/libcamera-devel/2023-January/036445.html\n> > > > > Actually it seems to echo quite a bit of what I say above.\n> > >\n> > > I think one of the sticking points for me is that I don't really\n> > > understand what the problems are. Perhaps we could revisit some of\n> > > this so that I can be more helpful?\n> > >\n> >\n> > 1) Transform is a libcamera concept, other frameworks have to learn\n> > how to use it. A specification exists and it's wildely adopted, why\n> > should we diverge from it ? Because there is code already using the\n> > current implementation ?\n>\n> The orientation flag is completely identical to the transform, just\n> with different names, so I don't feel this should be an area of\n\nWhy do you say it is identical ?\n\nThe Orientation enumeration is defined based on the EXIF tag 274\n'orientation' and describes 2D transforms in terms of clock-wise\nrotation and horizontal mirroring, while Transform is a custom-defined\ntype which uses Transpose and V/HFlips. Why do you think they're\nidentical ?\n\nThe EXIF defined values are also used by\n\ngstreamer/gtk\nhttps://api.gtkd.org/gstreamer.c.types.TAG_IMAGE_ORIENTATION.html\n\npipewire\nhttps://docs.pipewire.org/group__spa__buffer.html#gab4afeb1516aa33126757cfa275efd7a3\n\n> difficulty. We can change the names, or add synonyms, I don't really\n> mind. But the transform does have useful compose and inverse functions\n> which I would not want to lose.\n>\n> This code is in quite wide use in the Pi community, so I do generally\n> want to inconvenience users as little as possible.\n>\n\nI understand this argument, but it can't be used as an excuse to never\nchange the API, in particular if there is a standard already adopted\nby other frameworks we don't ahere to.\n\n> >\n> > 2) The library applies partial transforms (like \"I'll do HFlip if I'm\n> > asked Rot270 and I can't do Transpose\"). This is confusing as\n> > applications have to compose transforms and do computations. This\n> > patch simplifies it: either you can do it in full, or leave the stream\n> > un-touched. There are no clear advantages that I can think of in\n> > partially applying transforms.\n>\n> I like the partial transforms because it means I have only one\n> \"exception\" case to deal with, and not four. In particular, I can\n> control what the \"left over\" transform will be, if I want to. Though\n> of course, I don't have to.\n>\n\nI think what makes a difference here is the behaviour of the consumers\nof a \"rotated\" image.\n\nWhat is the \"next\" component that in most cases have to compensate for\na non-fully corrected rotation ? I presume it generally is the display\ncompositor or a framework like gstreamer that has elements like\n'videoflip'. We know gtk/gstreamer internally uses the EXIF tag, but\nwhen it comes to compositor, why should for them be better to deal\nwith \"Transpose\" instead that on Rot270 ? I'm no expert there but I\npresume rendering libraries have methods for quickly rotate images\nbefore rendering but they might not have \"transpose only\" ones. Also\nbecause \"transpose along the diagonal axis from the top-left corner to\nthe right-bottom one\" is a concept I haven't found documented anywhere\nelse if not in the Transform class (of course I might have missed it\nas I'm certainly no expert on rendering).\n\n> I think it would be OK to change the default behaviour to what you\n> have described (i.e. do nothing at all if it can't do the whole\n> transform), so long as I can still generate the transform that I want,\n> or my users are able to.\n>\n> >\n> > 3) properties::Rotation, the documentation does not report 'mounting\n> > rotation' but I admit we (I myself for one) have often used that term.\n> > That's because, originally, my understanding was that no\n> > autocorrection should have been performed. Since it has been\n> > introduced (by copying your PH implementation) and others liked it,\n> > now properties::Rotation reports the stream rotation when no\n> > ::transform is specified. I think it's confusing to have\n> > properties::Rotation=180 and a stream oriented correctly, applications\n> > might wonder what's happening and if libcamera is doing something\n> > behind their back.\n>\n> I don't mind changing the meaning of the Rotation property, but I\n> think we should also be fixing up the existing code so that its\n> behaviour doesn't change. It would also be nice if you could query the\n\nWhat existing code relies on Rotation being the mounting rotation ? In\nlibcamera ? In the apps ?\n\n> mounting rotation from an application, though I would agree an\n> application should never need to use it.\n>\n> >\n> > I think these are the main discussion points, aren't they ?\n>\n> I'd like to make a proposal, if I may.\n>\n> 1. I'm happy to change the Rotation property, but at the same time we\n> need to fix the code so that the behaviour of transforms is not\n> changed. I would still like there to be a property so that an\n> application can query the mounting rotation if it wants to.\n\nI'm fine introducing one\n\n> 2. We change the default behaviour to apply no transform at all if it\n> can't apply the full transform. (Applications that don't want this can\n> easily sort themselves out now.)\n>\n> At this point I think everyone has the functionality that they want.\n> Just to try and flesh out the application code a bit so that we are\n> forced to understand it, we have the following.\n>\n> Applications that want the simple behaviour:\n>\n> Transform requestedTransform; // the transform the user wants\n> Transform actualTransform; // what the user will get, from checking\n> the transform field after validate()\n>\n> if (requestedTransform != actualTransform)\n> /* The application will have to apply a transform equivalent to\n> -actualTransform * requestedTransform */;\n>\n> Applications that want to control the transform that they apply (let's\n> call this the \"leftoverTransform\"):\n>\n> if (requestedTransform != actualTransform)\n> requestedTransform *= -leftoverTransform; // now just try again\n\nI still don't get why people should do any of this calculations.\n\nI don't expect anyone to write their own rotation libraries, but\ninstead rely on the rendering libraries or on piping gstreamer\nelements to the capture output. Why should they calculate the\n\"leftoverTransform\" ? What is the use case for an application in doing\nthis ?\n\n>\n> But maybe double-check if that all feels right!!\n\nI think it's relevant to know how gstreamer/pipewire expects to\ndeal with transforms, and more importantly what 'transform' display\ncompositors supports natively.\n\n>\n> 3. We can consider swapping the transform in the configuration for an\n> \"orientation\" with a different set of names. We need to add\n> transform/orientation conversion functions so that I (and my users)\n> can still use it.\n\nSee the newly added functions in transform.cpp\n\n>\n> How does that sound?\n>\n> David\n>\n> >\n> > > Thanks\n> > > David\n> > >\n> > > > >\n> > > > > Thanks\n> > > > > David\n> > > > >\n> > > > > >\n> > > > > > >\n> > > > > > > >\n> > > > > > > > This series clearly breaks the application API as it removes a member from\n> > > > > > > > CameraConfiguration, so it should be introduced probably only when a new release\n> > > > > > > > is cut.\n> > > > > > >\n> > > > > > > I can't comment on this as I haven't really understood the change\n> > > > > > > here. Obviously we have many end users for whom an API change would be\n> > > > > > > unwelcome, but I suppose the change could be hidden from them.\n> > > > > > >\n> > > > > > > I apologise if there have been some discussions flying around that I\n> > > > > > > haven't followed sufficiently. I'm afraid I don't normally study\n> > > > > > > gstreamer related posts in detail, perhaps I need to try harder...\n> > > > > > >\n> > > > > > > Thanks\n> > > > > > > David\n> > > > > > >\n> > > > > > > >\n> > > > > > > > Tested on RPi imx219 with the newly introduced cam option, flip, mirror and\n> > > > > > > > rotation work as expected.\n> > > > > > > >\n> > > > > > > > Tested on Pinephone Pro, where sensors are 90/270 degrees rotated (libcamera\n> > > > > > > > performs no correction)\n> > > > > > > >\n> > > > > > > > Comments welcome of course.\n> > > > > > > >\n> > > > > > > > Thanks\n> > > > > > > > j\n> > > > > > > >\n> > > > > > > > Jacopo Mondi (4):\n> > > > > > > > libcamera: camera_sensor: Cache rotationTransform_\n> > > > > > > > libcamera: camera: Introduce CameraConfiguration::orientation\n> > > > > > > > libcamera: Move to use CameraConfiguration::orientation\n> > > > > > > > apsp: cam: Add option to set stream orientation\n> > > > > > > >\n> > > > > > > > Documentation/Doxyfile.in | 2 +\n> > > > > > > > Documentation/rotation/flip-rotate-0.eps | 170 +++++++++++++++\n> > > > > > > > Documentation/rotation/flip-rotate-0.png | Bin 0 -> 16488 bytes\n> > > > > > > > Documentation/rotation/flip-rotate-180.eps | 191 +++++++++++++++++\n> > > > > > > > Documentation/rotation/flip-rotate-180.png | Bin 0 -> 22198 bytes\n> > > > > > > > Documentation/rotation/flip-rotate-270.eps | 197 ++++++++++++++++++\n> > > > > > > > Documentation/rotation/flip-rotate-270.png | Bin 0 -> 21367 bytes\n> > > > > > > > Documentation/rotation/flip-rotate-90.eps | 195 +++++++++++++++++\n> > > > > > > > Documentation/rotation/flip-rotate-90.png | Bin 0 -> 21020 bytes\n> > > > > > > > Documentation/rotation/rotate-0.eps | 169 +++++++++++++++\n> > > > > > > > Documentation/rotation/rotate-0.png | Bin 0 -> 9086 bytes\n> > > > > > > > Documentation/rotation/rotate-180.eps | 189 +++++++++++++++++\n> > > > > > > > Documentation/rotation/rotate-180.png | Bin 0 -> 22182 bytes\n> > > > > > > > Documentation/rotation/rotate-270.eps | 196 +++++++++++++++++\n> > > > > > > > Documentation/rotation/rotate-270.png | Bin 0 -> 21016 bytes\n> > > > > > > > Documentation/rotation/rotate-90.eps | 196 +++++++++++++++++\n> > > > > > > > Documentation/rotation/rotate-90.png | Bin 0 -> 21528 bytes\n> > > > > > > > include/libcamera/camera.h | 14 +-\n> > > > > > > > include/libcamera/internal/camera_sensor.h | 4 +-\n> > > > > > > > include/libcamera/transform.h | 4 +\n> > > > > > > > src/apps/cam/camera_session.cpp | 17 ++\n> > > > > > > > src/apps/cam/main.cpp | 5 +\n> > > > > > > > src/apps/cam/main.h | 1 +\n> > > > > > > > src/libcamera/camera.cpp | 54 ++++-\n> > > > > > > > src/libcamera/camera_sensor.cpp | 137 ++++++------\n> > > > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +-\n> > > > > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +-\n> > > > > > > > .../pipeline/rpi/common/pipeline_base.cpp | 9 +-\n> > > > > > > > src/libcamera/pipeline/simple/simple.cpp | 6 +-\n> > > > > > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +-\n> > > > > > > > src/libcamera/pipeline/vimc/vimc.cpp | 4 +-\n> > > > > > > > src/libcamera/transform.cpp | 58 ++++++\n> > > > > > > > 32 files changed, 1739 insertions(+), 97 deletions(-)\n> > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-0.eps\n> > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-0.png\n> > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-180.eps\n> > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-180.png\n> > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-270.eps\n> > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-270.png\n> > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-90.eps\n> > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-90.png\n> > > > > > > > create mode 100644 Documentation/rotation/rotate-0.eps\n> > > > > > > > create mode 100644 Documentation/rotation/rotate-0.png\n> > > > > > > > create mode 100644 Documentation/rotation/rotate-180.eps\n> > > > > > > > create mode 100644 Documentation/rotation/rotate-180.png\n> > > > > > > > create mode 100644 Documentation/rotation/rotate-270.eps\n> > > > > > > > create mode 100644 Documentation/rotation/rotate-270.png\n> > > > > > > > create mode 100644 Documentation/rotation/rotate-90.eps\n> > > > > > > > create mode 100644 Documentation/rotation/rotate-90.png\n> > > > > > > >\n> > > > > > > > --\n> > > > > > > > 2.40.1\n> > > > > > > >","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 4E542C3237\n\tfor ;\n\tThu, 22 Jun 2023 08:08:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 82FA1628C0;\n\tThu, 22 Jun 2023 10:08:14 +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 06ACC61E3D\n\tfor ;\n\tThu, 22 Jun 2023 10:08:13 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:5d2e:52c9:72c3:346:a663:c82d])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D3BB8891;\n\tThu, 22 Jun 2023 10:07:36 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1687421294;\n\tbh=unP0vh4DKlX7zVFcwzwQ3OrAPOCGFHs6IpKlPlkdoxk=;\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=jdTy9WVW6Xc+orvHFl+fAXdkP2pp0MsnGJjxHNtE0Emo13L+ar2WYuD3UZpV/OgpN\n\th24QFZfG2i7Tnbmlgh2naBtuzN8opJTbh2uz+hodRNZaCiWt4XcV/eKURDMRqjB1/J\n\t+1/oeyRmqkSo3PvoTJRBxFBE3GuL0/4huv5chqkKwdk+7qdZwStGjwVTcn47+M0ExR\n\toRugiJ3PFhcwU4Jt5mazqJDoI2qLlc0+yxv1aZ8xsci/2eYm+11L+sRzNImO0/BiEA\n\t0/1qRd5SgMAR9CftcZ+irphOnTZHdsXHG8JWa6h79SHGsQ9GWvmSk3LvPS0EEiw3D8\n\tfz6IMAT3NeieQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1687421257;\n\tbh=unP0vh4DKlX7zVFcwzwQ3OrAPOCGFHs6IpKlPlkdoxk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tg1ojPfHtqqCNLYN/58Y2WgV9MKwmDQWO1S9pvzi/+iZ5KtUXnaTUIgTKNe6vcioi\n\t9v88ihOrrOECMHTbw4zYCfSrpCK9Q8sKz2HV/+UgwT2KtJR7UeuoMqAJL42+lb2bpX\n\tQV+s+bfkV12ucbZ2p2qS3h98U+QBRxUgwmlAsCBs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"tg1ojPfH\"; dkim-atps=neutral","Date":"Thu, 22 Jun 2023 10:08:09 +0200","To":"David Plowman ","Message-ID":"<73lhowfjv54atljwwggxb6jfs2rc6cgjyksp6d3ks4bsmw2y6k@m5jwwbwlpugz>","References":"<20230620142904.72600-1-jacopo.mondi@ideasonboard.com>\n\t\n\t\n\t\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC 0/4] libcamera: Replace\n\tCameraConfiguration::transform","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":27413,"web_url":"https://patchwork.libcamera.org/comment/27413/","msgid":"","date":"2023-06-22T12:47:42","subject":"Re: [libcamera-devel] [RFC 0/4] libcamera: Replace\n\tCameraConfiguration::transform","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"HI Jacopo\n\nOn Thu, 22 Jun 2023 at 09:08, Jacopo Mondi\n wrote:\n>\n> Hi David\n>\n> On Wed, Jun 21, 2023 at 05:33:45PM +0100, David Plowman wrote:\n> > Hi again\n> >\n> > On Wed, 21 Jun 2023 at 15:30, Jacopo Mondi\n> > wrote:\n> > >\n> > > Hi David\n> > > let me summarize, as usual the discussion become hard to follow.\n> > >\n> > > On Wed, Jun 21, 2023 at 03:14:28PM +0100, David Plowman wrote:\n> > >\n> > > [snip]\n> > >\n> > > > > >\n> > > > > > I think the last email I sent on the subject was this one, after which\n> > > > > > things went quiet:\n> > > > > > https://lists.libcamera.org/pipermail/libcamera-devel/2023-January/036445.html\n> > > > > > Actually it seems to echo quite a bit of what I say above.\n> > > >\n> > > > I think one of the sticking points for me is that I don't really\n> > > > understand what the problems are. Perhaps we could revisit some of\n> > > > this so that I can be more helpful?\n> > > >\n> > >\n> > > 1) Transform is a libcamera concept, other frameworks have to learn\n> > > how to use it. A specification exists and it's wildely adopted, why\n> > > should we diverge from it ? Because there is code already using the\n> > > current implementation ?\n> >\n> > The orientation flag is completely identical to the transform, just\n> > with different names, so I don't feel this should be an area of\n>\n> Why do you say it is identical ?\n\nSorry, I could have been clearer. The orientation flag is describing\nexactly the same 8 plane transformations as the transform, with\nexactly the same meanings. I believe the only difference is in the\nnaming, the precise encoding in terms of numeric values (hope no one\nrelies on those!), and the availability of extra functions to\n\"manipulate\" them (compose/invert etc.).\n\nI think different categories of users (maybe those who care about\nrendering images, and those who are interested in image analysis, for\nexample) may have slightly different expectations for naming these\nthings, so I definitely think there's some scope for re-examining\nthose.\n\n>\n> The Orientation enumeration is defined based on the EXIF tag 274\n> 'orientation' and describes 2D transforms in terms of clock-wise\n> rotation and horizontal mirroring, while Transform is a custom-defined\n> type which uses Transpose and V/HFlips. Why do you think they're\n> identical ?\n>\n> The EXIF defined values are also used by\n>\n> gstreamer/gtk\n> https://api.gtkd.org/gstreamer.c.types.TAG_IMAGE_ORIENTATION.html\n>\n> pipewire\n> https://docs.pipewire.org/group__spa__buffer.html#gab4afeb1516aa33126757cfa275efd7a3\n>\n> > difficulty. We can change the names, or add synonyms, I don't really\n> > mind. But the transform does have useful compose and inverse functions\n> > which I would not want to lose.\n> >\n> > This code is in quite wide use in the Pi community, so I do generally\n> > want to inconvenience users as little as possible.\n> >\n>\n> I understand this argument, but it can't be used as an excuse to never\n> change the API, in particular if there is a standard already adopted\n> by other frameworks we don't ahere to.\n\nI agree. Generally speaking we try not to change things more than\nnecessary, though there are many occasions where you have to. There\nare always different things to consider and a good balance needs to be\nfound.\n\n>\n> > >\n> > > 2) The library applies partial transforms (like \"I'll do HFlip if I'm\n> > > asked Rot270 and I can't do Transpose\"). This is confusing as\n> > > applications have to compose transforms and do computations. This\n> > > patch simplifies it: either you can do it in full, or leave the stream\n> > > un-touched. There are no clear advantages that I can think of in\n> > > partially applying transforms.\n> >\n> > I like the partial transforms because it means I have only one\n> > \"exception\" case to deal with, and not four. In particular, I can\n> > control what the \"left over\" transform will be, if I want to. Though\n> > of course, I don't have to.\n> >\n>\n> I think what makes a difference here is the behaviour of the consumers\n> of a \"rotated\" image.\n>\n> What is the \"next\" component that in most cases have to compensate for\n> a non-fully corrected rotation ? I presume it generally is the display\n> compositor or a framework like gstreamer that has elements like\n> 'videoflip'. We know gtk/gstreamer internally uses the EXIF tag, but\n> when it comes to compositor, why should for them be better to deal\n> with \"Transpose\" instead that on Rot270 ? I'm no expert there but I\n> presume rendering libraries have methods for quickly rotate images\n> before rendering but they might not have \"transpose only\" ones. Also\n> because \"transpose along the diagonal axis from the top-left corner to\n> the right-bottom one\" is a concept I haven't found documented anywhere\n> else if not in the Transform class (of course I might have missed it\n> as I'm certainly no expert on rendering).\n\nMy aim is to try and give everyone the functionality that they want.\n\nI wouldn't want us to think that rendering is the only use for images.\nWe have many users who want to process images and analyse them in\ndifferent ways. They may be running neural networks, doing filtering\noperations, combining images, all kinds of things. It's actually very\ncommon that they aren't rendering at all, and it's all about the\ndownstream image processing. Rendering is very important, but there\nare other uses too, and it would be great to give them some\nflexibility as well.\n\nEven within rendering, I wonder what would happen if (for example) you\nhad hardware that can do rot90 and rot270, but not transpose. What\nwould you do if an application happened to ask for an orientation that\nleft you needing to do a transpose? It would be nice if you could\neasily calculate the transform you need to ask for so that you're left\nwith a rot90 instead.\n\n>\n> > I think it would be OK to change the default behaviour to what you\n> > have described (i.e. do nothing at all if it can't do the whole\n> > transform), so long as I can still generate the transform that I want,\n> > or my users are able to.\n> >\n> > >\n> > > 3) properties::Rotation, the documentation does not report 'mounting\n> > > rotation' but I admit we (I myself for one) have often used that term.\n> > > That's because, originally, my understanding was that no\n> > > autocorrection should have been performed. Since it has been\n> > > introduced (by copying your PH implementation) and others liked it,\n> > > now properties::Rotation reports the stream rotation when no\n> > > ::transform is specified. I think it's confusing to have\n> > > properties::Rotation=180 and a stream oriented correctly, applications\n> > > might wonder what's happening and if libcamera is doing something\n> > > behind their back.\n> >\n> > I don't mind changing the meaning of the Rotation property, but I\n> > think we should also be fixing up the existing code so that its\n> > behaviour doesn't change. It would also be nice if you could query the\n>\n> What existing code relies on Rotation being the mounting rotation ? In\n> libcamera ? In the apps ?\n\nI would agree that no one \"needs\" the mounting rotation, like they\ndon't \"need\" the pixel cell size. But we normally like to be able to\nreport everything that we know about a sensor, and applications may\nhave a \"list all the info about this sensor\" feature. So it just seems\nlike a helpful thing to do.\n\n>\n> > mounting rotation from an application, though I would agree an\n> > application should never need to use it.\n> >\n> > >\n> > > I think these are the main discussion points, aren't they ?\n> >\n> > I'd like to make a proposal, if I may.\n> >\n> > 1. I'm happy to change the Rotation property, but at the same time we\n> > need to fix the code so that the behaviour of transforms is not\n> > changed. I would still like there to be a property so that an\n> > application can query the mounting rotation if it wants to.\n>\n> I'm fine introducing one\n\nGreat!\n\n>\n> > 2. We change the default behaviour to apply no transform at all if it\n> > can't apply the full transform. (Applications that don't want this can\n> > easily sort themselves out now.)\n\nI think this was the biggest concern, so I'm hoping this addresses it.\n\n> >\n> > At this point I think everyone has the functionality that they want.\n> > Just to try and flesh out the application code a bit so that we are\n> > forced to understand it, we have the following.\n> >\n> > Applications that want the simple behaviour:\n> >\n> > Transform requestedTransform; // the transform the user wants\n> > Transform actualTransform; // what the user will get, from checking\n> > the transform field after validate()\n> >\n> > if (requestedTransform != actualTransform)\n> > /* The application will have to apply a transform equivalent to\n> > -actualTransform * requestedTransform */;\n> >\n> > Applications that want to control the transform that they apply (let's\n> > call this the \"leftoverTransform\"):\n> >\n> > if (requestedTransform != actualTransform)\n> > requestedTransform *= -leftoverTransform; // now just try again\n>\n> I still don't get why people should do any of this calculations.\n\nI hope I've addressed that above. There are other use cases, and it\nwould be great to give everyone the functionality they want.\n\n>\n> I don't expect anyone to write their own rotation libraries, but\n> instead rely on the rendering libraries or on piping gstreamer\n> elements to the capture output. Why should they calculate the\n> \"leftoverTransform\" ? What is the use case for an application in doing\n> this ?\n\nI hope that's covered above. I'd like everyone to have access to\nfunctionality they might want, whilst not impacting those who don't\nwish to use it.\n\n>\n> >\n> > But maybe double-check if that all feels right!!\n>\n> I think it's relevant to know how gstreamer/pipewire expects to\n> deal with transforms, and more importantly what 'transform' display\n> compositors supports natively.\n\nAbsolutely, and I think my proposal covers this, I believe everyone\ngets to do exactly what they want. If there are still particular\nthings that don't work, or are difficult, then we should definitely\nlook into them.\n\n>\n> >\n> > 3. We can consider swapping the transform in the configuration for an\n> > \"orientation\" with a different set of names. We need to add\n> > transform/orientation conversion functions so that I (and my users)\n> > can still use it.\n>\n> See the newly added functions in transform.cpp\n\nThat seems fine. Also very happy to discuss all this at a later date!\n\nThanks!\nDavid\n\n>\n> >\n> > How does that sound?\n> >\n> > David\n> >\n> > >\n> > > > Thanks\n> > > > David\n> > > >\n> > > > > >\n> > > > > > Thanks\n> > > > > > David\n> > > > > >\n> > > > > > >\n> > > > > > > >\n> > > > > > > > >\n> > > > > > > > > This series clearly breaks the application API as it removes a member from\n> > > > > > > > > CameraConfiguration, so it should be introduced probably only when a new release\n> > > > > > > > > is cut.\n> > > > > > > >\n> > > > > > > > I can't comment on this as I haven't really understood the change\n> > > > > > > > here. Obviously we have many end users for whom an API change would be\n> > > > > > > > unwelcome, but I suppose the change could be hidden from them.\n> > > > > > > >\n> > > > > > > > I apologise if there have been some discussions flying around that I\n> > > > > > > > haven't followed sufficiently. I'm afraid I don't normally study\n> > > > > > > > gstreamer related posts in detail, perhaps I need to try harder...\n> > > > > > > >\n> > > > > > > > Thanks\n> > > > > > > > David\n> > > > > > > >\n> > > > > > > > >\n> > > > > > > > > Tested on RPi imx219 with the newly introduced cam option, flip, mirror and\n> > > > > > > > > rotation work as expected.\n> > > > > > > > >\n> > > > > > > > > Tested on Pinephone Pro, where sensors are 90/270 degrees rotated (libcamera\n> > > > > > > > > performs no correction)\n> > > > > > > > >\n> > > > > > > > > Comments welcome of course.\n> > > > > > > > >\n> > > > > > > > > Thanks\n> > > > > > > > > j\n> > > > > > > > >\n> > > > > > > > > Jacopo Mondi (4):\n> > > > > > > > > libcamera: camera_sensor: Cache rotationTransform_\n> > > > > > > > > libcamera: camera: Introduce CameraConfiguration::orientation\n> > > > > > > > > libcamera: Move to use CameraConfiguration::orientation\n> > > > > > > > > apsp: cam: Add option to set stream orientation\n> > > > > > > > >\n> > > > > > > > > Documentation/Doxyfile.in | 2 +\n> > > > > > > > > Documentation/rotation/flip-rotate-0.eps | 170 +++++++++++++++\n> > > > > > > > > Documentation/rotation/flip-rotate-0.png | Bin 0 -> 16488 bytes\n> > > > > > > > > Documentation/rotation/flip-rotate-180.eps | 191 +++++++++++++++++\n> > > > > > > > > Documentation/rotation/flip-rotate-180.png | Bin 0 -> 22198 bytes\n> > > > > > > > > Documentation/rotation/flip-rotate-270.eps | 197 ++++++++++++++++++\n> > > > > > > > > Documentation/rotation/flip-rotate-270.png | Bin 0 -> 21367 bytes\n> > > > > > > > > Documentation/rotation/flip-rotate-90.eps | 195 +++++++++++++++++\n> > > > > > > > > Documentation/rotation/flip-rotate-90.png | Bin 0 -> 21020 bytes\n> > > > > > > > > Documentation/rotation/rotate-0.eps | 169 +++++++++++++++\n> > > > > > > > > Documentation/rotation/rotate-0.png | Bin 0 -> 9086 bytes\n> > > > > > > > > Documentation/rotation/rotate-180.eps | 189 +++++++++++++++++\n> > > > > > > > > Documentation/rotation/rotate-180.png | Bin 0 -> 22182 bytes\n> > > > > > > > > Documentation/rotation/rotate-270.eps | 196 +++++++++++++++++\n> > > > > > > > > Documentation/rotation/rotate-270.png | Bin 0 -> 21016 bytes\n> > > > > > > > > Documentation/rotation/rotate-90.eps | 196 +++++++++++++++++\n> > > > > > > > > Documentation/rotation/rotate-90.png | Bin 0 -> 21528 bytes\n> > > > > > > > > include/libcamera/camera.h | 14 +-\n> > > > > > > > > include/libcamera/internal/camera_sensor.h | 4 +-\n> > > > > > > > > include/libcamera/transform.h | 4 +\n> > > > > > > > > src/apps/cam/camera_session.cpp | 17 ++\n> > > > > > > > > src/apps/cam/main.cpp | 5 +\n> > > > > > > > > src/apps/cam/main.h | 1 +\n> > > > > > > > > src/libcamera/camera.cpp | 54 ++++-\n> > > > > > > > > src/libcamera/camera_sensor.cpp | 137 ++++++------\n> > > > > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +-\n> > > > > > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +-\n> > > > > > > > > .../pipeline/rpi/common/pipeline_base.cpp | 9 +-\n> > > > > > > > > src/libcamera/pipeline/simple/simple.cpp | 6 +-\n> > > > > > > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +-\n> > > > > > > > > src/libcamera/pipeline/vimc/vimc.cpp | 4 +-\n> > > > > > > > > src/libcamera/transform.cpp | 58 ++++++\n> > > > > > > > > 32 files changed, 1739 insertions(+), 97 deletions(-)\n> > > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-0.eps\n> > > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-0.png\n> > > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-180.eps\n> > > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-180.png\n> > > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-270.eps\n> > > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-270.png\n> > > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-90.eps\n> > > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-90.png\n> > > > > > > > > create mode 100644 Documentation/rotation/rotate-0.eps\n> > > > > > > > > create mode 100644 Documentation/rotation/rotate-0.png\n> > > > > > > > > create mode 100644 Documentation/rotation/rotate-180.eps\n> > > > > > > > > create mode 100644 Documentation/rotation/rotate-180.png\n> > > > > > > > > create mode 100644 Documentation/rotation/rotate-270.eps\n> > > > > > > > > create mode 100644 Documentation/rotation/rotate-270.png\n> > > > > > > > > create mode 100644 Documentation/rotation/rotate-90.eps\n> > > > > > > > > create mode 100644 Documentation/rotation/rotate-90.png\n> > > > > > > > >\n> > > > > > > > > --\n> > > > > > > > > 2.40.1\n> > > > > > > > >","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 621D6BD87C\n\tfor ;\n\tThu, 22 Jun 2023 12:47:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DAEEB628C0;\n\tThu, 22 Jun 2023 14:47:56 +0200 (CEST)","from mail-ua1-x92a.google.com (mail-ua1-x92a.google.com\n\t[IPv6:2607:f8b0:4864:20::92a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 201836002B\n\tfor ;\n\tThu, 22 Jun 2023 14:47:55 +0200 (CEST)","by mail-ua1-x92a.google.com with SMTP id\n\ta1e0cc1a2514c-786e637f06dso2659021241.2\n\tfor ;\n\tThu, 22 Jun 2023 05:47:55 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1687438076;\n\tbh=6nAdE3WhlwKm9hGTv2GVvXcKDexRuxlAAaGm53dahwo=;\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=oc21AbLd26Gp+Bf6vCvenBGRPCUNnr5FLqz/DYJGnSms35vpKbKVFmzuHXIQzVg1A\n\ttxbVMAfm7EpPg2H/TgO+Uo4xolryWsTrLnOoPlldinxTvgsodkFFGIGLMCXKP4fFW9\n\tjzkurbO6aZ96DpRqaPjRL8XqKpcYUFYqbOnKgFWOEK3vfYSgft69hFB3T6QFeXNIR7\n\t2s4RRbTi8WOrhWy+A3n2U+szC2kWcp1kO5qldcfeR/jRoVQGjGlWqWNiGMg7OTG8ci\n\tqP8soUdfTTcSEb7ZFVO2CFCrQ//2jVEcUGOw1k4nkBWWVj1wNmwuv7wd1J1/l0igZY\n\tqPB6Vfz1PH50Q==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1687438074; x=1690030074;\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=8R9gzp8jaHIOXp/KuZihI+URSORh3Y2ureuXzJuEqXU=;\n\tb=o39Ji6e25bUFsl62s5wgOf84ma1KFXb9d07FSHB54X4kPraXLrVyRH3g1Uwhzia6Qb\n\twowACKSyRND41K2IdGUkdx5ZpyQ4cuI5kK0NkMUANmYJ2hiBleajDkfx2s0x1qhWQ5gG\n\tmQGVwPLV/Dpaj2NFwswDoL1O13C7AkMtZqsgYxoq8HTTb6d7viQa8zMwGsEDkO+Nt4Ay\n\tVPbme6vA82cc4EN/P+orDo/Tvw3IJQzQzoXn75ne4f+DHibJdvBrWIf/ZG5pfgQ7PWJf\n\tX4Sav+ptzlUp7g4HaaLVJnJiIR5To9/4MdXucXaXlsZCce8B6yvPwdwNXmwlihaAN1P8\n\tN/PA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"o39Ji6e2\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1687438074; x=1690030074;\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=8R9gzp8jaHIOXp/KuZihI+URSORh3Y2ureuXzJuEqXU=;\n\tb=BUW6jVs7PW7eZBhmhR29kllr/gEPQ3M/CHJuGJoV/xt4H32A5+K6UEVoB5TiMJyaY3\n\t5XnW/EBVhiDpAXbWLgXSIybfTuoPMUB7lIhhklXaC44bbnxltdRvK6QqNfJEkhEZzNNk\n\tH8RvMC6n0UkHmWdhRR6y/Wc229+aUnaQoQzB5uMbGJwm6S+Vv9BLXeITm81cfLqvibZc\n\tNHLNfIp9ph78oJMYCSjoYQnuoo+M247BKMYfAfhhvVZDppBGIMXVZ5SsoEzv3OvSve0p\n\tfw1XlrO0073BhOGFf8OmKng4XmZG1ZtH0Hs+r8wY4lzfQ+ltB4U00EbW0g1g6gtXs7lg\n\tS7iQ==","X-Gm-Message-State":"AC+VfDyqNm/jk+5Sdsk35os1bggDkpdq4IpPSStGNU/wuX1nLLorV8KJ\n\tJe+W85ATslSo/cHr8e48Wm64IqfVhSAmkz1XZCd3Gg==","X-Google-Smtp-Source":"ACHHUZ5LaWnE0qsVQ68vjjFz9iS46AVwMzTfOEXJE3ELD2D+e5JYDdpeP58KKYw7RKR/M7PIy4XPI7wrf2No70YX42Q=","X-Received":"by 2002:a05:6102:3674:b0:440:abf3:4c3e with SMTP id\n\tbg20-20020a056102367400b00440abf34c3emr6185912vsb.25.1687438073728;\n\tThu, 22 Jun 2023 05:47:53 -0700 (PDT)","MIME-Version":"1.0","References":"<20230620142904.72600-1-jacopo.mondi@ideasonboard.com>\n\t\n\t\n\t\n\t\n\t\n\t\n\t\n\t<73lhowfjv54atljwwggxb6jfs2rc6cgjyksp6d3ks4bsmw2y6k@m5jwwbwlpugz>","In-Reply-To":"<73lhowfjv54atljwwggxb6jfs2rc6cgjyksp6d3ks4bsmw2y6k@m5jwwbwlpugz>","Date":"Thu, 22 Jun 2023 13:47:42 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC 0/4] libcamera: Replace\n\tCameraConfiguration::transform","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27414,"web_url":"https://patchwork.libcamera.org/comment/27414/","msgid":"<20230622151406.GA950@pendragon.ideasonboard.com>","date":"2023-06-22T15:14:06","subject":"Re: [libcamera-devel] [RFC 0/4] libcamera: Replace\n\tCameraConfiguration::transform","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nI'm replying in the middle of the thread as the next e-mail trimmed part\nof the conversation. I'll reply to the second part just after.\n\nOn Wed, Jun 21, 2023 at 03:14:28PM +0100, David Plowman via libcamera-devel wrote:\n> On Wed, 21 Jun 2023 at 13:57, Jacopo Mondi wrote:\n> > On Wed, Jun 21, 2023 at 01:37:11PM +0100, David Plowman wrote:\n> > > On Wed, 21 Jun 2023 at 10:23, Jacopo Mondi wrote:\n> > > > On Wed, Jun 21, 2023 at 10:08:25AM +0100, David Plowman wrote:\n> > > > > Hi Jacopo\n> > > > >\n> > > > > Thanks for sending this proposal. I wasn't sure I really understood\n> > > > > it, perhaps you can help me out!\n> > > > >\n> > > > > On Tue, 20 Jun 2023 at 15:29, Jacopo Mondi wrote:\n> > > > > >\n> > > > > > Hello everyone\n> > > > > >\n> > > > > > We have been discussing rotation/transformations for a long time, and\n> > > > > > the currently implemented CameraConfiguration::transform behaviour has\n> > > > > > proven to be confusing for several reasons: it was poorly documented,\n> > > > > > translating it to something consumable for upper layer frameworks was\n> > > > > > difficult and it behaved differently than any other field part of the\n> > > > > > CameraConfiguration, as it was meant to tell libcamera \"what to do\" instead\n> > > > > > of expressing what applications want.\n> > > > >\n\nThe last point is slightly debatable. I can't tell if this is right, as\nit depends how one interprets the confusing documentation :-) Maybe this\nis the source of the disagreement. What is absolutely clear is that the\noriginal intent, the documentation and the implementation don't align\nproperly. Whether it is the documentation or the code (or both) that we\nneed to fix is what we're discussing here.\n\nI also agree with Jacopo that consuming the API in upper layers is\ncurrently problematic.\n\n> > > > > As far as I understand it, I don't believe this is correct, and the\n> > > > > transform is where you say what you want.\n> > > > >\n> > > > > For example, on a Raspberry Pi I always set the transform to\n> > > > > Transform::Identity because I want the images \"the right way up\". (I\n> > > > > know that internally, it will calculate and apply a 180 rotation to\n> > > > > counteract the sensor mounting rotation.) Does that sound right?\n\nThe documentation states\n\n * The transform is a user-specified 2D plane transform that will be applied\n * to the camera images by the processing pipeline before being handed to\n * the application. This is subsequent to any transform that is already\n * required to fix up any platform-defined rotation.\n\nI can't interpret the first sentence as anything else than a\ntransformation applied on the image, I don't see how it can be\nconstrued as meaning the desired orientation of the image at the output\nof the pipeline.\n\nOf course, for 180° rotations, the two are equivalent. The fact that 90°\nand 270° support has been implemented in\nCameraSensor::validateTransform() but not tested (due to lack of\nhardware support) only contributes to additional confusion as what the\noriginal intent was.\n\nThe second sentence sounds very confusing to me.\n\n> > > >\n> > > > Yes, now that we correct properties::Rotation to 0 as the library\n> > > > compensate the mounting rotation behind your back, that's what you\n> > > > get.\n> > > >\n> > > > Before we had properties::Rotation = 180; transform = Identity and you\n> > > > got an upright image. Confusing, isn't it ?\n> > >\n> > > I understood the previous arrangement, but the new one seems\n> > > counter-intuitive to me.\n> > >\n> > > On a Raspberry Pi, I expect the properties::Rotation to report 180\n> > > degrees because the sensor is mounted upside down. Then I can ask for\n> > > Transform::Identity and internally a 180 degree rotation will be\n> > > applied to give me what I asked for. I understand this.\n> > >\n> > > Are we saying that now the rotation will report 0 degrees? Will it now\n> > > always report 0 degrees, whatever the sensor and platform? That would\n> > > obviously break things...\n> >\n> > So you're an app, you read properties::Rotation and you get back 180.\n> > Then you star streaming and you get a stream oriented \"correctly\".\n> >\n> > Isn't this wrong ?\n> \n> I think a lot of this hinges on what the Rotation property is.\n> \n> It _used_ to be the mounting rotation of the sensor (is that right?),\n> at which point everything was as I have described it. But I think it\n> is being changed now, which is indeed something that was under\n> discussion in that thread back in January. We're free to do that, but\n> perhaps there have been some unintended consequences?\n\nThis stems from the fact that you wanted (if I recall correctly) the\nplatform to auto-correct the mounting rotation when possible. Exposing\nto the user the fact that the sensor is mounted upside-down, *and*\nauto-correcting this when possible, isn't a behaviour I like. There are\ntoo many implicit assumptions, it's just confusing. If you want\nauto-correction, it needs to be completely transparent, libcamera needs\nto hide the fact that the sensor is mounted upside-down from the\napplication in that case.\n\nI want the API to be clear and simple when no auto-correction is\napplied. I'm OK with auto-correction being added on top if it's totally\ntransparent for applications. This means that auto-correction shouldn't\neven need to be mentioned in the API specification, as applications\nshouldn't have to care.\n\n> > > > > If the problem is actually to do with documentation, I would always\n> > > > > agree that it can be improved, and am very happy to help!\n\nDocumentation surely needs to be improved, regardless of what we decide\nto do with the code.\n\n> > > > > > For these reasons this series proposes to replace the usage to Transform\n> > > > > > in the public API in favour of a new CameraConfiguration::Orientation type,\n> > > > > > defined based on the EXIF specification, tag 274, 'orientation'. For reference\n> > > > > > this is the same as implemented by gstreamer:\n> > > > > > https://gstreamer.freedesktop.org/documentation/gstreamer/gsttaglist.html?gi-language=c#GST_TAG_IMAGE_ORIENTATION\n> > > > >\n> > > > > I must confess to not liking the term \"orientation\". There is such a\n> > > > > thing as a \"2D plane transformation\" and I'd expect to refer to them\n> > > > > as transformations or maybe transforms for short. I don't understand\n> > > > > what an \"orientation\" is without reading extra documentation. Are\n> > > > > \"orientations\" composable? Is there such a thing as an \"inverse\n> > > > > orientation\"?\n\n\"Orientation\" is a term widely used in other frameworks and\nspecifications, including EXIF, and GStreamer. While I don't rule out\nfinding a better name, it doesn't seem bad to me.\n\n\"Transform\" has a strong implication that it specifies what\ntransformation is applied to the image, not what orientation the image\nhas at the output of the pipeline. If you prefer the \"transform\" name,\nthen the meaning of the parameter has to match the name.\n\n> > > > I'm fine bikeshedding on the name. What matters to me is that now we\n> > > > conform to an existing specification adopeted by most frameworks\n> > > > instead of defining our own Transform.\n> > >\n> > > I'd like to find something that works for all libcamera users.\n> > >\n> > > The Transform class allows composition, inverse operations and so on,\n> > > the idea being that users can calculate for themselves what to ask for\n> > > if the default behaviour isn't what they want. I did comment that\n> > > making it easier for folks to get a specific behaviour might be\n> > > something to look into.\n\nHelpers exposed by the libcamera core API need to have a purpose for\nlibcamera. If the only purpose of a Transform class, as used by\napplications, is to compose transformations to calculate how to\nconfigure a compositor based on the orientation of the image produced by\nlibcamera, then that helper doesn't belong to the libcamera core API. I\ndon't mind having it in a conveniency utility library though.\n\nI don't want to digress too much, but some context will be useful to\nunderstand my point here. With the need to support more languages in the\nfuture, as well as the ABI stability requirements, I expect we will\nstrip down the core libcamera API from helpers. The core API will likely\nbe specified using some form of IDL, and the ABI will be plain C. The\nlanguage bindings, including C++ bindings, will be implemented (likely\nauto-generated from the API IDL) based on the C ABI. Utilities that are\nnot part of the core API will be implemented directly in the language\nbindings. For instance, we will retain geometry structures to represent\npoints, sizes and rectangles, but the core API will not include the\ngeometry helpers. The existing geometry helpers will become internal to\nlibcamera (as I expect the libcamera internal implementation to remain\nin C++, and the helpers have shown to be quite useful). A separate\nversion of the helpers will also likely be provided by the C++ bindings\nand other language bindings. If any particular language has native\nclasses to represent the geometry primitives, then we will likely use\nthose.\n\nI don't want to discuss this plan in details as part of this mail\nthread, but let's keep it in mind for the libcamera core API design.\n\n> > > > > > The newly introduced CameraConfiguration::orientation replaces the\n> > > > > > existing CameraConfiguration::tranform, and it is meant for application to\n> > > > > > express how they would like the images to be oriented, not to tell libcamera\n> > > > > > what to do. As an example, passing in 'rotation0' means that the application\n> > > > > > expects the images to be rotated upright, and doesn't tell libcamera not to\n> > > > > > apply any rotation like passing in \"Transform::Identity\" did.\n> > > > >\n> > > > > My understanding is that passing \"Transform::Identity\" has always\n> > > > > meant that the application wants its images \"upright\". (Are there\n> > > > > cases where this doesn't happen?)\n\nMy understanding, as noted above, is that passing \"Transform::Identity\"\nhas always meant that the application wants the images in the\norientation produced by the sensor (or, to be precise, the orientation\nof the pixel array, as the h/v flip are applied in the sensor too). If\nwe want CameraConfiguration::transform to indicate what orientation the\noutput image should have (and it seems we all agree on that), then it\nshould be named differently.\n\n> > > > As we have auto-correction in place, yes.\n> > >\n> > > My understanding is that everything got \"auto-corrected\" before. I'm\n> > > still struggling to understand how it seems to be broken now.\n> >\n> > It is not 'broken'. Simply nobody got how thing worked in practice and\n> > the existing behaviour (when it comes to adjusting ::transform) is not\n> > what most users expects. See below\n> >\n> > > > > > The value CameraConfiguration::orientation is set to after a validate() also\n> > > > > > differs in meaning, as instead of reporting \"what applications have to do\n> > > > > > to obtain what they originally asked for\" it simply reports the actual\n> > > > > > orientation of the stream: this means that if libcamera cannot fully satisfy the\n> > > > > > user request it will set ::orientation to report the native images rotation\n> > > > > > and the CameraConfiguration::status will be set to Adjusted.\n> > > > >\n> > > > > I believe this to be the current behaviour of the transform field.\n> > > >\n> > > > Not really, see below\n> > > >\n> > > > > > Handling of 90 and 270 degrees rotation has also changed: as the camera sensor\n> > > > > > cannot correct rotations that include a transposition, requests for a 90/270\n> > > > > > degrees are ignored, and cameras with a mounting rotation of 90/270 degrees\n> > > > > > are never re-oriented. This makes it clear and less confusing for applications\n> > > > > > that they have to deal with correction fully by themselves. As an example, with\n> > > > > > the current implementation if the application requires a Rot270 (HFlip +\n> > > > > > Transpose) libcamera will do the HFlip and leave transposition to the upper\n> > > > > > layers. There is no clear advantage in doing so, and display compositors are\n> > > > > > better suited for handling transpositions and flipping in a single pass instead\n> > > > > > of having the library try to handle part of that.\n> > > > >\n> > > > > Again, I'm not really understanding this.\n> > > > >\n> > > > > The current behaviour is such that, if it can't give you the\n> > > > > transformation that you ask for (on account of not being able to do\n> > > > > transposition), then it guarantees to give you what you requested\n> > > > > _apart_ from the transposition. So the application only has one case\n> > > > > to deal with - applying a transpose. It doesn't have to worry about\n> > > > > the other 3 \"transforms-that-I-can't-handle\",\n\nThat only holds true if the camera can apply h/v flip. If it can't, the\napplication will need to handle the 90° and 270° rotations, so generic\napplications will always have to support those cases in addition to\ntransposition. It can thus be argued that having to support 90°\nrotation, 270° rotation and transposition is more complicated for the\napplication than having to support rotations only.\n\n> > > > > thereby making life easier for the application.\n> > > >\n> > > > The _apart_ is the key here.\n> > > >\n> > > > If you currently ask for Rot270 the library will do an HFlip and set\n> > > > transform to Transpose (ie what applications still have to do to\n> > > > obtain Rot270). This is not how the other fields of\n> > >\n> > > What it _should_ do is set the transform field to the actual transform\n> > > that the user will perceive the images to have. I believe this to be\n> > > \"proper\" libcamera configuration behaviour. I'm not understanding why\n> > > things are different now.\n> >\n> > Let's start with the fact that \"transpose\" is a concept that is\n> > introduced in Transform and it's not part of the EXIF standard which\n> > gstreamer/pipewire expect to use.\n\nNote that \"rotation\" isn't either. EXIF supports the 8 combinations of\nh/v flip and tranpose, assigning them clear values, but doesn't use the\n\"rotation\", \"transpose\" or \"flip\" names to document them. I'm fine using\nthose names in the libcamera API.\n\n> > Again, you set transform = rot270, libcamera does HFLIP and you have\n> > to do Transpose, correct ?\n> \n> That's one option. Or, if you want to do a \"rot270\" in the application\n> you just set the transform to \"userTransform * -Transform::rot270\".\n> \n> As I've said, I'm really happy to try and make this easier for people.\n> \n> > For compositor this is more work that just do \"rot270\"\n\nDo all display engines that support rotation at the hardware\nlevel be able to apply transposition without also flipping (that is,\npure transposition that isn't a 90° or 270° rotation) ? Do all\ncompositors that support rotation expose a pure transpose transformation\nin their API ? And the other way around, are there hardware devices or\ncompositor APIs that can transpose but can't flip ?\n\nUnless I'm mistaken, there's no clear and universal answer to these\nquestions. I thus don't want to assume what transformations the\nconsumers of the images will be able to apply easily and efficiently.\nI'm missing the reason why libcamera should consider that producing an\nimage that requires rotation by 90° in the application is better (or\nworse) than producing an image that requires transposing. If there's no\noption that is intrinsically better, then I prefer aiming for the\nsimpler and easier to understand behaviour, which is to let the\napplication deal with it.\n\n> > > Beyond this, the current behaviour is to leave the application to do\n> > > the transpose, but that's just a choice. The point of transforms is\n> > > that it's easy to use them to produce something different if you want.\n> > > I don't mind looking at that if we can make it easier for everyone to\n> > > get their preferred behaviour, which I suggested in my original reply.\n> >\n> > I don't get why we should be imposing other frameworks/apps to learn\n> > Transpose when there is a standard to follow to be honest.\n> >\n> > > > CameraConfiguration work, and mostly, applying a flip and letting apps\n> > > > only deal with transpose is an \"optimization\" that might actually make\n> > > > life harder for apps, as display compositors usually have means to\n> > > > deal with known rotations, while in this case you only ask them to do\n> > > > a Transpose, which might require them more work ?\n> > > >\n> > > > In any case, I think it's saner if the library cannot do what it has\n> > > > been asked for to set ::transform to what the stream orientation\n> > > > actually is, instead of telling apps what \"they still have to do\".\n> > >\n> > > I don't understand the repeated statement that the transform is\n> > > telling apps \"what they still have to do\". That was never what it\n> > > meant, unless it got changed somewhere.\n> >\n> > Isn't it ? I'm not repeating the Rot270 example, but applications are\n> > left with ::transform = Transpose. Isn't this what they have to do to\n> > obtain Rot270 once HFlip has been done by libcamera ?\n> >\n> > So they have to \"I asked for Rot270, I got back Transpose. Ah!\n> > libcamera did HFlip for me, so now I have to do Transpose.\"\n> >\n> > Is it what happens ?\n> \n> The code that I am currently running returns HFlip as the transform\n> when I ask for Rot270, which is indeed what the application will get.\n> \n> In this case yes, I still have to do a transpose. But if I had, for\n> example, code that does a 90 degree rotation, I'd ask for \"Rot270 *\n> -Rot90\" instead. The transform field would come back as HVFlip telling\n> me that my images will be upside down, and all I have to do is call my\n> rotate 90 function.\n> \n> The idea is that it should be simple to get whatever behaviour you\n> want, but I'm very open to trying to make this easier.\n> \n> > > > > Because transforms are composable, it's easy to calculate what\n> > > > > transform to ask for to get a different behaviour (for example, do\n> > > > > nothing at all if there's a transpose involved). I could imagine a\n> > > > > slightly different API might be helpful, perhaps where you get to say\n> > > > > not only \"what you want\", but also what you want the \"residual\n> > > > > transform\" to be (being the one the application has to apply after) if\n> > > > > it can't deliver what you wanted. Would that be useful?\n\nThat's too complex I think. If we want to extend the API (not merely\nchange it), we could report the transformation capabilities of the\ncamera as a property to let applications discover what can be done. It\nwill then be the responsibility of the application to ask for something\nthat is supported, and we won't have to design complex heuristics to\nfind the \"best\" option when the applications asks for an unsupported\ntransformation. The API will be simpler to document, and simpler to\nunderstand.\n\n> > > > I think it's better to leave the stream orientation unmodified if it\n> > > > cannot be corrected. This matches the behaviour of the other\n> > > > CameraConfiguration fields, and seems easier to deal with from\n> > > > applications.\n> > >\n> > > I feel we ought to be able to come up with something that makes it\n> > > convenient for everyone to get what they want.\n> > >\n> > > > > I also recall that there was some discussion a while back about the\n> > > > > precise orientation of the rotations which could affect some of the\n> > > > > details, but I don't think it was ever concluded. I don't know if\n> > > > > that's causing any problems?\n> > > >\n> > > > Sorry, I don't remember such discussion. Any link ?\n> > >\n> > > I think the last email I sent on the subject was this one, after which\n> > > things went quiet:\n> > > https://lists.libcamera.org/pipermail/libcamera-devel/2023-January/036445.html\n> > > Actually it seems to echo quite a bit of what I say above.\n> \n> I think one of the sticking points for me is that I don't really\n> understand what the problems are. Perhaps we could revisit some of\n> this so that I can be more helpful?\n> \n> > > > > > This series clearly breaks the application API as it removes a member from\n> > > > > > CameraConfiguration, so it should be introduced probably only when a new release\n> > > > > > is cut.\n\nThe series can be merged when ready, whenever that will be. There's no\nneed to tie it to a \"release\".\n\n> > > > > I can't comment on this as I haven't really understood the change\n> > > > > here. Obviously we have many end users for whom an API change would be\n> > > > > unwelcome, but I suppose the change could be hidden from them.\n\nI'm afraid we will have many API changes, much more invasive than this\none, in the future before we stabilize the API.\n\n> > > > > I apologise if there have been some discussions flying around that I\n> > > > > haven't followed sufficiently. I'm afraid I don't normally study\n> > > > > gstreamer related posts in detail, perhaps I need to try harder...\n> > > > >\n> > > > > > Tested on RPi imx219 with the newly introduced cam option, flip, mirror and\n> > > > > > rotation work as expected.\n> > > > > >\n> > > > > > Tested on Pinephone Pro, where sensors are 90/270 degrees rotated (libcamera\n> > > > > > performs no correction)\n> > > > > >\n> > > > > > Comments welcome of course.\n> > > > > >\n> > > > > > Thanks\n> > > > > > j\n> > > > > >\n> > > > > > Jacopo Mondi (4):\n> > > > > > libcamera: camera_sensor: Cache rotationTransform_\n> > > > > > libcamera: camera: Introduce CameraConfiguration::orientation\n> > > > > > libcamera: Move to use CameraConfiguration::orientation\n> > > > > > apsp: cam: Add option to set stream orientation\n> > > > > >\n> > > > > > Documentation/Doxyfile.in | 2 +\n> > > > > > Documentation/rotation/flip-rotate-0.eps | 170 +++++++++++++++\n> > > > > > Documentation/rotation/flip-rotate-0.png | Bin 0 -> 16488 bytes\n> > > > > > Documentation/rotation/flip-rotate-180.eps | 191 +++++++++++++++++\n> > > > > > Documentation/rotation/flip-rotate-180.png | Bin 0 -> 22198 bytes\n> > > > > > Documentation/rotation/flip-rotate-270.eps | 197 ++++++++++++++++++\n> > > > > > Documentation/rotation/flip-rotate-270.png | Bin 0 -> 21367 bytes\n> > > > > > Documentation/rotation/flip-rotate-90.eps | 195 +++++++++++++++++\n> > > > > > Documentation/rotation/flip-rotate-90.png | Bin 0 -> 21020 bytes\n> > > > > > Documentation/rotation/rotate-0.eps | 169 +++++++++++++++\n> > > > > > Documentation/rotation/rotate-0.png | Bin 0 -> 9086 bytes\n> > > > > > Documentation/rotation/rotate-180.eps | 189 +++++++++++++++++\n> > > > > > Documentation/rotation/rotate-180.png | Bin 0 -> 22182 bytes\n> > > > > > Documentation/rotation/rotate-270.eps | 196 +++++++++++++++++\n> > > > > > Documentation/rotation/rotate-270.png | Bin 0 -> 21016 bytes\n> > > > > > Documentation/rotation/rotate-90.eps | 196 +++++++++++++++++\n> > > > > > Documentation/rotation/rotate-90.png | Bin 0 -> 21528 bytes\n> > > > > > include/libcamera/camera.h | 14 +-\n> > > > > > include/libcamera/internal/camera_sensor.h | 4 +-\n> > > > > > include/libcamera/transform.h | 4 +\n> > > > > > src/apps/cam/camera_session.cpp | 17 ++\n> > > > > > src/apps/cam/main.cpp | 5 +\n> > > > > > src/apps/cam/main.h | 1 +\n> > > > > > src/libcamera/camera.cpp | 54 ++++-\n> > > > > > src/libcamera/camera_sensor.cpp | 137 ++++++------\n> > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +-\n> > > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +-\n> > > > > > .../pipeline/rpi/common/pipeline_base.cpp | 9 +-\n> > > > > > src/libcamera/pipeline/simple/simple.cpp | 6 +-\n> > > > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +-\n> > > > > > src/libcamera/pipeline/vimc/vimc.cpp | 4 +-\n> > > > > > src/libcamera/transform.cpp | 58 ++++++\n> > > > > > 32 files changed, 1739 insertions(+), 97 deletions(-)\n> > > > > > create mode 100644 Documentation/rotation/flip-rotate-0.eps\n> > > > > > create mode 100644 Documentation/rotation/flip-rotate-0.png\n> > > > > > create mode 100644 Documentation/rotation/flip-rotate-180.eps\n> > > > > > create mode 100644 Documentation/rotation/flip-rotate-180.png\n> > > > > > create mode 100644 Documentation/rotation/flip-rotate-270.eps\n> > > > > > create mode 100644 Documentation/rotation/flip-rotate-270.png\n> > > > > > create mode 100644 Documentation/rotation/flip-rotate-90.eps\n> > > > > > create mode 100644 Documentation/rotation/flip-rotate-90.png\n> > > > > > create mode 100644 Documentation/rotation/rotate-0.eps\n> > > > > > create mode 100644 Documentation/rotation/rotate-0.png\n> > > > > > create mode 100644 Documentation/rotation/rotate-180.eps\n> > > > > > create mode 100644 Documentation/rotation/rotate-180.png\n> > > > > > create mode 100644 Documentation/rotation/rotate-270.eps\n> > > > > > create mode 100644 Documentation/rotation/rotate-270.png\n> > > > > > create mode 100644 Documentation/rotation/rotate-90.eps\n> > > > > > create mode 100644 Documentation/rotation/rotate-90.png","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 ED64BBD87C\n\tfor ;\n\tThu, 22 Jun 2023 15:14:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 64EA5628C0;\n\tThu, 22 Jun 2023 17:14:09 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0D43B6002B\n\tfor ;\n\tThu, 22 Jun 2023 17:14:08 +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 A2B92905;\n\tThu, 22 Jun 2023 17:13:31 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1687446849;\n\tbh=kqtbFqxjN+uA9ZQDywqD2UsvUBJkN2C/pUv5eBh1moE=;\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=FYbGgR2UkecCaaf2r/vMQ784YVfh2rEfnFYpPU7QQrDukyY9PaiX0H/sYu09pX0qC\n\tft+V5dS3UFm0t74K9uR3GlxRAafvaoAmTuF46KtzbmuyVdd5sH6LtUaeSB6WWoohDK\n\t4wNxL5rjRSoxtpInqquDdw90weeSQDm2xYbCzunk6Li6W38TsQnBnwpSP/q3veoF8I\n\tjP7PZypn8YcODA4wdK4OlXiMLppX5O/sfU4qEl4ybSeUJDorhmMDOCRk73d4B2Nsob\n\tzk0P6hWlu5tD3RYPBvBNtduZ286RyoRWzmVbLyoh3dTTMN/2jTx078+DLqlRqHcZ3w\n\tiWp2x+haJ/mLQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1687446811;\n\tbh=kqtbFqxjN+uA9ZQDywqD2UsvUBJkN2C/pUv5eBh1moE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SZxjqfZV2vuHOal2BBaQile2/fL6QHiqsfa/B39/6DCC+o7cD7QKuzUSXjVVuhgL3\n\tKdcSqcCazckRCNbeAErqAFbQizaH78WrbjupArXQC4aLS71Qxnyx7YYp7ufQe+Ytom\n\tbRYeju1qA7xIh/WpOKg3BcsNteKzg6XIbQOoK0YY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"SZxjqfZV\"; dkim-atps=neutral","Date":"Thu, 22 Jun 2023 18:14:06 +0300","To":"David Plowman ","Message-ID":"<20230622151406.GA950@pendragon.ideasonboard.com>","References":"<20230620142904.72600-1-jacopo.mondi@ideasonboard.com>\n\t\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC 0/4] libcamera: Replace\n\tCameraConfiguration::transform","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27416,"web_url":"https://patchwork.libcamera.org/comment/27416/","msgid":"","date":"2023-06-23T08:26:21","subject":"Re: [libcamera-devel] [RFC 0/4] libcamera: Replace\n\tCameraConfiguration::transform","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi everyone\n\nThanks for that message. I think it's very helpful to quote the source\ndocumentation:\n\n * The transform is a user-specified 2D plane transform that will be applied\n * to the camera images by the processing pipeline before being handed to\n * the application. This is subsequent to any transform that is already\n * required to fix up any platform-defined rotation.\n\nWhat this is trying to say, badly, is that the transform you request\nis applied after (\"subsequent to\") fixing up the platform rotation. So\nyou do indeed get auto-correction. This is also the actual behaviour\nof the code on the Pi, has been for, I don't know, nearly 2 years? At\nthis point, the earth does indeed get my official permission to\nswallow me up for not writing something that was less prone to\nmisinterpretation,\n\nBut I think this does leave us in a position where large amounts of\nthis thread are a mixture of not very helpful or even plain confusing,\nand with people's permission, I'd honestly like to start again, with\neveryone on the same page.\n\nYou might have noticed that \"orientations\" were really bugging me as\nto what they were, and how one could use them. I've managed to resolve\nthese problems in my own mind and think that there's a very simple\nconnection between \"orientations\" and \"transforms\" that I would be OK\nto implement and use. I think our next attempt at this discussion\nshould start by being clear on our requirements, and then we can look\nat how \"orientations\" and \"transforms\" can get us there.\n\n(Rest of thread purposefully deleted... hope that's ok!)\n\nThanks!\nDavid","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 88696BD87C\n\tfor ;\n\tFri, 23 Jun 2023 08:26:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9C55C628C0;\n\tFri, 23 Jun 2023 10:26:35 +0200 (CEST)","from mail-qv1-xf33.google.com (mail-qv1-xf33.google.com\n\t[IPv6:2607:f8b0:4864:20::f33])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 81D4261E3F\n\tfor ;\n\tFri, 23 Jun 2023 10:26:33 +0200 (CEST)","by mail-qv1-xf33.google.com with SMTP id\n\t6a1803df08f44-62ff1cdf079so3909036d6.0\n\tfor ;\n\tFri, 23 Jun 2023 01:26:33 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1687508795;\n\tbh=bOukoXE1a3MKb6Y58+4hsLYQc11EsrhB3jKMapmgm2g=;\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=dl2J3RTisBPCHILLequkwuGVpsECS6VqwxJ4SILc7AsiB0bgPUKzTkvP2EKmdssJw\n\tdJ7Z/79ZhOMjoxF4Q2W0r3HNvZcfyvbNthItiHWV1lbB5l3lYaDOFEfKZEWBqw2+dP\n\t8T+GAMzxvXwBSrQvVJLnLR5HpE68Jkw9xMruWcAYU8Hx80EnssbvmXEKpVMdfKWCrO\n\tCP6pITAJYYkdxuAQnjVNbBIsQyJIg2yJ+MuczcmfitQlNhH/mGzrVnpBfCgwJ3/J7w\n\tSX3J3w9JSWpGE5wLNydABQBC8CuGyRd0Dw8e2ZdhjfXUSJVNisJiORPAz3cWxe9vY2\n\tNyMt6KZBOzkJg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1687508792; x=1690100792;\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=bOukoXE1a3MKb6Y58+4hsLYQc11EsrhB3jKMapmgm2g=;\n\tb=oM37o0rfrPl4OrMxy5TOIJFFLEKI1jDyBF4HmF2H4HV2iCdOQq69rOaZlYodiPABU4\n\tbyugZpZV3H0dGlBSf+XlF40QMEmPckhRl06TR0BQ+t80u8WLehks8j2yRyUhxNTBvJnS\n\tNpW5auhpJRzSLNn6PtSEH29yNwvi4cFAftQda6WQJMnWgugVbAGJrDlNW7gogiPAFCM2\n\tixs2Kw++j28efvtejP/2NFEtyNubIkANi9EeE/cbA7jBr1yE7O5CVOW9Clo83VSjDV8r\n\to79aHxGBwxg97QM9EzA6v4AXb10zZ8cMUqT6BkEvmqbZYkdaMp3OZfiKLfzlXP83rC6j\n\tFjIQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"oM37o0rf\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1687508792; x=1690100792;\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=bOukoXE1a3MKb6Y58+4hsLYQc11EsrhB3jKMapmgm2g=;\n\tb=UQXReRrkYovRrr+EI2Y+4lHfmAYGwNf6GP4AQ9jcWsLcQWfP3ZRlRDTSKQ0GFHtXCZ\n\tjjGv2ByE8ZQwniuX15sNGgT6WP1cMrprbpDR2qO8HiDkAx3nlLDeqnbpPoHh31jb3+Qv\n\td1opO34Dmn101k5k8piaVUneHrIXHijEDg57pQDH13vMhLe8211CEvbSB8kM59QDD3VC\n\tuMm1cwaG7WFLmwXOIuamnvHpKJKbwLMUxuqev7WVS6woY4ej2YmciAdDkE+uRgMtqiBF\n\terNc1JBMM3uAwCSlpykMJDw7xjJfO6UTWSFFlFo50NKiQ6MObZVD0+nHeZ0iiHBlPNuu\n\teNvw==","X-Gm-Message-State":"AC+VfDyloGhjq2F+3tkF+XNO5Rfs2Uhu+jN3K+K8nggh2opzeMbHXtiX\n\tIYERiP7T9mbikql2yQ+mmjdwgDXlrHJfp9LXPsyZMg==","X-Google-Smtp-Source":"ACHHUZ6HW66q84nZWHhKRqet0qZQJlYbHjDa3WIkeyAPngrp/ZvIwor1wu7vW4TDbrN2WIY3MEv2JL2W5/Wqn2bXJKQ=","X-Received":"by 2002:a05:6214:2623:b0:631:f059:691f with SMTP id\n\tgv3-20020a056214262300b00631f059691fmr16808662qvb.3.1687508792181;\n\tFri, 23 Jun 2023 01:26:32 -0700 (PDT)","MIME-Version":"1.0","References":"<20230620142904.72600-1-jacopo.mondi@ideasonboard.com>\n\t\n\t\n\t\n\t\n\t\n\t<20230622151406.GA950@pendragon.ideasonboard.com>","In-Reply-To":"<20230622151406.GA950@pendragon.ideasonboard.com>","Date":"Fri, 23 Jun 2023 09:26:21 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC 0/4] libcamera: Replace\n\tCameraConfiguration::transform","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27417,"web_url":"https://patchwork.libcamera.org/comment/27417/","msgid":"<3090b510-b3e9-e9fe-379e-278bb661f4d9@collabora.com>","date":"2023-06-23T11:04:57","subject":"Re: [libcamera-devel] [RFC 0/4] libcamera: Replace\n\tCameraConfiguration::transform","submitter":{"id":140,"url":"https://patchwork.libcamera.org/api/people/140/","name":"Robert Mader","email":"robert.mader@collabora.com"},"content":"Hi David, thanks for initiative to bring us all on the same page.\n\n From what I understand, this series aims to do two things:\n\n\n1.: Make the public API follow the EXIF specification\n\nI personally, having implemented the existing API in Pipewire and, still \npending, the Gstreamer element, am rather neutral on this, as the new \nand old values have a bijective relationship. All I care about that it's \neasy for apps to transition and support both APIs for at least a short \nwhile.\n\n\n2.: Clarifying the expected behavior and fixing existing inconsistencies.\n\nThe main issue with the existing implementation I personally have been \nrunning into, that is fixed by this series, was the returned Transform \nvalue from `validate()`.\n\nTo give an example, consider the following two cases:\n\n * A camera mounted 90 deg rotated, a config requesting\n `Transform::Identity`\n * No camera rotation, a config requesting `Transform::rot90`\n\nIn the first case (typical for Linux phones like the Pinephone (Pro) and \nLibrem5) the returned value will be `Transform::rot270` (IIRC) and the \nuser can use that to adapt the output downstream.\n\nIn the second case, the returned value will be `Transform::Identity` and \nthe user has to remember that it asked for something different.\n\nCombining a non-identity mounting rotation and a non-identity requested \ntransform thus essentially leads to random results and makes a clean \nimplementation almost impossible.\n\nI personally like the proposed behavior - either you get the requested \nresult or you have to apply the combined transform yourself - a lot.\n\n From what I understand there are only two open questions here:\n\n 1. Should the sensor do partial transforms?\n 2. Do we really want/need the new `Orientation` enum or could/should we\n instead with `Transform` and just update/clarify the expected results?\n\n\nHope you agree that these are the issues to agree on.\n\nThanks everyone and cheers,\n\nRobert\n\nOn 23.06.23 10:26, David Plowman via libcamera-devel wrote:\n> Hi everyone\n>\n> Thanks for that message. I think it's very helpful to quote the source\n> documentation:\n>\n> * The transform is a user-specified 2D plane transform that will be applied\n> * to the camera images by the processing pipeline before being handed to\n> * the application. This is subsequent to any transform that is already\n> * required to fix up any platform-defined rotation.\n>\n> What this is trying to say, badly, is that the transform you request\n> is applied after (\"subsequent to\") fixing up the platform rotation. So\n> you do indeed get auto-correction. This is also the actual behaviour\n> of the code on the Pi, has been for, I don't know, nearly 2 years? At\n> this point, the earth does indeed get my official permission to\n> swallow me up for not writing something that was less prone to\n> misinterpretation,\n>\n> But I think this does leave us in a position where large amounts of\n> this thread are a mixture of not very helpful or even plain confusing,\n> and with people's permission, I'd honestly like to start again, with\n> everyone on the same page.\n>\n> You might have noticed that \"orientations\" were really bugging me as\n> to what they were, and how one could use them. I've managed to resolve\n> these problems in my own mind and think that there's a very simple\n> connection between \"orientations\" and \"transforms\" that I would be OK\n> to implement and use. I think our next attempt at this discussion\n> should start by being clear on our requirements, and then we can look\n> at how \"orientations\" and \"transforms\" can get us there.\n>\n> (Rest of thread purposefully deleted... hope that's ok!)\n>\n> Thanks!\n> David","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 5E6D5C3237\n\tfor ;\n\tFri, 23 Jun 2023 11:05:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B73FD61E3C;\n\tFri, 23 Jun 2023 13:05:02 +0200 (CEST)","from madras.collabora.co.uk (madras.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e5ab])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C99ED61E3C\n\tfor ;\n\tFri, 23 Jun 2023 13:05:00 +0200 (CEST)","from [IPV6:2001:4091:a245:801c:945b:3615:8447:c08d] (unknown\n\t[IPv6:2001:4091:a245:801c:945b:3615:8447:c08d])\n\t(using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)\n\tkey-exchange X25519 server-signature RSA-PSS (4096 bits)\n\tserver-digest SHA256)\n\t(No client certificate requested) (Authenticated sender: rmader)\n\tby madras.collabora.co.uk (Postfix) with ESMTPSA id 4734B6607135\n\tfor ;\n\tFri, 23 Jun 2023 12:05:00 +0100 (BST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1687518302;\n\tbh=acWe1EjluOFMGy5xK7hFT1KqA7aRLkF6aZ925cA9ZFs=;\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:\n\tFrom;\n\tb=2NiGuIiWxI6aHeg31JbuDZVw1gFDjXnFjMeafPmkM7VSJXEBb+f2iLKyzVPwNQ1Ws\n\tk+5th6dBZlaWcKEKY0LfIeWK6Fga4Zv1E8sIX+9EmsLdEBTgLvQrf2T7u1o0QlupGK\n\tjdeyVG7afd34GdcRjzoic0OMdxjI3Dh3HQvF1esMHkzxb/NS8rka3g5S4ijvTSfF8g\n\t8cVvXr9DuAUfxHBmH4UFbJCg5aWeKYrYmkdcayZ+0Spb0q/1SXSfFZU7sdYo1GlY4I\n\t7KzznP6n6rgwip6zv/mjP50pZPg/SQg8aFemGtGQAy/+td6AahxxrbOR1dmnffiC1G\n\tpdIDjk3v4CcSw==","v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com;\n\ts=mail; t=1687518300;\n\tbh=acWe1EjluOFMGy5xK7hFT1KqA7aRLkF6aZ925cA9ZFs=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=i3xcUlfRUTnqG3128xUAfjo1BHQeXEZbmG6gmumwpdTBRDqBfUO8G4/ilxY3oeBDb\n\tPG1PT+jyl2AyNhwR+9VxvnsOx6Fjcaukwdeh49e00hR4dO8lxSd9s8PP4/0P/s2yyu\n\truhAGFL+0R1lPaa00NWDxm5O6KRXRGzHAGiSqpvsHu3Yh+UzGvyT9r/0Sfl6WmX6jh\n\t0vWFN0r3eAepblFjGu8XPd/vFR/Ysluq3xSiNzmwUuF94RGrZwDjHKTgnrZMgdaXks\n\tCiwZSVrgk6+kkgdO17NPk9DeyvecjYylgeZzkOayt2HQb2Kj7Snxa061CTyNam12b/\n\tYLGo1SK2HD2gQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=collabora.com\n\theader.i=@collabora.com\n\theader.b=\"i3xcUlfR\"; dkim-atps=neutral","Content-Type":"multipart/alternative;\n\tboundary=\"------------HrVPFG8gJvfYLL0WQnkXi2so\"","Message-ID":"<3090b510-b3e9-e9fe-379e-278bb661f4d9@collabora.com>","Date":"Fri, 23 Jun 2023 13:04:57 +0200","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.12.0","To":"libcamera-devel@lists.libcamera.org","References":"<20230620142904.72600-1-jacopo.mondi@ideasonboard.com>\n\t\n\t\n\t\n\t\n\t\n\t<20230622151406.GA950@pendragon.ideasonboard.com>\n\t","Content-Language":"en-US, de-DE","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC 0/4] libcamera: Replace\n\tCameraConfiguration::transform","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":"Robert Mader via libcamera-devel ","Reply-To":"Robert Mader ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27421,"web_url":"https://patchwork.libcamera.org/comment/27421/","msgid":"","date":"2023-06-26T08:59:12","subject":"Re: [libcamera-devel] [RFC 0/4] libcamera: Replace\n\tCameraConfiguration::transform","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Robert, everyone\n\nThanks for the message. I think that's helpful and makes sense to me.\nIt seems to me that a large part of this is to clarify what's meant to\nhappen, to resolve inconsistencies (and bugs!), and to make sure that\neveryone gets what they need. To try and summarise my own\nrequirements:\n\n1. I want any transform to be auto-corrected, that is, for any\nmounting rotation to be corrected transparently for me. However, I\nstill think we should report what the mounting rotation is.\n\n2. When it's possible to produce the requested transform, then\nobviously I want it to do that!\n\n3. When it's not possible to deliver the requested transform, then I\ndon't really mind what we choose to do, contingent on having\nrequirement 4. For example, I would be fine with libcamera doing\nnothing at all, so that the \"actual\" transform returned would be the\nsensor mounting rotation (does everyone agree that this is what would\nhappen?).\n\n4. I want to be able to use the libcamera classes to compare what I\nrequested versus what I got, so that I can figure out what I need to\ndo. This should have absolutely no impact on the libcamera API (which\nis principally a question of what happens to the transform during\nvalidate()). Using the libcamera classes to work out what to do is\nentirely optional.\n\nA few final observations:\n\n* I'm fine with changing the meaning of the \"rotation\" property.\nThough we need to ensure this doesn't change the existing behaviour\n(on the Pi, can't speak for certain about anything else!), and as\nnoted above, I think we should report the mounting rotation in another\nway.\n\n* I'm fine with introducing an Orientation if that's helpful to\npeople. If we put the orientation into the camera configuration\n(replacing the transform) then I want the orientation class to\ninteract correctly with the transform class so that we still have\nrequirement 4. In this case the orientation class also needs to be\nfully implemented in the Python bindings, as transforms are.\n\n* I still have this niggle in my mind that the sense of the rotations\nwas not settled in our previous discussions. We need to resolve this.\n\nThat's everything I can think of at the minute. Does it seem reasonable?\n\nThanks!\nDavid\n\nOn Fri, 23 Jun 2023 at 12:05, Robert Mader via libcamera-devel\n wrote:\n>\n> Hi David, thanks for initiative to bring us all on the same page.\n>\n> From what I understand, this series aims to do two things:\n>\n>\n> 1.: Make the public API follow the EXIF specification\n>\n> I personally, having implemented the existing API in Pipewire and, still pending, the Gstreamer element, am rather neutral on this, as the new and old values have a bijective relationship. All I care about that it's easy for apps to transition and support both APIs for at least a short while.\n>\n>\n> 2.: Clarifying the expected behavior and fixing existing inconsistencies.\n>\n> The main issue with the existing implementation I personally have been running into, that is fixed by this series, was the returned Transform value from `validate()`.\n>\n> To give an example, consider the following two cases:\n>\n> A camera mounted 90 deg rotated, a config requesting `Transform::Identity`\n> No camera rotation, a config requesting `Transform::rot90`\n>\n> In the first case (typical for Linux phones like the Pinephone (Pro) and Librem5) the returned value will be `Transform::rot270` (IIRC) and the user can use that to adapt the output downstream.\n>\n> In the second case, the returned value will be `Transform::Identity` and the user has to remember that it asked for something different.\n>\n> Combining a non-identity mounting rotation and a non-identity requested transform thus essentially leads to random results and makes a clean implementation almost impossible.\n>\n> I personally like the proposed behavior - either you get the requested result or you have to apply the combined transform yourself - a lot.\n>\n> From what I understand there are only two open questions here:\n>\n> Should the sensor do partial transforms?\n> Do we really want/need the new `Orientation` enum or could/should we instead with `Transform` and just update/clarify the expected results?\n>\n>\n> Hope you agree that these are the issues to agree on.\n>\n> Thanks everyone and cheers,\n>\n> Robert\n>\n> On 23.06.23 10:26, David Plowman via libcamera-devel wrote:\n>\n> Hi everyone\n>\n> Thanks for that message. I think it's very helpful to quote the source\n> documentation:\n>\n> * The transform is a user-specified 2D plane transform that will be applied\n> * to the camera images by the processing pipeline before being handed to\n> * the application. This is subsequent to any transform that is already\n> * required to fix up any platform-defined rotation.\n>\n> What this is trying to say, badly, is that the transform you request\n> is applied after (\"subsequent to\") fixing up the platform rotation. So\n> you do indeed get auto-correction. This is also the actual behaviour\n> of the code on the Pi, has been for, I don't know, nearly 2 years? At\n> this point, the earth does indeed get my official permission to\n> swallow me up for not writing something that was less prone to\n> misinterpretation,\n>\n> But I think this does leave us in a position where large amounts of\n> this thread are a mixture of not very helpful or even plain confusing,\n> and with people's permission, I'd honestly like to start again, with\n> everyone on the same page.\n>\n> You might have noticed that \"orientations\" were really bugging me as\n> to what they were, and how one could use them. I've managed to resolve\n> these problems in my own mind and think that there's a very simple\n> connection between \"orientations\" and \"transforms\" that I would be OK\n> to implement and use. I think our next attempt at this discussion\n> should start by being clear on our requirements, and then we can look\n> at how \"orientations\" and \"transforms\" can get us there.\n>\n> (Rest of thread purposefully deleted... hope that's ok!)\n>\n> Thanks!\n> David","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 1763AC3237\n\tfor ;\n\tMon, 26 Jun 2023 08:59:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7D792628BE;\n\tMon, 26 Jun 2023 10:59:26 +0200 (CEST)","from mail-qv1-xf31.google.com (mail-qv1-xf31.google.com\n\t[IPv6:2607:f8b0:4864:20::f31])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CA0A061E3B\n\tfor ;\n\tMon, 26 Jun 2023 10:59:24 +0200 (CEST)","by mail-qv1-xf31.google.com with SMTP id\n\t6a1803df08f44-6300afaa43bso24746806d6.3\n\tfor ;\n\tMon, 26 Jun 2023 01:59:24 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1687769966;\n\tbh=NtWhq32ZCHiYeHGI9qmDicsZjlb/eKXPUBMSzOOR45Q=;\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=jyP3PZwgNoikdSxZ1ZEsrzL0/zBxnwX9Z7sIypFg1SZ3fN62GeyLLAWmGJJ5Kyzl8\n\tz1u/7Q296zxwwylltB6SjSeg4P1AJByev9T4hbJX24mfWrqDLkDt207oosv0oyrrXt\n\tg7lybbiuyqRgTOhkM+lPvcziFTxKBOGZyD4uVNxGnR3LB5/xX1p7Iz76GnMbKg9I5g\n\tECjVFp10k8f0RC5YpTZg1bBYCmZ98vhZzMPOJNB0tZTW4WeTIHqMDLqdICyG3p+DSI\n\tRu+VRLW0AGrvyIVr0WkQVSCopNmiiYP6Bm4RCDZp2cWshQm0/zkfL82qi6tBK4Y5AH\n\tDN5D1PZeMoepA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1687769963; x=1690361963;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=bKosjt9SYfcPNQYFKSe/hOY9ol5CRva19OngAwHg7jI=;\n\tb=P7M9fFBVdnKwz5+UoBTimWiWQEXti467dDXhKwfejeX1OiZMJp6B1yQjaI+O5y0P+U\n\tPXtocQ2vnTknx91F05yDBIEZM+l4VEhI10WTHc2/VOVgWmisbEmenOxh36s5nBrGdaSp\n\t1hFG8Z3jK/MtZLQgGIKuI2+4AHQSV0GSaKEqrI/WyvtJd83Tg5zPudZ5Ge118YbWHqLH\n\toXP41WOFJMPcterR8DGsUzcp6sTDXhD0YjF3BOrQrK61mv5wXXmysDdjJzniCtpBdAwb\n\txYZuqTQW9d1IcquJEreFnvAICPX2j4Sv9hiAfTMg6lfxSjVCaH1mWaUxw8V8u4K4t4jD\n\tXWlw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"P7M9fFBV\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1687769963; x=1690361963;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=bKosjt9SYfcPNQYFKSe/hOY9ol5CRva19OngAwHg7jI=;\n\tb=SvKIOFbFD+Z97RWfK7aYqSj3Orkx5cBXTzsv4G4zuaCbID7FQxClXnKIGusNwPD2+4\n\tRU4Faayz5MFMkO8gP3nO1dNpir857FD1oYbhQrk/HE+A8dz2MnH2mZ5ciz719hMOHLxW\n\tG0KNlxe+Gm9yAtuwlTYYtMGsYCfCRKnLiMefAG/0vOUAGM27mI6e5g0FOdY4ACDHU6s3\n\t7v/CLJRpNqnrwmFCZb7gQHLFJFwBaTeWQV9HNYpQhiGTPB/ueV+InatQ0zzaKJ/wwLGs\n\te7BNn/bAqm9OzNJfaGbAkZhVnq7CP+Lt0RHlltidBr5gkLKRc3Ed1T6BbIhHZ7Req/F9\n\t32jg==","X-Gm-Message-State":"AC+VfDz2oUrObHC6kTTvYxBcDF4pelVqZ9yjSMQnswE+VmrfsB1PT5m+\n\t+/mZgQzeUe8Vrf1XnAE9Mx9JdpCIUjjfyyhENn8KwDo5tSy65T+G4sE=","X-Google-Smtp-Source":"ACHHUZ6733mgV3YLihIqDNpDF8QDloJQSKp/Rd4EdANX9YpBCMFhnU9UvDHaYjd/q9l+VWFTBH7KR+7WJQ8zHJTnrYw=","X-Received":"by 2002:ad4:5aac:0:b0:62d:eda3:4335 with SMTP id\n\tu12-20020ad45aac000000b0062deda34335mr34829612qvg.29.1687769963564;\n\tMon, 26 Jun 2023 01:59:23 -0700 (PDT)","MIME-Version":"1.0","References":"<20230620142904.72600-1-jacopo.mondi@ideasonboard.com>\n\t\n\t\n\t\n\t\n\t\n\t<20230622151406.GA950@pendragon.ideasonboard.com>\n\t\n\t<3090b510-b3e9-e9fe-379e-278bb661f4d9@collabora.com>","In-Reply-To":"<3090b510-b3e9-e9fe-379e-278bb661f4d9@collabora.com>","Date":"Mon, 26 Jun 2023 09:59:12 +0100","Message-ID":"","To":"Robert Mader ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [RFC 0/4] libcamera: Replace\n\tCameraConfiguration::transform","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]