Fix issues with incorrect wrap checks when having big buffers and high bitrate.

Introduces shared functions for timestamp and sequence number wrap checks.

BUG=1607
TESTS=trybots

Review URL: https://webrtc-codereview.appspot.com/1291005

git-svn-id: http://webrtc.googlecode.com/svn/trunk@3833 4adac7df-926f-26a2-2b94-8c16560cd09d

webrtc/modules/interface/module_common_types.h

@@ -1044,6 +1044,28 @@ AudioFrame::operator-=(const AudioFrame& rhs)
     return *this;
 }
 
+inline bool IsNewerSequenceNumber(uint16_t sequence_number,
+                                  uint16_t prev_sequence_number) {
+  return sequence_number != prev_sequence_number &&
+      static_cast<uint16_t>(sequence_number - prev_sequence_number) < 0x8000;
+}
+
+inline bool IsNewerTimestamp(uint32_t timestamp, uint32_t prev_timestamp) {
+  return timestamp != prev_timestamp &&
+      static_cast<uint32_t>(timestamp - prev_timestamp) < 0x80000000;
+}
+
+inline uint16_t LatestSequenceNumber(uint16_t sequence_number1,
+                                     uint16_t sequence_number2) {
+  return IsNewerSequenceNumber(sequence_number1, sequence_number2) ?
+      sequence_number1 : sequence_number2;
+}
+
+inline uint32_t LatestTimestamp(uint32_t timestamp1, uint32_t timestamp2) {
+  return IsNewerTimestamp(timestamp1, timestamp2) ? timestamp1 :
+      timestamp2;
+}
+
 } // namespace webrtc
 
 #endif // MODULE_COMMON_TYPES_H

webrtc/modules/module_common_types_unittest.cc

