[libcamera-devel] [RFC 0/4] libcamera: Replace CameraConfiguration::transform
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Wed Jun 21 11:23:49 CEST 2023
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 ?
>
> 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
> >
More information about the libcamera-devel
mailing list