1476600 - Mid from stopped transceiver is reused

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect, P2)

Description

[ben.browitt]

Attachment: pc1.zip

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.

Comment 1

[Byron Campen [:bwc]]

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.

Comment 2

[Byron Campen [:bwc]]

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

Comment 3

[ben.browitt]

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

Comment 4

[Byron Campen [:bwc]]

(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.

Comment 5

[ben.browitt]

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

Comment 6

[Byron Campen [:bwc]]

(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.

Comment 7

[ben.browitt]

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

Comment 8

[Byron Campen [:bwc]]

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.

Comment 9

[Byron Campen [:bwc]]

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.

Comment 10

[ben.browitt]

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.

Comment 11

[Byron Campen [:bwc]]

(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.

Comment 12

[ben.browitt]

Great. Thanks.

Comment 13

[Byron Campen [:bwc]]

Attachment: Bug 1476600: Stop reusing mids from stopped transceivers.

Review commit: https://reviewboard.mozilla.org/r/258216/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258216/

Comment 14

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d656713dff5a

Comment 15

[Jeremy Lainé]

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", ..

Comment 16

[Byron Campen [:bwc]]

Comment on attachment 8993514 [details]
Bug 1476600: Stop reusing mids from stopped transceivers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258216/diff/1-2/

Comment 17

[Byron Campen [:bwc]]

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.

Comment 18

[Byron Campen [:bwc]]

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

Comment 19

[ben.browitt]

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.

Comment 20

[Byron Campen [:bwc]]

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...

Comment 21

[Byron Campen [:bwc]]

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

Comment 22

[Byron Campen [:bwc]]

Attachment: Bug 1476600: Stop reusing mids from stopped transceivers. r?jib

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

Comment 23

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(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 24

[Phabricator Automation]

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 25

[Phabricator Automation]

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 26

[Phabricator Automation]

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 27

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 28

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d215c566027f39a24529347504f2f764ea80a2f

Comment 29

[Byron Campen [:bwc]]

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 30

[Phabricator Automation]

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 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4b38c8495686
Stop reusing mids from stopped transceivers. r=jib

Comment 32

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

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

Comment 33

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

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

Comment 34

[Dorel Luca [:dluca]]

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

Comment 35

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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?...

Comment 36

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged