Closed Bug 786152 Opened 12 years ago Closed 12 years ago

Implement SDP for WebRTC DataChannels

Categories

(Core :: WebRTC: Signaling, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: jesup, Assigned: emannion)

References

Details

(Whiteboard: [WebRTC], [blocking-webrtc+], [qa-])

Attachments

(2 files, 6 obsolete files)

See draft-ietf-mmusic-sctp-sdp, but since that doesn't define enough for our use-case.

Proposal that allows other protocols to be used (in theory) and allows even multiple associations on different virtual ports of the DTLS connection.

m=application 2345 SCTP/DTLS 5000
a=fmtp:5000 protocol=webrtc-datachannel;streams=32
a=<dtls certificate>

5000 in this case is the virtual "port" sctp uses internally on DTLS.  Note that currently an answer MUST use a different 'port' than the offer.
Whiteboard: [WebRTC], [blocking-webrtc+]
A few questions before implementation

Will I follow this RFC exactly when adding attributes and making changes to m lines.
http://datatracker.ietf.org/doc/draft-ietf-mmusic-sctp-sdp/?include_text=1


examples being:

m=image 54111 SCTP *
c=IN IP4 192.0.2.2
a=setup:passive
a=connection:new


Is it possible to use the a= fingerprint attribute if it only gets offered at the SDP session level.
I added the stub function to VcmSipccBinding.cpp

vcmSetDataChannelParameters(const char *peerconnection, cc_uint16_t streams, int sctp_port, char* protocol)

This is for you to fill out into it I pass the negotiated Data Channel SDP attributes.

Here is a sample of offer answer with Data Channel SDP added.

Init Complete
onCreateOfferSuccess = v=0
o=Cisco-SIPUA 21115 0 IN IP4 0.0.0.0
s=SIP Call
t=0 0
a=ice-ufrag:656f6a95
a=ice-pwd:4175f8d2fdf0cf765eb38e90c5f0144f
a=fingerprint:sha-1 7d:5a:f7:90:30:43:eb:f5:11:3f:ba:ac:85:24:58:b6:7a:a7:be:1f
m=audio 59382 RTP/SAVPF 109 0 8 101
c=IN IP4 79.97.215.79
a=rtpmap:109 opus/48000
a=ptime:20
a=rtpmap:0 PCMU/8000
a=rtpmap:8 PCMA/8000
a=rtpmap:101 telephone-event/8000
a=fmtp:101 0-15
a=sendrecv
a=candidate:0 1 UDP 2113667327 192.168.1.102 59382 typ host
a=candidate:1 1 UDP 1694302207 79.97.215.79 59382 typ srflx raddr 192.168.1.102 rport 59382
a=candidate:0 2 UDP 2113667326 192.168.1.102 52404 typ host
a=candidate:1 2 UDP 1694302206 79.97.215.79 52404 typ srflx raddr 192.168.1.102 rport 52404
m=video 65513 RTP/SAVPF 120
c=IN IP4 79.97.215.79
a=rtpmap:120 VP8/90000
a=sendrecv
a=candidate:0 1 UDP 2113667327 192.168.1.102 65513 typ host
a=candidate:1 1 UDP 1694302207 79.97.215.79 65513 typ srflx raddr 192.168.1.102 rport 65513
a=candidate:0 2 UDP 2113667326 192.168.1.102 60684 typ host
a=candidate:1 2 UDP 1694302206 79.97.215.79 60684 typ srflx raddr 192.168.1.102 rport 60684
m=application 61389 SCTP/DTLS 5000 
c=IN IP4 79.97.215.79
a=fmtp:5000 protocol=webrtc-datachannel;streams=16
a=sendrecv
a=candidate:0 1 UDP 2113667327 192.168.1.102 61389 typ host
a=candidate:1 1 UDP 1694302207 79.97.215.79 61389 typ srflx raddr 192.168.1.102 rport 61389
a=candidate:0 2 UDP 2113667326 192.168.1.102 61436 typ host
a=candidate:1 2 UDP 1694302206 79.97.215.79 61436 typ srflx raddr 192.168.1.102 rport 61436

