From 417c2426251b02f461785fb56b5d549b04a8fa5f Mon Sep 17 00:00:00 2001 From: tonihei Date: Thu, 2 Dec 2021 09:45:24 +0000 Subject: [PATCH] Prohibit duplicate TrackGroups in TrackGroupArray Allowing duplicate groups caused some other code working with the array to use reference equality comparison. This is error-prone, easily forgotten (e.g. when using the TrackGroups in a map) and causes bugs when TrackGroups are serialized to disk or to another process. All TrackGroups created by ExoPlayer are already unique and custom code creating TrackGroupArrays with identical groups can easily distringuish them by adding an id to each group. Issue: google/ExoPlayer#9718 PiperOrigin-RevId: 413617005 --- .../media3/common/TrackGroupArray.java | 50 ++++++++++++------- .../exoplayer/offline/DownloadHelper.java | 4 +- .../trackselection/MappingTrackSelector.java | 9 ++-- .../media3/exoplayer/util/EventLogger.java | 5 +- .../DefaultTrackSelectorTest.java | 24 +++++++-- .../MappingTrackSelectorTest.java | 25 ++++++---- 6 files changed, 70 insertions(+), 47 deletions(-) diff --git a/libraries/common/src/main/java/androidx/media3/common/TrackGroupArray.java b/libraries/common/src/main/java/androidx/media3/common/TrackGroupArray.java index 64fca47fe50..a9dc8242adf 100644 --- a/libraries/common/src/main/java/androidx/media3/common/TrackGroupArray.java +++ b/libraries/common/src/main/java/androidx/media3/common/TrackGroupArray.java @@ -19,34 +19,40 @@ import androidx.annotation.IntDef; import androidx.annotation.Nullable; import androidx.media3.common.util.BundleableUtil; +import androidx.media3.common.util.Log; import androidx.media3.common.util.UnstableApi; import com.google.common.collect.ImmutableList; -import com.google.common.collect.Lists; import java.lang.annotation.Documented; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; -import java.util.Arrays; import java.util.List; /** An immutable array of {@link TrackGroup}s. */ @UnstableApi public final class TrackGroupArray implements Bundleable { + private static final String TAG = "TrackGroupArray"; + /** The empty array. */ public static final TrackGroupArray EMPTY = new TrackGroupArray(); /** The number of groups in the array. Greater than or equal to zero. */ public final int length; - private final TrackGroup[] trackGroups; + private final ImmutableList trackGroups; // Lazily initialized hashcode. private int hashCode; - /** Construct a {@code TrackGroupArray} from an array of (possibly empty) trackGroups. */ + /** + * Construct a {@code TrackGroupArray} from an array of {@link TrackGroup TrackGroups}. + * + *

