[libcamera-devel] [RFC 0/4] libcamera: Replace CameraConfiguration::transform
David Plowman
david.plowman at raspberrypi.com
Wed Jun 21 14:37:11 CEST 2023
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...
>
> >
> > 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
> > >
More information about the libcamera-devel
mailing list