From ce2e2797cb0a91496c27ee8650a90c064c05e935 Mon Sep 17 00:00:00 2001 From: tonihei Date: Tue, 30 Jul 2019 12:47:31 +0100 Subject: [PATCH] Improve DefaultMediaClock behaviour. DefaultMediaClock has currently two non-ideal behaviours: 1. One part of checking if it should use the renderer clock is checking whether the associated renderer finished reading its stream. This only makes sense if the renderer isn't already reading ahead into the next period. This can be solved by forwarding if we are reading ahead to the sync command. 2. When switching from stand-alone to renderer clock we assume they are exactly at the same position. This is true in theory, but in practise there may be small differences due to the different natures of these clocks. To prevent jumping backwards in time, we can temporarily stop the stand-alone clock and only switch once the renderer clock reached the same position. PiperOrigin-RevId: 260690468 --- .../android/exoplayer2/DefaultMediaClock.java | 88 ++++++++++++------- .../exoplayer2/ExoPlayerImplInternal.java | 4 +- .../exoplayer2/DefaultMediaClockTest.java | 79 +++++++++++------ 3 files changed, 111 insertions(+), 60 deletions(-) diff --git a/library/core/src/main/java/com/google/android/exoplayer2/DefaultMediaClock.java b/library/core/src/main/java/com/google/android/exoplayer2/DefaultMediaClock.java index 410dffd558..1971a4cefc 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/DefaultMediaClock.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/DefaultMediaClock.java @@ -40,11 +40,13 @@ import com.google.android.exoplayer2.util.StandaloneMediaClock; void onPlaybackParametersChanged(PlaybackParameters newPlaybackParameters); } - private final StandaloneMediaClock standaloneMediaClock; + private final StandaloneMediaClock standaloneClock; private final PlaybackParameterListener listener; @Nullable private Renderer rendererClockSource; @Nullable private MediaClock rendererClock; + private boolean isUsingStandaloneClock; + private boolean standaloneClockIsStarted; /** * Creates a new instance with listener for playback parameter changes and a {@link Clock} to use @@ -56,21 +58,24 @@ import com.google.android.exoplayer2.util.StandaloneMediaClock; */ public DefaultMediaClock(PlaybackParameterListener listener, Clock clock) { this.listener = listener; - this.standaloneMediaClock = new StandaloneMediaClock(clock); + this.standaloneClock = new StandaloneMediaClock(clock); + isUsingStandaloneClock = true; } /** * Starts the standalone fallback clock. */ public void start() { - standaloneMediaClock.start(); + standaloneClockIsStarted = true; + standaloneClock.start(); } /** * Stops the standalone fallback clock. */ public void stop() { - standaloneMediaClock.stop(); + standaloneClockIsStarted = false; + standaloneClock.stop(); } /** @@ -79,7 +84,7 @@ import com.google.android.exoplayer2.util.StandaloneMediaClock; * @param positionUs The position to set in microseconds. */ public void resetPosition(long positionUs) { - standaloneMediaClock.resetPosition(positionUs); + standaloneClock.resetPosition(positionUs); } /** @@ -99,8 +104,7 @@ import com.google.android.exoplayer2.util.StandaloneMediaClock; } this.rendererClock = rendererMediaClock; this.rendererClockSource = renderer; - rendererClock.setPlaybackParameters(standaloneMediaClock.getPlaybackParameters()); - ensureSynced(); + rendererClock.setPlaybackParameters(standaloneClock.getPlaybackParameters()); } } @@ -114,30 +118,25 @@ import com.google.android.exoplayer2.util.StandaloneMediaClock; if (renderer == rendererClockSource) { this.rendererClock = null; this.rendererClockSource = null; + isUsingStandaloneClock = true; } } /** * Syncs internal clock if needed and returns current clock position in microseconds. + * + * @param isReadingAhead Whether the renderers are reading ahead. */ - public long syncAndGetPositionUs() { - if (isUsingRendererClock()) { - ensureSynced(); - return rendererClock.getPositionUs(); - } else { - return standaloneMediaClock.getPositionUs(); - } + public long syncAndGetPositionUs(boolean isReadingAhead) { + syncClocks(isReadingAhead); + return getPositionUs(); } // MediaClock implementation. @Override public long getPositionUs() { - if (isUsingRendererClock()) { - return rendererClock.getPositionUs(); - } else { - return standaloneMediaClock.getPositionUs(); - } + return isUsingStandaloneClock ? standaloneClock.getPositionUs() : rendererClock.getPositionUs(); } @Override @@ -146,32 +145,53 @@ import com.google.android.exoplayer2.util.StandaloneMediaClock; rendererClock.setPlaybackParameters(playbackParameters); playbackParameters = rendererClock.getPlaybackParameters(); } - standaloneMediaClock.setPlaybackParameters(playbackParameters); + standaloneClock.setPlaybackParameters(playbackParameters); } @Override public PlaybackParameters getPlaybackParameters() { - return rendererClock != null ? rendererClock.getPlaybackParameters() - : standaloneMediaClock.getPlaybackParameters(); + return rendererClock != null + ? rendererClock.getPlaybackParameters() + : standaloneClock.getPlaybackParameters(); } - private void ensureSynced() { + private void syncClocks(boolean isReadingAhead) { + if (shouldUseStandaloneClock(isReadingAhead)) { + isUsingStandaloneClock = true; + if (standaloneClockIsStarted) { + standaloneClock.start(); + } + return; + } long rendererClockPositionUs = rendererClock.getPositionUs(); - standaloneMediaClock.resetPosition(rendererClockPositionUs); + if (isUsingStandaloneClock) { + // Ensure enabling the renderer clock doesn't jump backwards in time. + if (rendererClockPositionUs < standaloneClock.getPositionUs()) { + standaloneClock.stop(); + return; + } + isUsingStandaloneClock = false; + if (standaloneClockIsStarted) { + standaloneClock.start(); + } + } + // Continuously sync stand-alone clock to renderer clock so that it can take over if needed. + standaloneClock.resetPosition(rendererClockPositionUs); PlaybackParameters playbackParameters = rendererClock.getPlaybackParameters(); - if (!playbackParameters.equals(standaloneMediaClock.getPlaybackParameters())) { - standaloneMediaClock.setPlaybackParameters(playbackParameters); + if (!playbackParameters.equals(standaloneClock.getPlaybackParameters())) { + standaloneClock.setPlaybackParameters(playbackParameters); listener.onPlaybackParametersChanged(playbackParameters); } } - private boolean isUsingRendererClock() { - // Use the renderer clock if the providing renderer has not ended or needs the next sample - // stream to reenter the ready state. The latter case uses the standalone clock to avoid getting - // stuck if tracks in the current period have uneven durations. - // See: https://github.com/google/ExoPlayer/issues/1874. - return rendererClockSource != null && !rendererClockSource.isEnded() - && (rendererClockSource.isReady() || !rendererClockSource.hasReadStreamToEnd()); + private boolean shouldUseStandaloneClock(boolean isReadingAhead) { + // Use the standalone clock if the clock providing renderer is not set or has ended. Also use + // the standalone clock if the renderer is not ready and we have finished reading the stream or + // are reading ahead to avoid getting stuck if tracks in the current period have uneven + // durations. See: https://github.com/google/ExoPlayer/issues/1874. + return rendererClockSource == null + || rendererClockSource.isEnded() + || (!rendererClockSource.isReady() + && (isReadingAhead || rendererClockSource.hasReadStreamToEnd())); } - } 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 b6317941fb..488d002ab2 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 @@ -535,7 +535,9 @@ import java.util.concurrent.atomic.AtomicBoolean; playbackInfoUpdate.setPositionDiscontinuity(Player.DISCONTINUITY_REASON_INTERNAL); } } else { - rendererPositionUs = mediaClock.syncAndGetPositionUs(); + rendererPositionUs = + mediaClock.syncAndGetPositionUs( + /* isReadingAhead= */ playingPeriodHolder != queue.getReadingPeriod()); periodPositionUs = playingPeriodHolder.toPeriodTime(rendererPositionUs); maybeTriggerPendingMessages(playbackInfo.positionUs, periodPositionUs); playbackInfo.positionUs = periodPositionUs; diff --git a/library/core/src/test/java/com/google/android/exoplayer2/DefaultMediaClockTest.java b/library/core/src/test/java/com/google/android/exoplayer2/DefaultMediaClockTest.java index c42edb32ae..b6e3d7a648 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/DefaultMediaClockTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/DefaultMediaClockTest.java @@ -53,13 +53,14 @@ public class DefaultMediaClockTest { @Test public void standaloneResetPosition_getPositionShouldReturnSameValue() throws Exception { mediaClock.resetPosition(TEST_POSITION_US); - assertThat(mediaClock.syncAndGetPositionUs()).isEqualTo(TEST_POSITION_US); + assertThat(mediaClock.syncAndGetPositionUs(/* isReadingAhead= */ false)) + .isEqualTo(TEST_POSITION_US); } @Test public void standaloneGetAndResetPosition_shouldNotTriggerCallback() throws Exception { mediaClock.resetPosition(TEST_POSITION_US); - mediaClock.syncAndGetPositionUs(); + mediaClock.syncAndGetPositionUs(/* isReadingAhead= */ false); verifyNoMoreInteractions(listener); } @@ -77,7 +78,7 @@ public class DefaultMediaClockTest { @Test public void standaloneStart_shouldStartClock() throws Exception { mediaClock.start(); - assertClockIsRunning(); + assertClockIsRunning(/* isReadingAhead= */ false); } @Test @@ -98,7 +99,7 @@ public class DefaultMediaClockTest { mediaClock.start(); mediaClock.stop(); mediaClock.start(); - assertClockIsRunning(); + assertClockIsRunning(/* isReadingAhead= */ false); } @Test @@ -130,7 +131,7 @@ public class DefaultMediaClockTest { mediaClock.setPlaybackParameters(TEST_PLAYBACK_PARAMETERS); mediaClock.start(); // Asserts that clock is running with speed declared in getPlaybackParameters(). - assertClockIsRunning(); + assertClockIsRunning(/* isReadingAhead= */ false); } @Test @@ -165,6 +166,7 @@ public class DefaultMediaClockTest { FakeMediaClockRenderer mediaClockRenderer = new MediaClockRenderer(TEST_PLAYBACK_PARAMETERS, /* playbackParametersAreMutable= */ false); mediaClock.onRendererEnabled(mediaClockRenderer); + mediaClock.syncAndGetPositionUs(/* isReadingAhead= */ false); verify(listener).onPlaybackParametersChanged(TEST_PLAYBACK_PARAMETERS); } @@ -174,6 +176,7 @@ public class DefaultMediaClockTest { FakeMediaClockRenderer mediaClockRenderer = new MediaClockRenderer(PlaybackParameters.DEFAULT, /* playbackParametersAreMutable= */ false); mediaClock.onRendererEnabled(mediaClockRenderer); + mediaClock.syncAndGetPositionUs(/* isReadingAhead= */ false); verifyNoMoreInteractions(listener); } @@ -183,7 +186,9 @@ public class DefaultMediaClockTest { FakeMediaClockRenderer mediaClockRenderer = new MediaClockRenderer(TEST_PLAYBACK_PARAMETERS, /* playbackParametersAreMutable= */ false); mediaClock.onRendererEnabled(mediaClockRenderer); + mediaClock.syncAndGetPositionUs(/* isReadingAhead= */ false); mediaClock.onRendererDisabled(mediaClockRenderer); + mediaClock.syncAndGetPositionUs(/* isReadingAhead= */ false); assertThat(mediaClock.getPlaybackParameters()).isEqualTo(TEST_PLAYBACK_PARAMETERS); } @@ -193,6 +198,7 @@ public class DefaultMediaClockTest { FakeMediaClockRenderer mediaClockRenderer = new MediaClockRenderer(PlaybackParameters.DEFAULT, /* playbackParametersAreMutable= */ true); mediaClock.onRendererEnabled(mediaClockRenderer); + mediaClock.syncAndGetPositionUs(/* isReadingAhead= */ false); mediaClock.setPlaybackParameters(TEST_PLAYBACK_PARAMETERS); assertThat(mediaClock.getPlaybackParameters()).isEqualTo(TEST_PLAYBACK_PARAMETERS); } @@ -203,6 +209,7 @@ public class DefaultMediaClockTest { FakeMediaClockRenderer mediaClockRenderer = new MediaClockRenderer(PlaybackParameters.DEFAULT, /* playbackParametersAreMutable= */ true); mediaClock.onRendererEnabled(mediaClockRenderer); + mediaClock.syncAndGetPositionUs(/* isReadingAhead= */ false); mediaClock.setPlaybackParameters(TEST_PLAYBACK_PARAMETERS); verifyNoMoreInteractions(listener); } @@ -213,6 +220,7 @@ public class DefaultMediaClockTest { FakeMediaClockRenderer mediaClockRenderer = new MediaClockRenderer(PlaybackParameters.DEFAULT, /* playbackParametersAreMutable= */ false); mediaClock.onRendererEnabled(mediaClockRenderer); + mediaClock.syncAndGetPositionUs(/* isReadingAhead= */ false); mediaClock.setPlaybackParameters(TEST_PLAYBACK_PARAMETERS); assertThat(mediaClock.getPlaybackParameters()).isEqualTo(PlaybackParameters.DEFAULT); } @@ -223,7 +231,8 @@ public class DefaultMediaClockTest { mediaClock.start(); mediaClock.onRendererEnabled(mediaClockRenderer); mediaClockRenderer.positionUs = TEST_POSITION_US; - assertThat(mediaClock.syncAndGetPositionUs()).isEqualTo(TEST_POSITION_US); + assertThat(mediaClock.syncAndGetPositionUs(/* isReadingAhead= */ false)) + .isEqualTo(TEST_POSITION_US); // We're not advancing the renderer media clock. Thus, the clock should appear to be stopped. assertClockIsStopped(); } @@ -235,9 +244,11 @@ public class DefaultMediaClockTest { mediaClock.start(); mediaClock.onRendererEnabled(mediaClockRenderer); mediaClockRenderer.positionUs = TEST_POSITION_US; - assertThat(mediaClock.syncAndGetPositionUs()).isEqualTo(TEST_POSITION_US); + assertThat(mediaClock.syncAndGetPositionUs(/* isReadingAhead= */ false)) + .isEqualTo(TEST_POSITION_US); mediaClock.resetPosition(0); - assertThat(mediaClock.syncAndGetPositionUs()).isEqualTo(TEST_POSITION_US); + assertThat(mediaClock.syncAndGetPositionUs(/* isReadingAhead= */ false)) + .isEqualTo(TEST_POSITION_US); } @Test @@ -246,23 +257,24 @@ public class DefaultMediaClockTest { mediaClock.start(); mediaClock.onRendererEnabled(mediaClockRenderer); mediaClockRenderer.positionUs = TEST_POSITION_US; - mediaClock.syncAndGetPositionUs(); + mediaClock.syncAndGetPositionUs(/* isReadingAhead= */ false); mediaClock.onRendererDisabled(mediaClockRenderer); fakeClock.advanceTime(SLEEP_TIME_MS); - assertThat(mediaClock.syncAndGetPositionUs()) + assertThat(mediaClock.syncAndGetPositionUs(/* isReadingAhead= */ false)) .isEqualTo(TEST_POSITION_US + C.msToUs(SLEEP_TIME_MS)); - assertClockIsRunning(); + assertClockIsRunning(/* isReadingAhead= */ false); } @Test public void getPositionWithPlaybackParameterChange_shouldTriggerCallback() throws ExoPlaybackException { - MediaClockRenderer mediaClockRenderer = new MediaClockRenderer(PlaybackParameters.DEFAULT, - /* playbackParametersAreMutable= */ true); + MediaClockRenderer mediaClockRenderer = + new MediaClockRenderer( + PlaybackParameters.DEFAULT, /* playbackParametersAreMutable= */ true); mediaClock.onRendererEnabled(mediaClockRenderer); // Silently change playback parameters of renderer clock. mediaClockRenderer.playbackParameters = TEST_PLAYBACK_PARAMETERS; - mediaClock.syncAndGetPositionUs(); + mediaClock.syncAndGetPositionUs(/* isReadingAhead= */ false); verify(listener).onPlaybackParametersChanged(TEST_PLAYBACK_PARAMETERS); } @@ -283,7 +295,18 @@ public class DefaultMediaClockTest { /* isEnded= */ false, /* hasReadStreamToEnd= */ true); mediaClock.start(); mediaClock.onRendererEnabled(mediaClockRenderer); - assertClockIsRunning(); + assertClockIsRunning(/* isReadingAhead= */ false); + } + + @Test + public void rendererNotReadyAndReadingAhead_shouldFallbackToStandaloneClock() + throws ExoPlaybackException { + MediaClockRenderer mediaClockRenderer = + new MediaClockRenderer( + /* isReady= */ false, /* isEnded= */ false, /* hasReadStreamToEnd= */ false); + mediaClock.start(); + mediaClock.onRendererEnabled(mediaClockRenderer); + assertClockIsRunning(/* isReadingAhead= */ true); } @Test @@ -293,7 +316,7 @@ public class DefaultMediaClockTest { /* isEnded= */ true, /* hasReadStreamToEnd= */ true); mediaClock.start(); mediaClock.onRendererEnabled(mediaClockRenderer); - assertClockIsRunning(); + assertClockIsRunning(/* isReadingAhead= */ false); } @Test @@ -302,7 +325,8 @@ public class DefaultMediaClockTest { MediaClockRenderer mediaClockRenderer = new MediaClockRenderer(); mediaClockRenderer.positionUs = TEST_POSITION_US; mediaClock.onRendererDisabled(mediaClockRenderer); - assertThat(mediaClock.syncAndGetPositionUs()).isEqualTo(C.msToUs(fakeClock.elapsedRealtime())); + assertThat(mediaClock.syncAndGetPositionUs(/* isReadingAhead= */ false)) + .isEqualTo(C.msToUs(fakeClock.elapsedRealtime())); } @Test @@ -312,7 +336,8 @@ public class DefaultMediaClockTest { mediaClock.onRendererEnabled(mediaClockRenderer); mediaClock.onRendererEnabled(mediaClockRenderer); mediaClockRenderer.positionUs = TEST_POSITION_US; - assertThat(mediaClock.syncAndGetPositionUs()).isEqualTo(TEST_POSITION_US); + assertThat(mediaClock.syncAndGetPositionUs(/* isReadingAhead= */ false)) + .isEqualTo(TEST_POSITION_US); } @Test @@ -328,20 +353,24 @@ public class DefaultMediaClockTest { } catch (ExoPlaybackException e) { // Expected. } - assertThat(mediaClock.syncAndGetPositionUs()).isEqualTo(TEST_POSITION_US); + assertThat(mediaClock.syncAndGetPositionUs(/* isReadingAhead= */ false)) + .isEqualTo(TEST_POSITION_US); } - private void assertClockIsRunning() { - long clockStartUs = mediaClock.syncAndGetPositionUs(); + private void assertClockIsRunning(boolean isReadingAhead) { + long clockStartUs = mediaClock.syncAndGetPositionUs(isReadingAhead); fakeClock.advanceTime(SLEEP_TIME_MS); - assertThat(mediaClock.syncAndGetPositionUs()).isEqualTo(clockStartUs - + mediaClock.getPlaybackParameters().getMediaTimeUsForPlayoutTimeMs(SLEEP_TIME_MS)); + assertThat(mediaClock.syncAndGetPositionUs(isReadingAhead)) + .isEqualTo( + clockStartUs + + mediaClock.getPlaybackParameters().getMediaTimeUsForPlayoutTimeMs(SLEEP_TIME_MS)); } private void assertClockIsStopped() { - long positionAtStartUs = mediaClock.syncAndGetPositionUs(); + long positionAtStartUs = mediaClock.syncAndGetPositionUs(/* isReadingAhead= */ false); fakeClock.advanceTime(SLEEP_TIME_MS); - assertThat(mediaClock.syncAndGetPositionUs()).isEqualTo(positionAtStartUs); + assertThat(mediaClock.syncAndGetPositionUs(/* isReadingAhead= */ false)) + .isEqualTo(positionAtStartUs); } @SuppressWarnings("HidingField")