Closed Bug 786152 Opened 13 years ago Closed 13 years ago

Implement SDP for WebRTC DataChannels

Categories

(Core :: WebRTC: Signaling, defect)

Product:
Core
Component:
WebRTC: Signaling
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla19

People

(Reporter: jesup, Assigned: emannion)

References

Details

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

Attachments

(2 files, 6 obsolete files)

Resed: Data Channel support added to SDP in SIPCC
62.71 KB, patch
Details | Diff | Splinter Review
Increment SCTP port on negotiation
5.56 KB, patch
ehugg
: review+
Details | Diff | Splinter Review
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: 13 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

Creator:
Created:
Updated:
Size: