From ce03f664e61da7549a49c09288ddda93fff6310e Mon Sep 17 00:00:00 2001 From: Brandon Casey <2381475+brandonocasey@users.noreply.github.com> Date: Wed, 26 May 2021 16:59:53 -0400 Subject: [PATCH] fix: Append valid syncRequests, better sync request choice, less getMediaInfoForTime rounding (#1127) --- src/playlist.js | 12 +-- src/segment-loader.js | 90 +++++++++++++++------- test/loader-common.js | 6 +- test/playlist-loader.test.js | 10 +++ test/segment-loader.test.js | 140 ++++++++++++++++++++++++----------- 5 files changed, 178 insertions(+), 80 deletions(-) diff --git a/src/playlist.js b/src/playlist.js index 5076e0ad7..51fefb481 100644 --- a/src/playlist.js +++ b/src/playlist.js @@ -5,8 +5,8 @@ */ import videojs from 'video.js'; import window from 'global/window'; -import {TIME_FUDGE_FACTOR} from './ranges.js'; import {isAudioCodec} from '@videojs/vhs-utils/es/codecs.js'; +import {TIME_FUDGE_FACTOR} from './ranges.js'; const {createTimeRange} = videojs; @@ -391,9 +391,10 @@ export const getMediaInfoForTime = function( for (let i = startIndex - 1; i >= 0; i--) { const segment = partsAndSegments[i]; - time += (segment.duration + TIME_FUDGE_FACTOR); + time += segment.duration; - if (time > 0) { + // TODO: consider not using TIME_FUDGE_FACTOR at all here + if ((time + TIME_FUDGE_FACTOR) > 0) { return { mediaIndex: segment.segmentIndex, startTime: startTime - sumDurations(playlist, startIndex, segment.segmentIndex), @@ -433,9 +434,10 @@ export const getMediaInfoForTime = function( for (let i = startIndex; i < partsAndSegments.length; i++) { const partSegment = partsAndSegments[i]; - time -= partSegment.duration + TIME_FUDGE_FACTOR; + time -= partSegment.duration; - if (time < 0) { + // TODO: consider not using TIME_FUDGE_FACTOR at all here + if ((time - TIME_FUDGE_FACTOR) < 0) { return { mediaIndex: partSegment.segmentIndex, startTime: startTime + sumDurations(playlist, startIndex, partSegment.segmentIndex), diff --git a/src/segment-loader.js b/src/segment-loader.js index a3ccf8d50..f24aa2f56 100644 --- a/src/segment-loader.js +++ b/src/segment-loader.js @@ -32,30 +32,33 @@ import {getKnownPartCount} from './playlist.js'; * generate a syncPoint. This function returns a good candidate index * for that process. * - * @param {Object} playlist - the playlist object to look for a + * @param {Array} segments - the segments array from a playlist. * @return {number} An index of a segment from the playlist to load */ -export const getSyncSegmentCandidate = function(currentTimeline, {segments = []} = {}) { - // if we don't currently have a real timeline yet. - if (currentTimeline === -1) { - return 0; - } +export const getSyncSegmentCandidate = function(currentTimeline, segments, targetTime) { + segments = segments || []; + const timelineSegments = []; + let time = 0; + + for (let i = 0; i < segments.length; i++) { + const segment = segments[i]; + + if (currentTimeline === segment.timeline) { + timelineSegments.push(i); + time += segment.duration; - const segmentIndexArray = segments.reduce((acc, s, i) => { - if (s.timeline === currentTimeline) { - acc.push(i); + if (time > targetTime) { + return i; + } } - return acc; - }, []); + } - if (segmentIndexArray.length) { - // TODO: why do we do this? Basically we choose index 0 if - // segmentIndexArray.length is 1 and index = 1 if segmentIndexArray.length - // is greater then 1 - return segmentIndexArray[Math.min(segmentIndexArray.length - 1, 1)]; + if (timelineSegments.length === 0) { + return 0; } - return Math.max(segments.length - 1, 0); + // default to the last timeline segment + return timelineSegments[timelineSegments.length - 1]; }; // In the event of a quota exceeded error, keep at least one second of back buffer. This @@ -137,6 +140,7 @@ const segmentInfoString = (segmentInfo) => { startOfSegment, duration, segment, + part, playlist: { mediaSequence: seq, id, @@ -155,15 +159,15 @@ const segmentInfoString = (segmentInfo) => { } else if (segmentInfo.isSyncRequest) { selection = 'getSyncSegmentCandidate (isSyncRequest)'; } - const {start, end} = segment; + const hasPartIndex = typeof partIndex === 'number'; const name = segmentInfo.segment.uri ? 'segment' : 'pre-segment'; - const totalParts = hasPartIndex ? getKnownPartCount({preloadSegment: segment}) - 1 : 0; + const zeroBasedPartCount = hasPartIndex ? getKnownPartCount({preloadSegment: segment}) - 1 : 0; - return `${name} [${index}/${segmentLen}]` + - (hasPartIndex ? ` part [${partIndex}/${totalParts}]` : '') + - ` mediaSequenceNumber [${seq}/${seq + segmentLen}]` + - ` start/end [${start} => ${end}]` + + return `${name} [${seq + index}/${seq + segmentLen}]` + + (hasPartIndex ? ` part [${partIndex}/${zeroBasedPartCount}]` : '') + + ` segment start/end [${segment.start} => ${segment.end}]` + + (hasPartIndex ? ` part start/end [${part.start} => ${part.end}]` : '') + ` startOfSegment [${startOfSegment}]` + ` duration [${duration}]` + ` timeline [${timeline}]` + @@ -1381,7 +1385,7 @@ export default class SegmentLoader extends videojs.EventTarget { }; if (next.isSyncRequest) { - next.mediaIndex = getSyncSegmentCandidate(this.currentTimeline_, this.playlist_); + next.mediaIndex = getSyncSegmentCandidate(this.currentTimeline_, segments, bufferedEnd); } else if (this.mediaIndex !== null) { const segment = segments[this.mediaIndex]; const partIndex = typeof this.partIndex === 'number' ? this.partIndex : -1; @@ -2030,6 +2034,31 @@ export default class SegmentLoader extends videojs.EventTarget { // as we use the start of the segment to offset the best guess (playlist provided) // timestamp offset. this.updateSourceBufferTimestampOffset_(segmentInfo); + + // if this is a sync request we need to determine whether it should + // be appended or not. + if (segmentInfo.isSyncRequest) { + // first save/update our timing info for this segment. + // this is what allows us to choose an accurate segment + // and the main reason we make a sync request. + this.updateTimingInfoEnd_(segmentInfo); + this.syncController_.saveSegmentTimingInfo({ + segmentInfo, + shouldSaveTimelineMapping: this.loaderType_ === 'main' + }); + + const next = this.chooseNextRequest_(); + + // If the sync request isn't the segment that would be requested next + // after taking into account its timing info, do not append it. + if (next.mediaIndex !== segmentInfo.mediaIndex || next.partIndex !== segmentInfo.partIndex) { + this.logger_('sync segment was incorrect, not appending'); + return; + } + // otherwise append it like any other segment as our guess was correct. + this.logger_('sync segment was correct, appending'); + } + // Save some state so that in the future anything waiting on first append (and/or // timestamp offset(s)) can process immediately. While the extra state isn't optimal, // we need some notion of whether the timestamp offset or other relevant information @@ -2884,8 +2913,6 @@ export default class SegmentLoader extends videojs.EventTarget { }); } - this.logger_(`Appended ${segmentInfoString(segmentInfo)}`); - const segmentDurationMessage = getTroublesomeSegmentDurationMessage(segmentInfo, this.sourceType_); @@ -2903,9 +2930,18 @@ export default class SegmentLoader extends videojs.EventTarget { if (segmentInfo.isSyncRequest) { this.trigger('syncinfoupdate'); - return; + // if the sync request was not appended + // then it was not the correct segment. + // throw it away and use the data it gave us + // to get the correct one. + if (!segmentInfo.hasAppendedData_) { + this.logger_(`Throwing away un-appended sync request ${segmentInfoString(segmentInfo)}`); + return; + } } + this.logger_(`Appended ${segmentInfoString(segmentInfo)}`); + this.addSegmentMetadataCue_(segmentInfo); this.fetchAtBuffer_ = true; if (this.currentTimeline_ !== segmentInfo.timeline) { diff --git a/test/loader-common.js b/test/loader-common.js index 98328772b..b31f2ffa6 100644 --- a/test/loader-common.js +++ b/test/loader-common.js @@ -870,7 +870,7 @@ export const LoaderCommonFactory = ({ }); QUnit.test('drops partIndex if playlist update drops parts', function(assert) { - assert.timeout(100000000000000000000); + loader.duration_ = () => Infinity; return setupMediaSource(loader.mediaSource_, loader.sourceUpdater_).then(() => { loader.playlist(playlistWithDuration(50, { mediaSequence: 0, @@ -899,8 +899,8 @@ export const LoaderCommonFactory = ({ assert.equal( this.requests[0].url, - '1.ts', - 'requested mediaIndex 1 only' + '0.ts', + 'requested mediaIndex 0 only' ); }); }); diff --git a/test/playlist-loader.test.js b/test/playlist-loader.test.js index 9596c109b..1c69a2f11 100644 --- a/test/playlist-loader.test.js +++ b/test/playlist-loader.test.js @@ -2143,6 +2143,7 @@ QUnit.module('Playlist Loader', function(hooks) { this.requests.shift().respond( 200, null, '#EXTM3U\n' + + '#EXT-X-PART-INF:PART-TARGET=1\n' + '#EXT-X-MEDIA-SEQUENCE:0\n' + '#EXTINF:2\n' + 'low-1.ts\n' + @@ -2163,6 +2164,7 @@ QUnit.module('Playlist Loader', function(hooks) { this.requests.shift().respond( 200, null, '#EXTM3U\n' + + '#EXT-X-PART-INF:PART-TARGET=1\n' + '#EXT-X-MEDIA-SEQUENCE:0\n' + '#EXTINF:2\n' + 'low-1.ts\n' + @@ -2182,6 +2184,7 @@ QUnit.module('Playlist Loader', function(hooks) { this.requests.shift().respond( 200, null, '#EXTM3U\n' + + '#EXT-X-PART-INF:PART-TARGET=1\n' + '#EXT-X-MEDIA-SEQUENCE:0\n' + '#EXTINF:2\n' + 'segment0.ts\n' + @@ -2208,6 +2211,7 @@ QUnit.module('Playlist Loader', function(hooks) { this.requests.shift().respond( 200, null, '#EXTM3U\n' + + '#EXT-X-PART-INF:PART-TARGET=1\n' + '#EXT-X-MEDIA-SEQUENCE:0\n' + '#EXT-X-SERVER-CONTROL:CAN-SKIP-UNTIL=3\n' + '#EXTINF:2\n' + @@ -2242,6 +2246,7 @@ QUnit.module('Playlist Loader', function(hooks) { this.requests.shift().respond( 200, null, '#EXTM3U\n' + + '#EXT-X-PART-INF:PART-TARGET=1\n' + '#EXT-X-MEDIA-SEQUENCE:0\n' + '#EXT-X-SERVER-CONTROL:CAN-SKIP-UNTIL=3,CAN-SKIP-DATERANGES=YES\n' + '#EXTINF:2\n' + @@ -2276,6 +2281,7 @@ QUnit.module('Playlist Loader', function(hooks) { this.requests.shift().respond( 200, null, '#EXTM3U\n' + + '#EXT-X-PART-INF:PART-TARGET=1\n' + '#EXT-X-MEDIA-SEQUENCE:0\n' + '#EXT-X-SERVER-CONTROL:CAN-BLOCK-RELOAD=YES\n' + '#EXTINF:2\n' + @@ -2309,6 +2315,7 @@ QUnit.module('Playlist Loader', function(hooks) { this.requests.shift().respond( 200, null, '#EXTM3U\n' + + '#EXT-X-PART-INF:PART-TARGET=1\n' + '#EXT-X-MEDIA-SEQUENCE:0\n' + '#EXT-X-SERVER-CONTROL:CAN-BLOCK-RELOAD=YES\n' + '#EXTINF:2\n' + @@ -2341,6 +2348,7 @@ QUnit.module('Playlist Loader', function(hooks) { this.requests.shift().respond( 200, null, '#EXTM3U\n' + + '#EXT-X-PART-INF:PART-TARGET=1\n' + '#EXT-X-MEDIA-SEQUENCE:0\n' + '#EXT-X-SERVER-CONTROL:CAN-BLOCK-RELOAD=YES\n' + '#EXTINF:2\n' + @@ -2373,6 +2381,7 @@ QUnit.module('Playlist Loader', function(hooks) { this.requests.shift().respond( 200, null, '#EXTM3U\n' + + '#EXT-X-PART-INF:PART-TARGET=1\n' + '#EXT-X-MEDIA-SEQUENCE:0\n' + '#EXT-X-SERVER-CONTROL:CAN-BLOCK-RELOAD=YES\n' + '#EXTINF:2\n' + @@ -2407,6 +2416,7 @@ QUnit.module('Playlist Loader', function(hooks) { this.requests.shift().respond( 200, null, '#EXTM3U\n' + + '#EXT-X-PART-INF:PART-TARGET=1\n' + '#EXT-X-MEDIA-SEQUENCE:0\n' + '#EXT-X-SERVER-CONTROL:CAN-BLOCK-RELOAD=YES,CAN-SKIP-UNTIL=3\n' + '#EXTINF:2\n' + diff --git a/test/segment-loader.test.js b/test/segment-loader.test.js index a39851406..95bb517ab 100644 --- a/test/segment-loader.test.js +++ b/test/segment-loader.test.js @@ -79,51 +79,38 @@ SegmentLoader.prototype.addSegmentMetadataCue_ = function() {}; QUnit.module('SegmentLoader Isolated Functions'); QUnit.test('getSyncSegmentCandidate works as expected', function(assert) { - assert.equal(getSyncSegmentCandidate(-1, {}), 0, '-1 timeline, try index 0'); - assert.equal(getSyncSegmentCandidate(0, {}), 0, '0 timeline, no segments, try index 0'); - - assert.equal( - getSyncSegmentCandidate(0, {segments: [ - {timeline: 0}, - {timeline: 0} - ]}), - 1, - '0 timeline, two timeline 0 segments, try index 1' - ); - - assert.equal( - getSyncSegmentCandidate(0, {segments: [ - {timeline: 0}, - {timeline: 0}, - {timeline: 0}, - {timeline: 0} - ]}), - 1, - '0 timeline, four timeline 0 segments, try index 1' - ); - - assert.equal( - getSyncSegmentCandidate(0, {segments: [ - {timeline: 0}, - {timeline: 1}, - {timeline: 1}, - {timeline: 1} - ]}), - 0, - '0 timeline, one timeline 0 segment, three timeline 1 segments, try index 0' - ); - - assert.equal( - getSyncSegmentCandidate(0, {segments: [ - {timeline: 1}, - {timeline: 1}, - {timeline: 1}, - {timeline: 1} - ]}), - 3, - '0 timeline, four timeline 1 segments, try index 3' - ); - + let segments = []; + + assert.equal(getSyncSegmentCandidate(-1, segments, 0), 0, '-1 timeline, no segments, 0 target'); + assert.equal(getSyncSegmentCandidate(0, segments, 0), 0, '0 timeline, no segments, 0 target'); + + segments = [ + {timeline: 0, duration: 4}, + {timeline: 0, duration: 4}, + {timeline: 0, duration: 4}, + {timeline: 0, duration: 4} + ]; + + assert.equal(getSyncSegmentCandidate(-1, segments, 0), 0, '-1 timeline, 4x 0 segments, 0 target'); + assert.equal(getSyncSegmentCandidate(0, segments, 1), 0, '0 timeline, 4x 0 segments, 1 target'); + assert.equal(getSyncSegmentCandidate(0, segments, 4), 1, '0 timeline, 4x 0 segments, 4 target'); + assert.equal(getSyncSegmentCandidate(-1, segments, 8), 0, '-1 timeline, 4x 0 segments, 8 target'); + assert.equal(getSyncSegmentCandidate(0, segments, 8), 2, '0 timeline, 4x 0 segments, 8 target'); + assert.equal(getSyncSegmentCandidate(0, segments, 20), 3, '0 timeline, 4x 0 segments, 20 target'); + + segments = [ + {timeline: 1, duration: 4}, + {timeline: 0, duration: 4}, + {timeline: 1, duration: 4}, + {timeline: 0, duration: 4}, + {timeline: 2, duration: 4}, + {timeline: 1, duration: 4}, + {timeline: 0, duration: 4} + ]; + + assert.equal(getSyncSegmentCandidate(1, segments, 8), 5, '1 timeline, mixed timeline segments, 8 target'); + assert.equal(getSyncSegmentCandidate(0, segments, 8), 6, '0 timeline, mixed timeline segments, 8 target'); + assert.equal(getSyncSegmentCandidate(2, segments, 8), 4, '2 timeline, mixed timeline segments, 8 target'); }); QUnit.test('illegalMediaSwitch detects illegal media switches', function(assert) { @@ -2967,6 +2954,69 @@ QUnit.module('SegmentLoader', function(hooks) { }); }); + QUnit.test('sync request can be thrown away', function(assert) { + const appends = []; + const logs = []; + + return setupMediaSource(loader.mediaSource_, loader.sourceUpdater_, {isVideoOnly: true}).then(() => { + const origAppendToSourceBuffer = loader.appendToSourceBuffer_.bind(loader); + + // set the mediaSource duration as it is usually set by + // master playlist controller, which is not present here + loader.mediaSource_.duration = Infinity; + + loader.appendToSourceBuffer_ = (config) => { + appends.push(config); + origAppendToSourceBuffer(config); + }; + + loader.playlist(playlistWithDuration(20)); + loader.load(); + this.clock.tick(1); + standardXHRResponse(this.requests.shift(), videoSegment()); + + return new Promise((resolve, reject) => { + loader.one('appended', resolve); + loader.one('error', reject); + }); + }).then(() => { + this.clock.tick(1); + + assert.equal(appends.length, 1, 'one append'); + assert.equal(appends[0].type, 'video', 'appended to video buffer'); + assert.ok(appends[0].initSegment, 'appended video init segment'); + + loader.playlist(playlistWithDuration(20, { uri: 'new-playlist.m3u8' })); + // remove old aborted request + this.requests.shift(); + // get the new request + this.clock.tick(1); + loader.chooseNextRequest_ = () => ({partIndex: null, mediaIndex: 1}); + loader.logger_ = (line) => { + logs.push(line); + }; + standardXHRResponse(this.requests.shift(), videoSegment()); + + // since it's a sync request, wait for the syncinfoupdate event (we won't get the + // appended event) + return new Promise((resolve, reject) => { + loader.one('syncinfoupdate', resolve); + loader.one('error', reject); + }); + }).then(() => { + this.clock.tick(1); + assert.equal(appends.length, 1, 'still only one append'); + assert.true( + logs.some((l) => (/^sync segment was incorrect, not appending/).test(l)), + 'has log line' + ); + assert.true( + logs.some((l) => (/^Throwing away un-appended sync request segment/).test(l)), + 'has log line' + ); + }); + }); + QUnit.test('re-appends init segments on discontinuity', function(assert) { const appends = [];