1874471 - Possible race condition when demuxing video packets of new transceiver

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Taylor Brandstetter]

Attachment: mozlog_demux_error.txt

Steps to reproduce:

1. Set up a RTCPeerConnection with a sendonly video transceiver and perform an offer/answer exchange.
2. Apply a remote description adding a new recvonly video m= section containing an a=ssrc line (MID and RID header extensions not in use, demuxing by SSRC only). In the attached log this is SSRC 2804095052.
3. At the same time, or sooner, packets from this SSRC are arriving from the remote endpoint.

Actual results:

There appears to be a race condition when setting up the receive stream. From the attached log:

1. VideoConduit 7f1d6cd24900 (presumably representing the recvonly transceiver) sets up the receive stream as expected for SSRC 2804095052
2. VideoConduit 7f1d7b51fc00 (presumably the sendonly transceiver) processes a packet from SSRC 2804095052, and due to code that handles the "unknown ssrc (and ssrc-not-signaled case)", adopts this as its new remote SSRC
3. This unsets 2804095052 from VideoConduit 7f1d6cd24900, which generates a new remote SSRC.
4. VideoConduit 7f1d6cd24900 now fails to demux packets from 2804095052.

My guess is that, while there's only one video m= section, all packets are routed to it in order to handle unknown/unsignaled SSRCs. This presumably changes when the new remote description is applied containing a new video m= section, but packets may still be queued up in the first VideoConduit, which when processed unset the SSRC of the VideoConduit which is actually meant to be handling that SSRC.

We had the same exact problem in Chrome, which was quite a pain to work around: https://source.chromium.org/chromium/_/webrtc/src.git/+/15e078c574981597c5d6ecc13476f54e667dc568

It looks like it can be avoided by making sure there's never exactly one video m= section, but adding a superfluous m= section seems to be causing other issues which I'm still looking into.

Expected results:

Demuxing by SSRC works as expected.

Comment 1

[Taylor Brandstetter]

Just wanted to clarify, in #2, the m= section is sendonly in the remote description, recvonly in the local description.

Comment 2

[Michael Froman [:mjf]]

Byron, any thoughts here?

Comment 3

[Byron Campen [:bwc]]

... so this is a scenario that uses bundle, which requires the use of SDP mid, but does not use RTP mid?

At the risk of being labelled a smart-aleck:

Patient: Doctor, it hurts when I do this!
Doctor: Then don't do that.

More seriously, I think that we must be seeing early arrival of that new RTP stream. That causes the filter for the first conduit to see a unique payload type, and learn the SSRC. Then, when we negotiate, the filter for the second conduit also will let that SSRC through. Lastly, because we're doing bundle without the MID RTP extension, we disable the ssrc switchover logic, because if we don't we'll just get random assignment with unsignaled SSRCs (this is what prevents us getting into an infinite back-and-forth ssrc switching).

I suppose we could add a "filter out everything because you're not negotiated to receive" bit on the filters.

Comment 4

[Taylor Brandstetter]

No, I agree, we should definitely be using the RTP MID extension. Unfortunately, given how things currently work, that's easier said than done...

Comment 5

[Byron Campen [:bwc]]

Ok, I think I'm going to do a couple of things:

1. Disregard the negotiated payload types of non-receiving m-sections for the purposes of unique payload type filtering.
2. If a filter knows an ssrc, either from negotiation or learning it from RTP, we disable payload type matching for that filter. This will avoid incorrectly learning a new m-section's ssrc if the RTP arrives early.

Comment 6

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/jobs?repo=try&revision=9dd96fa754f6e060515f4aa10c71b37e375a50b7
https://treeherder.mozilla.org/jobs?repo=try&revision=a6cdfecb18a45545e06710ae2ff4311b6181429f
https://treeherder.mozilla.org/jobs?repo=try&revision=18fa9bdcf9e6702b5fa11b3fbf8ede9772b9d21d

Comment 7

[Byron Campen [:bwc]]

Attachment: Bug 1874471: Only use receive payload types in MediaPipelineFilter. r?mjf

Comment 8

[Byron Campen [:bwc]]

Attachment: Bug 1874471: Disable payload-type matching for filters that know an ssrc. r?mjf

Depends on D201766

Comment 9

[Byron Campen [:bwc]]

Try looks good.

Could you grab a binary from one of those try pushes and see if it addresses the issue?

Comment 10

[Taylor Brandstetter]

I'm not sure what I'm missing, but I'm having trouble reproducing the issue in an isolated way. I have a certain integration test which reproduces it (at least 10% of the time), but am not able to run that on a custom browser... Would it be ok for me to verify when the next release comes around?

Comment 11

[Byron Campen [:bwc]]

I'm a little bit leary of landing a non-verified behavioral change the day before a soft freeze, so maybe this will go into the next nightly. If that helps you substantially, I can try an early uplift.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d26e5bcff843
Only use receive payload types in MediaPipelineFilter. r=mjf
https://hg.mozilla.org/integration/autoland/rev/a7847a74255d
Disable payload-type matching for filters that know an ssrc. r=mjf

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/d26e5bcff843
https://hg.mozilla.org/mozilla-central/rev/a7847a74255d