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

[Georg Chini]

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.