MediaGroups: various bug fixes and refactor #1243

mjneil · 2017-08-25T21:01:16Z

Description

This PR is a WIP. Tests still need to be written.

There are some issues with the way we currently handle media groups. A big issue is that when switching to a new track, the PlaylistLoader is disposed of and a fresh one is created. The problem there is that the PlaylistLoader also holds onto the playlist object reference. When we dispose of the loader, we lose any sync information we have learned about that stream, so returning to it is basically starting fresh.
Alternate audio was also using incorrect sync information causing delayed switches, and sometimes desync of audio and video.
This also removes the Firefox 48 check for for supporting a change in audio info

Specific Changes proposed

Pull setup of MediaGroups out of MasterPlaylistController and into its own module.
Fix delayed switching between audio tracks and intermittent desync.
removes the Firefox 48 check for for supporting a change in audio info

gesinger

Still have to look at the tests, but wanted to submit an initial review. The refactor looks much cleaner and more straightforward than the old code 👍

gesinger · 2017-08-29T21:10:33Z

src/master-playlist-controller.js

@@ -381,14 +349,25 @@ export class MasterPlaylistController extends videojs.EventTarget {
        this.mainSegmentLoader_.load();
      }

-      this.fillAudioTracks_();
-      this.setupAudio();
+      const mediaGroupSettings = {


Since it's only used in one place, probably can get rid of the var

gesinger · 2017-08-29T21:15:17Z

src/master-playlist-controller.js

      this.audioSegmentLoader_.load();
    }
-    if (this.subtitlePlaylistLoader_) {


Is this load no longer necessary?

I'm not sure what you are asking. load is still called on the subtitleSegmentLoader_ it's just that the check for the playlist loader was changed to a different reference

Maybe there is confusion because git diff is matching the load call with one much lower from another function that was removed? (line 1159 for this file, line 636 in the new file)?

Yup, it was the diff 👍

gesinger · 2017-08-29T21:46:58Z

src/media-groups.js

+};
+
+/**
+ * Start loading for provided segment loader and playlist loader


gesinger · 2017-08-29T22:57:55Z

src/media-groups.js

+    if (!masterGroups[type] ||
+        Object.keys(masterGroups[type]).length === 0 ||
+        mode !== 'html5') {
+      masterGroups[type] = { main: { default: { defualt: true } } };


gesinger · 2017-08-30T20:05:27Z

src/media-groups.js

+  AUDIO: (type, settings) => () => {
+    const { mediaGroups: { [type]: { tracks } } } = settings;
+
+    for (let id in tracks) {


Preference, but this can probably be return tracks.filter((track) => track.enabled)[0] || null?

gesinger · 2017-09-05T21:56:59Z

src/media-groups.js

+};
+
+/**
+ * Returns a function used to get the active group of type provided


the provided type

gesinger · 2017-09-05T21:57:21Z

src/media-groups.js

+ *        Object containing required information for media groups
+ * @return {Function}
+ *         Function that returns the active media group for the provided type. Takes an
+ *         optional paramter {TextTrack} track. If no track is provided, a list of all


gesinger · 2017-09-05T22:08:46Z

src/media-groups.js

+    return null;
+  }
+
+  let result = null;


Maybe this should be called variants?

gesinger · 2017-09-05T22:10:35Z

src/media-groups.js

+    return null;
+  }
+
+  return result.reduce((final, props) => props.id === track.id ? props : final, null);


Preference, but might be clearer as result.filter((props) => props.id === track.id)[0] || null

gesinger · 2017-09-05T22:11:54Z

src/media-groups.js

+  } = settings;
+
+  // setup active group and track getters and change event handlers
+


Extra space?

gesinger · 2017-09-11T18:26:48Z

test/master-playlist-controller.test.js

@@ -573,8 +577,8 @@ QUnit.test('updates the enabled track when switching audio groups', function(ass
                                '0.ts\n' +
                                '#EXT-X-ENDLIST\n');

-  assert.ok(mpc.activeAudioGroup().filter((track) => track.enabled)[0],
-           'enabled a track in the new audio group');
+  assert.notOk(mpc.mediaTypes_.AUDIO.activePlaylistLoader,


Shouldn't this still have an activePlaylistLoader?

It is switching from unmuxed to muxed audio. When we are in the muxed audio case, there is no active audio playlist loader because the audio content is coming from the video stream

gesinger · 2017-09-11T18:27:29Z

test/master-playlist-controller.test.js

@@ -863,40 +867,6 @@ function(assert) {
  assert.equal(this.player.tech_.hls.stats.bandwidth, 1, 'bandwidth we set above');
 });

-QUnit.test('blacklists the current playlist when audio changes in Firefox 48 & below',


Are we foregoing this?

gesinger · 2017-09-11T18:30:36Z

test/master-playlist-controller.test.js

               'no active subtitle group');

  // media
  this.standardXHRResponse(this.requests.shift());

-  assert.ok(masterPlaylistController.activeSubtitleGroup_(), 'active subtitle group');
+  assert.ok(masterPlaylistController.mediaTypes_.SUBTITLES.activeGroup(),


Does this test make sense in this file?

I left these tests in here as integration tests. I'm in favor of (re?)moving them if you think they would be better placed in the media-groups.test.js or if they are already covered by another test

I'm in favor of either of those, since this test won't directly touch the segment loader.

gesinger · 2017-09-11T18:30:53Z

test/master-playlist-controller.test.js

@@ -2296,7 +2272,7 @@ QUnit.test('can get active subtitle track', function(assert) {

  const masterPlaylistController = this.player.tech_.hls.masterPlaylistController_;

-  assert.notOk(masterPlaylistController.activeSubtitleTrack_(),
+  assert.notOk(masterPlaylistController.mediaTypes_.SUBTITLES.activeTrack(),


Does this test belong in this file?

I left these tests in here as integration tests. I'm in favor of (re?)moving them if you think they would be better placed in the media-groups.test.js or if they are already covered by another test

Any of those options sound good 👍

gesinger · 2017-09-11T18:47:19Z

test/media-groups.test.js

+  const result = MediaGroups.createMediaTypes();
+
+  assert.ok(result.AUDIO, 'created AUDIO media group object');
+  assert.equal(JSON.stringify(result.AUDIO.groups), '{}',


Will deep equal not work for empty objects?

It might, I didn't think to try that for some reason. I was trying strictEqual but that was not working
edit: it does :)

gesinger · 2017-09-11T20:12:45Z

test/media-groups.test.js

+  assert.equal(mainSegmentLoaderResetCalls, 1,
+    'no main reset changing to group with playlist loader');
+  assert.equal(segmentLoaderResetCalls, 0,
+    'no reset when active group hasnt changed');


Nice try, but we need the \'

gesinger · 2017-09-11T20:16:44Z

test/media-groups.test.js

+  onError();
+
+  assert.equal(blacklistCurrentPlaylistCalls, 0, 'did not blacklist current playlist');
+  assert.equal(onTrackChangedCalls, 1, 'called onTrackChanged after chaging to default');


gesinger · 2017-09-11T20:22:51Z

test/media-groups.test.js

+  this.env.log.warn.callCount = 0;
+});
+
+QUnit.test('setupListeners adds correct playlist loader listeners', function(assert) {


I'm surprised this test passes

gesinger · 2017-09-11T20:23:54Z

test/media-groups.test.js

+    { main: { default: { default: true } } }, 'forced default audio group');
+  assert.deepEqual(this.mediaTypes[type].groups,
+    { main: [ { id: 'default', playlistLoader: null, default: true } ] },
+    'creates group properites and no playlist loader');


gesinger · 2017-09-11T20:26:12Z

test/media-groups.test.js

+  assert.ok(this.mediaTypes[type].tracks.fr, 'created text track');
+});
+
+QUnit.test('initialize closed-captions correctly generates tracks and NO laoders',


…dedmetadata

mjneil added 8 commits August 22, 2017 17:35

start refactor of media group handling

3dfb285

do more media group things outside of masterplaylistcontroller

3104bc4

more progress, live with subtitles still buggy

f9ca0e1

set new track right away

a4f4cbc

cleanup

fcf0d6c

fix null reference error

c4766a5

fix some tests

319ed8a

add unit tests

db873b5

mjneil changed the title ~~[WIP] MediaGroups: various bug fixes and refactor~~ MediaGroups: various bug fixes and refactor Sep 1, 2017

gesinger reviewed Sep 5, 2017

View reviewed changes

minor cleanup and name changing

985bba7

gesinger reviewed Sep 11, 2017

View reviewed changes

fix up tests

991709a

gesinger approved these changes Sep 12, 2017

View reviewed changes

videojs deleted a comment from shahlabs Sep 13, 2017

mjneil added 2 commits September 18, 2017 16:33

do not set playlist or start segment loaders until loadedplaylist/loa…

ee9628c

…dedmetadata

remove line

25ffba4

gesinger approved these changes Sep 18, 2017

View reviewed changes

mjneil merged commit 24894e6 into videojs:master Sep 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MediaGroups: various bug fixes and refactor #1243

MediaGroups: various bug fixes and refactor #1243

mjneil commented Aug 25, 2017

gesinger left a comment

gesinger Aug 29, 2017

gesinger Aug 29, 2017

mjneil Sep 6, 2017

mjneil Sep 6, 2017

gesinger Sep 6, 2017

gesinger Aug 29, 2017

gesinger Aug 29, 2017

gesinger Aug 30, 2017

mjneil Sep 6, 2017

gesinger Sep 5, 2017

gesinger Sep 5, 2017

gesinger Sep 5, 2017

gesinger Sep 5, 2017

gesinger Sep 5, 2017

gesinger Sep 11, 2017

mjneil Sep 11, 2017

gesinger Sep 11, 2017

gesinger Sep 11, 2017

mjneil Sep 11, 2017

gesinger Sep 12, 2017

gesinger Sep 11, 2017

mjneil Sep 11, 2017

gesinger Sep 12, 2017

gesinger Sep 11, 2017

mjneil Sep 11, 2017 •

edited

Loading

gesinger Sep 11, 2017

gesinger Sep 11, 2017

gesinger Sep 11, 2017

gesinger Sep 11, 2017

gesinger Sep 11, 2017

		} = settings;

		// setup active group and track getters and change event handlers

MediaGroups: various bug fixes and refactor #1243

MediaGroups: various bug fixes and refactor #1243

Conversation

mjneil commented Aug 25, 2017

Description

Specific Changes proposed

gesinger left a 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

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

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

mjneil Sep 11, 2017 • edited Loading

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

mjneil Sep 11, 2017 •

edited

Loading