From 76745300617b37eabacad7239d3fb071f1660d53 Mon Sep 17 00:00:00 2001 From: brandonocasey Date: Fri, 26 Jun 2020 13:36:20 -0400 Subject: [PATCH 1/7] fix: more conservative stalled download check, better logging --- src/master-playlist-controller.js | 3 --- src/media-groups.js | 2 +- src/playback-watcher.js | 18 ++++++++++-------- src/segment-loader.js | 3 ++- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/master-playlist-controller.js b/src/master-playlist-controller.js index abb1c5a24..96ea798e2 100644 --- a/src/master-playlist-controller.js +++ b/src/master-playlist-controller.js @@ -1346,9 +1346,6 @@ export class MasterPlaylistController extends videojs.EventTarget { if (usingAudioLoader && unsupportedAudio && this.media().attributes.AUDIO) { const audioGroup = this.media().attributes.AUDIO; - this.mediaTypes_.AUDIO.activePlaylistLoader.pause(); - this.audioSegmentLoader_.pause(); - this.audioSegmentLoader_.abort(); this.master().playlists.forEach(variant => { const variantAudioGroup = variant.attributes && variant.attributes.AUDIO; diff --git a/src/media-groups.js b/src/media-groups.js index ea5f71724..b293f2f57 100644 --- a/src/media-groups.js +++ b/src/media-groups.js @@ -744,7 +744,7 @@ export const setupMediaGroups = (settings) => { mediaTypes.AUDIO.onTrackChanged(); } - masterPlaylistLoader.on('mediachange', () => { + masterPlaylistLoader.on(['mediachange', 'mediachanging'], () => { ['AUDIO', 'SUBTITLES'].forEach(type => mediaTypes[type].onGroupChanged()); }); diff --git a/src/playback-watcher.js b/src/playback-watcher.js index 9bfebb37b..6e96e32e2 100644 --- a/src/playback-watcher.js +++ b/src/playback-watcher.js @@ -170,7 +170,7 @@ export default class PlaybackWatcher { const loader = this.masterPlaylistController_[`${type}SegmentLoader_`]; if (this[`${type}StalledDownloads_`] > 0) { - this.logger_(`resetting stalled downloads for ${type} loader`); + this.logger_(`resetting possible stalled download count for ${type} loader`); } this[`${type}StalledDownloads_`] = 0; this[`${type}Buffered_`] = loader.buffered_(); @@ -204,16 +204,18 @@ export default class PlaybackWatcher { this[`${type}StalledDownloads_`]++; - this.logger_(`found stalled download #${this[`${type}StalledDownloads_`]} for ${type} loader`); - // We will technically get past this on the fourth bad append - // rather than the third. As the first will almost always cause - // buffered to change which means that StalledDownloads_ will - // not be incremented - if (this[`${type}StalledDownloads_`] < 3) { + this.logger_(`found #${this[`${type}StalledDownloads_`]} ${type} appends that did not increase buffer (possible stalled download)`, { + playlistId: loader.playlist_ && loader.playlist_.id, + buffered: Ranges.timeRangesToArray(buffered) + + }); + + // after 5 possibly stalled appends with no reset, exclude + if (this[`${type}StalledDownloads_`] < 5) { return; } - this.logger_(`${type} loader download exclusion`); + this.logger_(`${type} loader stalled download exclusion`); this.resetSegmentDownloads_(type); this.tech_.trigger({type: 'usage', name: `vhs-${type}-download-exclusion`}); diff --git a/src/segment-loader.js b/src/segment-loader.js index 1d19465c6..e713bc40d 100644 --- a/src/segment-loader.js +++ b/src/segment-loader.js @@ -2481,7 +2481,6 @@ export default class SegmentLoader extends videojs.EventTarget { * @private */ handleAppendsDone_() { - this.trigger('appendsdone'); if (!this.pendingSegment_) { this.state = 'READY'; // TODO should this move into this.checkForAbort to speed up requests post abort in @@ -2492,6 +2491,8 @@ export default class SegmentLoader extends videojs.EventTarget { return; } + this.trigger('appendsdone'); + const segmentInfo = this.pendingSegment_; // Now that the end of the segment has been reached, we can set the end time. It's From 1adf37b92d1539c081a4ede05da4f89e3f35ed50 Mon Sep 17 00:00:00 2001 From: brandonocasey Date: Mon, 6 Jul 2020 17:22:25 -0400 Subject: [PATCH 2/7] bump up to 10 --- src/playback-watcher.js | 4 ++-- test/playback-watcher.test.js | 42 ++++++++++++++++++----------------- 2 files changed, 24 insertions(+), 22 deletions(-) diff --git a/src/playback-watcher.js b/src/playback-watcher.js index 6e96e32e2..87a618a81 100644 --- a/src/playback-watcher.js +++ b/src/playback-watcher.js @@ -210,8 +210,8 @@ export default class PlaybackWatcher { }); - // after 5 possibly stalled appends with no reset, exclude - if (this[`${type}StalledDownloads_`] < 5) { + // after 10 possibly stalled appends with no reset, exclude + if (this[`${type}StalledDownloads_`] < 10) { return; } diff --git a/test/playback-watcher.test.js b/test/playback-watcher.test.js index 979a0d6f8..43a9a2852 100644 --- a/test/playback-watcher.test.js +++ b/test/playback-watcher.test.js @@ -786,6 +786,8 @@ QUnit.test('jumps to buffered content if seeking just before', function(assert) const loaderTypes = ['audio', 'main', 'subtitle']; +const EXCLUDE_APPEND_COUNT = 10; + QUnit.module('PlaybackWatcher download detection', { beforeEach(assert) { this.env = useFakeEnvironment(assert); @@ -872,17 +874,15 @@ loaderTypes.forEach(function(type) { } this.setBuffered(videojs.createTimeRanges([[0, 30]])); - loader.trigger('appendsdone'); - assert.equal(this.playbackWatcher[`${type}StalledDownloads_`], 0, '1st append 0 stalled downloads'); - - loader.trigger('appendsdone'); - assert.equal(this.playbackWatcher[`${type}StalledDownloads_`], 1, '2nd append 1 stalled downloads'); - - loader.trigger('appendsdone'); - assert.equal(this.playbackWatcher[`${type}StalledDownloads_`], 2, '3rd append 2 stalled downloads'); - loader.trigger('appendsdone'); - assert.equal(this.playbackWatcher[`${type}StalledDownloads_`], 0, '4th append 0 stalled downloads'); + for (let i = 0; i <= EXCLUDE_APPEND_COUNT; i++) { + loader.trigger('appendsdone'); + if (i === EXCLUDE_APPEND_COUNT) { + assert.equal(this.playbackWatcher[`${type}StalledDownloads_`], 0, `append #${i} resets stalled downloads to 0`); + } else { + assert.equal(this.playbackWatcher[`${type}StalledDownloads_`], i, `append #${i + 1} ${i} stalled downloads`); + } + } const expectedUsage = {}; @@ -927,16 +927,18 @@ loaderTypes.forEach(function(type) { const loader = this.mpc[`${type}SegmentLoader_`]; const playlists = this.mpc.master().playlists; const excludeAndVerify = () => { - loader.trigger('appendsdone'); - assert.equal(this.playbackWatcher[`${type}StalledDownloads_`], 1, '1st append 1 stalled downloads'); - - loader.trigger('appendsdone'); - assert.equal(this.playbackWatcher[`${type}StalledDownloads_`], 2, '2nd append 1 stalled downloads'); - - const oldPlaylist = this.mpc.media(); - - loader.trigger('appendsdone'); - assert.equal(this.playbackWatcher[`${type}StalledDownloads_`], 0, '3rd append 0 stalled downloads'); + let oldPlaylist; + // this test only needs 9 appends, since we do an intial append + + for (let i = 0; i < EXCLUDE_APPEND_COUNT; i++) { + oldPlaylist = this.mpc.media(); + loader.trigger('appendsdone'); + if (i === EXCLUDE_APPEND_COUNT - 1) { + assert.equal(this.playbackWatcher[`${type}StalledDownloads_`], 0, `append #${i} resets stalled downloads to 0`); + } else { + assert.equal(this.playbackWatcher[`${type}StalledDownloads_`], i + 1, `append #${i + 1} ${i + 1} stalled downloads`); + } + } const expectedUsage = {}; From 9a087ee1831b3e4292715eee5ca656513b6c619e Mon Sep 17 00:00:00 2001 From: brandonocasey Date: Tue, 7 Jul 2020 17:10:31 -0400 Subject: [PATCH 3/7] remove mediachanging --- src/media-groups.js | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/media-groups.js b/src/media-groups.js index b293f2f57..c39861828 100644 --- a/src/media-groups.js +++ b/src/media-groups.js @@ -736,15 +736,12 @@ export const setupMediaGroups = (settings) => { // DO NOT enable the default subtitle or caption track. // DO enable the default audio track const audioGroup = mediaTypes.AUDIO.activeGroup(); + const groupId = (audioGroup.filter(group => group.default)[0] || audioGroup[0]).id; - if (audioGroup) { - const groupId = (audioGroup.filter(group => group.default)[0] || audioGroup[0]).id; + mediaTypes.AUDIO.tracks[groupId].enabled = true; + mediaTypes.AUDIO.onTrackChanged(); - mediaTypes.AUDIO.tracks[groupId].enabled = true; - mediaTypes.AUDIO.onTrackChanged(); - } - - masterPlaylistLoader.on(['mediachange', 'mediachanging'], () => { + masterPlaylistLoader.on('mediachange', () => { ['AUDIO', 'SUBTITLES'].forEach(type => mediaTypes[type].onGroupChanged()); }); From f470fa3aa262ec777f13e433f82f7ff59d9ec47c Mon Sep 17 00:00:00 2001 From: brandonocasey Date: Wed, 8 Jul 2020 15:12:59 -0400 Subject: [PATCH 4/7] bring back loader pause/abort --- src/master-playlist-controller.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/master-playlist-controller.js b/src/master-playlist-controller.js index 96ea798e2..abb1c5a24 100644 --- a/src/master-playlist-controller.js +++ b/src/master-playlist-controller.js @@ -1346,6 +1346,9 @@ export class MasterPlaylistController extends videojs.EventTarget { if (usingAudioLoader && unsupportedAudio && this.media().attributes.AUDIO) { const audioGroup = this.media().attributes.AUDIO; + this.mediaTypes_.AUDIO.activePlaylistLoader.pause(); + this.audioSegmentLoader_.pause(); + this.audioSegmentLoader_.abort(); this.master().playlists.forEach(variant => { const variantAudioGroup = variant.attributes && variant.attributes.AUDIO; From 4e161e9995b7069eff37dedbb1d31dc103e2d78f Mon Sep 17 00:00:00 2001 From: brandonocasey Date: Wed, 8 Jul 2020 15:41:34 -0400 Subject: [PATCH 5/7] add tests --- test/segment-loader.test.js | 74 +++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/test/segment-loader.test.js b/test/segment-loader.test.js index 70fb4553e..44fecb03f 100644 --- a/test/segment-loader.test.js +++ b/test/segment-loader.test.js @@ -763,6 +763,80 @@ QUnit.module('SegmentLoader', function(hooks) { }); }); + QUnit.test('appendsdone happens after appends complete', function(assert) { + const done = assert.async(); + + return setupMediaSource(loader.mediaSource_, loader.sourceUpdater_).then(() => { + let appendsdone = false; + + loader.playlist(playlistWithDuration(20)); + loader.load(); + this.clock.tick(1); + + standardXHRResponse(this.requests.shift(), muxedSegment()); + loader.one('appendsdone', () => { + appendsdone = true; + }); + + let appends = 0; + + const finish = function() { + appends++; + + if (appends < 2) { + return; + } + + assert.ok(appendsdone, 'appendsdone triggered'); + done(); + }; + + loader.sourceUpdater_.videoBuffer.addEventListener('updateend', () => { + loader.sourceUpdater_.videoQueueCallback(finish); + loader.sourceUpdater_.audioQueueCallback(finish); + }); + }); + }); + + QUnit.test('appendsdone does not happen after abort during append', function(assert) { + const done = assert.async(); + + return setupMediaSource(loader.mediaSource_, loader.sourceUpdater_).then(() => { + let appendsdone = false; + + loader.playlist(playlistWithDuration(20)); + loader.load(); + this.clock.tick(1); + + standardXHRResponse(this.requests.shift(), muxedSegment()); + loader.one('appendsdone', () => { + appendsdone = true; + }); + + loader.one('appending', () => { + loader.abort(); + }); + + let appends = 0; + + const finish = function() { + appends++; + + if (appends < 2) { + return; + } + + assert.notOk(appendsdone, 'appendsdone triggered'); + done(); + }; + + loader.sourceUpdater_.videoBuffer.addEventListener('updateend', () => { + loader.sourceUpdater_.videoQueueCallback(finish); + loader.sourceUpdater_.audioQueueCallback(finish); + }); + }); + }); + QUnit.test('audio loader waits to request segment until it has enough info', function(assert) { loader.dispose(); loader = new SegmentLoader(LoaderCommonSettings.call(this, { From 178c7647da23dc417581f72bd8d536872251b53c Mon Sep 17 00:00:00 2001 From: brandonocasey Date: Wed, 8 Jul 2020 16:32:52 -0400 Subject: [PATCH 6/7] fix test --- test/segment-loader.test.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/segment-loader.test.js b/test/segment-loader.test.js index 44fecb03f..346d3aa41 100644 --- a/test/segment-loader.test.js +++ b/test/segment-loader.test.js @@ -793,6 +793,9 @@ QUnit.module('SegmentLoader', function(hooks) { loader.sourceUpdater_.videoBuffer.addEventListener('updateend', () => { loader.sourceUpdater_.videoQueueCallback(finish); + }); + + loader.sourceUpdater_.audioBuffer.addEventListener('updateend', () => { loader.sourceUpdater_.audioQueueCallback(finish); }); }); From 0107e6ed04a956b28929058087c5f3bc901e2cd7 Mon Sep 17 00:00:00 2001 From: brandonocasey Date: Thu, 9 Jul 2020 10:54:56 -0400 Subject: [PATCH 7/7] revert media groups change --- src/media-groups.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/media-groups.js b/src/media-groups.js index c39861828..ea5f71724 100644 --- a/src/media-groups.js +++ b/src/media-groups.js @@ -736,10 +736,13 @@ export const setupMediaGroups = (settings) => { // DO NOT enable the default subtitle or caption track. // DO enable the default audio track const audioGroup = mediaTypes.AUDIO.activeGroup(); - const groupId = (audioGroup.filter(group => group.default)[0] || audioGroup[0]).id; - mediaTypes.AUDIO.tracks[groupId].enabled = true; - mediaTypes.AUDIO.onTrackChanged(); + if (audioGroup) { + const groupId = (audioGroup.filter(group => group.default)[0] || audioGroup[0]).id; + + mediaTypes.AUDIO.tracks[groupId].enabled = true; + mediaTypes.AUDIO.onTrackChanged(); + } masterPlaylistLoader.on('mediachange', () => { ['AUDIO', 'SUBTITLES'].forEach(type => mediaTypes[type].onGroupChanged());