PTS/DTS adjustment produces negative PTS/DTS values

[jon-valliere]

Environment

• The stream has correct Access-Control-Allow-Origin headers (CORS)
• There are no network errors such as 404s in the browser console when trying to play the stream
• The issue observed is not already reported by searching on Github under https://github.com/video-dev/hls.js/issues
• The issue occurs in the latest reference client on http://video-dev.github.io/hls.js/demo and not just on my page

• Link to playable M3U8 file: NONE
• Hls.js version: HEAD
• Browser name/version: NONE
• OS name/version: NONE

Steps to reproduce

https://github.com/video-dev/hls.js/blob/master/src/demux/tsdemuxer.js

Both PTS and DTS values have an adjustment code block

if (pesPts > 4294967295) { pesPts -= 8589934592; }

I believe this to be a mistake and the number should be decremented by 2^32 instead of 2^33. This will produce always positive numbers and forcing the integer rollover to occur at the 32bit Javascript integer ceiling.

@tchakabam Had suggested using DataView for reading the numbers. I do not believe this is possible because the PTS/DTS values are coded using the following format.

Using DataView to read a 32bit number would not produce the correct number in this situation.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[jon-valliere]

Bump

[jon-valliere]

It will work correctly as-is running for several days. I don't know why it does... maybe the data is being appended as sequence and the timestamps are being overridden... either that or some miracle code elsewhere is correcting this. I'm not sure.

[fredvb]

The src/remux/mp4-remuxer.js file also has a ptsNormalize method that probably does the same thing.

[itsjamie]

From https://www.w3.org/2013/12/byte-stream-format-registry/mp2t-byte-stream-format.html#mpeg2ts-discontinuities

If folks agree with the below implementation plan I'll go about implementing. Where it talks about timestampOffset, I believe this is already handled, as is the discontinuity. So like the issue report, this is purely about doing the rollover calculation correct.

First step will be to find a MPEG-TS sample with a rollover to add it's bytes as a unit test. If anyone has one on hand, would appreciate it!

7. Timestamp Rollover & Discontinuities
Timestamp rollovers and discontinuities must be handled by the UA. The UA's MPEG-2 TS
implementation must maintain an internal offset variable, MPEG2TS_timestampOffset, to keep track
of the offset that needs to be applied to timestamps that have rolled over or are part of a 
discontinuity. MPEG2TS_timestampOffset is initially set to 0 when the SourceBuffer is created. This 
offset must be applied to the timestamps as part of the conversion process from MPEG-2 TS 
packets into coded frames for the coded frame processing algorithm. This results in the coded 
frame timestamps for a packet being computed by the following equations:

    Coded Frame Presentation Timestamp = (MPEG-2 TS presentation timestamp) + MPEG2TS_timestampOffset
    Coded Frame Decode Timestamp = (MPEG-2 TS decode timestamp) + MPEG2TS_timestampOffset

MPEG2TS_timestampOffset is updated in the following ways:

- Each time a timestamp rollover is detected, 2^33 must be added to MPEG2TS_timestampOffset.
- When a discontinuity is detected, MPEG2TS_timestampOffset must be adjusted to make the timestamps after the discontinuity appear to come immediately after the timestamps before the discontinuity.
- When abort() is called, MPEG2TS_timestampOffset must be set to 0.
- When timestampOffset is successfully set, MPEG2TS_timestampOffset must be set to 0.

[jon-valliere]

I think the first step is to fix any and all rollover math so the numbers don't turn negative. I can work on generating an MPEG-TS version of Big Buck Bunny that overflows in the first 15 seconds or so.

[itsjamie]

We should be fine subtracting the 33-bit number itself. Javascript can safely represent integers to 53 bits.

However, the bitwise operators work on 32-bit integer, so we will need to ensure the current code handles this accurately for the 33-bit timestamp. If something is done incorrectly in this process, I'd imagine it will be there.

[itsjamie]

Hey @jon-valliere I got a test asset generated using Big Buck Bunny.

ffmpeg -i ~/Downloads/bbb_sunflower_1080p_30fps_normal.mp4 \
-c:a libfdk_aac \
-c:v h264 \
-copyts -vsync 2 \
-flags +cgop -g 30 \
-master_pl_name master.m3u8 \
-hls_ts_options "output_ts_offset=95438" \
-hls_time 2 -t 10 \
~/out3/out.m3u8

ffprobe -i ~/out3/master.m3u8 -show_frames | less

Confirms the roll-over in its own way. I can also see it in the hls.js PES parsing code. Looking at places we can host this for long term functional testing. Going to start looking at the code now that I've got all the respective pieces to ensure it's working accurately. 👍

[itsjamie]

szatmary while helping me craft the test asset also made an important point. If we want to handle the rollover completely, one case to keep in mind is one where you join a stream right at the point of rollover.

It's possible the DTS will still be post-rollover, and the PTS is pre-rollover.

[robwalch]

Please see v0.14.3...v0.14.5#diff-14a34ee6c08b15d0e309d10c2c05f3d0 for recent changes in the remuxer.

PTSNormalize has not changed functionally. These changes do clamp PTS to 0 or greater. Negative DTS is allowed to account for composition time (CTS). A PTS sample at '0' with any composition time must have a negative DTS value otherwise CTS is crushed and the AVC frame will not be decoded correctly (might even error in Safari). The down side is really 'big" negative values can slip through, when various workarounds kick in because of bad content (unmarked decreasing discontinuities for example).

Let me know if there's anything I can do to see this work across the line. 👍

[itsjamie]

So, one thing I've identified, because of the interesting property of the hls.js code subtracting 2^32-1, the MPEG-TS PTS rollover point is actually a smooth transition from small negative numbers to small positive numbers.

I'm generating a second asset that will test the rollover point for us internally, which is at 2^32.

It'd be nice to know the original intent behind the way the rollover point is handled now, but I believe it has to do with wanting easier bitwise operations on the PTS. But I don't think it's documented anywhere. The original commit is here; 48119b3 and it references keeping the sign in the 33-bit.

The hls.js rollover point can be tested with;

ffmpeg -i ~/Downloads/bbb_sunflower_1080p_30fps_normal.mp4 \
-c:a libfdk_aac \
-c:v h264 \
-copyts -vsync 2 \
-flags +cgop -g 30 \
-master_pl_name master.m3u8 \
-hls_ts_options "output_ts_offset=47716" \
-hls_time 2 -t 10 \
~/out4-32bit-rollover/out.m3u8

Both continue to playback seamlessly across both the true MPEG-TS PTS rollover point, and when hls.js rolls over to (I believe) keep the PTS represented in a signed 32bit space.

Additionally, I am able to remove the adjustment being made to the PTS / DTS, and nothing seems to care. This might be some code we can just remove now. Putting up a PR to get a Travis run with this removed to test.

The above was true on feature/v1 based branch. It is not so for master. On master, removing this adjustment code leads to very odd behaviour (410 hour stream time offset). The way we calculate InitPTS, and where that is applied has changed between the two branches. And the new way in feature/v1 doesn't set an incorrect stream time duration based on the initial (large) PTS value.

As such, I've removed this code in the transmuxer TS PR; in a48df5b (#2823). However this is definitely tied up in all the fixes that John did on his path to the V1 branch.

[itsjamie]

@robwalch I don't know if there is anything to be done here for < v1. But the changes you've been making more recently around the muxing code if you could get them merged up into feature/v1.0.0 that way we can test to ensure that it isn't something you've done more recently that has changed the behaviour, that I think would be enough to close this out, as the internal PTS and DTS will then be able to be the real values, and unadjusted in v1 at the tsdemuxer API.

I don't believe there is any incorrect behaviour coming from this internal adjustment on either branch, but it does seem to be unnecessary on feature/v1.0.0.

[robwalch]

HI @itsjamie,

I'll get the changes up to v0.14.6 merged in to feature/v1.0.0 by tomorrow afternoon.

[tchakabam]

@tchakabam Had suggested using DataView for reading the numbers. I do not believe this is possible because the PTS/DTS values are coded using the following format.

@jon-valliere I dont' remember exactly in what context this quote was fit, but I understand this is a case of bitstream reading, which obviously DataView itself does not provide. We have a bitstream reader in the demuxing codebase however as part of the Exponential-Golomb decoder which we use for the H264 syntax (ExpGolomb class).

EDIT: That being said, you can also always read any bytes, and get to the bitwise values using appropriates bitmasks/shifting/adding operations :) Nothing else is what the bitstream reader will do for you in the end.

[tchakabam]

On a side note, also check if we can rather make use of Number.MAX_SAFE_INTEGER, which corresponds to 2^53 as @itsjamie pointed out - in most i.e practically all cases. However, using a runtime defined constant here instead of relying on assumption is always a cleaner - and more future-robust approach :)

[jon-valliere]

@tchakabam DataView was suggested as a way to read 64 bit integers directly using getBigUint64() from the data w/o having to do any math, etc however PTS/DTS as represented in MP2T is not coded as integers (as shown in the attached image). Therefore DataView is not a simple solution for producing accurate read out of the PTS/DTS values.

[jon-valliere]

@robwalch The problem with a negative erroneous DTS (negative millions) is when the CTS is computed by subtracting the CTS = PTS - DTS you get a huge crazy number.

[robwalch]

@robwalch I don't know if there is anything to be done here for < v1. But the changes you've been making more recently around the muxing code if you could get them merged up into feature/v1.0.0 that way we can test to ensure that it isn't something you've done more recently that has changed the behaviour, that I think would be enough to close this out, as the internal PTS and DTS will then be able to be the real values, and unadjusted in v1 at the tsdemuxer API.

See #2928

I think what you'll want to do is set enableWorker to false and then step through remuxVideo and remuxAudio with some streams that have wrapping timestamps. What would be really interesting is if there are streams where PTS wraps on a sample before DTS does. There is a small change I added in this PR to set those "ZTS" (Crazy Time Stamps) to 0 in such an event:

https://github.com/video-dev/hls.js/pull/2928/files#diff-08ad4da53cab1312d324d6065c7722d1R266-R271

if (!contiguous || nextAvcDts === null) {
  const pts = timeOffset * timeScale;
  // TODO: Handle case where pts value is wrapped, but dts is not
  const cts = Math.max(0, inputSamples[0].pts - inputSamples[0].dts);
  // if not contiguous, let's use target timeOffset
  nextAvcDts = pts - cts;
  logger.log(`[mp4-remuxer]: nextAvcDts generated as ${nextAvcDts}`);	
}

[robwalch]

Hi @jon-valliere,

Please take a look at this build https://deploy-preview-2930--hls-js-dev.netlify.app/demo/ from #2930 and let it know if it addresses this issue. If you have stream samples with rolling PTS in audio/video/text/id3 tracks to test with it would be great to know they are working.

Thanks!