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

Fix flickering OSD and refactor logic for volume slider. #152

Closed
wants to merge 3 commits into from

Conversation

reitermarkus
Copy link
Contributor

This fixes the flickering/hiding of the display's OSD by readding errorRecoveryWaitTime: self.hideOsd ? 0 : nil which was removed.

Also, the volume slider now behaves the same way as the native macOS volume slider: It will only change the volume after a mouse-up event and it will also play the volume-changed sound.

if !isAlreadySet {
guard self.ddc?.write(command: .audioSpeakerVolume, value: volumeDDCValue) == true else {
return
DispatchQueue.global(qos: .userInitiated).async {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will undo the performance improvements for repeated and held-down media key volume commands made by executing the set volume DDC command on the main thread (per my initial analysis in #134).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you saying we should be using DispatchQueue.main instead of DispatchQueue.global(qos: .userInitiated) here?

Copy link
Contributor

Choose a reason for hiding this comment

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

The DDC write command is already being run on the main thread, so you shouldn't need to enclose this in a DispatchQueue block at all, unless you're running the setVolume method from a background thread.

I'm happy to test out this PR and tinker with it as well to see if there's any regression in performance from this change though - my monitor at work has an OSD that behaves in a similar way to yours, but my home one doesn't, so I only have limited opportunities to test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you shouldn't need to enclose this

Doesn't hurt to be explicit, especially since in this case it is critical where it is run.

I'm happy to test out this PR

Please do.

Copy link
Member

@JoniVR JoniVR left a comment

Choose a reason for hiding this comment

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

I can confirm this returns the Osd hiding on my display back to previous behaviour.

One problem I noticed during testing:
When I mute and then unmute, volume is not being restored to the previous value (before muting) with these changes.

@reitermarkus
Copy link
Contributor Author

volume is not being restored to the previous value

You're right, same for me. I'll look into it.

@reitermarkus
Copy link
Contributor Author

Should be fixed.

@reitermarkus
Copy link
Contributor Author

Also, @ScorpionDev, I just tried switching from DispatchQueue.global(qos: .userInitiated) to DispatchQueue.main and performance gets worse for me.

@robertbressi
Copy link
Contributor

I've spotted two issues with this build:

  1. The "mute" OSD image now comes up intermittently or is completely wrong when toggling mute (see gif below):
    intermittent_osd_image_mute
  2. When holding down the volume up key, when the volume gets to maximum, the volume pop sound continues to be played as long as you hold down the key instead of stopping once the volume hits maximum

Performance seems fine with the writes happening on the background thread, from what I can see, anyway.

@JoniVR JoniVR added Priority: Minor Issue is minor (e.g. Monitor or cable type specific…) Type: Bug Issue is a bug (e.g. Crash, …) labels Dec 11, 2019
@jererobles
Copy link

Was there any development on this PR? Currently facing the same issue.

@JoniVR JoniVR added the Status: Conflicts Issue has conflicts that needs to be resolved before merging label Nov 22, 2020
@JoniVR
Copy link
Member

JoniVR commented Jul 25, 2021

Closing this as it looks pretty abandoned, feel free to reopen :)

@JoniVR JoniVR closed this Jul 25, 2021
@JoniVR JoniVR added the Status: Abandoned Issue will not be worked on anymore (reason in comment) label Jul 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Minor Issue is minor (e.g. Monitor or cable type specific…) Status: Abandoned Issue will not be worked on anymore (reason in comment) Status: Conflicts Issue has conflicts that needs to be resolved before merging Type: Bug Issue is a bug (e.g. Crash, …)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants