Hi Tanu, On Sun, Nov 18, 2012 at 10:55 PM, Tanu Kaskinen <tanuk at iki.fi> wrote: > The bug was most likely introduced in this commit: > http://cgit.freedesktop.org/pulseaudio/pulseaudio/commit/?id=ea45f2c7951a81b2f43029892535d6a6280eb97e Yes, I checked this and you're right, it was introduced by this commit. Sorry for that. > BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=57239 > --- > src/modules/bluetooth/bluetooth-util.c | 10 +++++++--- > src/modules/bluetooth/bluetooth-util.h | 1 + > src/modules/bluetooth/module-bluetooth-device.c | 24 ++++++++++++++++++++--- > 3 files changed, 29 insertions(+), 6 deletions(-) > > diff --git a/src/modules/bluetooth/bluetooth-util.c b/src/modules/bluetooth/bluetooth-util.c > index 272b6ce..402bcdc 100644 > --- a/src/modules/bluetooth/bluetooth-util.c > +++ b/src/modules/bluetooth/bluetooth-util.c > @@ -345,9 +345,13 @@ static int parse_device_property(pa_bluetooth_discovery *y, pa_bluetooth_device > > if (pa_streq(key, "Paired")) > d->paired = !!value; > - else if (pa_streq(key, "Connected")) > - d->device_connected = !!value; > - else if (pa_streq(key, "Trusted")) > + else if (pa_streq(key, "Connected")) { > + if (d->device_connected != !!value) { > + d->device_connected = !!value; > + pa_log_debug("Device %s %s.", d->path, d->device_connected ? "connected" : "disconnected"); > + pa_hook_fire(&d->hooks[PA_BLUETOOTH_DEVICE_HOOK_CONNECTED_CHANGED], NULL); I agree about need to have a hook to report changes regarding the connection state, but I think the approach needs some improvement. You're using the Device.Connected property, while module-bluetooth-discover is additionally checking if at least one audio profile is connected. This can be problematic is you have some other Bluetooth profile connected. In this case, the problem described in the bug report would reproduce: when some audio profile gets reconnected, module-bluetooth-discover would load the module again, leading to two instances of module-bluetotoh-device. The reason is Device.Connected was never set to false, and thus module-bluetooth-device was never unloaded. Therefore, I would suggest that the new hook is fired in filter_cb(), immediately before run_callback() is called. This could also scale better, if at some point we wanted to modify the card according to which profiles are currently connected (i.e. create card profile only if corresponding bluetooth profile is connected). > + } > + } else if (pa_streq(key, "Trusted")) > d->trusted = !!value; > > /* pa_log_debug("Value %s", pa_yes_no(value)); */ > diff --git a/src/modules/bluetooth/bluetooth-util.h b/src/modules/bluetooth/bluetooth-util.h > index 8a3f2ad..30dd2e8 100644 > --- a/src/modules/bluetooth/bluetooth-util.h > +++ b/src/modules/bluetooth/bluetooth-util.h > @@ -94,6 +94,7 @@ typedef enum pa_bt_audio_state { > /* Hook data: pa_bluetooth_device pointer. */ > typedef enum pa_bluetooth_device_hook { > PA_BLUETOOTH_DEVICE_HOOK_REMOVED, /* Call data: NULL. */ > + PA_BLUETOOTH_DEVICE_HOOK_CONNECTED_CHANGED, /* Call data: NULL. */ > PA_BLUETOOTH_DEVICE_HOOK_UUID_ADDED, /* Call data: const char *uuid. */ > PA_BLUETOOTH_DEVICE_HOOK_MAX > } pa_bluetooth_device_hook_t; > diff --git a/src/modules/bluetooth/module-bluetooth-device.c b/src/modules/bluetooth/module-bluetooth-device.c > index 8a9d39f..f7a45b0 100644 > --- a/src/modules/bluetooth/module-bluetooth-device.c > +++ b/src/modules/bluetooth/module-bluetooth-device.c > @@ -146,6 +146,7 @@ struct userdata { > char *accesstype; > pa_hook_slot *transport_removed_slot; > pa_hook_slot *device_removed_slot; > + pa_hook_slot *device_connected_changed_slot; > > pa_bluetooth_discovery *discovery; > pa_bool_t auto_connect; > @@ -2522,6 +2523,20 @@ static pa_hook_result_t device_removed_cb(pa_bluetooth_device *d, void *call_dat > return PA_HOOK_OK; > } > > +/* Run from main thread */ > +static pa_hook_result_t device_connected_changed_cb(pa_bluetooth_device *d, void *call_data, struct userdata *u) { > + pa_assert(d); > + pa_assert(u); > + > + if (d->device_connected) > + return PA_HOOK_OK; If you agree with the idea described above, the condition here should match the one in module-bluetooth-discover: if ((d->audio_state >= PA_BT_AUDIO_STATE_CONNECTED || d->audio_source_state >= PA_BT_AUDIO_STATE_CONNECTED || d->hfgw_state >= PA_BT_AUDIO_STATE_CONNECTED)) return PA_HOOK_OK; > + > + pa_log_debug("Unloading module, because device %s became disconnected.", d->path); > + pa_module_unload(u->core, u->module, TRUE); > + > + return PA_HOOK_OK; > +} > + > int pa__init(pa_module* m) { > pa_modargs *ma; > uint32_t channels; > @@ -2594,6 +2609,8 @@ int pa__init(pa_module* m) { > > u->device_removed_slot = pa_hook_connect(&device->hooks[PA_BLUETOOTH_DEVICE_HOOK_REMOVED], PA_HOOK_NORMAL, > (pa_hook_cb_t) device_removed_cb, u); > + u->device_connected_changed_slot = pa_hook_connect(&device->hooks[PA_BLUETOOTH_DEVICE_HOOK_CONNECTED_CHANGED], PA_HOOK_NORMAL, > + (pa_hook_cb_t) device_connected_changed_cb, u); > > u->device = device; > > @@ -2684,10 +2701,11 @@ void pa__done(pa_module *m) { > > stop_thread(u); > > - if (u->device_removed_slot) { > + if (u->device_connected_changed_slot) > + pa_hook_slot_free(u->device_connected_changed_slot); > + > + if (u->device_removed_slot) > pa_hook_slot_free(u->device_removed_slot); > - u->device_removed_slot = NULL; > - } > > if (USE_SCO_OVER_PCM(u)) > restore_sco_volume_callbacks(u); > -- Additionally, for a later patch, we should probably modify module-bluetooth-discover such that we don't have similar problems in the future. Basically, we could unregister the device module from the hashmap only when the module has been unloaded, without relying on some policy that needs to match 100% the unloading policy in module-bluetooth-device. Otherwise the hashmap gets out of sync and thus we can end up having duplicated instances of module-bluetooth-device. Cheers, Mikel
