[libcamera-devel] [RFC 0/4] libcamera: Replace CameraConfiguration::transform

[David Plowman]

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 at 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
>