Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: more accurate segment choices and logging #1127

Merged
merged 14 commits into from
May 26, 2021
Merged

fix: more accurate segment choices and logging #1127

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
a4a45df
fix: more accurate segment choices and logging
brandonocasey May 5, 2021
d2d4aab
Merge branch 'main' into fix/better-segment-choice
brandonocasey May 24, 2021
127e075
ie 11 fix
brandonocasey May 24, 2021
6ddcf4c
pass segments to getSyncSegmentCandidate
brandonocasey May 25, 2021
fa97a95
Update src/segment-loader.js
brandonocasey May 25, 2021
eae4f41
Update src/segment-loader.js
brandonocasey May 25, 2021
17dda63
inccorrect assertion text
brandonocasey May 25, 2021
ad088c5
fix trailing space
brandonocasey May 25, 2021
8ebbc50
time fudge factor comment
brandonocasey May 26, 2021
c6f36e4
call updateTimingInfoEnd and add some comments
brandonocasey May 26, 2021
2bd3f53
better comment
brandonocasey May 26, 2021
dd29591
Merge remote-tracking branch 'origin/main' into fix/better-segment-ch…
brandonocasey May 26, 2021
a971ff0
add PART-TARGET
brandonocasey May 26, 2021
504f434
totalParts -> zeroBasedPartCount
brandonocasey May 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/playlist.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,9 @@ export const getMediaInfoForTime = function(

time += segment.duration;

// TODO: We should consider not using TIME_FUDGE_FACTOR at all here, but at
// the time where this change was made there wasn't enough time to test without it.
// We should make some time to see if we can remove TIME_FUDGE_FACTOR here.
brandonocasey marked this conversation as resolved.
Show resolved Hide resolved
if ((time + TIME_FUDGE_FACTOR) > 0) {
Copy link
Contributor Author

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_FACTOR at all?

Copy link
Member

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_FACTOR used for here? Is using it causing timing issues down the line?

Copy link
Contributor

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).

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 think this change basically encompasses what you are saying. We only use getMediaIndexForTime when we are not incrementing mediaIndex/partIndex. This change makes it so that cumulatively we can only be off by TIME_FUDGE_FACTOR rather than every single part/segments duration being incremented or decremented by TIME_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.

Copy link
Contributor

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.

Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

return {
mediaIndex: segment.segmentIndex,
Expand Down Expand Up @@ -418,6 +421,9 @@ export const getMediaInfoForTime = function(

time -= partSegment.duration;

// TODO: We should consider not using TIME_FUDGE_FACTOR at all here, but at
// the time where this change was made there wasn't enough time to test without it.
// We should make some time to see if we can remove TIME_FUDGE_FACTOR here.
if ((time - TIME_FUDGE_FACTOR) < 0) {
return {
mediaIndex: partSegment.segmentIndex,
Expand Down
10 changes: 7 additions & 3 deletions src/segment-loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2038,9 +2038,13 @@ export default class SegmentLoader extends videojs.EventTarget {
// timestamp offset.
this.updateSourceBufferTimestampOffset_(segmentInfo);

// throw away the isSyncRequest segment if
// it wouldn't have been the next segment we request.
// 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({
Copy link
Contributor

Choose a reason for hiding this comment

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

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

// Now that the end of the segment has been reached, we can set the end time. It's
// best to wait until all appends are done so we're sure that the primary media is
// finished (and we have its end time).
this.updateTimingInfoEnd_(segmentInfo);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 end. Seems like we should call it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

called

segmentInfo,
shouldSaveTimelineMapping: this.loaderType_ === 'main'
Expand All @@ -2054,8 +2058,8 @@ export default class SegmentLoader extends videojs.EventTarget {
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
Expand Down