Hello Tanu! Sorry it took me so long to get back on this issue, but I was busy with some urgent things lately. Please find my reply in-line below. On Sun, Oct 7, 2018 at 12:46 AM Tanu Kaskinen <tanuk@xxxxxx> wrote: > > On Fri, 2018-10-05 at 18:26 -0700, João Paulo Rechi Vita wrote: > > On Wed, Oct 3, 2018 at 3:22 AM Tanu Kaskinen <tanuk@xxxxxx> wrote: > > > On Tue, 2018-08-07 at 22:00 -0700, João Paulo Rechi Vita wrote: > > > > Prefer the active profile only if it has ports with available status YES > > > > on the same direction of the port we are trying to switch to and a > > > > higher priority than whichever profile we would select to switch to a > > > > new port. > > > > > > I don't really understand this logic. Not all ports with status YES on > > > a profile are necessarily active, and I don't see why inactive ports > > > should prevent switching away from the current profile. > > > > > > > Previously if there was an active port with higher priority than the > > port we were trying to switch to and availability YES or UNKNOWN, we > > would avoid switching away from the active port to the new port. The > > idea is to reduce the preference given to the active port to only when > > it has higher priority than the port we were trying to switch to and > > availability YES only. The rationale is that since a port with > > availability UNKNOWN might not really be available, we shouldn't avoid > > the port switching in that case. > > I probably didn't make my point clear enough. The patch considers also > ports that are *not active* if they are part of the active profile. > This is the thing that doesn't make sense to me. To me it seems that > only active ports should be considered. > It looks like I got a bit confused on my previous reply to this -- I agree with your point here. > As for ignoring currently active ports with UNKNOWN status - that > doesn't seem like a good idea either. If the port is active, it's very > likely that it's also available, because otherwise the user would have > switched to something else. Is there some real use case where treating > UNKNOWN and YES statuses as the same has caused problems? > IIRC this was to solve a problem on a machine without internal speakers and only headphones and HDMI ports, where HDMI availability was always UNKNOWN despite the cable being connected or not. Since it was the higher priority port with availability != NO it would be selected when no headphones were connected. Later when connecting headphones the audio would not be routed to headphones since the currently active port had availability UNKNOWN. I can't reproduce it anymore though, and this commit has been carried over for a couple of years now, so we can probably ignore it. I'll drop it downstream and if we find any problems I'll come back with a better informed proposal :) > > > > This patch removes the code that ensures that no profile switching will > > > > happen when a new port is available if the currently active port's > > > > available state is YES or UNKNOWN, for example when you have the > > > > computer permanently plugged to a HDMI device with audio capabilities > > > > and plug something on the headphones port. > > > > > > So you want to switch to the headphones in that case? That's good, but > > > what if I have headphones plugged in and I plug in a HDMI monitor that > > > I don't want to use for sound? This patch breaks that use case. > > > > > > I'm not sure what we should do. Maybe add some HDMI specific code, so > > > that when HDMI is plugged in, we switch to it only if the HDMI port is > > > the card's preferred port (which means that the user has previously > > > manually chosen the HDMI output). > > > > > > > My idea is that unless a port has been manually selected by the user > > at some point in time, it should always follow the priorities defined > > in mixer paths files. In both cases you described headphones should > > win since the have the higher priority. Notice that the change does > > not blindly choose the active profile if it is active and has a YES > > port, it simply adds it to the list of profiles that would be > > considered, but it still has to have the higher priority by the end of > > the loop (which gives a bonus to preferred profile). > > > > So unless I'm missing something or mixing up concepts, despite the > > order of connection, if you connect both HDMI and headphones, the > > headphones will always be preferred because the have higher priority > > (assuming no manual choices were made at any point). > > My example may have been bad, because I didn't consider that the active > profile with the headphone port may have higher priority than any of > the potential HDMI profiles. However, we shouldn't be comparing profile > priorities anyway when deciding between headphones and HDMI, we should > compare the port priorities. Your patch removed the only place where > port priorities are compared. > > A better example about the special nature of HDMI would be S/PDIF > instead of headphones. S/PDIF has lower priority than HDMI, but we > still shouldn't switch from S/PDIF to HDMI automatically if the user > hasn't chosen HDMI before. In my opinion we should never switch to HDMI > automatically before the user has indicated that they want to use HDMI > for audio. We don't know if the HDMI monitor has audio capabilities, or > it may only have headphone output. If we switch to it automatically, in > some cases it will look like audio stopped working altogether, which is > pretty bad. > > Regarding your example of an HDMI monitor being used while headphones > are plugged in: I agree that switching to headphones makes sense, but > now that I read the patch again, it seems that this case should have > been working already. The patch removed this code from > profile_good_for_output(): > > if ((sink->active_port->available != PA_AVAILABLE_NO) && (sink->active_port->priority >= port->priority)) > return false; > > That code doesn't prevent the switch to headphones, because the > headphone port has higher priority than the HDMI port. > Yes, I guess this bit has changed since then. This was part of a big downstream refactor and an attempt to upstream as much of our fixes as possible, and I let this slip through. > > This is consistent with the results I got when testing this, but at > > this point is important to say we actually change the ports priorities > > downstream to give external devices a higher priority: > > https://github.com/endlessm/pulseaudio/commit/40d00e72f74b2177ebead548427debadd6d1c05e. > > I believe it should still work the same way for the HDMI+HP case we > > are discussing, but I'm now not so sure if we will end up with > > Speakers always winning with the upstream priorities. > > Yes, the port priorities are not set up as good as they should. > Speakers shouldn't be the highest-priority port. That problem is > mitigated to a large extent by marking speakers unavailable when > headphones or lineout is plugged in, but that's not really ideal. > > The priority order of ports should actually depend on whether they have > jack detection support or not. Speakers should have higher priority > than external ports without jack detection, but lower priority than > external ports with jack detection. > Is there a way to specify that in the paths files? > > Actually, > > probably this commit should have been left for a separate series, > > together with the one pointed by the link above. I'll re-organize > > things a bit and re-send -- should RFCs continue to be sent to the > > list, or should they also be submitted though a gitlab MR? > > I guess the mailing list makes sense if you hope to get comments from a > larger audience. If you just want comments from the maintainers, then > MRs are better. Either is fine for me. > Thanks for the throughout review, and sorry for wasting your time with an unnecessary patch. -- João Paulo Rechi Vita http://about.me/jprvita _______________________________________________ pulseaudio-discuss mailing list pulseaudio-discuss@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
