Closed Bug 783313 Opened 12 years ago Closed 12 years ago

Need to be able to negotiateRTP/SAVPF

Categories

(Core :: WebRTC: Signaling, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: emannion, Assigned: emannion)

Details

(Whiteboard: [qa-])

Attachments

(1 file, 6 obsolete files)

Attached patch Adding RTP/SAVPF to SIPCC (obsolete) — Splinter Review
This patch added the transport RTP/SAVPF to SIPCC
Attachment #652481 - Flags: feedback?(ethanhugg)
Attachment #652481 - Flags: feedback?(ekr)
Attached patch Adding RTP/SAVPF to SIPCC (obsolete) — Splinter Review
Attachment #652481 - Attachment is obsolete: true
Attachment #652481 - Flags: feedback?(ethanhugg)
Attachment #652481 - Flags: feedback?(ekr)
Attachment #652574 - Flags: feedback?(ethanhugg)
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?
(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.
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)
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 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+
(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.
Attachment #652769 - Attachment is obsolete: true
Attachment #652799 - Flags: feedback?(ethanhugg)
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+
Attachment #652799 - Attachment is obsolete: true
Attachment #652811 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: