Closed Bug 1425956 Opened 7 years ago Closed 7 years ago

Removing a track and later re-adding it to a peer connection causes InvalidSessionDescriptionError

Categories

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

Product:
Core
Component:
WebRTC: Signaling
Version:
59 Branch
Platform:
Unspecified
All
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: jib, Assigned: bwc)

References

Details

Attachments

(4 files)

Bug 1425956 - Part 1: Test-case that removes and re-adds a track that was negotiated previously.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1425956 - Part 2: Don't emit duplicate track ids in SDP.
59 bytes, text/x-review-board-request
drno
: review+
Details
Bug 1425956 - Part 3: Remove duplicate track ids on incoming SDP.
59 bytes, text/x-review-board-request
drno
: review+
Details
Bug 1425956 - Part 1.1: Fix flaw in hasProps noticed during review where {} would match null.
59 bytes, text/x-review-board-request
jib
: review+
Details 
STRs:
 1. Open https://jsfiddle.net/jib1/an44uhtd/ and share cam+mic
 2. check ☑ hold
 3. uncheck ☐ hold

Expected result:
  video appears, then freezes (separate bug)

Actual result:
  InvalidSessionDescriptionError: track id:{133942a0-a162-5c4b-b57f-6b35d4218a99} appears in more than one m-section at level 2

The first fiddle in the "Evolution of WebRTC" blog [2] is broken the same way.

Basically, we believe this is caused by a broken JSEP spec. [1]

[1] https://github.com/rtcweb-wg/jsep/issues/842
[2] https://blog.mozilla.org/webrtc/the-evolution-of-webrtc/
Rank: 11
Also, InvalidSessionDescriptionError isn't in the spec. See [1].

[1] https://github.com/w3c/webrtc-pc/issues/1702
Rank: 11
Rank: 11
Comment on attachment 8937584 [details]
Bug 1425956 - Part 1: Test-case that removes and re-adds a track that was negotiated previously.

https://reviewboard.mozilla.org/r/208258/#review214046

Lgtm modulo nits.

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:662
(Diff revision 1)
> +    // pc1 is offerer
> +    offer = await pc1.createOffer();
> +    await pc2.setRemoteDescription(offer);
> +    await pc1.setLocalDescription(offer);
> +    answer = await pc2.createAnswer();
> +    await pc1.setRemoteDescription(answer);
> +    await pc2.setLocalDescription(answer);

Have you considered a reusable sub-function?

    await negotiate(pc1, pc2);

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:703
(Diff revision 1)
> +        }
> +      ]);
> +
> +    // pc1 is answerer. We need to create a new transceiver so pc1 will have
> +    // something to attach the re-added track to
> +    pc2.addTransceiver("audio");

This will fire a track event on pc1 after negotiation. Do we test that someplace else? If not, should we test it here?

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:723
(Diff revision 1)
> +        {
> +          receiver: {track: {}},
> +          currentDirection: "inactive"
> +        },
> +        {
> +          sender: {track: {}},

Isn't sender.track == null here?

Shouldn't we test receiver.track as well?
Attachment #8937584 - Flags: review?(jib) → review+
Comment on attachment 8937585 [details]
Bug 1425956 - Part 2: Don't emit duplicate track ids in SDP.

https://reviewboard.mozilla.org/r/208260/#review214062

This looks good for one round of offer-answer. But I'm not convinced yet that it won't cause other interop problems.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:390
(Diff revision 1)
> +            new SdpMsidAttributeList(mediaAttrs.GetMsid()));
> +        for (auto& msid : newMsids->mMsids) {
> +          msid.appdata = trackId;
> +        }
> +
> +        mediaAttrs.SetAttribute(newMsids.release());

Don't you need to this new IDs in a permanent lookup table to enable re-writing to the same fake track ID for every following renegotiation?

Or in other words: isn't this going to create new track IDs in new offer-answer round, which then will/might confuse a track based implementation?
Attachment #8937585 - Flags: review?(drno) → review-
Comment on attachment 8937586 [details]
Bug 1425956 - Part 3: Remove duplicate track ids on incoming SDP.

https://reviewboard.mozilla.org/r/208262/#review214066

LGTM
Do we want additional JSEP unit tests for this to "describe" the expected behavior (given that this is not described in any spec as of right now)?
Attachment #8937586 - Flags: review?(drno) → review+
Comment on attachment 8937584 [details]
Bug 1425956 - Part 1: Test-case that removes and re-adds a track that was negotiated previously.

https://reviewboard.mozilla.org/r/208258/#review214046

> This will fire a track event on pc1 after negotiation. Do we test that someplace else? If not, should we test it here?

We have tested this in other places plenty.

> Isn't sender.track == null here?
> 
> Shouldn't we test receiver.track as well?

What about receiver.track would we test? I suppose we could check the kind, but we already do that testing elsewhere.
Comment on attachment 8937586 [details]
Bug 1425956 - Part 3: Remove duplicate track ids on incoming SDP.

https://reviewboard.mozilla.org/r/208262/#review214066

I don't know, maybe a test that inserts a dupe and just verifies that we don't barf?
Comment on attachment 8937749 [details]
Bug 1425956 - Part 1.1: Fix flaw in hasProps noticed during review where {} would match null.

https://reviewboard.mozilla.org/r/208456/#review214250
Attachment #8937749 - Flags: review?(jib) → review+
Check on try, looks good so far.
Flags: needinfo?(docfaraday)
Comment on attachment 8937585 [details]
Bug 1425956 - Part 2: Don't emit duplicate track ids in SDP.

https://reviewboard.mozilla.org/r/208260/#review214340

Copying over the MSID from the last stable SDP hopefully solves my concern about renegotiation.
Attachment #8937585 - Flags: review?(drno) → review+
Pushed by bcampen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/76f26552f433
Part 1: Test-case that removes and re-adds a track that was negotiated previously. r=jib
https://hg.mozilla.org/integration/autoland/rev/ae1d83fc9eab
Part 1.1: Fix flaw in hasProps noticed during review where {} would match null. r=jib
https://hg.mozilla.org/integration/autoland/rev/a51a639d16c0
Part 2: Don't emit duplicate track ids in SDP. r=drno
https://hg.mozilla.org/integration/autoland/rev/8e682e487ef1
Part 3: Remove duplicate track ids on incoming SDP. r=drno
Flags: needinfo?(docfaraday)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: