player_control: protect command, state, error with a mutex

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;

src/decoder_control.c

@@ -18,7 +18,7 @@
  */
 
 #include "decoder_control.h"
-#include "notify.h"
+#include "player_control.h"
 
 #include <assert.h>
 
@@ -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;

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);

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;
 	}

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:

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();

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

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;
 }

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;

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

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;
@@ -110,6 +119,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
  * not point to an invalid pointer.