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

Live fMP4 HLS with WebVTT does not display due to timestamp adjustments overflowing int64 #1763

Closed
1 task done
wmho24 opened this issue Sep 26, 2024 · 4 comments
Closed
1 task done
Assignees
Labels

Comments

@wmho24
Copy link

wmho24 commented Sep 26, 2024

Version

Media3 1.3.1

More version details

commit sha1 d833d59

Devices that reproduce the issue

Android Studio Koala Feature Drop 2024.1.2 Patch 1 Medium Phone API 35 emulator

Devices that do not reproduce the issue

No response

Reproducible in the demo app?

Yes

Reproduction steps

Given the following WebVTT content

WEBVTT
X-TIMESTAMP-MAP=LOCAL:479819:32:36.732,MPEGTS:895859864

479819:32:36.732 --> 479819:32:36.920
LANE. JADEN IVEY HANDS IT OFF
INSIDE TO JAYLENOTHER END. ISH G

479819:32:36.920 --> 479819:32:36.954
LANE. JADEN IVEY HANDS IT OFF
INSIDE TO JAYLENOTHER END. ISH G

479819:32:36.954 --> 479819:32:36.987
LANE. JADEN IVEY HANDS IT OFF
INSIDE TO JAYLENOTHER END. ISH G

479819:32:36.987 --> 479819:32:37.120
LANE. JADEN IVEY HANDS IT OFF
INSIDE TO JAYLENOTHER END. ISH G

Loading this WebVTT fragment will result in a calls to androidx.media3.exoplayer.hls.WebVttExtractor::processSample(). Debugging the following to evaluate sampleTimeUs will reveal an int64 overflow on the timestamp passed to usToWrappedPts()

  long sampleTimeUs =
        timestampAdjuster.adjustTsTimestamp(
            TimestampAdjuster.usToWrappedPts(firstCueTimeUs + tsTimestampUs - vttTimestampUs));

When reading fMP4 media, TimestampAdjuster's lastUnadjustedTimestampUs member has a value of 1727354212517266 at the time of the first call to processSample(). This value is in excess of 51-bits, meaning that the call to usToUnwrappedPts() will overflow due to a multiplication with 90000

public synchronized long adjustTsTimestamp(long pts90Khz) {
    if (pts90Khz == C.TIME_UNSET) {
      return C.TIME_UNSET;
    }
    if (lastUnadjustedTimestampUs != C.TIME_UNSET) {
      // The wrap count for the current PTS may be closestWrapCount or (closestWrapCount - 1),
      // and we need to snap to the one closest to lastSampleTimestampUs.
      long lastPts = usToNonWrappedPts(lastUnadjustedTimestampUs);
      long closestWrapCount = (lastPts + (MAX_PTS_PLUS_ONE / 2)) / MAX_PTS_PLUS_ONE;

Note that when the same code executes with HLS TS segments, the value of TimestampAdjuster's lastUnadjustedTimestampUs is a lot smaller: 14405760488 because the first PES header timestamp stayed within 33-bits. It will take many PCR wraparounds before reaching the magnitude seen for fMP4 TFDT 64-bit EPOCH time values above

It is possible to prevent this overflow by multiplying refactored scaling factors 9/100 and 100/9 instead of 90000/1000000 and 1000000/90000

The live HLS playout URL is not available on a permanent basis so an example segment is attached (video + webVtt) that should be enough to initialize exoplayer's TimestampAdjuster and reproduce the issue in debug mode.

A similar issue was opened in April 2023 that relates strongly to this bug report:

google/ExoPlayer#10969

The original issue highlighted a problem with adjustTsTimestamp() but did not dwell on it any further. The workaround cited in the cited issue worked by no longer calling the functions that overflow.

In our case, using scaling factors 9/100 for usToUnwrappedPts and 100/9 for ptsToUs fixes the issue

Expected result

WebVTT content displays when playing fMP4 HLS live feed (as in shakaPlayer and akamai hls.js)

image

Actual result

Nothing is displayed.

Media

segment_287604122.zip

`

Bug Report

@icbaker
Copy link
Collaborator

icbaker commented Sep 26, 2024

Thanks for the report.

It is possible to prevent this overflow by multiplying refactored scaling factors 9/100 and 100/9 instead of 90000/1000000 and 1000000/90000

If you have an easy way to repro with a local dependency on ExoPlayer, could you see if replacing the implementation of TimestampAdjuster.usToNonWrappedPts with the following resolves the problem?

public static long usToNonWrappedPts(long us) {
  return Util.scaleLargeTimestamp(us, 90000, C.MICROS_PER_SECOND);
}

This method is designed to be more overflow-resistant than naive arithmetic.

@wmho24
Copy link
Author

wmho24 commented Sep 27, 2024

Hello,
Thanks for the quick reply. Yes, Util.scaleLargeTimestamp() works without overflowing.
Kind regards

@icbaker
Copy link
Collaborator

icbaker commented Oct 8, 2024

Thanks for confirming, I have a change in review to switch to using this method - it will be linked here when submitted and published to GitHub.

copybara-service bot pushed a commit that referenced this issue Oct 9, 2024
This helps avoid overflows in intermediate calculations.

Verified the value in the test using `BigInteger`:

```
jshell> BigInteger.valueOf(1L << 52).multiply(BigInteger.valueOf(90000)).divide(BigInteger.valueOf(1000000))
$3 ==> 405323966463344
```

Issue: #1763

#cherrypick

PiperOrigin-RevId: 684028178
@icbaker
Copy link
Collaborator

icbaker commented Oct 9, 2024

This should be fixed by b6d0540

@icbaker icbaker closed this as completed Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants