From c6801d4522ab94f965e258e68259fde312023654 Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Wed, 15 Jul 2020 08:38:33 +0200 Subject: [PATCH] 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} --- pc/session_description.h | 5 ++ pc/webrtc_sdp.cc | 102 +++++++++++++++++++++++--------------- pc/webrtc_sdp_unittest.cc | 24 ++++++++- 3 files changed, 88 insertions(+), 43 deletions(-) diff --git a/pc/session_description.h b/pc/session_description.h index 3405accbf3..91b2a8bbe4 100644 --- a/pc/session_description.h +++ b/pc/session_description.h @@ -126,6 +126,10 @@ 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) { @@ -251,6 +255,7 @@ 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 af584791be..ad005ff9b6 100644 --- a/pc/webrtc_sdp.cc +++ b/pc/webrtc_sdp.cc @@ -225,7 +225,8 @@ static const char kMediaPortRejected[] = "0"; static const char kDummyAddress[] = "0.0.0.0"; static const char kDummyPort[] = "9"; // RFC 3556 -static const char kApplicationSpecificMaximum[] = "AS"; +static const char kApplicationSpecificBandwidth[] = "AS"; +static const char kTransportSpecificBandwidth[] = "TIAS"; static const char kDefaultSctpmapProtocol[] = "webrtc-datachannel"; @@ -1437,9 +1438,14 @@ void BuildMediaDescription(const ContentInfo* content_info, // RFC 4566 // b=AS: - if (media_desc->bandwidth() >= 1000) { - InitLine(kLineTypeSessionBandwidth, kApplicationSpecificMaximum, &os); - os << kSdpDelimiterColon << (media_desc->bandwidth() / 1000); + 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; AddLine(os.str(), message); } @@ -2983,46 +2989,60 @@ bool ParseContent(const std::string& message, // b=* (zero or more bandwidth information lines) if (IsLineType(line, kLineTypeSessionBandwidth)) { std::string bandwidth; - if (HasAttribute(line, kApplicationSpecificMaximum)) { - if (!GetValue(line, kApplicationSpecificMaximum, &bandwidth, error)) { + std::string bandwidth_type; + if (HasAttribute(line, kApplicationSpecificBandwidth)) { + if (!GetValue(line, kApplicationSpecificBandwidth, &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; } - 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); } // Parse the media level connection data. diff --git a/pc/webrtc_sdp_unittest.cc b/pc/webrtc_sdp_unittest.cc index 7b83c86ab1..67bcb2ec26 100644 --- a/pc/webrtc_sdp_unittest.cc +++ b/pc/webrtc_sdp_unittest.cc @@ -2189,16 +2189,18 @@ TEST_F(WebRtcSdpTest, SerializeSessionDescriptionWithBundle) { TEST_F(WebRtcSdpTest, SerializeSessionDescriptionWithBandwidth) { VideoContentDescription* vcd = GetFirstVideoContentDescription(&desc_); - vcd->set_bandwidth(100 * 1000); + vcd->set_bandwidth(100 * 1000 + 755); // Integer division will drop the 755. + vcd->set_bandwidth_type("AS"); 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=AS:50\r\n", + InjectAfter("c=IN IP4 74.125.127.126\r\n", "b=TIAS:50000\r\n", &sdp_with_bandwidth); EXPECT_EQ(sdp_with_bandwidth, message); } @@ -2309,6 +2311,7 @@ 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); @@ -2612,6 +2615,23 @@ 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;