916429 - No a=sctpmap for datachannel in SDP

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

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

Attachment: data_test.html

My data_test.html program shows we're not adding a=ftmp's for datachannel in an answer; please check the draft - I suspect we should be, but maybe I'm wrong on that.

Comment 1

[Adam Roach [:abr]]

To make this easier to read; the relevant portion of the offer in this test case is:

> m=application 57640 DTLS/SCTP 5000
> c=IN IP4 99.152.145.110
> a=fmtp:5000 protocol=webrtc-datachannel;streams=16
> a=sendrecv
> a=candidate:0 1 UDP 2113405183 192.168.0.185 57640 typ host
> a=candidate:1 1 UDP 1694040063 99.152.145.110 57640 typ srflx raddr 192.168.0.185 rport 57640
> a=candidate:0 2 UDP 2113405182 192.168.0.185 49371 typ host
> a=candidate:1 2 UDP 1694040062 99.152.145.110 49371 typ srflx raddr 192.168.0.185 rport 49371

And the answer is:

> m=application 54170 DTLS/SCTP 5001
> c=IN IP4 99.152.145.110
> a=sendrecv
> a=candidate:0 1 UDP 2113405183 192.168.0.185 54170 typ host
> a=candidate:1 1 UDP 1694040063 99.152.145.110 54170 typ srflx raddr 192.168.0.185 rport 54170
> a=candidate:0 2 UDP 2113405182 192.168.0.185 58788 typ host
> a=candidate:1 2 UDP 1694040062 99.152.145.110 58788 typ srflx raddr 192.168.0.185 rport 58788

In practice, I think these are both not quite right; I think the binding to protocols is supposed to use sctpmap (cf. http://tools.ietf.org/html/draft-ietf-mmusic-sctp-sdp-04).

Comment 2

[Adam Roach [:abr]]

I'll note that on the RTCWEB call today (September 18), there was not universal agreement around the use of sctpmap versus fmtp. We need to track JSEP to make sure that our ultimate behavior conforms; in the meanwhile, we should coordinate with Chrome to make sure we don't break interop by removing fmtp in a way that might make it sad.

Comment 3

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

The draft does say a=sctpmap.  I know I showed examples in presentations showing sample SDP that had fmtp, but I forget when/why salvatore changed it - it might have changed around the whole 3-way vs 1-way issue (Boston).  I think I kept a=fmtp in my presentations, so maybe that's where it comes from.  I think this showed up in Salvatore's draft around last October.

I think the worst ramification is that we won't start an association with the same idea for the number of streams - but we auto-increase max streams, so it's not a big problem.  I'm pretty sure we don't check for a=fmtp being there currently.

Comment 4

[Ethan Hugg [:ehugg]]

I'm looking at draft-ietf-mmusic-sctp-sdp-04 and it looks like instead of 

a=fmtp:5000 protocol=webrtc-datachannel;streams=16

we should have

a=sctpmap:5000 webrtc-datachannel 16

In the offer, and presumably in our case the same in the answer.  I am not planning on enforcing this from the remote side it seems we currently accept anything.


Also, I noticed section 9.3 of this document has lines like this:

a=webrtc-DataChannel:5000 stream:2;label="channel 2";subprotocol="file transfer"

I was not planning to add this as part of this bug, but if Firefox needs these declared we should make a new bug for it.

Please let me know if you disagree.

Comment 5

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

We agreed (in rtcweb) to get rid of the SDP channel negotiation, so those should go away (a=webrtc-datachannel)

Sounds good.  If the number of streams isn't read out correctly, that's ok - it auto-extends when needed.

Comment 6

[Ethan Hugg [:ehugg]]

Attachment: use line for datachannels

Comment 7

[Ethan Hugg [:ehugg]]

Attachment: use sctpmap line for datachannels

Comment 8

[Maire Reavy [:mreavy]]

targeting for Firefox 27

Comment 9

[Ethan Hugg [:ehugg]]

Attachment: use sctpmap line for datachannels

Comment 10

[Ethan Hugg [:ehugg]]

Comment on attachment 808849 [details] [diff] [review]
use sctpmap line for datachannels

Review of attachment 808849 [details] [diff] [review]:
-----------------------------------------------------------------

a=sctpmap line appears now on both offer and answer.  I took the datachannel parameters out of the fmtp attribute struct and put them in a new sctpmap one.  Also removed the sendrecv line for datachannels.

Draft is here - http://tools.ietf.org/html/draft-ietf-mmusic-sctp-sdp-03

Jesup noted that the "a=webrtc-DataChannel" lines in that draft are unlikely to be in the final and should be ignored for now.

Comment 11

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

Comment on attachment 808849 [details] [diff] [review]
use sctpmap line for datachannels

Review of attachment 808849 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr.c
@@ +2133,5 @@
> +
> +sdp_result_e sdp_build_attr_sctpmap(sdp_t *sdp_p, sdp_attr_t *attr_p,
> +                                    flex_string *fs)
> +{
> +    MOZ_ASSERT(strlen(attr_p->attr.sctpmap.protocol) > 0);

Just verify that MOZ_ASSERT is correct here and this couldn't happen due to ugly/wrong SDP

Comment 12

[Adam Roach [:abr]]

Comment on attachment 808849 [details] [diff] [review]
use sctpmap line for datachannels

Quick formatting nit on the patch: please remove tabs from lines 6939 and 6967 of sdp_attr_access.c

Comment 13

[Adam Roach [:abr]]

Comment on attachment 808849 [details] [diff] [review]
use sctpmap line for datachannels

Review of attachment 808849 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't do as careful a review as I would feel necessary to give this an r+ -- use jesup's instead -- but I did read through all of it quickly. Architecturally, it's exactly what I would expect.

Comment 14

[Ethan Hugg [:ehugg]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/784b1e8abd5b

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/784b1e8abd5b