fix(rtp): improve timestamp accuracy

[andygrundman]

Background

I am experimenting with the iOS AVSampleBufferAudioRenderer and AVSampleBufferRenderSynchronizer APIs, which include the ability to use RTP timestamps as a way to pace playback, provide a/v sync that won't drift, use variable-speed playback to cover for short glitches (such as Game Mode's drop in Bluetooth latency), proper spatial audio support for free, and more. It should also be nicely compatible with the AVSampleBufferDisplayLayer API currently used by the iOS video code. The biggest challenge with these APIs is their higher latency, but I'm hopeful it can get to a reasonable level. Worst case, it could be a good option when you are willing to trade a little latency for solid playback.

Description

• Video: instead of using now() when the RTP packet is created, use the earlier packet->frame_timestamp that we're already collecting for host latency stats. This timestamp should be more accurate to when we captured the frame. I am not sure if all backends support this timestamp, so there is a fallback to the current method.
• Audio: fix a bug where the RTP timestamps stop advancing when no audio is being sent. Audio now uses the same timer-based source for this field, but remains as milliseconds. Because of this bug, I am pretty sure no client is using these timestamps for anything.
• Both use steady_clock and microseconds / 1000 to maintain precision. Video still uses 90khz time base per the spec, and audio is effectively at packet duration resolution of 5ms/10ms.

Known issue: I had to comment out one assert in Moonlight, because this patch changes one of the FEC timestamps. The FEC RFC says "The timestamp SHALL be set to a time corresponding to the repair packet's transmission time. Note that the timestamp value has no use in the actual FEC protection process and is usually useful for jitter calculations.", so I don't think this assert is correct anyway. Even though this shouldn't affect Moonlight release builds which don't use asserts or the extra FEC verification code, this probably needs to be resolved before this can be accepted, either by only sending correct timestamp packets to newer Moonlight versions, or fixing it in the FEC code. (Not to get too sidetracked but was the Opus FEC support ever tried? I sometimes wonder if the 40% FEC overhead on audio is too high for benefit.)

RtpAudioQueue.c:300
LC_ASSERT_VT(existingBlock->fecHeader.baseTimestamp == fecBlockBaseTs);

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Dependency update (updates to dependencies)
• Documentation update (changes to documentation)
• Repository update (changes to repository files, e.g. .github/...)

Checklist

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

[andygrundman]

Oops, I changed the title in the commit, but forgot to do the PR. I guess it's still too long anyway:

fix(rtp): maintain audio timestamp continuity even during periods of silence. Measure video timestamps closer to the time of capture by reusing host latency's frame_timestamp.

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 14 lines in your changes missing coverage. Please review.

Project coverage is 6.18%. Comparing base (9b9767b) to head (da59b33).

Files with missing lines	Patch %	Lines
src/stream.cpp	0.00%	14 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           master   #3513      +/-   ##
=========================================
- Coverage    8.44%   6.18%   -2.27%     
=========================================
  Files          90      66      -24     
  Lines       16069   11989    -4080     
  Branches     7633    5584    -2049     
=========================================
- Hits         1357     741     -616     
+ Misses      14214   11072    -3142     
+ Partials      498     176     -322     

Flag	Coverage Δ
Linux	?
Windows	6.18% <0.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/stream.cpp	2.49% <0.00%> (-0.02%)	⬇️

... and 49 files with indirect coverage changes

[cgutman]

Because of this bug, I am pretty sure no client is using these timestamps for anything.

I believe that's accurate. The clients that I know of that try to pace audio do it via monitoring the amount of pending audio (either in moonlight-common-c's queue, in the OS audio stack, or both).

Known issue: I had to comment out one assert in Moonlight, because this patch changes one of the FEC timestamps. The FEC RFC says "The timestamp SHALL be set to a time corresponding to the repair packet's transmission time. Note that the timestamp value has no use in the actual FEC protection process and is usually useful for jitter calculations.", so I don't think this assert is correct anyway.

AFAIK, Nvidia did their own thing and didn't implement any known RTP FEC standard, so it's asserting what GFE and current Sunshine do.

IIUC, what is happening in your case is that you hit the (common) case where a data packet is received first and we synthesize an FEC header using this code:

        // This is a data packet, so we will need to synthesize an FEC header
        fecBlockPayloadType = packet->packetType;
        fecBlockBaseSeqNum = (packet->sequenceNumber / RTPA_DATA_SHARDS) * RTPA_DATA_SHARDS;
        fecBlockBaseTs = packet->timestamp - ((packet->sequenceNumber - fecBlockBaseSeqNum) * AudioPacketDuration);
        fecBlockSsrc = packet->ssrc;

Since you've adjusted the logic to no longer guarantee that there is AudioPacketDuration milliseconds between the timestamps in each packet, the synthesized FEC header won't always match the real thing that we will get later. If this an accurate description of what you saw going on, then I think removing the assert is fine.

Even though this shouldn't affect Moonlight release builds which don't use asserts or the extra FEC verification code, this probably needs to be resolved before this can be accepted, either by only sending correct timestamp packets to newer Moonlight versions, or fixing it in the FEC code.

Did you have some idea of how you would "fix it"? AFAICT, it seems basically intrinsic to this model where timestamps are real measured things and not just a glorified counter.

Not to get too sidetracked but was the Opus FEC support ever tried? I sometimes wonder if the 40% FEC overhead on audio is too high for benefit.

I know Nvidia didn't use it. Given how tiny of a fraction that audio data is of the total traffic, it probably made sense to use the existing Reed-Solomon FEC scheme they were already using for video. They didn't even bother with a variable FEC scheme for audio, unlike video FEC which can change based on client feedback.

[cgutman]

I think it's possible to get capture time from the OS too like we do with video frames. IAudioCaptureClient::GetBuffer() takes an optional pu64QPCPosition parameter that gives you the timestamp of capture.

[andygrundman]

Oh nice, that seems useful and we might as well use it, even though the difference between this and the timestamp I put in the patch is probably so small to be meaningless. I took a crack at adding this in, but got as far as adding a packet_timestamp into auto packets = mail::man->queue<audio::packet_t>(mail::audio_packets); and then got stumped by those brutal C++ tuple macros and fun compiler errors. Anyway it's here if one of you guys can spot the right way to get that timestamp through the queue.

andygrundman@0767e00

[sonarqubecloud]

Quality Gate failed

Failed conditions
9 New issues
9 New Code Smells (required ≤ 0)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

[andygrundman]

This ended up getting a bit insane due to the layering of the queues and the need to refactor every layer to deal with passing additional metadata. I don't really know how I feel about any of this now, it was supposed to be a simple patch... I'm not sure I have it in me to deal with the SonarQube nonsense it will complain about.

Some things I noticed:

• On my Windows system, audio capture is in 10ms chunks. Does this mean the tOpus packet size of 5ms provides no benefit? I tried the buffer duration parameter in Initialize() but I don't think a capturing program can influence this anyway. Matching the Opus packet size might mean less glitches and lower resource usage on the client side. I need to do more testing.
• There is a variable called default_latency_ms that has a math bug and is 10x the correct value. Caused by the weird unit in Windows that is 100ns (e.g. need to divide by 10000 for ms). It's only used for the timeout to a WaitForSingleObjectEx() API call so I think it's harmless.

[ReenigneArcher]

Sonar is only a tool, not an absolute deal breaker or blocker if there are failures.