node-taglib-sharp incorrectly parses genres if it starts with numbers

[digimezzo]

The attached file contains a genre 20er Jahre. The user that provided this file, likes to organize genre by decade.
During parsing of the TCON field in textInformationFrame.ts, there is this piece of code:

if (Id3v2Settings.useNonStandardV2V3NumericGenres) {
    const text = Genres.indexToAudio(v2, false);
    if (text) {
        v2 = text;
    }
} 

v2 contains 20er Jahre, which is passed to Genres.indexToAudio. This is the code for indexToAudio:

public static indexToAudio(index: number|string, allowParenthesis: boolean): string {
        const safeIndex = typeof(index) === "string"
            ? Genres.stringToByte(index, allowParenthesis)
            : index;
        return Number.isSafeInteger(safeIndex) && safeIndex < Genres.AUDIO_GENRES.length && safeIndex >= 0
            ? Genres.AUDIO_GENRES[safeIndex]
            : undefined;
    }

index contains 20er Jahre, which is converted to safeIndex=20. Index 20 in Genres.AUDIO_GENRES= 'Alternative', which is returned as genre.

The file in question: genre tag test.mp3

[benrr101]

Hmmm! Ok, I'm on vacation and my task for this vacation is to get this issue taken care of!

After getting back up to speed with this chunk of code and the spec, I think I understand what's going on. This is an inherent flaw in the ID3v2.3 (and v2.2) spec, where they wanted to support the v1 genres as numbers but also support free text genres, refinements of genres (both numeric and freeform), as well as any combination thereof - without defining a separator for a genre. However, for ID3v2.3 they specified that any numeric genre should be wrapped in (), so (20) should be read as Alternative but 20er should be read as 20er, if we're following strict behavior.

The TagLib# code doesn't take this into consideration at all, and just splits the text by a separator (even though v2.3 doesn't specify one) and tries to convert each split to a v1 numeric genre regardless of parentheses. If it can't convert it, it treats it as text. It also doesn't support refinements of numeric genres or remix/cover strings as specified in the spec.

So in a relatively recent (like 3 years ago) update, I tried to fix this behavior and properly support the v2.3 spec, despite it being so incomplete. I added two settings:

