Correctly set initial controlled/controlling values

RESOLVED FIXED in mozilla20

Status

()

Core
WebRTC: Networking
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ekr, Assigned: jib)

Tracking

unspecified
mozilla20
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 4 obsolete attachments)

Comment hidden (empty)
(Reporter)

Comment 1

5 years ago
Created attachment 687555 [details] [diff] [review]
Correctly set initial controlled/controlling values
(Reporter)

Comment 2

5 years ago
Current behavior is to always be controlling. This patch sets the values appropriately based on who is offerer or answerer.
(Reporter)

Updated

5 years ago
Whiteboard: [WebRTC],[blocking-interop]
(Reporter)

Updated

5 years ago
Whiteboard: [WebRTC],[blocking-interop] → [WebRTC][blocking-interop]
(Reporter)

Updated

5 years ago
Whiteboard: [WebRTC][blocking-interop] → [WebRTC][blocking-webrtc-interop]
(Assignee)

Updated

5 years ago
Assignee: ekr → jib
Whiteboard: [WebRTC][blocking-webrtc-interop] → [WebRTC][blocking-webrtc-interop],[blocking-webrtc+]
(Assignee)

Comment 3

5 years ago
Created attachment 693463 [details] [diff] [review]
Correctly set initial controlled/controlling values
(Assignee)

Updated

5 years ago
Attachment #687555 - Attachment is obsolete: true

Comment 4

5 years ago
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);
(Assignee)

Comment 5

5 years ago
Created attachment 693597 [details] [diff] [review]
Set initial controlled/controlling values, based on inbound T/F
(Assignee)

Updated

5 years ago
Attachment #693463 - Attachment is obsolete: true
(Assignee)

Comment 6

5 years ago
Created attachment 693610 [details] [diff] [review]
Set initial controlled/controlling values, based on inbound T/F
(Assignee)

Updated

5 years ago
Attachment #693597 - Attachment is obsolete: true
(Assignee)

Comment 7

5 years ago
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).
Attachment #693610 - Flags: review?(rjesup)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
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
Attachment #693610 - Flags: review?(rjesup) → review+
(Assignee)

Comment 9

5 years ago
Created attachment 694127 [details] [diff] [review]
Set initial controlled/controlling values, based on inbound T/F.
(Assignee)

Updated

5 years ago
Attachment #693610 - Attachment is obsolete: true
(Assignee)

Comment 10

5 years ago
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.
Attachment #694127 - Flags: review+
Attachment #694127 - Flags: checkin?(rjesup)
https://hg.mozilla.org/integration/mozilla-inbound/rev/e48db20a0a46
Target Milestone: --- → mozilla20

Updated

5 years ago
Attachment #694127 - Flags: checkin?(rjesup) → checkin+
https://hg.mozilla.org/mozilla-central/rev/e48db20a0a46
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Whiteboard: [WebRTC][blocking-webrtc-interop],[blocking-webrtc+] → [WebRTC][blocking-webrtc-interop],[blocking-webrtc+][qa-]
You need to log in before you can comment on or make changes to this bug.