Some audio handling rework, mainly to clean up the mess with the buffer.

The initial reason was to install safeguards against multiple ao->close() operations, which was done, too.
Now either the buffer or the main mpg123 process touch the audio devices; in the buffered case the main program querying the buffer process for audio capabilities.
That and some other potential bug(s) fixed.

This is too much movement in a release candidate, but it is necessary - we want some quality for 1.0!



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

src/audio.c

@@ -21,6 +21,8 @@ audio_output_t* open_output_module( const char* name )
 	audio_output_t *ao = NULL;
 	int result = 0;
 
+	if(param.usebuffer || param.outmode != DECODE_AUDIO) return NULL;
+
 	/* Open the module */
 	module = open_module( "output", name );
 	if (module == NULL) return NULL;
@@ -42,6 +44,7 @@ audio_output_t* open_output_module( const char* name )
 	/* Call the init function */
 	ao->device = param.output_device;
 	ao->flags  = param.output_flags;
+	ao->is_open = FALSE;
 	result = module->init_output(ao);
 	if (result) {
 		error1( "Module's init function failed: %d", result );
@@ -60,12 +63,12 @@ audio_output_t* open_output_module( const char* name )
 /* Close the audio output and close the module */
 void close_output_module( audio_output_t* ao ) 
 {
-	if (!ao) return;
+	if (!ao) return; /* That covers buffer mode, too (ao == NULL there). */
 	
 	debug("closing output module");
 
 	/* Close the audio output */
-	/* No: we do that before, twice hurts. if (ao->close) ao->close( ao );*/
+	if(ao->is_open && ao->close != NULL) ao->close(ao);
 
 	/* Deinitialise the audio output */
 	if (ao->deinit) ao->deinit( ao );
@@ -165,10 +168,16 @@ void print_capabilities(audio_output_t *ao, mpg123_handle *mh)
 	size_t      num_rates;
 	const int  *encs;
 	size_t      num_encs;
+	const char *name = "<buffer>";
+	const char *dev  = "<none>";
+	if(!param.usebuffer)
+	{
+		name = ao->module->name;
+		if(ao->device != NULL) dev = ao->device;
+	}
 	mpg123_rates(&rates, &num_rates);
 	mpg123_encodings(&encs, &num_encs);
-	fprintf(stderr,"\nAudio driver: %s\nAudio device: %s\nAudio capabilities:\n(matrix of [S]tereo or [M]ono support for sample format and rate in Hz)\n        |",
-	        ao->module->name, ao->device != NULL ? ao->device : "<none>");
+	fprintf(stderr,"\nAudio driver: %s\nAudio device: %s\nAudio capabilities:\n(matrix of [S]tereo or [M]ono support for sample format and rate in Hz)\n        |", name, dev);
 	for(e=0;e<num_encs;e++) fprintf(stderr," %5s |",audio_encoding_name(encs[e], 0));
 	fprintf(stderr,"\n --------------------------------------------------------\n");
 	for(r=0; r<num_rates; ++r) capline(mh, rates[r]);
@@ -178,13 +187,17 @@ void print_capabilities(audio_output_t *ao, mpg123_handle *mh)
 	fprintf(stderr,"\n");
 }
 
+/* This uses the currently opened audio device, queries its caps.
+   In case of buffered playback, this works _once_ by querying the buffer for the caps before entering the main loop. */
 void audio_capabilities(audio_output_t *ao, mpg123_handle *mh)
 {
 	int fmts;
 	int ri;
-	audio_output_t ao1 = *ao; /* a copy */
+	long rate;
+	int channels;
 	const long *rates;
 	size_t      num_rates;
+	debug("audio_capabilities");
 	mpg123_rates(&rates, &num_rates);
 
 	if(param.outmode != DECODE_AUDIO)
@@ -195,21 +208,39 @@ void audio_capabilities(audio_output_t *ao, mpg123_handle *mh)
 
 	mpg123_format_none(mh); /* Start with nothing. */
 
-	/* If audio_open fails, the device is just not capable of anything... */
-	if(ao1.open(&ao1) < 0) error("failed to open audio device");
-	else
-	{
-		for(ao1.channels=1; ao1.channels<=2; ao1.channels++)
+	for(channels=1; channels<=2; channels++)
 	for(ri = param.force_rate>0 ? -1 : 0;ri<num_rates;ri++)
 	{
-			ao1.rate = ri >= 0 ? rates[ri] : param.force_rate;
-			fmts = ao1.get_formats(&ao1);
+		rate = ri >= 0 ? rates[ri] : param.force_rate;
+#ifndef NOXFERMEM
+		if(param.usebuffer)
+		{ /* Ask the buffer process. It is waiting for this. */
+			buffermem->rate     = rate; 
+			buffermem->channels = channels;
+			buffermem->format   = 0; /* Just have it initialized safely. */
+			debug2("asking for formats for %liHz/%ich", rate, channels);
+			xfermem_putcmd(buffermem->fd[XF_WRITER], XF_CMD_AUDIOCAP);
+			xfermem_getcmd(buffermem->fd[XF_WRITER], TRUE);
+			fmts = buffermem->format;
+		}
+		else
+#endif
+		{ /* Check myself. */
+			ao->rate     = rate;
+			ao->channels = channels;
+			fmts = ao->get_formats(ao);
+		}
+		debug1("got formats: 0x%x", fmts);
+
 		if(fmts < 0) continue;
-			else mpg123_format(mh, ao1.rate, ao1.channels, fmts);
-		}
-		ao1.close(&ao1);
+		else mpg123_format(mh, rate, channels, fmts);
 	}
 
+#ifndef NOXFERMEM
+	/* Buffer loop shall start normal operation now. */
+	if(param.usebuffer) xfermem_putcmd(buffermem->fd[XF_WRITER], XF_CMD_WAKEUP);
+#endif
+
 	if(param.verbose > 1) print_capabilities(ao, mh);
 }
 
@@ -225,7 +256,7 @@ static void catch_child(void)
 
 /* FIXME: Old output initialization code that needs updating */
 
-int init_output(audio_output_t *ao, mpg123_handle *mh)
+int init_output(audio_output_t **ao)
 {
 	static int init_done = FALSE;
 	
@@ -269,56 +300,69 @@ int init_output(audio_output_t *ao, mpg123_handle *mh)
 			error("cannot fork!");
 			return -1;
 			case 0: /* child */
-			/* oh, is that trouble here? well, buffer should actually be opened before loading tracks IMHO */
-			mpg123_close(mh); /* child doesn't need the input stream */
+			{
+				/* Buffer process handles all audio stuff itself. */
+				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.");
+					close_output_module(bao);
+					exit(2);
+				}
 				xfermem_init_reader (buffermem);
-			buffer_loop (ao, &oldsigset);
+				buffer_loop(bao, &oldsigset); /* Here the work happens. */
 				xfermem_done_reader (buffermem);
 				xfermem_done (buffermem);
+				close_output(bao);
+				close_output_module(bao);
 				exit(0);
+			}
 			default: /* parent */
 			xfermem_init_writer (buffermem);
-			param.outmode = DECODE_BUFFER;
 		}
 	}
 #endif
-	/* Open audio if not decoding to buffer */
-	switch(param.outmode) {
-		case DECODE_AUDIO:
-			if(ao->open(ao) < 0) {
-				error("failed to open audio device");
-				return 1;
+	if(param.outmode == DECODE_AUDIO && !param.usebuffer)
+	{ /* Only if I handle audio device output: Get that module. */
+		*ao = open_output_module(param.output_module);
+		if(!ao)
+		{
+			error("Failed to open audio output module");
+			return -1;
 		}
-		break;
-		case DECODE_WAV:
-			wav_open(ao,param.filename);
-		break;
-		case DECODE_AU:
-			au_open(ao,param.filename);
-		break;
-		case DECODE_CDR:
-			cdr_open(ao,param.filename);
-		break;
 	}
+	else *ao = NULL; /* That ensures we won't try to free it later... */
+	/* This has internal protection for buffer mode. */
+	if(open_output(*ao) < 0) return -1;
 
 	return 0;
 }
 
-void flush_output(int outmode, audio_output_t *ao, unsigned char *bytes, size_t count)
+void flush_output(audio_output_t *ao, unsigned char *bytes, size_t count)
 {
 	if(count)
 	{
-		switch(outmode)
+		/* Error checks? */
+		if(param.usebuffer) xfermem_write(buffermem, bytes, count);
+		else
+		switch(param.outmode)
 		{
-			case DECODE_FILE:
-				write (OutputDescriptor, bytes, count);
-			break;
 			case DECODE_AUDIO:
 				ao->write(ao, bytes, count);
 			break;
-			case DECODE_BUFFER:
-				error("The buffer doesn't work like that... I shouldn't ever be getting here.");
-				write (buffer_fd[1], bytes, count);
+			case DECODE_FILE:
+				write(OutputDescriptor, bytes, count);
 			break;
 			case DECODE_WAV:
 			case DECODE_CDR:
@@ -326,21 +370,60 @@ void flush_output(int outmode, audio_output_t *ao, unsigned char *bytes, size_t
 				wav_write(bytes, count);
 			break;
 		}
-		count = 0;
 	}
 }
 
-/* is this used? */
-void close_output(int outmode, audio_output_t *ao)
+int open_output(audio_output_t *ao)
 {
-	debug("closing output");
-    switch(outmode) {
+	if(param.usebuffer) return 0;
+
+	switch(param.outmode)
+	{
 		case DECODE_AUDIO:
-        ao->close(ao);
-        /* Module frees and closes its resources, but may not reset them. */
-        ao->userptr = NULL;
-        ao->fn = -1;
+			if(ao == NULL)
+			{
+				error("ao should not be NULL here!");
+				exit(110);
+			}
+			debug3("ao=%p, ao->is_open=%i, ao->open=%p", ao, ao->is_open, ao->open);
+			ao->is_open = ao->open(ao) < 0 ? FALSE : TRUE;
+			if(!ao->is_open)
+			{
+				error("failed to open audio device");
+				return -1;
+			}
+			else return 0;
 		break;
+		case DECODE_WAV:
+			return wav_open(ao,param.filename);
+		break;
+		case DECODE_AU:
+			return au_open(ao,param.filename);
+		break;
+		case DECODE_CDR:
+			return cdr_open(ao,param.filename);
+		break;
+	}
+	return -1; /* That's an error ... unknown outmode? */
+}
+
+/* is this used? */
+void close_output(audio_output_t *ao)
+{
+	if(param.usebuffer) return;
+
+	debug("closing output");
+	switch(param.outmode)
+	{
+		case DECODE_AUDIO:
+		/* Guard that close call; could be nasty. */
+		if(ao->is_open)
+		{
+			ao->is_open = FALSE;
+			if(ao->close != NULL) ao->close(ao);
+		}
+		break;
+		/* These are safe to be called too often. */
 		case DECODE_WAV:
 		wav_close();
 		break;
@@ -351,7 +434,6 @@ void close_output(int outmode, audio_output_t *ao)
 		cdr_close();
 		break;
 	}
-
 }
 
 /* Also for WAV decoding? */
@@ -359,8 +441,8 @@ int reset_output(audio_output_t *ao)
 {
 	if(param.outmode == DECODE_AUDIO)
 	{
-		close_output(param.outmode, ao);
-		return ao->open(ao);
+		close_output(ao);
+		return open_output(ao);
 	}
 	else return 0;
 }

src/audio.h

@@ -57,6 +57,7 @@ typedef struct audio_output_struct
 	long gain;		/* output gain */
 	int channels;	/* number of channels */
 	int format;		/* format flags */
+	int is_open;	/* something opened? */
 } audio_output_t;
 
 struct audio_format_name {
@@ -75,9 +76,10 @@ void audio_capabilities(audio_output_t *ao, mpg123_handle *mh);
 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, mpg123_handle *mh);
-void flush_output(int outmode, audio_output_t *ao, unsigned char *bytes, size_t count);
-void close_output(int mod, audio_output_t *ao );
+int init_output(audio_output_t **ao);
+void 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);
 
 #endif

src/buffer.c

@@ -104,10 +104,34 @@ void buffer_loop(audio_output_t *ao, sigset_t *oldsigset)
 	catchsignal (SIGINT, catch_interrupt);
 	catchsignal (SIGUSR1, catch_usr1);
 	sigprocmask (SIG_SETMASK, oldsigset, NULL);
-	if (param.outmode == DECODE_AUDIO) {
-		if (ao->open(ao) < 0) {
-			perror("audio");
-			exit(1);
+
+	if(param.outmode == DECODE_AUDIO)
+	{
+		debug("audio output: waiting for cap requests");
+		/* wait for audio setup queries */
+		while(1)
+		{
+			int cmd;
+			cmd = xfermem_block(XF_READER, xf);
+			if(cmd == XF_CMD_AUDIOCAP)
+			{
+				ao->rate     = xf->rate;
+				ao->channels = xf->channels;
+				ao->format   = ao->get_formats(ao);
+				debug3("formats for %liHz/%ich: 0x%x", ao->rate, ao->channels, ao->format);
+				xf->format = ao->format;
+				xfermem_putcmd(my_fd, XF_CMD_AUDIOCAP);
+			}
+			else if(cmd == XF_CMD_WAKEUP)
+			{
+				debug("got wakeup... leaving config mode");
+				break;
+			}
+			else
+			{
+				error1("unexpected command %i", cmd);
+				return;
+			}
 		}
 	}
 
@@ -141,9 +165,9 @@ void buffer_loop(audio_output_t *ao, sigset_t *oldsigset)
 			 */
 			if (xf->wakeme[XF_WRITER])
 				xfermem_putcmd(my_fd, XF_CMD_WAKEUP);
-			ao->rate = xf->buf[0]; 
-			ao->channels = xf->buf[1]; 
-			ao->format = xf->buf[2];
+			ao->rate = xf->rate; 
+			ao->channels = xf->channels; 
+			ao->format = xf->format;
 			if (reset_output(ao) < 0) {
 				error1("failed to reset audio: %s", strerror(errno));
 				exit(1);
@@ -188,10 +212,10 @@ void buffer_loop(audio_output_t *ao, sigset_t *oldsigset)
 						done=TRUE;
 						break;
 					case -1:
-						if(errno==EINTR)
+						if(errno==EINTR) /* Got signal, handle it at top of loop... */
 							continue;
 						if(errno)
-							perror("Yuck! Error in buffer handling...");
+							perror("Yuck! Error in buffer handling... or somewhere unexpected.");
 						done = TRUE;
 						xf->readindex = xf->freeindex;
 						xfermem_putcmd(xf->fd[XF_READER], XF_CMD_TERMINATE);
@@ -213,6 +237,7 @@ 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. */
 		if (param.outmode == DECODE_FILE)
 			bytes = write(OutputDescriptor, xf->data + xf->readindex, bytes);
 		else if (param.outmode == DECODE_AUDIO)
@@ -242,9 +267,6 @@ void buffer_loop(audio_output_t *ao, sigset_t *oldsigset)
 		if (xf->wakeme[XF_WRITER])
 			xfermem_putcmd(my_fd, XF_CMD_WAKEUP);
 	}
-
-	if (param.outmode == DECODE_AUDIO)
-		ao->close(ao);
 }
 
 #endif

src/mpg123.c

@@ -400,7 +400,7 @@ topt opts[] = {
  *   Change the playback sample rate.
  *   Consider that changing it after starting playback is not covered by gapless code!
  */
-static void reset_audio(void)
+static void reset_audio(long rate, int channels, int format)
 {
 #ifndef NOXFERMEM
 	if (param.usebuffer) {
@@ -418,9 +418,9 @@ static void reset_audio(void)
 		buffermem->freeindex = 0;
 		if (intflag)
 			return;
-		buffermem->buf[0] = ao->rate; 
-		buffermem->buf[1] = ao->channels; 
-		buffermem->buf[2] = ao->format;
+		buffermem->rate     = rate; 
+		buffermem->channels = channels; 
+		buffermem->format   = format;
 		buffer_reset();
 	}
 	else 
@@ -431,6 +431,14 @@ static void reset_audio(void)
 		 *   the device's internal buffer before
 		 *   changing the sample rate.   [OF]
 		 */
+		if(ao == NULL)
+		{
+			error("Audio handle should not be NULL here!");
+			safe_exit(98);
+		}
+		ao->rate     = rate; 
+		ao->channels = channels; 
+		ao->format   = format;
 		if(reset_output(ao) < 0)
 		{
 			error1("failed to reset audio device: %s", strerror(errno));
@@ -507,16 +515,8 @@ int play_frame(void)
 			if(param.verbose) print_header(mh);
 			else print_header_compact(mh);
 		}
-#ifndef NOXFERMEM
-		if(param.usebuffer)
-		{ /* Instead of directly decoding to buffer, we copy explicitly here and handle the wrapping!
-			   Just giving libmpg123 the buffermem is no good idea since it doesn't know about the ringbuffer. */
-			if(xfermem_write(buffermem, audio, bytes)) return 1;
-		}
-		else
-#endif
-		/* Normal flushing of data. */
-		flush_output(param.outmode, ao, audio, bytes);
+		/* Normal flushing of data, includes buffer decoding. */
+		flush_output(ao, audio, bytes);
 		if(param.checkrange)
 		{
 			long clip = mpg123_clip(mh);
@@ -538,20 +538,12 @@ int play_frame(void)
 		}
 		if(mc == MPG123_NEW_FORMAT)
 		{
-			int ret = 0;
-			mpg123_getformat(mh, &ao->rate, &ao->channels, &ao->format);
-			ret = init_output(ao, mh); /* This has to become an abstraction to defer the action to the buffer process if there! */
-			if(ret == 1)
-			{
-				warning("I am not sure if my code is ready to switch audio format during playback!");
-				reset_audio();
-				/* safe_exit(-1); */
-			}
-			if(ret < 0)
-			{
-				error1("init_output() returned %i!", ret);
-				safe_exit(1);
-			}
+			long rate;
+			int channels, format;
+			mpg123_getformat(mh, &rate, &channels, &format);
+			if(param.verbose > 2) fprintf(stderr, "Note: New output format %liHz %ich, format %i\n", rate, channels, format);
+
+			reset_audio(rate, channels, format);
 		}
 	}
 	return 1;
@@ -582,14 +574,16 @@ int main(int argc, char *argv[])
 			break;
 		}
 	}
-	httpdata_init(&htd);
+
+	/* Need to initialize mpg123 lib here for default parameter values. */
+
 	result = mpg123_init();
 	if(result != MPG123_OK)
 	{
 		error1("Cannot initialize mpg123 library: %s", mpg123_plain_strerror(result));
-		safe_exit(77);
+		exit(77);
 	}
-	mp = mpg123_new_pars(&result);
+	mp = mpg123_new_pars(&result); /* This may get leaked on premature exit(), which is mainly a cosmetic issue... */
 	if(mp == NULL)
 	{
 		error1("Crap! Cannot get mpg123 parameters: %s", mpg123_plain_strerror(result));
@@ -609,7 +603,6 @@ int main(int argc, char *argv[])
 #endif
 	mpg123_getpar(mp, MPG123_FLAGS, &parr, NULL);
 	param.flags = (int) parr;
-	bufferblock = mpg123_safe_buffer();
 
 #ifdef OS2
         _wildcard(&argc,&argv);
@@ -629,6 +622,23 @@ int main(int argc, char *argv[])
 			usage(1);
 	}
 
+	/* Init audio as early as possible.
+	   If there is the buffer process to be spawned, it shouldn't carry the mpg123_handle with it. */
+	bufferblock = mpg123_safe_buffer(); /* Can call that before mpg123_init(), it's stateless. */
+	if(init_output(&ao) < 0)
+	{
+		error("Failed to initialize output, goodbye.");
+		mpg123_delete_pars(mp);
+		exit(99); /* It's safe here... nothing nasty happened yet. */
+	}
+
+	/* ========================================================================================================= */
+	/* Enterning the leaking zone... we start messing with stuff here that should be taken care of when leaving. */
+	/* Don't just exit() or return out...                                                                        */
+	/* ========================================================================================================= */
+
+	httpdata_init(&htd);
+
 	if(param.list_cpu)
 	{
 		char **all_dec = mpg123_decoders();
@@ -701,14 +711,8 @@ int main(int argc, char *argv[])
 	}
 	mpg123_delete_pars(mp); /* Don't need the parameters anymore ,they're in the handle now. */
 
-	/* Open audio output module */
-	ao = open_output_module( param.output_module );
-	if (!ao) {
-		error("Failed to open audio output module.");
-		safe_exit(1);
-	}
-
-	audio_capabilities(ao, mh); /* Query audio output parameters, store in mpg123 handle. */
+	/* Now either check caps myself or query buffer for that. */
+	audio_capabilities(ao, mh);
 
 	if(equalfile != NULL)
 	{ /* tst; ThOr: not TRUE or FALSE: allocated or not... */
@@ -768,7 +772,7 @@ int main(int argc, char *argv[])
 	if(param.remote) {
 		int ret;
 		ret = control_generic(mh);
-		close_output(param.outmode, ao);
+		close_output(ao);
 		close_output_module(ao);
 		safe_exit(ret);
 	}
@@ -855,8 +859,6 @@ tc_hack:
 #ifndef NOXFERMEM
 				if (param.verbose > 1 || !(framenum & 0x7))
 					print_stat(mh,0,xfermem_get_usedspace(buffermem)); 
-				if(param.verbose > 2 && param.usebuffer)
-					fprintf(stderr,"[%08x %08x]", (unsigned int)buffermem->readindex, (unsigned int)buffermem->freeindex);
 #else
 				if(param.verbose > 1 || !(framenum & 0x7))	print_stat(mh,0,0);
 #endif
@@ -958,12 +960,9 @@ tc_hack:
       xfermem_done (buffermem);
     }
 #endif
-	/* Close the output */
-	close_output(param.outmode, ao);
-    
-    /* Close the audio output module */
-    close_output_module( ao );
-
+	/* 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);

src/xfermem.h

@@ -34,7 +34,9 @@ typedef struct {
 	byte *metadata;
 	size_t size;
 	size_t metasize;
-	int buf[3];
+	long rate;
+	int  channels;
+	int  format;
 } txfermem;
 /*
  *   [W] -- May be written to by the writing process only!
@@ -51,6 +53,7 @@ size_t xfermem_get_usedspace (txfermem *xf);
 #define XF_CMD_WAKEUP_INFO  0x04
 #define XF_CMD_WAKEUP    0x02
 #define XF_CMD_TERMINATE 0x03
+#define XF_CMD_AUDIOCAP  0x05
 #define XF_WRITER 0
 #define XF_READER 1
 int xfermem_getcmd (int fd, int block);