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

Audio Stream Playback Generator Broken #65155

Closed
Tracked by #76797
jaylinwylie opened this issue Aug 31, 2022 · 15 comments · Fixed by #81508
Closed
Tracked by #76797

Audio Stream Playback Generator Broken #65155

jaylinwylie opened this issue Aug 31, 2022 · 15 comments · Fixed by #81508

Comments

@jaylinwylie
Copy link

Godot version

Godot 4.X

System information

Windows 11, Default WASAPI drivers,

Issue description

When running the audio generator demo. using get_stream_playback() returns null which causes playback.get_frames_available() to error out with Attempt to call function 'get_frames_available' in base 'null instance' on a null instance..
I've tried writing a condition in the _process() function to check if it is null and retry getting the playback node. After a few cycles, it will return positive but will only fill the buffer once and will not process the frames any further, leaving playback.get_frames_available() to return 0 for the remainder of the runtime.

Code:

extends Node

var sample_hz = 44100/4.0 # Keep the number of samples to mix low, GDScript is not super fast.
var pulse_hz = 440.0
var phase = 0.0

var playback: AudioStreamPlayback = null # Actual playback stream, assigned in _ready().

func _fill_buffer():
	var increment = pulse_hz / sample_hz

	var to_fill = playback.get_frames_available()
	print(to_fill)
	var buffer : PackedVector2Array
	while to_fill > 0:
		playback.push_frame(Vector2.ONE * sin(phase * TAU)) # Audio frames are stereo.
		phase = fmod(phase + increment, 1.0)
		to_fill -= 1


func _process(_delta):
	if playback != null: _fill_buffer()
	else: 
		print(playback)
		_ready()

func _ready():
	$Player.stream.mix_rate = sample_hz # Setting mix rate is only possible before play().
	playback = $Player.get_stream_playback()
	if playback != null: _fill_buffer() # Prefill, do before play() to avoid delay.
	$Player.play()

Log:

--- Debugging process started ---
Godot Engine v4.0.alpha14.official.106b68050 - https://godotengine.org
Vulkan API 1.2.0 - Using Vulkan Device #0: NVIDIA - NVIDIA GeForce RTX 2070 SUPER

[Object:null]
[Object:null]
8191
0
128
0
0
0
0
0
0
0
0
0
0
0
0
0
0
0

Steps to reproduce

Open Audio Generator Demo in any Godot 4,X version
https://github.com/godotengine/godot-demo-projects/tree/3.4-b0d4a7c/audio/generator

Minimal reproduction project

generator - issue.zip

@chtoedev
Copy link

chtoedev commented Sep 24, 2022

I can confirm this issue as well in Godot 4. I was able to resolve by setting Autoplay to True on the AudioStreamPlayer object -- rest of the code works fine after that change. I can also confirm when opening the tutorial project in Godot 3.4 they do NOT set Autoplay to True. Not sure if a bug or not, but hope this helps!

@chimeforest
Copy link

chimeforest commented Oct 20, 2022

I can confirm this also.

However, setting Autoplay to True does not really fix the problem for me. It gets rid of the error, but the sound will only play for a fraction of a second before cutting off. In Godot 3.5 the tone continues to play until you exit.

Godot v3.5.stable.official [991bb6a]
Godot v4.0.beta2.official [f8745f2]
Windows 10 Pro, Microsoft audioendpoint 10.0.19041.1

EDIT
I did a little more playing around. Here is the code that I used for the tests.
Code:

extends Node

var sample_hz = 44100/4.0 # Keep the number of samples to mix low, GDScript is not super fast.
var pulse_hz = 440.0
var phase = 0.0
var playback_is_not_null = false

var playback: AudioStreamPlayback = null # Actual playback stream, assigned in _ready().

func _ready():
	$Player.stream.mix_rate = sample_hz # Setting mix rate is only possible before play().
	playback = $Player.get_stream_playback()
	_try_fill()
	$Player.play()

func _process(_delta):
	_try_fill()

func _try_fill():
	if playback != null:
		if playback_is_not_null == false:
			print("Playback: ", playback)
			playback_is_not_null = true
		_fill_buffer()
	else:
		print("Playback: ", playback)

func _fill_buffer():
	var increment = pulse_hz / sample_hz

	var to_fill = playback.get_frames_available()
	print("To Fill: ", to_fill)
	while to_fill > 0:
		playback.push_frame(Vector2.ONE * sin(phase * TAU)) # Audio frames are stereo.
		phase = fmod(phase + increment, 1.0)
		to_fill -= 1

It prints out the playback object until it's not null.
Then it prints it one last time.
Then it prints out how much of the buffer needs filling.

Godot 4 Output without AutoPlay:

--- Debugging process started ---
Godot Engine v4.0.beta2.official.f8745f2f7 - https://godotengine.org
Vulkan API 1.2.198 - Using Vulkan Device #0: NVIDIA - GeForce GTX 1050
 
Playback: <Object#null>
Playback: <Object#null>
Playback: <Object#null>
Playback: <Object#null>
Playback: <Object#null>
Playback: <Object#null>
Playback: <Object#null>
Playback: <Object#null>
Playback: <Object#null>
Playback: <Object#null>
Playback: <Object#null>
Playback: <Object#null>
Playback: <Object#null>
Playback: <Object#null>
Playback: <Object#null>
Playback: <Object#null>
--- Debugging process stopped ---

Godot 4 Output with AutoPlay = true:

--- Debugging process started ---
Godot Engine v4.0.beta2.official.f8745f2f7 - https://godotengine.org
Vulkan API 1.2.198 - Using Vulkan Device #0: NVIDIA - GeForce GTX 1050
 
Playback: <AudioStreamGeneratorPlayback#-9223372011034639975>
To Fill: 32767
To Fill: 128
To Fill: 128
To Fill: 0
To Fill: 0
To Fill: 0
To Fill: 0
To Fill: 0
To Fill: 0
To Fill: 0
To Fill: 0
To Fill: 0
To Fill: 0
--- Debugging process stopped ---

Godot 3.5 Output:

--- Debugging process started ---
Godot Engine v3.5.stable.official.991bb6ac7 - https://godotengine.org
OpenGL ES 2.0 Renderer: GeForce GTX 1050/PCIe/SSE2
 
Playback: [AudioStreamGeneratorPlayback:1266]
To Fill: 8191
To Fill: 0
To Fill: 0
To Fill: 0
To Fill: 768
To Fill: 0
To Fill: 0
To Fill: 512
To Fill: 0
To Fill: 512
To Fill: 0
To Fill: 0
To Fill: 256
To Fill: 0
To Fill: 0
To Fill: 2304
--- Debugging process stopped ---

I think the problem in Godot 4 is two-fold.

  1. You shouldn't have to have AutoPlay on for it to work (Or there should be a way from code to instantiate it?).
  2. The buffer fills, but then is never emptied, so there are never any new spots to fill.. I think that's what is going on anyway...

Hopefully this helps somehow.

@Calinou Calinou added this to the 4.0 milestone Oct 20, 2022
@MegaKeegMan
Copy link

MegaKeegMan commented Nov 13, 2022

I have been playing with the audio generator as well. I also noticed that setting autoplay to true "fixed" the issue by allowing the audio to play for just a fraction of a second. However, I was able to get the demo to play a prolonged note by also commenting out the $Player.play() invocation, (in the demo of course).

Garuda Linux, Godot 4.0.beta3

@jon-heard
Copy link

jon-heard commented Jan 12, 2023

I’ve noted that the auto play workaround is spotty. If streaming a simple sine wave it’s about 50/50. When loading a file first, then starting to stream, it basically never works. I did get it to work at 50/50 by streaming silence until the file was loaded, then switching to that. It seems possible that the sooner you start streaming the more likely it is to work.

@seocwen
Copy link

seocwen commented Jan 18, 2023

I decided to take a deep dive into the problems I was having this afternoon and made a minimal reproduction project with some of the code I was tinkering with. I've included it below. Apologies if the evil music generated makes your ears bleed.

Audio.zip

In the default configuration, the project is likely to play on startup. Of note--like many of you, I've noticed that, on startup, sound may "chirp" briefly as the initial audio buffer is consumed and overtaken by permanent silence. However, in my experience, it's totally random whether generated audio chirps and dies or continues to play on in good health. I have found this is mostly affected by the sample rate.

