Closed Bug 1096795 Opened 10 years ago Closed 9 years ago

WebRTC SDP does not include required a=rtcp:<port>

Categories

(Core :: WebRTC: Signaling, defect)

33 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox33 --- wontfix
firefox34 --- wontfix
firefox35 --- wontfix
firefox36 --- wontfix
firefox37 --- wontfix
firefox38 --- wontfix
firefox38.0.5 --- wontfix
firefox39 --- wontfix
firefox40 --- fixed
firefox41 --- fixed
firefox-esr31 --- wontfix
firefox-esr38 --- affected
b2g-v2.0 --- wontfix
b2g-v2.1 --- affected
b2g-v2.2 --- affected

People

(Reporter: ramanbhati.10, Assigned: bwc)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached file firefox caller.txt
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/38.0.2125.111 Safari/537.36

Steps to reproduce:

At the time of creating an offer in WebRTC, the generated SDP has two ICE candidates. The port number of second ICE candidate does not match with what the other party expects it to be. 


Actual results:

The offerer gets ICE-Mismatch error. This happens because the generated SDP does not have any RTCP line (a=rtcp:PORTNO IN IP4 IPADDR). In that case, it should change the port number of second ICE candidate and increment the port number by 1. For example: 

a=candidate:0 1 UDP PRI IPADDR 63495 typ host
a=candidate:0 2 UDP PRI IPADDR 49190 typ host

should be

a=candidate:0 1 UDP PRI IPADDR 63495 typ host
a=candidate:0 2 UDP PRI IPADDR 63496 typ host


Expected results:

The call should be established. On Windows, Firefox generates valid SDP and the call gets established. But, on Macs, it generates incorrect SDP which causes ICE-Mismatch error.
Summary: Generation of incorrect SDP causing ICE-Mismatch on MAC in WebRTC project → Generation of incorrect SDP causing ICE-Mismatch on Mac systems in WebRTC
I suspect the far-end system doesn't understand rtcp-mux, and assumes rtcp must be rtp+1 (without a specified RTCP port), but I'm too tired to look closely.  I do note we don't include an a=rtcp line, so a recipient who doesn't understand a=rtcpmux will assume rtcp=rtp+1 - except I presume ICE relaxes that requirement (here my lack of ICE spec knowledge causes me to defer to those who know).
Flags: needinfo?(docfaraday)
Quoting from RFC 5245:
      For the RTP component,
      the default IP address is in the c line of the SDP, and the port
      is in the m line.  For the RTCP component, it is in the rtcp
      attribute when present, and when not present, the IP address is in
      the c line and 1 plus the port is in the m line.

So looks like we need to check the rtcp-mux spec extension and what it suggests for backward compatibility with clients which don't understand rtcp-mux.
RFC 5761 5.1.3:
   If it is desired to use both ICE and multiplexed RTP and RTCP, the
   initial offer MUST contain an "a=rtcp-mux" attribute to indicate that
   RTP and RTCP multiplexing is desired and MUST contain "a=candidate:"
   lines for both RTP and RTCP along with an "a=rtcp:" line indicating a
   fallback port for RTCP in the case that the answerer does not support
   RTP and RTCP multiplexing.  This MUST be done for each media where
   RTP and RTCP multiplexing is desired.

So, per the spec we're missing a=rtcp, and so a non-rtcp-mux-supporting answerer must assume that the rtcp port = rtp+1.   However, the part you quote if for a non-ICE-aware answerer; WebRTC mandates support for ICE (or at least ICE-lite).  The relevant portion of rfc 5245 appears to be 4.3 (Encoding the SDP):

   If the agent is utilizing RTCP, it MUST encode
   the RTCP candidate using the a=rtcp attribute as defined in RFC 3605
   [RFC3605].  If RTCP is not in use, the agent MUST signal that using
   b=RS:0 and b=RR:0 as defined in RFC 3556 [RFC3556].

So that's a clear miss on the spec for our current implementation.  We must include a=rtcp: as well as rtcp-mux
Status: UNCONFIRMED → NEW
Ever confirmed: true
So it would be set with:
  (void) sdp_add_new_attr(sdp_p, level, 0, SDP_ATTR_RTCP, &inst_num);
  (void) sdp_attr_set_simple_u32(sdp_p, SDP_ATTR_FMTP, level, 0, inst_num, port);

Currently, there's no code that sets a=rtcp: anywhere (and really no code that reads it), though it gets parsed I think.
Summary: Generation of incorrect SDP causing ICE-Mismatch on Mac systems in WebRTC → WebRTC SDP does not include required a=rtcp:<port>
first cut at tracking flags.

Note this really only affects certain interop scenarios where the final target doesn't support a=rtcp-mux or doesn't want to use it, and they don't let the ICE candidate override the missing a=rtcp.
OS: Mac OS X → All
Hardware: x86 → All
Seems like the spec question has been cleared up. This will be an easy fix with the JSEP rewrite in bug 1091242, and a bit more work without the rewrite. Sipcc does not support the notion of "end of ICE" at all, which is necessary for setting the default candidate correctly, so we would need to create new API. This might not be too awful since most of the wrapper code is out of the way now.
Flags: needinfo?(docfaraday)
If interoping with an implementation that supports ICE, it will never use the value in the a=rtcp attribute, and if interoping with an implementation that does not support ICE, the offer is not complete until gathering is done. Therefore, once gathering is done, the a=rtcp attribute is set (or should be; is that not what you're seeing?).
Flags: needinfo?(pgpb.padilla)
For Firefox on OS X, what we see is that the `a=rtcp<port>` line is not present event after ICE gathering.

For Firefox on Windows 8, what we see is that the `a=rtcp<port>` is present, in fact we have no issues when we work on Windows, only under OS X.
Flags: needinfo?(pgpb.padilla)
That's _really_ strange... I'll have to look into that.
Ok, I thought we had implemented this, but apparently not.
Attached file MozReview Request: bz://1096795/bwc (obsolete) —
/r/8445 - Bug 1096795: (WIP) Put a=rtcp in SDP when gathering ends.

Pull down this commit:

hg pull -r f3d4f01f236bfd514a2531c5109ea58306a08d7f https://reviewboard-hg.mozilla.org/gecko/
Assignee: nobody → docfaraday
https://reviewboard.mozilla.org/r/8443/#review7135

::: media/mtransport/nricemediastream.h:139
(Diff revision 1)
>    // TODO(bug 1096795): This needs to take a component number, so we can get

You'll want to remove this TODO.

::: media/webrtc/signaling/src/jsep/JsepSession.h:134
(Diff revision 1)
> +      const std::string& defaultCandidateAddr,
> -                                        uint16_t defaultCandidatePort,
> +      uint16_t defaultCandidatePort,

Suggest rename these to RTP

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:855
(Diff revision 1)
> +      stream->GetDefaultCandidate(&rtcpCandidate, 2); // Optional, ignore errors

When would this happen? If you're muxing?
https://reviewboard.mozilla.org/r/8443/#review7201

> When would this happen? If you're muxing?

Yeah. I can make the comment more clear.
To make this testable, we'll need to make the rtcp attribute support in sipcc more complete (there's no support for parsing the optional stuff after the port number).
Comment on attachment 8603613 [details]
MozReview Request: bz://1096795/bwc

/r/8445 - Bug 1096795: Put a=rtcp in SDP when gathering ends.

Pull down this commit:

hg pull -r d6a631637f6b171b8486898e017f6a3a81004173 https://reviewboard-hg.mozilla.org/gecko/
https://reviewboard.mozilla.org/r/8443/#review7257

> Suggest rename these to RTP

Wouldn't exactly be true for datachannel.
Comment on attachment 8603613 [details]
MozReview Request: bz://1096795/bwc

/r/8445 - Bug 1096795: Put a=rtcp in SDP when gathering ends.

Pull down this commit:

hg pull -r 1c714d028eb46456922d3a19b67b7bac3bca756d https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8603613 [details]
MozReview Request: bz://1096795/bwc

Ran out of small review requests. Have a few big ones though...
Attachment #8603613 - Flags: review?(martin.thomson)
Comment on attachment 8603613 [details]
MozReview Request: bz://1096795/bwc

https://reviewboard.mozilla.org/r/8443/#review7363

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:854
(Diff revision 3)
> +      rtcpCandidate.cand_addr.port = 0; // Avoid valgrind warnings if no rtcp

Move that to GetDefaultCandidate methinks.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:856
(Diff revision 3)
> +      stream->GetDefaultCandidate(2, &rtcpCandidate);

It sort of bothers me that the return value isn't checked here and that you are relying on the value of rtcpCandidate being left in a usable state.

::: media/webrtc/signaling/src/sdp/SdpAttribute.cpp:200
(Diff revision 3)
> +      !mAddress.empty()) {

This bothers me too.  I'd rather we had just one way to signal that there is no address, rather than three, as this implies.

I'd rather we have a consistent set of values: mNetType and mAddrType both being none and mAddress being empty OR good values in all of them.  An assert to this effect in the constructor might be nice.

::: media/webrtc/signaling/test/jsep_session_unittest.cpp:629
(Diff revision 3)
> +    session.AddRemoteIceCandidate(kAEqualsCandidate + kRtcpCandidates[0], "", 0);
> +    session.AddRemoteIceCandidate(kAEqualsCandidate + kRtcpCandidates[1], "", 0);
> +    session.AddRemoteIceCandidate(kAEqualsCandidate + kRtcpCandidates[2], "", 0);
> +
> +    session.AddRemoteIceCandidate(kAEqualsCandidate + kRtcpCandidates[3], "", 1);
> +    session.AddRemoteIceCandidate(kAEqualsCandidate + kRtcpCandidates[4], "", 1);
> +    session.AddRemoteIceCandidate(kAEqualsCandidate + kRtcpCandidates[5], "", 1);

for (size_t i = 0; ...
Attachment #8603613 - Flags: review?(martin.thomson)
Attachment #8603613 - Flags: review+
Comment on attachment 8603613 [details]
MozReview Request: bz://1096795/bwc

https://reviewboard.mozilla.org/r/8443/#review7373

Ship It!
https://reviewboard.mozilla.org/r/8443/#review7381

> Move that to GetDefaultCandidate methinks.

I'm keeping this here, in an if (NS_FAILED(GetDefaultCandidate(...))), since in this area of the code the convention is to not have init happen in functions that have failed.
Attachment #8603613 - Flags: review+
Comment on attachment 8603613 [details]
MozReview Request: bz://1096795/bwc

/r/8445 - Bug 1096795: Put a=rtcp in SDP when gathering ends.

Pull down this commit:

hg pull -r 1da4cd3653e68747d7e9053dc1231ae5d55f1743 https://reviewboard-hg.mozilla.org/gecko/
https://reviewboard.mozilla.org/r/8443/#review7383

> I'm keeping this here, in an if (NS_FAILED(GetDefaultCandidate(...))), since in this area of the code the convention is to not have init happen in functions that have failed.

After is fine; though I'd request that you also set the address value as well, just to make it explicit (and since you are relying on it being the empty string as a signal elsewhere).
https://reviewboard.mozilla.org/r/8443/#review7387

> After is fine; though I'd request that you also set the address value as well, just to make it explicit (and since you are relying on it being the empty string as a signal elsewhere).

Yep, I did that too.
Check back on try.
Flags: needinfo?(docfaraday)
Flags: needinfo?(docfaraday)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c4b4d5aaa2d2
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Byron - given the size and impact of this patch, I think this is something we need for Fx 40 - what do you think?
Flags: needinfo?(docfaraday)
This is maybe not super-urgent, since we've never put a=rtcp in our SDP, and the set of things that require it is kinda small. But, since we're early in the cycle, I'd be ok with requesting uplift.
Flags: needinfo?(docfaraday)
Thanks, Byron.  I'm getting feedback that this is actually important for certain web devs; so I'd like to uplift this to Fx40 this week.  Can you write the uplift request?
Flags: needinfo?(docfaraday)
Comment on attachment 8603613 [details]
MozReview Request: bz://1096795/bwc

Approval Request Comment
[Feature/regressing bug #]:

   Not a regression, has been around since the beginning.

[User impact if declined]:

   Some webrtc gateway services either won't work, or will work poorly because no RTCP is flowing.

[Describe test coverage new/current, TreeHerder]:

   We've added some unit-test and mochitest code to check that the attribute is present and takes the expected value.

[Risks and why]: 

   Fairly low, the changes were not all that invasive.

[String/UUID change made/needed]:

   None.
Flags: needinfo?(docfaraday)
Attachment #8603613 - Flags: approval-mozilla-aurora?
If I understand correctly, we don't want that in beta/39.
Attachment #8603613 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
After testing the fix on Firefox Developer version 40.0a2 we found the rtcp:<port> is present in the offer and answer sdp after ice candidate . But there are no UDP packets flowing between the clients after offer and answer. 

We noticed rtcp:<port> IN <IP> on Create Answer has wrong ports and IP (check rfc3605) and m=audio <port> also doesn't match the rtcp ports 


Application Basics
------------------
Name: Firefox
Version: 40.0a2
Build ID: 20150607004009
Update Channel: aurora
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:40.0) Gecko/20100101 Firefox/40.0
Multiprocess Windows: 0/1 (default: false)

-----------------------------------------
Expected Result

m=audio 60952 RTP/SAVPF 109

c=IN IP4 A.B.C.D

a=candidate:0 1 UDP 2130379007 A.B.C.D 60952 typ host

a=candidate:0 2 UDP 2130379006 A.B.C.D 63082 typ host


a=rtcp:63082 IN IP4 A.B.C.D

-----------------------------------------

Actual Result (After createAnswer and waiting for gathering): 
  
m=audio 36419 RTP/SAVPF 109

c=IN IP4 1.2.3.4

a=candidate:0 1 UDP 2130379007 A.B.C.D 60952 typ host

a=candidate:0 2 UDP 2130379006 A.B.C.D 63082 typ host


a=rtcp:39901 IN IP4 1.2.3.4

-----------------------------------------------
Flags: needinfo?(docfaraday)
Can you post the full SDP? Are there any srflx or relay candidates in the final SDP?
Flags: needinfo?(arun3528)
v=0

o=mozilla...THIS_IS_SDPARTA-40.0a2 5623678862762364550 0 IN IP4 0.0.0.0

s=-

t=0 0

a=sendrecv

a=fingerprint:sha-256 FF:1C:98:59:54:48:F0:EF:0E:FA:B8:79:59:90:9F:A5:93:4D:D7:29:EB:C5:27:E3:B7:69:30:7D:FF:E1:43:FB

a=ice-options:trickle

a=msid-semantic:WMS *

m=audio 29219 RTP/SAVPF 109

c=IN IP4 166.176.186.xxx

a=candidate:0 1 UDP 2130379007 192.168.43.xx 54606 typ host

a=candidate:0 2 UDP 2130379006 192.168.43.xx 57809 typ host

a=sendrecv

a=end-of-candidates

a=ice-pwd:535da00e9ee3a34cb05d1b623352fffd

a=ice-ufrag:29c85806

a=mid:sdparta_0

a=msid:7c972fde-aba4-c943-8123-809b9adddc6d 0e83961a-b22d-dd4b-8e62-a3edb0c36b12

a=rtcp:34211 IN IP4 166.176.186.xxx

a=rtpmap:109 opus/48000/2

a=setup:active

a=ssrc:1922531597 cname:{48d1e873-74a7-b14b-bdb6-321fe0c4064d}

m=video 14896 RTP/SAVPF 120

c=IN IP4 166.176.186.xxx

a=candidate:0 1 UDP 2130379007 192.168.43.xx 53638 typ host

a=candidate:0 2 UDP 2130379006 192.168.43.xx 52911 typ host

a=sendrecv

a=end-of-candidates

a=fmtp:120 max-fs=12288;max-fr=60

a=ice-pwd:535da00e9ee3a34cb05d1b623352fffd

a=ice-ufrag:29c85806

a=mid:sdparta_1

a=msid:7c972fde-aba4-c943-8123-809b9adddc6d c4c8e229-3a00-2949-b5ec-242d6ea8e381

a=rtcp:54058 IN IP4 166.176.186.xxx

a=rtcp-fb:120 nack

a=rtcp-fb:120 nack pli

a=rtcp-fb:120 ccm fir

a=rtpmap:120 VP8/90000

a=setup:active

a=ssrc:151008062 cname:{48d1e873-74a7-b14b-bdb6-321fe0c4064d}
Flags: needinfo?(docfaraday)
Flags: needinfo?(arun3528)
There are no srflx or relay candidates on ice Candidate gathering complete
Flags: needinfo?(docfaraday)
What service is this on, so I can attempt to reproduce?
Hi Byran ,   
            The port looks good now after retesting but still the IPs are different for c line and candidates. And there are no UDP packets sent or received from the client . Please find below the sample app for testing 

https://www.attwebrtc.com/dev1/firefoxdemo/


Step to make a Call
1) Open two browser tabs

2) Click on Account ID users on the Login screen . Give random userid and click login. example alice and bob

3) on successfull login you will be shown home screen.

4)From one of the client click "Call" button and enter the other user login in other tab example : bob@attwebrtc.com and click dial

5) bob will recive a ring tone with button to accept a Call. accept the call by clicking green button.

6) check about: webrtc for the localsdp for bob. And if possible check wireshark for UDP packets


Hope these details will help us to find the issue  .
Flags: needinfo?(docfaraday)
Attachment #8603613 - Attachment is obsolete: true
I'm having no luck reproducing the specific SDP problem you're seeing, but I think I know what might be going on. We are gathering only peer reflexive candidates, and no server reflexives. I think we must have a bug that prevents peer reflexives from being incorporated into the local SDP. If/when we get a response form the STUN server, the address it tells us is a duplicate of the peer reflexive discovered earlier, and is pruned, and doesn't make it into the SDP either. Then, gathering finishes, and the defaults are set on the c line, m-line, and rtcp attr, and since peer reflexive is more likely to work than host, they are the defaults.
The primary reason you aren't seeing media flow is because the DTLS handshake is failing, probably because the cipher suites you support do not have perfect forward secrecy (https://hacks.mozilla.org/2015/02/webrtc-requires-perfect-forward-secrecy-pfs-starting-in-firefox-38/).
Note here that Firefox 38 does not in fact require PFS, because the change was actually delayed (in case you are wondering why this works with Firefox 38).

