Closed Bug 1437741 Opened 3 years ago Closed 3 years ago

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

Categories

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

Product:
Core
Component:
WebRTC: Signaling
Version:
59 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 + fixed
firefox60 --- fixed

People

(Reporter: mroberts, Assigned: bwc)

References

Details

(Keywords: regression)

Attachments

(2 files)

Bug 1437741 - Part 1: Expect datachannel m-sections to be last.
59 bytes, text/x-review-board-request
drno
: review+
lizzard
: approval-mozilla-beta+
Details
Bug 1437741 - Part 2: Map datachannel to an m-line after all RTP transceivers have been mapped.
59 bytes, text/x-review-board-request
drno
: review+
lizzard
: approval-mozilla-beta+
Details 
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
This is probably due to the transceivers support that landed in 59. I can look into it.
Rank: 15
Component: Untriaged → WebRTC: Signaling
Priority: -- → P2
Product: Firefox → Core
Assignee: nobody → docfaraday
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.
(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 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
Attachment #8951292 - Flags: review?(drno) → review+
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.
Attachment #8951291 - Flags: review?(drno) → review-
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 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?
Attachment #8951291 - Flags: review?(drno) → review-
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 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 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.
Attachment #8951291 - Flags: review- → review+
Request beta uplift once this is ok on m-c.
Flags: needinfo?(docfaraday)
Pushed by bcampen@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/1ece6d0a7605
https://hg.mozilla.org/mozilla-central/rev/5f9b1f5a1955
Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox60: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
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.
Flags: needinfo?(docfaraday)
Attachment #8951292 - Flags: approval-mozilla-beta?
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!
Attachment #8951292 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8951291 [details]
Bug 1437741 - Part 1: Expect datachannel m-sections to be last.

Assuming we can uplift the test fixes too.
Attachment #8951291 - Flags: approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.