Move mixing out of the AudioStreamPlayer* nodes #51296

ellenhp · 2021-08-05T23:10:03Z

Implements godotengine/godot-proposals#2299

My focus here is parity with current master, not to add features, but it's still an enormous change. I can probably split off the individual commits into their own PRs if people would prefer that.

More detail is in the proposal above, but this basically moves all mixing into the audio server. Audio streams register their playback objects with the audio server and then update the volume during their panning or spatialization steps. This eliminates an entire class of bugs in which the player node's state is updated and it can't continue a volume ramp or fade out.

This also opens the door to lots of really exciting things! Now that we can crossfade across audio buses, we can have Area2D/Area3D based bus overrides that smoothly transition to the new bus. Imagine the mouth of a cave that slowly fades reverb in, or an area of a 2d platformer level that adds a distortion to audio. Before these things would cause pops and abrupt transitions, but with this change the pops are gone and it'll be fairly easy to add smooth transitions.

Another thing we can add back is polyphony, which I think users will appreciate. An audio stream player node can simply instance multiple playbacks for a stream and send them all to the audio server which will mix them independently. That's also not included in this change but should be simple to add.

A few notes:

I changed the mix buffer size to 512, which I think is fine. It should probably be a project setting or at least influenced by the audio driver's buffer size, i.e. based on the latency project setting. It doesn't make sense for the audio server to have a bigger buffer than the audio driver.
The NO_THREADS implementation of my SafeList data structure is probably broken, but I don't really know how to test it given that web exports aren't working in 4.0 right now, unless I'm mistaken on that.
This PR contains a mediocre workaround for AudioStreamPlayer2D doesn't play any audio when played #50958 that I put in for testing purposes only. I'm basically including the current viewport in the list of viewports to iterate through while looking for the active listener. It works for simple scenes but we should still address AudioStreamPlayer2D doesn't play any audio when played #50958 somehow.

ellenhp · 2021-08-08T15:18:26Z

This is the project I've been using to test: audioservertest.zip

It's probably good to test this on other more complex projects too.

akien-mga

Great work!

I left some style nitpicks and only skimmed through the detailed code changes, but it looks pretty solid!

akien-mga · 2021-08-17T19:52:18Z

core/templates/safe_list.h

+//   instead the node will be deallocated at a later time when it is safe to do so.
+// - No blocking synchronization primitives will be used.
+
+// This is used in very specific areas of the engine where it's critical that these guarantees are held


Suggested change

// This is used in very specific areas of the engine where it's critical that these guarantees are held

// This is used in very specific areas of the engine where it's critical that these guarantees are held.

akien-mga · 2021-08-17T19:52:53Z

core/templates/safe_list.h

+	struct SafeListNode {
+		std::atomic<SafeListNode *> next = nullptr;
+
+		// If the node is logically deleted, this pointer will typically point to the previous list item in time that was also logically deleted.


We don't have a line length limit but for long comments it can be worth wrapping manually:

Suggested change

// If the node is logically deleted, this pointer will typically point to the previous list item in time that was also logically deleted.

// If the node is logically deleted, this pointer will typically point

// to the previous list item in time that was also logically deleted.

akien-mga · 2021-08-17T19:54:58Z

core/templates/safe_list.h

+				// Fall through upon failure, try again.
+			}
+		}
+		// Then queue it for deletion by putting it in the node graveyard. Don't touch `next` because an iterator might still be pointing at this node.


Suggested change

// Then queue it for deletion by putting it in the node graveyard. Don't touch `next` because an iterator might still be pointing at this node.

// Then queue it for deletion by putting it in the node graveyard.

// Don't touch `next` because an iterator might still be pointing at this node.

a few other comments that might be worth wrapping, I'd recommend keeping line length at max 100-120 chars for comments.

akien-mga · 2021-08-17T19:57:12Z

core/templates/safe_list.h

+	}
+};
+
+#else


I'd suggest putting a comment to remind what this #else is about:

Suggested change

#else

#else // NO_THREADS

akien-mga · 2021-08-17T19:57:50Z

scene/2d/audio_stream_player_2d.cpp


