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.

Sign up for GitHub

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

Jump to bottom

add test case for issue #18597(audio crash on iOS), and fix #18865

Merged
merged 17 commits into from Jul 11, 2018
Merged

add test case for issue #18597(audio crash on iOS), and fix #18865

merged 17 commits into from Jul 11, 2018

Conversation

drelaptop
Copy link
Contributor

add test case for #18597, and try to fix

@drelaptop drelaptop mentioned this pull request May 30, 2018
Closed
@drelaptop drelaptop requested a review from dumganhar June 5, 2018 10:57
@drelaptop
Copy link
Contributor Author

@dumganhar could you review this PR?

@drelaptop drelaptop changed the title add test case for #18597, and try to fix add test case for #18597, and fix Jun 5, 2018
dumganhar
dumganhar reviewed Jun 6, 2018
View reviewed changes
cocos/audio/apple/AudioPlayer.mm Outdated
ALint bufferProcessed = 0;
alGetSourcei(_alSource, AL_BUFFERS_PROCESSED, &bufferProcessed);
while (bufferProcessed < QUEUEBUFFER_NUM) {
std::this_thread::sleep_for(std::chrono::milliseconds(10));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this sleep waiting be smaller, it may lead to audio lag if there are buffers in queue.
Please test it with 1 or 2 milliseconds.

dumganhar
dumganhar reviewed Jun 6, 2018
View reviewed changes
cocos/audio/apple/AudioPlayer.mm Outdated
@@ -290,7 +305,13 @@ of this software and associated documentation files (the "Software"), to deal
break;
}
}

