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

Force queue to play sources on frame boundaries #455

Merged
merged 13 commits into from Nov 23, 2022

Conversation

dis-da-moe
Copy link
Contributor

Purpose

Problem

As I understand it, this is the issue:

  • Previously, when a queue has keep_alive_if_empty set to true, and it becomes empty, then it will push a silence lasting 10ms onto the queue.
  • This is an issue because current_frame_len would have returned the worst case, 512, and the silence lasts less than that.
  • This means that unless the source is added immediately to the queue, and so a silence is never played, then the first actual source could start playing at a frame that is not aligned to its channels, or play at the wrong sample rate.
  • This is only determined by when the source is added to the queue after its initialization. This explains why the issue was inconsistent, as it relied on the speed of execution of code which is basically random.

Solution

  • Change the functionality of Zero to add a method to create a silence with a certain number of frames.
  • Replace the 10ms silence with a silence the length of THRESHOLD
  • Change queue's current_frame_len to return THRESHOLD if a silence will be played next.

Other Changes

At first, I could only reproduce this issue in the bevy game engine where the time a sink is created to when a source is appended can be long. The spatial example was the only example where stereo sound mattered, and so the issue was actually audible. However it had a bug where the right ear was at -1 and the left was at 1, so that combined with the inconsistency of the bug made it hard to reproduce.

To hopefully prevent stereo bugs in the future, I fixed the spatial example and made it clearer what was happening, and added a stereoexample where the file is played in the right ear and then in the left.

@harudagondi
Copy link
Contributor

stereo and spatial examples worked for me in debug and release builds.

Would it be fine to include @rparrett's sound file here?

@rparrett
Copy link

I authored the RL.wav and consent to its use here.

Also just super excited about this getting fixed up. Great work!

Copy link
Member

@est31 est31 left a comment

Choose a reason for hiding this comment

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

a .wav file of 4.61 MB is way too big of an addition. Check the other files in the directory, they are all less than 1 MB. Is that example needed for something? I don't think we should add it. Otherwise the PR is fine, good work getting to the root cause of it.

@dis-da-moe
Copy link
Contributor Author

dis-da-moe commented Nov 22, 2022

Forgot to compress. It's needed and only 200kb now.

@est31
Copy link
Member

est31 commented Nov 22, 2022

What's the license of the file? Can you maybe add it to README.md in the assets dir and explain the license?

@dis-da-moe
Copy link
Contributor Author

I'd need @rparrett to confirm but I assume it would be the same as any other contribution.

@rparrett
Copy link

I relinquish all rights to RL.wav and you may modify or license it however you wish.

@est31
Copy link
Member

est31 commented Nov 23, 2022

So it's MIT OR Apache then. Fine.

@est31 est31 merged commit 268ddda into RustAudio:master Nov 23, 2022
@DaforLynx
Copy link

Possibly fixes #369 too?

@dis-da-moe
Copy link
Contributor Author

Possibly, could you test it and see? Also @est31 do you know when this will be included in a release?

@est31
Copy link
Member

est31 commented Nov 23, 2022

@dis-da-moe I don't know, i think I want to do a cpal release first, then make a rodio release.

@DaforLynx
Copy link

DaforLynx commented Nov 25, 2022

@dis-da-moe Testing on master, the issue doesn't seem to be fixed. The beginning and end of the beep (pure sine wave) are weird.

25--13-34-39.mp4

@dis-da-moe
Copy link
Contributor Author

That seems like a separate issue, I'm not sure what causes it.

@keigen-shu
Copy link

@dis-da-moe I had encountered the issue that this PR proposes to fix before, in #286.

Have you considered what happens if the output channel count is not a factor of 512, like 11?

Prior to this PR, the silence is hard-coded as a single channel 44100Hz sample at 10ms. That is a 441 samples long buffer, which does not make a stereo frame boundary.

I had worked around the issue by simply hard-coding queue.rs, sink.rs and empty.rs to 2, since I only needed it to work with stereo output. Changing the sampling rate, or frame count, would've also worked; as long as it generates a buffer that is a multiple of the output stream channel count.

@dis-da-moe
Copy link
Contributor Author

I haven't considered it since I've never encountered a case where the channel count is not a factor of 512. I assumed that's why 512 was chosen as the length of THRESHOLD before hand. If you are able to test the issue and find a solution to it then feel free to.

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