-
-
Notifications
You must be signed in to change notification settings - Fork 22.4k
Implement audio absolute time (DSP time) and scheduled play #105510
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
base: master
Are you sure you want to change the base?
Conversation
39034ac
to
ac855dd
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
ac855dd
to
70052c0
Compare
70052c0
to
7771d07
Compare
7771d07
to
f1e6dc6
Compare
I removed breaks compat because we decided to only change the new parameters to double. Edited: At least that's the approach we want to take. It may still be breaking compat, but that's not by design and is a bug. |
This comment was marked as outdated.
This comment was marked as outdated.
f1e6dc6
to
0f1f501
Compare
Added, thanks for the pioneer work and review! |
Two reasons to change this: * At a default mix rate of 44100, the playback position in seconds can experience rounding errors with a 32-bit type if the value is high enough. 44100 requires 15 free bits in the mantissa to be within 1/2 an audio frame, so the cutoff is 512 seconds before rounding issues occur (512=2^9, and 9+15 > 23 mantissa bits in 32-bit float). * Internally, AudioStreamPlayback and AudioStream use a 64-bit type for playback position. See this discussion: godotengine#105510 (comment)
@@ -1263,6 +1277,14 @@ void AudioServer::start_playback_stream(Ref<AudioStreamPlayback> p_playback, con | |||
playback_node->highshelf_gain.set(p_highshelf_gain); | |||
playback_node->attenuation_filter_cutoff_hz.set(p_attenuation_cutoff_hz); | |||
|
|||
uint64_t scheduled_start_frame = uint64_t(p_scheduled_start_time * get_mix_rate()); | |||
if (scheduled_start_frame > 0 && scheduled_start_frame < mix_frames) { |
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.
How does this treat the web browser situation where the frame rate goes near zero and all the deadlines are missed when tabbed away?
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.
For the threads version, the audio process
is run on a separate thread that doesn't get throttled when tabbed out, so the absolute time continues incrementing at regular speed. The game logic that does the scheduling stops running though, so the metronome stops and eventually the music as well (song loop is manually scheduled). Returning back to the tab causes the metronome to be off due to the song scheduled before the current time as shown in the console error logs, but later scheduling brings everything back to normal.
I uploaded a nothreads version as well. Surprisingly, the onmessage
callback on the audio worklet that triggers the audio process
on the C++ side doesn't get throttled, so things work similarly as the threads version.
One key difference is that with lower framerate on nothreads, the audio gets crackly and the absolute time lags behind since there are less mixes happening than there should be. With "Use play_scheduled()" toggled off, the metronome (OS time based) is now unsynced from the song due to the missed mix deadlines.
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.
Does it spam overlap at the restart?
Edited: Should it?
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.
With the current implementation the song does spam restart. Simplified code:
func _process(delta: float) -> void:
if curr_time > last_scheduled_time:
last_scheduled_time += song_length
play_scheduled(last_scheduled_time)
You can definitely prevent this behavior with something like:
func _process(delta: float) -> void:
if curr_time > last_scheduled_time:
var next_song_loop = ceil((curr_time - initial_start_time) / song_length)
last_scheduled_time = initial_start_time + song_length * next_song_loop
play_scheduled(last_scheduled_time)
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.
Would like audio team input if the deadline has past if we should spam or ignore the events.
This sort of thing happens on mobile device sleep and web browser sleep.
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.
Yes, when a Godot app goes into the background or is hidden by another window I've noticed all manner of unpredictable timing issues and changes in frame rate. (These play havoc with my voip code.) This play_scheduled()
feature is low-level enough that almost all of these problems can be fixed by adding GDScript code to handle the cases. I don't think it's possible to anticipate all the problems until it is put into use in various demanding projects. Can it marked it as @experimental as I have noticed on some features, such as AnimationNodeExtension?
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.
Marking as experimental sounds good to me.
I'd imagine most of the issues caused by timing issues would be due to some mishandling in the GDScript side, like how my demo implementation is flawed. Some prevention mechanisms here would be good to add in the future as issues come up.
Pardon the Intrusion with a related but tangential to a degree matter to the current discussions This current method of syncing visuals with gameplay Is finicky and poor in accuracy. It is my understanding that the monotonic nature of DSP time should allow proper non jittery advancement of notes in rhythm games as well as any music synced lerped movements right? I bring it up because the linked fire GitHub gist post or whatever it's called (https://gist.github.com/fire/b9ed7853e7be24ab1d5355ef01f46bf1 )had syncing visuals as a goal when implementing this. But it doesn't seem present in the demo so I wondered if it wasnt aming the functionality being introduced in this PR |
Maybe we can create demos for https://github.com/godotengine/godot-demo-projects I talked about: Use cases:
|
0f1f501
to
012215c
Compare
Oh gotcha, yeah this is a much easier way to handle it. I've made the change.
I think it's better to keep the existing behavior for a couple of reasons:
I do think this can be useful, and we can add a param in the future to enable this behavior if this becomes a common use case. |
I honestly don't see this PR helping too much with the 2nd case since The 3rd case would certainly be more exact with |
if your second point holds true thats a shame, im guessing most consumers of DSP time that really need it are rhythm games ( only other large scale usage i can think of is a midi player or other audio software) , most of which have a scrolling playfield ( taiko , SDVX, IIDX, etc etc ) , any amount of jitter is basically an experience killer, and the jitter really is quite bad, even in ideal scenarios where youre running both the audio driver and the logic at unreasonably taxing rates the jitter is bad, as it stands godot is not really viable for any sort of competitive rhythm game in my experience, there is one major exception, that being project heartbeat , but that uses a fork that tears out and replaces half the audio internals so it might aswell be an unreal engine game as far as that goes. I hope your smoothing out solution pans out though and dont take this as a criticism of this PR which was very much neccesary, just that it seems like it needs accompanying adaptation to really reach its true potential usage wise |
The code for dejittering can be done with the 1 euro filter. https://github.com/patrykkalinowski/godot-xr-kit/blob/master/addons/xr-kit/smooth-input-filter/scripts/one_euro_filter.gd |
012215c
to
ed81d23
Compare
The small godot audio meeting today (5 attendees) thought this one looked good. |
@mk56-spn As promised, here's the rhythm game demo project with the playback position smoothing using the 1€ filter: godotengine/godot-demo-projects#1197. |
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.
Part of what originally confused up on the purpose of this PR is that there's no documented reason for this method to exist.
What I mean is, at a glance, most users will look at both play
and play_scheduled
. They will see that the latter has a longer name, longer description, and takes longer to set up. As such, they may stick to _process()
callbacks, Timer nodes, etc. for accurate audio purposes. There's barely any mention, or any examples as to why this should be used over play()
.
I have no suggestions about that, but I would heavily recommend to at least provide one.
ed81d23
to
67c01ab
Compare
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 generally questioning an implementation detail. I have not looked much at the prior conversation, so I'm not sure if this has ever been brought up.
If AudioServer.get_absolute_time
is so integral to AudioStreamPlayer.play_scheduled
, why should it be fetched every time in user code? Why should play_scheduled
require an absolute time, instead of relative time?
Essentially my suggestion is as follows:
# Before:
var future_time = AudioServer.get_absolute_time() + 1
player1.play_scheduled(future_time)
# After:
player1.play_scheduled(1)
Regarding the precision of these methods, I wonder if #81873 would be of any inspiration, or if it would even supersede AudioServer.get_absolute_time
in some situations?
The absolute time is calculated based on the total mixed frames and the mix rate. This means it only updates when a mix step happens. Specific play_scheduled behaviors: - If a sound is playing, play_scheduled() will stop that sound. This matches the behavior of play(). - If a sound is scheduled, then paused, then resumed before the schedule happens, the sound still plays at the correct scheduled time. - If a playing sound is paused, then play_scheduled() is called, the sound will restart from the beginning. This matches the behavior of play(). - With a higher max_polyphony, multiple sounds can be scheduled. - play_scheduled is unaffected by pitch scale - play_scheduled does not support samples Scheduled stop is not implemented due to limited use cases. Co-authored-by: K. S. Ernest (iFire) Lee <[email protected]>
67c01ab
to
4af7bec
Compare
Here's a trimmed down version of the metronome in my demo project: var _start_absolute_time: float
var _scheduled_time: float
func _process(_delta: float) -> void:
# Once the currently scheduled tick has started, begin scheduling the next
# one.
var curr_time := AudioServer.get_absolute_time()
if curr_time > _scheduled_time:
var next_tick := ceili((curr_time - _start_absolute_time) / beat_length)
_scheduled_time = _start_absolute_time + next_tick * beat_length
play_scheduled(_scheduled_time)
func start(start_absolute_time: float) -> void:
_start_absolute_time = start_absolute_time
_scheduled_time = start_absolute_time
play_scheduled(_scheduled_time) If we were to change the API to be relative, reimplementing it would look something like: var _delay: float
func _process(delta: float) -> void:
_delay -= delta
if _delay > 0:
return
# Now that the currently scheduled tick has started, begin scheduling the
# next one.
var curr_time := song_player.get_playback_position()
var next_tick := ceili(curr_time / beat_length)
var relative_delay := curr_time - next_tick * beat_length
play_scheduled(curr_time)
_delay = relative_delay
func start(delay: float) -> void:
_delay = delay
play_scheduled(_delay) For scheduling sounds at the very start, using relative seems a lot easier since you only need to care about the initial delay. However once the game logic advances past that first frame, you need some sort of "current time" in both cases in order to know when to schedule sounds (in the above example, the next metronome tick). Using
To me, the monotonic and audio thread synced nature of
IMO this follows best engine practices with this low-level API exposed to users that need it so they can build their own solutions. |
(I'm so sorry for the review delay. 🙇) I will summarize here the thread I made on the Godot Developers Chat. First and foremost, thank you @PizzaLovers007 for your great PR. You did put a lot of work on it, and it shows! So. The need for a new PRI think a new PR is needed, as a little twist is needed on how to implement the functionality. I think nobody here is doubting of the necessity to add such feature to Godot. And @PizzaLovers007 you should totally let this one intact (just to keep the history of the code and such), and start a new one if you're still interested! The problem with the current implementation is that it relies heavily on the We need to ponder about the fact that
I think we should inspire ourselves from the Web Audio API An And the API is so small: there's nothing really you can do once it started. All you can do is to disconnect the node from it's audio destination. An alternative way to handle scheduled playWhat about We can keep using // When `p_start == -1.0`, it would be the equivalent of "now"
// I initially thought about `AudioStreamPlayer.schedule_play()`, but I think the new name states better what it does... and especially what it returns.
Ref<AudioStreamPlaybackScheduled> AudioStreamPlayer::start_scheduled_stream_playback(double p_start = -1.0, double p_stream_offset = 0.0);
// No default here because it's an optional call. Thanks @PizzaLovers007 for the suggestion.
void AudioStreamPlaybackScheduled::set_scheduled_stop(double p_end); This has the added bonus to give the control to the user. The user can summon any number of It can be useful to summon multiple streams in advance too. Currently, in this PR, the metronome plays on-time when the process is at 1FPS, but fails a lot of time to actually play, because the main code didn't have the opportunity to queue up new ticks. As a metronome is predictable, you could easily whip up something like this: var player_in_range: = true
var playback_instances: Array[AudioStreamPlaybackScheduled] = []
const MAX_PLAYBACK_INSTANCES: = 10
func _on_playback_end(playback: AudioStreamPlaybackScheduled) -> void:
playback_instances.erase(playback)
func _process() -> void:
if player_in_range:
while playback_instances.size() < MAX_PLAYBACK_INSTANCES:
var playback: = $player.start_scheduled_stream_playback(AudioServer.get_current_time() + playback_instances.size())
playback.connect("playback_ended", _on_playback_end.bind(playback), CONNECT_ONE_SHOT)
playback_instances.push_back(playback)
else:
for playback_instance in playback_instances:
AudioServer.stop_playback_stream(playback_instance)
|
This is a rewrite of godotengine#105510 that pulls the silence frames logic into a separate AudioStreamPlaybackScheduled class. The rewrite allows both the AudioServer and the player (mostly) to treat it as if it were a generic playback. Main differences: - play_scheduled returns an AudioStreamPlaybackScheduled instance, which is tied to the player that created it. - The start time can be changed after scheduling. - You can now set an end time for the playback. - The scheduled playback can be cancelled separately from other playbacks on the player. Co-authored-by: K. S. Ernest (iFire) Lee <[email protected]>
This is a rewrite of godotengine#105510 that moves the silence frames logic into a separate AudioStreamPlaybackScheduled class. The rewrite allows both the AudioServer and the player (mostly) to treat it as if it were a generic playback. It also simplifies the addition of new features. Main differences: - play_scheduled returns an AudioStreamPlaybackScheduled instance, which is tied to the player that created it. - The start time can be changed after scheduling. - You can now set an end time for the playback. - The scheduled playback can be cancelled separately from other playbacks on the player. Co-authored-by: K. S. Ernest (iFire) Lee <[email protected]>
Thanks for the review! I've given this some more thought after trying to implement it (WIP) as well as seeing some of the post-rework discussions in the chat. I can agree that the scheduling logic should be moved to its own playback. However, I think my main worry about dissociating the playback from the player is that there are some settings like volume and panning that as soon as you schedule the sound, you'd need to interact with Additionally from your example:
If we want to expand on the playback idea but keep it tied to the player, I came up with few alternatives: Alternative 1: Add the playback to the playervoid AudioStreamPlayer::add_playback(AudioStreamPlayback playback); Benefits of this would be that the user is free to create any kind of playback from any stream but still reap the benefits of the player frontends. The drawback here would be that the playback itself may have nothing to do with the attached Alternative 2:
|
This is a rewrite of godotengine#105510 that moves the silence frames logic into a separate AudioStreamPlaybackScheduled class. The rewrite allows both the AudioServer and the player (mostly) to treat it as if it were a generic playback. It also simplifies the addition of new features. Main differences: - play_scheduled returns an AudioStreamPlaybackScheduled instance, which is tied to the player that created it. - The start time can be changed after scheduling. - You can now set an end time for the playback. - The scheduled playback can be cancelled separately from other playbacks on the player. Co-authored-by: K. S. Ernest (iFire) Lee <[email protected]>
This is a rewrite of godotengine#105510 that moves the silence frames logic into a separate AudioStreamPlaybackScheduled class. The rewrite allows both the AudioServer and the player (mostly) to treat it as if it were a generic playback. It also simplifies the addition of new features. Main differences: - play_scheduled returns an AudioStreamPlaybackScheduled instance, which is tied to the player that created it. - The start time can be changed after scheduling. - You can now set an end time for the playback. - The scheduled playback can be cancelled separately from other playbacks on the player. Co-authored-by: K. S. Ernest (iFire) Lee <[email protected]>
@PizzaLovers007 We discussed your proposal during the last audio meeting and I had an idea of a counter proposal. I just didn't have the time yet to do so. I'll try to do it today or in the next days. |
Implementation is based off of @fire's notes 4 years ago: https://gist.github.com/fire/b9ed7853e7be24ab1d5355ef01f46bf1
The absolute time is calculated based on the total mixed frames and the mix rate. This means it only updates when a mix step happens.
Specific
play_scheduled
behaviors:play_scheduled()
will stop that sound (with single polyphony). This matches the behavior of play().max_polyphony
, multiple sounds can be scheduled, and playing sounds can continue playing.play_scheduled
is unaffected by pitch scale.play_scheduled
does not support samples. The "Stream" default playback type is required for Web builds (ideally with threads enabled).Scheduled stop is not implemented due to limited use cases.
Fixes godotengine/godot-proposals#1151.