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

Tokio migration #581

Merged
merged 21 commits into from
Jan 26, 2021
Merged

Conversation

ashthespy
Copy link
Member

I rebased the old PR (#487) and merged work from #574 into the current tree..
This should fix all the conflicts in both the above PRs, so that progress can be made..

Big thanks to @Johannesd3 for the significant progress and the porting over to tokio 1.0! :-)

@ashthespy
Copy link
Member Author

ashthespy commented Jan 25, 2021

I'm a bit strapped for time until March, and IIRC @Johannesd3 is in a similar boat?
@sashahilton00 Shall we merge this into the migration branch and let others build upon this as and when they see fit?
If it takes a few more months to finalise the port, I'll try and keep dev merged in from time to time, so we don't diverge too much..

@ashthespy
Copy link
Member Author

PS: I could build with ALSA on my Linux box, but on Windows Rodio fails

error[E0277]: `*mut c_void` cannot be sent between threads safely
  --> playback\src\audio_backend\mod.rs:71:15
   |
15 | fn mk_sink<S: Sink + Open + Send + 'static>(device: Option<String>) -> Box<dyn Sink + Send> {
   |                             ---- required by this bound in `audio_backend::mk_sink`
...
71 |     ("rodio", mk_sink::<RodioSink>),
   |               ^^^^^^^^^^^^^^^^^^^^ `*mut c_void` cannot be sent between threads safely
   |
   = help: within `RodioSink`, the trait `std::marker::Send` is not implemented for `*mut c_void`
   = note: required because it appears within the type `WasapiStream`
   = note: required because it appears within the type `platform::platform_impl::StreamInner`
   = note: required because it appears within the type `cpal::Stream`
   = note: required because it appears within the type `OutputStream`
   = note: required because it appears within the type `RodioSink`

@Johannesd3
Copy link
Contributor

Sounds good. I am not planning to do very much until March, but the last remaining big thing is librespot_connect. @marcelbuesing mentioned he wants to work on it.

It's important that someone who knows more about the audio and playback crates than me looks over it. The comments in playback warned that the PlayerInternal future contained blocking code, and I didn't find a satisfactory way to spawn a single future in a single thread in tokio 1.0, so I used tokio's block_in_place every time a comment said this line was blocking. It works, but I'm not sure if I missed something here.

It will be hard to merge all changes from dev, the branches already differ very much. Maybe it's better to let them diverge and implement some things in both branches. But this is only acceptable if the migration doesn't last to long.

Previously, it was not required that Sinks are Send, but at some point it became necessary. I don't know exactly why this happened, but maybe there's another solution.

@Johannesd3
Copy link
Contributor

Sure, PlayerInternal owns a Sink, and PlayerInternal is now spawned in the tokio runtime instead of a single thread so it must be Send.

@sashahilton00
Copy link
Member

@ashthespy i think merging into tokio_migration would be a good idea, are you happy to review and merge this given that this is an extension on the work you started?

@ashthespy
Copy link
Member Author

I'm merging this in, it's a WIP branch anyway :-)

@ashthespy ashthespy merged commit aa90278 into librespot-org:tokio_migration Jan 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants