Reland: [macOS] Use CVDisplayLink to drive repaint by knopp · Pull Request #51126 · flutter/engine

knopp · 2024-03-01T15:36:10Z

This relands the PR reverted in #51095

Changes since the original PR:

The macOS embedder does not assume particular clock when calling the embedder API, but converts CAMediaTime to engine time and back (FlutterTimeConverter)
FlutterVSyncWaiter does not wait for displaylink callback during warmup frame. This should prevent timeToFirstFrameRasterizedMicros regressions.
When enforcing frame pacing the raster thread is not blocked. This should prevent average_frame_rasterizer_time_millis regressions.

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 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.

shell/platform/darwin/macos/framework/Source/FlutterEngine.mm

cbracken · 2024-03-04T18:37:45Z

shell/platform/darwin/macos/framework/Source/FlutterCompositor.mm

+  }
+
+  // Notify block below may be called asynchronously, hence the need to copy
+  // the layer information instead of passing the original pointers from embedder.


Thanks for adding this comment to explain why we couldn't for example, just use a std::span or otherwise to wrap the layers being passed down, which will potentially be collected before they're used by us.

cbracken

New changes lgtm.

…9159)" (flutter#51095)" This reverts commit dd3383d.

…ngine#51126)

flutter/engine@0d8588b...4001881 2024-03-04 matej.knopp@gmail.com Reland: [macOS] Use CVDisplayLink to drive repaint (flutter/engine#51126) 2024-03-04 jason-simmons@users.noreply.github.com Support gtest-parallel when running Impeller unit tests (flutter/engine#51079) 2024-03-04 matanlurey@users.noreply.github.com Scenario App: Adds a `run_{count}.{backend}.` file prefix to every run (on CI) (flutter/engine#51102) 2024-03-04 matanlurey@users.noreply.github.com Use Instrumentation.waitForIdleSync() after rotation requests. (flutter/engine#51169) 2024-03-04 skia-flutter-autoroll@skia.org Roll Skia from 9c7d13c05e77 to f65ecbdfb09c (1 revision) (flutter/engine#51170) 2024-03-04 dnfield@google.com Guard against API 22 (flutter/engine#51167) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC aaclarke@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: 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

cbracken · 2024-03-05T03:19:43Z

Reason for revert: looks like this regressed Skia benchmarks: https://flutter-flutter-perf.skia.org/e/?keys=Xbf8dce9c233bce34d20e2280e3f1d8db&selected=commit%3D39653%26name%3D%2Carch%3Dintel%2Cbranch%3Dmaster%2Cconfig%3Ddefault%2Cdevice_type%3Dnone%2Cdevice_version%3Dnone%2Chost_type%3Dmac%2Csub_result%3DtimeToFirstFrameRasterizedMicros%2Ctest%3Dflutter_gallery_macos__start_up%2C&xbaroffset=39569

The regression starts with the engine roll containing this CVDisplayLink patch: 0d8588b...4001881

auto-submit · 2024-03-05T03:19:46Z

A reason for requesting a revert of flutter/engine/51126 could
not be found or the reason was not properly formatted. Begin a comment with 'Reason for revert:' to tell the bot why
this issue is being reverted.

This reverts commit 4001881.

…#51192) Reverts #51126 Initiated by: cbracken Reason for reverting: looks like this regressed Skia benchmarks: https://flutter-flutter-perf.skia.org/e/?keys=Xbf8dce9c233bce34d20e2280e3f1d8db&selected=commit%3D39653%26name%3D%2Carch%3Dintel%2Cbranch%3Dmaster%2Cconfig%3Ddefault%2Cdevice_type%3Dnone%2Cdevice_version%3Dnone%2Chost_type%3Dmac%2Csub_result%3DtimeToFirstFrameRasterizedMicros%2Ctest%3Dflutter_gallery_macos__start_up%2C&xbaroffset=39569 The regression s Original PR Author: knopp Reviewed By: {cbracken} This change reverts the following previous change: Original Description: This relands the PR reverted in #51095 Changes since the original PR: - The macOS embedder does not assume particular clock when calling the embedder API, but converts CAMediaTime to engine time and back (`FlutterTimeConverter`) - `FlutterVSyncWaiter` does not wait for displaylink callback during warmup frame. This should prevent `timeToFirstFrameRasterizedMicros` regressions. - When enforcing frame pacing the raster thread is not blocked. This should prevent `average_frame_rasterizer_time_millis` regressions. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

…lutter#51126)" (flutter#51192)" This reverts commit 78cecee.

[Original reland](#51126) was [reverted](78cecee) because of skiaperf regressions. The regressions are caused by runner machines running display at 30hz. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat

knopp requested review from cbracken and chinmaygarde as code owners March 1, 2024 15:36

github-actions bot added affects: desktop platform-macos labels Mar 1, 2024

cbracken reviewed Mar 4, 2024

View reviewed changes

shell/platform/darwin/macos/framework/Source/FlutterEngine.mm Show resolved Hide resolved

cbracken reviewed Mar 4, 2024

View reviewed changes

cbracken approved these changes Mar 4, 2024

View reviewed changes

knopp added 9 commits March 4, 2024 21:13

Revert "Revert "[macOS] Use CVDisplayLink to drive repaint (flutter#4…

b5042c1

…9159)" (flutter#51095)" This reverts commit dd3383d.

Convert time to and from engine time

b42069d

Speculative fix for timeToFirstFrameRasterizedMicros regression

1b0f028

Fix FlutterEngineTest.Compositor flakyness

92c1872

Do not block raster thread when delaying present

352038a

Fix guards

9451227

Add licenses

b1bfc96

Remove leftover code

7dbb156

Add issue link

e086d7a

knopp force-pushed the reland_display_link branch from 8bee545 to e086d7a Compare March 4, 2024 20:15

knopp merged commit 4001881 into flutter:main Mar 4, 2024

knopp deleted the reland_display_link branch March 4, 2024 20:57

engine-flutter-autoroll mentioned this pull request Mar 4, 2024

Roll Flutter Engine from 0d8588b1de7a to 40018811ec0e (6 revisions) flutter/flutter#144570

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 4, 2024

4001881 Reland: [macOS] Use CVDisplayLink to drive repaint (flutter/e…

4e7c036

…ngine#51126)

cbracken added the revert Label used to revert changes in a closed and merged pull request. label Mar 5, 2024

auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Mar 5, 2024

cbracken added the revert Label used to revert changes in a closed and merged pull request. label Mar 5, 2024

auto-submit bot pushed a commit that referenced this pull request Mar 5, 2024

Revert "Reland: [macOS] Use CVDisplayLink to drive repaint (#51126)"

856f398

This reverts commit 4001881.

auto-submit bot mentioned this pull request Mar 5, 2024

Reverts "Reland: [macOS] Use CVDisplayLink to drive repaint (#51126)" #51192

Merged

8 tasks

auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Mar 5, 2024

knopp added a commit to knopp/engine that referenced this pull request Mar 5, 2024

Revert "Reverts "Reland: [macOS] Use CVDisplayLink to drive repaint (f…

864e6c1

…lutter#51126)" (flutter#51192)" This reverts commit 78cecee.

knopp mentioned this pull request Mar 5, 2024

Reland "Reland: [macOS] Use CVDisplayLink to drive repaint" #51210

Merged

8 tasks

knopp mentioned this pull request Mar 12, 2024

FrameTiming API is inaccurate for elapsed time flutter/flutter#144782

Closed

knopp added a commit to knopp/engine that referenced this pull request Mar 13, 2024

Revert "Reverts "Reland: [macOS] Use CVDisplayLink to drive repaint (f…

49610cc

…lutter#51126)" (flutter#51192)" This reverts commit 78cecee.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reland: [macOS] Use CVDisplayLink to drive repaint#51126

Reland: [macOS] Use CVDisplayLink to drive repaint#51126
knopp merged 9 commits intoflutter:mainfrom
knopp:reland_display_link

knopp commented Mar 1, 2024 •

edited

Loading

Uh oh!

Uh oh!

cbracken Mar 4, 2024

Uh oh!

cbracken left a comment

Uh oh!

cbracken commented Mar 5, 2024 •

edited

Loading

Uh oh!

auto-submit bot commented Mar 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

knopp commented Mar 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Uh oh!

cbracken Mar 4, 2024

Choose a reason for hiding this comment

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

cbracken commented Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

auto-submit bot commented Mar 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

knopp commented Mar 1, 2024 •

edited

Loading

cbracken commented Mar 5, 2024 •

edited

Loading