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: Clear the blacklist for other playlists if final rendition errors #396

Merged
merged 5 commits into from
Apr 1, 2019

Conversation

forbesjo
Copy link
Contributor

@forbesjo forbesjo commented Feb 7, 2019

videojs/videojs-contrib-hls#1083

Description

Before the behavior is we never blacklist the final request and wait up to 5 minutes for other playlists' blacklist duration to be cleared. Change the behavior as follows:

  • If there is only one rendition and it errors, never blacklist it
  • if there are multiple renditions and we are down to the last one (the previous renditions are all blacklisted) and it errors, we clear the blacklist duration for the other playlists, and retry the best available playlist after a delay.

Requirements Checklist

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

videojs.log.warn('Removing all playlists from the blacklist because the last ' +
'rendition is about to be blacklisted.');
playlists.forEach((playlist) => {
delete playlist.excludeUntil;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this was a bug from the original PR, but we need to check to ensure we don't clear incompatible playlists from the blacklist (where excludeUntil === Infinity).

@gkatsev
Copy link
Member

gkatsev commented Mar 22, 2019

Good to go once we ignore excludeUntil === Infinity

@gkatsev gkatsev self-assigned this Apr 1, 2019
@gkatsev
Copy link
Member

gkatsev commented Apr 1, 2019

Not sure why the test for commit f805916 is showing as passed, it failed as you can see here: https://travis-ci.org/videojs/http-streaming/builds/514306305#L2492. However, it's supposed to fail to show that the test works as expected. The build for the fix commit (b37e4fc) is here: https://travis-ci.org/videojs/http-streaming/builds/514308525.

@gkatsev
Copy link
Member

gkatsev commented Apr 1, 2019

@gkatsev gkatsev requested a review from gesinger April 1, 2019 19:14
@forbesjo
Copy link
Contributor Author

forbesjo commented Apr 1, 2019

Looks good to me

@gkatsev gkatsev merged commit 6e6c8c2 into master Apr 1, 2019
@gkatsev gkatsev deleted the blacklist branch April 1, 2019 19:50
gkatsev added a commit that referenced this pull request Apr 16, 2019
gkatsev added a commit that referenced this pull request Apr 16, 2019
brandonocasey pushed a commit that referenced this pull request Apr 25, 2019
#396)

Ignore unsupported renditions (those with an excludeUntil of Infinity)
gkatsev pushed a commit that referenced this pull request Apr 26, 2019
#479)

Ignore unsupported renditions (those with an excludeUntil of Infinity)

This time use a separate timer for the final rendition.

Second try at #396. Reverts #471.
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.

4 participants