From 438bea1ee6cf57142301ae04d1bfb3d56eacc437 Mon Sep 17 00:00:00 2001 From: Michael Froman Date: Tue, 5 Apr 2016 20:12:04 -0500 Subject: [PATCH] Bug 906986 - Rework rollback/finalize to include a committed state. r=bwc, r=drno MozReview-Commit-ID: z7uEn5xEBf --HG-- extra : rebase_source : 18565a3c49045af9445a93de8753110169caf465 --- media/mtransport/transportlayerice.cpp | 7 +- media/mtransport/transportlayerice.h | 5 +- .../peerconnection/MediaPipelineFactory.cpp | 2 +- .../src/peerconnection/PeerConnectionImpl.cpp | 67 ++++++++++++----- .../src/peerconnection/PeerConnectionImpl.h | 1 + .../peerconnection/PeerConnectionMedia.cpp | 71 +++++++++++++++++-- .../src/peerconnection/PeerConnectionMedia.h | 13 ++++ 7 files changed, 134 insertions(+), 32 deletions(-) diff --git a/media/mtransport/transportlayerice.cpp b/media/mtransport/transportlayerice.cpp index 71fbfbe5b16a..47bc9fd5502d 100644 --- a/media/mtransport/transportlayerice.cpp +++ b/media/mtransport/transportlayerice.cpp @@ -128,8 +128,6 @@ void TransportLayerIce::PostSetup() { &TransportLayerIce::IcePacketReceived); if (stream_->state() == NrIceMediaStream::ICE_OPEN) { TL_SET_STATE(TS_OPEN); - // Reset old ice stream if new stream is good - ResetOldStream(); } } @@ -137,7 +135,7 @@ void TransportLayerIce::ResetOldStream() { if (old_stream_ == nullptr) { return; // no work to do } - // ICE Ready on the new stream, we can forget the old stream now + // ICE restart successful on the new stream, we can forget the old stream now MOZ_MTLOG(ML_INFO, LAYER_INFO << "ResetOldStream(" << old_stream_->name() << ")"); old_stream_->SignalReady.disconnect(this); @@ -150,6 +148,7 @@ void TransportLayerIce::RestoreOldStream() { if (old_stream_ == nullptr) { return; // no work to do } + // ICE restart rollback, we need to restore the old stream MOZ_MTLOG(ML_INFO, LAYER_INFO << "RestoreOldStream(" << old_stream_->name() << ")"); stream_->SignalReady.disconnect(this); @@ -201,8 +200,6 @@ void TransportLayerIce::IceReady(NrIceMediaStream *stream) { MOZ_MTLOG(ML_INFO, LAYER_INFO << "ICE Ready(" << stream->name() << "," << component_ << ")"); TL_SET_STATE(TS_OPEN); - // Reset old ice stream if new stream is good after ice restart - ResetOldStream(); } void TransportLayerIce::IceFailed(NrIceMediaStream *stream) { diff --git a/media/mtransport/transportlayerice.h b/media/mtransport/transportlayerice.h index 99d2eac75c02..2489dc145937 100644 --- a/media/mtransport/transportlayerice.h +++ b/media/mtransport/transportlayerice.h @@ -38,6 +38,9 @@ class TransportLayerIce : public TransportLayer { RefPtr stream, int component); + void ResetOldStream(); // called after successful ice restart + void RestoreOldStream(); // called after unsuccessful ice restart + // Transport layer overrides. virtual TransportResult SendPacket(const unsigned char *data, size_t len); @@ -53,8 +56,6 @@ class TransportLayerIce : public TransportLayer { private: DISALLOW_COPY_ASSIGN(TransportLayerIce); void PostSetup(); - void ResetOldStream(); // called after successful ice restart - void RestoreOldStream(); // called after unsuccessful ice restart const std::string name_; RefPtr ctx_; diff --git a/media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp b/media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp index af0dd1b69c1b..bf16b5ac6718 100644 --- a/media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp +++ b/media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp @@ -227,7 +227,7 @@ MediaPipelineFactory::CreateOrGetTransportFlow( flow = mPCMedia->GetTransportFlow(aLevel, aIsRtcp); if (flow) { - if (mPCMedia->ice_ctx_hdlr()->IsRestarting()) { + if (mPCMedia->IsIceRestarting()) { MOZ_MTLOG(ML_INFO, "Flow[" << flow->id() << "]: " << "detected ICE restart - level: " << aLevel << " rtcp: " << aIsRtcp); diff --git a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp index 4ab4bdef14c6..7d1680bb535a 100644 --- a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp +++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp @@ -1545,7 +1545,9 @@ PeerConnectionImpl::CreateOffer(const JsepOfferOptions& aOptions) { PC_AUTO_ENTER_API_CALL(true); bool restartIce = aOptions.mIceRestart.isSome() && *(aOptions.mIceRestart); - if (!restartIce && mMedia->ice_ctx_hdlr()->IsRestarting()) { + if (!restartIce && + mMedia->GetIceRestartState() == + PeerConnectionMedia::ICE_RESTART_PROVISIONAL) { RollbackIceRestart(); } @@ -1565,8 +1567,21 @@ PeerConnectionImpl::CreateOffer(const JsepOfferOptions& aOptions) CSFLogDebug(logTag, "CreateOffer()"); nsresult nrv; - if (aOptions.mIceRestart.isSome() && *(aOptions.mIceRestart) && - !mMedia->ice_ctx_hdlr()->IsRestarting()) { + if (restartIce) { + // If restart is requested and a restart is already in progress, we + // need to make room for the restart request so we either rollback + // or finalize to "clear" the previous restart. + if (mMedia->GetIceRestartState() == + PeerConnectionMedia::ICE_RESTART_PROVISIONAL) { + // we're mid-restart and can rollback + RollbackIceRestart(); + } else if (mMedia->GetIceRestartState() == + PeerConnectionMedia::ICE_RESTART_COMMITTED) { + // we're mid-restart and can't rollback, finalize restart even + // though we're not really ready yet + FinalizeIceRestart(); + } + CSFLogInfo(logTag, "Offerer restarting ice"); nrv = SetupIceRestart(); if (NS_FAILED(nrv)) { @@ -1625,13 +1640,18 @@ PeerConnectionImpl::CreateAnswer() nsresult nrv; if (mJsepSession->RemoteIceIsRestarting()) { - CSFLogInfo(logTag, "Answerer restarting ice"); - nrv = SetupIceRestart(); - if (NS_FAILED(nrv)) { - CSFLogError(logTag, "%s: SetupIceRestart failed, res=%u", - __FUNCTION__, - static_cast(nrv)); - return nrv; + if (mMedia->GetIceRestartState() == + PeerConnectionMedia::ICE_RESTART_COMMITTED) { + FinalizeIceRestart(); + } else if (!mMedia->IsIceRestarting()) { + CSFLogInfo(logTag, "Answerer restarting ice"); + nrv = SetupIceRestart(); + if (NS_FAILED(nrv)) { + CSFLogError(logTag, "%s: SetupIceRestart failed, res=%u", + __FUNCTION__, + static_cast(nrv)); + return nrv; + } } } @@ -1669,7 +1689,7 @@ PeerConnectionImpl::CreateAnswer() nsresult PeerConnectionImpl::SetupIceRestart() { - if (mMedia->ice_ctx_hdlr()->IsRestarting()) { + if (mMedia->IsIceRestarting()) { CSFLogError(logTag, "%s: ICE already restarting", __FUNCTION__); return NS_ERROR_UNEXPECTED; @@ -1719,6 +1739,15 @@ PeerConnectionImpl::RollbackIceRestart() return NS_OK; } +void +PeerConnectionImpl::FinalizeIceRestart() +{ + mMedia->FinalizeIceRestart(); + // clear the previous ice creds since they are no longer needed + mPreviousIceUfrag = ""; + mPreviousIcePwd = ""; +} + NS_IMETHODIMP PeerConnectionImpl::SetLocalDescription(int32_t aAction, const char* aSDP) { @@ -2929,8 +2958,13 @@ PeerConnectionImpl::SetSignalingState_m(PCImplSignalingState aSignalingState, bool fireNegotiationNeeded = false; if (mSignalingState == PCImplSignalingState::SignalingStable) { - if (rollback && mMedia->ice_ctx_hdlr()->IsRestarting()) { - RollbackIceRestart(); + if (mMedia->GetIceRestartState() == + PeerConnectionMedia::ICE_RESTART_PROVISIONAL) { + if (rollback) { + RollbackIceRestart(); + } else { + mMedia->CommitIceRestart(); + } } // Either negotiation is done, or we've rolled back. In either case, we @@ -3227,11 +3261,8 @@ void PeerConnectionImpl::IceConnectionStateChange( if (mIceConnectionState == PCImplIceConnectionState::Connected || mIceConnectionState == PCImplIceConnectionState::Completed || mIceConnectionState == PCImplIceConnectionState::Failed) { - if (mMedia->ice_ctx_hdlr()->IsRestarting()) { - mMedia->FinalizeIceRestart(); - // clear the previous ice creds since they are no longer needed - mPreviousIceUfrag = ""; - mPreviousIcePwd = ""; + if (mMedia->IsIceRestarting()) { + FinalizeIceRestart(); } } diff --git a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h index eddf44a438a7..63de8cd37018 100644 --- a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h +++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h @@ -714,6 +714,7 @@ private: nsresult SetupIceRestart(); nsresult RollbackIceRestart(); + void FinalizeIceRestart(); #if !defined(MOZILLA_EXTERNAL_LINKAGE) static void GetStatsForPCObserver_s( diff --git a/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp b/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp index 3e808f71ad63..d6377781f2d4 100644 --- a/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp +++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp @@ -239,7 +239,8 @@ PeerConnectionMedia::PeerConnectionMedia(PeerConnectionImpl *parent) mUuidGen(MakeUnique()), mMainThread(mParent->GetMainThread()), mSTSThread(mParent->GetSTSThread()), - mProxyResolveCompleted(false) { + mProxyResolveCompleted(false), + mIceRestartState(ICE_RESTART_NONE) { } nsresult PeerConnectionMedia::Init(const std::vector& stun_servers, @@ -586,11 +587,30 @@ PeerConnectionMedia::StartIceChecks_s( mIceCtxHdlr->ctx()->StartChecks(); } +bool +PeerConnectionMedia::IsIceRestarting() const +{ + ASSERT_ON_THREAD(mMainThread); + + return (mIceRestartState != ICE_RESTART_NONE); +} + +PeerConnectionMedia::IceRestartState +PeerConnectionMedia::GetIceRestartState() const +{ + ASSERT_ON_THREAD(mMainThread); + + return mIceRestartState; +} + void PeerConnectionMedia::BeginIceRestart(const std::string& ufrag, const std::string& pwd) { ASSERT_ON_THREAD(mMainThread); + if (IsIceRestarting()) { + return; + } bool default_address_only = GetPrefDefaultAddressOnly(); RefPtr new_ctx = mIceCtxHdlr->CreateCtx(ufrag, @@ -603,6 +623,8 @@ PeerConnectionMedia::BeginIceRestart(const std::string& ufrag, &PeerConnectionMedia::BeginIceRestart_s, new_ctx), NS_DISPATCH_NORMAL); + + mIceRestartState = ICE_RESTART_PROVISIONAL; } void @@ -613,22 +635,37 @@ PeerConnectionMedia::BeginIceRestart_s(RefPtr new_ctx) // hold the original context so we can disconnect signals if needed RefPtr originalCtx = mIceCtxHdlr->ctx(); - mIceCtxHdlr->BeginIceRestart(new_ctx); - if (mIceCtxHdlr->IsRestarting()) { + if (mIceCtxHdlr->BeginIceRestart(new_ctx)) { ConnectSignals(mIceCtxHdlr->ctx().get(), originalCtx.get()); } } +void +PeerConnectionMedia::CommitIceRestart() +{ + ASSERT_ON_THREAD(mMainThread); + if (mIceRestartState != ICE_RESTART_PROVISIONAL) { + return; + } + + mIceRestartState = ICE_RESTART_COMMITTED; +} + void PeerConnectionMedia::FinalizeIceRestart() { ASSERT_ON_THREAD(mMainThread); + if (!IsIceRestarting()) { + return; + } RUN_ON_THREAD(GetSTSThread(), WrapRunnable( RefPtr(this), &PeerConnectionMedia::FinalizeIceRestart_s), NS_DISPATCH_NORMAL); + + mIceRestartState = ICE_RESTART_NONE; } void @@ -636,6 +673,17 @@ PeerConnectionMedia::FinalizeIceRestart_s() { ASSERT_ON_THREAD(mSTSThread); + // reset old streams since we don't need them anymore + for (auto i = mTransportFlows.begin(); + i != mTransportFlows.end(); + ++i) { + RefPtr aFlow = i->second; + if (!aFlow) continue; + TransportLayerIce* ice = + static_cast(aFlow->GetLayer(TransportLayerIce::ID())); + ice->ResetOldStream(); + } + mIceCtxHdlr->FinalizeIceRestart(); } @@ -643,6 +691,9 @@ void PeerConnectionMedia::RollbackIceRestart() { ASSERT_ON_THREAD(mMainThread); + if (mIceRestartState != ICE_RESTART_PROVISIONAL) { + return; + } RUN_ON_THREAD(GetSTSThread(), WrapRunnable( @@ -655,13 +706,21 @@ void PeerConnectionMedia::RollbackIceRestart_s() { ASSERT_ON_THREAD(mSTSThread); - if (!mIceCtxHdlr->IsRestarting()) { - return; - } // hold the restart context so we can disconnect signals RefPtr restartCtx = mIceCtxHdlr->ctx(); + // restore old streams since we're rolling back + for (auto i = mTransportFlows.begin(); + i != mTransportFlows.end(); + ++i) { + RefPtr aFlow = i->second; + if (!aFlow) continue; + TransportLayerIce* ice = + static_cast(aFlow->GetLayer(TransportLayerIce::ID())); + ice->RestoreOldStream(); + } + mIceCtxHdlr->RollbackIceRestart(); ConnectSignals(mIceCtxHdlr->ctx().get(), restartCtx.get()); } diff --git a/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h b/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h index e1342d04be45..9ea5bb8f2343 100644 --- a/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h +++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h @@ -288,6 +288,11 @@ class PeerConnectionMedia : public sigslot::has_slots<> { public: explicit PeerConnectionMedia(PeerConnectionImpl *parent); + enum IceRestartState { ICE_RESTART_NONE, + ICE_RESTART_PROVISIONAL, + ICE_RESTART_COMMITTED + }; + PeerConnectionImpl* GetPC() { return mParent; } nsresult Init(const std::vector& stun_servers, const std::vector& turn_servers, @@ -316,9 +321,14 @@ class PeerConnectionMedia : public sigslot::has_slots<> { // Start ICE checks. void StartIceChecks(const JsepSession& session); + bool IsIceRestarting() const; + IceRestartState GetIceRestartState() const; + // Begin ICE restart void BeginIceRestart(const std::string& ufrag, const std::string& pwd); + // Commit ICE Restart - offer/answer complete, no rollback possible + void CommitIceRestart(); // Finalize ICE restart void FinalizeIceRestart(); // Abort ICE restart @@ -613,6 +623,9 @@ class PeerConnectionMedia : public sigslot::has_slots<> { // Used to store the result of the request. UniquePtr mProxyServer; + // Used to track the state of ice restart + IceRestartState mIceRestartState; + NS_INLINE_DECL_THREADSAFE_REFCOUNTING(PeerConnectionMedia) };