From 97ab7122e2e05c8fdf22a63c13ce20334814e7ca Mon Sep 17 00:00:00 2001 From: Garrett Singer Date: Wed, 17 Feb 2021 10:51:39 -0500 Subject: [PATCH] fix: allow buffer removes when there's no current media info in loader (#1070) Previously, if there wasn't any current media info in the loader, then a remove of buffered contents would not be processed. This would intuitively make sense, as if there wasn't current media info, then in theory there shouldn't be any buffered contents. However, it is possible for there to be buffered contents without current media info in the event of a non destructive rendition switch. This was noticed when switching renditions, expecting a fast quality change to clear the contents and continue playback at a new rendition. Sometimes the remove wouldn't happen, so the content would continue to play back the old buffered content from a different rendition. This fix instead allows the removes so long as there is starting media info (which is available after first segment download). --- src/master-playlist-controller.js | 1 + src/segment-loader.js | 13 ++++++-- test/master-playlist-controller.test.js | 2 +- test/segment-loader.test.js | 41 +++++++++++++++++++++++-- 4 files changed, 52 insertions(+), 5 deletions(-) diff --git a/src/master-playlist-controller.js b/src/master-playlist-controller.js index d05d21ed0..d82ebcda6 100644 --- a/src/master-playlist-controller.js +++ b/src/master-playlist-controller.js @@ -750,6 +750,7 @@ export class MasterPlaylistController extends videojs.EventTarget { */ fastQualityChange_(media = this.selectPlaylist()) { if (media === this.masterPlaylistLoader_.media()) { + this.logger_('skipping fastQualityChange because new media is same as old'); return; } diff --git a/src/segment-loader.js b/src/segment-loader.js index be9ff8f6c..36eee38d4 100644 --- a/src/segment-loader.js +++ b/src/segment-loader.js @@ -1096,7 +1096,8 @@ export default class SegmentLoader extends videojs.EventTarget { end = this.duration_(); } - if (!this.sourceUpdater_ || !this.currentMediaInfo_) { + if (!this.sourceUpdater_ || !this.startingMediaInfo_) { + this.logger_('skipping remove because no source updater or starting media info'); // nothing to remove if we haven't processed any media return; } @@ -1115,7 +1116,15 @@ export default class SegmentLoader extends videojs.EventTarget { this.sourceUpdater_.removeAudio(start, end, removeFinished); } - if (this.loaderType_ === 'main' && this.currentMediaInfo_ && this.currentMediaInfo_.hasVideo) { + // While it would be better to only remove video if the main loader has video, this + // should be safe with audio only as removeVideo will call back even if there's no + // video buffer. + // + // In theory we can check to see if there's video before calling the remove, but in + // the event that we're switching between renditions and from video to audio only + // (when we add support for that), we may need to clear the video contents despite + // what the new media will contain. + if (this.loaderType_ === 'main') { this.gopBuffer_ = removeGopBuffer(this.gopBuffer_, start, end, this.timeMapping_); removesRemaining++; this.sourceUpdater_.removeVideo(start, end, removeFinished); diff --git a/test/master-playlist-controller.test.js b/test/master-playlist-controller.test.js index 0a742e35d..f033eae79 100644 --- a/test/master-playlist-controller.test.js +++ b/test/master-playlist-controller.test.js @@ -534,7 +534,7 @@ QUnit.test('resets everything for a fast quality change', function(assert) { origRemove.call(segmentLoader, start, end); }; - segmentLoader.currentMediaInfo_ = { hasVideo: true }; + segmentLoader.startingMediaInfo_ = { hasVideo: true }; segmentLoader.audioDisabled_ = true; segmentLoader.sourceUpdater_.removeVideo = function(start, end) { diff --git a/test/segment-loader.test.js b/test/segment-loader.test.js index c1c5cfcda..746c7175f 100644 --- a/test/segment-loader.test.js +++ b/test/segment-loader.test.js @@ -2556,6 +2556,44 @@ QUnit.module('SegmentLoader', function(hooks) { }); }); + QUnit.test('does not remove until starting media info', function(assert) { + let audioRemoves = 0; + let videoRemoves = 0; + + return setupMediaSource(loader.mediaSource_, loader.sourceUpdater_).then(() => { + const playlist = playlistWithDuration(40); + + loader.playlist(playlist); + loader.load(); + this.clock.tick(1); + + loader.sourceUpdater_.removeAudio = (start, end) => { + audioRemoves++; + }; + loader.sourceUpdater_.removeVideo = (start, end) => { + videoRemoves++; + }; + + // segment is requested but not yet downloaded, therefore there's no starting + // media info + // + // callback won't be called + loader.remove(0, 100, () => {}); + assert.equal(audioRemoves, 0, 'no audio removes'); + assert.equal(videoRemoves, 0, 'no video removes'); + + standardXHRResponse(this.requests.shift(), muxedSegment()); + return new Promise((resolve, reject) => { + loader.one('appended', resolve); + loader.one('error', reject); + }); + }).then(() => { + loader.remove(0, 100, () => {}); + assert.equal(audioRemoves, 1, 'one audio remove'); + assert.equal(videoRemoves, 1, 'one video remove'); + }); + }); + QUnit.test('triggers appenderror when append errors', function(assert) { return setupMediaSource(loader.mediaSource_, loader.sourceUpdater_).then(() => { @@ -4176,8 +4214,7 @@ QUnit.module('SegmentLoader: FMP4', function(hooks) { endTime: 2, text: 'test' }); - // set currentMediaInfo_ - loader.currentMediaInfo_ = {hasVideo: true, hasAudio: true}; + loader.startingMediaInfo_ = {hasVideo: true, hasAudio: true}; loader.remove(0, 2); assert.equal(this.inbandTextTracks.CC1.cues.length, 0, 'all cues have been removed');