From 5f305f844f90d344975f18830ad41f0dea38a9da Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Wed, 21 Feb 2018 09:50:36 -0500 Subject: [PATCH] Send ICE updates immediately after sending CallOffer for faster call connection. For legacy reasons, the call sender used to wait until after receiving the call answer before sending the ICE updates. The primary motivation was that if the receiving user hadn't accepted a new identity change, rather than just seeing one "Tap to Accept New Safety Number" messages for a call, they'd see one for the call offer and then a dozen more as ICE updates trickled in. We changed that behavior long ago, and effectively all clients will avoid that case, while sending ICE updates immediately will allow calls to connect without having to wait for an additional serialized round trip between the caller and call recipient. // FREEBIE --- Signal/src/call/CallService.swift | 53 +++------------------- Signal/src/call/PeerConnectionClient.swift | 8 ++-- 2 files changed, 11 insertions(+), 50 deletions(-) diff --git a/Signal/src/call/CallService.swift b/Signal/src/call/CallService.swift index e346ae5ee..c1425177c 100644 --- a/Signal/src/call/CallService.swift +++ b/Signal/src/call/CallService.swift @@ -163,17 +163,6 @@ protocol CallServiceObserver: class { private var fulfillCallConnectedPromise: (() -> Void)? private var rejectCallConnectedPromise: ((Error) -> Void)? - /** - * In the process of establishing a connection between the clients (ICE process) we must exchange ICE updates. - * Because this happens via Signal Service it's possible the callee user has not accepted any change in the caller's - * identity. In which case *each* ICE update would cause an "identity change" warning on the callee's device. Since - * this could be several messages, the caller stores all ICE updates until receiving positive confirmation that the - * callee has received a message from us. This positive confirmation comes in the form of the callees `CallAnswer` - * message. - */ - var sendIceUpdatesImmediately = true - var pendingIceUpdateMessages = [OWSCallIceUpdateMessage]() - // Used by waitForPeerConnectionClient to make sure any received // ICE messages wait until the peer connection client is set up. private var fulfillPeerConnectionClientPromise: (() -> Void)? @@ -283,9 +272,6 @@ protocol CallServiceObserver: class { self.call = call - sendIceUpdatesImmediately = false - pendingIceUpdateMessages = [] - let callRecord = TSCall(timestamp: NSDate.ows_millisecondTimeStamp(), withCallNumber: call.remotePhoneNumber, callType: RPRecentCallTypeOutgoingIncomplete, in: call.thread) callRecord.save() call.callRecord = callRecord @@ -405,19 +391,6 @@ protocol CallServiceObserver: class { return } - // Now that we know the recipient trusts our identity, we no longer need to enqueue ICE updates. - self.sendIceUpdatesImmediately = true - - if pendingIceUpdateMessages.count > 0 { - Logger.error("\(self.logTag) Sending \(pendingIceUpdateMessages.count) pendingIceUpdateMessages") - - let callMessage = OWSOutgoingCallMessage(thread: thread, iceUpdateMessages: pendingIceUpdateMessages) - let sendPromise = messageSender.sendPromise(message: callMessage).catch { error in - Logger.error("\(self.logTag) failed to send ice updates in \(#function) with error: \(error)") - } - sendPromise.retainUntilComplete() - } - guard let peerConnectionClient = self.peerConnectionClient else { OWSProdError(OWSAnalyticsEvents.callServicePeerConnectionMissing(), file: #file, function: #function, line: #line) handleFailedCall(failedCall: call, error: CallError.assertionError(description: "peerConnectionClient was unexpectedly nil in \(#function)")) @@ -696,7 +669,7 @@ protocol CallServiceObserver: class { AssertIsOnMainThread() guard let call = self.call else { - Logger.warn("ignoring remote ice update for thread: \(thread.uniqueId) since there is no current call. Call already ended?") + Logger.warn("ignoring remote ice update for thread: \(String(describing: thread.uniqueId)) since there is no current call. Call already ended?") return } @@ -706,12 +679,12 @@ protocol CallServiceObserver: class { } guard thread.contactIdentifier() == call.thread.contactIdentifier() else { - Logger.warn("ignoring remote ice update for thread: \(thread.uniqueId) due to thread mismatch. Call already ended?") + Logger.warn("ignoring remote ice update for thread: \(String(describing: thread.uniqueId)) due to thread mismatch. Call already ended?") return } guard let peerConnectionClient = self.peerConnectionClient else { - Logger.warn("ignoring remote ice update for thread: \(thread.uniqueId) since there is no current peerConnectionClient. Call already ended?") + Logger.warn("ignoring remote ice update for thread: \(String(describing: thread.uniqueId)) since there is no current peerConnectionClient. Call already ended?") return } @@ -753,19 +726,10 @@ protocol CallServiceObserver: class { let iceUpdateMessage = OWSCallIceUpdateMessage(callId: call.signalingId, sdp: iceCandidate.sdp, sdpMLineIndex: iceCandidate.sdpMLineIndex, sdpMid: iceCandidate.sdpMid) - if self.sendIceUpdatesImmediately { - Logger.info("\(self.logTag) in \(#function). Sending immediately.") - let callMessage = OWSOutgoingCallMessage(thread: call.thread, iceUpdateMessage: iceUpdateMessage) - let sendPromise = self.messageSender.sendPromise(message: callMessage) - sendPromise.retainUntilComplete() - } else { - // For outgoing messages, we wait to send ice updates until we're sure client received our call message. - // e.g. if the client has blocked our message due to an identity change, we'd otherwise - // bombard them with a bunch *more* undecipherable messages. - Logger.info("\(self.logTag) in \(#function). Enqueing for later.") - self.pendingIceUpdateMessages.append(iceUpdateMessage) - return - } + Logger.info("\(self.logTag) in \(#function) sending ICE Candidate.") + let callMessage = OWSOutgoingCallMessage(thread: call.thread, iceUpdateMessage: iceUpdateMessage) + let sendPromise = self.messageSender.sendPromise(message: callMessage) + sendPromise.retainUntilComplete() }.catch { error in OWSProdInfo(OWSAnalyticsEvents.callServiceErrorHandleLocalAddedIceCandidate(), file: #file, function: #function, line: #line) Logger.error("\(self.logTag) in \(#function) waitUntilReadyToSendIceUpdates failed with error: \(error)") @@ -1503,9 +1467,6 @@ protocol CallServiceObserver: class { self.callUIAdapter.didTerminateCall(self.call) self.call = nil - self.sendIceUpdatesImmediately = true - Logger.info("\(self.logTag) clearing pendingIceUpdateMessages") - self.pendingIceUpdateMessages = [] self.fulfillCallConnectedPromise = nil // In case we're still waiting on this promise somewhere, we need to reject it to avoid a memory leak. diff --git a/Signal/src/call/PeerConnectionClient.swift b/Signal/src/call/PeerConnectionClient.swift index 367409db0..f4969281a 100644 --- a/Signal/src/call/PeerConnectionClient.swift +++ b/Signal/src/call/PeerConnectionClient.swift @@ -1,5 +1,5 @@ // -// Copyright (c) 2017 Open Whisper Systems. All rights reserved. +// Copyright (c) 2018 Open Whisper Systems. All rights reserved. // import Foundation @@ -657,7 +657,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client") return } - Logger.debug("\(self.TAG) didChange IceConnectionState:\(newState.debugDescription)") + Logger.info("\(self.TAG) didChange IceConnectionState:\(newState.debugDescription)") switch newState { case .connected, .completed: if let delegate = self.delegate { @@ -684,7 +684,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD /** Called any time the IceGatheringState changes. */ internal func peerConnection(_ peerConnection: RTCPeerConnection, didChange newState: RTCIceGatheringState) { - Logger.debug("\(TAG) didChange IceGatheringState:\(newState.debugDescription)") + Logger.info("\(TAG) didChange IceGatheringState:\(newState.debugDescription)") } /** New ice candidate has been found. */ @@ -716,7 +716,7 @@ class PeerConnectionClient: NSObject, RTCPeerConnectionDelegate, RTCDataChannelD Logger.debug("\(self.TAG) \(#function) Ignoring obsolete event in terminated client") return } - Logger.debug("\(self.TAG) didOpen dataChannel:\(dataChannel)") + Logger.info("\(self.TAG) didOpen dataChannel:\(dataChannel)") assert(self.dataChannel == nil) self.dataChannel = dataChannel dataChannel.delegate = self