Resync on poor initial segment choice #1016

imbcmdth · 2017-02-15T23:53:29Z

Description

The SegmentLoader is designed to generate a conservative guess that is always at or before the target time so that we can enter a mode where we fetch a contiguous set of segments and will be guaranteed to hit the "play head".

With very long playlists (> 2 hours) this conservative guess can be quite a bit off. For instance, a playlist with a duration of 5 hours can result in a guess that is a minute or more behind the playhead. The reason for this is that the only known segments will be early in the playlist and we attempt to make a guess some 5 hours and thousands of segments later often without very accurate timing info.

This PR addresses this by allowing the SegmentLoader to guess again but using the information learned from the original poor guess. In testing, this second guess is usually pretty close to perfect since the original guess was (relatively) fairly close to begin with.

To accomplish this some enhancements were necessary to SyncController as well. The Segment strategy did not take into account the currentTime and would always return the latest (in time) segment with timing information. Now we always return the segment closest to the time ensuring that guesses are more accurate.

Some minor reorganizing was done to SegmentLoader in the earlier commits so that function order better reflects the flow of execution and puts related functions near each other for easier development and understanding.

SyncController was improved in other small ways and no longer returns the very first sync-point found but tries all strategies and returns the one closest to the currentTime which is the most accurate sync-point to base calculations off of.

Specific Changes proposed

Please list the specific changes involved in this pull request.

Requirements Checklist

Feature implemented / Bug fixed
If necessary, more likely in a feature request than a bug fix
- Unit Tests updated or fixed
Reviewed by Two Core Contributors

mjneil · 2017-02-16T18:59:32Z

src/segment-loader.js

