From 157f1f0787f44def613b585d33cbb55641f5753d Mon Sep 17 00:00:00 2001 From: Alexander Capehart Date: Mon, 21 Nov 2022 11:33:48 -0700 Subject: [PATCH] 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(""); } }