-
Notifications
You must be signed in to change notification settings - Fork 424
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: race condition preventing qualityLevels from being populating #707
Changes from all commits
209934d
a84cc14
89171ab
fab164d
7abf951
99c5fd2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -92,6 +92,9 @@ let renditionSelectionMixin = function(hlsHandler) { | |
|
||
// Add a single API-specific function to the HlsHandler instance | ||
hlsHandler.representations = () => { | ||
if (!playlists || !playlists.master || !playlists.master.playlists) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably return an empty array if we don't have playlists yet, Right now we will just throw an error and some of the tests where failing due to this. |
||
return []; | ||
} | ||
return playlists | ||
.master | ||
.playlists | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -716,7 +716,7 @@ class HlsHandler extends Component { | |
this.tech_.trigger('progress'); | ||
}); | ||
|
||
this.tech_.ready(() => this.setupQualityLevels_()); | ||
this.setupQualityLevels_(); | ||
|
||
// do nothing if the tech has been disposed already | ||
// this can occur if someone sets the src in player.ready(), for instance | ||
|
@@ -737,17 +737,21 @@ class HlsHandler extends Component { | |
setupQualityLevels_() { | ||
let player = videojs.players[this.tech_.options_.playerId]; | ||
|
||
if (player && player.qualityLevels) { | ||
this.qualityLevels_ = player.qualityLevels(); | ||
// if there isn't a player or there isn't a qualityLevels plugin | ||
// or qualityLevels_ listeners have already been setup, do nothing. | ||
if (!player || !player.qualityLevels || this.qualityLevels_) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only setup quality levels listeners once, the rest of the code is the same except we return early instead of adding the logic in an |
||
return; | ||
} | ||
|
||
this.masterPlaylistController_.on('selectedinitialmedia', () => { | ||
handleHlsLoadedMetadata(this.qualityLevels_, this); | ||
}); | ||
this.qualityLevels_ = player.qualityLevels(); | ||
|
||
this.playlists.on('mediachange', () => { | ||
handleHlsMediaChange(this.qualityLevels_, this.playlists); | ||
}); | ||
} | ||
this.masterPlaylistController_.on('selectedinitialmedia', () => { | ||
handleHlsLoadedMetadata(this.qualityLevels_, this); | ||
}); | ||
|
||
this.playlists.on('mediachange', () => { | ||
handleHlsMediaChange(this.qualityLevels_, this.playlists); | ||
}); | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3094,8 +3094,7 @@ QUnit.test('passes useCueTags hls option to master playlist controller', functio | |
videojs.options.hls = origHlsOptions; | ||
}); | ||
|
||
// TODO: This test fails intermittently. Turn on when fixed to always pass. | ||
QUnit.skip('populates quality levels list when available', function(assert) { | ||
QUnit.test('populates quality levels list when available', function(assert) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test was previously skipped because of intermittent failures. I think fixing this bug fixes the test. |
||
this.player.src({ | ||
src: 'manifest/master.m3u8', | ||
type: 'application/vnd.apple.mpegurl' | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,7 @@ | |
|
||
<!-- player stats visualization --> | ||
<link href="stats.css" rel="stylesheet"> | ||
<script src="/node_modules/d3/d3.min.js"></script> | ||
<script src="../../node_modules/d3/d3.min.js"></script> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. have to use local paths to get it to work on netlify. |
||
|
||
<style> | ||
body { | ||
|
@@ -345,25 +345,23 @@ <h3>Timed Metadata</h3> | |
videojs.Hls.displayStats(document.querySelector('.switching-stats'), player); | ||
videojs.Hls.displayCues(document.querySelector('.segment-timeline'), player); | ||
|
||
player.ready(function() { | ||
var qualityLevels = player.qualityLevels(); | ||
var qualityLevels = player.qualityLevels(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. quality levels can exist before tech/player ready |
||
|
||
qualityLevels.on('addqualitylevel', function(event) { | ||
createQualityButton(event.qualityLevel, qualityButtons); | ||
}); | ||
qualityLevels.on('addqualitylevel', function(event) { | ||
createQualityButton(event.qualityLevel, qualityButtons); | ||
}); | ||
|
||
qualityLevels.on('change', function(event) { | ||
for (var i = 0; i < qualityLevels.length; i++) { | ||
var level = qualityLevels[i]; | ||
var button = document.getElementById('quality-level-' + level.id); | ||
qualityLevels.on('change', function(event) { | ||
for (var i = 0; i < qualityLevels.length; i++) { | ||
var level = qualityLevels[i]; | ||
var button = document.getElementById('quality-level-' + level.id); | ||
|
||
button.classList.remove('selected'); | ||
} | ||
button.classList.remove('selected'); | ||
} | ||
|
||
var selected = qualityLevels[event.selectedIndex]; | ||
var button = document.getElementById('quality-level-' + selected.id); | ||
button.classList.add('selected'); | ||
}); | ||
var selected = qualityLevels[event.selectedIndex]; | ||
var button = document.getElementById('quality-level-' + selected.id); | ||
button.classList.add('selected'); | ||
}); | ||
}); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
naming files what getting a bit absurd in here