-			int new_output_count = 0;
+		//stop playing if no longer active


Suggested change

//stop playing if no longer active

// Stop playing if no longer active.

akien-mga · 2021-08-17T20:11:54Z

servers/audio/audio_stream.cpp

 	} else {
 		for (int i = 0; i < p_frames; i++) {
 			p_buffer[i] = AudioFrame(0, 0);
 		}
+		// TODO does this make sense?


I thought about it for a while, and I think it does make sense :) Removed as a todo.

akien-mga · 2021-08-17T20:12:44Z

servers/audio_server.cpp

 #include "servers/audio/effects/audio_effect_compressor.h"
+#include <cstring>


Suggested change

#include "servers/audio/effects/audio_effect_compressor.h"

#include <cstring>

#include "servers/audio/effects/audio_effect_compressor.h"

#include <cstring>

akien-mga · 2021-08-17T20:13:49Z

servers/audio_server.h

+		AUDIO_DATA_INVALID_ID = -1,
+		MAX_CHANNELS_PER_BUS = 4,
+		MAX_BUSES_PER_PLAYBACK = 6,
+		LOOKAHEAD_BUFFER_SIZE = 32


Suggested change

LOOKAHEAD_BUFFER_SIZE = 32

LOOKAHEAD_BUFFER_SIZE = 32,

Trailing comma can ensure that future additions won't generate unnecessary line diff.

akien-mga · 2021-08-17T20:15:27Z

servers/audio_server.h

+		std::atomic<float> setseek = -1;
+		std::atomic<float> pitch_scale = 1;
+		std::atomic<float> highshelf_gain = 0;
+		std::atomic<float> attenuation_filter_cutoff_hz = 0; // This isn't used unless highshelf_gain is nonzero.


I know we tend to use our own SafeNumeric instead of std::atomic (which wraps std::atomic), but I'm not familiar with its semantics and whether it would actually make sense here. CC @RandomShaper

That's right. SafeNumeric<T> is intended to be the Godot "STL" counterpart of std::atomic. It should fit most cases, where acquire-release semantics is the right choice.

Elaborating on that, I see atomic are being used throughout this PR will the default memory ordering semantics (sequential). Even if we didn't have SafeNumeric, it'd be good to check if acquire-release is enough, because, AFAIK, that is more performant on certain architectures.

I think acquire-release is probably fine everywhere except in SafeList but I find it very difficult to reason about these things so I'm not sure. I do think a system-wide barrier of some sort is appropriate during insertions or removals to SafeList because they tend to be fairly rare and the penalty if something goes wrong could be a double free or a use after free. These scalar values can absolutely have weaker guarantees.

The main thing we're attempting to avoid here is a sleeping on mutex contention, right? A memory barrier is not in the same league performance-wise as sleeping the audio thread.

Apparently a compare-swap operation is about as expensive as a cache miss on x86 server platforms. I'm curious about other architectures but even if it were 100x more expensive than a cache miss I don't think I'd consider moving to another memory model for SafeList given how rarely insertions and deletions happen. I'm also hesitant to optimize without profiling results.

Side-note, thank you so much for reviewing this @RandomShaper. I know it's a difficult PR to review and I appreciate it. :)

No worries! It's also hard for me to reason about every possible case. I just have the mindset of use acq-rel by default, which is already stronger than certain cases would need, but it still allows for more perfomant load/stores on some architectures than sequential would cause. When it's about compare-exchange, to be honest, I still haven't studied much about the performane consequences of using one memory ordering or the other.

In the case of SafeList, I can't but agree that the wisest thing is to leave sequential ordering.

This said, the only thing I'd ask you to do is switch to SafeNumeric in the other cases.

Makes sense! I'll do that later today. I think I also want to get my NO_THREADS implementation of the SafeList class to a better state than it is right now. I haven't tested it so it might be broken and I don't want to check in broken code. I can ping you when everything is done if you'd like?

I'm glad I'm not the only one who finds this sort of thing difficult. Writing the SafeList class nearly broke my brain. Lock-free programming is hard. 🤣

Sure, feel free to ping me.

Yeah, lockless is hell. I wouldn't dare myself to write a lockless data structure unless there's no other option, like basing it on some third party one known to be good.

@RandomShaper Ready for another review. I've had lots of trouble thinking about whether or not the remaining atomic usages are okay for acquire-release. The scalar values are definitely okay to be SafeNumerics, but other than those, all remaining usages of the default atomic behavior impact control flow in non-trivial ways. If you'd like, I can switch the playback state enum over to be acquire-release, but I'd prefer to keep the bus details pointer as-is. I should be honest though, it's fear of double-frees or other weird behavior that's making me say this, not any sort of logic or reason. :)

akien-mga · 2021-08-17T20:16:00Z

servers/audio_server.h

+	bool get_is_playback_active(Ref<AudioStreamPlayback> p_playback);
+	float get_playback_position(Ref<AudioStreamPlayback> p_playback);
+	bool get_is_playback_paused(Ref<AudioStreamPlayback> p_playback);


For boolean getters, we usually start directly with is_ and not with get_is_.

ellenhp · 2021-08-18T02:05:55Z

Made the changes Akien pointed out and rebased. I also added a preprocessor warning in the NO_THREADS implementation of SafeList because it occurred to me that this could cause some frustrating bugs in the future if nobody is aware of it. I used a pattern that I've seen elsewhere in the codebase to avoid throwing an error on MSVC toolchains. I just want it documented in the code that NO_THREADS SafeLists are untested so someone debugging can identify issues quickly. Hopefully that's fine.

I also wanted to ask about whether my silly workaround in AudioStreamPlayer2D is appropriate to keep in this PR. I left it in for ease of testing but I don't think it's the correct solution for #50958.

RandomShaper · 2021-08-20T08:04:49Z

servers/audio_server.h

+		// Updating this ref after the list node is created breaks consistency guarantees, don't do it!
+		Ref<AudioStreamPlayback> stream_playback;
+		// Playback state determines the fate of a particular AudioStreamListNode during the mix step. Must be atomically replaced.
+		std::atomic<PlaybackState> state = AWAITING_DELETION;


I guess this one can become a SafeNumeric, too, with the same provision about checking that acquire-release is fine.

RandomShaper · 2021-08-20T08:06:11Z

servers/audio_server.h


 	struct CallbackItem {
+		std::atomic<CallbackItem *> next;


In this case, since we don't have a SafePointer<T>, using std::atomic directly is fine. If we find many cases of atomic pointers sometime in the future, we can always introduce it.

In any case, again, it'd be good to check if acquire-release (or even something weaker) can be used intead of sequential in the operations.

Mentioned above, but I'm not entirely comfortable switching over to acquire release here. I'm not convinced it would be a problem, but I can't convince myself it's safe either.

Wow nevermind! These atomics can be removed. They're dead code leftover from before I factored out SafeList into its own file.

AndreaCatania · 2021-08-28T08:55:16Z

Hello, when I play the game I've a segmentation fault at this line: https://github.com/ellenhp/godot/blob/3598d300cb43797a4f18b34d921875d060ce7de7/servers/audio_server.cpp#L1237 which was added by this PR. Any idea how to fix it?

handle_crash: Program crashed with signal 11
Engine version: Godot Engine v4.0.dev.custom_build (e6e5502a165d34762e29203658dbfd624a5bb9bd)
Dumping the backtrace. Please include this when reporting the bug on https://github.com/godotengine/godot/issues
[1] /lib64/libc.so.6(+0x3d320) [0x7ffff7a87320] (??:0)
[2] AudioServer::set_playback_all_bus_volumes_linear(Ref<AudioStreamPlayback>, Vector<AudioFrame>) (/home/andrea/Workspace/momento/wander/source/engine/godot/servers/audio_server.cpp:1237)
[3] AudioStreamPlayer::set_volume_db(float) (/home/andrea/Workspace/momento/wander/source/engine/godot/scene/audio/audio_stream_player.cpp:98 (discriminator 4))
[4] void call_with_variant_args_helper<__UnexistingClass, float, 0ul>(__UnexistingClass*, void (__UnexistingClass::*)(float), Variant const**, Callable::CallError&, IndexSequence<0ul>) (/home/andrea/Workspace/momento/wander/source/engine/godot/./core/variant/binder_common.h:227 (discriminator 4))
[5] void call_with_variant_args_dv<__UnexistingClass, float>(__UnexistingClass*, void (__UnexistingClass::*)(float), Variant const**, int, Callable::CallError&, Vector<Variant> const&) (/home/andrea/Workspace/momento/wander/source/engine/godot/./core/variant/binder_common.h:370)
[6] MethodBindT<float>::call(Object*, Variant const**, int, Callable::CallError&) (/home/andrea/Workspace/momento/wander/source/engine/godot/./core/object/method_bind.h:286)
[7] ClassDB::set_property(Object*, StringName const&, Variant const&, bool*) (/home/andrea/Workspace/momento/wander/source/engine/godot/core/object/class_db.cpp:1189)
[8] Object::set(StringName const&, Variant const&, bool*) (/home/andrea/Workspace/momento/wander/source/engine/godot/core/object/object.cpp:407)
[9] Variant::set_named(StringName const&, Variant const&, bool&) (/home/andrea/Workspace/momento/wander/source/engine/godot/core/variant/variant_setget.cpp:234)
[10] GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Callable::CallError&, GDScriptFunction::CallState*) (/home/andrea/Workspace/momento/wander/source/engine/godot/modules/gdscript/gdscript_vm.cpp:947)
[11] GDScriptInstance::call(StringName const&, Variant const**, int, Callable::CallError&) (/home/andrea/Workspace/momento/wander/source/engine/godot/./modules/gdscript/gdscript.cpp:1486)
[12] Node::_gdvirtual__process_call(double) (/home/andrea/Workspace/momento/wander/source/engine/godot/scene/main/node.h:221 (discriminator 5))
[13] Node::_notification(int) (/home/andrea/Workspace/momento/wander/source/engine/godot/scene/main/node.cpp:59)
[14] Node::_notificationv(int, bool) (/home/andrea/Workspace/momento/wander/source/engine/godot/./scene/main/node.h:48 (discriminator 14))
[15] Object::notification(int, bool) (/home/andrea/Workspace/momento/wander/source/engine/godot/core/object/object.cpp:843)
[16] SceneTree::_notify_group_pause(StringName const&, int) (/home/andrea/Workspace/momento/wander/source/engine/godot/scene/main/scene_tree.cpp:851)
[17] SceneTree::process(double) (/home/andrea/Workspace/momento/wander/source/engine/godot/scene/main/scene_tree.cpp:455 (discriminator 2))
[18] Main::iteration() (/home/andrea/Workspace/momento/wander/source/engine/godot/main/main.cpp:2554)
[19] OS_LinuxBSD::run() (/home/andrea/Workspace/momento/wander/source/engine/godot/platform/linuxbsd/os_linuxbsd.cpp:342)
[20] /home/andrea/Workspace/momento/wander/source/engine/godot/bin/godot.linuxbsd.tools.64(main+0x144) [0x24ad95a] (/home/andrea/Workspace/momento/wander/source/engine/godot/platform/linuxbsd/godot_linuxbsd.cpp:60)
[21] /lib64/libc.so.6(__libc_start_main+0xd5) [0x7ffff7a71b75] (??:0)
[22] /home/andrea/Workspace/momento/wander/source/engine/godot/bin/godot.linuxbsd.tools.64(_start+0x2e) [0x24ad75e] (??:?)
-- END OF BACKTRACE --
================================================================

AndreaCatania · 2021-08-28T09:24:44Z

I spot the issue, the following code playback_node->bus_details.load() is returning a nullptr:

		if (playback_node->bus_details.load()->bus_active[bus_idx]) {
			map[playback_node->bus_details.load()->bus[bus_idx]] = p_volumes;
		}

so it crashes. To fix it we should check it in this way:

	if (playback_node->bus_details.load()) {
		if (playback_node->bus_details.load()->bus_active[bus_idx]) {
			map[playback_node->bus_details.load()->bus[bus_idx]] = p_volumes;
		}
	}

