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

Use symphonia::io::MediaSource for Reader extensions #61

Merged
merged 15 commits into from
Apr 21, 2021
Merged

Use symphonia::io::MediaSource for Reader extensions #61

merged 15 commits into from
Apr 21, 2021

Conversation

james7132
Copy link
Contributor

@james7132 james7132 commented Apr 10, 2021

This PR does the following:

  • Changes both Reader::Extension and Reader::ExtensionSeek to use symphonia::io::MediaSourceStream.
  • Removes the File and Vec variants of readers, instead opting to provide a from_file and from_memory associated function to create readers from the File and Cursor<Vec<u8>> implementations of MediaSource.
  • Removes the ReadSeek trait.
  • Added a dependency on symphonia_core. This crate has no additional dependencies.

This allows Songbird to be extended with streams from Symphonia, which should allows for a common interface for defining user provided streams that also allow easily hooking into Symphonia's ecosystem for in process transcoding. This would likely be more lightweight than reading from a pipe from ffmpeg and allow for transcoding streams fetched from non-local sources (i.e. YouTube, SoundCloud).

One potentially important change is that MediaSourceStream is now used to implement both in-memory and file based Readers. This adds, by default, a 32KB buffer to every reader of the aforementioned types, though it can be configured. If this is an unacceptable cost, it can be changed to Box<dyn MediaSource> instead. It should be noted that MediaSourceStream itself implements MediaSource, but defaulting to a boxing a MediaSourceStream for buffering will result in additional dynamic dispatch which may not be desired.

This PR is still a WIP. It's dependent on a fork of Symphonia to add a Send marker on MediaSource to make it compatible with Songbird's threading model.

arqunis and others added 7 commits March 23, 2021 09:17
)

* Depend on Serenity's `next` branch on Songbird's `next` branch

* Update the examples to use the `next` branch
Joining a channel returns a future which fires on receipt of two messages from discord (by locally storing a channel). However, joining this same channel again after a success returns only *one* such message, causing the command to hang until another join fires or the channel is left. This alters internal behaviour to correctly cancel an in-progress connection attempt, or return success with known data if such a connection is present.

This introduces a breaking change on `Call::update_state` to include the target `ChannelId`. The reason for this is that although the `ChannelId` of a target channel was being stored, server admins may move or kick a bot from its voice channel. This changes the true channel, and may accidentally trigger a "double join" elsewhere.

This fix was tested by using an example to have a bot join its channel twice, to do so in a channel it had been moved to, and to move from a channel it had been moved to.
This change increases the version of DiscoRTP, which fixes upstream issues in libpnet and allows additional RTCP types to be added over time without breaking semver. This is a breaking change for users explicitly matching on RTCP packet types, as DiscoRTP is exposed here.

