player_control: add GError attribute

Rewrite of the pc_get_error_message() function, now using a GError
object instead of the complicated "errored_song" attribute.

5 files changed, 57 insertions, 68 deletions

diff --git a/NEWS b/NEWS
index f5857557a..0cc597237 100644
--- a/NEWS
+++ b/NEWS
@@ -1,4 +1,5 @@
 ver 0.18 (2012/??/??)
+* improved decoder/output error reporting
 
 ver 0.17.1 (2012/??/??)
 * protocol:
diff --git a/src/player_control.c b/src/player_control.c
index 32ceedbe2..69357bf26 100644
--- a/src/player_control.c
+++ b/src/player_control.c
@@ -76,15 +76,6 @@ player_wait_decoder(struct player_control *pc, struct decoder_control *dc)
 	g_cond_wait(pc->cond, dc->mutex);
 }
 
-void
-pc_song_deleted(struct player_control *pc, const struct song *song)
-{
-	if (pc->errored_song == song) {
-		pc->error_type = PLAYER_ERROR_NONE;
-		pc->errored_song = NULL;
-	}
-}
-
 static void
 player_command_wait_locked(struct player_control *pc)
 {
@@ -229,42 +220,42 @@ pc_get_status(struct player_control *pc, struct player_status *status)
 }
 
 void
-pc_clear_error(struct player_control *pc)
+pc_set_error(struct player_control *pc, enum player_error type,
+	     GError *error)
 {
-	player_lock(pc);
-	pc->error_type = PLAYER_ERROR_NONE;
-	pc->errored_song = NULL;
-	player_unlock(pc);
+	assert(pc != NULL);
+	assert(type != PLAYER_ERROR_NONE);
+	assert(error != NULL);
+
+	if (pc->error_type != PLAYER_ERROR_NONE)
+	    g_error_free(pc->error);
+
+	pc->error_type = type;
+	pc->error = error;
 }
 
-static char *
-pc_errored_song_uri(struct player_control *pc)
+void
+pc_clear_error(struct player_control *pc)
 {
-	return song_get_uri(pc->errored_song);
+	player_lock(pc);
+
+	if (pc->error_type != PLAYER_ERROR_NONE) {
+	    pc->error_type = PLAYER_ERROR_NONE;
+	    g_error_free(pc->error);
+	}
+
+	player_unlock(pc);
 }
 
 char *
 pc_get_error_message(struct player_control *pc)
 {
-	char *error;
-	char *uri;
-
-	switch (pc->error_type) {
-	case PLAYER_ERROR_NONE:
-		return NULL;
-
-	case PLAYER_ERROR_DECODER:
-		uri = pc_errored_song_uri(pc);
-		error = g_strdup_printf("problems decoding \"%s\"", uri);
-		g_free(uri);
-		return error;
-
-	case PLAYER_ERROR_OUTPUT:
-		return g_strdup("problems opening audio device");
-	}
-
-	assert(false);
-	return NULL;
+	player_lock(pc);
+	char *message = pc->error_type != PLAYER_ERROR_NONE
+		? g_strdup(pc->error->message)
+		: NULL;
+	player_unlock(pc);
+	return message;
 }
 
 static void
diff --git a/src/player_control.h b/src/player_control.h
index 228da90fa..8be6c4694 100644
--- a/src/player_control.h
+++ b/src/player_control.h
@@ -111,12 +111,19 @@ struct player_control {
 
 	enum player_error error_type;
 
+	/**
+	 * The error that occurred in the player thread.  This
+	 * attribute is only valid if #error is not
+	 * #PLAYER_ERROR_NONE.  The object must be freed when this
+	 * object transitions back to #PLAYER_ERROR_NONE.
+	 */
+	GError *error;
+
 	uint16_t bit_rate;
 	struct audio_format audio_format;
 	float total_time;
 	float elapsed_time;
 	struct song *next_song;
-	const struct song *errored_song;
 	double seek_where;
 	float cross_fade_seconds;
 	float mixramp_db;
@@ -191,14 +198,6 @@ player_lock_signal(struct player_control *pc)
 	player_unlock(pc);
 }
 
-/**
- * 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.
- */
-void
-pc_song_deleted(struct player_control *pc, const struct song *song);
-
 void
 pc_play(struct player_control *pc, struct song *song);
 
@@ -226,6 +225,19 @@ pc_get_state(struct player_control *pc)
 	return pc->state;
 }
 
