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

[3.x] Fix crash in AudioServer when switching audio devices with different audio channels count #59778

Merged

Conversation

Listwon
Copy link
Contributor

@Listwon Listwon commented Apr 1, 2022

Fixes #52275

Switching audio devices with different audio channels count caused crashes in AudioServer::_mix_step() because it tried to access non-existent effects instances in bus->channels[k].effect_instances. AudioServer::init_channels_and_buffers() is called if there is change in get_channel_count(). The update of effects (creating new effects instances) on new channels was missing here.

…audio channels count (connecting PS5 controller, bluetooth 5.1 headphones etc.)
@Listwon Listwon requested a review from a team as a code owner April 1, 2022 09:01
@Listwon Listwon changed the title Fix crash in AudioServer when switching audio devices with different audio channels count [3.x] Fix crash in AudioServer when switching audio devices with different audio channels count Apr 1, 2022
@akien-mga akien-mga added this to the 3.5 milestone Apr 1, 2022
@akien-mga
Copy link
Member

Is this only relevant for 3.x, or is there the same bug in master?

@Listwon
Copy link
Contributor Author

Listwon commented Apr 1, 2022

@akien-mga I've just tried to reproduce it in master build and it's the same bug (easy reproduction with PS5 controller connected/disconnected via USB cable), the same fix works on master too. Should I prepare separate PR for master or you'll apply it yourself when it's tested and merged on 3.x?

@akien-mga
Copy link
Member

A separate PR for master would be great. We usually only cherry-pick from newer to older branches, so we don't have a pipeline to keep track of changes the other way around.

@ellenhp
Copy link
Contributor

ellenhp commented Apr 1, 2022

What happens if the audio thread is actively mixing when this happens? I could imagine we might need to lock the AudioServer in this method before messing with the buffers? LGTM though, because this should fix it most of the time.

@Listwon
Copy link
Contributor Author

Listwon commented Apr 1, 2022

@ellenhp I thought the same initially, but if I'm not mistaken init_channels_and_buffers() is called synchronously before the _mix_step(). Both in the same audio thread, where AudioDriver is already locked before those calls.
obraz

@ellenhp
Copy link
Contributor

ellenhp commented Apr 1, 2022

@akien-mga I've approved both of these and I'll let you either merge both or merge into master and cherry-pick or whatever we want to do.

@Listwon Thanks for figuring this out and fixing it!

@akien-mga akien-mga merged commit 39346f8 into godotengine:3.x Apr 1, 2022
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 3.4.5.

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