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

Fix a pop on play() in AudioStreamPlayer2D and 3D #46151

Merged
merged 1 commit into from
Feb 18, 2021

Conversation

ellenhp
Copy link
Contributor

@ellenhp ellenhp commented Feb 18, 2021

Successor to #46078.

This fixes pops by ensuring that audio stream players start their playback streams before attempting to mix them. This issue was introduced in 016f7bd and causes _mix_audio() to occasionally attempt to mix using a playback stream before the physics process notification has properly started it. Specifically, if setseek is left unset, the audio stream playback's start() method will never be called. If you're having difficulty reproducing the pop that this fixes, try halving your physics framerate until you can repro it.

In addition to reverting the change referenced above, I added some logic so I don't reintroduce the issue it fixed.

While I think it fixes the worst of the pops, I'm not sure if it would be appropriate to call #22016 completely solved with it, since there may be other issues. AudioStreamPlayer2D and 3D still have pops on stop() but they sound just as clean at the beginning of playback as a regular AudioStreamPlayer now.

To test, download this sample project.

new_test_project.zip

Run the test project and select the options to play an OGG or MP3 using both 3D and 2D player nodes. WAV files will be unaffected because they don't need to be started before being used.

@ellenhp ellenhp marked this pull request as ready for review February 18, 2021 03:23
@ellenhp ellenhp requested review from a team as code owners February 18, 2021 03:23
@akien-mga akien-mga added bug cherrypick:3.x Considered for cherry-picking into a future 3.x release topic:audio labels Feb 18, 2021
@akien-mga akien-mga added this to the 4.0 milestone Feb 18, 2021
@akien-mga akien-mga requested review from a team and removed request for a team February 18, 2021 09:11
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good!

@akien-mga akien-mga merged commit 9d84e3b into godotengine:master Feb 18, 2021
@akien-mga
Copy link
Member

Thanks! Good job pinpointing the cause of the regression, that does confirm that this is a better fix to the original issue + the regression.

@akien-mga
Copy link
Member

Cherry-picked for 3.2.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants