Fix timing frames and FEC conflict
Reenable pacer_exit timestamp updates for the timing frames and
exclude timing-frames carrying packets from the FEC.
BUG=webrtc:7859
Review-Url: https://codereview.webrtc.org/2947133002
Cr-Commit-Position: refs/heads/master@{#18702}
diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc
index 0dfa062..759bc9c 100644
--- a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc
@@ -736,6 +736,14 @@
packet_to_send = packet_rtx.get();
}
+ // Bug webrtc:7859. While FEC is invoked from rtp_sender_video, and not after
+ // the pacer, these modifications of the header below are happening after the
+ // FEC protection packets are calculated. This will corrupt recovered packets
+ // at the same place. It's not an issue for extensions, which are present in
+ // all the packets (their content just may be incorrect on recovered packets).
+ // In case of VideoTimingExtension, since it's present not in every packet,
+ // data after rtp header may be corrupted if these packets are protected by
+ // the FEC.
int64_t now_ms = clock_->TimeInMilliseconds();
int64_t diff_ms = now_ms - capture_time_ms;
packet_to_send->SetExtension<TransmissionOffset>(kTimestampTicksPerMs *
@@ -743,11 +751,8 @@
packet_to_send->SetExtension<AbsoluteSendTime>(
AbsoluteSendTime::MsTo24Bits(now_ms));
- // TODO(ilnik): (webrtc:7859) For now we can't modify pacer exit timestamp in
- // video timing extension because only some packets have it and it will break
- // FEC recovered packets, which will lead to corruptions. Ideally, here
- // |packet->set_pacer_exit_time_ms(now_ms)| should be called if
- // |VideoTimingExtension| is present.
+ if (packet_to_send->HasExtension<VideoTimingExtension>())
+ packet_to_send->set_pacer_exit_time_ms(now_ms);
PacketOptions options;
if (UpdateTransportSequenceNumber(packet_to_send, &options.packet_id)) {
@@ -836,11 +841,8 @@
if (packet->capture_time_ms() > 0) {
packet->SetExtension<TransmissionOffset>(
kTimestampTicksPerMs * (now_ms - packet->capture_time_ms()));
- // TODO(ilnik): (webrtc:7859) For now we can't modify pacer exit timestamp
- // in video timing extension because only some packets have it an it will
- // break FEC recovered packets, which will lead to corruptions. Ideally,
- // here |packet->set_pacer_exit_time_ms(now_ms)| should be called if
- // |VideoTimingExtension| is present.
+ if (packet->HasExtension<VideoTimingExtension>())
+ packet->set_pacer_exit_time_ms(now_ms);
}
packet->SetExtension<AbsoluteSendTime>(AbsoluteSendTime::MsTo24Bits(now_ms));
diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc
index 3e9ed6e..9759f17 100644
--- a/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc
@@ -463,9 +463,7 @@
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));
@@ -944,6 +942,102 @@
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 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;
+ 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());
+
+ // Now try to send not a timing frame.
+ uint16_t flexfec_seq_num;
+ EXPECT_CALL(mock_paced_sender_, InsertPacket(RtpPacketSender::kLowPriority,
+ kFlexfecSsrc, _, _, _, false))
+ .WillOnce(testing::SaveArg<2>(&flexfec_seq_num));
+ EXPECT_CALL(mock_paced_sender_,
+ InsertPacket(RtpPacketSender::kLowPriority, kMediaSsrc,
+ kSeqNum + 1, _, _, false));
+ video_header.video_timing.is_timing_frame = false;
+ EXPECT_TRUE(rtp_sender_->SendOutgoingData(
+ kVideoFrameKey, kPayloadType, kTimestamp + 1, kCaptureTimeMs + 1,
+ kPayloadData, sizeof(kPayloadData), nullptr, &video_header, nullptr));
+
+ EXPECT_CALL(mock_rtc_event_log_,
+ LogRtpHeader(PacketDirection::kOutgoingPacket, _, _, _))
+ .Times(2);
+ EXPECT_TRUE(rtp_sender_->TimeToSendPacket(kMediaSsrc, kSeqNum + 1,
+ fake_clock_.TimeInMilliseconds(),
+ false, PacedPacketInfo()));
+ EXPECT_TRUE(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 int kMediaPayloadType = 127;
constexpr int kFlexfecPayloadType = 118;
diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc
index 41af62b..351056e2 100644
--- a/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc
@@ -394,13 +394,19 @@
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;
}
- 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.
diff --git a/webrtc/video/video_send_stream_tests.cc b/webrtc/video/video_send_stream_tests.cc
index db1ba4f..d759ed6 100644
--- a/webrtc/video/video_send_stream_tests.cc
+++ b/webrtc/video/video_send_stream_tests.cc
@@ -296,9 +296,9 @@
}
TEST_F(VideoSendStreamTest, SupportsVideoContentType) {
- class VideoRotationObserver : public test::SendTest {
+ class VideoContentTypeObserver : public test::SendTest {
public:
- VideoRotationObserver() : SendTest(kDefaultTimeoutMs) {
+ VideoContentTypeObserver() : SendTest(kDefaultTimeoutMs) {
EXPECT_TRUE(parser_->RegisterRtpHeaderExtension(
kRtpExtensionVideoContentType, test::kVideoContentTypeExtensionId));
}
@@ -338,9 +338,9 @@
}
TEST_F(VideoSendStreamTest, SupportsVideoTimingFrames) {
- class VideoRotationObserver : public test::SendTest {
+ class VideoTimingObserver : public test::SendTest {
public:
- VideoRotationObserver() : SendTest(kDefaultTimeoutMs) {
+ VideoTimingObserver() : SendTest(kDefaultTimeoutMs) {
EXPECT_TRUE(parser_->RegisterRtpHeaderExtension(
kRtpExtensionVideoTiming, test::kVideoTimingExtensionId));
}
@@ -348,9 +348,11 @@
Action OnSendRtp(const uint8_t* packet, size_t length) override {
RTPHeader header;
EXPECT_TRUE(parser_->Parse(packet, length, &header));
- if (header.extension.has_video_timing) {
- observation_complete_.Set();
- }
+ // Only the last packet of the frame must have extension.
+ if (!header.markerBit)
+ return SEND_PACKET;
+ EXPECT_TRUE(header.extension.has_video_timing);
+ observation_complete_.Set();
return SEND_PACKET;
}
