Other weird mp3-files

[mqus]

I debugged the remaining files and they had one the following cases:

1. An album with Lyrics3v2 (see https://web.archive.org/web/20190118053655/id3.org/Lyrics3 and https://web.archive.org/web/20190118053653/http://id3.org/Lyrics3v2 )
2. One file with other weird appended stuff (probably proprietary, I didn't find anything)
3. A file which was actually corrupt and only sounded fine because it sounded like it was a part of the song and the players glossed over it
4. Files where the size of the last mp3-frame was estimated larger than it actually was and a following APETAGEX/TAG frame, resulting in the parser jumping right into it. This had 2 variants: 4.1. (1 Audiobook): the estimated frame-size was always the same, but the actual framesize varied. This also affected only about 1/3rd of the mp3-files of the audiobook. 4.2. (1 Album): The estimated Frame-Size differed, but it always jumped to the same position in the following TAG (id3v1) Frame, specifically the year field.
5. A file with 193 unidentifiable non-zero bytes between the ID3v2 tag and the start of the first following 0xFF
6. A file which threw a InvalidSamplingRate { sampling_rate: 3 } (but plays just fine in players)

I can send you some files, if you want to check it out yourself.

Thoughts on possible solutions:

• Only (1) and possibly (2) is somehow conforming to existing standards. The rest is a question of strictness.
• (1) would propably be a PITA to parse with the current design and it's probably not that common anymore (in my case this was an album from 2002, converted to mp3 sometime before 2009)
• I don't know how to solve (6), but all other cases would be served with a (optional) 'lenient mode' where the parser will just look for valid frames and ignore everything it doesn't know. For this I would do something like rotate the header_buffer one byte to the left at the end of the loop and then try to fill that one byte at the start before trying again. The probability of falsely matching an mp3 frame would be low because we already skip most of the troublemakers or encounter them at the end (id3v2, ape,etc). This would also make the padding-eater of #8 obsolete.
• Another way would be to put the already parsed Duration into the Error and leave the choice to the library user. This would pretty much solve the cases 1,2 and 4 (which covers most of my files). The disadvantage with this is that failure::Error can't be used for that because everything within it is inaccessible. Personally I'm just fine with a custom error type and a local bail! macro and it probably wouldn't change or increase the existing code much.

[agersant]

Nice sleuthing!

I mostly agree with your analysis.

I don't like the option of scanning unknown data until encountering a valid frame header. While it would be rare that a valid frame header appears in arbitrary data, it would be a very undesirable failure case.

I would be happy to return the current duration after encountering unexpected data. That is already what we do for unexpected end of file (even in the middle of an audio frame), and most bizarre frames reside after the audio data. Since this will be a breaking change anyway, I think the duration after an unexpected EoF should also go in the error.

I would be interested in files from 1/2/4, although I'll have to craft public domain ones for unit testing.