| Summary: | module-switch-on-port-available will only select unavailable ports when a new sink or source appears | ||
|---|---|---|---|
| Product: | PulseAudio | Reporter: | Vivek Dasmohapatra <vivek> |
| Component: | modules | Assignee: | pulseaudio-bugs |
| Status: | RESOLVED FIXED | QA Contact: | pulseaudio-bugs |
| Severity: | normal | ||
| Priority: | medium | CC: | lennart |
| Version: | unspecified | ||
| Hardware: | Other | ||
| OS: | All | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Attachments: | pulseaudio log (-vvvv --log-time=1) | ||
I believe you misunderstood the intention of the code (which is no wonder - comments would be very welcome here). If the name parameter of new_sink_source() is set, then that means that some other module has already chosen the initial port. If so, module-switch-on-port-available shouldn't override that, unless the chosen port is unavailable. The function works correctly as far as I can tell. In the bug title you say that the function selects unavailable ports, but to me that seems to be possible only if all ports are unavailable. The code seems a bit weird if the only purpose is to prevent other modules from selecting unavailable ports, and if that's indeed the only purpose, I think it would be better to fix those other modules and not worry about the initial port at all in module-switch-on-port-available. I don't understand how your change could result in better port choices. Details about what ports exist on your system, the availability status of the ports, and the choices made before and after your change would be helpful. I don't think your analysis of what this function does matches what happens here. Here is some logging I added with the logic corrected per my analysis (MSOPA lines are logging added by me): MSOPA: no port mapping, iterating: MSOPA: current : (null) / 0 MSOPA: candidate: analog-output / 9900 MSOPA: no port mapping, iterating: MSOPA: current : analog-output / 9900 MSOPA: candidate: analog-output-speaker / 10000 MSOPA: no port mapping, iterating: MSOPA: current : analog-output-speaker / 10000 MSOPA: candidate: analog-output-headphones / 9000 MSOPA: availability (0) is UNKNOWN, would return NULL // this is where we would have returned MSOPA: picking port analog-output-speaker on (null) MSOPA: Change initial port for sink alsa_output.pci-0000_00_1f.3.analog-stereo? (0xc1bd50) Switching initial port for sink 'alsa_output.pci-0000_00_1f.3.analog-stereo' to 'analog-output-speaker' MSOPA: no port mapping, iterating: MSOPA: current : (null) / 0 MSOPA: candidate: analog-output / 9900 MSOPA: no port mapping, iterating: MSOPA: current : analog-output / 9900 MSOPA: candidate: analog-output-speaker / 10000 MSOPA: no port mapping, iterating: MSOPA: current : analog-output-speaker / 10000 MSOPA: candidate: analog-output-headphones / 9000 MSOPA: availability (0) is UNKNOWN, returning NULL (original logic) MSOPA: Change initial port for sink alsa_output.pci-0000_00_1f.3.analog-stereo? ((nil)) MSOPA: Leaving initial port for alsa_output.pci-0000_00_1f.3.analog-stereo alone Shortly thereafter , with the original logic, PA says this [pulseaudio] module-device-restore.c: Database contains invalid data for key: sink:alsa_output.pci-0000_00_1f.3.analog-stereo:null Whereas with the fixed logic: [pulseaudio] module-device-restore.c: Restoring volume for sink alsa_output.pci-0000_00_1f.3.analog-stereo: front-left: 49145 / 75%, front-right: 49145 / 75% [pulseaudio] module-device-restore.c: Restoring mute state for sink alsa_output.pci-0000_00_1f.3.analog-stereo. Net result: with the inverted logic volume and mute state are restored on reboot on this laptop, with the original logic they are not. Georg Chini noted the following on IRC: 03-29 15:12 <gchini> I think it should rather be return p after the check 03-29 15:12 <gchini> instead of return NULL 03-29 15:14 <gchini> In the sense "If the port with highest priority is available, return it, else look for another port" 03-29 15:28 <gchini> After a second look I am quite sure it should be return p, because this would also return the named port if name was given You seem to be using pretty old version. 5.0? Since the problem is about restoring volume and mute, I suspect this patch will fix the problem: https://cgit.freedesktop.org/pulseaudio/pulseaudio/commit/?id=e0e6bf687573ea8ee3b6b53f8e44bcb30f4914ec That does appear to work around the problem... But is that really better than fixing new_sink_source so that it can return an available port (ever)? Seems bizarre to have a port selector that can't ever return an available port. (In reply to Vivek Dasmohapatra from comment #5) > Seems bizarre to have a port selector that can't ever return an available > port. That would be bizarre, but that's not how the function works. If the name parameter is set, some other module has already selected a port. If that port is available, new_sink_source() will return NULL, and as a result, the port choice is not overridden. If the port that the other module chose is unavailable, then new_sink_source() will override that with what find_best_port() returns. If the name parameter is not set, then there shouldn't be need for new_sink_source() to do anything, but for some reason it nevertheless first picks the highest priority port. If that port is available, then new_sink_source() returns NULL. As a result, no port will be selected, and pa_sink_new() will do its own selection logic. If the highest priority is unavailable, then new_sink_source() will return what find_best_port() returns. This is bizarre, but harmless. If you want to change the code, I would suggest removing the port selection logic from module-switch-on-port-available and fix all other modules so that they never choose an unavailable port (actually module-device-restore is the only other module that configures the initial port of new sinks and sources). Hm. If you say so. One last question: You say “I would suggest removing the port selection logic from module-switch-on-port-available” — I can't see any documentation in the module itself: Isn't port selection why it exists? (In reply to Vivek Dasmohapatra from comment #7) > One last question: You say “I would suggest removing the port selection > logic from module-switch-on-port-available” — I can't see any documentation > in the module itself: Isn't port selection why it exists? The way I see it, this module exists to do automatic profile and port switches when things are plugged in and out. The module doesn't provide any extra value for choosing the initial port. Or well, currently it provides some value, because module-device-restore doesn't take the port availability into account when it restores the port, so module-switch-on-port-available helps by fixing stupid choices made by module-device-restore, but I think that's a bug in module-device-restore. To be clear, when I said "removing the port selection logic", I meant only the initial port selection for new sinks and sources. Ok - what I would say to that is it's a pretty common pattern (and a sensible one imo) to treat "the first time we see a device" the same as "device was just plugged in" - cf the treatment of network devices, storage, displays etc while a machine is booting up. Otherwise you end up having to special case "device already existed when we started up". In any case - there appears to be a workaround already so no fix required from a user's perspective but I still think it would make more sense for this module to pick the right port on startup... but it's not my project. (In reply to Vivek Dasmohapatra from comment #9) > Ok - what I would say to that is it's a pretty common pattern (and a > sensible one imo) to treat "the first time we see a device" the same as > "device was just plugged in" - cf the treatment of network devices, storage, > displays etc while a machine is booting up. > > Otherwise you end up having to special case "device already existed when we > started up". I don't see where such special case would appear in the code. I'm suggesting removing a bunch of code from module-switch-on-port-available, and I don't see the need to add any new special cases because of that. Since the original problem is already fixed in newer versions, I'll close this bug. Created attachment 131228 [details]
pulseaudio log (-vvvv --log-time=1)
(In reply to Tanu Kaskinen from comment #1) > I believe you misunderstood the intention of the code (which is no wonder - > comments would be very welcome here). > > If the name parameter of new_sink_source() is set, then that means that some > other module has already chosen the initial port. If so, > module-switch-on-port-available shouldn't override that, unless the chosen > port is unavailable. > > The function works correctly as far as I can tell. In the bug title you say > that the function selects unavailable ports, but to me that seems to be > possible only if all ports are unavailable. I'm experiencing what you are describing (PulseAudio selecting as unavailable port, even through there is a available port), see attachment: https://bugs.freedesktop.org/attachment.cgi?id=131228 . As you can see online line 469, it set hdmi-output-0 to no (available: no) ( 0.234| 0.000) D: [pulseaudio] device-port.c: Setting port hdmi-output-0 to status no On line 477, it set hdmi-output-1 to yes (available: yes) ( 0.234| 0.000) D: [pulseaudio] device-port.c: Setting port hdmi-output-1 to status yes But even through it know hdmi-output-0 is unavailable and hdmi-output-1 is available it choose hdmi-output-0. (Line 508) ( 0.252| 0.000) D: [pulseaudio] module-switch-on-port-available.c: Switching initial port for sink 'alsa_output.pci-0000_00_1f.3.hdmi-stereo' to 'hdmi-output-0' As I understand the "initial port" logic, it will always choose a unavailable port? But maybe I'm misunderstanding something. https://github.com/pulseaudio/pulseaudio/blob/master/src/modules/module-switch-on-port-available.c#L340 > > The code seems a bit weird if the only purpose is to prevent other modules > from selecting unavailable ports, and if that's indeed the only purpose, I > think it would be better to fix those other modules and not worry about the > initial port at all in module-switch-on-port-available. > > I don't understand how your change could result in better port choices. > Details about what ports exist on your system, the availability status of > the ports, and the choices made before and after your change would be > helpful. HDMI sinks only have one port, so the referenced code can't do anything but choose the only port that exists. The problem is somewhere else - the HDMI card has multiple profiles, and apparently a wrong one gets selected. According to the log, module-card-restore doesn't set the profile. I'm pretty sure the logic in pa_card_choose_initial_profile() in src/pulsecore/card.c picks the initial profile. That function should take the profile availability into account, so is the profile availability information wrong? If I write a patch to help with debugging, will you be able to try it out? Or maybe you can add logging to pa_card_choose_initial_profile() yourself? You seem to be able to read code, maybe you can write too... The log looked like PulseAudio probed only the HDMI profiles. Have you modified /usr/share/pulseaudio/alsa-mixer/profile-sets/default.conf to only include HDMI mappings? Have you made other modifications to the configuration or code? |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.
module-switch-on-port-available - the helper function new_sink_source iterates over the device ports and picks the one with the highest priority but then, just before returning, does this: if (p->available != PA_AVAILABLE_NO) return NULL; ie the selected port is returned _only_ if it is NOT available. This logic error appears to be in pulseaudio 10.x also. This is used by (for example): static pa_hook_result_t sink_new_hook_callback(pa_core *c, pa_sink_new_data *new_data, void *u) { pa_device_port *p = new_sink_source(new_data->ports, new_data->active_port); if (p) { pa_log_debug("Switching initial port for sink '%s' to '%s'", new_data->name, p->name); pa_sink_new_data_set_port(new_data, p->name); } return PA_HOOK_OK; } So this module will never choose an available port. The p->available != PA_AVAILABLE_NO pattern appears all over the place in this module but I _think_ this is the only place where it's used the wrong way round. The following fixed the symptom(s) here (and the pa -vvvv log indicated that a more sensible set of choices was being made with it applied): --- a/src/modules/module-switch-on-port-available.c +++ b/src/modules/module-switch-on-port-available.c @@ -256,7 +256,7 @@ p = i; if (!p) return NULL; - if (p->available != PA_AVAILABLE_NO) + if (p->available == PA_AVAILABLE_NO) return NULL; pa_assert_se(p = find_best_port(ports));