Skip to content

Commit

Permalink
Fix several bugs with multi-Period content.
Browse files Browse the repository at this point in the history
First, Player should not call AbrManager.setVariants as part of load().
Before, it would set them for the first Period; however, this doesn't
work with a start time in another Period.  StreamingEngine will call
chooseStreams_ for the starting Period before startup finishes.

Second, we should handle Period transitions before we handle buffering
goal.  Before, we would wait until the playhead moves closer to the
Period transition even if we have buffered the entire Period.  This
can cause problems when seeking, especially with text content that
buffers quickly.  If we seek and we have buffered text but not video,
the pipeline will stall since text is waiting for the playhead to move
while video is waiting for the Period transition.

Lastly, it is possible for multiple Period transitions to occur closely
together.  If we seek into a Period that is not setup yet, and then
seek back to a Period that is setup, then the second transition will
complete and the first will override it once it finishes setting up
the Period.  We should stop any old transitions if another starts.

Issue #655

Change-Id: Iab8961c606a65487704c9f0efaa255db0e3dc942
  • Loading branch information
TheModMaker authored and joeyparrish committed Jan 28, 2017
1 parent 6a01da0 commit 01b82ce
Show file tree
Hide file tree
Showing 3 changed files with 42 additions and 30 deletions.
49 changes: 30 additions & 19 deletions lib/media/streaming_engine.js
Original file line number Diff line number Diff line change
Expand Up @@ -824,16 +824,6 @@ shaka.media.StreamingEngine.prototype.update_ = function(mediaState) {
rebufferingGoal,
this.bufferingGoalScale_ * this.config_.bufferingGoal);

// If we've buffered to the buffering goal then schedule an update.
if (bufferedAhead >= bufferingGoal) {
shaka.log.v2(logPrefix, 'buffering goal met');

// Do not try to schedule the next update. Just poll twice every second.
// The playback rate can change at any time, so any prediction we make now
// could be terribly invalid soon.
return 0.5;
}

