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

[Luiz Augusto von Dentz]

Hi Colin, Tanu,

On Sun, Nov 17, 2013 at 3:17 PM, Colin Guthrie <gmane at colin.guthr.ie> wrote:
> '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.

I can try and apply the same logic to Sink/Source, it just that
overall this kind of policy should be in one place or then remove it
altogether and make sure the list is always ordered by their module so
the core should just pick the first one, the problem we were trying to
solve was that there is no hook for priority changes thus they are
considered static which is probably why we are relying on
pa_card_set_profile_available +
PA_CORE_HOOK_CARD_PROFILE_AVAILABLE_CHANGED to indicate the profiles
changes.

So perhaps we should came to an agreement where and how we should be
doing this sort of policy, we could perhaps generalize
module-bluetooth-policy, or do we want to continue with per technology
policy?



-- 
Luiz Augusto von Dentz