[macOS] Use CVDisplayLink to drive repaint #49159

knopp · 2023-12-17T19:51:33Z

This PR synchronises updates with display refresh allowing for true 120hz repaint. It also enforces frame pacing resulting in smoother experience at both 60hz and 120hz.

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

knopp · 2023-12-21T18:20:24Z

@cbracken, I think this is in good enough shape for review.

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

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

lesnitsky · 2024-02-01T15:58:09Z

@knopp is anything blocking this PR?

knopp · 2024-02-01T16:20:43Z

@knopp is anything blocking this PR?

Not that I know of, possibly a bandwidth issue. @cbracken ?

loic-sharma · 2024-02-01T19:20:53Z

@knopp Chris is out sick and may be delayed in reviewing this. Apologies for the delay.

cbracken

A few more quick comments.

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

cbracken · 2024-02-20T23:16:40Z

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

@@ -1029,6 +1088,13 @@ - (void)engineCallbackOnPreEngineRestart {
  }
 }

+- (void)onVSync:(uintptr_t)baton {
+  @synchronized(_vsyncWaiters) {
+    FlutterVSyncWaiter* waiter = [_vsyncWaiters objectForKey:@(kFlutterImplicitViewId)];


I'm assuming we'll need to generalise this for multi-views, in which case, you'll want to add a TODO here to wire up the proper view ID with an issue link. You may need to file a new subissue under flutter/flutter#142845 if nothing existing fits.

I added a TODO for this. Right now it seems a bit unclear how exactly vsync is going to work with multiview. We might not have a viewId available.

At least for the multiview MVP we'll probably use some heuristic for this - i.e. if views are on different displays use waiter for view on primary display, or with display with higher refresh. We'll cross that bridge when we get there, but either way this should be a minor change.

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

shell/platform/darwin/macos/framework/Source/FlutterThreadSynchronizer.h

cbracken

Awesome - thanks so much for your patience on this one! Looks great! Ship it! 🚀

cbracken · 2024-02-27T17:52:47Z

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

+    if (screen != nil) {
+      // https://developer.apple.com/documentation/appkit/nsscreen/1388360-devicedescription?language=objc
+      _display_id = (CGDirectDisplayID)[
+          [[screen deviceDescription] objectForKey:@"NSScreenNumber"] unsignedIntValue];


Sure would have been nice if they'd provided a const NSString for this key! Thanks for the doc link to explain it.

cbracken · 2024-02-27T17:55:05Z

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

+#include "flutter/fml/synchronization/waitable_event.h"
+#include "flutter/testing/testing.h"
+
+#import <AppKit/AppKit.h>


nit: standard C/Obj-C headers like this go after the associated header (FlutterDisplayLink.h) but before the local flutter headers.

At some point we'll get round do adding a better header lint, though the associated header is sometimes tricky to figure out.

2461280c38 Roll Fuchsia Linux SDK from JCdhkDSFXzHyPuP4I... to T1xAi_ww_mWEiDkVN... (flutter/engine#51011) 8940ea0887 Code consistency fixes in FlScrollingManager (flutter/engine#50959) 21474ee4a4 [macOS] Use CVDisplayLink to drive repaint (flutter/engine#49159) 6f7b939568 Fail lazily when 1+ Skia gold comparions fail. (flutter/engine#51010) 249daf5512 Adjust Android emulator test timeouts (flutter/engine#51004)

This reverts commit 21474ee.

The original PR assumed that `fml::TimePoint` has same base as `CACurrentMediaTime()`. However due to recent change in Dart SDK that's no longer true. Dart SDK [replaced](https://dart-review.googlesource.com/c/sdk/+/348044?tab=comments) call to `mach_absolute_time` with `clock_gettime_nsec_np(CLOCK_MONOTONIC_RAW)`, while `CACurrentMediaTime()` corresponds to `CLOCK_UPTIME_RAW`. This needs to be fixed either in Dart SDK dart-lang/sdk#55071 or the original PR needs to be relanded with appropriate conversions. This reverts commit 21474ee. *Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.* *List which issues are fixed by this PR. You must list at least one issue.* *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 [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

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

alterhuman · 2024-03-13T14:39:53Z

Is there any possibility of this getting merged again?

knopp · 2024-03-13T14:41:07Z

Is there any possibility of this getting merged again?

#51210

alterhuman · 2024-03-13T15:12:03Z

@knopp thank you so much for this! Can't wait to see the difference. Very excited.

This comment was marked as outdated.

Sign in to view

github-actions bot added affects: desktop platform-macos labels Dec 17, 2023

knopp requested review from cbracken and chinmaygarde as code owners December 19, 2023 17:52

knopp force-pushed the macos_display_link branch from 4189a33 to b60b239 Compare December 20, 2023 11:23

knopp changed the title ~~WIP [macOS] Use CVDisplayLink to drive repaint~~ [macOS] Use CVDisplayLink to drive repaint Dec 20, 2023

knopp force-pushed the macos_display_link branch 2 times, most recently from 9423f12 to e06c2c6 Compare December 20, 2023 19:51

cbenhagen mentioned this pull request Dec 21, 2023

[video_player_avfoundation] Flickering on macOS flutter/flutter#135999

Open

2 tasks

knopp force-pushed the macos_display_link branch 2 times, most recently from 4d508d2 to c579a92 Compare December 25, 2023 13:49

cbracken reviewed Jan 18, 2024

View reviewed changes

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

cbracken reviewed Jan 18, 2024

View reviewed changes

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

knopp force-pushed the macos_display_link branch from c579a92 to 41da795 Compare January 19, 2024 12:41

loic-sharma mentioned this pull request Jan 29, 2024

Even very simple animations experience jittering and dropped frames. flutter/flutter#142218

Open

cbracken reviewed Feb 20, 2024

View reviewed changes

knopp force-pushed the macos_display_link branch 2 times, most recently from b9c3e77 to 5c2c131 Compare February 26, 2024 13:28

cbracken approved these changes Feb 27, 2024

View reviewed changes

knopp added 6 commits February 27, 2024 19:10

[macOS] Use CVDisplayLink to drive repaint

d546a7d

Nits

02ca690

extract constant out

b393e2c

rename main thread to platform thread

a34e734

add TODO

85840f4

header gueards

58931a0

knopp added 2 commits February 27, 2024 19:10

Use correct timescale for host time

0f32c9a

nit: reorder header

a4910bc

knopp force-pushed the macos_display_link branch from d8055c6 to a4910bc Compare February 27, 2024 18:10

knopp merged commit 21474ee into flutter:main Feb 27, 2024
25 checks passed

knopp deleted the macos_display_link branch February 27, 2024 19:48

zanderso mentioned this pull request Feb 28, 2024

Manual roll Flutter Engine 5e0d9ba35dd5 to 2461280c38b7 flutter/flutter#144288

Merged

gaaclarke mentioned this pull request Feb 28, 2024

5x regression in flutter_gallery_macos__start_up:timeToFirstFrameRasterizedMicros flutter/flutter#144341

Closed

knopp added a commit to knopp/engine that referenced this pull request Feb 29, 2024

Revert "[macOS] Use CVDisplayLink to drive repaint (flutter#49159)"

7c6f6ca

This reverts commit 21474ee.

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

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

184b026

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

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

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

b5042c1

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[macOS] Use CVDisplayLink to drive repaint #49159

[macOS] Use CVDisplayLink to drive repaint #49159

knopp commented Dec 17, 2023 •

edited

This comment was marked as outdated.

knopp commented Dec 21, 2023

lesnitsky commented Feb 1, 2024

knopp commented Feb 1, 2024 •

edited

loic-sharma commented Feb 1, 2024

cbracken left a comment

cbracken Feb 20, 2024

knopp Feb 21, 2024 •

edited

cbracken left a comment

cbracken Feb 27, 2024

cbracken Feb 27, 2024

alterhuman commented Mar 13, 2024

knopp commented Mar 13, 2024 •

edited

alterhuman commented Mar 13, 2024

[macOS] Use CVDisplayLink to drive repaint #49159

[macOS] Use CVDisplayLink to drive repaint #49159

Conversation

knopp commented Dec 17, 2023 • edited

Pre-launch Checklist

This comment was marked as outdated.

knopp commented Dec 21, 2023

lesnitsky commented Feb 1, 2024

knopp commented Feb 1, 2024 • edited

loic-sharma commented Feb 1, 2024

cbracken left a comment

Choose a reason for hiding this comment

cbracken Feb 20, 2024

Choose a reason for hiding this comment

knopp Feb 21, 2024 • edited

Choose a reason for hiding this comment

cbracken left a comment

Choose a reason for hiding this comment

cbracken Feb 27, 2024

Choose a reason for hiding this comment

cbracken Feb 27, 2024

Choose a reason for hiding this comment

alterhuman commented Mar 13, 2024

knopp commented Mar 13, 2024 • edited

alterhuman commented Mar 13, 2024

knopp commented Dec 17, 2023 •

edited

knopp commented Feb 1, 2024 •

edited

knopp Feb 21, 2024 •

edited

knopp commented Mar 13, 2024 •

edited