Do not call SDL_Mixer functions from the callback set by the Mix_Chan…

…nelFinished() (#5608)

src/engine/audio.cpp

@@ -27,6 +27,7 @@
 #include <map>
 #include <mutex>
 #include <numeric>
+#include <utility>
 
 #include <SDL.h>
 #include <SDL_mixer.h>
@@ -72,25 +73,107 @@ namespace
     // be acquired in any callback functions that can be called by SDL_Mixer.
     std::recursive_mutex audioMutex;
 
+    class SoundSampleManager
+    {
+    public:
+        SoundSampleManager() = default;
+        SoundSampleManager( const SoundSampleManager & ) = delete;
+
+        ~SoundSampleManager()
+        {
+            // Make sure that all sound samples have been eventually freed
+            assert( std::all_of( _channelSamples.begin(), _channelSamples.end(), []( const auto & item ) {
+                static const decltype( item.second ) nullQueue{ nullptr, nullptr };
+
+                return item.second == nullQueue;
+            } ) );
+        }
+
+        SoundSampleManager & operator=( const SoundSampleManager & ) = delete;
+
+        void channelStarted( const int channelId, Mix_Chunk * sample )
+        {
+            assert( channelId >= 0 && sample != nullptr );
+
+            const auto iter = _channelSamples.find( channelId );
+
+            if ( iter != _channelSamples.end() ) {
+                auto & sampleQueue = iter->second;
+
+                if ( sampleQueue.first == nullptr ) {
+                    sampleQueue.first = sample;
+                }
+                else if ( sampleQueue.second == nullptr ) {
+                    sampleQueue.second = sample;
+                }
+                else {
+                    // The sample queue is already full, this shouldn't happen
+                    assert( 0 );
+                }
+
+                return;
+            }
+
+            const auto res = _channelSamples.try_emplace( channelId, std::make_pair( sample, nullptr ) );
+
+            if ( !res.second ) {
+                assert( 0 );
+            }
+        }
+
+        // This is the only method that can be called from the SDL_Mixer callback (without acquiring the audioMutex)
+        void channelFinished( const int channelId )
+        {
+            assert( channelId >= 0 );
+
+            const std::scoped_lock<std::mutex> lock( _channelsToCleanupMutex );
+
+            _channelsToCleanup.push_back( channelId );
+        }
+
+        void clearFinishedSamples()
+        {
+            std::vector<int> channelsToCleanup;
+
+            {
+                const std::scoped_lock<std::mutex> lock( _channelsToCleanupMutex );
+
+                std::swap( channelsToCleanup, _channelsToCleanup );
+            }
+
+            for ( const int channel : channelsToCleanup ) {
+                const auto iter = _channelSamples.find( channel );
+                assert( iter != _channelSamples.end() );
+
+                auto & sampleQueue = iter->second;
+                assert( sampleQueue.first != nullptr );
+
+                Mix_FreeChunk( sampleQueue.first );
+
+                // Shift the sample queue
+                sampleQueue.first = sampleQueue.second;
+                sampleQueue.second = nullptr;
+            }
+        }
+
+    private:
+        std::map<int, std::pair<Mix_Chunk *, Mix_Chunk *>> _channelSamples;
+
+        std::vector<int> _channelsToCleanup;
+        // This mutex protects operations with _channelsToCleanup
+        std::mutex _channelsToCleanupMutex;
+    };
+
+    SoundSampleManager soundSampleManager;
+
     // This is the callback function set by Mix_ChannelFinished(). As a rule, it is called from
-    // a SDL_Mixer internal thread.
-    //
-    // TODO: according to SDL_Mixer manual, calls of any SDL_Mixer functions are not allowed in
-    // TODO: callbacks. The current code works, but it would be good to find a reliable way to
-    // TODO: perform channel cleanup without calling these functions.
+    // a SDL_Mixer internal thread. Calls of any SDL_Mixer functions are not allowed in callbacks.
     void channelFinished( const int channelId )
     {
         // This callback function should never be called if audio is not initialized
         assert( isInitialized );
 
-        Mix_Chunk * sample = Mix_GetChunk( channelId );
-        assert( sample != nullptr );
-
-        Mix_FreeChunk( sample );
-
-        if ( Mix_UnregisterAllEffects( channelId ) == 0 ) {
-            ERROR_LOG( "Failed to unregister all effects from channel " << channelId << ". The error: " << Mix_GetError() )
-        }
+        soundSampleManager.channelFinished( channelId );
     }
 
     int playSound( const uint8_t * ptr, const uint32_t size, const int channelId, const bool loop )
@@ -106,7 +189,16 @@ namespace
         const int channel = Mix_PlayChannel( channelId, sample, loop ? -1 : 0 );
         if ( channel < 0 ) {
             ERROR_LOG( "Failed to play an audio chunk for channel " << channel << ". The error: " << Mix_GetError() )
+
+            Mix_FreeChunk( sample );
         }
+        else {
+            // There can be a maximum of two items in the sample queue for a channel:
+            // the previous sample (if it hasn't been released yet) and the current one
+            soundSampleManager.channelStarted( channel, sample );
+        }
+
+        soundSampleManager.clearFinishedSamples();
 
         return channel;
     }
@@ -494,6 +586,7 @@ void Audio::Quit()
 
         Mix_ChannelFinished( nullptr );
 
+        soundSampleManager.clearFinishedSamples();
         musicSettings.trackManager.clear();
 
         Mix_CloseAudio();
@@ -667,14 +760,14 @@ int Mixer::setVolume( const int channelId, const int volumePercentage )
         return normalizeFromSDLVolume( prevVolume );
     }
 
-    const size_t channelNum = static_cast<size_t>( channelId );
+    const size_t channel = static_cast<size_t>( channelId );
 
-    if ( channelNum >= savedMixerVolumes.size() ) {
+    if ( channel >= savedMixerVolumes.size() ) {
         return 0;
     }
 
-    const int prevVolume = savedMixerVolumes[channelNum];
-    savedMixerVolumes[channelNum] = volume;
+    const int prevVolume = savedMixerVolumes[channel];
+    savedMixerVolumes[channel] = volume;
 
     return normalizeFromSDLVolume( prevVolume );
 }