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

Bad sound quality when upgrading to Rust 1.45 #303

Closed
Rua opened this issue Jul 19, 2020 · 14 comments
Closed

Bad sound quality when upgrading to Rust 1.45 #303

Rua opened this issue Jul 19, 2020 · 14 comments
Labels
Milestone

Comments

@Rua
Copy link

Rua commented Jul 19, 2020

I updated my toolchain to use Rust 1.45, this has broken the audio playback using Rodio 0.11.0. The sound comes out as somewhat muffled, like it was low-pass filtered. I can restore the correct audio by downgrading Rust back to 1.44.1, so clearly there is something going on with changes made in the compiler and how they interact with Rodio. Perhaps the changes in float-to-integer casts are to blame.

I've attached a sound recording for comparison. First, two sound effects are heard from Rust 1.45, then the same two effects in Rust 1.44.1. (Yes, these are old Doom sound effects, if anyone is wondering :) )

brokensound.ogg.zip

To play this audio, I use a custom Source that reads its samples from an Arc<[i16]>, so that I can keep the buffer around after playing it. This is then wrapped in the ChannelVolume, Done and SamplesConverter sources, sent to another thread (to avoid the COM initialisation bug on Windows), where it is then played via rodio::play_raw.

@est31
Copy link
Member

est31 commented Jul 19, 2020

Hi! interesting report. You could try to narrow it down by bisecting the rustc: https://github.com/rust-lang/cargo-bisect-rustc/blob/master/TUTORIAL.md

Then we'd know what to look for.

@est31 est31 added the bug label Jul 19, 2020
@Rua
Copy link
Author

Rua commented Jul 20, 2020

Here's the result I got from it:

searched nightlies: from nightly-2020-05-01 to nightly-2020-07-20
regressed nightly: nightly-2020-05-20
searched commits: from rust-lang/rust@d887886 to rust-lang/rust@3a7dfda
regressed commit: rust-lang/rust@672b272

@Rua
Copy link
Author

Rua commented Jul 20, 2020

I just checked the output of the Source that's being fed to rodio::play_raw in both versions. It gives the exact same stream of f32s. So it's definitely not a problem in my side of the code, something is going wrong after Rodio gets the Source.

@est31
Copy link
Member

est31 commented Jul 20, 2020

It's probably due to some UB caused by the tiny vecs change. Interesting...

@Rua
Copy link
Author

Rua commented Jul 20, 2020

I've confirmed the bug on my laptop as well. The project in question exhibiting the bug is https://github.com/Rua/ferret.

@nbraud
Copy link
Contributor

nbraud commented Jul 20, 2020

For what it's worth, looking at the code there's nothing in rodio that seems suspect, but cpal has a lot of unsafety in the ALSA backend ( both src/alsa/ and alsa-sys/ ).

It would be interesting to check on the other cpal backends, to confirm whether the issue is specific to ALSA,

@est31
Copy link
Member

est31 commented Jul 20, 2020

@Rua which OS are you using? Windows?

Also it would be great to have a self contained example, i.e. one that doesn't need quake wad files. Do the rodio examples work for you?

@nbraud
Copy link
Contributor

nbraud commented Jul 20, 2020

@Rua which OS are you using? Windows?

Rua had the issue on Linux (Mint, IIRC), and I reproduced it on Debian.

@Rua
Copy link
Author

Rua commented Jul 21, 2020

I cloned a copy of Rodio 0.11.0 and modified it so that in audio_callback, the samples being received from the mixer are printed before they are written to cpal's buffer. I did this for both Rust 1.44.1 ("good") and Rust 1.45 ("bad"), and here are the results:

good.txt bad.txt (48 kHz stereo)

Clearly, the two files are not the same. So it appears that the fault is somewhere between rodio::play_raw and audio_callback.

@Rua
Copy link
Author

Rua commented Jul 21, 2020

I've narrowed the problem down further to SampleRateConverter. If I remove that from the processing pipeline, the samples in the two versions become identical again.

@Rua
Copy link
Author

Rua commented Jul 21, 2020

I found the problem! In SampleRateConverter, various bits of code rely on the return value of capacity(). So in SampleRateConverter::new I printed the capacity of current_frame, next_frame and output_buffer:
Rust 1.41.1 output: 2 2 1
Rust 1.45 output: 4 4 1
So this is how that commit in Rust caused problems. Rodio was relying on the capacity being a certain value, and it changed. Presumably, the code should be modified so it uses a measure other than the capacity?

@est31
Copy link
Member

est31 commented Jul 21, 2020

Nice, a logic bug... Yeah it should be changed to have outputs independent of the capacity.

@Rua
Copy link
Author

Rua commented Aug 30, 2020

Has any work been done on this? I am thinking how I could fix it myself, but I don't know how the code is supposed to work or why it's using the capacity in the first place.

@Rua
Copy link
Author

Rua commented Aug 30, 2020

It appears that @tomaka was the one who originally wrote the code, maybe he has some insight?

@est31 est31 added this to the 0.12 milestone Oct 2, 2020
@est31 est31 closed this as completed in 9feb54d Oct 4, 2020
est31 added a commit that referenced this issue Oct 4, 2020
SampleRateConverter: Fix reliance on Vec internals (see #303)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants