-
-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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
Prevent WASAPI error spam when device cannot be initialized. #100116
Prevent WASAPI error spam when device cannot be initialized. #100116
Conversation
3d1f518
to
d753f55
Compare
Possibly unrelated to the changes of this PR, there is memory leaking in the editor during the time that it can't use/initialize an audio device: I haven't compared with "before this PR", because without this fix there are tens of thousands of error messages logged to the console, so "before this PR" is obviously going to be messy in terms of memory anyway. I'm converting this PR to be a draft while I look into why memory is increasing. |
5fe7163
to
80aba1b
Compare
Well, I've found a bunch of memory leaks and fixed them up, but still haven't got things entirely stable. Here's where I'm at with my profiler: Now I'm only getting around 100 allocations every ten seconds instead of something like 1,500. This is a lot better than how things were before, but it appears that things could be better... |
78282e5
to
616ed47
Compare
Alright, it turns out that the This seems to remove any remaining memory leaks that happen from attempting (and failing) to reinitialize an audio device literally thousands of times per second: There are still some allocation increases and decreases that can be seen in those snapshots, but I don't believe these are leaks related to WASAPI. The initial large allocation is likely from the console error messages in the editor. The next bit of work is to make the re-initialization attempts a little bit more reasonable. These should not be called thousands of times per second. Maybe once every second or so would be a better approach? |
616ed47
to
086eabe
Compare
I've updated the PR to only attempt to reinitialize audio devices once every 1 second or so instead of once every millisecond. At this point, I think this PR addresses the outstanding major issues of error spam, memory leaks, and pathologically attempting to re-initialize an unavailable audio device every millisecond. This can probably be safely cherry-picked, but I'll leave that to the production team to assess. If anyone wants to test the scenario where the audio device is lost due to being taken over by an exclusive mode device, you can use foobar2000 and switch to "Primary Sound Driver [exclusive]" while the Godot editor is open. Switch back to the default "Primary Sound Driver" to successfully re-initialize the WASAPI audio device. |
Fixes godotengine#99968 and prevents the error spam referenced in comments of godotengine#18732. Also fixes a number of memory leaks that occur when an audio device is reinitialized or fails to reinitialize and gates reinitialization attempts to around 1 per second instead of ~1000 attempts per second. Co-authored-by: Kusok <118438257+kus04e4ek@users.noreply.github.com>
086eabe
to
db63d3e
Compare
Cross-posting here that someone was able to test this PR and confirm it fixes the issue it intends to fix: #18732 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not too familiar with WASAPI but this looks like a good consolidation of the error handling and leak prevention.
Thanks! |
Fixes #99968 and prevents the error spam referenced in comments of #18732.
Also fixes a number of memory leaks that occur when an audio device is reinitialized or fails to reinitialize and gates reinitialization attempts to around 1 per second instead of ~1000 attempts per second.
This PR supersedes #94252. I have used some of the logic from @kus04e4ek, so I have put them as a co-author. (Originally, I put the call to
finish_output_device()
in a different, less correct spot and the other PR's approach is better.)