From d9d5839e16b29db8dd3ff242fb45802ad8be51d3 Mon Sep 17 00:00:00 2001 From: Celelibi Date: Sun, 9 Dec 2018 16:42:25 +0100 Subject: sdl_sound.cpp: Rework the circular buffer Previous code had several oddities. Like checks against buffer overflows / underflows done at several places in the code, usage of a spinlock, unnecessary use of "volatile" qualifier, redundant informations about the circular buffer and exposed details about the split nature of ranges in the buffer. This lead to an unlikely deadlock once in a great while. Signed-off-by: Celelibi --- src/osd/modules/sound/sdl_sound.cpp | 360 ++++++++++++++++-------------------- 1 file 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(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(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; } -- cgit v1.2.3