[pulseaudio-discuss] [PATCH] bluez5: Do not suspend on no -> unknown profile transitions

[David Henningsson]

On 2014-12-19 10:55, Tanu Kaskinen wrote:
> On Fri, 2014-12-19 at 10:24 +0100, David Henningsson wrote:
>> In case a transport is currently disconnected and transitions to
>> idle, that should not count as a "remote hang up" event.
>
> Great, thanks for debugging this!
>
>> Signed-off-by: David Henningsson <david.henningsson at canonical.com>
>> ---
>>
>> When used together with Tanu's module-card-restore patch, then the a2dp profile could
>> successfully be restored when PA starts up.
>>
>> That said, I somewhat prefer my own version which never sets the profile to off. Just like
>> an available port does not cause the backend to switch away from it, (this logic is up to
>> module-switch-on-port-available), unavailable profiles should work the same way IMO.
>
> I think that's comparing apples to oranges. When an alsa port is
> unavailable, we are still able to write audio to the device just fine.

Not necessarily. For HDMI, the capabilities can change when you 
plug/unplug an HDMI devices. For some embedded devices, we can't change 
some of the mixer controls when a stream is active.

So a port is no longer that simple.

> An apples to apples comparison would be the case where the alsa PCM
> device disappears (which is currently handled by unloading the whole
> card module - not a good model to emulate in bluetooth).


>>   src/modules/bluetooth/module-bluez5-device.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c
>> index e6a8071..995e550 100644
>> --- a/src/modules/bluetooth/module-bluez5-device.c
>> +++ b/src/modules/bluetooth/module-bluez5-device.c
>> @@ -1968,11 +1968,13 @@ static void handle_transport_state_change(struct userdata *u, struct pa_bluetoot
>>       bool release = false;
>>       pa_card_profile *cp;
>>       pa_device_port *port;
>> +    pa_available_t oldavail;
>>
>>       pa_assert(u);
>>       pa_assert(t);
>>       pa_assert_se(cp = pa_hashmap_get(u->card->profiles, pa_bluetooth_profile_to_string(t->profile)));
>>
>> +    oldavail = cp->available;
>>       pa_card_profile_set_available(cp, transport_state_to_availability(t->state));
>>
>>       /* Update port availability */
>> @@ -1983,7 +1985,7 @@ static void handle_transport_state_change(struct userdata *u, struct pa_bluetoot
>>
>>       /* Acquire or release transport as needed */
>>       acquire = (t->state == PA_BLUETOOTH_TRANSPORT_STATE_PLAYING && u->profile == t->profile);
>> -    release = (t->state != PA_BLUETOOTH_TRANSPORT_STATE_PLAYING && u->profile == t->profile);
>> +    release = (oldavail != PA_AVAILABLE_NO && t->state != PA_BLUETOOTH_TRANSPORT_STATE_PLAYING && u->profile == t->profile);
>
> The profile availability and the transport state have 1:1 mapping, but
> it's nevertheless (or perhaps precisely for that reason) ugly to mix the
> two. I'd fix this by making the old transport state available in the
> state change hook.
>
> If you disagree, I don't care enough to block the patch on this issue.

I admit it looks a bit ugly, but the alternative of creating some struct 
that just contains a transport pointer and an old transport state (just 
to use for the hook) seems equally ugly to me.

Ok to push together with your module-card-restore patch?
We could have a wider discussion on how we think about (and deal with) 
unavailable profiles for PulseAudio v7 or so.

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic