WohlSoft / SDL-Mixer-X

SDL Mixer X (Or "MixerX" shortly) - An audio mixer library based on the SDL library, a fork of SDL_mixer
https://wohlsoft.github.io/SDL-Mixer-X/
Other
104 stars 26 forks source link

Replace ID3Tag with own-coded thing #31

Closed Wohlstand closed 4 years ago

Wohlstand commented 4 years ago

libID3TAG is a GPL-licensed dependency that shouldn't be. Also, it gives lots of unneeded stuff and lacks support for some other. So, on the "wip-build-polishing" here is a work on a new thing.

Example of Lyrics tag from "v22obsolete.mp3":

LYRICSBEGININD0000200ETT00034LYRIC01234567890123456789012345678000063LYRICS200

Idk where is a specification that explains it more accurate Found it at id3-org, phew...

Wohlstand commented 4 years ago

APEv1and2tag.samples.zip APEv1 and v2 tag samples, hand-made via hex editor, may be incorrect.

sezero commented 4 years ago

https://robert.muth.org/Apetag/ https://github.com/robertmuth/apetag.git

Wohlstand commented 4 years ago

Oh, thanks, useful thing! I need to verify my code better...

Wohlstand commented 4 years ago

Okay, I re-made some tags: APEv1and2tag.samples2.zip And yeah, here is a possible bug that causes MPG123 to try read a APEv1 tag as a frame, therefore it says "Bad frame, going to re-sync", so, I should fix that... (APEv1 tags still hand-made because of no wrirers available, but, it's very easy to make it: remove header, and then, replace in a footer two bytes are meaning version, and voila!)

Wohlstand commented 4 years ago

Okay, it's not a fault of tagger, it's a fault of music_mpg123.c as it's doesn't prevents library to access tag areas, therefore, MPG123 gets a fail to parse frame on a place of some tag. To fix this, I need to make the limitter that will don't allow mpg123 go more than given limit to avoid it see any tags.

Wohlstand commented 4 years ago

So, the only one minor thing is left - make skipping of "LYRICSxxxx" tags

Wohlstand commented 4 years ago

About Lyrics tags, after searching, I have found two specifications:

They are should be skipped

Also, @sezero , because of these lyrics tags, your MAD code can't find and skip them.

Also, some random code: https://github.com/jiajen/mp3edit/commit/d4c845feabd0140d00ae4467def2c96c47b8553d

sezero commented 4 years ago

About Lyrics tags, after searching, I have found two specifications:

* http://id3.org/Lyrics3

* http://id3.org/Lyrics3v2

They are should be skipped

Also, @sezero , because of these lyrics tags, your MAD code can't find and skip them.

Also, some random code: jiajen/mp3edit@d4c845f

F... Is there no end to this thing??

Wohlstand commented 4 years ago

What do you mean? If you mean more tags, I guess, there is no more should be more than this. By a specification, looks like it's very simple algorithm to skip them, will make a thing if I get my home.

Wohlstand commented 4 years ago

MP3 format is a mess and therefore lots of alien tag formats ...

sezero commented 4 years ago

What do you mean? If you mean more tags,

Yes

sezero commented 4 years ago

Also, @sezero , because of these lyrics tags, your MAD code can't find and skip them.

AFAIK, libmpg123 doesn't handle those tags? Does it have issues decoding any such mp3 files? (I don't have any such mp3 files myself.)

Wohlstand commented 4 years ago

Try to play my examples with APEv1, both will make mpg123 to print out a frame error

sezero commented 4 years ago

Try to play my examples with APEv1, both will make mpg123 to print out a frame error

