Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

graouts
Copy link
Contributor

@graouts graouts commented May 16, 2024

5ff129e

Adopt CADisplayLink on macOS 14
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:

a7bf94e

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ§ͺ wpe-wk2 βœ… πŸ§ͺ wincairo-tests
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ§ͺ api-wpe
βœ… πŸ§ͺ ios-wk2-wpt βœ… πŸ§ͺ mac-wk1 βœ… πŸ›  wpe-cairo
βœ… πŸ›  πŸ§ͺ jsc βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk2 βœ… πŸ›  gtk
βœ… πŸ›  πŸ§ͺ jsc-arm64 βœ… πŸ›  tv βœ… πŸ§ͺ mac-AS-debug-wk2 βœ… πŸ§ͺ gtk-wk2
βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-wk2-stress βœ… πŸ§ͺ api-gtk
βœ… πŸ›  watch βœ… πŸ›  jsc-armv7
βœ… πŸ›  watch-sim ❌ πŸ§ͺ jsc-armv7-tests

@graouts graouts requested a review from cdumez as a code owner May 16, 2024 18:19
@graouts graouts self-assigned this May 16, 2024
@graouts graouts added the Layout and Rendering For bugs with layout and rendering of Web pages. label May 16, 2024
@graouts graouts requested a review from smfr May 16, 2024 18:21
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 16, 2024
@graouts graouts removed the merging-blocked Applied to prevent a change from being merged label May 16, 2024
Source/WebKit/UIProcess/mac/DisplayLinkMac.mm Outdated Show resolved Hide resolved
Source/WebKit/UIProcess/DisplayLink.h Outdated Show resolved Hide resolved
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:
@bnham bnham self-requested a review May 24, 2024 23:45

- (void)invalidate
{
[_displayLink invalidate];
Copy link
Contributor

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];
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Layout and Rendering For bugs with layout and rendering of Web pages.
Projects
None yet
5 participants