When closing AVDTP we should wait for for CLOSE request to complete
(so stream go to idle state) before disconnecting signalling socket.
In case CLOSE is rejected, we simply abort stream.
---
android/a2dp.c | 4 +++-
android/avdtp.c | 21 +++++++++++++++++----
2 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/android/a2dp.c b/android/a2dp.c
index 8cff535..8d6e7bf 100644
--- a/android/a2dp.c
+++ b/android/a2dp.c
@@ -1166,8 +1166,10 @@ static void sep_close_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
DBG("");
- if (err)
+ if (err) {
+ avdtp_abort(session, stream);
return;
+ }
setup_remove_by_id(endpoint->id);
}
diff --git a/android/avdtp.c b/android/avdtp.c
index e26d6ec..b9d1992 100644
--- a/android/avdtp.c
+++ b/android/avdtp.c
@@ -398,6 +398,8 @@ struct avdtp {
struct pending_req *req;
GSList *disconnect;
+
+ bool shutdown;
};
static GSList *lseps = NULL;
@@ -913,6 +915,11 @@ static void avdtp_sep_set_state(struct avdtp *session,
session->streams = g_slist_remove(session->streams, stream);
stream_free(stream);
}
+
+ if (session->io && session->shutdown && session->streams == NULL) {
+ int sock = g_io_channel_unix_get_fd(session->io);
+ shutdown(sock, SHUT_RDWR);
+ }
}
static void finalize_discovery(struct avdtp *session, int err)
@@ -2141,7 +2148,7 @@ gboolean avdtp_remove_disconnect_cb(struct avdtp *session, unsigned int id)
void avdtp_shutdown(struct avdtp *session)
{
GSList *l;
- int sock;
+ bool closing = false;
if (!session->io)
return;
@@ -2149,12 +2156,18 @@ void avdtp_shutdown(struct avdtp *session)
for (l = session->streams; l; l = g_slist_next(l)) {
struct avdtp_stream *stream = l->data;
- avdtp_close(session, stream, TRUE);
+ if (avdtp_close(session, stream, TRUE) == 0)
+ closing = true;
}
- sock = g_io_channel_unix_get_fd(session->io);
+ if (closing) {
+ /* defer shutdown until all streams closed */
+ session->shutdown = true;
+ } else {
+ int sock = g_io_channel_unix_get_fd(session->io);
- shutdown(sock, SHUT_RDWR);
+ shutdown(sock, SHUT_RDWR);
+ }
}
static void queue_request(struct avdtp *session, struct pending_req *req,
--
1.8.5.3
Hi Andrzej,
On Fri, Feb 7, 2014 at 11:45 AM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Andrzej,
>
> On Thu, Feb 6, 2014 at 7:54 PM, Andrzej Kaczmarek
> <[email protected]> wrote:
>> In case audio IPC is suddenly disconnected (most likely due to crash of
>> mediaserver process) we should disconnect headset since it is no longer
>> associated with valid setup and cannot be used properly.
>> ---
>> android/a2dp.c | 7 +++++++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/android/a2dp.c b/android/a2dp.c
>> index 8eabfeb..f67a593 100644
>> --- a/android/a2dp.c
>> +++ b/android/a2dp.c
>> @@ -1515,6 +1515,7 @@ static gboolean audio_retry_register(void *data)
>>
>> static void audio_disconnected(void *data)
>> {
>> + GSList *l;
>> bool restart;
>>
>> DBG("");
>> @@ -1526,6 +1527,12 @@ static void audio_disconnected(void *data)
>>
>> bt_audio_unregister();
>>
>> + for (l = devices; l; l = g_slist_next(l)) {
>> + struct a2dp_device *dev = l->data;
>> +
>> + avdtp_shutdown(dev->session);
>> + }
>> +
>> if (!restart)
>> return;
>
> If we are unregistering the endpoints properly this should not happen,
> perhaps what is wrong is avdtp_unregister_sep is not aborting existing
> streams properly. Btw we should probably add a unit test if this is
> happening in practice because otherwise the remote won't notice.
I checked the code and actually this should not be a problem since we
drop the stream transport connection it should indicate a unclean
abort as it should be, in the past we had many times this happening
with PA crashing and it seems the headsets react properly to the
stream transport dropping unexpectedly, we can still do abort though.
--
Luiz Augusto von Dentz
Hi Andrzej,
On Thu, Feb 6, 2014 at 7:54 PM, Andrzej Kaczmarek
<[email protected]> wrote:
> In case audio IPC is suddenly disconnected (most likely due to crash of
> mediaserver process) we should disconnect headset since it is no longer
> associated with valid setup and cannot be used properly.
> ---
> android/a2dp.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/android/a2dp.c b/android/a2dp.c
> index 8eabfeb..f67a593 100644
> --- a/android/a2dp.c
> +++ b/android/a2dp.c
> @@ -1515,6 +1515,7 @@ static gboolean audio_retry_register(void *data)
>
> static void audio_disconnected(void *data)
> {
> + GSList *l;
> bool restart;
>
> DBG("");
> @@ -1526,6 +1527,12 @@ static void audio_disconnected(void *data)
>
> bt_audio_unregister();
>
> + for (l = devices; l; l = g_slist_next(l)) {
> + struct a2dp_device *dev = l->data;
> +
> + avdtp_shutdown(dev->session);
> + }
> +
> if (!restart)
> return;
If we are unregistering the endpoints properly this should not happen,
perhaps what is wrong is avdtp_unregister_sep is not aborting existing
streams properly. Btw we should probably add a unit test if this is
happening in practice because otherwise the remote won't notice.
--
Luiz Augusto von Dentz
Hi Andrzej,
On Thu, Feb 6, 2014 at 7:54 PM, Andrzej Kaczmarek
<[email protected]> wrote:
> When closing AVDTP we should wait for for CLOSE request to complete
> (so stream go to idle state) before disconnecting signalling socket.
> In case CLOSE is rejected, we simply abort stream.
> ---
> android/a2dp.c | 4 +++-
> android/avdtp.c | 21 +++++++++++++++++----
> 2 files changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/android/a2dp.c b/android/a2dp.c
> index 8cff535..8d6e7bf 100644
> --- a/android/a2dp.c
> +++ b/android/a2dp.c
> @@ -1166,8 +1166,10 @@ static void sep_close_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
>
> DBG("");
>
> - if (err)
> + if (err) {
> + avdtp_abort(session, stream);
> return;
> + }
>
> setup_remove_by_id(endpoint->id);
> }
> diff --git a/android/avdtp.c b/android/avdtp.c
> index e26d6ec..b9d1992 100644
> --- a/android/avdtp.c
> +++ b/android/avdtp.c
> @@ -398,6 +398,8 @@ struct avdtp {
> struct pending_req *req;
>
> GSList *disconnect;
> +
> + bool shutdown;
> };
>
> static GSList *lseps = NULL;
> @@ -913,6 +915,11 @@ static void avdtp_sep_set_state(struct avdtp *session,
> session->streams = g_slist_remove(session->streams, stream);
> stream_free(stream);
> }
> +
> + if (session->io && session->shutdown && session->streams == NULL) {
> + int sock = g_io_channel_unix_get_fd(session->io);
> + shutdown(sock, SHUT_RDWR);
> + }
> }
>
> static void finalize_discovery(struct avdtp *session, int err)
> @@ -2141,7 +2148,7 @@ gboolean avdtp_remove_disconnect_cb(struct avdtp *session, unsigned int id)
> void avdtp_shutdown(struct avdtp *session)
> {
> GSList *l;
> - int sock;
> + bool closing = false;
>
> if (!session->io)
> return;
> @@ -2149,12 +2156,18 @@ void avdtp_shutdown(struct avdtp *session)
> for (l = session->streams; l; l = g_slist_next(l)) {
> struct avdtp_stream *stream = l->data;
>
> - avdtp_close(session, stream, TRUE);
> + if (avdtp_close(session, stream, TRUE) == 0)
> + closing = true;
You could assign true directly to session->shutdown and return, also
it is probably a good idea to check if the flag is already set and if
it does call avdtp_abort, in fact I think we should call avdtp_abort
not avdtp_close anyway since we are shutting it down there is no point
of given the remote even a chance to reject.
--
Luiz Augusto von Dentz
---
android/a2dp.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/android/a2dp.c b/android/a2dp.c
index 8d6e7bf..8eabfeb 100644
--- a/android/a2dp.c
+++ b/android/a2dp.c
@@ -990,6 +990,8 @@ static gboolean sep_close_ind(struct avdtp *session,
return FALSE;
}
+ bt_audio_notify_state(setup, HAL_AUDIO_STOPPED);
+
setup_remove(setup);
return TRUE;
@@ -1163,6 +1165,7 @@ static void sep_close_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
void *user_data)
{
struct a2dp_endpoint *endpoint = user_data;
+ struct a2dp_setup *setup;
DBG("");
@@ -1171,7 +1174,16 @@ static void sep_close_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
return;
}
- setup_remove_by_id(endpoint->id);
+ setup = find_setup(endpoint->id);
+ if (!setup) {
+ error("Unable to find stream setup for %u endpoint",
+ endpoint->id);
+ return;
+ }
+
+ bt_audio_notify_state(setup, HAL_AUDIO_STOPPED);
+
+ setup_remove(setup);
}
static void sep_abort_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
--
1.8.5.3
In case audio IPC is suddenly disconnected (most likely due to crash of
mediaserver process) we should disconnect headset since it is no longer
associated with valid setup and cannot be used properly.
---
android/a2dp.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/android/a2dp.c b/android/a2dp.c
index 8eabfeb..f67a593 100644
--- a/android/a2dp.c
+++ b/android/a2dp.c
@@ -1515,6 +1515,7 @@ static gboolean audio_retry_register(void *data)
static void audio_disconnected(void *data)
{
+ GSList *l;
bool restart;
DBG("");
@@ -1526,6 +1527,12 @@ static void audio_disconnected(void *data)
bt_audio_unregister();
+ for (l = devices; l; l = g_slist_next(l)) {
+ struct a2dp_device *dev = l->data;
+
+ avdtp_shutdown(dev->session);
+ }
+
if (!restart)
return;
--
1.8.5.3
Unregistering a SEP can trigger abort_cfm callback if some device is
connected thus we should free setups list before all endpoints are
unregistered to avoid error in abort_cfm due to non-existing setup.
---
android/a2dp.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/android/a2dp.c b/android/a2dp.c
index f67a593..7a2f3cf 100644
--- a/android/a2dp.c
+++ b/android/a2dp.c
@@ -1483,12 +1483,12 @@ static void bt_audio_unregister(void)
if (audio_retry_id > 0)
g_source_remove(audio_retry_id);
- g_slist_free_full(setups, setup_free);
- setups = NULL;
-
g_slist_free_full(endpoints, unregister_endpoint);
endpoints = NULL;
+ g_slist_free_full(setups, setup_free);
+ setups = NULL;
+
audio_ipc_cleanup();
}
--
1.8.5.3
---
android/hal-audio.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 81 insertions(+)
diff --git a/android/hal-audio.c b/android/hal-audio.c
index 766327b..9312659 100644
--- a/android/hal-audio.c
+++ b/android/hal-audio.c
@@ -289,6 +289,78 @@ static int sbc_get_presets(struct audio_preset *preset, size_t *len)
return i;
}
+static int sbc_freq2int(uint8_t freq)
+{
+ switch (freq) {
+ case SBC_SAMPLING_FREQ_16000:
+ return 16000;
+ case SBC_SAMPLING_FREQ_32000:
+ return 32000;
+ case SBC_SAMPLING_FREQ_44100:
+ return 44100;
+ case SBC_SAMPLING_FREQ_48000:
+ return 48000;
+ default:
+ return 0;
+ }
+}
+
+static const char *sbc_mode2str(uint8_t mode)
+{
+ switch (mode) {
+ case SBC_CHANNEL_MODE_MONO:
+ return "Mono";
+ case SBC_CHANNEL_MODE_DUAL_CHANNEL:
+ return "DualChannel";
+ case SBC_CHANNEL_MODE_STEREO:
+ return "Stereo";
+ case SBC_CHANNEL_MODE_JOINT_STEREO:
+ return "JointStereo";
+ default:
+ return "(unknown)";
+ }
+}
+
+static int sbc_blocks2int(uint8_t blocks)
+{
+ switch (blocks) {
+ case SBC_BLOCK_LENGTH_4:
+ return 4;
+ case SBC_BLOCK_LENGTH_8:
+ return 8;
+ case SBC_BLOCK_LENGTH_12:
+ return 12;
+ case SBC_BLOCK_LENGTH_16:
+ return 16;
+ default:
+ return 0;
+ }
+}
+
+static int sbc_subbands2int(uint8_t subbands)
+{
+ switch (subbands) {
+ case SBC_SUBBANDS_4:
+ return 4;
+ case SBC_SUBBANDS_8:
+ return 8;
+ default:
+ return 0;
+ }
+}
+
+static const char *sbc_allocation2str(uint8_t allocation)
+{
+ switch (allocation) {
+ case SBC_ALLOCATION_SNR:
+ return "SNR";
+ case SBC_ALLOCATION_LOUDNESS:
+ return "Loudness";
+ default:
+ return "(unknown)";
+ }
+}
+
static void sbc_init_encoder(struct sbc_data *sbc_data)
{
a2dp_sbc_t *in = &sbc_data->sbc;
@@ -298,6 +370,15 @@ static void sbc_init_encoder(struct sbc_data *sbc_data)
out->endian = SBC_LE;
out->bitpool = in->max_bitpool;
+
+ DBG("frequency=%d channel_mode=%s block_length=%d subbands=%d "
+ "allocation=%s bitpool=%d-%d",
+ sbc_freq2int(in->frequency),
+ sbc_mode2str(in->channel_mode),
+ sbc_blocks2int(in->block_length),
+ sbc_subbands2int(in->subbands),
+ sbc_allocation2str(in->allocation_method),
+ in->min_bitpool, in->max_bitpool);
}
static int sbc_codec_init(struct audio_preset *preset, uint16_t mtu,
--
1.8.5.3
We should not try to neither auto-resume nor write when state is set to
NONE as this is case when we're being closed and it's ok do ignore
write request.
---
android/hal-audio.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/android/hal-audio.c b/android/hal-audio.c
index efdf823..766327b 100644
--- a/android/hal-audio.c
+++ b/android/hal-audio.c
@@ -831,6 +831,10 @@ static ssize_t out_write(struct audio_stream_out *stream, const void *buffer,
{
struct a2dp_stream_out *out = (struct a2dp_stream_out *) stream;
+ /* just return in case we're closing */
+ if (out->audio_state == AUDIO_A2DP_STATE_NONE)
+ return -1;
+
/* We can auto-start only from standby */
if (out->audio_state == AUDIO_A2DP_STATE_STANDBY) {
DBG("stream in standby, auto-start");
--
1.8.5.3