OnAddStream called hints=1
OnAddStream called hints=2
onCreateAnswerSuccess = v=0
o=Cisco-SIPUA 7401 0 IN IP4 0.0.0.0
s=SIP Call
t=0 0
a=ice-ufrag:118120bc
a=ice-pwd:1a44ad12c6c6215145dca205c5316932
a=fingerprint:sha-1 ba:c7:f6:c0:30:bd:61:74:b7:e7:8d:e2:92:74:44:ad:17:a1:1f:50
m=audio 59279 RTP/SAVPF 109 101
c=IN IP4 79.97.215.79
a=rtpmap:109 opus/48000
a=ptime:20
a=rtpmap:101 telephone-event/8000
a=fmtp:101 0-15
a=sendrecv
a=candidate:0 1 UDP 2113667327 192.168.1.102 59279 typ host
a=candidate:1 1 UDP 1694302207 79.97.215.79 59279 typ srflx raddr 192.168.1.102 rport 59279
a=candidate:0 2 UDP 2113667326 192.168.1.102 53298 typ host
a=candidate:1 2 UDP 1694302206 79.97.215.79 53298 typ srflx raddr 192.168.1.102 rport 53298
m=video 60408 RTP/SAVPF 120
c=IN IP4 79.97.215.79
a=rtpmap:120 VP8/90000
a=sendrecv
a=candidate:0 1 UDP 2113667327 192.168.1.102 60408 typ host
a=candidate:1 1 UDP 1694302207 79.97.215.79 60408 typ srflx raddr 192.168.1.102 rport 60408
a=candidate:0 2 UDP 2113667326 192.168.1.102 54183 typ host
a=candidate:1 2 UDP 1694302206 79.97.215.79 54183 typ srflx raddr 192.168.1.102 rport 54183
m=application 60878 SCTP/DTLS 5000 
c=IN IP4 79.97.215.79
a=fmtp:5000 protocol=webrtc-datachannel;streams=16
a=sendrecv
a=candidate:0 1 UDP 2113667327 192.168.1.102 60878 typ host
a=candidate:1 1 UDP 1694302207 79.97.215.79 60878 typ srflx raddr 192.168.1.102 rport 60878
a=candidate:0 2 UDP 2113667326 192.168.1.102 49970 typ host
a=candidate:1 2 UDP 1694302206 79.97.215.79 49970 typ srflx raddr 192.168.1.102 rport 49970
Attachment #659767 - Flags: feedback?(rjesup)
Attachment #659767 - Flags: feedback?(ethanhugg)
Comment on attachment 659767 [details] [diff] [review]
Data Channel support added to SDP

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

Going to build with -Werror on Linux.  Will post again if any errors.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +421,5 @@
>    mIceCtx->SignalGatheringCompleted.connect(this, &PeerConnectionImpl::IceGatheringCompleted);
>    mIceCtx->SignalCompleted.connect(this, &PeerConnectionImpl::IceCompleted);
>  
>    // Create two streams to start with, assume one for audio and
>    // one for video

Might want to update this comment.

