From 1a13c8f11aab24dbea8b337adf64cb56c0536eec Mon Sep 17 00:00:00 2001 From: Anton Sukhanov Date: Thu, 25 Jul 2019 14:36:41 -0700 Subject: [PATCH] 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 Reviewed-by: Steve Anton Reviewed-by: Stefan Holmer Reviewed-by: Bjorn Mellem Cr-Commit-Position: refs/heads/master@{#28686} --- modules/rtp_rtcp/source/rtp_sender_video.cc | 39 ++++++++++++++++++++- modules/rtp_rtcp/source/rtp_sender_video.h | 2 ++ pc/datagram_dtls_adaptor.cc | 5 +++ 3 files changed, 45 insertions(+), 1 deletion(-) diff --git a/modules/rtp_rtcp/source/rtp_sender_video.cc b/modules/rtp_rtcp/source/rtp_sender_video.cc index 632088baec..51441f6ab8 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video.cc +++ b/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( + &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()); diff --git a/modules/rtp_rtcp/source/rtp_sender_video.h b/modules/rtp_rtcp/source/rtp_sender_video.h index 3555958e3c..2505fe544b 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video.h +++ b/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 diff --git a/pc/datagram_dtls_adaptor.cc b/pc/datagram_dtls_adaptor.cc index e1added653..0b47078274 100644 --- a/pc/datagram_dtls_adaptor.cc +++ b/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();