1339246 - When switching from sendrecv to recvonly to sendrecv, FF sends RTP with the previous SSRC of the same m= line rather than the new one

Status: VERIFIED FIXED

(Core :: WebRTC: Networking, defect, P1)

Description

[Iñaki Baz Castillo]

Scenario:

Within an already established audio&video session, Firefox removes its local video, renegotiates, creates a new local video track, adds it to its PeerConnection and renegotiates again.


Issue:

Firefox's initial SDP had SSRC XXXX for its sending video track. When the video track is removed and a new one added later, the new SDP clearly contains a new SSRC YYYY for the video track (and Firefox sends video with such a new SSRC YYYY).

But, when it comes to send RTCP Sender Reports for the new video track, Firefox sends them with SSRC XXXX instead of SSRC YYYY. So, the Sender Report cannot be matched by the remote endpoint.


Notes:

* This is the same scenario as the one described in issue https://bugzilla.mozilla.org/show_bug.cgi?id=1339234, but forcing VP8 PT to 120 to make Firefox happy during renegotiations. This detail is important because the m= section used to send the new local video track is the SAME one as the m= section previously used to send the first local video track.

* May be related to issue 1335469 (no idea).

Comment 1

[Iñaki Baz Castillo]

The issue is much worse than what I initially reported: Firefox is announcing a sending SSRC but it's sending with the previous one.


To be clear, this is the initial SDP m=video sending section of Firefox:

m=video 9 RTP/SAVPF 123
c=IN IP4 0.0.0.0
a=sendonly
a=ice-pwd:a1667113c7d57af7263daf9f7b39bbb2
a=ice-ufrag:5181df4d
a=mid:recv-video-track-1
a=msid:{c5fbcb25-d801-bd44-a2ac-4fff71307e37} {bd9706b7-20b0-9741-9e6b-8bca7964d23f}
a=rtcp-fb:123 nack
a=rtcp-fb:123 nack pli
a=rtcp-fb:123 ccm fir
a=rtcp-mux
a=rtpmap:123 VP8/90000
a=setup:active
a=ssrc:133618545 cname:{0b2b71da-1d8e-0849-8422-8240da681a05}


And this is the same SDP m=video section after removing the previous local video track, adding a new one (or the same) into the PeerConnection and calling createOffer():

m=video 9 RTP/SAVPF 123
c=IN IP4 0.0.0.0
a=sendonly
a=ice-pwd:a1667113c7d57af7263daf9f7b39bbb2
a=ice-ufrag:5181df4d
a=mid:recv-video-track-1
a=msid:{c5fbcb25-d801-bd44-a2ac-4fff71307e37} {059b31c4-6e01-6840-96b6-d1eecf6f3123}
a=rtcp-fb:123 nack
a=rtcp-fb:123 nack pli
a=rtcp-fb:123 ccm fir
a=rtcp-mux
a=rtpmap:123 VP8/90000
a=setup:active
a=ssrc:4293051699 cname:{0b2b71da-1d8e-0849-8422-8240da681a05}


It's clear that the new video track should be sent with SSRC 4293051699. However, Firefox is still using the previous SSRC 133618545 (I've confirmed it via network traces).

So Firefox is announcing a sending SSRC but it uses other one (the first sending SSRC used in the same m= section).

Comment 2

[Iñaki Baz Castillo]

BTW: It does NOT fail in Firefox stable 51.0.1 (OSX).

Comment 3

[Iñaki Baz Castillo]

You can reproduce the issue here:

https://demo.mediasoup.org/

Just join with Chrome and Firefox stable and play removing/re-adding local video. It works. Then do the same with Nightly. When re-adding the local video (which in fact is a new local video track) it won't be received by the others. This is because it announces a new SSRC in the SDP but still uses the previous one for the RTP packets and SenderReports.

Comment 4

[Byron Campen [:bwc]]

Maybe this is fallout from the webrtc.org update?

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

very likely.

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Attachment: force rebuild of SendStream if the SSRCs have been changed

Note that while this seems to fix things for video, I got a crash in MSG at dom/media/MediaStreamGraph.cpp:2351 (assertion on mInputStream)

Comment 7

[Byron Campen [:bwc]]

Comment on attachment 8844623 [details] [diff] [review]
force rebuild of SendStream if the SSRCs have been changed

Review of attachment 8844623 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +280,5 @@
>      }
> +  } else {
> +    // On the next StartTransmitting() or ConfigureSendMediaCodec, force
> +    // building a new SendStream to switch SSRCs
> +    mForceNewSendStream = true;

So we call DeleteSendStream above if we were transmitting already; if we always called DeleteSendStream, would that do the trick all by itself?

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

We don't want to destroy it always - on StartTransmitting() or on a renegotiate that doesn't have any incompatible bits we want to just ReconfigureVideoEncoder() (see RequiresNewSendStream()).  We call StopTransmitting() when reconfiguring; forcing destroy in these cases would force everything to re-initialize from scratch (and re-init bandwidth estimation, etc, etc)

I'd *love* to modify VideoSendStream/etc to allow SetLocalSSRCs() to avoid a full teardown in ssrc-change cases, but that's not there and is a bunch of (upstream) work.

Comment 9

[Byron Campen [:bwc]]

(In reply to Randell Jesup [:jesup] from comment #8)
> We don't want to destroy it always - on StartTransmitting() or on a
> renegotiate that doesn't have any incompatible bits we want to just
> ReconfigureVideoEncoder() (see RequiresNewSendStream()).  We call
> StopTransmitting() when reconfiguring; forcing destroy in these cases would
> force everything to re-initialize from scratch (and re-init bandwidth
> estimation, etc, etc)
> 
> I'd *love* to modify VideoSendStream/etc to allow SetLocalSSRCs() to avoid a
> full teardown in ssrc-change cases, but that's not there and is a bunch of
> (upstream) work.

So we bail out of that function early if the SSRCs have not changed, so a renegotiation that doesn't have anything incompatible should not get that far, right? As for the initial configuration, setting the local SSRCs is the first thing we do anyway, so we shouldn't end up stomping anything...

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Attachment: force rebuild of SendStream if the SSRCs have been changed

MozReview-Commit-ID: 5RNrkBZFmV3

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e90c48f2617
force rebuild of SendStream if the SSRCs have been changed r=bwc

Comment 12

[Iris Hsiao [:ihsiao]]

backed this out for media test failures in test_peerConnection_addtrack_removetrack_events.html like https://treeherder.mozilla.org/logviewer.html#?job_id=82357136&repo=mozilla-inbound&lineNumber=10862

https://hg.mozilla.org/integration/mozilla-inbound/rev/a308f1d04a74

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Attachment: force rebuild of Send/RecvStream if the SSRCs have been changed

Turns out we had the exact same sort of bug on the receive side...

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

Green Try: https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&selectedJob=82384643

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c4489a633ed
force rebuild of Send/RecvStream if the SSRCs have been changed r=bwc

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7c4489a633ed

Comment 17

[Iñaki Baz Castillo]

Thanks a lot guys. I'll test it once it lands on Nightly 55.

Comment 18

[Iñaki Baz Castillo]

Just upgraded to Firefox Nightly 55.0a1 (2017-03-08). The fix does not seem to be included in it (makes sense as the build is from yesterday day 8th). May I know which build will include the fix?

Comment 19

[Iñaki Baz Castillo]

Good work, guys. It works now in Firefox Nightly 55.0a1 (2017-03-09).

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora/Beta approval on this when you get a chance.

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8844790 [details] [diff] [review]
force rebuild of Send/RecvStream if the SSRCs have been changed

Approval Request Comment
[Feature/Bug causing the regression]: webrtc 49 landing

[User impact if declined]: various renegotiations will result in failed calls

[Is this code covered by automated tests?]: Not directly

[Has the fix been verified in Nightly?]: yes

[Needs manual test from QE? If yes, steps to reproduce]:  no, tested by us and by reporter

[List of other uplifts needed for the feature/fix]: none

[Is the change risky?]:  no

[Why is the change risky/not risky?]: uses normal re-create-streams flow; just makes sure that we invoke it in this case.

[String changes made/needed]: none

Comment 22

[Gerry Chang [:gchang]]

Comment on attachment 8844790 [details] [diff] [review]
force rebuild of Send/RecvStream if the SSRCs have been changed

Fix a webrtc issue related to renegotiations and was verified in nightly. Beta53+ & Aurora54+.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3d46587f5dbe

Comment 24

[Carsten Book [:Tomcat]]

needs rebase for beta

grafting 403982:3d46587f5dbe "Bug 1339246: force rebuild of Send/RecvStream if the SSRCs have been changed r=bwc a=gchang"
merging media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
merging media/webrtc/signaling/src/media-conduit/VideoConduit.h
warning: conflicts while merging media/webrtc/signaling/src/media-conduit/VideoConduit.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging media/webrtc/signaling/src/media-conduit/VideoConduit.h! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 25

[Randell Jesup [:jesup] (needinfo me)]

Thanks

Conflicts are due to bug 1337777 not being landed on 53.  I put it up for 53 now; if I get approval this patch should apply without changes on top of that.  I'll merge this to beta once we resolve that one

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6bdac526f998

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Bug 1337777 needed to be backed out, so this was as well.

https://hg.mozilla.org/releases/mozilla-beta/rev/9e4af5282696

Comment 28

[Iñaki Baz Castillo]

Amazing work.

Comment 29

[Randell Jesup [:jesup] (needinfo me)]

 https://hg.mozilla.org/releases/mozilla-beta/rev/65c3d718dd5fb6f11912820a5433ff6d13e6362f

Comment 30

[Andrei Vaida [:avaida]]

Setting qe-verify- based on Jesup's assessment on manual testing needs (see Comment 21).