From 4916baabf039d6f9cbcc901aebdc02cfb4ea56be Mon Sep 17 00:00:00 2001 From: tonihei Date: Fri, 9 Feb 2018 05:21:52 -0800 Subject: [PATCH] Fix bug in media period queue update at dynamic timeline changes. If the period uid doesn't match, the update procedure currently doesn't remove the correct periods. This may cause the player to get stuck or to play the wrong periods. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=185129503 --- .../android/exoplayer2/MediaPeriodQueue.java | 15 ++++--- .../DynamicConcatenatingMediaSource.java | 4 +- .../android/exoplayer2/ExoPlayerTest.java | 45 +++++++++++++++++++ .../exoplayer2/testutil/FakeMediaSource.java | 6 +++ 4 files changed, 61 insertions(+), 9 deletions(-) diff --git a/library/core/src/main/java/com/google/android/exoplayer2/MediaPeriodQueue.java b/library/core/src/main/java/com/google/android/exoplayer2/MediaPeriodQueue.java index 4b6ef1807e..208a235777 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/MediaPeriodQueue.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/MediaPeriodQueue.java @@ -291,7 +291,8 @@ import com.google.android.exoplayer2.util.Assertions; /** * Updates media periods in the queue to take into account the latest timeline, and returns * whether the timeline change has been fully handled. If not, it is necessary to seek to the - * current playback position. + * current playback position. The method assumes that the first media period in the queue is still + * consistent with the new timeline. * * @param playingPeriodId The current playing media period identifier. * @param rendererPositionUs The current renderer position in microseconds. @@ -311,6 +312,11 @@ import com.google.android.exoplayer2.util.Assertions; periodHolder.info = getUpdatedMediaPeriodInfo(periodHolder.info, periodIndex); } else { // Check this period holder still follows the previous one, based on the new timeline. + if (periodIndex == C.INDEX_UNSET + || !periodHolder.uid.equals(timeline.getPeriod(periodIndex, period, true).uid)) { + // The holder uid is inconsistent with the new timeline. + return !removeAfter(previousPeriodHolder); + } MediaPeriodInfo periodInfo = getFollowingMediaPeriodInfo(previousPeriodHolder, rendererPositionUs); if (periodInfo == null) { @@ -326,15 +332,10 @@ import com.google.android.exoplayer2.util.Assertions; } if (periodHolder.info.isLastInTimelinePeriod) { - // Move on to the next timeline period, if there is one. + // Move on to the next timeline period index, if there is one. periodIndex = timeline.getNextPeriodIndex( periodIndex, period, window, repeatMode, shuffleModeEnabled); - if (periodIndex == C.INDEX_UNSET - || !periodHolder.uid.equals(timeline.getPeriod(periodIndex, period, true).uid)) { - // The holder is inconsistent with the new timeline. - return previousPeriodHolder == null || !removeAfter(previousPeriodHolder); - } } previousPeriodHolder = periodHolder; diff --git a/library/core/src/main/java/com/google/android/exoplayer2/source/DynamicConcatenatingMediaSource.java b/library/core/src/main/java/com/google/android/exoplayer2/source/DynamicConcatenatingMediaSource.java index 4deb97879c..f52c1bfd0f 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/source/DynamicConcatenatingMediaSource.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/source/DynamicConcatenatingMediaSource.java @@ -780,7 +780,7 @@ public final class DynamicConcatenatingMediaSource extends CompositeMediaSource< @Override public Period getPeriod(int periodIndex, Period period, boolean setIds) { timeline.getPeriod(periodIndex, period, setIds); - if (period.uid == replacedId) { + if (Util.areEqual(period.uid, replacedId)) { period.uid = DUMMY_ID; } return period; @@ -788,7 +788,7 @@ public final class DynamicConcatenatingMediaSource extends CompositeMediaSource< @Override public int getIndexOfPeriod(Object uid) { - return timeline.getIndexOfPeriod(uid == DUMMY_ID ? replacedId : uid); + return timeline.getIndexOfPeriod(DUMMY_ID.equals(uid) ? replacedId : uid); } } diff --git a/library/core/src/test/java/com/google/android/exoplayer2/ExoPlayerTest.java b/library/core/src/test/java/com/google/android/exoplayer2/ExoPlayerTest.java index f10e889390..ec5a8ccfce 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/ExoPlayerTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/ExoPlayerTest.java @@ -24,6 +24,7 @@ import com.google.android.exoplayer2.Player.EventListener; import com.google.android.exoplayer2.Timeline.Window; import com.google.android.exoplayer2.source.ConcatenatingMediaSource; import com.google.android.exoplayer2.source.MediaSource; +import com.google.android.exoplayer2.source.MediaSource.MediaPeriodId; import com.google.android.exoplayer2.source.TrackGroup; import com.google.android.exoplayer2.source.TrackGroupArray; import com.google.android.exoplayer2.source.ads.AdPlaybackState; @@ -1765,6 +1766,50 @@ public final class ExoPlayerTest { .blockUntilEnded(TIMEOUT_MS); } + @Test + public void testTimelineUpdateDropsPrebufferedPeriods() throws Exception { + Timeline timeline1 = + new FakeTimeline( + new TimelineWindowDefinition(/* periodCount= */ 1, /* id= */ 1), + new TimelineWindowDefinition(/* periodCount= */ 1, /* id= */ 2)); + final Timeline timeline2 = + new FakeTimeline( + new TimelineWindowDefinition(/* periodCount= */ 1, /* id= */ 1), + new TimelineWindowDefinition(/* periodCount= */ 1, /* id= */ 3)); + final FakeMediaSource mediaSource = + new FakeMediaSource(timeline1, /* manifest= */ null, Builder.VIDEO_FORMAT); + ActionSchedule actionSchedule = + new ActionSchedule.Builder("testTimelineUpdateDropsPeriods") + .pause() + .waitForPlaybackState(Player.STATE_READY) + // Ensure next period is pre-buffered by playing until end of first period. + .playUntilPosition( + /* windowIndex= */ 0, + /* positionMs= */ C.usToMs(TimelineWindowDefinition.DEFAULT_WINDOW_DURATION_US)) + .executeRunnable( + new Runnable() { + @Override + public void run() { + mediaSource.setNewSourceInfo(timeline2, /* newManifest= */ null); + } + }) + .waitForTimelineChanged(timeline2) + .play() + .build(); + ExoPlayerTestRunner testRunner = + new ExoPlayerTestRunner.Builder() + .setMediaSource(mediaSource) + .setActionSchedule(actionSchedule) + .build() + .start() + .blockUntilEnded(TIMEOUT_MS); + testRunner.assertPlayedPeriodIndices(0, 1); + // Assert that the second period was re-created from the new timeline. + assertThat(mediaSource.getCreatedMediaPeriods()) + .containsExactly(new MediaPeriodId(0), new MediaPeriodId(1), new MediaPeriodId(1)) + .inOrder(); + } + // Internal methods. private static ActionSchedule.Builder addSurfaceSwitch(ActionSchedule.Builder builder) { diff --git a/testutils/src/main/java/com/google/android/exoplayer2/testutil/FakeMediaSource.java b/testutils/src/main/java/com/google/android/exoplayer2/testutil/FakeMediaSource.java index 17613ce519..da81bbb62c 100644 --- a/testutils/src/main/java/com/google/android/exoplayer2/testutil/FakeMediaSource.java +++ b/testutils/src/main/java/com/google/android/exoplayer2/testutil/FakeMediaSource.java @@ -30,6 +30,7 @@ import com.google.android.exoplayer2.upstream.Allocator; import com.google.android.exoplayer2.util.Assertions; import java.io.IOException; import java.util.ArrayList; +import java.util.List; /** * Fake {@link MediaSource} that provides a given timeline. Creating the period will return a @@ -160,6 +161,11 @@ public class FakeMediaSource implements MediaSource { assertThat(createdMediaPeriods).contains(mediaPeriodId); } + /** Returns a list of {@link MediaPeriodId}s, with one element for each created media period. */ + public List getCreatedMediaPeriods() { + return createdMediaPeriods; + } + protected FakeMediaPeriod createFakeMediaPeriod(MediaPeriodId id, TrackGroupArray trackGroupArray, Allocator allocator) { return new FakeMediaPeriod(trackGroupArray);