Bug 1543425 - Part 1: Ensure that just-added (and just-stopped) transceivers do not have their m-section recycled just because that m-section was disabled last negotiation. r=mjf
authorByron Campen [:bwc] <docfaraday@gmail.com>
Thu, 11 Apr 2019 15:08:21 +0000
changeset 469528 90786b7142a87c74d04baba8663ea5840d80fbc6
parent 469527 4e44a4a0495a2753160ca76247effe746919c847
child 469529 fcd532097b933b2a7cd4f43f3e0c786a2ba94e5a
push id112801
push userccoroiu@mozilla.com
push dateMon, 15 Apr 2019 21:40:09 +0000
treeherdermozilla-inbound@afb20612c0e5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmjf
bugs1543425
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1543425 - Part 1: Ensure that just-added (and just-stopped) transceivers do not have their m-section recycled just because that m-section was disabled last negotiation. r=mjf Differential Revision: https://phabricator.services.mozilla.com/D26932
media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
media/webrtc/signaling/src/jsep/JsepSessionImpl.h
media/webrtc/signaling/src/jsep/JsepTransceiver.h
--- a/media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
+++ b/media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
@@ -972,16 +972,17 @@ nsresult JsepSessionImpl::HandleNegotiat
     // Skip disabled m-sections.
     if (answer.GetMediaSection(i).GetPort() == 0) {
       transceiver->mTransport.Close();
       transceiver->Stop();
       transceiver->Disassociate();
       transceiver->ClearBundleLevel();
       transceiver->mSendTrack.SetActive(false);
       transceiver->mRecvTrack.SetActive(false);
+      transceiver->SetCanRecycle();
       // Do not clear mLevel yet! That will happen on the next negotiation.
       continue;
     }
 
     rv = MakeNegotiatedTransceiver(remote->GetMediaSection(i),
                                    local->GetMediaSection(i), transceiver);
     NS_ENSURE_SUCCESS(rv, rv);
   }
