Revert "sdp: parse and serialize b=TIAS"

This reverts commit c6801d4522.

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 <philipp.hancke@googlemail.com>
> Reviewed-by: Taylor <deadbeef@webrtc.org>
> 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 <deadbeef@webrtc.org>
Commit-Queue: Taylor <deadbeef@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31762}
This commit is contained in:
Taylor 2020-07-17 19:37:51 +00:00 committed by Commit Bot
parent de90862cce
commit 20b701f3d7
3 changed files with 43 additions and 88 deletions

View file

@ -126,10 +126,6 @@ class MediaContentDescription {
virtual int bandwidth() const { return bandwidth_; } virtual int bandwidth() const { return bandwidth_; }
virtual void set_bandwidth(int bandwidth) { bandwidth_ = 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<CryptoParams>& cryptos() const { return cryptos_; } virtual const std::vector<CryptoParams>& cryptos() const { return cryptos_; }
virtual void AddCrypto(const CryptoParams& params) { virtual void AddCrypto(const CryptoParams& params) {
@ -255,7 +251,6 @@ class MediaContentDescription {
bool rtcp_reduced_size_ = false; bool rtcp_reduced_size_ = false;
bool remote_estimate_ = false; bool remote_estimate_ = false;
int bandwidth_ = kAutoBandwidth; int bandwidth_ = kAutoBandwidth;
std::string bandwidth_type_;
std::string protocol_; std::string protocol_;
std::vector<CryptoParams> cryptos_; std::vector<CryptoParams> cryptos_;
std::vector<webrtc::RtpExtension> rtp_header_extensions_; std::vector<webrtc::RtpExtension> rtp_header_extensions_;

View file

@ -225,8 +225,7 @@ static const char kMediaPortRejected[] = "0";
static const char kDummyAddress[] = "0.0.0.0"; static const char kDummyAddress[] = "0.0.0.0";
static const char kDummyPort[] = "9"; static const char kDummyPort[] = "9";
// RFC 3556 // RFC 3556
static const char kApplicationSpecificBandwidth[] = "AS"; static const char kApplicationSpecificMaximum[] = "AS";
static const char kTransportSpecificBandwidth[] = "TIAS";
static const char kDefaultSctpmapProtocol[] = "webrtc-datachannel"; static const char kDefaultSctpmapProtocol[] = "webrtc-datachannel";
@ -1438,14 +1437,9 @@ void BuildMediaDescription(const ContentInfo* content_info,
// RFC 4566 // RFC 4566
// b=AS:<bandwidth> // b=AS:<bandwidth>
int bandwidth = media_desc->bandwidth(); if (media_desc->bandwidth() >= 1000) {
if (bandwidth >= 1000) { InitLine(kLineTypeSessionBandwidth, kApplicationSpecificMaximum, &os);
std::string bandwidth_type = media_desc->bandwidth_type(); os << kSdpDelimiterColon << (media_desc->bandwidth() / 1000);
InitLine(kLineTypeSessionBandwidth, bandwidth_type, &os);
if (bandwidth_type == kApplicationSpecificBandwidth) {
bandwidth /= 1000;
}
os << kSdpDelimiterColon << bandwidth;
AddLine(os.str(), message); AddLine(os.str(), message);
} }
@ -2989,20 +2983,10 @@ bool ParseContent(const std::string& message,
// b=* (zero or more bandwidth information lines) // b=* (zero or more bandwidth information lines)
if (IsLineType(line, kLineTypeSessionBandwidth)) { if (IsLineType(line, kLineTypeSessionBandwidth)) {
std::string bandwidth; std::string bandwidth;
std::string bandwidth_type; if (HasAttribute(line, kApplicationSpecificMaximum)) {
if (HasAttribute(line, kApplicationSpecificBandwidth)) { if (!GetValue(line, kApplicationSpecificMaximum, &bandwidth, error)) {
if (!GetValue(line, kApplicationSpecificBandwidth, &bandwidth, error)) {
return false; return false;
}
bandwidth_type = kApplicationSpecificBandwidth;
} else if (HasAttribute(line, kTransportSpecificBandwidth)) {
if (!GetValue(line, kTransportSpecificBandwidth, &bandwidth, error)) {
return false;
}
bandwidth_type = kTransportSpecificBandwidth;
} else { } else {
continue;
}
int b = 0; int b = 0;
if (!GetValueFromString(line, bandwidth, &b, error)) { if (!GetValueFromString(line, bandwidth, &b, error)) {
return false; return false;
@ -3012,37 +2996,33 @@ bool ParseContent(const std::string& message,
// though ommitting the "b=AS" entirely will do just that. Once we've // though ommitting the "b=AS" entirely will do just that. Once we've
// transitioned applications to doing the right thing, it would be // transitioned applications to doing the right thing, it would be
// better to treat this as a hard error instead of just ignoring it. // better to treat this as a hard error instead of just ignoring it.
if (bandwidth_type == kApplicationSpecificBandwidth && b == -1) { if (b == -1) {
RTC_LOG(LS_WARNING) << "Ignoring \"b=AS:-1\"; will be treated as \"no " RTC_LOG(LS_WARNING)
<< "Ignoring \"b=AS:-1\"; will be treated as \"no "
"bandwidth limit\"."; "bandwidth limit\".";
continue; continue;
} }
if (b < 0) { if (b < 0) {
return ParseFailed( return ParseFailed(line, "b=AS value can't be negative.", error);
line, "b=" + bandwidth_type + " value can't be negative.", error);
} }
// We should never use more than the default bandwidth for RTP-based // We should never use more than the default bandwidth for RTP-based
// data channels. Don't allow SDP to set the bandwidth, because // data channels. Don't allow SDP to set the bandwidth, because
// that would give JS the opportunity to "break the Internet". // that would give JS the opportunity to "break the Internet".
// See: https://code.google.com/p/chromium/issues/detail?id=280726 // 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 && if (media_type == cricket::MEDIA_TYPE_DATA &&
cricket::IsRtpProtocol(protocol) && cricket::IsRtpProtocol(protocol) &&
(b > cricket::kDataMaxBandwidth / 1000 || b > cricket::kDataMaxBandwidth / 1000) {
bandwidth_type == kTransportSpecificBandwidth)) {
rtc::StringBuilder description; rtc::StringBuilder description;
description << "RTP-based data channels may not send more than " description << "RTP-based data channels may not send more than "
<< cricket::kDataMaxBandwidth / 1000 << "kbps."; << cricket::kDataMaxBandwidth / 1000 << "kbps.";
return ParseFailed(line, description.str(), error); return ParseFailed(line, description.str(), error);
} }
// Convert values. Prevent integer overflow. // Prevent integer overflow.
if (bandwidth_type == kApplicationSpecificBandwidth) { b = std::min(b, INT_MAX / 1000);
b = std::min(b, INT_MAX / 1000) * 1000; media_desc->set_bandwidth(b * 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. // Parse the media level connection data.

View file

@ -2189,18 +2189,16 @@ TEST_F(WebRtcSdpTest, SerializeSessionDescriptionWithBundle) {
TEST_F(WebRtcSdpTest, SerializeSessionDescriptionWithBandwidth) { TEST_F(WebRtcSdpTest, SerializeSessionDescriptionWithBandwidth) {
VideoContentDescription* vcd = GetFirstVideoContentDescription(&desc_); VideoContentDescription* vcd = GetFirstVideoContentDescription(&desc_);
vcd->set_bandwidth(100 * 1000 + 755); // Integer division will drop the 755. vcd->set_bandwidth(100 * 1000);
vcd->set_bandwidth_type("AS");
AudioContentDescription* acd = GetFirstAudioContentDescription(&desc_); AudioContentDescription* acd = GetFirstAudioContentDescription(&desc_);
acd->set_bandwidth(50 * 1000); acd->set_bandwidth(50 * 1000);
acd->set_bandwidth_type("TIAS");
ASSERT_TRUE(jdesc_.Initialize(desc_.Clone(), jdesc_.session_id(), ASSERT_TRUE(jdesc_.Initialize(desc_.Clone(), jdesc_.session_id(),
jdesc_.session_version())); jdesc_.session_version()));
std::string message = webrtc::SdpSerialize(jdesc_); std::string message = webrtc::SdpSerialize(jdesc_);
std::string sdp_with_bandwidth = kSdpFullString; std::string sdp_with_bandwidth = kSdpFullString;
InjectAfter("c=IN IP4 74.125.224.39\r\n", "b=AS:100\r\n", InjectAfter("c=IN IP4 74.125.224.39\r\n", "b=AS:100\r\n",
&sdp_with_bandwidth); &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); &sdp_with_bandwidth);
EXPECT_EQ(sdp_with_bandwidth, message); EXPECT_EQ(sdp_with_bandwidth, message);
} }
@ -2311,7 +2309,6 @@ TEST_F(WebRtcSdpTest, SerializeSessionDescriptionWithDataChannelAndBandwidth) {
JsepSessionDescription jsep_desc(kDummyType); JsepSessionDescription jsep_desc(kDummyType);
AddRtpDataChannel(); AddRtpDataChannel();
data_desc_->set_bandwidth(100 * 1000); data_desc_->set_bandwidth(100 * 1000);
data_desc_->set_bandwidth_type("AS");
MakeDescriptionWithoutCandidates(&jsep_desc); MakeDescriptionWithoutCandidates(&jsep_desc);
std::string message = webrtc::SdpSerialize(jsep_desc); std::string message = webrtc::SdpSerialize(jsep_desc);
@ -2615,23 +2612,6 @@ TEST_F(WebRtcSdpTest, DeserializeSessionDescriptionWithBandwidth) {
EXPECT_TRUE(CompareSessionDescription(jdesc_, jdesc_with_bandwidth)); 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) { TEST_F(WebRtcSdpTest, DeserializeSessionDescriptionWithIceOptions) {
JsepSessionDescription jdesc_with_ice_options(kDummyType); JsepSessionDescription jdesc_with_ice_options(kDummyType);
std::string sdp_with_ice_options = kSdpFullString; std::string sdp_with_ice_options = kSdpFullString;