Fix timing frames and FEC conflict

webrtc/modules/rtp_rtcp/source/rtp_sender_video.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 367 matching lines @@
   size_t num_packets =
       packetizer->SetPayloadData(payload_data, payload_size, frag);
 
   if (num_packets == 0)
     return false;
 
   bool first_frame = first_frame_sent_();
   for (size_t i = 0; i < num_packets; ++i) {
     bool last = (i + 1) == num_packets;
     auto packet = last ? std::move(last_packet)
                        : rtc::MakeUnique<RtpPacketToSend>(*rtp_header);

[åsapersson, 2017/06/21 11:14:00]

Maybe also check in VideoSendStreamTest.SupportsVideoTimingFrames that if a
packet has the timing extension, it is the last packet in a frame (i.e marker
bit set).

[ilnik, 2017/06/21 11:22:04]

Done.

     if (!packetizer->NextPacket(packet.get()))
       return false;
     RTC_DCHECK_LE(packet->payload_size(),
                   last ? max_data_payload_length - last_packet_reduction_len
                        : max_data_payload_length);
     if (!rtp_sender_->AssignSequenceNumber(packet.get()))
       return false;
 
+    bool protect_packet =
+            (packetizer->GetProtectionType() == kProtectedPacket);
     // Put packetization finish timestamp into extension.
     if (last && is_timing_frame) {
       packet->set_packetization_finish_time_ms(clock_->TimeInMilliseconds());
+      // TODO(ilnik): Due to webrtc:7859, packets with timing extensions are not
+      // protected by FEC. It reduces FEC efficiency a bit. When FEC is moved
+      // below the pacer, it can be re-enabled for these packets.
+      // NOTE: Any RTP stream processor in the network, modifying 'network'
+      // timestamps in the timing frames extension have to be an end-point for
+      // FEC, otherwise recovered by FEC packets will be corrupted.
+      protect_packet = false;

[brandtr, 2017/06/21 11:05:39]

Can you add a unit test for this?

[ilnik, 2017/06/21 11:17:18]

What exactly do you want me to test? What packets with timing frames are not
protected by the FEC? What there are no corruptions with timing frames?

[brandtr, 2017/06/21 11:28:15]

The former. Either through a unit test here, or using an end-to-end test that
verifies that sent FEC packets are not protecting packets with timing headers. I
think the unit test should be less involved to write.

I will think about more ways to catch corruptions, so you don't have to add any
tests for that.

[ilnik, 2017/06/21 12:49:42]

Done.

     }
 
-    const bool protect_packet =
-        (packetizer->GetProtectionType() == kProtectedPacket);
     if (flexfec_enabled()) {
       // TODO(brandtr): Remove the FlexFEC code path when FlexfecSender
       // is wired up to PacedSender instead.
       SendVideoPacketWithFlexfec(std::move(packet), storage, protect_packet);
     } else if (red_enabled) {
       SendVideoPacketAsRedMaybeWithUlpfec(std::move(packet), storage,
                                           protect_packet);
     } else {
       SendVideoPacket(std::move(packet), storage);
     }
@@ skipping 29 matching lines @@
   rtc::CritScope cs(&crit_);
   return retransmission_settings_;
 }
 
 void RTPSenderVideo::SetSelectiveRetransmissions(uint8_t settings) {
   rtc::CritScope cs(&crit_);
   retransmission_settings_ = settings;
 }
 
 }  // namespace webrtc