Hls.GOAL_BUFFER_LENGTH can override GOAL_BUFFER_LENGTH again (#686)

* add tests for all options in HLS
* add configuration hierarchy testing
* Hls.GOAL_BUFFER_LENGTH is settable to a number greater than 0 with a warning
* use Hls.GOAL_BUFFER_LENGTH if it exists
* added unit tests to verify warning logs
* fixed comment about Mbps vs MB/s

+export default {
+  GOAL_BUFFER_LENGTH: 30
+};

@@ -6,13 +6,11 @@ import {getMediaIndexForTime_ as getMediaIndexForTime, duration} from './playlis
 import videojs from 'video.js';
 import SourceUpdater from './source-updater';
 import {Decrypter} from './decrypter';
+import Config from './config';
 
 // in ms
 const CHECK_BUFFER_DELAY = 500;
 
-// the desired length of video to maintain in the buffer, in seconds
-export const GOAL_BUFFER_LENGTH = 30;
-
 /**
  * Updates segment with information about its end-point in time and, optionally,
  * the segment duration if we have enough information to determine a segment duration
@@ -78,8 +76,9 @@ const detectEndOfStream = function(playlist, mediaSource, segmentIndex, currentB
     (appendedLastSegment || bufferedToEnd);
 };
 
-/*  Turns segment byterange into a string suitable for use in
- *  HTTP Range requests
+/**
+ * Turns segment byterange into a string suitable for use in
+ * HTTP Range requests
  */
 const byterangeStr = function(byterange) {
   let byterangeStart;
@@ -92,7 +91,8 @@ const byterangeStr = function(byterange) {
   return 'bytes=' + byterangeStart + '-' + byterangeEnd;
 };
 
-/*  Defines headers for use in the xhr request for a particular segment.
+/**
+ * Defines headers for use in the xhr request for a particular segment.
  */
 const segmentXhrHeaders = function(segment) {
   let headers = {};
@@ -385,7 +385,7 @@ export default class SegmentLoader extends videojs.EventTarget {
 
       // if there is plenty of content buffered, and the video has
       // been played before relax for awhile
-      if (this.hasPlayed_() && bufferedTime >= GOAL_BUFFER_LENGTH) {
+      if (this.hasPlayed_() && bufferedTime >= Config.GOAL_BUFFER_LENGTH) {
         return null;
       }
       mediaIndex = getMediaIndexForTime(playlist,

@@ -14,6 +14,7 @@ import {MediaSource, URL} from 'videojs-contrib-media-sources';
 import m3u8 from './m3u8';
 import videojs from 'video.js';
 import MasterPlaylistController from './master-playlist-controller';
+import Config from './config';
 
 /**
  * determine if an object a is differnt from
@@ -52,8 +53,23 @@ const Hls = {
   xhr: xhrFactory()
 };
 
-// the desired length of video to maintain in the buffer, in seconds
-Hls.GOAL_BUFFER_LENGTH = 30;
+Object.defineProperty(Hls, 'GOAL_BUFFER_LENGTH', {
+  get() {
+    videojs.log.warn('using Hls.GOAL_BUFFER_LENGTH is UNSAFE be sure ' +
+                     'you know what you are doing');
+    return Config.GOAL_BUFFER_LENGTH;
+  },
+  set(v) {
+    videojs.log.warn('using Hls.GOAL_BUFFER_LENGTH is UNSAFE be sure ' +
+                     'you know what you are doing');
+    if (typeof v !== 'number' || v <= 0) {
+      videojs.log.warn('value passed to Hls.GOAL_BUFFER_LENGTH ' +
+                       'must be a number and greater than 0');
+      return;
+    }
+    Config.GOAL_BUFFER_LENGTH = v;
+  }
+});
 
 // A fudge factor to apply to advertised playlist bitrates to account for
 // temporary flucations in client bandwidth
@@ -283,14 +299,13 @@ class HlsHandler extends Component {
       }
     }
 
-    this.options_ = videojs.mergeOptions(videojs.options.hls || {}, options.hls);
     this.tech_ = tech;
     this.source_ = source;
 
-    // start playlist selection at a reasonable bandwidth for
-    // broadband internet
-    // 0.5 Mbps
-    this.bandwidth = this.options_.bandwidth || 4194304;
+    // handle global & Source Handler level options
+    this.options_ = videojs.mergeOptions(videojs.options.hls || {}, options.hls);
+    this.setOptions_();
+
     this.bytesReceived = 0;
 
     // listen for fullscreenchange events for this player so that we
@@ -325,6 +340,24 @@ class HlsHandler extends Component {
     this.on(this.tech_, 'play', this.play);
   }
 
+  setOptions_() {
+    // defaults
+    this.options_.withCredentials = this.options_.withCredentials || false;
+
+    // start playlist selection at a reasonable bandwidth for
+    // broadband internet
+    // 0.5 MB/s
+    this.options_.bandwidth = this.options_.bandwidth || 4194304;
+
+    // grab options passed to player.src
+    ['withCredentials', 'bandwidth'].forEach((option) => {
+      if (typeof this.source_[option] !== 'undefined') {
+        this.options_[option] = this.source_[option];
+      }
+    });
+
+    this.bandwidth = this.options_.bandwidth;
+  }
   /**
    * called when player.src gets called, handle a new source
    *
@@ -335,17 +368,13 @@ class HlsHandler extends Component {
     if (!src) {
       return;
     }
-
-    ['withCredentials', 'bandwidth'].forEach((option) => {
-      if (typeof this.source_[option] !== 'undefined') {
-        this.options_[option] = this.source_[option];
-      }
-    });
+    this.setOptions_();
+    // add master playlist controller options
     this.options_.url = this.source_.src;
     this.options_.tech = this.tech_;
     this.options_.externHls = Hls;
-    this.options_.bandwidth = this.bandwidth;
     this.masterPlaylistController_ = new MasterPlaylistController(this.options_);
+
     // `this` in selectPlaylist should be the HlsHandler for backwards
     // compatibility with < v2
     this.masterPlaylistController_.selectPlaylist =

+import QUnit from 'qunit';
+import {
+  createPlayer,
+  useFakeEnvironment,
+  openMediaSource,
+  useFakeMediaSource
+} from './test-helpers.js';
+import videojs from 'video.js';
+
+/* eslint-disable no-unused-vars */
+// we need this so that it can register hls with videojs
+import {HlsSourceHandler, HlsHandler, Hls} from '../src/videojs-contrib-hls';
+/* eslint-enable no-unused-vars */
+import Config from '../src/config';
+
+// list of posible options
+// name - the proprety name
+// default - the default value
+// test - alternative value to verify that default is not used
+// alt - another alternative value to very that test/default are not used
+const options = [{
+  name: 'withCredentials',
+  default: false,
+  test: true,
+  alt: false
+}, {
+  name: 'bandwidth',
+  default: 4194304,
+  test: 5,
+  alt: 555
+}];
+
+QUnit.module('Configuration - Deprication', {
+  beforeEach() {
+    this.env = useFakeEnvironment();
+    this.requests = this.env.requests;
+    this.mse = useFakeMediaSource();
+    this.clock = this.env.clock;
+    this.old = {};
+    this.old.GOAL_BUFFER_LENGTH = Config.GOAL_BUFFER_LENGTH;
+    // force the HLS tech to run
+    this.old.NativeHlsSupport = videojs.Hls.supportsNativeHls;
+    videojs.Hls.supportsNativeHls = false;
+  },
+
+  afterEach() {
+    Config.GOAL_BUFFER_LENGTH = this.old.GOAL_BUFFER_LENGTH;
+
+    this.env.restore();
+    this.mse.restore();
+    videojs.Hls.supportsNativeHls = this.old.NativeHlsSupport;
+  }
+});
+
+QUnit.test('GOAL_BUFFER_LENGTH get warning', function() {
+  QUnit.equal(Hls.GOAL_BUFFER_LENGTH,
+              Config.GOAL_BUFFER_LENGTH,
+              'Hls.GOAL_BUFFER_LENGTH returns the default');
+  QUnit.equal(this.env.log.warn.calls, 1, 'logged a warning');
+});
+
+QUnit.test('GOAL_BUFFER_LENGTH set warning', function() {
+  Hls.GOAL_BUFFER_LENGTH = 10;
+  QUnit.equal(this.env.log.warn.calls, 1, 'logged a warning');
+
+  QUnit.equal(Config.GOAL_BUFFER_LENGTH, 10, 'returns what we set it to');
+});
+
+QUnit.test('GOAL_BUFFER_LENGTH set warning and invalid', function() {
+  Hls.GOAL_BUFFER_LENGTH = 'nope';
+  QUnit.equal(this.env.log.warn.calls, 2, 'logged two warnings');
+
+  QUnit.equal(Config.GOAL_BUFFER_LENGTH, 30, 'default');
+
+  Hls.GOAL_BUFFER_LENGTH = 0;
+  QUnit.equal(this.env.log.warn.calls, 2, 'logged two warnings');
+
+  QUnit.equal(Config.GOAL_BUFFER_LENGTH, 30, 'default');
+});
+
+QUnit.module('Configuration - Options', {
+  beforeEach() {
+    this.env = useFakeEnvironment();
+    this.requests = this.env.requests;
+    this.mse = useFakeMediaSource();
+    this.clock = this.env.clock;
+    this.old = {};
+
+    // force the HLS tech to run
+    this.old.NativeHlsSupport = videojs.Hls.supportsNativeHls;
+    videojs.Hls.supportsNativeHls = false;
+  },
+
+  afterEach() {
+    this.env.restore();
+    this.mse.restore();
+    videojs.Hls.supportsNativeHls = this.old.NativeHlsSupport;
+
+    this.player.dispose();
+    videojs.options.hls = {};
+
+  }
+});
+
+options.forEach((opt) => {
+  QUnit.test(`default ${opt.name}`, function() {
+    this.player = createPlayer();
+    this.player.src({
+      src: 'http://example.com/media.m3u8',
+      type: 'application/vnd.apple.mpegurl'
+    });
+
+    let hls = this.player.tech_.hls;
+
+    openMediaSource(this.player, this.clock);
+    QUnit.equal(hls.options_[opt.name],
+                opt.default,
+                `${opt.name} should be default`);
+  });
+
+  QUnit.test(`global ${opt.name}`, function() {
+    videojs.options.hls[opt.name] = opt.test;
+    this.player = createPlayer();
+    this.player.src({
+      src: 'http://example.com/media.m3u8',
+      type: 'application/vnd.apple.mpegurl'
+    });
+
+    let hls = this.player.tech_.hls;
+
+    openMediaSource(this.player, this.clock);
+    QUnit.equal(hls.options_[opt.name],
+                opt.test,
+                `${opt.name} should be equal to global`);
+  });
+
+  QUnit.test(`sourceHandler ${opt.name}`, function() {
+    let sourceHandlerOptions = {html5: {hls: {}}};
+
+    sourceHandlerOptions.html5.hls[opt.name] = opt.test;
+    this.player = createPlayer(sourceHandlerOptions);
+    this.player.src({
+      src: 'http://example.com/media.m3u8',
+      type: 'application/vnd.apple.mpegurl'
+    });
+
+    let hls = this.player.tech_.hls;
+
+    openMediaSource(this.player, this.clock);
+    QUnit.equal(hls.options_[opt.name],
+                opt.test,
+                `${opt.name} should be equal to sourceHandler Option`);
+  });
+
+  QUnit.test(`src ${opt.name}`, function() {
+    let srcOptions = {
+      src: 'http://example.com/media.m3u8',
+      type: 'application/vnd.apple.mpegurl'
+    };
+
+    srcOptions[opt.name] = opt.test;
+    this.player = createPlayer();
+    this.player.src(srcOptions);
+
+    let hls = this.player.tech_.hls;
+
+    openMediaSource(this.player, this.clock);
+    QUnit.equal(hls.options_[opt.name],
+                opt.test,
+                `${opt.name} should be equal to src option`);
+  });
+
+  QUnit.test(`srcHandler overrides global ${opt.name}`, function() {
+    let sourceHandlerOptions = {html5: {hls: {}}};
+
+    sourceHandlerOptions.html5.hls[opt.name] = opt.test;
+    videojs.options.hls[opt.name] = opt.alt;
+    this.player = createPlayer(sourceHandlerOptions);
+    this.player.src({
+      src: 'http://example.com/media.m3u8',
+      type: 'application/vnd.apple.mpegurl'
+    });
+
+    let hls = this.player.tech_.hls;
+
+    openMediaSource(this.player, this.clock);
+    QUnit.equal(hls.options_[opt.name],
+                opt.test,
+                `${opt.name} should be equal to sourchHandler option`);
+  });
+
+  QUnit.test(`src overrides sourceHandler ${opt.name}`, function() {
+    let sourceHandlerOptions = {html5: {hls: {}}};
+    let srcOptions = {
+      src: 'http://example.com/media.m3u8',
+      type: 'application/vnd.apple.mpegurl'
+    };
+
+    sourceHandlerOptions.html5.hls[opt.name] = opt.alt;
+    srcOptions[opt.name] = opt.test;
+    this.player = createPlayer(sourceHandlerOptions);
+    this.player.src(srcOptions);
+
+    let hls = this.player.tech_.hls;
+
+    openMediaSource(this.player, this.clock);
+    QUnit.equal(hls.options_[opt.name],
+                opt.test,
+                `${opt.name} should be equal to sourchHandler option`);
+  });
+});
+
+QUnit.module('Configuration - Global Only', {
+  beforeEach() {
+    videojs.options.hls = {};
+  },
+
+  afterEach() {
+    videojs.options.hls = {};
+  }
+});
+
+QUnit.test('global mode override - flash', function() {
+  videojs.options.hls.mode = 'flash';
+  let htmlSourceHandler = new HlsSourceHandler('html5');
+  let flashSourceHandler = new HlsSourceHandler('flash');
+
+  QUnit.equal(
+    htmlSourceHandler.canHandleSource({type: 'application/x-mpegURL'}),
+    false,
+    'Cannot play html as we are overriden not to');
+
+  QUnit.equal(
+    flashSourceHandler.canHandleSource({type: 'application/x-mpegURL'}),
+    true,
+    'Can play flash as it is supported and overides allow');
+});
+
+QUnit.test('global mode override - html', function() {
+  videojs.options.hls.mode = 'html5';
+  let htmlSourceHandler = new HlsSourceHandler('html5');
+  let flashSourceHandler = new HlsSourceHandler('flash');
+
+  QUnit.equal(
+    htmlSourceHandler.canHandleSource({type: 'application/x-mpegURL'}),
+    true,
+    'Can play html as we support it and overides allow');
+
+  QUnit.equal(
+    flashSourceHandler.canHandleSource({type: 'application/x-mpegURL'}),
+    false,
+    'Cannot play flash as we are overiden not to');
+});
+

 import QUnit from 'qunit';
-import {GOAL_BUFFER_LENGTH, default as SegmentLoader} from '../src/segment-loader';
+import SegmentLoader from '../src/segment-loader';
 import videojs from 'video.js';
 import xhrFactory from '../src/xhr';
-import { useFakeEnvironment, useFakeMediaSource } from './test-helpers.js';
+import {useFakeEnvironment, useFakeMediaSource} from './test-helpers.js';
+import Config from '../src/config';
 
 const playlistWithDuration = function(time, conf) {
   let result = {
@@ -184,13 +185,13 @@ QUnit.test('regularly checks the buffer while unpaused', function() {
   this.requests[0].response = new Uint8Array(10).buffer;
   this.requests.shift().respond(200, null, '');
   sourceBuffer.buffered = videojs.createTimeRanges([[
-    0, GOAL_BUFFER_LENGTH
+    0, Config.GOAL_BUFFER_LENGTH
   ]]);
   sourceBuffer.trigger('updateend');
   QUnit.equal(this.requests.length, 0, 'no outstanding requests');
 
   // play some video to drain the buffer
-  currentTime = GOAL_BUFFER_LENGTH;
+  currentTime = Config.GOAL_BUFFER_LENGTH;
   this.clock.tick(10 * 1000);
   QUnit.equal(this.requests.length, 1, 'requested another segment');
 });
@@ -917,6 +918,18 @@ QUnit.test('key request timeouts reset bandwidth', function() {
   QUnit.ok(isNaN(loader.roundTrip), 'reset round trip time');
 });
 
+QUnit.test('GOAL_BUFFER_LENGTH changes to 1 segment ' +
+           ' which is already buffered, no new request is formed', function() {
+  Config.GOAL_BUFFER_LENGTH = 1;
+  loader.mimeType(this.mimeType);
+  let segmentInfo = loader.checkBuffer_(videojs.createTimeRanges([[0, 1]]),
+                                        playlistWithDuration(20),
+                                        0);
+
+  QUnit.ok(!segmentInfo, 'no request generated');
+  Config.GOAL_BUFFER_LENGTH = 30;
+});
+
 QUnit.module('Segment Loading Calculation', {
   beforeEach() {
     this.env = useFakeEnvironment();
@@ -969,7 +982,7 @@ QUnit.test('does not download the next segment if the buffer is full', function(
   loader.mimeType(this.mimeType);
 
   buffered = videojs.createTimeRanges([
-    [0, 15 + GOAL_BUFFER_LENGTH]
+    [0, 15 + Config.GOAL_BUFFER_LENGTH]
   ]);
   segmentInfo = loader.checkBuffer_(buffered, playlistWithDuration(30), 15);
 
@@ -1067,7 +1080,7 @@ QUnit.test('adjusts calculations based on expired time', function() {
 
   segmentInfo = loader.checkBuffer_(buffered,
                                     playlist,
-                                    40 - GOAL_BUFFER_LENGTH);
+                                    40 - Config.GOAL_BUFFER_LENGTH);
 
   QUnit.ok(segmentInfo, 'fetched a segment');
   QUnit.equal(segmentInfo.uri, '2.ts', 'accounted for expired time');

@@ -1145,39 +1145,6 @@ QUnit.test('if withCredentials global option is used, withCredentials is set on
   videojs.options.hls = hlsOptions;
 });
 
-QUnit.test('if withCredentials src option is used, withCredentials is set on the XHR object', function() {
-  this.player.dispose();
-  this.player = createPlayer();
-  this.player.src({
-    src: 'http://example.com/media.m3u8',
-    type: 'application/vnd.apple.mpegurl',
-    withCredentials: true
-  });
-  openMediaSource(this.player, this.clock);
-  QUnit.ok(this.requests[0].withCredentials,
-           'with credentials should be set to true if that option is passed in');
-});
-
-QUnit.test('src level credentials supersede the global options', function() {
-  let hlsOptions = videojs.options.hls;
-
-  this.player.dispose();
-  videojs.options.hls = {
-    withCredentials: false
-  };
-
-  this.player = createPlayer();
-  this.player.src({
-    src: 'http://example.com/media.m3u8',
-    type: 'application/vnd.apple.mpegurl',
-    withCredentials: true
-  });
-  openMediaSource(this.player, this.clock);
-  QUnit.ok(this.requests[0].withCredentials,
-           'with credentials should be set to true if that option is passed in');
-  videojs.options.hls = hlsOptions;
-});
-
 QUnit.test('if mode global option is used, mode is set to global option', function() {
   let hlsOptions = videojs.options.hls;