From c47c6fdfa33164c51efbcfedae4d426e4217aafe Mon Sep 17 00:00:00 2001 From: Adam Waldron Date: Mon, 15 Jul 2024 11:05:27 -0700 Subject: [PATCH 1/8] fix: bad timeline changes --- src/segment-loader.js | 32 ++++++++++++++++ test/segment-loader.test.js | 76 ++++++++++++++++++++++++++++++++++++- 2 files changed, 107 insertions(+), 1 deletion(-) diff --git a/src/segment-loader.js b/src/segment-loader.js index fe1b201ca..b2667df46 100644 --- a/src/segment-loader.js +++ b/src/segment-loader.js @@ -406,6 +406,31 @@ export const shouldWaitForTimelineChange = ({ return false; }; +export const shouldFixBadTimelineChanges = (timelineChangeController) => { + if (!timelineChangeController) { + return false; + } + const pendingAudioTimelineChange = timelineChangeController.pendingTimelineChange({ type: 'audio' }); + const pendingMainTimelineChange = timelineChangeController.pendingTimelineChange({ type: 'main' }); + const hasPendingTimelineChanges = pendingAudioTimelineChange && pendingMainTimelineChange; + const differentPendingChanges = hasPendingTimelineChanges && pendingAudioTimelineChange.to !== pendingMainTimelineChange.to; + + if (differentPendingChanges) { + return true; + } + + return false; +}; + +export const fixBadTimelineChange = (segmentLoader) => { + if (!segmentLoader) { + return; + } + segmentLoader.pause(); + segmentLoader.resetEverything(); + segmentLoader.load(); +}; + export const mediaDuration = (timingInfos) => { let maxDuration = 0; @@ -2130,6 +2155,9 @@ Fetch At Buffer: ${this.fetchAtBuffer_} audioDisabled: this.audioDisabled_ }) ) { + if (shouldFixBadTimelineChanges(this.timelineChangeController_)) { + fixBadTimelineChange(this); + } return false; } @@ -2180,6 +2208,7 @@ Fetch At Buffer: ${this.fetchAtBuffer_} return false; } + // we need to allow an append here even if we're moving to different timelines. if ( shouldWaitForTimelineChange({ timelineChangeController: this.timelineChangeController_, @@ -2189,6 +2218,9 @@ Fetch At Buffer: ${this.fetchAtBuffer_} audioDisabled: this.audioDisabled_ }) ) { + if (shouldFixBadTimelineChanges(this.timelineChangeController_)) { + fixBadTimelineChange(this); + } return false; } diff --git a/test/segment-loader.test.js b/test/segment-loader.test.js index 2dba67148..480be3064 100644 --- a/test/segment-loader.test.js +++ b/test/segment-loader.test.js @@ -9,7 +9,9 @@ import { mediaDuration, getTroublesomeSegmentDurationMessage, getSyncSegmentCandidate, - segmentInfoString + segmentInfoString, + shouldFixBadTimelineChanges, + fixBadTimelineChange } from '../src/segment-loader'; import mp4probe from 'mux.js/lib/mp4/probe'; import { @@ -465,6 +467,78 @@ QUnit.test('main loader does not wait if pending audio timeline change matches s ); }); +QUnit.module('shouldFixBadTimelineChange'); + +QUnit.test('shouldFixBadTimelineChange returns true when timelines are both changing to different timelines', function(assert) { + const timelineChangeController = { + pendingTimelineChange({ type }) { + if (type === 'audio') { + return { from: 1, to: 2 }; + } else if (type === 'main') { + return { from: 2, to: 1 }; + } + } + }; + + assert.ok(shouldFixBadTimelineChanges(timelineChangeController), 'should fix a bad timeline change'); +}); + +QUnit.test('shouldFixBadTimelineChange returns false when only one timeline has a pending change', function(assert) { + const timelineChangeController = { + pendingTimelineChange({ type }) { + if (type === 'audio') { + return { from: 1, to: 2 }; + } + } + }; + + assert.notOk(shouldFixBadTimelineChanges(timelineChangeController), 'should not fix a timeline change'); +}); + +QUnit.test('shouldFixBadTimelineChange returns false when both timelines are changing to the same value', function(assert) { + const timelineChangeController = { + pendingTimelineChange({ type }) { + if (type === 'audio') { + return { from: 1, to: 2 }; + } else if (type === 'main') { + return { from: 1, to: 2 }; + } + } + }; + + assert.notOk(shouldFixBadTimelineChanges(timelineChangeController), 'should not fix a good timeline change'); +}); + +QUnit.test('shouldFixBadTimelineChange returns false when timelineChangeController is undefined', function(assert) { + const timelineChangeController = undefined; + + assert.notOk(shouldFixBadTimelineChanges(timelineChangeController), 'should not fix a timeline change with no timelineChangeController'); +}); + +QUnit.module('fixBadTimelineChange'); + +QUnit.test('fixBadTimelineChange calls pause, resetEverything and load on a segmentLoader', function(assert) { + let pauseCalls = 0; + let resetEverythingCalls = 0; + let loadCalls = 0; + const mockSegmentLoader = { + pause() { + pauseCalls++; + }, + resetEverything() { + resetEverythingCalls++; + }, + load() { + loadCalls++; + } + }; + + fixBadTimelineChange(mockSegmentLoader); + assert.equal(pauseCalls, 1, 'calls pause once'); + assert.equal(resetEverythingCalls, 1, 'calls resetEverything once'); + assert.equal(loadCalls, 1, 'calls load once'); +}); + QUnit.module('safeBackBufferTrimTime'); QUnit.test('uses 30s before playhead when seekable start is 0', function(assert) { From 5bec70b656c0eeff45685e75776a241e7f34e958 Mon Sep 17 00:00:00 2001 From: Adam Waldron Date: Mon, 15 Jul 2024 12:23:55 -0700 Subject: [PATCH 2/8] do not fix bad change on initial timeline change. --- src/segment-loader.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/segment-loader.js b/src/segment-loader.js index b2667df46..c37e180a0 100644 --- a/src/segment-loader.js +++ b/src/segment-loader.js @@ -414,8 +414,9 @@ export const shouldFixBadTimelineChanges = (timelineChangeController) => { const pendingMainTimelineChange = timelineChangeController.pendingTimelineChange({ type: 'main' }); const hasPendingTimelineChanges = pendingAudioTimelineChange && pendingMainTimelineChange; const differentPendingChanges = hasPendingTimelineChanges && pendingAudioTimelineChange.to !== pendingMainTimelineChange.to; + const isNotInitialPendingTimelineChange = hasPendingTimelineChanges && pendingAudioTimelineChange.from !== -1 && pendingMainTimelineChange.from !== -1; - if (differentPendingChanges) { + if (isNotInitialPendingTimelineChange && differentPendingChanges) { return true; } From e4f42e0e0f7e09fd7d034c7c0ef7c19c4383211b Mon Sep 17 00:00:00 2001 From: Adam Waldron Date: Mon, 15 Jul 2024 22:19:02 -0700 Subject: [PATCH 3/8] fix: test coverage --- test/segment-loader.test.js | 126 ++++++++++++++++++++++++++++++++++++ 1 file changed, 126 insertions(+) diff --git a/test/segment-loader.test.js b/test/segment-loader.test.js index 480be3064..d3a06b3e9 100644 --- a/test/segment-loader.test.js +++ b/test/segment-loader.test.js @@ -1656,6 +1656,132 @@ QUnit.module('SegmentLoader', function(hooks) { }); }); + QUnit.test('hasEnoughInfoToLoad_ calls fixBadTimelineChange', function(assert) { + loader.dispose(); + loader = new SegmentLoader(LoaderCommonSettings.call(this, { + loaderType: 'audio' + }), {}); + const origPause = loader.pause; + const origLoad = loader.load; + const origResetEverything = loader.resetEverything; + let pauseCalls = 0; + let loadCalls = 0; + let resetEverythingCalls = 0; + + loader.pause = () => { + pauseCalls++; + origPause.call(loader); + }; + loader.load = () => { + loadCalls++; + origLoad.call(loader); + }; + loader.resetEverything = () => { + resetEverythingCalls++; + origResetEverything.call(loader); + }; + loader.timelineChangeController_.pendingTimelineChange = ({ type }) => { + if (type === 'audio') { + return { + from: 3, + to: 2 + }; + } else if (type === 'main') { + return { + from: 0, + to: 1 + }; + } + }; + + const playlist = playlistWithDuration(20); + + playlist.discontinuityStarts = [1]; + loader.getCurrentMediaInfo_ = () => { + return { + hasVideo: true, + hasAudio: false, + isMuxed: false + }; + }; + + return this.setupMediaSource(loader.mediaSource_, loader.sourceUpdater_).then(() => { + loader.playlist(playlist); + loader.load(); + this.clock.tick(1); + assert.equal(pauseCalls, 1, '1 pause call expected'); + assert.equal(loadCalls, 2, '2 load calls expected'); + assert.equal(resetEverythingCalls, 2, '1 load calls expected'); + }); + }); + + QUnit.test('hasEnoughInfoToAppend_ calls fixBadTimelineChange', function(assert) { + loader.dispose(); + loader = new SegmentLoader(LoaderCommonSettings.call(this, { + loaderType: 'main' + }), {}); + const origPause = loader.pause; + const origLoad = loader.load; + const origResetEverything = loader.resetEverything; + let pauseCalls = 0; + let loadCalls = 0; + let resetEverythingCalls = 0; + + loader.pause = () => { + pauseCalls++; + origPause.call(loader); + }; + loader.load = () => { + loadCalls++; + origLoad.call(loader); + }; + loader.resetEverything = () => { + resetEverythingCalls++; + origResetEverything.call(loader); + }; + loader.timelineChangeController_.pendingTimelineChange = ({ type }) => { + if (type === 'audio') { + return { + from: 3, + to: 2 + }; + } else if (type === 'main') { + return { + from: 0, + to: 1 + }; + } + }; + this.sourceUpdater_.ready = () => { + return true; + }; + + const playlist = playlistWithDuration(20); + + playlist.discontinuityStarts = [1]; + loader.getCurrentMediaInfo_ = () => { + return { + hasVideo: true, + hasAudio: false, + isMuxed: false + }; + }; + loader.pendingSegment_ = { + foo: 'bar', + videoTimingInfo: 1 + }; + loader.audioDisabled_ = true; + + return this.setupMediaSource(loader.mediaSource_, loader.sourceUpdater_).then(() => { + loader.playlist(playlist); + loader.load(); + this.clock.tick(1); + assert.equal(pauseCalls, 1, '1 pause call expected'); + assert.equal(loadCalls, 2, '2 load calls expected'); + assert.equal(resetEverythingCalls, 2, '1 load calls expected'); + }); + }); + QUnit.test('audio loader does not wait to request segment even if timestamp offset is nonzero', function(assert) { loader.dispose(); loader = new SegmentLoader(LoaderCommonSettings.call(this, { From 17e04ba4a444c4a3786e57245d2259cfb57fd45f Mon Sep 17 00:00:00 2001 From: Adam Waldron Date: Mon, 15 Jul 2024 22:30:21 -0700 Subject: [PATCH 4/8] chore: additional coverage --- test/segment-loader.test.js | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/test/segment-loader.test.js b/test/segment-loader.test.js index d3a06b3e9..3b5063935 100644 --- a/test/segment-loader.test.js +++ b/test/segment-loader.test.js @@ -521,7 +521,7 @@ QUnit.test('fixBadTimelineChange calls pause, resetEverything and load on a segm let pauseCalls = 0; let resetEverythingCalls = 0; let loadCalls = 0; - const mockSegmentLoader = { + let mockSegmentLoader = { pause() { pauseCalls++; }, @@ -537,6 +537,13 @@ QUnit.test('fixBadTimelineChange calls pause, resetEverything and load on a segm assert.equal(pauseCalls, 1, 'calls pause once'); assert.equal(resetEverythingCalls, 1, 'calls resetEverything once'); assert.equal(loadCalls, 1, 'calls load once'); + + // early return if undefined. call counts remain the same. + mockSegmentLoader = undefined; + fixBadTimelineChange(mockSegmentLoader); + assert.equal(pauseCalls, 1, 'calls pause once'); + assert.equal(resetEverythingCalls, 1, 'calls resetEverything once'); + assert.equal(loadCalls, 1, 'calls load once'); }); QUnit.module('safeBackBufferTrimTime'); From 436aea5df732abed2cdbaf1290f1c2cfb3b7da90 Mon Sep 17 00:00:00 2001 From: Adam Waldron Date: Tue, 16 Jul 2024 21:46:03 -0700 Subject: [PATCH 5/8] fix: add abort_ call --- src/segment-loader.js | 1 + test/segment-loader.test.js | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/src/segment-loader.js b/src/segment-loader.js index c37e180a0..b4b8b0baa 100644 --- a/src/segment-loader.js +++ b/src/segment-loader.js @@ -428,6 +428,7 @@ export const fixBadTimelineChange = (segmentLoader) => { return; } segmentLoader.pause(); + segmentLoader.abort_(); segmentLoader.resetEverything(); segmentLoader.load(); }; diff --git a/test/segment-loader.test.js b/test/segment-loader.test.js index 3b5063935..10e136ac3 100644 --- a/test/segment-loader.test.js +++ b/test/segment-loader.test.js @@ -521,10 +521,14 @@ QUnit.test('fixBadTimelineChange calls pause, resetEverything and load on a segm let pauseCalls = 0; let resetEverythingCalls = 0; let loadCalls = 0; + let abortCalls = 0; let mockSegmentLoader = { pause() { pauseCalls++; }, + abort_() { + abortCalls++; + }, resetEverything() { resetEverythingCalls++; }, @@ -544,6 +548,7 @@ QUnit.test('fixBadTimelineChange calls pause, resetEverything and load on a segm assert.equal(pauseCalls, 1, 'calls pause once'); assert.equal(resetEverythingCalls, 1, 'calls resetEverything once'); assert.equal(loadCalls, 1, 'calls load once'); + assert.equal(abortCalls, 1, 'calls abort once'); }); QUnit.module('safeBackBufferTrimTime'); From b5637fa4d133008be6ba52fb81aa55dbbfc2788e Mon Sep 17 00:00:00 2001 From: Adam Waldron Date: Tue, 16 Jul 2024 22:20:45 -0700 Subject: [PATCH 6/8] fix: allow append after timeline change fix --- src/segment-loader.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/segment-loader.js b/src/segment-loader.js index b4b8b0baa..e3e1f8ca4 100644 --- a/src/segment-loader.js +++ b/src/segment-loader.js @@ -2222,6 +2222,9 @@ Fetch At Buffer: ${this.fetchAtBuffer_} ) { if (shouldFixBadTimelineChanges(this.timelineChangeController_)) { fixBadTimelineChange(this); + // we want to append segments whenever we can, so + // try and fix the bad change then append the segment anyway. + return true; } return false; } From a11774da303abba53471bee6b395aff00f87eb2f Mon Sep 17 00:00:00 2001 From: Adam Waldron Date: Wed, 17 Jul 2024 14:11:27 -0700 Subject: [PATCH 7/8] fix: don't return true --- src/segment-loader.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/segment-loader.js b/src/segment-loader.js index e3e1f8ca4..39310e861 100644 --- a/src/segment-loader.js +++ b/src/segment-loader.js @@ -431,6 +431,7 @@ export const fixBadTimelineChange = (segmentLoader) => { segmentLoader.abort_(); segmentLoader.resetEverything(); segmentLoader.load(); + segmentLoader.trigger('fix-timeline-change'); }; export const mediaDuration = (timingInfos) => { @@ -2222,9 +2223,6 @@ Fetch At Buffer: ${this.fetchAtBuffer_} ) { if (shouldFixBadTimelineChanges(this.timelineChangeController_)) { fixBadTimelineChange(this); - // we want to append segments whenever we can, so - // try and fix the bad change then append the segment anyway. - return true; } return false; } From 48551a633c5efbad0edc8c6250ac44acfc618fd4 Mon Sep 17 00:00:00 2001 From: Adam Waldron Date: Thu, 18 Jul 2024 08:58:46 -0700 Subject: [PATCH 8/8] remove debug event --- src/segment-loader.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/segment-loader.js b/src/segment-loader.js index 39310e861..b4b8b0baa 100644 --- a/src/segment-loader.js +++ b/src/segment-loader.js @@ -431,7 +431,6 @@ export const fixBadTimelineChange = (segmentLoader) => { segmentLoader.abort_(); segmentLoader.resetEverything(); segmentLoader.load(); - segmentLoader.trigger('fix-timeline-change'); }; export const mediaDuration = (timingInfos) => {