From b77c716d8afc3333c8d65b2738cb645ad4ed00f7 Mon Sep 17 00:00:00 2001 From: stefan Date: Mon, 6 Feb 2017 06:29:38 -0800 Subject: [PATCH] Enable send-side BWE by default for video in call tests. Also fixes a bug where RTCP transport feedback was sent even though RTCP was disabled. May affect perf numbers since the behavior of the send-side BWE differs a lot from the recv-side BWE. BUG=webrtc:7111 Review-Url: https://codereview.webrtc.org/2669413003 Cr-Commit-Position: refs/heads/master@{#16451} --- webrtc/modules/rtp_rtcp/source/rtcp_sender.cc | 6 + webrtc/test/call_test.cc | 6 +- webrtc/video/end_to_end_tests.cc | 131 ++++++++++-------- webrtc/video/video_send_stream_tests.cc | 26 +--- 4 files changed, 88 insertions(+), 81 deletions(-) diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc b/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc index 48f7595d77..ff535ea4c6 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc @@ -1049,6 +1049,12 @@ bool RTCPSender::SendFeedbackPacket(const rtcp::TransportFeedback& packet) { // but we can't because of an incorrect warning (C4822) in MVS 2013. } sender(transport_, event_log_); + { + rtc::CritScope lock(&critical_section_rtcp_sender_); + if (method_ == RtcpMode::kOff) + return false; + } + RTC_DCHECK_LE(max_packet_size_, IP_PACKET_SIZE); uint8_t buffer[IP_PACKET_SIZE]; return packet.BuildExternalBuffer(buffer, max_packet_size_, &sender) && diff --git a/webrtc/test/call_test.cc b/webrtc/test/call_test.cc index 87bbea7d8d..f42c990ae1 100644 --- a/webrtc/test/call_test.cc +++ b/webrtc/test/call_test.cc @@ -200,7 +200,8 @@ void CallTest::CreateSendConfig(size_t num_video_streams, video_send_config_.encoder_settings.payload_type = kFakeVideoSendPayloadType; video_send_config_.rtp.extensions.push_back( - RtpExtension(RtpExtension::kAbsSendTimeUri, kAbsSendTimeExtensionId)); + RtpExtension(RtpExtension::kTransportSequenceNumberUri, + kTransportSequenceNumberExtensionId)); FillEncoderConfiguration(num_video_streams, &video_encoder_config_); for (size_t i = 0; i < num_video_streams; ++i) @@ -231,7 +232,8 @@ void CallTest::CreateMatchingReceiveConfigs(Transport* rtcp_send_transport) { if (num_video_streams_ > 0) { RTC_DCHECK(!video_send_config_.rtp.ssrcs.empty()); VideoReceiveStream::Config video_config(rtcp_send_transport); - video_config.rtp.remb = true; + video_config.rtp.remb = false; + video_config.rtp.transport_cc = true; video_config.rtp.local_ssrc = kReceiverLocalVideoSsrc; for (const RtpExtension& extension : video_send_config_.rtp.extensions) video_config.rtp.extensions.push_back(extension); diff --git a/webrtc/video/end_to_end_tests.cc b/webrtc/video/end_to_end_tests.cc index 6812467cb7..966a6d3f25 100644 --- a/webrtc/video/end_to_end_tests.cc +++ b/webrtc/video/end_to_end_tests.cc @@ -1308,6 +1308,7 @@ void EndToEndTest::RespectsRtcpMode(RtcpMode rtcp_mode) { private: Action OnSendRtp(const uint8_t* packet, size_t length) override { + rtc::CritScope lock(&crit_); if (++sent_rtp_ % 3 == 0) return DROP_PACKET; @@ -1315,6 +1316,7 @@ void EndToEndTest::RespectsRtcpMode(RtcpMode rtcp_mode) { } Action OnReceiveRtcp(const uint8_t* packet, size_t length) override { + rtc::CritScope lock(&crit_); ++sent_rtcp_; test::RtcpPacketParser parser; EXPECT_TRUE(parser.Parse(packet, length)); @@ -1323,7 +1325,10 @@ void EndToEndTest::RespectsRtcpMode(RtcpMode rtcp_mode) { switch (rtcp_mode_) { case RtcpMode::kCompound: - if (parser.receiver_report()->num_packets() == 0) { + // TODO(holmer): We shouldn't send transport feedback alone if + // compound RTCP is negotiated. + if (parser.receiver_report()->num_packets() == 0 && + parser.transport_feedback()->num_packets() == 0) { ADD_FAILURE() << "Received RTCP packet without receiver report for " "RtcpMode::kCompound."; observation_complete_.Set(); @@ -1362,8 +1367,11 @@ void EndToEndTest::RespectsRtcpMode(RtcpMode rtcp_mode) { } RtcpMode rtcp_mode_; - int sent_rtp_; - int sent_rtcp_; + rtc::CriticalSection crit_; + // Must be protected since RTCP can be sent by both the process thread + // and the pacer thread. + int sent_rtp_ GUARDED_BY(&crit_); + int sent_rtcp_ GUARDED_BY(&crit_); } test(rtcp_mode); RunBaseTest(&test); @@ -1803,10 +1811,6 @@ class TransportFeedbackTester : public test::EndToEndTest { VideoSendStream::Config* send_config, std::vector* receive_configs, VideoEncoderConfig* encoder_config) override { - send_config->rtp.extensions.clear(); - send_config->rtp.extensions.push_back( - RtpExtension(RtpExtension::kTransportSequenceNumberUri, kExtensionId)); - (*receive_configs)[0].rtp.extensions = send_config->rtp.extensions; (*receive_configs)[0].rtp.transport_cc = feedback_enabled_; } @@ -1934,6 +1938,17 @@ TEST_P(EndToEndTest, ReceiveStreamSendsRemb) { public: RembObserver() : EndToEndTest(kDefaultTimeoutMs) {} + void ModifyVideoConfigs( + VideoSendStream::Config* send_config, + std::vector* receive_configs, + VideoEncoderConfig* encoder_config) override { + send_config->rtp.extensions.clear(); + send_config->rtp.extensions.push_back(RtpExtension( + RtpExtension::kAbsSendTimeUri, test::kAbsSendTimeExtensionId)); + (*receive_configs)[0].rtp.remb = true; + (*receive_configs)[0].rtp.transport_cc = false; + } + Action OnReceiveRtcp(const uint8_t* packet, size_t length) override { test::RtcpPacketParser parser; EXPECT_TRUE(parser.Parse(packet, length)); @@ -1958,46 +1973,66 @@ TEST_P(EndToEndTest, ReceiveStreamSendsRemb) { RunBaseTest(&test); } -TEST_P(EndToEndTest, VerifyBandwidthStats) { - class RtcpObserver : public test::EndToEndTest { - public: - RtcpObserver() - : EndToEndTest(kDefaultTimeoutMs), - sender_call_(nullptr), - receiver_call_(nullptr), - has_seen_pacer_delay_(false) {} +class BandwidthStatsTest : public test::EndToEndTest { + public: + explicit BandwidthStatsTest(bool send_side_bwe) + : EndToEndTest(test::CallTest::kDefaultTimeoutMs), + sender_call_(nullptr), + receiver_call_(nullptr), + has_seen_pacer_delay_(false), + send_side_bwe_(send_side_bwe) {} - Action OnSendRtp(const uint8_t* packet, size_t length) override { - Call::Stats sender_stats = sender_call_->GetStats(); - Call::Stats receiver_stats = receiver_call_->GetStats(); - if (!has_seen_pacer_delay_) - has_seen_pacer_delay_ = sender_stats.pacer_delay_ms > 0; - if (sender_stats.send_bandwidth_bps > 0 && - receiver_stats.recv_bandwidth_bps > 0 && has_seen_pacer_delay_) { + void ModifyVideoConfigs( + VideoSendStream::Config* send_config, + std::vector* receive_configs, + VideoEncoderConfig* encoder_config) override { + if (!send_side_bwe_) { + send_config->rtp.extensions.clear(); + send_config->rtp.extensions.push_back(RtpExtension( + RtpExtension::kAbsSendTimeUri, test::kAbsSendTimeExtensionId)); + (*receive_configs)[0].rtp.remb = true; + (*receive_configs)[0].rtp.transport_cc = false; + } + } + + Action OnSendRtp(const uint8_t* packet, size_t length) override { + Call::Stats sender_stats = sender_call_->GetStats(); + Call::Stats receiver_stats = receiver_call_->GetStats(); + if (!has_seen_pacer_delay_) + has_seen_pacer_delay_ = sender_stats.pacer_delay_ms > 0; + if (sender_stats.send_bandwidth_bps > 0 && has_seen_pacer_delay_) { + if (send_side_bwe_ || receiver_stats.recv_bandwidth_bps > 0) observation_complete_.Set(); - } - return SEND_PACKET; } + return SEND_PACKET; + } - void OnCallsCreated(Call* sender_call, Call* receiver_call) override { - sender_call_ = sender_call; - receiver_call_ = receiver_call; - } + void OnCallsCreated(Call* sender_call, Call* receiver_call) override { + sender_call_ = sender_call; + receiver_call_ = receiver_call; + } - void PerformTest() override { - EXPECT_TRUE(Wait()) << "Timed out while waiting for " - "non-zero bandwidth stats."; - } + void PerformTest() override { + EXPECT_TRUE(Wait()) << "Timed out while waiting for " + "non-zero bandwidth stats."; + } - private: - Call* sender_call_; - Call* receiver_call_; - bool has_seen_pacer_delay_; - } test; + private: + Call* sender_call_; + Call* receiver_call_; + bool has_seen_pacer_delay_; + const bool send_side_bwe_; +}; +TEST_P(EndToEndTest, VerifySendSideBweStats) { + BandwidthStatsTest test(true); RunBaseTest(&test); } +TEST_P(EndToEndTest, VerifyRecvSideBweStats) { + BandwidthStatsTest test(false); + RunBaseTest(&test); +} // Verifies that it's possible to limit the send BWE by sending a REMB. // This is verified by allowing the send BWE to ramp-up to >1000 kbps, @@ -2042,18 +2077,11 @@ TEST_P(EndToEndTest, RembWithSendSideBwe) { std::vector* receive_configs, VideoEncoderConfig* encoder_config) override { ASSERT_EQ(1u, send_config->rtp.ssrcs.size()); - send_config->rtp.extensions.clear(); - send_config->rtp.extensions.push_back( - RtpExtension(RtpExtension::kTransportSequenceNumberUri, - test::kTransportSequenceNumberExtensionId)); sender_ssrc_ = send_config->rtp.ssrcs[0]; encoder_config->max_bitrate_bps = 2000000; ASSERT_EQ(1u, receive_configs->size()); - (*receive_configs)[0].rtp.remb = false; - (*receive_configs)[0].rtp.transport_cc = true; - (*receive_configs)[0].rtp.extensions = send_config->rtp.extensions; RtpRtcp::Configuration config; config.receiver_only = true; config.clock = clock_; @@ -2307,11 +2335,6 @@ void EndToEndTest::VerifyHistogramStats(bool use_rtx, VideoSendStream::Config* send_config, std::vector* receive_configs, VideoEncoderConfig* encoder_config) override { - static const int kExtensionId = 8; - send_config->rtp.extensions.push_back(RtpExtension( - RtpExtension::kTransportSequenceNumberUri, kExtensionId)); - (*receive_configs)[0].rtp.extensions.push_back(RtpExtension( - RtpExtension::kTransportSequenceNumberUri, kExtensionId)); // NACK send_config->rtp.nack.rtp_history_ms = kNackRtpHistoryMs; (*receive_configs)[0].rtp.nack.rtp_history_ms = kNackRtpHistoryMs; @@ -3974,16 +3997,6 @@ TEST_P(EndToEndTest, TransportSeqNumOnAudioAndVideo) { size_t GetNumVideoStreams() const override { return 1; } size_t GetNumAudioStreams() const override { return 1; } - void ModifyVideoConfigs( - VideoSendStream::Config* send_config, - std::vector* receive_configs, - VideoEncoderConfig* encoder_config) override { - send_config->rtp.extensions.clear(); - send_config->rtp.extensions.push_back(RtpExtension( - RtpExtension::kTransportSequenceNumberUri, kExtensionId)); - (*receive_configs)[0].rtp.extensions = send_config->rtp.extensions; - } - void ModifyAudioConfigs( AudioSendStream::Config* send_config, std::vector* receive_configs) override { diff --git a/webrtc/video/video_send_stream_tests.cc b/webrtc/video/video_send_stream_tests.cc index d128399f8a..b8dd14ff1f 100644 --- a/webrtc/video/video_send_stream_tests.cc +++ b/webrtc/video/video_send_stream_tests.cc @@ -240,9 +240,6 @@ TEST_F(VideoSendStreamTest, SupportsTransportWideSequenceNumbers) { std::vector* receive_configs, VideoEncoderConfig* encoder_config) override { send_config->encoder_settings.encoder = &encoder_; - send_config->rtp.extensions.clear(); - send_config->rtp.extensions.push_back(RtpExtension( - RtpExtension::kTransportSequenceNumberUri, kExtensionId)); } void PerformTest() override { @@ -460,12 +457,12 @@ class UlpfecObserver : public test::EndToEndTest { VideoSendStreamTest::kRedPayloadType; send_config->rtp.ulpfec.ulpfec_payload_type = VideoSendStreamTest::kUlpfecPayloadType; - if (header_extensions_enabled_) { + EXPECT_FALSE(send_config->rtp.extensions.empty()); + if (!header_extensions_enabled_) { + send_config->rtp.extensions.clear(); + } else { send_config->rtp.extensions.push_back(RtpExtension( RtpExtension::kAbsSendTimeUri, test::kAbsSendTimeExtensionId)); - send_config->rtp.extensions.push_back( - RtpExtension(RtpExtension::kTransportSequenceNumberUri, - test::kTransportSequenceNumberExtensionId)); } (*receive_configs)[0].rtp.ulpfec.red_payload_type = send_config->rtp.ulpfec.red_payload_type; @@ -614,9 +611,8 @@ class FlexfecObserver : public test::EndToEndTest { RtpExtension::kAbsSendTimeUri, test::kAbsSendTimeExtensionId)); send_config->rtp.extensions.push_back(RtpExtension( RtpExtension::kTimestampOffsetUri, test::kTOffsetExtensionId)); - send_config->rtp.extensions.push_back( - RtpExtension(RtpExtension::kTransportSequenceNumberUri, - test::kTransportSequenceNumberExtensionId)); + } else { + send_config->rtp.extensions.clear(); } } @@ -1272,19 +1268,9 @@ TEST_F(VideoSendStreamTest, PaddingIsPrimarilyRetransmissions) { VideoSendStream::Config* send_config, std::vector* receive_configs, VideoEncoderConfig* encoder_config) override { - send_config->rtp.extensions.clear(); - send_config->rtp.extensions.push_back( - RtpExtension(RtpExtension::kTransportSequenceNumberUri, - test::kTransportSequenceNumberExtensionId)); // Turn on RTX. send_config->rtp.rtx.payload_type = kFakeVideoSendPayloadType; send_config->rtp.rtx.ssrcs.push_back(kVideoSendSsrcs[0]); - - (*receive_configs)[0].rtp.extensions.clear(); - (*receive_configs)[0].rtp.extensions.push_back( - RtpExtension(RtpExtension::kTransportSequenceNumberUri, - test::kTransportSequenceNumberExtensionId)); - (*receive_configs)[0].rtp.transport_cc = true; } void PerformTest() override {