-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat(ABR): Additional request information for ABR Managers #6313
feat(ABR): Additional request information for ABR Managers #6313
Conversation
Incremental code coverage: 100.00% |
* @exportDoc | ||
*/ | ||
segmentDownloaded(deltaTimeMs, numBytes, allowSwitch) {} | ||
segmentDownloaded(deltaTimeMs, numBytes, allowSwitch, request) {} |
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 don't see this new parameter added to our default AbrManager or any test fakes.
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.
We are using a custom ABR manager and are experimenting with different algorithms and I didn't want to complicate or change the behaviour of the simple ABR manager.
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.
Can you explain the algorithm? Maybe can be interesting in Shaka…
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.
Its basically to mitigate the reasons outlined in this comment
* Minimum number of bytes, under which samples are discarded. Our models |
The concept of min bytes is a problem when you get down to really low bandwidths.
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 don't see this new parameter added to our default AbrManager or any test fakes.
I didn't want to complicate or change the behaviour of the simple ABR manager.
You don't need to implement the behavior. Please just add the argument, so that the method signature of our builtins always agree with the external definition.
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.
Done!
This PR makes the following request information available for ABR consideration. Allowing the ABR manager to know about the request latency from the time to first byte and knowing the order order of a packet, as well as the contentType of the request.