Skip to content

Commit

Permalink
fix: fix several issues with calculate timestamp offset for each segm…
Browse files Browse the repository at this point in the history 
…ent (#1451)

* fix: account replaceSegmentsUntil_ when calculating timestampOffset

* fix: do not reset transmuxer if this is not a discontinuity

* chore: update tests

* chore: update tests

* fix: set fetch at buffer when replace segments until is null

---------

Co-authored-by: Dzianis Dashkevich <ddashkevich@brightcove.com>
  • Loading branch information
@dzianis-dashkevich
dzianis-dashkevich and Dzianis Dashkevich authored Nov 28, 2023
1 parent af39ba5 commit 3bbc6ef
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 10 deletions.
41 changes: 31 additions & 10 deletions src/segment-loader.js
View file
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
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

0 comments on commit 3bbc6ef

Please sign in to comment.