[pulseaudio-discuss] [PATCH v2] bluetooth: ofono: Use Acquire method if available

[Georg Chini]

On 10.05.2017 15:35, Georg Chini wrote:
> On 10.05.2017 15:08, Luiz Augusto von Dentz wrote:
>> Hi Georg,
>>
>> On Mon, May 8, 2017 at 9:28 PM, Georg Chini <georg at chini.tk> wrote:
>>> On 08.05.2017 10:37, Luiz Augusto von Dentz wrote:
>>>> From: Luiz Augusto von Dentz <luiz.von.dentz at intel.com>
>>>>
>>>> Attempt to use Acquire method if available falling back to Connect in
>>>> case it fails.
>>>> ---
>>>>    src/modules/bluetooth/backend-ofono.c | 59
>>>> +++++++++++++++++++++++++----------
>>>>    1 file changed, 43 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/src/modules/bluetooth/backend-ofono.c
>>>> b/src/modules/bluetooth/backend-ofono.c
>>>> index 6e9a366..d098402 100644
>>>> --- a/src/modules/bluetooth/backend-ofono.c
>>>> +++ b/src/modules/bluetooth/backend-ofono.c
>>>> @@ -150,6 +150,46 @@ static int socket_accept(int sock)
>>>>        return 0;
>>>>    }
>>>>    +static int card_acquire(struct hf_audio_card *card) {
>>>> +    pa_bluetooth_transport *t = card->transport;
>>>> +    DBusMessage *m, *r;
>>>> +    DBusError err;
>>>> +
>>>> +    if (card->connecting)
>>>> +        return -EAGAIN;
>>>> +
>>>> +    /* Try acquiring the stream first */
>>>> +    dbus_error_init(&err);
>>>> +    pa_assert_se(m = dbus_message_new_method_call(t->owner, t->path,
>>>> "org.ofono.HandsfreeAudioCard", "Acquire"));
>>>> +    r =
>>>> dbus_connection_send_with_reply_and_block(pa_dbus_connection_get(card->backend->connection), 
>>>>
>>>> m, -1, &err);
>>>> +    if (!r) {
>>>> +        if (!pa_streq(err.name, DBUS_ERROR_UNKNOWN_METHOD)) {
>>>> +            pa_log_error("Failed to acquire %s: %s", err.name,
>>>> err.message);
>>>> +            return -1;
>>>> +        }
>>>> +    } else if ((dbus_message_get_args(r, NULL,
>>>> +                         DBUS_TYPE_UNIX_FD, &card->fd,
>>>> +                         DBUS_TYPE_BYTE, &card->codec,
>>>> +                         DBUS_TYPE_INVALID) == true)) {
>>>> +        return card->fd;
>>>> +    } else
>>>> +        return -1;
>>>> +
>>>> +    /* Fallback to Connect as this might be an old version of 
>>>> ofono */
>>>> +    card->connecting = true;
>>>> +
>>>> +    dbus_error_init(&err);
>>>> +    pa_assert_se(m = dbus_message_new_method_call(t->owner, t->path,
>>>> "org.ofono.HandsfreeAudioCard", "Connect"));
>>>> +    r =
>>>> dbus_connection_send_with_reply_and_block(pa_dbus_connection_get(card->backend->connection), 
>>>>
>>>> m, -1, &err);
>>>> +    if (!r)
>>>> +        return -1;
>>>> +
>>>> +    if (card->connecting)
>>>> +        return -EAGAIN;
>>>> +
>>>> +    return card->fd;
>>>> +}
>>>> +
>>>>    static int 
>>>> hf_audio_agent_transport_acquire(pa_bluetooth_transport *t,
>>>> bool optional, size_t *imtu, size_t *omtu) {
>>>>        struct hf_audio_card *card = t->userdata;
>>>>        int err;
>>>> @@ -157,22 +197,9 @@ static int
>>>> hf_audio_agent_transport_acquire(pa_bluetooth_transport *t, bool opti
>>>>        pa_assert(card);
>>>>          if (!optional && card->fd < 0) {
>>>> -        DBusMessage *m, *r;
>>>> -        DBusError derr;
>>>> -
>>>> -        if (card->connecting)
>>>> -            return -EAGAIN;
>>>> -
>>>> -        card->connecting = true;
>>>> -
>>>> -        dbus_error_init(&derr);
>>>> -        pa_assert_se(m = dbus_message_new_method_call(t->owner, 
>>>> t->path,
>>>> "org.ofono.HandsfreeAudioCard", "Connect"));
>>>> -        r =
>>>> dbus_connection_send_with_reply_and_block(pa_dbus_connection_get(card->backend->connection), 
>>>>
>>>> m, -1, &derr);
>>>> -        if (!r)
>>>> -            return -1;
>>>> -
>>>> -        if (card->connecting)
>>>> -            return -EAGAIN;
>>>> +        err = card_acquire(card);
>>>> +        if (err < 0)
>>>> +            return err;
>>>>        }
>>>>          /* The correct block size should take into account the SCO 
>>>> MTU
>>>> from
>>>
>>> In general, it looks OK. However, what seems missing are two things 
>>> which
>>> are normally
>>> done from hf_audio_agent_new_connection():
>>>
>>> 1) card->transport->codec is not set. Or did you mean to use
>>> card->transport->codec
>>> in the dbus_message_get_args() call?
>> It is there already:
>>
>>      } else if ((dbus_message_get_args(r, NULL,
>>                           DBUS_TYPE_UNIX_FD, &card->fd,
>>                           DBUS_TYPE_BYTE, &card->codec,
>>                           DBUS_TYPE_INVALID) == true)) {
>
> Sorry, I see card->codec, not card->transport->codec.
>
>>
>>> 2) pa_bluetooth_transport_set_state() is not called, so the 
>>> transport state
>>> is not updated
>>> properly.
>>>
>>> The call to pa_bluetooth_transport_set_state() is a bit of a problem,
>>> because it is expected
>>> to be run from the main thread and the transport_acquired() function is
>>> called from both
>>> threads. Maybe you need to check if you are in main or I/O thread 
>>> and either
>>> call it directly
>>> or send a message to the main thread.
>> And none of the existing transport do that anyway, so that perhaps
>> shall be handled separately as it is not unique to ofono backend.
>
> What do you mean? The ofono backend at least does it correctly (after
> my patch) and I think it is also valid for the native backend now.
>
I checked this. You are right for the AG role of the native backend. For
the HS role, the transport is set to playing in sco_io_callback() and
set to idle in transport_release(). For the ofono backend it is set to
playing in hf_audio_agent_new_connection() and also set to idle in
transport_release(). So there is only one place left which needs fixing.
I'll send a patch which does that and also fixes the problem for your
patch. It already seems to be working but I want to do a few more
tests before I submit it.
So apart from the typo, there will probably be no further changes
needed for your patch.