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.

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

Revert "Fix repeated segments issue during bandwidth update" #1488

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 9 additions & 3 deletions src/playlist.js
Original file line number Diff line number Diff line change
Expand Up @@ -529,7 +529,9 @@ export const getMediaInfoForTime = function({
// if we passed buffered.end -> it means that this segment is already loaded and buffered

// we should select the next segment if we have one:
continue;
if (i !== partsAndSegments.length - 1) {
continue;
}
}

if (exactManifestTimings) {
Expand All @@ -552,8 +554,12 @@ export const getMediaInfoForTime = function({
};
}

// We are out of possible candidates so there will not be a segment
return null;
// We are out of possible candidates so load the last one...
return {
segmentIndex: partsAndSegments[partsAndSegments.length - 1].segmentIndex,
partIndex: partsAndSegments[partsAndSegments.length - 1].partIndex,
startTime: currentTime
};
};

/**
Expand Down
39 changes: 1 addition & 38 deletions src/segment-loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -1453,7 +1453,7 @@ export default class SegmentLoader extends videojs.EventTarget {
}
} else {
// Find the segment containing the end of the buffer or current time.
let mediaInfo = Playlist.getMediaInfoForTime({
const {segmentIndex, startTime, partIndex} = Playlist.getMediaInfoForTime({
exactManifestTimings: this.exactManifestTimings,
playlist: this.playlist_,
currentTime: this.fetchAtBuffer_ ? bufferedEnd : this.currentTime_(),
Expand All @@ -1462,43 +1462,6 @@ export default class SegmentLoader extends videojs.EventTarget {
startTime: this.syncPoint_.time
});

if (!mediaInfo) {
return null;
}

if (this.fetchAtBuffer_) {
const segment = segments[mediaInfo.segmentIndex];
const segmentEnd = mediaInfo.startTime + segment.duration;

if (segmentEnd > bufferedEnd) {
const difference = segmentEnd - bufferedEnd;
const extendedFudgeFactor = 2 * TIME_FUDGE_FACTOR;

// difference <= 0.06
if (difference <= extendedFudgeFactor) {
// we are trying to choose segment that had been already appended from previous quality
// lets try to choose segment with buffered.end + padding (difference + 0.06)
mediaInfo = Playlist.getMediaInfoForTime({
exactManifestTimings: this.exactManifestTimings,
playlist: this.playlist_,
currentTime: bufferedEnd + difference + extendedFudgeFactor,
startingPartIndex: this.syncPoint_.partIndex,
startingSegmentIndex: this.syncPoint_.segmentIndex,
startTime: this.syncPoint_.time
});

if (!mediaInfo) {
// could not find next segment/part
// early return and wait until playlist is refreshed
return null;
}
// found next segment/part
}
}
}

const {segmentIndex, startTime, partIndex} = mediaInfo;

next.getMediaInfoForTime = this.fetchAtBuffer_ ?
`bufferedEnd ${bufferedEnd}` : `currentTime ${this.currentTime_()}`;
next.mediaIndex = segmentIndex;
Expand Down
54 changes: 0 additions & 54 deletions test/loader-common.js
Original file line number Diff line number Diff line change
Expand Up @@ -1347,60 +1347,6 @@ export const LoaderCommonFactory = ({
assert.ok(!segmentInfo, 'no request generated');
}
);

QUnit.test('does not choose to request if no available playlist for target time', function(assert) {
loader.buffered_ = () => createTimeRanges([[0, 100]]);
const playlist = playlistWithDuration(100);

loader.hasPlayed_ = () => true;
loader.currentTime_ = () => 80;
loader.duration_ = () => Infinity;

loader.playlist(playlist);
loader.load();

loader.fetchAtBuffer_ = true;
const segmentInfo = loader.chooseNextRequest_();

assert.ok(!segmentInfo, 'no request generated');
});

QUnit.test('should select next segment if selected segment\'s end is overlaps with buffered end slightly', function(assert) {
loader.buffered_ = () => createTimeRanges([[0, 99.96]]);
const playlist = playlistWithDuration(110);

loader.hasPlayed_ = () => true;
loader.currentTime_ = () => 80;
loader.duration_ = () => Infinity;

loader.playlist(playlist);
loader.load();

loader.fetchAtBuffer_ = true;
const segmentInfo = loader.chooseNextRequest_();

assert.ok(segmentInfo, 'has segment to select');
assert.equal(segmentInfo.mediaIndex, 10, 'next segment selected');
assert.equal(segmentInfo.startOfSegment, 100, 'next segment selected');
});

QUnit.test('should not select next segment if selected segment\'s end is overlaps with buffered end slightly and it is last segment available', function(assert) {
loader.buffered_ = () => createTimeRanges([[0, 109.96]]);
const playlist = playlistWithDuration(110);

loader.hasPlayed_ = () => true;
loader.currentTime_ = () => 80;
loader.duration_ = () => Infinity;

loader.playlist(playlist);
loader.load();

loader.fetchAtBuffer_ = true;
const segmentInfo = loader.chooseNextRequest_();

assert.ok(!segmentInfo, 'no segment selected');
});

QUnit.test(
'does choose to request if next index is last, we have ended, and are seeking',
function(assert) {
Expand Down
10 changes: 9 additions & 1 deletion test/playback.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,15 @@ if (!videojs.browser.IS_FIREFOX) {
playFor(player, 2, function() {
assert.ok(true, 'played for at least two seconds');
assert.equal(player.error(), null, 'no errors');
done();

player.one('ended', () => {
assert.ok(true, 'triggered ended event');
done();
});

// Firefox sometimes won't loop if seeking directly to the duration, or to too close
// to the duration (e.g., 10ms from duration). 100ms seems to work.
player.currentTime(player.duration() - 0.5);
});

player.src({
Expand Down
12 changes: 6 additions & 6 deletions test/playlist.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1164,8 +1164,8 @@ QUnit.module('Playlist', function() {

assert.deepEqual(
this.getMediaInfoForTime({currentTime: 22}),
null,
'null when out of boundaries'
{partIndex: null, segmentIndex: 2, startTime: 22},
'time greater than the length is index 2'
);
}
);
Expand Down Expand Up @@ -1490,13 +1490,13 @@ QUnit.module('Playlist', function() {

assert.deepEqual(
this.getMediaInfoForTime({currentTime: 159, startTime: 150}),
null,
'returns null when time is equal to end of last segment'
{segmentIndex: 1, startTime: 154, partIndex: null},
'returns last segment when time is equal to end of last segment'
);
assert.deepEqual(
this.getMediaInfoForTime({currentTime: 160, startTime: 150}),
null,
'returns null when time is past end of last segment'
{segmentIndex: 1, startTime: 160, partIndex: null},
'returns last segment when time is past end of last segment'
);
}
);
Expand Down
Loading