TrackSelection does not jump to live with HlsMediaPeriod #9386

stevemayhew · 2021-09-03T20:27:43Z

This is a fix for issue #9347

Once the MaskingMediaPeriod completes prepare() (when the masked HlsMediaPeriod reports the
first real Timeline with a duration and window start position) and the preparePositionUs is used
by the player in the onPrepared() callback to set the render position it should not use it again.

The bug is a track selection causes a jump to live if you are at position 0
in the timeline, even after playback was started from the live point (the prepare position override).

stevemayhew · 2021-09-03T22:17:42Z

Use Cases

Most origin vendors support live playback with a rolling-buffer in support of Cloud DVR. In these cases the live window represents only the part of what is available for playback with a simple seekTo().

For example, a SOCU (Start Over / Catch UP) operation converts the sliding live playlist into a live EVENT playlist, assuming the program is in progress.

The operations and how they are implemented are:

Initiate SOCU for in-progress Live

Here live playback is restarted playback (prepare) with URL indicating start-time / end-time (end time in this case is in the future). The steps in the player are simple:

	ExoPlayer player = ...
	liveEventSource = // code converts current live URL to SOCU
	player.setMediaSource(liveEventSource, 0);
	player.prepare();

Restart on BLWE

This (obviously) can happen prior to the user switching to SOCU they scrub or trick-play to the start of the live window, position is very possibly negative at the time of the error report.

It is disruptive in this case to jump to live edge. Instead, in the error handler, logically like to do:

	player.seekTo(0);
	player.prepare();

During SOCU this is obviously never possible, even if the Timeline is still dynamic.

Current Flow

The SOCU Prepare Flow Sequence Diagram shows how the player flow works for the SOCU use case above.

The "Create Loading Period" works perfectly, picking up the requested prepare position into the MaskingMediaPeriod

Looking at the "First Source Info Refresh" issue #7975 comes into play, the masked media period (HlsMediaPeriod) in-correctly picks and overrides the requested prepare position. For other use cases the position is correctly set to start at the live edge requested by the playlist.

Next, HLS library completes the "prepare" operation and reports onPrepared() event. One option is to reset the preparePositionOverrideUs to C.TIME_UNSET at this point, as we have already "used" the prepare position.

The "Subsequent Source Info Refresh" flow may occur before or after the "Prepare Completes" flow (definitely possible for non-chunkless prepare). Updating the 'prepare' position might be warranted in these cases. But, of onPrepared() has been signaled there seems no point.

The block of code in MaskingMediaPeriod is what most certainly causes this issue #9347

  @Override
  public long selectTracks(
      ...
    if (preparePositionOverrideUs != C.TIME_UNSET && positionUs == preparePositionUs) {
      positionUs = preparePositionOverrideUs;
      preparePositionOverrideUs = C.TIME_UNSET;
    }

Unless there is a real use case for this (no test case fail with the code removed) then removing it is the most strait forward solution. Otherwise resetting preparePositionOverrideUs after onPrepared() is signaled may also cover it.

I owe unit test cases for this request once we decide on the correct path forward @tonihei and @ojw28 what do you think.

This is a fix for issue google#9347 Once the MaskingMediaPeriod completes `prepare()` (when the masked HlsMediaPeriod reports the first real Timeline with a duration and window start position) and the `preparePositionUs` is used by the player in the `onPrepared()` callback to set the render position it should not use it again. The bug is a track selection causes a jump to live if you are at position 0 in the timeline, even after playback was started from the live point (the prepare position override).

Test case fails without the change to `MaskingMediaPeriod` so it's `selectTracks` method does not force a seek to the override start position.

stevemayhew · 2021-09-08T17:59:20Z

Test cases added. See inline comments

stevemayhew · 2021-09-08T18:02:21Z

library/core/src/main/java/com/google/android/exoplayer2/source/MaskingMediaPeriod.java

-    if (preparePositionOverrideUs != C.TIME_UNSET && positionUs == preparePositionUs) {
-      positionUs = preparePositionOverrideUs;
-      preparePositionOverrideUs = C.TIME_UNSET;
-    }
    return castNonNull(mediaPeriod)


The position change to start at the default position has already happened when the onPrepare() callback is processed by ExoPlayerImplInternal. Removing this causes no existing test cases to fail.

stevemayhew · 2021-09-08T18:05:00Z

library/core/src/main/java/com/google/android/exoplayer2/source/MaskingMediaSource.java

-      if (unpreparedMaskingMediaPeriod != null) {
-        // Reset override in case the duration changed and we need to update our override.
-        setPreparePositionOverrideToUnpreparedMaskingPeriod(
-            unpreparedMaskingMediaPeriod.getPreparePositionOverrideUs());
-      }


This seems remotely possible, perhaps if the timeline is growing when it is a newly started live playback where it has not reached its target window size. @marcbaechinger what do you think? My new test case still passes if we remove this change, however I'd suggest if we leave it that it is covered by a unit test case (right now it is not).

stevemayhew · 2021-09-08T18:05:29Z

library/core/src/test/java/com/google/android/exoplayer2/ExoPlayerTest.java

+        new FakeMediaSource(new FakeTimeline(), ExoPlayerTestRunner.VIDEO_FORMAT, ExoPlayerTestRunner.AUDIO_FORMAT);
+    FakeTrackSelector trackSelector = new FakeTrackSelector();
+    FakeRenderer videoRenderer = new FakeRenderer(C.TRACK_TYPE_VIDEO);
+    FakeRenderer audioRenderer = new FakeRenderer(C.TRACK_TYPE_AUDIO);


Start with a MediaSource with both audio and video

stevemayhew · 2021-09-08T18:06:06Z

library/core/src/test/java/com/google/android/exoplayer2/ExoPlayerTest.java

+        new MediaItem.Builder().setUri(Uri.EMPTY).build();
+    final long windowDefaultStartPositionUs = 5 * C.MICROS_PER_SECOND;
+    final AtomicLong expectedPosition = new AtomicLong(C.TIME_UNSET);
+    final int lastSeekPosition = 0;


Window start override is 5 seconds, the seekTo 0 is what does not get honored.

stevemayhew · 2021-09-08T18:08:09Z

library/core/src/test/java/com/google/android/exoplayer2/ExoPlayerTest.java

+            .playUntilPosition(0, windowDefaultStartPositionUs)
+            .pause()
+            .seekAndWait(lastSeekPosition) // paused at this position, should not change from here
+            .executeRunnable(() -> {
+              DefaultTrackSelector.Parameters parameters = trackSelector.getParameters();
+              trackSelector.setParameters(parameters.buildUpon()
+                  .setMaxAudioBitrate(0)    // Cause track selection change
+                  .build()
+              );
+            })


Playback starts, reaches the default window start and is paused there. Issue a track selection that changes parameters so as to invalidate the current selection.

stevemayhew · 2021-09-08T18:08:57Z

library/core/src/test/java/com/google/android/exoplayer2/ExoPlayerTest.java

+                expectedPosition.set(player.getCurrentPosition());
+              }
+            })


