Don't detect a new frame if a previous packet is used in a previous frame.

webrtc/modules/video_coding/packet_buffer.cc

 /*
  *  Copyright (c) 2016 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.
  */
 
@@ skipping 22 matching lines @@
 }
 
 PacketBuffer::PacketBuffer(Clock* clock,
                            size_t start_buffer_size,
                            size_t max_buffer_size,
                            OnReceivedFrameCallback* received_frame_callback)
     : clock_(clock),
       size_(start_buffer_size),
       max_size_(max_buffer_size),
       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) {
   RTC_DCHECK_LE(start_buffer_size, max_buffer_size);
   // Buffer size must always be a power of 2.
   RTC_DCHECK((start_buffer_size & (start_buffer_size - 1)) == 0);
   RTC_DCHECK((max_buffer_size & (max_buffer_size - 1)) == 0);
 }
 
 PacketBuffer::~PacketBuffer() {
   Clear();
 }
 
 bool PacketBuffer::InsertPacket(VCMPacket* packet) {
   std::vector<std::unique_ptr<RtpFrameObject>> found_frames;
   {
     rtc::CritScope lock(&crit_);
     uint16_t seq_num = packet->seqNum;
     size_t index = seq_num % size_;
 
     if (!first_packet_received_) {
       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_) {
         delete[] packet->dataPtr;
         packet->dataPtr = nullptr;
         return false;
       }
 
@@ skipping 14 matching lines @@
       index = seq_num % size_;
 
       // Packet buffer is still full.
       if (sequence_buffer_[index].used) {
         delete[] packet->dataPtr;
         packet->dataPtr = nullptr;
         return false;
       }
     }
 
-    if (AheadOf(seq_num, last_seq_num_))
-      last_seq_num_ = seq_num;
-
     sequence_buffer_[index].frame_begin = packet->isFirstPacket;
     sequence_buffer_[index].frame_end = packet->markerBit;
     sequence_buffer_[index].seq_num = packet->seqNum;
     sequence_buffer_[index].continuous = false;
     sequence_buffer_[index].frame_created = false;
     sequence_buffer_[index].used = true;
     data_buffer_[index] = *packet;
     packet->dataPtr = nullptr;
 
     found_frames = FindFrames(seq_num);
@@ skipping 63 matching lines @@
   int prev_index = index > 0 ? index - 1 : size_ - 1;
 
   if (!sequence_buffer_[index].used)
     return false;
   if (sequence_buffer_[index].frame_created)
     return false;
   if (sequence_buffer_[index].frame_begin)
     return true;
   if (!sequence_buffer_[prev_index].used)
     return false;
+  if (sequence_buffer_[prev_index].frame_created)

[stefan-webrtc, 2017/01/10 11:50:00]

It's not clear to me why this is needed. Why can't it be a new frame in this
case? Is it because the previous packet created a new frame, and this packet
doesn't have the frame_begin flag set?

I also don't follow how the test added would capture this... :)

[philipel, 2017/01/10 12:03:36]

This check covers this case:

p1:
 seqnum 1
 first true
 marker true

p2:
 seqnum 2
 first false
 marker true

When we receive p1 it will be correctly detected as a complete frame. When we
receive p2 it will check if the previous packet is continuous, which it is, and
then try to create a new frame since this is the last packet of the frame.

Without this fix then a frame containing p1 and p2 would be created, even though
p1 is already part of a frame. With this fix we detect that p1 is already part
of a frame and should not be used to create a new frame.

+    return false;
   if (sequence_buffer_[prev_index].seq_num !=
       sequence_buffer_[index].seq_num - 1) {
     return false;
   }
   if (sequence_buffer_[prev_index].continuous)
     return true;
 
   return false;
 }
 
@@ skipping 94 matching lines @@
 int PacketBuffer::Release() const {
   int count = rtc::AtomicOps::Decrement(&ref_count_);
   if (!count) {
     delete this;
   }
   return count;
 }
 
 }  // namespace video_coding
 }  // namespace webrtc