On Fri, 2017-10-27 at 21:39 +0200, Georg Chini wrote: > On 21.09.2017 14:47, Tanu Kaskinen wrote: > > When a new card shows up (during pulseaudio startup or hotplugged), > > pulseaudio needs to pick the initial profile for the card. Unavailable > > profiles shouldn't be picked, but module-alsa-card sometimes marked > > unavailable profiles as available, causing bad initial profile choices. > > > > This patch changes module-alsa-card so that it marks all profiles > > unavailable whose all output ports or all input ports are unavailable. > > Previously only those profiles were marked as unavailable whose all > > ports were unavailable. For example, if a profile contains one sink and > > one source, and the sink is unavailable and the source is available, > > previously such profile was marked as available, but now it's marked as > > unavailable. > > > > BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=102902 > > --- > > src/modules/alsa/module-alsa-card.c | 62 +++++++++++++++++++++++++++++++------ > > 1 file changed, 52 insertions(+), 10 deletions(-) > > > > diff --git a/src/modules/alsa/module-alsa-card.c b/src/modules/alsa/module-alsa-card.c > > index 4f1236ecd..2adfa6d7b 100644 > > --- a/src/modules/alsa/module-alsa-card.c > > +++ b/src/modules/alsa/module-alsa-card.c > > @@ -427,29 +427,71 @@ static int report_jack_state(snd_mixer_elem_t *melem, unsigned int mask) { > > if (tp->avail == PA_AVAILABLE_NO) > > pa_device_port_set_available(tp->port, tp->avail); > > > > - /* Update profile availabilities. The logic could be improved; for now we > > - * only set obviously unavailable profiles (those that contain only > > - * unavailable ports) to PA_AVAILABLE_NO and all others to > > - * PA_AVAILABLE_UNKNOWN. */ > > + /* Update profile availabilities. Ideally we would mark all profiles > > + * unavailable that contain unavailable devices. We can't currently do that > > + * in all cases, because if there are multiple sinks in a profile, and the > > + * profile contains a mix of available and unavailable ports, we don't know > > + * how the ports are distributed between the different sinks. It's possible > > + * that some sinks contain only unavailable ports, in which case we should > > + * mark the profile as unavailable, but it's also possible that all sinks > > + * contain at least one available port, in which case we should mark the > > + * profile as available. Until the data structures are improved so that we > > + * can distinguish between these two cases, we mark the problematic cases > > + * as available (well, "unknown" to be precise, but there's little > > + * practical difference). > > + * > > + * When all output ports are unavailable, we know that all sinks are > > + * unavailable, and therefore the profile is marked unavailable as well. > > + * The same applies to input ports as well, of course. > > + * > > + * If there are no output ports at all, but the profile contains at least > > + * one sink, then the output is considered to be available. */ > > PA_HASHMAP_FOREACH(profile, u->card->profiles, state) { > > pa_device_port *port; > > void *state2; > > - pa_available_t available = PA_AVAILABLE_NO; > > + bool all_output_ports_are_unavailable = false; > > + bool all_input_ports_are_unavailable = false; > > + pa_available_t available = PA_AVAILABLE_UNKNOWN; > > > > - /* Don't touch the "off" profile. */ > > - if (profile->n_sources == 0 && profile->n_sinks == 0) > > - continue; > > + /* Check if all output ports are unavailable. */ > > + PA_HASHMAP_FOREACH(port, u->card->ports, state2) { > > + if (port->direction != PA_DIRECTION_OUTPUT) > > + continue; > > + > > + if (!pa_hashmap_get(port->profiles, profile->name)) > > + continue; > > > > + if (port->available == PA_AVAILABLE_NO) > > + all_output_ports_are_unavailable = true; > > + else { > > + all_output_ports_are_unavailable = false; > > + break; > > + } > > + } > > + > > + /* Check if all input ports are unavailable. */ > > PA_HASHMAP_FOREACH(port, u->card->ports, state2) { > > + if (port->direction != PA_DIRECTION_INPUT) > > + continue; > > + > > if (!pa_hashmap_get(port->profiles, profile->name)) > > continue; > > > > - if (port->available != PA_AVAILABLE_NO) { > > - available = PA_AVAILABLE_UNKNOWN; > > + if (port->available == PA_AVAILABLE_NO) > > + all_input_ports_are_unavailable = true; > > + else { > > + all_input_ports_are_unavailable = false; > > break; > > } > > } > > > > + if (all_output_ports_are_unavailable || all_input_ports_are_unavailable) > > + available = PA_AVAILABLE_NO; > > + > > + /* The "off" profile is always available. */ > > + if (profile->n_sources == 0 && profile->n_sinks == 0) > > + available = PA_AVAILABLE_YES; > > The extra check for the "off" profile is unnecessary, it has no ports and > will therefore be marked as available (or rather as UNKNOWN). I think it's good to set the availability to yes when we know that the profile is definitely available. But I'll remove this anyway, since there's no real practical difference. > > + > > pa_card_profile_set_available(profile, available); > > } > > > > Otherwise in general OK, but would it not be clearer to use one loop > like this: > > has_input_port = false; > has_output_port = false; > found_available_input_port = false; > found_available_output_port = false; > PA_HASHMAP_FOREACH(port, u->card->ports, state2) { > > if (!pa_hashmap_get(port->profiles, profile->name)) > continue; > > if (port->direction == PA_DIRECTION_INPUT) { > has_input_port = true; > if (port->available != PA_AVAILABLE_NO) > found_available_input_port = true; > } > > if (port->direction == PA_DIRECTION_OUTPUT) { > has_output_port = true; > if (port->available != PA_AVAILABLE_NO) > found_available_input_port = true; > } > } > > > If you prefer to leave it as is, I don't have objections though. I strongly prefer your way. Thanks for the review! -- Tanu https://www.patreon.com/tanuk
