-
Notifications
You must be signed in to change notification settings - Fork 6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added ID3 chapter support. #2333
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I've added a bunch of comments, but it's just minor stuff / nits. Please take a look, and we can get this merged. Thanks for the contribution!
&& endTime == other.endTime | ||
&& startOffset == other.startOffset | ||
&& endOffset == other.endOffset | ||
&& title != null ? title.equals(other.title) : other.title == null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- You can use Util.areEqual for this (ditto next 2 lines as well). It's neater + for consistency with L72.
return false; | ||
} | ||
ChapterFrame other = (ChapterFrame) obj; | ||
return Util.areEqual(chapterId, other.chapterId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nitty, but it's nice to put the more expensive checks after the cheap ones (i.e. do the int comparisons first, then put this one with the string ones further down).
return false; | ||
} | ||
ChapterTOCFrame other = (ChapterTOCFrame) obj; | ||
return elementId != null ? elementId.equals(other.elementId) : other.elementId == null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Util.areEquals (ditto for title). Do after primitive comparisons.
return elementId != null ? elementId.equals(other.elementId) : other.elementId == null | ||
&& isRoot == other.isRoot | ||
&& isOrdered == other.isOrdered | ||
&& Arrays.equals(children, other.children) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do this one last.
|
||
@Override | ||
public void writeToParcel(Parcel dest, int flags) { | ||
dest.writeString(this.elementId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: No need for "this." (ditto on other lines in this method).
} | ||
if (frame instanceof TextInformationFrame) { | ||
TextInformationFrame textFrame = (TextInformationFrame)frame; | ||
if (textFrame.id != null && textFrame.id.equals("TIT2")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By convention we do CONST.equals(variable) because it cannot result in a null pointer exception. It also simplifies the expression here, since you can just do:
"TIT2".equals(textFrame.id)
int frameHeaderSize = majorVersion == 2 ? 6 : 10; | ||
while (tocData.bytesLeft() >= frameHeaderSize) { | ||
Id3Frame frame = decodeFrame(majorVersion, tocData, unsignedIntFrameSizeHack); | ||
if (frame == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can remove this.
} | ||
if (frame instanceof TextInformationFrame) { | ||
TextInformationFrame textFrame = (TextInformationFrame)frame; | ||
if (textFrame.id != null && textFrame.id.equals("TIT2")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"TIT2".equals(textFrame.id)
|
||
int entryCount = tocData.readUnsignedByte(); | ||
String[] children = new String[entryCount]; | ||
for (int i = 0; i < entryCount && tocData.bytesLeft() > 0; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the tocData.bytesLeft() > 0 check really necessary? i < entryCount should be sufficient to ensure this unless the metadata is really quite broken.
String url = null; | ||
ApicFrame image = null; | ||
|
||
int frameHeaderSize = majorVersion == 2 ? 6 : 10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we pass frameheaderSize through decodeFrame (like we do for majorVersion) so that we don't have to calculate it again in two different places (ditto in decodeChapterTOCFrame)?
@@ -0,0 +1,125 @@ | |||
/* | |||
* Copyright (C) 2016 The Android Open Source Project |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2017 for new files please (don't update header in existing files though).
@@ -282,6 +282,12 @@ private static Id3Frame decodeFrame(int majorVersion, ParsableByteArray id3Data, | |||
} 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') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to handle WXX if majorVersion == 2. See the condition for calling decodeTxxxFrame above to see how to do this.
@ojw28 Thanks for the great feedback. I changed the name of the frame classes to match the style of the project. ChapterFrame is ChapFrame, ChapterTOCFrame is CtocFrame and UrlLinkFrame is WxxxFrame. Please let me know if you aren't happy with that or any other changes and I will fix it. |
int majorVersion, boolean unsignedIntFrameSizeHack, int frameHeaderSize) | ||
throws UnsupportedEncodingException { | ||
byte[] frameBytes = new byte[frameSize]; | ||
id3Data.readBytes(frameBytes, 0, frameSize - 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be:
id3Data.readBytes(frameBytes, 0, frameSize);
int majorVersion, boolean unsignedIntFrameSizeHack, int frameHeaderSize) | ||
throws UnsupportedEncodingException { | ||
byte[] frameBytes = new byte[frameSize]; | ||
id3Data.readBytes(frameBytes, 0, frameSize - 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
Improved the ID3 decoder to include support for chapter frames.
See issue for more details #2316