Require negotiation to send transport cc feedback over RTCP.
BUG=4312
Review URL: https://codereview.webrtc.org/1452883002
Cr-Commit-Position: refs/heads/master@{#10735}
diff --git a/talk/media/base/codec.cc b/talk/media/base/codec.cc
index 5b747d1..5fbae2c 100644
--- a/talk/media/base/codec.cc
+++ b/talk/media/base/codec.cc
@@ -334,6 +334,11 @@
FeedbackParam(kRtcpFbParamRemb, kParamValueEmpty));
}
+bool HasTransportCc(const VideoCodec& codec) {
+ return codec.HasFeedbackParam(
+ FeedbackParam(kRtcpFbParamTransportCc, kParamValueEmpty));
+}
+
bool CodecNamesEq(const std::string& name1, const std::string& name2) {
return _stricmp(name1.c_str(), name2.c_str()) == 0;
}
diff --git a/talk/media/base/codec.h b/talk/media/base/codec.h
index 3bb08e7..afdf5fd 100644
--- a/talk/media/base/codec.h
+++ b/talk/media/base/codec.h
@@ -271,6 +271,7 @@
bool CodecNamesEq(const std::string& name1, const std::string& name2);
bool HasNack(const VideoCodec& codec);
bool HasRemb(const VideoCodec& codec);
+bool HasTransportCc(const VideoCodec& codec);
} // namespace cricket
diff --git a/talk/media/base/constants.cc b/talk/media/base/constants.cc
index 4063004..2361be6 100644
--- a/talk/media/base/constants.cc
+++ b/talk/media/base/constants.cc
@@ -90,6 +90,7 @@
const char kRtcpFbParamNack[] = "nack";
const char kRtcpFbNackParamPli[] = "pli";
const char kRtcpFbParamRemb[] = "goog-remb";
+const char kRtcpFbParamTransportCc[] = "transport-cc";
const char kRtcpFbParamCcm[] = "ccm";
const char kRtcpFbCcmParamFir[] = "fir";
diff --git a/talk/media/base/constants.h b/talk/media/base/constants.h
index b6a9e56..706a7bd 100644
--- a/talk/media/base/constants.h
+++ b/talk/media/base/constants.h
@@ -107,6 +107,9 @@
// rtcp-fb messages according to
// http://tools.ietf.org/html/draft-alvestrand-rmcat-remb-00
extern const char kRtcpFbParamRemb[];
+// rtcp-fb messages according to
+// https://tools.ietf.org/html/draft-holmer-rmcat-transport-wide-cc-extensions-01
+extern const char kRtcpFbParamTransportCc[];
// ccm submessages according to RFC 5104
extern const char kRtcpFbParamCcm[];
extern const char kRtcpFbCcmParamFir[];
diff --git a/talk/media/webrtc/webrtcvideoengine2.cc b/talk/media/webrtc/webrtcvideoengine2.cc
index 1612183..2d648b7 100644
--- a/talk/media/webrtc/webrtcvideoengine2.cc
+++ b/talk/media/webrtc/webrtcvideoengine2.cc
@@ -166,6 +166,8 @@
codec->AddFeedbackParam(FeedbackParam(kRtcpFbParamNack, kParamValueEmpty));
codec->AddFeedbackParam(FeedbackParam(kRtcpFbParamNack, kRtcpFbNackParamPli));
codec->AddFeedbackParam(FeedbackParam(kRtcpFbParamRemb, kParamValueEmpty));
+ codec->AddFeedbackParam(
+ FeedbackParam(kRtcpFbParamTransportCc, kParamValueEmpty));
}
static VideoCodec MakeVideoCodecWithDefaultFeedbackParams(int payload_type,
@@ -960,12 +962,15 @@
RTC_DCHECK(kv.second != nullptr);
kv.second->SetCodec(supported_codecs.front());
}
- LOG(LS_INFO) << "SetNackAndRemb on all the receive streams because the send "
- "codec has changed.";
+ LOG(LS_INFO)
+ << "SetFeedbackOptions on all the receive streams because the send "
+ "codec has changed.";
for (auto& kv : receive_streams_) {
RTC_DCHECK(kv.second != nullptr);
- kv.second->SetNackAndRemb(HasNack(supported_codecs.front().codec),
- HasRemb(supported_codecs.front().codec));
+ kv.second->SetFeedbackParameters(
+ HasNack(supported_codecs.front().codec),
+ HasRemb(supported_codecs.front().codec),
+ HasTransportCc(supported_codecs.front().codec));
}
// TODO(holmer): Changing the codec parameters shouldn't necessarily mean that
@@ -1215,6 +1220,8 @@
config.sync_group = sp.sync_label;
config.rtp.remb = send_codec_ ? HasRemb(send_codec_->codec) : false;
+ config.rtp.transport_cc =
+ send_codec_ ? HasTransportCc(send_codec_->codec) : false;
receive_streams_[ssrc] = new WebRtcVideoReceiveStream(
call_, sp, config, external_decoder_factory_, default_stream,
@@ -2465,20 +2472,28 @@
RecreateWebRtcStream();
}
-void WebRtcVideoChannel2::WebRtcVideoReceiveStream::SetNackAndRemb(
- bool nack_enabled, bool remb_enabled) {
+void WebRtcVideoChannel2::WebRtcVideoReceiveStream::SetFeedbackParameters(
+ bool nack_enabled,
+ bool remb_enabled,
+ bool transport_cc_enabled) {
int nack_history_ms = nack_enabled ? kNackHistoryMs : 0;
if (config_.rtp.nack.rtp_history_ms == nack_history_ms &&
- config_.rtp.remb == remb_enabled) {
- LOG(LS_INFO) << "Ignoring call to SetNackAndRemb because parameters are "
- "unchanged; nack=" << nack_enabled
- << ", remb=" << remb_enabled;
+ config_.rtp.remb == remb_enabled &&
+ config_.rtp.transport_cc == transport_cc_enabled) {
+ LOG(LS_INFO)
+ << "Ignoring call to SetFeedbackParameters because parameters are "
+ "unchanged; nack="
+ << nack_enabled << ", remb=" << remb_enabled
+ << ", transport_cc=" << transport_cc_enabled;
return;
}
config_.rtp.remb = remb_enabled;
config_.rtp.nack.rtp_history_ms = nack_history_ms;
- LOG(LS_INFO) << "RecreateWebRtcStream (recv) because of SetNackAndRemb; nack="
- << nack_enabled << ", remb=" << remb_enabled;
+ config_.rtp.transport_cc = transport_cc_enabled;
+ LOG(LS_INFO)
+ << "RecreateWebRtcStream (recv) because of SetFeedbackParameters; nack="
+ << nack_enabled << ", remb=" << remb_enabled
+ << ", transport_cc=" << transport_cc_enabled;
RecreateWebRtcStream();
}
diff --git a/talk/media/webrtc/webrtcvideoengine2.h b/talk/media/webrtc/webrtcvideoengine2.h
index 24033a1..85e7928 100644
--- a/talk/media/webrtc/webrtcvideoengine2.h
+++ b/talk/media/webrtc/webrtcvideoengine2.h
@@ -400,7 +400,9 @@
const std::vector<uint32_t>& GetSsrcs() const;
void SetLocalSsrc(uint32_t local_ssrc);
- void SetNackAndRemb(bool nack_enabled, bool remb_enabled);
+ void SetFeedbackParameters(bool nack_enabled,
+ bool remb_enabled,
+ bool transport_cc_enabled);
void SetRecvCodecs(const std::vector<VideoCodecSettings>& recv_codecs);
void SetRtpExtensions(const std::vector<webrtc::RtpExtension>& extensions);
diff --git a/talk/media/webrtc/webrtcvideoengine2_unittest.cc b/talk/media/webrtc/webrtcvideoengine2_unittest.cc
index 87a0ac8..832c4d0 100644
--- a/talk/media/webrtc/webrtcvideoengine2_unittest.cc
+++ b/talk/media/webrtc/webrtcvideoengine2_unittest.cc
@@ -75,6 +75,8 @@
EXPECT_TRUE(codec.HasFeedbackParam(cricket::FeedbackParam(
cricket::kRtcpFbParamRemb, cricket::kParamValueEmpty)));
EXPECT_TRUE(codec.HasFeedbackParam(cricket::FeedbackParam(
+ cricket::kRtcpFbParamTransportCc, cricket::kParamValueEmpty)));
+ EXPECT_TRUE(codec.HasFeedbackParam(cricket::FeedbackParam(
cricket::kRtcpFbParamCcm, cricket::kRtcpFbCcmParamFir)));
}
@@ -1451,6 +1453,11 @@
EXPECT_TRUE(stream->GetConfig().rtp.remb);
}
+TEST_F(WebRtcVideoChannel2Test, TransportCcIsEnabledByDefault) {
+ FakeVideoReceiveStream* stream = AddRecvStream();
+ EXPECT_TRUE(stream->GetConfig().rtp.transport_cc);
+}
+
TEST_F(WebRtcVideoChannel2Test, RembCanBeEnabledAndDisabled) {
FakeVideoReceiveStream* stream = AddRecvStream();
EXPECT_TRUE(stream->GetConfig().rtp.remb);
@@ -1471,6 +1478,27 @@
EXPECT_TRUE(stream->GetConfig().rtp.remb);
}
+TEST_F(WebRtcVideoChannel2Test, TransportCcCanBeEnabledAndDisabled) {
+ FakeVideoReceiveStream* stream = AddRecvStream();
+ EXPECT_TRUE(stream->GetConfig().rtp.transport_cc);
+
+ // Verify that transport cc feedback is turned off when send(!) codecs without
+ // transport cc feedback are set.
+ cricket::VideoSendParameters parameters;
+ parameters.codecs.push_back(kVp8Codec);
+ EXPECT_TRUE(parameters.codecs[0].feedback_params.params().empty());
+ EXPECT_TRUE(channel_->SetSendParameters(parameters));
+ stream = fake_call_->GetVideoReceiveStreams()[0];
+ EXPECT_FALSE(stream->GetConfig().rtp.transport_cc);
+
+ // Verify that transport cc feedback is turned on when setting default codecs
+ // since the default codecs have transport cc feedback enabled.
+ parameters.codecs = engine_.codecs();
+ EXPECT_TRUE(channel_->SetSendParameters(parameters));
+ stream = fake_call_->GetVideoReceiveStreams()[0];
+ EXPECT_TRUE(stream->GetConfig().rtp.transport_cc);
+}
+
TEST_F(WebRtcVideoChannel2Test, NackIsEnabledByDefault) {
VerifyCodecHasDefaultFeedbackParams(default_codec_);
diff --git a/webrtc/video/end_to_end_tests.cc b/webrtc/video/end_to_end_tests.cc
index e2450d9..e541d37 100644
--- a/webrtc/video/end_to_end_tests.cc
+++ b/webrtc/video/end_to_end_tests.cc
@@ -1533,9 +1533,8 @@
tester.RunTest();
}
-TEST_F(EndToEndTest, ReceivesTransportFeedback) {
+void TransportFeedbackTest(bool feedback_enabled) {
static const int kExtensionId = 5;
-
class TransportFeedbackObserver : public test::DirectTransport {
public:
TransportFeedbackObserver(Call* receiver_call, rtc::Event* done_event)
@@ -1563,12 +1562,16 @@
class TransportFeedbackTester : public MultiStreamTest {
public:
- TransportFeedbackTester() : done_(false, false) {}
+ explicit TransportFeedbackTester(bool feedback_enabled)
+ : feedback_enabled_(feedback_enabled), done_(false, false) {}
virtual ~TransportFeedbackTester() {}
protected:
void Wait() override {
- EXPECT_TRUE(done_.Wait(CallTest::kDefaultTimeoutMs));
+ const int64_t kDisabledFeedbackTimeoutMs = 5000;
+ EXPECT_EQ(feedback_enabled_, done_.Wait(feedback_enabled_
+ ? test::CallTest::kDefaultTimeoutMs
+ : kDisabledFeedbackTimeoutMs));
}
void UpdateSendConfig(
@@ -1585,6 +1588,7 @@
VideoReceiveStream::Config* receive_config) override {
receive_config->rtp.extensions.push_back(
RtpExtension(RtpExtension::kTransportSequenceNumber, kExtensionId));
+ receive_config->rtp.transport_cc = feedback_enabled_;
}
test::DirectTransport* CreateReceiveTransport(
@@ -1593,10 +1597,20 @@
}
private:
+ const bool feedback_enabled_;
rtc::Event done_;
- } tester;
+ } tester(feedback_enabled);
tester.RunTest();
}
+
+TEST_F(EndToEndTest, ReceivesTransportFeedback) {
+ TransportFeedbackTest(true);
+}
+
+TEST_F(EndToEndTest, TransportFeedbackNotConfigured) {
+ TransportFeedbackTest(false);
+}
+
TEST_F(EndToEndTest, ObserversEncodedFrames) {
class EncodedFrameTestObserver : public EncodedFrameObserver {
public:
diff --git a/webrtc/video/rampup_tests.cc b/webrtc/video/rampup_tests.cc
index 6450e02..c9b8e11 100644
--- a/webrtc/video/rampup_tests.cc
+++ b/webrtc/video/rampup_tests.cc
@@ -124,16 +124,20 @@
send_config->rtp.extensions.clear();
bool remb;
+ bool transport_cc;
if (extension_type_ == RtpExtension::kAbsSendTime) {
remb = true;
+ transport_cc = false;
send_config->rtp.extensions.push_back(
RtpExtension(extension_type_.c_str(), kAbsSendTimeExtensionId));
} else if (extension_type_ == RtpExtension::kTransportSequenceNumber) {
remb = false;
+ transport_cc = true;
send_config->rtp.extensions.push_back(RtpExtension(
extension_type_.c_str(), kTransportSequenceNumberExtensionId));
} else {
remb = true;
+ transport_cc = false;
send_config->rtp.extensions.push_back(RtpExtension(
extension_type_.c_str(), kTransmissionTimeOffsetExtensionId));
}
@@ -153,6 +157,7 @@
size_t i = 0;
for (VideoReceiveStream::Config& recv_config : *receive_configs) {
recv_config.rtp.remb = remb;
+ recv_config.rtp.transport_cc = transport_cc;
recv_config.rtp.extensions = send_config->rtp.extensions;
recv_config.rtp.remote_ssrc = ssrcs_[i];
diff --git a/webrtc/video/video_quality_test.cc b/webrtc/video/video_quality_test.cc
index ebceb56..a46fe1b 100644
--- a/webrtc/video/video_quality_test.cc
+++ b/webrtc/video/video_quality_test.cc
@@ -828,6 +828,7 @@
receive_configs_[i].rtp.rtx[kSendRtxPayloadType].ssrc = kSendRtxSsrcs[i];
receive_configs_[i].rtp.rtx[kSendRtxPayloadType].payload_type =
kSendRtxPayloadType;
+ receive_configs_[i].rtp.transport_cc = params_.common.send_side_bwe;
}
}
diff --git a/webrtc/video/video_receive_stream.cc b/webrtc/video/video_receive_stream.cc
index 287f535..683e89a 100644
--- a/webrtc/video/video_receive_stream.cc
+++ b/webrtc/video/video_receive_stream.cc
@@ -81,6 +81,7 @@
<< (rtcp_xr.receiver_reference_time_report ? "on" : "off");
ss << '}';
ss << ", remb: " << (remb ? "on" : "off");
+ ss << ", transport_cc: " << (transport_cc ? "on" : "off");
ss << ", nack: {rtp_history_ms: " << nack.rtp_history_ms << '}';
ss << ", fec: " << fec.ToString();
ss << ", rtx: {";
@@ -153,7 +154,8 @@
call_stats_(call_stats) {
LOG(LS_INFO) << "VideoReceiveStream: " << config_.ToString();
- bool send_side_bwe = UseSendSideBwe(config_.rtp.extensions);
+ bool send_side_bwe =
+ config.rtp.transport_cc && UseSendSideBwe(config_.rtp.extensions);
RemoteBitrateEstimator* bitrate_estimator =
congestion_controller_->GetRemoteBitrateEstimator(send_side_bwe);
diff --git a/webrtc/video_receive_stream.h b/webrtc/video_receive_stream.h
index 275162c..6be2c5a 100644
--- a/webrtc/video_receive_stream.h
+++ b/webrtc/video_receive_stream.h
@@ -113,6 +113,9 @@
// See draft-alvestrand-rmcat-remb for information.
bool remb = false;
+ // See draft-holmer-rmcat-transport-wide-cc-extensions for details.
+ bool transport_cc = false;
+
// See NackConfig for description.
NackConfig nack;