+/**
+ * Set the error.  Discards any previous error condition.
+ *
+ * Caller must lock the object.
+ *
+ * @param type the error type; must not be #PLAYER_ERROR_NONE
+ * @param error detailed error information; must not be NULL; the
+ * #player_control takes over ownership of this #GError instance
+ */
+void
+pc_set_error(struct player_control *pc, enum player_error type,
+	     GError *error);
+
 void
 pc_clear_error(struct player_control *pc);
 
diff --git a/src/player_thread.c b/src/player_thread.c
index be314849b..486a8c96e 100644
--- a/src/player_thread.c
+++ b/src/player_thread.c
@@ -235,10 +235,10 @@ player_wait_for_decoder(struct player *player)
 
 	player->queued = false;
 
-	if (decoder_lock_has_failed(dc)) {
+	GError *error = dc_lock_get_error(dc);
+	if (error != NULL) {
 		player_lock(pc);
-		pc->errored_song = dc->song;
-		pc->error_type = PLAYER_ERROR_DECODER;
+		pc_set_error(pc, PLAYER_ERROR_DECODER, error);
 		pc->next_song = NULL;
 		player_unlock(pc);
 
@@ -318,7 +318,6 @@ player_open_output(struct player *player)
 		return true;
 	} else {
 		g_warning("%s", error->message);
-		g_error_free(error);
 
 		player->output_open = false;
 
@@ -327,7 +326,7 @@ player_open_output(struct player *player)
 		player->paused = true;
 
 		player_lock(pc);
-		pc->error_type = PLAYER_ERROR_OUTPUT;
+		pc_set_error(pc, PLAYER_ERROR_OUTPUT, error);
 		pc->state = PLAYER_STATE_PAUSE;
 		player_unlock(pc);
 
@@ -352,13 +351,13 @@ player_check_decoder_startup(struct player *player)
 
 	decoder_lock(dc);
 
-	if (decoder_has_failed(dc)) {
+	GError *error = dc_get_error(dc);
+	if (error != NULL) {
 		/* the decoder failed */
 		decoder_unlock(dc);
 
 		player_lock(pc);
-		pc->errored_song = dc->song;
-		pc->error_type = PLAYER_ERROR_DECODER;
+		pc_set_error(pc, PLAYER_ERROR_DECODER, error);
 		player_unlock(pc);
 
 		return false;
@@ -797,13 +796,12 @@ play_next_chunk(struct player *player)
 	if (!play_chunk(player->pc, player->song, chunk,
 			&player->play_audio_format, &error)) {
 		g_warning("%s", error->message);
-		g_error_free(error);
 
 		music_buffer_return(player_buffer, chunk);
 
 		player_lock(pc);
 
-		pc->error_type = PLAYER_ERROR_OUTPUT;
+		pc_set_error(pc, PLAYER_ERROR_OUTPUT, error);
 
 		/* pause: the user may resume playback as soon as an
 		   audio output becomes available */
diff --git a/src/playlist_edit.c b/src/playlist_edit.c
index 7adbccd7c..1dfe68daa 100644
--- a/src/playlist_edit.c
+++ b/src/playlist_edit.c
@@ -45,14 +45,6 @@ playlist_clear(struct playlist *playlist, struct player_control *pc)
 {
 	playlist_stop(playlist, pc);
 
-	/* make sure there are no references to allocated songs
-	   anymore */
-	for (unsigned i = 0; i < queue_length(&playlist->queue); i++) {
-		const struct song *song = queue_get(&playlist->queue, i);
-		if (!song_in_database(song))
-			pc_song_deleted(pc, song);
-	}
-
 	queue_clear(&playlist->queue);
 
 	playlist->current = -1;
@@ -287,9 +279,6 @@ playlist_delete_internal(struct playlist *playlist, struct player_control *pc,
 
 	/* now do it: remove the song */
 
-	if (!song_in_database(queue_get(&playlist->queue, song)))
-		pc_song_deleted(pc, queue_get(&playlist->queue, song));
-
 	queue_delete(&playlist->queue, song);
 
 	/* update the "current" and "queued" variables */
@@ -363,8 +352,6 @@ playlist_delete_song(struct playlist *playlist, struct player_control *pc,
 	for (int i = queue_length(&playlist->queue) - 1; i >= 0; --i)
 		if (song == queue_get(&playlist->queue, i))
 			playlist_delete(playlist, pc, i);
-
-	pc_song_deleted(pc, song);
 }
 
 enum playlist_result