Improved ID3 chapter parsing code from feedback given.

5153e9e9 · Philip Simpson · f2f10580 · 5153e9e9 · 5153e9e9 · 5153e9e9
Commit 5153e9e9 authored Jan 17, 2017 by Philip Simpson
Showing with 84 additions and 76 deletions
library/src/main/java/com/google/android/exoplayer2/metadata/id3/ChapterFrame.java → library/src/main/java/com/google/android/exoplayer2/metadata/id3/ChapFrame.java
library/src/main/java/com/google/android/exoplayer2/metadata/id3/ChapterTOCFrame.java → library/src/main/java/com/google/android/exoplayer2/metadata/id3/CtocFrame.java
library/src/main/java/com/google/android/exoplayer2/metadata/id3/Id3Decoder.java
library/src/main/java/com/google/android/exoplayer2/metadata/id3/UrlLinkFrame.java → library/src/main/java/com/google/android/exoplayer2/metadata/id3/WxxxFrame.java
--- a/library/src/main/java/com/google/android/exoplayer2/metadata/id3/ChapterFrame.java
+++ b/library/src/main/java/com/google/android/exoplayer2/metadata/id3/ChapterFrame.java
 /*
- * Copyright (C) 2016 The Android Open Source Project
+ * Copyright (C) 2017 The Android Open Source Project
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
@@ -22,7 +22,7 @@ import com.google.android.exoplayer2.util.Util;
 /**
 * Chapter information "CHAP" ID3 frame.
 */
