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
