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

Fix a segment hop in live #928

Merged
merged 2 commits into from
Dec 21, 2016
Merged

Conversation

gesinger
Copy link
Contributor

@gesinger gesinger commented Dec 1, 2016

Description

In a case where the live playlist updates during processing of the first segment request, it is possible that we'd skip over the next segment and proceed down the playlist. This accounts for the change in media sequence during update end.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
  • Reviewed by Two Core Contributors

In a case where the live playlist updates during processing of the first segment request, it is possible that we'd skip over the next segment and proceed down the playlist. This accounts for the change in media sequence during update end.
@mjneil
Copy link
Contributor

mjneil commented Dec 16, 2016

LGTM (jumped the gun a bit on this one)

let currentMediaIndex = segmentInfo.mediaIndex;

currentMediaIndex +=
segmentInfo.playlist.mediaSequence - this.playlist_.mediaSequence;

if (!segmentInfo.isSyncRequest) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we actually get to this check if the segment is a sync request? In handleSegment_ we return if it is a sync request and never append, so I would think this check is unnecessary.

Copy link
Member

Choose a reason for hiding this comment

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

I think Matt means "Can we actually get rid of this check" which I agree with!

@mjneil
Copy link
Contributor

mjneil commented Dec 21, 2016

LGTM official this time

@imbcmdth imbcmdth merged commit 2f9c5a0 into videojs:master Dec 21, 2016
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