Dorska/quality selector #556

adorsk · 2018-04-19T14:47:52Z

What are the relevant tickets?

What's this PR do?

Adds a new control to the VideoPlayer control bar that allows the user to select a video quality setting.

How should this be manually tested?

Ensure that you have a video that's at least a few minutes long. I'm a fan of https://ia802702.us.archive.org/16/items/fireplace/FireplaceGlenLakeMi.11-06-07_512kb.mp4 .
Check that the selected bitrate changes after ten seconds:
1. Open up dev tools, and activate (1) 'Network Conditions' section and (2) the 'Network' tab.
2. Start the video playing. you should start to see lines in the Network tab like video_...-2gfh90...ts . The "2gfh90" indicates that the highest bitrate is being used.
3. Let ten seconds go by, and then in the 'Network Conditions' tab, set 'Network throttling' to a slower value (like 'slow 3G').
4. Ensure that you see the lines in the 'Network' tab change. Probably to something like video_...-06dkm6...ts.
Check that the you can set the bitrate.
1. Reload the page, and start the video playing again.
2. Let ten seconds go by.
3. Now click the 'HD' button in the video player controls and select a value.
4. Ensure that the 'Network' tab shows different .ts lines.
5. Ensure that different selections in the menu result in different .ts lines.
Check that the auto bitrate works.
1. Reload the page, and start the video playing again.
2. Let ten seconds go by.
3. Now click the 'HD' button in the video player controls and select a value.
4. Then select the 'auto' value.
5. Let ten seconds go by, and then in the 'Network Conditions' tab, set 'Network throttling' to a slower value (like 'slow 3G').
6. Ensure that you see different .ts lines.
7. Disable the 'Network throttling', and ensure that you see different .ts lines.

Any background context you want to provide?

Normally videojs adaptively determines which bitrate to use, per:
https://github.com/videojs/videojs-contrib-hls/blob/master/docs/bitrate-switching.md
We can override the normal behavior by toggling active bitrates. The videojs-contrib-quality-levels plugin provides an API for doing this toggling.
And in this PR we add a control that allows humans to interact with that API.

codecov-io · 2018-04-19T15:03:34Z

Codecov Report

Merging #556 into master will decrease coverage by 0.93%.
The diff coverage is 35.29%.

@@            Coverage Diff             @@
##           master     #556      +/-   ##
==========================================
- Coverage    95.9%   94.96%   -0.94%     
==========================================
  Files         151      152       +1     
  Lines        5465     5542      +77     
  Branches      220      220              
==========================================
+ Hits         5241     5263      +22     
- Misses        189      244      +55     
  Partials       35       35

Impacted Files	Coverage Δ
webpack.config.shared.js	`100% <ø> (ø)`	⬆️
static/js/lib/video.js	`96.55% <100%> (+0.39%)`	⬆️
static/js/components/VideoPlayer_test.js	`99.5% <100%> (+0.04%)`	⬆️
static/js/components/VideoPlayer.js	`93.5% <100%> (+0.04%)`	⬆️
static/js/lib/videojs_hls_quality_selector.js	`3.5% <3.5%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39dca4b...4fdb76f. Read the comment docs.

pdpinch · 2018-04-23T13:30:18Z

@adorsk if this is ready for review, can you add the "Needs Review" label? That's our most reliable signal that a PR is ready.

mbertrand · 2018-04-23T15:59:03Z

requirements.txt

+-e git+https://github.com/mitodl/django-elastic-transcoder.git#egg=django-elastic-transcoder
+-e git+https://github.com/Brown-University-Library/django-shibboleth-remoteuser.git#egg=django-shibboleth-remoteuser
+-e git+https://github.com/mitodl/mit-moira.git#egg=mit-moira
+-e git+https://github.com/mitodl/pycaption.git#egg=pycaption


Can you revert the changes to this file?

mbertrand · 2018-04-23T16:15:35Z

static/js/components/VideoPlayer.js

      return _.last(playlists)
    }
-    // Return playlist with highest bandwidth <= system bandwidth


This overrides the native HLS algorithm so that the video player's size in the browser is ignored; and should still be used unless someone manually selects a quality level.

mbertrand · 2018-04-23T16:16:01Z

static/js/components/VideoPlayer.js

@@ -339,7 +337,12 @@ class VideoPlayer extends React.Component<*, void> {
          })
        })
        if (this.tech_.hls !== undefined) {
-          this.tech_.hls.selectPlaylist = selectPlaylist
+          const _originalSelectPlaylist = this.tech_.hls.selectPlaylist


This was overriden in a previous PR so that the size of the video in the browser is ignored when determining the optimal resolution to play. Can you revert to using that custom algorithm instead of the default one?

Ah, I see what you mean.

Updated to use a modified version of the previous custom algorithm. The modification is: the algorithm will now select the highest active playlist that is less than the available system bandwidth, or the first active playlist if no active playlist is <= system bandwidth.

The quality selector widget sets all non-selected playlists to disabled. If 'auto' is selected it sets all playlists to active.

mbertrand · 2018-04-23T18:40:25Z

static/js/components/VideoPlayer.js

    )
  }

+  selectInitialQualityLevel() {


This function doesn't seem to be used, can it be removed?

ah, nice catch. removed.

mbertrand · 2018-04-23T18:42:53Z

static/js/lib/videojs_hls_quality_selector.js

@@ -0,0 +1,135 @@
+/* Adapted from https://github.com/chrisboustead/videojs-hls-quality-selector .


Would it be possible to bring in/adapt some of the unit tests for this plugin too, to improve coverage?

I wasn't sure on whether this would be worth it. The original plugin just tests that it can register against videojs:
https://github.com/chrisboustead/videojs-hls-quality-selector/blob/master/test/plugin.test.js

We could write tests, but I think it would mostly look like a lot of mocking for videojs functions.

Do you think it's worth pursuing? Either way is fine with me.

Ok with me to skip it if it won't add much value.

…m pip 10.0.1

adorsk · 2018-04-24T15:42:58Z

@mbertrand reverted requirements.txt to take out -e flags, should work with travis now per bug fix from pip 10.0.1 (pypa/pip#5251).

Ready for review again.

mbertrand

👍

adorsk · 2018-04-24T18:00:21Z

Closed #377.

adorsk and others added 2 commits April 18, 2018 16:42

initial work on quality selector button

6c870de

test/format updates

0086746

adorsk requested a review from mbertrand April 19, 2018 14:48

fix pip string for pip 10 (which tox force installs >:( )

0285493

adorsk added the Needs Review label Apr 23, 2018

mbertrand self-assigned this Apr 23, 2018

mbertrand requested changes Apr 23, 2018

View reviewed changes

mbertrand added Waiting on Author and removed Needs Review labels Apr 23, 2018

change playlist selector to select highest available active playlist

e8cab7c

adorsk force-pushed the dorska/quality_selector branch from 82de554 to e8cab7c Compare April 23, 2018 18:10

mbertrand requested changes Apr 23, 2018

View reviewed changes

adorsk added 2 commits April 23, 2018 15:52

remove defunct fn

8b661ea

revert '-e' changes for requirements, no need for '-e' w/ bug fix fro…

4fdb76f

…m pip 10.0.1

adorsk added Needs Review and removed Waiting on Author labels Apr 24, 2018

mbertrand approved these changes Apr 24, 2018

View reviewed changes

adorsk merged commit a109d38 into master Apr 24, 2018

adorsk deleted the dorska/quality_selector branch April 24, 2018 17:59

adorsk removed the Needs Review label Apr 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dorska/quality selector #556

Dorska/quality selector #556

adorsk commented Apr 19, 2018

codecov-io commented Apr 19, 2018 •

edited

Loading

pdpinch commented Apr 23, 2018

mbertrand Apr 23, 2018

mbertrand Apr 23, 2018

mbertrand Apr 23, 2018

adorsk Apr 23, 2018

mbertrand Apr 23, 2018

adorsk Apr 23, 2018

mbertrand Apr 23, 2018

adorsk Apr 23, 2018

mbertrand Apr 23, 2018

adorsk commented Apr 24, 2018

mbertrand left a comment

adorsk commented Apr 24, 2018

		@@ -0,0 +1,135 @@
		/* Adapted from https://github.com/chrisboustead/videojs-hls-quality-selector .

Dorska/quality selector #556

Dorska/quality selector #556

Conversation

adorsk commented Apr 19, 2018

What are the relevant tickets?

What's this PR do?

How should this be manually tested?

Any background context you want to provide?

codecov-io commented Apr 19, 2018 • edited Loading

Codecov Report

pdpinch commented Apr 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adorsk commented Apr 24, 2018

mbertrand left a comment

Choose a reason for hiding this comment

adorsk commented Apr 24, 2018

codecov-io commented Apr 19, 2018 •

edited

Loading