@@ -1384,17 +1385,17 @@ JsepTransceiver* JsepSessionImpl::GetTra
     }
   }
 
   return nullptr;
 }
 
 JsepTransceiver* JsepSessionImpl::GetTransceiverForLocal(size_t level) {
   if (JsepTransceiver* transceiver = GetTransceiverForLevel(level)) {
-    if (WasMsectionDisabledLastNegotiation(level) && transceiver->IsStopped() &&
+    if (transceiver->CanRecycle() &&
         transceiver->GetMediaType() != SdpMediaSection::kApplication) {
       // Attempt to recycle. If this fails, the old transceiver stays put.
       transceiver->Disassociate();
       JsepTransceiver* newTransceiver =
           FindUnassociatedTransceiver(transceiver->GetMediaType(), false);
       if (newTransceiver) {
         newTransceiver->SetLevel(level);
         transceiver->ClearLevel();
@@ -1426,18 +1427,17 @@ JsepTransceiver* JsepSessionImpl::GetTra
 
   return nullptr;
 }
 
 JsepTransceiver* JsepSessionImpl::GetTransceiverForRemote(
     const SdpMediaSection& msection) {
   size_t level = msection.GetLevel();
   if (JsepTransceiver* transceiver = GetTransceiverForLevel(level)) {
-    if (!WasMsectionDisabledLastNegotiation(level) ||
-        !transceiver->IsStopped()) {
+    if (!transceiver->CanRecycle()) {
       return transceiver;
     }
     transceiver->Disassociate();
     transceiver->ClearLevel();
   }
 
   // No transceiver for |level|
 
@@ -1515,26 +1515,16 @@ nsresult JsepSessionImpl::UpdateTranscei
     }
 
     transceiver->mRecvTrack.UpdateRecvTrack(remote, msection);
   }
 
   return NS_OK;
 }
 
-bool JsepSessionImpl::WasMsectionDisabledLastNegotiation(size_t level) const {
-  const Sdp* answer(GetAnswer());
-
-  if (answer && (level < answer->GetMediaSectionCount())) {
-    return mSdpHelper.MsectionIsDisabled(answer->GetMediaSection(level));
-  }
-
-  return false;
-}
-
 JsepTransceiver* JsepSessionImpl::FindUnassociatedTransceiver(
     SdpMediaSection::MediaType type, bool magic) {
   // Look through transceivers that are not mapped to an m-section
   for (RefPtr<JsepTransceiver>& transceiver : mTransceivers) {
     if (type == SdpMediaSection::kApplication &&
         type == transceiver->GetMediaType()) {
       transceiver->RestartDatachannelTransceiver();
       return transceiver.get();
--- a/media/webrtc/signaling/src/jsep/JsepSessionImpl.h
+++ b/media/webrtc/signaling/src/jsep/JsepSessionImpl.h
@@ -167,17 +167,16 @@ class JsepSessionImpl : public JsepSessi
   nsresult SetLocalDescriptionAnswer(JsepSdpType type, UniquePtr<Sdp> answer);
   nsresult SetRemoteDescriptionOffer(UniquePtr<Sdp> offer);
   nsresult SetRemoteDescriptionAnswer(JsepSdpType type, UniquePtr<Sdp> answer);
   nsresult ValidateLocalDescription(const Sdp& description, JsepSdpType type);
   nsresult ValidateRemoteDescription(const Sdp& description);
   nsresult ValidateOffer(const Sdp& offer);
   nsresult ValidateAnswer(const Sdp& offer, const Sdp& answer);
   nsresult UpdateTransceiversFromRemoteDescription(const Sdp& remote);
-  bool WasMsectionDisabledLastNegotiation(size_t level) const;
   JsepTransceiver* GetTransceiverForLevel(size_t level);
   JsepTransceiver* GetTransceiverForMid(const std::string& mid);
   JsepTransceiver* GetTransceiverForLocal(size_t level);
   JsepTransceiver* GetTransceiverForRemote(const SdpMediaSection& msection);
   JsepTransceiver* GetTransceiverWithTransport(const std::string& transportId);
   // The w3c and IETF specs have a lot of "magical" behavior that happens when
   // addTrack is used. This was a deliberate design choice. Sadface.
   JsepTransceiver* FindUnassociatedTransceiver(SdpMediaSection::MediaType type,
--- a/media/webrtc/signaling/src/jsep/JsepTransceiver.h
+++ b/media/webrtc/signaling/src/jsep/JsepTransceiver.h
@@ -31,32 +31,34 @@ class JsepTransceiver {
         mSendTrack(type, sdp::kSend),
         mRecvTrack(type, sdp::kRecv),
         mLevel(SIZE_MAX),
         mBundleLevel(SIZE_MAX),
         mAddTrackMagic(false),
         mWasCreatedBySetRemote(false),
         mStopped(false),
         mRemoved(false),
-        mNegotiated(false) {}
+        mNegotiated(false),
+        mCanRecycle(false) {}
 
   // Can't use default copy c'tor because of the refcount members. Ugh.
   JsepTransceiver(const JsepTransceiver& orig)
       : mJsDirection(orig.mJsDirection),
         mSendTrack(orig.mSendTrack),
         mRecvTrack(orig.mRecvTrack),
         mTransport(orig.mTransport),
         mMid(orig.mMid),
         mLevel(orig.mLevel),
         mBundleLevel(orig.mBundleLevel),
         mAddTrackMagic(orig.mAddTrackMagic),
         mWasCreatedBySetRemote(orig.mWasCreatedBySetRemote),
         mStopped(orig.mStopped),
         mRemoved(orig.mRemoved),
-        mNegotiated(orig.mNegotiated) {}
+        mNegotiated(orig.mNegotiated),
+        mCanRecycle(orig.mCanRecycle) {}
 
   NS_INLINE_DECL_THREADSAFE_REFCOUNTING(JsepTransceiver);
 
   void Rollback(JsepTransceiver& oldTransceiver, bool rollbackLevel) {
     MOZ_ASSERT(oldTransceiver.GetMediaType() == GetMediaType());
     MOZ_ASSERT(!oldTransceiver.IsNegotiated() || !oldTransceiver.HasLevel() ||
                !HasLevel() || oldTransceiver.GetLevel() == GetLevel());
     mTransport = oldTransceiver.mTransport;
@@ -150,16 +152,20 @@ class JsepTransceiver {
   void SetNegotiated() {
     MOZ_ASSERT(IsAssociated());
     MOZ_ASSERT(HasLevel());
     mNegotiated = true;
   }
 
   bool IsNegotiated() const { return mNegotiated; }
 
+  void SetCanRecycle() { mCanRecycle = true; }
+
+  bool CanRecycle() const { return mCanRecycle; }
+
   // Convenience function
   SdpMediaSection::MediaType GetMediaType() const {
     MOZ_ASSERT(mRecvTrack.GetMediaType() == mSendTrack.GetMediaType());
     return mRecvTrack.GetMediaType();
   }
 
   bool HasOwnTransport() const {
     if (mTransport.mComponents &&
@@ -184,13 +190,14 @@ class JsepTransceiver {
   size_t mBundleLevel;  // SIZE_MAX if no bundle level
   // The w3c and IETF specs have a lot of "magical" behavior that happens
   // when addTrack is used. This was a deliberate design choice. Sadface.
   bool mAddTrackMagic;
   bool mWasCreatedBySetRemote;
   bool mStopped;
   bool mRemoved;
   bool mNegotiated;
+  bool mCanRecycle;
 };
 
 }  // namespace mozilla
 
 #endif  // _JSEPTRANSCEIVER_H_