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

Default to Symphonia for MP3 decoding #409

Closed
Shnatsel opened this issue Jan 23, 2022 · 10 comments · Fixed by #453
Closed

Default to Symphonia for MP3 decoding #409

Shnatsel opened this issue Jan 23, 2022 · 10 comments · Fixed by #453

Comments

@Shnatsel
Copy link
Contributor

Shnatsel commented Jan 23, 2022

Symphonia is already a supported backend for MP3 decoding in rodio, but it is not the default. I believe Symphonia MP3 decoder has matured, and it's time to switch to using it by default.

Currently rodio defaults to minimp3 for decoding, which is written in C and causes a number of issues:

Symphonia has an MP3 decoder written in 100% safe Rust. The decoding performance is exactly on par with ffmpeg in my tests.

I have tested the correctness of Symphonia's MP3 decoder on over 50,000 real-world files created by a variety of encoders to ensure correct decoding and compatibility with the files found in the wild, which resulted in multiple fixes (see pdeljanov/Symphonia#72, pdeljanov/Symphonia#78, pdeljanov/Symphonia#99). These fixes are going to ship in the upcoming v0.5 along with support for gapless playback for MP3 files.

With safety, performance and compatibility all being in great shape for Symphonia's MP3 decoder, I believe it is time to switch to using it by default instead of minimp3.

@est31
Copy link
Member

est31 commented Jan 23, 2022

Sounds like a plan! I don't really want to completely remove support for the C implementation as long as no extremely liberally licensed alternative exists though (MPL might be too strict for some people).

@Shnatsel
Copy link
Contributor Author

At a glance, most integration issues between Rodio and Symphonia have been fixed (#379, #401). #405 still outstanding, but the current mp3 adapter doesn't expose the total duration either, so it's not a blocker.

@Shnatsel
Copy link
Contributor Author

Symphonia v0.5 has now shipped with an extensively tested MP3 decoder (I've run over 500,000 MP3s through it alone, and compared the results to mpg123, libmad and ffmpeg).

This seems like a good time to switch!

@est31
Copy link
Member

est31 commented Jan 31, 2022

I've checked the code and it seems that if you enable the symphonia-mp3 feature, it already is the default. What do you mean by making symphonia the default?

@est31
Copy link
Member

est31 commented Jan 31, 2022

Note that minimp3 is also behind a feature. Both are default off.

@Shnatsel
Copy link
Contributor Author

Ah! I was under the impression that minimp3 was enabled by default, but it wasn't. This report is invalid then; but I'll open a new issue for switching to Symphonia v0.5

@Shnatsel
Copy link
Contributor Author

Nevermind, I see you have updated to Symphonia v0.5 in git already.

@est31
Copy link
Member

est31 commented Jan 31, 2022

Actually I just checked the Cargo.toml and minimp3 is a default enabled feature, so there is something left to do, switching symphonia-mp3 with minimp3 in default.

@Shnatsel
Copy link
Contributor Author

Shnatsel commented Nov 8, 2022

Is there any way I can help move this forward? It should be a rather trivial change.

@est31
Copy link
Member

est31 commented Nov 9, 2022

A PR would be welcome.

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 a pull request may close this issue.

2 participants