From ba6ea78677846f23efb4cd82ed642530af9c2a70 Mon Sep 17 00:00:00 2001 From: Byron Campen Date: Fri, 3 Feb 2023 16:34:23 +0000 Subject: [PATCH] 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 | 2 +- dom/media/webrtc/jsapi/RTCRtpSender.cpp | 6 ++ dom/media/webrtc/jsapi/RTCRtpTransceiver.cpp | 2 - dom/media/webrtc/jsep/JsepSessionImpl.cpp | 7 +- dom/media/webrtc/jsep/JsepTransceiver.h | 17 +++-- .../signaling/gtest/jsep_session_unittest.cpp | 65 +++++++++++-------- .../webrtc/RTCRtpTransceiver.https.html.ini | 4 -- 7 files changed, 59 insertions(+), 44 deletions(-) diff --git a/dom/media/PeerConnection.jsm b/dom/media/PeerConnection.jsm index 074fc1576125..bf9b9a637133 100644 --- a/dom/media/PeerConnection.jsm +++ b/dom/media/PeerConnection.jsm @@ -1390,9 +1390,9 @@ class RTCPeerConnection { streams, direction: "sendrecv", }); + transceiver.setAddTrackMagic(); } - transceiver.setAddTrackMagic(); this.updateNegotiationNeeded(); return transceiver.sender; } diff --git a/dom/media/webrtc/jsapi/RTCRtpSender.cpp b/dom/media/webrtc/jsapi/RTCRtpSender.cpp index 1a77ddb79c74..11705501d241 100644 --- a/dom/media/webrtc/jsapi/RTCRtpSender.cpp +++ b/dom/media/webrtc/jsapi/RTCRtpSender.cpp @@ -1144,6 +1144,12 @@ void RTCRtpSender::SetTrack(const RefPtr& 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( diff --git a/dom/media/webrtc/jsapi/RTCRtpTransceiver.cpp b/dom/media/webrtc/jsapi/RTCRtpTransceiver.cpp index 5a078c2b0645..2b3d365bcb0b 100644 --- a/dom/media/webrtc/jsapi/RTCRtpTransceiver.cpp +++ b/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(); diff --git a/dom/media/webrtc/jsep/JsepSessionImpl.cpp b/dom/media/webrtc/jsep/JsepSessionImpl.cpp index 754de9c8fc5b..ced5690c0d19 100644 --- a/dom/media/webrtc/jsep/JsepSessionImpl.cpp +++ b/dom/media/webrtc/jsep/JsepSessionImpl.cpp @@ -1634,7 +1634,7 @@ JsepTransceiver* JsepSessionImpl::GetTransceiverForRemote( RefPtr 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 temp( @@ -1762,7 +1759,7 @@ void JsepSessionImpl::RollbackRemoteOffer() { InitTransceiver(*temp); transceiver->Rollback(*temp, true); - if (shouldRemove) { + if (transceiver->OnlyExistsBecauseOfSetRemote()) { transceiver->Stop(); transceiver->SetRemoved(); } diff --git a/dom/media/webrtc/jsep/JsepTransceiver.h b/dom/media/webrtc/jsep/JsepTransceiver.h index 1a5c08c3086e..734f7a3dbd98 100644 --- a/dom/media/webrtc/jsep/JsepTransceiver.h +++ b/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; diff --git a/media/webrtc/signaling/gtest/jsep_session_unittest.cpp b/media/webrtc/signaling/gtest/jsep_session_unittest.cpp index 43b62b5f2810..7334d3f39453 100644 --- a/media/webrtc/signaling/gtest/jsep_session_unittest.cpp +++ b/media/webrtc/signaling/gtest/jsep_session_unittest.cpp @@ -306,6 +306,7 @@ class JsepSessionTest : public JsepSessionTestBase, RefPtr 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); + if (magic == ADDTRACK_MAGIC) { + suitableTransceiver->SetAddTrackMagic(); + } } std::cerr << "Updating send track for transceiver " << i << std::endl; - if (magic == ADDTRACK_MAGIC) { - suitableTransceiver->SetAddTrackMagic(); - } + 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()); diff --git a/testing/web-platform/meta/webrtc/RTCRtpTransceiver.https.html.ini b/testing/web-platform/meta/webrtc/RTCRtpTransceiver.https.html.ini index 72b464207ddc..78993e041bd0 100644 --- a/testing/web-platform/meta/webrtc/RTCRtpTransceiver.https.html.ini +++ b/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