-
Notifications
You must be signed in to change notification settings - Fork 795
Conversation
Also, update phantom. Conflicts: src/videojs-hls.js
This doesn't currently check whether the first downloaded playlist (first rendition in master) is the item we want to download if the user clicks play too soon or if we can/should download the the first segment from the playlist we were just switching to. |
oldMediaPlaylist = this.playlists.media(); | ||
|
||
if (this.bandwidth !== this.playlists.bandwidth) { | ||
if (this.bandwidth === undefined || this.bandwidth !== undefined && this.bandwidth < this.playlists.bandwidth) { |
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.
These conditionals are really complex. At the least, there should be a comment describing the desired effect here. It feels like we're trying everything and seeing what works, however. I think this deserves a re-think.
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.
That's true. I especially don't like that there are 3 if clauses that just do this.fillBuffer()
.
Though, there are test cases that cover each of these conditions.
Only set up timeupdate and waiting event listeners for fillBuffer and drainBuffer after we have upshifted. This is so that we would download the first segment of the new playlist if the player is setup with autoplay or a user clicks play too early. Also, we should only upshift if our estimated download time for the new segment is less than one second. This is done by estimating the duration length of the segment of the new playlist with the duration of the current segment in the current playlist.
No longer need to trigger loadedmetadata manually.
@@ -409,12 +448,20 @@ videojs.Hls.prototype.fillBuffer = function(offset) { | |||
this.loadSegment(segmentUri, offset); | |||
}; | |||
|
|||
// Encapsulate the setBandwidth routine for future expansion | |||
videojs.Hls.prototype.setBandwidthByXHR = function(xhr) { |
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.
If we're going to expose this function, can we make it a bit more generic? We could allow it to accept a generic object that included the bandwidth stats we're looking for.
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.
I'll change (and also the playlist loader one) it to setBandwidth
.
Makes the function more generic. It just accepts an object with a 'bandwidth' property.
No description provided.