Performance enhancements for media key shortcuts, repeated keys #134
Conversation
JoniVR
added
Priority: Major
|
Testing out the scenario presented in #133 today made me realize something pertinent to this change - reading DDC values on the background thread triggers a race condition which sometimes causes incorrect values to be set against individual preferences. Background here is that I sometimes experience issues on restarting MonitorControl where individual values would be wildly different from their current DDC values - for example: before starting MonitorControl, my brightness would be at 100 and my speaker volume at a very low value or 0, but when I tried to adjust either of these, my speaker volume would be at 100 (which, trust me, while you have audio playing, is extremely startling, especially on monitors that have loud speakers) and my brightness would be at 0. For example, I noticed the following from the "Log (background queue)" in the above comment - my contrast DDC value (which was 1/100) has been set to my audioSpeakerVolume DDC value (which was 0/100), my brightness DDC value (which was 100/100) has been set to my audioMuteScreenBlank DDC value (which was 1/2) and my audioSpeakerVolume DDC value (which was 0/100) has been set to my brightness DDC value (which was 100/100): Compare this with the "Log (main queue)" in the above comment - all of the values are aligned correctly with their corresponding commands (0/100 for audioSpeakerVolume, 1/2 for audioMuteScreenBlank, 100/100 for brightness/luminance and 1/100 for contrast): I believe that moving DDC read/write commands to the main thread also removes the race condition from DDC reads, preventing the issue that I described - just another reason why I think this change should be considered :) |
|
I agree with you on this. I had also thought about the background calls possibly causing some of the performance issues but hadn't looked into it properly. I do remember back with some of the earlier versions of MonitorControl when it used ddcctl for ddc, there were less background calls and better performance overall. I can confirm that I have also noticed the same scenario you mentioned above with the restart. So as far as I'm concerned, once I (or someone else) get to reviewing this PR and can properly test and confirm that it does indeed solve the issues and improve performance, it's fine by me. :) |
…already been set For example: 0 -> 0, 100 -> 100
|
Rebased this PR on 39f76e5 - there were merge conflicts that have been resolved. |
|
I've been running MonitorControl for a few days using a build generated from this branch and it seems pretty good on my monitor (Samsung C34J79x). Once the code is reviewed, I'd like to get testing on this change on a wider-array of monitors than I have access to - do we have a core of beta testers who might be willing to use a development build of MonitorControl based on this branch and provide feedback? |
Not as far as I'm aware, but we could perhaps open an issue for this where we can ask people who want to be testers? |
|
Can you try using safe unwrapping a bit more wherever possible? I understand that it can be a lot of extra syntax but it's a bit safer than manual nil checking and then force unwrapping and prevents mistakes much more easily. |
I assume you're referring to optional binding (e.g.: if let nonOptional = optional)? If not, please let me know. I'm not super well-versed in Swift, so I'm happy to take any suggestions and feedback on board and make changes accordingly :) |
Yep, that's it 🙂 so instead of: if oppositeKey != nil, self.keyRepeatTimers[oppositeKey!] != nil, self.keyRepeatTimers[oppositeKey!]!.isValid {
self.keyRepeatTimers[oppositeKey!]!.invalidate()
} else if self.keyRepeatTimers[mediaKey] != nil, self.keyRepeatTimers[mediaKey]!.isValid {
// If there's already an active timer for the key being held down, let it run rather than executing it again
if isRepeat {
return
}
self.keyRepeatTimers[mediaKey]!.invalidate()
}Try something like (I have quickly adapted this so might contain mistakes): if let oppositeKey = oppositeKey, let oppositeKeyTimer = self.keyRepeatTimers[oppositeKey], oppositeKeyTimer.isValid {
oppositeKeyTimer.invalidate()
} else if let mediaKeyTimer = self.keyRepeatTimers[mediaKey], mediaKeyTimer.isValid {
if isRepeat {
return
}
mediaKeyTimer.invalidate()
}Not sure about that Also, if you ever have a line like this: It's a bit different syntax wise than most languages, but because you write it this way, you can mostly prevent nil errors because you have to explicitly unwrap your values. The goal in Swift is mostly to always prevent force unwrapping (! vs ?) and try to safe unwrap wherever possible because you write safer code by doing so. https://docs.swift.org/swift-book/LanguageGuide/OptionalChaining.html |
JoniVR
added
Type: Enhancement
|
Done! Thanks for calling this out - I didn't really like the force-unwrapping thing either, but didn't realize there was an alternative. Glad to have learned something new! 👍 |
|
Yeah, Swift is really fun once you start to really get into it's powerful features but the nice thing about it is that you can mostly still use it like any other language if you don't know the language specifics. |
JoniVR
changed the title
|
Oh cool - thanks for merging! I hadn't had a chance to squash yet though 😅 |
|
Does it matter for you to squash yourself? Figured it was on a separate branch so I did a squash and merge on here |
|
Oh, not at all - didn’t realize you had squashed it yourself, haha. Thanks for that! Sent with GitHawk |
NOTE: This PR depends on
#131, #133 and should not be merged before those two fixes - for the sake of avoiding conflicts.This set of changes is bound to be controversial, so I wanted to get eyes on it ASAP. Each change has been split into a separate commit:
Add handling for holding down media shortcut keys
Currently, the media key handler doesn't handle shortcut keys being held down. This change sets a 50ms timer for key repeats and also handles when the "opposite" key is pressed
Add handling to not send through DDC commands for values which had already been set
Currently, it's possible to keep flooding the DDC interface with values which are already set on the monitor (e.g.: going from 0 to 0 or 100 to 100). This change prevents this from happening by simply not sending through the duplicate command.
Moved DDC reads and writes onto the main thread
This commit is the big change.
I noticed that when setting the volume, brightness and contrast from the sliders (which occurs on the main thread), there is no lag in sending many commands through to the DDC interface.
This is in comparison to the media shortcut keys, in which DDC commands are sent through a background queue, and huge amounts of lag are experienced, especially while holding down the media shortcut keys - as shown below (this experience was so bad that I almost had to quit the app altogether after recording because it was still trying to send through commands after I released the media key):
When running the DDC command writes through the main thread however, the experience looks more like this:
Similarly, for reading DDC values, here's a log with timestamps of the app starting app with values being read on the background queue (reading is 3+ seconds and there are multiple request failures):
Log (background queue)
Compare this to results on the main thread (startup in 800ms, no failed requests):
Log (main queue)
So, I'm not sure whether this experience differs with other configuration sets, such as if there's a longer delay or higher polling set, but I wanted to get it in front of the contributors' eyes to see if this may be a meaningful set of improvements.
As always, happy to accept feedback and make whatever changes are necessary (or to split out individual changes if the whole thing is a little much for one PR!) - let me know :)