Skip to content

Commit

Permalink
fix: fix for streams that would occasionally never fire an ended event
Browse files Browse the repository at this point in the history
* fix: Only fire `ended` when sourcebuffers are not updating

* fix logging for audio playlists

* more logs

* fix updating check

* cr

* add test
  • Loading branch information
brandonocasey authored and misteroneill committed Mar 5, 2019
1 parent a4c056e commit fc09926
Show file tree
Hide file tree
Showing 4 changed files with 75 additions and 13 deletions.
2 changes: 2 additions & 0 deletions src/master-playlist-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -699,6 +699,7 @@ export class MasterPlaylistController extends videojs.EventTarget {
return;
}

this.logger_(`calling mediaSource.endOfStream()`);
// on chrome calling endOfStream can sometimes cause an exception,
// even when the media source is in a valid state.
try {
Expand Down Expand Up @@ -981,6 +982,7 @@ export class MasterPlaylistController extends videojs.EventTarget {
// on firefox setting the duration may sometimes cause an exception
// even if the media source is open and source buffers are not
// updating, something about the media source being in an invalid state.
this.logger_(`Setting duration from ${this.mediaSource.duration} => ${newDuration}`);
try {
this.mediaSource.duration = newDuration;
} catch (e) {
Expand Down
39 changes: 27 additions & 12 deletions src/segment-loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -445,9 +445,17 @@ export default class SegmentLoader extends videojs.EventTarget {
};
}

const oldId = oldPlaylist ? oldPlaylist.id : null;
let oldId = null;

this.logger_(`playlist update [${oldId} => ${newPlaylist.id}]`);
if (oldPlaylist) {
if (oldPlaylist.id) {
oldId = oldPlaylist.id;
} else if (oldPlaylist.uri) {
oldId = oldPlaylist.uri;
}
}

this.logger_(`playlist update [${oldId} => ${newPlaylist.id || newPlaylist.uri}]`);

// in VOD, this is always a rendition switch (or we updated our syncInfo above)
// in LIVE, we always want to update with new playlists (including refreshes)
Expand Down Expand Up @@ -665,11 +673,7 @@ export default class SegmentLoader extends videojs.EventTarget {
return;
}

let isEndOfStream = detectEndOfStream(this.playlist_,
this.mediaSource_,
segmentInfo.mediaIndex);

if (isEndOfStream) {
if (this.isEndOfStream_(segmentInfo.mediaIndex)) {
this.endOfStream();
return;
}
Expand Down Expand Up @@ -698,6 +702,21 @@ export default class SegmentLoader extends videojs.EventTarget {
this.loadSegment_(segmentInfo);
}

/**
* Determines if this segment loader is at the end of it's stream.
*
* @param {Number} mediaIndex the index of segment we last appended
* @param {Object} [playlist=this.playlist_] a media playlist object
* @returns {Boolean} true if at end of stream, false otherwise.
*/
isEndOfStream_(mediaIndex, playlist = this.playlist_) {
return detectEndOfStream(
playlist,
this.mediaSource_,
mediaIndex
) && !this.sourceUpdater_.updating();
}

/**
* Determines what segment request should be made, given current playback
* state.
Expand Down Expand Up @@ -1333,11 +1352,7 @@ export default class SegmentLoader extends videojs.EventTarget {
// any time an update finishes and the last segment is in the
// buffer, end the stream. this ensures the "ended" event will
// fire if playback reaches that point.
const isEndOfStream = detectEndOfStream(segmentInfo.playlist,
this.mediaSource_,
segmentInfo.mediaIndex + 1);

if (isEndOfStream) {
if (this.isEndOfStream_(segmentInfo.mediaIndex + 1, segmentInfo.playlist)) {
this.endOfStream();
}

Expand Down
5 changes: 4 additions & 1 deletion src/source-updater.js
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,10 @@ export default class SourceUpdater {
* @return {Boolean} the updating status of the SourceBuffer
*/
updating() {
return !this.sourceBuffer_ || this.sourceBuffer_.updating || this.pendingCallback_;
// we are updating if the sourcebuffer is updating or
return !this.sourceBuffer_ || this.sourceBuffer_.updating ||
// if we have a pending callback that is not our internal noop
(!!this.pendingCallback_ && this.pendingCallback_ !== noop);
}

/**
Expand Down
42 changes: 42 additions & 0 deletions test/segment-loader.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -615,6 +615,48 @@ QUnit.module('SegmentLoader: M2TS', function(hooks) {
assert.equal(endOfStreams, 1, 'triggered ended');
});

QUnit.test('endOfStream does not happen while sourceUpdater is updating', function(assert) {
let endOfStreams = 0;
let bandwidthupdates = 0;
let buffered = videojs.createTimeRanges();

loader.buffered_ = () => buffered;

loader.playlist(playlistWithDuration(20));
loader.mimeType(this.mimeType);
loader.load();
this.clock.tick(1);

loader.mediaSource_ = {
readyState: 'open',
sourceBuffers: this.mediaSource.sourceBuffers
};

loader.on('ended', () => endOfStreams++);

loader.on('bandwidthupdate', () => {
bandwidthupdates++;
// Simulate a rendition switch
loader.resetEverything();
});

this.requests[0].response = new Uint8Array(10).buffer;
this.requests.shift().respond(200, null, '');
buffered = videojs.createTimeRanges([[0, 10]]);
this.updateend();
this.clock.tick(10);

loader.sourceUpdater_.updating = () => true;
this.requests[0].response = new Uint8Array(10).buffer;
this.requests.shift().respond(200, null, '');
buffered = videojs.createTimeRanges([[0, 10]]);

this.updateend();

assert.equal(bandwidthupdates, 0, 'did not trigger bandwidthupdate');
assert.equal(endOfStreams, 0, 'did not trigger trigger ended');
});

QUnit.test('live playlists do not trigger ended', function(assert) {
let endOfStreams = 0;
let playlist;
Expand Down

0 comments on commit fc09926

Please sign in to comment.