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

Mix_MusicDuration return value incorrect with dr_mp3 backend? #439

Closed
a-hurst opened this issue Aug 22, 2022 · 6 comments
Closed

Mix_MusicDuration return value incorrect with dr_mp3 backend? #439

a-hurst opened this issue Aug 22, 2022 · 6 comments
Milestone

Comments

@a-hurst
Copy link

a-hurst commented Aug 22, 2022

Hi all,

Just happened on a potential bug in SDL_mixer with the new dr_mp3 backend, thanks to the efforts of @smcv: py-sdl/py-sdl2#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
Copy link
Contributor

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 🤔

@slouken slouken added this to the 2.8.0 milestone Jan 14, 2024
slouken added a commit that referenced this issue Jan 14, 2024
This reverts commit c94f6ce.

minimp3 supports LAME tags for more precise seeking and length information

Fixes #439
@Wohlstand
Copy link
Contributor

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
Copy link
Contributor

sezero commented Jan 14, 2024

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: #244 (comment)

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
Copy link
Collaborator

slouken commented Jan 14, 2024

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
Copy link
Contributor

Wohlstand commented Jan 14, 2024

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 👀

@sezero
Copy link
Contributor

sezero commented Jan 14, 2024

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 mackron/dr_libs#263

Hopefully it gets resolved soon and we can get back

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

No branches or pull requests

4 participants