No a=sctpmap for datachannel in SDP

RESOLVED FIXED in mozilla27

Status

()

Core
WebRTC: Networking
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jesup, Assigned: ehugg)

Tracking

22 Branch
mozilla27
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [webrtc])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 804857 [details]
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

5 years ago
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).

Updated

5 years ago
Summary: No a=fmtp for datachannel in SDP answers → No a=sctpmap for datachannel in SDP

Comment 2

5 years ago
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.
(Reporter)

Comment 3

5 years ago
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.
(Assignee)

Comment 4

5 years ago
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.
(Reporter)

Comment 5

5 years ago
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.
(Assignee)

Comment 6

5 years ago
Created attachment 807981 [details] [diff] [review]
use  line for datachannels
(Assignee)

Updated

5 years ago
Attachment #807981 - Attachment is obsolete: true
(Assignee)

Comment 7

5 years ago
Created attachment 808830 [details] [diff] [review]
use sctpmap line for datachannels
targeting for Firefox 27
Target Milestone: --- → mozilla27
(Assignee)

Comment 9

5 years ago
Created attachment 808849 [details] [diff] [review]
use sctpmap line for datachannels
(Assignee)

Updated

5 years ago
Attachment #808830 - Attachment is obsolete: true
(Assignee)

Comment 10

5 years ago
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.
Attachment #808849 - Flags: review?(adam)
(Reporter)

Comment 11

5 years ago
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
Attachment #808849 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #808849 - Flags: review?(adam)

Comment 12

5 years ago
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

5 years ago
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.
Attachment #808849 - Flags: feedback+
https://hg.mozilla.org/mozilla-central/rev/784b1e8abd5b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.