[macOS] Use CVDisplayLink to drive repaint #49159
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
knopp
force-pushed
the
macos_display_link
branch
from
4189a33 to
b60b239
Compare
knopp
changed the title
knopp
force-pushed
the
macos_display_link
branch
2 times, most recently
from
9423f12 to
e06c2c6
Compare
|
@cbracken, I think this is in good enough shape for review. |
knopp
force-pushed
the
macos_display_link
branch
2 times, most recently
from
4d508d2 to
c579a92
Compare
shell/platform/darwin/macos/framework/Source/FlutterCompositor.mm
Outdated
Show resolved
Hide resolved
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
|
@knopp is anything blocking this PR? |
|
@knopp Chris is out sick and may be delayed in reviewing this. Apologies for the delay. |
|
|
||
| - (void)onVSync:(uintptr_t)baton { | ||
| @synchronized(_vsyncWaiters) { | ||
| FlutterVSyncWaiter* waiter = [_vsyncWaiters objectForKey:@(kFlutterImplicitViewId)]; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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/FlutterThreadSynchronizer.h
Outdated
Show resolved
Hide resolved
knopp
force-pushed
the
macos_display_link
branch
2 times, most recently
from
b9c3e77 to
5c2c131
Compare
cbracken
left a comment
cbracken
left a comment
There was a problem hiding this comment.
Awesome - thanks so much for your patience on this one! Looks great! Ship it! 🚀
| if (screen != nil) { | ||
| // https://developer.apple.com/documentation/appkit/nsscreen/1388360-devicedescription?language=objc | ||
| _display_id = (CGDirectDisplayID)[ | ||
| [[screen deviceDescription] objectForKey:@"NSScreenNumber"] unsignedIntValue]; |
There was a problem hiding this comment.
Sure would have been nice if they'd provided a const NSString for this key! Thanks for the doc link to explain it.
| #include "flutter/fml/synchronization/waitable_event.h" | ||
| #include "flutter/testing/testing.h" | ||
|
|
||
| #import <AppKit/AppKit.h> |
There was a problem hiding this comment.
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.
knopp
force-pushed
the
macos_display_link
branch
from
d8055c6 to
a4910bc
Compare
|
Is there any possibility of this getting merged again? |
|
|
@knopp thank you so much for this! Can't wait to see the difference. Very excited. |
5 participants
Fixes flutter/flutter#49757
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
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.