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

[Jacopo Mondi]

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 ?

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.

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 think these are the main discussion points, aren't they ?

> 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
> > > > > >