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

Replace ID3Tag with own-coded thing #31

Closed
10 of 11 tasks
Wohlstand opened this issue Dec 2, 2019 · 42 comments
Closed
10 of 11 tasks

Replace ID3Tag with own-coded thing #31

Wohlstand opened this issue Dec 2, 2019 · 42 comments
Assignees

Comments

@Wohlstand
Copy link
Member

Wohlstand commented Dec 2, 2019

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.

  • Use the tag skipping base made in music_mad.c by @sezero previously
  • Make parsing of ID3v1 tags (don't parse when ID3v2 or APE was successfully parsed)
  • Make parsing of ID3v2.3 (specification)
  • Make parsing of ID3v2.4 (specification of main structure, frames)
  • Make parsing of ID3v2.2 (specification)
  • Make parsing of APE tags (don't parse when ID3v2 was successfully parsed) (specification)
  • Find a way to skip the Lyrics3v2 tag that appears in some MP3 files (specification)
  • Find a way to skip the Lyrics3v1 tag that may appear in some MP3s (specification)
  • Fix the incorrect logic of ID3v1ext (TAG+): the content of TAG+ should be added to end of ID3v1, it shouldn't replace it with self
  • Prevent MPG123 to see tags, otherwise, it may spawn "Invalid frame, re-syncing" error message into stdout. Is not critical as MPG123 is smart to don't decode crap that tries to look like a frame.
  • Check out for Musicmatch tag format

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 Wohlstand self-assigned this Dec 2, 2019
@Wohlstand
Copy link
Member Author

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

@sezero
Copy link
Collaborator

sezero commented Dec 5, 2019

@Wohlstand
Copy link
Member Author

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

@Wohlstand
Copy link
Member Author

Wohlstand commented Dec 5, 2019

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
Copy link
Member Author

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
Copy link
Member Author

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

@Wohlstand
Copy link
Member Author

Wohlstand commented Dec 5, 2019

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: jiajen/mp3edit@d4c845f

@sezero
Copy link
Collaborator

sezero commented Dec 5, 2019

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
Copy link
Member Author

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
Copy link
Member Author

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

@sezero
Copy link
Collaborator

sezero commented Dec 5, 2019

What do you mean? If you mean more tags,

Yes

@sezero
Copy link
Collaborator

sezero commented Dec 5, 2019

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
Copy link
Member Author

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

@sezero
Copy link
Collaborator

sezero commented Dec 5, 2019

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
Copy link
Member Author

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
Copy link
Member Author

It also causes mpg123 to print an error

@Wohlstand
Copy link
Member Author

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
Copy link
Member Author

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

178e5f3#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
Copy link
Collaborator

sezero commented Dec 6, 2019

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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

  • no more duplicated code because I using tail_size offset as a helper to allow same code read both at end and at before ID3v1.
  • handling ONE OF tags: endian APE, Lyrics3, and ID3+ in a condition that ID3v1 is presented.

Can you review current state of code?

@Wohlstand
Copy link
Member Author

Wohlstand commented Dec 6, 2019

@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
Copy link
Member Author

Wohlstand commented Dec 6, 2019

Oh, wow, one of them contains TWO Lyrics3v2 tag entries 🤔

@Wohlstand
Copy link
Member Author

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
Copy link
Member Author

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

sezero commented Dec 6, 2019

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

@Wohlstand
Copy link
Member Author

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

@Wohlstand Wohlstand pinned this issue Dec 6, 2019
@Wohlstand
Copy link
Member Author

@Wohlstand
Copy link
Member Author

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
Copy link
Member Author

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

@Wohlstand Wohlstand unpinned this issue Dec 8, 2019
@sezero
Copy link
Collaborator

sezero commented Dec 9, 2019

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
Copy link
Member Author

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

sezero commented Dec 9, 2019

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

@Wohlstand
Copy link
Member Author

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

Looks useful, thanks 🦊

@Wohlstand Wohlstand reopened this Dec 9, 2019
@Wohlstand
Copy link
Member Author

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

@Wohlstand
Copy link
Member Author

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
Copy link
Member Author

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

@sezero
Copy link
Collaborator

sezero commented Dec 11, 2019

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

Closing.

@sezero sezero closed this as completed Dec 11, 2019
@sezero
Copy link
Collaborator

sezero commented Dec 14, 2019

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

musicmatch.tag.zip

@Wohlstand
Copy link
Member Author

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

@WohlSoft WohlSoft deleted a comment from Wohlstand Dec 15, 2019
@sezero sezero removed their assignment Dec 15, 2019
@Wohlstand
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants