[pulseaudio-discuss] [RFC 16/24] bluetooth: Create card profiles based on transport
Mikel Astiz
mikel.astiz.oss at gmail.com
Wed Mar 27 09:37:10 PDT 2013
Hi João Paulo,
On Wed, Mar 27, 2013 at 4:54 PM, João Paulo Rechi Vita
<jprvita at gmail.com> wrote:
> 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.
Actually this late UUID handling was added quite recently, see
d4368aa608b79f58a279018eb74abd5a6bff30ac.
As you mention, the use-case is device pairing. PulseAudio might have
already loaded the module (and created card) by the time the UUID is
announced by BlueZ.
Given that devices are not paired so often, and the late UUID case is
even less likely, I still consider your approach an unnecessary abuse
of this feature. If a device supports a UUID, the card profile should
exist. This is also why we (now) have the card profile availability.
>
> 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.
This holds also true for BlueZ 4. If oFono was not installed, you
would never be able to connect HFGW. So it's not a valid argument.
>
> 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.
I'm not following here. PulseAudio doesn't trigger profile connections
in any way, and this hasn't changed from BlueZ 4 to BlueZ 5. The user
(meaning the UI) needs to connect them either individually or
altogether in BlueZ's API or oFono's.
>
> 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.
As you say, this is a problem with the UI. It should probably consider
the card profile availability flag.
>
> 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.
I see little or no benefit in your proposal assuming my points above are valid.
The main disadvantage is what I already mentioned: I believe card
profiles are expected to be fairly static. Modules might be interested
in storing profile-specific information (like module-card-restore) as
long as the card profile exists. Even the UI might want to gray out
"a2dp" profile in case A2DP is supported but not connected.
In general, profile-unsupported is different from profile-disconnected
so I find it intuitively more accurate to use the current mapping.
Cheers,
Mikel
More information about the pulseaudio-discuss
mailing list