We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 5322 - detect_music_type() doesn't always detect mp3s
Summary: detect_music_type() doesn't always detect mp3s
Status: RESOLVED FIXED
Alias: None
Product: SDL_mixer
Classification: Unclassified
Component: misc (show other bugs)
Version: unspecified
Hardware: x86_64 Linux
: P2 minor
Assignee: Ryan C. Gordon
QA Contact: Sam Lantinga
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-10-18 03:11 UTC by David Gow
Modified: 2020-10-23 20:02 UTC (History)
1 user (show)

See Also:

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description David Gow 2020-10-18 03:11:29 UTC 
The magic number detection for mp3 files in detect_music_type() checks either for an ID3 tag ('ID3' as the first three bytes), or for a frame header, checking the first two bytes as follows[1]:

magic[0] == 0xFF && (magic[1] & 0xFE) == 0xFA

This effectively checks the frame sync (first 11 bits are 1), the MPEG audio version (next two bits must be 11, for MPEG 1), and the layer (next two bits are 11, for Layer III). See [2] for a description of the header format.

However, some MP3 files use a different MPEG audio version, but otherwise play fine. In my case, I have an MPEG 2 MP3 file[3], which decodes properly with mpg123 (and hence SDL_Mixer), if it's detected, but as it isn't, fails to load (SDL_Mixer thinks it's a MOD). The first two bytes of that file are $FF $F3 (the version field being 10, for MPEG 2).

I've worked around this by adding an ID3 tag to the file[4], after which it loads and plays perfectly.

Even if SDL_Mixer couldn't play MPEG 2 / MPEG 2.5 MP3 files, it probably would make sense to detect them properly in detect_music_type(), so that at least we'd get MP3 decoding errors, rather than trying to treat it as a .mod file.

My proposed solution would therefore be to change the 0xFE mask to 0xE6, and compare it again 0xE2, making that line:

magic[0] == 0xFF && (magic[1] & 0xE6) == 0xE2

This would only check the frame sync and layer fields.

[1]: https://hg.libsdl.org/SDL_mixer/file/128995c3a5cf/src/music.c#l531
[2]: http://www.mp3-tech.org/programmer/frame_header.html
[3]: https://github.com/sulix/ld3sdl/blob/a1cabd9a6d158c7fb5253ca4a74a84c5e0426a95/sfx/shotgun.mp3
[4]: https://github.com/sulix/ld3sdl/blob/d75c81b6fce40468bc72d3db73db789f85308d91/sfx/shotgun.mp3
Comment 1 Ozkan Sezer 2020-10-23 17:25:08 UTC 
(In reply to David Gow from comment #0)
> My proposed solution would therefore be to change the 0xFE mask to 0xE6,
> and compare it again 0xE2, making that line:
> 
> magic[0] == 0xFF && (magic[1] & 0xE6) == 0xE2

Does your suggestion not reject version 1 files?  Or am I
misreading it?

If I am not misreading: We can still check magic[1] & 0xE6,
but compare to 0xFA (v1), 0xF2 (v2), and 0xE2 (v2.5) so as
to support all three versions.
Comment 2 Ozkan Sezer 2020-10-23 19:28:11 UTC 
(In reply to Ozkan Sezer from comment #1)
> > magic[0] == 0xFF && (magic[1] & 0xE6) == 0xE2
> 
> Does your suggestion not reject version 1 files?  Or am I
> misreading it?

Obviously, I was misreading. Sorry for the noise.
Comment 3 Ozkan Sezer 2020-10-23 20:02:28 UTC 
Applied your change: https://hg.libsdl.org/SDL_mixer/rev/ecb8508abf6b

Closing as fixed.