Only advance |first_seq_num_| if packets are explicitly cleared from the PacketBuffer.

In this CL:
 - Don't insert a packet if we have explicitly cleared past it.
 - Added some logging to ExpandBufferSize.
 - Renamed IsContinuous to PotentialNewFrame.
 - Unittests updated/added for this new behavior.
 - Refactored TestPacketBuffer unittests.

BUG=webrtc:5514
R=danilchap@webrtc.org, stefan@webrtc.org

Review URL: https://codereview.webrtc.org/2399373002 .

Cr-Commit-Position: refs/heads/master@{#14871}

third_party/drmemory

@@ -0,0 +1 @@
+../chromium/src/third_party/drmemory

webrtc/modules/video_coding/packet_buffer.cc

@@ -42,6 +42,7 @@ PacketBuffer::PacketBuffer(Clock* clock,
       first_seq_num_(0),
       last_seq_num_(0),
       first_packet_received_(false),
+      is_cleared_to_first_seq_num_(false),
       data_buffer_(start_buffer_size),
       sequence_buffer_(start_buffer_size),
       received_frame_callback_(received_frame_callback) {
@@ -51,7 +52,9 @@ PacketBuffer::PacketBuffer(Clock* clock,
   RTC_DCHECK((max_buffer_size & (max_buffer_size - 1)) == 0);
 }
 
-PacketBuffer::~PacketBuffer() {}
+PacketBuffer::~PacketBuffer() {
+  Clear();
+}
 
 bool PacketBuffer::InsertPacket(const VCMPacket& packet) {
   rtc::CritScope lock(&crit_);
@@ -59,9 +62,16 @@ bool PacketBuffer::InsertPacket(const VCMPacket& packet) {
   size_t index = seq_num % size_;
 
   if (!first_packet_received_) {
-    first_seq_num_ = seq_num - 1;
+    first_seq_num_ = seq_num;
     last_seq_num_ = seq_num;
     first_packet_received_ = true;
+  } else if (AheadOf(first_seq_num_, seq_num)) {
+    // If we have explicitly cleared past this packet then it's old,
+    // don't insert it.
+    if (is_cleared_to_first_seq_num_)
+      return false;
+
+    first_seq_num_ = seq_num;
   }
 
   if (sequence_buffer_[index].used) {
@@ -106,19 +116,39 @@ bool PacketBuffer::InsertPacket(const VCMPacket& packet) {
 
 void PacketBuffer::ClearTo(uint16_t seq_num) {
   rtc::CritScope lock(&crit_);
+
+  // If the packet buffer was cleared between a frame was created and returned.
+  if (!first_packet_received_)
+    return;
+
+  is_cleared_to_first_seq_num_ = true;
+  while (AheadOrAt<uint16_t>(seq_num, first_seq_num_)) {
     size_t index = first_seq_num_ % size_;
-  while (AheadOf<uint16_t>(seq_num, first_seq_num_ + 1)) {
-    index = (index + 1) % size_;
-    ++first_seq_num_;
     delete[] data_buffer_[index].dataPtr;
     data_buffer_[index].dataPtr = nullptr;
     sequence_buffer_[index].used = false;
+    ++first_seq_num_;
   }
 }
 
+void PacketBuffer::Clear() {
+  rtc::CritScope lock(&crit_);
+  for (size_t i = 0; i < size_; ++i) {
+    delete[] data_buffer_[i].dataPtr;
+    data_buffer_[i].dataPtr = nullptr;
+    sequence_buffer_[i].used = false;
+  }
+
+  first_packet_received_ = false;
+  is_cleared_to_first_seq_num_ = false;
+}
+
 bool PacketBuffer::ExpandBufferSize() {
-  if (size_ == max_size_)
+  if (size_ == max_size_) {
+    LOG(LS_WARNING) << "PacketBuffer is already at max size (" << max_size_
+                    << "), failed to increase size.";
     return false;
+  }
 
   size_t new_size = std::min(max_size_, 2 * size_);
   std::vector<VCMPacket> new_data_buffer(new_size);
@@ -133,10 +163,11 @@ bool PacketBuffer::ExpandBufferSize() {
   size_ = new_size;
   sequence_buffer_ = std::move(new_sequence_buffer);
   data_buffer_ = std::move(new_data_buffer);
+  LOG(LS_INFO) << "PacketBuffer size expanded to " << new_size;
   return true;
 }
 
-bool PacketBuffer::IsContinuous(uint16_t seq_num) const {
+bool PacketBuffer::PotentialNewFrame(uint16_t seq_num) const {
   size_t index = seq_num % size_;
   int prev_index = index > 0 ? index - 1 : size_ - 1;
 
@@ -144,13 +175,22 @@ bool PacketBuffer::IsContinuous(uint16_t seq_num) const {
     return false;
   if (sequence_buffer_[index].frame_created)
     return false;
-  if (sequence_buffer_[index].frame_begin)
+  if (sequence_buffer_[index].frame_begin &&
+      (!sequence_buffer_[prev_index].used ||
+       AheadOf(seq_num, sequence_buffer_[prev_index].seq_num))) {
+    // The reason we only return true if this packet is the first packet of the
+    // frame and the sequence number is newer than the packet with the previous
+    // index is because we want to avoid an inifite loop in the case where
+    // a single frame containing more packets than the current size of the
+    // packet buffer is inserted.
     return true;
+  }
   if (!sequence_buffer_[prev_index].used)
     return false;
   if (sequence_buffer_[prev_index].seq_num !=
-      static_cast<uint16_t>(seq_num - 1))
+      sequence_buffer_[index].seq_num - 1) {
     return false;
+  }
   if (sequence_buffer_[prev_index].continuous)
     return true;
 
@@ -158,8 +198,8 @@ bool PacketBuffer::IsContinuous(uint16_t seq_num) const {
 }
 
 void PacketBuffer::FindFrames(uint16_t seq_num) {
+  while (PotentialNewFrame(seq_num)) {
     size_t index = seq_num % size_;
-  while (IsContinuous(seq_num)) {
     sequence_buffer_[index].continuous = true;
 
     // If all packets of the frame is continuous, find the first packet of the
@@ -192,7 +232,6 @@ void PacketBuffer::FindFrames(uint16_t seq_num) {
       received_frame_callback_->OnReceivedFrame(std::move(frame));
     }
 
-    index = (index + 1) % size_;
     ++seq_num;
   }
 }
@@ -212,13 +251,6 @@ void PacketBuffer::ReturnFrame(RtpFrameObject* frame) {
     index = (index + 1) % size_;
     ++seq_num;
   }
-
-  index = first_seq_num_ % size_;
-  while (AheadOf<uint16_t>(last_seq_num_, first_seq_num_) &&
-         !sequence_buffer_[index].used) {
-    ++first_seq_num_;
-    index = (index + 1) % size_;
-  }
 }
 
 bool PacketBuffer::GetBitstream(const RtpFrameObject& frame,
@@ -254,14 +286,6 @@ VCMPacket* PacketBuffer::GetPacket(uint16_t seq_num) {
   return &data_buffer_[index];
 }
 
-void PacketBuffer::Clear() {
-  rtc::CritScope lock(&crit_);
-  for (size_t i = 0; i < size_; ++i)
-    sequence_buffer_[i].used = false;
-
-  first_packet_received_ = false;
-}
-
 int PacketBuffer::AddRef() const {
   return rtc::AtomicOps::Increment(&ref_count_);
 }

webrtc/modules/video_coding/packet_buffer.h

@@ -48,7 +48,8 @@ class PacketBuffer {
 
   virtual ~PacketBuffer();
 
-  // Made virtual for testing.
+  // Returns true if |packet| is inserted into the packet buffer,
+  // false otherwise. Made virtual for testing.
   virtual bool InsertPacket(const VCMPacket& packet);
   void ClearTo(uint16_t seq_num);
   void Clear();
@@ -94,7 +95,8 @@ class PacketBuffer {
   bool ExpandBufferSize() EXCLUSIVE_LOCKS_REQUIRED(crit_);
 
   // Test if all previous packets has arrived for the given sequence number.
-  bool IsContinuous(uint16_t seq_num) const EXCLUSIVE_LOCKS_REQUIRED(crit_);
+  bool PotentialNewFrame(uint16_t seq_num) const
+      EXCLUSIVE_LOCKS_REQUIRED(crit_);
 
   // Test if all packets of a frame has arrived, and if so, creates a frame.
   // May create multiple frames per invocation.
@@ -127,6 +129,9 @@ class PacketBuffer {
   // If the packet buffer has received its first packet.
   bool first_packet_received_ GUARDED_BY(crit_);
 
+  // If the buffer is cleared to |first_seq_num_|.
+  bool is_cleared_to_first_seq_num_ GUARDED_BY(crit_);
+
   // Buffer that holds the inserted packets.
   std::vector<VCMPacket> data_buffer_ GUARDED_BY(crit_);
 

webrtc/modules/video_coding/video_packet_buffer_unittest.cc

@@ -49,7 +49,7 @@ class TestPacketBuffer : public ::testing::Test,
   enum IsFirst { kFirst, kNotFirst };
   enum IsLast { kLast, kNotLast };
 
-  void InsertPacket(uint16_t seq_num,           // packet sequence number
+  bool Insert(uint16_t seq_num,           // packet sequence number
               IsKeyFrame keyframe,        // is keyframe
               IsFirst first,              // is first packet of frame
               IsLast last,                // is last packet of frame
@@ -64,7 +64,7 @@ class TestPacketBuffer : public ::testing::Test,
     packet.sizeBytes = data_size;
     packet.dataPtr = data;
 
-    EXPECT_TRUE(packet_buffer_->InsertPacket(packet));
+    return packet_buffer_->InsertPacket(packet);
   }
 
   void CheckFrame(uint16_t first_seq_num) {
@@ -84,26 +84,55 @@ class TestPacketBuffer : public ::testing::Test,
 };
 
 TEST_F(TestPacketBuffer, InsertOnePacket) {
-  uint16_t seq_num = Rand();
+  const uint16_t seq_num = Rand();
-  InsertPacket(seq_num, kKeyFrame, kFirst, kLast);
+  EXPECT_TRUE(Insert(seq_num, kKeyFrame, kFirst, kLast));
 }
 
 TEST_F(TestPacketBuffer, InsertMultiplePackets) {
-  uint16_t seq_num = Rand();
+  const uint16_t seq_num = Rand();
-  InsertPacket(seq_num, kKeyFrame, kFirst, kLast);
+  EXPECT_TRUE(Insert(seq_num, kKeyFrame, kFirst, kLast));
-  InsertPacket(seq_num + 1, kKeyFrame, kFirst, kLast);
+  EXPECT_TRUE(Insert(seq_num + 1, kKeyFrame, kFirst, kLast));
-  InsertPacket(seq_num + 2, kKeyFrame, kFirst, kLast);
+  EXPECT_TRUE(Insert(seq_num + 2, kKeyFrame, kFirst, kLast));
-  InsertPacket(seq_num + 3, kKeyFrame, kFirst, kLast);
+  EXPECT_TRUE(Insert(seq_num + 3, kKeyFrame, kFirst, kLast));
 }
 
 TEST_F(TestPacketBuffer, InsertDuplicatePacket) {
-  uint16_t seq_num = Rand();
+  const uint16_t seq_num = Rand();
-  InsertPacket(seq_num, kKeyFrame, kFirst, kLast);
+  EXPECT_TRUE(Insert(seq_num, kKeyFrame, kFirst, kLast));
-  InsertPacket(seq_num, kKeyFrame, kFirst, kLast);
+  EXPECT_TRUE(Insert(seq_num, kKeyFrame, kFirst, kLast));
+}
+
+TEST_F(TestPacketBuffer, SeqNumWrap) {
+  EXPECT_TRUE(Insert(0xFFFF, kKeyFrame, kFirst, kLast));
+  EXPECT_TRUE(Insert(0x0, kKeyFrame, kFirst, kLast));
+
+  CheckFrame(0xFFFF);
+}
+
+TEST_F(TestPacketBuffer, InsertOldPackets) {
+  const uint16_t seq_num = Rand();
+
+  EXPECT_TRUE(Insert(seq_num, kKeyFrame, kFirst, kNotLast));
+  EXPECT_TRUE(Insert(seq_num + 2, kDeltaFrame, kFirst, kLast));
+  EXPECT_TRUE(Insert(seq_num + 1, kKeyFrame, kNotFirst, kLast));
+  ASSERT_EQ(2UL, frames_from_callback_.size());
+
+  frames_from_callback_.erase(seq_num + 2);
+  EXPECT_TRUE(Insert(seq_num, kKeyFrame, kFirst, kNotLast));
+  ASSERT_EQ(1UL, frames_from_callback_.size());
+
+  frames_from_callback_.erase(frames_from_callback_.find(seq_num));
+  ASSERT_TRUE(Insert(seq_num, kKeyFrame, kFirst, kNotLast));
+  EXPECT_TRUE(Insert(seq_num + 2, kDeltaFrame, kFirst, kLast));
+
+  packet_buffer_->ClearTo(seq_num + 2);
+  EXPECT_FALSE(Insert(seq_num + 2, kDeltaFrame, kFirst, kLast));
+  EXPECT_TRUE(Insert(seq_num + 3, kDeltaFrame, kFirst, kLast));
+  ASSERT_EQ(2UL, frames_from_callback_.size());
 }
 
 TEST_F(TestPacketBuffer, NackCount) {
-  uint16_t seq_num = Rand();
+  const uint16_t seq_num = Rand();
 
   VCMPacket packet;
   packet.codec = kVideoCodecGeneric;
@@ -129,57 +158,64 @@ TEST_F(TestPacketBuffer, NackCount) {
   packet.timesNacked = 1;
   packet_buffer_->InsertPacket(packet);
 
-
   ASSERT_EQ(1UL, frames_from_callback_.size());
   RtpFrameObject* frame = frames_from_callback_.begin()->second.get();
   EXPECT_EQ(3, frame->times_nacked());
 }
 
 TEST_F(TestPacketBuffer, FrameSize) {
-  uint16_t seq_num = Rand();
+  const uint16_t seq_num = Rand();
   uint8_t data[] = {1, 2, 3, 4, 5};
 
-  InsertPacket(seq_num, kKeyFrame, kFirst, kNotLast, 5, data);
+  EXPECT_TRUE(Insert(seq_num, kKeyFrame, kFirst, kNotLast, 5, data));
-  InsertPacket(seq_num + 1, kKeyFrame, kNotFirst, kNotLast, 5, data);
+  EXPECT_TRUE(Insert(seq_num + 1, kKeyFrame, kNotFirst, kNotLast, 5, data));
-  InsertPacket(seq_num + 2, kKeyFrame, kNotFirst, kNotLast, 5, data);
+  EXPECT_TRUE(Insert(seq_num + 2, kKeyFrame, kNotFirst, kNotLast, 5, data));
-  InsertPacket(seq_num + 3, kKeyFrame, kNotFirst, kLast, 5, data);
+  EXPECT_TRUE(Insert(seq_num + 3, kKeyFrame, kNotFirst, kLast, 5, data));
 
   ASSERT_EQ(1UL, frames_from_callback_.size());
   EXPECT_EQ(20UL, frames_from_callback_.begin()->second->size());
 }
 
 TEST_F(TestPacketBuffer, ExpandBuffer) {
-  uint16_t seq_num = Rand();
+  const uint16_t seq_num = Rand();
 
   for (int i = 0; i < kStartSize + 1; ++i) {
-    InsertPacket(seq_num + i, kKeyFrame, kFirst, kLast);
+    EXPECT_TRUE(Insert(seq_num + i, kKeyFrame, kFirst, kLast));
   }
 }
 
+TEST_F(TestPacketBuffer, SingleFrameExpandsBuffer) {
+  const uint16_t seq_num = Rand();
+
+  EXPECT_TRUE(Insert(seq_num, kKeyFrame, kFirst, kNotLast));
+  for (int i = 1; i < kStartSize; ++i)
+    EXPECT_TRUE(Insert(seq_num + i, kKeyFrame, kNotFirst, kNotLast));
+  EXPECT_TRUE(Insert(seq_num + kStartSize, kKeyFrame, kNotFirst, kLast));
+
+  ASSERT_EQ(1UL, frames_from_callback_.size());
+  CheckFrame(seq_num);
+}
+
 TEST_F(TestPacketBuffer, ExpandBufferOverflow) {
-  uint16_t seq_num = Rand();
+  const uint16_t seq_num = Rand();
 
-  for (int i = 0; i < kMaxSize; ++i) {
+  for (int i = 0; i < kMaxSize; ++i)
-    InsertPacket(seq_num + i, kKeyFrame, kFirst, kLast);
+    EXPECT_TRUE(Insert(seq_num + i, kKeyFrame, kFirst, kLast));
-  }
+  EXPECT_FALSE(Insert(seq_num + kMaxSize + 1, kKeyFrame, kFirst, kLast));
-
-  VCMPacket packet;
-  packet.seqNum = seq_num + kMaxSize + 1;
-  EXPECT_FALSE(packet_buffer_->InsertPacket(packet));
 }
 
 TEST_F(TestPacketBuffer, OnePacketOneFrame) {
-  uint16_t seq_num = Rand();
+  const uint16_t seq_num = Rand();
-  InsertPacket(seq_num, kKeyFrame, kFirst, kLast);
+  EXPECT_TRUE(Insert(seq_num, kKeyFrame, kFirst, kLast));
   ASSERT_EQ(1UL, frames_from_callback_.size());
   CheckFrame(seq_num);
 }
 
 TEST_F(TestPacketBuffer, TwoPacketsTwoFrames) {
-  uint16_t seq_num = Rand();
+  const uint16_t seq_num = Rand();
 
-  InsertPacket(seq_num, kKeyFrame, kFirst, kLast);
+  EXPECT_TRUE(Insert(seq_num, kKeyFrame, kFirst, kLast));
-  InsertPacket(seq_num + 1, kKeyFrame, kFirst, kLast);
+  EXPECT_TRUE(Insert(seq_num + 1, kKeyFrame, kFirst, kLast));
 
   EXPECT_EQ(2UL, frames_from_callback_.size());
   CheckFrame(seq_num);
@@ -187,75 +223,33 @@ TEST_F(TestPacketBuffer, TwoPacketsTwoFrames) {
 }
 
 TEST_F(TestPacketBuffer, TwoPacketsOneFrames) {
-  uint16_t seq_num = Rand();
+  const uint16_t seq_num = Rand();
 
-  InsertPacket(seq_num, kKeyFrame, kFirst, kNotLast);
+  EXPECT_TRUE(Insert(seq_num, kKeyFrame, kFirst, kNotLast));
-  InsertPacket(seq_num + 1, kKeyFrame, kNotFirst, kLast);
+  EXPECT_TRUE(Insert(seq_num + 1, kKeyFrame, kNotFirst, kLast));
 
   EXPECT_EQ(1UL, frames_from_callback_.size());
   CheckFrame(seq_num);
 }
 
 TEST_F(TestPacketBuffer, ThreePacketReorderingOneFrame) {
-  uint16_t seq_num = Rand();
+  const uint16_t seq_num = Rand();
 
-  InsertPacket(seq_num, kKeyFrame, kFirst, kNotLast);
+  EXPECT_TRUE(Insert(seq_num, kKeyFrame, kFirst, kNotLast));
-  InsertPacket(seq_num + 2, kKeyFrame, kNotFirst, kLast);
+  EXPECT_TRUE(Insert(seq_num + 2, kKeyFrame, kNotFirst, kLast));
-  InsertPacket(seq_num + 1, kKeyFrame, kNotFirst, kNotLast);
+  EXPECT_TRUE(Insert(seq_num + 1, kKeyFrame, kNotFirst, kNotLast));
 
   EXPECT_EQ(1UL, frames_from_callback_.size());
   CheckFrame(seq_num);
 }
 
-TEST_F(TestPacketBuffer, DiscardOldPacket) {
-  VCMPacket packet;
-  packet.seqNum = Rand();
-  EXPECT_TRUE(packet_buffer_->InsertPacket(packet));
-  packet.seqNum += 2;
-  EXPECT_TRUE(packet_buffer_->InsertPacket(packet));
-
-  for (int i = 3; i < kMaxSize; ++i) {
-    ++packet.seqNum;
-    EXPECT_TRUE(packet_buffer_->InsertPacket(packet));
-  }
-
-  ++packet.seqNum;
-  EXPECT_FALSE(packet_buffer_->InsertPacket(packet));
-  packet_buffer_->ClearTo(packet.seqNum + 1);
-  EXPECT_TRUE(packet_buffer_->InsertPacket(packet));
-}
-
-TEST_F(TestPacketBuffer, DiscardMultipleOldPackets) {
-  uint16_t seq_num = Rand();
-  VCMPacket packet;
-  packet.seqNum = seq_num;
-  EXPECT_TRUE(packet_buffer_->InsertPacket(packet));
-  packet.seqNum += 2;
-  EXPECT_TRUE(packet_buffer_->InsertPacket(packet));
-
-  for (int i = 3; i < kMaxSize; ++i) {
-    ++packet.seqNum;
-    EXPECT_TRUE(packet_buffer_->InsertPacket(packet));
-  }
-
-  packet_buffer_->ClearTo(seq_num + 15);
-  for (int i = 0; i < 15; ++i) {
-    ++packet.seqNum;
-    EXPECT_TRUE(packet_buffer_->InsertPacket(packet));
-  }
-  for (int i = 15; i < kMaxSize; ++i) {
-    ++packet.seqNum;
-    EXPECT_FALSE(packet_buffer_->InsertPacket(packet));
-  }
-}
-
 TEST_F(TestPacketBuffer, Frames) {
-  uint16_t seq_num = Rand();
+  const uint16_t seq_num = Rand();
 
-  InsertPacket(seq_num, kKeyFrame, kFirst, kLast);
+  EXPECT_TRUE(Insert(seq_num, kKeyFrame, kFirst, kLast));
-  InsertPacket(seq_num + 1, kDeltaFrame, kFirst, kLast);
+  EXPECT_TRUE(Insert(seq_num + 1, kDeltaFrame, kFirst, kLast));
-  InsertPacket(seq_num + 2, kDeltaFrame, kFirst, kLast);
+  EXPECT_TRUE(Insert(seq_num + 2, kDeltaFrame, kFirst, kLast));
-  InsertPacket(seq_num + 3, kDeltaFrame, kFirst, kLast);
+  EXPECT_TRUE(Insert(seq_num + 3, kDeltaFrame, kFirst, kLast));
 
   ASSERT_EQ(4UL, frames_from_callback_.size());
   CheckFrame(seq_num);
@@ -264,13 +258,45 @@ TEST_F(TestPacketBuffer, Frames) {
   CheckFrame(seq_num + 3);
 }
 
-TEST_F(TestPacketBuffer, FramesReordered) {
+TEST_F(TestPacketBuffer, ClearSinglePacket) {
-  uint16_t seq_num = Rand();
+  const uint16_t seq_num = Rand();
 
-  InsertPacket(seq_num + 1, kDeltaFrame, kFirst, kLast);
+  for (int i = 0; i < kMaxSize; ++i)
-  InsertPacket(seq_num, kKeyFrame, kFirst, kLast);
+    EXPECT_TRUE(Insert(seq_num + i, kDeltaFrame, kFirst, kLast));
-  InsertPacket(seq_num + 3, kDeltaFrame, kFirst, kLast);
+
-  InsertPacket(seq_num + 2, kDeltaFrame, kFirst, kLast);
+  packet_buffer_->ClearTo(seq_num);
+  EXPECT_TRUE(Insert(seq_num + kMaxSize, kDeltaFrame, kFirst, kLast));
+}
+
+TEST_F(TestPacketBuffer, OneIncompleteFrame) {
+  const uint16_t seq_num = Rand();
+
+  EXPECT_TRUE(Insert(seq_num, kDeltaFrame, kFirst, kNotLast));
+  EXPECT_TRUE(Insert(seq_num + 1, kDeltaFrame, kNotFirst, kLast));
+  EXPECT_TRUE(Insert(seq_num - 1, kDeltaFrame, kNotFirst, kLast));
+
+  ASSERT_EQ(1UL, frames_from_callback_.size());
+  CheckFrame(seq_num);
+}
+
+TEST_F(TestPacketBuffer, TwoIncompleteFramesFullBuffer) {
+  const uint16_t seq_num = Rand();
+
+  for (int i = 1; i < kMaxSize - 1; ++i)
+    EXPECT_TRUE(Insert(seq_num + i, kDeltaFrame, kNotFirst, kNotLast));
+  EXPECT_TRUE(Insert(seq_num, kDeltaFrame, kFirst, kNotLast));
+  EXPECT_TRUE(Insert(seq_num - 1, kDeltaFrame, kNotFirst, kLast));
+
+  ASSERT_EQ(0UL, frames_from_callback_.size());
+}
+
+TEST_F(TestPacketBuffer, FramesReordered) {
+  const uint16_t seq_num = Rand();
+
+  EXPECT_TRUE(Insert(seq_num + 1, kDeltaFrame, kFirst, kLast));
+  EXPECT_TRUE(Insert(seq_num, kKeyFrame, kFirst, kLast));
+  EXPECT_TRUE(Insert(seq_num + 3, kDeltaFrame, kFirst, kLast));
+  EXPECT_TRUE(Insert(seq_num + 2, kDeltaFrame, kFirst, kLast));
 
   ASSERT_EQ(4UL, frames_from_callback_.size());
   CheckFrame(seq_num);
@@ -289,14 +315,15 @@ TEST_F(TestPacketBuffer, GetBitstreamFromFrame) {
   uint8_t
       result[sizeof(many) + sizeof(bitstream) + sizeof(such) + sizeof(data)];
 
-  uint16_t seq_num = Rand();
+  const uint16_t seq_num = Rand();
 
-  InsertPacket(seq_num, kKeyFrame, kFirst, kNotLast, sizeof(many), many);
+  EXPECT_TRUE(Insert(seq_num, kKeyFrame, kFirst, kNotLast, sizeof(many), many));
-  InsertPacket(seq_num + 1, kDeltaFrame, kNotFirst, kNotLast, sizeof(bitstream),
+  EXPECT_TRUE(Insert(seq_num + 1, kDeltaFrame, kNotFirst, kNotLast,
-               bitstream);
+                     sizeof(bitstream), bitstream));
-  InsertPacket(seq_num + 2, kDeltaFrame, kNotFirst, kNotLast, sizeof(such),
+  EXPECT_TRUE(Insert(seq_num + 2, kDeltaFrame, kNotFirst, kNotLast,
-               such);
+                     sizeof(such), such));
-  InsertPacket(seq_num + 3, kDeltaFrame, kNotFirst, kLast, sizeof(data), data);
+  EXPECT_TRUE(
+      Insert(seq_num + 3, kDeltaFrame, kNotFirst, kLast, sizeof(data), data));
 
   ASSERT_EQ(1UL, frames_from_callback_.size());
   CheckFrame(seq_num);
@@ -305,50 +332,48 @@ TEST_F(TestPacketBuffer, GetBitstreamFromFrame) {
 }
 
 TEST_F(TestPacketBuffer, FreeSlotsOnFrameDestruction) {
-  uint16_t seq_num = Rand();
+  const uint16_t seq_num = Rand();
 
-  InsertPacket(seq_num, kKeyFrame, kFirst, kNotLast);
+  EXPECT_TRUE(Insert(seq_num, kKeyFrame, kFirst, kNotLast));
-  InsertPacket(seq_num + 1, kDeltaFrame, kNotFirst, kNotLast);
+  EXPECT_TRUE(Insert(seq_num + 1, kDeltaFrame, kNotFirst, kNotLast));
-  InsertPacket(seq_num + 2, kDeltaFrame, kNotFirst, kLast);
+  EXPECT_TRUE(Insert(seq_num + 2, kDeltaFrame, kNotFirst, kLast));
   EXPECT_EQ(1UL, frames_from_callback_.size());
   CheckFrame(seq_num);
 
   frames_from_callback_.clear();
 
   // Insert frame that fills the whole buffer.
-  InsertPacket(seq_num + 3, kKeyFrame, kFirst, kNotLast);
+  EXPECT_TRUE(Insert(seq_num + 3, kKeyFrame, kFirst, kNotLast));
   for (int i = 0; i < kMaxSize - 2; ++i)
-    InsertPacket(seq_num + i + 4, kDeltaFrame, kNotFirst, kNotLast);
+    EXPECT_TRUE(Insert(seq_num + i + 4, kDeltaFrame, kNotFirst, kNotLast));
-  InsertPacket(seq_num + kMaxSize + 2, kKeyFrame, kNotFirst, kLast);
+  EXPECT_TRUE(Insert(seq_num + kMaxSize + 2, kKeyFrame, kNotFirst, kLast));
   EXPECT_EQ(1UL, frames_from_callback_.size());
   CheckFrame(seq_num + 3);
 }
 
 TEST_F(TestPacketBuffer, Clear) {
-  uint16_t seq_num = Rand();
+  const uint16_t seq_num = Rand();
 
-  InsertPacket(seq_num, kKeyFrame, kFirst, kNotLast);
+  EXPECT_TRUE(Insert(seq_num, kKeyFrame, kFirst, kNotLast));
-  InsertPacket(seq_num + 1, kDeltaFrame, kNotFirst, kNotLast);
+  EXPECT_TRUE(Insert(seq_num + 1, kDeltaFrame, kNotFirst, kNotLast));
-  InsertPacket(seq_num + 2, kDeltaFrame, kNotFirst, kLast);
+  EXPECT_TRUE(Insert(seq_num + 2, kDeltaFrame, kNotFirst, kLast));
   EXPECT_EQ(1UL, frames_from_callback_.size());
   CheckFrame(seq_num);
 
   packet_buffer_->Clear();
 
-  InsertPacket(seq_num + kStartSize, kKeyFrame, kFirst, kNotLast);
+  EXPECT_TRUE(Insert(seq_num + kStartSize, kKeyFrame, kFirst, kNotLast));
-  InsertPacket(seq_num + kStartSize + 1, kDeltaFrame, kNotFirst, kNotLast);
+  EXPECT_TRUE(
-  InsertPacket(seq_num + kStartSize + 2, kDeltaFrame, kNotFirst, kLast);
+      Insert(seq_num + kStartSize + 1, kDeltaFrame, kNotFirst, kNotLast));
+  EXPECT_TRUE(Insert(seq_num + kStartSize + 2, kDeltaFrame, kNotFirst, kLast));
   EXPECT_EQ(2UL, frames_from_callback_.size());
   CheckFrame(seq_num + kStartSize);
 }
 
 TEST_F(TestPacketBuffer, InvalidateFrameByClearing) {
-  VCMPacket packet;
+  const uint16_t seq_num = Rand();
-  packet.frameType = kVideoFrameKey;
+
-  packet.isFirstPacket = true;
+  EXPECT_TRUE(Insert(seq_num, kKeyFrame, kFirst, kLast));
-  packet.markerBit = true;
-  packet.seqNum = Rand();
-  EXPECT_TRUE(packet_buffer_->InsertPacket(packet));
   ASSERT_EQ(1UL, frames_from_callback_.size());
 
   packet_buffer_->Clear();