From 8a2a527129e0060a9fc1ab51da256e82d404192d Mon Sep 17 00:00:00 2001 From: tonihei Date: Mon, 20 Jan 2020 17:03:27 +0000 Subject: [PATCH] Ensure timeline and position are updated even if an error occurs If an exception is thrown between updating the timeline and updating the position in playbackInfo, the state may be inconsistent. Exceptions are expected to be thrown while updating the player state and we should handle such cases in a consistent way. Similar to how we handle the same situation in seekToInternal, the state is updated in a final block such that it gets updated to the latest state even if an error occurs. Moving both the timeline and position update together also ensures they always stay consistent. PiperOrigin-RevId: 290624020 --- .../exoplayer2/ExoPlayerImplInternal.java | 99 +++++++++---------- .../android/exoplayer2/ExoPlayerTest.java | 71 ++++++++++++- 2 files changed, 117 insertions(+), 53 deletions(-) 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 2e2d592d0a..54b363274c 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 @@ -1535,69 +1535,59 @@ import java.util.concurrent.atomic.AtomicBoolean; shuffleModeEnabled, window, period); - - playbackInfo = playbackInfo.copyWithTimeline(timeline); - resolvePendingMessagePositions(); - if (!timeline.isEmpty()) { - // Retain pending seek position only while the timeline is still empty. - pendingInitialSeekPosition = null; - } - - MediaPeriodId oldPeriodId = playbackInfo.periodId; - long oldContentPositionUs = - oldPeriodId.isAd() ? playbackInfo.contentPositionUs : playbackInfo.positionUs; MediaPeriodId newPeriodId = positionUpdate.periodId; long newContentPositionUs = positionUpdate.contentPositionUs; boolean forceBufferingState = positionUpdate.forceBufferingState; long newPositionUs = newPeriodId.isAd() ? 0 : newContentPositionUs; + long oldContentPositionUs = + playbackInfo.periodId.isAd() ? playbackInfo.contentPositionUs : playbackInfo.positionUs; + boolean isPlaybackPositionUnchanged = + playbackInfo.periodId.equals(newPeriodId) && oldContentPositionUs == newContentPositionUs; - if (positionUpdate.endPlayback) { - if (playbackInfo.playbackState != Player.STATE_IDLE) { - setState(Player.STATE_ENDED); + try { + if (positionUpdate.endPlayback) { + if (playbackInfo.playbackState != Player.STATE_IDLE) { + setState(Player.STATE_ENDED); + } + resetInternal( + /* resetRenderers= */ false, + /* resetPosition= */ false, + /* releasePlaylist= */ false, + /* clearPlaylist= */ false, + /* resetError= */ true); } - playbackInfo = copyWithNewPosition(newPeriodId, newPositionUs, newContentPositionUs); - // Reset, but retain the playlist and new position. - resetInternal( - /* resetRenderers= */ false, - /* resetPosition= */ false, - /* releasePlaylist= */ false, - /* clearPlaylist= */ false, - /* resetError= */ true); - if (timeline.isEmpty()) { - return; - } - } - - if (oldPeriodId.equals(newPeriodId) && oldContentPositionUs == newContentPositionUs) { - // We can keep the current playing period. Update the rest of the queued periods. - if (!queue.updateQueuedPeriods( - timeline, rendererPositionUs, getMaxRendererReadPositionUs())) { - seekToCurrentPosition(/* sendDiscontinuity= */ false); - } - } else { - // Something changed. Seek to new start position. - @Nullable MediaPeriodHolder periodHolder = queue.getPlayingPeriod(); - if (periodHolder != null) { - // Update the new playing media period info if it already exists. - while (periodHolder.getNext() != null) { - periodHolder = periodHolder.getNext(); - if (periodHolder.info.id.equals(newPeriodId)) { - periodHolder.info = queue.getUpdatedMediaPeriodInfo(timeline, periodHolder.info); + if (isPlaybackPositionUnchanged) { + // We can keep the current playing period. Update the rest of the queued periods. + if (!queue.updateQueuedPeriods( + timeline, rendererPositionUs, getMaxRendererReadPositionUs())) { + seekToCurrentPosition(/* sendDiscontinuity= */ false); + } + } else if (!timeline.isEmpty()) { + // Something changed. Seek to new start position. + @Nullable MediaPeriodHolder periodHolder = queue.getPlayingPeriod(); + if (periodHolder != null) { + // Update the new playing media period info if it already exists. + while (periodHolder.getNext() != null) { + periodHolder = periodHolder.getNext(); + if (periodHolder.info.id.equals(newPeriodId)) { + periodHolder.info = queue.getUpdatedMediaPeriodInfo(timeline, periodHolder.info); + } } } + newPositionUs = seekToPeriodPosition(newPeriodId, newPositionUs, forceBufferingState); } - if (!newPeriodId.isAd() && newContentPositionUs == C.TIME_UNSET) { - // Get the default position for the first new period that is not an ad. - int windowIndex = timeline.getPeriodByUid(newPeriodId.periodUid, period).windowIndex; - newContentPositionUs = timeline.getWindow(windowIndex, window).getDefaultPositionUs(); - newPositionUs = newContentPositionUs; + } finally { + if (!isPlaybackPositionUnchanged) { + playbackInfo = copyWithNewPosition(newPeriodId, newPositionUs, newContentPositionUs); } - // Actually do the seek. - long seekedToPositionUs = - seekToPeriodPosition(newPeriodId, newPositionUs, forceBufferingState); - playbackInfo = copyWithNewPosition(newPeriodId, seekedToPositionUs, newContentPositionUs); + playbackInfo = playbackInfo.copyWithTimeline(timeline); + resolvePendingMessagePositions(); + if (!timeline.isEmpty()) { + // Retain pending seek position only while the timeline is still empty. + pendingInitialSeekPosition = null; + } + handleLoadingMediaPeriodChanged(/* loadingTrackSelectionChanged= */ false); } - handleLoadingMediaPeriodChanged(/* loadingTrackSelectionChanged= */ false); } private long getMaxRendererReadPositionUs() { @@ -2178,12 +2168,17 @@ import java.util.concurrent.atomic.AtomicBoolean; // Ensure ad insertion metadata is up to date. long contentPositionForAdResolution = newContentPositionUs; if (contentPositionForAdResolution == C.TIME_UNSET) { + // TODO: Fix me. Using a window position as period position is wrong. contentPositionForAdResolution = timeline.getWindow(timeline.getPeriodByUid(newPeriodUid, period).windowIndex, window) .defaultPositionUs; } MediaPeriodId periodIdWithAds = queue.resolveMediaPeriodIdForAds(timeline, newPeriodUid, contentPositionForAdResolution); + if (!periodIdWithAds.isAd() && newContentPositionUs == C.TIME_UNSET) { + // We are not going to play an ad, so use resolved content position. + newContentPositionUs = contentPositionForAdResolution; + } boolean oldAndNewPeriodIdAreSame = oldPeriodId.periodUid.equals(newPeriodUid) && !oldPeriodId.isAd() 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 36809c4ee7..8e42309e89 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 @@ -5593,7 +5593,7 @@ public final class ExoPlayerTest { } @Test - public void errorThrownDuringPeriodTransition_keepsConsistentPlayerState() throws Exception { + public void errorThrownDuringPeriodTransition_keepsConsistentPlayerState() { FakeMediaSource source1 = new FakeMediaSource(new FakeTimeline(/* windowCount= */ 1), Builder.VIDEO_FORMAT); FakeMediaSource source2 = @@ -5658,6 +5658,75 @@ public final class ExoPlayerTest { assertThat(trackSelectionsAfterError.get().get(1)).isNotNull(); // Audio renderer. } + @Test + public void errorThrownDuringPlaylistUpdate_keepsConsistentPlayerState() { + FakeMediaSource source1 = + new FakeMediaSource(new FakeTimeline(/* windowCount= */ 1), Builder.VIDEO_FORMAT); + FakeMediaSource source2 = + new FakeMediaSource(new FakeTimeline(/* windowCount= */ 1), Builder.AUDIO_FORMAT); + FakeRenderer videoRenderer = new FakeRenderer(Builder.VIDEO_FORMAT); + FakeRenderer audioRenderer = + new FakeRenderer(Builder.AUDIO_FORMAT) { + @Override + protected void onEnabled(boolean joining) throws ExoPlaybackException { + // Fail when enabling the renderer. This will happen during the playlist update. + throw createRendererException(new IllegalStateException(), Builder.AUDIO_FORMAT); + } + }; + AtomicReference timelineAfterError = new AtomicReference<>(); + AtomicReference trackGroupsAfterError = new AtomicReference<>(); + AtomicReference trackSelectionsAfterError = new AtomicReference<>(); + AtomicInteger windowIndexAfterError = new AtomicInteger(); + ActionSchedule actionSchedule = + new ActionSchedule.Builder("errorThrownDuringPlaylistUpdate_keepsConsistentPlayerState") + .executeRunnable( + new PlayerRunnable() { + @Override + public void run(SimpleExoPlayer player) { + player.addAnalyticsListener( + new AnalyticsListener() { + @Override + public void onPlayerError( + EventTime eventTime, ExoPlaybackException error) { + timelineAfterError.set(player.getCurrentTimeline()); + trackGroupsAfterError.set(player.getCurrentTrackGroups()); + trackSelectionsAfterError.set(player.getCurrentTrackSelections()); + windowIndexAfterError.set(player.getCurrentWindowIndex()); + } + }); + } + }) + .pause() + // Wait until fully buffered so that the new renderer can be enabled immediately. + .waitForIsLoading(true) + .waitForIsLoading(false) + .waitForIsLoading(true) + .waitForIsLoading(false) + .removeMediaItem(0) + .build(); + ExoPlayerTestRunner testRunner = + new Builder() + .setMediaSources(source1, source2) + .setActionSchedule(actionSchedule) + .setRenderers(videoRenderer, audioRenderer) + .build(context); + + assertThrows( + ExoPlaybackException.class, + () -> + testRunner + .start(/* doPrepare= */ true) + .blockUntilActionScheduleFinished(TIMEOUT_MS) + .blockUntilEnded(TIMEOUT_MS)); + + assertThat(timelineAfterError.get().getWindowCount()).isEqualTo(1); + assertThat(windowIndexAfterError.get()).isEqualTo(0); + assertThat(trackGroupsAfterError.get().length).isEqualTo(1); + assertThat(trackGroupsAfterError.get().get(0).getFormat(0)).isEqualTo(Builder.AUDIO_FORMAT); + assertThat(trackSelectionsAfterError.get().get(0)).isNull(); // Video renderer. + assertThat(trackSelectionsAfterError.get().get(1)).isNotNull(); // Audio renderer. + } + // Internal methods. private static ActionSchedule.Builder addSurfaceSwitch(ActionSchedule.Builder builder) {