Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix calculate timestamp offset for each segment #1451

Merged
merged 5 commits into from
Nov 28, 2023
Merged

Fix calculate timestamp offset for each segment #1451

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a79eda8
fix: account replaceSegmentsUntil_ when calculating timestampOffset
Nov 27, 2023
d599256
fix: do not reset transmuxer if this is not a discontinuity
Nov 27, 2023
4855445
chore: update tests
Nov 27, 2023
838ea45
chore: update tests
Nov 27, 2023
7c11f4d
fix: set fetch at buffer when replace segments until is null
Nov 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 31 additions & 10 deletions src/segment-loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,9 +179,17 @@ export const segmentInfoString = (segmentInfo) => {

const timingInfoPropertyForMedia = (mediaType) => `${mediaType}TimingInfo`;

const getBufferedEndOrFallback = (buffered, fallback) => buffered.length ?
buffered.end(buffered.length - 1) :
fallback;
const getTimestampOffset = (buffered, replaceSegmentsUntil, fallback) => {
if (replaceSegmentsUntil !== null) {
return fallback;
}

if (buffered.length) {
return buffered.end(buffered.length - 1);
}

return fallback;
};

/**
* Returns the timestamp offset to use for the segment.
Expand All @@ -196,6 +204,8 @@ const getBufferedEndOrFallback = (buffered, fallback) => buffered.length ?
* The loader's buffer
* @param {boolean} calculateTimestampOffsetForEachSegment
* Feature flag to always calculate timestampOffset
* @param {number|null} replaceSegmentsUntil
* value if we switched quality recently and replacing buffered with a new quality
* @param {boolean} overrideCheck
* If true, no checks are made to see if the timestamp offset value should be set,
* but sets it directly to a value.
Expand All @@ -210,10 +220,11 @@ export const timestampOffsetForSegment = ({
startOfSegment,
buffered,
calculateTimestampOffsetForEachSegment,
replaceSegmentsUntil,
overrideCheck
}) => {
if (calculateTimestampOffsetForEachSegment) {
return getBufferedEndOrFallback(buffered, startOfSegment);
return getTimestampOffset(buffered, replaceSegmentsUntil, startOfSegment);
}

// Check to see if we are crossing a discontinuity to see if we need to set the
Expand Down Expand Up @@ -259,7 +270,7 @@ export const timestampOffsetForSegment = ({
// should often be correct, it's better to rely on the buffered end, as the new
// content post discontinuity should line up with the buffered end as if it were
// time 0 for the new content.
return getBufferedEndOrFallback(buffered, startOfSegment);
return getTimestampOffset(buffered, replaceSegmentsUntil, startOfSegment);
};

/**
Expand Down Expand Up @@ -1600,6 +1611,7 @@ export default class SegmentLoader extends videojs.EventTarget {
startOfSegment,
buffered: this.buffered_(),
calculateTimestampOffsetForEachSegment: this.calculateTimestampOffsetForEachSegment_,
replaceSegmentsUntil: this.replaceSegmentsUntil_,
overrideCheck
});

Expand Down Expand Up @@ -2466,7 +2478,7 @@ export default class SegmentLoader extends videojs.EventTarget {
//
// Even though keepOriginalTimestamps is set to true for the transmuxer, timestamp
// offset must be passed to the transmuxer for stream correcting adjustments.
if (this.shouldUpdateTransmuxerTimestampOffset_(segmentInfo.timestampOffset)) {
if (this.shouldUpdateTransmuxerTimestampOffset_(segmentInfo)) {
this.gopBuffer_.length = 0;
// gopsToAlignWith was set before the GOP buffer was cleared
segmentInfo.gopsToAlignWith = [];
Expand Down Expand Up @@ -2757,21 +2769,26 @@ export default class SegmentLoader extends videojs.EventTarget {
}
}

shouldUpdateTransmuxerTimestampOffset_(timestampOffset) {
if (timestampOffset === null) {
shouldUpdateTransmuxerTimestampOffset_(segmentInfo) {
if (this.calculateTimestampOffsetForEachSegment_) {
// is discontinuity
return segmentInfo.timeline !== this.currentTimeline_;
}

if (segmentInfo.timestampOffset === null) {
return false;
}

// note that we're potentially using the same timestamp offset for both video and
// audio

if (this.loaderType_ === 'main' &&
timestampOffset !== this.sourceUpdater_.videoTimestampOffset()) {
segmentInfo.timestampOffset !== this.sourceUpdater_.videoTimestampOffset()) {
return true;
}

if (!this.audioDisabled_ &&
timestampOffset !== this.sourceUpdater_.audioTimestampOffset()) {
segmentInfo.timestampOffset !== this.sourceUpdater_.audioTimestampOffset()) {
return true;
}

Expand Down Expand Up @@ -3075,8 +3092,12 @@ export default class SegmentLoader extends videojs.EventTarget {
this.addSegmentMetadataCue_(segmentInfo);
if (this.replaceSegmentsUntil_ !== null && this.currentTime_() >= this.replaceSegmentsUntil_) {
this.replaceSegmentsUntil_ = null;
}

if (this.replaceSegmentsUntil_ === null) {
this.fetchAtBuffer_ = true;
}

if (this.currentTimeline_ !== segmentInfo.timeline) {
this.timelineChangeController_.lastTimelineChange({
type: this.loaderType_,
Expand Down
10 changes: 10 additions & 0 deletions test/segment-loader.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,7 @@ QUnit.module('timestampOffsetForSegment');
QUnit.test('returns startOfSegment when calculateTimestampOffsetForEachSegment is enabled and the buffer is empty with the same timeline', function(assert) {
const timestampOffset = timestampOffsetForSegment({
calculateTimestampOffsetForEachSegment: true,
replaceSegmentsUntil: null,
segmentTimeline: 0,
currentTimeline: 0,
startOfSegment: 3,
Expand All @@ -237,6 +238,7 @@ QUnit.test('returns startOfSegment when calculateTimestampOffsetForEachSegment i
QUnit.test('returns startOfSegment when calculateTimestampOffsetForEachSegment is enabled and the buffer is empty with different timeline', function(assert) {
const timestampOffset = timestampOffsetForSegment({
calculateTimestampOffsetForEachSegment: true,
replaceSegmentsUntil: null,
segmentTimeline: 1,
currentTimeline: 0,
startOfSegment: 3,
Expand All @@ -249,6 +251,7 @@ QUnit.test('returns startOfSegment when calculateTimestampOffsetForEachSegment i
QUnit.test('returns buffered.end when calculateTimestampOffsetForEachSegment is enabled and there exists buffered content with the same timeline', function(assert) {
const timestampOffset = timestampOffsetForSegment({
calculateTimestampOffsetForEachSegment: true,
replaceSegmentsUntil: null,
segmentTimeline: 0,
currentTimeline: 0,
startOfSegment: 3,
Expand All @@ -261,6 +264,7 @@ QUnit.test('returns buffered.end when calculateTimestampOffsetForEachSegment is
QUnit.test('returns buffered.end when calculateTimestampOffsetForEachSegment is enabled and there exists buffered content with different timeline', function(assert) {
const timestampOffset = timestampOffsetForSegment({
calculateTimestampOffsetForEachSegment: true,
replaceSegmentsUntil: null,
segmentTimeline: 1,
currentTimeline: 0,
startOfSegment: 3,
Expand All @@ -273,6 +277,7 @@ QUnit.test('returns buffered.end when calculateTimestampOffsetForEachSegment is
QUnit.test('returns startOfSegment when timeline changes and the buffer is empty', function(assert) {
assert.equal(
timestampOffsetForSegment({
replaceSegmentsUntil: null,
segmentTimeline: 1,
currentTimeline: 0,
startOfSegment: 3,
Expand All @@ -286,6 +291,7 @@ QUnit.test('returns startOfSegment when timeline changes and the buffer is empty
QUnit.test('returns buffered end when timeline changes and there exists buffered content', function(assert) {
assert.equal(
timestampOffsetForSegment({
replaceSegmentsUntil: null,
segmentTimeline: 1,
currentTimeline: 0,
startOfSegment: 3,
Expand All @@ -299,6 +305,7 @@ QUnit.test('returns buffered end when timeline changes and there exists buffered
QUnit.test('returns null when timeline does not change', function(assert) {
assert.ok(
timestampOffsetForSegment({
replaceSegmentsUntil: null,
segmentTimeline: 0,
currentTimeline: 0,
startOfSegment: 3,
Expand All @@ -309,6 +316,7 @@ QUnit.test('returns null when timeline does not change', function(assert) {

assert.ok(
timestampOffsetForSegment({
replaceSegmentsUntil: null,
segmentTimeline: 1,
currentTimeline: 1,
startOfSegment: 3,
Expand All @@ -321,6 +329,7 @@ QUnit.test('returns null when timeline does not change', function(assert) {
QUnit.test('returns value when overrideCheck is true', function(assert) {
assert.equal(
timestampOffsetForSegment({
replaceSegmentsUntil: null,
segmentTimeline: 0,
currentTimeline: 0,
startOfSegment: 3,
Expand All @@ -335,6 +344,7 @@ QUnit.test('returns value when overrideCheck is true', function(assert) {
QUnit.test('uses startOfSegment when timeline is before current', function(assert) {
assert.equal(
timestampOffsetForSegment({
replaceSegmentsUntil: null,
segmentTimeline: 0,
currentTimeline: 1,
startOfSegment: 3,
Expand Down