[RFC v2 0/4] Optional acquire in Media API

5 messages, 1 author, 2012-07-26 · open the first message on its own page

[RFC v2 0/4] Optional acquire in Media API

From: Mikel Astiz <hidden>
Date: 2012-07-26 10:13:25

From: Mikel Astiz <redacted>

NOTE: please do not apply this patches until the PulseAudio part gets completely clarified.

This third proposal the feedback from Luiz (some code refactoring and commit message extended to insist on backward compatibility), along with an important fix in patch v1 1/4.

Otherwise the idea remains the same: add a "?" flag in the Acquire method to avoid a race condition between BlueZ and PulseAudio.

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: Watch A2DP sink-source state changes
  media: Add boolean playing field to transport
  media: Extend media API with optional acquire
  media: Remove transport in_use flag

 audio/media.c     |   75 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 audio/transport.c |   38 +++++++++++++-------------
 audio/transport.h |    2 +
 doc/media-api.txt |    7 +++++
 4 files changed, 100 insertions(+), 22 deletions(-)

-- 
1.7.7.6

[RFC v2 1/4] media: Watch A2DP sink-source state changes

From: Mikel Astiz <hidden>
Date: 2012-07-26 10:13:26

From: Mikel Astiz <redacted>

Add watches to know about the state changes affecting the A2DP sinks
and sources. The callback functions are just skeletons to be used in
following patches.
---
 audio/media.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 46 insertions(+), 0 deletions(-)

diff --git a/audio/media.c b/audio/media.c
index ea6d582..505a490 100644
--- a/audio/media.c
+++ b/audio/media.c

@@ -48,6 +48,8 @@
 #include "avrcp.h"
 #include "headset.h"
 #include "gateway.h"
+#include "sink.h"
+#include "source.h"
 #include "manager.h"
 
 #define MEDIA_INTERFACE "org.bluez.Media"

