From d5f50a1b53f740b7273fc859b6c832af21daa473 Mon Sep 17 00:00:00 2001
From: "henrik.lundin" <henrik.lundin@webrtc.org>
Date: Wed, 22 Jun 2016 09:07:04 -0700
Subject: [PATCH] NetEq: Fix a bug in DelayPeakDetector causing asserts to
 trigger

In some situation, typically when incoming packets were reordered, the
DelayPeakDetector::Update method may be called twice (or more) with
non-zero inter_arrival_time argument, but without the TickTimer object
being updated in between (i.e., packets coming in more or less at the
same time). In these situations, a delay peak may be registered with
zero peak period. This could eventually trigger the DCHECK in
DelayPeakDetector::MaxPeakPeriod().

With this fix, the problem is solved by not registering peaks for which
the TickTimer object has not moved since the last peak.

The problem was originally introduced with
https://codereview.webrtc.org/1921163003.

BUG=webrtc:6021

Review-Url: https://codereview.webrtc.org/2085233002
Cr-Commit-Position: refs/heads/master@{#13257}
---
 .../audio_coding/neteq/delay_peak_detector.cc | 42 ++++++++++---------
 .../neteq/delay_peak_detector_unittest.cc     | 18 ++++++++
 2 files changed, 40 insertions(+), 20 deletions(-)

diff --git a/webrtc/modules/audio_coding/neteq/delay_peak_detector.cc b/webrtc/modules/audio_coding/neteq/delay_peak_detector.cc
index ce9133bdae..10535e23f5 100644
--- a/webrtc/modules/audio_coding/neteq/delay_peak_detector.cc
+++ b/webrtc/modules/audio_coding/neteq/delay_peak_detector.cc
@@ -77,27 +77,29 @@ bool DelayPeakDetector::Update(int inter_arrival_time, int target_level) {
     if (!peak_period_stopwatch_) {
       // This is the first peak. Reset the period counter.
       peak_period_stopwatch_ = tick_timer_->GetNewStopwatch();
-    } else if (peak_period_stopwatch_->ElapsedMs() <= kMaxPeakPeriodMs) {
-      // This is not the first peak, and the period is valid.
-      // Store peak data in the vector.
-      Peak peak_data;
-      peak_data.period_ms = peak_period_stopwatch_->ElapsedMs();
-      peak_data.peak_height_packets = inter_arrival_time;
-      peak_history_.push_back(peak_data);
-      while (peak_history_.size() > kMaxNumPeaks) {
-        // Delete the oldest data point.
-        peak_history_.pop_front();
+    } else if (peak_period_stopwatch_->ElapsedMs() > 0) {
+      if (peak_period_stopwatch_->ElapsedMs() <= kMaxPeakPeriodMs) {
+        // This is not the first peak, and the period is valid.
+        // Store peak data in the vector.
+        Peak peak_data;
+        peak_data.period_ms = peak_period_stopwatch_->ElapsedMs();
+        peak_data.peak_height_packets = inter_arrival_time;
+        peak_history_.push_back(peak_data);
+        while (peak_history_.size() > kMaxNumPeaks) {
+          // Delete the oldest data point.
+          peak_history_.pop_front();
+        }
+        peak_period_stopwatch_ = tick_timer_->GetNewStopwatch();
+      } else if (peak_period_stopwatch_->ElapsedMs() <= 2 * kMaxPeakPeriodMs) {
+        // Invalid peak due to too long period. Reset period counter and start
+        // looking for next peak.
+        peak_period_stopwatch_ = tick_timer_->GetNewStopwatch();
+      } else {
+        // More than 2 times the maximum period has elapsed since the last peak
+        // was registered. It seams that the network conditions have changed.
+        // Reset the peak statistics.
+        Reset();
       }
-      peak_period_stopwatch_ = tick_timer_->GetNewStopwatch();
-    } else if (peak_period_stopwatch_->ElapsedMs() <= 2 * kMaxPeakPeriodMs) {
-      // Invalid peak due to too long period. Reset period counter and start
-      // looking for next peak.
-      peak_period_stopwatch_ = tick_timer_->GetNewStopwatch();
-    } else {
-      // More than 2 times the maximum period has elapsed since the last peak
-      // was registered. It seams that the network conditions have changed.
-      // Reset the peak statistics.
-      Reset();
     }
   }
   return CheckPeakConditions();
diff --git a/webrtc/modules/audio_coding/neteq/delay_peak_detector_unittest.cc b/webrtc/modules/audio_coding/neteq/delay_peak_detector_unittest.cc
index 32b36b25ef..71a86ff013 100644
--- a/webrtc/modules/audio_coding/neteq/delay_peak_detector_unittest.cc
+++ b/webrtc/modules/audio_coding/neteq/delay_peak_detector_unittest.cc
@@ -122,4 +122,22 @@ TEST(DelayPeakDetector, DoNotTriggerPeakMode) {
     time += 10;  // Increase time 10 ms.
   }
 }
+
+// In situations with reordered packets, the DelayPeakDetector may be updated
+// back-to-back (i.e., without the tick_timer moving) but still with non-zero
+// inter-arrival time. This test is to make sure that this does not cause
+// problems.
+TEST(DelayPeakDetector, ZeroDistancePeaks) {
+  TickTimer tick_timer;
+  DelayPeakDetector detector(&tick_timer);
+  const int kPacketSizeMs = 30;
+  detector.SetPacketAudioLength(kPacketSizeMs);
+
+  const int kTargetBufferLevel = 2;  // Define peaks to be iat > 4.
+  const int kInterArrivalTime = 3 * kTargetBufferLevel;  // Will trigger a peak.
+  EXPECT_FALSE(detector.Update(kInterArrivalTime, kTargetBufferLevel));
+  EXPECT_FALSE(detector.Update(kInterArrivalTime, kTargetBufferLevel));
+  EXPECT_FALSE(detector.Update(kInterArrivalTime, kTargetBufferLevel));
+}
+
 }  // namespace webrtc