[pulseaudio-discuss] [PATCH] alsa-card: improve the profile availability logic
Tanu Kaskinen
tanuk at iki.fi
Sun Oct 29 19:49:26 UTC 2017
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
More information about the pulseaudio-discuss
mailing list