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

Large TFDT Base Media Decode Time values fail with JS_INTEGER_OVERFLOW #3784

Closed
stevemayhew opened this issue Dec 1, 2021 · 11 comments
Closed
Labels
priority: P3 Useful but not urgent status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Milestone

Comments

@stevemayhew
Copy link

Have you read the FAQ and checked for duplicate open issues?

Yes, it looks like #1610 might be the only possible related issue, but the chance it is seems remote. With this issue you get a JS_INTEGER_OVERFLOW error, no chance for playback.

What version of Shaka Player are you using?
Multiple, all have this same issue

Can you reproduce the issue with our latest release version?
Yes

Can you reproduce the issue with the latest code from master?
Yes

Are you using the demo app or your own custom app?
The off the rack demo app reproduces it. I'll attach a sample MP4 segment, I will look for a test case that parses a segment which should reproduce it easily.

If custom app, can you reproduce the issue using our demo app?
Yes

What browser and OS are you using?
Chrome Version 96.0.4664.55 (Official Build) (x86_64) on Mac OS X BigSur

For embedded devices (smart TVs, etc.), what model and firmware version are you using?
N/A

What are the manifest and license server URIs?

I can create an HLS playlist, I have a simple single video segment that reproduces it.

What configuration are you using? What is the output of player.getConfiguration()?

N/A

What did you do?

Play the indicated playlist.

What did you expect to happen?
I should play normally

What actually happened?

It fails immediately with JS_INTEGER_OVERFLOW

The reason is the TRUN box Base Media Decode time is represented as 1/10 microsecond since the epoch, this is indicated in the init segment:

mp4dump --verbosity 3  1280x720_10333984/CCURStream_MultiPortMulticast1-1_1637252673_init.cmfv 
[ftyp] size=8+20
  major_brand = cmfc
  minor_version = 0
  compatible_brand = iso6
  compatible_brand = dsms
  compatible_brand = dash
[moov] size=8+775
  [mvhd] size=12+108, version=1
    timescale = 10000000
    duration = 600000000
    duration(ms) = 60000

In the first sample, the base media decode time is:

[moof] size=8+808
  [mfhd] size=12+4
    sequence number = 360701
  [traf] size=8+784
    [tfhd] size=12+8, flags=20020
      track ID = 1
      default sample flags = 1010000
    [tfdt] size=12+8, version=1
      base media decode time = 16379747984633557
    [trun] size=12+732, version=1, flags=b05
      sample count = 60
      data offset = 824
      first sample flags = 2000000

So you can see:

16379747984633557 / 10000000 = 1637974798.4633557

$ epochToTime 1637974798
+ gdate --date=@1637974798
Fri Nov 26 16:59:58 PST 2021

Perfectly valid timestamp. Of course I understand Javascript cannot natively represent the this in an IEEE double.

If all the stakeholders agree on the approach I will take the bug and create a pull request with a solution. I'm already a contributor to ExoPlayer, Hls.js and other players.

My plan is to scale the value before it is converted to a JavaScript Number (53 bit mantissa double). Once it is seconds in a shaka.media.SegmentReference the changes end (as 2**53 seconds is a ridiculous time in the future)

