Fixed mediaIndex tracking so that it is consistent when the playlist updates during a live stream #977
Conversation
…updates during a live stream * Removed any code in SegmentLoader#handleUpdateEnd_ that changed the mediaIndex * Reordered SegmentLoader#playlist to make it easier to follow * All changes to both mediaIndexes (SegmentLoader's and segmentInfo's) now happen in SegmentLoader#playlist
| // this is important because we can abort a request and this value must be | ||
| // equal to the last appended mediaIndex | ||
| if (this.mediaIndex !== null) { | ||
| this.mediaIndex -= mediaSequenceDiff; |
There was a problem hiding this comment.
In the case where this results in a mediaIndex < -1 (we fell two slots behind the end of the playlist), do we want to handle the case in any way (since we knowingly fell off the back), or let playback watcher catch that we fell off the back in its own time (or another resolution, e.g., we keep the index at -1 and hope it'll create a 1 segment gap that will be skipped over by the gap skipper)?
There was a problem hiding this comment.
I think it is ok to let playback watcher handle this scenario instead of introducing complexity here. What do you think?
There was a problem hiding this comment.
Yeah, it's probably fine to do that. The behavior at least will be the same as current.
But it may be an interesting idea for the future. Since we're in a place where we knowingly will fall back to playback-watcher, and the user will experience a pause in playback, maybe we should at least add a comment or TODO to consider taking action.
| } | ||
| } else if (!this.hasPlayed_()) { | ||
| // when we haven't started playing yet, the start of the playlist is always | ||
| // our zero-time so force a sync update each time we get a new playlist |
There was a problem hiding this comment.
"each time we get a new playlist" may be a bit confusing in this context
|
It would be nice to have a unit test that emulates the scenario that caused the issue. |
| }); | ||
|
|
||
| QUnit.test('segmentInfo.mediaIndex is adjusted when live playlist is updated', function(assert) { | ||
| currentTime = 31; |
There was a problem hiding this comment.
A short comment here explaining that you're setting currentTime to 31 so the loader selects segment 3.ts would be helpful. It took me a while to figure out why segmentInfo.mediaIndex starts at 3 down below when loader.mediaIndex = null was being set
|
LGTM |
What
During low-bandwidth conditions, segment requests can take long enough that the playlist reloads while we are fetching segments. This confused our handling of
mediaIndexforcing the player to load the same segment that it loaded previously.If network conditions are bad enough (and we are on the lowest rendition) then we can get into a situation where we refresh the playlist during each segment's request. The end result is that we will fetch the same segment over and over again without making progress. In extreme cases (when two playlist loads happened while fetching a segment) we can actually go backwards!!
Why
There was code in
SegmentLoader#handleUpdateEnd_that would adjustsegmentInfo.mediaIndexbefore settingthis.mediaIndex. The change was based on the difference between the saved playlist object and the current playlist'smediaSequence.This action by
handleUpdateEnd_was ONLY required whenthis.mediaIndexwasnullbut we didn't have a check. Whenthis.mediaIndexwas notnull,SegmentLoader#playlistwas responsible for adjustingmediaIndex.For this PR, I decided to remove the code in
handleUpdateEnd_and move all the responsibility of trackingmediaIndexto one place -SegmentLoader#playlist.Changes
SegmentLoader#handleUpdateEnd_that modified themediaIndexSegmentLoader#playlistto make it easier to follow and less deeply nestedmediaIndex-es (SegmentLoader's and segmentInfo's) now happen solely inSegmentLoader#playlist