From 318618d7a2412facf1274bd166608b6c2f8f0b7d Mon Sep 17 00:00:00 2001 From: tonihei Date: Wed, 13 Dec 2017 01:59:28 -0800 Subject: [PATCH] Fix seek/prepare/stop acks when exception is thrown. 1. The player doesn't acknowledge phantom stops when an exception is thrown anymore. 2. It also makes sure it doesn't reset the pendingPrepareCount unless it's actually immediately acknowledging these prepares. 3. It ensures a seek is acknowledged even though an exception is thrown during seeking. Added tests (which previously failed) for all three cases. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=178876362 --- .../android/exoplayer2/ExoPlayerTest.java | 100 +++++++++++++++ .../exoplayer2/ExoPlayerImplInternal.java | 121 ++++++++---------- .../testutil/ExoPlayerTestRunner.java | 6 +- 3 files changed, 158 insertions(+), 69 deletions(-) diff --git a/library/core/src/androidTest/java/com/google/android/exoplayer2/ExoPlayerTest.java b/library/core/src/androidTest/java/com/google/android/exoplayer2/ExoPlayerTest.java index 714dfff676..dad891718e 100644 --- a/library/core/src/androidTest/java/com/google/android/exoplayer2/ExoPlayerTest.java +++ b/library/core/src/androidTest/java/com/google/android/exoplayer2/ExoPlayerTest.java @@ -15,6 +15,8 @@ */ package com.google.android.exoplayer2; +import com.google.android.exoplayer2.Player.DefaultEventListener; +import com.google.android.exoplayer2.Player.EventListener; import com.google.android.exoplayer2.source.ConcatenatingMediaSource; import com.google.android.exoplayer2.source.MediaSource; import com.google.android.exoplayer2.source.TrackGroup; @@ -344,6 +346,39 @@ public final class ExoPlayerTest extends TestCase { assertEquals(Player.STATE_BUFFERING, (int) playbackStatesWhenSeekProcessed.get(3)); } + public void testSeekProcessedCalledWithIllegalSeekPosition() throws Exception { + ActionSchedule actionSchedule = + new ActionSchedule.Builder("testSeekProcessedCalledWithIllegalSeekPosition") + .waitForPlaybackState(Player.STATE_BUFFERING) + // Cause an illegal seek exception by seeking to an invalid position while the media + // source is still being prepared and the player doesn't immediately know it will fail. + // Because the media source prepares immediately, the exception will be thrown when the + // player processed the seek. + .seek(/* windowIndex= */ 100, /* positionMs= */ 0) + .waitForPlaybackState(Player.STATE_IDLE) + .build(); + final boolean[] onSeekProcessedCalled = new boolean[1]; + EventListener listener = + new DefaultEventListener() { + @Override + public void onSeekProcessed() { + onSeekProcessedCalled[0] = true; + } + }; + ExoPlayerTestRunner testRunner = + new ExoPlayerTestRunner.Builder() + .setActionSchedule(actionSchedule) + .setEventListener(listener) + .build(); + try { + testRunner.start().blockUntilActionScheduleFinished(TIMEOUT_MS).blockUntilEnded(TIMEOUT_MS); + fail(); + } catch (ExoPlaybackException e) { + // Expected exception. + } + assertTrue(onSeekProcessedCalled[0]); + } + public void testSeekDiscontinuity() throws Exception { FakeTimeline timeline = new FakeTimeline(1); ActionSchedule actionSchedule = new ActionSchedule.Builder("testSeekDiscontinuity") @@ -808,4 +843,69 @@ public final class ExoPlayerTest extends TestCase { testRunner.assertTimelineChangeReasonsEqual(Player.TIMELINE_CHANGE_REASON_PREPARED); testRunner.assertPositionDiscontinuityReasonsEqual(Player.DISCONTINUITY_REASON_SEEK); } + + public void testReprepareAfterPlaybackError() throws Exception { + Timeline timeline = new FakeTimeline(/* windowCount= */ 1); + ActionSchedule actionSchedule = + new ActionSchedule.Builder("testReprepareAfterPlaybackError") + .waitForPlaybackState(Player.STATE_BUFFERING) + // Cause an internal exception by seeking to an invalid position while the media source + // is still being prepared and the player doesn't immediately know it will fail. + .seek(/* windowIndex= */ 100, /* positionMs= */ 0) + .waitForPlaybackState(Player.STATE_IDLE) + .prepareSource( + new FakeMediaSource(timeline, /* manifest= */ null), + /* resetPosition= */ false, + /* resetState= */ false) + .build(); + ExoPlayerTestRunner testRunner = + new ExoPlayerTestRunner.Builder() + .setTimeline(timeline) + .setActionSchedule(actionSchedule) + .build(); + try { + testRunner.start().blockUntilActionScheduleFinished(TIMEOUT_MS).blockUntilEnded(TIMEOUT_MS); + fail(); + } catch (ExoPlaybackException e) { + // Expected exception. + } + testRunner.assertTimelinesEqual(timeline, timeline); + testRunner.assertTimelineChangeReasonsEqual( + Player.TIMELINE_CHANGE_REASON_PREPARED, Player.TIMELINE_CHANGE_REASON_PREPARED); + } + + public void testPlaybackErrorDuringSourceInfoRefreshStillUpdatesTimeline() throws Exception { + final Timeline timeline = new FakeTimeline(/* windowCount= */ 1); + final FakeMediaSource mediaSource = + new FakeMediaSource(/* timeline= */ null, /* manifest= */ null); + ActionSchedule actionSchedule = + new ActionSchedule.Builder("testPlaybackErrorDuringSourceInfoRefreshStillUpdatesTimeline") + .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( + new Runnable() { + @Override + public void run() { + mediaSource.setNewSourceInfo(timeline, /* manifest= */ null); + } + }) + .waitForPlaybackState(Player.STATE_IDLE) + .build(); + ExoPlayerTestRunner testRunner = + new ExoPlayerTestRunner.Builder() + .setMediaSource(mediaSource) + .setActionSchedule(actionSchedule) + .build(); + try { + testRunner.start().blockUntilActionScheduleFinished(TIMEOUT_MS).blockUntilEnded(TIMEOUT_MS); + fail(); + } catch (ExoPlaybackException e) { + // Expected exception. + } + testRunner.assertTimelinesEqual(timeline); + testRunner.assertTimelineChangeReasonsEqual(Player.TIMELINE_CHANGE_REASON_PREPARED); + } } 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 20d75ec1bd..a1fe8c09c5 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 @@ -328,7 +328,7 @@ import java.io.IOException; setSeekParametersInternal((SeekParameters) msg.obj); return true; case MSG_STOP: - stopInternal(/* reset= */ msg.arg1 != 0); + stopInternal(/* reset= */ msg.arg1 != 0, /* acknowledgeStop= */ true); return true; case MSG_RELEASE: releaseInternal(); @@ -353,19 +353,19 @@ import java.io.IOException; } } catch (ExoPlaybackException e) { Log.e(TAG, "Renderer error.", e); + stopInternal(/* reset= */ false, /* acknowledgeStop= */ false); eventHandler.obtainMessage(MSG_ERROR, e).sendToTarget(); - stopInternal(/* reset= */ false); return true; } catch (IOException e) { Log.e(TAG, "Source error.", e); + stopInternal(/* reset= */ false, /* acknowledgeStop= */ false); eventHandler.obtainMessage(MSG_ERROR, ExoPlaybackException.createForSource(e)).sendToTarget(); - stopInternal(/* reset= */ false); return true; } catch (RuntimeException e) { Log.e(TAG, "Internal runtime error.", e); + stopInternal(/* reset= */ false, /* acknowledgeStop= */ false); eventHandler.obtainMessage(MSG_ERROR, ExoPlaybackException.createForUnexpected(e)) .sendToTarget(); - stopInternal(/* reset= */ false); return true; } } @@ -635,49 +635,50 @@ import java.io.IOException; return; } - Pair periodPosition = resolveSeekPosition(seekPosition); - if (periodPosition == null) { - // The seek position was valid for the timeline that it was performed into, but the - // timeline has changed and a suitable seek position could not be resolved in the new one. - setState(Player.STATE_ENDED); - // Reset, but retain the source so that it can still be used should a seek occur. - resetInternal( - /* releaseMediaSource= */ false, /* resetPosition= */ true, /* resetState= */ false); - eventHandler - .obtainMessage(MSG_SEEK_ACK, /* seekAdjusted */ 1, 0, playbackInfo) - .sendToTarget(); - return; - } - boolean seekPositionAdjusted = seekPosition.windowPositionUs == C.TIME_UNSET; - int periodIndex = periodPosition.first; - long periodPositionUs = periodPosition.second; - long contentPositionUs = periodPositionUs; - MediaPeriodId periodId = - mediaPeriodInfoSequence.resolvePeriodPositionForAds(periodIndex, periodPositionUs); - if (periodId.isAd()) { - seekPositionAdjusted = true; - periodPositionUs = 0; - } try { - if (periodId.equals(playbackInfo.periodId)) { - long adjustedPeriodPositionUs = periodPositionUs; - if (playingPeriodHolder != null) { - adjustedPeriodPositionUs = - playingPeriodHolder.mediaPeriod.getAdjustedSeekPositionUs( - adjustedPeriodPositionUs, SeekParameters.DEFAULT); - } - if ((adjustedPeriodPositionUs / 1000) == (playbackInfo.positionUs / 1000)) { - // Seek will be performed to the current position. Do nothing. - periodPositionUs = playbackInfo.positionUs; - return; - } + Pair periodPosition = resolveSeekPosition(seekPosition); + if (periodPosition == null) { + // The seek position was valid for the timeline that it was performed into, but the + // timeline has changed and a suitable seek position could not be resolved in the new one. + setState(Player.STATE_ENDED); + // Reset, but retain the source so that it can still be used should a seek occur. + resetInternal( + /* releaseMediaSource= */ false, /* resetPosition= */ true, /* resetState= */ false); + seekPositionAdjusted = true; + return; + } + + int periodIndex = periodPosition.first; + long periodPositionUs = periodPosition.second; + long contentPositionUs = periodPositionUs; + MediaPeriodId periodId = + mediaPeriodInfoSequence.resolvePeriodPositionForAds(periodIndex, periodPositionUs); + if (periodId.isAd()) { + seekPositionAdjusted = true; + periodPositionUs = 0; + } + try { + if (periodId.equals(playbackInfo.periodId)) { + long adjustedPeriodPositionUs = periodPositionUs; + if (playingPeriodHolder != null) { + adjustedPeriodPositionUs = + playingPeriodHolder.mediaPeriod.getAdjustedSeekPositionUs( + adjustedPeriodPositionUs, SeekParameters.DEFAULT); + } + if ((adjustedPeriodPositionUs / 1000) == (playbackInfo.positionUs / 1000)) { + // Seek will be performed to the current position. Do nothing. + periodPositionUs = playbackInfo.positionUs; + return; + } + } + long newPeriodPositionUs = seekToPeriodPosition(periodId, periodPositionUs); + seekPositionAdjusted |= periodPositionUs != newPeriodPositionUs; + periodPositionUs = newPeriodPositionUs; + } finally { + playbackInfo = playbackInfo.fromNewPosition(periodId, periodPositionUs, contentPositionUs); } - long newPeriodPositionUs = seekToPeriodPosition(periodId, periodPositionUs); - seekPositionAdjusted |= periodPositionUs != newPeriodPositionUs; - periodPositionUs = newPeriodPositionUs; } finally { - playbackInfo = playbackInfo.fromNewPosition(periodId, periodPositionUs, contentPositionUs); eventHandler.obtainMessage(MSG_SEEK_ACK, seekPositionAdjusted ? 1 : 0, 0, playbackInfo) .sendToTarget(); } @@ -775,12 +776,10 @@ import java.io.IOException; this.seekParameters = seekParameters; } - private void stopInternal(boolean reset) { + private void stopInternal(boolean reset, boolean acknowledgeStop) { resetInternal( /* releaseMediaSource= */ true, /* resetPosition= */ reset, /* resetState= */ reset); - int prepareOrStopAcks = pendingPrepareCount + 1; - pendingPrepareCount = 0; - notifySourceInfoRefresh(prepareOrStopAcks, playbackInfo); + notifySourceInfoRefresh(acknowledgeStop); loadControl.onStopped(); setState(Player.STATE_IDLE); } @@ -1011,15 +1010,13 @@ import java.io.IOException; playbackInfo = playbackInfo.copyWithTimeline(timeline, manifest); if (oldTimeline == null) { - int processedPrepareAcks = pendingPrepareCount; - pendingPrepareCount = 0; if (pendingInitialSeekPosition != null) { Pair periodPosition = resolveSeekPosition(pendingInitialSeekPosition); pendingInitialSeekPosition = null; if (periodPosition == null) { // The seek position was valid for the timeline that it was performed into, but the // timeline has changed and a suitable seek position could not be resolved in the new one. - handleSourceInfoRefreshEndedPlayback(processedPrepareAcks); + handleSourceInfoRefreshEndedPlayback(); } else { int periodIndex = periodPosition.first; long positionUs = periodPosition.second; @@ -1027,11 +1024,11 @@ import java.io.IOException; mediaPeriodInfoSequence.resolvePeriodPositionForAds(periodIndex, positionUs); playbackInfo = playbackInfo.fromNewPosition(periodId, periodId.isAd() ? 0 : positionUs, positionUs); - notifySourceInfoRefresh(processedPrepareAcks); + notifySourceInfoRefresh(); } } else if (playbackInfo.startPositionUs == C.TIME_UNSET) { if (timeline.isEmpty()) { - handleSourceInfoRefreshEndedPlayback(processedPrepareAcks); + handleSourceInfoRefreshEndedPlayback(); } else { Pair defaultPosition = getPeriodPosition(timeline, timeline.getFirstWindowIndex(shuffleModeEnabled), C.TIME_UNSET); @@ -1041,10 +1038,10 @@ import java.io.IOException; startPositionUs); playbackInfo = playbackInfo.fromNewPosition(periodId, periodId.isAd() ? 0 : startPositionUs, startPositionUs); - notifySourceInfoRefresh(processedPrepareAcks); + notifySourceInfoRefresh(); } } else { - notifySourceInfoRefresh(processedPrepareAcks); + notifySourceInfoRefresh(); } return; } @@ -1171,26 +1168,20 @@ import java.io.IOException; } private void handleSourceInfoRefreshEndedPlayback() { - handleSourceInfoRefreshEndedPlayback(0); - } - - private void handleSourceInfoRefreshEndedPlayback(int prepareAcks) { setState(Player.STATE_ENDED); // Reset, but retain the source so that it can still be used should a seek occur. resetInternal( /* releaseMediaSource= */ false, /* resetPosition= */ true, /* resetState= */ false); - notifySourceInfoRefresh(prepareAcks, playbackInfo); + notifySourceInfoRefresh(); } private void notifySourceInfoRefresh() { - notifySourceInfoRefresh(0); + notifySourceInfoRefresh(/* acknowledgeStop= */ false); } - private void notifySourceInfoRefresh(int prepareOrStopAcks) { - notifySourceInfoRefresh(prepareOrStopAcks, playbackInfo); - } - - private void notifySourceInfoRefresh(int prepareOrStopAcks, PlaybackInfo playbackInfo) { + private void notifySourceInfoRefresh(boolean acknowledgeStop) { + int prepareOrStopAcks = pendingPrepareCount + (acknowledgeStop ? 1 : 0); + pendingPrepareCount = 0; eventHandler.obtainMessage(MSG_SOURCE_INFO_REFRESHED, prepareOrStopAcks, 0, playbackInfo) .sendToTarget(); } diff --git a/testutils/src/main/java/com/google/android/exoplayer2/testutil/ExoPlayerTestRunner.java b/testutils/src/main/java/com/google/android/exoplayer2/testutil/ExoPlayerTestRunner.java index fddeb60bf0..4905fc2233 100644 --- a/testutils/src/main/java/com/google/android/exoplayer2/testutil/ExoPlayerTestRunner.java +++ b/testutils/src/main/java/com/google/android/exoplayer2/testutil/ExoPlayerTestRunner.java @@ -478,9 +478,8 @@ public final class ExoPlayerTestRunner extends Player.DefaultEventListener } /** - * Blocks the current thread until the action schedule finished. Also returns when an - * {@link ExoPlaybackException} is thrown. This does not release the test runner and the test must - * still call {@link #blockUntilEnded(long)}. + * Blocks the current thread until the action schedule finished. This does not release the test + * runner and the test must still call {@link #blockUntilEnded(long)}. * * @param timeoutMs The maximum time to wait for the action schedule to finish. * @return This test runner. @@ -611,7 +610,6 @@ public final class ExoPlayerTestRunner extends Player.DefaultEventListener while (endedCountDownLatch.getCount() > 0) { endedCountDownLatch.countDown(); } - actionScheduleFinishedCountDownLatch.countDown(); } // Player.EventListener