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
Add mute support to audio, core, and MPD layers #529
Conversation
It was already called with the argument, and both the MPD and HTTP frontends handled it/expected it. It was just the default implementation in CoreListener that lacked the argument.
This is required for e.g. ncmpcpp to detect that an enableoutput/disableoutput command worked, making it possible to toggle the output back without restarting ncmpcpp.
@@ -13,7 +12,9 @@ def disableoutput(context, outputid): | |||
|
|||
Turns an output off. | |||
""" | |||
raise MpdNotImplemented # TODO | |||
if int(outputid) == 0: | |||
context.core.playback.set_mute(True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imagine the case were you have say alsa based mixing/muting enabled, do we really want disabling an output in mopidy to mute all audio system wide? In my head the disable output calls are more stop sending data to alsa, not mute alsa.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- We're going away from hardware mixing by default, so the problem becomes far smaller.
- Nobody is claiming that this is what
outputs
in MPD are intended for. It's just a convenient way to toggle flags in Mopidy through MPD. Coincidentally, muting an audio output is quite close to Mopidy stopping its usage of an audio output. - I've never seen a client disabling/enabling outputs through MPD automatically or as a side effect of something else. In all clients I've seen, it's a separate menu choice.
That said, we could probably consider renaming the output from Default
to something else, though I'm not sure exactly what.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mute
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disregarded Mute
at first, since it doesn't work to name it Mute
and have it enabled by default, but of course, we can have it disabled by default.
PR updated. |
It seems good to me... I will merge it to my audio-mute branch, test it and report back if I see something strange... |
Add mute support to audio, core, and MPD layers
This builds upon pull request #512 to fix feature request #186.
The code has been manually tested to work with ncmpcpp using both software mixing and alsamixer.