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.

6 files changed, 34 insertions, 14 deletions

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
@@ -110,6 +110,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
 	 * explicitly reopened with "play").
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;