From 3147a53c470e20351e1a5e2a5f2ee06862213042 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 c45921567a..d2779ced0c 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -12,6 +12,10 @@ * Fix a bug when [depending on ExoPlayer locally](README.md#locally) with a relative path ([#9403](https://github.com/google/ExoPlayer/issues/9403)). + * 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 5e9ff6a65d..3e308e1296 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; @@ -1156,7 +1159,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. @@ -1193,6 +1198,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 69483fe91c..79ee88b144 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 @@ -3660,7 +3660,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() { @@ -5413,7 +5413,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, @@ -5452,7 +5452,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]; @@ -5523,7 +5523,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]; @@ -10570,7 +10570,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));