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 ogg edge cases #55846

Merged
merged 1 commit into from Jun 22, 2022
Merged

Fix ogg edge cases #55846

merged 1 commit into from Jun 22, 2022

Conversation

ellenhp
Copy link
Contributor

@ellenhp ellenhp commented Dec 12, 2021

This should fix the edge cases described in the comments of #55820. This PR needs testing and issues tracking each edge case to be created and linked before review/merging.

In its current state, this PR is just an attempt at simplifying some of the logic in the AudioStreamPlaybackResampled class and its Ogg implementation. A few things I was doing before that were error-prone have been removed. It's not perfect but I think it's a step in the right direction.

Bugsquad edit: Fix #55849, Fix #55848

@cstaavetti
Copy link

cstaavetti commented Dec 12, 2021

From just testing the code I can tell that it does indeed correctly play all the ogg files that previously failed to stop.
However I did notice that playing some mp3 files such as the included one do cause an error message

ERROR: Condition "!active" is true. Returning: 0
at: _mix_internal (modules/minimp3/audio_stream_mp3.cpp:41)

It still sounds correct and stops.
mp3error.zip

Looks like its because "if (is_playing()) {" was dropped and _mix_internal is called when the stream is no longer playing.

@ellenhp
Copy link
Contributor Author

ellenhp commented Dec 12, 2021

Oh good catch, I should make the same change to the MP3 stream. This change brings things more in line with the way I envisioned everything working when I did the audio server rewrite. The audio server attempts to mix a certain number of samples, and the stream playback can simply refuse to do the mix, if it's unable, which will cause the audio server to fade-out the playback to silence and then stop it completely.

@cstaavetti
Copy link

Ok, I see what you mean now, audio_stream_mp3.cpp should also return 0 when !active.
I read the current changes and in my opinion this PR does improve code quality and it does fix the related issues.
Once mp3's are fixed it should be ok, unless there is also some other format that needs updating.

@ellenhp
Copy link
Contributor Author

ellenhp commented Dec 12, 2021

Thanks! I'll update MP3 streams sometime today then get an official maintainer review after that. There does seem to be one last issue that I can only reproduce by selecting an ogg stream in the editor then clicking on the play/pause button as fast as I can. It's a crashing bug related to exhausting the ogg packet stream, then pausing, then playing. I'll make a new issue for it.

@fire
Copy link
Member

fire commented Dec 15, 2021

When you're ready, post your pr for review. I'll try to gather the relevant parties.

@ellenhp ellenhp marked this pull request as ready for review April 17, 2022 21:22
@ellenhp ellenhp requested a review from a team as a code owner April 17, 2022 21:22
@ellenhp
Copy link
Contributor Author

ellenhp commented Apr 17, 2022

I think this is ready now.

@ellenhp ellenhp changed the title Draft: Fix ogg edge cases Fix ogg edge cases Apr 18, 2022
@akien-mga akien-mga merged commit d1dac84 into godotengine:master Jun 22, 2022
@akien-mga
Copy link
Member

Thanks!

@ellenhp ellenhp deleted the fix_ogg_edge_cases branch July 24, 2022 18:03
@bluenote10
Copy link
Contributor

bluenote10 commented Feb 12, 2023

@ellenhp It looks like the change here (specifically to the AudioStreamPlaybackResampled::mix function) has broken the audio stream generator, which is a bit of a showstopper currently for migrating some music project to Godot 4. I did a small investigation and have a rough understanding why it is happening, but I lack the knowledge of the audio system and the idea behind this PR. If you are still around, it would be great to hear your opinion on some of the ideas I've laid out to fix it in #65155 (comment)!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants