Closed
Bug 783313
Opened 12 years ago
Closed 12 years ago
Need to be able to negotiateRTP/SAVPF
Categories
(Core :: WebRTC: Signaling, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: emannion, Assigned: emannion)
Details
(Whiteboard: [qa-])
Attachments
(1 file, 6 obsolete files)
32.42 KB,
patch
|
Details | Diff | Splinter Review |
This patch added the transport RTP/SAVPF to SIPCC
Attachment #652481 -
Flags: feedback?(ethanhugg)
Attachment #652481 -
Flags: feedback?(ekr)
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #652481 -
Attachment is obsolete: true
Attachment #652481 -
Flags: feedback?(ethanhugg)
Attachment #652481 -
Flags: feedback?(ekr)
Attachment #652574 -
Flags: feedback?(ethanhugg)
Comment 2•12 years ago
|
||
Comment on attachment 652574 [details] [diff] [review] Adding RTP/SAVPF to SIPCC Review of attachment 652574 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp_crypto.c @@ +1333,5 @@ > + /* > + * The signaling is not encrypted or this media can not support > + * security. > + */ > + media->transport = SDP_TRANSPORT_RTPAVP; Why AVP and not AVPF? Generally, I think we need a config parameter for if we are going to ask for AVPF/SAVPF rather than AVP/SAVP. ::: media/webrtc/signaling/src/sipcc/core/gsm/media_cap_tbl.c @@ +53,5 @@ > cc_media_cap_table_t g_media_table = { > 1, > { > {CC_AUDIO_1,SDP_MEDIA_AUDIO,TRUE,TRUE,SDP_DIRECTION_SENDRECV}, > + {CC_VIDEO_1,SDP_MEDIA_VIDEO,FALSE,TRUE,SDP_DIRECTION_SENDRECV}, What does this do?
Assignee | ||
Comment 3•12 years ago
|
||
(In reply to Eric Rescorla from comment #2) > Comment on attachment 652574 [details] [diff] [review] > Adding RTP/SAVPF to SIPCC > > Review of attachment 652574 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp_crypto.c > @@ +1333,5 @@ > > + /* > > + * The signaling is not encrypted or this media can not support > > + * security. > > + */ > > + media->transport = SDP_TRANSPORT_RTPAVP; > > Why AVP and not AVPF? Now by default SAVFP will be offered and if the new config setting is set to false then SAVP is set to true. AVP can not be set now unless the config is further extended. > > Generally, I think we need a config parameter for if we are going to ask for > AVPF/SAVPF rather than AVP/SAVP. Done. > > ::: media/webrtc/signaling/src/sipcc/core/gsm/media_cap_tbl.c > @@ +53,5 @@ > > cc_media_cap_table_t g_media_table = { > > 1, > > { > > {CC_AUDIO_1,SDP_MEDIA_AUDIO,TRUE,TRUE,SDP_DIRECTION_SENDRECV}, > > + {CC_VIDEO_1,SDP_MEDIA_VIDEO,FALSE,TRUE,SDP_DIRECTION_SENDRECV}, > > What does this do? Turns on encryption for Video, this will come into effect when SAVP is used.
Assignee | ||
Comment 4•12 years ago
|
||
I added some code tidy up to this patch from the config files I changed.
Attachment #652574 -
Attachment is obsolete: true
Attachment #652574 -
Flags: feedback?(ethanhugg)
Attachment #652735 -
Flags: feedback?(ethanhugg)
Attachment #652735 -
Flags: feedback?(ekr)
Assignee | ||
Comment 5•12 years ago
|
||
Disable rtcp-mux added to this patch
Attachment #652735 -
Attachment is obsolete: true
Attachment #652735 -
Flags: feedback?(ethanhugg)
Attachment #652735 -
Flags: feedback?(ekr)
Attachment #652769 -
Flags: feedback?(ethanhugg)
Comment 6•12 years ago
|
||
Comment on attachment 652769 [details] [diff] [review] revised: Adding RTP/SAVPF to SIPCC Review of attachment 652769 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp_crypto.c @@ +1349,5 @@ > + media->transport = SDP_TRANSPORT_RTPSAVP; > + return; > + } > + } > + media->transport = SDP_TRANSPORT_RTPSAVPF; This seems to be the only place that the RTPSAVPF value is used I wonder if it's properly named. I'm thinking either two bools, one for the S and one for the F, or an int that gives SDP_TRANSPORT_ type. If you think this is best though, I would be willing to leave it as is. Also I would consider simplifying this code block to look like this: if (rtcsavpf) { media->transport = SDP_TRANSPORT_RTPSAVPF; } else if (sdpmode) { media->transport = SDP_TRANSPORT_RTPSAVP; } else if ((sip_regmgr_get_sec_level(dcb_p->line) != ENCRYPTED) || (!FSM_CHK_FLAGS(media->flags, FSM_MEDIA_F_SUPPORT_SECURITY))) { /* * The signaling is not encrypted or this media can not support * security. */ media->transport = SDP_TRANSPORT_RTPAVP; } else { media->transport = SDP_TRANSPORT_RTPSAVP; } which I think would be more clear. ::: media/webrtc/signaling/src/sipcc/core/sdp/sdp.h @@ +213,5 @@ > SDP_TRANSPORT_AAL1AVP, > SDP_TRANSPORT_UDPSPRT, > SDP_TRANSPORT_RTPSAVP, > SDP_TRANSPORT_TCP, > + SDP_TRANSPORT_RTPSAVPF, I would add AVPF here as well, even though we don't use it yet. ::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_main.c @@ +255,5 @@ > {"AAL1/AVP", sizeof("AAL1/AVP")}, > {"udpsprt", sizeof("udpsprt")}, > {"RTP/SAVP", sizeof("RTP/SAVP")}, > + {"tcp", sizeof("tcp")}, > + {"RTP/SAVPF", sizeof("RTP/SAVPF")} Same here
Attachment #652769 -
Flags: feedback?(ethanhugg) → feedback+
Assignee | ||
Comment 7•12 years ago
|
||
(In reply to Ethan Hugg [:ehugg] from comment #6) > Comment on attachment 652769 [details] [diff] [review] > revised: Adding RTP/SAVPF to SIPCC > > Review of attachment 652769 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp_crypto.c > @@ +1349,5 @@ > > + media->transport = SDP_TRANSPORT_RTPSAVP; > > + return; > > + } > > + } > > + media->transport = SDP_TRANSPORT_RTPSAVPF; > > This seems to be the only place that the RTPSAVPF value is used I wonder if > it's properly named. I'm thinking either two bools, one for the S and one > for the F, or an int that gives SDP_TRANSPORT_ type. If you think this is > best though, I would be willing to leave it as is. I vote to leave it as RTPSAVP is used in many places and would like to keep RTPSAVPF looking similar. > > Also I would consider simplifying this code block to look like this: > > if (rtcsavpf) { > media->transport = SDP_TRANSPORT_RTPSAVPF; > } > else if (sdpmode) { > media->transport = SDP_TRANSPORT_RTPSAVP; > } > else if ((sip_regmgr_get_sec_level(dcb_p->line) != ENCRYPTED) || > (!FSM_CHK_FLAGS(media->flags, FSM_MEDIA_F_SUPPORT_SECURITY))) { > /* > * The signaling is not encrypted or this media can not support > * security. > */ > media->transport = SDP_TRANSPORT_RTPAVP; > } > else { > media->transport = SDP_TRANSPORT_RTPSAVP; > } > > which I think would be more clear. I agree, I changes this. > > ::: media/webrtc/signaling/src/sipcc/core/sdp/sdp.h > @@ +213,5 @@ > > SDP_TRANSPORT_AAL1AVP, > > SDP_TRANSPORT_UDPSPRT, > > SDP_TRANSPORT_RTPSAVP, > > SDP_TRANSPORT_TCP, > > + SDP_TRANSPORT_RTPSAVPF, > > I would add AVPF here as well, even though we don't use it yet. > > ::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_main.c > @@ +255,5 @@ > > {"AAL1/AVP", sizeof("AAL1/AVP")}, > > {"udpsprt", sizeof("udpsprt")}, > > {"RTP/SAVP", sizeof("RTP/SAVP")}, > > + {"tcp", sizeof("tcp")}, > > + {"RTP/SAVPF", sizeof("RTP/SAVPF")} > > Same here I could but would like to wait until we need to support it as sometimes changing the SDP types can have a ripple effect and its working well as is. If if you really want me to change I will.
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #652769 -
Attachment is obsolete: true
Attachment #652799 -
Flags: feedback?(ethanhugg)
Comment 9•12 years ago
|
||
Comment on attachment 652799 [details] [diff] [review] revised: Adding RTP/SAVPF to SIPCC Review of attachment 652799 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp_crypto.c @@ +1345,5 @@ > media->transport = SDP_TRANSPORT_RTPAVP; > + return; > + } else { > + media->transport = SDP_TRANSPORT_RTPSAVP; > + return; I don't think you need these two returns here.
Attachment #652799 -
Flags: feedback?(ethanhugg) → feedback+
Comment 10•12 years ago
|
||
Updated•12 years ago
|
Attachment #652799 -
Attachment is obsolete: true
Comment 11•12 years ago
|
||
Updated•12 years ago
|
Attachment #652811 -
Attachment is obsolete: true
Updated•12 years ago
|
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•