Skip to content

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

Merged
joeyparrish merged 2 commits intoshaka-project:masterfrom
sanbornhilland:bugFix/fix_bad_bandwidth_sampling
Nov 16, 2016
Merged

Avoid ewma sampling from failing on 0 duration segments.#583
joeyparrish merged 2 commits intoshaka-project:masterfrom
sanbornhilland:bugFix/fix_bad_bandwidth_sampling

Conversation

@sanbornhilland
Copy link
Contributor

@sanbornhilland sanbornhilland commented Nov 10, 2016

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 Outdated
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

@joeyparrish joeyparrish Nov 10, 2016

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

@sanbornhilland sanbornhilland Nov 10, 2016

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

sanbornhilland commented Nov 10, 2016

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

joeyparrish commented Nov 15, 2016

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

@shaka-bot
Copy link
Collaborator

shaka-bot commented Nov 15, 2016

Testing in progress...

@shaka-bot
Copy link
Collaborator

shaka-bot commented Nov 15, 2016

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

joeyparrish commented Nov 15, 2016

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

shaka-bot commented Nov 16, 2016

Testing in progress...

@shaka-bot
Copy link
Collaborator

shaka-bot commented Nov 16, 2016

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