Speculative fix for a crash (#959)

Usage of main-thread AppKit APIs in other thread is assumed to lead to the crash. Second attempt for #958. Structure display link control reactively around Wake/Sleep and Screen change notifications. --------- Co-authored-by: Igor Demin <[email protected]>
JetBrains · Jul 30, 2024 · c8147b1 · c8147b1
1 parent e7d2dd3
commit c8147b1
Show file tree

Hide file tree

Showing 3 changed files with 144 additions and 61 deletions.
diff --git a/skiko/src/awtMain/kotlin/org/jetbrains/skiko/redrawer/DisplayLinkThrottler.kt b/skiko/src/awtMain/kotlin/org/jetbrains/skiko/redrawer/DisplayLinkThrottler.kt
@@ -2,22 +2,22 @@ package org.jetbrains.skiko.redrawer
 
 import org.jetbrains.skiko.Library
 
-internal class DisplayLinkThrottler {
-    private val implPtr = create()
+internal class DisplayLinkThrottler(windowPtr: Long) {
+    private val implPtr = create(windowPtr)
 
     internal fun dispose() = dispose(implPtr)
 
     /*
      * Creates a DisplayLink if needed with refresh rate matching NSScreen of NSWindow passed in [windowPtr].
      * If DisplayLink is already active, blocks until next vsync for physical screen of NSWindow passed in [windowPtr].
      */
-    internal fun waitVSync(windowPtr: Long) = waitVSync(implPtr, windowPtr)
+    private external fun create(windowPtr: Long): Long
 
-    private external fun create(): Long
+    fun waitVSync() = waitVSync(implPtr)
 
     private external fun dispose(implPtr: Long)
 
-    private external fun waitVSync(implPtr: Long, windowPtr: Long)
+    private external fun waitVSync(implPtr: Long)
 
     companion object {
         init {

diff --git a/skiko/src/awtMain/kotlin/org/jetbrains/skiko/redrawer/MetalRedrawer.kt b/skiko/src/awtMain/kotlin/org/jetbrains/skiko/redrawer/MetalRedrawer.kt
@@ -60,7 +60,7 @@ internal class MetalRedrawer(
         }
 
     private val adapter = chooseMetalAdapter(properties.adapterPriority)
-    private val displayLinkThrottler = DisplayLinkThrottler()
+    private val displayLinkThrottler = DisplayLinkThrottler(layer.windowHandle)
 
     private val windowOcclusionStateChannel = Channel<Boolean>(Channel.CONFLATED)
     @Volatile private var isWindowOccluded = false
@@ -159,7 +159,7 @@ internal class MetalRedrawer(
             // Wait for vsync because:
             // - macOS drops the second/next drawables if they are sent in the same vsync
             // - it makes frames consistent and limits FPS
-            displayLinkThrottler.waitVSync(windowPtr = layer.windowHandle)
+            displayLinkThrottler.waitVSync()
 
             autoreleasepool {
                 contextHandler.draw()

diff --git a/skiko/src/awtMain/objectiveC/macos/DisplayLinkThrottler.mm b/skiko/src/awtMain/objectiveC/macos/DisplayLinkThrottler.mm
@@ -5,6 +5,33 @@
 #import <QuartzCore/QuartzCore.h>
 #import <AppKit/AppKit.h>
 #import <stdatomic.h>
+#import <objc/runtime.h>
+
+typedef void (^OnScreenChangeCallback)(void);
+
+@interface NSWindow (OnScreenChangeCallbackExtension)
+
+- (void)skiko_setupOnScreenChangeCallback:(OnScreenChangeCallback)callback;
+
+@end
+
+static void *OnScreenChangeCallbackKey = &OnScreenChangeCallbackKey;
+
+@implementation NSWindow (OnScreenChangeCallbackExtension)
+
+- (void)skiko_setupOnScreenChangeCallback:(OnScreenChangeCallback)callback {
+    objc_setAssociatedObject(self, OnScreenChangeCallbackKey, [callback copy], OBJC_ASSOCIATION_COPY_NONATOMIC);
+
+    NSNotificationCenter *center = [NSNotificationCenter defaultCenter];
+    [center addObserver:self selector:@selector(skiko_onScreenChangeCallback) name:NSWindowDidChangeScreenNotification object:nil];
+}
+
+- (void)skiko_onScreenChangeCallback {
+    OnScreenChangeCallback callback = objc_getAssociatedObject(self, OnScreenChangeCallbackKey);
+    callback();
+}
+
+@end
 
 @interface DisplayLinkThrottler : NSObject
 
@@ -21,125 +48,184 @@ static CVReturn displayLinkCallback(CVDisplayLinkRef displayLink, const CVTimeSt
 }
 
 @implementation DisplayLinkThrottler {
-    NSScreen *_displayLinkScreen;
+    // CGDirectDisplayID is an alias for uint32_t, we'll use int64_t to keep -1 in case there is no screen
+    int64_t _currentScreenID;
+    CGDirectDisplayID _displayLinkScreenID;
     CVDisplayLinkRef _displayLink;
+
+    // Lock to protect internal state
+    NSLock *_lock;
     NSConditionLock *_vsyncConditionLock;
     BOOL _isSleeping;
+    __weak NSWindow *_window;
 }
 
-- (instancetype)init {
+- (instancetype)initWithWindow:(NSWindow *)window {
     self = [super init];
 
     if (self) {
-        _displayLinkScreen = nil;
+        _currentScreenID = -1;
+        _window = window;
         _displayLink = nil;
         _vsyncConditionLock = [[NSConditionLock alloc] initWithCondition: 1];
+        _lock = [NSLock new];
         _isSleeping = NO;
 
         NSNotificationCenter *notificationCenter = [[NSWorkspace sharedWorkspace] notificationCenter];
 
         [notificationCenter addObserver:self
-                            selector:@selector(systemWillSleep:)
+                            selector:@selector(systemWillSleep)
                             name:NSWorkspaceWillSleepNotification
                             object:nil];
 
         [notificationCenter addObserver:self
-                            selector:@selector(systemDidWake:)
+                            selector:@selector(systemDidWake)
                             name:NSWorkspaceDidWakeNotification
                             object:nil];
+
+        __weak DisplayLinkThrottler *weakSelf = self;
+        [window skiko_setupOnScreenChangeCallback:^{
+            [weakSelf onScreenDidChange];
+        }];
+
+        [self onScreenDidChange];
     }
 
     return self;
 }
 
-- (void)systemWillSleep:(NSNotification *)notification {
+- (void)onScreenDidChange {
+    [_lock lock];
+
+    assert(NSThread.currentThread.isMainThread);
+
+    NSScreen *screen = _window.screen;
+    NSDictionary* screenDescription = [screen deviceDescription];
+    NSNumber* screenNumber = [screenDescription objectForKey:@"NSScreenNumber"];
+
+    if (screenNumber) {
+        _currentScreenID = [screenNumber longValue];
+    } else {
+        _currentScreenID = -1;
+    }
+
+    [self updateDisplayLink];
+
+    [_lock unlock];
+}
+
+- (void)updateDisplayLink {
+    // Assumed to be run inside _lock
+
+    if (_isSleeping) {
+        // invalidate or do nothing if no display link is present
+        [self invalidateDisplayLink];
+    } else {
+        if (_displayLink) {
+            // if display link is present, check if it's for the correct screen, avoid conversion shenanigans
+            int64_t displayLinkScreenID = _displayLinkScreenID;
+
+            if (displayLinkScreenID != _currentScreenID) {
+                [self createDisplayLink];
+            }
+        } else {
+            // no display link, create one
+            [self createDisplayLink];
+        }
+    }
+}
+
+- (void)systemWillSleep {
+    [_lock lock];
+
     _isSleeping = YES;
-    [self invalidateDisplayLink];
+    [self updateDisplayLink];
     [self onVSync];
+
+    [_lock unlock];
 }
 
-- (void)systemDidWake:(NSNotification *)notification {
+- (void)systemDidWake {
+    [_lock lock];
+
     _isSleeping = NO;
+    [self updateDisplayLink];
+
+    [_lock unlock];
 }
 
 - (void)onVSync {
-    /// Lock condition lock and immediately unlock setting condition variable to 1 (can render now)
+    // Lock condition lock and immediately unlock setting condition variable to 1 (can render now)
     [_vsyncConditionLock lock];
     [_vsyncConditionLock unlockWithCondition:1];
 }
 
 - (void)waitVSync {
-    /// If display link is not constructed (due to failure or explicit opt-out), don't perform any waiting
-    if (!_displayLink) {
+    BOOL hasDisplayLink;
+
+    [_lock lock];
+    hasDisplayLink = _displayLink != nil;
+    [_lock unlock];
+
+    // No display link to wait for
+    if (!hasDisplayLink) {
         return;
     }
 
-    /// Wait until `onVSync` signals 1, then immediately lock, set it to 0 and unlock again.
+    // Wait until `onVSync` signals 1, then immediately lock, set it to 0 and unlock again.
     [_vsyncConditionLock lockWhenCondition:1];
     [_vsyncConditionLock unlockWithCondition:0];
 }
 
 - (void)invalidateDisplayLink {
+    // Assumed to be run inside _lock, except for dealloc
     if (_displayLink) {
         CVDisplayLinkStop(_displayLink);
         CVDisplayLinkRelease(_displayLink);
         _displayLink = nil;
-        _displayLinkScreen = nil;
-    }
-}
 
-- (void)setupDisplayLinkForWindow:(NSWindow *)window {
-    if (_isSleeping) {
-        /// System is sleeping, don't setup display link
-        return;
     }
+}
 
-    NSScreen *screen = window.screen;
+- (void)createDisplayLink {
+    // Assumed to be run inside _lock
 
-    if (!screen) {
-        /// window is not yet attached to screen (or window is nil)
-        return;
-    }
+    [self invalidateDisplayLink];
 
-    if ([screen isEqualTo: _displayLinkScreen]) {
-        /// display link is already active for this screen, do nothing
+    if (_currentScreenID < 0) {
         return;
     }
 
-    [self invalidateDisplayLink];
-
-    NSDictionary* screenDescription = [screen deviceDescription];
-    NSNumber* screenID = [screenDescription objectForKey:@"NSScreenNumber"];
-
-    _displayLink = [self createDisplayLinkForScreen:screenID];
-    _displayLinkScreen = screen;
-}
-
-- (CVDisplayLinkRef)createDisplayLinkForScreen:(NSNumber *)screenID {
     CVReturn result;
-    CVDisplayLinkRef displayLink;
 
-    result = CVDisplayLinkCreateWithCGDisplay([screenID unsignedIntValue], &displayLink);
+    result = CVDisplayLinkCreateWithCGDisplay(
+        _currentScreenID /* truncated to uint32_t aka CGDirectDisplayID */,
+        &_displayLink
+    );
 
     if (result != kCVReturnSuccess) {
-        return nil;
+        _displayLink = nil;
+        return;
     }
 
-    result = CVDisplayLinkSetOutputCallback(displayLink, &displayLinkCallback, (__bridge void *)(self));
+    result = CVDisplayLinkSetOutputCallback(_displayLink, &displayLinkCallback, (__bridge void *)(self));
 
     if (result != kCVReturnSuccess) {
-        CVDisplayLinkRelease(displayLink);
-        return nil;
+        CVDisplayLinkRelease(_displayLink);
+        _displayLink = nil;
+        return;
     }
 
-    result = CVDisplayLinkStart(displayLink);
+    result = CVDisplayLinkStart(_displayLink);
 
     if (result != kCVReturnSuccess) {
-        CVDisplayLinkRelease(displayLink);
-        return nil;
+        CVDisplayLinkRelease(_displayLink);
+        _displayLink = nil;
+        return;
     }
 
-    return displayLink;
+    _displayLinkScreenID = _currentScreenID;
+
 }
 
 - (void)dealloc {
@@ -150,24 +236,21 @@ - (void)dealloc {
 
 extern "C" {
 
-JNIEXPORT jlong JNICALL Java_org_jetbrains_skiko_redrawer_DisplayLinkThrottler_create(JNIEnv *env, jobject obj) {
-    DisplayLinkThrottler *throttler = [[DisplayLinkThrottler alloc] init];
+JNIEXPORT jlong JNICALL Java_org_jetbrains_skiko_redrawer_DisplayLinkThrottler_create(JNIEnv *env, jobject obj, jlong windowPtr) {
+    NSWindow *window = (__bridge NSWindow *) (void *) windowPtr;
+    DisplayLinkThrottler *throttler = [[DisplayLinkThrottler alloc] initWithWindow:window];
 
     return (jlong) (__bridge_retained void *) throttler;
 }
 
 JNIEXPORT void JNICALL Java_org_jetbrains_skiko_redrawer_DisplayLinkThrottler_dispose(JNIEnv *env, jobject obj, jlong throttlerPtr) {
     DisplayLinkThrottler *throttler = (__bridge_transfer DisplayLinkThrottler *) (void *) throttlerPtr;
-    /// throttler will be released by ARC and deallocated in the end of this scope.
+    // throttler will be released by ARC and deallocated in the end of this scope.
 }
 
-JNIEXPORT void JNICALL Java_org_jetbrains_skiko_redrawer_DisplayLinkThrottler_waitVSync(JNIEnv *env, jobject obj, jlong throttlerPtr, jlong windowPtr) {
+JNIEXPORT void JNICALL Java_org_jetbrains_skiko_redrawer_DisplayLinkThrottler_waitVSync(JNIEnv *env, jobject obj, jlong throttlerPtr) {
     DisplayLinkThrottler *throttler = (__bridge DisplayLinkThrottler *) (void *) throttlerPtr;
-    NSWindow *window = (__bridge NSWindow *) (void *) windowPtr;
 
-    // CVDisplayLink is conditionally set up on each draw request to track window.screen change to match throttling
-    // with the refresh rate of the actual screen the window is shown on.
-    [throttler setupDisplayLinkForWindow:window];
     [throttler waitVSync];
 }