What I would do is use the BigInt poly fill (https://github.com/peterolson/BigInteger.js) and scale the value first with the timescale before using it. I would scale it with a solution similar to what ExoPlayer use in Util.scaleLargeTimestamp() to only use the scaled value.

The change would:

  1. add a method to reader, DataViewReader. readBigInt64()
  2. Create a util shamelessly copied from Util.scaleLargeTimestamp() (It's the same CLA right?)
  3. Use this in _parseEMSGand parseSIDX_

Other 64 bit integers in BMFF (length of boxes, etc) are extremely unlikely to exceed 2**53 so retrofitting this existing code would be rather pointless.

This is certainly not simple, so I would not start on it until (other then this design) until it is agreed this is a reasonable design.

FWIW this is a major Origin Server vendor and transcoder that produces the CMAF segments, and they are completely to spec processed by ExoPlayer with out any issues. Solution for us will deploy on desktop browsers and at some point in later on Tizen and WebOS based TVs.

@stevemayhew stevemayhew added the type: bug Something isn't working correctly label Dec 1, 2021
@shaka-bot shaka-bot added this to the v3.3 milestone Dec 1, 2021
@TheModMaker TheModMaker added the priority: P3 Useful but not urgent label Jan 18, 2022
@avelad avelad modified the milestones: v3.3, v4.1 May 4, 2022
@avelad avelad modified the milestones: v4.1, v4.2 Jun 3, 2022
@avelad avelad modified the milestones: v4.2, v4.3 Aug 17, 2022
@avelad avelad modified the milestones: v4.3, v4.4 Nov 11, 2022
@alexgerv
Copy link

Hi @stevemayhew. Did you end up implementing the approach you proposed ? We are facing the exact same problem, i'm curious to know what was your outcome.

@avelad
Copy link
Member

avelad commented Jul 5, 2023

Can you check again against the main branch? Thanks!

@avelad avelad added status: waiting on response Waiting on a response from the reporter(s) of the issue component: HLS The issue involves Apple's HLS manifest format labels Jul 5, 2023
@alexgerv
Copy link

alexgerv commented Jul 7, 2023

Hi @avelad . Are you asking because there is a specific commit which improved things, or just wondering if the issue is repro'able ?

Also seeing the "component: HLS" label, just noting it does not matter which streaming protocol is used (DASH or HLS)

@joeyparrish joeyparrish removed the component: HLS The issue involves Apple's HLS manifest format label Jul 7, 2023
@joeyparrish
Copy link
Member

We would like to know if it can still be repro'd with the latest code in main, because there were some recent changes to remove the restriction in cases where a slightly-inaccurate value is acceptable, such as when dividing by the timescale to compute a time in seconds.

Those changes are not in a release version yet, so we would like you to check a build from main or use the nightly demo build we host: https://nightly-dot-shaka-player-demo.appspot.com/

(When we need an exact integer, such as when filling in a value for $Time$ in a DASH segment template, the restriction remains in place.)

Thanks!

@github-actions github-actions bot removed the status: waiting on response Waiting on a response from the reporter(s) of the issue label Jul 7, 2023
@alexgerv
Copy link

Hi Joey.
Our specific issue is with the Mp4CeaParser.

When parsing the TFDT box to get the baseMediaDecodeTime, in does still try to readUint64 with full precision.

Unfortunately that code is still there in the main branch.

@alexgerv
Copy link

Ahh I see, in #5329, parseTFDTInaccurate is being used for MediaSourceEngine, not for Mp4CeaParser.

Would it be a good idea to use the inaccurate version in Mp4CeaParser too ?

@joeyparrish
Copy link
Member

Yes, I think so. The only place I can think of where an exact media timestamp is required in timescale units is in DASH SegmentTemplate's $Time$ replacement. Everywhere else, we process media times in seconds, where rounding of a 64-bit int with a large timescale shouldn't matter.

@alexgerv, if you are motivated to contribute, I would be happy to review a PR that uses parseTFDTInaccurate in more places. New test cases for that would also be appreciated. Thanks!

@joeyparrish joeyparrish added the flag: seeking PR We are actively seeking PRs for this; we do not currently expect the core team will resolve this label Jul 11, 2023
@stevemayhew
Copy link
Author

stevemayhew commented Jul 12, 2023 via email

@avelad
Copy link
Member

avelad commented Aug 23, 2023

@alexgerv Can you check again against the main branch? Thanks!
#5501 has been merged recently

@avelad avelad added status: waiting on response Waiting on a response from the reporter(s) of the issue and removed flag: seeking PR We are actively seeking PRs for this; we do not currently expect the core team will resolve this labels Aug 23, 2023
@github-actions
Copy link
Contributor

Closing due to inactivity. If this is still an issue for you or if you have further questions, the OP can ask shaka-bot to reopen it by including @shaka-bot reopen in a comment.

@github-actions github-actions bot removed the status: waiting on response Waiting on a response from the reporter(s) of the issue label Aug 30, 2023
@alexgerv
Copy link

alexgerv commented Sep 6, 2023

@alexgerv Can you check again against the main branch? Thanks! #5501 has been merged recently

@avelad @joeyparrish thank you very much for that change, it does fix the CEA-608 closed-captions issue we had with large baseMediaDecodeTime values.

@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Oct 29, 2023
@shaka-project shaka-project locked as resolved and limited conversation to collaborators Oct 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
priority: P3 Useful but not urgent status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Projects
None yet
Development

No branches or pull requests

6 participants