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 469516 90786b7142a8
parent 469515 4e44a4a0495a
child 469517 fcd532097b93
push id35873
push userccoroiu@mozilla.com
push dateMon, 15 Apr 2019 21:36:26 +0000
treeherdermozilla-central@b8f49a14c458 [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_