User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0 Build ID: 20180704003137 Steps to reproduce: When a remote peer sends an answer to Firefox with a=inactive track and use port=0, Firefox reuse old transceivers with old mid and msid. This attached example munge the sdp and use this line to cause the problem (audio 0): m=audio 0 UDP/TLS/RTP/SAVPF 109 9 0 8 101 This line is good (audio 9): m=audio 9 UDP/TLS/RTP/SAVPF 109 9 0 8 101 Run the attached pc1.zip. 1. Click on Start. 2. Click on Call. 3. Click on Toggle Audio. 4. Click on Toggle Video. 5. Click on Toggle Audio. 6. Clear the console. 7. Click on Toggle Audio. 8. Look at the pc1 transceivers at the top of the console. Actual results: Old transceivers reused with old mid and msid. 0: RTCRtpTransceiver { mid: "sdparta_0", stopped: false, direction: "recvonly", … } 1: RTCRtpTransceiver { mid: "sdparta_1", stopped: false, direction: "recvonly", … } 2: RTCRtpTransceiver { mid: null, stopped: true, direction: "recvonly", … } 3: RTCRtpTransceiver { mid: "sdparta_3", stopped: false, direction: "recvonly", … } 4: RTCRtpTransceiver { mid: "sdparta_2", stopped: false, direction: "sendrecv", … } 5: RTCRtpTransceiver { mid: "sdparta_4", stopped: false, direction: "sendrecv", … } Expected results: New transceivers should be created with correct mid and msid. I'm not sure what's the correct sdp in the answer for inactive track but it's probably shouldn't confuse the transceivers in the offerer.

We don't reuse stopped transceivers, but we probably do reuse the old mid in the new transceiver. I'm looking at the spec now to see what it says about that.

Ok, yeah, we should be using a new mid when this happens.

It's not just the mid. In the last step two tracks of the same stream get different msid. Forgot to say that the attached test should be placed in the webrtc/samples project here: https://github.com/webrtc/samples/tree/gh-pages/src/content/peerconnection/pc1 What's the correct SDP for inactive track? What does port 9 means in this line and what spec explains it? m=audio 9 UDP/TLS/RTP/SAVPF 109 9 0 8 101

