fix: more accurate segment choices and logging #1127
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,30 +32,33 @@ import {getKnownPartCount} from './playlist.js'; | |
| * generate a syncPoint. This function returns a good candidate index | ||
| * for that process. | ||
| * | ||
| * @param {Array} segments - the segments array from a playlist. | ||
| * @return {number} An index of a segment from the playlist to load | ||
| */ | ||
| export const getSyncSegmentCandidate = function(currentTimeline, segments, targetTime) { | ||
| segments = segments || []; | ||
| const timelineSegments = []; | ||
| let time = 0; | ||
|
|
||
| for (let i = 0; i < segments.length; i++) { | ||
| const segment = segments[i]; | ||
|
|
||
| if (currentTimeline === segment.timeline) { | ||
| timelineSegments.push(i); | ||
| time += segment.duration; | ||
|
|
||
| if (time > targetTime) { | ||
| return i; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (timelineSegments.length === 0) { | ||
| return 0; | ||
| } | ||
|
|
||
| // default to the last timeline segment | ||
| return timelineSegments[timelineSegments.length - 1]; | ||
| }; | ||
|
|
||
| // In the event of a quota exceeded error, keep at least one second of back buffer. This | ||
|
|
@@ -137,6 +140,7 @@ const segmentInfoString = (segmentInfo) => { | |
| startOfSegment, | ||
| duration, | ||
| segment, | ||
| part, | ||
| playlist: { | ||
| mediaSequence: seq, | ||
| id, | ||
|
|
@@ -155,15 +159,15 @@ const segmentInfoString = (segmentInfo) => { | |
| } else if (segmentInfo.isSyncRequest) { | ||
| selection = 'getSyncSegmentCandidate (isSyncRequest)'; | ||
| } | ||
|
|
||
| const hasPartIndex = typeof partIndex === 'number'; | ||
| const name = segmentInfo.segment.uri ? 'segment' : 'pre-segment'; | ||
| const totalParts = hasPartIndex ? getKnownPartCount({preloadSegment: segment}) - 1 : 0; | ||
|
|
||
| return `${name} [${seq + index}/${seq + segmentLen}]` + | ||
| (hasPartIndex ? ` part [${partIndex}/${totalParts}]` : '') + | ||
|
There was a problem hiding this comment. Should this take into account the 0 based indexing vs total. Maybe we can just say `[index 0, 3 total] There was a problem hiding this comment. I think we should keep the name |
||
| ` segment start/end [${segment.start} => ${segment.end}]` + | ||
| (hasPartIndex ? ` part start/end [${part.start} => ${part.end}]` : '') + | ||
| ` startOfSegment [${startOfSegment}]` + | ||
| ` duration [${duration}]` + | ||
| ` timeline [${timeline}]` + | ||
|
|
@@ -1381,7 +1385,7 @@ export default class SegmentLoader extends videojs.EventTarget { | |
| }; | ||
|
|
||
| if (next.isSyncRequest) { | ||
| next.mediaIndex = getSyncSegmentCandidate(this.currentTimeline_, segments, bufferedEnd); | ||
| } else if (this.mediaIndex !== null) { | ||
| const segment = segments[this.mediaIndex]; | ||
| const partIndex = typeof this.partIndex === 'number' ? this.partIndex : -1; | ||
|
|
@@ -2030,6 +2034,31 @@ export default class SegmentLoader extends videojs.EventTarget { | |
| // as we use the start of the segment to offset the best guess (playlist provided) | ||
| // timestamp offset. | ||
| this.updateSourceBufferTimestampOffset_(segmentInfo); | ||
|
|
||
| // if this is a sync request we need to determine whether it should | ||
| // be appended or not. | ||
| if (segmentInfo.isSyncRequest) { | ||
| // first save/update our timing info for this segment. | ||
| // this is what allows us to choose an accurate segment | ||
| // and the main reason we make a sync request. | ||
| this.updateTimingInfoEnd_(segmentInfo); | ||
| this.syncController_.saveSegmentTimingInfo({ | ||
|
There was a problem hiding this comment. We may have to be careful here, since we're losing some of our timing logic, which may lead to inaccurate next requests: http-streaming/src/segment-loader.js Lines 2860 to 2863 in 1c7a63b There was a problem hiding this comment. Should we call this function within this block? From a brief glance it seems like it only estimates the segment end point using start/duration unless we have an There was a problem hiding this comment. I think we should probably call it. On that note we may also want to consider adding a property to the segment whether the last time we requested that segment it was a sync request (and clear it if it's requested on a non sync request). It may help us for debugging, particularly now that it's no longer being appended. |
||
| segmentInfo, | ||
| shouldSaveTimelineMapping: this.loaderType_ === 'main' | ||
| }); | ||
|
|
||
| const next = this.chooseNextRequest_(); | ||
|
|
||
| // If the sync request isn't the segment that would be requested next | ||
| // after taking into account its timing info, do not append it. | ||
| if (next.mediaIndex !== segmentInfo.mediaIndex || next.partIndex !== segmentInfo.partIndex) { | ||
| this.logger_('sync segment was incorrect, not appending'); | ||
| return; | ||
| } | ||
| // otherwise append it like any other segment as our guess was correct. | ||
| this.logger_('sync segment was correct, appending'); | ||
| } | ||
|
|
||
| // Save some state so that in the future anything waiting on first append (and/or | ||
| // timestamp offset(s)) can process immediately. While the extra state isn't optimal, | ||
| // we need some notion of whether the timestamp offset or other relevant information | ||
|
|
@@ -2884,8 +2913,6 @@ export default class SegmentLoader extends videojs.EventTarget { | |
| }); | ||
| } | ||
|
|
||
| const segmentDurationMessage = | ||
| getTroublesomeSegmentDurationMessage(segmentInfo, this.sourceType_); | ||
|
|
||
|
|
@@ -2903,9 +2930,18 @@ export default class SegmentLoader extends videojs.EventTarget { | |
|
|
||
| if (segmentInfo.isSyncRequest) { | ||
| this.trigger('syncinfoupdate'); | ||
| // if the sync request was not appended | ||
| // then it was not the correct segment. | ||
| // throw it away and use the data it gave us | ||
| // to get the correct one. | ||
| if (!segmentInfo.hasAppendedData_) { | ||
| this.logger_(`Throwing away un-appended sync request ${segmentInfoString(segmentInfo)}`); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| this.logger_(`Appended ${segmentInfoString(segmentInfo)}`); | ||
|
|
||
| this.addSegmentMetadataCue_(segmentInfo); | ||
| this.fetchAtBuffer_ = true; | ||
| if (this.currentTimeline_ !== segmentInfo.timeline) { | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we keep
TIME_FUDGE_FACTORat all?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: What is
TIME_FUDGE_FACTORused for here? Is using it causing timing issues down the line?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this may be the wrong place for us to use time fudging. We should probably only ever fudge time in one place: when we are trying to determine the next segment to request and are not simply walking the playlist (mediaIndex++). In that case, all requests to get media info/segment info should be as precise as possible given the current segment information, then, once the final segment is determined, we should decide whether we want to fudge the time at all to conservatively request a segment (i.e., whether to request back 1 segment, or, as an alternative, get media info for time with a fudge factor included).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change basically encompasses what you are saying. We only use
getMediaIndexForTimewhen we are not incrementing mediaIndex/partIndex. This change makes it so that cumulatively we can only be off byTIME_FUDGE_FACTORrather than every single part/segments duration being incremented or decremented byTIME_FUDGE_FACTOR. For the sake of time I think that we might want to add a TODO here to see if we should even use TIME_FUDGE_FACTOR at all. I lean towards not using it, but it will require a bit more testing and could easily enough be an isolated change.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 a TODO would be good for now. This definitely is better than before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably worth making an issue/a note in our major/refactoring epic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added