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
This commit is contained in:
tonihei 2021-12-02 09:45:24 +00:00
parent e077edded5
commit 417c242625
6 changed files with 70 additions and 47 deletions

View File

@ -19,34 +19,40 @@ import android.os.Bundle;
import androidx.annotation.IntDef; import androidx.annotation.IntDef;
import androidx.annotation.Nullable; import androidx.annotation.Nullable;
import androidx.media3.common.util.BundleableUtil; import androidx.media3.common.util.BundleableUtil;
import androidx.media3.common.util.Log;
import androidx.media3.common.util.UnstableApi; import androidx.media3.common.util.UnstableApi;
import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
import java.lang.annotation.Documented; import java.lang.annotation.Documented;
import java.lang.annotation.Retention; import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy; import java.lang.annotation.RetentionPolicy;
import java.util.Arrays;
import java.util.List; import java.util.List;
/** An immutable array of {@link TrackGroup}s. */ /** An immutable array of {@link TrackGroup}s. */
@UnstableApi @UnstableApi
public final class TrackGroupArray implements Bundleable { public final class TrackGroupArray implements Bundleable {
private static final String TAG = "TrackGroupArray";
/** The empty array. */ /** The empty array. */
public static final TrackGroupArray EMPTY = new TrackGroupArray(); public static final TrackGroupArray EMPTY = new TrackGroupArray();
/** The number of groups in the array. Greater than or equal to zero. */ /** The number of groups in the array. Greater than or equal to zero. */
public final int length; public final int length;
private final TrackGroup[] trackGroups; private final ImmutableList<TrackGroup> trackGroups;
// Lazily initialized hashcode. // Lazily initialized hashcode.
private int 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) { public TrackGroupArray(TrackGroup... trackGroups) {
this.trackGroups = trackGroups; this.trackGroups = ImmutableList.copyOf(trackGroups);
this.length = trackGroups.length; this.length = trackGroups.length;
verifyCorrectness();
} }
/** /**
@ -56,7 +62,7 @@ public final class TrackGroupArray implements Bundleable {
* @return The group. * @return The group.
*/ */
public TrackGroup get(int index) { public TrackGroup get(int index) {
return trackGroups[index]; return trackGroups.get(index);
} }
/** /**
@ -65,16 +71,9 @@ public final class TrackGroupArray implements Bundleable {
* @param group The group. * @param group The group.
* @return The index of the group, or {@link C#INDEX_UNSET} if no such group exists. * @return The index of the group, or {@link C#INDEX_UNSET} if no such group exists.
*/ */
@SuppressWarnings("ReferenceEquality")
public int indexOf(TrackGroup group) { public int indexOf(TrackGroup group) {
for (int i = 0; i < length; i++) { int index = trackGroups.indexOf(group);
// Suppressed reference equality warning because this is looking for the index of a specific return index >= 0 ? index : C.INDEX_UNSET;
// TrackGroup object, not the index of a potential equal TrackGroup.
if (trackGroups[i] == group) {
return i;
}
}
return C.INDEX_UNSET;
} }
/** Returns whether this track group array is empty. */ /** Returns whether this track group array is empty. */
@ -85,7 +84,7 @@ public final class TrackGroupArray implements Bundleable {
@Override @Override
public int hashCode() { public int hashCode() {
if (hashCode == 0) { if (hashCode == 0) {
hashCode = Arrays.hashCode(trackGroups); hashCode = trackGroups.hashCode();
} }
return hashCode; return hashCode;
} }
@ -99,7 +98,7 @@ public final class TrackGroupArray implements Bundleable {
return false; return false;
} }
TrackGroupArray other = (TrackGroupArray) obj; TrackGroupArray other = (TrackGroupArray) obj;
return length == other.length && Arrays.equals(trackGroups, other.trackGroups); return length == other.length && trackGroups.equals(other.trackGroups);
} }
// Bundleable implementation. // Bundleable implementation.
@ -117,8 +116,7 @@ public final class TrackGroupArray implements Bundleable {
public Bundle toBundle() { public Bundle toBundle() {
Bundle bundle = new Bundle(); Bundle bundle = new Bundle();
bundle.putParcelableArrayList( bundle.putParcelableArrayList(
keyForField(FIELD_TRACK_GROUPS), keyForField(FIELD_TRACK_GROUPS), BundleableUtil.toBundleArrayList(trackGroups));
BundleableUtil.toBundleArrayList(Lists.newArrayList(trackGroups)));
return bundle; return bundle;
} }
@ -133,6 +131,20 @@ public final class TrackGroupArray implements Bundleable {
return new TrackGroupArray(trackGroups.toArray(new TrackGroup[0])); 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) { private static String keyForField(@FieldNumber int field) {
return Integer.toString(field, Character.MAX_RADIX); return Integer.toString(field, Character.MAX_RADIX);
} }

View File

@ -835,8 +835,6 @@ public final class DownloadHelper {
* Runs the track selection for a given period index with the current parameters. The selected * Runs the track selection for a given period index with the current parameters. The selected
* tracks will be added to {@link #trackSelectionsByPeriodAndRenderer}. * tracks will be added to {@link #trackSelectionsByPeriodAndRenderer}.
*/ */
// Intentional reference comparison of track group instances.
@SuppressWarnings("ReferenceEquality")
@RequiresNonNull({ @RequiresNonNull({
"trackGroupArrays", "trackGroupArrays",
"trackSelectionsByPeriodAndRenderer", "trackSelectionsByPeriodAndRenderer",
@ -861,7 +859,7 @@ public final class DownloadHelper {
boolean mergedWithExistingSelection = false; boolean mergedWithExistingSelection = false;
for (int j = 0; j < existingSelectionList.size(); j++) { for (int j = 0; j < existingSelectionList.size(); j++) {
ExoTrackSelection existingSelection = existingSelectionList.get(j); ExoTrackSelection existingSelection = existingSelectionList.get(j);
if (existingSelection.getTrackGroup() == newSelection.getTrackGroup()) { if (existingSelection.getTrackGroup().equals(newSelection.getTrackGroup())) {
// Merge with existing selection. // Merge with existing selection.
scratchSet.clear(); scratchSet.clear();
for (int k = 0; k < existingSelection.length(); k++) { for (int k = 0; k < existingSelection.length(); k++) {

View File

@ -563,13 +563,10 @@ public abstract class MappingTrackSelector extends TrackSelector {
for (int trackIndex = 0; trackIndex < trackGroup.length; trackIndex++) { for (int trackIndex = 0; trackIndex < trackGroup.length; trackIndex++) {
trackSupport[trackIndex] = trackSupport[trackIndex] =
mappedTrackInfo.getTrackSupport(rendererIndex, groupIndex, 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 = boolean isTrackSelected =
(trackSelection != null) trackSelection != null
&& (trackSelection.getTrackGroup() == trackGroup) && trackSelection.getTrackGroup().equals(trackGroup)
&& (trackSelection.indexOf(trackIndex) != C.INDEX_UNSET); && trackSelection.indexOf(trackIndex) != C.INDEX_UNSET;
selected[trackIndex] = isTrackSelected; selected[trackIndex] = isTrackSelected;
} }
@C.TrackType int trackGroupType = mappedTrackInfo.getRendererType(rendererIndex); @C.TrackType int trackGroupType = mappedTrackInfo.getRendererType(rendererIndex);

View File

@ -655,14 +655,11 @@ public class EventLogger implements AnalyticsListener {
} }
} }
// 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( private static String getTrackStatusString(
@Nullable TrackSelection selection, TrackGroup group, int trackIndex) { @Nullable TrackSelection selection, TrackGroup group, int trackIndex) {
return getTrackStatusString( return getTrackStatusString(
selection != null selection != null
&& selection.getTrackGroup() == group && selection.getTrackGroup().equals(group)
&& selection.indexOf(trackIndex) != C.INDEX_UNSET); && selection.indexOf(trackIndex) != C.INDEX_UNSET);
} }

View File

@ -250,6 +250,8 @@ public final class DefaultTrackSelectorTest {
@Test @Test
public void selectTracks_withEmptyTrackOverrideForDifferentTracks_hasNoEffect() public void selectTracks_withEmptyTrackOverrideForDifferentTracks_hasNoEffect()
throws ExoPlaybackException { throws ExoPlaybackException {
TrackGroup videoGroup0 = VIDEO_TRACK_GROUP.copyWithId("0");
TrackGroup videoGroup1 = VIDEO_TRACK_GROUP.copyWithId("1");
trackSelector.setParameters( trackSelector.setParameters(
trackSelector trackSelector
.buildUponParameters() .buildUponParameters()
@ -263,11 +265,16 @@ public final class DefaultTrackSelectorTest {
TrackSelectorResult result = TrackSelectorResult result =
trackSelector.selectTracks( trackSelector.selectTracks(
RENDERER_CAPABILITIES, RENDERER_CAPABILITIES,
new TrackGroupArray(VIDEO_TRACK_GROUP, AUDIO_TRACK_GROUP, VIDEO_TRACK_GROUP), new TrackGroupArray(videoGroup0, AUDIO_TRACK_GROUP, videoGroup1),
periodId, periodId,
TIMELINE); 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) assertThat(result.rendererConfigurations)
.isEqualTo(new RendererConfiguration[] {DEFAULT, DEFAULT}); .isEqualTo(new RendererConfiguration[] {DEFAULT, DEFAULT});
} }
@ -366,17 +373,24 @@ public final class DefaultTrackSelectorTest {
/** Tests that an override is not applied for a different set of available track groups. */ /** Tests that an override is not applied for a different set of available track groups. */
@Test @Test
public void selectTracksWithNullOverrideForDifferentTracks() throws ExoPlaybackException { public void selectTracksWithNullOverrideForDifferentTracks() throws ExoPlaybackException {
TrackGroup videoGroup0 = VIDEO_TRACK_GROUP.copyWithId("0");
TrackGroup videoGroup1 = VIDEO_TRACK_GROUP.copyWithId("1");
trackSelector.setParameters( trackSelector.setParameters(
trackSelector trackSelector
.buildUponParameters() .buildUponParameters()
.setSelectionOverride(0, new TrackGroupArray(VIDEO_TRACK_GROUP), null)); .setSelectionOverride(0, new TrackGroupArray(VIDEO_TRACK_GROUP.copyWithId("2")), null));
TrackSelectorResult result = TrackSelectorResult result =
trackSelector.selectTracks( trackSelector.selectTracks(
RENDERER_CAPABILITIES, RENDERER_CAPABILITIES,
new TrackGroupArray(VIDEO_TRACK_GROUP, AUDIO_TRACK_GROUP, VIDEO_TRACK_GROUP), new TrackGroupArray(videoGroup0, AUDIO_TRACK_GROUP, videoGroup1),
periodId, periodId,
TIMELINE); 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) assertThat(result.rendererConfigurations)
.isEqualTo(new RendererConfiguration[] {DEFAULT, DEFAULT}); .isEqualTo(new RendererConfiguration[] {DEFAULT, DEFAULT});
} }

View File

@ -96,10 +96,13 @@ public final class MappingTrackSelectorTest {
@Test @Test
public void selectTracks_multipleVideoAndAudioTracks_mappedToSameRenderer() public void selectTracks_multipleVideoAndAudioTracks_mappedToSameRenderer()
throws ExoPlaybackException { 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(); FakeMappingTrackSelector trackSelector = new FakeMappingTrackSelector();
TrackGroupArray trackGroups = TrackGroupArray trackGroups =
new TrackGroupArray( new TrackGroupArray(videoGroup0, audioGroup0, audioGroup1, videoGroup1);
VIDEO_TRACK_GROUP, AUDIO_TRACK_GROUP, AUDIO_TRACK_GROUP, VIDEO_TRACK_GROUP);
RendererCapabilities[] rendererCapabilities = RendererCapabilities[] rendererCapabilities =
new RendererCapabilities[] { new RendererCapabilities[] {
VIDEO_CAPABILITIES, AUDIO_CAPABILITIES, VIDEO_CAPABILITIES, AUDIO_CAPABILITIES VIDEO_CAPABILITIES, AUDIO_CAPABILITIES, VIDEO_CAPABILITIES, AUDIO_CAPABILITIES
@ -107,16 +110,18 @@ public final class MappingTrackSelectorTest {
trackSelector.selectTracks(rendererCapabilities, trackGroups, periodId, TIMELINE); trackSelector.selectTracks(rendererCapabilities, trackGroups, periodId, TIMELINE);
trackSelector.assertMappedTrackGroups(0, VIDEO_TRACK_GROUP, VIDEO_TRACK_GROUP); trackSelector.assertMappedTrackGroups(0, videoGroup0, videoGroup1);
trackSelector.assertMappedTrackGroups(1, AUDIO_TRACK_GROUP, AUDIO_TRACK_GROUP); trackSelector.assertMappedTrackGroups(1, audioGroup0, audioGroup1);
} }
@Test @Test
public void selectTracks_multipleMetadataTracks_mappedToDifferentRenderers() public void selectTracks_multipleMetadataTracks_mappedToDifferentRenderers()
throws ExoPlaybackException { throws ExoPlaybackException {
TrackGroup metadataGroup0 = METADATA_TRACK_GROUP.copyWithId("0");
TrackGroup metadataGroup1 = METADATA_TRACK_GROUP.copyWithId("1");
FakeMappingTrackSelector trackSelector = new FakeMappingTrackSelector(); FakeMappingTrackSelector trackSelector = new FakeMappingTrackSelector();
TrackGroupArray trackGroups = TrackGroupArray trackGroups =
new TrackGroupArray(VIDEO_TRACK_GROUP, METADATA_TRACK_GROUP, METADATA_TRACK_GROUP); new TrackGroupArray(VIDEO_TRACK_GROUP, metadataGroup0, metadataGroup1);
RendererCapabilities[] rendererCapabilities = RendererCapabilities[] rendererCapabilities =
new RendererCapabilities[] { new RendererCapabilities[] {
VIDEO_CAPABILITIES, METADATA_CAPABILITIES, METADATA_CAPABILITIES VIDEO_CAPABILITIES, METADATA_CAPABILITIES, METADATA_CAPABILITIES
@ -125,8 +130,8 @@ public final class MappingTrackSelectorTest {
trackSelector.selectTracks(rendererCapabilities, trackGroups, periodId, TIMELINE); trackSelector.selectTracks(rendererCapabilities, trackGroups, periodId, TIMELINE);
trackSelector.assertMappedTrackGroups(0, VIDEO_TRACK_GROUP); trackSelector.assertMappedTrackGroups(0, VIDEO_TRACK_GROUP);
trackSelector.assertMappedTrackGroups(1, METADATA_TRACK_GROUP); trackSelector.assertMappedTrackGroups(1, metadataGroup0);
trackSelector.assertMappedTrackGroups(2, METADATA_TRACK_GROUP); trackSelector.assertMappedTrackGroups(2, metadataGroup1);
} }
private static TrackGroup buildTrackGroup(String sampleMimeType) { private static TrackGroup buildTrackGroup(String sampleMimeType) {
@ -141,10 +146,10 @@ public final class MappingTrackSelectorTest {
new int[] {C.TRACK_TYPE_AUDIO, C.TRACK_TYPE_VIDEO}, new int[] {C.TRACK_TYPE_AUDIO, C.TRACK_TYPE_VIDEO},
new TrackGroupArray[] { new TrackGroupArray[] {
new TrackGroupArray( new TrackGroupArray(
new TrackGroup(new Format.Builder().build()), new TrackGroup("0", new Format.Builder().build()),
new TrackGroup(new Format.Builder().build())), new TrackGroup("1", new Format.Builder().build())),
new TrackGroupArray( 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[] { new int[] {
RendererCapabilities.ADAPTIVE_SEAMLESS, RendererCapabilities.ADAPTIVE_NOT_SUPPORTED RendererCapabilities.ADAPTIVE_SEAMLESS, RendererCapabilities.ADAPTIVE_NOT_SUPPORTED