admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix timing frames and FEC conflict

Browse Source 
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}
This commit is contained in:
ilnik 2017-06-21 08:23:19 -07:00 committed by Commit Bot
parent 83c97da593
commit 10894996ef
4 changed files with 126 additions and 22 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

22
webrtc/modules/rtp_rtcp/source/rtp_sender.cc
View File

@ -736,6 +736,14 @@ bool RTPSender::PrepareAndSendPacket(std::unique_ptr<RtpPacketToSend> 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<TransmissionOffset>(kTimestampTicksPerMs *
@ -743,11 +751,8 @@ bool RTPSender::PrepareAndSendPacket(std::unique_ptr<RtpPacketToSend> packet,
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 @@ bool RTPSender::SendToNetwork(std::unique_ptr<RtpPacketToSend> packet,
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));

100
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc
View File

@ -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<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;

10
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc
View File

@ -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.

16
webrtc/video/video_send_stream_tests.cc
View File

@ -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;
}