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

Resolve some popping and mistiming in AudioStreamInteractive #99264

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

colinator27
Copy link
Contributor

@colinator27 colinator27 commented Nov 15, 2024

Fixes #94538. Tested using that issue's MRP, as well as with small modifications to it (e.g. converting to WAV and checking for pops/mistiming there as well). Also tested using the original interactive music project from #64488 to ensure no issues appeared there.

In all, from what I could identify, there are several issues at play here, which produce different popping and mistimings:

  • When queuing for auto-advance, the order of clips/states directly alters the timing of the sound output, due to the single-pass loop to mix all the states. This PR addresses this by adding a queue_active field to each state, which queues a state to be activated after all currently-active states have finished mixing (and potentially queuing). This is only used for auto-advance, as it is irrelevant otherwise.
  • The time used to queue for auto-advance gets the playback position from the from_state's playback position, which is from after mixing the current buffer. This erroneously offsets the queue timing by the duration of the mixing buffer. This PR solves this by tracking the playback position of the state before the current mixing started, and using that for auto-advance queuing, which is the only place that is affected. This also sidesteps issues from AudioStreamPlaybackResampled causing another time offset with the duration of its internal buffer, because it immediately begins mixing when start() is called.
  • When one clip stops to transition into the next clip, some old audio data was being erroneously repeated, which led to an additional pop sound. It seems that AudioStreamPlaybackResampled::mix was the culprit (which is used for OGGs). When it runs out of data to mix, it seems like it reuses old data that happens to be in the internal buffer, repeating it. While mix() did originally return a lower number of mixed frames to signal this, AudioStreamInteractive did not account for it, and I'm sure other locations do not as well. I modified mix() to always return p_frames, and to sample silence when it runs out of data. This seems to work well, but I would like verification that this is a good solution - I tried some others beforehand, with limited success.
  • While testing, I encountered an additional bug with filler clips, where with fading disabled, the transition from the filler clip to the next clip causes a fade-in for 1 second, due to what looks like a typo. I made a MRP for this, which tests for both this bug and the popping/mistiming that the overall PR addresses: Audio Testing Filler Bug.zip. It is intended to play a consistent tone while switching between three clips. In case the fade is somehow intended behavior, this can be undone.
  • This is more of a future-proofing thing (and feel free to undo these changes for this PR if desired), but I changed instances of float to double when calculating queue times/offsets. This is to prevent floating point inaccuracy drifting a few samples in edge cases where music can last very long (probably at least 10-20 minutes), and the cost is negligible due to how infrequently _queue() is called. It's also consistent with all the other values using double. This has been undone for now and will be reintroduced in a future PR, see Resolve some popping and mistiming in AudioStreamInteractive #99264 (comment).

@AThousandShips AThousandShips added bug topic:audio cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Nov 15, 2024
@AThousandShips AThousandShips added this to the 4.4 milestone Nov 15, 2024
@colinator27
Copy link
Contributor Author

I did some further testing, and I have discovered a new bug introduced by this PR, specifically due to the changes from float to double. In some cases, like waiting for the end of a bar, interactive music can queue a clip to start at exactly the end of that clip. Normally, this is detected and wrapped around to 0. However, it seems that there are 32-bit floats used to calculate the lengths of OGGs and at least other types of streams like MP3, which cause their calculated stream length to be slightly higher. This causes errors, at least with OGG, where it fails to play and spams errors.

Since this seems like an underlying issue with those streams, I'm going to undo the changes from float to double for this PR so as not to introduce any regressions, and properly address it in a future PR. It seems like a fix that needs to be made across each stream so that they calculate their lengths using 64-bit floats instead.

@Repiteo Repiteo marked this pull request as ready for review November 15, 2024 22:29
@Repiteo Repiteo requested a review from a team as a code owner November 15, 2024 22:29
@Repiteo Repiteo marked this pull request as draft November 15, 2024 22:30
@Repiteo
Copy link
Contributor

Repiteo commented Nov 15, 2024

Oops, tried to enable your workflow but that removed the draft status for some reason; that should be resolved now

@rburing
Copy link
Member

rburing commented Nov 17, 2024

This is currently a draft, as this is my first contribution and it could probably use some testing from others (and feedback) before moving forward.

The draft status is for when you think the PR needs more work from your side and should not be merged as is (e.g. in between the time you found a bug in your PR and fixed it). If you think it's ready and you would like others to take a look and give feedback it should be marked as ready for review.

@colinator27
Copy link
Contributor Author

This is currently a draft, as this is my first contribution and it could probably use some testing from others (and feedback) before moving forward.

The draft status is for when you think the PR needs more work from your side and should not be merged as is (e.g. in between the time you found a bug in your PR and fixed it). If you think it's ready and you would like others to take a look and give feedback it should be marked as ready for review.

Yeah, it should be good to go now. I'll mark it as ready. Thanks!

@colinator27 colinator27 marked this pull request as ready for review November 17, 2024 18:39
@colinator27
Copy link
Contributor Author

Just encountered another regression due to the change with always returning p_frames (which I thought about while looking over other issues) - the finished signals were no longer working correctly, at least with OGG. I have now restored that logic, and that behavior now works again. Silence is still added to the internal buffer for AudioStreamPlaybackResampled, as that fixes one of the audio pops.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release topic:audio
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AudioStreamInteractive: Pop and mistiming in seamless transitions
4 participants