admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Revert "Reject the subsequent offer with fewer m= sections."

Browse Source 
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 <deadbeef@webrtc.org>
> Commit-Queue: Zhi Huang <zhihuang@webrtc.org>
> 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 <tommi@webrtc.org>
Commit-Queue: Tommi <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#20300}
This commit is contained in:
Tommi 2017-10-15 21:20:46 +00:00 committed by Commit Bot
parent 8eb9c7d838
commit 589ae45d0f
2 changed files with 20 additions and 61 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

33
pc/peerconnectioninterface_unittest.cc
View File

@ -3740,7 +3740,7 @@ TEST_F(PeerConnectionInterfaceTest, CreateAnswerWithoutRemoteDescription) {
EXPECT_TRUE(DoCreateAnswer(&answer, nullptr)); 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. // respect the order of existing media sections.
TEST_F(PeerConnectionInterfaceTest, TEST_F(PeerConnectionInterfaceTest,
MediaSectionOrderEnforcedForSubsequentOffers) { MediaSectionOrderEnforcedForSubsequentOffers) {
@ -3773,37 +3773,6 @@ TEST_F(PeerConnectionInterfaceTest,
EXPECT_FALSE(DoSetLocalDescription(std::move(offer))); 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<SessionDescriptionInterface> 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<SessionDescriptionInterface> 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 { class PeerConnectionMediaConfigTest : public testing::Test {
protected: protected:
void SetUp() override { void SetUp() override {

48
pc/webrtcsession.cc
View File

@ -140,30 +140,25 @@ IceCandidatePairType GetIceCandidatePairCounter(
return kIceCandidatePairMax; return kIceCandidatePairMax;
} }
// Verify that the order of media sections in |new_desc| matches // Verify that the order of media sections in |desc1| matches |desc2|. The
// |existing_desc|. The number of m= sections in |new_desc| should be no less // number of m= sections could be different.
// than |existing_desc|. static bool MediaSectionsInSameOrder(const SessionDescription* desc1,
static bool MediaSectionsInSameOrder(const SessionDescription* existing_desc, const SessionDescription* desc2) {
const SessionDescription* new_desc) { if (!desc1 || !desc2) {
if (!existing_desc || !new_desc) {
return false; return false;
} }
for (size_t i = 0;
if (existing_desc->contents().size() > new_desc->contents().size()) { i < desc1->contents().size() && i < desc2->contents().size(); ++i) {
if ((desc2->contents()[i].name) != desc1->contents()[i].name) {
return false; return false;
} }
const MediaContentDescription* desc2_mdesc =
for (size_t i = 0; i < existing_desc->contents().size(); ++i) {
if (new_desc->contents()[i].name != existing_desc->contents()[i].name) {
return false;
}
const MediaContentDescription* new_desc_mdesc =
static_cast<const MediaContentDescription*>( static_cast<const MediaContentDescription*>(
new_desc->contents()[i].description); desc2->contents()[i].description);
const MediaContentDescription* existing_desc_mdesc = const MediaContentDescription* desc1_mdesc =
static_cast<const MediaContentDescription*>( static_cast<const MediaContentDescription*>(
existing_desc->contents()[i].description); desc1->contents()[i].description);
if (new_desc_mdesc->type() != existing_desc_mdesc->type()) { if (desc2_mdesc->type() != desc1_mdesc->type()) {
return false; return false;
} }
} }
@ -2170,21 +2165,16 @@ bool WebRtcSession::ValidateSessionDescription(
const cricket::SessionDescription* offer_desc = const cricket::SessionDescription* offer_desc =
(source == cricket::CS_LOCAL) ? remote_description()->description() (source == cricket::CS_LOCAL) ? remote_description()->description()
: local_description()->description(); : local_description()->description();
if (!MediaSectionsHaveSameCount(offer_desc, sdesc->description()) || if (!MediaSectionsHaveSameCount(sdesc->description(), offer_desc) ||
!MediaSectionsInSameOrder(offer_desc, sdesc->description())) { !MediaSectionsInSameOrder(sdesc->description(), offer_desc)) {
return BadAnswerSdp(source, kMlineMismatchInAnswer, err_desc); return BadAnswerSdp(source, kMlineMismatchInAnswer, err_desc);
} }
} else { } else {
const cricket::SessionDescription* current_desc = nullptr; // The re-offers should respect the order of m= sections in current local
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
// description. See RFC3264 Section 8 paragraph 4 for more details. // description. See RFC3264 Section 8 paragraph 4 for more details.
if (current_desc && if (local_description() &&
!MediaSectionsInSameOrder(current_desc, sdesc->description())) { !MediaSectionsInSameOrder(sdesc->description(),
local_description()->description())) {
return BadOfferSdp(source, kMlineMismatchInSubsequentOffer, err_desc); return BadOfferSdp(source, kMlineMismatchInSubsequentOffer, err_desc);
} }
} }