[libcamera-devel] [PATCH v6 0/12] libcamera: Replace CameraConfiguration::transform
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Mon Oct 23 10:58:10 CEST 2023
On Mon, Oct 23, 2023 at 01:56:30AM +0300, Laurent Pinchart via libcamera-devel wrote:
> Hi Jacopo,
>
> Thank you for the patches.
>
> CC'ing David, for feedback on the impact on Raspberry Pi (if any).
>
> On Thu, Oct 19, 2023 at 04:01:21PM +0200, Jacopo Mondi via libcamera-devel wrote:
> > v5->v6:
> > - Rename enum members
> > - replace images with svg
> > - fix documentation
> >
> > v4->v5:
> > - Add support in Python layer
> > - Rebased on master
> >
> > v3->v4:
> > - Remove unrelated change in 8/10
> > - Remove a double test case
> > - Add David's tags
> >
> > v2->v3:
> > - Move Orientation out of CameraConfiguration to a dedicated file
> > - Change operator*(Transform, Transform)
> > - Address David's comments in documentation and commit messages
> >
> > v1->v2:
> > - Provide a better definitio of Orientation based on two basic operations
> > - Provide functions to combine Orientation and Transpose
> > - Provide tests
> > - Do not adjust properties::Rotation anymore
> >
> > This series proposes to replace the usage to Transform in the public API in
> > favour of a new 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
> >
> > (the names of the enum values are different as gstreamer expresses the
> > "correction" while we express the actual orientation in memory buffers).
> >
> > 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.
> >
> > 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.
> >
> > 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
> > corrections are ignored. 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.
> >
> > 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.
> >
> > Tested on RPi imx219 with the newly introduced cam option, flip, mirror and
> > rotation work as expected.
>
> Overall this looks good to me. I've just sent a series of patches on top
> of thes series (numbered 13/12 to 21/12), with the first patches being
> small fixups that I propose squashing into the corresponding base patch.
> The last three patches would require more complex rework of your series
> if we wanted to squash them, so I think we can keep them on top.
>
> I wanted to replace the
>
> Orientation operator*(const Orientation &o, const Transform &t);
>
> with a
>
> Orientation Transform::operator()(const Orientation &o);
>
> in order to be able to write
>
> Transform t = ...;
> Orientation o1 = ...,
> Orientation o2 = t(o1);
>
> or also
>
> Orientation o1 = ...,
> Orientation o2 = Transform::HFlip(o1);
>
> which I think would be a nicer syntax. However, I couldn't find a way to
> nicely do so :-( Transform is currently an enum class, which is not a
> class, and thus can't have member operators. The call operator having no
> non-member (outside of the class definition) variant, I had to try and
> turn Transform into a class. I got stuck at the point where HFlip had to
> be a static constexpr member of the Transform class (to be able to write
> `Transform::HFlip`), but also an instance of the Transform class (to be
> able to write `Transform::HFlip(o)`). The compiler rightfully complained
> that I was trying to use a type that hasn't been fully defined yet. I
> gave up for now (but would be very interested into hearing from a C++
> enthousiast on how something similar could be achieved). If we keep the
> multiplication operator as-is, then your patch 06/12 that inverts the
> order of the operands to the multiplication operator between two
> Transform instances is needed.
>
> If you're fine with the additional changes, I can handle the squashing
> and pushing. I'll need R-b tags for the last three patches though. As
> for this series, with the changes on top,
Minor comments apart, I'm fine with all of this.
With a few questions clarified, feel free to squash in what's
squashable and push
Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
Thanks
j
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> An additional change I would like to see on top would be the rewrite of
> the Python Transform class in Python, without going through C++
> bindings. This would allow us to drop the transformFromRotation() which
> is used in the Python bindings code only. Any opinion on this ?
>
> > Jacopo Mondi (12):
> > libcamera: camera_sensor: Cache rotationTransform_
> > libcamera: camera: Introduce Orientation
> > Documentation: Add figures to document Orientation
> > libcamera: properties: Make 'Rotation' the mounting rotation
> > libcamera: transform: Add functions to convert Orientation
> > libcamera: transform: Invert operator*() operands
> > libcamera: transform: Add operations with Orientation
> > test: Add unit test for Transform and Orientation
> > py: libcamera: Define and use Orientation
> > libcamera: Use CameraConfiguration::orientation
> > apps: cam: Add option to set stream orientation
> > py: cam: Add option to set stream orientation
> >
> > Documentation/Doxyfile.in | 2 +
> > Documentation/rotation/rotate0.svg | 132 +++++++
> > Documentation/rotation/rotate0Mirror.svg | 135 +++++++
> > Documentation/rotation/rotate180.svg | 135 +++++++
> > Documentation/rotation/rotate180Mirror.svg | 135 +++++++
> > Documentation/rotation/rotate270.svg | 135 +++++++
> > Documentation/rotation/rotate270Mirror.svg | 135 +++++++
> > Documentation/rotation/rotate90.svg | 135 +++++++
> > Documentation/rotation/rotate90Mirror.svg | 135 +++++++
> > include/libcamera/camera.h | 4 +-
> > include/libcamera/internal/camera_sensor.h | 5 +-
> > include/libcamera/meson.build | 1 +
> > include/libcamera/orientation.h | 28 ++
> > include/libcamera/transform.h | 7 +
> > src/apps/cam/camera_session.cpp | 18 +
> > src/apps/cam/main.cpp | 5 +
> > src/apps/cam/main.h | 1 +
> > src/libcamera/camera.cpp | 20 +-
> > src/libcamera/camera_sensor.cpp | 131 ++++---
> > src/libcamera/meson.build | 1 +
> > src/libcamera/orientation.cpp | 79 +++++
> > 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/property_ids.yaml | 8 +-
> > src/libcamera/transform.cpp | 112 +++++-
> > src/py/cam/cam.py | 21 ++
> > src/py/libcamera/py_enums.cpp | 10 +
> > src/py/libcamera/py_main.cpp | 2 +-
> > test/meson.build | 1 +
> > test/transform.cpp | 329 ++++++++++++++++++
> > 34 files changed, 1787 insertions(+), 112 deletions(-)
> > create mode 100644 Documentation/rotation/rotate0.svg
> > create mode 100644 Documentation/rotation/rotate0Mirror.svg
> > create mode 100644 Documentation/rotation/rotate180.svg
> > create mode 100644 Documentation/rotation/rotate180Mirror.svg
> > create mode 100644 Documentation/rotation/rotate270.svg
> > create mode 100644 Documentation/rotation/rotate270Mirror.svg
> > create mode 100644 Documentation/rotation/rotate90.svg
> > create mode 100644 Documentation/rotation/rotate90Mirror.svg
> > create mode 100644 include/libcamera/orientation.h
> > create mode 100644 src/libcamera/orientation.cpp
> > create mode 100644 test/transform.cpp
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list