Allow VideoTimingExtension to be used with FEC
This CL allows for FEC protection of packets with VideoTimingExtension by
zero-ing out data, which is changed after FEC protection is generated (i.e
in the pacer or by the SFU).
Actual FEC protection of these packets would be enabled later, when all
modern receivers have this change.
Bug: webrtc:10750
Change-Id: If4785392204d68cb8527629727b5c062f9fb6600
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/143760
Reviewed-by: Niels Moller <nisse@webrtc.org>
Reviewed-by: Rasmus Brandt <brandtr@webrtc.org>
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Commit-Queue: Ilya Nikolaevskiy <ilnik@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28396}
diff --git a/modules/rtp_rtcp/include/ulpfec_receiver.h b/modules/rtp_rtcp/include/ulpfec_receiver.h
index fce6e88..30fac72 100644
--- a/modules/rtp_rtcp/include/ulpfec_receiver.h
+++ b/modules/rtp_rtcp/include/ulpfec_receiver.h
@@ -11,6 +11,10 @@
#ifndef MODULES_RTP_RTCP_INCLUDE_ULPFEC_RECEIVER_H_
#define MODULES_RTP_RTCP_INCLUDE_ULPFEC_RECEIVER_H_
+#include <memory>
+
+#include "api/array_view.h"
+#include "api/rtp_parameters.h"
#include "modules/rtp_rtcp/include/rtp_rtcp_defines.h"
namespace webrtc {
@@ -30,8 +34,10 @@
class UlpfecReceiver {
public:
- static UlpfecReceiver* Create(uint32_t ssrc,
- RecoveredPacketReceiver* callback);
+ static std::unique_ptr<UlpfecReceiver> Create(
+ uint32_t ssrc,
+ RecoveredPacketReceiver* callback,
+ rtc::ArrayView<const RtpExtension> extensions);
virtual ~UlpfecReceiver() {}
diff --git a/modules/rtp_rtcp/source/flexfec_receiver.cc b/modules/rtp_rtcp/source/flexfec_receiver.cc
index 27dacac..4c788f4 100644
--- a/modules/rtp_rtcp/source/flexfec_receiver.cc
+++ b/modules/rtp_rtcp/source/flexfec_receiver.cc
@@ -123,10 +123,10 @@
received_packet->is_fec = false;
// Insert entire packet into erasure code.
- // TODO(brandtr): Remove this memcpy too.
received_packet->pkt = rtc::scoped_refptr<ForwardErrorCorrection::Packet>(
new ForwardErrorCorrection::Packet());
- memcpy(received_packet->pkt->data, packet.data(), packet.size());
+ // Create a copy and fill with zeros all mutable extensions.
+ packet.CopyAndZeroMutableExtensions(received_packet->pkt->data);
received_packet->pkt->length = packet.size();
}
diff --git a/modules/rtp_rtcp/source/rtp_packet.cc b/modules/rtp_rtcp/source/rtp_packet.cc
index 7e7a697..282bea2 100644
--- a/modules/rtp_rtcp/source/rtp_packet.cc
+++ b/modules/rtp_rtcp/source/rtp_packet.cc
@@ -156,6 +156,51 @@
ByteWriter<uint32_t>::WriteBigEndian(WriteAt(8), ssrc);
}
+void RtpPacket::CopyAndZeroMutableExtensions(
+ rtc::ArrayView<uint8_t> buffer) const {
+ RTC_CHECK_GE(buffer.size(), buffer_.size());
+ memcpy(buffer.data(), buffer_.cdata(), buffer_.size());
+ for (const ExtensionInfo& extension : extension_entries_) {
+ switch (extensions_.GetType(extension.id)) {
+ case RTPExtensionType::kRtpExtensionNone: {
+ RTC_LOG(LS_WARNING) << "Unidentified extension in the packet.";
+ break;
+ }
+ case RTPExtensionType::kRtpExtensionVideoTiming: {
+ // Nullify 3 last entries: packetization delay and 2 network timestamps.
+ // Each of them is 2 bytes.
+ memset(buffer.data() + extension.offset +
+ VideoSendTiming::kPacerExitDeltaOffset,
+ 0, 6);
+ break;
+ }
+ case RTPExtensionType::kRtpExtensionTransportSequenceNumber:
+ case RTPExtensionType::kRtpExtensionTransportSequenceNumber02:
+ case RTPExtensionType::kRtpExtensionTransmissionTimeOffset:
+ case RTPExtensionType::kRtpExtensionAbsoluteSendTime: {
+ // Nullify whole extension, as it's filled in the pacer.
+ memset(buffer.data() + extension.offset, 0, extension.length);
+ break;
+ }
+ case RTPExtensionType::kRtpExtensionAudioLevel:
+ case RTPExtensionType::kRtpExtensionColorSpace:
+ case RTPExtensionType::kRtpExtensionFrameMarking:
+ case RTPExtensionType::kRtpExtensionGenericFrameDescriptor00:
+ case RTPExtensionType::kRtpExtensionGenericFrameDescriptor01:
+ case RTPExtensionType::kRtpExtensionMid:
+ case RTPExtensionType::kRtpExtensionNumberOfExtensions:
+ case RTPExtensionType::kRtpExtensionPlayoutDelay:
+ case RTPExtensionType::kRtpExtensionRepairedRtpStreamId:
+ case RTPExtensionType::kRtpExtensionRtpStreamId:
+ case RTPExtensionType::kRtpExtensionVideoContentType:
+ case RTPExtensionType::kRtpExtensionVideoRotation: {
+ // Non-mutable extension. Don't change it.
+ break;
+ }
+ }
+ }
+}
+
void RtpPacket::SetCsrcs(rtc::ArrayView<const uint32_t> csrcs) {
RTC_DCHECK_EQ(extensions_size_, 0);
RTC_DCHECK_EQ(payload_size_, 0);
diff --git a/modules/rtp_rtcp/source/rtp_packet.h b/modules/rtp_rtcp/source/rtp_packet.h
index a0ecf1e..8c4ddf3 100644
--- a/modules/rtp_rtcp/source/rtp_packet.h
+++ b/modules/rtp_rtcp/source/rtp_packet.h
@@ -85,6 +85,10 @@
void SetTimestamp(uint32_t timestamp);
void SetSsrc(uint32_t ssrc);
+ // Copies the buffer with zero-ed mutable extensions,
+ // which are modified after FEC protection is generated.
+ void CopyAndZeroMutableExtensions(rtc::ArrayView<uint8_t> buffer) const;
+
// Writes csrc list. Assumes:
// a) There is enough room left in buffer.
// b) Extension headers, payload or padding data has not already been added.
diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc
index cebc813..7cd1367 100644
--- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc
+++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc
@@ -1324,114 +1324,6 @@
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, NoFlexfecForTimingFrames) {
- constexpr uint32_t kTimestamp = 1234;
- const int64_t kCaptureTimeMs = fake_clock_.TimeInMilliseconds();
- 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,
- kNoMid, 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.ssrc(), &seq_num_allocator_, nullptr, nullptr, nullptr,
- &mock_rtc_event_log_, &send_packet_observer_,
- &retransmission_rate_limiter_, nullptr, false, nullptr, false, false,
- FieldTrialBasedConfig()));
- rtp_sender_->SetSSRC(kMediaSsrc);
- rtp_sender_->SetSequenceNumber(kSeqNum);
- rtp_sender_->SetStorePacketsStatus(true, 10);
-
- PlayoutDelayOracle playout_delay_oracle;
- RTPSenderVideo rtp_sender_video(
- &fake_clock_, rtp_sender_.get(), &flexfec_sender, &playout_delay_oracle,
- nullptr, false, false, FieldTrialBasedConfig());
- rtp_sender_video.RegisterPayloadType(kMediaPayloadType, "GENERIC",
- /*raw_payload=*/false);
-
- // Need extension to be registered for timing frames to be sent.
- ASSERT_EQ(0, rtp_sender_->RegisterRtpHeaderExtension(
- kRtpExtensionVideoTiming, kVideoTimingExtensionId));
-
- // 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_video.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.
-
- RTPVideoHeader video_header;
- video_header.video_timing.flags = VideoSendTiming::kTriggeredByTimer;
- EXPECT_TRUE(rtp_sender_video.SendVideo(
- VideoFrameType::kVideoFrameKey, kMediaPayloadType, kTimestamp,
- kCaptureTimeMs, kPayloadData, sizeof(kPayloadData), nullptr,
- &video_header, kDefaultExpectedRetransmissionTimeMs));
-
- EXPECT_CALL(mock_rtc_event_log_,
- LogProxy(SameRtcEventTypeAs(RtcEvent::Type::RtpPacketOutgoing)))
- .Times(1);
- EXPECT_EQ(RtpPacketSendResult::kSuccess,
- 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());
-
- // Now try to send not a timing frame.
- uint16_t flexfec_seq_num;
- EXPECT_CALL(mock_paced_sender_, InsertPacket(RtpPacketSender::kLowPriority,
- kFlexfecSsrc, _, _, _, false))
- .WillOnce(SaveArg<2>(&flexfec_seq_num));
- EXPECT_CALL(mock_paced_sender_,
- InsertPacket(RtpPacketSender::kLowPriority, kMediaSsrc,
- kSeqNum + 1, _, _, false));
- video_header.video_timing.flags = VideoSendTiming::kInvalid;
- EXPECT_TRUE(rtp_sender_video.SendVideo(
- VideoFrameType::kVideoFrameKey, kMediaPayloadType, kTimestamp + 1,
- kCaptureTimeMs + 1, kPayloadData, sizeof(kPayloadData), nullptr,
- &video_header, kDefaultExpectedRetransmissionTimeMs));
-
- EXPECT_CALL(mock_rtc_event_log_,
- LogProxy(SameRtcEventTypeAs(RtcEvent::Type::RtpPacketOutgoing)))
- .Times(2);
- EXPECT_EQ(RtpPacketSendResult::kSuccess,
- rtp_sender_->TimeToSendPacket(kMediaSsrc, kSeqNum + 1,
- fake_clock_.TimeInMilliseconds(),
- false, PacedPacketInfo()));
- EXPECT_EQ(RtpPacketSendResult::kSuccess,
- rtp_sender_->TimeToSendPacket(kFlexfecSsrc, flexfec_seq_num,
- fake_clock_.TimeInMilliseconds(),
- false, PacedPacketInfo()));
- ASSERT_EQ(3, transport_.packets_sent());
- const RtpPacketReceived& media_packet2 = transport_.sent_packets_[1];
- EXPECT_EQ(kMediaPayloadType, media_packet2.PayloadType());
- EXPECT_EQ(kSeqNum + 1, media_packet2.SequenceNumber());
- EXPECT_EQ(kMediaSsrc, media_packet2.Ssrc());
- const RtpPacketReceived& flexfec_packet = transport_.sent_packets_[2];
- EXPECT_EQ(kFlexfecPayloadType, flexfec_packet.PayloadType());
- EXPECT_EQ(flexfec_seq_num, flexfec_packet.SequenceNumber());
- EXPECT_EQ(kFlexfecSsrc, flexfec_packet.Ssrc());
-}
-
TEST_P(RtpSenderTestWithoutPacer, SendFlexfecPackets) {
constexpr uint32_t kTimestamp = 1234;
constexpr int kMediaPayloadType = 127;
diff --git a/modules/rtp_rtcp/source/rtp_sender_video.cc b/modules/rtp_rtcp/source/rtp_sender_video.cc
index bbadda8..750dcf5 100644
--- a/modules/rtp_rtcp/source/rtp_sender_video.cc
+++ b/modules/rtp_rtcp/source/rtp_sender_video.cc
@@ -692,12 +692,12 @@
// Put packetization finish timestamp into extension.
if (packet->HasExtension<VideoTimingExtension>()) {
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.
+ // TODO(webrtc:10750): wait a couple of months and remove the statement
+ // below. For now we can't use packets with VideoTimingFrame extensions in
+ // Fec because the extension is modified after FEC is calculated by pacer
+ // and network. This may cause corruptions in video payload and header.
+ // The fix in receive code is implemented, but until all the receivers
+ // are updated, senders can't send potentially breaking packets.
protect_packet = false;
}
diff --git a/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc b/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc
index 614d4aa..8efcaf2 100644
--- a/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc
+++ b/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc
@@ -14,21 +14,28 @@
#include <memory>
#include <utility>
+#include "absl/memory/memory.h"
#include "api/scoped_refptr.h"
#include "modules/rtp_rtcp/source/byte_io.h"
+#include "modules/rtp_rtcp/source/rtp_packet_received.h"
#include "rtc_base/logging.h"
#include "rtc_base/time_utils.h"
namespace webrtc {
-UlpfecReceiver* UlpfecReceiver::Create(uint32_t ssrc,
- RecoveredPacketReceiver* callback) {
- return new UlpfecReceiverImpl(ssrc, callback);
+std::unique_ptr<UlpfecReceiver> UlpfecReceiver::Create(
+ uint32_t ssrc,
+ RecoveredPacketReceiver* callback,
+ rtc::ArrayView<const RtpExtension> extensions) {
+ return absl::make_unique<UlpfecReceiverImpl>(ssrc, callback, extensions);
}
-UlpfecReceiverImpl::UlpfecReceiverImpl(uint32_t ssrc,
- RecoveredPacketReceiver* callback)
+UlpfecReceiverImpl::UlpfecReceiverImpl(
+ uint32_t ssrc,
+ RecoveredPacketReceiver* callback,
+ rtc::ArrayView<const RtpExtension> extensions)
: ssrc_(ssrc),
+ extensions_(extensions),
recovered_packet_callback_(callback),
fec_(ForwardErrorCorrection::CreateUlpfec(ssrc_)) {}
@@ -243,6 +250,13 @@
recovered_packet_callback_->OnRecoveredPacket(packet->data,
packet->length);
crit_sect_.Enter();
+ RtpPacketReceived rtp_packet;
+ // TODO(ilnik): move extension nullifying out of RtpPacket, so there's no
+ // need to create one here, and avoid two memcpy calls below.
+ rtp_packet.Parse(packet->data, packet->length); // Does memcopy.
+ rtp_packet.IdentifyExtensions(extensions_);
+ rtp_packet.CopyAndZeroMutableExtensions( // Does memcopy.
+ rtc::MakeArrayView(packet->data, packet->length));
}
fec_->DecodeFec(*received_packet, &recovered_packets_);
}
diff --git a/modules/rtp_rtcp/source/ulpfec_receiver_impl.h b/modules/rtp_rtcp/source/ulpfec_receiver_impl.h
index 2821513..fca80c4 100644
--- a/modules/rtp_rtcp/source/ulpfec_receiver_impl.h
+++ b/modules/rtp_rtcp/source/ulpfec_receiver_impl.h
@@ -17,6 +17,7 @@
#include <vector>
#include "api/rtp_headers.h"
+#include "modules/rtp_rtcp/include/rtp_header_extension_map.h"
#include "modules/rtp_rtcp/include/rtp_rtcp_defines.h"
#include "modules/rtp_rtcp/include/ulpfec_receiver.h"
#include "modules/rtp_rtcp/source/forward_error_correction.h"
@@ -26,7 +27,9 @@
class UlpfecReceiverImpl : public UlpfecReceiver {
public:
- explicit UlpfecReceiverImpl(uint32_t ssrc, RecoveredPacketReceiver* callback);
+ explicit UlpfecReceiverImpl(uint32_t ssrc,
+ RecoveredPacketReceiver* callback,
+ rtc::ArrayView<const RtpExtension> extensions);
~UlpfecReceiverImpl() override;
int32_t AddReceivedRedPacket(const RTPHeader& rtp_header,
@@ -40,6 +43,7 @@
private:
const uint32_t ssrc_;
+ const RtpHeaderExtensionMap extensions_;
rtc::CriticalSection crit_sect_;
RecoveredPacketReceiver* recovered_packet_callback_;
diff --git a/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc b/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc
index 2203fc8..dd33d6b 100644
--- a/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc
+++ b/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc
@@ -49,8 +49,9 @@
protected:
UlpfecReceiverTest()
: fec_(ForwardErrorCorrection::CreateUlpfec(kMediaSsrc)),
- receiver_fec_(
- UlpfecReceiver::Create(kMediaSsrc, &recovered_packet_receiver_)),
+ receiver_fec_(UlpfecReceiver::Create(kMediaSsrc,
+ &recovered_packet_receiver_,
+ {})),
packet_generator_(kMediaSsrc) {}
// Generates |num_fec_packets| FEC packets, given |media_packets|.
@@ -180,7 +181,7 @@
NullRecoveredPacketReceiver null_callback;
std::unique_ptr<UlpfecReceiver> receiver_fec(
- UlpfecReceiver::Create(kMediaSsrc, &null_callback));
+ UlpfecReceiver::Create(kMediaSsrc, &null_callback, {}));
receiver_fec->AddReceivedRedPacket(header, data, length, ulpfec_payload_type);
}
diff --git a/test/fuzzers/ulpfec_receiver_fuzzer.cc b/test/fuzzers/ulpfec_receiver_fuzzer.cc
index 7cb0fc6..1124e01 100644
--- a/test/fuzzers/ulpfec_receiver_fuzzer.cc
+++ b/test/fuzzers/ulpfec_receiver_fuzzer.cc
@@ -36,7 +36,7 @@
DummyCallback callback;
std::unique_ptr<UlpfecReceiver> receiver(
- UlpfecReceiver::Create(ulpfec_ssrc, &callback));
+ UlpfecReceiver::Create(ulpfec_ssrc, &callback, {}));
std::unique_ptr<uint8_t[]> packet;
size_t packet_length;
diff --git a/video/rtp_video_stream_receiver.cc b/video/rtp_video_stream_receiver.cc
index 237a4c2..547c2cf 100644
--- a/video/rtp_video_stream_receiver.cc
+++ b/video/rtp_video_stream_receiver.cc
@@ -179,7 +179,9 @@
ntp_estimator_(clock),
rtp_header_extensions_(config_.rtp.extensions),
rtp_receive_statistics_(rtp_receive_statistics),
- ulpfec_receiver_(UlpfecReceiver::Create(config->rtp.remote_ssrc, this)),
+ ulpfec_receiver_(UlpfecReceiver::Create(config->rtp.remote_ssrc,
+ this,
+ config->rtp.extensions)),
receiving_(false),
last_packet_log_ms_(-1),
rtp_rtcp_(CreateRtpRtcpModule(clock,
diff --git a/video/video_send_stream_tests.cc b/video/video_send_stream_tests.cc
index fc5a5b9..77a63f6 100644
--- a/video/video_send_stream_tests.cc
+++ b/video/video_send_stream_tests.cc
@@ -591,6 +591,7 @@
send_config->rtp.extensions.push_back(
RtpExtension(RtpExtension::kAbsSendTimeUri, kAbsSendTimeExtensionId));
}
+ (*receive_configs)[0].rtp.extensions = send_config->rtp.extensions;
encoder_config->codec_type = PayloadStringToCodecType(payload_name_);
(*receive_configs)[0].rtp.red_payload_type =
send_config->rtp.ulpfec.red_payload_type;
@@ -787,6 +788,7 @@
} else {
send_config->rtp.extensions.clear();
}
+ (*receive_configs)[0].rtp.extensions = send_config->rtp.extensions;
encoder_config->codec_type = PayloadStringToCodecType(payload_name_);
}
@@ -2033,8 +2035,7 @@
class StartBitrateObserver : public test::FakeEncoder {
public:
StartBitrateObserver()
- : FakeEncoder(Clock::GetRealTimeClock()),
- start_bitrate_kbps_(0) {}
+ : FakeEncoder(Clock::GetRealTimeClock()), start_bitrate_kbps_(0) {}
int32_t InitEncode(const VideoCodec* config,
const Settings& settings) override {
rtc::CritScope lock(&crit_);
