From 25a806a347ce420126eb75d82c5fb875eb0a5e0d Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Sat, 31 Oct 2009 17:02:12 +0100 Subject: player_control: protect command, state, error with a mutex Use GMutex/GCond instead of the notify library. Manually lock the player_control object before accessing the protected attributes. Use the GCond object to notify the player thread and the main thread. --- src/decoder_api.c | 10 ++-- src/decoder_control.c | 36 ++++++------- src/decoder_control.h | 10 ++-- src/decoder_internal.c | 5 +- src/decoder_thread.c | 12 ++--- src/main.c | 6 +-- src/main.h | 2 +- src/output_all.c | 9 +++- src/output_thread.c | 2 +- src/player_control.c | 42 ++++++++++++--- src/player_control.h | 78 +++++++++++++++++++++++++-- src/player_thread.c | 140 +++++++++++++++++++++++++++++++++++++++---------- 12 files changed, 263 insertions(+), 89 deletions(-) diff --git a/src/decoder_api.c b/src/decoder_api.c index 4cff9916c..070151a67 100644 --- a/src/decoder_api.c +++ b/src/decoder_api.c @@ -61,7 +61,7 @@ void decoder_initialized(G_GNUC_UNUSED struct decoder * decoder, dc.state = DECODE_STATE_DECODE; decoder_unlock(); - notify_signal(&pc.notify); + player_lock_signal(); g_debug("audio_format=%u:%u:%u, seekable=%s", dc.in_audio_format.sample_rate, dc.in_audio_format.bits, @@ -112,7 +112,7 @@ void decoder_command_finished(G_GNUC_UNUSED struct decoder * decoder) dc.command = DECODE_COMMAND_NONE; decoder_unlock(); - notify_signal(&pc.notify); + player_lock_signal(); } double decoder_seek_where(G_GNUC_UNUSED struct decoder * decoder) @@ -184,7 +184,7 @@ do_send_tag(struct decoder *decoder, struct input_stream *is, /* there is a partial chunk - flush it, we want the tag in a new chunk */ decoder_flush_chunk(decoder); - notify_signal(&pc.notify); + player_lock_signal(); } assert(decoder->chunk == NULL); @@ -297,7 +297,7 @@ decoder_data(struct decoder *decoder, if (dest == NULL) { /* the chunk is full, flush it */ decoder_flush_chunk(decoder); - notify_signal(&pc.notify); + player_lock_signal(); continue; } @@ -324,7 +324,7 @@ decoder_data(struct decoder *decoder, if (full) { /* the chunk is full, flush it */ decoder_flush_chunk(decoder); - notify_signal(&pc.notify); + player_lock_signal(); } data += nbytes; diff --git a/src/decoder_control.c b/src/decoder_control.c index 3b993431c..d7f5808fc 100644 --- a/src/decoder_control.c +++ b/src/decoder_control.c @@ -18,7 +18,7 @@ */ #include "decoder_control.h" -#include "notify.h" +#include "player_control.h" #include @@ -40,38 +40,34 @@ void dc_deinit(void) } static void -dc_command_wait_locked(struct notify *notify) +dc_command_wait_locked(void) { while (dc.command != DECODE_COMMAND_NONE) { decoder_signal(); - decoder_unlock(); - - notify_wait(notify); - - decoder_lock(); + player_wait_decoder(); } } void -dc_command_wait(struct notify *notify) +dc_command_wait(void) { decoder_lock(); - dc_command_wait_locked(notify); + dc_command_wait_locked(); decoder_unlock(); } static void -dc_command_locked(struct notify *notify, enum decoder_command cmd) +dc_command_locked(enum decoder_command cmd) { dc.command = cmd; - dc_command_wait_locked(notify); + dc_command_wait_locked(); } static void -dc_command(struct notify *notify, enum decoder_command cmd) +dc_command(enum decoder_command cmd) { decoder_lock(); - dc_command_locked(notify, cmd); + dc_command_locked(cmd); decoder_unlock(); } @@ -86,13 +82,13 @@ static void dc_command_async(enum decoder_command cmd) } void -dc_start(struct notify *notify, struct song *song) +dc_start(struct song *song) { assert(dc.pipe != NULL); assert(song != NULL); dc.next_song = song; - dc_command(notify, DECODE_COMMAND_START); + dc_command(DECODE_COMMAND_START); } void @@ -106,7 +102,7 @@ dc_start_async(struct song *song) } void -dc_stop(struct notify *notify) +dc_stop(void) { decoder_lock(); @@ -115,16 +111,16 @@ dc_stop(struct notify *notify) late and the decoder thread is already executing the old command, we'll call STOP again in this function (see below). */ - dc_command_locked(notify, DECODE_COMMAND_STOP); + dc_command_locked(DECODE_COMMAND_STOP); if (dc.state != DECODE_STATE_STOP && dc.state != DECODE_STATE_ERROR) - dc_command_locked(notify, DECODE_COMMAND_STOP); + dc_command_locked(DECODE_COMMAND_STOP); decoder_unlock(); } bool -dc_seek(struct notify *notify, double where) +dc_seek(double where) { assert(dc.state != DECODE_STATE_START); assert(where >= 0.0); @@ -135,7 +131,7 @@ dc_seek(struct notify *notify, double where) dc.seek_where = where; dc.seek_error = false; - dc_command(notify, DECODE_COMMAND_SEEK); + dc_command(DECODE_COMMAND_SEEK); if (dc.seek_error) return false; diff --git a/src/decoder_control.h b/src/decoder_control.h index 7e861f970..6b65da2f2 100644 --- a/src/decoder_control.h +++ b/src/decoder_control.h @@ -30,8 +30,6 @@ #define DECODE_TYPE_FILE 0 #define DECODE_TYPE_URL 1 -struct notify; - enum decoder_state { DECODE_STATE_STOP = 0, DECODE_STATE_START, @@ -205,19 +203,19 @@ decoder_current_song(void) } void -dc_command_wait(struct notify *notify); +dc_command_wait(void); void -dc_start(struct notify *notify, struct song *song); +dc_start(struct song *song); void dc_start_async(struct song *song); void -dc_stop(struct notify *notify); +dc_stop(void); bool -dc_seek(struct notify *notify, double where); +dc_seek(double where); void dc_quit(void); diff --git a/src/decoder_internal.c b/src/decoder_internal.c index 1b064d0aa..db90333b4 100644 --- a/src/decoder_internal.c +++ b/src/decoder_internal.c @@ -58,10 +58,7 @@ need_chunks(struct input_stream *is, bool do_wait) if ((is == NULL || decoder_input_buffer(is) <= 0) && do_wait) { decoder_wait(); - - decoder_unlock(); - notify_signal(&pc.notify); - decoder_lock(); + player_signal(); return dc.command; } diff --git a/src/decoder_thread.c b/src/decoder_thread.c index 846b12353..712e4d20c 100644 --- a/src/decoder_thread.c +++ b/src/decoder_thread.c @@ -112,9 +112,7 @@ static void decoder_run_song(const struct song *song, const char *uri) dc.state = DECODE_STATE_START; dc.command = DECODE_COMMAND_NONE; - decoder_unlock(); - notify_signal(&pc.notify); - decoder_lock(); + player_signal(); /* wait for the input stream to become ready; its metadata will be available then */ @@ -294,17 +292,13 @@ static gpointer decoder_task(G_GNUC_UNUSED gpointer arg) dc.command = DECODE_COMMAND_NONE; - decoder_unlock(); - notify_signal(&pc.notify); - decoder_lock(); + player_signal(); break; case DECODE_COMMAND_STOP: dc.command = DECODE_COMMAND_NONE; - decoder_unlock(); - notify_signal(&pc.notify); - decoder_lock(); + player_signal(); break; case DECODE_COMMAND_NONE: diff --git a/src/main.c b/src/main.c index 86fe069e5..18647eb80 100644 --- a/src/main.c +++ b/src/main.c @@ -93,7 +93,7 @@ enum { GThread *main_task; GMainLoop *main_loop; -struct notify main_notify; +GCond *main_cond; static void glue_daemonize_init(const struct options *options) @@ -321,7 +321,7 @@ int main(int argc, char *argv[]) main_task = g_thread_self(); main_loop = g_main_loop_new(NULL, FALSE); - notify_init(&main_notify); + main_cond = g_cond_new(); event_pipe_init(); event_pipe_register(PIPE_EVENT_IDLE, idle_event_emitted); @@ -415,7 +415,7 @@ int main(int argc, char *argv[]) sticker_global_finish(); #endif - notify_deinit(&main_notify); + g_cond_free(main_cond); event_pipe_deinit(); playlist_list_global_finish(); diff --git a/src/main.h b/src/main.h index 8ed02bf5d..de673829d 100644 --- a/src/main.h +++ b/src/main.h @@ -26,6 +26,6 @@ extern GThread *main_task; extern GMainLoop *main_loop; -extern struct notify main_notify; +extern GCond *main_cond; #endif diff --git a/src/output_all.c b/src/output_all.c index 08f4ee0dc..bdf0385bb 100644 --- a/src/output_all.c +++ b/src/output_all.c @@ -445,10 +445,15 @@ audio_output_all_check(void) bool audio_output_all_wait(unsigned threshold) { - if (audio_output_all_check() < threshold) + player_lock(); + + if (audio_output_all_check() < threshold) { + player_unlock(); return true; + } - notify_wait(&pc.notify); + player_wait(); + player_unlock(); return audio_output_all_check() < threshold; } diff --git a/src/output_thread.c b/src/output_thread.c index 24a22f6df..ef8fe7418 100644 --- a/src/output_thread.c +++ b/src/output_thread.c @@ -359,7 +359,7 @@ ao_play(struct audio_output *ao) ao->chunk_finished = true; g_mutex_unlock(ao->mutex); - notify_signal(&pc.notify); + player_lock_signal(); g_mutex_lock(ao->mutex); return true; diff --git a/src/player_control.c b/src/player_control.c index 23ae136ac..aa018ceda 100644 --- a/src/player_control.c +++ b/src/player_control.c @@ -18,6 +18,7 @@ */ #include "player_control.h" +#include "decoder_control.h" #include "path.h" #include "log.h" #include "tag.h" @@ -35,7 +36,10 @@ void pc_init(unsigned buffer_chunks, unsigned int buffered_before_play) { pc.buffer_chunks = buffer_chunks; pc.buffered_before_play = buffered_before_play; - notify_init(&pc.notify); + + pc.mutex = g_mutex_new(); + pc.cond = g_cond_new(); + pc.command = PLAYER_COMMAND_NONE; pc.error = PLAYER_ERROR_NOERROR; pc.state = PLAYER_STATE_STOP; @@ -44,7 +48,16 @@ void pc_init(unsigned buffer_chunks, unsigned int buffered_before_play) void pc_deinit(void) { - notify_deinit(&pc.notify); + g_cond_free(pc.cond); + g_mutex_free(pc.mutex); +} + +void +player_wait_decoder(void) +{ + /* during this function, the decoder lock is held, because + we're waiting for the decoder thread */ + g_cond_wait(pc.cond, dc.mutex); } void @@ -56,15 +69,30 @@ pc_song_deleted(const struct song *song) } } -static void player_command(enum player_command cmd) +static void +player_command_wait_locked(void) +{ + while (pc.command != PLAYER_COMMAND_NONE) { + player_signal(); + g_cond_wait(main_cond, pc.mutex); + } +} + +static void +player_command_locked(enum player_command cmd) { assert(pc.command == PLAYER_COMMAND_NONE); pc.command = cmd; - while (pc.command != PLAYER_COMMAND_NONE) { - notify_signal(&pc.notify); - notify_wait(&main_notify); - } + player_command_wait_locked(); +} + +static void +player_command(enum player_command cmd) +{ + player_lock(); + player_command_locked(cmd); + player_unlock(); } void diff --git a/src/player_control.h b/src/player_control.h index c0f1d4f07..449e98aa5 100644 --- a/src/player_control.h +++ b/src/player_control.h @@ -88,10 +88,19 @@ struct player_control { thread isn't running */ GThread *thread; - struct notify notify; - volatile enum player_command command; - volatile enum player_state state; - volatile enum player_error error; + /** + * This lock protects #command, #state, #error. + */ + GMutex *mutex; + + /** + * Trigger this object after you have modified #command. + */ + GCond *cond; + + enum player_command command; + enum player_state state; + enum player_error error; uint16_t bit_rate; struct audio_format audio_format; float total_time; @@ -109,6 +118,67 @@ void pc_init(unsigned buffer_chunks, unsigned buffered_before_play); void pc_deinit(void); +/** + * Locks the #player_control object. + */ +static inline void +player_lock(void) +{ + g_mutex_lock(pc.mutex); +} + +/** + * Unlocks the #player_control object. + */ +static inline void +player_unlock(void) +{ + g_mutex_unlock(pc.mutex); +} + +/** + * Waits for a signal on the #player_control object. This function is + * only valid in the player thread. The object must be locked prior + * to calling this function. + */ +static inline void +player_wait(void) +{ + g_cond_wait(pc.cond, pc.mutex); +} + +/** + * Waits for a signal on the #player_control object. This function is + * only valid in the player thread. The #decoder_control object must + * be locked prior to calling this function. + * + * Note the small difference to the player_wait() function! + */ +void +player_wait_decoder(void); + +/** + * Signals the #player_control object. The object should be locked + * prior to calling this function. + */ +static inline void +player_signal(void) +{ + g_cond_signal(pc.cond); +} + +/** + * Signals the #player_control object. The object is temporarily + * locked by this function. + */ +static inline void +player_lock_signal(void) +{ + player_lock(); + player_signal(); + player_unlock(); +} + /** * Call this function when the specified song pointer is about to be * invalidated. This makes sure that player_control.errored_song does diff --git a/src/player_thread.c b/src/player_thread.c index dbafa168f..0c88d1a5c 100644 --- a/src/player_thread.c +++ b/src/player_thread.c @@ -106,21 +106,30 @@ struct player { static struct music_buffer *player_buffer; -static void player_command_finished(void) +static void player_command_finished_locked(void) { assert(pc.command != PLAYER_COMMAND_NONE); pc.command = PLAYER_COMMAND_NONE; - notify_signal(&main_notify); + g_cond_signal(main_cond); +} + +static void player_command_finished(void) +{ + player_lock(); + player_command_finished_locked(); + player_unlock(); } /** * Stop the decoder and clears (and frees) its music pipe. + * + * Player lock is not held. */ static void player_dc_stop(struct player *player) { - dc_stop(&pc.notify); + dc_stop(); if (dc.pipe != NULL) { /* clear and free the decoder pipe */ @@ -138,17 +147,23 @@ player_dc_stop(struct player *player) * After the decoder has been started asynchronously, wait for the * "START" command to finish. The decoder may not be initialized yet, * i.e. there is no audio_format information yet. + * + * The player lock is not held. */ static bool player_wait_for_decoder(struct player *player) { - dc_command_wait(&pc.notify); + dc_command_wait(); if (decoder_lock_has_failed()) { assert(dc.next_song == NULL || dc.next_song->uri != NULL); + + player_lock(); pc.errored_song = dc.next_song; pc.error = PLAYER_ERROR_FILE; pc.next_song = NULL; + player_unlock(); + player->queued = false; return false; } @@ -177,6 +192,8 @@ player_wait_for_decoder(struct player *player) * The decoder has acknowledged the "START" command (see * player_wait_for_decoder()). This function checks if the decoder * initialization has completed yet. + * + * The player lock is not held. */ static bool player_check_decoder_startup(struct player *player) @@ -234,8 +251,8 @@ player_check_decoder_startup(struct player *player) } else { /* the decoder is not yet ready; wait some more */ + player_wait_decoder(); decoder_unlock(); - notify_wait(&pc.notify); return true; } @@ -245,6 +262,8 @@ player_check_decoder_startup(struct player *player) * Sends a chunk of silence to the audio outputs. This is called when * there is not enough decoded data in the pipe yet, to prevent * underruns in the hardware buffers. + * + * The player lock is not held. */ static bool player_send_silence(struct player *player) @@ -281,6 +300,8 @@ player_send_silence(struct player *player) /** * This is the handler for the #PLAYER_COMMAND_SEEK command. + * + * The player lock is not held. */ static bool player_seek_decoder(struct player *player) { @@ -332,7 +353,7 @@ static bool player_seek_decoder(struct player *player) if (where < 0.0) where = 0.0; - ret = dc_seek(&pc.notify, where); + ret = dc_seek(where); if (!ret) { /* decoder failure */ player_command_finished(); @@ -353,6 +374,9 @@ static bool player_seek_decoder(struct player *player) return true; } +/** + * Player lock must be held before calling. + */ static void player_process_command(struct player *player) { switch (pc.command) { @@ -363,8 +387,10 @@ static void player_process_command(struct player *player) break; case PLAYER_COMMAND_UPDATE_AUDIO: + player_unlock(); audio_output_all_enable_disable(); - player_command_finished(); + player_lock(); + player_command_finished_locked(); break; case PLAYER_COMMAND_QUEUE: @@ -373,21 +399,28 @@ static void player_process_command(struct player *player) assert(dc.pipe == NULL || dc.pipe == player->pipe); player->queued = true; - player_command_finished(); + player_command_finished_locked(); break; case PLAYER_COMMAND_PAUSE: + player_unlock(); + player->paused = !player->paused; if (player->paused) { audio_output_all_pause(); + player_lock(); + pc.state = PLAYER_STATE_PAUSE; } else if (!audio_format_defined(&player->play_audio_format)) { /* the decoder hasn't provided an audio format yet - don't open the audio device yet */ + player_lock(); pc.state = PLAYER_STATE_PLAY; } else if (audio_output_all_open(&player->play_audio_format, player_buffer)) { /* unpaused, continue playing */ + player_lock(); + pc.state = PLAYER_STATE_PLAY; } else { /* the audio device has failed - rollback to @@ -397,13 +430,17 @@ static void player_process_command(struct player *player) pc.error = PLAYER_ERROR_AUDIO; player->paused = true; + + player_lock(); } - player_command_finished(); + player_command_finished_locked(); break; case PLAYER_COMMAND_SEEK: + player_unlock(); player_seek_decoder(player); + player_lock(); break; case PLAYER_COMMAND_CANCEL: @@ -415,26 +452,32 @@ static void player_process_command(struct player *player) return; } - if (dc.pipe != NULL && dc.pipe != player->pipe) + if (dc.pipe != NULL && dc.pipe != player->pipe) { /* the decoder is already decoding the song - stop it and reset the position */ + player_unlock(); player_dc_stop(player); + player_lock(); + } pc.next_song = NULL; player->queued = false; - player_command_finished(); + player_command_finished_locked(); break; case PLAYER_COMMAND_REFRESH: if (audio_format_defined(&player->play_audio_format) && - !player->paused) + !player->paused) { + player_unlock(); audio_output_all_check(); + player_lock(); + } pc.elapsed_time = audio_output_all_get_elapsed_time(); if (pc.elapsed_time < 0.0) pc.elapsed_time = player->elapsed_time; - player_command_finished(); + player_command_finished_locked(); break; } } @@ -468,6 +511,8 @@ update_song_tag(struct song *song, const struct tag *new_tag) * Plays a #music_chunk object (after applying software volume). If * it contains a (stream) tag, copy it to the current song, so MPD's * playlist reflects the new stream tag. + * + * Player lock is not held. */ static bool play_chunk(struct song *song, struct music_chunk *chunk, @@ -553,10 +598,9 @@ play_next_chunk(struct player *player) } else { /* wait for the decoder */ decoder_signal(); + player_wait_decoder(); decoder_unlock(); - notify_wait(&pc.notify); - return true; } } @@ -574,6 +618,8 @@ play_next_chunk(struct player *player) if (!success) { music_buffer_return(player_buffer, chunk); + player_lock(); + pc.error = PLAYER_ERROR_AUDIO; /* pause: the user may resume playback as soon as an @@ -581,6 +627,8 @@ play_next_chunk(struct player *player) pc.state = PLAYER_STATE_PAUSE; player->paused = true; + player_unlock(); + return false; } @@ -602,6 +650,8 @@ play_next_chunk(struct player *player) * has consumed all chunks of the current song, and we should start * sending chunks from the next one. * + * The player lock is not held. + * * @return true on success, false on error (playback will be stopped) */ static bool @@ -643,31 +693,38 @@ static void do_play(void) .elapsed_time = 0.0, }; + player_unlock(); + player.pipe = music_pipe_new(); dc.buffer = player_buffer; dc.pipe = player.pipe; - dc_start(&pc.notify, pc.next_song); + dc_start(pc.next_song); if (!player_wait_for_decoder(&player)) { player_dc_stop(&player); player_command_finished(); music_pipe_free(player.pipe); event_pipe_emit(PIPE_EVENT_PLAYLIST); + player_lock(); return; } + player_lock(); pc.state = PLAYER_STATE_PLAY; - player_command_finished(); + player_command_finished_locked(); while (true) { player_process_command(&player); if (pc.command == PLAYER_COMMAND_STOP || pc.command == PLAYER_COMMAND_EXIT || pc.command == PLAYER_COMMAND_CLOSE_AUDIO) { + player_unlock(); audio_output_all_cancel(); break; } + player_unlock(); + if (player.buffering) { /* buffering at the start of the song - wait until the buffer is large enough, to @@ -683,7 +740,10 @@ static void do_play(void) !player_send_silence(&player)) break; - notify_wait(&pc.notify); + decoder_lock(); + /* XXX race condition: check decoder again */ + player_wait_decoder(); + decoder_unlock(); continue; } else { /* buffering is complete */ @@ -698,6 +758,8 @@ static void do_play(void) success = player_check_decoder_startup(&player); if (!success) break; + + player_lock(); continue; } @@ -741,9 +803,11 @@ static void do_play(void) player.xfade = XFADE_DISABLED; } - if (player.paused) - notify_wait(&pc.notify); - else if (music_pipe_size(player.pipe) > 0) { + if (player.paused) { + player_lock(); + player_wait(); + continue; + } else if (music_pipe_size(player.pipe) > 0) { /* at least one music chunk is ready - send it to the audio output */ @@ -773,6 +837,8 @@ static void do_play(void) if (!player_send_silence(&player)) break; } + + player_lock(); } player_dc_stop(&player); @@ -783,8 +849,13 @@ static void do_play(void) music_pipe_clear(player.pipe, player_buffer); music_pipe_free(player.pipe); + player_lock(); pc.state = PLAYER_STATE_STOP; + player_unlock(); + event_pipe_emit(PIPE_EVENT_PLAYLIST); + + player_lock(); } static gpointer player_task(G_GNUC_UNUSED gpointer arg) @@ -793,6 +864,8 @@ static gpointer player_task(G_GNUC_UNUSED gpointer arg) player_buffer = music_buffer_new(pc.buffer_chunks); + player_lock(); + while (1) { switch (pc.command) { case PLAYER_COMMAND_QUEUE: @@ -802,18 +875,25 @@ static gpointer player_task(G_GNUC_UNUSED gpointer arg) break; case PLAYER_COMMAND_STOP: + player_unlock(); audio_output_all_cancel(); + player_lock(); + /* fall through */ case PLAYER_COMMAND_SEEK: case PLAYER_COMMAND_PAUSE: pc.next_song = NULL; - player_command_finished(); + player_command_finished_locked(); break; case PLAYER_COMMAND_CLOSE_AUDIO: + player_unlock(); + audio_output_all_close(); - player_command_finished(); + + player_lock(); + player_command_finished_locked(); #ifndef NDEBUG /* in the DEBUG build, check for leaked @@ -826,33 +906,39 @@ static gpointer player_task(G_GNUC_UNUSED gpointer arg) break; case PLAYER_COMMAND_UPDATE_AUDIO: + player_unlock(); audio_output_all_enable_disable(); - player_command_finished(); + player_lock(); + player_command_finished_locked(); break; case PLAYER_COMMAND_EXIT: + player_unlock(); + dc_quit(); audio_output_all_close(); music_buffer_free(player_buffer); + player_command_finished(); g_thread_exit(NULL); break; case PLAYER_COMMAND_CANCEL: pc.next_song = NULL; - player_command_finished(); + player_command_finished_locked(); break; case PLAYER_COMMAND_REFRESH: /* no-op when not playing */ - player_command_finished(); + player_command_finished_locked(); break; case PLAYER_COMMAND_NONE: - notify_wait(&pc.notify); + player_wait(); break; } } + return NULL; } -- cgit v1.2.3