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

Fix AudioStreamPlayer get_playback_position() for web build #95197

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

yahkr
Copy link
Contributor

@yahkr yahkr commented Aug 6, 2024

fixes #95128

get_playback_position was missing a js hookup with audio samples updates

@yahkr yahkr requested review from a team as code owners August 6, 2024 13:48
@yahkr yahkr force-pushed the 95128-audio-fix branch 2 times, most recently from 0351ebc to 7312bb8 Compare August 6, 2024 13:51
@adamscott
Copy link
Member

adamscott commented Aug 6, 2024

This fix is interesting. But it takes for granted that the browser will play the sample without a hitch. Technically, I think that we should rather implement something like mentioned here: WebAudio/web-audio-api#2397 (comment)

With that solution, we get a more precise position of the playhead, not just an approximation.

@yahkr yahkr force-pushed the 95128-audio-fix branch from 7312bb8 to b79da04 Compare August 6, 2024 13:52
@adamscott adamscott self-assigned this Aug 6, 2024
@AThousandShips AThousandShips added this to the 4.3 milestone Aug 6, 2024
@yahkr yahkr force-pushed the 95128-audio-fix branch 2 times, most recently from 3a7f95e to da56a2a Compare August 6, 2024 13:57
@adamscott adamscott modified the milestones: 4.3, 4.4 Aug 6, 2024
@adamscott adamscott changed the title Fix AudioStreamPlayer get_playback_position() for web build Fix AudioStreamPlayer get_playback_position() for web build Aug 6, 2024
@yahkr yahkr force-pushed the 95128-audio-fix branch 3 times, most recently from 7f25773 to ccb3cae Compare August 6, 2024 14:40
@yahkr
Copy link
Contributor Author

yahkr commented Aug 6, 2024

This fix is interesting. But it takes for granted that the browser will play the sample without a hitch. Technically, I think that we should rather implement something like mentioned here: WebAudio/web-audio-api#2397 (comment)

With that solution, we get a more precise position of the playhead, not just an approximation.

Definitely looks like a tricky implementation with how godot-processor works, wondering if this approximation is a good fix for the moment

will keep digging into this

@yahkr yahkr force-pushed the 95128-audio-fix branch 3 times, most recently from eea927c to 0d42a13 Compare August 7, 2024 15:40
@yahkr
Copy link
Contributor Author

yahkr commented Aug 7, 2024

@adamscott Gave it a shot... basically constructing a new AudioWorklet for each SampleNode to keep track of its playback by parsing the incoming buffer length.

process(inputs, _outputs, _parameters) {
		if (inputs.length > 0) {
			const input = inputs[0];
			if (input.length > 0) {
				this.position += input[0].length;
				this.port.postMessage({ 'cmd': 'position', 'data': this.position });
				return true;
			}
		}
		return true;
	}

image

@yahkr yahkr force-pushed the 95128-audio-fix branch 2 times, most recently from e209e46 to 4e1e8fd Compare August 8, 2024 13:29
Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Didn't test it yet, I'm doing this right now.
If it works well, I'll try to push the fix for 4.3.

There's only a few renames and nitpicks, but globally, it's a great implementation. Thanks!

@yahkr yahkr force-pushed the 95128-audio-fix branch from fe9b2d4 to 224e2e4 Compare August 8, 2024 18:16
@yahkr
Copy link
Contributor Author

yahkr commented Aug 8, 2024

oh im dumb, i forgot why i moved to manual, audio cant autoplay on web until you click into the page. so thats why i didnt hear yours half the time. but we would still need the this.start() in the setup as it used to be called upon SampleNode creation when you call .play()

Give this latest push a try, autoplay / manual appear working.

edit: it appears i curse myself anytime i say its working. manual playing isnt in latest....
edit2: nvm, I just tried to play an audio past the end of the mp3. all is well :)

updated_mrp.zip

image

@yahkr yahkr force-pushed the 95128-audio-fix branch 5 times, most recently from 0117ccf to aa69645 Compare August 8, 2024 18:49
@yahkr yahkr force-pushed the 95128-audio-fix branch from aa69645 to b079b16 Compare August 8, 2024 19:43
Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last changes that I can see.

@yahkr yahkr force-pushed the 95128-audio-fix branch 3 times, most recently from 8e2fb6b to e4f73d6 Compare August 8, 2024 19:57
@yahkr yahkr force-pushed the 95128-audio-fix branch from e4f73d6 to bcd776e Compare August 8, 2024 19:58
@yahkr
Copy link
Contributor Author

yahkr commented Aug 8, 2024

Last changes that I can see.

The errors do need separate names. But thank you for the reviews! Made changes :)

Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me! Thank you, @yahkr, for your time and effort. Very appreciated.

@adamscott adamscott modified the milestones: 4.4, 4.3 Aug 9, 2024
@akien-mga akien-mga modified the milestones: 4.3, 4.4 Aug 12, 2024
@akien-mga akien-mga added the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Aug 12, 2024
@akien-mga akien-mga merged commit f2fb335 into godotengine:master Aug 16, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 4.3.1.

@akien-mga akien-mga removed the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Sep 16, 2024
@yahkr yahkr deleted the 95128-audio-fix branch December 4, 2024 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AudioStreamPlayer not returning position on get_playback_position() on HTML5
4 participants