Fix timing frames and FEC conflict

webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc

 /*
  *  Copyright (c) 2012 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.
  */
 
@@ skipping 445 matching lines @@
                                 fake_clock_.TimeInMilliseconds(), false,
                                 PacedPacketInfo());
 
   const auto& packet = transport_.last_sent_packet();
   uint16_t transport_seq_no;
   EXPECT_TRUE(packet.GetExtension<TransportSequenceNumber>(&transport_seq_no));
   EXPECT_EQ(kTransportSequenceNumber, transport_seq_no);
   EXPECT_EQ(transport_.last_packet_id_, transport_seq_no);
 }
 
-// Disabled due to webrtc:7859. Until issues with FEC resolved, pacer exit
-// timstamp is not updated in the pacer.
-TEST_P(RtpSenderTestWithoutPacer, DISABLED_WritesTimestampToTimingExtension) {
+TEST_P(RtpSenderTestWithoutPacer, WritesTimestampToTimingExtension) {
   rtp_sender_->SetStorePacketsStatus(true, 10);
   EXPECT_EQ(0, rtp_sender_->RegisterRtpHeaderExtension(
                    kRtpExtensionVideoTiming, kVideoTimingExtensionId));
   int64_t capture_time_ms = fake_clock_.TimeInMilliseconds();
   auto packet = rtp_sender_->AllocatePacket();
   packet->SetPayloadType(kPayload);
   packet->SetMarker(true);
   packet->SetTimestamp(kTimestamp);
   packet->set_capture_time_ms(capture_time_ms);
   const VideoTiming kVideoTiming = {0u, 0u, 0u, 0u, 0u, 0u, true};
@@ skipping 458 matching lines @@
   const RtpPacketReceived& media_packet = transport_.sent_packets_[0];
   EXPECT_EQ(kMediaPayloadType, media_packet.PayloadType());
   EXPECT_EQ(kSeqNum, media_packet.SequenceNumber());
   EXPECT_EQ(kMediaSsrc, media_packet.Ssrc());
   const RtpPacketReceived& flexfec_packet = transport_.sent_packets_[1];
   EXPECT_EQ(kFlexfecPayloadType, flexfec_packet.PayloadType());
   EXPECT_EQ(flexfec_seq_num, flexfec_packet.SequenceNumber());
   EXPECT_EQ(kFlexfecSsrc, flexfec_packet.Ssrc());
 }
 
+// TODO(ilnik): because of webrtc:7859. Once FEC moved below pacer, this test
+// should be removed.
+TEST_P(RtpSenderTest, NoFlexForTimingFrames) {

[brandtr, 2017/06/21 13:10:27]

Flexfec

[ilnik, 2017/06/21 13:28:06]

Done.

+  constexpr int kMediaPayloadType = 127;
+  constexpr int kFlexfecPayloadType = 118;
+  constexpr uint32_t kMediaSsrc = 1234;
+  constexpr uint32_t kFlexfecSsrc = 5678;
+  const std::vector<RtpExtension> kNoRtpExtensions;
+  const std::vector<RtpExtensionSize> kNoRtpExtensionSizes;
+  FlexfecSender flexfec_sender(kFlexfecPayloadType, kFlexfecSsrc, kMediaSsrc,
+                               kNoRtpExtensions, kNoRtpExtensionSizes,
+                               nullptr /* rtp_state */, &fake_clock_);
+
+  // Reset |rtp_sender_| to use FlexFEC.
+  rtp_sender_.reset(new RTPSender(
+      false, &fake_clock_, &transport_, &mock_paced_sender_, &flexfec_sender,
+      &seq_num_allocator_, nullptr, nullptr, nullptr, nullptr,
+      &mock_rtc_event_log_, &send_packet_observer_,
+      &retransmission_rate_limiter_, nullptr));
+  rtp_sender_->SetSSRC(kMediaSsrc);
+  rtp_sender_->SetSequenceNumber(kSeqNum);
+  rtp_sender_->SetSendPayloadType(kMediaPayloadType);
+  rtp_sender_->SetStorePacketsStatus(true, 10);
+
+  // Parameters selected to generate a single FEC packet per media packet.
+  FecProtectionParams params;
+  params.fec_rate = 15;
+  params.max_fec_frames = 1;
+  params.fec_mask_type = kFecMaskRandom;
+  rtp_sender_->SetFecParameters(params, params);
+
+  EXPECT_CALL(mock_paced_sender_,
+              InsertPacket(RtpPacketSender::kLowPriority, kMediaSsrc, kSeqNum,
+                           _, _, false));
+  EXPECT_CALL(mock_paced_sender_, InsertPacket(RtpPacketSender::kLowPriority,
+                                               kFlexfecSsrc, _, _, _, false))
+      .Times(0);  // Not called because packet should not be protected.
+
+  const uint32_t kTimestamp = 1234;
+  const uint8_t kPayloadType = 127;
+  const int64_t kCaptureTimeMs = fake_clock_.TimeInMilliseconds();
+  char payload_name[RTP_PAYLOAD_NAME_SIZE] = "GENERIC";
+  EXPECT_EQ(0, rtp_sender_->RegisterPayload(payload_name, kPayloadType, 90000,
+                                            0, 1500));
+  RTPVideoHeader video_header;
+  memset(&video_header, 0, sizeof(RTPVideoHeader));
+  video_header.video_timing.is_timing_frame = true;

[brandtr, 2017/06/21 13:10:26]

How about sending the same frame, but with |is_timing_frame| set to false as
well?

video_header.video_timing.is_timing_frame = false;
SendOutgoingData();
EXPECT_CALL...Times(1);

video_header.video_timing.is_timing_frame = true;
SendOutgoingData();
EXPECT_CALL...Times(0);

ASSERT_EQ(3, transport_.packets_sent());

[ilnik, 2017/06/21 13:28:06]

It's already done in RtpSenderTestWithoutPacer.SendFlexfecPackets but maybe for
clarity, it makes sense. Added.

[brandtr, 2017/06/21 13:32:00]

Right, this is just to verify that FEC actually would have been sent, if this
was not a timing frame.

[ilnik, 2017/06/21 13:50:47]

Acknowledged.

+  EXPECT_TRUE(rtp_sender_->SendOutgoingData(
+      kVideoFrameKey, kPayloadType, kTimestamp, kCaptureTimeMs, kPayloadData,
+      sizeof(kPayloadData), nullptr, &video_header, nullptr));
+
+  EXPECT_CALL(mock_rtc_event_log_,
+              LogRtpHeader(PacketDirection::kOutgoingPacket, _, _, _))
+      .Times(1);
+  EXPECT_TRUE(rtp_sender_->TimeToSendPacket(kMediaSsrc, kSeqNum,
+                                            fake_clock_.TimeInMilliseconds(),
+                                            false, PacedPacketInfo()));
+  ASSERT_EQ(1, transport_.packets_sent());
+  const RtpPacketReceived& media_packet = transport_.sent_packets_[0];
+  EXPECT_EQ(kMediaPayloadType, media_packet.PayloadType());
+  EXPECT_EQ(kSeqNum, media_packet.SequenceNumber());
+  EXPECT_EQ(kMediaSsrc, media_packet.Ssrc());
+}
+
+
 TEST_P(RtpSenderTestWithoutPacer, SendFlexfecPackets) {
   constexpr int kMediaPayloadType = 127;
   constexpr int kFlexfecPayloadType = 118;
   constexpr uint32_t kMediaSsrc = 1234;
   constexpr uint32_t kFlexfecSsrc = 5678;
   const std::vector<RtpExtension> kNoRtpExtensions;
   const std::vector<RtpExtensionSize> kNoRtpExtensionSizes;
   FlexfecSender flexfec_sender(kFlexfecPayloadType, kFlexfecSsrc, kMediaSsrc,
                                kNoRtpExtensions, kNoRtpExtensionSizes,
                                nullptr /* rtp_state */, &fake_clock_);
@@ skipping 661 matching lines @@
 INSTANTIATE_TEST_CASE_P(WithAndWithoutOverhead,
                         RtpSenderTestWithoutPacer,
                         ::testing::Bool());
 INSTANTIATE_TEST_CASE_P(WithAndWithoutOverhead,
                         RtpSenderVideoTest,
                         ::testing::Bool());
 INSTANTIATE_TEST_CASE_P(WithAndWithoutOverhead,
                         RtpSenderAudioTest,
                         ::testing::Bool());
 }  // namespace webrtc