output: consistently lock audio output objects

Always keep the audio_output object locked within the output thread,
unless a plugin method is called.  This fixes several race conditions.

7 files changed, 103 insertions, 50 deletions

diff --git a/NEWS b/NEWS
index 9fe1dc18..082e456d 100644
--- a/NEWS
+++ b/NEWS
@@ -36,6 +36,7 @@ ver 0.16 (20??/??/??)
   - jack: connect to server on MPD startup
   - wildcards allowed in audio_format configuration
   - alsa: don't recover on CANCEL
+  - consistently lock audio output objects
 * mixers:
   - removed support for legacy mixer configuration
   - reimplemented software volume as mixer+filter plugin
diff --git a/src/output_all.c b/src/output_all.c
index a16be738..05cd1d35 100644
--- a/src/output_all.c
+++ b/src/output_all.c
@@ -180,10 +180,18 @@ audio_output_all_enable_disable(void)
 static bool
 audio_output_all_finished(void)
 {
-	for (unsigned i = 0; i < num_audio_outputs; ++i)
-		if (audio_output_is_open(&audio_outputs[i]) &&
-		    !audio_output_command_is_finished(&audio_outputs[i]))
+	for (unsigned i = 0; i < num_audio_outputs; ++i) {
+		struct audio_output *ao = &audio_outputs[i];
+		bool not_finished;
+
+		g_mutex_lock(ao->mutex);
+		not_finished = audio_output_is_open(ao) &&
+			!audio_output_command_is_finished(ao);
+		g_mutex_unlock(ao->mutex);
+
+		if (not_finished)
 			return false;
+	}
 
 	return true;
 }
@@ -261,8 +269,7 @@ audio_output_all_play(struct music_chunk *chunk)
 	music_pipe_push(g_mp, chunk);
 
 	for (i = 0; i < num_audio_outputs; ++i)
-		if (audio_output_is_open(&audio_outputs[i]))
-			audio_output_play(&audio_outputs[i]);
+		audio_output_play(&audio_outputs[i]);
 
 	return true;
 }
@@ -454,8 +461,7 @@ audio_output_all_pause(void)
 	audio_output_all_update();
 
 	for (i = 0; i < num_audio_outputs; ++i)
-		if (audio_output_is_open(&audio_outputs[i]))
-			audio_output_pause(&audio_outputs[i]);
+		audio_output_pause(&audio_outputs[i]);
 
 	audio_output_wait_all();
 }
@@ -467,10 +473,8 @@ audio_output_all_cancel(void)
 
 	/* send the cancel() command to all audio outputs */
 
-	for (i = 0; i < num_audio_outputs; ++i) {
-		if (audio_output_is_open(&audio_outputs[i]))
-			audio_output_cancel(&audio_outputs[i]);
-	}
+	for (i = 0; i < num_audio_outputs; ++i)
+		audio_output_cancel(&audio_outputs[i]);
 
 	audio_output_wait_all();
 
diff --git a/src/output_control.c b/src/output_control.c
index 6512cbe7..46d8f869 100644
--- a/src/output_control.c
+++ b/src/output_control.c
@@ -24,6 +24,7 @@
 #include "mixer_control.h"
 #include "mixer_plugin.h"
 #include "filter_plugin.h"
+#include "notify.h"
 
 #include <assert.h>
 #include <stdlib.h>
@@ -39,8 +40,10 @@ struct notify audio_output_client_notify;
 static void ao_command_wait(struct audio_output *ao)
 {
 	while (ao->command != AO_COMMAND_NONE) {
-		notify_signal(&ao->notify);
+		g_cond_signal(ao->cond);
+		g_mutex_unlock(ao->mutex);
 		notify_wait(&audio_output_client_notify);
+		g_mutex_lock(ao->mutex);
 	}
 }
 
@@ -56,7 +59,7 @@ static void ao_command_async(struct audio_output *ao,
 {
 	assert(ao->command == AO_COMMAND_NONE);
 	ao->command = cmd;
-	notify_signal(&ao->notify);
+	g_cond_signal(ao->cond);
 }
 
 void
@@ -74,7 +77,9 @@ audio_output_enable(struct audio_output *ao)
 		audio_output_thread_start(ao);
 	}
 
+	g_mutex_lock(ao->mutex);
 	ao_command(ao, AO_COMMAND_ENABLE);
+	g_mutex_unlock(ao->mutex);
 }
 
 void
@@ -91,7 +96,9 @@ audio_output_disable(struct audio_output *ao)
 		return;
 	}
 
+	g_mutex_lock(ao->mutex);
 	ao_command(ao, AO_COMMAND_DISABLE);
+	g_mutex_unlock(ao->mutex);
 }
 
 static bool
@@ -157,23 +164,31 @@ audio_output_update(struct audio_output *ao,
 {
 	assert(mp != NULL);
 
+	g_mutex_lock(ao->mutex);
+
 	if (ao->enabled && ao->really_enabled) {
 		if (ao->fail_timer == NULL ||
-		    g_timer_elapsed(ao->fail_timer, NULL) > REOPEN_AFTER)
-			return audio_output_open(ao, audio_format, mp);
+		    g_timer_elapsed(ao->fail_timer, NULL) > REOPEN_AFTER) {
+			bool success = audio_output_open(ao, audio_format, mp);
+			g_mutex_unlock(ao->mutex);
+			return success;
+		}
 	} else if (audio_output_is_open(ao))
 		audio_output_close(ao);
 
+	g_mutex_unlock(ao->mutex);
 	return false;
 }
 
 void
 audio_output_play(struct audio_output *ao)
 {
-	if (!ao->open)
-		return;
+	g_mutex_lock(ao->mutex);
 
-	notify_signal(&ao->notify);
+	if (audio_output_is_open(ao))
+		g_cond_signal(ao->cond);
+
+	g_mutex_unlock(ao->mutex);
 }
 
 void audio_output_pause(struct audio_output *ao)
@@ -184,27 +199,37 @@ void audio_output_pause(struct audio_output *ao)
 		   mixer_auto_close()) */
 		mixer_auto_close(ao->mixer);
 
-	ao_command_async(ao, AO_COMMAND_PAUSE);
+	g_mutex_lock(ao->mutex);
+	if (audio_output_is_open(ao))
+		ao_command_async(ao, AO_COMMAND_PAUSE);
+	g_mutex_unlock(ao->mutex);
 }
 
 void audio_output_cancel(struct audio_output *ao)
 {
-	ao_command_async(ao, AO_COMMAND_CANCEL);
+	g_mutex_lock(ao->mutex);
+	if (audio_output_is_open(ao))
+		ao_command_async(ao, AO_COMMAND_CANCEL);
+	g_mutex_unlock(ao->mutex);
 }
 
 void audio_output_close(struct audio_output *ao)
 {
-	assert(!ao->open || ao->fail_timer == NULL);
-
 	if (ao->mixer != NULL)
 		mixer_auto_close(ao->mixer);
 
+	g_mutex_lock(ao->mutex);
+
+	assert(!ao->open || ao->fail_timer == NULL);
+
 	if (ao->open)
 		ao_command(ao, AO_COMMAND_CLOSE);
 	else if (ao->fail_timer != NULL) {
 		g_timer_destroy(ao->fail_timer);
 		ao->fail_timer = NULL;
 	}
+
+	g_mutex_unlock(ao->mutex);
 }
 
 void audio_output_finish(struct audio_output *ao)
@@ -214,7 +239,9 @@ void audio_output_finish(struct audio_output *ao)
 	assert(ao->fail_timer == NULL);
 
 	if (ao->thread != NULL) {
+		g_mutex_lock(ao->mutex);
 		ao_command(ao, AO_COMMAND_KILL);
+		g_mutex_unlock(ao->mutex);
 		g_thread_join(ao->thread);
 	}
 
@@ -223,7 +250,7 @@ void audio_output_finish(struct audio_output *ao)
 
 	ao_plugin_finish(ao->plugin, ao->data);
 
-	notify_deinit(&ao->notify);
+	g_cond_free(ao->cond);
 	g_mutex_free(ao->mutex);
 
 	filter_free(ao->filter);
diff --git a/src/output_init.c b/src/output_init.c
index 5cb9ac92..a7272bfc 100644
--- a/src/output_init.c
+++ b/src/output_init.c
@@ -191,9 +191,9 @@ audio_output_init(struct audio_output *ao, const struct config_param *param,
 	assert(ao->filter != NULL);
 
 	ao->thread = NULL;
-	notify_init(&ao->notify);
 	ao->command = AO_COMMAND_NONE;
 	ao->mutex = g_mutex_new();
+	ao->cond = g_cond_new();
 
 	ao->data = ao_plugin_init(plugin,
 				  &ao->config_audio_format,
diff --git a/src/output_internal.h b/src/output_internal.h
index 0c25348a..6b81bbc7 100644
--- a/src/output_internal.h
+++ b/src/output_internal.h
@@ -21,7 +21,8 @@
 #define MPD_OUTPUT_INTERNAL_H
 
 #include "audio_format.h"
-#include "notify.h"
+
+#include <glib.h>
 
 #include <time.h>
 
@@ -141,11 +142,6 @@ struct audio_output {
 	GThread *thread;
 
 	/**
-	 * Notify object for the thread.
-	 */
-	struct notify notify;
-
-	/**
 	 * The next command to be performed by the output thread.
 	 */
 	enum audio_output_command command;
@@ -161,6 +157,12 @@ struct audio_output {
 	GMutex *mutex;
 
 	/**
+	 * This condition object wakes up the output thread after
+	 * #command has been set.
+	 */
+	GCond *cond;
+
+	/**
 	 * The #music_chunk which is currently being played.  All
 	 * chunks before this one may be returned to the
 	 * #music_buffer, because they are not going to be used by
diff --git a/src/output_thread.c b/src/output_thread.c
index 9d25d475..af908cac 100644
--- a/src/output_thread.c
+++ b/src/output_thread.c
@@ -39,18 +39,25 @@ static void ao_command_finished(struct audio_output *ao)
 {
 	assert(ao->command != AO_COMMAND_NONE);
 	ao->command = AO_COMMAND_NONE;
+
+	g_mutex_unlock(ao->mutex);
 	notify_signal(&audio_output_client_notify);
+	g_mutex_lock(ao->mutex);
 }
 
 static bool
 ao_enable(struct audio_output *ao)
 {
 	GError *error = NULL;
+	bool success;
 
 	if (ao->really_enabled)
 		return true;
 
-	if (!ao_plugin_enable(ao->plugin, ao->data, &error)) {
+	g_mutex_unlock(ao->mutex);
+	success = ao_plugin_enable(ao->plugin, ao->data, &error);
+	g_mutex_lock(ao->mutex);
+	if (!success) {
 		g_warning("Failed to enable \"%s\" [%s]: %s\n",
 			  ao->name, ao->plugin->name, error->message);
 		g_error_free(error);
@@ -72,7 +79,10 @@ ao_disable(struct audio_output *ao)
 
 	if (ao->really_enabled) {
 		ao->really_enabled = false;
+
+		g_mutex_unlock(ao->mutex);
 		ao_plugin_disable(ao->plugin, ao->data);
+		g_mutex_lock(ao->mutex);
 	}
 }
 
@@ -111,9 +121,11 @@ ao_open(struct audio_output *ao)
 	audio_format_mask_apply(&ao->out_audio_format,
 				&ao->config_audio_format);
 
+	g_mutex_unlock(ao->mutex);
 	success = ao_plugin_open(ao->plugin, ao->data,
 				 &ao->out_audio_format,
 				 &error);
+	g_mutex_lock(ao->mutex);
 
 	assert(!ao->open);
 
@@ -129,9 +141,7 @@ ao_open(struct audio_output *ao)
 
 	convert_filter_set(ao->convert_filter, &ao->out_audio_format);
 
-	g_mutex_lock(ao->mutex);
 	ao->open = true;
-	g_mutex_unlock(ao->mutex);
 
 	g_debug("opened plugin=%s name=\"%s\" "
 		"audio_format=%u:%u:%u:%u",
@@ -157,9 +167,9 @@ ao_close(struct audio_output *ao, bool drain)
 
 	ao->pipe = NULL;
 
-	g_mutex_lock(ao->mutex);
 	ao->chunk = NULL;
 	ao->open = false;
+
 	g_mutex_unlock(ao->mutex);
 
 	if (drain)
@@ -170,6 +180,8 @@ ao_close(struct audio_output *ao, bool drain)
 	ao_plugin_close(ao->plugin, ao->data);
 	filter_close(ao->filter);
 
+	g_mutex_lock(ao->mutex);
+
 	g_debug("closed plugin=%s name=\"%s\"", ao->plugin->name, ao->name);
 }
 
@@ -193,14 +205,14 @@ ao_reopen_filter(struct audio_output *ao)
 
 		ao->pipe = NULL;
 
-		g_mutex_lock(ao->mutex);
 		ao->chunk = NULL;
 		ao->open = false;
-		g_mutex_unlock(ao->mutex);
+		ao->fail_timer = g_timer_new();
 
+		g_mutex_unlock(ao->mutex);
 		ao_plugin_close(ao->plugin, ao->data);
+		g_mutex_lock(ao->mutex);
 
-		ao->fail_timer = g_timer_new();
 		return;
 	}
 
@@ -246,8 +258,11 @@ ao_play_chunk(struct audio_output *ao, const struct music_chunk *chunk)
 	assert(music_chunk_check_format(chunk, &ao->in_audio_format));
 	assert(size % audio_format_frame_size(&ao->in_audio_format) == 0);
 
-	if (chunk->tag != NULL)
+	if (chunk->tag != NULL) {
+		g_mutex_unlock(ao->mutex);
 		ao_plugin_send_tag(ao->plugin, ao->data, chunk->tag);
+		g_mutex_lock(ao->mutex);
+	}
 
 	if (size == 0)
 		return true;
@@ -269,8 +284,10 @@ ao_play_chunk(struct audio_output *ao, const struct music_chunk *chunk)
 	while (size > 0 && ao->command == AO_COMMAND_NONE) {
 		size_t nbytes;
 
+		g_mutex_unlock(ao->mutex);
 		nbytes = ao_plugin_play(ao->plugin, ao->data, data, size,
 					&error);
+		g_mutex_lock(ao->mutex);
 		if (nbytes == 0) {
 			/* play()==0 means failure */
 			g_warning("\"%s\" [%s] failed to play: %s",
@@ -302,7 +319,6 @@ static void ao_play(struct audio_output *ao)
 
 	assert(ao->pipe != NULL);
 
-	g_mutex_lock(ao->mutex);
 	chunk = ao->chunk;
 	if (chunk != NULL)
 		/* continue the previous play() call */
@@ -315,12 +331,8 @@ static void ao_play(struct audio_output *ao)
 		assert(!ao->chunk_finished);
 
 		ao->chunk = chunk;
-		g_mutex_unlock(ao->mutex);
 
 		success = ao_play_chunk(ao, chunk);
-
-		g_mutex_lock(ao->mutex);
-
 		if (!success) {
 			assert(ao->chunk == NULL);
 			break;
@@ -331,21 +343,28 @@ static void ao_play(struct audio_output *ao)
 	}
 
 	ao->chunk_finished = true;
-	g_mutex_unlock(ao->mutex);
 
+	g_mutex_unlock(ao->mutex);
 	notify_signal(&pc.notify);
+	g_mutex_lock(ao->mutex);
 }
 
 static void ao_pause(struct audio_output *ao)
 {
 	bool ret;
 
+	g_mutex_unlock(ao->mutex);
 	ao_plugin_cancel(ao->plugin, ao->data);
+	g_mutex_lock(ao->mutex);
+
 	ao->pause = true;
 	ao_command_finished(ao);
 
 	do {
+		g_mutex_unlock(ao->mutex);
 		ret = ao_plugin_pause(ao->plugin, ao->data);
+		g_mutex_lock(ao->mutex);
+
 		if (!ret) {
 			ao_close(ao, false);
 			break;
@@ -359,6 +378,8 @@ static gpointer audio_output_task(gpointer arg)
 {
 	struct audio_output *ao = arg;
 
+	g_mutex_lock(ao->mutex);
+
 	while (1) {
 		switch (ao->command) {
 		case AO_COMMAND_NONE:
@@ -409,19 +430,21 @@ static gpointer audio_output_task(gpointer arg)
 			/* the player thread will now clear our music
 			   pipe - wait for a notify, to give it some
 			   time */
-			notify_wait(&ao->notify);
+			g_cond_wait(ao->cond, ao->mutex);
 			continue;
 
 		case AO_COMMAND_KILL:
 			ao->chunk = NULL;
 			ao_command_finished(ao);
+			g_mutex_unlock(ao->mutex);
 			return NULL;
 		}
 
 		if (ao->open)
 			ao_play(ao);
 
-		notify_wait(&ao->notify);
+		if (ao->command == AO_COMMAND_NONE)
+			g_cond_wait(ao->cond, ao->mutex);
 	}
 }
 
diff --git a/test/run_output.c b/test/run_output.c
index 238e16e5..0e91bb3f 100644
--- a/test/run_output.c
+++ b/test/run_output.c
@@ -58,10 +58,6 @@ pcm_convert(G_GNUC_UNUSED struct pcm_convert_state *state,
 	return NULL;
 }
 
-void notify_init(G_GNUC_UNUSED struct notify *notify)
-{
-}
-
 const struct filter_plugin *
 filter_plugin_by_name(G_GNUC_UNUSED const char *name)
 {