-
Notifications
You must be signed in to change notification settings - Fork 6k
[rotation_distortion]send interpolated viewport metrics to address rotation distortion #40412
Changes from all commits
863c5a0
e17cb4f
38d2034
580b9d2
4807468
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,6 +44,40 @@ | |
NSNotificationName const FlutterViewControllerShowHomeIndicator = | ||
@"FlutterViewControllerShowHomeIndicator"; | ||
|
||
/** | ||
* Compute the interpolated value under linear interpolation. | ||
*/ | ||
CGFloat FLTLinearInterpolatedValue(double progress, CGFloat from, CGFloat to, CGFloat scale) { | ||
// TODO(hellohuanlin): consider non-linear interpolation to further reduce rotation distortion. | ||
// See: https://github.com/flutter/flutter/issues/123248 | ||
NSCAssert(progress >= 0 && progress <= 1, @"progress must be between 0 and 1"); | ||
return (from * (1 - progress) + to * progress) * scale; | ||
} | ||
|
||
/** | ||
* Interpolate the viewport metrics for smoother rotation transition. | ||
*/ | ||
void FLTInterpolateViewportMetrics(flutter::ViewportMetrics& viewportMetrics, | ||
double rotationProgress, | ||
CGSize fromSize, | ||
UIEdgeInsets fromPadding, | ||
CGSize toSize, | ||
UIEdgeInsets toPadding, | ||
CGFloat scale) { | ||
viewportMetrics.physical_width = | ||
FLTLinearInterpolatedValue(rotationProgress, fromSize.width, toSize.width, scale); | ||
viewportMetrics.physical_height = | ||
FLTLinearInterpolatedValue(rotationProgress, fromSize.height, toSize.height, scale); | ||
viewportMetrics.physical_padding_top = | ||
FLTLinearInterpolatedValue(rotationProgress, fromPadding.top, toPadding.top, scale); | ||
viewportMetrics.physical_padding_left = | ||
FLTLinearInterpolatedValue(rotationProgress, fromPadding.left, toPadding.left, scale); | ||
viewportMetrics.physical_padding_bottom = | ||
FLTLinearInterpolatedValue(rotationProgress, fromPadding.bottom, toPadding.bottom, scale); | ||
viewportMetrics.physical_padding_right = | ||
FLTLinearInterpolatedValue(rotationProgress, fromPadding.right, toPadding.right, scale); | ||
} | ||
|
||
// Struct holding data to help adapt system mouse/trackpad events to embedder events. | ||
typedef struct MouseState { | ||
// Current coordinate of the mouse cursor in physical device pixels. | ||
|
@@ -63,6 +97,16 @@ @interface FlutterViewController () <FlutterBinaryMessenger, | |
@property(nonatomic, assign) BOOL isHomeIndicatorHidden; | ||
@property(nonatomic, assign) BOOL isPresentingViewControllerAnimating; | ||
|
||
/** | ||
* Whether it is interpolating viewport metrics for rotation transition. | ||
*/ | ||
@property(nonatomic, assign) BOOL isDuringViewportMetricsInterpolationForRotation; | ||
|
||
/** | ||
* The timer for sending interpolated viewport metrics during rotation. | ||
*/ | ||
@property(nonatomic, retain) NSTimer* rotationTimer; | ||
|
||
/** | ||
* Keyboard animation properties | ||
*/ | ||
|
@@ -843,6 +887,77 @@ - (void)viewDidDisappear:(BOOL)animated { | |
[super viewDidDisappear:animated]; | ||
} | ||
|
||
- (void)viewWillTransitionToSize:(CGSize)size | ||
withTransitionCoordinator:(id<UIViewControllerTransitionCoordinator>)coordinator { | ||
[super viewWillTransitionToSize:size withTransitionCoordinator:coordinator]; | ||
|
||
// We interpolate the viewport metrics (size and paddings) during rotation transition, to address | ||
// a bug with distorted aspect ratio. | ||
// See: https://github.com/flutter/flutter/issues/16322 | ||
// | ||
// For every `kRotationViewportMetricsUpdateInterval`, we send the metrics which is interpolated | ||
// between the old metrics before the rotation transition, to the new metrics after the rotation | ||
// transition. | ||
// | ||
// Currently it is using linear interpolation. Using non-linear ease-in/out interpolation may | ||
// achieve better results. It may also help to send only rotation info (such as rotation duration) | ||
// and perform the interpolation on the framework side, to reduce engine/framework communication. | ||
// However, since flutter's drawing happens on the ui thread, which is not iOS main thread, | ||
// there is no guarantee that the viewport metrics change is immediately taken effect, resulting | ||
// in some amount of unavoidable distortion. | ||
|
||
NSTimeInterval transitionDuration = coordinator.transitionDuration; | ||
// Do not interpolate if zero transition duration. | ||
if (transitionDuration == 0) { | ||
return; | ||
} | ||
|
||
// TODO(hellohuanlin): Use [self mainScreenIfViewLoaded] instead of [UIScreen mainScreen]. | ||
// This requires adding the view to window during unit tests, which calls multiple engine calls | ||
// that is hard to mock since they take/return structs. An alternative approach is to partial mock | ||
// the FlutterViewController to make view controller life cycle methods no-op, and insert | ||
// this mock into the responder chain. | ||
CGFloat scale = [UIScreen mainScreen].scale; | ||
_isDuringViewportMetricsInterpolationForRotation = YES; | ||
|
||
CGSize oldSize = self.view.bounds.size; | ||
UIEdgeInsets oldPadding = self.view.safeAreaInsets; | ||
|
||
__block double rotationProgress = 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alternatively use |
||
// Invalidate the timer to avoid race condition when a new rotation starts before the previous | ||
// rotation's timer ends. The `viewWillTransitionToSize` itself is guaranteed to be called after | ||
// the previous rotation is complete. However, there can still be race condition because: | ||
// 1. the transition duration may not be divisible by `kRotationViewportMetricsUpdateInterval`, | ||
// resulting in 1 additional frame. | ||
// 2. there can still be rounding errors when accumulating the progress which is normalized. | ||
// 3. NSTimer is backed by the run loop, which is not accurate timing. | ||
if ([_rotationTimer isValid]) { | ||
[_rotationTimer invalidate]; | ||
} | ||
self.rotationTimer = [NSTimer | ||
scheduledTimerWithTimeInterval:kRotationViewportMetricsUpdateInterval | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe replace kRotationViewportMetricsUpdateInterval with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i can test with promotion device There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Feel free to pull the branch, or can wait for me to fix my laptop's engine build (hopefully this week). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, let me see if I can expense one. |
||
repeats:YES | ||
block:^(NSTimer* timer) { | ||
double progressDelta = | ||
kRotationViewportMetricsUpdateInterval / transitionDuration; | ||
rotationProgress = fmin(1, rotationProgress + progressDelta); | ||
|
||
CGSize newSize = self.view.bounds.size; | ||
UIEdgeInsets newPadding = self.view.safeAreaInsets; | ||
|
||
FLTInterpolateViewportMetrics(_viewportMetrics, rotationProgress, | ||
oldSize, oldPadding, newSize, | ||
newPadding, scale); | ||
[self updateViewportMetricsIfNeeded:YES]; | ||
|
||
// End of rotation. Invalidate the timer. | ||
if (rotationProgress == 1) { | ||
_isDuringViewportMetricsInterpolationForRotation = NO; | ||
[timer invalidate]; | ||
} | ||
}]; | ||
} | ||
|
||
- (void)flushOngoingTouches { | ||
if (_engine && _ongoingTouches.get().count > 0) { | ||
auto packet = std::make_unique<flutter::PointerDataPacket>(_ongoingTouches.get().count); | ||
|
@@ -903,6 +1018,7 @@ - (void)dealloc { | |
[_rotationGestureRecognizer release]; | ||
_pencilInteraction.delegate = nil; | ||
[_pencilInteraction release]; | ||
[_rotationTimer release]; | ||
[super dealloc]; | ||
} | ||
|
||
|
@@ -1278,7 +1394,11 @@ - (void)pencilInteractionDidTap:(UIPencilInteraction*)interaction API_AVAILABLE( | |
|
||
#pragma mark - Handle view resizing | ||
|
||
- (void)updateViewportMetrics { | ||
- (void)updateViewportMetricsIfNeeded:(BOOL)forRotation { | ||
// update only if `_isDuringViewportMetricsInterpolationForRotation` matches `forRotation`. | ||
if (_isDuringViewportMetricsInterpolationForRotation != forRotation) { | ||
return; | ||
} | ||
if ([_engine.get() viewController] == self) { | ||
[_engine.get() updateViewportMetrics:_viewportMetrics]; | ||
} | ||
|
@@ -1299,7 +1419,7 @@ - (void)viewDidLayoutSubviews { | |
_viewportMetrics.physical_height = viewBounds.size.height * scale; | ||
|
||
[self updateViewportPadding]; | ||
[self updateViewportMetrics]; | ||
[self updateViewportMetricsIfNeeded:NO]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At the call sites it's not clear what the boolean does (missing named parameters in Objective-C 😞). How about |
||
|
||
// There is no guarantee that UIKit will layout subviews when the application is active. Creating | ||
// the surface when inactive will cause GPU accesses from the background. Only wait for the first | ||
|
@@ -1329,7 +1449,7 @@ - (void)viewDidLayoutSubviews { | |
|
||
- (void)viewSafeAreaInsetsDidChange { | ||
[self updateViewportPadding]; | ||
[self updateViewportMetrics]; | ||
[self updateViewportMetricsIfNeeded:NO]; | ||
[super viewSafeAreaInsetsDidChange]; | ||
} | ||
|
||
|
@@ -1661,15 +1781,15 @@ - (void)setupKeyboardAnimationVsyncClient { | |
flutterViewController.get()->_viewportMetrics.physical_view_inset_bottom = | ||
flutterViewController.get() | ||
.keyboardAnimationView.layer.presentationLayer.frame.origin.y; | ||
[flutterViewController updateViewportMetrics]; | ||
[flutterViewController updateViewportMetricsIfNeeded:NO]; | ||
} | ||
} else { | ||
fml::TimeDelta timeElapsed = recorder.get()->GetVsyncTargetTime() - | ||
flutterViewController.get().keyboardAnimationStartTime; | ||
|
||
flutterViewController.get()->_viewportMetrics.physical_view_inset_bottom = | ||
[[flutterViewController keyboardSpringAnimation] curveFunction:timeElapsed.ToSecondsF()]; | ||
[flutterViewController updateViewportMetrics]; | ||
[flutterViewController updateViewportMetricsIfNeeded:NO]; | ||
} | ||
}; | ||
flutter::Shell& shell = [_engine.get() shell]; | ||
|
@@ -1698,7 +1818,7 @@ - (void)ensureViewportMetricsIsCorrect { | |
if (_viewportMetrics.physical_view_inset_bottom != self.targetViewInsetBottom) { | ||
// Make sure the `physical_view_inset_bottom` is the target value. | ||
_viewportMetrics.physical_view_inset_bottom = self.targetViewInsetBottom; | ||
[self updateViewportMetrics]; | ||
[self updateViewportMetricsIfNeeded:NO]; | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I had issue with testing -
OCMock/NSInvocation
only supports populating NSObject references, and not structs. And I do not want to exposeviewportMetrics
property as it's anti-pattern. So I made this pure function for easier testing.