Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Audio crash on app exit while many sounds are playing #19507

Open
rh101 opened this Issue Mar 13, 2019 · 2 comments

Comments

Projects
None yet
1 participant
@rh101
Copy link
Contributor

rh101 commented Mar 13, 2019

  • 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

This comment has been minimized.

Copy link
Contributor Author

rh101 commented Mar 13, 2019

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

This comment has been minimized.

Copy link
Contributor Author

rh101 commented Mar 13, 2019

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);
    }
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.