Commit 138b2224 by tonihei Committed by Oliver Woodman

Prevent dummy period id in ExoPlayerImplInternal from leaking into actual use.

While the timeline is empty, we keep a dummy MediaPeriodId in PlaybackInfo with
a period index of 0. We leak this MediaPeriodId in actual use in these
situations:
 1. When issuing an IllegalSeekPosition after preparation. The timeline becomes
    non-empty, but the media period id stays at its dummy value.
 2. When re-adding sources to a previously empty timeline. The dummy period id
    is used as the start position for the new non-empty timeline.

This change makes:
 - the constructor of PlaybackInfo using those dummy values more explicit
 - prevents the issues above by using the correct default position in the new
   non-empty timeline for the above mentioned cases.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=205803006
parent ffdc17d0
...@@ -120,12 +120,7 @@ import java.util.concurrent.CopyOnWriteArraySet; ...@@ -120,12 +120,7 @@ import java.util.concurrent.CopyOnWriteArraySet;
ExoPlayerImpl.this.handleEvent(msg); ExoPlayerImpl.this.handleEvent(msg);
} }
}; };
playbackInfo = playbackInfo = PlaybackInfo.createDummy(/* startPositionUs= */ 0, emptyTrackSelectorResult);
new PlaybackInfo(
Timeline.EMPTY,
/* startPositionUs= */ 0,
TrackGroupArray.EMPTY,
emptyTrackSelectorResult);
pendingPlaybackInfoUpdates = new ArrayDeque<>(); pendingPlaybackInfoUpdates = new ArrayDeque<>();
internalPlayer = internalPlayer =
new ExoPlayerImplInternal( new ExoPlayerImplInternal(
......
...@@ -151,11 +151,7 @@ import java.util.Collections; ...@@ -151,11 +151,7 @@ import java.util.Collections;
seekParameters = SeekParameters.DEFAULT; seekParameters = SeekParameters.DEFAULT;
playbackInfo = playbackInfo =
new PlaybackInfo( PlaybackInfo.createDummy(/* startPositionUs= */ C.TIME_UNSET, emptyTrackSelectorResult);
Timeline.EMPTY,
/* startPositionUs= */ C.TIME_UNSET,
TrackGroupArray.EMPTY,
emptyTrackSelectorResult);
playbackInfoUpdate = new PlaybackInfoUpdate(); playbackInfoUpdate = new PlaybackInfoUpdate();
rendererCapabilities = new RendererCapabilities[renderers.length]; rendererCapabilities = new RendererCapabilities[renderers.length];
for (int i = 0; i < renderers.length; i++) { for (int i = 0; i < renderers.length; i++) {
...@@ -603,7 +599,7 @@ import java.util.Collections; ...@@ -603,7 +599,7 @@ import java.util.Collections;
if (resolvedSeekPosition == null) { if (resolvedSeekPosition == null) {
// The seek position was valid for the timeline that it was performed into, but the // The seek position was valid for the timeline that it was performed into, but the
// timeline has changed or is not ready and a suitable seek position could not be resolved. // timeline has changed or is not ready and a suitable seek position could not be resolved.
periodId = new MediaPeriodId(getFirstPeriodIndex()); periodId = getFirstMediaPeriodId();
periodPositionUs = C.TIME_UNSET; periodPositionUs = C.TIME_UNSET;
contentPositionUs = C.TIME_UNSET; contentPositionUs = C.TIME_UNSET;
seekPositionAdjusted = true; seekPositionAdjusted = true;
...@@ -753,12 +749,15 @@ import java.util.Collections; ...@@ -753,12 +749,15 @@ import java.util.Collections;
} }
} }
private int getFirstPeriodIndex() { private MediaPeriodId getFirstMediaPeriodId() {
Timeline timeline = playbackInfo.timeline; Timeline timeline = playbackInfo.timeline;
return timeline.isEmpty() if (timeline.isEmpty()) {
? 0 return PlaybackInfo.DUMMY_MEDIA_PERIOD_ID;
: timeline.getWindow(timeline.getFirstWindowIndex(shuffleModeEnabled), window) }
int firstPeriodIndex =
timeline.getWindow(timeline.getFirstWindowIndex(shuffleModeEnabled), window)
.firstPeriodIndex; .firstPeriodIndex;
return new MediaPeriodId(firstPeriodIndex);
} }
private void resetInternal( private void resetInternal(
...@@ -790,8 +789,7 @@ import java.util.Collections; ...@@ -790,8 +789,7 @@ import java.util.Collections;
nextPendingMessageIndex = 0; nextPendingMessageIndex = 0;
} }
// Set the start position to TIME_UNSET so that a subsequent seek to 0 isn't ignored. // Set the start position to TIME_UNSET so that a subsequent seek to 0 isn't ignored.
MediaPeriodId mediaPeriodId = MediaPeriodId mediaPeriodId = resetPosition ? getFirstMediaPeriodId() : playbackInfo.periodId;
resetPosition ? new MediaPeriodId(getFirstPeriodIndex()) : playbackInfo.periodId;
long startPositionUs = resetPosition ? C.TIME_UNSET : playbackInfo.positionUs; long startPositionUs = resetPosition ? C.TIME_UNSET : playbackInfo.positionUs;
long contentPositionUs = resetPosition ? C.TIME_UNSET : playbackInfo.contentPositionUs; long contentPositionUs = resetPosition ? C.TIME_UNSET : playbackInfo.contentPositionUs;
playbackInfo = playbackInfo =
...@@ -1155,8 +1153,15 @@ import java.util.Collections; ...@@ -1155,8 +1153,15 @@ import java.util.Collections;
playbackInfoUpdate.incrementPendingOperationAcks(pendingPrepareCount); playbackInfoUpdate.incrementPendingOperationAcks(pendingPrepareCount);
pendingPrepareCount = 0; pendingPrepareCount = 0;
if (pendingInitialSeekPosition != null) { if (pendingInitialSeekPosition != null) {
Pair<Integer, Long> periodPosition = Pair<Integer, Long> periodPosition;
resolveSeekPosition(pendingInitialSeekPosition, /* trySubsequentPeriods= */ true); try {
periodPosition =
resolveSeekPosition(pendingInitialSeekPosition, /* trySubsequentPeriods= */ true);
} catch (IllegalSeekPositionException e) {
playbackInfo =
playbackInfo.fromNewPosition(getFirstMediaPeriodId(), C.TIME_UNSET, C.TIME_UNSET);
throw e;
}
pendingInitialSeekPosition = null; pendingInitialSeekPosition = null;
if (periodPosition == null) { if (periodPosition == null) {
// The seek position was valid for the timeline that it was performed into, but the // The seek position was valid for the timeline that it was performed into, but the
...@@ -1189,20 +1194,26 @@ import java.util.Collections; ...@@ -1189,20 +1194,26 @@ import java.util.Collections;
return; return;
} }
int playingPeriodIndex = playbackInfo.periodId.periodIndex;
long contentPositionUs = playbackInfo.contentPositionUs;
if (oldTimeline.isEmpty()) { if (oldTimeline.isEmpty()) {
// If the old timeline is empty, the period queue is also empty. // If the old timeline is empty, the period queue is also empty.
if (!timeline.isEmpty()) { if (!timeline.isEmpty()) {
MediaPeriodId periodId = Pair<Integer, Long> defaultPosition =
queue.resolveMediaPeriodIdForAds(playingPeriodIndex, contentPositionUs); getPeriodPosition(
timeline, timeline.getFirstWindowIndex(shuffleModeEnabled), C.TIME_UNSET);
int periodIndex = defaultPosition.first;
long startPositionUs = defaultPosition.second;
MediaPeriodId periodId = queue.resolveMediaPeriodIdForAds(periodIndex, startPositionUs);
playbackInfo = playbackInfo =
playbackInfo.fromNewPosition( playbackInfo.fromNewPosition(
periodId, periodId.isAd() ? 0 : contentPositionUs, contentPositionUs); periodId,
/* startPositionUs= */ periodId.isAd() ? 0 : startPositionUs,
/* contentPositionUs= */ startPositionUs);
} }
return; return;
} }
MediaPeriodHolder periodHolder = queue.getFrontPeriod(); MediaPeriodHolder periodHolder = queue.getFrontPeriod();
int playingPeriodIndex = playbackInfo.periodId.periodIndex;
long contentPositionUs = playbackInfo.contentPositionUs;
Object playingPeriodUid = Object playingPeriodUid =
periodHolder == null ? oldTimeline.getUidOfPeriod(playingPeriodIndex) : periodHolder.uid; periodHolder == null ? oldTimeline.getUidOfPeriod(playingPeriodIndex) : periodHolder.uid;
int periodIndex = timeline.getIndexOfPeriod(playingPeriodUid); int periodIndex = timeline.getIndexOfPeriod(playingPeriodUid);
......
...@@ -47,17 +47,18 @@ import com.google.android.exoplayer2.util.Assertions; ...@@ -47,17 +47,18 @@ import com.google.android.exoplayer2.util.Assertions;
private Timeline timeline; private Timeline timeline;
private @RepeatMode int repeatMode; private @RepeatMode int repeatMode;
private boolean shuffleModeEnabled; private boolean shuffleModeEnabled;
private MediaPeriodHolder playing; private @Nullable MediaPeriodHolder playing;
private MediaPeriodHolder reading; private @Nullable MediaPeriodHolder reading;
private MediaPeriodHolder loading; private @Nullable MediaPeriodHolder loading;
private int length; private int length;
private Object oldFrontPeriodUid; private @Nullable Object oldFrontPeriodUid;
private long oldFrontPeriodWindowSequenceNumber; private long oldFrontPeriodWindowSequenceNumber;
/** Creates a new media period queue. */ /** Creates a new media period queue. */
public MediaPeriodQueue() { public MediaPeriodQueue() {
period = new Timeline.Period(); period = new Timeline.Period();
window = new Timeline.Window(); window = new Timeline.Window();
timeline = Timeline.EMPTY;
} }
/** /**
......
...@@ -25,6 +25,12 @@ import com.google.android.exoplayer2.trackselection.TrackSelectorResult; ...@@ -25,6 +25,12 @@ import com.google.android.exoplayer2.trackselection.TrackSelectorResult;
*/ */
/* package */ final class PlaybackInfo { /* package */ final class PlaybackInfo {
/**
* Dummy media period id used while the timeline is empty and no period id is specified. This id
* is used when playback infos are created with {@link #createDummy(long, TrackSelectorResult)}.
*/
public static final MediaPeriodId DUMMY_MEDIA_PERIOD_ID = new MediaPeriodId(/* periodIndex= */ 0);
/** The current {@link Timeline}. */ /** The current {@link Timeline}. */
public final Timeline timeline; public final Timeline timeline;
/** The current manifest. */ /** The current manifest. */
...@@ -69,22 +75,28 @@ import com.google.android.exoplayer2.trackselection.TrackSelectorResult; ...@@ -69,22 +75,28 @@ import com.google.android.exoplayer2.trackselection.TrackSelectorResult;
*/ */
public volatile long positionUs; public volatile long positionUs;
public PlaybackInfo( /**
Timeline timeline, * Creates empty dummy playback info which can be used for masking as long as no real playback
long startPositionUs, * info is available.
TrackGroupArray trackGroups, *
TrackSelectorResult trackSelectorResult) { * @param startPositionUs The start position at which playback should start, in microseconds.
this( * @param emptyTrackSelectorResult An empty track selector result with null entries for each
timeline, * renderer.
* @return A dummy playback info.
*/
public static PlaybackInfo createDummy(
long startPositionUs, TrackSelectorResult emptyTrackSelectorResult) {
return new PlaybackInfo(
Timeline.EMPTY,
/* manifest= */ null, /* manifest= */ null,
new MediaPeriodId(/* periodIndex= */ 0), DUMMY_MEDIA_PERIOD_ID,
startPositionUs, startPositionUs,
/* contentPositionUs =*/ C.TIME_UNSET, /* contentPositionUs =*/ C.TIME_UNSET,
Player.STATE_IDLE, Player.STATE_IDLE,
/* isLoading= */ false, /* isLoading= */ false,
trackGroups, TrackGroupArray.EMPTY,
trackSelectorResult, emptyTrackSelectorResult,
new MediaPeriodId(/* periodIndex= */ 0), DUMMY_MEDIA_PERIOD_ID,
startPositionUs, startPositionUs,
/* totalBufferedDurationUs= */ 0, /* totalBufferedDurationUs= */ 0,
startPositionUs); startPositionUs);
......
...@@ -52,6 +52,7 @@ import java.util.Arrays; ...@@ -52,6 +52,7 @@ import java.util.Arrays;
import java.util.Collections; import java.util.Collections;
import java.util.List; import java.util.List;
import java.util.concurrent.CountDownLatch; import java.util.concurrent.CountDownLatch;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicReference; import java.util.concurrent.atomic.AtomicReference;
import org.junit.Test; import org.junit.Test;
import org.junit.runner.RunWith; import org.junit.runner.RunWith;
...@@ -551,6 +552,7 @@ public final class ExoPlayerTest { ...@@ -551,6 +552,7 @@ public final class ExoPlayerTest {
fail(); fail();
} catch (ExoPlaybackException e) { } catch (ExoPlaybackException e) {
// Expected exception. // Expected exception.
assertThat(e.getUnexpectedException()).isInstanceOf(IllegalSeekPositionException.class);
} }
assertThat(onSeekProcessedCalled[0]).isTrue(); assertThat(onSeekProcessedCalled[0]).isTrue();
} }
...@@ -1280,12 +1282,97 @@ public final class ExoPlayerTest { ...@@ -1280,12 +1282,97 @@ public final class ExoPlayerTest {
fail(); fail();
} catch (ExoPlaybackException e) { } catch (ExoPlaybackException e) {
// Expected exception. // Expected exception.
assertThat(e.getUnexpectedException()).isInstanceOf(IllegalSeekPositionException.class);
} }
testRunner.assertTimelinesEqual(timeline); testRunner.assertTimelinesEqual(timeline);
testRunner.assertTimelineChangeReasonsEqual(Player.TIMELINE_CHANGE_REASON_PREPARED); testRunner.assertTimelineChangeReasonsEqual(Player.TIMELINE_CHANGE_REASON_PREPARED);
} }
@Test @Test
public void testPlaybackErrorDuringSourceInfoRefreshWithShuffleModeEnabledUsesCorrectFirstPeriod()
throws Exception {
Timeline timeline = new FakeTimeline(/* windowCount= */ 1);
FakeMediaSource mediaSource = new FakeMediaSource(/* timeline= */ null, /* manifest= */ null);
ConcatenatingMediaSource concatenatingMediaSource =
new ConcatenatingMediaSource(
/* isAtomic= */ false, new FakeShuffleOrder(0), mediaSource, mediaSource);
AtomicInteger windowIndexAfterReprepare = new AtomicInteger();
ActionSchedule actionSchedule =
new ActionSchedule.Builder("testPlaybackErrorDuringSourceInfoRefreshUsesCorrectFirstPeriod")
.setShuffleModeEnabled(true)
.waitForPlaybackState(Player.STATE_BUFFERING)
// Cause an internal exception by seeking to an invalid position while the media source
// is still being prepared. The error will be thrown while the player handles the new
// source info.
.seek(/* windowIndex= */ 100, /* positionMs= */ 0)
.executeRunnable(() -> mediaSource.setNewSourceInfo(timeline, /* newManifest= */ null))
.waitForPlaybackState(Player.STATE_IDLE)
// Re-prepare to play the source in its default shuffled order.
.prepareSource(
concatenatingMediaSource, /* resetPosition= */ false, /* resetState= */ false)
.waitForTimelineChanged(null)
.executeRunnable(
new PlayerRunnable() {
@Override
public void run(SimpleExoPlayer player) {
windowIndexAfterReprepare.set(player.getCurrentWindowIndex());
}
})
.build();
ExoPlayerTestRunner testRunner =
new ExoPlayerTestRunner.Builder()
.setMediaSource(concatenatingMediaSource)
.setActionSchedule(actionSchedule)
.build();
try {
testRunner.start().blockUntilActionScheduleFinished(TIMEOUT_MS).blockUntilEnded(TIMEOUT_MS);
fail();
} catch (ExoPlaybackException e) {
// Expected exception.
assertThat(e.getUnexpectedException()).isInstanceOf(IllegalSeekPositionException.class);
}
assertThat(windowIndexAfterReprepare.get()).isEqualTo(1);
}
@Test
public void testRestartAfterEmptyTimelineWithShuffleModeEnabledUsesCorrectFirstPeriod()
throws Exception {
Timeline timeline = new FakeTimeline(/* windowCount= */ 1);
FakeMediaSource mediaSource = new FakeMediaSource(timeline, /* manifest= */ null);
ConcatenatingMediaSource concatenatingMediaSource =
new ConcatenatingMediaSource(/* isAtomic= */ false, new FakeShuffleOrder(0));
AtomicInteger windowIndexAfterAddingSources = new AtomicInteger();
ActionSchedule actionSchedule =
new ActionSchedule.Builder("testRestartAfterEmptyTimelineUsesCorrectFirstPeriod")
.setShuffleModeEnabled(true)
// Preparing with an empty media source will transition to ended state.
.waitForPlaybackState(Player.STATE_ENDED)
// Add two sources at once such that the default start position in the shuffled order
// will be the second source.
.executeRunnable(
() ->
concatenatingMediaSource.addMediaSources(
Arrays.asList(mediaSource, mediaSource)))
.waitForTimelineChanged(null)
.executeRunnable(
new PlayerRunnable() {
@Override
public void run(SimpleExoPlayer player) {
windowIndexAfterAddingSources.set(player.getCurrentWindowIndex());
}
})
.build();
new ExoPlayerTestRunner.Builder()
.setMediaSource(concatenatingMediaSource)
.setActionSchedule(actionSchedule)
.build()
.start()
.blockUntilActionScheduleFinished(TIMEOUT_MS)
.blockUntilEnded(TIMEOUT_MS);
assertThat(windowIndexAfterAddingSources.get()).isEqualTo(1);
}
@Test
public void testPlaybackErrorAndReprepareDoesNotResetPosition() throws Exception { public void testPlaybackErrorAndReprepareDoesNotResetPosition() throws Exception {
final Timeline timeline = new FakeTimeline(/* windowCount= */ 2); final Timeline timeline = new FakeTimeline(/* windowCount= */ 2);
final long[] positionHolder = new long[3]; final long[] positionHolder = new long[3];
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or sign in to comment