Completed the output abstraction (also in buffer; also unified closing of output stuff with exit_output).

Made mpg123 check for errors in output (while writing - so after _opening_ was successful) so that it does't rage-decode in invalid files/pipes.


git-svn-id: svn://scm.orgis.org/mpg123/trunk@1278 35dc7657-300d-0410-a2e5-dc2837fedb53

Makefile.am

@@ -21,4 +21,5 @@ EXTRA_DIST = \
 	xmms2-plugin/mpg123/mpg123.c \
 	xmms2-plugin/mpg123/wscript \
 	makedll.sh \
+	test/forkfaint.c \
 	test/rms16.c

NEWS

@@ -15,6 +15,7 @@
   This sort of qualifies as bugfix, actually, since it's item 1786430 on the tracker;-)
 - Make flexible rate computations more safe (especially on 32bit platforms) by switching to the looping code instead of direct multiplication/division (which blows up with integers). A possible effect was premature track ending with a forced rate.
 - Flexible rate mode works _at_all_ again with fixed output support detection code (comparison of -1 with a size_t ended a loop all too early).
+- check for error in flushing output (this ends mpg123 for a broken pipe, for example)
 
 1.0rc1
 ----

src/audio.c

@@ -378,15 +378,12 @@ int init_output(audio_output_t **ao)
 				audio_output_t *bao = NULL; /* To be clear: That's the buffer's pointer. */
 				param.usebuffer = 0; /* The buffer doesn't use the buffer. */
 				/* Open audio output module */
-				if(param.outmode == DECODE_AUDIO)
-				{
 				bao = open_output_module(param.output_module);
 				if(!bao)
 				{
 					error("Failed to open audio output module.");
 					exit(1); /* communicate failure? */
 				}
-				}
 				if(open_output(bao) < 0)
 				{
 					error("Unable to open audio output.");
@@ -439,6 +436,25 @@ int init_output(audio_output_t **ao)
 	return 0;
 }
 
+void exit_output(audio_output_t *ao, int rude)
+{
+	debug("exit output");
+#ifndef NOXFERMEM
+	if (param.usebuffer)
+	{
+		debug("ending buffer");
+		buffer_stop(); /* Puts buffer into waiting-for-command mode. */
+		buffer_end(rude);  /* Gives command to end operation. */
+		xfermem_done_writer(buffermem);
+		waitpid (buffer_pid, NULL, 0);
+		xfermem_done (buffermem);
+	}
+#endif
+	/* Close the output... doesn't matter if buffer handled it, that's taken care of. */
+	close_output(ao);
+	close_output_module(ao);
+}
+
 void output_pause(audio_output_t *ao)
 {
 	if(param.usebuffer) buffer_stop();
@@ -450,17 +466,19 @@ void output_unpause(audio_output_t *ao)
 	if(param.usebuffer) buffer_start();
 }
 
-void flush_output(audio_output_t *ao, unsigned char *bytes, size_t count)
+int flush_output(audio_output_t *ao, unsigned char *bytes, size_t count)
 {
 	if(count)
 	{
 		/* Error checks? */
 #ifndef NOXFERMEM
-		if(param.usebuffer) xfermem_write(buffermem, bytes, count);
+		if(param.usebuffer){ if(xfermem_write(buffermem, bytes, count)) return -1; }
 		else
 #endif
-		if(param.outmode != DECODE_TEST) ao->write(ao, bytes, count);
+		if(param.outmode != DECODE_TEST)
+		return ao->write(ao, bytes, (int)count);
 	}
+	return (int)count; /* That is for DECODE_TEST */
 }
 
 int open_output(audio_output_t *ao)

src/audio.h

@@ -77,7 +77,8 @@ int audio_fit_capabilities(audio_output_t *ao,int c,int r);
 const char* audio_encoding_name(const int encoding, const int longer);
 
 int init_output(audio_output_t **ao);
-void flush_output(audio_output_t *ao, unsigned char *bytes, size_t count);
+void exit_output(audio_output_t *ao, int rude);
+int flush_output(audio_output_t *ao, unsigned char *bytes, size_t count);
 int open_output(audio_output_t *ao);
 void close_output(audio_output_t *ao );
 int reset_output(audio_output_t *ao);

