fix(playback-watcher): ignore subtitles #980
Conversation
It's possible that a subtitle track will have segments without any text in them depending on how the subtitles and captions where created. Therefor, we should ignore it in the playback-watcher were we're checking for excessive downloads.
| @@ -219,18 +218,6 @@ export default class PlaybackWatcher { | |||
| this.resetSegmentDownloads_(type); | |||
| this.tech_.trigger({type: 'usage', name: `vhs-${type}-download-exclusion`}); | |||
|
|
|||
| if (type === 'subtitle') { | |||
There was a problem hiding this comment.
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.
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.
Updated to return here but not remove the track.
| this.setup(); | ||
| const loader = this.mpc[`${type}SegmentLoader_`]; | ||
| const track = {label: 'foobar', mode: 'showing'}; | ||
| if (type !== 'subtitle') { |
There was a problem hiding this comment.
perhaps we should just return in here if type === 'subtitle' would keep the changes down. LGTM otherwise.
There was a problem hiding this comment.
I tried that but it didn't seem to work.
There was a problem hiding this comment.
That seems strange. It wouldn't execute for other loader types of returning early on subtitles?
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.
| this.setup(); | ||
| const loader = this.mpc[`${type}SegmentLoader_`]; | ||
| const track = {label: 'foobar', mode: 'showing'}; | ||
| if (type !== 'subtitle') { |
There was a problem hiding this comment.
That seems strange. It wouldn't execute for other loader types of returning early on subtitles?
| loader.track = () => track; | ||
| sinon.stub(this.player.tech_.textTracks(), 'removeTrack'); | ||
| } | ||
| if (type === 'subtitle') { |
There was a problem hiding this comment.
Should this past be removed because we return early on subtitles?
| expectedUsage['vhs-rendition-blacklisted'] = 1; | ||
| expectedUsage['hls-rendition-blacklisted'] = 1; | ||
| } | ||
| if (type !== 'subtitle') { |
There was a problem hiding this comment.
This conditional probably can be removed.
|
|
||
| if (type !== 'subtitle') { | ||
| const message = 'Playback cannot continue. No available working or supported playlists.'; | ||
| if (type !== 'subtitle') { |
There was a problem hiding this comment.
This conditional probably can be removed, and the else as well.
| @@ -220,14 +219,6 @@ export default class PlaybackWatcher { | |||
| this.tech_.trigger({type: 'usage', name: `vhs-${type}-download-exclusion`}); | |||
|
|
|||
| if (type === 'subtitle') { | |||
There was a problem hiding this comment.
Might be worth having a comment here. Something like // subtitles aren't guaranteed to be present for every bit of video, therefore, we can't determine whether downloads have stalled based on the subtitle's buffer
Updates based on late comments from #980.
Updates based on late comments from #980.
Updates based on late comments from #980.
Updates based on late comments from videojs#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.