Even if this code fix the issue momentarily on my project, it still unsafe. Atomic pointers, if not under some well documented and strict conditions, are unsafe since the check is guaranteed only the moment of the first if; any subsequent call may cause a segmentation fault, since the pointer can suddenly change.
To fix this issue, we need to use a mutex, or make sure that by design the pointer can't ever change at that point.

Please let me know if you need that I open an issue, I'm also available to test an eventual fix.

ellenhp · 2021-08-28T15:07:10Z

Good catch. This can be easily made to be safe, it was just an oversight on my part.

ellenhp · 2021-08-28T15:24:03Z

Maybe not as easily as I had hoped. Would you mind making an issue because this might need further discussion and I'd like to be able to point to something other than an enormous PR when I submit a fix.

ellenhp · 2021-08-28T15:58:52Z

@AndreaCatania would you mind testing #52187?

KoBeWi added enhancement topic:audio labels Aug 5, 2021

KoBeWi added this to the 4.0 milestone Aug 5, 2021

ellenhp changed the title ~~Mix audio in the audio server~~ Move mixing out of the AudioStreamPlayback* nodes Aug 5, 2021

ellenhp force-pushed the mix_in_audio_server branch 5 times, most recently from 439cd45 to b13e7fd Compare August 8, 2021 14:37

ellenhp marked this pull request as ready for review August 8, 2021 14:59

ellenhp requested review from a team as code owners August 8, 2021 14:59

ellenhp force-pushed the mix_in_audio_server branch from b13e7fd to ae5572e Compare August 8, 2021 17:23

ellenhp mentioned this pull request Aug 17, 2021

Add volume L/R controls to AudioStreamPlayer (master) #51667

Closed

reduz approved these changes Aug 17, 2021

View reviewed changes

akien-mga reviewed Aug 17, 2021

View reviewed changes

ellenhp force-pushed the mix_in_audio_server branch 3 times, most recently from 3d9a87f to 0a48adf Compare August 18, 2021 01:58

RandomShaper reviewed Aug 20, 2021

View reviewed changes

Do all audio mixing in the AudioServer

3598d30

ellenhp force-pushed the mix_in_audio_server branch from bb1b011 to 3598d30 Compare August 27, 2021 17:42

reduz merged commit 54caaa2 into godotengine:master Aug 27, 2021

ellenhp deleted the mix_in_audio_server branch August 28, 2021 04:32

ellenhp mentioned this pull request Aug 28, 2021

AudioStreamPlayer2D doesn't play any audio when played #50958

Closed

KoBeWi mentioned this pull request Aug 28, 2021

Add a reverse option to AudioStreamPlayer godotengine/godot-proposals#3207

Open

ellenhp mentioned this pull request Aug 28, 2021

Don't put nullptr into bus details pointer, fixes segfault on volume change #52187

Closed

Paulb23 mentioned this pull request Sep 1, 2021

Support SceneTree and signal testing - Code edit unit tests #50375

Merged

Calinou mentioned this pull request Sep 17, 2021

Remove unused out_of_range_mode property from AudioStreamPlayer3D #52774

Merged

This was referenced Oct 9, 2021

AudioStreamPlayer: play() resets pitch_scale back to 1 #53585

Closed

Allow AudioStreamPlayer(2D) to provide pitch_scale on playback #53641

Merged

Calinou mentioned this pull request Nov 16, 2021

Audio clips when changing volume of audio bus #32882

Open

jitspoe mentioned this pull request Nov 17, 2021

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

Closed

Calinou mentioned this pull request Dec 30, 2021

AudioStreamPlayer emits finished signal inconsistently. #56156

Open

Calinou mentioned this pull request Feb 12, 2022

Add a property to adjust spatial stereo panning on AudioStreamPlayer2D and AudioStreamPlayer3D godotengine/godot-proposals#3384

Closed

ellenhp mentioned this pull request Mar 22, 2022

[3.x] instance audio streams before AudioServer::lock call #59413

