Further progress on tokio migration #687
Conversation
Set MSRV to 1.41
…ault to 'librespot'
Replace version functions by constants
- Store and output samples as 32-bit floats instead of 16-bit integers. This provides 24-25 bits of transparency, allowing for 42-48 dB of headroom to do volume control and normalisation without throwing away bits or dropping dynamic range below 96 dB CD quality. - Perform volume control and normalisation in 64-bit arithmetic. - Add a dynamic limiter with configurable threshold, attack time, release or decay time, and steepness for the sigmoid transfer function. This mimics the native Spotify limiter, offering greater dynamic range than the old limiter, that just reduced overall gain to prevent clipping. - Make the configurable threshold also apply to the old limiter, which is still available. Resolves: librespot-org#608
Usage: `--format {F32|S16}`. Default is F32.
- Implemented for all backends, except for JACK audio which itself
only supports 32-bit output at this time. Setting JACK audio to S16
will panic and instruct the user to set output to F32.
- The F32 default works fine for Rodio on macOS, but not on Raspian 10
with Alsa as host. Therefore users on Linux systems are warned to set
output to S16 in case of garbled sound with Rodio. This seems an issue
with cpal incorrectly detecting the output stream format.
- While at it, DRY up lots of code in the backends and by that virtue,
also enable OggData passthrough on the subprocess backend.
- I tested Rodio, ALSA, pipe and subprocess quite a bit, and call on
others to join in and test the other backends.
While at it, add a small tweak when converting "silent" samples from float to integer. This ensures 0.0 converts to 0 and vice versa.
- DRY-ups - Remove incorrect optimization attempt in the libvorbis decoder, that skewed 0.0 samples non-linear - PortAudio and SDL backends do not support S24 output. The PortAudio bindings could, but not through this API.
Co-authored-by: Johannesd3 <[email protected]>
For Rodio, this fixes garbled sound on some but not all Alsa hosts.
Johannesd3
force-pushed
the
tokio_migration
branch
2 times, most recently
from
9f799c2
to
476ace5
Compare
I hear you, man 😆 By the way I just picked up Rust 1.51 and see that Rust 2021 introduces some new warnings (some of them by default, not clippy). I'll jump in after submitting dithering and noise shaping. |
…ntrol High-resolution volume control and normalisation
Johannesd3
force-pushed
the
tokio_migration
branch
from
4b01295
to
25acf26
Compare
|
@roderickvd You're now an expert in testing all those backends, I suppose. Could you verify that everything works as intended and I did not make any mistakes while merging? |
|
I don't know exactly what happened, but this assertion failed. Maybe the Spotify servers sent an invalid response? The previous build succeeded, and this build did not change the code. |
Librespot-connect uses hyper anyway, so no one needs to disable it only to reduce the number of dependencies. Furthermore, when using another backend, people will use --no-default-features and will forget to enable the apresolve feature again.
Some of the feature flags librespot uses are not really additive but rather mutual exclusive. A previous attempt to improve the situation had other drawbacks, so it's better to postpone a decision and restore the old behaviour.
fe37186 added the restriction that `Sink`s must be `Send`. It turned out later that this restrictions was unnecessary, and since some `Sink`s aren't `Send` yet, this restriction is lifted again. librespot-org#601 refactored the `RodioSink` in order to make it `Send`. These changes are partly reverted in favour of the initial simpler design. Furthermore, there were some compile errors in the gstreamer backend which are hereby fixed.
The first test checks whether apresolve works. A second test tries to create a Spotify sessions with fake credentials and asserts that an error is returned.
Johannesd3
force-pushed
the
tokio_migration
branch
from
0ba6f14
to
0337374
Compare
Run the tests and add build for Windows.
Johannesd3
force-pushed
the
tokio_migration
branch
from
0337374
to
26c127c
Compare
| // TODO: This is just a workaround to make utf-8 encoded usernames work. | ||
| // A better solution would be to use an uri struct and urlencode it directly | ||
| // before sending while saving the subscription under its unencoded form. | ||
| let mut uri_split = response.uri.split('/'); | ||
|
|
||
| let encoded_uri = std::iter::once(uri_split.next().unwrap().to_string()) | ||
| .chain(uri_split.map(|component| { | ||
| form_urlencoded::byte_serialize(component.as_bytes()).collect::<String>() | ||
| })) | ||
| .collect::<Vec<String>>() | ||
| .join("/"); |
There was a problem hiding this comment.
That's not very nice, but the previous solution wasn't very nice either.
All tested, all good! (with the same caveats as in #660 -- though Alsa on Rodio is working now with the latest cpal and Rodio) That's some amazingly swift work man. Minor point I just noticed: librespot/playback/src/audio_backend/mod.rs Lines 118 to 119 in 3e9aee1 If Rodio is intended to be the default, best to move it to the top of the list. If no backend is specified on the command line, it picks the first. This is not specific to your work but while you're at it... |
This bundles #668 and #683 into one PR (I'm sick of multiple parallel PRs).
devintotokio_migrationutilmodule (Formerly Eliminate most uses of util module #668).