Re: [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 <ge...@chini.tk> wrote:

On 08.05.2017 10:37, Luiz Augusto von Dentz wrote:

From: Luiz Augusto von Dentz <luiz.von.de...@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.
_______________________________________________
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss