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.

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 llhls fixes #1201

Merged
merged 26 commits into from
Oct 20, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
1feddea
fix: Only check for bad seeks after seeking and an append
brandonocasey Sep 8, 2021
96d5e9f
Apply suggestions from code review
brandonocasey Sep 14, 2021
b071028
Merge branch 'main' into fix/use-seeking
brandonocasey Sep 15, 2021
c6144ad
Merge branch 'main' into fix/use-seeking
brandonocasey Sep 15, 2021
ff350fa
fix: more llhls fixes
brandonocasey Sep 15, 2021
9e74eae
do the same for forward duration
brandonocasey Sep 15, 2021
c42b1e1
do not switch to the same media twice
brandonocasey Sep 16, 2021
e5fd656
bad part guess and reset loader rather than resync
brandonocasey Sep 17, 2021
a3b9f09
update video.js
brandonocasey Sep 17, 2021
1704056
small segment choice fixes
brandonocasey Sep 17, 2021
cf340b0
independent and timeAheadOf fixes
brandonocasey Sep 17, 2021
33052bb
import ranges once
brandonocasey Sep 17, 2021
b40a28c
less time for llhls to skip bad seeks
brandonocasey Sep 17, 2021
5fd0ff3
add/update tests
brandonocasey Sep 20, 2021
21463d4
add more tests
brandonocasey Sep 21, 2021
5387192
Merge remote-tracking branch 'origin/main' into fix/more-llhls-fixes
brandonocasey Sep 22, 2021
c7765ba
Merge branch 'main' into fix/more-llhls-fixes
brandonocasey Sep 23, 2021
2927cd8
Merge branch 'main' into fix/more-llhls-fixes
brandonocasey Oct 18, 2021
9b2395b
add helper for preload segment duration
brandonocasey Oct 18, 2021
124fcc9
segmentInfoString tests
brandonocasey Oct 18, 2021
bd6adce
reset for live and resync for vod
brandonocasey Oct 18, 2021
64d513f
add test
brandonocasey Oct 18, 2021
fe2c2a6
code review
brandonocasey Oct 19, 2021
feabe07
move isBuffered and forwardBuffer to shouldSwitch
brandonocasey Oct 19, 2021
42fd68c
Update src/master-playlist-controller.js
brandonocasey Oct 20, 2021
db2da0f
Update src/master-playlist-controller.js
brandonocasey Oct 20, 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
29 changes: 21 additions & 8 deletions src/master-playlist-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,9 @@ const sumLoaderStat = function(stat) {
};
const shouldSwitchToMedia = function({
currentPlaylist,
buffered,
currentTime,
nextPlaylist,
forwardBuffer,
bufferLowWaterLine,
bufferHighWaterLine,
duration,
Expand All @@ -73,15 +74,25 @@ const shouldSwitchToMedia = function({
return false;
}

// determine if current time is in a buffered range.
const isBuffered = Boolean(Ranges.findRange(buffered, currentTime).length);

// If the playlist is live, then we want to not take low water line into account.
// This is because in LIVE, the player plays 3 segments from the end of the
// playlist, and if `BUFFER_LOW_WATER_LINE` is greater than the duration availble
// in those segments, a viewer will never experience a rendition upswitch.
if (!currentPlaylist.endList) {
// For LLHLS live streams, don't switch renditions before playback has started, as it almost
// doubles the time to first playback.
if (!isBuffered && typeof currentPlaylist.partTargetDuration === 'number') {
log(`not ${sharedLogLine} as current playlist is live llhls, but currentTime isn't in buffered.`);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add the comment you made in github below above here.

return false;
}
log(`${sharedLogLine} as current playlist is live`);
return true;
}

const forwardBuffer = Ranges.timeAheadOf(buffered, currentTime);
const maxBufferLowWaterLine = experimentalBufferBasedABR ?
Config.EXPERIMENTAL_MAX_BUFFER_LOW_WATER_LINE : Config.MAX_BUFFER_LOW_WATER_LINE;

Expand Down Expand Up @@ -732,18 +743,18 @@ export class MasterPlaylistController extends videojs.EventTarget {
}

shouldSwitchToMedia_(nextPlaylist) {
const currentPlaylist = this.masterPlaylistLoader_.media();
const buffered = this.tech_.buffered();
const forwardBuffer = buffered.length ?
buffered.end(buffered.length - 1) - this.tech_.currentTime() : 0;

const currentPlaylist = this.masterPlaylistLoader_.media() ||
this.masterPlaylistLoader_.pendingMedia_;
const currentTime = this.tech_.currentTime();
const bufferLowWaterLine = this.bufferLowWaterLine();
const bufferHighWaterLine = this.bufferHighWaterLine();
const buffered = this.tech_.buffered();

return shouldSwitchToMedia({
buffered,
currentTime,
currentPlaylist,
nextPlaylist,
forwardBuffer,
bufferLowWaterLine,
bufferHighWaterLine,
duration: this.duration(),
Expand Down Expand Up @@ -1434,7 +1445,9 @@ export class MasterPlaylistController extends videojs.EventTarget {
onSyncInfoUpdate_() {
let audioSeekable;

if (!this.masterPlaylistLoader_) {
// If we have two source buffers and only one is created then the seekable range will be incorrect.
// We should wait until all source buffers are created.
if (!this.masterPlaylistLoader_ || this.sourceUpdater_.hasCreatedSourceBuffers()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was causing us to:

  1. As soon as audio/video segment loader fires this we come in here
  2. Since only one of the loaders is actually ready at this point, we get seekable from just that one
  3. We seek to the end of this invalid seekable range
  4. We come back in here again and this time we get an intersection between the two and a valid seekable
  5. We seek to the valid seekable end via playback watcher
  6. This causes content to be removes and things to be downloaded/appended again.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would be worth adding a small comment above stating that. Something like: // if only one source buffer has been created, the seekable range will be invalid

return;
}

Expand Down
13 changes: 9 additions & 4 deletions src/playback-watcher.js
Original file line number Diff line number Diff line change
Expand Up @@ -351,10 +351,15 @@ export default class PlaybackWatcher {
const buffered = this.tech_.buffered();
const audioBuffered = sourceUpdater.audioBuffer ? sourceUpdater.audioBuffered() : null;
const videoBuffered = sourceUpdater.videoBuffer ? sourceUpdater.videoBuffered() : null;
const media = this.media();

// verify that at least two segment durations or one part duration have been
// appended before checking for a gap.
const minAppendedDuration = media.partTargetDuration ? media.partTargetDuration :
(media.targetDuration - Ranges.TIME_FUDGE_FACTOR) * 2;

// verify that at least two segment durations have been
// appended before checking for a gap.
const twoSegmentDurations = (this.media().targetDuration - Ranges.TIME_FUDGE_FACTOR) * 2;
const bufferedToCheck = [audioBuffered, videoBuffered];

for (let i = 0; i < bufferedToCheck.length; i++) {
Expand All @@ -365,9 +370,9 @@ export default class PlaybackWatcher {

const timeAhead = Ranges.timeAheadOf(bufferedToCheck[i], currentTime);

// if we are less than two video/audio segment durations behind,
// we haven't appended enough to call this a bad seek.
if (timeAhead < twoSegmentDurations) {
// if we are less than two video/audio segment durations or one part
// duration behind we haven't appended enough to call this a bad seek.
if (timeAhead < minAppendedDuration) {
return false;
}
}
Expand Down
7 changes: 6 additions & 1 deletion src/playlist-loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,8 @@ const getAllSegments = function(media) {
export const isPlaylistUnchanged = (a, b) => a === b ||
(a.segments && b.segments && a.segments.length === b.segments.length &&
a.endList === b.endList &&
a.mediaSequence === b.mediaSequence);
a.mediaSequence === b.mediaSequence &&
a.preloadSegment === b.preloadSegment);

/**
* Returns a new master playlist that is the result of merging an
Expand Down Expand Up @@ -516,6 +517,8 @@ export default class PlaylistLoader extends EventTarget {

this.targetDuration = playlist.partTargetDuration || playlist.targetDuration;

this.pendingMedia_ = null;

if (update) {
this.master = update;
this.media_ = this.master.playlists[id];
Expand Down Expand Up @@ -662,6 +665,8 @@ export default class PlaylistLoader extends EventTarget {
this.trigger('mediachanging');
}

this.pendingMedia_ = playlist;
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 this to prevent switching to the same playlist twice at the start of playback.

Copy link
Contributor

Choose a reason for hiding this comment

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

Where does it get used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

masterPlaylistLoader_.shouldSwitchToMedia_()


this.request = this.vhs_.xhr({
uri: playlist.resolvedUri,
withCredentials: this.withCredentials
Expand Down
50 changes: 44 additions & 6 deletions src/playlist.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,43 @@ import {TIME_FUDGE_FACTOR} from './ranges.js';

const {createTimeRange} = videojs;

/**
* Get the duration of a segment, with special cases for
* llhls segments that do not have a duration yet.
*
* @param {Object} playlist
* the playlist that the segment belongs to.
* @param {Object} segment
* the segment to get a duration for.
*
* @return {number}
* the segment duration
*/
export const segmentDurationWithParts = (playlist, segment) => {
// if this isn't a preload segment
// then we will have a segment duration that is accurate.
if (!segment.preload) {
return segment.duration;
}

// otherwise we have to add up parts and preload hints
// to get an up to date duration.
let result = 0;

(segment.parts || []).forEach(function(p) {
result += p.duration;
});

// for preload hints we have to use partTargetDuration
// as they won't even have a duration yet.
(segment.preloadHints || []).forEach(function(p) {
if (p.type === 'PART') {
result += playlist.partTargetDuration;
}
});

return result;
};
/**
* A function to get a combined list of parts and segments with durations
* and indexes.
Expand Down Expand Up @@ -117,7 +154,7 @@ const backwardDuration = function(playlist, endSequence) {
return { result: result + segment.end, precise: true };
}

result += segment.duration;
result += segmentDurationWithParts(playlist, segment);

if (typeof segment.start !== 'undefined') {
return { result: result + segment.start, precise: true };
Expand Down Expand Up @@ -149,7 +186,7 @@ const forwardDuration = function(playlist, endSequence) {
};
}

result += segment.duration;
result += segmentDurationWithParts(playlist, segment);

if (typeof segment.end !== 'undefined') {
return {
Expand Down Expand Up @@ -321,19 +358,19 @@ export const playlistEnd = function(playlist, expired, useSafeLiveEnd, liveEdgeP

expired = expired || 0;

let lastSegmentTime = intervalDuration(
let lastSegmentEndTime = intervalDuration(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A more descriptive name as it isn't the start time, but the end time we are getting here.

playlist,
playlist.mediaSequence + playlist.segments.length,
expired
);

if (useSafeLiveEnd) {
liveEdgePadding = typeof liveEdgePadding === 'number' ? liveEdgePadding : liveEdgeDelay(null, playlist);
lastSegmentTime -= liveEdgePadding;
lastSegmentEndTime -= liveEdgePadding;
}

// don't return a time less than zero
return Math.max(0, lastSegmentTime);
return Math.max(0, lastSegmentEndTime);
};

/**
Expand Down Expand Up @@ -737,5 +774,6 @@ export default {
estimateSegmentRequestTime,
isLowestEnabledRendition,
isAudioOnly,
playlistMatch
playlistMatch,
segmentDurationWithParts
};
74 changes: 59 additions & 15 deletions src/segment-loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,7 @@ import {
import { gopsSafeToAlignWith, removeGopBuffer, updateGopBuffer } from './util/gops';
import shallowEqual from './util/shallow-equal.js';
import { QUOTA_EXCEEDED_ERR } from './error-codes';
import { timeRangesToArray } from './ranges';
import {lastBufferedEnd} from './ranges.js';
import {timeRangesToArray, lastBufferedEnd, timeAheadOf} from './ranges.js';
import {getKnownPartCount} from './playlist.js';

/**
Expand Down Expand Up @@ -135,7 +134,7 @@ export const safeBackBufferTrimTime = (seekable, currentTime, targetDuration) =>
return Math.min(maxTrimTime, trimTime);
};

const segmentInfoString = (segmentInfo) => {
export const segmentInfoString = (segmentInfo) => {
const {
startOfSegment,
duration,
Expand All @@ -160,6 +159,10 @@ const segmentInfoString = (segmentInfo) => {
selection = 'getSyncSegmentCandidate (isSyncRequest)';
}

if (segmentInfo.independent) {
selection += ` with independent ${segmentInfo.independent}`;
}

const hasPartIndex = typeof partIndex === 'number';
const name = segmentInfo.segment.uri ? 'segment' : 'pre-segment';
const zeroBasedPartCount = hasPartIndex ? getKnownPartCount({preloadSegment: segment}) - 1 : 0;
Expand Down Expand Up @@ -1024,9 +1027,20 @@ export default class SegmentLoader extends videojs.EventTarget {

if (!oldPlaylist || oldPlaylist.uri !== newPlaylist.uri) {
if (this.mediaIndex !== null) {
// we must "resync" the segment loader when we switch renditions and
// we must reset/resync the segment loader when we switch renditions and
// the segment loader is already synced to the previous rendition
this.resyncLoader();

// on playlist changes we want it to be possible to fetch
// at the buffer for vod but not for live. So we use resetLoader
// for live and resyncLoader for vod. We want this because
// if a playlist uses independent and non-independent segments/parts the
// buffer may not accurately reflect the next segment that we should try
// downloading.
if (!newPlaylist.endList) {
this.resetLoader();
} else {
this.resyncLoader();
}
}
this.currentMediaInfo_ = void 0;
this.trigger('playlistupdate');
Expand Down Expand Up @@ -1366,8 +1380,9 @@ export default class SegmentLoader extends videojs.EventTarget {
* @return {Object} a request object that describes the segment/part to load
*/
chooseNextRequest_() {
const bufferedEnd = lastBufferedEnd(this.buffered_()) || 0;
const bufferedTime = Math.max(0, bufferedEnd - this.currentTime_());
const buffered = this.buffered_();
const bufferedEnd = lastBufferedEnd(buffered) || 0;
const bufferedTime = timeAheadOf(buffered, this.currentTime_());
const preloaded = !this.hasPlayed_() && bufferedTime >= 1;
const haveEnoughBuffer = bufferedTime >= this.goalBufferLength_();
const segments = this.playlist_.segments;
Expand Down Expand Up @@ -1420,14 +1435,15 @@ export default class SegmentLoader extends videojs.EventTarget {
startTime: this.syncPoint_.time
});

next.getMediaInfoForTime = this.fetchAtBuffer_ ? 'bufferedEnd' : 'currentTime';
next.getMediaInfoForTime = this.fetchAtBuffer_ ?
`bufferedEnd ${bufferedEnd}` : `currentTime ${this.currentTime_()}`;
next.mediaIndex = segmentIndex;
next.startOfSegment = startTime;
next.partIndex = partIndex;
}

const nextSegment = segments[next.mediaIndex];
const nextPart = nextSegment &&
let nextPart = nextSegment &&
typeof next.partIndex === 'number' &&
nextSegment.parts &&
nextSegment.parts[next.partIndex];
Expand All @@ -1442,6 +1458,28 @@ export default class SegmentLoader extends videojs.EventTarget {
// Set partIndex to 0
if (typeof next.partIndex !== 'number' && nextSegment.parts) {
next.partIndex = 0;
nextPart = nextSegment.parts[0];
}

// if we have no buffered data then we need to make sure
// that the next part we append is "independent" if possible.
// So we check if the previous part is independent, and request
// it if it is.
if (!bufferedTime && nextPart && !nextPart.independent) {

if (next.partIndex === 0) {
const lastSegment = segments[next.mediaIndex - 1];
const lastSegmentLastPart = lastSegment.parts && lastSegment.parts.length && lastSegment.parts[lastSegment.parts.length - 1];

if (lastSegmentLastPart && lastSegmentLastPart.independent) {
next.mediaIndex -= 1;
next.partIndex = lastSegment.parts.length - 1;
next.independent = 'previous segment';
}
} else if (nextSegment.parts[next.partIndex - 1].independent) {
next.partIndex -= 1;
next.independent = 'previous part';
}
}

const ended = this.mediaSource_ && this.mediaSource_.readyState === 'ended';
Expand All @@ -1459,6 +1497,7 @@ export default class SegmentLoader extends videojs.EventTarget {

generateSegmentInfo_(options) {
const {
independent,
playlist,
mediaIndex,
startOfSegment,
Expand Down Expand Up @@ -1499,7 +1538,8 @@ export default class SegmentLoader extends videojs.EventTarget {
byteLength: 0,
transmuxer: this.transmuxer_,
// type of getMediaInfoForTime that was used to get this segment
getMediaInfoForTime
getMediaInfoForTime,
independent
};

const overrideCheck =
Expand Down Expand Up @@ -1991,7 +2031,7 @@ export default class SegmentLoader extends videojs.EventTarget {
this.setTimeMapping_(segmentInfo.timeline);

// for tracking overall stats
this.updateMediaSecondsLoaded_(segmentInfo.segment);
this.updateMediaSecondsLoaded_(segmentInfo.part || segmentInfo.segment);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

stats fix


// Note that the state isn't changed from loading to appending. This is because abort
// logic may change behavior depending on the state, and changing state too early may
Expand Down Expand Up @@ -2995,15 +3035,19 @@ export default class SegmentLoader extends videojs.EventTarget {
// and attempt to resync when the post-update seekable window and live
// point would mean that this was the perfect segment to fetch
this.trigger('syncinfoupdate');

const segment = segmentInfo.segment;
const part = segmentInfo.part;
const badSegmentGuess = segment.end &&
this.currentTime_() - segment.end > segmentInfo.playlist.targetDuration * 3;
const badPartGuess = part &&
part.end && this.currentTime_() - part.end > segmentInfo.playlist.partTargetDuration * 3;

// If we previously appended a segment that ends more than 3 targetDurations before
// If we previously appended a segment/part that ends more than 3 part/targetDurations before
// 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.end &&
this.currentTime_() - segment.end > segmentInfo.playlist.targetDuration * 3) {
if (badSegmentGuess || badPartGuess) {
this.logger_(`bad ${badSegmentGuess ? 'segment' : 'part'} ${segmentInfoString(segmentInfo)}`);
this.resetEverything();
return;
}
Expand Down
Loading