From d7da120b40f7a8a8357f23cf6b49aa03f67c1cf6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20Bostr=C3=B6m?= Date: Fri, 5 Jun 2015 14:09:38 +0200 Subject: [PATCH] Disable reduced-size RTCP in default config. Verifies that reduced-size isn't configured in WebRtcVideoEngine2 without explicit configuration (which doesn't exist). Also disables REMB in the default config because it requires reconfiguration. Adds default-config tests to make sure that they don't contain parameters that need to be negotiated between clients. BUG=chromium:497103, webrtc:4745 R=mflodman@webrtc.org, stefan@webrtc.org Review URL: https://codereview.webrtc.org/1171533002 Cr-Commit-Position: refs/heads/master@{#9384} --- .../webrtc/webrtcvideoengine2_unittest.cc | 5 ++ webrtc/test/call_test.cc | 1 + webrtc/video/bitrate_estimator_tests.cc | 1 + webrtc/video/end_to_end_tests.cc | 46 +++++++++++++++++++ webrtc/video/loopback.cc | 1 + webrtc/video_receive_stream.h | 4 +- 6 files changed, 56 insertions(+), 2 deletions(-) diff --git a/talk/media/webrtc/webrtcvideoengine2_unittest.cc b/talk/media/webrtc/webrtcvideoengine2_unittest.cc index 8ec7980a0..d83e8c4b9 100644 --- a/talk/media/webrtc/webrtcvideoengine2_unittest.cc +++ b/talk/media/webrtc/webrtcvideoengine2_unittest.cc @@ -1267,6 +1267,11 @@ TEST_F(WebRtcVideoChannel2Test, AddRecvStreamOnlyUsesOneReceiveStream) { EXPECT_EQ(1u, fake_call_->GetVideoReceiveStreams().size()); } +TEST_F(WebRtcVideoChannel2Test, RtcpIsCompoundByDefault) { + FakeVideoReceiveStream* stream = AddRecvStream(); + EXPECT_EQ(webrtc::newapi::kRtcpCompound, stream->GetConfig().rtp.rtcp_mode); +} + TEST_F(WebRtcVideoChannel2Test, RembIsEnabledByDefault) { FakeVideoReceiveStream* stream = AddRecvStream(); EXPECT_TRUE(stream->GetConfig().rtp.remb); diff --git a/webrtc/test/call_test.cc b/webrtc/test/call_test.cc index e9129824c..2548659ef 100644 --- a/webrtc/test/call_test.cc +++ b/webrtc/test/call_test.cc @@ -108,6 +108,7 @@ void CallTest::CreateMatchingReceiveConfigs() { assert(receive_configs_.empty()); assert(allocated_decoders_.empty()); VideoReceiveStream::Config config; + config.rtp.remb = true; config.rtp.local_ssrc = kReceiverLocalSsrc; for (const RtpExtension& extension : send_config_.rtp.extensions) config.rtp.extensions.push_back(extension); diff --git a/webrtc/video/bitrate_estimator_tests.cc b/webrtc/video/bitrate_estimator_tests.cc index 9209560ef..6e71ca24e 100644 --- a/webrtc/video/bitrate_estimator_tests.cc +++ b/webrtc/video/bitrate_estimator_tests.cc @@ -151,6 +151,7 @@ class BitrateEstimatorTest : public test::CallTest { // receive_config_.decoders will be set by every stream separately. receive_config_.rtp.remote_ssrc = send_config_.rtp.ssrcs[0]; receive_config_.rtp.local_ssrc = kReceiverLocalSsrc; + receive_config_.rtp.remb = true; receive_config_.rtp.extensions.push_back( RtpExtension(RtpExtension::kTOffset, kTOFExtensionId)); receive_config_.rtp.extensions.push_back( diff --git a/webrtc/video/end_to_end_tests.cc b/webrtc/video/end_to_end_tests.cc index 28504c4fc..ce5a0e5f8 100644 --- a/webrtc/video/end_to_end_tests.cc +++ b/webrtc/video/end_to_end_tests.cc @@ -2789,4 +2789,50 @@ TEST_F(EndToEndTest, CanCreateAndDestroyManyVideoStreams) { call->DestroyVideoReceiveStream(receive_stream); } } + +void VerifyEmptyNackConfig(const NackConfig& config) { + EXPECT_EQ(0, config.rtp_history_ms) + << "Enabling NACK requires rtcp-fb: nack negotiation."; +} + +void VerifyEmptyFecConfig(const FecConfig& config) { + EXPECT_EQ(-1, config.ulpfec_payload_type) + << "Enabling FEC requires rtpmap: ulpfec negotiation."; + EXPECT_EQ(-1, config.red_payload_type) + << "Enabling FEC requires rtpmap: red negotiation."; + EXPECT_EQ(-1, config.red_rtx_payload_type) + << "Enabling RTX in FEC requires rtpmap: rtx negotiation."; +} + +TEST_F(EndToEndTest, VerifyDefaultSendConfigParameters) { + VideoSendStream::Config default_send_config; + EXPECT_EQ(0, default_send_config.rtp.nack.rtp_history_ms) + << "Enabling NACK require rtcp-fb: nack negotiation."; + EXPECT_TRUE(default_send_config.rtp.rtx.ssrcs.empty()) + << "Enabling RTX requires rtpmap: rtx negotiation."; + EXPECT_TRUE(default_send_config.rtp.extensions.empty()) + << "Enabling RTP extensions require negotiation."; + + VerifyEmptyNackConfig(default_send_config.rtp.nack); + VerifyEmptyFecConfig(default_send_config.rtp.fec); +} + +TEST_F(EndToEndTest, VerifyDefaultReceiveConfigParameters) { + VideoReceiveStream::Config default_receive_config; + EXPECT_EQ(newapi::kRtcpCompound, default_receive_config.rtp.rtcp_mode) + << "Reduced-size RTCP require rtcp-rsize to be negotiated."; + EXPECT_FALSE(default_receive_config.rtp.remb) + << "REMB require rtcp-fb: goog-remb to be negotiated."; + EXPECT_FALSE( + default_receive_config.rtp.rtcp_xr.receiver_reference_time_report) + << "RTCP XR settings require rtcp-xr to be negotiated."; + EXPECT_TRUE(default_receive_config.rtp.rtx.empty()) + << "Enabling RTX requires rtpmap: rtx negotiation."; + EXPECT_TRUE(default_receive_config.rtp.extensions.empty()) + << "Enabling RTP extensions require negotiation."; + + VerifyEmptyNackConfig(default_receive_config.rtp.nack); + VerifyEmptyFecConfig(default_receive_config.rtp.fec); +} + } // namespace webrtc diff --git a/webrtc/video/loopback.cc b/webrtc/video/loopback.cc index 881fa49a1..7e5ca078c 100644 --- a/webrtc/video/loopback.cc +++ b/webrtc/video/loopback.cc @@ -115,6 +115,7 @@ void Loopback::Run() { receive_config.rtp.remote_ssrc = send_config.rtp.ssrcs[0]; receive_config.rtp.local_ssrc = kReceiverLocalSsrc; receive_config.rtp.nack.rtp_history_ms = 1000; + receive_config.rtp.remb = true; receive_config.rtp.rtx[kVideoPayloadType].ssrc = kSendRtxSsrc; receive_config.rtp.rtx[kVideoPayloadType].payload_type = kRtxVideoPayloadType; receive_config.rtp.extensions.push_back( diff --git a/webrtc/video_receive_stream.h b/webrtc/video_receive_stream.h index b523b1ec0..5e5ece5a2 100644 --- a/webrtc/video_receive_stream.h +++ b/webrtc/video_receive_stream.h @@ -107,8 +107,8 @@ class VideoReceiveStream { Rtp() : remote_ssrc(0), local_ssrc(0), - rtcp_mode(newapi::kRtcpReducedSize), - remb(true) {} + rtcp_mode(newapi::kRtcpCompound), + remb(false) {} std::string ToString() const; // Synchronization source (stream identifier) to be received.