Audio crash on app exit while many sounds are playing

[rh101]

• cocos2d-x version: 3.17.1
• devices test on: Win32 build on Windows 10
• developing environments
  • NDK version: N/A
  • Xcode version: N/A
  • VS version: VS2017 v15.9.9
  • browser type and version: N/A

Steps to Reproduce:

1. Play a lot of short audio sounds (mp3 in my case) very quickly
2. While the sounds are playing, close the application

void AudioEngineImpl::stop(int audioID)
{
    auto player = _audioPlayers[audioID];
    player->destroy();  <=== crashes on this, since player == null

    // Call 'update' method to cleanup immediately since the schedule may be cancelled without any notification.
    update(0.0f);
}

It happens often, but intermittently, and can be reproduced. Even if a null check is done for player, it will crash further on in the update() method since it also uses the value from _audioPlayers[audioID].

The reason ::stop() is called is because on exit, my code uncaches all audio files using this call:

void AudioEngine::uncache(const std::string &filePath)
{
    if(!_audioEngineImpl){
        return;
    }
    auto audioIDsIter = _audioPathIDMap.find(filePath);
    if (audioIDsIter != _audioPathIDMap.end())
    {
        //@Note: For safely iterating elements from the audioID list, we need to copy the list
        // since 'AudioEngine::remove' may be invoked in '_audioEngineImpl->stop' synchronously.
        // If this happens, it will break the iteration, and crash will appear on some devices.
        std::list<int> copiedIDs(audioIDsIter->second);

        for (int audioID : copiedIDs)
        {
            _audioEngineImpl->stop(audioID);  <====== this is where stop is called, which leads to the crash

            auto itInfo = _audioIDInfoMap.find(audioID);
            if (itInfo != _audioIDInfoMap.end())
            {
                if (itInfo->second.profileHelper)
                {
                    itInfo->second.profileHelper->audioIDs.remove(audioID);
                }
                _audioIDInfoMap.erase(audioID);
            }
        }
        _audioPathIDMap.erase(filePath);
    }

    if (_audioEngineImpl)
    {
        _audioEngineImpl->uncache(filePath);
    }
}

I tried to trace why _audioPlayers[audioID] == null, but I couldn't find where it was being set except this section:

int AudioEngineImpl::play2d(const std::string &filePath ,bool loop ,float volume)
{
...
     player->setCache(audioCache);
    _threadMutex.lock();
    _audioPlayers[_currentAudioID] = player;
    _threadMutex.unlock();

    _alSourceUsed[alSource] = true;
...
}

Any ideas as to why it would be null?

[rh101]

So, after looking into this a bit more, I can see that the audio player is being removed in this section of code:

void AudioEngineImpl::update(float dt)
{
...
        else if (player->_ready && sourceState == AL_STOPPED) {
            std::string filePath;
            if (player->_finishCallbak) {
                auto& audioInfo = AudioEngine::_audioIDInfoMap[audioID];
                filePath = *audioInfo.filePath;
                player->setCache(nullptr); // it's safe for player didn't free audio cache
            }

            AudioEngine::remove(audioID);

            _threadMutex.lock();
            it = _audioPlayers.erase(it);   //  ***** This is getting called on the problematic audioID that crashes, so it is being removed, but somehow it's still in the _audioPlayers dictionary with a null entry by the time the stop method is called
            _threadMutex.unlock();

            if (player->_finishCallbak) {
                player->_finishCallbak(audioID, filePath); //FIXME: callback will delay 50ms
            }
            delete player;
            _alSourceUsed[alSource] = false;
        }
...
}

[rh101]

Working fix (I don't think it addresses the real issue though. I still don't know why _audioPlayers[audioID] is null):

Add null checks to the stop() and stopAll() methods:

void AudioEngineImpl::stop(int audioID)
{
    auto player = _audioPlayers[audioID];
    if (player)
    {
        player->destroy();
    }
    //Note: Don't set the flag to false here, it should be set in 'update' function.
    // Otherwise, the state got from alSourceState may be wrong
//    _alSourceUsed[player->_alSource] = false;

    // Call 'update' method to cleanup immediately since the schedule may be cancelled without any notification.
    update(0.0f);
}

void AudioEngineImpl::stopAll()
{
    for(auto&& player : _audioPlayers)
    {
        if (player.second)
        {
            player.second->destroy();
        }
    }
    //Note: Don't set the flag to false here, it should be set in 'update' function.
    // Otherwise, the state got from alSourceState may be wrong
//    for(int index = 0; index < MAX_AUDIOINSTANCES; ++index)
//    {
//        _alSourceUsed[_alSources[index]] = false;
//    }

    // Call 'update' method to cleanup immediately since the schedule may be cancelled without any notification.
    update(0.0f);
}

Add a null check in the update() method:

void AudioEngineImpl::update(float dt)
{
    ALint sourceState;
    int audioID;
    AudioPlayer* player;
    ALuint alSource;

//    ALOGV("AudioPlayer count: %d", (int)_audioPlayers.size());

    for (auto it = _audioPlayers.begin(); it != _audioPlayers.end(); ) {
        audioID = it->first;
        player = it->second;
        // *** FIX *** check if player is null before using it
        if (!player)
        {
            AudioEngine::remove(audioID);
            _threadMutex.lock();
            it = _audioPlayers.erase(it);
            _threadMutex.unlock();
            continue;
        }

        alSource = player->_alSource;
        alGetSourcei(alSource, AL_SOURCE_STATE, &sourceState);

        if (player->_removeByAudioEngine)
        {
            AudioEngine::remove(audioID);
            _threadMutex.lock();
            it = _audioPlayers.erase(it);
            _threadMutex.unlock();
            delete player;
            _alSourceUsed[alSource] = false;
        }
        else if (player->_ready && sourceState == AL_STOPPED) {

            std::string filePath;
            if (player->_finishCallbak) {
                auto& audioInfo = AudioEngine::_audioIDInfoMap[audioID];
                filePath = *audioInfo.filePath;
                player->setCache(nullptr); // it's safe for player didn't free audio cache
            }

            AudioEngine::remove(audioID);

            _threadMutex.lock();
            it = _audioPlayers.erase(it);
            _threadMutex.unlock();

            if (player->_finishCallbak) {
                player->_finishCallbak(audioID, filePath); //FIXME: callback will delay 50ms
            }
            delete player;
            _alSourceUsed[alSource] = false;
        }
        else{
            ++it;
        }
    }

    if(_audioPlayers.empty()){
        _lazyInitLoop = true;
        _scheduler->unschedule(CC_SCHEDULE_SELECTOR(AudioEngineImpl::update), this);
    }
}