From 5c2752b4a92a076babc0bc1ff5651a306c20e693 Mon Sep 17 00:00:00 2001 From: tonihei Date: Mon, 27 Jun 2022 09:34:56 +0000 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 --- .../media3/exoplayer/ExoPlayerImpl.java | 9 ++-- .../exoplayer/ExoPlayerImplInternal.java | 40 ++++++---------- .../media3/exoplayer/PlaybackInfo.java | 46 ------------------- .../media3/exoplayer/ExoPlayerTest.java | 42 ++--------------- .../exoplayer/MediaPeriodQueueTest.java | 1 - .../robolectric/TestPlayerRunHelper.java | 35 -------------- 6 files changed, 23 insertions(+), 150 deletions(-) diff --git a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/ExoPlayerImpl.java b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/ExoPlayerImpl.java index a8587da565..de4f574764 100644 --- a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/ExoPlayerImpl.java +++ b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/ExoPlayerImpl.java @@ -433,6 +433,9 @@ import java.util.concurrent.TimeoutException; public void experimentalSetOffloadSchedulingEnabled(boolean offloadSchedulingEnabled) { verifyApplicationThread(); internalPlayer.experimentalSetOffloadSchedulingEnabled(offloadSchedulingEnabled); + for (AudioOffloadListener listener : audioOffloadListeners) { + listener.onExperimentalOffloadSchedulingEnabledChanged(offloadSchedulingEnabled); + } } @Override @@ -1962,12 +1965,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/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/ExoPlayerImplInternal.java b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/ExoPlayerImplInternal.java index 0b37727ecf..a9f0af6111 100644 --- a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/ExoPlayerImplInternal.java +++ b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/ExoPlayerImplInternal.java @@ -817,10 +817,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); } } @@ -1080,22 +1078,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(); } @@ -1128,15 +1128,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); @@ -1467,7 +1458,6 @@ import java.util.concurrent.atomic.AtomicBoolean; /* bufferedPositionUs= */ startPositionUs, /* totalBufferedDurationUs= */ 0, /* positionUs= */ startPositionUs, - offloadSchedulingEnabled, /* sleepingForOffload= */ false); if (releaseMediaSourceList) { mediaSourceList.release(); diff --git a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/PlaybackInfo.java b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/PlaybackInfo.java index f0e104a75a..9ea9b0e971 100644 --- a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/PlaybackInfo.java +++ b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/PlaybackInfo.java @@ -74,8 +74,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; @@ -122,7 +120,6 @@ import java.util.List; /* bufferedPositionUs= */ 0, /* totalBufferedDurationUs= */ 0, /* positionUs= */ 0, - /* offloadSchedulingEnabled= */ false, /* sleepingForOffload= */ false); } @@ -145,7 +142,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( @@ -166,7 +162,6 @@ import java.util.List; long bufferedPositionUs, long totalBufferedDurationUs, long positionUs, - boolean offloadSchedulingEnabled, boolean sleepingForOffload) { this.timeline = timeline; this.periodId = periodId; @@ -185,7 +180,6 @@ import java.util.List; this.bufferedPositionUs = bufferedPositionUs; this.totalBufferedDurationUs = totalBufferedDurationUs; this.positionUs = positionUs; - this.offloadSchedulingEnabled = offloadSchedulingEnabled; this.sleepingForOffload = sleepingForOffload; } @@ -237,7 +231,6 @@ import java.util.List; bufferedPositionUs, totalBufferedDurationUs, positionUs, - offloadSchedulingEnabled, sleepingForOffload); } @@ -267,7 +260,6 @@ import java.util.List; bufferedPositionUs, totalBufferedDurationUs, positionUs, - offloadSchedulingEnabled, sleepingForOffload); } @@ -297,7 +289,6 @@ import java.util.List; bufferedPositionUs, totalBufferedDurationUs, positionUs, - offloadSchedulingEnabled, sleepingForOffload); } @@ -327,7 +318,6 @@ import java.util.List; bufferedPositionUs, totalBufferedDurationUs, positionUs, - offloadSchedulingEnabled, sleepingForOffload); } @@ -357,7 +347,6 @@ import java.util.List; bufferedPositionUs, totalBufferedDurationUs, positionUs, - offloadSchedulingEnabled, sleepingForOffload); } @@ -387,7 +376,6 @@ import java.util.List; bufferedPositionUs, totalBufferedDurationUs, positionUs, - offloadSchedulingEnabled, sleepingForOffload); } @@ -421,7 +409,6 @@ import java.util.List; bufferedPositionUs, totalBufferedDurationUs, positionUs, - offloadSchedulingEnabled, sleepingForOffload); } @@ -451,38 +438,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); } @@ -512,7 +467,6 @@ import java.util.List; bufferedPositionUs, totalBufferedDurationUs, positionUs, - offloadSchedulingEnabled, sleepingForOffload); } } diff --git a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/ExoPlayerTest.java b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/ExoPlayerTest.java index c41f46941a..a6d9d4b6ff 100644 --- a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/ExoPlayerTest.java +++ b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/ExoPlayerTest.java @@ -59,7 +59,6 @@ import static androidx.media3.test.utils.robolectric.TestPlayerRunHelper.playUnt import static androidx.media3.test.utils.robolectric.TestPlayerRunHelper.runUntilPendingCommandsAreFullyHandled; import static androidx.media3.test.utils.robolectric.TestPlayerRunHelper.runUntilPlaybackState; import static androidx.media3.test.utils.robolectric.TestPlayerRunHelper.runUntilPositionDiscontinuity; -import static androidx.media3.test.utils.robolectric.TestPlayerRunHelper.runUntilReceiveOffloadSchedulingEnabledNewState; import static androidx.media3.test.utils.robolectric.TestPlayerRunHelper.runUntilSleepingForOffload; import static androidx.media3.test.utils.robolectric.TestPlayerRunHelper.runUntilTimelineChanged; import static com.google.common.truth.Truth.assertThat; @@ -9641,47 +9640,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/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/MediaPeriodQueueTest.java b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/MediaPeriodQueueTest.java index 155450ec7e..cb059bc241 100644 --- a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/MediaPeriodQueueTest.java +++ b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/MediaPeriodQueueTest.java @@ -1112,7 +1112,6 @@ public final class MediaPeriodQueueTest { /* bufferedPositionUs= */ 0, /* totalBufferedDurationUs= */ 0, /* positionUs= */ 0, - /* offloadSchedulingEnabled= */ false, /* sleepingForOffload= */ false); } diff --git a/libraries/test_utils_robolectric/src/main/java/androidx/media3/test/utils/robolectric/TestPlayerRunHelper.java b/libraries/test_utils_robolectric/src/main/java/androidx/media3/test/utils/robolectric/TestPlayerRunHelper.java index 29c3d09c57..658e9f56be 100644 --- a/libraries/test_utils_robolectric/src/main/java/androidx/media3/test/utils/robolectric/TestPlayerRunHelper.java +++ b/libraries/test_utils_robolectric/src/main/java/androidx/media3/test/utils/robolectric/TestPlayerRunHelper.java @@ -184,41 +184,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