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 in AudioStreamPlaybackResampled #46078

Closed
wants to merge 1 commit into from

Conversation

ellenhp
Copy link
Contributor

@ellenhp ellenhp commented Feb 16, 2021

This should fix some of the worst pops tracked by #22016. That issue seems to also be tracking some issues involving NaNs in the audio buffer though so I don't think this PR should close it.

The root cause of this pop seems to be related to the fact that start() calls in these streams trigger _begin_resample on the calling thread (usually the main thread, I'd think). Meanwhile, the stream is already active, so the audio thread is free to call its mix function at any time. This change prevents mixing from happening on both the audio thread and the main thread at the same time. There might be other ways to accomplish this, but this is what came to mind.

Notably, There's still a faint crackle on a AudioStreamPlayer2/3D's play() even with this change applied. I'm not sure what's causing that.

Testing is most easily done in this project or something similar:
sound_pop.zip

What's important is the fact that it creates and initializes a new AudioStream every few seconds and plays it. This means there are lots of opportunities for the bug to crop up. It is nondeterministic, so you might have to wait 15-30 seconds, but usually not any longer in my experience. The pop I'm fixing in this PR is very harsh and pretty hard to miss.

To reproduce the issue open up the repro project and run it, then switch to an ogg or mp3 stream, and switch to an AudioStream2D or AudioStream3D.

I'm not entirely sure why but this pop doesn't repro at all on a regular AudioStreamPlayer (perhaps it has some kind of fade-in mechanism that the others don't?)

@ellenhp ellenhp marked this pull request as draft February 16, 2021 02:15
@YeldhamDev YeldhamDev requested a review from a team February 16, 2021 04:56
@ellenhp ellenhp marked this pull request as ready for review February 16, 2021 05:51
@akien-mga akien-mga added this to the 4.0 milestone Feb 16, 2021
@ellenhp
Copy link
Contributor Author

ellenhp commented Feb 17, 2021

I'm aware that this fix has some code smell. I'd ultimately like to do something like godotengine/godot-proposals#2299 but that'll take a while if it does happen. And I doubt it would be easy to backport to 3.2, so I think this fix might be worth considering on its own merits.

@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Feb 17, 2021
@ellenhp ellenhp marked this pull request as draft February 18, 2021 02:16
@ellenhp
Copy link
Contributor Author

ellenhp commented Feb 18, 2021

This is the wrong solution. I think I figured out what's happening though. AudioStreamPlayer3D is calling mix() on a stream before it starts it. Presumably because of a race condition (set_stream racing with _mix_audio). Will update the bug with this info and a possible workaround.

@ellenhp ellenhp closed this Feb 18, 2021
@akien-mga akien-mga added archived and removed cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Feb 18, 2021
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.

3 participants