-
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 |
---|---|---|
|
@@ -260,6 +260,9 @@ export class MasterPlaylistController extends videojs.EventTarget { | |
this.useCueTags_ = useCueTags; | ||
this.blacklistDuration = blacklistDuration; | ||
this.enableLowInitialPlaylist = enableLowInitialPlaylist; | ||
this.goalBufferLength_ = Config.GOAL_BUFFER_LENGTH; | ||
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. Refactor to use the properties on |
||
this.maxGoalBufferLength_ = Config.MAX_GOAL_BUFFER_LENGTH; | ||
this.backBufferLength_ = Config.BACK_BUFFER_LENGTH; | ||
|
||
if (this.useCueTags_) { | ||
this.cueTagsTrack_ = this.tech_.addTextTrack('metadata', | ||
|
@@ -299,7 +302,9 @@ export class MasterPlaylistController extends videojs.EventTarget { | |
seeking: () => this.tech_.seeking(), | ||
duration: () => this.mediaSource.duration, | ||
hasPlayed: () => this.hasPlayed_(), | ||
goalBufferLength: () => this.goalBufferLength(), | ||
goalBufferLength: this.goalBufferLength.bind(this), | ||
maxGoalBufferLength: () => this.maxGoalBufferLength_, | ||
backBufferLength: this.backBufferLength.bind(this), | ||
bandwidth, | ||
syncController: this.syncController_, | ||
decrypter: this.decrypter_ | ||
|
@@ -584,6 +589,8 @@ 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() ? | ||
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 this should technically remain the same as the intention was to allow rendition switching on shorter videos (those shorter than the max low water line). Also, we should remove this TODO since we didn't actually change the value 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. Right, I guess since 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. Yep, exactly |
||
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 || | ||
|
@@ -1235,17 +1242,35 @@ export class MasterPlaylistController extends videojs.EventTarget { | |
} | ||
|
||
/** | ||
* Calculates the desired forward buffer length based on current time | ||
* Calculates or sets the desired forward buffer length based on current time | ||
* | ||
* @return {Number} Desired forward buffer length in seconds | ||
* @param {Number=} Desired forward buffer length in seconds | ||
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. |
||
* @return {Number} Current forward buffer target length | ||
*/ | ||
goalBufferLength() { | ||
const currentTime = this.tech_.currentTime(); | ||
const initial = Config.GOAL_BUFFER_LENGTH; | ||
const rate = Config.GOAL_BUFFER_LENGTH_RATE; | ||
const max = Math.max(initial, Config.MAX_GOAL_BUFFER_LENGTH); | ||
goalBufferLength(value) { | ||
if (value === undefined) { | ||
const currentTime = this.tech_.currentTime(); | ||
const initial = this.goalBufferLength_; | ||
const rate = Config.GOAL_BUFFER_LENGTH_RATE; | ||
const max = Math.max(initial, this.maxGoalBufferLength_); | ||
|
||
return Math.min(initial + currentTime * rate, max); | ||
} | ||
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 Math.min(initial + currentTime * rate, max); | ||
/** | ||
* Set or return the back buffer length | ||
* | ||
* @param {Number=} Desired back buffer length in seconds | ||
* @return {Number} Current back buffer target length | ||
*/ | ||
backBufferLength(value) { | ||
if (value === undefined) { | ||
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 |
||
} | ||
|
||
/** | ||
|
@@ -1259,6 +1284,6 @@ export class MasterPlaylistController extends videojs.EventTarget { | |
const rate = Config.BUFFER_LOW_WATER_LINE_RATE; | ||
const max = Math.max(initial, Config.MAX_BUFFER_LOW_WATER_LINE); | ||
|
||
return Math.min(initial + currentTime * rate, max); | ||
return Math.min(initial + currentTime * rate, max, this.goalBufferLength()); | ||
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.
This comment was marked as resolved.
Sorry, something went wrong. |
||
} | ||
} |
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.
does this mean we'll have a total buffer length of 60? (
GOAL_BUFFER_LENGTH
being the forward buffer)?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.
The forward buffer scales with time, so at the beginning it would be 60 (
GOAL_BUFFER_LENGTH + BACK_BUFFER_LENGTH
) and after playing for a bit it would be 90 (MAX_GOAL_BUFFER_LENGTH + BACK_BUFFER_LENGTH
).The constants could probably use some renaming to make them more clear :)
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.
hmm. Ok, curious if we should consider tweaking these values, but that's just curiosity... 🙃
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.
Indeed! I just used the values that were already in the code.
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 fine for now