// Check if we've buffered to the end of the presentation.
if (timeNeeded >= this.manifest_.presentationTimeline.getDuration()) {
// We shouldn't rebuffer if the playhead is close to the end of the
Expand All @@ -859,6 +849,16 @@ shaka.media.StreamingEngine.prototype.update_ = function(mediaState) {
return null;
}

// If we've buffered to the buffering goal then schedule an update.
if (bufferedAhead >= bufferingGoal) {
shaka.log.v2(logPrefix, 'buffering goal met');

// Do not try to predict the next update. Just poll twice every second.
// The playback rate can change at any time, so any prediction we make now
// could be terribly invalid soon.
return 0.5;
}

reference = this.getSegmentReferenceNeeded_(
mediaState, playheadTime, bufferEnd, currentPeriodIndex);
if (!reference) {
Expand Down Expand Up @@ -1504,6 +1504,25 @@ shaka.media.StreamingEngine.prototype.handlePeriodTransition_ = function(
this.setupPeriod_(needPeriodIndex).then(function() {
if (this.destroyed_) return;

// If we seek during a Period transition, we can start another transition.
// So we need to verify that:
// - We are still in need of the same Period.
// - All streams are still idle.
// - The current stream is not in the needed Period (another transition
// handled it).
var allReady = mediaStates.every(function(ms) {
var isIdle = shaka.media.StreamingEngine.isIdle_(ms);
var currentPeriodIndex = this.findPeriodContainingStream_(ms.stream);
return isIdle && ms.needPeriodIndex == needPeriodIndex &&
currentPeriodIndex != needPeriodIndex;
}.bind(this));
if (!allReady) {
// TODO: Write unit tests for this case.
shaka.log.debug(logPrefix, 'ignoring transition to Period',
needPeriodIndex, 'since another is happening');
return;
}

var needPeriod = this.manifest_.periods[needPeriodIndex];

shaka.log.v1(logPrefix, 'calling onChooseStreams_()...');
Expand Down Expand Up @@ -1536,15 +1555,7 @@ shaka.media.StreamingEngine.prototype.handlePeriodTransition_ = function(
for (var type in this.mediaStates_) {
var stream = streamsByType[type];
this.switch(type, stream, /* clearBuffer */ false);

var mediaState = this.mediaStates_[type];
if (shaka.media.StreamingEngine.isIdle_(mediaState)) {
this.scheduleUpdate_(mediaState, 0);
} else {
// TODO: Write unit tests to cover this case.
shaka.log.debug(logPrefix,
'seeked() was called while waiting for setupPeriod_()');
}
this.scheduleUpdate_(this.mediaStates_[type], 0);
}

// We've already set up the Period so call onCanSwitch_() right now.
Expand Down
3 changes: 1 addition & 2 deletions lib/player.js
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,7 @@ shaka.Player.prototype.load = function(manifestUri, opt_startTime,
this.mediaSourceOpen_
]);
}.bind(this)).then(function() {
this.config_.abr.manager.init(this.switch_.bind(this));

// MediaSource is open, so create the Playhead, MediaSourceEngine, and
// StreamingEngine.
Expand All @@ -456,8 +457,6 @@ shaka.Player.prototype.load = function(manifestUri, opt_startTime,
// Since the first streams just became active, send an adaptation event.
this.onAdaptation_();

this.config_.abr.manager.init(this.switch_.bind(this));

this.loadChain_ = null;
}.bind(this)).finalize().catch(function(error) {
goog.asserts.assert(error instanceof shaka.util.Error,
Expand Down
20 changes: 11 additions & 9 deletions test/media/streaming_engine_unit.js
Original file line number Diff line number Diff line change
Expand Up @@ -679,9 +679,10 @@ describe('StreamingEngine', function() {

// Seek backwards to a buffered region in the first Period. Note that
// since the buffering goal is 5 seconds and each segment is 10
// seconds long, the first segment of the second Period should be
// required when the playhead is at the 16 second mark.
expect(playhead.getTime()).toBe(16);
// seconds long, the second segment of this Period will be required at
// 6 seconds. Then it will load the next Period, but not require the
// new segments.
expect(playhead.getTime()).toBe(6);
playheadTime -= 5;
streamingEngine.seeked();

Expand Down Expand Up @@ -751,9 +752,9 @@ describe('StreamingEngine', function() {
mediaSourceEngine.endOfStream.and.callFake(function() {
// Seek backwards to a buffered region in the first Period. Note
// that since the buffering goal is 5 seconds and each segment is
// 10 seconds long, endOfStream() should be called at the 36 second
// mark.
expect(playhead.getTime()).toBe(36);
// 10 seconds long, the last segment should be required at 26 seconds.
// Then endOfStream() should be called.
expect(playhead.getTime()).toBe(26);
playheadTime -= 20;
streamingEngine.seeked();
});
Expand Down Expand Up @@ -876,8 +877,9 @@ describe('StreamingEngine', function() {

// Seek backwards to an unbuffered region in the first Period. Note
// that since the buffering goal is 5 seconds and each segment is 10
// seconds long, endOfStream() should be called at the 36 second mark.
expect(playhead.getTime()).toBe(36);
// seconds long, the last segment should be required at 26 seconds.
// Then endOfStream() should be called.
expect(playhead.getTime()).toBe(26);
playheadTime -= 20;
streamingEngine.seeked();

Expand Down Expand Up @@ -1022,7 +1024,7 @@ describe('StreamingEngine', function() {
mediaSourceEngine.endOfStream.and.callFake(function() {
// Seek backwards to an unbuffered region in the second Period. Do not
// call seeked().
expect(playhead.getTime()).toBe(36);
expect(playhead.getTime()).toBe(26);
playheadTime -= 10;
});

Expand Down

0 comments on commit 01b82ce

Please sign in to comment.