Closed
Bug 1437741
Opened 5 years ago
Closed 5 years ago
Firefox 59 generates m=application line first instead of last in SDP
Categories
(Core :: WebRTC: Signaling, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox58 | --- | unaffected |
firefox59 | + | fixed |
firefox60 | --- | fixed |
People
(Reporter: mroberts, Assigned: bwc)
References
Details
(Keywords: regression)
Attachments
(2 files)
59 bytes,
text/x-review-board-request
|
drno
:
review+
lizzard
:
approval-mozilla-beta+
|
Details |
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
Assignee | ||
Comment 1•5 years ago
|
||
This is probably due to the transceivers support that landed in 59. I can look into it.
Updated•5 years ago
|
Rank: 15
Component: Untriaged → WebRTC: Signaling
Priority: -- → P2
Product: Firefox → Core
Updated•5 years ago
|
status-firefox58:
--- → unaffected
status-firefox59:
--- → affected
status-firefox60:
--- → affected
tracking-firefox59:
--- → ?
Depends on: 1290948
Keywords: regression
Updated•5 years ago
|
Assignee: nobody → docfaraday
Comment 2•5 years ago
|
||
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.
Assignee | ||
Comment 3•5 years ago
|
||
(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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 6•5 years ago
|
||
mozreview-review |
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 7•5 years ago
|
||
mozreview-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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•5 years ago
|
||
mozreview-review-reply |
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•5 years ago
|
||
mozreview-review |
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-
Assignee | ||
Comment 12•5 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 13•5 years ago
|
||
mozreview-review-reply |
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•5 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 15•5 years ago
|
||
Request beta uplift once this is ok on m-c.
Flags: needinfo?(docfaraday)
Comment 16•5 years ago
|
||
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
Comment 17•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1ece6d0a7605 https://hg.mozilla.org/mozilla-central/rev/5f9b1f5a1955
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Assignee | ||
Comment 18•5 years ago
|
||
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+
Comment 21•5 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/6b694e4a98fe https://hg.mozilla.org/releases/mozilla-beta/rev/e683ed9cd792
Updated•5 years ago
|
status-firefox-esr52:
--- → unaffected
You need to log in
before you can comment on or make changes to this bug.
Description
•