From 8b0b4ff0860ea93850c2f44e72e8a8a5de05e13b Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Thu, 1 Sep 2011 07:13:21 +0200 Subject: [PATCH] output_thread: reimplement CANCEL synchronization The output thread could hang indefinitely after finishing CANCEL, because it could have missed the signal while the output was not unlocked in ao_command_finished(). This patch removes the wait() call after CANCEL, and adds the flag "allow_play" instead. While this flag is set, playback is skipped. With this flag, there will not be any excess wait() call after the pipe has been cleared. This patch fixes a bug that causes mpd to discontinue playback after seeking, due to the race condition described above. --- NEWS | 1 + src/output_all.c | 7 +++++-- src/output_control.c | 18 +++++++++++++----- src/output_control.h | 5 +++++ src/output_init.c | 1 + src/output_internal.h | 9 +++++++++ src/output_thread.c | 8 +------- 7 files changed, 35 insertions(+), 14 deletions(-) diff --git a/NEWS b/NEWS index 0bf08d0ab..56a846582 100644 --- a/NEWS +++ b/NEWS @@ -2,6 +2,7 @@ ver 0.16.4 (2011/??/??) * fix memory leaks * don't resume playback when seeking to another song while paused * apply follow_inside_symlinks to absolute symlinks +* fix playback discontinuation after seeking * input: - curl: limit the receive buffer size - curl: implement a hard-coded timeout of 10 seconds diff --git a/src/output_all.c b/src/output_all.c index 19c0f0166..551736a41 100644 --- a/src/output_all.c +++ b/src/output_all.c @@ -206,15 +206,18 @@ static void audio_output_wait_all(void) } /** - * Signals the audio output if it is open. This function locks the - * mutex. + * Signal the audio output if it is open, and set the "allow_play" + * flag. This function locks the mutex. */ static void audio_output_lock_signal(struct audio_output *ao) { g_mutex_lock(ao->mutex); + + ao->allow_play = true; if (audio_output_is_open(ao)) g_cond_signal(ao->cond); + g_mutex_unlock(ao->mutex); } diff --git a/src/output_control.c b/src/output_control.c index 0823b667b..14976dbfb 100644 --- a/src/output_control.c +++ b/src/output_control.c @@ -115,6 +115,8 @@ audio_output_open(struct audio_output *ao, { bool open; + assert(ao != NULL); + assert(ao->allow_play); assert(audio_format_valid(audio_format)); assert(mp != NULL); @@ -140,10 +142,6 @@ audio_output_open(struct audio_output *ao, /* we're not using audio_output_cancel() here, because that function is asynchronous */ ao_command(ao, AO_COMMAND_CANCEL); - - /* the audio output is now waiting for a - signal; wake it up immediately */ - g_cond_signal(ao->cond); } return true; @@ -181,6 +179,7 @@ static void audio_output_close_locked(struct audio_output *ao) { assert(ao != NULL); + assert(ao->allow_play); if (ao->mixer != NULL) mixer_auto_close(ao->mixer); @@ -223,6 +222,8 @@ audio_output_play(struct audio_output *ao) { g_mutex_lock(ao->mutex); + assert(ao->allow_play); + if (audio_output_is_open(ao)) g_cond_signal(ao->cond); @@ -238,6 +239,7 @@ void audio_output_pause(struct audio_output *ao) mixer_auto_close(ao->mixer); g_mutex_lock(ao->mutex); + assert(ao->allow_play); if (audio_output_is_open(ao)) ao_command_async(ao, AO_COMMAND_PAUSE); g_mutex_unlock(ao->mutex); @@ -247,6 +249,7 @@ void audio_output_drain_async(struct audio_output *ao) { g_mutex_lock(ao->mutex); + assert(ao->allow_play); if (audio_output_is_open(ao)) ao_command_async(ao, AO_COMMAND_DRAIN); g_mutex_unlock(ao->mutex); @@ -255,8 +258,12 @@ audio_output_drain_async(struct audio_output *ao) void audio_output_cancel(struct audio_output *ao) { g_mutex_lock(ao->mutex); - if (audio_output_is_open(ao)) + + if (audio_output_is_open(ao)) { + ao->allow_play = false; ao_command_async(ao, AO_COMMAND_CANCEL); + } + g_mutex_unlock(ao->mutex); } @@ -287,6 +294,7 @@ void audio_output_finish(struct audio_output *ao) if (ao->thread != NULL) { g_mutex_lock(ao->mutex); + assert(ao->allow_play); ao_command(ao, AO_COMMAND_KILL); g_mutex_unlock(ao->mutex); g_thread_join(ao->thread); diff --git a/src/output_control.h b/src/output_control.h index 7f4f4a53c..2b88d4103 100644 --- a/src/output_control.h +++ b/src/output_control.h @@ -70,6 +70,11 @@ void audio_output_pause(struct audio_output *ao); void audio_output_drain_async(struct audio_output *ao); +/** + * Clear the "allow_play" flag and send the "CANCEL" command + * asynchronously. To finish the operation, the caller has to set the + * "allow_play" flag and signal the thread. + */ void audio_output_cancel(struct audio_output *ao); void audio_output_close(struct audio_output *ao); diff --git a/src/output_init.c b/src/output_init.c index f4700dfb2..96f87f512 100644 --- a/src/output_init.c +++ b/src/output_init.c @@ -189,6 +189,7 @@ audio_output_init(struct audio_output *ao, const struct config_param *param, ao->really_enabled = false; ao->open = false; ao->pause = false; + ao->allow_play = true; ao->fail_timer = NULL; pcm_buffer_init(&ao->cross_fade_buffer); diff --git a/src/output_internal.h b/src/output_internal.h index 18d431352..7102ea5cd 100644 --- a/src/output_internal.h +++ b/src/output_internal.h @@ -109,6 +109,15 @@ struct audio_output { */ bool pause; + /** + * When this flag is set, the output thread will not do any + * playback. It will wait until the flag is cleared. + * + * This is used to synchronize the "clear" operation on the + * shared music pipe during the CANCEL command. + */ + bool allow_play; + /** * If not NULL, the device has failed, and this timer is used * to estimate how long it should stay disabled (unless diff --git a/src/output_thread.c b/src/output_thread.c index 2c2b8d116..bf56ca971 100644 --- a/src/output_thread.c +++ b/src/output_thread.c @@ -648,12 +648,6 @@ static gpointer audio_output_task(gpointer arg) } ao_command_finished(ao); - - /* the player thread will now clear our music - pipe - wait for a notify, to give it some - time */ - if (ao->command == AO_COMMAND_NONE) - g_cond_wait(ao->cond, ao->mutex); continue; case AO_COMMAND_KILL: @@ -663,7 +657,7 @@ static gpointer audio_output_task(gpointer arg) return NULL; } - if (ao->open && ao_play(ao)) + if (ao->open && ao->allow_play && ao_play(ao)) /* don't wait for an event if there are more chunks in the pipe */ continue;