Use pactl instead of KMix in PauseMusic Plugin
ClosedPublic

Authored by nicolasfella on Sep 21 2017, 4:16 PM.

Details

Summary

Using pulseaudio via pacmd for controlling the system volume is way easier than using KMix. Same could be done to control the system volume in the MPRIS Plugin

Diff Detail

Repository
R224 KDE Connect
Lint
Lint Skipped
Unit
Unit Tests Skipped
nicolasfella created this revision.Sep 21 2017, 4:16 PM

If I run pacmd set-sink-mute 0 0 I get a Daemon not responding. response. I have to run pactl set-sink-mute 0 0instead. What's the difference between these two commands?

@albertvaka Which system are you running on?

I know I am in the minority, but pulseaudio just does not work with dolby digital and dts on my system.
Please retain the functionality with kmix.
Can you maybe #ifdef it with a PULSEAUDIO_ENABLED option or check on ${PACMD} STREQUAL "PACMD-NOTFOUND"?

I know I am in the minority, but pulseaudio just does not work with dolby digital and dts on my system.
Please retain the functionality with kmix.

I think in your case it would be better to interact with alsa directly, but still kill kmix integration. Even if you don't use pulseaudio, do you even have kmix installed? It doesn't come pre-installed with most distributions, not to talk about non-Plasma systems.

And, in any case, this is only used for the "mute system sound" option of the "pause music on incoming calls" plugin, which doesn't even come enabled by default. Are you actually on the intersection of people who enable this feature AND install kmix?

Sorry for the fuss. I do not have this option enabled. Just got scared there for a second.

As it is proposed now the muting would just fail silently on my system.
Maybe disable the option in the UI if pacmd is not available?

For the record, I do use kmix. With gentoo it is quite easy not to use pulseaudio.

thomasp added inline comments.Sep 22 2017, 12:18 PM
plugins/pausemusic/pausemusicplugin.cpp
66

This function is static.

thomasp added a comment.EditedSep 22 2017, 12:24 PM

I took a quick look on how to do this with ALSA:

  • amixer set Master mute
  • amixer set Master unmute

Unfortunately it is not that easy with ALSA since you might not use Master as you primary output [sic].
In my case I would have to call amixer set 'PCM',0 0 and amixer set 'PCM',0 255 to mute and unmute respectively. Note that this channel does not support the mute/unmute commands. You would have to read the limits of the control and set it to min/max...
Looks like there is no easy way to interact directly with ALSA without allowing people to select their master channel - and thus enumerating the ALSA devices in the UI.

kmix allows to set a master channel. The current implementation works as expected.

Since I did not use this option I sure can live without it.

nicolasfella updated this revision to Diff 19785.EditedSep 22 2017, 12:41 PM

pactl only has a subset of the functionality of pacmd. pacmd also has an interactive cli, but we don't need that and pactl has all the features we need for both this and for setting the system volume. On my Arch/Manjaro system both do the job, but since pacmd fails on your machine we should go with pactl.

I would be ok with keeping the KMix codepath, but I think its quite a corner-case since most people use pulseaudio anyway. If we keep it we should make sure people are aware that they need pactl OR kmix for that feature. I am not aware that Kmix is a dependency on any distro (I wasn't even aware that we have this feature until yesterday :D)

Any comments on the cmake part? I don't know much about cmake.

If the approach is good I will do the same to enable system volume control in the MPRIS Plugin

nicolasfella retitled this revision from Use pacmd instead of KMix in PauseMusic Plugin to Use pactl instead of KMix in PauseMusic Plugin.Sep 22 2017, 12:43 PM

I would be ok with keeping the KMix codepath, but I think its quite a corner-case since most people use pulseaudio anyway. If we keep it we should make sure people are aware that they need pactl OR kmix for that feature. I am not aware that Kmix is a dependency on any distro (I wasn't even aware that we have this feature until yesterday :D)

I am totally fine with kmix support being removed. Please disable the option in the UI when it will not work if at all possible. It will prevent grieve for the dying breed of pulseaudio deniers.

Any comments on the cmake part? I don't know much about cmake.

See man cmake-commands for what find_program does. Also take a look at option and configure_file. The configure_file example shows how to transport values from cmake to c++.
This of course only detects if pactl was found at compile time. A runtime check is also an option.

anthonyfieroni added inline comments.
plugins/pausemusic/CMakeLists.txt
2

You can fallback to ALSA if pactl is not found. You can set a preprocesor directive if pactl is found or not. If not you can use

amixer set Master mute   
amixer set Master unmute
albertvaka accepted this revision.Sep 22 2017, 6:40 PM

IMO, I would be happier killing kmix support. Maybe we could fallback to the alsa command posted by Thomas if pactl is not installed, but I think this should be done at runtime. Even, if we have this fallback mechanism implemented, we could do pactl > pacmd > alsa. But, in any case, I think this patch is good for now to replace kmix by pulseaudio, and this would be an improvement that can be done later. Ship it!

This revision is now accepted and ready to land.Sep 22 2017, 6:40 PM
This revision was automatically updated to reflect the committed changes.