From 589ae45d0fa79e4d637bb21ce6bb0696f74db448 Mon Sep 17 00:00:00 2001 From: Tommi Date: Sun, 15 Oct 2017 21:20:46 +0000 Subject: [PATCH] Revert "Reject the subsequent offer with fewer m= sections." This reverts commit a8264dbdd97f5e125d45fd0e84356f2e1f747df1. Reason for revert: Reverting to unblock rolls into Chromium. See failure here: https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/565449 Fails: external/wpt/webrtc/RTCPeerConnection-setRemoteDescription-offer.html I'm guessing these lines from the output are relevant: 12:15:32.525 11839 [1:19:1015/121532.495175:16438293900:ERROR:webrtcsession.cc(350)] Failed to set remote offer sdp: The order of m-lines in subsequent offer doesn't match order from previous offer/answer. 12:15:32.525 11839 [1:20:1015/121532.497199:16438296127:WARNING:delay_based_bwe.cc(326)] BWE Setting start bitrate to: 300000 12:15:32.525 11839 [1:1:1015/121532.498272:16438296963:ERROR:webrtcsdp.cc(359)] Failed to parse: "Invalid SDP". Reason: Expect line: v= 12:15:32.525 11839 [1:1:1015/121532.498364:16438297040:ERROR:rtc_peer_connection_handler.cc(2183)] Failed to create native session description. Type: offer SDP: Invalid SDP 12:15:32.525 11839 [1:1:1015/121532.498432:16438297104:ERROR:rtc_peer_connection_handler.cc(1458)] Failed to parse SessionDescription. Invalid SDP Expect line: v= Original change's description: > Reject the subsequent offer with fewer m= sections. > > If the subsequent offer contains fewer m= sections than the existing > description, it would be rejected. > > The helper method MediaSectionsInSameOrder is modified and it will > compare the number of m= sections before matching the media type. > > Bug: chromium:773620 > Change-Id: Ic8999445f4bc023da1d85a65659583db1687ec37 > Reviewed-on: https://webrtc-review.googlesource.com/9621 > Reviewed-by: Taylor Brandstetter > Commit-Queue: Zhi Huang > Cr-Commit-Position: refs/heads/master@{#20298} TBR=deadbeef@webrtc.org,zhihuang@webrtc.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: chromium:773620 Change-Id: I4a3ff7a42abb95144615b1dd37fb21585ee07b5d Reviewed-on: https://webrtc-review.googlesource.com/10920 Reviewed-by: Tommi Commit-Queue: Tommi Cr-Commit-Position: refs/heads/master@{#20300} --- pc/peerconnectioninterface_unittest.cc | 33 +----------------- pc/webrtcsession.cc | 48 ++++++++++---------------- 2 files changed, 20 insertions(+), 61 deletions(-) diff --git a/pc/peerconnectioninterface_unittest.cc b/pc/peerconnectioninterface_unittest.cc index 2925f3ed0d..7a275e5b89 100644 --- a/pc/peerconnectioninterface_unittest.cc +++ b/pc/peerconnectioninterface_unittest.cc @@ -3740,7 +3740,7 @@ TEST_F(PeerConnectionInterfaceTest, CreateAnswerWithoutRemoteDescription) { EXPECT_TRUE(DoCreateAnswer(&answer, nullptr)); } -// Tests that an error is returned if a description is applied that doesn't +// Test that an error is returned if a description is applied that doesn't // respect the order of existing media sections. TEST_F(PeerConnectionInterfaceTest, MediaSectionOrderEnforcedForSubsequentOffers) { @@ -3773,37 +3773,6 @@ TEST_F(PeerConnectionInterfaceTest, EXPECT_FALSE(DoSetLocalDescription(std::move(offer))); } -// Tests that an error is returned if a description is applied that has fewer -// media sections than the existing description. -TEST_F(PeerConnectionInterfaceTest, - MediaSectionCountEnforcedForSubsequentOffer) { - CreatePeerConnection(); - FakeConstraints constraints; - constraints.SetMandatoryReceiveAudio(true); - constraints.SetMandatoryReceiveVideo(true); - std::unique_ptr offer; - ASSERT_TRUE(DoCreateOffer(&offer, &constraints)); - EXPECT_TRUE(DoSetRemoteDescription(std::move(offer))); - - // A remote offer with fewer media sections should be rejected. - ASSERT_TRUE(DoCreateOffer(&offer, &constraints)); - offer->description()->contents().pop_back(); - offer->description()->contents().pop_back(); - ASSERT_TRUE(offer->description()->contents().empty()); - EXPECT_FALSE(DoSetRemoteDescription(std::move(offer))); - - std::unique_ptr answer; - ASSERT_TRUE(DoCreateAnswer(&answer, nullptr)); - EXPECT_TRUE(DoSetLocalDescription(std::move(answer))); - - // A subsequent local offer with fewer media sections should be rejected. - ASSERT_TRUE(DoCreateOffer(&offer, &constraints)); - offer->description()->contents().pop_back(); - offer->description()->contents().pop_back(); - ASSERT_TRUE(offer->description()->contents().empty()); - EXPECT_FALSE(DoSetLocalDescription(std::move(offer))); -} - class PeerConnectionMediaConfigTest : public testing::Test { protected: void SetUp() override { diff --git a/pc/webrtcsession.cc b/pc/webrtcsession.cc index 55d029608c..f556204c6c 100644 --- a/pc/webrtcsession.cc +++ b/pc/webrtcsession.cc @@ -140,30 +140,25 @@ IceCandidatePairType GetIceCandidatePairCounter( return kIceCandidatePairMax; } -// Verify that the order of media sections in |new_desc| matches -// |existing_desc|. The number of m= sections in |new_desc| should be no less -// than |existing_desc|. -static bool MediaSectionsInSameOrder(const SessionDescription* existing_desc, - const SessionDescription* new_desc) { - if (!existing_desc || !new_desc) { +// Verify that the order of media sections in |desc1| matches |desc2|. The +// number of m= sections could be different. +static bool MediaSectionsInSameOrder(const SessionDescription* desc1, + const SessionDescription* desc2) { + if (!desc1 || !desc2) { return false; } - - if (existing_desc->contents().size() > new_desc->contents().size()) { - return false; - } - - for (size_t i = 0; i < existing_desc->contents().size(); ++i) { - if (new_desc->contents()[i].name != existing_desc->contents()[i].name) { + for (size_t i = 0; + i < desc1->contents().size() && i < desc2->contents().size(); ++i) { + if ((desc2->contents()[i].name) != desc1->contents()[i].name) { return false; } - const MediaContentDescription* new_desc_mdesc = + const MediaContentDescription* desc2_mdesc = static_cast( - new_desc->contents()[i].description); - const MediaContentDescription* existing_desc_mdesc = + desc2->contents()[i].description); + const MediaContentDescription* desc1_mdesc = static_cast( - existing_desc->contents()[i].description); - if (new_desc_mdesc->type() != existing_desc_mdesc->type()) { + desc1->contents()[i].description); + if (desc2_mdesc->type() != desc1_mdesc->type()) { return false; } } @@ -2170,21 +2165,16 @@ bool WebRtcSession::ValidateSessionDescription( const cricket::SessionDescription* offer_desc = (source == cricket::CS_LOCAL) ? remote_description()->description() : local_description()->description(); - if (!MediaSectionsHaveSameCount(offer_desc, sdesc->description()) || - !MediaSectionsInSameOrder(offer_desc, sdesc->description())) { + if (!MediaSectionsHaveSameCount(sdesc->description(), offer_desc) || + !MediaSectionsInSameOrder(sdesc->description(), offer_desc)) { return BadAnswerSdp(source, kMlineMismatchInAnswer, err_desc); } } else { - const cricket::SessionDescription* current_desc = nullptr; - if (source == cricket::CS_LOCAL && local_description()) { - current_desc = local_description()->description(); - } else if (source == cricket::CS_REMOTE && remote_description()) { - current_desc = remote_description()->description(); - } - // The re-offers should respect the order of m= sections in current + // The re-offers should respect the order of m= sections in current local // description. See RFC3264 Section 8 paragraph 4 for more details. - if (current_desc && - !MediaSectionsInSameOrder(current_desc, sdesc->description())) { + if (local_description() && + !MediaSectionsInSameOrder(sdesc->description(), + local_description()->description())) { return BadOfferSdp(source, kMlineMismatchInSubsequentOffer, err_desc); } }