-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Switch out minimp3 with the dr_mp3 implementation #2995
base: 2.6.x
Are you sure you want to change the base?
Conversation
7402faa
to
37b92c7
Compare
I'm open to this change. It's certainly a liability to use abandoned dependencies. I believe in the past we may have hit sanitizer failures in minimp3 but I could be misremembering. |
8457a0f
to
bbef094
Compare
Tested the implementation with various MP3 of different bitrates, mono, stereo, ID3 tags, APEv2 tags, large files, small files. So far everything played back correctly. As such for me the PR is ready for review. |
Are we sure this isn't too big to put into v2? I got |
In the test you've just hard coded the returned samples. There's of course no guarantee that two different decoder implementation return the exact same four samples values at the beginning of a file. Not sure what the "correct" values should be. Looking at the files in Audacity fully zoomed in, it seems like the first few samples would equal to zero, yet on FLAC it's the same and it too does report values to start with: Either way, I don't think this is too big. We've added audio tests in master only some months ago, so it's not like SFML didn't work before that. And of course the tests need to be adapted. minimp3 is pretty broken in a few ways and there's no fix in sight. |
The author of dr_mp3 mentioned to me that there is one known issue with LAME tags: mackron/dr_libs#263 Not if this warrants a mention somewhere or if we just hope for a fix before the 2.6.2 release 😄 |
bbef094
to
b5f9098
Compare
Just in case nobody noticed yet, the author of dr_libs is also the author of miniaudio and all the decoders (mp3, FLAC, wav) are also integrated into miniaudio. This PR wouldn't make sense targeting master since we would just use miniaudio for decoding instead. |
Cherry picking the commit onto master with the tests, we seem to run into the linked issue. The test MP3 has been encoded with LAME and the tags aren't parsed, so the timing and sample count are wrongly reported.
|
Description
Since @lieff seems to have stopped working on minimp3, it might be wise to switch to dr_mp3, which simplifies our implementation and might potentially have fixes for certain issues (tbd).
This also fixes #2215
Tasks
How to test this PR?