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
Smooth period transition #2580
Smooth period transition #2580
Conversation
Good stuff! Trying to seek to the next period doesn't work when you have already preloaded into the period. I get:
So I think a normal buffer reset is being called after the preload when you seek like this. |
@@ -644,6 +702,80 @@ function Stream(config) { | |||
checkIfInitializationCompleted(); | |||
} | |||
|
|||
function isCompatibleWithStream(stream) { | |||
return compareCodecs(stream, Constants.VIDEO) && compareCodecs(stream, Constants.AUDIO); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will return false for audio-only (and video-only streams), which is incorrect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This requires a few changes like checking if the previous / next period has audio and video to avoid saving the non-needed buffer or recreating it, will add it last.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added commit d7f4b8f to allow preload of audio-only or video-only periods, but won't work when switching from a video/audio period to a video-only one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following that commit, this works great for multiperiod audio-only services 🍾
Streams signalling InbandEvent throw the following error on Period transition, and playback stops:
Example manifest:
|
Overall, this is great stuff. The performance is significantly better than without this patch, but I see some strange behaviour (differing between browsers) happening and the result is not completely seamless. I believe the content is correct and should be seamlessly playable. In Chrome (66, Windows 7), the video always pauses on the frame displaying timecode 00:00:18:18. The audio sounds like it is seamless but it's hard to tell without a continuous tone. In Firefox (61, Windows 7), the video and audio glitch on the frame displaying timecode 00:00:19:04 (which is the final frame of the first Period). Example MPD: Same as in #2580 (comment), but with InbandEventStreams removed. |
src/streaming/SourceBufferSink.js
Outdated
return buffer.buffered; | ||
} catch (e) { | ||
log('getAllBufferRanges exception: ' + e.message); | ||
return []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm expecting this function to either return something null-like, or an object implementing the timeranges interface: .start(n)
.end(n)
.length
. Array here works, because the empty array returns length 0 and then nobody calls start or end in that case, but it seems initially confusing reading this to be returning an array. Maybe add an explainer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Null it is then
I added 2 commits to fix @robertbryer issues. @bbcrddave I can reproduce the EDIT: Fixed the InbandEvent issue, I also tested on Windows 10 and saw some frames are dropped while transitioning on Firefox if using old hardware ( i5-540M, RAM 4GB ) but Chrome and Edge worked like a charm. I tested it in a newer laptop and every browser worked fine. |
Thanks for all the great feedback! Given the importance of the changes of this PR and the need of testing it exhaustively, I am going to keep this out of dash.js v2.7.0 (code freeze today). Let's continue testing this and I will do the merge once we feel comfortable with this PR and after releasing dash.js v2.7.0. |
I updated Chrome to the latest patch level and it now appears to play smoothly through the transition 👍 |
Something weird is happening which, in the worst case, is occasionally causing playback to stop and, in the best case, causing segments to be redownloaded. Given the manifest below, at the period transition, a portion of the buffered but as-yet-unplayed range is removed. Sometimes the segments are redownloaded and appended again, sometimes playback stalls. The buffered range removed is always the same. Log highlights:
Sample manifest:
|
@bbcrddave fixed in ea21de8 , seek was called if track was only audio or only video, it's weird that it only caused troubled with some streams... |
6325e95
to
c18e23b
Compare
… while preloading the next one
c18e23b
to
37f8637
Compare
This PR introduces buffer reuse for compatible codecs and preload of next periods to avoid jumps and freezes when switching periods.
It also introduces some changes to the SourceBufferSink to handle the appends and removes with a queue.
It's the same as #2575 but with clearer commit history.
Closes #1708