For me

  • at 48000hz, my sample project usually plays (my project's default)
  • at 96000hz, my sample project rarely plays
  • at 24000hz, my sample project rarely breaks

On the other hand, I've had the sample rate as low as 4000hz and still had the audio randomly chirp and die. I haven't found any way to recover from this error either, so generated audio is a non-starter until it's fixed.

The length of the audio buffer, and how many samples it receives in ready() may also matter, but I might as well be talking about superstition and witchcraft. I might trying straining at the source code tomorrow to see if there are any obvious problems I can understandstand related to to the sample rate.

@RyianEEHunter
Copy link

As of Godot 4.0 beta 17 I can confirm that this is still an issue.

Working with the example project (https://github.com/godotengine/godot-demo-projects/tree/3.4-b0d4a7c/audio/generator), attempting to run the code as is will yield the following error:

E 0:00:01:0498 generator_demo.gd:27 @ _ready(): Player is inactive. Call play() before requesting get_stream_playback().
<C++ Error> Condition "stream_playbacks.is_empty()" is true. Returning: Ref()
<C++ Source> scene/audio/audio_stream_player.cpp:315 @ get_stream_playback()
generator_demo.gd:27 @ _ready()

Turning "Autoplay" to "On" on the Player node will result in what sounds like a single frame of audio being played.

If you then comment out "$Player.play()" on line 29, the tone will play as intended.

I also attempted to run the example linked in the comment above, but only got what sounded like a single frame to play.

I'm having similar issues with my own project.

To summarize, it seems like the issue is that the call to get the audio stream generator playback object requires that the audio already be playing before you fill the buffer with audio to be played. This is partially fixed by setting "Autoplay" to "On" since the audio will be playing when you try to get the object, but then the audio stops when you call "play()", hence why only a single frame of audio gets played for the linked projects.

Looking through the git history, my best guess is that this was broken when the check for "!stream_playbacks.is_empty()" was added before returning stream_playback. Unfortunately I'm not familiar enough with the architecture at this time to suggest a fix.

(Running Godot 4.0 beta 17 standard build for windows 64)

@reduz
Copy link
Member

reduz commented Feb 3, 2023

The problem here is that you need to call play() or use autoplay before using the stream. Because AudioStream is now polyphonic it can contain multiple ones.
Current master gives you the error above: Player is inactive. Call play() before requesting get_stream_playback()., so its not as confusing.

@RyianEEHunter
Copy link

I can confirm that in the example project if you move "$Player.play()" from line 29 up to above "playback = $Player.get_stream_playback()" the project works as intended. Additionally, if you leave the second "$Player.play()" command in place, it just chirps and dies. I think the cause is that as part of making AudioStream polyphonic the following was added to the play function in audio_stream_player.cpp:

if (stream->is_monophonic() && is_playing()) {
stop();
}

Meaning that if you call play twice (or autoplay and call play) the stream will be cut off.

@bluenote10
Copy link
Contributor

bluenote10 commented Feb 7, 2023

I'm trying to migrate my music project from Godot 3 to 4, and I'm running into a related issue. However, my problem is not that the get_stream_playback() returns null as described in the original post. My problem is probably more similar to what @seocwen described: It works randomly, currently roughly in 1 out of 20 starts I get some audio output. When it doesn't work, I only get a single buffer of audio output, and then silence.

I have a fully self-contained minimal reproduction example here on GitHub or here as AudioStreamBug.zip. The example doesn't require any scene tree, it's generating everything dynamically:

extends Node

var audio_stream_generator_playback: AudioStreamGeneratorPlayback
var i := 0

const SAMPLE_RATE = 44100.0


func _ready():
    var audio_stream_player = AudioStreamPlayer.new()
	
    var audio_stream_generator = AudioStreamGenerator.new()
    audio_stream_generator.set_mix_rate(SAMPLE_RATE)
    audio_stream_generator.set_buffer_length(0.1)
	
    audio_stream_player.set_stream(audio_stream_generator)
	
    add_child(audio_stream_player)
    audio_stream_player.play(0.0)
	
    audio_stream_generator_playback = audio_stream_player.get_stream_playback()


func _process(_delta):
	
    var frames_available = audio_stream_generator_playback.get_frames_available()
	
    if frames_available > 0:
        print("Filling buffer of length %s" % frames_available)
		
        var buffer = PackedVector2Array()
        buffer.resize(frames_available)

        var freq = 440.0

        for buffer_index in range(frames_available):
            var phase = 0.5 * sin(2.0 * PI * i / SAMPLE_RATE * freq)
            buffer[buffer_index] = Vector2(phase, phase);
            i += 1

        audio_stream_generator_playback.push_buffer(buffer)

When it works, the output logs is a constant stream of

[...]
Filling buffer of length 512
Filling buffer of length 512
Filling buffer of length 512
Filling buffer of length 512
Filling buffer of length 512
Filling buffer of length 512
Filling buffer of length 512
[...]

When it doesn't work, the output is just a single

Filling buffer of length 8191

And then nothing more. In other words, audio_stream_generator_playback.get_frames_available() only returns any frames available once, and from then point onward the number of available frames is just zero, i.e., my app never gets to fill the buffer again.

@bluenote10
Copy link
Contributor

I have investigated this a little bit and found out that the commit that broke the audio stream generator is afd2bba, i.e., this PR #55846. The commit immediately before (3017530) still works fine.

After skimming the code and adding some debug statements, I have a rough understanding of what is happening: The reason why it sometimes works and sometimes doesn't is a race condition between the audio server thread and the rendering thread. Typically user code calls playback.push_buffer() (if playback.get_frames_available() is non-zero) from the _process callback. The determining factor whether it works is whether the render thread (i.e., the _process function) gets to push something into buffer before AudioServer::_mix_step calls into AudioStreamPlaybackResampled::mix in the audio thread. If the _process function fills the buffer first, it works. However, if the audio server tries to "mix" from the playback while the buffer is still empty, it leads to an immediate erasure of the playback from that playback_list in the audio server. This happens because there is some logic that compares the requested number of frames with what actually has been produced by the playback. That's the crucial part of AudioServer::_mix_step:

unsigned int mixed_frames = playback->stream_playback->mix(&buf[LOOKAHEAD_BUFFER_SIZE], playback->pitch_scale.get(), buffer_size);
if (tag_used_audio_streams && playback->stream_playback->is_playing()) {
playback->stream_playback->tag_used_streams();
}
if (mixed_frames != buffer_size) {
// We know we have at least the size of our lookahead buffer for fade-out purposes.
float fadeout_base = 0.94;
float fadeout_coefficient = 1;
static_assert(LOOKAHEAD_BUFFER_SIZE == 64, "Update fadeout_base and comment here if you change LOOKAHEAD_BUFFER_SIZE.");
// 0.94 ^ 64 = 0.01906. There might still be a pop but it'll be way better than if we didn't do this.
for (unsigned int idx = mixed_frames; idx < buffer_size; idx++) {
fadeout_coefficient *= fadeout_base;
buf[idx] *= fadeout_coefficient;
}
AudioStreamPlaybackListNode::PlaybackState new_state;
new_state = AudioStreamPlaybackListNode::AWAITING_DELETION;
playback->state.store(new_state);
} else {

The buffer_size is always hard coded to 512 currently. Interestingly when calling stream_playback->mix in line 351 it seems to return 128 frames, even if the rendering thread has pushed nothing into the buffer so far (I still need to understand how that is supposed to work, and why it is always 128). Then in line 357 the comparison mixed_frames != buffer_size becomes 128 != 512, so we change the state of the playback to AWAITING_DELETION in line 369-370. Further down in AudioServer::_mix_step there is a cleanup functionality the erases playbacks from the playback_list that are in the AWAITING_DELETION/FADE_OUT_TO_DELETION.

switch (playback->state.load()) {
case AudioStreamPlaybackListNode::AWAITING_DELETION:
case AudioStreamPlaybackListNode::FADE_OUT_TO_DELETION:
playback_list.erase(playback, [](AudioStreamPlaybackListNode *p) {
delete p->prev_bus_details;
delete p->bus_details;
p->stream_playback.unref();
delete p;
});
break;
case AudioStreamPlaybackListNode::FADE_OUT_TO_PAUSE: {

So in summary, if the user thread doesn't manage to push something into the generator playback, the audio server will immediately remove the generator playback the first time it renders an audio buffer.


Now the question is what's the best option to resolve the issue. Since I've only read Godot's audio code for a few minutes now and don't understand the general design yet, I can only make some guesses. Would be great if one of the audio devs could chime in. Some thoughts:

  • Perhaps the implementation of AudioStreamPlaybackResampled::mix should be adapted to always return the number of requested frames, no matter what. This would mean that it would have to generate silence if it doesn't have the data available. Basically the typical buffer under-run effect.
  • Alternatively the audio server could use a smarter logic the determine when a playback needs to be erased. I.e., instead of just checking mixed_frames != buffer_size it could rather ask the playback "are you still playing?" or so. Perhaps this could even be implemented based on some get_stream_length function, i.e., finite streams could tell their finite lengths, while infinite streams could indicate that they are infinite by returning e.g. -1. Or perhaps just a playback_stream.is_infinite() getter. Some of these solutions need to be considered carefully, because the call into the playback would then come from the audio server, i.e., a different thread, so any kind of querying function needs to be thread safe.

The PR that broke the behavior (#55846) actually modified AudioStreamPlaybackResampled::mix, which could indicate that this is the right place to fix it (i.e., keep the mixed_frames != buffer_size in the audio server as it is). I don't really understand yet why this PR modified that function in the way it did, and why this change was necessary for fixing ogg edge cases. So simply reverting it would fix the generator playback, but presumably break something related to ogg which I don't understand yet.

@ellenhp
Copy link
Contributor

ellenhp commented Feb 12, 2023

I think your second proposal is the way to go to fix this. I think it's silly right now that it's returning an integer when really there are two things that the caller wants to know: How many frames did you mix, and are there more? Especially because you can't consistently infer either from the other.

@bluenote10 What do you think the behavior should be during an AudioStreamGenerator buffer underrun? The fade out logic is in the audio server and it's pretty tightly coupled to the lifecycle of an AudioStreamPlayback so I don't think it would be easy to add any fadein/out to soften a buffer underrun.

I think I remember seeing another bug go through my inbox that was caused by the same bit of code but I can't remember. Something about sounds doing something weird when the number of samples in them is evenly divided by the buffer size maybe?

@bluenote10
Copy link
Contributor

@ellenhp Thanks for the feedback!

What do you think the behavior should be during an AudioStreamGenerator buffer underrun?

I probably wouldn't do anything fancy. A buffer underrun always sounds bad, because even with a fade out + fade in, there is an audible interruption of the audio output. Since buffer underruns are therefore to be avoided with a high priority anyway, it is probably not worth the effort to do something complex. I.e., just outputting silence is as good/bad as anything.

I think your second proposal is the way to go to fix this. I think it's silly right now that it's returning an integer when really there are two things that the caller wants to know: How many frames did you mix, and are there more?

That would suggest to perhaps use a small struct to basically return a (num_frames_rendered, is_finished) tuple. On the other hand I'm wondering if that isn't redundant to the information provided by playback->stream_playback->is_playing(). I'm wondering if we could simple wrap that part that changes the state to AWAITING_DELETION under an if on that "is playing" as a very naive fix (the generator could always return true to stay alive, but not sure if it means it can never be deleted at all).

Currently my understanding of the audio system is much too shallow though to really judge what is best here. I hope I can find some time to do further experiments / code reading...

@ellenhp
Copy link
Contributor

ellenhp commented Feb 12, 2023

On the other hand I'm wondering if that isn't redundant to the information provided by playback->stream_playback->is_playing(). I'm wondering if we could simple wrap that part that changes the state to AWAITING_DELETION under an if on that "is playing" as a very naive fix

This makes sense.

(the generator could always return true to stay alive, but not sure if it means it can never be deleted at all).

This would probably just be fine, because you can still just call stop() to begin deletion I think. I don't have the code in front of me but I'm 90% sure that would work. If you play() a generator stream and then never fill it, it will stay in the playing state forever (until stop()) but I don't really think that's a problem.

I've been pretty out of the loop with what's going on with releases and stuff so I don't know the criteria for inclusion of a bugfix in the RC's, but this may (?) have to wait for a 4.0.x patch release because it could be a medium-risk change depending on how it's implemented. I'm up for trying to fix it today though, this piece of code has been bugging me for a while.

@ellenhp
Copy link
Contributor

ellenhp commented Feb 12, 2023

Also I wanted to add that this is great work on your part @bluenote10 tracking down the source of this issue. I scrolled up to the top and was surprised to see the way this issue ended up presenting itself. The cause is not at all obvious.

@ellenhp
Copy link
Contributor

ellenhp commented Feb 12, 2023

The simplest thing ended up working. @bluenote10 could you test #73162?

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