::: media/webrtc/signaling/src/sipcc/core/gsm/lsm.c
@@ +5348,5 @@
> +    fsmdef_dcb_t   *dcb;
> +    lsm_lcb_t *lcb;
> +
> +    lcb = lsm_get_lcb_by_call_id(call_id);
> +    if (lcb != NULL) {

if (lcb)

@@ +5350,5 @@
> +
> +    lcb = lsm_get_lcb_by_call_id(call_id);
> +    if (lcb != NULL) {
> +        dcb = lcb->dcb;
> +        if (dcb == NULL) {

if (!dcb)

::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr.c
@@ +2637,5 @@
> +          if (fmtp_p->protocol[0] != '\0') {
> +              if (semicolon) {
> +                  *ptr += snprintf(*ptr, MAX((endbuf_p - *ptr), 0), ";protocol=%s",
> +                                   attr_p->attr.fmtp.protocol);
> +                  semicolon = TRUE;

This seems unnecessary should already be true.

@@ +2648,5 @@
> +
> +         if (fmtp_p->streams > 0) {
> +    	    if (semicolon) {
> +                    *ptr += snprintf(*ptr, MAX((endbuf_p - *ptr), 0), ";streams=%u",attr_p->attr.fmtp.streams);
> +                     semicolon = TRUE;

same here

::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr_access.c
@@ +6679,5 @@
> +{
> +    sdp_t       *sdp_p = (sdp_t *)sdp_ptr;
> +    sdp_attr_t  *attr_p;
> +
> +    if (sdp_verify_sdp_ptr(sdp_p) == FALSE) {

moz standard is if (! here

@@ +6684,5 @@
> +        return (SDP_INVALID_SDP_PTR);
> +    }
> +
> +    attr_p = sdp_find_attr(sdp_p, level, cap_num, SDP_ATTR_FMTP, inst_num);
> +    if (attr_p == NULL) {

if (!attr_p)

@@ +6710,5 @@
> +        return (SDP_INVALID_PARAMETER);
> +    }
> +
> +    attr_p = sdp_find_attr(sdp_p, level, cap_num, SDP_ATTR_FMTP, inst_num);
> +    if (attr_p == NULL) {

if (!attr_p)

@@ +6743,5 @@
> +        return (SDP_INVALID_PARAMETER);
> +    }
> +
> +    attr_p = sdp_find_attr(sdp_p, level, cap_num, SDP_ATTR_FMTP, inst_num);
> +    if (attr_p == NULL) {

if (!attr_p)

@@ +6769,5 @@
> +
> +    sdp_t       *sdp_p = (sdp_t *)sdp_ptr;
> +    sdp_attr_t  *attr_p;
> +
> +    if (sdp_verify_sdp_ptr(sdp_p) == FALSE) {

moz standard would use ! instead of == FALSE

::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_token.c
@@ +1433,5 @@
> +            return (SDP_INVALID_PARAMETER);
> +        }
> +        port_ptr = port;
> +
> +        if (sdp_getchoosetok(port_ptr, &port_ptr, "/ \t", &result) == TRUE) {

Remove the == TRUE for moz standard.

@@ +1625,4 @@
>          }
> +    } else {
> +        /* Add port if SCTP/DTLS */
> +    	*ptr += snprintf(*ptr, MAX((endbuf_p - *ptr), 0), " %u ", (u16)mca_p->sctpport);

Here we are casting to a U16, wouldn't snprintf be expecting a U32?

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +29,5 @@
>  #include "mtransport_test_utils.h"
>  MtransportTestUtils test_utils;
>  
> +//static int kDefaultTimeout = 5000;
> +static int kDefaultTimeout = 500000;

This seems like a very long extension to the timeout, are you sure we want this?
Attachment #659767 - Flags: feedback?(ethanhugg) → feedback+
Comment on attachment 659767 [details] [diff] [review]
Data Channel support added to SDP


With -Werror on Linux

/home/ehugg/mozilla/alder/default/media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c: In function ‘gsmsdp_negotiate_datachannel_attribs’:
/home/ehugg/mozilla/alder/default/media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c:2916:2: error: implicit declaration of function ‘sdp_attr_get_fmtp_streams’ [-Werror=implicit-function-declaration]
/home/ehugg/mozilla/alder/default/media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c:2920:5: error: implicit declaration of function ‘sdp_attr_get_fmtp_data_channel_protocol’ [-Werror=implicit-function-declaration]
/home/ehugg/mozilla/alder/default/media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c:2920:21: error: assignment makes pointer from integer without a cast [-Werror]
/home/ehugg/mozilla/alder/default/media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c: In function ‘gsmsdp_negotiate_media_lines’:
/home/ehugg/mozilla/alder/default/media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c:4159:26: error: implicit declaration of function ‘lsm_data_channel_negotiated’ [-Werror=implicit-function-declaration]
cc1: all warnings being treated as errors
Comment on attachment 659767 [details] [diff] [review]
Data Channel support added to SDP

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

Incomplete review to capture current thinking.

Also need in lsm.h:
+void lsm_data_channel_negotiated (line_t line, callid_t call_id, fsmdef_media_t *media, int *pc_stream

and in sdp.h:
+sdp_result_e 
+sdp_attr_get_fmtp_streams (void *sdp_ptr, u16 level,
+                           u8 cap_num, u16 inst_num, u32* val);
+
+sdp_result_e
+sdp_attr_set_fmtp_data_channel_protocol (void *sdp_ptr, u16 level,
+                                         u8 cap_num, u16 inst_num,
+                                         const char *protocol);
+
+char* sdp_attr_get_fmtp_data_channel_protocol (void *sdp_ptr, u16 level,
+                                               u8 cap_num, u16 inst_num);
+

::: media/webrtc/signaling/src/sipcc/core/gsm/lsm.c
@@ +5356,5 @@
> +            return;
> +        }
> +
> +        /* have access to media->streams, media->protocol, media->sctp_port */
> +

Code missing here.

::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr.c
@@ +460,5 @@
>      u16          usedtx = 0;
>      u16          stereo = 0;
>      u16          useinbandfec = 0;
>      u16          cbr = 0;
> +    u16          streams = 0;

int

It's used for atoi(), which returns an int.  But even that is wrong, it should be a long, and this should be lower:

             tok = tmp;
             tok++;
-            streams = atoi(tok);
-            if (streams < 0) {
+            streams = strtol(tok,newtok,10);
+            if (tok == newtok || errno != 0) {
                 if (sdp_p->debug_flag[SDP_DEBUG_WARNINGS]) {
                     SDP_WARN("%s Warning: Invalid streams specified for "

We don't want to accept stream values in anything except base 10, and atoi doesn't have an error return (of any type).
(In reply to Ethan Hugg [:ehugg] from comment #3)
> Comment on attachment 659767 [details] [diff] [review]
> Data Channel support added to SDP
> 
> Review of attachment 659767 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Going to build with -Werror on Linux.  Will post again if any errors.
> 
> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> @@ +421,5 @@
> >    mIceCtx->SignalGatheringCompleted.connect(this, &PeerConnectionImpl::IceGatheringCompleted);
> >    mIceCtx->SignalCompleted.connect(this, &PeerConnectionImpl::IceCompleted);
> >  
> >    // Create two streams to start with, assume one for audio and
> >    // one for video
> 
> Might want to update this comment.

Done.

> 
> ::: media/webrtc/signaling/src/sipcc/core/gsm/lsm.c
> @@ +5348,5 @@
> > +    fsmdef_dcb_t   *dcb;
> > +    lsm_lcb_t *lcb;
> > +
> > +    lcb = lsm_get_lcb_by_call_id(call_id);
> > +    if (lcb != NULL) {
> 
> if (lcb)

Done.

> 
> @@ +5350,5 @@
> > +
> > +    lcb = lsm_get_lcb_by_call_id(call_id);
> > +    if (lcb != NULL) {
> > +        dcb = lcb->dcb;
> > +        if (dcb == NULL) {
> 
> if (!dcb)
> 
> ::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr.c
> @@ +2637,5 @@
> > +          if (fmtp_p->protocol[0] != '\0') {
> > +              if (semicolon) {
> > +                  *ptr += snprintf(*ptr, MAX((endbuf_p - *ptr), 0), ";protocol=%s",
> > +                                   attr_p->attr.fmtp.protocol);
> > +                  semicolon = TRUE;
> 
> This seems unnecessary should already be true.

Done.

> 
> @@ +2648,5 @@
> > +
> > +         if (fmtp_p->streams > 0) {
> > +    	    if (semicolon) {
> > +                    *ptr += snprintf(*ptr, MAX((endbuf_p - *ptr), 0), ";streams=%u",attr_p->attr.fmtp.streams);
> > +                     semicolon = TRUE;
> 
> same here
> 
> ::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr_access.c
> @@ +6679,5 @@
> > +{
> > +    sdp_t       *sdp_p = (sdp_t *)sdp_ptr;
> > +    sdp_attr_t  *attr_p;
> > +
> > +    if (sdp_verify_sdp_ptr(sdp_p) == FALSE) {
> 
> moz standard is if (! here
> 
> @@ +6684,5 @@
> > +        return (SDP_INVALID_SDP_PTR);
> > +    }
> > +
> > +    attr_p = sdp_find_attr(sdp_p, level, cap_num, SDP_ATTR_FMTP, inst_num);
> > +    if (attr_p == NULL) {
> 
> if (!attr_p)

For now I left this as it is the style used throughout this file. But will change if you insist.

> 
> @@ +6710,5 @@
> > +        return (SDP_INVALID_PARAMETER);
> > +    }
> > +
> > +    attr_p = sdp_find_attr(sdp_p, level, cap_num, SDP_ATTR_FMTP, inst_num);
> > +    if (attr_p == NULL) {
> 
> if (!attr_p)
> 
> @@ +6743,5 @@
> > +        return (SDP_INVALID_PARAMETER);
> > +    }
> > +
> > +    attr_p = sdp_find_attr(sdp_p, level, cap_num, SDP_ATTR_FMTP, inst_num);
> > +    if (attr_p == NULL) {
> 
> if (!attr_p)
> 
> @@ +6769,5 @@
> > +
> > +    sdp_t       *sdp_p = (sdp_t *)sdp_ptr;
> > +    sdp_attr_t  *attr_p;
> > +
> > +    if (sdp_verify_sdp_ptr(sdp_p) == FALSE) {
> 
> moz standard would use ! instead of == FALSE
> 
> ::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_token.c
> @@ +1433,5 @@
> > +            return (SDP_INVALID_PARAMETER);
> > +        }
> > +        port_ptr = port;
> > +
> > +        if (sdp_getchoosetok(port_ptr, &port_ptr, "/ \t", &result) == TRUE) {
> 
> Remove the == TRUE for moz standard.

Done.

> 
> @@ +1625,4 @@
> >          }
> > +    } else {
> > +        /* Add port if SCTP/DTLS */
> > +    	*ptr += snprintf(*ptr, MAX((endbuf_p - *ptr), 0), " %u ", (u16)mca_p->sctpport);
> 
> Here we are casting to a U16, wouldn't snprintf be expecting a U32?

Done.

> 
> ::: media/webrtc/signaling/test/signaling_unittests.cpp
> @@ +29,5 @@
> >  #include "mtransport_test_utils.h"
> >  MtransportTestUtils test_utils;
> >  
> > +//static int kDefaultTimeout = 5000;
> > +static int kDefaultTimeout = 500000;
> 
> This seems like a very long extension to the timeout, are you sure we want
> this?

I do this to help debugging, changed back now.
(In reply to Randell Jesup [:jesup] from comment #5)
> Comment on attachment 659767 [details] [diff] [review]
> Data Channel support added to SDP
> 
> Review of attachment 659767 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Incomplete review to capture current thinking.
> 
> Also need in lsm.h:
> +void lsm_data_channel_negotiated (line_t line, callid_t call_id,
> fsmdef_media_t *media, int *pc_stream

Done.

> 
> and in sdp.h:
> +sdp_result_e 
> +sdp_attr_get_fmtp_streams (void *sdp_ptr, u16 level,
> +                           u8 cap_num, u16 inst_num, u32* val);
> +
> +sdp_result_e
> +sdp_attr_set_fmtp_data_channel_protocol (void *sdp_ptr, u16 level,
> +                                         u8 cap_num, u16 inst_num,
> +                                         const char *protocol);
> +
> +char* sdp_attr_get_fmtp_data_channel_protocol (void *sdp_ptr, u16 level,
> +                                               u8 cap_num, u16 inst_num);
> +
> 
> ::: media/webrtc/signaling/src/sipcc/core/gsm/lsm.c
> @@ +5356,5 @@
> > +            return;
> > +        }
> > +
> > +        /* have access to media->streams, media->protocol, media->sctp_port */
> > +
> 
> Code missing here.

Incuded in revised patch.

> 
> ::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr.c
> @@ +460,5 @@
> >      u16          usedtx = 0;
> >      u16          stereo = 0;
> >      u16          useinbandfec = 0;
> >      u16          cbr = 0;
> > +    u16          streams = 0;
> 
> int
> 
> It's used for atoi(), which returns an int.  But even that is wrong, it
> should be a long, and this should be lower:
> 
>              tok = tmp;
>              tok++;
> -            streams = atoi(tok);
> -            if (streams < 0) {
> +            streams = strtol(tok,newtok,10);
> +            if (tok == newtok || errno != 0) {
>                  if (sdp_p->debug_flag[SDP_DEBUG_WARNINGS]) {
>                      SDP_WARN("%s Warning: Invalid streams specified for "
> 
> We don't want to accept stream values in anything except base 10, and atoi
> doesn't have an error return (of any type).

I did not implement this for now, will do this in a separate patch that will replace all atoi calls.
Attachment #659767 - Attachment is obsolete: true
Attachment #659767 - Flags: feedback?(rjesup)
Attachment #660053 - Flags: feedback?(rjesup)
Attachment #660053 - Flags: feedback?(ethanhugg)
Comment on attachment 660053 [details] [diff] [review]
Resed: Data Channel support added to SDP in SIPCC

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

incomplete review - up through lsm.h.  WIll continue later

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +994,5 @@
> +/* Set negotiated DataChannel parameters.
> + *
> + *  @param[in]  peerconnection - the peerconnection in use
> + *  @param[in]  ufrag - the ufrag
> + *  @param[in]  pwd - the pwd

Comments are wrong - cut and paste

@@ +1009,5 @@
> +  PR_ASSERT(pc);
> +  if (!pc) {
> +    return VCM_ERROR;
> +  }
> +

I assume "your code goes here" is now it works.  np

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +421,5 @@
>    mIceCtx->SignalGatheringCompleted.connect(this, &PeerConnectionImpl::IceGatheringCompleted);
>    mIceCtx->SignalCompleted.connect(this, &PeerConnectionImpl::IceCompleted);
>  
>    // Create two streams to start with, assume one for audio and
> +  // one for video, a thirs stream for a Data Channel is created

third

@@ +427,3 @@
>    mIceStreams.push_back(mIceCtx->CreateStream("stream1", 2));
>    mIceStreams.push_back(mIceCtx->CreateStream("stream2", 2));
> +  mIceStreams.push_back(mIceCtx->CreateStream("stream3", 2));

This probably needs to change, but not as part of this patch.  Ok.

::: media/webrtc/signaling/src/sipcc/core/gsm/h/fsm.h
@@ +68,5 @@
>  
>  #define FSMDEF_MAX_DIGEST_ALG_LEN  10
>  #define FSMDEF_MAX_DIGEST_LEN      32 * 3
>  
> +#define FSMDEF_MAX_SDP_STRING      80

This should not be used - elsewhere it uses SDP_MAX_STRING_LEN, though both are 80 - and it allocates using this sym, but strncpy's using the other!

Also, 80?? there's no such limit... though it may be enough for now.

::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr.c
@@ +460,5 @@
>      u16          usedtx = 0;
>      u16          stereo = 0;
>      u16          useinbandfec = 0;
>      u16          cbr = 0;
> +    u16          streams = 0;

Must be at least an int

@@ +1831,4 @@
>      	    tok++;
>      	    cbr = atoi(tok);
>      	    if (cbr > 1) {
> +            if (sdp_p->debug_flag[SDP_DEBUG_WARNINGS]) {

indents/tabs (at least check if it's "right" for the file; spliter review may interpret tabs "incorrectly")
Attachment #660053 - Attachment is obsolete: true
Attachment #660053 - Flags: feedback?(rjesup)
Attachment #660053 - Flags: feedback?(ethanhugg)
Attachment #660093 - Flags: feedback?(rjesup)
Attachment #660093 - Flags: feedback?(ethanhugg)
(In reply to Randell Jesup [:jesup] from comment #9)
> Comment on attachment 660053 [details] [diff] [review]
> Resed: Data Channel support added to SDP in SIPCC
> 
> Review of attachment 660053 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> incomplete review - up through lsm.h.  WIll continue later
> 
> ::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
> @@ +994,5 @@
> > +/* Set negotiated DataChannel parameters.
> > + *
> > + *  @param[in]  peerconnection - the peerconnection in use
> > + *  @param[in]  ufrag - the ufrag
> > + *  @param[in]  pwd - the pwd
> 
> Comments are wrong - cut and paste

Done.

> 
> @@ +1009,5 @@
> > +  PR_ASSERT(pc);
> > +  if (!pc) {
> > +    return VCM_ERROR;
> > +  }
> > +
> 
> I assume "your code goes here" is now it works.  np

I assume this is where you write your code that taks to libsctp?

> 
> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> @@ +421,5 @@
> >    mIceCtx->SignalGatheringCompleted.connect(this, &PeerConnectionImpl::IceGatheringCompleted);
> >    mIceCtx->SignalCompleted.connect(this, &PeerConnectionImpl::IceCompleted);
> >  
> >    // Create two streams to start with, assume one for audio and
> > +  // one for video, a thirs stream for a Data Channel is created
> 
> third
> 

Done.

> @@ +427,3 @@
> >    mIceStreams.push_back(mIceCtx->CreateStream("stream1", 2));
> >    mIceStreams.push_back(mIceCtx->CreateStream("stream2", 2));
> > +  mIceStreams.push_back(mIceCtx->CreateStream("stream3", 2));
> 
> This probably needs to change, but not as part of this patch.  Ok.
> 
> ::: media/webrtc/signaling/src/sipcc/core/gsm/h/fsm.h
> @@ +68,5 @@
> >  
> >  #define FSMDEF_MAX_DIGEST_ALG_LEN  10
> >  #define FSMDEF_MAX_DIGEST_LEN      32 * 3
> >  
> > +#define FSMDEF_MAX_SDP_STRING      80

|I removed this and use the same define in all places now.

> 
> This should not be used - elsewhere it uses SDP_MAX_STRING_LEN, though both
> are 80 - and it allocates using this sym, but strncpy's using the other!
> 
> Also, 80?? there's no such limit... though it may be enough for now.
> 
> ::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr.c
> @@ +460,5 @@
> >      u16          usedtx = 0;
> >      u16          stereo = 0;
> >      u16          useinbandfec = 0;
> >      u16          cbr = 0;
> > +    u16          streams = 0;
> 
> Must be at least an int

Done

> 
> @@ +1831,4 @@
> >      	    tok++;
> >      	    cbr = atoi(tok);
> >      	    if (cbr > 1) {
> > +            if (sdp_p->debug_flag[SDP_DEBUG_WARNINGS]) {
> 
> indents/tabs (at least check if it's "right" for the file; spliter review
> may interpret tabs "incorrectly")
Attachment #660093 - Attachment is obsolete: true
Attachment #660093 - Flags: feedback?(rjesup)
Attachment #660093 - Flags: feedback?(ethanhugg)
Attachment #660106 - Flags: feedback?(rjesup)
Attachment #660106 - Flags: feedback?(ethanhugg)
Comment on attachment 660106 [details] [diff] [review]
Resed: Data Channel support added to SDP in SIPCC

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

The 'streams' param needs to be int, not u32 or even s32.

r+ with the various nits in this and previous comments fixed

::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_private.h
@@ +162,5 @@
>      u16                       cbr;
>  
> +    /* some Data Channel specific fmtp params */
> +    u16                       streams;   /* Num streams per Data Channel */
> +    char                      protocol[SDP_MAX_STRING_LEN+1];

Not an issue for landing this patch, but all the SDP_MAX_STRING_LENs should be removed and replaced with something dynamic

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ -30,5 @@
>  MtransportTestUtils test_utils;
>  
>  static int kDefaultTimeout = 5000;
>  
> -

spurious whitespace change
Attachment #660106 - Flags: feedback?(rjesup) → feedback+
Attachment #660106 - Attachment is obsolete: true
Attachment #660106 - Flags: feedback?(ethanhugg)
Attachment #660125 - Flags: feedback?(rjesup)
Attachment #660125 - Flags: feedback?(ethanhugg)
Attachment #660125 - Attachment is obsolete: true
Attachment #660125 - Flags: feedback?(rjesup)
Attachment #660125 - Flags: feedback?(ethanhugg)
Attachment #660145 - Flags: feedback?(rjesup)
(In reply to Randell Jesup [:jesup] from comment #13)
> Comment on attachment 660106 [details] [diff] [review]
> Resed: Data Channel support added to SDP in SIPCC
> 
> Review of attachment 660106 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The 'streams' param needs to be int, not u32 or even s32.

Done.

> 
> r+ with the various nits in this and previous comments fixed
> 
> ::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_private.h
> @@ +162,5 @@
> >      u16                       cbr;
> >  
> > +    /* some Data Channel specific fmtp params */
> > +    u16                       streams;   /* Num streams per Data Channel */
> > +    char                      protocol[SDP_MAX_STRING_LEN+1];
> 
> Not an issue for landing this patch, but all the SDP_MAX_STRING_LENs should
> be removed and replaced with something dynamic

Can i get this in a separate patch.

> 
> ::: media/webrtc/signaling/test/signaling_unittests.cpp
> @@ -30,5 @@
> >  MtransportTestUtils test_utils;
> >  
> >  static int kDefaultTimeout = 5000;
> >  
> > -
> 
> spurious whitespace change

Done.
Comment on attachment 660145 [details] [diff] [review]
Resed: Data Channel support added to SDP in SIPCC

Pushed to Alder - https://hg.mozilla.org/projects/alder/rev/6fcf0ed0eeca
Attachment #660145 - Flags: feedback?(rjesup)
This patch will increment the SCTP port in the Answer SDP.  This patch builds upon the constraints patch and on top of mozilla-central.
Attachment #670747 - Flags: feedback?(rjesup)
Attachment #670747 - Flags: feedback?(ethanhugg)
Attachment #670747 - Flags: feedback?(rjesup) → review+
Attachment #670747 - Flags: feedback?(ethanhugg) → feedback+
I had some merge conflicts when I updated to m-c tip.
Attachment #670747 - Attachment is obsolete: true
Attachment #671116 - Flags: feedback?(ethanhugg)
Attachment #671116 - Flags: feedback?(ethanhugg) → review+
Attachment #671116 - Flags: checkin?(rjesup)
https://hg.mozilla.org/mozilla-central/rev/daf391b98703
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Attachment #671116 - Flags: checkin?(rjesup)
Bug 803318 should really be pushed before this one.
Enda, Look at comment 20.  This is already in M-C.
This is all fine now, I have update the patch on 803318 to tip
Keywords: verifyme
Blocks: 837035
Keywords: verifyme
Whiteboard: [WebRTC], [blocking-webrtc+] → [WebRTC], [blocking-webrtc+], [qa-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: