From 7658726f14ad672fac2a9f92eb19b4eb94491008 Mon Sep 17 00:00:00 2001 From: Jon-Carlos Rivera Date: Thu, 23 Feb 2017 11:18:21 -0500 Subject: [PATCH] fix: Fix a bug with the combination of seek-to-live and resync-on-a-poor-guess behaviors (#1023) * Fix a bug with the combination of seek-to-live (#1006) and resync-on-a-poor-guess (#1016) behaviors * Added tests to ensure the proper sequence of events for seekable and resync logic * Unregister the seekablechanged event handler from the tech on dispose --- src/master-playlist-controller.js | 2 + src/playback-watcher.js | 3 ++ src/segment-loader.js | 21 ++++++++--- src/sync-controller.js | 2 +- test/master-playlist-controller.test.js | 24 ++++++++++++ test/playback-watcher.test.js | 26 +++++++++++++ test/segment-loader.test.js | 49 ++++++++++++++++++++----- 7 files changed, 111 insertions(+), 16 deletions(-) diff --git a/src/master-playlist-controller.js b/src/master-playlist-controller.js index 534d41e4..ff5bf53d 100644 --- a/src/master-playlist-controller.js +++ b/src/master-playlist-controller.js @@ -957,6 +957,8 @@ export class MasterPlaylistController extends videojs.EventTarget { mainSeekable.end(0) ]]); } + + this.tech_.trigger('seekablechanged'); } /** diff --git a/src/playback-watcher.js b/src/playback-watcher.js index 6fd63bf4..ee6b9f6b 100644 --- a/src/playback-watcher.js +++ b/src/playback-watcher.js @@ -38,7 +38,9 @@ export default class PlaybackWatcher { let waitingHandler = () => this.waiting_(); let cancelTimerHandler = () => this.cancelTimer_(); + let fixesBadSeeksHandler = () => this.fixesBadSeeks_(); + this.tech_.on('seekablechanged', fixesBadSeeksHandler); this.tech_.on('waiting', waitingHandler); this.tech_.on(timerCancelEvents, cancelTimerHandler); this.monitorCurrentTime_(); @@ -46,6 +48,7 @@ export default class PlaybackWatcher { // Define the dispose function to clean up our events this.dispose = () => { this.logger_('dispose'); + this.tech_.off('seekablechanged', fixesBadSeeksHandler); this.tech_.off('waiting', waitingHandler); this.tech_.off(timerCancelEvents, cancelTimerHandler); if (this.checkCurrentTimeTimeout_) { diff --git a/src/segment-loader.js b/src/segment-loader.js index 6caad053..a706f4c5 100644 --- a/src/segment-loader.js +++ b/src/segment-loader.js @@ -186,7 +186,6 @@ export default class SegmentLoader extends videojs.EventTarget { * and reset to a default state */ abort() { - if (this.state !== 'WAITING') { if (this.pendingSegment_) { this.pendingSegment_ = null; @@ -532,7 +531,6 @@ export default class SegmentLoader extends videojs.EventTarget { segmentInfo.timestampOffset = segmentInfo.startOfSegment; } - this.currentTimeline_ = segmentInfo.timeline; this.loadSegment_(segmentInfo); } @@ -878,6 +876,8 @@ export default class SegmentLoader extends videojs.EventTarget { // calculate the download bandwidth based on segment request this.roundTrip = request.roundTripTime; this.bandwidth = request.bandwidth; + + // update analytics stats this.mediaBytesTransferred += request.bytesReceived || 0; this.mediaRequests += 1; this.mediaTransferDuration += request.roundTripTime || 0; @@ -1006,6 +1006,7 @@ export default class SegmentLoader extends videojs.EventTarget { this.syncController_.probeSegmentInfo(segmentInfo); if (segmentInfo.isSyncRequest) { + this.trigger('syncinfoupdate'); this.pendingSegment_ = null; this.state = 'READY'; return; @@ -1069,16 +1070,24 @@ export default class SegmentLoader extends videojs.EventTarget { this.state = 'READY'; + this.mediaIndex = segmentInfo.mediaIndex; + this.fetchAtBuffer_ = true; + this.currentTimeline_ = segmentInfo.timeline; + + // We must update the syncinfo to recalculate the seekable range before + // the following conditional otherwise it may consider this a bad "guess" + // and attempt to resync when the post-update seekable window and live + // point would mean that this was the perfect segment to fetch + this.trigger('syncinfoupdate'); + // If we previously appended a segment that ends more than 3 targetDurations before // the currentTime_ that means that our conservative guess was too conservative. // In that case, reset the loader state so that we try to use any information gained // from the previous request to create a new, more accurate, sync-point. if (segment.end && this.currentTime_() - segment.end > segmentInfo.playlist.targetDuration * 3) { - this.resetLoader(); - } else { - this.mediaIndex = segmentInfo.mediaIndex; - this.fetchAtBuffer_ = true; + this.resetEverything(); + return; } // Don't do a rendition switch unless the SegmentLoader is already walking forward diff --git a/src/sync-controller.js b/src/sync-controller.js index d450be1f..a23f0f07 100644 --- a/src/sync-controller.js +++ b/src/sync-controller.js @@ -368,7 +368,7 @@ export default class SyncController extends videojs.EventTarget { } else { return false; } - this.trigger('syncinfoupdate'); + return true; } diff --git a/test/master-playlist-controller.test.js b/test/master-playlist-controller.test.js index b6f984d2..c47aa962 100644 --- a/test/master-playlist-controller.test.js +++ b/test/master-playlist-controller.test.js @@ -831,6 +831,30 @@ function(assert) { Playlist.seekable = origSeekable; }); +QUnit.test('syncInfoUpdate triggers seekablechanged when seekable is updated', +function(assert) { + let origSeekable = Playlist.seekable; + let mpc = this.masterPlaylistController; + let tech = this.player.tech_; + let mainTimeRanges = []; + let media = {}; + let seekablechanged = 0; + + tech.on('seekablechanged', () => seekablechanged++); + + Playlist.seekable = () => { + return videojs.createTimeRanges(mainTimeRanges); + }; + this.masterPlaylistController.masterPlaylistLoader_.media = () => media; + + mainTimeRanges = [[0, 10]]; + mpc.seekable_ = videojs.createTimeRanges(); + mpc.onSyncInfoUpdate_(); + assert.equal(seekablechanged, 1, 'seekablechanged triggered'); + + Playlist.seekable = origSeekable; +}); + QUnit.test('calls to update cues on new media', function(assert) { let origHlsOptions = videojs.options.hls; diff --git a/test/playback-watcher.test.js b/test/playback-watcher.test.js index 400bfd69..070b6c79 100644 --- a/test/playback-watcher.test.js +++ b/test/playback-watcher.test.js @@ -369,6 +369,32 @@ QUnit.test('seeks to live point if we try to seek outside of seekable', function assert.equal(seeks.length, 4, 'did not seek'); }); +QUnit.test('calls fixesBadSeeks_ on seekablechanged', function(assert) { + // set an arbitrary live source + this.player.src({ + src: 'liveStart30sBefore.m3u8', + type: 'application/vnd.apple.mpegurl' + }); + + // start playback normally + this.player.tech_.triggerReady(); + this.clock.tick(1); + standardXHRResponse(this.requests.shift()); + openMediaSource(this.player, this.clock); + this.player.tech_.trigger('play'); + this.player.tech_.trigger('playing'); + this.clock.tick(1); + + let playbackWatcher = this.player.tech_.hls.playbackWatcher_; + let fixesBadSeeks_ = 0; + + playbackWatcher.fixesBadSeeks_ = () => fixesBadSeeks_++; + + this.player.tech_.trigger('seekablechanged'); + + assert.equal(fixesBadSeeks_, 1, 'fixesBadSeeks_ was called'); +}); + QUnit.module('PlaybackWatcher isolated functions', { beforeEach() { monitorCurrentTime_ = PlaybackWatcher.prototype.monitorCurrentTime_; diff --git a/test/segment-loader.test.js b/test/segment-loader.test.js index 7dc0c34f..e1b00d42 100644 --- a/test/segment-loader.test.js +++ b/test/segment-loader.test.js @@ -26,6 +26,7 @@ QUnit.module('Segment Loader', { this.clock = this.env.clock; this.requests = this.env.requests; this.mse = useFakeMediaSource(); + this.currentTime = 0; this.seekable = { length: 0 }; @@ -37,21 +38,18 @@ QUnit.module('Segment Loader', { this.timescale = sinon.stub(mp4probe, 'timescale'); this.startTime = sinon.stub(mp4probe, 'startTime'); - currentTime = 0; mediaSource = new videojs.MediaSource(); mediaSource.trigger('sourceopen'); - syncController = new SyncController(); + this.syncController = new SyncController(); decrypter = worker(Decrypter); loader = new SegmentLoader({ hls: this.fakeHls, - currentTime() { - return currentTime; - }, + currentTime: () => this.currentTime, seekable: () => this.seekable, seeking: () => false, hasPlayed: () => true, mediaSource, - syncController, + syncController: this.syncController, decrypter, loaderType: 'main' }); @@ -199,7 +197,7 @@ QUnit.test('regularly checks the buffer while unpaused', function(assert) { assert.equal(this.requests.length, 0, 'no outstanding requests'); // play some video to drain the buffer - currentTime = Config.GOAL_BUFFER_LENGTH; + this.currentTime = Config.GOAL_BUFFER_LENGTH; this.clock.tick(10 * 1000); assert.equal(this.requests.length, 1, 'requested another segment'); @@ -606,6 +604,39 @@ QUnit.test('detects init segment changes and downloads it', function(assert) { 'did not re-request the init segment'); }); +QUnit.test('triggers syncinfoupdate before attempting a resync', function(assert) { + let syncInfoUpdates = 0; + + loader.playlist(playlistWithDuration(20)); + loader.mimeType(this.mimeType); + loader.load(); + this.clock.tick(1); + + let sourceBuffer = mediaSource.sourceBuffers[0]; + + this.seekable = videojs.createTimeRanges([[0, 10]]); + this.syncController.probeSegmentInfo = (segmentInfo) => { + let segment = segmentInfo.segment; + + segment.end = 10; + }; + loader.on('syncinfoupdate', () => { + syncInfoUpdates++; + // Simulate the seekable window updating + this.seekable = videojs.createTimeRanges([[200, 210]]); + // Simulate the seek to live that should happen in playback-watcher + this.currentTime = 210; + }); + + this.requests[0].response = new Uint8Array(10).buffer; + this.requests.shift().respond(200, null, ''); + sourceBuffer.trigger('updateend'); + this.clock.tick(1); + + assert.equal(loader.mediaIndex, null, 'mediaIndex reset by seek to seekable'); + assert.equal(syncInfoUpdates, 1, 'syncinfoupdate was triggered'); +}); + QUnit.test('cancels outstanding requests on abort', function(assert) { loader.playlist(playlistWithDuration(20)); loader.mimeType(this.mimeType); @@ -689,7 +720,7 @@ QUnit.test('SegmentLoader.mediaIndex is adjusted when live playlist is updated', QUnit.test('segmentInfo.mediaIndex is adjusted when live playlist is updated', function(assert) { // Setting currentTime to 31 so that we start requesting at segment #3 - currentTime = 31; + this.currentTime = 31; loader.playlist(playlistWithDuration(50, { mediaSequence: 0, endList: false @@ -1417,7 +1448,7 @@ QUnit.module('Segment Loading Calculation', { this.hasPlayed = true; this.clock = this.env.clock; - currentTime = 0; + this.currentTime = 0; syncController = new SyncController(); loader = new SegmentLoader({ currentTime() {