Skip to content

Commit

Permalink
Correct comment on track selection during seek
Browse files Browse the repository at this point in the history 
The comment "all should be same" is not correct,  it is extremely likely they will be the same but not assured.   In either case the seek position will work, it just may not land exactly on a sync point in every variant.
  • Loading branch information
@stevemayhew
stevemayhew committed Nov 5, 2021
1 parent d3bba3b commit 530dd3f
Showing 1 changed file with 7 additions and 2 deletions.
9 changes: 7 additions & 2 deletions library/hls/src/main/java/com/google/android/exoplayer2/source/hls/HlsChunkSource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -257,8 +257,13 @@ public long getAdjustedSeekPositionUs(long positionUs, SeekParameters seekParame
mediaPlaylist = playlistTracker.getPlaylistSnapshot(playlistUrls[selectedIndex], /* isForPlayback= */ true);
}

// Resolve to a segment boundary, current track is fine (all should be same).
// and, segments must start with sync (EXT-X-INDEPENDENT-SEGMENTS must be present)
// If segments must start with sync (EXT-X-INDEPENDENT-SEGMENTS is set) and the playlist is not empty
// resolve using the nearest segment start and the next segment (if any) start as the first and second
// sync points.
// Note, the position returned is normalized to the period, so it will work if a track selection changes
// variants before the seek is executed. It is possible it may not land exactly on a segment sync point in the rare
// case the segment boundaries do not align across variants.
//
if (mediaPlaylist != null && mediaPlaylist.hasIndependentSegments && !mediaPlaylist.segments.isEmpty()) {
long startOfPlaylistInPeriodUs = mediaPlaylist.startTimeUs - playlistTracker.getInitialStartTimeUs();
long targetPositionInPlaylistUs = positionUs - startOfPlaylistInPeriodUs;
Expand Down

10 comments on commit 530dd3f

@stevemayhew
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a small problem with this code still, that I can now reproduce pretty easily (but in practice it is rare). The comment is true, iff the playlist snapshot is very fresh, however that may not be the case for non-EVENT live playback.

The steps to reproduce are:

  1. Track selection picks playlist A and plays it for a while
  2. Track selection switches to playlist B plays it for time X
  3. Track selection switches back to playlist A
  4. seek is issued, before playlist A becomes primary playlist, reloads, and source update signaled (MSG_PLAYLIST_UPDATE_REQUESTED processed)
  5. Exactly the case in the note happens, except the calculated Period relative position (targetPositionInPlaylistUs ) will be off by as much as time X

What happens is the the cached playlist snapshots, while valid (see isSnapshotValid() for loading the next segment, is not even close to up to date with the current Period position. This corrects itself after the playlist becomes primary and a couple of Player thread handler cycles.

I'm looking if a similar situation can happen for DASH Renditions. For HLS there are a couple of options that I've tried that fix the problem:

  1. Add code to this method that does basic sanity check on targetPositionInPlaylistUs (eg it is < durationUs of the selected playlist)
  2. Add a method to playlist tracker (HlsPlaylistTracker.isRefreshedPrimary(Uri url)) that checks the exact condition required (that is playlist is primary and < 1.5 x EXT-X-TARGETDURATION old)
  3. Some other idea I haven't thought of ;-)

@ojw28 or @tonihei what do you think? Now that I better understand how ad insertion and period time works better I hopefully won't make this kind of mistake again

@stevemayhew
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opened a pull request with option 2 above (simpler, just a method to get a fresh HlsMediaPlaylist) #10477

This is the code we are currently testing with (or at least the non re-formatted version from)

@tonihei
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

except the calculated Period relative position (targetPositionInPlaylistUs) will be off by as much as time X

I don't think I can follow why this is the case. It seems the targetPositionInPlaylistUs is correct for the stale playlist, but because the playlist is stale we can't resolve the seek positions correctly.

For example:

  • Live window of 10 seconds
  • Repeatedly loading playlist A until we have a snapshot with startOfPlaylistInPeriodUs=40_000_000 (containing segments up to 50 seconds in period time).
  • Continue loading playlist B until we have a snapshot startOfPlaylistInPeriodUs=80_000_000 (containing segments up to 90 seconds in period time).
  • Switching back to A, but still using the stale playlist snapshot from above.
  • Calling getAdjustedSeekPositionUs with positionUs=85_000_000.
  • targetPositionInPlaylistUs (for the stale playlist snapshot A) is correctly calculated as 45_000_000.
  • Resolving the seek position doesn't work though because 45_000_000 exceeds the 10_000_000 us worth of segments defined in the snapshot. The seek positions are likely resolved to the start time of the last defined segment.

The intended outcome is: Recognize that we can't resolve the position correctly and just return the unaltered input positionUs.

This means option (1) is actually the more direct and better way to solve this? We don't usually expect seeks outside of the defined Timeline.Window duration and this Timeline.Window is set by the primary playlist. So there shouldn't be a seek that goes beyond the duration of the playlist unless the playlist we are using is stale. For the rare case someone issues a seek request that goes beyond the defined playlist, it's fine to accept the potential performance impact.

@stevemayhew
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intended outcome is: Recognize that we can't resolve the position correctly and just return the unaltered input positionUs.

Yes, that is the correct outcome. Note, at most one seek will suffer this as the playlist becomes primary and refreshes in the next cycle.
Your example works with just considering exceeded duration, assuming the logic is correct (which from your comment here on commit #r79541401 seems it is). The we need a call to isSnapshotValid() and that would cover the duration exceeded case.

Visually the two cases are:

  • * — is current playback position in period
  • X — is the resolved position in the old playlist
  • x — is the resolved position in the current primary playlist
  • y — is the resolved position in the new primary playlist

Case 1 — Switch with valid cached playlist


+=================*==+
|     Period         |
+=================*==+

     +------X---+
     |     A    |
     +------X---+
         +--x-------+
         |     B    |
         +--x-------+

           seek and switch primary to A occur here...

          +-y--------+
          |    A'    |
          +-y--------+

Case 1 — Switch with an invalid cached playlist

+=======================*=======+
|           Period              |
+=======================*=======+

     +----------+
     |     A    |
     +----------+
                 +--x-------+
                 |     B    |
                 +--x-------+

           seek and switch primary to A occur here...

                  +-y--------+
                  |    A''   |
                  +-y--------+

So if all this worked as we expect, then adding this code to getAdjustedSeekPositionUs():

  public long getAdjustedSeekPositionUs(long positionUs, SeekParameters seekParameters) {
    long adjustedPositionUs = positionUs;

    int selectedIndex = trackSelection.getSelectedIndex();
    boolean haveTrackSelection = selectedIndex < playlistUrls.length && selectedIndex != C.INDEX_UNSET;
    @Nullable HlsMediaPlaylist mediaPlaylist = null;
    if (haveTrackSelection) {
      int selectedIndexInTrackGroup = trackSelection.getIndexInTrackGroup(selectedIndex);
      Uri playlistUrl = playlistUrls[selectedIndexInTrackGroup];
      if (playlistTracker.isSnapshotValid(playlistUrl)) {
        mediaPlaylist = playlistTracker.getPlaylistSnapshot(playlistUrl, true);
      }
...

Also sadly this does not fix the problem, we are still seeing the issue with Case 1 like updates, still investigating.

Can we keep the discussion here? as the pull request is likely to change. Note though at this point the pull request is the only logic that fixes the
issue. Thanks for helping out with this!

@tonihei
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The we need a call to isSnapshotValid() and that would cover the duration exceeded case.

I see how they are related, but it's hard to verify that isSnapshotValid() serves the same purpose as directly checking whether the duration is exceeded. For example, isSnapshotValid() checks lastSnapshotLoadMs + playlist.durationMs > currentTimeMs, but what if the snapshot contains segments before lastSnapshotLoadMs, then we can easily exceed the duration and still have a "valid" snapshot? Maybe I'm misreading the code :)

Have you tried the duration check to see if it solves your problem?

I agree though that we should not add this additional duration check if the playlist has an end tag or is of type VOD or EVENT.

@stevemayhew
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The duration fix is enough, I've evaluated all the cases with my testing, and convinced myself.
Here's what will be the commit comment on the new pull request:

Seek nearest SYNC does not adjust stale playlists

getPlaylistSnapshot() can return a cached but potentially quite stale playlist
that may be unsuitable for resolving a seek position.

This change fixes a race conditon that can cause a failed attempt to resolve
a seek position with a stale playlist.

Pre-conditions for the race:

  • A is the cached snapshot from previous playback of playlist A
  • B is the selected playlist at time of the seek
  • * — is current playback position in period
  • QQQ — positions in A cannot be seek targets from B, they have rolled out from Timeline.Window
  • WWW — are positions in A that cannot be resolved in A, but would be in A`
+=================*==+
|       Period       |
+=================*==+

     +QQQ-------+
     |     A    |
     +QQQ-------+
         +-------WWW+
         |     B    |
         +-------WWW+

           seek and switch back to A occur here...

          +----------+
          |    A'    |
          +----------+

The seek request is issued with Timeline.Window from playlist B, yet the call to getAdjustedSeekPositionUs()
occurs:

  1. After the current track selection swithces back to A
  2. But before playlist snapshot A` is loaded, replacing the old A

The check is simple, does basic sanity check on targetPositionInPlaylistUs (that it
is < durationUs of the selected playlist). This covers the positions WWW.

@tonihei
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, thanks! Also, if you can without too much additional effort, please file the PR against the Media3 project.

@stevemayhew
Copy link
Contributor Author

@stevemayhew stevemayhew commented on 530dd3f Jul 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, if you can without too much additional effort, please file the PR against the Media3 project.

Is there some coding convention that requires early exits and code hard line breaks at a certain column?

Because the code from the original pull request was reformatted in the merge commit check. This reformatting makes it really hard to cherry-pick to pull requests.

If code re-formatting is required to meet some style guide it would be best to do this all at once in a commit specific to that purpose rather then a merge commit.

@tonihei
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The formatting is done by some internal tooling and there is not much we can do about it. All changes we make to PRs (both automatic formatting and manual code changes) are only part of the merge commit unfortunately.

@stevemayhew
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @tonihei yes.. I reworked our code to match the early return style (it is not a bad pattern at all for complex logic like the cases to eliminate when the adjustment cannot be done).
I added a test case for the new condition. Lastly, I changed where the normalization back to playlist time was done, to match what is in current dev-v2, this is more clear to me as well
and functionally the same.

The hard part is when formatting changes are mixed in with code changes :-( Now the formatting and logic in our branch for this code matches what is in this pull request #10484

Please sign in to comment.