[pulseaudio-discuss] [RFC v0 12/15] bluetooth: Update to new BlueZ 5 transport acquire/release API

[Mikel Astiz]

Hi Tanu,

On Wed, Jan 2, 2013 at 2:04 PM, Tanu Kaskinen <tanuk at iki.fi> wrote:
> On Wed, 2012-12-19 at 13:58 +0100, Mikel Astiz wrote:
>> From: Mikel Astiz <mikel.astiz at bmw-carit.de>
>>
>> The new D-Bus API doesn't support access rights, which weren't used by
>> PulseAudio anyway, but it does solve a race condition: now optional
>> acquires can be implemented by bluetooth-util atomically using the D-Bus
>> TryAcquire() method.
>> ---
>>  src/modules/bluetooth/bluetooth-util.c | 64 +++++++++++++++++++++-------------
>>  1 file changed, 39 insertions(+), 25 deletions(-)
>>
>> diff --git a/src/modules/bluetooth/bluetooth-util.c b/src/modules/bluetooth/bluetooth-util.c
>> index 365a546..001e179 100644
>> --- a/src/modules/bluetooth/bluetooth-util.c
>> +++ b/src/modules/bluetooth/bluetooth-util.c
>> @@ -1440,8 +1440,6 @@ bool pa_bluetooth_device_any_audio_connected(const pa_bluetooth_device *d) {
>>  }
>>
>>  int pa_bluetooth_transport_acquire(pa_bluetooth_transport *t, bool optional, size_t *imtu, size_t *omtu) {
>> -    const char *accesstype = "rw";
>> -    const char *interface;
>>      DBusMessage *m, *r;
>>      DBusError err;
>>      int ret;
>> @@ -1451,29 +1449,41 @@ int pa_bluetooth_transport_acquire(pa_bluetooth_transport *t, bool optional, siz
>>      pa_assert(t->device);
>>      pa_assert(t->device->discovery);
>>
>> -    interface = t->device->discovery->version == BLUEZ_VERSION_4 ? "org.bluez.MediaTransport" : "org.bluez.MediaTransport1";
>> -
>> -    if (optional) {
>> -        /* FIXME: we are trying to acquire the transport only if the stream is
>> -           playing, without actually initiating the stream request from our side
>> -           (which is typically undesireable specially for hfgw use-cases.
>> -           However this approach is racy, since the stream could have been
>> -           suspended in the meantime, so we can't really guarantee that the
>> -           stream will not be requested until BlueZ's API supports this
>> -           atomically. */
>> -        if (t->state < PA_BLUETOOTH_TRANSPORT_STATE_PLAYING) {
>> -            pa_log_info("Failed optional acquire of transport %s", t->path);
>> -            return -1;
>> +    dbus_error_init(&err);
>> +
>> +    if (t->device->discovery->version == BLUEZ_VERSION_4) {
>> +        const char *accesstype = "rw";
>> +
>> +        if (optional) {
>> +            /* We are trying to acquire the transport only if the stream is
>> +               playing, without actually initiating the stream request from our side
>> +               (which is typically undesireable specially for hfgw use-cases.
>> +               However this approach is racy, since the stream could have been
>> +               suspended in the meantime, so we can't really guarantee that the
>> +               stream will not be requested with the API in BlueZ 4.x */
>> +            if (t->state < PA_BLUETOOTH_TRANSPORT_STATE_PLAYING) {
>> +                pa_log_info("Failed optional acquire of transport %s", t->path);
>> +                return -1;
>> +            }
>>          }
>> -    }
>>
>> -    dbus_error_init(&err);
>> +        pa_assert_se(m = dbus_message_new_method_call(t->owner, t->path, "org.bluez.MediaTransport", "Acquire"));
>> +        pa_assert_se(dbus_message_append_args(m, DBUS_TYPE_STRING, &accesstype, DBUS_TYPE_INVALID));
>> +    } else {
>> +        pa_assert(t->device->discovery->version == BLUEZ_VERSION_5);
>> +
>> +        if (optional)
>> +            pa_assert_se(m = dbus_message_new_method_call(t->owner, t->path, "org.bluez.MediaTransport1", "TryAcquire"));
>> +        else
>> +            pa_assert_se(m = dbus_message_new_method_call(t->owner, t->path, "org.bluez.MediaTransport1", "Acquire"));
>> +    }
>>
>> -    pa_assert_se(m = dbus_message_new_method_call(t->owner, t->path, interface, "Acquire"));
>> -    pa_assert_se(dbus_message_append_args(m, DBUS_TYPE_STRING, &accesstype, DBUS_TYPE_INVALID));
>>      r = dbus_connection_send_with_reply_and_block(pa_dbus_connection_get(t->device->discovery->connection), m, -1, &err);
>>
>>      if (dbus_error_is_set(&err) || !r) {
>
> Checking both things is redundant, and raises the question whether it's
> possible that one of the conditions is true and the other is false.
> That's not possible.

I'll send a separate patch fixing this and other similar occurrences
since this line is not part of the patch.

>
>> +        if (optional)
>> +            pa_log_info("Failed optional acquire of transport %s", t->path);
>
> An error message should be printed if this is not an optional acquire.

Given that the caller also prints some traces, I will drop this one entirely.

> The messages would be more informative if they included the error name
> and message from err.

TryAcquire() would typically fail because the audio is down by the
time TryAcquire() is processed, so I see little benefit in trying to
log this information, with the exception perhaps of standard D-Bus
errors. I will discuss in BlueZ's community if TryAcquire() should
guarantee a specific error in this typical audio-down scenario, so the
rest of the errors could be logged.

Cheers,
Mikel