-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adopt CADisplayLink on macOS 14 #28665
base: main
Are you sure you want to change the base?
Conversation
EWS run on previous version of this PR (hash b4a2369)
|
EWS run on previous version of this PR (hash 297d23e)
|
EWS run on previous version of this PR (hash 88d09e0)
|
EWS run on previous version of this PR (hash 5ff129e)
|
https://bugs.webkit.org/show_bug.cgi?id=274268 rdar://119217720 Reviewed by NOBODY (OOPS!). The CADisplayLink API is newly available on macOS 14. We add a new `HAVE(CA_DISPLAY_LINK_MAC)` directive and if it's not available, use a legacy codepath where `CVDisplayLink` is used instead, in `LegacyDisplayLinkMac.cpp`, and when available use `CADisplayLink` in `DisplayLinkMac.mm`. In order to preserve a similar behavior to `CVDisplayLink`, we spawn a dedicated thread for each `CADisplayLink`. Note that on macOS, the creation of a `CADisplayLink` is done through `NSScreen`, so we also export the `WebCore::screen()` method to be able to get the appropriate `NSScreen` for a given `DisplayLink` instance. * Source/WTF/wtf/PlatformHave.h: * Source/WebCore/PAL/pal/spi/cocoa/QuartzCoreSPI.h: * Source/WebCore/platform/PlatformScreen.h: * Source/WebKit/SourcesCocoa.txt: * Source/WebKit/UIProcess/DisplayLink.h: * Source/WebKit/UIProcess/mac/DisplayLinkMac.mm: Added. (-[WKDisplayLinkHandler initWithScreen:client:]): (-[WKDisplayLinkHandler threadWasCreated]): (-[WKDisplayLinkHandler dealloc]): (-[WKDisplayLinkHandler displayLinkFired:]): (-[WKDisplayLinkHandler invalidate]): (-[WKDisplayLinkHandler isRunning]): (-[WKDisplayLinkHandler setRunning:]): (-[WKDisplayLinkHandler nominalFramesPerSecond]): (WebKit::DisplayLink::platformInitialize): (WebKit::DisplayLink::platformFinalize): (WebKit::DisplayLink::platformIsRunning const): (WebKit::DisplayLink::platformStart): (WebKit::DisplayLink::platformStop): (WebKit::DisplayLink::displayLinkHandlerCallbackFired): * Source/WebKit/UIProcess/mac/LegacyDisplayLinkMac.cpp: Renamed from Source/WebKit/UIProcess/mac/DisplayLinkMac.cpp. (WebKit::DisplayLink::platformInitialize): (WebKit::DisplayLink::platformFinalize): (WebKit::DisplayLink::nominalFramesPerSecondFromDisplayLink): (WebKit::DisplayLink::platformIsRunning const): (WebKit::DisplayLink::platformStart): (WebKit::DisplayLink::platformStop): (WebKit::DisplayLink::displayLinkCallback): * Source/WebKit/UIProcess/mac/WKImmediateActionController.mm: Fix an include issue revealed by the addition of a new file. * Source/WebKit/WebKit.xcodeproj/project.pbxproj:
EWS run on current version of this PR (hash a7bf94e)
|
|
||
- (void)invalidate | ||
{ | ||
[_displayLink invalidate]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This invalidate
call from the main thread races against the call to -[CADisplayLink addToRunLoop:forMode:]
on the background thread.
[_displayLink invalidate]; | ||
_displayLink = nil; | ||
|
||
[_thread cancel]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't do anything except set a flag. To actually kick the runloop out of its wait loop, you would need something like CFRunLoopStop
here (and CFRunLoopRun()
in the background thread, since CFRunLoopStop
doesn't do anything for an -[NSRunLoop run]
invocation due to differing semantics of those calls).
Right now, even after you invalidate the CADisplayLink, the worker thread spawned will just run forever.
5ff129e
a7bf94e