From aedde2de396995e4354f3110cc37e86b48525892 Mon Sep 17 00:00:00 2001 From: tonihei Date: Mon, 27 Jun 2022 10:34:56 +0100 Subject: [PATCH] Clean up offload state tracking 1. The offloadSchedulingEnabled value doesn't need to be in PlaybackInfo because it's never updated in EPII. 2. The sleepingForOffload value in EPII wasn't updated explicitly (just via the return value of a method). It was also only meant to be enabled while the player is actively playing, but confusingly triggered from a path where the player may theoretically be buffering as well. 3. The offload sleeping (=not scheduling doSomeWork) was interwoven into the actual scheduling code making it slightly hard to follow. This can be improved slightly by keeping the offload sleeping decision and the scheduling separate. PiperOrigin-RevId: 457427293 --- .../android/exoplayer2/ExoPlayerImpl.java | 9 ++-- .../exoplayer2/ExoPlayerImplInternal.java | 40 ++++++---------- .../android/exoplayer2/PlaybackInfo.java | 46 ------------------- .../android/exoplayer2/ExoPlayerTest.java | 42 ++--------------- .../exoplayer2/MediaPeriodQueueTest.java | 1 - .../robolectric/TestPlayerRunHelper.java | 35 -------------- 6 files changed, 23 insertions(+), 150 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 a48a7e33ca..8883eb9864 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 @@ -422,6 +422,9 @@ import java.util.concurrent.TimeoutException; public void experimentalSetOffloadSchedulingEnabled(boolean offloadSchedulingEnabled) { verifyApplicationThread(); internalPlayer.experimentalSetOffloadSchedulingEnabled(offloadSchedulingEnabled); + for (AudioOffloadListener listener : audioOffloadListeners) { + listener.onExperimentalOffloadSchedulingEnabledChanged(offloadSchedulingEnabled); + } } @Override @@ -1951,12 +1954,6 @@ import java.util.concurrent.TimeoutException; updateAvailableCommands(); listeners.flushEvents(); - if (previousPlaybackInfo.offloadSchedulingEnabled != newPlaybackInfo.offloadSchedulingEnabled) { - for (AudioOffloadListener listener : audioOffloadListeners) { - listener.onExperimentalOffloadSchedulingEnabledChanged( - newPlaybackInfo.offloadSchedulingEnabled); - } - } if (previousPlaybackInfo.sleepingForOffload != newPlaybackInfo.sleepingForOffload) { for (AudioOffloadListener listener : audioOffloadListeners) { listener.onExperimentalSleepingForOffloadChanged(newPlaybackInfo.sleepingForOffload); 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 d107b14d59..7677740f5d 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 @@ -809,10 +809,8 @@ import java.util.concurrent.atomic.AtomicBoolean; return; } this.offloadSchedulingEnabled = offloadSchedulingEnabled; - @Player.State int state = playbackInfo.playbackState; - if (offloadSchedulingEnabled || state == Player.STATE_ENDED || state == Player.STATE_IDLE) { - playbackInfo = playbackInfo.copyWithOffloadSchedulingEnabled(offloadSchedulingEnabled); - } else { + if (!offloadSchedulingEnabled && playbackInfo.sleepingForOffload) { + // We need to wake the player up if offload scheduling is disabled and we are sleeping. handler.sendEmptyMessage(MSG_DO_SOME_WORK); } } @@ -1072,22 +1070,24 @@ import java.util.concurrent.atomic.AtomicBoolean; throw new IllegalStateException("Playback stuck buffering and not loading"); } - if (offloadSchedulingEnabled != playbackInfo.offloadSchedulingEnabled) { - playbackInfo = playbackInfo.copyWithOffloadSchedulingEnabled(offloadSchedulingEnabled); - } - - boolean sleepingForOffload = false; - if ((shouldPlayWhenReady() && playbackInfo.playbackState == Player.STATE_READY) - || playbackInfo.playbackState == Player.STATE_BUFFERING) { - sleepingForOffload = !maybeScheduleWakeup(operationStartTimeMs, ACTIVE_INTERVAL_MS); - } else if (enabledRendererCount != 0 && playbackInfo.playbackState != Player.STATE_ENDED) { - scheduleNextWork(operationStartTimeMs, IDLE_INTERVAL_MS); - } + boolean isPlaying = shouldPlayWhenReady() && playbackInfo.playbackState == Player.STATE_READY; + boolean sleepingForOffload = offloadSchedulingEnabled && requestForRendererSleep && isPlaying; if (playbackInfo.sleepingForOffload != sleepingForOffload) { playbackInfo = playbackInfo.copyWithSleepingForOffload(sleepingForOffload); } requestForRendererSleep = false; // A sleep request is only valid for the current doSomeWork. + if (sleepingForOffload || playbackInfo.playbackState == Player.STATE_ENDED) { + // No need to schedule next work. + return; + } else if (isPlaying || playbackInfo.playbackState == Player.STATE_BUFFERING) { + // We are actively playing or waiting for data to be ready. Schedule next work quickly. + scheduleNextWork(operationStartTimeMs, ACTIVE_INTERVAL_MS); + } else if (playbackInfo.playbackState == Player.STATE_READY && enabledRendererCount != 0) { + // We are ready, but not playing. Schedule next work less often to handle non-urgent updates. + scheduleNextWork(operationStartTimeMs, IDLE_INTERVAL_MS); + } + TraceUtil.endSection(); } @@ -1120,15 +1120,6 @@ import java.util.concurrent.atomic.AtomicBoolean; handler.sendEmptyMessageAtTime(MSG_DO_SOME_WORK, thisOperationStartTimeMs + intervalMs); } - private boolean maybeScheduleWakeup(long operationStartTimeMs, long intervalMs) { - if (offloadSchedulingEnabled && requestForRendererSleep) { - return false; - } - - scheduleNextWork(operationStartTimeMs, intervalMs); - return true; - } - private void seekToInternal(SeekPosition seekPosition) throws ExoPlaybackException { playbackInfoUpdate.incrementPendingOperationAcks(/* operationAcks= */ 1); @@ -1459,7 +1450,6 @@ import java.util.concurrent.atomic.AtomicBoolean; /* bufferedPositionUs= */ startPositionUs, /* totalBufferedDurationUs= */ 0, /* positionUs= */ startPositionUs, - offloadSchedulingEnabled, /* sleepingForOffload= */ false); if (releaseMediaSourceList) { mediaSourceList.release(); 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 d8295eb8ec..5a985901d6 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 @@ -70,8 +70,6 @@ import java.util.List; public final @PlaybackSuppressionReason int playbackSuppressionReason; /** The playback parameters. */ public final PlaybackParameters playbackParameters; - /** Whether offload scheduling is enabled for the main player loop. */ - public final boolean offloadSchedulingEnabled; /** Whether the main player loop is sleeping, while using offload scheduling. */ public final boolean sleepingForOffload; @@ -118,7 +116,6 @@ import java.util.List; /* bufferedPositionUs= */ 0, /* totalBufferedDurationUs= */ 0, /* positionUs= */ 0, - /* offloadSchedulingEnabled= */ false, /* sleepingForOffload= */ false); } @@ -141,7 +138,6 @@ import java.util.List; * @param bufferedPositionUs See {@link #bufferedPositionUs}. * @param totalBufferedDurationUs See {@link #totalBufferedDurationUs}. * @param positionUs See {@link #positionUs}. - * @param offloadSchedulingEnabled See {@link #offloadSchedulingEnabled}. * @param sleepingForOffload See {@link #sleepingForOffload}. */ public PlaybackInfo( @@ -162,7 +158,6 @@ import java.util.List; long bufferedPositionUs, long totalBufferedDurationUs, long positionUs, - boolean offloadSchedulingEnabled, boolean sleepingForOffload) { this.timeline = timeline; this.periodId = periodId; @@ -181,7 +176,6 @@ import java.util.List; this.bufferedPositionUs = bufferedPositionUs; this.totalBufferedDurationUs = totalBufferedDurationUs; this.positionUs = positionUs; - this.offloadSchedulingEnabled = offloadSchedulingEnabled; this.sleepingForOffload = sleepingForOffload; } @@ -233,7 +227,6 @@ import java.util.List; bufferedPositionUs, totalBufferedDurationUs, positionUs, - offloadSchedulingEnabled, sleepingForOffload); } @@ -263,7 +256,6 @@ import java.util.List; bufferedPositionUs, totalBufferedDurationUs, positionUs, - offloadSchedulingEnabled, sleepingForOffload); } @@ -293,7 +285,6 @@ import java.util.List; bufferedPositionUs, totalBufferedDurationUs, positionUs, - offloadSchedulingEnabled, sleepingForOffload); } @@ -323,7 +314,6 @@ import java.util.List; bufferedPositionUs, totalBufferedDurationUs, positionUs, - offloadSchedulingEnabled, sleepingForOffload); } @@ -353,7 +343,6 @@ import java.util.List; bufferedPositionUs, totalBufferedDurationUs, positionUs, - offloadSchedulingEnabled, sleepingForOffload); } @@ -383,7 +372,6 @@ import java.util.List; bufferedPositionUs, totalBufferedDurationUs, positionUs, - offloadSchedulingEnabled, sleepingForOffload); } @@ -417,7 +405,6 @@ import java.util.List; bufferedPositionUs, totalBufferedDurationUs, positionUs, - offloadSchedulingEnabled, sleepingForOffload); } @@ -447,38 +434,6 @@ import java.util.List; bufferedPositionUs, totalBufferedDurationUs, positionUs, - offloadSchedulingEnabled, - sleepingForOffload); - } - - /** - * Copies playback info with new offloadSchedulingEnabled. - * - * @param offloadSchedulingEnabled New offloadSchedulingEnabled state. See {@link - * #offloadSchedulingEnabled}. - * @return Copied playback info with new offload scheduling state. - */ - @CheckResult - public PlaybackInfo copyWithOffloadSchedulingEnabled(boolean offloadSchedulingEnabled) { - return new PlaybackInfo( - timeline, - periodId, - requestedContentPositionUs, - discontinuityStartPositionUs, - playbackState, - playbackError, - isLoading, - trackGroups, - trackSelectorResult, - staticMetadata, - loadingMediaPeriodId, - playWhenReady, - playbackSuppressionReason, - playbackParameters, - bufferedPositionUs, - totalBufferedDurationUs, - positionUs, - offloadSchedulingEnabled, sleepingForOffload); } @@ -508,7 +463,6 @@ import java.util.List; bufferedPositionUs, totalBufferedDurationUs, positionUs, - offloadSchedulingEnabled, sleepingForOffload); } } 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 39e9b64631..ca0ec0b6a0 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 @@ -53,7 +53,6 @@ import static com.google.android.exoplayer2.robolectric.TestPlayerRunHelper.play import static com.google.android.exoplayer2.robolectric.TestPlayerRunHelper.runUntilPendingCommandsAreFullyHandled; import static com.google.android.exoplayer2.robolectric.TestPlayerRunHelper.runUntilPlaybackState; import static com.google.android.exoplayer2.robolectric.TestPlayerRunHelper.runUntilPositionDiscontinuity; -import static com.google.android.exoplayer2.robolectric.TestPlayerRunHelper.runUntilReceiveOffloadSchedulingEnabledNewState; import static com.google.android.exoplayer2.robolectric.TestPlayerRunHelper.runUntilSleepingForOffload; import static com.google.android.exoplayer2.robolectric.TestPlayerRunHelper.runUntilTimelineChanged; import static com.google.android.exoplayer2.source.ads.ServerSideAdInsertionUtil.addAdGroupToAdPlaybackState; @@ -9631,47 +9630,16 @@ public final class ExoPlayerTest { } @Test - public void enableOffloadSchedulingWhileIdle_isToggled_isReported() throws Exception { + public void enableOffloadScheduling_isReported() throws Exception { ExoPlayer player = new TestExoPlayerBuilder(context).build(); + ExoPlayer.AudioOffloadListener mockListener = mock(ExoPlayer.AudioOffloadListener.class); + player.addAudioOffloadListener(mockListener); player.experimentalSetOffloadSchedulingEnabled(true); - assertThat(runUntilReceiveOffloadSchedulingEnabledNewState(player)).isTrue(); + verify(mockListener).onExperimentalOffloadSchedulingEnabledChanged(true); player.experimentalSetOffloadSchedulingEnabled(false); - assertThat(runUntilReceiveOffloadSchedulingEnabledNewState(player)).isFalse(); - } - - @Test - public void enableOffloadSchedulingWhilePlaying_isToggled_isReported() throws Exception { - FakeSleepRenderer sleepRenderer = new FakeSleepRenderer(C.TRACK_TYPE_AUDIO).sleepOnNextRender(); - ExoPlayer player = new TestExoPlayerBuilder(context).setRenderers(sleepRenderer).build(); - Timeline timeline = new FakeTimeline(); - player.setMediaSource(new FakeMediaSource(timeline, ExoPlayerTestRunner.AUDIO_FORMAT)); - player.prepare(); - player.play(); - - player.experimentalSetOffloadSchedulingEnabled(true); - assertThat(runUntilReceiveOffloadSchedulingEnabledNewState(player)).isTrue(); - - player.experimentalSetOffloadSchedulingEnabled(false); - assertThat(runUntilReceiveOffloadSchedulingEnabledNewState(player)).isFalse(); - } - - @Test - public void enableOffloadSchedulingWhileSleepingForOffload_isDisabled_isReported() - throws Exception { - FakeSleepRenderer sleepRenderer = new FakeSleepRenderer(C.TRACK_TYPE_AUDIO).sleepOnNextRender(); - ExoPlayer player = new TestExoPlayerBuilder(context).setRenderers(sleepRenderer).build(); - Timeline timeline = new FakeTimeline(); - player.setMediaSource(new FakeMediaSource(timeline, ExoPlayerTestRunner.AUDIO_FORMAT)); - player.experimentalSetOffloadSchedulingEnabled(true); - player.prepare(); - player.play(); - runUntilSleepingForOffload(player, /* expectedSleepForOffload= */ true); - - player.experimentalSetOffloadSchedulingEnabled(false); - - assertThat(runUntilReceiveOffloadSchedulingEnabledNewState(player)).isFalse(); + verify(mockListener).onExperimentalOffloadSchedulingEnabledChanged(false); } @Test diff --git a/library/core/src/test/java/com/google/android/exoplayer2/MediaPeriodQueueTest.java b/library/core/src/test/java/com/google/android/exoplayer2/MediaPeriodQueueTest.java index ced931e544..a09bc2f885 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/MediaPeriodQueueTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/MediaPeriodQueueTest.java @@ -1106,7 +1106,6 @@ public final class MediaPeriodQueueTest { /* bufferedPositionUs= */ 0, /* totalBufferedDurationUs= */ 0, /* positionUs= */ 0, - /* offloadSchedulingEnabled= */ false, /* sleepingForOffload= */ false); } diff --git a/robolectricutils/src/main/java/com/google/android/exoplayer2/robolectric/TestPlayerRunHelper.java b/robolectricutils/src/main/java/com/google/android/exoplayer2/robolectric/TestPlayerRunHelper.java index 58ed22f289..85869c0750 100644 --- a/robolectricutils/src/main/java/com/google/android/exoplayer2/robolectric/TestPlayerRunHelper.java +++ b/robolectricutils/src/main/java/com/google/android/exoplayer2/robolectric/TestPlayerRunHelper.java @@ -182,41 +182,6 @@ public class TestPlayerRunHelper { return checkNotNull(player.getPlayerError()); } - /** - * Runs tasks of the main {@link Looper} until {@link - * ExoPlayer.AudioOffloadListener#onExperimentalOffloadSchedulingEnabledChanged} is called or a - * playback error occurs. - * - *

