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

An async version of Symphonia #27

Open
GeoffClements opened this issue May 2, 2021 · 9 comments
Open

An async version of Symphonia #27

GeoffClements opened this issue May 2, 2021 · 9 comments
Labels
enhancement New feature or request

Comments

@GeoffClements
Copy link

Hello!,

I'm currently investigating the use of Symphonia as a dependency in a pet project of my own. Trouble is, my project is based on Tokio and Futures so I'm working out if I can make Symphonia async. Is there any interest in this with the Symphonia devs? I'm considering adding an "async" feature which will not be default; I believe that the async code can be added without changing the current source, i.e. by just adding new source enabled by the async feature although I need to complete my investigation before being sure.

@pdeljanov
Copy link
Owner

Hey @GeoffClements,

I think there are two ways Symphonia could be made async:

  1. Change MediaSourceStream to be async and update all FormatReaders and MetadataReaders to be async as well. I'd consider this a "true" async solution in the sense that Symphonia could run entirely on the event loop.

  2. Implement a MediaSource that wraps async IO and provides a traditional blocking (std::io::Read + std::io::Seek) interface. This is an async "compatible" solution. Symphonia would need to run on its own worker thread.

Solution 1 has a lot of problems, but I think solution 2 makes the most sense, and I believe this is what you're proposing. In that case, I don't think there should be any problem so long as the Read and Seek traits match the documented behavior.

I'd be happy to merge this async support into Symphonia so long as it's an optional feature with as minimal dependencies as possible. Unfortunately, my async experience is limited, but I'd be happy to answer any questions re: Symphonia itself.

@GeoffClements
Copy link
Author

GeoffClements commented May 3, 2021

Hi,

thanks for the feedback.

I think there are two ways Symphonia could be made async:

TBH I was thinking of something along the lines of what lewton has done which is to create async implementations for existing structs. I notice that Symphonia makes frequent use of trait objects and I'll have to work through the implications of this regarding async.

So having an AsyncRead implementation of MediaSource is my first thought for sure. However I'd like to extend the paradigm so that any public iterators implement Stream. (The term Stream is likely to get really overloaded!)

I think that would be the minimum for an async implementation however it may also be useful to add futures for any computational intensive operations.

This big downside to this approach is code duplication between non-async and async methods, I've not yet thought of an easy way around this.

I'd be happy to merge this async support into Symphonia so long as it's an optional feature with as minimal dependencies as possible. Unfortunately, my async experience is limited, but I'd be happy to answer any questions re: Symphonia itself.

OK thanks.

I understand the drive to minimise dependencies. Although Futures have now hit the standard library they have done so in a very limited manner so a dependency on the Futures crate is almost inevitable, however it will be behind an async feature option.

Overall I think it may be better to have an async branch as this may take some time to get right (if it's possible at all!). I'm still reading through the Symphonia codebase and my knowledge is cursory at best so I need to get myself into a position where I can start experimenting with async.

*Edit to fix a typo

@vishalcjha
Copy link

hi @pdeljanov - I am trying to re on-board with Rust echo system which I worked a few years ago. Things have changed for the better and hence the interest. I am not familiar with Symphonia, but let's say we want to go with Option 1, what are small milestones we want to get to. I would imagine a huge PR is never welcomed.

@GeoffClements
Copy link
Author

Hi vishalcjha,

I did make a start at this to see what the obstacles might be. Unfortunately life got in the way and I haven't coded for some time. I did make some headway though until I hit a major problem that I was still working through and, sadly, now cannot remember exactly what it was but it was to do with the Rust compiler and looked like it was non-trivial.

I found the async-std library to be most useful as it mirrors (more or less) Rust's std library. I used maybe-async to conditionally compile the code and async-trait to drastically reduce code duplication.

If you are interested I can push my local repo onto my Github repo so you can have a look but please remember that this code is a work-in-progress and does not compile,

@vishalcjha
Copy link

Sure @GeoffClements . I was thinking to use async-std. Push what you have and I can give it a try too. What I wanted before making start is small milestones that can be trackable. But if the plan is to make all async at a go, can do that too. Not sure if that code would be easy to test and merge then.

@GeoffClements
Copy link
Author

Sorry for the delay, my repo is here.

TBH I'm not sure how realistic it will be to have a series of small working PRs as async code is a complete paradigm shift from sync code. I think the best course will be to have an async branch as I have done in my repo and do all the work there. I suspect keeping this branch long-term may be a better option as clients of Symphonia will either want async or not.

In any case I will defer to someone more experienced in these things than I am.

Best wishes.

@pdeljanov
Copy link
Owner

Hi @GeoffClements and @vishalcjha, I took a look at what's already implemented and wanted to chime in with a few of my thoughts.

The first thing that sticks out to me is that you are making Decoder async. This doesn't have to be done because Decoder consumes Packets which are just memory buffers. In other words, a Decoder never reads from an async MediaSourceStream and thus doesn't need to be async. Only FormatReader need to be async. This is good, because making a Decoder async would destroy performance.

FormatReader generally operates at the byte-level, and MediaSourceStream provides a substantial buffer, so async in this context is not terrible for performance. Though I'd be curious for a benchmark.

I think the work that needs to be done is as follows:

  1. Implement an async version of the core::io::ReadBytes (and everything that implements ReadBytes), MediaSource, and MediaSourceStream.
  2. Define a AsyncFormatReader trait.
  3. Make a copy of, and convert a simple deumxer to use AsyncFormatReader. My suggestion would be Mp3Reader or AdtsReader since they're very simple. WavReader is the next level up. FlacReader, IsoMp4Reader, and OggReader are the hardest.
  4. Implement a new version of symphonia-play that uses these async interfaces.
  5. Finally, determine how the sync and async versions of the demuxers can share the same core logic.

For now, I'd like to keep all this work on a branch as we prototype a solution. I believe async is mostly a niche use-case (WASM target?) for Symphonia all things considered so I'd want to keep the maintenance burden low.

Another interesting solution that would be less involved is an adapter that implements a MediaSource for something that implements std::io::AsyncRead instead of std::io::Read. Using this, it should be possible to spawn Symphonia on a thread, but read from an async source (the Symphonia thread would block until the async source returns the data).

@pdeljanov pdeljanov added the enhancement New feature or request label Jan 9, 2022
@abonander
Copy link

Another option would be to build APIs that can resume after io::ErrorKind::WouldBlock, then it wouldn't be necessary to have a completely separate async interface. This is, for example, how tokio-rustls works, and then rustls itself doesn't need to know anything about async I/O to work with it.

@Steve-xmh
Copy link

Any progress of this issue? It should be a need. Like fetching network audio file and instant play without download it entirely. At lease return the io::ErrorKind::WouldBlock from the MediaSource struct without keep loop in read and blocking the thread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants