-
Notifications
You must be signed in to change notification settings - Fork 427
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
fix: Return the correct buffer for each segmentloader #542
Conversation
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 makes sense and seems to sort of match usage in remove()
.
I'd still like to test this before merging, though.
4a5c793
to
8ec0ed4
Compare
Added the source I used for testing, although you will have to force the 4k rendition to get it to happen. |
Would be nice to add the quality-levels plugin on this page as well. I added |
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.
Just tested this. Without the change, I kept getting sourcebuffer is full error messages and then about 6 minutes it stalled. With this change it is playing back without any error messages.
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.
LGTM, makes sense to me.
Edit: Did a quick test with the 4K rendition and got a lot of buffering but quick recovery and no stalls
@@ -150,6 +150,7 @@ | |||
var urls = [ | |||
'node_modules/video.js/dist/alt/video.core', | |||
'node_modules/videojs-contrib-eme/dist/videojs-contrib-eme', | |||
'node_modules/videojs-contrib-quality-levels/dist/videojs-contrib-quality-levels', |
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 breaks the Netlify build, probably because videojs-contrib-quality-levels
would have to be added to the devDependencies
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.
Ah, whoops. Needs to be copied over in the netlify script
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.
just needs to be added here
http-streaming/scripts/netlify.js
Lines 5 to 10 in 0d8eb57
const vjs = 'node_modules/video.js/dist/alt/video.core.js'; | |
const vjsCss = 'node_modules/video.js/dist/video-js.css'; | |
const eme = 'node_modules/videojs-contrib-eme/dist/videojs-contrib-eme.js'; | |
const indexScript = 'scripts/index.js'; | |
const deployDir = 'deploy'; | |
const files = [vjs, vjsCss, eme, indexScript]; |
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.
interesting will do
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.
is there any reason that we don't just deploy out of the root project directory?
6609a6b
to
ade1c1a
Compare
The only failing test is the live dash playback one. It is also failing unreliably, even if most of the time. When applying #545 it can reliably pass. |
With the current implementation a separate video and audio segment loader will try to use the combined buffered value, this causes all kinds of issues. For high bit rate sources we see a lot of append over max on source buffer errors.