+
+    // Don't do a rendition switch unless the SegmentLoader is already walking forward
+    if (isWalkingForward) {
+      this.trigger('progress');


I understand this is to limit ping-ponging between renditions without at least appending another segment, but i worry this could cause unwanted timeout errors. I could imagine that in a live scenario where we are close to the live point, and we do a rendition switch, now if this is a new rendition, we have to wait for a sync point, then another load + append, and another load + append before we are considered walkingForward. If in this intermediate time, we reach the end of the buffer and waiting on buffering, we may not fire progress events for some time if bandwidth conditions cause those loads to take a long time. This could cause a timeout when we otherwise would be notifying the player that we are making some type of progress

I discussed this with Matt in person. Basically, there are trade-offs either way. With this in place we may not respond to bandwidth issues as quickly but we ensure that we are always in a stable playback mode before we do switch. This is less important for VOD but it's a different story for live. One problem is that seekable can become stalled because we are changing playlists without having sync. Another problem is that we can conservatively guess multiple times in a row and will end up fetching the same segment over and over at different renditions (worst-case being ping-ponging as Matt put it.) This change should always result in a sync point being established and some forward progress being made before a rendition is allowed to change. Timeout-caused rendition changes will still happen immediately so that the player can take emergency action.

mjneil · 2017-02-16T19:02:01Z

Don't forget to migrate the change from here to this PR

gesinger · 2017-02-16T19:38:48Z

src/segment-loader.js

-    this.logger_('handleUpdateEnd_', this.mediaIndex);
+    this.state = 'READY';
+
+    // If we previously appended a segment that ends more than 3 targetDurations before


Might read better as just "If we just appended"

gesinger · 2017-02-16T19:39:49Z

src/segment-loader.js

+    // the currentTime_ that means that our conservative guess was too conservative.
+    // In that case, reset the loader state so that we try to use any information gained
+    // from the previous request to create a new, more accurate, sync-point.
+    if (segment &&


Is it possible for segment to be null here?

gesinger · 2017-02-16T19:53:25Z

src/sync-controller.js

+
+          // Once the distance begins to increase, we have passed
+          // currentTime and can stop looking for better candidates
+          if (lastDistance && lastDistance < distance) {


In a case where currentTime is equal to segment.start and we have a distance of 0, it seems that the next iteration will skip over this and skip over the next conditional to set the sync point to the next segment.

I'm not sure I understand. How can you ever have two segments with the same start-time?

Not two segments with the same start time, but if currentTime is equal to segment.start then distance is 0 and the syncPoint would be set to the following segment (if one exists) because when lastDistance is 0 for the next iteration, this condition is false and the next condition (where it checks for !lastDistance) would result in true (leading to an overwrite of syncPoint).

Oh, your complaint was that I need to do lastDistance !== null ... Why did you just say so! 😆

gesinger · 2017-02-16T19:57:12Z

src/sync-controller.js

+
+            // Once the distance begins to increase, we have passed
+            // currentTime and can stop looking for better candidates
+            if (lastDistance && lastDistance < distance) {


Same comment as above re: case of distance 0.

gesinger · 2017-02-16T20:11:56Z

test/sync-controller.test.js

      { timeline: 1 },
      { timeline: 1 },
-      { timeline: 1, start: 30 },


Would be good to have a test with exact start as currentTime as well

…s closest to currentTime to ensure the most accurate decisions

…ecting the sync-point that is closest to the currentTime regardless of the order of sync-strategies

…te a better sync-point and guess again

…red value for Flash compatibility

…int candidate

…oor-guess behaviors (#1023) * Fix a bug with the combination of seek-to-live (#1006) and resync-on-a-poor-guess (#1016) behaviors * Added tests to ensure the proper sequence of events for seekable and resync logic * Unregister the seekablechanged event handler from the tech on dispose

mjneil reviewed Feb 16, 2017

View reviewed changes

imbcmdth force-pushed the reorganize-segmentloader branch from a229782 to 74b4c40 Compare February 16, 2017 20:11

gesinger reviewed Feb 16, 2017

View reviewed changes

imbcmdth force-pushed the reorganize-segmentloader branch from 74b4c40 to e6177e6 Compare February 16, 2017 20:44

jrivera added 5 commits February 16, 2017 16:06

Fix discontinuity sync-strategy and re-enable tests

691587c

Fixed segment strategy to always return the segment sync-point that i…

6695196

…s closest to currentTime to ensure the most accurate decisions

Improve sync-point determination by trying each sync-strategy and sel…

fa0d2ac

…ecting the sync-point that is closest to the currentTime regardless of the order of sync-strategies

If the last segment loaded was too far in the past, attempt to genera…

61ee89f

…te a better sync-point and guess again

Changed the resync on poor guess logic to no longer rely on the buffe…

18a8622

…red value for Flash compatibility

imbcmdth force-pushed the resync-on-bad-guess branch from 1d5a079 to 18a8622 Compare February 16, 2017 21:08

imbcmdth changed the base branch from reorganize-segmentloader to master February 16, 2017 21:08

Fixed bug in sync-controller when currentTime exactly equals a syncPo…

04372ee

…int candidate

gesinger added the needs:testing label Feb 16, 2017

gesinger approved these changes Feb 16, 2017

View reviewed changes

mjneil mentioned this pull request Feb 21, 2017

Some m3u8 do not work #1020

Closed

gesinger added tested and removed needs:testing labels Feb 21, 2017

imbcmdth added needs:testing and removed tested labels Feb 21, 2017

imbcmdth merged commit 70ffe9e into master Feb 22, 2017

imbcmdth deleted the resync-on-bad-guess branch February 22, 2017 01:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resync on poor initial segment choice #1016

Resync on poor initial segment choice #1016

imbcmdth commented Feb 15, 2017

mjneil Feb 16, 2017

imbcmdth Feb 16, 2017

mjneil commented Feb 16, 2017

gesinger Feb 16, 2017

gesinger Feb 16, 2017

gesinger Feb 16, 2017

imbcmdth Feb 16, 2017

gesinger Feb 16, 2017

imbcmdth Feb 16, 2017

gesinger Feb 16, 2017

gesinger Feb 16, 2017

Resync on poor initial segment choice #1016

Resync on poor initial segment choice #1016

Conversation

imbcmdth commented Feb 15, 2017

Description

Specific Changes proposed

Requirements Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjneil commented Feb 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment