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

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

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

This patch series adds a flag to the Acquire method in the Media API, in order to fix the race condition described in patch v1 3/4.

v1 tries to follow the proposal from Luiz by avoiding changes in the internal callbacks, and instead using the state information available in struct media_transport.

Patch v1 4/4 needs careful review since the replacement of the in_use flag is not trivial.

>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     |   72 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 audio/transport.c |   38 ++++++++++++++--------------
 audio/transport.h |    2 +
 doc/media-api.txt |    7 +++++
 4 files changed, 98 insertions(+), 21 deletions(-)

-- 
1.7.7.6

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

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

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..1a54f4a 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,10 +674,28 @@ 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)
 {
+	endpoint->source_watch = source_add_state_cb(a2dp_source_state_changed,
+								endpoint);
+
 	endpoint->sep = a2dp_add_sep(&endpoint->adapter->src,
 					AVDTP_SEP_TYPE_SOURCE, endpoint->codec,
 					delay_reporting, &a2dp_endpoint,
@@ -678,10 +706,28 @@ static gboolean endpoint_init_a2dp_source(struct media_endpoint *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)
 {
+	endpoint->sink_watch = sink_add_state_cb(a2dp_sink_state_changed,
+								endpoint);
+
 	endpoint->sep = a2dp_add_sep(&endpoint->adapter->src,
 					AVDTP_SEP_TYPE_SINK, endpoint->codec,
 					delay_reporting, &a2dp_endpoint,
-- 
1.7.7.6

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

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

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     |   26 ++++++++++++++++++++++++--
 audio/transport.c |    7 +++++++
 audio/transport.h |    2 ++
 3 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/audio/media.c b/audio/media.c
index 1a54f4a..d12445f 100644
--- a/audio/media.c
+++ b/audio/media.c
@@ -494,21 +494,26 @@ 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;
 	case HEADSET_STATE_CONNECTING:
 		set_configuration(endpoint, dev, NULL, 0, headset_setconf_cb,
 								dev, NULL);
+		transport = find_device_transport(endpoint, dev);
+		media_transport_set_playing(transport, FALSE);
 		break;
 	case HEADSET_STATE_CONNECTED:
-		break;
 	case HEADSET_STATE_PLAY_IN_PROGRESS:
+		transport = find_device_transport(endpoint, dev);
+		media_transport_set_playing(transport, FALSE);
 		break;
 	case HEADSET_STATE_PLAYING:
+		transport = find_device_transport(endpoint, dev);
+		media_transport_set_playing(transport, TRUE);
 		break;
 	}
 }
@@ -660,16 +665,23 @@ 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;
 	case GATEWAY_STATE_CONNECTING:
 		set_configuration(endpoint, dev, NULL, 0,
 					gateway_setconf_cb, dev, NULL);
+		transport = find_device_transport(endpoint, dev);
+		media_transport_set_playing(transport, FALSE);
 		break;
 	case GATEWAY_STATE_CONNECTED:
+		transport = find_device_transport(endpoint, dev);
+		media_transport_set_playing(transport, FALSE);
 		break;
 	case GATEWAY_STATE_PLAYING:
+		transport = find_device_transport(endpoint, dev);
+		media_transport_set_playing(transport, TRUE);
 		break;
 	}
 }
@@ -687,6 +699,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 +736,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 v1 3/4] media: Extend media API with optional acquire

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

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.
---
 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 v1 4/4] media: Remove transport in_use flag

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

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

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

[Luiz Augusto von Dentz]

Hi Mikel,

On Tue, Jul 24, 2012 at 6:52 PM, Mikel Astiz <mikel.astiz.oss@gmail.com> wrote:
> From: Mikel Astiz <mikel.astiz@bmw-carit.de>
>
> 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     |   26 ++++++++++++++++++++++++--
>  audio/transport.c |    7 +++++++
>  audio/transport.h |    2 ++
>  3 files changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/audio/media.c b/audio/media.c
> index 1a54f4a..d12445f 100644
> --- a/audio/media.c
> +++ b/audio/media.c
> @@ -494,21 +494,26 @@ 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;
>         case HEADSET_STATE_CONNECTING:
>                 set_configuration(endpoint, dev, NULL, 0, headset_setconf_cb,
>                                                                 dev, NULL);
> +               transport = find_device_transport(endpoint, dev);
> +               media_transport_set_playing(transport, FALSE);
>                 break;
>         case HEADSET_STATE_CONNECTED:
> -               break;
>         case HEADSET_STATE_PLAY_IN_PROGRESS:
> +               transport = find_device_transport(endpoint, dev);
> +               media_transport_set_playing(transport, FALSE);
>                 break;
>         case HEADSET_STATE_PLAYING:
> +               transport = find_device_transport(endpoint, dev);
> +               media_transport_set_playing(transport, TRUE);
>                 break;
>         }
>  }

Looks like we can move transport = find_device_transport(endpoint,
dev); above so we don't have do it  in each case.

> @@ -660,16 +665,23 @@ 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;
>         case GATEWAY_STATE_CONNECTING:
>                 set_configuration(endpoint, dev, NULL, 0,
>                                         gateway_setconf_cb, dev, NULL);
> +               transport = find_device_transport(endpoint, dev);
> +               media_transport_set_playing(transport, FALSE);
>                 break;
>         case GATEWAY_STATE_CONNECTED:
> +               transport = find_device_transport(endpoint, dev);
> +               media_transport_set_playing(transport, FALSE);
>                 break;
>         case GATEWAY_STATE_PLAYING:
> +               transport = find_device_transport(endpoint, dev);
> +               media_transport_set_playing(transport, TRUE);
>                 break;
>         }
>  }

Same here.

-- 
Luiz Augusto von Dentz

Re: [RFC v1 0/4] Optional acquire in Media API

[Luiz Augusto von Dentz]

Hi Mikel,

On Tue, Jul 24, 2012 at 6:52 PM, Mikel Astiz <mikel.astiz.oss@gmail.com> wrote:
> From: Mikel Astiz <mikel.astiz@bmw-carit.de>
>
> This patch series adds a flag to the Acquire method in the Media API, in order to fix the race condition described in patch v1 3/4.
>
> v1 tries to follow the proposal from Luiz by avoiding changes in the internal callbacks, and instead using the state information available in struct media_transport.
>
> Patch v1 4/4 needs careful review since the replacement of the in_use flag is not trivial.
>
> 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.

Btw, I guess it is important to clarify that this doesn't break the
API as the new flag is ignored in previous versions. The other
important thing to mention is that this flag is not specific to one
role, otherwise we would have done this without adding a new API,
because there could be some situations where the transport must be
connected regardless of the role.

-- 
Luiz Augusto von Dentz