You really need to enable the mandatory cipher suites for WebRTC: http://rtcweb-wg.github.io/security-arch/#sec.proposal.comsec currently says TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA with a recommendation for the GCM variant of the same.  This is (highly) likely to be TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA in an upcoming version.
on what version of firefox will  TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA support be added as we tested the in developer version 40 and nightly it doesnt work . 

can we do anything from the client (javascript) or the application server side to make it work ?

do you need any traces or logs from our side to fix peer reflexive candidates IP issues?
Flags: needinfo?(docfaraday)
If that is the only cipher suite you offer, then we will currently be unable to support it.  Currently, we provide RSA certificates.  That means that we will accept ECDSA cipher suites but only if you also include _RSA_ suites (when you are the client) or RSA options in the signature_algorithms extension (when you are the server).  The latter probably requires DTLS 1.2; I see that you only have DTLS 1.0 enabled (are you on OpenSSL or BoringSSL by any chance?).

I have an implementation of the certificate management API, which will allow you to control what we use and offer.  That will come with a switch to using ECDSA certificates by default.  That's likely to be in Firefox 42 at this point for various boring reasons related to gecko internals.
Flags: needinfo?(docfaraday)
(In reply to arun3528 from comment #46)
> on what version of firefox will  TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA
> support be added as we tested the in developer version 40 and nightly it
> doesnt work . 
> 
> can we do anything from the client (javascript) or the application server
> side to make it work ?
> 
> do you need any traces or logs from our side to fix peer reflexive
> candidates IP issues?

   Just tried reproducing. One tab ends up being an offerer, and therefore the DTLS server, and the other is an answerer, and therefore the DTLS client. On the PeerConnection that is a DTLS server, I don't see any DTLS Client Hello from your end. On the PeerConnection that is a DTLS client, we send a Client Hello, but it never gets a response.
Hang on, the PeerConnection that is a DTLS server does get a Client Hello, but it has the same cipher suites as before.
so the DTLS client sends a "client hello" to DTLS server (offerer) but the DTLS server doesn't receive it.
is it because 

1) Is it because the Browser is not able to receive the "client hello" due to platform in between .  or
2) The browser is not able to decode it because ECDSA is not supported ?
The Client Hello from your server has only TLS_RSA_WITH_AES_128_CBC_SHA and TLS_EMPTY_RENEGOTIATION_INFO_SCSV (not actually a cipher), which we reject.
You need to log in before you can comment on or make changes to this bug.