fix: audio segment on incorrect timeline #1530
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1530 +/- ##
==========================================
+ Coverage 86.33% 86.36% +0.03%
==========================================
Files 43 43
Lines 11118 11144 +26
Branches 2540 2545 +5
==========================================
+ Hits 9599 9625 +26
Misses 1519 1519 ☔ View full report in Codecov by Sentry. |
src/segment-loader.js
Outdated
| @@ -413,6 +413,11 @@ export const shouldFixBadTimelineChanges = (timelineChangeController) => { | |||
| const pendingAudioTimelineChange = timelineChangeController.pendingTimelineChange({ type: 'audio' }); | |||
| const pendingMainTimelineChange = timelineChangeController.pendingTimelineChange({ type: 'main' }); | |||
| const hasPendingTimelineChanges = pendingAudioTimelineChange && pendingMainTimelineChange; | |||
|
|
|||
| if (hasPendingTimelineChanges && pendingAudioTimelineChange.to < pendingMainTimelineChange.to) { | |||
| timelineChangeController.trigger('audioTimelineBehind'); | |||
There was a problem hiding this comment.
Should it be inside should fix bad timeline changes?
this looks like a side effect, and should be moved outside
There was a problem hiding this comment.
I agree that this can be moved elsewhere.. but I think there are a few different places it could be placed.
Do you think it would make sense to just check after an audio timeline change?
src/playlist-controller.js
Outdated
| // forwards playback to a point where these two segment types are back on the same | ||
| // timeline. This time will be just after the end of the audio segment that is on | ||
| // a previous timeline. | ||
| this.timelineChangeController_.on('audioTimelineBehind', () => { |
There was a problem hiding this comment.
sounds like we can listen to this event only when dash
There was a problem hiding this comment.
I guess leaving that comment was to point out that this scenario should only happen when DASH.. but I am not sure if it hurts to handle it in any case.
I can probably make that more clear
There was a problem hiding this comment.
I mean, if this code is explicitly intended to handle a dash-specific case, why should we run it for HLS (and potentially impact HLS playback)
I think we should guard it with if statement
src/playlist-controller.js
Outdated
| // are on the same timeline. | ||
| const newTime = segmentInfo.segment.syncInfo.end + 0.01; | ||
|
|
||
| this.tech_.setCurrentTime(newTime); |
There was a problem hiding this comment.
We buffer segments ahead of time. This will jump ahead of what a user sees on the screen at the moment of the jump. Is that expected?
There was a problem hiding this comment.
In this scenario, I believe what the user will see at the moment of the jump is a loading screen. This is because the segments that should be playing are not loaded to due this timeline inconsistency.
In this case, I think the jump is OK, but let me know if I am off there.
There was a problem hiding this comment.
This is because the segments that should be playing are not loaded to due this timeline inconsistency.
Yeah, that is what I am trying to say; the segment you are loading is around bufferedEnd, while what users see is around currentTime. We do not load segments that should be playing right now, in most cases, we are loading future segments (ahead of time).
we load segments around currentTime only when we are switching rendition.
There was a problem hiding this comment.
I guess we should probably check whether currentTime === bufferedEnd, and we are really stuck so we can jump ahead. I suspect we should already have similar logic in the playback watcher...
src/segment-loader.js
Outdated
| return true; | ||
| } | ||
|
|
||
| return false; |
There was a problem hiding this comment.
you can simply:
return hasPendingTimelineChanges && pendingAudioTimelineChange.to < pendingMainTimelineChange.to;
There was a problem hiding this comment.
I really like how this ended up, great suggestions @dzianis-dashkevich and fantastic execution @wseymour15 , especially consolidating the logic around checkAndFixTimelines, that leaves us a cleaner way to add other "corrections" for edge cases here if necessary.
Description
Currently there is an edge case in DASH where certain points of audio segments and video segments do not always exist on the same timeline.
Example: We seek to
100seconds into the video. There is an audio segment that is from 98-100.25 seconds, and the video segment is from 100-109.95. The audio segment is on a timeline from 90-99.99, and the video segment is on a timeline from 110-119.99. We fall into a loop in this scenario because VHS catches that we have bad timelines, but we do not do anything to fix it in this scenario.This change allows us to check for this specific scenario. When we attempt to load an audio segment that is on a prior timeline to the video segment, we will now force the player forward to just past the faulty audio segment. This ensures that the audio and video segments will now be on the same timeline.
Specific Changes proposed
fixBadTimelinelogic outside of thehasEnoughInfoToLoad_andhasEnoughInfoToAppend_functions so we do not have unexpected actions on a function that should just return a boolean.currentTimeto just after the end of the faulty audio segment.Requirements Checklist