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 4295 - [PATCH] music_mad.c must handle and skip tags
Summary: [PATCH] music_mad.c must handle and skip tags
Status: RESOLVED FIXED
Alias: None
Product: SDL_mixer
Classification: Unclassified
Component: misc (show other bugs)
Version: unspecified
Hardware: All All
: P2 normal
Assignee: Ozkan Sezer
QA Contact: Sam Lantinga
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-10-05 16:20 UTC by Ozkan Sezer
Modified: 2018-10-07 08:50 UTC (History)
0 users

See Also:

Attachments
patch to make libmad skip tags (4.08 KB, patch)
2018-10-05 16:20 UTC, Ozkan Sezer 		Details | Diff
updated patch for SDL-1.2 (4.56 KB, patch)
2018-10-05 18:16 UTC, Ozkan Sezer 		Details | Diff
updated patch for SDL-2.0 (4.69 KB, patch)
2018-10-05 18:17 UTC, Ozkan Sezer 		Details | Diff
patch for SDL_mixer-2.0 (3.91 KB, patch)
2018-10-06 14:11 UTC, Ozkan Sezer 		Details | Diff
patch for SDL_mixer-1.2 (3.88 KB, patch)
2018-10-06 14:12 UTC, Ozkan Sezer 		Details | Diff
Show Obsolete (2) View All 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 Ozkan Sezer 2018-10-05 16:20:29 UTC 
Created attachment 3340 [details]
patch to make libmad skip tags

This file (https://github.com/WohlSoft/AudioCodecs/files/2448712/bad-mp3.zip)
plays back wrong with SDL_mixer if the library is built against libmad.  The
reason is that there is no tag handling and skipping in music_mad.c; see the
discussion led to this bugzilla entry here:
https://github.com/WohlSoft/AudioCodecs/issues/6#issuecomment-426941738

The lack of tag handling / skipping can even lead to crashes, as reported by
Vitaly Novichkov.

I cooked the attached patch, and it plays the mp3 referred to above normally.
The patch based mainly on sox, which in turn uses bits from libid3tag.  The
apetag parsing is based on a patch that was made for mpg123.

Can you see anything bad?  OK to apply?
Comment 1 Ozkan Sezer 2018-10-05 17:32:10 UTC 
The SDL-1.2 branch is worse that it certainly crashes:  valgrind
reports several invalid reads and invalid writes.  The patch fixes
the issue there too.
Comment 2 Ozkan Sezer 2018-10-05 18:16:33 UTC 
Created attachment 3343 [details]
updated patch for SDL-1.2
Comment 3 Ozkan Sezer 2018-10-05 18:17:09 UTC 
Created attachment 3344 [details]
updated patch for SDL-2.0
Comment 4 Sam Lantinga 2018-10-06 00:01:13 UTC 
We can't include GPL code unless MUSIC_MP3_MAD_GPL_DITHERING is defined, as package releases do not enable GPL code by default.

Do you want to do a clean implementation under the Zlib license, or add alternative code paths for non-GPL code?
Comment 5 Ozkan Sezer 2018-10-06 14:11:43 UTC 
Created attachment 3349 [details]
patch for SDL_mixer-2.0

The code was already half-way rephrased, but went back again, further
used id3.org docs and made the attached version. I don't think it can
be made more from scratch than this.  OK to apply?
Comment 6 Ozkan Sezer 2018-10-06 14:12:09 UTC 
Created attachment 3350 [details]
patch for SDL_mixer-1.2
Comment 7 Sam Lantinga 2018-10-07 00:58:20 UTC 
Yep, looks good, thanks!
Comment 8 Ozkan Sezer 2018-10-07 08:50:21 UTC 
SDL-1.2 branch: https://hg.libsdl.org/SDL_mixer/rev/ae9b46ccd5ab
default branch: https://hg.libsdl.org/SDL_mixer/rev/c39a11fa853e

Closing as fixed.