obs-outputs: Preserve signedness of FLV timestamp

[jeongki-kim]

Description

obs-outputs: Preserve signedness of FLV timestamp

Motivation and Context

Video File Format Specification Version 10 states that a timestamp in FLV format is a signed 32-bit integer, which are composed of two integers.

Field	Type	Comment
...	...	...
Timestamp	UI24	Time in milliseconds at which the data in this tag applies. This value is relative to the first tag in the FLV file, which always has a timestamp of 0.
TimestampExtended	UI8	Extension of the Timestamp fields to form a SI32 value. This field represents the upper 8 bits, while the previous Timestamp field represents the lower 24 bits of the time in milliseconds.
...	...	...

How Has This Been Tested?

Inspected RTMP packets with Wireshark.

Types of changes

• Bug fix (non-breaking change which fixes an issue)

Checklist:

• My code has been run through clang-format.
• I have read the contributing document.
• My code is not on the master branch.
• The code has been tested.
• All commit messages are properly formatted and commits squashed where appropriate.
• I have included updates to all appropriate documentation.

[derrod]

The timestamp in RTMP is unsigned, so this would result in invalid or out-of-order timestamps being sent. Based on advice from one of the authors of the Enhanced FLV/RTMP spec, we should never send negative or non-monotonic timestamps (he also believes the SI32 in the legacy FLV spec might be a mistake).

Since we already start the presentation at a non-0 time to compensate for b-frame delay, it follows that doing the same for audio encoder delay is probably the right approach, hence #10999.

[jeongki-kim]

As RTMP timestamps are unsigned like you said, and they are wrapped around when they overflow, the most significant bit in the timestamp value no longer represents signedness. Also discarding the MSB results the timestamp to roll over every 2,147,483,647 miliseconds, while the RTMP specification says it rolls over every 4,294,967,295 milliseconds.

Maybe this could be an off-topic, but I think a timestamp in RTMP does not have to start from zero as long as it rolls over at 4,294,967,295 milliseconds, though doing so would improve compatibility with broken implementations.

[jeongki-kim]

Even if OBS is going to count RTMP timestamp from zero when it starts streaming, there still might be a slight possibility that one might keep streaming longer than about 50 days, which the timestamp will overflow eventually.

Adobe RTMP Specification(HTML ver, PDF ver) covers such situation:

Because timestamps are 32 bits long, they roll over every 49 days, 17 hours, 2 minutes and 47.296 seconds. Because streams are allowed to run continuously, potentially for years on end, an RTMP application SHOULD use serial number arithmetic [RFC1982] when processing timestamps, and SHOULD be capable of handling wraparound. For example, an application assumes that all adjacent timestamps are within 2^31 - 1 milliseconds of each other, so 10000 comes after 4000000000, and 3000000000 comes before 4000000000.

Also I suspect the following code of librtmp will not work as expected, if the MSB is dropped.

obs-studio/plugins/obs-outputs/librtmp/rtmp.c

Line 4134 in 2fa77c4

uint32_t delta = packet->m_nTimeStamp - prevPacket->m_nTimeStamp;

[RytoEX]

With #10999 merged, is this still needed?

[jeongki-kim]

I think so because this is not only related to the initial timestamp, which was already handled by #10999, but also covers RTMP timestamp overflow.

Discarding the MSB, which is the identical bit that represents signedness in case of signed integers, limits its maximum value up to (2^31)-1 (==0x7FFF FFFF), and therefore results the timestamp resets to 0 after 0x7FFF FFFF. This behavior does not comply RTMP specification as it specifies that 0 comes after 0xFFFF FFFF.

Only few users would be affected because it matters when they streams over 0x7FFF FFFF miliseconds or about 24 days approximately. If you don't think it counts, this PR can be closed without merging.