[camera_android_camerax] Fix 90°-off preview rotation #8629
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
|
@camsim99 Do these fixes also address flutter/flutter#156974? Linking here in case this pull request fixes it as well. |
This wouldn't fix that issue, but I'll follow up there. |
matanlurey
left a comment
matanlurey
left a comment
There was a problem hiding this comment.
From an implementation POV, LGTM, great work.
From a testing POV, we can rely a bit less on mocks and testing internal state, and more on testing the exported state of the code itself; this will make the system less fragile (we're doing closer to e2e tests in a unit test environment) and more easily understood.
There are external guides talking about API testing, but I'll message one in Flutter Android chat that is internal I like a lot.
...era_android_camerax/android/src/main/java/io/flutter/plugins/camerax/PreviewHostApiImpl.java
Show resolved
Hide resolved
...era_android_camerax/android/src/main/java/io/flutter/plugins/camerax/PreviewHostApiImpl.java
Show resolved
Hide resolved
...era/camera_android_camerax/android/src/test/java/io/flutter/plugins/camerax/PreviewTest.java
Outdated
Show resolved
Hide resolved
| _subscriptionForDeviceOrientationChanges; | ||
| /// Whether or not the Android surface producer automatically handles | ||
| /// correcting the rotation of camera previews for the device this plugin runs on. | ||
| @visibleForTesting |
There was a problem hiding this comment.
I would try to not inspect the internal state of these variables as the test (in other words, not make them @visibleForTesting and make them private instead), and instead inspect the output of the widget tree. I'll leave more comments on the test itself.
There was a problem hiding this comment.
Left these as visible so that we can set them directly in the preview_rotation_test.dart. These values being fetched as expected is tested in android_camera_camerax_test.dart.
There was a problem hiding this comment.
Left these as visible so that we can set them directly in the preview_rotation_test.dart.
Is it possible to write that test without setting these variables directly? By setting them directly, we are bypassing the logic that is initially used to set them (in createCameraWithSettings) which if I remember correctly was a significant source of difficulty.
packages/camera/camera_android_camerax/lib/src/android_camera_camerax.dart
Outdated
Show resolved
Hide resolved
packages/camera/camera_android_camerax/test/android_camera_camerax_test.dart
Show resolved
Hide resolved
| expect((widget as Texture).textureId, cameraId); | ||
| }); | ||
|
|
||
| test( |
There was a problem hiding this comment.
This is a good start, but I'd split this into separate tests.
This is even complicated enough where I'd support a preview_rotation_test.dart.
Matrix of options I'd spect:
- When
handlesCropAndRotation = true, the preview is <not rotated or w/e it is>
- Sub test: Verify rotation the stream still works as expected
- When
handlesCropAndRotation = false
- Sub test: Based on the initial orientation; 1 test for each orientation
- Sub test: Preview changes as orientation changes (1 test is fine IMO)
- Sub test: How
sensorOrientationDegreesplays into things - Sub test: How
facingSignplays into things
Basically, we'd want to know that all of these parameters are being used to render the camera preview as expected. It should be possible to read this test suite and understand the contract of the camera preview.
There was a problem hiding this comment.
Added preview_rotation_test.dart with the intention of covering all of these cases.
Sub test: Verify rotation the stream still works as expected
Not 100% sure what you meant but I:
- Already have a test in
android_camera_camerax_test..dartthat tests theonDeviceOrientationChanged(). - Ensured that when
handlesCropAndRotationisfalse, noRotatedBoxis used inpreview_rotation_test.dart. - Added to
camera/camera'sCameraPreviewtest to ensure that its rotation behaves as expected. cc @bparrishMines because this is why your review was requested :)
3 made me realize a case that this PR will not cover; the preview rotation logic written here does not account for locked capture orientations. If it is locked, CameraPreview is rotated according to that instead of the device orientation. I'll file an issue and follow this PR up with a fix for that.
@matanlurey let me know if these cases don't cover what you were talking about!
There was a problem hiding this comment.
3 made me realize a case that this PR will not cover; the preview rotation logic written here does not account for locked capture orientations. If it is locked, CameraPreview is rotated according to that instead of the device orientation. I'll file an issue and follow this PR up with a fix for that.
Filed flutter/flutter#163857
packages/camera/camera_android_camerax/test/rotated_preview_test.dart
Outdated
Show resolved
Hide resolved
packages/camera/camera_android_camerax/test/rotated_preview_test.dart
Outdated
Show resolved
Hide resolved
matanlurey
left a comment
matanlurey
left a comment
There was a problem hiding this comment.
Mostly LGTM. I'd like to see us try not to use @visibleForTesting at all to manipulate the internal state for tests, but it's possible that is not doable or requires a lot more work than I expect. PTAL.
...era_android_camerax/android/src/main/java/io/flutter/plugins/camerax/PreviewHostApiImpl.java
Show resolved
Hide resolved
| _subscriptionForDeviceOrientationChanges; | ||
| /// Whether or not the Android surface producer automatically handles | ||
| /// correcting the rotation of camera previews for the device this plugin runs on. | ||
| @visibleForTesting |
There was a problem hiding this comment.
Left these as visible so that we can set them directly in the preview_rotation_test.dart.
Is it possible to write that test without setting these variables directly? By setting them directly, we are bypassing the logic that is initially used to set them (in createCameraWithSettings) which if I remember correctly was a significant source of difficulty.
There was a problem hiding this comment.
These tests look really good.
My only other comment (left in a previous file) is it would be better to go through the createCameraWithSettings branch instead of carving out the @visibleForTesting members.
If that is really difficult (for mocking or testability reasons), and you want help, let's pair (it's possible I'll agree and give up this point, but I'd want to give it a shot).
There was a problem hiding this comment.
Done. Also, did a minor refactor to remove a duplicate call to get the sensor orientation. Also also, I'll note it requires a call to availableCameras and createCameraWithSettings, so all of the tests go through that path. PTAL
| flutterSurfaceProducer.release(); | ||
| return; | ||
| } | ||
| throw new IllegalStateException( |
There was a problem hiding this comment.
Where does our code catch IllegalStateException? if we dont where do we document for our users that they need to catch an exception?
Native code thrown exceptions I remember tripping up my previous team on a couple of occasions to the point where I think we made a rule that exceptions were not allowed to leave the plugin api and if they did it was considered a plugin bug.
I think our best guidance is here https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#platform-exception-handling
There was a problem hiding this comment.
We throw exceptions like this throughout the camera_android_camerax implementation to signal that there may be an error in the plugin implementation to plugin authors, so this is not meant to be caught by users.
If you have greater concerns about this pattern, I suggest we file an issue and address this for the plugin as a whole in a follow up PR.
There was a problem hiding this comment.
OK, Non blocking question. Do we catch this (or other) exception(s) or is it thrown to users code?
There was a problem hiding this comment.
We don't catch it. They're really just supposed to help debug.
packages/camera/camera_android_camerax/lib/src/rotated_preview.dart
Outdated
Show resolved
Hide resolved
packages/camera/camera_android_camerax/lib/src/android_camera_camerax.dart
Show resolved
Hide resolved
camsim99
added
the
autosubmit
flutter/packages@c44c228...01d3d5c 2025-02-27 [email protected] Manual roll Flutter from 043b719 to 1659206 (19 revisions) (flutter/packages#8728) 2025-02-27 [email protected] Manual roll Flutter from 911aa75 to 043b719 (flutter/packages#8693) 2025-02-26 [email protected] Dependabot to update major and minor versions of test dependencies, ignore patch (flutter/packages#8712) 2025-02-26 [email protected] [local_auth] Update to use flutter.targetSdkVersion (flutter/packages#8695) 2025-02-26 [email protected] [go_router_builder]: Handle invaild params (flutter/packages#8405) 2025-02-26 [email protected] [pigeon] Timestamp test steps in CI (flutter/packages#8716) 2025-02-26 [email protected] [camera_android_camerax] Fix 90°-off preview rotation (flutter/packages#8629) 2025-02-26 [email protected] [go_router] Secured empty matches in canPop (flutter/packages#8557) 2025-02-26 [email protected] [tool] Update targetsdk version to 35 from 32 (flutter/packages#8694) 2025-02-26 [email protected] [various] Bump androidx.test:core to 1.4.0 (flutter/packages#8710) 2025-02-26 [email protected] [camera] Disable flaky tests (flutter/packages#8708) 2025-02-26 [email protected] [url_launcher][web] Prevent browser from navigating when followLink isn't called (flutter/packages#8675) 2025-02-26 [email protected] [various] Remove plugin-level `integration_test` dependencies (flutter/packages#8711) 2025-02-26 [email protected] [ci] Lengthen custom tests timeout (flutter/packages#8715) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…iented devices (#9097) Fixes the incorrect camera preview rotation on landscape-oriented devices (like some tablets). Really, this PR generalizes the fix added in #8629 for correcting the camera preview rotation by correcting the camera preview rotation according to the rotation from the natural device orientation instead of assuming the device is naturally oriented in portrait up. There are technically two different fixes in this PR; here's the breakdown for clarity: 1. For devices using the `SurfaceTexture` Impeller backend: Rotate the camera preview according to the current default display rotation (the rotation away from the natural device orientation) and ignore rotation added in `CameraPreview` widget completely (as it assumes the natural device orientation to be portrait up). 2. For devices using the `ImageReader` Impeller backend: Rotate the camera preview according to https://developer.android.com/media/camera/camera2/camera-preview#orientation_calculation, where `deviceOrientationDegrees` comes from [a call](https://developer.android.com/reference/android/view/Display?_gl=1*lysjx*_up*MQ..*_ga*NjY3Nzk0Mzg2LjE3NDU5NjY3ODc.*_ga_6HH9YJMN9M*MTc0NTk2Njc4Ni4xLjAuMTc0NTk2Njc4Ni4wLjAuMjEwMTI3NTg1Mg..#getRotation()) to get the current default display rotation (the rotation away from the natural device orientation) instead of assuming it to be the rotation degrees from portrait up. Also, fixes a potential regression that @bparrishMines discovered in the `DeviceOrientationManager`. The changes there essentially ensure that it resets its state every time it is "re-started" to listen into changes into device orientation changes. Fixes flutter/flutter#164493 *Seems to work on all physical devices I test, but not emulators (tried Pixel Tablet API 28, 35). I think that is an emulator bug? Otherwise tested on a new Pixel Tablet (API 34), Pixel Tablet C (API 12), Pixel 3A (phone, API 32). ## Pre-Review Checklist [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.
Fixes 90°-off preview rotation without locked capture orientation. Part of flutter/flutter#154241. The major changes in this PR: 1. Ensures the preview widget is rebuilt when a new device orientation is detected by changing the `buildPreview` to return a `StatefulWidget` instead of a `Texture` that wouldn't pick up changes in device orientation. 2. Uses [`handlesCropAndRotation`](https://api.flutter.dev/javadoc/io/flutter/view/TextureRegistry.SurfaceProducer.html#handlesCropAndRotation()) to detect if the preview is already correctly rotated or not. This API was added exactly for that purpose. 3. Corrects the preview when needed using https://developer.android.com/media/camera/camera2/camera-preview#orientation_calculation (also subtracts rotation applied in `CameraPreview` widget). See flutter/flutter#154241 (comment) for slightly more context if desired.
Fixes 90°-off preview rotation without locked capture orientation. Part of flutter/flutter#154241. The major changes in this PR: 1. Ensures the preview widget is rebuilt when a new device orientation is detected by changing the `buildPreview` to return a `StatefulWidget` instead of a `Texture` that wouldn't pick up changes in device orientation. 2. Uses [`handlesCropAndRotation`](https://api.flutter.dev/javadoc/io/flutter/view/TextureRegistry.SurfaceProducer.html#handlesCropAndRotation()) to detect if the preview is already correctly rotated or not. This API was added exactly for that purpose. 3. Corrects the preview when needed using https://developer.android.com/media/camera/camera2/camera-preview#orientation_calculation (also subtracts rotation applied in `CameraPreview` widget). See flutter/flutter#154241 (comment) for slightly more context if desired.
…iented devices (flutter#9097) Fixes the incorrect camera preview rotation on landscape-oriented devices (like some tablets). Really, this PR generalizes the fix added in flutter#8629 for correcting the camera preview rotation by correcting the camera preview rotation according to the rotation from the natural device orientation instead of assuming the device is naturally oriented in portrait up. There are technically two different fixes in this PR; here's the breakdown for clarity: 1. For devices using the `SurfaceTexture` Impeller backend: Rotate the camera preview according to the current default display rotation (the rotation away from the natural device orientation) and ignore rotation added in `CameraPreview` widget completely (as it assumes the natural device orientation to be portrait up). 2. For devices using the `ImageReader` Impeller backend: Rotate the camera preview according to https://developer.android.com/media/camera/camera2/camera-preview#orientation_calculation, where `deviceOrientationDegrees` comes from [a call](https://developer.android.com/reference/android/view/Display?_gl=1*lysjx*_up*MQ..*_ga*NjY3Nzk0Mzg2LjE3NDU5NjY3ODc.*_ga_6HH9YJMN9M*MTc0NTk2Njc4Ni4xLjAuMTc0NTk2Njc4Ni4wLjAuMjEwMTI3NTg1Mg..#getRotation()) to get the current default display rotation (the rotation away from the natural device orientation) instead of assuming it to be the rotation degrees from portrait up. Also, fixes a potential regression that @bparrishMines discovered in the `DeviceOrientationManager`. The changes there essentially ensure that it resets its state every time it is "re-started" to listen into changes into device orientation changes. Fixes flutter/flutter#164493 *Seems to work on all physical devices I test, but not emulators (tried Pixel Tablet API 28, 35). I think that is an emulator bug? Otherwise tested on a new Pixel Tablet (API 34), Pixel Tablet C (API 12), Pixel 3A (phone, API 32). ## Pre-Review Checklist [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.
…iented devices (flutter#9097) Fixes the incorrect camera preview rotation on landscape-oriented devices (like some tablets). Really, this PR generalizes the fix added in flutter#8629 for correcting the camera preview rotation by correcting the camera preview rotation according to the rotation from the natural device orientation instead of assuming the device is naturally oriented in portrait up. There are technically two different fixes in this PR; here's the breakdown for clarity: 1. For devices using the `SurfaceTexture` Impeller backend: Rotate the camera preview according to the current default display rotation (the rotation away from the natural device orientation) and ignore rotation added in `CameraPreview` widget completely (as it assumes the natural device orientation to be portrait up). 2. For devices using the `ImageReader` Impeller backend: Rotate the camera preview according to https://developer.android.com/media/camera/camera2/camera-preview#orientation_calculation, where `deviceOrientationDegrees` comes from [a call](https://developer.android.com/reference/android/view/Display?_gl=1*lysjx*_up*MQ..*_ga*NjY3Nzk0Mzg2LjE3NDU5NjY3ODc.*_ga_6HH9YJMN9M*MTc0NTk2Njc4Ni4xLjAuMTc0NTk2Njc4Ni4wLjAuMjEwMTI3NTg1Mg..#getRotation()) to get the current default display rotation (the rotation away from the natural device orientation) instead of assuming it to be the rotation degrees from portrait up. Also, fixes a potential regression that @bparrishMines discovered in the `DeviceOrientationManager`. The changes there essentially ensure that it resets its state every time it is "re-started" to listen into changes into device orientation changes. Fixes flutter/flutter#164493 *Seems to work on all physical devices I test, but not emulators (tried Pixel Tablet API 28, 35). I think that is an emulator bug? Otherwise tested on a new Pixel Tablet (API 34), Pixel Tablet C (API 12), Pixel 3A (phone, API 32). ## Pre-Review Checklist [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.
5 participants
Fixes 90°-off preview rotation without locked capture orientation. Fixes flutter/flutter#154241.
The major changes in this PR:
buildPreviewto return aStatefulWidgetinstead of aTexturethat wouldn't pick up changes in device orientation.handlesCropAndRotationto detect if the preview is already correctly rotated or not. This API was added exactly for that purpose.CameraPreviewwidget).See flutter/flutter#154241 (comment) for slightly more context if desired.
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.///).