[go: nahoru, domu]
Skip to content

Commit

Permalink
Fix some playback parameter signalling problems.
Browse files Browse the repository at this point in the history 
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#10882

#minor-release

PiperOrigin-RevId: 512658918
  • Loading branch information
@tonihei
tonihei committed Mar 1, 2023
1 parent ffc268e commit d363977
Show file tree
Hide file tree
Showing 2 changed files with 158 additions and 72 deletions.
26 changes: 19 additions & 7 deletions library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImplInternal.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -955,7 +955,7 @@ && shouldUseLivePlaybackSpeedControl(playbackInfo.timeline, playbackInfo.periodI
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,
Expand All @@ -965,6 +965,12 @@ && shouldUseLivePlaybackSpeedControl(playbackInfo.timeline, playbackInfo.periodI
}
}

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) {
Expand Down Expand Up @@ -1351,7 +1357,7 @@ private void resetRendererPosition(long periodPositionUs) throws ExoPlaybackExce

private void setPlaybackParametersInternal(PlaybackParameters playbackParameters)
throws ExoPlaybackException {
mediaClock.setPlaybackParameters(playbackParameters);
setMediaClockPlaybackParameters(playbackParameters);
handlePlaybackParameters(mediaClock.getPlaybackParameters(), /* acknowledgeCommand= */ true);
}

Expand Down Expand Up @@ -1666,7 +1672,7 @@ private void maybeTriggerPendingMessages(long oldPeriodPositionUs, long newPerio
nextPendingMessageIndexHint = nextPendingMessageIndex;
}

private void ensureStopped(Renderer renderer) throws ExoPlaybackException {
private void ensureStopped(Renderer renderer) {
if (renderer.getState() == Renderer.STATE_STARTED) {
renderer.stop();
}
Expand Down Expand Up @@ -1923,14 +1929,20 @@ private void updatePlaybackSpeedSettingsForNewPeriod(
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;
}
Expand Down Expand Up @@ -1979,7 +1991,7 @@ private long getMaxRendererReadPositionUs() {
return maxReadPositionUs;
}

private void updatePeriods() throws ExoPlaybackException, IOException {
private void updatePeriods() throws ExoPlaybackException {
if (playbackInfo.timeline.isEmpty() || !mediaSourceList.isPrepared()) {
// No periods available.
return;
Expand Down Expand Up @@ -2021,7 +2033,7 @@ private void maybeUpdateLoadingPeriod() throws ExoPlaybackException {
}
}

private void maybeUpdateReadingPeriod() {
private void maybeUpdateReadingPeriod() throws ExoPlaybackException {
@Nullable MediaPeriodHolder readingPeriodHolder = queue.getReadingPeriod();
if (readingPeriodHolder == null) {
return;
Expand Down
204 changes: 139 additions & 65 deletions library/core/src/test/java/com/google/android/exoplayer2/ExoPlayerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@
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.test.core.app.ApplicationProvider;
Expand Down Expand Up @@ -123,7 +124,6 @@
import com.google.android.exoplayer2.source.WrappingMediaSource;
import com.google.android.exoplayer2.source.ads.AdPlaybackState;
import com.google.android.exoplayer2.source.ads.ServerSideAdInsertionMediaSource;
import com.google.android.exoplayer2.testutil.Action;
import com.google.android.exoplayer2.testutil.ActionSchedule;
import com.google.android.exoplayer2.testutil.ActionSchedule.PlayerRunnable;
import com.google.android.exoplayer2.testutil.ActionSchedule.PlayerTarget;
Expand All @@ -149,7 +149,6 @@
import com.google.android.exoplayer2.testutil.FakeVideoRenderer;
import com.google.android.exoplayer2.testutil.TestExoPlayerBuilder;
import com.google.android.exoplayer2.text.TextOutput;
import com.google.android.exoplayer2.trackselection.DefaultTrackSelector;
import com.google.android.exoplayer2.upstream.Allocation;
import com.google.android.exoplayer2.upstream.Allocator;
import com.google.android.exoplayer2.upstream.Loader;
Expand Down Expand Up @@ -3842,41 +3841,29 @@ public void adInMovingLiveWindow_keepsContentPosition() throws Exception {
@Test
public void setPlaybackSpeedConsecutivelyNotifiesListenerForEveryChangeOnceAndIsMasked()
throws Exception {
ExoPlayer player = new TestExoPlayerBuilder(context).build();
List<Float> 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<Float> 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);
Expand All @@ -3886,46 +3873,28 @@ public void onPlaybackParametersChanged(PlaybackParameters playbackParameters) {
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<PlaybackParameters> 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(
Expand All @@ -3936,6 +3905,51 @@ public void onPlaybackParametersChanged(PlaybackParameters playbackParameters) {
.inOrder();
}

@Test
public void
setUnsupportedPlaybackSpeedDirectlyFollowedByDisablingTheRendererAndSupportedPlaybackSpeed_keepsCorrectFinalSpeedAndInformsListenersCorrectly()
throws Exception {
ExoPlayer player =
new TestExoPlayerBuilder(context)
.setRenderers(new AudioClockRendererWithoutSpeedChangeSupport())
.build();
List<PlaybackParameters> 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 =
Expand Down Expand Up @@ -10080,8 +10094,10 @@ public void playerIdle_withSetPlaybackSpeed_usesPlaybackParameterSpeedWithPitchU

@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> playbackParameters = new ArrayList<>();
ArrayList<Pair<Float, Float>> speedChanges = new ArrayList<>();
FakeMediaClockRenderer audioRenderer =
new FakeMediaClockRenderer(C.TRACK_TYPE_AUDIO) {
private long positionUs;
Expand Down Expand Up @@ -10109,6 +10125,12 @@ public PlaybackParameters getPlaybackParameters() {
? 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 =
Expand Down Expand Up @@ -10141,7 +10163,7 @@ public PlaybackParameters getPlaybackParameters() {
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),
Expand All @@ -10152,6 +10174,18 @@ public PlaybackParameters getPlaybackParameters() {
/* 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));
Expand Down Expand Up @@ -12523,6 +12557,46 @@ public Loader.LoadErrorAction onLoadError(
}
}

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.
*/
Expand Down

0 comments on commit d363977

Please sign in to comment.