Skip to content

Commit

Permalink
Prohibit duplicate TrackGroups in TrackGroupArray
Browse files Browse the repository at this point in the history 
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: #9718
PiperOrigin-RevId: 413617005
  • Loading branch information
@tonihei
tonihei committed Dec 2, 2021
1 parent 7e82225 commit 8c90ba5
Show file tree
Hide file tree
Showing 7 changed files with 75 additions and 47 deletions.
5 changes: 5 additions & 0 deletions RELEASENOTES.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,11 @@
track matching system Locale language) over technical track selection
constraints (for example, preferred MIME type, or maximum channel
count).
* Prohibit duplicate `TrackGroup`s in a `TrackGroupArray`. `TrackGroup`s
can always be made distinguishable by setting an `id` in the
`TrackGroup` constructor. This fixes a crash when resuming playback
after backgrounding the app with an active track override
((#9718)[https://github.com/google/ExoPlayer/issues/9718]).
* Extractors:
* Fix inconsistency with spec in H.265 SPS nal units parsing
((#9719)[https://github.com/google/ExoPlayer/issues/9719]).
Expand Down
50 changes: 31 additions & 19 deletions library/common/src/main/java/com/google/android/exoplayer2/source/TrackGroupArray.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,32 +21,38 @@
import com.google.android.exoplayer2.Bundleable;
import com.google.android.exoplayer2.C;
import com.google.android.exoplayer2.util.BundleableUtil;
import com.google.android.exoplayer2.util.Log;
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. */
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<TrackGroup> 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}.
*
* <p>The groups must not contain duplicates.
*/
public TrackGroupArray(TrackGroup... trackGroups) {
this.trackGroups = trackGroups;
this.trackGroups = ImmutableList.copyOf(trackGroups);
this.length = trackGroups.length;
verifyCorrectness();
}

/**
Expand All @@ -56,7 +62,7 @@ public TrackGroupArray(TrackGroup... trackGroups) {
* @return The group.
*/
public TrackGroup get(int index) {
return trackGroups[index];
return trackGroups.get(index);
}

/**
Expand All @@ -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. */
Expand All @@ -85,7 +84,7 @@ public boolean isEmpty() {
@Override
public int hashCode() {
if (hashCode == 0) {
hashCode = Arrays.hashCode(trackGroups);
hashCode = trackGroups.hashCode();
}
return hashCode;
}
Expand All @@ -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.
Expand All @@ -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;
}

Expand All @@ -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++) {

This comment has been minimized.

Sign in to view

Copy link
@eneim

eneim Feb 21, 2022 •
edited
Loading

Contributor

@tonihei Sorry this commit was merged, I just wonder if this check can be optimized by using a Set to wrap the trackGroups and then compare the size instead? Currently it runs in O(n^2). Just a comment because the size of the TrackGroup and the frequency of this method being used may not be significant.
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);
}
Expand Down
4 changes: 1 addition & 3 deletions library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -832,8 +832,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",
Expand All @@ -858,7 +856,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++) {
Expand Down
9 changes: 3 additions & 6 deletions ...core/src/main/java/com/google/android/exoplayer2/trackselection/MappingTrackSelector.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -560,13 +560,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);
Expand Down
5 changes: 1 addition & 4 deletions library/core/src/main/java/com/google/android/exoplayer2/util/EventLogger.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -651,14 +651,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);
}

Expand Down
24 changes: 19 additions & 5 deletions .../src/test/java/com/google/android/exoplayer2/trackselection/DefaultTrackSelectorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,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()
Expand All @@ -261,11 +263,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});
}
Expand Down Expand Up @@ -364,17 +371,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});
}
Expand Down
25 changes: 15 additions & 10 deletions .../src/test/java/com/google/android/exoplayer2/trackselection/MappingTrackSelectorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,27 +95,32 @@ 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
};

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
Expand All @@ -124,8 +129,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) {
Expand All @@ -140,10 +145,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
Expand Down

0 comments on commit 8c90ba5

Please sign in to comment.