[pulseaudio-discuss] card: Only set active_profile with available profile

[Colin Guthrie]

'Twas brillig, and Tanu Kaskinen at 16/11/13 08:51 did gyre and gimble:
> On Fri, 2013-11-15 at 09:31 +0100, Colin Guthrie wrote:
>> 'Twas brillig, and Tanu Kaskinen at 05/11/13 20:34 did gyre and gimble:
>>>  src/pulsecore/card.c |    7 ++++++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> New commits:
>>> commit f434087e42bca15fae938f6cc01d2875c4c7728b
>>> Author: Luiz Augusto von Dentz <luiz.von.dentz at intel.com>
>>> Date:   Sun Nov 3 15:05:34 2013 +0200
>>>
>>>     card: Only set active_profile with available profile
>>>     
>>>     When a card is being created and no profile has been assigned
>>>     pa_card_new will attempt to select one from the list but it does that
>>>     without checking the available flag which can lead to select profiles
>>>     not available.
>>>
>>> diff --git a/src/pulsecore/card.c b/src/pulsecore/card.c
>>> index e6e0836..4ae16c2 100644
>>> --- a/src/pulsecore/card.c
>>> +++ b/src/pulsecore/card.c
>>> @@ -195,9 +195,14 @@ pa_card *pa_card_new(pa_core *core, pa_card_new_data *data) {
>>>              c->save_profile = data->save_profile;
>>>  
>>>      if (!c->active_profile) {
>>> -        PA_HASHMAP_FOREACH(profile, c->profiles, state)
>>> +        PA_HASHMAP_FOREACH(profile, c->profiles, state) {
>>> +            if (profile->available == PA_AVAILABLE_NO)
>>> +                continue;
>>> +
>>>              if (!c->active_profile || profile->priority > c->active_profile->priority)
>>>                  c->active_profile = profile;
>>> +        }
>>> +        pa_assert(c->active_profile);
>>
>> Doesn't this leave the possibility of a crash if no active profiles exist?
>>
>> Wouldn't it make more sense to fall back to setting it to *something*
>> even if it is unavailable?
> 
> There's always the "off" profile that will be available, both in alsa
> and bluetooth.

Ahh fair point, although the off profile will technically be
PA_AVAILABLE_UNKNOWN rather than "available" - but it will exist not be
PA_AVAILABLE_NO which is I guess what you mean!

Of course currently ALL card profiles are PA_AVAILABLE_UNKNOWN except
bluetooth ones as this is the only place in the code which calls
pa_card_profile_set_available() to change it from the default value of
PA_AVAILABLE_UNKNOWN. In the bluetooth case, this property is set
directly via "cp->available = PA_AVAILABLE_YES;", but not in the alsa code.

This likely makes sense as the bt stuff is the only bit that makes use
of PA_CORE_HOOK_CARD_PROFILE_AVAILABLE_CHANGED and that infrastructure.

Anyway my general question is still valid - is it better to favour
"known available (PA_AVAILABLE_YES)" over "unknown available
(PA_AVAILABLE_UNKOWN)" when deciding the default.

The bit in my patch where it falls back to a "known unavailable
(PA_AVAILABLE_NO)" does seem logically impossible considering the
always-present and never PA_AVAILABLE_NO "Off" profile which I cannot
see going away.

> It seems that both you and Luiz posted patches that change the same
> code, and both remove the assertion.

My patches didn't remove the assertion, although they probably should
have. As you mentioned above it's impossible not to pick the Off profile
which always exists for all cards (except CoreAudio which always has an
On profile instead - but the same logic applies - it's logically
impossible to not pick one).

> Have you talked with Luiz? Could
> you two agree which version you prefer? I don't have an opinion at this
> point. 

CC'ed. The only question really is does it make sense to try
PA_AVAIALBLE_YES first and *then* PA_AVAILABLE_UNKONWN. I honestly don't
know if this is logicically worthwhile or valid.

> I haven't concentrated on the patches enough to even know what
> problem you're solving - is it just that the assertion makes you
> nervous, or is there some other benefit.

I'm not solving any specific problem per-se. I'm just looking at several
bugs where incorrect profiles appear to be selected by default on first
boot where HDMI is picked over regular analog output (for those alsa
cards which share a single card for both HDMI and Analog output rather
than having it presented as separate cards) despite HDMI having a lower
priority. This lead me to look at this code and got me thinking - this
is the only reason for the patch.

A further complication to my current bugs is that sometimes Headphone
*ports* are picked by default on first boot even when they are
unavailable and again are not the highest priority port. It seems like
the same problem but in a different, but similar, part of the code. This
is why the second set of patches applied the same logical fixes to the
Sink/Source code that Luiz added to the Card profiles. I think it makes
sense to keep these bits of code approximately in sync regardless of the
outcome of this discussion - the same logic should apply.


For reference the bug that covers both the afore mentioned cases is:
https://bugs.mageia.org/show_bug.cgi?id=11642 I am not ruling out some
low level KDE bits that poke at alsa directly and "change" the profile
by poking below PA at just the wrong time as this problem does not
appear to happen under gnome to the best of my knowledge.

All the best :)

Col

-- 

Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/

Day Job:
  Tribalogic Limited http://www.tribalogic.net/
Open Source:
  Mageia Contributor http://www.mageia.org/
  PulseAudio Hacker http://www.pulseaudio.org/
  Trac Hacker http://trac.edgewall.org/