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

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P1)

Description

[wyuan]

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

Comment 1

[wyuan]

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

Comment 2

[wyuan]

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

[Byron Campen [:bwc]]

That answer did not come from firefox; that either came from Chrome, or something that imitates Chrome.

Comment 4

[Nils Ohlmeier [:drno]]

Not able to reproduce the problem with 45.0b2 or 46: https://jsfiddle.net/dnvkmw2m/2/
Both correctly answer with 120 opus.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 6

[Maire Reavy [:mreavy]]

We know what's going on and we should have a fix coded, reviewed and likely landed today.

Comment 7

[Nils Ohlmeier [:drno]]

Just verified via manual testing that the patch I have is working. Working on tests..

Comment 8

[Nils Ohlmeier [:drno]]

Attachment: 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/

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 10

[Nils Ohlmeier [:drno]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=523bedfad049

Comment 11

[Nils Ohlmeier [:drno]]

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/

Comment 12

[Nils Ohlmeier [:drno]]

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

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/39903dd406e1

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/39903dd406e1

Comment 15

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/37631161ed0c

Comment 17

[Carsten Book [:Tomcat]]

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)

Comment 18

[Nils Ohlmeier [:drno]]

Attachment: bug1246011_beta.patch

Not sure how to do this properly with mozreview: this patch applies cleanly to Beta.

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-beta/rev/15ab48ee0814