[rotation_distortion]send interpolated viewport metrics to address rotation distortion #40412

hellohuanlin · 2023-03-17T23:40:11Z

Send interpolated viewport metrics to make the rotation smoother. Result is pretty good, as show below.

There are still some minor amount of distortion. There are 2 main reasons:

Flutter drawing happens on the UI thread, which is not iOS main thread. The delay between issuing engine command till the drawing happens is indeterministic and could vary depending on how busy/idle the app is during the rotation. This amount of distortion is unavoidable, and unlikely solvable.
We use linear interpolation for simplicity, but iOS window rotation is likely non-linear, with ease-in ease-out effect. Consider using non-linear interpolation if this becomes a problem.

Currently we are calling framework from engine per frame. A potential performance improvement can be just sending the rotation duration to the framework, and have the framework to do the interpolation by itself. Though this will require a whole lot more refactoring on both engine and framework.

3.size.interpolation.mp4

List which issues are fixed by this PR. You must list at least one issue.

Fixes flutter/flutter#16322

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

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. See testing the engine for instructions on writing and running engine tests.
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.

dnfield

This LGTM.

I think an alternative we might consider is to update the framework/Dart API so that this could be done in Dart code instead - but if iOS is really the only platform where this happens that may be overkill.

It would be worth filing a bug to improve the curve used on this. Historically iOS users care about this kind of thing more than I would have expected, and with this patch it should be relatively easy to fix for someone who wants to take the time to figure that out.

jmagman · 2023-03-22T17:11:53Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

+                                   UIEdgeInsets fromPadding,
+                                   CGSize toSize,
+                                   UIEdgeInsets toPadding) {
+  CGFloat scale = [UIScreen mainScreen].scale;


Suggest checking if the view is loaded before doing this:

Suggested change

CGFloat scale = [UIScreen mainScreen].scale;

UIScreen* mainScreen = self.mainScreenIfViewLoaded;

if (mainScreen == nil) {

return;

}

CGFloat scale = mainScreen.scale;

Just did some research - it turned out to be more involved and likely requires non-trivial refactoring to make it work in unit test, as explained here.

jmagman · 2023-03-22T17:12:50Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

+/**
+ * Interpolate the viewport metrics for smoother rotation transition.
+ */
+void FLTInterpolateViewportMetrics(flutter::ViewportMetrics& viewportMetrics,


Instead of passing in a viewportMetrics reference, can this instead be a method on the view controller that directly accesses _viewportMetrics? Or was that too difficult to test?

Yes I had issue with testing - OCMock/NSInvocation only supports populating NSObject references, and not structs. And I do not want to expose viewportMetrics property as it's anti-pattern. So I made this pure function for easier testing.

jmagman · 2023-03-22T17:21:18Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

+
+                                 // End of rotation. Invalidate the timer.
+                                 if (rotationProgress == 1) {
+                                   _isDuringRotationTransition = NO;


Is this potentially racy, can -viewWillTransitionToSize be called before the timer is invalidated/the rotation is complete? Can the rotation be reversed halfway through? Maybe this should instead be tracked with a NSConditionLock? Or the timer object be cached on the view controller and invalidated if -viewWillTransitionToSize is called a second time before it's complete?

Good catch. I did some experiment - the viewWillTransitionToSize itself behaves nicely, it gets called only after the previous rotation is done. However, race can still happen due to a few reasons (added in the comment).

…izes during rotation to fix a problem with aspect ratio distortion

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

cyanglaz · 2023-03-22T21:42:19Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

+  __block double rotationProgress = 0;
+  // Timer is retained by the run loop, and will be released after invalidated.
+  [NSTimer
+      scheduledTimerWithTimeInterval:kRotationViewportMetricsUpdateInterval


Maybe replace kRotationViewportMetricsUpdateInterval with 1/[DisplayLinkManager displayRefreshRate] for pro-motion devices.

Probably not necessary during rotation since the device is moving. I've even tried 30fps - not too bad either (And i don't have a pro motion device)

i can test with promotion device

Feel free to pull the branch, or can wait for me to fix my laptop's engine build (hopefully this week).

Actually, let me see if I can expense one.

hellohuanlin · 2023-03-23T18:30:55Z

Update: @cyanglaz and I found a performance issue for complex projects like flutter gallery.

Let me try a few things. Worst case scenario we may have to rely on the snapshot approach.

jmagman · 2023-03-23T21:13:09Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

+  CGSize oldSize = self.view.bounds.size;
+  UIEdgeInsets oldPadding = self.view.safeAreaInsets;
+
+  __block double rotationProgress = 0;


Alternatively use NSProgress, this is fine though.

jmagman · 2023-03-23T21:14:37Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

@@ -1299,7 +1419,7 @@ - (void)viewDidLayoutSubviews {
  _viewportMetrics.physical_height = viewBounds.size.height * scale;

  [self updateViewportPadding];
-  [self updateViewportMetrics];
+  [self updateViewportMetricsIfNeeded:NO];


At the call sites it's not clear what the boolean does (missing named parameters in Objective-C 😞). How about -updateViewportMetricsIfNeeded and -updateViewportMetricsIfNeededForRotation?

hellohuanlin · 2023-03-27T22:22:22Z

To share an update: I tried fine-tuning the interpolation count to address the perf issue on complex UI, and found 1/20 to 1/30 second (rather than 1/60 second) update interval worked for flutter gallery. However, this resulted in very "un-smooth" transition which is easily noticeable.

There is also a good news - Fun story, when i was playing basketball yesterday, a new solution came to my mind, and I call it delayed swap:

In the originally behavior, we swap the width/height immediately before the rotation, resulting in roughly ~4x distortion in the beginning. With "delayed swap" solution, we swap the width/height right in the middle of the rotation (i.e. delay the swap for half of the transition duration).

If I am imagining it right, this new "delayed swap" solution should give us the same result as the "snapshot" solution (reducing ~4x distortion to ~2x), but without the drawback of "snapshot" solution (i.e. it should work well with dynamic content like animation or video).

This "delayed swap" solution is much easier than both "size interpolation" and "snapshot" solution. I am quickly trying it out and hope to get some good result🤞

…distortion (#40730) The "size interpolation" solution didn't go well (more context [here](#40412 (comment))). Then a new solution came to my mind, and I call it **"delayed swap"**: In the originally behavior, we swap the width/height immediately before the rotation, resulting in roughly ~4x distortion in the beginning. With "delayed swap" solution, we **swap the width/height right in the middle of the rotation** (i.e. delay the swap for half of the transition duration). This new "delayed swap" solution gives us the same benefit as the "snapshot" solution: - reducing ~4x distortion to ~2x - most distorted frames occur in the middle of rotation when it's moving the fastest, making it hard to notice And it fixes the drawback of "snapshot" solution: - it works well with dynamic content like animation or video - it doesn't have a ~0.5 second penalty when taking the snapshot Looks pretty good on flutter gallery: https://user-images.githubusercontent.com/41930132/228383137-7cd09982-89a9-4c83-bf55-9431de708278.mp4 *List which issues are fixed by this PR. You must list at least one issue.* Fixes flutter/flutter#16322 *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

flutter-dashboard bot added the platform-ios label Mar 17, 2023

hellohuanlin force-pushed the rotation_size_interpolation branch 10 times, most recently from b7b91d2 to 2a51b57 Compare March 22, 2023 16:12

hellohuanlin marked this pull request as ready for review March 22, 2023 16:44

hellohuanlin requested review from jmagman, cyanglaz and dnfield March 22, 2023 16:45

dnfield approved these changes Mar 22, 2023

View reviewed changes

hellohuanlin mentioned this pull request Mar 22, 2023

Use non-linear viewport metrics interpolation to achieve smoother rotation transition flutter/flutter#123248

Closed

jmagman reviewed Mar 22, 2023

View reviewed changes

hellohuanlin added 3 commits March 22, 2023 12:25

[ios_rotation_distortion]use linear interpolation to send different s…

863c5a0

…izes during rotation to fix a problem with aspect ratio distortion

added TODO with non-linear interpolation with github ticket

e17cb4f

fix race condition by invalidating previous timer

38d2034

cyanglaz reviewed Mar 22, 2023

View reviewed changes

fix naming

580b9d2

hellohuanlin force-pushed the rotation_size_interpolation branch from 3a45ba4 to 580b9d2 Compare March 22, 2023 22:04

typo

4807468

jmagman reviewed Mar 23, 2023

View reviewed changes

hellohuanlin mentioned this pull request Mar 28, 2023

[rotation_distortion] Use "delayed swap" solution to reduce rotation distortion #40730

Merged

8 tasks

hellohuanlin closed this Mar 28, 2023

hellohuanlin mentioned this pull request Apr 3, 2023

Rotation on iOS does not look right flutter/flutter#16322

Closed

-  CGFloat scale = [UIScreen mainScreen].scale;
+  UIScreen* mainScreen = self.mainScreenIfViewLoaded;
+  if (mainScreen == nil) {
+    return;
+  }
+  CGFloat scale = mainScreen.scale;

[rotation_distortion]send interpolated viewport metrics to address rotation distortion #40412

[rotation_distortion]send interpolated viewport metrics to address rotation distortion #40412

Uh oh!

Conversation

hellohuanlin commented Mar 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hellohuanlin Mar 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hellohuanlin Mar 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hellohuanlin commented Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hellohuanlin commented Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

hellohuanlin commented Mar 17, 2023 •

edited

Loading

hellohuanlin Mar 22, 2023 •

edited

Loading

hellohuanlin Mar 22, 2023 •

edited

Loading

hellohuanlin commented Mar 23, 2023 •

edited

Loading

hellohuanlin commented Mar 27, 2023 •

edited

Loading