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

Fine tune blacklist handling #1269

Merged
merged 2 commits into from
Oct 16, 2017

Conversation

mjneil
Copy link
Contributor

@mjneil mjneil commented Oct 3, 2017

Description

Currently it doesn't really matter whether a representation is disabled through user interaction via the representations api, or blacklisted internally because of network or playback errors. This can cause undefined references in our playlist selector (#1206 #1267) when whatever playlists have not been manually disabled are all blacklisted internally due to errors.

Specific Changes proposed

  • never allow playlist selector to select a playlist that has been permanently blacklisted due to incompatible configuration
  • When filtering playlists within the playlist selectors, if there are no enabled playlists (i.e. not blacklisted internally AND not disabled by the user) available, then fall back to using the list of playlists not disabled by the user regardless of blacklist state.
  • make sure playlists blacklisted from an illegal media switch is permanently blacklisted, as there is no reason to try it again at a later time.
  • The representation api will return a list that filters out just incompatible playlists instead of both incompatible playlists and temporary blacklisted playlists.

Copy link
Contributor

@OshinKaramian OshinKaramian left a comment

Choose a reason for hiding this comment

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

Minor test comments.

@@ -487,7 +510,9 @@ Playlist.duration = duration;
Playlist.seekable = seekable;
Playlist.getMediaInfoForTime = getMediaInfoForTime;
Playlist.isEnabled = isEnabled;
Playlist.isDisabled = isDisabled;
Copy link
Contributor

Choose a reason for hiding this comment

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

Might hit a merge conflict due to: #1271.

Copy link
Contributor Author

@mjneil mjneil Oct 16, 2017

Choose a reason for hiding this comment

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

Ya we'll just merge that one first and i can rebase this or vice versa

playlist.excludeUntil = Infinity;
assert.notOk(Playlist.isEnabled(playlist), 'playlist is not enabled');

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.

Could we maybe break this test up in some way? Maybe one for if playerlist.disabled is set to true? Or maybe based on the expectation (should be enabled vs should not be enabled)?

It also might be safer to not maintain the const playlist here and instead pass the raw values for playlist in. As an example:

playlist.excludeUntil = Date.now() - 1;
assert.ok(Playlist.isEnabled(playlist), 'playlist is enabled');

Just becomes:

assert.ok(Playlist.isEnabled({ excludeUntil: Date.now() - 1 }), 'playlist is enabled');

This way you don't need to worry about deleting things and the order of the tests won't matter.

// incompatible means that the playlist was blacklisted due to incompatible
// configuration e.g. audio only stream when trying to playback audio and video.
// incompaatibility is denoted by a blacklist of Infinity.
const playlist = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to my comment below, you could just get rid of const playlist and pass the raw values into the method under test.

@mjneil mjneil merged commit ccf61d5 into videojs:master Oct 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants