fix: race condition preventing qualityLevels from being populating #707
Conversation
brandonocasey
changed the title
brandonocasey
changed the title
brandonocasey
changed the title
| const files = [vjs, vjsCss, eme]; | ||
|
|
||
| const files = [ | ||
| 'node_modules/video.js/dist/video-js.css', |
There was a problem hiding this comment.
naming files what getting a bit absurd in here
| @@ -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.
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.
| 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.
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 if scope.
| @@ -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.
have to use local paths to get it to work on netlify.
| @@ -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.
quality levels can exist before tech/player ready
| @@ -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.
This test was previously skipped because of intermittent failures. I think fixing this bug fixes the test.
scripts/netlify.js
Outdated
| @@ -20,5 +22,5 @@ files | |||
|
|
|||
| // copy over files, dist, and html files | |||
There was a problem hiding this comment.
Minor, but this comment is out-of-date. May be worth just removing it.
There was a problem hiding this comment.
updated the comment to make it more relevant
|
This fixes #677 as well. |
|
@brandonocasey can you make this change against master as well? Thanks! |
Description
Quality levels is not being populated due to a race condition where the tech is not ready until after we have fired
selectedintialmedia. You can get this to happen by in chrome by loading the tab in the background for 10s or by using a slow browser like ie 11, where it will happen in the main tab.Steps to reproduce
index.htmlStatsin a new page, and wait 10s before switching to that tab.qualityLevelsdo not show up on the page, andplayer.qualityLevels().levels_is a 0 length arrayplayer.representations()has the correct representations in the returned array.qualityLevelsbuttons do not show up on the page.Specific Changes proposed
this.setupQualityLevels_()Testing
Requirements Checklist
Closes #708
Fixes #677