search

libsdl-org / SDL_mixer

An audio mixer that supports various file formats for Simple Directmedia Layer.
zlib License
433 stars 146 forks source link

Mix_MusicDuration return value incorrect with dr_mp3 backend? #439

Closed a-hurst closed 10 months ago

a-hurst commented 2 years ago

Hi all,

Just happened on a potential bug in SDL_mixer with the new dr_mp3 backend, thanks to the efforts of @smcv: https://github.com/py-sdl/py-sdl2/issues/243

Essentially, with the default dr_mp3 backend, Mix_MusicDuration seems to be over-reporting its duration: on my system with the official macOS mixer 2.6.1 binaries, I get 0.209 seconds for the test MP3 (available here: https://github.com/py-sdl/py-sdl2/blob/master/sdl2/test/resources/soundtest.mp3), whereas with the libmpg123 backend it reports 0.1502 seconds for the same file (which @smcv helpfully confirmed was the correct duration in the linked issue above). Since the unit test I wrote based on that 0.209 number has been passing on Windows, macOS, and Linux CI runners (all with the dr_mp3 backend), I'm fairly certain it's not a platform specific issue.

Any ideas what's going on?

Wohlstand commented 2 years ago

There is possible incorrect meta-data inside of that MP3 file, or the length computing according to a number of frames, even ignoring the case that the last frame may be lesser :thinking:

Wohlstand commented 10 months ago

Off topic: Huh? @slouken, You decited to revert back to Mini MP3? Anyway, I'm curious, who originally decited to take the dr_mp3 instead of the initial Mini MP3 and for which purposes?

sezero commented 10 months ago

Off topic: Huh? @slouken, You decited to revert back to Mini MP3? Anyway, I'm curious, who originally decited to take the dr_mp3 instead of the initial Mini MP3 and for which purposes?

I had suggested it, @slouken had accepted it: https://github.com/libsdl-org/SDL_mixer/issues/244#issuecomment-1133416686

I don't like the revert to minimp3 though, possibly a mistake: Have any of you guys even reported the issue at hand to dr_libs?

slouken commented 10 months ago

Have any of you guys even reported the issue at hand to dr_libs?

I haven't, but I'm working on an SDL_mixer release and I'd rather use tested code that fixes the length issue than wait for an upstream dr_libs fix. I'll go ahead and report it and if there's a compelling reason to switch back once that's fixed, we can easily revert this revert.

Wohlstand commented 10 months ago

By the way, dr_mp3 is actually based on MiniMP3, and the MiniMP3 is a true mainstream, and the dr_mp3 is only a modded version that, seems, gets recent minimp3's updates slower. Anyway, seems the MiniMP3 didn't had any updats for 3 years :eyes:

sezero commented 10 months ago

By the way, dr_mp3 is actually based on MiniMP3, and the MiniMP3 is a true mainstream, and the dr_mp3 is only a modded version that, seems, gets recent minimp3's updates slower. Anyway, seems the MiniMP3 didn't had any updats for 3 years 👀

The duration / LAME tags issue report is at https://github.com/mackron/dr_libs/issues/263

Hopefully it gets resolved soon and we can get back