From: Mikel Astiz <[email protected]>
This patch series includes minor modifications that have been useful while testing the Media API for HFP use-cases.
Mikel Astiz (4):
audio: remove unnecessary method call
audio: gateway_unlock together with in_use = FALSE
a2dp: return NotSupported error if no server found
audio: do not disconnect on gateway SCO failure
audio/a2dp.c | 2 +-
audio/gateway.c | 2 +-
audio/transport.c | 4 ++--
3 files changed, 4 insertions(+), 4 deletions(-)
--
1.7.6.4
Hi,
On Thu, Dec 15, 2011, Luiz Augusto von Dentz wrote:
> On Thu, Dec 15, 2011 at 12:01 PM, Mikel Astiz <[email protected]> wrote:
> > From: Mikel Astiz <[email protected]>
> >
> > Calling gateway_suspend_stream should not be necessary given that
> > gateway_unlock with rw flags already does it.
> > ---
> > ?audio/transport.c | ? ?1 -
> > ?1 files changed, 0 insertions(+), 1 deletions(-)
> >
> > diff --git a/audio/transport.c b/audio/transport.c
> > index 7bde32d..6029528 100644
> > --- a/audio/transport.c
> > +++ b/audio/transport.c
> > @@ -535,7 +535,6 @@ static guint suspend_gateway(struct media_transport *transport,
> > ? ? ? ? ? ? ? ?return 0;
> > ? ? ? ?}
> >
> > - ? ? ? gateway_suspend_stream(device);
> > ? ? ? ?gateway_unlock(device, GATEWAY_LOCK_READ | GATEWAY_LOCK_WRITE);
> > ? ? ? ?g_idle_add(gateway_suspend_complete, owner);
> > ? ? ? ?return id++;
> > --
>
> gateway_suspend_stream as it is useless, but I think we should fix it
> to work similarly to headset_suspend_stream so we properly wait until
> the socket is closed before we unlock and reply, this is necessary in
> order to properly synchronize switching profiles otherwise
> gateway_suspend_complete maybe called without SCO being completely
> disconnected. Also when a fd is passed over to another process calling
> close/g_io_channel_shutdown is not enough since this only release our
> reference, in this case shutdown must be called.
The subsequent patches in the set fail to apply without this first one,
so the whole patch set is on hold until we get an update for it.
Johan
Hi Mikel,
On Thu, Dec 15, 2011 at 12:01 PM, Mikel Astiz <[email protected]> wrote:
> From: Mikel Astiz <[email protected]>
>
> Failure on BlueZ-initiated SCO requests should not drop the RFCOMM
> connection to the gateway. Instead, considering the stream as suspended
> should be enough.
> ---
> ?audio/gateway.c | ? ?2 +-
> ?1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/audio/gateway.c b/audio/gateway.c
> index 9b1aab3..f28b14d 100644
> --- a/audio/gateway.c
> +++ b/audio/gateway.c
> @@ -255,7 +255,7 @@ static void sco_connect_cb(GIOChannel *chan, GError *err, gpointer user_data)
>
> ? ? ? ?if (err) {
> ? ? ? ? ? ? ? ?error("sco_connect_cb(): %s", err->message);
> - ? ? ? ? ? ? ? gateway_close(dev);
> + ? ? ? ? ? ? ? gateway_suspend_stream(dev);
> ? ? ? ? ? ? ? ?return;
> ? ? ? ?}
>
> --
> 1.7.6.4
>
Ack, this should be up to the endpoint to decided since it may want to retry.
--
Luiz Augusto von Dentz
Hi Mikel,
On Thu, Dec 15, 2011 at 12:01 PM, Mikel Astiz <[email protected]> wrote:
> From: Mikel Astiz <[email protected]>
>
> With both sink and sources disabled, the A2DP server is not even
> registered in audio_manager_init. When trying to register the endpoint,
> this should result in the same error as if the server existed but the
> profile was disabled.
> ---
> ?audio/a2dp.c | ? ?2 +-
> ?1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/audio/a2dp.c b/audio/a2dp.c
> index 5ca105c..9ed2ebb 100644
> --- a/audio/a2dp.c
> +++ b/audio/a2dp.c
> @@ -1590,7 +1590,7 @@ struct a2dp_sep *a2dp_add_sep(const bdaddr_t *src, uint8_t type,
> ? ? ? ?server = find_server(servers, src);
> ? ? ? ?if (server == NULL) {
> ? ? ? ? ? ? ? ?if (err)
> - ? ? ? ? ? ? ? ? ? ? ? *err = -EINVAL;
> + ? ? ? ? ? ? ? ? ? ? ? *err = -EPROTONOSUPPORT;
> ? ? ? ? ? ? ? ?return NULL;
> ? ? ? ?}
>
> --
> 1.7.6.4
Ack.
--
Luiz Augusto von Dentz
Hi Mikel,
On Thu, Dec 15, 2011 at 12:01 PM, Mikel Astiz <[email protected]> wrote:
> From: Mikel Astiz <[email protected]>
>
> Calling gateway_unlock seems safer in gateway_suspend_complete, in
> combination with the update of transport->in_use. This avoids the
> transitional state of having the gateway unlocked but the transport
> still in use.
>
> This approach is also more consistent with the way headset and a2dp
> handle it.
> ---
> ?audio/transport.c | ? ?3 ++-
> ?1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/audio/transport.c b/audio/transport.c
> index 6029528..fc7026e 100644
> --- a/audio/transport.c
> +++ b/audio/transport.c
> @@ -510,6 +510,7 @@ static gboolean gateway_suspend_complete(gpointer user_data)
> ?{
> ? ? ? ?struct media_owner *owner = user_data;
> ? ? ? ?struct media_transport *transport = owner->transport;
> + ? ? ? struct audio_device *device = transport->device;
>
> ? ? ? ?/* Release always succeeds */
> ? ? ? ?if (owner->pending) {
> @@ -518,6 +519,7 @@ static gboolean gateway_suspend_complete(gpointer user_data)
> ? ? ? ? ? ? ? ?media_owner_remove(owner);
> ? ? ? ?}
>
> + ? ? ? gateway_unlock(device, GATEWAY_LOCK_READ | GATEWAY_LOCK_WRITE);
> ? ? ? ?transport->in_use = FALSE;
> ? ? ? ?media_transport_remove(transport, owner);
> ? ? ? ?return FALSE;
> @@ -535,7 +537,6 @@ static guint suspend_gateway(struct media_transport *transport,
> ? ? ? ? ? ? ? ?return 0;
> ? ? ? ?}
>
> - ? ? ? gateway_unlock(device, GATEWAY_LOCK_READ | GATEWAY_LOCK_WRITE);
> ? ? ? ?g_idle_add(gateway_suspend_complete, owner);
> ? ? ? ?return id++;
> ?}
> --
> 1.7.6.4
Ack, but see my comments about the first patch.
--
Luiz Augusto von Dentz
Hi Mikel,
On Thu, Dec 15, 2011 at 12:01 PM, Mikel Astiz <[email protected]> wrote:
> From: Mikel Astiz <[email protected]>
>
> Calling gateway_suspend_stream should not be necessary given that
> gateway_unlock with rw flags already does it.
> ---
> ?audio/transport.c | ? ?1 -
> ?1 files changed, 0 insertions(+), 1 deletions(-)
>
> diff --git a/audio/transport.c b/audio/transport.c
> index 7bde32d..6029528 100644
> --- a/audio/transport.c
> +++ b/audio/transport.c
> @@ -535,7 +535,6 @@ static guint suspend_gateway(struct media_transport *transport,
> ? ? ? ? ? ? ? ?return 0;
> ? ? ? ?}
>
> - ? ? ? gateway_suspend_stream(device);
> ? ? ? ?gateway_unlock(device, GATEWAY_LOCK_READ | GATEWAY_LOCK_WRITE);
> ? ? ? ?g_idle_add(gateway_suspend_complete, owner);
> ? ? ? ?return id++;
> --
gateway_suspend_stream as it is useless, but I think we should fix it
to work similarly to headset_suspend_stream so we properly wait until
the socket is closed before we unlock and reply, this is necessary in
order to properly synchronize switching profiles otherwise
gateway_suspend_complete maybe called without SCO being completely
disconnected. Also when a fd is passed over to another process calling
close/g_io_channel_shutdown is not enough since this only release our
reference, in this case shutdown must be called.
--
Luiz Augusto von Dentz
From: Mikel Astiz <[email protected]>
With both sink and sources disabled, the A2DP server is not even
registered in audio_manager_init. When trying to register the endpoint,
this should result in the same error as if the server existed but the
profile was disabled.
---
audio/a2dp.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/audio/a2dp.c b/audio/a2dp.c
index 5ca105c..9ed2ebb 100644
--- a/audio/a2dp.c
+++ b/audio/a2dp.c
@@ -1590,7 +1590,7 @@ struct a2dp_sep *a2dp_add_sep(const bdaddr_t *src, uint8_t type,
server = find_server(servers, src);
if (server == NULL) {
if (err)
- *err = -EINVAL;
+ *err = -EPROTONOSUPPORT;
return NULL;
}
--
1.7.6.4
From: Mikel Astiz <[email protected]>
Calling gateway_unlock seems safer in gateway_suspend_complete, in
combination with the update of transport->in_use. This avoids the
transitional state of having the gateway unlocked but the transport
still in use.
This approach is also more consistent with the way headset and a2dp
handle it.
---
audio/transport.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/audio/transport.c b/audio/transport.c
index 6029528..fc7026e 100644
--- a/audio/transport.c
+++ b/audio/transport.c
@@ -510,6 +510,7 @@ static gboolean gateway_suspend_complete(gpointer user_data)
{
struct media_owner *owner = user_data;
struct media_transport *transport = owner->transport;
+ struct audio_device *device = transport->device;
/* Release always succeeds */
if (owner->pending) {
@@ -518,6 +519,7 @@ static gboolean gateway_suspend_complete(gpointer user_data)
media_owner_remove(owner);
}
+ gateway_unlock(device, GATEWAY_LOCK_READ | GATEWAY_LOCK_WRITE);
transport->in_use = FALSE;
media_transport_remove(transport, owner);
return FALSE;
@@ -535,7 +537,6 @@ static guint suspend_gateway(struct media_transport *transport,
return 0;
}
- gateway_unlock(device, GATEWAY_LOCK_READ | GATEWAY_LOCK_WRITE);
g_idle_add(gateway_suspend_complete, owner);
return id++;
}
--
1.7.6.4
From: Mikel Astiz <[email protected]>
Failure on BlueZ-initiated SCO requests should not drop the RFCOMM
connection to the gateway. Instead, considering the stream as suspended
should be enough.
---
audio/gateway.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/audio/gateway.c b/audio/gateway.c
index 9b1aab3..f28b14d 100644
--- a/audio/gateway.c
+++ b/audio/gateway.c
@@ -255,7 +255,7 @@ static void sco_connect_cb(GIOChannel *chan, GError *err, gpointer user_data)
if (err) {
error("sco_connect_cb(): %s", err->message);
- gateway_close(dev);
+ gateway_suspend_stream(dev);
return;
}
--
1.7.6.4
From: Mikel Astiz <[email protected]>
Calling gateway_suspend_stream should not be necessary given that
gateway_unlock with rw flags already does it.
---
audio/transport.c | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/audio/transport.c b/audio/transport.c
index 7bde32d..6029528 100644
--- a/audio/transport.c
+++ b/audio/transport.c
@@ -535,7 +535,6 @@ static guint suspend_gateway(struct media_transport *transport,
return 0;
}
- gateway_suspend_stream(device);
gateway_unlock(device, GATEWAY_LOCK_READ | GATEWAY_LOCK_WRITE);
g_idle_add(gateway_suspend_complete, owner);
return id++;
--
1.7.6.4