Random crash in AudioStreamPlaybackRandomPitch::start / AudioStreamPlayer3D::_mix_audio

[jitspoe]

Godot version

3.3.4 stable and 3.4 stable

System information

Windows 10 and Linux

Issue description

Getting occasional random crashes. Here's the callstack in Linux

(gdb) bt
#0  0x0000000000000000 in ?? ()
#1  0x00000000028a3c2e in AudioStreamPlaybackRandomPitch::start (
    this=0x7d394d0, p_from_pos=0) at servers/audio/audio_stream.cpp:307
#2  0x0000000001e00f00 in AudioStreamPlayer3D::_mix_audio (this=0xeb6b690)
    at scene/3d/audio_stream_player_3d.cpp:147
#3  0x0000000001e0bc83 in AudioStreamPlayer3D::_mix_audios (self=0xeb6b690)
    at scene/3d/audio_stream_player_3d.h:119
#4  0x00000000025e38ed in AudioServer::_mix_step (this=0x65d34a0)
    at servers/audio_server.cpp:337
#5  0x00000000025e3277 in AudioServer::_driver_process (this=0x65d34a0, 
    p_frames=512, p_buffer=0x6868cb0) at servers/audio_server.cpp:252
#6  0x00000000025e2411 in AudioDriver::audio_server_process (
    this=0x7fffffffd728, p_frames=512, p_buffer=0x6868cb0, 
    p_update_mix_time=true) at servers/audio_server.cpp:62
#7  0x00000000018fa082 in AudioDriverPulseAudio::thread_func (
    p_udata=0x7fffffffd728)
    at drivers/pulseaudio/audio_driver_pulseaudio.cpp:389
#8  0x0000000002bee5e7 in Thread::callback (p_self=0x7fffffffd768, 
    p_settings=..., 
    p_callback=0x18f9f4a <AudioDriverPulseAudio::thread_func(void*)>, 
    p_userdata=0x7fffffffd728) at core/os/thread.cpp:77

Also happens in Windows, but I haven't been able to reproduce it with the debugger attached.

This seems like it might be a threading issue related to #52010

Steps to reproduce

Happens at random with my game FWOP: https://jitspoe.itch.io/fwop

I think it might be related to setting the stream on an AudioStreamPlayer3D too frequently. Possibly on the same frame? The fish plays juicy thwack impact sounds when the rigid bodies hit things, randomly swapping out the streams for variety, and the physics runs at 240fps, so it might have multiple impacts on one render frame. Might be possible to reproduce this more consistently by having multiple stream players swapping out streams and playing at a really high rate.

Minimal reproduction project

It's not minimal, but since @akien-mga already called me out on it, I'm just going to post FWOP as my minimal reproduction project. 😆 https://github.com/jitspoe/floppy_fish/tree/main/floppy_fish_godot

Edit: Minimal repro project attached here: https://github.com/godotengine/godot/files/7550741/StreamRandomPitchCrash.zip

Workaround

Until this crash is fixed, you can work around it simply by not using the AudioStreamRandomPitch and instead just set the pitch_scale before playing a stream, like so:

		stream_player.pitch_scale = rand_range(0.9, 1.0)
		stream_player.play()

[akien-mga]

CC @godotengine/audio

[jitspoe]

@Waridley suggested this change might be relevant: #46302 (Looks like it's only in mainline, not 3.x).

[jitspoe]

AudioStreamPlayer3D::set_stream() calls AudioServer::get_singleton()->lock();, however, AudioStreamRandomPitch::set_audio_stream() does not, so it's highly likely that setting the stream in the random pitch while it's being actively played causes the crash.

[jitspoe]

I've created a minimal repro project here. Still doesn't crash consistently, but you don't have to flop around anymore and can just leave it running:

StreamRandomPitchCrash.zip

[jitspoe]

Confirmed on windows -- it's the AudioStreamPlaybackRandomPitch.

Exception thrown at 0x656A624F in godot.windows.opt.tools.32.exe: 0xC0000005: Access violation executing location 0x656A624F.

godot.windows.opt.tools.32.exe!AudioStreamPlaybackRandomPitch::start(float p_from_pos) Line 309 C++
godot.windows.opt.tools.32.exe!AudioStreamPlayer3D::_mix_audio() Line 148 C++
godot.windows.opt.tools.32.exe!AudioServer::_mix_step() Line 337 C++

[ellenhp]

@Waridley suggested this change might be relevant: #46302 (Looks like it's only in mainline, not 3.x).

This change was never merged, but now we have SafeRef for any use-case that requires an atomic ref assignment.

[jitspoe]

So in master (4.x), the AudioStreamPlayer3D has been changed to just:

void AudioStreamPlayer3D::set_stream(Ref<AudioStream> p_stream) {
	stop();
	stream = p_stream;
}

Previously:

void AudioStreamPlayer3D::set_stream(Ref<AudioStream> p_stream) {
	AudioServer::get_singleton()->lock();

	mix_buffer.resize(AudioServer::get_singleton()->thread_get_mix_buffer_size());

	if (stream_playback.is_valid()) {
		stream_playback.unref();
		stream.unref();
		active.clear();
		setseek.set(-1);
	}

	if (p_stream.is_valid()) {
		stream = p_stream;
		stream_playback = p_stream->instance_playback();
	}

	AudioServer::get_singleton()->unlock();

	if (p_stream.is_valid() && stream_playback.is_null()) {
		stream.unref();
	}
}

