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 0a2ba36024..052b568b7d 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 189efc3258..31bea4d806 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 @@ package com.google.android.exoplayer2; 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 androidx.annotation.Nullable; 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 abstract class Timeline implements Bundleable { } /** - * 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 @@ public abstract class Timeline implements Bundleable { } 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 a497f20b09..93b8311894 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 @@ import java.util.concurrent.atomic.AtomicBoolean; 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 deb4e780e1..fb2913fd70 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 final class ExoPlayerTest { 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 final class ExoPlayerTest { 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 final class ExoPlayerTest { 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 final class ExoPlayerTest { 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 final class ExoPlayerTest { 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 09ac9b6df2..5f5b28262d 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 final class SinglePeriodTimelineTest { 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));