From a82cef5e17f519f8a34a7e38a17ebcd5d262057b Mon Sep 17 00:00:00 2001 From: Timothy Drews Date: Thu, 23 Jul 2015 16:10:35 -0700 Subject: [PATCH] Re-work seek handling during stream startup. * Instead of intercepting specific 'seeking' events to forego stream resync, record the particular seeks using a member variable, which is less error prone. * Don't rely on 'playing' events for stream resync after pause/play: 'playing' events aren't reliable; instead, just check if we need to clamp the playhead when we update the seek range. * Don't fire seeking events when starting the video at t=0, or when there is no timestamp correction. Closes #132 Closes #136 Change-Id: I350ee6e9966af9f44d3e8bda4dc8297271e41855 --- lib/player/stream_video_source.js | 278 ++++++++++++++++++++---------- spec/player_integration.js | 99 +++++++++-- 2 files changed, 269 insertions(+), 108 deletions(-) diff --git a/lib/player/stream_video_source.js b/lib/player/stream_video_source.js index 4b51f93f7f..1e890cddd3 100644 --- a/lib/player/stream_video_source.js +++ b/lib/player/stream_video_source.js @@ -122,6 +122,9 @@ shaka.player.StreamVideoSource = function(manifestInfo, estimator, abrManager) { /** @private {shaka.player.DrmSchemeInfo.Restrictions} */ this.cachedRestrictions_ = null; + /** @private {?number} */ + this.ignoreSeek_ = null; + /** @private {number} */ this.originalPlaybackRate_ = 1; @@ -162,6 +165,13 @@ goog.inherits(shaka.player.StreamVideoSource, shaka.util.FakeEventTarget); shaka.player.StreamVideoSource.MIN_UPDATE_PERIOD_ = 3; +/** + * @const {number} + * @private + */ +shaka.player.StreamVideoSource.SEEK_TOLERANCE_ = 0.01; + + /** * @override * @suppress {checkTypes} to set otherwise non-nullable types to null. @@ -1116,10 +1126,22 @@ shaka.player.StreamVideoSource.prototype.startStreams_ = function( streamStartTime = streamLimits.start; } - // Set the video's current time before starting the streams so that the - // streams begin buffering at the stream start time. shaka.log.info('Starting each stream from', streamStartTime); - this.video.currentTime = streamStartTime; + + // Start listening to 'seeking' events right away as we must handle seeking + // during stream startup. + this.eventManager.listen(this.video, 'seeking', this.onSeeking_.bind(this)); + + if (this.video.currentTime != streamStartTime) { + // Set the video's current time before starting the streams so that the + // streams start buffering at the stream start time. + this.video.currentTime = streamStartTime; + + // Ignore the resulting 'seeking' event since there's no need to resync the + // streams before buffering (see onSeeking_). + this.ignoreSeek_ = streamStartTime; + shaka.log.debug('Ignoring pending seek to', this.ignoreSeek_); + } // Inform the application of the initial seek range. this.fireSeekRangeChangedEvent_(streamLimits.start, streamLimits.end); @@ -1297,34 +1319,80 @@ shaka.player.StreamVideoSource.prototype.beginPlayback_ = function( this.fireSeekRangeChangedEvent_(streamLimits.start, streamLimits.end); } + // Determine the corrected current time and adjust the video's current time + // as necessary. If |maxTimestampCorrection_| is 0 then don't modify + // video.currentTime to avoid firing an unnecessary 'seeking' event. + // // Note that its particularly important to adjust the video's current time if // |maxTimestampCorrection_| is positive; otherwise, some streams may get // stuck, as their first segment may start after the beginning of their // SegmentIndex. - shaka.log.debug( - 'Adjusting video.currentTime by', - this.maxTimestampCorrection_, - 'seconds.'); - var currentTime = this.video.currentTime; - var correctedCurrentTime = currentTime + this.maxTimestampCorrection_; + var correctedCurrentTime; + if (this.maxTimestampCorrection_ != 0) { + shaka.log.debug( + 'Adjusting video.currentTime by', + this.maxTimestampCorrection_, + 'seconds.'); + correctedCurrentTime = + this.video.currentTime + this.maxTimestampCorrection_; + this.video.currentTime = correctedCurrentTime; + + // Ignore the resulting 'seeking' event since there's no need to resync the + // streams (see onSeeking_). + this.ignoreSeek_ = correctedCurrentTime; + shaka.log.debug('Ignoring pending seek to', this.ignoreSeek_); + } else { + correctedCurrentTime = this.video.currentTime; + } + shaka.asserts.assert(correctedCurrentTime != null); + + // Sanity check: check that |correctedCurrentTime| is within the stream + // limits. if (!COMPILED && streamLimits) { - if (correctedCurrentTime < streamLimits.start || - correctedCurrentTime > streamLimits.end) { + // For live content, if the available bandwidth is really low (e.g., lower + // than the bandwidth specified in the manifest) then it's possible for + // |correctedCurrentTime| to be legitimately outside of the stream limits, + // i.e., the seek window may move past the corrected playhead. + // + // Note: since video.currentTime may have less precision than + // |maxTimestampCorrection_|, include a tolerance. + var tolerance = 10e-6; + + if (this.manifestInfo.live && + correctedCurrentTime < streamLimits.start - tolerance) { + shaka.log.debug( + 'correctedCurrentTime (' + correctedCurrentTime + ')', + 'is outside of the stream limits before beginning playback!'); + } + + if ((!this.manifestInfo.live && + correctedCurrentTime < streamLimits.start - tolerance) || + (correctedCurrentTime > streamLimits.end + tolerance)) { shaka.log.error( - 'video.currentTime (' + correctedCurrentTime + ')', + 'correctedCurrentTime (' + correctedCurrentTime + ')', 'should be within the stream limits', - JSON.stringify(streamLimits)); + [streamLimits.start, streamLimits.end]); } } - this.video.currentTime = correctedCurrentTime; - if (this.manifestInfo.live) { + if (this.manifestInfo.live && streamLimits) { // While the streams are starting the live-edge is moving. So, the video's - // current time may be behind the live-edge by a few seconds. This may - // cause a poor UX since the UI might say "-00:03" instead of "LIVE". So, - // record the offset so we can use it when we compute the stream limits. + // current time may be behind the live-edge by a few seconds (note that + // |streamLimits| has already been corrected). This may cause a poor UX + // since the UI might say "-00:03" instead of "LIVE". So, record an offset + // to apply to the stream limits in the future. this.liveEdgeOffset_ = streamLimits.end - correctedCurrentTime; - shaka.asserts.assert(this.liveEdgeOffset_ >= 0); + + // Sanity check: check that |liveEdgeOffset_| is non-negative. + if (!COMPILED) { + // Note: since video.currentTime may have less precision than + // |maxTimestampCorrection_|, include a tolerance. + var tolerance = 10e-6; + shaka.asserts.assert(this.liveEdgeOffset_ >= -tolerance, + 'liveEdgeOffset_ should not be less than zero.'); + } + + this.liveEdgeOffset_ = Math.max(this.liveEdgeOffset_, 0); shaka.log.debug('Live-edge offset', this.liveEdgeOffset_); } @@ -1335,27 +1403,6 @@ shaka.player.StreamVideoSource.prototype.beginPlayback_ = function( // or after playback begins. this.video.playbackRate = this.originalPlaybackRate_; - // Start listening to 'playing' events, as we may need to seek back into the - // seek window after pausing and playing. Ignore the first 'playing' event - // which is caused by beginning playback. - var tempOnPlaying = (function(event) { - shaka.log.v1('Ignoring first \'playing\' event.'); - var video = /** @type {!EventTarget} */ (this.video); - this.eventManager.unlisten(video, 'playing'); - this.eventManager.listen(video, 'playing', this.onPlaying_.bind(this)); - }).bind(this); - this.eventManager.listen(this.video, 'playing', tempOnPlaying); - - // Start listening to 'seek' events. Ignore the first 'seeking' event which - // is caused by the seek operation above. - var tempOnSeeking = (function(event) { - shaka.log.v1('Ignoring first \'seeking\' event.'); - var video = /** @type {!EventTarget} */ (this.video); - this.eventManager.unlisten(video, 'seeking'); - this.eventManager.listen(video, 'seeking', this.onSeeking_.bind(this)); - }).bind(this); - this.eventManager.listen(this.video, 'seeking', tempOnSeeking); - if (this.manifestInfo.updatePeriod != null) { // Ensure the next update occurs within |manifestInfo.updatePeriod| seconds // by taking into account the time it took to start the streams. @@ -1368,20 +1415,45 @@ shaka.player.StreamVideoSource.prototype.beginPlayback_ = function( /** - * Computes a new seek range and fires a 'seekrangechanged' event. + * Computes a new seek range and fires a 'seekrangechanged' event. Also clamps + * the playhead to the seek start time during playback. * * @private */ shaka.player.StreamVideoSource.prototype.onUpdateSeekRange_ = function() { this.seekRangeTimer_ = null; + this.setSeekRangeTimer_(); var streamLimits = this.computeStreamLimits_(this.getSegmentIndexes_()); shaka.asserts.assert(streamLimits, 'Stream limits should not be null.'); - if (streamLimits) { - this.fireSeekRangeChangedEvent_(streamLimits.start, streamLimits.end); + if (!streamLimits) { + return; } - this.setSeekRangeTimer_(); + this.fireSeekRangeChangedEvent_(streamLimits.start, streamLimits.end); + + if (this.video.paused) { + return; + } + + // Clamping the playhead to the right here ensures that if the user pauses + // and then plays the video then the playhead is moved into the seekable + // range. Note that if the playhead moves to the right of the seekable range + // during playback then some of the streams must be buffering, so there's no + // need to clamp the playhead. + // TODO: Add live integration test that covers this case. + var currentTime = this.video.currentTime; + var start = streamLimits.start; + var end = streamLimits.end; + if (this.clampPlayheadToRight_(currentTime, start, end)) { + shaka.log.warning( + 'Playhead is outside of the seekable range:', + 'seekable', [start, end], + 'attempted', currentTime, + 'Adjusting...'); + // If the video's current time was clamped then there will be a + // 'seeking' event which is handled by onSeeking_(). + } }; @@ -1406,18 +1478,6 @@ shaka.player.StreamVideoSource.prototype.fireSeekRangeChangedEvent_ = function( }; -/** - * Video playing callback. - * - * @param {!Event} event - * @private - */ -shaka.player.StreamVideoSource.prototype.onPlaying_ = function(event) { - shaka.log.v1('onPlaying_', event); - this.clampCurrentTime_(); -}; - - /** * Video seeking callback. * @@ -1426,7 +1486,39 @@ shaka.player.StreamVideoSource.prototype.onPlaying_ = function(event) { */ shaka.player.StreamVideoSource.prototype.onSeeking_ = function(event) { shaka.log.v1('onSeeking_', event); - if (!this.clampCurrentTime_()) { + + var currentTime = this.video.currentTime; + + if (this.ignoreSeek_ != null) { + var tolerance = shaka.player.StreamVideoSource.SEEK_TOLERANCE_; + if ((currentTime >= this.ignoreSeek_ - tolerance) && + (currentTime <= this.ignoreSeek_ + tolerance)) { + shaka.log.debug('Ignored seek to', this.ignoreSeek_); + this.ignoreSeek_ = null; + return; + } + this.ignoreSeek_ = null; + } + + var streamLimits = this.computeStreamLimits_(this.getSegmentIndexes_()); + shaka.asserts.assert(streamLimits, 'Stream limits should not be null.'); + if (!streamLimits) { + return; + } + + var start = streamLimits.start; + var end = streamLimits.end; + if (this.clampPlayheadToRight_(currentTime, start, end) || + this.clampPlayheadToLeft_(currentTime, end)) { + shaka.log.warning( + 'Playhead has been moved outside of the seekable range:', + 'seekable', [start, end], + 'attempted', currentTime, + 'Adjusting...'); + // If the video's current time was clamped then there will be another + // 'seeking' event, so this function will get called again and the 'else' + // branch below will be executed. + } else { for (var type in this.streamsByType_) { this.streamsByType_[type].resync(); } @@ -1435,53 +1527,53 @@ shaka.player.StreamVideoSource.prototype.onSeeking_ = function(event) { /** - * Clamps the video's current time to the seek range if it is outside of the - * seek range. + * Clamps the video's current time to the right of the given start time + * (inclusive). * - * @return {boolean} True if the video's current time was clamped. + * @param {number} currentTime The video's current time. + * @param {number} start The start time in seconds. + * @param {number} end The end time in seconds, which must be provided to + * ensure that the playhead is not adjusted too far right. + * @return {boolean} True if the video's current was clamped, in which case a + * 'seeking' event will be fired by the video. * @private */ -shaka.player.StreamVideoSource.prototype.clampCurrentTime_ = function() { - var streamLimits = this.computeStreamLimits_(this.getSegmentIndexes_()); - shaka.asserts.assert(streamLimits, 'Stream limits should not be null.'); - if (!streamLimits) { +shaka.player.StreamVideoSource.prototype.clampPlayheadToRight_ = function( + currentTime, start, end) { + if (currentTime >= start - shaka.player.StreamVideoSource.SEEK_TOLERANCE_) { return false; } - var currentTime = this.video.currentTime; + // For live content, if we re-position the playhead too close to the seek + // start time then we may end up outside of the seek range again, as the seek + // window may be moving or we may have to buffer after we re-position. So, + // re-position the playhead ahead of the seek start time to compensate. + var compensation = this.manifestInfo.live ? + this.manifestInfo.minBufferTime : + 0; + + this.video.currentTime = Math.min(start + compensation, end); + return true; +}; - // Rounding tolerance. - var tolerance = 0.01; - if ((currentTime >= streamLimits.start - tolerance) && - (currentTime <= streamLimits.end + tolerance)) { +/** + * Clamps the video's current time to the left of the given end time + * (inclusive). + * + * @param {number} currentTime The video's current time. + * @param {number} end The end time in seconds. + * @return {boolean} True if the video's current was clamped, in which case a + * 'seeking' event will be fired by the video. + * @private + */ +shaka.player.StreamVideoSource.prototype.clampPlayheadToLeft_ = function( + currentTime, end) { + if (currentTime <= end + shaka.player.StreamVideoSource.SEEK_TOLERANCE_) { return false; } - // If we seek outside the seekable range then clamp the video's current time - // to the seekable range; this will trigger another 'seeking' event. - shaka.log.warning( - 'Cannot seek outside of seekable range:', - 'seekable', [streamLimits.start, streamLimits.end], - 'attempted', currentTime); - - var targetTime; - if (currentTime < streamLimits.start) { - // For live content, if we re-position the playhead too close to the seek - // start time then we may end up outside of the seek range again, as the - // seek window may be moving or we may have to buffer after we - // re-position. So, re-position the playhead ahead of the seek start - // time to compensate. - var compensation = this.manifestInfo.live ? - this.manifestInfo.minBufferTime : - 0; - targetTime = Math.min(streamLimits.start + compensation, streamLimits.end); - } else { - shaka.asserts.assert(currentTime > streamLimits.end); - targetTime = streamLimits.end; - } - this.video.currentTime = targetTime; - + this.video.currentTime = end; return true; }; diff --git a/spec/player_integration.js b/spec/player_integration.js index 86ecfbfa0e..6dc09545b6 100644 --- a/spec/player_integration.js +++ b/spec/player_integration.js @@ -144,6 +144,28 @@ describe('Player', function() { done(); }); }); + + // Before playback begins there may be an intial seek to the stream start + // time if one of the streams doesn't start at 0, and there may be another + // seek from applying a timestamp correction. So, if the streams all start + // at 0 and have no timestamp correction then there should be no 'seeking' + // events. + it('doesn\'t fire unnecessary \'seeking\' events.', function(done) { + var source = newSource(plainManifest); + eventManager.listen(video, 'seeking', function() { fail(); }); + + player.load(source).then(function() { + video.play(); + return waitForMovement(video, eventManager); + }).then(function() { + // Add an "expect" just so Jasmine doesn't complain. + expect(true).toBeTruthy(); + done(); + }).catch(function(error) { + fail(error); + done(); + }); + }); }); describe('selectVideoTrack', function() { @@ -381,7 +403,7 @@ describe('Player', function() { expect(ok).toBe(true); video.currentTime = 30.0; - return waitForTargetTime(video, eventManager, 33.0, 7.0); + return waitForTargetTime(video, eventManager, 33.0, 8.0); }).then(function() { done(); }).catch(function(error) { @@ -390,31 +412,78 @@ describe('Player', function() { }); }); - it('fires an event when seeking by zero ', function(done) { - // StreamVideoSource assumes that seeking by zero will fire a seek event. + // Starts the video 25 seconds in and then seeks back near the beginning + // before stream startup (initial buffering) has completed. Playback should + // begin from the seeked-to location and not hang. + it('can be used during stream startup w/ large < 0 seek', function(done) { + streamStartupTest(25, 3, done); + }); + + // The same as the above test, but tests on the boundary. + it('can be used during stream startup w/ small < 0 seek)', function(done) { + var tolerance = shaka.player.StreamVideoSource.SEEK_TOLERANCE_; + streamStartupTest(25, 25 - (tolerance / 2), done); + }); + + it('can be used during stream startup w/ large > 0 seek', function(done) { + streamStartupTest(25, 35, done); + }); + + function streamStartupTest(playbackStartTime, seekTarget, done) { var source = newSource(plainManifest); - var onSeeking = jasmine.createSpy('onSeeking'); + + player.configure({'streamBufferSize': 80}); + player.setPlaybackStartTime(playbackStartTime); + + // Force @minBufferTime to a large value so we have enough time to seek + // during startup. + var originalLoad = shaka.player.StreamVideoSource.prototype.load; + shaka.player.StreamVideoSource.load = function(preferredLanguage) { + expect(this.manifestInfo).not.toBe(null); + this.manifestInfo.minBufferTime = 80; + return this.originalLoad(preferredLanguage); + }; + + var pollTimer = null; player.load(source).then(function() { video.play(); - return waitForMovement(video, eventManager); - }).then(function() { - video.addEventListener('seeking', onSeeking); - video.currentTime += 0; - expect(onSeeking.calls.count()).toEqual(0); - return delay(0.5); + + // Continue once we've buffered at least one segment. + var p = shaka.util.PublicPromise(); + var pollBuffer = function() { + if (video && video.buffered.length == 1) { + window.clearInterval(pollTimer); + p.resolve(); + } + }; + pollTimer = window.setInterval(pollBuffer, 25); + return p; }).then(function() { - video.currentTime -= 0; - expect(onSeeking.calls.count()).toEqual(1); - return delay(0.5); + // Ensure we have buffered at least one segment but have not yet + // started playback. + expect(video.buffered.length).toBe(1); + expect(video.playbackRate).toBe(0); + // Now seek back near the beginning. + video.currentTime = seekTarget; + return delay(3); }).then(function() { - expect(onSeeking.calls.count()).toEqual(2); + expect(video.buffered.length).toBe(1); + expect(video.playbackRate).toBe(1); + expect(video.currentTime > seekTarget); + shaka.player.StreamVideoSource.load = originalLoad; done(); }).catch(function(error) { + shaka.player.StreamVideoSource.load = originalLoad; + + if (pollTimer != null) { + window.clearTimeout(pollTimer); + } + fail(error); done(); }); - }); + } }); describe('enableTextTrack', function() {