src/buffer.c

@@ -107,7 +107,7 @@ void buffer_sig(int signal, int block)
 
 void buffer_loop(audio_output_t *ao, sigset_t *oldsigset)
 {
-	int bytes;
+	int bytes, outbytes;
 	int my_fd = buffermem->fd[XF_READER];
 	txfermem *xf = buffermem;
 	int done = FALSE;
@@ -118,8 +118,7 @@ void buffer_loop(audio_output_t *ao, sigset_t *oldsigset)
 	sigprocmask (SIG_SETMASK, oldsigset, NULL);
 
 	xfermem_putcmd(my_fd, XF_CMD_WAKEUP);
-	if(param.outmode == DECODE_AUDIO)
+
-	{
 	debug("audio output: waiting for cap requests");
 	/* wait for audio setup queries */
 	while(1)
@@ -146,7 +145,6 @@ void buffer_loop(audio_output_t *ao, sigset_t *oldsigset)
 			return;
 		}
 	}
-	}
 
 	/* Fill complete buffer on first run before starting to play.
 	 * Live mp3 streams constantly approach buffer underrun otherwise. [dk]
@@ -272,18 +270,14 @@ void buffer_loop(audio_output_t *ao, sigset_t *oldsigset)
 		if (bytes > outburst)
 			bytes = outburst;
 
-		/* Could change that to use flush_output.... need to capture return value, then. */
 		debug("write");
-		if (param.outmode == DECODE_FILE)
+		outbytes = flush_output(ao, (unsigned char*) xf->data + xf->readindex, bytes);
-			bytes = write(OutputDescriptor, xf->data + xf->readindex, bytes);
-		else if (param.outmode == DECODE_AUDIO)
-			bytes = ao->write(ao,
-				(unsigned char *) (xf->data + xf->readindex), bytes);
 
-		if(bytes < 0) {
+		if(outbytes < bytes)
-			bytes = 0;
+		{
+			if(outbytes < 0) outbytes = 0;
 			if(!intflag && !usr1flag) {
-				perror("Ouch ... error while writing audio data: ");
+				error1("Ouch ... error while writing audio data: %s", strerror(errno));
 				/*
 				 * done==TRUE tells writer process to stop
 				 * sending data. There might be some latency
@@ -299,6 +293,7 @@ debug("write");
 			}
 			else debug("buffer interrupted");
 		}
+		bytes = outbytes;
 
 		xf->readindex = (xf->readindex + bytes) % xf->size;
 		if (xf->wakeme[XF_WRITER])

src/mpg123.c

@@ -115,6 +115,7 @@ char *prgName = NULL;
 char *equalfile = NULL;
 struct httpdata htd;
 int fresh = TRUE;
+int have_output = FALSE; /* If we are past the output init step. */
 
 int buffer_fd[2];
 int buffer_pid;
@@ -154,7 +155,10 @@ void safe_exit(int code)
 	if(param.term_ctrl)
 		term_restore();
 #endif
+	if(have_output) exit_output(ao, intflag);
+
 	if(mh != NULL) mpg123_delete(mh);
+
 	mpg123_exit();
 	httpdata_reset(&htd);
 	/* It's ugly... but let's just fix this still-reachable memory chunk of static char*. */
@@ -524,7 +528,11 @@ int play_frame(void)
 			else print_header_compact(mh);
 		}
 		/* Normal flushing of data, includes buffer decoding. */
-		flush_output(ao, audio, bytes);
+		if(flush_output(ao, audio, bytes) < (int)bytes)
+		{
+			error("Deep trouble! Cannot flush to my output anymore!");
+			safe_exit(133);
+		}
 		if(param.checkrange)
 		{
 			long clip = mpg123_clip(mh);
@@ -659,6 +667,7 @@ int main(int argc, char *argv[])
 		mpg123_delete_pars(mp);
 		return 99; /* It's safe here... nothing nasty happened yet. */
 	}
+	have_output = TRUE;
 
 	/* ========================================================================================================= */
 	/* Enterning the leaking zone... we start messing with stuff here that should be taken care of when leaving. */
@@ -776,8 +785,6 @@ int main(int argc, char *argv[])
 	if(param.remote) {
 		int ret;
 		ret = control_generic(mh);
-		close_output(ao);
-		close_output_module(ao);
 		safe_exit(ret);
 	}
 
@@ -964,23 +971,9 @@ int main(int argc, char *argv[])
 #endif
       }
     } /* end of loop over input files */
-#ifndef NOXFERMEM
-    if (param.usebuffer) {
-      debug("ending buffer");
-      buffer_stop(); /* Puts buffer into waiting-for-command mode. */
-      buffer_end(intflag);  /* Gives command to end operation. */
-      xfermem_done_writer (buffermem);
-      waitpid (buffer_pid, NULL, 0);
-      xfermem_done (buffermem);
-    }
-#endif
-	debug("close output");
-	/* Close the output... doesn't matter if buffer handled it, that's taken care of. */
-	close_output(ao);
-	close_output_module(ao);
 	/* Free up memory used by playlist */    
 	if(!param.remote) free_playlist();
-	safe_exit(0);
+	safe_exit(0); /* That closes output, too. */
 	return 0;
 }
 

src/xfermem.c

@@ -226,8 +226,12 @@ int xfermem_write(txfermem *xf, byte *buffer, size_t bytes)
 	/* You weren't so braindead to have not allocated enough space at all, right? */
 	while (xfermem_get_freespace(xf) < bytes)
 	{
-		if (xfermem_block(XF_WRITER, xf) == XF_CMD_TERMINATE)
+		int cmd =  xfermem_block(XF_WRITER, xf);
-		return TRUE;
+		if (cmd == XF_CMD_TERMINATE || cmd < 0)
+		{
+			error("failed to wait for free space");
+			return TRUE; /* Failure. */
+		}
 	}
 	/* Now we have enough space. copy the memory, possibly with the wrap. */
 	if(xf->size - xf->freeindex >= bytes)
@@ -244,7 +248,8 @@ int xfermem_write(txfermem *xf, byte *buffer, size_t bytes)
 	xf->freeindex = (xf->freeindex + bytes) % xf->size;
 	/* Wake up the buffer process if necessary. */
 	debug("write waking");
-	if(xf->wakeme[XF_READER]) xfermem_putcmd(xf->fd[XF_WRITER], XF_CMD_WAKEUP_INFO);
+	if(xf->wakeme[XF_READER])
+	return xfermem_putcmd(xf->fd[XF_WRITER], XF_CMD_WAKEUP_INFO) < 0 ? TRUE : FALSE;
 
 	return FALSE;
 }

test/forkfaint.c

@@ -0,0 +1,41 @@
+/*
+	A program that forks mpg123 to read it's output, then faints to release a zombie.
+	What does mpg123 do?
+		- pre 1.0rc2:   Decode wildly until input end reached, writing into the nonexistent pipe.
+		- since 1.0rc2: Ending because of I/O error.
+
+I guess that is what is at the root of the asterisk problem in debug mode.
+*/
+
+#include <stdio.h>
+#include <sys/types.h>
+#include <unistd.h>
+#include <errno.h>
+#include <signal.h>
+
+int main(int argc, char **argv)
+{
+	pid_t pid;
+	int strm[2];
+	int retval;
+	if(argc < 2) return -2;
+
+	/* Normally, the decoder gets killed on SIGPIPE when wanting to write to STDOUT,
+	   but with this in place, it gets normal I/O error. */
+	signal(SIGPIPE, SIG_IGN);
+
+	pipe(strm);
+	pid = fork();
+	if(pid == 0)
+	{
+		close(strm[0]);
+		dup2(strm[1],STDOUT_FILENO);
+		fprintf(stderr, "starting decoder\n");
+		execlp("mpg123", "mpg123", "-s", "-q", argv[1], NULL);
+		fprintf(stderr, "Still here? That's bad.(%s)\n", strerror(errno));
+
+		return -1;
+	}
+	printf("Now fainting\n");
+	return 0;
+}