From 20b701f3d79c499b0981f03fbf3a9b0fe531ac5d Mon Sep 17 00:00:00 2001 From: Taylor Date: Fri, 17 Jul 2020 19:37:51 +0000 Subject: [PATCH] Revert "sdp: parse and serialize b=TIAS" This reverts commit c6801d4522ab94f965e258e68259fde312023654. Reason for revert: Speculatively reverting since it possibly breaks downstream performance test. One issue I noticed is that the correct SDP won't be produced if set_bandwidth_type hasn't been called. Probably should default to b=AS in that case. Original change's description: > sdp: parse and serialize b=TIAS > > BUG=webrtc:5788 > > Change-Id: I063c756004e4c224fffa36d2800603c7b7e50dce > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/179223 > Commit-Queue: Philipp Hancke > Reviewed-by: Taylor > Cr-Commit-Position: refs/heads/master@{#31729} TBR=deadbeef@webrtc.org,hta@webrtc.org,minyue@webrtc.org,philipp.hancke@googlemail.com,jleconte@webrtc.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: webrtc:5788 Change-Id: I2a3f676b4359834e511dffd5adedc9388e0ea0f8 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/179620 Reviewed-by: Taylor Commit-Queue: Taylor Cr-Commit-Position: refs/heads/master@{#31762} --- pc/session_description.h | 5 -- pc/webrtc_sdp.cc | 102 +++++++++++++++----------------------- pc/webrtc_sdp_unittest.cc | 24 +-------- 3 files changed, 43 insertions(+), 88 deletions(-) diff --git a/pc/session_description.h b/pc/session_description.h index 91b2a8bbe4..3405accbf3 100644 --- a/pc/session_description.h +++ b/pc/session_description.h @@ -126,10 +126,6 @@ class MediaContentDescription { virtual int bandwidth() const { return bandwidth_; } virtual void set_bandwidth(int bandwidth) { bandwidth_ = bandwidth; } - virtual std::string bandwidth_type() const { return bandwidth_type_; } - virtual void set_bandwidth_type(std::string bandwidth_type) { - bandwidth_type_ = bandwidth_type; - } virtual const std::vector& cryptos() const { return cryptos_; } virtual void AddCrypto(const CryptoParams& params) { @@ -255,7 +251,6 @@ class MediaContentDescription { bool rtcp_reduced_size_ = false; bool remote_estimate_ = false; int bandwidth_ = kAutoBandwidth; - std::string bandwidth_type_; std::string protocol_; std::vector cryptos_; std::vector rtp_header_extensions_; diff --git a/pc/webrtc_sdp.cc b/pc/webrtc_sdp.cc index ad005ff9b6..af584791be 100644 --- a/pc/webrtc_sdp.cc +++ b/pc/webrtc_sdp.cc @@ -225,8 +225,7 @@ static const char kMediaPortRejected[] = "0"; static const char kDummyAddress[] = "0.0.0.0"; static const char kDummyPort[] = "9"; // RFC 3556 -static const char kApplicationSpecificBandwidth[] = "AS"; -static const char kTransportSpecificBandwidth[] = "TIAS"; +static const char kApplicationSpecificMaximum[] = "AS"; static const char kDefaultSctpmapProtocol[] = "webrtc-datachannel"; @@ -1438,14 +1437,9 @@ void BuildMediaDescription(const ContentInfo* content_info, // RFC 4566 // b=AS: - int bandwidth = media_desc->bandwidth(); - if (bandwidth >= 1000) { - std::string bandwidth_type = media_desc->bandwidth_type(); - InitLine(kLineTypeSessionBandwidth, bandwidth_type, &os); - if (bandwidth_type == kApplicationSpecificBandwidth) { - bandwidth /= 1000; - } - os << kSdpDelimiterColon << bandwidth; + if (media_desc->bandwidth() >= 1000) { + InitLine(kLineTypeSessionBandwidth, kApplicationSpecificMaximum, &os); + os << kSdpDelimiterColon << (media_desc->bandwidth() / 1000); AddLine(os.str(), message); } @@ -2989,60 +2983,46 @@ bool ParseContent(const std::string& message, // b=* (zero or more bandwidth information lines) if (IsLineType(line, kLineTypeSessionBandwidth)) { std::string bandwidth; - std::string bandwidth_type; - if (HasAttribute(line, kApplicationSpecificBandwidth)) { - if (!GetValue(line, kApplicationSpecificBandwidth, &bandwidth, error)) { + if (HasAttribute(line, kApplicationSpecificMaximum)) { + if (!GetValue(line, kApplicationSpecificMaximum, &bandwidth, error)) { return false; + } else { + int b = 0; + if (!GetValueFromString(line, bandwidth, &b, error)) { + return false; + } + // TODO(deadbeef): Historically, applications may be setting a value + // of -1 to mean "unset any previously set bandwidth limit", even + // though ommitting the "b=AS" entirely will do just that. Once we've + // transitioned applications to doing the right thing, it would be + // better to treat this as a hard error instead of just ignoring it. + if (b == -1) { + RTC_LOG(LS_WARNING) + << "Ignoring \"b=AS:-1\"; will be treated as \"no " + "bandwidth limit\"."; + continue; + } + if (b < 0) { + return ParseFailed(line, "b=AS value can't be negative.", error); + } + // We should never use more than the default bandwidth for RTP-based + // data channels. Don't allow SDP to set the bandwidth, because + // that would give JS the opportunity to "break the Internet". + // See: https://code.google.com/p/chromium/issues/detail?id=280726 + if (media_type == cricket::MEDIA_TYPE_DATA && + cricket::IsRtpProtocol(protocol) && + b > cricket::kDataMaxBandwidth / 1000) { + rtc::StringBuilder description; + description << "RTP-based data channels may not send more than " + << cricket::kDataMaxBandwidth / 1000 << "kbps."; + return ParseFailed(line, description.str(), error); + } + // Prevent integer overflow. + b = std::min(b, INT_MAX / 1000); + media_desc->set_bandwidth(b * 1000); } - bandwidth_type = kApplicationSpecificBandwidth; - } else if (HasAttribute(line, kTransportSpecificBandwidth)) { - if (!GetValue(line, kTransportSpecificBandwidth, &bandwidth, error)) { - return false; - } - bandwidth_type = kTransportSpecificBandwidth; - } else { - continue; } - int b = 0; - if (!GetValueFromString(line, bandwidth, &b, error)) { - return false; - } - // TODO(deadbeef): Historically, applications may be setting a value - // of -1 to mean "unset any previously set bandwidth limit", even - // though ommitting the "b=AS" entirely will do just that. Once we've - // transitioned applications to doing the right thing, it would be - // better to treat this as a hard error instead of just ignoring it. - if (bandwidth_type == kApplicationSpecificBandwidth && b == -1) { - RTC_LOG(LS_WARNING) << "Ignoring \"b=AS:-1\"; will be treated as \"no " - "bandwidth limit\"."; - continue; - } - if (b < 0) { - return ParseFailed( - line, "b=" + bandwidth_type + " value can't be negative.", error); - } - // We should never use more than the default bandwidth for RTP-based - // data channels. Don't allow SDP to set the bandwidth, because - // that would give JS the opportunity to "break the Internet". - // See: https://code.google.com/p/chromium/issues/detail?id=280726 - // Disallow TIAS since that is not generated for this. - if (media_type == cricket::MEDIA_TYPE_DATA && - cricket::IsRtpProtocol(protocol) && - (b > cricket::kDataMaxBandwidth / 1000 || - bandwidth_type == kTransportSpecificBandwidth)) { - rtc::StringBuilder description; - description << "RTP-based data channels may not send more than " - << cricket::kDataMaxBandwidth / 1000 << "kbps."; - return ParseFailed(line, description.str(), error); - } - // Convert values. Prevent integer overflow. - if (bandwidth_type == kApplicationSpecificBandwidth) { - b = std::min(b, INT_MAX / 1000) * 1000; - } else { - b = std::min(b, INT_MAX); - } - media_desc->set_bandwidth(b); - media_desc->set_bandwidth_type(bandwidth_type); + continue; } // Parse the media level connection data. diff --git a/pc/webrtc_sdp_unittest.cc b/pc/webrtc_sdp_unittest.cc index 67bcb2ec26..7b83c86ab1 100644 --- a/pc/webrtc_sdp_unittest.cc +++ b/pc/webrtc_sdp_unittest.cc @@ -2189,18 +2189,16 @@ TEST_F(WebRtcSdpTest, SerializeSessionDescriptionWithBundle) { TEST_F(WebRtcSdpTest, SerializeSessionDescriptionWithBandwidth) { VideoContentDescription* vcd = GetFirstVideoContentDescription(&desc_); - vcd->set_bandwidth(100 * 1000 + 755); // Integer division will drop the 755. - vcd->set_bandwidth_type("AS"); + vcd->set_bandwidth(100 * 1000); AudioContentDescription* acd = GetFirstAudioContentDescription(&desc_); acd->set_bandwidth(50 * 1000); - acd->set_bandwidth_type("TIAS"); ASSERT_TRUE(jdesc_.Initialize(desc_.Clone(), jdesc_.session_id(), jdesc_.session_version())); std::string message = webrtc::SdpSerialize(jdesc_); std::string sdp_with_bandwidth = kSdpFullString; InjectAfter("c=IN IP4 74.125.224.39\r\n", "b=AS:100\r\n", &sdp_with_bandwidth); - InjectAfter("c=IN IP4 74.125.127.126\r\n", "b=TIAS:50000\r\n", + InjectAfter("c=IN IP4 74.125.127.126\r\n", "b=AS:50\r\n", &sdp_with_bandwidth); EXPECT_EQ(sdp_with_bandwidth, message); } @@ -2311,7 +2309,6 @@ TEST_F(WebRtcSdpTest, SerializeSessionDescriptionWithDataChannelAndBandwidth) { JsepSessionDescription jsep_desc(kDummyType); AddRtpDataChannel(); data_desc_->set_bandwidth(100 * 1000); - data_desc_->set_bandwidth_type("AS"); MakeDescriptionWithoutCandidates(&jsep_desc); std::string message = webrtc::SdpSerialize(jsep_desc); @@ -2615,23 +2612,6 @@ TEST_F(WebRtcSdpTest, DeserializeSessionDescriptionWithBandwidth) { EXPECT_TRUE(CompareSessionDescription(jdesc_, jdesc_with_bandwidth)); } -TEST_F(WebRtcSdpTest, DeserializeSessionDescriptionWithTiasBandwidth) { - JsepSessionDescription jdesc_with_bandwidth(kDummyType); - std::string sdp_with_bandwidth = kSdpFullString; - InjectAfter("a=mid:video_content_name\r\na=sendrecv\r\n", "b=TIAS:100000\r\n", - &sdp_with_bandwidth); - InjectAfter("a=mid:audio_content_name\r\na=sendrecv\r\n", "b=TIAS:50000\r\n", - &sdp_with_bandwidth); - EXPECT_TRUE(SdpDeserialize(sdp_with_bandwidth, &jdesc_with_bandwidth)); - VideoContentDescription* vcd = GetFirstVideoContentDescription(&desc_); - vcd->set_bandwidth(100 * 1000); - AudioContentDescription* acd = GetFirstAudioContentDescription(&desc_); - acd->set_bandwidth(50 * 1000); - ASSERT_TRUE(jdesc_.Initialize(desc_.Clone(), jdesc_.session_id(), - jdesc_.session_version())); - EXPECT_TRUE(CompareSessionDescription(jdesc_, jdesc_with_bandwidth)); -} - TEST_F(WebRtcSdpTest, DeserializeSessionDescriptionWithIceOptions) { JsepSessionDescription jdesc_with_ice_options(kDummyType); std::string sdp_with_ice_options = kSdpFullString;