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(sync-controller): add a default sync point when currentTimeline is -1 #500

Closed
wants to merge 10 commits into from

Conversation

gkatsev
Copy link
Member

@gkatsev gkatsev commented May 6, 2019

This also fixes #464.

@mjneil
Copy link
Contributor

mjneil commented May 6, 2019

Would be good to add a unit test for the behavior of #464 that shows this sync point being returned properly when a rendition switch happens before the first segment download finishes

@gkatsev
Copy link
Member Author

gkatsev commented May 6, 2019

I can try looking into making a test for it. #464 is a bit more than just one rendition change, though, there's a bunch of them.

@squarebracket
Copy link
Contributor

I don't think you need multiple rendition switches, only the initial + 1. Can't you listen to mediachange on the tech to get the initial rendition switch, then disable that rendition, wait for the next mediachange on the tech, then see if you can get a sync point?

@gkatsev
Copy link
Member Author

gkatsev commented May 6, 2019

I can try doing the mediachange, but when I updated the example from #464 to wait for all the renditions to be available and then switch, it didn't repro, but it's possible it happened too early/too late at that point.

type: 'application/vnd.apple.mpegurl'
});
this.clock.tick(1);
this.player.play();
Copy link
Member Author

Choose a reason for hiding this comment

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

calling play or using autoplay causes us to be in a has played state by the time we switch the playlist, meaning the playlist won't get a syncinfo.

const levels = this.player.qualityLevels();

levels.one('change', () => {
this.player.tech_.hls.masterPlaylistController_.mainSegmentLoader_.fillBuffer_();
Copy link
Member Author

Choose a reason for hiding this comment

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

fillBuffer sets up the syncPoint if it can.

this.player.tech_.hls.masterPlaylistController_.mainSegmentLoader_.fillBuffer_();
assert.notEqual(this.player.tech_.hls.masterPlaylistController_.mainSegmentLoader_.syncPoint_, null);
// ignore warnings
this.env.log.warn.reset();
Copy link
Member Author

Choose a reason for hiding this comment

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

we don't respond to some manifests that are requests and so a warning is logged. Responding to them seems to cause this test to timeout but don't have time to spend figuring out why, instead, ignore these warnings.

@stale
Copy link

stale bot commented Jul 7, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the outdated label Jul 7, 2019
@stale
Copy link

stale bot commented Sep 8, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the outdated label Sep 8, 2019
@stale stale bot closed this Sep 15, 2019
@brandonocasey brandonocasey reopened this Sep 17, 2019
@stale
Copy link

stale bot commented Nov 16, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the outdated label Nov 16, 2019
@gkatsev gkatsev removed the outdated label Nov 22, 2019
gkatsev pushed a commit that referenced this pull request Jan 10, 2020
…oaded (#700)

* Fix live startup failures when play happens before playlist is downloaded

When joining a live stream, VHS starts playback at a time of 0,
regardless of how long the stream has been playing. This means that the
playlist will start with sync info of time 0 for the first media index.

However, if the stream "played" (either via API, autoplay, etc.) before
a playlist was downloaded, then after the playlist downloaded the
loader would reset the sync info, erasing the assumed time 0 for first
media index, and the player would request segments ad infinitum, never
able to place them in the timeline and get a new sync point.

This separates the notion of played between play initiation and playback
of content (progress on the timeline), in order to ensure that the
initial sync info is maintained.

* Use segment loader's state to determine when to update sync info

While using hasPlayedContent helped to alleviate most issues around
when to update the sync info in segment loader when updating a live
playlist, there still remained potential issues when a segment was
requested and the sync info was changed for the in-flight segment.

This change uses the segment loader's INIT state to determine if the
sync info should be updated, meaning in-flight segment requests keep
their sync info fixed.

Fixes #464, closes #496, closes #500.
@gkatsev
Copy link
Member Author

gkatsev commented Jan 10, 2020

Fixed as part of #700. Release soon.

@gkatsev gkatsev closed this Jan 10, 2020
@gkatsev gkatsev deleted the default-syncpoint branch January 10, 2020 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switching quality leads to infinite buffering
4 participants