From 8987194674048448a26276490da346edac011608 Mon Sep 17 00:00:00 2001 From: Gary Katsevman Date: Fri, 30 Oct 2020 19:00:04 -0400 Subject: [PATCH] fix(playback-watcher): ignore subtitles (#980) It's possible that a subtitle track will have segments without any text in them depending on how the subtitles and captions were created. Therefor, we should ignore it in the playback-watcher where we're checking for excessive downloads. --- src/playback-watcher.js | 9 ---- test/playback-watcher.test.js | 88 +++++++++++++++++------------------ 2 files changed, 44 insertions(+), 53 deletions(-) diff --git a/src/playback-watcher.js b/src/playback-watcher.js index 66d96f8e8..6ebb7ed55 100644 --- a/src/playback-watcher.js +++ b/src/playback-watcher.js @@ -11,7 +11,6 @@ import window from 'global/window'; import * as Ranges from './ranges'; import logger from './util/logger'; -import videojs from 'video.js'; // Set of events that reset the playback-watcher time check logic and clear the timeout const timerCancelEvents = [ @@ -220,14 +219,6 @@ export default class PlaybackWatcher { this.tech_.trigger({type: 'usage', name: `vhs-${type}-download-exclusion`}); if (type === 'subtitle') { - // TODO: Is there anything else that we can do here? - // removing the track and disabling could have accesiblity implications. - const track = loader.track(); - const label = track.label || track.language || 'Unknown'; - - videojs.log.warn(`Text track "${label}" is not working correctly. It will be disabled and excluded.`); - track.mode = 'disabled'; - this.tech_.textTracks().removeTrack(track); return; } diff --git a/test/playback-watcher.test.js b/test/playback-watcher.test.js index 64efa739c..3612838e7 100644 --- a/test/playback-watcher.test.js +++ b/test/playback-watcher.test.js @@ -933,64 +933,64 @@ QUnit.module('PlaybackWatcher download detection', { }); loaderTypes.forEach(function(type) { - QUnit.test(`detects ${type} appends without buffer changes and excludes`, function(assert) { - this.setup(); - const loader = this.mpc[`${type}SegmentLoader_`]; - const track = {label: 'foobar', mode: 'showing'}; + if (type !== 'subtitle') { + QUnit.test(`detects ${type} appends without buffer changes and excludes`, function(assert) { + this.setup(); + const loader = this.mpc[`${type}SegmentLoader_`]; + const track = {label: 'foobar', mode: 'showing'}; - if (type === 'subtitle') { - loader.track = () => track; - sinon.stub(this.player.tech_.textTracks(), 'removeTrack'); - } + if (type === 'subtitle') { + loader.track = () => track; + sinon.stub(this.player.tech_.textTracks(), 'removeTrack'); + } - this.setBuffered(videojs.createTimeRanges([[0, 30]])); + this.setBuffered(videojs.createTimeRanges([[0, 30]])); - 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`); + 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 = {}; + const expectedUsage = {}; - expectedUsage[`vhs-${type}-download-exclusion`] = 1; + expectedUsage[`vhs-${type}-download-exclusion`] = 1; - if (type !== 'subtitle') { - expectedUsage['vhs-rendition-blacklisted'] = 1; - expectedUsage['hls-rendition-blacklisted'] = 1; - } + if (type !== 'subtitle') { + expectedUsage['vhs-rendition-blacklisted'] = 1; + expectedUsage['hls-rendition-blacklisted'] = 1; + } - assert.deepEqual(this.usageEvents, expectedUsage, 'usage as expected'); + assert.deepEqual(this.usageEvents, expectedUsage, 'usage as expected'); - if (type !== 'subtitle') { - const message = 'Playback cannot continue. No available working or supported playlists.'; + if (type !== 'subtitle') { + const message = 'Playback cannot continue. No available working or supported playlists.'; - assert.equal(this.mpcErrors, 1, 'one mpc error'); - assert.equal(this.mpc.error, message, 'mpc error set'); - assert.equal(this.player.error().message, message, 'player error set'); - assert.equal(this.env.log.error.callCount, 1, 'player error logged'); - assert.equal(this.env.log.error.args[0][1], message, 'error message as expected'); + assert.equal(this.mpcErrors, 1, 'one mpc error'); + assert.equal(this.mpc.error, message, 'mpc error set'); + assert.equal(this.player.error().message, message, 'player error set'); + assert.equal(this.env.log.error.callCount, 1, 'player error logged'); + assert.equal(this.env.log.error.args[0][1], message, 'error message as expected'); - this.env.log.error.resetHistory(); - } else { - const message = 'Text track "foobar" is not working correctly. It will be disabled and excluded.'; + this.env.log.error.resetHistory(); + } else { + const message = 'Text track "foobar" is not working correctly. It will be disabled and excluded.'; - assert.equal(this.mpcErrors, 0, 'no mpc error set'); - assert.notOk(this.player.error(), 'no player error set'); - assert.equal(this.player.textTracks().removeTrack.callCount, 1, 'text track remove called'); - assert.equal(this.player.textTracks().removeTrack.args[0][0], track, 'text track remove called with expected'); - assert.equal(track.mode, 'disabled', 'mode set to disabled now'); - assert.equal(this.env.log.warn.callCount, 1, 'warning logged'); - assert.equal(this.env.log.warn.args[0][0], message, 'warning message as expected'); + assert.equal(this.mpcErrors, 0, 'no mpc error set'); + assert.notOk(this.player.error(), 'no player error set'); + assert.equal(this.player.textTracks().removeTrack.callCount, 1, 'text track remove called'); + assert.equal(this.player.textTracks().removeTrack.args[0][0], track, 'text track remove called with expected'); + assert.equal(track.mode, 'disabled', 'mode set to disabled now'); + assert.equal(this.env.log.warn.callCount, 1, 'warning logged'); + assert.equal(this.env.log.warn.args[0][0], message, 'warning message as expected'); - this.env.log.warn.resetHistory(); - } - }); + this.env.log.warn.resetHistory(); + } + }); - if (type !== 'subtitle') { QUnit.test(`detects ${type} appends without buffer changes and excludes many playlists`, function(assert) { this.setup({src: 'multipleAudioGroupsCombinedMain.m3u8', type: 'application/vnd.apple.mpegurl'});