This has been tested using `cargo make ready`.
This change fixes tasks hanging due to rare cases of messages being lost between full Discord reconnections by placing a configurable timeout on the `ConnectionInfo` responses. This is a companion fix to [serenity#1255](serenity-rs/serenity#1255). To make this doable, `Config`s are now used by all versions of `Songbird`/`Call`, and relevant functions are  added to simplify setup with configuration. These are now non-exhaustive, correcting an earlier oversight. For future extensibility, this PR moves the return type of `join`/`join_gateway` into a custom future (no longer leaking flume's `RecvFut` type).

Additionally, this fixes the Makefile's feature sets for driver/gateway-only compilation.

This is a breaking change in:
* the return types of `join`/`join_gateway`
* moving `crate::driver::Config` -> `crate::Config`,
* `Config` and `JoinError` becoming `#[non_breaking]`.

This was tested via `cargo make ready`, and by testing `examples/serenity/voice_receive` with various timeout settings.
This is a simple organisational change which moves `crate::Bitrate` to `crate::driver::Bitrate` to slightly clean up the crate root.

This has been tested using `cargo make ready`.
This PR makes many of the types under `EventContext` separate `#[non_exhaustive]` structs. This makes it more feasible to add further information to connection and packet events as required in future. On this note, driver (re)connection events now include the SSRC supplied by Discord and the domain name which was connected to.

In addition, this fixes global timed events to return a list of all live tracks, and extensively details/documents events at a high level.

This was tested using `cargo make ready`.
This silences a clippy lint around incorrect capitalisation of acronyms, but sis a breaking API change.

This was tested using `cargo make ready`.
@james7132 james7132 changed the base branch from current to next April 10, 2021 06:33
@FelixMcFelix
Copy link
Member

Thanks for the PR. I think this is a better way of allowing user extension, rather than our current (slightly ad-hoc) decoding setup. I think that I would much prefer Box<dyn MediaSource + Send> right now, as that this means that:

  • we only need to re-export one trait to users,
  • we get runtime checking of seek-compatibility,
  • users don't need to construct and configure a soft-buffered source, if they don't need/want to,
  • this shouldn't require you to manually put Send bounds in the upstream crate, hopefully.

Having to pay dynamic dispatch on Files/Cursor<Vec<u8>>s sucks, but it's likely negligible compared to encryption/encoding costs.

I haven't looked too much into symphonia, but I assume you're aiming to put full (reader + decoder) chains into a songbird Reader to get the transcoders you need working?

On another note, I think symphonia looks like a very promising family of crates if we want to change the audio stack down the line (and we probably do!). When I started work on reworking serenity voice, the only other Rust audio stack I knew of was rodio. I think it's worth keeping an eye on this, if it can prevent us from having to (re)implement mkv/ogg/etc. handlers or dynamically link to ffmpeg.

@james7132 james7132 changed the title Use symphonia::io::MediaSourceStream for Reader extensions Use symphonia::io::MediaSource for Reader extensions Apr 11, 2021
@james7132 james7132 requested a review from FelixMcFelix April 11, 2021 09:17
@james7132
Copy link
Contributor Author

On another note, I think symphonia looks like a very promising family of crates if we want to change the audio stack down the line (and we probably do!). When I started work on reworking serenity voice, the only other Rust audio stack I knew of was rodio. I think it's worth keeping an eye on this, if it can prevent us from having to (re)implement mkv/ogg/etc. handlers or dynamically link to ffmpeg.

There are also projects like rust-av, a pure Rust rewrite of lib-av, a fork of ffmpeg, though that's generally aimed at any form of transcoding, including video. Though it may be useful to extract audio from video containers (i.e MP4 files or YouTube).

Symphonia was the first one I happened on that fit the API of songbird well and supported several of the features similar libraries (i.e Lavaplayer) offer that songbird currently doesn't. Namely container and format definitions, metadata parsing, format probing and transcoding.

If there is a better option for those, songbird may be better served using that ecosystem instead.

I haven't looked too much into symphonia, but I assume you're aiming to put full (reader + decoder) chains into a songbird Reader to get the transcoders you need working?

I'm aiming to replicate the full Lavalink feature set in Oxilink, which relies heavily on many of the features provided by Lavaplayer for zero-IPC transcoding. Symphonia or dynamically linking in ffmpeg should cover the in-process transcoding features that Lavaplayer manually implemented that Songbird currently has no support for.

Copy link
Member

@FelixMcFelix FelixMcFelix left a comment

Choose a reason for hiding this comment

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

Apologies for force-rebasing next, one or two commits complicate history.

Looks good apart from some nits, thanks. Could I ask that you test the mixing benchmarks (cargo make bench) to ensure that this change doesn't break those? They rely upon Vec-based audio sources.

Formatting should go through nightly rust (cargo +nightly fmt) to get trailing commas etc working correctly.

@FelixMcFelix FelixMcFelix added breaking Will either cause existing code to fail to compile, or cause substantial behaviour changes enhancement New feature or request input Relates to raw audio data handling. labels Apr 11, 2021
Doing so collapses both Extension and ExtensionSeek into one value that
is evaluated at runtime.

Added warning to docs for is_seekable that the operation of determining
if it's seekable might involve blocking IO.
@james7132 james7132 marked this pull request as ready for review April 21, 2021 09:43
@james7132
Copy link
Contributor Author

james7132 commented Apr 21, 2021

Output of cargo bench:

Mix stereo source       time:   [621.07 ns 662.54 ns 700.92 ns]
                        change: [-5.7327% +2.1139% +12.968%] (p = 0.69 > 0.05)
                        No change in performance detected.
Found 19 outliers among 100 measurements (19.00%)
  2 (2.00%) high mild
  17 (17.00%) high severe

Mix mono source         time:   [434.42 ns 458.77 ns 483.26 ns]
                        change: [-6.8324% +0.3071% +12.644%] (p = 0.96 > 0.05)
                        No change in performance detected.
Found 17 outliers among 100 measurements (17.00%)
  1 (1.00%) high mild
  16 (16.00%) high severe

Copy link
Member

@FelixMcFelix FelixMcFelix left a comment

Choose a reason for hiding this comment

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

Thanks very much, looks good to me!

@FelixMcFelix FelixMcFelix merged commit 93d3ec5 into serenity-rs:next Apr 21, 2021
FelixMcFelix pushed a commit to FelixMcFelix/songbird that referenced this pull request May 6, 2021
This PR does the following:

 * Changes both `Reader::Extension` and `Reader::ExtensionSeek`  to use `symphonia::io::MediaSource`.
 * Removes the `File` and `Vec` variants of readers, instead opting to provide a `from_file` and `from_memory` associated function to create readers from the `File` and `Cursor<Vec<u8>>` implementations of `MediaSource`. 
 * Removes the ReadSeek trait.
 * Added a dependency on `symphonia_core`. This crate has no additional dependencies.
@james7132 james7132 deleted the extension-media-source branch May 9, 2021 07:50
FelixMcFelix pushed a commit to FelixMcFelix/songbird that referenced this pull request May 10, 2021
This PR does the following:

 * Changes both `Reader::Extension` and `Reader::ExtensionSeek`  to use `symphonia::io::MediaSource`.
 * Removes the `File` and `Vec` variants of readers, instead opting to provide a `from_file` and `from_memory` associated function to create readers from the `File` and `Cursor<Vec<u8>>` implementations of `MediaSource`. 
 * Removes the ReadSeek trait.
 * Added a dependency on `symphonia_core`. This crate has no additional dependencies.
FelixMcFelix pushed a commit to FelixMcFelix/songbird that referenced this pull request Jun 14, 2021
This PR does the following:

 * Changes both `Reader::Extension` and `Reader::ExtensionSeek`  to use `symphonia::io::MediaSource`.
 * Removes the `File` and `Vec` variants of readers, instead opting to provide a `from_file` and `from_memory` associated function to create readers from the `File` and `Cursor<Vec<u8>>` implementations of `MediaSource`. 
 * Removes the ReadSeek trait.
 * Added a dependency on `symphonia_core`. This crate has no additional dependencies.
FelixMcFelix pushed a commit to FelixMcFelix/songbird that referenced this pull request Jul 1, 2021
This PR does the following:

 * Changes both `Reader::Extension` and `Reader::ExtensionSeek`  to use `symphonia::io::MediaSource`.
 * Removes the `File` and `Vec` variants of readers, instead opting to provide a `from_file` and `from_memory` associated function to create readers from the `File` and `Cursor<Vec<u8>>` implementations of `MediaSource`. 
 * Removes the ReadSeek trait.
 * Added a dependency on `symphonia_core`. This crate has no additional dependencies.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Will either cause existing code to fail to compile, or cause substantial behaviour changes enhancement New feature or request input Relates to raw audio data handling.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants