-
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
parse edts box implemented #375
Conversation
Hi @gkatsev @gesinger @joeyparrish |
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.
Thank you for the PR @nklhrstv !
Looks good judging by the layout I see in https://developer.apple.com/library/archive/documentation/QuickTime/QTFF/QTFFChap2/qtff2.html#//apple_ref/doc/uid/TP40000939-CH204-32975
One minor comment.
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.
Thank you!
lib/tools/mp4-inspector.js
Outdated
i; | ||
for (i = 8; entryCount; i += 12, entryCount--) { | ||
result.edits.push({ | ||
segmentDuration: view.getUint32(i), |
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.
looking at the mp4 spec it seems like we need to look at result.version
and if result.version
is 0
then segmentDuration
and mediaTime
will 32 bit and if result.version
is 1
then segmentDurationand
mediaTime` will be 64 bit.
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.
for an example of grabbing 64 bit values (i know js doesn't actually support that many bits for numbers, but we can still get them up to 52 bits) see https://github.com/videojs/mux.js/blob/main/lib/tools/parse-sidx.js#L20-L21
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.
What about BigInt? Ive used .getBigUint64
.
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.
What is the minimum supported node version for this library?
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.
IE11 😂 (sorry)
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.
Check it now
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.
Ive added 64 bit test case.
should be all good now
Hi,
Ive implemented parsing for edts atom by following this spec