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

[Jacopo Mondi]

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