Use timestamp instead of seq_num to distinguish between packets.

In the case a frame_object is kept for some time before it is deleted,
it may happend that a new frame is received with overlapping sequence
numbers. If the old frame_object is removed while receiving the new
frame there used to be a crash.

Bug: webrtc:9629
Change-Id: I270a8caa2b58b73c000542aa504c0ebe277d49c4
Reviewed-on: https://webrtc-review.googlesource.com/102683
Reviewed-by: Philip Eliasson <philipel@webrtc.org>
Commit-Queue: Johannes Kron <kron@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#24914}

modules/video_coding/packet_buffer.cc

@@ -398,8 +398,12 @@ void PacketBuffer::ReturnFrame(RtpFrameObject* frame) {
   size_t index = frame->first_seq_num() % size_;
   size_t end = (frame->last_seq_num() + 1) % size_;
   uint16_t seq_num = frame->first_seq_num();
+  uint32_t timestamp = frame->Timestamp();
   while (index != end) {
-    if (sequence_buffer_[index].seq_num == seq_num) {
+    // Check both seq_num and timestamp to handle the case when seq_num wraps
+    // around too quickly for high packet rates.
+    if (sequence_buffer_[index].seq_num == seq_num &&
+        data_buffer_[index].timestamp == timestamp) {
       delete[] data_buffer_[index].dataPtr;
       data_buffer_[index].dataPtr = nullptr;
       sequence_buffer_[index].used = false;
@@ -417,11 +421,15 @@ bool PacketBuffer::GetBitstream(const RtpFrameObject& frame,
   size_t index = frame.first_seq_num() % size_;
   size_t end = (frame.last_seq_num() + 1) % size_;
   uint16_t seq_num = frame.first_seq_num();
+  uint32_t timestamp = frame.Timestamp();
   uint8_t* destination_end = destination + frame.size();
 
   do {
+    // Check both seq_num and timestamp to handle the case when seq_num wraps
+    // around too quickly for high packet rates.
     if (!sequence_buffer_[index].used ||
-        sequence_buffer_[index].seq_num != seq_num) {
+        sequence_buffer_[index].seq_num != seq_num ||
+        data_buffer_[index].timestamp != timestamp) {
       return false;
     }
 

modules/video_coding/video_packet_buffer_unittest.cc

@@ -82,6 +82,14 @@ class TestPacketBuffer : public ::testing::Test,
         << ".";
   }
 
+  void DeleteFrame(uint16_t first_seq_num) {
+    auto frame_it = frames_from_callback_.find(first_seq_num);
+    ASSERT_FALSE(frame_it == frames_from_callback_.end())
+        << "Could not find frame with first sequence number " << first_seq_num
+        << ".";
+    frames_from_callback_.erase(frame_it);
+  }
+
   static constexpr int kStartSize = 16;
   static constexpr int kMaxSize = 64;
 
@@ -494,6 +502,39 @@ TEST_F(TestPacketBuffer, GetBitstreamOneFrameFullBuffer) {
   EXPECT_EQ(memcmp(result, expected, kStartSize), 0);
 }
 
+TEST_F(TestPacketBuffer, InsertPacketAfterOldFrameObjectIsRemoved) {
+  uint16_t kFirstSeqNum = 0;
+  uint32_t kTimestampDelta = 100;
+  uint32_t timestamp = 10000;
+  uint16_t seq_num = kFirstSeqNum;
+
+  // Loop until seq_num wraps around.
+  SeqNumUnwrapper<uint16_t> unwrapper(0);
+  while (unwrapper.Unwrap(seq_num) < std::numeric_limits<uint16_t>::max()) {
+    Insert(seq_num++, kKeyFrame, kFirst, kNotLast, 0, nullptr, timestamp);
+    for (int i = 0; i < 5; ++i) {
+      Insert(seq_num++, kKeyFrame, kNotFirst, kNotLast, 0, nullptr, timestamp);
+    }
+    Insert(seq_num++, kKeyFrame, kNotFirst, kLast, 0, nullptr, timestamp);
+    timestamp += kTimestampDelta;
+  }
+
+  size_t number_of_frames = frames_from_callback_.size();
+  // Delete old frame object while receiving frame with overlapping sequence
+  // numbers.
+  Insert(seq_num++, kKeyFrame, kFirst, kNotLast, 0, nullptr, timestamp);
+  for (int i = 0; i < 5; ++i) {
+    Insert(seq_num++, kKeyFrame, kNotFirst, kNotLast, 0, nullptr, timestamp);
+  }
+  // Delete FrameObject connected to packets that have already been cleared.
+  DeleteFrame(kFirstSeqNum);
+  Insert(seq_num++, kKeyFrame, kNotFirst, kLast, 0, nullptr, timestamp);
+
+  // Regardless of the initial size, the number of frames should be constant
+  // after removing and then adding a new frame object.
+  EXPECT_EQ(number_of_frames, frames_from_callback_.size());
+}
+
 // If |sps_pps_idr_is_keyframe| is true, we require keyframes to contain
 // SPS/PPS/IDR and the keyframes we create as part of the test do contain
 // SPS/PPS/IDR. If |sps_pps_idr_is_keyframe| is false, we only require and