At this point, after track selection position should still be 0, but the removed code would have reset it to the default position (causing this test to fail)

stevemayhew · 2021-09-08T18:10:22Z

@ojw28 this code is in our source tree now, please have someone take a look at the change. It fixes the bug and includes a test case that demonstrates the bug.

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

stevemayhew · 2023-08-07T18:02:01Z

GitHub is not allowing a convert to Draft for this pull request, but it has an issue that an error recovery on playback that attempts to reset to the default playback position for live will not work (as it looks like only the MediaPeriod.selectTracks() commits the pending seek position).

stevemayhew · 2023-09-07T21:16:41Z

The test cases are good, but the fix is not. It breaks other things. Note this bug is related to #7975 (part of the same sequence of logic) but not the same. See pull request #11285 for a much simpler change that does not break existing use cases.

google-cla bot added the cla: yes label Sep 3, 2021

stevemayhew marked this pull request as ready for review September 3, 2021 22:21

stevemayhew mentioned this pull request Sep 7, 2021

Fix to allow Audio timestampMaster causes stream startup delay #8952

Open

stevemayhew added 2 commits September 7, 2021 08:52

Tests trackselection does not seek to default period start

9f9621d

Test case fails without the change to `MaskingMediaPeriod` so it's `selectTracks` method does not force a seek to the override start position.

stevemayhew force-pushed the p-fix-exo-issue-9347 branch from b7a7d94 to 9f9621d Compare September 8, 2021 17:58

stevemayhew commented Sep 8, 2021

View reviewed changes

krocard requested a review from tonihei September 29, 2021 12:46

krocard assigned tonihei Sep 29, 2021

tonihei mentioned this pull request Sep 29, 2021

Start position of 0 always resolves to default position for live playbacks #7975

Closed

tonihei assigned marcbaechinger and unassigned tonihei Sep 29, 2021

stevemayhew marked this pull request as draft August 7, 2023 17:13

stevemayhew closed this Sep 7, 2023

google locked and limited conversation to collaborators Nov 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TrackSelection does not jump to live with HlsMediaPeriod #9386

TrackSelection does not jump to live with HlsMediaPeriod #9386

stevemayhew commented Sep 3, 2021

stevemayhew commented Sep 3, 2021 •

edited

Loading

stevemayhew commented Sep 8, 2021

stevemayhew Sep 8, 2021

stevemayhew Sep 8, 2021

stevemayhew Sep 8, 2021

stevemayhew Sep 8, 2021

stevemayhew Sep 8, 2021

stevemayhew Sep 8, 2021

stevemayhew commented Sep 8, 2021

stevemayhew commented Aug 7, 2023

stevemayhew commented Sep 7, 2023

TrackSelection does not jump to live with HlsMediaPeriod #9386

TrackSelection does not jump to live with HlsMediaPeriod #9386

Conversation

stevemayhew commented Sep 3, 2021

stevemayhew commented Sep 3, 2021 • edited Loading

Use Cases

Initiate SOCU for in-progress Live

Restart on BLWE

Current Flow

stevemayhew commented Sep 8, 2021

stevemayhew Sep 8, 2021

Choose a reason for hiding this comment

stevemayhew Sep 8, 2021

Choose a reason for hiding this comment

stevemayhew Sep 8, 2021

Choose a reason for hiding this comment

stevemayhew Sep 8, 2021

Choose a reason for hiding this comment

stevemayhew Sep 8, 2021

Choose a reason for hiding this comment

stevemayhew Sep 8, 2021

Choose a reason for hiding this comment

stevemayhew commented Sep 8, 2021

stevemayhew commented Aug 7, 2023

stevemayhew commented Sep 7, 2023

stevemayhew commented Sep 3, 2021 •

edited

Loading