Merge pull request #4385 from Celelibi/sdl-sound-underflow-deadlock1

sdl_sound.cpp: Rework the circular buffer

1 files changed, 164 insertions, 196 deletions

diff --git a/src/osd/modules/sound/sdl_sound.cpp b/src/osd/modules/sound/sdl_sound.cpp
index ad38fab4bab..f6dadca0026 100644
--- a/src/osd/modules/sound/sdl_sound.cpp
+++ b/src/osd/modules/sound/sdl_sound.cpp
@@ -38,6 +38,23 @@ static void sdl_callback(void *userdata, Uint8 *stream, int len);
 //  CLASS
 //============================================================
 
+class ring_buffer
+{
+public:
+	ring_buffer(size_t size);
+	virtual ~ring_buffer();
+
+	size_t data_size();
+	size_t free_size();
+	int append(const void *data, size_t size);
+	int pop(void *data, size_t size);
+
+private:
+	int8_t *buffer;
+	size_t buffer_size;
+	int head, tail;
+};
+
 class sound_sdl : public osd_module, public sound_module
 {
 public:
@@ -51,7 +68,7 @@ public:
 	: osd_module(OSD_SOUND_PROVIDER, "sdl"), sound_module(),
 		stream_in_initialized(0),
 		stream_loop(0),
-		attenuation(0), buf_locked(0), stream_buffer(nullptr), stream_playpos(0), stream_buffer_size(0), stream_buffer_in(0), buffer_underflows(0), buffer_overflows(0)
+		attenuation(0), buf_locked(0), stream_buffer(nullptr), stream_buffer_size(0), buffer_underflows(0), buffer_overflows(0)
 {
 		sdl_xfer_samples = SDL_XFER_SAMPLES;
 	}
@@ -66,9 +83,9 @@ public:
 	virtual void set_mastervolume(int attenuation) override;
 
 private:
-	int lock_buffer(bool is_throttled, long offset, long size, void **buffer1, long *length1, void **buffer2, long *length2);
+	int lock_buffer(void);
 	void unlock_buffer(void);
-	void att_memcpy(void *dest, const int16_t *data, int bytes_to_copy);
+	void attenuate(int16_t *data, int bytes);
 	void copy_sample_data(bool is_throttled, const int16_t *data, int bytes_to_copy);
 	int sdl_create_buffers(void);
 	void sdl_destroy_buffers(void);
@@ -79,12 +96,9 @@ private:
 	int attenuation;
 
 	int              buf_locked;
+	ring_buffer      *stream_buffer;
+	uint32_t         stream_buffer_size;
 
-	int8_t             *stream_buffer;
-	volatile int32_t   stream_playpos;
-
-	uint32_t           stream_buffer_size;
-	uint32_t           stream_buffer_in;
 
 	// buffer over/underflow counts
 	int              buffer_underflows;
@@ -108,58 +122,109 @@ private:
 // debugging
 static FILE *sound_log;
 
+
 //============================================================
-//  sound_sdl - destructor
+//  ring_buffer - constructor
 //============================================================
 
+ring_buffer::ring_buffer(size_t size)
+: buffer(global_alloc_array_clear<int8_t>(size + 1)), buffer_size(size + 1), head(0), tail(0)
+{
+	// A size+1 bytes buffer is allocated because it can never be full.
+	// Otherwise the case head == tail couldn't be distinguished between a
+	// full buffer and an empty buffer.
+}
+
 //============================================================
-//  lock_buffer
+//  ring_buffer - destructor
 //============================================================
-int sound_sdl::lock_buffer(bool is_throttled, long offset, long size, void **buffer1, long *length1, void **buffer2, long *length2)
+
+ring_buffer::~ring_buffer()
 {
-	volatile long pstart, pend, lstart, lend;
+	global_free_array(buffer);
+}
 
-	if (!buf_locked)
-	{
-		if (is_throttled)
-		{
-			pstart = stream_playpos;
-			pend = (pstart + sdl_xfer_samples);
-			lstart = offset;
-			lend = lstart+size;
-			while (((pstart >= lstart) && (pstart <= lend)) ||
-					((pend >= lstart) && (pend <= lend)))
-			{
-				pstart = stream_playpos;
-				pend = pstart + sdl_xfer_samples;
-			}
-		}
+//============================================================
+//  ring_buffer::data_size
+//============================================================
 
-		SDL_LockAudio();
-		buf_locked++;
-	}
+size_t ring_buffer::data_size()
+{
+	return (tail - head + buffer_size) % buffer_size;
+}
+
+//============================================================
+//  ring_buffer::free_size
+//============================================================
 
-	// init lengths
-	*length1 = *length2 = 0;
+size_t ring_buffer::free_size() {
+	return (head - tail - 1 + buffer_size) % buffer_size;
+}
 
-	if ((offset + size) > stream_buffer_size)
-	{
-		// 2-piece case
-		*length1 = stream_buffer_size - offset;
-		*buffer1 = &stream_buffer[offset];
-		*length2 = size - *length1;
-		*buffer2 = stream_buffer;
-	}
+//============================================================
+//  ring_buffer::append
+//============================================================
+
+int ring_buffer::append(const void *data, size_t size)
+{
+	int8_t *data8 = (int8_t *)data;
+	size_t sz;
+
+	if (free_size() < size)
+		return -1;
+
+	sz = buffer_size - tail;
+	if (size <= sz)
+		sz = size;
 	else
-	{
-		// normal 1-piece case
-		*length1 = size;
-		*buffer1 = &stream_buffer[offset];
+		memcpy(buffer, &data8[sz], size - sz);
+
+	memcpy(&buffer[tail], data8, sz);
+	tail = (tail + size) % buffer_size;
+	return 0;
+}
+
+//============================================================
+//  ring_buffer::pop
+//============================================================
+
+int ring_buffer::pop(void *data, size_t size)
+{
+	int8_t *data8 = (int8_t *)data;
+	size_t sz;
+
+	if (data_size() < size)
+		return -1;
+
+	sz = buffer_size - head;
+	if (size <= sz)
+		sz = size;
+	else {
+		memcpy(&data8[sz], buffer, size - sz);
+		memset(buffer, 0, size - sz);
 	}
 
+	memcpy(data8, &buffer[head], sz);
+	memset(&buffer[head], 0, sz);
+	head = (head + size) % buffer_size;
+	return 0;
+}
+
+//============================================================
+//  sound_sdl - destructor
+//============================================================
+
+//============================================================
+//  lock_buffer
+//============================================================
+int sound_sdl::lock_buffer()
+{
+	if (!buf_locked)
+		SDL_LockAudio();
+	buf_locked++;
+
 	if (LOG_SOUND)
-		fprintf(sound_log, "locking %ld bytes at offset %ld (total %d, playpos %d): len1 %ld len2 %ld\n",
-			size, offset, stream_buffer_size, stream_playpos, *length1, *length2);
+		fprintf(sound_log, "locking\n");
 
 	return 0;
 }
@@ -182,14 +247,14 @@ void sound_sdl::unlock_buffer(void)
 //  Apply attenuation
 //============================================================
 
-void sound_sdl::att_memcpy(void *dest, const int16_t *data, int bytes_to_copy)
+void sound_sdl::attenuate(int16_t *data, int bytes_to_copy)
 {
-	int level= (int) (pow(10.0, (double) attenuation / 20.0) * 128.0);
-	int16_t *d = (int16_t *) dest;
-	int count = bytes_to_copy/2;
-	while (count>0)
+	int level = (int) (pow(10.0, (double) attenuation / 20.0) * 128.0);
+	int count = bytes_to_copy / sizeof(*data);
+	while (count > 0)
 	{
-		*d++ = (*data++ * level) >> 7; /* / 128 */
+		*data = (*data * level) >> 7; /* / 128 */
+		data++;
 		count--;
 	}
 }
@@ -200,45 +265,12 @@ void sound_sdl::att_memcpy(void *dest, const int16_t *data, int bytes_to_copy)
 
 void sound_sdl::copy_sample_data(bool is_throttled, const int16_t *data, int bytes_to_copy)
 {
-	void *buffer1, *buffer2 = (void *)nullptr;
-	long length1, length2;
-	int cur_bytes;
-
-	// attempt to lock the stream buffer
-	if (lock_buffer(is_throttled, stream_buffer_in, bytes_to_copy, &buffer1, &length1, &buffer2, &length2) < 0)
-	{
-		buffer_underflows++;
-		return;
-	}
-
-	// adjust the input pointer
-	stream_buffer_in += bytes_to_copy;
-	if (stream_buffer_in >= stream_buffer_size)
-	{
-		stream_buffer_in -= stream_buffer_size;
-		stream_loop = 1;
-
-		if (LOG_SOUND)
-			fprintf(sound_log, "stream_loop set to 1 (stream_buffer_in looped)\n");
-	}
-
-	// copy the first chunk
-	cur_bytes = (bytes_to_copy > length1) ? length1 : bytes_to_copy;
-	att_memcpy(buffer1, data, cur_bytes);
-
-	// adjust for the number of bytes
-	bytes_to_copy -= cur_bytes;
-	data = (int16_t *)((uint8_t *)data + cur_bytes);
-
-	// copy the second chunk
-	if (bytes_to_copy != 0)
-	{
-		cur_bytes = (bytes_to_copy > length2) ? length2 : bytes_to_copy;
-		att_memcpy(buffer2, data, cur_bytes);
-	}
-
-	// unlock
+	lock_buffer();
+	int err = stream_buffer->append((void *)data, bytes_to_copy);
 	unlock_buffer();
+
+	if (LOG_SOUND && err)
+		fprintf(sound_log, "Late detection of overflow. This shouldn't happen.\n");
 }
 
 
@@ -249,77 +281,40 @@ void sound_sdl::copy_sample_data(bool is_throttled, const int16_t *data, int byt
 void sound_sdl::update_audio_stream(bool is_throttled, const int16_t *buffer, int samples_this_frame)
 {
 	// if nothing to do, don't do it
-	if (sample_rate() != 0 && stream_buffer)
-	{
-		int bytes_this_frame = samples_this_frame * sizeof(int16_t) * 2;
-		int play_position, write_position, stream_in;
-		int orig_write; // used in LOG
-
-		play_position = stream_playpos;
-
-		write_position = stream_playpos + ((sample_rate() / 50) * sizeof(int16_t) * 2);
-		orig_write = write_position;
-
-		if (!stream_in_initialized)
-		{
-			stream_in = stream_buffer_in = (write_position + stream_buffer_size) / 2;
-
-			if (LOG_SOUND)
-			{
-				fprintf(sound_log, "stream_in = %d\n", (int)stream_in);
-				fprintf(sound_log, "stream_playpos = %d\n", (int)stream_playpos);
-				fprintf(sound_log, "write_position = %d\n", (int)write_position);
-			}
-			// start playing
-			SDL_PauseAudio(0);
-
-			stream_in_initialized = 1;
-		}
-		else
-		{
-			// normalize the stream in position
-			stream_in = stream_buffer_in;
-			if (stream_in < write_position && stream_loop == 1)
-				stream_in += stream_buffer_size;
-
-			// now we should have, in order:
-			//    <------pp---wp---si--------------->
-
-			// if we're between play and write positions, then bump forward, but only in full chunks
-			while (stream_in < write_position)
-			{
-				if (LOG_SOUND)
-					fprintf(sound_log, "Underflow: PP=%d  WP=%d(%d)  SI=%d(%d)  BTF=%d\n", (int)play_position, (int)write_position, (int)orig_write, (int)stream_in, (int)stream_buffer_in, (int)bytes_this_frame);
-
-				buffer_underflows++;
-				stream_in += bytes_this_frame;
-			}
-
-			// if we're going to overlap the play position, just skip this chunk
-			if (stream_in + bytes_this_frame > play_position + stream_buffer_size)
-			{
-				if (LOG_SOUND)
-					fprintf(sound_log, "Overflow: PP=%d  WP=%d(%d)  SI=%d(%d)  BTF=%d\n", (int)play_position, (int)write_position, (int)orig_write, (int)stream_in, (int)stream_buffer_in, (int)bytes_this_frame);
-
-				buffer_overflows++;
-				return;
-			}
-		}
+	if (sample_rate() == 0 || stream_buffer == nullptr)
+		return;
 
-		if (stream_in >= stream_buffer_size)
-		{
-			stream_in -= stream_buffer_size;
-			stream_loop = 1;
 
-			if (LOG_SOUND)
-				fprintf(sound_log, "stream_loop set to 1 (stream_in looped)\n");
+	if (!stream_in_initialized)
+	{
+		// Fill in some zeros to prevent an initial buffer underflow
+		int8_t zero = 0;
+		size_t zsize = stream_buffer->free_size() / 2;
+		while (zsize--)
+			stream_buffer->append(&zero, 1);
+
+		// start playing
+		SDL_PauseAudio(0);
+		stream_in_initialized = 1;
+	}
 
-		}
+	size_t bytes_this_frame = samples_this_frame * sizeof(*buffer) * 2;
+	size_t free_size = stream_buffer->free_size();
+	size_t data_size = stream_buffer->data_size();
 
-		// now we know where to copy; let's do it
-		stream_buffer_in = stream_in;
-		copy_sample_data(is_throttled, buffer, bytes_this_frame);
+	if (stream_buffer->free_size() < bytes_this_frame) {
+		if (LOG_SOUND)
+			fprintf(sound_log, "Overflow: DS=%lu FS=%lu BTF=%lu\n", data_size, free_size, bytes_this_frame);
+		buffer_overflows++;
+		return;
 	}
+
+	copy_sample_data(is_throttled, buffer, bytes_this_frame);
+
+	size_t nfree_size = stream_buffer->free_size();
+	size_t ndata_size = stream_buffer->data_size();
+	if (LOG_SOUND)
+		fprintf(sound_log, "Appended data: DS=%lu(%lu) FS=%lu(%lu) BTF=%lu\n", data_size, ndata_size, free_size, nfree_size, bytes_this_frame);
 }
 
 
@@ -348,54 +343,28 @@ void sound_sdl::set_mastervolume(int _attenuation)
 static void sdl_callback(void *userdata, Uint8 *stream, int len)
 {
 	sound_sdl *thiz = (sound_sdl *) userdata;
-	int len1, len2, sb_in;
+	size_t free_size = thiz->stream_buffer->free_size();
+	size_t data_size = thiz->stream_buffer->data_size();
 
-	sb_in = thiz->stream_buffer_in;
-	if (thiz->stream_loop)
-		sb_in += thiz->stream_buffer_size;
-
-	if (sb_in < (thiz->stream_playpos+len))
+	if (data_size < len)
 	{
+		thiz->buffer_underflows++;
 		if (LOG_SOUND)
-			fprintf(sound_log, "Underflow at sdl_callback: SPP=%d SBI=%d(%d) Len=%d\n", (int)thiz->stream_playpos, (int)sb_in, (int)thiz->stream_buffer_in, (int)len);
+			fprintf(sound_log, "Underflow at sdl_callback: DS=%lu FS=%lu Len=%d\n", data_size, free_size, len);
 
+		// Maybe read whatever is left in the stream_buffer anyway?
 		memset(stream, 0, len);
 		return;
 	}
-	else if ((thiz->stream_playpos+len) > thiz->stream_buffer_size)
-	{
-		len1 = thiz->stream_buffer_size - thiz->stream_playpos;
-		len2 = len - len1;
-	}
-	else
-	{
-		len1 = len;
-		len2 = 0;
-	}
 
-	memcpy(stream, thiz->stream_buffer + thiz->stream_playpos, len1);
-	memset(thiz->stream_buffer + thiz->stream_playpos, 0, len1); // no longer needed
-	if (len2)
-	{
-		memcpy(stream+len1, thiz->stream_buffer, len2);
-		memset(thiz->stream_buffer, 0, len2); // no longer needed
-	}
-
-
-	// move the play cursor
-	thiz->stream_playpos += len1 + len2;
-	if (thiz->stream_playpos >= thiz->stream_buffer_size)
-	{
-		thiz->stream_playpos -= thiz->stream_buffer_size;
-		thiz->stream_loop = 0;
+	int err = thiz->stream_buffer->pop((void *)stream, len);
+	if (LOG_SOUND && err)
+		fprintf(sound_log, "Late detection of underflow. This shouldn't happen.\n");
 
-		if (LOG_SOUND)
-			fprintf(sound_log, "stream_loop set to 0 (stream_playpos looped)\n");
-	}
+	thiz->attenuate((int16_t *)stream, len);
 
 	if (LOG_SOUND)
-		fprintf(sound_log, "callback: xfer len1 %d len2 %d, playpos %d\n",
-				len1, len2, thiz->stream_playpos);
+		fprintf(sound_log, "callback: xfer DS=%lu FS=%lu Len=%d\n", data_size, free_size, len);
 }
 
 
@@ -515,8 +484,7 @@ int sound_sdl::sdl_create_buffers(void)
 {
 	osd_printf_verbose("sdl_create_buffers: creating stream buffer of %u bytes\n", stream_buffer_size);
 
-	stream_buffer = global_alloc_array_clear<int8_t>(stream_buffer_size);
-	stream_playpos = 0;
+	stream_buffer = new ring_buffer(stream_buffer_size);
 	buf_locked = 0;
 	return 0;
 }
@@ -529,7 +497,7 @@ void sound_sdl::sdl_destroy_buffers(void)
 {
 	// release the buffer
 	if (stream_buffer)
-		global_free_array(stream_buffer);
+		delete stream_buffer;
 	stream_buffer = nullptr;
 }