media with id3v2.4 headers does not play

[PaulWoitaschek]

[REQUIRED] Content description

I have a bug report where a media file does not play when id3 v2.4 headers are set.

The original report is here:
PaulWoitaschek/Voice#858
Problematic media is attached there too.

The error log is:

E/ExoPlayerImplInternal: Source error.
    com.google.android.exoplayer2.upstream.Loader$UnexpectedLoaderException: Unexpected IllegalStateException: Top bit not zero: -16711852
        at com.google.android.exoplayer2.upstream.Loader$LoadTask.run(Loader.java:403)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
        at java.lang.Thread.run(Thread.java:914)
     Caused by: java.lang.IllegalStateException: Top bit not zero: -16711852
        at com.google.android.exoplayer2.util.ParsableByteArray.readUnsignedIntToInt(ParsableByteArray.java:397)
        at com.google.android.exoplayer2.metadata.id3.Id3Decoder.decodeFrame(Id3Decoder.java:284)
        at com.google.android.exoplayer2.metadata.id3.Id3Decoder.decodeChapterFrame(Id3Decoder.java:614)
        at com.google.android.exoplayer2.metadata.id3.Id3Decoder.decodeFrame(Id3Decoder.java:381)
        at com.google.android.exoplayer2.metadata.id3.Id3Decoder.decode(Id3Decoder.java:143)
        at com.google.android.exoplayer2.extractor.Id3Peeker.peekId3Data(Id3Peeker.java:75)
        at com.google.android.exoplayer2.extractor.mp3.Mp3Extractor.synchronize(Mp3Extractor.java:278)
        at com.google.android.exoplayer2.extractor.mp3.Mp3Extractor.sniff(Mp3Extractor.java:152)
        at com.google.android.exoplayer2.source.ExtractorMediaPeriod$ExtractorHolder.selectExtractor(ExtractorMediaPeriod.java:961)
        at com.google.android.exoplayer2.source.ExtractorMediaPeriod$ExtractingLoadable.load(ExtractorMediaPeriod.java:891)
        at com.google.android.exoplayer2.upstream.Loader$LoadTask.run(Loader.java:381)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167) 
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641) 
        at java.lang.Thread.run(Thread.java:914) 

[REQUIRED] Link to test content

PaulWoitaschek/Voice#858 (comment)

[REQUIRED] Version of ExoPlayer being used

2.9,4, 2.9.6

[REQUIRED] Device(s) and version(s) of Android being used

Pixel with Android O, but also happening on other devices

[marcbaechinger]

It fails when parsing the chapter ID3 frame. Like the stack trace says, the top bit is expected not to be set but it is set, which makes it fail. It's kind of easy to make the file play by not expecting an unsigned int but I think that would be a dirty hack.

It needs some further investigation to make sure it does the right thing.

[PaulWoitaschek]

So it's a bug in the application Mp3Tag?

It would be great if you could handle this bad media as people just expect my audiobook player to play stuff. Everyday I hear people saying

There is a bug in your app, VLC plays this file and you don't

¯\(ツ)/¯

[marcbaechinger]

It appears to me that there is an issue with the first subframe of the ID3 frame "CHAP". It reads all the data including the frameid of the first subframe, but then it fails while trying to read the size. The size read after the ID3 frameid of the subframe is negative (see stack trace above), instead of a positive size value. That doesn't look right.

The file Sie-reden-065b_id3v24_geht_auch_komisch.mp3 in the drive folder which according to the title surprisingly works does not include such a chapter tag.

I tried to see that chapter tag in any other audio player but I couldn't. So as quick fix I'd suggest to look into that file and see whether setting the CHAP ID3 frame was intentionally and whether it is correct. Try removing that CHAP frame and try playing again with ExoPlayer. This would allow you to play that file in a sense of a quick solution.

That said, we on our side should see whether we can handle this more gracefully in a sense that a broken ID3 tag possibly does not break playback or similar. Not sure now, but we need to look into this some more.

[marcbaechinger]

Adding spec for later reference: http://id3.org/id3v2-chapters-1.0#sec3.1

Drive folder with test files: https://drive.google.com/corp/drive/folders/1b_PSabud_k9zzncwJtCGOhBY-EXngF_E

[ojw28]

+1 to enabling playback to proceed. We could do this by using (int) readUnsignedInt() to read the frame size instead of readUnsignedIntToInt, and return null if it's negative?

[marcbaechinger]

@ojw28 Yes, that's how I made it work when investigating. I wasn't sure at that point whether this is just a hack as we may need to know the size to parse/skip bytes.

[ojw28]

Good point. It looks like other "bad data" cases further down that method skip to the end of the data by calling id3Data.setPosition(id3Data.limit());, to handle the fact that we no longer know where the next frame is.

So we should probably do the same. However, one nuance that you're probably alluding to is that when we're decoding a child frame (e.g. a frame nested inside a chapter frame), we should probably set the data limit is set to the end of the parent frame rather than the end of the entire ID3 data. That way we can still parse top level frames after the one that contains the problematic child.

[fheidenreich]

Not sure, if we're dealing with "bad data" here.

The test file which produces the reported exception is using the ID3v2.4 unsynchronization scheme (as indicated in the CHAP frame header flags). While Id3Decoder seems to support unsychronization, it fails to transform the data correctly.

In

ExoPlayer/library/core/src/main/java/com/google/android/exoplayer2/metadata/id3/Id3Decoder.java

Line 352 in 4bc79c9

frameSize = removeUnsynchronization(id3Data, frameSize);

the data with the current position pointer is passed along with the frame size of the current frame.

In

ExoPlayer/library/core/src/main/java/com/google/android/exoplayer2/metadata/id3/Id3Decoder.java

Line 714 in 4bc79c9

private static int removeUnsynchronization(ParsableByteArray data, int length) {

the position is then absolute to the beginning of the file, whereas length is the actual frame size. Thus, inplace removal of unsynchronization is not performed, resulting in a wrong offset when reading data after any unsync.

[marcbaechinger]

Thanks bringing this up! It seems to me you are right. The offset needs to be taken into account in removeUnsynchronization. For instance, when the getPosition is already advanced to a value larger than the length passed in, the for-loop is not entered at all.

I quickly tested adding the offset in the control expression of the loop and when calculating the length in arrayCopy inside the loop and the sample file plays. There are around 8 CHAP tags with the title encoded in subframes (that's where the stack trace from above is from).

Thanks again for pointing this out.

Aside: Do you happen to know in what circumstances unsynchronization is applied by an encoder? Like how can I generate test media having unsynchronizated frames. Can I tell lame to do so?

Edit: guess I need to try MP3TAG :)