1437741 - Firefox 59 generates m=application line first instead of last in SDP

Status: RESOLVED FIXED

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

Description

[mroberts]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/64.0.3282.140 Safari/537.36

Steps to reproduce:

Run this JSFiddle in Firefox Stable (58) and Beta (59): https://jsfiddle.net/p33jy4vf/

From my reading of JSEP, the Firefox Beta behavior may not be spec-compliant.

> 5.2.1.  Initial Offers
>
>   ...
>   
>   The next step is to generate m= sections, as specified in [RFC4566],
>   Section 5.14.  An m= section is generated for each RtpTransceiver
>   that has been added to the PeerConnection, excluding any stopped
>   RtpTransceivers; this is done in the order the RtpTransceivers were
>   added to the PeerConnection.
>
>   ...
>
>   Lastly, if a data channel has been created, a m= section MUST be
>   generated for data.


Actual results:

Firefox Beta shows

> Media sections, in order:
>
> 1. application
> 2. audio
> 3. video


Expected results:

Firefox Stable, Chrome Stable, and Safari 11 show

> Media sections, in order:
>
> 1. audio
> 2. video
> 3. application

Comment 1

[Byron Campen [:bwc]]

This is probably due to the transceivers support that landed in 59. I can look into it.

Comment 2

[Nils Ohlmeier [:drno]]

Looks like the problem is around this code here https://searchfox.org/mozilla-central/rev/9011be0a172711bc243e50dfca16d42e877bf4ec/media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp#1423 where we just iterate over the Transceivers in the order they were created/added and assign then level numbers.

