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

libobs: Rewrite macOS Hotkeys to use CGEventTap #9583

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gxalpha
Copy link
Member

@gxalpha gxalpha commented Sep 11, 2023

Description

Using CGEventTapCreate instead of addGlobalMonitorForEventsMatchingMask to listen to global hotkeys has the advantage of tighter control over what permissions we need. Since we only listen to and do not modify the event, it is enough to have "Input Monitoring" instead of the full "Accessibility" (which would allow modifying the event after catching it).

It also might fix the issues we have with hotkeys randomly breaking down after a while. I suspect that addGlobalMonitor could just be a wrapper around CGEventTap and not handle timeouts.
This is why I'd like people who currently have issues with hotkeys randomly dying after a while testing this Pull Request. That can be a bit tricky, as the binaries will not be signed. You will probably first have to remove OBS from the permissions list (and maybe even uninstall the release version). After that, open the test version and allow "Input Monitoring" ("Accessibility" is no longer needed). Re-open the test version and check if it works. If not, check the log file for hotkeys-cocoa: No permissions, could not add global hotkeys. If you see that, permissions were not set successfully, try again.

Motivation and Context

Got annoyed that our Hotkeys require "Accessibility" permissions. Saw an opportunity to maybe also fix the other issues.

How Has This Been Tested?

macOS 14
Tested a list of hotkeys both out-of-focus as well as in-focus. This included hotkeys with each type of modifier.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Tweak (non-breaking change to improve existing functionality)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@gxalpha gxalpha added Seeking Testers Build artifacts on CI macOS Affects macOS labels Sep 11, 2023
@gxalpha gxalpha requested a review from PatTheMav September 11, 2023 10:24
libobs/obs-cocoa.m Outdated Show resolved Hide resolved
Copy link
Member

@PatTheMav PatTheMav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks fine to me, uses the typical keylogger code for macOS and should be safe as you won't get any events for inputs made to a secure input field at all.

@gxalpha gxalpha force-pushed the macos-hotkeys-rewrite branch from 75bd569 to eb39f2f Compare September 11, 2023 13:53
@peppy
Copy link

peppy commented Sep 15, 2023

I will start testing this next week in my daily streams.

One initial caveat is that I had to manually add OBS to the Input Monitoring whitelist in order to have it handle global hotkeys. Could it be that this was not done automatically because OBS was already registered under Accessibility?

I tried removing OBS from both whitelists and opening it again, and the input monitoring request dialog did correctly appear, so maybe this is just one of those weird edge cases with macOS.

Things seem to work so far, let's see how it holds up 🎉

@gxalpha
Copy link
Member Author

gxalpha commented Sep 15, 2023

One initial caveat is that I had to manually add OBS to the Input Monitoring whitelist in order to have it handle global hotkeys. Could it be that this was not done automatically because OBS was already registered under Accessibility?

@peppy There are two things here:

My initial description wasn't completely accurate. If you have previously granted Accessibility, that includes Input Monitoring and under normal circumstances, no action should be required. I will push a change to the permissions dialog highlighting the quirk that it's indeed expected for OBS to not show up under Input Monitoring if it's listed in Accessibility (usually not a problem if Accessibility has already been granted, but a problem if it hasn't).

However when you run OBS with a different code signature or no signature at all (which you likely have), permissions tend to go all over the place (this can happen to all permissions, not just Accessibility). Should you ever find yourself in that situation again, you can close all open instances of OBS and run sudo tccutil reset All com.obsproject.obs-studio to reset the permissions completely.

@gxalpha gxalpha force-pushed the macos-hotkeys-rewrite branch from eb39f2f to facd740 Compare September 15, 2023 12:32
@gxalpha gxalpha changed the title [WIP] libobs: Rewrite macOS Hotkeys to use CGEventTap libobs: Rewrite macOS Hotkeys to use CGEventTap Sep 15, 2023
@gxalpha gxalpha marked this pull request as ready for review September 15, 2023 12:34
@peppy
Copy link

peppy commented Sep 19, 2023

I managed to break things in the same way as every other previous encounter of the bug after 47 minutes of usage. I'm on g74f4d4191 which I'm pretty sure is a build from this thread, although due to the force pushing I can't see it anymore.

As mentioned in the issue thread, hotkeys stopped working globally, and after focusing the OBS window, each hotkey works once before failing permanently.

@PatTheMav
Copy link
Member

@gxalpha if this does indeed not fix the issue (see the prior comment) is this change still desirable (given the amount of changes to the implementation it makes and that it might require another trip to the permission screen for some users)?

We might need to look the devil in the eye and register a callback for specific hotkeys with macOS directly which requires no permissions but uses parts of the old Carbon framework that refuse to die (or Apple didn't remove for lack of alternatives).

@gxalpha
Copy link
Member Author

gxalpha commented Dec 8, 2023

I'm a bit torn.

In principle I would still like this, my concern wouldn't be users having to do an additional trip to the settings because Input Monitoring is a subset of Accessibility (meaning anyone who already granted Accessibility would not need to grant anything again), but indeed the change to the permissions dialog. Looking at it again it's not too bad but also more than I would like.

Would have been nice to do it this way initially, but alas.

Copy link
Member

@PatTheMav PatTheMav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still have to test this locally, but code looks good so far.

@RytoEX
Copy link
Member

RytoEX commented Jan 23, 2025

This requires a rebase/refactor to account for the frontend refactor.

Using CGEventTapCreate instead of addGlobalMonitorForEventsMatchingMask
to listen to global hotkeys has the advantage of tighter control over
what permissions we need. Since we only listen to and do not modify the
event, it is enough to have "Input Monitoring" instead of the full
"Accessibility" (which would allow modifying the event after catching
it).
The previous commit switched global hotkeys from requiring Accessibility
to just Input Monitoring permissions. This adds the matching changes to
the permissions dialog, also accounting for the fact that Accessibility
includes Input Monitoring.
@gxalpha gxalpha force-pushed the macos-hotkeys-rewrite branch from 7b4b6f6 to f4f6bb0 Compare January 24, 2025 15:44
@gxalpha
Copy link
Member Author

gxalpha commented Jan 24, 2025

Rebased.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
macOS Affects macOS Seeking Testers Build artifacts on CI
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

4 participants