[ios] Correct handling for CADisplayLink paused-to-unpaused transitions#186457
Conversation
cca695a to
1c7d9f5
Compare
There was a problem hiding this comment.
Code Review
This pull request enhances the robustness of the iOS vsync client by handling scenarios where CADisplayLink timestamps are zero, which typically occurs during transitions from a paused state. It introduces logic to synthesize target timestamps using the current refresh rate and ensures the refresh rate is dynamically updated and rounded to the nearest Hz. Additionally, the PR includes comprehensive documentation for the vsync and display link manager classes and adds unit tests to verify these timing improvements. Review feedback highlights the need to guard against potential division-by-zero errors if the refresh rate is zero and suggests enforcing a minimum refresh rate value during recalculation.
|
Note to reviewers: this is stacked on top of #186457 Also did a bunch of manual verification on an iPad Pro 11 (M5) Simulator. |
1c7d9f5 to
18683df
Compare
hellohuanlin
left a comment
There was a problem hiding this comment.
Before the first frame is delivered, or when the display link is paused, these values evaluate to 0.0.
I wonder if it makes sense to keep the display link running, and when we want to pause, just toggle a boolean flag to avoid calling the engine callback?
18683df to
d49d072
Compare
We disable it to reduce CPU / drain on the battery, but I don't actually have stats on how much this saves us. I'm not aware of us having measured power efficiency in the past; though I know we've discussed it at various points. If we have measurements showing the effect of the additional callbacks is negligible, it would slightly simplify the code, but it's not too bad as-is. At least now that the first callback behaviour is documented. |
According to Apple's documentation `CADisplayLink.targetTimestamp` and `timestamp` properties are only valid after the display link has delivered at least one frame. Before the first frame is delivered, or when the isplay link is paused, these values evaluate to `0.0`. Because our vsync client uses an on-demand request-and-pause cycle to preserve battery, every new frame sequence begins with a paused-to-unpaused transition. Previously, receiving `0.0` on the initial vsync callback resulted in negative or incorrect frame durations and delay calculations passed downstream to the engine's frame timings recorder. Due to the way values were rebased relative to `fml::TimePoint`s, the end result was that the first frame was recorded with a duration of `0.0` and a target time equal to its start time. This commit ensures consistent frame progression by setting the start timestamp to `CACurrentMediaTime()` and synthesizing a frame end time as frame start + the inverse of the refresh rate. This behaviour is consistent with equivalent code in the macOS embedder's `FlutterDisplayLink.mm`. This also fixes a bug in `FlutterKeyboardInsetManager` wherein the start time was recorded using `ToSeconds()` (rounded to nearest integer second) rather than `ToSecondsF()` which records the current timestamp accurately. This adds tests for: * Verify VSyncClient behaviour on the paused-to-unpaused transition. * Integration test verifying that the keyboard inset is animated correctly on first frame.
d49d072 to
404a1f6
Compare
|
Rebased to tip of tree which undid the approval. No changes since last approval other than the rebase; good for another review pass. |
flutter/flutter@3598686...259aeae 2026-05-19 engine-flutter-autoroll@skia.org Roll Skia from 967ddb1aa561 to f1b406860c5e (2 revisions) (flutter/flutter#186731) 2026-05-19 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 5Ki-dBY4SpWdQMF_3... to -F9Ci3Opxt06MixDl... (flutter/flutter#186727) 2026-05-19 116356835+AbdeMohlbi@users.noreply.github.com Remove unused field in `ResourceExtractor` (flutter/flutter#186629) 2026-05-19 mbrase@google.com Update Fuchsia tests to use realm_builder_server as a subpackage (flutter/flutter#186409) 2026-05-19 engine-flutter-autoroll@skia.org Roll Skia from cebf49d034b8 to 967ddb1aa561 (4 revisions) (flutter/flutter#186720) 2026-05-19 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#186721) 2026-05-19 chris@bracken.jp [macOS][gn] support both x64/arm64 macOS host clang toolchains for ASAN (flutter/flutter#186669) 2026-05-19 chris@bracken.jp [macOS][gn] Use arm64 clang in generate_coverage.py on arm64 Macs (flutter/flutter#186662) 2026-05-19 awolff@google.com Fix broken link in impeller golden_tests readme (flutter/flutter#186470) 2026-05-19 engine-flutter-autoroll@skia.org Roll Skia from 27f7bba22600 to cebf49d034b8 (37 revisions) (flutter/flutter#186699) 2026-05-19 chris@bracken.jp [macOS][gn] Use arm64 clang in verify_exported.dart on arm64 Macs (flutter/flutter#186664) 2026-05-19 chris@bracken.jp [macOS][gn] Use arm64 clang in sanitizer_suppressions.sh on arm64 Macs (flutter/flutter#186663) 2026-05-19 chris@bracken.jp [macOS][gn] Use arm64 clang in copy_info_plist.py on arm64 Macs (flutter/flutter#186661) 2026-05-19 58529443+srujzs@users.noreply.github.com Complete completer only once in hot restart tests (flutter/flutter#186702) 2026-05-18 30870216+gaaclarke@users.noreply.github.com Testing autosubmit bot -- updating testowners (flutter/flutter#185226) 2026-05-18 chris@bracken.jp [ios] Correct handling for CADisplayLink paused-to-unpaused transitions (flutter/flutter#186457) 2026-05-18 chris@bracken.jp [Android][macOS][gn] support both x64/arm64 macOS host clang toolchains (flutter/flutter#186660) 2026-05-18 engine-flutter-autoroll@skia.org Roll Packages from 32c84d6 to b9bdd37 (2 revisions) (flutter/flutter#186683) 2026-05-18 jesswon@google.com [AGP 9] Upgrade Flutter Test Apps to AGP 9 (flutter/flutter#186200) 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 Please CC stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: 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
…) (flutter#186935) Migrates `FlutterVSyncClient` from a hybrid C++/Obj-C implementation to a pure Objective-C implementation by removing C++ types from its interface and implementation. Previously, `FlutterVSyncClient` stored a C++ `flutter::VsyncWaiter::Callback` and used a temporary `std::unique_ptr<flutter::FrameTimingsRecorder>` to propagate vsync timestamps. This was redundant: * `FlutterVSyncClient` allocated a temporary `FrameTimingsRecorder` and recorded vsync times. * It passed this to a C++ lambda wrapper. * The lambda extracted the start/target times as seconds and passed them to the Obj-C block callback. * The temporary recorder was immediately discarded. * `VsyncWaiterIOS` converted the seconds back to `fml::TimePoint`. * `VsyncWaiter::FireCallback` allocated *another* `FrameTimingsRecorder` and recorded the same times again. We now: * manage the callback as pure Obj-C block that accepts raw `CFTimeInterval` (double seconds) directly from `CADisplayLink`. * Remove the redundant `FrameTimingsRecorder` and C++ lambda wrapper. * Rely solely on `VsyncWaiter::FireCallback` to handle the final `FrameTimingsRecorder` creation and recording, which it already does. This is a re-land of flutter#186166 which was reverted in flutter#186266. That patch failed to correctly adjust the epoch of timestamps obtained from CoreAnimation (which uses `mach_absolute_time` under the hood) to the epoch used by the core engine, which is built on `fml::TimePoint`, which uses `std::chrono::steady_clock`, which uses `mach_continuous_time` under the hood, consistent with the C++ specification's of a continuous clock. While these two clocks start roughly synchronized at boot, `mach_absolute_time` pauses on device sleep, whereas `mach_continuous_time` does not. The iOS embedder now uses timings based on CoreAnimation throughout and translates to `fml::TimePoint` only at the engine-embedder boundary. This is consistent with embedders based on the embedder API, such as macOS, which does this work in `-[FlutterEngine viewControllerViewDidLoad]` using `FlutterTimeConverter`. Added tests for frame rate snapping/fallback. No other test changes because this introduces no semantic change and is covered by existing tests recently added in: flutter#186457 Issue: flutter#112232 <!-- Thanks for filing a pull request! Reviewers are typically assigned within a week of filing a request. To learn more about code review, see our documentation on Tree Hygiene: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md --> ## 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 tested the crap outta this cause I really don't want to write another post-mortem. - [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
…r#11737) flutter/flutter@3598686...259aeae 2026-05-19 engine-flutter-autoroll@skia.org Roll Skia from 967ddb1aa561 to f1b406860c5e (2 revisions) (flutter/flutter#186731) 2026-05-19 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 5Ki-dBY4SpWdQMF_3... to -F9Ci3Opxt06MixDl... (flutter/flutter#186727) 2026-05-19 116356835+AbdeMohlbi@users.noreply.github.com Remove unused field in `ResourceExtractor` (flutter/flutter#186629) 2026-05-19 mbrase@google.com Update Fuchsia tests to use realm_builder_server as a subpackage (flutter/flutter#186409) 2026-05-19 engine-flutter-autoroll@skia.org Roll Skia from cebf49d034b8 to 967ddb1aa561 (4 revisions) (flutter/flutter#186720) 2026-05-19 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#186721) 2026-05-19 chris@bracken.jp [macOS][gn] support both x64/arm64 macOS host clang toolchains for ASAN (flutter/flutter#186669) 2026-05-19 chris@bracken.jp [macOS][gn] Use arm64 clang in generate_coverage.py on arm64 Macs (flutter/flutter#186662) 2026-05-19 awolff@google.com Fix broken link in impeller golden_tests readme (flutter/flutter#186470) 2026-05-19 engine-flutter-autoroll@skia.org Roll Skia from 27f7bba22600 to cebf49d034b8 (37 revisions) (flutter/flutter#186699) 2026-05-19 chris@bracken.jp [macOS][gn] Use arm64 clang in verify_exported.dart on arm64 Macs (flutter/flutter#186664) 2026-05-19 chris@bracken.jp [macOS][gn] Use arm64 clang in sanitizer_suppressions.sh on arm64 Macs (flutter/flutter#186663) 2026-05-19 chris@bracken.jp [macOS][gn] Use arm64 clang in copy_info_plist.py on arm64 Macs (flutter/flutter#186661) 2026-05-19 58529443+srujzs@users.noreply.github.com Complete completer only once in hot restart tests (flutter/flutter#186702) 2026-05-18 30870216+gaaclarke@users.noreply.github.com Testing autosubmit bot -- updating testowners (flutter/flutter#185226) 2026-05-18 chris@bracken.jp [ios] Correct handling for CADisplayLink paused-to-unpaused transitions (flutter/flutter#186457) 2026-05-18 chris@bracken.jp [Android][macOS][gn] support both x64/arm64 macOS host clang toolchains (flutter/flutter#186660) 2026-05-18 engine-flutter-autoroll@skia.org Roll Packages from 32c84d6 to b9bdd37 (2 revisions) (flutter/flutter#186683) 2026-05-18 jesswon@google.com [AGP 9] Upgrade Flutter Test Apps to AGP 9 (flutter/flutter#186200) 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 Please CC stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: 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
…) (flutter#186935) Migrates `FlutterVSyncClient` from a hybrid C++/Obj-C implementation to a pure Objective-C implementation by removing C++ types from its interface and implementation. Previously, `FlutterVSyncClient` stored a C++ `flutter::VsyncWaiter::Callback` and used a temporary `std::unique_ptr<flutter::FrameTimingsRecorder>` to propagate vsync timestamps. This was redundant: * `FlutterVSyncClient` allocated a temporary `FrameTimingsRecorder` and recorded vsync times. * It passed this to a C++ lambda wrapper. * The lambda extracted the start/target times as seconds and passed them to the Obj-C block callback. * The temporary recorder was immediately discarded. * `VsyncWaiterIOS` converted the seconds back to `fml::TimePoint`. * `VsyncWaiter::FireCallback` allocated *another* `FrameTimingsRecorder` and recorded the same times again. We now: * manage the callback as pure Obj-C block that accepts raw `CFTimeInterval` (double seconds) directly from `CADisplayLink`. * Remove the redundant `FrameTimingsRecorder` and C++ lambda wrapper. * Rely solely on `VsyncWaiter::FireCallback` to handle the final `FrameTimingsRecorder` creation and recording, which it already does. This is a re-land of flutter#186166 which was reverted in flutter#186266. That patch failed to correctly adjust the epoch of timestamps obtained from CoreAnimation (which uses `mach_absolute_time` under the hood) to the epoch used by the core engine, which is built on `fml::TimePoint`, which uses `std::chrono::steady_clock`, which uses `mach_continuous_time` under the hood, consistent with the C++ specification's of a continuous clock. While these two clocks start roughly synchronized at boot, `mach_absolute_time` pauses on device sleep, whereas `mach_continuous_time` does not. The iOS embedder now uses timings based on CoreAnimation throughout and translates to `fml::TimePoint` only at the engine-embedder boundary. This is consistent with embedders based on the embedder API, such as macOS, which does this work in `-[FlutterEngine viewControllerViewDidLoad]` using `FlutterTimeConverter`. Added tests for frame rate snapping/fallback. No other test changes because this introduces no semantic change and is covered by existing tests recently added in: flutter#186457 Issue: flutter#112232 <!-- Thanks for filing a pull request! Reviewers are typically assigned within a week of filing a request. To learn more about code review, see our documentation on Tree Hygiene: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md --> ## 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 tested the crap outta this cause I really don't want to write another post-mortem. - [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
According to Apple's documentation
CADisplayLink.targetTimestampandtimestampproperties are only valid after the display link hasdelivered at least one frame. Before the first frame is delivered, or
when the isplay link is paused, these values evaluate to
0.0.Because our vsync client uses an on-demand request-and-pause cycle to
preserve battery, every new frame sequence begins with a
paused-to-unpaused transition. Previously, receiving
0.0on theinitial vsync callback resulted in negative or incorrect frame durations
and delay calculations passed downstream to the engine's frame timings
recorder. Due to the way values were rebased relative to
fml::TimePoints, the end result was that the first frame was recordedwith a duration of
0.0and a target time equal to its start time.This commit ensures consistent frame progression by setting the start
timestamp to
CACurrentMediaTime()and synthesizing a frame end time asframe start + the inverse of the refresh rate. This behaviour is
consistent with equivalent code in the macOS embedder's
FlutterDisplayLink.mm.This also fixes a bug in
FlutterKeyboardInsetManagerwherein the starttime was recorded using
ToSeconds()(rounded to nearest integersecond) rather than
ToSecondsF()which records the current timestampaccurately.
This adds tests for:
correctly on first frame.
Issue: #112232
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.