audrey icon indicating copy to clipboard operation
audrey copied to clipboard

Interim support for MP3

Open Cocalus opened this issue 5 years ago • 6 comments

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.

Cocalus avatar Jan 03 '20 22:01 Cocalus

See also https://github.com/RustAudio/rodio/issues/256

est31 avatar Jan 04 '20 01:01 est31

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 avatar Jan 04 '20 01:01 Cocalus

@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?

est31 avatar Jan 04 '20 08:01 est31

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.

Cocalus avatar Jan 04 '20 22:01 Cocalus

The state of MP3 decoding in Symphonia is advertised as follows:

Most media streams play. Inaudible glitches may be present. Most common features are supported.

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.

Shnatsel avatar Feb 16 '21 17:02 Shnatsel

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.

Shnatsel avatar Jan 31 '22 21:01 Shnatsel