From 44920679abd0e539f27140503e6bb23f5aee2350 Mon Sep 17 00:00:00 2001 From: Matthew Neil Date: Thu, 16 Mar 2017 15:09:48 -0400 Subject: [PATCH] prevent infinitely requesting same segment when segment has no subtitle data --- README.md | 1 + src/master-playlist-controller.js | 4 +- src/vtt-segment-loader.js | 70 +++++++++++++++--------- test/vtt-segment-loader.test.js | 91 ++++++++++++++++++------------- 4 files changed, 103 insertions(+), 63 deletions(-) diff --git a/README.md b/README.md index f1848e135..7bd591d7b 100644 --- a/README.md +++ b/README.md @@ -44,6 +44,7 @@ Maintenance Status: Stable - [Events](#events) - [loadedmetadata](#loadedmetadata) - [In-Band Metadata](#in-band-metadata) + - [Segment Metadata](#segment-metadata) - [Hosting Considerations](#hosting-considerations) - [Known Issues](#known-issues) - [IE10 and Below](#ie10-and-below) diff --git a/src/master-playlist-controller.js b/src/master-playlist-controller.js index 1280e76c7..0403ddbe8 100644 --- a/src/master-playlist-controller.js +++ b/src/master-playlist-controller.js @@ -843,7 +843,9 @@ export class MasterPlaylistController extends videojs.EventTarget { this.subtitlePlaylistLoader_.dispose(); } - // reset the segment loader + // reset the segment loader only when the subtitle playlist is changed instead of + // every time setupSubtitles is called since switching subtitle tracks fires + // multiple `change` events on the TextTrackList this.subtitleSegmentLoader_.resetEverything(); // can't reuse playlistloader because we're only using single renditions and not a diff --git a/src/vtt-segment-loader.js b/src/vtt-segment-loader.js index 9aeea4b74..ed2616bdb 100644 --- a/src/vtt-segment-loader.js +++ b/src/vtt-segment-loader.js @@ -217,6 +217,11 @@ export default class VTTSegmentLoader extends videojs.EventTarget { return videojs.createTimeRanges([[start, end]]); } + /** + * Returns the timestamp offset for the current timeline stored in the sync controller + * + * @return {Number} timestamp offset for the current timeline + */ timestampOffset() { return this.syncController_.timestampOffsetForTimeline(this.currentTimeline_); } @@ -472,22 +477,16 @@ export default class VTTSegmentLoader extends videojs.EventTarget { this.currentTime_(), this.syncPoint_); + segmentInfo = this.skipEmptySegments_(segmentInfo); + if (!segmentInfo) { return; } - // TODO is this check important - // if (segmentInfo.mediaIndex === this.playlist_.segments.length - 1 && - // this.mediaSource_.readyState === 'ended' && - // !this.seeking_()) { - // return; - // } - if (this.syncController_.timestampOffsetForTimeline(segmentInfo.timeline) === null) { // We don't have the timestamp offset that we need to sync subtitles. // Rerun on a timestamp offset or user interaction. let checkTimestampOffset = () => { - this.syncController_.off('timestampoffset', checkTimestampOffset); this.state = 'READY'; if (!this.paused()) { // if not paused, queue a buffer check as soon as possible @@ -495,7 +494,7 @@ export default class VTTSegmentLoader extends videojs.EventTarget { } }; - this.syncController_.on('timestampoffset', checkTimestampOffset); + this.syncController_.one('timestampoffset', checkTimestampOffset); this.state = 'WAITING_ON_TIMELINE'; return; } @@ -664,6 +663,26 @@ export default class VTTSegmentLoader extends videojs.EventTarget { }; } + /** + * Prevents the segment loader from requesting segments we know contain no subtitles + * by walking forward until we find the next segment that we don't know whether it is + * empty or not. + * + * @param {Object} segmentInfo + * a segment request object that describes the segment to load + * @return {Object} + * a segment request object that describes the segment to load + */ + skipEmptySegments_(segmentInfo) { + while (segmentInfo && segmentInfo.segment.empty) { + segmentInfo = this.generateSegmentInfo_(segmentInfo.playlist, + segmentInfo.mediaIndex + 1, + segmentInfo.startOfSegment + segmentInfo.duration, + segmentInfo.isSyncRequest); + } + return segmentInfo; + } + /** * load a specific segment from a request into the buffer * @@ -978,13 +997,12 @@ export default class VTTSegmentLoader extends videojs.EventTarget { this.subtitlesTrack_.tech_) { const loadHandler = () => { - this.subtitlesTrack_.tech_.off('vttjsloaded', loadHandler); this.handleSegment_(); }; this.state = 'WAITING_ON_VTTJS'; - this.subtitlesTrack_.tech_.on('vttjsloaded', loadHandler); - this.subtitlesTrack_.tech_.on('vttjserror', () => { + this.subtitlesTrack_.tech_.one('vttjsloaded', loadHandler); + this.subtitlesTrack_.tech_.one('vttjserror', () => { this.subtitlesTrack_.tech_.off('vttjsloaded', loadHandler); this.error({ message: 'Error loading vtt.js' @@ -1097,18 +1115,24 @@ export default class VTTSegmentLoader extends videojs.EventTarget { } updateTimeMapping_(segmentInfo, mappingObj, playlist) { - let segment = segmentInfo.segment; - let timestampmap = segmentInfo.timestampmap; + const segment = segmentInfo.segment; - if (!mappingObj || !segmentInfo.cues.length) { + if (!mappingObj) { // If the sync controller does not have a mapping of TS to Media Time for the - // timeline, then we don't have enough information to update the segment and cue + // timeline, then we don't have enough information to update the cue // start/end times + return; + } + + if (!segmentInfo.cues.length) { // If there are no cues, we also do not have enough information to figure out - // segment timing + // segment timing. Mark that the segment contains no cues so we don't re-request + // an empty segment. + segment.empty = true; return; } + const timestampmap = segmentInfo.timestampmap; const diff = (timestampmap.MPEGTS / 90000) - timestampmap.LOCAL + mappingObj.mapping; segmentInfo.cues.forEach((cue) => { @@ -1117,17 +1141,13 @@ export default class VTTSegmentLoader extends videojs.EventTarget { cue.endTime += diff; }); - const firstStart = segmentInfo.cues[0].startTime; - const lastStart = segmentInfo.cues[segmentInfo.cues.length - 1].startTime; - const midPoint = (firstStart + lastStart) / 2; - - segment.start = midPoint - (segment.duration / 2); - segment.end = midPoint + (segment.duration / 2); - if (!playlist.syncInfo) { + const firstStart = segmentInfo.cues[0].startTime; + const lastStart = segmentInfo.cues[segmentInfo.cues.length - 1].startTime; + playlist.syncInfo = { mediaSequence: playlist.mediaSequence + segmentInfo.mediaIndex, - time: segment.start + time: Math.min(firstStart, lastStart - segment.duration) }; } } diff --git a/test/vtt-segment-loader.test.js b/test/vtt-segment-loader.test.js index 873074e05..1fb5ecd40 100644 --- a/test/vtt-segment-loader.test.js +++ b/test/vtt-segment-loader.test.js @@ -782,19 +782,21 @@ QUnit.test('segmentInfo.mediaIndex is adjusted when live playlist is updated', f this.clock.tick(1); segmentInfo = loader.pendingSegment_; - assert.equal(segmentInfo.mediaIndex, 3, 'segmentInfo.mediaIndex starts at 3'); - assert.equal(this.requests[0].url, '3.vtt', 'requesting the segment at mediaIndex 3'); + // segment 3 had no cue data (because we didn't mock any) so next request should be + // segment 4 because of skipping empty segments + assert.equal(segmentInfo.mediaIndex, 4, 'segmentInfo.mediaIndex starts at 4'); + assert.equal(this.requests[0].url, '4.vtt', 'requesting the segment at mediaIndex 4'); // Update the playlist shifting the mediaSequence by 2 which will result - // in a decrement of the mediaIndex by 2 to 1 + // in a decrement of the mediaIndex by 4 to 2 loader.playlist(playlistWithDuration(50, { mediaSequence: 2, endList: false })); - assert.equal(segmentInfo.mediaIndex, 1, 'segmentInfo.mediaIndex is updated to 1'); + assert.equal(segmentInfo.mediaIndex, 2, 'segmentInfo.mediaIndex is updated to 2'); - expectedLoaderIndex = 1; + expectedLoaderIndex = 2; this.requests[0].response = new Uint8Array(10).buffer; this.requests.shift().respond(200, null, ''); this.clock.tick(1); @@ -827,7 +829,7 @@ QUnit.skip('sets the timestampOffset on timeline change', function(assert) { assert.equal(loader.mediaRequests, 2, '2 requests'); }); -QUnit.test('tracks segment end times as they are buffered', function(assert) { +QUnit.skip('tracks segment end times as they are buffered', function(assert) { let playlist = playlistWithDuration(20); loader.parseVTTCues_ = function(segmentInfo) { @@ -1380,24 +1382,17 @@ function(assert) { assert.equal(loader.pendingSegment_.uri, '1.vtt', 'second segment still pending'); assert.equal(loader.pendingSegment_.segment.uri, '1.vtt', 'correct segment reference'); - // mock probeSegmentInfo as the response bytes aren't parsable (and won't provide - // time info) + // mock parseVttCues_ to respond empty cue array loader.parseVTTCues_ = (segmentInfo) => { - segmentInfo.cues = [{ startTime: 10, endTime: 11 }, { startTime: 20, endTime: 21 }]; + segmentInfo.cues = []; segmentInfo.timestampmap = { MPEGTS: 0, LOCAL: 0 }; }; this.requests[0].response = new Uint8Array(10).buffer; this.requests.shift().respond(200, null, ''); - assert.equal(playlistUpdated.segments[0].start, - 10, - 'set start on segment of new playlist'); - assert.equal(playlistUpdated.segments[0].end, - 20, - 'set end on segment of new playlist'); - assert.ok(!playlist.segments[1].start, 'did not set start on segment of old playlist'); - assert.ok(!playlist.segments[1].end, 'did not set end on segment of old playlist'); + assert.ok(playlistUpdated.segments[0].empty, 'set empty on segment of new playlist'); + assert.ok(!playlist.segments[1].empty, 'did not set empty on segment of old playlist'); }); QUnit.test('saves segment info to old segment after playlist refresh if segment fell off', @@ -1439,26 +1434,19 @@ function(assert) { assert.equal(loader.pendingSegment_.uri, '1.vtt', 'second segment still pending'); assert.equal(loader.pendingSegment_.segment.uri, '1.vtt', 'correct segment reference'); - // mock probeSegmentInfo as the response bytes aren't parsable (and won't provide - // time info) + // mock parseVttCues_ to respond empty cue array loader.parseVTTCues_ = (segmentInfo) => { - segmentInfo.cues = [{ startTime: 10, endTime: 11 }, { startTime: 20, endTime: 21 }]; + segmentInfo.cues = []; segmentInfo.timestampmap = { MPEGTS: 0, LOCAL: 0 }; }; this.requests[0].response = new Uint8Array(10).buffer; this.requests.shift().respond(200, null, ''); - assert.equal(playlist.segments[1].start, - 10, - 'set start on segment of old playlist'); - assert.equal(playlist.segments[1].end, - 20, - 'set end on segment of old playlist'); - assert.ok(!playlistUpdated.segments[0].start, - 'no start info for first segment of new playlist'); - assert.ok(!playlistUpdated.segments[0].end, - 'no end info for first segment of new playlist'); + assert.ok(playlist.segments[1].empty, + 'set empty on segment of old playlist'); + assert.ok(!playlistUpdated.segments[0].empty, + 'no empty info for first segment of new playlist'); }); QUnit.test('new playlist always triggers syncinfoupdate', function(assert) { @@ -1495,7 +1483,7 @@ QUnit.test('waits for syncController to have sync info for the timeline of the v loadedSegment = true; }; loader.checkBuffer_ = () => { - return { mediaIndex: 2, timeline: 2 }; + return { mediaIndex: 2, timeline: 2, segment: { } }; }; loader.playlist(playlist); @@ -1538,7 +1526,7 @@ QUnit.test('waits for vtt.js to be loaded before attempting to parse cues', func let vttjsCallback = () => {}; this.track.tech_ = { - on(event, callback) { + one(event, callback) { if (event === 'vttjsloaded') { vttjsCallback = callback; } @@ -1591,13 +1579,11 @@ QUnit.test('uses timestampmap from vtt header to set cue and segment timing', fu { startTime: 19, endTime: 23 } ]; const expectedSegment = { - duration: 10, - start: 11.5, - end: 21.5 + duration: 10 }; const expectedPlaylist = { mediaSequence: 100, - syncInfo: { mediaSequence: 102, time: 11.5 } + syncInfo: { mediaSequence: 102, time: 9 } }; const mappingObj = { time: 0, @@ -1668,6 +1654,37 @@ QUnit.test('loader logs vtt.js ParsingErrors and does not trigger an error event this.env.log.warn.callCount = 0; }); +QUnit.test('loader does not re-request segments that contain no subtitles', function(assert) { + let playlist = playlistWithDuration(60); + + playlist.endList = false; + + loader.parseVTTCues_ = (segmentInfo) => { + // mock empty segment + segmentInfo.cues = []; + }; + + loader.currentTime_ = () => { + return 30; + } + + loader.playlist(playlist); + loader.track(this.track); + loader.load(); + + this.clock.tick(1); + + assert.equal(loader.pendingSegment_.mediaIndex, 2, 'requesting initial segment guess'); + + this.requests[0].response = new Uint8Array(10).buffer; + this.requests.shift().respond(200, null, ''); + + this.clock.tick(1); + + assert.ok(playlist.segments[2].empty, 'marked empty segment as empty'); + assert.equal(loader.pendingSegment_.mediaIndex, 3, 'walked forward skipping requesting empty segment'); +}); + QUnit.test('loader triggers error event on fatal vtt.js errors', function(assert) { let playlist = playlistWithDuration(40); let errors = 0; @@ -1704,7 +1721,7 @@ QUnit.test('loader triggers error event when vtt.js fails to load', function(ass let vttjsCallback = () => {}; this.track.tech_ = { - on(event, callback) { + one(event, callback) { if (event === 'vttjserror') { vttjsCallback = callback; }