From e79b47ccff39363543c514937aef517a855994f0 Mon Sep 17 00:00:00 2001 From: tonihei Date: Mon, 27 Feb 2023 18:06:36 +0000 Subject: [PATCH] Fix some playback parameter signalling problems. Playback parameter signalling can be quite complex because (a) the renderer clock often has a delay before it realizes that it doesn't support a previously set speed and (b) the speed set on media clock sometimes intentionally differs from the one surfaced to the user, e.g. during live speed adjustment or when overriding ad playback speed to 1.0f. This change fixes two problems related to this signalling: 1. When resetting the media clock speed at a period transition, we don't currently tell the renderers that this happened. 2. When a delayed speed change update from the media clock is pending and the renderer for this media clock is disabled before the change can be handled, the pending update becomes stale but it still applied later and overrides any other valid speed set in the meantime. Both edge cases are also covered by extended or new player tests. Issue: google/ExoPlayer#10882 #minor-release PiperOrigin-RevId: 512658918 --- RELEASENOTES.md | 3 + .../exoplayer/ExoPlayerImplInternal.java | 26 ++- .../media3/exoplayer/ExoPlayerTest.java | 204 ++++++++++++------ 3 files changed, 161 insertions(+), 72 deletions(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 26e39d18fd..b771a251b4 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -26,6 +26,9 @@ * Encapsulate Opus frames in Ogg packets in direct playbacks (offload). * Fix broken gapless MP3 playback on Samsung devices ([#8594](https://github.com/google/ExoPlayer/issues/8594)). + * Fix bug where playback speeds set immediately after disabling audio may + be overridden by a previous speed change + ([#10882](https://github.com/google/ExoPlayer/issues/10882)). * Video: * Map HEVC HDR10 format to `HEVCProfileMain10HDR10` instead of `HEVCProfileMain10`. 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 fc6bfca39d..ccdc6e74f5 100644 --- a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/ExoPlayerImplInternal.java +++ b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/ExoPlayerImplInternal.java @@ -963,7 +963,7 @@ import java.util.concurrent.atomic.AtomicBoolean; livePlaybackSpeedControl.getAdjustedPlaybackSpeed( getCurrentLiveOffsetUs(), getTotalBufferedDurationUs()); if (mediaClock.getPlaybackParameters().speed != adjustedSpeed) { - mediaClock.setPlaybackParameters(playbackInfo.playbackParameters.withSpeed(adjustedSpeed)); + setMediaClockPlaybackParameters(playbackInfo.playbackParameters.withSpeed(adjustedSpeed)); handlePlaybackParameters( playbackInfo.playbackParameters, /* currentPlaybackSpeed= */ mediaClock.getPlaybackParameters().speed, @@ -973,6 +973,12 @@ import java.util.concurrent.atomic.AtomicBoolean; } } + private void setMediaClockPlaybackParameters(PlaybackParameters playbackParameters) { + // Previously sent speed updates from the media clock now become stale. + handler.removeMessages(MSG_PLAYBACK_PARAMETERS_CHANGED_INTERNAL); + mediaClock.setPlaybackParameters(playbackParameters); + } + private void notifyTrackSelectionRebuffer() { MediaPeriodHolder periodHolder = queue.getPlayingPeriod(); while (periodHolder != null) { @@ -1359,7 +1365,7 @@ import java.util.concurrent.atomic.AtomicBoolean; private void setPlaybackParametersInternal(PlaybackParameters playbackParameters) throws ExoPlaybackException { - mediaClock.setPlaybackParameters(playbackParameters); + setMediaClockPlaybackParameters(playbackParameters); handlePlaybackParameters(mediaClock.getPlaybackParameters(), /* acknowledgeCommand= */ true); } @@ -1674,7 +1680,7 @@ import java.util.concurrent.atomic.AtomicBoolean; nextPendingMessageIndexHint = nextPendingMessageIndex; } - private void ensureStopped(Renderer renderer) throws ExoPlaybackException { + private void ensureStopped(Renderer renderer) { if (renderer.getState() == Renderer.STATE_STARTED) { renderer.stop(); } @@ -1931,14 +1937,20 @@ import java.util.concurrent.atomic.AtomicBoolean; MediaPeriodId newPeriodId, Timeline oldTimeline, MediaPeriodId oldPeriodId, - long positionForTargetOffsetOverrideUs) { + long positionForTargetOffsetOverrideUs) + throws ExoPlaybackException { if (!shouldUseLivePlaybackSpeedControl(newTimeline, newPeriodId)) { // Live playback speed control is unused for the current period, reset speed to user-defined // playback parameters or 1.0 for ad playback. PlaybackParameters targetPlaybackParameters = newPeriodId.isAd() ? PlaybackParameters.DEFAULT : playbackInfo.playbackParameters; if (!mediaClock.getPlaybackParameters().equals(targetPlaybackParameters)) { - mediaClock.setPlaybackParameters(targetPlaybackParameters); + setMediaClockPlaybackParameters(targetPlaybackParameters); + handlePlaybackParameters( + playbackInfo.playbackParameters, + targetPlaybackParameters.speed, + /* updatePlaybackInfo= */ false, + /* acknowledgeCommand= */ false); } return; } @@ -1987,7 +1999,7 @@ import java.util.concurrent.atomic.AtomicBoolean; return maxReadPositionUs; } - private void updatePeriods() throws ExoPlaybackException, IOException { + private void updatePeriods() throws ExoPlaybackException { if (playbackInfo.timeline.isEmpty() || !mediaSourceList.isPrepared()) { // No periods available. return; @@ -2029,7 +2041,7 @@ import java.util.concurrent.atomic.AtomicBoolean; } } - private void maybeUpdateReadingPeriod() { + private void maybeUpdateReadingPeriod() throws ExoPlaybackException { @Nullable MediaPeriodHolder readingPeriodHolder = queue.getReadingPeriod(); if (readingPeriodHolder == null) { return; 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 5d7e3b90a2..7fd92a6538 100644 --- a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/ExoPlayerTest.java +++ b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/ExoPlayerTest.java @@ -91,6 +91,7 @@ import android.media.AudioManager; import android.net.Uri; import android.os.Handler; import android.os.Looper; +import android.util.Pair; import android.view.Surface; import androidx.annotation.Nullable; import androidx.media3.common.AdPlaybackState; @@ -135,14 +136,12 @@ import androidx.media3.exoplayer.source.TrackGroupArray; import androidx.media3.exoplayer.source.WrappingMediaSource; import androidx.media3.exoplayer.source.ads.ServerSideAdInsertionMediaSource; import androidx.media3.exoplayer.text.TextOutput; -import androidx.media3.exoplayer.trackselection.DefaultTrackSelector; import androidx.media3.exoplayer.upstream.Allocation; import androidx.media3.exoplayer.upstream.Allocator; import androidx.media3.exoplayer.upstream.Loader; import androidx.media3.exoplayer.video.VideoRendererEventListener; import androidx.media3.extractor.metadata.id3.BinaryFrame; import androidx.media3.extractor.metadata.id3.TextInformationFrame; -import androidx.media3.test.utils.Action; import androidx.media3.test.utils.ActionSchedule; import androidx.media3.test.utils.ActionSchedule.PlayerRunnable; import androidx.media3.test.utils.ActionSchedule.PlayerTarget; @@ -3851,41 +3850,29 @@ public final class ExoPlayerTest { @Test public void setPlaybackSpeedConsecutivelyNotifiesListenerForEveryChangeOnceAndIsMasked() throws Exception { + ExoPlayer player = new TestExoPlayerBuilder(context).build(); List maskedPlaybackSpeeds = new ArrayList<>(); - Action getPlaybackSpeedAction = - new Action("getPlaybackSpeed", /* description= */ null) { - @Override - protected void doActionImpl( - ExoPlayer player, DefaultTrackSelector trackSelector, @Nullable Surface surface) { - maskedPlaybackSpeeds.add(player.getPlaybackParameters().speed); - } - }; - ActionSchedule actionSchedule = - new ActionSchedule.Builder(TAG) - .pause() - .waitForPlaybackState(Player.STATE_READY) - .setPlaybackParameters(new PlaybackParameters(/* speed= */ 1.1f)) - .apply(getPlaybackSpeedAction) - .setPlaybackParameters(new PlaybackParameters(/* speed= */ 1.2f)) - .apply(getPlaybackSpeedAction) - .setPlaybackParameters(new PlaybackParameters(/* speed= */ 1.3f)) - .apply(getPlaybackSpeedAction) - .play() - .build(); List reportedPlaybackSpeeds = new ArrayList<>(); - Player.Listener listener = + player.addListener( new Player.Listener() { @Override public void onPlaybackParametersChanged(PlaybackParameters playbackParameters) { reportedPlaybackSpeeds.add(playbackParameters.speed); } - }; - new ExoPlayerTestRunner.Builder(context) - .setActionSchedule(actionSchedule) - .setPlayerListener(listener) - .build() - .start() - .blockUntilEnded(TIMEOUT_MS); + }); + player.setMediaSource( + new FakeMediaSource(new FakeTimeline(), ExoPlayerTestRunner.AUDIO_FORMAT)); + player.prepare(); + runUntilPlaybackState(player, Player.STATE_READY); + + player.setPlaybackSpeed(1.1f); + maskedPlaybackSpeeds.add(player.getPlaybackParameters().speed); + player.setPlaybackSpeed(1.2f); + maskedPlaybackSpeeds.add(player.getPlaybackParameters().speed); + player.setPlaybackSpeed(1.3f); + maskedPlaybackSpeeds.add(player.getPlaybackParameters().speed); + runUntilPendingCommandsAreFullyHandled(player); + player.release(); assertThat(reportedPlaybackSpeeds).containsExactly(1.1f, 1.2f, 1.3f).inOrder(); assertThat(maskedPlaybackSpeeds).isEqualTo(reportedPlaybackSpeeds); @@ -3895,46 +3882,28 @@ public final class ExoPlayerTest { public void setUnsupportedPlaybackSpeedConsecutivelyNotifiesListenerForEveryChangeOnceAndResetsOnceHandled() throws Exception { - Renderer renderer = - new FakeMediaClockRenderer(C.TRACK_TYPE_AUDIO) { - @Override - public long getPositionUs() { - return 0; - } - - @Override - public void setPlaybackParameters(PlaybackParameters playbackParameters) {} - - @Override - public PlaybackParameters getPlaybackParameters() { - return PlaybackParameters.DEFAULT; - } - }; - ActionSchedule actionSchedule = - new ActionSchedule.Builder(TAG) - .pause() - .waitForPlaybackState(Player.STATE_READY) - .setPlaybackParameters(new PlaybackParameters(/* speed= */ 1.1f)) - .setPlaybackParameters(new PlaybackParameters(/* speed= */ 1.2f)) - .setPlaybackParameters(new PlaybackParameters(/* speed= */ 1.3f)) - .play() + ExoPlayer player = + new TestExoPlayerBuilder(context) + .setRenderers(new AudioClockRendererWithoutSpeedChangeSupport()) .build(); List reportedPlaybackParameters = new ArrayList<>(); - Player.Listener listener = + player.addListener( new Player.Listener() { @Override public void onPlaybackParametersChanged(PlaybackParameters playbackParameters) { reportedPlaybackParameters.add(playbackParameters); } - }; - new ExoPlayerTestRunner.Builder(context) - .setSupportedFormats(ExoPlayerTestRunner.AUDIO_FORMAT) - .setRenderers(renderer) - .setActionSchedule(actionSchedule) - .setPlayerListener(listener) - .build() - .start() - .blockUntilEnded(TIMEOUT_MS); + }); + player.setMediaSource( + new FakeMediaSource(new FakeTimeline(), ExoPlayerTestRunner.AUDIO_FORMAT)); + player.prepare(); + runUntilPlaybackState(player, Player.STATE_READY); + + player.setPlaybackSpeed(1.1f); + player.setPlaybackSpeed(1.2f); + player.setPlaybackSpeed(1.3f); + runUntilPendingCommandsAreFullyHandled(player); + player.release(); assertThat(reportedPlaybackParameters) .containsExactly( @@ -3945,6 +3914,51 @@ public final class ExoPlayerTest { .inOrder(); } + @Test + public void + setUnsupportedPlaybackSpeedDirectlyFollowedByDisablingTheRendererAndSupportedPlaybackSpeed_keepsCorrectFinalSpeedAndInformsListenersCorrectly() + throws Exception { + ExoPlayer player = + new TestExoPlayerBuilder(context) + .setRenderers(new AudioClockRendererWithoutSpeedChangeSupport()) + .build(); + List reportedPlaybackParameters = new ArrayList<>(); + player.addListener( + new Player.Listener() { + @Override + public void onPlaybackParametersChanged(PlaybackParameters playbackParameters) { + reportedPlaybackParameters.add(playbackParameters); + } + }); + player.setMediaSource( + new FakeMediaSource(new FakeTimeline(), ExoPlayerTestRunner.AUDIO_FORMAT)); + player.prepare(); + runUntilPlaybackState(player, Player.STATE_READY); + + player.setPlaybackSpeed(2f); + // We need to do something that reliably triggers a position sync with the renderer, but no + // further playback progress as we want to test what happens if the parameter reset is still + // pending when we disable the audio renderer below. Calling play and pause will achieve this. + player.play(); + player.pause(); + // Disabling the audio renderer and setting a new speed should work, and should not be affected + // by the still pending parameter reset from above. + player.setTrackSelectionParameters( + player + .getTrackSelectionParameters() + .buildUpon() + .setTrackTypeDisabled(C.TRACK_TYPE_AUDIO, /* disabled= */ true) + .build()); + player.setPlaybackSpeed(5f); + runUntilPendingCommandsAreFullyHandled(player); + player.release(); + + assertThat(reportedPlaybackParameters) + .containsExactly( + new PlaybackParameters(/* speed= */ 2f), new PlaybackParameters(/* speed= */ 5f)) + .inOrder(); + } + @Test public void simplePlaybackHasNoPlaybackSuppression() throws Exception { ActionSchedule actionSchedule = @@ -10089,8 +10103,10 @@ public final class ExoPlayerTest { @Test public void setPlaybackSpeed_withAdPlayback_onlyAppliesToContent() throws Exception { - // Create renderer with media clock to listen to playback parameter changes. + // Create renderer with media clock to listen to playback parameter changes and reported speed + // changes. ArrayList playbackParameters = new ArrayList<>(); + ArrayList> speedChanges = new ArrayList<>(); FakeMediaClockRenderer audioRenderer = new FakeMediaClockRenderer(C.TRACK_TYPE_AUDIO) { private long positionUs; @@ -10118,6 +10134,12 @@ public final class ExoPlayerTest { ? PlaybackParameters.DEFAULT : Iterables.getLast(playbackParameters); } + + @Override + public void setPlaybackSpeed(float currentPlaybackSpeed, float targetPlaybackSpeed) + throws ExoPlaybackException { + speedChanges.add(Pair.create(currentPlaybackSpeed, targetPlaybackSpeed)); + } }; ExoPlayer player = new TestExoPlayerBuilder(context).setRenderers(audioRenderer).build(); AdPlaybackState adPlaybackState = @@ -10150,7 +10172,7 @@ public final class ExoPlayerTest { runUntilPlaybackState(player, Player.STATE_ENDED); player.release(); - // Assert that the renderer received the playback speed updates at each ad/content boundary. + // Assert that the media clock received the playback parameters at each ad/content boundary. assertThat(playbackParameters) .containsExactly( /* preroll ad */ new PlaybackParameters(1f), @@ -10161,6 +10183,18 @@ public final class ExoPlayerTest { /* content after postroll */ new PlaybackParameters(5f)) .inOrder(); + // Assert that the renderer received the speed changes at each ad/content boundary. + assertThat(speedChanges) + .containsExactly( + /* initial setup */ Pair.create(5f, 5f), + /* preroll ad */ Pair.create(1f, 5f), + /* content after preroll */ Pair.create(5f, 5f), + /* midroll ad */ Pair.create(1f, 5f), + /* content after midroll */ Pair.create(5f, 5f), + /* postroll ad */ Pair.create(1f, 5f), + /* content after postroll */ Pair.create(5f, 5f)) + .inOrder(); + // Assert that user-set speed was reported, but none of the ad overrides. verify(mockListener).onPlaybackParametersChanged(any()); verify(mockListener).onPlaybackParametersChanged(new PlaybackParameters(5.0f)); @@ -12532,6 +12566,46 @@ public final class ExoPlayerTest { } } + private static final class AudioClockRendererWithoutSpeedChangeSupport + extends FakeMediaClockRenderer { + + private PlaybackParameters playbackParameters; + private boolean delayingPlaybackParameterReset; + private long positionUs; + + public AudioClockRendererWithoutSpeedChangeSupport() { + super(C.TRACK_TYPE_AUDIO); + playbackParameters = PlaybackParameters.DEFAULT; + } + + @Override + protected void onPositionReset(long positionUs, boolean joining) throws ExoPlaybackException { + super.onPositionReset(positionUs, joining); + this.positionUs = positionUs; + } + + @Override + public long getPositionUs() { + return positionUs; + } + + @Override + public void setPlaybackParameters(PlaybackParameters playbackParameters) { + this.playbackParameters = playbackParameters; + // Similar to a real renderer, the missing speed support is only detected with a delay. + delayingPlaybackParameterReset = true; + } + + @Override + public PlaybackParameters getPlaybackParameters() { + if (delayingPlaybackParameterReset) { + delayingPlaybackParameterReset = false; + return playbackParameters; + } + return PlaybackParameters.DEFAULT; + } + } + /** * Returns an argument matcher for {@link Timeline} instances that ignores period and window uids. */