Last Comment Bug 817430 - Correctly set initial controlled/controlling values
: Correctly set initial controlled/controlling values
Status: RESOLVED FIXED
[WebRTC][blocking-webrtc-interop],[bl...
:
Product: Core
Classification: Components
Component: WebRTC: Networking (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla20
Assigned To: Jan-Ivar Bruaroey [:jib]
: Jason Smith [:jsmith]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-02 13:43 PST by Eric Rescorla (:ekr)
Modified: 2012-12-23 14:01 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Correctly set initial controlled/controlling values (5.67 KB, patch)
2012-12-02 13:43 PST, Eric Rescorla (:ekr)
no flags Details | Diff | Splinter Review
Correctly set initial controlled/controlling values (6.27 KB, patch)
2012-12-18 09:15 PST, Jan-Ivar Bruaroey [:jib]
no flags Details | Diff | Splinter Review
Set initial controlled/controlling values, based on inbound T/F (6.25 KB, patch)
2012-12-18 13:24 PST, Jan-Ivar Bruaroey [:jib]
no flags Details | Diff | Splinter Review
Set initial controlled/controlling values, based on inbound T/F (8.04 KB, patch)
2012-12-18 13:44 PST, Jan-Ivar Bruaroey [:jib]
rjesup: review+
Details | Diff | Splinter Review
Set initial controlled/controlling values, based on inbound T/F. (8.05 KB, patch)
2012-12-19 15:54 PST, Jan-Ivar Bruaroey [:jib]
jib: review+
rjesup: checkin+
Details | Diff | Splinter Review

Description Eric Rescorla (:ekr) 2012-12-02 13:43:48 PST

    
Comment 1 Eric Rescorla (:ekr) 2012-12-02 13:43:53 PST
Created attachment 687555 [details] [diff] [review]
Correctly set initial controlled/controlling values
Comment 2 Eric Rescorla (:ekr) 2012-12-02 13:44:44 PST
Current behavior is to always be controlling. This patch sets the values appropriately based on who is offerer or answerer.
Comment 3 Jan-Ivar Bruaroey [:jib] 2012-12-18 09:15:46 PST
Created attachment 693463 [details] [diff] [review]
Correctly set initial controlled/controlling values
Comment 4 Ethan Hugg [:ehugg] 2012-12-18 13:09:12 PST
Since we are running in SDPMode, some of the dcb values do not get set.  In particular, the ones that might tell us if the connection was initiated in or out.  I propose setting dcb->inbound in fsmdev_ev_createoffer and fsmdev_ev_createanswer so we have it when needed in lsm_connected.  Something like this:

diff --git a/media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c b/media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
--- a/media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
+++ b/media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
@@ -2884,16 +2884,18 @@ fsmdef_ev_createoffer (sm_event_t *event
       return (fsmdef_release(fcb, cause, FALSE));
     }
 
     if (dcb == NULL) {
       FSM_DEBUG_SM(DEB_F_PREFIX"dcb is NULL.\n", DEB_F_PREFIX_ARGS(FSM, __FUNCTION__));
       return SM_RC_CLEANUP;
     }
 
+   dcb->inbound = FALSE;
+
    if (msg->data.session.has_constraints) {
         sess_data_p = (session_data_t *)findhash(msg->data.session.sessionid);
         if (sess_data_p) {
             gsmsdp_process_cap_constraints(dcb, sess_data_p->cc_constraints);
 
             if (0 > delhash(msg->data.session.sessionid)) {
                 FSM_DEBUG_SM (DEB_F_PREFIX"failed to delete hash sessid=0x%08x\n",
                 DEB_F_PREFIX_ARGS(SIP_CC_PROV, __FUNCTION__), msg->data.session.sessionid);
@@ -2990,16 +2992,18 @@ fsmdef_ev_createanswer (sm_event_t *even
         return (fsmdef_release(fcb, cause, FALSE));
     }
 
     if (dcb == NULL) {
         FSM_DEBUG_SM(DEB_F_PREFIX"dcb is NULL.\n", DEB_F_PREFIX_ARGS(FSM, __FUNCTION__));
         return SM_RC_CLEANUP;
     }
 
+    dcb->inbound = TRUE;
+
     if (msg->data.session.has_constraints) {
         sess_data_p = (session_data_t *)findhash(msg->data.session.sessionid);
         if (sess_data_p) {
             gsmsdp_process_cap_constraints(dcb, sess_data_p->cc_constraints);
 
             if (0 > delhash(msg->data.session.sessionid)) {
                 FSM_DEBUG_SM (DEB_F_PREFIX"failed to delete hash sessid=0x%08x\n",
                 DEB_F_PREFIX_ARGS(SIP_CC_PROV, __FUNCTION__), msg->data.session.sessionid);
Comment 5 Jan-Ivar Bruaroey [:jib] 2012-12-18 13:24:51 PST
Created attachment 693597 [details] [diff] [review]
Set initial controlled/controlling values, based on inbound T/F
Comment 6 Jan-Ivar Bruaroey [:jib] 2012-12-18 13:44:31 PST
Created attachment 693610 [details] [diff] [review]
Set initial controlled/controlling values, based on inbound T/F
Comment 7 Jan-Ivar Bruaroey [:jib] 2012-12-18 13:54:05 PST
Comment on attachment 693610 [details] [diff] [review]
Set initial controlled/controlling values, based on inbound T/F

Now includes the change from Ethan's comment. Tested that it produces the right result in the debugger on a two-way cross-machine call, initiated by either party (I don't know the observable outcome).
Comment 8 Randell Jesup [:jesup] 2012-12-19 12:50:57 PST
Comment on attachment 693610 [details] [diff] [review]
Set initial controlled/controlling values, based on inbound T/F

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

::: media/mtransport/nricectx.cpp
@@ +349,5 @@
>    nr_ice_peer_ctx_destroy(&peer_);
>  }
>  
> +nsresult NrIceCtx::SetControlling(Controlling controlling) {
> +  peer_->controlling = controlling == ICE_CONTROLLING ? 1 : 0;

parens around the ternary please
Comment 9 Jan-Ivar Bruaroey [:jib] 2012-12-19 15:54:41 PST
Created attachment 694127 [details] [diff] [review]
Set initial controlled/controlling values, based on inbound T/F.
Comment 10 Jan-Ivar Bruaroey [:jib] 2012-12-19 15:57:49 PST
Comment on attachment 694127 [details] [diff] [review]
Set initial controlled/controlling values, based on inbound T/F.

Added requested parens. Carrying forward r+ from rjesup.
Comment 12 Ed Morley [:emorley] 2012-12-20 13:42:52 PST
https://hg.mozilla.org/mozilla-central/rev/e48db20a0a46

Note You need to log in before you can comment on or make changes to this bug.