From: Mikel Astiz <[email protected]>
Same as v4 but rebased on the latest pushed patches.
>From original patch:
This patch reopens the discussion started by the thread "when is acquire
ok to call". The race condition seems to be real (even thought difficult
to reproduce), and I couldn't think of any approach to solve this
without altering the Media API.
Mikel Astiz (4):
media: Split transport state based on playing flag
media: Expose transport state in D-Bus
media: Automatically release transport when HUP
media: Extend media API with optional acquire
audio/transport.c | 113 +++++++++++++++++++++++++++++++++++++++++++++++-----
doc/media-api.txt | 19 +++++++++
2 files changed, 121 insertions(+), 11 deletions(-)
--
1.7.7.6
Hi Mikel,
On Fri, Sep 14, 2012 at 3:55 PM, Mikel Astiz <[email protected]> wrote:
> From: Mikel Astiz <[email protected]>
>
> Same as v4 but rebased on the latest pushed patches.
>
> From original patch:
>
> This patch reopens the discussion started by the thread "when is acquire
> ok to call". The race condition seems to be real (even thought difficult
> to reproduce), and I couldn't think of any approach to solve this
> without altering the Media API.
>
> Mikel Astiz (4):
> media: Split transport state based on playing flag
> media: Expose transport state in D-Bus
> media: Automatically release transport when HUP
> media: Extend media API with optional acquire
>
> audio/transport.c | 113 +++++++++++++++++++++++++++++++++++++++++++++++-----
> doc/media-api.txt | 19 +++++++++
> 2 files changed, 121 insertions(+), 11 deletions(-)
>
> --
> 1.7.7.6
All 4 patches are now upstream, thanks.
--
Luiz Augusto von Dentz
From: Mikel Astiz <[email protected]>
Acquiring a transport is needed in two different situations: either
we are initiating the audio stream locally, or the remote side initiated
it and thus we are just reacting. In the second case, we would expect
the stream is already available, and otherwise the operation should
fail. This means the media API needs to be extended in order to make
this difference.
This issue is specially relevant in the case of SCO, because the current
approach is racy. With HFP, for example (say BlueZ has the HS role), the
following race condition could be met:
1. Phone has an incoming call and thus starts in-band ringing.
2. SCO connection is accepted and stablished by BlueZ.
3. Gateway interface state is changed to Playing.
4. Exactly afterwards, the user routes the audio to the phone, to have
a private conversation. So the SCO link is closed.
5. In parallel, PulseAudio sees the transition to Playing, and acquires
the transport.
6. BlueZ receives an Acquire() request, but SCO is down. So it tries to
reconnect the SCO link.
The last step is an undesired behavior (the audio is routed back to the
car). BlueZ should be smart enough to know that the SCO connection
shouldn't be reestablished, but this is only possible if the endpoint
provides additional information in the media API.
Note that the API change introduced by this patch is backward
compatible, meaning that older versions of BlueZ will just ignore the
flag. So clients are encouraged to use it without necessarily adding a
dependency to newer versions of BlueZ.
---
audio/transport.c | 4 ++++
doc/media-api.txt | 11 +++++++++++
2 files changed, 15 insertions(+), 0 deletions(-)
diff --git a/audio/transport.c b/audio/transport.c
index 4653cc7..cc9c56f 100644
--- a/audio/transport.c
+++ b/audio/transport.c
@@ -807,6 +807,10 @@ static DBusMessage *acquire(DBusConnection *conn, DBusMessage *msg,
if (lock == 0)
return btd_error_invalid_args(msg);
+ if (transport->state != TRANSPORT_STATE_PENDING &&
+ g_strstr_len(accesstype, -1, "?") != NULL)
+ return btd_error_failed(msg, "Transport not playing");
+
if (media_transport_acquire(transport, lock) == FALSE)
return btd_error_not_authorized(msg);
diff --git a/doc/media-api.txt b/doc/media-api.txt
index da8ef53..b9802d9 100644
--- a/doc/media-api.txt
+++ b/doc/media-api.txt
@@ -282,6 +282,17 @@ Methods dict GetProperties()
"rw": Read and write access
+ The accesstype string can also be combined with a "?"
+ suffix, which will make the request optional. This
+ typically means the transport will only be acquired if
+ it is already available (remote-initiated), but
+ otherwise no request will be sent to the remote side.
+ In this last case the function will fail. Note that,
+ due to compatibility issues with older versions of
+ BlueZ, clients are encouraged to use exactly the same
+ accesstype for Release(), matching the string provided
+ to Acquire().
+
void Release(string accesstype)
Releases file descriptor.
--
1.7.7.6
From: Mikel Astiz <[email protected]>
Extend the Media API to expose the transport state in D-Bus, as a
property of the transport. This way the clients do not have to find
out which is the corresponding profile-specific interface for the
transport.
Additionally, this state along with the automatic release of transports
will allow clients to avoid the "optional release" or "accept remote
release" race condition. For example, with HSP/HFP profiles, the problem
is the following:
1. User suspends SCO in the remote end.
2. BlueZ signals the Playing->Connected state change in D-Bus.
3. Exactly afterwards, the user resumes SCO in the remote end.
4. In parallel, PulseAudio sees the aforementioned transition to
Connected, and thus releases the transport.
5. BlueZ receives a Release() request while SCO is up. So the audio
stream will be suspended.
The last step is an undesired behavior since the user explicitly wanted
to route the audio stream through Bluetooth.
The issue is difficult to reproduce but it can easily be solved by
exposing the transport state in D-Bus.
---
audio/transport.c | 29 +++++++++++++++++++++++++++++
doc/media-api.txt | 8 ++++++++
2 files changed, 37 insertions(+), 0 deletions(-)
diff --git a/audio/transport.c b/audio/transport.c
index e0402ad..742cc72 100644
--- a/audio/transport.c
+++ b/audio/transport.c
@@ -155,6 +155,22 @@ static transport_lock_t str2lock(const char *str)
return lock;
}
+static const char *state2str(transport_state_t state)
+{
+ switch (state) {
+ case TRANSPORT_STATE_IDLE:
+ case TRANSPORT_STATE_REQUESTING:
+ return "idle";
+ case TRANSPORT_STATE_PENDING:
+ return "pending";
+ case TRANSPORT_STATE_ACTIVE:
+ case TRANSPORT_STATE_SUSPENDING:
+ return "active";
+ }
+
+ return NULL;
+}
+
static gboolean state_in_use(transport_state_t state)
{
switch (state) {
@@ -174,6 +190,7 @@ static void transport_set_state(struct media_transport *transport,
transport_state_t state)
{
transport_state_t old_state = transport->state;
+ const char *str;
if (old_state == state)
return;
@@ -182,6 +199,13 @@ static void transport_set_state(struct media_transport *transport,
DBG("State changed %s: %s -> %s", transport->path, str_state[old_state],
str_state[state]);
+
+ str = state2str(state);
+
+ if (g_strcmp0(str, state2str(old_state)) != 0)
+ emit_property_changed(transport->conn, transport->path,
+ MEDIA_TRANSPORT_INTERFACE, "State",
+ DBUS_TYPE_STRING, &str);
}
void media_transport_destroy(struct media_transport *transport)
@@ -1017,6 +1041,7 @@ void transport_get_properties(struct media_transport *transport,
DBusMessageIter dict;
const char *uuid;
uint8_t codec;
+ const char *state;
dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
DBUS_DICT_ENTRY_BEGIN_CHAR_AS_STRING
@@ -1036,6 +1061,10 @@ void transport_get_properties(struct media_transport *transport,
dict_append_array(&dict, "Configuration", DBUS_TYPE_BYTE,
&transport->configuration, transport->size);
+ /* State */
+ state = state2str(transport->state);
+ dict_append_entry(&dict, "State", DBUS_TYPE_STRING, &state);
+
if (transport->get_properties)
transport->get_properties(transport, &dict);
diff --git a/doc/media-api.txt b/doc/media-api.txt
index e5eeaa0..da8ef53 100644
--- a/doc/media-api.txt
+++ b/doc/media-api.txt
@@ -317,6 +317,14 @@ Properties object Device [readonly]
Configuration blob, it is used as it is so the size and
byte order must match.
+ string State [readonly]
+
+ Indicates the state of the transport. Possible
+ values are:
+ "idle": not streaming
+ "pending": streaming but not acquired
+ "active": streaming and acquired
+
uint16 Delay [readwrite]
Optional. Transport delay in 1/10 of millisecond, this
--
1.7.7.6
From: Mikel Astiz <[email protected]>
When the remote end suspends the audio stream, release the transport
automatically without waiting until the clients call Release().
This affects the D-Bus API since clients will get an error when trying
to release the transport afterwards.
However, this should have no real impact, since most clients (i.e.
PulseAudio) would just log some error trace but otherwise ignore the
issue.
---
audio/transport.c | 14 +++++++++++++-
1 files changed, 13 insertions(+), 1 deletions(-)
diff --git a/audio/transport.c b/audio/transport.c
index 742cc72..4653cc7 100644
--- a/audio/transport.c
+++ b/audio/transport.c
@@ -60,7 +60,7 @@ typedef enum {
TRANSPORT_STATE_IDLE, /* Not acquired and suspended */
TRANSPORT_STATE_PENDING, /* Playing but not acquired */
TRANSPORT_STATE_REQUESTING, /* Acquire in progress */
- TRANSPORT_STATE_ACTIVE, /* Acquired (not necessarily playing) */
+ TRANSPORT_STATE_ACTIVE, /* Acquired and playing */
TRANSPORT_STATE_SUSPENDING, /* Release in progress */
} transport_state_t;
@@ -1178,6 +1178,18 @@ static void transport_update_playing(struct media_transport *transport,
if (playing == FALSE) {
if (transport->state == TRANSPORT_STATE_PENDING)
transport_set_state(transport, TRANSPORT_STATE_IDLE);
+ else if (transport->state == TRANSPORT_STATE_ACTIVE) {
+ /* Remove all owners */
+ while (transport->owners != NULL) {
+ struct media_owner *owner;
+
+ owner = transport->owners->data;
+ media_transport_remove(transport, owner);
+ }
+
+ /* Suspend so that the locks get released */
+ transport->suspend(transport, NULL);
+ }
} else if (transport->state == TRANSPORT_STATE_IDLE)
transport_set_state(transport, TRANSPORT_STATE_PENDING);
}
--
1.7.7.6
From: Mikel Astiz <[email protected]>
Split the transport states (formerly in_use) into more specific states
where the stream state (playing or suspended) is explicitly represented,
along with the transitional states (locally initiated suspend and
resume).
TRANSPORT_STATE_ACTIVE is an exception since it also includes the state
where the transport is acquired, but the audio was later suspended (not
released yet though).
---
audio/transport.c | 68 ++++++++++++++++++++++++++++++++++++++++++++--------
1 files changed, 57 insertions(+), 11 deletions(-)
diff --git a/audio/transport.c b/audio/transport.c
index 3026022..e0402ad 100644
--- a/audio/transport.c
+++ b/audio/transport.c
@@ -57,13 +57,19 @@ typedef enum {
} transport_lock_t;
typedef enum {
- TRANSPORT_STATE_IDLE, /* Not acquired */
- TRANSPORT_STATE_ACQUIRED, /* Acquired (not necessarily playing) */
+ TRANSPORT_STATE_IDLE, /* Not acquired and suspended */
+ TRANSPORT_STATE_PENDING, /* Playing but not acquired */
+ TRANSPORT_STATE_REQUESTING, /* Acquire in progress */
+ TRANSPORT_STATE_ACTIVE, /* Acquired (not necessarily playing) */
+ TRANSPORT_STATE_SUSPENDING, /* Release in progress */
} transport_state_t;
static char *str_state[] = {
"TRANSPORT_STATE_IDLE",
- "TRANSPORT_STATE_ACQUIRED",
+ "TRANSPORT_STATE_PENDING",
+ "TRANSPORT_STATE_REQUESTING",
+ "TRANSPORT_STATE_ACTIVE",
+ "TRANSPORT_STATE_SUSPENDING",
};
struct media_request {
@@ -153,8 +159,11 @@ static gboolean state_in_use(transport_state_t state)
{
switch (state) {
case TRANSPORT_STATE_IDLE:
+ case TRANSPORT_STATE_PENDING:
return FALSE;
- case TRANSPORT_STATE_ACQUIRED:
+ case TRANSPORT_STATE_REQUESTING:
+ case TRANSPORT_STATE_ACTIVE:
+ case TRANSPORT_STATE_SUSPENDING:
return TRUE;
}
@@ -356,6 +365,8 @@ static void a2dp_resume_complete(struct avdtp *session,
media_owner_remove(owner);
+ transport_set_state(transport, TRANSPORT_STATE_ACTIVE);
+
return;
fail:
@@ -382,7 +393,8 @@ static guint resume_a2dp(struct media_transport *transport,
if (a2dp_sep_lock(sep, a2dp->session) == FALSE)
return 0;
- transport_set_state(transport, TRANSPORT_STATE_ACQUIRED);
+ if (transport->state == TRANSPORT_STATE_IDLE)
+ transport_set_state(transport, TRANSPORT_STATE_REQUESTING);
done:
return a2dp_resume(a2dp->session, sep, a2dp_resume_complete, owner);
@@ -417,7 +429,12 @@ static guint suspend_a2dp(struct media_transport *transport,
if (!owner) {
a2dp_sep_unlock(sep, a2dp->session);
- transport_set_state(transport, TRANSPORT_STATE_IDLE);
+
+ if (a2dp_sep_is_playing(sep))
+ transport_set_state(transport, TRANSPORT_STATE_PENDING);
+ else
+ transport_set_state(transport, TRANSPORT_STATE_IDLE);
+
return 0;
}
@@ -468,6 +485,8 @@ static void headset_resume_complete(struct audio_device *dev, void *user_data)
media_owner_remove(owner);
+ transport_set_state(transport, TRANSPORT_STATE_ACTIVE);
+
return;
fail:
@@ -486,7 +505,8 @@ static guint resume_headset(struct media_transport *transport,
HEADSET_LOCK_WRITE) == FALSE)
return 0;
- transport_set_state(transport, TRANSPORT_STATE_ACQUIRED);
+ if (transport->state == TRANSPORT_STATE_IDLE)
+ transport_set_state(transport, TRANSPORT_STATE_REQUESTING);
done:
return headset_request_stream(device, headset_resume_complete,
@@ -516,8 +536,15 @@ static guint suspend_headset(struct media_transport *transport,
struct audio_device *device = transport->device;
if (!owner) {
+ headset_state_t state = headset_get_state(device);
+
headset_unlock(device, HEADSET_LOCK_READ | HEADSET_LOCK_WRITE);
- transport_set_state(transport, TRANSPORT_STATE_IDLE);
+
+ if (state == HEADSET_STATE_PLAYING)
+ transport_set_state(transport, TRANSPORT_STATE_PENDING);
+ else
+ transport_set_state(transport, TRANSPORT_STATE_IDLE);
+
return 0;
}
@@ -574,6 +601,8 @@ static void gateway_resume_complete(struct audio_device *dev, GError *err,
media_owner_remove(owner);
+ transport_set_state(transport, TRANSPORT_STATE_ACTIVE);
+
return;
fail:
@@ -592,7 +621,8 @@ static guint resume_gateway(struct media_transport *transport,
GATEWAY_LOCK_WRITE) == FALSE)
return 0;
- transport_set_state(transport, TRANSPORT_STATE_ACQUIRED);
+ if (transport->state == TRANSPORT_STATE_IDLE)
+ transport_set_state(transport, TRANSPORT_STATE_REQUESTING);
done:
return gateway_request_stream(device, gateway_resume_complete,
@@ -625,8 +655,15 @@ static guint suspend_gateway(struct media_transport *transport,
static int id = 1;
if (!owner) {
+ gateway_state_t state = gateway_get_state(device);
+
gateway_unlock(device, GATEWAY_LOCK_READ | GATEWAY_LOCK_WRITE);
- transport_set_state(transport, TRANSPORT_STATE_IDLE);
+
+ if (state == GATEWAY_STATE_PLAYING)
+ transport_set_state(transport, TRANSPORT_STATE_PENDING);
+ else
+ transport_set_state(transport, TRANSPORT_STATE_IDLE);
+
return 0;
}
@@ -806,6 +843,8 @@ static DBusMessage *release(DBusConnection *conn, DBusMessage *msg,
return btd_error_in_progress(msg);
}
+ transport_set_state(transport, TRANSPORT_STATE_SUSPENDING);
+
id = transport->suspend(transport, owner);
if (id == 0) {
media_transport_remove(transport, owner);
@@ -1104,7 +1143,14 @@ static void headset_nrec_changed(struct audio_device *dev, gboolean nrec,
static void transport_update_playing(struct media_transport *transport,
gboolean playing)
{
- DBG("%s Playing=%d", transport->path, playing);
+ DBG("%s State=%s Playing=%d", transport->path,
+ str_state[transport->state], playing);
+
+ if (playing == FALSE) {
+ if (transport->state == TRANSPORT_STATE_PENDING)
+ transport_set_state(transport, TRANSPORT_STATE_IDLE);
+ } else if (transport->state == TRANSPORT_STATE_IDLE)
+ transport_set_state(transport, TRANSPORT_STATE_PENDING);
}
static void headset_state_changed(struct audio_device *dev,
--
1.7.7.6