From 9937476e187d9ddca5c8fb54e6727ee4a5ee6a13 Mon Sep 17 00:00:00 2001 From: Leon Styhre Date: Tue, 16 Nov 2021 17:49:05 +0100 Subject: [PATCH] Replaced some mutex locks with atomic variables. Also removed an SDL audio issue workaround from AudioManager. --- es-core/src/AudioManager.cpp | 39 ---------- es-core/src/AudioManager.h | 17 ++--- es-core/src/Sound.cpp | 6 -- es-core/src/Sound.h | 11 +-- es-core/src/Window.cpp | 16 +---- es-core/src/Window.h | 5 +- es-core/src/components/VideoComponent.cpp | 15 ---- es-core/src/components/VideoComponent.h | 9 ++- .../src/components/VideoFFmpegComponent.cpp | 71 ++++++------------- es-core/src/components/VideoFFmpegComponent.h | 9 +-- es-core/src/resources/TextureData.cpp | 8 +-- es-core/src/resources/TextureData.h | 3 +- es-core/src/resources/TextureDataManager.cpp | 8 +-- es-core/src/resources/TextureDataManager.h | 3 +- 14 files changed, 51 insertions(+), 169 deletions(-) diff --git a/es-core/src/AudioManager.cpp b/es-core/src/AudioManager.cpp index 9c986aa0e..ec1d18a0f 100644 --- a/es-core/src/AudioManager.cpp +++ b/es-core/src/AudioManager.cpp @@ -194,9 +194,7 @@ void AudioManager::mixAudio(void* /*unused*/, Uint8* stream, int len) // stream is not played when the video player has been stopped. Otherwise there would // be a short time period when the audio would keep playing after the video was stopped // and before the stream was cleared in clearStream(). - std::unique_lock audioLock{mAudioLock}; bool muteStream = sMuteStream; - audioLock.unlock(); if (muteStream) { SDL_MixAudioFormat(stream, &converted.at(0), sAudioFormat.format, processedLength, 0); } @@ -290,41 +288,4 @@ void AudioManager::clearStream() SDL_LockAudioDevice(sAudioDevice); SDL_AudioStreamClear(sConversionStream); SDL_UnlockAudioDevice(sAudioDevice); - - // TEMPORARY: For evaluating if SDL_AudioStreamClear is stable. - return; - - // If sSoundVector is empty it means we are shutting down. In this case don't attempt - // to clear the stream as this could lead to a crash. - if (sSoundVector.empty()) - return; - - SDL_LockAudioDevice(sAudioDevice); - - // This code is required as there's seemingly a bug in SDL_AudioStreamAvailable(). - // The function sometimes returns 0 even if there is data left in the buffer, possibly - // because the remaining data is less than the configured sample size. It happens almost - // permanently on NetBSD but also on at least Linux from time to time. Adding some data - // to the stream buffer to get above this threshold before calling the function will - // return the proper number. So adding 10000 as we do here would give a return value of - // for instance 10880 instead of 0, assuming there were 880 bytes of data left in the buffer. - // Fortunately the SDL_AudioStreamGet() function acts correctly on any arbitrary sample size - // so we can actually clear the entire buffer. If this workaround was not implemented, there - // would be a sound glitch when some samples from the previous video would play any time a - // new video was started (assuming the issue was triggered by some remaining stream data). - std::vector writeBuffer(10000); - if (SDL_AudioStreamPut(sConversionStream, reinterpret_cast(&writeBuffer.at(0)), - 10000) == -1) { - LOG(LogError) << "Failed to put samples in the conversion stream:"; - LOG(LogError) << SDL_GetError(); - SDL_UnlockAudioDevice(sAudioDevice); - return; - } - - int length = SDL_AudioStreamAvailable(sConversionStream); - - std::vector readBuffer(length); - SDL_AudioStreamGet(sConversionStream, static_cast(&readBuffer.at(0)), length); - - SDL_UnlockAudioDevice(sAudioDevice); } diff --git a/es-core/src/AudioManager.h b/es-core/src/AudioManager.h index 76363f95d..cca5b06a0 100644 --- a/es-core/src/AudioManager.h +++ b/es-core/src/AudioManager.h @@ -10,8 +10,8 @@ #define ES_CORE_AUDIO_MANAGER_H #include +#include #include -#include #include class Sound; @@ -36,16 +36,8 @@ public: void processStream(const void* samples, unsigned count); void clearStream(); - void muteStream() - { - std::unique_lock audioLock{mAudioLock}; - sMuteStream = true; - } - void unmuteStream() - { - std::unique_lock audioLock{mAudioLock}; - sMuteStream = false; - } + void muteStream() { sMuteStream = true; } + void unmuteStream() { sMuteStream = false; } bool getHasAudioDevice() { return sHasAudioDevice; } @@ -58,10 +50,9 @@ private: static void mixAudio(void* unused, Uint8* stream, int len); static void mixAudio2(void* unused, Uint8* stream, int len); - inline static std::mutex mAudioLock; inline static SDL_AudioStream* sConversionStream; inline static std::vector> sSoundVector; - inline static bool sMuteStream = false; + inline static std::atomic sMuteStream = false; inline static bool sHasAudioDevice = true; }; diff --git a/es-core/src/Sound.cpp b/es-core/src/Sound.cpp index bbd7f4d97..0ece4f5c1 100644 --- a/es-core/src/Sound.cpp +++ b/es-core/src/Sound.cpp @@ -151,8 +151,6 @@ void Sound::play() SDL_LockAudioDevice(AudioManager::sAudioDevice); - std::unique_lock playerLock(mMutex); - if (mPlaying) // Replay from start. rewind the sample to the beginning. mSamplePos = 0; @@ -160,8 +158,6 @@ void Sound::play() // Flag our sample as playing. mPlaying = true; - playerLock.unlock(); - SDL_UnlockAudioDevice(AudioManager::sAudioDevice); // Tell the AudioManager to start playing samples. AudioManager::getInstance().play(); @@ -171,7 +167,6 @@ void Sound::stop() { // Flag our sample as not playing and rewind its position. SDL_LockAudioDevice(AudioManager::sAudioDevice); - std::unique_lock playerLock(mMutex); mPlaying = false; mSamplePos = 0; SDL_UnlockAudioDevice(AudioManager::sAudioDevice); @@ -182,7 +177,6 @@ void Sound::setPosition(Uint32 newPosition) mSamplePos = newPosition; if (mSamplePos >= mSampleLength) { // Got to or beyond the end of the sample. stop playing. - std::unique_lock playerLock(mMutex); mPlaying = false; mSamplePos = 0; } diff --git a/es-core/src/Sound.h b/es-core/src/Sound.h index 772e38271..30f52e804 100644 --- a/es-core/src/Sound.h +++ b/es-core/src/Sound.h @@ -11,9 +11,9 @@ #define ES_CORE_SOUND_H #include +#include #include #include -#include #include #include #include @@ -31,11 +31,7 @@ public: void loadFile(const std::string& path); void play(); - bool isPlaying() const - { - std::unique_lock playerLock(mMutex); - return mPlaying; - } + bool isPlaying() const { return mPlaying; } void stop(); const Uint8* getData() const { return mSampleData; } @@ -51,14 +47,13 @@ public: private: Sound(const std::string& path = ""); - inline static std::mutex mMutex; static std::map> sMap; std::string mPath; SDL_AudioSpec mSampleFormat; Uint8* mSampleData; Uint32 mSamplePos; Uint32 mSampleLength; - bool mPlaying; + std::atomic mPlaying; }; enum NavigationSoundsID { diff --git a/es-core/src/Window.cpp b/es-core/src/Window.cpp index 9e68784bc..0e1175b69 100644 --- a/es-core/src/Window.cpp +++ b/es-core/src/Window.cpp @@ -799,26 +799,14 @@ void Window::closeLaunchScreen() mRenderLaunchScreen = false; } -void Window::increaseVideoPlayerCount() -{ - mVideoCountMutex.lock(); - mVideoPlayerCount++; - mVideoCountMutex.unlock(); -} +void Window::increaseVideoPlayerCount() { mVideoPlayerCount++; } -void Window::decreaseVideoPlayerCount() -{ - mVideoCountMutex.lock(); - mVideoPlayerCount--; - mVideoCountMutex.unlock(); -} +void Window::decreaseVideoPlayerCount() { mVideoPlayerCount--; } int Window::getVideoPlayerCount() { int videoPlayerCount; - mVideoCountMutex.lock(); videoPlayerCount = mVideoPlayerCount; - mVideoCountMutex.unlock(); return videoPlayerCount; } diff --git a/es-core/src/Window.h b/es-core/src/Window.h index a1c73d84e..3ed856449 100644 --- a/es-core/src/Window.h +++ b/es-core/src/Window.h @@ -16,8 +16,8 @@ #include "Settings.h" #include "resources/TextureResource.h" +#include #include -#include #include class FileData; @@ -192,8 +192,7 @@ private: bool mCachedBackground; bool mInvalidatedCachedBackground; - int mVideoPlayerCount; - std::mutex mVideoCountMutex; + std::atomic mVideoPlayerCount; float mTopScale; bool mRenderedHelpPrompts; diff --git a/es-core/src/components/VideoComponent.cpp b/es-core/src/components/VideoComponent.cpp index cf93fcdab..5e42a92dc 100644 --- a/es-core/src/components/VideoComponent.cpp +++ b/es-core/src/components/VideoComponent.cpp @@ -90,12 +90,9 @@ void VideoComponent::setImage(std::string path) void VideoComponent::onShow() { - std::unique_lock playerLock(mPlayerMutex); mBlockPlayer = false; mPause = false; mShowing = true; - playerLock.unlock(); - manageState(); } @@ -113,30 +110,22 @@ void VideoComponent::onStopVideo() void VideoComponent::onPauseVideo() { - std::unique_lock playerLock(mPlayerMutex); mBlockPlayer = true; mPause = true; - playerLock.unlock(); - manageState(); } void VideoComponent::onUnpauseVideo() { - std::unique_lock playerLock(mPlayerMutex); mBlockPlayer = false; mPause = false; - playerLock.unlock(); - manageState(); } void VideoComponent::onScreensaverActivate() { - std::unique_lock playerLock(mPlayerMutex); mBlockPlayer = true; mPause = true; - playerLock.unlock(); if (Settings::getInstance()->getString("ScreensaverType") == "dim") stopVideo(); @@ -170,20 +159,16 @@ void VideoComponent::onGameLaunchedDeactivate() void VideoComponent::topWindow(bool isTop) { if (isTop) { - std::unique_lock playerLock(mPlayerMutex); mBlockPlayer = false; mPause = false; - playerLock.unlock(); // Stop video when closing the menu to force a reload of the // static image (if the theme is configured as such). stopVideo(); } else { - std::unique_lock playerLock(mPlayerMutex); mBlockPlayer = true; mPause = true; - playerLock.unlock(); } manageState(); } diff --git a/es-core/src/components/VideoComponent.h b/es-core/src/components/VideoComponent.h index f363b7ccd..51a4c7ea2 100644 --- a/es-core/src/components/VideoComponent.h +++ b/es-core/src/components/VideoComponent.h @@ -12,7 +12,7 @@ #include "GuiComponent.h" #include "components/ImageComponent.h" -#include +#include #include class MediaViewer; @@ -110,7 +110,6 @@ private: protected: Window* mWindow; ImageComponent mStaticImage; - std::mutex mPlayerMutex; unsigned mVideoWidth; unsigned mVideoHeight; @@ -124,9 +123,9 @@ protected: std::string mPlayingVideoPath; unsigned mStartTime; bool mStartDelayed; - bool mIsPlaying; - bool mIsActuallyPlaying; - bool mPause; + std::atomic mIsPlaying; + std::atomic mIsActuallyPlaying; + std::atomic mPause; bool mShowing; bool mDisable; bool mMediaViewerMode; diff --git a/es-core/src/components/VideoFFmpegComponent.cpp b/es-core/src/components/VideoFFmpegComponent.cpp index 20f5189a1..be47a74f8 100644 --- a/es-core/src/components/VideoFFmpegComponent.cpp +++ b/es-core/src/components/VideoFFmpegComponent.cpp @@ -130,10 +130,8 @@ void VideoFFmpegComponent::render(const glm::mat4& parentTrans) if (mIsPlaying && mFormatContext) { unsigned int color; - std::unique_lock pictureLock(mPictureMutex); - bool decodedFrame = mDecodedFrame; - pictureLock.unlock(); - if (decodedFrame && mFadeIn < 1) { + + if (mDecodedFrame && mFadeIn < 1) { const unsigned int fadeIn = static_cast(mFadeIn * 255.0f); color = Renderer::convertRGBAToABGR((fadeIn << 24) | (fadeIn << 16) | (fadeIn << 8) | 255); @@ -162,13 +160,11 @@ void VideoFFmpegComponent::render(const glm::mat4& parentTrans) for (int i = 0; i < 4; i++) vertices[i].pos = glm::round(vertices[i].pos); - pictureLock.lock(); - // This is needed to avoid a slight gap before the video starts playing. - if (!mDecodedFrame) { - pictureLock.unlock(); + if (!mDecodedFrame) return; - } + + std::unique_lock pictureLock(mPictureMutex); if (!mOutputPicture.hasBeenRendered) { // Move the contents of mOutputPicture to a temporary vector in order to call @@ -238,11 +234,12 @@ void VideoFFmpegComponent::updatePlayer() } if (mIsActuallyPlaying && mStartTimeAccumulation) { - mAccumulatedTime += + mAccumulatedTime = + mAccumulatedTime + static_cast(std::chrono::duration_cast( std::chrono::high_resolution_clock::now() - mTimeReference) .count()) / - 1000000000.0l; + 1000000000.0l; } mTimeReference = std::chrono::high_resolution_clock::now(); @@ -268,18 +265,16 @@ void VideoFFmpegComponent::frameProcessing() if (mAudioCodecContext) audioFilter = setupAudioFilters(); - bool isPlaying = true; - bool pause = false; - - while (isPlaying && !pause && videoFilter && (!mAudioCodecContext || audioFilter)) { + while (mIsPlaying && !mPause && videoFilter && (!mAudioCodecContext || audioFilter)) { readFrames(); - getProcessedFrames(); - outputFrames(); + if (!mIsPlaying) + break; - std::unique_lock playerLock(mPlayerMutex); - isPlaying = mIsPlaying; - pause = mPause; - playerLock.unlock(); + getProcessedFrames(); + if (!mIsPlaying) + break; + + outputFrames(); // This 1 ms wait makes sure that the thread does not consume all available CPU cycles. SDL_Delay(1); @@ -672,10 +667,8 @@ void VideoFFmpegComponent::readFrames() } } - if (readFrameReturn < 0) { - std::unique_lock playerLock(mPlayerMutex); + if (readFrameReturn < 0) mEndOfVideo = true; - } } void VideoFFmpegComponent::getProcessedFrames() @@ -761,11 +754,7 @@ void VideoFFmpegComponent::outputFrames() // Process the audio frames that have a PTS value below mAccumulatedTime (plus a small // buffer to avoid underflows). while (!mAudioFrameQueue.empty()) { - std::unique_lock audioLock(mAudioMutex); - auto accumulatedTime = mAccumulatedTime; - audioLock.unlock(); - - if (mAudioFrameQueue.front().pts < accumulatedTime + AUDIO_BUFFER) { + if (mAudioFrameQueue.front().pts < mAccumulatedTime + AUDIO_BUFFER) { // Enable only when needed, as this generates a lot of debug output. if (DEBUG_VIDEO) { LOG(LogDebug) << "Processing audio frame with PTS: " @@ -787,7 +776,7 @@ void VideoFFmpegComponent::outputFrames() if (outputSound) { // The audio is output to AudioManager from updatePlayer() in the main thread. - audioLock.lock(); + std::unique_lock audioLock(mAudioMutex); mOutputAudio.insert(mOutputAudio.end(), mAudioFrameQueue.front().resampledData.begin(), @@ -803,19 +792,11 @@ void VideoFFmpegComponent::outputFrames() } } - std::unique_lock playerLock(mPlayerMutex); - bool isActuallyPlaying = mIsActuallyPlaying; - playerLock.unlock(); - // Process all available video frames that have a PTS value below mAccumulatedTime. // But if more than one frame is processed here, it means that the computer can't // keep up for some reason. - while (isActuallyPlaying && !mVideoFrameQueue.empty()) { - std::unique_lock audioLock(mAudioMutex); - double accumulatedTime = mAccumulatedTime; - audioLock.unlock(); - - if (mVideoFrameQueue.front().pts < accumulatedTime) { + while (mIsActuallyPlaying && !mVideoFrameQueue.empty()) { + if (mVideoFrameQueue.front().pts < mAccumulatedTime) { // Enable only when needed, as this generates a lot of debug output. if (DEBUG_VIDEO) { LOG(LogDebug) << "Processing video frame with PTS: " @@ -843,7 +824,7 @@ void VideoFFmpegComponent::outputFrames() // can't keep up. This approach primarily decreases stuttering for videos with frame // rates close to, or at, the rendering frame rate, for example 59.94 and 60 FPS. if (mDecodedFrame && !mOutputPicture.hasBeenRendered) { - double timeDifference = accumulatedTime - mVideoFrameQueue.front().pts - + double timeDifference = mAccumulatedTime - mVideoFrameQueue.front().pts - mVideoFrameQueue.front().frameDuration * 2.0l; if (timeDifference < mVideoFrameQueue.front().frameDuration) { pictureLock.unlock(); @@ -1408,13 +1389,11 @@ void VideoFFmpegComponent::startVideo() void VideoFFmpegComponent::stopVideo() { - std::unique_lock playerLock(mPlayerMutex); mIsPlaying = false; mIsActuallyPlaying = false; mStartDelayed = false; mPause = false; mEndOfVideo = false; - playerLock.unlock(); mTexture.reset(); if (mFrameProcessingThread) { @@ -1459,11 +1438,7 @@ void VideoFFmpegComponent::pauseVideo() void VideoFFmpegComponent::handleLooping() { - std::unique_lock playerLock(mPlayerMutex); - bool endOfVideo = mEndOfVideo; - playerLock.unlock(); - - if (mIsPlaying && endOfVideo) { + if (mIsPlaying && mEndOfVideo) { // If the screensaver video swap time is set to 0, it means we should // skip to the next game when the video has finished playing. if (mScreensaverMode && diff --git a/es-core/src/components/VideoFFmpegComponent.h b/es-core/src/components/VideoFFmpegComponent.h index 66b80e984..9a0aef74e 100644 --- a/es-core/src/components/VideoFFmpegComponent.h +++ b/es-core/src/components/VideoFFmpegComponent.h @@ -23,6 +23,7 @@ extern "C" { #include } +#include #include #include #include @@ -161,10 +162,10 @@ private: int mVideoFrameReadCount; int mVideoFrameDroppedCount; - double mAccumulatedTime; - bool mStartTimeAccumulation; - bool mDecodedFrame; - bool mEndOfVideo; + std::atomic mAccumulatedTime; + std::atomic mStartTimeAccumulation; + std::atomic mDecodedFrame; + std::atomic mEndOfVideo; bool mSWDecoder; }; diff --git a/es-core/src/resources/TextureData.cpp b/es-core/src/resources/TextureData.cpp index e5e8d0a4a..e8d9b84f6 100644 --- a/es-core/src/resources/TextureData.cpp +++ b/es-core/src/resources/TextureData.cpp @@ -179,9 +179,7 @@ bool TextureData::load() const ResourceData& data = rm->getFileData(mPath); // Is it an SVG? if (Utils::String::toLower(mPath.substr(mPath.size() - 4, std::string::npos)) == ".svg") { - std::unique_lock lock(mMutex); mScalable = true; - lock.unlock(); std::string dataString; dataString.assign(std::string(reinterpret_cast(data.ptr.get()), data.length)); retval = initSVGFromMemory(dataString); @@ -285,11 +283,7 @@ float TextureData::sourceHeight() void TextureData::setSourceSize(float width, float height) { - std::unique_lock lock(mMutex); - bool scalable = mScalable; - lock.unlock(); - - if (scalable) { + if (mScalable) { if ((mSourceWidth != width) || (mSourceHeight != height)) { mSourceWidth = width; mSourceHeight = height; diff --git a/es-core/src/resources/TextureData.h b/es-core/src/resources/TextureData.h index 6562643c1..062e54d6f 100644 --- a/es-core/src/resources/TextureData.h +++ b/es-core/src/resources/TextureData.h @@ -11,6 +11,7 @@ #include "utils/MathUtil.h" +#include #include #include #include @@ -79,7 +80,7 @@ private: int mHeight; float mSourceWidth; float mSourceHeight; - bool mScalable; + std::atomic mScalable; bool mLinearMagnify; bool mReloadable; bool mForceRasterization; diff --git a/es-core/src/resources/TextureDataManager.cpp b/es-core/src/resources/TextureDataManager.cpp index f43a84b3f..b9e7256da 100644 --- a/es-core/src/resources/TextureDataManager.cpp +++ b/es-core/src/resources/TextureDataManager.cpp @@ -161,10 +161,11 @@ TextureLoader::~TextureLoader() std::unique_lock lock(mMutex); mTextureDataQ.clear(); mTextureDataLookup.clear(); + lock.unlock(); // Exit the thread. mExit = true; - lock.unlock(); + mEvent.notify_one(); mThread->join(); mThread.reset(); @@ -172,9 +173,7 @@ TextureLoader::~TextureLoader() void TextureLoader::threadProc() { - bool exit = false; - - while (!exit) { + while (!mExit) { std::shared_ptr textureData; { // Wait for an event to say there is something in the queue. @@ -199,7 +198,6 @@ void TextureLoader::threadProc() mTextureDataLookup.erase(mTextureDataLookup.find(textureData.get())); } } - exit = mExit; } } diff --git a/es-core/src/resources/TextureDataManager.h b/es-core/src/resources/TextureDataManager.h index 1989e1915..d1a1b03ef 100644 --- a/es-core/src/resources/TextureDataManager.h +++ b/es-core/src/resources/TextureDataManager.h @@ -9,6 +9,7 @@ #ifndef ES_CORE_RESOURCES_TEXTURE_DATA_MANAGER_H #define ES_CORE_RESOURCES_TEXTURE_DATA_MANAGER_H +#include #include #include #include @@ -41,7 +42,7 @@ private: std::unique_ptr mThread; std::mutex mMutex; std::condition_variable mEvent; - bool mExit; + std::atomic mExit; }; //