@@ -83,6 +85,8 @@ struct media_endpoint {
 	size_t			size;		/* Endpoint capabilities size */
 	guint			hs_watch;
 	guint			ag_watch;
+	guint			sink_watch;
+	guint			source_watch;
 	guint			watch;
 	GSList			*requests;
 	struct media_adapter	*adapter;

@@ -157,6 +161,12 @@ static void media_endpoint_destroy(struct media_endpoint *endpoint)
 	if (endpoint->ag_watch)
 		gateway_remove_state_cb(endpoint->ag_watch);
 
+	if (endpoint->sink_watch)
+		sink_remove_state_cb(endpoint->sink_watch);
+
+	if (endpoint->source_watch)
+		source_remove_state_cb(endpoint->source_watch);
+
 	media_endpoint_cancel_all(endpoint);
 
 	g_slist_free_full(endpoint->transports,

@@ -664,6 +674,21 @@ static void gateway_state_changed(struct audio_device *dev,
 	}
 }
 
+static void a2dp_source_state_changed(struct audio_device *dev,
+					source_state_t old_state,
+					source_state_t new_state,
+					void *user_data)
+{
+	struct media_endpoint *endpoint = user_data;
+	struct media_transport *transport;
+
+	DBG("");
+
+	transport = find_device_transport(endpoint, dev);
+	if (transport == NULL)
+		return;
+}
+
 static gboolean endpoint_init_a2dp_source(struct media_endpoint *endpoint,
 						gboolean delay_reporting,
 						int *err)

@@ -675,9 +700,27 @@ static gboolean endpoint_init_a2dp_source(struct media_endpoint *endpoint,
 	if (endpoint->sep == NULL)
 		return FALSE;
 
+	endpoint->source_watch = source_add_state_cb(a2dp_source_state_changed,
+								endpoint);
+
 	return TRUE;
 }
 
+static void a2dp_sink_state_changed(struct audio_device *dev,
+					sink_state_t old_state,
+					sink_state_t new_state,
+					void *user_data)
+{
+	struct media_endpoint *endpoint = user_data;
+	struct media_transport *transport;
+
+	DBG("");
+
+	transport = find_device_transport(endpoint, dev);
+	if (transport == NULL)
+		return;
+}
+
 static gboolean endpoint_init_a2dp_sink(struct media_endpoint *endpoint,
 						gboolean delay_reporting,
 						int *err)

@@ -689,6 +732,9 @@ static gboolean endpoint_init_a2dp_sink(struct media_endpoint *endpoint,
 	if (endpoint->sep == NULL)
 		return FALSE;
 
+	endpoint->sink_watch = sink_add_state_cb(a2dp_sink_state_changed,
+								endpoint);
+
 	return TRUE;
 }

-- 
1.7.7.6

[RFC v2 2/4] media: Add boolean playing field to transport

From: Mikel Astiz <hidden>
Date: 2012-07-26 10:13:27

From: Mikel Astiz <redacted>

This flag represents whether the transport is actually streaming, which
is mapped trivially after the state changes in gateway, headset and A2DP
sink or sources.
---
 audio/media.c     |   29 ++++++++++++++++++++++++++---
 audio/transport.c |    7 +++++++
 audio/transport.h |    2 ++
 3 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/audio/media.c b/audio/media.c
index 505a490..8eee149 100644
--- a/audio/media.c
+++ b/audio/media.c

@@ -494,12 +494,12 @@ static void headset_state_changed(struct audio_device *dev,
 	switch (new_state) {
 	case HEADSET_STATE_DISCONNECTED:
 		transport = find_device_transport(endpoint, dev);
-
 		if (transport != NULL) {
 			DBG("Clear endpoint %p", endpoint);
+			media_transport_set_playing(transport, FALSE);
 			clear_configuration(endpoint, transport);
 		}
-		break;
+		return;
 	case HEADSET_STATE_CONNECTING:
 		set_configuration(endpoint, dev, NULL, 0, headset_setconf_cb,
 								dev, NULL);

@@ -511,6 +511,12 @@ static void headset_state_changed(struct audio_device *dev,
 	case HEADSET_STATE_PLAYING:
 		break;
 	}
+
+	transport = find_device_transport(endpoint, dev);
+	if (new_state == HEADSET_STATE_PLAYING)
+		media_transport_set_playing(transport, TRUE);
+	else
+		media_transport_set_playing(transport, FALSE);
 }
 
 static const char *get_name(struct a2dp_sep *sep, void *user_data)

@@ -660,9 +666,10 @@ static void gateway_state_changed(struct audio_device *dev,
 		transport = find_device_transport(endpoint, dev);
 		if (transport != NULL) {
 			DBG("Clear endpoint %p", endpoint);
+			media_transport_set_playing(transport, FALSE);
 			clear_configuration(endpoint, transport);
 		}
-		break;
+		return;
 	case GATEWAY_STATE_CONNECTING:
 		set_configuration(endpoint, dev, NULL, 0,
 					gateway_setconf_cb, dev, NULL);

@@ -672,6 +679,12 @@ static void gateway_state_changed(struct audio_device *dev,
 	case GATEWAY_STATE_PLAYING:
 		break;
 	}
+
+	transport = find_device_transport(endpoint, dev);
+	if (new_state == GATEWAY_STATE_PLAYING)
+		media_transport_set_playing(transport, TRUE);
+	else
+		media_transport_set_playing(transport, FALSE);
 }
 
 static void a2dp_source_state_changed(struct audio_device *dev,

@@ -687,6 +700,11 @@ static void a2dp_source_state_changed(struct audio_device *dev,
 	transport = find_device_transport(endpoint, dev);
 	if (transport == NULL)
 		return;
+
+	if (new_state == SOURCE_STATE_PLAYING)
+		media_transport_set_playing(transport, TRUE);
+	else
+		media_transport_set_playing(transport, FALSE);
 }
 
 static gboolean endpoint_init_a2dp_source(struct media_endpoint *endpoint,

@@ -719,6 +737,11 @@ static void a2dp_sink_state_changed(struct audio_device *dev,
 	transport = find_device_transport(endpoint, dev);
 	if (transport == NULL)
 		return;
+
+	if (new_state == SINK_STATE_PLAYING)
+		media_transport_set_playing(transport, TRUE);
+	else
+		media_transport_set_playing(transport, FALSE);
 }
 
 static gboolean endpoint_init_a2dp_sink(struct media_endpoint *endpoint,

diff --git a/audio/transport.c b/audio/transport.c
index 832ad2a..f988d48 100644
--- a/audio/transport.c
+++ b/audio/transport.c

@@ -86,6 +86,7 @@ struct media_transport {
 	uint16_t		omtu;		/* Transport output mtu */
 	gboolean		read_lock;
 	gboolean		write_lock;
+	gboolean		playing;
 	gboolean		in_use;
 	guint			(*resume) (struct media_transport *transport,
 					struct media_owner *owner);

@@ -898,6 +899,12 @@ static void get_properties_gateway(struct media_transport *transport,
 	/* None */
 }
 
+void media_transport_set_playing(struct media_transport *transport,
+							gboolean playing)
+{
+	transport->playing = playing;
+}
+
 void transport_get_properties(struct media_transport *transport,
 							DBusMessageIter *iter)
 {

diff --git a/audio/transport.h b/audio/transport.h
index d20c327..78323b2 100644
--- a/audio/transport.h
+++ b/audio/transport.h

@@ -37,5 +37,7 @@ void media_transport_update_delay(struct media_transport *transport,
 							uint16_t delay);
 void media_transport_update_volume(struct media_transport *transport,
 								uint8_t volume);
+void media_transport_set_playing(struct media_transport *transport,
+							gboolean playing);
 void transport_get_properties(struct media_transport *transport,
 							DBusMessageIter *iter);

-- 
1.7.7.6

[RFC v2 3/4] media: Extend media API with optional acquire

From: Mikel Astiz <hidden>
Date: 2012-07-26 10:13:28

From: Mikel Astiz <redacted>

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 |    3 +++
 doc/media-api.txt |    7 +++++++
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/audio/transport.c b/audio/transport.c
index f988d48..ffc637c 100644
--- a/audio/transport.c
+++ b/audio/transport.c

@@ -679,6 +679,9 @@ static DBusMessage *acquire(DBusConnection *conn, DBusMessage *msg,
 	if (media_transport_acquire(transport, accesstype) == FALSE)
 		return btd_error_not_authorized(msg);
 
+	if (!transport->playing && g_strstr_len(accesstype, -1, "?") != NULL)
+		return btd_error_failed(msg, "Transport not playing");
+
 	owner = media_owner_create(conn, msg, accesstype);
 	id = transport->resume(transport, owner);
 	if (id == 0) {

diff --git a/doc/media-api.txt b/doc/media-api.txt
index e5eeaa0..4853196 100644
--- a/doc/media-api.txt
+++ b/doc/media-api.txt

@@ -282,6 +282,13 @@ 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.
+
 		void Release(string accesstype)
 
 			Releases file descriptor.

-- 
1.7.7.6

[RFC v2 4/4] media: Remove transport in_use flag

From: Mikel Astiz <hidden>
Date: 2012-07-26 10:13:29

From: Mikel Astiz <redacted>

With the introduction of the playing flag, the previously existing
in_use flag is somehow redundant. The playing flag along with the lock
states implicitly represent the same information.
---
 audio/transport.c |   28 +++++++++-------------------
 1 files changed, 9 insertions(+), 19 deletions(-)

diff --git a/audio/transport.c b/audio/transport.c
index ffc637c..bef04e5 100644
--- a/audio/transport.c
+++ b/audio/transport.c

@@ -87,7 +87,6 @@ struct media_transport {
 	gboolean		read_lock;
 	gboolean		write_lock;
 	gboolean		playing;
-	gboolean		in_use;
 	guint			(*resume) (struct media_transport *transport,
 					struct media_owner *owner);
 	guint			(*suspend) (struct media_transport *transport,

@@ -215,7 +214,7 @@ static void media_transport_remove(struct media_transport *transport,
 	media_owner_free(owner);
 
 	/* Suspend if there is no longer any owner */
-	if (transport->owners == NULL && transport->in_use)
+	if (transport->owners == NULL && transport->playing)
 		transport->suspend(transport, NULL);
 }

@@ -297,11 +296,10 @@ static guint resume_a2dp(struct media_transport *transport,
 			return 0;
 	}
 
-	if (transport->in_use == TRUE)
+	if (transport->playing == TRUE)
 		goto done;
 
-	transport->in_use = a2dp_sep_lock(sep, a2dp->session);
-	if (transport->in_use == FALSE)
+	if (a2dp_sep_lock(sep, a2dp->session) == FALSE)
 		return 0;
 
 done:

@@ -324,7 +322,6 @@ static void a2dp_suspend_complete(struct avdtp *session,
 	}
 
 	a2dp_sep_unlock(sep, a2dp->session);
-	transport->in_use = FALSE;
 	media_transport_remove(transport, owner);
 }

@@ -337,7 +334,6 @@ static guint suspend_a2dp(struct media_transport *transport,
 
 	if (!owner) {
 		a2dp_sep_unlock(sep, a2dp->session);
-		transport->in_use = FALSE;
 		return 0;
 	}

@@ -399,12 +395,11 @@ static guint resume_headset(struct media_transport *transport,
 {
 	struct audio_device *device = transport->device;
 
-	if (transport->in_use == TRUE)
+	if (transport->playing == TRUE)
 		goto done;
 
-	transport->in_use = headset_lock(device, HEADSET_LOCK_READ |
-						HEADSET_LOCK_WRITE);
-	if (transport->in_use == FALSE)
+	if (headset_lock(device, HEADSET_LOCK_READ |
+						HEADSET_LOCK_WRITE) == FALSE)
 		return 0;
 
 done:

@@ -425,7 +420,6 @@ static void headset_suspend_complete(struct audio_device *dev, void *user_data)
 	}
 
 	headset_unlock(dev, HEADSET_LOCK_READ | HEADSET_LOCK_WRITE);
-	transport->in_use = FALSE;
 	media_transport_remove(transport, owner);
 }

@@ -436,7 +430,6 @@ static guint suspend_headset(struct media_transport *transport,
 
 	if (!owner) {
 		headset_unlock(device, HEADSET_LOCK_READ | HEADSET_LOCK_WRITE);
-		transport->in_use = FALSE;
 		return 0;
 	}

@@ -504,12 +497,11 @@ static guint resume_gateway(struct media_transport *transport,
 {
 	struct audio_device *device = transport->device;
 
-	if (transport->in_use == TRUE)
+	if (transport->playing == TRUE)
 		goto done;
 
-	transport->in_use = gateway_lock(device, GATEWAY_LOCK_READ |
-						GATEWAY_LOCK_WRITE);
-	if (transport->in_use == FALSE)
+	if (gateway_lock(device, GATEWAY_LOCK_READ |
+						GATEWAY_LOCK_WRITE) == FALSE)
 		return 0;
 
 done:

@@ -531,7 +523,6 @@ static gboolean gateway_suspend_complete(gpointer user_data)
 	}
 
 	gateway_unlock(device, GATEWAY_LOCK_READ | GATEWAY_LOCK_WRITE);
-	transport->in_use = FALSE;
 	media_transport_remove(transport, owner);
 	return FALSE;
 }

@@ -544,7 +535,6 @@ static guint suspend_gateway(struct media_transport *transport,
 
 	if (!owner) {
 		gateway_unlock(device, GATEWAY_LOCK_READ | GATEWAY_LOCK_WRITE);
-		transport->in_use = FALSE;
 		return 0;
 	}

-- 
1.7.7.6

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help