From 68ee587e25235d4db89f47d1d192d2b6f34a2419 Mon Sep 17 00:00:00 2001 From: olly Date: Fri, 10 Sep 2021 14:46:32 +0100 Subject: [PATCH] Constrain resolved period positions to be within the period This is a candidate fix for #8906. As mentioned in that issue, negative positions within windows might be (kind of) valid in live streaming scenarios, where the window starts at some non-zero position within the period. However, negative positions within periods are definitely not valid. Neither are positions that exceed the period duration. There was already logic in ExoPlayerImplInternal to prevent a resolved seek position from exceeding the period duration. This fix adds the equivalent constraint for the start of the period. It also moves the application of the constraints into Timeline. This has the advantage that the constraints are applied as part of state masking in ExoPlayerImpl.seekTo, removing any UI flicker where the invalid seek position is temporarily visible. Issue: #8906 PiperOrigin-RevId: 395917413 --- RELEASENOTES.md | 4 ++++ .../com/google/android/exoplayer2/Timeline.java | 16 +++++++++++++++- .../exoplayer2/ExoPlayerImplInternal.java | 15 ++++----------- .../google/android/exoplayer2/ExoPlayerTest.java | 10 +++++----- .../source/SinglePeriodTimelineTest.java | 4 ++-- 5 files changed, 30 insertions(+), 19 deletions(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 0a2ba360245..052b568b7d7 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -24,6 +24,10 @@ `MediaSourceFactory` instances from being re-used by `ExoPlayer` instances with non-overlapping lifecycles ([#9099](https://github.com/google/ExoPlayer/issues/9099)). + * Better handle invalid seek requests. Seeks to positions that are before + the start or after the end of the media are now handled as seeks to the + start and end respectively + ([8906](https://github.com/google/ExoPlayer/issues/8906)). * Extractors: * Support TS packets without PTS flag ([#9294](https://github.com/google/ExoPlayer/issues/9294)). diff --git a/library/common/src/main/java/com/google/android/exoplayer2/Timeline.java b/library/common/src/main/java/com/google/android/exoplayer2/Timeline.java index 189efc3258b..31bea4d8061 100644 --- a/library/common/src/main/java/com/google/android/exoplayer2/Timeline.java +++ b/library/common/src/main/java/com/google/android/exoplayer2/Timeline.java @@ -17,6 +17,8 @@ import static com.google.android.exoplayer2.util.Assertions.checkArgument; import static com.google.android.exoplayer2.util.Assertions.checkState; +import static java.lang.Math.max; +import static java.lang.Math.min; import android.net.Uri; import android.os.Bundle; @@ -28,6 +30,7 @@ import com.google.android.exoplayer2.source.ads.AdPlaybackState; import com.google.android.exoplayer2.util.Assertions; import com.google.android.exoplayer2.util.BundleUtil; +import com.google.android.exoplayer2.util.Log; import com.google.android.exoplayer2.util.Util; import com.google.common.collect.ImmutableList; import java.lang.annotation.Documented; @@ -1158,7 +1161,9 @@ public final Pair getPeriodPosition( } /** - * Converts (windowIndex, windowPositionUs) to the corresponding (periodUid, periodPositionUs). + * Converts {@code (windowIndex, windowPositionUs)} to the corresponding {@code (periodUid, + * periodPositionUs)}. The returned {@code periodPositionUs} is constrained to be non-negative, + * and to be less than the containing period's duration if it is known. * * @param window A {@link Window} that may be overwritten. * @param period A {@link Period} that may be overwritten. @@ -1195,6 +1200,15 @@ && getPeriod(periodIndex + 1, period).positionInWindowUs <= windowPositionUs) { } getPeriod(periodIndex, period, /* setIds= */ true); long periodPositionUs = windowPositionUs - period.positionInWindowUs; + // The period positions must be less than the period duration, if it is known. + if (period.durationUs != C.TIME_UNSET) { + periodPositionUs = min(periodPositionUs, period.durationUs - 1); + } + // Period positions cannot be negative. + periodPositionUs = max(0, periodPositionUs); + if (periodPositionUs == 9) { + Log.e("XXX", "YYY"); + } return Pair.create(Assertions.checkNotNull(period.uid), periodPositionUs); } 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 a497f20b095..93b83118947 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 @@ -1273,17 +1273,10 @@ private long seekToPeriodPosition( if (!newPlayingPeriodHolder.prepared) { newPlayingPeriodHolder.info = newPlayingPeriodHolder.info.copyWithStartPositionUs(periodPositionUs); - } else { - if (newPlayingPeriodHolder.info.durationUs != C.TIME_UNSET - && periodPositionUs >= newPlayingPeriodHolder.info.durationUs) { - // Make sure seek position doesn't exceed period duration. - periodPositionUs = max(0, newPlayingPeriodHolder.info.durationUs - 1); - } - if (newPlayingPeriodHolder.hasEnabledTracks) { - periodPositionUs = newPlayingPeriodHolder.mediaPeriod.seekToUs(periodPositionUs); - newPlayingPeriodHolder.mediaPeriod.discardBuffer( - periodPositionUs - backBufferDurationUs, retainBackBufferFromKeyframe); - } + } else if (newPlayingPeriodHolder.hasEnabledTracks) { + periodPositionUs = newPlayingPeriodHolder.mediaPeriod.seekToUs(periodPositionUs); + newPlayingPeriodHolder.mediaPeriod.discardBuffer( + periodPositionUs - backBufferDurationUs, retainBackBufferFromKeyframe); } resetRendererPosition(periodPositionUs); maybeContinueLoading(); 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 deb4e780e13..fb2913fd70a 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 @@ -3663,7 +3663,7 @@ public void delegatingMediaSourceApproach() throws Exception { Timeline fakeTimeline = new FakeTimeline( new TimelineWindowDefinition( - /* isSeekable= */ true, /* isDynamic= */ false, /* durationUs= */ 10_000)); + /* isSeekable= */ true, /* isDynamic= */ false, /* durationUs= */ 10_000_000)); final ConcatenatingMediaSource underlyingSource = new ConcatenatingMediaSource(); CompositeMediaSource delegatingMediaSource = new CompositeMediaSource() { @@ -5416,7 +5416,7 @@ public void seekToIndexLargerThanNumberOfPlaylistItems() throws Exception { Timeline fakeTimeline = new FakeTimeline( new TimelineWindowDefinition( - /* isSeekable= */ true, /* isDynamic= */ false, /* durationUs= */ 10_000)); + /* isSeekable= */ true, /* isDynamic= */ false, /* durationUs= */ 10_000_000)); ConcatenatingMediaSource concatenatingMediaSource = new ConcatenatingMediaSource( /* isAtomic= */ false, @@ -5455,7 +5455,7 @@ public void seekToIndexWithEmptyMultiWindowMediaSource() throws Exception { Timeline fakeTimeline = new FakeTimeline( new TimelineWindowDefinition( - /* isSeekable= */ true, /* isDynamic= */ false, /* durationUs= */ 10_000)); + /* isSeekable= */ true, /* isDynamic= */ false, /* durationUs= */ 10_000_000)); ConcatenatingMediaSource concatenatingMediaSource = new ConcatenatingMediaSource(/* isAtomic= */ false); int[] currentWindowIndices = new int[2]; @@ -5526,7 +5526,7 @@ public void seekToIndexWithEmptyMultiWindowMediaSource_usesLazyPreparation() thr Timeline fakeTimeline = new FakeTimeline( new TimelineWindowDefinition( - /* isSeekable= */ true, /* isDynamic= */ false, /* durationUs= */ 10_000)); + /* isSeekable= */ true, /* isDynamic= */ false, /* durationUs= */ 10_000_000)); ConcatenatingMediaSource concatenatingMediaSource = new ConcatenatingMediaSource(/* isAtomic= */ false); int[] currentWindowIndices = new int[2]; @@ -10729,7 +10729,7 @@ public void seekForward_pastDuration_seeksToDuration() throws Exception { TestPlayerRunHelper.runUntilPlaybackState(player, Player.STATE_READY); player.seekForward(); - assertThat(player.getCurrentPosition()).isEqualTo(C.DEFAULT_SEEK_FORWARD_INCREMENT_MS / 2); + assertThat(player.getCurrentPosition()).isEqualTo(C.DEFAULT_SEEK_FORWARD_INCREMENT_MS / 2 - 1); player.release(); } diff --git a/library/core/src/test/java/com/google/android/exoplayer2/source/SinglePeriodTimelineTest.java b/library/core/src/test/java/com/google/android/exoplayer2/source/SinglePeriodTimelineTest.java index 09ac9b6df27..5f5b28262da 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/source/SinglePeriodTimelineTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/source/SinglePeriodTimelineTest.java @@ -79,9 +79,9 @@ public void getPeriodPositionDynamicWindowKnownDuration() { timeline.getPeriodPosition(window, period, 0, C.TIME_UNSET, windowDurationUs + 1); assertThat(position).isNull(); // Should return (0, duration) with a projection equal to window duration. - position = timeline.getPeriodPosition(window, period, 0, C.TIME_UNSET, windowDurationUs); + position = timeline.getPeriodPosition(window, period, 0, C.TIME_UNSET, windowDurationUs - 1); assertThat(position.first).isEqualTo(timeline.getUidOfPeriod(0)); - assertThat(position.second).isEqualTo(windowDurationUs); + assertThat(position.second).isEqualTo(windowDurationUs - 1); // Should return (0, 0) without a position projection. position = timeline.getPeriodPosition(window, period, 0, C.TIME_UNSET, 0); assertThat(position.first).isEqualTo(timeline.getUidOfPeriod(0));