From 10894996efb088c305db07365c1871e02c98a818 Mon Sep 17 00:00:00 2001 From: ilnik Date: Wed, 21 Jun 2017 08:23:19 -0700 Subject: [PATCH] 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} --- webrtc/modules/rtp_rtcp/source/rtp_sender.cc | 22 ++-- .../rtp_rtcp/source/rtp_sender_unittest.cc | 100 +++++++++++++++++- .../rtp_rtcp/source/rtp_sender_video.cc | 10 +- webrtc/video/video_send_stream_tests.cc | 16 +-- 4 files changed, 126 insertions(+), 22 deletions(-) diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc index 0dfa062e3c..759bc9c980 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc @@ -736,6 +736,14 @@ bool RTPSender::PrepareAndSendPacket(std::unique_ptr packet, 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(kTimestampTicksPerMs * @@ -743,11 +751,8 @@ bool RTPSender::PrepareAndSendPacket(std::unique_ptr packet, packet_to_send->SetExtension( 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()) + packet_to_send->set_pacer_exit_time_ms(now_ms); PacketOptions options; if (UpdateTransportSequenceNumber(packet_to_send, &options.packet_id)) { @@ -836,11 +841,8 @@ bool RTPSender::SendToNetwork(std::unique_ptr packet, if (packet->capture_time_ms() > 0) { packet->SetExtension( 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()) + packet->set_pacer_exit_time_ms(now_ms); } packet->SetExtension(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 3e9ed6e195..9759f1799d 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 @@ TEST_P(RtpSenderTest, SendsPacketsWithTransportSequenceNumber) { 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 @@ TEST_P(RtpSenderTest, SendFlexfecPackets) { 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 kNoRtpExtensions; + const std::vector 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 41af62b9fa..351056e216 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 @@ bool RTPSenderVideo::SendVideo(RtpVideoCodecTypes video_type, 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 db1ba4f490..d759ed6ed6 100644 --- a/webrtc/video/video_send_stream_tests.cc +++ b/webrtc/video/video_send_stream_tests.cc @@ -296,9 +296,9 @@ TEST_F(VideoSendStreamTest, SupportsVideoRotation) { } 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, SupportsVideoContentType) { } 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 @@ TEST_F(VideoSendStreamTest, SupportsVideoTimingFrames) { 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; }