Prepare RTCP Sender Reports by considering the last RTP timestamp sent.

[atoppi]

CONTEXT
Janus periodically sends a RTCP Sender Report every 1 second.
When calculating the RTP timestamp to include in that report it does the following:

gettimeofday(&tv, NULL);
int64_t ntp = tv.tv_sec*G_USEC_PER_SEC + tv.tv_usec;
uint32_t rtp_ts = ((ntp-stream->audio_first_ntp_ts)*(rtcp_ctx->tb))/1000000 + stream->audio_first_rtp_ts;

stream->audio_first_ntp_ts and stream->audio_first_rtp_ts are respectively:

• the NTP timestamp of the first sent packet
• the RTP timestamp of the first sent packet

So basically Janus keeps memory of the first packet sent to an endpoint, then every 1 second calculates how much time has passed since that moment and it converts that time difference in a RTP timestamp difference. That value is the final SR timestamp.

PROBLEM
The first sample pair (stream->audio_first_ntp_ts and stream->audio_first_rtp_ts) that Janus uses to calculate future RTCP SR may be affected by an error due to a very large number of reasons (like unexpected network delay, temporary RTP encoder issue on the sender's side and so on).
Once Janus has stored a wrong starting point, it will basically introduce a constant error in the future SR, confusing the RTP decoders that will try to compensate the associated media track.
In addition to this constant error, another worse issue might happen: in scenarios where the RTP clock of a sender is not honouring the nominal value (e.g. very high CPU load, clock drift etc.) and the SR sent by the sender itself would be affected by the same problem, Janus will continue to send RTCP SR to the receiver according to its own clock, again confusing the RTP decoder on the playout time of the media.
This described behavior may explain some of the lipsync problems that we have recently observed, including the still open #1653 .

RATIONALE
The core concept here is that Janus, being a SFU, will never be a media source and so it should not generate the RTCP by itself with its own local clock. Instead it should strictly follow the RTP timestamps that it is delivering, so that any clock issue on the sender's side would be acknowledged and compensated by the recipient. Trying to "fix" the RTCP SR clock, leaving the RTP clock as it is, will just confuse the RTP decoder and potentially lead to the aforementioned issues.

FIX
The proposed fix is to generate the RTCP SR timestamps by evaluating the elapsed time since the last sending, converting that elapsed time in RTP timestamp units and then add the difference to the last RTP timestamp sent.

gettimeofday(&tv, NULL);
int64_t ntp = tv.tv_sec*G_USEC_PER_SEC + tv.tv_usec;
uint32_t rtp_ts = ((ntp-stream->video_last_ntp_ts)*(rtcp_ctx->tb))/1000000 + stream->video_last_rtp_ts;

Incidentally, it looks like mediasoup is taking the same approach. I'd like to hear what @ibc thinks about this.

[ibc]

Makes lot of sense IMHO.

[lminiero]

I personally don't like this approach at all: it basically means we're letting whatever is sending us media have control of what we'll send via SR. If they're sending pure junk, our SR messages will be junk too, causing at the very least very confusing, if not completely broken, RTT estimation on the other side for instance. No matter how broken the stream is, it is my opinion that our SR should be proper: that's why I was a bit surprised to hear mediasoup follows this new approach too, any info on the rationale @ibc? I thought you guys were doing some other computation on media sources.

That said, I agreed to share this effort anyway and gather feedback. Who knows, maybe it will indeed improve the audio/video sync issues some have experienced, and I'll be forced to live with an ugly hack in the code.

[alexamirante]

As I already told you in a private conversation, my feeling is that since an SFU does not generate media by itself but just forwards them, SR it sends on the subscribers legs should carry some information about the original publisher. If this publisher sends pure junk, SR must inform receivers about that. Otherwise you would fool them by saying everything is ok while it is clearly not.

[jmillan]

@atoppi, I don't find a better way to do it than the one we have adopted in mediasoup.

We are using the RTP TS of the last received RTP packet, in case this has the highest sequence number. We store it as maxPacketTs, we record the moment at which we receive it as maxPacketMs.

When it's time for us to generate a RTCP SR we use maxPacketTs and add the difference between now and maxPacketMs in RTP TS units.

This is not perfect since we are not considering network delay fluctuations, so there must be place for improvement.

[atoppi]

Hi @jmillan, thanks for confirming mediasoup implementation.
The only difference is that we are using the latest sending time as maxPacketMs.

[ibc]

@lminiero: the client can send junk anyway, for instance in the RTP. I don't think the SFU should try to fix that.

The problem I see in current Janus implementation (as the original issue description describes) is that it considers the first received RTP packet as the source of truth. If for instance the first video packet arrived late (due to network congestion when the client started sending media) you'll keep that delay forever in the receiver.

This is not perfect since we are not considering network delay fluctuations,

@jmillan correct me if I'm wrong: If network delay fluctuations happens, mediasoup will eventually accommodate to them when it stabilizes, so receivers will get audio and video in sync.

[jmillan]

@jmillan correct me if I'm wrong: If network delay fluctuations happens, mediasoup will eventually accommodate to them when it stabilizes, so receivers will get audio and video in sync.

We are relying on the RTP packet arrival time to the SFU, which make us not resilient to network conditions.

In summary, we should generate RTCP SR packets representing the original (encoder) time source that was used to generate the RTP packets.

Currently:

Encoder (RTP packet generation with TS) --> Network --> SFU (RTP TS and current time association)

Instead of relating TS to MS at the moment the packet arrives to SFU, lets remove the network fluctuations. An approach coming to my mind in order to do so is making use of abs-send-time RTP header extension. Which represents the moment where the packets leaves the encoder machine.

That moves the scenario into:

Encoder (RTP packet generation with TS) --> Network --> SFU (RTP TS and RTP abs-send-time value)

Still this is not perfect because the RTP packet generation in the encoder (and hence TS value) and the time it leaves the machine may also have certain fluctuation. But we get closer to the source clock..

I believe this is a great enhancement. Thoughts?

NOTE: In mediasoup we only record the MS for the packet with the highest RTP TS, considering field wraps up, of course.

[jmillan]

In order to be accurate, additionally using of rtp-hdrext:toffset header extension (for video) will give us the exact MS time realted to the TS.

[ibc]

@jmillan:

Instead of relating TS to MS at the moment the packet arrives to SFU, lets remove the network fluctuations

We cannot do this for audio (if there is no abs header) since we'll never know which received packet to consider the source of truth.

For video it may make sense to use those headers (if present). However, if we apply different TS calculation to audio and video, we'll end generating lipsync. Am I wrong?

[atoppi]

@jmillan @ibc I think we should consider the arrival time of the highest RTP timestamp seen (not seq num or simply latest packet like i am doing). For example in video frames the rtp ts is the same for all rtp packets, seq num is incremented, but the time marking (what you call maxPacketMs) should be done on the first RTP packet of the same frame, because its arrival time is the closest to the frame generation time.

[jmillan]

@jmillan @ibc I think we should consider the arrival time of the highest RTP timestamp seen (not seq num or simply latest packet like i am doing).

Indeed. This would be an improvement over the current implementation.

[jmillan]

Ups, we already to that in mediasoup :-):

https://github.com/versatica/mediasoup/blob/v3/worker/src/RTC/RtpStream.cpp#L139

[jmillan]

I mean, we are considering the TS and not the SeqNum

[jmillan]

We cannot do this for audio (if there is no abs header) since we'll never know which received packet to consider the source of truth.

Of course I'm considering abs-send-time is being used, otherwise we cannot do better that we do now IMHO.

For video it may make sense to use those headers (if present). However, if we apply different TS calculation to audio and video, we'll end generating lipsync. Am I wrong?

Yes, we need to calculate audio and video TS the same way.

[ibc]

Right now, in mediasoup we disable REMB if Transport-CC is available. We don't negotiate abs-send-time if transport-wide is available.

[lminiero]

Right now, in mediasoup we disable REMB if Transport-CC is available. We don't negotiate abs-send-time if transport-wide is available.

There's no harm in keeping them both active. We sometimes use REMB as a cap, and then transport-cc computes bitrates that never exceed those values.

[ibc]

I've should have been more precise. We do cap via REMB even if Transport-CC is in use. It's just that we don't need abs-send-time exten for that.

[tmatth]

@atoppi @lminiero FWIW, with this PR we are seeing a drastic improvement for WebRTC-publisher-to WebRTC-client streams (e.g. videoroom type), what was before sometimes on the order of seconds of AV desync is now closer to 75ms.

[michaelshiel]

Hard to quantify, but I am not seeing necessarily better or worse performance than previously. I haven't really noticed de-sync issues in the past, but this certainly didn't make anything worse performance-wise.

[cb22]

@atoppi I can confirm what @tmatth said. With this change in a videoroom, AV sync appears nearly perfect in both simulated and real poor network conditions, whereas previously we'd often get large amounts of drift (also, on the order of a few seconds).

[lminiero]

Looks like it's helping, so I'll merge.