From ca2105d50ce3f09c8e43e2fce7a1fd25cdfe56c1 Mon Sep 17 00:00:00 2001 From: tonihei Date: Fri, 17 Apr 2020 12:49:01 +0100 Subject: [PATCH] Wait with throwing stuck buffering error until pending load is finished We currently check for shouldContinueLoading, which is the intention to load, not playbackInfo.isLoading, which is the actual loading state, when detecting stuck buffering issues. They only differ if the LoadControl said stop loading (based on nextLoadPosition), but there is still a load in flight (updating bufferedPosition). This may cause the exception to be thrown in edge cases that are only temporary. PiperOrigin-RevId: 307022608 --- .../exoplayer2/ExoPlayerImplInternal.java | 2 +- .../android/exoplayer2/SimpleExoPlayer.java | 17 +++++ .../android/exoplayer2/ExoPlayerTest.java | 73 +++++++++++++++++++ .../exoplayer2/testutil/TestExoPlayer.java | 33 +++++++++ 4 files changed, 124 insertions(+), 1 deletion(-) 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 56cce6d984..02e271e7a4 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 @@ -870,7 +870,7 @@ import java.util.concurrent.atomic.AtomicBoolean; } } if (throwWhenStuckBuffering - && !shouldContinueLoading + && !playbackInfo.isLoading && playbackInfo.totalBufferedDurationUs < 500_000 && isLoadingPossible()) { // Throw if the LoadControl prevents loading even if the buffer is empty or almost empty. We diff --git a/library/core/src/main/java/com/google/android/exoplayer2/SimpleExoPlayer.java b/library/core/src/main/java/com/google/android/exoplayer2/SimpleExoPlayer.java index 67c4b88899..bddfa65b06 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/SimpleExoPlayer.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/SimpleExoPlayer.java @@ -100,6 +100,7 @@ public class SimpleExoPlayer extends BasePlayer private AnalyticsCollector analyticsCollector; private Looper looper; private boolean useLazyPreparation; + private boolean throwWhenStuckBuffering; private boolean buildCalled; /** @@ -294,6 +295,19 @@ public class SimpleExoPlayer extends BasePlayer return this; } + /** + * Sets whether the player should throw when it detects it's stuck buffering. + * + *

This method is experimental, and will be renamed or removed in a future release. + * + * @param throwWhenStuckBuffering Whether to throw when the player detects it's stuck buffering. + * @return This builder. + */ + public Builder experimental_setThrowWhenStuckBuffering(boolean throwWhenStuckBuffering) { + this.throwWhenStuckBuffering = throwWhenStuckBuffering; + return this; + } + /** * Sets the {@link Clock} that will be used by the player. Should only be set for testing * purposes. @@ -384,6 +398,9 @@ public class SimpleExoPlayer extends BasePlayer builder.useLazyPreparation, builder.clock, builder.looper); + if (builder.throwWhenStuckBuffering) { + player.experimental_throwWhenStuckBuffering(); + } } /** 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 b4101dca64..eb75c42ef0 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 @@ -67,6 +67,7 @@ import com.google.android.exoplayer2.testutil.FakeTimeline; import com.google.android.exoplayer2.testutil.FakeTimeline.TimelineWindowDefinition; import com.google.android.exoplayer2.testutil.FakeTrackSelection; import com.google.android.exoplayer2.testutil.FakeTrackSelector; +import com.google.android.exoplayer2.testutil.TestExoPlayer; import com.google.android.exoplayer2.trackselection.TrackSelection; import com.google.android.exoplayer2.trackselection.TrackSelectionArray; import com.google.android.exoplayer2.upstream.Allocation; @@ -3539,6 +3540,78 @@ public final class ExoPlayerTest { assertThat(exception.getUnexpectedException()).isInstanceOf(IllegalStateException.class); } + @Test + public void + nextLoadPositionExceedingLoadControlMaxBuffer_whileCurrentLoadInProgress_doesNotThrowException() { + long maxBufferUs = 2 * C.MICROS_PER_SECOND; + LoadControl loadControlWithMaxBufferUs = + new DefaultLoadControl() { + @Override + public boolean shouldContinueLoading(long bufferedDurationUs, float playbackSpeed) { + return bufferedDurationUs < maxBufferUs; + } + + @Override + public boolean shouldStartPlayback( + long bufferedDurationUs, float playbackSpeed, boolean rebuffering) { + return true; + } + }; + MediaSource mediaSourceWithLoadInProgress = + new FakeMediaSource( + new FakeTimeline(/* windowCount= */ 1), ExoPlayerTestRunner.VIDEO_FORMAT) { + @Override + protected FakeMediaPeriod createFakeMediaPeriod( + MediaPeriodId id, + TrackGroupArray trackGroupArray, + Allocator allocator, + EventDispatcher eventDispatcher, + @Nullable TransferListener transferListener) { + return new FakeMediaPeriod(trackGroupArray, eventDispatcher) { + @Override + public long getBufferedPositionUs() { + // Pretend not to have buffered data yet. + return 0; + } + + @Override + public long getNextLoadPositionUs() { + // Set next load position beyond the maxBufferUs configured in the LoadControl. + return Long.MAX_VALUE; + } + + @Override + public boolean isLoading() { + return true; + } + }; + } + }; + FakeRenderer rendererWaitingForData = + new FakeRenderer(C.TRACK_TYPE_VIDEO) { + @Override + public boolean isReady() { + return false; + } + }; + + SimpleExoPlayer player = + new TestExoPlayer.Builder(context) + .setRenderers(rendererWaitingForData) + .setLoadControl(loadControlWithMaxBufferUs) + .experimental_setThrowWhenStuckBuffering(true) + .build(); + player.setMediaSource(mediaSourceWithLoadInProgress); + player.prepare(); + + // Wait until the MediaSource is prepared, i.e. returned its timeline, and at least one + // iteration of doSomeWork after this was run. + TestExoPlayer.runUntilTimelineChanged(player, /* expectedTimeline= */ null); + TestExoPlayer.runUntilPendingCommandsAreFullyHandled(player); + + assertThat(player.getPlayerError()).isNull(); + } + @Test public void loadControlNeverWantsToPlay_playbackDoesNotGetStuck() throws Exception { LoadControl neverLoadingOrPlayingLoadControl = diff --git a/testutils/src/main/java/com/google/android/exoplayer2/testutil/TestExoPlayer.java b/testutils/src/main/java/com/google/android/exoplayer2/testutil/TestExoPlayer.java index ad36635b92..4b4d8892ba 100644 --- a/testutils/src/main/java/com/google/android/exoplayer2/testutil/TestExoPlayer.java +++ b/testutils/src/main/java/com/google/android/exoplayer2/testutil/TestExoPlayer.java @@ -36,6 +36,7 @@ import com.google.android.exoplayer2.upstream.DefaultBandwidthMeter; import com.google.android.exoplayer2.util.Assertions; import com.google.android.exoplayer2.util.Clock; import com.google.android.exoplayer2.util.Supplier; +import com.google.android.exoplayer2.util.Util; import com.google.android.exoplayer2.video.VideoListener; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; @@ -78,6 +79,7 @@ public class TestExoPlayer { @Nullable private Renderer[] renderers; @Nullable private RenderersFactory renderersFactory; private boolean useLazyPreparation; + private boolean throwWhenStuckBuffering; private @MonotonicNonNull Looper looper; public Builder(Context context) { @@ -247,6 +249,19 @@ public class TestExoPlayer { return looper; } + /** + * Sets whether the player should throw when it detects it's stuck buffering. + * + *

This method is experimental, and will be renamed or removed in a future release. + * + * @param throwWhenStuckBuffering Whether to throw when the player detects it's stuck buffering. + * @return This builder. + */ + public Builder experimental_setThrowWhenStuckBuffering(boolean throwWhenStuckBuffering) { + this.throwWhenStuckBuffering = throwWhenStuckBuffering; + return this; + } + /** * Builds an {@link SimpleExoPlayer} using the provided values or their defaults. * @@ -281,6 +296,7 @@ public class TestExoPlayer { .setClock(clock) .setUseLazyPreparation(useLazyPreparation) .setLooper(looper) + .experimental_setThrowWhenStuckBuffering(throwWhenStuckBuffering) .build(); } } @@ -436,6 +452,23 @@ public class TestExoPlayer { runUntil(() -> receivedCallback.get()); } + /** + * Runs tasks of the main {@link Looper} until the {@code player} handled all previously issued + * commands completely on the internal playback thread. + */ + public static void runUntilPendingCommandsAreFullyHandled(SimpleExoPlayer player) { + verifyMainTestThread(player); + // Send message to player that will arrive after all other pending commands. Thus, the message + // execution on the app thread will also happen after all other pending command + // acknowledgements have arrived back on the app thread. + AtomicBoolean receivedMessageCallback = new AtomicBoolean(false); + player + .createMessage((type, data) -> receivedMessageCallback.set(true)) + .setHandler(Util.createHandler()) + .send(); + runUntil(() -> receivedMessageCallback.get()); + } + /** Run tasks of the main {@link Looper} until the {@code condition} returns {@code true}. */ public static void runUntil(Supplier condition) { verifyMainTestThread();