From a4a45df3d0ae3c587dd73f9915c7ae5806c186ab Mon Sep 17 00:00:00 2001 From: brandonocasey Date: Wed, 5 May 2021 11:53:37 -0400 Subject: [PATCH 01/12] fix: more accurate segment choices and logging --- src/playlist.js | 10 +-- src/segment-loader.js | 93 ++++++++++++++++-------- test/loader-common.js | 6 +- test/segment-loader.test.js | 136 ++++++++++++++++++++++++------------ 4 files changed, 161 insertions(+), 84 deletions(-) diff --git a/src/playlist.js b/src/playlist.js index dd4eb0784..550bd7ff2 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; @@ -374,9 +374,9 @@ 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) { + if ((time + TIME_FUDGE_FACTOR) > 0) { return { mediaIndex: segment.segmentIndex, startTime: startTime - sumDurations(playlist, startIndex, segment.segmentIndex), @@ -416,9 +416,9 @@ 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) { + 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 ed1c75ca1..07a276023 100644 --- a/src/segment-loader.js +++ b/src/segment-loader.js @@ -34,27 +34,27 @@ import {lastBufferedEnd} from './ranges.js'; * @param {Object} playlist - the playlist object to look for a * @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, playlist, targetTime) { + const segments = playlist.segments || []; - const segmentIndexArray = segments.reduce((acc, s, i) => { - if (s.timeline === currentTimeline) { - acc.push(i); - } - return acc; - }, []); + 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; - 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 (time > targetTime) { + return i; + } + } } - return Math.max(segments.length - 1, 0); + // default to grabbing the last timeline segment or zero. + return Math.max(0, timelineSegments[timelineSegments.length - 1] || 0); }; // In the event of a quota exceeded error, keep at least one second of back buffer. This @@ -135,11 +135,8 @@ const segmentInfoString = (segmentInfo) => { const { startOfSegment, duration, - segment: { - start, - end, - parts - }, + segment, + part, playlist: { mediaSequence: seq, id, @@ -159,12 +156,18 @@ const segmentInfoString = (segmentInfo) => { selection = 'getSyncSegmentCandidate (isSyncRequest)'; } + const hasPartIndex = typeof partIndex === 'number'; const name = segmentInfo.segment.uri ? 'segment' : 'pre-segment'; - - return `${name} [${index}/${segmentLen}]` + - (partIndex ? ` part [${partIndex}/${parts.length - 1}]` : '') + - ` mediaSequenceNumber [${seq}/${seq + segmentLen}]` + - ` start/end [${start} => ${end}]` + + const partCount = segment.parts ? segment.parts.length : 0; + const preloadPartCount = segment.preloadHints ? + segment.preloadHints.filter((h) => h.type === 'PART').length : + 0; + const zeroBasedPartCount = partCount + preloadPartCount - 1 - (preloadPartCount > 0 ? 1 : 0); + + 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}]` + @@ -1382,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_, this.playlist_, bufferedEnd); } else if (this.mediaIndex !== null) { const segment = segments[this.mediaIndex]; const partIndex = typeof this.partIndex === 'number' ? this.partIndex : -1; @@ -2031,6 +2034,27 @@ 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); + + // throw away the isSyncRequest segment if + // it wouldn't have been the next segment we request. + if (segmentInfo.isSyncRequest) { + this.syncController_.saveSegmentTimingInfo({ + segmentInfo, + shouldSaveTimelineMapping: this.loaderType_ === 'main' + }); + + const next = this.chooseNextRequest_(); + + // if the sync request isn't the segment that would be request next + // after taking into account it's data. do not append it. + if (next.mediaIndex !== segmentInfo.mediaIndex || next.partIndex !== segmentInfo.partIndex) { + this.logger_('sync segment was incorrect, not appending'); + return; + } + 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 @@ -2873,8 +2897,6 @@ export default class SegmentLoader extends videojs.EventTarget { }); } - this.logger_(`Appended ${segmentInfoString(segmentInfo)}`); - const segmentDurationMessage = getTroublesomeSegmentDurationMessage(segmentInfo, this.sourceType_); @@ -2892,9 +2914,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/segment-loader.test.js b/test/segment-loader.test.js index 41aa036fd..4ccd22b07 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' - ); - + const playlist = {}; + + assert.equal(getSyncSegmentCandidate(-1, playlist, 0), 0, '-1 timeline, no segments, 0 target'); + assert.equal(getSyncSegmentCandidate(0, playlist, 0), 0, '0 timeline, no segments, 0 target'); + + playlist.segments = [ + {timeline: 0, duration: 4}, + {timeline: 0, duration: 4}, + {timeline: 0, duration: 4}, + {timeline: 0, duration: 4} + ]; + + assert.equal(getSyncSegmentCandidate(-1, playlist, 0), 0, '-1 timeline, 4x 0 segments, 0 target'); + assert.equal(getSyncSegmentCandidate(0, playlist, 1), 0, '0 timeline, 4x 0 segments, 4 target'); + assert.equal(getSyncSegmentCandidate(0, playlist, 4), 1, '0 timeline, 4x 0 segments, 4 target'); + assert.equal(getSyncSegmentCandidate(-1, playlist, 8), 0, '-1 timeline, 4x 0 segments, 8 target'); + assert.equal(getSyncSegmentCandidate(0, playlist, 8), 2, '0 timeline, 4x 0 segments, 8 target'); + assert.equal(getSyncSegmentCandidate(0, playlist, 20), 3, '0 timeline, 4x 0 segments, 20 target'); + + playlist.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, playlist, 8), 5, '1 timeline, mixed timeline segments, 8 target'); + assert.equal(getSyncSegmentCandidate(0, playlist, 8), 6, '0 timeline, mixed timeline segments, 8 target'); + assert.equal(getSyncSegmentCandidate(2, playlist, 8), 4, '2 timeline, mixed timeline segments, 8 target'); }); QUnit.test('illegalMediaSwitch detects illegal media switches', function(assert) { @@ -2967,6 +2954,65 @@ 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); + + 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 = []; From 127e0751f7b7d35b947a4e7d23c69d7590e0f173 Mon Sep 17 00:00:00 2001 From: brandonocasey Date: Mon, 24 May 2021 17:23:33 -0400 Subject: [PATCH 02/12] ie 11 fix --- test/segment-loader.test.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/segment-loader.test.js b/test/segment-loader.test.js index 6b26f4c6a..3c8960a5c 100644 --- a/test/segment-loader.test.js +++ b/test/segment-loader.test.js @@ -2961,6 +2961,10 @@ QUnit.module('SegmentLoader', function(hooks) { 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); From 6ddcf4c6b7328421f1b9d66fdf431c341dfe7d87 Mon Sep 17 00:00:00 2001 From: brandonocasey Date: Tue, 25 May 2021 16:37:07 -0400 Subject: [PATCH 03/12] pass segments to getSyncSegmentCandidate --- src/segment-loader.js | 9 ++++----- test/segment-loader.test.js | 28 ++++++++++++++-------------- 2 files changed, 18 insertions(+), 19 deletions(-) diff --git a/src/segment-loader.js b/src/segment-loader.js index 057ab13f7..9f4740a39 100644 --- a/src/segment-loader.js +++ b/src/segment-loader.js @@ -31,12 +31,11 @@ import {lastBufferedEnd} from './ranges.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, playlist, targetTime) { - const segments = playlist.segments || []; - +export const getSyncSegmentCandidate = function(currentTimeline, segments, targetTime) { + segments = segments || []; const timelineSegments = []; let time = 0; @@ -1385,7 +1384,7 @@ export default class SegmentLoader extends videojs.EventTarget { }; if (next.isSyncRequest) { - next.mediaIndex = getSyncSegmentCandidate(this.currentTimeline_, this.playlist_, bufferedEnd); + 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; diff --git a/test/segment-loader.test.js b/test/segment-loader.test.js index 3c8960a5c..0603491ab 100644 --- a/test/segment-loader.test.js +++ b/test/segment-loader.test.js @@ -79,26 +79,26 @@ SegmentLoader.prototype.addSegmentMetadataCue_ = function() {}; QUnit.module('SegmentLoader Isolated Functions'); QUnit.test('getSyncSegmentCandidate works as expected', function(assert) { - const playlist = {}; + let segments = []; - assert.equal(getSyncSegmentCandidate(-1, playlist, 0), 0, '-1 timeline, no segments, 0 target'); - assert.equal(getSyncSegmentCandidate(0, playlist, 0), 0, '0 timeline, no segments, 0 target'); + 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'); - playlist.segments = [ + segments = [ {timeline: 0, duration: 4}, {timeline: 0, duration: 4}, {timeline: 0, duration: 4}, {timeline: 0, duration: 4} ]; - assert.equal(getSyncSegmentCandidate(-1, playlist, 0), 0, '-1 timeline, 4x 0 segments, 0 target'); - assert.equal(getSyncSegmentCandidate(0, playlist, 1), 0, '0 timeline, 4x 0 segments, 4 target'); - assert.equal(getSyncSegmentCandidate(0, playlist, 4), 1, '0 timeline, 4x 0 segments, 4 target'); - assert.equal(getSyncSegmentCandidate(-1, playlist, 8), 0, '-1 timeline, 4x 0 segments, 8 target'); - assert.equal(getSyncSegmentCandidate(0, playlist, 8), 2, '0 timeline, 4x 0 segments, 8 target'); - assert.equal(getSyncSegmentCandidate(0, playlist, 20), 3, '0 timeline, 4x 0 segments, 20 target'); + 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, 4 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'); - playlist.segments = [ + segments = [ {timeline: 1, duration: 4}, {timeline: 0, duration: 4}, {timeline: 1, duration: 4}, @@ -108,9 +108,9 @@ QUnit.test('getSyncSegmentCandidate works as expected', function(assert) { {timeline: 0, duration: 4} ]; - assert.equal(getSyncSegmentCandidate(1, playlist, 8), 5, '1 timeline, mixed timeline segments, 8 target'); - assert.equal(getSyncSegmentCandidate(0, playlist, 8), 6, '0 timeline, mixed timeline segments, 8 target'); - assert.equal(getSyncSegmentCandidate(2, playlist, 8), 4, '2 timeline, mixed timeline segments, 8 target'); + 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) { From fa97a95fba500b5057cbb94f356770c572da76bd Mon Sep 17 00:00:00 2001 From: Brandon Casey <2381475+brandonocasey@users.noreply.github.com> Date: Tue, 25 May 2021 16:39:22 -0400 Subject: [PATCH 04/12] Update src/segment-loader.js Co-authored-by: Garrett Singer --- src/segment-loader.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/segment-loader.js b/src/segment-loader.js index 9f4740a39..7234557f7 100644 --- a/src/segment-loader.js +++ b/src/segment-loader.js @@ -52,8 +52,12 @@ export const getSyncSegmentCandidate = function(currentTimeline, segments, targe } } - // default to grabbing the last timeline segment or zero. - return Math.max(0, timelineSegments[timelineSegments.length - 1] || 0); + if (timelineSegments.length === 0) { + return 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 From eae4f412b9d9a628f5dde52c6b990bf88254cb82 Mon Sep 17 00:00:00 2001 From: Brandon Casey <2381475+brandonocasey@users.noreply.github.com> Date: Tue, 25 May 2021 16:39:34 -0400 Subject: [PATCH 05/12] Update src/segment-loader.js Co-authored-by: Garrett Singer --- src/segment-loader.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/segment-loader.js b/src/segment-loader.js index 7234557f7..ee66cd02c 100644 --- a/src/segment-loader.js +++ b/src/segment-loader.js @@ -2048,8 +2048,8 @@ export default class SegmentLoader extends videojs.EventTarget { const next = this.chooseNextRequest_(); - // if the sync request isn't the segment that would be request next - // after taking into account it's data. do not append it. + // 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; From 17dda6392c4c0e62f0fdf83113e3b353c5e6e426 Mon Sep 17 00:00:00 2001 From: brandonocasey Date: Tue, 25 May 2021 16:45:14 -0400 Subject: [PATCH 06/12] inccorrect assertion text --- test/segment-loader.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/segment-loader.test.js b/test/segment-loader.test.js index 0603491ab..95bb517ab 100644 --- a/test/segment-loader.test.js +++ b/test/segment-loader.test.js @@ -92,7 +92,7 @@ QUnit.test('getSyncSegmentCandidate works as expected', function(assert) { ]; 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, 4 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'); From ad088c548e414fa1ce4c3ec4208a08a306dfa113 Mon Sep 17 00:00:00 2001 From: brandonocasey Date: Tue, 25 May 2021 16:50:51 -0400 Subject: [PATCH 07/12] fix trailing space --- src/segment-loader.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/segment-loader.js b/src/segment-loader.js index ee66cd02c..d864d513c 100644 --- a/src/segment-loader.js +++ b/src/segment-loader.js @@ -55,7 +55,7 @@ export const getSyncSegmentCandidate = function(currentTimeline, segments, targe if (timelineSegments.length === 0) { return 0; } - + // default to the last timeline segment return timelineSegments[timelineSegments.length - 1]; }; From 8ebbc50611d1da70d646d9b50e5189a02467ec91 Mon Sep 17 00:00:00 2001 From: brandonocasey Date: Wed, 26 May 2021 15:37:22 -0400 Subject: [PATCH 08/12] time fudge factor comment --- src/playlist.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/playlist.js b/src/playlist.js index 550bd7ff2..34c4148a3 100644 --- a/src/playlist.js +++ b/src/playlist.js @@ -376,6 +376,9 @@ export const getMediaInfoForTime = function( time += segment.duration; + // TODO: We should consider not using TIME_FUDGE_FACTOR at all here, but at + // the time where this change was made there wasn't enough time to test without it. + // We should make some time to see if we can remove TIME_FUDGE_FACTOR here. if ((time + TIME_FUDGE_FACTOR) > 0) { return { mediaIndex: segment.segmentIndex, @@ -418,6 +421,9 @@ export const getMediaInfoForTime = function( time -= partSegment.duration; + // TODO: We should consider not using TIME_FUDGE_FACTOR at all here, but at + // the time where this change was made there wasn't enough time to test without it. + // We should make some time to see if we can remove TIME_FUDGE_FACTOR here. if ((time - TIME_FUDGE_FACTOR) < 0) { return { mediaIndex: partSegment.segmentIndex, From c6f36e4a3613030fc4ab94b1494c3cd500efb8b3 Mon Sep 17 00:00:00 2001 From: brandonocasey Date: Wed, 26 May 2021 15:40:28 -0400 Subject: [PATCH 09/12] call updateTimingInfoEnd and add some comments --- src/segment-loader.js | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/segment-loader.js b/src/segment-loader.js index d864d513c..04a750fdf 100644 --- a/src/segment-loader.js +++ b/src/segment-loader.js @@ -2038,9 +2038,13 @@ export default class SegmentLoader extends videojs.EventTarget { // timestamp offset. this.updateSourceBufferTimestampOffset_(segmentInfo); - // throw away the isSyncRequest segment if - // it wouldn't have been the next segment we request. + // 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' @@ -2054,8 +2058,8 @@ export default class SegmentLoader extends videojs.EventTarget { 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 From 2bd3f535621375bb409cc33411757c9fb2c776f7 Mon Sep 17 00:00:00 2001 From: brandonocasey Date: Wed, 26 May 2021 15:59:25 -0400 Subject: [PATCH 10/12] better comment --- src/playlist.js | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/playlist.js b/src/playlist.js index 34c4148a3..a92dc4bc5 100644 --- a/src/playlist.js +++ b/src/playlist.js @@ -376,9 +376,7 @@ export const getMediaInfoForTime = function( time += segment.duration; - // TODO: We should consider not using TIME_FUDGE_FACTOR at all here, but at - // the time where this change was made there wasn't enough time to test without it. - // We should make some time to see if we can remove TIME_FUDGE_FACTOR here. + // TODO: consider not using TIME_FUDGE_FACTOR at all here if ((time + TIME_FUDGE_FACTOR) > 0) { return { mediaIndex: segment.segmentIndex, @@ -421,9 +419,7 @@ export const getMediaInfoForTime = function( time -= partSegment.duration; - // TODO: We should consider not using TIME_FUDGE_FACTOR at all here, but at - // the time where this change was made there wasn't enough time to test without it. - // We should make some time to see if we can remove TIME_FUDGE_FACTOR here. + // TODO: consider not using TIME_FUDGE_FACTOR at all here if ((time - TIME_FUDGE_FACTOR) < 0) { return { mediaIndex: partSegment.segmentIndex, From a971ff06b863d651ff384f27ab4c01cf9e6c49df Mon Sep 17 00:00:00 2001 From: brandonocasey Date: Wed, 26 May 2021 16:42:20 -0400 Subject: [PATCH 11/12] add PART-TARGET --- test/playlist-loader.test.js | 10 ++++++++++ 1 file changed, 10 insertions(+) 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' + From 504f4346263fed6a71d1719d7e28c9080470d2fa Mon Sep 17 00:00:00 2001 From: brandonocasey Date: Wed, 26 May 2021 16:51:13 -0400 Subject: [PATCH 12/12] totalParts -> zeroBasedPartCount --- src/segment-loader.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/segment-loader.js b/src/segment-loader.js index 5fb65a7cb..f24aa2f56 100644 --- a/src/segment-loader.js +++ b/src/segment-loader.js @@ -162,10 +162,10 @@ const segmentInfoString = (segmentInfo) => { 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} [${seq + index}/${seq + segmentLen}]` + - (hasPartIndex ? ` part [${partIndex}/${totalParts}]` : '') + + (hasPartIndex ? ` part [${partIndex}/${zeroBasedPartCount}]` : '') + ` segment start/end [${segment.start} => ${segment.end}]` + (hasPartIndex ? ` part start/end [${part.start} => ${part.end}]` : '') + ` startOfSegment [${startOfSegment}]` +