I meant mp3 files with that lyrics3 thing. (And I don't know if there really any APEv1 tagged mp3s are in the wild..)

Wohlstand commented 4 years ago

Files with lyrics: some of files in my first samples pack, and probably, one of ID3V2.3 called as "obsolete" or may be another file, it's with a Lyrics3v2

Wohlstand commented 4 years ago

It also causes mpg123 to print an error

Wohlstand commented 4 years ago

I meant mp3 files with that lyrics3 thing. (I don't know if there really any APEv1 tagged mp3s are in the wild..)

I'll try to dig off my music collection, maybe some are? Btw, ApeV2 tags I have found in some songs of Mayumi Kojima.

Wohlstand commented 4 years ago

Okay, for lyrics3 tags I made a skipper and it works on my samples: Lyrics3-samples.zip

https://github.com/WohlSoft/SDL-Mixer-X/commit/178e5f3f170dbcec6f614b8648b4796406879e32#diff-be03c50dd3ee1164877fb642e43ac967R481-R483

I guess there are files around in a wild with these tags to verify the work better. Lyrics3v1 tag is very simple to create by hands: it needs two marks and any random text typed between them. Lyrics3v2 is a more complex thing that can't be made easily, but, I had some samples are used it.

sezero commented 4 years ago

You should test against files in the wild, not by hand-crafted ones. To that end, I'm unsure whether we should really keep the extended ir3v1 (TAG+) support in there.

Wohlstand commented 4 years ago

I'm unsure whether we should really keep the extended

ir3v1 (TAG+) support in there. The fact it exists a long time, we should support it at least to just skip.

You should test against files in the wild, not by hand-crafted ones.

I would dig around the world some of them, I bet there exist as these tags were in a standard. Lyrics tags are not for parsing: skipping only. For a MAD who is silly and won't correctly validate a frame, it's critically to skip all tags as possible to prevent a crash. MPG123 at the same time validates frames and prints out errors when finding certain frame is not a frame and something odd.

BTW: will fix your notices tomorrow.

Wohlstand commented 4 years ago

Updated Lytics3 samples pack: Lyrics3-samples.zip I added few "invalid" files: with broken begin tag and broken end tags to verify error handling.

Wohlstand commented 4 years ago

The change e7cb2d5 I made just now, should be fine:

Can you review current state of code?

Wohlstand commented 4 years ago

@sezero, okay, just now I have found a BUNCH of files with a Lyrics3 tag are taken from the wild: I have found on my server's depths a ton of MP3 musics of my friend, and I downloaded all of them. I did a scan of them for some tags by grep, and I have found 11 songs are has this tag. I'll email them to you.

Снимок экрана от 2019-12-06 13-51-41

Wohlstand commented 4 years ago

Oh, wow, one of them contains TWO Lyrics3v2 tag entries :thinking:

Wohlstand commented 4 years ago

Okay, I took a multi-tag (two Lyrics3 tags in the same file) from a wild file and I have updated Lyrics3 samples: Lyrics3-samples.zip

Wohlstand commented 4 years ago

A little test for some players: APEv2mixLyrics3.zip I made a mixture of APE2 and Lyrics3, and VLC, Xplayer and Rhythmbox won't show tag of mixture files, but showing a tag of a normal APE tag. It's a confirmation of a think that these tags can't be used together in a same file as they are breaking their specification: each of these tags should be used before ID3v2 or at end of the file.

sezero commented 4 years ago

Ok. Give me some time to mess with this mess.

Wohlstand commented 4 years ago

Ok, I'll don't do anything here right now I think, work/research as you need. :wink:

Wohlstand commented 4 years ago
Wohlstand commented 4 years ago

Okay, while worked on ID3v1ext, I have found a minor bug in ID3v1 itself (last character losts) which I have been fixed: Снимок экрана_2019-12-07_01-52-54

Wohlstand commented 4 years ago

Okay, all necessary tag formats are working fine, so, the issue has been completed!

sezero commented 4 years ago

Musicmatch tag (noticed while browsing the following page): https://www.codeproject.com/Articles/8295/MPEG-Audio-Frame-Header#AddTags

A documentation (and a LGPL implementation) can be found in id3lib: https://sourceforge.net/projects/id3lib/

Don't know if worth supporting

Wohlstand commented 4 years ago

Musicmatch tag (noticed while browsing the following page):

Never heard about this tag format... Looks like my nightmare dream has been predicated me another work on one another tag format. and:

F... Is there no end to this thing??

your quote about Lyrics3 you didn't knew, I would to say same about Musicmatch tag I didn't knew :rage2:

Will check out some... Also, I'll try to check my MP3 tons for a hope to find this tag in a wild

sezero commented 4 years ago

Useful archive to test your parsing code: https://github.com/JamesHeinrich/getID3-testfiles

Wohlstand commented 4 years ago

Useful archive to test your parsing code: https://github.com/JamesHeinrich/getID3-testfiles

Looks useful, thanks :fox_face:

Wohlstand commented 4 years ago

Inside of an archive I have found this text file: musicmatch.txt

Wohlstand commented 4 years ago

Yeah, btw, in that getID3 the sample doesn't contains the Musicmatch tag example, or I should check it more careful, right now I gonna to sleep, will check some tomorrow.

Wohlstand commented 4 years ago

Okay, I have found one wild example, I have sent it to you :wink: I'll work on this on my end later, right now I am tired and I gonna to sleep.

sezero commented 4 years ago

The MusicMatch tag skip support is in mainstream, looks fair for now. Having more mp3 files to test would be really good.

Closing.

sezero commented 4 years ago

Attaching the MusicMatch tag extracted from the mp3 you sent to me. We need more mp3 files with MusicMatch tags.

musicmatch.tag.zip

Wohlstand commented 4 years ago

Possibly, it's existing in some other MP3 files... I should to grep them...

Wohlstand commented 4 years ago

Okay, just now I have checked more files in my server, and it's no more Musicmatch samples I have found...