From b337e0ff6f3eab8df4fe6c82ef8a2b3e778236b9 Mon Sep 17 00:00:00 2001 From: rcoaxil Date: Tue, 18 Oct 2016 05:04:46 +0300 Subject: [PATCH 1/4] consistified seeking to invalid location behavior removed few redundant operations and checks moved sound start seeking from prepare to play fixed potential bug with sources that failed to play not releasing AL source back into the pool sources now stop if seeked to invalid location looped sources play from 0 if seeked to invalid location while playing --HG-- branch : minor --- src/modules/audio/openal/Source.cpp | 63 +++++++++++++++++++---------- 1 file changed, 42 insertions(+), 21 deletions(-) diff --git a/src/modules/audio/openal/Source.cpp b/src/modules/audio/openal/Source.cpp index 1de16979e..55148b78e 100644 --- a/src/modules/audio/openal/Source.cpp +++ b/src/modules/audio/openal/Source.cpp @@ -280,9 +280,6 @@ bool Source::update() switch (type) { case TYPE_STATIC: - // Looping mode could have changed. - // FIXME: make looping mode not change without you noticing so that this is not needed - alSourcei(source, AL_LOOPING, isLooping() ? AL_TRUE : AL_FALSE); return !isFinished(); case TYPE_STREAM: if (!isFinished()) @@ -401,24 +398,26 @@ void Source::seekAtomic(float offset, void *unit) break; } + bool wasPlaying = isPlaying(); switch (type) { case TYPE_STATIC: - alSourcef(source, AL_SAMPLE_OFFSET, offsetSamples); - offsetSamples = offsetSeconds = 0; + if (valid) + { + alSourcef(source, AL_SAMPLE_OFFSET, offsetSamples); + offsetSamples = offsetSeconds = 0; + } break; case TYPE_STREAM: { - bool wasPlaying = isPlaying(); - // To drain all buffers if (valid) - stopAtomic(); + stop(); decoder->seek(offsetSeconds); if (wasPlaying) - playAtomic(source); + play(); break; } @@ -454,6 +453,13 @@ void Source::seekAtomic(float offset, void *unit) case TYPE_MAX_ENUM: break; } + if (wasPlaying && (alGetError() == AL_INVALID_VALUE || (type == TYPE_STREAM && !isPlaying()))) + { + stop(); + if (isLooping()) + play(); + return; + } this->offsetSamples = offsetSamples; this->offsetSeconds = offsetSeconds; } @@ -734,9 +740,6 @@ void Source::prepareAtomic() { case TYPE_STATIC: alSourcei(source, AL_BUFFER, staticBuffer->getBuffer()); - //source can be seeked while not valid - if (offsetSamples >= 0) - alSourcef(source, AL_SAMPLE_OFFSET, offsetSamples); break; case TYPE_STREAM: while (unusedBufferPeek() != AL_NONE) @@ -760,8 +763,6 @@ void Source::prepareAtomic() for (unsigned int i = top + 1; i < MAX_BUFFERS; i++) unusedBufferPush(unusedBuffers[i]); - if (offsetSamples >= 0) - alSourcef(source, AL_SAMPLE_OFFSET, offsetSamples); break; } case TYPE_MAX_ENUM: @@ -774,7 +775,6 @@ void Source::teardownAtomic() switch (type) { case TYPE_STATIC: - alSourcef(source, AL_SAMPLE_OFFSET, 0); break; case TYPE_STREAM: { @@ -832,16 +832,32 @@ bool Source::playAtomic(ALuint source) alSourcePlay(source); - // alSourcePlay may fail if the system has reached its limit of simultaneous - // playing sources. bool success = alGetError() == AL_NO_ERROR; - valid = true; //if it fails it will be set to false again - //but this prevents a horrible, horrible bug + if (type == TYPE_STREAM) + { + valid = true; //isPlaying() needs source to be valid + if (!isPlaying()) + success = false; + } + else if (success) + { + alSourcef(source, AL_SAMPLE_OFFSET, offsetSamples); + success = alGetError() == AL_NO_ERROR; + } - if (type != TYPE_STREAM) + if (!success) + { + valid = true; //stop() needs source to be valid + stop(); + } + else if (type != TYPE_STREAM) offsetSamples = offsetSeconds = 0; + //this is set to success state afterwards anyway, but setting it here + //to true preemptively avoids race condition with update bug + valid = true; + return success; } @@ -862,7 +878,12 @@ void Source::pauseAtomic() void Source::resumeAtomic() { if (valid && !isPlaying()) + { alSourcePlay(source); + + if (alGetError() == AL_INVALID_VALUE || (type == TYPE_STREAM && unusedBufferTop == MAX_BUFFERS - 1)) + stop(); + } } bool Source::playAtomic(const std::vector &sources, const std::vector &ids, const std::vector &wasPlaying) @@ -941,7 +962,7 @@ void Source::pauseAtomic(const std::vector &sources) void Source::reset() { - alSourcei(source, AL_BUFFER, 0); + alSourcei(source, AL_BUFFER, AL_NONE); alSourcefv(source, AL_POSITION, position); alSourcefv(source, AL_VELOCITY, velocity); alSourcefv(source, AL_DIRECTION, direction); From 9bed2ac33a3c5d6dfa0484ebaf07fe9c5728a15e Mon Sep 17 00:00:00 2001 From: rcoaxil Date: Tue, 18 Oct 2016 05:33:34 +0300 Subject: [PATCH 2/4] moved back update cycle constant looping override for static sources --HG-- branch : minor --- src/libraries/stb/stb_image.h | 5 +++++ src/modules/audio/openal/Source.cpp | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/src/libraries/stb/stb_image.h b/src/libraries/stb/stb_image.h index fd4c872bc..93600d783 100644 --- a/src/libraries/stb/stb_image.h +++ b/src/libraries/stb/stb_image.h @@ -712,9 +712,14 @@ static int stbi__sse2_available() static int stbi__sse2_available() { +/* #if defined(__GNUC__) && (__GNUC__ * 100 + __GNUC_MINOR__) >= 408 // GCC 4.8 or later // GCC 4.8+ has a nice way to do this return __builtin_cpu_supports("sse2"); +*/ +#if defined(STBI__X64_TARGET) + // GCC 5 has this function buggy for a moment, but an x64 CPU can be assumed to have SSE2 anyway + return 1; #else // portable way to do this, preferably without using GCC inline ASM? // just bail for now. diff --git a/src/modules/audio/openal/Source.cpp b/src/modules/audio/openal/Source.cpp index 55148b78e..e17f80b80 100644 --- a/src/modules/audio/openal/Source.cpp +++ b/src/modules/audio/openal/Source.cpp @@ -280,7 +280,12 @@ bool Source::update() switch (type) { case TYPE_STATIC: + { + // Looping mode could have changed. +- // FIXME: make looping mode change atomically so this is not needed + alSourcei(source, AL_LOOPING, isLooping() ? AL_TRUE : AL_FALSE); return !isFinished(); + } case TYPE_STREAM: if (!isFinished()) { From 329e10d35b06fd8a0ecc78dd99de960f35dfd6c6 Mon Sep 17 00:00:00 2001 From: rcoaxil Date: Tue, 18 Oct 2016 05:36:28 +0300 Subject: [PATCH 3/4] stb hack revert --HG-- branch : minor --- src/libraries/stb/stb_image.h | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/libraries/stb/stb_image.h b/src/libraries/stb/stb_image.h index 93600d783..fd4c872bc 100644 --- a/src/libraries/stb/stb_image.h +++ b/src/libraries/stb/stb_image.h @@ -712,14 +712,9 @@ static int stbi__sse2_available() static int stbi__sse2_available() { -/* #if defined(__GNUC__) && (__GNUC__ * 100 + __GNUC_MINOR__) >= 408 // GCC 4.8 or later // GCC 4.8+ has a nice way to do this return __builtin_cpu_supports("sse2"); -*/ -#if defined(STBI__X64_TARGET) - // GCC 5 has this function buggy for a moment, but an x64 CPU can be assumed to have SSE2 anyway - return 1; #else // portable way to do this, preferably without using GCC inline ASM? // just bail for now. From 3c413a293b7edea05a200c863b253d9f7592bc50 Mon Sep 17 00:00:00 2001 From: rcoaxil Date: Wed, 19 Oct 2016 05:13:30 +0300 Subject: [PATCH 4/4] stray minus sign --HG-- branch : minor --- src/modules/audio/openal/Source.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/modules/audio/openal/Source.cpp b/src/modules/audio/openal/Source.cpp index e17f80b80..093b0fd0c 100644 --- a/src/modules/audio/openal/Source.cpp +++ b/src/modules/audio/openal/Source.cpp @@ -282,7 +282,7 @@ bool Source::update() case TYPE_STATIC: { // Looping mode could have changed. -- // FIXME: make looping mode change atomically so this is not needed + // FIXME: make looping mode change atomically so this is not needed alSourcei(source, AL_LOOPING, isLooping() ? AL_TRUE : AL_FALSE); return !isFinished(); }