-
Notifications
You must be signed in to change notification settings - Fork 422
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(playback-watcher): ignore subtitles #980
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. perhaps we should just return in here if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried that but it didn't seem to work. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That seems strange. It wouldn't execute for other loader types of returning early on subtitles? |
||
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') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this past be removed because we return early on subtitles? |
||
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') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This conditional probably can be removed. |
||
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') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This conditional probably can be removed, and the else as well. |
||
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'}); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think by removing this we will still try to blacklist subtitle tracks when we don't have subtitles in the segments. Should we just return early for subtitles tracks, or, in loaderTypes above, remove subtitles?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, we'll still try to exclude it but at least it won't be removed. Yeah, exiting early here when it's of
type
subtitle
sounds good.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to return here but not remove the track.