| Message ID | 20231019140133.32090-1-jacopo.mondi@ideasonboard.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
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@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
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@ideasonboard.com> Thanks j > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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
Hi 2023. október 23., hétfő 0:56 keltezéssel, Laurent Pinchart via libcamera-devel <libcamera-devel@lists.libcamera.org> írta: > [...] > 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. > [...] I believe something like the following can work: https://gcc.godbolt.org/z/ecqoGTGGs It's a lot of code compared to the enum... On a slightly related note, I think using a pipe is also not bad: "o1 | t1 | t2 | t3 ...". Regards, Barnabás Pőcze