Merged

reduz mentioned this pull request Feb 12, 2023

Fix AudioStreamGenerator stopping playback after a buffer underrun #73162

Closed

rsubtil mentioned this pull request Apr 5, 2023

MP3 importer works incorrectly #75223

Open

AThousandShips mentioned this pull request Sep 9, 2023

Remove some dead declarations in audio_stream_player.h #81485

Merged

bluenote10 mentioned this pull request Sep 10, 2023

Fix audio stream generators getting freed accidentally #81508

Merged

ellenhp changed the title ~~Move mixing out of the AudioStreamPlayback* nodes~~ Move mixing out of the AudioStreamPlayer* nodes Sep 10, 2023

bluenote10 mentioned this pull request Sep 24, 2023

instances leaked at exit with AudioStreamPlayer #76745

Open

jauwnio mentioned this pull request Feb 8, 2024

Audio files distort inside Godot when using a Focusrite DAC #75109

Closed

ellenhp mentioned this pull request Mar 26, 2024

Audio input gets muted after a while on android #86428

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move mixing out of the AudioStreamPlayer* nodes #51296

Move mixing out of the AudioStreamPlayer* nodes #51296

ellenhp commented Aug 5, 2021 •

edited

Loading

ellenhp commented Aug 8, 2021

akien-mga left a comment

akien-mga Aug 17, 2021

akien-mga Aug 17, 2021

akien-mga Aug 17, 2021

akien-mga Aug 17, 2021

akien-mga Aug 17, 2021

akien-mga Aug 17, 2021

akien-mga Aug 17, 2021

ellenhp Aug 18, 2021

akien-mga Aug 17, 2021

akien-mga Aug 17, 2021

akien-mga Aug 17, 2021

RandomShaper Aug 20, 2021

RandomShaper Aug 20, 2021

ellenhp Aug 21, 2021

ellenhp Aug 21, 2021

RandomShaper Aug 21, 2021

ellenhp Aug 21, 2021 •

edited

Loading

RandomShaper Aug 21, 2021

ellenhp Aug 25, 2021

akien-mga Aug 17, 2021

ellenhp commented Aug 18, 2021

RandomShaper Aug 20, 2021

RandomShaper Aug 20, 2021

ellenhp Aug 25, 2021

ellenhp Aug 25, 2021

AndreaCatania commented Aug 28, 2021

AndreaCatania commented Aug 28, 2021 •

edited

Loading

ellenhp commented Aug 28, 2021

ellenhp commented Aug 28, 2021

ellenhp commented Aug 28, 2021

	// This is used in very specific areas of the engine where it's critical that these guarantees are held
	// This is used in very specific areas of the engine where it's critical that these guarantees are held.

	// If the node is logically deleted, this pointer will typically point to the previous list item in time that was also logically deleted.
	// If the node is logically deleted, this pointer will typically point
	// to the previous list item in time that was also logically deleted.

	// Then queue it for deletion by putting it in the node graveyard. Don't touch `next` because an iterator might still be pointing at this node.
	// Then queue it for deletion by putting it in the node graveyard.
	// Don't touch `next` because an iterator might still be pointing at this node.

+              	}
+              };
+              #else

	//stop playing if no longer active
	// Stop playing if no longer active.

		#include "servers/audio/effects/audio_effect_compressor.h"
		#include <cstring>

Move mixing out of the AudioStreamPlayer* nodes #51296

Move mixing out of the AudioStreamPlayer* nodes #51296

Conversation

ellenhp commented Aug 5, 2021 • edited Loading

ellenhp commented Aug 8, 2021

akien-mga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellenhp Aug 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellenhp commented Aug 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreaCatania commented Aug 28, 2021

AndreaCatania commented Aug 28, 2021 • edited Loading

ellenhp commented Aug 28, 2021

ellenhp commented Aug 28, 2021

ellenhp commented Aug 28, 2021

ellenhp commented Aug 5, 2021 •

edited

Loading

ellenhp Aug 21, 2021 •

edited

Loading

AndreaCatania commented Aug 28, 2021 •

edited

Loading