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

Jump to bottom

Fix: livestream timeout issues #469

Merged
merged 4 commits into from
May 13, 2019
Merged

Fix: livestream timeout issues #469

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f10c5ef
fix: Do not delete alternative audio renditions
brandonocasey Apr 12, 2019
e612e21
do not use distance
brandonocasey Apr 12, 2019
2a3a12c
better fix, and skip test
brandonocasey Apr 22, 2019
956827d
smaller change
brandonocasey Apr 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 1 addition & 21 deletions src/media-groups.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,10 +337,6 @@ export const setupListeners = {
}
};

const byGroupId = (type, groupId) => (playlist) => playlist.attributes[type] === groupId;

const byResolvedUri = (resolvedUri) => (playlist) => playlist.resolvedUri === resolvedUri;

export const initialize = {
/**
* Setup PlaylistLoaders and AudioTracks for the audio groups
Expand All @@ -357,7 +353,7 @@ export const initialize = {
sourceType,
segmentLoaders: { [type]: segmentLoader },
requestOptions,
master: { mediaGroups, playlists },
master: { mediaGroups },
mediaTypes: {
[type]: {
groups,
Expand All @@ -380,25 +376,9 @@ export const initialize = {

// List of playlists that have an AUDIO attribute value matching the current
// group ID
const groupPlaylists = playlists.filter(byGroupId(type, groupId));

for (let variantLabel in mediaGroups[type][groupId]) {
let properties = mediaGroups[type][groupId][variantLabel];

// List of playlists for the current group ID that have a matching uri with
// this alternate audio variant
const matchingPlaylists =
groupPlaylists.filter(byResolvedUri(properties.resolvedUri));

if (matchingPlaylists.length) {
// If there is a playlist that has the same uri as this audio variant, assume
// that the playlist is audio only. We delete the resolvedUri property here
// to prevent a playlist loader from being created so that we don't have
// both the main and audio segment loaders loading the same audio segments
// from the same playlist.
delete properties.resolvedUri;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this, can cause two segments to be download for the first segment of audio only videos. I think that we should log that as an issue to fix. This makes it so that we do not timeout when a video has separate audio

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the issue is that for an audio-only stream, the main segment loader and the audio segment loader would both download the audio segment. Previously, this delete statement would prevent the audio segment loader from being created. However, this causes issues when audio and video are unmuxed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some regression testing should be done on playlists this affects as I believe there was another issue where one of the segment loaders were not calling end of stream

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this causes issues with unmuxed audio/video. Yes we should do some regression testing, but I think that other endofstream fixes that have been done fixed those issues, at least for the streams that I have been testing with.

}

let playlistLoader;

if (properties.resolvedUri) {
Expand Down
4 changes: 2 additions & 2 deletions src/sync-controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,9 @@ export const syncPointStrategies = [
let segmentStart = segmentTime + syncController.datetimeToDisplayTime;
let distance = Math.abs(currentTime - segmentStart);

// Once the distance begins to increase, we have passed
// Once the distance begins to increase, or if distance is 0, we have passed
// currentTime and can stop looking for better candidates
if (lastDistance !== null && lastDistance < distance) {
if (lastDistance !== null && (distance === 0 || lastDistance < distance)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a legitimate fix, the previous code assumed that if the currentTime started on the segmentStart that we should include that as a valid syncPoint. From my testing this is not the case. We should instead exclude syncPoints that would start exactly on currentTime.

EX: segment#16 at 170s and currentTime at 170s should choose segment#15 rather than segment#16 as currentTime may need data from segment#15 and segment#16 to play.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This explanation would be good to put in the commit body when we merge.

break;
}

Expand Down
2 changes: 1 addition & 1 deletion test/media-groups.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -831,7 +831,7 @@ function(assert) {
'no playlist loader when misconfigured');
});

QUnit.test('initialize audio does not create playlist loader for alternate tracks with' +
QUnit.skip('initialize audio does not create playlist loader for alternate tracks with' +
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a way around this test, since it is testing for something that we do not want to happen for now.

' main stream as URI attribute', function(assert) {
this.master.mediaGroups.AUDIO.aud1 = {
en: { default: true, language: 'en', resolvedUri: 'main.m3u8' },
Expand Down
19 changes: 13 additions & 6 deletions test/sync-controller.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,12 +68,12 @@ QUnit.test('returns correct sync point for ProgramDateTime strategy', function(a

QUnit.test('ProgramDateTime strategy finds nearest segment for sync', function(assert) {
let strategy = getStrategy('ProgramDateTime');
let playlist = playlistWithDuration(40);
let playlist = playlistWithDuration(200);
let timeline = 0;
let duration = Infinity;
let syncPoint;

syncPoint = strategy.run(this.syncController, playlist, duration, timeline, 23);
syncPoint = strategy.run(this.syncController, playlist, duration, timeline, 170);

assert.equal(syncPoint, null, 'no syncpoint when datetimeToDisplayTime not set');

Expand All @@ -83,7 +83,7 @@ QUnit.test('ProgramDateTime strategy finds nearest segment for sync', function(a

this.syncController.setDateTimeMapping(playlist);

let newPlaylist = playlistWithDuration(40);
let newPlaylist = playlistWithDuration(200);

syncPoint = strategy.run(this.syncController, newPlaylist, duration, timeline);

Expand All @@ -93,12 +93,19 @@ QUnit.test('ProgramDateTime strategy finds nearest segment for sync', function(a
segment.dateTimeObject = new Date(2012, 11, 12, 12, 12, 22 + (index * 10));
});

syncPoint = strategy.run(this.syncController, newPlaylist, duration, timeline, 23);
syncPoint = strategy.run(this.syncController, newPlaylist, duration, timeline, 170);

assert.deepEqual(syncPoint, {
time: 20,
segmentIndex: 1
time: 160,
segmentIndex: 15
}, 'syncpoint found for ProgramDateTime set');

syncPoint = strategy.run(this.syncController, newPlaylist, duration, timeline, 0);

assert.deepEqual(syncPoint, {
time: 10,
segmentIndex: 0
}, 'syncpoint found for ProgramDateTime set at 0');
});

QUnit.test('Does not set date time mapping if date time info not on first segment',
Expand Down