[libcamera-devel] [RFC 0/4] libcamera: Replace CameraConfiguration::transform
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Thu Jun 22 10:08:09 CEST 2023
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 ?
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.
> >
> > 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).
> 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 ?
> 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
> 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.)
>
> 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 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 ?
>
> 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.
>
> 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
>
> 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
> > > > > > > >
More information about the libcamera-devel
mailing list