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

Fix math round when number > 2^52 on IE11 #832

Merged
merged 4 commits into from
Jun 2, 2017
Merged

Fix math round when number > 2^52 on IE11 #832

merged 4 commits into from
Jun 2, 2017

Conversation

itaykinnrot
Copy link
Contributor

based on https://stackoverflow.com/questions/12830742/javascript-math-round-bug-in-ie
The issue is that the url that the dash utils provide sometimes wrong due to the Math.round on IE.
https://github.com/google/shaka-player/blob/master/lib/dash/mpd_utils.js#L101

TheModMaker
TheModMaker suggested changes May 31, 2017
View reviewed changes
Copy link
Contributor

@TheModMaker TheModMaker left a comment

Choose a reason for hiding this comment

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

Interesting, I've never heard of this. But thanks for finding it and fixing it.

lib/polyfill/mathround.js Outdated
shaka.log.debug('mathRound.install');

var agent = navigator.userAgent;
if (agent && agent.indexOf('rv:11.0') >= 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to use feature detection instead of browser detection. How about:

if (Math.round(4503599627370497) != 4503599627370497) {
...
}

lib/polyfill/mathround.js Outdated
var original_mathRound = Math.round;
Math.round = function(number) {
var result = number;
// workaround for IE brain-dead Math.round() implementation
Copy link
Contributor

Choose a reason for hiding this comment

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

Please be more politically correct:

// Workaround for a rounding bug in IE11.

lib/polyfill/mathround.js Outdated
// https://stackoverflow.com/questions/12830742/javascript-math-round-bug-in-ie
if (number < 4503599627370496) {
result = original_mathRound(number);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment that includes why it doesn't need to be rounded. Something like "Otherwise, due to the precision of JavaScript numbers, the number must already be an integer."

shaka-player.uncompiled.js Outdated
@@ -49,5 +49,6 @@ goog.require('shaka.polyfill.MediaSource');
goog.require('shaka.polyfill.Promise');
goog.require('shaka.polyfill.VTTCue');
goog.require('shaka.polyfill.VideoPlaybackQuality');
goog.require('shaka.polyfill.MathRound');
Copy link
Contributor

Choose a reason for hiding this comment

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

Alphabetize these calls.

joeyparrish
joeyparrish reviewed May 31, 2017
View reviewed changes
lib/polyfill/mathround.js Outdated
var result = number;
// workaround for IE brain-dead Math.round() implementation
// https://stackoverflow.com/questions/12830742/javascript-math-round-bug-in-ie
if (number < 4503599627370496) {
Copy link
Member

Choose a reason for hiding this comment

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

This constant would be more readable as 0x10000000000000, I think. Also, please move this to a static const. Something like:

/**

  • @const {number}
  • @Private
    */
    shaka.polyfill.MathRound.MAX_ACCURATE_INPUT_ = 0x10000000000000;

Finally, please be careful about < vs <=. From the stackoverflow thread, it appears that this constant does round correctly, so we should use the original for inputs <= the constant.

@TheModMaker TheModMaker mentioned this pull request May 31, 2017
Closed
@joeyparrish joeyparrish added this to the v2.2.0 milestone May 31, 2017
@joeyparrish joeyparrish added type: bug Something isn't working correctly type: external An issue with an external dependency; not our issue; sometimes kept open for tracking labels May 31, 2017
@itaykinnrot
Copy link
Contributor Author

@TheModMaker @joeyparrish Thanks for the review - fixed.

joeyparrish
joeyparrish approved these changes Jun 1, 2017
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.

Looks good to me. @TheModMaker, any feedback?

TheModMaker
TheModMaker approved these changes Jun 2, 2017
View reviewed changes
Copy link
Contributor

@TheModMaker TheModMaker left a comment

Choose a reason for hiding this comment

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

LGTM. Let me run this through our build bot.

@shaka-bot
Copy link
Collaborator

All tests passed!

@TheModMaker TheModMaker merged commit f145dff into shaka-project:master Jun 2, 2017
joeyparrish pushed a commit that referenced this pull request Jun 5, 2017
@joeyparrish
Fix math round when number > 2^52 on IE11 (#832)
734ac9b 
Don't use math round on ie11 if number > 2^52.
@joeyparrish
Copy link
Member

Cherry-picked to v2.1.3.

@avelad avelad mentioned this pull request Apr 11, 2022
Merged
@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

@TheModMaker TheModMaker TheModMaker approved these changes

@joeyparrish joeyparrish joeyparrish approved these changes

Assignees
No one assigned
Labels
status: archived Archived and locked; will not be updated type: bug Something isn't working correctly type: external An issue with an external dependency; not our issue; sometimes kept open for tracking
Projects
None yet
Milestone
v2.2.0
Development

Successfully merging this pull request may close these issues.
4 participants
@itaykinnrot @shaka-bot @joeyparrish @TheModMaker