Skip to content

Commit

Permalink
Use correct period-window offset for initial prepare position.
Browse files Browse the repository at this point in the history 
MaskingMediaSource needs to resolve the prepare position set for a MaskingPeriod
while the source was still unprepared to the first actual prepare position.

It currently assumes that the period-window offset and the default position is
zero. This assumption is correct when a PlaceholderTimeline is used, but it
may not be true if the real timeline is already known (e.g. when re-preparing
a live stream after a playback error).

Fix this by using the known timeline at the time of the preparation.
Also:
 - Update a test that should have caught this to use lazy re-preparation.
 - Change the demo app code to use the recommended way to restart playback
   after a BehindLiveWindowException.

Issue: #8675
PiperOrigin-RevId: 361604191
  • Loading branch information
@tonihei @icbaker
tonihei authored and icbaker committed Mar 12, 2021
1 parent ea0f72e commit bc9fb86
Show file tree
Hide file tree
Showing 5 changed files with 72 additions and 54 deletions.
3 changes: 3 additions & 0 deletions RELEASENOTES.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@
`ExoPlayer`.
* Reset playback speed when live playback speed control becomes unused
([#8664](https://github.com/google/ExoPlayer/issues/8664)).
* Fix playback position issue when re-preparing playback after a
BehindLiveWindowException
([#8675](https://github.com/google/ExoPlayer/issues/8675)).
* Remove deprecated symbols:
* Remove `Player.DefaultEventListener`. Use `Player.EventListener`
instead.
Expand Down
4 changes: 2 additions & 2 deletions demos/main/src/main/java/com/google/android/exoplayer2/demo/PlayerActivity.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -440,8 +440,8 @@ public void onPlaybackStateChanged(@Player.State int playbackState) {
@Override
public void onPlayerError(@NonNull ExoPlaybackException e) {
if (isBehindLiveWindow(e)) {
clearStartPosition();
initializePlayer();
player.seekToDefaultPosition();
player.prepare();
} else {
updateButtonVisibility();
showControls();
Expand Down
10 changes: 7 additions & 3 deletions library/core/src/main/java/com/google/android/exoplayer2/source/MaskingMediaSource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,13 +178,17 @@ protected void onChildSourceInfoRefreshed(
// anyway.
newTimeline.getWindow(/* windowIndex= */ 0, window);
long windowStartPositionUs = window.getDefaultPositionUs();
Object windowUid = window.uid;
if (unpreparedMaskingMediaPeriod != null) {
long periodPreparePositionUs = unpreparedMaskingMediaPeriod.getPreparePositionUs();
if (periodPreparePositionUs != 0) {
windowStartPositionUs = periodPreparePositionUs;
timeline.getPeriodByUid(unpreparedMaskingMediaPeriod.id.periodUid, period);
long windowPreparePositionUs = period.getPositionInWindowUs() + periodPreparePositionUs;
long oldWindowDefaultPositionUs =
timeline.getWindow(/* windowIndex= */ 0, window).getDefaultPositionUs();
if (windowPreparePositionUs != oldWindowDefaultPositionUs) {
windowStartPositionUs = windowPreparePositionUs;
}

This comment has been minimized.

Sign in to view

Copy link
@stevemayhew

stevemayhew Jul 7, 2021

Contributor

This if test incorrectly fails to override the default window start position with the initial seek if the seek is to position 0. This is because the default position was never set in the masking period and the "unset" value is also 0.

This comment has been minimized.

Sign in to view

Copy link
@stevemayhew

stevemayhew Jul 25, 2023

Contributor

@tonihei I have another similar bug to this, recovering from other live playback errors by using the sequence:

      player.seekToDefaultPosition();
      player.prepare();

I'll keep you in the loop on this, but the test case is simple:

  1. pull lan cable from a streamer box (or stop the AP if wireless)
  2. let buffer run out to 0
  3. attempt to recover from the fatal error with code above (error is ERROR_CODE_IO_NETWORK_CONNECTION_FAILED)

We expect it to honor the default start position in the new timeline, it doesn't the MaskingMediaPeriod.setPreparePositionOverrideToUnpreparedMaskingPeriod() is never called. The MediaSource has the correct position, but that is never fixed up into the EPII until after trackSelection, by code we had to back out to fix this issue #9347

The result is the position chosen is to close to the live edge to start playback.

Anyway, lots of this is collected in bug, #7975 and #9386. I think that would all be fixed if EPII had the correct position at the first source update rather than waiting for the selectTracks() call.

This comment has been minimized.

Sign in to view

Copy link
@tonihei

tonihei Jul 26, 2023

Author Collaborator

Would you mind moving this to a new issue, so that we can better track any improvements and bug fixes? We recently fixed something similar (see androidx/media@56c62d1), so it's worth checking whether your observation is still true on the latest dev state if you haven't done so already.

This comment has been minimized.

Sign in to view

Copy link
@stevemayhew

stevemayhew Jul 26, 2023

Contributor

Sure, a better home might be issue #9347, as our change to fix that issue causes this one.

IMO the real root cause is "fixing up the position" in the implementation of MediaPeriod.selectTracks(). I'll add this to #9347

This comment has been minimized.

Sign in to view

Copy link
@stevemayhew

stevemayhew Jul 26, 2023

Contributor

Actually, looking at your comment, I'll consolidate the discussion in #7975, as most analysis is in that issue. They are all related to how the pre-prepare position is saved and ultimately committed as the final starting playback position.
}
Object windowUid = window.uid;
Pair<Object, Long> periodPosition =
newTimeline.getPeriodPosition(
window, period, /* windowIndex= */ 0, windowStartPositionUs);
Expand Down
86 changes: 38 additions & 48 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 @@ -17,6 +17,7 @@

import static com.google.android.exoplayer2.robolectric.RobolectricUtil.runMainLooperUntil;
import static com.google.android.exoplayer2.robolectric.TestPlayerRunHelper.playUntilStartOfWindow;
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.runUntilReceiveOffloadSchedulingEnabledNewState;
import static com.google.android.exoplayer2.robolectric.TestPlayerRunHelper.runUntilSleepingForOffload;
Expand Down Expand Up @@ -1653,55 +1654,44 @@ public void reprepareAfterPlaybackError() throws Exception {
}

@Test
public void seekAndReprepareAfterPlaybackError() throws Exception {
Timeline timeline = new FakeTimeline();
final long[] positionHolder = new long[2];
ActionSchedule actionSchedule =
new ActionSchedule.Builder(TAG)
.pause()
.waitForPlaybackState(Player.STATE_READY)
.throwPlaybackException(ExoPlaybackException.createForSource(new IOException()))
.waitForPlaybackState(Player.STATE_IDLE)
.seek(/* positionMs= */ 50)
.waitForPendingPlayerCommands()
.executeRunnable(
new PlayerRunnable() {
@Override
public void run(SimpleExoPlayer player) {
positionHolder[0] = player.getCurrentPosition();
}
})
.prepare()
.waitForPlaybackState(Player.STATE_READY)
.executeRunnable(
new PlayerRunnable() {
@Override
public void run(SimpleExoPlayer player) {
positionHolder[1] = player.getCurrentPosition();
}
})
.play()
.build();
ExoPlayerTestRunner testRunner =
new ExoPlayerTestRunner.Builder(context)
.setTimeline(timeline)
.setActionSchedule(actionSchedule)
.build();
public void seekAndReprepareAfterPlaybackError_keepsSeekPositionAndTimeline() throws Exception {
SimpleExoPlayer player = new TestExoPlayerBuilder(context).build();
Player.EventListener mockListener = mock(Player.EventListener.class);
player.addListener(mockListener);
FakeMediaSource fakeMediaSource = new FakeMediaSource();
player.setMediaSource(fakeMediaSource);

assertThrows(
ExoPlaybackException.class,
() ->
testRunner
.start()
.blockUntilActionScheduleFinished(TIMEOUT_MS)
.blockUntilEnded(TIMEOUT_MS));
testRunner.assertTimelinesSame(placeholderTimeline, timeline);
testRunner.assertTimelineChangeReasonsEqual(
Player.TIMELINE_CHANGE_REASON_PLAYLIST_CHANGED,
Player.TIMELINE_CHANGE_REASON_SOURCE_UPDATE);
testRunner.assertPositionDiscontinuityReasonsEqual(Player.DISCONTINUITY_REASON_SEEK);
assertThat(positionHolder[0]).isEqualTo(50);
assertThat(positionHolder[1]).isEqualTo(50);
player.prepare();
runUntilPlaybackState(player, Player.STATE_READY);
player
.createMessage(
(type, payload) -> {
throw ExoPlaybackException.createForSource(new IOException());
})
.send();
runUntilPlaybackState(player, Player.STATE_IDLE);
player.seekTo(/* positionMs= */ 50);
runUntilPendingCommandsAreFullyHandled(player);
long positionAfterSeekHandled = player.getCurrentPosition();
// Delay re-preparation to force player to use its masking mechanisms.
fakeMediaSource.setAllowPreparation(false);
player.prepare();
runUntilPendingCommandsAreFullyHandled(player);
long positionAfterReprepareHandled = player.getCurrentPosition();
fakeMediaSource.setAllowPreparation(true);
runUntilPlaybackState(player, Player.STATE_READY);
long positionWhenFullyReadyAfterReprepare = player.getCurrentPosition();
player.release();

// Ensure we don't receive further timeline updates when repreparing.
verify(mockListener)
.onTimelineChanged(any(), eq(Player.TIMELINE_CHANGE_REASON_PLAYLIST_CHANGED));
verify(mockListener).onTimelineChanged(any(), eq(Player.TIMELINE_CHANGE_REASON_SOURCE_UPDATE));
verify(mockListener, times(2)).onTimelineChanged(any(), anyInt());

assertThat(positionAfterSeekHandled).isEqualTo(50);
assertThat(positionAfterReprepareHandled).isEqualTo(50);
assertThat(positionWhenFullyReadyAfterReprepare).isEqualTo(50);
}

@Test
Expand Down
23 changes: 22 additions & 1 deletion testutils/src/main/java/com/google/android/exoplayer2/testutil/FakeMediaSource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/
package com.google.android.exoplayer2.testutil;

import static com.google.android.exoplayer2.util.Assertions.checkState;
import static com.google.android.exoplayer2.util.Util.castNonNull;
import static com.google.common.truth.Truth.assertThat;

Expand Down Expand Up @@ -86,6 +87,7 @@ public Window getWindow(int windowIndex, Window window, long defaultPositionProj
private final ArrayList<MediaPeriodId> createdMediaPeriods;
private final DrmSessionManager drmSessionManager;

private boolean preparationAllowed;
private @MonotonicNonNull Timeline timeline;
private boolean preparedSource;
private boolean releasedSource;
Expand Down Expand Up @@ -154,6 +156,22 @@ public FakeMediaSource(
this.createdMediaPeriods = new ArrayList<>();
this.drmSessionManager = drmSessionManager;
this.trackDataFactory = trackDataFactory;
preparationAllowed = true;
}

/**
* Sets whether the next call to {@link #prepareSource} is allowed to finish. If not allowed, a
* later call to this method with {@code allowPreparation} set to true will finish the
* preparation.
*
* @param allowPreparation Whether preparation is allowed to finish.
*/
public synchronized void setAllowPreparation(boolean allowPreparation) {
preparationAllowed = allowPreparation;
if (allowPreparation && sourceInfoRefreshHandler != null) {
sourceInfoRefreshHandler.post(
() -> finishSourcePreparation(/* sendManifestLoadEvents= */ true));
}
}

@Nullable
Expand Down Expand Up @@ -204,7 +222,7 @@ public synchronized void prepareSourceInternal(@Nullable TransferListener mediaT
preparedSource = true;
releasedSource = false;
sourceInfoRefreshHandler = Util.createHandlerForCurrentLooper();
if (timeline != null) {
if (preparationAllowed && timeline != null) {
finishSourcePreparation(/* sendManifestLoadEvents= */ true);
}
}
Expand Down Expand Up @@ -273,11 +291,14 @@ public void setNewSourceInfo(Timeline newTimeline) {
* Sets a new timeline. If the source is already prepared, this triggers a source info refresh
* message being sent to the listener.
*
* <p>Must only be called if preparation is {@link #setAllowPreparation(boolean) allowed}.
*
* @param newTimeline The new {@link Timeline}.
* @param sendManifestLoadEvents Whether to treat this as a manifest refresh and send manifest
* load events to listeners.
*/
public synchronized void setNewSourceInfo(Timeline newTimeline, boolean sendManifestLoadEvents) {
checkState(preparationAllowed);
if (sourceInfoRefreshHandler != null) {
sourceInfoRefreshHandler.post(
() -> {
Expand Down

0 comments on commit bc9fb86

Please sign in to comment.