From a0298283134ba7a8d79a3d8d6ec9703137188298 Mon Sep 17 00:00:00 2001 From: "Byron Campen [:bwc]" Date: Thu, 6 Feb 2020 15:50:03 +0000 Subject: [PATCH] Bug 1591199: Reduce redundant code. r=mjf,smaug Differential Revision: https://phabricator.services.mozilla.com/D56404 --HG-- extra : moz-landing-system : lando --- dom/media/PeerConnection.jsm | 40 ++--- dom/webidl/PeerConnectionObserver.webidl | 6 +- .../src/peerconnection/PeerConnectionImpl.cpp | 158 +++++++----------- .../src/peerconnection/PeerConnectionImpl.h | 2 +- 4 files changed, 78 insertions(+), 128 deletions(-) diff --git a/dom/media/PeerConnection.jsm b/dom/media/PeerConnection.jsm index 1a35e05fe537..fa0beca348c1 100644 --- a/dom/media/PeerConnection.jsm +++ b/dom/media/PeerConnection.jsm @@ -1087,8 +1087,8 @@ class RTCPeerConnection { return this._chain(async () => { await this._getPermission(); await new Promise((resolve, reject) => { - this._onSetLocalDescriptionSuccess = resolve; - this._onSetLocalDescriptionFailure = reject; + this._onSetDescriptionSuccess = resolve; + this._onSetDescriptionFailure = reject; this._impl.setLocalDescription(action, sdp); }); this._negotiationNeeded = false; @@ -1181,19 +1181,26 @@ class RTCPeerConnection { await this._getPermission(); if (type == "offer" && this.signalingState == "have-local-offer") { await new Promise((resolve, reject) => { - this._onSetLocalDescriptionSuccess = resolve; - this._onSetLocalDescriptionFailure = reject; + this._onSetDescriptionSuccess = resolve; + this._onSetDescriptionFailure = reject; this._impl.setLocalDescription( Ci.IPeerConnection.kActionRollback, "" ); }); + this._processTrackAdditionsAndRemovals(); + this._fireLegacyAddStreamEvents(); + this._transceivers = this._transceivers.filter(t => !t.shouldRemove); + this._updateCanTrickle(); } await new Promise((resolve, reject) => { - this._onSetRemoteDescriptionSuccess = resolve; - this._onSetRemoteDescriptionFailure = reject; + this._onSetDescriptionSuccess = resolve; + this._onSetDescriptionFailure = reject; this._impl.setRemoteDescription(action, sdp); }); + this._processTrackAdditionsAndRemovals(); + this._fireLegacyAddStreamEvents(); + this._transceivers = this._transceivers.filter(t => !t.shouldRemove); this._updateCanTrickle(); })(); @@ -1983,25 +1990,12 @@ class PeerConnectionObserver { this._dompc._onCreateAnswerFailure(this.newError(error)); } - onSetLocalDescriptionSuccess() { - this._dompc._onSetLocalDescriptionSuccess(); + onSetDescriptionSuccess() { + this._dompc._onSetDescriptionSuccess(); } - onSetRemoteDescriptionSuccess() { - this._dompc._processTrackAdditionsAndRemovals(); - this._dompc._fireLegacyAddStreamEvents(); - this._dompc._transceivers = this._dompc._transceivers.filter( - t => !t.shouldRemove - ); - this._dompc._onSetRemoteDescriptionSuccess(); - } - - onSetLocalDescriptionError(error) { - this._dompc._onSetLocalDescriptionFailure(this.newError(error)); - } - - onSetRemoteDescriptionError(error) { - this._dompc._onSetRemoteDescriptionFailure(this.newError(error)); + onSetDescriptionError(error) { + this._dompc._onSetDescriptionFailure(this.newError(error)); } onAddIceCandidateSuccess() { diff --git a/dom/webidl/PeerConnectionObserver.webidl b/dom/webidl/PeerConnectionObserver.webidl index 95b7c6e2e50d..b4769aa99205 100644 --- a/dom/webidl/PeerConnectionObserver.webidl +++ b/dom/webidl/PeerConnectionObserver.webidl @@ -26,10 +26,8 @@ interface PeerConnectionObserver void onCreateOfferError(PCErrorData error); void onCreateAnswerSuccess(DOMString answer); void onCreateAnswerError(PCErrorData error); - void onSetLocalDescriptionSuccess(); - void onSetRemoteDescriptionSuccess(); - void onSetLocalDescriptionError(PCErrorData error); - void onSetRemoteDescriptionError(PCErrorData error); + void onSetDescriptionSuccess(); + void onSetDescriptionError(PCErrorData error); void onAddIceCandidateSuccess(); void onAddIceCandidateError(PCErrorData error); void onIceCandidate(unsigned short level, DOMString mid, DOMString candidate, DOMString ufrag); diff --git a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp index a2e0549d0b93..29984480e3fd 100644 --- a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp +++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp @@ -1351,62 +1351,14 @@ PeerConnectionImpl::SetLocalDescription(int32_t aAction, const char* aSDP) { std::string errorString = mJsepSession->GetLastError(); CSFLogError(LOGTAG, "%s: pc = %s, error = %s", __FUNCTION__, mHandle.c_str(), errorString.c_str()); - mPCObserver->OnSetLocalDescriptionError( - *buildJSErrorData(result, errorString), rv); + mPCObserver->OnSetDescriptionError(*buildJSErrorData(result, errorString), + rv); } else { if (wasRestartingIce) { RecordIceRestartStatistics(sdpType); } - auto newSignalingState = GetSignalingState(); - // Spec says we queue a task for this stuff - mThread->Dispatch(NS_NewRunnableFunction( - __func__, - [this, self = RefPtr(this), newSignalingState] { - if (IsClosed()) { - return; - } - JSErrorResult jrv; - mPCObserver->SyncTransceivers(jrv); - if (NS_WARN_IF(jrv.Failed())) { - return; - } - mPendingRemoteDescription = - mJsepSession->GetRemoteDescription(kJsepDescriptionPending); - mCurrentRemoteDescription = - mJsepSession->GetRemoteDescription(kJsepDescriptionCurrent); - mPendingLocalDescription = - mJsepSession->GetLocalDescription(kJsepDescriptionPending); - mCurrentLocalDescription = - mJsepSession->GetLocalDescription(kJsepDescriptionCurrent); - mPendingOfferer = mJsepSession->IsPendingOfferer(); - mCurrentOfferer = mJsepSession->IsCurrentOfferer(); - if (newSignalingState != mSignalingState) { - mSignalingState = newSignalingState; - mPCObserver->OnStateChange(PCObserverStateType::SignalingState, - jrv); - } - mPCObserver->OnSetLocalDescriptionSuccess(jrv); - })); - - // We'd like to handle this in PeerConnectionMedia::UpdateNetworkState. - // Unfortunately, if the WiFi switch happens quickly, we never see - // that state change. We need to detect the ice restart here and - // reset the PeerConnectionMedia's stun addresses so they are - // regathered when PeerConnectionMedia::GatherIfReady is called. - if (sdpType == mozilla::kJsepSdpOffer && wasRestartingIce) { - mMedia->ResetStunAddrsForIceRestart(); - } - - // We do this after queueing the above task, to ensure that ICE state - // changes don't start happening before sRD finishes. - if (sdpType != mozilla::kJsepSdpRollback) { - mMedia->EnsureTransports(*mJsepSession); - } - - if (mJsepSession->GetState() == kJsepStateStable) { - OnOfferAnswerComplete(sdpType == mozilla::kJsepSdpRollback); - } + OnSetDescriptionSuccess(sdpType == mozilla::kJsepSdpRollback); } return NS_OK; @@ -1483,8 +1435,8 @@ PeerConnectionImpl::SetRemoteDescription(int32_t action, const char* aSDP) { std::string errorString = mJsepSession->GetLastError(); CSFLogError(LOGTAG, "%s: pc = %s, error = %s", __FUNCTION__, mHandle.c_str(), errorString.c_str()); - mPCObserver->OnSetRemoteDescriptionError( - *buildJSErrorData(result, errorString), jrv); + mPCObserver->OnSetDescriptionError(*buildJSErrorData(result, errorString), + jrv); } else { // Iterate over the JSEP transceivers that were just created for (size_t i = originalTransceiverCount; @@ -1535,51 +1487,7 @@ PeerConnectionImpl::SetRemoteDescription(int32_t action, const char* aSDP) { RecordIceRestartStatistics(sdpType); } - // Spec says we queue a task for all the stuff that ends up back in JS - auto newSignalingState = GetSignalingState(); - mThread->Dispatch(NS_NewRunnableFunction( - __func__, - [this, self = RefPtr(this), newSignalingState] { - if (IsClosed()) { - return; - } - JSErrorResult jrv; - mPCObserver->SyncTransceivers(jrv); - if (NS_WARN_IF(jrv.Failed())) { - return; - } - mPendingRemoteDescription = - mJsepSession->GetRemoteDescription(kJsepDescriptionPending); - mCurrentRemoteDescription = - mJsepSession->GetRemoteDescription(kJsepDescriptionCurrent); - mPendingLocalDescription = - mJsepSession->GetLocalDescription(kJsepDescriptionPending); - mCurrentLocalDescription = - mJsepSession->GetLocalDescription(kJsepDescriptionCurrent); - mPendingOfferer = mJsepSession->IsPendingOfferer(); - mCurrentOfferer = mJsepSession->IsCurrentOfferer(); - if (newSignalingState != mSignalingState) { - mSignalingState = newSignalingState; - mPCObserver->OnStateChange(PCObserverStateType::SignalingState, - jrv); - } - mPCObserver->OnSetRemoteDescriptionSuccess(jrv); - })); - - // We'd like to handle this in PeerConnectionMedia::UpdateNetworkState. - // Unfortunately, if the WiFi switch happens quickly, we never see - // that state change. We need to detect the ice restart here and - // reset the PeerConnectionMedia's stun addresses so they are - // regathered when PeerConnectionMedia::GatherIfReady is called. - if (sdpType == mozilla::kJsepSdpOffer && wasRestartingIce) { - mMedia->ResetStunAddrsForIceRestart(); - } - - // We do this after queueing the above task, to ensure that ICE state - // changes don't start happening before sRD finishes. - if (mJsepSession->GetState() == kJsepStateStable) { - OnOfferAnswerComplete(sdpType == mozilla::kJsepSdpRollback); - } + OnSetDescriptionSuccess(sdpType == kJsepSdpRollback); startCallTelem(); } @@ -2344,7 +2252,57 @@ void PeerConnectionImpl::ShutdownMedia() { mMedia.forget().take()->SelfDestruct(); } -void PeerConnectionImpl::OnOfferAnswerComplete(bool rollback) { +void PeerConnectionImpl::OnSetDescriptionSuccess(bool rollback) { + // Spec says we queue a task for all the stuff that ends up back in JS + auto newSignalingState = GetSignalingState(); + + mThread->Dispatch(NS_NewRunnableFunction( + __func__, + [this, self = RefPtr(this), newSignalingState] { + if (IsClosed()) { + return; + } + JSErrorResult jrv; + mPCObserver->SyncTransceivers(jrv); + if (NS_WARN_IF(jrv.Failed())) { + return; + } + mPendingRemoteDescription = + mJsepSession->GetRemoteDescription(kJsepDescriptionPending); + mCurrentRemoteDescription = + mJsepSession->GetRemoteDescription(kJsepDescriptionCurrent); + mPendingLocalDescription = + mJsepSession->GetLocalDescription(kJsepDescriptionPending); + mCurrentLocalDescription = + mJsepSession->GetLocalDescription(kJsepDescriptionCurrent); + mPendingOfferer = mJsepSession->IsPendingOfferer(); + mCurrentOfferer = mJsepSession->IsCurrentOfferer(); + if (newSignalingState != mSignalingState) { + mSignalingState = newSignalingState; + mPCObserver->OnStateChange(PCObserverStateType::SignalingState, jrv); + } + mPCObserver->OnSetDescriptionSuccess(jrv); + })); + + // We do this after queueing the above task, to ensure that ICE state + // changes don't start happening before sRD finishes. + if (!rollback && (newSignalingState == RTCSignalingState::Have_local_offer || + mSignalingState == RTCSignalingState::Have_remote_offer)) { + // We'd like to handle this in PeerConnectionMedia::UpdateNetworkState. + // Unfortunately, if the WiFi switch happens quickly, we never see + // that state change. We need to detect the ice restart here and + // reset the PeerConnectionMedia's stun addresses so they are + // regathered when PeerConnectionMedia::GatherIfReady is called. + if (mJsepSession->IsIceRestarting()) { + mMedia->ResetStunAddrsForIceRestart(); + } + mMedia->EnsureTransports(*mJsepSession); + } + + if (mJsepSession->GetState() != kJsepStateStable) { + return; // The rest of this stuff is done only when offer/answer is done + } + // If we're rolling back a local offer, we might need to remove some // transports, and stomp some MediaPipeline setup, but nothing further // needs to be done. @@ -2352,7 +2310,7 @@ void PeerConnectionImpl::OnOfferAnswerComplete(bool rollback) { if (NS_FAILED(mMedia->UpdateMediaPipelines())) { CSFLogError(LOGTAG, "Error Updating MediaPipelines"); NS_ASSERTION(false, - "Error Updating MediaPipelines in OnOfferAnswerComplete()"); + "Error Updating MediaPipelines in OnSetDescriptionSuccess()"); // XXX what now? Not much we can do but keep going, without major // restructuring } diff --git a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h index 139fbd9029c0..db7566213a0e 100644 --- a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h +++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h @@ -459,7 +459,7 @@ class PeerConnectionImpl final // Gets the RTC Signaling State of the JSEP session dom::RTCSignalingState GetSignalingState() const; - void OnOfferAnswerComplete(bool rollback); + void OnSetDescriptionSuccess(bool rollback); bool IsClosed() const; // called when DTLS connects; we only need this once