(In reply to ben.browitt from comment #3) > It's not just the mid. In the last step two tracks of the same stream get > different msid. Are you talking about the remote msid? > Forgot to say that the attached test should be placed in the webrtc/samples > project here: > https://github.com/webrtc/samples/tree/gh-pages/src/content/peerconnection/ > pc1 Could you simplify this and put it on jsfiddle? > What's the correct SDP for inactive track? To temporarily stop a track you use the direction attribute, but don't mess with the port on the m-line. Setting the port to 0 stops the transceiver, which is a little more permanent. > What does port 9 means in this line and what spec explains it? > m=audio 9 UDP/TLS/RTP/SAVPF 109 9 0 8 101 That's a trickle ICE thing, although I'm not sure where that bit of specification lives now.

I'm talking about local msid. Audio and video tracks from the same stream have different msid in the sdp. Probably because the transceiver reuse old mid. I can reproduce it in my app but not in the attached test. You can download a self contained test from here: https://drive.google.com/open?id=1aQRRfoEBlmAkgfbTof7n758ZhsAxsPLj 1. Extract the zip. 2. Run python3 -m http.server 3. Navigate with Firefox 61 to http://localhost:8000

(In reply to ben.browitt from comment #5) > I'm talking about local msid. > Audio and video tracks from the same stream have different msid in the sdp. So something that you might be tripping over is the requirement (in the JSEP spec) that a=msid lines never change in the SDP, even if their underlying tracks change. Can you attach the SDP from the offer/answer exchanges here, that will probably tell me what I need to know.

Lip sync depends on a=msid. Two tracks of the same stream shouldn't have different msid. This happens because the old mid is reused. The test case have anything you need to reproduce this bug. https://drive.google.com/open?id=1aQRRfoEBlmAkgfbTof7n758ZhsAxsPLj

Your test case seems to call addTrack/removeTrack on one side of the call (pc1). The result is that pc2's local tracks continue using sdparta_1 and sdparta_2, while pc1's (removed and re-added) local tracks end up in new transceivers, due to the rules about transceiver reuse in the spec (calling addTrack will only reuse a transceiver of the appropriate type that has never sent RTP before, otherwise a new transceiver will be created). So, the transceivers are doing exactly what the spec says to do. As for pc1's a=msid attributes that are left in sdparta_1 and sdparta_2, those are there because the spec forbids us to change them; the direction attribute tells you whether a track is still there or not, a=msid is of no help there. Even if we were to use replaceTrack to assign new (unrelated) tracks to those transceivers, they have to keep exactly the same a=msid, which can lead to collisions with the scenario you've written (because you are re-adding the old tracks). This is https://github.com/rtcweb-wg/jsep/issues/842 you're tripping over, I think.

Btw, it looks like the spec is moving toward omitting the track ids in a=msid, and just putting the stream id in there. The transceivers-based spec update made the track ids useless anyway, so we're better off not having them in the SDP, but man has backward compat been trashed. What a mess.

Are you getting the following transceivers in the log on the final step? 0: RTCRtpTransceiver { mid: "sdparta_0", stopped: false, direction: "recvonly", … } 1: RTCRtpTransceiver { mid: "sdparta_1", stopped: false, direction: "recvonly", … } 2: RTCRtpTransceiver { mid: null, stopped: true, direction: "recvonly", … } 3: RTCRtpTransceiver { mid: "sdparta_3", stopped: false, direction: "recvonly", … } 4: RTCRtpTransceiver { mid: "sdparta_2", stopped: false, direction: "sendrecv", … } 5: RTCRtpTransceiver { mid: "sdparta_4", stopped: false, direction: "sendrecv", … } As you can see, transceiver number 4 has mid="sdparta_2" instead of "sdparta_5". This is the bug.

(In reply to ben.browitt from comment #10) > Are you getting the following transceivers in the log on the final step? > 0: RTCRtpTransceiver { mid: "sdparta_0", stopped: false, direction: > "recvonly", … } > 1: RTCRtpTransceiver { mid: "sdparta_1", stopped: false, direction: > "recvonly", … } > 2: RTCRtpTransceiver { mid: null, stopped: true, direction: "recvonly", … } > 3: RTCRtpTransceiver { mid: "sdparta_3", stopped: false, direction: > "recvonly", … } > 4: RTCRtpTransceiver { mid: "sdparta_2", stopped: false, direction: > "sendrecv", … } > 5: RTCRtpTransceiver { mid: "sdparta_4", stopped: false, direction: > "sendrecv", … } > > As you can see, transceiver number 4 has mid="sdparta_2" instead of > "sdparta_5". This is the bug. That is the bug I was talking about in comment 1.

Great. Thanks.

Regarding the choice of MIDs, it would be good to use shorter MIDs (i.e. without the "sdparta_" prefix) in order to avoid bloating the RTP header extension. The BUNDLE draft recommends 1-3 byte values: https://tools.ietf.org/html/draft-ietf-mmusic-sdp-bundle-negotiation-52#section-14.2 Chrome uses "0", "1", "2", ..

Note to self: bug 1478367 is going to rot this pretty badly. Once that lands, rebase and implement comment 15 since both of those things will involve messing with the same parts of the same test.

Try looks good, can you try out the binary here? https://queue.taskcluster.net/v1/task/fP0lt9u4R1qlNWVcIRvnoQ/runs/0/artifacts/public/build/install/sea/target.installer.exe

Tested the 1-8 steps in the first post on Ubuntu 18.04 with fake stream and: https://queue.taskcluster.net/v1/task/UYdvEA32ReKxmUl6LMEhlw/runs/0/artifacts/public/build/target.tar.bz2 Got a tab crash but it's not on about:crashes.

Ugh. Looks like the revision I based my patch on crashes with your test somewhere in MSG. Now I have to figure out where it broke...

Looks like your test case hits an assertion down in MSG, and has for some time. I'll file a separate bug. In the meantime, let's try a non-debug build: https://queue.taskcluster.net/v1/task/Ro7GDfC9SfqaXWCjApUF7w/runs/0/artifacts/public/build/target.tar.bz2

When a transceiver is stopped, its mid should not be reused by a new transceiver.

(In reply to Byron Campen [:bwc] from comment #17) > Note to self: bug 1478367 is going to rot this pretty badly. Once that > lands, rebase and implement comment 15 since both of those things will > involve messing with the same parts of the same test. Bug 1478367 has landed. On the upside those tests no longer check for "sdparta_*" specifically, so implementing comment 15 should require fewer test changes at least.

Comment on attachment 8996026 [details] Bug 1476600: Stop reusing mids from stopped transceivers. r?jib Jan-Ivar Bruaroey [:jib] (needinfo? me) has approved the revision. https://phabricator.services.mozilla.com/D2518

Comment on attachment 8996026 [details] Bug 1476600: Stop reusing mids from stopped transceivers. r?jib Jan-Ivar Bruaroey [:jib] (needinfo? me) has requested changes to the revision. https://phabricator.services.mozilla.com/D2518

Comment on attachment 8996026 [details] Bug 1476600: Stop reusing mids from stopped transceivers. r?jib Jan-Ivar Bruaroey [:jib] (needinfo? me) has approved the revision. https://phabricator.services.mozilla.com/D2518

Sorry, getting used to Phabricator workflow where comments are apparently not reflected in bugzilla anymore. r+ with nits.

Comment on attachment 8996026 [details] Bug 1476600: Stop reusing mids from stopped transceivers. r?jib Re-asking for review in Phabricator apparently doesn't have any effect on bugzilla...

Comment on attachment 8996026 [details] Bug 1476600: Stop reusing mids from stopped transceivers. r?jib Jan-Ivar Bruaroey [:jib] (needinfo? me) has approved the revision. https://phabricator.services.mozilla.com/D2518

Pushed by bcampen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4b38c8495686 Stop reusing mids from stopped transceivers. r=jib

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12249 for changes under testing/web-platform/tests

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment on attachment 8996026 [details] Bug 1476600: Stop reusing mids from stopped transceivers. r?jib Unlike mozReview, it looks like Phabricator does not clear manual r?...

Upstream PR merged