Don't send FEC for H.264 with NACK enabled.

webrtc/video/video_send_stream_tests.cc

 /*
  *  Copyright (c) 2013 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 #include <algorithm>  // max
@@ skipping 290 matching lines @@
       return true;
     }
 
     RtcpStatistics stats_;
   };
 
   rtc::scoped_ptr<LossyStatistician> lossy_stats_;
   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<int>(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<size_t>(header.headerLength + header.paddingLength) <
+          length) {
+        // Not padding-only, media received outside of RED.
+        EXPECT_FALSE(expect_red_);
+        received_media_ = true;
+      }
     }
 
     if (header_extensions_enabled_) {
       EXPECT_TRUE(header.extension.hasAbsoluteSendTime);
       uint32_t kHalf24BitsSpace = 0xFFFFFF / 2;
       if (header.extension.absoluteSendTime <= kHalf24BitsSpace &&
           prev_header_.extension.absoluteSendTime > kHalf24BitsSpace) {
         // 24 bits wrap.
         EXPECT_GT(prev_header_.extension.absoluteSendTime,
                   header.extension.absoluteSendTime);
       } else {
         EXPECT_GE(header.extension.absoluteSendTime,
                   prev_header_.extension.absoluteSendTime);
       }
       EXPECT_TRUE(header.extension.hasTransportSequenceNumber);
       uint16_t seq_num_diff = header.extension.transportSequenceNumber -
                               prev_header_.extension.transportSequenceNumber;
       EXPECT_EQ(1, seq_num_diff);
     }
 
     if (encapsulated_payload_type != -1) {
       if (encapsulated_payload_type ==
           VideoSendStreamTest::kUlpfecPayloadType) {
         received_fec_ = true;
       } else {
         received_media_ = true;
       }
     }
 
-    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<VideoReceiveStream::Config>* receive_configs,
       VideoEncoderConfig* encoder_config) override {
     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;
+        (*receive_configs)[0].rtp.fec.red_payload_type =
+            VideoSendStreamTest::kRedPayloadType;
     send_config->rtp.fec.ulpfec_payload_type =
-        VideoSendStreamTest::kUlpfecPayloadType;
+        (*receive_configs)[0].rtp.fec.ulpfec_payload_type =
+            VideoSendStreamTest::kUlpfecPayloadType;

[stefan-webrtc, 2016/02/12 10:46:27]

I personally prefer to configure the sender first and the receivers last, which
I think is what is typically done in this file. Feel free to change to be
aligned.

[pbos-webrtc, 2016/02/12 12:00:27]

Done.

     if (header_extensions_enabled_) {
       send_config->rtp.extensions.push_back(RtpExtension(
           RtpExtension::kAbsSendTime, test::kAbsSendTimeExtensionId));
       send_config->rtp.extensions.push_back(
           RtpExtension(RtpExtension::kTransportSequenceNumber,
                        test::kTransportSequenceNumberExtensionId));
     }
   }
 
   void PerformTest() override {
     EXPECT_TRUE(Wait()) << "Timed out waiting for FEC and media packets.";
   }
 
   rtc::scoped_ptr<internal::TransportAdapter> transport_adapter_;
+  rtc::scoped_ptr<VideoEncoder> encoder_;
+  const std::string payload_name_;
+  const bool use_nack_;
+  const bool expect_red_;
   int send_count_;
   bool received_media_;
   bool received_fec_;
   bool header_extensions_enabled_;
   RTPHeader prev_header_;
 };
 
 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, efficiently wasting

[stefan-webrtc, 2016/02/12 10:46:27]

"effectively wasting"?

[pbos-webrtc, 2016/02/12 12:00:27]

Done.

+// bandwidth. Also FEC packets are not stored on the sender, so retransmission
+// requests for FEC packets are dropped.

[stefan-webrtc, 2016/02/12 10:46:27]

I think the main issue is that we have to wait for the FEC packets to arrive
before we can decode anything since we can't determine if we were missing
something valuable.

[pbos-webrtc, 2016/02/12 12:00:27]

Done.

+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);
+}
+
 void VideoSendStreamTest::TestNackRetransmission(
     uint32_t retransmit_ssrc,
     uint8_t retransmit_payload_type) {
   class NackObserver : public test::SendTest {
    public:
     explicit NackObserver(uint32_t retransmit_ssrc,
                           uint8_t retransmit_payload_type)
         : SendTest(kDefaultTimeoutMs),
           send_count_(0),
           retransmit_ssrc_(retransmit_ssrc),
@@ skipping 1813 matching lines @@
         observation_complete_.Set();
       }
     }
   } test;
 
   RunBaseTest(&test);
 }
 #endif
 
 }  // namespace webrtc