[pulseaudio-discuss] [RFC 21/24] bluetooth: Fix stream setup for PROFILE_HFP_AG

[Mikel Astiz]

Hi Jõao 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:37 AM, Mikel Astiz <mikel.astiz.oss at gmail.com> wrote:
>> Hi João Paulo,
>>
>> On Wed, Mar 27, 2013 at 6:17 AM,  <jprvita at gmail.com> wrote:
>>> From: João Paulo Rechi Vita <jprvita at openbossa.org>
>>>
>>> Use the correct value for block size and instead of trying to acquire
>>> the transport, get it from the HF card.
>>> ---
>>>  src/modules/bluetooth/bluetooth-util.c          | 15 +++++++++++++++
>>>  src/modules/bluetooth/module-bluetooth-device.c |  2 +-
>>>  2 files changed, 16 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/modules/bluetooth/bluetooth-util.c b/src/modules/bluetooth/bluetooth-util.c
>>> index a2be396..37d96ee 100644
>>> --- a/src/modules/bluetooth/bluetooth-util.c
>>> +++ b/src/modules/bluetooth/bluetooth-util.c
>>> @@ -1838,6 +1838,21 @@ int pa_bluetooth_transport_acquire(pa_bluetooth_transport *t, bool optional, siz
>>>      pa_assert(t->device);
>>>      pa_assert(t->device->discovery);
>>>
>>> +    if (t->profile == PROFILE_HFP_AG) {
>>> +        struct handsfree_card *hf_card = pa_hashmap_get(t->device->discovery->hf_cards, t->path);
>>> +
>>> +        /* TODO: get block size from adapter and USB endpoint */
>>> +        if (imtu)
>>> +            *imtu = 48;
>>> +        if (omtu)
>>> +            *omtu = 48;
>>> +
>>> +        if (hf_card)
>>> +            return hf_card->fd;
>>> +        else
>>> +            return -1;
>>> +    }
>>> +
>>>      dbus_error_init(&err);
>>>
>>>      if (t->device->discovery->version == BLUEZ_VERSION_4) {
>>> diff --git a/src/modules/bluetooth/module-bluetooth-device.c b/src/modules/bluetooth/module-bluetooth-device.c
>>> index 1027dd5..47457e5 100644
>>> --- a/src/modules/bluetooth/module-bluetooth-device.c
>>> +++ b/src/modules/bluetooth/module-bluetooth-device.c
>>> @@ -243,7 +243,7 @@ static void a2dp_set_bitpool(struct userdata *u, uint8_t bitpool)
>>>  /* from IO thread, except in SCO over PCM */
>>>  static void bt_transport_config_mtu(struct userdata *u) {
>>>      /* Calculate block sizes */
>>> -    if (u->profile == PROFILE_HSP || u->profile == PROFILE_HFGW) {
>>> +    if (u->profile == PROFILE_HSP || u->profile == PROFILE_HFGW || u->profile == PROFILE_HFP_AG) {
>>>          u->read_block_size = u->read_link_mtu;
>>>          u->write_block_size = u->write_link_mtu;
>>>      } else {
>>> --
>>> 1.7.11.7
>>>
>>
>> This is actually not what pa_bluetooth_transport_acquire() is supposed
>> to do. What you implemented works only if optional==true, but
>> otherwise you're supposed to initiate the audio stream.
>>
>
> You're right, I've overlooked this in the eager of making it work.
>
>> Having a look at oFono's API, you should be calling
>> HandsfreeAudioCard.Connect(), and block until the fd is received. I'm
>> eager to see how you achieve this since oFono's API seems quite
>> inconvenient to implement this, given that Connect() doesn't return
>> the fd. This is admittedly a problem in PA, but due to practical
>> reasons, unless you find a nice way to block until the NewConnection
>> call is received, I would suggest oFono's API should be modified to
>> return this fd as a result of HandsfreeAudioCard.Connect() for
>> convenience of PA-like clients.
>>
>
> I think when optional==false and hf_card->fd==-1 we should simply
> return -1 for the caller, so transport acquire would fail and the
> sink/source would be (kept) suspended. Later on, when oFono calls
> NewConnection() we will have to signalize the I/O thread that the
> transport is now available to be acquired. I think the use-case that
> relates to this is when the HF wants to ask the AG to transfer the
> audio connection from the AG to the HF, right? How are you planning to
> trigger this by the UI? Maybe at this point the UI could tell the user

That's exactly the use-case. You can for example achieve this in
pavucontrol by streaming some audio to the "hfgw" sink, assuming
module-suspend-on-idle is loaded.

> something like "audio connection is not yet established" and
> automatically serve that user when the transport become available and
> the sink/source resumes. BTW, this isn't the same way we'll handle
> audio connections initiated by the AG, just with the addition of the
> "we're not ready yet" intermediate step?

I think there's very little point in trying to deny that PulseAudio
should be able to trigger SCO connections. This is specially obvious
for the inverse role (i.e. when PA plays the gateway role), where we
could list multiple obvious cases.

Unless you have another proposal, the way PulseAudio requests an SCO
connection is by using pa_bluetooth_transport_acquire(). And it has to
be blocking, because the SCO state maps to the sink-source suspended
flag.

Cheers,
Mikel