[libcamera-devel] [PATCH v6 0/12] libcamera: Replace CameraConfiguration::transform
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Oct 23 00:56:30 CEST 2023
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,
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