[libcamera-devel] [RFC 0/4] libcamera: Replace CameraConfiguration::transform
David Plowman
david.plowman at raspberrypi.com
Wed Jun 21 18:33:45 CEST 2023
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
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.
>
> 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 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
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.
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
But maybe double-check if that all feels right!!
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.
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