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.

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

Merged
merged 8 commits into from
Nov 29, 2021

Conversation

brandonocasey
Copy link
Contributor

@brandonocasey brandonocasey commented May 17, 2021

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.

@@ -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));
Copy link
Contributor Author

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;
Copy link
Contributor Author

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 trafs, 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) {
Copy link
Contributor Author

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'),
Copy link
Contributor Author

@brandonocasey brandonocasey May 17, 2021

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.

lib/mp4/caption-parser.js Outdated Show resolved Hide resolved
lib/utils/max-uint-32.js Outdated Show resolved Hide resolved
@gkatsev gkatsev merged commit 83779b9 into main Nov 29, 2021
@gkatsev gkatsev deleted the feat/64-bit-support branch November 29, 2021 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants