From 29ffdc1a15e31bd81e806ff135c2100d811714f0 Mon Sep 17 00:00:00 2001 From: deadbeef Date: Fri, 12 Feb 2016 12:00:42 -0800 Subject: [PATCH] Revert of Don't send FEC for H.264 with NACK enabled. (patchset #5 id:80001 of https://codereview.webrtc.org/1687303002/ ) Reason for revert: Broke the VerifyHistogramStatsWithRed test on the Windows DrMemory Full bot and Linux Memcheck bot. Please fix the test and reland. Original issue's description: > Don't send FEC for H.264 with NACK enabled. > > The H.264 does not contain picture IDs and are not sufficient to > determine that a packet may be skipped. This causes retransmission > requests for FEC that are currently dropped by the sender (since they > should be redundant). > > The receiver is then unable to continue without having the packet gap > filled (unlike VP8/VP9 which moves on since it has a consecutive stream > of picture IDs). > > Even if FEC retransmission did work it's a huge waste of bandwidth, > since it just adds additional overhead that has to be unconditionally > transmitted. This bandwidth is better used to send higher-quality > frames. > > BUG=webrtc:5264 > R=stefan@webrtc.org > > Committed: https://crrev.com/25558ad819b4df41ba51537e26a77480ace1e525 > Cr-Commit-Position: refs/heads/master@{#11601} TBR=stefan@webrtc.org,pbos@webrtc.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=webrtc:5264 Review URL: https://codereview.webrtc.org/1692783005 Cr-Commit-Position: refs/heads/master@{#11607} --- webrtc/video/end_to_end_tests.cc | 32 ++----- webrtc/video/video_send_stream.cc | 54 ++--------- webrtc/video/video_send_stream_tests.cc | 118 ++++++------------------ webrtc/video/vie_channel.cc | 6 +- 4 files changed, 49 insertions(+), 161 deletions(-) diff --git a/webrtc/video/end_to_end_tests.cc b/webrtc/video/end_to_end_tests.cc index 0deb4a59fd..28d371a5af 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 enable_rtx, bool enable_red); + void DecodesRetransmittedFrame(bool use_rtx, bool use_red); void ReceivesPliAndRecovers(int rtp_history_ms); void RespectsRtcpMode(RtcpMode rtcp_mode); void TestXrReceiverReferenceTimeReport(bool enable_rrtr); @@ -701,20 +701,18 @@ 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 enable_rtx, bool enable_red) { +void EndToEndTest::DecodesRetransmittedFrame(bool use_rtx, bool use_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: - RetransmissionObserver(bool enable_rtx, bool enable_red) + explicit RetransmissionObserver(bool use_rtx, bool use_red) : EndToEndTest(kDefaultTimeoutMs), - 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)), + payload_type_(GetPayloadType(false, use_red)), + retransmission_ssrc_(use_rtx ? kSendRtxSsrcs[0] : kVideoSendSsrcs[0]), + retransmission_payload_type_(GetPayloadType(use_rtx, use_red)), marker_bits_observed_(0), num_packets_observed_(0), retransmitted_timestamp_(0), @@ -792,12 +790,6 @@ void EndToEndTest::DecodesRetransmittedFrame(bool enable_rtx, bool enable_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 { @@ -820,13 +812,11 @@ void EndToEndTest::DecodesRetransmittedFrame(bool enable_rtx, bool enable_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(enable_rtx, enable_red); + } test(use_rtx, use_red); RunBaseTest(&test); } @@ -2098,10 +2088,6 @@ 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) {} @@ -2141,9 +2127,6 @@ 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; } @@ -2173,7 +2156,6 @@ 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 fafb0223ae..9ce1a84273 100644 --- a/webrtc/video/video_send_stream.cc +++ b/webrtc/video/video_send_stream.cc @@ -107,35 +107,6 @@ 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) { @@ -244,19 +215,7 @@ VideoSendStream::VideoSendStream( // Enable NACK, FEC or both. const bool enable_protection_nack = config_.rtp.nack.rtp_history_ms > 0; - 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; - } + const bool enable_protection_fec = config_.rtp.fec.red_payload_type != -1; // 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, @@ -361,8 +320,15 @@ bool VideoSendStream::ReconfigureVideoEncoder( VideoCodec video_codec; memset(&video_codec, 0, sizeof(video_codec)); - video_codec.codecType = - PayloadNameToCodecType(config_.encoder_settings.payload_name); + 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; + } 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 01267735e0..00aa2a9018 100644 --- a/webrtc/video/video_send_stream_tests.cc +++ b/webrtc/video/video_send_stream_tests.cc @@ -308,55 +308,48 @@ class FakeReceiveStatistics : public NullReceiveStatistics { StatisticianMap stats_map_; }; -class FecObserver : public test::EndToEndTest { +class FecObserver : public test::SendTest { public: - 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), + explicit FecObserver(bool header_extensions_enabled) + : SendTest(VideoSendStreamTest::kDefaultTimeoutMs), send_count_(0), received_media_(false), received_fec_(false), - 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(); - } - } + header_extensions_enabled_(header_extensions_enabled) {} private: Action OnSendRtp(const uint8_t* packet, size_t length) override { RTPHeader header; EXPECT_TRUE(parser_->Parse(packet, length, &header)); - ++send_count_; + // 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)); + } + 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_) { @@ -386,27 +379,14 @@ class FecObserver : public test::EndToEndTest { } } - if (send_count_ > 100 && received_media_) { - if (received_fec_ || !expect_red_) - observation_complete_.Set(); - } + if (received_media_ && received_fec_ && send_count_ > 100) + 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, @@ -414,17 +394,10 @@ class FecObserver : public test::EndToEndTest { 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)); @@ -432,10 +405,6 @@ class FecObserver : public test::EndToEndTest { 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 { @@ -443,10 +412,6 @@ class FecObserver : public test::EndToEndTest { } 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_; @@ -455,37 +420,14 @@ class FecObserver : public test::EndToEndTest { }; TEST_F(VideoSendStreamTest, SupportsFecWithExtensions) { - FecObserver test(true, false, true, "VP8"); + FecObserver test(true); + RunBaseTest(&test); } TEST_F(VideoSendStreamTest, SupportsFecWithoutExtensions) { - FecObserver test(false, false, true, "VP8"); - RunBaseTest(&test); -} + FecObserver test(false); -// 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 b41e21b380..d49f505365 100644 --- a/webrtc/video/vie_channel.cc +++ b/webrtc/video/vie_channel.cc @@ -414,15 +414,13 @@ void ViEChannel::SetProtectionMode(bool enable_nack, bool enable_fec, int payload_type_red, int payload_type_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) { + // Validate payload types. + if (enable_fec) { 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.