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

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 24 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),
+      has_cleared_to_(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() {}

[danilchap, 2016/10/07 18:25:37]

call Clear?

[philipel, 2016/10/18 12:22:19]

Done.

 
 bool PacketBuffer::InsertPacket(const VCMPacket& packet) {
   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 - 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 (has_cleared_to_)
+      return false;
+
+    first_seq_num_ = seq_num;
   }
 
   if (sequence_buffer_[index].used) {
     // Duplicate packet, do nothing.
     if (data_buffer_[index].seqNum == packet.seqNum)
       return true;

[danilchap, 2016/10/07 18:25:37]

what does return false mean for this function?
if you insert duplicate packet, it would return true if packet wasn't handled
and cleaned and 'false' otherwise?

[philipel, 2016/10/18 12:22:19]

If |packet| is in the PacketBuffer when this function returns, true, otherwise
false.

[danilchap, 2016/10/19 09:25:10]

Probably would help to put this comment near InsertPacket declaration in the .h

[philipel, 2016/10/19 13:08:50]

Done.

 
     // The packet buffer is full, try to expand the buffer.
     while (ExpandBufferSize() && sequence_buffer_[seq_num % size_].used) {
     }
     index = seq_num % size_;
 
     // Packet buffer is still full.
     if (sequence_buffer_[index].used)
       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;
 
   // Since the data pointed to by |packet.dataPtr| is non-persistent the
   // data has to be copied to its own buffer.
   // TODO(philipel): Take ownership instead of copying payload when
   //                 bitstream-fixing has been implemented.
   if (packet.sizeBytes) {

[danilchap, 2016/10/07 18:25:37]

if packet.sizeBytes == 0, you will not allocate buffer, but because used = true
might try to delete it later.
since in Clear() you do not reset dataPtr to nullptr, it might create an error

[philipel, 2016/10/18 12:22:19]

I now set |packet.dataPtr| to nullptr in Clear().

     uint8_t* payload = new uint8_t[packet.sizeBytes];
     memcpy(payload, packet.dataPtr, packet.sizeBytes);
     data_buffer_[index].dataPtr = payload;
   }
 
   FindFrames(seq_num);
   return true;
 }
 
 void PacketBuffer::ClearTo(uint16_t seq_num) {

[danilchap, 2016/10/07 18:25:37]

when this function is called? juts tests for now?

[philipel, 2016/10/18 12:22:19]

For now, just in tests, but it will be used for the new jitter buffer.

   rtc::CritScope lock(&crit_);
+
+  // If the packet buffer was cleared between a frame was created and returned.
+  if (!first_packet_received_)
+    return;
+
+  has_cleared_to_ = true;
   size_t index = first_seq_num_ % size_;

[danilchap, 2016/10/07 18:25:37]

may be move this statement inside while loop.
no need for 2 increment statements then.

[philipel, 2016/10/18 12:22:19]

Done.

-  while (AheadOf<uint16_t>(seq_num, first_seq_num_ + 1)) {
-    index = (index + 1) % size_;
-    ++first_seq_num_;
+  while (AheadOrAt<uint16_t>(seq_num, first_seq_num_)) {
     delete[] data_buffer_[index].dataPtr;
     data_buffer_[index].dataPtr = nullptr;
     sequence_buffer_[index].used = false;
+    index = (index + 1) % size_;
+    ++first_seq_num_;
   }
 }
 
+void PacketBuffer::Clear() {
+  rtc::CritScope lock(&crit_);
+  for (size_t i = 0; i < size_; ++i) {
+    if (sequence_buffer_[i].used)
+      delete[] data_buffer_[i].dataPtr;
+    sequence_buffer_[i].used = false;
+  }
+
+  first_packet_received_ = false;
+  has_cleared_to_ = 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);
   std::vector<ContinuityInfo> new_sequence_buffer(new_size);
   for (size_t i = 0; i < size_; ++i) {
     if (sequence_buffer_[i].used) {
       size_t index = sequence_buffer_[i].seq_num % new_size;
       new_sequence_buffer[index] = sequence_buffer_[i];
       new_data_buffer[index] = data_buffer_[i];
     }
   }
   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;
 
   if (!sequence_buffer_[index].used)
     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))) {
     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) {

[danilchap, 2016/10/07 18:25:37]

add some tests to check seq_num = 0 wraps properly.

[philipel, 2016/10/18 12:22:19]

Done.

     return false;
+  }
   if (sequence_buffer_[prev_index].continuous)
     return true;
 
   return false;
 }
 
 void PacketBuffer::FindFrames(uint16_t seq_num) {
   size_t index = seq_num % size_;
-  while (IsContinuous(seq_num)) {
+  while (PotentialNewFrame(seq_num)) {
     sequence_buffer_[index].continuous = true;
 
     // If all packets of the frame is continuous, find the first packet of the
     // frame and create an RtpFrameObject.
     if (sequence_buffer_[index].frame_end) {
       size_t frame_size = 0;
       int max_nack_count = -1;
       uint16_t start_seq_num = seq_num;
 
       // Find the start index by searching backward until the packet with
@@ skipping 32 matching lines @@
   while (index != end) {
     if (sequence_buffer_[index].seq_num == seq_num) {
       delete[] data_buffer_[index].dataPtr;
       data_buffer_[index].dataPtr = nullptr;
       sequence_buffer_[index].used = false;
     }
 
     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,
                                 uint8_t* destination) {
   rtc::CritScope lock(&crit_);
 
   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();
   while (index != end) {
@@ skipping 15 matching lines @@
 VCMPacket* PacketBuffer::GetPacket(uint16_t seq_num) {
   rtc::CritScope lock(&crit_);
   size_t index = seq_num % size_;
   if (!sequence_buffer_[index].used ||
       seq_num != sequence_buffer_[index].seq_num) {
     return nullptr;
   }
   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_);
 }
 
 int PacketBuffer::Release() const {
   int count = rtc::AtomicOps::Decrement(&ref_count_);
   if (!count) {
     delete this;
   }
   return count;
 }
 
 }  // namespace video_coding
 }  // namespace webrtc