Fix audio stream generators getting freed accidentally #81508
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #65155
This is an alternative to #73162 following @reduz suggestion there #73162 (comment)
To recap: In Godot 4.x audio stream generators have a fundamental bug that they get killed by the audio server in case of a buffer underrun. This leads to a very ugly race condition already when trying to set them up: If the very first "buffer fill" comes too late, the audio server will kill the playback generator even before it got its first chance to fill the buffer! Taking my minimal debug project as an example, I just had to launch this small app 53 times (!) to get one working instance of an audio stream generator. This means that in Godot 4.x, generators are almost completely unusable right now. (I have a music project that I'd love to migrate to Godot 4.x, but this bug is a complete showstopper.)
Possible minimal bugfixes: There are a few ways to fix this issue without the need for a big refactoring. #73162 had proposed to fix it on the level of the audio server, by changing the logic when to kill a playback to incorporate the
.is_playing()
information. This PR here basically solves it on the other end -- inAudioStreamGeneratorPlayback
itself. In my opinion both fixes are a big improvement over the more or less completely broken state in 4.x / master. There are a few minor aspects why the approach taken in this PR may be preferable:It is a more local change, not having to touch audio server logic itself, thus affecting only the anyway broken
AudioStreamGeneratorPlayback
.The change makes the behavior consistent with the other playback's
_mix_internal
implementations (AudioStreamPlaybackMP3::_mix_internal
,AudioStreamPlaybackOggVorbis::_mix_internal
), which all have the semantics: Return 0 if inactive/stopped, otherwise return either the number of requested frames, or the number of available frames due to their finite nature. A generator conceptually falls into the infinite category though. Therefore, it should always simply return the number of requested frames. In particular, a buffer underrun should not become a signal to the audio server to kill the playback -- a buffer underrun is always an accident!There is currently an inconsistency between how
mixed
gets updated vs the return value:godot/servers/audio/effects/audio_stream_generator.cpp
Lines 162 to 163 in fc99492
mixed
already gets incremented by an amount corresponding top_frames
(the number of requested frames), but the return value can indicate something else. This PR would make the update tomixed
and the return value consistent.In the long term I fully agree with @reduz that it is probably best to partially revert #51296 and do things like ramp up/down rather on the playback/stream level, because they have better capabilities of doing lookaheads e.g. in case of file based playback. I've collected a few relevant parts of the discussion:
However, this would obviously require a bigger refactoring.
For the time being, I'd really appreciate if we could either merge (and release) the bugfix in #73162 or this PR here. Over the course over the last half year I've tried numerous hacks on user side trying to work around this bug, but I don't think there is anything that's really practical. Either of the two PRs would change the situation from "more or less unusable" to "basically fully working", so I hope we can go for a temporary fix now, and look into this bigger refactoring as a second step.