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

[David Plowman]

HI Jacopo

On Thu, 22 Jun 2023 at 09:08, Jacopo Mondi
<jacopo.mondi at ideasonboard.com> wrote:
>
> Hi David
>
> On Wed, Jun 21, 2023 at 05:33:45PM +0100, David Plowman wrote:
> > Hi again
> >
> > On Wed, 21 Jun 2023 at 15:30, Jacopo Mondi
> > <jacopo.mondi at ideasonboard.com> wrote:
> > >
> > > Hi David
> > >    let me summarize, as usual the discussion become hard to follow.
> > >
> > > On Wed, Jun 21, 2023 at 03:14:28PM +0100, David Plowman wrote:
> > >
> > > [snip]
> > >
> > > > > >
> > > > > > 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?
> > > >
> > >
> > > 1) Transform is a libcamera concept, other frameworks have to learn
> > > how to use it. A specification exists and it's wildely adopted, why
> > > should we diverge from it ? Because there is code already using the
> > > current implementation ?
> >
> > The orientation flag is completely identical to the transform, just
> > with different names, so I don't feel this should be an area of
>
> Why do you say it is identical ?

Sorry, I could have been clearer. The orientation flag is describing
exactly the same 8 plane transformations as the transform, with
exactly the same meanings. I believe the only difference is in the
naming, the precise encoding in terms of numeric values (hope no one
relies on those!), and the availability of extra functions to
"manipulate" them (compose/invert etc.).

I think different categories of users (maybe those who care about
rendering images, and those who are interested in image analysis, for
example) may have slightly different expectations for naming these
things, so I definitely think there's some scope for re-examining
those.

>
> The Orientation enumeration is defined based on the EXIF tag 274
> 'orientation' and describes 2D transforms in terms of clock-wise
> rotation and horizontal mirroring, while Transform is a custom-defined
> type which uses Transpose and V/HFlips. Why do you think they're
> identical ?
>
> The EXIF defined values are also used by
>
> gstreamer/gtk
> https://api.gtkd.org/gstreamer.c.types.TAG_IMAGE_ORIENTATION.html
>
> pipewire
> https://docs.pipewire.org/group__spa__buffer.html#gab4afeb1516aa33126757cfa275efd7a3
>
> > difficulty. We can change the names, or add synonyms, I don't really
> > mind. But the transform does have useful compose and inverse functions
> > which I would not want to lose.
> >
> > This code is in quite wide use in the Pi community, so I do generally
> > want to inconvenience users as little as possible.
> >
>
> I understand this argument, but it can't be used as an excuse to never
> change the API, in particular if there is a standard already adopted
> by other frameworks we don't ahere to.

I agree. Generally speaking we try not to change things more than
necessary, though there are many occasions where you have to. There
are always different things to consider and a good balance needs to be
found.

