1965960 - {Audio|Video}Conduit process packets for inactive receivers

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[Andreas Pehrson [:pehrsons]]

See Pernosco.

As seen on https://networktest.twilio.com per bug 1965831, Twilio triggers some signaling that hits a very (I believe) narrow corner case on our end, where an inactive video recv track processes packets in both MediaPipeline and VideoConduit before hitting the end of the road in libwebrtc, and by doing so causes another, active, video recv track to stop processing those packets destined for it.

Nowadays there's a bit of a model mismatch between our packet filtering and libwebrtc's. Our filtering is meant to route packets only to the matching video conduit, because that's how libwebrtc wanted to be served packets back in the day. They have since moved on and now employ their own packet filtering that routes packets to the correct receive stream. Because of this we instead feed received packets, after they have been routed to the correct conduit, into the webrtc::Call instance (mapping 1-1 to RTCPeerConnection on our end).

Somehow, we don't yet understand fully how the regression is activated, Twilio's signaling and bug 1949282 unmasks a bug in our stack, where an inactive receiver (and its inactive MediaPipeline and non-receiving VideoConduit) processes packets. Other parts of our code doesn't expect this to happen, leading to the duplicate-PT logic in our packet filter being bypassed.

What makes this more interesting is that Twilio's signaling has set up the inactive receiver without any SSRC associations, whereas the active receiver from signaling knows its SSRC association. Because the inactive receiver doesn't have any associations, it will bind to the first SSRC that passes through its filter, whose duplicate-PT logic isn't working (because there are two receivers receiving packets, one could argue there are duplicate PTs present, as both will accept PT 120 for instance). The inactive receiver effectively ends up binding to the active receiver's already-bound SSRC, and when that happens, it tells all other receivers to give up that SSRC. At this point, the active receiver will no longer process packets, because it is no longer bound to the SSRC it was signaled for.

Intricate? Yes! But at least the fix is simple.

Comment 1

[Dan Baker[:dbaker]]

Attachment: Bug 1965960 - Dont set unique PTs for tracks that are inactive and ignore packets received in conduits that are not receiving.;r?pehrsons,bwc

Comment 2

[Dan Baker[:dbaker]]

Bug 1949282 introduced this issue most likely due to the change to update the filter when we are creating offers. There was no check put in place to ensure the track was actually active. So in the past with twilio our MediaPipelineFilter would have discarded the packets thus not sending them to the conduit. After the changes in bug 1949282 it was incorrectly setting up the filter with unique PTs even though it was inactive exposing the issue in the conduits. We are trying to only update our unique PTs for the MediaPipelineFilter when it is active AND changing the OnRtpReceived in the conduits to not handle packets if it is not configured to receive.

Comment 3

[Andreas Pehrson [:pehrsons]]

Let's make this bug about the conduits, and use it for a minimal patch that can be uplifted.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4ea9cfc3e584
Dont set unique PTs for tracks that are inactive and ignore packets received in conduits that are not receiving.;r=pehrsons

Comment 5

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/4ea9cfc3e584

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta uplift ASAP.

Comment 7

[Dan Baker[:dbaker]]

Comment on attachment 9487173 [details]
Bug 1965960 - Dont set unique PTs for tracks that are inactive and ignore packets received in conduits that are not receiving.;r?pehrsons,bwc

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: This resolves several issues with sites using Twilio video SDK.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Different issues could arise from unknown call signaling that we do not have testing for and that is different from current tests.
• String changes made/needed:
• Is Android affected?: Yes

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9487173 [details]
Bug 1965960 - Dont set unique PTs for tracks that are inactive and ignore packets received in conduits that are not receiving.;r?pehrsons,bwc

Approved for 139.0rc1.

Comment 9

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-release/rev/97deb4be34cf