From bde3d1433997af8cc430f4b9d38e5bde97d3b760 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Thu, 29 Oct 2009 17:06:40 +0100 Subject: 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. --- NEWS | 1 + src/output_all.c | 26 +++++++++++++----------- src/output_control.c | 51 ++++++++++++++++++++++++++++++++++++----------- src/output_init.c | 2 +- src/output_internal.h | 14 +++++++------ src/output_thread.c | 55 ++++++++++++++++++++++++++++++++++++--------------- test/run_output.c | 4 ---- 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 #include @@ -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 #include @@ -140,11 +141,6 @@ struct audio_output { */ GThread *thread; - /** - * Notify object for the thread. - */ - struct notify notify; - /** * The next command to be performed by the output thread. */ @@ -160,6 +156,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 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) { -- cgit v1.2.3