• Id3v2Settings.useNonStandardV2V3GenreSeparators - allow using / or ; to split a TCON frame into multiple values on ID3v2.2 and ID3v2.3. When true ABC/DEF;GHI becomes ["ABC", "DEF", "GHI"]. When false ABC/DEF;GHI becomes `["ABC/DEF;GHI"]. Defaults to true.
• Id3v2Settings.useNonStandardV2V3NumericGenres - determines whether or not to follow the spec when parsing ID3v1 numeric tags. When true (20) and 20 become ["Alternative"]. When false, (20) becomes ["Alternative"] and 20 becomes ["20"]. Defaults to true.

There is at least one bug in the implementation ... Because js's parseInt() ignores any characters in a string after a valid int, it will happily turn "20er" into 20 which gets turned into "Alternative".

On your end, you could set Id3v2Settings.useNonStandardV2V3NumericGenres to false and then it should ignore 20er because it isn't wrapped in (). But even though it follows the spec, the defacto spec has drifted over time. Either way, everyone should be using ID3v2.4 since it's definitive with genres :).

On my end, I'll fix that bug with the parsing (taking your PR into consideration), and make sure that existing tests pass and that I have a full matrix of possibilities to validate.

How's that sound? :D

[digimezzo]

Sounds good. Thank you!

[benrr101]

Hi @digimezzo ! I made a bunch of progress on this issue over the past week. I've come up with a matrix of tests to validate, and I've compared them with what TagLib# does. I could use your input in the best way to proceed!

Input	TagLib# Output	nts Current Output	nts Proposed Output
""	[]	[]	✔️ []
Classical	["Classical"]	["Classical"]	✔️ ["Classical"]
(32)	["32"]	["Classical"]	✔️ ["Classical"]
(32)foo	["32", "foo"]	["Classical", "foo"]	❓ ["Classical foo"]
(32)f((oo	["32", "f((oo"]	["Classical", "f(oo"]	❓ ["Classical f(oo"]
(32)f(oo	["32", "f(oo"]	["Classical", "f(oo"]	❓ ["Classical f(oo"]
(32)(33)	["32", "33"]	["Classical", "Instrumental"]	✔️ ["Classical", "Instrumental"]
(32)foo(33)	["32", "foo(33)"]	["Classical", "foo(33)"]	❌ ["Classical foo", "Instrumental"]
(32)(33)bar	["32", "33", "bar"]	["Classical", "Instrumental", "bar"]	❓ ["Classical", "Instrumental bar"]
(32)foo(33)bar	["32", "foo(33)bar"]	["Classical", "foo(33)bar"]	❌ ["Classical foo", "Instrumental bar"]
(32)f((oo(33)	["32", "f((oo(33)"	["Classical", "f(oo(33)"]	❌ ["Classical f(oo", "Instrumental"]
(32)(33)b((ar	["32", "33", "b((ar"]	["Classical", "Instrumental", "b(ar)"]	❓ ["Classical", "Instrumental b(ar"]
(32)f((oo(33)b((ar	["32", "f((oo(33)b((ar"]	["Classical", "f(oo(33)b((ar"]	❌ ["Classical f(oo", "Instrumental b(ar"]
(32)f(oo(33)bar	["32", "f(oo(33)"]	["Classical", f(oo(33)"]	❓ ["Classical f(oo(33)"]
(32)(33)b(ar	["32", "33", "b(ar"]	["Classical", "Instrumental", "b(ar"]	❓ ["Classical", "Instrumental b(ar"]
(32)f(oo(33)b(ar	["32", "f(oo(33)b(ar"]	["Classical", "f(oo(33)b(ar)"]	❓ ["Classical f(oo(33)b(ar"]
32	["32"]	["32"]	✔️ ["32"]
32‡	N/A	["Classical"]	✔️ ["Classical"]
32foo	["32foo"]	["32foo"]	✔️ ["32foo"]
32foo‡	N/A	["Classical"] 👈	❌ ["32foo"]
32foo33bar	["32foo33bar"]	["32foo33bar"]	✔️ ["32foo33bar"]
32foo33bar‡	N/A	["Classical"] 👈	❌ ["32foo33bar"]

Things that need to be called out here:

• 👈 Denotes the cases that this issue applies to
• ✔️ Denotes cases that I think are behaving properly
• ❓ Denotes cases where I'm not sure if the current output is better or the proposed output is better, I'll address these further down
• ❌ Denotes cases that the current implementation is wrong.

Refresher on ID3v1 Numeric Genres in ID3v2

From the spec:

References to the ID3v1 genres can be made by, as first byte, enter "(" followed by a number from the genres list (appendix A) and ended with a ")" character. This is optionally followed by a refinement, e.g. "(21)" or "(4)Eurodisco". Several references can be made in the same frame, e.g. "(51)(39)". If the refinement should begin with a "(" character it should be replaced with "((", e.g. "((I can figure out any genre)" or "(55)((I think...)". The following new content types is defined in ID3v2 and is implemented in the same way as the numerig content types, e.g. "(RX)".

I take this to mean that a "standard" numeric genre in an ID3v2.(2|3) tag is wrapped in parentheses, and refinements are the text after parentheses. Any use of ( in the refinement text should be doubled to escape it. Otherwise it should be considered the start of another numeric genre.

Question 1: Should refinements be separate elements in the text array?
Currently, refinements are stored as separate elements, which is kind of useful. But considering the refinements are refinements "of" the numeric genre, I proposed appending them to the converted string of the genre (which admittedly would be a very English-centric behavior ... but then again the hardcoded list of ID3v1 genres is also very English-centric).

You'll also notice that the TagLib# behavior pretty much doesn't match existing behavior or proposed behavior. I think TagLib#'s code is severely lacking in how it handles ID3v1 genres in TCON frames.... To get these results, I basically copied the entire test suite into the current main branch of TagLib# and ran the suite.

A deep dive into TagLib#'s TCON implementation

In the TestIdenticationFrame parsing, there's this block of code:

if (FrameId == FrameType.TCON) {
	while (value.Length > 1 && value[0] == '(') {
		int closing = value.IndexOf (')');
		if (closing < 0)
			break;

		string number = value.Substring (1, closing - 1);

		field_list.Add (number);

		value = value.Substring (closing + 1).TrimStart ('/', ' ');

		string text = Genres.IndexToAudio (number);
		if (text != null && value.StartsWith (text))
			value = value.Substring (text.Length).TrimStart ('/', ' ');
	}

	if (value.Length > 0)
		field_list.AddRange (value.Split (new[] { '/', ';' }));
}

• If the body starts with a (, find the matching )
• Add whatever is in the () to the text (eg, for (32), 32 is added to the array)
• Remove the match from the body (eg, (32) -> "")
• Take the number and attempt to get the genre for it (eg, 32 -> Classical)
• If there was a genre, and the remaining body begins with that genre name, skip over name.

So, for example (32)Classical(33)foobarbaz would become ["32", "33", "foobarbaz"], but (32)foo(33)bar(34)baz, which is totally valid ID3v2 as per the spec, would turn into ["32", "foo(33)bar(34)baz"]. Also nowhere is escaping (( handled. By my estimation refinements are not correctly supported. And this is primarily due to the ambiguity of ID3v(2|3) TCON frame now providing a separator for the frame. ID3v2.4 fixed that by forcing a $00 separator between entries.

The rest of the lifting is done in the Genres property in Id3v2 tag:

get {
    string[] text = GetTextAsArray (FrameType.TCON);
    
    if (text.Length == 0)
    	return text;
    
    var list = new List<string> ();
    
    foreach (string genre in text) {
    	if (string.IsNullOrEmpty (genre))
    		continue;
    
    	// The string may just be a genre
    	// number.
    
    	string genre_from_index = TagLib.Genres.IndexToAudio (genre);
    
    	if (genre_from_index != null)
    		list.Add (genre_from_index);
    	else
    		list.Add (genre);
    }
    
    return list.ToArray ();
}

For each element in the text, it will then try to convert the number into a genre and return that. This is how if you have an input of a "nonstandard" numeric genre, it turns into the text. (eg, 32 is unprocessed by the first block, but is converted to Classical in the second).

So, I ultimately shuffled some of this code around - doing all those checks each time the Genre property was gotten was too expensive imo. And I want to support spec-adherent behavior, even if TagLib# is doing it differently. So, I added the option to disable "nonstandard" numeric genres, but defaulted them to enabled.

Writing all this out, I've got an even better understanding of how TagLib# ends up with the values that it does. But I still disagree that it's correct. If you've got any suggestions on how to better address this than my proposal, please do let me know!!

[digimezzo]

Hi @benrr101,
Thank you for your thorough analysis and work on this issue, and for the new release as well. Sorry for the extremely late reply. Life got in between and I lost track of this.
I think it’s hard to define a perfect solution for parsing genres containing numbers, but your proposed approach seems logical to me and makes sense overall.

Thanks again!