-public final class ChapterFrame extends Id3Frame {
+public final class ChapFrame extends Id3Frame {
  public static final String ID = "CHAP";
@@ -35,8 +35,8 @@ public final class ChapterFrame extends Id3Frame {
  public final String url;
  public final ApicFrame image;
-  public ChapterFrame(String chapterId, int startTime, int endTime, int startOffset, int endOffset,
+  public ChapFrame(String chapterId, int startTime, int endTime, int startOffset, int endOffset,
-    String title, String url, ApicFrame image) {
+                   String title, String url, ApicFrame image) {
    super(ID);
    this.chapterId = chapterId;
    this.startTime = startTime;
@@ -48,7 +48,7 @@ public final class ChapterFrame extends Id3Frame {
    this.image = image;
  }
-  /* package */ ChapterFrame(Parcel in) {
+  /* package */ ChapFrame(Parcel in) {
    super(ID);
    this.chapterId = in.readString();
    this.startTime = in.readInt();
@@ -68,15 +68,15 @@ public final class ChapterFrame extends Id3Frame {
    if (obj == null || getClass() != obj.getClass()) {
      return false;
    }
-    ChapterFrame other = (ChapterFrame) obj;
+    ChapFrame other = (ChapFrame) obj;
-    return Util.areEqual(chapterId, other.chapterId)
+    return startTime == other.startTime
-      && startTime == other.startTime
      && endTime == other.endTime
      && startOffset == other.startOffset
      && endOffset == other.endOffset
-      && title != null ? title.equals(other.title) : other.title == null
+      && Util.areEqual(chapterId, other.chapterId)
-      && url != null ? url.equals(other.url) : other.url == null
+      && Util.areEqual(title, other.title)
-      && image != null ? image.equals(other.image) : other.image == null;
+      && Util.areEqual(url, other.url)
+      && Util.areEqual(image, other.image);
  }
  @Override
@@ -103,7 +103,7 @@ public final class ChapterFrame extends Id3Frame {
    dest.writeString(title);
    dest.writeString(url);
    dest.writeString(title);
-    dest.writeParcelable(this.image, flags);
+    dest.writeParcelable(image, flags);
  }
  @Override
@@ -111,15 +111,15 @@ public final class ChapterFrame extends Id3Frame {
    return 0;
  }
-  public static final Creator<ChapterFrame> CREATOR = new Creator<ChapterFrame>() {
+  public static final Creator<ChapFrame> CREATOR = new Creator<ChapFrame>() {
    @Override
-    public ChapterFrame createFromParcel(Parcel in) {
+    public ChapFrame createFromParcel(Parcel in) {
-      return new ChapterFrame(in);
+      return new ChapFrame(in);
    }
    @Override
-    public ChapterFrame[] newArray(int size) {
+    public ChapFrame[] newArray(int size) {
-      return new ChapterFrame[size];
+      return new ChapFrame[size];
    }
  };
 }
--- a/library/src/main/java/com/google/android/exoplayer2/metadata/id3/ChapterTOCFrame.java
+++ b/library/src/main/java/com/google/android/exoplayer2/metadata/id3/ChapterTOCFrame.java
 /*
- * Copyright (C) 2016 The Android Open Source Project
+ * Copyright (C) 2017 The Android Open Source Project
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
@@ -17,12 +17,14 @@ package com.google.android.exoplayer2.metadata.id3;
 import android.os.Parcel;
+import com.google.android.exoplayer2.util.Util;
 import java.util.Arrays;
 /**
 * Chapter table of contents information "CTOC" ID3 frame.
 */
-public class ChapterTOCFrame extends Id3Frame {
+public final class CtocFrame extends Id3Frame {
  public static final String ID = "CTOC";
@@ -32,7 +34,7 @@ public class ChapterTOCFrame extends Id3Frame {
  public final String[] children;
  public final String title;
-  public ChapterTOCFrame(String elementId, boolean isRoot, boolean isOrdered, String[] children, String title) {
+  public CtocFrame(String elementId, boolean isRoot, boolean isOrdered, String[] children, String title) {
    super(ID);
    this.elementId = elementId;
    this.isRoot = isRoot;
@@ -41,7 +43,7 @@ public class ChapterTOCFrame extends Id3Frame {
    this.title = title;
  }
-  /* package */ ChapterTOCFrame(Parcel in) {
+  /* package */ CtocFrame(Parcel in) {
    super(ID);
    this.elementId = in.readString();
    this.isRoot = in.readByte() != 0;
@@ -58,12 +60,12 @@ public class ChapterTOCFrame extends Id3Frame {
    if (obj == null || getClass() != obj.getClass()) {
      return false;
    }
-    ChapterTOCFrame other = (ChapterTOCFrame) obj;
+    CtocFrame other = (CtocFrame) obj;
-    return elementId != null ? elementId.equals(other.elementId) : other.elementId == null
+    return isRoot == other.isRoot
-      && isRoot == other.isRoot
      && isOrdered == other.isOrdered
-      && Arrays.equals(children, other.children)
+      && Util.areEqual(elementId, other.elementId)
-      && title != null ? title.equals(other.title) : other.title == null;
+      && Util.areEqual(title, other.title)
+      && Arrays.equals(children, other.children);
  }
  @Override
@@ -79,22 +81,22 @@ public class ChapterTOCFrame extends Id3Frame {
  @Override
  public void writeToParcel(Parcel dest, int flags) {
-    dest.writeString(this.elementId);
+    dest.writeString(elementId);
-    dest.writeByte((byte)(this.isRoot ? 1 : 0));
+    dest.writeByte((byte)(isRoot ? 1 : 0));
-    dest.writeByte((byte)(this.isOrdered ? 1 : 0));
+    dest.writeByte((byte)(isOrdered ? 1 : 0));
-    dest.writeStringArray(this.children);
+    dest.writeStringArray(children);
-    dest.writeString(this.title);
+    dest.writeString(title);
  }
-  public static final Creator<ChapterTOCFrame> CREATOR = new Creator<ChapterTOCFrame>() {
+  public static final Creator<CtocFrame> CREATOR = new Creator<CtocFrame>() {
    @Override
-    public ChapterTOCFrame createFromParcel(Parcel in) {
+    public CtocFrame createFromParcel(Parcel in) {
-      return new ChapterTOCFrame(in);
+      return new CtocFrame(in);
    }
    @Override
-    public ChapterTOCFrame[] newArray(int size) {
+    public CtocFrame[] newArray(int size) {
-      return new ChapterTOCFrame[size];
+      return new CtocFrame[size];
    }
  };
 }
--- a/library/src/main/java/com/google/android/exoplayer2/metadata/id3/Id3Decoder.java
+++ b/library/src/main/java/com/google/android/exoplayer2/metadata/id3/Id3Decoder.java
@@ -84,7 +84,7 @@ public final class Id3Decoder implements MetadataDecoder {
    int frameHeaderSize = id3Header.majorVersion == 2 ? 6 : 10;
    while (id3Data.bytesLeft() >= frameHeaderSize) {
-      Id3Frame frame = decodeFrame(id3Header.majorVersion, id3Data, unsignedIntFrameSizeHack);
+      Id3Frame frame = decodeFrame(id3Header.majorVersion, id3Data, unsignedIntFrameSizeHack, frameHeaderSize);
      if (frame != null) {
        id3Frames.add(frame);
      }
@@ -190,7 +190,7 @@ public final class Id3Decoder implements MetadataDecoder {
  }
  private static Id3Frame decodeFrame(int majorVersion, ParsableByteArray id3Data,
-      boolean unsignedIntFrameSizeHack)  {
+      boolean unsignedIntFrameSizeHack, int frameHeaderSize)  {
    int frameId0 = id3Data.readUnsignedByte();
    int frameId1 = id3Data.readUnsignedByte();
    int frameId2 = id3Data.readUnsignedByte();
@@ -282,12 +282,15 @@ public final class Id3Decoder implements MetadataDecoder {
      } else if (frameId0 == 'C' && frameId1 == 'O' && frameId2 == 'M'
          && (frameId3 == 'M' || majorVersion == 2)) {
        frame = decodeCommentFrame(id3Data, frameSize);
-      } else if (frameId0 == 'W' && frameId1 == 'X' && frameId2 == 'X' && frameId3 == 'X') {
+      } else if (majorVersion == 2 ? (frameId0 == 'W' && frameId1 == 'X' && frameId2 == 'X')
-        frame = decodeUrlLinkFrame(id3Data, frameSize);
+          : (frameId0 == 'W' && frameId1 == 'X' && frameId2 == 'X' && frameId3 == 'X')) {
+        frame = decodeWxxxFrame(id3Data, frameSize);
      } else if (frameId0 == 'C' && frameId1 == 'H' && frameId2 == 'A' && frameId3 == 'P') {
-        frame = decodeChapterFrame(id3Data, frameSize, majorVersion, unsignedIntFrameSizeHack);
+        frame = decodeChapFrame(id3Data, frameSize, majorVersion, unsignedIntFrameSizeHack,
+          frameHeaderSize);
      } else if (frameId0 == 'C' && frameId1 == 'T' && frameId2 == 'O' && frameId3 == 'C') {
-        frame = decodeChapterTOCFrame(id3Data, frameSize, majorVersion, unsignedIntFrameSizeHack);
+        frame = decodeCtocFrame(id3Data, frameSize, majorVersion, unsignedIntFrameSizeHack,
+          frameHeaderSize);
      } else {
        String id = majorVersion == 2
            ? String.format(Locale.US, "%c%c%c", frameId0, frameId1, frameId2)
@@ -450,7 +453,7 @@ public final class Id3Decoder implements MetadataDecoder {
    return new TextInformationFrame(id, description);
  }
-  private static UrlLinkFrame decodeUrlLinkFrame(ParsableByteArray id3Data,
+  private static WxxxFrame decodeWxxxFrame(ParsableByteArray id3Data,
      int frameSize) throws UnsupportedEncodingException {
    int encoding = id3Data.readUnsignedByte();
    String charset = getCharsetName(encoding);
@@ -461,16 +464,21 @@ public final class Id3Decoder implements MetadataDecoder {
    int descriptionEndIndex = indexOfEos(data, 0, encoding);
    String description = new String(data, 0, descriptionEndIndex, charset);
-    int urlStartIndex = descriptionEndIndex + 1;
+    String url;
-    int urlEndIndex = indexOfEos(data, urlStartIndex, encoding);
+    int urlStartIndex = descriptionEndIndex + delimiterLength(encoding);
-    int urlLength = urlEndIndex - urlStartIndex;
+    if (urlStartIndex < data.length) {
-    String url = new String(data, urlStartIndex, urlLength, charset);
+      int urlEndIndex = indexOfEos(data, urlStartIndex, encoding);
+      url = new String(data, urlStartIndex, urlEndIndex - urlStartIndex, charset);
+    } else {
+      url = "";
+    }
-    return new UrlLinkFrame(description, url);
+    return new WxxxFrame(description, url);
  }
-  private static ChapterFrame decodeChapterFrame(ParsableByteArray id3Data, int frameSize,
+  private static ChapFrame decodeChapFrame(ParsableByteArray id3Data, int frameSize,
-      int majorVersion, boolean unsignedIntFrameSizeHack) throws UnsupportedEncodingException {
+      int majorVersion, boolean unsignedIntFrameSizeHack, int frameHeaderSize)
+      throws UnsupportedEncodingException {
    byte[] frameBytes = new byte[frameSize];
    id3Data.readBytes(frameBytes, 0, frameSize - 1);
@@ -489,9 +497,9 @@ public final class Id3Decoder implements MetadataDecoder {
    String url = null;
    ApicFrame image = null;
-    int frameHeaderSize = majorVersion == 2 ? 6 : 10;
    while (chapterData.bytesLeft() >= frameHeaderSize) {
-      Id3Frame frame = decodeFrame(majorVersion, chapterData, unsignedIntFrameSizeHack);
+      Id3Frame frame = decodeFrame(majorVersion, chapterData, unsignedIntFrameSizeHack,
+        frameHeaderSize);
      if (frame == null) {
        continue;
      }
@@ -501,8 +509,8 @@ public final class Id3Decoder implements MetadataDecoder {
          title = textFrame.description;
        }
      }
-      else if (frame instanceof UrlLinkFrame) {
+      else if (frame instanceof WxxxFrame) {
-        UrlLinkFrame linkFrame = (UrlLinkFrame)frame;
+        WxxxFrame linkFrame = (WxxxFrame)frame;
        url = linkFrame.url;
      }
      else if (frame instanceof ApicFrame) {
@@ -510,11 +518,12 @@ public final class Id3Decoder implements MetadataDecoder {
      }
    }
-    return new ChapterFrame(chapterId, startTime, endTime, startOffset, endOffset, title, url, image);
+    return new ChapFrame(chapterId, startTime, endTime, startOffset, endOffset, title, url, image);
  }
-  private static ChapterTOCFrame decodeChapterTOCFrame(ParsableByteArray id3Data, int frameSize,
+  private static CtocFrame decodeCtocFrame(ParsableByteArray id3Data, int frameSize,
-    int majorVersion, boolean unsignedIntFrameSizeHack) throws UnsupportedEncodingException {
+      int majorVersion, boolean unsignedIntFrameSizeHack, int frameHeaderSize)
+      throws UnsupportedEncodingException {
    byte[] frameBytes = new byte[frameSize];
    id3Data.readBytes(frameBytes, 0, frameSize - 1);
@@ -530,7 +539,7 @@ public final class Id3Decoder implements MetadataDecoder {
    int entryCount = tocData.readUnsignedByte();
    String[] children = new String[entryCount];
-    for (int i = 0; i < entryCount && tocData.bytesLeft() > 0; i++) {
+    for (int i = 0; i < entryCount; i++) {
      int startIndex = tocData.getPosition();
      int endIndex = indexOfZeroByte(frameBytes, startIndex) + 1;
      int stringLength = endIndex - startIndex;
@@ -539,21 +548,18 @@ public final class Id3Decoder implements MetadataDecoder {
    }
    String title = null;
-    int frameHeaderSize = majorVersion == 2 ? 6 : 10;
    while (tocData.bytesLeft() >= frameHeaderSize) {
-      Id3Frame frame = decodeFrame(majorVersion, tocData, unsignedIntFrameSizeHack);
+      Id3Frame frame = decodeFrame(majorVersion, tocData, unsignedIntFrameSizeHack,
-      if (frame == null) {
+        frameHeaderSize);
-        continue;
-      }
      if (frame instanceof TextInformationFrame) {
        TextInformationFrame textFrame = (TextInformationFrame)frame;
-        if (textFrame.id != null && textFrame.id.equals("TIT2")) {
+        if ("TIT2".equals(textFrame.id)) {
          title = textFrame.description;
        }
      }
    }
-    return new ChapterTOCFrame(id, isRoot, isOrdered, children, title);
+    return new CtocFrame(id, isRoot, isOrdered, children, title);
  }
  private static BinaryFrame decodeBinaryFrame(ParsableByteArray id3Data, int frameSize,

--- a/library/src/main/java/com/google/android/exoplayer2/metadata/id3/UrlLinkFrame.java
+++ b/library/src/main/java/com/google/android/exoplayer2/metadata/id3/UrlLinkFrame.java
 /*
- * Copyright (C) 2016 The Android Open Source Project
+ * Copyright (C) 2017 The Android Open Source Project
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
@@ -23,20 +23,20 @@ import com.google.android.exoplayer2.util.Util;
 /**
 * Url Frame "WXX" ID3 frame.
 */
-public class UrlLinkFrame extends Id3Frame {
+public final class WxxxFrame extends Id3Frame {
  public static final String ID = "WXXX";
  public final String description;
  public final String url;
-  public UrlLinkFrame(String description, String url) {
+  public WxxxFrame(String description, String url) {
    super(ID);
    this.description = description;
    this.url = url;
  }
-  /* package */ UrlLinkFrame(Parcel in) {
+  /* package */ WxxxFrame(Parcel in) {
    super(ID);
    description = in.readString();
    url = in.readString();
@@ -50,7 +50,7 @@ public class UrlLinkFrame extends Id3Frame {
    if (obj == null || getClass() != obj.getClass()) {
      return false;
    }
-    UrlLinkFrame other = (UrlLinkFrame) obj;
+    WxxxFrame other = (WxxxFrame) obj;
    return Util.areEqual(description, other.description)
        && Util.areEqual(url, other.url);
  }
@@ -69,17 +69,17 @@ public class UrlLinkFrame extends Id3Frame {
    dest.writeString(url);
  }
-  public static final Parcelable.Creator<UrlLinkFrame> CREATOR =
+  public static final Parcelable.Creator<WxxxFrame> CREATOR =
-      new Parcelable.Creator<UrlLinkFrame>() {
+      new Parcelable.Creator<WxxxFrame>() {
      @Override
-      public UrlLinkFrame createFromParcel(Parcel in) {
+      public WxxxFrame createFromParcel(Parcel in) {
-        return new UrlLinkFrame(in);
+        return new WxxxFrame(in);
      }
      @Override
-      public UrlLinkFrame[] newArray(int size) {
+      public WxxxFrame[] newArray(int size) {
-        return new UrlLinkFrame[size];
+        return new WxxxFrame[size];
      }
    };