diff --git a/tools/valgrind-webrtc/gtest_exclude/video_engine_tests.gtest-memcheck.txt b/tools/valgrind-webrtc/gtest_exclude/video_engine_tests.gtest-memcheck.txt index 0fb70c8263..c6bc5a96cd 100644 --- a/tools/valgrind-webrtc/gtest_exclude/video_engine_tests.gtest-memcheck.txt +++ b/tools/valgrind-webrtc/gtest_exclude/video_engine_tests.gtest-memcheck.txt @@ -2,8 +2,10 @@ EndToEndTest.CanSwitchToUseAllSsrcs EndToEndTest.SendsAndReceivesVP9 VideoSendStreamTest.VP9FlexMode +# Times out due to using a real VP8 encoder. +EndToEndTest.VerifyHistogramStatsWithRed -# Flaky under memcheck (WebRTC issue 5134) +# https://bugs.chromium.org/p/webrtc/issues/detail?id=5134 EndToEndTest.AssignsTransportSequenceNumbers # https://bugs.chromium.org/p/webrtc/issues/detail?id=5312 RtcEventLogTest.DropOldEvents diff --git a/webrtc/video/end_to_end_tests.cc b/webrtc/video/end_to_end_tests.cc index 28d371a5af..0deb4a59fd 100644 --- a/webrtc/video/end_to_end_tests.cc +++ b/webrtc/video/end_to_end_tests.cc @@ -76,7 +76,7 @@ class EndToEndTest : public test::CallTest { } }; - void DecodesRetransmittedFrame(bool use_rtx, bool use_red); + void DecodesRetransmittedFrame(bool enable_rtx, bool enable_red); void ReceivesPliAndRecovers(int rtp_history_ms); void RespectsRtcpMode(RtcpMode rtcp_mode); void TestXrReceiverReferenceTimeReport(bool enable_rrtr); @@ -701,18 +701,20 @@ TEST_F(EndToEndTest, DISABLED_ReceivedFecPacketsNotNacked) { // This test drops second RTP packet with a marker bit set, makes sure it's // retransmitted and renders. Retransmission SSRCs are also checked. -void EndToEndTest::DecodesRetransmittedFrame(bool use_rtx, bool use_red) { +void EndToEndTest::DecodesRetransmittedFrame(bool enable_rtx, bool enable_red) { // Must be set high enough to allow the bitrate probing to finish. static const int kMinProbePackets = 30; static const int kDroppedFrameNumber = kMinProbePackets + 1; class RetransmissionObserver : public test::EndToEndTest, public I420FrameCallback { public: - explicit RetransmissionObserver(bool use_rtx, bool use_red) + RetransmissionObserver(bool enable_rtx, bool enable_red) : EndToEndTest(kDefaultTimeoutMs), - payload_type_(GetPayloadType(false, use_red)), - retransmission_ssrc_(use_rtx ? kSendRtxSsrcs[0] : kVideoSendSsrcs[0]), - retransmission_payload_type_(GetPayloadType(use_rtx, use_red)), + payload_type_(GetPayloadType(false, enable_red)), + retransmission_ssrc_(enable_rtx ? kSendRtxSsrcs[0] + : kVideoSendSsrcs[0]), + retransmission_payload_type_(GetPayloadType(enable_rtx, enable_red)), + encoder_(VideoEncoder::Create(VideoEncoder::EncoderType::kVp8)), marker_bits_observed_(0), num_packets_observed_(0), retransmitted_timestamp_(0), @@ -790,6 +792,12 @@ void EndToEndTest::DecodesRetransmittedFrame(bool use_rtx, bool use_red) { (*receive_configs)[0].rtp.rtx[payload_type_].payload_type = kSendRtxPayloadType; } + // Configure encoding and decoding with VP8, since generic packetization + // doesn't support FEC with NACK. + RTC_DCHECK_EQ(1u, (*receive_configs)[0].decoders.size()); + send_config->encoder_settings.encoder = encoder_.get(); + send_config->encoder_settings.payload_name = "VP8"; + (*receive_configs)[0].decoders[0].payload_name = "VP8"; } void PerformTest() override { @@ -812,11 +820,13 @@ void EndToEndTest::DecodesRetransmittedFrame(bool use_rtx, bool use_red) { const int payload_type_; const uint32_t retransmission_ssrc_; const int retransmission_payload_type_; + rtc::scoped_ptr encoder_; + const std::string payload_name_; int marker_bits_observed_; int num_packets_observed_; uint32_t retransmitted_timestamp_ GUARDED_BY(&crit_); bool frame_retransmitted_; - } test(use_rtx, use_red); + } test(enable_rtx, enable_red); RunBaseTest(&test); } @@ -2088,6 +2098,10 @@ void EndToEndTest::VerifyHistogramStats(bool use_rtx, use_rtx_(use_rtx), use_red_(use_red), screenshare_(screenshare), + // This test uses NACK, so to send FEC we can't use a fake encoder. + vp8_encoder_( + use_red ? VideoEncoder::Create(VideoEncoder::EncoderType::kVp8) + : nullptr), sender_call_(nullptr), receiver_call_(nullptr), start_runtime_ms_(-1) {} @@ -2127,6 +2141,9 @@ void EndToEndTest::VerifyHistogramStats(bool use_rtx, if (use_red_) { send_config->rtp.fec.ulpfec_payload_type = kUlpfecPayloadType; send_config->rtp.fec.red_payload_type = kRedPayloadType; + send_config->encoder_settings.encoder = vp8_encoder_.get(); + send_config->encoder_settings.payload_name = "VP8"; + (*receive_configs)[0].decoders[0].payload_name = "VP8"; (*receive_configs)[0].rtp.fec.red_payload_type = kRedPayloadType; (*receive_configs)[0].rtp.fec.ulpfec_payload_type = kUlpfecPayloadType; } @@ -2156,6 +2173,7 @@ void EndToEndTest::VerifyHistogramStats(bool use_rtx, const bool use_rtx_; const bool use_red_; const bool screenshare_; + const rtc::scoped_ptr vp8_encoder_; Call* sender_call_; Call* receiver_call_; int64_t start_runtime_ms_; diff --git a/webrtc/video/video_send_stream.cc b/webrtc/video/video_send_stream.cc index 9ce1a84273..fafb0223ae 100644 --- a/webrtc/video/video_send_stream.cc +++ b/webrtc/video/video_send_stream.cc @@ -107,6 +107,35 @@ std::string VideoSendStream::Config::ToString() const { namespace { +VideoCodecType PayloadNameToCodecType(const std::string& payload_name) { + if (payload_name == "VP8") + return kVideoCodecVP8; + if (payload_name == "VP9") + return kVideoCodecVP9; + if (payload_name == "H264") + return kVideoCodecH264; + return kVideoCodecGeneric; +} + +bool PayloadTypeSupportsSkippingFecPackets(const std::string& payload_name) { + switch (PayloadNameToCodecType(payload_name)) { + case kVideoCodecVP8: + case kVideoCodecVP9: + return true; + case kVideoCodecH264: + case kVideoCodecGeneric: + return false; + case kVideoCodecI420: + case kVideoCodecRED: + case kVideoCodecULPFEC: + case kVideoCodecUnknown: + RTC_NOTREACHED(); + return false; + } + RTC_NOTREACHED(); + return false; +} + CpuOveruseOptions GetCpuOveruseOptions(bool full_overuse_time) { CpuOveruseOptions options; if (full_overuse_time) { @@ -215,7 +244,19 @@ VideoSendStream::VideoSendStream( // Enable NACK, FEC or both. const bool enable_protection_nack = config_.rtp.nack.rtp_history_ms > 0; - const bool enable_protection_fec = config_.rtp.fec.red_payload_type != -1; + bool enable_protection_fec = config_.rtp.fec.red_payload_type != -1; + // Payload types without picture ID cannot determine that a stream is complete + // without retransmitting FEC, so using FEC + NACK for H.264 (for instance) is + // a waste of bandwidth since FEC packets still have to be transmitted. Note + // that this is not the case with FLEXFEC. + if (enable_protection_nack && + !PayloadTypeSupportsSkippingFecPackets( + config_.encoder_settings.payload_name)) { + LOG(LS_WARNING) << "Transmitting payload type without picture ID using" + "NACK+FEC is a waste of bandwidth since FEC packets " + "also have to be retransmitted. Disabling FEC."; + enable_protection_fec = false; + } // TODO(changbin): Should set RTX for RED mapping in RTP sender in future. vie_channel_.SetProtectionMode(enable_protection_nack, enable_protection_fec, config_.rtp.fec.red_payload_type, @@ -320,15 +361,8 @@ bool VideoSendStream::ReconfigureVideoEncoder( VideoCodec video_codec; memset(&video_codec, 0, sizeof(video_codec)); - if (config_.encoder_settings.payload_name == "VP8") { - video_codec.codecType = kVideoCodecVP8; - } else if (config_.encoder_settings.payload_name == "VP9") { - video_codec.codecType = kVideoCodecVP9; - } else if (config_.encoder_settings.payload_name == "H264") { - video_codec.codecType = kVideoCodecH264; - } else { - video_codec.codecType = kVideoCodecGeneric; - } + video_codec.codecType = + PayloadNameToCodecType(config_.encoder_settings.payload_name); switch (config.content_type) { case VideoEncoderConfig::ContentType::kRealtimeVideo: diff --git a/webrtc/video/video_send_stream_tests.cc b/webrtc/video/video_send_stream_tests.cc index 00aa2a9018..01267735e0 100644 --- a/webrtc/video/video_send_stream_tests.cc +++ b/webrtc/video/video_send_stream_tests.cc @@ -308,48 +308,55 @@ class FakeReceiveStatistics : public NullReceiveStatistics { StatisticianMap stats_map_; }; -class FecObserver : public test::SendTest { +class FecObserver : public test::EndToEndTest { public: - explicit FecObserver(bool header_extensions_enabled) - : SendTest(VideoSendStreamTest::kDefaultTimeoutMs), + FecObserver(bool header_extensions_enabled, + bool use_nack, + bool expect_red, + const std::string& codec) + : EndToEndTest(VideoSendStreamTest::kDefaultTimeoutMs), + payload_name_(codec), + use_nack_(use_nack), + expect_red_(expect_red), send_count_(0), received_media_(false), received_fec_(false), - header_extensions_enabled_(header_extensions_enabled) {} + header_extensions_enabled_(header_extensions_enabled) { + if (codec == "H264") { + encoder_.reset(new test::FakeH264Encoder(Clock::GetRealTimeClock())); + } else if (codec == "VP8") { + encoder_.reset(VideoEncoder::Create(VideoEncoder::EncoderType::kVp8)); + } else if (codec == "VP9") { + encoder_.reset(VideoEncoder::Create(VideoEncoder::EncoderType::kVp9)); + } else { + RTC_NOTREACHED(); + } + } private: Action OnSendRtp(const uint8_t* packet, size_t length) override { RTPHeader header; EXPECT_TRUE(parser_->Parse(packet, length, &header)); - // Send lossy receive reports to trigger FEC enabling. - if (send_count_++ % 2 != 0) { - // Receive statistics reporting having lost 50% of the packets. - FakeReceiveStatistics lossy_receive_stats( - VideoSendStreamTest::kVideoSendSsrcs[0], header.sequenceNumber, - send_count_ / 2, 127); - RTCPSender rtcp_sender(false, Clock::GetRealTimeClock(), - &lossy_receive_stats, nullptr, nullptr, - transport_adapter_.get()); - - rtcp_sender.SetRTCPStatus(RtcpMode::kReducedSize); - rtcp_sender.SetRemoteSSRC(VideoSendStreamTest::kVideoSendSsrcs[0]); - - RTCPSender::FeedbackState feedback_state; - - EXPECT_EQ(0, rtcp_sender.SendRTCP(feedback_state, kRtcpRr)); - } - + ++send_count_; int encapsulated_payload_type = -1; if (header.payloadType == VideoSendStreamTest::kRedPayloadType) { + EXPECT_TRUE(expect_red_); encapsulated_payload_type = static_cast(packet[header.headerLength]); if (encapsulated_payload_type != - VideoSendStreamTest::kFakeVideoSendPayloadType) + VideoSendStreamTest::kFakeVideoSendPayloadType) { EXPECT_EQ(VideoSendStreamTest::kUlpfecPayloadType, encapsulated_payload_type); + } } else { EXPECT_EQ(VideoSendStreamTest::kFakeVideoSendPayloadType, header.payloadType); + if (static_cast(header.headerLength + header.paddingLength) < + length) { + // Not padding-only, media received outside of RED. + EXPECT_FALSE(expect_red_); + received_media_ = true; + } } if (header_extensions_enabled_) { @@ -379,14 +386,27 @@ class FecObserver : public test::SendTest { } } - if (received_media_ && received_fec_ && send_count_ > 100) - observation_complete_.Set(); + if (send_count_ > 100 && received_media_) { + if (received_fec_ || !expect_red_) + observation_complete_.Set(); + } prev_header_ = header; return SEND_PACKET; } + test::PacketTransport* CreateSendTransport(Call* sender_call) override { + // At low RTT (< kLowRttNackMs) -> NACK only, no FEC. + // Configure some network delay. + const int kNetworkDelayMs = 100; + FakeNetworkPipe::Config config; + config.loss_percent = 50; + config.queue_delay_ms = kNetworkDelayMs; + return new test::PacketTransport(sender_call, this, + test::PacketTransport::kSender, config); + } + void ModifyVideoConfigs( VideoSendStream::Config* send_config, std::vector* receive_configs, @@ -394,10 +414,17 @@ class FecObserver : public test::SendTest { transport_adapter_.reset( new internal::TransportAdapter(send_config->send_transport)); transport_adapter_->Enable(); + if (use_nack_) { + send_config->rtp.nack.rtp_history_ms = + (*receive_configs)[0].rtp.nack.rtp_history_ms = + VideoSendStreamTest::kNackRtpHistoryMs; + } + send_config->encoder_settings.encoder = encoder_.get(); + send_config->encoder_settings.payload_name = payload_name_; send_config->rtp.fec.red_payload_type = - VideoSendStreamTest::kRedPayloadType; + VideoSendStreamTest::kRedPayloadType; send_config->rtp.fec.ulpfec_payload_type = - VideoSendStreamTest::kUlpfecPayloadType; + VideoSendStreamTest::kUlpfecPayloadType; if (header_extensions_enabled_) { send_config->rtp.extensions.push_back(RtpExtension( RtpExtension::kAbsSendTime, test::kAbsSendTimeExtensionId)); @@ -405,6 +432,10 @@ class FecObserver : public test::SendTest { RtpExtension(RtpExtension::kTransportSequenceNumber, test::kTransportSequenceNumberExtensionId)); } + (*receive_configs)[0].rtp.fec.red_payload_type = + send_config->rtp.fec.red_payload_type; + (*receive_configs)[0].rtp.fec.ulpfec_payload_type = + send_config->rtp.fec.ulpfec_payload_type; } void PerformTest() override { @@ -412,6 +443,10 @@ class FecObserver : public test::SendTest { } rtc::scoped_ptr transport_adapter_; + rtc::scoped_ptr encoder_; + const std::string payload_name_; + const bool use_nack_; + const bool expect_red_; int send_count_; bool received_media_; bool received_fec_; @@ -420,14 +455,37 @@ class FecObserver : public test::SendTest { }; TEST_F(VideoSendStreamTest, SupportsFecWithExtensions) { - FecObserver test(true); - + FecObserver test(true, false, true, "VP8"); RunBaseTest(&test); } TEST_F(VideoSendStreamTest, SupportsFecWithoutExtensions) { - FecObserver test(false); + FecObserver test(false, false, true, "VP8"); + RunBaseTest(&test); +} +// The FEC scheme used is not efficient for H264, so we should not use RED/FEC +// since we'll still have to re-request FEC packets, effectively wasting +// bandwidth since the receiver has to wait for FEC retransmissions to determine +// that the received state is actually decodable. +TEST_F(VideoSendStreamTest, DoesNotUtilizeRedForH264WithNackEnabled) { + FecObserver test(false, true, false, "H264"); + RunBaseTest(&test); +} + +// Without retransmissions FEC for H264 is fine. +TEST_F(VideoSendStreamTest, DoesUtilizeRedForH264WithoutNackEnabled) { + FecObserver test(false, false, true, "H264"); + RunBaseTest(&test); +} + +TEST_F(VideoSendStreamTest, DoesUtilizeRedForVp8WithNackEnabled) { + FecObserver test(false, true, true, "VP8"); + RunBaseTest(&test); +} + +TEST_F(VideoSendStreamTest, DoesUtilizeRedForVp9WithNackEnabled) { + FecObserver test(false, true, true, "VP9"); RunBaseTest(&test); } diff --git a/webrtc/video/vie_channel.cc b/webrtc/video/vie_channel.cc index d49f505365..b41e21b380 100644 --- a/webrtc/video/vie_channel.cc +++ b/webrtc/video/vie_channel.cc @@ -414,13 +414,15 @@ void ViEChannel::SetProtectionMode(bool enable_nack, bool enable_fec, int payload_type_red, int payload_type_fec) { - // Validate payload types. - if (enable_fec) { + // Validate payload types. If either RED or FEC payload types are set then + // both should be. If FEC is enabled then they both have to be set. + if (enable_fec || payload_type_red != -1 || payload_type_fec != -1) { RTC_DCHECK_GE(payload_type_red, 0); RTC_DCHECK_GE(payload_type_fec, 0); RTC_DCHECK_LE(payload_type_red, 127); RTC_DCHECK_LE(payload_type_fec, 127); } else { + // Payload types unset. RTC_DCHECK_EQ(payload_type_red, -1); RTC_DCHECK_EQ(payload_type_fec, -1); // Set to valid uint8_ts to be castable later without signed overflows.