[pulseaudio-discuss] [RFC 16/24] bluetooth: Create card profiles based on transport
João Paulo Rechi Vita
jprvita at gmail.com
Tue Apr 2 07:11:18 PDT 2013
On Wed, Mar 27, 2013 at 1:37 PM, Mikel Astiz <mikel.astiz.oss at gmail.com> wrote:
> 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.
>
Ok, I wasn't adding the card profile availability flag into the mix.
Considering that we can create the card profiles as unavailable and
make them available later on, I starting to think the one to blame is
pavucontrol again here. But still, that make the logic that creates
the profile for a HFP AG device dependent on BlueZ instead of
oFono-only, making both services more coupled inside PulseAudio.
>>
>> 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.
>
It holds true, I agree. But it doesn't that this is a nice truth ;)
>>
>> 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.
>
Yes, that was what I said as well. So if the profile is not connected
it is not usable and PulseAudio have no way to change that. And we
don't want to force the Audio UIs out there to talk to BlueZ or oFono
to trigger this connection (although some more specific UIs may do
this if they want to).
>>
>> 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.
>
Yes, that's probably true. Is this flag available to the UIs?
>>
>> 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.
>
I still don't think card profiles were designed with this restriction
in mind. If a module like module-card-restore is relying on card
profile availability never changing, it's probably doing something
wrong, since we could remove and re-pair with the same device and it
could have a different UUID list, or BlueZ could change in the future
and do a SDP discovery during the device's bonding lifetime, or even
with the device connected, and its UUID list be completely different.
> In general, profile-unsupported is different from profile-disconnected
> so I find it intuitively more accurate to use the current mapping.
>
In the current mapping we still have a profile-unsupported in the
card, in the case we don't have oFono installed and a device have any
HFP UUID available. But I see your point and to summarize, I think the
profile availability flag is enough to make only the card profiles
that are in a usable state available to the user. I'll taking this
approach and updating the code accordingly.
--
João Paulo Rechi Vita
http://about.me/jprvita
More information about the pulseaudio-discuss
mailing list