[vlc-devel] [VLC 3.x 2/4] macosx: SPMediaKeyTap: Move to main thread

[David Fuhrmann]

> Am 22.04.2021 um 17:39 schrieb Marvin Scholz <epirat07 at gmail.com>:
> 
> Move the event tap to the main thread, as the event tap callback
> makes use of NSEvent APIs which apparently crash at least on M1
> machines running Big Sur when called from a non-main thread.
> 
> Additionally remove the now unnecessary @synchronized recursive lock.
> 
> Fix videolan/vlc#25650
> ---
> modules/gui/macosx/SPMediaKeyTap.m | 62 +++++++-----------------------
> 1 file changed, 13 insertions(+), 49 deletions(-)
> 
> diff --git a/modules/gui/macosx/SPMediaKeyTap.m b/modules/gui/macosx/SPMediaKeyTap.m
> index 1981b45775..03e8e67920 100644
> --- a/modules/gui/macosx/SPMediaKeyTap.m
> +++ b/modules/gui/macosx/SPMediaKeyTap.m
> @@ -32,19 +32,15 @@ NSString *kIgnoreMediaKeysDefaultsKey = @"SPIgnoreMediaKeys";
> @interface SPMediaKeyTap () {
>     CFMachPortRef _eventPort;
>     CFRunLoopSourceRef _eventPortSource;
> -    CFRunLoopRef _tapThreadRL;
> -    NSThread *_tapThread;
>     BOOL _shouldInterceptMediaKeyEvents;
>     id _delegate;
>     // The app that is frontmost in this list owns media keys
>     NSMutableArray<NSRunningApplication *> *_mediaKeyAppList;
> }
> 
> -- (BOOL)shouldInterceptMediaKeyEvents;
> - (void)setShouldInterceptMediaKeyEvents:(BOOL)newSetting;
> - (void)startWatchingAppSwitching;
> - (void)stopWatchingAppSwitching;
> -- (void)eventTapThread;
> @end
> 
> static CGEventRef tapEventCallback(CGEventTapProxy proxy, CGEventType type, CGEventRef event, void *refcon);
> @@ -121,21 +117,16 @@ static CGEventRef tapEventCallback(CGEventTapProxy proxy, CGEventType type, CGEv
>     if (_eventPortSource == NULL)
>         return NO;
> 
> -    // Let's do this in a separate thread so that a slow app doesn't lag the event tap
> -    _tapThread = [[NSThread alloc] initWithTarget:self
> -                                         selector:@selector(eventTapThread)
> -                                           object:nil];
> -    [_tapThread start];
> +    CFRunLoopAddSource(CFRunLoopGetCurrent(), _eventPortSource, kCFRunLoopCommonModes);
> 
>     return YES;
> }
> 
> - (void)stopWatchingMediaKeys
> {
> -    // Shut down tap thread
> -    if(_tapThreadRL){
> -        CFRunLoopStop(_tapThreadRL);
> -        _tapThreadRL = nil;
> +    // Remove runloop source
> +    if(_eventPortSource){
> +        CFRunLoopRemoveSource(CFRunLoopGetCurrent(), _eventPortSource, kCFRunLoopCommonModes);
>     }
> 
>     // Remove tap port
> @@ -199,35 +190,15 @@ static CGEventRef tapEventCallback(CGEventTapProxy proxy, CGEventType type, CGEv
>     return bundleIdentifiers;
> }
> 
> -
> -- (BOOL)shouldInterceptMediaKeyEvents
> -{
> -    BOOL shouldIntercept = NO;
> -    @synchronized(self) {
> -        shouldIntercept = _shouldInterceptMediaKeyEvents;
> -    }
> -    return shouldIntercept;
> -}
> -
> -- (void)pauseTapOnTapThread:(NSNumber *)yeahno
> -{
> -    CGEventTapEnable(self->_eventPort, [yeahno boolValue]);
> -}
> -
> - (void)setShouldInterceptMediaKeyEvents:(BOOL)newSetting
> {
> -    BOOL oldSetting;
> -    @synchronized(self) {
> -        oldSetting = _shouldInterceptMediaKeyEvents;
> -        _shouldInterceptMediaKeyEvents = newSetting;
> -    }
> -    if(_tapThreadRL && oldSetting != newSetting) {
> -        [self performSelector:@selector(pauseTapOnTapThread:)
> -                     onThread:_tapThread
> -                   withObject:@(newSetting)
> -                waitUntilDone:NO];
> +    if (_eventPort == NULL)
> +        return;
> +    if (_shouldInterceptMediaKeyEvents == newSetting)
> +        return;
> +    _shouldInterceptMediaKeyEvents = newSetting;

Can you please move this code (setting _shouldInterceptMediaKeyEvents), before the "_eventPort == NULL“ check, to reinstate the old behaviour here?

In fact this case is used when inside the enabling media keys method (I need to investigate a bit whether we need to call CGEventTapEnable also for enabling the key-watch or not).

> 
> -    }
> +    CGEventTapEnable(_eventPort, newSetting);
> }
> 
> 
> @@ -265,9 +236,6 @@ static CGEventRef tapEventCallback2(CGEventTapProxy proxy, CGEventType type, CGE
>     if (keyCode != NX_KEYTYPE_PLAY && keyCode != NX_KEYTYPE_FAST && keyCode != NX_KEYTYPE_REWIND && keyCode != NX_KEYTYPE_PREVIOUS && keyCode != NX_KEYTYPE_NEXT)
>         return event;
> 
> -    if (![self shouldInterceptMediaKeyEvents])
> -        return event;

From my understanding if we do not handle the event we should return it here in this callback to put it back to the event loop. Therefore I would propose to keep this code path here, and as everything is on the main thread it should be ok here without additional locking.

> -
>     [self performSelectorOnMainThread:@selector(handleAndReleaseMediaKeyEvent:) withObject:nsEvent waitUntilDone:NO];
> 
>     return NULL;
> @@ -283,14 +251,10 @@ static CGEventRef tapEventCallback(CGEventTapProxy proxy, CGEventType type, CGEv
> 
> - (void)handleAndReleaseMediaKeyEvent:(NSEvent *)event
> {
> -    [_delegate mediaKeyTap:self receivedMediaKeyEvent:event];
> -}
> +    if (!_shouldInterceptMediaKeyEvents)
> +        return;
> 
> -- (void)eventTapThread
> -{
> -    _tapThreadRL = CFRunLoopGetCurrent();
> -    CFRunLoopAddSource(_tapThreadRL, _eventPortSource, kCFRunLoopCommonModes);
> -    CFRunLoopRun();
> +    [_delegate mediaKeyTap:self receivedMediaKeyEvent:event];
> }
> 
> 
> -- 
> 2.30.1
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel