| Message ID | 20230620142904.72600-1-jacopo.mondi@ideasonboard.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo Thanks for sending this proposal. I wasn't sure I really understood it, perhaps you can help me out! On Tue, 20 Jun 2023 at 15:29, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hello everyone > > We have been discussing rotation/transformations for a long time, and > the currently implemented CameraConfiguration::transform behaviour has > proven to be confusing for several reasons: it was poorly documented, > translating it to something consumable for upper layer frameworks was > difficult and it behaved differently than any other field part of the > CameraConfiguration, as it was meant to tell libcamera "what to do" instead > of expressing what applications want. As far as I understand it, I don't believe this is correct, and the transform is where you say what you want. For example, on a Raspberry Pi I always set the transform to Transform::Identity because I want the images "the right way up". (I know that internally, it will calculate and apply a 180 rotation to counteract the sensor mounting rotation.) Does that sound right? If the problem is actually to do with documentation, I would always agree that it can be improved, and am very happy to help! > > For these reasons this series proposes to replace the usage to Transform > in the public API in favour of a new CameraConfiguration::Orientation type, > defined based on the EXIF specification, tag 274, 'orientation'. For reference > this is the same as implemented by gstreamer: > https://gstreamer.freedesktop.org/documentation/gstreamer/gsttaglist.html?gi-language=c#GST_TAG_IMAGE_ORIENTATION I must confess to not liking the term "orientation". There is such a thing as a "2D plane transformation" and I'd expect to refer to them as transformations or maybe transforms for short. I don't understand what an "orientation" is without reading extra documentation. Are "orientations" composable? Is there such a thing as an "inverse orientation"? > > The newly introduced CameraConfiguration::orientation replaces the > existing CameraConfiguration::tranform, and it is meant for application to > express how they would like the images to be oriented, not to tell libcamera > what to do. As an example, passing in 'rotation0' means that the application > expects the images to be rotated upright, and doesn't tell libcamera not to > apply any rotation like passing in "Transform::Identity" did. My understanding is that passing "Transform::Identity" has always meant that the application wants its images "upright". (Are there cases where this doesn't happen?) > > The value CameraConfiguration::orientation is set to after a validate() also > differs in meaning, as instead of reporting "what applications have to do > to obtain what they originally asked for" it simply reports the actual > orientation of the stream: this means that if libcamera cannot fully satisfy the > user request it will set ::orientation to report the native images rotation > and the CameraConfiguration::status will be set to Adjusted. I believe this to be the current behaviour of the transform field. > > Handling of 90 and 270 degrees rotation has also changed: as the camera sensor > cannot correct rotations that include a transposition, requests for a 90/270 > degrees are ignored, and cameras with a mounting rotation of 90/270 degrees > are never re-oriented. This makes it clear and less confusing for applications > that they have to deal with correction fully by themselves. As an example, with > the current implementation if the application requires a Rot270 (HFlip + > Transpose) libcamera will do the HFlip and leave transposition to the upper > layers. There is no clear advantage in doing so, and display compositors are > better suited for handling transpositions and flipping in a single pass instead > of having the library try to handle part of that. Again, I'm not really understanding this. The current behaviour is such that, if it can't give you the transformation that you ask for (on account of not being able to do transposition), then it guarantees to give you what you requested _apart_ from the transposition. So the application only has one case to deal with - applying a transpose. It doesn't have to worry about the other 3 "transforms-that-I-can't-handle", thereby making life easier for the application. Because transforms are composable, it's easy to calculate what transform to ask for to get a different behaviour (for example, do nothing at all if there's a transpose involved). I could imagine a slightly different API might be helpful, perhaps where you get to say not only "what you want", but also what you want the "residual transform" to be (being the one the application has to apply after) if it can't deliver what you wanted. Would that be useful? I also recall that there was some discussion a while back about the precise orientation of the rotations which could affect some of the details, but I don't think it was ever concluded. I don't know if that's causing any problems? > > This series clearly breaks the application API as it removes a member from > CameraConfiguration, so it should be introduced probably only when a new release > is cut. I can't comment on this as I haven't really understood the change here. Obviously we have many end users for whom an API change would be unwelcome, but I suppose the change could be hidden from them. I apologise if there have been some discussions flying around that I haven't followed sufficiently. I'm afraid I don't normally study gstreamer related posts in detail, perhaps I need to try harder... Thanks David > > Tested on RPi imx219 with the newly introduced cam option, flip, mirror and > rotation work as expected. > > Tested on Pinephone Pro, where sensors are 90/270 degrees rotated (libcamera > performs no correction) > > Comments welcome of course. > > Thanks > j > > Jacopo Mondi (4): > libcamera: camera_sensor: Cache rotationTransform_ > libcamera: camera: Introduce CameraConfiguration::orientation > libcamera: Move to use CameraConfiguration::orientation > apsp: cam: Add option to set stream orientation > > Documentation/Doxyfile.in | 2 + > Documentation/rotation/flip-rotate-0.eps | 170 +++++++++++++++ > Documentation/rotation/flip-rotate-0.png | Bin 0 -> 16488 bytes > Documentation/rotation/flip-rotate-180.eps | 191 +++++++++++++++++ > Documentation/rotation/flip-rotate-180.png | Bin 0 -> 22198 bytes > Documentation/rotation/flip-rotate-270.eps | 197 ++++++++++++++++++ > Documentation/rotation/flip-rotate-270.png | Bin 0 -> 21367 bytes > Documentation/rotation/flip-rotate-90.eps | 195 +++++++++++++++++ > Documentation/rotation/flip-rotate-90.png | Bin 0 -> 21020 bytes > Documentation/rotation/rotate-0.eps | 169 +++++++++++++++ > Documentation/rotation/rotate-0.png | Bin 0 -> 9086 bytes > Documentation/rotation/rotate-180.eps | 189 +++++++++++++++++ > Documentation/rotation/rotate-180.png | Bin 0 -> 22182 bytes > Documentation/rotation/rotate-270.eps | 196 +++++++++++++++++ > Documentation/rotation/rotate-270.png | Bin 0 -> 21016 bytes > Documentation/rotation/rotate-90.eps | 196 +++++++++++++++++ > Documentation/rotation/rotate-90.png | Bin 0 -> 21528 bytes > include/libcamera/camera.h | 14 +- > include/libcamera/internal/camera_sensor.h | 4 +- > include/libcamera/transform.h | 4 + > src/apps/cam/camera_session.cpp | 17 ++ > src/apps/cam/main.cpp | 5 + > src/apps/cam/main.h | 1 + > src/libcamera/camera.cpp | 54 ++++- > src/libcamera/camera_sensor.cpp | 137 ++++++------ > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +- > .../pipeline/rpi/common/pipeline_base.cpp | 9 +- > src/libcamera/pipeline/simple/simple.cpp | 6 +- > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +- > src/libcamera/pipeline/vimc/vimc.cpp | 4 +- > src/libcamera/transform.cpp | 58 ++++++ > 32 files changed, 1739 insertions(+), 97 deletions(-) > create mode 100644 Documentation/rotation/flip-rotate-0.eps > create mode 100644 Documentation/rotation/flip-rotate-0.png > create mode 100644 Documentation/rotation/flip-rotate-180.eps > create mode 100644 Documentation/rotation/flip-rotate-180.png > create mode 100644 Documentation/rotation/flip-rotate-270.eps > create mode 100644 Documentation/rotation/flip-rotate-270.png > create mode 100644 Documentation/rotation/flip-rotate-90.eps > create mode 100644 Documentation/rotation/flip-rotate-90.png > create mode 100644 Documentation/rotation/rotate-0.eps > create mode 100644 Documentation/rotation/rotate-0.png > create mode 100644 Documentation/rotation/rotate-180.eps > create mode 100644 Documentation/rotation/rotate-180.png > create mode 100644 Documentation/rotation/rotate-270.eps > create mode 100644 Documentation/rotation/rotate-270.png > create mode 100644 Documentation/rotation/rotate-90.eps > create mode 100644 Documentation/rotation/rotate-90.png > > -- > 2.40.1 >
Hi David On Wed, Jun 21, 2023 at 10:08:25AM +0100, David Plowman wrote: > Hi Jacopo > > Thanks for sending this proposal. I wasn't sure I really understood > it, perhaps you can help me out! > > On Tue, 20 Jun 2023 at 15:29, Jacopo Mondi > <jacopo.mondi@ideasonboard.com> wrote: > > > > Hello everyone > > > > We have been discussing rotation/transformations for a long time, and > > the currently implemented CameraConfiguration::transform behaviour has > > proven to be confusing for several reasons: it was poorly documented, > > translating it to something consumable for upper layer frameworks was > > difficult and it behaved differently than any other field part of the > > CameraConfiguration, as it was meant to tell libcamera "what to do" instead > > of expressing what applications want. > > As far as I understand it, I don't believe this is correct, and the > transform is where you say what you want. > > For example, on a Raspberry Pi I always set the transform to > Transform::Identity because I want the images "the right way up". (I > know that internally, it will calculate and apply a 180 rotation to > counteract the sensor mounting rotation.) Does that sound right? Yes, now that we correct properties::Rotation to 0 as the library compensate the mounting rotation behind your back, that's what you get. Before we had properties::Rotation = 180; transform = Identity and you got an upright image. Confusing, isn't it ? > > If the problem is actually to do with documentation, I would always > agree that it can be improved, and am very happy to help! > > > > > For these reasons this series proposes to replace the usage to Transform > > in the public API in favour of a new CameraConfiguration::Orientation type, > > defined based on the EXIF specification, tag 274, 'orientation'. For reference > > this is the same as implemented by gstreamer: > > https://gstreamer.freedesktop.org/documentation/gstreamer/gsttaglist.html?gi-language=c#GST_TAG_IMAGE_ORIENTATION > > I must confess to not liking the term "orientation". There is such a > thing as a "2D plane transformation" and I'd expect to refer to them > as transformations or maybe transforms for short. I don't understand > what an "orientation" is without reading extra documentation. Are > "orientations" composable? Is there such a thing as an "inverse > orientation"? > I'm fine bikeshedding on the name. What matters to me is that now we conform to an existing specification adopeted by most frameworks instead of defining our own Transform. > > > > The newly introduced CameraConfiguration::orientation replaces the > > existing CameraConfiguration::tranform, and it is meant for application to > > express how they would like the images to be oriented, not to tell libcamera > > what to do. As an example, passing in 'rotation0' means that the application > > expects the images to be rotated upright, and doesn't tell libcamera not to > > apply any rotation like passing in "Transform::Identity" did. > > My understanding is that passing "Transform::Identity" has always > meant that the application wants its images "upright". (Are there > cases where this doesn't happen?) > As we have auto-correction in place, yes. > > > > The value CameraConfiguration::orientation is set to after a validate() also > > differs in meaning, as instead of reporting "what applications have to do > > to obtain what they originally asked for" it simply reports the actual > > orientation of the stream: this means that if libcamera cannot fully satisfy the > > user request it will set ::orientation to report the native images rotation > > and the CameraConfiguration::status will be set to Adjusted. > > I believe this to be the current behaviour of the transform field. > Not really, see below > > > > Handling of 90 and 270 degrees rotation has also changed: as the camera sensor > > cannot correct rotations that include a transposition, requests for a 90/270 > > degrees are ignored, and cameras with a mounting rotation of 90/270 degrees > > are never re-oriented. This makes it clear and less confusing for applications > > that they have to deal with correction fully by themselves. As an example, with > > the current implementation if the application requires a Rot270 (HFlip + > > Transpose) libcamera will do the HFlip and leave transposition to the upper > > layers. There is no clear advantage in doing so, and display compositors are > > better suited for handling transpositions and flipping in a single pass instead > > of having the library try to handle part of that. > > Again, I'm not really understanding this. > > The current behaviour is such that, if it can't give you the > transformation that you ask for (on account of not being able to do > transposition), then it guarantees to give you what you requested > _apart_ from the transposition. So the application only has one case > to deal with - applying a transpose. It doesn't have to worry about > the other 3 "transforms-that-I-can't-handle", thereby making life > easier for the application. The _apart_ is the key here. If you currently ask for Rot270 the library will do an HFlip and set transform to Transpose (ie what applications still have to do to obtain Rot270). This is not how the other fields of CameraConfiguration work, and mostly, applying a flip and letting apps only deal with transpose is an "optimization" that might actually make life harder for apps, as display compositors usually have means to deal with known rotations, while in this case you only ask them to do a Transpose, which might require them more work ? In any case, I think it's saner if the library cannot do what it has been asked for to set ::transform to what the stream orientation actually is, instead of telling apps what "they still have to do". > > Because transforms are composable, it's easy to calculate what > transform to ask for to get a different behaviour (for example, do > nothing at all if there's a transpose involved). I could imagine a > slightly different API might be helpful, perhaps where you get to say > not only "what you want", but also what you want the "residual > transform" to be (being the one the application has to apply after) if > it can't deliver what you wanted. Would that be useful? > I think it's better to leave the stream orientation unmodified if it cannot be corrected. This matches the behaviour of the other CameraConfiguration fields, and seems easier to deal with from applications. > I also recall that there was some discussion a while back about the > precise orientation of the rotations which could affect some of the > details, but I don't think it was ever concluded. I don't know if > that's causing any problems? Sorry, I don't remember such discussion. Any link ? > > > > > This series clearly breaks the application API as it removes a member from > > CameraConfiguration, so it should be introduced probably only when a new release > > is cut. > > I can't comment on this as I haven't really understood the change > here. Obviously we have many end users for whom an API change would be > unwelcome, but I suppose the change could be hidden from them. > > I apologise if there have been some discussions flying around that I > haven't followed sufficiently. I'm afraid I don't normally study > gstreamer related posts in detail, perhaps I need to try harder... > > Thanks > David > > > > > Tested on RPi imx219 with the newly introduced cam option, flip, mirror and > > rotation work as expected. > > > > Tested on Pinephone Pro, where sensors are 90/270 degrees rotated (libcamera > > performs no correction) > > > > Comments welcome of course. > > > > Thanks > > j > > > > Jacopo Mondi (4): > > libcamera: camera_sensor: Cache rotationTransform_ > > libcamera: camera: Introduce CameraConfiguration::orientation > > libcamera: Move to use CameraConfiguration::orientation > > apsp: cam: Add option to set stream orientation > > > > Documentation/Doxyfile.in | 2 + > > Documentation/rotation/flip-rotate-0.eps | 170 +++++++++++++++ > > Documentation/rotation/flip-rotate-0.png | Bin 0 -> 16488 bytes > > Documentation/rotation/flip-rotate-180.eps | 191 +++++++++++++++++ > > Documentation/rotation/flip-rotate-180.png | Bin 0 -> 22198 bytes > > Documentation/rotation/flip-rotate-270.eps | 197 ++++++++++++++++++ > > Documentation/rotation/flip-rotate-270.png | Bin 0 -> 21367 bytes > > Documentation/rotation/flip-rotate-90.eps | 195 +++++++++++++++++ > > Documentation/rotation/flip-rotate-90.png | Bin 0 -> 21020 bytes > > Documentation/rotation/rotate-0.eps | 169 +++++++++++++++ > > Documentation/rotation/rotate-0.png | Bin 0 -> 9086 bytes > > Documentation/rotation/rotate-180.eps | 189 +++++++++++++++++ > > Documentation/rotation/rotate-180.png | Bin 0 -> 22182 bytes > > Documentation/rotation/rotate-270.eps | 196 +++++++++++++++++ > > Documentation/rotation/rotate-270.png | Bin 0 -> 21016 bytes > > Documentation/rotation/rotate-90.eps | 196 +++++++++++++++++ > > Documentation/rotation/rotate-90.png | Bin 0 -> 21528 bytes > > include/libcamera/camera.h | 14 +- > > include/libcamera/internal/camera_sensor.h | 4 +- > > include/libcamera/transform.h | 4 + > > src/apps/cam/camera_session.cpp | 17 ++ > > src/apps/cam/main.cpp | 5 + > > src/apps/cam/main.h | 1 + > > src/libcamera/camera.cpp | 54 ++++- > > src/libcamera/camera_sensor.cpp | 137 ++++++------ > > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +- > > .../pipeline/rpi/common/pipeline_base.cpp | 9 +- > > src/libcamera/pipeline/simple/simple.cpp | 6 +- > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +- > > src/libcamera/pipeline/vimc/vimc.cpp | 4 +- > > src/libcamera/transform.cpp | 58 ++++++ > > 32 files changed, 1739 insertions(+), 97 deletions(-) > > create mode 100644 Documentation/rotation/flip-rotate-0.eps > > create mode 100644 Documentation/rotation/flip-rotate-0.png > > create mode 100644 Documentation/rotation/flip-rotate-180.eps > > create mode 100644 Documentation/rotation/flip-rotate-180.png > > create mode 100644 Documentation/rotation/flip-rotate-270.eps > > create mode 100644 Documentation/rotation/flip-rotate-270.png > > create mode 100644 Documentation/rotation/flip-rotate-90.eps > > create mode 100644 Documentation/rotation/flip-rotate-90.png > > create mode 100644 Documentation/rotation/rotate-0.eps > > create mode 100644 Documentation/rotation/rotate-0.png > > create mode 100644 Documentation/rotation/rotate-180.eps > > create mode 100644 Documentation/rotation/rotate-180.png > > create mode 100644 Documentation/rotation/rotate-270.eps > > create mode 100644 Documentation/rotation/rotate-270.png > > create mode 100644 Documentation/rotation/rotate-90.eps > > create mode 100644 Documentation/rotation/rotate-90.png > > > > -- > > 2.40.1 > >
Hi Jacopo Thanks for the reply. On Wed, 21 Jun 2023 at 10:23, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi David > > On Wed, Jun 21, 2023 at 10:08:25AM +0100, David Plowman wrote: > > Hi Jacopo > > > > Thanks for sending this proposal. I wasn't sure I really understood > > it, perhaps you can help me out! > > > > On Tue, 20 Jun 2023 at 15:29, Jacopo Mondi > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > Hello everyone > > > > > > We have been discussing rotation/transformations for a long time, and > > > the currently implemented CameraConfiguration::transform behaviour has > > > proven to be confusing for several reasons: it was poorly documented, > > > translating it to something consumable for upper layer frameworks was > > > difficult and it behaved differently than any other field part of the > > > CameraConfiguration, as it was meant to tell libcamera "what to do" instead > > > of expressing what applications want. > > > > As far as I understand it, I don't believe this is correct, and the > > transform is where you say what you want. > > > > For example, on a Raspberry Pi I always set the transform to > > Transform::Identity because I want the images "the right way up". (I > > know that internally, it will calculate and apply a 180 rotation to > > counteract the sensor mounting rotation.) Does that sound right? > > Yes, now that we correct properties::Rotation to 0 as the library > compensate the mounting rotation behind your back, that's what you > get. > > Before we had properties::Rotation = 180; transform = Identity and you > got an upright image. Confusing, isn't it ? I understood the previous arrangement, but the new one seems counter-intuitive to me. On a Raspberry Pi, I expect the properties::Rotation to report 180 degrees because the sensor is mounted upside down. Then I can ask for Transform::Identity and internally a 180 degree rotation will be applied to give me what I asked for. I understand this. Are we saying that now the rotation will report 0 degrees? Will it now always report 0 degrees, whatever the sensor and platform? That would obviously break things... > > > > > If the problem is actually to do with documentation, I would always > > agree that it can be improved, and am very happy to help! > > > > > > > > For these reasons this series proposes to replace the usage to Transform > > > in the public API in favour of a new CameraConfiguration::Orientation type, > > > defined based on the EXIF specification, tag 274, 'orientation'. For reference > > > this is the same as implemented by gstreamer: > > > https://gstreamer.freedesktop.org/documentation/gstreamer/gsttaglist.html?gi-language=c#GST_TAG_IMAGE_ORIENTATION > > > > I must confess to not liking the term "orientation". There is such a > > thing as a "2D plane transformation" and I'd expect to refer to them > > as transformations or maybe transforms for short. I don't understand > > what an "orientation" is without reading extra documentation. Are > > "orientations" composable? Is there such a thing as an "inverse > > orientation"? > > > > I'm fine bikeshedding on the name. What matters to me is that now we > conform to an existing specification adopeted by most frameworks > instead of defining our own Transform. I'd like to find something that works for all libcamera users. The Transform class allows composition, inverse operations and so on, the idea being that users can calculate for themselves what to ask for if the default behaviour isn't what they want. I did comment that making it easier for folks to get a specific behaviour might be something to look into. > > > > > > > The newly introduced CameraConfiguration::orientation replaces the > > > existing CameraConfiguration::tranform, and it is meant for application to > > > express how they would like the images to be oriented, not to tell libcamera > > > what to do. As an example, passing in 'rotation0' means that the application > > > expects the images to be rotated upright, and doesn't tell libcamera not to > > > apply any rotation like passing in "Transform::Identity" did. > > > > My understanding is that passing "Transform::Identity" has always > > meant that the application wants its images "upright". (Are there > > cases where this doesn't happen?) > > > > As we have auto-correction in place, yes. My understanding is that everything got "auto-corrected" before. I'm still struggling to understand how it seems to be broken now. > > > > > > > The value CameraConfiguration::orientation is set to after a validate() also > > > differs in meaning, as instead of reporting "what applications have to do > > > to obtain what they originally asked for" it simply reports the actual > > > orientation of the stream: this means that if libcamera cannot fully satisfy the > > > user request it will set ::orientation to report the native images rotation > > > and the CameraConfiguration::status will be set to Adjusted. > > > > I believe this to be the current behaviour of the transform field. > > > > Not really, see below > > > > > > > Handling of 90 and 270 degrees rotation has also changed: as the camera sensor > > > cannot correct rotations that include a transposition, requests for a 90/270 > > > degrees are ignored, and cameras with a mounting rotation of 90/270 degrees > > > are never re-oriented. This makes it clear and less confusing for applications > > > that they have to deal with correction fully by themselves. As an example, with > > > the current implementation if the application requires a Rot270 (HFlip + > > > Transpose) libcamera will do the HFlip and leave transposition to the upper > > > layers. There is no clear advantage in doing so, and display compositors are > > > better suited for handling transpositions and flipping in a single pass instead > > > of having the library try to handle part of that. > > > > Again, I'm not really understanding this. > > > > The current behaviour is such that, if it can't give you the > > transformation that you ask for (on account of not being able to do > > transposition), then it guarantees to give you what you requested > > _apart_ from the transposition. So the application only has one case > > to deal with - applying a transpose. It doesn't have to worry about > > the other 3 "transforms-that-I-can't-handle", thereby making life > > easier for the application. > > The _apart_ is the key here. > > If you currently ask for Rot270 the library will do an HFlip and set > transform to Transpose (ie what applications still have to do to > obtain Rot270). This is not how the other fields of What it _should_ do is set the transform field to the actual transform that the user will perceive the images to have. I believe this to be "proper" libcamera configuration behaviour. I'm not understanding why things are different now. Beyond this, the current behaviour is to leave the application to do the transpose, but that's just a choice. The point of transforms is that it's easy to use them to produce something different if you want. I don't mind looking at that if we can make it easier for everyone to get their preferred behaviour, which I suggested in my original reply. > CameraConfiguration work, and mostly, applying a flip and letting apps > only deal with transpose is an "optimization" that might actually make > life harder for apps, as display compositors usually have means to > deal with known rotations, while in this case you only ask them to do > a Transpose, which might require them more work ? > > In any case, I think it's saner if the library cannot do what it has > been asked for to set ::transform to what the stream orientation > actually is, instead of telling apps what "they still have to do". I don't understand the repeated statement that the transform is telling apps "what they still have to do". That was never what it meant, unless it got changed somewhere. > > > > > Because transforms are composable, it's easy to calculate what > > transform to ask for to get a different behaviour (for example, do > > nothing at all if there's a transpose involved). I could imagine a > > slightly different API might be helpful, perhaps where you get to say > > not only "what you want", but also what you want the "residual > > transform" to be (being the one the application has to apply after) if > > it can't deliver what you wanted. Would that be useful? > > > > I think it's better to leave the stream orientation unmodified if it > cannot be corrected. This matches the behaviour of the other > CameraConfiguration fields, and seems easier to deal with from > applications. I feel we ought to be able to come up with something that makes it convenient for everyone to get what they want. > > > I also recall that there was some discussion a while back about the > > precise orientation of the rotations which could affect some of the > > details, but I don't think it was ever concluded. I don't know if > > that's causing any problems? > > Sorry, I don't remember such discussion. Any link ? I think the last email I sent on the subject was this one, after which things went quiet: https://lists.libcamera.org/pipermail/libcamera-devel/2023-January/036445.html Actually it seems to echo quite a bit of what I say above. Thanks David > > > > > > > > > This series clearly breaks the application API as it removes a member from > > > CameraConfiguration, so it should be introduced probably only when a new release > > > is cut. > > > > I can't comment on this as I haven't really understood the change > > here. Obviously we have many end users for whom an API change would be > > unwelcome, but I suppose the change could be hidden from them. > > > > I apologise if there have been some discussions flying around that I > > haven't followed sufficiently. I'm afraid I don't normally study > > gstreamer related posts in detail, perhaps I need to try harder... > > > > Thanks > > David > > > > > > > > Tested on RPi imx219 with the newly introduced cam option, flip, mirror and > > > rotation work as expected. > > > > > > Tested on Pinephone Pro, where sensors are 90/270 degrees rotated (libcamera > > > performs no correction) > > > > > > Comments welcome of course. > > > > > > Thanks > > > j > > > > > > Jacopo Mondi (4): > > > libcamera: camera_sensor: Cache rotationTransform_ > > > libcamera: camera: Introduce CameraConfiguration::orientation > > > libcamera: Move to use CameraConfiguration::orientation > > > apsp: cam: Add option to set stream orientation > > > > > > Documentation/Doxyfile.in | 2 + > > > Documentation/rotation/flip-rotate-0.eps | 170 +++++++++++++++ > > > Documentation/rotation/flip-rotate-0.png | Bin 0 -> 16488 bytes > > > Documentation/rotation/flip-rotate-180.eps | 191 +++++++++++++++++ > > > Documentation/rotation/flip-rotate-180.png | Bin 0 -> 22198 bytes > > > Documentation/rotation/flip-rotate-270.eps | 197 ++++++++++++++++++ > > > Documentation/rotation/flip-rotate-270.png | Bin 0 -> 21367 bytes > > > Documentation/rotation/flip-rotate-90.eps | 195 +++++++++++++++++ > > > Documentation/rotation/flip-rotate-90.png | Bin 0 -> 21020 bytes > > > Documentation/rotation/rotate-0.eps | 169 +++++++++++++++ > > > Documentation/rotation/rotate-0.png | Bin 0 -> 9086 bytes > > > Documentation/rotation/rotate-180.eps | 189 +++++++++++++++++ > > > Documentation/rotation/rotate-180.png | Bin 0 -> 22182 bytes > > > Documentation/rotation/rotate-270.eps | 196 +++++++++++++++++ > > > Documentation/rotation/rotate-270.png | Bin 0 -> 21016 bytes > > > Documentation/rotation/rotate-90.eps | 196 +++++++++++++++++ > > > Documentation/rotation/rotate-90.png | Bin 0 -> 21528 bytes > > > include/libcamera/camera.h | 14 +- > > > include/libcamera/internal/camera_sensor.h | 4 +- > > > include/libcamera/transform.h | 4 + > > > src/apps/cam/camera_session.cpp | 17 ++ > > > src/apps/cam/main.cpp | 5 + > > > src/apps/cam/main.h | 1 + > > > src/libcamera/camera.cpp | 54 ++++- > > > src/libcamera/camera_sensor.cpp | 137 ++++++------ > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +- > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +- > > > .../pipeline/rpi/common/pipeline_base.cpp | 9 +- > > > src/libcamera/pipeline/simple/simple.cpp | 6 +- > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +- > > > src/libcamera/pipeline/vimc/vimc.cpp | 4 +- > > > src/libcamera/transform.cpp | 58 ++++++ > > > 32 files changed, 1739 insertions(+), 97 deletions(-) > > > create mode 100644 Documentation/rotation/flip-rotate-0.eps > > > create mode 100644 Documentation/rotation/flip-rotate-0.png > > > create mode 100644 Documentation/rotation/flip-rotate-180.eps > > > create mode 100644 Documentation/rotation/flip-rotate-180.png > > > create mode 100644 Documentation/rotation/flip-rotate-270.eps > > > create mode 100644 Documentation/rotation/flip-rotate-270.png > > > create mode 100644 Documentation/rotation/flip-rotate-90.eps > > > create mode 100644 Documentation/rotation/flip-rotate-90.png > > > create mode 100644 Documentation/rotation/rotate-0.eps > > > create mode 100644 Documentation/rotation/rotate-0.png > > > create mode 100644 Documentation/rotation/rotate-180.eps > > > create mode 100644 Documentation/rotation/rotate-180.png > > > create mode 100644 Documentation/rotation/rotate-270.eps > > > create mode 100644 Documentation/rotation/rotate-270.png > > > create mode 100644 Documentation/rotation/rotate-90.eps > > > create mode 100644 Documentation/rotation/rotate-90.png > > > > > > -- > > > 2.40.1 > > >
Hi David On Wed, Jun 21, 2023 at 01:37:11PM +0100, David Plowman wrote: > Hi Jacopo > > Thanks for the reply. > > On Wed, 21 Jun 2023 at 10:23, Jacopo Mondi > <jacopo.mondi@ideasonboard.com> wrote: > > > > Hi David > > > > On Wed, Jun 21, 2023 at 10:08:25AM +0100, David Plowman wrote: > > > Hi Jacopo > > > > > > Thanks for sending this proposal. I wasn't sure I really understood > > > it, perhaps you can help me out! > > > > > > On Tue, 20 Jun 2023 at 15:29, Jacopo Mondi > > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > > > Hello everyone > > > > > > > > We have been discussing rotation/transformations for a long time, and > > > > the currently implemented CameraConfiguration::transform behaviour has > > > > proven to be confusing for several reasons: it was poorly documented, > > > > translating it to something consumable for upper layer frameworks was > > > > difficult and it behaved differently than any other field part of the > > > > CameraConfiguration, as it was meant to tell libcamera "what to do" instead > > > > of expressing what applications want. > > > > > > As far as I understand it, I don't believe this is correct, and the > > > transform is where you say what you want. > > > > > > For example, on a Raspberry Pi I always set the transform to > > > Transform::Identity because I want the images "the right way up". (I > > > know that internally, it will calculate and apply a 180 rotation to > > > counteract the sensor mounting rotation.) Does that sound right? > > > > Yes, now that we correct properties::Rotation to 0 as the library > > compensate the mounting rotation behind your back, that's what you > > get. > > > > Before we had properties::Rotation = 180; transform = Identity and you > > got an upright image. Confusing, isn't it ? > > I understood the previous arrangement, but the new one seems > counter-intuitive to me. > > On a Raspberry Pi, I expect the properties::Rotation to report 180 > degrees because the sensor is mounted upside down. Then I can ask for > Transform::Identity and internally a 180 degree rotation will be > applied to give me what I asked for. I understand this. > > Are we saying that now the rotation will report 0 degrees? Will it now > always report 0 degrees, whatever the sensor and platform? That would > obviously break things... > So you're an app, you read properties::Rotation and you get back 180. Then you star streaming and you get a stream oriented "correctly". Isn't this wrong ? > > > > > > > > If the problem is actually to do with documentation, I would always > > > agree that it can be improved, and am very happy to help! > > > > > > > > > > > For these reasons this series proposes to replace the usage to Transform > > > > in the public API in favour of a new CameraConfiguration::Orientation type, > > > > defined based on the EXIF specification, tag 274, 'orientation'. For reference > > > > this is the same as implemented by gstreamer: > > > > https://gstreamer.freedesktop.org/documentation/gstreamer/gsttaglist.html?gi-language=c#GST_TAG_IMAGE_ORIENTATION > > > > > > I must confess to not liking the term "orientation". There is such a > > > thing as a "2D plane transformation" and I'd expect to refer to them > > > as transformations or maybe transforms for short. I don't understand > > > what an "orientation" is without reading extra documentation. Are > > > "orientations" composable? Is there such a thing as an "inverse > > > orientation"? > > > > > > > I'm fine bikeshedding on the name. What matters to me is that now we > > conform to an existing specification adopeted by most frameworks > > instead of defining our own Transform. > > I'd like to find something that works for all libcamera users. > > The Transform class allows composition, inverse operations and so on, > the idea being that users can calculate for themselves what to ask for > if the default behaviour isn't what they want. I did comment that > making it easier for folks to get a specific behaviour might be > something to look into. > > > > > > > > > > > The newly introduced CameraConfiguration::orientation replaces the > > > > existing CameraConfiguration::tranform, and it is meant for application to > > > > express how they would like the images to be oriented, not to tell libcamera > > > > what to do. As an example, passing in 'rotation0' means that the application > > > > expects the images to be rotated upright, and doesn't tell libcamera not to > > > > apply any rotation like passing in "Transform::Identity" did. > > > > > > My understanding is that passing "Transform::Identity" has always > > > meant that the application wants its images "upright". (Are there > > > cases where this doesn't happen?) > > > > > > > As we have auto-correction in place, yes. > > My understanding is that everything got "auto-corrected" before. I'm > still struggling to understand how it seems to be broken now. > It is not 'broken'. Simply nobody got how thing worked in practice and the existing behaviour (when it comes to adjusting ::transform) is not what most users expects. See below > > > > > > > > > > The value CameraConfiguration::orientation is set to after a validate() also > > > > differs in meaning, as instead of reporting "what applications have to do > > > > to obtain what they originally asked for" it simply reports the actual > > > > orientation of the stream: this means that if libcamera cannot fully satisfy the > > > > user request it will set ::orientation to report the native images rotation > > > > and the CameraConfiguration::status will be set to Adjusted. > > > > > > I believe this to be the current behaviour of the transform field. > > > > > > > Not really, see below > > > > > > > > > > Handling of 90 and 270 degrees rotation has also changed: as the camera sensor > > > > cannot correct rotations that include a transposition, requests for a 90/270 > > > > degrees are ignored, and cameras with a mounting rotation of 90/270 degrees > > > > are never re-oriented. This makes it clear and less confusing for applications > > > > that they have to deal with correction fully by themselves. As an example, with > > > > the current implementation if the application requires a Rot270 (HFlip + > > > > Transpose) libcamera will do the HFlip and leave transposition to the upper > > > > layers. There is no clear advantage in doing so, and display compositors are > > > > better suited for handling transpositions and flipping in a single pass instead > > > > of having the library try to handle part of that. > > > > > > Again, I'm not really understanding this. > > > > > > The current behaviour is such that, if it can't give you the > > > transformation that you ask for (on account of not being able to do > > > transposition), then it guarantees to give you what you requested > > > _apart_ from the transposition. So the application only has one case > > > to deal with - applying a transpose. It doesn't have to worry about > > > the other 3 "transforms-that-I-can't-handle", thereby making life > > > easier for the application. > > > > The _apart_ is the key here. > > > > If you currently ask for Rot270 the library will do an HFlip and set > > transform to Transpose (ie what applications still have to do to > > obtain Rot270). This is not how the other fields of > > What it _should_ do is set the transform field to the actual transform > that the user will perceive the images to have. I believe this to be > "proper" libcamera configuration behaviour. I'm not understanding why > things are different now. > Let's start with the fact that "transpose" is a concept that is introduced in Transform and it's not part of the EXIF standard which gstreamer/pipewire expect to use. Again, you set transform = rot270, libcamera does HFLIP and you have to do Transpose, correct ? For compositor this is more work that just do "rot270" > Beyond this, the current behaviour is to leave the application to do > the transpose, but that's just a choice. The point of transforms is > that it's easy to use them to produce something different if you want. > I don't mind looking at that if we can make it easier for everyone to > get their preferred behaviour, which I suggested in my original reply. > I don't get why we should be imposing other frameworks/apps to learn Transpose when there is a standard to follow to be honest. > > CameraConfiguration work, and mostly, applying a flip and letting apps > > only deal with transpose is an "optimization" that might actually make > > life harder for apps, as display compositors usually have means to > > deal with known rotations, while in this case you only ask them to do > > a Transpose, which might require them more work ? > > > > In any case, I think it's saner if the library cannot do what it has > > been asked for to set ::transform to what the stream orientation > > actually is, instead of telling apps what "they still have to do". > > I don't understand the repeated statement that the transform is > telling apps "what they still have to do". That was never what it > meant, unless it got changed somewhere. > Isn't it ? I'm not repeating the Rot270 example, but applications are left with ::transform = Transpose. Isn't this what they have to do to obtain Rot270 once HFlip has been done by libcamera ? So they have to "I asked for Rot270, I got back Transpose. Ah! libcamera did HFlip for me, so now I have to do Transpose." Is it what happens ? > > > > > > > > Because transforms are composable, it's easy to calculate what > > > transform to ask for to get a different behaviour (for example, do > > > nothing at all if there's a transpose involved). I could imagine a > > > slightly different API might be helpful, perhaps where you get to say > > > not only "what you want", but also what you want the "residual > > > transform" to be (being the one the application has to apply after) if > > > it can't deliver what you wanted. Would that be useful? > > > > > > > I think it's better to leave the stream orientation unmodified if it > > cannot be corrected. This matches the behaviour of the other > > CameraConfiguration fields, and seems easier to deal with from > > applications. > > I feel we ought to be able to come up with something that makes it > convenient for everyone to get what they want. > > > > > > I also recall that there was some discussion a while back about the > > > precise orientation of the rotations which could affect some of the > > > details, but I don't think it was ever concluded. I don't know if > > > that's causing any problems? > > > > Sorry, I don't remember such discussion. Any link ? > > I think the last email I sent on the subject was this one, after which > things went quiet: > https://lists.libcamera.org/pipermail/libcamera-devel/2023-January/036445.html > Actually it seems to echo quite a bit of what I say above. > > Thanks > David > > > > > > > > > > > > > > This series clearly breaks the application API as it removes a member from > > > > CameraConfiguration, so it should be introduced probably only when a new release > > > > is cut. > > > > > > I can't comment on this as I haven't really understood the change > > > here. Obviously we have many end users for whom an API change would be > > > unwelcome, but I suppose the change could be hidden from them. > > > > > > I apologise if there have been some discussions flying around that I > > > haven't followed sufficiently. I'm afraid I don't normally study > > > gstreamer related posts in detail, perhaps I need to try harder... > > > > > > Thanks > > > David > > > > > > > > > > > Tested on RPi imx219 with the newly introduced cam option, flip, mirror and > > > > rotation work as expected. > > > > > > > > Tested on Pinephone Pro, where sensors are 90/270 degrees rotated (libcamera > > > > performs no correction) > > > > > > > > Comments welcome of course. > > > > > > > > Thanks > > > > j > > > > > > > > Jacopo Mondi (4): > > > > libcamera: camera_sensor: Cache rotationTransform_ > > > > libcamera: camera: Introduce CameraConfiguration::orientation > > > > libcamera: Move to use CameraConfiguration::orientation > > > > apsp: cam: Add option to set stream orientation > > > > > > > > Documentation/Doxyfile.in | 2 + > > > > Documentation/rotation/flip-rotate-0.eps | 170 +++++++++++++++ > > > > Documentation/rotation/flip-rotate-0.png | Bin 0 -> 16488 bytes > > > > Documentation/rotation/flip-rotate-180.eps | 191 +++++++++++++++++ > > > > Documentation/rotation/flip-rotate-180.png | Bin 0 -> 22198 bytes > > > > Documentation/rotation/flip-rotate-270.eps | 197 ++++++++++++++++++ > > > > Documentation/rotation/flip-rotate-270.png | Bin 0 -> 21367 bytes > > > > Documentation/rotation/flip-rotate-90.eps | 195 +++++++++++++++++ > > > > Documentation/rotation/flip-rotate-90.png | Bin 0 -> 21020 bytes > > > > Documentation/rotation/rotate-0.eps | 169 +++++++++++++++ > > > > Documentation/rotation/rotate-0.png | Bin 0 -> 9086 bytes > > > > Documentation/rotation/rotate-180.eps | 189 +++++++++++++++++ > > > > Documentation/rotation/rotate-180.png | Bin 0 -> 22182 bytes > > > > Documentation/rotation/rotate-270.eps | 196 +++++++++++++++++ > > > > Documentation/rotation/rotate-270.png | Bin 0 -> 21016 bytes > > > > Documentation/rotation/rotate-90.eps | 196 +++++++++++++++++ > > > > Documentation/rotation/rotate-90.png | Bin 0 -> 21528 bytes > > > > include/libcamera/camera.h | 14 +- > > > > include/libcamera/internal/camera_sensor.h | 4 +- > > > > include/libcamera/transform.h | 4 + > > > > src/apps/cam/camera_session.cpp | 17 ++ > > > > src/apps/cam/main.cpp | 5 + > > > > src/apps/cam/main.h | 1 + > > > > src/libcamera/camera.cpp | 54 ++++- > > > > src/libcamera/camera_sensor.cpp | 137 ++++++------ > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +- > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +- > > > > .../pipeline/rpi/common/pipeline_base.cpp | 9 +- > > > > src/libcamera/pipeline/simple/simple.cpp | 6 +- > > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +- > > > > src/libcamera/pipeline/vimc/vimc.cpp | 4 +- > > > > src/libcamera/transform.cpp | 58 ++++++ > > > > 32 files changed, 1739 insertions(+), 97 deletions(-) > > > > create mode 100644 Documentation/rotation/flip-rotate-0.eps > > > > create mode 100644 Documentation/rotation/flip-rotate-0.png > > > > create mode 100644 Documentation/rotation/flip-rotate-180.eps > > > > create mode 100644 Documentation/rotation/flip-rotate-180.png > > > > create mode 100644 Documentation/rotation/flip-rotate-270.eps > > > > create mode 100644 Documentation/rotation/flip-rotate-270.png > > > > create mode 100644 Documentation/rotation/flip-rotate-90.eps > > > > create mode 100644 Documentation/rotation/flip-rotate-90.png > > > > create mode 100644 Documentation/rotation/rotate-0.eps > > > > create mode 100644 Documentation/rotation/rotate-0.png > > > > create mode 100644 Documentation/rotation/rotate-180.eps > > > > create mode 100644 Documentation/rotation/rotate-180.png > > > > create mode 100644 Documentation/rotation/rotate-270.eps > > > > create mode 100644 Documentation/rotation/rotate-270.png > > > > create mode 100644 Documentation/rotation/rotate-90.eps > > > > create mode 100644 Documentation/rotation/rotate-90.png > > > > > > > > -- > > > > 2.40.1 > > > >
Hi again On Wed, 21 Jun 2023 at 13:57, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi David > > On Wed, Jun 21, 2023 at 01:37:11PM +0100, David Plowman wrote: > > Hi Jacopo > > > > Thanks for the reply. > > > > On Wed, 21 Jun 2023 at 10:23, Jacopo Mondi > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > Hi David > > > > > > On Wed, Jun 21, 2023 at 10:08:25AM +0100, David Plowman wrote: > > > > Hi Jacopo > > > > > > > > Thanks for sending this proposal. I wasn't sure I really understood > > > > it, perhaps you can help me out! > > > > > > > > On Tue, 20 Jun 2023 at 15:29, Jacopo Mondi > > > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > > > > > Hello everyone > > > > > > > > > > We have been discussing rotation/transformations for a long time, and > > > > > the currently implemented CameraConfiguration::transform behaviour has > > > > > proven to be confusing for several reasons: it was poorly documented, > > > > > translating it to something consumable for upper layer frameworks was > > > > > difficult and it behaved differently than any other field part of the > > > > > CameraConfiguration, as it was meant to tell libcamera "what to do" instead > > > > > of expressing what applications want. > > > > > > > > As far as I understand it, I don't believe this is correct, and the > > > > transform is where you say what you want. > > > > > > > > For example, on a Raspberry Pi I always set the transform to > > > > Transform::Identity because I want the images "the right way up". (I > > > > know that internally, it will calculate and apply a 180 rotation to > > > > counteract the sensor mounting rotation.) Does that sound right? > > > > > > Yes, now that we correct properties::Rotation to 0 as the library > > > compensate the mounting rotation behind your back, that's what you > > > get. > > > > > > Before we had properties::Rotation = 180; transform = Identity and you > > > got an upright image. Confusing, isn't it ? > > > > I understood the previous arrangement, but the new one seems > > counter-intuitive to me. > > > > On a Raspberry Pi, I expect the properties::Rotation to report 180 > > degrees because the sensor is mounted upside down. Then I can ask for > > Transform::Identity and internally a 180 degree rotation will be > > applied to give me what I asked for. I understand this. > > > > Are we saying that now the rotation will report 0 degrees? Will it now > > always report 0 degrees, whatever the sensor and platform? That would > > obviously break things... > > > > So you're an app, you read properties::Rotation and you get back 180. > Then you star streaming and you get a stream oriented "correctly". > > Isn't this wrong ? I think a lot of this hinges on what the Rotation property is. It _used_ to be the mounting rotation of the sensor (is that right?), at which point everything was as I have described it. But I think it is being changed now, which is indeed something that was under discussion in that thread back in January. We're free to do that, but perhaps there have been some unintended consequences? > > > > > > > > > > > > If the problem is actually to do with documentation, I would always > > > > agree that it can be improved, and am very happy to help! > > > > > > > > > > > > > > For these reasons this series proposes to replace the usage to Transform > > > > > in the public API in favour of a new CameraConfiguration::Orientation type, > > > > > defined based on the EXIF specification, tag 274, 'orientation'. For reference > > > > > this is the same as implemented by gstreamer: > > > > > https://gstreamer.freedesktop.org/documentation/gstreamer/gsttaglist.html?gi-language=c#GST_TAG_IMAGE_ORIENTATION > > > > > > > > I must confess to not liking the term "orientation". There is such a > > > > thing as a "2D plane transformation" and I'd expect to refer to them > > > > as transformations or maybe transforms for short. I don't understand > > > > what an "orientation" is without reading extra documentation. Are > > > > "orientations" composable? Is there such a thing as an "inverse > > > > orientation"? > > > > > > > > > > I'm fine bikeshedding on the name. What matters to me is that now we > > > conform to an existing specification adopeted by most frameworks > > > instead of defining our own Transform. > > > > I'd like to find something that works for all libcamera users. > > > > The Transform class allows composition, inverse operations and so on, > > the idea being that users can calculate for themselves what to ask for > > if the default behaviour isn't what they want. I did comment that > > making it easier for folks to get a specific behaviour might be > > something to look into. > > > > > > > > > > > > > > > The newly introduced CameraConfiguration::orientation replaces the > > > > > existing CameraConfiguration::tranform, and it is meant for application to > > > > > express how they would like the images to be oriented, not to tell libcamera > > > > > what to do. As an example, passing in 'rotation0' means that the application > > > > > expects the images to be rotated upright, and doesn't tell libcamera not to > > > > > apply any rotation like passing in "Transform::Identity" did. > > > > > > > > My understanding is that passing "Transform::Identity" has always > > > > meant that the application wants its images "upright". (Are there > > > > cases where this doesn't happen?) > > > > > > > > > > As we have auto-correction in place, yes. > > > > My understanding is that everything got "auto-corrected" before. I'm > > still struggling to understand how it seems to be broken now. > > > > It is not 'broken'. Simply nobody got how thing worked in practice and > the existing behaviour (when it comes to adjusting ::transform) is not > what most users expects. See below > > > > > > > > > > > > > > The value CameraConfiguration::orientation is set to after a validate() also > > > > > differs in meaning, as instead of reporting "what applications have to do > > > > > to obtain what they originally asked for" it simply reports the actual > > > > > orientation of the stream: this means that if libcamera cannot fully satisfy the > > > > > user request it will set ::orientation to report the native images rotation > > > > > and the CameraConfiguration::status will be set to Adjusted. > > > > > > > > I believe this to be the current behaviour of the transform field. > > > > > > > > > > Not really, see below > > > > > > > > > > > > > Handling of 90 and 270 degrees rotation has also changed: as the camera sensor > > > > > cannot correct rotations that include a transposition, requests for a 90/270 > > > > > degrees are ignored, and cameras with a mounting rotation of 90/270 degrees > > > > > are never re-oriented. This makes it clear and less confusing for applications > > > > > that they have to deal with correction fully by themselves. As an example, with > > > > > the current implementation if the application requires a Rot270 (HFlip + > > > > > Transpose) libcamera will do the HFlip and leave transposition to the upper > > > > > layers. There is no clear advantage in doing so, and display compositors are > > > > > better suited for handling transpositions and flipping in a single pass instead > > > > > of having the library try to handle part of that. > > > > > > > > Again, I'm not really understanding this. > > > > > > > > The current behaviour is such that, if it can't give you the > > > > transformation that you ask for (on account of not being able to do > > > > transposition), then it guarantees to give you what you requested > > > > _apart_ from the transposition. So the application only has one case > > > > to deal with - applying a transpose. It doesn't have to worry about > > > > the other 3 "transforms-that-I-can't-handle", thereby making life > > > > easier for the application. > > > > > > The _apart_ is the key here. > > > > > > If you currently ask for Rot270 the library will do an HFlip and set > > > transform to Transpose (ie what applications still have to do to > > > obtain Rot270). This is not how the other fields of > > > > What it _should_ do is set the transform field to the actual transform > > that the user will perceive the images to have. I believe this to be > > "proper" libcamera configuration behaviour. I'm not understanding why > > things are different now. > > > > Let's start with the fact that "transpose" is a concept that is > introduced in Transform and it's not part of the EXIF standard which > gstreamer/pipewire expect to use. > > Again, you set transform = rot270, libcamera does HFLIP and you have > to do Transpose, correct ? That's one option. Or, if you want to do a "rot270" in the application you just set the transform to "userTransform * -Transform::rot270". As I've said, I'm really happy to try and make this easier for people. > > For compositor this is more work that just do "rot270" > > > Beyond this, the current behaviour is to leave the application to do > > the transpose, but that's just a choice. The point of transforms is > > that it's easy to use them to produce something different if you want. > > I don't mind looking at that if we can make it easier for everyone to > > get their preferred behaviour, which I suggested in my original reply. > > > > I don't get why we should be imposing other frameworks/apps to learn > Transpose when there is a standard to follow to be honest. > > > > CameraConfiguration work, and mostly, applying a flip and letting apps > > > only deal with transpose is an "optimization" that might actually make > > > life harder for apps, as display compositors usually have means to > > > deal with known rotations, while in this case you only ask them to do > > > a Transpose, which might require them more work ? > > > > > > In any case, I think it's saner if the library cannot do what it has > > > been asked for to set ::transform to what the stream orientation > > > actually is, instead of telling apps what "they still have to do". > > > > I don't understand the repeated statement that the transform is > > telling apps "what they still have to do". That was never what it > > meant, unless it got changed somewhere. > > > > Isn't it ? I'm not repeating the Rot270 example, but applications are > left with ::transform = Transpose. Isn't this what they have to do to > obtain Rot270 once HFlip has been done by libcamera ? > > So they have to "I asked for Rot270, I got back Transpose. Ah! > libcamera did HFlip for me, so now I have to do Transpose." > > Is it what happens ? The code that I am currently running returns HFlip as the transform when I ask for Rot270, which is indeed what the application will get. In this case yes, I still have to do a transpose. But if I had, for example, code that does a 90 degree rotation, I'd ask for "Rot270 * -Rot90" instead. The transform field would come back as HVFlip telling me that my images will be upside down, and all I have to do is call my rotate 90 function. The idea is that it should be simple to get whatever behaviour you want, but I'm very open to trying to make this easier. > > > > > > > > > > > > Because transforms are composable, it's easy to calculate what > > > > transform to ask for to get a different behaviour (for example, do > > > > nothing at all if there's a transpose involved). I could imagine a > > > > slightly different API might be helpful, perhaps where you get to say > > > > not only "what you want", but also what you want the "residual > > > > transform" to be (being the one the application has to apply after) if > > > > it can't deliver what you wanted. Would that be useful? > > > > > > > > > > I think it's better to leave the stream orientation unmodified if it > > > cannot be corrected. This matches the behaviour of the other > > > CameraConfiguration fields, and seems easier to deal with from > > > applications. > > > > I feel we ought to be able to come up with something that makes it > > convenient for everyone to get what they want. > > > > > > > > > I also recall that there was some discussion a while back about the > > > > precise orientation of the rotations which could affect some of the > > > > details, but I don't think it was ever concluded. I don't know if > > > > that's causing any problems? > > > > > > Sorry, I don't remember such discussion. Any link ? > > > > I think the last email I sent on the subject was this one, after which > > things went quiet: > > https://lists.libcamera.org/pipermail/libcamera-devel/2023-January/036445.html > > Actually it seems to echo quite a bit of what I say above. I think one of the sticking points for me is that I don't really understand what the problems are. Perhaps we could revisit some of this so that I can be more helpful? Thanks David > > > > Thanks > > David > > > > > > > > > > > > > > > > > > > This series clearly breaks the application API as it removes a member from > > > > > CameraConfiguration, so it should be introduced probably only when a new release > > > > > is cut. > > > > > > > > I can't comment on this as I haven't really understood the change > > > > here. Obviously we have many end users for whom an API change would be > > > > unwelcome, but I suppose the change could be hidden from them. > > > > > > > > I apologise if there have been some discussions flying around that I > > > > haven't followed sufficiently. I'm afraid I don't normally study > > > > gstreamer related posts in detail, perhaps I need to try harder... > > > > > > > > Thanks > > > > David > > > > > > > > > > > > > > Tested on RPi imx219 with the newly introduced cam option, flip, mirror and > > > > > rotation work as expected. > > > > > > > > > > Tested on Pinephone Pro, where sensors are 90/270 degrees rotated (libcamera > > > > > performs no correction) > > > > > > > > > > Comments welcome of course. > > > > > > > > > > Thanks > > > > > j > > > > > > > > > > Jacopo Mondi (4): > > > > > libcamera: camera_sensor: Cache rotationTransform_ > > > > > libcamera: camera: Introduce CameraConfiguration::orientation > > > > > libcamera: Move to use CameraConfiguration::orientation > > > > > apsp: cam: Add option to set stream orientation > > > > > > > > > > Documentation/Doxyfile.in | 2 + > > > > > Documentation/rotation/flip-rotate-0.eps | 170 +++++++++++++++ > > > > > Documentation/rotation/flip-rotate-0.png | Bin 0 -> 16488 bytes > > > > > Documentation/rotation/flip-rotate-180.eps | 191 +++++++++++++++++ > > > > > Documentation/rotation/flip-rotate-180.png | Bin 0 -> 22198 bytes > > > > > Documentation/rotation/flip-rotate-270.eps | 197 ++++++++++++++++++ > > > > > Documentation/rotation/flip-rotate-270.png | Bin 0 -> 21367 bytes > > > > > Documentation/rotation/flip-rotate-90.eps | 195 +++++++++++++++++ > > > > > Documentation/rotation/flip-rotate-90.png | Bin 0 -> 21020 bytes > > > > > Documentation/rotation/rotate-0.eps | 169 +++++++++++++++ > > > > > Documentation/rotation/rotate-0.png | Bin 0 -> 9086 bytes > > > > > Documentation/rotation/rotate-180.eps | 189 +++++++++++++++++ > > > > > Documentation/rotation/rotate-180.png | Bin 0 -> 22182 bytes > > > > > Documentation/rotation/rotate-270.eps | 196 +++++++++++++++++ > > > > > Documentation/rotation/rotate-270.png | Bin 0 -> 21016 bytes > > > > > Documentation/rotation/rotate-90.eps | 196 +++++++++++++++++ > > > > > Documentation/rotation/rotate-90.png | Bin 0 -> 21528 bytes > > > > > include/libcamera/camera.h | 14 +- > > > > > include/libcamera/internal/camera_sensor.h | 4 +- > > > > > include/libcamera/transform.h | 4 + > > > > > src/apps/cam/camera_session.cpp | 17 ++ > > > > > src/apps/cam/main.cpp | 5 + > > > > > src/apps/cam/main.h | 1 + > > > > > src/libcamera/camera.cpp | 54 ++++- > > > > > src/libcamera/camera_sensor.cpp | 137 ++++++------ > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +- > > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +- > > > > > .../pipeline/rpi/common/pipeline_base.cpp | 9 +- > > > > > src/libcamera/pipeline/simple/simple.cpp | 6 +- > > > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +- > > > > > src/libcamera/pipeline/vimc/vimc.cpp | 4 +- > > > > > src/libcamera/transform.cpp | 58 ++++++ > > > > > 32 files changed, 1739 insertions(+), 97 deletions(-) > > > > > create mode 100644 Documentation/rotation/flip-rotate-0.eps > > > > > create mode 100644 Documentation/rotation/flip-rotate-0.png > > > > > create mode 100644 Documentation/rotation/flip-rotate-180.eps > > > > > create mode 100644 Documentation/rotation/flip-rotate-180.png > > > > > create mode 100644 Documentation/rotation/flip-rotate-270.eps > > > > > create mode 100644 Documentation/rotation/flip-rotate-270.png > > > > > create mode 100644 Documentation/rotation/flip-rotate-90.eps > > > > > create mode 100644 Documentation/rotation/flip-rotate-90.png > > > > > create mode 100644 Documentation/rotation/rotate-0.eps > > > > > create mode 100644 Documentation/rotation/rotate-0.png > > > > > create mode 100644 Documentation/rotation/rotate-180.eps > > > > > create mode 100644 Documentation/rotation/rotate-180.png > > > > > create mode 100644 Documentation/rotation/rotate-270.eps > > > > > create mode 100644 Documentation/rotation/rotate-270.png > > > > > create mode 100644 Documentation/rotation/rotate-90.eps > > > > > create mode 100644 Documentation/rotation/rotate-90.png > > > > > > > > > > -- > > > > > 2.40.1 > > > > >
Hi David let me summarize, as usual the discussion become hard to follow. On Wed, Jun 21, 2023 at 03:14:28PM +0100, David Plowman wrote: [snip] > > > > > > I think the last email I sent on the subject was this one, after which > > > things went quiet: > > > https://lists.libcamera.org/pipermail/libcamera-devel/2023-January/036445.html > > > Actually it seems to echo quite a bit of what I say above. > > I think one of the sticking points for me is that I don't really > understand what the problems are. Perhaps we could revisit some of > this so that I can be more helpful? > 1) Transform is a libcamera concept, other frameworks have to learn how to use it. A specification exists and it's wildely adopted, why should we diverge from it ? Because there is code already using the current implementation ? 2) The library applies partial transforms (like "I'll do HFlip if I'm asked Rot270 and I can't do Transpose"). This is confusing as applications have to compose transforms and do computations. This patch simplifies it: either you can do it in full, or leave the stream un-touched. There are no clear advantages that I can think of in partially applying transforms. 3) properties::Rotation, the documentation does not report 'mounting rotation' but I admit we (I myself for one) have often used that term. That's because, originally, my understanding was that no autocorrection should have been performed. Since it has been introduced (by copying your PH implementation) and others liked it, now properties::Rotation reports the stream rotation when no ::transform is specified. I think it's confusing to have properties::Rotation=180 and a stream oriented correctly, applications might wonder what's happening and if libcamera is doing something behind their back. I think these are the main discussion points, aren't they ? > Thanks > David > > > > > > > Thanks > > > David > > > > > > > > > > > > > > > > > > > > > > > > This series clearly breaks the application API as it removes a member from > > > > > > CameraConfiguration, so it should be introduced probably only when a new release > > > > > > is cut. > > > > > > > > > > I can't comment on this as I haven't really understood the change > > > > > here. Obviously we have many end users for whom an API change would be > > > > > unwelcome, but I suppose the change could be hidden from them. > > > > > > > > > > I apologise if there have been some discussions flying around that I > > > > > haven't followed sufficiently. I'm afraid I don't normally study > > > > > gstreamer related posts in detail, perhaps I need to try harder... > > > > > > > > > > Thanks > > > > > David > > > > > > > > > > > > > > > > > Tested on RPi imx219 with the newly introduced cam option, flip, mirror and > > > > > > rotation work as expected. > > > > > > > > > > > > Tested on Pinephone Pro, where sensors are 90/270 degrees rotated (libcamera > > > > > > performs no correction) > > > > > > > > > > > > Comments welcome of course. > > > > > > > > > > > > Thanks > > > > > > j > > > > > > > > > > > > Jacopo Mondi (4): > > > > > > libcamera: camera_sensor: Cache rotationTransform_ > > > > > > libcamera: camera: Introduce CameraConfiguration::orientation > > > > > > libcamera: Move to use CameraConfiguration::orientation > > > > > > apsp: cam: Add option to set stream orientation > > > > > > > > > > > > Documentation/Doxyfile.in | 2 + > > > > > > Documentation/rotation/flip-rotate-0.eps | 170 +++++++++++++++ > > > > > > Documentation/rotation/flip-rotate-0.png | Bin 0 -> 16488 bytes > > > > > > Documentation/rotation/flip-rotate-180.eps | 191 +++++++++++++++++ > > > > > > Documentation/rotation/flip-rotate-180.png | Bin 0 -> 22198 bytes > > > > > > Documentation/rotation/flip-rotate-270.eps | 197 ++++++++++++++++++ > > > > > > Documentation/rotation/flip-rotate-270.png | Bin 0 -> 21367 bytes > > > > > > Documentation/rotation/flip-rotate-90.eps | 195 +++++++++++++++++ > > > > > > Documentation/rotation/flip-rotate-90.png | Bin 0 -> 21020 bytes > > > > > > Documentation/rotation/rotate-0.eps | 169 +++++++++++++++ > > > > > > Documentation/rotation/rotate-0.png | Bin 0 -> 9086 bytes > > > > > > Documentation/rotation/rotate-180.eps | 189 +++++++++++++++++ > > > > > > Documentation/rotation/rotate-180.png | Bin 0 -> 22182 bytes > > > > > > Documentation/rotation/rotate-270.eps | 196 +++++++++++++++++ > > > > > > Documentation/rotation/rotate-270.png | Bin 0 -> 21016 bytes > > > > > > Documentation/rotation/rotate-90.eps | 196 +++++++++++++++++ > > > > > > Documentation/rotation/rotate-90.png | Bin 0 -> 21528 bytes > > > > > > include/libcamera/camera.h | 14 +- > > > > > > include/libcamera/internal/camera_sensor.h | 4 +- > > > > > > include/libcamera/transform.h | 4 + > > > > > > src/apps/cam/camera_session.cpp | 17 ++ > > > > > > src/apps/cam/main.cpp | 5 + > > > > > > src/apps/cam/main.h | 1 + > > > > > > src/libcamera/camera.cpp | 54 ++++- > > > > > > src/libcamera/camera_sensor.cpp | 137 ++++++------ > > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +- > > > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +- > > > > > > .../pipeline/rpi/common/pipeline_base.cpp | 9 +- > > > > > > src/libcamera/pipeline/simple/simple.cpp | 6 +- > > > > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +- > > > > > > src/libcamera/pipeline/vimc/vimc.cpp | 4 +- > > > > > > src/libcamera/transform.cpp | 58 ++++++ > > > > > > 32 files changed, 1739 insertions(+), 97 deletions(-) > > > > > > create mode 100644 Documentation/rotation/flip-rotate-0.eps > > > > > > create mode 100644 Documentation/rotation/flip-rotate-0.png > > > > > > create mode 100644 Documentation/rotation/flip-rotate-180.eps > > > > > > create mode 100644 Documentation/rotation/flip-rotate-180.png > > > > > > create mode 100644 Documentation/rotation/flip-rotate-270.eps > > > > > > create mode 100644 Documentation/rotation/flip-rotate-270.png > > > > > > create mode 100644 Documentation/rotation/flip-rotate-90.eps > > > > > > create mode 100644 Documentation/rotation/flip-rotate-90.png > > > > > > create mode 100644 Documentation/rotation/rotate-0.eps > > > > > > create mode 100644 Documentation/rotation/rotate-0.png > > > > > > create mode 100644 Documentation/rotation/rotate-180.eps > > > > > > create mode 100644 Documentation/rotation/rotate-180.png > > > > > > create mode 100644 Documentation/rotation/rotate-270.eps > > > > > > create mode 100644 Documentation/rotation/rotate-270.png > > > > > > create mode 100644 Documentation/rotation/rotate-90.eps > > > > > > create mode 100644 Documentation/rotation/rotate-90.png > > > > > > > > > > > > -- > > > > > > 2.40.1 > > > > > >
Hi again On Wed, 21 Jun 2023 at 15:30, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi David > let me summarize, as usual the discussion become hard to follow. > > On Wed, Jun 21, 2023 at 03:14:28PM +0100, David Plowman wrote: > > [snip] > > > > > > > > > I think the last email I sent on the subject was this one, after which > > > > things went quiet: > > > > https://lists.libcamera.org/pipermail/libcamera-devel/2023-January/036445.html > > > > Actually it seems to echo quite a bit of what I say above. > > > > I think one of the sticking points for me is that I don't really > > understand what the problems are. Perhaps we could revisit some of > > this so that I can be more helpful? > > > > 1) Transform is a libcamera concept, other frameworks have to learn > how to use it. A specification exists and it's wildely adopted, why > should we diverge from it ? Because there is code already using the > current implementation ? The orientation flag is completely identical to the transform, just with different names, so I don't feel this should be an area of difficulty. We can change the names, or add synonyms, I don't really mind. But the transform does have useful compose and inverse functions which I would not want to lose. This code is in quite wide use in the Pi community, so I do generally want to inconvenience users as little as possible. > > 2) The library applies partial transforms (like "I'll do HFlip if I'm > asked Rot270 and I can't do Transpose"). This is confusing as > applications have to compose transforms and do computations. This > patch simplifies it: either you can do it in full, or leave the stream > un-touched. There are no clear advantages that I can think of in > partially applying transforms. I like the partial transforms because it means I have only one "exception" case to deal with, and not four. In particular, I can control what the "left over" transform will be, if I want to. Though of course, I don't have to. I think it would be OK to change the default behaviour to what you have described (i.e. do nothing at all if it can't do the whole transform), so long as I can still generate the transform that I want, or my users are able to. > > 3) properties::Rotation, the documentation does not report 'mounting > rotation' but I admit we (I myself for one) have often used that term. > That's because, originally, my understanding was that no > autocorrection should have been performed. Since it has been > introduced (by copying your PH implementation) and others liked it, > now properties::Rotation reports the stream rotation when no > ::transform is specified. I think it's confusing to have > properties::Rotation=180 and a stream oriented correctly, applications > might wonder what's happening and if libcamera is doing something > behind their back. I don't mind changing the meaning of the Rotation property, but I think we should also be fixing up the existing code so that its behaviour doesn't change. It would also be nice if you could query the mounting rotation from an application, though I would agree an application should never need to use it. > > I think these are the main discussion points, aren't they ? I'd like to make a proposal, if I may. 1. I'm happy to change the Rotation property, but at the same time we need to fix the code so that the behaviour of transforms is not changed. I would still like there to be a property so that an application can query the mounting rotation if it wants to. 2. We change the default behaviour to apply no transform at all if it can't apply the full transform. (Applications that don't want this can easily sort themselves out now.) At this point I think everyone has the functionality that they want. Just to try and flesh out the application code a bit so that we are forced to understand it, we have the following. Applications that want the simple behaviour: Transform requestedTransform; // the transform the user wants Transform actualTransform; // what the user will get, from checking the transform field after validate() if (requestedTransform != actualTransform) /* The application will have to apply a transform equivalent to -actualTransform * requestedTransform */; Applications that want to control the transform that they apply (let's call this the "leftoverTransform"): if (requestedTransform != actualTransform) requestedTransform *= -leftoverTransform; // now just try again But maybe double-check if that all feels right!! 3. We can consider swapping the transform in the configuration for an "orientation" with a different set of names. We need to add transform/orientation conversion functions so that I (and my users) can still use it. How does that sound? David > > > Thanks > > David > > > > > > > > > > Thanks > > > > David > > > > > > > > > > > > > > > > > > > > > > > > > > > > > This series clearly breaks the application API as it removes a member from > > > > > > > CameraConfiguration, so it should be introduced probably only when a new release > > > > > > > is cut. > > > > > > > > > > > > I can't comment on this as I haven't really understood the change > > > > > > here. Obviously we have many end users for whom an API change would be > > > > > > unwelcome, but I suppose the change could be hidden from them. > > > > > > > > > > > > I apologise if there have been some discussions flying around that I > > > > > > haven't followed sufficiently. I'm afraid I don't normally study > > > > > > gstreamer related posts in detail, perhaps I need to try harder... > > > > > > > > > > > > Thanks > > > > > > David > > > > > > > > > > > > > > > > > > > > Tested on RPi imx219 with the newly introduced cam option, flip, mirror and > > > > > > > rotation work as expected. > > > > > > > > > > > > > > Tested on Pinephone Pro, where sensors are 90/270 degrees rotated (libcamera > > > > > > > performs no correction) > > > > > > > > > > > > > > Comments welcome of course. > > > > > > > > > > > > > > Thanks > > > > > > > j > > > > > > > > > > > > > > Jacopo Mondi (4): > > > > > > > libcamera: camera_sensor: Cache rotationTransform_ > > > > > > > libcamera: camera: Introduce CameraConfiguration::orientation > > > > > > > libcamera: Move to use CameraConfiguration::orientation > > > > > > > apsp: cam: Add option to set stream orientation > > > > > > > > > > > > > > Documentation/Doxyfile.in | 2 + > > > > > > > Documentation/rotation/flip-rotate-0.eps | 170 +++++++++++++++ > > > > > > > Documentation/rotation/flip-rotate-0.png | Bin 0 -> 16488 bytes > > > > > > > Documentation/rotation/flip-rotate-180.eps | 191 +++++++++++++++++ > > > > > > > Documentation/rotation/flip-rotate-180.png | Bin 0 -> 22198 bytes > > > > > > > Documentation/rotation/flip-rotate-270.eps | 197 ++++++++++++++++++ > > > > > > > Documentation/rotation/flip-rotate-270.png | Bin 0 -> 21367 bytes > > > > > > > Documentation/rotation/flip-rotate-90.eps | 195 +++++++++++++++++ > > > > > > > Documentation/rotation/flip-rotate-90.png | Bin 0 -> 21020 bytes > > > > > > > Documentation/rotation/rotate-0.eps | 169 +++++++++++++++ > > > > > > > Documentation/rotation/rotate-0.png | Bin 0 -> 9086 bytes > > > > > > > Documentation/rotation/rotate-180.eps | 189 +++++++++++++++++ > > > > > > > Documentation/rotation/rotate-180.png | Bin 0 -> 22182 bytes > > > > > > > Documentation/rotation/rotate-270.eps | 196 +++++++++++++++++ > > > > > > > Documentation/rotation/rotate-270.png | Bin 0 -> 21016 bytes > > > > > > > Documentation/rotation/rotate-90.eps | 196 +++++++++++++++++ > > > > > > > Documentation/rotation/rotate-90.png | Bin 0 -> 21528 bytes > > > > > > > include/libcamera/camera.h | 14 +- > > > > > > > include/libcamera/internal/camera_sensor.h | 4 +- > > > > > > > include/libcamera/transform.h | 4 + > > > > > > > src/apps/cam/camera_session.cpp | 17 ++ > > > > > > > src/apps/cam/main.cpp | 5 + > > > > > > > src/apps/cam/main.h | 1 + > > > > > > > src/libcamera/camera.cpp | 54 ++++- > > > > > > > src/libcamera/camera_sensor.cpp | 137 ++++++------ > > > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +- > > > > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +- > > > > > > > .../pipeline/rpi/common/pipeline_base.cpp | 9 +- > > > > > > > src/libcamera/pipeline/simple/simple.cpp | 6 +- > > > > > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +- > > > > > > > src/libcamera/pipeline/vimc/vimc.cpp | 4 +- > > > > > > > src/libcamera/transform.cpp | 58 ++++++ > > > > > > > 32 files changed, 1739 insertions(+), 97 deletions(-) > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-0.eps > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-0.png > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-180.eps > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-180.png > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-270.eps > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-270.png > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-90.eps > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-90.png > > > > > > > create mode 100644 Documentation/rotation/rotate-0.eps > > > > > > > create mode 100644 Documentation/rotation/rotate-0.png > > > > > > > create mode 100644 Documentation/rotation/rotate-180.eps > > > > > > > create mode 100644 Documentation/rotation/rotate-180.png > > > > > > > create mode 100644 Documentation/rotation/rotate-270.eps > > > > > > > create mode 100644 Documentation/rotation/rotate-270.png > > > > > > > create mode 100644 Documentation/rotation/rotate-90.eps > > > > > > > create mode 100644 Documentation/rotation/rotate-90.png > > > > > > > > > > > > > > -- > > > > > > > 2.40.1 > > > > > > >
Hi David On Wed, Jun 21, 2023 at 05:33:45PM +0100, David Plowman wrote: > Hi again > > On Wed, 21 Jun 2023 at 15:30, Jacopo Mondi > <jacopo.mondi@ideasonboard.com> wrote: > > > > Hi David > > let me summarize, as usual the discussion become hard to follow. > > > > On Wed, Jun 21, 2023 at 03:14:28PM +0100, David Plowman wrote: > > > > [snip] > > > > > > > > > > > > I think the last email I sent on the subject was this one, after which > > > > > things went quiet: > > > > > https://lists.libcamera.org/pipermail/libcamera-devel/2023-January/036445.html > > > > > Actually it seems to echo quite a bit of what I say above. > > > > > > I think one of the sticking points for me is that I don't really > > > understand what the problems are. Perhaps we could revisit some of > > > this so that I can be more helpful? > > > > > > > 1) Transform is a libcamera concept, other frameworks have to learn > > how to use it. A specification exists and it's wildely adopted, why > > should we diverge from it ? Because there is code already using the > > current implementation ? > > The orientation flag is completely identical to the transform, just > with different names, so I don't feel this should be an area of Why do you say it is identical ? The Orientation enumeration is defined based on the EXIF tag 274 'orientation' and describes 2D transforms in terms of clock-wise rotation and horizontal mirroring, while Transform is a custom-defined type which uses Transpose and V/HFlips. Why do you think they're identical ? The EXIF defined values are also used by gstreamer/gtk https://api.gtkd.org/gstreamer.c.types.TAG_IMAGE_ORIENTATION.html pipewire https://docs.pipewire.org/group__spa__buffer.html#gab4afeb1516aa33126757cfa275efd7a3 > difficulty. We can change the names, or add synonyms, I don't really > mind. But the transform does have useful compose and inverse functions > which I would not want to lose. > > This code is in quite wide use in the Pi community, so I do generally > want to inconvenience users as little as possible. > I understand this argument, but it can't be used as an excuse to never change the API, in particular if there is a standard already adopted by other frameworks we don't ahere to. > > > > 2) The library applies partial transforms (like "I'll do HFlip if I'm > > asked Rot270 and I can't do Transpose"). This is confusing as > > applications have to compose transforms and do computations. This > > patch simplifies it: either you can do it in full, or leave the stream > > un-touched. There are no clear advantages that I can think of in > > partially applying transforms. > > I like the partial transforms because it means I have only one > "exception" case to deal with, and not four. In particular, I can > control what the "left over" transform will be, if I want to. Though > of course, I don't have to. > I think what makes a difference here is the behaviour of the consumers of a "rotated" image. What is the "next" component that in most cases have to compensate for a non-fully corrected rotation ? I presume it generally is the display compositor or a framework like gstreamer that has elements like 'videoflip'. We know gtk/gstreamer internally uses the EXIF tag, but when it comes to compositor, why should for them be better to deal with "Transpose" instead that on Rot270 ? I'm no expert there but I presume rendering libraries have methods for quickly rotate images before rendering but they might not have "transpose only" ones. Also because "transpose along the diagonal axis from the top-left corner to the right-bottom one" is a concept I haven't found documented anywhere else if not in the Transform class (of course I might have missed it as I'm certainly no expert on rendering). > I think it would be OK to change the default behaviour to what you > have described (i.e. do nothing at all if it can't do the whole > transform), so long as I can still generate the transform that I want, > or my users are able to. > > > > > 3) properties::Rotation, the documentation does not report 'mounting > > rotation' but I admit we (I myself for one) have often used that term. > > That's because, originally, my understanding was that no > > autocorrection should have been performed. Since it has been > > introduced (by copying your PH implementation) and others liked it, > > now properties::Rotation reports the stream rotation when no > > ::transform is specified. I think it's confusing to have > > properties::Rotation=180 and a stream oriented correctly, applications > > might wonder what's happening and if libcamera is doing something > > behind their back. > > I don't mind changing the meaning of the Rotation property, but I > think we should also be fixing up the existing code so that its > behaviour doesn't change. It would also be nice if you could query the What existing code relies on Rotation being the mounting rotation ? In libcamera ? In the apps ? > mounting rotation from an application, though I would agree an > application should never need to use it. > > > > > I think these are the main discussion points, aren't they ? > > I'd like to make a proposal, if I may. > > 1. I'm happy to change the Rotation property, but at the same time we > need to fix the code so that the behaviour of transforms is not > changed. I would still like there to be a property so that an > application can query the mounting rotation if it wants to. I'm fine introducing one > 2. We change the default behaviour to apply no transform at all if it > can't apply the full transform. (Applications that don't want this can > easily sort themselves out now.) > > At this point I think everyone has the functionality that they want. > Just to try and flesh out the application code a bit so that we are > forced to understand it, we have the following. > > Applications that want the simple behaviour: > > Transform requestedTransform; // the transform the user wants > Transform actualTransform; // what the user will get, from checking > the transform field after validate() > > if (requestedTransform != actualTransform) > /* The application will have to apply a transform equivalent to > -actualTransform * requestedTransform */; > > Applications that want to control the transform that they apply (let's > call this the "leftoverTransform"): > > if (requestedTransform != actualTransform) > requestedTransform *= -leftoverTransform; // now just try again I still don't get why people should do any of this calculations. I don't expect anyone to write their own rotation libraries, but instead rely on the rendering libraries or on piping gstreamer elements to the capture output. Why should they calculate the "leftoverTransform" ? What is the use case for an application in doing this ? > > But maybe double-check if that all feels right!! I think it's relevant to know how gstreamer/pipewire expects to deal with transforms, and more importantly what 'transform' display compositors supports natively. > > 3. We can consider swapping the transform in the configuration for an > "orientation" with a different set of names. We need to add > transform/orientation conversion functions so that I (and my users) > can still use it. See the newly added functions in transform.cpp > > How does that sound? > > David > > > > > > Thanks > > > David > > > > > > > > > > > > > Thanks > > > > > David > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > This series clearly breaks the application API as it removes a member from > > > > > > > > CameraConfiguration, so it should be introduced probably only when a new release > > > > > > > > is cut. > > > > > > > > > > > > > > I can't comment on this as I haven't really understood the change > > > > > > > here. Obviously we have many end users for whom an API change would be > > > > > > > unwelcome, but I suppose the change could be hidden from them. > > > > > > > > > > > > > > I apologise if there have been some discussions flying around that I > > > > > > > haven't followed sufficiently. I'm afraid I don't normally study > > > > > > > gstreamer related posts in detail, perhaps I need to try harder... > > > > > > > > > > > > > > Thanks > > > > > > > David > > > > > > > > > > > > > > > > > > > > > > > Tested on RPi imx219 with the newly introduced cam option, flip, mirror and > > > > > > > > rotation work as expected. > > > > > > > > > > > > > > > > Tested on Pinephone Pro, where sensors are 90/270 degrees rotated (libcamera > > > > > > > > performs no correction) > > > > > > > > > > > > > > > > Comments welcome of course. > > > > > > > > > > > > > > > > Thanks > > > > > > > > j > > > > > > > > > > > > > > > > Jacopo Mondi (4): > > > > > > > > libcamera: camera_sensor: Cache rotationTransform_ > > > > > > > > libcamera: camera: Introduce CameraConfiguration::orientation > > > > > > > > libcamera: Move to use CameraConfiguration::orientation > > > > > > > > apsp: cam: Add option to set stream orientation > > > > > > > > > > > > > > > > Documentation/Doxyfile.in | 2 + > > > > > > > > Documentation/rotation/flip-rotate-0.eps | 170 +++++++++++++++ > > > > > > > > Documentation/rotation/flip-rotate-0.png | Bin 0 -> 16488 bytes > > > > > > > > Documentation/rotation/flip-rotate-180.eps | 191 +++++++++++++++++ > > > > > > > > Documentation/rotation/flip-rotate-180.png | Bin 0 -> 22198 bytes > > > > > > > > Documentation/rotation/flip-rotate-270.eps | 197 ++++++++++++++++++ > > > > > > > > Documentation/rotation/flip-rotate-270.png | Bin 0 -> 21367 bytes > > > > > > > > Documentation/rotation/flip-rotate-90.eps | 195 +++++++++++++++++ > > > > > > > > Documentation/rotation/flip-rotate-90.png | Bin 0 -> 21020 bytes > > > > > > > > Documentation/rotation/rotate-0.eps | 169 +++++++++++++++ > > > > > > > > Documentation/rotation/rotate-0.png | Bin 0 -> 9086 bytes > > > > > > > > Documentation/rotation/rotate-180.eps | 189 +++++++++++++++++ > > > > > > > > Documentation/rotation/rotate-180.png | Bin 0 -> 22182 bytes > > > > > > > > Documentation/rotation/rotate-270.eps | 196 +++++++++++++++++ > > > > > > > > Documentation/rotation/rotate-270.png | Bin 0 -> 21016 bytes > > > > > > > > Documentation/rotation/rotate-90.eps | 196 +++++++++++++++++ > > > > > > > > Documentation/rotation/rotate-90.png | Bin 0 -> 21528 bytes > > > > > > > > include/libcamera/camera.h | 14 +- > > > > > > > > include/libcamera/internal/camera_sensor.h | 4 +- > > > > > > > > include/libcamera/transform.h | 4 + > > > > > > > > src/apps/cam/camera_session.cpp | 17 ++ > > > > > > > > src/apps/cam/main.cpp | 5 + > > > > > > > > src/apps/cam/main.h | 1 + > > > > > > > > src/libcamera/camera.cpp | 54 ++++- > > > > > > > > src/libcamera/camera_sensor.cpp | 137 ++++++------ > > > > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +- > > > > > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +- > > > > > > > > .../pipeline/rpi/common/pipeline_base.cpp | 9 +- > > > > > > > > src/libcamera/pipeline/simple/simple.cpp | 6 +- > > > > > > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +- > > > > > > > > src/libcamera/pipeline/vimc/vimc.cpp | 4 +- > > > > > > > > src/libcamera/transform.cpp | 58 ++++++ > > > > > > > > 32 files changed, 1739 insertions(+), 97 deletions(-) > > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-0.eps > > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-0.png > > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-180.eps > > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-180.png > > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-270.eps > > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-270.png > > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-90.eps > > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-90.png > > > > > > > > create mode 100644 Documentation/rotation/rotate-0.eps > > > > > > > > create mode 100644 Documentation/rotation/rotate-0.png > > > > > > > > create mode 100644 Documentation/rotation/rotate-180.eps > > > > > > > > create mode 100644 Documentation/rotation/rotate-180.png > > > > > > > > create mode 100644 Documentation/rotation/rotate-270.eps > > > > > > > > create mode 100644 Documentation/rotation/rotate-270.png > > > > > > > > create mode 100644 Documentation/rotation/rotate-90.eps > > > > > > > > create mode 100644 Documentation/rotation/rotate-90.png > > > > > > > > > > > > > > > > -- > > > > > > > > 2.40.1 > > > > > > > >
HI Jacopo On Thu, 22 Jun 2023 at 09:08, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi David > > On Wed, Jun 21, 2023 at 05:33:45PM +0100, David Plowman wrote: > > Hi again > > > > On Wed, 21 Jun 2023 at 15:30, Jacopo Mondi > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > Hi David > > > let me summarize, as usual the discussion become hard to follow. > > > > > > On Wed, Jun 21, 2023 at 03:14:28PM +0100, David Plowman wrote: > > > > > > [snip] > > > > > > > > > > > > > > > I think the last email I sent on the subject was this one, after which > > > > > > things went quiet: > > > > > > https://lists.libcamera.org/pipermail/libcamera-devel/2023-January/036445.html > > > > > > Actually it seems to echo quite a bit of what I say above. > > > > > > > > I think one of the sticking points for me is that I don't really > > > > understand what the problems are. Perhaps we could revisit some of > > > > this so that I can be more helpful? > > > > > > > > > > 1) Transform is a libcamera concept, other frameworks have to learn > > > how to use it. A specification exists and it's wildely adopted, why > > > should we diverge from it ? Because there is code already using the > > > current implementation ? > > > > The orientation flag is completely identical to the transform, just > > with different names, so I don't feel this should be an area of > > Why do you say it is identical ? Sorry, I could have been clearer. The orientation flag is describing exactly the same 8 plane transformations as the transform, with exactly the same meanings. I believe the only difference is in the naming, the precise encoding in terms of numeric values (hope no one relies on those!), and the availability of extra functions to "manipulate" them (compose/invert etc.). I think different categories of users (maybe those who care about rendering images, and those who are interested in image analysis, for example) may have slightly different expectations for naming these things, so I definitely think there's some scope for re-examining those. > > The Orientation enumeration is defined based on the EXIF tag 274 > 'orientation' and describes 2D transforms in terms of clock-wise > rotation and horizontal mirroring, while Transform is a custom-defined > type which uses Transpose and V/HFlips. Why do you think they're > identical ? > > The EXIF defined values are also used by > > gstreamer/gtk > https://api.gtkd.org/gstreamer.c.types.TAG_IMAGE_ORIENTATION.html > > pipewire > https://docs.pipewire.org/group__spa__buffer.html#gab4afeb1516aa33126757cfa275efd7a3 > > > difficulty. We can change the names, or add synonyms, I don't really > > mind. But the transform does have useful compose and inverse functions > > which I would not want to lose. > > > > This code is in quite wide use in the Pi community, so I do generally > > want to inconvenience users as little as possible. > > > > I understand this argument, but it can't be used as an excuse to never > change the API, in particular if there is a standard already adopted > by other frameworks we don't ahere to. I agree. Generally speaking we try not to change things more than necessary, though there are many occasions where you have to. There are always different things to consider and a good balance needs to be found. > > > > > > > 2) The library applies partial transforms (like "I'll do HFlip if I'm > > > asked Rot270 and I can't do Transpose"). This is confusing as > > > applications have to compose transforms and do computations. This > > > patch simplifies it: either you can do it in full, or leave the stream > > > un-touched. There are no clear advantages that I can think of in > > > partially applying transforms. > > > > I like the partial transforms because it means I have only one > > "exception" case to deal with, and not four. In particular, I can > > control what the "left over" transform will be, if I want to. Though > > of course, I don't have to. > > > > I think what makes a difference here is the behaviour of the consumers > of a "rotated" image. > > What is the "next" component that in most cases have to compensate for > a non-fully corrected rotation ? I presume it generally is the display > compositor or a framework like gstreamer that has elements like > 'videoflip'. We know gtk/gstreamer internally uses the EXIF tag, but > when it comes to compositor, why should for them be better to deal > with "Transpose" instead that on Rot270 ? I'm no expert there but I > presume rendering libraries have methods for quickly rotate images > before rendering but they might not have "transpose only" ones. Also > because "transpose along the diagonal axis from the top-left corner to > the right-bottom one" is a concept I haven't found documented anywhere > else if not in the Transform class (of course I might have missed it > as I'm certainly no expert on rendering). My aim is to try and give everyone the functionality that they want. I wouldn't want us to think that rendering is the only use for images. We have many users who want to process images and analyse them in different ways. They may be running neural networks, doing filtering operations, combining images, all kinds of things. It's actually very common that they aren't rendering at all, and it's all about the downstream image processing. Rendering is very important, but there are other uses too, and it would be great to give them some flexibility as well. Even within rendering, I wonder what would happen if (for example) you had hardware that can do rot90 and rot270, but not transpose. What would you do if an application happened to ask for an orientation that left you needing to do a transpose? It would be nice if you could easily calculate the transform you need to ask for so that you're left with a rot90 instead. > > > I think it would be OK to change the default behaviour to what you > > have described (i.e. do nothing at all if it can't do the whole > > transform), so long as I can still generate the transform that I want, > > or my users are able to. > > > > > > > > 3) properties::Rotation, the documentation does not report 'mounting > > > rotation' but I admit we (I myself for one) have often used that term. > > > That's because, originally, my understanding was that no > > > autocorrection should have been performed. Since it has been > > > introduced (by copying your PH implementation) and others liked it, > > > now properties::Rotation reports the stream rotation when no > > > ::transform is specified. I think it's confusing to have > > > properties::Rotation=180 and a stream oriented correctly, applications > > > might wonder what's happening and if libcamera is doing something > > > behind their back. > > > > I don't mind changing the meaning of the Rotation property, but I > > think we should also be fixing up the existing code so that its > > behaviour doesn't change. It would also be nice if you could query the > > What existing code relies on Rotation being the mounting rotation ? In > libcamera ? In the apps ? I would agree that no one "needs" the mounting rotation, like they don't "need" the pixel cell size. But we normally like to be able to report everything that we know about a sensor, and applications may have a "list all the info about this sensor" feature. So it just seems like a helpful thing to do. > > > mounting rotation from an application, though I would agree an > > application should never need to use it. > > > > > > > > I think these are the main discussion points, aren't they ? > > > > I'd like to make a proposal, if I may. > > > > 1. I'm happy to change the Rotation property, but at the same time we > > need to fix the code so that the behaviour of transforms is not > > changed. I would still like there to be a property so that an > > application can query the mounting rotation if it wants to. > > I'm fine introducing one Great! > > > 2. We change the default behaviour to apply no transform at all if it > > can't apply the full transform. (Applications that don't want this can > > easily sort themselves out now.) I think this was the biggest concern, so I'm hoping this addresses it. > > > > At this point I think everyone has the functionality that they want. > > Just to try and flesh out the application code a bit so that we are > > forced to understand it, we have the following. > > > > Applications that want the simple behaviour: > > > > Transform requestedTransform; // the transform the user wants > > Transform actualTransform; // what the user will get, from checking > > the transform field after validate() > > > > if (requestedTransform != actualTransform) > > /* The application will have to apply a transform equivalent to > > -actualTransform * requestedTransform */; > > > > Applications that want to control the transform that they apply (let's > > call this the "leftoverTransform"): > > > > if (requestedTransform != actualTransform) > > requestedTransform *= -leftoverTransform; // now just try again > > I still don't get why people should do any of this calculations. I hope I've addressed that above. There are other use cases, and it would be great to give everyone the functionality they want. > > I don't expect anyone to write their own rotation libraries, but > instead rely on the rendering libraries or on piping gstreamer > elements to the capture output. Why should they calculate the > "leftoverTransform" ? What is the use case for an application in doing > this ? I hope that's covered above. I'd like everyone to have access to functionality they might want, whilst not impacting those who don't wish to use it. > > > > > But maybe double-check if that all feels right!! > > I think it's relevant to know how gstreamer/pipewire expects to > deal with transforms, and more importantly what 'transform' display > compositors supports natively. Absolutely, and I think my proposal covers this, I believe everyone gets to do exactly what they want. If there are still particular things that don't work, or are difficult, then we should definitely look into them. > > > > > 3. We can consider swapping the transform in the configuration for an > > "orientation" with a different set of names. We need to add > > transform/orientation conversion functions so that I (and my users) > > can still use it. > > See the newly added functions in transform.cpp That seems fine. Also very happy to discuss all this at a later date! Thanks! David > > > > > How does that sound? > > > > David > > > > > > > > > Thanks > > > > David > > > > > > > > > > > > > > > > Thanks > > > > > > David > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > This series clearly breaks the application API as it removes a member from > > > > > > > > > CameraConfiguration, so it should be introduced probably only when a new release > > > > > > > > > is cut. > > > > > > > > > > > > > > > > I can't comment on this as I haven't really understood the change > > > > > > > > here. Obviously we have many end users for whom an API change would be > > > > > > > > unwelcome, but I suppose the change could be hidden from them. > > > > > > > > > > > > > > > > I apologise if there have been some discussions flying around that I > > > > > > > > haven't followed sufficiently. I'm afraid I don't normally study > > > > > > > > gstreamer related posts in detail, perhaps I need to try harder... > > > > > > > > > > > > > > > > Thanks > > > > > > > > David > > > > > > > > > > > > > > > > > > > > > > > > > > Tested on RPi imx219 with the newly introduced cam option, flip, mirror and > > > > > > > > > rotation work as expected. > > > > > > > > > > > > > > > > > > Tested on Pinephone Pro, where sensors are 90/270 degrees rotated (libcamera > > > > > > > > > performs no correction) > > > > > > > > > > > > > > > > > > Comments welcome of course. > > > > > > > > > > > > > > > > > > Thanks > > > > > > > > > j > > > > > > > > > > > > > > > > > > Jacopo Mondi (4): > > > > > > > > > libcamera: camera_sensor: Cache rotationTransform_ > > > > > > > > > libcamera: camera: Introduce CameraConfiguration::orientation > > > > > > > > > libcamera: Move to use CameraConfiguration::orientation > > > > > > > > > apsp: cam: Add option to set stream orientation > > > > > > > > > > > > > > > > > > Documentation/Doxyfile.in | 2 + > > > > > > > > > Documentation/rotation/flip-rotate-0.eps | 170 +++++++++++++++ > > > > > > > > > Documentation/rotation/flip-rotate-0.png | Bin 0 -> 16488 bytes > > > > > > > > > Documentation/rotation/flip-rotate-180.eps | 191 +++++++++++++++++ > > > > > > > > > Documentation/rotation/flip-rotate-180.png | Bin 0 -> 22198 bytes > > > > > > > > > Documentation/rotation/flip-rotate-270.eps | 197 ++++++++++++++++++ > > > > > > > > > Documentation/rotation/flip-rotate-270.png | Bin 0 -> 21367 bytes > > > > > > > > > Documentation/rotation/flip-rotate-90.eps | 195 +++++++++++++++++ > > > > > > > > > Documentation/rotation/flip-rotate-90.png | Bin 0 -> 21020 bytes > > > > > > > > > Documentation/rotation/rotate-0.eps | 169 +++++++++++++++ > > > > > > > > > Documentation/rotation/rotate-0.png | Bin 0 -> 9086 bytes > > > > > > > > > Documentation/rotation/rotate-180.eps | 189 +++++++++++++++++ > > > > > > > > > Documentation/rotation/rotate-180.png | Bin 0 -> 22182 bytes > > > > > > > > > Documentation/rotation/rotate-270.eps | 196 +++++++++++++++++ > > > > > > > > > Documentation/rotation/rotate-270.png | Bin 0 -> 21016 bytes > > > > > > > > > Documentation/rotation/rotate-90.eps | 196 +++++++++++++++++ > > > > > > > > > Documentation/rotation/rotate-90.png | Bin 0 -> 21528 bytes > > > > > > > > > include/libcamera/camera.h | 14 +- > > > > > > > > > include/libcamera/internal/camera_sensor.h | 4 +- > > > > > > > > > include/libcamera/transform.h | 4 + > > > > > > > > > src/apps/cam/camera_session.cpp | 17 ++ > > > > > > > > > src/apps/cam/main.cpp | 5 + > > > > > > > > > src/apps/cam/main.h | 1 + > > > > > > > > > src/libcamera/camera.cpp | 54 ++++- > > > > > > > > > src/libcamera/camera_sensor.cpp | 137 ++++++------ > > > > > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +- > > > > > > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +- > > > > > > > > > .../pipeline/rpi/common/pipeline_base.cpp | 9 +- > > > > > > > > > src/libcamera/pipeline/simple/simple.cpp | 6 +- > > > > > > > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +- > > > > > > > > > src/libcamera/pipeline/vimc/vimc.cpp | 4 +- > > > > > > > > > src/libcamera/transform.cpp | 58 ++++++ > > > > > > > > > 32 files changed, 1739 insertions(+), 97 deletions(-) > > > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-0.eps > > > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-0.png > > > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-180.eps > > > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-180.png > > > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-270.eps > > > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-270.png > > > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-90.eps > > > > > > > > > create mode 100644 Documentation/rotation/flip-rotate-90.png > > > > > > > > > create mode 100644 Documentation/rotation/rotate-0.eps > > > > > > > > > create mode 100644 Documentation/rotation/rotate-0.png > > > > > > > > > create mode 100644 Documentation/rotation/rotate-180.eps > > > > > > > > > create mode 100644 Documentation/rotation/rotate-180.png > > > > > > > > > create mode 100644 Documentation/rotation/rotate-270.eps > > > > > > > > > create mode 100644 Documentation/rotation/rotate-270.png > > > > > > > > > create mode 100644 Documentation/rotation/rotate-90.eps > > > > > > > > > create mode 100644 Documentation/rotation/rotate-90.png > > > > > > > > > > > > > > > > > > -- > > > > > > > > > 2.40.1 > > > > > > > > >
Hello, I'm replying in the middle of the thread as the next e-mail trimmed part of the conversation. I'll reply to the second part just after. On Wed, Jun 21, 2023 at 03:14:28PM +0100, David Plowman via libcamera-devel wrote: > On Wed, 21 Jun 2023 at 13:57, Jacopo Mondi wrote: > > On Wed, Jun 21, 2023 at 01:37:11PM +0100, David Plowman wrote: > > > On Wed, 21 Jun 2023 at 10:23, Jacopo Mondi wrote: > > > > On Wed, Jun 21, 2023 at 10:08:25AM +0100, David Plowman wrote: > > > > > Hi Jacopo > > > > > > > > > > Thanks for sending this proposal. I wasn't sure I really understood > > > > > it, perhaps you can help me out! > > > > > > > > > > On Tue, 20 Jun 2023 at 15:29, Jacopo Mondi wrote: > > > > > > > > > > > > Hello everyone > > > > > > > > > > > > We have been discussing rotation/transformations for a long time, and > > > > > > the currently implemented CameraConfiguration::transform behaviour has > > > > > > proven to be confusing for several reasons: it was poorly documented, > > > > > > translating it to something consumable for upper layer frameworks was > > > > > > difficult and it behaved differently than any other field part of the > > > > > > CameraConfiguration, as it was meant to tell libcamera "what to do" instead > > > > > > of expressing what applications want. > > > > > The last point is slightly debatable. I can't tell if this is right, as it depends how one interprets the confusing documentation :-) Maybe this is the source of the disagreement. What is absolutely clear is that the original intent, the documentation and the implementation don't align properly. Whether it is the documentation or the code (or both) that we need to fix is what we're discussing here. I also agree with Jacopo that consuming the API in upper layers is currently problematic. > > > > > As far as I understand it, I don't believe this is correct, and the > > > > > transform is where you say what you want. > > > > > > > > > > For example, on a Raspberry Pi I always set the transform to > > > > > Transform::Identity because I want the images "the right way up". (I > > > > > know that internally, it will calculate and apply a 180 rotation to > > > > > counteract the sensor mounting rotation.) Does that sound right? The documentation states * The transform is a user-specified 2D plane transform that will be applied * to the camera images by the processing pipeline before being handed to * the application. This is subsequent to any transform that is already * required to fix up any platform-defined rotation. I can't interpret the first sentence as anything else than a transformation applied on the image, I don't see how it can be construed as meaning the desired orientation of the image at the output of the pipeline. Of course, for 180° rotations, the two are equivalent. The fact that 90° and 270° support has been implemented in CameraSensor::validateTransform() but not tested (due to lack of hardware support) only contributes to additional confusion as what the original intent was. The second sentence sounds very confusing to me. > > > > > > > > Yes, now that we correct properties::Rotation to 0 as the library > > > > compensate the mounting rotation behind your back, that's what you > > > > get. > > > > > > > > Before we had properties::Rotation = 180; transform = Identity and you > > > > got an upright image. Confusing, isn't it ? > > > > > > I understood the previous arrangement, but the new one seems > > > counter-intuitive to me. > > > > > > On a Raspberry Pi, I expect the properties::Rotation to report 180 > > > degrees because the sensor is mounted upside down. Then I can ask for > > > Transform::Identity and internally a 180 degree rotation will be > > > applied to give me what I asked for. I understand this. > > > > > > Are we saying that now the rotation will report 0 degrees? Will it now > > > always report 0 degrees, whatever the sensor and platform? That would > > > obviously break things... > > > > So you're an app, you read properties::Rotation and you get back 180. > > Then you star streaming and you get a stream oriented "correctly". > > > > Isn't this wrong ? > > I think a lot of this hinges on what the Rotation property is. > > It _used_ to be the mounting rotation of the sensor (is that right?), > at which point everything was as I have described it. But I think it > is being changed now, which is indeed something that was under > discussion in that thread back in January. We're free to do that, but > perhaps there have been some unintended consequences? This stems from the fact that you wanted (if I recall correctly) the platform to auto-correct the mounting rotation when possible. Exposing to the user the fact that the sensor is mounted upside-down, *and* auto-correcting this when possible, isn't a behaviour I like. There are too many implicit assumptions, it's just confusing. If you want auto-correction, it needs to be completely transparent, libcamera needs to hide the fact that the sensor is mounted upside-down from the application in that case. I want the API to be clear and simple when no auto-correction is applied. I'm OK with auto-correction being added on top if it's totally transparent for applications. This means that auto-correction shouldn't even need to be mentioned in the API specification, as applications shouldn't have to care. > > > > > If the problem is actually to do with documentation, I would always > > > > > agree that it can be improved, and am very happy to help! Documentation surely needs to be improved, regardless of what we decide to do with the code. > > > > > > For these reasons this series proposes to replace the usage to Transform > > > > > > in the public API in favour of a new CameraConfiguration::Orientation type, > > > > > > defined based on the EXIF specification, tag 274, 'orientation'. For reference > > > > > > this is the same as implemented by gstreamer: > > > > > > https://gstreamer.freedesktop.org/documentation/gstreamer/gsttaglist.html?gi-language=c#GST_TAG_IMAGE_ORIENTATION > > > > > > > > > > I must confess to not liking the term "orientation". There is such a > > > > > thing as a "2D plane transformation" and I'd expect to refer to them > > > > > as transformations or maybe transforms for short. I don't understand > > > > > what an "orientation" is without reading extra documentation. Are > > > > > "orientations" composable? Is there such a thing as an "inverse > > > > > orientation"? "Orientation" is a term widely used in other frameworks and specifications, including EXIF, and GStreamer. While I don't rule out finding a better name, it doesn't seem bad to me. "Transform" has a strong implication that it specifies what transformation is applied to the image, not what orientation the image has at the output of the pipeline. If you prefer the "transform" name, then the meaning of the parameter has to match the name. > > > > I'm fine bikeshedding on the name. What matters to me is that now we > > > > conform to an existing specification adopeted by most frameworks > > > > instead of defining our own Transform. > > > > > > I'd like to find something that works for all libcamera users. > > > > > > The Transform class allows composition, inverse operations and so on, > > > the idea being that users can calculate for themselves what to ask for > > > if the default behaviour isn't what they want. I did comment that > > > making it easier for folks to get a specific behaviour might be > > > something to look into. Helpers exposed by the libcamera core API need to have a purpose for libcamera. If the only purpose of a Transform class, as used by applications, is to compose transformations to calculate how to configure a compositor based on the orientation of the image produced by libcamera, then that helper doesn't belong to the libcamera core API. I don't mind having it in a conveniency utility library though. I don't want to digress too much, but some context will be useful to understand my point here. With the need to support more languages in the future, as well as the ABI stability requirements, I expect we will strip down the core libcamera API from helpers. The core API will likely be specified using some form of IDL, and the ABI will be plain C. The language bindings, including C++ bindings, will be implemented (likely auto-generated from the API IDL) based on the C ABI. Utilities that are not part of the core API will be implemented directly in the language bindings. For instance, we will retain geometry structures to represent points, sizes and rectangles, but the core API will not include the geometry helpers. The existing geometry helpers will become internal to libcamera (as I expect the libcamera internal implementation to remain in C++, and the helpers have shown to be quite useful). A separate version of the helpers will also likely be provided by the C++ bindings and other language bindings. If any particular language has native classes to represent the geometry primitives, then we will likely use those. I don't want to discuss this plan in details as part of this mail thread, but let's keep it in mind for the libcamera core API design. > > > > > > The newly introduced CameraConfiguration::orientation replaces the > > > > > > existing CameraConfiguration::tranform, and it is meant for application to > > > > > > express how they would like the images to be oriented, not to tell libcamera > > > > > > what to do. As an example, passing in 'rotation0' means that the application > > > > > > expects the images to be rotated upright, and doesn't tell libcamera not to > > > > > > apply any rotation like passing in "Transform::Identity" did. > > > > > > > > > > My understanding is that passing "Transform::Identity" has always > > > > > meant that the application wants its images "upright". (Are there > > > > > cases where this doesn't happen?) My understanding, as noted above, is that passing "Transform::Identity" has always meant that the application wants the images in the orientation produced by the sensor (or, to be precise, the orientation of the pixel array, as the h/v flip are applied in the sensor too). If we want CameraConfiguration::transform to indicate what orientation the output image should have (and it seems we all agree on that), then it should be named differently. > > > > As we have auto-correction in place, yes. > > > > > > My understanding is that everything got "auto-corrected" before. I'm > > > still struggling to understand how it seems to be broken now. > > > > It is not 'broken'. Simply nobody got how thing worked in practice and > > the existing behaviour (when it comes to adjusting ::transform) is not > > what most users expects. See below > > > > > > > > The value CameraConfiguration::orientation is set to after a validate() also > > > > > > differs in meaning, as instead of reporting "what applications have to do > > > > > > to obtain what they originally asked for" it simply reports the actual > > > > > > orientation of the stream: this means that if libcamera cannot fully satisfy the > > > > > > user request it will set ::orientation to report the native images rotation > > > > > > and the CameraConfiguration::status will be set to Adjusted. > > > > > > > > > > I believe this to be the current behaviour of the transform field. > > > > > > > > Not really, see below > > > > > > > > > > Handling of 90 and 270 degrees rotation has also changed: as the camera sensor > > > > > > cannot correct rotations that include a transposition, requests for a 90/270 > > > > > > degrees are ignored, and cameras with a mounting rotation of 90/270 degrees > > > > > > are never re-oriented. This makes it clear and less confusing for applications > > > > > > that they have to deal with correction fully by themselves. As an example, with > > > > > > the current implementation if the application requires a Rot270 (HFlip + > > > > > > Transpose) libcamera will do the HFlip and leave transposition to the upper > > > > > > layers. There is no clear advantage in doing so, and display compositors are > > > > > > better suited for handling transpositions and flipping in a single pass instead > > > > > > of having the library try to handle part of that. > > > > > > > > > > Again, I'm not really understanding this. > > > > > > > > > > The current behaviour is such that, if it can't give you the > > > > > transformation that you ask for (on account of not being able to do > > > > > transposition), then it guarantees to give you what you requested > > > > > _apart_ from the transposition. So the application only has one case > > > > > to deal with - applying a transpose. It doesn't have to worry about > > > > > the other 3 "transforms-that-I-can't-handle", That only holds true if the camera can apply h/v flip. If it can't, the application will need to handle the 90° and 270° rotations, so generic applications will always have to support those cases in addition to transposition. It can thus be argued that having to support 90° rotation, 270° rotation and transposition is more complicated for the application than having to support rotations only. > > > > > thereby making life easier for the application. > > > > > > > > The _apart_ is the key here. > > > > > > > > If you currently ask for Rot270 the library will do an HFlip and set > > > > transform to Transpose (ie what applications still have to do to > > > > obtain Rot270). This is not how the other fields of > > > > > > What it _should_ do is set the transform field to the actual transform > > > that the user will perceive the images to have. I believe this to be > > > "proper" libcamera configuration behaviour. I'm not understanding why > > > things are different now. > > > > Let's start with the fact that "transpose" is a concept that is > > introduced in Transform and it's not part of the EXIF standard which > > gstreamer/pipewire expect to use. Note that "rotation" isn't either. EXIF supports the 8 combinations of h/v flip and tranpose, assigning them clear values, but doesn't use the "rotation", "transpose" or "flip" names to document them. I'm fine using those names in the libcamera API. > > Again, you set transform = rot270, libcamera does HFLIP and you have > > to do Transpose, correct ? > > That's one option. Or, if you want to do a "rot270" in the application > you just set the transform to "userTransform * -Transform::rot270". > > As I've said, I'm really happy to try and make this easier for people. > > > For compositor this is more work that just do "rot270" Do all display engines that support rotation at the hardware level be able to apply transposition without also flipping (that is, pure transposition that isn't a 90° or 270° rotation) ? Do all compositors that support rotation expose a pure transpose transformation in their API ? And the other way around, are there hardware devices or compositor APIs that can transpose but can't flip ? Unless I'm mistaken, there's no clear and universal answer to these questions. I thus don't want to assume what transformations the consumers of the images will be able to apply easily and efficiently. I'm missing the reason why libcamera should consider that producing an image that requires rotation by 90° in the application is better (or worse) than producing an image that requires transposing. If there's no option that is intrinsically better, then I prefer aiming for the simpler and easier to understand behaviour, which is to let the application deal with it. > > > Beyond this, the current behaviour is to leave the application to do > > > the transpose, but that's just a choice. The point of transforms is > > > that it's easy to use them to produce something different if you want. > > > I don't mind looking at that if we can make it easier for everyone to > > > get their preferred behaviour, which I suggested in my original reply. > > > > I don't get why we should be imposing other frameworks/apps to learn > > Transpose when there is a standard to follow to be honest. > > > > > > CameraConfiguration work, and mostly, applying a flip and letting apps > > > > only deal with transpose is an "optimization" that might actually make > > > > life harder for apps, as display compositors usually have means to > > > > deal with known rotations, while in this case you only ask them to do > > > > a Transpose, which might require them more work ? > > > > > > > > In any case, I think it's saner if the library cannot do what it has > > > > been asked for to set ::transform to what the stream orientation > > > > actually is, instead of telling apps what "they still have to do". > > > > > > I don't understand the repeated statement that the transform is > > > telling apps "what they still have to do". That was never what it > > > meant, unless it got changed somewhere. > > > > Isn't it ? I'm not repeating the Rot270 example, but applications are > > left with ::transform = Transpose. Isn't this what they have to do to > > obtain Rot270 once HFlip has been done by libcamera ? > > > > So they have to "I asked for Rot270, I got back Transpose. Ah! > > libcamera did HFlip for me, so now I have to do Transpose." > > > > Is it what happens ? > > The code that I am currently running returns HFlip as the transform > when I ask for Rot270, which is indeed what the application will get. > > In this case yes, I still have to do a transpose. But if I had, for > example, code that does a 90 degree rotation, I'd ask for "Rot270 * > -Rot90" instead. The transform field would come back as HVFlip telling > me that my images will be upside down, and all I have to do is call my > rotate 90 function. > > The idea is that it should be simple to get whatever behaviour you > want, but I'm very open to trying to make this easier. > > > > > > Because transforms are composable, it's easy to calculate what > > > > > transform to ask for to get a different behaviour (for example, do > > > > > nothing at all if there's a transpose involved). I could imagine a > > > > > slightly different API might be helpful, perhaps where you get to say > > > > > not only "what you want", but also what you want the "residual > > > > > transform" to be (being the one the application has to apply after) if > > > > > it can't deliver what you wanted. Would that be useful? That's too complex I think. If we want to extend the API (not merely change it), we could report the transformation capabilities of the camera as a property to let applications discover what can be done. It will then be the responsibility of the application to ask for something that is supported, and we won't have to design complex heuristics to find the "best" option when the applications asks for an unsupported transformation. The API will be simpler to document, and simpler to understand. > > > > I think it's better to leave the stream orientation unmodified if it > > > > cannot be corrected. This matches the behaviour of the other > > > > CameraConfiguration fields, and seems easier to deal with from > > > > applications. > > > > > > I feel we ought to be able to come up with something that makes it > > > convenient for everyone to get what they want. > > > > > > > > I also recall that there was some discussion a while back about the > > > > > precise orientation of the rotations which could affect some of the > > > > > details, but I don't think it was ever concluded. I don't know if > > > > > that's causing any problems? > > > > > > > > Sorry, I don't remember such discussion. Any link ? > > > > > > I think the last email I sent on the subject was this one, after which > > > things went quiet: > > > https://lists.libcamera.org/pipermail/libcamera-devel/2023-January/036445.html > > > Actually it seems to echo quite a bit of what I say above. > > I think one of the sticking points for me is that I don't really > understand what the problems are. Perhaps we could revisit some of > this so that I can be more helpful? > > > > > > > This series clearly breaks the application API as it removes a member from > > > > > > CameraConfiguration, so it should be introduced probably only when a new release > > > > > > is cut. The series can be merged when ready, whenever that will be. There's no need to tie it to a "release". > > > > > I can't comment on this as I haven't really understood the change > > > > > here. Obviously we have many end users for whom an API change would be > > > > > unwelcome, but I suppose the change could be hidden from them. I'm afraid we will have many API changes, much more invasive than this one, in the future before we stabilize the API. > > > > > I apologise if there have been some discussions flying around that I > > > > > haven't followed sufficiently. I'm afraid I don't normally study > > > > > gstreamer related posts in detail, perhaps I need to try harder... > > > > > > > > > > > Tested on RPi imx219 with the newly introduced cam option, flip, mirror and > > > > > > rotation work as expected. > > > > > > > > > > > > Tested on Pinephone Pro, where sensors are 90/270 degrees rotated (libcamera > > > > > > performs no correction) > > > > > > > > > > > > Comments welcome of course. > > > > > > > > > > > > Thanks > > > > > > j > > > > > > > > > > > > Jacopo Mondi (4): > > > > > > libcamera: camera_sensor: Cache rotationTransform_ > > > > > > libcamera: camera: Introduce CameraConfiguration::orientation > > > > > > libcamera: Move to use CameraConfiguration::orientation > > > > > > apsp: cam: Add option to set stream orientation > > > > > > > > > > > > Documentation/Doxyfile.in | 2 + > > > > > > Documentation/rotation/flip-rotate-0.eps | 170 +++++++++++++++ > > > > > > Documentation/rotation/flip-rotate-0.png | Bin 0 -> 16488 bytes > > > > > > Documentation/rotation/flip-rotate-180.eps | 191 +++++++++++++++++ > > > > > > Documentation/rotation/flip-rotate-180.png | Bin 0 -> 22198 bytes > > > > > > Documentation/rotation/flip-rotate-270.eps | 197 ++++++++++++++++++ > > > > > > Documentation/rotation/flip-rotate-270.png | Bin 0 -> 21367 bytes > > > > > > Documentation/rotation/flip-rotate-90.eps | 195 +++++++++++++++++ > > > > > > Documentation/rotation/flip-rotate-90.png | Bin 0 -> 21020 bytes > > > > > > Documentation/rotation/rotate-0.eps | 169 +++++++++++++++ > > > > > > Documentation/rotation/rotate-0.png | Bin 0 -> 9086 bytes > > > > > > Documentation/rotation/rotate-180.eps | 189 +++++++++++++++++ > > > > > > Documentation/rotation/rotate-180.png | Bin 0 -> 22182 bytes > > > > > > Documentation/rotation/rotate-270.eps | 196 +++++++++++++++++ > > > > > > Documentation/rotation/rotate-270.png | Bin 0 -> 21016 bytes > > > > > > Documentation/rotation/rotate-90.eps | 196 +++++++++++++++++ > > > > > > Documentation/rotation/rotate-90.png | Bin 0 -> 21528 bytes > > > > > > include/libcamera/camera.h | 14 +- > > > > > > include/libcamera/internal/camera_sensor.h | 4 +- > > > > > > include/libcamera/transform.h | 4 + > > > > > > src/apps/cam/camera_session.cpp | 17 ++ > > > > > > src/apps/cam/main.cpp | 5 + > > > > > > src/apps/cam/main.h | 1 + > > > > > > src/libcamera/camera.cpp | 54 ++++- > > > > > > src/libcamera/camera_sensor.cpp | 137 ++++++------ > > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +- > > > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +- > > > > > > .../pipeline/rpi/common/pipeline_base.cpp | 9 +- > > > > > > src/libcamera/pipeline/simple/simple.cpp | 6 +- > > > > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +- > > > > > > src/libcamera/pipeline/vimc/vimc.cpp | 4 +- > > > > > > src/libcamera/transform.cpp | 58 ++++++ > > > > > > 32 files changed, 1739 insertions(+), 97 deletions(-) > > > > > > create mode 100644 Documentation/rotation/flip-rotate-0.eps > > > > > > create mode 100644 Documentation/rotation/flip-rotate-0.png > > > > > > create mode 100644 Documentation/rotation/flip-rotate-180.eps > > > > > > create mode 100644 Documentation/rotation/flip-rotate-180.png > > > > > > create mode 100644 Documentation/rotation/flip-rotate-270.eps > > > > > > create mode 100644 Documentation/rotation/flip-rotate-270.png > > > > > > create mode 100644 Documentation/rotation/flip-rotate-90.eps > > > > > > create mode 100644 Documentation/rotation/flip-rotate-90.png > > > > > > create mode 100644 Documentation/rotation/rotate-0.eps > > > > > > create mode 100644 Documentation/rotation/rotate-0.png > > > > > > create mode 100644 Documentation/rotation/rotate-180.eps > > > > > > create mode 100644 Documentation/rotation/rotate-180.png > > > > > > create mode 100644 Documentation/rotation/rotate-270.eps > > > > > > create mode 100644 Documentation/rotation/rotate-270.png > > > > > > create mode 100644 Documentation/rotation/rotate-90.eps > > > > > > create mode 100644 Documentation/rotation/rotate-90.png
Hi everyone
Thanks for that message. I think it's very helpful to quote the source
documentation:
* The transform is a user-specified 2D plane transform that will be applied
* to the camera images by the processing pipeline before being handed to
* the application. This is subsequent to any transform that is already
* required to fix up any platform-defined rotation.
What this is trying to say, badly, is that the transform you request
is applied after ("subsequent to") fixing up the platform rotation. So
you do indeed get auto-correction. This is also the actual behaviour
of the code on the Pi, has been for, I don't know, nearly 2 years? At
this point, the earth does indeed get my official permission to
swallow me up for not writing something that was less prone to
misinterpretation,
But I think this does leave us in a position where large amounts of
this thread are a mixture of not very helpful or even plain confusing,
and with people's permission, I'd honestly like to start again, with
everyone on the same page.
You might have noticed that "orientations" were really bugging me as
to what they were, and how one could use them. I've managed to resolve
these problems in my own mind and think that there's a very simple
connection between "orientations" and "transforms" that I would be OK
to implement and use. I think our next attempt at this discussion
should start by being clear on our requirements, and then we can look
at how "orientations" and "transforms" can get us there.
(Rest of thread purposefully deleted... hope that's ok!)
Thanks!
David
Hi David, thanks for initiative to bring us all on the same page.
From what I understand, this series aims to do two things:
1.: Make the public API follow the EXIF specification
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.
2.: Clarifying the expected behavior and fixing existing inconsistencies.
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()`.
To give an example, consider the following two cases:
* A camera mounted 90 deg rotated, a config requesting
`Transform::Identity`
* No camera rotation, a config requesting `Transform::rot90`
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.
In the second case, the returned value will be `Transform::Identity` and
the user has to remember that it asked for something different.
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.
I personally like the proposed behavior - either you get the requested
result or you have to apply the combined transform yourself - a lot.
From what I understand there are only two open questions here:
1. Should the sensor do partial transforms?
2. Do we really want/need the new `Orientation` enum or could/should we
instead with `Transform` and just update/clarify the expected results?
Hope you agree that these are the issues to agree on.
Thanks everyone and cheers,
Robert
On 23.06.23 10:26, David Plowman via libcamera-devel wrote:
> Hi everyone
>
> Thanks for that message. I think it's very helpful to quote the source
> documentation:
>
> * The transform is a user-specified 2D plane transform that will be applied
> * to the camera images by the processing pipeline before being handed to
> * the application. This is subsequent to any transform that is already
> * required to fix up any platform-defined rotation.
>
> What this is trying to say, badly, is that the transform you request
> is applied after ("subsequent to") fixing up the platform rotation. So
> you do indeed get auto-correction. This is also the actual behaviour
> of the code on the Pi, has been for, I don't know, nearly 2 years? At
> this point, the earth does indeed get my official permission to
> swallow me up for not writing something that was less prone to
> misinterpretation,
>
> But I think this does leave us in a position where large amounts of
> this thread are a mixture of not very helpful or even plain confusing,
> and with people's permission, I'd honestly like to start again, with
> everyone on the same page.
>
> You might have noticed that "orientations" were really bugging me as
> to what they were, and how one could use them. I've managed to resolve
> these problems in my own mind and think that there's a very simple
> connection between "orientations" and "transforms" that I would be OK
> to implement and use. I think our next attempt at this discussion
> should start by being clear on our requirements, and then we can look
> at how "orientations" and "transforms" can get us there.
>
> (Rest of thread purposefully deleted... hope that's ok!)
>
> Thanks!
> David
Hi Robert, everyone Thanks for the message. I think that's helpful and makes sense to me. It seems to me that a large part of this is to clarify what's meant to happen, to resolve inconsistencies (and bugs!), and to make sure that everyone gets what they need. To try and summarise my own requirements: 1. I want any transform to be auto-corrected, that is, for any mounting rotation to be corrected transparently for me. However, I still think we should report what the mounting rotation is. 2. When it's possible to produce the requested transform, then obviously I want it to do that! 3. When it's not possible to deliver the requested transform, then I don't really mind what we choose to do, contingent on having requirement 4. For example, I would be fine with libcamera doing nothing at all, so that the "actual" transform returned would be the sensor mounting rotation (does everyone agree that this is what would happen?). 4. I want to be able to use the libcamera classes to compare what I requested versus what I got, so that I can figure out what I need to do. This should have absolutely no impact on the libcamera API (which is principally a question of what happens to the transform during validate()). Using the libcamera classes to work out what to do is entirely optional. A few final observations: * I'm fine with changing the meaning of the "rotation" property. Though we need to ensure this doesn't change the existing behaviour (on the Pi, can't speak for certain about anything else!), and as noted above, I think we should report the mounting rotation in another way. * I'm fine with introducing an Orientation if that's helpful to people. If we put the orientation into the camera configuration (replacing the transform) then I want the orientation class to interact correctly with the transform class so that we still have requirement 4. In this case the orientation class also needs to be fully implemented in the Python bindings, as transforms are. * I still have this niggle in my mind that the sense of the rotations was not settled in our previous discussions. We need to resolve this. That's everything I can think of at the minute. Does it seem reasonable? Thanks! David On Fri, 23 Jun 2023 at 12:05, Robert Mader via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote: > > Hi David, thanks for initiative to bring us all on the same page. > > From what I understand, this series aims to do two things: > > > 1.: Make the public API follow the EXIF specification > > 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. > > > 2.: Clarifying the expected behavior and fixing existing inconsistencies. > > 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()`. > > To give an example, consider the following two cases: > > A camera mounted 90 deg rotated, a config requesting `Transform::Identity` > No camera rotation, a config requesting `Transform::rot90` > > 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. > > In the second case, the returned value will be `Transform::Identity` and the user has to remember that it asked for something different. > > 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. > > I personally like the proposed behavior - either you get the requested result or you have to apply the combined transform yourself - a lot. > > From what I understand there are only two open questions here: > > Should the sensor do partial transforms? > Do we really want/need the new `Orientation` enum or could/should we instead with `Transform` and just update/clarify the expected results? > > > Hope you agree that these are the issues to agree on. > > Thanks everyone and cheers, > > Robert > > On 23.06.23 10:26, David Plowman via libcamera-devel wrote: > > Hi everyone > > Thanks for that message. I think it's very helpful to quote the source > documentation: > > * The transform is a user-specified 2D plane transform that will be applied > * to the camera images by the processing pipeline before being handed to > * the application. This is subsequent to any transform that is already > * required to fix up any platform-defined rotation. > > What this is trying to say, badly, is that the transform you request > is applied after ("subsequent to") fixing up the platform rotation. So > you do indeed get auto-correction. This is also the actual behaviour > of the code on the Pi, has been for, I don't know, nearly 2 years? At > this point, the earth does indeed get my official permission to > swallow me up for not writing something that was less prone to > misinterpretation, > > But I think this does leave us in a position where large amounts of > this thread are a mixture of not very helpful or even plain confusing, > and with people's permission, I'd honestly like to start again, with > everyone on the same page. > > You might have noticed that "orientations" were really bugging me as > to what they were, and how one could use them. I've managed to resolve > these problems in my own mind and think that there's a very simple > connection between "orientations" and "transforms" that I would be OK > to implement and use. I think our next attempt at this discussion > should start by being clear on our requirements, and then we can look > at how "orientations" and "transforms" can get us there. > > (Rest of thread purposefully deleted... hope that's ok!) > > Thanks! > David