784161 - Implement trickle ICE

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Eric Rescorla (:ekr)]

Attachment: WIP

Comment 1

[enda mannion (:emannion)]

Attachment: Add Trickle ICE to SIPCC and PeerConnection

maybe this could be a bit tidier but it is working from my debugging and unit test.

Comment 2

[Eric Rescorla (:ekr)]

Comment on attachment 654595 [details] [diff] [review]
Add Trickle ICE to SIPCC and PeerConnection

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

I'd like to get ehugg to comment on the style you're using or extensions here. It seems like there ought to be a way to do these extensions so we don't need to keep adding arguments/fields to all the messages

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +771,5 @@
> + *  @param[in]  level - the m line level
> + *
> + *  @return 0 success, error failure
> + */
> +short vcmSetIceCandidate(const char *peerconnection, char *icecandidate, uint16_t level)

const char icecandidate

::: media/webrtc/signaling/src/sipcc/core/ccapp/cc_call_feature.c
@@ +140,5 @@
>      callFeature.featData.ccData.action = action;
>      callFeature.featData.ccData.media_type = media_type;
>      callFeature.featData.ccData.stream_id = stream_id;
>      callFeature.featData.ccData.track_id = track_id;
> +    callFeature.featData.ccData.level = int_data;

If you are going to explicitly set this value into level, then you should call it level in the args list and move it before the other "unbound" values, i.e. data and data1.

::: media/webrtc/signaling/src/sipcc/core/gsm/ccapi.c
@@ +1176,5 @@
> +    pmsg->level      = int_data;
> +
> +    if (CC_FEATURE_ADDICECANDIDATE == feature_id) {
> +        init_empty_str(pmsg->candidate);
> +        strncpy(pmsg->candidate, str_data, CANDIDATE_SIZE);

sstrncpy.

::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
@@ +3526,5 @@
> +    string_t            candidate = msg->candidate;
> +    uint16_t            level = msg->level;
> +    short               vcm_res;
> +
> +    FSM_DEBUG_SM(DEB_F_PREFIX"Entered.\n", DEB_F_PREFIX_ARGS(FSM, __FUNCTION__));

So you're basically ignoring mid? Does SIPCC not implement it?

@@ +3546,5 @@
> +    	FSM_DEBUG_SM(DEB_F_PREFIX"failure setting ice candidate.\n", DEB_F_PREFIX_ARGS(FSM, __FUNCTION__));
> +    }
> +
> +
> +	return (SM_RC_END);

Indentation.

::: media/webrtc/signaling/src/sipcc/core/includes/ccapi.h
@@ +896,5 @@
>      char                 sdp[SDP_SIZE];
>      cc_media_track_id_t  track_id;
>      cc_media_type_t      media_type;
> +    uint16_t             level;
> +    char                 candidate[CANDIDATE_SIZE];

Why aren't you stuffing candidate in a cc_feature_data_t?

Comment 3

[enda mannion (:emannion)]

(In reply to Eric Rescorla from comment #2)
> Comment on attachment 654595 [details] [diff] [review]
> Add Trickle ICE to SIPCC and PeerConnection
> 
> Review of attachment 654595 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd like to get ehugg to comment on the style you're using or extensions
> here. It seems like there ought to be a way to do these extensions so we
> don't need to keep adding arguments/fields to all the messages
> 
> ::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
> @@ +771,5 @@
> > + *  @param[in]  level - the m line level
> > + *
> > + *  @return 0 success, error failure
> > + */
> > +short vcmSetIceCandidate(const char *peerconnection, char *icecandidate, uint16_t level)
> 
> const char icecandidate

done

> 
> ::: media/webrtc/signaling/src/sipcc/core/ccapp/cc_call_feature.c
> @@ +140,5 @@
> >      callFeature.featData.ccData.action = action;
> >      callFeature.featData.ccData.media_type = media_type;
> >      callFeature.featData.ccData.stream_id = stream_id;
> >      callFeature.featData.ccData.track_id = track_id;
> > +    callFeature.featData.ccData.level = int_data;
> 
> If you are going to explicitly set this value into level, then you should
> call it level in the args list and move it before the other "unbound"
> values, i.e. data and data1.

done.

> 
> ::: media/webrtc/signaling/src/sipcc/core/gsm/ccapi.c
> @@ +1176,5 @@
> > +    pmsg->level      = int_data;
> > +
> > +    if (CC_FEATURE_ADDICECANDIDATE == feature_id) {
> > +        init_empty_str(pmsg->candidate);
> > +        strncpy(pmsg->candidate, str_data, CANDIDATE_SIZE);
> 
> sstrncpy.

done.

> 
> ::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
> @@ +3526,5 @@
> > +    string_t            candidate = msg->candidate;
> > +    uint16_t            level = msg->level;
> > +    short               vcm_res;
> > +
> > +    FSM_DEBUG_SM(DEB_F_PREFIX"Entered.\n", DEB_F_PREFIX_ARGS(FSM, __FUNCTION__));
> 
> So you're basically ignoring mid? Does SIPCC not implement it?

I thought we would leave that for a while, SIPCC does support mid and group, uses it for lip sync, but I feel I need more time to understand that more.  Can it wait?

> 
> @@ +3546,5 @@
> > +    	FSM_DEBUG_SM(DEB_F_PREFIX"failure setting ice candidate.\n", DEB_F_PREFIX_ARGS(FSM, __FUNCTION__));
> > +    }
> > +
> > +
> > +	return (SM_RC_END);
> 
> Indentation.

done.

> 
> ::: media/webrtc/signaling/src/sipcc/core/includes/ccapi.h
> @@ +896,5 @@
> >      char                 sdp[SDP_SIZE];
> >      cc_media_track_id_t  track_id;
> >      cc_media_type_t      media_type;
> > +    uint16_t             level;
> > +    char                 candidate[CANDIDATE_SIZE];
> 
> Why aren't you stuffing candidate in a cc_feature_data_t?

I am now.

Comment 4

[enda mannion (:emannion)]

Attachment: revised: Add Trickle ICE to SIPCC and PeerConnection

Comment 5

[Ethan Hugg [:ehugg]]

Comment on attachment 654668 [details] [diff] [review]
revised: Add Trickle ICE to SIPCC and PeerConnection

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

::: media/webrtc/signaling/src/media/CSFAudioTermination.h
@@ +116,5 @@
>      AudioPayloadType_RFC2833 = 38,
>      AudioPayloadType_ILBC20 = 39,
>      AudioPayloadType_ILBC30 = 40,
> +    AudioPayloadType_ISAC = 41,
> +    AudioPayloadType_OPUS = 109

THis looks like the OPUS patch to me.

Comment 6

[enda mannion (:emannion)]

Attachment: revised: Add Trickle ICE to SIPCC and PeerConnection

Comment 7

[Ethan Hugg [:ehugg]]

Comment on attachment 654675 [details] [diff] [review]
revised: Add Trickle ICE to SIPCC and PeerConnection

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

::: media/webrtc/signaling/src/sipcc/core/includes/ccapi.h
@@ +780,5 @@
> +
> +typedef struct cc_feature_candidate_t_ {
> +  uint16_t             level;
> +  char                 candidate[CANDIDATE_SIZE];
> +} cc_feature_candidate_t;

Should we add mid to this for now even though we don't know exactly what to do with it yet?

@@ +898,5 @@
>      callid_t             call_id;
>      line_t               line;
>      cc_features_t        feature_id;
>      cc_feature_data_t    data;
> +    cc_feature_data_t    data1;

What is this for?  I can't see that it is used anywhere.

Comment 8

[enda mannion (:emannion)]

Attachment: revised: Add Trickle ICE to SIPCC and PeerConnection

I added your comments.  I added mid processing code but commented its usage out as we want mid to process strings or tokens.

Comment 9

[Ethan Hugg [:ehugg]]

Attachment: add trickle ICE to signaling

Comment 10

[Ethan Hugg [:ehugg]]

Comment on attachment 655163 [details] [diff] [review]
add trickle ICE to signaling


Updated to apply on latest and fixed warning:

warnings-as-errors found this bug:

gsm_sdp.c:5997
       if (strcmp(mid, buf) == 0) {
-          level = media->level;
+          *level = media->level;
          return CC_CAUSE_OK;

Comment 11

[Ethan Hugg [:ehugg]]

Comment on attachment 655163 [details] [diff] [review]
add trickle ICE to signaling


Pushed to Alder - http://hg.mozilla.org/projects/alder/rev/c4901ee326cd

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

THis was pushed to alder before it merged to m-c - fixed.   If there are any other issues, we'll open new bugs