On Wed, Mar 27, 2013 at 5:28 AM, Mikel Astiz <mikel.astiz.oss at gmail.com> wrote:
> Hi Jo?o Paulo,
>
> On Wed, Mar 27, 2013 at 6:16 AM, <jprvita at gmail.com> wrote:
>> From: Jo?o Paulo Rechi Vita <jprvita at openbossa.org>
>>
>> Instead of creating card profiles based on the device UUIDs, create them
>> based on the available transports. This way the card profiles are
>> available only when the respective bluetooth profile is connected.
>> ---
>> src/modules/bluetooth/bluetooth-util.c | 1 +
>> src/modules/bluetooth/module-bluetooth-device.c | 50 ++++++++++++++++---------
>> 2 files changed, 34 insertions(+), 17 deletions(-)
>>
>> diff --git a/src/modules/bluetooth/bluetooth-util.c b/src/modules/bluetooth/bluetooth-util.c
>> index 241da41..47d164b 100644
>> --- a/src/modules/bluetooth/bluetooth-util.c
>> +++ b/src/modules/bluetooth/bluetooth-util.c
>> @@ -2162,6 +2162,7 @@ static DBusMessage *endpoint_set_configuration(DBusConnection *conn, DBusMessage
>> pa_assert_se(pa_hashmap_put(y->transports, t->path, t) >= 0);
>>
>> pa_log_debug("Transport %s profile %d available", t->path, t->profile);
>> + pa_hook_fire(&y->hooks[PA_BLUETOOTH_HOOK_TRANSPORT_STATE_CHANGED], t);
>>
>> pa_assert_se(r = dbus_message_new_method_return(m));
>> pa_assert_se(dbus_connection_send(pa_dbus_connection_get(y->connection), r, NULL));
>> diff --git a/src/modules/bluetooth/module-bluetooth-device.c b/src/modules/bluetooth/module-bluetooth-device.c
>> index 91eb6c9..a1d8690 100644
>> --- a/src/modules/bluetooth/module-bluetooth-device.c
>> +++ b/src/modules/bluetooth/module-bluetooth-device.c
>> @@ -1240,6 +1240,8 @@ static pa_available_t get_port_availability(struct userdata *u, pa_direction_t d
>> return result;
>> }
>>
>> +static pa_card_profile *create_card_profile(struct userdata *u, pa_bluetooth_transport *t);
>> +
>> /* Run from main thread */
>> static void handle_transport_state_change(struct userdata *u, struct pa_bluetooth_transport *transport) {
>> bool acquire = false;
>> @@ -1255,10 +1257,21 @@ static void handle_transport_state_change(struct userdata *u, struct pa_bluetoot
>> profile = transport->profile;
>> state = transport->state;
>>
>> - /* Update profile availability */
>> - if (!(cp = pa_hashmap_get(u->card->profiles, pa_bt_profile_to_string(profile))))
>> - return;
>> + /* Find profile */
>> + if (!(cp = pa_hashmap_get(u->card->profiles, pa_bt_profile_to_string(profile)))) {
>> + cp = create_card_profile(u, transport);
>> + if (!cp)
>> + return;
>> +
>> + if (pa_hashmap_get(u->card->profiles, cp->name)) {
>> + pa_card_profile_free(cp);
>> + return;
>> + }
>>
>> + pa_card_add_profile(u->card, cp);
>> + }
>> +
>> + /* Update profile availability */
>> pa_card_profile_set_available(cp, transport_state_to_availability(state));
>>
>> /* Update port availability */
>> @@ -2078,11 +2091,14 @@ static void create_card_ports(struct userdata *u, pa_hashmap *ports) {
>> }
>>
>> /* Run from main thread */
>> -static pa_card_profile *create_card_profile(struct userdata *u, const char *uuid) {
>> +static pa_card_profile *create_card_profile(struct userdata *u, pa_bluetooth_transport *t) {
>> pa_card_profile *p = NULL;
>> enum profile *d;
>>
>> - if (pa_streq(uuid, A2DP_SINK_UUID)) {
>> + pa_assert(u);
>> + pa_assert(t);
>> +
>> + if (t->profile == PROFILE_A2DP) {
>> p = pa_card_profile_new("a2dp", _("High Fidelity Playback (A2DP)"), sizeof(enum profile));
>> p->priority = 10;
>> p->n_sinks = 1;
>> @@ -2092,7 +2108,7 @@ static pa_card_profile *create_card_profile(struct userdata *u, const char *uuid
>>
>> d = PA_CARD_PROFILE_DATA(p);
>> *d = PROFILE_A2DP;
>> - } else if (pa_streq(uuid, A2DP_SOURCE_UUID)) {
>> + } else if (t->profile == PROFILE_A2DP_SOURCE) {
>> p = pa_card_profile_new("a2dp_source", _("High Fidelity Capture (A2DP)"), sizeof(enum profile));
>> p->priority = 10;
>> p->n_sinks = 0;
>> @@ -2102,7 +2118,7 @@ static pa_card_profile *create_card_profile(struct userdata *u, const char *uuid
>>
>> d = PA_CARD_PROFILE_DATA(p);
>> *d = PROFILE_A2DP_SOURCE;
>> - } else if (pa_streq(uuid, HSP_HS_UUID) || pa_streq(uuid, HFP_HS_UUID)) {
>> + } else if (t->profile == PROFILE_HSP) {
>> p = pa_card_profile_new("hsp", _("Telephony Duplex (HSP/HFP)"), sizeof(enum profile));
>> p->priority = 20;
>> p->n_sinks = 1;
>> @@ -2112,7 +2128,7 @@ static pa_card_profile *create_card_profile(struct userdata *u, const char *uuid
>>
>> d = PA_CARD_PROFILE_DATA(p);
>> *d = PROFILE_HSP;
>> - } else if (pa_streq(uuid, HFP_AG_UUID)) {
>> + } else if (t->profile == PROFILE_HFGW) {
>> p = pa_card_profile_new("hfgw", _("Handsfree Gateway"), sizeof(enum profile));
>> p->priority = 20;
>> p->n_sinks = 1;
>> @@ -2124,12 +2140,8 @@ static pa_card_profile *create_card_profile(struct userdata *u, const char *uuid
>> *d = PROFILE_HFGW;
>> }
>>
>> - if (p) {
>> - pa_bluetooth_transport *t;
>> -
>> - if ((t = u->device->transports[*d]))
>> - p->available = transport_state_to_availability(t->state);
>> - }
>> + if (p)
>> + p->available = transport_state_to_availability(t->state);
>>
>> return p;
>> }
>> @@ -2144,7 +2156,7 @@ static int add_card(struct userdata *u) {
>> char *n;
>> const char *default_profile;
>> const pa_bluetooth_device *device = u->device;
>> - const pa_bluetooth_uuid *uuid;
>> + int i;
>>
>> pa_assert(u);
>> pa_assert(device);
>> @@ -2174,9 +2186,13 @@ static int add_card(struct userdata *u) {
>> return -1;
>> }
>>
>> - PA_LLIST_FOREACH(uuid, device->uuids) {
>> - p = create_card_profile(u, uuid->uuid);
>> + for (i = 0; i < PA_BLUETOOTH_PROFILE_COUNT; i++) {
>> + pa_bluetooth_transport *t;
>> +
>> + if (!(t = device->transports[i]))
>> + continue;
>>
>> + p = create_card_profile(u, t);
>> if (!p)
>> continue;
>>
>> --
>> 1.7.11.7
>>
>
> With this patch I understand better what patch 15 is doing, but I
> still don't understand why you want to change this.
>
> Transport objects come and go based on the connection state and I
> don't think card profiles are originally designed to be created and
> destroyed this way.
>
If you're refering to card profiles as a whole in PulseAudio, I don't
know if they were designed to be added and removed dinamically to a
card, but the fact that the card API allows it makes me think that
there is no problem in doing so (although the only user of
pa_card_add_profile() is module-bluetooth-device.c). If you're
refering to the module-bluetooth-device card profiles, if I'm not
mistaken (Luiz will know better about this) in the past it was
possible that UUIDs would be added to the device object after the
connection, and uuid_added_cb() was there exactly to handle this
situation. FWIW, nowadays we only do SDP on device discovery and
pairing, so the UUID list will remain the same for the lifetime of a
bonding.
Additionally, with BlueZ 5 the UUID list doesn't reflect what profiles
should be available on the card anymore, for 2 reasons:
1. With the addition of external profiles, it is possible to have a
UUID listed in the UUID list of the device but do not have support for
that. An example of this case is HFP, which is now implemented
externally by oFono. The HFP UUIDs appear for the device UUID list
even if there is no implementation of the profile.
2. It's possible to connect profiles individually, and starting with
BlueZ this can be done on BlueZ-initiated connections. So if a
Bluetooth profile is not connected, it will never be possible to
switch to that profile, unless the user triggers a connection to that
profile out of PulseAudio.
In both cases there will be a card profile present that is not in a
usable state, and might never be (reason #1 listed above). This will
just confuse the user. When it tries to switch to that profile the
card will refuse to switch, at least in pavucontrol, this is not
reflected by the UI (although this is probably a bug in the UI). Even
if the UI handles it nicely, there will be a choice available for the
user that never works.
OTOH, having card profiles availability based on transport
availability the profiles available only if that Bluetooth profile is
supported and connected (not necessarily playing), that is, the
profile is really available to be used. Taking the reasoning above
into consideration, I don't see any drawbacks of having card profile
availability based on transport availability, but if you think there
is any disadvantage in this scheme, please bring it up so we can avoid
headaches.
--
Jo?o Paulo Rechi Vita
http://about.me/jprvita
