Bug 1767820: Only set addTrack magic for transceivers _created_ by addTrack. r=mjf

Differential Revision: https://phabricator.services.mozilla.com/D168570

dom/media/PeerConnection.jsm

@@ -1390,9 +1390,9 @@ class RTCPeerConnection {
         streams,
         direction: "sendrecv",
       });
+      transceiver.setAddTrackMagic();
     }
 
-    transceiver.setAddTrackMagic();
     this.updateNegotiationNeeded();
     return transceiver.sender;
   }

dom/media/webrtc/jsapi/RTCRtpSender.cpp

@@ -1144,6 +1144,12 @@ void RTCRtpSender::SetTrack(const RefPtr<MediaStreamTrack>& aTrack) {
   // Used for RTCPeerConnection.removeTrack and RTCPeerConnection.addTrack
   mSenderTrack = aTrack;
   SeamlessTrackSwitch(aTrack);
+  if (aTrack) {
+    // RFC says:
+    // However, an RtpTransceiver MUST NOT be removed if a track was attached
+    // to the RtpTransceiver via the addTrack method.
+    GetJsepTransceiver().SetOnlyExistsBecauseOfSetRemote(false);
+  }
 }
 
 bool RTCRtpSender::SetSenderTrackWithClosedCheck(

dom/media/webrtc/jsapi/RTCRtpTransceiver.cpp

@@ -584,8 +584,6 @@ void RTCRtpTransceiver::SetDirectionInternal(
 }
 
 void RTCRtpTransceiver::SetAddTrackMagic() {
-  // TODO(bug 1767820): Refactor this to only forbid removal, not to set the
-  // magic bit
   // We do this immediately, without waiting for a SyncToJsep, because this is
   // set at transceiver creation time.
   GetJsepTransceiver()->SetAddTrackMagic();

dom/media/webrtc/jsep/JsepSessionImpl.cpp

@@ -1634,7 +1634,7 @@ JsepTransceiver* JsepSessionImpl::GetTransceiverForRemote(
   RefPtr<JsepTransceiver> newTransceiver(new JsepTransceiver(
       msection.GetMediaType(), *mUuidGen, SdpDirectionAttribute::kRecvonly));
   newTransceiver->SetLevel(level);
-  newTransceiver->SetCreatedBySetRemote();
+  newTransceiver->SetOnlyExistsBecauseOfSetRemote(true);
   AddTransceiver(newTransceiver);
   return newTransceiver.get();
 }
@@ -1752,9 +1752,6 @@ void JsepSessionImpl::RollbackRemoteOffer() {
     }
 
     // New transceiver!
-    bool shouldRemove = !transceiver->HasAddTrackMagic() &&
-                        transceiver->WasCreatedBySetRemote();
-
     // We rollback even for transceivers we will remove, just to ensure we end
     // up at the starting state.
     RefPtr<JsepTransceiver> temp(
@@ -1762,7 +1759,7 @@ void JsepSessionImpl::RollbackRemoteOffer() {
     InitTransceiver(*temp);
     transceiver->Rollback(*temp, true);
 
-    if (shouldRemove) {
+    if (transceiver->OnlyExistsBecauseOfSetRemote()) {
       transceiver->Stop();
       transceiver->SetRemoved();
     }

dom/media/webrtc/jsep/JsepTransceiver.h

@@ -41,7 +41,7 @@ class JsepTransceiver {
         mLevel(SIZE_MAX),
         mBundleLevel(SIZE_MAX),
         mAddTrackMagic(false),
-        mWasCreatedBySetRemote(false),
+        mOnlyExistsBecauseOfSetRemote(false),
         mStopped(false),
         mRemoved(false),
         mNegotiated(false),
@@ -62,7 +62,7 @@ class JsepTransceiver {
         mLevel(orig.mLevel),
         mBundleLevel(orig.mBundleLevel),
         mAddTrackMagic(orig.mAddTrackMagic),
-        mWasCreatedBySetRemote(orig.mWasCreatedBySetRemote),
+        mOnlyExistsBecauseOfSetRemote(orig.mOnlyExistsBecauseOfSetRemote),
         mStopped(orig.mStopped),
         mRemoved(orig.mRemoved),
         mNegotiated(orig.mNegotiated),
@@ -162,9 +162,13 @@ class JsepTransceiver {
 
   bool HasAddTrackMagic() const { return mAddTrackMagic; }
 
-  void SetCreatedBySetRemote() { mWasCreatedBySetRemote = true; }
+  void SetOnlyExistsBecauseOfSetRemote(bool aValue) {
+    mOnlyExistsBecauseOfSetRemote = aValue;
+  }
 
-  bool WasCreatedBySetRemote() const { return mWasCreatedBySetRemote; }
+  bool OnlyExistsBecauseOfSetRemote() const {
+    return mOnlyExistsBecauseOfSetRemote;
+  }
 
   void SetNegotiated() {
     MOZ_ASSERT(IsAssociated());
@@ -215,9 +219,10 @@ class JsepTransceiver {
   // Is this track pair sharing a transport with another?
   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.
+  // when addTrack is used to create a transceiver. This was a deliberate
+  // design choice. Sadface.
   bool mAddTrackMagic;
-  bool mWasCreatedBySetRemote;
+  bool mOnlyExistsBecauseOfSetRemote;
   bool mStopped;
   bool mRemoved;
   bool mNegotiated;

media/webrtc/signaling/gtest/jsep_session_unittest.cpp

@@ -306,6 +306,7 @@ class JsepSessionTest : public JsepSessionTestBase,
       RefPtr<JsepTransceiver> suitableTransceiver;
       size_t i;
       if (magic == ADDTRACK_MAGIC) {
+        // We're simulating addTrack.
         for (i = 0; i < side.GetTransceivers().size(); ++i) {
           auto transceiver = side.GetTransceivers()[i];
           if (transceiver->mSendTrack.GetMediaType() != type) {
@@ -324,12 +325,13 @@ class JsepSessionTest : public JsepSessionTestBase,
         i = side.GetTransceivers().size();
         suitableTransceiver = new JsepTransceiver(type, mUuidGen);
         side.AddTransceiver(suitableTransceiver);
-      }
-
-      std::cerr << "Updating send track for transceiver " << i << std::endl;
         if (magic == ADDTRACK_MAGIC) {
           suitableTransceiver->SetAddTrackMagic();
         }
+      }
+
+      std::cerr << "Updating send track for transceiver " << i << std::endl;
+      suitableTransceiver->SetOnlyExistsBecauseOfSetRemote(false);
       suitableTransceiver->mJsDirection |=
           SdpDirectionAttribute::Direction::kSendonly;
       suitableTransceiver->mSendTrack.UpdateStreamIds(
@@ -6459,10 +6461,8 @@ TEST_F(JsepSessionTest, AnswerWithoutVP8) {
 
 // Ok. Hear me out.
 // The JSEP spec specifies very different behavior for the following two cases:
-// 1. AddTrack either caused a transceiver to be created, or set the send
-// track on a preexisting transceiver.
-// 2. The transceiver was not created as a side-effect of AddTrack, and the
-// send track was put in place by some other means than AddTrack.
+// 1. AddTrack caused a transceiver to be created.
+// 2. The transceiver was not created as a side-effect of AddTrack.
 //
 // All together now...
 //
@@ -6885,9 +6885,7 @@ TEST_F(JsepSessionTest, NoAddTrackMagicReplaceTrack) {
   ASSERT_TRUE(mSessionAns->GetTransceivers()[3]->IsAssociated());
 }
 
-// Check that transceivers that were created without a send track, but that
-// were subsequently given a send track with addTrack, are now "magical".
-TEST_F(JsepSessionTest, AddTrackMakesTransceiverMagical) {
+TEST_F(JsepSessionTest, AddTrackDoesNotMakeTransceiverMagical) {
   types = BuildTypes("audio,video");
   AddTracks(*mSessionOff);
   AddTracks(*mSessionAns);
@@ -6912,7 +6910,9 @@ TEST_F(JsepSessionTest, AddTrackMakesTransceiverMagical) {
   ASSERT_FALSE(mSessionAns->GetTransceivers()[2]->IsAssociated());
   ASSERT_FALSE(mSessionAns->GetTransceivers()[2]->HasAddTrackMagic());
 
-  // :D MAGIC! D:
+  // This attaches a track to the third transceiver, but does _not_ set the
+  // addTrack magic bit, meaning it will not auto-pair with the track added
+  // to the offerer.
   AddTracks(*mSessionAns, "audio");
 
   ASSERT_EQ(3U, mSessionAns->GetTransceivers().size());
@@ -6925,21 +6925,26 @@ TEST_F(JsepSessionTest, AddTrackMakesTransceiverMagical) {
   ASSERT_FALSE(mSessionAns->GetTransceivers()[2]->HasLevel());
   ASSERT_FALSE(mSessionAns->GetTransceivers()[2]->IsStopped());
   ASSERT_FALSE(mSessionAns->GetTransceivers()[2]->IsAssociated());
-  ASSERT_TRUE(mSessionAns->GetTransceivers()[2]->HasAddTrackMagic());
+  ASSERT_FALSE(mSessionAns->GetTransceivers()[2]->HasAddTrackMagic());
 
   OfferAnswer(CHECK_SUCCESS);
 
-  ASSERT_EQ(3U, mSessionAns->GetTransceivers().size());
+  // The offer's new transceiver does not pair up with the transceiver we added
+  ASSERT_EQ(4U, mSessionAns->GetTransceivers().size());
   ASSERT_EQ(0U, mSessionAns->GetTransceivers()[0]->GetLevel());
   ASSERT_FALSE(mSessionAns->GetTransceivers()[0]->IsStopped());
   ASSERT_TRUE(mSessionAns->GetTransceivers()[0]->IsAssociated());
   ASSERT_EQ(1U, mSessionAns->GetTransceivers()[1]->GetLevel());
   ASSERT_FALSE(mSessionAns->GetTransceivers()[1]->IsStopped());
   ASSERT_TRUE(mSessionAns->GetTransceivers()[1]->IsAssociated());
-  ASSERT_EQ(2U, mSessionAns->GetTransceivers()[2]->GetLevel());
+  ASSERT_FALSE(mSessionAns->GetTransceivers()[2]->HasLevel());
   ASSERT_FALSE(mSessionAns->GetTransceivers()[2]->IsStopped());
-  ASSERT_TRUE(mSessionAns->GetTransceivers()[2]->IsAssociated());
-  ASSERT_TRUE(mSessionAns->GetTransceivers()[2]->HasAddTrackMagic());
+  ASSERT_FALSE(mSessionAns->GetTransceivers()[2]->IsAssociated());
+  ASSERT_FALSE(mSessionAns->GetTransceivers()[2]->HasAddTrackMagic());
+  ASSERT_EQ(2U, mSessionAns->GetTransceivers()[3]->GetLevel());
+  ASSERT_FALSE(mSessionAns->GetTransceivers()[3]->IsStopped());
+  ASSERT_TRUE(mSessionAns->GetTransceivers()[3]->IsAssociated());
+  ASSERT_FALSE(mSessionAns->GetTransceivers()[3]->HasAddTrackMagic());
 }
 
 TEST_F(JsepSessionTest, ComplicatedRemoteRollback) {
@@ -6958,41 +6963,47 @@ TEST_F(JsepSessionTest, ComplicatedRemoteRollback) {
   ASSERT_FALSE(mSessionAns->GetTransceivers()[0]->IsStopped());
   ASSERT_TRUE(mSessionAns->GetTransceivers()[0]->IsAssociated());
   ASSERT_TRUE(mSessionAns->GetTransceivers()[0]->HasAddTrackMagic());
-  ASSERT_FALSE(mSessionAns->GetTransceivers()[0]->WasCreatedBySetRemote());
+  ASSERT_FALSE(
+      mSessionAns->GetTransceivers()[0]->OnlyExistsBecauseOfSetRemote());
 
   // Second video transceiver, not matched with offer
   ASSERT_FALSE(mSessionAns->GetTransceivers()[1]->HasLevel());
   ASSERT_FALSE(mSessionAns->GetTransceivers()[1]->IsStopped());
   ASSERT_FALSE(mSessionAns->GetTransceivers()[1]->IsAssociated());
   ASSERT_TRUE(mSessionAns->GetTransceivers()[1]->HasAddTrackMagic());
-  ASSERT_FALSE(mSessionAns->GetTransceivers()[1]->WasCreatedBySetRemote());
+  ASSERT_FALSE(
+      mSessionAns->GetTransceivers()[1]->OnlyExistsBecauseOfSetRemote());
 
   // Audio transceiver, created due to application of SetRemote
   ASSERT_EQ(0U, mSessionAns->GetTransceivers()[2]->GetLevel());
   ASSERT_FALSE(mSessionAns->GetTransceivers()[2]->IsStopped());
   ASSERT_TRUE(mSessionAns->GetTransceivers()[2]->IsAssociated());
   ASSERT_FALSE(mSessionAns->GetTransceivers()[2]->HasAddTrackMagic());
-  ASSERT_TRUE(mSessionAns->GetTransceivers()[2]->WasCreatedBySetRemote());
+  ASSERT_TRUE(
+      mSessionAns->GetTransceivers()[2]->OnlyExistsBecauseOfSetRemote());
 
   // Audio transceiver, created due to application of SetRemote
   ASSERT_EQ(1U, mSessionAns->GetTransceivers()[3]->GetLevel());
   ASSERT_FALSE(mSessionAns->GetTransceivers()[3]->IsStopped());
   ASSERT_TRUE(mSessionAns->GetTransceivers()[3]->IsAssociated());
   ASSERT_FALSE(mSessionAns->GetTransceivers()[3]->HasAddTrackMagic());
-  ASSERT_TRUE(mSessionAns->GetTransceivers()[3]->WasCreatedBySetRemote());
+  ASSERT_TRUE(
+      mSessionAns->GetTransceivers()[3]->OnlyExistsBecauseOfSetRemote());
 
   // Audio transceiver, created due to application of SetRemote
   ASSERT_EQ(2U, mSessionAns->GetTransceivers()[4]->GetLevel());
   ASSERT_FALSE(mSessionAns->GetTransceivers()[4]->IsStopped());
   ASSERT_TRUE(mSessionAns->GetTransceivers()[4]->IsAssociated());
   ASSERT_FALSE(mSessionAns->GetTransceivers()[4]->HasAddTrackMagic());
-  ASSERT_TRUE(mSessionAns->GetTransceivers()[4]->WasCreatedBySetRemote());
+  ASSERT_TRUE(
+      mSessionAns->GetTransceivers()[4]->OnlyExistsBecauseOfSetRemote());
 
-  // This will cause the first audio transceiver to become "magical", and
-  // thereby it will stick around after rollback, even though we clear it out
-  // with replaceTrack.
+  // This will prevent rollback from eating this transceiver, even though we
+  // call replaceTrack(null) on it.
   AddTracks(*mSessionAns, "audio");
-  ASSERT_TRUE(mSessionAns->GetTransceivers()[2]->HasAddTrackMagic());
+  ASSERT_FALSE(mSessionAns->GetTransceivers()[2]->HasAddTrackMagic());
+  ASSERT_FALSE(
+      mSessionAns->GetTransceivers()[2]->OnlyExistsBecauseOfSetRemote());
   mSessionAns->GetTransceivers()[2]->mSendTrack.ClearStreamIds();
   mSessionAns->GetTransceivers()[2]->mJsDirection =
       SdpDirectionAttribute::Direction::kRecvonly;
@@ -7037,7 +7048,9 @@ TEST_F(JsepSessionTest, ComplicatedRemoteRollback) {
   ASSERT_FALSE(mSessionAns->GetTransceivers()[2]->HasLevel());
   ASSERT_FALSE(mSessionAns->GetTransceivers()[2]->IsStopped());
   ASSERT_FALSE(mSessionAns->GetTransceivers()[2]->IsAssociated());
-  ASSERT_TRUE(mSessionAns->GetTransceivers()[2]->HasAddTrackMagic());
+  ASSERT_FALSE(mSessionAns->GetTransceivers()[2]->HasAddTrackMagic());
+  ASSERT_FALSE(
+      mSessionAns->GetTransceivers()[2]->OnlyExistsBecauseOfSetRemote());
   ASSERT_TRUE(IsNull(mSessionAns->GetTransceivers()[2]->mSendTrack));
   ASSERT_FALSE(mSessionAns->GetTransceivers()[2]->IsRemoved());
 

testing/web-platform/meta/webrtc/RTCRtpTransceiver.https.html.ini

@@ -37,10 +37,6 @@
     bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1568296
     expected: FAIL
 
-  [checkAddTransceiverThenAddTrackPairs]
-    bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1767820
-    expected: FAIL
-
   [checkMsectionReuse]
     bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1568296
     expected: FAIL