-
Notifications
You must be signed in to change notification settings - Fork 16
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
Interim support for MP3 #24
Comments
See also RustAudio/rodio#256 |
I missed those. Doesn't look like either claims to be complete for mp3 yet. Sonata is testing mp3 and puremp3 is version 0.1.0. Sonata looks like a self contained replacement for Audrey, too bad it isn't finished yet. If either of those are more finished than they advertise I could look at how hard it would be to add them to Audrey. Right now minimp3 wouldn't be that hard for me to do. |
@Cocalus could you study the puremp3 crate in a production setting and assemble a list of stuff it has to implement/fix until we can consider switching to it? |
I'm not that familiar with the MP3 standard but I'd start with a more complete test vectors and fuzzing. https://github.com/lieff/minimp3 seems to have a pretty good set of vectors and a fuzz harness. At least a lot more than the one file of puremp3. Maybe port the test harness to Rust then verify that minimp3-sys (assuming it covers the test API, if not make one that does) of minimp3-rs still works under the Rust harness. Personally I'd probably try to do the whole test driven C -> Rust conversion on the 2K lines of minimp3, with some criterion benchmarks against minimp3-sys. Then it's just small mechanical code transformations and I could avoid having grok the standard. |
The state of MP3 decoding in Symphonia is advertised as follows:
I suppose the easiest way to test it is to get a bunch of MP3s from archive.org or some such, decode them and compare the results to a reference decoder. But right now I'm busy doing that very thing to 8 HTTP clients. |
Symphonia v0.5 has shipped just recently with a much improved MP3 decoder! I have personally tested it on over 500,000 MP3 files and compared the output to mpg123, ffmpeg and libmad. All the bugs that this uncovered were fixed prior to v0.5 release. In fact, this testing has found quite a few bugs in ffmpeg! So Symphonia v0.5 should me more than good enough for use in Audrey. |
Since there hasn't been much movement on https://github.com/RustAudio/mp3. I could make a PR to add mp3 support via https://crates.io/crates/minimp3. Since that is a C wrapper and against the stated goal of Audrey, perhaps making it a non default feature would be acceptable. Once a production ready Rust mp3 decoder is made that can be swapped in and the feature enabled by default. The API should stay the same with the exception of the types wrapped by the Reader and Error Enums. Maybe calling the non default mp3 feature "mp3_unstable", with a README comment about that expected breakage would be enough. I don't think most users will need to access the inner values anyway.
I currently have a a Enum wrapper around Audrey and minimp3 abstracting the two in some code I'm using. The only slightly weird thing is that mp3's can change sample rate (it's apparently set per block) and maybe channels. So I'll need to extend minimp3's errors with another wrapper type in the middle, to error if those values change in the middle of a file.
The text was updated successfully, but these errors were encountered: