From 138b222457015bbe416de83c2aef24816c18c0de Mon Sep 17 00:00:00 2001 From: tonihei Date: Tue, 24 Jul 2018 03:14:15 -0700 Subject: [PATCH] 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 --- .../android/exoplayer2/ExoPlayerImpl.java | 7 +- .../exoplayer2/ExoPlayerImplInternal.java | 49 +++++++---- .../android/exoplayer2/MediaPeriodQueue.java | 9 +- .../android/exoplayer2/PlaybackInfo.java | 34 +++++--- .../android/exoplayer2/ExoPlayerTest.java | 87 +++++++++++++++++++ 5 files changed, 146 insertions(+), 40 deletions(-) diff --git a/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java b/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java index e0d2233e46..0435d221fe 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java @@ -120,12 +120,7 @@ import java.util.concurrent.CopyOnWriteArraySet; ExoPlayerImpl.this.handleEvent(msg); } }; - playbackInfo = - new PlaybackInfo( - Timeline.EMPTY, - /* startPositionUs= */ 0, - TrackGroupArray.EMPTY, - emptyTrackSelectorResult); + playbackInfo = PlaybackInfo.createDummy(/* startPositionUs= */ 0, emptyTrackSelectorResult); pendingPlaybackInfoUpdates = new ArrayDeque<>(); internalPlayer = new ExoPlayerImplInternal( diff --git a/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImplInternal.java b/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImplInternal.java index ba80407860..a4b1238840 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImplInternal.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImplInternal.java @@ -151,11 +151,7 @@ import java.util.Collections; seekParameters = SeekParameters.DEFAULT; playbackInfo = - new PlaybackInfo( - Timeline.EMPTY, - /* startPositionUs= */ C.TIME_UNSET, - TrackGroupArray.EMPTY, - emptyTrackSelectorResult); + PlaybackInfo.createDummy(/* startPositionUs= */ C.TIME_UNSET, emptyTrackSelectorResult); playbackInfoUpdate = new PlaybackInfoUpdate(); rendererCapabilities = new RendererCapabilities[renderers.length]; for (int i = 0; i < renderers.length; i++) { @@ -603,7 +599,7 @@ import java.util.Collections; if (resolvedSeekPosition == null) { // 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. - periodId = new MediaPeriodId(getFirstPeriodIndex()); + periodId = getFirstMediaPeriodId(); periodPositionUs = C.TIME_UNSET; contentPositionUs = C.TIME_UNSET; seekPositionAdjusted = true; @@ -753,12 +749,15 @@ import java.util.Collections; } } - private int getFirstPeriodIndex() { + private MediaPeriodId getFirstMediaPeriodId() { Timeline timeline = playbackInfo.timeline; - return timeline.isEmpty() - ? 0 - : timeline.getWindow(timeline.getFirstWindowIndex(shuffleModeEnabled), window) + if (timeline.isEmpty()) { + return PlaybackInfo.DUMMY_MEDIA_PERIOD_ID; + } + int firstPeriodIndex = + timeline.getWindow(timeline.getFirstWindowIndex(shuffleModeEnabled), window) .firstPeriodIndex; + return new MediaPeriodId(firstPeriodIndex); } private void resetInternal( @@ -790,8 +789,7 @@ import java.util.Collections; nextPendingMessageIndex = 0; } // Set the start position to TIME_UNSET so that a subsequent seek to 0 isn't ignored. - MediaPeriodId mediaPeriodId = - resetPosition ? new MediaPeriodId(getFirstPeriodIndex()) : playbackInfo.periodId; + MediaPeriodId mediaPeriodId = resetPosition ? getFirstMediaPeriodId() : playbackInfo.periodId; long startPositionUs = resetPosition ? C.TIME_UNSET : playbackInfo.positionUs; long contentPositionUs = resetPosition ? C.TIME_UNSET : playbackInfo.contentPositionUs; playbackInfo = @@ -1155,8 +1153,15 @@ import java.util.Collections; playbackInfoUpdate.incrementPendingOperationAcks(pendingPrepareCount); pendingPrepareCount = 0; if (pendingInitialSeekPosition != null) { - Pair periodPosition = - resolveSeekPosition(pendingInitialSeekPosition, /* trySubsequentPeriods= */ true); + Pair periodPosition; + try { + periodPosition = + resolveSeekPosition(pendingInitialSeekPosition, /* trySubsequentPeriods= */ true); + } catch (IllegalSeekPositionException e) { + playbackInfo = + playbackInfo.fromNewPosition(getFirstMediaPeriodId(), C.TIME_UNSET, C.TIME_UNSET); + throw e; + } pendingInitialSeekPosition = null; if (periodPosition == null) { // The seek position was valid for the timeline that it was performed into, but the @@ -1189,20 +1194,26 @@ import java.util.Collections; return; } - int playingPeriodIndex = playbackInfo.periodId.periodIndex; - long contentPositionUs = playbackInfo.contentPositionUs; if (oldTimeline.isEmpty()) { // If the old timeline is empty, the period queue is also empty. if (!timeline.isEmpty()) { - MediaPeriodId periodId = - queue.resolveMediaPeriodIdForAds(playingPeriodIndex, contentPositionUs); + Pair defaultPosition = + getPeriodPosition( + timeline, timeline.getFirstWindowIndex(shuffleModeEnabled), C.TIME_UNSET); + int periodIndex = defaultPosition.first; + long startPositionUs = defaultPosition.second; + MediaPeriodId periodId = queue.resolveMediaPeriodIdForAds(periodIndex, startPositionUs); playbackInfo = playbackInfo.fromNewPosition( - periodId, periodId.isAd() ? 0 : contentPositionUs, contentPositionUs); + periodId, + /* startPositionUs= */ periodId.isAd() ? 0 : startPositionUs, + /* contentPositionUs= */ startPositionUs); } return; } MediaPeriodHolder periodHolder = queue.getFrontPeriod(); + int playingPeriodIndex = playbackInfo.periodId.periodIndex; + long contentPositionUs = playbackInfo.contentPositionUs; Object playingPeriodUid = periodHolder == null ? oldTimeline.getUidOfPeriod(playingPeriodIndex) : periodHolder.uid; int periodIndex = timeline.getIndexOfPeriod(playingPeriodUid); diff --git a/library/core/src/main/java/com/google/android/exoplayer2/MediaPeriodQueue.java b/library/core/src/main/java/com/google/android/exoplayer2/MediaPeriodQueue.java index be618275c4..e9be2d985e 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/MediaPeriodQueue.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/MediaPeriodQueue.java @@ -47,17 +47,18 @@ import com.google.android.exoplayer2.util.Assertions; private Timeline timeline; private @RepeatMode int repeatMode; private boolean shuffleModeEnabled; - private MediaPeriodHolder playing; - private MediaPeriodHolder reading; - private MediaPeriodHolder loading; + private @Nullable MediaPeriodHolder playing; + private @Nullable MediaPeriodHolder reading; + private @Nullable MediaPeriodHolder loading; private int length; - private Object oldFrontPeriodUid; + private @Nullable Object oldFrontPeriodUid; private long oldFrontPeriodWindowSequenceNumber; /** Creates a new media period queue. */ public MediaPeriodQueue() { period = new Timeline.Period(); window = new Timeline.Window(); + timeline = Timeline.EMPTY; } /** diff --git a/library/core/src/main/java/com/google/android/exoplayer2/PlaybackInfo.java b/library/core/src/main/java/com/google/android/exoplayer2/PlaybackInfo.java index d79f0538ae..b556db6be9 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/PlaybackInfo.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/PlaybackInfo.java @@ -25,6 +25,12 @@ import com.google.android.exoplayer2.trackselection.TrackSelectorResult; */ /* 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}. */ public final Timeline timeline; /** The current manifest. */ @@ -69,22 +75,28 @@ import com.google.android.exoplayer2.trackselection.TrackSelectorResult; */ public volatile long positionUs; - public PlaybackInfo( - Timeline timeline, - long startPositionUs, - TrackGroupArray trackGroups, - TrackSelectorResult trackSelectorResult) { - this( - timeline, + /** + * Creates empty dummy playback info which can be used for masking as long as no real playback + * info is available. + * + * @param startPositionUs The start position at which playback should start, in microseconds. + * @param emptyTrackSelectorResult An empty track selector result with null entries for each + * renderer. + * @return A dummy playback info. + */ + public static PlaybackInfo createDummy( + long startPositionUs, TrackSelectorResult emptyTrackSelectorResult) { + return new PlaybackInfo( + Timeline.EMPTY, /* manifest= */ null, - new MediaPeriodId(/* periodIndex= */ 0), + DUMMY_MEDIA_PERIOD_ID, startPositionUs, /* contentPositionUs =*/ C.TIME_UNSET, Player.STATE_IDLE, /* isLoading= */ false, - trackGroups, - trackSelectorResult, - new MediaPeriodId(/* periodIndex= */ 0), + TrackGroupArray.EMPTY, + emptyTrackSelectorResult, + DUMMY_MEDIA_PERIOD_ID, startPositionUs, /* totalBufferedDurationUs= */ 0, startPositionUs); diff --git a/library/core/src/test/java/com/google/android/exoplayer2/ExoPlayerTest.java b/library/core/src/test/java/com/google/android/exoplayer2/ExoPlayerTest.java index 846d538abc..67222f5f13 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/ExoPlayerTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/ExoPlayerTest.java @@ -52,6 +52,7 @@ import java.util.Arrays; import java.util.Collections; import java.util.List; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; import org.junit.Test; import org.junit.runner.RunWith; @@ -551,6 +552,7 @@ public final class ExoPlayerTest { fail(); } catch (ExoPlaybackException e) { // Expected exception. + assertThat(e.getUnexpectedException()).isInstanceOf(IllegalSeekPositionException.class); } assertThat(onSeekProcessedCalled[0]).isTrue(); } @@ -1280,11 +1282,96 @@ public final class ExoPlayerTest { fail(); } catch (ExoPlaybackException e) { // Expected exception. + assertThat(e.getUnexpectedException()).isInstanceOf(IllegalSeekPositionException.class); } testRunner.assertTimelinesEqual(timeline); testRunner.assertTimelineChangeReasonsEqual(Player.TIMELINE_CHANGE_REASON_PREPARED); } + @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 { final Timeline timeline = new FakeTimeline(/* windowCount= */ 2);