Skip to content

Commit

Permalink
Clarify behavior for out-of-bounds indices and align implementations
Browse files Browse the repository at this point in the history 
Some Player methods operate relative to existing indices in the
playlist (add,remove,move,seek). As these operations may be issued
from a place with a stale playlist (e.g. a controller that sends
a command while the playlist is changing), we have to handle out-
of-bounds indices gracefully. In most cases this is already
documented and implemented correctly. However, some cases are not
documented and the existing player implementations don't handle
these cases consistently (or in some cases not even correctly).

PiperOrigin-RevId: 495856295
  • Loading branch information
@tonihei @tianyif
tonihei authored and tianyif committed Dec 21, 2022
1 parent 9b82d53 commit a1c0b10
Show file tree
Hide file tree
Showing 4 changed files with 145 additions and 66 deletions.
27 changes: 14 additions & 13 deletions extensions/cast/src/main/java/com/google/android/exoplayer2/ext/cast/CastPlayer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@
import com.google.android.exoplayer2.source.TrackGroup;
import com.google.android.exoplayer2.text.CueGroup;
import com.google.android.exoplayer2.trackselection.TrackSelectionParameters;
import com.google.android.exoplayer2.util.Assertions;
import com.google.android.exoplayer2.util.Clock;
import com.google.android.exoplayer2.util.ListenerSet;
import com.google.android.exoplayer2.util.Log;
Expand Down Expand Up @@ -294,7 +293,7 @@ public void setMediaItems(List<MediaItem> mediaItems, int startIndex, long start

@Override
public void addMediaItems(int index, List<MediaItem> mediaItems) {
Assertions.checkArgument(index >= 0);
checkArgument(index >= 0);
int uid = MediaQueueItem.INVALID_ITEM_ID;
if (index < currentTimeline.getWindowCount()) {
uid = (int) currentTimeline.getWindow(/* windowIndex= */ index, window).uid;
Expand All @@ -304,14 +303,11 @@ public void addMediaItems(int index, List<MediaItem> mediaItems) {

@Override
public void moveMediaItems(int fromIndex, int toIndex, int newIndex) {
Assertions.checkArgument(
fromIndex >= 0
&& fromIndex <= toIndex
&& toIndex <= currentTimeline.getWindowCount()
&& newIndex >= 0
&& newIndex < currentTimeline.getWindowCount());
newIndex = min(newIndex, currentTimeline.getWindowCount() - (toIndex - fromIndex));
if (fromIndex == toIndex || fromIndex == newIndex) {
checkArgument(fromIndex >= 0 && fromIndex <= toIndex && newIndex >= 0);
int playlistSize = currentTimeline.getWindowCount();
toIndex = min(toIndex, playlistSize);
newIndex = min(newIndex, playlistSize - (toIndex - fromIndex));
if (fromIndex >= playlistSize || fromIndex == toIndex || fromIndex == newIndex) {
// Do nothing.
return;
}
Expand All @@ -324,9 +320,10 @@ public void moveMediaItems(int fromIndex, int toIndex, int newIndex) {

@Override
public void removeMediaItems(int fromIndex, int toIndex) {
Assertions.checkArgument(fromIndex >= 0 && toIndex >= fromIndex);
toIndex = min(toIndex, currentTimeline.getWindowCount());
if (fromIndex == toIndex) {
checkArgument(fromIndex >= 0 && toIndex >= fromIndex);
int playlistSize = currentTimeline.getWindowCount();
toIndex = min(toIndex, playlistSize);
if (fromIndex >= playlistSize || fromIndex == toIndex) {
// Do nothing.
return;
}
Expand Down Expand Up @@ -404,6 +401,10 @@ public void seekTo(
long positionMs,
@Player.Command int seekCommand,
boolean isRepeatingCurrentItem) {
checkArgument(mediaItemIndex >= 0);
if (!currentTimeline.isEmpty() && mediaItemIndex >= currentTimeline.getWindowCount()) {
return;
}
MediaStatus mediaStatus = getMediaStatus();
// We assume the default position is 0. There is no support for seeking to the default position
// in RemoteMediaClient.
Expand Down
27 changes: 15 additions & 12 deletions library/common/src/main/java/com/google/android/exoplayer2/Player.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1669,7 +1669,8 @@ default void onMetadata(Metadata metadata) {}
/**
* Moves the media item at the current index to the new index.
*
* @param currentIndex The current index of the media item to move.
* @param currentIndex The current index of the media item to move. If the index is larger than
* the size of the playlist, the request is ignored.
* @param newIndex The new index of the media item. If the new index is larger than the size of
* the playlist the item is moved to the end of the playlist.
*/
Expand All @@ -1678,8 +1679,10 @@ default void onMetadata(Metadata metadata) {}
/**
* Moves the media item range to the new index.
*
* @param fromIndex The start of the range to move.
* @param toIndex The first item not to be included in the range (exclusive).
* @param fromIndex The start of the range to move. If the index is larger than the size of the
* playlist, the request is ignored.
* @param toIndex The first item not to be included in the range (exclusive). If the index is
* larger than the size of the playlist, items up to the end of the playlist are moved.
* @param newIndex The new index of the first media item of the range. If the new index is larger
* than the size of the remaining playlist after removing the range, the range is moved to the
* end of the playlist.
Expand All @@ -1689,16 +1692,18 @@ default void onMetadata(Metadata metadata) {}
/**
* Removes the media item at the given index of the playlist.
*
* @param index The index at which to remove the media item.
* @param index The index at which to remove the media item. If the index is larger than the size
* of the playlist, the request is ignored.
*/
void removeMediaItem(int index);

/**
* Removes a range of media items from the playlist.
*
* @param fromIndex The index at which to start removing media items.
* @param fromIndex The index at which to start removing media items. If the index is larger than
* the size of the playlist, the request is ignored.
* @param toIndex The index of the first item to be kept (exclusive). If the index is larger than
* the size of the playlist, media items to the end of the playlist are removed.
* the size of the playlist, media items up to the end of the playlist are removed.
*/
void removeMediaItems(int fromIndex, int toIndex);

Expand Down Expand Up @@ -1879,9 +1884,8 @@ default void onMetadata(Metadata metadata) {}
* For other streams it will typically be the start.
*
* @param mediaItemIndex The index of the {@link MediaItem} whose associated default position
* should be seeked to.
* @throws IllegalSeekPositionException If the player has a non-empty timeline and the provided
* {@code mediaItemIndex} is not within the bounds of the current timeline.
* should be seeked to. If the index is larger than the size of the playlist, the request is
* ignored.
*/
void seekToDefaultPosition(int mediaItemIndex);

Expand All @@ -1896,11 +1900,10 @@ default void onMetadata(Metadata metadata) {}
/**
* Seeks to a position specified in milliseconds in the specified {@link MediaItem}.
*
* @param mediaItemIndex The index of the {@link MediaItem}.
* @param mediaItemIndex The index of the {@link MediaItem}. If the index is larger than the size
* of the playlist, the request is ignored.
* @param positionMs The seek position in the specified {@link MediaItem}, or {@link C#TIME_UNSET}
* to seek to the media item's default position.
* @throws IllegalSeekPositionException If the player has a non-empty timeline and the provided
* {@code mediaItemIndex} is not within the bounds of the current timeline.
*/
void seekTo(int mediaItemIndex, long positionMs);

Expand Down
38 changes: 22 additions & 16 deletions library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import static com.google.android.exoplayer2.Renderer.MSG_SET_VIDEO_OUTPUT;
import static com.google.android.exoplayer2.Renderer.MSG_SET_VIDEO_OUTPUT_RESOLUTION;
import static com.google.android.exoplayer2.Renderer.MSG_SET_VOLUME;
import static com.google.android.exoplayer2.util.Assertions.checkArgument;
import static com.google.android.exoplayer2.util.Assertions.checkNotNull;
import static com.google.android.exoplayer2.util.Assertions.checkState;
import static com.google.android.exoplayer2.util.Util.castNonNull;
Expand Down Expand Up @@ -83,7 +84,6 @@
import com.google.android.exoplayer2.trackselection.TrackSelector;
import com.google.android.exoplayer2.trackselection.TrackSelectorResult;
import com.google.android.exoplayer2.upstream.BandwidthMeter;
import com.google.android.exoplayer2.util.Assertions;
import com.google.android.exoplayer2.util.Clock;
import com.google.android.exoplayer2.util.ConditionVariable;
import com.google.android.exoplayer2.util.HandlerWrapper;
Expand Down Expand Up @@ -613,7 +613,6 @@ public void setMediaSources(
@Override
public void addMediaItems(int index, List<MediaItem> mediaItems) {
verifyApplicationThread();
index = min(index, mediaSourceHolderSnapshots.size());
addMediaSources(index, createMediaSources(mediaItems));
}

Expand All @@ -638,7 +637,8 @@ public void addMediaSources(List<MediaSource> mediaSources) {
@Override
public void addMediaSources(int index, List<MediaSource> mediaSources) {
verifyApplicationThread();
Assertions.checkArgument(index >= 0);
checkArgument(index >= 0);
index = min(index, mediaSourceHolderSnapshots.size());
Timeline oldTimeline = getCurrentTimeline();
pendingOperationAcks++;
List<MediaSourceList.MediaSourceHolder> holders = addMediaSourceHolders(index, mediaSources);
Expand All @@ -664,7 +664,13 @@ public void addMediaSources(int index, List<MediaSource> mediaSources) {
@Override
public void removeMediaItems(int fromIndex, int toIndex) {
verifyApplicationThread();
toIndex = min(toIndex, mediaSourceHolderSnapshots.size());
checkArgument(fromIndex >= 0 && toIndex >= fromIndex);
int playlistSize = mediaSourceHolderSnapshots.size();
toIndex = min(toIndex, playlistSize);
if (fromIndex >= playlistSize || fromIndex == toIndex) {
// Do nothing.
return;
}
PlaybackInfo newPlaybackInfo = removeMediaItemsInternal(fromIndex, toIndex);
boolean positionDiscontinuity =
!newPlaybackInfo.periodId.periodUid.equals(playbackInfo.periodId.periodUid);
Expand All @@ -683,14 +689,16 @@ public void removeMediaItems(int fromIndex, int toIndex) {
@Override
public void moveMediaItems(int fromIndex, int toIndex, int newFromIndex) {
verifyApplicationThread();
Assertions.checkArgument(
fromIndex >= 0
&& fromIndex <= toIndex
&& toIndex <= mediaSourceHolderSnapshots.size()
&& newFromIndex >= 0);
checkArgument(fromIndex >= 0 && fromIndex <= toIndex && newFromIndex >= 0);
int playlistSize = mediaSourceHolderSnapshots.size();
toIndex = min(toIndex, playlistSize);
newFromIndex = min(newFromIndex, playlistSize - (toIndex - fromIndex));
if (fromIndex >= playlistSize || fromIndex == toIndex || fromIndex == newFromIndex) {
// Do nothing.
return;
}
Timeline oldTimeline = getCurrentTimeline();
pendingOperationAcks++;
newFromIndex = min(newFromIndex, mediaSourceHolderSnapshots.size() - (toIndex - fromIndex));
Util.moveItems(mediaSourceHolderSnapshots, fromIndex, toIndex, newFromIndex);
Timeline newTimeline = createMaskingTimeline();
PlaybackInfo newPlaybackInfo =
Expand Down Expand Up @@ -819,11 +827,11 @@ public void seekTo(
@Player.Command int seekCommand,
boolean isRepeatingCurrentItem) {
verifyApplicationThread();
checkArgument(mediaItemIndex >= 0);
analyticsCollector.notifySeekStarted();
Timeline timeline = playbackInfo.timeline;
if (mediaItemIndex < 0
|| (!timeline.isEmpty() && mediaItemIndex >= timeline.getWindowCount())) {
throw new IllegalSeekPositionException(timeline, mediaItemIndex, positionMs);
if (!timeline.isEmpty() && mediaItemIndex >= timeline.getWindowCount()) {
return;
}
pendingOperationAcks++;
if (isPlayingAd()) {
Expand Down Expand Up @@ -2251,8 +2259,6 @@ private List<MediaSourceList.MediaSourceHolder> addMediaSourceHolders(
}

private PlaybackInfo removeMediaItemsInternal(int fromIndex, int toIndex) {
Assertions.checkArgument(
fromIndex >= 0 && toIndex >= fromIndex && toIndex <= mediaSourceHolderSnapshots.size());
int currentIndex = getCurrentMediaItemIndex();
Timeline oldTimeline = getCurrentTimeline();
int currentMediaSourceCount = mediaSourceHolderSnapshots.size();
Expand Down Expand Up @@ -2291,7 +2297,7 @@ private Timeline createMaskingTimeline() {

private PlaybackInfo maskTimelineAndPosition(
PlaybackInfo playbackInfo, Timeline timeline, @Nullable Pair<Object, Long> periodPositionUs) {
Assertions.checkArgument(timeline.isEmpty() || periodPositionUs != null);
checkArgument(timeline.isEmpty() || periodPositionUs != null);
Timeline oldTimeline = playbackInfo.timeline;
// Mask the timeline.
playbackInfo = playbackInfo.copyWithTimeline(timeline);
Expand Down
119 changes: 94 additions & 25 deletions library/core/src/test/java/com/google/android/exoplayer2/ExoPlayerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -921,31 +921,100 @@ public void periodHoldersReleasedAfterSeekWithRepeatModeAll() throws Exception {
}

@Test
public void illegalSeekPositionDoesThrow() throws Exception {
final IllegalSeekPositionException[] exception = new IllegalSeekPositionException[1];
ActionSchedule actionSchedule =
new ActionSchedule.Builder(TAG)
.waitForPlaybackState(Player.STATE_BUFFERING)
.executeRunnable(
new PlayerRunnable() {
@Override
public void run(ExoPlayer player) {
try {
player.seekTo(/* mediaItemIndex= */ 100, /* positionMs= */ 0);
} catch (IllegalSeekPositionException e) {
exception[0] = e;
}
}
})
.waitForPlaybackState(Player.STATE_ENDED)
.build();
new ExoPlayerTestRunner.Builder(context)
.setActionSchedule(actionSchedule)
.build()
.start()
.blockUntilActionScheduleFinished(TIMEOUT_MS)
.blockUntilEnded(TIMEOUT_MS);
assertThat(exception[0]).isNotNull();
public void seekTo_indexLargerThanPlaylist_isIgnored() throws Exception {
ExoPlayer player = new TestExoPlayerBuilder(context).build();
player.setMediaItem(MediaItem.fromUri("http://test"));

player.seekTo(/* mediaItemIndex= */ 1, /* positionMs= */ 1000);

assertThat(player.getCurrentMediaItemIndex()).isEqualTo(0);
player.release();
}

@Test
public void addMediaItems_indexLargerThanPlaylist_addsToEndOfPlaylist() throws Exception {
ExoPlayer player = new TestExoPlayerBuilder(context).build();
player.setMediaItem(MediaItem.fromUri("http://test"));
ImmutableList<MediaItem> addedItems =
ImmutableList.of(MediaItem.fromUri("http://new1"), MediaItem.fromUri("http://new2"));

player.addMediaItems(/* index= */ 5000, addedItems);

assertThat(player.getMediaItemCount()).isEqualTo(3);
assertThat(player.getMediaItemAt(1)).isEqualTo(addedItems.get(0));
assertThat(player.getMediaItemAt(2)).isEqualTo(addedItems.get(1));
player.release();
}

@Test
public void removeMediaItems_fromIndexLargerThanPlaylist_isIgnored() throws Exception {
ExoPlayer player = new TestExoPlayerBuilder(context).build();
player.setMediaItems(
ImmutableList.of(MediaItem.fromUri("http://item1"), MediaItem.fromUri("http://item2")));

player.removeMediaItems(/* fromIndex= */ 5000, /* toIndex= */ 6000);

assertThat(player.getMediaItemCount()).isEqualTo(2);
player.release();
}

@Test
public void removeMediaItems_toIndexLargerThanPlaylist_removesUpToEndOfPlaylist()
throws Exception {
ExoPlayer player = new TestExoPlayerBuilder(context).build();
player.setMediaItems(
ImmutableList.of(MediaItem.fromUri("http://item1"), MediaItem.fromUri("http://item2")));

player.removeMediaItems(/* fromIndex= */ 1, /* toIndex= */ 6000);

assertThat(player.getMediaItemCount()).isEqualTo(1);
assertThat(player.getMediaItemAt(0).localConfiguration.uri.toString())
.isEqualTo("http://item1");
player.release();
}

@Test
public void moveMediaItems_fromIndexLargerThanPlaylist_isIgnored() throws Exception {
ExoPlayer player = new TestExoPlayerBuilder(context).build();
ImmutableList<MediaItem> items =
ImmutableList.of(MediaItem.fromUri("http://item1"), MediaItem.fromUri("http://item2"));
player.setMediaItems(items);

player.moveMediaItems(/* fromIndex= */ 5000, /* toIndex= */ 6000, /* newIndex= */ 0);

assertThat(player.getMediaItemAt(0)).isEqualTo(items.get(0));
assertThat(player.getMediaItemAt(1)).isEqualTo(items.get(1));
player.release();
}

@Test
public void moveMediaItems_toIndexLargerThanPlaylist_movesItemsUpToEndOfPlaylist()
throws Exception {
ExoPlayer player = new TestExoPlayerBuilder(context).build();
ImmutableList<MediaItem> items =
ImmutableList.of(MediaItem.fromUri("http://item1"), MediaItem.fromUri("http://item2"));
player.setMediaItems(items);

player.moveMediaItems(/* fromIndex= */ 1, /* toIndex= */ 6000, /* newIndex= */ 0);

assertThat(player.getMediaItemAt(0)).isEqualTo(items.get(1));
assertThat(player.getMediaItemAt(1)).isEqualTo(items.get(0));
player.release();
}

@Test
public void moveMediaItems_newIndexLargerThanPlaylist_movesItemsUpToEndOfPlaylist()
throws Exception {
ExoPlayer player = new TestExoPlayerBuilder(context).build();
ImmutableList<MediaItem> items =
ImmutableList.of(MediaItem.fromUri("http://item1"), MediaItem.fromUri("http://item2"));
player.setMediaItems(items);

player.moveMediaItems(/* fromIndex= */ 0, /* toIndex= */ 1, /* newIndex= */ 5000);

assertThat(player.getMediaItemAt(0)).isEqualTo(items.get(1));
assertThat(player.getMediaItemAt(1)).isEqualTo(items.get(0));
player.release();
}

@Test
Expand Down

0 comments on commit a1c0b10

Please sign in to comment.