Do not reset segmentloaders when switching media groups (#1155)

* set loader state to ready on aborts even when loader is paused

src/master-playlist-controller.js

@@ -509,7 +509,7 @@ export class MasterPlaylistController extends videojs.EventTarget {
       activeAudioGroup = this.activeAudioGroup();
       activeTrack = activeAudioGroup.filter((track) => track.enabled)[0];
       if (!activeTrack) {
-        this.setupAudio();
+        this.mediaGroupChanged();
         this.trigger('audioupdate');
       }
       this.setupSubtitles();
@@ -799,14 +799,41 @@ export class MasterPlaylistController extends videojs.EventTarget {
     this.setupSubtitles();
   }
 
+  /**
+   * Determine the correct audio renditions based on the active
+   * AudioTrack and initialize a PlaylistLoader and SegmentLoader if
+   * necessary. This method is only called when the media-group changes
+   * and performs non-destructive 'resync' of the SegmentLoader(s) since
+   * the playlist has likely changed
+   */
+  mediaGroupChanged() {
+    let track = this.getActiveAudioTrack_();
+
+    this.stopAudioLoaders_();
+    this.resyncAudioLoaders_(track);
+  }
+
   /**
    * Determine the correct audio rendition based on the active
    * AudioTrack and initialize a PlaylistLoader and SegmentLoader if
    * necessary. This method is called once automatically before
    * playback begins to enable the default audio track and should be
-   * invoked again if the track is changed.
+   * invoked again if the track is changed. Performs destructive 'reset'
+   * on the SegmentLoaders(s) to ensure we start loading audio as
+   * close to currentTime as possible
    */
   setupAudio() {
+    let track = this.getActiveAudioTrack_();
+
+    this.stopAudioLoaders_();
+    this.resetAudioLoaders_(track);
+  }
+
+  /**
+   * Returns the currently active track or the default track if none
+   * are active
+   */
+  getActiveAudioTrack_() {
     // determine whether seperate loaders are required for the audio
     // rendition
     let audioGroup = this.activeAudioGroup();
@@ -821,19 +848,56 @@ export class MasterPlaylistController extends videojs.EventTarget {
       track.enabled = true;
     }
 
+    return track;
+  }
+
+  /**
+   * Destroy the PlaylistLoader and pause the SegmentLoader specifically
+   * for audio when switching audio tracks
+   */
+  stopAudioLoaders_() {
     // stop playlist and segment loading for audio
     if (this.audioPlaylistLoader_) {
       this.audioPlaylistLoader_.dispose();
       this.audioPlaylistLoader_ = null;
     }
     this.audioSegmentLoader_.pause();
+  }
 
+  /**
+   * Destructive reset of the mainSegmentLoader (when audio is muxed)
+   * or audioSegmentLoader (when audio is demuxed) to prepare them
+   * to start loading new data right at currentTime
+   */
+  resetAudioLoaders_(track) {
     if (!track.properties_.resolvedUri) {
       this.mainSegmentLoader_.resetEverything();
       return;
     }
+
     this.audioSegmentLoader_.resetEverything();
+    this.setupAudioPlaylistLoader_(track);
+  }
 
+  /**
+   * Non-destructive resync of the audioSegmentLoader (when audio
+   * is demuxed) to prepare to continue appending new audio data
+   * at the end of the current buffered region
+   */
+  resyncAudioLoaders_(track) {
+    if (!track.properties_.resolvedUri) {
+      return;
+    }
+
+    this.audioSegmentLoader_.resyncLoader();
+    this.setupAudioPlaylistLoader_(track);
+  }
+
+  /**
+   * Setup a new audioPlaylistLoader and start the audioSegmentLoader
+   * to begin loading demuxed audio
+   */
+  setupAudioPlaylistLoader_(track) {
     // startup playlist and segment loaders for the enabled audio
     // track
     this.audioPlaylistLoader_ = new PlaylistLoader(track.properties_.resolvedUri,

src/segment-loader.js

@@ -138,6 +138,7 @@ export default class SegmentLoader extends videojs.EventTarget {
    */
   dispose() {
     this.state = 'DISPOSED';
+    this.pause();
     this.abort_();
     if (this.sourceUpdater_) {
       this.sourceUpdater_.dispose();
@@ -159,10 +160,15 @@ export default class SegmentLoader extends videojs.EventTarget {
 
     this.abort_();
 
+    // We aborted the requests we were waiting on, so reset the loader's state to READY
+    // since we are no longer "waiting" on any requests. XHR callback is not always run
+    // when the request is aborted. This will prevent the loader from being stuck in the
+    // WAITING state indefinitely.
+    this.state = 'READY';
+
     // don't wait for buffer check timeouts to begin fetching the
     // next segment
     if (!this.paused()) {
-      this.state = 'READY';
       this.monitorBuffer_();
     }
   }
@@ -445,6 +451,7 @@ export default class SegmentLoader extends videojs.EventTarget {
   resyncLoader() {
     this.mediaIndex = null;
     this.syncPoint_ = null;
+    this.abort();
   }
 
   /**
@@ -847,6 +854,7 @@ export default class SegmentLoader extends videojs.EventTarget {
     // an error occurred from the active pendingSegment_ so reset everything
     if (error) {
       this.pendingSegment_ = null;
+      this.state = 'READY';
 
       // the requests were aborted just record the aborted stat and exit
       // this is not a true error condition and nothing corrective needs
@@ -856,7 +864,6 @@ export default class SegmentLoader extends videojs.EventTarget {
         return;
       }
 
-      this.state = 'READY';
       this.pause();
 
       // the error is really just that at least one of the requests timed-out

test/master-playlist-controller.test.js

@@ -281,11 +281,18 @@ QUnit.test('fast quality change resyncs audio segment loader', function(assert)
 
   this.player.audioTracks()[0].enabled = true;
 
+  let resyncs = 0;
   let resets = 0;
+  let realReset = masterPlaylistController.audioSegmentLoader_.resetLoader;
+
+  masterPlaylistController.audioSegmentLoader_.resetLoader = function() {
+    resets++;
+    realReset.call(this);
+  };
 
-  masterPlaylistController.audioSegmentLoader_.resetLoader = () => resets++;
+  masterPlaylistController.audioSegmentLoader_.resyncLoader = () => resyncs++;
   masterPlaylistController.fastQualityChange_();
-  assert.equal(resets, 0, 'does not reset the audio segment loader when media same');
+  assert.equal(resyncs, 0, 'does not resync the audio segment loader when media same');
 
   // force different media
   masterPlaylistController.selectPlaylist = () => {
@@ -295,10 +302,52 @@ QUnit.test('fast quality change resyncs audio segment loader', function(assert)
   assert.equal(this.requests.length, 1, 'one request');
   masterPlaylistController.fastQualityChange_();
   assert.equal(this.requests.length, 2, 'added a request for new media');
-  assert.equal(resets, 0, 'does not reset the audio segment loader yet');
+  assert.equal(resyncs, 0, 'does not resync the audio segment loader yet');
   // new media request
   this.standardXHRResponse(this.requests[1]);
-  assert.equal(resets, 1, 'resets the audio segment loader when media changes');
+  assert.equal(resyncs, 1, 'resyncs the audio segment loader when media changes');
+  assert.equal(resets, 0, 'does not reset the audio segment loader when media changes');
+});
+
+QUnit.test('audio segment loader is reset on audio track change', function(assert) {
+  this.requests.length = 0;
+  this.player = createPlayer();
+  this.player.src({
+    src: 'alternate-audio-multiple-groups.m3u8',
+    type: 'application/vnd.apple.mpegurl'
+  });
+
+  const masterPlaylistController = this.player.tech_.hls.masterPlaylistController_;
+
+  masterPlaylistController.selectPlaylist = () => {
+    return masterPlaylistController.master().playlists[0];
+  };
+
+  // master
+  this.standardXHRResponse(this.requests.shift());
+  // media
+  this.standardXHRResponse(this.requests.shift());
+
+  masterPlaylistController.mediaSource.trigger('sourceopen');
+
+  let resyncs = 0;
+  let resets = 0;
+  let realReset = masterPlaylistController.audioSegmentLoader_.resetLoader;
+
+  masterPlaylistController.audioSegmentLoader_.resetLoader = function() {
+    resets++;
+    realReset.call(this);
+  };
+  masterPlaylistController.audioSegmentLoader_.resyncLoader = () => resyncs++;
+
+  assert.equal(this.requests.length, 1, 'one request');
+  assert.equal(resyncs, 0, 'does not resync the audio segment loader yet');
+
+  this.player.audioTracks()[1].enabled = true;
+
+  assert.equal(this.requests.length, 2, 'two requests');
+  assert.equal(resyncs, 1, 'resyncs the audio segment loader when audio track changes');
+  assert.equal(resets, 1, 'resets the audio segment loader when audio track changes');
 });
 
 QUnit.test('if buffered, will request second segment byte range', function(assert) {

test/videojs-contrib-hls.test.js

@@ -671,7 +671,7 @@ QUnit.test('buffer checks are noops when only the master is ready', function(ass
   this.standardXHRResponse(this.requests.shift());
   this.clock.tick(10 * 1000);
 
-  assert.strictEqual(1, this.requests.length, 'one request was made');
+  assert.strictEqual(this.requests.length, 1, 'one request was made');
   assert.strictEqual(this.requests[0].url,
                      absoluteUrl('manifest/media1.m3u8'),
                      'media playlist requested');

utils/manifest/alternate-audio-multiple-groups.m3u8

@@ -1,6 +1,8 @@
 #EXTM3U
 #EXT-X-MEDIA:TYPE=AUDIO,GROUP-ID="audio",LANGUAGE="eng",NAME="English",AUTOSELECT=YES, DEFAULT=YES,URI="eng/prog_index.m3u8"
+#EXT-X-MEDIA:TYPE=AUDIO,GROUP-ID="audio",LANGUAGE="esp",NAME="Spanish",AUTOSELECT=YES, URI="esp/prog_index.m3u8"
 #EXT-X-MEDIA:TYPE=AUDIO,GROUP-ID="audio-hi",LANGUAGE="eng",NAME="English",AUTOSELECT=YES, DEFAULT=YES,URI="eng/prog_index.m3u8"
+#EXT-X-MEDIA:TYPE=AUDIO,GROUP-ID="audio-hi",LANGUAGE="esp",NAME="Spanish",AUTOSELECT=YES, URI="esp/prog_index.m3u8"
 
 #EXT-X-STREAM-INF:PROGRAM-ID=1,BANDWIDTH=195023,CODECS="avc1.42e00a,mp4a.40.2",AUDIO="audio"
 lo/prog_index.m3u8