@@ -0,0 +1,104 @@
+/*
+ *  Copyright (c) 2013 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#include "webrtc/modules/interface/module_common_types.h"
+
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace webrtc {
+
+TEST(IsNewerSequenceNumber, Equal) {
+  EXPECT_FALSE(IsNewerSequenceNumber(0x0001, 0x0001));
+}
+
+TEST(IsNewerSequenceNumber, NoWrap) {
+  EXPECT_TRUE(IsNewerSequenceNumber(0xFFFF, 0xFFFE));
+  EXPECT_TRUE(IsNewerSequenceNumber(0x0001, 0x0000));
+  EXPECT_TRUE(IsNewerSequenceNumber(0x0100, 0x00FF));
+}
+
+TEST(IsNewerSequenceNumber, ForwardWrap) {
+  EXPECT_TRUE(IsNewerSequenceNumber(0x0000, 0xFFFF));
+  EXPECT_TRUE(IsNewerSequenceNumber(0x0000, 0xFF00));
+  EXPECT_TRUE(IsNewerSequenceNumber(0x00FF, 0xFFFF));
+  EXPECT_TRUE(IsNewerSequenceNumber(0x00FF, 0xFF00));
+}
+
+TEST(IsNewerSequenceNumber, BackwardWrap) {
+  EXPECT_FALSE(IsNewerSequenceNumber(0xFFFF, 0x0000));
+  EXPECT_FALSE(IsNewerSequenceNumber(0xFF00, 0x0000));
+  EXPECT_FALSE(IsNewerSequenceNumber(0xFFFF, 0x00FF));
+  EXPECT_FALSE(IsNewerSequenceNumber(0xFF00, 0x00FF));
+}
+
+TEST(IsNewerTimestamp, Equal) {
+  EXPECT_FALSE(IsNewerTimestamp(0x00000001, 0x000000001));
+}
+
+TEST(IsNewerTimestamp, NoWrap) {
+  EXPECT_TRUE(IsNewerTimestamp(0xFFFFFFFF, 0xFFFFFFFE));
+  EXPECT_TRUE(IsNewerTimestamp(0x00000001, 0x00000000));
+  EXPECT_TRUE(IsNewerTimestamp(0x00010000, 0x0000FFFF));
+}
+
+TEST(IsNewerTimestamp, ForwardWrap) {
+  EXPECT_TRUE(IsNewerTimestamp(0x00000000, 0xFFFFFFFF));
+  EXPECT_TRUE(IsNewerTimestamp(0x00000000, 0xFFFF0000));
+  EXPECT_TRUE(IsNewerTimestamp(0x0000FFFF, 0xFFFFFFFF));
+  EXPECT_TRUE(IsNewerTimestamp(0x0000FFFF, 0xFFFF0000));
+}
+
+TEST(IsNewerTimestamp, BackwardWrap) {
+  EXPECT_FALSE(IsNewerTimestamp(0xFFFFFFFF, 0x00000000));
+  EXPECT_FALSE(IsNewerTimestamp(0xFFFF0000, 0x00000000));
+  EXPECT_FALSE(IsNewerTimestamp(0xFFFFFFFF, 0x0000FFFF));
+  EXPECT_FALSE(IsNewerTimestamp(0xFFFF0000, 0x0000FFFF));
+}
+
+TEST(LatestSequenceNumber, NoWrap) {
+  EXPECT_EQ(0xFFFFu, LatestSequenceNumber(0xFFFF, 0xFFFE));
+  EXPECT_EQ(0x0001u, LatestSequenceNumber(0x0001, 0x0000));
+  EXPECT_EQ(0x0100u, LatestSequenceNumber(0x0100, 0x00FF));
+
+  EXPECT_EQ(0xFFFFu, LatestSequenceNumber(0xFFFE, 0xFFFF));
+  EXPECT_EQ(0x0001u, LatestSequenceNumber(0x0000, 0x0001));
+  EXPECT_EQ(0x0100u, LatestSequenceNumber(0x00FF, 0x0100));
+}
+
+TEST(LatestSequenceNumber, Wrap) {
+  EXPECT_EQ(0x0000u, LatestSequenceNumber(0x0000, 0xFFFF));
+  EXPECT_EQ(0x0000u, LatestSequenceNumber(0x0000, 0xFF00));
+  EXPECT_EQ(0x00FFu, LatestSequenceNumber(0x00FF, 0xFFFF));
+  EXPECT_EQ(0x00FFu, LatestSequenceNumber(0x00FF, 0xFF00));
+
+  EXPECT_EQ(0x0000u, LatestSequenceNumber(0xFFFF, 0x0000));
+  EXPECT_EQ(0x0000u, LatestSequenceNumber(0xFF00, 0x0000));
+  EXPECT_EQ(0x00FFu, LatestSequenceNumber(0xFFFF, 0x00FF));
+  EXPECT_EQ(0x00FFu, LatestSequenceNumber(0xFF00, 0x00FF));
+}
+
+TEST(LatestTimestamp, NoWrap) {
+  EXPECT_EQ(0xFFFFFFFFu, LatestTimestamp(0xFFFFFFFF, 0xFFFFFFFE));
+  EXPECT_EQ(0x00000001u, LatestTimestamp(0x00000001, 0x00000000));
+  EXPECT_EQ(0x00010000u, LatestTimestamp(0x00010000, 0x0000FFFF));
+}
+
+TEST(LatestTimestamp, Wrap) {
+  EXPECT_EQ(0x00000000u, LatestTimestamp(0x00000000, 0xFFFFFFFF));
+  EXPECT_EQ(0x00000000u, LatestTimestamp(0x00000000, 0xFFFF0000));
+  EXPECT_EQ(0x0000FFFFu, LatestTimestamp(0x0000FFFF, 0xFFFFFFFF));
+  EXPECT_EQ(0x0000FFFFu, LatestTimestamp(0x0000FFFF, 0xFFFF0000));
+
+  EXPECT_EQ(0x00000000u, LatestTimestamp(0xFFFFFFFF, 0x00000000));
+  EXPECT_EQ(0x00000000u, LatestTimestamp(0xFFFF0000, 0x00000000));
+  EXPECT_EQ(0x0000FFFFu, LatestTimestamp(0xFFFFFFFF, 0x0000FFFF));
+  EXPECT_EQ(0x0000FFFFu, LatestTimestamp(0xFFFF0000, 0x0000FFFF));
+}
+}  // namespace webrtc

webrtc/modules/modules.gyp

@@ -50,6 +50,19 @@
         'video_coding/codecs/tools/video_codecs_tools.gypi',
         'video_processing/main/test/vpm_tests.gypi',
       ], # includes
+      'targets': [
+        {
+          'target_name': 'modules_unittests',
+          'type': 'executable',
+          'dependencies': [
+            '<(DEPTH)/testing/gtest.gyp:gtest',
+            '<(webrtc_root)/test/test.gyp:test_support_main',
+          ],
+          'sources': [
+            'module_common_types_unittest.cc',
+          ],
+        },
+      ],
     }], # include_tests
   ], # conditions
 }

webrtc/modules/rtp_rtcp/source/forward_error_correction.cc

@@ -62,8 +62,7 @@ class FecPacket : public ForwardErrorCorrection::SortablePacket {
 bool ForwardErrorCorrection::SortablePacket::LessThan(
     const SortablePacket* first,
     const SortablePacket* second) {
-  return (first->seqNum != second->seqNum &&
+  return IsNewerSequenceNumber(second->seqNum, first->seqNum);
-      LatestSequenceNumber(first->seqNum, second->seqNum) == second->seqNum);
 }
 
 ForwardErrorCorrection::ForwardErrorCorrection(int32_t id)
@@ -826,19 +825,4 @@ int32_t ForwardErrorCorrection::DecodeFEC(
 uint16_t ForwardErrorCorrection::PacketOverhead() {
   return kFecHeaderSize + kUlpHeaderSizeLBitSet;
 }
-
-uint16_t ForwardErrorCorrection::LatestSequenceNumber(uint16_t first,
-                                                      uint16_t second) {
-  bool wrap = (first < 0x00ff && second > 0xff00) ||
-          (first > 0xff00 && second < 0x00ff);
-  if (second > first && !wrap)
-    return second;
-  else if (second <= first && !wrap)
-    return first;
-  else if (second < first && wrap)
-    return second;
-  else
-    return first;
-}
-
 } // namespace webrtc

webrtc/modules/rtp_rtcp/source/forward_error_correction.h

@@ -326,9 +326,6 @@ class ForwardErrorCorrection {
   // This function returns 2 when two or more packets are missing.
   static int NumCoveredPacketsMissing(const FecPacket* fec_packet);
 
-  static uint16_t LatestSequenceNumber(uint16_t first,
-                                       uint16_t second);
-
   static void DiscardFECPacket(FecPacket* fec_packet);
   static void DiscardOldPackets(RecoveredPacketList* recoveredPacketList);
   static uint16_t ParseSequenceNumber(uint8_t* packet);

webrtc/modules/rtp_rtcp/source/rtp_receiver.cc

@@ -585,28 +585,13 @@ bool RTPReceiver::RetransmitOfOldPacket(
 }
 
 bool RTPReceiver::InOrderPacket(const uint16_t sequence_number) const {
-  if (received_seq_max_ >= sequence_number) {
+  if (IsNewerSequenceNumber(sequence_number, received_seq_max_)) {
-    // Detect wrap-around.
+    return true;
-    if (!(received_seq_max_ > 0xff00 && sequence_number < 0x0ff)) {
-      if (received_seq_max_ - max_reordering_threshold_ > sequence_number) {
-        // We have a restart of the remote side.
-      } else {
-        // we received a retransmit of a packet we already have.
-        return false;
-      }
-    }
   } else {
-    // Detect wrap-around.
+    // If we have a restart of the remote side this packet is still in order.
-    if (sequence_number > 0xff00 && received_seq_max_ < 0x0ff) {
+    return !IsNewerSequenceNumber(sequence_number, received_seq_max_ -
-      if (received_seq_max_ - max_reordering_threshold_ > sequence_number) {
+                                  max_reordering_threshold_);
-        // We have a restart of the remote side
-      } else {
-        // We received a retransmit of a packet we already have
-        return false;
-      }
-    }
   }
-  return true;
 }
 
 uint16_t RTPReceiver::SequenceNumber() const {

webrtc/modules/rtp_rtcp/source/rtp_utility.cc

@@ -82,24 +82,6 @@ uint32_t ConvertNTPTimeToMS(uint32_t NTPsec, uint32_t NTPfrac) {
   return MStime;
 }
 
-bool OldTimestamp(uint32_t newTimestamp,
-                  uint32_t existingTimestamp,
-                  bool* wrapped) {
-  bool tmpWrapped =
-    (newTimestamp < 0x0000ffff && existingTimestamp > 0xffff0000) ||
-    (newTimestamp > 0xffff0000 && existingTimestamp < 0x0000ffff);
-  *wrapped = tmpWrapped;
-  if (existingTimestamp > newTimestamp && !tmpWrapped) {
-    return true;
-  } else if (existingTimestamp <= newTimestamp && !tmpWrapped) {
-    return false;
-  } else if (existingTimestamp < newTimestamp && tmpWrapped) {
-    return true;
-  } else {
-    return false;
-  }
-}
-
 /*
  * Misc utility routines
  */

