Add option to remove transport sequence number from FEC packet calculation

This is experimental field trial to exclude transport sequence number from FEC packets and should only be used in conjunction with datagram transport. Datagram transport removes transport sequence numbers from RTP packets and uses datagram feedback loop to re-generate RTCP feedback packets, but FEC contorol packets are calculated before sequence number is removed and as a result recovered packets will be corrupt unless we also remove transport sequence number during FEC calculations.

This change is a bit embarrassing, but it was the easiest workaround we found to make FEC work with datagrams. Added TODO to find better long term solution.

TODO(sukhanov): We need to find find better way to implement FEC with datagram transport, probably moving FEC to datagram integration layter. Wealso remove special field trial once we switch datagram path from RTCConfiguration flags to field trial and use the same field trial for FECworkaround.

Bug: webrtc:9719
Change-Id: I1e23c56e3cbaa087460410942fb6c5b4921a763e
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/146221
Commit-Queue: Anton Sukhanov <sukhanov@webrtc.org>
Reviewed-by: Steve Anton <steveanton@webrtc.org>
Reviewed-by: Stefan Holmer <stefan@webrtc.org>
Reviewed-by: Bjorn Mellem <mellem@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28686}

modules/rtp_rtcp/source/rtp_sender_video.cc

@@ -39,6 +39,22 @@ constexpr size_t kRedForFecHeaderLength = 1;
 constexpr size_t kRtpSequenceNumberMapMaxEntries = 1 << 13;
 constexpr int64_t kMaxUnretransmittableFrameIntervalMs = 33 * 4;
 
+// This is experimental field trial to exclude transport sequence number from
+// FEC packets and should only be used in conjunction with datagram transport.
+// Datagram transport removes transport sequence numbers from RTP packets and
+// uses datagram feedback loop to re-generate RTCP feedback packets, but FEC
+// contorol packets are calculated before sequence number is removed and as a
+// result recovered packets will be corrupt unless we also remove transport
+// sequence number during FEC calculation.
+//
+// TODO(sukhanov): We need to find find better way to implement FEC with
+// datagram transport, probably moving FEC to datagram integration layter. We
+// should also remove special field trial once we switch datagram path from
+// RTCConfiguration flags to field trial and use the same field trial for FEC
+// workaround.
+const char kExcludeTransportSequenceNumberFromFecFieldTrial[] =
+    "WebRTC-ExcludeTransportSequenceNumberFromFec";
+
 void BuildRedPayload(const RtpPacketToSend& media_packet,
                      RtpPacketToSend* red_packet) {
   uint8_t* red_payload = red_packet->AllocatePayload(
@@ -212,7 +228,10 @@ RTPSenderVideo::RTPSenderVideo(Clock* clock,
       require_frame_encryption_(require_frame_encryption),
       generic_descriptor_auth_experiment_(
           field_trials.Lookup("WebRTC-GenericDescriptorAuth").find("Enabled") ==
-          0) {
+          0),
+      exclude_transport_sequence_number_from_fec_experiment_(
+          field_trials.Lookup(kExcludeTransportSequenceNumberFromFecFieldTrial)
+              .find("Enabled") == 0) {
   RTC_DCHECK(playout_delay_oracle_);
 }
 
@@ -277,6 +296,24 @@ void RTPSenderVideo::SendVideoPacketAsRedMaybeWithUlpfec(
     red_packet->SetPayloadType(red_payload_type_);
     if (ulpfec_enabled()) {
       if (protect_media_packet) {
+        if (exclude_transport_sequence_number_from_fec_experiment_) {
+          // See comments at the top of the file why experiment
+          // "WebRTC-kExcludeTransportSequenceNumberFromFec" is needed in
+          // conjunction with datagram transport.
+          // TODO(sukhanov): We may also need to implement it for flexfec_sender
+          // if we decide to keep this approach in the future.
+          uint16_t transport_senquence_number;
+          if (media_packet->GetExtension<webrtc::TransportSequenceNumber>(
+                  &transport_senquence_number)) {
+            if (!media_packet->RemoveExtension(
+                    webrtc::TransportSequenceNumber::kId)) {
+              RTC_NOTREACHED()
+                  << "Failed to remove transport sequence number, packet="
+                  << media_packet->ToString();
+            }
+          }
+        }
+
         ulpfec_generator_.AddRtpPacketAndGenerateFec(
             media_packet->data(), media_packet->payload_size(),
             media_packet->headers_size());

modules/rtp_rtcp/source/rtp_sender_video.h

@@ -220,6 +220,8 @@ class RTPSenderVideo {
   bool require_frame_encryption_;
   // Set to true if the generic descriptor should be authenticated.
   const bool generic_descriptor_auth_experiment_;
+
+  const bool exclude_transport_sequence_number_from_fec_experiment_;
 };
 
 }  // namespace webrtc

pc/datagram_dtls_adaptor.cc

@@ -92,6 +92,11 @@ DatagramDtlsAdaptor::DatagramDtlsAdaptor(
                          "datagram transport connection";
   }
 
+  // TODO(sukhanov): Add CHECK to make sure that field trial
+  // WebRTC-ExcludeTransportSequenceNumberFromFecFieldTrial is enabled.
+  // If feedback loop is translation is enabled, FEC packets must exclude
+  // transport sequence numbers, otherwise recovered packets will be corrupt.
+
   RTC_DCHECK(ice_transport_);
   RTC_DCHECK(datagram_transport_);
   ConnectToIceTransport();