Fix: SyncController.selectSyncPoint_ and segment-loader properly handling gaps in live mode #1292
Conversation
-> otherwise we dont take in account for selection first sync-point candidate
…mpOffset on source-buffer otherwise we are appending at the offset calculated from the segment duration sum but the segment internal timestamp can be different when the playlist misses a segment -> we generally need to be able to handle gaps that are not advertised -> therefore the only ground-truth can be the timestamps in the TS, not the duration-sum => by compensating back the mapping here, this resolves to the actual TS timestamps
… sync-point. somehow stalls in 20% of attempts, but otherwise works nicely.
…n currentTime/lastBufferedEnd to syncPoint.time
There was a problem hiding this comment.
Thanks for another PR @tchakabam !
I left some comments within. When you get a chance, let me know your thoughts.
| @@ -285,7 +285,7 @@ export default class SyncController extends videojs.EventTarget { | |||
| let bestDistance = Math.abs(syncPoints[0].syncPoint[target.key] - target.value); | |||
| let bestStrategy = syncPoints[0].strategy; | |||
|
|
|||
| for (let i = 1; i < syncPoints.length; i++) { | |||
| for (let i = 0; i < syncPoints.length; i++) { | |||
There was a problem hiding this comment.
I think this is meant to be 1 since the values are initialized to the first element at 0
| @@ -680,6 +682,22 @@ export default class SegmentLoader extends videojs.EventTarget { | |||
| 'fetchAtBuffer:', this.fetchAtBuffer_, | |||
| 'bufferedTime:', bufferedTime); | |||
|
|
|||
| let syncPointError = 0; | |||
| if (currentTime < lastBufferedStart) { | |||
There was a problem hiding this comment.
I think that handling the gap resolution logic may be better in playback watcher, probably within fixedBadSeeks_:
videojs-contrib-hls/src/playback-watcher.js
Line 157 in 2158b75
This would also allow us to re-seek.
| segmentInfo.timestampOffset !== this.sourceUpdater_.timestampOffset()) { | ||
| this.sourceUpdater_.timestampOffset(segmentInfo.timestampOffset); | ||
| const timelineMapping = this.syncController_.mappingForTimeline(segmentInfo.timeline); | ||
| const absoluteTimestampOffset = segmentInfo.timestampOffset - timelineMapping; |
There was a problem hiding this comment.
Since it's all relative to the first timestamp offset we append, wouldn't the numbers come out equitable whether we subtract the starting timestamp offset versus use the actual timestamp offsets provided? I saw you mentioned that it helps handle gaps, but would you be able to provide a case where it happens?
| && resolvedTimestampOffset !== this.sourceUpdater_.timestampOffset()) { | ||
|
|
||
|
|
||
| console.log('setting timestamp offset (resolved to timeline):', absoluteTimestampOffset); |
|
Thank you for your PR but we will not be accepting new changes for this repo and will be archived very soon. I would advise you to open your PR against the next iteration of this project at https://github.com/videojs/http-streaming. |
Description
Please describe the change as necessary.
If it's a feature or enhancement please be as detailed as possible.
If it's a bug fix, please link the issue that it fixes or describe the bug in as much detail.
Specific Changes proposed
We didn't handle unadvertised gaps in playlists in live cases yet where we were sync'ing in playlist-mode.
This adds handling the case where the initial guess was wrong and we have started loading to far ahead and not on the target time.
Also, we generally should append with the actual timestamps offset, as otherwise we are not able for handling timestamp gaps at all properly.
Also, there seemed to be a bug in the SyncController, where we were iterating from 1 instead of 0 on the sync methods.
Requirements Checklist