-
Notifications
You must be signed in to change notification settings - Fork 428
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: don't save bandwidth and throughput for really small segments #1024
fix: don't save bandwidth and throughput for really small segments #1024
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.
also wondering about partial segment durations or ll-hls which can have really tiny segment durations.
src/segment-loader.js
Outdated
// With most content hovering around 30fps, if a segment has a duration less than | ||
// one frame, the bandwidth and throughput calculations will not accurately reflect | ||
// the rest of the content. | ||
const MIN_SEGMENT_DURATION_TO_SAVE_STATS = 1 / 30; |
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.
would it make sense to use FRAME-RATE
from the playlist if it exists here so that this doesn't mess with calculations for 60fps content? Could be a future optimization too I guess.
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.
Maybe this should just be 1 / 60 instead? Not sure if that would be too low though.
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 figured even for 60 fps content we'd probably be safe (it's possible, though I'd imagine unlikely, for someone to send less than 2 frames of content, even at 60fps), but it could be worth noting here. 1 / 60 may be cutting it close, as it should work when rounded, but might be problematic if truncated or floored.
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.
Thinking about it again though, maybe it is worth it to just use 1/60. Will give it some testing.
I'm not sure we need to care about LL-HLS for this, but we'll want to note it for when we do work on LLHLS. |
@gkatsev , I think we should be OK for LL-HLS as we'll have to parse EXT-X-PART differently to note the duration. Though if we try to reuse the normal segment parsing and properties names then we may have to consider it. Though even then I think we'll be alright with 1 frame at 30fps or 2 frames at 60fps (I guess people can send just 1 or 2 frames at a time for LL-HLS, but I imagine it as unlikely). |
Also fix VTT loader bandwidth save call
Description
Really small segments can mess with our ABR algorithm by not reflecting our bandwidth and throughput accurately for the majority of segments.
Requirements Checklist