[libcamera-devel] [RFC 0/4] libcamera: Replace CameraConfiguration::transform
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Jun 22 17:14:06 CEST 2023
Hello,
I'm replying in the middle of the thread as the next e-mail trimmed part
of the conversation. I'll reply to the second part just after.
On Wed, Jun 21, 2023 at 03:14:28PM +0100, David Plowman via libcamera-devel wrote:
> On Wed, 21 Jun 2023 at 13:57, Jacopo Mondi wrote:
> > On Wed, Jun 21, 2023 at 01:37:11PM +0100, David Plowman wrote:
> > > On Wed, 21 Jun 2023 at 10:23, Jacopo Mondi wrote:
> > > > 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 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.
> > > > >
The last point is slightly debatable. I can't tell if this is right, as
it depends how one interprets the confusing documentation :-) Maybe this
is the source of the disagreement. What is absolutely clear is that the
original intent, the documentation and the implementation don't align
properly. Whether it is the documentation or the code (or both) that we
need to fix is what we're discussing here.
I also agree with Jacopo that consuming the API in upper layers is
currently problematic.
> > > > > 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?
The documentation states
* The transform is a user-specified 2D plane transform that will be applied
* to the camera images by the processing pipeline before being handed to
* the application. This is subsequent to any transform that is already
* required to fix up any platform-defined rotation.
I can't interpret the first sentence as anything else than a
transformation applied on the image, I don't see how it can be
construed as meaning the desired orientation of the image at the output
of the pipeline.
Of course, for 180° rotations, the two are equivalent. The fact that 90°
and 270° support has been implemented in
CameraSensor::validateTransform() but not tested (due to lack of
hardware support) only contributes to additional confusion as what the
original intent was.
The second sentence sounds very confusing to me.
> > > >
> > > > 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?
This stems from the fact that you wanted (if I recall correctly) the
platform to auto-correct the mounting rotation when possible. Exposing
to the user the fact that the sensor is mounted upside-down, *and*
auto-correcting this when possible, isn't a behaviour I like. There are
too many implicit assumptions, it's just confusing. If you want
auto-correction, it needs to be completely transparent, libcamera needs
to hide the fact that the sensor is mounted upside-down from the
application in that case.
I want the API to be clear and simple when no auto-correction is
applied. I'm OK with auto-correction being added on top if it's totally
transparent for applications. This means that auto-correction shouldn't
even need to be mentioned in the API specification, as applications
shouldn't have to care.
> > > > > If the problem is actually to do with documentation, I would always
> > > > > agree that it can be improved, and am very happy to help!
Documentation surely needs to be improved, regardless of what we decide
to do with the code.
> > > > > > 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"?
"Orientation" is a term widely used in other frameworks and
specifications, including EXIF, and GStreamer. While I don't rule out
finding a better name, it doesn't seem bad to me.
"Transform" has a strong implication that it specifies what
transformation is applied to the image, not what orientation the image
has at the output of the pipeline. If you prefer the "transform" name,
then the meaning of the parameter has to match the name.
> > > > 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.
Helpers exposed by the libcamera core API need to have a purpose for
libcamera. If the only purpose of a Transform class, as used by
applications, is to compose transformations to calculate how to
configure a compositor based on the orientation of the image produced by
libcamera, then that helper doesn't belong to the libcamera core API. I
don't mind having it in a conveniency utility library though.
I don't want to digress too much, but some context will be useful to
understand my point here. With the need to support more languages in the
future, as well as the ABI stability requirements, I expect we will
strip down the core libcamera API from helpers. The core API will likely
be specified using some form of IDL, and the ABI will be plain C. The
language bindings, including C++ bindings, will be implemented (likely
auto-generated from the API IDL) based on the C ABI. Utilities that are
not part of the core API will be implemented directly in the language
bindings. For instance, we will retain geometry structures to represent
points, sizes and rectangles, but the core API will not include the
geometry helpers. The existing geometry helpers will become internal to
libcamera (as I expect the libcamera internal implementation to remain
in C++, and the helpers have shown to be quite useful). A separate
version of the helpers will also likely be provided by the C++ bindings
and other language bindings. If any particular language has native
classes to represent the geometry primitives, then we will likely use
those.
I don't want to discuss this plan in details as part of this mail
thread, but let's keep it in mind for the libcamera core API design.
> > > > > > 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?)
My understanding, as noted above, is that passing "Transform::Identity"
has always meant that the application wants the images in the
orientation produced by the sensor (or, to be precise, the orientation
of the pixel array, as the h/v flip are applied in the sensor too). If
we want CameraConfiguration::transform to indicate what orientation the
output image should have (and it seems we all agree on that), then it
should be named differently.
> > > > 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",
That only holds true if the camera can apply h/v flip. If it can't, the
application will need to handle the 90° and 270° rotations, so generic
applications will always have to support those cases in addition to
transposition. It can thus be argued that having to support 90°
rotation, 270° rotation and transposition is more complicated for the
application than having to support rotations only.
> > > > > 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.
Note that "rotation" isn't either. EXIF supports the 8 combinations of
h/v flip and tranpose, assigning them clear values, but doesn't use the
"rotation", "transpose" or "flip" names to document them. I'm fine using
those names in the libcamera API.
> > 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"
Do all display engines that support rotation at the hardware
level be able to apply transposition without also flipping (that is,
pure transposition that isn't a 90° or 270° rotation) ? Do all
compositors that support rotation expose a pure transpose transformation
in their API ? And the other way around, are there hardware devices or
compositor APIs that can transpose but can't flip ?
Unless I'm mistaken, there's no clear and universal answer to these
questions. I thus don't want to assume what transformations the
consumers of the images will be able to apply easily and efficiently.
I'm missing the reason why libcamera should consider that producing an
image that requires rotation by 90° in the application is better (or
worse) than producing an image that requires transposing. If there's no
option that is intrinsically better, then I prefer aiming for the
simpler and easier to understand behaviour, which is to let the
application deal with it.
> > > 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?
That's too complex I think. If we want to extend the API (not merely
change it), we could report the transformation capabilities of the
camera as a property to let applications discover what can be done. It
will then be the responsibility of the application to ask for something
that is supported, and we won't have to design complex heuristics to
find the "best" option when the applications asks for an unsupported
transformation. The API will be simpler to document, and simpler to
understand.
> > > > 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?
>
> > > > > > 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.
The series can be merged when ready, whenever that will be. There's no
need to tie it to a "release".
> > > > > 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'm afraid we will have many API changes, much more invasive than this
one, in the future before we stabilize the API.
> > > > > 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...
> > > > >
> > > > > > 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
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list