Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Avoid ewma sampling from failing on 0 duration segments. #583

Merged
merged 2 commits into from
Nov 16, 2016
Merged

Avoid ewma sampling from failing on 0 duration segments. #583

merged 2 commits into from
Nov 16, 2016

Conversation

sanbornhilland
Copy link
Contributor

Fixes #582

joeyparrish
joeyparrish reviewed Nov 10, 2016
View reviewed changes
Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Can you also add a new test in test/abr/simple_abr_manager.js to prevent a regression? You can run the tests with ./build/test.py.

lib/abr/ewma.js
@@ -54,7 +54,10 @@ shaka.abr.Ewma = function(halfLife) {
*/
shaka.abr.Ewma.prototype.sample = function(weight, value) {
var adjAlpha = Math.pow(this.alpha_, weight);
this.estimate_ = value * (1 - adjAlpha) + adjAlpha * this.estimate_;
var newEstimate = value * (1 - adjAlpha) + adjAlpha * this.estimate_;
// In rare circumstances newEstimate may come out to NaN. In these
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In these cases, it would be preferable to drop the sample (return early). Adding to totalWeight_ in these cases will skew future samples.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, will do.

@joeyparrish joeyparrish added this to the v2.1.0 milestone Nov 10, 2016
@joeyparrish joeyparrish added the type: bug Something isn't working correctly label Nov 10, 2016
@joeyparrish joeyparrish self-assigned this Nov 10, 2016
@sanbornhilland sanbornhilland force-pushed the bugFix/fix_bad_bandwidth_sampling branch from 3a64b0d to b818ed7 Compare November 10, 2016 21:08
@sanbornhilland
Copy link
Contributor Author

Okay, I simply overwrote the old commit to address your feedback. Please double check that the test I have added seem appropriate. It's a rather indirect way of testing for this condition but I'm not sure how else to do it.

@joeyparrish
Copy link
Member

Looks good to me. I'll run it through the build bot to check.

@shaka-bot
Copy link
Collaborator

Testing in progress...

@shaka-bot
Copy link
Collaborator

Failure:

+ echo START-BUILD
START-BUILD
+ ./build/all.py
----- FILE  :  /var/lib/jenkins/jobs/Manual PR Test/workspace/test/abr/simple_abr_manager_unit.js -----
Line 203, E:0006: Wrong indentation: expected any of {6} but got 10
Line 204, E:0006: Wrong indentation: expected any of {6} but got 10
Line 205, E:0006: Wrong indentation: expected any of {4} but got 8
Found 3 errors, including 3 new errors, in 1 files (161 files OK).
Generating Closure dependencies...
Running Closure linter...
Build step 'Execute shell' marked build as failure

@joeyparrish
Copy link
Member

Looks like it failed some of the linter's checks. You can run python build/all.py to check this locally.

@shaka-bot
Copy link
Collaborator

Testing in progress...

@shaka-bot
Copy link
Collaborator

All tests passed!

@joeyparrish joeyparrish merged commit 465206b into shaka-project:master Nov 16, 2016
joeyparrish pushed a commit that referenced this pull request Nov 30, 2016
@joeyparrish
Avoid ewma sampling from failing on 0 duration segments. (#583)
661a1d2 
This fixes a divide-by-zero that caused the estimate to become NaN.

Fixes #582
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@joeyparrish joeyparrish joeyparrish left review comments

Assignees

@joeyparrish joeyparrish

Labels
status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Projects
None yet
Milestone
v2.1.0
Development

Successfully merging this pull request may close these issues.
3 participants
@sanbornhilland @joeyparrish @shaka-bot