I guess we need to replace that simple loop with something more complex which iterates over the transceivers each time for audio, video and application :-(

Or do we just do audio and video in what ever random order they got added/created and only need to ensure to put the application m-section at the end? I'm wondering how many interop issues that could still create if the video m-section comes before the audio m-section.

Comment 3

[Byron Campen [:bwc]]

(In reply to Nils Ohlmeier [:drno] from comment #2)
> Looks like the problem is around this code here
> https://searchfox.org/mozilla-central/rev/
> 9011be0a172711bc243e50dfca16d42e877bf4ec/media/webrtc/signaling/src/jsep/
> JsepSessionImpl.cpp#1423 where we just iterate over the Transceivers in the
> order they were created/added and assign then level numbers.
> 
> I guess we need to replace that simple loop with something more complex
> which iterates over the transceivers each time for audio, video and
> application :-(
> 
> Or do we just do audio and video in what ever random order they got
> added/created and only need to ensure to put the application m-section at
> the end? I'm wondering how many interop issues that could still create if
> the video m-section comes before the audio m-section.

The spec looks like we should iterate over the transceivers in the order that they are created, and then create a datachannel m-section if necessary. That does of course mean that adding audio/video in a renegotiation can cause a datachannel m-section to appear before audio/video.

Comment 4

[Byron Campen [:bwc]]

Attachment: Bug 1437741 - Part 1: Expect datachannel m-sections to be last.

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

Comment 5

[Byron Campen [:bwc]]

Attachment: Bug 1437741 - Part 2: Map datachannel to an m-line after all RTP transceivers have been mapped.

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

Comment 6

[Nils Ohlmeier [:drno]]

Comment on attachment 8951292 [details]
Bug 1437741 - Part 2: Map datachannel to an m-line after all RTP transceivers have been mapped.

https://reviewboard.mozilla.org/r/220552/#review226556

LGTM

Comment 7

[Nils Ohlmeier [:drno]]

Comment on attachment 8951291 [details]
Bug 1437741 - Part 1: Expect datachannel m-sections to be last.

https://reviewboard.mozilla.org/r/220550/#review226554

As far as I can tell this patch only ensures that transceivers in the test are generated in the right order. I think what we need is an actual verification that the SDP from createOffer/createAnswer are returning strings with the properly sorted m-sections (similar to the other verification's we do in these functions already).

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:291
(Diff revision 1)
>        if (chunk == "audio") {
> -        type = SdpMediaSection::kAudio;
> +        result.push_back(SdpMediaSection::kAudio);
>        } else if (chunk == "video") {
> -        type = SdpMediaSection::kVideo;
> +        result.push_back(SdpMediaSection::kVideo);
>        } else if (chunk == "datachannel") {
> -        type = SdpMediaSection::kApplication;
> +        hasDatachannel = true;

I think we should add here a MOZ_CRASH if |hasDatachannel| is true already.

Comment 8

[Byron Campen [:bwc]]

Comment on attachment 8951291 [details]
Bug 1437741 - Part 1: Expect datachannel m-sections to be last.

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

Comment 9

[Byron Campen [:bwc]]

Comment on attachment 8951292 [details]
Bug 1437741 - Part 2: Map datachannel to an m-line after all RTP transceivers have been mapped.

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

Comment 10

[Byron Campen [:bwc]]

Comment on attachment 8951291 [details]
Bug 1437741 - Part 1: Expect datachannel m-sections to be last.

https://reviewboard.mozilla.org/r/220550/#review226554

> I think we should add here a MOZ_CRASH if |hasDatachannel| is true already.

I've completely redone this, and it no longer applies.

Comment 11

[Nils Ohlmeier [:drno]]

Comment on attachment 8951291 [details]
Bug 1437741 - Part 1: Expect datachannel m-sections to be last.

https://reviewboard.mozilla.org/r/220550/#review226678

Lots of good improvements. But I still don't see any verification of the order of the m-sections generated by JsepSessionImpl, e.g. when calling createOffer() or createAnswer().

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:239
(Diff revision 2)
> +    // Now, we move datachannel to the end
> +    auto it = std::find(types.begin(), types.end(),
> +                        SdpMediaSection::kApplication);
> +    if (it != types.end()) {
> +      types.erase(it);
> +      types.push_back(SdpMediaSection::kApplication);
> +    }

Wouldn't it make more sense to do this re-shuffling before calling AddTrack?

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:253
(Diff revision 2)
>    void
>    AddTracks(JsepSessionImpl& side,
>              const std::string& mediatypes,
>              AddTrackMagic magic = ADDTRACK_MAGIC)
>    {
>      AddTracks(side, BuildTypes(mediatypes), magic);

I think here the mediatypes would need to get sorted as well, or?

Comment 12

[Byron Campen [:bwc]]

Comment on attachment 8951291 [details]
Bug 1437741 - Part 1: Expect datachannel m-sections to be last.

https://reviewboard.mozilla.org/r/220550/#review226678

We are verifying that transceivers with level n have the same type as types[n]. Is that good enough?

> Wouldn't it make more sense to do this re-shuffling before calling AddTrack?

No, because we want to test hitting the JSEP API in different orders.

Comment 13

[Byron Campen [:bwc]]

Comment on attachment 8951291 [details]
Bug 1437741 - Part 1: Expect datachannel m-sections to be last.

https://reviewboard.mozilla.org/r/220550/#review226678

> I think here the mediatypes would need to get sorted as well, or?

No, because this doesn't end up in |types|, and we don't want to alter the order in which we hit the JSEP API.

Comment 14

[Nils Ohlmeier [:drno]]

Comment on attachment 8951291 [details]
Bug 1437741 - Part 1: Expect datachannel m-sections to be last.

https://reviewboard.mozilla.org/r/220550/#review226926

After chatting on IRC bwc pointed me to already existing code which now with this patch together ensures the right order.

Comment 15

[Byron Campen [:bwc]]

Request beta uplift once this is ok on m-c.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ece6d0a7605
Part 1: Expect datachannel m-sections to be last. r=drno
https://hg.mozilla.org/integration/autoland/rev/5f9b1f5a1955
Part 2: Map datachannel to an m-line after all RTP transceivers have been mapped. r=drno

Comment 17

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/1ece6d0a7605
https://hg.mozilla.org/mozilla-central/rev/5f9b1f5a1955

Comment 18

[Byron Campen [:bwc]]

Comment on attachment 8951292 [details]
Bug 1437741 - Part 2: Map datachannel to an m-line after all RTP transceivers have been mapped.

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

   Bug 1290948

[User impact if declined]:

   Some webrtc services that use datachannel will stop working properly.

[Is this code covered by automated tests?]:

   Yes.

[Has the fix been verified in Nightly?]:

   Yes.

[Needs manual test from QE? If yes, steps to reproduce]: 

   No.

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

   None.

[Is the change risky?]:

   Not really.

[Why is the change risky/not risky?]:

   The change is simple and small.

[String changes made/needed]:

   None.

Comment 19

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8951292 [details]
Bug 1437741 - Part 2: Map datachannel to an m-line after all RTP transceivers have been mapped.

Fix for a recent regression, let's uplift for beta 12. 
Thanks for fixing the tests!

Comment 20

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8951291 [details]
Bug 1437741 - Part 1: Expect datachannel m-sections to be last.

Assuming we can uplift the test fixes too.

Comment 21

[Cosmin Sabou [:CosminS]]

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