Fix: livestream timeout issues #469
Conversation
| // 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; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
brandonocasey
force-pushed
the
fix/livestream-timeout
branch
from
4be8ef1
to
e612e21
Compare
brandonocasey
force-pushed
the
fix/livestream-timeout
branch
4 times, most recently
from
ed8c4ce
to
c0d6852
Compare
brandonocasey
force-pushed
the
fix/livestream-timeout
branch
from
c0d6852
to
956827d
Compare
| // currentTime and can stop looking for better candidates | ||
| if (lastDistance !== null && lastDistance < distance) { | ||
| if (lastDistance !== null && (distance === 0 || lastDistance < distance)) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This explanation would be good to put in the commit body when we merge.
| @@ -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' + | |||
There was a problem hiding this comment.
I don't see a way around this test, since it is testing for something that we do not want to happen for now.
Description
This pull requests fixes some HLS livestream timeouts that were happing before playback would even begin. I will go over them in code comments.