Repository navigation
Cleanup VSync infrastructure and improve iOS touch to present latency - #191444
Conversation
github-actions
Bot
added
platform-ios
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the PointerDataDispatcher abstraction and its subclasses, simplifying pointer event dispatching by sending packets directly to the runtime controller. It also removes the touch rate correction vsync client from the iOS platform view and updates VsyncWaiterIOS to manage display link pausing state using a new waiting_for_vsync_ flag. Feedback on these changes highlights potential performance overhead from aggressive pausing/unpausing of CADisplayLink, a risk of zero-interval frames when firing callbacks with identical start and target times, and a potential null pointer dereference when dispatching pointer packets.
| auto vsync_future = clock_->NextVSync(); | ||
|
|
||
| auto async_wait = std::async([&vsync_future, this]() { | ||
| std::thread([this, vsync_future = std::move(vsync_future)]() mutable { |
There was a problem hiding this comment.
This had to be changed from std::future to spawning thread, because the future blocks in destructor, which would mean that AwaitVsync() actually blocks until VSync is signalled, which is all wrong. This only worked because previous implementation of AvaitVsync() was unconditionally scheduling a runloop turn.
knopp
changed the title
f6b38ce to
7a06537
Compare
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
There was a problem hiding this comment.
LGTM other than the PauseDartEventLoopTasks change, restoring those two deleted headless tests, and adding a FlushMicrotasksQueue() call after we dispatch the pointer event so we preserve the old semantics.
Most of the rest of the comments are nits and me just talking to myself.
cbracken
left a comment
cbracken
left a comment
There was a problem hiding this comment.
Just a few nits (which means a re-lgtm unfortunately) but this looks awesome.
57f09a9 to
8df6f6e
Compare
|
fyi this improved the following benchmark, but regressed another improvedaverage raster time regressed90th percentile frame build time @cbracken can you please evaluate if that is expected and desired? |
|
I don't have explanation for this - the PR shouldn ot impact either. I also wasn't able to reproduce this locally. |
…flutter#191444) Fixes flutter#191199 Fixes flutter#190917 This PR reduces touch to present latency on iOS by 1 frame. - Removes `PointerDataDispatcher`, `SmoothPointerDataDispatcher` and `DefaultPointerDataDispatcher` introduced in flutter-team-archive/engine#12385. The PR was meant to solve the event jitter, but the main reason for event jitter was that touch events are delivered on platform thread while our `CADisplayLink` was running on separate UI thread. With `CADisplayLink` now running on platform thread this is no longer an issue, because the display link dispatch is a guaranteed UIKit phase. - Removes `touchRateCorrectionVSyncClient` introduced in flutter-team-archive/engine#35592. This is no longer necessary because we're running our `CADisplayLink` in main thread always. - Ensures that the path from `touchesMoved` all the way to `VSyncWaiter::AwaitVSync` is completely synchronous without scheduling unnecessary event loop turns. This is required so that `CADisplayLink` callback is always called after `AwaitVSync`. Other embedders will likely benefit from this change too. - Removes secondary vsync callbacks. The only remaining usecase for these was to clean unused traceIds, but with synchronous path from `touchesMoved` to `VSyncWaiter::AwaitVSync` this can be easily done right after event dispatch, no secondary vsync callback is necessary. - Modifies `vsync_waiter_ios` to not waste time when scheduling the first frame and does not pause the display link after each tick. - Removes `CADisplayLink` from `FlutterMetalLayer`. This has been unnecessary since merged threads. With this PR, this is the flow for each frame during tracking on iOS (and other platform with merged threads): ``` ----- FRAME BOUNDARY ----------------------------------------- +-- touchesMoved: event dispatch phase | \/ synchronous operation --+ pointer processing in framework requests new frame (same runloop turn) | \/ +-- vsync request +-- display link callback display link phase ---| \/ +-- framework produces frame and sents it to raster thread ----- FRAME BOUNDARY ----------------------------------------- ``` Note that UIKit phases are [well defined](https://developer.apple.com/documentation/uikit/uiupdateactionphase?language=objc) and event dispatch is guaranteed to come before display link callback. *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 - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
|
Thank you so much @knopp and @cbracken for your work on this! I've been looking forward to this specific UX improvement for Flutter for a long time, and I'm delighted to have read about this being rolled into the latest Flutter release. This progress makes me very excited about Flutter’s future. I'm grateful for all the investigation, testing, and care that went into getting this shipped 🩷 Equally pleased to see that #193073 is underway. Exciting times! Thank you! |
Fixes #191199
Fixes #190917
This PR reduces touch to present latency on iOS by 1 frame.
PointerDataDispatcher,SmoothPointerDataDispatcherandDefaultPointerDataDispatcherintroduced in Reland "Smooth out iOS irregular input events delivery (#12280)" flutter-team-archive/engine#12385. The PR was meant to solve the event jitter, but the main reason for event jitter was that touch events are delivered on platform thread while ourCADisplayLinkwas running on separate UI thread. WithCADisplayLinknow running on platform thread this is no longer an issue, because the display link dispatch is a guaranteed UIKit phase.touchRateCorrectionVSyncClientintroduced in [iOS] Avoid jitter and laggy when user is dragging on iOS Promotion devices flutter-team-archive/engine#35592. This is no longer necessary because we're running ourCADisplayLinkin main thread always.touchesMovedall the way toVSyncWaiter::AwaitVSyncis completely synchronous without scheduling unnecessary event loop turns. This is required so thatCADisplayLinkcallback is always called afterAwaitVSync. Other embedders will likely benefit from this change too.touchesMovedtoVSyncWaiter::AwaitVSyncthis can be easily done right after event dispatch, no secondary vsync callback is necessary.vsync_waiter_iosto not waste time when scheduling the first frame and does not pause the display link after each tick.CADisplayLinkfromFlutterMetalLayer. This has been unnecessary since merged threads.With this PR, this is the flow for each frame during tracking on iOS (and other platform with merged threads):
Note that UIKit phases are well defined and event dispatch is guaranteed to come before display link callback.
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
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.