Reland "sdp: parse and serialize b=TIAS"
This reverts commit 20b701f3d79c499b0981f03fbf3a9b0fe531ac5d.
Reason for reland: Reverting did not affect the test regression.
Original change's description:
> 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 <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}
TBR=nisse@webrtc.org
Bug: webrtc:5788
Change-Id: I5c0ef29d275bb2264d9b706b085f7933d59e2801
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/179760
Commit-Queue: Taylor <deadbeef@webrtc.org>
Reviewed-by: Taylor <deadbeef@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31816}
diff --git a/media/base/media_constants.cc b/media/base/media_constants.cc
index a918078..bb3403f 100644
--- a/media/base/media_constants.cc
+++ b/media/base/media_constants.cc
@@ -119,4 +119,8 @@
const size_t kConferenceMaxNumSpatialLayers = 3;
const size_t kConferenceMaxNumTemporalLayers = 3;
const size_t kConferenceDefaultNumTemporalLayers = 3;
+
+// RFC 3556 and RFC 3890
+const char kApplicationSpecificBandwidth[] = "AS";
+const char kTransportSpecificBandwidth[] = "TIAS";
} // namespace cricket
diff --git a/media/base/media_constants.h b/media/base/media_constants.h
index 5579b6e..4528167 100644
--- a/media/base/media_constants.h
+++ b/media/base/media_constants.h
@@ -145,6 +145,9 @@
extern const size_t kConferenceMaxNumSpatialLayers;
extern const size_t kConferenceMaxNumTemporalLayers;
extern const size_t kConferenceDefaultNumTemporalLayers;
+
+extern const char kApplicationSpecificBandwidth[];
+extern const char kTransportSpecificBandwidth[];
} // namespace cricket
#endif // MEDIA_BASE_MEDIA_CONSTANTS_H_
diff --git a/pc/session_description.h b/pc/session_description.h
index 3405acc..53c981a 100644
--- a/pc/session_description.h
+++ b/pc/session_description.h
@@ -26,6 +26,7 @@
#include "api/rtp_parameters.h"
#include "api/rtp_transceiver_interface.h"
#include "media/base/media_channel.h"
+#include "media/base/media_constants.h"
#include "media/base/stream_params.h"
#include "p2p/base/transport_description.h"
#include "p2p/base/transport_info.h"
@@ -126,6 +127,10 @@
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<CryptoParams>& cryptos() const { return cryptos_; }
virtual void AddCrypto(const CryptoParams& params) {
@@ -251,6 +256,7 @@
bool rtcp_reduced_size_ = false;
bool remote_estimate_ = false;
int bandwidth_ = kAutoBandwidth;
+ std::string bandwidth_type_ = kApplicationSpecificBandwidth;
std::string protocol_;
std::vector<CryptoParams> cryptos_;
std::vector<webrtc::RtpExtension> rtp_header_extensions_;
diff --git a/pc/webrtc_sdp.cc b/pc/webrtc_sdp.cc
index af58479..aa98e48 100644
--- a/pc/webrtc_sdp.cc
+++ b/pc/webrtc_sdp.cc
@@ -55,9 +55,11 @@
using cricket::CryptoParams;
using cricket::ICE_CANDIDATE_COMPONENT_RTCP;
using cricket::ICE_CANDIDATE_COMPONENT_RTP;
+using cricket::kApplicationSpecificBandwidth;
using cricket::kCodecParamMaxPTime;
using cricket::kCodecParamMinPTime;
using cricket::kCodecParamPTime;
+using cricket::kTransportSpecificBandwidth;
using cricket::MediaContentDescription;
using cricket::MediaProtocolType;
using cricket::MediaType;
@@ -224,8 +226,6 @@
// Use IPV4 per default.
static const char kDummyAddress[] = "0.0.0.0";
static const char kDummyPort[] = "9";
-// RFC 3556
-static const char kApplicationSpecificMaximum[] = "AS";
static const char kDefaultSctpmapProtocol[] = "webrtc-datachannel";
@@ -1436,10 +1436,18 @@
AddLine(os.str(), message);
// RFC 4566
- // b=AS:<bandwidth>
- if (media_desc->bandwidth() >= 1000) {
- InitLine(kLineTypeSessionBandwidth, kApplicationSpecificMaximum, &os);
- os << kSdpDelimiterColon << (media_desc->bandwidth() / 1000);
+ // b=AS:<bandwidth> or
+ // b=TIAS:<bandwidth>
+ int bandwidth = media_desc->bandwidth();
+ std::string bandwidth_type = media_desc->bandwidth_type();
+ if (bandwidth_type == kApplicationSpecificBandwidth && bandwidth >= 1000) {
+ InitLine(kLineTypeSessionBandwidth, bandwidth_type, &os);
+ bandwidth /= 1000;
+ os << kSdpDelimiterColon << bandwidth;
+ AddLine(os.str(), message);
+ } else if (bandwidth_type == kTransportSpecificBandwidth && bandwidth > 0) {
+ InitLine(kLineTypeSessionBandwidth, bandwidth_type, &os);
+ os << kSdpDelimiterColon << bandwidth;
AddLine(os.str(), message);
}
@@ -2983,46 +2991,61 @@
// 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 it shouldn't be generated for RTP data channels in
+ // the first place and provides another way to get around the limitation.
+ 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 7b83c86..2a4c36d 100644
--- a/pc/webrtc_sdp_unittest.cc
+++ b/pc/webrtc_sdp_unittest.cc
@@ -2189,16 +2189,31 @@
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(555);
+ 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:555\r\n",
+ &sdp_with_bandwidth);
+ EXPECT_EQ(sdp_with_bandwidth, message);
+}
+
+// Should default to b=AS if bandwidth_type isn't set.
+TEST_F(WebRtcSdpTest, SerializeSessionDescriptionWithMissingBandwidthType) {
+ VideoContentDescription* vcd = GetFirstVideoContentDescription(&desc_);
+ vcd->set_bandwidth(100 * 1000);
+ 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);
EXPECT_EQ(sdp_with_bandwidth, message);
}
@@ -2309,6 +2324,7 @@
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 +2628,23 @@
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;
