From 0eb56f65b063501abf0f629e83a35004b39e8a60 Mon Sep 17 00:00:00 2001 From: Alexander Capehart Date: Mon, 21 Nov 2022 11:55:14 -0700 Subject: [PATCH] Improve multi-value text frame parsing Rework the parsing of multi-value text frames to reduce duplication and increase efficiency. --- .../android/exoplayer2/ExoPlayerTest.java | 2 +- .../extractor/mp4/MetadataUtil.java | 9 ++-- .../exoplayer2/metadata/id3/Id3Decoder.java | 53 ++++++++----------- .../metadata/id3/TextInformationFrame.java | 7 +-- .../id3/TextInformationFrameTest.java | 30 ++++++----- 5 files changed, 49 insertions(+), 52 deletions(-) diff --git a/library/core/src/test/java/com/google/android/exoplayer2/ExoPlayerTest.java b/library/core/src/test/java/com/google/android/exoplayer2/ExoPlayerTest.java index 428c388de9..7c53406c74 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/ExoPlayerTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/ExoPlayerTest.java @@ -10395,7 +10395,7 @@ public final class ExoPlayerTest { new Metadata( new BinaryFrame(/* id= */ "", /* data= */ new byte[0]), new TextInformationFrame( - /* id= */ "TT2", /* description= */ null, /* value= */ "title"))) + /* id= */ "TT2", /* description= */ null, /* value= */ Collections.singletonList("title")))) .build(); // Set multiple values together. diff --git a/library/extractor/src/main/java/com/google/android/exoplayer2/extractor/mp4/MetadataUtil.java b/library/extractor/src/main/java/com/google/android/exoplayer2/extractor/mp4/MetadataUtil.java index 9aa19cff56..d91e0d423e 100644 --- a/library/extractor/src/main/java/com/google/android/exoplayer2/extractor/mp4/MetadataUtil.java +++ b/library/extractor/src/main/java/com/google/android/exoplayer2/extractor/mp4/MetadataUtil.java @@ -31,6 +31,7 @@ import com.google.android.exoplayer2.metadata.id3.TextInformationFrame; import com.google.android.exoplayer2.metadata.mp4.MdtaMetadataEntry; import com.google.android.exoplayer2.util.Log; import com.google.android.exoplayer2.util.ParsableByteArray; +import java.util.Collections; import org.checkerframework.checker.nullness.compatqual.NullableType; /** Utilities for handling metadata in MP4. */ @@ -452,7 +453,7 @@ import org.checkerframework.checker.nullness.compatqual.NullableType; if (atomType == Atom.TYPE_data) { data.skipBytes(8); // version (1), flags (3), empty (4) String value = data.readNullTerminatedString(atomSize - 16); - return new TextInformationFrame(id, /* description= */ null, value); + return new TextInformationFrame(id, /* description= */ null, Collections.singletonList(value)); } Log.w(TAG, "Failed to parse text attribute: " + Atom.getAtomTypeString(type)); return null; @@ -484,7 +485,7 @@ import org.checkerframework.checker.nullness.compatqual.NullableType; } if (value >= 0) { return isTextInformationFrame - ? new TextInformationFrame(id, /* description= */ null, Integer.toString(value)) + ? new TextInformationFrame(id, /* description= */ null, Collections.singletonList(Integer.toString(value))) : new CommentFrame(C.LANGUAGE_UNDETERMINED, id, Integer.toString(value)); } Log.w(TAG, "Failed to parse uint8 attribute: " + Atom.getAtomTypeString(type)); @@ -505,7 +506,7 @@ import org.checkerframework.checker.nullness.compatqual.NullableType; if (count > 0) { value += "/" + count; } - return new TextInformationFrame(attributeName, /* description= */ null, value); + return new TextInformationFrame(attributeName, /* description= */ null, Collections.singletonList(value)); } } Log.w(TAG, "Failed to parse index/count attribute: " + Atom.getAtomTypeString(type)); @@ -521,7 +522,7 @@ import org.checkerframework.checker.nullness.compatqual.NullableType; ? STANDARD_GENRES[genreCode - 1] : null; if (genreString != null) { - return new TextInformationFrame("TCON", /* description= */ null, genreString); + return new TextInformationFrame("TCON", /* description= */ null, Collections.singletonList(genreString)); } Log.w(TAG, "Failed to parse standard genre code"); return null; diff --git a/library/extractor/src/main/java/com/google/android/exoplayer2/metadata/id3/Id3Decoder.java b/library/extractor/src/main/java/com/google/android/exoplayer2/metadata/id3/Id3Decoder.java index 17d53a5c85..444befa1fc 100644 --- a/library/extractor/src/main/java/com/google/android/exoplayer2/metadata/id3/Id3Decoder.java +++ b/library/extractor/src/main/java/com/google/android/exoplayer2/metadata/id3/Id3Decoder.java @@ -15,6 +15,7 @@ */ package com.google.android.exoplayer2.metadata.id3; +import androidx.annotation.NonNull; import androidx.annotation.Nullable; import com.google.android.exoplayer2.C; import com.google.android.exoplayer2.metadata.Metadata; @@ -25,10 +26,12 @@ import com.google.android.exoplayer2.util.ParsableBitArray; import com.google.android.exoplayer2.util.ParsableByteArray; import com.google.android.exoplayer2.util.Util; import com.google.common.base.Ascii; +import com.google.common.collect.ImmutableList; import java.io.UnsupportedEncodingException; import java.nio.ByteBuffer; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.List; import java.util.Locale; @@ -458,26 +461,9 @@ public final class Id3Decoder extends SimpleMetadataDecoder { int descriptionEndIndex = indexOfTerminator(data, 0, encoding); String description = new String(data, 0, descriptionEndIndex, charset); - // In ID3v2.4, text information frames can contain multiple values delimited by a null - // terminator. Thus, we after each "end of stream" marker we actually need to keep looking - // for more data, at least until the index is equal to the data length. - ArrayList values = new ArrayList<>(); - - int valueStartIndex = descriptionEndIndex + delimiterLength(encoding); - if (valueStartIndex >= data.length) { - return new TextInformationFrame("TXXX", description, new String[0]); - } - - int valueEndIndex = indexOfTerminator(data, valueStartIndex, encoding); - while (valueStartIndex < valueEndIndex) { - String value = decodeStringIfValid(data, valueStartIndex, valueEndIndex, charset); - values.add(value); - - valueStartIndex = valueEndIndex + delimiterLength(encoding); - valueEndIndex = indexOfTerminator(data, valueStartIndex, encoding); - } - - return new TextInformationFrame("TXXX", description, values.toArray(new String[0])); + List values = decodeTextInformationFrameValues( + data, encoding, descriptionEndIndex + delimiterLength(encoding)); + return new TextInformationFrame("TXXX", description, values); } @Nullable @@ -489,31 +475,38 @@ public final class Id3Decoder extends SimpleMetadataDecoder { } int encoding = id3Data.readUnsignedByte(); - String charset = getCharsetName(encoding); byte[] data = new byte[frameSize - 1]; id3Data.readBytes(data, 0, frameSize - 1); + List values = decodeTextInformationFrameValues(data, encoding, 0); + return new TextInformationFrame(id, null, values); + } + + @NonNull + private static List decodeTextInformationFrameValues( + byte[] data, final int encoding, final int index) throws UnsupportedEncodingException { + ArrayList values = new ArrayList<>(); + + if (index >= data.length) { + return Collections.emptyList(); + } + // In ID3v2.4, text information frames can contain multiple values delimited by a null // terminator. Thus, we after each "end of stream" marker we actually need to keep looking // for more data, at least until the index is equal to the data length. - ArrayList values = new ArrayList<>(); - - int valueStartIndex = 0; - if (valueStartIndex >= data.length) { - return new TextInformationFrame(id, null, new String[0]); - } - + String charset = getCharsetName(encoding); + int valueStartIndex = index; int valueEndIndex = indexOfTerminator(data, valueStartIndex, encoding); while (valueStartIndex < valueEndIndex) { - String value = decodeStringIfValid(data, valueStartIndex, valueEndIndex, charset); + String value = new String(data, valueStartIndex, valueEndIndex - valueStartIndex, charset); values.add(value); valueStartIndex = valueEndIndex + delimiterLength(encoding); valueEndIndex = indexOfTerminator(data, valueStartIndex, encoding); } - return new TextInformationFrame(id, null, values.toArray(new String[0])); + return values; } @Nullable diff --git a/library/extractor/src/main/java/com/google/android/exoplayer2/metadata/id3/TextInformationFrame.java b/library/extractor/src/main/java/com/google/android/exoplayer2/metadata/id3/TextInformationFrame.java index 036878b706..0658f1340a 100644 --- a/library/extractor/src/main/java/com/google/android/exoplayer2/metadata/id3/TextInformationFrame.java +++ b/library/extractor/src/main/java/com/google/android/exoplayer2/metadata/id3/TextInformationFrame.java @@ -25,6 +25,7 @@ import com.google.android.exoplayer2.MediaMetadata; import com.google.android.exoplayer2.util.Util; import com.google.common.collect.ImmutableList; import java.util.ArrayList; +import java.util.Collections; import java.util.List; /** Text information ID3 frame. */ @@ -38,11 +39,11 @@ public final class TextInformationFrame extends Id3Frame { @NonNull public final ImmutableList values; - public TextInformationFrame(String id, @Nullable String description, @NonNull String[] values) { + public TextInformationFrame(String id, @Nullable String description, @NonNull List values) { super(id); this.description = description; - if( values.length > 0 ) { + if( values.size() > 0 ) { this.values = ImmutableList.copyOf(values); } else { this.values = ImmutableList.of(""); @@ -54,7 +55,7 @@ public final class TextInformationFrame extends Id3Frame { /** @deprecated Use {@code TextInformationFrame(String id, String description, String[] values} instead */ @Deprecated public TextInformationFrame(String id, @Nullable String description, String value) { - this(id, description, new String[] {value } ); + this(id, description, Collections.singletonList(value) ); } /* package */ TextInformationFrame(Parcel in) { diff --git a/library/extractor/src/test/java/com/google/android/exoplayer2/metadata/id3/TextInformationFrameTest.java b/library/extractor/src/test/java/com/google/android/exoplayer2/metadata/id3/TextInformationFrameTest.java index e7a9ffc795..dfc1dadbaa 100644 --- a/library/extractor/src/test/java/com/google/android/exoplayer2/metadata/id3/TextInformationFrameTest.java +++ b/library/extractor/src/test/java/com/google/android/exoplayer2/metadata/id3/TextInformationFrameTest.java @@ -22,6 +22,7 @@ import androidx.test.ext.junit.runners.AndroidJUnit4; import com.google.android.exoplayer2.MediaMetadata; import com.google.android.exoplayer2.metadata.Metadata; import com.google.common.collect.ImmutableList; +import java.util.Collections; import java.util.List; import org.junit.Test; import org.junit.runner.RunWith; @@ -32,7 +33,7 @@ public class TextInformationFrameTest { @Test public void parcelable() { - TextInformationFrame textInformationFrameToParcel = new TextInformationFrame("", "", ""); + TextInformationFrame textInformationFrameToParcel = new TextInformationFrame("", "", Collections.singletonList("")); Parcel parcel = Parcel.obtain(); textInformationFrameToParcel.writeToParcel(parcel, 0); @@ -62,28 +63,29 @@ public class TextInformationFrameTest { List entries = ImmutableList.of( - new TextInformationFrame(/* id= */ "TT2", /* description= */ null, /* value= */ new String[] { title }), - new TextInformationFrame(/* id= */ "TP1", /* description= */ null, /* value= */ new String[] { artist }), + new TextInformationFrame(/* id= */ "TT2", /* description= */ null, /* value= */ + Collections.singletonList(title)), + new TextInformationFrame(/* id= */ "TP1", /* description= */ null, /* values= */ Collections.singletonList( artist )), new TextInformationFrame( - /* id= */ "TAL", /* description= */ null, /* value= */ new String[] { albumTitle }), + /* id= */ "TAL", /* description= */ null, /* values= */ Collections.singletonList( albumTitle )), new TextInformationFrame( - /* id= */ "TP2", /* description= */ null, /* value= */ new String[] { albumArtist }), + /* id= */ "TP2", /* description= */ null, /* values= */ Collections.singletonList( albumArtist )), new TextInformationFrame( - /* id= */ "TRK", /* description= */ null, /* value= */ new String[] { trackNumberInfo}), + /* id= */ "TRK", /* description= */ null, /* values= */ Collections.singletonList( trackNumberInfo)), new TextInformationFrame( - /* id= */ "TYE", /* description= */ null, /* value= */ new String[] { recordingYear }), + /* id= */ "TYE", /* description= */ null, /* values= */ Collections.singletonList( recordingYear )), new TextInformationFrame( /* id= */ "TDA", /* description= */ null, - /* value= */ new String[] { recordingDay + recordingMonth }), + /* value= */ Collections.singletonList( recordingDay + recordingMonth )), new TextInformationFrame( - /* id= */ "TDRL", /* description= */ null, /* value= */ new String[] { releaseDate }), + /* id= */ "TDRL", /* description= */ null, /* values= */ Collections.singletonList( releaseDate )), new TextInformationFrame( - /* id= */ "TCM", /* description= */ null, /* value= */ new String[] { composer }), + /* id= */ "TCM", /* description= */ null, /* values= */ Collections.singletonList( composer )), new TextInformationFrame( - /* id= */ "TP3", /* description= */ null, /* value= */ new String[] { conductor }), + /* id= */ "TP3", /* description= */ null, /* values= */ Collections.singletonList( conductor )), new TextInformationFrame( - /* id= */ "TXT", /* description= */ null, /* value= */ new String[] { writer })); + /* id= */ "TXT", /* description= */ null, /* values= */ Collections.singletonList( writer ))); MediaMetadata.Builder builder = MediaMetadata.EMPTY.buildUpon(); for (Metadata.Entry entry : entries) { @@ -110,8 +112,8 @@ public class TextInformationFrameTest { // Test empty value array entries = - ImmutableList.of( - new TextInformationFrame(/* id= */ "TT2", /* description= */ null, /* value= */ new String[0]) + Collections.singletonList( + new TextInformationFrame(/* id= */ "TT2", /* description= */ null, /* values= */ Collections.emptyList()) ); builder = MediaMetadata.EMPTY.buildUpon();