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: better time to first frame for live playlists #1105

Merged
merged 9 commits into from
Apr 5, 2021
Merged

fix: better time to first frame for live playlists #1105

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3cc24ca
fix: correct playlist end logic for live playlists
brandonocasey Mar 25, 2021
31a4fc7
test fixes
brandonocasey Mar 26, 2021
cef33d7
cr
brandonocasey Mar 26, 2021
86c1c1e
remove safeLiveIndex
brandonocasey Mar 26, 2021
02f025c
Merge branch 'main' into fix/better-ttff
brandonocasey Mar 29, 2021
9677a53
Merge branch 'main' into fix/better-ttff
brandonocasey Mar 30, 2021
f1ba676
add tests and organize
brandonocasey Mar 30, 2021
124d9cf
Merge branch 'main' into fix/better-ttff
brandonocasey Mar 30, 2021
ff70d0b
remove duplicate test
brandonocasey Apr 5, 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
15 changes: 10 additions & 5 deletions src/playlist.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ const forwardDuration = function(playlist, endSequence) {
*/
const intervalDuration = function(playlist, endSequence, expired) {
if (typeof endSequence === 'undefined') {
endSequence = playlist.mediaSequence + playlist.segments.length;
endSequence = playlist.mediaSequence + (playlist.segments.length - 1);
brandonocasey marked this conversation as resolved.
Show resolved Hide resolved
}

if (endSequence < playlist.mediaSequence) {
Expand Down Expand Up @@ -293,13 +293,18 @@ export const playlistEnd = function(playlist, expired, useSafeLiveEnd, liveEdgeP

expired = expired || 0;

const endSequence = useSafeLiveEnd ? safeLiveIndex(playlist, liveEdgePadding) : playlist.segments.length;
Copy link
Contributor Author

@brandonocasey brandonocasey Mar 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It doesn't really make sense to get a safe live index, based on how many seconds we want to start behind the playback head. Then get how many seconds that index starts at from behind the end of the playlist. We should instead get the timing information for the end of the playlist. Then subtract the live edge padding from that.

This was causing us to download 3/4 (but it could be any number really, especially for a fast connection.) segments before a frame of video would even be shown, because we were setting seekableEnd to an exact segment end point. We would then download a segment that should start -0.001s before the start of seekable end but when we append it the buffered region is actually +0.001s ahead of seekable end. Eventually playback-watcher would seek us into the buffered range.

This fix is better because:

  1. The spec stats that we should start 3 target durations behind live
  2. We will almost always start in the middle of a segment as targetDuration is almost never exact, preventing any rounding errors from stalling playback.


return intervalDuration(
let lastSegmentTime = intervalDuration(
playlist,
playlist.mediaSequence + endSequence,
playlist.mediaSequence + playlist.segments.length - 1,
expired
);

if (useSafeLiveEnd) {
liveEdgePadding = typeof liveEdgePadding === 'number' ? liveEdgePadding : playlist.targetDuration * 3;
brandonocasey marked this conversation as resolved.
Show resolved Hide resolved
lastSegmentTime -= (playlist.targetDuration * 3);
brandonocasey marked this conversation as resolved.
Show resolved Hide resolved
}

return lastSegmentTime;
};

/**
Expand Down
16 changes: 12 additions & 4 deletions src/sync-controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,12 @@ import {sumDurations} from './playlist';
import videojs from 'video.js';
import logger from './util/logger';

const getSegmentIndex = (i, playlist, currentTime = 0) => {
const segments = playlist.segments;

return (playlist.endList || currentTime === 0) ? i : segments.length - (i + 1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Loop from the back of the playlist when looking for a live sync point, as we are much more likely to be closer to the end.

};

export const syncPointStrategies = [
// Stategy "VOD": Handle the VOD-case where the sync-point is *always*
// the equivalence display-time 0 === segment-index 0
Expand Down Expand Up @@ -38,7 +44,8 @@ export const syncPointStrategies = [
currentTime = currentTime || 0;

for (let i = 0; i < segments.length; i++) {
const segment = segments[i];
const segmentIndex = getSegmentIndex(i, playlist, currentTime);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since ProgramDateTime is rare to see in VOD, the VOD strategy should be used more often for VOD, and, in theory, this code should not need to run very often for VOD (unless there's heavy rendition switching going on), we may be better off just always starting from the end, and keeping the code simpler.

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 don't think it's that much code and we share this bit between the two loops. I think that we will end up with more code if we change this to looping backwards.

const segment = segments[segmentIndex];
const datetimeMapping =
syncController.timelineToDatetimeMappings[segment.timeline];

Expand All @@ -60,7 +67,7 @@ export const syncPointStrategies = [
lastDistance = distance;
syncPoint = {
time: segmentStart,
segmentIndex: i
segmentIndex
};
}
}
Expand All @@ -79,7 +86,8 @@ export const syncPointStrategies = [
currentTime = currentTime || 0;

for (let i = 0; i < segments.length; i++) {
const segment = segments[i];
const segmentIndex = getSegmentIndex(i, playlist, currentTime);
const segment = segments[segmentIndex];

if (segment.timeline === currentTimeline &&
typeof segment.start !== 'undefined') {
Expand All @@ -95,7 +103,7 @@ export const syncPointStrategies = [
lastDistance = distance;
syncPoint = {
time: segment.start,
segmentIndex: i
segmentIndex
};
}

Expand Down