SDP answer chooses dynamic payload type without rtmap over lower priority codec

RESOLVED FIXED in Firefox 45

Status

()

P1
normal
Rank:
10
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: wyuan, Assigned: drno)

Tracking

({regression})

45 Branch
mozilla47
regression
Points:
---

Firefox Tracking Flags

(firefox44 unaffected, firefox45 fixed, firefox46 fixed, firefox47 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/47.0.2526.106 Safari/537.36

Steps to reproduce:

We send a webrtc audio call from mobile facebook messenger app on iOS to a facebook user on Firefox 45.0b2 beta on windows 8.


Actual results:

The call failed. 
From our mobile client, we sent the following offer SDP to firefox 45.0b2 and got the following answer SDP. The answer SDP from firefox does not contain any audio codec.


1) we send offer
NativeEngine - message sent: call_id[3821438796] msg_id[777203802] type[offer] peer_id[499719533] destination[any] content[v=0
	
	o=- 7279199083950916342 2 IN IP4 127.0.0.1
	
	s=-
	
	t=0 0
	
	a=group:BUNDLE audio video
	
	a=msid-semantic: WMS stream_label
	
	m=audio 1 RTP/SAVPF 101 103 96 120 105
	
	c=IN IP4 0.0.0.0
	
	a=rtcp:1 IN IP4 0.0.0.0
	
	a=ice-ufrag:Jy2Ey2jpYLSXIVG9
	
	a=ice-pwd:sIga4mZa97+yX5W+czdCXxSR
	
	a=ice-options:google-ice fb-force-5245
	
	a=fingerprint:sha-256 E4:FC:6C:84:9D:74:0A:3C:56:8D:C6:B2:F8:E3:AE:47:40:5F:0D:A6:A2:4F:4F:96:29:72:45:5E:7B:48:59:C5
	
	a=extmap:1 urn:ietf:params:rtp-hdrext:ssrc-audio-level
	
	a=sendrecv
	
	a=mid:audio
	
	a=rtcp-mux
	
	a=crypto:1 AES_CM_128_HMAC_SHA1_80 inline:Se4U09GvnyihOPI78NytoWshD6dW8wQOvRXb6e2j
	
	a=rtpmap:101 ISPX_WB_2/16000
	
	a=fmtp:101 ispx_fec=
	
	a=rtpmap:103 ISAC/16000
	
	a=rtpmap:96 speex/8000
	
	a=fmtp:96 speexusejitterbit=
	
	a=rtpmap:120 opus/48000/2
	
	a=fmtp:120 maxaveragebitrate=12000; minptime=10; sprop-stereo=0; stereo=0
	
	a=rtpmap:105 CN/16000
	
	a=maxptime:60
	
	a=ssrc:2813934391 cname:QIBCZ
	
	a=ssrc:2813934391 msid:stream_label 10455466651001579923
	
	a=ssrc:2813934391 mslabel:stream_label
	
	a=ssrc:2813934391 label:10455466651001579923
	
	m=video 1 RTP/SAVPF 100 116
	
	c=IN IP4 0.0.0.0
	
	a=rtcp:1 IN IP4 0.0.0.0
	
	a=ice-ufrag:Jy2Ey2jpYLSXIVG9
	
	a=ice-pwd:sIga4mZa97+yX5W+czdCXxSR
	
	a=ice-options:google-ice fb-force-5245
	
	a=fingerprint:sha-256 E4:FC:6C:84:9D:74:0A:3C:56:8D:C6:B2:F8:E3:AE:47:40:5F:0D:A6:A2:4F:4F:96:29:72:45:5E:7B:48:59:C5
	
	a=extmap:2 urn:ietf:params:rtp-hdrext:toffset
	
	a=sendrecv
	
	a=mid:video
	
	a=rtcp-mux
	
	a=crypto:1 AES_CM_128_HMAC_SHA1_80 inline:Se4U09GvnyihOPI78NytoWshD6dW8wQOvRXb6e2j
	
	a=rtpmap:100 VP8/90000
	
	a=rtcp-fb:100 ccm fir
	
	a=rtcp-fb:100 nack 
	
	a=rtcp-fb:100 goog-remb 
	
	a=rtpmap:116 red/90000
	
	a=ssrc:1931255615 cname:QIBCZ
	
	a=ssrc:1931255615 msid:stream_label 6619395763298928794
	
	a=ssrc:1931255615 mslabel:stream_label
	
	a=ssrc:1931255615 label:6619395763298928794
	
	]

2) we get answer

 message received: call_id[3821438796] msg_id[585469982] type[answer] peer_id[499719533] content[v=0
	
	o=- 4891121299812188730 0 IN IP4 127.0.0.1
	
	s=-
	
	t=0 0
	
	a=group:BUNDLE audio video
	
	a=msid-semantic: WMS {be0b39f8-0dab-4565-95c3-5999fb4a1048}
	
	m=audio 1 RTP/SAVPF 0
	
	c=IN IP4 0.0.0.0
	
	a=rtcp:1 IN IP4 0.0.0.0
	
	a=ice-ufrag:cd5b7a70
	
	a=ice-pwd:41e8b83fa45ad440e3193c95f4fa3ae7
	
	a=ice-options:trickle fb-force-5245
	
	a=fingerprint:sha-256 D5:07:7A:90:93:3D:27:9E:1D:4A:4C:85:0B:9D:4B:BA:EF:26:91:FF:D3:A8:88:4C:6D:94:AE:EB:90:FB:CE:1D
	
	a=extmap:1 urn:ietf:params:rtp-hdrext:ssrc-audio-level
	
	a=sendrecv
	
	a=mid:audio
	
	a=rtcp-mux
	
	a=ssrc:108919066 cname:{de64a5e4-46f3-4726-8f19-a40ca4e056cb}
	
	a=ssrc:108919066 msid:{be0b39f8-0dab-4565-95c3-5999fb4a1048} {936be59c-4221-47e1-84d0-3b12a7935f24}
	
	a=ssrc:108919066 mslabel:{be0b39f8-0dab-4565-95c3-5999fb4a1048}
	
	a=ssrc:108919066 label:{936be59c-4221-47e1-84d0-3b12a7935f24}
	
	m=video 1 RTP/SAVPF 100
	
	c=IN IP4 0.0.0.0
	
	a=rtcp:1 IN IP4 0.0.0.0
	
	a=ice-ufrag:cd5b7a70
	
	a=ice-pwd:41e8b83fa45ad440e3193c95f4fa3ae7
	
	a=ice-options:trickle fb-force-5245
	
	a=fingerprint:sha-256 D5:07:7A:90:93:3D:27:9E:1D:4A:4C:85:0B:9D:4B:BA:EF:26:91:FF:D3:A8:88:4C:6D:94:AE:EB:90:FB:CE:1D
	
	a=sendrecv
	
	a=mid:video
	
	a=rtcp-mux
	
	a=rtpmap:100 VP8/90000
	
	a=rtcp-fb:100 ccm fir
	
	a=rtcp-fb:100 nack 
	
	a=ssrc:3445741659 cname:{de64a5e4-46f3-4726-8f19-a40ca4e056cb}
	
	a=ssrc:3445741659 msid:{be0b39f8-0dab-4565-95c3-5999fb4a1048} {cab2171d-7832-445c-a20e-eab6df367c46}
	
	a=ssrc:3445741659 mslabel:{be0b39f8-0dab-4565-95c3-5999fb4a1048}
	
	a=ssrc:3445741659 label:{cab2171d-7832-445c-a20e-eab6df367c46}
	
	] 


Expected results:

We would expect a valid audio codec, opus or iSAC from the answer SDP so that the codec negotiation can proceed
(Reporter)

Comment 1

3 years ago
This behavior is new to 45.0b2 beta, different from previous releases

Updated

3 years ago
Component: Untriaged → WebRTC: Networking
Product: Firefox → Core
(Reporter)

Comment 2

3 years ago
If the first codec of the offer SDP is iSAC or speex, we experience the same problem. Only way to work around it seems to put opus at the top of the codec list

Comment 3

3 years ago
That answer did not come from firefox; that either came from Chrome, or something that imitates Chrome.
(Assignee)

Comment 4

3 years ago
Not able to reproduce the problem with 45.0b2 or 46: https://jsfiddle.net/dnvkmw2m/2/
Both correctly answer with 120 opus.
I modified pc_test to inject that offer.  I get a *very* different answer; if the answer above comes from Firefox, it's been heavily modified by a midbox somewhere, and given all the modifications, the m=line could well have been munged as well.  (starting with o=mozilla.... is missing, and a generic o= line is there).

I also get Opus in response to that SDP.
We know what's going on and we should have a fix coded, reviewed and likely landed today.
Assignee: nobody → drno
Status: UNCONFIRMED → NEW
Rank: 10
Ever confirmed: true
Priority: -- → P1
(Assignee)

Updated

3 years ago
backlog: --- → webrtc/webaudio+
status-firefox44: --- → unaffected
status-firefox45: --- → affected
status-firefox46: --- → affected
status-firefox47: --- → affected
(Assignee)

Comment 7

3 years ago
Just verified via manual testing that the patch I have is working. Working on tests..
(Assignee)

Comment 8

3 years ago
Created attachment 8716469 [details]
MozReview Request: Bug 1246011: fixed PT comparising for PT's without rtpmap. r=jesup

Review commit: https://reviewboard.mozilla.org/r/33827/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33827/
Attachment #8716469 - Flags: review?(rjesup)

Updated

3 years ago
Attachment #8716469 - Flags: review?(rjesup) → review+
Comment on attachment 8716469 [details]
MozReview Request: Bug 1246011: fixed PT comparising for PT's without rtpmap. r=jesup

https://reviewboard.mozilla.org/r/33827/#review30491

::: dom/media/tests/mochitest/test_peerConnection_basicAudioDynamicPtMissingRtpmap.html:17
(Diff revision 1)
> -    options.opus = false;
> +    // we want Opus to get selected and 101 to be ignore

ignored
(Assignee)

Comment 11

3 years ago
Comment on attachment 8716469 [details]
MozReview Request: Bug 1246011: fixed PT comparising for PT's without rtpmap. r=jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33827/diff/1-2/
(Assignee)

Updated

3 years ago
Depends on: 1221837
Keywords: regression
(Assignee)

Updated

3 years ago
Summary: answer SDP during a webrtc call from firefox 45.0b2 contains no audio codec → SDP answer chooses dynamic payload type without rtmap over lower priority codec
(Assignee)

Comment 12

3 years ago
Comment on attachment 8716469 [details]
MozReview Request: Bug 1246011: fixed PT comparising for PT's without rtpmap. r=jesup

Approval Request Comment
[Feature/regressing bug #]: 1221837

[User impact if declined]: WebRTC calls might get established with the G.722 codec even though the offer did not offer (or even supports) G.722. This codec mismatch results in audio being broken on the call. This currently affects Facebook's video calling.

[Describe test coverage new/current, TreeHerder]: The test from bug 1221837 has been fixed to properly ensure that G.722 no longer gets chosen. And a new mochitest has been added with this patch to cover this new scenario.

[Risks and why]: The risk is pretty small, as the code only gets triggered in a corner case (if the offerer sends offers a payload without a matching rtpmap - which is against the spec).

[String/UUID change made/needed]: N/A
Attachment #8716469 - Flags: approval-mozilla-beta?
Attachment #8716469 - Flags: approval-mozilla-aurora?

Comment 14

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/39903dd406e1
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment on attachment 8716469 [details]
MozReview Request: Bug 1246011: fixed PT comparising for PT's without rtpmap. r=jesup

Fix a recent regression, taking it.
Attachment #8716469 - Flags: approval-mozilla-beta?
Attachment #8716469 - Flags: approval-mozilla-beta+
Attachment #8716469 - Flags: approval-mozilla-aurora?
Attachment #8716469 - Flags: approval-mozilla-aurora+

Comment 16

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/37631161ed0c
status-firefox46: affected → fixed
has problems during beta uplift:

grafting 327694:37631161ed0c "Bug 1246011: fixed PT comparising for PT's without rtpmap. r=jesup, a=sylvestre"
merging dom/media/tests/mochitest/mochitest.ini
merging dom/media/tests/mochitest/sdpUtils.js
warning: conflicts while merging dom/media/tests/mochitest/sdpUtils.js! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)
(Assignee)

Comment 18

3 years ago
Created attachment 8717073 [details] [diff] [review]
bug1246011_beta.patch

Not sure how to do this properly with mozreview: this patch applies cleanly to Beta.
Flags: needinfo?(drno)
Attachment #8717073 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-beta/rev/15ab48ee0814
status-firefox45: affected → fixed
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.