Add test which verifies that the RTP header extensions are set correctly for FEC packets.

Also taking the opportunity to do a little bit of clean up.

BUG=webrtc:705
R=pbos@webrtc.org

Review URL: https://codereview.webrtc.org/1506863002 .

Cr-Commit-Position: refs/heads/master@{#10927}

webrtc/modules/rtp_rtcp/source/forward_error_correction.cc

@@ -129,9 +129,7 @@ int32_t ForwardErrorCorrection::GenerateFEC(const PacketList& media_packet_list,
   int num_maskBytes = l_bit ? kMaskSizeLBitSet : kMaskSizeLBitClear;
 
   // Do some error checking on the media packets.
-  PacketList::const_iterator media_list_it = media_packet_list.begin();
+  for (Packet* media_packet : media_packet_list) {
-  while (media_list_it != media_packet_list.end()) {
-    Packet* media_packet = *media_list_it;
     assert(media_packet);
 
     if (media_packet->length < kRtpHeaderSize) {
@@ -146,7 +144,6 @@ int32_t ForwardErrorCorrection::GenerateFEC(const PacketList& media_packet_list,
       LOG(LS_WARNING) << "Media packet " << media_packet->length << " bytes "
                       << "with overhead is larger than " << IP_PACKET_SIZE;
     }
-    media_list_it++;
   }
 
   int num_fec_packets =
@@ -167,29 +164,30 @@ int32_t ForwardErrorCorrection::GenerateFEC(const PacketList& media_packet_list,
 
   // -- Generate packet masks --
   // Always allocate space for a large mask.
-  uint8_t* packet_mask = new uint8_t[num_fec_packets * kMaskSizeLBitSet];
+  rtc::scoped_ptr<uint8_t[]> packet_mask(
-  memset(packet_mask, 0, num_fec_packets * num_maskBytes);
+      new uint8_t[num_fec_packets * kMaskSizeLBitSet]);
+  memset(packet_mask.get(), 0, num_fec_packets * num_maskBytes);
   internal::GeneratePacketMasks(num_media_packets, num_fec_packets,
                                 num_important_packets, use_unequal_protection,
-                                mask_table, packet_mask);
+                                mask_table, packet_mask.get());
 
-  int num_maskBits = InsertZerosInBitMasks(media_packet_list, packet_mask,
+  int num_mask_bits = InsertZerosInBitMasks(
-                                           num_maskBytes, num_fec_packets);
+      media_packet_list, packet_mask.get(), num_maskBytes, num_fec_packets);
 
-  l_bit = (num_maskBits > 8 * kMaskSizeLBitClear);
+  l_bit = (num_mask_bits > 8 * kMaskSizeLBitClear);
 
-  if (num_maskBits < 0) {
+  if (num_mask_bits < 0) {
-    delete[] packet_mask;
     return -1;
   }
   if (l_bit) {
     num_maskBytes = kMaskSizeLBitSet;
   }
 
-  GenerateFecBitStrings(media_packet_list, packet_mask, num_fec_packets, l_bit);
+  GenerateFecBitStrings(media_packet_list, packet_mask.get(), num_fec_packets,
-  GenerateFecUlpHeaders(media_packet_list, packet_mask, l_bit, num_fec_packets);
+                        l_bit);
+  GenerateFecUlpHeaders(media_packet_list, packet_mask.get(), l_bit,
+                        num_fec_packets);
 
-  delete[] packet_mask;
   return 0;
 }
 

webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc

@@ -42,13 +42,13 @@ RTPSenderVideo::RTPSenderVideo(Clock* clock, RTPSenderInterface* rtpSender)
       _retransmissionSettings(kRetransmitBaseLayer),
 
       // Generic FEC
-      _fec(),
+      fec_(),
-      _fecEnabled(false),
+      fec_enabled_(false),
-      _payloadTypeRED(-1),
+      red_payload_type_(-1),
-      _payloadTypeFEC(-1),
+      fec_payload_type_(-1),
       delta_fec_params_(),
       key_fec_params_(),
-      producer_fec_(&_fec),
+      producer_fec_(&fec_),
       _fecOverheadRate(clock, NULL),
       _videoBitrate(clock, NULL) {
   memset(&delta_fec_params_, 0, sizeof(delta_fec_params_));
@@ -130,7 +130,7 @@ void RTPSenderVideo::SendVideoPacketAsRed(uint8_t* data_buffer,
     // Only protect while creating RED and FEC packets, not when sending.
     CriticalSectionScoped cs(crit_.get());
     red_packet.reset(producer_fec_.BuildRedPacket(
-        data_buffer, payload_length, rtp_header_length, _payloadTypeRED));
+        data_buffer, payload_length, rtp_header_length, red_payload_type_));
     if (protect) {
       producer_fec_.AddRtpPacketAndGenerateFec(data_buffer, payload_length,
                                                rtp_header_length);
@@ -140,7 +140,7 @@ void RTPSenderVideo::SendVideoPacketAsRed(uint8_t* data_buffer,
       next_fec_sequence_number =
           _rtpSender.AllocateSequenceNumber(num_fec_packets);
       fec_packets = producer_fec_.GetFecPackets(
-          _payloadTypeRED, _payloadTypeFEC, next_fec_sequence_number,
+          red_payload_type_, fec_payload_type_, next_fec_sequence_number,
           rtp_header_length);
       RTC_DCHECK_EQ(num_fec_packets, fec_packets.size());
       if (_retransmissionSettings & kRetransmitFECPackets)
@@ -180,9 +180,9 @@ void RTPSenderVideo::SetGenericFECStatus(const bool enable,
                                          const uint8_t payloadTypeRED,
                                          const uint8_t payloadTypeFEC) {
   CriticalSectionScoped cs(crit_.get());
-  _fecEnabled = enable;
+  fec_enabled_ = enable;
-  _payloadTypeRED = payloadTypeRED;
+  red_payload_type_ = payloadTypeRED;
-  _payloadTypeFEC = payloadTypeFEC;
+  fec_payload_type_ = payloadTypeFEC;
   memset(&delta_fec_params_, 0, sizeof(delta_fec_params_));
   memset(&key_fec_params_, 0, sizeof(key_fec_params_));
   delta_fec_params_.max_fec_frames = key_fec_params_.max_fec_frames = 1;
@@ -194,14 +194,14 @@ void RTPSenderVideo::GenericFECStatus(bool& enable,
                                       uint8_t& payloadTypeRED,
                                       uint8_t& payloadTypeFEC) const {
   CriticalSectionScoped cs(crit_.get());
-  enable = _fecEnabled;
+  enable = fec_enabled_;
-  payloadTypeRED = _payloadTypeRED;
+  payloadTypeRED = red_payload_type_;
-  payloadTypeFEC = _payloadTypeFEC;
+  payloadTypeFEC = fec_payload_type_;
 }
 
 size_t RTPSenderVideo::FECPacketOverhead() const {
   CriticalSectionScoped cs(crit_.get());
-  if (_fecEnabled) {
+  if (fec_enabled_) {
     // Overhead is FEC headers plus RED for FEC header plus anything in RTP
     // header beyond the 12 bytes base header (CSRC list, extensions...)
     // This reason for the header extensions to be included here is that
@@ -247,7 +247,7 @@ int32_t RTPSenderVideo::SendVideo(const RtpVideoCodecTypes videoType,
         frameType == kVideoFrameKey ? &key_fec_params_ : &delta_fec_params_;
     producer_fec_.SetFecParameters(fec_params, 0);
     storage = packetizer->GetStorageType(_retransmissionSettings);
-    fec_enabled = _fecEnabled;
+    fec_enabled = fec_enabled_;
   }
 
   // Register CVO rtp header extension at the first time when we receive a frame

webrtc/modules/rtp_rtcp/source/rtp_sender_video.h

@@ -110,10 +110,10 @@ private:
   int32_t _retransmissionSettings GUARDED_BY(crit_);
 
   // FEC
-  ForwardErrorCorrection _fec;
+  ForwardErrorCorrection fec_;
-  bool _fecEnabled GUARDED_BY(crit_);
+  bool fec_enabled_ GUARDED_BY(crit_);
-  int8_t _payloadTypeRED GUARDED_BY(crit_);
+  int8_t red_payload_type_ GUARDED_BY(crit_);
-  int8_t _payloadTypeFEC GUARDED_BY(crit_);
+  int8_t fec_payload_type_ GUARDED_BY(crit_);
   FecProtectionParams delta_fec_params_ GUARDED_BY(crit_);
   FecProtectionParams key_fec_params_ GUARDED_BY(crit_);
   ProducerFec producer_fec_ GUARDED_BY(crit_);

webrtc/video/video_send_stream_tests.cc

@@ -127,12 +127,11 @@ TEST_F(VideoSendStreamTest, SupportsCName) {
 }
 
 TEST_F(VideoSendStreamTest, SupportsAbsoluteSendTime) {
-  static const uint8_t kAbsSendTimeExtensionId = 13;
   class AbsoluteSendTimeObserver : public test::SendTest {
    public:
     AbsoluteSendTimeObserver() : SendTest(kDefaultTimeoutMs) {
       EXPECT_TRUE(parser_->RegisterRtpHeaderExtension(
-          kRtpExtensionAbsoluteSendTime, kAbsSendTimeExtensionId));
+          kRtpExtensionAbsoluteSendTime, test::kAbsSendTimeExtensionId));
     }
 
     Action OnSendRtp(const uint8_t* packet, size_t length) override {
@@ -152,8 +151,8 @@ TEST_F(VideoSendStreamTest, SupportsAbsoluteSendTime) {
                        std::vector<VideoReceiveStream::Config>* receive_configs,
                        VideoEncoderConfig* encoder_config) override {
       send_config->rtp.extensions.clear();
-      send_config->rtp.extensions.push_back(
+      send_config->rtp.extensions.push_back(RtpExtension(
-          RtpExtension(RtpExtension::kAbsSendTime, kAbsSendTimeExtensionId));
+          RtpExtension::kAbsSendTime, test::kAbsSendTimeExtensionId));
     }
 
     void PerformTest() override {
@@ -310,81 +309,123 @@ class FakeReceiveStatistics : public NullReceiveStatistics {
   StatisticianMap stats_map_;
 };
 
-TEST_F(VideoSendStreamTest, SupportsFec) {
+class FecObserver : public test::SendTest {
-  class FecObserver : public test::SendTest {
+ public:
-   public:
+  explicit FecObserver(bool header_extensions_enabled)
-    FecObserver()
+      : SendTest(VideoSendStreamTest::kDefaultTimeoutMs),
-        : SendTest(kDefaultTimeoutMs),
+        send_count_(0),
-          send_count_(0),
+        received_media_(false),
-          received_media_(false),
+        received_fec_(false),
-          received_fec_(false) {
+        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 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::kSendSsrcs[0], header.sequenceNumber,
+          send_count_ / 2, 127);
+      RTCPSender rtcp_sender(false, Clock::GetRealTimeClock(),
+                             &lossy_receive_stats, nullptr,
+                             transport_adapter_.get());
+
+      rtcp_sender.SetRTCPStatus(RtcpMode::kReducedSize);
+      rtcp_sender.SetRemoteSSRC(VideoSendStreamTest::kSendSsrcs[0]);
+
+      RTCPSender::FeedbackState feedback_state;
+
+      EXPECT_EQ(0, rtcp_sender.SendRTCP(feedback_state, kRtcpRr));
     }
 
-   private:
+    int encapsulated_payload_type = -1;
-    Action OnSendRtp(const uint8_t* packet, size_t length) override {
+    if (header.payloadType == VideoSendStreamTest::kRedPayloadType) {
-      RTPHeader header;
+      encapsulated_payload_type = static_cast<int>(packet[header.headerLength]);
-      EXPECT_TRUE(parser_->Parse(packet, length, &header));
+      if (encapsulated_payload_type !=
+          VideoSendStreamTest::kFakeSendPayloadType)
+        EXPECT_EQ(VideoSendStreamTest::kUlpfecPayloadType,
+                  encapsulated_payload_type);
+    } else {
+      EXPECT_EQ(VideoSendStreamTest::kFakeSendPayloadType, header.payloadType);
+    }
 
-      // Send lossy receive reports to trigger FEC enabling.
+    if (header_extensions_enabled_) {
-      if (send_count_++ % 2 != 0) {
+      EXPECT_TRUE(header.extension.hasAbsoluteSendTime);
-        // Receive statistics reporting having lost 50% of the packets.
+      uint32_t kHalf24BitsSpace = 0xFFFFFF / 2;
-        FakeReceiveStatistics lossy_receive_stats(
+      if (header.extension.absoluteSendTime <= kHalf24BitsSpace &&
-            kSendSsrcs[0], header.sequenceNumber, send_count_ / 2, 127);
+          prev_header_.extension.absoluteSendTime > kHalf24BitsSpace) {
-        RTCPSender rtcp_sender(false, Clock::GetRealTimeClock(),
+        // 24 bits wrap.
-                               &lossy_receive_stats, nullptr,
+        EXPECT_GT(prev_header_.extension.absoluteSendTime,
-                               transport_adapter_.get());
+                  header.extension.absoluteSendTime);
-
-        rtcp_sender.SetRTCPStatus(RtcpMode::kReducedSize);
-        rtcp_sender.SetRemoteSSRC(kSendSsrcs[0]);
-
-        RTCPSender::FeedbackState feedback_state;
-
-        EXPECT_EQ(0, rtcp_sender.SendRTCP(feedback_state, kRtcpRr));
-      }
-
-      int encapsulated_payload_type = -1;
-      if (header.payloadType == kRedPayloadType) {
-        encapsulated_payload_type =
-            static_cast<int>(packet[header.headerLength]);
-        if (encapsulated_payload_type != kFakeSendPayloadType)
-          EXPECT_EQ(kUlpfecPayloadType, encapsulated_payload_type);
       } else {
-        EXPECT_EQ(kFakeSendPayloadType, header.payloadType);
+        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 != -1) {
-        if (encapsulated_payload_type == kUlpfecPayloadType) {
+      if (encapsulated_payload_type ==
-          received_fec_ = true;
+          VideoSendStreamTest::kUlpfecPayloadType) {
-        } else {
+        received_fec_ = true;
-          received_media_ = true;
+      } else {
-        }
+        received_media_ = true;
       }
-
-      if (received_media_ && received_fec_)
-        observation_complete_->Set();
-
-      return SEND_PACKET;
     }
 
-    void ModifyConfigs(VideoSendStream::Config* send_config,
+    if (received_media_ && received_fec_ && send_count_ > 100)
-                       std::vector<VideoReceiveStream::Config>* receive_configs,
+      observation_complete_->Set();
-                       VideoEncoderConfig* encoder_config) override {
-      transport_adapter_.reset(
-          new internal::TransportAdapter(send_config->send_transport));
-      transport_adapter_->Enable();
-      send_config->rtp.fec.red_payload_type = kRedPayloadType;
-      send_config->rtp.fec.ulpfec_payload_type = kUlpfecPayloadType;
-    }
 
-    void PerformTest() override {
+    prev_header_ = header;
-      EXPECT_TRUE(Wait()) << "Timed out waiting for FEC and media packets.";
-    }
 
-    rtc::scoped_ptr<internal::TransportAdapter> transport_adapter_;
+    return SEND_PACKET;
-    int send_count_;
+  }
-    bool received_media_;
+
-    bool received_fec_;
+  void ModifyConfigs(VideoSendStream::Config* send_config,
-  } test;
+                     std::vector<VideoReceiveStream::Config>* receive_configs,
+                     VideoEncoderConfig* encoder_config) override {
+    transport_adapter_.reset(
+        new internal::TransportAdapter(send_config->send_transport));
+    transport_adapter_->Enable();
+    send_config->rtp.fec.red_payload_type =
+        VideoSendStreamTest::kRedPayloadType;
+    send_config->rtp.fec.ulpfec_payload_type =
+        VideoSendStreamTest::kUlpfecPayloadType;
+    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_;
+  int send_count_;
+  bool received_media_;
+  bool received_fec_;
+  bool header_extensions_enabled_;
+  RTPHeader prev_header_;
+};
+
+TEST_F(VideoSendStreamTest, SupportsFecWithExtensions) {
+  FecObserver test(true);
+
+  RunBaseTest(&test, FakeNetworkPipe::Config());
+}
+
+TEST_F(VideoSendStreamTest, SupportsFecWithoutExtensions) {
+  FecObserver test(false);
 
   RunBaseTest(&test, FakeNetworkPipe::Config());
 }