zqz/platform-external-webrtc
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Bugfix: FlexFEC causes retransmit bitrate increase.

Browse Source 
When FlexFEC is enabled, sometimes media packet will be recovered by FEC before the actual media packet's arrival. In current implementation this will be considered as packet out of order and nack will be sent, thus cause large increase in retransmit bitrate.
This fix:
1. Avoid sending nack for packet out of order caused by "early" recovered media packets.
2. Save recovered media packet in a set, and do not send nack for these packets.

Bug: None
Change-Id: I008ef4e33668bce6d2cb9ff52b4b5c8e3f349965
Reviewed-on: https://webrtc-review.googlesource.com/c/108090
Commit-Queue: Ying Wang <yinwa@webrtc.org>
Reviewed-by: Philip Eliasson <philipel@webrtc.org>
Reviewed-by: Stefan Holmer <stefan@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#25444}
This commit is contained in:
Ying Wang
2018-10-31 10:12:27 +01:00
committed by Commit Bot
parent 8b7d206d37
commit b32bb959c9
5 changed files with 89 additions and 47 deletions
Download Patch File Download Diff File Expand all files Collapse all files

29
modules/video_coding/nack_module.cc
View File

@ -56,6 +56,12 @@ NackModule::NackModule(Clock* clock,
}
int NackModule::OnReceivedPacket(uint16_t seq_num, bool is_keyframe) {
return OnReceivedPacket(seq_num, is_keyframe, false);
}
int NackModule::OnReceivedPacket(uint16_t seq_num,
bool is_keyframe,
bool is_recovered) {
rtc::CritScope lock(&crit_);
// TODO(philipel): When the packet includes information whether it is
// retransmitted or not, use that value instead. For
@ -88,8 +94,6 @@ int NackModule::OnReceivedPacket(uint16_t seq_num, bool is_keyframe) {
UpdateReorderingStatistics(seq_num);
return nacks_sent_for_packet;
}
AddPacketsToNack(newest_seq_num_ + 1, seq_num);
newest_seq_num_ = seq_num;
// Keep track of new keyframes.
if (is_keyframe)
@ -100,6 +104,21 @@ int NackModule::OnReceivedPacket(uint16_t seq_num, bool is_keyframe) {
if (it != keyframe_list_.begin())
keyframe_list_.erase(keyframe_list_.begin(), it);
if (is_recovered) {
recovered_list_.insert(seq_num);
// Remove old ones so we don't accumulate recovered packets.
auto it = recovered_list_.lower_bound(seq_num - kMaxPacketAge);
if (it != recovered_list_.begin())
recovered_list_.erase(recovered_list_.begin(), it);
// Do not send nack for packets recovered by FEC or RTX.
return 0;
}
AddPacketsToNack(newest_seq_num_ + 1, seq_num);
newest_seq_num_ = seq_num;
// Are there any nacks that are waiting for this seq_num.
std::vector<uint16_t> nack_batch = GetNackBatch(kSeqNumOnly);
if (!nack_batch.empty())
@ -113,6 +132,8 @@ void NackModule::ClearUpTo(uint16_t seq_num) {
nack_list_.erase(nack_list_.begin(), nack_list_.lower_bound(seq_num));
keyframe_list_.erase(keyframe_list_.begin(),
keyframe_list_.lower_bound(seq_num));
recovered_list_.erase(recovered_list_.begin(),
recovered_list_.lower_bound(seq_num));
}
void NackModule::UpdateRtt(int64_t rtt_ms) {
@ -124,6 +145,7 @@ void NackModule::Clear() {
rtc::CritScope lock(&crit_);
nack_list_.clear();
keyframe_list_.clear();
recovered_list_.clear();
}
int64_t NackModule::TimeUntilNextProcess() {
@ -200,6 +222,9 @@ void NackModule::AddPacketsToNack(uint16_t seq_num_start,
}
for (uint16_t seq_num = seq_num_start; seq_num != seq_num_end; ++seq_num) {
// Do not send nack for packets that are already recovered by FEC or RTX
if (recovered_list_.find(seq_num) != recovered_list_.end())
continue;
NackInfo nack_info(seq_num, seq_num + WaitNumberOfPackets(0.5));
RTC_DCHECK(nack_list_.find(seq_num) == nack_list_.end());
nack_list_[seq_num] = nack_info;

4
modules/video_coding/nack_module.h
View File

@ -32,6 +32,8 @@ class NackModule : public Module {
KeyFrameRequestSender* keyframe_request_sender);
int OnReceivedPacket(uint16_t seq_num, bool is_keyframe);
int OnReceivedPacket(uint16_t seq_num, bool is_keyframe, bool is_recovered);
void ClearUpTo(uint16_t seq_num);
void UpdateRtt(int64_t rtt_ms);
void Clear();
@ -87,6 +89,8 @@ class NackModule : public Module {
RTC_GUARDED_BY(crit_);
std::set<uint16_t, DescendingSeqNumComp<uint16_t>> keyframe_list_
RTC_GUARDED_BY(crit_);
std::set<uint16_t, DescendingSeqNumComp<uint16_t>> recovered_list_
RTC_GUARDED_BY(crit_);
video_coding::Histogram reordering_histogram_ RTC_GUARDED_BY(crit_);
bool initialized_ RTC_GUARDED_BY(crit_);
int64_t rtt_ms_ RTC_GUARDED_BY(crit_);

86
modules/video_coding/nack_module_unittest.cc
View File

@ -40,38 +40,38 @@ class TestNackModule : public ::testing::Test,
};
TEST_F(TestNackModule, NackOnePacket) {
nack_module_.OnReceivedPacket(1, false);
nack_module_.OnReceivedPacket(3, false);
nack_module_.OnReceivedPacket(1, false, false);
nack_module_.OnReceivedPacket(3, false, false);
EXPECT_EQ(1u, sent_nacks_.size());
EXPECT_EQ(2, sent_nacks_[0]);
}
TEST_F(TestNackModule, WrappingSeqNum) {
nack_module_.OnReceivedPacket(0xfffe, false);
nack_module_.OnReceivedPacket(1, false);
nack_module_.OnReceivedPacket(0xfffe, false, false);
nack_module_.OnReceivedPacket(1, false, false);
EXPECT_EQ(2u, sent_nacks_.size());
EXPECT_EQ(0xffff, sent_nacks_[0]);
EXPECT_EQ(0, sent_nacks_[1]);
}
TEST_F(TestNackModule, WrappingSeqNumClearToKeyframe) {
nack_module_.OnReceivedPacket(0xfffe, false);
nack_module_.OnReceivedPacket(1, false);
nack_module_.OnReceivedPacket(0xfffe, false, false);
nack_module_.OnReceivedPacket(1, false, false);
EXPECT_EQ(2u, sent_nacks_.size());
EXPECT_EQ(0xffff, sent_nacks_[0]);
EXPECT_EQ(0, sent_nacks_[1]);
sent_nacks_.clear();
nack_module_.OnReceivedPacket(2, true);
nack_module_.OnReceivedPacket(2, true, false);
EXPECT_EQ(0u, sent_nacks_.size());
nack_module_.OnReceivedPacket(501, true);
nack_module_.OnReceivedPacket(501, true, false);
EXPECT_EQ(498u, sent_nacks_.size());
for (int seq_num = 3; seq_num < 501; ++seq_num)
EXPECT_EQ(seq_num, sent_nacks_[seq_num - 3]);
sent_nacks_.clear();
nack_module_.OnReceivedPacket(1001, false);
nack_module_.OnReceivedPacket(1001, false, false);
EXPECT_EQ(499u, sent_nacks_.size());
for (int seq_num = 502; seq_num < 1001; ++seq_num)
EXPECT_EQ(seq_num, sent_nacks_[seq_num - 502]);
@ -91,7 +91,7 @@ TEST_F(TestNackModule, WrappingSeqNumClearToKeyframe) {
// It will then clear all nacks up to the next keyframe (seq num 2),
// thus removing 0xffff and 0 from the nack list.
sent_nacks_.clear();
nack_module_.OnReceivedPacket(1004, false);
nack_module_.OnReceivedPacket(1004, false, false);
EXPECT_EQ(2u, sent_nacks_.size());
EXPECT_EQ(1002, sent_nacks_[0]);
EXPECT_EQ(1003, sent_nacks_[1]);
@ -107,7 +107,7 @@ TEST_F(TestNackModule, WrappingSeqNumClearToKeyframe) {
// Adding packet 1007 will cause the nack module to overflow again, thus
// clearing everything up to 501 which is the next keyframe.
nack_module_.OnReceivedPacket(1007, false);
nack_module_.OnReceivedPacket(1007, false, false);
sent_nacks_.clear();
clock_->AdvanceTimeMilliseconds(100);
nack_module_.Process();
@ -146,8 +146,8 @@ TEST_F(TestNackModule, DontBurstOnTimeSkip) {
}
TEST_F(TestNackModule, ResendNack) {
nack_module_.OnReceivedPacket(1, false);
nack_module_.OnReceivedPacket(3, false);
nack_module_.OnReceivedPacket(1, false, false);
nack_module_.OnReceivedPacket(3, false, false);
EXPECT_EQ(1u, sent_nacks_.size());
EXPECT_EQ(2, sent_nacks_[0]);
@ -169,15 +169,15 @@ TEST_F(TestNackModule, ResendNack) {
nack_module_.Process();
EXPECT_EQ(4u, sent_nacks_.size());
nack_module_.OnReceivedPacket(2, false);
nack_module_.OnReceivedPacket(2, false, false);
clock_->AdvanceTimeMilliseconds(50);
nack_module_.Process();
EXPECT_EQ(4u, sent_nacks_.size());
}
TEST_F(TestNackModule, ResendPacketMaxRetries) {
nack_module_.OnReceivedPacket(1, false);
nack_module_.OnReceivedPacket(3, false);
nack_module_.OnReceivedPacket(1, false, false);
nack_module_.OnReceivedPacket(3, false, false);
EXPECT_EQ(1u, sent_nacks_.size());
EXPECT_EQ(2, sent_nacks_[0]);
@ -193,35 +193,35 @@ TEST_F(TestNackModule, ResendPacketMaxRetries) {
}
TEST_F(TestNackModule, TooLargeNackList) {
nack_module_.OnReceivedPacket(0, false);
nack_module_.OnReceivedPacket(1001, false);
nack_module_.OnReceivedPacket(0, false, false);
nack_module_.OnReceivedPacket(1001, false, false);
EXPECT_EQ(1000u, sent_nacks_.size());
EXPECT_EQ(0, keyframes_requested_);
nack_module_.OnReceivedPacket(1003, false);
nack_module_.OnReceivedPacket(1003, false, false);
EXPECT_EQ(1000u, sent_nacks_.size());
EXPECT_EQ(1, keyframes_requested_);
nack_module_.OnReceivedPacket(1004, false);
nack_module_.OnReceivedPacket(1004, false, false);
EXPECT_EQ(1000u, sent_nacks_.size());
EXPECT_EQ(1, keyframes_requested_);
}
TEST_F(TestNackModule, TooLargeNackListWithKeyFrame) {
nack_module_.OnReceivedPacket(0, false);
nack_module_.OnReceivedPacket(1, true);
nack_module_.OnReceivedPacket(1001, false);
nack_module_.OnReceivedPacket(0, false, false);
nack_module_.OnReceivedPacket(1, true, false);
nack_module_.OnReceivedPacket(1001, false, false);
EXPECT_EQ(999u, sent_nacks_.size());
EXPECT_EQ(0, keyframes_requested_);
nack_module_.OnReceivedPacket(1003, false);
nack_module_.OnReceivedPacket(1003, false, false);
EXPECT_EQ(1000u, sent_nacks_.size());
EXPECT_EQ(0, keyframes_requested_);
nack_module_.OnReceivedPacket(1005, false);
nack_module_.OnReceivedPacket(1005, false, false);
EXPECT_EQ(1000u, sent_nacks_.size());
EXPECT_EQ(1, keyframes_requested_);
}
TEST_F(TestNackModule, ClearUpTo) {
nack_module_.OnReceivedPacket(0, false);
nack_module_.OnReceivedPacket(100, false);
nack_module_.OnReceivedPacket(0, false, false);
nack_module_.OnReceivedPacket(100, false, false);
EXPECT_EQ(99u, sent_nacks_.size());
sent_nacks_.clear();
@ -233,8 +233,8 @@ TEST_F(TestNackModule, ClearUpTo) {
}
TEST_F(TestNackModule, ClearUpToWrap) {
nack_module_.OnReceivedPacket(0xfff0, false);
nack_module_.OnReceivedPacket(0xf, false);
nack_module_.OnReceivedPacket(0xfff0, false, false);
nack_module_.OnReceivedPacket(0xf, false, false);
EXPECT_EQ(30u, sent_nacks_.size());
sent_nacks_.clear();
@ -246,20 +246,20 @@ TEST_F(TestNackModule, ClearUpToWrap) {
}
TEST_F(TestNackModule, PacketNackCount) {
EXPECT_EQ(0, nack_module_.OnReceivedPacket(0, false));
EXPECT_EQ(0, nack_module_.OnReceivedPacket(2, false));
EXPECT_EQ(1, nack_module_.OnReceivedPacket(1, false));
EXPECT_EQ(0, nack_module_.OnReceivedPacket(0, false, false));
EXPECT_EQ(0, nack_module_.OnReceivedPacket(2, false, false));
EXPECT_EQ(1, nack_module_.OnReceivedPacket(1, false, false));
sent_nacks_.clear();
nack_module_.UpdateRtt(100);
EXPECT_EQ(0, nack_module_.OnReceivedPacket(5, false));
EXPECT_EQ(0, nack_module_.OnReceivedPacket(5, false, false));
clock_->AdvanceTimeMilliseconds(100);
nack_module_.Process();
clock_->AdvanceTimeMilliseconds(100);
nack_module_.Process();
EXPECT_EQ(3, nack_module_.OnReceivedPacket(3, false));
EXPECT_EQ(3, nack_module_.OnReceivedPacket(4, false));
EXPECT_EQ(0, nack_module_.OnReceivedPacket(4, false));
EXPECT_EQ(3, nack_module_.OnReceivedPacket(3, false, false));
EXPECT_EQ(3, nack_module_.OnReceivedPacket(4, false, false));
EXPECT_EQ(0, nack_module_.OnReceivedPacket(4, false, false));
}
TEST_F(TestNackModule, NackListFullAndNoOverlapWithKeyframes) {
@ -267,14 +267,22 @@ TEST_F(TestNackModule, NackListFullAndNoOverlapWithKeyframes) {
const unsigned int kFirstGap = kMaxNackPackets - 20;
const unsigned int kSecondGap = 200;
uint16_t seq_num = 0;
nack_module_.OnReceivedPacket(seq_num++, true);
nack_module_.OnReceivedPacket(seq_num++, true, false);
seq_num += kFirstGap;
nack_module_.OnReceivedPacket(seq_num++, true);
nack_module_.OnReceivedPacket(seq_num++, true, false);
EXPECT_EQ(kFirstGap, sent_nacks_.size());
sent_nacks_.clear();
seq_num += kSecondGap;
nack_module_.OnReceivedPacket(seq_num, true);
nack_module_.OnReceivedPacket(seq_num, true, false);
EXPECT_EQ(kSecondGap, sent_nacks_.size());
}
TEST_F(TestNackModule, HandleFecRecoveredPacket) {
nack_module_.OnReceivedPacket(1, false, false);
nack_module_.OnReceivedPacket(4, false, true);
EXPECT_EQ(0u, sent_nacks_.size());
nack_module_.OnReceivedPacket(5, false, false);
EXPECT_EQ(2u, sent_nacks_.size());
}
} // namespace webrtc

14
video/rtp_video_stream_receiver.cc
View File

@ -218,14 +218,15 @@ int32_t RtpVideoStreamReceiver::OnReceivedPayloadData(
size_t payload_size,
const WebRtcRTPHeader* rtp_header) {
return OnReceivedPayloadData(payload_data, payload_size, rtp_header,
absl::nullopt);
absl::nullopt, false);
}
int32_t RtpVideoStreamReceiver::OnReceivedPayloadData(
const uint8_t* payload_data,
size_t payload_size,
const WebRtcRTPHeader* rtp_header,
const absl::optional<RtpGenericFrameDescriptor>& generic_descriptor) {
const absl::optional<RtpGenericFrameDescriptor>& generic_descriptor,
bool is_recovered) {
WebRtcRTPHeader rtp_header_with_ntp = *rtp_header;
rtp_header_with_ntp.ntp_time_ms =
ntp_estimator_.Estimate(rtp_header->header.timestamp);
@ -237,7 +238,8 @@ int32_t RtpVideoStreamReceiver::OnReceivedPayloadData(
rtp_header->frameType == kVideoFrameKey;
packet.timesNacked = nack_module_->OnReceivedPacket(
rtp_header->header.sequenceNumber, is_keyframe);
rtp_header->header.sequenceNumber, is_keyframe, is_recovered);
} else {
packet.timesNacked = -1;
}
@ -568,7 +570,8 @@ void RtpVideoStreamReceiver::ReceivePacket(const RtpPacketReceived& packet) {
}
OnReceivedPayloadData(parsed_payload.payload, parsed_payload.payload_length,
&webrtc_rtp_header, generic_descriptor_wire);
&webrtc_rtp_header, generic_descriptor_wire,
packet.recovered());
}
void RtpVideoStreamReceiver::ParseAndHandleEncapsulatingHeader(
@ -600,7 +603,8 @@ void RtpVideoStreamReceiver::NotifyReceiverOfEmptyPacket(uint16_t seq_num) {
reference_finder_->PaddingReceived(seq_num);
packet_buffer_->PaddingReceived(seq_num);
if (nack_module_) {
nack_module_->OnReceivedPacket(seq_num, /* is_keyframe = */ false);
nack_module_->OnReceivedPacket(seq_num, /* is_keyframe = */ false,
/* is _recovered = */ false);
}
}

3
video/rtp_video_stream_receiver.h
View File

@ -104,7 +104,8 @@ class RtpVideoStreamReceiver : public RecoveredPacketReceiver,
const uint8_t* payload_data,
size_t payload_size,
const WebRtcRTPHeader* rtp_header,
const absl::optional<RtpGenericFrameDescriptor>& generic_descriptor);
const absl::optional<RtpGenericFrameDescriptor>& generic_descriptor,
bool is_recovered);
// Implements RecoveredPacketReceiver.
void OnRecoveredPacket(const uint8_t* packet, size_t packet_length) override;