webrtc/modules/video_coding/main/source/decoding_state.cc

@@ -51,16 +51,14 @@ bool VCMDecodingState::IsOldFrame(const VCMFrameBuffer* frame) const {
   assert(frame != NULL);
   if (in_initial_state_)
     return false;
-  return (LatestTimestamp(time_stamp_, frame->TimeStamp(), NULL)
+  return !IsNewerTimestamp(frame->TimeStamp(), time_stamp_);
-          == time_stamp_);
 }
 
 bool VCMDecodingState::IsOldPacket(const VCMPacket* packet) const {
   assert(packet != NULL);
   if (in_initial_state_)
     return false;
-  return (LatestTimestamp(time_stamp_, packet->timestamp, NULL)
+  return !IsNewerTimestamp(packet->timestamp, time_stamp_);
-           == time_stamp_);
 }
 
 void VCMDecodingState::SetState(const VCMFrameBuffer* frame) {
@@ -106,7 +104,7 @@ void VCMDecodingState::UpdateOldPacket(const VCMPacket* packet) {
   if (packet->timestamp == time_stamp_) {
     // Late packet belonging to the last decoded frame - make sure we update the
     // last decoded sequence number.
-    sequence_num_ = LatestSequenceNumber(packet->seqNum, sequence_num_, NULL);
+    sequence_num_ = LatestSequenceNumber(packet->seqNum, sequence_num_);
   }
 }
 

webrtc/modules/video_coding/main/source/jitter_buffer.cc

@@ -36,8 +36,7 @@ class FrameSmallerTimestamp {
  public:
   explicit FrameSmallerTimestamp(uint32_t timestamp) : timestamp_(timestamp) {}
   bool operator()(VCMFrameBuffer* frame) {
-    return (LatestTimestamp(timestamp_, frame->TimeStamp(), NULL) ==
+    return IsNewerTimestamp(timestamp_, frame->TimeStamp());
-            timestamp_);
   }
 
  private:
@@ -718,7 +717,7 @@ VCMFrameBufferEnum VCMJitterBuffer::InsertPacket(VCMEncodedFrame* encoded_frame,
       request_key_frame = true;
     }
     latest_received_sequence_number_ = LatestSequenceNumber(
-        latest_received_sequence_number_, packet.seqNum, NULL);
+        latest_received_sequence_number_, packet.seqNum);
   }
 
   // Empty packets may bias the jitter estimate (lacking size component),
