[libcamera-devel] [RFC 0/4] libcamera: Replace CameraConfiguration::transform
David Plowman
david.plowman at raspberrypi.com
Wed Jun 21 16:14:28 CEST 2023
Hi again
On Wed, 21 Jun 2023 at 13:57, Jacopo Mondi
<jacopo.mondi at 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 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 ?
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
> > > > >
More information about the libcamera-devel
mailing list