The groups must not contain duplicates. + */ public TrackGroupArray(TrackGroup... trackGroups) { - this.trackGroups = trackGroups; + this.trackGroups = ImmutableList.copyOf(trackGroups); this.length = trackGroups.length; + verifyCorrectness(); } /** @@ -56,7 +62,7 @@ public TrackGroupArray(TrackGroup... trackGroups) { * @return The group. */ public TrackGroup get(int index) { - return trackGroups[index]; + return trackGroups.get(index); } /** @@ -65,16 +71,9 @@ public TrackGroup get(int index) { * @param group The group. * @return The index of the group, or {@link C#INDEX_UNSET} if no such group exists. */ - @SuppressWarnings("ReferenceEquality") public int indexOf(TrackGroup group) { - for (int i = 0; i < length; i++) { - // Suppressed reference equality warning because this is looking for the index of a specific - // TrackGroup object, not the index of a potential equal TrackGroup. - if (trackGroups[i] == group) { - return i; - } - } - return C.INDEX_UNSET; + int index = trackGroups.indexOf(group); + return index >= 0 ? index : C.INDEX_UNSET; } /** Returns whether this track group array is empty. */ @@ -85,7 +84,7 @@ public boolean isEmpty() { @Override public int hashCode() { if (hashCode == 0) { - hashCode = Arrays.hashCode(trackGroups); + hashCode = trackGroups.hashCode(); } return hashCode; } @@ -99,7 +98,7 @@ public boolean equals(@Nullable Object obj) { return false; } TrackGroupArray other = (TrackGroupArray) obj; - return length == other.length && Arrays.equals(trackGroups, other.trackGroups); + return length == other.length && trackGroups.equals(other.trackGroups); } // Bundleable implementation. @@ -117,8 +116,7 @@ public boolean equals(@Nullable Object obj) { public Bundle toBundle() { Bundle bundle = new Bundle(); bundle.putParcelableArrayList( - keyForField(FIELD_TRACK_GROUPS), - BundleableUtil.toBundleArrayList(Lists.newArrayList(trackGroups))); + keyForField(FIELD_TRACK_GROUPS), BundleableUtil.toBundleArrayList(trackGroups)); return bundle; } @@ -133,6 +131,20 @@ public Bundle toBundle() { return new TrackGroupArray(trackGroups.toArray(new TrackGroup[0])); }; + private void verifyCorrectness() { + for (int i = 0; i < trackGroups.size(); i++) { + for (int j = i + 1; j < trackGroups.size(); j++) { + if (trackGroups.get(i).equals(trackGroups.get(j))) { + Log.e( + TAG, + "", + new IllegalArgumentException( + "Multiple identical TrackGroups added to one TrackGroupArray.")); + } + } + } + } + private static String keyForField(@FieldNumber int field) { return Integer.toString(field, Character.MAX_RADIX); } diff --git a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/offline/DownloadHelper.java b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/offline/DownloadHelper.java index 4c239079ce5..ff98e26cd28 100644 --- a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/offline/DownloadHelper.java +++ b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/offline/DownloadHelper.java @@ -835,8 +835,6 @@ private void assertPreparedWithMedia() { * Runs the track selection for a given period index with the current parameters. The selected * tracks will be added to {@link #trackSelectionsByPeriodAndRenderer}. */ - // Intentional reference comparison of track group instances. - @SuppressWarnings("ReferenceEquality") @RequiresNonNull({ "trackGroupArrays", "trackSelectionsByPeriodAndRenderer", @@ -861,7 +859,7 @@ private TrackSelectorResult runTrackSelection(int periodIndex) { boolean mergedWithExistingSelection = false; for (int j = 0; j < existingSelectionList.size(); j++) { ExoTrackSelection existingSelection = existingSelectionList.get(j); - if (existingSelection.getTrackGroup() == newSelection.getTrackGroup()) { + if (existingSelection.getTrackGroup().equals(newSelection.getTrackGroup())) { // Merge with existing selection. scratchSet.clear(); for (int k = 0; k < existingSelection.length(); k++) { diff --git a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/trackselection/MappingTrackSelector.java b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/trackselection/MappingTrackSelector.java index 7c62e88dca9..edb44977223 100644 --- a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/trackselection/MappingTrackSelector.java +++ b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/trackselection/MappingTrackSelector.java @@ -563,13 +563,10 @@ private static int[] getMixedMimeTypeAdaptationSupports( for (int trackIndex = 0; trackIndex < trackGroup.length; trackIndex++) { trackSupport[trackIndex] = mappedTrackInfo.getTrackSupport(rendererIndex, groupIndex, trackIndex); - // Suppressing reference equality warning because the track group stored in the track - // selection must point to the exact track group object to be considered part of it. - @SuppressWarnings("ReferenceEquality") boolean isTrackSelected = - (trackSelection != null) - && (trackSelection.getTrackGroup() == trackGroup) - && (trackSelection.indexOf(trackIndex) != C.INDEX_UNSET); + trackSelection != null + && trackSelection.getTrackGroup().equals(trackGroup) + && trackSelection.indexOf(trackIndex) != C.INDEX_UNSET; selected[trackIndex] = isTrackSelected; } @C.TrackType int trackGroupType = mappedTrackInfo.getRendererType(rendererIndex); diff --git a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/util/EventLogger.java b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/util/EventLogger.java index 455b37157c8..3b3d169364b 100644 --- a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/util/EventLogger.java +++ b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/util/EventLogger.java @@ -655,14 +655,11 @@ private static String getAdaptiveSupportString( } } - // Suppressing reference equality warning because the track group stored in the track selection - // must point to the exact track group object to be considered part of it. - @SuppressWarnings("ReferenceEquality") private static String getTrackStatusString( @Nullable TrackSelection selection, TrackGroup group, int trackIndex) { return getTrackStatusString( selection != null - && selection.getTrackGroup() == group + && selection.getTrackGroup().equals(group) && selection.indexOf(trackIndex) != C.INDEX_UNSET); } diff --git a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/trackselection/DefaultTrackSelectorTest.java b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/trackselection/DefaultTrackSelectorTest.java index d3edb5407f6..b64ba3fbcfd 100644 --- a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/trackselection/DefaultTrackSelectorTest.java +++ b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/trackselection/DefaultTrackSelectorTest.java @@ -250,6 +250,8 @@ public void selectTrack_withMixedEmptyAndNonEmptyTrackOverrides_appliesNonEmptyO @Test public void selectTracks_withEmptyTrackOverrideForDifferentTracks_hasNoEffect() throws ExoPlaybackException { + TrackGroup videoGroup0 = VIDEO_TRACK_GROUP.copyWithId("0"); + TrackGroup videoGroup1 = VIDEO_TRACK_GROUP.copyWithId("1"); trackSelector.setParameters( trackSelector .buildUponParameters() @@ -263,11 +265,16 @@ public void selectTracks_withEmptyTrackOverrideForDifferentTracks_hasNoEffect() TrackSelectorResult result = trackSelector.selectTracks( RENDERER_CAPABILITIES, - new TrackGroupArray(VIDEO_TRACK_GROUP, AUDIO_TRACK_GROUP, VIDEO_TRACK_GROUP), + new TrackGroupArray(videoGroup0, AUDIO_TRACK_GROUP, videoGroup1), periodId, TIMELINE); - assertThat(result.selections).asList().containsExactlyElementsIn(TRACK_SELECTIONS).inOrder(); + assertThat(result.selections) + .asList() + .containsExactly( + new FixedTrackSelection(videoGroup0, /* track= */ 0), + new FixedTrackSelection(AUDIO_TRACK_GROUP, /* track= */ 0)) + .inOrder(); assertThat(result.rendererConfigurations) .isEqualTo(new RendererConfiguration[] {DEFAULT, DEFAULT}); } @@ -366,17 +373,24 @@ public void selectTracks_withOverrideForUnmappedGroup_disablesAllRenderersOfSame /** Tests that an override is not applied for a different set of available track groups. */ @Test public void selectTracksWithNullOverrideForDifferentTracks() throws ExoPlaybackException { + TrackGroup videoGroup0 = VIDEO_TRACK_GROUP.copyWithId("0"); + TrackGroup videoGroup1 = VIDEO_TRACK_GROUP.copyWithId("1"); trackSelector.setParameters( trackSelector .buildUponParameters() - .setSelectionOverride(0, new TrackGroupArray(VIDEO_TRACK_GROUP), null)); + .setSelectionOverride(0, new TrackGroupArray(VIDEO_TRACK_GROUP.copyWithId("2")), null)); TrackSelectorResult result = trackSelector.selectTracks( RENDERER_CAPABILITIES, - new TrackGroupArray(VIDEO_TRACK_GROUP, AUDIO_TRACK_GROUP, VIDEO_TRACK_GROUP), + new TrackGroupArray(videoGroup0, AUDIO_TRACK_GROUP, videoGroup1), periodId, TIMELINE); - assertSelections(result, TRACK_SELECTIONS); + assertThat(result.selections) + .asList() + .containsExactly( + new FixedTrackSelection(videoGroup0, /* track= */ 0), + new FixedTrackSelection(AUDIO_TRACK_GROUP, /* track= */ 0)) + .inOrder(); assertThat(result.rendererConfigurations) .isEqualTo(new RendererConfiguration[] {DEFAULT, DEFAULT}); } diff --git a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/trackselection/MappingTrackSelectorTest.java b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/trackselection/MappingTrackSelectorTest.java index 795a7e32647..4885587d6bd 100644 --- a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/trackselection/MappingTrackSelectorTest.java +++ b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/trackselection/MappingTrackSelectorTest.java @@ -96,10 +96,13 @@ public void selectTracks_audioAndVideo_reverseOrderToRenderers_mappedToCorrectRe @Test public void selectTracks_multipleVideoAndAudioTracks_mappedToSameRenderer() throws ExoPlaybackException { + TrackGroup videoGroup0 = VIDEO_TRACK_GROUP.copyWithId("0"); + TrackGroup videoGroup1 = VIDEO_TRACK_GROUP.copyWithId("1"); + TrackGroup audioGroup0 = AUDIO_TRACK_GROUP.copyWithId("0"); + TrackGroup audioGroup1 = AUDIO_TRACK_GROUP.copyWithId("1"); FakeMappingTrackSelector trackSelector = new FakeMappingTrackSelector(); TrackGroupArray trackGroups = - new TrackGroupArray( - VIDEO_TRACK_GROUP, AUDIO_TRACK_GROUP, AUDIO_TRACK_GROUP, VIDEO_TRACK_GROUP); + new TrackGroupArray(videoGroup0, audioGroup0, audioGroup1, videoGroup1); RendererCapabilities[] rendererCapabilities = new RendererCapabilities[] { VIDEO_CAPABILITIES, AUDIO_CAPABILITIES, VIDEO_CAPABILITIES, AUDIO_CAPABILITIES @@ -107,16 +110,18 @@ public void selectTracks_multipleVideoAndAudioTracks_mappedToSameRenderer() trackSelector.selectTracks(rendererCapabilities, trackGroups, periodId, TIMELINE); - trackSelector.assertMappedTrackGroups(0, VIDEO_TRACK_GROUP, VIDEO_TRACK_GROUP); - trackSelector.assertMappedTrackGroups(1, AUDIO_TRACK_GROUP, AUDIO_TRACK_GROUP); + trackSelector.assertMappedTrackGroups(0, videoGroup0, videoGroup1); + trackSelector.assertMappedTrackGroups(1, audioGroup0, audioGroup1); } @Test public void selectTracks_multipleMetadataTracks_mappedToDifferentRenderers() throws ExoPlaybackException { + TrackGroup metadataGroup0 = METADATA_TRACK_GROUP.copyWithId("0"); + TrackGroup metadataGroup1 = METADATA_TRACK_GROUP.copyWithId("1"); FakeMappingTrackSelector trackSelector = new FakeMappingTrackSelector(); TrackGroupArray trackGroups = - new TrackGroupArray(VIDEO_TRACK_GROUP, METADATA_TRACK_GROUP, METADATA_TRACK_GROUP); + new TrackGroupArray(VIDEO_TRACK_GROUP, metadataGroup0, metadataGroup1); RendererCapabilities[] rendererCapabilities = new RendererCapabilities[] { VIDEO_CAPABILITIES, METADATA_CAPABILITIES, METADATA_CAPABILITIES @@ -125,8 +130,8 @@ public void selectTracks_multipleMetadataTracks_mappedToDifferentRenderers() trackSelector.selectTracks(rendererCapabilities, trackGroups, periodId, TIMELINE); trackSelector.assertMappedTrackGroups(0, VIDEO_TRACK_GROUP); - trackSelector.assertMappedTrackGroups(1, METADATA_TRACK_GROUP); - trackSelector.assertMappedTrackGroups(2, METADATA_TRACK_GROUP); + trackSelector.assertMappedTrackGroups(1, metadataGroup0); + trackSelector.assertMappedTrackGroups(2, metadataGroup1); } private static TrackGroup buildTrackGroup(String sampleMimeType) { @@ -141,10 +146,10 @@ public void buildTrackInfos_withTestValues_isAsExpected() { new int[] {C.TRACK_TYPE_AUDIO, C.TRACK_TYPE_VIDEO}, new TrackGroupArray[] { new TrackGroupArray( - new TrackGroup(new Format.Builder().build()), - new TrackGroup(new Format.Builder().build())), + new TrackGroup("0", new Format.Builder().build()), + new TrackGroup("1", new Format.Builder().build())), new TrackGroupArray( - new TrackGroup(new Format.Builder().build(), new Format.Builder().build())) + new TrackGroup("2", new Format.Builder().build(), new Format.Builder().build())) }, new int[] { RendererCapabilities.ADAPTIVE_SEAMLESS, RendererCapabilities.ADAPTIVE_NOT_SUPPORTED