Prevent updating state in the delay manager if the packet was reordered.
Currently, if the last packet was reordered (e.g. due to retransmission) then the next packet's inter-arrival time will be estimated incorrectly due to the jump in sequence numbers. This change prevents that by not resetting the stopwatch on reordered packets.
This will also better estimate inter-arrival times when we have multiple reordered packets in a burst. Currently we would only measure the iat of the first reordered packet correctly and not the ones coming after it.
There is a slight risk introducing this: If we would receive an out of order packet far into the future (in sequence numbers) and then continue getting packets in the normal order, then we would not update the current sequence number for these and incorrectly estimate their inter-arrival times since they would all be considered reordered.
Change-Id: Ic938a37cbddf1cb9c30b610218f56794568d3d01
Bug: webrtc:10178
Reviewed-on: https://webrtc-review.googlesource.com/c/119949
Reviewed-by: Minyue Li <minyue@webrtc.org>
Reviewed-by: Henrik Lundin <henrik.lundin@webrtc.org>
Commit-Queue: Jakob Ivarsson‎ <jakobi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26572}diff --git a/modules/audio_coding/neteq/decision_logic_unittest.cc b/modules/audio_coding/neteq/decision_logic_unittest.cc
index e54c213..ebc24d4 100644
--- a/modules/audio_coding/neteq/decision_logic_unittest.cc
+++ b/modules/audio_coding/neteq/decision_logic_unittest.cc
@@ -31,7 +31,7 @@
TickTimer tick_timer;
PacketBuffer packet_buffer(10, &tick_timer);
DelayPeakDetector delay_peak_detector(&tick_timer, false);
- DelayManager delay_manager(240, 0, &delay_peak_detector, &tick_timer);
+ DelayManager delay_manager(240, 0, false, &delay_peak_detector, &tick_timer);
BufferLevelFilter buffer_level_filter;
DecisionLogic* logic = DecisionLogic::Create(
fs_hz, output_size_samples, false, &decoder_database, packet_buffer,
@@ -48,7 +48,7 @@
TickTimer tick_timer;
PacketBuffer packet_buffer(10, &tick_timer);
DelayPeakDetector delay_peak_detector(&tick_timer, false);
- DelayManager delay_manager(240, 0, &delay_peak_detector, &tick_timer);
+ DelayManager delay_manager(240, 0, false, &delay_peak_detector, &tick_timer);
BufferLevelFilter buffer_level_filter;
{
test::ScopedFieldTrials field_trial(
diff --git a/modules/audio_coding/neteq/delay_manager.cc b/modules/audio_coding/neteq/delay_manager.cc
index 4ca545d1..c97d535 100644
--- a/modules/audio_coding/neteq/delay_manager.cc
+++ b/modules/audio_coding/neteq/delay_manager.cc
@@ -34,6 +34,8 @@
// Steady-state forgetting factor for |iat_vector_|, 0.9993 in Q15.
constexpr int kIatFactor_ = 32745;
constexpr int kMaxIat = 64; // Max inter-arrival time to register.
+constexpr int kMaxReorderedPackets =
+ 10; // Max number of consecutive reordered packets.
absl::optional<int> GetForcedLimitProbability() {
constexpr char kForceTargetDelayPercentileFieldTrial[] =
@@ -63,6 +65,7 @@
DelayManager::DelayManager(size_t max_packets_in_buffer,
int base_min_target_delay_ms,
+ bool enable_rtx_handling,
DelayPeakDetector* peak_detector,
const TickTimer* tick_timer)
: first_packet_received_(false),
@@ -85,7 +88,8 @@
last_pack_cng_or_dtmf_(1),
frame_length_change_experiment_(
field_trial::IsEnabled("WebRTC-Audio-NetEqFramelengthExperiment")),
- forced_limit_probability_(GetForcedLimitProbability()) {
+ forced_limit_probability_(GetForcedLimitProbability()),
+ enable_rtx_handling_(enable_rtx_handling) {
assert(peak_detector); // Should never be NULL.
RTC_DCHECK_GE(base_min_target_delay_ms_, 0);
RTC_DCHECK_LE(minimum_delay_ms_, maximum_delay_ms_);
@@ -146,6 +150,7 @@
rtc::saturated_cast<int>(1000 * packet_len_samp / sample_rate_hz);
}
+ bool reordered = false;
if (packet_len_ms > 0) {
// Cannot update statistics unless |packet_len_ms| is valid.
// Calculate inter-arrival time (IAT) in integer "packet times"
@@ -158,7 +163,6 @@
}
// Check for discontinuous packet sequence and re-ordering.
- bool reordered = false;
if (IsNewerSequenceNumber(sequence_number, last_seq_no_ + 1)) {
// Compensate for gap in the sequence numbers. Reduce IAT with the
// expected extra time due to lost packets, but ensure that the IAT is
@@ -183,6 +187,12 @@
LimitTargetLevel();
} // End if (packet_len_ms > 0).
+ if (enable_rtx_handling_ && reordered &&
+ num_reordered_packets_ < kMaxReorderedPackets) {
+ ++num_reordered_packets_;
+ return 0;
+ }
+ num_reordered_packets_ = 0;
// Prepare for next packet arrival.
packet_iat_stopwatch_ = tick_timer_->GetNewStopwatch();
last_seq_no_ = sequence_number;
diff --git a/modules/audio_coding/neteq/delay_manager.h b/modules/audio_coding/neteq/delay_manager.h
index 8eee49c..652a773 100644
--- a/modules/audio_coding/neteq/delay_manager.h
+++ b/modules/audio_coding/neteq/delay_manager.h
@@ -36,6 +36,7 @@
// PeakDetector object to the DelayManager.
DelayManager(size_t max_packets_in_buffer,
int base_min_target_delay_ms,
+ bool enable_rtx_handling,
DelayPeakDetector* peak_detector,
const TickTimer* tick_timer);
@@ -177,6 +178,8 @@
int last_pack_cng_or_dtmf_;
const bool frame_length_change_experiment_;
const absl::optional<int> forced_limit_probability_;
+ const bool enable_rtx_handling_;
+ int num_reordered_packets_ = 0; // Number of consecutive reordered packets.
RTC_DISALLOW_COPY_AND_ASSIGN(DelayManager);
};
diff --git a/modules/audio_coding/neteq/delay_manager_unittest.cc b/modules/audio_coding/neteq/delay_manager_unittest.cc
index b0e23a9..e33b2aa 100644
--- a/modules/audio_coding/neteq/delay_manager_unittest.cc
+++ b/modules/audio_coding/neteq/delay_manager_unittest.cc
@@ -46,6 +46,7 @@
MockDelayPeakDetector detector_;
uint16_t seq_no_;
uint32_t ts_;
+ bool enable_rtx_handling_ = false;
};
DelayManagerTest::DelayManagerTest()
@@ -60,8 +61,8 @@
void DelayManagerTest::RecreateDelayManager() {
EXPECT_CALL(detector_, Reset()).Times(1);
- dm_.reset(new DelayManager(kMaxNumberOfPackets, kMinDelayMs, &detector_,
- &tick_timer_));
+ dm_.reset(new DelayManager(kMaxNumberOfPackets, kMinDelayMs,
+ enable_rtx_handling_, &detector_, &tick_timer_));
}
void DelayManagerTest::SetPacketAudioLength(int lengt_ms) {
@@ -320,6 +321,31 @@
EXPECT_EQ(0, dm_->Update(seq_no_ - 1, ts_ - kFrameSizeMs, kFs));
}
+TEST_F(DelayManagerTest, EnableRtxHandling) {
+ enable_rtx_handling_ = true;
+ RecreateDelayManager();
+
+ // Insert first packet.
+ SetPacketAudioLength(kFrameSizeMs);
+ InsertNextPacket();
+
+ // Insert reordered packet.
+ // TODO(jakobi): Test estimated inter-arrival time by mocking the histogram
+ // instead of checking the call to the peak detector.
+ EXPECT_CALL(detector_, Update(3, true, _));
+ EXPECT_EQ(0, dm_->Update(seq_no_ - 3, ts_ - 3 * kFrameSizeMs, kFs));
+
+ // Insert another reordered packet.
+ EXPECT_CALL(detector_, Update(2, true, _));
+ EXPECT_EQ(0, dm_->Update(seq_no_ - 2, ts_ - 2 * kFrameSizeMs, kFs));
+
+ // Insert the next packet in order and verify that the inter-arrival time is
+ // estimated correctly.
+ IncreaseTime(kFrameSizeMs);
+ EXPECT_CALL(detector_, Update(1, false, _));
+ InsertNextPacket();
+}
+
// Tests that skipped sequence numbers (simulating empty packets) are handled
// correctly.
TEST_F(DelayManagerTest, EmptyPacketsReported) {
diff --git a/modules/audio_coding/neteq/mock/mock_delay_manager.h b/modules/audio_coding/neteq/mock/mock_delay_manager.h
index 51c8300..67c7a8a 100644
--- a/modules/audio_coding/neteq/mock/mock_delay_manager.h
+++ b/modules/audio_coding/neteq/mock/mock_delay_manager.h
@@ -21,10 +21,12 @@
public:
MockDelayManager(size_t max_packets_in_buffer,
int base_min_target_delay_ms,
+ bool enable_rtx_handling,
DelayPeakDetector* peak_detector,
const TickTimer* tick_timer)
: DelayManager(max_packets_in_buffer,
base_min_target_delay_ms,
+ enable_rtx_handling,
peak_detector,
tick_timer) {}
virtual ~MockDelayManager() { Die(); }
diff --git a/modules/audio_coding/neteq/neteq_impl.cc b/modules/audio_coding/neteq/neteq_impl.cc
index 8b87d07..8115135 100644
--- a/modules/audio_coding/neteq/neteq_impl.cc
+++ b/modules/audio_coding/neteq/neteq_impl.cc
@@ -65,6 +65,7 @@
new DelayPeakDetector(tick_timer.get(), config.enable_rtx_handling)),
delay_manager(new DelayManager(config.max_packets_in_buffer,
config.min_delay_ms,
+ config.enable_rtx_handling,
delay_peak_detector.get(),
tick_timer.get())),
dtmf_buffer(new DtmfBuffer(config.sample_rate_hz)),
diff --git a/modules/audio_coding/neteq/neteq_impl_unittest.cc b/modules/audio_coding/neteq/neteq_impl_unittest.cc
index a970104..cf627d9 100644
--- a/modules/audio_coding/neteq/neteq_impl_unittest.cc
+++ b/modules/audio_coding/neteq/neteq_impl_unittest.cc
@@ -97,7 +97,7 @@
if (use_mock_delay_manager_) {
std::unique_ptr<MockDelayManager> mock(new MockDelayManager(
config_.max_packets_in_buffer, config_.min_delay_ms,
- delay_peak_detector_, tick_timer_));
+ config_.enable_rtx_handling, delay_peak_detector_, tick_timer_));
mock_delay_manager_ = mock.get();
EXPECT_CALL(*mock_delay_manager_, set_streaming_mode(false)).Times(1);
deps.delay_manager = std::move(mock);
diff --git a/modules/audio_coding/neteq/tools/neteq_test.cc b/modules/audio_coding/neteq/tools/neteq_test.cc
index b12a407..97e71bf 100644
--- a/modules/audio_coding/neteq/tools/neteq_test.cc
+++ b/modules/audio_coding/neteq/tools/neteq_test.cc
@@ -136,6 +136,7 @@
: -1;
*text_log_ << "Packet - wallclock: " << std::setw(5) << time_now_ms
<< ", delta wc: " << std::setw(4) << delta_wallclock
+ << ", seq_no: " << packet_data->header.sequenceNumber
<< ", timestamp: " << std::setw(10)
<< packet_data->header.timestamp
<< ", delta ts: " << std::setw(4) << delta_timestamp
