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

VERIFIED FIXED in Firefox 53

Status

()

Core
WebRTC: Networking
P1
normal
Rank:
15
VERIFIED FIXED
a year ago
11 months ago

People

(Reporter: Iñaki Baz Castillo, Assigned: jesup)

Tracking

({regression})

54 Branch
mozilla55
regression
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox52 unaffected, firefox-esr52 unaffected, firefox53 fixed, firefox54 fixed, firefox55 verified)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

a year ago
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).
(Reporter)

Updated

a year ago
Summary: After re-adding a track and re-negotiating, Firefox sends RTCP Sender Report with the previous SSRC → After re-adding a track and re-negotiating, Firefox sends RTP with the previous SSRC of the same m= line rather than the new one
(Reporter)

Comment 1

a year ago
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).
(Reporter)

Comment 2

a year ago
BTW: It does NOT fail in Firefox stable 51.0.1 (OSX).
(Reporter)

Updated

a year ago
Summary: After re-adding a track and re-negotiating, Firefox sends RTP with the previous SSRC of the same m= line rather than the new one → [regression] After re-adding a track and re-negotiating, Firefox sends RTP with the previous SSRC of the same m= line rather than the new one
(Reporter)

Updated

a year ago
Has Regression Range: --- → yes
Rank: 15
Priority: -- → P1
(Reporter)

Comment 3

a year ago
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.
Maybe this is fallout from the webrtc.org update?
Flags: needinfo?(rjesup)
(Assignee)

Comment 5

a year ago
very likely.
Flags: needinfo?(rjesup)
(Assignee)

Comment 6

a year ago
Created attachment 8844623 [details] [diff] [review]
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)
Attachment #8844623 - Flags: review?(docfaraday)
(Assignee)

Updated

a year ago
Assignee: nobody → rjesup
Status: NEW → ASSIGNED
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?
Attachment #8844623 - Flags: review?(docfaraday)
(Assignee)

Comment 8

a year ago
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.
(Assignee)

Updated

a year ago
status-firefox53: --- → affected
status-firefox55: --- → affected
Keywords: regression
Summary: [regression] After re-adding a track and re-negotiating, Firefox sends RTP with the previous SSRC of the same m= line rather than the new one → 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
(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...
(Assignee)

Comment 10

a year ago
Created attachment 8844638 [details] [diff] [review]
force rebuild of SendStream if the SSRCs have been changed

MozReview-Commit-ID: 5RNrkBZFmV3
Attachment #8844638 - Flags: review?(docfaraday)
(Assignee)

Updated

a year ago
Attachment #8844623 - Attachment is obsolete: true

Updated

a year ago
Attachment #8844638 - Flags: review?(docfaraday) → review+

Comment 11

a year ago
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e90c48f2617
force rebuild of SendStream if the SSRCs have been changed r=bwc
(Assignee)

Updated

a year ago
Blocks: 1250356
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
Flags: needinfo?(rjesup)
(Assignee)

Comment 13

a year ago
Created attachment 8844790 [details] [diff] [review]
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...
Attachment #8844790 - Flags: review?(docfaraday)

Updated

a year ago
Attachment #8844790 - Flags: review?(docfaraday) → review+

Comment 15

a year ago
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c4489a633ed
force rebuild of Send/RecvStream if the SSRCs have been changed r=bwc

Comment 16

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7c4489a633ed
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Reporter)

Comment 17

a year ago
Thanks a lot guys. I'll test it once it lands on Nightly 55.
(Reporter)

Comment 18

a year ago
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?
(Reporter)

Comment 19

a year ago
Good work, guys. It works now in Firefox Nightly 55.0a1 (2017-03-09).
Please request Aurora/Beta approval on this when you get a chance.
Status: RESOLVED → VERIFIED
status-firefox52: --- → unaffected
status-firefox55: fixed → verified
status-firefox-esr52: --- → unaffected
Flags: needinfo?(rjesup)
Attachment #8844638 - Attachment is obsolete: true
(Assignee)

Comment 21

a year ago
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
Flags: needinfo?(rjesup)
Attachment #8844790 - Flags: approval-mozilla-beta?
Attachment #8844790 - Flags: approval-mozilla-aurora?
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+.
Attachment #8844790 - Flags: approval-mozilla-beta?
Attachment #8844790 - Flags: approval-mozilla-beta+
Attachment #8844790 - Flags: approval-mozilla-aurora?
Attachment #8844790 - Flags: approval-mozilla-aurora+

Comment 23

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/3d46587f5dbe
status-firefox54: affected → fixed
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')
Flags: needinfo?(rjesup)
(Assignee)

Comment 25

a year ago
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
Bug 1337777 needed to be backed out, so this was as well.

https://hg.mozilla.org/releases/mozilla-beta/rev/9e4af5282696
status-firefox53: fixed → affected
(Reporter)

Comment 28

a year ago
Amazing work.
(Assignee)

Updated

a year ago
status-firefox53: affected → fixed
Setting qe-verify- based on Jesup's assessment on manual testing needs (see Comment 21).
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.