-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
split ThroughputHistory out of ThroughputRule, update InsufficientBufferRule #2003
split ThroughputHistory out of ThroughputRule, update InsufficientBufferRule #2003
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.
LGTM
@robertbryer @bbcrddave Kevin is looking for outside reviewers for the ABR PR. |
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.
It LGTM but @robertbryer is our ABR expert, so it'd be worth him looking.
|
||
arr = arr.slice(-sampleSize); // still works if sampleSize too large | ||
// arr.length >= 1 | ||
return arr.reduce((av, elem, i) => av + (elem - av) / (i + 1)); |
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'd prefer arr.reduce((av, elem) => av + elem) / arr.length - more intelligible and only one divide is done.
let throughput = throughputHistory.getAverageThroughput(mediaType); | ||
let latency = throughputHistory.getAverageLatency(mediaType); | ||
|
||
let bitrate = throughput * (bufferLevel / fragmentDuration) * INSUFFICIENT_BUFFER_SAFETY_FACTOR; |
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.
Assuming this rule will be triggered immediately after an append to buffer (likely, at these buffer levels), (bufferLevel/fragmentDuration) will be a minimum of 1, so the throughput can only ever be halved at most - Was a more radical bitrate change wanted?
In the same vein, does this still work for MPDs where fragment duration is not constant?
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.
... the throughput can only ever be halved at most - Was a more radical bitrate change wanted?
I thought 0.5 should be conservative enough, but it's just an arbitrary value which I wouldn't mind changing.
In the same vein, does this still work for MPDs where fragment duration is not constant?
I don't think fragment duration varies too much. This still works fine with small (<10%) variation in fragment size. Did you have a particular example in mind with larger variation?
Move the throughput/latency estimation from ThroughputRule to a separate ThroughputHistory.
Update the InsufficientBufferRule to also avoid downloading bitrates that are more prone to lead to rebuffering.