@@ -909,12 +908,10 @@ bool VCMJitterBuffer::UpdateNackList(uint16_t sequence_number) {
   if (!last_decoded_state_.in_initial_state()) {
     latest_received_sequence_number_ = LatestSequenceNumber(
         latest_received_sequence_number_,
-        last_decoded_state_.sequence_num(),
+        last_decoded_state_.sequence_num());
-        NULL);
   }
-  bool in_order = LatestSequenceNumber(sequence_number,
+  if (IsNewerSequenceNumber(sequence_number,
-      latest_received_sequence_number_, NULL) == sequence_number;
+                            latest_received_sequence_number_)) {
-  if (in_order) {
     // Push any missing sequence numbers to the NACK list.
     for (uint16_t i = latest_received_sequence_number_ + 1;
         i < sequence_number; ++i) {

webrtc/modules/video_coding/main/source/jitter_buffer.h

@@ -168,10 +168,7 @@ class VCMJitterBuffer {
    public:
     bool operator() (const uint16_t& sequence_number1,
                      const uint16_t& sequence_number2) const {
-      if (sequence_number1 == sequence_number2)
+      return IsNewerSequenceNumber(sequence_number2, sequence_number1);
-        return false;
-      return LatestSequenceNumber(sequence_number1, sequence_number2, NULL) ==
-          sequence_number2;
     }
   };
   typedef std::set<uint16_t, SequenceNumberLessThan> SequenceNumberSet;

webrtc/modules/video_coding/main/source/jitter_buffer_common.cc

@@ -1,60 +0,0 @@
-/*
- *  Copyright (c) 2011 The WebRTC project authors. All Rights Reserved.
- *
- *  Use of this source code is governed by a BSD-style license
- *  that can be found in the LICENSE file in the root of the source
- *  tree. An additional intellectual property rights grant can be found
- *  in the file PATENTS.  All contributing project authors may
- *  be found in the AUTHORS file in the root of the source tree.
- */
-
-#include "jitter_buffer_common.h"
-
-#include <cstdlib>
-
-namespace webrtc {
-
-uint32_t LatestTimestamp(uint32_t timestamp1,
-                               uint32_t timestamp2,
-                               bool* has_wrapped) {
-  bool wrap = (timestamp2 < 0x0000ffff && timestamp1 > 0xffff0000) ||
-      (timestamp2 > 0xffff0000 && timestamp1 < 0x0000ffff);
-  if (has_wrapped != NULL)
-    *has_wrapped = wrap;
-  if (timestamp1 > timestamp2 && !wrap)
-      return timestamp1;
-  else if (timestamp1 <= timestamp2 && !wrap)
-      return timestamp2;
-  else if (timestamp1 < timestamp2 && wrap)
-      return timestamp1;
-  else
-      return timestamp2;
-}
-
-int32_t LatestSequenceNumber(int32_t seq_num1,
-                                   int32_t seq_num2,
-                                   bool* has_wrapped) {
-  if (seq_num1 < 0 && seq_num2 < 0)
-    return -1;
-  else if (seq_num1 < 0)
-    return seq_num2;
-  else if (seq_num2 < 0)
-    return seq_num1;
-
-  bool wrap = (seq_num1 < 0x00ff && seq_num2 > 0xff00) ||
-          (seq_num1 > 0xff00 && seq_num2 < 0x00ff);
-
-  if (has_wrapped != NULL)
-    *has_wrapped = wrap;
-
-  if (seq_num2 > seq_num1 && !wrap)
-    return seq_num2;
-  else if (seq_num2 <= seq_num1 && !wrap)
-    return seq_num1;
-  else if (seq_num2 < seq_num1 && wrap)
-    return seq_num2;
-  else
-    return seq_num1;
-}
-
-}  // namespace webrtc

webrtc/modules/video_coding/main/source/jitter_buffer_common.h

@@ -59,19 +59,6 @@ enum VCMNaluCompleteness {
   kNaluIncomplete,      // Packet is not beginning or end of NALU
   kNaluEnd,             // Packet is the end of a NALU
 };
-
-// Returns the latest of the two timestamps, compensating for wrap arounds.
-// This function assumes that the two timestamps are close in time.
-uint32_t LatestTimestamp(uint32_t timestamp1,
-                               uint32_t timestamp2,
-                               bool* has_wrapped);
-
-// Returns the latest of the two sequence numbers, compensating for wrap
-// arounds. This function assumes that the two sequence numbers are close in
-// time.
-int32_t LatestSequenceNumber(int32_t seq_num1,
-                                   int32_t seq_num2,
-                                   bool* has_wrapped);
 }  // namespace webrtc
 
 #endif  // WEBRTC_MODULES_VIDEO_CODING_JITTER_BUFFER_COMMON_H_

webrtc/modules/video_coding/main/source/session_info.cc

@@ -44,8 +44,9 @@ int VCMSessionInfo::LowSequenceNumber() const {
 int VCMSessionInfo::HighSequenceNumber() const {
   if (packets_.empty())
     return empty_seq_num_high_;
-  return LatestSequenceNumber(packets_.back().seqNum, empty_seq_num_high_,
+  if (empty_seq_num_high_ == -1)
-                              NULL);
+    return packets_.back().seqNum;
+  return LatestSequenceNumber(packets_.back().seqNum, empty_seq_num_high_);
 }
 
 int VCMSessionInfo::PictureId() const {
@@ -388,8 +389,7 @@ int VCMSessionInfo::InsertPacket(const VCMPacket& packet,
   // order and insert it. Loop over the list in reverse order.
   ReversePacketIterator rit = packets_.rbegin();
   for (; rit != packets_.rend(); ++rit)
-    if (LatestSequenceNumber((*rit).seqNum, packet.seqNum, NULL) ==
+    if (LatestSequenceNumber(packet.seqNum, (*rit).seqNum) == packet.seqNum)
-        packet.seqNum)
       break;
 
   // Check for duplicate packets.
@@ -412,11 +412,12 @@ void VCMSessionInfo::InformOfEmptyPacket(uint16_t seq_num) {
   // follow the data packets, therefore, we should only keep track of the high
   // and low sequence numbers and may assume that the packets in between are
   // empty packets belonging to the same frame (timestamp).
-  empty_seq_num_high_ = LatestSequenceNumber(seq_num, empty_seq_num_high_,
+  if (empty_seq_num_high_ == -1)
-                                             NULL);
+    empty_seq_num_high_ = seq_num;
-  if (empty_seq_num_low_ == -1 ||
+  else
-      LatestSequenceNumber(seq_num, empty_seq_num_low_, NULL) ==
+    empty_seq_num_high_ = LatestSequenceNumber(seq_num, empty_seq_num_high_);
-          empty_seq_num_low_)
+  if (empty_seq_num_low_ == -1 || IsNewerSequenceNumber(empty_seq_num_low_,
+                                                        seq_num))
     empty_seq_num_low_ = seq_num;
 }
 

webrtc/modules/video_coding/main/source/video_coding.gypi

@@ -76,7 +76,6 @@
         'generic_encoder.cc',
         'inter_frame_delay.cc',
         'jitter_buffer.cc',
-        'jitter_buffer_common.cc',
         'jitter_estimator.cc',
         'media_opt_util.cc',
         'media_optimization.cc',