Skip to content
This repository has been archived by the owner on Jan 12, 2019. It is now read-only.

Pre segment switch #178

Merged
merged 24 commits into from
Oct 30, 2014
Merged

Pre segment switch #178

merged 24 commits into from
Oct 30, 2014

Conversation

gkatsev
Copy link
Member

@gkatsev gkatsev commented Oct 27, 2014

No description provided.

@gkatsev
Copy link
Member Author

gkatsev commented Oct 28, 2014

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) {
Copy link
Member

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.

Copy link
Member Author

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) {
Copy link
Member

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.

Copy link
Member Author

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.
dmlap added a commit that referenced this pull request Oct 30, 2014
@dmlap dmlap merged commit 4b0e731 into master Oct 30, 2014
@dmlap dmlap deleted the pre-segment-switch branch October 30, 2014 15:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants