-
Notifications
You must be signed in to change notification settings - Fork 792
handle dynamic buffer sizes (4k support) #1416
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -261,7 +261,7 @@ export class MasterPlaylistController extends videojs.EventTarget { | |
this.blacklistDuration = blacklistDuration; | ||
this.enableLowInitialPlaylist = enableLowInitialPlaylist; | ||
this.goalBufferLength_ = Config.GOAL_BUFFER_LENGTH; | ||
this.maxGoalBufferLength_ = Config.MAX_GOAL_BUFFER_LENGTH; | ||
this.maxGoalBufferLength_ = Math.max(Config.GOAL_BUFFER_LENGTH, Config.MAX_GOAL_BUFFER_LENGTH); | ||
this.backBufferLength_ = Config.BACK_BUFFER_LENGTH; | ||
|
||
if (this.useCueTags_) { | ||
|
@@ -589,8 +589,6 @@ export class MasterPlaylistController extends videojs.EventTarget { | |
if (!currentPlaylist.endList || | ||
// For the same reason as LIVE, we ignore the low water line when the VOD | ||
// duration is below the max potential low water line | ||
// TODO: This probably needs changing? Not sure what to change it to though. | ||
// Maybe just this.bufferLowWaterLine() ? | ||
this.duration() < Config.MAX_BUFFER_LOW_WATER_LINE || | ||
// we want to switch down to lower resolutions quickly to continue playback, but | ||
nextPlaylist.attributes.BANDWIDTH < currentPlaylist.attributes.BANDWIDTH || | ||
|
@@ -1258,6 +1256,7 @@ export class MasterPlaylistController extends videojs.EventTarget { | |
} | ||
this.goalBufferLength_ = Math.min(this.goalBufferLength_, value); | ||
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
Sorry, something went wrong. |
||
this.maxGoalBufferLength_ = Math.min(this.maxGoalBufferLength_, value); | ||
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
Sorry, something went wrong. |
||
return this.goalBufferLength; | ||
} | ||
|
||
/** | ||
|
@@ -1271,6 +1270,7 @@ export class MasterPlaylistController extends videojs.EventTarget { | |
return this.backBufferLength_; | ||
} | ||
this.backBufferLength_ = Math.min(this.backBufferLength_, value); | ||
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
Sorry, something went wrong. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even if another part of the code tries to make the buffers larger, the If your argument is semantic -- that a thing that looks and sounds like a setter should behave like a setter -- then I understand where you're coming from. They could, I suppose, be split into functions like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe that only |
||
return this.backBufferLength_; | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -84,7 +84,7 @@ export const illegalMediaSwitch = (loaderType, startingMedia, newSegmentMedia) = | |
* @param {Number} targetDuration | ||
* The target duration of the current playlist | ||
* @param {Number} backBufferLength | ||
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
Sorry, something went wrong. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a note here that after some discussions on slack, forward buffer should indeed be prioritized. I would say that is done as much as possible here, without having access to the GOP timing info. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed |
||
* The amount of back buffer to preserve | ||
* The amount (in seconds) of back buffer to preserve | ||
* @return {Number} | ||
* Time that is safe to remove from the back buffer without interupting playback | ||
*/ | ||
|
@@ -97,8 +97,9 @@ export const safeBackBufferTrimTime = (seekable, currentTime, targetDuration, ba | |
// If we have a seekable range use that as the limit for what can be removed safely | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this could potentially leave us with a buffer greater than the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. full disclosure: I have no idea what the seekable range means. I don't think I ever entered this code path when dealing with this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the seekable range only really comes into play when you have a live stream with a sliding window (in other words you might be disallowed from seeking to the beginning of the live stream). To be fair, it seems unlikely that you'll have a 4K live stream 😆 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, ok. I'm not sure it's reasonable to assume there will never be 4K live streams. I guess I should try to test with one. |
||
removeToTime = seekable.start(0); | ||
} else { | ||
// otherwise remove anything older than the back buffer | ||
removeToTime = currentTime - backBufferLength; | ||
// otherwise remove anything older than the back buffer, clamping to 0 | ||
// so that we don't have a negative removeToTime | ||
removeToTime = Math.max(0, currentTime - backBufferLength); | ||
} | ||
|
||
// Don't allow removing from the buffer within target duration of current time | ||
|
@@ -385,8 +386,13 @@ export default class SegmentLoader extends videojs.EventTarget { | |
init_() { | ||
this.state = 'READY'; | ||
this.sourceUpdater_ = new SourceUpdater(this.mediaSource_, this.mimeType_); | ||
// TODO: Make sure these are properly removed | ||
this.sourceUpdater_.on('adjustBuffers', () => this.adjustBuffers_(true)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if there's a reset method for the segment loaders, we should remove the event handlers there There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops, good catch, should also be added to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh actually, I'm not sure the segment loaders are re- |
||
this.sourceUpdater_.on('bufferMaxed', (event) => this.handleBufferMaxed_(event)); | ||
// Algo should be: | ||
// - adjust maxes (using segment metadata cues, even if they're wrong) | ||
// - remove non-buffered segment cues | ||
// - handleBufferMaxed | ||
|
||
this.resetEverything(); | ||
return this.monitorBuffer_(); | ||
|
@@ -521,7 +527,7 @@ export default class SegmentLoader extends videojs.EventTarget { | |
resetEverything() { | ||
this.ended_ = false; | ||
this.resetLoader(); | ||
this.remove(0, this.duration_()); | ||
this.remove(0, Infinity); | ||
this.trigger('reseteverything'); | ||
} | ||
|
||
|
@@ -688,6 +694,7 @@ export default class SegmentLoader extends videojs.EventTarget { | |
return null; | ||
} | ||
|
||
// Take the worst case for the amount of bytes in the buffer | ||
let bufferedBytes = this.maxBufferedBytes(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure whether the variable name There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the only byte information we have is at the entire segment level, if our buffer size ends up not locked to segment boundaries, it's unknown how much data we should include from that partial segment. So we have So here, we're getting the upper bound for the amount of bytes in the buffer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
So in the above we're saying "I'm guessing we were able to fit the whole last segment in the buffer"(bufferedBytes) "now see if we can fit the next one in without going over the maxBytes"? If so, I would comment over this line explaining that we're playing it safe and assuming the buffer is full to the upper bound There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm not sure what this has to do with the last segment...? What happened in the past is irrelevant, it's strictly based on the amount of bytes currently in the buffer. We're just testing if the next segment request ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I only said the last segment since we're appending full segments in here and we're using segment level info to guess how much we have in the buffer(referencing your explanation of Still, it would be useful to have a comment over the declaration of |
||
|
||
// If the next segment request is too large to fit into the current buffer, | ||
|
@@ -1230,20 +1237,20 @@ export default class SegmentLoader extends videojs.EventTarget { | |
* @param {Boolean=} setByteLimit | ||
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
Sorry, something went wrong. |
||
* Whether we should set maxBytes or just the buffer sizes | ||
*/ | ||
adjustBuffers_(setByteLimit) { | ||
adjustBuffers_(setByteLimit = false) { | ||
if (this.mediaRequests === 1) { | ||
// we failed on the first request, so don't adjust buffers | ||
// the first append caused a QuotaExceededError, so don't adjust buffers | ||
return; | ||
} | ||
this.removeNonBufferedSegmentCues(); | ||
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
Sorry, something went wrong. |
||
let buffered = this.buffered_(); | ||
let forwardBuffer = this.maxGoalBufferLength_(); | ||
let reverseBuffer = this.backBufferLength_(); | ||
let configTotalBuffer = forwardBuffer + reverseBuffer; | ||
let forwardBufferGoal = this.maxGoalBufferLength_(); | ||
let backBufferGoal = this.backBufferLength_(); | ||
let totalBufferGoal = forwardBufferGoal + backBufferGoal; | ||
let bytesInBuffer = 0; | ||
let secondsInBuffer = 0; | ||
let maxForwardBuffer; | ||
let maxReverseBuffer; | ||
let newForwardBufferGoal; | ||
let newBackBufferGoal; | ||
let i; | ||
let loggerMsg; | ||
|
||
|
@@ -1255,40 +1262,45 @@ export default class SegmentLoader extends videojs.EventTarget { | |
// we're downloading a single rate (and don't know what that rate is) | ||
// so we just use the seconds in the buffer to calculate the new buffer | ||
// sizes. | ||
maxForwardBuffer = Math.max((forwardBuffer / configTotalBuffer) * secondsInBuffer, | ||
newForwardBufferGoal = Math.max((forwardBufferGoal / totalBufferGoal) * secondsInBuffer, | ||
this.playlist_.targetDuration); | ||
maxReverseBuffer = secondsInBuffer - maxForwardBuffer; | ||
newBackBufferGoal = secondsInBuffer - newForwardBufferGoal; | ||
} else { | ||
// since we potentially have more than one rate in the buffer, we use | ||
// the amount of bytes in the buffer to calculate the upper limit on | ||
// the equivalent time at the current rate | ||
let upperTimeLimit = (bytesInBuffer * 8) / this.playlist_.attributes.BANDWIDTH; | ||
|
||
maxForwardBuffer = (forwardBuffer / configTotalBuffer) * upperTimeLimit; | ||
maxReverseBuffer = (reverseBuffer / configTotalBuffer) * upperTimeLimit; | ||
newForwardBufferGoal = (forwardBufferGoal / totalBufferGoal) * upperTimeLimit; | ||
newBackBufferGoal = (backBufferGoal / totalBufferGoal) * upperTimeLimit; | ||
} | ||
|
||
maxForwardBuffer = Math.min(forwardBuffer, maxForwardBuffer); | ||
maxReverseBuffer = Math.min(reverseBuffer, maxReverseBuffer); | ||
newForwardBufferGoal = Math.min(forwardBufferGoal, newForwardBufferGoal); | ||
newBackBufferGoal = Math.min(backBufferGoal, newBackBufferGoal); | ||
|
||
loggerMsg = '\n\tbytes in buffer: ' + bytesInBuffer + | ||
', seconds in buffer: ' + secondsInBuffer + | ||
'\n\tforward buffer: ' + forwardBuffer + ' => ' + maxForwardBuffer + | ||
'\n\treverse buffer: ' + reverseBuffer + ' => ' + maxReverseBuffer; | ||
loggerMsg = | ||
`\n\tbytes in buffer: ${bytesInBuffer}, seconds in buffer: ${secondsInBuffer}` + | ||
`\n\tforward buffer: ${forwardBufferGoal} => ${newForwardBufferGoal}` + | ||
`\n\treverse buffer: ${backBufferGoal} => ${newBackBufferGoal}`; | ||
|
||
if (setByteLimit) { | ||
loggerMsg += '\n\tbyte limit: ' + this.maxBytes_ + ' => ' + bytesInBuffer; | ||
loggerMsg += `\n\tbyte limit: ${this.maxBytes_} => ${bytesInBuffer}`; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if this log would be a little inaccurate if we end up staying at the original |
||
this.maxBytes_ = Math.min(this.maxBytes_, bytesInBuffer); | ||
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
Sorry, something went wrong. |
||
} | ||
this.logger_('adjustBuffers_', loggerMsg); | ||
|
||
this.goalBufferLength_(maxForwardBuffer); | ||
this.backBufferLength_(maxReverseBuffer); | ||
this.goalBufferLength_(newForwardBufferGoal); | ||
this.backBufferLength_(newBackBufferGoal); | ||
} | ||
|
||
/** | ||
* handler to run when a call to appendBuffer fails because the source | ||
* buffer can't hold any more information | ||
* Handle a call to appendBuffer that failed | ||
* | ||
* Depending on what caused the QuotaExceededError, one of two things will happen. | ||
* If it happened when appending the first segment of a stream, it will be split | ||
* into smaller pieces and appended immediately. If it happened in the regular | ||
* course of playing a feed, it will wait until there's sufficient room in the | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was there are reason for using different strategies here? If the error happens after the first segment, couldn't we remove from the back buffer and then append small pieces of the next segment instead of waiting for enough space to append the whole segment? |
||
* buffer and then re-append. | ||
* | ||
* @private | ||
* | ||
|
@@ -1301,13 +1313,17 @@ export default class SegmentLoader extends videojs.EventTarget { | |
handleBufferMaxed_(event) { | ||
let bytesTried = event.segment.byteLength; | ||
let segmentBytes = event.segment; | ||
// Note that is a shimmed WrappedSourceBuffer in the case of an mpegts hls feed, | ||
// and a native SourceBuffer in the case of an fmp4 hls feed | ||
let sourceBuffer = event.target; | ||
// Whe start time (in seconds) of the currently-playing GOP at the time the | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
// QuotaExceededError was thrown. Will be null for fmp4 hls feeds. | ||
let safeRemovePoint = event.safeRemovePoint; | ||
let pendingAppend = true; | ||
let callback = event.done; | ||
|
||
// if we fail on the first request, it's probably a glitch, and | ||
// we should just slice the segment up into small bits and append | ||
// if we get a QuotaExceededError on the first request, it's probably a | ||
// glitch, and we should just slice the segment into small bits and append | ||
if (this.mediaRequests === 1) { | ||
let slices = []; | ||
let start = 0; | ||
|
@@ -1322,12 +1338,12 @@ export default class SegmentLoader extends videojs.EventTarget { | |
}; | ||
|
||
while (start < bytesTried) { | ||
end += 5000000; | ||
end += Config.FIRST_SEGMENT_SLICE_SIZE; | ||
if (end > bytesTried) { | ||
end = bytesTried; | ||
} | ||
slices.push(segmentBytes.slice(start, end)); | ||
start += 5000000; | ||
start += Config.FIRST_SEGMENT_SLICE_SIZE; | ||
} | ||
sourceBuffer.addEventListener('updateend', sliceAppend); | ||
sourceBuffer.appendBuffer(slices.shift()); | ||
|
@@ -1351,7 +1367,7 @@ export default class SegmentLoader extends videojs.EventTarget { | |
callback(); | ||
pendingAppend = false; | ||
} catch (error) { | ||
videojs.error('QuoteExceededError'); | ||
videojs.error('QuotaExceededError'); | ||
} | ||
}; | ||
|
||
|
@@ -1431,6 +1447,7 @@ export default class SegmentLoader extends videojs.EventTarget { | |
if (!this.nextSegmentSize_) { | ||
this.nextSegmentSize_ = segmentInfo.byteLength; | ||
} else { | ||
// Calculate the moving average of the next segment size | ||
this.nextSegmentSize_ -= this.nextSegmentSize_ / | ||
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
Sorry, something went wrong. |
||
this.segmentMetadataTrack_.cues.length; | ||
this.nextSegmentSize_ += segmentInfo.byteLength / | ||
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
Sorry, something went wrong. |
||
|
@@ -1585,7 +1602,7 @@ export default class SegmentLoader extends videojs.EventTarget { | |
for (j = 0; j < buffered.length; j++) { | ||
cmpStart = (start > buffered.start(j)) ? start : buffered.start(j); | ||
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
Sorry, something went wrong. |
||
cmpEnd = (end < buffered.end(j)) ? end : buffered.end(j); | ||
if (cue.startTime <= cmpEnd && cue.endTime >= cmpStart) { | ||
if (cue.startTime <= cmpEnd || cue.endTime >= cmpStart) { | ||
bytes += cue.value.byteLength; | ||
// we only want to count the bytes once, so break now | ||
break; | ||
|
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.
Refactor to use the properties on
Hls
(see this)