>
> > >
> > > 2) The library applies partial transforms (like "I'll do HFlip if I'm
> > > asked Rot270 and I can't do Transpose"). This is confusing as
> > > applications have to compose transforms and do computations. This
> > > patch simplifies it: either you can do it in full, or leave the stream
> > > un-touched. There are no clear advantages that I can think of in
> > > partially applying transforms.
> >
> > I like the partial transforms because it means I have only one
> > "exception" case to deal with, and not four. In particular, I can
> > control what the "left over" transform will be, if I want to. Though
> > of course, I don't have to.
> >
>
> I think what makes a difference here is the behaviour of the consumers
> of a "rotated" image.
>
> What is the "next" component that in most cases have to compensate for
> a non-fully corrected rotation ? I presume it generally is the display
> compositor or a framework like gstreamer that has elements like
> 'videoflip'. We know gtk/gstreamer internally uses the EXIF tag, but
> when it comes to compositor, why should for them be better to deal
> with "Transpose" instead that on Rot270 ? I'm no expert there but I
> presume rendering libraries have methods for quickly rotate images
> before rendering but they might not have "transpose only" ones. Also
> because "transpose along the diagonal axis from the top-left corner to
> the right-bottom one" is a concept I haven't found documented anywhere
> else if not in the Transform class (of course I might have missed it
> as I'm certainly no expert on rendering).

My aim is to try and give everyone the functionality that they want.

I wouldn't want us to think that rendering is the only use for images.
We have many users who want to process images and analyse them in
different ways. They may be running neural networks, doing filtering
operations, combining images, all kinds of things. It's actually very
common that they aren't rendering at all, and it's all about the
downstream image processing. Rendering is very important, but there
are other uses too, and it would be great to give them some
flexibility as well.

Even within rendering, I wonder what would happen if (for example) you
had hardware that can do rot90 and rot270, but not transpose. What
would you do if an application happened to ask for an orientation that
left you needing to do a transpose? It would be nice if you could
easily calculate the transform you need to ask for so that you're left
with a rot90 instead.

>
> > I think it would be OK to change the default behaviour to what you
> > have described (i.e. do nothing at all if it can't do the whole
> > transform), so long as I can still generate the transform that I want,
> > or my users are able to.
> >
> > >
> > > 3) properties::Rotation, the documentation does not report 'mounting
> > > rotation' but I admit we (I myself for one) have often used that term.
> > > That's because, originally, my understanding was that no
> > > autocorrection should have been performed. Since it has been
> > > introduced (by copying your PH implementation) and others liked it,
> > > now properties::Rotation reports the stream rotation when no
> > > ::transform is specified. I think it's confusing to have
> > > properties::Rotation=180 and a stream oriented correctly, applications
> > > might wonder what's happening and if libcamera is doing something
> > > behind their back.
> >
> > I don't mind changing the meaning of the Rotation property, but I
> > think we should also be fixing up the existing code so that its
> > behaviour doesn't change. It would also be nice if you could query the
>
> What existing code relies on Rotation being the mounting rotation ? In
> libcamera ? In the apps ?

I would agree that no one "needs" the mounting rotation, like they
don't "need" the pixel cell size. But we normally like to be able to
report everything that we know about a sensor, and applications may
have a "list all the info about this sensor" feature. So it just seems
like a helpful thing to do.

>
> > mounting rotation from an application, though I would agree an
> > application should never need to use it.
> >
> > >
> > > I think these are the main discussion points, aren't they ?
> >
> > I'd like to make a proposal, if I may.
> >
> > 1. I'm happy to change the Rotation property, but at the same time we
> > need to fix the code so that the behaviour of transforms is not
> > changed. I would still like there to be a property so that an
> > application can query the mounting rotation if it wants to.
>
> I'm fine introducing one

Great!

>
> > 2. We change the default behaviour to apply no transform at all if it
> > can't apply the full transform. (Applications that don't want this can
> > easily sort themselves out now.)

I think this was the biggest concern, so I'm hoping this addresses it.

> >
> > At this point I think everyone has the functionality that they want.
> > Just to try and flesh out the application code a bit so that we are
> > forced to understand it, we have the following.
> >
> > Applications that want the simple behaviour:
> >
> > Transform requestedTransform; // the transform the user wants
> > Transform actualTransform; // what the user will get, from checking
> > the transform field after validate()
> >
> > if (requestedTransform != actualTransform)
> >     /* The application will have to apply a transform equivalent to
> > -actualTransform * requestedTransform */;
> >
> > Applications that want to control the transform that they apply (let's
> > call this the "leftoverTransform"):
> >
> > if (requestedTransform != actualTransform)
> >     requestedTransform *= -leftoverTransform; // now just try again
>
> I still don't get why people should do any of this calculations.

I hope I've addressed that above. There are other use cases, and it
would be great to give everyone the functionality they want.

>
> I don't expect anyone to write their own rotation libraries, but
> instead rely on the rendering libraries or on piping gstreamer
> elements to the capture output. Why should they calculate the
> "leftoverTransform" ? What is the use case for an application in doing
> this ?

I hope that's covered above. I'd like everyone to have access to
functionality they might want, whilst not impacting those who don't
wish to use it.

>
> >
> > But maybe double-check if that all feels right!!
>
> I think it's relevant to know how gstreamer/pipewire expects to
> deal with transforms, and more importantly what 'transform' display
> compositors supports natively.

Absolutely, and I think my proposal covers this, I believe everyone
gets to do exactly what they want. If there are still particular
things that don't work, or are difficult, then we should definitely
look into them.

>
> >
> > 3. We can consider swapping the transform in the configuration for an
> > "orientation" with a different set of names. We need to add
> > transform/orientation conversion functions so that I (and my users)
> > can still use it.
>
> See the newly added functions in transform.cpp

That seems fine. Also very happy to discuss all this at a later date!

Thanks!
David

>
> >
> > How does that sound?
> >
> > David
> >
> > >
> > > > 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
> > > > > > > > >