/*
While the
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the: don't put just two words in a line.

drelaptop
drelaptop commented Jun 6, 2018
View reviewed changes
cocos/audio/apple/AudioPlayer.mm
if (sourceState == AL_PLAYING) {
alGetSourcei(_alSource, AL_BUFFERS_PROCESSED, &bufferProcessed);
while (bufferProcessed < QUEUEBUFFER_NUM) {
std::this_thread::sleep_for(std::chrono::milliseconds(2));
Copy link
Contributor Author

@drelaptop drelaptop Jun 6, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have counted time cost of this code block, it's about 2 * QUEUEBUFFER_TIME_STEP, about cost 160-240 ms when the QUEUEBUFFER_TIME_STEP is 0.1, about 80-120 ms when the QUEUEBUFFER_TIME_STEP is 0.05.

time cost when the QUEUEBUFFER_TIME_STEP is 0.05

UnqueueBuffers spend extra time: 131.72 ms
UnqueueBuffers spend extra time: 86.84 ms
issues 18597 audio crash test
UnqueueBuffers spend extra time: 98.89 ms
UnqueueBuffers spend extra time: 87.53 ms
issues 18597 audio crash test
UnqueueBuffers spend extra time: 121.34 ms
UnqueueBuffers spend extra time: 86.42 ms
issues 18597 audio crash test
UnqueueBuffers spend extra time: 80.64 ms
UnqueueBuffers spend extra time: 86.46 ms

not matter when changing the sleep time to 1 ms or 2 ms. change the QUEUEBUFFER_TIME_STEP from 0.1 to 0.05 will half the time cost. It's meaningful.

after changing QUEUEBUFFER_TIME_STEP to 0.05, I have tested play bigger audio file on iPhone 5s, 6, 8, all play smoothly.

cocos/audio/apple/AudioPlayer.mm
@@ -305,15 +330,15 @@ of this software and associated documentation files (the "Software"), to deal

if (!_needWakeupRotateThread)
{
_sleepCondition.wait_for(lk,std::chrono::milliseconds(75));
_sleepCondition.wait_for(lk,std::chrono::milliseconds(rotateSleepTime));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the sleep time in rotate buffer thread shouldn't been hard coded, it depends on QUEUEBUFFER_TIME_STEP

@drelaptop drelaptop added this to the 3.18 milestone Jul 10, 2018
@drelaptop
Copy link
Contributor Author

according to the feedback form #18597 , this patch works well.

@drelaptop drelaptop changed the title add test case for #18597, and fix add test case for issue #18597(audio crash on iOS), and fix Jul 11, 2018
@drelaptop drelaptop merged commit 8d4ee80 into cocos2d:v3 Jul 11, 2018
@DonMizzi
Copy link

@drelaptop
I have a problem, maybe related
I get problems with background/foreground with maintheme music in loop:

// Mainpage::init

int idBackGroundMusic = AudioEngine::play2d(maintheme.mp3);
AudioEngine::setLoop(idBackGroundMusic, true);
AudioEngine::setVolume(idBackGroundMusic, 0.5f);
return true;

after some background/foreground (pauseAll()/resumeAll()) I received this log, and maintheme stops

V/AudioPlayer (70): ~AudioPlayer() (0x103817430), id=1
V/AudioPlayer (84): AudioPlayer::destroy begin, id=1
V/AudioPlayer (334): Exit rotate buffer thread ...
V/AudioPlayer (124): rotateBufferThread exited!
V/AudioPlayer (140): UnqueueBuffers Before alSourceStop
V/AudioPlayer (146): Before alSourceStop
V/AudioPlayer (148): Before alSourcei
V/AudioPlayer (154): AudioPlayer::destroy end, id=1

Cocos2d-x 3.16, iOS, iPad mini 3

@drelaptop
Copy link
Contributor Author

@DonMizzi could you try to pick this PR into your 3.16, and then test again.

@DonMizzi
Copy link

DonMizzi commented Nov 16, 2018 •
edited

Yes, I have just tried, but forgot to report:

https://discuss.cocos2d-x.org/t/audioengine-stop-loop-background-music-on-resume/44552/2

With this PR frequency of occurency incraease.
Seems related to QUEUEBUFFER_TIME_STEP value.

If I set QUEUEBUFFER_TIME_STEP = 0.1, problems happens less frequent with or without this PR.
If I set QUEUEBUFFER_TIME_STEP = 0.05, problems happens more frequent with or without this PR.

There are differences also using home button or lock screen button: with lock screen error always happens more frequent.

With home button error happens on resume, with lock button happens on background

So:

with this patch and QUEUEBUFFER_TIME_STEP = 0.05 using Home Button - 9/10 tests - Error
with this patch and QUEUEBUFFER_TIME_STEP = 0.05 using Lock Screen - 1/2 tests - Error

without this patch and QUEUEBUFFER_TIME_STEP = 0.1 using Home Button - 50 tests - NO Error
without this patch and QUEUEBUFFER_TIME_STEP = 0.1 using Lock Screen - 30/40 tests - Error (very rare)

@DonMizzi
Copy link

DonMizzi commented Nov 16, 2018 •
edited

After a few more tests, seems that with or without PR something change:

without this patch and QUEUEBUFFER_TIME_STEP = 0.1 using Home Button - 50 tests - NO Error
without this patch and QUEUEBUFFER_TIME_STEP = 0.1 using Lock Screen - 30/40 tests - Error (very rare)

with this patch but QUEUEBUFFER_TIME_STEP = 0.1 using Home Button - 40/50 tests - Error (very rare)
with this patch but QUEUEBUFFER_TIME_STEP = 0.1 using Lock Screen - 10/20 tests - Error

test is a single Background/Foreground

@stevetranby
Copy link
Contributor

Why is the AudioPlayer getting destroyed when pauseAll is called?
Is that the expected behavior or AudioEngine?

@drelaptop drelaptop deleted the fix-audio-crash branch December 16, 2018 07:41
@PatriceJiang PatriceJiang mentioned this pull request Aug 26, 2019
Open
@PPpro PPpro mentioned this pull request Jul 16, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dumganhar dumganhar dumganhar left review comments

Assignees
No one assigned
Labels
module:audio platform:ios
Projects
None yet
Milestone
3.17.1
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@drelaptop @DonMizzi @stevetranby @dumganhar