Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

[camera] Fix iOS rotation issue #3591

Merged
merged 11 commits into from
Mar 24, 2021
Merged

[camera] Fix iOS rotation issue #3591

merged 11 commits into from
Mar 24, 2021

Conversation

mvanbeusekom
Copy link
Contributor

This PR fixes an issue where on iOS the default orientation was set to landscape right while it should have been set to portrait up. To compensate for this problem the CameraPreview widget used an additional offset when calculating the rotation on iOS.

This PR solves the problem at the root and also removed the extra offset needed on iOS to adjust the orientation.

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@google-cla google-cla bot added the cla: yes label Feb 22, 2021
bparrishMines
bparrishMines reviewed Mar 11, 2021
View reviewed changes
Copy link
Contributor

@bparrishMines bparrishMines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good, but this still lacks tests. Potentially a widget test?

@mvanbeusekom
Copy link
Contributor Author

@bparrishMines on iOS this indeed looks good but on Android I am running into the following problem: https://discord.com/channels/608014603317936148/608020293944082452/815873332574158890 (if you have a solution or advise I would love to hear it)

I will mark the PR as draft until this gets resolved. And I will also make sure to add tests.

@mvanbeusekom mvanbeusekom marked this pull request as draft March 11, 2021 19:33
cyanglaz
cyanglaz reviewed Mar 11, 2021
View reviewed changes
packages/camera/camera/lib/src/camera_preview.dart
return turns[_getApplicableOrientation()]!;
}

DeviceOrientation _getApplicableOrientation() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm, there is no diff in this method right? The method was just moved down?

Copy link
Contributor Author

@mvanbeusekom mvanbeusekom Mar 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, it made more sense to me to keep it closer to where it was used. I see now that it causes a bit of noice

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keeping it close to where it was used makes sense to me!

@mvanbeusekom mvanbeusekom marked this pull request as ready for review March 16, 2021 11:31
@mvanbeusekom
Copy link
Contributor Author

Everything looks good, but this still lacks tests. Potentially a widget test?

@bparrishMines I have added some widget tests to make sure the rotated box turns correctly according to the different settings provided by the CameraController. Would appreciate it is you could have another look.

@bparrishMines
Copy link
Contributor

@mvanbeusekom As stated on Discord, Camera2 on Android automatically changes the orientation of the output while iOS requires you to set the value.

On iOS, have you tried listening to the the UIDeviceOrientationDidChangeNotification and automatically updating the output camera preview orientation? I don't think there's anything that can be done on Android about Orientation since we use a SurfaceTextureEntry and not a TextureView.

@mvanbeusekom
Copy link
Contributor Author

mvanbeusekom commented Mar 19, 2021 •
edited
Loading

On iOS, have you tried listening to the the UIDeviceOrientationDidChangeNotification and automatically updating the output camera preview orientation?

Yes that is exactly what I did in this pull request, which we tested extensively and solves all orientation issues on iOS without the need of a RotatedBox on the Dart side. This is now ready and added additional automation tests to ensure this is working properly.

I was just wondering if we could do something similar on Android so we could remove the RotatedBox on the Dart side altogether. I had an offline discussion with @cyanglaz and we agreed to land this PR first fixing the issues on iOS (which are currently the biggest problem) and file a new feature request for the Flutter Engine to see if its possible to expose the TextureView in the future so we could also solve this issue in a more elegant way for Android (without the need for additional coding on the Dart side).

So in conclusion, this PR should be ready and would be great if you could review it.

@martin-labanic martin-labanic mentioned this pull request Mar 22, 2021
Closed
bparrishMines
bparrishMines approved these changes Mar 24, 2021
View reviewed changes
Copy link
Contributor

@bparrishMines bparrishMines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mvanbeusekom mvanbeusekom added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Mar 24, 2021
@mvanbeusekom mvanbeusekom merged commit 9cd84bd into flutter:master Mar 24, 2021
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 24, 2021
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 24, 2021
@engine-flutter-autoroll
9cd84bd [camera] Fix iOS rotation issue (flutter/plugins#3591)
c993e07
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 24, 2021
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 24, 2021
@engine-flutter-autoroll
9cd84bd [camera] Fix iOS rotation issue (flutter/plugins#3591)
ac5ee4a
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 25, 2021
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 25, 2021
@engine-flutter-autoroll
9cd84bd [camera] Fix iOS rotation issue (flutter/plugins#3591)
25c11fb
@snoopdoggy322
Copy link

In my case, updating to a new version did not fix this problem
Device: iPhone SE(old)

RPReplay_Final1623858543.MP4

@mvanbeusekom mvanbeusekom deleted the issue/76210 branch September 21, 2021 09:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cyanglaz cyanglaz cyanglaz left review comments

@bparrishMines bparrishMines bparrishMines approved these changes

@BeMacized BeMacized Awaiting requested review from BeMacized

Assignees
No one assigned
Labels
cla: yes p: camera platform-ios waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Camera Plugin Stream is rotated 90 degree on iPhone since 0.7.0
4 participants
@mvanbeusekom @bparrishMines @snoopdoggy322 @cyanglaz