If a playback error occurs it will be thrown wrapped in an {@link IllegalStateException}. - * - * @param player The {@link Player}. - * @return The new offloadSchedulingEnabled state. - * @throws TimeoutException If the {@link RobolectricUtil#DEFAULT_TIMEOUT_MS default timeout} is - * exceeded. - */ - public static boolean runUntilReceiveOffloadSchedulingEnabledNewState(ExoPlayer player) - throws TimeoutException { - verifyMainTestThread(player); - AtomicReference<@NullableType Boolean> offloadSchedulingEnabledReceiver = - new AtomicReference<>(); - ExoPlayer.AudioOffloadListener listener = - new ExoPlayer.AudioOffloadListener() { - @Override - public void onExperimentalOffloadSchedulingEnabledChanged( - boolean offloadSchedulingEnabled) { - offloadSchedulingEnabledReceiver.set(offloadSchedulingEnabled); - } - }; - player.addAudioOffloadListener(listener); - runMainLooperUntil( - () -> offloadSchedulingEnabledReceiver.get() != null || player.getPlayerError() != null); - player.removeAudioOffloadListener(listener); - if (player.getPlayerError() != null) { - throw new IllegalStateException(player.getPlayerError()); - } - return checkNotNull(offloadSchedulingEnabledReceiver.get()); - } - /** * Runs tasks of the main {@link Looper} until {@link * ExoPlayer.AudioOffloadListener#onExperimentalSleepingForOffloadChanged(boolean)} is called or a