But the AudioStreamRandomPitch has not changed. It's still:

void AudioStreamRandomPitch::set_audio_stream(const Ref<AudioStream> &p_audio_stream) {
	audio_stream = p_audio_stream;
	if (audio_stream.is_valid()) {
		for (Set<AudioStreamPlaybackRandomPitch *>::Element *E = playbacks.front(); E; E = E->next()) {
			E->get()->playback = audio_stream->instance_playback();
		}
	}
}

So I'm wondering if the solution is to put locks around it in 3.x and change it to avoid setting the playback directly in 4.x or what. I could submit a pr to put locks around both of them for now.

[jitspoe]

Just went ahead and created a pull request here: #55043

Attempting to reproduce the crash in 4.x at the moment.

Here's the (slightly modified) project for 4.x:
StreamRandomPitchCrash4x.zip

I haven't reproduced it so far, even without the fix, but since it's rare and random, it's hard to say if it will reproduce in a reasonable time or not. Wanted to test before and after to verify the crash and fix, but I went ahead and put the pull request up for now. Seems to fix it for 3.x at least.

[ellenhp]

Looks like I was wrong about SafeRef existing? I guess I imagined that.

AudioStreamRandomPitch serves a fundamentally different role than the stream players so I don't think comparing them makes a ton of sense. I'm guessing this is a race between the audio thread and the main thread.

Specifically I think AudioStreamRandomPitch::start is racing with AudioStreamRandomPitch::set_audio_stream and that assignments to the playback reference are being interleaved causing a use after free. We generally try to avoid locks on the audio thread but I guess I wouldn't be opposed to a lock here as long as it's held only long enough to perform a reference assignment. If we hold the lock long enough that the OS might sleep the thread then we might cause underruns.

Please use a mutex local to AudioStreamRandomPitch, not the global audio server lock.

[ellenhp]

I'd love if you could run your automated repro project overnight on whatever PR you come up with by the way. I don't want to merge something unless we're reasonably sure it solves the issue.

[jitspoe]

I've been running the fix on 3.x for hours without a crash. Crashed fairly regularly before. Though I'm using the global audio server lock rather than a local mutex as you just suggested.

I have yet to reproduce the crash in 4.x, even without my fix, so I'm not sure if it's just luck, or some fundamental changes to the audio behavior have caused this to no longer happen. Will be difficult to determine if a change fixes the potential(?) crash there.

Honestly, I'm wondering if it's even worth keeping AudioStreamRandomPitch in 4.0 considering it's literally 1 line of gdscript to achieve the same functionality.

[ellenhp]

I've wondered the same thing. I opened #52592 to improve its functionality but removing it altogether is also an option.

…

On Tue, Nov 16, 2021 at 7:42 PM jitspoe ***@***.***> wrote: I've been running the fix on 3.x for hours without a crash. Crashed fairly regularly before. Though I'm using the global audio server lock rather than a local mutex as you just suggested. I have yet to reproduce the crash in 4.x, even without my fix, so I'm not sure if it's just luck, or some fundamental changes to the audio behavior have caused this to no longer happen. Will be difficult to determine if a change fixes the potential(?) crash there. Honestly, I'm wondering if it's even worth keeping AudioStreamRandomPitch in 4.0 considering it's literally 1 line of gdscript to achieve the same functionality. — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#54866 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOY263W7SFUIPMWMEP44KLUMMQCDANCNFSM5HZ3EGJQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[jitspoe]

Something to randomly select sounds and have pitches and whatnot makes more sense than just randomizing pitches. There was also a proposal for a full node based system, which might cover all this and more.

I still haven't been able to reproduce the crash on 4.x. The AudioStreamPlayer3D no longer handles the mix, so if it is possible to reproduce it, the callstack would be different. Perhaps in the rewrite to do the mixing outside of the stream players, this crash is fixed, and we only need a solution for 3.x.

Edit: Oh, that was actually your change: #51296

So we could either try to pull that to 3.x or put a lock around the set_audio_stream just in 3.x. There may not need to be any fix applied to master/4.x.

[ellenhp]

Backporting #51296 isn't an option. I'm not opposed to holding a lock for a single reference assignment. Locks have fairly low performance penalties unless there's contention, and if there is contention then we get to decide if we want to crash or risk an underrun. Personally I'll take the underrun every time. reduz is more anti-mutex than I am, but I'd argue pretty strongly that for very low contention scenarios like this one it really doesn't matter. Either the lock is there and you waste a few/dozen/hundred nanoseconds every mix or the lock isn't there and you crash every few hours. A few hours might seem like a long time for reproducing a bug but think about the last time you got really into a video game. A few hours is nothing for some people when they're really interested. We shouldn't boot them out of the experience and end their fun just to save a few nanoseconds in my opinion.

[lawnjelly]

Fixed by #96261.