From b125a2b0b3a0910f057713844e5508d111c71050 Mon Sep 17 00:00:00 2001 From: Alexander Capehart Date: Wed, 7 Sep 2022 20:40:10 -0600 Subject: [PATCH 01/10] Add support for ID3v2.4 multi-value tags Add support for multi-value tags based on null terminators. These are specific to ID3v2.4, but is backwards compatible with ID3v2.3, so no version checks are needed. --- .../exoplayer2/metadata/id3/Id3Decoder.java | 36 +++++++++-- .../metadata/id3/TextInformationFrame.java | 64 +++++++++++++------ 2 files changed, 75 insertions(+), 25 deletions(-) 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 46bd482178..473cf12ae7 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 @@ -29,8 +29,10 @@ 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; +import org.w3c.dom.Text; /** Decodes ID3 tags. */ public final class Id3Decoder extends SimpleMetadataDecoder { @@ -458,11 +460,23 @@ public final class Id3Decoder extends SimpleMetadataDecoder { int descriptionEndIndex = indexOfEos(data, 0, encoding); String description = new String(data, 0, descriptionEndIndex, charset); + // 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); int valueEndIndex = indexOfEos(data, valueStartIndex, encoding); - String value = decodeStringIfValid(data, valueStartIndex, valueEndIndex, charset); + while (valueStartIndex < valueEndIndex) { + String value = decodeStringIfValid(data, valueStartIndex, valueEndIndex, charset); + values.add(value); - return new TextInformationFrame("TXXX", description, value); + valueStartIndex = valueEndIndex + delimiterLength(encoding); + valueEndIndex = indexOfEos(data, valueStartIndex, encoding); + } + + + return new TextInformationFrame("TXXX", description, values.toArray(new String[0])); } @Nullable @@ -479,10 +493,22 @@ public final class Id3Decoder extends SimpleMetadataDecoder { byte[] data = new byte[frameSize - 1]; id3Data.readBytes(data, 0, frameSize - 1); - int valueEndIndex = indexOfEos(data, 0, encoding); - String value = new String(data, 0, valueEndIndex, charset); + // 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<>(); - return new TextInformationFrame(id, null, value); + int valueStartIndex = 0; + int valueEndIndex = indexOfEos(data, valueStartIndex, encoding); + while (valueStartIndex < valueEndIndex) { + String value = decodeStringIfValid(data, valueStartIndex, valueEndIndex, charset); + values.add(value); + + valueStartIndex = valueEndIndex + delimiterLength(encoding); + valueEndIndex = indexOfEos(data, valueStartIndex, encoding); + } + + return new TextInformationFrame(id, null, values.toArray(new String[0])); } @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 7bab697331..fe2f5652a7 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 @@ -23,48 +23,71 @@ import androidx.annotation.Nullable; import com.google.android.exoplayer2.MediaMetadata; import com.google.android.exoplayer2.util.Util; import java.util.ArrayList; +import java.util.Arrays; import java.util.List; /** Text information ID3 frame. */ public final class TextInformationFrame extends Id3Frame { + private final static String MULTI_VALUE_DELIMITER = ", "; @Nullable public final String description; + + /** @deprecated Use {@code values} instead. */ + @Deprecated public final String value; - public TextInformationFrame(String id, @Nullable String description, String value) { + public final String[] values; + + public TextInformationFrame(String id, @Nullable String description, String[] values) { super(id); + if (values.length == 0) { + throw new IllegalArgumentException("A text information frame must have at least one value."); + } + this.description = description; - this.value = value; + this.values = values; + this.value = values[0]; + } + + /** @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 } ); } /* package */ TextInformationFrame(Parcel in) { super(castNonNull(in.readString())); description = in.readString(); - value = castNonNull(in.readString()); + values = new String[] {}; + in.readStringArray(values); + this.value = values[0]; } @Override public void populateMediaMetadata(MediaMetadata.Builder builder) { + // Depending on the context this frame is in, we either take the first value of a multi-value + // frame because multiple values make no sense, or we join the values together with a comma + // when multiple values do make sense. switch (id) { case "TT2": case "TIT2": - builder.setTitle(value); + builder.setTitle(values[0]); break; case "TP1": case "TPE1": - builder.setArtist(value); + builder.setArtist(String.join(MULTI_VALUE_DELIMITER, values)); break; case "TP2": case "TPE2": - builder.setAlbumArtist(value); + builder.setAlbumArtist(String.join(MULTI_VALUE_DELIMITER, values)); break; case "TAL": case "TALB": - builder.setAlbumTitle(value); + builder.setAlbumTitle(values[0]); break; case "TRK": case "TRCK": - String[] trackNumbers = Util.split(value, "/"); + String[] trackNumbers = Util.split(values[0], "/"); try { int trackNumber = Integer.parseInt(trackNumbers[0]); @Nullable @@ -78,7 +101,7 @@ public final class TextInformationFrame extends Id3Frame { case "TYE": case "TYER": try { - builder.setRecordingYear(Integer.parseInt(value)); + builder.setRecordingYear(Integer.parseInt(values[0])); } catch (NumberFormatException e) { // Do nothing, invalid input. } @@ -86,15 +109,16 @@ public final class TextInformationFrame extends Id3Frame { case "TDA": case "TDAT": try { - int month = Integer.parseInt(value.substring(2, 4)); - int day = Integer.parseInt(value.substring(0, 2)); + String date = values[0]; + int month = Integer.parseInt(date.substring(2, 4)); + int day = Integer.parseInt(date.substring(0, 2)); builder.setRecordingMonth(month).setRecordingDay(day); } catch (NumberFormatException | StringIndexOutOfBoundsException e) { // Do nothing, invalid input. } break; case "TDRC": - List recordingDate = parseId3v2point4TimestampFrameForDate(value); + List recordingDate = parseId3v2point4TimestampFrameForDate(values[0]); switch (recordingDate.size()) { case 3: builder.setRecordingDay(recordingDate.get(2)); @@ -112,7 +136,7 @@ public final class TextInformationFrame extends Id3Frame { } break; case "TDRL": - List releaseDate = parseId3v2point4TimestampFrameForDate(value); + List releaseDate = parseId3v2point4TimestampFrameForDate(values[0]); switch (releaseDate.size()) { case 3: builder.setReleaseDay(releaseDate.get(2)); @@ -131,15 +155,15 @@ public final class TextInformationFrame extends Id3Frame { break; case "TCM": case "TCOM": - builder.setComposer(value); + builder.setComposer(String.join(MULTI_VALUE_DELIMITER, values)); break; case "TP3": case "TPE3": - builder.setConductor(value); + builder.setConductor(String.join(MULTI_VALUE_DELIMITER, values)); break; case "TXT": case "TEXT": - builder.setWriter(value); + builder.setWriter(String.join(MULTI_VALUE_DELIMITER, values)); break; default: break; @@ -157,7 +181,7 @@ public final class TextInformationFrame extends Id3Frame { TextInformationFrame other = (TextInformationFrame) obj; return Util.areEqual(id, other.id) && Util.areEqual(description, other.description) - && Util.areEqual(value, other.value); + && Util.areEqual(values, other.values); } @Override @@ -165,13 +189,13 @@ public final class TextInformationFrame extends Id3Frame { int result = 17; result = 31 * result + id.hashCode(); result = 31 * result + (description != null ? description.hashCode() : 0); - result = 31 * result + (value != null ? value.hashCode() : 0); + result = 31 * result + Arrays.hashCode(values); return result; } @Override public String toString() { - return id + ": description=" + description + ": value=" + value; + return id + ": description=" + description + ": values=" + String.join(MULTI_VALUE_DELIMITER, values); } // Parcelable implementation. @@ -180,7 +204,7 @@ public final class TextInformationFrame extends Id3Frame { public void writeToParcel(Parcel dest, int flags) { dest.writeString(id); dest.writeString(description); - dest.writeString(value); + dest.writeStringArray(values); } public static final Parcelable.Creator CREATOR = From 0cdece3a3f9beb237e7558ab8c48ca4d74c75d40 Mon Sep 17 00:00:00 2001 From: Alexander Capehart Date: Wed, 7 Sep 2022 20:57:53 -0600 Subject: [PATCH 02/10] Clarify comments Clarify that null-termianted multi-value separators are specific to ID3v2.4. --- .../exoplayer2/metadata/id3/Id3Decoder.java | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) 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 473cf12ae7..bf0ab5a5ca 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 @@ -459,10 +459,10 @@ public final class Id3Decoder extends SimpleMetadataDecoder { int descriptionEndIndex = indexOfEos(data, 0, encoding); String description = new String(data, 0, descriptionEndIndex, charset); - - // 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. + + // 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); @@ -475,7 +475,6 @@ public final class Id3Decoder extends SimpleMetadataDecoder { valueEndIndex = indexOfEos(data, valueStartIndex, encoding); } - return new TextInformationFrame("TXXX", description, values.toArray(new String[0])); } @@ -493,9 +492,9 @@ public final class Id3Decoder extends SimpleMetadataDecoder { byte[] data = new byte[frameSize - 1]; id3Data.readBytes(data, 0, frameSize - 1); - // 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. + // 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; From b054813e6da86c9d17b12af309d6c8c232011863 Mon Sep 17 00:00:00 2001 From: Alexander Capehart Date: Wed, 7 Sep 2022 22:11:06 -0600 Subject: [PATCH 03/10] Rename eos terminology Rename indexOfEos to indexOfTerminator to better reflect how a terminator != the end of a frame now. --- .../exoplayer2/metadata/id3/Id3Decoder.java | 28 +++++++++---------- 1 file changed, 13 insertions(+), 15 deletions(-) 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 bf0ab5a5ca..bfe745b88f 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 @@ -29,10 +29,8 @@ 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; -import org.w3c.dom.Text; /** Decodes ID3 tags. */ public final class Id3Decoder extends SimpleMetadataDecoder { @@ -457,22 +455,22 @@ public final class Id3Decoder extends SimpleMetadataDecoder { byte[] data = new byte[frameSize - 1]; id3Data.readBytes(data, 0, frameSize - 1); - int descriptionEndIndex = indexOfEos(data, 0, encoding); + 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); - int valueEndIndex = indexOfEos(data, valueStartIndex, encoding); + int valueEndIndex = indexOfTerminator(data, valueStartIndex, encoding); while (valueStartIndex < valueEndIndex) { String value = decodeStringIfValid(data, valueStartIndex, valueEndIndex, charset); values.add(value); valueStartIndex = valueEndIndex + delimiterLength(encoding); - valueEndIndex = indexOfEos(data, valueStartIndex, encoding); + valueEndIndex = indexOfTerminator(data, valueStartIndex, encoding); } return new TextInformationFrame("TXXX", description, values.toArray(new String[0])); @@ -498,13 +496,13 @@ public final class Id3Decoder extends SimpleMetadataDecoder { ArrayList values = new ArrayList<>(); int valueStartIndex = 0; - int valueEndIndex = indexOfEos(data, valueStartIndex, encoding); + int valueEndIndex = indexOfTerminator(data, valueStartIndex, encoding); while (valueStartIndex < valueEndIndex) { String value = decodeStringIfValid(data, valueStartIndex, valueEndIndex, charset); values.add(value); valueStartIndex = valueEndIndex + delimiterLength(encoding); - valueEndIndex = indexOfEos(data, valueStartIndex, encoding); + valueEndIndex = indexOfTerminator(data, valueStartIndex, encoding); } return new TextInformationFrame(id, null, values.toArray(new String[0])); @@ -524,7 +522,7 @@ public final class Id3Decoder extends SimpleMetadataDecoder { byte[] data = new byte[frameSize - 1]; id3Data.readBytes(data, 0, frameSize - 1); - int descriptionEndIndex = indexOfEos(data, 0, encoding); + int descriptionEndIndex = indexOfTerminator(data, 0, encoding); String description = new String(data, 0, descriptionEndIndex, charset); int urlStartIndex = descriptionEndIndex + delimiterLength(encoding); @@ -571,11 +569,11 @@ public final class Id3Decoder extends SimpleMetadataDecoder { String mimeType = new String(data, 0, mimeTypeEndIndex, "ISO-8859-1"); int filenameStartIndex = mimeTypeEndIndex + 1; - int filenameEndIndex = indexOfEos(data, filenameStartIndex, encoding); + int filenameEndIndex = indexOfTerminator(data, filenameStartIndex, encoding); String filename = decodeStringIfValid(data, filenameStartIndex, filenameEndIndex, charset); int descriptionStartIndex = filenameEndIndex + delimiterLength(encoding); - int descriptionEndIndex = indexOfEos(data, descriptionStartIndex, encoding); + int descriptionEndIndex = indexOfTerminator(data, descriptionStartIndex, encoding); String description = decodeStringIfValid(data, descriptionStartIndex, descriptionEndIndex, charset); @@ -613,7 +611,7 @@ public final class Id3Decoder extends SimpleMetadataDecoder { int pictureType = data[mimeTypeEndIndex + 1] & 0xFF; int descriptionStartIndex = mimeTypeEndIndex + 2; - int descriptionEndIndex = indexOfEos(data, descriptionStartIndex, encoding); + int descriptionEndIndex = indexOfTerminator(data, descriptionStartIndex, encoding); String description = new String( data, descriptionStartIndex, descriptionEndIndex - descriptionStartIndex, charset); @@ -642,11 +640,11 @@ public final class Id3Decoder extends SimpleMetadataDecoder { data = new byte[frameSize - 4]; id3Data.readBytes(data, 0, frameSize - 4); - int descriptionEndIndex = indexOfEos(data, 0, encoding); + int descriptionEndIndex = indexOfTerminator(data, 0, encoding); String description = new String(data, 0, descriptionEndIndex, charset); int textStartIndex = descriptionEndIndex + delimiterLength(encoding); - int textEndIndex = indexOfEos(data, textStartIndex, encoding); + int textEndIndex = indexOfTerminator(data, textStartIndex, encoding); String text = decodeStringIfValid(data, textStartIndex, textEndIndex, charset); return new CommentFrame(language, description, text); @@ -823,7 +821,7 @@ public final class Id3Decoder extends SimpleMetadataDecoder { : String.format(Locale.US, "%c%c%c%c", frameId0, frameId1, frameId2, frameId3); } - private static int indexOfEos(byte[] data, int fromIndex, int encoding) { + private static int indexOfTerminator(byte[] data, int fromIndex, int encoding) { int terminationPos = indexOfZeroByte(data, fromIndex); // For single byte encoding charsets, we're done. From d8729e552cda2f3c748f722e6e21cf96221343c8 Mon Sep 17 00:00:00 2001 From: Alexander Capehart Date: Sun, 20 Nov 2022 18:02:45 -0700 Subject: [PATCH 04/10] Fix second-order issues Fix second-order issues stemming from the addition of multi-value tags. --- .../ImaServerSideAdInsertionMediaSource.java | 2 +- .../metadata/MetadataRendererTest.java | 2 +- .../extractor/mp3/Mp3Extractor.java | 2 +- .../exoplayer2/metadata/id3/Id3Decoder.java | 8 +++++++ .../metadata/id3/TextInformationFrame.java | 22 ++++++++++--------- .../metadata/id3/Id3DecoderTest.java | 11 +++++----- 6 files changed, 29 insertions(+), 18 deletions(-) diff --git a/extensions/ima/src/main/java/com/google/android/exoplayer2/ext/ima/ImaServerSideAdInsertionMediaSource.java b/extensions/ima/src/main/java/com/google/android/exoplayer2/ext/ima/ImaServerSideAdInsertionMediaSource.java index c06b39da59..a592efbcb4 100644 --- a/extensions/ima/src/main/java/com/google/android/exoplayer2/ext/ima/ImaServerSideAdInsertionMediaSource.java +++ b/extensions/ima/src/main/java/com/google/android/exoplayer2/ext/ima/ImaServerSideAdInsertionMediaSource.java @@ -824,7 +824,7 @@ public final class ImaServerSideAdInsertionMediaSource extends CompositeMediaSou if (entry instanceof TextInformationFrame) { TextInformationFrame textFrame = (TextInformationFrame) entry; if ("TXXX".equals(textFrame.id)) { - streamPlayer.triggerUserTextReceived(textFrame.value); + streamPlayer.triggerUserTextReceived(textFrame.values[0]); } } else if (entry instanceof EventMessage) { EventMessage eventMessage = (EventMessage) entry; diff --git a/library/core/src/test/java/com/google/android/exoplayer2/metadata/MetadataRendererTest.java b/library/core/src/test/java/com/google/android/exoplayer2/metadata/MetadataRendererTest.java index 42dcaa572d..85e49d5834 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/metadata/MetadataRendererTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/metadata/MetadataRendererTest.java @@ -107,7 +107,7 @@ public class MetadataRendererTest { assertThat(metadata).hasSize(1); assertThat(metadata.get(0).length()).isEqualTo(1); TextInformationFrame expectedId3Frame = - new TextInformationFrame("TXXX", "Test description", "Test value"); + new TextInformationFrame("TXXX", "Test description", new String[] { "Test value" }); assertThat(metadata.get(0).get(0)).isEqualTo(expectedId3Frame); } diff --git a/library/extractor/src/main/java/com/google/android/exoplayer2/extractor/mp3/Mp3Extractor.java b/library/extractor/src/main/java/com/google/android/exoplayer2/extractor/mp3/Mp3Extractor.java index 245e99c2e4..f0621ab1c2 100644 --- a/library/extractor/src/main/java/com/google/android/exoplayer2/extractor/mp3/Mp3Extractor.java +++ b/library/extractor/src/main/java/com/google/android/exoplayer2/extractor/mp3/Mp3Extractor.java @@ -592,7 +592,7 @@ public final class Mp3Extractor implements Extractor { Metadata.Entry entry = metadata.get(i); if (entry instanceof TextInformationFrame && ((TextInformationFrame) entry).id.equals("TLEN")) { - return Util.msToUs(Long.parseLong(((TextInformationFrame) entry).value)); + return Util.msToUs(Long.parseLong(((TextInformationFrame) entry).values[0])); } } } 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 bfe745b88f..17d53a5c85 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 @@ -464,6 +464,10 @@ public final class Id3Decoder extends SimpleMetadataDecoder { 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); @@ -496,6 +500,10 @@ public final class Id3Decoder extends SimpleMetadataDecoder { ArrayList values = new ArrayList<>(); int valueStartIndex = 0; + if (valueStartIndex >= data.length) { + return new TextInformationFrame(id, null, new String[0]); + } + int valueEndIndex = indexOfTerminator(data, valueStartIndex, encoding); while (valueStartIndex < valueEndIndex) { String value = decodeStringIfValid(data, valueStartIndex, valueEndIndex, charset); 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 fe2f5652a7..fc1f26d7d9 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 @@ -19,6 +19,7 @@ import static com.google.android.exoplayer2.util.Util.castNonNull; import android.os.Parcel; import android.os.Parcelable; +import androidx.annotation.NonNull; import androidx.annotation.Nullable; import com.google.android.exoplayer2.MediaMetadata; import com.google.android.exoplayer2.util.Util; @@ -36,17 +37,19 @@ public final class TextInformationFrame extends Id3Frame { @Deprecated public final String value; + @NonNull public final String[] values; - public TextInformationFrame(String id, @Nullable String description, String[] values) { + public TextInformationFrame(String id, @Nullable String description, @NonNull String[] values) { super(id); - if (values.length == 0) { - throw new IllegalArgumentException("A text information frame must have at least one value."); - } - this.description = description; this.values = values; - this.value = values[0]; + + if (values.length > 0) { + this.value = values[0]; + } else { + this.value = null; + } } /** @deprecated Use {@code TextInformationFrame(String id, String description, String[] values} instead */ @@ -58,8 +61,7 @@ public final class TextInformationFrame extends Id3Frame { /* package */ TextInformationFrame(Parcel in) { super(castNonNull(in.readString())); description = in.readString(); - values = new String[] {}; - in.readStringArray(values); + values = in.createStringArray(); this.value = values[0]; } @@ -181,7 +183,7 @@ public final class TextInformationFrame extends Id3Frame { TextInformationFrame other = (TextInformationFrame) obj; return Util.areEqual(id, other.id) && Util.areEqual(description, other.description) - && Util.areEqual(values, other.values); + && Arrays.equals(values, other.values); } @Override @@ -195,7 +197,7 @@ public final class TextInformationFrame extends Id3Frame { @Override public String toString() { - return id + ": description=" + description + ": values=" + String.join(MULTI_VALUE_DELIMITER, values); + return id + ": description=" + description + ": value=" + String.join(MULTI_VALUE_DELIMITER, values); } // Parcelable implementation. diff --git a/library/extractor/src/test/java/com/google/android/exoplayer2/metadata/id3/Id3DecoderTest.java b/library/extractor/src/test/java/com/google/android/exoplayer2/metadata/id3/Id3DecoderTest.java index 833a290cea..ae53ac3c43 100644 --- a/library/extractor/src/test/java/com/google/android/exoplayer2/metadata/id3/Id3DecoderTest.java +++ b/library/extractor/src/test/java/com/google/android/exoplayer2/metadata/id3/Id3DecoderTest.java @@ -52,7 +52,7 @@ public final class Id3DecoderTest { TextInformationFrame textInformationFrame = (TextInformationFrame) metadata.get(0); assertThat(textInformationFrame.id).isEqualTo("TXXX"); assertThat(textInformationFrame.description).isEmpty(); - assertThat(textInformationFrame.value).isEqualTo("mdialog_VINDICO1527664_start"); + assertThat(textInformationFrame.values[0]).isEqualTo("mdialog_VINDICO1527664_start"); // Test UTF-16. rawId3 = @@ -67,7 +67,7 @@ public final class Id3DecoderTest { textInformationFrame = (TextInformationFrame) metadata.get(0); assertThat(textInformationFrame.id).isEqualTo("TXXX"); assertThat(textInformationFrame.description).isEqualTo("Hello World"); - assertThat(textInformationFrame.value).isEmpty(); + assertThat(textInformationFrame.values).isEmpty(); // Test empty. rawId3 = buildSingleFrameTag("TXXX", new byte[0]); @@ -81,7 +81,7 @@ public final class Id3DecoderTest { textInformationFrame = (TextInformationFrame) metadata.get(0); assertThat(textInformationFrame.id).isEqualTo("TXXX"); assertThat(textInformationFrame.description).isEmpty(); - assertThat(textInformationFrame.value).isEmpty(); + assertThat(textInformationFrame.values).isEmpty(); } @Test @@ -95,7 +95,8 @@ public final class Id3DecoderTest { TextInformationFrame textInformationFrame = (TextInformationFrame) metadata.get(0); assertThat(textInformationFrame.id).isEqualTo("TIT2"); assertThat(textInformationFrame.description).isNull(); - assertThat(textInformationFrame.value).isEqualTo("Hello World"); + assertThat(textInformationFrame.values.length).isEqualTo(1); + assertThat(textInformationFrame.values[0]).isEqualTo("Hello World"); // Test empty. rawId3 = buildSingleFrameTag("TIT2", new byte[0]); @@ -109,7 +110,7 @@ public final class Id3DecoderTest { textInformationFrame = (TextInformationFrame) metadata.get(0); assertThat(textInformationFrame.id).isEqualTo("TIT2"); assertThat(textInformationFrame.description).isNull(); - assertThat(textInformationFrame.value).isEmpty(); + assertThat(textInformationFrame.values).isEmpty(); } @Test From c96b55029dc20269549357cdd33e621a1d3e2fb5 Mon Sep 17 00:00:00 2001 From: Alexander Capehart Date: Sun, 20 Nov 2022 18:28:30 -0700 Subject: [PATCH 05/10] Add ID3v2 multi-value tests Add tests for ID3v2 multi-value text frames. --- .../metadata/id3/Id3DecoderTest.java | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/library/extractor/src/test/java/com/google/android/exoplayer2/metadata/id3/Id3DecoderTest.java b/library/extractor/src/test/java/com/google/android/exoplayer2/metadata/id3/Id3DecoderTest.java index ae53ac3c43..8226771abd 100644 --- a/library/extractor/src/test/java/com/google/android/exoplayer2/metadata/id3/Id3DecoderTest.java +++ b/library/extractor/src/test/java/com/google/android/exoplayer2/metadata/id3/Id3DecoderTest.java @@ -69,6 +69,21 @@ public final class Id3DecoderTest { assertThat(textInformationFrame.description).isEqualTo("Hello World"); assertThat(textInformationFrame.values).isEmpty(); + // Test multiple values. + rawId3 = buildSingleFrameTag( + "TXXX", + new byte[] { + 1, 0, 72, 0, 101, 0, 108, 0, 108, 0, 111, 0, 32, 0, 87, 0, 111, 0, 114, 0, 108, 0, + 100, 0, 0, 0, 70, 0, 111, 0, 111, 0, 0, 0, 66, 0, 97, 0, 114, 0, 0 + }); + metadata = decoder.decode(rawId3, rawId3.length); + assertThat(metadata.length()).isEqualTo(1); + textInformationFrame = (TextInformationFrame) metadata.get(0); + assertThat(textInformationFrame.description).isEqualTo("Hello World"); + assertThat(textInformationFrame.values.length).isEqualTo(2); + assertThat(textInformationFrame.values[0]).isEqualTo("Foo"); + assertThat(textInformationFrame.values[1]).isEqualTo("Bar"); + // Test empty. rawId3 = buildSingleFrameTag("TXXX", new byte[0]); metadata = decoder.decode(rawId3, rawId3.length); @@ -98,6 +113,15 @@ public final class Id3DecoderTest { assertThat(textInformationFrame.values.length).isEqualTo(1); assertThat(textInformationFrame.values[0]).isEqualTo("Hello World"); + // Test multiple values. + rawId3 = buildSingleFrameTag("TIT2", new byte[] {3, 70, 111, 111, 0, 66, 97, 114, 0}); + metadata = decoder.decode(rawId3, rawId3.length); + assertThat(metadata.length()).isEqualTo(1); + textInformationFrame = (TextInformationFrame) metadata.get(0); + assertThat(textInformationFrame.values.length).isEqualTo(2); + assertThat(textInformationFrame.values[0]).isEqualTo("Foo"); + assertThat(textInformationFrame.values[1]).isEqualTo("Bar"); + // Test empty. rawId3 = buildSingleFrameTag("TIT2", new byte[0]); metadata = decoder.decode(rawId3, rawId3.length); From 157f1f0787f44def613b585d33cbb55641f5753d Mon Sep 17 00:00:00 2001 From: Alexander Capehart Date: Mon, 21 Nov 2022 11:33:48 -0700 Subject: [PATCH 06/10] Rework ID3v2 text frame values interface Rework the interface for ID3v2 text frames by: - Switching from a mutable array to ImmutableList - Never leaving the array empty, instead filling it with "" if there are no values This makes the surface safer and more in line with the rest of the module. --- .../ImaServerSideAdInsertionMediaSource.java | 2 +- .../extractor/mp3/Mp3Extractor.java | 2 +- .../metadata/id3/TextInformationFrame.java | 55 +++++++++---------- .../metadata/id3/Id3DecoderTest.java | 27 +++++---- .../id3/TextInformationFrameTest.java | 38 +++++++++---- 5 files changed, 71 insertions(+), 53 deletions(-) diff --git a/extensions/ima/src/main/java/com/google/android/exoplayer2/ext/ima/ImaServerSideAdInsertionMediaSource.java b/extensions/ima/src/main/java/com/google/android/exoplayer2/ext/ima/ImaServerSideAdInsertionMediaSource.java index 2632aaf0d0..d813aeb445 100644 --- a/extensions/ima/src/main/java/com/google/android/exoplayer2/ext/ima/ImaServerSideAdInsertionMediaSource.java +++ b/extensions/ima/src/main/java/com/google/android/exoplayer2/ext/ima/ImaServerSideAdInsertionMediaSource.java @@ -811,7 +811,7 @@ public final class ImaServerSideAdInsertionMediaSource extends CompositeMediaSou if (entry instanceof TextInformationFrame) { TextInformationFrame textFrame = (TextInformationFrame) entry; if ("TXXX".equals(textFrame.id)) { - streamPlayer.triggerUserTextReceived(textFrame.values[0]); + streamPlayer.triggerUserTextReceived(textFrame.values.get(0)); } } else if (entry instanceof EventMessage) { EventMessage eventMessage = (EventMessage) entry; diff --git a/library/extractor/src/main/java/com/google/android/exoplayer2/extractor/mp3/Mp3Extractor.java b/library/extractor/src/main/java/com/google/android/exoplayer2/extractor/mp3/Mp3Extractor.java index f0621ab1c2..40bf68e152 100644 --- a/library/extractor/src/main/java/com/google/android/exoplayer2/extractor/mp3/Mp3Extractor.java +++ b/library/extractor/src/main/java/com/google/android/exoplayer2/extractor/mp3/Mp3Extractor.java @@ -592,7 +592,7 @@ public final class Mp3Extractor implements Extractor { Metadata.Entry entry = metadata.get(i); if (entry instanceof TextInformationFrame && ((TextInformationFrame) entry).id.equals("TLEN")) { - return Util.msToUs(Long.parseLong(((TextInformationFrame) entry).values[0])); + return Util.msToUs(Long.parseLong(((TextInformationFrame) entry).values.get(0))); } } } 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 fc1f26d7d9..036878b706 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 @@ -23,33 +23,32 @@ import androidx.annotation.NonNull; import androidx.annotation.Nullable; 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.Arrays; import java.util.List; /** Text information ID3 frame. */ public final class TextInformationFrame extends Id3Frame { - private final static String MULTI_VALUE_DELIMITER = ", "; - @Nullable public final String description; - /** @deprecated Use {@code values} instead. */ + /** @deprecated Use {@code values[0]} instead. */ @Deprecated public final String value; @NonNull - public final String[] values; + public final ImmutableList values; public TextInformationFrame(String id, @Nullable String description, @NonNull String[] values) { super(id); this.description = description; - this.values = values; - - if (values.length > 0) { - this.value = values[0]; + + if( values.length > 0 ) { + this.values = ImmutableList.copyOf(values); } else { - this.value = null; + this.values = ImmutableList.of(""); } + + this.value = this.values.get(0); } /** @deprecated Use {@code TextInformationFrame(String id, String description, String[] values} instead */ @@ -61,8 +60,8 @@ public final class TextInformationFrame extends Id3Frame { /* package */ TextInformationFrame(Parcel in) { super(castNonNull(in.readString())); description = in.readString(); - values = in.createStringArray(); - this.value = values[0]; + values = ImmutableList.copyOf(in.createStringArray()); + value = values.get(0); } @Override @@ -73,23 +72,23 @@ public final class TextInformationFrame extends Id3Frame { switch (id) { case "TT2": case "TIT2": - builder.setTitle(values[0]); + builder.setTitle(values.get(0)); break; case "TP1": case "TPE1": - builder.setArtist(String.join(MULTI_VALUE_DELIMITER, values)); + builder.setArtist(String.join(", ", values)); break; case "TP2": case "TPE2": - builder.setAlbumArtist(String.join(MULTI_VALUE_DELIMITER, values)); + builder.setAlbumArtist(String.join(", ", values)); break; case "TAL": case "TALB": - builder.setAlbumTitle(values[0]); + builder.setAlbumTitle(values.get(0)); break; case "TRK": case "TRCK": - String[] trackNumbers = Util.split(values[0], "/"); + String[] trackNumbers = Util.split(values.get(0), "/"); try { int trackNumber = Integer.parseInt(trackNumbers[0]); @Nullable @@ -103,7 +102,7 @@ public final class TextInformationFrame extends Id3Frame { case "TYE": case "TYER": try { - builder.setRecordingYear(Integer.parseInt(values[0])); + builder.setRecordingYear(Integer.parseInt(values.get(0))); } catch (NumberFormatException e) { // Do nothing, invalid input. } @@ -111,7 +110,7 @@ public final class TextInformationFrame extends Id3Frame { case "TDA": case "TDAT": try { - String date = values[0]; + String date = values.get(0); int month = Integer.parseInt(date.substring(2, 4)); int day = Integer.parseInt(date.substring(0, 2)); builder.setRecordingMonth(month).setRecordingDay(day); @@ -120,7 +119,7 @@ public final class TextInformationFrame extends Id3Frame { } break; case "TDRC": - List recordingDate = parseId3v2point4TimestampFrameForDate(values[0]); + List recordingDate = parseId3v2point4TimestampFrameForDate(values.get(0)); switch (recordingDate.size()) { case 3: builder.setRecordingDay(recordingDate.get(2)); @@ -138,7 +137,7 @@ public final class TextInformationFrame extends Id3Frame { } break; case "TDRL": - List releaseDate = parseId3v2point4TimestampFrameForDate(values[0]); + List releaseDate = parseId3v2point4TimestampFrameForDate(values.get(0)); switch (releaseDate.size()) { case 3: builder.setReleaseDay(releaseDate.get(2)); @@ -157,15 +156,15 @@ public final class TextInformationFrame extends Id3Frame { break; case "TCM": case "TCOM": - builder.setComposer(String.join(MULTI_VALUE_DELIMITER, values)); + builder.setComposer(String.join(", ", values)); break; case "TP3": case "TPE3": - builder.setConductor(String.join(MULTI_VALUE_DELIMITER, values)); + builder.setConductor(String.join(", ", values)); break; case "TXT": case "TEXT": - builder.setWriter(String.join(MULTI_VALUE_DELIMITER, values)); + builder.setWriter(String.join(", ", values)); break; default: break; @@ -183,7 +182,7 @@ public final class TextInformationFrame extends Id3Frame { TextInformationFrame other = (TextInformationFrame) obj; return Util.areEqual(id, other.id) && Util.areEqual(description, other.description) - && Arrays.equals(values, other.values); + && values.equals(other.values); } @Override @@ -191,13 +190,13 @@ public final class TextInformationFrame extends Id3Frame { int result = 17; result = 31 * result + id.hashCode(); result = 31 * result + (description != null ? description.hashCode() : 0); - result = 31 * result + Arrays.hashCode(values); + result = 31 * result + values.hashCode(); return result; } @Override public String toString() { - return id + ": description=" + description + ": value=" + String.join(MULTI_VALUE_DELIMITER, values); + return id + ": description=" + description + ": value=" + String.join(", ", values); } // Parcelable implementation. @@ -206,7 +205,7 @@ public final class TextInformationFrame extends Id3Frame { public void writeToParcel(Parcel dest, int flags) { dest.writeString(id); dest.writeString(description); - dest.writeStringArray(values); + dest.writeStringArray(values.toArray(new String[0])); } public static final Parcelable.Creator CREATOR = diff --git a/library/extractor/src/test/java/com/google/android/exoplayer2/metadata/id3/Id3DecoderTest.java b/library/extractor/src/test/java/com/google/android/exoplayer2/metadata/id3/Id3DecoderTest.java index 8226771abd..e83772ff4a 100644 --- a/library/extractor/src/test/java/com/google/android/exoplayer2/metadata/id3/Id3DecoderTest.java +++ b/library/extractor/src/test/java/com/google/android/exoplayer2/metadata/id3/Id3DecoderTest.java @@ -52,7 +52,7 @@ public final class Id3DecoderTest { TextInformationFrame textInformationFrame = (TextInformationFrame) metadata.get(0); assertThat(textInformationFrame.id).isEqualTo("TXXX"); assertThat(textInformationFrame.description).isEmpty(); - assertThat(textInformationFrame.values[0]).isEqualTo("mdialog_VINDICO1527664_start"); + assertThat(textInformationFrame.values.get(0)).isEqualTo("mdialog_VINDICO1527664_start"); // Test UTF-16. rawId3 = @@ -67,7 +67,8 @@ public final class Id3DecoderTest { textInformationFrame = (TextInformationFrame) metadata.get(0); assertThat(textInformationFrame.id).isEqualTo("TXXX"); assertThat(textInformationFrame.description).isEqualTo("Hello World"); - assertThat(textInformationFrame.values).isEmpty(); + assertThat(textInformationFrame.values.size()).isEqualTo(1); + assertThat(textInformationFrame.values.get(0)).isEqualTo(""); // Test multiple values. rawId3 = buildSingleFrameTag( @@ -80,9 +81,9 @@ public final class Id3DecoderTest { assertThat(metadata.length()).isEqualTo(1); textInformationFrame = (TextInformationFrame) metadata.get(0); assertThat(textInformationFrame.description).isEqualTo("Hello World"); - assertThat(textInformationFrame.values.length).isEqualTo(2); - assertThat(textInformationFrame.values[0]).isEqualTo("Foo"); - assertThat(textInformationFrame.values[1]).isEqualTo("Bar"); + assertThat(textInformationFrame.values.size()).isEqualTo(2); + assertThat(textInformationFrame.values.get(0)).isEqualTo("Foo"); + assertThat(textInformationFrame.values.get(1)).isEqualTo("Bar"); // Test empty. rawId3 = buildSingleFrameTag("TXXX", new byte[0]); @@ -96,7 +97,8 @@ public final class Id3DecoderTest { textInformationFrame = (TextInformationFrame) metadata.get(0); assertThat(textInformationFrame.id).isEqualTo("TXXX"); assertThat(textInformationFrame.description).isEmpty(); - assertThat(textInformationFrame.values).isEmpty(); + assertThat(textInformationFrame.values.size()).isEqualTo(1); + assertThat(textInformationFrame.values.get(0)).isEqualTo(""); } @Test @@ -110,17 +112,17 @@ public final class Id3DecoderTest { TextInformationFrame textInformationFrame = (TextInformationFrame) metadata.get(0); assertThat(textInformationFrame.id).isEqualTo("TIT2"); assertThat(textInformationFrame.description).isNull(); - assertThat(textInformationFrame.values.length).isEqualTo(1); - assertThat(textInformationFrame.values[0]).isEqualTo("Hello World"); + assertThat(textInformationFrame.values.size()).isEqualTo(1); + assertThat(textInformationFrame.values.get(0)).isEqualTo("Hello World"); // Test multiple values. rawId3 = buildSingleFrameTag("TIT2", new byte[] {3, 70, 111, 111, 0, 66, 97, 114, 0}); metadata = decoder.decode(rawId3, rawId3.length); assertThat(metadata.length()).isEqualTo(1); textInformationFrame = (TextInformationFrame) metadata.get(0); - assertThat(textInformationFrame.values.length).isEqualTo(2); - assertThat(textInformationFrame.values[0]).isEqualTo("Foo"); - assertThat(textInformationFrame.values[1]).isEqualTo("Bar"); + assertThat(textInformationFrame.values.size()).isEqualTo(2); + assertThat(textInformationFrame.values.get(0)).isEqualTo("Foo"); + assertThat(textInformationFrame.values.get(1)).isEqualTo("Bar"); // Test empty. rawId3 = buildSingleFrameTag("TIT2", new byte[0]); @@ -134,7 +136,8 @@ public final class Id3DecoderTest { textInformationFrame = (TextInformationFrame) metadata.get(0); assertThat(textInformationFrame.id).isEqualTo("TIT2"); assertThat(textInformationFrame.description).isNull(); - assertThat(textInformationFrame.values).isEmpty(); + assertThat(textInformationFrame.values.size()).isEqualTo(1); + assertThat(textInformationFrame.values.get(0)).isEqualTo(""); } @Test 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 00fdcb6087..e7a9ffc795 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 @@ -62,28 +62,28 @@ public class TextInformationFrameTest { List entries = ImmutableList.of( - new TextInformationFrame(/* id= */ "TT2", /* description= */ null, /* value= */ title), - new TextInformationFrame(/* id= */ "TP1", /* description= */ null, /* value= */ artist), + new TextInformationFrame(/* id= */ "TT2", /* description= */ null, /* value= */ new String[] { title }), + new TextInformationFrame(/* id= */ "TP1", /* description= */ null, /* value= */ new String[] { artist }), new TextInformationFrame( - /* id= */ "TAL", /* description= */ null, /* value= */ albumTitle), + /* id= */ "TAL", /* description= */ null, /* value= */ new String[] { albumTitle }), new TextInformationFrame( - /* id= */ "TP2", /* description= */ null, /* value= */ albumArtist), + /* id= */ "TP2", /* description= */ null, /* value= */ new String[] { albumArtist }), new TextInformationFrame( - /* id= */ "TRK", /* description= */ null, /* value= */ trackNumberInfo), + /* id= */ "TRK", /* description= */ null, /* value= */ new String[] { trackNumberInfo}), new TextInformationFrame( - /* id= */ "TYE", /* description= */ null, /* value= */ recordingYear), + /* id= */ "TYE", /* description= */ null, /* value= */ new String[] { recordingYear }), new TextInformationFrame( /* id= */ "TDA", /* description= */ null, - /* value= */ recordingDay + recordingMonth), + /* value= */ new String[] { recordingDay + recordingMonth }), new TextInformationFrame( - /* id= */ "TDRL", /* description= */ null, /* value= */ releaseDate), + /* id= */ "TDRL", /* description= */ null, /* value= */ new String[] { releaseDate }), new TextInformationFrame( - /* id= */ "TCM", /* description= */ null, /* value= */ composer), + /* id= */ "TCM", /* description= */ null, /* value= */ new String[] { composer }), new TextInformationFrame( - /* id= */ "TP3", /* description= */ null, /* value= */ conductor), + /* id= */ "TP3", /* description= */ null, /* value= */ new String[] { conductor }), new TextInformationFrame( - /* id= */ "TXT", /* description= */ null, /* value= */ writer)); + /* id= */ "TXT", /* description= */ null, /* value= */ new String[] { writer })); MediaMetadata.Builder builder = MediaMetadata.EMPTY.buildUpon(); for (Metadata.Entry entry : entries) { @@ -107,5 +107,21 @@ public class TextInformationFrameTest { assertThat(mediaMetadata.composer.toString()).isEqualTo(composer); assertThat(mediaMetadata.conductor.toString()).isEqualTo(conductor); assertThat(mediaMetadata.writer.toString()).isEqualTo(writer); + + // Test empty value array + entries = + ImmutableList.of( + new TextInformationFrame(/* id= */ "TT2", /* description= */ null, /* value= */ new String[0]) + ); + + builder = MediaMetadata.EMPTY.buildUpon(); + + for (Metadata.Entry entry : entries) { + entry.populateMediaMetadata(builder); + } + + mediaMetadata = builder.build(); + + assertThat(mediaMetadata.title.toString()).isEqualTo(""); } } From 0eb56f65b063501abf0f629e83a35004b39e8a60 Mon Sep 17 00:00:00 2001 From: Alexander Capehart Date: Mon, 21 Nov 2022 11:55:14 -0700 Subject: [PATCH 07/10] 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(); From 6dd20ed13a057a7ff0c7c0121e196485fda9f12a Mon Sep 17 00:00:00 2001 From: Alexander Capehart Date: Mon, 21 Nov 2022 12:51:47 -0700 Subject: [PATCH 08/10] Make text information frame values abstract Make the values of TextInformationFrame an abstract list instead of an explicit ImmutableList. --- .../android/exoplayer2/metadata/id3/TextInformationFrame.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 0658f1340a..70fc21e05d 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 @@ -37,7 +37,7 @@ public final class TextInformationFrame extends Id3Frame { public final String value; @NonNull - public final ImmutableList values; + public final List values; public TextInformationFrame(String id, @Nullable String description, @NonNull List values) { super(id); From 46f376e40b323a8f8066f0c1504adbef8f270dad Mon Sep 17 00:00:00 2001 From: Alexander Capehart Date: Mon, 21 Nov 2022 14:00:32 -0700 Subject: [PATCH 09/10] Fix non-updated constructor usage Fix a constructor usage that tried to use an array instead of a list. --- .../metadata/MetadataRendererTest.java | 2 +- .../metadata/id3/TextInformationFrameTest.java | 18 +++++++++--------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/library/core/src/test/java/com/google/android/exoplayer2/metadata/MetadataRendererTest.java b/library/core/src/test/java/com/google/android/exoplayer2/metadata/MetadataRendererTest.java index ae658ed9bc..823ed43626 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/metadata/MetadataRendererTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/metadata/MetadataRendererTest.java @@ -107,7 +107,7 @@ public class MetadataRendererTest { assertThat(metadata).hasSize(1); assertThat(metadata.get(0).length()).isEqualTo(1); TextInformationFrame expectedId3Frame = - new TextInformationFrame("TXXX", "Test description", new String[] { "Test value" }); + new TextInformationFrame("TXXX", "Test description", Collections.singletonList("Test value")); assertThat(metadata.get(0).get(0)).isEqualTo(expectedId3Frame); } 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 dfc1dadbaa..8c727bd51a 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 @@ -65,27 +65,27 @@ public class TextInformationFrameTest { ImmutableList.of( new TextInformationFrame(/* id= */ "TT2", /* description= */ null, /* value= */ Collections.singletonList(title)), - new TextInformationFrame(/* id= */ "TP1", /* description= */ null, /* values= */ Collections.singletonList( artist )), + new TextInformationFrame(/* id= */ "TP1", /* description= */ null, /* values= */ Collections.singletonList(artist)), new TextInformationFrame( - /* id= */ "TAL", /* description= */ null, /* values= */ Collections.singletonList( albumTitle )), + /* id= */ "TAL", /* description= */ null, /* values= */ Collections.singletonList(albumTitle)), new TextInformationFrame( - /* id= */ "TP2", /* description= */ null, /* values= */ Collections.singletonList( albumArtist )), + /* id= */ "TP2", /* description= */ null, /* values= */ Collections.singletonList(albumArtist)), new TextInformationFrame( - /* id= */ "TRK", /* description= */ null, /* values= */ Collections.singletonList( trackNumberInfo)), + /* id= */ "TRK", /* description= */ null, /* values= */ Collections.singletonList(trackNumberInfo)), new TextInformationFrame( - /* id= */ "TYE", /* description= */ null, /* values= */ Collections.singletonList( recordingYear )), + /* id= */ "TYE", /* description= */ null, /* values= */ Collections.singletonList(recordingYear)), new TextInformationFrame( /* id= */ "TDA", /* description= */ null, /* value= */ Collections.singletonList( recordingDay + recordingMonth )), new TextInformationFrame( - /* id= */ "TDRL", /* description= */ null, /* values= */ Collections.singletonList( releaseDate )), + /* id= */ "TDRL", /* description= */ null, /* values= */ Collections.singletonList(releaseDate)), new TextInformationFrame( - /* id= */ "TCM", /* description= */ null, /* values= */ Collections.singletonList( composer )), + /* id= */ "TCM", /* description= */ null, /* values= */ Collections.singletonList(composer)), new TextInformationFrame( - /* id= */ "TP3", /* description= */ null, /* values= */ Collections.singletonList( conductor )), + /* id= */ "TP3", /* description= */ null, /* values= */ Collections.singletonList(conductor)), new TextInformationFrame( - /* id= */ "TXT", /* description= */ null, /* values= */ Collections.singletonList( writer ))); + /* id= */ "TXT", /* description= */ null, /* values= */ Collections.singletonList(writer))); MediaMetadata.Builder builder = MediaMetadata.EMPTY.buildUpon(); for (Metadata.Entry entry : entries) { From 85c4d9870b77760e354b738781d5a8d0257cfee7 Mon Sep 17 00:00:00 2001 From: Alexander Capehart Date: Tue, 22 Nov 2022 09:28:05 -0700 Subject: [PATCH 10/10] Move ID3v2 text frame fallback to ID3v2 decoder Instead of the ID3v2 text frame falling back to a singleton list of an empty string when the given values are empty, make that case throw an exception within the text frame, and move that fallback behavior into the ID3v2 decoder. --- .../exoplayer2/metadata/id3/Id3Decoder.java | 11 ++++++----- .../metadata/id3/TextInformationFrame.java | 13 ++++++------- .../metadata/id3/TextInformationFrameTest.java | 16 ---------------- 3 files changed, 12 insertions(+), 28 deletions(-) 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 444befa1fc..15adda2f8a 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 @@ -489,12 +489,9 @@ public final class Id3Decoder extends SimpleMetadataDecoder { ArrayList values = new ArrayList<>(); if (index >= data.length) { - return Collections.emptyList(); + return Collections.singletonList(""); } - // 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. String charset = getCharsetName(encoding); int valueStartIndex = index; int valueEndIndex = indexOfTerminator(data, valueStartIndex, encoding); @@ -506,7 +503,11 @@ public final class Id3Decoder extends SimpleMetadataDecoder { valueEndIndex = indexOfTerminator(data, valueStartIndex, encoding); } - return values; + if (values.isEmpty()) { + return Collections.singletonList(""); + } else { + 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 70fc21e05d..8be0811d96 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 @@ -36,19 +36,18 @@ public final class TextInformationFrame extends Id3Frame { @Deprecated public final String value; + /** The text values of this frame. Will always have at least one element. */ @NonNull public final List values; public TextInformationFrame(String id, @Nullable String description, @NonNull List values) { super(id); - this.description = description; - - if( values.size() > 0 ) { - this.values = ImmutableList.copyOf(values); - } else { - this.values = ImmutableList.of(""); + if (values.isEmpty()) { + throw new IllegalArgumentException("Text information frames must have at least one value"); } - + + this.description = description; + this.values = ImmutableList.copyOf(values); this.value = this.values.get(0); } 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 8c727bd51a..712b6c0734 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 @@ -109,21 +109,5 @@ public class TextInformationFrameTest { assertThat(mediaMetadata.composer.toString()).isEqualTo(composer); assertThat(mediaMetadata.conductor.toString()).isEqualTo(conductor); assertThat(mediaMetadata.writer.toString()).isEqualTo(writer); - - // Test empty value array - entries = - Collections.singletonList( - new TextInformationFrame(/* id= */ "TT2", /* description= */ null, /* values= */ Collections.emptyList()) - ); - - builder = MediaMetadata.EMPTY.buildUpon(); - - for (Metadata.Entry entry : entries) { - entry.populateMediaMetadata(builder); - } - - mediaMetadata = builder.build(); - - assertThat(mediaMetadata.title.toString()).isEqualTo(""); } }