Invalid frames result in wrong length being reported

[kaliatech]

When an MP3 file contains corrupt frames, the mp3agic library will often report an invalid length/duration. This can be problematic because there is no indication from mp3agic that there were any errors while parsing, and yet the duration is wrong. Other mp3 parsers, such as VLC and FFmpeg, report the correct duration.

Possibly related open issues:

• #85 - Unknown bug resulting in truncated file
• #90 - MpegFrame: getLengthInBytes returns wrong value
• #92 - Files with invalid frames
• #125 - getLengthInseconds is giving invalid length

As hinted at in #92, I believe the cause is in this sequence:

• Mp3File.scanFile -> Mp3File.scanBlock -> MpegFrame.constructor -> MpegFrame.setFields

Using my test file, this line in the setFields method throws exception: if (frameSync != FRAME_SYNC) throw new InvalidDataException("Frame sync missing");

When that exception occurs, the mp3agic library swallows the error and stops parsing for more frames. Looking at my test file in a hex editor, I can see that the frame header is shifted one byte left to for unknown reasons. I assume it was corrupted at some point, although I wish I knew how.

I'm not sure what the mp3agic maintainer's thoughts are on if/how best to handle this. The three options that I see:

1. Change nothing - I think this has severe consequences for the reliability of mp3agic though.
2. Report corrupt frames as a parsing error - I think this would be fine, but depends on maintainer's intent. It would mean mp3agic can not be used with partially corrupted mp3 files.
3. When encountering a corrupt frame, try to skip it and find the next good frame - I believe this is what many other libraries do.

I started implementing option 3 and have a working solution. The main change is to Mp3File.java. Instead of immediately failing on header verification error, we instead skip a byte and try again:

MpegFrame frame;
try {
	frame = new MpegFrame(bytes[offset], bytes[offset + 1], bytes[offset + 2], bytes[offset + 3]);
	sanityCheckFrame(frame, absoluteOffset + offset);
}
catch(InvalidDataException ide) {
	if (skipBadFrames) {
		offset++;
		continue;
	}
	throw ide;
}

This branch in my fork shows the changes, along with tests and a corrupted test file:

• https://github.com/kaliatech/mp3agic/commits/allow-parsing-with-invalid-frames

However, there are two issues that made me decide to not submit as a PR for review and submit this as an issue first:

• It's not obvious how best to maintain compatibility with the existing library functionality. I added a new "skipCorruptedFrames" parameter, but it's not very elegant and might not fit with maintainer's expectations.
• There are some existing unit tests using "incompletempegframe.mp3". It's not clear to me why these tests don't fail and why the duration for those appears to be correct even if incomplete. It's hard to parse the intent just looking at the test file in a binary editor though. I'm hoping maintainer(s) can explain this more easily.

[LeiYangGH]

I'm wondering why such a popular library is giving wrong duration. Could any one check if my files contain Invalid frames? http://www.filedropper.com/8k http://www.filedropper.com/160k

[1fexd]

Hello, is this issue still being worked on? Because I have the same problem: this mp3 file is 59 minutes and 25 seconds long, but your library provides only a length of 9 minutes and 55 seconds.