-
Notifications
You must be signed in to change notification settings - Fork 211
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(BREAKING): use bigint for 64 bit ints if needed and available. #383
Conversation
@@ -581,8 +582,8 @@ traf = function(track) { | |||
0x00, 0x00, 0x00, 0x00 // default_sample_flags | |||
])); | |||
|
|||
upperWordBaseMediaDecodeTime = Math.floor(track.baseMediaDecodeTime / (UINT32_MAX + 1)); | |||
lowerWordBaseMediaDecodeTime = Math.floor(track.baseMediaDecodeTime % (UINT32_MAX + 1)); | |||
upperWordBaseMediaDecodeTime = Math.floor(track.baseMediaDecodeTime / (MAX_UINT32)); |
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 subtracted 1 from it at the top, only to minus 1 later...
@@ -87,52 +88,55 @@ timescale = function(init) { | |||
* fragment, in seconds | |||
*/ | |||
startTime = function(timescale, fragment) { | |||
var trafs, baseTimes, result; | |||
var trafs, result; |
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.
This code was all kinds of weird. We create an empty array twice to map over another array of trafs?? Then we map over all tfhd/tfdt and parse them, but only take the first? It's all kinds of nuts.
I switched it so that we loop over all traf
s, and only grab the lowest time that we find (which is what we ultimately did later in the code). Furthermore we only look for one tfhd
and one tfdt
as there will only ever be one, and we ignored other in the old code anyway.
seconds = baseTime / scale; | ||
} | ||
|
||
if (seconds < Number.MAX_SAFE_INTEGER) { |
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.
if converting it to the appropriate scale, brings it below the threshold to be a bigInt bring it back to a number.
@@ -403,7 +404,7 @@ QUnit.test('can parse a moov', function(assert) { | |||
flags: new Uint8Array([0, 0, 0]), | |||
edits: [{ | |||
segmentDuration: 0, | |||
mediaTime: 1152921504606847000, | |||
mediaTime: BigInt('0x1000000000000000'), |
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.
This number was 100% wrong before and relying on consistent (but incorrect) rounding in javascript.
b4b9582
to
c8c9bf7
Compare
Stop reading numbers imprecisely by using bigint (if needed).
EDIT: the only way I can see this being non-breaking is if we add an option to the function to use bigint if available, but i'm not sure if that is worth it.